Skip to content

fix(agent-adapter): replay codex code-mode MCP calls, Script envelopes, and skill rows - #435

Open
Zerlight wants to merge 2 commits into
ruocheng/code-575from
ruocheng/code-576
Open

fix(agent-adapter): replay codex code-mode MCP calls, Script envelopes, and skill rows#435
Zerlight wants to merge 2 commits into
ruocheng/code-575from
ruocheng/code-576

Conversation

@Zerlight

@Zerlight Zerlight commented Aug 9, 2026

Copy link
Copy Markdown
Member

Summary

Reseeding a codex conversation from history (any SWR refocus revalidation rebuilds the
transcript from the rollout replay) was badly lossy for codex 0.144.x code-mode sessions:

  • Nested MCP calls (e.g. the branded list_issues · linear card) vanished — code mode persists
    them only as event_msg mcp_tool_call_end rows, which mapCodexHistoryEvents ignored.
    They now replay as MCP tool cards through the shared codexMcpSlug; the event's call_id IS
    the live mcpToolCall item id (verified in codex-rs rust-v0.144.6), so replayed and live
    cards converge by id and the seed's covered-by-seed cut holds. Legacy response-backed calls
    (announce → end → output in real rollouts) skip the synthesized end row — the trailing settle
    would otherwise overwrite a structured failed with completed.
  • Exec-script rows replayed with the raw code-mode envelope as content; parseCodexToolOutput
    now unwraps Script completed|failed|terminated|running with cell ID N / Wall time / Output:
    and fails failed/terminated scripts. apply_patch verification failed receipts settle as
    failed instead of completed.
  • Machine-injected <skill> (the invoked SKILL.md), <recommended_plugins>, and
    <codex_internal_context user rows rendered as real user bubbles; they join
    SYNTHETIC_USER_MARKERS.

Transport safety for the newly replayed results: sliceHistoryEventPage now counts tool
rawOutput toward the page budget alongside attachments, and a single stored MCP result caps at
256 KiB serialized (oversized results replay status-only), so result-heavy transcripts fan
across cursor pages instead of exceeding the tunnel's reassembly budget.

Stacked on #434 (ruocheng/code-575); review this branch's delta against it.

Closes CODE-576

Verification

  • 7 new unit tests (MCP end replay + plugin split + Err/isError statuses, legacy dedup,
    oversized-result cap, Script envelope + statuses, verification-failure settle, skill-row
    filtering, page-budget accounting); the two focused history files pass 48/48 and the full
    vitest run passes 2778 (--maxWorkers=2).
  • Smoke against real rollouts through the mapper: the affected linear session replays its failed
    mcp__linear__list_issues / list_teams cards with no <skill> bubble and clean exec bodies;
    the worst MCP-heavy rollout (~18 MB of results, 1301 events / 141 MCP cards) pages as
    10.6 / 6.6 / 4.5 MB — all under the 20,971,520 transport budget (previously one over-budget page).
  • Protocol facts verified against the actual 0.144.6 binary (app-server generate-ts), the
    matching codex-rs source, and a sweep of 840 local rollouts (0.140 → 0.146-alpha).

Checklist

  • pnpm check:ci and pnpm test both pass (plus cargo fmt / clippy / test for Rust changes)
  • I ran the affected surface and observed the change working
  • If a wire message changed: WIRE_PROTOCOL_VERSION is bumped
  • New code and assets are my own work, or their origin and license compatibility are noted above
  • Docs and comments are updated where behavior changed

Copilot AI lite review requested due to automatic review settings August 9, 2026 14:27
@linear-code

linear-code Bot commented Aug 9, 2026

Copy link
Copy Markdown

CODE-576

Copilot AI 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.

Pull request overview

This PR improves Codex rollout history replay in @linkcode/host/agent-adapter, especially for codex 0.144.x code-mode sessions, so reseeding a transcript from stored rollout history preserves MCP tool cards, properly unwraps script output envelopes, filters machine-injected “user” rows, and prevents oversized pages from exceeding transport limits.

Changes:

  • Replay nested code-mode MCP calls from event_msg mcp_tool_call_end rows and dedupe against response-backed tool rows.
  • Unwrap code-mode Script ... / Wall time ... / Output: envelopes (and treat failed/terminated/verification-failure receipts as failures).
  • Extend history paging to account for tool-call rawOutput payload size (with tests covering paging and new replay behaviors).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/host/agent-adapter/src/native/codex/history.ts Replays mcp_tool_call_end rows into tool-call events; filters additional synthetic user markers.
packages/host/agent-adapter/src/native/codex/history-tools.ts Adds MCP end-row mapping and script-envelope parsing updates.
packages/host/agent-adapter/src/history-util.ts Updates paging budget logic to include tool-call raw outputs.
packages/host/agent-adapter/src/tests/history-util.test.ts Adds coverage ensuring tool raw outputs contribute to page budgeting.
packages/host/agent-adapter/src/tests/codex-history.test.ts Adds unit tests for MCP end replay, deduping, oversized results, script envelope unwrapping, and synthetic row filtering.
packages/host/agent-adapter/AGENTS.md Updates adapter behavior documentation for new history replay and paging behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +43 to +47
function eventPayloadLength(event: AgentHistoryEvent): number {
if (event.event.type === 'tool-call') {
const raw = event.event.toolCall.rawOutput;
return raw === undefined ? 0 : JSON.stringify(raw).length;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in f7fa4f4 — the page budget now measures Buffer.byteLength(JSON.stringify(...), "utf8"), and (per the sibling thread) covers the whole serialized tool call, not just rawOutput.

Comment on lines +186 to +189
rawOutput:
raw !== undefined && JSON.stringify(raw).length <= MCP_RESULT_MAX_JSON_LENGTH
? raw
: undefined,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in f7fa4f4 — the cap is now Buffer.byteLength(JSON.stringify(raw), "utf8") <= MCP_RESULT_MAX_JSON_BYTES, with a regression test whose CJK result is under the cap in UTF-16 code units but over it in UTF-8 bytes.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important

Two of the three replay fixes land correctly, but the MCP dedup discards the structured failure status that only the mcp_tool_call_end row carries — the PR's own new test encodes a failed call replaying as completed. Separately, the Script failed branch is dead against the pinned 0.144.6 binary, while the Script error: form that binary does emit is left unwrapped.

Reviewed changes

  • native/codex/history.ts — new collectRespondedToolCallIds + event_msg / mcp_tool_call_end replay branch, three new SYNTHETIC_USER_MARKERS.
  • native/codex/history-tools.ts — new codexMcpEndToolCall with a 256 KiB rawOutput cap, new SCRIPT_ENVELOPE_RE unwrapping in parseCodexToolOutput.
  • history-util.tseventAttachmentLengtheventPayloadLength, now counting tool rawOutput toward the page budget.
  • Tests — 6 new codex-history cases, 1 new history-util case. I verified each new test genuinely fails without its corresponding fix (not test theatre), and the suite passes locally (2 files / 48 tests).
  • AGENTS.md — History bullet rewritten.

Confirmed correct: the call_id ⇒ live-item-id convergence claim in AGENTS.md checks out against codex-rs at tag rust-v0.144.6core/src/mcp_tool_call.rs builds McpToolCallItem { id: call_id.to_string() } and app-server-protocol/src/protocol/thread_history.rs uses id: payload.call_id.clone(). Replayed and live ids will match, so the transcript seed's uptoSeq cut will not duplicate cards.

Scope note (no line to anchor to): this is stacked on #434 (ruocheng/code-575); I reviewed only the delta against that base, so #434's own changes are out of scope here.

ℹ️ Nitpicks

  • collectRespondedToolCallIds keys on both CODEX_TOOL_ANNOUNCE_TYPES and CODEX_TOOL_OUTPUT_TYPES. The stated rationale is "a response row already produced this card", but only an output row settles one. Scoping the set to output types only is strictly smaller, matches the rationale, and lets an announce-only (truncated / interrupted) call still settle from its event_msg end row.
  • Every new Wall time test fixture is colon-less (Wall time 0.3 seconds). The pinned binary's string table has Wall time: with a colon, clustered with Exit code: / Chunk ID: next to core/src/tools/code_mode/execute_handler.rs. [^\n]* tolerates both so behaviour is fine, but the fixtures don't match what the binary emits.
  • The 256 KiB MCP_RESULT_MAX_JSON_LENGTH elision drops rawOutput to undefined silently — from the user's side an oversized MCP result is indistinguishable from one that returned nothing. A one-line placeholder would read better.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/host/agent-adapter/src/native/codex/history.ts Outdated
Comment thread packages/host/agent-adapter/src/history-util.ts Outdated
Comment thread packages/host/agent-adapter/src/native/codex/history-tools.ts
Copilot AI review requested due to automatic review settings August 9, 2026 15:09

Copilot AI 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.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/host/agent-adapter/src/history-util.ts:48

  • sliceHistoryEventPage() now measures tool-call payload size by JSON.stringify()ing the entire ToolCall snapshot for every event. For large transcripts (especially tool calls with big text/diff output), this creates a large intermediate JSON string just to count bytes, which can be very costly in CPU and memory during paging.

A cheaper (and still byte-accurate for the large/unbounded fields) approach is to size the specific unbounded fields (rawInput/rawOutput, and text/diff content) without serializing the full object.

function eventPayloadLength(event: AgentHistoryEvent): number {
  if (event.event.type === 'tool-call') {
    return Buffer.byteLength(JSON.stringify(event.event.toolCall), 'utf8');
  }

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important

The delta itself is clean — both the page-budget and the MCP-status findings are fixed correctly, and I've resolved those threads. I'm not approving only because the third finding from the prior review is untouched and unanswered: Script failed is still dead against the pinned 0.144.6 binary while Script error: — the form that binary does emit — is still neither unwrapped nor failed.

Reviewed changesf7fa4f44 only, the follow-up commit answering review 4891643854.

  • Carried the MCP end row's failure verdict onto the response settlemcpEndFailures records Err/isError for response-backed calls and overrides the settle's output-text heuristic, so a failed direct MCP call no longer replays green.
  • Widened the page budget to the whole serialized tool calleventPayloadLength now measures Buffer.byteLength(JSON.stringify(toolCall), 'utf8'), so content-borne exec output and apply_patch diffs count, and the docstring no longer claims per-result caps bound every adapter.
  • Moved the 256 KiB MCP result cap onto UTF-8 bytesMCP_RESULT_MAX_JSON_LENGTHMCP_RESULT_MAX_JSON_BYTES, closing the UTF-16 undercount.
  • Extracted codexMcpEndFailed — the failure discriminator is now shared between codexMcpEndToolCall and the dedup path instead of being computed and thrown away.
  • Added three regression tests — a CJK result under the cap in code units but over it in bytes, a response-backed failed call now asserting failed, and an exec row whose body rides content with rawOutput: 0.

Verification I did rather than assumed. I enumerated the row orderings the reconciliation can see — end→announce→output, announce→end→output, announce→output→end, duplicate end rows, and a plan-call id collision — and every one lands on the right final status. The late-end-row branch at history.ts:562 is genuinely reachable, not dead: recordToolEvent writes every emitted snapshot back into announced, so announced.get(id) is the settled card by then. I also checked the budget units, since the measure is in UTF-8 bytes but the constant is named for base64 length: MAX_ATTACHMENT_TOTAL_BASE64_LENGTH is 16,777,216 ASCII base64 characters, the tunnel frames raw UTF-8 with no second encoding, and the assembler evicts on aggregate pending bytes rather than per-message size — so the two branches are commensurable and the budget errs conservative. Both history test files pass 49/49, and each new test fails without its fix.

ℹ️ Nitpicks

  • On the dedup path, mcpEndFailures.add also fires for a call that has an announce row but no output row (a truncated or interrupted rollout). Nothing consumes the set in that case, so a call the end row says failed replays stuck at in_progress. Scoping collectRespondedToolCallIds to CODEX_TOOL_OUTPUT_TYPES only — the standing nit from the prior review — would let that call settle from its end row instead.
  • AGENTS.md:87 still describes the page budget as "aggregate embedded-attachment payload … so image-heavy transcripts fan across cursor pages". After this commit whole tool payloads count too, and the motivating case is result-heavy transcripts.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

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.

2 participants