From 30cd269fe1dbf01a5acea9a76f6f7151187c7625 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 16 Jul 2026 18:29:33 +0200 Subject: [PATCH 1/4] fix(replay): default-exclude observation-only reads from repair heals, add --record opt-in (#1271 stage 2) Amends ADR 0012 decision 6: snapshot/get/is/a read-only find are excluded from a repair-armed heal by default (session.saveScriptBoundary set), never from ordinary open --save-script authoring recording. wait keeps recording (flow timing, not observation). The corrective-read trap (wave-3 E3: the diverged step was itself a get) means blanket read-exclusion is unsafe, so a new --record flag forces one action through when the correction is itself a read. --record/--no-record are mutually exclusive (INVALID_ARGS if both are set) and are plumbed identically across CLI, the Node client, and MCP. The exclusion lives at the single daemon-side choke point (recordActionEntry/isExcludedRepairSegmentObservation), so an excluded read never grows session.actions.length -- the same counter the existing record-and-heal resume watermark (describeUnperformedRecordAndHeal) already checks, so the empty-segment fail-loud guard falls out for free (message updated to mention --record). Also fixes a latent bug found along the way: the get/is/find/snapshot CLI readers never forwarded --no-record/--record into the built request (only `open` did), so stage 1's "use --no-record" guidance was silently inert via the CLI. --- docs/adr/0012-interactive-replay.md | 116 ++++- src/cli/parser/cli-help.ts | 2 +- src/client/client-types.ts | 32 +- src/commands/capture/snapshot.ts | 8 +- src/commands/capture/wait.ts | 4 +- src/commands/cli-grammar/common.ts | 25 +- .../cli-grammar/flag-definitions-action.ts | 8 + src/commands/cli-grammar/flag-groups.ts | 6 + src/commands/command-flags.ts | 1 + src/commands/interaction/index.ts | 6 +- src/commands/interaction/interactions.ts | 26 +- src/commands/interaction/metadata.ts | 14 + src/commands/interaction/selectors.ts | 9 +- src/contracts/cli-flags.ts | 10 + .../request-router-record-flags.test.ts | 55 +++ .../__tests__/selector-recording.test.ts | 70 +++ .../__tests__/session-action-recorder.test.ts | 88 ++++ ...ion-replay-repair-record-exclusion.test.ts | 409 ++++++++++++++++++ .../session-replay-repair.fixtures.ts | 8 + src/daemon/handlers/find.ts | 13 + .../handlers/session-replay-action-runtime.ts | 9 +- .../handlers/session-replay-runtime-plan.ts | 20 +- src/daemon/request-router.ts | 20 +- src/daemon/selector-recording.ts | 6 + src/daemon/selector-runtime.ts | 9 +- src/daemon/session-action-recorder.ts | 91 +++- src/daemon/snapshot-runtime.ts | 7 + src/daemon/types.ts | 21 + src/mcp/__tests__/command-tools.test.ts | 44 ++ src/replay/__tests__/divergence.test.ts | 27 +- src/replay/divergence.ts | 48 +- src/selectors/find.ts | 17 + 32 files changed, 1148 insertions(+), 81 deletions(-) create mode 100644 src/daemon/__tests__/request-router-record-flags.test.ts create mode 100644 src/daemon/__tests__/selector-recording.test.ts create mode 100644 src/daemon/__tests__/session-action-recorder.test.ts create mode 100644 src/daemon/handlers/__tests__/session-replay-repair-record-exclusion.test.ts diff --git a/docs/adr/0012-interactive-replay.md b/docs/adr/0012-interactive-replay.md index 87a187f2c..4addebb5d 100644 --- a/docs/adr/0012-interactive-replay.md +++ b/docs/adr/0012-interactive-replay.md @@ -2,7 +2,7 @@ ## Status -Accepted (2026-07-10); partially implemented (last updated 2026-07-13). See [Migration progress](#migration-progress) for the per-step landing record. +Accepted (2026-07-10); partially implemented (last updated 2026-07-16). See [Migration progress](#migration-progress) for the per-step landing record. **Implemented and merged to `main`:** @@ -12,6 +12,10 @@ Accepted (2026-07-10); partially implemented (last updated 2026-07-13). See [Mig `REPLAY_DIVERGENCE` repair-loop fix (#1223). - Decision 6, the base agent-supervised re-record repair — `replay --save-script` arming, the post-watermark healed slice, `repairHint`, and the writer's bare-`@ref` fail-loud guard (#1228). +- Decision 6, R7 + commit state machine (repair-transaction lifecycle) — the ARMED → COMPLETE → COMMITTED + commit state machine, keep-alive keyed off the persisted transaction (`resume.repairSessionHeld`, never + the per-request `--save-script` flag), terminal-source-`close` skipping, the `REPAIR_SESSION_EXPIRED` + tombstone, and race-safe atomic publication via exclusive `linkSync` (#1235). - Decision 4 amendment, `screen`'s capture scope and ref selection — the divergence `screen` capture runs through the same `captureSnapshot` wrapper as plain `snapshot` (full-window scope + Android freshness / post-action retry parity) under a clean, fixed capture-flags policy (a failed raw/scoped/`-d` action can @@ -31,6 +35,9 @@ Accepted (2026-07-10); partially implemented (last updated 2026-07-13). See [Mig end-to-end; a container whose subtree holds a competing interactive node (canonical classification or hittable flag), or whose selected descendant's center falls outside the container rect, is recorded unchanged; `fill` is excluded (#1280). +- Decision 6 amendment, repair-segment default exclusion of observation-only commands and the `--record` + opt-in — stage 1 (interim guidance only, #1287) and stage 2 (the semantic default-exclusion fix, + this amendment) of #1271. See the amendment under decision 6 below. **Accepted but NOT yet implemented** (this amendment; tracked by #1235 — repair-transaction lifecycle): the R7 repair-transaction keep-alive and its distinct `resume.repairSessionHeld` signal, the ARMED → @@ -976,6 +983,90 @@ re-run guidance to restart the repair from the original script), never a bare `S `replay --save-script` on the same key **clears the tombstone and starts a fresh transaction** (`ARMED`). The tombstone itself expires after its bounded window, after which the key is fully free. +> **Amendment (#1271 stage 2): repair-segment default exclusion of observation-only commands, and the +> `--record` opt-in.** #1271 reported that read-only diagnostics an agent runs mid-repair to LOCATE the +> repair target — `snapshot -i`, `get attrs`, `find`, `is` — recorded into the healed script exactly like +> the corrective action itself. The wave-3 E3 experiment measured 0/4 trials producing a clean healed +> script hands-off; one recorded `get attrs` caused a second, self-inflicted `identity-mismatch` divergence +> on fresh replay. Stage 1 (#1287) shipped interim guidance only — the divergence `repairHint` and `help +> workflow` told the agent to pass `--no-record` on those commands by hand. This amendment makes the safer +> behavior the default, superseding stage 1's "opt out by hand" guidance with "opt in by hand" for the one +> case that needs it. +> +> **1. Which read-only commands are excluded.** Exactly the enumerated set stage 1 already named: +> `snapshot`, `get`, `is`, and a **read-only** `find` sub-action (`exists`, `wait`, `get_text`, `get_attrs`). +> A **mutating** `find … click|fill|focus|type` is not in this set — it is a corrective action like `press`/ +> `fill` and always records. The top-level `wait` command is deliberately **not** in this set either: it is +> flow timing/synchronisation (pacing the script), not observation, so it must keep recording unconditionally +> even inside a repair segment — a healed script that silently dropped its own timing steps would replay +> differently from what actually happened. +> +> **2. The corrective-read trap, and why blanket exclusion by command class is unsafe.** "Exclude every +> read from a repair segment" is not a safe rule on its own: a divergence's **correction** can itself be a +> read. In the wave-3 E3 drift, the diverged step **was** a `get`, so its corrective replacement is a +> recorded `get` that **must** land in the heal — a blanket read-exclusion would silently drop the very +> step being repaired, producing an incomplete heal with no signal anything was missing (the same class of +> silent-failure mode decision 3 exists to close for target-binding, and decision 6 R4 exists to close for +> bare `@ref` exports). Distinguishing "the read that replaces the diverged step" from "reads used to locate +> the target" by POSITION (e.g., relative to the eventual `--from` resume) was considered and rejected: the +> daemon records each action as it happens and has no reliable way to know, at record time, which one the +> agent will later treat as the resume boundary — and a position-based rule would silently reinterpret +> intent after the fact rather than let the agent state it. The adopted rule instead makes this an **explicit +> opt-in per action**: `--record` (below) marks the ONE read that must count, independent of its position in +> the repair segment. +> +> **3. Explicit recording remains possible.** `--record` is a new, standalone flag — distinct from +> `saveScript`, and the mirror image of `--no-record` — that forces the action it is passed on into the heal +> even though its command is observation-only. It is a no-op outside a repair-armed session (there is no +> default exclusion to override) and a no-op on any non-observation-only command (those already always +> record). `--record` and `--no-record` express opposite intents for the same action and are **mutually +> exclusive**: passing both is rejected as `INVALID_ARGS` before the command runs, uniformly for every +> surface, rather than letting one silently win. +> +> **4. How the rule projects across CLI/Node/MCP surfaces.** The exclusion is enforced at a single +> daemon-side choke point — `isExcludedRepairSegmentObservation` in `recordActionEntry` +> (`src/daemon/session-action-recorder.ts`), gated on `session.saveScriptBoundary !== undefined` (set ONLY +> by a repair-armed `replay --save-script`; an ordinary, non-repair `open --save-script`/`close --save-script` +> authoring recording never sets it — see decision 6's "Scope" note above) and on the entry's +> `observationOnly` marker (set by the caller — `get`/`is`/a read-only `find` in +> `src/daemon/selector-recording.ts`, `snapshot` in `src/daemon/snapshot-runtime.ts` — never inferred from +> `command`/`positionals`, since a mutating `find … click` shares the literal string `command: 'find'` with +> its read-only sibling). Every surface reaches this same function because every surface's request reaches +> the same daemon: the CLI parses `--record`/`--no-record` into `CommandFlags` (`src/contracts/cli-flags.ts`, +> `src/commands/cli-grammar/flag-definitions-action.ts`, `src/commands/cli-grammar/flag-groups.ts`'s +> `COMMON_COMMAND_SUPPORTED_FLAG_KEYS` — so both flags are syntactically valid on every command, exactly like +> `--no-record` already was); the Node client threads the same-named option through +> `buildRequestFlags`/`buildFlags` (`src/commands/command-flags.ts`) from `InternalRequestOptions` and +> `AppOpenOptions` (`src/client/client-types.ts`) and from each typed per-command Options type +> (`GetOptions`/`IsOptions`/`FindOptions`/`CaptureSnapshotOptions`); and the MCP tool schema exposes `record`/ +> `noRecord` fields on the `get`/`is`/`find`/`snapshot` tools (`src/commands/interaction/metadata.ts`, +> `src/commands/capture/snapshot.ts`) that flow through the same `CommandMetadata.inputSchema` + +> `readMetadataCommandFlags` path every other typed field already uses. `management/app.ts` mirrors both +> flags onto `open` for symmetry with `--no-record`'s existing presence there, though neither has an effect +> on a command that is never observation-only. No surface has a separate recording contract; all of them +> hit the identical daemon-side default. +> +> **5. The fail-loud guard is the EXISTING resume watermark, not new state.** Because the exclusion happens +> at the same point `--no-record` is enforced — before an action ever reaches `session.actions` — an excluded +> diagnostic read never grows `session.actions.length`. That counter is exactly what +> `describeUnperformedRecordAndHeal` (`src/daemon/handlers/session-replay-runtime-plan.ts`, R2/#1262's +> `pendingRecordAndHeal` watermark) already compares against `actionsCountAtDivergence` to prove a corrective +> action happened before authorizing a `record-and-heal`-shaped `--from` resume. No additional bookkeeping is +> needed: a repair segment containing only excluded diagnostic reads is indistinguishable, at that check, +> from a repair segment containing no activity at all, so the existing guard already refuses it. This +> amendment only updates the guard's message to name `--record` alongside the existing `--no-record` +> mention, since the missing corrective action may have been a read rather than a press: "no corrective +> action was recorded in this repair segment; press the correct control via a blessed `@ref` … — or, if your +> corrective action was a read, re-run it with `--record` … — before resuming." +> +> **Regression coverage** proves: a diagnostic read inside an armed repair segment is omitted from the +> healed script by default; a corrective action remains for both a mutating press/fill (unaffected — never +> observation-only) and a `--record`ed read (the diverged-step-was-a-`get` case); the resulting healed script +> replays cleanly in a fresh session; the empty-segment guard refuses a blind resume with the actionable +> `--record` hint; and ordinary, non-repair `open --save-script` authoring recording is completely unchanged +> — a read in a fresh authoring session still records with no flag needed, because `session.saveScriptBoundary` +> is never set there. + ## Consequences - `--from` makes app state the caller's responsibility, and only accepts a resume when the planner can @@ -1073,8 +1164,8 @@ each states its dependencies explicitly. daemon-side `repairHint` computation over all four `kind`s (R3), `--save-script` arming plus the repair-run boundary watermark on `replay` (R1/R6), the writer's post-watermark slice and bare-`@ref` fail-loud guard (R4/R6) — reusing `close --save-script`'s existing `session.actions` serializer. -9. **Repair-transaction lifecycle** (decision 6, R7 + commit state machine) — **NOT YET IMPLEMENTED, - tracked by #1235**; depends on 8. Adds: the distinct `resume.repairSessionHeld` divergence signal and +9. **Repair-transaction lifecycle** (decision 6, R7 + commit state machine) — **SHIPPED (#1235)**; + depends on 8. Adds: the distinct `resume.repairSessionHeld` divergence signal and R7 keep-alive keyed off the session's **persisted** repair-transaction state (so `--from` continuations need no repeat of `--save-script`), with `fail-fast-before-step-1` when keep-alive is impossible; the `ARMED → COMPLETE → COMMITTED` commit state machine where **any teardown** (explicit `close`, @@ -1087,10 +1178,19 @@ each states its dependencies explicitly. pre-existing target — complete or partial, default or explicit path alike, and uniformly for an ordinary non-repair recording's target too, since the publish primitive is shared — never overwriting one; see "Scope" under Decision 6 above). +10. **Repair-segment default exclusion of observation-only commands, and the `--record` opt-in** + (decision 6 amendment) — **SHIPPED (#1271 stage 2)**; depends on 9 (`session.saveScriptBoundary`/ + `repairSessionHeld` are the signals the exclusion and its guidance key off). Adds: default exclusion of + `snapshot`/`get`/`is`/a read-only `find` from `session.actions` while repair-armed + (`isExcludedRepairSegmentObservation`, `session-action-recorder.ts`); the `--record` opt-in, plumbed + across CLI/Node client/MCP identically to `--no-record`, and rejected together with it as + `INVALID_ARGS`; and an updated `describeUnperformedRecordAndHeal` message naming `--record` for the + corrective-read case. Stage 1 (#1287) shipped interim `--no-record` guidance only, gated on this same + `repairSessionHeld` signal. ## Migration progress -Landing record for the plan above (main as of 2026-07-13). This section tracks progress only; it does +Landing record for the plan above (main as of 2026-07-16). This section tracks progress only; it does not restate or amend the decisions. | Step | Decision | Status | Landed in | @@ -1103,11 +1203,13 @@ not restate or amend the decisions. | 6. `--update` retirement | 1 | Shipped | #1211 | | 7. Benchmark extension | 5 | Deferred | — | | 8. Agent-supervised re-record repair, base | 6 (R1-R6) | Shipped | #1228 | -| 9. Repair-transaction lifecycle | 6 (R7) | Not yet | #1235 | +| 9. Repair-transaction lifecycle | 6 (R7) | Shipped | #1235 | +| 10. Repair-segment default exclusion + `--record` | 6 amendment | Shipped | #1271 stage 2 | Step 4 (#1209) added the `selector-miss`/`identity-mismatch`/`identity-unverifiable` divergence kinds and a post-resolution target guard that cross-checks the dispatched winner against the verified member. Issue #1221 ("Complete ADR 0012 replay target-binding verification") was closed as already implemented: its verification scope is covered by step 4. Step 7 remains deferred. Step 8 implements the repair -design accepted in #1226 and shipped in #1228; step 9 (R7 + commit machine) remains unimplemented, -tracked by #1235. +design accepted in #1226 and shipped in #1228; step 9 (R7 + commit machine) shipped in #1235. Step 10 +is #1271's two-stage arc: stage 1 (#1287) shipped interim `--no-record` guidance only; stage 2 (this +amendment) makes exclusion the default and adds the `--record` opt-in. diff --git a/src/cli/parser/cli-help.ts b/src/cli/parser/cli-help.ts index af1178357..d5a1f2532 100644 --- a/src/cli/parser/cli-help.ts +++ b/src/cli/parser/cli-help.ts @@ -325,7 +325,7 @@ Validation and evidence: agent-device screenshot agent-device press 124 817 agent-device snapshot -i - Startup/CPU/memory/frame first pass: perf metrics --json (bare perf and metrics are aliases). Focused frame/jank health: perf frames --json. Memory-only sample: perf memory sample --json returns compact JSON with bounded top offenders. Heap/memgraph artifact escalation: perf memory snapshot --out heap.artifact; use --kind android-hprof on Android or --kind memgraph on supported Apple simulator/macOS app sessions. Android native profiling: perf cpu profile start|stop|report --kind simpleperf --out ; Android native traces: perf trace start|stop --kind perfetto --out . Artifact collectors return compact state/path/size metadata only; raw heap/profile/trace files stay on disk. Treat native perf output as the agent evidence: for example, a Perfetto stop can return state=stopped, outPath=/tmp/app.perfetto-trace, sizeBytes=5392410, and method=adb-shell-perfetto while the 5.3 MB raw trace stays in the artifact. This is better than raw dumps for agents because it is stable, bounded, and keeps large artifacts out of context. heapprofd is deferred until Perfetto plumbing is available. Replay divergence and resume: a failing replay/test step returns REPLAY_DIVERGENCE with a bounded report (screen digest, ranked selector suggestions, resume). Repair app state, then resume with replay --from --plan-digest (both from the report's resume field) to continue from the failed step without re-running earlier ones; resume never re-executes skipped steps, so app state is the caller's responsibility, and it is rejected with INVALID_ARGS when the plan digest is stale, --from is out of range, or the skipped range/target touches runtime control flow. The plan digest binds the script, its includes, the effective --platform/--target, and per-action runtime/identity. Native .ad interpolation is late-bound after planning, so changing only its values keeps the digest; Maestro environment substitution occurs during compatibility parsing and can change action inputs, includes, or control expansion, so it can change the digest. --from is replay-only; test rejects it. --update/-u no longer rewrites the script (ADR 0012) — it is a no-op kept for compatibility; every divergence already carries the same ranked suggestions. Agent-supervised repair (heal-by-doing): arm replay .ad --save-script[=] before step 1 (armed once; --from continuations do not need it again). Every divergence carries a repairHint: record-and-heal means press the correct control via a blessed @ref from the divergence's screen.refs, recorded (no --no-record), then continue with replay --from --plan-digest ; state-repair means the script is correct but app state is not, so fix state with --no-record actions, then replay --from --plan-digest to re-run the unchanged step; caution means something already matches the recorded selector, so a blind re-press may repeat the mistake; manual means no safe automated repair could be proven. While armed, read-only inspection used to locate the repair target (snapshot -i, get attrs, find, is) is recorded too — pass --no-record on those commands, or they land in the healed script alongside the corrective action. Ending the repair with close --save-script[=] writes only the steps recorded since the arming replay as , defaulting to the sibling .healed.ad — review its diff before promoting it over the original. + Startup/CPU/memory/frame first pass: perf metrics --json (bare perf and metrics are aliases). Focused frame/jank health: perf frames --json. Memory-only sample: perf memory sample --json returns compact JSON with bounded top offenders. Heap/memgraph artifact escalation: perf memory snapshot --out heap.artifact; use --kind android-hprof on Android or --kind memgraph on supported Apple simulator/macOS app sessions. Android native profiling: perf cpu profile start|stop|report --kind simpleperf --out ; Android native traces: perf trace start|stop --kind perfetto --out . Artifact collectors return compact state/path/size metadata only; raw heap/profile/trace files stay on disk. Treat native perf output as the agent evidence: for example, a Perfetto stop can return state=stopped, outPath=/tmp/app.perfetto-trace, sizeBytes=5392410, and method=adb-shell-perfetto while the 5.3 MB raw trace stays in the artifact. This is better than raw dumps for agents because it is stable, bounded, and keeps large artifacts out of context. heapprofd is deferred until Perfetto plumbing is available. Replay divergence and resume: a failing replay/test step returns REPLAY_DIVERGENCE with a bounded report (screen digest, ranked selector suggestions, resume). Repair app state, then resume with replay --from --plan-digest (both from the report's resume field) to continue from the failed step without re-running earlier ones; resume never re-executes skipped steps, so app state is the caller's responsibility, and it is rejected with INVALID_ARGS when the plan digest is stale, --from is out of range, or the skipped range/target touches runtime control flow. The plan digest binds the script, its includes, the effective --platform/--target, and per-action runtime/identity. Native .ad interpolation is late-bound after planning, so changing only its values keeps the digest; Maestro environment substitution occurs during compatibility parsing and can change action inputs, includes, or control expansion, so it can change the digest. --from is replay-only; test rejects it. --update/-u no longer rewrites the script (ADR 0012) — it is a no-op kept for compatibility; every divergence already carries the same ranked suggestions. Agent-supervised repair (heal-by-doing): arm replay .ad --save-script[=] before step 1 (armed once; --from continuations do not need it again). Every divergence carries a repairHint: record-and-heal means press the correct control via a blessed @ref from the divergence's screen.refs, recorded (no --no-record), then continue with replay --from --plan-digest ; state-repair means the script is correct but app state is not, so fix state with --no-record actions, then replay --from --plan-digest to re-run the unchanged step; caution means something already matches the recorded selector, so a blind re-press may repeat the mistake; manual means no safe automated repair could be proven. While armed, read-only inspection used to locate the repair target (snapshot -i, get attrs, find, is) is excluded from the healed script by default — no --no-record needed; if the step you are repairing is itself a read, pass --record on that one command so it lands in the heal (--record and --no-record are mutually exclusive). Ending the repair with close --save-script[=] writes only the steps recorded since the arming replay as , defaulting to the sibling .healed.ad — review its diff before promoting it over the original. Recording: record start/stop. The default scope is app and expects an active session created by open ; this keeps app proof videos tied to the intended app session. Use record start --scope device/system to explicitly request whole-screen capture where the selected backend supports it, such as recordings that intentionally span multiple apps, home screen, settings, or app transitions. Use --max-size to cap the longest edge and --quality medium|high to choose output quality across Android and Apple targets. By default, stop burns touch overlays into the video; use record start --hide-touches for the fastest raw recording. Android record start publishes a durable device manifest. Android adb screenrecord has a 180s platform limit, so longer Android recordings are returned as multiple MP4 chunks while the daemon stays alive; after daemon restart, record stop recovers only manifest-owned chunks and warns when gesture overlays are unavailable. For gesture-heavy iOS simulator proof videos, prefer --hide-touches because overlay timing depends on a stable runner session while gestures are executing. Tracing: trace start ./trace.log, trace stop ./trace.log. Paths are positional. Stable known flow: batch ./steps.json, not workflow batch. Inline batch JSON example: diff --git a/src/client/client-types.ts b/src/client/client-types.ts index 5d595f716..c88c90b3c 100644 --- a/src/client/client-types.ts +++ b/src/client/client-types.ts @@ -456,6 +456,13 @@ export type CaptureSnapshotOptions = AgentDeviceRequestOverrides & raw?: boolean; forceFull?: boolean; timeoutMs?: number; + /** + * #1271 stage 2 (ADR 0012 amendment): `snapshot` is observation-only and + * excluded from a repair-armed heal by default; `record` forces it + * through. Mutually exclusive with `noRecord`. + */ + noRecord?: boolean; + record?: boolean; }; export type CaptureSnapshotResult = { @@ -818,21 +825,35 @@ export type RotateGestureOptions = DeviceCommandBaseOptions & { export type TransformGestureOptions = DeviceCommandBaseOptions & TransformGestureParams; +/** + * #1271 stage 2 (ADR 0012 amendment): `get`/`is`/`find` are observation-only + * and excluded from a repair-armed heal by default. `record` forces this + * action through (the corrective-read case); `noRecord` continues to opt the + * action out entirely. Mutually exclusive. + */ +type RecordControlOptions = { + noRecord?: boolean; + record?: boolean; +}; + export type GetOptions = DeviceCommandBaseOptions & SelectorSnapshotCommandOptions & - ElementTarget & { + ElementTarget & + RecordControlOptions & { format: 'text' | 'attrs'; }; type IsTextPredicateOptions = DeviceCommandBaseOptions & - SelectorSnapshotCommandOptions & { + SelectorSnapshotCommandOptions & + RecordControlOptions & { predicate: 'text'; selector: string; value: string; }; type IsStatePredicateOptions = DeviceCommandBaseOptions & - SelectorSnapshotCommandOptions & { + SelectorSnapshotCommandOptions & + RecordControlOptions & { predicate: 'visible' | 'hidden' | 'exists' | 'editable' | 'selected' | 'focused'; selector: string; value?: never; @@ -841,7 +862,8 @@ type IsStatePredicateOptions = DeviceCommandBaseOptions & export type IsOptions = IsTextPredicateOptions | IsStatePredicateOptions; type FindBaseOptions = DeviceCommandBaseOptions & - FindSnapshotCommandOptions & { + FindSnapshotCommandOptions & + RecordControlOptions & { locator?: FindLocator; query: string; first?: boolean; @@ -1098,6 +1120,8 @@ export type InternalRequestOptions = AgentDeviceClientConfig & deviceHub?: boolean; testIme?: boolean; noRecord?: boolean; + /** #1271 stage 2: force-record this action; mutually exclusive with `noRecord`. */ + record?: boolean; backMode?: BackMode; metroHost?: string; metroPort?: number; diff --git a/src/commands/capture/snapshot.ts b/src/commands/capture/snapshot.ts index 1079a9e2c..a7b21ff1a 100644 --- a/src/commands/capture/snapshot.ts +++ b/src/commands/capture/snapshot.ts @@ -5,7 +5,8 @@ import { defineExecutableCommand } from '../command-contract.ts'; import { commonInputFromFlags, direct, - recordControlInputFromFlags, + noRecordInputFromFlags, + observationRecordInputFromFlags, } from '../cli-grammar/common.ts'; import type { CliReader, DaemonWriter } from '../cli-grammar/types.ts'; import { defineCommandFacet } from '../family/types.ts'; @@ -40,12 +41,13 @@ const snapshotCliSchema = { helpDescription: 'Capture accessibility tree or diff against the previous session baseline. For iOS raw-coordinate fallback after a no-op ref press, inspect rects with snapshot -i --json, press the rect center, then verify with diff snapshot -i or snapshot --diff.', summary: 'Capture accessibility tree or diff against the previous session baseline', - allowedFlags: ['snapshotDiff', ...SNAPSHOT_FLAGS, 'snapshotForceFull', 'timeoutMs'], + allowedFlags: ['snapshotDiff', ...SNAPSHOT_FLAGS, 'snapshotForceFull', 'timeoutMs', 'record'], } as const; export const snapshotCliReader: CliReader = (_positionals, flags) => ({ ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), + ...observationRecordInputFromFlags(flags), interactiveOnly: flags.snapshotInteractiveOnly, depth: flags.snapshotDepth, scope: flags.snapshotScope, diff --git a/src/commands/capture/wait.ts b/src/commands/capture/wait.ts index 8dd2941f2..eabe07a03 100644 --- a/src/commands/capture/wait.ts +++ b/src/commands/capture/wait.ts @@ -16,7 +16,7 @@ import { defineExecutableCommand } from '../command-contract.ts'; import { direct, optionalNumber, - recordControlInputFromFlags, + noRecordInputFromFlags, selectionOptionsFromFlags, selectorSnapshotOptionsFromFlags, } from '../cli-grammar/common.ts'; @@ -93,7 +93,7 @@ function readWaitOptionsFromPositionals( const base = { ...selectionOptionsFromFlags(flags), ...selectorSnapshotOptionsFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), }; if (parsed.kind === 'sleep') return { ...base, durationMs: parsed.durationMs }; if (parsed.kind === 'text') { diff --git a/src/commands/cli-grammar/common.ts b/src/commands/cli-grammar/common.ts index 1e0cbb83e..e7c998e9c 100644 --- a/src/commands/cli-grammar/common.ts +++ b/src/commands/cli-grammar/common.ts @@ -70,12 +70,35 @@ export function commonInputFromFlags(flags: CliFlags): Record { // it: command-flags.ts maps options.noRecord onto the daemon request, and // recordActionEntry reads it from there. A reader that never names it accepts // the flag and silently drops it. -export function recordControlInputFromFlags(flags: CliFlags): Record { +// +// #1271 stage 2: `--record` deliberately does NOT ride along here. It applies +// only to observation-only commands (snapshot/get/is/find — the ones the +// repair-segment default exclusion can drop), so it gets its own helper below +// rather than an `allowRecord` policy argument on this one. The capability is +// the helper's NAME: a mutating reader physically cannot forward `--record` +// without spreading a helper whose name says it is for observations, whereas a +// boolean policy arg would let a future mutating reader opt in by flipping a +// literal with no schema change — the fail-open this split exists to prevent. +export function noRecordInputFromFlags(flags: CliFlags): Record { return compactRecord({ noRecord: flags.noRecord, }); } +/** + * #1271 stage 2 (ADR 0012 decision 6 amendment): the `--record` opt-in that + * forces an observation-only action into a repair-armed heal. Spread ONLY by + * readers whose command can be excluded by default — `snapshot`, `get`, `is`, + * and `find`. Every one of those readers must ALSO spread + * `noRecordInputFromFlags`, which stays universal (`--no-record` applies to + * every recordable command, mutations included). + */ +export function observationRecordInputFromFlags(flags: CliFlags): Record { + return compactRecord({ + record: flags.record, + }); +} + export function selectionOptionsFromFlags(flags: CliFlags): SelectionOptions { return { platform: flags.platform, diff --git a/src/commands/cli-grammar/flag-definitions-action.ts b/src/commands/cli-grammar/flag-definitions-action.ts index 709fb670d..b255c3d7c 100644 --- a/src/commands/cli-grammar/flag-definitions-action.ts +++ b/src/commands/cli-grammar/flag-definitions-action.ts @@ -294,4 +294,12 @@ export const ACTION_FLAG_DEFINITIONS: readonly FlagDefinition[] = [ usageLabel: '--no-record', usageDescription: 'Do not record this action', }, + { + key: 'record', + names: ['--record'], + type: 'boolean', + usageLabel: '--record', + usageDescription: + 'Force-record this action even though its command is observation-only and would otherwise be excluded from a repair-armed heal by default (mutually exclusive with --no-record)', + }, ]; diff --git a/src/commands/cli-grammar/flag-groups.ts b/src/commands/cli-grammar/flag-groups.ts index 4c9e784bb..c4d8f3321 100644 --- a/src/commands/cli-grammar/flag-groups.ts +++ b/src/commands/cli-grammar/flag-groups.ts @@ -81,6 +81,12 @@ export const COMMON_COMMAND_SUPPORTED_FLAG_KEYS = flagKeys( 'iosXctestEnvDir', 'androidDeviceAllowlist', 'session', + // `--no-record` is genuinely common: it applies to every recordable command, + // mutations included. `--record` is NOT — it only means anything for the + // observation-only commands the repair-segment exclusion can drop + // (snapshot/get/is/find), so it is scoped per-command via each schema's + // `allowedFlags` instead of being accepted everywhere and silently ignored + // (#1271 stage 2). 'noRecord', ); diff --git a/src/commands/command-flags.ts b/src/commands/command-flags.ts index 9cb784364..548e3df9f 100644 --- a/src/commands/command-flags.ts +++ b/src/commands/command-flags.ts @@ -52,6 +52,7 @@ function buildFlags(options: InternalRequestOptions): CommandFlags { deviceHub: options.deviceHub, testIme: options.testIme, noRecord: options.noRecord, + record: options.record, backMode: options.backMode, metroHost: options.metroHost, metroPort: options.metroPort, diff --git a/src/commands/interaction/index.ts b/src/commands/interaction/index.ts index 413b1d467..e54c85341 100644 --- a/src/commands/interaction/index.ts +++ b/src/commands/interaction/index.ts @@ -60,7 +60,7 @@ const interactionCliSchemas = { get: { usageOverride: 'get text|attrs <@ref|selector>', positionalArgs: ['subcommand', 'target'], - allowedFlags: [...SELECTOR_SNAPSHOT_FLAGS], + allowedFlags: [...SELECTOR_SNAPSHOT_FLAGS, 'record'], }, find: { usageOverride: 'find [value] [--first|--last]', @@ -68,12 +68,12 @@ const interactionCliSchemas = { summary: 'Find an element and act', positionalArgs: ['query', 'action', 'value?'], allowsExtraPositionals: true, - allowedFlags: ['snapshotDepth', 'snapshotRaw', 'findFirst', 'findLast'], + allowedFlags: ['snapshotDepth', 'snapshotRaw', 'findFirst', 'findLast', 'record'], }, is: { positionalArgs: ['predicate', 'selector', 'value?'], allowsExtraPositionals: true, - allowedFlags: [...SELECTOR_SNAPSHOT_FLAGS], + allowedFlags: [...SELECTOR_SNAPSHOT_FLAGS, 'record'], }, click: { usageOverride: 'click ', diff --git a/src/commands/interaction/interactions.ts b/src/commands/interaction/interactions.ts index cf1672c29..6d6995e4a 100644 --- a/src/commands/interaction/interactions.ts +++ b/src/commands/interaction/interactions.ts @@ -23,7 +23,8 @@ import { optionalNumber, readElementTargetFromPositionals, readGetFormat, - recordControlInputFromFlags, + noRecordInputFromFlags, + observationRecordInputFromFlags, request, requiredDaemonString, repeatedInputFromFlags, @@ -36,7 +37,7 @@ import type { CliReader, DaemonWriter } from '../cli-grammar/types.ts'; export const interactionCliReaders = { click: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), ...repeatedInputFromFlags(flags), ...settleInputFromFlags(flags), @@ -46,7 +47,7 @@ export const interactionCliReaders = { }), press: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), ...repeatedInputFromFlags(flags), ...settleInputFromFlags(flags), @@ -57,7 +58,7 @@ export const interactionCliReaders = { const decoded = readLongPressTargetFromPositionals(positionals); return { ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), ...settleInputFromFlags(flags), target: targetInputFromClientTarget(decoded), @@ -66,7 +67,7 @@ export const interactionCliReaders = { }, swipe: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), ...swipePayloadFromPositionals(positionals, { count: flags.count, pauseMs: flags.pauseMs, @@ -75,13 +76,13 @@ export const interactionCliReaders = { }), focus: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), x: Number(positionals[0]), y: Number(positionals[1]), }), type: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), text: positionals.join(' '), delayMs: flags.delayMs, }), @@ -89,7 +90,7 @@ export const interactionCliReaders = { const decoded = readFillTargetFromPositionals(positionals); return { ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), ...settleInputFromFlags(flags), target: targetInputFromClientTarget(decoded.target), @@ -100,15 +101,20 @@ export const interactionCliReaders = { }, scroll: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), direction: readScrollDirection(positionals[0]), amount: optionalCliNumber(positionals[1]), pixels: flags.pixels, durationMs: flags.durationMs, }), + // The one observation-only reader in this file: `get` can be excluded from a + // repair-armed heal by default, so it also takes the `--record` opt-in + // (#1271 stage 2). Every other reader here is a mutation and takes only + // `--no-record`. get: (positionals, flags) => ({ ...commonInputFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), + ...observationRecordInputFromFlags(flags), ...selectorSnapshotInputFromFlags(flags), format: readGetFormat(positionals[0]), target: targetInputFromClientTarget(readElementTargetFromPositionals(positionals.slice(1))), diff --git a/src/commands/interaction/metadata.ts b/src/commands/interaction/metadata.ts index be7f0651d..9949ccd71 100644 --- a/src/commands/interaction/metadata.ts +++ b/src/commands/interaction/metadata.ts @@ -168,10 +168,22 @@ const scrollFields = { }), }; +// #1271 stage 2: `get`/`is`/`find` are observation-only and excluded from a +// repair-armed heal by default (ADR 0012 amendment). Both flags are exposed +// here so the Node SDK's typed options and the MCP tool schema can actually +// set them, mirroring `--no-record`/`--record` on the CLI. +const recordControlFields = () => ({ + noRecord: booleanField('Do not record this action.'), + record: booleanField( + 'Force-record this action even though its command is observation-only and would otherwise be excluded from a repair-armed heal by default (mutually exclusive with noRecord).', + ), +}); + const getFields = { format: requiredField(enumField(['text', 'attrs'] as const)), target: requiredField(elementTargetField()), ...selectorSnapshotFields(), + ...recordControlFields(), }; const isFields = { @@ -181,6 +193,7 @@ const isFields = { selector: requiredField(stringField()), value: stringField(), ...selectorSnapshotFields(), + ...recordControlFields(), }; const findFields = { @@ -193,6 +206,7 @@ const findFields = { last: booleanField(), depth: integerField(), raw: booleanField(), + ...recordControlFields(), }; const gestureFields = { diff --git a/src/commands/interaction/selectors.ts b/src/commands/interaction/selectors.ts index e8ffef175..b7c4cbf5c 100644 --- a/src/commands/interaction/selectors.ts +++ b/src/commands/interaction/selectors.ts @@ -11,7 +11,8 @@ import { direct, optionalCliNumber, optionalNumber, - recordControlInputFromFlags, + noRecordInputFromFlags, + observationRecordInputFromFlags, request, selectionOptionsFromFlags, selectorSnapshotOptionsFromFlags, @@ -65,7 +66,8 @@ function readFindOptionsFromPositionals(positionals: string[], flags: CliFlags): const base = { ...findSnapshotOptionsFromFlags(flags), ...selectionOptionsFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), + ...observationRecordInputFromFlags(flags), first: flags.findFirst, last: flags.findLast, }; @@ -113,7 +115,8 @@ function readIsOptionsFromPositionals(positionals: string[], flags: CliFlags): I const base = { ...selectorSnapshotOptionsFromFlags(flags), ...selectionOptionsFromFlags(flags), - ...recordControlInputFromFlags(flags), + ...noRecordInputFromFlags(flags), + ...observationRecordInputFromFlags(flags), }; const normalized = normalizeIsPositionals(positionals); const predicate = normalized[0]; diff --git a/src/contracts/cli-flags.ts b/src/contracts/cli-flags.ts index d628ba880..445b9033c 100644 --- a/src/contracts/cli-flags.ts +++ b/src/contracts/cli-flags.ts @@ -116,6 +116,16 @@ export type CliFlags = CloudProviderProfileFields & headless?: boolean; restart?: boolean; noRecord?: boolean; + /** + * #1271 stage 2 (ADR 0012 amendment): opt-in that forces THIS action into + * a repair-armed heal even though its command is observation-only + * (`snapshot`/`get`/`is`/`find`) — the corrective-read case, where the + * diverged step's repair is itself a read. Mutually exclusive with + * `noRecord` (rejected as `INVALID_ARGS` if both are set); a no-op + * outside a repair-armed session and on any non-observation-only + * command. + */ + record?: boolean; retainPaths?: boolean; retentionMs?: number; replayUpdate?: boolean; diff --git a/src/daemon/__tests__/request-router-record-flags.test.ts b/src/daemon/__tests__/request-router-record-flags.test.ts new file mode 100644 index 000000000..a87362500 --- /dev/null +++ b/src/daemon/__tests__/request-router-record-flags.test.ts @@ -0,0 +1,55 @@ +/** + * #1271 stage 2 (ADR 0012 amendment): `--record` and `--no-record` express + * opposite recording intents for the same action, so both together is + * rejected uniformly for every surface (CLI/Node client/MCP all funnel + * through this same request entry point) — before any session/device work, + * so no session or device setup is needed to exercise it. + */ +import { test, expect } from 'vitest'; +import path from 'node:path'; +import os from 'node:os'; +import { createRequestHandler } from '../request-router.ts'; +import { LeaseRegistry } from '../lease-registry.ts'; +import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts'; + +function createHandler() { + return createRequestHandler({ + logPath: path.join(os.tmpdir(), 'daemon.log'), + token: 'test-token', + sessionStore: makeSessionStore('agent-device-router-record-flags-'), + leaseRegistry: new LeaseRegistry(), + trackDownloadableArtifact: () => 'artifact-id', + }); +} + +test('--record and --no-record together are rejected as INVALID_ARGS before any command runs', async () => { + const handler = createHandler(); + const response = await handler({ + token: 'test-token', + session: 'default', + command: 'get', + positionals: ['attrs', '@e1'], + flags: { record: true, noRecord: true }, + }); + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('INVALID_ARGS'); + expect(response.error.message).toMatch(/--record/); + expect(response.error.message).toMatch(/mutually exclusive/); +}); + +test('--record alone is accepted (rejected only by the pairing, not the flag itself)', async () => { + const handler = createHandler(); + const response = await handler({ + token: 'test-token', + session: 'default', + command: 'get', + positionals: ['attrs', '@e1'], + flags: { record: true }, + }); + // No active session for this ref-based get — fails downstream, but NOT on + // the mutual-exclusivity check. + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.message).not.toMatch(/mutually exclusive/); +}); diff --git a/src/daemon/__tests__/selector-recording.test.ts b/src/daemon/__tests__/selector-recording.test.ts new file mode 100644 index 000000000..a3d88c738 --- /dev/null +++ b/src/daemon/__tests__/selector-recording.test.ts @@ -0,0 +1,70 @@ +/** + * #1271 stage 2 (ADR 0012 amendment): `recordIfSession`'s classification of + * which commands are observation-only for the repair-segment default + * exclusion (`recordActionEntry`, `session-action-recorder.ts`). `get`/`is`/ + * a read-only `find` are observation-only; the top-level `wait` command is + * deliberately excluded from that set (flow timing/synchronisation, not + * observation) and must keep recording unconditionally even while + * repair-armed. + */ +import { test, expect } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { recordIfSession } from '../selector-recording.ts'; +import { SessionStore } from '../session-store.ts'; +import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts'; +import type { DaemonRequest } from '../types.ts'; + +function makeStore(): SessionStore { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-selector-recording-')); + return new SessionStore(path.join(root, 'sessions')); +} + +function req(command: string, flags: DaemonRequest['flags'] = {}): DaemonRequest { + return { token: 't', session: 'default', command, positionals: [], flags }; +} + +test('a repair-armed session excludes get/is/find by default but keeps recording wait', () => { + const store = makeStore(); + const session = makeIosSession('default', { saveScriptBoundary: 0 }); + store.set('default', session); + + recordIfSession(store, 'default', req('get'), {}); + recordIfSession(store, 'default', req('is'), {}); + recordIfSession(store, 'default', req('find'), {}); + recordIfSession(store, 'default', req('wait'), {}); + + expect(store.get('default')!.actions.map((a) => a.command)).toEqual(['wait']); +}); + +test('--record forces get/is/find through even while repair-armed', () => { + const store = makeStore(); + const session = makeIosSession('default', { saveScriptBoundary: 0 }); + store.set('default', session); + + recordIfSession(store, 'default', req('get', { record: true }), {}); + recordIfSession(store, 'default', req('is', { record: true }), {}); + recordIfSession(store, 'default', req('find', { record: true }), {}); + + expect(store.get('default')!.actions.map((a) => a.command)).toEqual(['get', 'is', 'find']); +}); + +test('outside a repair-armed session, get/is/find/wait all record normally', () => { + const store = makeStore(); + const session = makeIosSession('default'); + expect(session.saveScriptBoundary).toBeUndefined(); + store.set('default', session); + + recordIfSession(store, 'default', req('get'), {}); + recordIfSession(store, 'default', req('is'), {}); + recordIfSession(store, 'default', req('find'), {}); + recordIfSession(store, 'default', req('wait'), {}); + + expect(store.get('default')!.actions.map((a) => a.command)).toEqual([ + 'get', + 'is', + 'find', + 'wait', + ]); +}); diff --git a/src/daemon/__tests__/session-action-recorder.test.ts b/src/daemon/__tests__/session-action-recorder.test.ts new file mode 100644 index 000000000..85b8d53b8 --- /dev/null +++ b/src/daemon/__tests__/session-action-recorder.test.ts @@ -0,0 +1,88 @@ +/** + * #1271 stage 2 (ADR 0012 amendment): `recordActionEntry`'s repair-segment + * default exclusion — the single daemon-side choke point every recording + * surface (CLI/Node client/MCP) funnels through via `SessionStore.recordAction`. + * Isolates the mechanism itself from the classification wiring + * (`selector-recording.test.ts` covers which commands set `interactiveObservation`). + */ +import { test, expect } from 'vitest'; +import { recordActionEntry } from '../session-action-recorder.ts'; +import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts'; + +test('an observation-only action is excluded while repair-armed and no --record is given', () => { + const session = makeIosSession('default', { saveScriptBoundary: 0 }); + const action = recordActionEntry(session, { + command: 'get', + positionals: ['attrs', 'id="save"'], + flags: {}, + result: {}, + interactiveObservation: true, + }); + expect(action).toBeUndefined(); + expect(session.actions).toHaveLength(0); +}); + +test('--record forces an observation-only action through while repair-armed', () => { + const session = makeIosSession('default', { saveScriptBoundary: 0 }); + const action = recordActionEntry(session, { + command: 'get', + positionals: ['attrs', 'id="save"'], + flags: { record: true }, + result: {}, + interactiveObservation: true, + }); + expect(action).toBeDefined(); + expect(session.actions.map((a) => a.command)).toEqual(['get']); +}); + +test('an observation-only action records normally outside a repair-armed session (ordinary authoring recording is unchanged)', () => { + const session = makeIosSession('default'); + expect(session.saveScriptBoundary).toBeUndefined(); + const action = recordActionEntry(session, { + command: 'is', + positionals: ['visible', 'id="save"'], + flags: {}, + result: {}, + interactiveObservation: true, + }); + expect(action).toBeDefined(); + expect(session.actions.map((a) => a.command)).toEqual(['is']); +}); + +test('a mutating action is never excluded, repair-armed or not', () => { + const session = makeIosSession('default', { saveScriptBoundary: 0 }); + const action = recordActionEntry(session, { + command: 'press', + positionals: ['@e5'], + flags: {}, + result: {}, + }); + expect(action).toBeDefined(); + expect(session.actions.map((a) => a.command)).toEqual(['press']); +}); + +test('a command explicitly marked NOT observation-only (e.g. the top-level `wait`) always records, even while repair-armed', () => { + const session = makeIosSession('default', { saveScriptBoundary: 0 }); + const action = recordActionEntry(session, { + command: 'wait', + positionals: ['500'], + flags: {}, + result: {}, + interactiveObservation: false, + }); + expect(action).toBeDefined(); + expect(session.actions.map((a) => a.command)).toEqual(['wait']); +}); + +test('--no-record still takes precedence over an observation-only action, repair-armed or not', () => { + const session = makeIosSession('default', { saveScriptBoundary: 0 }); + const action = recordActionEntry(session, { + command: 'get', + positionals: ['attrs', 'id="save"'], + flags: { noRecord: true }, + result: {}, + interactiveObservation: true, + }); + expect(action).toBeUndefined(); + expect(session.actions).toHaveLength(0); +}); diff --git a/src/daemon/handlers/__tests__/session-replay-repair-record-exclusion.test.ts b/src/daemon/handlers/__tests__/session-replay-repair-record-exclusion.test.ts new file mode 100644 index 000000000..861cf8cc1 --- /dev/null +++ b/src/daemon/handlers/__tests__/session-replay-repair-record-exclusion.test.ts @@ -0,0 +1,409 @@ +/** + * #1271 stage 2 (ADR 0012 amendment): repair-segment default exclusion of + * observation-only commands (`snapshot`/`get`/`is`/a read-only `find`), the + * `--record` opt-in for the corrective-read case, and the fail-loud empty- + * segment guard. Exercised end to end at the same layer the ADR 0012 decision + * 6 lifecycle tests use — `runReplayScriptFile` + `handleCloseCommand` sharing + * a live `SessionStore`, exactly like an agent's separate CLI invocations + * against the same daemon session. + * + * Required regression coverage (per the maintainer's triage on #1271): + * - a diagnostic read inside a repair segment is omitted from the heal; + * - a corrective action remains — both a mutating press (never + * observation-only) and a `--record`ed read (the diverged-step-was-a-`get` + * case); + * - the resulting healed script replays cleanly; + * - the empty-segment guard refuses with the actionable `--record` error; + * - non-repair authoring recording (a fresh `open --save-script` session) is + * completely unchanged. + */ +import { test, expect, vi, beforeEach } from 'vitest'; + +vi.mock('../../../platforms/apple/core/simulator.ts', async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, shutdownSimulator: vi.fn() }; +}); +vi.mock('../../../utils/exec.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, runCmd: vi.fn() }; +}); +vi.mock('../../../platforms/apple/core/runner/runner-client.ts', async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, stopIosRunnerSession: vi.fn(async () => {}) }; +}); +vi.mock('../../../platforms/apple/core/perf-xctrace.ts', async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, cleanupAppleXctracePerfCapture: vi.fn(async () => ({})) }; +}); +vi.mock('../../runtime-hints.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, clearRuntimeHintsFromApp: vi.fn(async () => {}) }; +}); +vi.mock('../../../core/dispatch.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, dispatchCommand: vi.fn(async () => ({})), resolveTargetDevice: vi.fn() }; +}); +vi.mock('../session-device-utils.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, settleIosSimulator: vi.fn(async () => {}) }; +}); + +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { runReplayScriptFile } from '../session-replay-runtime.ts'; +import { handleCloseCommand } from '../session-close.ts'; +import { SessionStore } from '../../session-store.ts'; +import { LeaseRegistry } from '../../lease-registry.ts'; +import { dispatchCommand } from '../../../core/dispatch.ts'; +import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts'; +import { parseReplayScriptDetailed } from '../../../replay/script.ts'; +import { + baseReplayRequest as baseReq, + writeReplayFile, +} from './session-replay-runtime.fixtures.ts'; +import { freshEvidence, makeRecordingReplayInvoke } from './session-replay-repair.fixtures.ts'; + +const mockDispatchCommand = vi.mocked(dispatchCommand); + +beforeEach(() => { + mockDispatchCommand.mockReset(); + // The "current" app state: "save" was renamed to "save-v2" (why the click + // on `id="save"` diverges as a `selector-miss`) — nested under a "Toolbar" + // container so the recorded ancestry (below) has a real, present container + // to find (`repairHint`'s R3 container-presence test requires a non-empty + // ancestry/scrollRegion signal, else it fails safe to `manual`). "confirm" + // is also present so the surviving `click id="confirm"` step (freshly + // annotated during the repair) and the healed script's fresh re-replay both + // verify cleanly. + mockDispatchCommand.mockResolvedValue({ + nodes: [ + { + index: 0, + depth: 0, + type: 'View', + label: 'Toolbar', + rect: { x: 0, y: 0, width: 200, height: 400 }, + }, + { + index: 1, + depth: 1, + type: 'Button', + identifier: 'save-v2', + label: 'Save V2', + parentIndex: 0, + rect: { x: 10, y: 10, width: 40, height: 20 }, + }, + { + index: 2, + depth: 1, + type: 'Button', + identifier: 'confirm', + label: 'Confirm', + parentIndex: 0, + rect: { x: 10, y: 40, width: 40, height: 20 }, + }, + ], + truncated: false, + backend: 'xctest', + }); +}); + +const SAVE_ANNOTATION = + '# agent-device:target-v1 {"id":"save","role":"button","label":"Save","ancestry":[{"role":"view","label":"Toolbar"}],"sibling":0,"viewportOrder":0,"verification":"verified"}'; + +function setup(prefix: string) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + const sessionStore = new SessionStore(path.join(root, 'sessions')); + const sessionName = 'default'; + sessionStore.set(sessionName, makeIosSession(sessionName, { appBundleId: 'com.example.app' })); + return { + root, + sessionStore, + sessionName, + logPath: path.join(root, 'daemon.log'), + leaseRegistry: new LeaseRegistry(), + }; +} + +/** Drives leg 1 of the repair: arm + diverge on the renamed `id="save"`. */ +async function armAndDiverge(params: ReturnType, filePath: string) { + const { sessionStore, sessionName, logPath } = params; + const invoke = makeRecordingReplayInvoke({ + sessionStore, + sessionName, + evidence: (req) => (req.command === 'click' ? freshEvidence('confirm', 'Confirm') : undefined), + }); + const leg1 = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath], flags: { saveScript: true } }), + sessionName, + logPath, + sessionStore, + invoke, + }); + expect(leg1.ok).toBe(false); + if (leg1.ok) throw new Error('expected a divergence'); + const divergence = leg1.error.details?.divergence as { + kind: string; + repairHint: string; + resume: { allowed: boolean; from: number; planDigest: string; repairSessionHeld?: boolean }; + }; + expect(divergence.kind).toBe('selector-miss'); + expect(divergence.repairHint).toBe('record-and-heal'); + expect(divergence.resume.allowed).toBe(true); + expect(divergence.resume.from).toBe(3); + expect(divergence.resume.repairSessionHeld).toBe(true); + return { invoke, divergence }; +} + +test('diagnostic get/is reads mid-repair are excluded from the healed script by default; the corrective press remains', async () => { + const ctx = setup('agent-device-repair-record-exclusion-press-'); + const filePath = writeReplayFile(ctx.root, [ + 'open "Demo" --relaunch', + SAVE_ANNOTATION, + 'click id="save"', + 'click id="confirm"', + 'close', + ]); + const { invoke, divergence } = await armAndDiverge(ctx, filePath); + const session = ctx.sessionStore.get(ctx.sessionName)!; + + // --- The agent explores mid-repair: a couple of diagnostic reads to locate + // the renamed control. Both are observation-only and the session is + // repair-armed, so per the default exclusion neither is appended. --- + ctx.sessionStore.recordAction(session, { + command: 'get', + positionals: ['attrs', '@e5'], + flags: {}, + result: { selectorChain: ['id="save-v2"'] }, + interactiveObservation: true, + }); + ctx.sessionStore.recordAction(session, { + command: 'is', + positionals: ['visible', 'id="save-v2"'], + flags: {}, + result: {}, + interactiveObservation: true, + }); + // Excluded: `session.actions` did not grow past the recorded `open`. + expect(session.actions.map((a) => a.command)).toEqual(['open']); + + // --- The agent performs the corrective press (blessed @ref), recorded live — + // a mutating action is never observation-only, so it is unaffected. --- + ctx.sessionStore.recordAction(session, { + command: 'press', + positionals: ['@e7'], + flags: {}, + result: { selectorChain: ['id="save-v2"'] }, + targetEvidence: freshEvidence('save-v2', 'Save V2'), + }); + expect(session.actions.map((a) => a.command)).toEqual(['open', 'press']); + + // --- `replay --from N+1` resumes to completion (terminal close skipped). --- + const leg2 = await runReplayScriptFile({ + req: baseReq({ + positionals: [filePath], + flags: { saveScript: true, replayFrom: 3, replayPlanDigest: divergence.resume.planDigest }, + }), + sessionName: ctx.sessionName, + logPath: ctx.logPath, + sessionStore: ctx.sessionStore, + invoke, + }); + expect(leg2.ok).toBe(true); + expect(session.actions.map((a) => a.command)).toEqual(['open', 'press', 'click']); + expect(session.saveScriptComplete).toBe(true); + + // --- Finalize: `close --save-script` commits the healed `.ad`. --- + const closeResponse = await handleCloseCommand({ + req: { + token: 't', + session: ctx.sessionName, + command: 'close', + positionals: [], + flags: { saveScript: true }, + }, + sessionName: ctx.sessionName, + logPath: ctx.logPath, + sessionStore: ctx.sessionStore, + leaseRegistry: ctx.leaseRegistry, + }); + expect(closeResponse.ok).toBe(true); + + const healedPath = path.join(ctx.root, 'flow.healed.ad'); + const healedScript = fs.readFileSync(healedPath, 'utf8'); + expect(healedScript).not.toContain('get attrs'); + expect(healedScript).not.toContain('is visible'); + const parsed = parseReplayScriptDetailed(healedScript); + // Exactly the repair run's own execution path: open, the corrective press, + // the surviving click, and the agent's own close — the diagnostic get/is + // never appear, and neither does the source plan's original (skipped) close. + expect(parsed.actions.map((a) => a.command)).toEqual(['open', 'press', 'click', 'close']); + + // --- The healed script replays cleanly in a FRESH session. --- + const fresh = setup('agent-device-repair-record-exclusion-press-fresh-'); + const freshInvoke = makeRecordingReplayInvoke({ + sessionStore: fresh.sessionStore, + sessionName: fresh.sessionName, + }); + const freshRun = await runReplayScriptFile({ + req: baseReq({ positionals: [healedPath] }), + sessionName: fresh.sessionName, + logPath: fresh.logPath, + sessionStore: fresh.sessionStore, + invoke: freshInvoke, + }); + expect(freshRun.ok).toBe(true); +}); + +test("a --record'ed diagnostic read lands in the healed script (the diverged-step-was-a-get case)", async () => { + const ctx = setup('agent-device-repair-record-exclusion-record-'); + const filePath = writeReplayFile(ctx.root, [ + 'open "Demo" --relaunch', + SAVE_ANNOTATION, + 'click id="save"', + 'click id="confirm"', + 'close', + ]); + const { invoke, divergence } = await armAndDiverge(ctx, filePath); + const session = ctx.sessionStore.get(ctx.sessionName)!; + + // --- The agent's correction for the diverged step IS itself a read (the + // wave-3 E3 shape): `get attrs` on the renamed control, explicitly forced + // into the heal with `--record` since it would otherwise be excluded. --- + ctx.sessionStore.recordAction(session, { + command: 'get', + positionals: ['attrs', '@e7'], + flags: { record: true }, + result: { selectorChain: ['id="save-v2"'] }, + interactiveObservation: true, + }); + expect(session.actions.map((a) => a.command)).toEqual(['open', 'get']); + + const leg2 = await runReplayScriptFile({ + req: baseReq({ + positionals: [filePath], + flags: { saveScript: true, replayFrom: 3, replayPlanDigest: divergence.resume.planDigest }, + }), + sessionName: ctx.sessionName, + logPath: ctx.logPath, + sessionStore: ctx.sessionStore, + invoke, + }); + expect(leg2.ok).toBe(true); + expect(session.actions.map((a) => a.command)).toEqual(['open', 'get', 'click']); + + const closeResponse = await handleCloseCommand({ + req: { + token: 't', + session: ctx.sessionName, + command: 'close', + positionals: [], + flags: { saveScript: true }, + }, + sessionName: ctx.sessionName, + logPath: ctx.logPath, + sessionStore: ctx.sessionStore, + leaseRegistry: ctx.leaseRegistry, + }); + expect(closeResponse.ok).toBe(true); + + const healedPath = path.join(ctx.root, 'flow.healed.ad'); + const healedScript = fs.readFileSync(healedPath, 'utf8'); + const parsed = parseReplayScriptDetailed(healedScript); + // The --record'ed corrective read DOES land in the heal, resolved to a + // selector (never a bare @ref). + expect(parsed.actions.map((a) => a.command)).toEqual(['open', 'get', 'click', 'close']); + const getAction = parsed.actions.find((a) => a.command === 'get'); + expect(getAction?.positionals).toEqual(['attrs', 'id="save-v2"']); + const bareRefs = parsed.actions.flatMap((a) => a.positionals.filter((p) => p.startsWith('@'))); + expect(bareRefs).toEqual([]); + + // --- Replays cleanly in a fresh session too. --- + const fresh = setup('agent-device-repair-record-exclusion-record-fresh-'); + const freshInvoke = makeRecordingReplayInvoke({ + sessionStore: fresh.sessionStore, + sessionName: fresh.sessionName, + }); + const freshRun = await runReplayScriptFile({ + req: baseReq({ positionals: [healedPath] }), + sessionName: fresh.sessionName, + logPath: fresh.logPath, + sessionStore: fresh.sessionStore, + invoke: freshInvoke, + }); + expect(freshRun.ok).toBe(true); +}); + +test('empty-segment guard: a --from resume refuses with an actionable --record hint when only excluded diagnostic reads happened since the divergence', async () => { + const ctx = setup('agent-device-repair-record-exclusion-empty-segment-'); + const filePath = writeReplayFile(ctx.root, [ + 'open "Demo" --relaunch', + SAVE_ANNOTATION, + 'click id="save"', + 'click id="confirm"', + 'close', + ]); + const { invoke, divergence } = await armAndDiverge(ctx, filePath); + const session = ctx.sessionStore.get(ctx.sessionName)!; + + // --- The agent ONLY inspects — never performs a corrective action. Both + // reads are excluded from `session.actions`, so nothing was recorded in + // this repair segment. --- + ctx.sessionStore.recordAction(session, { + command: 'get', + positionals: ['attrs', '@e5'], + flags: {}, + result: { selectorChain: ['id="save-v2"'] }, + interactiveObservation: true, + }); + ctx.sessionStore.recordAction(session, { + command: 'find', + positionals: ['id', 'save-v2', 'exists'], + flags: {}, + result: { found: true }, + interactiveObservation: true, + }); + expect(session.actions.map((a) => a.command)).toEqual(['open']); + + const blindResume = await runReplayScriptFile({ + req: baseReq({ + positionals: [filePath], + flags: { saveScript: true, replayFrom: 3, replayPlanDigest: divergence.resume.planDigest }, + }), + sessionName: ctx.sessionName, + logPath: ctx.logPath, + sessionStore: ctx.sessionStore, + invoke, + }); + expect(blindResume.ok).toBe(false); + if (blindResume.ok) return; + expect(blindResume.error.code).toBe('INVALID_ARGS'); + expect(blindResume.error.message).toMatch(/no corrective action/); + expect(blindResume.error.message).toMatch(/--record/); + // Refused before any device action ran — the excluded reads are the only + // activity since the divergence. + expect(session.actions.map((a) => a.command)).toEqual(['open']); +}); + +test('non-repair authoring recording is unchanged: a read in a fresh `open --save-script` session still records with no flag needed', async () => { + const ctx = setup('agent-device-repair-record-exclusion-authoring-'); + // An ordinary, non-repair recording session: no `saveScriptBoundary` (never + // armed by a repair `replay --save-script`). + const session = ctx.sessionStore.get(ctx.sessionName)!; + session.recordSession = true; + expect(session.saveScriptBoundary).toBeUndefined(); + + ctx.sessionStore.recordAction(session, { + command: 'get', + positionals: ['attrs', '@e5'], + flags: {}, + result: { selectorChain: ['id="save"'] }, + interactiveObservation: true, + }); + // Recorded normally — no `--record` needed, no repair segment in play. + expect(session.actions.map((a) => a.command)).toEqual(['get']); +}); diff --git a/src/daemon/handlers/__tests__/session-replay-repair.fixtures.ts b/src/daemon/handlers/__tests__/session-replay-repair.fixtures.ts index da72b2c87..d6f2e450f 100644 --- a/src/daemon/handlers/__tests__/session-replay-repair.fixtures.ts +++ b/src/daemon/handlers/__tests__/session-replay-repair.fixtures.ts @@ -8,6 +8,7 @@ */ import type { DaemonInvokeFn, DaemonRequest, DaemonResponse } from '../../types.ts'; import { SessionStore } from '../../session-store.ts'; +import { isInteractiveObservation } from '../../session-action-recorder.ts'; import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts'; import type { TargetAnnotationV1 } from '../../../replay/target-identity.ts'; @@ -63,6 +64,13 @@ export function makeRecordingReplayInvoke(config: RecordingReplayInvokeConfig): flags: req.flags ?? {}, runtime: req.runtime, result: {}, + // #1271 stage 2: the PRODUCTION classifier, not a mirror of it — this + // mock stands in for the real handlers' recording call, so it must make + // the same interactive-vs-authored decision they do. It reads + // `internal.replayPlanStep`, which the real `invokeReplayAction` stamps + // on every plan step, so a replayed observation is correctly treated as + // authored here without the fixture knowing anything about provenance. + interactiveObservation: isInteractiveObservation(req), ...(evidence ? { targetEvidence: evidence } : {}), }); return { ok: true, data: {} }; diff --git a/src/daemon/handlers/find.ts b/src/daemon/handlers/find.ts index a3e7fcf23..9073771f7 100644 --- a/src/daemon/handlers/find.ts +++ b/src/daemon/handlers/find.ts @@ -1,6 +1,7 @@ import { dispatchCommand } from '../../core/dispatch.ts'; import { findBestMatchesByLocator, + isReadOnlyFindAction, parseFindArgs, parseFindSelectorExpression, type FindLocator, @@ -77,6 +78,18 @@ export async function handleFindCommands(params: { if (req.flags?.findFirst && req.flags?.findLast) { return errorResponse('INVALID_ARGS', 'find accepts only one of --first or --last'); } + // #1271 stage 2: `--record` only means something for an action the + // repair-segment exclusion can drop. `find`'s observe-vs-mutate split is a + // POSITIONAL, so unlike snapshot/get/is it cannot be settled by the CLI + // grammar's per-command `allowedFlags` — it is validated here instead, before + // any device work, so every surface (CLI/Node/MCP) inherits the same refusal + // rather than silently ignoring the flag on a mutating find. + if (req.flags?.record && !isReadOnlyFindAction(action)) { + return errorResponse( + 'INVALID_ARGS', + `find ${action} is a mutating action and is always recorded; --record only applies to a read-only find (exists, wait, get text, get attrs).`, + ); + } const runtimeResponse = await dispatchFindReadOnlyViaRuntime({ req, sessionName, diff --git a/src/daemon/handlers/session-replay-action-runtime.ts b/src/daemon/handlers/session-replay-action-runtime.ts index c541676d2..afb1734e6 100644 --- a/src/daemon/handlers/session-replay-action-runtime.ts +++ b/src/daemon/handlers/session-replay-action-runtime.ts @@ -120,7 +120,14 @@ async function invokeResolvedReplayAction(params: { flags, runtime: resolved.runtime, meta: req.meta, - internal: req.internal, + // #1271 stage 2: the single point every replay plan step is dispatched, so + // the one place authored provenance can be stamped for all of them — + // annotated or not, target-binding kinds and plain dispatches alike. The + // repair-segment exclusion (`isInteractiveObservation`, + // `session-action-recorder.ts`) reads it to keep an authored + // `get`/`is`/`find`/`snapshot` step in its own healed script while still + // excluding an interactive diagnostic read of the same command. + internal: { ...req.internal, replayPlanStep: true }, }; return await invoke(buildReplayInteractionRequest(baseReq, resolved)); } diff --git a/src/daemon/handlers/session-replay-runtime-plan.ts b/src/daemon/handlers/session-replay-runtime-plan.ts index 394ca0ac4..9913d6973 100644 --- a/src/daemon/handlers/session-replay-runtime-plan.ts +++ b/src/daemon/handlers/session-replay-runtime-plan.ts @@ -153,6 +153,19 @@ function describeOutOfRangeResumeFrom(params: { * `resume.from` stays at the failed step unchanged and is never subject to * this check (it never matches `expectedFrom`, which only ever targets * `failedIndex + 1`), so the message below is intentionally hint-neutral. + * + * #1271 stage 2 (ADR 0012 amendment): this same growth check is now also the + * repair-segment empty-heal guard. Observation-only actions + * (`snapshot`/`get`/`is`/`find`) are, by default, excluded from + * `session.actions` while repair-armed (`isExcludedRepairSegmentObservation`, + * `session-action-recorder.ts`), so a repair segment containing ONLY + * unrecorded diagnostic reads never grows `sessionActionsLength` either — + * this check refuses it exactly as it already refused "no corrective press + * happened," converting the corrective-read case's one silent-failure mode + * (an excluded read silently missing from the heal) into this same loud + * rejection. The message therefore names `--record` alongside the existing + * `--no-record` mention, since the missing corrective action may have been a + * read rather than a press. */ function describeUnperformedRecordAndHeal(params: { from: number; @@ -167,9 +180,10 @@ function describeUnperformedRecordAndHeal(params: { return undefined; } return ( - `replay --from ${from} continues a record-and-heal-shaped repair, but no corrective action has ` + - 'been recorded on this session since that divergence; press the correct control via a blessed ' + - "@ref from the divergence's screen.refs (recorded, no --no-record) before resuming with " + + `replay --from ${from} continues a record-and-heal-shaped repair, but no corrective action was ` + + "recorded in this repair segment; press the correct control via a blessed @ref from the divergence's " + + 'screen.refs (recorded, no --no-record) — or, if your corrective action was a read ' + + '(get/is/find/snapshot), re-run it with --record so it lands in the heal — before resuming with ' + `--from ${from}.` ); } diff --git a/src/daemon/request-router.ts b/src/daemon/request-router.ts index 111891771..978059f72 100644 --- a/src/daemon/request-router.ts +++ b/src/daemon/request-router.ts @@ -10,7 +10,7 @@ import type { CloudArtifactProvider } from '../cloud-artifacts.ts'; import type { DaemonInvokeFn, DaemonRequest, DaemonResponse, DaemonResponseData } from './types.ts'; import { RESPONSE_VIEWS } from './response-views.ts'; import { SessionStore } from './session-store.ts'; -import { noActiveSessionError } from './handlers/response.ts'; +import { errorResponse, noActiveSessionError } from './handlers/response.ts'; import { type AndroidAdbProviderResolver, type AppleRunnerProviderResolver, @@ -135,6 +135,9 @@ export function createRequestHandler(deps: RequestRouterDeps): DaemonInvokeFn { if (!timingSafeStringEqual(req.token, token)) { return unauthorizedResponse(); } + if (req.flags?.record && req.flags?.noRecord) { + return mutuallyExclusiveRecordFlagsResponse(); + } let scope: RequestExecutionScope | undefined; try { @@ -283,6 +286,21 @@ function unauthorizedResponse(): DaemonResponse { }; } +/** + * #1271 stage 2 (ADR 0012 amendment): `--record` and `--no-record` express + * opposite recording intents for the same action — force it into a + * repair-armed heal versus opt it out entirely — so both together is a + * contradiction the daemon rejects up front, uniformly for every surface + * (CLI/Node client/MCP all funnel through this same request entry point), + * rather than silently letting one win. + */ +function mutuallyExclusiveRecordFlagsResponse(): DaemonResponse { + return errorResponse( + 'INVALID_ARGS', + '--record and --no-record are mutually exclusive; pass at most one.', + ); +} + async function dispatchGenericForLockedScope(params: { lockedScope: LockedRequestScope; logPath: string; diff --git a/src/daemon/selector-recording.ts b/src/daemon/selector-recording.ts index 367bffb95..4abe008e9 100644 --- a/src/daemon/selector-recording.ts +++ b/src/daemon/selector-recording.ts @@ -1,5 +1,6 @@ import type { DaemonRequest } from './types.ts'; import { SessionStore } from './session-store.ts'; +import { isInteractiveObservation } from './session-action-recorder.ts'; import { computeTargetEvidence, type RecordedTargetCapture } from './session-target-evidence.ts'; export function buildFindRecordResult( @@ -99,6 +100,11 @@ export function recordIfSession( positionals: req.positionals ?? [], flags: req.flags ?? {}, result, + // #1271 stage 2: the shared recorder for `get`, `is`, `wait`, and read-only + // `find`. `isInteractiveObservation` decides both halves — the command + // class AND the provenance — so an authored plan step recorded through + // this same path keeps its place in the healed script. + interactiveObservation: isInteractiveObservation(req), ...(targetEvidence ? { targetEvidence } : {}), }); } diff --git a/src/daemon/selector-runtime.ts b/src/daemon/selector-runtime.ts index 86cebbe25..11c7d1291 100644 --- a/src/daemon/selector-runtime.ts +++ b/src/daemon/selector-runtime.ts @@ -8,7 +8,7 @@ import type { DaemonRequest, DaemonResponse, SessionState } from './types.ts'; import { errorResponse, requireCommandSupported } from './handlers/response.ts'; import { markSessionPartialRefsIssued, resolveRefStalenessWarning } from './session-snapshot.ts'; import { resolveSessionDevice, withSessionlessRunnerCleanup } from './handlers/snapshot-session.ts'; -import { parseFindArgs, type FindAction } from '../selectors/find.ts'; +import { isReadOnlyFindAction, parseFindArgs } from '../selectors/find.ts'; import { splitIsSelectorArgs } from '../selectors/index.ts'; import { refSnapshotFlagGuardResponse } from './handlers/interaction-flags.ts'; import { parseVersionedRefPositional } from './handlers/interaction-touch-targets.ts'; @@ -610,10 +610,3 @@ async function maybeAndroidForegroundBlockerResponse( ); } -function isReadOnlyFindAction( - action: FindAction['kind'], -): action is 'exists' | 'wait' | 'get_text' | 'get_attrs' { - return ( - action === 'exists' || action === 'wait' || action === 'get_text' || action === 'get_attrs' - ); -} diff --git a/src/daemon/session-action-recorder.ts b/src/daemon/session-action-recorder.ts index a80b6ef64..128f9a499 100644 --- a/src/daemon/session-action-recorder.ts +++ b/src/daemon/session-action-recorder.ts @@ -1,7 +1,7 @@ import type { CommandFlags } from '../core/dispatch.ts'; import { SCREENSHOT_ACTION_FLAG_KEYS } from '../contracts/screenshot.ts'; import { emitDiagnostic } from '../utils/diagnostics.ts'; -import type { SessionAction, SessionRuntimeHints, SessionState } from './types.ts'; +import type { DaemonRequest, SessionAction, SessionRuntimeHints, SessionState } from './types.ts'; import { expandSessionPath } from './session-paths.ts'; import type { TargetAnnotationV1 } from '../replay/target-identity.ts'; @@ -12,6 +12,20 @@ export type RecordActionEntry = { runtime?: SessionRuntimeHints; result?: Record; targetEvidence?: TargetAnnotationV1; + /** + * #1271 stage 2 (ADR 0012 amendment): an observation-only command + * (`snapshot`/`get`/`is`/read-only `find`) dispatched OUT OF BAND — typed by + * the agent mid-repair rather than replayed from the `.ad` plan under + * repair. Computed by `isInteractiveObservation` below; only actions with + * this flag are subject to the repair-segment default exclusion. + * + * The qualifier is load-bearing: command class ALONE is the wrong + * discriminator. An authored `get`/`is`/`find` plan step is the same command + * as an interactive diagnostic read, but it must survive into its own healed + * script — dropping it would make the heal quietly stop asserting what the + * original flow asserted. Provenance separates them. + */ + interactiveObservation?: boolean; }; /** @@ -41,6 +55,7 @@ export function recordActionEntry( entry: RecordActionEntry, ): SessionAction | undefined { if (entry.flags?.noRecord) return undefined; + if (isExcludedRepairSegmentObservation(session, entry)) return undefined; if (entry.flags?.saveScript) { session.recordSession = true; if (typeof entry.flags.saveScript === 'string') { @@ -90,6 +105,79 @@ export function recordActionEntry( return action; } +/** + * #1271 stage 2 (ADR 0012 amendment): observation-only commands — the ONLY + * commands the repair-segment exclusion can drop. `wait` is deliberately + * absent: it is flow timing/synchronisation, not observation, so it always + * records. A mutating `find … click|fill|focus|type` never reaches a caller of + * `isInteractiveObservation` (it records through `recordSessionAction`, + * `handlers/handler-utils.ts`), so `find` here always means a read-only + * sub-action; `diff` is likewise absent because only `snapshot` is classified + * at the snapshot-runtime call site. + */ +const OBSERVATION_ONLY_COMMANDS: ReadonlySet = new Set([ + 'snapshot', + 'get', + 'is', + 'find', +]); + +/** + * #1271 stage 2 (ADR 0012 amendment): is this request an out-of-band + * interactive observation — the only thing a repair segment excludes? + * + * Two facts, ANDed, and the second is the one that matters: + * 1. the command is observation-only (above); and + * 2. it is NOT a replay plan step (`internal.replayPlanStep`, stamped by + * `invokeResolvedReplayAction`, `handlers/session-replay-action-runtime.ts`). + * + * (2) is why this is a PROVENANCE rule, not a command-class rule. Replayed + * plan steps dispatch through the ordinary request path and land in + * `session.actions` like any other action; the healed script is that slice + * (`buildOptimizedActions` over `session.actions.slice(saveScriptBoundary)`). + * So excluding by command class alone would replay an authored `is visible` + * assertion and then silently drop it from its own heal — the healed flow + * would quietly stop checking what it used to check. Authored observations + * must survive automatically; users must never have to annotate their own + * `.ad` steps with `--record`. + */ +export function isInteractiveObservation(req: DaemonRequest): boolean { + if (req.internal?.replayPlanStep === true) return false; + return OBSERVATION_ONLY_COMMANDS.has(req.command); +} + +/** + * #1271 stage 2 (ADR 0012 amendment): the repair-segment default exclusion. + * + * `session.saveScriptBoundary !== undefined` is set ONLY by a repair-armed + * `replay --save-script` (decision 6, R1/R6) — an ordinary, non-repair + * `open --save-script`/`close --save-script` authoring recording never sets + * it (see the ADR's "Scope" note under decision 6). Gating on this field, + * rather than on `session.recordSession` alone, is exactly what keeps + * ordinary authoring recording completely unchanged: a fresh `open + * --save-script` session records every action, including reads, precisely as + * it always has. + * + * Inside a repair-armed session, an out-of-band interactive observation is + * excluded from `session.actions` unless the caller passed `--record`. + * Because the exclusion happens HERE — at the same point `--no-record` is + * enforced — an excluded action never grows `session.actions.length`, which is + * the exact counter `describeUnperformedRecordAndHeal` + * (`session-replay-runtime-plan.ts`) already watches to prove a corrective + * action was recorded since the divergence. That existing fail-loud guard + * therefore correctly refuses a `--from` resume whose only "activity" since + * the divergence was excluded diagnostic reads, with no separate bookkeeping + * needed here. + */ +function isExcludedRepairSegmentObservation( + session: SessionState, + entry: RecordActionEntry, +): boolean { + if (!entry.interactiveObservation) return false; + if (session.saveScriptBoundary === undefined) return false; + return entry.flags?.record !== true; +} + const SANITIZED_FLAG_KEYS = [ 'platform', 'device', @@ -110,6 +198,7 @@ const SANITIZED_FLAG_KEYS = [ 'saveScript', 'force', 'noRecord', + 'record', 'fps', 'quality', 'hideTouches', diff --git a/src/daemon/snapshot-runtime.ts b/src/daemon/snapshot-runtime.ts index 6244f38fc..11d8027a3 100644 --- a/src/daemon/snapshot-runtime.ts +++ b/src/daemon/snapshot-runtime.ts @@ -6,6 +6,7 @@ import { AppError } from '../kernel/errors.ts'; import type { SnapshotDiffSummary } from '../snapshot/snapshot-diff.ts'; import type { DaemonRequest, DaemonResponse, DaemonResponseData, SessionState } from './types.ts'; import { SessionStore } from './session-store.ts'; +import { isInteractiveObservation } from './session-action-recorder.ts'; import { errorResponse, requireCommandSupported } from './handlers/response.ts'; import { captureSnapshot, resolveSnapshotScope } from './handlers/snapshot-capture.ts'; import { snapshotCaptureAnnotationsFrom } from '../snapshot-capture-annotations.ts'; @@ -347,6 +348,12 @@ function recordSnapshotRuntimeAction(params: { positionals: params.req.positionals ?? [], flags: params.req.flags ?? {}, result: toRecordedSnapshotRuntimeResult(params.result), + // #1271 stage 2: `snapshot` is observation-only and subject to the + // repair-segment default exclusion; `diff` reaches this same recorder and + // is deliberately NOT in the classifier's command set, so it is + // unaffected. An authored `snapshot` plan step carries replay provenance + // and stays in the heal. + interactiveObservation: isInteractiveObservation(params.req), }); } diff --git a/src/daemon/types.ts b/src/daemon/types.ts index ef3cbcb08..2f18a54b8 100644 --- a/src/daemon/types.ts +++ b/src/daemon/types.ts @@ -84,6 +84,27 @@ type DaemonRequestInternal = { * side-effect seam and expires the frame). */ findResolvedTarget?: boolean; + /** + * #1271 stage 2 (ADR 0012 decision 6 amendment): PROVENANCE — set by the + * replay runtime (`invokeResolvedReplayAction`, + * `session-replay-action-runtime.ts`) on every action it dispatches from a + * replay plan, annotated or not. It marks the action as AUTHORED (it came + * from the `.ad` under repair) rather than typed out-of-band by the agent + * mid-repair. + * + * The repair-segment exclusion keys off its ABSENCE: an authored + * `get`/`is`/`find`/`snapshot` step must survive into its own healed script + * (silently dropping it would make the heal quietly stop asserting what the + * original flow asserted), while an interactive diagnostic read used only to + * LOCATE the repair target must not. Command class alone cannot tell those + * apart — they are the same command — so provenance is the discriminator and + * `--record` is only for deliberately inserting an interactive read. + * + * Trustworthy because `internal` is daemon-only: `toDaemonRequest` + * (`server/http-server.ts`) never copies it off the wire, so no client can + * spoof authored provenance. Same channel as `replayTargetGuard` above. + */ + replayPlanStep?: boolean; }; export type DaemonRequest = Omit & { diff --git a/src/mcp/__tests__/command-tools.test.ts b/src/mcp/__tests__/command-tools.test.ts index 68f37bbe5..07811cd87 100644 --- a/src/mcp/__tests__/command-tools.test.ts +++ b/src/mcp/__tests__/command-tools.test.ts @@ -1159,3 +1159,47 @@ test('MCP tool error without a divergence never touches existing pins (merge-onl await executor.execute('press', { target: { kind: 'ref', ref: '@e2' } }); assert.deepEqual(runCalls[2]?.input, { target: { kind: 'ref', ref: '@e2~s7' } }); }); + +// --- #1271 stage 2 (ADR 0012 amendment): `record`/`noRecord` on the MCP +// tool schema for the observation-only commands the repair-segment default +// exclusion targets. --- + +test('MCP get/is/find/snapshot tools expose record and noRecord in their input schema', () => { + for (const name of ['get', 'is', 'find', 'snapshot']) { + const tool = listCommandTools().find((candidate) => candidate.name === name); + assert.ok(tool, `expected an MCP tool named ${name}`); + const properties = tool.inputSchema.properties ?? {}; + assert.equal( + (properties.record as { type?: string } | undefined)?.type, + 'boolean', + `${name} tool is missing a boolean 'record' input`, + ); + assert.equal( + (properties.noRecord as { type?: string } | undefined)?.type, + 'boolean', + `${name} tool is missing a boolean 'noRecord' input`, + ); + } +}); + +test('MCP forwards record/noRecord from a get tool call through to the executed command input', async () => { + const calls: unknown[] = []; + const executor = createCommandToolExecutor({ + createClient: () => ({}) as AgentDeviceClient, + runCommand: async (_client, name, input) => { + calls.push({ name, input }); + return { text: 'hello' }; + }, + }); + await executor.execute('get', { + format: 'text', + target: { kind: 'ref', ref: '@e5' }, + record: true, + }); + assert.deepEqual(calls, [ + { + name: 'get', + input: { format: 'text', target: { kind: 'ref', ref: '@e5' }, record: true }, + }, + ]); +}); diff --git a/src/replay/__tests__/divergence.test.ts b/src/replay/__tests__/divergence.test.ts index c03263673..b8c5f81ec 100644 --- a/src/replay/__tests__/divergence.test.ts +++ b/src/replay/__tests__/divergence.test.ts @@ -680,19 +680,21 @@ test('formatReplayDivergenceReport renders neither caution command when resume i assert.match(report!, /cannot currently be resumed automatically/); }); -// --- #1271 stage 1 (safe interim; default exclusion is stage 2, gated on an -// ADR-0012 amendment): read-only diagnostics an agent runs mid-repair to -// LOCATE the target (snapshot -i, get attrs, find, is) are recorded into the -// healed script by default. The repairHint guidance must teach this — but -// ONLY when `resume.repairSessionHeld === true` (decision 6, R7 C1's signal -// that this divergence came from a repair-armed `--save-script` replay). -// A plain non-repair divergence never carries `repairSessionHeld` and must -// never render the clause — it would be pure noise. --- +// --- #1271 stage 2 (ADR-0012 amendment): read-only diagnostics an agent runs +// mid-repair to LOCATE the target (snapshot -i, get attrs, find, is) are +// EXCLUDED from the healed script by default (stage 1's interim +// "use --no-record" guidance is superseded now the daemon enforces this +// itself). The repairHint guidance must teach the new default AND the +// `--record` escape hatch for a corrective action that is itself a read — +// but ONLY when `resume.repairSessionHeld === true` (decision 6, R7 C1's +// signal that this divergence came from a repair-armed `--save-script` +// replay). A plain non-repair divergence never carries `repairSessionHeld` +// and must never render the clause — it would be pure noise. --- const REPAIR_DIAGNOSTICS_CLAUSE_PATTERN = - /Read-only inspection while armed \(snapshot -i, get attrs, find, is\) is recorded too — use --no-record on those commands or they will land in the healed script\./; + /Read-only inspection while armed \(snapshot -i, get attrs, find, is\) is excluded from the healed script by default — no --no-record needed\. If the step you are repairing is itself a read, add --record to that command so it lands in the heal\./; -test('formatReplayDivergenceReport appends the diagnostics --no-record clause for record-and-heal when repairSessionHeld is true', async () => { +test('formatReplayDivergenceReport appends the diagnostics default-exclusion clause for record-and-heal when repairSessionHeld is true', async () => { const { formatReplayDivergenceReport } = await import('../divergence.ts'); const report = formatReplayDivergenceReport({ divergence: { @@ -735,7 +737,7 @@ test('formatReplayDivergenceReport OMITS the diagnostics clause for record-and-h }); assert.ok(report); assert.doesNotMatch(report!, /Read-only inspection/); - assert.doesNotMatch(report!, /--no-record on those commands/); + assert.doesNotMatch(report!, /excluded from the healed script by default/); }); test('formatReplayDivergenceReport appends the diagnostics clause for state-repair when armed, distinct from the existing app-state --no-record clause', async () => { @@ -838,7 +840,8 @@ test('formatReplayDivergenceReport appends the diagnostics clause for manual whe // `repairSessionHeld` reports the daemon KEPT THE SESSION LIVE, independent // of `resume.allowed` (plan-resumability) — the diagnostics clause must // still render here: the agent may still inspect the (held) session while - // deciding on a manual repair, and any such read is still recorded. + // deciding on a manual repair, and any such read is excluded from the heal + // by default unless recorded with `--record`. const report = formatReplayDivergenceReport({ divergence: { version: 1, diff --git a/src/replay/divergence.ts b/src/replay/divergence.ts index 91557ae15..716410322 100644 --- a/src/replay/divergence.ts +++ b/src/replay/divergence.ts @@ -417,12 +417,14 @@ export function formatReplayDivergenceReport( * `resume.allowed` is false, a resume command is never rendered for any hint, * and the reported `reason` is surfaced instead. * - * #1271 stage 1: whenever `resume.repairSessionHeld` is `true` (this - * divergence is from a repair-armed `--save-script` replay), every hint's - * guidance also appends `REPAIR_DIAGNOSTICS_NO_RECORD_CLAUSE` — read-only - * diagnostics used to locate the repair target are recorded into the healed - * script by default, so the agent must pass `--no-record` on inspection - * commands. See `buildRepairHintGuidance`. + * #1271 stage 2 (ADR 0012 amendment): whenever `resume.repairSessionHeld` is + * `true` (this divergence is from a repair-armed `--save-script` replay), + * every hint's guidance also appends + * `REPAIR_DIAGNOSTICS_DEFAULT_EXCLUSION_CLAUSE` — read-only diagnostics used + * to locate the repair target (`snapshot -i`, `get attrs`, `find`, `is`) are + * excluded from the healed script by default (no `--no-record` needed), and + * an agent whose CORRECTIVE action is itself a read must pass `--record` on + * that one command so it lands in the heal. See `buildRepairHintGuidance`. */ function divergenceRepairHintLine(repairHint: unknown, resume: unknown): string[] { if (typeof repairHint !== 'string') return []; @@ -464,21 +466,23 @@ function formatResumeCommand(from: number, planDigest: string): string { } /** - * #1271 stage 1 (safe interim; the maintainer's default-exclusion stage 2 is - * gated on an ADR-0012 amendment): read-only diagnostics an agent runs to - * LOCATE the repair target (`snapshot -i`, `get attrs`, `find`, `is`) are, by - * default, recorded into the healed script exactly like the corrective press - * — the wave-3 E3 experiment measured 0/4 trials producing a clean healed - * script hands-off, and one recorded `get attrs` caused a second, - * self-inflicted `identity-mismatch` divergence on fresh replay. Distinct from - * the existing `--no-record` mentions above (`state-repair`'s "fix app state - * with --no-record actions", and `buildDualPathRepairHintGuidance`'s - * state-fix clause), which are about correcting APP STATE, not about - * inspection reads — both clauses can legitimately apply to the same - * divergence. + * #1271 stage 2 (ADR 0012 amendment; supersedes stage 1's interim + * "use --no-record" guidance now that the daemon enforces default + * exclusion itself — `isExcludedRepairSegmentObservation`, + * `session-action-recorder.ts`): read-only diagnostics an agent runs to + * LOCATE the repair target (`snapshot -i`, `get attrs`, `find`, `is`) are, + * by default, excluded from the healed script — the wave-3 E3 experiment's + * 0/4 clean-heal rate motivated the exclusion, but a blanket "exclude every + * read" would silently drop a diverged step whose OWN correction is itself + * a read (the E3 case). `--record` is the opt-in that forces exactly that + * one action through. Distinct from the existing `--no-record` mentions + * above (`state-repair`'s "fix app state with --no-record actions", and + * `buildDualPathRepairHintGuidance`'s state-fix clause), which are about + * correcting APP STATE via a MUTATING action, not about inspection reads — + * both clauses can legitimately apply to the same divergence. */ -const REPAIR_DIAGNOSTICS_NO_RECORD_CLAUSE = - 'Read-only inspection while armed (snapshot -i, get attrs, find, is) is recorded too — use --no-record on those commands or they will land in the healed script.'; +const REPAIR_DIAGNOSTICS_DEFAULT_EXCLUSION_CLAUSE = + 'Read-only inspection while armed (snapshot -i, get attrs, find, is) is excluded from the healed script by default — no --no-record needed. If the step you are repairing is itself a read, add --record to that command so it lands in the heal.'; /** * Gated on `resume.repairSessionHeld === true` (decision 6, R7 C1): that is @@ -496,7 +500,9 @@ function buildRepairHintGuidance(repairHint: string, resume: unknown): string | const guidance = readResumeGuidance(resume); const core = buildRepairHintGuidanceCore(repairHint, guidance); if (core === undefined) return undefined; - return isRepairSessionHeld(resume) ? `${core} ${REPAIR_DIAGNOSTICS_NO_RECORD_CLAUSE}` : core; + return isRepairSessionHeld(resume) + ? `${core} ${REPAIR_DIAGNOSTICS_DEFAULT_EXCLUSION_CLAUSE}` + : core; } function buildRepairHintGuidanceCore( diff --git a/src/selectors/find.ts b/src/selectors/find.ts index 2a372b322..378d9f511 100644 --- a/src/selectors/find.ts +++ b/src/selectors/find.ts @@ -22,6 +22,23 @@ export type FindAction = | { kind: 'exists' } | { kind: 'wait'; timeoutMs?: number }; +export type ReadOnlyFindAction = 'exists' | 'wait' | 'get_text' | 'get_attrs'; + +/** + * `find` is the one command whose observation-vs-mutation split is a + * POSITIONAL, not the command name — so it cannot be settled statically by the + * CLI grammar the way `snapshot`/`get`/`is` are. Both the read-only dispatch + * (`dispatchFindReadOnlyViaRuntime`) and the mutating handler + * (`handleFindCommands`) read this one definition, so `--record`'s dynamic + * validation (#1271 stage 2) and the read-only routing can never disagree + * about which sub-actions observe. + */ +export function isReadOnlyFindAction(action: FindAction['kind']): action is ReadOnlyFindAction { + return ( + action === 'exists' || action === 'wait' || action === 'get_text' || action === 'get_attrs' + ); +} + type FindMatchOptions = { requireRect?: boolean; }; From 78126f5748a6f59e783e5ccde27e3adcdc21ae4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 16 Jul 2026 18:57:07 +0200 Subject: [PATCH 2/4] test(integration): cover --record with a provider-backed repair-segment scenario (#1271 stage 2) The progress ratchet (test:integration:progress:check) flagged `record` as an unclassified public CLI flag. Classifying alone would only trade that failure for "missing Provider-backed integration workflow flag coverage" -- and the exclusions bucket is for config/output/transport flags, not behavior flags, so using it would dodge the ratchet rather than satisfy it. Adds a focused provider-backed scenario instead, next to the `--no-record` precedent in android-lifecycle.test.ts. It drives the real request router, session store, replay runtime, and script writer (only the ADB provider is faked), and proves the flag's actual purpose end-to-end: inside a repair-armed `replay --save-script` segment that diverged, the SAME `get text ` runs twice differing only in `--record`; exactly one line lands in the committed healed .ad. Also asserts `--record` + `--no-record` is INVALID_ARGS. Verified the scenario reproduces the bug: with the exclusion neutered it fails on "a diagnostic read inside a repair segment must not be recorded". --- scripts/integration-progress-model.ts | 1 + .../replay-repair-record-exclusion.test.ts | 163 ++++++++++++++++++ 2 files changed, 164 insertions(+) create mode 100644 test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts diff --git a/scripts/integration-progress-model.ts b/scripts/integration-progress-model.ts index 49a63731a..841cdf5e7 100644 --- a/scripts/integration-progress-model.ts +++ b/scripts/integration-progress-model.ts @@ -175,6 +175,7 @@ function summarizeProviderScenarioFlagCoverage(files) { ['restart', 'logs clear --restart workflow'], ['networkInclude', 'network dump include modes', ['include']], ['noRecord', 'action recording suppression'], + ['record', 'repair-segment observation-only recording opt-in (ADR 0012)'], ['replayUpdate', 'retired --update no-op replays without rewriting (ADR 0012)', ['update']], ['replayEnv', 'replay/test variable injection', ['env']], ['replayFrom', 'replay resume skips completed steps (ADR 0012)', ['resumeFrom']], diff --git a/test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts b/test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts new file mode 100644 index 000000000..aa94d2b5c --- /dev/null +++ b/test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts @@ -0,0 +1,163 @@ +/** + * #1271 stage 2 (ADR 0012 decision 6 amendment): the repair-segment default + * exclusion of observation-only commands, and the `--record` opt-in that + * forces one back in. + * + * Provider-backed rather than a handler unit test on purpose: the exclusion is + * a DAEMON-side recording contract every surface inherits, so it has to be + * proven through the real request router, session store, replay runtime, and + * script writer — only the device provider is faked. The counterpart + * `--no-record` coverage lives in `android-lifecycle.test.ts`. + */ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'vitest'; +import { ANDROID_SNAPSHOT_HELPER_FIXTURE_ARTIFACT } from '../../../src/__tests__/test-utils/index.ts'; +import type { AndroidAdbProvider } from '../../../src/platforms/android/adb-executor.ts'; +import { assertRpcError, assertRpcOk } from './assertions.ts'; +import { androidSettingsXml, androidSnapshotHelperOutput } from './android-world.ts'; +import { PROVIDER_SCENARIO_ANDROID } from './fixtures.ts'; +import { createProviderScenarioHarness } from './harness.ts'; + +const SEARCH_SELECTOR = 'id=com.android.settings:id/search'; + +test('Provider-backed integration: a repair-armed segment excludes diagnostic reads by default and --record forces the corrective read into the heal', async () => { + const adbProvider: AndroidAdbProvider = { + snapshotHelperArtifact: ANDROID_SNAPSHOT_HELPER_FIXTURE_ARTIFACT, + exec: async (args) => androidRepairAdbResult(args), + }; + const daemon = await createProviderScenarioHarness({ + androidAdbProvider: () => adbProvider, + deviceInventoryProvider: async () => [PROVIDER_SCENARIO_ANDROID], + }); + const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-repair-record-exclusion-')); + + try { + const client = daemon.client(); + const selection = { platform: 'android' as const, serial: PROVIDER_SCENARIO_ANDROID.id }; + + const open = await client.apps.open({ app: 'settings', ...selection }); + assert.equal(open.device?.id, PROVIDER_SCENARIO_ANDROID.id); + + // Step 2 diverges: a selector that matches nothing in the current tree. + const repairPath = path.join(tempRoot, 'repair.ad'); + fs.writeFileSync( + repairPath, + ['snapshot -i', 'press label="NoSuchControl"', `get text ${SEARCH_SELECTOR}`, ''].join('\n'), + ); + + // --- Arm the repair transaction (R1): `replay --save-script` before step 1. --- + const divergenceError = await client.replay + .run({ path: repairPath, saveScript: true, ...selection }) + .then( + () => null, + (error: unknown) => error as { code?: string; details?: Record }, + ); + assert.ok(divergenceError, 'expected the armed replay to diverge on the missing selector'); + assert.equal(divergenceError.code, 'REPLAY_DIVERGENCE'); + const divergenceReport = divergenceError.details?.divergence as { + resume: { allowed: boolean; from: number; planDigest: string; repairSessionHeld?: boolean }; + }; + assert.equal(divergenceReport.resume.allowed, true); + // R7/C1: the divergence reports the repair transaction as held — the exact + // signal the #1271 stage-1 guidance clause is gated on. + assert.equal(divergenceReport.resume.repairSessionHeld, true); + // The session is repair-armed: `saveScriptBoundary` is the boundary the + // exclusion keys off (an ordinary `open --save-script` never sets it). + const armedBoundary = daemon.session()?.saveScriptBoundary; + assert.equal(typeof armedBoundary, 'number'); + + // --- The exclusion contrast: the SAME observation-only command, against the + // same selector, run twice inside the repair segment — differing only in + // `--record`. --- + const actionsBeforeReads = daemon.session()?.actions.length ?? 0; + + // (a) A diagnostic read used to LOCATE the target: excluded by default, + // with no `--no-record` needed (the #1271 stage-1 foot-gun). + const diagnosticRead = await daemon.callCommand('get', ['text', SEARCH_SELECTOR], { + ...selection, + }); + assertRpcOk(diagnosticRead); + assert.equal( + daemon.session()?.actions.length, + actionsBeforeReads, + 'a diagnostic read inside a repair segment must not be recorded', + ); + + // (b) The corrective read (the wave-3 E3 shape: the diverged step is itself + // a read), forced into the heal with `--record`. + const correctiveRead = await daemon.callCommand('get', ['text', SEARCH_SELECTOR], { + ...selection, + record: true, + }); + assertRpcOk(correctiveRead); + const armedActions = daemon.session()?.actions ?? []; + assert.equal(armedActions.length, actionsBeforeReads + 1); + assert.equal(armedActions.at(-1)?.command, 'get'); + + // (c) `--record` and `--no-record` are opposite intents for one action. + const conflictingFlags = await daemon.callCommand('get', ['text', SEARCH_SELECTOR], { + ...selection, + record: true, + noRecord: true, + }); + assertRpcError(conflictingFlags, 'INVALID_ARGS', /--record and --no-record are mutually/); + + // --- Resume past the diverged step, completing the plan (transaction COMPLETE). --- + const resumed = await client.replay.run({ + path: repairPath, + resumeFrom: divergenceReport.resume.from + 1, + resumePlanDigest: divergenceReport.resume.planDigest, + ...selection, + }); + assert.equal(resumed.replayed, 1); + + // --- Commit the heal and read what actually landed on disk. --- + const close = await daemon.callCommand('close', [], { saveScript: true }); + assertRpcOk(close); + const healedPath = path.join(tempRoot, 'repair.healed.ad'); + assert.equal(fs.existsSync(healedPath), true, 'the completed repair must publish a healed .ad'); + const healedScript = fs.readFileSync(healedPath, 'utf8'); + + // The heal carries EXACTLY ONE `get text` line: the `--record`ed corrective + // read. The identical diagnostic read that ran first is absent — the whole + // point of the amendment, and the reason a blanket read-exclusion would be + // unsafe (it would drop this line too, silently). + const getLines = healedScript + .split('\n') + .filter((line) => line.trim().startsWith('get ') || line.trim().startsWith('get\t')); + assert.equal(getLines.length, 1, `expected exactly one recorded get, got: ${healedScript}`); + assert.match(getLines[0]!, /^get\s+"?text"?\s+/); + assert.match(getLines[0]!, /com\.android\.settings:id\/search/); + } finally { + fs.rmSync(tempRoot, { recursive: true, force: true }); + await daemon.close(); + } +}); + +function androidRepairAdbResult(args: string[]): { + stdout: string; + stderr: string; + exitCode: number; +} { + if (args.join(' ') === 'shell getprop sys.boot_completed') { + return { stdout: '1\n', stderr: '', exitCode: 0 }; + } + if (args.join(' ') === 'shell dumpsys window windows') { + return { + stdout: 'mCurrentFocus=Window{42 u0 com.android.settings/.Settings}\n', + stderr: '', + exitCode: 0, + }; + } + if (args.join(' ').startsWith('shell am instrument ')) { + return { + stdout: androidSnapshotHelperOutput(androidSettingsXml('Display')), + stderr: '', + exitCode: 0, + }; + } + return { stdout: '', stderr: '', exitCode: 0 }; +} From e1f2fed913e8750bcd5838ec5f5ac078e6aaa9fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 16 Jul 2026 19:54:37 +0200 Subject: [PATCH 3/4] fix(replay): key the repair-segment exclusion on provenance, scope --record (#1271 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the maintainer review on #1303. P1 — the exclusion dropped PLANNED reads from the heal. It discriminated by command class, but the real discriminator is provenance. Replayed plan steps dispatch through the ordinary request path, so an authored get/is/find step hit the same recordIfSession -> exclusion path as an interactive read and never reached session.actions -- and the heal IS session.actions.slice(boundary). A repaired flow therefore replayed its authored `is visible` assertion and then silently dropped it from its own healed script: the heal quietly stops checking what it used to check, which for a 10x-QA-replay suite is the worst failure mode. Fix: an explicit provenance marker, not a heuristic. `internal.replayPlanStep` is stamped by invokeResolvedReplayAction -- the single point every plan step is dispatched, so it covers annotated and unannotated steps alike. `internal` is daemon-only (toDaemonRequest never copies it off the wire), so authored provenance cannot be spoofed; same channel as replayTargetGuard. The rule now lives once in isInteractiveObservation and both recording call sites consume it, so the mock fixture uses the production classifier instead of mirroring it. Planned observations survive automatically -- users never annotate their own .ad steps. --record is no longer a common flag: removed from COMMON_COMMAND_SUPPORTED_FLAG_KEYS, statically scoped via allowedFlags to snapshot/get/is, and validated dynamically for find (read-only allows; a mutating find click|fill|focus|type is INVALID_ARGS before any device work, sharing one isReadOnlyFindAction predicate with the read-only routing so the two cannot disagree). --no-record stays shared -- it applies to every recordable command. Removed from `open`, which is never observation-only. Rebased onto #1304 and dropped the four hand-rolled reader blocks. Split its helper rather than broadening it: noRecordInputFromFlags (all 13 readers) + observationRecordInputFromFlags (snapshot/get/is/find only). Two named helpers over one `allowRecord` policy arg -- the capability is then the helper's NAME, so a mutating reader physically cannot forward --record, whereas a policy arg would let a future mutating reader opt in by flipping a literal with no schema change. ADR-0012 decision 6 now states the provenance rule, not a command-class rule. The scenario gates the P1: its authored step is a distinguishable `is visible`, and it fails without the provenance check ("the authored 'is visible' step must survive the heal"). --- docs/adr/0012-interactive-replay.md | 147 ++++++++++++------ src/cli/parser/cli-help.ts | 2 +- src/commands/capture/snapshot.ts | 8 + src/commands/interaction/metadata.ts | 13 +- .../__tests__/selector-recording.test.ts | 22 +++ .../handlers/__tests__/find-args.test.ts | 18 ++- src/daemon/handlers/__tests__/find.test.ts | 19 +++ src/daemon/selector-runtime.ts | 1 - src/daemon/session-action-recorder.ts | 7 +- .../replay-repair-record-exclusion.test.ts | 42 ++++- 10 files changed, 208 insertions(+), 71 deletions(-) diff --git a/docs/adr/0012-interactive-replay.md b/docs/adr/0012-interactive-replay.md index 4addebb5d..e3a048d1d 100644 --- a/docs/adr/0012-interactive-replay.md +++ b/docs/adr/0012-interactive-replay.md @@ -993,17 +993,38 @@ The tombstone itself expires after its bounded window, after which the key is fu > behavior the default, superseding stage 1's "opt out by hand" guidance with "opt in by hand" for the one > case that needs it. > -> **1. Which read-only commands are excluded.** Exactly the enumerated set stage 1 already named: -> `snapshot`, `get`, `is`, and a **read-only** `find` sub-action (`exists`, `wait`, `get_text`, `get_attrs`). -> A **mutating** `find … click|fill|focus|type` is not in this set — it is a corrective action like `press`/ -> `fill` and always records. The top-level `wait` command is deliberately **not** in this set either: it is -> flow timing/synchronisation (pacing the script), not observation, so it must keep recording unconditionally -> even inside a repair segment — a healed script that silently dropped its own timing steps would replay -> differently from what actually happened. +> **1. Which observations are excluded — a PROVENANCE rule, not a command-class rule.** The exclusion +> applies to an observation that is BOTH: > -> **2. The corrective-read trap, and why blanket exclusion by command class is unsafe.** "Exclude every -> read from a repair segment" is not a safe rule on its own: a divergence's **correction** can itself be a -> read. In the wave-3 E3 drift, the diverged step **was** a `get`, so its corrective replacement is a +> - **observation-only by command**: `snapshot`, `get`, `is`, or a **read-only** `find` sub-action +> (`exists`, `wait`, `get_text`, `get_attrs`). A **mutating** `find … click|fill|focus|type` is not in +> this set — it is a corrective action like `press`/`fill` and always records. The top-level `wait` +> command is deliberately **not** in this set either: it is flow timing/synchronisation (pacing the +> script), not observation, so it must keep recording unconditionally — a healed script that silently +> dropped its own timing steps would replay differently from what actually happened; **AND** +> - **dispatched OUT OF BAND**: typed by the agent mid-repair, NOT replayed from the `.ad` plan under +> repair. +> +> The second clause is load-bearing and is the rule's real discriminator. Replayed plan steps dispatch +> through the ordinary request path and land in `session.actions` like any other action, and the healed +> script IS that slice (`buildOptimizedActions` over `session.actions.slice(saveScriptBoundary)`). So a +> command-class-only exclusion would replay an authored `is visible` assertion and then **silently drop it +> from its own healed script** — the repaired flow would quietly stop checking what the original checked, +> which for a deterministic QA suite is the worst possible failure mode. **Planned prefix/suffix +> observations must survive automatically; a user must never have to annotate authored `.ad` steps with +> `--record` to keep them.** An authored observation and an interactive diagnostic read are the same +> command, so only provenance separates them. +> +> Provenance travels as `internal.replayPlanStep`, stamped by `invokeResolvedReplayAction` +> (`src/daemon/handlers/session-replay-action-runtime.ts`) — the single point every plan step is +> dispatched, so the marker covers annotated and unannotated steps alike. `internal` is a daemon-only +> channel (`toDaemonRequest`, `src/daemon/server/http-server.ts`, never copies it off the wire), so +> authored provenance cannot be spoofed by a client; this is the same channel `replayTargetGuard` and +> `findResolvedTarget` already use. The rule is written once, in `isInteractiveObservation` +> (`src/daemon/session-action-recorder.ts`), and both recording call sites consume it. +> +> **2. The corrective-read trap: why even out-of-band reads need an opt-in.** Excluding every out-of-band +> read is still not safe on its own: a divergence's **correction** can itself be a read. In the wave-3 E3 drift, the diverged step **was** a `get`, so its corrective replacement is a > recorded `get` that **must** land in the heal — a blanket read-exclusion would silently drop the very > step being repaired, producing an incomplete heal with no signal anything was missing (the same class of > silent-failure mode decision 3 exists to close for target-binding, and decision 6 R4 exists to close for @@ -1015,36 +1036,57 @@ The tombstone itself expires after its bounded window, after which the key is fu > opt-in per action**: `--record` (below) marks the ONE read that must count, independent of its position in > the repair segment. > -> **3. Explicit recording remains possible.** `--record` is a new, standalone flag — distinct from -> `saveScript`, and the mirror image of `--no-record` — that forces the action it is passed on into the heal -> even though its command is observation-only. It is a no-op outside a repair-armed session (there is no -> default exclusion to override) and a no-op on any non-observation-only command (those already always -> record). `--record` and `--no-record` express opposite intents for the same action and are **mutually -> exclusive**: passing both is rejected as `INVALID_ARGS` before the command runs, uniformly for every -> surface, rather than letting one silently win. +> **3. Explicit recording remains possible.** `--record` forces an out-of-band observation into the heal +> even though its command is observation-only. It exists for exactly one job: an interactive corrective +> read the agent is deliberately inserting into the healed script. It is a no-op outside a repair-armed +> session (there is no default exclusion to override), and it is **not** needed for authored plan steps — +> those survive on provenance alone (rule 1). +> +> `--record` is **not a common flag**, unlike `--no-record`. `--no-record` applies to every recordable +> command, mutations included, so it stays universal +> (`COMMON_COMMAND_SUPPORTED_FLAG_KEYS`). `--record` only means something for a command the exclusion can +> drop, so it is scoped **statically** to `snapshot`, `get`, and `is` through each command schema's +> `allowedFlags` — accepting it on `press`/`fill`/`click` would be misleading, and the grammar rejects +> unsupported uses before projection. `find` is the one command whose observe-vs-mutate split is a +> POSITIONAL rather than the command name, so it cannot be settled statically: it allows `--record` +> in the grammar and validates **dynamically** in the daemon (`handleFindCommands`), rejecting +> `--record` on a mutating `find … click|fill|focus|type` with `INVALID_ARGS` before any device work. Both +> halves read one shared predicate (`isReadOnlyFindAction`, `src/selectors/find.ts`), so the routing and +> the validation can never disagree about which sub-actions observe. +> +> `--record` and `--no-record` express opposite intents for the same action and are **mutually exclusive**: +> passing both is rejected as `INVALID_ARGS` before the command runs, uniformly for every surface, rather +> than letting one silently win. > > **4. How the rule projects across CLI/Node/MCP surfaces.** The exclusion is enforced at a single > daemon-side choke point — `isExcludedRepairSegmentObservation` in `recordActionEntry` > (`src/daemon/session-action-recorder.ts`), gated on `session.saveScriptBoundary !== undefined` (set ONLY > by a repair-armed `replay --save-script`; an ordinary, non-repair `open --save-script`/`close --save-script` > authoring recording never sets it — see decision 6's "Scope" note above) and on the entry's -> `observationOnly` marker (set by the caller — `get`/`is`/a read-only `find` in -> `src/daemon/selector-recording.ts`, `snapshot` in `src/daemon/snapshot-runtime.ts` — never inferred from -> `command`/`positionals`, since a mutating `find … click` shares the literal string `command: 'find'` with -> its read-only sibling). Every surface reaches this same function because every surface's request reaches -> the same daemon: the CLI parses `--record`/`--no-record` into `CommandFlags` (`src/contracts/cli-flags.ts`, -> `src/commands/cli-grammar/flag-definitions-action.ts`, `src/commands/cli-grammar/flag-groups.ts`'s -> `COMMON_COMMAND_SUPPORTED_FLAG_KEYS` — so both flags are syntactically valid on every command, exactly like -> `--no-record` already was); the Node client threads the same-named option through -> `buildRequestFlags`/`buildFlags` (`src/commands/command-flags.ts`) from `InternalRequestOptions` and -> `AppOpenOptions` (`src/client/client-types.ts`) and from each typed per-command Options type -> (`GetOptions`/`IsOptions`/`FindOptions`/`CaptureSnapshotOptions`); and the MCP tool schema exposes `record`/ -> `noRecord` fields on the `get`/`is`/`find`/`snapshot` tools (`src/commands/interaction/metadata.ts`, -> `src/commands/capture/snapshot.ts`) that flow through the same `CommandMetadata.inputSchema` + -> `readMetadataCommandFlags` path every other typed field already uses. `management/app.ts` mirrors both -> flags onto `open` for symmetry with `--no-record`'s existing presence there, though neither has an effect -> on a command that is never observation-only. No surface has a separate recording contract; all of them -> hit the identical daemon-side default. +> `interactiveObservation` marker, which both recording call sites +> (`src/daemon/selector-recording.ts` for `get`/`is`/read-only `find`, `src/daemon/snapshot-runtime.ts` for +> `snapshot`) compute from the shared `isInteractiveObservation` predicate rather than each re-deriving it. +> Every surface reaches this same function because every surface's request reaches the same daemon. +> +> The flags project asymmetrically, matching their different scopes: +> +> - `--no-record` stays a common flag: its key is in `COMMON_COMMAND_SUPPORTED_FLAG_KEYS`, and every +> recordable reader forwards it through the shared `noRecordInputFromFlags` helper +> (`src/commands/cli-grammar/common.ts`, per #1304). +> - `--record` is scoped: its key is NOT common, it appears in `allowedFlags` only for `snapshot`/`get`/`is` +> (plus `find`, which validates dynamically per rule 3), and only those readers spread +> `observationRecordInputFromFlags`. Two named helpers rather than one helper with an `allowRecord` +> policy argument: the capability is then the helper's NAME, so a mutating reader physically cannot +> forward `--record`, whereas a policy argument would let a future mutating reader opt in by flipping a +> literal with no schema change — the fail-open the split exists to prevent. +> +> From there both flags ride the ordinary typed path: the Node client threads them through +> `buildRequestFlags`/`buildFlags` (`src/commands/command-flags.ts`) from `InternalRequestOptions` and each +> typed per-command Options type (`GetOptions`/`IsOptions`/`FindOptions`/`CaptureSnapshotOptions`), and the +> MCP tool schema exposes `record`/`noRecord` on the `get`/`is`/`find`/`snapshot` tools +> (`src/commands/interaction/metadata.ts`, `src/commands/capture/snapshot.ts`) through the same +> `CommandMetadata.inputSchema` + `readMetadataCommandFlags` path every other typed field already uses. No +> surface has a separate recording contract; all of them hit the identical daemon-side default. > > **5. The fail-loud guard is the EXISTING resume watermark, not new state.** Because the exclusion happens > at the same point `--no-record` is enforced — before an action ever reaches `session.actions` — an excluded @@ -1059,13 +1101,16 @@ The tombstone itself expires after its bounded window, after which the key is fu > action was recorded in this repair segment; press the correct control via a blessed `@ref` … — or, if your > corrective action was a read, re-run it with `--record` … — before resuming." > -> **Regression coverage** proves: a diagnostic read inside an armed repair segment is omitted from the -> healed script by default; a corrective action remains for both a mutating press/fill (unaffected — never -> observation-only) and a `--record`ed read (the diverged-step-was-a-`get` case); the resulting healed script -> replays cleanly in a fresh session; the empty-segment guard refuses a blind resume with the actionable -> `--record` hint; and ordinary, non-repair `open --save-script` authoring recording is completely unchanged -> — a read in a fresh authoring session still records with no flag needed, because `session.saveScriptBoundary` -> is never set there. +> **Regression coverage** proves: an out-of-band diagnostic read inside an armed repair segment is omitted +> from the healed script by default; an **authored** `is visible` plan step in the same repair, carrying no +> `--record`, **survives** into the heal (the provenance rule — this fails if the exclusion is +> command-class-only); a corrective action remains for both a mutating press/fill (unaffected — never +> observation-only) and a `--record`ed read (the diverged-step-was-a-`get` case); the resulting healed +> script replays cleanly in a fresh session; `--record` on a mutating `find … click` is refused with +> `INVALID_ARGS`; the empty-segment guard refuses a blind resume with the actionable `--record` hint; and +> ordinary, non-repair `open --save-script` authoring recording is completely unchanged — a read in a fresh +> authoring session still records with no flag needed, because `session.saveScriptBoundary` is never set +> there. ## Consequences @@ -1178,15 +1223,19 @@ each states its dependencies explicitly. pre-existing target — complete or partial, default or explicit path alike, and uniformly for an ordinary non-repair recording's target too, since the publish primitive is shared — never overwriting one; see "Scope" under Decision 6 above). -10. **Repair-segment default exclusion of observation-only commands, and the `--record` opt-in** +10. **Repair-segment default exclusion of out-of-band observations, and the `--record` opt-in** (decision 6 amendment) — **SHIPPED (#1271 stage 2)**; depends on 9 (`session.saveScriptBoundary`/ - `repairSessionHeld` are the signals the exclusion and its guidance key off). Adds: default exclusion of - `snapshot`/`get`/`is`/a read-only `find` from `session.actions` while repair-armed - (`isExcludedRepairSegmentObservation`, `session-action-recorder.ts`); the `--record` opt-in, plumbed - across CLI/Node client/MCP identically to `--no-record`, and rejected together with it as - `INVALID_ARGS`; and an updated `describeUnperformedRecordAndHeal` message naming `--record` for the - corrective-read case. Stage 1 (#1287) shipped interim `--no-record` guidance only, gated on this same - `repairSessionHeld` signal. + `repairSessionHeld` are the signals the exclusion and its guidance key off) and on #1304 (the shared + reader helper `--record`'s scoped sibling is split from). Adds: the `internal.replayPlanStep` + provenance marker stamped by `invokeResolvedReplayAction` (`session-replay-action-runtime.ts`); + default exclusion of OUT-OF-BAND `snapshot`/`get`/`is`/read-only `find` from `session.actions` while + repair-armed, keyed off that provenance via `isInteractiveObservation`/ + `isExcludedRepairSegmentObservation` (`session-action-recorder.ts`), so authored plan steps survive + their own heal automatically; the `--record` opt-in, statically scoped to `snapshot`/`get`/`is` and + dynamically validated for `find`, rejected together with `--no-record` as `INVALID_ARGS`; and an + updated `describeUnperformedRecordAndHeal` message naming `--record` for the corrective-read case. + Stage 1 (#1287) shipped interim `--no-record` guidance only, gated on this same `repairSessionHeld` + signal. ## Migration progress diff --git a/src/cli/parser/cli-help.ts b/src/cli/parser/cli-help.ts index d5a1f2532..95d09f913 100644 --- a/src/cli/parser/cli-help.ts +++ b/src/cli/parser/cli-help.ts @@ -325,7 +325,7 @@ Validation and evidence: agent-device screenshot agent-device press 124 817 agent-device snapshot -i - Startup/CPU/memory/frame first pass: perf metrics --json (bare perf and metrics are aliases). Focused frame/jank health: perf frames --json. Memory-only sample: perf memory sample --json returns compact JSON with bounded top offenders. Heap/memgraph artifact escalation: perf memory snapshot --out heap.artifact; use --kind android-hprof on Android or --kind memgraph on supported Apple simulator/macOS app sessions. Android native profiling: perf cpu profile start|stop|report --kind simpleperf --out ; Android native traces: perf trace start|stop --kind perfetto --out . Artifact collectors return compact state/path/size metadata only; raw heap/profile/trace files stay on disk. Treat native perf output as the agent evidence: for example, a Perfetto stop can return state=stopped, outPath=/tmp/app.perfetto-trace, sizeBytes=5392410, and method=adb-shell-perfetto while the 5.3 MB raw trace stays in the artifact. This is better than raw dumps for agents because it is stable, bounded, and keeps large artifacts out of context. heapprofd is deferred until Perfetto plumbing is available. Replay divergence and resume: a failing replay/test step returns REPLAY_DIVERGENCE with a bounded report (screen digest, ranked selector suggestions, resume). Repair app state, then resume with replay --from --plan-digest (both from the report's resume field) to continue from the failed step without re-running earlier ones; resume never re-executes skipped steps, so app state is the caller's responsibility, and it is rejected with INVALID_ARGS when the plan digest is stale, --from is out of range, or the skipped range/target touches runtime control flow. The plan digest binds the script, its includes, the effective --platform/--target, and per-action runtime/identity. Native .ad interpolation is late-bound after planning, so changing only its values keeps the digest; Maestro environment substitution occurs during compatibility parsing and can change action inputs, includes, or control expansion, so it can change the digest. --from is replay-only; test rejects it. --update/-u no longer rewrites the script (ADR 0012) — it is a no-op kept for compatibility; every divergence already carries the same ranked suggestions. Agent-supervised repair (heal-by-doing): arm replay .ad --save-script[=] before step 1 (armed once; --from continuations do not need it again). Every divergence carries a repairHint: record-and-heal means press the correct control via a blessed @ref from the divergence's screen.refs, recorded (no --no-record), then continue with replay --from --plan-digest ; state-repair means the script is correct but app state is not, so fix state with --no-record actions, then replay --from --plan-digest to re-run the unchanged step; caution means something already matches the recorded selector, so a blind re-press may repeat the mistake; manual means no safe automated repair could be proven. While armed, read-only inspection used to locate the repair target (snapshot -i, get attrs, find, is) is excluded from the healed script by default — no --no-record needed; if the step you are repairing is itself a read, pass --record on that one command so it lands in the heal (--record and --no-record are mutually exclusive). Ending the repair with close --save-script[=] writes only the steps recorded since the arming replay as , defaulting to the sibling .healed.ad — review its diff before promoting it over the original. + Startup/CPU/memory/frame first pass: perf metrics --json (bare perf and metrics are aliases). Focused frame/jank health: perf frames --json. Memory-only sample: perf memory sample --json returns compact JSON with bounded top offenders. Heap/memgraph artifact escalation: perf memory snapshot --out heap.artifact; use --kind android-hprof on Android or --kind memgraph on supported Apple simulator/macOS app sessions. Android native profiling: perf cpu profile start|stop|report --kind simpleperf --out ; Android native traces: perf trace start|stop --kind perfetto --out . Artifact collectors return compact state/path/size metadata only; raw heap/profile/trace files stay on disk. Treat native perf output as the agent evidence: for example, a Perfetto stop can return state=stopped, outPath=/tmp/app.perfetto-trace, sizeBytes=5392410, and method=adb-shell-perfetto while the 5.3 MB raw trace stays in the artifact. This is better than raw dumps for agents because it is stable, bounded, and keeps large artifacts out of context. heapprofd is deferred until Perfetto plumbing is available. Replay divergence and resume: a failing replay/test step returns REPLAY_DIVERGENCE with a bounded report (screen digest, ranked selector suggestions, resume). Repair app state, then resume with replay --from --plan-digest (both from the report's resume field) to continue from the failed step without re-running earlier ones; resume never re-executes skipped steps, so app state is the caller's responsibility, and it is rejected with INVALID_ARGS when the plan digest is stale, --from is out of range, or the skipped range/target touches runtime control flow. The plan digest binds the script, its includes, the effective --platform/--target, and per-action runtime/identity. Native .ad interpolation is late-bound after planning, so changing only its values keeps the digest; Maestro environment substitution occurs during compatibility parsing and can change action inputs, includes, or control expansion, so it can change the digest. --from is replay-only; test rejects it. --update/-u no longer rewrites the script (ADR 0012) — it is a no-op kept for compatibility; every divergence already carries the same ranked suggestions. Agent-supervised repair (heal-by-doing): arm replay .ad --save-script[=] before step 1 (armed once; --from continuations do not need it again). Every divergence carries a repairHint: record-and-heal means press the correct control via a blessed @ref from the divergence's screen.refs, recorded (no --no-record), then continue with replay --from --plan-digest ; state-repair means the script is correct but app state is not, so fix state with --no-record actions, then replay --from --plan-digest to re-run the unchanged step; caution means something already matches the recorded selector, so a blind re-press may repeat the mistake; manual means no safe automated repair could be proven. While armed, read-only inspection YOU run to locate the repair target (snapshot -i, get attrs, find, is) is excluded from the healed script by default — no --no-record needed; the script's own authored get/is/find steps are unaffected and stay in the heal. If the step you are repairing is itself a read, pass --record on that one command so it lands in the heal (--record is accepted only on snapshot/get/is and a read-only find; it is mutually exclusive with --no-record). Ending the repair with close --save-script[=] writes only the steps recorded since the arming replay as , defaulting to the sibling .healed.ad — review its diff before promoting it over the original. Recording: record start/stop. The default scope is app and expects an active session created by open ; this keeps app proof videos tied to the intended app session. Use record start --scope device/system to explicitly request whole-screen capture where the selected backend supports it, such as recordings that intentionally span multiple apps, home screen, settings, or app transitions. Use --max-size to cap the longest edge and --quality medium|high to choose output quality across Android and Apple targets. By default, stop burns touch overlays into the video; use record start --hide-touches for the fastest raw recording. Android record start publishes a durable device manifest. Android adb screenrecord has a 180s platform limit, so longer Android recordings are returned as multiple MP4 chunks while the daemon stays alive; after daemon restart, record stop recovers only manifest-owned chunks and warns when gesture overlays are unavailable. For gesture-heavy iOS simulator proof videos, prefer --hide-touches because overlay timing depends on a stable runner session while gestures are executing. Tracing: trace start ./trace.log, trace stop ./trace.log. Paths are positional. Stable known flow: batch ./steps.json, not workflow batch. Inline batch JSON example: diff --git a/src/commands/capture/snapshot.ts b/src/commands/capture/snapshot.ts index a7b21ff1a..d41b2b42b 100644 --- a/src/commands/capture/snapshot.ts +++ b/src/commands/capture/snapshot.ts @@ -27,6 +27,14 @@ const snapshotCommandMetadata = defineFieldCommandMetadata( raw: booleanField(), forceFull: booleanField(), timeoutMs: integerField('Maximum wall-clock time for the snapshot command.'), + // #1271 stage 2: `snapshot` is observation-only, so a repair-armed heal + // excludes an out-of-band one by default (ADR 0012 amendment). Exposed + // here so the Node SDK's typed options and the MCP tool schema can set + // both flags, mirroring `--no-record`/`--record` on the CLI. + noRecord: booleanField('Do not record this action.'), + record: booleanField( + 'Force-record this out-of-band observation into a repair-armed heal (mutually exclusive with noRecord). Authored replay steps are recorded automatically and never need this.', + ), }, ); diff --git a/src/commands/interaction/metadata.ts b/src/commands/interaction/metadata.ts index 9949ccd71..64420e195 100644 --- a/src/commands/interaction/metadata.ts +++ b/src/commands/interaction/metadata.ts @@ -168,14 +168,17 @@ const scrollFields = { }), }; -// #1271 stage 2: `get`/`is`/`find` are observation-only and excluded from a -// repair-armed heal by default (ADR 0012 amendment). Both flags are exposed -// here so the Node SDK's typed options and the MCP tool schema can actually -// set them, mirroring `--no-record`/`--record` on the CLI. +// #1271 stage 2 (ADR 0012 amendment): `get`/`is`/`find` are observation-only, +// so a repair-armed heal excludes an OUT-OF-BAND one by default. Both flags +// are exposed here so the Node SDK's typed options and the MCP tool schema can +// set them, mirroring `--no-record`/`--record` on the CLI. On `find`, `record` +// is valid only for a read-only action — a mutating `find … click|fill|focus| +// type` is refused as INVALID_ARGS by the daemon (`handleFindCommands`), since +// the observe-vs-mutate split is a positional the schema cannot see. const recordControlFields = () => ({ noRecord: booleanField('Do not record this action.'), record: booleanField( - 'Force-record this action even though its command is observation-only and would otherwise be excluded from a repair-armed heal by default (mutually exclusive with noRecord).', + 'Force-record this out-of-band observation into a repair-armed heal (mutually exclusive with noRecord). Authored replay steps are recorded automatically and never need this. On find, valid only for a read-only action.', ), }); diff --git a/src/daemon/__tests__/selector-recording.test.ts b/src/daemon/__tests__/selector-recording.test.ts index a3d88c738..6d5cd9950 100644 --- a/src/daemon/__tests__/selector-recording.test.ts +++ b/src/daemon/__tests__/selector-recording.test.ts @@ -25,6 +25,11 @@ function req(command: string, flags: DaemonRequest['flags'] = {}): DaemonRequest return { token: 't', session: 'default', command, positionals: [], flags }; } +/** A request as the replay runtime dispatches it: authored provenance stamped on `internal`. */ +function planStepReq(command: string, flags: DaemonRequest['flags'] = {}): DaemonRequest { + return { ...req(command, flags), internal: { replayPlanStep: true } }; +} + test('a repair-armed session excludes get/is/find by default but keeps recording wait', () => { const store = makeStore(); const session = makeIosSession('default', { saveScriptBoundary: 0 }); @@ -38,6 +43,23 @@ test('a repair-armed session excludes get/is/find by default but keeps recording expect(store.get('default')!.actions.map((a) => a.command)).toEqual(['wait']); }); +// The P1 the command-class rule got wrong: an AUTHORED get/is/find plan step is +// the same command as an interactive diagnostic read, but it must survive into +// its own healed script — otherwise a repaired flow silently stops asserting +// what it used to assert, and users would have to annotate their own .ad steps +// with --record to keep them. +test('a repair-armed session still records get/is/find dispatched as replay plan steps (authored provenance)', () => { + const store = makeStore(); + const session = makeIosSession('default', { saveScriptBoundary: 0 }); + store.set('default', session); + + recordIfSession(store, 'default', planStepReq('get'), {}); + recordIfSession(store, 'default', planStepReq('is'), {}); + recordIfSession(store, 'default', planStepReq('find'), {}); + + expect(store.get('default')!.actions.map((a) => a.command)).toEqual(['get', 'is', 'find']); +}); + test('--record forces get/is/find through even while repair-armed', () => { const store = makeStore(); const session = makeIosSession('default', { saveScriptBoundary: 0 }); diff --git a/src/daemon/handlers/__tests__/find-args.test.ts b/src/daemon/handlers/__tests__/find-args.test.ts index ef5ad407a..bbe56c003 100644 --- a/src/daemon/handlers/__tests__/find-args.test.ts +++ b/src/daemon/handlers/__tests__/find-args.test.ts @@ -1,5 +1,9 @@ import { expect, test } from 'vitest'; -import { parseFindArgs, parseFindSelectorExpression } from '../../../selectors/find.ts'; +import { + isReadOnlyFindAction, + parseFindArgs, + parseFindSelectorExpression, +} from '../../../selectors/find.ts'; test('parseFindArgs defaults to click with any locator', () => { const parsed = parseFindArgs(['Login']); @@ -52,3 +56,15 @@ test('parseFindSelectorExpression only treats bare selector-shaped queries as se expect(parseFindSelectorExpression('text', 'label="Continue"')).toBeNull(); expect(parseFindSelectorExpression('any', 'a=b')).toBeNull(); }); + +// #1271 stage 2: `--record` is statically scoped to snapshot/get/is via each +// command schema's `allowedFlags`, but `find`'s observe-vs-mutate split is a +// POSITIONAL, so it is validated dynamically against this shared predicate. +test('isReadOnlyFindAction separates the find sub-actions --record may accompany', () => { + for (const action of ['exists', 'wait', 'get_text', 'get_attrs'] as const) { + expect(isReadOnlyFindAction(action)).toBe(true); + } + for (const action of ['click', 'fill', 'focus', 'type'] as const) { + expect(isReadOnlyFindAction(action)).toBe(false); + } +}); diff --git a/src/daemon/handlers/__tests__/find.test.ts b/src/daemon/handlers/__tests__/find.test.ts index 0251b538b..2159bf054 100644 --- a/src/daemon/handlers/__tests__/find.test.ts +++ b/src/daemon/handlers/__tests__/find.test.ts @@ -782,3 +782,22 @@ test('handleFindCommands click omits refsGeneration — a mutating find never is expect((response.data as Record).refsGeneration).toBeUndefined(); } }); + +// #1271 stage 2: `find`'s observe-vs-mutate split is a positional, so unlike +// snapshot/get/is it cannot be settled by the CLI grammar's per-command +// `allowedFlags`. A mutating find always records, so `--record` on one is +// meaningless — refuse it loudly rather than accept and ignore it. Enforced +// daemon-side so every surface (CLI/Node/MCP) inherits the same refusal. +test('find rejects --record on a mutating action before any device work', async () => { + const { response, invokeCalls } = await runFindClickScenario({ + positionals: ['label', 'Apps', 'click'], + flags: { record: true }, + }); + + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('INVALID_ARGS'); + expect(response.error.message).toMatch(/--record only applies to a read-only find/); + // Refused before the action dispatched. + expect(invokeCalls).toHaveLength(0); +}); diff --git a/src/daemon/selector-runtime.ts b/src/daemon/selector-runtime.ts index 11c7d1291..e48297395 100644 --- a/src/daemon/selector-runtime.ts +++ b/src/daemon/selector-runtime.ts @@ -609,4 +609,3 @@ async function maybeAndroidForegroundBlockerResponse( }, ); } - diff --git a/src/daemon/session-action-recorder.ts b/src/daemon/session-action-recorder.ts index 128f9a499..45cf7f8c6 100644 --- a/src/daemon/session-action-recorder.ts +++ b/src/daemon/session-action-recorder.ts @@ -115,12 +115,7 @@ export function recordActionEntry( * sub-action; `diff` is likewise absent because only `snapshot` is classified * at the snapshot-runtime call site. */ -const OBSERVATION_ONLY_COMMANDS: ReadonlySet = new Set([ - 'snapshot', - 'get', - 'is', - 'find', -]); +const OBSERVATION_ONLY_COMMANDS: ReadonlySet = new Set(['snapshot', 'get', 'is', 'find']); /** * #1271 stage 2 (ADR 0012 amendment): is this request an out-of-band diff --git a/test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts b/test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts index aa94d2b5c..80a55c1e3 100644 --- a/test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts +++ b/test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts @@ -42,10 +42,15 @@ test('Provider-backed integration: a repair-armed segment excludes diagnostic re assert.equal(open.device?.id, PROVIDER_SCENARIO_ANDROID.id); // Step 2 diverges: a selector that matches nothing in the current tree. + // Step 3 is an AUTHORED observation, deliberately `is visible` — a + // different command from the interactive `get` reads below, so the healed + // script is unambiguous about which line came from where. const repairPath = path.join(tempRoot, 'repair.ad'); fs.writeFileSync( repairPath, - ['snapshot -i', 'press label="NoSuchControl"', `get text ${SEARCH_SELECTOR}`, ''].join('\n'), + ['snapshot -i', 'press label="NoSuchControl"', `is visible ${SEARCH_SELECTOR}`, ''].join( + '\n', + ), ); // --- Arm the repair transaction (R1): `replay --save-script` before step 1. --- @@ -121,21 +126,42 @@ test('Provider-backed integration: a repair-armed segment excludes diagnostic re assert.equal(fs.existsSync(healedPath), true, 'the completed repair must publish a healed .ad'); const healedScript = fs.readFileSync(healedPath, 'utf8'); - // The heal carries EXACTLY ONE `get text` line: the `--record`ed corrective + // The heal carries EXACTLY ONE `get` line: the `--record`ed corrective // read. The identical diagnostic read that ran first is absent — the whole // point of the amendment, and the reason a blanket read-exclusion would be // unsafe (it would drop this line too, silently). - const getLines = healedScript - .split('\n') - .filter((line) => line.trim().startsWith('get ') || line.trim().startsWith('get\t')); - assert.equal(getLines.length, 1, `expected exactly one recorded get, got: ${healedScript}`); - assert.match(getLines[0]!, /^get\s+"?text"?\s+/); + const getLines = healedLines(healedScript, 'get'); + assert.equal(getLines.length, 1, `expected exactly one recorded get, got:\n${healedScript}`); assert.match(getLines[0]!, /com\.android\.settings:id\/search/); + + // PROVENANCE (the rule the exclusion actually keys off): the AUTHORED + // `is visible` plan step must survive into its own healed script. It is + // the same command class as the excluded diagnostic read above and carries + // no `--record`, so a command-class exclusion would silently drop it — + // leaving a healed flow that quietly stopped asserting what the original + // asserted. Users must never have to annotate their own `.ad` steps. + const isLines = healedLines(healedScript, 'is'); + assert.equal( + isLines.length, + 1, + `the authored 'is visible' step must survive the heal, got:\n${healedScript}`, + ); + assert.match(isLines[0]!, /com\.android\.settings:id\/search/); } finally { fs.rmSync(tempRoot, { recursive: true, force: true }); await daemon.close(); } -}); + // Matches the provider-scenario convention (android-lifecycle uses the same + // budget): this drives a full arm -> diverge -> resume -> commit chain. +}, 15_000); + +/** Recorded action lines for one command, ignoring the context header, `target-v1` annotations, and the sentinel. */ +function healedLines(script: string, command: string): string[] { + return script + .split('\n') + .map((line) => line.trim()) + .filter((line) => line === command || line.startsWith(`${command} `)); +} function androidRepairAdbResult(args: string[]): { stdout: string; From 89301e87911b3fa01853bb686cf1921dac732912 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 16 Jul 2026 20:15:44 +0200 Subject: [PATCH 4/4] test(daemon): pin that wire-supplied `internal` never reaches a daemon request #1271 stage 2 made `DaemonRequest.internal` semantics-affecting: `internal.replayPlanStep` decides whether an observation-only command is an authored plan step (kept in a repair heal) or an out-of-band diagnostic (excluded). That makes "internal means internally-stamped" worth pinning rather than leaving to convention. The invariant already holds, structurally and twice over: the boundary's `commandRpcParamsSchema` is an allowlist projection emitting only its eight named fields, and `toDaemonRequest` then builds the request field by field. Neither can carry `internal` off the wire. This posts a real JSON-RPC request carrying `internal: { replayPlanStep: true }` through a loopback server and asserts the dispatched request has no `internal`. Verified it fails ("a wire-supplied `internal` must never reach the daemon request") when both allowlists are regressed, so it guards the composite contract instead of restating one layer. --- .../http-server-rpc-validation.test.ts | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/daemon/__tests__/http-server-rpc-validation.test.ts b/src/daemon/__tests__/http-server-rpc-validation.test.ts index ea063365c..73c1b39a7 100644 --- a/src/daemon/__tests__/http-server-rpc-validation.test.ts +++ b/src/daemon/__tests__/http-server-rpc-validation.test.ts @@ -76,3 +76,50 @@ test('malformed command params (command as number) yield 400 / -32602', async (t assert.equal(body.error?.data?.code, 'INVALID_ARGS'); }, t); }); + +// `DaemonRequest.internal` carries semantics-affecting bits stamped inside the +// daemon — `replayPlanStep` (#1271 stage 2) decides whether a read is an +// authored plan step or an out-of-band diagnostic, and so whether it lands in a +// repair heal. Two independent allowlists keep it unreachable from the wire: +// `commandRpcParamsSchema` projects only its eight named fields, and +// `toDaemonRequest` then builds the request field by field. Both would have to +// regress for a caller to stamp its own provenance; this pins the resulting +// boundary contract so neither drifts silently. +test('the rpc boundary never accepts internal request fields from the wire', async (t) => { + if (await skipWhenLoopbackUnavailable(t)) return; + + const received: DaemonRequest[] = []; + const handleRequest = async (req: DaemonRequest): Promise => { + received.push(req); + return { ok: true, data: { ok: true } }; + }; + const server = await createDaemonHttpServer({ handleRequest }); + + try { + const port = await listenOnLoopback(server); + const response = await fetch(`http://127.0.0.1:${port}/rpc`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 'req-1', + method: 'agent_device.command', + params: { + command: 'get', + positionals: ['text', 'id=whatever'], + internal: { replayPlanStep: true, replayTargetGuard: { ref: '@e1' } }, + }, + }), + }); + + assert.equal(response.status, 200); + assert.equal(received.length, 1); + assert.equal( + received[0]?.internal, + undefined, + 'a wire-supplied `internal` must never reach the daemon request', + ); + } finally { + await closeLoopbackServer(server); + } +});