fix(coding-agent): complete spill cleanup lifecycle - #1142
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ce219da5d
ℹ️ 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
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
B1. Merge blocker: GitHub reports this PR as mergeable=CONFLICTING. The PR's packages/coding-agent/CHANGELOG.md edit conflicts with the newer [Unreleased] entries already on main, so this branch cannot be merged as submitted. Rebase or merge current main, resolve the changelog (and any resulting documentation drift), and rerun the required checks on the resolved head.
B2. The claimed "always clean up" guarantee still fails for non-Error failures. The new instanceof Error fast paths in the executor and shell tool rethrow before closing/removing the active spill. A custom BashOperations.exec can reject a string/object, and an onUpdate/onChunk consumer can throw a string or a cross-realm error; the executor then exits with the stream and path still live. Worse, a non-Error thrown by the timer-driven emitOutputUpdate escapes the timer callback as an uncaught exception, with no promise path left to perform cleanup. Normalize unknown failures (or move cleanup into an unconditional finally) and add a regression that exercises non-Error failures through both the command and update paths.
B3. Failed unlink permanently discards the cleanup handle. removeTempFile() sets tempFilePath to undefined before awaiting rm(). If rm fails - a failure this PR explicitly aggregates and tests - the spill remains on disk but the accumulator/executor can no longer retry or report the path for a subsequent cleanup attempt. Only clear the field after a successful removal, or retain/retry the path when removal fails; add an assertion that a failed removal does not make later cleanup a silent no-op.
The existing CI checks being green does not make this mergeable or cover these untested thrown-value paths.
|
Addressed review 5058031058.
Verification:
|
|
WORKING: review-1142-r2 - reading round-1 review, author summary, and new diff. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-2 adversarial review of ffac7df.
Round-1 blocker verdicts:
- B1 VERIFIED. GitHub now reports
mergeable=MERGEABLE(the previous changelog conflict is resolved). Comparing HEAD toorigin/mainshows the merge retained the main branch's Unreleased Added/Changed content and added both spill-cleanup bullets under Fixed. The unrelated conflict marker insrc/modes/interactive/changes.mdis already present onorigin/main, so I am not attributing it to this PR. - B2 PARTIALLY VERIFIED, but not resolved: the executor and shell-tool finalization paths now catch arbitrary primary thrown values, and the new custom-operation regressions cover string command rejection, cross-realm final-update rejection, and timer-driven update failure. The
await operations.exec(...)boundary also covers both a synchronous throw and a rejected promise by code inspection. - B3 VERIFIED. Both executor and accumulator
removeTempFile()implementations retain the path untilawait rm(...)succeeds. A failed removal therefore leaves the handle observable and retryable; there is no retry loop or automatic unbounded retry. The new retry regression passed.
B4. Blocker: local-stream onChunk failures still bypass cleanup and can escape as uncaught exceptions. createLocalShellOperations() installs the executor's onData callback directly as the child stdout/stderr data listener (packages/coding-agent/src/core/tools/bash.ts:132-133), while executeBashWithOperations() calls options.onChunk(text) directly from onData (packages/coding-agent/src/core/bash-executor.ts:205-207). A thrown value from a real local onChunk consumer therefore propagates out of the stream EventEmitter rather than through the operations.exec() promise catch, so closeTempFileAndCleanup() is never reached. I reproduced this on HEAD with a 60KB local command and onChunk: () => { throw "onChunk failed"; }: the process received an uncaught string exception, the executor promise resolved successfully, and the spill path remained on disk. The added onChunk regression does not catch this because it invokes onData directly inside a custom async exec, where the throw is caught by the executor's command catch. Route local stream callback failures through the promise-controlled execution path (and add a regression using createLocalBashOperations).
The focused regressions pass (10/10 and 14/14), and TypeScript diagnostics/build checking is clean, but B4 means the claimed complete cleanup lifecycle is not yet verifiably true for the real local execution surface.
|
Round-2 B4 fixed in commit
Verification:
|
|
WORKING: review-1142-r3 - inspecting the local stdout/stderr wrapper, executor cleanup and abort ordering, then running focused real-shell reproductions. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-3 adversarial review of 6e5bf6e7a7101655801984492f78afae560335b2.
B4. VERIFIED resolved. I reran the real local-shell shape with output larger than DEFAULT_MAX_BYTES through both stdout and stderr. Synchronous onChunk throws of a string, object, and Error all propagated with the identical original value, produced no uncaught exception, and left no new pi-bash-*.log spill. I also verified true synchronous executor throws and rejected promises for string/object/Error values; each preserved identity and removed the spill. Normal combined stdout/stderr delivered 55,296 bytes on each side, returned exit code 0, and retained the readable spill on success. A detached child plus descendant were both gone before the callback failure reached the executor rejection, with no spill left, confirming abort-before-cleanup and no observed double-cleanup.
Focused verification passed: local regression 1/1; spill finalization/storage regressions 25/25; related child-process, late-output, timeout, and persistence tests 68/68; npx tsc --noEmit; and LSP diagnostics are clean.
- Async
onChunkrejection still escapes cleanup and leaves a spill.BashExecutorOptions.onChunkis declared(chunk: string) => void, but TypeScript permits anasyncfunction at a void-returning callback site.executeBashWithOperations()invokesoptions.onChunk(text)without observing a thenable (packages/coding-agent/src/core/bash-executor.ts:206-207), and the new local wrapper only catches synchronous exceptions aroundonData(packages/coding-agent/src/core/tools/bash.ts:126-136). I reproduced this on the new head with the same large-output local command for both stdout and stderr andonChunk: async () => { throw value; }: the execution resolved withexitCode: 0, the rejected value reachedunhandledRejection, and one new spill file remained. Either make the callback contract/runtime handling explicitly async-aware (capturing the rejection, aborting the child, and routing it through executor cleanup) or enforce a truly synchronous callback contract; add regression coverage for both streams and arbitrary rejected values.
The synchronous local-stream B4 path is fixed, but this unresolved callback failure path means the claimed complete spill-cleanup lifecycle is not yet complete.
|
Round-3 blocker fixed in pushed commit Async Regression coverage exercises real large-output local commands on both stdout and stderr for all three rejection shapes. Failing-first: 6 async cases failed against the prior implementation (resolved successfully and left spill files). Green: async regression 7/7; related spill/child-process/late-output/timeout/persistence set 67 passed, 2 expected platform skips; The PR head branch was updated non-force to |
|
WORKING: review-1142-r4 - auditing async callback settlement, listener lifetime, abort/finalization ordering, and scoped rejection observation before the required real-shell and targeted test passes. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-4 adversarial review of 699c871efd4a26ad9aebc7650c90105faba4cb7a.
B4. VERIFIED resolved at the executor/local-operations boundary. I ran real local-shell commands with output larger than DEFAULT_MAX_BYTES on both stdout and stderr. Async onChunk rejection of a string, object, and Error preserved the exact rejection identity, produced no unhandledRejection, and left no new pi-bash-*.log spill. Mixed sync+async sequences returned the first observed failure without an unhandled rejection; a thenable-but-not-Promise that rejected and then threw preserved the first rejection identity; and a rejection after the local shell operation had exited still finalized and removed the spill. The delayed-rejection probe also killed an escaped descendant and left no spill. Focused Vitest passed: local async-onChunk regression 7/7 and finalization/storage/late-output regressions 25/25; npx tsc --noEmit and LSP diagnostics were clean.
I also verified the rejection observer is scoped: an unrelated Promise.reject(realBug) still reached the process unhandledRejection handler, while the direct onChunk rejection did not. I found no additional defensible blocker in the direct executor path from listener lifetime or abort/finalization ordering.
B5. Blocker: the public AgentSession.executeBash() adapter still drops async onChunk rejections. At packages/coding-agent/src/core/agent-session.ts:7590-7597, the adapter calls onChunk?.(delta) but does not return or await its result, then calls _emit() (which returns void). Consequently the callback passed into the newly fixed executeBashWithOperations() appears synchronous and its thenable is never registered in callbackPromises; the executor cannot abort or clean up that failure. TypeScript still permits an async function at this void callback type.
I reproduced this through the actual AgentSession.prototype.executeBash method with the real local operations backend and an isolated TMPDIR: for all six combinations (stdout/stderr x rejected string/object/Error), a command producing >50KB resolved with exitCode: 0, the callback rejection reached unhandledRejection, and the returned fullOutputPath still existed. Under Node's default strict unhandled-rejection policy this is process-fatal. Return the user's callback result from this adapter (while retaining the event emission), or otherwise explicitly observe it and route it through the executor. Until the public execution path is fixed, the round-3 async cleanup blocker remains reachable in normal use.
|
Fixed B5 in commit |
|
WORKING: review-1142-r5 - reproducing the public AgentSession.executeBash callback contract (all six async shapes plus sync/order/non-Promise cases), then auditing callback wrappers and TypeScript compatibility before submitting the single review. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-5 adversarial review of 357b6fa8a3614fb7ce00c5c0ad387a05215d01ac.
B5. VERIFIED resolved on the direct public AgentSession.executeBash() path. With a real local shell backend and an isolated TMPDIR, I reproduced all six stdout/stderr x string/object/Error async rejection shapes through AgentSession.executeBash(). Each rejected with the identical original value, produced no unhandledRejection, and removed the newly created pi-bash-*.log spill. I also verified a synchronous throw still preserves identity and cleans up; callback -> bash_execution_update ordering remains synchronous and unchanged; non-Promise callback returns do not change the result; and exactly one update event is emitted per chunk. The focused Vitest set passed 48/48, TypeScript and LSP diagnostics are clean.
B6. Blocker: the shared interactive-host proxy still drops async onChunk rejections. In packages/coding-agent/src/modes/interactive/interactive-host-runtime.ts:265, the wire-event handler invokes bashChunk?.(wireEvent.delta) without observing or routing the return value. This is a supported createInteractiveHostRuntime() execution path and bypasses the fixed AgentSession.executeBash() adapter when the command runs on the shared RPC host. I reproduced it with a real RPC host and runtime.session.executeBash() using >50KB output plus an async callback rejecting an object: the call resolved with exitCode: 0, the callback rejection reached the client process unhandledRejection, and the host returned a fullOutputPath that still existed. Because the callback runs in the client while the spill belongs to the host, this must be handled across the proxy/protocol (for example, observe the callback failure, abort the host bash, and reject the client call) rather than being fire-and-forget.
B7. Blocker: the exported agent harness shell-capture adapter has the same async hole. packages/agent/src/harness/utils/shell-output.ts:140 calls options?.onChunk?.(text, createProgress) and discards the result. ShellCaptureOptions and executeShellWithCapture() are exported from packages/agent/src/index.ts, so this is a public shell/spill path, not an internal test-only callback. With a real NodeExecutionEnv, isolated TMPDIR, and >50KB stdout/stderr commands, I reproduced all six async string/object/Error cases: each capture resolved successfully with exitCode: 0, returned a live fullOutputPath, and emitted one unhandledRejection. Make this adapter async-aware (or enforce/actually enforce a synchronous contract) and ensure callback failures terminate through its cleanup path.
Until B6 and B7 are addressed, the repository still has reachable public adapters that can resolve successful large-output executions while leaving spill artifacts and unhandled callback rejections behind.
|
Round-5 blockers B6/B7 resolved in 5a0a61a (pushed to fix/edquot-write-crash).\n\n- B6: shared interactive-host bash proxy now observes sync throws and async callback results, aborts the host bash on the first callback failure, waits for callback settlement, and rethrows the identical original rejection value at the client executor boundary. Added real RPC-host regression coverage for stdout large output and string/object/Error rejection values; 3/3 focused cases pass with no unhandled rejection.\n- B7: exported harness shell capture now accepts async onChunk callbacks, awaits stdout/stderr callback settlement in NodeExecutionEnv, routes callback failures through capture cleanup, preserves the original cause, and removes any created full-output spill on failure. Six stdout/stderr x string/object/Error cases pass; 35 tests passed (1 skipped), with no unhandled rejection.\n- Sweep: every remaining TypeScript onChunk invocation was inspected. The shell capture adapter uses await; agent-session/bash-executor already observe callback results; remaining codemode/AI calls are synchronous callback contracts and do not discard promise-capable results.\n- Gates: agent and coding-agent builds passed; repository pre-commit check passed (Biome, pinned deps, imports, shrinkwrap, install lock, SDK platform lock, root typecheck, browser smoke); changelog gate against origin/main passed: changes.md coverage complete (7 production paths) and coding-agent CHANGELOG updated.\n\nThe initial failing-first harness regression reproduced all six unhandled async rejection shapes; after the adapter/runtime fixes the full focused harness set is green. The initial RPC regression exposed host startup instability from an overly long Unix socket path, then the shortened real-host regression suite passed 3/3. No merge/close/force-push actions taken. |
|
WORKING: review-1142-r6 - inspecting the fixed RPC proxy and harness adapter, reproducing async rejection/abort/concurrency/never-settling cases, and independently auditing every onChunk invocation before the single final review. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-6 adversarial review of 5a0a61a.
B6/B7 are improved for the tested rejection shapes, but the cleanup lifecycle is not complete and the new RPC state is not re-entrant.
B8. Shared-host callback failures still leave host spill files. The new proxy does abort the host and preserves the callback value, but the host-side executeBashWithOperations() cancellation branch returns a cancelled result with fullOutputPath rather than removing that path, and the proxy discards that result once Promise.all() rejects. I reran the real RPC-host test with an isolated TMPDIR: all three callback shapes rejected with the identical object/string/Error and no unhandledRejection, but one pi-bash-*.log remained for each case (100001 bytes). This is the original B6 leak across the RPC boundary, not a test-only artifact. The undefined sentinel also loses a synchronous throw undefined: a real RPC probe fulfilled successfully instead of rejecting with the identical value.
B9. The exported harness capture adapter does not abort the child on callback rejection. executeShellWithCapture() catches the user callback rejection inside its onChunk wrapper and stores captureError; therefore NodeExecutionEnv.handleChunk() sees a fulfilled callback promise and never calls its onAbort(). With a real NodeExecutionEnv, printf x; sleep 2 plus an async rejecting onChunk took about 2024 ms and returned callback_error, rather than terminating at the first rejection. The six stdout/stderr x string/object/Error cases do preserve the original cause, avoid unhandledRejection, and remove the spill after a finite command, but a rejected callback must terminate the underlying command as claimed.
B10. Harness spill-removal failures are silently ignored. cleanupFullOutput() calls env.remove() without checking its Result; the harness contract explicitly says filesystem methods encode failures as { ok: false } and do not throw. I used an ExecutionEnv whose remove() returned FileError("permission_denied"): the capture returned only the callback error while the full-output file still existed. Cleanup failure must be observed/returned or aggregated, not dropped.
B11. The remote proxy's callback state is global, so concurrent remote bash executions corrupt one another. bashChunk, bashCallbackPromises, and bashCallbackError are single variables shared by all calls. A real RPC-host probe started two runtime.session.executeBash() calls concurrently; the first rejected with TypeError: undefined is not iterable while the second callback received both A and B chunks and returned only B output. AgentSession supports multiple active bash executions, so the proxy must keep tracking per execution.
B12. Promise tracking is unbounded and a never-settling callback prevents abort completion. The RPC proxy appends every callback promise to an array and never removes settled entries; the core executor has the same append-only tracking for promise-returning operation callbacks. A long-running/high-volume command therefore retains one promise per chunk until completion. A synthetic never-settling callback remained pending, and the real RPC probe was still pending 1.2 s after abortBash(); it completed only after the callback promise was manually resolved. Tracking needs bounded cleanup and abort must not leave the execution lifecycle hostage to an unresolved observer.
B13. Events continue invoking the callback after the first rejection while abort is in flight. The wire handler has no failure gate before calling bashChunk. With abortBash() delayed by 100 ms, a real RPC probe whose first callback rejected invoked the callback 20 times total, including 19 calls after the rejecting first callback. Those post-failure callbacks can perform side effects and create more work after the declared first-failure abort; dispatch should stop or be explicitly sequenced after the first failure.
Prior-blocker verdicts:
- B6: NOT RESOLVED - identity/no-unhandled propagation passes, but isolated RPC reproduction still leaves host spill files (B8).
- B7: PARTIALLY RESOLVED - all six harness shapes pass the finite-command cleanup/cause checks, but child abort and removal-error handling remain broken (B9/B10).
Sweep verdict: PASS for the claimed reachable shell paths. The repository grep found the shell onChunk calls in the core executor (thenable observed), AgentSession (callback result returned into the executor), the harness capture adapter (awaited), and the interactive proxy (tracked). The remaining codemode OutputSink/image/cell callbacks and the harness tool's internal callback are typed synchronous contracts; they do not expose a promise-capable shell callback. The AI test callback is unrelated to shell output.
Cause/catch-site check: No separate blocker found. The six target shapes and a raw backend-string probe preserve the original value in ExecutionError.cause, and no relevant repository shell consumer assumes that cause is always an Error.
Verification: the focused agent harness suite passed 35 tests with 1 expected skip; the focused coding-agent B6 test passed 3/3; the four related coding-agent regression/persistence files passed 48/48; root TypeScript and both package build TypeScript checks passed. The broader mixed interactive run was affected by pre-existing QA-port exhaustion from other worktrees, so it was not used as the acceptance signal.
VERDICT: REQUEST_CHANGES
|
Round-6 blocker fixes pushed in
Evidence:
Verified with |
|
WORKING: review-1142-r7 - auditing execution-id lifecycle/reconnect behavior and bounded callback settlement, then running the required focused reproductions before the single final review. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-7 adversarial re-review of cf913fb.
Blocker resolution verdicts:
- B8: NOT RESOLVED. I ran the real RPC path with an isolated TMPDIR and a large-output callback rejection. The callback rejected with the identical Error, but the host left one
pi-bash-*.logbehind. The command can finish and finalize its successful spill before the client receives the update failure; the later client-sideabortBash()then has no active host execution to clean that path, and the discarded result cannot clean a host-side artifact. - B9: NOT RESOLVED. Direct
NodeExecutionEnv/executeShellWithCapturereproduction withprintf x; sleep 2and an async rejectingonChunktook 2024 ms and returnedcallback_error.executeShellWithCapture.onChunkcatches the rejection and storescaptureError, soNodeExecutionEnv.handleChunk()sees a fulfilled callback promise and never invokesonAbort(). - B10: NOT RESOLVED. I used an
ExecutionEnvwhoseremove()returned{ ok: false, error: FileError("permission_denied") }. A large-output callback failure returned only the callback error,remove()was attempted, and the spill path still existed.cleanupFullOutput()still ignores the filesystem Result instead of surfacing or aggregating it. - B11: RESOLVED for two concurrent executions through one proxy: an isolated real RPC run delivered A only to A and B only to B, with no TypeError. However, the execution-id scheme has a new collision case below.
- B12: RESOLVED for the never-settling case: an isolated real RPC run with a never-settling callback and explicit abort completed in 108 ms and returned
cancelled: true; the direct executor probe completed in 102 ms. - B13: RESOLVED on the real RPC wire path: an extension operation emitting a burst delivered exactly one callback invocation after the first rejection (zero calls after it), and the original rejection identity was preserved.
- throw-undefined: RESOLVED. Both direct executor and real RPC probes rejected with the actual
undefinedvalue rather than fulfilling.
Fresh blockers:
B14. Host spill still leaks when callback failure races host completion. This is the concrete B8 failure above: the callback-failure abort is client-side, but the spill is created and owned by the host. Cleanup must cover the successful-host-result/failing-client-observer ordering too; fixing only the host cancellation branch is insufficient.
B15. Harness callback rejection still does not abort the child. executeShellWithCapture() catches the user callback rejection internally, which prevents the newly async-capable NodeExecutionEnv callback boundary from seeing a rejection and killing the process. The required printf x; sleep 2 reproduction still waits for the full command.
B16. Harness cleanup failures remain silently dropped. env.remove() is a Result-returning, non-throwing filesystem operation, but cleanupFullOutput() does not inspect its Result. This leaves artifacts while reporting only the callback/command error, violating the cleanup lifecycle and losing the cleanup failure.
B17. The hardcoded 100 ms callback wait abandons legitimate slow callbacks. Both executeBashWithOperations() and the remote proxy race allSettled() against setTimeout(..., 100). A direct executor callback that legitimately awaited 500 ms and then rejected caused the command to return success after about 103 ms; the same real RPC case returned success after about 108 ms, and the later callback failure could no longer affect the execution. The code now publicly accepts PromiseLike callbacks, so this is not limited to an intentionally never-settling observer. The bound needs an explicit contract/justification or the callback failure must remain observable without silently returning success.
B18. Execution IDs collide across attached client proxies. nextBashExecutionId is local to each createRemoteSessionProxy, while the multi-session SessionEventWriter broadcasts session events to every attached connection. I created two real interactive runtimes attached to the same host session and started printf A and printf B concurrently. Both proxies generated bash-1; each callback received both A and B (a=["A","B"], b=["A","B"]) even though the returned command outputs were separately A and B. IDs must be unique across the host session/event broadcast domain, or update events must be targeted to the originating attachment. This also affects reconnects: a new proxy can restart at bash-1 while an old execution's events are still being broadcast.
Verification: package-agent harness suite passed 35 tests with 1 expected skip; focused coding-agent spill/persistence suites passed 48/48; root npx tsc --noEmit, coding-agent build, git diff --check, and LSP diagnostics were clean. The full interactive-host test file was not usable because all shared QA ports 18990-18999 were occupied by unrelated worktrees; the blocker verdicts above came from independent isolated-socket RPC and direct harness probes.
VERDICT: REQUEST_CHANGES
|
Round-7 blocker fixes pushed in
Verification:
|
|
WORKING: review-1142-r8 - reproducing finalization, rejection/abort, cleanup-result, slow-callback, attached-client, RPC-unreachable, and reconnect-correlation behavior before the single final review. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-8 adversarial re-review of 950e515bad07d3d7820db8f3c86e59efe493ef2b.
Blocker resolution verdicts:
- B14: VERIFIED resolved. With an isolated
TMPDIR, a real RPC host, >50KB output, and an async callback that rejects, the proxy rejected with the identical callback error and the host-owned spill directory had no new spill after completion. This covers the successful-host-result/client-callback-failure finalization race. - B15: NOT RESOLVED. The exact
printf x; sleep 2harness reproduction still took about 2015ms and returnedcallback_error.executeShellWithCapture()still catches the user'sonChunkrejection and storescaptureErrorwithout rethrowing;NodeExecutionEnv.handleChunk()therefore sees a fulfilled callback promise and never callsonAbort(). The child is not terminated promptly. - B16: VERIFIED resolved. An
ExecutionEnvwhoseremove()returnederr(new FileError("permission_denied", ...))produced an error whoseAggregateErrorcause contained both the original callback failure and the cleanup failure. - B17: VERIFIED resolved. A direct executor callback that awaited 500ms and then rejected completed in about 503ms with the identical error and no spill; the real RPC proxy case completed in about 534ms with the identical error. The 100ms escape is no longer used for normal completion.
- B18: VERIFIED resolved for concurrent attached clients. Two attached interactive proxies executing
printf Aandprintf Bconcurrently received only their own chunks (["A"]and["B"]), and their wire execution IDs were distinct UUID-namespaced values.
Fresh blockers:
B19. The harness adapter still fulfills callback rejection before the child can abort (B15 remains reachable). This is a supported exported executeShellWithCapture() path, not just a direct NodeExecutionEnv callback test. On the current head, printf x; sleep 2 plus onChunk: async () => { throw callbackError; } waited for the full command instead of terminating at the first rejection. Fix the adapter so the callback rejection reaches the environment's abort-aware boundary, while retaining the original cause and cleanup aggregation.
B20. Client-side spill-removal RPC leaks again if the host transport is lost at the cleanup boundary. The proxy obtains a successful host result, then waits for callbacks and calls client.cleanupBashOutput(result.fullOutputPath) only after the host command has finished. I reproduced the transport-loss ordering by stopping the client immediately before that cleanup request: the call rejected with Client not started, the original callback error was replaced, and the host spill path still existed in the isolated TMPDIR. Cleanup must have a host-side/lifecycle fallback or otherwise retain a recoverable cleanup obligation when the RPC request cannot be delivered.
B21. A reattached proxy cannot correlate an execution that was already in flight. I started printf A; sleep 0.5; printf B on one attached proxy, disconnected it after the first update, and attached a new proxy to the same session while the host execution continued. The new connection received the old UUID-tagged bash_execution_update at the transport level, but the new proxy's bashExecutions map only contains its newly generated UUID namespace, so the old continuation is ignored; the old client only received A and the reattached proxy cannot resume/correlate B. If in-flight work is intended to survive reconnect/reattach, the execution identity and callback state need a session/attachment-stable resumption mechanism (or the protocol must explicitly terminate and clean up orphaned executions).
B22. Normal completion can hang forever on a legitimately stuck callback. executeBashWithOperations() now awaits Promise.allSettled([...callbackPromises]) without a bound whenever the caller signal is not explicitly aborted. A direct probe with a successful command and an onChunk callback returning a never-settling promise remained pending after 200ms and completed only when the callback was manually released. Since PromiseLike callbacks are part of the public contract, a callback stalled by a UI/network dependency can permanently hold the shell command and its lifecycle hostage with no cancellation path. Add an explicit callback timeout/abort contract or preserve a bounded, observable failure policy for normal completion.
Verification: coding-agent focused spill/local/persistence regressions passed 48/48; agent harness nodejs-env.test.ts passed 35 with 1 expected skip; the targeted shared-host async callback test passed 3/3; root TypeScript and Biome checks passed. The full interactive-host file remains environment-blocked by unrelated occupied QA ports, so the RPC claims above use isolated Unix-socket probes.
VERDICT: REQUEST_CHANGES
|
Round-8 blocker fixes pushed in a49c361.
Verification: failing-first existing B19 probes reproduced the reviewed behavior and then passed after the fix; focused tests pass (111 passed, 1 skipped), including deterministic B22 tests for 500ms and never-settling callbacks. Agent/coding-agent typechecks, builds, Biome, repository pre-commit checks, and |
|
WORKING: review-1142-r9 - reproducing B19-B22 on a49c361 and probing long callbacks, transport-loss cleanup races, local-fallback write races, and reattach orphan policy before the single final review. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-9 adversarial re-review of a49c361779d4a55edd84c445ef0113e6151df25d.
Prior-blocker verdicts:
- B19: VERIFIED resolved. The exact exported-harness reproduction,
printf x; sleep 2plus an async rejectingonChunk, returnedcallback_errorwith the identical original cause in 122 ms. The child no longer runs to its normal ~2 s completion. - B20: VERIFIED resolved for the claimed transport-loss ordering. I forced the runtime's RPC client down exactly when
cleanupBashOutput(path)was invoked after a real isolated-host large-output callback failure. The proxy preserved the original callback error, the local fallback removed the host spill path, and the isolatedTMPDIRhad zero remainingpi-bash-*files. Code ordering also rules out the proposed host-write race: fallback is reached only afterclient.bash()has returned a finalized result, not while that host executor is still streaming. - B21: VERIFIED resolved only for an actually orphaned execution. I dropped the originating client after its first in-flight update while keeping an observer attachment alive, then reattached through
createInteractiveHostRuntime(). Reattach aborted the old execution, host state returned toisBashRunning: false, and the isolated spill directory was empty. The broader attached-client policy has a fresh blocker below. - B22: NOT RESOLVED on the shared-host path. The direct executor does release a never-settling callback at 5001 ms, but the remote proxy still calls
waitForBashCallbacks(execution.promises, false)with no normal or cancellation bound. A real isolated-socket callback was confirmed started and remained pending after 5209 ms even though hostisBashRunningwas already false; it completed only when I manually released the callback. Explicitruntime.session.abortBash()also remained pending after 503 ms with the host already stopped. - B17 regression check: PASS. A direct 500 ms callback completed in 502 ms; the real shared-host case completed in 562 ms.
Fresh blockers:
B23. Shared-host normal completion and abort still wait forever on a never-settling callback. interactive-host-runtime.ts:546 always passes false to waitForBashCallbacks, whose false branch is an unbounded await settled. The new 5-second policy was added only to executeBashWithOperations() and NodeExecutionEnv; it was not applied to the connection-local RPC callback set. This leaves B22 reachable on the supported interactive shared-host surface, including after explicit abort.
B24. The 5-second cutoff silently loses a legitimate late callback failure and its spill-cleanup obligation. With a public callback doing six seconds of real async work and then rejecting, the direct executor fulfilled successfully at 5004 ms. The callback rejected at 6056 ms, but the returned large-output spill still existed. The PromiseLike callback contract says nothing about failures becoming unobservable after five seconds; the only documentation is an internal changes entry saying callbacks are waited for "up to" five seconds. Either surface an explicit timeout/failure result and clean up, or make abandonment an explicit public contract that cannot later require cleanup.
B25. A new attachment aborts wanted work owned by a surviving attachment. createInteractiveHostRuntime() treats any opened.state.isBashRunning as orphaned and calls session-wide abortBash(). In a real two-runtime reproduction, runtime A received A from printf A; sleep 2; printf B; merely attaching runtime B caused A to finish in 99 ms with cancelled: true and no B. The RPC registry explicitly supports co-attached live sessions, so "running" does not imply "orphaned". Abortion must be scoped to a genuinely disconnected execution/attachment or adoption must be represented; a new observer cannot kill another live client's command by default.
B26. The new timeout timers are not cleared and pin process shutdown for five seconds after successful callback settlement. A direct command whose callback completed immediately reported settlement in 0 ms, but /usr/bin/time showed the Node process remained alive for 6.29 s. Both bash-executor.ts and NodeExecutionEnv create the 5-second race timer without clearing or unref'ing it when callbacks win. This is user-visible shutdown latency on every promise-returning output callback and explains the focused public callback cases taking ~17.7 s in isolation.
B27. The PR is currently unmergeable. GitHub reports mergeable=CONFLICTING / mergeStateStatus=DIRTY; an independent git merge-tree --write-tree HEAD origin/main reproduces a content conflict in packages/coding-agent/src/core/changes.md. Rebase/merge current main, resolve the conflict, and rerun the affected checks on the resolved head.
Verification: agent harness focused suite passed 35 with 1 expected skip; focused spill finalization/storage/local-callback regressions passed 31/31; related abort/late-output suites passed 7/7; shared-host callback rejection passed 3/3 on an isolated socket; agent and coding-agent TypeScript builds and git diff --check passed. A combined five-file coding-agent run exposed three nondeterministic spill-list assertions because the tests scan the process-global temp directory while Vitest files run concurrently; the same cases passed alone. The full shared-port interactive suite was not used.
VERDICT: REQUEST_CHANGES
|
Round-9 blocker resolution summary:
Verification: agent harness 35 passed (1 expected skip); coding-agent spill regressions 21 passed; agent and coding-agent builds passed; repository pre-commit check passed; changelog gate passed against origin/main. The full interactive-host file was attempted, but its fixed QA ports were occupied by unrelated stale processes and the suite timed out; focused spill/host coverage was run separately. |
|
Round-9 follow-up: late callback failures are now explicitly surfaced via process warning after the bounded lifecycle returns, while the executor performs spill cleanup. Final pushed head is 8461da5. Focused green evidence: 56 passed, 1 expected skip across spill regressions and agent harness; both package builds, full repository pre-commit check, and changelog gate passed. GitHub reports headRefOid=8461da572719940eb01b3e6fc9e2641e5d7da02d and mergeable=MERGEABLE. |
|
WORKING: review-1142-r10 - inspecting round-9 findings, diff, and targeted reproduction paths |
|
WORKING: review-1142-r10 - reproducing direct late rejection/cleanup/warning, timer shutdown, isolated shared-host bounds, and two-runtime survivor behavior |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-10 adversarial re-review of head 8461da5.
Named blocker resolution:
- B23 VERIFIED. On an isolated Unix-socket host, a never-settling callback on normal completion returned
Bash output callback did not settle within 5000msin about 5013 ms. The same callback with an explicitabortBash()whileprintf x; sleep 10was running returned in about 5011 ms (about 5005 ms after abort), rather than remaining pending. Both shared-host paths are bounded. - B24 VERIFIED. A direct large-output execution whose callback rejected only after 6 seconds returned the explicit callback-settlement timeout at about 5006 ms; after the late rejection, the isolated spill directory contained no new
pi-bash-*.log, and the process warning handler observed codeBASH_CALLBACK_ERRORwith the late failure text. - B25 VERIFIED. Two runtimes attached to the same session were probed while runtime A ran
printf A; sleep 1; printf B; attaching runtime B did not abort A. A completed with outputABand callback chunks["A", "B"]. - B26 VERIFIED. Immediate callback settlement showed no lingering timeout resource in
process.getActiveResourcesInfo(). A direct executor plusNodeExecutionEnvprobe under/usr/bin/time -pexited promptly (real 0.27s), and the focused finalization/storage tests passed. - B27 VERIFIED.
git diff --check origin/main...HEADpassed andgit merge-tree --write-tree HEAD origin/mainproduced a clean tree (e989c666242c98c016732375b208977e3d716ee2); the direct and package builds also passed.
Fresh blocker:
- B28 REQUEST CHANGES: the PR's own targeted
test/tools.test.tsregression is deterministically red undernpm --prefix packages/coding-agent test -- --pool=forks --maxWorkers=1 test/tools.test.ts -t 'does not hang forever on a never-settling output callback'. The test advances the bound and assertsexecutionresolves with{ output: "output", exitCode: 0 }, but the implemented contract now correctly rejects withBash output callback did not settle within 5000msand cleans up. Vitest reports that rejection as an unhandled error as well. This test is part of the PR, so the focused suite cannot pass as submitted; update the assertion to the explicit timeout/error-and-cleanup contract (and consume the rejection).
Additional verification: the harness suite passed 35 tests with 1 expected skip; the shared-host async callback test passed 3/3; the surviving-attachment RPC tests passed 3/3; LSP diagnostics were clean. I found no additional runtime blocker beyond the failing changed regression test.
VERDICT: REQUEST_CHANGES
|
Round-10 B28 fixed in commit
The auxiliary |
|
WORKING: review-1142-r11 - B28 targeted and full tools.test.ts verification complete; submitting the single final review. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-11 adversarial re-review of head 3789a99.
B28 VERIFIED. The updated package-local regression passes independently: npm test -- --pool=forks --maxWorkers=1 test/tools.test.ts -t 'rejects and cleans its spill when an output callback never settles' reports 1 passed (75 skipped). The complete tools.test.ts passes 76/76 with no unhandled-rejection warning. The test genuinely pins the documented contract: it uses a never-settling callback, remains unsettled after advancing fake time by 4,999 ms, then awaits rejection after 5,000 ms with the exact Bash output callback did not settle within 5000ms message, and verifies that the spill identified by a unique marker is removed. Removing the timeout would leave the test pending/failing; removing cleanup would fail the spill assertion. The implementation matches this through the bounded callback wait and cleanup path.
B23-B27 were verified in round 10 and remain the only previously resolved findings. I found no new regression or concrete blocker in this head. The workspace-level filtered command was noisy because npm propagated the filter to unrelated packages with no matching tests, but the required coding-agent package-local full and targeted runs are green.
VERDICT: APPROVE
2 participants
Summary
Follow-up to #1138, which merged while the extended review was still running. This keeps the already-merged early-error fix and adds the remaining verified lifecycle guarantees:
close, including late EIO and premature-close handlingAggregateErrororderVerification
npm run check: exit 0env -u KIMI_API_KEY CI=1 npm test: exit 0; coding-agent 8,839 passed / 37 skipped; all workspaces greenfs.closeEIO drivers: both owners reject, close callbacks complete, no leaked fd/pathScope
No PTY, codemode, shared SpillFile abstraction, dependency, or lockfile changes. The unrelated historical macOS PTY flake remains out of scope.
Summary by cubic
Completes the bash spill cleanup lifecycle so storage failures and sync or async output-callback failures always fail the bash tool call through one cleanup boundary, instead of escaping to
uncaughtExceptionor leaving orphaned spill files.close, rejecting premature close and lateEIO/EDQUOTerrors.fullOutputPath.AggregateErrororder without masking the primary error.onChunkfailures across local streams,OutputAccumulator, publicexecuteBash, and the shared-host RPC proxy abort and reject through cleanup with no unhandled rejections.Written for commit 3789a99. Summary will update on new commits.