Fix recovery secret retention and retry never-started requests

.github/workflows/ci.yml

@@ -46,3 +46,12 @@ jobs:
       - name: Actual Midscene with controlled provider
         if: always() && !cancelled()
         run: timeout 90s node test/vision.smoke.mjs
+      - name: Review regressions for retention and preflight
+        if: always() && !cancelled()
+        run: timeout 90s node test/review-followups.smoke.mjs
+      - name: Recovery capture isolation and late frames
+        if: always() && !cancelled()
+        run: timeout 120s node test/recovery-capture.smoke.mjs
+      - name: Environment retention across dispatch and handoff
+        if: always() && !cancelled()
+        run: timeout 90s node test/environment-retention.smoke.mjs

docs/design.md

@@ -24,6 +24,8 @@ A run journal persists work, not browser memory. It flushes intent before dispat
 
 [recover-form.test.json](../examples/recover-form.test.json) shows a half-filled form. Only use `safeToRepeat` where the application's behavior justifies it. Typing and navigation may autosave or submit changes. Recovery freezes retained inputs before navigation, so an empty replacement form cannot erase them. Secrets, files, one-time codes, and opaque editor state are not recoverable payloads; re-entry may be necessary.
 
+Before an environment-backed UI dispatch, the run records `recovery.capture_suspended`. Automatic recovery-value observations then remain disabled for that run, including resume, reconstruction, and executor handoff; selectors and exact-value matching cannot reliably identify transformed or replaced secrets. Existing snapshots and explicit non-secret Playwright fill intents remain usable, but later page-derived changes are not captured. Legacy Playwright dispatch receipts without the guard marker suspend capture conservatively. Keep recovery allowlists non-sensitive; this does not scrub old files or promise complete redaction of screenshots/page evidence.
+
 Declare `recovery.reconcile` and `reconcileOutcome` before a possibly uncertain effect. `reconcile --run ID` evaluates that UI evidence. For a completed deterministic dispatch, matching evidence advances its cursor once instead of resubmitting. For a completed autonomous dispatch without a cursor, continuation stays blocked rather than guessing.
 
 After a browser crash, a new observation task can inspect application state in the same profile; `reconcile --run ORIGINAL --from-run OBSERVER` uses its live page with the original expectations. This does not retarget or restore the original run. Evidence of absence must be strong enough for the application; a missing toast is not proof that Save did nothing.

package.json

@@ -26,7 +26,7 @@
     "test:python": "python3 -m unittest discover -s worker/tests -v",
     "check": "npm run typecheck && npm test && npm run test:python",
     "prepack": "npm run build",
-    "test:browser": "node test/adapter.smoke.mjs && node test/workflows.smoke.mjs && node test/vision.smoke.mjs"
+    "test:browser": "node test/adapter.smoke.mjs && node test/workflows.smoke.mjs && node test/vision.smoke.mjs && node test/review-followups.smoke.mjs && node test/recovery-capture.smoke.mjs && node test/environment-retention.smoke.mjs"
   },
   "dependencies": {
     "@effect/platform-node": "4.0.0-rc.116",

src/page-worker.ts

@@ -2,7 +2,7 @@ import * as fs from "node:fs";
 import path from "node:path";
 import { pathToFileURL } from "node:url";
 import { randomUUID } from "node:crypto";
-import type { Browser, Page, Locator, FrameLocator } from "playwright-core";
+import type { Browser, Page, Locator, FrameLocator, ElementHandle, Frame } from "playwright-core";
 import { BrowserError, type Check } from "./contracts.js";
 import { Journal, remaining, digest, atomic, type Data } from "./journal.js";
 import { Cdp, checkpoint, liveStateExpression } from "./cdp.js";
@@ -75,15 +75,124 @@ export async function pollChecks(page: Page, checks: readonly Check[], timeoutMs
     await new Promise(resolve => setTimeout(resolve, 100));
   }
 }
-async function captureFields(page: Page, journal: Journal): Promise<void> {
-  for (const f of journal.meta().task.recovery?.fields ?? []) {
-    const { locator } = await scoped(page, f);
-    if (await locator.count() !== 1) continue;
-    const result = await locator.evaluate(readElement, { kind: "value" });
-    if (result.error) throw new BrowserError({ code: "retention_forbidden", reason: "A recovery allowlist includes a sensitive or unsupported control. Remove it; no value was saved." });
-    if (result.available === false) continue;
-    journal.saveField(f.key, result.actual, "observed");
+// Recovery is a passive snapshot: a later step's frame may not exist yet.
+// Pin each existing frame without waiting or relaxing required action/check scopes.
+async function captureScope(page: Page, s: Scoped): Promise<{ frame: Frame; locator: Locator } | undefined> {
+  if (s.shadow === "closed") throw new BrowserError({ code: "unsupported_scope", reason: "Closed shadow-root DOM access is unsupported. Use explicitly requested visual evidence." });
+  let frame = page.mainFrame();
+  for (const selector of s.frames ?? []) {
+    const nodes = await frame.locator(css(selector, s.shadow)).elementHandles();
+    try {
+      if (nodes.length === 0) return undefined;
+      if (nodes.length !== 1 || !(await nodes[0]!.evaluate(e => e.nodeType === 1 && /^(IFRAME|FRAME)$/.test((e as Element).tagName)))) {
+        throw new BrowserError({ code: "unsupported_scope", reason: "The frame path did not identify exactly one frame. Recovery capture cannot establish its scope." });
+      }
+      const child = await nodes[0]!.contentFrame();
+      if (!child) return undefined;
+      frame = child;
+    } finally { await Promise.allSettled(nodes.map(node => node.dispose())); }
+  }
+  return { frame, locator: frame.locator(css(s.selector ?? "body", s.shadow)) };
+}
+function suspendRecoveryCapture(journal: Journal): void {
+  if (!journal.events().some(e => e.type === "recovery.capture_suspended")) {
+    journal.append("recovery.capture_suspended", {
+      reason: "Environment-backed input may be moved or transformed by the page. Automatic recovery-value capture is disabled for this run; prior snapshots and explicit non-secret input intents remain available.",
+    });
+  }
+}
+async function captureFields(page: Page, journal: Journal, save = true): Promise<void> {
+  const task = journal.meta().task;
+  const fields = task.recovery?.fields ?? [];
+  if (fields.length === 0) return;
+  const steps = [...(task.steps ?? []), ...(task.recovery?.reconstruct ?? [])];
+  const events = journal.events();
+  const suspended = events.some(e => e.type === "recovery.capture_suspended");
+  // Old receipts cannot prove which environment-input boundary was crossed.
+  const legacy = steps.some(s => s.valueFromEnv !== undefined) && events.some(e =>
+    e.type === "action.start" && e.data.engine === "playwright" && e.data.recoveryGuarded !== true);
+  if (suspended || legacy) {
+    if (!suspended) suspendRecoveryCapture(journal);
+    save = false; // Still reject known protected aliases, but never read new page values.
   }
+  const protectedSteps = steps.filter(s => s.valueFromEnv !== undefined && s.selector !== undefined);
+  // These values never enter browser evaluation arguments. A page may redefine
+  // its prototypes; equality against environment secrets belongs in this process.
+  const protectedValues = new Set(protectedSteps
+    .map(step => process.env[step.valueFromEnv!])
+    .filter((value): value is string => value !== undefined && value.length > 0));
+  const handles: ElementHandle[] = [];
+  try {
+    const candidates: Array<{ field: (typeof fields)[number]; node: ElementHandle<Element>; frame: Frame }> = [];
+    for (const field of fields) {
+      const scope = await captureScope(page, field);
+      if (!scope) continue;
+      const nodes = await scope.locator.elementHandles();
+      handles.push(...nodes);
+      if (nodes.length !== 1) continue;
+      // Locator matches are Elements; Playwright declares their handles as Node.
+      const node = nodes[0]! as ElementHandle<Element>;
+      const frame = await node.ownerFrame();
+      if (frame) candidates.push({ field, node, frame });
+    }
+    if (candidates.length === 0) return;
+
+    // Pin recovery nodes first, then resolve each protected frame path once.
+    // Cache only this snapshot's scopes, never protected node identities/values.
+    const protectedFrames = new Map<string, Frame | undefined>();
+    const protectedLocators = new Map<Frame, Locator[]>();
+    for (const step of protectedSteps) {
+      const key = JSON.stringify([step.frames ?? [], step.shadow ?? "none"]);
+      if (!protectedFrames.has(key)) protectedFrames.set(key, (await captureScope(page, step))?.frame);
+      const frame = protectedFrames.get(key);
+      if (!frame) continue;
+      const locators = protectedLocators.get(frame) ?? [];
+      locators.push(frame.locator(css(step.selector!, step.shadow)));
+      protectedLocators.set(frame, locators);
+    }
+
+    for (const { field: f, node, frame } of candidates) {
+      const current = protectedLocators.get(frame) ?? [];
+      if (!save) {
+        for (const protectedLocator of current) {
+          const overlaps = await protectedLocator.evaluateAll((elements, target) => {
+            for (let i = 0; i < elements.length; i++) if (elements[i] === target) return true;
+            return false;
+          }, node);
+          if (overlaps) {
+            throw new BrowserError({ code: "retention_forbidden", reason: "An environment-backed control overlaps the recovery allowlist. Remove it and use its environment reference for reconstruction." });
+          }
+        }
+        continue;
+      }
+
+      // Resolve protected selectors with Playwright's shadow-aware semantics,
+      // reject identity/sensitivity, then read that exact pinned node atomically.
+      let protectedLocator = current[0];
+      for (const next of current.slice(1)) protectedLocator = protectedLocator!.or(next);
+      const result: { actual: string; protected?: boolean; error?: string; available?: boolean } = protectedLocator
+        ? await protectedLocator.evaluateAll((elements, target) => {
+            const element = target as unknown as Element;
+            for (let i = 0; i < elements.length; i++) if (elements[i] === element) return { actual: "", protected: true };
+            // A page-controlled value getter can change these attributes. Check
+            // them before any value access, including environment comparisons.
+            const sensitive = element.matches('input[type=password], input[type=file], [autocomplete="one-time-code"]');
+            if (sensitive) return { actual: "", error: "Sensitive control values cannot be collected." };
+            if (!("value" in element)) return { actual: "", error: "Target is not a value control." };
+            const rect = element.getBoundingClientRect();
+            const visible = !!rect.width && !!rect.height && element.checkVisibility({ checkOpacity: true, checkVisibilityCSS: true });
+            if (!visible) return { actual: "", available: false };
+            return { actual: String((element as HTMLInputElement).value) };
+          }, node)
+        : await node.evaluate(readElement, { kind: "value" });
+      if (result.protected || protectedValues.has(result.actual)) {
+        throw new BrowserError({ code: "retention_forbidden", reason: "An environment-backed control overlaps the recovery allowlist. Remove it and use its environment reference for reconstruction." });
+      }
+      if (result.error) throw new BrowserError({ code: "retention_forbidden", reason: "A recovery allowlist includes a sensitive or unsupported control. Remove it; no value was saved." });
+      if (result.available === false) continue;
+      journal.saveField(f.key, result.actual, "observed");
+    }
+  } finally { await Promise.allSettled(handles.map(handle => handle.dispose())); }
 }
 function valueFor(s: Step, journal: Journal, retained = journal.recoveryFields()): string {
   if (s.valueFromInput) {
@@ -116,6 +225,7 @@ async function runSteps(page: Page, journal: Journal, job: PageJob, snap: () =>
       throw new BrowserError({ code: "ambiguous_target", reason: "Action requires one matching control. Inspect the scoped page; no action was dispatched." });
     }
     if (!rebuilding) await captureFields(page, journal);
+    else if (s.valueFromEnv) await captureFields(page, journal, false);
     if (s.kind === "fill") for (const f of journal.meta().task.recovery?.fields ?? []) {
       if (f.selector === s.selector && JSON.stringify(f.frames) === JSON.stringify(s.frames) && f.shadow === s.shadow && !s.valueFromEnv) {
         const result = await locator.evaluate(readElement, { kind: "value" });
@@ -130,8 +240,12 @@ async function runSteps(page: Page, journal: Journal, job: PageJob, snap: () =>
     let actionId: string | undefined;
     if (s.kind !== "checkpoint") {
       if (remaining(journal.state()).actions <= 0) throw new BrowserError({ code: "budget_exhausted", reason: "Action budget exhausted." });
+      // Selectors, node identities and exact-value matching cannot follow an
+      // arbitrary formatter/rerender. Persist this boundary before dispatch so
+      // resume, reconstruction and a Jev/Midscene handoff cannot forget it.
+      if (s.valueFromEnv) suspendRecoveryCapture(journal);
       actionId = randomUUID();
-      journal.append("action.start", { id: actionId, kind: s.kind, safeToRepeat: s.safeToRepeat === true, step: index, selector: s.selector, frames: s.frames, engine: "playwright" });
+      journal.append("action.start", { id: actionId, kind: s.kind, safeToRepeat: s.safeToRepeat === true, step: index, selector: s.selector, frames: s.frames, engine: "playwright", recoveryGuarded: true });
     }
     switch (s.kind) {
       case "navigate": {

src/task.ts

@@ -75,6 +75,8 @@ export function validateTask(task: Task, testing: boolean): void {
     if (x.schema && JSON.stringify(x.schema).length > 20_000) invalid("Extraction schema too large.");
   }
   if (task.inputs && (Object.keys(task.inputs).length > 30 || Object.values(task.inputs).some(v => v.length > 20_000))) invalid("Inputs exceed their storage budget.");
+  const targetKey = (s: { selector?: string | undefined; frames?: readonly string[] | undefined; shadow?: string | undefined }) =>
+    JSON.stringify([s.selector?.trim(), (s.frames ?? []).map(f => f.trim()), s.shadow ?? "none"]);
   const fieldKeys = new Set<string>();
   for (const f of task.recovery?.fields ?? []) {
     scope(f);
@@ -99,6 +101,9 @@ export function validateTask(task: Task, testing: boolean): void {
       if (["navigate", "fill", "press", "select", "check", "scroll"].includes(s.kind) && values.length !== 1) invalid(`${s.kind} requires exactly one value source.`);
       if (s.valueFromInput && (!/^[a-zA-Z][\w-]{0,63}$/.test(s.valueFromInput) || task.inputs?.[s.valueFromInput] === undefined)) invalid("A named step input is missing.");
       if (s.valueFromEnv && !/^[A-Z_][A-Z_0-9]{0,127}$/.test(s.valueFromEnv)) invalid("Invalid environment reference.");
+      if (s.valueFromEnv && s.selector && task.recovery?.fields?.some(f => targetKey(f) === targetKey(s))) {
+        invalid("An environment-backed control cannot be a recovery field. Remove it from recovery.fields; use its environment reference for reconstruction.");
+      }
       if (s.valueFromRecovery && !fieldKeys.has(s.valueFromRecovery)) invalid("Recovery key is not allowlisted.");
       if (s.kind === "goal" && !s.goal?.trim()) invalid("goal step requires a goal.");
       if (s.kind === "checkpoint" && !s.checks?.length) invalid("checkpoint requires assertions.");

src/workflows.ts

@@ -249,7 +249,11 @@ export function runTask(input: Task, testing = false) {
     const config = yield* io(host.configuration);
     const profile = yield* io(() => fs.promises.realpath(config.profileDir));
     const { journal, duplicate } = yield* sync(() => createRun(host.home(), task, testing, profile));
-    if (duplicate) return { ...summary(journal.state()), duplicate: true };
+    // Only preflight failures are retryable through the idempotency key. Once
+    // execution started (or an unknown event exists), recovery must be explicit.
+    if (duplicate && !journal.events().every(e => e.type === "created" || e.type === "preflight.failed")) {
+      return { ...summary(journal.state()), duplicate: true };
+    }
     return yield* io(host.connect).pipe(Effect.flatMap(session => attempt(journal, session, "new")), Effect.tap(result => sync(() => {
       if (task.recipe && ["precondition_failed", "ambiguous_target", "assertion_failed"].includes(journal.state().reasonCode)) quarantineRecipe(host.home(), task.recipe, journal.state().reasonCode);
     })), Effect.catchTag("BrowserError", e => sync(() => {

test/adapter.smoke.mjs

@@ -4,7 +4,7 @@ import {fixture} from './support/chrome.mjs';
 import {mcpClient} from './support/mcp-client.mjs';
 import {Cdp,checkpoint} from '../dist/cdp.js';
 import {createRun,atomic} from '../dist/journal.js';
-import {attachPage,pageJob,checkOne} from '../dist/page-worker.js';
+import {attachPage,pageJob,checkOne,pollChecks} from '../dist/page-worker.js';
 import {legacyExpression} from '../dist/dom.js';
 import {screenshotRun} from '../dist/workflows.js';
 import path from 'node:path';
@@ -28,6 +28,23 @@ try {
  await assert.rejects(()=>checkOne(connected.page,{kind:'count',frames:['#missing'],selector:'button',value:'0'}),/frame path/);
  assert.equal((await checkOne(connected.page,{kind:'text',selector:'#hidden',value:'<not visible>'})).passed,false);
  assert.equal((await raw.evaluate(sid,legacyExpression([{kind:'text',selector:'#missing',value:'matching elements'}])))[0].passed,false);checks.push('unsupported scopes and diagnostic sentinels cannot pass');
+ // Missing elements are not visible; missing scopes and ambiguous matches remain unknown.
+ for(const scope of [{},{frames:['#frame']},{frames:['#frame','#nested']},{shadow:'open'}]) for(const value of ['false','true']) {
+  const c={kind:'visible',selector:'#absent',value,...scope}, result=await checkOne(connected.page,c);
+  assert.equal(result.actual,'false');assert.equal(result.passed,value==='false');
+  if(!scope.frames && !scope.shadow)assert.deepEqual(result,(await raw.evaluate(sid,legacyExpression([c])))[0]);
+ }
+ assert.equal((await checkOne(connected.page,{kind:'visible',selector:'#hidden',value:'false'})).passed,true);
+ for(const value of ['false','true'])assert.equal((await checkOne(connected.page,{kind:'visible',selector:'h1, #name',value})).passed,false);
+ for(const kind of ['text','value','checked'])assert.equal((await checkOne(connected.page,{kind,selector:'#absent',value:'<0 matching elements>'})).passed,false);
+ assert.equal((await checkOne(connected.page,{kind:'count',selector:'#absent',value:'0'})).passed,true);
+ await assert.rejects(()=>checkOne(connected.page,{kind:'visible',frames:['#missing'],selector:'button',value:'false'}),/frame path/);
+ await assert.rejects(()=>checkOne(connected.page,{kind:'visible',shadow:'closed',selector:'button',value:'false'}),/Closed shadow/);
+ await connected.page.evaluate(()=>{const e=document.createElement('span');e.id='transient';e.textContent='Busy';document.body.append(e)});
+ const disappears={kind:'visible',selector:'#transient',value:'false'};
+ assert.equal((await checkOne(connected.page,disappears)).passed,false);
+ const waiting=pollChecks(connected.page,[disappears],3000);await connected.page.locator('#transient').evaluate(e=>e.remove());
+ assert.equal((await waiting)[0].passed,true);checks.push('negative visibility, legacy parity, and disappearance polling');
  const extracted=await pageJob({root:f.directory,runId:journal.runId,session:f.session,targetId,operation:'extract',extract:[{name:'good',kind:'value',selector:'#inside',shadow:'open'},{name:'missing',kind:'value',selector:'#absent'}],timeoutMs:10000},new AbortController().signal);assert.equal(extracted.fields.good.value,'Shadow');assert.ok(extracted.fields.missing.error);assert.equal(extracted.complete,false);checks.push('partial extraction preserves valid fields');
  const first=await Effect.runPromise(screenshotRun({runId:journal.runId}));assert.ok(first.width<=1800);assert.equal(first.scale.x,first.width/first.css.width);
  await raw.send('Emulation.setDeviceMetricsOverride',{width:1280,height:800,deviceScaleFactor:2,mobile:false},sid);