Skip to content

fix(reducer): bound diagnostic log reads before reduction - #64

Open
hi-neason wants to merge 2 commits into
NVlabs:mainfrom
hi-neason:fix/bound-reducer-log-reads
Open

hi-neason wants to merge 2 commits into
NVlabs:mainfrom
hi-neason:fix/bound-reducer-log-reads

Conversation

@hi-neason

@hi-neason hi-neason commented Sep 15, 2026

Copy link
Copy Markdown

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.
  • Targeted candidate and reducer tests: 48 passed.
  • node scripts/check-pi-compat.mjs and git diff --check: passed.
  • Regression coverage includes the reviewer's 100,000-byte file truncated after the first 65,536-byte read; open/stat/read/close failures; final stat failure; growth, truncation, and same-size rewriting at EOF; normal short reads; and Bash/Edit/Write long previews that must not reach archival or model access. Fused Edit/Write tests obtain the path from the actual full-output footer format.
  • Existing coverage retains non-diagnostic zero-I/O checks, byte bounds, exact-limit and over-limit UTF-8 inputs, and handle cleanup.
  • In isolated mutation checks, restoring preview fallback caused 10 test failures; removing EOF integrity checks caused 5 test failures, including the reported truncation case.

These are local validation results. The project has no lint script or ESLint dependency.

Checklist

  • Uses Pi public extension APIs without modifying Pi core.
  • Regression tests and reproducible validation included.
  • Resource-efficiency benefit and trade-offs described without unsupported token-saving claims.
  • Existing configuration and dependency set preserved.

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 gaoanze888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Once a trusted fullOutputPath is present, open/stat/read/close failures can return the inline truncated preview whenever sourceOverLimit is still false. The new test explicitly locks in read failure -> "preview". If that preview exceeds minBytes, 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 return undefined, preserving the original tool result, not make the preview eligible.

  2. 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.

@hi-neason

Copy link
Copy Markdown
Author

Addressed both points from the review in eba3d8e:

  1. Trusted full-output path validation and open/stat/read/close failures now return undefined, so the inline preview cannot reach archival or reduction. This also covers close failures after a successful read.
  2. EOF now requires the total bytes read to equal the opened handle's initial size. A final stat also checks size, mtime, and ctime to reject observed changes. The finalized-log assumption and the lack of atomic snapshot guarantees are documented in the code.

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 gaoanze888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Owen718

Owen718 commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

@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.

This was referenced Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants