diff --git a/CHANGELOG.md b/CHANGELOG.md index fa789eea5..2735e38aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,13 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A long page's text is cut between characters, not through one + +A navigation hands the Bot the first 6000 UTF-16 code units of the page's readable text. When that +limit fell between the two halves of an emoji, the Bot was handed text ending on half a character, +which reads as U+FFFD: a broken character that is not on the page. It now stops one code unit short +in that case, which is what a control's name and value in a page snapshot already did. + ### Tool selection reads a skill choice the model wrapped in a code fence Before a run, the deployment's model picks which of a Bot's skills the message needs, so a Bot holding diff --git a/agent-computer/src/aria-snapshot.ts b/agent-computer/src/aria-snapshot.ts index d15781bba..88db6ad7e 100644 --- a/agent-computer/src/aria-snapshot.ts +++ b/agent-computer/src/aria-snapshot.ts @@ -163,8 +163,12 @@ export function parseDescriptor(text: string): Descriptor | null { * high surrogate last: JSON carries it as a bare `\ud83d` and UTF-8 as U+FFFD, and the Bot reads a * broken character that is not on the page. The server's `cutAtCodeUnits` is the same rule; this * process shares no code with the server, so it is repeated here rather than imported. + * + * Exported for `index.ts`, which cuts the readable page text the same way, and so that the rule has + * its own tests. It lives here rather than beside that caller because this module imports no + * Playwright: `index.ts` does, at load, so a helper declared there could not be tested at all. */ -function cutAtCodeUnits(text: string, limit: number): string { +export function cutAtCodeUnits(text: string, limit: number): string { const sliced = text.slice(0, limit); const last = sliced.charCodeAt(sliced.length - 1); return last >= 0xd800 && last <= 0xdbff ? sliced.slice(0, -1) : sliced; diff --git a/agent-computer/src/index.ts b/agent-computer/src/index.ts index 0d32fffdb..6fd7ed295 100644 --- a/agent-computer/src/index.ts +++ b/agent-computer/src/index.ts @@ -1,7 +1,10 @@ import { serve } from "bun"; import type { Page } from "playwright"; -import { parseAriaSnapshot, type SnapshotElement } from "./aria-snapshot"; -import { browserModeFromEnv } from "./browser-mode"; +import { + cutAtCodeUnits, + parseAriaSnapshot, + type SnapshotElement, +} from "./aria-snapshot"; import { actsOnTheComputer, isOpenPath, @@ -9,6 +12,7 @@ import { offeredToken, } from "./authorisation"; import { isPlainBotId } from "./bot-id"; +import { browserModeFromEnv } from "./browser-mode"; import { type Control, ControlError, @@ -289,7 +293,18 @@ async function readablePageText( const collapsed = raw.replace(/\n{3,}/g, "\n\n").trim(); return { - text: collapsed.slice(0, TEXT_EXTRACT_LIMIT), + /* + * Cut between characters, not through one. #539 fixed this for a control's name and value in the + * snapshot and left the page text, which is the same bug at thirty times the length: `slice` + * counts UTF-16 code units, an emoji is two, and a limit landing between the halves hands the Bot + * a lone high surrogate that reads as U+FFFD — a character that is not on the page. + * + * More likely to bite here than there, for the reason the limit is bigger. A 200-unit control + * name rarely reaches an emoji; 6000 units of somebody's page usually passes through several, and + * whether the cut lands mid-character is decided by whatever was above it. + */ + text: cutAtCodeUnits(collapsed, TEXT_EXTRACT_LIMIT), + // Unaffected by the line above: dropping one more unit cannot make an over-limit string fit. truncated: collapsed.length > TEXT_EXTRACT_LIMIT, }; } diff --git a/agent-computer/tests/aria-snapshot.test.ts b/agent-computer/tests/aria-snapshot.test.ts index f95619460..4052b47ee 100644 --- a/agent-computer/tests/aria-snapshot.test.ts +++ b/agent-computer/tests/aria-snapshot.test.ts @@ -1,5 +1,9 @@ import { describe, expect, test } from "bun:test"; -import { parseAriaSnapshot, parseDescriptor } from "../src/aria-snapshot"; +import { + cutAtCodeUnits, + parseAriaSnapshot, + parseDescriptor, +} from "../src/aria-snapshot"; /** * The parser, tested against captured Playwright output. @@ -200,6 +204,46 @@ describe("a name or value too long to keep whole", () => { }); }); +/** + * The cut itself, at any limit. + * + * Tested directly as well as through the parser because `index.ts` cuts the readable page text with + * it at 6000 rather than 200, and that caller imports Playwright at load, so there is no test that + * can reach it. The rule is the thing worth pinning, so it is pinned where it is declared. + */ +describe("cutting at code units", () => { + const EMOJI = "\u{1F600}"; + + test("a limit landing between the halves of a character drops the character", () => { + const text = `${"a".repeat(5999)}${EMOJI}tail`; + const cut = cutAtCodeUnits(text, 6000); + expect(cut).toBe("a".repeat(5999)); + // Not a lone high surrogate, which is what a bare `slice` leaves and what reads as U+FFFD. + expect(cut.charCodeAt(cut.length - 1)).toBeLessThan(0xd800); + }); + + test("a character that ends exactly at the limit is kept whole", () => { + const text = `${"a".repeat(5998)}${EMOJI}tail`; + expect(cutAtCodeUnits(text, 6000)).toBe(`${"a".repeat(5998)}${EMOJI}`); + }); + + test("text that fits is returned unchanged, emoji and all", () => { + const text = `hello ${EMOJI} world`; + expect(cutAtCodeUnits(text, 6000)).toBe(text); + }); + + test("empty text is empty rather than a thrown index", () => { + // `charCodeAt(-1)` is NaN and every comparison against it is false, so this returns "". + expect(cutAtCodeUnits("", 6000)).toBe(""); + }); + + test("a low surrogate last is a whole character and is kept", () => { + // The guard must look only for an UNPAIRED high surrogate. A complete pair ends on its low half, + // and dropping that would cost a character the limit had room for. + expect(cutAtCodeUnits(EMOJI, 2)).toBe(EMOJI); + }); +}); + describe("values a real parser handles and a pattern got wrong", () => { test("a quoted numeric value is not left with its quotes", () => { // Numeric-looking text remains a string, so one-time codes are not coerced.