Conversation
Skip non-diagnostic commands before accessing full-output files. Decode eligible logs in bounded chunks and retain oversize rejection even if closing the file fails. Cover early filtering, UTF-8 limits, growing files, cleanup errors, and fused output paths without changing Pi or dependencies.
gaoanze888
left a comment
There was a problem hiding this comment.
Reviewed exact head 93e8748bd189027334c48e8b943f226dbab3d092. Locally, npm ci --ignore-scripts, npm run check (19 files / 158 tests), node scripts/check-pi-compat.mjs, and git diff --check pass. The early command gate and bounded UTF-8 decoder are good, but the full-output path still has two fail-conservative integrity gaps:
-
Once a trusted
fullOutputPathis present,open/stat/read/closefailures can return the inline truncated preview wheneversourceOverLimitis still false. The new test explicitly locks inread failure -> "preview". If that preview exceedsminBytes, it can be archived and reduced even though the complete source was never read and may be far over the configured limit. With a trusted full-output indicator, inability to obtain the complete bounded source should returnundefined, preserving the original tool result, not make the preview eligible. -
EOF does not verify that the bytes read match the initial
stats.size. If a file is truncated during the read, the function accepts the partial prefix as complete. For example, an initial 100,000-byte file can pass the byte precheck, be truncated after the first 64 KiB read, then hit EOF and return that prefix even though the original source exceeded a 65,536-character limit. Please reject short reads relative to the opened handle’s initial size (and ideally document or check the stability assumption), with a truncation regression.
Both issues affect the core guarantee that only a complete, proven-under-limit full log enters reduction.
|
Addressed both points from the review in eba3d8e:
Regression tests reproduce your 100,000-byte file truncated after the first 65,536-byte read, and cover I/O failures, changes at EOF, and long Bash/Edit/Write previews. All 172 tests pass locally with Node 24.15.0 / Pi 0.84.2, along with typecheck, package dry-run, Pi compatibility, and whitespace checks. Isolated mutation checks confirm the tests fail when either reviewed defect is reintroduced. Could you take another look? |
gaoanze888
left a comment
There was a problem hiding this comment.
Re-reviewed exact head eba3d8eaac30000899af96ede9a8f6d065a94880. Both prior integrity gaps are closed: after a trusted full-output path is identified, validation/open/stat/read/final-stat/close failure now makes the preview ineligible, and EOF requires the bytes read to match the initial size plus stable size/mtime/ctime metadata. I also checked tri-state unsafe-path fallback, short reads with incremental decoding, truncation/growth/rewrite cases, and fused Bash/Edit/Write end-to-end rejection. Locally, npm ci --ignore-scripts, npm run check (19 files / 172 tests), node scripts/check-pi-compat.mjs, and git diff --check passed. No blocking issues found.
|
@gaoanze888 is not a member of the SoL-Pi maintenance team and is not authorized to speak on its behalf. Their reviews, approvals, change requests, and merge-order recommendations are independent contributor feedback, not maintainer decisions. Contributors are not required to act on those requests unless a maintainer confirms them. We have documented a pattern of batch posting: on September 16, six reviews were submitted between 11:15:45 and 11:15:46 China Standard Time (UTC+8), followed by three reviews at 16:44:41. Identical integration comments were also posted to three PRs within the same second. Several of your PR descriptions explicitly disclose AI-assisted implementation and review. While these records do not establish how much human review occurred before publication, they do establish batch posting that is creating a substantial maintenance burden and confusion about who represents the project. @gaoanze888, stop posting further reviews and coordination comments in this repository, and disable any automation that publishes them. Merge decisions and contributor coordination are handled by the maintenance team. Continued posting may result in blocking. |
Problem and change
EPR read complete Pi bash logs before checking whether the command was diagnostic or the source exceeded its character limit, so outputs that would be skipped still incurred full-file I/O and allocation.
This change filters commands before filesystem access and reads eligible full logs in bounded chunks. Incremental UTF-8 decoding preserves the existing UTF-16 character limit, with a separate byte cap even if the file grows after the initial size check. Files whose size already proves they exceed the limit are rejected without reading their contents.
For trusted full-output paths, path-validation or open/stat/read/close failures reject reduction instead of making the truncated preview eligible. At EOF, the byte count must match the opened handle's initial size; a second stat checks size, modification time, and change time. Incomplete reads and observed file changes preserve the original tool result before archival or model access. The fallback journal reason now accounts for unavailable sources as well as oversized ones.
Scope and trade-offs
The benefit is reduced unnecessary disk I/O and memory allocation, with potential latency improvement; no direct token-saving claim is made. Pi logs are expected to be finalized before tool_result. Metadata checks detect observed changes but do not provide an atomic snapshot. Existing behavior for explicitly unsafe paths is preserved. No new configuration, dependencies, or Pi core changes are introduced.
Validation
Current revision, Node 24.15.0 and pinned Pi 0.84.2:
npm run check: typecheck, 172 tests across 19 files, and package dry-run passed.node scripts/check-pi-compat.mjsandgit diff --check: passed.These are local validation results. The project has no lint script or ESLint dependency.
Checklist