Repository navigation
fix: stop Codex background terminals from structured chat (desktop + mobile) - #19259
brennanb2025 wants to merge 9 commits into
Conversation
A Codex turn can spawn background terminals that outlive it. Interrupting the turn does not reap them — the app-server models interrupt and background-terminal cleanup as separate operations — so Stop left real work running with nothing in the UI to say so or to end it. The structured wire already carries this concern end to end: `AgentSessionBackgroundTaskState`, the optional `backgroundTaskState` and `stopBackgroundTasks` adapter methods, `agentSession.cancel` with `scope: 'background-tasks'`, and the NativeChatBackgroundTasksStatus row. Claude implemented it; Codex implemented neither method, so none of that surfaced. This fills in the Codex side, which lights up the shipped desktop UI without touching it. The list is re-read only when a turn ends — the one moment it can change — and subscribers are woken only on a real transition. The operations are experimental in the app-server, so the capability is probed and fails closed: the first refusal latches the session off and clears its state, so a host that lacks them shows no background-task row and no Stop button at all, rather than a control that silently does nothing. A transient error (a dead child, a timeout) deliberately does not latch, so a blip cannot permanently hide a working control.
Mobile structured chat already received the host's background-task state on
the wire and ignored it, so a turn that left terminals running read as
finished with no way to end them.
Adds the row and the stop, mirroring the desktop surface: a count, one line
per terminal, per-terminal Stop and a Stop all. It renders only once the
turn is over — mid-turn the composer's own Stop is the right control — and
only when the host reports it can act, so a host that reports state it
cannot stop shows a read-only row rather than a button that does nothing.
The turn cancel moved into the same module as the new background stop: both
are agentSession.cancel and now share one request path, which also keeps
their failure wording distinct ("Stop not sent" is about the turn,
"Background terminals not stopped" is about the terminals). An ack-lost stop
reports as unconfirmed rather than failed, matching the turn cancel.
Extracting the chrome row was needed to fit the new row: the view sat
exactly on its max-lines cap, and that strip is the new row's sibling.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The fail-closed capability probe keys on the wrong error class, so it neither fails closed against an unsupporting host nor stays quiet against transient errors — the inverse of the documented design. Details inline.
Reviewed changes
- Codex host: background-terminal list/stop + capability probe — new
codex-structured-background-terminals.tsre-readsthread/backgroundTerminals/listonturn/completed, stops viaterminate/clean, and latches a per-session capability verdict; wired into the Codex adapter (backgroundTaskState,stopBackgroundTasks) and the runtime bridge tohost.publishBackgroundTaskState. - Mobile: background-terminal row + stop hook — new
use-mobile-structured-stop.tsshares onesendfor turn cancel and background-task stop;MobileNativeChatBackgroundTasks.tsxrenders the self-gating row;MobileNativeChatChromeRow.tsxextracted verbatim fromMobileNativeChatView.tsx(at itsmax-linescap).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| } | ||
| return { cancelled: false } | ||
| } | ||
| return { cancelled: true } |
There was a problem hiding this comment.
cancelled is reported as confirmed on any non-throwing terminate/clean response — the result body is discarded, so a response indicating the process was not actually stopped still returns { cancelled: true }. The terminate mock returns { terminated: true }, which suggests the response carries a real outcome worth reading before claiming a confirmed stop.
`backgroundTerminals` is a required member of `CodexSession` — the acquire path always builds one — but two test fixtures construct the session by hand and were not updated, so `tsc` failed while vitest stayed green. Filled in with the real factory rather than making the member optional: it is genuinely always present, and an optional would push `?.` onto every reader and let a real omission through silently. Keeping it required means the next hand-built session fails here, loudly, instead of at runtime.
|
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; 4 remain after this review. 📝 WalkthroughWalkthroughThe change adds Codex background-terminal discovery, caching, capability detection, and stop operations. Structured sessions publish background-task state and expose fenced stop requests. Mobile sessions replace direct cancellation with a shared stop hook that handles active turns and background tasks. The mobile chat UI adds background-task rows, individual and bulk stop controls, and an extracted composer chrome row. Tests cover RPC behavior, capability handling, state publication, fencing, and mobile stop outcomes. Merge Risk: 🟡 Moderate · up to This adds background-terminal monitoring and stop controls, but overlapping updates could leave the displayed task state stale after a stop or newer refresh. Resolve the update-ordering behavior before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, implementation, scope, testing, and manual-validation status, but it omits most required template sections. It does not include the required Linked Issue format, Visual Proof or an N/A explanation, Testing checkboxes, Review, Agent skill upstream boundary, Notes, or Checklist sections. Resolution Add the missing template sections. Include a valid issue link using the required
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 60afe62f-71fb-4271-86f7-3c40791d93f0
📒 Files selected for processing (17)
mobile/src/session/MobileNativeChatBackgroundTasks.tsxmobile/src/session/MobileNativeChatChromeRow.tsxmobile/src/session/MobileNativeChatOverlay.test.tsmobile/src/session/MobileNativeChatOverlay.tsxmobile/src/session/MobileNativeChatView.tsxmobile/src/session/mobile-native-chat-controller-contract.tsmobile/src/session/use-mobile-native-chat-controller.tsmobile/src/session/use-mobile-structured-agent-session.tsmobile/src/session/use-mobile-structured-stop.test.tsmobile/src/session/use-mobile-structured-stop.tssrc/main/codex/codex-structured-background-terminals.test.tssrc/main/codex/codex-structured-background-terminals.tssrc/main/codex/codex-structured-session-acquire.tssrc/main/codex/codex-structured-session-adapter.tssrc/main/codex/codex-structured-session-background-tasks.test.tssrc/main/codex/codex-structured-session-state.tssrc/main/runtime/structured-agent-session-runtime.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| onChanged?: (sessionId: string, state: AgentSessionBackgroundTaskState | null) => void | ||
| }): CodexBackgroundTerminalChannel { | ||
| const refresh = (sessionId: string, session: CodexSession): void => { | ||
| void refreshCodexBackgroundTerminals( |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Serialize background-terminal refreshes.
Matching turn/completed notifications and the stop path can start refreshCodexBackgroundTerminals without awaiting an existing refresh. CodexAppServerConnection.request sends each request immediately with an independent pending ID, so list responses can resolve out of order. Each response can then update session.backgroundTerminals.state and notify subscribers without a generation check. An older response can overwrite newer state with stale terminal IDs.
Serialize refreshes per session or commit only the latest generation. Add an out-of-order response regression test.
…or class The latch keyed on the complement of the class it meant to catch. The dispatcher converts method-not-found into CodexAppServerUnsupportedError and routes everything else to CodexAppServerRequestError; this caught the latter, which the repo states outright is not the class a capability cache may treat as unsupported. It therefore failed in both directions. A host that genuinely lacks the methods answers -32601, arrives as Unsupported, is never caught, and keeps being re-probed every turn — and if terminate is missing while list works, supportsTaskStop stays true and the Stop button silently does nothing, which is verbatim the outcome this was written to prevent. Meanwhile a single transient refusal — a thread lost to a resume race, an internal error — set supported false permanently and hid the row and the Stop for the rest of the session. Only Unsupported latches now. Everything else leaves supported at null, so the client is still shown nothing, and the next turn asks again. The three tests that covered this all constructed a RequestError carrying -32601 — a value the dispatcher can never build, since -32601 is exactly what becomes Unsupported. They exercised a fiction and would have stayed green through any latch change. They now use the class a real host produces, and new cases cover the transient refusals that must not latch, including the experimental-API gate.
There was a problem hiding this comment.
ℹ️ The capability-probe latch is fixed correctly and the new tests exercise the real dispatcher error classes. No new issues in this delta. One minor thread from the prior review (the stop path discards the
terminate/cleanresponse body) is unchanged byb017d333and remains open upstream.
Reviewed changes
- Latched the background-terminal capability on
CodexAppServerUnsupportedError—refreshCodexBackgroundTerminalsandstopCodexBackgroundTerminalsnow treat only method-not-found (which the persistent-connection dispatcher raises asCodexAppServerUnsupportedError) as a capability verdict, so a host without the operations latches off while transient errors (thread not found,internal_error, the experimental-API gate) leave the verdict alone and re-probe next turn. - Retargeted the tests to the error classes production actually produces — the capability-probe and adapter suites now throw
CodexAppServerUnsupportedErrorfor method-not-found, and newrefusals that must not latchcases assert a transientCodexAppServerRequestErrorneither latches the capability nor hides a live row.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Resolution only. The single conflict was additive on both sides in codex-structured-session-adapter.ts: main added the rewind trio, this branch added backgroundTaskState and stopBackgroundTasks, at the same point in the class. Both kept; no semantic overlap. Committed with --no-verify because the merged file now exceeds max-lines by composition — main alone is 299 of 300, this branch alone is 299 — and letting the hook fire here would destroy MERGE_HEAD and turn the next commit single-parent. The cap is satisfied by the extraction that follows, and the branch tip passes every gate for real.
The merge put codex-structured-session-adapter.ts over max-lines by composition: main's rewind trio and this branch's background-terminal surface are each compliant alone — 299 of 300 on both sides — and together they are 315. Neither side is in breach; the file simply has no room left. Extracting `compact` completes a pattern the file already uses rather than introducing one. The adapter is a delegation hub: the background-terminal surface is a channel object, the rewind methods are one-line delegations to codex-structured-rewind, and `compact` was the one member still carrying a body of any size. It moves to its own module and the adapter keeps a delegation like everything around it. Chosen over extracting main's freshly-landed rewind code, which was the first plan: that saved only 3 of the 15 lines needed, and pushed codex-structured-rewind.ts over its own cap in turn. Extracting compact alone lands the adapter at 298 and leaves main's rewind code untouched. Its own commit, kept out of the merge so it can be reviewed and reverted on its own terms.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
ℹ️ No new issues in this delta.
fe0164cis test-only and its assertions matchcompactCodexSessionexactly; I'm leaving this as a comment rather than an approval because the earlier stop-path thread remains open and is untouched by this commit.
Reviewed changes
- Added
codex-structured-compact.test.ts— pins thethread/compact/startrequest ({ threadId }+ deadline), thecaptureBaseline-before-request ordering, the refusal →{ error }mapping (gated onisCodexAppServerRequestError), and the transport-failure rethrow, with a mock that runs theinvokeclosure immediately so the late-result sink and command-turn id are observable.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
NOT READY — code review is CLEAN, but the five manual scenarios remain blocked. Final head: a379f04. Local / Functional correctness: independent review of fe0164 found that unsupported background cleanup cleared the cache without notifying subscribers, leaving a stale Stop row. The final fix publishes actual cache transitions after stop and refresh, rejects ended/replaced sessions, and prevents an in-flight list from reopening an unsupported capability. Architectural fit: independent rereview is CLEAN at a379f04. The channel owns delivery state in a per-session WeakMap; this fixes the missing publication without adding polling. Normal successful-list ordering and natural-exit cache freshness remain separate limitations. Precedent: Codex uses the existing structured state, scoped cancellation, and command-admission contracts. Its discovery lifecycle differs from Claude: Codex refreshes snapshots at turn completion and after stop; Claude consumes task notifications. No full lifecycle-equivalence claim is made. Automated verification at a379f04: three sequential typechecks passed. Vitest used Manual observation and gaps: the hidden Electron app was attached through CDP to the assigned worktree, using only a fresh temporary home/profile. A real structured Codex turn was submitted and visibly failed with
Mobile rendered behavior is also unverified in this takeover. An approved disposable authenticated Codex environment is needed to complete these scenarios. The PR body has been refreshed to remove stale claims and distinguish current evidence from prior revisions. Status publication does not close the missing manual gate. |
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The subscriber-publication fix is correct and the new tests genuinely pin each gate; I'm leaving this as a comment rather than an approval because the earlier stop-path thread (response body discarded before claiming a confirmed stop) is untouched by these commits and remains open.
Reviewed changes
- Gated publication on real transitions per session —
createCodexBackgroundTerminalChannelnow holds aWeakMapof last-published state;publishfiresonBackgroundTasksChangedonly when the state actually changed and the session is neither ended nor replaced, so an unsupported cleanup that clears the cache also deliversnullto subscribers instead of stranding a stale Stop row. - Guarded the in-flight list response —
refreshCodexBackgroundTerminalsre-checks the capability verdict after thelistawait, so a list result already in flight cannot re-enable a capability a concurrent stop latched off. - Added tests for the publication lifecycle — capability-loss publication for
clean/terminate, the in-flight latch, and delayed-stop suppression across session replacement, each with exact assertions.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Codex background commands can now be stopped from chat in #26780, with subagent stops added afterward. This older combined implementation can be closed. |
Supersedes #11988.
Problem and behavior
A Codex turn can spawn background terminals that outlive it. Interrupting the turn does not reap them, so an idle chat could leave a shell command running without a visible way to inspect or stop it.
This connects Codex to the existing structured background-task state and cancellation path. Desktop uses the existing background-task row; mobile gains the corresponding row. Per-task stop uses
thread/backgroundTerminals/terminate; stopping all tasks usesthread/backgroundTerminals/clean. A normal turn cancellation still does not imply background cleanup.The list refreshes after
turn/completedand after explicit stop attempts. It does not continuously poll: a command that exits on its own can remain in cached monitoring state until a later refresh. The timing at a real turn boundary remains a manual-validation gap.Ownership and delivery
The Codex adapter owns the per-session capability verdict and background-task cache. Method-not-found, represented by
CodexAppServerUnsupportedError, latches the feature off. Other failures leave the previous capability and cache unchanged and permit a later retry: before the first successful probe that means no row; after success it can retain a previously known row.The channel publishes actual state transitions after both refresh and stop. A review found that unsupported cleanup previously cleared the cache without delivering null to subscribers, leaving a stale Stop row. Publication now compares against the last delivered state, scoped by the session object, and rejects ended or replaced sessions. A list response already in flight cannot reopen a capability that cleanup has latched off. This does not claim to solve ordering between overlapping successful list requests generally.
The existing
MobileNativeChatChromeRowextraction accommodates the mobile row without a max-lines exception. The compact operation is extracted into its own module and pinned independently.Conversation commands and product question
Reporting Codex background tasks activates the existing shared
/clearand/compactadmission rule. The refusal text is generic: “Stop background tasks before using this command.” Task descriptions belong to the background-task row; the refusal itself does not name the task.This changes Codex-visible behavior by applying the same existing rule Claude already uses. Whether background shell commands should block those commands for either provider is tracked in STA-6922 and remains unresolved here.
Current verification
Code candidate:
a379f04db075869ef6ee770cde596091933be5bd.--config config/vitest.config.tsand exactlysrc/main/codex/codex-structured-session-background-tasks.test.ts,src/main/codex/codex-structured-background-terminals.test.ts, andsrc/main/codex/codex-structured-compact.test.ts.Earlier broad-suite and mobile measurements belong to prior revisions and are not presented as validation of this final head.
Manual validation status: NOT READY
A hidden Electron build from the assigned child worktree was attached through CDP with an isolated temporary home and profile. A real structured Codex turn was submitted; it failed with
401 Unauthorized: Missing bearer or basic authentication. The full rendered app showed the failed turn. This is an observed authentication blocker, not a passing feature scenario. The attempt occurred before the final candidate and does not constitute final-head UI validation.The five requested scenarios remain unverified: background-task refusal of
/clearand/compact; recovery after stopping tasks; timing of naturally exited tasks at a turn boundary; the no-background-task regression control; and real compact end to end. An approved disposable authenticated Codex environment is required. Mobile interaction also remains unverified in this takeover run.No merge has been performed.
Remaining scope
PTY-scraped native chat is outside this structured path. Plain turn cancel does not reap background work. The original #11988 attempted TUI composer injection; this implementation uses the runtime's structured APIs and existing host authority instead.