Skip to content

fix(codex): a Codex Stop is the interrupt alone, so it no longer says "Cancellation was not confirmed" - #23850

Merged
brennanb2025 merged 7 commits into
mainfrom
brennanb2025/stop-unconfirmed-row
Sep 30, 2026
Merged

brennanb2025 merged 7 commits into
mainfrom
brennanb2025/stop-unconfirmed-row

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

When you pressed Stop in a Codex chat, the chat could say "Cancellation was not confirmed." even though the Stop had worked and the reply had stopped. It happened because, after Codex stopped the turn, Orca also tried to kill every process that turn had started and waited for that to finish. When it couldn't prove they were gone, it blamed the Stop. Now a Stop just asks Codex to stop the turn, as Codex's own app does. It confirms as soon as Codex answers, and the false line is gone.

What Changed

Before

  • Stop asked Codex to interrupt the turn. Orca then also swept the turn's processes: every process started since the message was sent. It held the turn's end until the sweep finished, which could add about 4.5 s after Codex had already answered.
  • If the sweep could not prove the processes were gone, Orca wrote "Cancellation was not confirmed." for a Stop that named no turn, and "The provider had already finished this turn." for one that named a turn. Neither was true: the turn had been interrupted. In live QA with real Codex, the turn's interrupted end arrived 3 ms after the wrong line.
  • Every send and compaction also read the process table first, to take a snapshot for that sweep.

After

  • A Codex Stop is the interrupt alone. Codex answers an interrupt only as the turn ends: it holds the request until the turn aborts, answers, then reports the turn completed. So its answer is the Stop's result. The chat shows "Cancellation requested." and the turn reads as interrupted, as soon as Codex sends it.
  • "Cancellation was not confirmed." still appears when Codex never answers the interrupt, which means the turn never ended.
  • A long-running command started in the stopped turn keeps running. Examples are a dev server, or a command Codex left running to check on later. It runs until the chat's thread ends, and a later turn can still use it. This is Codex's own design: it keeps these sessions alive across an interrupt on purpose, and ends them when the thread shuts down. Short commands that finish with the turn are stopped by Codex itself.
  • Sends and compactions no longer read the process table.

Mechanism

  • Deleted the turn-process sweep (codex-structured-turn-processes.ts), the descendant snapshot taken at dispatch and compaction, and the hold on turn/completed in codex-structured-turn-cancellation.ts. What's left there is one function, interruptCodexTurn. The turn's end now goes through the normal notification path.
  • The adapter's unconfirmed Stop outcome had only one producer, the sweep, so it is removed from the adapter contract (structured-agent-session-adapter.ts), along with its row branch in structured-agent-session-turns-cancel.ts.
  • The process helpers that app-server teardown, terminal cleanup and the Claude exit check still use are unchanged.
  • Also in this PR, one comment-only commit correcting why an interrupted turn's un-echoed Codex message is withdrawn (see below). No behaviour change.

Why

The wrong line came from a second question, "are the processes gone?", asked after Codex had already answered the first. That second question was also the wrong one: Codex deliberately keeps a turn's long-running sessions alive through an interrupt. The sweep's main effect was killing processes Codex meant to keep, and holding the Stop while it did. No reported problem motivated it. Removing it removes the false row, the wait before Stop confirms, and the process-table reads on every send, instead of adding another guard.

Alternatives considered:

  • Keep the sweep, but log a failed check instead of showing it. That was this PR's first version. It keeps killing sessions Codex keeps alive, keeps the up-to-4.5 s hold on Stop, and keeps a process-table read on every send.
  • End the whole Codex app-server on every Stop. Nothing would be left running, but it ends background work the user started, and the next message has to start Codex again. Orca keeps the agent running across a Stop.
  • Ask Codex to end its background terminals on Stop. Codex's call for that is experimental, and it ends every background command in the thread, not only this turn's.

Differences from the common pattern

  • Withdrawing an interrupted message: a common approach never withdraws a Codex message once Codex answers the start request; it shows the message as delivered. Orca withdraws it when its turn ends interrupted before Codex echoes it. This is kept because of a failure Orca observed: in fix(codex): a message whose turn was stopped before Codex took it is withdrawn, not stuck #23618's live QA with real Codex, 2 of 3 such Stops left the message recorded nowhere, the third kept it only in the model's context, and Codex's own history showed no user message in any of the three. Codex gives saving that input only 100 ms before it hard-aborts the cancelled task, so showing the message as delivered would misreport one that was lost.

Linked Issue

No issue. Found in live QA of #23026 against real Codex.

Visual Proof

Live on this laptop with real Codex.

Before (current main). The wrong line needs the process check to fail, which is rare, so this arm carries a QA-only local patch, never pushed, that makes the check report failure after running. The result is a Stop that interrupted the turn, followed by the false line:

Before, main plus a patch forcing the process check to fail: a Stop that interrupted the Codex turn, followed by "Cancellation was not confirmed."

After (this PR, no patch; the sweep and its check no longer exist): two Stops, each "Cancellation requested." with the turn interrupted, 1-2 ms after the turn's end:

After, this PR with no patch: two interrupted Codex turns, each followed by "Cancellation requested."

A long-running command survives a Stop, as in Codex: sleep 300 and a local web server started in the turn were still running, and the server still answered, 30 s after the Stop. This was checked with the sweep switched off on the installed Codex 0.159.0:

After a Stop: the turn is interrupted and the chat shows its two long-running shells still working

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • codex-structured-session-cancel.test.ts covers two cases. Every process read or kill is mocked as a promise that never settles.

    • The interrupted end is published in the same tick as Codex's answer.
    • No process read or kill happens from the send through the Stop.
    • Both fail on the previous head by assertion, and putting the hold back fails the first.
  • structured-agent-session-codex-stop-row.test.ts runs the real host, journal and Codex adapter. The status rows are exactly ["Cancellation requested."] and the turn reads interrupted, for a Stop that names no turn and one that names its turn.

  • The genuinely unconfirmed case, where Codex never answers the interrupt, still writes "Cancellation was not confirmed.".

  • Tests that pinned only the sweep or the hold are deleted. Two prompt-ownership tests that pinned the hold now expect the Stop to confirm at once.

  • 33 related test files pass. pnpm tc:node and tc:cli, oxlint, check:code-quality:changed, check:react-doctor:changed, the reliability-gate check and the anti-slop audit pass.

AI Disclosure

Review

  • First version (log the failed sweep): one review loop, CLEAN.
  • Sweep deletion: one review loop, CLEAN. It checked four things:
    • nothing of the sweep, snapshot or hold is left;
    • the process helpers that other code uses are untouched;
    • removing a prompt-claim re-check is safe, because nothing can go stale between the claim and the interrupt any more, and a question answer still holds its claim across its write;
    • the interrupt answer and the turn's end give one Stop row in either order.
  • One comment's justification was corrected.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Merge order: merge this PR before #23861. #23861 still reads the unconfirmed Stop outcome this PR removes; once this lands, main is merged into #23861 so it adopts the removal.

  • Codex's process-group kill for a short command is a no-op on some platforms (Windows at least, possibly macOS), so that command's child processes can outlive a Stop there. The sweep used to catch them; now they are left as Codex leaves them. This matches Codex itself; only background commands on macOS were checked live.
  • If users want Stop to also end a turn's background commands, the follow-up is an explicit "Stop background commands" control that asks Codex to clean up its background terminals. It is not in this PR.
  • Nothing on the wire changes: the unconfirmed outcome was internal to the host.
  • Same on macOS, Linux, Windows and SSH hosts: this is the host talking to its own Codex process.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

Author: @BrennanKB5

…ot confirmed"

Codex answers a turn's interrupt only as that turn ends, then sends the turn's
own end. Orca also required its sweep of the turn's processes to prove them
gone, and when that sweep could not read the process table it wrote
"Cancellation was not confirmed." a few milliseconds before the turn's
interrupted end arrived. A Stop that named its turn said "The provider had
already finished this turn." in the same case.

Codex's answer is now what confirms the Stop. The sweep still runs and still
holds the turn's end until it finishes, but its result is logged, not shown.
A Stop Codex never answers, which is a turn that never ends, still reads
"Cancellation was not confirmed."
@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Refactors turn cancellation and process termination logic.

The PR does not appear safe to merge while the earlier concern about a process surviving Stop remains unresolved.

Findings

  1. P1 Stop can leave a process running ▶

Summary

The PR changes Codex Stop confirmation to rely on Codex’s interrupt response and removes the separate process sweep and deferred turn-completion handling.

  • The host no longer emits the process-check-driven “Cancellation was not confirmed” row.
  • The branch also incorporates journal, workspace-trust, and mobile changes made since the previous review.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Stop[Stop request] --> Interrupt[Codex turn/interrupt]
  Interrupt -->|answered| Confirmed[Cancellation requested]
  Interrupt -->|no answer or error| Unconfirmed[Stop not confirmed]
  Confirmed --> TurnEnd[Turn completion delivered normally]
Loading

Reviews (5) · Last reviewed commit: "Merge origin/main into #23850 (the Stop-..."

Comment on lines +122 to +124
if (acknowledged) {
if (!terminated) {
console.warn('[codex-structured] could not prove an interrupted turn left no processes')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Stop can leave a process running

The process sweep returns false both when it cannot read the process table and when it sees a turn-started process still alive at its deadline. If Codex answers the interrupt in the latter case, this branch only warns, releases the turn completion, and returns cancelled: true. There is no later per-turn sweep, so Stop can appear successful while that process keeps running. Please distinguish a proven-live process from an unreadable process table.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 014f44e8-257b-474c-90b5-d0d5b4d6fe68

📥 Commits

Reviewing files that changed from the base of the PR and between 179837a and 9a63a1c.

📒 Files selected for processing (3)
  • src/main/codex/codex-structured-prompt-ownership.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-conversation-stop.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Codex cancellation now sends an interrupt directly and treats a successful interrupt response as confirmed cancellation. Turn-end notifications are delivered without cancellation deferral. The cancellation outcome type no longer includes an unconfirmed state, and failed cancellation uses the stop-refused status path. The adapter no longer captures or terminates turn processes. Tests cover interrupt responses, turn completion, Stop requests with and without a turn ID, and interrupt timeouts. Comments describe how completion and interruption affect pending input.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 9a63a

The change makes Stop reporting more accurate by treating Codex's interrupt answer as confirmation. No concrete merge-blocking issue is established in the supplied evidence, though a narrow prompt-ownership edge case raised earlier is worth a follow-up check.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 9a63a

Stopping work now relies on Codex to terminate commands instead of independently terminating and checking them. The inspected controls constrain which session and turn are targeted, but command cleanup after successful interruption is not independently established. No surviving command or authorization bypass was demonstrated.

Retained concerns

  • Medium · security · inferred: The PR removes independent descendant termination and exit verification while confirming Stop from the provider’s successful interrupt response. Command-containment assurance therefore depends on an externally unverified provider contract. If a one-shot command survives that response, it can continue its existing effects after Stop is reported; this failure mode was not demonstrated.
Security review details

Security Blast Radius

  • inferred — The evidenced containment change concerns the selected local provider session and commands beneath its process tree. The inspected interrupt request names a thread and turn rather than arbitrary process identifiers. Exact filesystem, credential, network, and environment exposure of potentially surviving commands was not established.

Security Findings and Attack Paths

  • inferred — The conditional failure path is a successful provider interrupt reply without complete one-shot command cessation. The host then reports cancellation and skips its command-specific child-stop fallback, potentially allowing ongoing effects under the command’s existing authority. Source establishes the dependency change, not a verified survivor, exploit, or privilege escalation.

Trust Boundaries and Controls

  • observed — Shared admission recomputes the payload fingerprint and consults the durable operation ledger before execution. The adapter maps turn identity within the selected session, and prompt-bound cancellation checks the fence and claims the prompt. Explicit non-prompt cancellation relies on upstream admission for fence policy; complete caller authorization and lease enforcement were not resolved, so no bypass is asserted.

Resilience and Maintainability Implications

  • observed — Recognized interrupt refusals remain distinguishable from confirmation. Prompt claims are released on refusal or pre-confirmation failure, and rejected prompt-lifecycle admission throws rather than returning success. A running command whose cancellation is not confirmed retains the provider-child stop fallback. These controls limit failure paths but do not independently check descendants after successful interruption.

Hardening Proposals

  • proposed — Establish a version-bounded provider contract for one-shot descendant cleanup and repeated interruption. Validate it with real subprocesses, delayed completion, and provider failure, while separately checking the intended lifetime and cleanup of background terminals. This is a proposed assurance measure, not evidence that those guarantees fail.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 19 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly describes the main user-visible change: a Codex Stop no longer reports the false "Cancellation was not confirmed" message.
Description check ✅ Passed The description is detailed and covers the required explanation, changes, rationale, testing, visual proof, review notes, compatibility considerations, and checklist. It states that no linked issue ex…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Stop confirmation no longer depends on the process sweep. CodexStructuredTurnCancellation.cancel now treats a resolved turn/interrupt as proof the turn ended and returns {cancelled:true}; a sweep that cannot prove the turn's processes are gone becomes a console.warn instead of a false chat row. The sweep still runs and is still awaited for cleanup.
  • unconfirmed removed from the adapter cancel contract. It was only produced by that Codex branch; the shared wire result (AgentSessionCancelResult) is unchanged, and no other producer or consumer of the field remains in src/ or mobile/.
  • Host row branch removed. The !cancelled && outcome.unconfirmed status row is gone; the genuinely unanswered interrupt still reaches "Cancellation was not confirmed." through the adapter-rejection catch (the timeout error rethrows because it is neither a request nor an unsupported error).
  • New host-level regression test (structured-agent-session-codex-stop-row.test.ts) runs the real host, journal and Codex adapter with the sweep forced to fail and turn/completed (interrupted) arriving 4 ms after the ack; it asserts exactly ["Cancellation requested."] and an interrupted turn, for both a no-turn and a named-turn Stop. Updated codex-structured-session-cancel.test.ts and structured-agent-session-conversation-stop.test.ts accordingly.
  • Comment-only changes in codex-structured-turn-end-settlement.ts and codex-structured-dispatch-echo.ts explaining why an interrupted turn's un-echoed send is withdrawn.

I verified the load-bearing external claim ("Codex answers an interrupt only as the turn ends") against upstream openai/codex: request_processors/turn_processor.rs withholds the response for a non-empty turnId and bespoke_event_handling.rs releases it from the TurnAborted/TurnComplete arms, enqueuing the response just before turn/completed. Orca only ever sends a non-empty id, so the startup fast path is unreachable, and the normal-completion overlap is the PR's documented known cost. Ran the touched suites (codex session-cancel, prompt-ownership, conversation-stop, turn-open-wait, turn-end-settlement, the new stop-row test, host, prompt-cancel, turns), oxlint on changed files, and pnpm tc:node — all green.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status

Ready. Head e4246044dec, based on main. CI green. The one red was Mobile Checks failing to install dependencies on the runner, which passed on re-run; this PR touches no mobile code.

The failure: after a Codex Stop that did end the turn, the chat said "Cancellation was not confirmed." (or "The provider had already finished this turn." for a Stop naming its turn). Orca's separate check that the turn's processes were gone returned false and spoke for the Stop.

The fix: Codex answers an interrupt only as the turn ends, so that answer is the confirmation. The process check still runs for cleanup but no longer writes a false line. It removes the cause, not a guard around it.

Review

  • One focused loop: CLEAN, no changes. It confirmed from Codex's source that an interrupt is answered only when a turn completes or aborts, before turn/completed, with steered and compaction turns following the same rule.
  • Nothing else reads the removed field.
  • The new test fails with the fix reverted.

Validation

  • tc:node and tc:cli, oxlint, the changed-lines gate, React Doctor and the anti-slop audit pass.
  • The new host test reproduces the live 4 ms ordering for both kinds of Stop.
  • The genuinely unconfirmed case (Codex never answers) still shows its line.

Known, not in this PR: the logged warning cannot tell "processes survived" from "could not check".

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟠 Major · Wait for turn/completed before confirming… · codex-structured-turn-cancellation.ts:120-125

src/main/codex/codex-structured-turn-cancellation.ts:120-125
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Wait for turn/completed before confirming cancellation.

The Codex app-server turn/interrupt response is an acknowledgement. It does not contain the final turn status. CodexStructuredTurnCancellation.cancel treats that response as confirmation, releases the blocked completion, and returns { cancelled: true } even when process termination is unproven. The session layer then shows “Cancellation requested.” while the turn can still be active.

Change src/main/codex/codex-structured-turn-cancellation.ts to wait for the matching turn/completed notification and require turn.status === 'interrupted' before returning { cancelled: true }. Return an unconfirmed result for a timeout or another terminal status.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 03331e50-39cd-4043-ae6a-79cfb5e3cad3

📥 Commits

Reviewing files that changed from the base of the PR and between e424604 and daf0ae0.

📒 Files selected for processing (2)
  • src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-turns-cancel.ts
💤 Files with no reviewable changes (1)
  • src/main/native-chat/agent-session-wire/structured-agent-session-turns-cancel.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status update: merged current main in (head 2bacba1bfb5), CI green (22 passed). Main had moved the Stop code into structured-agent-session-turns-cancel.ts; this PR's removal of the "not confirmed" branch is re-applied there, and its own Stop-row test passes on that path. Merge this before #23861. Not merged.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Visual Proof update: a live before/after with real Codex is now in the description. Both arms carry the same QA-only forcing patch (the process check reports failure after running), and the arms differ only by this PR's 8 files. Before shows the false "Cancellation was not confirmed." after a Stop that interrupted the turn; after shows "Cancellation requested." with the turn interrupted. The real Codex and Claude settings files were unchanged across the runs. Not merged.

A Codex Stop sent turn/interrupt and, alongside it, swept the turn's
processes: it compared a snapshot of the app-server's descendants taken at
each send and compaction against a fresh one and killed what was new, then
waited for those processes to exit. The turn's end was held back until the
sweep finished.

Codex already owns this. It kills a turn's one-shot commands on interrupt
and deliberately keeps unified-exec background terminals running across one,
so the sweep killed work Codex means to keep, read the process table on
every send, and delayed the interrupted end behind a kill-and-wait.

A Stop is now turn/interrupt only. Codex's answer confirms it and the turn's
end publishes the moment it arrives, through the same path as any other
notification. A long-running command started in that turn keeps running
until the thread ends, as it does in Codex itself.

Removed: the turn-process module and its integration test, the snapshot
taken at dispatch and compaction, the held turn/completed, the adapter
dependencies that injected both, and the prompt-claim re-check that only
covered the wait for the snapshot.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 8ed235e0-d3fc-44f2-b9de-75b83f8ff986

📥 Commits

Reviewing files that changed from the base of the PR and between 2bacba1 and 179837a.

📒 Files selected for processing (22)
  • config/scripts/ci-shard-timings.json
  • src/main/codex/codex-prompt-registry-retention.test.ts
  • src/main/codex/codex-prompt-registry.ts
  • src/main/codex/codex-structured-dispatch-boundary.test.ts
  • src/main/codex/codex-structured-dispatch-test-support.ts
  • src/main/codex/codex-structured-prompt-ownership.test.ts
  • src/main/codex/codex-structured-prompt-ownership.ts
  • src/main/codex/codex-structured-provider-events.ts
  • src/main/codex/codex-structured-session-acquire.ts
  • src/main/codex/codex-structured-session-adapter-fixture.ts
  • src/main/codex/codex-structured-session-adapter-lifecycle.test.ts
  • src/main/codex/codex-structured-session-adapter.ts
  • src/main/codex/codex-structured-session-cancel.test.ts
  • src/main/codex/codex-structured-session-state.ts
  • src/main/codex/codex-structured-turn-cancellation.ts
  • src/main/codex/codex-structured-turn-processes.integration.test.ts
  • src/main/codex/codex-structured-turn-processes.ts
  • src/main/crash-reporting/self-initiated-tree-kill-log.test.ts
  • src/main/crash-reporting/self-initiated-tree-kill-log.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-codex-stop-row.test.ts
  • src/main/refused-tree-kill-root-termination.test.ts
  • src/main/runtime/structured-agent-session-codex-turn-end-settlement.test.ts
💤 Files with no reviewable changes (7)
  • config/scripts/ci-shard-timings.json
  • src/main/codex/codex-structured-turn-processes.integration.test.ts
  • src/main/codex/codex-prompt-registry.ts
  • src/main/runtime/structured-agent-session-codex-turn-end-settlement.test.ts
  • src/main/codex/codex-structured-session-state.ts
  • src/main/codex/codex-structured-turn-processes.ts
  • src/main/codex/codex-structured-dispatch-test-support.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Comment on lines 99 to +110
let interruptConfirmed = false
try {
const result = await cancellation.cancel(
const result = await interruptCodexTurn({
session,
claim.prompt.threadId,
promptTurnId,
isCurrent,
() => {
threadId: claim.prompt.threadId,
turnId: promptTurnId,
requestTimeoutMs,
onConfirmed: () => {
interruptConfirmed = true
return session.translator?.cancelPrompt(prompt.itemId) ?? { accepted: true }
}
)
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

sed -n '1,170p' src/main/codex/codex-structured-prompt-ownership.ts
sed -n '130,260p' src/main/codex/codex-prompt-registry.ts
rg -n 'cancelPrompt|claimBoundPrompt|ownsClaim|interruptConfirmed' src/main/codex
git diff 280733273545f0b3eeedc1be54b14d406239030e 179837a1aaecfd1f34fa9dcf30bbbbbdb6030741 -- src/main/codex/codex-structured-prompt-ownership.ts src/main/codex/codex-structured-turn-cancellation.ts

Repository: stablyai/orca

Length of output: 24879


🏁 Script executed:

set -e
printf '%s\n' '--- translator contract and implementation ---'
sed -n '1,120p' src/main/codex/codex-structured-journal-contracts.ts
sed -n '220,320p' src/main/codex/codex-structured-journal-translation.ts
rg -n -C 6 'cancelPrompt|function cancel|cancel\(|bindJournalItemId|clearTurn|forget\(|replace|replacement|turn/(completed|started)|activeTurnIds' src/main/codex --glob '*.ts'
printf '%s\n' '--- current cancellation tests ---'
rg -n -C 10 'cancelCodexStructuredTurn|interruptCodexTurn|cancelPrompt|prompt.*cancel|replacement|acquisitionGeneration|ownsBoundClaim' src/main/codex/codex-structured-prompt-ownership.test.ts src/main/codex --glob '*test.ts'
printf '%s\n' '--- exact base ownership implementation ---'
git show 280733273545f0b3eeedc1be54b14d406239030e:src/main/codex/codex-structured-prompt-ownership.ts | sed -n '1,145p'
printf '%s\n' '--- relevant current ownership and registry ranges ---'
cat -n src/main/codex/codex-structured-prompt-ownership.ts | sed -n '65,125p'
cat -n src/main/codex/codex-prompt-registry.ts | sed -n '145,225p'

Repository: stablyai/orca

Length of output: 45659


🏁 Script executed:

printf '%s\n' '--- cancelPrompt definitions ---'
rg -n -C 12 'cancelPrompt' src/main/codex
printf '%s\n' '--- prompt registry lifecycle calls ---'
rg -n -C 8 'bindJournalItemId|clearTurn|prompts\.forget|prompts\.clear|prompts\.cancel|cancel\(' src/main/codex --glob '*.ts'
printf '%s\n' '--- base cancellation wrapper ---'
git show 280733273545f0b3eeedc1be54b14d406239030e:src/main/codex/codex-structured-turn-cancellation.ts | sed -n '55,155p'

Repository: stablyai/orca

Length of output: 41780


🏁 Script executed:

set -e
printf '%s\n' '--- prompt cancellation implementation ---'
cat -n src/main/codex/codex-structured-journal-prompts.ts | sed -n '80,145p'
printf '%s\n' '--- cancellation and turn lifecycle tests ---'
rg -n -C 14 'cancel\(|cancelled|cancelPrompt|same-item|replacement|turn/completed|clearPromptTurn' src/main/codex/codex-structured-journal-translation-turn-lifecycle.test.ts src/main/codex/codex-structured-prompt-ownership.test.ts src/main/codex/codex-prompt-registry-retention.test.ts

Repository: stablyai/orca

Length of output: 42362


Preserve prompt ownership through interrupt confirmation.

turn/completed can clear the claimed prompt while turn/interrupt is pending. A replacement can reuse the same journal item ID. The onConfirmed callback then passes that ID to cancelPrompt, which can cancel the replacement and other prompts in its turn. Recheck ownership before cancelling.

Suggested fix
   if (!claim || !promptTurnId) {
     if (claim) {
       session.prompts.releaseClaim(claim)
     }
     return { cancelled: false }
   }
+  const acquisitionGeneration = session.acquisitionGeneration
   let interruptConfirmed = false
   try {
     const result = await interruptCodexTurn({
       session,
       threadId: claim.prompt.threadId,
       turnId: promptTurnId,
       requestTimeoutMs,
       onConfirmed: () => {
+        if (
+          sessions.get(request.sessionId) !== session ||
+          session.ended ||
+          session.fence !== request.fence ||
+          session.acquisitionGeneration !== acquisitionGeneration ||
+          providerTurnId(session, requestedTurnId) !== turnId ||
+          !session.prompts.ownsClaim(claim)
+        ) {
+          session.prompts.releaseClaim(claim)
+          return { accepted: true }
+        }
         interruptConfirmed = true
         return session.translator?.cancelPrompt(prompt.itemId) ?? { accepted: true }
       }
     })

@brennanb2025 brennanb2025 changed the title fix(native-chat): a Codex Stop that ended its turn no longer says "Cancellation was not confirmed" fix(codex): a Codex Stop is the interrupt alone, so it no longer says "Cancellation was not confirmed" Sep 30, 2026
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status update: this PR now deletes the Codex Stop process sweep and the hold on the turn's end. A Stop is the interrupt alone, as in Codex itself, and confirms as soon as Codex answers. The description is rewritten for that: a long-running command started in the stopped turn keeps running until the thread ends, which is checked live on Codex 0.159.0; the one remaining "Differences" entry cites #23618's live evidence; the Windows caveat and a possible "Stop background commands" follow-up are named. Live before/after with real Codex is in Visual Proof, the after with no patch. One review loop on the deletion: CLEAN. main merged in (head 9a63a1c0535); CI is running. Not merged.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one stale audit harness is left behind by the process-sweep removal.

Reviewed changes

Re-reviewed the two commits landed since the prior pullfrog review (e424604de): 7d845e0ef7 ("a Codex Stop is the interrupt alone") and the doc-only 179837a1aa — a 22-file delta.

  • A Stop is now the interrupt alone. Deleted codex-structured-turn-processes.ts and CodexStructuredTurnCancellation; interruptCodexTurn sends only turn/interrupt and treats a resolved request as confirmation ({cancelled:true}). Removed the captureTurnProcesses/terminateTurnProcesses deps and all adapter/fixture wiring.
  • The turn/completed deferral is gone. Notifications publish every turn end as Codex sends it, so the turn's end reaches the journal ahead of the Stop's answer.
  • Contracts trimmed. Dropped ownsBoundClaim (Codex), unconfirmed from the adapter cancel outcome, the !cancelled && outcome.unconfirmed row branch, and the cancellation/compactions cancel parameters; cancelCodexConversation gained a post-wait liveness guard.
  • Tests and comments. Pruned tests that exercised the deleted sweep, rebalanced the crash-reporting ring-size comment, and simplified the dispatch-boundary ordering test.

I verified the load-bearing external claim against upstream openai/codex: turn/interrupt cancels the turn's task token, which terminates the legacy exec process group and the unified_exec one-shot command, while background/interactive terminals are deliberately left alive until thread shutdown. Deleting Orca's descendant sweep therefore cannot orphan a one-shot command. I also traced the new ordering: a turn/completed processed before onConfirmed still converts the turn's pending prompts through settleCodexJournalTurn, so the prompt row reads cancelled either way — no wrong row. Touched suites and pnpm tc:node pass.

ℹ️ The codex-prompt-claim-retention audit harness is now stale

The new commits remove CodexStructuredTurnCancellation, CodexPromptRegistry.ownsBoundClaim, and the cancellation/compactions parameters of cancelCodexStructuredTurn, but docs/audits/codex-prompt-claim-retention/ still targets all of them. Its documented reproduce command can no longer run, and the README still names the removed class as the mechanism that confirms a cancellation.

Technical details
# Stale codex-prompt-claim-retention audit after the cancellation refactor

## Affected sites
- `docs/audits/codex-prompt-claim-retention/reproduce.cjs:18` — re-exports the removed `CodexStructuredTurnCancellation` from `codex-structured-turn-cancellation`; bundling now fails on the missing named export (and would be `undefined` at runtime).
- `docs/audits/codex-prompt-claim-retention/scenario.cjs:52` — `new api.CodexStructuredTurnCancellation({...})`; `:101`/`:102` pass `compactions`/`cancellation` to `cancelCodexStructuredTurn`; `:119` passes `turnCancellation` to `translateCodexNotification`; `:189` calls the removed `state.prompts.ownsBoundClaim(...)`.
- `docs/audits/codex-prompt-claim-retention/README.md:10` — describes "`The actual CodexStructuredTurnCancellation` invokes the confirmation callback" as the current confirmation path.

## Required outcome
- Either update the harness to the new flow (`interruptCodexTurn` + immediate notification delivery, `ownsClaim` in place of `ownsBoundClaim`), or retire the audit and remove its README instructions if the claim-lifetime reproduction is no longer meaningful.
- Keep the documented reproduce command runnable if it stays.

## Open questions for the human
- Is this audit still load-bearing (referenced from a decision record or incident), or safe to delete now that the confirmation mechanism changed?

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025
brennanb2025 merged commit 1aa9437 into main Sep 30, 2026
34 checks passed
brennanb2025 added a commit that referenced this pull request Sep 30, 2026
brennanb2025 added a commit that referenced this pull request Sep 30, 2026
Main now settles a Codex send from the turn it was answered into
(#23618, #23679, #23850): an interrupted or failed turn that never
echoed a send withdraws or refuses it, and a completed turn leaves it
to its echo, which Codex records before turn/completed. That replaces
this branch's own mechanism, so every conflict resolves to main:

- journal-level terminal settlement (ownerEndedClientMessageIds,
  multi-row enqueueMany, terminalTurnFences, canResolveDispatch,
  hasTerminalTurn, turn_settled_before_acknowledgement) and the sink's
  onCommitted/onAbandoned/onDiscarded plumbing
- the dispatch-echo ownership model (start candidates, started
  evidence, terminal snapshots) and the late-settlement retry
- the connection's onResult response observer
- the shared-predicate command admission, which main expresses with
  its own fence and handover rules (#23524)

Codex's turn/steer, the part the host-owned queue still needs, is
re-expressed on main's model in the following commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant