fix: close steer stop and recovery ownership gaps - #740
Conversation
Model: gpt-5
4057899 to
7f2e513
Compare
Model: gpt-5
Codex Review SummaryThis 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05c12f7097
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| await this.deps.workspaceDocument.repo.upsertDocMeta?.(roomId, recoveryPatch); | ||
| if (pendingUserMsgId) | ||
| await this.deps.executionService.acknowledgeSteerTurn(sessionId, pendingUserMsgId); |
There was a problem hiding this comment.
When lastMissingHistoryUserMsgId already protects turn C and a late refused steer B remains in steerTurnStatuses, getPendingUserTurnActivationId selects B after excluding C. This recovery path then replaces the single tombstone with B and acknowledges B here, so C becomes active again through latestUserMsgId; if C's history subsequently arrives, it can be dispatched despite already being reported undelivered, potentially duplicating work after an explicit resend. Keep the existing tombstone and defer B, or represent all missing-history acknowledgements without evicting another turn.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L33-L37
Useful? React with 👍 / 👎.
| .enum([ | ||
| 'pending', | ||
| 'pending_apply', | ||
| 'delivery_unknown', |
There was a problem hiding this comment.
When a new daemon writes delivery_unknown into a session that is open in an older renderer, that renderer's isSessionHistoryDelivered treats every status except pending and pending_apply as delivered, while its editable-tail planner blocks only pending_apply or _lodyDeliveryKind: 'steer'. Unknown projections intentionally do not set that delivery marker, so an older client shows the turn as delivered and permits Edit & Resend without the duplicate-work warning. Store this outcome in a representation older readers handle safely, or version/gate the durable status before writing it.
Useful? React with 👍 / 👎.
| sessionId: SessionIdSchema, | ||
| userTurnId: z.string().trim().min(1), | ||
| applied: z.boolean(), | ||
| recoveryOwned: z.boolean().optional(), |
There was a problem hiding this comment.
In mixed-version operation, a new daemon now adds recoveryOwned to rejected steer responses, but an older Streams client validates them with the previous strict response schema and therefore converts the whole response to null. For promotion-failed, that prevents the older renderer's only recovery attempt and leaves the proven-undelivered turn stranded in pending_apply; local older clients that bypass this parser can instead run the obsolete pointer-writing fallback. Advertise/version this semantic through MachineMeta.protocolCapabilities and emit it only to compatible callers.
AGENTS.md reference: packages/shared/AGENTS.md:L71-L75
Useful? React with 👍 / 👎.
1 participant
Related issue
Closes #477
Closes #666
Problem / pressure
Stop could leave a steer holding the session operation queue while preparation, configuration, or the provider verdict remained unresolved. Late steer results could also overwrite a newer activation or make terminal history appear active again. Unknown delivery needed an explicit recovery state without blind replay.
Summary
steerTurnStatuses, preserving newer producer activations and preventing late results from rewinding pointers.delivery_unknownhistory/UI state with an explicit resend confirmation warning about possible duplicate work; unknown outcomes are never auto-dispatched.Visual explanation
sequenceDiagram participant U as Stop / completion participant E as Execution owner participant A as ACP adapter participant H as History + metadata participant W as Watcher / UI U->>E: abort local steer wait E->>A: keep submitted request owned E->>E: drain raw prompt/steer/config or terminate A-->>E: applied / not-applied / unknown E->>H: project by exact userTurnId H-->>W: wake and reconcile W->>W: dispatch only proven refusal W->>U: explicit warning before unknown resendBefore / after
delivery_unknownis terminal for dispatch and offers only an explicit, warned fresh send.processingover terminal history.Test plan
apps/cli: 263 focused tests passed across AgentClient, dispatch logic/watcher, and execution service.packages/components: 55 focused tests passed.packages/shared: 70 focused tests passed.DEEPSEEK_HARNESS_PROFILE_*,createDeepSeekHarness*).pnpm run docs checkstill reports the repository's pre-existing two broken DSH usage-test links.Context handoff
Instructions for reviewing agents
apps/cli/src/session/session-execution-service.ts, exact-id reconciliation in the dispatch watcher, and the shared history terminal guards.unknownmust remain non-replayable and thatsteerTurnStatusesis the right durable activation index instead of rewindinglatestUserMsgId.Authoring context