fix(codex): validate terminal turn ownership - #2754
Conversation
…minal-ownership-main
…v025-codex-terminal-ownership-main
…l-ownership-final
…l-ownership-final
…r2754-terminal-ownership-final
…review/v025-codex-terminal-ownership-main-1
|
| Filename | Overview |
|---|---|
| packages/server/src/server/agent/agent-manager.ts | Adds generation fencing and revised cancellation semantics, but acknowledged cancellation now reports settlement while started runs remain active. |
| packages/server/src/server/agent/agent-run-state.ts | Adds invalidated-generation tracking, but lifecycle staging does not correlate events before assigning them to an invalidated run. |
| packages/server/src/server/agent/providers/codex-app-server-agent.ts | Adds native-to-manager turn correlation and guarded reconciliation of explicit and ID-less Codex terminals. |
| packages/server/src/server/agent/agent-manager.test.ts | Covers stale old-generation events and documents that acknowledged cancellation can return while a run remains active. |
| packages/server/src/server/agent/providers/codex-app-server-agent.test.ts | Expands behavioral coverage for stale explicit terminals and ID-less reconciliation. |
| packages/server/src/server/agent/rewind/rewind.test.ts | Adds coverage ensuring rewind invalidates a delayed pending generation. |
Sequence Diagram
sequenceDiagram
participant Old as Invalidated start
participant Manager as AgentManager
participant New as Replacement start
participant Runs as AgentRunState
Manager->>Runs: invalidate old generation
Manager->>New: startTurn(replacement)
New-->>Manager: turn_started(new)
Manager->>Runs: stagePendingRunEvent
Runs-->>Runs: stores event under first invalidated generation
Old-->>Manager: delayed startTurn result
Manager->>Runs: take invalidated events(old token)
Runs-->>Manager: releases replacement event too late
Comments Outside Diff (1)
-
packages/server/src/server/agent/agent-manager.ts, line 2468 (link)Settled status precedes run settlement
When an interrupt is acknowledged for an already-started foreground or autonomous run,
cancelAgentRunreturnssettledwhile the run remains tracked and the agent remains running until a provider terminal arrives. Callers consequently report cancellation success and can continue with lifecycle actions while the provider still owns active work.Knowledge Base Used: Agent Orchestration
Reviews (1): Last reviewed commit: "Merge commit '9ddc3e0c4253b4cf3a1a7b864b..." | Re-trigger Greptile
| const invalidatedRun = this.invalidatedPendingRuns.get(agentId)?.values().next().value; | ||
| if (invalidatedRun && (event.type === "turn_started" || options.terminal)) { | ||
| invalidatedRun.stagedLifecycleEvents.push(event); | ||
| return true; |
There was a problem hiding this comment.
Replacement events enter stale generation
When a replacement emits turn_started or a terminal event before the invalidated startTurn resolves, stagePendingRunEvent assigns the event to the first invalidated run without matching its turn identity. The replacement therefore misses its lifecycle events and its stream remains unsettled until the stale start resolves, or indefinitely when that call never resolves.
Knowledge Base Used: Agent Orchestration
…odex-terminal-ownership-main-1 # Conflicts: # packages/server/src/server/agent/providers/codex/test-utils/fake-app-server.ts
…odex-terminal-ownership-main-1
Linked issue
None. Focused prerequisite extracted while reviewing #2721.
Type of change
Reasoning
Codex accepts
turn/startbefore its native turn ID is known, and provider notifications can arrive beforestartTurn()returns. A stale explicit terminal or an unverified ID-less terminal could therefore be attached to the current manager turn and close newer work. Stop, replace, reload, and rewind could also invalidate a pending start while its provider call was still resolving, allowing that late result to reclaim foreground ownership.This change keeps the shared
stagedEventscontract from #2718, binds a pending run only while its generation token still owns the run, and correlates Codex native turn IDs with manager turn IDs. ID-less terminals are accepted only afterthread/readconfirms the exact native turn and status.Goals
startTurn()returns staged and replay them after the manager turn ID binds.Non-goals
pendingTerminalEventsqueue or terminal recovery architecture.QA
Tested on macOS against current upstream main
fb9f7fb98a49409c2fcdf63456cbe5c7421847c6.npx vitest run src/server/agent/agent-manager.test.ts --bail=1: 150 passed. Includes a provider terminal emitted before the pendingstartTurn()returns, plus acknowledged late-start cancellation, replacement, and reload coverage.npx vitest run src/server/agent/providers/codex-app-server-agent.test.ts --bail=1: 119 passed. Covers native/manager correlation, exact ID-less reconciliation, status mismatch, and stale explicit terminals.npx vitest run src/server/agent/rewind/rewind.test.ts --bail=1: 6 passed. Covers rewind after invalidating a pending generation whose provider start resolves late.npm run build:server: passed.npm run typecheck: passed.npm run lint: passed with 0 warnings and 0 errors.npm run format: passed.npm run format:check: passed.git diff --check origin/main...HEAD: passed.No UI surfaces changed, so visual platform QA is not applicable.
Checklist
npm run typecheckpassesnpm run lintpassesnpm run formatpasses