perf(demo-agent): stop paying for JSON punctuation and a redundant tool - #146
Conversation
One measured product-demo run spent $5.36 on 1,013,992 input tokens against 11,776 output — 95% of the bill is what we send, not what we get back. Two things dominate it. The accessibility snapshot was `JSON.stringify` of the full tree, ~707k of those tokens. The waste is structural: JSON repeats "role"/"name"/"children" on every node and wraps two short strings in braces and quotes. Serialize the same tree as indented `role "name" prop=value` lines instead — 44% fewer characters on an app-shaped tree (11,438 -> 6,399), with every scalar property preserved, so this is a re-encoding rather than a filter. A 3,000-node budget cuts at a node boundary and states the remainder, where a `slice` on the old JSON produced an unparseable string. The `screenshot` tool spent a whole round-trip per story choosing a key frame that play.ts's unconditional final capture already produces. Delete it; the `ModelAction` variant stays as an inert path so the capability is one tool definition away. `rationale` leaves nav/key/wait — it is write-only, and on those three the argument already is the intent — and stays on click and type, the actions that actually go wrong. Consumers whose story prose says "capture a screenshot" must drop that instruction in the same rollout: an unrecognised tool name maps to done/failed and sinks the chapter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fy9fwu7GQnyUkiGacPmDSB
There was a problem hiding this comment.
AI code review — 💬 Comment
Risk tier: full · 0 critical · 1 warnings · 0 suggestions
Reviewers: security
1. ⚠️ Warning — Snapshot budget does not bound traversal work
📍 packages/demo-agent/src/cdp.ts:67-68
When the 3,000-node budget is exhausted, 'countNodes' recursively walks the entire omitted subtree to compute 'dropped'. A very large or deeply nested accessibility tree can therefore still incur unbounded traversal time or a call-stack overflow, undermining the protection this budget is intended to provide. Use an iterative counter or maintain subtree counts while walking, with a safe cap for remainder reporting.
…t unforgeable The line encoding still carried `backendNodeId` and `loaderId` — puppeteer's `serialize()` sets both on EVERY node, so each line spent ~45 chars on ids no model reads (~135KB on a 3,000-node page), which is the repetition this encoding exists to delete. Page content is attacker-influenceable and the newline is structural here. `JSON.stringify` escapes neither U+2028, U+2029 nor U+0085, so a crafted accessible name could forge a node line — or a line byte-identical to the truncation marker. Escaping now covers those three code points on the name, on every property value, and on the role; the marker sits at column 0 behind a lone-backslash prefix that no rendered role or name can produce (both double their backslashes). Also: `checked`/`pressed` keep their `false` — puppeteer emits those two only when the node has that state, so dropping it made an unchecked checkbox identical to one with no checked state. A 280-char cap bounds an accessible name, the per-node cost the NODE budget cannot see (~200 paragraph-length StaticText names outspend 3,000 buttons without tripping the marker). The removed `screenshot` tool was fatal, not inert: `toolCallToAction`'s `Match.orElse` mapped it to done/failed, so consumer prose that still says "capture a screenshot" would sink the chapter. The legacy name now maps to a 0ms wait; unknown tools still fail loudly. With nothing able to produce it, the `screenshot` variant leaves `ModelAction` and play.ts — play.ts's unconditional final capture is unchanged and is now the only key-frame source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fy9fwu7GQnyUkiGacPmDSB
|
Review team ran four lenses over the diff — correctness, tests, security, architecture. Nine findings, all addressed in 113b49c. Four of them are corrections to claims the original commit asserted confidently, two of those in comments describing properties the code did not have. Two blockers.
Corrections. Puppeteer treats Also: dropped the 89 tests pass (20 in |
The budget stopped what gets rendered but not the work: on exhaustion, countNodes recursed over the entire omitted subtree to produce a number whose only job is telling the model "there is more". Unbounded traversal, and a stack proportional to tree depth, to compute a figure nobody reads precisely. Count iteratively against a 50,000-node ceiling on the TOTAL remainder, not per omitted sibling — a page with many omitted top-level nodes would otherwise pay the ceiling once per sibling. Past it the marker says "at least", rather than stating a saturated ceiling as if it were the true count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fy9fwu7GQnyUkiGacPmDSB
A
product-demorun measured at $5.36 of model spend — 1,013,992 input tokens against 11,776 output across 110 calls. 95% of the bill is what the agent sends, not what it gets back, and the input is flat across the run rather than growing, which points at a large payload re-sent per action rather than an accumulating history.Problem & Insight
Two things dominate that payload.
The accessibility snapshot is
JSON.stringifyof the whole tree, uncapped — roughly 707k of the 1.01M input tokens, 7,069 per call, with one call at ~76,300. The waste is structural, not semantic: JSON repeats the keys"role","name"and"children"on every one of thousands of nodes, and spends braces and quotes on nodes whose entire content is two short strings. Puppeteer'sinterestingOnlyfilter has already dropped the uninteresting nodes, so there is little left to prune — the encoding is the cost.The
screenshottool spends a full model round-trip per story to nominate a key frame, andplay.tsalready takes an unconditional final screenshot as a fallback. In 8 of the 10 chapters of the demo that produced these numbers, the prose reaches for it immediately beforedone, so the nominated frame and the fallback frame are the same picture.Take
Serialize the tree as indented
role "name" prop=valuelines. Measured on an app-shaped tree — nav, a 40-row table, a form — 11,438 characters of JSON become 6,399, a 44% cut. Every scalar property survives, so a node'svalue,disabled,checkedandexpandedstill reach the model, which the play prompt explicitly asks it to reason about; this is a re-encoding, not a filter. A 3,000-node budget bounds pathological pages, and it cuts at a node boundary with the remainder stated, where asliceon the old JSON produced an unparseable string that would sink whichever chapter hit it.Delete the
screenshottool.ModelAction'sscreenshotvariant and play.ts's handling stay as an inert path — nothing emits one now, they cost no tokens, and the capability is one tool definition away.rationaleleavesnav/key/wait, where the argument already is the intent, and stays onclickandtype, the two actions that actually go wrong.Key actions
toolCallToAction'sMatch.orElsemaps an unrecognised tool name to{type:"done", status:"failed"}, so prose that still asks for a deleted tool fails the chapter on the spot.81 tests pass,
tsc --noEmitclean. Nine new cases cover the serializer: property preservation,false-boolean dropping, quote-and-newline escaping so a crafted accessible name cannot forge a node line, node-boundary truncation with a descendant-aware remainder, and the size comparison itself.Not in this PR: routing
anthropic/models to the gateway's native endpoint so the stable prefix can carrycache_control. Prompt caching is unavailable on the OpenAI-compat path this adapter uses today, and Anthropic silently ignores the field rather than erroring, so it needs its own branch and its own review. A probe against the native endpoint confirms the gateway passescache_controlthrough verbatim and reportscache_creation_input_tokensthencache_read_input_tokenson a repeat call, so the premise holds.