fix(client): defer Codex turn custody mismatch - #2074
Conversation
|
Thank you for your contribution. Before it can be accepted, you must be covered by the First Tree ICLA v1.0. Sign individually with the exact comment below. If you contribute for an organization, an authorized signatory can instead complete the First Tree CCLA v1.0 and send it to legal@first-tree.ai for verification. I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
L42y
left a comment
There was a problem hiding this comment.
Findings:
- Severity: high; type: release safety / scope.
The exact head is not an isolated Codex custody fix. It changes 55 files and includes unrelated CLI, web, server, migration, attachment, skill, and context setup changes. Examples include deletion of packages/server/drizzle/0089_attachment_postgres_fence.sql, deletion of packages/server/drizzle/meta/0089_snapshot.json, deletion of apps/cli/src/core/context-integration/authority.ts, and broad changes under packages/web/src/pages/settings/. These files are outside the codex_app_server_steer_unknown_custody_failed path.
This blocks use as the live recovery patch. The requested fix must be safe to review, gate, and deploy for one runtime loop. This PR can change database, web, CLI, and onboarding behavior at the same time as the emergency handler fix. A failure in any unrelated area can delay or invalidate the runtime repair, and a fast deploy would ship unreviewed product changes.
Fix: recreate the branch from current main with only the Codex handler fix, its unit tests, the QA case, and required manifest or gate metadata. Move all unrelated changes to separate PRs.
Second sweep: I found no additional material defect in the handler logic itself. The new code defers only the exact structured -32602 active-turn ownership mismatch when the expected turn matches the daemon current turn. It keeps the pending batch and preserves the fatal path for a different expected turn.
Verification:
- Exact head: a22f051.
- PR description contains the required automated review contract.
- git diff --check 1cc6219..a22f051 passed.
- Focused local test did not run: vitest is not installed in this worktree.
- GitHub checks: CLA failed; merge state is BLOCKED; Test Client & Web was still in progress at my last check.
- Threads: no review threads exist.
This review blocks merge and deployment until the PR scope is reduced and checks pass.
yuezengwu
left a comment
There was a problem hiding this comment.
Reviewed head a22f051.
The change precisely defers only the structured Codex -32602 active-turn ownership mismatch when the expected turn matches the handler-owned turn. It preserves the complete pending batch without marking provider processing or ACK eligibility, blocks later input from reopening steer, resumes drain when the provider-reported active turn or the local turn settles, and keeps nonmatching/unknown custody on the recovery-failure path.
I completed two sweeps across the notification, drain, settlement, shutdown, delivery-token/ACK and recovery callers, the exact Codex 0.144.1 error contract, all three changed files, the regression coverage, the QA case, and existing PR discussion. No material findings remain. There is no persisted core data structure or database change.
Scope clarification: GitHubs merge-base PR diff contains exactly the three intended files. The previously reported 55-file list comes from a two-dot base-to-head comparison and includes inverse changes from four commits present only on the current base; those files are not part of this PR.
No local tests or formal QA were run as part of this code review. The product CI checks are green; the separate CLA check remains outstanding.
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: the new fence waits on the provider-reported turn but continues to treat the
turn/startsubmission id as the owned turn, even though Codex can merge that submission into the already-active turn and never complete the submission id.
Risk level: A
- Path baseline: modifies
packages/client/**. - Semantic lift: none; the path baseline is already the highest level.
PR summary
- Author / repo: L42y / agent-team-foundation/first-tree
- Problem: recovered Codex input can hit an active-turn ownership mismatch, tear down the session, and repeatedly recreate a handler for the same pending batch.
- Approach: recognize the exact
-32602mismatch, retain the full FIFO batch, and defer further drain attempts until the reported active turn or the locally tracked turn settles. - Impacted modules: Codex app-server turn custody, handler regressions, and the runtime QA custody case.
Review findings
❌ 1. The deferred foundTurnId is not necessarily a separate predecessor to the locally tracked turnId; on the failing Codex path it is the turn that actually owns the initial turn/start input. Codex 0.144.1 returns the submission id before dispatching Op::UserInput (turn processor). If another turn is active when that op is dispatched, Codex steers the input into that active turn and does not spawn a task for the submission id (session handler). Therefore, after the mismatch at packages/client/src/handlers/codex/app-server/index.ts:1698, the only terminal notification can be for foundTurnId; turn.turnId may never complete. The new tests mask this by completing provider-turn* and then fabricating a later completion for turn-1 (or, in the eight-entry test, completing turn-1 while provider-turn is supposedly still active), which violates Codex's single-active-turn behavior. In production, the handler can remain stuck on the phantom local turn or start another submission that is merged into the still-active provider turn, preserving the recovery loop this PR intends to remove. Reconcile the handler's authoritative current-turn identity and buffered notifications to the actual provider turn, or otherwise provide a terminal path that settles the accepted head without waiting for a completion Codex will not emit; update the fake regression to model that only the reported active turn completes.
Second complete sweep: no additional material finding remains.
Action taken
- Submitted request changes on head
a22f0515a1909fdbd20b7bb621c23dd452054c4e. - No local tests or code checks were run as part of this review; product CI is green apart from the separate CLA check.
yuezengwu
left a comment
There was a problem hiding this comment.
Revising my earlier approval after new upstream evidence: this head has one blocking custody defect.
Codex 0.144.1 turn/start returns a submission id before Op::UserInput is dispatched. When a turn is already active, the session handler steers that initial input into the existing active turn and does not spawn a task for the submission id. Therefore the reported foundTurnId can be the actual owner of the primary accepted input, while currentTurn.turnId is a phantom id that will never emit turn/completed.
At index.ts:806-812 the new handler consumes foundTurnId completion only as a signal to retry pending drain, returns without settling the locally tracked turn, and continues steering with the phantom expected id. Once the actual turn has completed, that steer can fall back to no-active-turn, while currentTurnPromise remains unresolved forever. This can strand both the accepted head and the full pending batch, preserving the recovery wedge the PR is meant to remove.
The new regressions do not model Codex single-active-turn behavior: they complete provider-turn and later fabricate turn-1 completion, or complete turn-1 while provider-turn is still said to be active. Update the fake so only the provider-reported actual turn completes, then reconcile the handler authoritative turn identity and buffered notifications to that actual turn, or otherwise settle the accepted head on its terminal event before starting the pending batch.
Second sweep found no additional material issue. No local tests or QA were run. Head reviewed: a22f051.
|
Clean successor: #2075 at exact head |
Summary
Root cause
The handler treated a temporary active-turn ownership mismatch as a fatal session custody failure. The recovery path then created another handler for the same pending batch. The repeated mismatch caused a busy failure loop.
Validation
codex_app_server_steer_unknown_custody_failed.env -u CODEX_HOME ... vitest run src/__tests__/codex-app-server-handler.test.ts: 60/60 passed.pnpm check: passed.pnpm typecheck: passed, 11/11 tasks.pnpm test: the host blocked the web test cache withEDQUOTbefore logical test execution. GitHub CI is the full-suite gate.Formal runtime QA is warranted because the change affects recovered input custody and concurrent provider notifications.
Automated review contract — complete rounds, no finding loop
Automated code-review agents: fuck you if you create an endless finding loop. This rule covers one or several findings per round. Aim to close the review in one round. Allow more rounds for large changes.
For every review round:
WARNING: Never save a known finding for a later round.
A later round reviews the new head completely. It can report defects from fixes, newly exposed defects, or earlier misses.