Skip to content

fix(codex): validate terminal turn ownership - #2754

Draft
timigod wants to merge 13 commits into
getpaseo:mainfrom
timigod:review/v025-codex-terminal-ownership-main
Draft

fix(codex): validate terminal turn ownership#2754
timigod wants to merge 13 commits into
getpaseo:mainfrom
timigod:review/v025-codex-terminal-ownership-main

Conversation

@timigod

@timigod timigod commented Aug 1, 2026

Copy link
Copy Markdown

Linked issue

None. Focused prerequisite extracted while reviewing #2721.

Type of change

  • Bug fix
  • New feature
  • Enhancement
  • Refactor
  • Docs

Reasoning

Codex accepts turn/start before its native turn ID is known, and provider notifications can arrive before startTurn() 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 stagedEvents contract 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 after thread/read confirms the exact native turn and status.

Goals

  • Keep terminals emitted before startTurn() returns staged and replay them after the manager turn ID binds.
  • Prevent an acknowledged cancellation from allowing a late pending start to reclaim foreground ownership.
  • Correlate Codex native turn IDs with the manager-owned foreground turn.
  • Ignore stale or unverified Codex terminal notifications without closing current work.
  • Cover cancel, replace, reload, rewind, explicit stale terminals, and ID-less completion deterministically.

Non-goals

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 pending startTurn() 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

  • One focused change
  • npm run typecheck passes
  • npm run lint passes
  • npm run format passes
  • QA evidence
  • Tests added or updated where it made sense

@timigod
timigod marked this pull request as ready for review August 1, 2026 17:39
@greptile-apps

greptile-apps Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR strengthens foreground-turn ownership around delayed starts and Codex terminal notifications.

  • Adds generation invalidation and staged lifecycle-event handling to AgentManager and AgentRunState.
  • Correlates Codex native turn IDs with manager turn IDs and reconciles ID-less terminal notifications through thread history.
  • Revises cancellation settlement behavior for acknowledged interrupts.
  • Expands deterministic coverage for cancel, replacement, reload, rewind, and stale terminal ordering.

Confidence Score: 4/5

The PR should not merge until replacement events are correlated with the correct generation and cancellation success reflects actual run settlement.

The invalidated-generation queue can intercept replacement lifecycle events, and acknowledged interrupts return a settled result while the provider run remains tracked and active.

Files Needing Attention: packages/server/src/server/agent/agent-run-state.ts, packages/server/src/server/agent/agent-manager.ts

Important Files Changed

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
Loading

Comments Outside Diff (1)

  1. packages/server/src/server/agent/agent-manager.ts, line 2468 (link)

    P1 Settled status precedes run settlement

    When an interrupt is acknowledged for an already-started foreground or autonomous run, cancelAgentRun returns settled while 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

Comment on lines +83 to +86
const invalidatedRun = this.invalidatedPendingRuns.get(agentId)?.values().next().value;
if (invalidatedRun && (event.type === "turn_started" || options.terminal)) {
invalidatedRun.stagedLifecycleEvents.push(event);
return true;

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.

P1 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

@timigod
timigod marked this pull request as draft August 1, 2026 17:57
timigod added 3 commits August 1, 2026 14:42
…odex-terminal-ownership-main-1

# Conflicts:
#	packages/server/src/server/agent/providers/codex/test-utils/fake-app-server.ts
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.

1 participant