Skip to content

feat(replay): ADR 0012 migration steps 5+6 — resume + --update retirement#1211

Merged
thymikee merged 9 commits into
mainfrom
feat/replay-resume-and-update-retirement
Jul 11, 2026
Merged

feat(replay): ADR 0012 migration steps 5+6 — resume + --update retirement#1211
thymikee merged 9 commits into
mainfrom
feat/replay-resume-and-update-retirement

Conversation

@thymikee

Copy link
Copy Markdown
Member

Summary

Implements ADR-0012 ("Interactive Replay") migration step 5 (decision 4, replay resume) and step 6 (decision 1, --update retirement). Depends only on step 2 (#1197, merged) per the migration plan's ordering.

Step 5 — replay --from/--plan-digest resume

  • replay --from <n> --plan-digest <sha256> resumes at 1-based plan step n, skipping 1..n-1 without executing them. test rejects both flags — at the CLI-schema layer (replayCliSchema.allowedFlags only, not shared REPLAY_FLAGS) and, in case a caller reaches the daemon directly, at handleSessionReplayCommands (the only place that still sees the original test command name before it's rewritten to a nested replay request).
  • Every REPLAY_DIVERGENCE report now carries a real resume: { allowed, from, planDigest, reason? } (replacing the old always-{allowed:false} stub from feat(replay): ADR 0012 migration step 2 — structured replay divergence report #1197). planDigest is a canonical SHA-256 over the fully expanded plan (command, positionals, flags, control-flow shape, source provenance) — new src/replay/plan-digest.ts.
  • Preflight (src/daemon/handlers/session-replay-resume.ts) rejects INVALID_ARGS before any device action when: the plan digest no longer matches the current script (edit/include/platform-conditioned expansion changed), --from is out of range, a skipped step can produce outputEnv values a later step might consume (Maestro runScript), or the skipped range/target is runtime control flow (retry/runFlow.when — these execute dynamically and are always a single plan entry, never individually addressable by index).
  • plan-digest.ts lives in src/replay/ (type-only import of SessionAction, matching the existing script.ts pattern); the resume preflight (needs MAESTRO_RUNTIME_COMMAND, a value import) lives in src/daemon/handlers/ instead, to avoid a replay <-> compat import cycle (compat/replay-input.ts already imports from replay/).
  • session-replay-runtime.ts changes are minimal and localized per the coordination note: digest computation + entry-index preflight before the loop, entryIndex.value as the loop start, and planActions/planDigest threaded into the existing failure branch.

Step 6 — --update retirement

The ADR text (decision 1) mandates a no-op, not an error or flag removal: "after retirement, --update at any level performs no rewrite and returns the same bounded suggestions object." Since #1197 already made suggestions unconditional on every divergence, --update now runs identically to a plain replay.

  • Removed healReplayAction's retry-and-rewrite arm and its exclusive helpers from session-replay-heal.ts; kept collectReplaySelectorCandidates (still powers decision 1's ranked suggestions).
  • Removed the writeReplayScript call from the runtime loop, and the env-directive/${VAR}-interpolation/compat-flow refusal guards that existed only to protect that rewrite (a Maestro flow, env directives, and interpolated selectors are now all fine under --update — there's nothing to protect).
  • writeReplayScript itself (general .ad serializer, kept its own round-trip test coverage) ended up with zero remaining callers once the update path stopped invoking it — the check:production-exports gate flagged it as dead, so it's deleted; its tests were rewritten to exercise the underlying formatPortableActionLine/formatTargetAnnotationLines primitives directly (still live, shared with the session recorder's writer).
  • healed stays 0 on every response (kept on the wire shape for existing test-summary/JUnit reporters).

Docs

cli-help.ts (workflow topic + --update/--from flag descriptions), AGENTS.md selector-pipeline note, docs/maestro-compat-debt-map.md, website/docs/docs/replay-e2e.md (new "Replay divergence and resume" section replacing the old before/after rewrite examples), website/docs/docs/commands.md.

ADR-vs-code note

None encountered — decision 1 and decision 4 read unambiguously against the current code; the one judgment call (no-op vs. error vs. removal for --update) is directly answered by the ADR's own text quoted above.

Test plan

  • pnpm check:tooling (lint, typecheck, layering, production-exports, mcp-metadata, build, bundle-owner-files) — green
  • pnpm check:fallow (complexity/dead-code audit against merge-base) — green (one complexity finding on resolveReplayEntryIndex fixed by extracting validators)
  • pnpm test:unit (unit-core + android-adb) — green; 2 unrelated pre-existing flaky timeouts under full-suite CPU contention (client-metro.test.ts, daemon-client.test.ts), confirmed passing in isolation
  • pnpm test:smoke — green
  • New: src/replay/__tests__/plan-digest.test.ts, src/daemon/handlers/__tests__/session-replay-resume.test.ts (preflight unit tests: control-flow skip/target, outputEnv, from===1)
  • Extended session-replay-runtime.test.ts: resume happy path (skip-without-invoking, replayed count), flag-pairing/out-of-range/stale-digest rejection, retry-wrapped control-flow rejection end-to-end, --update no-write regression + healed:0 + env/interpolation guard removal, migrated JSON/malformed-.ad parse-error tests from the deleted replay-heal.test.ts
  • session-replay.test.ts: test --from/test --plan-digest daemon-layer rejection
  • commands/replay/index.test.ts: resumeFrom/resumePlanDigest CLI-reader/daemon-writer mapping, confirmed absent from test
  • Manual CLI check: help replay/help test/help workflow render correctly; test --from rejected pre-dispatch with INVALID_ARGS

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jul 11, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.7 MB 1.7 MB +1.8 kB
JS gzip 530.9 kB 531.6 kB +676 B
npm tarball 640.1 kB 641.1 kB +993 B
npm unpacked 2.3 MB 2.3 MB +2.5 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 21.6 ms 21.7 ms +0.1 ms
CLI --help 43.9 ms 44.3 ms +0.4 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/cli-help.js +1.2 kB +492 B
dist/src/registry.js +894 B +377 B
dist/src/internal/daemon.js +366 B +84 B
dist/src/session.js +251 B +52 B
dist/src/tv-remote.js -59 B -38 B

@github-actions

github-actions Bot commented Jul 11, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-11 13:07 UTC

@thymikee

Copy link
Copy Markdown
Member Author

CI fix pushed as 40c8b2e (heads-up for the in-flight review): the Integration Tests failure was the architecture-progress gate rejecting the new --from/--plan-digest flags as unclassified public CLI flags — not a test failure (all provider-integration tests were green). The commit classifies both as device-observable workflow flags in scripts/integration-progress-model.ts, adds a real resume loop to the Android lifecycle provider scenario (divergence -> assert resume.allowed/from/planDigest -> resume at the next index, replayed: 1), and refreshes the stale replayUpdate coverage reason. Verified locally: test:integration:provider 120/120 and test:integration:progress:check exit 0.

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review — ADR-0012 steps 5+6 (resume + --update retirement)

Reviewed in an isolated detached worktree at 2fc999c37. Verified against docs/adr/0012-interactive-replay.md decisions 4 and 1. Live-verified on iPhone 17 Pro sim (C25DBB5B), com.callstack.agentdevicelab. All gates below reproduced locally (check:tooling, check:fallow, full test:unit — 3732/3732 green).

1. BLOCKING — CI is red: new public flags unclassified by the integration-progress gate

gh pr checks 1211Integration Tests fails at the "Check Provider-backed integration architecture progress" step:

provider-backed integration progress check failed: unclassified public CLI flags: replayFrom, replayPlanDigest

scripts/integration-progress-model.ts requires every public CLI flag to be bucketed (see the Public CLI flag coverage outside provider-backed integration table). --from/--plan-digest (replayFrom/replayPlanDigest in cli-flags.ts) were added but never added to that classification table — they'd naturally sit in the same "parser/client-only command flags" bucket as replayMaestro/replayExportFormat. This needs a fix before merge; it's not flaky, reproduced identically against the PR head commit.

2. MEDIUM — planDigest deliberately excludes ${VAR} substitution values; verify this is the intended ADR reading, and document it

src/replay/plan-digest.ts computes the digest over the unexpanded ${VAR} text — variable substitution happens later, per-action, at invocation (resolveReplayAction in session-replay-action-runtime.ts). This is deliberate and tested (plan-digest.test.ts: "computeReplayPlanDigest never changes based on unsubstituted ${VAR} text").

Live-reproduced: identical digest regardless of -e value —

replay var-digest-demo.ad -e LABEL_VAR=alpha              → digest 53ba9b28...5ba5f
replay var-digest-demo.ad -e LABEL_VAR=zzz_completely_different_beta → digest 53ba9b28...5ba5f  (same)

The ADR's own resume paragraph says "edits, include changes, or environment-driven expansion cannot silently retarget ordinal N" — which reads (at least on one plausible interpretation) as requiring digest sensitivity to variable-driven behavior, not just to the presence of a ${VAR} placeholder. The counter-argument for the current design is also in the ADR text: "The only variables available after a resume are explicit script/header, CLI, and shell inputs" — implying CLI (-e) inputs are legitimately expected to vary across a resume, similar to how the ADR puts app-state repair on the caller. Both readings are defensible from the text; this looks like it needs an explicit maintainer call, not an implicit one buried in a source comment.

Practical risk either way: an agent that gets a divergence report, then resumes with a different -e/env value than the original run used (accidentally, or "helpfully" auto-corrected), gets no signal that downstream step behavior (e.g. fill ${TEXT}, open --launch-url ${URL}) changed — the plan-digest match gives a false sense that "nothing changed." Recommend at minimum: call this out explicitly in --help replay/--help workflow ("--plan-digest does not pin -e/shell-env variable values — only the script text and structural expansion"), so it's a documented caller responsibility rather than a silent gap.

3. INFO — comment-only edits correctly bump the digest (verified, matches spec)

canonicalizeAction includes source: { path, line }, and actionLines are literal physical file line numbers (script.ts: actionLines.push(index + 1)), so inserting a comment/blank line above an action shifts its digest. Live-reproduced: adding one comment line then resuming with the old digest → rejected INVALID_ARGS, zero device-action events logged (only request.started/request.finished, 1ms). This matches the ADR's explicit inclusion of "source provenance" in the digest — correct, if occasionally surprising (a purely cosmetic edit forces a fresh full replay). Worth a one-line help-text mention, not a defect.

4. Verified sound — resume preflight honesty (axis 2)

  • resolveReplayEntryIndex/validateReplayResumeRequest/evaluateReplayResumePreflight (session-replay-runtime.ts, session-replay-resume.ts) run entirely before the loop starts; the loop only begins at entryIndex.value after entryIndex.ok is true. Live-verified: rejections (digest mismatch) produce zero device-action log entries.
  • Out-of-range --from, control-flow-interior skip/target (retry/maestroRunFlowWhen), and outputEnv-producing skipped steps (Maestro runScript) are all rejected INVALID_ARGS pre-action — code-verified against session-replay-resume.ts.
  • test --from rejected at both layers as claimed: CLI-schema (testCliSchema.allowedFlags in commands/replay/index.ts omits replayFrom/replayPlanDigest) and daemon (handleSessionReplayCommands). Both live-verified — CLI layer: "Flags --from, --plan-digest are not supported for command test."
  • Live end-to-end resume: broke a step, got resume: {allowed:true, from:4, planDigest:...}, resumed with --from 5 --plan-digest <sha> after manually completing the equivalent step live → replayed: 2 (only the tail of a 6-step plan executed; confirmed via session event log that no open/earlier wait actions re-ran).

5. Verified sound — retirement (axis 3)

  • writeReplayScript fully deleted; grepped the whole repo (including dynamic-import patterns) — zero remaining callers outside a comment in script.test.ts.
  • healReplayAction's retry-and-rewrite arm removed cleanly; collectReplaySelectorCandidates retained and still the sole source for ranked suggestions per decision 1.
  • replayUpdate stays wired on the wire shape (client → CLI → daemon request), as the ADR requires ("kept... regardless of level" pre-retirement / "no-op... at any level" post), but is fully inert in runReplayScriptFile — the shouldUpdate branch and its snapshot/rewrite arm are gone outright, not merely short-circuited. No residual fs-write path reachable from replay.

6. Verified sound — digest canonicality (axis 1 b/d)

Include-file content changes and iOS-vs-Android runs both change the digest, by construction (action content flows through canonicalizeAction; metadata.platform is a top-level digest field) — matches plan-digest.test.ts coverage.

7. INFORMATIONAL — merge-ordering collision with #1209

#1209 (target-binding verification enforcement) and this PR both substantially rewrite the same loop body in session-replay-runtime.ts:

  • Textual: #1209 moves buildReplayDivergenceFailureResponse out to session-replay-divergence.ts and wraps each dispatch in a new verifyReplayActionTarget pre-check; this PR changes the loop's start index (entryIndex.value) and removes the healReplayAction branch but leaves buildReplayDivergenceFailureResponse local. Whoever rebases second has a real (not just line-noise) merge here.
  • Semantic gap, more important than the textual conflict: #1209's session-replay-target-verification.ts currently hardcodes resume: REPLAY_DIVERGENCE_RESUME_NOT_SUPPORTED / { allowed: false, reason: 'resume not yet supported' } on every target-binding divergence it builds (written before this PR's resume machinery existed). Post-merge, target-binding divergences (selector-miss/identity-mismatch/identity-unverifiable) would keep reporting resume.allowed:false with a stale reason even when resume from that step is actually safe, unless whoever merges second explicitly wires this PR's buildReplayDivergenceResume into #1209's target-verification path. Recommend landing this PR (#1211) first — smaller footprint on the shared loop — then rebasing #1209 to consume buildReplayDivergenceResume.

Gates

  • pnpm check:tooling, pnpm check:fallow: green in the scratch worktree.
  • pnpm test:unit: 401 files / 3732 tests green.
  • Targeted resume/digest/retirement suites: all green (93 tests across plan-digest.test.ts, session-replay-resume.test.ts, session-replay-runtime.test.ts, session-replay.test.ts, commands/replay/index.test.ts, script.test.ts).
  • gh pr checks 1211: red — see finding 1.

No paragraph-long workaround-justification comments found in the diff — the added comments are scoped ADR-reference architecture notes, consistent with repo convention.

thymikee added a commit that referenced this pull request Jul 11, 2026
…inner against the verified member

Coordinator addition to step 4 (PR #1209): pre-action verification resolves
without dispatch's occlusion-filtering/visibility-preferring guards, so
verification's winner W could differ from the node dispatch actually acts
on — verified-but-taps-a-different-element, or a false identity-mismatch.
Close the split with a cheap cross-check at dispatch's own resolution:

- Verified outcomes now carry the verified member's normalized local
  identity (`ReplayVerifiedTargetGuard`: readNodeLocalIdentity(winner) +
  verification's matchCount). The replay loop threads it onto the dispatch
  request as `internal.replayTargetGuard`.
- Interaction handlers (press/click/longpress/fill in interaction-touch.ts,
  get in selector-runtime.ts) pass it into command options as
  `expectedResolvedTarget`, and a guarded dispatch skips the direct-iOS
  selector fast path and the native-ref tapTarget/fillTarget fast paths —
  the guard needs the runtime tree resolution to have a node to check.
- `assertExpectedResolvedTarget` (resolution.ts) compares the resolution
  winner's identity — PRE-promotion (hittable-ancestor promotion
  deliberately retargets to the same element's actionable container) and
  PRE-action — against the verified identity, and refuses with
  `details.reason: 'replay_target_guard_mismatch'` on a mismatch. Enforced
  on the selector path, the ref path, and get's ref/selector reads.
- The replay loop converts that refusal into an identity-mismatch
  target-binding divergence (`buildReplayTargetGuardMismatchResponse`):
  classification == kind, matchCount from verification's recorded-selector
  resolution (present, per the rule), recorded/observed identities,
  field-level mismatches, fresh screen observation — via the same
  bounding/sanitizing builder as every other target-binding divergence
  (finalize refactored into module-level
  buildTargetBindingDivergenceResponse for the two callers).
- The identity reader moved to src/replay/target-identity-node.ts
  (readNodeLocalIdentity + localIdentitiesEqual + the reason marker) so the
  commands and daemon layers share it without a layering back-edge;
  session-target-evidence.ts's boundedLocalIdentity is now an alias.

The guard is replay-only by construction: `internal.replayTargetGuard` is
set exclusively by the replay step loop for annotated verified actions;
live interactive commands never carry it.

PR #1211 coordination: buildTargetBindingDivergenceResponse is the single
resume site for target-binding divergences, marked as the step-5 wiring
point — after #1211 merges, replace the not-supported stub with the real
computed resume (pre-action divergences typically allow same-step resume)
plus a test. Kept as a small follow-up by construction.

Tests: split-resolver fixture (covered deeper A vs visible sibling B —
verification verifies A, dispatch resolves B) proving the guard refuses
pre-action with zero backend taps, the matching-identity pass-through, the
ref-fast-path forcing, guard threading on the dispatch request (and its
absence for unannotated actions), and end-to-end conversion of a guard
mismatch into an identity-mismatch divergence with matchCount present.
thymikee added a commit that referenced this pull request Jul 11, 2026
…inner against the verified member

Coordinator addition to step 4 (PR #1209): pre-action verification resolves
without dispatch's occlusion-filtering/visibility-preferring guards, so
verification's winner W could differ from the node dispatch actually acts
on — verified-but-taps-a-different-element, or a false identity-mismatch.
Close the split with a cheap cross-check at dispatch's own resolution:

- Verified outcomes now carry the verified member's normalized local
  identity (`ReplayVerifiedTargetGuard`: readNodeLocalIdentity(winner) +
  verification's matchCount). The replay loop threads it onto the dispatch
  request as `internal.replayTargetGuard`.
- Interaction handlers (press/click/longpress/fill in interaction-touch.ts,
  get in selector-runtime.ts) pass it into command options as
  `expectedResolvedTarget`, and a guarded dispatch skips the direct-iOS
  selector fast path and the native-ref tapTarget/fillTarget fast paths —
  the guard needs the runtime tree resolution to have a node to check.
- `assertExpectedResolvedTarget` (resolution.ts) compares the resolution
  winner's identity — PRE-promotion (hittable-ancestor promotion
  deliberately retargets to the same element's actionable container) and
  PRE-action — against the verified identity, and refuses with
  `details.reason: 'replay_target_guard_mismatch'` on a mismatch. Enforced
  on the selector path, the ref path, and get's ref/selector reads.
- The replay loop converts that refusal into an identity-mismatch
  target-binding divergence (`buildReplayTargetGuardMismatchResponse`):
  classification == kind, matchCount from verification's recorded-selector
  resolution (present, per the rule), recorded/observed identities,
  field-level mismatches, fresh screen observation — via the same
  bounding/sanitizing builder as every other target-binding divergence
  (finalize refactored into module-level
  buildTargetBindingDivergenceResponse for the two callers).
- The identity reader moved to src/replay/target-identity-node.ts
  (readNodeLocalIdentity + localIdentitiesEqual + the reason marker) so the
  commands and daemon layers share it without a layering back-edge;
  session-target-evidence.ts's boundedLocalIdentity is now an alias.

The guard is replay-only by construction: `internal.replayTargetGuard` is
set exclusively by the replay step loop for annotated verified actions;
live interactive commands never carry it.

PR #1211 coordination: buildTargetBindingDivergenceResponse is the single
resume site for target-binding divergences, marked as the step-5 wiring
point — after #1211 merges, replace the not-supported stub with the real
computed resume (pre-action divergences typically allow same-step resume)
plus a test. Kept as a small follow-up by construction.

Tests: split-resolver fixture (covered deeper A vs visible sibling B —
verification verifies A, dispatch resolves B) proving the guard refuses
pre-action with zero backend taps, the matching-identity pass-through, the
ref-fast-path forcing, guard threading on the dispatch request (and its
absence for unannotated actions), and end-to-end conversion of a guard
mismatch into an identity-mismatch divergence with matchCount present.
@thymikee

Copy link
Copy Markdown
Member Author

Review at 40c8b2e7 found blockers; the branch also became DIRTY after #1210 merged, so it must be rebased before fixes/re-review.

  • P1: rebase conflicts include the removed src/cli/parser/cli-flags.ts surface plus replay runtime/heal, client normalizers, and AGENTS. Port flags to the new workflow flag ownership on current main.
  • P1: planDigest omits behavior-affecting SessionAction.runtime; changing replay open launch/bundle/Metro runtime inputs can preserve a stale digest.
  • P1: planDigest deliberately omits targetEvidence, but merged/current step-4 enforcement consumes it before dispatch. Editing a target-v1 annotation changes replay behavior without invalidating resume.
  • P1: digest metadata uses script platform/target, not the effective CLI override from replayReq; the same digest can resume against another CLI-selected app target.
  • P2: SkillGym still requires --update to maintain the script in place, contradicting the retired no-write behavior.
  • P2: new behavior was added to 599-line session-replay-runtime.ts and expands its test to 1,050 LOC. Extract coherent resume/runtime modules and mirrored tests per the mandatory >500 LOC rule.

Please rebase first, then address these on the new architecture and request re-review on the resulting head.

thymikee added 2 commits July 11, 2026 09:52
…ment

Step 5 (decision 4, resume): replay --from <n> --plan-digest <sha256>
resumes at a 1-based plan step, skipping 1..n-1 without executing them.
Every divergence report now carries a real resume object (allowed, from,
planDigest, reason?) computed by a preflight that rejects INVALID_ARGS
before any action when: the plan digest no longer matches the current
script (edits/includes/platform-conditioned expansion), --from is out of
range, a skipped step can produce outputEnv values, or the skipped range
or resume target is runtime control flow (retry/runFlow.when — these are
single plan entries, never individually addressable). `test` rejects
--from/--plan-digest both at the CLI-schema layer and at the daemon
dispatch layer (the original command name is only visible before test
rewrites its nested request to `command: 'replay'`).

New modules: src/replay/plan-digest.ts (canonical SHA-256 plan digest)
and src/daemon/handlers/session-replay-resume.ts (preflight + the
report's resume object), kept out of src/replay/ to avoid a
replay<->compat import cycle.

Step 6 (decision 1, retirement): --update/-u no longer rewrites .ad
files. The ADR mandates a no-op, not an error or flag removal: --update
now runs identically to a plain replay and returns the same bounded
suggestions every divergence already carries. Removed: healReplayAction's
retry-and-rewrite arm and its exclusive helpers (collectReplaySelectorCandidates
stays — decision 1's suggestions still use it), the write call from the
runtime loop, and the env/${VAR}-interpolation/compat-flow refusal guards
that existed only to protect that rewrite. writeReplayScript itself keeps
its own round-trip tests but is otherwise unused now; deleted after the
production-exports gate flagged it as dead.

Docs: cli-help.ts workflow topic + --update/--from flag help, AGENTS.md
selector pipeline note, maestro-compat-debt-map.md, website replay-e2e.md
and commands.md updated for the retired rewrite and the new resume loop.
The Integration Tests job's architecture-progress gate
(test:integration:progress:check) requires every public CLI flag to be
classified; --from/--plan-digest (replayFrom/replayPlanDigest) were
unclassified. Classify them as device-observable workflow flags and add
real provider-backed coverage to the Android lifecycle scenario: a full
replay diverges on a missing selector, the report's resume object is
asserted (allowed/from/planDigest), and resuming at the next index
replays only the tail. Also refresh the stale replayUpdate reason
("selector-healing replay update" -> the retired no-op).
@thymikee thymikee force-pushed the feat/replay-resume-and-update-retirement branch from 40c8b2e to 931f914 Compare July 11, 2026 07:55
@thymikee

Copy link
Copy Markdown
Member Author

Rebased onto origin/main (now includes #1193 selector-resolution disclosure and #1210 architecture-ownership consolidation) — new head is 931f914 (was 40c8b2e; same two commits, rebased). Non-trivial resolutions, both sides preserved: (1) #1210 deleted src/cli/parser/cli-flags.ts — my --from/--plan-digest definitions and the retired --update description re-landed in src/commands/cli-grammar/flag-definitions-workflow.ts, the CliFlags fields in src/contracts/cli-flags.ts, and the client flag mapping in the relocated buildFlags (src/commands/command-flags.ts); (2) replay success payload keeps #1210's satisfies ReplayCommandResult AND my resumed-tail replayed count; (3) session-replay-heal.ts imports updated to #1210's src/selectors/ + kernel/collections paths over my slimmed suggestions-only module. Full gate re-run green locally: tooling (lint/typecheck/layering/production-exports/mcp-metadata/build/bundle-owner-files), fallow audit, unit 3745/3745, provider-integration lifecycle scenario, integration progress check exit 0.

@thymikee

Copy link
Copy Markdown
Member Author

Re-review of rebased head 931f9143: the architecture rebase is clean and CI is 23/23, but the six blockers from the previous review remain unchanged.

  • P1: planDigest omits execution-affecting SessionAction.runtime.
  • P1: it binds script metadata rather than effective CLI platform/target, so a digest can be reused against another target.
  • P1: it explicitly omits targetEvidence, although step-4 verification consumes it pre-action.
  • P2: SkillGym still teaches --update as an in-place rewrite despite retirement to a no-op.
  • P2: session-replay-runtime.test.ts is 1,050 LOC after adding 406 lines.
  • P2: session-replay-runtime.ts remains 600 LOC and needs coherent extraction before adding behavior.

The rebase fixed ownership conflicts only; it did not address these correctness/guidance/locality findings.

@thymikee

Copy link
Copy Markdown
Member Author

Fixer re-review at 85027a57: the six original blockers are closed, but one locality issue remains. The test split does not mirror source topology 1:1: session-replay-runtime-artifacts.ts is tested from unrelated session-replay.test.ts with no matching artifacts test, while maintenance/redaction test files have no matching production modules. Please align test filenames/ownership with the split source concepts without growing another aggregation.

@thymikee

Copy link
Copy Markdown
Member Author

One final P2 guidance inconsistency remains at 1549aec7: the revised SkillGym case requires manually editing the drifted selector and then resuming with the divergence's old --plan-digest. That workflow cannot succeed: selector edits change the digest and preflight correctly returns INVALID_ARGS; ADR 0012 requires a fresh full replay after script edits. The website troubleshooting line about resuming “past a repaired step” is similarly ambiguous. Teach two distinct paths: edit script -> fresh full replay/new digest; unchanged script + manually repaired app state/completed failed action -> resume with the reported digest/index.

@thymikee

Copy link
Copy Markdown
Member Author

Final P2 wording issue at 0fc26ec7: website/docs/docs/commands.md still describes --from 4 as “Resume past a repaired step,” but replay starts at step 4 and executes it. Align the command reference with the corrected replay guide so users do not manually complete a step and then repeat it.

@thymikee

Copy link
Copy Markdown
Member Author

Final re-review at c89f220f: all review findings and CI regressions are resolved. Digest safety now covers runtime hints, target evidence, and effective CLI target/platform; runtime/test topology is below limits and mirrored; SkillGym/docs distinguish edited-script fresh replay from unchanged-script state repair/resume. Independent GPT-5.5 re-review found no remaining issues, the focused SkillGym case passed 2/2, and all 23 checks are green.

ready-for-human is still withheld because this device-facing resume path has no practical current-head simulator/emulator evidence showing: a real divergence emits resume, app state is repaired without editing the script, and replay with the reported --from/--plan-digest executes the expected tail only. Static/provider contracts and generic smoke jobs do not prove that exact shipped workflow.

…R 0012 + workflow help)

Settled decision from the PR #1211 re-review (maintainer-approved): interpolated
${VAR}/--env/AD_VAR_* VALUES are deliberately NOT part of the resume plan digest.
Substitution happens after the digest is computed over the still-unsubstituted
${VAR} text, so re-running the same script with different variable values keeps
the same digest and stays resumable — supplying the right values on resume is the
caller's responsibility. The digest still binds the script/includes, the effective
--platform/--target, and per-action runtime hints + target-v1 identity. Documented
in ADR 0012 decision 4 and the `help workflow` resume topic.
@thymikee

Copy link
Copy Markdown
Member Author

New head 747c58b: I rebased onto the maintainer's own fixes for the six blockers (plan-digest completeness, module/test splits, skillgym, and the three CI-regression fixes — all authored by @thymikee). My commit on top adds only the settled ${VAR}-out-of-digest clarification to ADR 0012 decision 4 and the help workflow resume topic (the one item not in the maintainer's commits). Verified green locally before push: typecheck, oxlint --deny-warnings, format:check, fallow audit (--base origin/main), production-exports, and the cli-help test. Only two doc files changed in this commit.

thymikee added a commit that referenced this pull request Jul 11, 2026
…inner against the verified member

Coordinator addition to step 4 (PR #1209): pre-action verification resolves
without dispatch's occlusion-filtering/visibility-preferring guards, so
verification's winner W could differ from the node dispatch actually acts
on — verified-but-taps-a-different-element, or a false identity-mismatch.
Close the split with a cheap cross-check at dispatch's own resolution:

- Verified outcomes now carry the verified member's normalized local
  identity (`ReplayVerifiedTargetGuard`: readNodeLocalIdentity(winner) +
  verification's matchCount). The replay loop threads it onto the dispatch
  request as `internal.replayTargetGuard`.
- Interaction handlers (press/click/longpress/fill in interaction-touch.ts,
  get in selector-runtime.ts) pass it into command options as
  `expectedResolvedTarget`, and a guarded dispatch skips the direct-iOS
  selector fast path and the native-ref tapTarget/fillTarget fast paths —
  the guard needs the runtime tree resolution to have a node to check.
- `assertExpectedResolvedTarget` (resolution.ts) compares the resolution
  winner's identity — PRE-promotion (hittable-ancestor promotion
  deliberately retargets to the same element's actionable container) and
  PRE-action — against the verified identity, and refuses with
  `details.reason: 'replay_target_guard_mismatch'` on a mismatch. Enforced
  on the selector path, the ref path, and get's ref/selector reads.
- The replay loop converts that refusal into an identity-mismatch
  target-binding divergence (`buildReplayTargetGuardMismatchResponse`):
  classification == kind, matchCount from verification's recorded-selector
  resolution (present, per the rule), recorded/observed identities,
  field-level mismatches, fresh screen observation — via the same
  bounding/sanitizing builder as every other target-binding divergence
  (finalize refactored into module-level
  buildTargetBindingDivergenceResponse for the two callers).
- The identity reader moved to src/replay/target-identity-node.ts
  (readNodeLocalIdentity + localIdentitiesEqual + the reason marker) so the
  commands and daemon layers share it without a layering back-edge;
  session-target-evidence.ts's boundedLocalIdentity is now an alias.

The guard is replay-only by construction: `internal.replayTargetGuard` is
set exclusively by the replay step loop for annotated verified actions;
live interactive commands never carry it.

PR #1211 coordination: buildTargetBindingDivergenceResponse is the single
resume site for target-binding divergences, marked as the step-5 wiring
point — after #1211 merges, replace the not-supported stub with the real
computed resume (pre-action divergences typically allow same-step resume)
plus a test. Kept as a small follow-up by construction.

Tests: split-resolver fixture (covered deeper A vs visible sibling B —
verification verifies A, dispatch resolves B) proving the guard refuses
pre-action with zero backend taps, the matching-identity pass-through, the
ref-fast-path forcing, guard threading on the dispatch request (and its
absence for unannotated actions), and end-to-end conversion of a guard
mismatch into an identity-mismatch divergence with matchCount present.
@thymikee

Copy link
Copy Markdown
Member Author

Live resume-loop evidence (head 747c58b)

Built feat/replay-resume-and-update-retirement at 747c58b in a scratch git worktree (pnpm install --frozen-lockfile && pnpm build:node, no XCUITest runner changes in this PR so the pre-existing runner build was reused). Drove it live against the react-navigation playground (org.reactnavigation.playground) on an iPhone 17 Pro simulator (iOS 26.2, UDID C25DBB5B-9254-4293-A8D5-2785C78DE03A), Metro on 127.0.0.1:8082, isolated session/state-dir (spot-1211 / /tmp/spot-1211-state).

Note: the PR head has since advanced to 0b589a93 ("docs: clarify replay digest interpolation"); git diff --stat 747c58b4b..0b589a93 confirms that commit only touches docs/adr/0012-interactive-replay.md, src/cli/parser/cli-help.ts, and its test — no functional code — so this evidence still reflects the current PR behavior.

Flow exercised: rne://stack-prevent-remove (React Navigation's Stack "prevent remove" demo — Input screen with a beforeRemove guard, and an Article screen), 4 label presses: Push Article → Push Input → Discard and go back → Pop to top.

1. Record (open --save-script) — PASS

Recorded an 8-step .ad (2 open + wait 3000 + 4 press + close), each press step annotated with agent-device:target-v1 evidence (role/label/ancestry/rect):

context platform=ios device="iPhone 17 Pro" kind=simulator theme=unknown
open "org.reactnavigation.playground" --relaunch --platform ios --metro-host 127.0.0.1 --metro-port 8082 --bundle-url http://127.0.0.1:8082/.expo/.virtual-metro-entry.bundle?platform=ios&dev=true&minify=false
open "rne://stack-prevent-remove"
wait 3000
# agent-device:target-v1 {"role":"button","label":"Push Article",...,"verification":"verified"}
press "role=\"button\" label=\"Push Article\" || label=\"Push Article\""
# agent-device:target-v1 {"role":"button","label":"Push Input",...,"verification":"verified"}
press "role=\"button\" label=\"Push Input\" || label=\"Push Input\""
# agent-device:target-v1 {"role":"button","label":"Discard and go back",...,"verification":"verified"}
press "role=\"button\" label=\"Discard and go back\" || label=\"Discard and go back\""
# agent-device:target-v1 {"role":"button","label":"Pop to top",...,"verification":"verified"}
press "role=\"button\" label=\"Pop to top\" || label=\"Pop to top\""
close

2. Clean replay — PASS

$ agent-device replay spot-1211-stack-prevent-remove.ad --session spot-1211 --state-dir /tmp/spot-1211-state --platform ios --udid C25DBB5B-9254-4293-A8D5-2785C78DE03A
Replayed 8 steps in 14.7s

Deterministic, zero model turns, single CLI invocation.

3. Induced divergence → resume loop — PASS (the ready-for-human proof)

3a. Real divergence, not a contrived one. This reproduces the exact class of bug ADR 0012 documents in its "Live hands-on evidence" section: the playground persists nav state to AsyncStorage, and a relaunch can restore mid-flow state that a subsequent deep link doesn't always override. I drove the app to the Article screen inside the same stack-prevent-remove navigator (so its state gets persisted), then ran the recorded .ad unmodified. Step 4 (press "Push Article") legitimately diverged because relaunch + deep link + the recorded wait 3000 landed back on the persisted Article screen, which has no "Push Article" button:

{
  "success": false,
  "error": {
    "code": "REPLAY_DIVERGENCE",
    "message": "Replay failed at step 4 (press \"role=\\\"button\\\" label=\\\"Push Article\\\" || label=\\\"Push Article\\\"\"): Selector did not match: role=\"button\" label=\"Push Article\" || label=\"Push Article\"",
    "details": {
      "step": 4,
      "action": "press",
      "divergence": {
        "version": 1,
        "kind": "action-failure",
        "step": { "index": 4, "source": { "line": 6 } },
        "screen": {
          "state": "available",
          "refsGeneration": 245929,
          "refs": [
            { "ref": "e3", "role": "other", "label": "Article" },
            { "ref": "e6", "role": "button", "label": "Push Input" },
            { "ref": "e7", "role": "button", "label": "Pop to top" }
          ]
        },
        "resume": {
          "allowed": true,
          "from": 4,
          "planDigest": "a7b656521f01a592b2234f4e184d968ea107d8be74c8fbbfb012e73437d50c12"
        }
      }
    }
  }
}

(refs trimmed above to the disambiguating entries; full response also carries the Article body text nodes.)

3b. Manual repair. Tapped the header back button ("Input, back") to put the app on the exact screen step 4 expects (root Input, "Push Article" visible again) — confirmed via snapshot.

3c. Resume from the reported step — PASS.

$ agent-device replay spot-1211-stack-prevent-remove.ad --from 4 --plan-digest a7b656521f01a592b2234f4e184d968ea107d8be74c8fbbfb012e73437d50c12 --session spot-1211 --state-dir /tmp/spot-1211-state --platform ios --udid C25DBB5B-9254-4293-A8D5-2785C78DE03A --json
{
  "success": true,
  "data": {
    "replayed": 5,
    "healed": 0,
    "session": "spot-1211",
    "message": "Replayed 5 steps in 5.1s"
  }
}

replayed: 5 is exactly steps 4–8 (press×4 + close) of the 8-step plan — steps 1–3 (open --relaunch, deep link, wait 3000) were not re-executed, confirmed both by the reduced step count and by the session actually needing a fresh open afterward (a follow-up snapshot on the same session correctly errored SESSION_NOT_FOUND, proving close — step 8 — ran and nothing re-opened a new app session in between).

4. Digest-mismatch rejection — PASS

Edited the recorded script's step-4 selector (Push ArticlePush Article Now) and replayed with the stale digest from 3a:

$ agent-device replay spot-1211-stack-prevent-remove-edited.ad --from 4 --plan-digest a7b656521f01a592b2234f4e184d968ea107d8be74c8fbbfb012e73437d50c12 --session spot-1211 --state-dir /tmp/spot-1211-state --platform ios --udid C25DBB5B-9254-4293-A8D5-2785C78DE03A --json
{
  "success": false,
  "error": {
    "code": "INVALID_ARGS",
    "message": "replay --plan-digest does not match the current plan digest; the script, its includes, or its platform-conditioned expansion changed since the divergence report was generated. Run a fresh full replay to get a new digest.",
    "diagnosticId": "mrgc6tf9-ed1a2f18"
  }
}

Rejected before any device action (no simulator activity), per the ADR's "mismatch...fail INVALID_ARGS before any action" rule.

5. --update no-op — PASS

$ agent-device replay spot-1211-stack-prevent-remove.ad --update --session spot-1211 --state-dir /tmp/spot-1211-state --platform ios --udid C25DBB5B-9254-4293-A8D5-2785C78DE03A
Replayed 8 steps in 14.1s

Same shape/message as the plain replay in item 2 ({ replayed: 8, healed: 0, message: "Replayed N steps in X.Xs" }). File proof — ran --update twice, checked mtime + md5 before/after both runs:

before: mtime=Jul 11 14:16:20 2026  size=2127  md5=cdb3b9a4e9d997aae24e64453dfd7ec7
after:  mtime=Jul 11 14:16:20 2026  size=2127  md5=cdb3b9a4e9d997aae24e64453dfd7ec7

Unchanged — --update never rewrote the .ad file, confirming the ADR 0012 retirement is real, not just documented.


Verdict: 5/5 PASS. All evidence gathered live against a real iOS simulator, no fabricated output. Scratch worktree, daemon, and XCUITest runner process torn down after; simulator and Metro 8082 left running untouched, port 8081 and the PagerView-named simulator were never touched.

@thymikee thymikee merged commit e2bfed5 into main Jul 11, 2026
23 checks passed
@thymikee thymikee deleted the feat/replay-resume-and-update-retirement branch July 11, 2026 13:06
thymikee added a commit that referenced this pull request Jul 11, 2026
…lution guard + step-5 resume wiring

Rebuilt onto current main (#1210 selector relocation, #1211 resume + --update
retirement, #1213, #1215) as one coherent commit — the prior 4-commit history
collided structurally with #1211's own session-replay-runtime split.

Step 4 (decision 3, enforcement): every annotated resolved-target replay/test
action is verified against a fresh pre-action snapshot BEFORE dispatch
(verifyReplayActionTarget → classifyReplayTarget, decision 3's six paths). Only
a verified outcome sends the action; selector-miss / identity-mismatch /
identity-unverifiable each return a complete target-binding REPLAY_DIVERGENCE
built from its own capture, with targetBinding {classification==kind,
matchCount per the presence rule, recorded/observed, mismatches, candidates}
through #1211's shared failure-response + bounding/sanitizer pipeline.

Post-resolution guard: a verified outcome mints internal.replayTargetGuard
(the verified member's identity); dispatch's own resolution (occlusion/
visibility guards verification does not replicate) must land on the SAME
element or assertExpectedResolvedTarget refuses pre-action, which the loop
converts to an identity-mismatch divergence. Guarded dispatch skips the
direct-iOS and native-ref fast paths so a tree node exists to check.

Step-5 resume wiring (this rebase): the target-binding divergence resume stub
is replaced with #1211's buildReplayDivergenceResume — a pre-action divergence
resumes AT the failed step (from = step index, real SHA-256 planDigest, allowed
unless preflight rejects the skipped range). buildTargetBindingDivergenceResponse
is the single resume site; planActions/planDigest are threaded from the loop.

--update healing is retired by #1211, so the in-branch "verify healed action"
work is dropped (its module and test removed as moot).

Tests: classification (13), guard (4), token (5), runtime end-to-end (9, incl.
old-script pass-through, guard threading + mismatch conversion, and a real
computed resume assertion), plus target-binding wire tests in divergence.test.ts
(classification==kind, matchCount presence, overflow-fallback targetBinding,
computed-resume-not-stub, text report). Also repoints
daemon-http-disconnect.test.ts already handled upstream by #1215.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant