fix: reject selector-shaped wait arguments instead of reading them as text - #1813
Merged
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
… 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.
thymikee
force-pushed
the
fix/1800-wait-selector-shaped-text
branch
from
August 18, 2026 09:43
8b4c133 to
b06bf1f
Compare
Member
Author
|
Clean review at b06bf1f: selector-shaped wait inputs are now rejected at both the owning positional parser and the shared daemon dispatch seam before device work. The typed invalid variant, timeout-budget handling, production-path regression, planted-red evidence, and live iOS validation are sound. All completed gates are green; only iOS Smoke remains pending. Code review is ready-for-human. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
waitaccepted an argument shape its grammar does not hold, joined the leftover positionals into one literal-text string, and then waited out the full timeout for text that could never appear on screen — reading as a false "element absent" instead of the caller's own mistake.Before (0.20.10):
(after the full 25s timeout, exit code non-zero but read as "element not found")
After:
(immediate, before any device work)
parseWaitPositionals(src/core/wait-positionals.ts) now returns a typedinvalidvariant whenever, after stripping the trailing timeout, a positional token is
selector-shaped — a recognized selector key (
isSelectorToken), or an unrecognizedkey=value(detectUnknownSelectorKeyToken) — but the token list does not form avalid selector expression, or a valid selector prefix is followed by unquoted trailing
tokens. The message:
a
label=/role="did you mean" hint (mirrors Bare ref positional produces cryptic coordinate error instead of a hint #1035's click/press/fill/get fix);exists,present,appears,gone,disappears) at theselector form, e.g.
wait 'visible label="Open"' [timeoutMs]—visible/hiddenare already selector keys, so they parse correctly and are unaffected;
wait text '<text>' [timeoutMs].Bare text (
wait Continue 1500,wait Sign in 2000) and the explicittextkeywordform are untouched — including the documented boundary that a single word matching a
recognized selector key (e.g.
wait id 3000) is now rejected rather than silentlyread as literal text, since it must go through
wait text 'id' 3000to disambiguate.resolveWaitBudgetMs(used by the command descriptor's timeout-policy budget) returnsnullfor a rejected list, same as an unparseable one.dispatchWaitViaRuntime(
src/daemon/selector-runtime.ts) — the one seam both a live CLI wait and a replayed.adwait step dispatch through — rejects before any session/device work. Excludinginvalidfrom the typetoWaitTargetaccepts makes its kind-by-kind narrowingexhaustive at compile time, with no runtime fallback branch.
Closes #1800
Validation
src/core/wait-positionals.test.ts: the issue's repro shapes (open/exists+selector value), an unknown
key=valuetoken, a valid selector prefix followed by anunquoted trailing word,
visible/bare-text/text-keyword forms staying unaffected,the bare-recognized-key-word boundary (
wait id 3000), and two fast-check properties— a generated valid selector expression never parses as
text, and any token listcontaining a
key=value-shaped token never parses astext.wait-positionals.ts,wait.ts,selector-runtime.ts) and reran the new/updated tests against pre-fix code. All 9assertions failed as expected — 7 asserted
'text'/a stale budget instead of'invalid', the property test found["", 'editable="!"', 0]as a counterexample inone run, and the new
dispatchWaitViaRuntimedaemon-dispatch test hung to the5000ms test timeout (the pre-fix code actually dispatches and polls instead of
rejecting immediately) — confirming this is exactly the "silent full-timeout" defect
class from the issue, not a vacuous pin. Restored the fix; all 45 tests pass.
snapshot.test.tscase that had codified the bug(
parseWaitArgs falls back to text when selector-like token is invalid, usingfoo=bar— an unrecognized key) to assert the newinvalidrejection.(
wait-landmark-recording.test.ts) provingdispatchWaitViaRuntime— the route botha live CLI wait and a replayed
.adwait step call — rejects before anysession/device work, since neither the
.adscript grammar(
packages/ad-script) nortest/replay-compatinterpret wait's positional shape atparse time (
pnpm exec vitest run --project unit-core test/replay-compat— all 55cases pass unchanged, no corpus verdict moved).
pnpm check:affected --rungreen (format, lint, typecheck, layering, fallow, build,and the full
vitest-relatedselection — 424 files / 3651 tests).wait-positionals.tsisn't a mutation-kernel module (
scripts/mutation/modules.ts) and I didn't touchpackages/selectors/src/**, so the mutation gate wasn't selected — confirmed by theselector's own output.
iPhone 17 Pro, isolated--state-dir,com.apple.Preferences):wait open 'label="General"' 3000→ immediateINVALID_ARGSin 0.32s (not a 3stimeout), quoting the same message shape as above.
wait exists 'label="General"' 3000→ immediateINVALID_ARGSin 0.068s, with thecondition-word hint (
wait 'visible label="General"' [timeoutMs]).wait 'label="General"' 3000→ correctly dispatched and got a legitimateAMBIGUOUS_MATCH(multiple "General" nodes on the live Settings screen — pre-existing,by-design direct-iOS-selector behavior, unrelated to this fix);
wait 'role="button" label="General"' 3000(disambiguated) → exit 0.wait General 3000(bare text) → exit 0.wait text 'General' 3000(explicit form) → exit 0.the udid afterward.
Docs: no
website/docs/docs/adrchange needed — the acceptedwaitforms areunchanged, only a previously-silent-degrading input now gets a typed refusal instead of
a timeout (same precedent as #1035's click "Did you mean" fix, which also didn't touch
docs).
6 files changed (5 touched, 1 new test file); scope stayed within the
waitcommandfamily (
src/core/wait-positionals.ts, its CLI reader, and the one daemon dispatchseam) plus test updates — no scope expansion.