Repository navigation
fix(native-chat): a Stop's note records whether it took, so a Stop that stopped nothing never marks a later turn as yours - #24478
brennanb2025 wants to merge 10 commits into
Conversation
… reads it A turnless Stop that interrupted nothing no longer marks the turn that opens next as the person's cancellation.
…p and a downgrade
…top defers whatever it answered The note linked an answer to its Stop event by the event's time. Two of a person's events can share a millisecond (a close right after a declined Stop), and the writer read the event back to learn which one it wrote. The event now carries an id its writer mints, and the note's answer names it (`stop.eventId`); a rewind's restatement keeps the id. Answers are read only from Stop notes. A host stop or eviction after a person's Stop that did nothing wrote its own event, which lifted the person's queue pause and sent the held card on reopen. The host now defers to a person's Stop that covers the work whatever it answered (`personStopCovers`); only the turn-end attribution applies the answers (`personStopDecides`), so that turn still ends as news.
…whenever it waits
…d end The rewrite rule let `interrupt-unconfirmed` replace `end-owed` for the same Stop, meant for a writer that reports the owed process end failing. No such writer exists yet, and performCancel itself could take it: a repeat press of a Claude Stop that finds the agent's adapter closed throws its interrupt and no longer owes an end, so it overwrote the owed end and the turn lost its "Interrupted" label. A press now never replaces an answer that the same Stop took. Also pins the restart's red "the agent stopped" row: written after a Stop that did not take, muted after one that took.
…ok, not end-owed A Stop that ends the provider's process next answered `end-owed` whatever the interrupt did. When the provider took the interrupt the turn already stopped and the process end is cleanup, so it now answers `took`. `end-owed` is left for a Stop whose interrupt was refused or went unanswered, whose effect really is the owed end: the one answer a later report of that end failing may revise.
Review status: ready for a merge decision (head 784b594)The problem. In a native Codex chat, suppose you press Stop right after sending, before Codex has picked the message up. Orca has nothing to interrupt, so the chat says "Codex had no turn running to stop", and then Codex runs the turn anyway. If that turn later died on its own (Codex crashed, or Orca restarted and settled it), Orca labelled it "Interrupted after N" with the muted "you cancelled this" mark. Your Stop did nothing, so that label was wrong. The same thing happened after a Stop that Codex declined. The cause: Orca records a Stop the moment it starts, before it knows whether the Stop worked, and the turn-end rule treated every Stop as one that worked. What changes for you. A turn whose Stop did nothing now ends like any other turn that dies on its own. After a restart it reads "Failed after N", with the failed mark and the usual red "The agent stopped while this response was in progress" row. A Stop that worked still reads "Interrupted after N", both live and after a restart. When a Stop doesn't work, a later press of the same Stop that does work still makes the turn yours. A later press that finds nothing to stop no longer overwrites a row saying an earlier press worked. Row text is otherwise unchanged, and there is no client change. How. Each Stop's existing row now also records, in a form code can read, what the Stop did: Fixed during review
Deferred (P3)
Verified
Not verified
|
|
| stopIsTurnCancellation(state, stop, turnId, userItemId) && | ||
| (endedAt === undefined || endedAt >= stop.event.at) | ||
| stopCoversTurnEnd(state, stop, turnId, userItemId, endedAt) && | ||
| journalStopStillCounts(state, stop.event) |
There was a problem hiding this comment.
Turn end can outrun answer. If Codex ends a turn while its interrupt request is pending, the turn-end row can be written after the Stop event but before the Stop’s answer. This check treats the unanswered Stop as effective and stores the end as the user’s cancellation. If the later answer says the Stop did nothing, it does not correct that outcome, so the turn can still appear to have been cancelled by the user.
| for (const [itemId, item] of state.items) { | ||
| if (!isStructuredAgentSessionStopNote(itemId)) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Answer lookup scans entire chat. When a Stop’s only answer is
declined or no-effect, this lookup examines every journal item and parses each key. It runs when a covered turn ends, adding work that grows with the conversation on the serialized write path. In long chats, that can make turn endings progressively slower.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Stop event identity: every Stop event now carries a writer-minted
id(runRecordedStop,recordStopEvent),appendStopEventrequires it, and a rewind restatement keeps it;atis demoted to timing only. - Recorded answer: every note
performCancelwrites gainsstop: { answer, eventId? }with the five-valuetook/end-owed/declined/interrupt-unconfirmed/no-effectvocabulary, andeventIdnames the event it wrote or the in-force one a repeat press repeated. - One rule reads it:
journalStopStillCountstreats a Stop as counting until it has an answer and only while some answer says it took;turnEndAfterStopandpersonStopDecidesTurnapply it, whilepersonStopCoversTurn(host-stop deference) intentionally ignores answers so a person's queue pause holds. - Row rewrite guard:
agentJournalStopAnswerReplacesprevents a later non-taking press from erasing an earlier answer that took. - Schema refactor: the typed journal facts moved to
agent-session-journal-fact-schemas.ts, withstopadded optionally to the status item body. - Tests: dedicated
journal-stop-answers,agent-session-stop-answer, and real-hoststructured-agent-session-stop-answersuites, updated Codex/Claude stop-row and restart tests, and a new cross-version downgrade journey wired intopr.yml.
I traced the answer mapping through each performCancel branch, the event-id plumbing from writer to row, the replacement rule, and the turn-end rule against the real host tests, and I found no correctness, regression, or coverage gaps to raise. The cross-version test first asserts the rows under test actually carry stop.eventId and the event id, so it can genuinely fail.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (33)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe journal now assigns IDs to Stop events and records Stop answers in status notes. Turn-ending logic separates whether a person’s Stop covers a turn from whether its answer makes the Stop count. The wire flow passes Stop event IDs into cancellation and records answers such as Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No identified issue blocks merging the Stop-answer change. The investigated edge cases follow the intended Stop behavior. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change records whether a Stop worked without showing a new permission or authority grant. Event identity and answer-matching checks limit the apparent exposure, but complete synchronization and recovery behavior remains unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 54 functions across 32 files. (1 skipped: 1 unsupported.) ✨ 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 |
…wer-on-note # Conflicts: # .github/workflows/pr.yml
| const stop = state.queuePauseMarks.latestStop | ||
| return ( | ||
| stop !== null && | ||
| stopIsTurnCancellation(state, stop, turnId, userItemId) && | ||
| (endedAt === undefined || endedAt >= stop.event.at) | ||
| stopCoversTurnEnd(state, stop, turnId, userItemId, endedAt) && | ||
| journalStopStillCounts(state, stop.event) |
There was a problem hiding this comment.
Earlier effective Stop gets lost. If a Stop takes effect, a message is sent into the still-visible turn, and a second Stop finds nothing to interrupt, the second press writes a new Stop event with a
no-effect answer. This check considers only that latest event, so a later interrupted end is treated as an agent failure even though the first Stop worked. The second answer also replaces the first on the turn’s Stop note.
There was a problem hiding this comment.
Important
The turn-end rule reads only the latest Stop event. A later Stop that covers the same turn but didn't take can therefore erase an earlier Stop that did, flipping a genuinely cancelled turn back to "news". Main happened to label this case correctly, so it is a regression.
Reviewed changes
- Stop event identity: every Stop event now carries a writer-minted
id(runRecordedStop,recordStopEvent);appendStopEventrequires it andatis timing-only. - Recorded answer:
performCancelwritesstop: { answer, eventId? }on every Stop note, with the five-armtook/end-owed/declined/interrupt-unconfirmed/no-effectvocabulary, naming the event it wrote or the in-force event a repeat repeated. - Reader rule:
journalStopStillCountscounts a Stop until it has an answer and then only while some answer says it took;turnEndAfterStop/personStopDecidesTurnapply it, whilepersonStopCoversTurndeliberately ignores answers. - Rewrite guard:
agentJournalStopAnswerReplacesstops a later non-taking press from erasing a same-event answer that took. - Schema: typed journal facts split into
agent-session-journal-fact-schemas.ts, withstopoptional on the status body; cross-version downgrade journeys added.
I read the whole diff, traced the answer mapping through each performCancel branch and the event-id plumbing, verified the renames have no stale callers, and confirmed the merge commit touched no PR source file (conflict was only .github/workflows/pr.yml).
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
| stopCoversTurnEnd(state, stop, turnId, userItemId, endedAt) && | ||
| journalStopStillCounts(state, stop.event) |
There was a problem hiding this comment.
An earlier Stop that took the interrupt can be erased by a later, distinct Stop event on the same turn that didn't. Because the rule reads only queuePauseMarks.latestStop, a turn that was genuinely cancelled ends as "news" (agent failure), which is the opposite of what this PR is fixing — and main got this case right, so it is a regression.
Technical details
# Earlier taking Stop lost to a later non-taking Stop on the same turn
## Affected sites
- `src/main/native-chat/agent-session-journal/journal-stop-turn-end.ts:149-153` — `stopEndsTurnAsCancellation` consults only `state.queuePauseMarks.latestStop` and `journalStopStillCounts(stop.event)`, so any earlier covering Stop event is invisible.
- `src/main/native-chat/agent-session-wire/structured-agent-session-queued-stop.ts:71-100` — `stopRecordedWork` returns `'unrecorded'` (writer mints a fresh event) whenever `sentSince` is true (`acceptedSequence > inForce.sequence`) or the person stop is no longer in force, even for the same turn.
- `src/shared/agent-session-stop-answer.ts:61-71` — `agentJournalStopAnswerReplaces` allows a different `eventId` to overwrite, so E2 also replaces E1 on the `stop:T` note.
## Reachable shape
1. Turn T running; Stop E1 names/opens T and answers `took` (or `end-owed`).
2. Before T's interrupted row is written, a submission is accepted (a direct/non-`queue-if-active` send, or Send-now). This lifts the person stop's pause (`latestPersonTurnSequence` rises) → `stopRecordedWork` returns `'unrecorded'` → E2 is written with a new id and, while T is still the live turn, `turnId: T`.
3. The second Stop finds nothing to interrupt and answers `no-effect`/`declined`/`interrupt-unconfirmed`.
4. T's end is written last; `latestStop` is E2, `journalStopStillCounts(E2)` is false, so no `cancellation` outcome is applied although E1 took.
The same-turn Stop note is keyed `stop:T`, so step 3 also overwrites E1's answer. `agent-session-stop-answer.test.ts:110-116` pins that a different `eventId` is allowed to replace, which is the enabling condition; no current test combines it with `latestStop`.
## Required outcome
A turn whose end is reached by any person Stop that took should still write the `cancellation` outcome, even when a later distinct Stop event covering the same turn did not take.
## Suggested approach (optional)
Have the turn-end rule consider every person Stop event that covers the turn and took, rather than only `latestStop`; or have `journalStopStillCounts` accept the covering events and return true if any of them took. `personStopCoversTurn`'s host-deference behavior is unaffected and should stay answer-blind.
## Open questions for the human (optional)
- Is the `latestStop`-only read intentional for distinct events, or was the "a later miss never erases an earlier success" claim scoped to the same event by oversight?
- If the window is deemed unreachable in practice (it needs an accepted send while T is still the live turn), a test pinning that would let the finding be closed as designed.|
Closing in favour of #24864. The problem this PR addressed: a Stop that stopped nothing could still mark a later turn as the person's cancellation. It came from inferring, when a turn's end is written, that an unnamed Stop owns the turn its stopped message later opened. #24864 removes that inference at its cause, so no answer has to be recorded on the Stop's note:
#24369 ("Stopping…") is step 2 and stacks on #24864. It no longer reads the Stop's answer. |

What this fixes
The bug. You send a message to a native Codex chat and press Stop right away, before Codex has picked the message up. Codex isn't running anything yet, so Orca sends no interrupt and the chat says "Codex had no turn running to stop." Codex then starts on the message anyway. If that turn later dies on its own (Codex crashes, or Orca restarts and settles it), Orca labels it "Interrupted after N" with the muted "you cancelled this" mark. That's wrong: your Stop did nothing, so it should read "Failed after N", like any other turn that dies on its own.
The same mislabel hits a Stop Codex declined. The interrupt failed, Orca couldn't prove the Codex process ended, and the chat said "Codex didn't stop: …". If the turn then died, it still read as your cancellation.
Why it happened. Orca never recorded whether a Stop actually took effect. #24311 made a turn's end read the Stop's event, which is written the moment the Stop starts, before anyone knows whether it worked. The Stop's answer existed only as the words on its chat row ("Cancellation requested." versus "didn't stop"), and nothing reads those words. So the turn-end rule treated every Stop as one that worked. For a Stop pressed before any turn showed, it bound the Stop to the next turn the stopped send opened, even when the Stop had stopped nothing. #24311 lists this gap as a Temporary item.
Scope. Native (structured) Codex and Claude chats on the host that runs the agent, local or SSH. Terminal agent tabs are unaffected. Clients are unchanged: they already label turns from the outcome the host stores.
ELI5
When you press Stop, Orca already writes a row in the chat ("Cancellation requested." or "Codex didn't stop"). That row now also records, in a form code can read, what the Stop actually did, and which Stop it was. When a turn ends, Orca counts it as "cancelled by you" only if your Stop really took effect. Before, it assumed every Stop worked.
What Changed
Before (for you):
After (for you):
Mechanism.
Each Stop event gets an id. The Stop event (fix(native-chat): Stop's pause is worked out from the chat's history, so a steered message is never re-sent #24072) gains
id, minted by whoever writes the event: the Stop's own step (runRecordedStop) mints it before the write, and a host stop or close (recordStopEvent) mints its own.appendStopEventrequires it, so no writer can skip it. A rewind restates the event whole, so the id survives. The event's time (at) is now only timing, never identity. The id is optional on the type only for events that pre-release development builds wrote; such an event has no answers and counts, as before this PR. The event has not shipped in a release yet (fix(native-chat): Stop's pause is worked out from the chat's history, so a steered message is never re-sent #24072 is not in v1.4.218), so nothing else has to migrate.The answer, recorded. Every Stop row
performCancelwrites now carries an optionalstop: { answer, eventId? }(structured-agent-session-turns-cancel.ts).eventIdnames the Stop event the answer belongs to: the event this Stop wrote, or the one still in force that a repeat press repeated. The Stop's step learns its event's id from its own write, without reading the history back. The answers:answertookend-oweddeclinedinterrupt-unconfirmedno-effectOne rule reads it.
journalStopStillCounts(journal-stop-answers.ts): a Stop counts while it has no answer yet, or while any answer to it says it took effect. It reads answers only from Stop rows (the rows whose id starts withstop:) and matches them to the event by id. A later answer that it didn't take effect never cancels an earlier one that it did. It decides who owns a turn's end. The turn-end write (turnEndAfterStop) applies it, whether the agent, the host's fallback or a relaunch writes the end. So doespersonStopDecides, which serves the two readers that must choose before the end is written: the relaunch's choice to skip the red error row, and the Claude translator's reading of an error result.journal.stopMarks.stillCounts(event)exposes the same rule for the "Stopping…" state (feat(native-chat): the chat reads "Stopping…" from your Stop until the turn actually ends #24369), which is its first caller.The host still defers to your Stop, whatever it answered. A host stop or eviction asks a separate question,
personStopCovers: is a person's Stop in force for this work? It doesn't read answers. If it did, a host stop after a Stop that did nothing would write its own event, which ends your queue's "stopped" pause, and the message you held would go to the agent when the chat next opened.personStopDecidesruns the same check aspersonStopCovers, also given the turn's end time and opener, and then applies the answer rule. Both share one function, so they can't drift apart.One rule for rewriting the row.
agentJournalStopAnswerReplaces(src/shared/agent-session-stop-answer.ts): a later press never replaces an answer that the same Stop took,end-owedincluded.end-owedis now only for a Stop whose effect depends on the owed process end, so it is the one answer a later report that the end failed could revise. An earlier draft allowedinterrupt-unconfirmedto replaceend-owed, for when the owed process end fails. Nothing writes that report yet, and the Stop's own step could take the exception: a repeat Claude Stop that reaches the agent after its adapter has closed (no path to that was found outside shutdown) gets an interrupt error and no longer owes an end, and it stripped the turn's "Interrupted" label. So the exception is gone. feat(native-chat): the chat reads "Stopping…" from your Stop until the turn actually ends #24369, which adds the report of a failed process end, adds its own revision with it.Schema. The row schema accepts
stopwith an openanswerstring and a non-emptyeventId. To stay under the line limit, the typed-fact schemas (thread goal, failure, Stop answer) moved to their own module, agent-session-journal-fact-schemas.ts. The Stop row's key helpers moved next to the answer reader (src/shared/agent-session-stop-answer.ts), so the history store can tell a Stop row from any other.Vocabulary, checked against current main. All five answers occur on main today, and each is pinned by a test. Changes from the design first proposed in #24311's follow-up:
refusedandunconfirmedare nameddeclinedandinterrupt-unconfirmed, as feat(native-chat): the chat reads "Stopping…" from your Stop until the turn actually ends #24369's review asked.reasonorturnId. The event holds both, and the row's key and turn scope already place it, so copies would be a second record of the same fact.eventIdis the one link the event can't supply.Why
The rule needs a fact, not an inference from words. Whether a Stop worked is only known once the provider answers, after the event is written, and the answer differs per provider and per path. Recording it where the answer is produced, on the row that already reports it, gives every reader the same fact. That includes the "Stopping…" state in #24369, which currently infers it from the row's failure wording.
Alternatives considered:
stop-refused, as first proposed). It would be a second trace for one Stop, it can't express "took" or "owed", and fix(native-chat): a Stop still reads as yours after Orca restarts, because the turn's end reads the Stop's event #24311 deleted it.Differences from the common pattern
took,end-owed,declined,interrupt-unconfirmed,no-effect) rather than a yes/no, because a later revision needs them: anend-owedanswer whose process end then fails becomesinterrupt-unconfirmed.end-owedis never revised totookonce the process end completes. Both count the same, and the turn's own end is the record that it finished.end-owedand still counts as your cancellation, although nothing stopped the turn. Nothing records the failed end yet; it only reaches a log line. Main labels this case the same way, so it is not a regression. Follow-up: feat(native-chat): the chat reads "Stopping…" from your Stop until the turn actually ends #24369 records the failed end and revises the answer tointerrupt-unconfirmed.Linked Issue
Closes #24311's Temporary item "a Codex Stop pressed before Codex opened the turn" (such a Stop still marked that turn as your cancellation if it later crashed or was settled at relaunch). No issue: maintainer change.
Visual Proof
N/A: no client change. The label comes from the turn outcome the host stores. The new tests pin that outcome (none, so not a cancellation) and the red row's presence, and the existing restart suite pins that an interrupted turn with no outcome reads "Failed after N". No app run was done for this PR.
Testing
New tests:
no-effect,took,end-owedanddeclinedthroughperformCancel, each naming its event's id; for a Stop that ends the provider's process next (Claude's),tookwhen the interrupt was taken, andend-owedwhen it was refused or went unanswered;tookorend-owed, and the turn still ends as the person's cancellation;Updated tests:
took,declined,interrupt-unconfirmedandno-effectasserted on the real interrupt paths, including a Stop pressed before Codex opened the turn, which Codex never opens.tookwhen Claude takes the interrupt,end-owedwhen it refuses it.id.Cross-version (both run in the CI cross-version job):
stop.eventIdand the eventid. An older host replays the history with no malformed or unknown row and keeps the rows after it. It sends the row on, and the release's own client code applies the update and shows the row's text.idand keeps the rows after them; after a writable downgrade and upgrade, the Stop is the same event (same id) and still pauses the queue.Runs, all at this PR's head:
*.integration.*tests.Ablations, at this PR's head: each new piece was undone alone and the target tests rerun (10 files, 147 tests, all passing before each ablation). Failing test counts:
end-owedeven when the interrupt was takenThe answer mapping in
performCancelis unchanged from the first revision, whose ablations (each answer written wrong, unknown answers read as known) all failed tests; those were not rerun here, but every test they failed still passes.Lint: oxlint on changed files is clean.
Typecheck:
tc:nodeandtc:webpass at this head.AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
answervocabulary that a newer answer never makes malformed (docs/reference/remote-wire-compatibility.md, rules 1 and 4), so no capability is needed. Older hosts keep the row: unknown keys in an item body are ignored on replay, and the cross-version test pins that. Older clients show only the text, which is unchanged. The Stop event itself never goes over the wire (it is host-only, and v1.4.218 ignores every key inside it). Its id reaches clients only as the row'sstop.eventId, an optional key no client reads.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred): locally oxlint on changed files,tc:node,tc:weband the targeted tests above; CI covers the rest