Conversation
…s, not through an emoji The page snapshot keeps the first 200 UTF-16 code units of each control's accessible name and value. When the limit landed between the halves of a surrogate pair, the element handed to the Bot ended on a lone high surrogate: JSON carries it as a bare `\ud83d` and UTF-8 encodes it as U+FFFD, so the Bot read a broken character that was not on the page, often at the end of text it had just typed into a box. The cut now stops one code unit short in that case, the rule the server's `cutAtCodeUnits` applies to tool results and relayed answers. The computer shares no code with the server, so the few lines are repeated beside the parser. Text that fits, and a cut that lands between characters, are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Hotragn
left a comment
There was a problem hiding this comment.
I wrote the server-side half of this in #525, so I checked the two things I would want checked if it were mine: whether the rule really is the same rule, and whether the site you left out has to be left out.
The rule is the same. cutAtCodeUnits here is character-for-character server/src/channels/text.ts:25-29, so there is no second dialect of "where it is safe to cut" to keep in sync later. The duplication argument holds too — I looked for a seam and there isn't one: agent-computer/package.json is @openbot/agent-computer, nothing under agent-computer/src reaches into server, and there is no shared package to put this in. Four lines beside the parser is the right call over inventing one.
I ran it: bun test tests/aria-snapshot.test.ts gives 30 pass, 0 fail, and the two new tests fail on main the way you describe.
The third site is reachable, and it is three lines
You set aside index.ts:290 on the grounds that the module imports Playwright at load so there is no test you could run. That is true of readablePageText, but not of the fix — index.ts:3 already imports from ./aria-snapshot:
import { parseAriaSnapshot, type SnapshotElement } from "./aria-snapshot";So the helper is already on the Playwright-free side of the boundary. Export it, add it to that import, and swap the call:
-function cutAtCodeUnits(text: string, limit: number): string {
+export function cutAtCodeUnits(text: string, limit: number): string {
-import { parseAriaSnapshot, type SnapshotElement } from "./aria-snapshot";
+import { cutAtCodeUnits, parseAriaSnapshot, type SnapshotElement } from "./aria-snapshot";
- text: collapsed.slice(0, TEXT_EXTRACT_LIMIT),
+ text: cutAtCodeUnits(collapsed, TEXT_EXTRACT_LIMIT),I applied exactly that on top of your branch: bunx tsc --noEmit in agent-computer is clean and the suite is still 30 pass, 0 fail. The helper stays testable where it already is, so the untestable module never needs a test — which I think was the real obstacle, not the coverage.
truncated: collapsed.length > TEXT_EXTRACT_LIMIT is unaffected, since dropping one more unit cannot make an over-limit string not over-limit.
Worth doing in this PR rather than a follow-up, for the reason your own changelog entry gives: the entry says "the computer's page snapshot", and a reader will take that to mean the page text too. The 6000-unit extract is also the more likely one to land mid-emoji in practice — 6000 units of page text passes through far more emoji than a 200-unit control name does.
One small thing
cutAtCodeUnits("") takes charCodeAt(-1) → NaN, and NaN >= 0xd800 is false, so it returns "". Correct, but only by way of a comparison against NaN, and the same accident is now in two files. Not worth a guard clause; worth half a sentence in the doc comment that empty is handled, so nobody later "fixes" it into a length check.
Nothing else. The diagnosis is right, the measurement in the description is the part I would keep, and a character that ends at the cut is kept is the test that stops the guard from costing a whole emoji every time — which is the failure the obvious version of this fix has.
What this changes
toElementinagent-computer/src/aria-snapshot.tskeeps the first 200 characters of each control's accessible name and value withslice(0, 200), which counts UTF-16 code units. When the limit lands between the two halves of a surrogate pair (any emoji, any astral-plane glyph), the element ends on a lone high surrogate. Measured onmain:That element is what
POST /snapshotreturns and what the gateway hands to the Bot, so the Bot reads a broken character that is not on the page. Thevalueside is the likely one to hit: a Bot types a long message with an emoji into a text box, takes a snapshot to check its work, and reads back text that ends in U+FFFD.The fix is the rule the server's
cutAtCodeUnitsalready applies to tool results and relayed answers (#525): stop one code unit short when the cut would split a character. The computer is a separate deployable that shares no code with the server (the module says so aboutSnapshotElement), so those few lines are repeated beside the parser instead of imported. Text that fits, and a cut that lands between characters, are unchanged.Not changed: the page text extract in
agent-computer/src/index.ts(collapsed.slice(0, TEXT_EXTRACT_LIMIT)) has the same cut, but that module imports Playwright at load, so there is no test I could run for it here.Where it runs
Boundary and audit
Changelog
CHANGELOG.mdunderUnreleased. It sits at the top of that section like every other entry, so if another PR lands there first I'm happy to rebase.Proof
Three tests in
agent-computer/tests/aria-snapshot.test.ts: a name cut through an emoji, a value cut through an emoji, and an emoji that ends exactly at the limit, so the guard cannot cost a whole character.Before the fix:
After:
The whole
agent-computersuite on Windows: 285 pass, 24 fail. Every failure is inshell.test.ts(spawns/bin/bash),workspace.test.ts(creates symlinks) oridentity.test.ts(Unix sockets), none of which import the snapshot parser.🤖 Generated with Claude Code