Skip to content

Commit 9db4fb0

Browse files
thymikeethymikee
committed
fix(web): key the batch refusals by what broke, and pin each one
Second round on #3533. The refusals all shared one message prefix, which meant no test could tell them apart and an operator could not branch on them: each now carries a `reason` identifier (`batch-result-not-a-list`, `entry-count`, `entry-shape`, `entry-command`) beside the sentence, and the whole-batch fault is no longer reported as an `entry count` on an answer that was never a list - naming nothing counted. `requestedRefs` is the capture's ref count rather than the current group's, with `groupRefs` beside it, because the answer reads as a statement about the capture and one capture is measured in several groups. The empty-capture decision was encoded twice - an early return in the projection and `refs.length > 0` in the provider - with nothing pinning either. The provider keeps it (the loop over groups already does nothing) and a test states the consequence the projection depends on: a capture of only ref-less text sends no batch, and so cannot trip the all-unmeasurable refusal. Tests, which is where most of this lives: - The shape guard had no test that exercised it. Its fixture's bad member was `{}`, which is an object and therefore fell through to the echo check - deleting the guard kept all tests green. The member is now a non-object and the test names `entry-shape`. Mutation-checked: removing the shape guard or the list guard fails exactly one test each. - Every refusal test now asserts its own `reason` instead of the shared prefix. - The batch is the call that carries up to 200 commands and it had no cancellation assertion - a dropped signal there costs a full 30 s after the caller gave up. Pinned against the recorded `options.signal`; mutation-checked against the signal threading in `snapshot`. - `MAX_REFS_PER_RECT_BATCH`'s comment justified 200 by the stdin payload, which is not the constraint (23 bytes per ref). It is now bounded by the measurement that does decide it: a group has to finish inside `AGENT_BROWSER_TIMEOUT_MS`, and 200 refs measured in 0.28 s against 30 s. The spawn count in the same comment said 3 where the capture is 4.
1 parent ef9652a commit 9db4fb0

3 files changed

Lines changed: 113 additions & 39 deletions

File tree

‎packages/platform-web/src/agent-browser-provider-rects.test.ts‎

Lines changed: 80 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -207,8 +207,9 @@ test('agent-browser provider refuses snapshot geometry when the batch resolves n
207207
});
208208

209209
test('agent-browser provider refuses geometry when the batch answers with too few entries', async () => {
210-
// Entries pair back to refs by position, so a short answer cannot be trusted: pairing four refs
211-
// against the entries that exist would hand each node some other node's box.
210+
// Each entry has to name the command it answers, so an answer with fewer entries than commands
211+
// cannot be paired at all: filling four refs from the entries that exist would hand each node some
212+
// other node's box.
212213
await withWebProvider(async (provider) => {
213214
await assert.rejects(
214215
() =>
@@ -223,10 +224,7 @@ test('agent-browser provider refuses geometry when the batch answers with too fe
223224
},
224225
async () => await provider.snapshot({ includeRects: true }),
225226
),
226-
(error: unknown) =>
227-
error instanceof AppError &&
228-
error.code === 'COMMAND_FAILED' &&
229-
/unusable batch result/.test(error.message),
227+
refusedFor('entry-count'),
230228
);
231229
});
232230
});
@@ -244,17 +242,15 @@ test('agent-browser provider refuses geometry when the batch answers with an obj
244242
: snapshotPayload(),
245243
async () => await provider.snapshot({ includeRects: true }),
246244
),
247-
(error: unknown) =>
248-
error instanceof AppError &&
249-
error.code === 'COMMAND_FAILED' &&
250-
/unusable batch result/.test(error.message),
245+
refusedFor('batch-result-not-a-list'),
251246
);
252247
});
253248
});
254249

255250
test('agent-browser provider refuses geometry when a batch entry is not an entry', async () => {
256-
// Right number of answers, one of them not a result object — an empty object carrying neither the
257-
// command it answers nor a verdict: the positions no longer describe the refs they measured.
251+
// Right number of answers, one of them not an object at all. The point of this test is the shape
252+
// guard, so the member has to be one the guard alone rejects: an object would be refused further
253+
// down by the echo check and the branch under test would go unpinned.
258254
await withWebProvider(async (provider) => {
259255
await assert.rejects(
260256
() =>
@@ -265,18 +261,15 @@ test('agent-browser provider refuses geometry when a batch entry is not an entry
265261
return jsonResult(
266262
commands.map((command, index) =>
267263
index === 0
268-
? {}
264+
? null
269265
: { command, success: true, error: null, result: boxRect(command[2]) },
270266
),
271267
1,
272268
);
273269
},
274270
async () => await provider.snapshot({ includeRects: true }),
275271
),
276-
(error: unknown) =>
277-
error instanceof AppError &&
278-
error.code === 'COMMAND_FAILED' &&
279-
/unusable batch result/.test(error.message),
272+
refusedFor('entry-shape'),
280273
);
281274
});
282275
});
@@ -305,10 +298,7 @@ test('agent-browser provider refuses geometry when an entry echoes a different c
305298
},
306299
async () => await provider.snapshot({ includeRects: true }),
307300
),
308-
(error: unknown) =>
309-
error instanceof AppError &&
310-
error.code === 'COMMAND_FAILED' &&
311-
/unusable batch result/.test(error.message),
301+
refusedFor('entry-command'),
312302
);
313303
});
314304
});
@@ -356,6 +346,64 @@ test('agent-browser provider surfaces a stale ref as a refusal of the geometry i
356346
});
357347
});
358348

349+
test('agent-browser provider passes snapshot cancellation to the geometry batch', async () => {
350+
// The batch is the call that now carries up to 200 commands, so it is the one where a dropped
351+
// signal costs a whole 30 s after the caller gave up. The capture call has its own test; this is
352+
// the geometry half.
353+
const controller = new AbortController();
354+
await withWebProvider(async (provider) => {
355+
const signals: Array<AbortSignal | undefined> = [];
356+
await withCommandExecutorOverride(
357+
async (_cmd, args, options) => {
358+
signals.push(options.signal);
359+
if (agentBrowserCliArgs(args)[0] !== 'batch') return snapshotPayload();
360+
const commands = readBatchCommands(options.stdin);
361+
return jsonResult(
362+
commands.map((command) => ({
363+
command,
364+
success: true,
365+
error: null,
366+
result: boxRect(command[2]),
367+
})),
368+
);
369+
},
370+
async () => await provider.snapshot({ includeRects: true, signal: controller.signal }),
371+
);
372+
assert.equal(signals.length, 2, 'one capture call and one geometry batch');
373+
assert.equal(
374+
signals[1],
375+
controller.signal,
376+
'the geometry batch is cancellable by the request that asked for it',
377+
);
378+
});
379+
});
380+
381+
test('agent-browser provider asks the backend nothing for a capture of only ref-less text', async () => {
382+
// A capture of only ref-less text has no box to ask for. Declining is the fetcher's decision, and
383+
// pinning it here is what keeps the all-unmeasurable refusal from firing on an empty tree.
384+
await withWebProvider(async (provider) => {
385+
const calls: AgentBrowserCall[] = [];
386+
const snapshot = await withCommandExecutorOverride(
387+
async (cmd, args) => {
388+
recordCall(calls, cmd, args);
389+
return jsonResult({
390+
success: true,
391+
data: { refs: {}, snapshot: '- status "Hello Zed"', truncated: false },
392+
});
393+
},
394+
async () => await provider.snapshot({ includeRects: true }),
395+
);
396+
397+
assert.deepEqual(
398+
calls.map((call) => call.args[0]),
399+
['snapshot'],
400+
'no batch is sent when nothing in the capture has a ref',
401+
);
402+
assert.equal(snapshot.nodes.length, 1);
403+
assert.equal(snapshot.nodes[0]?.ref, undefined);
404+
});
405+
});
406+
359407
async function withWebProvider(
360408
testFn: (provider: ReturnType<typeof createAgentBrowserWebProvider>) => void | Promise<void>,
361409
): Promise<void> {
@@ -410,6 +458,17 @@ function assertGeometryRects(snapshot: WebSnapshotResult): void {
410458
);
411459
}
412460

461+
/**
462+
* The refusal this file is about, pinned to the invariant that broke. Every branch below shares the
463+
* message prefix, so matching the prefix alone would let any of them satisfy any of these tests.
464+
*/
465+
function refusedFor(reason: string) {
466+
return (error: unknown) =>
467+
error instanceof AppError &&
468+
error.code === 'COMMAND_FAILED' &&
469+
error.details?.reason === reason;
470+
}
471+
413472
/** The commands the provider wrote for one `batch`, which is the shape this file is about. */
414473
function readBatchCommands(stdin: string | Buffer | undefined): string[][] {
415474
assert.equal(typeof stdin, 'string', 'batch reads its commands from stdin');

‎packages/platform-web/src/agent-browser-provider.ts‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -260,17 +260,18 @@ function buildSnapshotArgs(options: WebSnapshotOptions | undefined): string[] {
260260
}
261261

262262
/**
263-
* Refs per backend `batch` call. The backend reads a batch from stdin as one JSON array, so the
264-
* group is bounded by that payload rather than by a round-trip budget: measured on a 408-ref page,
265-
* 200 refs measured in 0.28 s inside one process, so a group finishes far inside the command timeout
266-
* the per-ref path also had — and the whole capture now costs 3 spawns instead of 381.
263+
* Refs per backend `batch` call. The bound is the group that finishes inside the command timeout it
264+
* runs under (`AGENT_BROWSER_TIMEOUT_MS`, 30 s) with room to spare: measured on a 408-ref page, 200
265+
* refs measured in 0.28 s and all 408 in 0.33 s inside one process, because a command inside a batch
266+
* is a DOM read of an already-loaded page rather than an interpreter boot. Raising the number is safe
267+
* only while that ratio holds — the stdin payload itself (~23 bytes per ref) is not the constraint.
267268
*/
268269
const MAX_REFS_PER_RECT_BATCH = 200;
269270

270271
/**
271272
* The bounds of every ref of the capture, in `batch` round trips: the backend measures them inside
272-
* one process, where the per-ref alternative pays one spawn per ref (measured on a 408-ref page: 90 s
273-
* of spawns against 0.3 s of batching).
273+
* one process, where the per-ref alternative pays one spawn per ref (measured on a 408-ref page: one
274+
* capture cost 385 spawns and 90 s, and 4 spawns and 0.3 s batched).
274275
*
275276
* A ref the backend could not measure comes back as a failed entry and simply gets no rect — an
276277
* element that is absent or has no box is an ordinary answer, and the entry's own `success` field is
@@ -282,12 +283,16 @@ async function fetchRefRects(
282283
refs: readonly string[],
283284
): Promise<Map<string, Rect>> {
284285
const rects = new Map<string, Rect>();
286+
// A capture with no addressable node is an empty group list, so the loop below never runs and the
287+
// backend is never called. That is the one place this seam declines to ask, and it is also what
288+
// keeps such a capture from tripping the refusal: nothing failed, there was nothing to measure.
285289
for (let start = 0; start < refs.length; start += MAX_REFS_PER_RECT_BATCH) {
286290
const group = refs.slice(start, start + MAX_REFS_PER_RECT_BATCH);
287291
const commands = group.map((ref) => ['get', 'box', browserRefSelector(ref)]);
288292
const entries = parseBatchEntries(
289293
await runJson(['batch'], { stdin: JSON.stringify(commands) }),
290294
commands,
295+
refs.length,
291296
);
292297
for (const [index, entry] of entries.entries()) {
293298
const ref = group[index];
@@ -326,13 +331,20 @@ type AgentBrowserBatchEntry = {
326331
/**
327332
* The backend answers a batch with one entry per command, echoing each command back, and reports a
328333
* whole-batch fault as a single object instead of an array. The echo is what pairs an entry to the
329-
* ref it measured: a count that disagrees, an entry that is not an object, or an entry describing
330-
* some other command means the answer cannot be matched back, so the geometry is refused rather than
331-
* guessed at. Position is trusted only as the order of the echo, never as the pairing itself.
334+
* ref it measured, so position is trusted only as the order of the echo, never as the pairing
335+
* itself. Anything that breaks that pairing — an answer that is not a list of entries, a list that
336+
* does not cover the commands sent, an entry with no verdict, an entry describing some other command
337+
* — means the geometry cannot be matched back and is refused, naming which invariant broke.
338+
*
339+
* Each refusal is keyed by a stable `reason` identifier rather than by its own sentence, so a caller
340+
* branches on the failure without reading prose. `captureRefs` is the capture's whole ref count
341+
* instead of this group's, because the answer reads as a statement about the capture and one capture
342+
* can be measured in several groups.
332343
*/
333344
function parseBatchEntries(
334345
data: unknown,
335346
commands: readonly (readonly string[])[],
347+
captureRefs: number,
336348
): AgentBrowserBatchEntry[] {
337349
const unusable = (reason: string): AppError =>
338350
new AppError(
@@ -341,15 +353,18 @@ function parseBatchEntries(
341353
{
342354
cmd: AGENT_BROWSER,
343355
args: ['batch'],
344-
requestedRefs: commands.length,
356+
reason,
357+
requestedRefs: captureRefs,
358+
groupRefs: commands.length,
345359
hint: AGENT_BROWSER_DOCTOR_HINT,
346360
},
347361
);
348-
if (!Array.isArray(data) || data.length !== commands.length) throw unusable('entry count');
349-
if (!data.every(isBatchEntry)) throw unusable('entry shape');
362+
if (!Array.isArray(data)) throw unusable('batch-result-not-a-list');
363+
if (data.length !== commands.length) throw unusable('entry-count');
364+
if (!data.every(isBatchEntry)) throw unusable('entry-shape');
350365
const entries = data;
351366
for (const [index, entry] of entries.entries()) {
352-
if (!sameBatchCommand(entry.command, commands[index])) throw unusable('entry command');
367+
if (!sameBatchCommand(entry.command, commands[index])) throw unusable('entry-command');
353368
}
354369
return entries;
355370
}

‎packages/platform-web/src/agent-browser-snapshot.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,11 +79,11 @@ async function attachDraftRects(
7979
drafts: SnapshotDraftNode[],
8080
fetchRects: SnapshotRectFetch,
8181
): Promise<void> {
82-
// Only a node the backend issued a ref for has a box to ask for: a ref-less line is text the
83-
// backend never measured, and `get box` has no handle for it.
84-
const addressable = drafts.flatMap((draft) => (draft.ref === undefined ? [] : [draft.ref]));
85-
if (addressable.length === 0) return;
86-
const rects = await fetchRects(addressable);
82+
// One fetch for the whole capture. A capture of nothing addressable still comes through, and the
83+
// fetcher decides not to call the backend for it: that decision belongs with the refusal below.
84+
const rects = await fetchRects(
85+
drafts.flatMap((draft) => (draft.ref === undefined ? [] : [draft.ref])),
86+
);
8787
for (const draft of drafts) {
8888
const ref = draft.ref;
8989
const rect = ref === undefined ? undefined : rects.get(ref);

0 commit comments

Comments
 (0)