Skip to content

Commit 7951072

Browse files
committed
fix: address PR #1196 review — contract boundary, formatting, path-6 reasons, worst-case sizing
- native-ref contract test: deliberately updated to assert the new ADR 0012 boundary — the preflight's node/preActionNodes ride the INTERNAL runtime result and visualization payload only; the public responseData never carries node/preActionNodes/targetEvidence (asserted directly against buildInteractionResponseData). - oxfmt formatting on all touched files. - classifyTargetBindingMatch path 6 now distinguishes decision 3's two spec-distinct outcomes: a signal isolating a member that differs from the winner ('signal-isolated-wrong', the paths-4/5 comparison class, future identity-mismatch) vs true fall-through ('no-signal-isolation', future identity-unverifiable), so step 4 can consume the reasons directly. - writer reduction loop sizes every candidate against the worst-case verification value ("unverifiable" is 4 bytes longer than "verified"), so a fail-closed self-check downgrade can never push an accepted payload over the 4 KiB cap; pinned by a 1-byte-granularity sweep across the boundary that fails against the old placeholder-sized check.
1 parent 3f8097d commit 7951072

7 files changed

Lines changed: 170 additions & 40 deletions

File tree

src/daemon/__tests__/session-target-evidence.test.ts

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,10 @@ test('computeTargetEvidence: max-size labels across K=8 ancestors reduce ancestr
187187
const evidence = computeTargetEvidence({ node: winner, nodes });
188188
assert.ok(evidence);
189189
assert.ok(evidence.ancestry.length <= TARGET_ANNOTATION_MAX_ANCESTRY);
190-
assert.ok(evidence.ancestry.length < TARGET_ANNOTATION_MAX_ANCESTRY, 'ancestry must have been reduced');
190+
assert.ok(
191+
evidence.ancestry.length < TARGET_ANNOTATION_MAX_ANCESTRY,
192+
'ancestry must have been reduced',
193+
);
191194
// Nearest-ancestor-first: the first (kept) entries are the leaf-side ones.
192195
assert.equal(evidence.ancestry[0]?.label, maxLabel);
193196

@@ -345,3 +348,62 @@ test('computeTargetEvidence: a node with an over-cap id still matches itself and
345348
assert.equal(evidence.id, overCapId.slice(0, TARGET_ANNOTATION_MAX_FIELD_BYTES));
346349
assert.equal(evidence.verification, 'verified');
347350
});
351+
352+
// ---------------------------------------------------------------------------
353+
// Worst-case verification sizing: "unverifiable" serializes 4 bytes longer
354+
// than "verified". The reduction loop must size each candidate against the
355+
// worst-case value so a fail-closed self-check downgrade can never push an
356+
// already-accepted payload over the 4 KiB cap. The window is only 4 bytes
357+
// wide, so sweep a tunable root-side label length across the boundary — some
358+
// length in the sweep necessarily lands inside the window, where a
359+
// placeholder-sized ("verified") check would accept a payload whose
360+
// downgraded form overflows.
361+
// ---------------------------------------------------------------------------
362+
363+
test('computeTargetEvidence: reduction sizes against the worst-case verification value across the 4 KiB boundary', () => {
364+
const maxField = 'x'.repeat(TARGET_ANNOTATION_MAX_FIELD_BYTES);
365+
366+
const buildChain = (tunableLabelLength: number): SnapshotNode[] => {
367+
const raw: RawSnapshotNode[] = [];
368+
// Root (index 0) .. leaf's parent (index 7): 8 ancestors total (K=8).
369+
// Root-side entries are dropped first, so the tunable label sits at the
370+
// root to sweep total payload size in 1-byte steps.
371+
raw.push({ index: 0, type: 'Window', label: 'w'.repeat(tunableLabelLength), depth: 0 });
372+
raw.push({ index: 1, type: 'View', label: 'v', depth: 1, parentIndex: 0 });
373+
for (let depth = 2; depth < 8; depth += 1) {
374+
raw.push({ index: depth, type: maxField, label: maxField, depth, parentIndex: depth - 1 });
375+
}
376+
raw.push({
377+
index: 8,
378+
type: maxField,
379+
label: maxField,
380+
rect: { x: 0, y: 0, width: 10, height: 10 },
381+
depth: 8,
382+
parentIndex: 7,
383+
});
384+
return toSnapshotNodes(raw);
385+
};
386+
387+
let sawFullAncestry = false;
388+
let sawReducedAncestry = false;
389+
for (let tunable = 0; tunable <= TARGET_ANNOTATION_MAX_FIELD_BYTES; tunable += 1) {
390+
const nodes = buildChain(tunable);
391+
const evidence = computeTargetEvidence({ node: nodes.at(-1)!, nodes });
392+
assert.ok(evidence);
393+
if (evidence.ancestry.length === TARGET_ANNOTATION_MAX_ANCESTRY) sawFullAncestry = true;
394+
else sawReducedAncestry = true;
395+
// The emitted payload must fit even re-serialized with the longer
396+
// verification value — the invariant the worst-case sizing guarantees.
397+
const worstCase = serializeTargetAnnotationV1({ ...evidence, verification: 'unverifiable' });
398+
assert.ok(
399+
utf8ByteLength(worstCase) <= TARGET_ANNOTATION_MAX_PAYLOAD_BYTES,
400+
`worst-case payload overflows at tunable label length ${tunable}`,
401+
);
402+
// And the parser accepts the writer's actual output, as always.
403+
parseTargetAnnotationV1Payload(serializeTargetAnnotationV1(evidence));
404+
}
405+
// The sweep must actually cross the reduction boundary for the window to
406+
// have been exercised.
407+
assert.ok(sawFullAncestry, 'sweep never produced an unreduced payload — fixture too large');
408+
assert.ok(sawReducedAncestry, 'sweep never forced a reduction — fixture too small');
409+
});

src/daemon/handlers/interaction-common.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,4 +114,4 @@ function extractTargetEvidenceForRecording(
114114
return { result: rest, targetEvidence: undefined };
115115
}
116116
return { result: rest, targetEvidence: computeTargetEvidence({ node, nodes: preActionNodes }) };
117-
}
117+
}

src/daemon/session-target-evidence.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,17 @@ export function computeTargetEvidence(params: {
8888

8989
for (let ancestryLength = fullAncestry.length; ancestryLength >= floor; ancestryLength -= 1) {
9090
const { candidate, domain } = buildCandidate(ancestryLength);
91-
if (utf8ByteLength(serializeTargetAnnotationV1(candidate)) <= TARGET_ANNOTATION_MAX_PAYLOAD_BYTES) {
91+
// Size against the WORST-CASE verification value ("unverifiable" is 4
92+
// serialized bytes longer than "verified") so the payload provably fits
93+
// no matter what the self-check below returns — otherwise a payload
94+
// within 4 bytes of the cap could pass this check as "verified" and then
95+
// overflow once a fail-closed self-check downgraded it, violating the
96+
// writer-parser invariant exactly in the rare capture-anomaly case it
97+
// exists for.
98+
if (
99+
utf8ByteLength(serializeTargetAnnotationV1({ ...candidate, verification: 'unverifiable' })) <=
100+
TARGET_ANNOTATION_MAX_PAYLOAD_BYTES
101+
) {
92102
candidate.verification = runRecordTimeSelfCheck({ node, domain });
93103
return candidate;
94104
}
@@ -100,7 +110,9 @@ export function computeTargetEvidence(params: {
100110
// rect (never compared) as one last, spec-consistent reduction rather
101111
// than emit a payload the parser would reject.
102112
candidate.verification = 'unverifiable';
103-
if (utf8ByteLength(serializeTargetAnnotationV1(candidate)) > TARGET_ANNOTATION_MAX_PAYLOAD_BYTES) {
113+
if (
114+
utf8ByteLength(serializeTargetAnnotationV1(candidate)) > TARGET_ANNOTATION_MAX_PAYLOAD_BYTES
115+
) {
104116
delete candidate.rect;
105117
}
106118
return candidate;
@@ -152,8 +164,12 @@ function buildAncestryChain(
152164
while (current && !visited.has(current.index) && chain.length < limit) {
153165
visited.add(current.index);
154166
const identity = boundedLocalIdentity(current);
155-
chain.push({ role: identity.role, ...(identity.label !== undefined ? { label: identity.label } : {}) });
156-
current = typeof current.parentIndex === 'number' ? byIndex.get(current.parentIndex) : undefined;
167+
chain.push({
168+
role: identity.role,
169+
...(identity.label !== undefined ? { label: identity.label } : {}),
170+
});
171+
current =
172+
typeof current.parentIndex === 'number' ? byIndex.get(current.parentIndex) : undefined;
157173
}
158174
return chain;
159175
}

src/replay/__tests__/script.test.ts

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -395,9 +395,11 @@ const SAVE_EVIDENCE_LINE =
395395
'# agent-device:target-v1 {"id":"save","role":"button","label":"Save","ancestry":[{"role":"toolbar","label":"Editor"},{"role":"window"}],"sibling":0,"viewportOrder":0,"scrollRegion":{"role":"scrollview","id":"editor-scroll"},"verification":"verified"}';
396396

397397
test('a target-v1 annotation immediately preceding an action line attaches to that action', () => {
398-
const script = ['context platform=ios device=iPhone', SAVE_EVIDENCE_LINE, 'click @e12 "Save"'].join(
399-
'\n',
400-
);
398+
const script = [
399+
'context platform=ios device=iPhone',
400+
SAVE_EVIDENCE_LINE,
401+
'click @e12 "Save"',
402+
].join('\n');
401403
const { actions } = parseReplayScriptDetailed(script);
402404
assert.equal(actions.length, 1);
403405
assert.deepEqual(actions[0]?.targetEvidence, SAVE_EVIDENCE);
@@ -438,11 +440,7 @@ test('a malformed target-v1 payload is rejected as INVALID_ARGS, not silently dr
438440
});
439441

440442
test('an unknown future target-vN comment is an ordinary comment: no binding requirement, no evidence attached', () => {
441-
const script = [
442-
'# agent-device:target-v2 {"whatever":true}',
443-
'',
444-
'click @e12 "Save"',
445-
].join('\n');
443+
const script = ['# agent-device:target-v2 {"whatever":true}', '', 'click @e12 "Save"'].join('\n');
446444
const { actions } = parseReplayScriptDetailed(script);
447445
assert.equal(actions.length, 1);
448446
assert.equal(actions[0]?.targetEvidence, undefined);

src/replay/__tests__/target-identity.test.ts

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,7 @@ function baseEvidence(overrides: Partial<TargetAnnotationV1> = {}): TargetAnnota
2222
id: 'save',
2323
role: 'button',
2424
label: 'Save',
25-
ancestry: [
26-
{ role: 'toolbar', label: 'Editor' },
27-
{ role: 'window' },
28-
],
25+
ancestry: [{ role: 'toolbar', label: 'Editor' }, { role: 'window' }],
2926
sibling: 0,
3027
viewportOrder: 0,
3128
scrollRegion: { role: 'scrollview', id: 'editor-scroll' },
@@ -171,10 +168,7 @@ test('matchesAncestryPrefix accepts an observed chain that is a superset on the
171168
});
172169

173170
test('matchesAncestryPrefix rejects an inserted wrapper ancestor (structure is part of identity)', () => {
174-
const recorded = [
175-
{ role: 'toolbar', label: 'Editor' },
176-
{ role: 'window' },
177-
];
171+
const recorded = [{ role: 'toolbar', label: 'Editor' }, { role: 'window' }];
178172
// A new wrapper view inserted directly above the target shifts every entry
179173
// one level down — the leaf-anchored prefix no longer matches.
180174
const observedWithInsertedWrapper = [
@@ -205,10 +199,7 @@ test('matchesLocalIdentity: a recorded id never matches a node without that id',
205199

206200
test('matchesLocalIdentity: with no recorded id, role+label must both match, absent-absent counts as equal', () => {
207201
assert.equal(matchesLocalIdentity({ role: 'button' }, { role: 'button' }), true);
208-
assert.equal(
209-
matchesLocalIdentity({ role: 'button', label: 'Save' }, { role: 'button' }),
210-
false,
211-
);
202+
assert.equal(matchesLocalIdentity({ role: 'button', label: 'Save' }, { role: 'button' }), false);
212203
});
213204

214205
// ---------------------------------------------------------------------------
@@ -249,7 +240,9 @@ test('parser rejects a payload exceeding the 4 KiB cap', () => {
249240
});
250241

251242
test('parser rejects more than 8 ancestry entries', () => {
252-
const ancestry = Array.from({ length: TARGET_ANNOTATION_MAX_ANCESTRY + 1 }, () => ({ role: 'view' }));
243+
const ancestry = Array.from({ length: TARGET_ANNOTATION_MAX_ANCESTRY + 1 }, () => ({
244+
role: 'view',
245+
}));
253246
assertInvalidArgs(
254247
() =>
255248
parseTargetAnnotationV1Payload(
@@ -418,7 +411,7 @@ test('classifyTargetBindingMatch path 6: a scroll region that no longer exists f
418411
regionMemberRefs: undefined, // region unavailable
419412
viewportCandidateRef: undefined,
420413
});
421-
assert.deepEqual(result, { path: 6, outcome: 'unverifiable' });
414+
assert.deepEqual(result, { path: 6, outcome: 'unverifiable', reason: 'no-signal-isolation' });
422415
});
423416

424417
test('classifyTargetBindingMatch path 6: an out-of-range recorded viewportOrder falls through to unverifiable', () => {
@@ -430,17 +423,40 @@ test('classifyTargetBindingMatch path 6: an out-of-range recorded viewportOrder
430423
regionMemberRefs: ['e1', 'e2'],
431424
viewportCandidateRef: undefined, // ordinal was out of range
432425
});
433-
assert.deepEqual(result, { path: 6, outcome: 'unverifiable' });
426+
assert.deepEqual(result, { path: 6, outcome: 'unverifiable', reason: 'no-signal-isolation' });
434427
});
435428

436-
test('classifyTargetBindingMatch path 6: viewportOrder resolves to a DIFFERENT node than the winner is unverifiable, never a silent pick', () => {
437-
const result = classifyTargetBindingMatch({
429+
test('classifyTargetBindingMatch path 6: a signal isolating a DIFFERENT node than the winner is the paths-4/5 comparison class, not fall-through', () => {
430+
// Decision 3 path 6.i/6.ii: when a signal denotes exactly one member,
431+
// "compare with W as in paths 4/5" — an isolated-but-different member is
432+
// the same class as path 5's unique-but-wrong rebind (a future
433+
// identity-mismatch), distinct from true no-isolation fall-through (a
434+
// future identity-unverifiable with candidates).
435+
const viaViewportOrder = classifyTargetBindingMatch({
438436
winnerRef: 'e1',
439437
matchedRefs: ['e1', 'e2'],
440438
identitySetRefs: ['e1', 'e2'],
441439
siblingMatchRefs: ['e1', 'e2'],
442440
regionMemberRefs: ['e1', 'e2'],
443441
viewportCandidateRef: 'e2',
444442
});
445-
assert.deepEqual(result, { path: 6, outcome: 'unverifiable' });
443+
assert.deepEqual(viaViewportOrder, {
444+
path: 6,
445+
outcome: 'unverifiable',
446+
reason: 'signal-isolated-wrong',
447+
});
448+
449+
const viaSibling = classifyTargetBindingMatch({
450+
winnerRef: 'e1',
451+
matchedRefs: ['e1', 'e2'],
452+
identitySetRefs: ['e1', 'e2'],
453+
siblingMatchRefs: ['e2'], // sibling isolates e2, not the winner
454+
regionMemberRefs: ['e1', 'e2'],
455+
viewportCandidateRef: 'e1',
456+
});
457+
assert.deepEqual(viaSibling, {
458+
path: 6,
459+
outcome: 'unverifiable',
460+
reason: 'signal-isolated-wrong',
461+
});
446462
});

src/replay/target-identity.ts

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,7 @@ export function normalizeRoleField(value: string): string {
8080
/** label fields: NFC, trim, collapse internal whitespace runs to one space. */
8181
export function normalizeLabelField(value: string | undefined): string | undefined {
8282
if (value === undefined) return undefined;
83-
const collapsed = nfc(value)
84-
.trim()
85-
.replace(/\s+/g, ' ');
83+
const collapsed = nfc(value).trim().replace(/\s+/g, ' ');
8684
return collapsed.length > 0 ? collapsed : undefined;
8785
}
8886

@@ -413,12 +411,25 @@ export type TargetBindingClassificationInput = {
413411
viewportCandidateRef: string | undefined;
414412
};
415413

414+
/**
415+
* Decision 3 keeps two spec-distinct failure classes inside path 6, and the
416+
* `reason` field preserves the distinction for migration step 4's divergence
417+
* `kind` mapping:
418+
*
419+
* - a disambiguation signal ISOLATING exactly one member that differs from
420+
* the winner is "compare with W as in paths 4/5" — the same class as path
421+
* 5's unique-but-wrong rebind, i.e. a future `identity-mismatch`
422+
* (`signal-isolated-wrong`);
423+
* - neither signal isolating any member is the true fall-through — a future
424+
* `identity-unverifiable` with up to 5 candidates (`no-signal-isolation`).
425+
*/
416426
export type TargetBindingClassification =
417427
| { path: 2; outcome: 'unverifiable'; reason: 'selector-miss' }
418428
| { path: 3; outcome: 'unverifiable'; reason: 'identity-set-empty' }
419429
| { path: 4; outcome: 'verified' }
420430
| { path: 5; outcome: 'unverifiable'; reason: 'unique-but-wrong' }
421-
| { path: 6; outcome: 'verified' | 'unverifiable' };
431+
| { path: 6; outcome: 'verified' }
432+
| { path: 6; outcome: 'unverifiable'; reason: 'signal-isolated-wrong' | 'no-signal-isolation' };
422433

423434
/**
424435
* Decision 3 "Replay-time verification", paths 2-6. `matchCount == 0` (path
@@ -442,18 +453,22 @@ export function classifyTargetBindingMatch(
442453
: { path: 5, outcome: 'unverifiable', reason: 'unique-but-wrong' };
443454
}
444455
if (input.siblingMatchRefs.length === 1) {
456+
// The sibling signal isolates exactly one member: the evidence denotes
457+
// it — compare with the winner as in paths 4/5 (decision 3, path 6.i).
445458
return input.siblingMatchRefs[0] === input.winnerRef
446459
? { path: 6, outcome: 'verified' }
447-
: { path: 6, outcome: 'unverifiable' };
460+
: { path: 6, outcome: 'unverifiable', reason: 'signal-isolated-wrong' };
448461
}
449462
if (
450463
input.regionMemberRefs !== undefined &&
451464
input.regionMemberRefs.length > 0 &&
452465
input.viewportCandidateRef !== undefined
453466
) {
467+
// Region-scoped viewportOrder denotes a member: compare as in paths 4/5
468+
// (decision 3, path 6.ii).
454469
return input.viewportCandidateRef === input.winnerRef
455470
? { path: 6, outcome: 'verified' }
456-
: { path: 6, outcome: 'unverifiable' };
471+
: { path: 6, outcome: 'unverifiable', reason: 'signal-isolated-wrong' };
457472
}
458-
return { path: 6, outcome: 'unverifiable' };
473+
return { path: 6, outcome: 'unverifiable', reason: 'no-signal-isolation' };
459474
}

test/integration/interaction-contract/native-ref.contract.test.ts

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import type { InteractionGuarantee } from '../../../src/contracts/interaction-gu
44
import type { SnapshotState } from '../../../src/kernel/snapshot.ts';
55
import { ref } from '../../../src/commands/index.ts';
66
import { scenarioName } from './coverage-manifest.ts';
7+
import { buildInteractionResponseData } from '../../../src/daemon/handlers/interaction-touch-response.ts';
78
import { NATIVE_REF_COVERAGE } from './native-ref.coverage.ts';
89
import {
910
closedDrawerSnapshot,
@@ -169,5 +170,27 @@ test(scenario('responseConstruction'), async () => {
169170
assert.deepEqual(result.target, { kind: 'ref', ref: '@e1' });
170171
assert.deepEqual(result.backendResult, { ref: 'e1' });
171172
assert.equal(result.point, undefined);
172-
assert.equal(result.node, undefined);
173+
// ADR 0012 decision 3: the preflight's already-fetched stored-snapshot node
174+
// and tree now ride on the INTERNAL runtime result so record-time
175+
// target-binding evidence can be computed at zero extra capture cost. This
176+
// is the internal boundary only — the shared construction site
177+
// (buildInteractionResponseData) attaches them exclusively to the internal
178+
// visualization payload, and the public responseData never carries them
179+
// (asserted below).
180+
assert.equal(result.node?.ref, 'e1');
181+
assert.ok(Array.isArray(result.preActionNodes));
182+
183+
const { result: visualization, responseData } = buildInteractionResponseData({
184+
source: { kind: 'runtime', result },
185+
referenceFrame: undefined,
186+
});
187+
// Internal visualization payload: carries the node/tree for the recording
188+
// pipeline (stripped again before session-history persistence by
189+
// extractTargetEvidenceForRecording).
190+
assert.ok(visualization.node);
191+
assert.ok(visualization.preActionNodes);
192+
// Public payload: the raw node/tree must never reach the wire.
193+
assert.equal('node' in responseData, false);
194+
assert.equal('preActionNodes' in responseData, false);
195+
assert.equal('targetEvidence' in responseData, false);
173196
});

0 commit comments

Comments
 (0)