Skip to content

fix(sdk): handle cyclic run-state on session resume - #1341

Open
KazenDev wants to merge 1 commit into
CodebuffAI:mainfrom
KazenDev:fix/cyclic-run-state-resume
Open

fix(sdk): handle cyclic run-state on session resume#1341
KazenDev wants to merge 1 commit into
CodebuffAI:mainfrom
KazenDev:fix/cyclic-run-state-resume

Conversation

@KazenDev

@KazenDev KazenDev commented Sep 12, 2026

Copy link
Copy Markdown

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 main rather 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

Error: JSON.stringify cannot serialize cyclic structures.

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 schemasmcp.ts converts each server tool with convertJsonSchemaToZod — and a zod schema is self-referential: its _zod internals point back at the schema itself. JSON.stringify on that state therefore throws.

So the repro is one config file and two messages:

  1. Put any server in .agents/mcp.json:

    { "mcpServers": { "exa": { "type": "http", "url": "https://mcp.exa.ai/mcp" } } }
  2. 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:

build run
this patch missing (tool-set copy already fixed, see #1342) dies with JSON.stringify cannot serialize cyclic structures
this patch in completes; turns go through

With 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

applyOverridesToSessionState in sdk/src/run-state.ts deep-clones the session state with a JSON round-trip (JSON.parse(JSON.stringify(...))). The run state can carry cyclic values — a recursive zod lazy schema inside a tool block is self-referential — and JSON.stringify throws 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 cloneSessionState already 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, so safeParse survives and the result still answers Object and looks like a schema. It only blows up on the first zod call:

TypeError: undefined is not an object (evaluating 'schema._zod.parent')

Reproduced directly:

_zod on the original present
_zod on a plain cloneDeep copy absent
safeParse on the copy present — the trap
schema.description on the copy throws _zod.parent, same as #1342

So the fallback goes through cloneDeepPreservingZodSchemas, which is cloneDeepWith plus 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.stringify failure modes (e.g. BigInt, undefined) could silently fall into the cloneDeep path with different semantics than intended.

Both were done. The answer is no, and it is narrower than it looks:

Value JSON.stringify Path taken
cycle throws falls back to the deep clone
BigInt throws falls back to the deep clone
undefined, function, symbol no throw, key dropped JSON path, unchanged
Date, URL no throw, coerced JSON path, unchanged

Only 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 _zod and still converts with z.toJSONSchema. It fails on main, and it would fail again if the schema rule were dropped from the fallback.

All fail on main and 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 — clean
  • bun run --cwd sdk test — 618 pass, 1 fail

The failure is pre-existing and unrelated: sponsored rooted filesystem > a failed directory guard cannot continue in the wrong ancestor matches the English mkdir: ... File exists message, 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. prettier reports a pre-existing union-type diff at run-state.ts:874 — byte-identical on main, and no workflow runs prettier, so it is left out.

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.
@KazenDev
KazenDev force-pushed the fix/cyclic-run-state-resume branch from 87f0d91 to 5e01a4a Compare September 12, 2026 22:56
@codebuff-team

Copy link
Copy Markdown
Contributor

Good bug report and good fix. The root cause is precise — MCP tool schemas carry non-enumerable _zod internals and can be cyclic via lazy, so JSON.parse(JSON.stringify(...)) in applyOverridesToSessionState (sdk/src/run-state.ts) throws on resume. The fallback via cloneDeepPreservingZodSchemas correctly special-cases zod schemas (checked via safeParse + _zod) rather than doing a naive cloneDeep, which would silently corrupt them — you called this out explicitly and it matters.

The three added tests are well-targeted: cyclic plain object, cyclic object holding a live zod schema (verifying _zod survives and toJSONSchema still works), and a BigInt case showing the fallback isn't cycle-specific. That's the kind of coverage this change needs given how easy it'd be to regress silently.

One thing worth double-checking before porting: the bare catch {} swallows any JSON.stringify failure, not just cycles/BigInt — your comment argues this is fine because other odd values (functions, symbols, Date, URL) don't throw and so never reach the fallback, but it'd be worth a maintainer confirming there's no other pathological input (e.g. very deep nesting, some non-cyclic throw) that would silently take the slow path unexpectedly. Not a blocker, just worth a second pair of eyes since the catch is unnarrowed.

The shared-reference-not-cloned choice for zod schemas is reasonable since they're immutable, but flagging it explicitly in a code comment near cloneDeepPreservingZodSchemas (not just the PR description) would help future readers who don't have this context.

Overall: correct diagnosis, minimal targeted fix, in-scope files, solid tests. Good candidate for porting.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants