fix(sdk): handle cyclic run-state on session resume - #1341
Conversation
applyOverridesToSessionState cloned the state with a JSON round-trip, which throws on cyclic state. A resumed session carries the previous turn's tool blocks, and MCP tools are stored as live zod schemas -- self-referential by construction -- so the resume died with "JSON.stringify cannot serialize cyclic structures". Fall back to a deep clone that keeps zod schemas as they are: lodash copies own enumerable properties only, and zod keeps its internals on a non-enumerable `_zod`, so a plain cloneDeep returns a schema-shaped object that throws on first use.
87f0d91 to
5e01a4a
Compare
|
Good bug report and good fix. The root cause is precise — MCP tool schemas carry non-enumerable The three added tests are well-targeted: cyclic plain object, cyclic object holding a live zod schema (verifying One thing worth double-checking before porting: the bare The shared-reference-not-cloned choice for zod schemas is reasonable since they're immutable, but flagging it explicitly in a code comment near Overall: correct diagnosis, minimal targeted fix, in-scope files, solid tests. Good candidate for porting. |
Resubmission of #945, auto-closed when the repo history was rewritten — per @victorxheng, "not a judgment on this PR". Its base commit no longer exists, so this is re-applied on current
mainrather than rebased — the re-application itself brings no other change. The review feedback is addressed below.Fixes a crash surfaced while testing #944 (the /undo and /redo feature): on the second interaction of a resumed session, the CLI died with
The trigger (added after reproducing this in the product)
The state goes cyclic when an MCP server is configured. MCP tools live in the run state as live zod schemas —
mcp.tsconverts each server tool withconvertJsonSchemaToZod— and a zod schema is self-referential: its_zodinternals point back at the schema itself.JSON.stringifyon that state therefore throws.So the repro is one config file and two messages:
Put any server in
.agents/mcp.json:{ "mcpServers": { "exa": { "type": "http", "url": "https://mcp.exa.ai/mcp" } } }Start the CLI in a project and send a message. The turn that clones the run state dies with
JSON.stringify cannot serialize cyclic structures.Verified with a locally built binary, MCP server configured:
JSON.stringify cannot serialize cyclic structuresWith both patches missing, the error that shows up first is the tool-set one —
undefined is not an object (evaluating 'H._zod.parent'), sent separately as #1342 — which is how I know the two are independent. Fixing that one is what surfaced this one.Root cause
applyOverridesToSessionStateinsdk/src/run-state.tsdeep-clones the session state with a JSON round-trip (JSON.parse(JSON.stringify(...))). The run state can carry cyclic values — a recursive zodlazyschema inside a tool block is self-referential — andJSON.stringifythrows on cycles. So resuming a session whose run state holds such a schema crashes the resume path.It is the second interaction that dies once an MCP server has supplied one, which is why it reads as intermittent — see the trigger section above for the shortest repro.
Fix
Try the JSON path first; if it throws, fall back to a deep clone that handles cycles. This mirrors what
cloneSessionStatealready does in the same file — no new deps, lodash is already imported.The fallback must keep zod schemas intact
The obvious fallback, a plain
cloneDeep, would hand back a corpse, and that is not a hypothetical: the values that make this state cyclic are the zod schemas themselves.zod installs its internals on a non-enumerable
_zod. lodash copies own enumerable properties only, so it drops_zod— but the clone keeps the prototype, sosafeParsesurvives and the result still answersObjectand looks like a schema. It only blows up on the first zod call:Reproduced directly:
_zodon the original_zodon a plaincloneDeepcopysafeParseon the copyschema.descriptionon the copy_zod.parent, same as #1342So the fallback goes through
cloneDeepPreservingZodSchemas, which iscloneDeepWithplus one rule: a zod schema is passed through by reference instead of copied. Schemas are immutable, so there is nothing to copy — and it keeps the hazard from coming back through the same door if anything else in the state ever holds a schema.Response to the review
1. The bare
catch {}The review asked to check — "or at least comment" — whether other
JSON.stringifyfailure modes (e.g. BigInt,undefined) could silently fall into thecloneDeeppath with different semantics than intended.Both were done. The answer is no, and it is narrower than it looks:
JSON.stringifyBigIntundefined, function, symbolDate,URLOnly a cycle or a BigInt throws, so anything JSON merely drops or coerces never reaches the fallback. The catch stays unnarrowed on purpose and the comment records why — including that the fallback only runs where this function used to throw, so it cannot change the semantics of anything that used to work. The BigInt case has a test.
2. No test was added
Added three regression tests in
sdk/src/__tests__/run-state-git-changes.test.ts:clones a resumed state whose run state carries a cycle— it does not throw, the override still applies, and the copy is independent with the cycle intact.falls back to a deep copy for values JSON.stringify rejects (BigInt)— pins the second throw trigger above.keeps a live zod schema intact when the fallback clone runs— the schema still has_zodand still converts withz.toJSONSchema. It fails onmain, and it would fail again if the schema rule were dropped from the fallback.All fail on
mainand pass with the patch.Verification
Public CI builds the SDK and smoke-tests the binary, but it does not run the tests, so this was reproduced locally:
bun run --cwd sdk typecheck— cleanbun run --cwd sdk test— 618 pass, 1 failThe failure is pre-existing and unrelated:
sponsored rooted filesystem > a failed directory guard cannot continue in the wrong ancestormatches the Englishmkdir: ... File existsmessage, which GNU coreutils translates, so it fails on non-English machines and passes on the English CI runners. Untouched here, reported separately in #1340.Scope
Two files, +128/-5, no behaviour change on the normal path.
prettierreports a pre-existing union-type diff atrun-state.ts:874— byte-identical onmain, and no workflow runs prettier, so it is left out.