🍱 fix: Carry Coding Approval Mode With Queued Turns - #16326
Conversation
|
Review handoff for exact pushed head Scope: server-owned queued composer turns now snapshot the policy-filtered coding mode at enqueue, persist it with the queue fingerprint and replay checks, and forward the owner-scoped claimed value through trusted continuation preparation. Existing live BYOM policy still validates at run admission. Preexisting queue records and clients without a mode retain their old default. This follows merged #16322 without reopening its background-return scope; there is no continuation-wide Approve grant here. Checks on this exact head: 492 focused tests across 12 suites passed, including the merged background-return regressions, real queued-turn Mongo storage, client queue submission, and live policy tests. All four changed TypeScript workspaces passed Subsystem self-review covered event-payload isolation, queue ownership and tenant boundaries, persisted record projections, same-id replay conflicts, legacy rows, retry and claim/release paths, policy-fenced admissions, and pause/warm-steer behavior. Please review 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: d4a1ea28be
ℹ️ 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".
| item.manualSkills.length > 0 && { | ||
| manualSkills: item.manualSkills, | ||
| }), | ||
| ...(codeApprovalMode != null && { codeApprovalMode }), |
There was a problem hiding this comment.
During a rolling deployment with any pre-change queue replica, this field can be silently discarded: the old Zod enqueue schema strips the unknown property, and an old worker's record projection also omits it, while the advertised capability remains only { supported, durability }. A queued fullAccess/acceptEdits turn can therefore be accepted but execute as Ask; if its first response is lost and the retry reaches a new replica, the new exact-intent comparison instead returns an idempotency conflict because the stored row has no mode. Advertise and require a queue protocol/capability version before sending the snapshot so mixed replicas preserve deterministic behavior.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9f06ac905c23e56ce8f53a086a54179790e6ef68. Approval-bearing requests negotiate protocol v2 and use a versioned enqueue URL, so an older replica cannot strip the snapshot and accept it as v1. New deliveries require agent_queued_turn_v2 workers. Their v2 delivery key is reserved atomically with the queue row, preventing a crash between insertion and publication from allowing a legacy recovery worker to publish a v1 delivery. Regressions cover legacy endpoints, replica changes, lost-response retry, atomic reservation conflicts, and capable-worker lease recovery. Snapshot-free legacy requests retain their old path.
| const setDefaultAction = useSetRecoilState(store.duringRunDefaultAction); | ||
| const steerInterruptsByDefault = useRecoilValue(store.steerInterruptsByDefault); | ||
|
|
||
| const { selected: codeApprovalMode } = useCodeApprovalMode(conversation); |
There was a problem hiding this comment.
When an added conversation is active, the visible CodeApprovalMenu and ordinary-send path call useCodeApprovalMode(conversation, addedConvo), but this new queue path evaluates only the primary conversation. If the conversation retains fullAccess while the added agent's environment disallows it or its metadata is incomplete, the menu visibly falls back to Ask while queuing still snapshots fullAccess; admission can then either reject the turn or run the primary agent unattended contrary to the displayed selection. Pass the host's already policy-filtered combined selection, or at least addedConvo, into this hook.
AGENTS.md reference: AGENTS.md:L112-L118
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9f06ac905c23e56ce8f53a086a54179790e6ef68. ChatForm now passes the active added conversation to useSteering, which evaluates useCodeApprovalMode(conversation, addedConversation) just like the visible approval menu. Queue snapshots therefore use the combined-agent selection, not the primary-only preference. Tests cover the queue handoff plus real policy evaluation with restricted and incomplete added-agent metadata.
|
Review fixes pushed and verified at Both Codex findings have code fixes and replies in their original threads. This head uses the combined-agent selection for queued snapshots and fences snapshot-bearing requests and deliveries with protocol v2, including the insertion-before-publication crash window. Existing snapshot-free requests remain compatible. Live BYOM admission still validates the mode; this is not a continuation-wide permission grant. Verification: 418 focused tests across 11 suites passed (API 51, client 189, real Mongo queue/delivery 149, transport/schema 18, routes 11). All four changed TypeScript workspaces passed Lighthouse was rerun with worktree-local Chromium. Mongo and the test server started, but Chromium could not launch because the host lacks Subsystem self-review covered combined-agent writers/readers, owner/tenant authorization, mode validation, queue replay conflicts, atomic publication identity, old/new replica compatibility, claim/lease recovery, and legacy rows. CI and a new exact-head external review are still pending. A maintainer must trigger Codex; the Lia GitHub App cannot trigger it. |
|
@codex review the latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f06ac905c
ℹ️ 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".
| const send = (res, result) => res.status(result.status).json(result.body); | ||
|
|
||
| const AgentQueuedTurnEnqueueController = async (req, res) => { | ||
| const enqueueController = (protocolVersion) => async (req, res) => { |
There was a problem hiding this comment.
enqueueController adds a behavior-selecting helper to the legacy CJS controller, making /api responsible for choosing and injecting the queue protocol version. Export version-specific handlers or a factory from packages/api instead, leaving this file with only requires, request plumbing, and calls into the TypeScript module.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 47a6478d27d296f14016c11b356bc7e710a7c957.
Moved protocol selection and enqueue error handling into createAgentQueuedTurnEnqueueHandlers in packages/api/src/agents/queuedTurnHttp.ts. The CJS file now only supplies the existing dependencies and exports the returned handlers. Regressions exercise the real handler through both adapters, pinned protocol selection, owner-scoped receipt replay, and bounded error handling.
| const status = (error as { response?: { status?: number } })?.response?.status; | ||
| if (status === 404 || status === 501) throw unsupported(); |
There was a problem hiding this comment.
When an approval-enabled user chooses Interrupt & Send, the v2 POST includes priority: true, so the enqueue handler returns 501 QUEUED_TURN_PRIORITY_UNSUPPORTED. This catch rewrites that response to 409 QUEUED_TURN_PROTOCOL_REQUIRED; consequently isDefiniteQueuedTurnsUnsupported no longer recognizes the priority response and useSteering leaves the message rejected instead of downgrading it to the local queue after aborting the active run. Only translate unstructured old-replica responses, and preserve recognized 501 codes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 47a6478d27d296f14016c11b356bc7e710a7c957.
The transport now preserves structured origin responses, including 501 QUEUED_TURN_PRIORITY_UNSUPPORTED, instead of relabeling them as missing protocol support. Only unstructured 404/501 responses from an unavailable v2 endpoint become QUEUED_TURN_PROTOCOL_REQUIRED. Transport tests cover both cases, and the existing client priority-fallback regressions pass.
| response: { status: 409, data: { code: 'QUEUED_TURN_PROTOCOL_REQUIRED' } }, | ||
| }); | ||
| try { | ||
| const { capability } = await listAgentQueuedTurns(payload.conversationId); |
There was a problem hiding this comment.
If the capability GET times out, returns 5xx, or yields malformed data, execution never reaches the mutating POST, but the shared catch rethrows the raw error. useSteering interprets such non-4xx errors as an outcome-ambiguous enqueue, polls for a receipt that cannot exist for 60 seconds, and then leaves the message requiring manual recovery. Handle negotiation failures separately as definite preflight failures (or retry the read), reserving the ambiguous state for errors after the POST begins.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 47a6478d27d296f14016c11b356bc7e710a7c957.
Removed the capability-list preflight entirely. The versioned POST is the capability gate, so no pre-POST timeout, 5xx, or malformed list response can create an ambiguous enqueue state. Tests assert there is no GET dependency. Real POST transport failures retain their original uncertainty semantics.
| response: { status: 409, data: { code: 'QUEUED_TURN_PROTOCOL_REQUIRED' } }, | ||
| }); | ||
| try { | ||
| const { capability } = await listAgentQueuedTurns(payload.conversationId); |
There was a problem hiding this comment.
When an earlier v2 POST commits but its response is lost, a retry needs to reach the enqueue handler, which deliberately looks up the existing receipt before mutable conversation and agent checks. This unconditional capability GET instead calls the list handler, which authorizes the current conversation before reading receipts; if the conversation is deleted, its agent changes, or access is revoked between attempts, the GET returns 404/403/501 and the retry never reaches that replay path. The accepted turn may therefore execute while the caller sees a rejection, so negotiate without gating retries or make the preflight aware of the existing request identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 47a6478d27d296f14016c11b356bc7e710a7c957.
Retries now go straight to the same versioned POST with the same client request ID and approval snapshot. No mutable list/access check runs first. The lost-response regression revokes list access before retrying and still recovers the committed receipt. Backend tests continue to verify owner-scoped receipt lookup before mutable checks.
9f06ac9 to
7ee9085
Compare
|
Review handoff for exact pushed head Backend CI failed in All four new Codex findings are addressed, with replies in their original threads:
Local checks: 659 focused tests passed across 17 suites (API 268, transport/schema 21, client 189, legacy routes 11, real Mongo queue/delivery 170). All four TypeScript workspace typechecks and shared package builds passed. Staged lint, formatting, import ordering, and circular-dependency checks passed. Lighthouse was attempted again. Mongo and the test server started, but Chromium cannot launch without host library Subsystem self-review covered alternate producers/readers, combined-agent policy, owner/tenant boundaries, receipt-first retries, definite versus uncertain transport failures, priority fallback, atomic publication identity, leases, mixed-version workers, and legacy rows. No new permission grant or persistence format is introduced by this closeout patch. A review of this exact head is still pending. A maintainer must trigger Codex; the Lia GitHub App cannot trigger it. |
|
Exact-head review handoff: All four new Codex findings remain addressed by ancestor Verification on the merged tree: 865 focused tests across 23 suites passed (API 292, client/recovery 371, transport/schema 21, route adapters 11, Mongo queue/delivery 170). All four affected workspace typechecks passed. Staged static checks and the API rebuild passed. Redis recovery integration tests passed using a disposable worktree-local Redis process, stopped after the test run; no machine services were changed. Subsystem re-review confirmed the combined-agent selection, snapshot transport, receipt replay, structured priority fallback, v2 worker fences, and dev's recovery safeguards remain intact. No new per-tool or continuation-wide permission grant was introduced. CI and external review must cover this exact SHA. A maintainer must trigger Codex; the Lia GitHub App cannot trigger it. Lighthouse remains locally blocked by missing Chromium host libraries; no live BYOM browser approval-flow test ran. CI update: Backend workflow |
2 participants
Pull Request
Summary
Queued Agent turns retain text, files, quotes, and selected skills, but previously lost the conversation's selected coding approval mode. When a queued message starts as a fresh ordinary turn, Full Access or Accept Edits can silently fall back to Ask. This is separate from the background-result return path fixed in #16322.
Capture the policy-filtered selection with each queued message and keep it through the durable queue, idempotent replay, and trusted continuation preparation. At execution, the existing live BYOM admission checks still decide whether the requested mode is permitted. An old client or existing row with no snapshot retains the old default, and changing an existing request ID's mode is a conflict rather than an implicit permission upgrade.
This PR addresses server-owned queued composer turns, including choosing Queue instead of a warm steer. A degraded steer that becomes a local follow-up still uses the existing ordinary-send path. HITL pause/resume already pins the original turn's mode, and a warm steer stays within its current run; neither path gains new authority here. This does not make the per-tool Approve action grant continuation-wide Full Access.
How it works
No additional conversation lookup or model invocation is introduced. A mode named in an event payload cannot replace the claimed turn's selection.
Type of change
Testing
66cc995b08f08cf85682bbd0ead54803872b3de5: 865 focused tests passed across 23 suites, including the previously failing backend delivery suite.npx tsc --noEmitpassed in all four affected TypeScript workspaces. Shared package builds passed.libatk-1.0.so.0is missing on the host. No page audits ran.Review Notes
The queued mode is a selection, not an approval grant. Client and HTTP schema validate allowed values, persistence and replay bind it to one queued intent, only an owner-scoped claimed row supplies the mode to the trigger host, and BYOM checks still revalidate against current settings on the new run. Omitting it on old records preserves legacy behavior. Existing pause/resume and background-completion tests remain part of the focused follow-up check.
Codegraph orientation matched skill
2026-09-24.4; the startingdevgraph reflected56301c1b5a32f37e5c59e09f0c8567f559ee0b76before #16322 merged. Codegraph HUD.Title: 🍱 represents a queued turn carrying its selected mode alongside its other contents. It had 1 indexed LibreChat commit use and 1 subject-leading use in 5,540 commits through 2026-09-23 23:08:36; the sentinel passed. The older use was about a compaction index, not queued turns. This nightly count is an as-of floor; the applied PR title is the spend record.
Review-Fix Update
Both original findings and all four subsequent Codex findings have fixes and replies. Protocol selection lives in the TypeScript backend. Direct versioned POST admission replaces the capability-list preflight, preserving structured priority responses and allowing idempotent receipt recovery after mutable access changes. Snapshot-bearing deliveries still require v2 workers and an atomic versioned publication reservation.
The approved dev synchronization is complete at
66cc995b08f08cf85682bbd0ead54803872b3de5. Both import additions were retained, the maintainer's rebase is preserved, and no force-push was used. All CI checks are green on this exact head, including every backend and frontend test shard. The prior conflict blocker is resolved; exact-head external re-review is still pending.