Repository navigation
Record Stop immediately and settle subagents from exit proof - #28051
Conversation
2d46899 to
6ba01b3
Compare
d070825 to
ef1ad55
Compare
Main now carries #28046's startup design (squash-merged), so its hunks match the previous merge; this adds main's newer changes. Conflicts took the Stop branch's side, since main's text in each conflicted file equals the startup branch tip already reconciled. Tests whose premise the Stop change moved: a Codex Stop now ends the app server, so the replayed-Stop race resumes the saved thread on a new child; a Stop before startup now records its request beside the taken-back send's stopped result. The background-task test regains the tracker import its proof cases use.
The two #29905 cases lived in the turns test this branch removed. They pin that a listed Stop never ends a helper lifetime it did not capture: a same-id restart during acceptance, or a refused helper that appears mid Stop-all, which must not close the agent.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. ℹ️ Recent review info
Walkthrough
Priority: Merge Risk:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments: 1
src/main/codex/codex-app-server-teardown.integration.test.ts: 1
147-155: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRequire the detached grandchild to be reaped on supported POSIX runs.
The suite excludes Windows, and the fixture keeps the provider root alive while
close()captures the process tree. The currentelsebranch accepts an unproven result without checking the grandchild. A reaper regression can therefore pass while the grandchild remains alive. Require the exited-tree result and the grandchild's absence.♻️ Suggested tightening
const rootExited = await connection.close() expect(rootExited).toBe(true) - if (connection.closeResult?.tree === 'exited') { - await vi.waitFor(() => expect(processExists(pids.grandchild)).toBe(false)) - expect(connection.closeResult).toMatchObject({ root: 'exited', tree: 'exited' }) - } else { - expect(connection.closeResult?.tree).not.toBe('exited') - expect(connection.processTreeUnproven).toBe(true) - } + expect(connection.closeResult).toMatchObject({ root: 'exited', tree: 'exited' }) + await vi.waitFor(() => expect(processExists(pids.grandchild)).toBe(false)) + expect(connection.processTreeUnproven).toBe(false)
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b63f489d-9884-4676-85d2-cd7769633edf
📒 Files selected for processing: 77
config/scripts/vitest-sqlite-runtime-files.mjssrc/main/acp/acp-structured-host-lifecycle.test.tssrc/main/acp/acp-structured-host-start-aborts.test.tssrc/main/acp/acp-structured-session-adapter.tssrc/main/agent-hooks/server/server-status-application.tssrc/main/agent-hooks/server/server-status-update.tssrc/main/claude/claude-close-turn-end.test.tssrc/main/claude/claude-scripted-child.test-fixture.tssrc/main/claude/claude-structured-prompt-ownership.tssrc/main/claude/claude-structured-session-adapter.tssrc/main/codex/codex-app-server-connection-tree-unproven.test.tssrc/main/codex/codex-app-server-connection-types.tssrc/main/codex/codex-app-server-connection.test.tssrc/main/codex/codex-app-server-connection.tssrc/main/codex/codex-app-server-teardown.integration.test.tssrc/main/codex/codex-background-task-tracker.tssrc/main/codex/codex-child-work-evidence.test.tssrc/main/codex/codex-child-work-evidence.tssrc/main/codex/codex-request-end-wait.tssrc/main/codex/codex-structured-session-adapter.tssrc/main/codex/codex-structured-session-background-tasks.test.tssrc/main/codex/codex-structured-session-close.test.tssrc/main/codex/codex-structured-session-close.tssrc/main/codex/codex-structured-session-teardown.tssrc/main/codex/codex-structured-turn-cancellation.tssrc/main/daemon/pty-subprocess-spawn-file-foreground.test.tssrc/main/daemon/terminal-host-process-inspection.test.tssrc/main/github/github-remote-identity-parsing.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter-stop.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter.tssrc/main/native-chat/agent-session-wire/structured-agent-session-chat-stop.tssrc/main/native-chat/agent-session-wire/structured-agent-session-child-close.tssrc/main/native-chat/agent-session-wire/structured-agent-session-claude-hung-start-stop.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-codex-stop-note-end.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-codex-stop-row.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-conversation-stop.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-create.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-listed-stop-lifetime.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-prompt-cancel.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-card-holds.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-message-rig-provider.test-fixture.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-pause-lift.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-stop-row.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-repeated-stop.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-startup-attempt-host.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-steer-while-stopping.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-event-binding.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-event-entries.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-event-restart.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-event.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-note-opened-turn.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-note-withdrawn-send.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-receipts.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-settle-binding.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-wind-down.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stopped-history.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stopping.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-turn-stop-notes.tssrc/main/native-chat/agent-session-wire/structured-agent-session-turns-cancel.tssrc/main/native-chat/agent-session-wire/structured-agent-session-turns.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-unnamed-stop-races.test.tssrc/main/native-chat/agent-session-wire/structured-conversation-compaction.test.tssrc/main/pi/rpc-session-adapter.tssrc/main/providers/local-pty-child-process-verdict.test.tssrc/main/providers/local-pty-provider-foreground-process.test.tssrc/main/runtime/structured-agent-session-codex-compact-exit.test.tssrc/main/runtime/structured-agent-session-codex-interrupt-order.test.tssrc/main/runtime/structured-agent-session-codex-scripted-child.test.tssrc/main/runtime/structured-agent-session-codex-stopped-send-order.test.tssrc/main/runtime/structured-agent-session-codex-turn-end-settlement.test.tssrc/main/runtime/structured-agent-session-integration.test.tssrc/main/runtime/structured-foreground-command-strip.test.tssrc/renderer/src/components/terminal-pane/remote-pane-layout-push.tstests/e2e/cross-version-wire/cross-version-stop-note-test-rig.ts
💤 Files with no reviewable changes: 22
- src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.test.ts
- src/main/codex/codex-structured-session-close.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-host.test.ts
- src/main/codex/codex-app-server-connection.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-stop-event-entries.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-stop-event-restart.test.ts
- src/main/pi/rpc-session-adapter.ts
- src/main/native-chat/agent-session-wire/structured-conversation-compaction.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-stop-settle-binding.test.ts
- src/main/claude/claude-structured-session-adapter.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-steer-while-stopping.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-turns.test.ts
- src/main/codex/codex-child-work-evidence.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-stop-event-binding.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-queued-stop-row.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-queued-card-holds.test.ts
- src/main/runtime/structured-foreground-command-strip.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-stopping.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-create.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-conversation-stop.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-codex-stop-row.test.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-codex-stop-note-end.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
Status: review and real-app QA finished; checks green on the current headThe problem. Pressing Stop in a native chat could leave Codex and the command it was running alive, show no feedback before the agent's first reply, mark child work stopped before anyone knew it had exited, and (found in this round) skip ending the agent entirely when it had just picked up a follow-up message the user sent earlier. In that last case the chat reported the Stop as accepted while the agent kept working. What changes for the user. A Stop is recorded the moment you press it and the turn reads Interrupted. The agent's runtime ends (background work included), and your next message resumes the same conversation. A message withdrawn before it reached the agent reads Stopped manually beside Cancellation requested. This now also holds when the agent moves on to an already-sent follow-up while the Stop is being accepted: that agent process still ends, and that turn reads Interrupted, not Failed. Child work is marked stopped only once its runtime and workload have really exited. What was fixed since the last status (review rounds: 2, plus the final readiness pass).
Deferred, with tickets.
Verified for real (M4Air, ticket 2034, head e55d6cb). Chats were created on shipped Orca 1.4.237 with a fresh profile, then carried through a real shutdown and upgrade to this build. In each chat: a command running about a minute, a follow-up message sent while it ran, then Stop.
A second Stop while the first was closing wasn't possible: the Stop control was already gone, which is the intended outcome. Real agent config files on the machine were unchanged. Earlier rounds (tickets 1294, 1392, 1491) covered Pi Stop/resume, Sleep during startup and phone Stop on earlier heads; the Stop path changed since, so phone and Pi rely on the host tests now (phone Stop goes through the same host code as desktop Stop, which names the running turn the same way). 31 named test files (216 tests) pass on the merged head. Pi passed real Stop/resume on ticket 1294 (head bfa6885: two real replies, one Stop, Interrupted, original runtime and workload exited, real reply after resuming the same conversation). This PR's own Pi change ( Not verified. Claude's resumed reply wrongly claimed the command never ran; process records show it did. That is the model's account, not Orca's. Differences from the common pattern (each labelled; Stop ending the chat's runtime is the common pattern, not a difference): intended — outcomes settle from real exit proof; the Stop request and queue withdrawal are written as one durable record; Stop can abort a send still being prepared; the "Cancellation requested" note. Temporary — the extra transcript read above (STA-11949). For Brennan's reviewClaude: before Stop, after Stop, after resume Codex: before Stop, after Stop, after resume OpenCode: before Stop, after Stop, after resume Shipped 1.4.237, same Stop (shows the older "1 failed" badge) Three places where judgment matters:
|










ELI5
Pressing Stop in a Codex chat could interrupt the reply while leaving its background server and commands running. A Stop accepted before the first reply could also be missing from the transcript, and child tasks could read stopped before Orca knew their work had exited.
Stop now ends the agent runtime for every chat agent. The conversation records the accepted request once, and the next message resumes the same conversation. A started turn stopped by the user reads Interrupted; a send withdrawn before startup retains its existing Stopped manually result beside the request. Child tasks read stopped only when the execution host has proved that both their root process and its workload exited.
What Changed
Codex first receives an interrupt. It gets up to five seconds from the Stop request to settle, then Orca closes its background server and cleans up its owned descendants using process IDs and start times. A later proven exit can settle an earlier unconfirmed close. Chat Stop and workspace Sleep both count a proven runtime root exit as stopped. Child-task outcomes have the stronger requirement: both root and workload must be proved exited. Unavailable descendant evidence leaves those outcomes unconfirmed without blocking runtime-level Sleep or a new send.
The transcript writes Cancellation requested. when Stop is accepted, including when an accepted send is still starting or has not echoed its turn yet. It stays beside that message's stopped result and is deduplicated across repeated Stop requests. Stopping a startup with no queued message adds no request row. Question and approval cards keep their current Cancel behavior. Failure to save the request row is logged and cannot prevent the Stop.
Why
An accepted Stop is user intent; an exited process is an observed fact. Keeping those separate gives immediate feedback without claiming unfinished background work has stopped. Reusing the host's existing child-work records preserves completed child outcomes and avoids a second status store.
Differences from the common pattern
Linked Issue
Continues the Stop work tracked in #26561. The startup changes from #28046 and the Claude resumed-message repair from #30982 are already on main and included here.
Visual Proof
Real main-versus-branch Electron QA on M4Air compared main
fece03b5ac4749a50f24f7ad5a148e1f0154a4c8with head70c552fe35f0abe6658f88b800d3cb201ba3bfa4, carrying the same isolated saved conversations through a restart and upgrade. Codex and Claude each produced two real replies after upgrade, then stopped through the UI with physical root and workload exit proof, then produced two further replies in the same native thread. Their latest turn stayed Interrupted beyond sixty seconds. Expanded turn details showed exactly one accepted request for each observed Stop. Final child cancellation also proved root and both workloads exited before the child read stopped; its completed sibling remained completed.Screenshots and the detailed coverage limits are in the existing review status comment. The native Windows process contract passed on this head with the same saved conversation and identical 180-second workload: actual Stop ended the four observed original processes, recorded one request, and allowed a real same-thread reply. Main’s root and workload remained alive about 8.6 seconds after Stop. Independent Windows handles proved the final physical exits; normal-flow hook delivery remains unproven and an internal process-tree proof warning is retained. This short qualification does not cover every descendant or waive the full QA gaps. Overall QA remains BLOCKED. Pi native admission is blocked by the version-probe defect tracked in #30393; the installed CLI writes its version to standard error while admission reads only standard output. OpenCode ACP failed during native startup in this isolated rig; another lane ran the same version successfully, so its working provider recipe will be reused. No ACP Stop ran here. Starting Sleep was not exercised: no Send occurred before its bounded observer expired. Paired web connected to the correct isolated host, but its native chat was unavailable even through Agent Session History; native phone coverage is separate and unexercised. Repeated accepted Stop remains targeted live QA. Empty acquisition, unavailable card/scoped routes, child outcome retention and hook POST transport are verified by named automated tests listed in the review comment. The hook tests invoke generated scripts with controlled input; normal native-send hook delivery was not observed and is not claimed. Actual CLI native launch and a real reply passed; disabled search and unsupported native terminal routes remain gaps.
Testing
Head
70c552fe35f0passed 100 targeted tests across ten files. The tests cover accepted choices with missing readback, delayed interrupt, late response, opening-turn retry, legacy queued withdrawal, startup with and without an accepted send, runtime exit receipts and Sleep. Seventeen bounded source-module scenarios also passed; those traces do not replace real-agent QA. All seven exact-head workflows passed, including the full PR Checks workflow, its required aggregate and Windows load tests.The earlier Windows setup block remains historical. The lane owner authorized a fresh QA budget, which produced the M4Air results above. The separate short native Windows process qualification passed within the stated bounds; the lane owner accepts that observed flow as passing, with the process-tree proof warning retained as a known limit. Hook transport is verified by controlled tests, with live native-send delivery unobserved. Both fresh QA rigs are cleaned and their tickets released. Overall readiness remains blocked by required agents and consumer coverage.
Review
Full review, cumulative round 2: independent runtime and host/surface reviewers verified the fixes and found no new proven behavioral defect. The architecture ruling retained host termination and separate runtime/child proof, with no new persisted state. The full graceful sequence uses one original deadline, and an empty startup produces no cancellation request row. Separate readiness and performance source scans found no new proven defect. The complete final mechanism comparison passed on this head. An inherited refusal when clearing effort to an unknown configured default is owned separately under STA-11443/STA-11451; this PR does not label it intended.
Current head includes pinned main
fece03b5ac47; GitHub reports merge conflicts andbrennan-ready --force --pr 28051reports NOT READY for conflicts and the absent completion marker. Per the lane dependency ruling, the next main refresh waits for #30393 to merge, then requires new-head CI and targeted real Pi startup/Stop/resume, native phone Codex Stop, repeated Stop, starting Sleep/wake, Codex sanity and OpenCode ACP Stop/resume with the proven recipe. No completion marker or ready-to-merge claim is valid yet.Agent skill upstream boundary
Notes
Process actions and exit evidence stay on the execution host. Folder workspaces and independently updated peers retain their existing routes. No new saved field or wire operation is introduced.
Checklist