fix(recorder): carry a recorded wait step's duration through to replay

[myselfsiddharth]

Closes #83.

Summary

TrajectoryRecorder.wait(intent, ms) (src/recorder/session.ts) always takes an explicit duration but never wrote it anywhere the trajectory could carry it:

async wait(intent: string, ms: number): Promise<void> {
  await this.recordStep({
    intent,
    action: { type: "wait" },      // <- ms dropped here

At replay, executeAction's "wait" case (src/runner/actions.ts) found no bound duration via firstParam(action, params) and fell through to the bounded networkidle probe. So a recorded "wait 500ms" replayed as "wait for network idle" — a different condition, on a different clock, that can pass or fail independently of what was actually recorded.

Latent today (no trajectory/bundle in the tree has a wait step — verified across experiments/gate-v1/trajectories/*.json, contracts/examples/trajectory.example.json, artifacts/compiled/*.bundle.json), but real the moment a recorded task includes a deliberate wait.

Decision — ADR-0008

Per CONTRIBUTING ("prefer extending a schema via ADR over ad-hoc JSON fields"), this is a schema change, so it gets one. Considered and rejected synthesizing a param_ref + literal binding for the recorded duration (conflates a compile-time constant with the runtime-supplied-value mechanism fill/select/upload already use, and would let a same-named --param silently override what was recorded). Chose instead a literal wait_ms field, the same treatment already given to other compile-time-constant action fields (key for press, url_template for navigate).

Changes

• Schemas: wait_ms (non-negative integer) added to the action shape in contracts/trajectory.schema.json and contracts/cache-row.schema.json. Additive/optional — no existing artifact is invalidated, and none needed regenerating (nothing in-tree has a wait step).
• Recorder: wait() now emits action: { type: "wait", wait_ms: ms }. Its only public entry point requires ms, so a recorder-produced trajectory can no longer emit a bare, duration-less wait.
• Compiler: buildCompiledAction() passes wait_ms through unchanged, same as key/url_template/custom_op.
• Runner: the "wait" case now resolves a duration as action.wait_ms → firstParam(action, params) (the existing, separately-tested runtime-bound mechanism) → the unchanged networkidle fallback. The literal is checked first so it can't be silently overridden by a same-named --param binding.
• Docs: docs/gate/runner.md's "Bounded waits" section documents the fix; docs/README.md's ADR table and docs/gate/runner.md's frontmatter dates updated.

Test plan

• New unit tests in tests/unit/compiler.test.ts: wait_ms survives compileTrajectory unchanged; stays absent when the recorded step didn't carry one.
• New unit tests in tests/unit/runner-bounded-wait.test.ts: a wait_ms-carrying action replays as a sleep (no settled field, meaning networkidle never ran); a recorded wait_ms takes precedence over a same-action --param binding that would otherwise make it wait 10s.
• Extended the existing end-to-end recorder test in tests/unit/recorder.test.ts with a real recorder.wait("...", 50) call against a live fixture page, asserting the emitted trajectory step carries wait_ms: 50 and the whole trajectory still validates against the (updated) schema.
• Confirmed the two already-existing tests this change must not break still pass unchanged: the parameterless-wait → networkidle fallback tests, and "still uses a plain sleep when the step carries a positive duration" (the pre-existing param_refs-based mechanism, untouched).
• npm run ci (secret-scan, contracts, lint, lint:docs — 46 docs, typecheck, unit — 175 tests, integration — 5 tests) — all green
• npm run test:canary — green (privacy boundary, merge-blocking)

🤖 Generated with Claude Code

[myselfsiddharth]

@copilot resolve the merge conflicts in this pull request

[myselfsiddharth]

@copilot Review this PR after reading the docs

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved in commit 323648f by merging origin/main into this branch and fixing the conflict in docs/gate/runner.md.

[Copilot]

@copilot Review this PR after reading the docs

Reviewed after reading the updated docs (docs/gate/runner.md and docs/decisions/ADR-0008-wait-step-duration.md). The PR is consistent with the documented behavior, and targeted validation passed (lint:docs and the wait-related unit tests). No additional changes needed from my side.

[OM152002]

Ran it rather than read it. The fix is correct; one defect at a boundary the schema itself declares valid.

Verified: "latent today" holds (zero wait steps across all five in-tree trajectory/bundle files); validate:contracts passes on all six existing artifacts unregenerated; wait_ms: 150 slept 152 ms with settled undefined. Both guards bite — reverting the recorder one-liner fails the e2e test, and reverting the precedence to firstParam(...) ?? action.wait_ms fails at 10004 ms, proving the --param would have won. CI green locally: 178 unit, 14 integration.

wait_ms: 0 still falls through to networkidle

Both schemas say "minimum": 0, and wait(intent, 0) is a legal call — but the runner gates on the value:

const msRaw = action.wait_ms ?? firstParam(action, params);   // 0 ?? x -> 0   (correct)
if (Number.isFinite(ms) && ms > 0) { ... }                    // 0 fails here

Measured on this PR's own never-idle fixture:

wait_ms: 150  ->   152ms  settled=undefined  SLEPT (recorded path)
wait_ms: 0    ->  5004ms  settled=false      *** NETWORKIDLE PROBE ***

That is the exact semantic drift ADR-0008 closes, surviving where the schema says zero is valid. The ?? is right; the downstream > 0 undoes it. Either set exclusiveMinimum: 0 in both schemas, or — better, and truer to the ADR — gate on action.wait_ms !== undefined rather than on the value. A recorded zero is a recorded observation; replay should reproduce it as an instant no-op, not reinterpret it as "no duration given".

A fifth action shape didn't get the field

src/*/types.ts has five copies of the action shape, all carrying key? / url_template? / custom_op?. Four were updated; src/cache/types.ts:50-57 was missed. Runtime survives (finalize() spreads ...candidate.compiled_action), but cache-row.schema.json now declares a field the cache package's own type does not — so a cache-side consumer can't read row.compiled_action.wait_ms without a type error. Relevant to #64, which rewrites rows through that type.

Non-blocking

"replays as that sleep" asserts elapsed < 3000 + settled === undefined. The settled check is the real discriminator, but expect(elapsed).toBeGreaterThanOrEqual(150) would pin that it slept the recorded duration, not just that it avoided the probe.

Option C is the right call and well-argued — keeping networkidle because it's a different primitive rather than a degraded one, with loadProgram() accepting hand-written programs as the concrete reason. Rejecting option A because a --param could silently override a recorded value, and then testing that precedence, is the part most PRs skip.

[OM152002]

Can you fix the PR with suggested changes in the comment? And if it's deliberate, lmk....I'll approve and merge

[myselfsiddharth]

Both defects were real, neither was deliberate. Fixed in 2475dcb.

wait_ms: 0 → networkidle

Your read of the mechanism is exactly right: the ?? resolved 0 correctly and the > 0 one line later threw it away. Took the second option you offered — gate on presence, not magnitude — for the reason you gave: a recorded zero is an observation, and reinterpreting it as "no duration given" is the same condition-swap the ADR exists to close.

const recorded = action.wait_ms;
if (recorded !== undefined && Number.isFinite(recorded) && recorded >= 0) {
  await page.waitForTimeout(recorded);
  return { ok: true };
}
// runtime-bound param path unchanged below — still gated on > 0, since an
// unusable binding genuinely does mean "no duration was specified"

Reproduced your measurement before and after, on this PR's own never-idle fixture:

	before	after
wait_ms: 0	5004 ms, settled=false	8 ms, settled=undefined
wait_ms: 150	152 ms	155 ms

Left minimum: 0 alone rather than taking exclusiveMinimum: 0 — zero is now meaningful, so forbidding it would be the wrong half of the fix.

One thing your comment didn't ask for but the same gate implied: a negative or non-finite wait_ms can only reach the runner from a hand-authored program, and it must not become waitForTimeout(-1). It now falls through as if nothing was recorded. Pinned by a test.

The new zero test bites — reverting the gate fails it at 5008 ms on expected false to be undefined.

Fifth action shape

Confirmed and fixed. Also confirmed it was load-bearing rather than cosmetic, by the path you named:

probe.ts(3,30): error TS2339: Property 'wait_ms' does not exist on type 'CompiledAction'.

That's row.compiled_action.wait_ms against src/cache/types.ts before the change, clean after — so #64 would have hit it.

Non-blocking

Taken. Both sleep tests now assert elapsed >= 150 alongside the settled check, so a wait_ms that is read and then ignored no longer passes on the ceiling alone.

Also

The zero case is pinned in both packages, not just the runner — buildCompiledAction already used !== undefined so it was correct, but nothing stopped a future edit from letting a falsy-but-recorded 0 collapse into "absent", which would break the runner's precedence chain from upstream. ADR-0008's Decision section and docs/gate/runner.md now state the presence-not-magnitude rule and why minimum: 0 stays.

npm run ci green (181 unit, 14 integration), npm run test:canary green.

---

Generated by Claude Code

[myselfsiddharth]

@OM152002