Repository navigation
fix(ci): read the changed-path list past the first pipe buffer - #19409
Conversation
`pr-code-change-scope.mjs` read stdin with `readFileSync(0, 'utf8')`, a single read of fd 0. Once the writer outgrows the 64 KB pipe buffer that read returns early or throws EAGAIN, the script exits 0 having emitted no `name=value` pairs, and `tee -a "$GITHUB_OUTPUT"` records nothing -- so every lane the classifier gates is silently skipped rather than failing loudly. A PR opened long ago carries a stale `pull_request.base.sha`, so the gate's merge-base diff spans the whole base branch. PR #13178 diffed 13,294 files (773 KB) against a base 1,592 commits behind main and lost typecheck, test, static analysis, xterm patch sync, package and e2e to this. Stream stdin instead, matching how the sibling `pr-e2e-source-routing.mjs` already reads the same list in the same workflow.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
pr-code-change-scope.mjsstreams stdin instead ofreadFileSync(0)— the CLI block now consumesprocess.stdinwithfor await, eliminating the early-read/EAGAINfailure when the changed-path list outgrows a single pipe buffer. Matches the siblingpr-e2e-source-routing.mjsidiom for the same list.- New regression test over a real slow-writer pipe —
classifies a path that arrives after the first pipe bufferspawns the script with >64 KB written in chunks and the sole code-relevant path last, asserting exit 0, noEAGAIN/EPIPE, and correct classification. Exact final-state assertions, so it is deterministic for the fixed code. - Unused
node:fsimport removed;split('\n')tightened tosplit(/\r?\n/)for CRLF robustness.
The fix is correctly scoped: the streaming conversion is the established pattern the sibling script already uses, no classification logic or workflow YAML changed, and the remaining readFileSync(0) call site in this path is gone. The stale-base.sha over-triggering is explicitly deferred and fails safe (runs more, not fewer), so there is nothing blocking here.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe CLI now reads stdin asynchronously in UTF-8 chunks. It supports CRLF input and removes empty lines before classification. The test suite adds an async spawned-process test that sends more than 64 KiB of delayed path input. The test verifies successful classification and confirms that the process produces no Merge Risk: ⚪ Minimal · up to This change prevents large changed-path lists from being truncated or skipped in PR job classification, while preserving existing path classification behavior. The large-input regression coverage indicates no current merge-blocking risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed, relevant, and explains the problem, implementation, evidence, and testing. However, it omits the required Linked Issue section, the required Visual Proof declaration for a non-UI change, and several template checklist sections.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ELI5
The PR Checks gate reads its list of changed files from stdin with one
readFileSync(0). When that list is bigger than a pipe buffer, the read returns early, the script exits 0 having printed nothing, and every lane it gates gets silently skipped instead of failing. Stream stdin instead.Why now
PR #13178 hit this. GitHub gave it a
pull_request.base.shaofbb09dc17— 1,592 commits behind current main, because the PR was opened long ago and later refreshed onto main. The gate's merge-base diff therefore produced 13,294 files / 773,461 bytes instead of that PR's actual 4 files, and blew past the 64 KB pipe buffer.Result on #13178:
detect code-relevant changesfailed on all 3 attempts (never a flake),verifycascaded (CODE_PATHS: failure), and these 16 lanes were skipped, not run:The remaining green checks came from unrelated workflows, so the PR read as "mostly green" while none of its own validation had executed. That silent-skip is the part worth fixing: the failure mode is invisible rather than loud.
What changed
config/scripts/pr-code-change-scope.mjsnow streamsprocess.stdin— the exact pattern its siblingconfig/scripts/pr-e2e-source-routing.mjsalready uses for the same list in the same workflow step. No classification logic changed; no workflow YAML changed.Evidence
Threshold bisected on the real failing input — a clean pipe-buffer boundary:
EAGAIN: resource temporarily unavailable, readEAGAINAfter the fix, the 773 KB input classifies correctly. The real 4-file #13178 diff and the docs-only short-circuit (
README.md→should_run=false) classify byte-identically to before.Testing
classifies a path that arrives after the first pipe buffer: feeds >64 KB through a real pipe with a slow writer, with the only code-relevant path placed last. Verified red against the old script (expected 'node:fs:441...' not to contain 'EAGAIN') and green after.spawn, not a shell pipeline, so it runs on the Windows lane too. NotespawnSync({input})does not reproduce the bug (it hands the child a blocking fd 0); the slow-writer pipe does.config/scripts/pr-code-change-scope.test.mjs31/31;pr-e2e-source-routing,pr-workflow-lint-parity,pr-e2e-gate-contract,pr-workflow-parallelismpass.Follow-up not in this PR
This makes the gate robust to a large list; it does not make the list correct. A stale
base.shastill over-triggers lanes (safe direction — it runs more, not fewer). Narrowing the diff to the true merge base against the base branch tip is a separate change.