diff --git a/scripts/cdp-bridge/dist/cdp/recovery.js b/scripts/cdp-bridge/dist/cdp/recovery.js index 42fd4882..e243b617 100644 --- a/scripts/cdp-bridge/dist/cdp/recovery.js +++ b/scripts/cdp-bridge/dist/cdp/recovery.js @@ -1,6 +1,21 @@ const FRESHNESS_PROBE_MS = 2000; const STALE_RETRY_DELAY_MS = 500; const STALE_RETRY_PROBE_MS = 3000; +// GH #186: cross-tool "the CDP target may be stale" signal. A device-session +// runner-leak recovery can re-foreground/relaunch the app out from under CDP, +// after which the next cdp_* call would hit a ~47s STALE_TARGET timeout in the +// handler. The recovery sets this flag; withConnection consumes it before the +// next handler and proactively re-pins via recoverFromStaleTarget, turning that +// 47s catch-path recovery into a fast pre-handler one. Process-scoped boolean — +// a single MCP serves one device at a time, so no per-client keying is needed. +let cdpStale = false; +export function markCdpStale() { cdpStale = true; } +/** Read-and-clear: returns whether the stale flag was set, resetting it. */ +export function consumeCdpStale() { + const was = cdpStale; + cdpStale = false; + return was; +} export async function probeFreshness(client, timeoutMs = FRESHNESS_PROBE_MS) { if (!client.isConnected) { return { fresh: false, version: null, probed: false }; diff --git a/scripts/cdp-bridge/dist/domain/maestro-validator.js b/scripts/cdp-bridge/dist/domain/maestro-validator.js index d5776f74..1f91bf37 100644 --- a/scripts/cdp-bridge/dist/domain/maestro-validator.js +++ b/scripts/cdp-bridge/dist/domain/maestro-validator.js @@ -16,6 +16,8 @@ * default-deny rationale. */ import yaml from 'yaml'; +import { join, dirname, isAbsolute, sep } from 'node:path'; +import { readFileSync, realpathSync } from 'node:fs'; // ── Errors ────────────────────────────────────────────────────────── export class MaestroValidationError extends Error { constructor(message) { @@ -105,6 +107,12 @@ const ALLOWED_COMMANDS = new Set([ 'killApp', 'stopApp', 'tap', + // GH #186: runFlow (conditional dialog handling — deep-link "Open in", Expo + // dev-client picker). Validated specially (validateRunFlowValue) so nested + // `commands` get full command-level allowlist checks, and {file} refs are + // securely resolved + expanded inline (expandRunFlows) — they are NOT passed + // through generic validateValue, which would miss nested denied commands. + 'runFlow', ]); const DENIED_COMMANDS = new Set([ 'runScript', @@ -153,8 +161,52 @@ function validateCommand(cmd) { if (!ALLOWED_COMMANDS.has(key)) { throw new MaestroValidationError(`Command not in allowlist: ${key}`); } + if (key === 'runFlow') { + validateRunFlowValue(cmd[key]); + return; + } validateValue(cmd[key]); } +/** + * GH #186: validate a runFlow value. The string/`{file}` form is a sub-flow + * file ref (a safe scalar here; secure resolution happens in expandRunFlows). + * Inline `commands` are validated as COMMANDS (recursive allowlist check) — the + * critical difference from generic validateValue, which would let a nested + * `runScript` slip through as a plain scalar key/value. + */ +function validateRunFlowValue(v) { + if (typeof v === 'string') { + if (!isSafeMaestroScalar(v)) { + throw new MaestroValidationError(`Unsafe runFlow file ref: ${JSON.stringify(v).slice(0, 80)}`); + } + return; + } + if (v === null || typeof v !== 'object' || Array.isArray(v)) { + throw new MaestroValidationError(`runFlow value must be a file string or an object, got ${Array.isArray(v) ? 'array' : typeof v}`); + } + const obj = v; + if ('file' in obj && (typeof obj.file !== 'string' || !isSafeMaestroScalar(obj.file))) { + throw new MaestroValidationError(`runFlow.file must be a safe scalar string`); + } + if ('when' in obj) + validateValue(obj.when); + if ('commands' in obj) { + if (!Array.isArray(obj.commands)) { + throw new MaestroValidationError(`runFlow.commands must be an array`); + } + for (const c of obj.commands) + validateCommand(c); + } + // Any other keys (env/label/config) are validated as generic safe values. + for (const [k, val] of Object.entries(obj)) { + if (k === 'file' || k === 'when' || k === 'commands') + continue; + if (!isSafeMaestroScalar(k)) { + throw new MaestroValidationError(`Unsafe runFlow key: ${JSON.stringify(k).slice(0, 80)}`); + } + validateValue(val); + } +} function validateValue(v) { if (v === null || v === undefined) return; @@ -182,6 +234,117 @@ function validateValue(v) { } throw new MaestroValidationError(`Unsupported value type: ${typeof v}`); } +// GH #186: recognize a single-key `runFlow` command and extract its shape. +// Returns null for non-runFlow OR malformed runFlow (which validateCommand then +// rejects), so expandRunFlows passes those through untouched. +function asRunFlow(cmd) { + if (!cmd || typeof cmd !== 'object' || Array.isArray(cmd)) + return null; + const keys = Object.keys(cmd); + if (keys.length !== 1 || keys[0] !== 'runFlow') + return null; + const v = cmd.runFlow; + if (typeof v === 'string') + return { file: v }; + if (v && typeof v === 'object' && !Array.isArray(v)) { + const o = v; + return { + file: typeof o.file === 'string' ? o.file : undefined, + when: o.when, + commands: Array.isArray(o.commands) ? o.commands : undefined, + }; + } + return null; +} +// GH #186: resolve a runFlow file ref to a canonical path, enforcing: relative +// only, no `..`, .yaml/.yml only, and containment within flowRoot after realpath +// (defeats symlink escape). Throws on any violation or missing root context. +function resolveRunFlowTarget(file, opts) { + if (!opts.flowDir || !opts.flowRoot) { + throw new MaestroValidationError(`runFlow file ref "${file}" requires a flow root context (flowDir + flowRoot)`); + } + if (isAbsolute(file)) { + throw new MaestroValidationError(`runFlow file ref must be relative, got absolute: ${file}`); + } + if (file.split(/[\\/]/).includes('..')) { + throw new MaestroValidationError(`runFlow file ref must not contain '..': ${file}`); + } + if (!/\.ya?ml$/i.test(file)) { + throw new MaestroValidationError(`runFlow file ref must be a .yaml/.yml file: ${file}`); + } + const realpath = opts.realpathFn ?? realpathSync; + let resolved; + let rootReal; + try { + resolved = realpath(join(opts.flowDir, file)); + rootReal = realpath(opts.flowRoot); + } + catch (err) { + throw new MaestroValidationError(`runFlow file ref "${file}" could not be resolved: ${err.message}`); + } + if (resolved !== rootReal && !resolved.startsWith(rootReal + sep)) { + throw new MaestroValidationError(`runFlow file ref "${file}" escapes the flow root`); + } + return resolved; +} +// GH #186: expand runFlow file refs inline so the serialized flow (written to +// /tmp) has no remaining file references. A `{file}` with a `when` becomes an +// inline conditional `{when, commands}` (semantics preserved); without `when` +// the sub-flow's commands are spliced flat. Inline runFlow is recursed into. +export function expandRunFlows(commands, opts) { + const out = []; + for (const cmd of commands) { + const rf = asRunFlow(cmd); + if (!rf) { + out.push(cmd); + continue; + } + if (rf.file !== undefined) { + const depth = opts._depth ?? 0; + const max = opts.maxRunFlowDepth ?? 5; + if (depth >= max) { + throw new MaestroValidationError(`runFlow nesting exceeded max depth ${max}`); + } + const resolved = resolveRunFlowTarget(rf.file, opts); + const visited = opts._visited ?? new Set(); + if (visited.has(resolved)) { + throw new MaestroValidationError(`runFlow cycle detected at "${rf.file}"`); + } + const readFile = opts.readFileFn ?? ((p) => readFileSync(p, 'utf8')); + let subText; + try { + subText = readFile(resolved); + } + catch (err) { + throw new MaestroValidationError(`runFlow file "${rf.file}" could not be read: ${err.message}`); + } + const sub = parseAndValidateFlow(subText, { + ...opts, + rejectHeader: true, + flowDir: dirname(resolved), + _depth: depth + 1, + _visited: new Set([...visited, resolved]), + }); + if (rf.when !== undefined) { + out.push({ runFlow: { when: rf.when, commands: sub.commands } }); + } + else { + out.push(...sub.commands); + } + } + else { + // Inline runFlow (no file) — recurse into nested commands, keep the wrapper. + const inner = rf.commands + ? expandRunFlows(rf.commands, { ...opts, _depth: (opts._depth ?? 0) + 1 }) + : []; + const wrapped = { commands: inner }; + if (rf.when !== undefined) + wrapped.when = rf.when; + out.push({ runFlow: wrapped }); + } + } + return out; +} export function parseAndValidateFlow(yamlText, opts = {}) { let docs; try { @@ -216,9 +379,12 @@ export function parseAndValidateFlow(yamlText, opts = {}) { if (!Array.isArray(body)) { throw new MaestroValidationError(`Flow body must be an array, got ${typeof body}`); } - for (const cmd of body) { + // GH #186: resolve + inline runFlow file refs FIRST, then validate the + // expanded (file-ref-free) body so what we serialize is self-contained. + const expanded = expandRunFlows(body, opts); + for (const cmd of expanded) { validateCommand(cmd); } - const raw = buildMaestroFlow(appId !== undefined ? { appId } : {}, body); - return { appId, commands: body, raw }; + const raw = buildMaestroFlow(appId !== undefined ? { appId } : {}, expanded); + return { appId, commands: expanded, raw }; } diff --git a/scripts/cdp-bridge/dist/domain/reusable-action.js b/scripts/cdp-bridge/dist/domain/reusable-action.js index e14c4f6c..abadfe38 100644 --- a/scripts/cdp-bridge/dist/domain/reusable-action.js +++ b/scripts/cdp-bridge/dist/domain/reusable-action.js @@ -164,6 +164,9 @@ export function parseM7Header(yamlText, fallbackId) { else if (key === 'produces') { meta.produces = parseProducesMap(raw); } + else if (key === 'expectedRouteSequence') { + meta.expectedRouteSequence = raw.replace(/^\[|\]$/g, '').split(',').map((t) => t.trim()).filter(Boolean); + } else if (key === 'id' || key === 'intent' || key === 'status' || key === 'appId' || key === 'createdAt' || key === 'author') { meta[key] = raw; } @@ -193,6 +196,7 @@ export function parseM7Header(yamlText, fallbackId) { createdAt: meta.createdAt, author: meta.author, produces: meta.produces, + expectedRouteSequence: meta.expectedRouteSequence, }; } /** @@ -262,5 +266,8 @@ export function serializeM7Header(metadata) { }); lines.push(`# produces: { ${pairs.join(', ')} }`); } + if (metadata.expectedRouteSequence && metadata.expectedRouteSequence.length) { + lines.push(`# expectedRouteSequence: [${metadata.expectedRouteSequence.map(stripNewlines).join(', ')}]`); + } return lines.join('\n'); } diff --git a/scripts/cdp-bridge/dist/nav-graph/route-sequence.js b/scripts/cdp-bridge/dist/nav-graph/route-sequence.js new file mode 100644 index 00000000..3d3df571 --- /dev/null +++ b/scripts/cdp-bridge/dist/nav-graph/route-sequence.js @@ -0,0 +1,32 @@ +export function validateRouteSequenceAgainstGraph(graph, expected) { + if (!expected || expected.length === 0) + return { ok: true }; + const known = new Set(graph?.all_screens ?? []); + // Empty/unknown graph → can't make a confident judgement; don't false-positive. + if (known.size === 0) + return { ok: true }; + const missing = expected.filter((s) => !known.has(s)); + if (missing.length > 0) { + return { + ok: false, + reason: `action expects screen(s) the nav graph no longer has: ${missing.join(', ')}`, + missing, + }; + } + return { ok: true }; +} +export function classifyRouteDriftAfterFailure(input) { + const { expectedSequence, liveRoute } = input; + if (!expectedSequence || expectedSequence.length === 0) + return { isDrift: false, liveRoute }; + if (!liveRoute) + return { isDrift: false, liveRoute }; + if (!expectedSequence.includes(liveRoute)) { + return { + isDrift: true, + liveRoute, + reason: `live route "${liveRoute}" is not in the action's expected sequence [${expectedSequence.join(' → ')}] — an unexpected screen appeared (structural drift, not a stale selector)`, + }; + } + return { isDrift: false, liveRoute }; +} diff --git a/scripts/cdp-bridge/dist/tools/device-session.js b/scripts/cdp-bridge/dist/tools/device-session.js index c680badd..ce527ddc 100644 --- a/scripts/cdp-bridge/dist/tools/device-session.js +++ b/scripts/cdp-bridge/dist/tools/device-session.js @@ -2,6 +2,7 @@ import { execFile as execFileCb } from 'node:child_process'; import { promisify } from 'node:util'; import { runAgentDevice, setActiveSession, clearActiveSession, getActiveSession, ensureFastRunner, cacheSnapshot, getAdbSerial, } from '../agent-device-wrapper.js'; import { stopFastRunner } from '../runners/rn-fast-runner-client.js'; +import { markCdpStale } from '../cdp/recovery.js'; import { detectLegacyAgentDevice, detectAndroidExternalRunner, } from '../runners/external-runner-detect.js'; import { okResult, failResult, warnResult } from '../utils.js'; import { resolveBundleId } from '../project-config.js'; @@ -200,9 +201,22 @@ export function createDeviceSnapshotHandler() { openSession: ({ appId, platform, attachOnly }) => reopenSessionForRecovery(appId, platform, attachOnly), resnapshot: () => rawSnapshot(), parseNodes: parseSnapshotNodes, + // GH #186: non-destructive reacquire tried before the destructive + // close/relaunch tiers. Only when we have the full iOS context + // (appId + deviceId) needed to re-foreground the app and restart the + // fast-runner; otherwise omitted so recovery falls back to the + // existing tiers. + reacquire: (session?.platform === 'ios' && session?.appId && session?.deviceId) + ? () => reacquireIosTargetApp(session.appId, session.deviceId) + : undefined, }); if (recovery.recovered) { cacheSnapshotIfPossible(recovery.result); + // GH #186: the recovery re-foregrounded/relaunched the app, which can + // leave the CDP target pinned to a now-stale context. Flag it so the + // next cdp_* call re-pins proactively (fast) instead of hitting the + // ~47s STALE_TARGET timeout that prompted this issue. + markCdpStale(); return wrapWithMeta(recovery.result, { recovered: 'agent-device-runner-leak', recoveryTier: recovery.tier, @@ -230,6 +244,33 @@ export function runnerLeakFailureHint(reason, session) { } return 'Manually close + reopen the session with action=open appId= platform=ios (full launch, not attachOnly). Upstream: Callstack/agent-device, see B119/GH#35.'; } +/** + * GH #186: non-destructive reacquire of the iOS target app after a runner-leak + * sentinel. Both the daemon-leak and a maestro-eviction (a foreign XCUITest + * session stealing focus) surface as the same sentinel, so rather than closing + * the session + relaunching (~44s, drops JS/CDP state) we: stop the + * (possibly evicted) fast-runner so it can't compete for focus, re-foreground + * the TARGET app via simctl (displacing the foreign session), then restart the + * fast-runner bound to the app. The caller (recoverFromRunnerLeak) re-snapshots + * and only falls through to the destructive tiers if the sentinel persists. + * Mirrors repair-action.ts:bringTargetAppToForeground, kept local here to keep + * the dependency surface tight (same rationale as that copy). + */ +async function reacquireIosTargetApp(appId, deviceId) { + try { + stopFastRunner(); + } + catch { /* best-effort — may already be dead */ } + try { + await execFile('xcrun', ['simctl', 'launch', 'booted', appId], { timeout: 5000, encoding: 'utf8' }); + } + catch { /* best-effort — the sentinel re-check covers a failed foreground */ } + try { + await ensureFastRunner(deviceId, appId); + } + catch { /* non-fatal — re-snapshot will surface a still-broken runner */ } + return okResult({ reacquired: true, appId }); +} async function rawSnapshot() { return runAgentDevice(['snapshot', '-i']); } diff --git a/scripts/cdp-bridge/dist/tools/maestro-run.js b/scripts/cdp-bridge/dist/tools/maestro-run.js index 941ee418..bd1f1296 100644 --- a/scripts/cdp-bridge/dist/tools/maestro-run.js +++ b/scripts/cdp-bridge/dist/tools/maestro-run.js @@ -2,7 +2,7 @@ import { execFile as execFileCb } from 'node:child_process'; import { promisify } from 'node:util'; import { existsSync, readFileSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join, dirname } from 'node:path'; import { okResult, failResult, warnResult } from '../utils.js'; import { getActiveSession } from '../agent-device-wrapper.js'; import { resolveBundleId, readExpoSlug } from '../project-config.js'; @@ -81,7 +81,13 @@ export function createMaestroRunHandler() { return failResult('Provide either flowPath or inlineYaml.'); } try { - const parsed = parseAndValidateFlow(rawYaml); + // GH #186: when running a saved flow FILE, resolve+inline any runFlow file + // refs relative to that file's directory, contained within it. Inline YAML + // has no on-disk root, so runFlow file refs stay rejected there. + const runFlowOpts = args.flowPath + ? { flowDir: dirname(args.flowPath), flowRoot: dirname(args.flowPath) } + : {}; + const parsed = parseAndValidateFlow(rawYaml, runFlowOpts); const rawAppId = resolveAppId(args.appId, platform); const headerAppId = parsed.appId ?? (rawAppId && isValidBundleId(rawAppId) ? rawAppId : undefined); if (rawAppId && !parsed.appId && !isValidBundleId(rawAppId)) { diff --git a/scripts/cdp-bridge/dist/tools/run-action.js b/scripts/cdp-bridge/dist/tools/run-action.js index f62e2bca..4420a76a 100644 --- a/scripts/cdp-bridge/dist/tools/run-action.js +++ b/scripts/cdp-bridge/dist/tools/run-action.js @@ -34,6 +34,7 @@ import { parseMaestroFailure, isAutoRepairable, } from '../domain/maestro-error- import { createMaestroRunHandler } from './maestro-run.js'; import { createRepairActionHandler } from './repair-action.js'; import { isValidActionId } from '../domain/path-safety.js'; +import { classifyRouteDriftAfterFailure } from '../nav-graph/route-sequence.js'; /** * Map a parsed Maestro failure kind to an `ActionFailureCode` (for * RunRecord telemetry) and a `ToolErrorCode` (for the failResult @@ -110,6 +111,7 @@ function mapRefusedReason(repairCode, repairError) { export function createRunActionHandler(deps = {}) { const maestroRun = deps.maestroRun ?? createMaestroRunHandler(); const repairAction = deps.repairAction ?? createRepairActionHandler(); + const getLiveRoute = deps.getLiveRoute ?? (async () => null); return async (args) => { if (!args.actionId || typeof args.actionId !== 'string') { return failResult('cdp_run_action requires actionId', 'BAD_FILENAME'); @@ -182,6 +184,35 @@ export function createRunActionHandler(deps = {}) { } // ─── First attempt failed — classify ───────────────────────────── const failure = parseMaestroFailure(firstOutput); + // GH #186: structural route-drift takes precedence over selector repair. + // If the action recorded an expected route sequence and the LIVE route is + // off it, an unexpected screen appeared (e.g. an inserted CouponCode) — a + // fuzzy selector repair would be wrong, so reclassify as ROUTE_DRIFT and + // skip repair. Live route is fetched within a bounded budget (best-effort; + // the default fetcher is a no-op until index.ts wires a CDP-backed one). + const expectedSeq = action.metadata.expectedRouteSequence; + if (failure.kind === 'SELECTOR_NOT_FOUND' && expectedSeq && expectedSeq.length > 0) { + const liveRoute = await getLiveRoute().catch(() => null); + const drift = classifyRouteDriftAfterFailure({ expectedSequence: expectedSeq, liveRoute }); + if (drift.isDrift) { + const autoRepair = { + attempted: false, + outcome: 'refused', + refusedReason: 'ROUTE_DRIFT', + phases: { firstAttemptMs }, + }; + await persistRun(args.actionId, projectRoot, { + timestamp: new Date().toISOString(), + durationMs: Date.now() - t0, + status: 'fail', + failureCode: 'ROUTE_DRIFT', + failureDetail: drift.reason ?? 'route drift', + trigger, + autoRepair, + }); + return failResult(`cdp_run_action: ${args.actionId} hit structural route-drift — ${drift.reason}. The flow changed shape; re-record the action. Auto-repair skipped (it only fixes stale selectors, not inserted/changed screens).`, 'ROUTE_DRIFT', { actionId: args.actionId, failureKind: 'ROUTE_DRIFT', liveRoute: drift.liveRoute, expectedRouteSequence: expectedSeq, autoRepair }); + } + } // Skip repair if disabled or if the failure isn't a repair shape. if (!autoRepairEnabled || !isAutoRepairable(failure)) { // PR #115 review (both providers conf 88): distinguish opt-out diff --git a/scripts/cdp-bridge/dist/tools/runner-leak-recovery.js b/scripts/cdp-bridge/dist/tools/runner-leak-recovery.js index cacc5c6e..3ed9bc2c 100644 --- a/scripts/cdp-bridge/dist/tools/runner-leak-recovery.js +++ b/scripts/cdp-bridge/dist/tools/runner-leak-recovery.js @@ -64,6 +64,16 @@ export async function recoverFromRunnerLeak(ctx, deps) { return { recovered: false, result: emptyResult(), reason: 'no-session-context' }; } const sleep = deps.sleep ?? defaultSleep; + // Tier 0 (GH #186): non-destructive reacquire — re-foreground the target app + // via the fast-runner WITHOUT closing the session or relaunching. Most + // state-preserving and fastest; strictly additive (falls through to the + // existing tiers if it doesn't clear the sentinel). + if (deps.reacquire) { + const tier0 = await attemptReacquireCycle(deps, sleep); + if (tier0.phase === 'success') { + return { recovered: true, result: tier0.result, tier: 'reacquire' }; + } + } // Tier 1: attachOnly reopen — preserves app state when it works. const tier1 = await attemptRecoveryCycle(ctx, deps, true, sleep); if (tier1.phase === 'success') { @@ -79,6 +89,21 @@ export async function recoverFromRunnerLeak(ctx, deps) { } return { recovered: false, result: tier2.result, reason: 'reopen-failed' }; } +async function attemptReacquireCycle(deps, sleep) { + const reacqResult = await deps.reacquire(); + if (reacqResult.isError) { + return { phase: 'reopen-failed', result: reacqResult }; + } + await sleep(DAEMON_SETTLE_MS); + const retryResult = await deps.resnapshot(); + if (retryResult.isError) { + return { phase: 'snapshot-failed', result: retryResult }; + } + if (isAgentDeviceRunnerSentinel(deps.parseNodes(retryResult))) { + return { phase: 'sentinel', result: retryResult }; + } + return { phase: 'success', result: retryResult }; +} async function attemptRecoveryCycle(ctx, deps, attachOnly, sleep) { await deps.closeSession(); await sleep(DAEMON_SETTLE_MS); diff --git a/scripts/cdp-bridge/dist/utils.js b/scripts/cdp-bridge/dist/utils.js index 3c1d0254..301dc96b 100644 --- a/scripts/cdp-bridge/dist/utils.js +++ b/scripts/cdp-bridge/dist/utils.js @@ -1,6 +1,6 @@ import { hasActiveSession } from './agent-device-wrapper.js'; import { handleDevClientPicker } from './tools/dev-client-picker.js'; -import { probeFreshness, recoverFromStaleTarget } from './cdp/recovery.js'; +import { probeFreshness, recoverFromStaleTarget, consumeCdpStale } from './cdp/recovery.js'; // S1 (D631): cache the freshness probe for up to 2s per (client, generation). // Eliminates a CDP round-trip on back-to-back tool calls while still invalidating // on reconnect (connectionGeneration bumps) and on any failure (cache is never set). @@ -143,6 +143,19 @@ export function withConnection(getClient, handler, options = {}) { } } } + // GH #186: a device-session runner-leak recovery may have re-foregrounded + // or relaunched the app out from under CDP. If it flagged the target as + // stale, re-pin proactively NOW (recoverFromStaleTarget is a no-op when + // the target is actually fresh) so the handler doesn't hit a ~47s + // STALE_TARGET timeout. Best-effort — the catch-path recovery still covers + // any failure here. + if (client.isConnected && consumeCdpStale()) { + try { + await recoverFromStaleTarget(client); + forgetFreshness(client); + } + catch { /* fall through — handler + catch-path recovery still apply */ } + } // D502: Proactive freshness check (D631/S1 caches result for 2s per generation). // D633: delegated to cdp/recovery.probeFreshness. if (requireHelpers && client.helpersInjected && !isFreshnessCached(client)) { diff --git a/scripts/cdp-bridge/src/cdp/recovery.ts b/scripts/cdp-bridge/src/cdp/recovery.ts index bc3fcb08..297f4936 100644 --- a/scripts/cdp-bridge/src/cdp/recovery.ts +++ b/scripts/cdp-bridge/src/cdp/recovery.ts @@ -5,6 +5,22 @@ const FRESHNESS_PROBE_MS = 2000; const STALE_RETRY_DELAY_MS = 500; const STALE_RETRY_PROBE_MS = 3000; +// GH #186: cross-tool "the CDP target may be stale" signal. A device-session +// runner-leak recovery can re-foreground/relaunch the app out from under CDP, +// after which the next cdp_* call would hit a ~47s STALE_TARGET timeout in the +// handler. The recovery sets this flag; withConnection consumes it before the +// next handler and proactively re-pins via recoverFromStaleTarget, turning that +// 47s catch-path recovery into a fast pre-handler one. Process-scoped boolean — +// a single MCP serves one device at a time, so no per-client keying is needed. +let cdpStale = false; +export function markCdpStale(): void { cdpStale = true; } +/** Read-and-clear: returns whether the stale flag was set, resetting it. */ +export function consumeCdpStale(): boolean { + const was = cdpStale; + cdpStale = false; + return was; +} + export interface FreshnessResult { fresh: boolean; version: number | null; diff --git a/scripts/cdp-bridge/src/domain/maestro-validator.ts b/scripts/cdp-bridge/src/domain/maestro-validator.ts index 4092f944..e08e0a27 100644 --- a/scripts/cdp-bridge/src/domain/maestro-validator.ts +++ b/scripts/cdp-bridge/src/domain/maestro-validator.ts @@ -16,6 +16,8 @@ * default-deny rationale. */ import yaml from 'yaml'; +import { join, dirname, isAbsolute, sep } from 'node:path'; +import { readFileSync, realpathSync } from 'node:fs'; // ── Errors ────────────────────────────────────────────────────────── @@ -111,6 +113,12 @@ const ALLOWED_COMMANDS = new Set([ 'killApp', 'stopApp', 'tap', + // GH #186: runFlow (conditional dialog handling — deep-link "Open in", Expo + // dev-client picker). Validated specially (validateRunFlowValue) so nested + // `commands` get full command-level allowlist checks, and {file} refs are + // securely resolved + expanded inline (expandRunFlows) — they are NOT passed + // through generic validateValue, which would miss nested denied commands. + 'runFlow', ]); const DENIED_COMMANDS = new Set([ @@ -172,9 +180,51 @@ function validateCommand(cmd: unknown): void { if (!ALLOWED_COMMANDS.has(key)) { throw new MaestroValidationError(`Command not in allowlist: ${key}`); } + if (key === 'runFlow') { + validateRunFlowValue((cmd as Record)[key]); + return; + } validateValue((cmd as Record)[key]); } +/** + * GH #186: validate a runFlow value. The string/`{file}` form is a sub-flow + * file ref (a safe scalar here; secure resolution happens in expandRunFlows). + * Inline `commands` are validated as COMMANDS (recursive allowlist check) — the + * critical difference from generic validateValue, which would let a nested + * `runScript` slip through as a plain scalar key/value. + */ +function validateRunFlowValue(v: unknown): void { + if (typeof v === 'string') { + if (!isSafeMaestroScalar(v)) { + throw new MaestroValidationError(`Unsafe runFlow file ref: ${JSON.stringify(v).slice(0, 80)}`); + } + return; + } + if (v === null || typeof v !== 'object' || Array.isArray(v)) { + throw new MaestroValidationError(`runFlow value must be a file string or an object, got ${Array.isArray(v) ? 'array' : typeof v}`); + } + const obj = v as Record; + if ('file' in obj && (typeof obj.file !== 'string' || !isSafeMaestroScalar(obj.file))) { + throw new MaestroValidationError(`runFlow.file must be a safe scalar string`); + } + if ('when' in obj) validateValue(obj.when); + if ('commands' in obj) { + if (!Array.isArray(obj.commands)) { + throw new MaestroValidationError(`runFlow.commands must be an array`); + } + for (const c of obj.commands) validateCommand(c); + } + // Any other keys (env/label/config) are validated as generic safe values. + for (const [k, val] of Object.entries(obj)) { + if (k === 'file' || k === 'when' || k === 'commands') continue; + if (!isSafeMaestroScalar(k)) { + throw new MaestroValidationError(`Unsafe runFlow key: ${JSON.stringify(k).slice(0, 80)}`); + } + validateValue(val); + } +} + function validateValue(v: unknown): void { if (v === null || v === undefined) return; if (typeof v === 'boolean' || typeof v === 'number') return; @@ -212,6 +262,128 @@ export interface ParsedFlow { export interface ParseAndValidateOptions { /** Reject flows that include an `appId` header. Default `false`. */ rejectHeader?: boolean; + // GH #186: runFlow {file} support. When flowDir + flowRoot are provided, + // file refs are resolved relative to flowDir, required to canonicalize within + // flowRoot (no `..`/absolute/symlink escape), recursively parsed+validated, + // and EXPANDED INLINE so the serialized flow has no remaining file refs (the + // flow is later written to /tmp, where a relative ref would break). A {file} + // ref with no flowRoot context is rejected. + flowDir?: string; + flowRoot?: string; + /** Max runFlow nesting depth. Default 5. */ + maxRunFlowDepth?: number; + /** Test seam — defaults to fs.readFileSync(utf8). */ + readFileFn?: (path: string) => string; + /** Test seam — defaults to fs.realpathSync. */ + realpathFn?: (path: string) => string; + /** Internal: current recursion depth. */ + _depth?: number; + /** Internal: canonical paths already on the resolution stack (cycle guard). */ + _visited?: Set; +} + +// GH #186: recognize a single-key `runFlow` command and extract its shape. +// Returns null for non-runFlow OR malformed runFlow (which validateCommand then +// rejects), so expandRunFlows passes those through untouched. +function asRunFlow(cmd: unknown): { file?: string; when?: unknown; commands?: unknown[] } | null { + if (!cmd || typeof cmd !== 'object' || Array.isArray(cmd)) return null; + const keys = Object.keys(cmd as Record); + if (keys.length !== 1 || keys[0] !== 'runFlow') return null; + const v = (cmd as Record).runFlow; + if (typeof v === 'string') return { file: v }; + if (v && typeof v === 'object' && !Array.isArray(v)) { + const o = v as Record; + return { + file: typeof o.file === 'string' ? o.file : undefined, + when: o.when, + commands: Array.isArray(o.commands) ? o.commands : undefined, + }; + } + return null; +} + +// GH #186: resolve a runFlow file ref to a canonical path, enforcing: relative +// only, no `..`, .yaml/.yml only, and containment within flowRoot after realpath +// (defeats symlink escape). Throws on any violation or missing root context. +function resolveRunFlowTarget(file: string, opts: ParseAndValidateOptions): string { + if (!opts.flowDir || !opts.flowRoot) { + throw new MaestroValidationError(`runFlow file ref "${file}" requires a flow root context (flowDir + flowRoot)`); + } + if (isAbsolute(file)) { + throw new MaestroValidationError(`runFlow file ref must be relative, got absolute: ${file}`); + } + if (file.split(/[\\/]/).includes('..')) { + throw new MaestroValidationError(`runFlow file ref must not contain '..': ${file}`); + } + if (!/\.ya?ml$/i.test(file)) { + throw new MaestroValidationError(`runFlow file ref must be a .yaml/.yml file: ${file}`); + } + const realpath = opts.realpathFn ?? realpathSync; + let resolved: string; + let rootReal: string; + try { + resolved = realpath(join(opts.flowDir, file)); + rootReal = realpath(opts.flowRoot); + } catch (err) { + throw new MaestroValidationError(`runFlow file ref "${file}" could not be resolved: ${(err as Error).message}`); + } + if (resolved !== rootReal && !resolved.startsWith(rootReal + sep)) { + throw new MaestroValidationError(`runFlow file ref "${file}" escapes the flow root`); + } + return resolved; +} + +// GH #186: expand runFlow file refs inline so the serialized flow (written to +// /tmp) has no remaining file references. A `{file}` with a `when` becomes an +// inline conditional `{when, commands}` (semantics preserved); without `when` +// the sub-flow's commands are spliced flat. Inline runFlow is recursed into. +export function expandRunFlows(commands: unknown[], opts: ParseAndValidateOptions): unknown[] { + const out: unknown[] = []; + for (const cmd of commands) { + const rf = asRunFlow(cmd); + if (!rf) { out.push(cmd); continue; } + + if (rf.file !== undefined) { + const depth = opts._depth ?? 0; + const max = opts.maxRunFlowDepth ?? 5; + if (depth >= max) { + throw new MaestroValidationError(`runFlow nesting exceeded max depth ${max}`); + } + const resolved = resolveRunFlowTarget(rf.file, opts); + const visited = opts._visited ?? new Set(); + if (visited.has(resolved)) { + throw new MaestroValidationError(`runFlow cycle detected at "${rf.file}"`); + } + const readFile = opts.readFileFn ?? ((p: string) => readFileSync(p, 'utf8')); + let subText: string; + try { + subText = readFile(resolved); + } catch (err) { + throw new MaestroValidationError(`runFlow file "${rf.file}" could not be read: ${(err as Error).message}`); + } + const sub = parseAndValidateFlow(subText, { + ...opts, + rejectHeader: true, + flowDir: dirname(resolved), + _depth: depth + 1, + _visited: new Set([...visited, resolved]), + }); + if (rf.when !== undefined) { + out.push({ runFlow: { when: rf.when, commands: sub.commands } }); + } else { + out.push(...sub.commands); + } + } else { + // Inline runFlow (no file) — recurse into nested commands, keep the wrapper. + const inner = rf.commands + ? expandRunFlows(rf.commands, { ...opts, _depth: (opts._depth ?? 0) + 1 }) + : []; + const wrapped: Record = { commands: inner }; + if (rf.when !== undefined) wrapped.when = rf.when; + out.push({ runFlow: wrapped }); + } + } + return out; } export function parseAndValidateFlow(yamlText: string, opts: ParseAndValidateOptions = {}): ParsedFlow { @@ -248,10 +420,13 @@ export function parseAndValidateFlow(yamlText: string, opts: ParseAndValidateOpt if (!Array.isArray(body)) { throw new MaestroValidationError(`Flow body must be an array, got ${typeof body}`); } - for (const cmd of body) { + // GH #186: resolve + inline runFlow file refs FIRST, then validate the + // expanded (file-ref-free) body so what we serialize is self-contained. + const expanded = expandRunFlows(body, opts); + for (const cmd of expanded) { validateCommand(cmd); } - const raw = buildMaestroFlow(appId !== undefined ? { appId } : {}, body); - return { appId, commands: body, raw }; + const raw = buildMaestroFlow(appId !== undefined ? { appId } : {}, expanded); + return { appId, commands: expanded, raw }; } diff --git a/scripts/cdp-bridge/src/domain/reusable-action.ts b/scripts/cdp-bridge/src/domain/reusable-action.ts index bdfbc16f..a15be6f8 100644 --- a/scripts/cdp-bridge/src/domain/reusable-action.ts +++ b/scripts/cdp-bridge/src/domain/reusable-action.ts @@ -38,6 +38,10 @@ export type ActionLifecycle = 'experimental' | 'active' | 'deprecated'; */ export type ActionFailureCode = | 'SELECTOR_NOT_FOUND' + // GH #186: the live route diverged from the action's expectedRouteSequence + // (a screen was inserted/changed) — structural drift, distinct from selector + // churn, so it must NOT trigger fuzzy selector repair. + | 'ROUTE_DRIFT' | 'STATE_MISMATCH' | 'MUTATE_PRECONDITION_FAILED' | 'ENV_UNREACHABLE' @@ -130,6 +134,17 @@ export interface M7Metadata { * values. */ produces?: Record; + + /** + * GH #186 — ordered list of screen/route names this action walked when it was + * recorded (captured from nav events at save_as_action time). run-action uses + * it for structural drift detection: a pre-flight check against the live nav + * graph (definite-mismatch fail-fast) and a post-failure check that + * reclassifies a SELECTOR_NOT_FOUND as ROUTE_DRIFT when the live route is off + * this sequence (a screen was inserted). Optional — actions without it skip + * the drift checks. + */ + expectedRouteSequence?: string[]; } // ───────────────────────────────────────────────────────────────────────────── @@ -143,6 +158,9 @@ export type AutoRepairRefusedReason = | 'NO_MATCH' | 'SNAPSHOT_FAILED' | 'NOT_REPAIRABLE_KIND' + // GH #186: refused because the failure was structural route-drift, not a + // stale selector — fuzzy repair would be wrong here. + | 'ROUTE_DRIFT' // PR #115 multi-LLM review: distinguish user-driven opt-outs from // genuine refusals so MTTR analysis (#105) can see "user disabled // repair" as operationally healthy vs. budget/edit/match refusals. @@ -458,6 +476,8 @@ export function parseM7Header(yamlText: string, fallbackId?: string): M7Metadata meta.params = raw.replace(/^\[|\]$/g, '').split(',').map((t) => t.trim()).filter(Boolean); } else if (key === 'produces') { meta.produces = parseProducesMap(raw); + } else if (key === 'expectedRouteSequence') { + meta.expectedRouteSequence = raw.replace(/^\[|\]$/g, '').split(',').map((t) => t.trim()).filter(Boolean); } else if (key === 'id' || key === 'intent' || key === 'status' || key === 'appId' || key === 'createdAt' || key === 'author') { meta[key] = raw; } @@ -483,6 +503,7 @@ export function parseM7Header(yamlText: string, fallbackId?: string): M7Metadata createdAt: meta.createdAt as string | undefined, author: meta.author as ActionAuthor | undefined, produces: meta.produces as Record | undefined, + expectedRouteSequence: meta.expectedRouteSequence as string[] | undefined, }; } @@ -547,5 +568,8 @@ export function serializeM7Header(metadata: M7Metadata): string { }); lines.push(`# produces: { ${pairs.join(', ')} }`); } + if (metadata.expectedRouteSequence && metadata.expectedRouteSequence.length) { + lines.push(`# expectedRouteSequence: [${metadata.expectedRouteSequence.map(stripNewlines).join(', ')}]`); + } return lines.join('\n'); } diff --git a/scripts/cdp-bridge/src/nav-graph/route-sequence.ts b/scripts/cdp-bridge/src/nav-graph/route-sequence.ts new file mode 100644 index 00000000..1106bc48 --- /dev/null +++ b/scripts/cdp-bridge/src/nav-graph/route-sequence.ts @@ -0,0 +1,66 @@ +import type { NavGraph } from './types.js'; + +/** + * GH #186: structural route-drift detection. A saved action records the route + * sequence it walked (M7 `expectedRouteSequence`). Two pure checks compare that + * against reality: + * + * - validateRouteSequenceAgainstGraph (PRE-flight): cheap, conservative — + * fails ONLY on a definite mismatch (an expected screen the nav graph no + * longer knows). It deliberately no-ops on an empty/unknown graph so an + * incomplete graph can't false-positive a healthy replay (per the review). + * + * - classifyRouteDriftAfterFailure (POST-failure): stronger — when Maestro + * reports SELECTOR_NOT_FOUND, the live route being OFF the expected sequence + * means a screen was inserted/changed (the report's CouponCode case), which + * is structural drift, not selector churn — so callers should reclassify it + * as ROUTE_DRIFT and skip the fuzzy selector repair. + */ + +export interface RouteSequenceCheck { + ok: boolean; + reason?: string; + missing?: string[]; +} + +export function validateRouteSequenceAgainstGraph( + graph: NavGraph | null | undefined, + expected: string[] | undefined, +): RouteSequenceCheck { + if (!expected || expected.length === 0) return { ok: true }; + const known = new Set(graph?.all_screens ?? []); + // Empty/unknown graph → can't make a confident judgement; don't false-positive. + if (known.size === 0) return { ok: true }; + const missing = expected.filter((s) => !known.has(s)); + if (missing.length > 0) { + return { + ok: false, + reason: `action expects screen(s) the nav graph no longer has: ${missing.join(', ')}`, + missing, + }; + } + return { ok: true }; +} + +export interface RouteDriftVerdict { + isDrift: boolean; + liveRoute: string | null; + reason?: string; +} + +export function classifyRouteDriftAfterFailure(input: { + expectedSequence: string[] | undefined; + liveRoute: string | null; +}): RouteDriftVerdict { + const { expectedSequence, liveRoute } = input; + if (!expectedSequence || expectedSequence.length === 0) return { isDrift: false, liveRoute }; + if (!liveRoute) return { isDrift: false, liveRoute }; + if (!expectedSequence.includes(liveRoute)) { + return { + isDrift: true, + liveRoute, + reason: `live route "${liveRoute}" is not in the action's expected sequence [${expectedSequence.join(' → ')}] — an unexpected screen appeared (structural drift, not a stale selector)`, + }; + } + return { isDrift: false, liveRoute }; +} diff --git a/scripts/cdp-bridge/src/tools/device-interact.ts b/scripts/cdp-bridge/src/tools/device-interact.ts index 81c0756f..df7b9f3b 100644 --- a/scripts/cdp-bridge/src/tools/device-interact.ts +++ b/scripts/cdp-bridge/src/tools/device-interact.ts @@ -7,6 +7,7 @@ import type { ToolResult } from '../utils.js'; import { okResult, failResult, createStepTimer } from '../utils.js'; import { runMaestroInline, yamlEscape } from '../maestro-invoke.js'; import { isAgentDeviceRunnerSentinel, recoverFromRunnerLeak } from './runner-leak-recovery.js'; +import type { RecoveryTier } from './runner-leak-recovery.js'; import { reopenSessionForRecovery } from './device-session.js'; import type { FlatNode } from '../fast-runner-ref-map.js'; @@ -123,7 +124,7 @@ function parseSnapshotEnvelope(result: ToolResult): SnapshotNode[] | null { } export type SnapshotFetchResult = - | { ok: true; nodes: SnapshotNode[]; recoveredTier?: 'attach-only' | 'full-relaunch' } + | { ok: true; nodes: SnapshotNode[]; recoveredTier?: RecoveryTier } | { ok: false; reason: 'fetch-failed' } | { ok: false; reason: 'runner-leak-unrecovered'; recoveryReason?: string }; @@ -163,7 +164,7 @@ async function fetchSnapshotNodes(): Promise { } export type FindCandidatesResult = - | { ok: true; candidates: FindCandidate[]; recoveredTier?: 'attach-only' | 'full-relaunch' } + | { ok: true; candidates: FindCandidate[]; recoveredTier?: RecoveryTier } | { ok: false; reason: 'fetch-failed' } | { ok: false; reason: 'runner-leak-unrecovered'; recoveryReason?: string }; @@ -210,7 +211,7 @@ async function pressCandidate(candidate: FindCandidate, action?: string): Promis // B119: when an underlying snapshot triggered runner-leak recovery, surface // that side-effect on the wrapping result so callers (LLM agents) know the // app may have been relaunched and CDP/state may have been invalidated. -function tagPressIfRecovered(result: ToolResult, tier?: 'attach-only' | 'full-relaunch'): ToolResult { +function tagPressIfRecovered(result: ToolResult, tier?: RecoveryTier): ToolResult { if (!tier || result.isError) return result; try { const envelope = JSON.parse(result.content[0].text) as { ok?: boolean; data?: unknown; meta?: Record }; diff --git a/scripts/cdp-bridge/src/tools/device-session.ts b/scripts/cdp-bridge/src/tools/device-session.ts index f7f890b6..6c5302be 100644 --- a/scripts/cdp-bridge/src/tools/device-session.ts +++ b/scripts/cdp-bridge/src/tools/device-session.ts @@ -10,6 +10,7 @@ import { getAdbSerial, } from '../agent-device-wrapper.js'; import { stopFastRunner } from '../runners/rn-fast-runner-client.js'; +import { markCdpStale } from '../cdp/recovery.js'; import { detectLegacyAgentDevice, detectAndroidExternalRunner, @@ -284,11 +285,24 @@ export function createDeviceSnapshotHandler(): (args: SnapshotArgs) => Promise rawSnapshot(), parseNodes: parseSnapshotNodes, + // GH #186: non-destructive reacquire tried before the destructive + // close/relaunch tiers. Only when we have the full iOS context + // (appId + deviceId) needed to re-foreground the app and restart the + // fast-runner; otherwise omitted so recovery falls back to the + // existing tiers. + reacquire: (session?.platform === 'ios' && session?.appId && session?.deviceId) + ? () => reacquireIosTargetApp(session.appId!, session.deviceId!) + : undefined, }, ); if (recovery.recovered) { cacheSnapshotIfPossible(recovery.result); + // GH #186: the recovery re-foregrounded/relaunched the app, which can + // leave the CDP target pinned to a now-stale context. Flag it so the + // next cdp_* call re-pins proactively (fast) instead of hitting the + // ~47s STALE_TARGET timeout that prompted this issue. + markCdpStale(); return wrapWithMeta(recovery.result, { recovered: 'agent-device-runner-leak', recoveryTier: recovery.tier, @@ -327,6 +341,29 @@ export function runnerLeakFailureHint( return 'Manually close + reopen the session with action=open appId= platform=ios (full launch, not attachOnly). Upstream: Callstack/agent-device, see B119/GH#35.'; } +/** + * GH #186: non-destructive reacquire of the iOS target app after a runner-leak + * sentinel. Both the daemon-leak and a maestro-eviction (a foreign XCUITest + * session stealing focus) surface as the same sentinel, so rather than closing + * the session + relaunching (~44s, drops JS/CDP state) we: stop the + * (possibly evicted) fast-runner so it can't compete for focus, re-foreground + * the TARGET app via simctl (displacing the foreign session), then restart the + * fast-runner bound to the app. The caller (recoverFromRunnerLeak) re-snapshots + * and only falls through to the destructive tiers if the sentinel persists. + * Mirrors repair-action.ts:bringTargetAppToForeground, kept local here to keep + * the dependency surface tight (same rationale as that copy). + */ +async function reacquireIosTargetApp(appId: string, deviceId: string): Promise { + try { stopFastRunner(); } catch { /* best-effort — may already be dead */ } + try { + await execFile('xcrun', ['simctl', 'launch', 'booted', appId], { timeout: 5000, encoding: 'utf8' }); + } catch { /* best-effort — the sentinel re-check covers a failed foreground */ } + try { + await ensureFastRunner(deviceId, appId); + } catch { /* non-fatal — re-snapshot will surface a still-broken runner */ } + return okResult({ reacquired: true, appId }); +} + async function rawSnapshot(): Promise { return runAgentDevice(['snapshot', '-i']); } diff --git a/scripts/cdp-bridge/src/tools/maestro-run.ts b/scripts/cdp-bridge/src/tools/maestro-run.ts index e7e9ab67..2ab0310f 100644 --- a/scripts/cdp-bridge/src/tools/maestro-run.ts +++ b/scripts/cdp-bridge/src/tools/maestro-run.ts @@ -2,7 +2,7 @@ import { execFile as execFileCb } from 'node:child_process'; import { promisify } from 'node:util'; import { existsSync, readFileSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join, dirname } from 'node:path'; import type { ToolResult } from '../utils.js'; import { okResult, failResult, warnResult } from '../utils.js'; import { getActiveSession } from '../agent-device-wrapper.js'; @@ -115,7 +115,13 @@ export function createMaestroRunHandler(): (args: MaestroRunArgs) => Promise; repairAction?: ReturnType; + /** + * GH #186: fetch the live deepest route name (bounded, best-effort) for + * structural route-drift detection on a SELECTOR_NOT_FOUND. Defaults to a + * no-op (null) so the drift check is inert until index.ts wires a real + * CDP-backed fetcher; tests inject a fake. + */ + getLiveRoute?: () => Promise; } export function createRunActionHandler(deps: RunActionDeps = {}) { const maestroRun = deps.maestroRun ?? createMaestroRunHandler(); const repairAction = deps.repairAction ?? createRepairActionHandler(); + const getLiveRoute = deps.getLiveRoute ?? (async () => null); return async (args: RunActionArgs): Promise => { if (!args.actionId || typeof args.actionId !== 'string') { return failResult('cdp_run_action requires actionId', 'BAD_FILENAME'); @@ -273,6 +282,40 @@ export function createRunActionHandler(deps: RunActionDeps = {}) { // ─── First attempt failed — classify ───────────────────────────── const failure = parseMaestroFailure(firstOutput); + // GH #186: structural route-drift takes precedence over selector repair. + // If the action recorded an expected route sequence and the LIVE route is + // off it, an unexpected screen appeared (e.g. an inserted CouponCode) — a + // fuzzy selector repair would be wrong, so reclassify as ROUTE_DRIFT and + // skip repair. Live route is fetched within a bounded budget (best-effort; + // the default fetcher is a no-op until index.ts wires a CDP-backed one). + const expectedSeq = action.metadata.expectedRouteSequence; + if (failure.kind === 'SELECTOR_NOT_FOUND' && expectedSeq && expectedSeq.length > 0) { + const liveRoute = await getLiveRoute().catch(() => null); + const drift = classifyRouteDriftAfterFailure({ expectedSequence: expectedSeq, liveRoute }); + if (drift.isDrift) { + const autoRepair: AutoRepairOutcome = { + attempted: false, + outcome: 'refused', + refusedReason: 'ROUTE_DRIFT', + phases: { firstAttemptMs }, + }; + await persistRun(args.actionId, projectRoot, { + timestamp: new Date().toISOString(), + durationMs: Date.now() - t0, + status: 'fail', + failureCode: 'ROUTE_DRIFT', + failureDetail: drift.reason ?? 'route drift', + trigger, + autoRepair, + }); + return failResult( + `cdp_run_action: ${args.actionId} hit structural route-drift — ${drift.reason}. The flow changed shape; re-record the action. Auto-repair skipped (it only fixes stale selectors, not inserted/changed screens).`, + 'ROUTE_DRIFT', + { actionId: args.actionId, failureKind: 'ROUTE_DRIFT', liveRoute: drift.liveRoute, expectedRouteSequence: expectedSeq, autoRepair }, + ); + } + } + // Skip repair if disabled or if the failure isn't a repair shape. if (!autoRepairEnabled || !isAutoRepairable(failure)) { // PR #115 review (both providers conf 88): distinguish opt-out diff --git a/scripts/cdp-bridge/src/tools/runner-leak-recovery.ts b/scripts/cdp-bridge/src/tools/runner-leak-recovery.ts index 37432c09..f9c278ed 100644 --- a/scripts/cdp-bridge/src/tools/runner-leak-recovery.ts +++ b/scripts/cdp-bridge/src/tools/runner-leak-recovery.ts @@ -63,13 +63,23 @@ export interface RecoveryDeps { resnapshot: () => Promise; parseNodes: (result: ToolResult) => RunnerLeakNode[] | null; sleep?: (ms: number) => Promise; + /** + * GH #186: non-destructive reacquire — re-foreground the TARGET app via the + * fast-runner (stopFastRunner → ensureFastRunner → activate) WITHOUT closing + * the session or relaunching the app. When supplied, it's tried first: both + * the daemon-leak and the maestro-eviction case surface as the same sentinel, + * so rather than distinguishing them we attempt a cheap state-preserving + * reacquire and only fall through to the destructive tiers if it doesn't + * clear the sentinel. Optional so non-iOS / older callers are unaffected. + */ + reacquire?: () => Promise; } const DAEMON_SETTLE_MS = 600; const defaultSleep = (ms: number): Promise => new Promise((r) => setTimeout(r, ms)); -export type RecoveryTier = 'attach-only' | 'full-relaunch'; +export type RecoveryTier = 'reacquire' | 'attach-only' | 'full-relaunch'; export interface RecoveryOutcome { recovered: boolean; @@ -114,6 +124,17 @@ export async function recoverFromRunnerLeak( const sleep = deps.sleep ?? defaultSleep; + // Tier 0 (GH #186): non-destructive reacquire — re-foreground the target app + // via the fast-runner WITHOUT closing the session or relaunching. Most + // state-preserving and fastest; strictly additive (falls through to the + // existing tiers if it doesn't clear the sentinel). + if (deps.reacquire) { + const tier0 = await attemptReacquireCycle(deps, sleep); + if (tier0.phase === 'success') { + return { recovered: true, result: tier0.result, tier: 'reacquire' }; + } + } + // Tier 1: attachOnly reopen — preserves app state when it works. const tier1 = await attemptRecoveryCycle(ctx, deps, true, sleep); if (tier1.phase === 'success') { @@ -137,6 +158,25 @@ interface RecoveryCycleResult { result: ToolResult; } +async function attemptReacquireCycle( + deps: RecoveryDeps, + sleep: (ms: number) => Promise, +): Promise { + const reacqResult = await deps.reacquire!(); + if (reacqResult.isError) { + return { phase: 'reopen-failed', result: reacqResult }; + } + await sleep(DAEMON_SETTLE_MS); + const retryResult = await deps.resnapshot(); + if (retryResult.isError) { + return { phase: 'snapshot-failed', result: retryResult }; + } + if (isAgentDeviceRunnerSentinel(deps.parseNodes(retryResult))) { + return { phase: 'sentinel', result: retryResult }; + } + return { phase: 'success', result: retryResult }; +} + async function attemptRecoveryCycle( ctx: RecoveryContext, deps: RecoveryDeps, diff --git a/scripts/cdp-bridge/src/types.ts b/scripts/cdp-bridge/src/types.ts index 4b6f309d..358273f8 100644 --- a/scripts/cdp-bridge/src/types.ts +++ b/scripts/cdp-bridge/src/types.ts @@ -206,7 +206,10 @@ export type ToolErrorCode = | 'CROSS_PLATFORM_MISMATCH' // GH #184: cdp_status aborted fast because the Dev Client picker was blocking // the bundle (React unreachable on a non-Hermes target within the budget). - | 'PICKER_BLOCKING'; + | 'PICKER_BLOCKING' + // GH #186: cdp_run_action replay hit structural route-drift (live route off + // the action's expectedRouteSequence) — distinct from a stale selector. + | 'ROUTE_DRIFT'; export interface ResultEnvelope { ok: boolean; diff --git a/scripts/cdp-bridge/src/utils.ts b/scripts/cdp-bridge/src/utils.ts index 9df16b6b..f91840d9 100644 --- a/scripts/cdp-bridge/src/utils.ts +++ b/scripts/cdp-bridge/src/utils.ts @@ -2,7 +2,7 @@ import type { CDPClient } from './cdp-client.js'; import type { ResultEnvelope, ToolErrorCode } from './types.js'; import { hasActiveSession } from './agent-device-wrapper.js'; import { handleDevClientPicker } from './tools/dev-client-picker.js'; -import { probeFreshness, recoverFromStaleTarget } from './cdp/recovery.js'; +import { probeFreshness, recoverFromStaleTarget, consumeCdpStale } from './cdp/recovery.js'; // S1 (D631): cache the freshness probe for up to 2s per (client, generation). // Eliminates a CDP round-trip on back-to-back tool calls while still invalidating @@ -165,6 +165,18 @@ export function withConnection( } } } + // GH #186: a device-session runner-leak recovery may have re-foregrounded + // or relaunched the app out from under CDP. If it flagged the target as + // stale, re-pin proactively NOW (recoverFromStaleTarget is a no-op when + // the target is actually fresh) so the handler doesn't hit a ~47s + // STALE_TARGET timeout. Best-effort — the catch-path recovery still covers + // any failure here. + if (client.isConnected && consumeCdpStale()) { + try { + await recoverFromStaleTarget(client); + forgetFreshness(client); + } catch { /* fall through — handler + catch-path recovery still apply */ } + } // D502: Proactive freshness check (D631/S1 caches result for 2s per generation). // D633: delegated to cdp/recovery.probeFreshness. if (requireHelpers && client.helpersInjected && !isFreshnessCached(client)) { diff --git a/scripts/cdp-bridge/test/unit/gh-186-reacquire-tier.test.js b/scripts/cdp-bridge/test/unit/gh-186-reacquire-tier.test.js new file mode 100644 index 00000000..dcf7d2b6 --- /dev/null +++ b/scripts/cdp-bridge/test/unit/gh-186-reacquire-tier.test.js @@ -0,0 +1,68 @@ +// GH #186 P0: runner-leak recovery should try a NON-DESTRUCTIVE reacquire +// (re-foreground the target app via the fast-runner) BEFORE the ~44s full +// relaunch. Both the daemon-leak and the maestro-eviction cases surface as the +// same AgentDeviceRunner sentinel, so we don't try to distinguish them — we add +// a strictly-additive cheap tier that falls back to today's behavior. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { recoverFromRunnerLeak } from '../../dist/tools/runner-leak-recovery.js'; +import { markCdpStale, consumeCdpStale } from '../../dist/cdp/recovery.js'; + +const SENTINEL = [{ label: 'AgentDeviceRunner' }]; // isAgentDeviceRunnerSentinel → true +const APP = [{ label: 'Home', identifier: 'home' }]; // → false + +const ok = () => ({ content: [{ type: 'text', text: JSON.stringify({ ok: true }) }] }); +const snap = (nodes) => ({ content: [{ type: 'text', text: JSON.stringify({ ok: true, data: { nodes } }) }], __nodes: nodes }); + +function makeDeps(snapshotQueue, { withReacquire = true } = {}) { + const calls = []; + let i = 0; + const deps = { + closeSession: async () => { calls.push('close'); return ok(); }, + openSession: async (a) => { calls.push(a.attachOnly ? 'open:attach' : 'open:relaunch'); return ok(); }, + resnapshot: async () => { calls.push('resnap'); return snap(snapshotQueue[i++] ?? SENTINEL); }, + parseNodes: (r) => r.__nodes ?? null, + sleep: async () => {}, + }; + if (withReacquire) deps.reacquire = async () => { calls.push('reacquire'); return ok(); }; + return { calls, deps }; +} + +test('reacquire tier clears the sentinel WITHOUT any relaunch/attach', async () => { + const { calls, deps } = makeDeps([APP]); // reacquire's resnapshot → real app tree + const out = await recoverFromRunnerLeak({ platform: 'ios', appId: 'com.x' }, deps); + assert.equal(out.recovered, true); + assert.equal(out.tier, 'reacquire'); + assert.ok(calls.includes('reacquire'), 'tried reacquire'); + assert.ok(!calls.some((c) => c.startsWith('open:')), 'no openSession — app state preserved, no 44s relaunch'); +}); + +test('falls through reacquire → attachOnly → full-relaunch when reacquire does not clear the sentinel', async () => { + // reacquire→SENTINEL, attachOnly→SENTINEL, full-relaunch→APP + const { calls, deps } = makeDeps([SENTINEL, SENTINEL, APP]); + const out = await recoverFromRunnerLeak({ platform: 'ios', appId: 'com.x' }, deps); + assert.equal(out.recovered, true); + assert.equal(out.tier, 'full-relaunch'); + assert.deepEqual( + calls.filter((c) => c === 'reacquire' || c.startsWith('open:')), + ['reacquire', 'open:attach', 'open:relaunch'], + 'tier order: reacquire first, then the existing attach + relaunch tiers', + ); +}); + +test('without a reacquire dep, behavior is unchanged (attach-only tier still works)', async () => { + const { calls, deps } = makeDeps([APP], { withReacquire: false }); + const out = await recoverFromRunnerLeak({ platform: 'ios', appId: 'com.x' }, deps); + assert.equal(out.recovered, true); + assert.equal(out.tier, 'attach-only'); + assert.ok(!calls.includes('reacquire')); +}); + +// CDP re-pin flag — read-and-clear so withConnection picks it up exactly once. +test('markCdpStale / consumeCdpStale is a read-and-clear flag', () => { + consumeCdpStale(); // normalize any prior state + assert.equal(consumeCdpStale(), false, 'starts clear'); + markCdpStale(); + assert.equal(consumeCdpStale(), true, 'set then consumed true'); + assert.equal(consumeCdpStale(), false, 'cleared after one consume'); +}); diff --git a/scripts/cdp-bridge/test/unit/gh-186-route-drift-runaction.test.js b/scripts/cdp-bridge/test/unit/gh-186-route-drift-runaction.test.js new file mode 100644 index 00000000..aeefca3b --- /dev/null +++ b/scripts/cdp-bridge/test/unit/gh-186-route-drift-runaction.test.js @@ -0,0 +1,67 @@ +// GH #186 P1: run-action reclassifies a SELECTOR_NOT_FOUND as ROUTE_DRIFT (and +// SKIPS the fuzzy selector repair) when the action recorded an expected route +// sequence and the live route is off it — a screen was inserted/changed. +import { test, beforeEach, afterEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { createRunActionHandler } from '../../dist/tools/run-action.js'; +import { createTmpProject } from '../helpers/tmp-project.js'; + +const FAIL_SELECTOR_ENV = { + ok: false, + data: { passed: false, output: "Element with id 'addr-line1' not found", flowFile: 'x', platform: 'ios' }, +}; +const fakeMaestroRun = (env) => async () => ({ content: [{ type: 'text', text: JSON.stringify(env) }], isError: true }); + +const ACTION_YAML = [ + '# id: demo', + '# intent: address flow', + '# status: active', + '# expectedRouteSequence: [HomeAddress, PhoneNumber]', + '', + '- tapOn:', + ' id: "addr-line1"', +].join('\n'); + +let project; +beforeEach(() => { project = createTmpProject(); }); +afterEach(() => { project.cleanup(); }); + +function parse(r) { return JSON.parse(r.content[0].text); } + +test('SELECTOR_NOT_FOUND on an OFF-sequence live route → ROUTE_DRIFT, repair skipped', async () => { + project.seedAction('demo', ACTION_YAML); + let repairCalled = false; + const handler = createRunActionHandler({ + maestroRun: fakeMaestroRun(FAIL_SELECTOR_ENV), + repairAction: async () => { repairCalled = true; return { content: [{ type: 'text', text: JSON.stringify({ ok: true, data: { patched: true } }) }] }; }, + getLiveRoute: async () => 'CouponCode', // an inserted screen, not in the expected sequence + }); + const env = parse(await handler({ actionId: 'demo', projectRoot: project.root })); + assert.equal(env.ok, false); + assert.equal(env.code, 'ROUTE_DRIFT'); + assert.equal(repairCalled, false, 'fuzzy selector repair must be skipped on structural drift'); +}); + +test('SELECTOR_NOT_FOUND on an EXPECTED live route still attempts repair (not drift)', async () => { + project.seedAction('demo', ACTION_YAML); + let repairCalled = false; + const handler = createRunActionHandler({ + maestroRun: fakeMaestroRun(FAIL_SELECTOR_ENV), + repairAction: async () => { repairCalled = true; return { content: [{ type: 'text', text: JSON.stringify({ ok: false, data: { patched: false } }) }] }; }, + getLiveRoute: async () => 'PhoneNumber', // on the expected sequence → not drift + }); + await handler({ actionId: 'demo', projectRoot: project.root }); + assert.equal(repairCalled, true, 'an expected-route selector failure should still go to repair'); +}); + +test('no expectedRouteSequence → drift check is inert, repair attempted as before', async () => { + project.seedAction('demo', ['# id: demo', '# intent: x', '# status: active', '', '- tapOn:', ' id: "addr-line1"'].join('\n')); + let repairCalled = false; + const handler = createRunActionHandler({ + maestroRun: fakeMaestroRun(FAIL_SELECTOR_ENV), + repairAction: async () => { repairCalled = true; return { content: [{ type: 'text', text: JSON.stringify({ ok: false, data: { patched: false } }) }] }; }, + getLiveRoute: async () => 'CouponCode', // would be drift IF a sequence were recorded + }); + await handler({ actionId: 'demo', projectRoot: project.root }); + assert.equal(repairCalled, true, 'without a recorded sequence the drift check must not fire'); +}); diff --git a/scripts/cdp-bridge/test/unit/gh-186-route-sequence.test.js b/scripts/cdp-bridge/test/unit/gh-186-route-sequence.test.js new file mode 100644 index 00000000..cc9f02f2 --- /dev/null +++ b/scripts/cdp-bridge/test/unit/gh-186-route-sequence.test.js @@ -0,0 +1,55 @@ +// GH #186 P1: structural route-drift detection. A saved action records its +// expected route sequence; if the live flow inserts a screen (the report saw a +// CouponCode screen appear between HomeAddress and PhoneNumber), a selector +// failure should be classified as ROUTE_DRIFT — NOT SELECTOR_NOT_FOUND — so +// auto-repair doesn't waste a fuzzy-match cycle on a structural change. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + validateRouteSequenceAgainstGraph, + classifyRouteDriftAfterFailure, +} from '../../dist/nav-graph/route-sequence.js'; + +const graph = (screens) => ({ meta: {}, navigators: [], all_screens: screens }); + +// ── pre-flight: expected screen missing from the graph (definite mismatch) ── + +test('validateRouteSequenceAgainstGraph passes when every expected screen exists', () => { + const r = validateRouteSequenceAgainstGraph(graph(['Home', 'HomeAddress', 'PhoneNumber']), ['HomeAddress', 'PhoneNumber']); + assert.equal(r.ok, true); +}); + +test('validateRouteSequenceAgainstGraph fails when an expected screen is gone (renamed/removed)', () => { + const r = validateRouteSequenceAgainstGraph(graph(['Home', 'PhoneNumber']), ['HomeAddress', 'PhoneNumber']); + assert.equal(r.ok, false); + assert.deepEqual(r.missing, ['HomeAddress']); +}); + +test('validateRouteSequenceAgainstGraph does NOT false-positive on an empty/unknown graph', () => { + assert.equal(validateRouteSequenceAgainstGraph(graph([]), ['HomeAddress']).ok, true); + assert.equal(validateRouteSequenceAgainstGraph(null, ['HomeAddress']).ok, true); +}); + +test('validateRouteSequenceAgainstGraph is a no-op with no expected sequence', () => { + assert.equal(validateRouteSequenceAgainstGraph(graph(['Home']), []).ok, true); + assert.equal(validateRouteSequenceAgainstGraph(graph(['Home']), undefined).ok, true); +}); + +// ── post-failure: live route off the expected sequence (inserted screen) ── + +test('classifyRouteDriftAfterFailure flags an inserted screen as drift', () => { + const v = classifyRouteDriftAfterFailure({ expectedSequence: ['HomeAddress', 'PhoneNumber'], liveRoute: 'CouponCode' }); + assert.equal(v.isDrift, true); + assert.equal(v.liveRoute, 'CouponCode'); + assert.match(v.reason, /CouponCode/); +}); + +test('classifyRouteDriftAfterFailure does NOT flag drift when the live route is expected', () => { + const v = classifyRouteDriftAfterFailure({ expectedSequence: ['HomeAddress', 'PhoneNumber'], liveRoute: 'PhoneNumber' }); + assert.equal(v.isDrift, false); +}); + +test('classifyRouteDriftAfterFailure is conservative with no sequence / no live route', () => { + assert.equal(classifyRouteDriftAfterFailure({ expectedSequence: [], liveRoute: 'X' }).isDrift, false); + assert.equal(classifyRouteDriftAfterFailure({ expectedSequence: ['A'], liveRoute: null }).isDrift, false); +}); diff --git a/scripts/cdp-bridge/test/unit/gh-186-runflow-validator.test.js b/scripts/cdp-bridge/test/unit/gh-186-runflow-validator.test.js new file mode 100644 index 00000000..8d223240 --- /dev/null +++ b/scripts/cdp-bridge/test/unit/gh-186-runflow-validator.test.js @@ -0,0 +1,110 @@ +// GH #186 P1: runFlow allowlist via inline-at-validation. runFlow was excluded +// from ALLOWED_COMMANDS, so cdp_run_action hard-failed any saved action using it +// (commonly for conditional dialog handling). We allow it, recursively validate +// its inline commands, and securely resolve + EXPAND {file} refs inline (so the +// serialized flow written to /tmp has no remaining file references), with +// path-traversal / containment / cycle / depth guards. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { parseAndValidateFlow, MaestroValidationError } from '../../dist/domain/maestro-validator.js'; + +const FLOW_ROOT = '/proj/.rn-agent/actions'; +const FLOW_DIR = FLOW_ROOT; + +// In-memory sub-flow files for the {file} tests. +function makeFs(files) { + return { + readFileFn: (p) => { + if (!(p in files)) throw new Error('ENOENT ' + p); + return files[p]; + }, + // identity realpath unless the path is mapped to an "escaped" location + realpathFn: (p) => (files.__realpath && files.__realpath[p]) || p, + }; +} + +// ── inline runFlow ────────────────────────────────────────────────────── + +test('inline runFlow {when, commands} is allowed and its nested commands are validated', () => { + const yaml = `- runFlow:\n when:\n visible: "Open in"\n commands:\n - tapOn: "Open in"`; + const flow = parseAndValidateFlow(yaml, { rejectHeader: true }); + assert.equal(flow.commands.length, 1); + assert.ok('runFlow' in flow.commands[0]); +}); + +test('inline runFlow with a DENIED nested command is rejected', () => { + const yaml = `- runFlow:\n commands:\n - runScript: evil.js`; + assert.throws(() => parseAndValidateFlow(yaml, { rejectHeader: true }), MaestroValidationError); +}); + +// ── {file} resolution + inline expansion ───────────────────────────────── + +test('runFlow {file} within flowRoot is expanded inline (no file ref remains)', () => { + const sub = `${FLOW_ROOT}/dialog.yaml`; + const fs = makeFs({ [sub]: `- tapOn: "Allow"` }); + const yaml = `- runFlow: dialog.yaml`; + const flow = parseAndValidateFlow(yaml, { rejectHeader: true, flowDir: FLOW_DIR, flowRoot: FLOW_ROOT, ...fs }); + // The sub-flow's command is present and the serialized raw has no runFlow file ref. + assert.ok(!/dialog\.yaml/.test(flow.raw), 'no file ref remains in serialized flow'); + assert.ok(/Allow/.test(flow.raw), 'sub-flow command was inlined'); +}); + +test('runFlow {file} with .. traversal is rejected', () => { + const fs = makeFs({}); + assert.throws( + () => parseAndValidateFlow(`- runFlow: ../../etc/evil.yaml`, { rejectHeader: true, flowDir: FLOW_DIR, flowRoot: FLOW_ROOT, ...fs }), + MaestroValidationError, + ); +}); + +test('runFlow {file} with an absolute path is rejected', () => { + const fs = makeFs({}); + assert.throws( + () => parseAndValidateFlow(`- runFlow: /etc/evil.yaml`, { rejectHeader: true, flowDir: FLOW_DIR, flowRoot: FLOW_ROOT, ...fs }), + MaestroValidationError, + ); +}); + +test('runFlow {file} that realpath-escapes flowRoot (symlink) is rejected', () => { + const inside = `${FLOW_ROOT}/link.yaml`; + const fs = makeFs({ [inside]: `- tapOn: x`, __realpath: { [inside]: '/etc/outside.yaml' } }); + assert.throws( + () => parseAndValidateFlow(`- runFlow: link.yaml`, { rejectHeader: true, flowDir: FLOW_DIR, flowRoot: FLOW_ROOT, ...fs }), + MaestroValidationError, + ); +}); + +test('runFlow {file} with a non-yaml extension is rejected', () => { + const fs = makeFs({ [`${FLOW_ROOT}/x.js`]: `whatever` }); + assert.throws( + () => parseAndValidateFlow(`- runFlow: x.js`, { rejectHeader: true, flowDir: FLOW_DIR, flowRoot: FLOW_ROOT, ...fs }), + MaestroValidationError, + ); +}); + +test('runFlow {file} cycle (a -> b -> a) is rejected', () => { + const a = `${FLOW_ROOT}/a.yaml`; + const b = `${FLOW_ROOT}/b.yaml`; + const fs = makeFs({ [a]: `- runFlow: b.yaml`, [b]: `- runFlow: a.yaml` }); + assert.throws( + () => parseAndValidateFlow(`- runFlow: a.yaml`, { rejectHeader: true, flowDir: FLOW_DIR, flowRoot: FLOW_ROOT, ...fs }), + MaestroValidationError, + ); +}); + +test('runFlow {file} exceeding max depth is rejected', () => { + const files = {}; + for (let i = 0; i < 8; i++) files[`${FLOW_ROOT}/d${i}.yaml`] = `- runFlow: d${i + 1}.yaml`; + const fs = makeFs(files); + assert.throws( + () => parseAndValidateFlow(`- runFlow: d0.yaml`, { rejectHeader: true, flowDir: FLOW_DIR, flowRoot: FLOW_ROOT, maxRunFlowDepth: 3, ...fs }), + MaestroValidationError, + ); +}); + +test('runFlow {file} with no flowRoot context is rejected (cannot resolve safely)', () => { + assert.throws( + () => parseAndValidateFlow(`- runFlow: dialog.yaml`, { rejectHeader: true }), + MaestroValidationError, + ); +});