test(config): stop a broken pipe preempting the exit-code diagnosis - #6455
test(config): stop a broken pipe preempting the exit-code diagnosis#6455johnstonmatt wants to merge 1 commit into
Conversation
`git diff-tree --stdin` given a cwd outside any repository exits before it reads a hash, so the write that follows can land on a dead process and raise `EPIPE: broken pipe, send` instead of the exit-code error the caller reports. Which one surfaces is a race against process startup — green on an idle machine, red on a loaded CI runner. The exit code and stderr are the diagnosis, so a broken pipe on stdin is dropped and the reporting left to them.
…gnosis" This reverts b772491. The fix is sound but has nothing to do with workers logs; it landed here to stabilise this branch's CI. Moved to #6455 off develop so it does not merge or revert with the workers work. Raised in review on #6410. Until #6455 lands, the `packages/config` release-script test it stabilises can flake on a loaded runner.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Review
Both reviews completed: Claude reported five findings and Codex reported none. One pre-existing sibling EPIPE-handling gap is confirmed, one hypothetical EPIPE-plus-zero-exit completeness risk remains uncertain, and three findings are refuted. No confirmed defect was found in the PR's changed behavior itself.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | packages/config/scripts/semantic-release-path-filter.ts:133 |
error-handling |
claude | A caught EPIPE followed by a zero git exit code could allow parsing incomplete stdout without detecting truncated input. |
| 🟡 MINOR | packages/config/scripts/build.ts:74 |
error-handling |
claude | renderJsonSchema has the same unguarded subprocess-stdin write pattern, so an early oxfmt exit can surface a broken-pipe error before its exit-code and stderr diagnosis. |
Findings outside the diff
- 🟡 MINOR
packages/config/scripts/build.ts:74— renderJsonSchema has the same unguarded subprocess-stdin write pattern, so an early oxfmt exit can surface a broken-pipe error before its exit-code and stderr diagnosis.
Refuted findings (kept for transparency, not posted as review comments)
packages/config/scripts/semantic-release-path-filter.ts:132(resource-cleanup): The catch branches leave the subprocess and stream-reader promises uncleaned, potentially leaking resources or causing unhandled rejections.
Refuted: A recognized EPIPE continues to line 139, which awaits proc.exited, stdoutText, and stderrText; the reader has already closed the pipe, so skipping stdin.end does not leave git waiting for EOF. The non-EPIPE rethrow behavior already existed before this PR, and the code provides no evidence that either reader rejects unhandled.packages/config/scripts/semantic-release-path-filter.ts:59(test-coverage): The private isBrokenPipe predicate lacks a direct unit test, so a runtime error-shape change could restore the flaky failure without test detection.
Refuted: The integration test exercises the relevant runtime boundary and fails if a write error escapes instead of producing the expected git diagnosis. A unit test using fabricated objects would only test the predicate's comparison and would not validate Bun's actual error shape.packages/config/scripts/semantic-release-path-filter.ts:56(documentation): Mentioning Node's EPIPE convention is misleading because the current caller uses Bun.spawn.
Refuted: The comment does not claim this call executes under Node; it accurately explains why the stable code property is checked instead of runtime-specific message text. The Bun-only caller does not make that comparison incorrect.
Stats
Claude findings: 5 · Codex findings: 0 · Confirmed: 1 · Refuted: 3 · Uncertain: 1
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| try { | ||
| await proc.stdin.write(`${hashes.join("\n")}\n`); | ||
| await proc.stdin.end(); | ||
| } catch (cause) { |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
A caught EPIPE followed by a zero git exit code could allow parsing incomplete stdout without detecting truncated input.
Evidence: packages/config/scripts/semantic-release-path-filter.ts:133-137 suppresses EPIPE, while lines 139-159 reject only nonzero exits and otherwise parse and return the observed hashes.
Suggested fix: Track whether the write failed and reject a zero-exit result unless completeness can otherwise be established.
Adjudication (uncertain): The control flow permits the claimed combination, but the harmful outcome requires git diff-tree to close stdin and exit successfully before consuming all hashes. Neither the implementation nor tests establish that this can occur, so silent truncation could not be verified.
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@3bb7b37df0362f9e9e831b6e1049be5d3cdb22d4Preview package for commit |
1 participant
Summary
filterCommitsToPackagefeeds commit hashes togit diff-tree --stdinover the subprocess's stdin. Agitthat rejects its arguments — acwdoutside any repository, say — exits before it reads a single hash, so the write that follows can land on a process that has already gone and raiseEPIPE: broken pipe, send.Which error surfaces is a race against process startup: green on an idle machine, red on a loaded CI runner. The exit code and stderr below the write are the real diagnosis, so a broken pipe on stdin is now dropped and the reporting left to them. Any other write failure still throws.
isBrokenPipematches on theEPIPEcode rather than the message text, since Node and Bun word it differently.Context
Split out of the workers stack. This landed on
FUNC-853/workers-logs-command(#6410) to stabilise that branch's CI, but it has nothing to do with workers — it is apackages/configrelease-script fix, and it should not merge or revert with the workers work. Raised in review on #6410.Carries one fix on top of the original commit:
isBrokenPipehad been inserted betweenfilterCommitsToPackage's JSDoc and the function itself, orphaning the doc. The helper now sits above it.