Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion agent-computer/src/aria-snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
21 changes: 18 additions & 3 deletions agent-computer/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
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,
matchesToken,
offeredToken,
} from "./authorisation";
import { isPlainBotId } from "./bot-id";
import { browserModeFromEnv } from "./browser-mode";
import {
type Control,
ControlError,
Expand Down Expand Up @@ -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,
};
}
Expand Down
46 changes: 45 additions & 1 deletion agent-computer/tests/aria-snapshot.test.ts
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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.
Expand Down