Skip to content

fix: resolve cyclic structure serialization crash in token counter - #839

Open
OmarAlaaeldein wants to merge 1 commit into
CodebuffAI:mainfrom
OmarAlaaeldein:fix/zod-cyclic-structure-crash
Open

fix: resolve cyclic structure serialization crash in token counter#839
OmarAlaaeldein wants to merge 1 commit into
CodebuffAI:mainfrom
OmarAlaaeldein:fix/zod-cyclic-structure-crash

Conversation

@OmarAlaaeldein

Copy link
Copy Markdown

This PR fixes a TypeError crash: JSON.stringify cannot serialize cyclic structures when the token count API fails and falls back to local estimation.

  • Implement safeJsonStringify in token-counter.ts to handle circular references.
  • Sanitize the inputSchema inside toolDefinitions at creation time using JSON.parse(safeJsonStringify(schema)) so it is a plain, serializable object.
  • This also prevents circular structure errors when writing the chat run state to disk (run-state.json).

@codebuff-team

Copy link
Copy Markdown
Contributor

Good catch on the root cause — a recursive/self-referential inputSchema (e.g. from a z.lazy tool schema) blowing up JSON.stringify and crashing the token estimator fallback. safeJsonStringify in token-counter.ts is a reasonable, minimal circular-ref guard, and sanitizing inputSchema once at toolDefinitions creation time (run-agent-step.ts:863) rather than on every stringify call is the right place to put the fix.

Two things worth tightening before this could be ported:

  1. countTokensJson now special-cases typeof text === 'string' and returns countTokens(text) directly instead of countTokens(JSON.stringify(text)). That's a real behavior change beyond the crash fix — previously a string input got wrapped in quotes before counting, now it doesn't. It's probably an improvement, but it's not mentioned in the PR description and isn't covered by a test, so it's easy to overlook as scope creep when reviewing the diff.
  2. No tests were added for either safeJsonStringify or the new countTokensJson branch. Given this is fixing a crash, a regression test with an object containing a circular reference (and one with a function property) would make the fix much easier to trust and to port — right now a reviewer has to reason about WeakSet correctness by hand.

Also worth double-checking: converting functions to value.toString() inside safeJsonStringify is surprising for a function named for JSON-safety — silently embedding source code into a schema that gets round-tripped through JSON.parse could produce confusing output for tool definitions that (incorrectly) contain functions. Might be safer to drop functions entirely (return undefined) unless there's a concrete reason to keep them as strings.

Core idea is worth porting; please add a test and split out (or justify) the string-input behavior change.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written stale No activity after a maintainer request; queued for closing labels Aug 20, 2026
@codebuff-team

Copy link
Copy Markdown
Contributor

Marking this stale - there has been no activity here for 50 days. It will close in 7 days unless someone comments.

This is backlog upkeep, not a verdict on the pull request. A single comment keeps it open, and anything closed this way can be reopened.

When the token count API call fails or is not available, the CLI fallback logic
serializes  using . However,
contains raw Zod schema objects (), which have circular references,
causing a  TypeError.

Changes:
- Added a  utility to strip circular references from objects.
- Sanitize the  inside  at creation time using
   so it is a plain, serializable object.
  This also prevents similar serialization failures when saving run state to disk.
@OmarAlaaeldein
OmarAlaaeldein force-pushed the fix/zod-cyclic-structure-crash branch from 85753fa to 7adbf86 Compare August 23, 2026 20:09
@OmarAlaaeldein

Copy link
Copy Markdown
Author

Rebased onto current main and tightened the serialization fix. countTokensJson keeps its existing JSON-string serialization behavior; safeJsonStringify handles circular values, drops function properties using JSON semantics, and is used when persisting tool schemas. Added regression coverage for circular references, function properties, and string serialization. Verified with: bun test packages/agent-runtime/src/util/tests/token-counter.test.ts (9 passing).

@codebuff-team codebuff-team removed the stale No activity after a maintainer request; queued for closing label Aug 31, 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:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants