Skip to content

Commit 8b4c133

Browse files
committed
fix: reject selector-shaped wait arguments instead of reading them as text
`wait <condition> '<selector>' [timeoutMs]` (e.g. `wait open 'label="Open"' 25000`, `wait exists 'label="x"' 100`) and any unrecognized `key=value` token used to fall through parseWaitPositionals' text fallback and wait out the full timeout for literal text that could never appear on screen — reading as a false "element absent" instead of the caller's own argument mistake (#1035 is the sibling fix for click/press/fill/get). parseWaitPositionals now returns a typed `invalid` variant whenever a positional token is selector-shaped (a recognized key, or an unrecognized key=value) but the list doesn't form a valid selector expression, or a valid selector prefix is followed by unquoted trailing tokens. The message names the offending token, points condition words (exists/present/appears/gone/disappears) at the selector form, and always offers the explicit `wait text '<text>'` escape hatch. Bare text (single- and multi-word) and the explicit `text` keyword form are unaffected. Excluding `invalid` from the type consumed by selector-runtime's toWaitTarget makes the remaining kind-by-kind narrowing exhaustive without a runtime fallback branch.
1 parent 3651047 commit 8b4c133

6 files changed

Lines changed: 349 additions & 15 deletions

File tree

src/commands/capture/wait.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,9 @@ function readWaitOptionsFromPositionals(
113113
...readTimeoutOption(parsed.timeoutMs),
114114
};
115115
}
116+
if (parsed.kind === 'invalid') {
117+
throw new AppError('INVALID_ARGS', parsed.message);
118+
}
116119
return {
117120
...base,
118121
selector: parsed.selectorExpression,

src/core/wait-positionals.test.ts

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
/**
2+
* #1800: a positional list shaped like a selector (a recognized key, or an
3+
* unrecognized `key=value`) must never silently degrade to a `text` wait —
4+
* that degradation is exactly the shape that reads as "element absent" after
5+
* a full timeout instead of the caller's own argument mistake. See #1035 for
6+
* the sibling fix on click/press/fill/get.
7+
*/
8+
import assert from 'node:assert/strict';
9+
import fc from 'fast-check';
10+
import { test } from 'vitest';
11+
import { isValidSelectorExpression, SELECTOR_KEY_NAMES } from '@agent-device/selectors';
12+
import { PROPERTY_RUNS } from '../__tests__/test-utils/index.ts';
13+
import { parseWaitPositionals, resolveWaitBudgetMs } from './wait-positionals.ts';
14+
15+
function assertInvalid(args: string[], messageFragment: string) {
16+
const result = parseWaitPositionals(args);
17+
assert.ok(result, `expected a result for ${JSON.stringify(args)}`);
18+
assert.equal(result.kind, 'invalid');
19+
if (result.kind !== 'invalid') return;
20+
assert.ok(
21+
result.message.includes(messageFragment),
22+
`expected message to include ${JSON.stringify(messageFragment)}, got ${JSON.stringify(result.message)}`,
23+
);
24+
}
25+
26+
// --- the issue's reproduction shapes -----------------------------------
27+
28+
test('a condition word followed by a selector-shaped value is rejected, not read as text (#1800 repro)', () => {
29+
const result = parseWaitPositionals(['open', 'label="Open"', '25000']);
30+
assert.ok(result);
31+
assert.notEqual(result.kind, 'text');
32+
assertInvalid(['open', 'label="Open"', '25000'], 'label="Open"');
33+
});
34+
35+
test('"exists" followed by a selector-shaped value is rejected and points at the selector form', () => {
36+
assertInvalid(['exists', 'label="x"', '100'], "wait 'visible");
37+
});
38+
39+
test('unknown selector key is rejected with the supported-key list, not read as text', () => {
40+
const result = parseWaitPositionals(['lbl="x"', '100']);
41+
assert.ok(result);
42+
assert.equal(result.kind, 'invalid');
43+
if (result.kind !== 'invalid') return;
44+
assert.equal(result.reason, 'unknown-selector-key');
45+
assert.ok(result.message.includes('lbl'));
46+
assert.ok(result.message.includes('label='));
47+
});
48+
49+
// --- still-valid selector forms are untouched ---------------------------
50+
51+
test('a valid selector still parses as selector', () => {
52+
const result = parseWaitPositionals(['label="Open"', '5000']);
53+
assert.deepEqual(result, {
54+
kind: 'selector',
55+
selectorExpression: 'label="Open"',
56+
timeoutMs: 5000,
57+
});
58+
});
59+
60+
test('"visible" leading a valid selector still parses as selector (boolean key, not a condition word)', () => {
61+
const result = parseWaitPositionals(['visible', 'label="Open"', '25000']);
62+
assert.deepEqual(result, {
63+
kind: 'selector',
64+
selectorExpression: 'visible label="Open"',
65+
timeoutMs: 25000,
66+
});
67+
});
68+
69+
// --- trailing tokens after a valid selector prefix are rejected too -----
70+
71+
test('a valid selector prefix followed by an unquoted extra word is rejected, not merged into text', () => {
72+
assertInvalid(['label="Open"', 'foo', '5000'], 'extra arguments');
73+
});
74+
75+
// --- bare text keeps working ---------------------------------------------
76+
77+
test('bare single-word text still parses as text', () => {
78+
const result = parseWaitPositionals(['Continue', '1500']);
79+
assert.deepEqual(result, { kind: 'text', text: 'Continue', timeoutMs: 1500 });
80+
});
81+
82+
test('bare multi-word text still parses as text', () => {
83+
const result = parseWaitPositionals(['Sign', 'in', '2000']);
84+
assert.deepEqual(result, { kind: 'text', text: 'Sign in', timeoutMs: 2000 });
85+
});
86+
87+
test('explicit "text" keyword form bypasses selector-shape rejection entirely', () => {
88+
// Even though "label=Open" is selector-shaped, the explicit `text` keyword is the caller's
89+
// unambiguous escape hatch and must always win.
90+
const result = parseWaitPositionals(['text', 'label=Open', '5000']);
91+
assert.deepEqual(result, { kind: 'text', text: 'label=Open', timeoutMs: 5000 });
92+
});
93+
94+
test('free prose containing a bare "=" with no key stays literal text', () => {
95+
const result = parseWaitPositionals(['Total', '=', '5', '3000']);
96+
assert.deepEqual(result, { kind: 'text', text: 'Total = 5', timeoutMs: 3000 });
97+
});
98+
99+
// --- documented boundary: a bare selector-key word alone is rejected, not text ---
100+
101+
test('a single word that IS a recognized selector key is rejected rather than read as literal text', () => {
102+
// "id" alone cannot become a valid selector (it needs a value), but it also must not silently
103+
// become literal wait text — an agent that meant the word "id" has to say so via `wait text`.
104+
assertInvalid(['id', '3000'], "wait text 'id'");
105+
});
106+
107+
// --- resolveWaitBudgetMs stays sane for the new variant -------------------
108+
109+
test('resolveWaitBudgetMs returns null for a rejected selector-shaped positional list', () => {
110+
assert.equal(resolveWaitBudgetMs(['open', 'label="Open"', '25000']), null);
111+
});
112+
113+
// --- properties over examples ---------------------------------------------
114+
115+
const TEXT_SELECTOR_KEYS = SELECTOR_KEY_NAMES.filter((key) => !isValidSelectorExpression(key));
116+
const BOOLEAN_SELECTOR_KEYS = SELECTOR_KEY_NAMES.filter((key) => isValidSelectorExpression(key));
117+
118+
const selectorValueArb = fc.oneof(
119+
fc.constantFrom('Open', 'Sign in', "it's", 'a || b', 'key=value'),
120+
fc.string({ minLength: 1, maxLength: 8 }).filter((value) => value.trim().length > 0),
121+
);
122+
123+
/** One token of a generated, guaranteed-valid selector expression. */
124+
const selectorTokenArb: fc.Arbitrary<string> = fc.oneof(
125+
fc
126+
.record({ key: fc.constantFrom(...TEXT_SELECTOR_KEYS), value: selectorValueArb })
127+
.map(({ key, value }) => `${key}=${JSON.stringify(value)}`),
128+
fc.constantFrom(...BOOLEAN_SELECTOR_KEYS),
129+
);
130+
131+
const validSelectorTokensArb: fc.Arbitrary<string[]> = fc.array(selectorTokenArb, {
132+
minLength: 1,
133+
maxLength: 3,
134+
});
135+
136+
test('property: a generated valid selector expression never parses as text or invalid', () => {
137+
fc.assert(
138+
fc.property(validSelectorTokensArb, (tokens) => {
139+
const result = parseWaitPositionals(tokens);
140+
assert.ok(result);
141+
assert.equal(result.kind, 'selector');
142+
}),
143+
{ numRuns: PROPERTY_RUNS },
144+
);
145+
});
146+
147+
const plainWordArb = fc
148+
.string({ minLength: 1, maxLength: 6 })
149+
.filter(
150+
(token) =>
151+
token.trim().length > 0 &&
152+
!token.includes('=') &&
153+
!token.startsWith('@') &&
154+
token !== 'text' &&
155+
token !== 'stable' &&
156+
Number.isNaN(Number(token)),
157+
);
158+
159+
const recognizedKeyValueTokenArb: fc.Arbitrary<string> = fc
160+
.record({ key: fc.constantFrom(...SELECTOR_KEY_NAMES), value: selectorValueArb })
161+
.map(({ key, value }) => `${key}=${JSON.stringify(value)}`);
162+
163+
const unrecognizedKeyValueTokenArb: fc.Arbitrary<string> = fc
164+
.record({
165+
key: fc
166+
.string({ minLength: 1, maxLength: 8 })
167+
.filter(
168+
(key) =>
169+
/^[a-zA-Z][a-zA-Z0-9]*$/.test(key) &&
170+
!SELECTOR_KEY_NAMES.includes(key.toLowerCase() as (typeof SELECTOR_KEY_NAMES)[number]),
171+
),
172+
value: selectorValueArb,
173+
})
174+
.map(({ key, value }) => `${key}=${JSON.stringify(value)}`);
175+
176+
const keyValueShapedTokenArb = fc.oneof(recognizedKeyValueTokenArb, unrecognizedKeyValueTokenArb);
177+
178+
test('property: any positional list containing a key=value-shaped token never parses as text', () => {
179+
fc.assert(
180+
fc.property(
181+
fc.array(plainWordArb, { maxLength: 3 }),
182+
keyValueShapedTokenArb,
183+
fc.nat({ max: 3 }),
184+
(plainTokens, kvToken, insertAtRaw) => {
185+
const insertAt = Math.min(insertAtRaw, plainTokens.length);
186+
const tokens = [...plainTokens.slice(0, insertAt), kvToken, ...plainTokens.slice(insertAt)];
187+
const result = parseWaitPositionals(tokens);
188+
assert.ok(result);
189+
assert.notEqual(result.kind, 'text');
190+
},
191+
),
192+
{ numRuns: PROPERTY_RUNS },
193+
);
194+
});

src/core/wait-positionals.ts

Lines changed: 126 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,152 @@
11
import { parseTimeout } from '../utils/parse-timeout.ts';
2-
import { isValidSelectorExpression, splitSelectorFromArgs } from '@agent-device/selectors';
2+
import {
3+
detectUnknownSelectorKeyToken,
4+
isRoleHintWord,
5+
isSelectorToken,
6+
isValidSelectorExpression,
7+
SELECTOR_KEY_NAMES,
8+
splitSelectorFromArgs,
9+
} from '@agent-device/selectors';
10+
11+
export type WaitInvalidReason = 'unknown-selector-key' | 'selector-shaped-text';
312

413
export type WaitParsed =
514
| { kind: 'sleep'; durationMs: number }
615
| { kind: 'ref'; rawRef: string; timeoutMs: number | null }
716
| { kind: 'selector'; selectorExpression: string; timeoutMs: number | null }
817
| { kind: 'text'; text: string; timeoutMs: number | null }
9-
| { kind: 'stable'; quietMs: number | null; timeoutMs: number | null };
18+
| { kind: 'stable'; quietMs: number | null; timeoutMs: number | null }
19+
| { kind: 'invalid'; reason: WaitInvalidReason; message: string };
20+
21+
// Words that name a wait CONDITION rather than a selector key. A caller leading with one of these
22+
// almost certainly wants the selector form — `visible`/`hidden` are already selector keys, so they
23+
// never reach this list; they parse as valid boolean-key selector terms instead (#1800).
24+
const CONDITION_WORDS = new Set(['exists', 'present', 'appears', 'gone', 'disappears']);
1025

1126
export function parseWaitPositionals(args: string[]): WaitParsed | null {
1227
const firstArg = args[0];
1328
if (firstArg === undefined) return null;
1429
const sleepMs = parseTimeout(firstArg);
1530
if (sleepMs !== null) return { kind: 'sleep', durationMs: sleepMs };
1631
const timeoutMs = parseTimeout(args[args.length - 1]);
17-
if (firstArg === 'text') {
18-
const text = timeoutMs !== null ? args.slice(1, -1).join(' ') : args.slice(1).join(' ');
19-
return { kind: 'text', text: text.trim(), timeoutMs };
20-
}
21-
if (firstArg === 'stable') {
22-
const rest = args.slice(1);
23-
const stableTimeoutMs = rest.length > 1 ? parseTimeout(rest[1]) : null;
24-
const quietMs = rest.length > 0 ? parseTimeout(rest[0]) : null;
25-
return { kind: 'stable', quietMs, timeoutMs: stableTimeoutMs };
26-
}
32+
if (firstArg === 'text') return parseTextKeyword(args, timeoutMs);
33+
if (firstArg === 'stable') return parseStableKeyword(args);
2734
if (firstArg.startsWith('@')) return { kind: 'ref', rawRef: firstArg, timeoutMs };
35+
return parseSelectorOrText(args, timeoutMs);
36+
}
37+
38+
function parseTextKeyword(args: string[], timeoutMs: number | null): WaitParsed {
39+
const text = timeoutMs !== null ? args.slice(1, -1).join(' ') : args.slice(1).join(' ');
40+
return { kind: 'text', text: text.trim(), timeoutMs };
41+
}
42+
43+
function parseStableKeyword(args: string[]): WaitParsed {
44+
const rest = args.slice(1);
45+
const stableTimeoutMs = rest.length > 1 ? parseTimeout(rest[1]) : null;
46+
const quietMs = rest.length > 0 ? parseTimeout(rest[0]) : null;
47+
return { kind: 'stable', quietMs, timeoutMs: stableTimeoutMs };
48+
}
49+
50+
/**
51+
* The fallback path once `wait` isn't a keyword form, a sleep, or a `@ref`: either a full selector
52+
* expression, or literal text. #1800: a token shaped like a selector (a recognized key, or an
53+
* unrecognized `key=value`) must never silently degrade to literal text just because the overall
54+
* positional list failed to parse as a full selector expression — that is exactly the shape that
55+
* reads as an absent element after a full timeout instead of the caller's own mistake.
56+
*/
57+
function parseSelectorOrText(args: string[], timeoutMs: number | null): WaitParsed {
2858
const argsWithoutTimeout = timeoutMs !== null ? args.slice(0, -1) : args.slice();
2959
const split = splitSelectorFromArgs(argsWithoutTimeout);
3060
if (split && split.rest.length === 0 && isValidSelectorExpression(split.selectorExpression)) {
3161
return { kind: 'selector', selectorExpression: split.selectorExpression, timeoutMs };
3262
}
63+
const rejection = detectSelectorShapedRejection(argsWithoutTimeout, split);
64+
if (rejection) return rejection;
3365
const text = timeoutMs !== null ? args.slice(0, -1).join(' ') : args.join(' ');
3466
return { kind: 'text', text: text.trim(), timeoutMs };
3567
}
3668

69+
function detectSelectorShapedRejection(
70+
argsWithoutTimeout: string[],
71+
split: { selectorExpression: string; rest: string[] } | null,
72+
): Extract<WaitParsed, { kind: 'invalid' }> | null {
73+
for (const token of argsWithoutTimeout) {
74+
const unknownKey = detectUnknownSelectorKeyToken(token);
75+
if (unknownKey) {
76+
return {
77+
kind: 'invalid',
78+
reason: 'unknown-selector-key',
79+
message: formatUnknownSelectorKeyMessage(unknownKey, argsWithoutTimeout),
80+
};
81+
}
82+
}
83+
if (split && split.rest.length > 0) {
84+
return {
85+
kind: 'invalid',
86+
reason: 'selector-shaped-text',
87+
message: formatTrailingArgsMessage(split, argsWithoutTimeout),
88+
};
89+
}
90+
if (argsWithoutTimeout.some((token) => isSelectorToken(token))) {
91+
return {
92+
kind: 'invalid',
93+
reason: 'selector-shaped-text',
94+
message: formatSelectorShapedMessage(argsWithoutTimeout),
95+
};
96+
}
97+
return null;
98+
}
99+
100+
function formatUnknownSelectorKeyMessage(
101+
unknownKey: { key: string; value: string },
102+
argsWithoutTimeout: string[],
103+
): string {
104+
const raw = argsWithoutTimeout.join(' ');
105+
const hintValue = isRoleHintWord(unknownKey.key)
106+
? `role=${unknownKey.key} label="${unknownKey.value}"`
107+
: `label="${unknownKey.value}"`;
108+
return (
109+
`Unknown selector key "${unknownKey.key}" in "${raw}". Supported: ${SELECTOR_KEY_NAMES.join(', ')}. ` +
110+
`Use wait '${hintValue}' [timeoutMs] to wait for the selector, or wait text '${raw}' [timeoutMs] to wait for the literal text "${raw}".`
111+
);
112+
}
113+
114+
function formatTrailingArgsMessage(
115+
split: { selectorExpression: string; rest: string[] },
116+
argsWithoutTimeout: string[],
117+
): string {
118+
const raw = argsWithoutTimeout.join(' ');
119+
return (
120+
`Selector ${split.selectorExpression} is followed by unexpected extra arguments: "${split.rest.join(' ')}". ` +
121+
`Selector values with spaces need quotes, e.g. label="Sign in". ` +
122+
`Use wait '<selector>' [timeoutMs], or wait text '${raw}' [timeoutMs] to wait for the literal text "${raw}".`
123+
);
124+
}
125+
126+
function formatSelectorShapedMessage(argsWithoutTimeout: string[]): string {
127+
const raw = argsWithoutTimeout.join(' ');
128+
const plainTokens = argsWithoutTimeout.filter((token) => !isSelectorToken(token));
129+
const selectorTokens = argsWithoutTimeout.filter((token) => isSelectorToken(token));
130+
if (plainTokens.length > 0) {
131+
const offending = plainTokens[0]!;
132+
const conditionHint = CONDITION_WORDS.has(offending.toLowerCase())
133+
? ` "${offending}" describes a condition, not a selector key. Use the selector form: ` +
134+
`wait 'visible ${selectorTokens.join(' ')}' [timeoutMs] to wait for it to appear, or ` +
135+
`wait 'hidden ${selectorTokens.join(' ')}' [timeoutMs] to wait for it to disappear.`
136+
: ` "${offending}" is not a recognized selector key (${SELECTOR_KEY_NAMES.join(', ')}).`;
137+
return (
138+
`"${raw}" mixes plain word "${offending}" with selector-shaped "${selectorTokens.join(' ')}", ` +
139+
`so it is neither a valid selector nor safe as literal text.${conditionHint} ` +
140+
`Use wait '<selector>' [timeoutMs] for a selector, or wait text '${raw}' [timeoutMs] to wait for the literal text "${raw}".`
141+
);
142+
}
143+
return (
144+
`"${raw}" looks like a selector key but is not a valid selector expression. ` +
145+
`Use wait '<selector>' [timeoutMs], e.g. wait '${argsWithoutTimeout[0]}="value"' [timeoutMs], ` +
146+
`or wait text '${raw}' [timeoutMs] to wait for the literal text "${raw}".`
147+
);
148+
}
149+
37150
/**
38151
* The user-supplied budget of a `wait` invocation, or null when none was given.
39152
* The budget travels as a positional, not a flag, so it is parsed the same way
@@ -44,5 +157,6 @@ export function resolveWaitBudgetMs(positionals: string[]): number | null {
44157
const parsed = parseWaitPositionals(positionals);
45158
if (!parsed) return null;
46159
if (parsed.kind === 'sleep') return parsed.durationMs;
160+
if (parsed.kind === 'invalid') return null;
47161
return parsed.timeoutMs;
48162
}

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,9 +107,14 @@ test('parseWaitArgs parses selector expression with timeout', () => {
107107
}
108108
});
109109

110-
test('parseWaitArgs falls back to text when selector-like token is invalid', () => {
110+
// #1800: this used to fall back to a literal-text wait for "foo=bar" — a caller typo (or a
111+
// straight-up unrecognized selector key) that can only ever time out, never match. It is now a
112+
// typed rejection instead. Full coverage of the rejection shapes lives in
113+
// src/core/wait-positionals.test.ts, which mirrors this module's source topology.
114+
test('parseWaitArgs rejects an unrecognized selector-like key=value token instead of reading it as text', () => {
111115
const result = parseWaitArgs(['foo=bar', '5000']);
112-
assert.deepEqual(result, { kind: 'text', text: 'foo=bar', timeoutMs: 5000 });
116+
assert.ok(result);
117+
assert.equal(result.kind, 'invalid');
113118
});
114119

115120
test('parseWaitArgs parses bare multi-word text', () => {

0 commit comments

Comments
 (0)