fix: audit follow-ups across Android input, remote auth, iOS corroboration, and CLI help - #1639
fix: audit follow-ups across Android input, remote auth, iOS corroboration, and CLI help#1639thymikee wants to merge 5 commits into
Conversation
… no request flags matchingCaptureFlags dropped the baseline snapshot's scope/depth/raw whenever the incoming request carried no flags, so the post-action corroboration capture ran at the default presentation and could never match a non-default baseline's presentationKey. The corroboration then silently declined to engage, leaking the raw XCTEST_RECORDED_FAILURE it exists to eliminate.
When close cannot confirm the device was released, it deliberately keeps the advisory claim (handing an unconfirmed device to the next session would be worse) but deletes the session record on the next line regardless, leaving a claim naming a session the daemon no longer tracks with no trace. Emit a warn diagnostic naming the device key and session, mirroring the open path's existing rollbackNewSessionClaim handling. Retention policy is unchanged.
Text entry (input text) and clipboard write (cmd clipboard set text) now quote their free-form text argument with the same shellQuoteIfNeeded helper app-lifecycle.ts already uses for deep-link URLs and launch arguments, and app-lifecycle.ts's local duplicate of that helper is retired in favor of the shared one. Multi-word clipboard writes also now arrive at the device as a single argument instead of being re-tokenized into separate ones. Updates the provider-scenario test harness's scripted clipboard-state simulator to unwrap shell quoting the same way a device shell does, so it keeps modelling what the device actually receives.
bin.ts's `--help` fast path resolved aliases through a hand-written two-entry table that had drifted out of sync with the real CLI_COMMAND_ALIASES registry (five entries). `tap`, `launch`, and `relaunch` missed the table and silently fell through to a full runCli() bootstrap just to print static help text (~150-165ms vs ~45-50ms for aliases already in the table). Delegate to the shared normalizeCliCommandAlias registry instead of the stale local table, so every alias the registry knows about gets the fast path automatically.
ADR 0007 requires generated connection profiles to strip daemon and Metro bearer tokens; only the Metro half was honored. `connect` was writing the daemon bearer token into the 0600 connection-state file, and every later command read it back out. Stop writing `authToken` into `RemoteConnectionState['daemon']` and resolve it at each reader from the existing flag -> environment (AGENT_DEVICE_DAEMON_AUTH_TOKEN) -> remote-config-profile chain instead, matching src/cli/auth-session.ts's precedence. Behavior change: a user who ran `connect --daemon-auth-token <value>` and relied on later commands picking the token back up from the state file will now get an auth failure. They must export AGENT_DEVICE_DAEMON_AUTH_TOKEN, set daemonAuthToken in their remote config, or pass --daemon-auth-token on each command. website/docs/docs/remote-proxy.md is updated to show the supported env-var workflow.
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head
Completed checks are green; iOS Smoke is still running. No |
|
Thanks — the P1 is real, and I reproduced the mechanism independently before touching anything. Recording the exact shape since it is narrower than it first looks: Of the two release call sites, only one is wrong. The root error is mine and worth naming: the token-strip commit swapped the token source at N call sites without asking which connection each site was releasing. The persisted token had been masking the distinction. Fix in progress on Splitting the batch, as you asked. Five single-commit branches off
Live evidence: agreed, and I am not going to argue the fixture tests cover it. The three device-facing PRs go up as drafts and stay drafts until exact-head evidence exists. Worth flagging one trap for the Android run: it has to execute with the test IME off, or the On the iOS corroboration one — it and #1634 fix different halves of the same fail-closed gate ( |
|
Superseded by the split, as requested. All three review points stand against this head and are being addressed on the split branches rather than here — which is why this head never moved.
Closing in favour of the per-slice PRs so each carries its own validation and rollback story. |
1 participant
Summary
Five independent defect fixes found by a read-only audit of the codebase at
46abd0f48. Each is one commit, so this is trivially splittable if you'd rather land them separately — see "Scope" below.1. Android free-text arguments are now shell-quoted (
fix(android))adb shell <argv>joins its arguments and the device's shell re-tokenizes the result, which is whyapp-lifecycle.tsalready quotes deep-link URLs and launch arguments. The two paths carrying genuinely free-form text —type/fill(input text) andclipboard write(cmd clipboard set text) — did not. They now use the same sharedshellQuoteIfNeededhelper, andapp-lifecycle.ts's byte-identical local copy of it is retired.The ASCII gate on the text path was never protection here: every shell metacharacter sits inside
0x20–0x7eand passes it.User-visible side effect worth noting: multi-word
clipboard writenow arrives at the device as one argument instead of being re-tokenized into several.2. Remote connections no longer persist the daemon bearer token (
fix(remote)) — BREAKINGADR 0007 states that generated connection profiles "must strip daemon and Metro bearer tokens". The Metro half was honored; the daemon half was not —
connectwrote the token into the connection-state file and every later command read it back.This changes behavior. After this, a token passed once to
connectis not reused by later commands:Every alternative path ADR 0007 names already existed (
AGENT_DEVICE_DAEMON_AUTH_TOKEN, thedaemonAuthTokenconfig key,--daemon-auth-token), so this is conformance rather than new machinery.website/docs/docs/remote-proxy.mdis updated to the env-var workflow.3. iOS tap corroboration keeps its baseline's presentation (
fix(ios))matchingCaptureFlagsreturnedundefinedwhenever the request carried noflags, discarding the baseline'sdepth/scope/raw. The probe was then captured at default presentation,presentationKeycouldn't match a non-default baseline, and #1605's rescue silently declined to engage — leaving the caller with the possibly-falseXCTEST_RECORDED_FAILUREit exists to eliminate. The asymmetry was visible in the same function:interactiveOnlywas read off the presentation unconditionally while the other three fields were dropped.Reachable via batch steps (
cli/batch-steps.tsomits theflagskey entirely when undefined) and the JSON-RPC boundary; CLI and Node-client callers always populate it.4. A device claim retained by a failed close is now reported (
fix(daemon))When
closecan't confirm the device was released it deliberately keeps the advisory claim — correct — but then deletes the session record unconditionally and said nothing. The claim was left owned by a session name nothing else knew about, reclaimable only by reopening under that exact name or by daemon death. It now emits adevice_claim_close_effects_unconfirmedwarn diagnostic carrying the device key and session name, mirroringrollbackNewSessionClaimon the open path. Retention policy is unchanged.5. Command aliases take the
--helpfast path (perf(cli))bin.tsresolved aliases through a hand-written two-entry table while the real registry has five, sotap,launch, andrelaunchfell through to a full CLI bootstrap just to print static help. It now delegates tonormalizeCliCommandAlias.--helppress(control)taplaunchrelaunchrotatedeliberately still misses the fast path so its migration error keeps rendering.Validation
Every regression test here was proven to fail without its fix — reverted, run, failure captured, restored. Two were instructive: the tap-corroboration test fails with
expected false to be true, and the retained-claim test fails withENOENT ... diagnostics.ndjson, because zero diagnostic events are emitted without the change.pnpm check:affected --runis green on the assembled branch. Android quoting was additionally validated throughpnpm test:integration:provider(42/42 files, 149/149 tests).One test-harness change deserves attention:
provider-scenarios/android-world.tssimulated the device clipboard by exact-matching the joined argv, so quoting stopped the match firing and an unrelated assertion failed first. That harness was modelling the unquoted argv as correct — i.e. encoding the pre-fix behavior — so it now matches the command prefix and shell-unquotes the value, mirroring what a real device shell does beforecmdsees it. Matching the new quoted literal instead would have re-frozen the same mistake one step over.Contention note: these ran on a machine reaching load average 70+, producing shifting timeout-only failures in unrelated files (
doctor.test.ts,ios-lifecycle.test.ts, iOS install/exec suites). Each was confirmed to be a timeout rather than an assertion failure and to pass in isolation before being dismissed, per the documented contention-retry policy. Final gate runs were clean at load ~5.Residual risk — no live device evidence
Per
docs/agents/pull-requests.md, device-facing behavior isn't merge-ready on fixture-backed tests alone, and this PR has no live simulator/emulator run. Three changes touch device-facing paths:adb shellfallback is actually exercised (open --test-imeoff), typing text containing;,`,$(), and a quote.Treating this as residual risk rather than calling it verified. Happy to run any of these before merge.
Scope and follow-ups
20 files, +516/−38. Scope deliberately spans five unrelated areas because this is an audit batch, not one feature — one commit per fix, so splitting is a
git cherry-pickaway. Against the repo's own "keep changes to one command family" rule, that's a real tradeoff and reviewer's call.matchingCaptureFlags's immediate neighbourhood ininteraction-ios-tap-outcome.ts. They fix different halves of the same fail-closed gate — fix(ios): pin tap-outcome corroboration probes to the baseline's backend #1634 the backend condition, this the presentation condition — and both are worth having. Whichever merges second needs a trivial rebase. Details in my review on fix(ios): pin tap-outcome corroboration probes to the baseline's backend #1634.kernel-errorsrecords 55.19% though test(kernel): cover all six errors.ts exports, kernel-errors mutation 55.19% → 86.26% #1475 took it to 86.26%;scroll-edge-staterecords 28.75% though test: scroll-edge-state has zero direct tests and a 28.75% mutation score — write the missing suite #1455 closed that gap). Because those entries carry an oldconfigHash, the ratchet treats a rerun as provenance drift and replaces the score rather than takingMath.max— so running the sweep on a contended machine would write a falsely low floor and lock it in permanently, which is the exact failure that finding is about. Those numbers should come from the weekly lane's artifact on a quiet runner. Related: gate(mutation): ratchet graduates on stability alone — a 28.75% score can lock in as the permanently gated floor #1457.CONTEXT.md's corroboration bullet now under-describes the mechanism.