Skip to content

fix(client): defer Codex turn custody mismatch - #2074

Open
L42y wants to merge 1 commit into
agent-team-foundation:mainfrom
L42y:fix/codex-turn-custody-deferred-input
Open

fix(client): defer Codex turn custody mismatch#2074
L42y wants to merge 1 commit into
agent-team-foundation:mainfrom
L42y:fix/codex-turn-custody-deferred-input

Conversation

@L42y

@L42y L42y commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Defer only the structured Codex active-turn ownership mismatch.
  • Keep the complete pending input batch until valid custody returns.
  • Add exact eight-entry, repeated-mismatch, and concurrency regressions.
  • Add a runtime QA case for session recovery custody.

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

  • The exact regression fails on the base head with 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 with EDQUOT before 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:

  • Review the exact current head deeply and completely.
  • Inspect every changed file and each transitive caller or consumer.
  • Inspect matching schemas, types, validators, persisted data, side effects, tests, CI, deployment paths, and existing review threads.
  • Trace each suspected defect to its root cause.
  • Group related symptoms under one finding.
  • Prefer a few detailed, actionable findings to many shallow, duplicate, or speculative comments.
  • Report every known material finding in the current round.
  • Run a second complete sweep before you post the review.
  • If no additional material finding remains, say so explicitly.

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.

@github-actions

Copy link
Copy Markdown

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 L42y left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings:

  1. 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 yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: request changes

  • Rationale: the new fence waits on the provider-reported turn but continues to treat the turn/start submission 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 -32602 mismatch, 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 yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@L42y

L42y commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Clean successor: #2075 at exact head 70fdc30e85ce35be42007ae5732778f1ef6da48d, based on current upstream main 1cc62191c496fdac9f92a888195dbf024509a877. GitHub confirms exactly three changed files: the Codex handler, focused handler tests, and runtime QA case. This PR remains open until the successor gate and review complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants