🚥 fix: Pause Rejected Steer Recoveries - #16306
Conversation
|
Ready for review of exact head This head pauses rejected steer recoveries without resending them as ordinary messages, reserves cancellation before queue drain, preserves cancelled rows when a guarded edit cannot complete, and suppresses late restoration/redelivery. The server keeps the legacy error code while adding specific rejection reasons and payload-free diagnostics. Verification at this head: 641 focused tests passed, including real Redis and the full resumable HTTP-controller test file. Targeted static checks passed. Both TypeScript workspaces reproduce baseline diagnostics with no additions from this patch. Lighthouse was attempted but its prerequisite API declaration build fails at the same unchanged The invariant self-review covered queue writers and readers, direct and automatic sends, cancellation interleavings, late responses, remount/reload, disabled storage, owner checks, source preservation, and legacy protocol conversion. No external review has been received for this head yet. Please trigger the review against this SHA. |
|
@codex review |
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: f3f9309079
ℹ️ 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".
| head != null && | ||
| recoveryDisposition(jotaiStore.get(recoveryDispositionsFamily(conversationId)), head) != | ||
| null; | ||
| const next = shouldDrain && !held ? (head ?? null) : null; |
There was a problem hiding this comment.
When a completed run encounters a held recovery at the head of a queue, this branch leaves the row in place but has already consumed the one-shot run-end signal. If the user then dismisses that recovery, dismissRecovery only removes the row and does not rearm the drain, so any ordinary follow-ups behind it remain queued indefinitely until another generation happens or the user sends one manually. Keep the terminal signal parked while the head is held, or explicitly rearm it when the hold is resolved.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ebfc7b7. A completed run-end now stays parked while a recovery is held; queue-head changes wake the drain when dismissal removes that row. Tests cover blocked and confirmed-cancelled holds, new-conversation migration, aborted runs, and exactly one ordinary successor after dismissal.
| onRestoreToComposer( | ||
| message.text, | ||
| message.files, |
There was a problem hiding this comment.
When the composer already contains text, files, quotes, or skills—or is reserved by answer mode—onRestoreToComposer returns false, but this new Copy action ignores that result and still shows the review toast. In that common scenario, clicking “Copy to composer for review” copies nothing and provides no indication that the action failed; handle the false result with appropriate feedback instead of reporting the normal outcome.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ebfc7b7. The Copy action now checks the composer callback's boolean return. A refused copy shows an error toast instead of a success toast and leaves the paused recovery intact; acceptance and refusal are both covered by tests.
|
Ready for review of exact head Addressed both Codex findings on the previous head: a held recovery no longer consumes the one-shot completed run-end, and dismissing it wakes the next ordinary queued item. A refused Copy to composer now reports an error instead of claiming success, leaving the paused row untouched. Tests also cover cancelled holds, first-run migration, aborted runs, and one successor per terminal boundary. Replied to each inline finding with this commit. Verification for this review round: 357 focused client tests passed across six suites; targeted static checks (ESLint, Prettier, imports, package validation, circular dependencies) passed. Ran |
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
|
CI update for exact head All other current-head CI checks passed. An exact-head rerun of the failed job was attempted but this integration was denied by GitHub ( |
|
@codex review the latest head |
|
Ready for review of exact pushed head Merged current Checks on the merged content: 357 client recovery/queue/UI tests, 134 backend tests including real Redis, and 160 full resumable-controller tests passed. PR-targeted static checks passed. Ran No newer Codex review has arrived for this SHA yet. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Summary
A rejected parked-steer recovery was restored to the local queue with the same recovery and request IDs. The next completed run submitted it again. Cancelling the chip could race queue drain, letting a late rejection restore a source that had already been cancelled.
This PR stops those automatic retries without turning uncertain delivery into a fresh ordinary send. It covers steer recovery only, not durable-worker or subagent-discovery polling.
Behavior
reason:source_missing,protocol_mismatch,owner_mismatch,invalid_payload, orpayload_mismatch. The existingRECOVERY_PAYLOAD_MISMATCHwire code remains compatible with older clients. Logs include conversation/source identity and reason, not payload content.A missing source is not proof of non-delivery. A genuine payload mismatch leaves its source parked. Neither outcome is automatically downgraded to a fresh message.
Verification
Verified at merge-resolved pushed head
ea41f6d3a8ec49ae8de3d01a3d2574bba1958bde, merged with currentdevat34938ebcbad520be862fb3862fbfd757b9b026adwithout a force-push. Resolved the overlapping controller test mock by retaining both the steer-recovery failure helper and the new agent-error metadata helper.devcases)Validation limitations:
npx tsc --noEmitin both changed TypeScript workspaces. Both report local baseline errors. Repeated the checks in a clean worktree at the current base with the same dependency overlay; this patch adds no diagnostics.ebfc7b7bd2e6b8eac771a37aca4704323a786193failed its 4,500 ms median LCP budget at 4,548.585 ms. The earlier PR head passed at 4,349.916 ms. The latest merge push reruns this gate; do not treat it as passed until its result is available.npm run lighthousedid not reach browser measurement because an isolated-declarationTS9010in unchangedpackages/api/src/stream/jobStoreCapabilities.ts:11also reproduced in its clean base worktree. GitHub CI builds using its own dependencies; the local tooling blocker is not a diagnosis of the earlier measured CI LCP failure.The original historical rejecting branch remains unproven. These regressions establish the current failure modes and corrected behavior, not a reconstruction of expired production state.