Improve OpenCode turn abort handling and failure messages

[L42y]

Summary

• Abort the previous OpenCode subprocess when a newer turn starts, so superseded review runs do not keep competing for CPU.
• Replace the generic turn aborted or timed out before a safe terminal event message with specific causes: timeout, superseded delivery, inactive session, or lifecycle abort.
• Include lightweight progress hints (missing step_finish, partial assistant text) in the failure card.

Context

Review agents on OpenCode were failing with an opaque timeout message while multiple opencode run processes stacked on a loaded host. The handler incremented generation for new turns but did not abort the prior subprocess, and all abort paths collapsed to one message.

Test plan

• pnpm exec vitest run src/providers/opencode/__tests__/turn-abort.test.ts
• Manual: trigger a Review OpenCode timeout/supersede and confirm the failure card names the specific abort cause

Made with Cursor

[baixiaohang]

Recommendation: request changes

• Rationale: The new cause-specific text is also being used as the input to provider failure classification, so a superseded lifecycle path can change delivery settlement and consume work that the stale generation no longer owns.

Risk level: A

• Path baseline: includes packages/client/**.
• Semantic lift: none; A is already the highest level.

PR summary

• Author / repo: L42y / first-tree-ai/first-tree
• Problem: OpenCode review turns can leave superseded subprocesses running and show operators an opaque abort message.
• Approach: abort the prior controller when a newer turn starts, derive a more specific cause, and add partial-progress hints to the failure card.
• Impacted modules: packages/client OpenCode provider lifecycle and tests.

Review findings
❌ 1. Keep diagnostic copy separate from delivery-settlement classification. The superseded, session-inactive, and lifecycle strings no longer match the shared transient-transport classifier. If a stale turn captured partial text, settleFailure marks it user_visible, the unknown classification takes the unsafe_replay stop path, and token.complete consumes that obsolete delivery even though a newer generation already owns the chat. Before this patch the generic text contained timed out, so this case stayed on the transient/non-consuming path. A superseded generation must not ACK work through text-derived classification: carry a structured cause/classification into settlement, fence completion to the owning generation, and cover the handler-level old-token/new-token behavior rather than only the formatter helper. [R4/R5 / packages/client/src/providers/opencode/index.ts:1042]

Action taken

• Submitted request changes.

[yuezengwu]

Request changes on exact head 8668fbe905c4020a70d119e73ff45bd1648fc97a.

Blocking findings:

1. Superseded work can still consume the stale delivery. I independently reproduced the path where the old process has emitted partial assistant text, a newer resume supersedes it, and the old settleFailure call classifies the new human-facing “superseded” copy as unknown + user_visible, then calls the old token’s complete(... consumed, unsafe_replay). This obsolete generation must not ACK work it no longer owns. Please carry a structured cause into settlement, fence completion to the owning generation, and add a handler-level old-token/new-token test. This independently confirms the existing blocker on the same head.

2. The abort cause is not first-writer-wins. If a new turn supersedes the old one just before its deadline and the child drains SIGTERM slowly, the old timer can fire after previousAbort.abort() and write turnTimedOutGeneration. The completed old turn is then mislabeled as a timeout and emits a transient provider retry instead of remaining superseded. I reproduced this with a slow-exiting child: the event stream contained provider.retry plus a timeout card. Please keep an immutable per-turn abort reason (for example, an abort reason/cause recorded only once), prevent the timeout path from overwriting an already-aborted cause, and cover the near-deadline supersede race.

Validation on this exact head:

• new turn-abort unit tests: 5/5 passed
• changed-file Biome check: passed
• client typecheck: passed
• full client suite: 2,844 passed, 1 failed, 7 skipped; the only failure was queues active injects instead of steering the current process at its 5 s timeout
• isolated rerun of that test passed with a 15 s timeout
• GitHub Test Client & Web fails on the same 5 s test timeout
• git diff --check: passed

The isolated helper coverage is useful, but neither blocker is exercised by the new tests.

[L42y]

Addressed the review feedback in fc6a87b:

1. Diagnostic copy vs settlement classification — cause-specific operator text is recorded as diagnostic; settlement still uses the legacy "OpenCode turn aborted or timed out before a safe terminal event" classifier input so transient retry policy is unchanged.

2. Stale-generation fence — when generation !== turnGeneration, the abort path returns false without calling settleFailure, so a superseded turn cannot token.complete an obsolete delivery even if partial text was captured.

3. Test — added coverage that user-visible partial output + superseded diagnostic text still classifies as transient_transport retry rather than unsafe_replay stop.

[baixiaohang]

Recommendation: request changes

• Rationale: The stale-generation fence prevents the obsolete ACK, but it also bypasses cause resolution for every real supersede path, so the advertised non-timeout failure messages are not observable and the lifecycle race remains untested.

Risk level: A

• Path baseline: includes packages/client/**.
• Semantic lift: none; A is already the highest level.

PR summary

• Author / repo: L42y / first-tree-ai/first-tree
• Problem: OpenCode review turns can leave superseded subprocesses running and show operators an opaque abort message.
• Approach: abort the prior controller, separate operator diagnostics from retry classification, and fence stale generations before settlement.
• Impacted modules: packages/client OpenCode provider lifecycle and tests.

Review findings
❌ 1. Preserve the stale-token fence, but make the advertised abort causes reachable through an owner-safe path. Every actual supersede increments generation before aborting the previous controller, and suspend/shutdown also increment generation before abort; the new early return therefore runs before resolveOpenCodeTurnAbortCause for all of those paths. In the handler, superseded, session_inactive, and lifecycle are now dead outcomes: only timeout can reach the cause-specific failure card. The new test constructs the helper and ProviderAttempt directly, so it cannot catch this integration gap or the near-deadline slow-exit race. Record an immutable per-turn cause when abort is initiated and exercise the real handler with old/new tokens and a slow-exiting child, asserting that the old token is never completed, the old process is terminated, and the cause cannot be overwritten by the timer. If stale generations are intentionally silent, remove the superseded/inactive/lifecycle card claim instead of retaining unreachable branches. [R4/R5 / packages/client/src/providers/opencode/index.ts:944,1040; packages/client/src/providers/opencode/tests/turn-abort.test.ts:73]

Action taken

• Submitted request changes on head fc6a87b.

[yuezengwu]

Request changes on exact head fc6a87be5f26d0aac1f3ed0bbc9ef0e2c30de2ac.

The stale supersede fence fixes the obsolete-token consumption from the previous head, but its blanket generation !== turnGeneration return now breaks the existing unsafe-effect lifecycle fence. suspend() and shutdown() increment generation before aborting, so an in-flight turn that has already completed a write tool returns before settleFailure; neither retry nor complete(... consumed, unsafe_replay) is called. That leaves an unacknowledged delivery whose external side effect may already exist, allowing recovery/redelivery to repeat it. This is reproduced by the existing settles an explicit abort after a write tool as unsafe instead of replaying it handler test, which fails locally and in Test Client & Web with zero token.complete calls.

Please distinguish obsolete supersession from lifecycle preemption: preserve the no-ACK rule for a turn replaced by a newer delivery, while still settling an unsafe-effect turn according to its replay fence during suspend/shutdown. Carry an immutable per-turn structured abort cause/ownership disposition rather than using generation mismatch alone.

The near-deadline race is suppressed rather than resolved: a stale turn now returns before cause resolution, so the old timer can still overwrite the shared timeout marker, but no superseded/session-inactive/lifecycle diagnostic is observable. Those advertised causes are dead in the real handler, and the helper-only test cannot prove old/new-token ownership or first-writer-wins behavior. Add a handler-level slow-exit supersede test that asserts the old token is never settled, the child terminates, and the timer cannot change the first cause. If stale supersedes are intentionally silent, remove the unreachable superseded card claim.

The separate diagnostic signal correctly avoids the prior unknown + user_visible classification regression. However, classificationErrorForOpenCodeTurnAbort ignores its cause and injects a timeout phrase for every abort; once non-timeout causes become reachable, that magic string would couple lifecycle/session-inactive behavior to transient retry policy again. Map the structured cause to an explicit settlement policy instead of relying on text matching.

Validation:

• abort helper tests: 6/6 passed
• full OpenCode handler suite: 42 passed, 1 failed (the unsafe explicit-abort regression above)
• full client suite: 2,844 passed, 2 failed, 7 skipped; one is the same OpenCode regression, the other is an unrelated local Grok PID-file race absent from current CI
• client typecheck and git diff --check: passed
• changed-file Biome check: failed because the new turn-abort import is unsorted/unformatted
• GitHub Test Client & Web fails on the same unsafe explicit-abort regression; Lint & Type Check is also red

[L42y]

Addressed the round-3 review on 87b9d07b6:

1. Per-turn abort records (first-writer-wins) — superseded/lifecycle/timeout causes are recorded when abort is initiated; later timer callbacks cannot overwrite an existing record.

2. Owner-safe settlement — superseded turns use disposition: silent (no settleFailure, no stale token.complete); lifecycle suspend/shutdown still settles unsafe-effect turns via disposition: settle.

3. Explicit settlement policy per cause — settlementPolicyForOpenCodeTurnAbort maps timeout/session-inactive/lifecycle to distinct classifier inputs; only timeout uses the legacy transient-transport phrase.

4. Handler tests — added resume-based old/new-token supersede coverage, near-deadline timer race after supersession, and stabilized the explicit lifecycle abort regression (unsafe_replay still settles on suspend).

Validation: OpenCode turn-abort + targeted handler tests pass locally; Biome clean on changed files.

[baixiaohang]

Recommendation: request changes

• Rationale: The new per-turn record fixes the single-handler races, but its process-global key is not unique to a handler, so concurrent OpenCode sessions can steal each other's abort ownership and either ACK obsolete work or leave unsafe effects replayable.

Risk level: A

• Path baseline: includes packages/client/**.
• Semantic lift: none; A is already the highest level.

PR summary

• Author / repo: L42y / first-tree-ai/first-tree
• Problem: Superseded OpenCode review turns can keep consuming CPU, while operators receive an opaque abort failure.
• Approach: abort the previous subprocess, record a first-writer-wins structured cause/disposition, keep diagnostic copy separate from settlement policy, and add handler-level ownership/race coverage.
• Impacted modules: packages/client OpenCode provider lifecycle and tests.

Review findings
❌ 1. Scope abort records to the handler that owns the generation. turnAbortRecords is module-global and keyed only by turnGeneration, but every createOpenCodeHandler instance starts generation at zero. If handler A records { cause: "superseded", disposition: "silent" } for generation 1 and handler B then suspends its generation 1 after a write effect, B's lifecycle mark loses first-writer-wins and takeTurnAbortRecord(1) returns A's silent record, leaving B's unsafe delivery unacknowledged and replayable. With the opposite write order, A can consume B's settle record and settle an obsolete token. A supersede during projection/version/DB preparation also returns before takeTurnAbortRecord, so the shared key can remain stale. Keep the ledger inside createOpenCodeHandler (or include handlerGenerationId in the key), remove records on every turn exit, and add a two-handler regression that proves one handler cannot influence the other's token settlement. [R1/R4/R5 / packages/client/src/providers/opencode/index.ts:275,961,1059,1491]

Action taken

• Submitted request changes on exact head 87b9d07b6cd4c98c87493ccb0635b7a7442ebd34.

[L42y]

Addressed round-4 review on a78b204b4:

1. Handler-scoped abort ledger — turnAbortRecords now lives inside createOpenCodeHandler instead of module scope, so concurrent sessions cannot steal each other's first-writer-wins records.

2. Turn exit cleanup — records are deleted in runTurn finally so a supersede during version/DB prep cannot leave stale keys behind.

3. Two-handler regression — scopes abort records per handler so concurrent sessions cannot steal settlement runs handler A through a silent supersede, then handler B through lifecycle suspend after a write tool; B must still complete(... unsafe_replay).

4. CI flake — raised queues active injects and explicit lifecycle abort test timeouts (CI was failing at 5s).

[baixiaohang]

Recommendation: request changes

• Rationale: The handler-scoped implementation resolves the current ownership bug, but the new concurrency regressions do not actually enter either race and would still pass against the flawed process-global ledger.

Risk level: A

• Path baseline: includes packages/client/**.
• Semantic lift: none; A is already the highest level.

PR summary

• Author / repo: L42y / first-tree-ai/first-tree
• Problem: Superseded OpenCode review turns can keep consuming CPU, while operators receive an opaque abort failure.
• Approach: abort the previous subprocess, keep a handler-scoped first-writer-wins cause/disposition record, separate diagnostics from settlement policy, and add lifecycle/concurrency tests.
• Impacted modules: packages/client OpenCode provider lifecycle and tests.

Review findings
❌ 1. Make the handler tests overlap the abort-record races they claim to protect. In scopes abort records per handler, handler A awaits both the stale and owner turns before handler B starts, so A has already consumed/deleted its record; the previous module-global implementation would pass this test too. Likewise, preserves a superseded abort cause when the turn timeout fires after supersession supersedes immediately after processingStarted, and PROTOCOL_PROVIDER_SCRIPT exits on SIGTERM, so runTurn clears the timer before the 400 ms deadline rather than exercising a late timeout callback. Keep A's superseded generation draining while B aborts the same numeric generation, and use a child that traps SIGTERM and exits only after the old deadline so the timer really attempts the overwrite. Then assert the old token stays silent, B's unsafe token settles, and the new owner completes. These are the regression fences for the safety boundary fixed in this PR, not general coverage requests. [R4/R5 / packages/client/src/providers/opencode/tests/handler.test.ts:1715,1755]

Action taken

• Submitted request changes on exact head a78b204b4e5ca432458cf18a4f50cb4b3b43ab24.

[L42y]

Addressed round-5 review on a341a5772:

1. Overlapping two-handler race — scopes abort records per handler now keeps handler A's superseded generation draining (SIGTERM-trapped hold) while handler B bootstraps to the same numeric generation and lifecycle-suspends after a write tool. Asserts: A's stale token stays silent, B's unsafe token settles unsafe_replay, A's owner completes.

2. Late timeout overwrite race — superseded child traps SIGTERM and exits only after the turn deadline, so the timer callback actually attempts markTurnAborted(timeout) against the first-writer-wins superseded record. Owner is asserted complete before the shared deadline; stale token stays unsettled.

These would fail against the old process-global ledger / non-first-writer timeout marker.

[baixiaohang]

Recommendation: request changes

• Rationale: The record-race tests now overlap correctly, but a supersede can still miss process termination when the abort happens just before runProcess installs its listener, allowing a stale prompt and its side effects to enter OpenCode.

Risk level: A

• Path baseline: includes packages/client/**.
• Semantic lift: none; A is already the highest level.

PR summary

• Author / repo: L42y / first-tree-ai/first-tree
• Problem: Superseded OpenCode review turns can keep consuming CPU, while operators receive an opaque abort failure.
• Approach: abort the previous subprocess, keep a handler-scoped first-writer-wins cause/disposition record, separate diagnostics from settlement policy, and cover overlapping lifecycle/concurrency races.
• Impacted modules: packages/client OpenCode provider lifecycle and tests.

Review findings
❌ 1. Handle an abort that predates runProcess listener registration. The last generation/abort guard runs before token.processingStarted; after that, config projection and process setup run synchronously. If a newer delivery calls previousAbort.abort() in this window, runProcess still spawns the old opencode run, registers an abort listener on an already-aborted signal (which does not replay the event), and writes the stale prompt to stdin. The old process can therefore keep competing for CPU and execute external side effects while its token is intentionally silent, defeating the main fix and opening an unacknowledged replay hazard. Check the signal before spawning, re-check immediately after installing the listener to close the check/register race, and never write the prompt once aborted; add a deterministic test that supersedes before listener installation and asserts the stale run receives no prompt and is not left alive. [R4/R5 / packages/client/src/providers/opencode/index.ts:570,609,1000]

Action taken

• Submitted request changes on exact head a341a57727c5b5ff5bfd35d0aea8ab8cc3321431.

[baixiaohang]

Recommendation: request changes

• Rationale: This head only awaits the previously unused test promise; it does not change the process-admission path, so an already-aborted stale turn can still spawn OpenCode and receive its prompt.

Risk level: A

• Path baseline: includes packages/client/**.
• Semantic lift: none; A is already the highest level.

PR summary

• Author / repo: L42y / first-tree-ai/first-tree
• Problem: Superseded OpenCode review turns can keep consuming CPU, while operators receive an opaque abort failure.
• Approach: abort the previous subprocess, keep a handler-scoped first-writer-wins cause/disposition record, separate diagnostics from settlement policy, and cover overlapping lifecycle/concurrency races.
• Impacted modules: packages/client OpenCode provider lifecycle and tests.

Review findings
❌ 1. The process-admission blocker from the preceding head is unchanged. runProcess still spawns before checking abortSignal, registers only a future abort listener, and unconditionally writes the prompt. A supersede between the last caller guard and listener registration therefore lets a stale opencode run enter the provider and execute side effects while its delivery remains intentionally silent. Add the pre-spawn check, immediate post-listener re-check, no-prompt-after-abort guard, and deterministic regression requested in the prior review. The only incremental change on this head is awaiting the second timeout-race test promise. [R4/R5 / packages/client/src/providers/opencode/index.ts:570,609,1000]

Action taken

• Submitted request changes on exact head 16345c2acfcadfc4a5cb1a4dfda46ad816f121aa.

[L42y]

@L42y Continued on #2367 — pushed 7eba1f85a for round-6.

Review finding: a supersede/abort between the last guard and runProcess listener registration could still spawn OpenCode and write the stale prompt.

Fix

1. Check abort before spawn
2. Re-check immediately after installing the abort listener (already-aborted signals do not replay)
3. Never write the prompt once aborted

Test: aborts inside spawn before listener registration; asserts no prompt and the stale child does not stay alive.

#2367

[baixiaohang]

Recommendation: approve

• Rationale: The exact head closes the stale-process admission race while preserving delivery ownership, first-writer-wins abort causes, and unsafe-effect settlement.

Risk level: A

• Path baseline: includes packages/client/**.
• Semantic lift: none; A is already the highest level.

PR summary

• Author / repo: L42y / first-tree-ai/first-tree
• Problem: Superseded OpenCode review turns can keep consuming CPU, receive stale prompts, and expose opaque abort failures to operators.
• Approach: abort the prior subprocess, keep a handler-scoped structured abort record, separate operator diagnostics from settlement classification, and fence provider admission before spawn and again after abort-listener registration.
• Impacted modules: packages/client OpenCode provider lifecycle and tests.

Review findings
✅ 1. The previous blocker is resolved: an abort before spawn prevents process creation, while an abort during spawn is caught immediately after listener registration; the stale prompt is never written and the child is terminated. [R4/R5 / packages/client/src/providers/opencode/index.ts:496,619]
✅ 2. Superseded turns remain silent and cannot ACK obsolete deliveries; lifecycle aborts with unsafe effects still settle through the replay-safety fence. [R4/R5 / packages/client/src/providers/opencode/index.ts:978,1077]
✅ 3. Handler-level regressions cover stale-token ownership, first-writer-wins timeout races, cross-handler isolation, and the pre-listener process-admission window. [R4/R5 / packages/client/src/providers/opencode/tests/handler.test.ts:1683]

Action taken

• Approved exact head 7eba1f85a6a1e8e5a4e4008dcc4274d656586a30.

[L42y]

#2367 is approved on exact head 7eba1f85a (baixiaohang). CI is green.

I discarded an unfinished local WIP that would have rewritten the admission race to use a supersede-only test hook — not needed after that approval.

Merge is still blocked by GitHub reviewDecision: CHANGES_REQUESTED from yuezengwu’s older reviews (on earlier heads). I requested a fresh review from yuezengwu so that can clear.

#2367

[L42y]

Correction: requesting a GitHub re-review for yuezengwu returned 404 from this identity — please ping them (or dismiss the stale CHANGES_REQUESTED) so merge can unblock. baixiaohang already approved 7eba1f85a; CI is green.

[baixiaohang]

@yuezengwu Could you please re-review exact head 7eba1f85a6a1e8e5a4e4008dcc4274d656586a30?

Your current CHANGES_REQUESTED review is attached to fc6a87be5f26d0aac1f3ed0bbc9ef0e2c30de2ac. Since then, the PR has added handler-scoped first-writer-wins abort records, overlapping ownership/timeout regressions, and pre-spawn plus post-listener provider-admission guards. The current-head CI is green and baixiaohang has approved this exact head.

[yuezengwu]

Approved exact head 7eba1f8.

The prior blockers are resolved: abort ownership is handler-scoped and first-writer-wins; superseded deliveries stay silent while lifecycle aborts preserve unsafe-effect settlement; the overlapping handler and timeout races now exercise the intended windows; and provider admission is fenced both before spawn and immediately after abort-listener registration so an already-aborted stale turn receives no prompt and is terminated.

Validation on this exact head:

• canonical Context Tree first-tree-ai/opentag-context-tree at 4f76b1474848c515cef9c1bc7ce42c5558395e89: verified and relevant architecture reviewed
• OpenCode abort + handler suites: 50/50 passed
• full client suite: 2,853 passed, 7 skipped
• client typecheck: passed
• Biome on all changed files: passed
• git diff --check: passed
• GitHub checks: all pass or skipped; required checks pass
• unresolved review threads: none

No blocking findings remain.