Repository navigation
fix(server): stop labels for continued and provider-interrupted runs; server notices reopen nothing - #93
Conversation
… server notices reopen nothing - A run interrupted outside any command, with no Stop asked for, records provider_interrupted instead of other. - A completed or waiting run a restart continued reads as having lost its background work, not as cut short; t3_thread_read and t3_thread_result look up a continuation under the same rule. - thread.run.result events carry stopReason. - A post-only notice the server writes leaves a settled or snoozed thread as it was. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…d continuations as retried Review of record findings on #93: a message or steer that replaces the running turn was labelled provider_interrupted once the provider confirmed; a continuation that failed before its prompt lost its retried-as link; stopReason is sent only for cancelled or interrupted states. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of record: independent Claude Opus review of
|
chrisohearn
left a comment
There was a problem hiding this comment.
Independent Claude Opus review of 9a20598 (coordinator's review of record)
Scope: diff plus source at head. Mergeable/CLEAN against base ff68541 (#85 merged); git merge-tree against the base is clean; CI is green. No tests run locally.
(a) Event consumers: stopReason is Schema.optionalKey (packages/contracts/src/mcpEvents.ts:89) and is only spread when it is defined (McpEvents.ts:130). Webhook bodies are encoded through McpEventEnvelope, so the field survives encoding. Struct decode ignores keys it does not know, so a rolled-back build still decodes new mcp_event_deliveries rows (relay replay at McpEventRelay.ts:466). Additive and safe.
(b) No new events: the stamper rewrites payloads only, and observeEvent emits under the same conditions as before. The run is stamped before append, so the stored event carries stop.
(c) A server notice is still written as a post-only message and turn item (Orchestrator.ts:6937), so it shows in t3_thread_read and the recipient's run.stop. Nothing resurfaces it when a snooze ends, because posts never start turns; it is visible, not lost.
(d) mayHaveRestartContinuation (RunStop.ts:232) is a superset of isRestartContinuationSource and continuationFailedBeforePrompt. Dropping interrupted is correct: RestartBackgroundNote.ts:147 says interrupted runs never continue. The "failed to start" label only appears when a retry run points at the failed one. Provider runtime recovery passes commandType: "provider-runtime.reconcile", so restart cancels are not relabelled provider_interrupted.
P1: none.
P2: none.
P3 McpEventRelay.ts:298-322: the inbox relay "event" body builds its own payload with no state or stopReason, so an inbox-relay consumer still cannot tell a restart cancel from a real cancel. Only webhook deliveries and replay (source: body.data, :487) carry the field. Consider adding it as a follow-up.
P3 RunStop.ts:117,175: interrupt requests are now held for every command. A promote-to-steer that stops a compacting turn (Orchestrator.ts:~11518) therefore labels that interrupted run "delivered into the running turn instead", which describes the queued message, not the stopped run.
P3 Orchestrator.ts:6886: deliberate trade-off. A settled sender waiting on a reply gets no unsettle signal; its only routes are reading the thread or an event subscription.
Overlap with #94 (text conflict at Orchestrator.ts ~6999): keep both changes. Use #93's three-term automaticWake (notification || delegatedCompletion || serverNotice, with serverNotice declared above the settle block). Below it, keep #94's whole respectsSnooze block unchanged, and guard with if (snoozedUntil != null && !automaticWake && !respectsSnooze). A server notice is then an automatic wake: respectsSnooze is false, the snooze stays, and the post returns before queueBehindRun. After resolving, re-run both runtimeLayer.test.ts cases (post-only, and respect-snooze hold).
Verdict: no blocking findings
7eddc7f
into
integration/mcp-events-20261109
Follow-up to #84 (merged as 7836910), for the coordinator's review P3s. Ledger key
r4/w2-restarts. No migration.Mislabelled continued runs (
RunStop.tsrunStopDetail)A restart can continue a run that already completed or is waiting, when the restart cut background work the run left running (
RestartContinuation.ts;isRestartContinuationSource). Those runs were labelled "cut short by restart", as if the turn itself had been cancelled.They now read like this:
t3_thread_readandt3_thread_resultagree on continuationt3_thread_readlooked for a continuation of every run.t3_thread_resultlooked only for cancelled, interrupted or failed runs. A completed run with cut background work could therefore show a continuation in one read and not in the other.Both now use one rule,
mayHaveRestartContinuation. A continuation is looked up for a run that:cancelled;restartCancelledBackgroundWork;failedcontinuation whose start failed before the provider took the prompt, whichretryContinuationStartretries as a new run.These are the runs restart recovery continues.
t3_thread_resultstill reads the thread's other runs only when the rule holds. A failed retry reads asfailed to start; retried as run N.Provider interrupts get
provider_interrupted, nototherA run that becomes
interruptedin a write with no command (the provider or its runtime ended the turn) now recordsreason: "provider_interrupted",actor: server.An interrupt a command asked for still wins. That covers a Stop, and also a message or steer that replaces the running turn (
stopRunForQueuedCommand). The command's reason (stopped,replaced,steered) and its actor are held until the provider's confirmation or the effect worker's settle.thread.run.resultcarriesstopReasonMcpEventDatagains an optionalstopReason, set fromrun.stop.reasonon arun.updatedwhose state iscancelledorinterrupted. A rolled-back run that still carries an earlier stop gets nostopReason. A subscriber such as Jarvis can then tellserver_shutdown/server_restartfrom a real cancel without reading the thread.The field is optional, and the advertised payload schema is built from
McpEventData, so existing subscriptions keep validating. The field is absent when a run has no stop.The cancel notice no longer reopens settled or snoozed senders
Decision: respect the snooze, and the settle. Every
message.dispatchused to unsettle its thread, and every dispatch without anotificationended a snooze. The #84 notice therefore reopened a settled sender, or woke a snoozed one, for a message it can't act on until it resumes anyway. That is the pattern behind tonight's snooze incident.The notice is written for the record. Skipping it would lose the information, and waking the sender spends a turn the user deferred. So a post-only message the server creates (
creationSource: "server", today only this notice) now counts like other automatic wakes:t3_thread_read.Messages from people and agents, including MCP
t3_thread_post(creationSource: "mcp"), still reopen as before. The existing post-only test asserts both.The trade-off: a sender that stays settled or snoozed learns of the dropped message only when it resumes or reads its thread. The recipient's run record (
stop) shows the same thing.Review of record
Codex code review hit its usage limit, so an independent read-only Claude Opus review of
48376ec596is posted as a comment. It found nothing blocking. Its two should-fixes and its nit are fixed in9a20598652.Acceptance checks (read-only, after deploy)
Set
Tto the deploy time andDB=~/.t3/userdata/statev2.sqlite.Every cancelled or interrupted run has a specific reason. Pass:
missing= 0 andother= 0.provider_interruptedis a valid reason.Run-result events carry the reason. Delivered events are pruned once acknowledged, so check pending deliveries right after a restart. Pass: rows show
server_shutdownorserver_restart. Otherwise, Jarvis's next restart-cancel event showsstopReasonin its data.A notice to a settled or snoozed sender leaves it that way. For each notice since deploy, the sender's thread should not have gained a
thread.unsettledorthread.unsnoozedat that moment. Spot-check: pass if a sender that was settled or snoozed before its notice still is.Tests
RunStop.test.ts:provider_interrupted;message.dispatch(replaced);McpEvents.test.ts: a restart-cancelledrun.updatedgivesstopReason: "server_shutdown", and a run without a stop gives no field.runtimeLayer.test.ts(extended post-only case): a server notice leaves a settled, snoozed thread settled and snoozed. This fails on the base (expected null to be 'settled'). The existing MCP post in the same test still reopens.Not run: the
mcp-tool-useeval. This changes optional fields in tool results and events and no descriptions, and the eval calls paid models.🤖 Generated with Claude Code