fix(coding-agent): contain bash spill stream errors

[minpeter]

Summary

• attach spill-file error listeners before the first write in both bash output paths
• preserve the first storage failure and reject it through the existing awaited close boundary
• add deterministic EDQUOT regressions for executeBashWithOperations and OutputAccumulator

Root cause

When large bash output crossed the in-memory limit, Senpi opened a /tmp WriteStream and started writing before installing its only error listener. An early EDQUOT/ENOSPC event therefore reached the process-level uncaughtException handler and terminated the interactive session.

Verification

• RED before fix: 2/2 new regression cases resolved instead of rejecting, proving the early stream error was unowned
• GREEN after fix: focused bash/tool suites, 78 passed and 2 platform-skipped
• npm run check
• npm run build
• env -u KIMI_API_KEY CI=1 npm test (full hermetic workspace suite)
• LSP diagnostics clean on both implementations and the regression test
• real built spill driver: SPILL_FAILURE=ENOTDIR:open, then PROCESS_SURVIVED=1
• isolated Senpi CLI smoke: 8/8 passed
• isolated zero-token mock bash loop: 4/4 passed, real auth unchanged

The first non-hermetic full-suite attempt activated legacy live Kimi E2E tests from ambient KIMI_API_KEY and received account-level 403 usage-limit responses. Re-running with that opt-in credential removed completed cleanly.

---

Summary by cubic

Fixes bash output spill files so an early EDQUOT/ENOSPC stream error fails only the tool call instead of terminating the interactive session through uncaughtException.

Bug Fixes

• Attaches spill-file error listeners before the first write in both bash output paths.
• Waits for the stream's terminal close event so late filesystem failures after finish (and cancellation racing command settlement) still reject through the awaited close boundary.
• Closes the spill stream before propagating decoder-flush or output-preparation errors, aggregating cleanup failures.
• Adds deterministic EDQUOT regressions for executeBashWithOperations and OutputAccumulator.

^{Written for commit 7b47dba. Summary will update on new commits.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 893ad173d4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74f6fd56b0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[code-yeongyu]

Verdict

REQUEST_CHANGES. The early spill-error listener fixes the reported process-level crash, but both close helpers still have a real error-masking path: a terminal write/close failure can arrive after finish, after the helper has already resolved and cleared the stream. That can return success with an incomplete fullOutputPath.

Blockers

1. packages/coding-agent/src/core/bash-executor.ts:105-116 and packages/coding-agent/src/core/tools/output-accumulator.ts:152-163 — closeTempFileStream() / closeTempFile() resolve on finish, while Node WriteStream can emit a final fs.close error after finish and before/with close. I reproduced the ordering locally with a custom fs.close: finish -> error:EIO -> close. The new persistent listener records that late error only after the promise has resolved and tempFileStream has been cleared, so the caller can return success and advertise a truncated or missing spill file. The same race also bypasses the promised AggregateError when output finalization fails and spill cleanup fails late. Required change: make the close boundary await terminal stream completion (including late close/error), reconcile the captured first error before resolving, and remove/settle the early listener only after that terminal boundary; preserve the first storage error and aggregate it with a finalization error when both occur. Add deterministic regression coverage for the finish -> error -> close ordering in both spill paths.

Non-blocking notes

• Listener lifecycle / duplicate handling: the early on("error") listener is installed before writes and ??= preserves the first failure. The close-time once("error") listener sees the same event and Promise rejection is idempotent, so there is no second rejection; its persistent sibling still needs the terminal lifecycle fix above.
• First-failure preservation and abort semantics: failures emitted before or during the currently awaited close are preserved, and moving successful finalization outside the command-execution catch correctly prevents a close failure from becoming a successful cancelled result. Genuine operation aborts still return the existing cancelled shape. The late-close blocker applies to both normal and abort finalization.
• AggregateError: the message Bash output finalization and spill cleanup failed is actionable and the [error, closeError] ordering is sensible when cleanup rejects before finish; the late terminal-error race above is the missing case.
• output-accumulator.ts mirrors the bash path correctly for early errors, but therefore shares the same late-close defect.
• Tests: from a fresh /tmp clone, npm ci --ignore-scripts succeeded and cd packages/coding-agent && npm test -- test/suite/regressions/bash-spill-storage-error.test.ts passed (4/4). Temporarily removing both new production listeners made the suite RED (3 failures: both bash cases and the accumulator case); the mock emits its error on the imported production stream path, so this is not a mock-only assertion. The suite does not yet cover the late terminal-close ordering required above. The dynamic node:stream import inside the Vitest mock also conflicts with the repository's normal module-scope import convention, but is non-blocking.
• CI: Terminal tools (macos-latest) failed only in @earendil-works/pi-pty's test/native-wait-threadpool.test.ts with native wait did not report cancellation. This PR changes no packages/pty or crates/senpi-pty files, and the latest main CI run passed the same macOS PTY test (58/58), so this is clearly unrelated/flaky rather than a PR blocker.
• Ledger/release metadata: scripts/check-pr-changelog.mjs --base 4ec1acf1df269cea1b083599360394b7b71b1375 passed with both changed production paths covered; audit-changes-md.mjs reported 247/247 covered. The two tracker blocks are dated and contain the four canonical headings, and the package CHANGELOG entry is under [Unreleased].
• Bot feedback: the connector's close-after-finish suggestion is valid and is the blocker above; its earlier abort-race suggestion is addressed by the finalization refactor; the module-scope dynamic-import suggestion is a style/policy note, not a correctness blocker.

[code-yeongyu]

Verdict

APPROVE. The round-1 blocker is genuinely resolved in both close helpers: each now waits for the WriteStream's terminal close event, records the first error through the close boundary, and settles/removes the temporary listeners exactly once. A finish -> error -> close mutation fails both new late-failure tests, while the restored implementation passes them.

Blockers

none

Non-blocking notes

• Fresh filtered clone and npm ci completed successfully. The new-commit diff is confined to packages/coding-agent/src/core/bash-executor.ts, packages/coding-agent/src/core/tools/output-accumulator.ts, packages/coding-agent/test/suite/regressions/bash-spill-storage-error.test.ts, the two requested changes.md ledgers, and packages/coding-agent/CHANGELOG.md.
• The dedicated regression file passes 6/6. The related suites pass 80 tests; the 2 Windows-only tests are skipped on this macOS host. Language-server diagnostics are clean for both production files and the regression file.
• Mutation check: restoring finish as the close boundary while retaining the new listeners made both late-failure tests fail with a prematurely resolved promise; restoring the PR implementation made all 6 regression tests pass.
• Adversarial checks passed: normal spill output remains truncated and its fullOutputPath contains the complete output; the abort spill path remains cancelled with a valid complete file; the OutputAccumulator spill file is complete after close; and finalization plus cleanup failures remain an AggregateError with the original finalization error first.
• node scripts/audit-changes-md.mjs exited 0: 247/247 production paths covered, 0 uncovered.

[minpeter]

Extended review continued after this PR merged. The verified lifecycle follow-up is #1142 at head f35720002baca9999e8c5facaad64a3b4c6e0fe0.

It adds terminal-close settlement, causal dual-error preservation, callback-safe spill finalization, failure-only artifact cleanup, and deterministic Node/Bun + full-suite evidence. Final mass-ulw behavior, quality, evidence, and Architect audits all passed unconditionally.