fix(runtime): avoid implicit computer-use images - #3585
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head 99c27eb2be4563eefa6ccbe7d1d369d9488d2bcb.
Coverage: what decides whether a screenshot reaches the model, which paths this change moves from implicit-attach to no-attach, and whether anything that should still get a frame loses one. Not covered: wording.
The intent is right and the observable behaviour matches it: the decision is per tool call, not per turn or session; observe: true, screenshot, zoom, and the legacy coordinate actions still carry an image, while other semantic actions keep a fresh accessibility observation without the frame. Two problems in how that decision is made.
[P1] The image gate reads mutable caller input, not the frozen call it executed
packages/runtime/src/computer-use-tools.ts:2818-2832 — toModelOutput: ({ input, output }) calls shouldSendScreenshotToModel(input), and that input is the original caller-held object at settlement time, not the immutable snapshot the implementation actually ran with.
Reproduced: begin an async settlement with observe / include_screenshot: false; after impl has started, mutate the original object to true; the returned result carries the PiP screenshot and the provider modelOutput then contains a file block. The reverse mutation swallows an image the caller explicitly asked for. The PiP frame a semantic action carries can be reclassified the same way before settlement — which walks straight through the boundary this PR exists to establish.
Cloning inside the callback would already be too late. The fix is for toModelOutput to use the same verified immutable call, or for impl to emit an explicit model-image decision derived from the frozen input.
[P2] The action catalogue is untyped and parallel to the canonical union
computer-use-tools.ts:345-367 introduces MODEL_SCREENSHOT_ACTIONS as a ReadonlySet<string> sitting alongside CU_ACTION_TYPES / CU_TOOL_ACTION_TYPES, and the tests sample only five inputs.
The consequence is a silent default: a legitimately added action will simply not send an image, while every existing schema and parity test still passes. That is precisely the drift the comments near core warn about. Deriving the set exhaustively from the canonical action union — a typed map or derived set — and iterating every action in a classification test would make a new action a compile-time or test-time decision instead of a silent one.
Verification and limits
Built clean in dependency order (core → storage → mcp → runtime). Computer Use suites 261/261; Biome and git diff --check pass. Across the whole Runtime: 3064 pass, 1 fail, 13 skipped — the single failure is a macOS Bash executable-root environment assertion in untouched code, reproducible standalone, and hosted CI is green on this exact head.
Astro-Han
left a comment
There was a problem hiding this comment.
Incremental closure check at exact head 99c27eb2be4563eefa6ccbe7d1d369d9488d2bcb: the head has not changed since review 5002157364, so both previously reported blockers remain. Reposting them inline at their exact diff authorities; no APPROVE on this head.
| value: [ | ||
| { type: 'text', text }, | ||
| ...(o.screenshot | ||
| ...(o.screenshot && shouldSendScreenshotToModel(input) |
There was a problem hiding this comment.
[P1] Gate the image using the same immutable invocation that impl executed, not this settlement-time caller object. input here is still the caller-held reference: mutating include_screenshot or action after impl starts can add a provider file block to a call that executed with images disabled, or remove one from an explicitly visual call. Cloning here is already too late; derive an explicit model-image decision from the frozen input inside execution (or pass the verified immutable call through) and consume that decision here.
| screenshot?: { base64: string; mimeType: string }; | ||
| } | ||
|
|
||
| const MODEL_SCREENSHOT_ACTIONS: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
[P2] Make this classification exhaustive against the canonical Computer Use action union. A ReadonlySet<string> is a parallel untyped catalogue, so adding a legitimate action silently defaults it to no model image while schemas and sampled tests still pass. Use a typed exhaustive record/derived set and iterate every action in the classification test so each new action requires an explicit compile-time or test-time choice.
Summary
element_sequenceRoot cause
Mutating Computer Use actions attach a final screenshot for the desktop PiP.
toModelOutputpreviously projected every attached screenshot into the next provider request, so a semanticelement_sequenceresult carried an unnecessary base64 PNG even though it already returned a fresh accessibility observation.Measured result
A real Calculator run using
element_sequencecompleted successfully. The final provider request dropped from 156,262 bytes to 73,727 bytes, contained no base64 string over 10 KB, and reduced the post-action input-token increase from 1,575 to 1,056 tokens.Verification
npm --workspace @maka/runtime test(3,089 tests passed; 13 skipped)npx biome check packages/runtime/src/computer-use-tools.ts packages/runtime/src/__tests__/computer-use-tools.test.tsgit diff --check origin/main...HEADelement_sequence