Skip to content

Commit 98f2d74

Browse files
committed
fix(#2682): disclose only the repair a request paid for
Adversarial review findings on #2693: - A cache hit is not a repair. Selector reads may answer from the tree an earlier command stored (`reusableLastSnapshot`, `reusableSessionSnapshot`), and stamping the disclosure off that consumed tree told a command "the session app was not foreground when THIS command arrived" when it never looked. The repair now travels on its own per-request proof, filled only by a capture the request actually took, first fact wins; the #2438 surface disclosure keeps reading the consumed tree, which is what the response actually describes. - The snapshot route read the stored snapshot unconditionally, including on a failure response. It now binds the fact to its own capture inside `execute`, so a snapshot that failed before capturing inherits nothing. - Docs said a command that consumed no capture "stays silent", which reads as "no repair happened". That is ADR 0012's category wearing a disclaimer: the runner may well have activated and the interaction projection drops the stamp. The docs now say silence proves nothing and point at #2694. - `sessions.md` still claimed iOS `appstate` worked; the appstate pointer, the `bundle_changed` definition, and the missing `missing_after_wait` / `interaction_foreground_guard` reasons are fixed, and the identity rename lands here too (`otherActiveApplicationPid`).
1 parent 80470a0 commit 98f2d74

18 files changed

Lines changed: 381 additions & 63 deletions

‎packages/kernel/src/snapshot.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -445,14 +445,15 @@ export type IosTargetActivationPriorState = (typeof IOS_TARGET_ACTIVATION_PRIOR_
445445
/**
446446
* Foreground repair the Apple runner performed while serving one command (#2682). `priorState` is
447447
* the session app's state BEFORE the runner activated it, so the fact describes what was repaired
448-
* rather than what the repair produced. `foregroundPid` is present only when exactly one
449-
* application other than the session app had an active accessibility session at that moment: the
450-
* private AX client the runner reads resolves pids and answers no bundle id for an arbitrary app.
448+
* rather than what the repair produced. `otherActiveApplicationPid` is present only when exactly one
449+
* application other than the session app held an active accessibility session at that moment: a
450+
* liveness claim and nothing more, since the private AX client reports no ordering of
451+
* `activeApplications`, resolves pids only, and answers no bundle id for an arbitrary app.
451452
*/
452453
export type IosTargetActivation = Readonly<{
453454
reason: IosTargetActivationReason;
454455
priorState: IosTargetActivationPriorState;
455-
foregroundPid?: number;
456+
otherActiveApplicationPid?: number;
456457
}>;
457458

458459
export type SnapshotState = {

‎src/commands/capture/output.test.ts‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { test } from 'vitest';
22
import assert from 'node:assert/strict';
33
import { attachRefs, type RawSnapshotNode } from '@agent-device/kernel/snapshot';
44
import type { CaptureSnapshotResult } from '@agent-device/contracts/client';
5+
import { iosTargetActivationDisclosure } from '@agent-device/contracts/ios-target-activation';
56
import { snapshotCliOutput } from './output.ts';
67

78
function buildResult(raw: RawSnapshotNode[]): CaptureSnapshotResult {
@@ -101,21 +102,16 @@ test('snapshot output presents the materialized fallback screenshot path', async
101102
* never has to parse the sentence.
102103
*/
103104
test('a repaired capture renders the disclosure and keeps the structured fact', async () => {
105+
const fact = {
106+
reason: 'stale_target',
107+
priorState: 'runningBackground',
108+
otherActiveApplicationPid: 4562,
109+
} as const;
104110
const output = await snapshotCliOutput({
105111
result: {
106112
...buildResult(REPEATED_CHAIN),
107-
warnings: [
108-
'The session app was not foreground when this command arrived (another app (pid 4562) held ' +
109-
'it, prior state runningBackground), so the runner activated it before answering (reason ' +
110-
'stale_target). Any capture taken earlier in this session described another app (pid 4562), ' +
111-
'not the session app. Re-capture now that the session app answers, or drive the other app ' +
112-
'in its own session.',
113-
],
114-
targetActivation: {
115-
reason: 'stale_target',
116-
priorState: 'runningBackground',
117-
foregroundPid: 4562,
118-
},
113+
warnings: [iosTargetActivationDisclosure(fact)],
114+
targetActivation: fact,
119115
} as CaptureSnapshotResult,
120116
});
121117

@@ -124,6 +120,6 @@ test('a repaired capture renders the disclosure and keeps the structured fact',
124120
assert.deepEqual((output.jsonData as { targetActivation?: unknown }).targetActivation, {
125121
reason: 'stale_target',
126122
priorState: 'runningBackground',
127-
foregroundPid: 4562,
123+
otherActiveApplicationPid: 4562,
128124
});
129125
});

‎src/daemon/__tests__/capture-disclosure-target-activation.test.ts‎

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import type { DaemonResponse } from '../daemon-request.ts';
88
const FACT: IosTargetActivation = {
99
reason: 'stale_target',
1010
priorState: 'runningBackground',
11-
foregroundPid: 4562,
11+
otherActiveApplicationPid: 4562,
1212
};
1313

1414
function okResponse(data: Record<string, unknown>): DaemonResponse {
@@ -63,15 +63,39 @@ test('a capture with no repair leaves the response byte-identical', () => {
6363
assert.deepEqual(response, untouched);
6464
});
6565

66+
/**
67+
* A failure carries the sentence in `error.details.hint`. That carrier is exactly where a route could
68+
* cheaply borrow the previous command's repair off the stored snapshot and blame it on this request,
69+
* so the repair travels only on the request's own proof (#2682).
70+
*/
71+
test('a failure does not borrow a repair the stored snapshot happens to carry', () => {
72+
const failed: DaemonResponse = {
73+
ok: false,
74+
error: { code: 'COMMAND_FAILED', message: 'snapshot failed' } as never,
75+
};
76+
77+
const response = withCaptureDisclosures({
78+
response: failed,
79+
consumedTree: { targetActivation: FACT },
80+
activationProof: {},
81+
});
82+
83+
assert.equal(response, failed);
84+
});
85+
6686
/**
6787
* The two carriers are pre-existing: the surface disclosure extends `data.warning`, the capture's
6888
* own warnings arrive as `data.warnings` (#2438 predates the array). A response can therefore carry
6989
* both, and each disclosure must survive in the carrier it belongs to.
7090
*/
7191
test('surface and foreground disclosures ride one response together', () => {
72-
const response = withCaptureDisclosures(okResponse({ nodes: [] }), {
73-
iosSystemSurfaceBundleId: 'com.apple.SafariViewService',
74-
targetActivation: FACT,
92+
const response = withCaptureDisclosures({
93+
response: okResponse({ nodes: [] }),
94+
consumedTree: {
95+
iosSystemSurfaceBundleId: 'com.apple.SafariViewService',
96+
targetActivation: FACT,
97+
},
98+
activationProof: { state: { targetActivation: FACT } },
7599
});
76100
const data = dataOf(response);
77101
assert.match(String(data.warning), /system web sign-in sheet/);

‎src/daemon/__tests__/selector-capture-runtime.test.ts‎

Lines changed: 120 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@ import type {
33
CaptureSnapshotInput,
44
SnapshotResult,
55
} from '@agent-device/contracts/snapshot-runtime';
6-
import { buildSnapshotPresentationKey } from '@agent-device/kernel/snapshot';
6+
import { buildSnapshotPresentationKey, type SnapshotState } from '@agent-device/kernel/snapshot';
7+
import type { IosTargetActivation } from '@agent-device/contracts/ios-target-activation';
8+
import type { DaemonResponse } from '../daemon-request.ts';
9+
import { type RequestActivationProof, withCaptureDisclosures } from '../capture-disclosure.ts';
710
import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts';
811
import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts';
912
import { createSelectorCaptureRuntime } from '../selector-capture-runtime.ts';
@@ -191,6 +194,122 @@ test('sparse verdict recovery retries with query scope and stores recovered snap
191194
});
192195
});
193196

197+
const REPAIR: IosTargetActivation = {
198+
reason: 'stale_target',
199+
priorState: 'runningBackground',
200+
otherActiveApplicationPid: 4562,
201+
};
202+
203+
function proofRuntime(params: {
204+
sessionName: string;
205+
storedSnapshot?: SnapshotState;
206+
capturedTargetActivation?: IosTargetActivation;
207+
}) {
208+
const sessionStore = makeSessionStore('agent-device-selector-capture-');
209+
const session = makeIosSession(
210+
params.sessionName,
211+
params.storedSnapshot ? { snapshot: params.storedSnapshot } : {},
212+
);
213+
sessionStore.set(params.sessionName, session);
214+
boundCapture.mockResolvedValue({
215+
backend: 'xctest',
216+
producer: 'apple-runner',
217+
nodes: [{ index: 0, type: 'Button', label: 'Captured' }],
218+
...(params.capturedTargetActivation
219+
? { targetActivation: params.capturedTargetActivation }
220+
: {}),
221+
} as never);
222+
const consumedSnapshot: { state?: SnapshotState } = {};
223+
const activationProof: RequestActivationProof = {};
224+
const runtime = createSelectorCaptureRuntime({
225+
device: session.device,
226+
session,
227+
sessionStore,
228+
sessionName: params.sessionName,
229+
consumedSnapshot,
230+
activationProof,
231+
capture: boundCapture,
232+
req: {
233+
token: 't',
234+
session: params.sessionName,
235+
command: 'get',
236+
positionals: [],
237+
flags: {},
238+
},
239+
});
240+
return { runtime, consumedSnapshot, activationProof };
241+
}
242+
243+
/**
244+
* A selector read answered from the tree an earlier command stored performed no device work, so it
245+
* owns no foreground repair (#2682). The stored tree may still be disclosed as the surface the
246+
* response describes (#2438); the repair sentence is a claim about THIS command and would be
247+
* fabricated here.
248+
*/
249+
test('a session-snapshot cache hit consumes a repaired tree without earning the repair proof', async () => {
250+
const holders = proofRuntime({
251+
sessionName: 'selector-cache-hit-repair-proof',
252+
storedSnapshot: {
253+
createdAt: Date.now(),
254+
presentationKey: buildSnapshotPresentationKey({}),
255+
nodes: [{ ref: 'e1', index: 0, type: 'Button', label: 'Stored' }],
256+
targetActivation: REPAIR,
257+
},
258+
});
259+
260+
await holders.runtime.capture({ flags: {}, cache: { useSessionSnapshot: true } });
261+
262+
expect(boundCapture).not.toHaveBeenCalled();
263+
expect(holders.consumedSnapshot.state?.targetActivation).toEqual(REPAIR);
264+
expect(holders.activationProof.state).toBeUndefined();
265+
266+
const response = withCaptureDisclosures({
267+
response: { ok: true, data: { nodes: [] } } as DaemonResponse,
268+
consumedTree: holders.consumedSnapshot.state,
269+
activationProof: holders.activationProof,
270+
});
271+
expect(response.ok).toBe(true);
272+
if (response.ok) {
273+
expect(response.data?.targetActivation).toBeUndefined();
274+
expect(response.data?.warnings).toBeUndefined();
275+
}
276+
});
277+
278+
test('a capture the request took itself earns the repair proof', async () => {
279+
const holders = proofRuntime({
280+
sessionName: 'selector-fresh-capture-repair-proof',
281+
capturedTargetActivation: REPAIR,
282+
});
283+
284+
await holders.runtime.capture({ flags: {}, cache: { useSessionSnapshot: true } });
285+
286+
expect(boundCapture).toHaveBeenCalledTimes(1);
287+
expect(holders.activationProof.state?.targetActivation).toEqual(REPAIR);
288+
});
289+
290+
/**
291+
* A poll or a recovery re-capture inside one request can answer from an already-foreground app while
292+
* an earlier capture in that same request reported the repair. The later fact-less tree must not
293+
* erase the disclosure the request earned (#2682).
294+
*/
295+
test('a later fact-less capture does not erase an earlier repair proof', async () => {
296+
const holders = proofRuntime({
297+
sessionName: 'selector-second-capture-keeps-proof',
298+
capturedTargetActivation: REPAIR,
299+
});
300+
301+
await holders.runtime.capture({ flags: {}, cache: { forceFresh: true } });
302+
boundCapture.mockResolvedValue({
303+
backend: 'xctest',
304+
producer: 'apple-runner',
305+
nodes: [{ index: 0, type: 'Button', label: 'Quiet' }],
306+
} as never);
307+
await holders.runtime.capture({ flags: {}, cache: { forceFresh: true } });
308+
309+
expect(boundCapture).toHaveBeenCalledTimes(2);
310+
expect(holders.activationProof.state?.targetActivation).toEqual(REPAIR);
311+
});
312+
194313
function makeCaptureRuntime(sessionName: string) {
195314
const sessionStore = makeSessionStore('agent-device-selector-capture-');
196315
const session = makeIosSession(sessionName);
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
import path from 'node:path';
2+
import { beforeEach, expect, test, vi } from 'vitest';
3+
import { iosTargetActivationDisclosure } from '@agent-device/contracts/ios-target-activation';
4+
import type { IosTargetActivation } from '@agent-device/kernel/snapshot';
5+
import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts';
6+
import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts';
7+
import { SessionStore } from '../session-store.ts';
8+
import { handleSnapshotCommands } from '../handlers/snapshot.ts';
9+
import { legacyDispatchCapture } from './legacy-snapshot-capture-fixture.ts';
10+
import { snapshotRuntimeFixture } from './snapshot-runtime-fixture.ts';
11+
12+
vi.mock('../snapshot-interactor-capture.ts', async () => {
13+
const fixture = await import('./legacy-snapshot-capture-fixture.ts');
14+
return { captureSnapshotWithInteractor: fixture.captureSnapshotThroughLegacyDispatchFixture };
15+
});
16+
17+
const REPAIR: IosTargetActivation = {
18+
reason: 'stale_target',
19+
priorState: 'runningBackground',
20+
otherActiveApplicationPid: 4562,
21+
};
22+
23+
beforeEach(() => {
24+
legacyDispatchCapture.mockReset();
25+
});
26+
27+
test('a snapshot that captured a repaired tree discloses the repair it paid for', async () => {
28+
const input = scenario({});
29+
legacyDispatchCapture.mockResolvedValue({
30+
backend: 'xctest',
31+
truncated: false,
32+
targetActivation: REPAIR,
33+
nodes: [
34+
{
35+
index: 0,
36+
depth: 0,
37+
type: 'Button',
38+
label: 'Continue',
39+
rect: { x: 0, y: 0, width: 100, height: 44 },
40+
hittable: true,
41+
},
42+
],
43+
});
44+
45+
const response = await dispatchSnapshot(input);
46+
47+
expect(response.ok).toBe(true);
48+
if (response.ok) {
49+
expect(response.data?.targetActivation).toEqual(REPAIR);
50+
expect(response.data?.warnings).toContain(iosTargetActivationDisclosure(REPAIR));
51+
}
52+
});
53+
54+
function scenario(params: { storedRepair?: boolean }) {
55+
const root = mkdtempForTestSync('agent-device-snapshot-target-activation');
56+
const sessionName = 'default';
57+
const sessionStore = new SessionStore(path.join(root, 'sessions'));
58+
const session = makeIosSession(sessionName, { appBundleId: 'com.example.app' });
59+
if (params.storedRepair) {
60+
session.snapshot = {
61+
createdAt: Date.now(),
62+
nodes: [{ ref: 'e1', index: 0, type: 'Button', label: 'Earlier' }],
63+
targetActivation: REPAIR,
64+
};
65+
}
66+
sessionStore.set(sessionName, session);
67+
return { sessionStore, sessionName, logPath: path.join(root, 'daemon.log') };
68+
}
69+
70+
async function dispatchSnapshot(input: ReturnType<typeof scenario>) {
71+
const response = await handleSnapshotCommands({
72+
req: { command: 'snapshot', positionals: [], token: 't', session: input.sessionName },
73+
sessionName: input.sessionName,
74+
logPath: input.logPath,
75+
sessionStore: input.sessionStore,
76+
...snapshotRuntimeFixture(),
77+
});
78+
if (!response) throw new Error('snapshot route did not answer');
79+
return response;
80+
}

‎src/daemon/capture-disclosure.ts‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,20 @@ export type CaptureProvenance = Pick<
1010
'systemSurfaceOnly' | 'iosSystemSurfaceBundleId' | 'targetActivation'
1111
>;
1212

13+
/**
14+
* The foreground repair THIS request paid for, filled by the capture path only when the request
15+
* actually captured a tree (#2682).
16+
*
17+
* Separate from the consumed tree on purpose. A selector read may answer from a cached or stored
18+
* tree — that tree still describes the surface the response is about, which is what #2438 discloses
19+
* — but a cache hit performed no device work, so it can own no repair. Stamping the repair off the
20+
* consumed tree would tell a command "you found the session app out of foreground" when it never
21+
* looked, which is a fabricated observation rather than a disclosure.
22+
*/
23+
export type RequestActivationProof = {
24+
state?: CaptureProvenance;
25+
};
26+
1327
/**
1428
* Append the occluding-system-surface disclosure to a selector-route response whose consumed
1529
* snapshot was a system surface: an Android notification shade / quick settings, or an iOS in-place
@@ -51,12 +65,22 @@ export function withTargetActivationDisclosure(
5165
return { ...response, data };
5266
}
5367

54-
/** Every capture-provenance disclosure a consumed tree owes its response (#2438, #2682). */
55-
export function withCaptureDisclosures(
56-
response: DaemonResponse,
57-
snapshot: CaptureProvenance | undefined,
58-
): DaemonResponse {
59-
return withTargetActivationDisclosure(withSystemSurfaceDisclosure(response, snapshot), snapshot);
68+
/**
69+
* Every capture-provenance disclosure a response owes, from the two different things a capture can
70+
* prove: what the answered tree describes (#2438 — cache tiers included, because the surface is
71+
* still on screen) and what this request's own capture found (#2682 — cache hits excluded, because
72+
* a request that captured nothing repaired nothing).
73+
*/
74+
export function withCaptureDisclosures(params: {
75+
response: DaemonResponse;
76+
consumedTree: CaptureProvenance | undefined;
77+
activationProof?: RequestActivationProof;
78+
}): DaemonResponse {
79+
const { response, consumedTree, activationProof } = params;
80+
return withTargetActivationDisclosure(
81+
withSystemSurfaceDisclosure(response, consumedTree),
82+
activationProof?.state,
83+
);
6084
}
6185

6286
function responseWarnings(value: unknown): string[] {

‎src/daemon/interaction/internal/__tests__/interaction-target-activation-disclosure.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { contextFromFlags, makeSession } from './interaction-touch-fixtures.ts';
1212
const FACT: IosTargetActivation = {
1313
reason: 'stale_target',
1414
priorState: 'runningBackground',
15-
foregroundPid: 4562,
15+
otherActiveApplicationPid: 4562,
1616
};
1717

1818
function capturedTree(params: {

0 commit comments

Comments
 (0)