fix(acp): harden reviewer session lifecycle

[ewen-poch]

Problem

Reviewer Session IDs were registered only after asynchronous startup. A Reviewer could alias an active or in-flight Primary Session, temporarily narrow Primary permissions/tools, and delete shared metadata during disposal. Reviewer or Primary teardown failures could also leave stale startup continuations, bridge authority, reconnect barriers, or backend resources alive across a replacement connection.

Proposed change

Harden ACP session identity and teardown ownership without changing normal Reviewer behavior or public contracts.

flowchart LR
  Primary["Primary create / resume / adoption"] --> PPending["Owner-token Primary reservation"]
  PPending --> PSetup["Stage Specialist / Permission / model projections"]
  PSetup --> PCommit["Assert owner + publish atomically"]

  Reviewer["Reviewer start"] --> RPending["Owner-token Reviewer reservation"]
  RPending --> RActive["Promote ActiveSession owner"]
  RActive --> Bridge["Register Reviewer bridge authority"]

  Teardown["Disconnect / reconnect / close"] --> Fence["Invalidate startup generation"]
  Fence --> Detach["Detach old connection / process / lease"]
  Detach --> Cleanup["Best-effort cleanup; preserve first error"]

Loading

• Known stable Primary app IDs are reserved before the first network/setup await. Provider protocol IDs extend the same owner token synchronously when they become known.
• Pending identities grant no Reviewer, Permission, bridge, or capability authority. Specialist, Permission, and applied-model projections remain local until the owner token is revalidated and the Session is published.
• Active Reviewer ownership includes the concrete ActiveSession, so a late dispose from an old same-ID Reviewer cannot remove its successor's route, bridge scope, metadata, or temporary directory.
• Disconnect detaches generation-owned connection/process/bridge resources before asynchronous cleanup, continues after individual disposal failures, and rethrows the first lifecycle error after cleanup.
• Reconnect completion is generation-owned, so an older teardown cannot clear a newer provider/skills request or barrier.
• HTTP MCP routes and Notebook bearer capabilities stay provisional until the app Session is published. Failure cleanup unregisters every provisional route independently and releases the exact bearer generation, so a stale same-ID teardown cannot revoke its successor.
• A resume failure revalidates its Primary reservation before any broad cleanup or fresh adoption. Superseded attempts release only their concrete bearer lease; current owners also clean session-wide metadata and onSessionReleased. Provisional HTTP cleanup uses the transport mode captured by that startup rather than the later mutable framework.
• Context reset captures the concrete published ActiveSession before its first await. Only a startup that began without a usable connection (or behind an already-armed reconnect barrier) receives a one-shot reservation renewal across expected connection setup; an explicit teardown invalidates an older reset instead of letting it replace a same-ID successor.
• Every caller revalidates the concrete connection returned by ensureConnected before using it. Per-session deletion epochs supersede older reset/resume reservations, an in-flight deletion rejects later same-ID startups, and owner refcounts retain each epoch until every stale continuation has unwound.
• Connection bootstrap and per-session Permission/model/effort setup stay bound to one captured protocol connection. Initialize/auth/provider requests, event callbacks, cleanup, and the final status commit are generation-fenced; a stale setup cannot target or overwrite a same-ID successor connection.
• One-shot authentication/provider configuration is cleared after successful consumption or a same-generation bootstrap failure, while a superseded attempt cannot erase the successor generation's intent even when the resolver reuses the same object.
• Shared HTTP MCP host shutdown is generation-owned. A delayed disconnect or unexpected-close cleanup cannot close routes registered by a replacement connection; the newest disconnect still closes the host exactly once.
• Alias, Specialist, event, and state callbacks run outside the publication rollback boundary; callback failures are logged without destroying a Session that is already usable.

Scope and non-goals

• Reject collisions with active Primary IDs, fresh-adoption Provider IDs, active Reviewers, and pending Primary/Reviewer identities.
• Preserve Codex non-UUID fresh adoption, context reset, stable app ID/provider ID mapping, transcript replay, and resource cleanup semantics.
• Preserve latest-main Specialist immediate handoff appends/prompt identity and Compute Skill materialization/control-capability semantics.
• Preserve the collision or original startup/disposal error as primary; secondary cleanup failures are logged without replacing it.
• Preserve Electron/local Web/remote Web Reviewer APIs and existing normal flows.
• No CLI/Task Reviewer API, IPC/Web payload/event, schema, Session relationship, UI, Specialist capability, Permission allowlist, Compute policy, or Issue [Feature]: Provider-neutral multi-agent research orchestration #458 orchestration change.

Validation

• ACP/Reviewer/Immediate-handoff/Notebook cross-point focused suites: 430/430 passed.
• npm run typecheck: passed.
• npm run lint: passed with 0 errors; 19 pre-existing warnings remain outside this diff.
• Final-head npm test -- --run: 669 files passed, 15 skipped; 9,837 tests passed, 184 skipped.
• git diff --check: passed.
• Latest-main rebase: base a842397, head cd63636, 16 commits, ahead/behind 16/0.
• Fixed-SHA Spec review for a842397..cd63636 reports P0/P1/P2/P3 = 0/0/0/0. Standards reports P0/P1/P2 = 0/0/0 and three non-blocking P3 notes: prerequisite diff size, lifecycle-test discoverability, and a missing direct Notebook durable-binding combination test.

Coverage includes Reviewer/Primary same-ID races; create/resume/context-reset/fresh-adoption app/provider reservations; invalidated context reset racing a same-ID successor; reset/resume across delete, failed delete, and a second disconnect after connection setup; public connect, disconnect, shutdown, deferred reconnect, and unexpected close; same-ID successor permission setup; reentrant initialized/failure event disconnects; initialize/authentication intent failure cleanup; stale HTTP host close racing a successor route; real disposal failure followed by a fresh backend/bridge; stale Reviewer disposal after replacement; staged Specialist/Permission/model projection commits; partial/throwing HTTP route cleanup; published-session observer/alias failures; exact Notebook capability release; and disposal failure preserving required-model/startup errors while reservations are revoked.

Review focus

Please focus on generation ownership at teardown boundaries, concrete ActiveSession ownership for Reviewer disposal and context reset, the one-shot connection-setup renewal permit, captured connection ownership through bootstrap/session setup, and the no-await assertion-to-publication sections for Primary create/resume/adoption.

Real external ACP providers were not run; provider boundaries use the project's existing fakes. Merge by squash only after all required CI and AI review checks pass.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Identity reservations survive runtime teardown

src/main/acp/runtime.ts:643

Impact: disconnect() and unexpected connection-close cleanup clear active sessions but not the new pending reviewer/primary reservations. A disconnect racing an in-flight startup can leave the next startup rejected for the same ID, potentially indefinitely if the original operation hangs.

Recommendation: Invalidate in-flight startups and clear both pending reservation collections during every connection teardown before allowing new sessions.

[P2] Collision handling leaks provisioned session resources

src/main/acp/runtime.ts:1418

Impact: On a primary ID collision, the new code only disposes the local ActiveSession. MCP HTTP routes created before startup are not yet recorded in routing maps, so they cannot be unregistered; the same leak exists in the resume/adoption collision branches at lines 2058 and 2155. The failed protocol session and its scoped resources remain until full runtime disconnect.

Recommendation: Centralize collision abort cleanup to cancel/close the protocol session and unregister all MCP routing IDs created for that attempt before throwing.

Summary: Static inspection found two lifecycle defects in the new identity-reservation and collision cleanup paths. No project commands were run.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Invalidate pending identity reservations during connection teardown

src/main/acp/runtime.ts:643

Impact: pendingReviewerSessionIds and pendingPrimarySessionIds are not cleared by clearReviewerSessionState() and are excluded from hasBlockingActivity(). A reconnect or connection close during session/new/setMode startup can leave an ID reserved indefinitely or allow the old startup to activate against a new connection/bridge, causing subsequent collisions or incorrect session routing.

Recommendation: Treat pending startups as blocking activity, clear or cancel their reservations during teardown, and guard the pending-to-active transition against a stale connection generation before registering bridge authority.

Summary: The new pending session identity state is not integrated with connection teardown or reconnect gating, creating a lifecycle and routing race during interrupted startup.

[ewen-poch]

Rebased onto current main (bbbd1ac) and force-updated to 2337f47. The latest review fixes add generation-owned reconnect/teardown, owner-token Reviewer disposal, staged Primary Specialist/Permission/model projections, and best-effort startup cleanup that preserves the primary error while revoking Notebook capabilities. Focused ACP/Reviewer tests pass 334/334, typecheck and zero-error lint pass, and two independent fixed-SHA closure reviews report 0 findings. The two unrelated full-suite timing failures both passed on isolated rerun; merge remains gated on fresh CI and AI review.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Clean up provisional MCP routes on startup collision

src/main/acp/runtime.ts:2280

Impact: createMcpServers registers HTTP routes before fresh-session adoption validates the provider/app identities. If the new collision branch runs, the session is disposed but the provisional artifact, notebook, or skill-import routes are never unregistered, leaving stale session-scoped resources active until a full disconnect.

Recommendation: Track provisional routing IDs and unregister them on every startup failure, including identity-collision branches, before propagating the collision error.

Summary: Static inspection found a stale MCP-route leak on newly added session-collision paths.

[ewen-poch]

Force-updated to d882005 on latest main (74a09ec).

This closes the prior provisional-route finding and the deeper same-ID capability lifecycle cases:

• every provisional Artifact/Notebook/Skill HTTP route is cleaned independently without replacing the startup error;
• Notebook cleanup uses the exact connection release, so a stale startup cannot revoke a same-ID successor;
• create/resume/fresh-adopt transfer route and bearer ownership before alias/Specialist/event/state observers run;
• context reset atomically publishes the replacement Notebook release and retires the previous generation, including adapters whose replacement connection has no release callback.

Exact-head evidence: focused 357/357, full 650 files / 9,668 tests, typecheck passed, lint 0 errors, git diff --check passed, and independent Standards + Spec reviews report 0 actionable findings. Public IPC/Web/Electron/CLI behavior, Specialist/Permission/Compute policy, persistence, UI, and Issue #458 boundaries remain unchanged. Merge remains gated on the fresh CI and Codex review for this head, then squash only.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defects in the requested commit range.

[ewen-poch]

Rebased the branch onto current main (4edb5e8) and force-updated the PR to exact head 7c47428.

The latest-main overlap with #611 exposed one real composition regression: the Claude/OpenCode-ID fresh-adoption fast path did not pass A3's existing Primary identity reservation. 7c47428 now carries the same owner token through adoption, matching the adjacent cross-framework, Codex, and unresumable-resume paths. The focused regression passes.

Latest-main Specialist immediate handoff/prompt identity and Compute Skill/control-capability behavior remain intact. No IPC payload/channel, persisted schema, UI, Electron/Web/CLI/Task availability, Permission policy, or Issue #458 orchestration contract changed.

Local exact-head evidence:

• focused ACP/Reviewer/Immediate-handoff/Notebook suite: 417/417
• full suite: 670 files and 9,813 tests passed; 15 files and 184 tests skipped
• typecheck: passed
• lint: 0 errors; 19 pre-existing warnings
• Spec review: P0/P1/P2/P3 = 0/0/0/0
• Standards review: no P0/P1/P2; two non-blocking P3 judgement notes recorded in the PR description

Waiting for CI and GitHub AI review on 7c47428; merge remains squash-only after the exact head is green.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Prevent stale resume cleanup from revoking a successor capability

src/main/acp/runtime.ts:2341

Impact: A superseded resume can revoke Notebook/compute access belonging to a successor using the same app session ID.

Recommendation: Check reservation ownership before broad cleanup and fresh adoption; stale startups should release only their concrete provisional lease.

[P1] Always clean up provisional HTTP MCP routes

src/main/acp/runtime.ts:4886

Impact: A framework-switch race can leave failed-startup artifact, notebook, or skill routes registered with authenticated capabilities.

Recommendation: Remove the mutable framework check or capture the startup transport mode for cleanup.

Summary: Static inspection found two concrete lifecycle defects. Branch and PR title prechecks are valid.

[ewen-poch]

Updated PR #604 to exact head c2e2d09 with fixes for both findings from the 7c47428 Codex review.

• A failed resume now revalidates its Primary identity reservation immediately after the network rejection. If superseded, it releases only its captured concrete Notebook bearer lease, skips broad app-ID cleanup, and does not enter fresh adoption.
• Provisional MCP cleanup now uses the HTTP/stdio transport mode captured immediately before that startup built its servers. A later framework switch cannot suppress cleanup; reservation ownership still prevents an old same-ID startup from unregistering its successor.

New/strengthened tests cover the stale resume publishing a same-ID successor before the old not-found arrives, exact-vs-legacy capability release, and an HTTP startup failing after the framework pointer switches.

Exact-head evidence: focused 418/418; full 670 files / 9,814 tests passed (15 files / 184 tests skipped); typecheck passed; lint 0 errors / 19 baseline warnings; Spec P0-P3 all zero; Standards no P0/P1/P2. No public IPC, persistence, UI, Specialist/Permission/Compute surface, or Issue #458 change.

Waiting for CI and GitHub AI review on c2e2d09; merge remains squash-only after that exact head is green.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Do not skip session-wide cleanup after releasing a provisional Notebook capability

src/main/acp/runtime.ts:4943

Impact: When an owner-scoped release exists, this return skips app-session cleanup and the wired onSessionReleased hook after startup failure, leaving stale completion-handoff authority and session metadata.

Recommendation: Release the concrete capability, then perform broad cleanup when ownsStableIdentity is true; skip broad cleanup only for superseded startups.

Summary: Static inspection found one lifecycle regression. Branch and PR metadata prechecks are valid; no project commands were run.

[ewen-poch]

Updated PR #604 to exact head f1c319e, amending the provisional cleanup commit to close the c2e2d09 Codex P2 without adding another commit.

The cleanup rule is now explicit:

• always attempt the captured exact bearer release;
• when the startup still owns the stable identity, continue session-wide metadata cleanup and onSessionReleased;
• when reservation revalidation says the startup was superseded, skip broad app-ID cleanup so the same-ID successor remains valid.

A new regression forces a post-session-start permission failure and verifies that the current owner performs both exact release and releaseSessionCapabilities. The stale-resume and framework-switch tests still verify both earlier P1 fences.

Exact-head evidence: focused 419/419; full 670 files / 9,815 tests passed (15 files / 184 tests skipped); typecheck passed; lint 0 errors / 19 baseline warnings; Spec P0-P3 all zero; Standards no P0/P1/P2. No public IPC, persistence, UI, Specialist/Permission/Compute surface, or Issue #458 change.

Waiting for CI and GitHub AI review on f1c319e; merge remains squash-only after that exact head is green.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Do not let an invalidated context reset adopt a successor session

src/main/acp/runtime.ts:1903

Impact: After a reset is invalidated by disconnect/reconnect, a newer resume can publish the same app ID. The stale reset then accepts that successor, disposes it, and replaces its provider session, losing newer context and routing state.

Recommendation: Tie renewal to the reset’s original session/generation, or reject renewal after explicit teardown invalidation. Never treat a successor’s current app ID as the reset’s published identity.

Summary: Static inspection found one lifecycle race in context-reset identity renewal. Branch and pull-request title prechecks are valid.

[ewen-poch]

Updated PR #604 locally to exact head ecfdce8 with the fix for the context-reset race reported on f1c319e and the two follow-up stale-renewal findings from fixed-SHA review.

• A context reset now captures the concrete published ActiveSession before its first await and rejects completion when a same-ID successor has replaced that owner.
• Primary identity reservations receive a one-shot generation renewal only when startup began without a usable connection or behind an already-armed reconnect barrier.
• A reset begun against a live connection receives no renewal permit, so explicit disconnect/reconnect invalidates it instead of allowing it to dispose and replace a successor.
• Existing disconnected resume and pre-armed provider reconnect behavior remains unchanged.
• A concrete connection identity/status fence rejects a continuation if another disconnect or shutdown occurs after connection setup returns.
• Per-session deletion epochs supersede reset/resume reservations that began before delete. An in-flight deletion rejects later same-ID startups; refcounted, idempotent reservation release retains the epoch until all stale owners unwind and safely allows retry after a failed delete.

The new regression stalls reset during ensureConnected, disconnects, publishes a same-ID successor on a new process, then releases the stale reset. It verifies the reset is superseded, the successor remains active, and no stale fresh adoption occurs.

Local exact-head evidence:

• focused ACP/Reviewer/Immediate-handoff/Notebook suite: 425/425
• full suite: 670 files and 9,821 tests passed; 15 files and 184 tests skipped
• typecheck: passed
• lint: 0 errors; 19 pre-existing warnings
• git diff --check: passed
• base/head: 4edb5e8..ecfdce8, ahead/behind 14/0
• exact-SHA Spec: P0/P1/P2/P3 = 0/0/0/0
• exact-SHA Standards: P0/P1/P2 = 0/0/0; three judgement-only P3 notes recorded in the PR description

No public IPC, persistence, UI, Electron/Web/CLI/Task availability, Specialist/Permission/Compute policy, or Issue #458 orchestration contract changed. I will now push and wait for fresh CI plus GitHub Codex review. Merge remains squash-only.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Prevent stale startup permission setup from mutating a successor session

src/main/acp/runtime.ts:1638

Impact: An invalidated startup can continue using mutable this.connection; if the connection is replaced and the provider reuses the session ID, its stale mode request can apply the old permission profile to the successor while the runtime publishes the successor as Ask, silently elevating tool authority.

Recommendation: Capture the startup’s connection/generation and pass it through permission, model, and effort setup; assert it is still current immediately before each protocol request and abort when superseded.

Summary: Static inspection found one merge-blocking ACP lifecycle race: startup reservations prevent stale publication but do not prevent stale setup requests from mutating a same-ID successor session after teardown/reconnect.

[ewen-poch]

Rebased A3.0 onto latest origin/main=a842397 (including #614/#615/#616/#617/#613/#618) and fixed the connection-setup race reported by GitHub Codex on ecfdce8.

• Permission, model, and effort setup now use the concrete connection captured for that session startup; a stale setup cannot switch to a successor connection that reuses the same Provider Session ID.
• Initialize, authentication, and provider configuration stay on one concrete bootstrap connection and are fenced after every await.
• One-shot auth/provider intent is cleared on success or a same-generation bootstrap failure, but stale generations cannot clear a successor's possibly identity-equal intent.
• Reentrant Agent initialized / Connection failed callbacks cannot let an old connect clean up or overwrite a successor's status.
• Added regressions for same-ID successor permission setup, initialized/failure callback disconnects, and initialize/authentication intent failures.

Exact-head local evidence for a842397..5585aef:

• focused ACP/Reviewer/Immediate-handoff/Notebook suite: 429/429
• full suite: 669 files and 9,836 tests passed; 15 files and 184 tests skipped
• typecheck: passed
• lint: 0 errors; 19 pre-existing warnings
• git diff --check: passed
• ahead/behind: 15/0
• fixed-SHA Standards: P0/P1/P2 = 0/0/0; three non-blocking P3 maintainability/coverage notes recorded in the PR description
• fixed-SHA Spec: P0/P1/P2/P3 = 0/0/0/0

No public IPC, persistence, UI, Electron/Web/CLI/Task availability, Specialist/Permission/Compute policy, or Issue #458 orchestration contract changed. I will wait for fresh exact-head CI and GitHub Codex review before squash merging.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Guard MCP host shutdown against a newer connection

src/main/acp/runtime.ts:2725

Impact: An older disconnect can close the shared HTTP MCP host after a replacement connection starts, stopping routes used by the replacement session and breaking notebook, artifact, and skill MCP calls.

Recommendation: Make HTTP-host cleanup generation-aware, or use generation-specific hosts so an older teardown cannot close a successor’s host.

Summary: Static inspection found one lifecycle race in concurrent disconnect/reconnect handling.

[ewen-poch]

Fixed the exact-head Codex P1 about an older disconnect closing a successor's shared HTTP MCP host.

• disconnect() now passes its captured teardown generation into host cleanup.
• The generation is checked synchronously immediately before host.close(); a stale teardown skips the close once a successor connection owns the runtime.
• Unexpected-close cleanup carries the same owner fence. Terminal shutdown() remains intentionally unconditional.
• The regression parks an old disconnect after synchronous detach/route clear, publishes a successor HTTP Notebook route, then releases the old teardown. It verifies the stale close is skipped, the successor remains connected, and the newest disconnect closes the host exactly once.

Exact candidate a842397..cd63636:

• focused ACP/Reviewer/Immediate-handoff/Notebook: 430/430
• full suite: 669 files and 9,837 tests passed; 15 files and 184 tests skipped
• typecheck: passed
• lint: 0 errors; 19 pre-existing warnings
• Spec: P0/P1/P2/P3 = 0/0/0/0
• Standards: P0/P1/P2 = 0/0/0; three pre-existing non-blocking P3 notes remain in the PR description
• git diff --check: passed

No public IPC, persistence, UI, Electron/Web/CLI/Task availability, Specialist/Permission/Compute policy, or Issue #458 orchestration contract changed. Merge remains gated on fresh CI and exact-head GitHub Codex review, then squash only.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defects in the pull request changes.