Skip to content

fix(mcp): negotiate per-turn lifecycle notifications - #1851

Merged
wqymi merged 4 commits into
mainfrom
codex/mcp-turn-lifecycle
Jul 29, 2026
Merged

fix(mcp): negotiate per-turn lifecycle notifications#1851
wqymi merged 4 commits into
mainfrom
codex/mcp-turn-lifecycle

Conversation

@wqymi

@wqymi wqymi commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

  • negotiate a provider-neutral MCP turn lifecycle capability
  • propagate one stable turn context through direct tool calls
  • serialize terminal notifications and recover cleanly from timeout, abort, or rejection
  • keep all behavior capability-gated with no browser- or Codex-specific branches

Review follow-ups

  • Dropped the dead turn context fields. Tool.Context.turnID / turnActorID were
    residue of the MCP-in-tool_script path that main removed in 8c29041a8
    (request-scoped tool discovery). Nothing reads them at this head, so both the
    declaration and its population in SessionPrompt are deleted rather than left as
    latent surface.
  • A stuck lifecycle send no longer taxes later turns. A client.notification()
    that never settled kept its pending-map entry forever, so every subsequent turn for
    that client queued behind it, waited out the full 1s timeout and was dropped — a
    permanent, invisible per-turn stall with no signal. Each pending send now records
    startedAt, and a send that has outlived the per-turn budget is released (with a
    log.warn) so the next turn sends immediately. The orphaned promise is never awaited
    again and its eventual rejection is still swallowed, so no unhandled rejection
    escapes. Serialization for healthy sends is unchanged.

Verification

  • test/mcp/lifecycle.test.ts: 31/31 pass (30 pre-existing + 1 new regression test
    proving a turn after a permanently hung send is still attempted promptly)
  • prompt-effect MCP lifecycle cases: 4/4 pass
  • bun typecheck: exit 0, 12/12 tasks
  • oxlint and Prettier checks passed

Independent re-verification (follow-up pass, no code change)

Both review follow-ups above were re-checked from scratch against this head; no further
change was needed, so this branch is unchanged at 7b487037b.

  • Finding 1, the boundary question re-traced. The user-facing tool contract is
    packages/plugin/src/tool.ts:4-21 (ToolContext: sessionID, messageID, agent, directory,
    worktree, abort, metadata, ask) — turnID/turnActorID were never declared on it. They were
    nonetheless runtime-visible to plugin tools, because src/tool/registry.ts:184 spreads
    ...toolCtx into pluginCtx before calling def.execute at :189. Reachable only through an
    as any cast against an undeclared field, that is incidental spread leakage rather than an API
    surface — documenting it would have enshrined an accident — so deletion remains the right call.
  • Finding 2, revert probe. Restoring only the pre-fix src/mcp/index.ts (3 insertions,
    37 deletions) while keeping the new test reproduces the bug exactly:
    expect(stuckState.notificationCalls).toBe(2) → Expected: 2 / Received: 1 at
    test/mcp/lifecycle.test.ts:739 — the turn after the hung send is never attempted. Restoring the
    fix returns 1 pass / 12 expects.
  • Invariants confirmed intact: the pending map is still a WeakMap keyed by client instance
    (src/mcp/index.ts:94) with identity checks before every delete (:126, :147), and all four
    healthy-path notificationMaxInFlight === 1 assertions are unchanged and passing.
  • Measured here: test/mcp/ 51 pass / 0 fail (172 expects, 6 files) · test/mcp/lifecycle.test.ts
    31 pass / 0 fail (102 expects) · bun typecheck exit 0, 12/12.
  • Reviewer caution: the four .live prompt-effect MCP lifecycle cases have hard-coded 3s/5s
    budgets and flake on a loaded machine. Across four runs at load 23-27 the failing member varied
    ({cancelled, hangs} → {cancelled} → {cancelled}), and with the fix reverted a different member
    failed ({hangs}) — a non-deterministic set, i.e. scheduler starvation, not a regression.
  • Known residual (bounded, by design): release is age-checked at the start of the next send
    rather than eagerly on the timeout path, to avoid racing the outer
    Effect.timeout(TURN_LIFECYCLE_NOTIFICATION_TIMEOUT) at the same duration. A turn arriving inside
    the first 1s after a hung send can therefore still pay one timeout and be dropped; every later turn
    releases the orphan and sends immediately. The cumulative head-of-line block is gone; what remains
    is ordinary single-notification best-effort loss.

@wqymi
wqymi force-pushed the codex/mcp-turn-lifecycle branch 2 times, most recently from f0219d9 to 782295e Compare July 27, 2026 07:55
wqymi added 4 commits July 27, 2026 22:37
@wqymi
fix(mcp): negotiate per-turn lifecycle notifications
9b0e030 
Propagate one stable turn context through MCP tool execution and notify negotiated servers exactly once when a turn completes, is cancelled, or fails. Keep the behavior capability-gated and provider-neutral, with serialization and recovery tests for overlapping notifications.
@wqymi
fix(mcp): make lifecycle negotiation cancellation-safe
cd16c20 
Advertise the exact lifecycle v1 client capability and propagate turn cancellation into in-flight MCP calls so terminal notifications cannot race active tool work.
@wqymi
test(mcp): pin lifecycle MCP tests to a direct-MCP model
54e0c33 
main's request-scoped MCP discovery gates MCP tools behind mcp_tool_search
for the default test model, so mcp_lifecycle never executed and no turn
context was captured. Pin the two tool-calling lifecycle tests to the
non-GPT model that still exposes MCP tools directly.
@wqymi
fix(mcp): drop dead turn context fields and unblock stuck lifecycle s…
7b48703 
…ends

turnID/turnActorID on the tool Context were residue of the MCP-in-tool_script
path that main removed in 8c29041; nothing reads them, so delete them.

A lifecycle notification that never settles kept its pending-map entry forever,
so every later turn for that client queued behind it, waited out the 1s timeout
and was dropped -- a permanent, invisible per-turn stall. Record when a send
started and release an entry that has outlived the budget so the next turn sends
immediately, without ever awaiting the orphaned promise.
@wqymi
wqymi force-pushed the codex/mcp-turn-lifecycle branch from 782295e to 7b48703 Compare July 27, 2026 14:40
@wqymi
wqymi merged commit 7167c47 into main Jul 29, 2026
6 checks passed
wqymi added a commit that referenced this pull request Jul 29, 2026
@wqymi
fix(session): report a dropped empty shell to session.post and MCP tu…
1288499 
…rn-lifecycle

The empty-shell guard deletes a contentless errored assistant row, so
"how did this turn end?" can no longer be answered by re-reading the
session. runLoop's tail already handles that via `droppedShell`, but
`firePostSession` is a SECOND, independent reader of the turn's final
assistant — it re-derives it from the persisted slice with
filterCompactedEffect + findLast(role === "assistant").

With the row gone, findLast skips the hole to an older successful
assistant (or finds none), so `finalIsError` went false and an
`llm.error(400)` turn reported outcome "completed". That silently
mis-reported the `session.post` plugin outcome and, since #1851 (merged
7167c47) derives the MCP turn-lifecycle `status` from the same two
flags, published `status: "completed"` for a failed turn — breaking
"MCP lifecycle emits one error notification when the outer run fails".

Consult `droppedShell` first here too, mirroring the precedence runLoop
already uses for `final`. Parts are empty because the row is gone.
This was referenced Jul 29, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@wqymi