Skip to content

Cut a long control name or value in a page snapshot between characters, not through an emoji - #539

Open
kevin9327 wants to merge 1 commit into
CopilotKit:mainfrom
kevin9327:fix/snapshot-cut-between-characters
Open

kevin9327 wants to merge 1 commit into
CopilotKit:mainfrom
kevin9327:fix/snapshot-cut-between-characters

Conversation

@kevin9327

Copy link
Copy Markdown
Contributor

What this changes

toElement in agent-computer/src/aria-snapshot.ts keeps the first 200 characters of each control's accessible name and value with slice(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 on main:

last unit: d83d  JSON tail: aaa\ud83d"  UTF-8 tail: efbfbd

That element is what POST /snapshot returns and what the gateway hands to the Bot, so the Bot reads a broken character that is not on the page. The value side 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 cutAtCodeUnits already 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 about SnapshotElement), 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

  • New state that outlives a request? None. A pure function over a string already in hand.
  • What happens on the second replica? Nothing changes; no state.
  • Anything serialised? No.
  • Anything fanned out to a browser? No.
  • New listener, port, or schedule? No.

Boundary and audit

  • Every acting call still goes through the gateway: resolve, decide, audit, then act. Untouched; a snapshot is read-only and only the text of its elements changes.
  • New refusals and new failures each write a row. None added.
  • Nothing new is trusted from the client that the server can resolve itself.

Changelog

  • A line in CHANGELOG.md under Unreleased. 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:

cd agent-computer && bun test tests/aria-snapshot.test.ts
(fail) a name or value too long to keep whole > a name cut inside a character loses the whole character, not half of it
(fail) a name or value too long to keep whole > a value cut inside a character loses the whole character, not half of it
Expected: "aaaa…aaaa"
Received: "aaaa…aaaa�"
 28 pass
 2 fail

After:

cd agent-computer && bun test tests/aria-snapshot.test.ts
 30 pass
 0 fail

cd agent-computer && bun run typecheck          # tsc --noEmit, exit 0
bunx biome check agent-computer/src/aria-snapshot.ts agent-computer/tests/aria-snapshot.test.ts CHANGELOG.md   # no fixes

The whole agent-computer suite on Windows: 285 pass, 24 fail. Every failure is in shell.test.ts (spawns /bin/bash), workspace.test.ts (creates symlinks) or identity.test.ts (Unix sockets), none of which import the snapshot parser.

🤖 Generated with Claude Code

…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>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Hotragn Hotragn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants