Skip to content

Commit 64c50e2

Browse files
author
agent
committed
fix(get): admit before the direct-iOS fast path; close the element-read outcome
Review blockers on #1877. 1. `dispatchGetViaRuntime` could complete the direct-iOS selector query before `resolveBoundGetRuntime`. Once `get` declares `device-runtime`, ADR 0019 requires resolve -> admit -> bind before anything in the request path operates, so admission now runs first for every target shape and the fast path is a fast path *within* an admitted request. Regression: an eligible direct selector cannot operate when facts refuse admission. 2. `readTextAtPoint` returned `Promise<string>` and `readTextForNode` caught any throw and fell back, assigning a typed diagnostic after an untyped failure. It now returns a closed `ElementTextReadOutcome`; fallback happens only for the contract's classified reasons; unexpected errors propagate. The reason union is derived from its runtime list so the two cannot drift, and an unhandled reason is a compile error at the consumer. This retires the generic catch the start record promised.
1 parent 693de9d commit 64c50e2

10 files changed

Lines changed: 292 additions & 82 deletions
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
import assert from 'node:assert/strict';
2+
import { test } from 'vitest';
3+
import {
4+
elementTextRead,
5+
type ElementTextReadOutcome,
6+
type ElementTextUnreadableReason,
7+
} from './element-text-runtime.ts';
8+
9+
/**
10+
* The reasons this suite exercises. Kept local on purpose: exhaustiveness is enforced at the
11+
* CONSUMER by `classifiedFallbackReason`'s `never` arm (a new reason is a compile error there),
12+
* so a second exported runtime list would be an unconsumed parallel source of truth that could
13+
* silently drift. The annotation is what ties this list back to the union.
14+
*/
15+
const UNREADABLE_REASONS: readonly ElementTextUnreadableReason[] = [
16+
'no-text-at-point',
17+
'surface-not-readable',
18+
];
19+
20+
/**
21+
* ADR 0019 §2 contract coverage for the preferred element-text read.
22+
*
23+
* A preferred operation may fall its consumer back to the required path only through a TYPED
24+
* reason. These tests pin that the reason set is closed and exhaustively enumerated, so a new
25+
* reason cannot be added without a consumer having to classify it — which is what keeps the
26+
* retired generic `catch` from creeping back as "some other failure, just fall back".
27+
*/
28+
29+
test('the outcome union is closed: every value is a read or a classified unreadable', () => {
30+
const outcomes: readonly ElementTextReadOutcome[] = [
31+
elementTextRead('live value'),
32+
...UNREADABLE_REASONS.map((reason) => ({ status: 'unreadable', reason }) as const),
33+
];
34+
for (const outcome of outcomes) {
35+
if (outcome.status === 'read') {
36+
assert.equal(typeof outcome.text, 'string');
37+
continue;
38+
}
39+
assert.ok(
40+
(UNREADABLE_REASONS as readonly string[]).includes(outcome.reason),
41+
`unreadable outcome carries an unclassified reason: ${outcome.reason}`,
42+
);
43+
}
44+
});
45+
46+
test('a non-blank owner answer is a read that preserves the exact text', () => {
47+
const outcome = elementTextRead(' padded value ');
48+
assert.deepEqual(outcome, { status: 'read', text: ' padded value ' });
49+
});
50+
51+
// Blank is a classification, not a read: an owner answering with whitespace has said there is
52+
// nothing at this point, and saying so by reason keeps consumers off "empty or failed?" guesswork.
53+
for (const [label, value] of [
54+
['empty string', ''],
55+
['whitespace', ' \n\t '],
56+
['undefined', undefined],
57+
['null', null],
58+
] as const) {
59+
test(`a ${label} owner answer classifies as no-text-at-point`, () => {
60+
assert.deepEqual(elementTextRead(value), {
61+
status: 'unreadable',
62+
reason: 'no-text-at-point',
63+
});
64+
});
65+
}
66+
67+
test('read outcomes are frozen so a consumer cannot mutate a classification', () => {
68+
assert.ok(Object.isFrozen(elementTextRead('value')));
69+
assert.ok(Object.isFrozen(elementTextRead('')));
70+
});

packages/contracts/src/element-text-runtime.ts

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,48 @@ export type ReadTextAtPointInput = Readonly<{
1717
execution?: ElementTextRuntimeExecution;
1818
}>;
1919

20+
/**
21+
* Why an owner that HAS a live read still produced no text for this point.
22+
*
23+
* Closed on purpose (ADR 0019 §2): a consumer may fall back to the required path only for a
24+
* reason named here. Anything else — a runner transport failure, a helper crash, a bug — is an
25+
* unexpected error and propagates, because silently answering from a stale captured tree after
26+
* an unclassified failure is exactly the "generic catch fallback" the ADR forbids.
27+
*/
28+
export type ElementTextUnreadableReason =
29+
/** The owner queried successfully and there is nothing readable at this point. */
30+
| 'no-text-at-point'
31+
/** The owner's read surface exists but declined this query (unsupported element/surface). */
32+
| 'surface-not-readable';
33+
34+
/** The closed outcome of one live element-text read. */
35+
export type ElementTextReadOutcome =
36+
| Readonly<{ status: 'read'; text: string }>
37+
| Readonly<{ status: 'unreadable'; reason: ElementTextUnreadableReason }>;
38+
39+
/**
40+
* Normalizes a raw owner read into the closed outcome. Blank text is not a read: an owner that
41+
* answers with whitespace has told us there is nothing at this point, and saying so by reason
42+
* keeps every consumer off "did it fail or is it empty?" guesswork.
43+
*/
44+
export function elementTextRead(text: string | undefined | null): ElementTextReadOutcome {
45+
if (typeof text !== 'string' || text.trim().length === 0) {
46+
return Object.freeze({ status: 'unreadable', reason: 'no-text-at-point' } as const);
47+
}
48+
return Object.freeze({ status: 'read', text } as const);
49+
}
50+
2051
export type ElementTextRuntimeOperations = Readonly<{
2152
/**
2253
* The live text an owner reads at a point, which can exceed the readable text carried by an
2354
* already-captured snapshot node (an editable field whose value is longer than its label).
2455
* Declared `preferred`, never `required`: every consumer's required path answers from the
2556
* snapshot tree, so an owner without this operation still executes the command completely.
57+
*
58+
* Returns a closed typed outcome rather than a bare string, so a consumer never has to
59+
* distinguish "no text here" from "the read blew up" by catching.
2660
*/
27-
readTextAtPoint(input: ReadTextAtPointInput): Promise<string>;
61+
readTextAtPoint(input: ReadTextAtPointInput): Promise<ElementTextReadOutcome>;
2862
}>;
2963

3064
export type ElementTextRuntimeOperationFacts = Readonly<{
@@ -43,7 +77,7 @@ export function elementTextRuntimeOperationFacts(
4377
* interactor-resolver seam.
4478
*/
4579
export type ElementTextRuntimeHost = Readonly<{
46-
readTextAtPoint(device: DeviceInfo, input: ReadTextAtPointInput): Promise<string>;
80+
readTextAtPoint(device: DeviceInfo, input: ReadTextAtPointInput): Promise<ElementTextReadOutcome>;
4781
}>;
4882

4983
/** Captures one selected owner's read authority for the lifetime of a request binding. */

packages/contracts/src/facades/platform.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,13 +263,16 @@ export type {
263263
} from '../viewport-runtime.ts';
264264
export {
265265
bindElementTextRuntime,
266+
elementTextRead,
266267
elementTextRuntimeOperationFacts,
267268
} from '../element-text-runtime.ts';
268269
export type {
270+
ElementTextReadOutcome,
269271
ElementTextRuntimeExecution,
270272
ElementTextRuntimeHost,
271273
ElementTextRuntimeOperationFacts,
272274
ElementTextRuntimeOperations,
275+
ElementTextUnreadableReason,
273276
ReadTextAtPointInput,
274277
} from '../element-text-runtime.ts';
275278
export type {

src/daemon/handlers/__tests__/interaction-get-runtime-fixture.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
type CaptureSnapshotInput,
88
type DeviceBinding,
99
type PlatformRuntimeOperations,
10+
type ElementTextReadOutcome,
1011
type ReadTextAtPointInput,
1112
type RuntimeFacts,
1213
} from '@agent-device/contracts/platform';
@@ -22,15 +23,27 @@ import { captureSnapshotWithInteractor } from '../snapshot-interactor-capture.ts
2223
* at `core/dispatch.ts`. The bound capture still runs the interactor capture the surrounding
2324
* interaction tests already mock, so only the two `get` operations are fixture-owned here.
2425
*/
25-
export const mockReadTextAtPoint = vi.fn(async (_input: ReadTextAtPointInput) => '');
26+
export const mockReadTextAtPoint = vi.fn(
27+
async (_input: ReadTextAtPointInput): Promise<ElementTextReadOutcome> =>
28+
Object.freeze({ status: 'unreadable', reason: 'no-text-at-point' } as const),
29+
);
2630

27-
/** Flip to model an owner whose facts advertise no live element read (web, HarmonyOS, provider). */
28-
export const elementReadFixtureState = { readTextAtPointAvailable: true };
31+
/**
32+
* Flip to model an exact owner cell: no live element read (web, HarmonyOS, provider), or no
33+
* capture at all (the watchOS sentinel, an inactive provider), which refuses admission outright.
34+
*/
35+
export const elementReadFixtureState = {
36+
readTextAtPointAvailable: true,
37+
captureSnapshotAvailable: true,
38+
};
2939

3040
export function resetGetRuntimeFixture(): void {
3141
mockReadTextAtPoint.mockReset();
32-
mockReadTextAtPoint.mockResolvedValue('');
42+
mockReadTextAtPoint.mockResolvedValue(
43+
Object.freeze({ status: 'unreadable', reason: 'no-text-at-point' } as const),
44+
);
3345
elementReadFixtureState.readTextAtPointAvailable = true;
46+
elementReadFixtureState.captureSnapshotAvailable = true;
3447
}
3548

3649
const available = Object.freeze({ available: true } as const);
@@ -61,7 +74,7 @@ function elementReadFacts(device: DeviceInfo): RuntimeFacts<PlatformRuntimeOpera
6174
device: base.device,
6275
operations: {
6376
...base.operations,
64-
captureSnapshot: available,
77+
captureSnapshot: elementReadFixtureState.captureSnapshotAvailable ? available : unavailable,
6578
readTextAtPoint: elementReadFixtureState.readTextAtPointAvailable ? available : unavailable,
6679
},
6780
});

src/daemon/handlers/__tests__/interaction-read.test.ts

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,21 @@
11
import { beforeEach, describe, expect, it, vi } from 'vitest';
22
import type { SnapshotNode } from '@agent-device/kernel/snapshot';
3-
import type { ReadTextAtPointInput } from '@agent-device/contracts/platform';
3+
import {
4+
elementTextRead,
5+
type ElementTextReadOutcome,
6+
type ReadTextAtPointInput,
7+
} from '@agent-device/contracts/platform';
48
import { readTextForNode } from '../interaction-read.ts';
59

610
/**
711
* Bound at the seam the handler consumes (the runtime's `readTextAtPoint` operation), never at
812
* `core/dispatch.ts`: `get` is migrated, so the live read reaches this fake through the request
913
* binding rather than through the legacy dispatcher.
1014
*/
11-
const readTextAtPoint = vi.fn(async (_input: ReadTextAtPointInput) => 'backend-text');
15+
const readTextAtPoint = vi.fn(
16+
async (_input: ReadTextAtPointInput): Promise<ElementTextReadOutcome> =>
17+
elementTextRead('backend-text'),
18+
);
1219

1320
function node(overrides: Partial<SnapshotNode>): SnapshotNode {
1421
return {
@@ -91,21 +98,34 @@ describe('readTextForNode', () => {
9198
expect(readTextAtPoint).not.toHaveBeenCalled();
9299
});
93100

94-
it('falls back to the captured tree through a typed reason when the live read fails', async () => {
95-
readTextAtPoint.mockRejectedValueOnce(new Error('runner transport closed'));
101+
// ADR 0019 §2: the ONLY fallbacks are the contract's classified reasons.
102+
it.each(['no-text-at-point', 'surface-not-readable'] as const)(
103+
'falls back to the captured tree for the classified reason %s',
104+
async (reason) => {
105+
readTextAtPoint.mockResolvedValueOnce({ status: 'unreadable', reason });
106+
const text = await readTextForNode({
107+
...baseParams,
108+
node: node({ type: 'textfield', value: 'snap' }),
109+
});
110+
expect(text).toBe('snap');
111+
},
112+
);
113+
114+
it('classifies a blank live read as no-text-at-point rather than reading blank text', async () => {
115+
readTextAtPoint.mockResolvedValueOnce(elementTextRead(' '));
96116
const text = await readTextForNode({
97117
...baseParams,
98118
node: node({ type: 'textfield', value: 'snap' }),
99119
});
100120
expect(text).toBe('snap');
101121
});
102122

103-
it('falls back to the captured tree when the live read returns blank text', async () => {
104-
readTextAtPoint.mockResolvedValueOnce(' ');
105-
const text = await readTextForNode({
106-
...baseParams,
107-
node: node({ type: 'textfield', value: 'snap' }),
108-
});
109-
expect(text).toBe('snap');
123+
// The retired generic catch: an unclassified failure must NOT become "this element has no
124+
// text". It propagates, so a runner/helper failure can never be answered from a stale tree.
125+
it('propagates an unexpected live-read failure instead of falling back', async () => {
126+
readTextAtPoint.mockRejectedValueOnce(new Error('runner transport closed'));
127+
await expect(
128+
readTextForNode({ ...baseParams, node: node({ type: 'textfield', value: 'snap' }) }),
129+
).rejects.toThrow(/runner transport closed/);
110130
});
111131
});

src/daemon/handlers/__tests__/interaction.test.ts

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ vi.mock('../../../platforms/apple/core/runner/runner-client.ts', async (importOr
5858
};
5959
});
6060

61+
import { elementTextRead } from '@agent-device/contracts/platform';
6162
import {
6263
elementReadFixtureState,
6364
getRuntimeBindings,
@@ -153,7 +154,9 @@ test('get text uses backend read expansion when the resolved node has a rect', a
153154
};
154155
sessionStore.set(sessionName, session);
155156

156-
mockReadTextAtPoint.mockResolvedValue('package com.example.app\nclass MainActivity {}');
157+
mockReadTextAtPoint.mockResolvedValue(
158+
elementTextRead('package com.example.app\nclass MainActivity {}'),
159+
);
157160

158161
const response = await handleInteractionCommands({
159162
req: {
@@ -224,6 +227,42 @@ test('get text answers from the captured tree when the bound owner advertises no
224227
}
225228
});
226229

230+
// ADR 0019 regression: `get` declares `device-runtime`, so an ELIGIBLE direct-iOS selector —
231+
// one the fast path would otherwise answer without a tree capture — must not reach the device
232+
// until the request has resolved, admitted, and bound. A refused admission means zero runner
233+
// queries, not a fast-path answer that skipped exact-owner facts entirely.
234+
test('an eligible direct iOS selector cannot operate before admission', async () => {
235+
const sessionStore = makeSessionStore();
236+
const sessionName = 'get-text-direct-before-admission';
237+
sessionStore.set(sessionName, makeIosSession(sessionName, { appBundleId: 'com.example.app' }));
238+
elementReadFixtureState.captureSnapshotAvailable = false;
239+
mockRunAppleRunnerCommand.mockResolvedValue({
240+
found: true,
241+
text: 'Ada Lovelace',
242+
nodes: [{ index: 0, depth: 0, type: 'StaticText', label: 'Ada Lovelace' }],
243+
});
244+
245+
const response = await handleInteractionCommands({
246+
req: {
247+
token: 't',
248+
session: sessionName,
249+
command: 'get',
250+
positionals: ['text', 'id=name'],
251+
flags: {},
252+
},
253+
sessionName,
254+
sessionStore,
255+
contextFromFlags,
256+
...getRuntimeBindings(),
257+
});
258+
259+
expect(response?.ok).toBe(false);
260+
if (response && !response.ok) expect(response.error.code).toBe('UNSUPPORTED_OPERATION');
261+
// The whole point: the fast path never ran.
262+
expect(mockRunAppleRunnerCommand).not.toHaveBeenCalled();
263+
expect(mockDispatch).not.toHaveBeenCalled();
264+
});
265+
227266
test('get text simple iOS id selector uses runner query without snapshot', async () => {
228267
const sessionStore = makeSessionStore();
229268
const sessionName = 'get-text-ios-direct-selector';

src/daemon/handlers/interaction-read-legacy-dispatch.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { dispatchCommand } from '../../core/dispatch.ts';
22
import type { SessionState } from '../types.ts';
33
import type { ContextFromFlags } from './interaction-common.ts';
44
import type { CommandFlags } from '@agent-device/contracts/command';
5+
import { elementTextRead } from '@agent-device/contracts/platform';
56
import type { ReadElementTextAtPoint } from './interaction-read.ts';
67

78
/**
@@ -39,6 +40,6 @@ export function legacyDispatchReadTextAtPoint(params: {
3940
},
4041
);
4142
const data = rawData && typeof rawData === 'object' ? rawData : undefined;
42-
return typeof data?.text === 'string' ? data.text : '';
43+
return elementTextRead(typeof data?.text === 'string' ? data.text : undefined);
4344
};
4445
}

0 commit comments

Comments
 (0)