Skip to content

Commit 12a7cae

Browse files
thymikeethymikee
committed
fix(daemon): exclude keyboard descendants (not just the container) from baseline evidence
PR review on #1563 (two findings, blocking merge): 1. isKeyboardChromeKind excluded only the [Keyboard] container node itself. collectKeyboardChrome (src/core/snapshot-chrome.ts, the established source of truth) classifies the WHOLE keyboard window/subtree — keys, AND the "Next keyboard"/"Dictate" assistant buttons, which are documented siblings of the container, not descendants, so a container-descendant walk alone provably misses them. In the scope-drift case this PR supports, a successful scroll can leave only those keyboard descendants shared between a baseline and a later capture, and the narrower check called that a baseline match — extending a fresh result to the 3.5s stale-read cap. Fixed by exporting a narrow predicate, collectKeyboardChromeRefs(nodes), from snapshot-chrome.ts (returns collectKeyboardChrome(nodes).refs, no Android union — this caller has no appBundleId in scope and only needs the iOS half). buildInteractionSurfaceSignature computes it once per signature build and threads it into buildInteractionSurfaceEntry, so discriminating is now `!isViewportRootKind(node) && !keyboardChromeRefs .has(node.ref)` — reusing the real ancestor-walk classification instead of a per-node type check, no ancestry needed in the signature entries themselves. 2. post-gesture-stabilization.test.ts had grown to 550 LOC, past the repository's 500-line extraction tripwire (AGENTS.md: "past 500, extract before adding behavior... Tests are not exempt"). Split along subject lines: the pure decidePostGestureStabilityVerdict coverage moved to a new sibling post-gesture-stabilization-verdict.test.ts, and shared fixtures (pickupSnapshot, deliverySnapshot, applicationRootNode, keyboardWindowNodes, makeSession) moved to a new non-test post-gesture-stabilization-fixtures.ts. The async capturePostGesture- StabilizedResult loop tests stay in the original file. Assertions unchanged, only relocation, plus the new regression tests below. Resulting LOC: post-gesture-stabilization.test.ts 381, -verdict.test.ts 208, -fixtures.ts 129 (interaction-outcome-policy.test.ts grew to 413, still under the tripwire). Tests: the reviewer's exact regression — a shared overlap consisting only of keyboard descendants (a key + the "Next keyboard" sibling button, NOT the container) plus real content that changed (Pickup -> Delivery) — at three layers: classifyBaselineSurfaceEvidence directly (ambiguous), the verdict function (trust, elapsedMs: 0), and the full async capture loop (settles in 2 attempts, no cap tax). Counterfactual: reverted isNonDiscriminatingSurfaceNode to a container-only check (normalizeType(node.type) === 'keyboard') and reran — 3 of the new tests went red across all three layers, including the async test, which timed out after 5s (not just a failed assertion) because the container-only exclusion genuinely extended the interaction to the 3.5s distrust cap the test's 1s timer advance never covers — the same "extends to cap" failure shape as the review's finding 1. Restored, 40/40 green. _Generated by [Claude Code](https://claude.ai/code)_
1 parent 191c351 commit 12a7cae

6 files changed

Lines changed: 513 additions & 244 deletions

‎src/core/snapshot-chrome.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,26 @@ export function collectSettleChromeRefs(
325325
return collectSettleChrome(nodes, appBundleId).refs;
326326
}
327327

328+
/**
329+
* Refs of iOS keyboard-window chrome ONLY — no Android union, so callers with
330+
* no `appBundleId` in scope can still reuse the real subtree/window-aware
331+
* classification (`collectKeyboardChrome`) instead of a narrower per-node type
332+
* check. Container-descendant walk alone provably misses the "Next keyboard"
333+
* / "Dictate" assistant buttons (siblings of the `[Keyboard]` container, not
334+
* descendants — see `collectKeyboardChrome`'s doc comment), so a caller that
335+
* only excludes nodes whose OWN type is `keyboard` still leaks every key and
336+
* assistant control as "discriminating" evidence.
337+
*
338+
* Used by `src/daemon/interaction-outcome-policy.ts`'s post-gesture
339+
* baseline-distrust discriminating-overlap classification (#1542 defect 2,
340+
* #1563 review): that comparison operates on flat signature entries with no
341+
* ref-selection budget of its own, so it needs the ref set directly rather
342+
* than a node-filtering helper like `withoutSettleChrome`.
343+
*/
344+
export function collectKeyboardChromeRefs(nodes: SnapshotNode[]): ReadonlySet<string> {
345+
return collectKeyboardChrome(nodes).refs;
346+
}
347+
328348
/**
329349
* Windows eligible for whole-window chrome classification: nearest `[window]`
330350
* ancestor of each `[Keyboard]` container, minus windows hosting editable

‎src/daemon/__tests__/interaction-outcome-policy.test.ts‎

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ test('classifyBaselineSurfaceEvidence is ambiguous when the current capture is t
175175
assert.equal(classifyBaselineSurfaceEvidence(baseline, current), 'ambiguous');
176176
});
177177

178-
test('classifyBaselineSurfaceEvidence excludes keyboard chrome from discriminating overlap', () => {
178+
test('classifyBaselineSurfaceEvidence excludes the keyboard container from discriminating overlap', () => {
179179
const keyboardNode = {
180180
ref: 'e3',
181181
index: 2,
@@ -192,6 +192,91 @@ test('classifyBaselineSurfaceEvidence excludes keyboard chrome from discriminati
192192
assert.equal(classifyBaselineSurfaceEvidence(baseline, current), 'ambiguous');
193193
});
194194

195+
// #1563 review, finding 2: a container-only exclusion still misses keyboard
196+
// DESCENDANTS (individual keys) and SIBLINGS (assistant buttons like "Next
197+
// keyboard"/"Dictate", which live outside the container per
198+
// src/core/snapshot-chrome.ts's collectKeyboardChrome doc comment — a
199+
// container-descendant walk alone provably misses them, hence the whole-
200+
// window classification that module reuses here via collectKeyboardChromeRefs).
201+
test('classifyBaselineSurfaceEvidence excludes keyboard DESCENDANTS and window SIBLINGS, not just the container, from discriminating overlap', () => {
202+
const shared = keyboardWindowNodes(); // window + [Keyboard] container + a key + a sibling "Next keyboard" button
203+
const baseline = buildInteractionSurfaceSignature([
204+
applicationRootNode(),
205+
{
206+
ref: 'e-pickup',
207+
index: 20,
208+
parentIndex: 0,
209+
type: 'Button',
210+
identifier: 'shipping-pickup',
211+
label: 'Pickup',
212+
rect: { x: 20, y: 500, width: 200, height: 44 },
213+
},
214+
...shared,
215+
]);
216+
// Real content changed (Pickup -> Delivery, a genuine successful scroll);
217+
// the keyboard subtree is identical — a keyboard does not move when app
218+
// content scrolls.
219+
const current = buildInteractionSurfaceSignature([
220+
applicationRootNode(),
221+
{
222+
ref: 'e-delivery',
223+
index: 20,
224+
parentIndex: 0,
225+
type: 'Button',
226+
identifier: 'shipping-delivery',
227+
label: 'Delivery',
228+
rect: { x: 20, y: 120, width: 200, height: 44 },
229+
},
230+
...shared,
231+
]);
232+
233+
assert.equal(classifyBaselineSurfaceEvidence(baseline, current), 'ambiguous');
234+
});
235+
236+
/**
237+
* A keyboard-window subtree: a `[Keyboard]` container plus a SIBLING "Next
238+
* keyboard" assistant button under the same window — matches the shape in
239+
* `src/daemon/__tests__/post-gesture-stabilization-fixtures.ts`'s
240+
* `keyboardWindowNodes` (kept local here rather than imported: this file's
241+
* fixtures are raw node literals consumed directly by
242+
* `buildInteractionSurfaceSignature`, not `SnapshotState`-wrapped like that
243+
* module's).
244+
*/
245+
function keyboardWindowNodes() {
246+
return [
247+
{
248+
ref: 'e-kb-window',
249+
index: 10,
250+
parentIndex: 0,
251+
type: 'Window',
252+
rect: { x: 0, y: 400, width: 390, height: 444 },
253+
},
254+
{
255+
ref: 'e-kb-container',
256+
index: 11,
257+
parentIndex: 10,
258+
type: 'Keyboard',
259+
rect: { x: 0, y: 500, width: 390, height: 300 },
260+
},
261+
{
262+
ref: 'e-kb-key-a',
263+
index: 12,
264+
parentIndex: 11, // descendant of the container
265+
type: 'Key',
266+
label: 'A',
267+
rect: { x: 10, y: 520, width: 30, height: 40 },
268+
},
269+
{
270+
ref: 'e-kb-next',
271+
index: 13,
272+
parentIndex: 10, // sibling of the container, NOT a descendant
273+
type: 'Button',
274+
label: 'Next keyboard',
275+
rect: { x: 340, y: 520, width: 40, height: 40 },
276+
},
277+
];
278+
}
279+
195280
test('classifyBaselineSurfaceEvidence still reports unchanged when the root AND a real discriminating element both match (guards against over-excluding)', () => {
196281
// Root-sharing alone is not disqualifying — it just cannot be the ONLY
197282
// evidence. Once a real, frozen discriminating element is also shared

‎src/daemon/__tests__/post-gesture-stabilization-fixtures.ts‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
import { ANDROID_EMULATOR, IOS_SIMULATOR } from '../../__tests__/test-utils/device-fixtures.ts';
2+
import { makeSnapshotState } from '../../__tests__/test-utils/index.ts';
3+
import type { SessionState } from '../types.ts';
4+
5+
/**
6+
* Shared fixtures for post-gesture-stabilization.test.ts (the async capture
7+
* loop) and post-gesture-stabilization-verdict.test.ts (the pure
8+
* verdict/classifier coverage) — split by subject per #1563 review, to stay
9+
* under the repo's 500-line test-file tripwire (AGENTS.md). Not a `.test.ts`
10+
* file, so vitest never tries to run it directly.
11+
*/
12+
13+
export function pickupSnapshot(y = 500) {
14+
return makeSnapshotState([
15+
{ index: 0, type: 'Application', label: 'App', rect: { x: 0, y: 0, width: 390, height: 844 } },
16+
{
17+
index: 1,
18+
parentIndex: 0,
19+
type: 'Button',
20+
identifier: 'shipping-pickup',
21+
label: 'Pickup',
22+
rect: { x: 20, y, width: 200, height: 44 },
23+
},
24+
]);
25+
}
26+
27+
// Same Application root as pickupSnapshot, but a DIFFERENT real element —
28+
// models a genuine, successful scroll that swapped every real element in
29+
// view, so the only entry shared with a pickupSnapshot baseline is the root.
30+
export function deliverySnapshot(y = 500) {
31+
return makeSnapshotState([
32+
{ index: 0, type: 'Application', label: 'App', rect: { x: 0, y: 0, width: 390, height: 844 } },
33+
{
34+
index: 1,
35+
parentIndex: 0,
36+
type: 'Button',
37+
identifier: 'shipping-delivery',
38+
label: 'Delivery',
39+
rect: { x: 20, y, width: 200, height: 44 },
40+
},
41+
]);
42+
}
43+
44+
// Broader-scope variant: adds a non-interactive text node an interactive-only
45+
// capture would never return, modeling the real pre-gesture-baseline vs
46+
// post-gesture-selector-capture scope mismatch.
47+
export function pickupSnapshotWithExtraText(y = 500) {
48+
const base = pickupSnapshot(y);
49+
return {
50+
...base,
51+
nodes: [
52+
...base.nodes,
53+
{
54+
ref: 'e3',
55+
index: 2,
56+
parentIndex: 0,
57+
type: 'Text',
58+
label: 'Delivery choices',
59+
rect: { x: 20, y: 300, width: 200, height: 20 },
60+
},
61+
],
62+
};
63+
}
64+
65+
export function applicationRootNode() {
66+
return {
67+
ref: 'e-root',
68+
index: 0,
69+
type: 'Application',
70+
label: 'App',
71+
rect: { x: 0, y: 0, width: 390, height: 844 },
72+
};
73+
}
74+
75+
/**
76+
* A keyboard-window subtree modeling the #1563 review's second finding: a
77+
* `[Keyboard]` container PLUS a sibling "Next keyboard" assistant button
78+
* under the SAME window — a container-descendant-only walk provably misses
79+
* the sibling (see `collectKeyboardChrome`'s doc comment in
80+
* src/core/snapshot-chrome.ts, the source of truth this fixture's shape is
81+
* drawn from: "a SIBLING subtree holding the 'Next keyboard' and 'Dictate'
82+
* buttons — siblings of the container, so a container-descendant walk alone
83+
* provably misses them"). Neither entry is the container itself, so sharing
84+
* only these between a baseline and a later capture is the exact
85+
* keyboard-descendants-only regression shape.
86+
*/
87+
export function keyboardWindowNodes() {
88+
return [
89+
{
90+
ref: 'e-kb-window',
91+
index: 10,
92+
parentIndex: 0,
93+
type: 'Window',
94+
rect: { x: 0, y: 400, width: 390, height: 444 },
95+
},
96+
{
97+
ref: 'e-kb-container',
98+
index: 11,
99+
parentIndex: 10,
100+
type: 'Keyboard',
101+
rect: { x: 0, y: 500, width: 390, height: 300 },
102+
},
103+
{
104+
ref: 'e-kb-key-a',
105+
index: 12,
106+
parentIndex: 11, // descendant of the container
107+
type: 'Key',
108+
label: 'A',
109+
rect: { x: 10, y: 520, width: 30, height: 40 },
110+
},
111+
{
112+
ref: 'e-kb-next',
113+
index: 13,
114+
parentIndex: 10, // sibling of the container, NOT a descendant
115+
type: 'Button',
116+
label: 'Next keyboard',
117+
rect: { x: 340, y: 520, width: 40, height: 40 },
118+
},
119+
];
120+
}
121+
122+
export function makeSession(platform: 'ios' | 'android' = 'ios'): SessionState {
123+
return {
124+
name: platform,
125+
device: platform === 'android' ? ANDROID_EMULATOR : IOS_SIMULATOR,
126+
createdAt: Date.now(),
127+
actions: [],
128+
};
129+
}

0 commit comments

Comments
 (0)