Skip to content

feat(gate): drive a real browser per matrix version, remove the exit-2 guard - #91

Merged
myselfsiddharth merged 2 commits into
mainfrom
track1/b4-live-matrix
Jul 30, 2026
Merged

feat(gate): drive a real browser per matrix version, remove the exit-2 guard#91
myselfsiddharth merged 2 commits into
mainfrom
track1/b4-live-matrix

Conversation

@OM152002

Copy link
Copy Markdown
Collaborator

Closes #62.

npm run gate:matrix refused to run for real (exit 2), and the dry-run path hard-coded every
step to PASS. ReplayRunner already supported live mode; what was missing was the driver.
It 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.

It runs, and it reports failure honestly

Two versions live, same program, on a real daemon:

gate:matrix live — grafana-oss matrix, 2 version(s) to walk, program=prog-traj-example-grafana-login-nav (2 step(s))
[1/2] 9.5.21
  9.5.21: docker compose up (grafana/grafana:9.5.21)
  9.5.21: ready after 0.0s
  9.5.21: SUCCESS steps_valid=2/2 repairs=0 wall=0.5s
[2/2] 11.0.0
  11.0.0: SUCCESS steps_valid=2/2 repairs=0 wall=0.5s

Corrupted locator — every candidate in step 1's action chain broken:

{"step": 0, "outcome": "PASS"}
{"step": 1, "outcome": "REPAIR_EXHAUSTED", "first_pass": "LOCATOR_NOT_FOUND"}

task_success: false, steps_valid=1/2. Not a crash, not a pass.

Container killed mid-run — the matrix records it and keeps going:

9.5.21: SKIPPED (compose-up) — Container paragent-tb-9-5-21-grafana-1  Started /
                               container paragent-tb-9-5-21-grafana-1 exited (137)
11.0.0: SUCCESS steps_valid=2/2 repairs=0 wall=0.6s

docker ps -a after every run above: zero paragent-* containers.

A skip is not a failure

A version whose container never started, whose image would not pull, whose login broke, or whose
seed state differs from the base version produced no measurement. It goes to
out/matrix-run.json with a stage (compose-up, readiness, seed, fingerprint, browser,
login-preamble) and a reason, and never reaches the NDJSON. Recording it as a failed run would
invent a data point; dropping it would shrink the denominator in silence.

The fingerprint gate is the non-obvious one: if a version's seeded state differs from the base
version's, a step failure could be the seed's fault rather than the surface's, and there is no
honest way to attribute it afterwards. That version yields nothing instead of something
misleading.

Two things the live path exposed

1. The committed example bundle asserts something that cannot hold. Step 0 targets
getByRole("form") on Grafana's login page. Measured on 9.5.21:

<form> elements present : 1
getByRole("form")       : 0 match(es)
first <form> attrs      : {"ariaLabel":null,"labelledby":null,"name":null}

An unnamed <form> has no ARIA role, so the assertion times out. That bundle is a compile of a
hand-written example — exactly what #25 exists to replace. The driver surfaced it on its first
live run instead of crashing, which is the harness working. This PR does not fix the bundle;
hand-editing a compile artifact is not a fix, and the gate task lands with #25.

2. The NDJSON cannot say why a step failed. contracts/metrics.schema.json sets
additionalProperties: false and has no field for a failure reason, and StubRepairModelClient
always proposes null — so every genuine failure emits REPAIR_EXHAUSTED, flattening
LOCATOR_NOT_FOUND, ASSERTION_FAILED and TIMEOUT into one value. The issue's own acceptance
criterion ("expect LOCATOR_NOT_FOUND in the NDJSON") is unmeetable today, including at
--max-repairs 0.

Widening the metric row is a contract change and is deliberately not made here. Instead
StepAttemptResult.first_pass_outcome keeps the real outcome in memory and the driver records it
in its own ledger, which is harness output rather than a contract artifact. Both gate/runner.md
and the harness README say plainly that this is a workaround, and it is filed as an open question
needing an ADR.

Also moved: the bundle → program adapter

tests/integration/pipeline.test.ts carried it with the comment "local to this test on purpose:
the runtime never needs it today". This is the change that made the runtime need it, so it moved
to src/runner/program.ts and the integration test now exercises the
same adapter the gate runs. It is a mapping, not a schema change — no field is invented on either
contract, and cache bookkeeping (confidence, pool_eligible, …) is dropped rather than
smuggled through. architecture.md break 2 and stub 7 are narrowed accordingly; the
bundle → cache hop is still unwired.

Dry-run is unchanged and still labelled

--dry-run keeps working — it exercises the harness without Docker and the CI job depends on it.
mode is now recorded per run and dry rows still carry
dry-run — tokens remain 0; not a gate measurement. Mixing live and dry rows in one report is
the easiest way to publish a fabricated gate number.

What this still does not give you

A gate number. One run per version is not a sample — §9 asks for ≥42 runs and ≥400
step-executions, and nothing here can separate churn from flakiness yet. That is #66, which
needed this driver first: repeat runs of a dry run all produce the same hard-coded row.

New flags

--headed, --keep-up, --program <path> (a CompiledProgram or a compiled_trajectory
bundle), --param k=v (repeatable), --port <n>, --no-preamble for programs that log in as a
measured step. base_url/host/port are bound by the driver and cannot be overridden by
--param — a program must not be able to redirect itself away from the version being measured.

Tests

16 new unit tests covering what decides what gets recorded, none of which need a container:
the bundle→program mapping, the fingerprint-mismatch decision, and skip-reason extraction. Guard
proven — deleting the step_index sort fails
orders steps by step_index, not by position in the file.

composeFailureReason exists because the first version of this reported "Creating" as the
reason a version was skipped: compose interleaves progress with the cause and does not
consistently pick a stream.

npm run ci            # green — 125 unit, 1 integration, secret-scan clean, lint-docs clean (43 docs)
npm run test:canary   # 6 pass

🤖 Generated with Claude Code

…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>
@OM152002
OM152002 requested review from a team and myselfsiddharth as code owners July 29, 2026 08:00
@github-actions github-actions Bot added size/XL > 600 changed lines — consider splitting documentation Improvements or additions to documentation gate PRD section 9 gate measurement area: runner Touches runner area: experiments Touches experiments labels Jul 29, 2026
@myselfsiddharth

Copy link
Copy Markdown
Contributor

Code review

I read the PR description, the full diff, and cross-checked the runtime behavior against src/metrics/emitter.ts, src/runner/replay.ts, and src/testbed/docker.ts on the PR branch.

Overview

Closes #62: removes the exit-2 guard that blocked gate:matrix from ever driving a real browser, and adds experiments/gate-v1/live-run.ts, a per-version driver that composes up a pinned Grafana OSS tag, waits for readiness, seeds it, checks a seed-fingerprint against a baseline version, drives Chromium through the compiled program, and tears down in a finally. It also moves the bundle→program adapter out of a test-only helper into src/runner/program.ts so the gate driver and the integration test share one implementation, and adds a first_pass_outcome field that survives in-memory (not on the wire) so REPAIR_EXHAUSTED rows don't lose the real failure mode while repair is stubbed.

The PR is explicit about what it doesn't claim: it doesn't fix the broken example bundle assertion, doesn't widen the metrics contract, and doesn't produce a gate number (one run per version isn't a sample). That matches the repo's stated culture well.

Correctness

A crash mid-matrix silently discards every already-measured version — this is the one I'd fix before merge.

  • MetricsEmitter (src/metrics/emitter.ts) only buffers rows in memory (this.rows.push(...) on emit()); nothing hits disk until flush() is called.
  • run-matrix.ts's main() calls emitter.flush() and writes matrix-run.json exactly once, after walkVersions() returns — and neither call is wrapped in try/finally.
  • walkVersions() has no try/catch around runVersionLive(); the JSDoc says this is deliberate ("anything that still throws is a harness bug and is left to propagate").
  • But not everything that can throw inside runVersionLive is actually a harness bug worth crashing the whole run for. In live-run.ts:238-239, browser.newContext() and context.newPage() are unguarded — unlike chromium.launch() two lines above, which is wrapped and correctly turned into a "browser"-stage skip. A transient Playwright context/page failure on, say, version 6 of 8 propagates all the way to main().catch(), which just logs and process.exit(1)s.

Net effect: if that happens, metrics.ndjson and matrix-run.json are never written at all — not just for the crashing version, but for the versions that were already successfully measured and sitting in the emitter's memory. For a harness whose entire design point is "a skip is not a failure, and dropping data would shrink the denominator in silence," silently losing real, already-collected measurements on an unattended multi-container run is a bigger version of the exact failure mode this PR is trying to prevent.

Two independent fixes, either is sufficient:

  1. Wrap context = await browser.newContext() / context.newPage() in the same try/catch pattern as chromium.launch, attributed to the "browser" stage.
  2. Wrap the walkVersions() call in try/finally so emitter.flush() and the summary write always happen with whatever was collected before a crash.

Minor: skip stage mislabeling for matrix-config-unavailable versions. In run-matrix.ts, versions pre-marked "status": "unavailable" in scripts/testbed/matrix.json are force-labeled stage: "compose-up", even though they were never actually attempted. SkipStage has no slot for "config says don't even try", so the ledger overstates what happened. Worth a follow-up, not blocking.

Everything else checks out — I verified rather than trusted the PR's own claims:

  • first_pass_outcome is genuinely never emitted to the NDJSON — emitStepMetric in replay.ts:418-445 builds StepMetric field-by-field (allowlist, not spread), so additionalProperties: false stays safe even though StepAttemptResult now carries the extra field.
  • bundleToProgram correctly sorts by step_index rather than array order, and correctly overrides only testbed_version while leaving site_key/task_key alone — matches the stated rationale and is pinned by tests.
  • composeUp/composeDown are synchronous (spawnSync), so the unawaited-looking calls are correct, not bugs.
  • The fingerprint-baseline logic only adopts a baseline from a version that completed a full run (not from a skip).
  • Teardown genuinely runs unconditionally via finally, including on early skip returns.

Code quality & conventions

Strong fit with the repo's documented style — every non-obvious decision has a comment explaining why, matching CONTRIBUTING's tone. Naming is consistent with existing code. Docs (architecture.md, gate/runner.md, experiments/gate-v1/README.md) are updated in the same PR and don't overclaim (the broken getByRole("form") assertion and "one run isn't a sample" are both stated plainly). size/XL label is accurate but the PR reads as a coherent single unit.

Test coverage

16 new unit tests cover exactly the parts the PR claims: bundle→program ordering/bookkeeping-stripping, isCompiledBundle discrimination, fingerprintMismatch (canonical comparison, drift detection), and composeFailureReason (stderr preference, progress-chatter filtering, honest fallback). Well-targeted at the logic that can silently corrupt the gate's raw data without needing Docker.

Gap: nothing tests the crash-data-loss scenario above, and it's hard to unit-test given it needs the full driver. A cheap regression test: mock runVersionLive to resolve for version 1 and reject for version 2, call walkVersions/main, assert metrics.ndjson still contains version 1's row. That test fails today.

Security

No credential handling changes. Fixture constants only, no shell interpolation on --param values, no secret-scan/canary concerns.

Summary

Solid, well-documented, honest implementation that does what it says. The one thing worth fixing before merge is the crash-mid-matrix data loss (unguarded newContext/newPage, plus no flush()-on-partial-failure) — a real gap in a PR whose entire design philosophy is "never lose or invent a data point," on the code path most likely to throw something unanticipated. The stage-mislabeling nit and missing regression test are good follow-ups but not blockers.


🤖 Reviewed with Claude Code

@myselfsiddharth myselfsiddharth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SOLID !

@myselfsiddharth
myselfsiddharth merged commit 4dd1c7c into main Jul 30, 2026
12 checks passed
@myselfsiddharth
myselfsiddharth deleted the track1/b4-live-matrix branch July 30, 2026 08:56
myselfsiddharth added a commit that referenced this pull request Jul 30, 2026
…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 added a commit that referenced this pull request Jul 30, 2026
…ruption (#92)

* feat(gate): drive a real browser per matrix version, remove the exit-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>

* feat(gate): repeat runs per version, per-version variance, safe interruption

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>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: myselfsiddharth <siddharthmehta0906@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: experiments Touches experiments area: runner Touches runner documentation Improvements or additions to documentation gate PRD section 9 gate measurement size/XL > 600 changed lines — consider splitting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Live matrix runner: drive a real browser per version (remove the exit-2 guard)

2 participants