feat(gate): repeat runs per version, per-version variance, safe interruption - #92
Conversation
…2 guard Closes #62. `npm run gate:matrix` refused to run for real, and its dry-run path hard-coded every step to PASS. The driver that was missing — bring up a seeded version, open a browser against it, replay the compiled program, tear it down — now lives in experiments/gate-v1/live-run.ts. Per version: compose up -> /api/health readiness -> seed -> seed-fingerprint gate -> Chromium -> login preamble -> ReplayRunner with dryRun:false -> teardown in a finally. Nothing in that file retries a step, downgrades an outcome, or catches an assertion failure; repair is the only permitted second attempt. A skip is not a failure. A version whose container never started, whose login broke, or whose seed state differs from the base version produced no measurement: it is recorded in out/matrix-run.json with a stage and a reason and never reaches the NDJSON. Counting it as a failed run would invent a data point. Two things the live path exposed: - The committed example bundle asserts getByRole("form") on Grafana's login page, which matches zero elements — an unnamed <form> has no ARIA role. The driver reported it honestly on its first live run rather than crashing. - contracts/metrics.schema.json has additionalProperties:false and no field for a failure reason, and stub repair always proposes null, so every genuine failure emits REPAIR_EXHAUSTED. LOCATOR_NOT_FOUND, ASSERTION_FAILED and TIMEOUT are flattened into one value. Widening the contract needs an ADR, so StepAttemptResult.first_pass_outcome keeps the real outcome in memory and the driver records it in its own ledger. That is a workaround, not a fix, and both docs say so. The bundle -> CompiledProgram adapter moves from tests/integration into src/runner/program.ts. Its comment said it was test-local because "the runtime never needs it today"; this is the change that made the runtime need it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ruption Closes #66. Stacked on #91 (the live matrix runner it repeats). One run per version is not a sample. PRD section 9 specifies 3x/day for 14 days -- >=42 runs and >=400 step-executions -- and swapping the calendar for the version matrix does not change the statistics. Eight pins at one run each is 8 runs, and it cannot tell "this locator broke on v12" from "that run flaked". --runs <n>, default 3. Clearing the section 9 floor across the eight pins needs --runs 6 (48 runs); --runs 5 gives 40 and lands two short, so the issue's own suggestion does not quite clear its own floor. The shortfall is reported rather than enforced: the CLI prints it before the first container boots and report.json carries a sample section with meets_floor and the exact gap. Each run is independent -- a fresh browser context and a fresh login per run, not per version. Reusing either would correlate the repeats and understate the spread, which is the one thing repeat runs exist to measure. perVersionBreakdown() in src/metrics/aggregate.ts reports runs_attempted, runs_succeeded, step_validity_per_run and step_validity_spread. A pooled ratio makes 3/3 and 2/3 the same number. status is no_data rather than 0 when a run emitted no step rows, because 0 is indistinguishable from every step failing. Interruption is safe, and building it found a real bug: Playwright installs its own SIGINT handler and closes the browser on Ctrl-C, which fired mid-run and crashed the matrix with "Target page, context or browser has been closed". chromium.launch now takes handleSIGINT/SIGTERM/SIGHUP false -- the driver owns that lifecycle. Ctrl-C finishes the run in flight, tears down, and records the version as SKIPPED (interrupted) with how far it got. Rows are appended after every run instead of written once at the end, so a partial NDJSON is valid. No run is ever discarded, including a failed one and one cut short. Measured: three live repeats of 9.5.21 against one unchanged container agreed exactly (spread 0). That is a weak probe -- a 2-step example program -- and the 12-step gate task from #25 is where flakiness would surface. Recorded as such. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code reviewRead the PR description, the full diff (stacked on #91), and cross-checked the runtime behavior against OverviewCloses #66. Adds This PR fixes the data-loss issue I flagged in my #91 review. I verified the mechanism directly: New correctness issue: two disagreeing implementations of the §9 floor
meets_floor: runs.length * program.steps.length >= 400This assumes every completed run executed every step in the program. That's not always true: Fix: have Residual / minor items
Code quality, tests, securitySame strong fit with repo conventions as #91 — rationale-heavy comments, honest docs updates, no contract changes smuggled in. SummaryMeaningfully improves on #91: the crash/data-loss issue is genuinely fixed via per-run incremental persistence, and the SIGINT/Playwright interaction bug is a legitimate find caught by actually building the interrupt path. The one thing worth fixing before merge is the 🤖 Reviewed with Claude Code |
…ding) # Conflicts: # docs/gate/runner.md # experiments/gate-v1/README.md # experiments/gate-v1/live-run.ts # experiments/gate-v1/run-matrix.ts # tests/unit/gate-matrix.test.ts
myselfsiddharth
left a comment
There was a problem hiding this comment.
Approving. Merge conflicts against `main` (from #91 + #89/ADR-0006 landing) resolved by merge commit — every conflicted file (`docs/gate/runner.md`, `experiments/gate-v1/README.md`, `experiments/gate-v1/live-run.ts`, `experiments/gate-v1/run-matrix.ts`, `tests/unit/gate-matrix.test.ts`) verified byte-identical to this PR's original intended content after resolution — nothing from #92 was lost or altered.
`npm run ci` and `npm run test:canary` both green locally (152 + 6 tests), and all required GitHub checks pass: `lint-typecheck-test-secrets`, `privacy-canary`, `testbed-smoke`, CodeQL, PR title.
Per my earlier review, one known non-blocking issue (`matrix-run.json`'s `section9_floor.meets_floor` can diverge from `report.json`'s once a run fails partway through a step sequence) is tracked as a follow-up in #95 rather than blocking this merge.
🤖 Reviewed with Claude Code
Closes #66. Stacked on #91 (the live matrix runner it repeats) — base is
track1/b4-live-matrix; retarget tomainonce #91 lands.One run per version is not a sample. §9 specifies 3×/day for 14 days — ≥42 runs and ≥400 step-executions — and swapping the calendar for the version matrix does not change the statistics. Eight pins at one run each is 8 runs, and it cannot tell "this locator broke on v12" from "that run flaked".
--runs, default 3, and the arithmetic--runsThe issue suggests "8 versions × 5 gets there". It doesn't: 40 < 42.
runsToClearSection9(8)returns 6, and there's a test pinning that off-by-one, because it is exactly the shape of thing that ships as "we cleared §9".Default 3 is a deliberate compromise, not the floor: enough to see disagreement between repeats, cheap enough that people run it. 6 × 8 versions is 48 container boots. The floor is a property of the published measurement, not of the loop you run while developing — so the shortfall is reported, never enforced:
and in
report.json:Per-version variance
perVersionBreakdown()— a pooled ratio makes 3/3 and 2/3 the same number:{"testbed_version": "11.0.0", "runs_attempted": 3, "runs_succeeded": 2, "step_validity_per_run": [1, 0.5, 1], "step_validity_spread": 0.5, "status": "computed"}status: "no_data"rather than0when a run emitted no step rows — zero would be indistinguishable from every step having failed.Building the interrupt path found a real bug
Playwright installs its own SIGINT handler and closes the browser on Ctrl-C. That fires during the in-flight run, so the next
capturePageStatethrew and the matrix died with a stack trace instead of finishing the run it had been told to finish:chromium.launchnow passeshandleSIGINT/SIGTERM/SIGHUP: false— the driver already closes the browser in its ownfinally. After the fix:Verified after that interrupt:
exit=0, 6/6 NDJSON lines parse, 2 run rows with 2 distinctrun_ids,interrupted: truewithruns_planned: 6/runs_completed: 2,gate:reporthandled the partial data, and zero leftover containers or networks.Rows are now appended after every run rather than written once at the end —
MetricsEmitter.flush()overwrites, so the previous behaviour lost every row on interrupt.Independence is real, not asserted
A fresh browser context and a fresh login per run, not per version. Reusing either would let run 1's cookies and cache decide run 2's outcome; the repeats would correlate and the spread would understate the variance it exists to measure.
State-mutating tasks are handled explicitly rather than magically. The gate task creates a dashboard, so run 2 against one container collides and fails for a reason that is not churn:
npm run gate:matrix -- --runs 3 --param 'dashboard_title=Paragent Gate {run}'{run}in a--paramvalue is substituted per run. Auto-suffixing every param would silently change values a recording captured while the assertion templates still compared against the recorded hole.The measurement #66 asked for
Three live repeats of 9.5.21 against one unchanged container:
No harness flakiness observed. Stated with its caveat, in the PR and in
gate/runner.md: this is a weak probe. The program was the 2-step example bundle (navigate + fill on a login page). The 12-step gate task with drawer and picker interactions is where flakiness would surface, and that arrives with #25.No run is ever discarded
Including a failed one, and one cut short by an interrupt. A skip can now arrive with completed runs (interrupted partway, or login broke on repeat 2) and those runs are kept. Dropping an outlier is the single easiest way to manufacture a passing gate and would be undetectable in the report.
Skips stay distinct from failures:
versions_skipped[]gains aninterruptedstage, and skipped versions still never reach the NDJSON.Not done
Nothing reseeds between runs.
{run}substitution covers a task whose mutation is parameterisable; a task that mutates state it cannot parameterise away would need a container recreated per run, which is not implemented and is called out in the README.Tests
11 new unit tests (27 in the file): the 3/3-vs-2/3 distinction,
no_datavs0, both §9 floors independently, the 8×5=40 off-by-one, and{run}substitution including that it returns a new object. Guard proven — forcingstep_validity_spreadto 0 failsseparates 3/3 from 2/3.🤖 Generated with Claude Code