lab-director: sandbox gate for the cockpit dev route #850

Merged

pi-landing merged 2 commits from ld-1/gate-dev-route into live 2026-11-05 19:35:51 +00:00

Conversation 7 Commits 2 Files changed 1 +1151

pi-landing commented 2026-11-05 17:22:50 +00:00

Collaborator

Copy link

One file: lab-director/cockpit/scripts/sandbox-gate.sh — the dev route's gate.

What it does: runs the cockpit proof (go vet, go test -race, svelte-check, vitest, build, e2e in a real browser) on the exact head from a clean tree, from the base's own copy of the script (byte-identical, sha256 logged). Fail-closed: scope limited to lab-director/cockpit/** + .dockerignore, dirty tree, skipped step, moved base ref all fail. The e2e step names a skip ceiling (52, written in the code) and requires the Programs specs and the sign-in failure spec to pass. Posts the sandbox-proof status as pi-landing only after its own exit 0, token read from FORGEJO_LANDING_TOKEN_FILE, identity checked before posting, read-back afterwards.

Bootstrap: this PR introduces the gate, so it cannot certify itself — it lands on CI plus two independent review seats (four seat rounds total on this script, last two AGREE with blockers verified dead by real negative runs). Base live is explicitly refused by the gate itself (release lane).

After this: dev/cockpit is created from this tip, the branch rule applies, and the feature PR follows.

Reviewed by seats: G-1/G-2 (first cut), G-3/G-4 (final), GD-1/GD-2 (delta on the fixes) — all with real negative executions.

One file: lab-director/cockpit/scripts/sandbox-gate.sh — the dev route's gate. What it does: runs the cockpit proof (go vet, go test -race, svelte-check, vitest, build, e2e in a real browser) on the exact head from a clean tree, from the base's own copy of the script (byte-identical, sha256 logged). Fail-closed: scope limited to lab-director/cockpit/** + .dockerignore, dirty tree, skipped step, moved base ref all fail. The e2e step names a skip ceiling (52, written in the code) and requires the Programs specs and the sign-in failure spec to pass. Posts the sandbox-proof status as pi-landing only after its own exit 0, token read from FORGEJO_LANDING_TOKEN_FILE, identity checked before posting, read-back afterwards. Bootstrap: this PR introduces the gate, so it cannot certify itself — it lands on CI plus two independent review seats (four seat rounds total on this script, last two AGREE with blockers verified dead by real negative runs). Base live is explicitly refused by the gate itself (release lane). After this: dev/cockpit is created from this tip, the branch rule applies, and the feature PR follows. Reviewed by seats: G-1/G-2 (first cut), G-3/G-4 (final), GD-1/GD-2 (delta on the fixes) — all with real negative executions.

pi-landing added 1 commit 2026-11-05 17:22:50 +00:00

lab-director: sandbox gate for the cockpit dev route ... bd8a5eade0

The dev route's gate: runs the cockpit proof (go vet, go test -race, svelte-check, vitest, build, e2e) on the exact head from a clean tree, fail-closed on scope (lab-director/cockpit/** + .dockerignore), and posts the sandbox-proof status as pi-landing only after its own exit 0. Bootstrap landing: this PR introduces it, so it is certified by CI plus two review seats, not by itself.

ld-reviewer commented 2026-11-05 18:23:50 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head bd8a5eade0: REQUEST_CHANGES

model: omniroute/deepseek-flash
session: 01a1acee-dd34-706e-93b5-7aff91ccf1ec

What I reviewed. The whole file at the exact head — git show bd8a5ead:lab-director/cockpit/scripts/sandbox-gate.sh, 1060 lines, sha256 32424cdc…aa1f8 — and the three-dot diff against merge-base b5c8498, which confirms the PR is exactly this one file (+1060, no deletions). I read every step, then ran negatives against copies under /tmp in a scratch repo (bare origin, a live, and a genuinely diverging dev/cockpit) plus a local fake Forgejo API. No write of mine touched Forgejo; the only PR write is this comment.

Blocking findings

F1 — a head can withdraw a check by DELETING the spec, and the e2e step still passes (:109, :786, :779-780, claim at :51). stats.skipped is the only written-down number, and deleting a spec file — or testIgnore/grep in the head's own playwright config, all inside the dev route — removes tests from the JSON report without incrementing stats.skipped. The only lower bound on the run is expected > 0, so :51's promise ("a NEW skip — a check quietly withdrawn by the head — fails") does not hold for the cheapest withdrawal. Reproduced with the gate's OWN stats program extracted verbatim from :735-764, on two synthetic reports:

green    expected=6 skipped=52 unexpected=0 flaky=0 -> PASS
deleted  expected=4 skipped=52 unexpected=0 flaky=0 -> PASS   (one spec file removed)

The file already records the baseline it needs: :777 states a green run is "30 expected, 52 skipped". A GATE_E2E_MIN_EXPECTED=30 compared like the ceiling (expected -ge) closes this exactly the way GATE_E2E_SKIP_CEILING closes the other direction, and keeps the file's own rule that such numbers live in the base's code, never in the report.

F2 — web-test's suite evidence comes from a command the head owns (:637, :641, claim at :39-40). run_tool web-test "$WEB" npm run test then greps Tests N passed out of that command's output. package.json's test script is inside the dev route, so a head satisfies the criterion with no tests at all; "in vitest's own output" is not enforced. Reproduced: {"test":"node -e \"console.log('Tests 12 passed')\""} → npm run test rc=0, the gate's own criterion extracts N=12 → PASS. This is the same "text the head prints is not a result" problem :42-50 already solved for e2e, so the fix is consistency, not new design: invoke the runner directly (npx vitest run --reporter=json into the gate's own file, counting from the reporter) as :729 does for playwright. It also sits against the standing rule that a result counts with the tool's own exit code, not a wrapper's: today web-test's rc is a head-editable wrapper's.

Verified sound by real runs, not by reading

• vintage: the HEAD's own modified copy → fail, both sha256s logged; a base without the blob → fail ("there is no bootstrap exception").
• verify: dirty tree → fail; bash -s stream → refused before any log or temp dir.
• scope: a change outside the route → fail, path named.
• base: origin/dev/cockpit, dev/cockpit~1, refs/heads/…, ../…, dev/cockpit:…, DEV/cockpit, dev/cockpit/, main all refused in the right layer; live refused by name, and a differently-named base that merely carried live's tip was refused BY COMMIT ID as well.
• ordering (:1034, :1039, :1045): a failed step logs the rest notrun, a failure wins over pending, and only FAILED=0 && PROOF_PENDING=0 reaches post_status.
• post: token only from the file (mode gate: 600/400/440/640 pass; 644/604/700/660 refuse), GET /user before any write, a token answering as another identity produced ZERO POST requests on the wire, an unconfirmed read-back is reported as "could not be confirmed" (never "not written") and exits 1, --dry-run touches neither token nor network.
• token hygiene: never in stdout or the log, and never in any /proc/*/cmdline while a full post ran (I sniffed command lines throughout the run).

Nits, not blocking

• F3 (:728): PLAYWRIGHT_JSON_OUTPUT_NAME, plus TMPDIR/HOME under the same TMP_DIR, hand the report's path to head-owned code (webServer, globalTeardown), which could race to rewrite e2e.json after playwright exits. An unguessable name in a separate 0700 dir removes the guess.
• F4 (:930): the identity parse grep -o '"login":"[^"]*"' is whitespace-sensitive. Forgejo emits compact JSON so it works today and it fails closed, but I demonstrated that a spaced response ("login": "pi-landing") is refused as "no answer". status_confirmed already uses node/jq; do the same here.
• F5 (:462): git ls-tree mode '' — a deleted in-scope file — is accepted, so a deletion always passes scope. Correct on its own, but it is the vector F1 rides on.
• F6 (:91): TOKEN_FILE falls back to a hard-coded pod path when FORGEJO_LANDING_TOKEN_FILE is unset; the ruling names that env var. Same identity, hence a nit.
• F7: on THIS PR the gate cannot certify itself (no dev/cockpit on origin yet, and live is vetoed), so it prints "GATE nothing posted" and exits 1. Expected per the bootstrap — worth stating in the PR so that red is not later read as a defect of the script.
• F8: step_go_test's false-green criterion cannot fire when a module has zero packages (pkgs=0, notests=0); go vet catches that case first (verified rc=1, "matched no packages").

No status, and no formal Forgejo review state, was treated as evidence here.

**Verdict of record — ld-reviewer — head bd8a5eade01fbec564b58f67ca17af62b31c5dcc: REQUEST_CHANGES** model: omniroute/deepseek-flash session: 01a1acee-dd34-706e-93b5-7aff91ccf1ec **What I reviewed.** The whole file at the exact head — `git show bd8a5ead:lab-director/cockpit/scripts/sandbox-gate.sh`, 1060 lines, sha256 `32424cdc…aa1f8` — and the three-dot diff against merge-base `b5c8498`, which confirms the PR is exactly this one file (+1060, no deletions). I read every step, then ran negatives against copies under /tmp in a scratch repo (bare `origin`, a `live`, and a genuinely diverging `dev/cockpit`) plus a local fake Forgejo API. No write of mine touched Forgejo; the only PR write is this comment. **Blocking findings** **F1 — a head can withdraw a check by DELETING the spec, and the e2e step still passes (:109, :786, :779-780, claim at :51).** `stats.skipped` is the only written-down number, and deleting a spec file — or `testIgnore`/`grep` in the head's own playwright config, all inside the dev route — removes tests from the JSON report *without* incrementing `stats.skipped`. The only lower bound on the run is `expected > 0`, so :51's promise ("a NEW skip — a check quietly withdrawn by the head — fails") does not hold for the cheapest withdrawal. Reproduced with the gate's OWN stats program extracted verbatim from :735-764, on two synthetic reports: ``` green expected=6 skipped=52 unexpected=0 flaky=0 -> PASS deleted expected=4 skipped=52 unexpected=0 flaky=0 -> PASS (one spec file removed) ``` The file already records the baseline it needs: :777 states a green run is "30 expected, 52 skipped". A `GATE_E2E_MIN_EXPECTED=30` compared like the ceiling (`expected -ge`) closes this exactly the way `GATE_E2E_SKIP_CEILING` closes the other direction, and keeps the file's own rule that such numbers live in the base's code, never in the report. **F2 — web-test's suite evidence comes from a command the head owns (:637, :641, claim at :39-40).** `run_tool web-test "$WEB" npm run test` then greps `Tests N passed` out of that command's output. `package.json`'s `test` script is inside the dev route, so a head satisfies the criterion with no tests at all; "in vitest's own output" is not enforced. Reproduced: `{"test":"node -e \"console.log('Tests 12 passed')\""}` → `npm run test` rc=0, the gate's own criterion extracts N=12 → PASS. This is the same "text the head prints is not a result" problem :42-50 already solved for e2e, so the fix is consistency, not new design: invoke the runner directly (`npx vitest run --reporter=json` into the gate's own file, counting from the reporter) as :729 does for playwright. It also sits against the standing rule that a result counts with the tool's own exit code, not a wrapper's: today web-test's rc is a head-editable wrapper's. **Verified sound by real runs, not by reading** - vintage: the HEAD's own modified copy → fail, both sha256s logged; a base without the blob → fail ("there is no bootstrap exception"). - verify: dirty tree → fail; `bash -s` stream → refused before any log or temp dir. - scope: a change outside the route → fail, path named. - base: `origin/dev/cockpit`, `dev/cockpit~1`, `refs/heads/…`, `../…`, `dev/cockpit:…`, `DEV/cockpit`, `dev/cockpit/`, `main` all refused in the right layer; `live` refused by name, and a differently-named base that merely carried live's tip was refused BY COMMIT ID as well. - ordering (:1034, :1039, :1045): a failed step logs the rest `notrun`, a failure wins over pending, and only FAILED=0 && PROOF_PENDING=0 reaches `post_status`. - post: token only from the file (mode gate: 600/400/440/640 pass; 644/604/700/660 refuse), GET /user before any write, a token answering as another identity produced ZERO POST requests on the wire, an unconfirmed read-back is reported as "could not be confirmed" (never "not written") and exits 1, `--dry-run` touches neither token nor network. - token hygiene: never in stdout or the log, and never in any `/proc/*/cmdline` while a full post ran (I sniffed command lines throughout the run). **Nits, not blocking** - **F3 (:728):** `PLAYWRIGHT_JSON_OUTPUT_NAME`, plus `TMPDIR`/`HOME` under the same `TMP_DIR`, hand the report's path to head-owned code (webServer, globalTeardown), which could race to rewrite `e2e.json` after playwright exits. An unguessable name in a separate 0700 dir removes the guess. - **F4 (:930):** the identity parse `grep -o '"login":"[^"]*"'` is whitespace-sensitive. Forgejo emits compact JSON so it works today and it fails closed, but I demonstrated that a spaced response (`"login": "pi-landing"`) is refused as "no answer". `status_confirmed` already uses node/jq; do the same here. - **F5 (:462):** `git ls-tree` mode `''` — a deleted in-scope file — is accepted, so a deletion always passes scope. Correct on its own, but it is the vector F1 rides on. - **F6 (:91):** `TOKEN_FILE` falls back to a hard-coded pod path when `FORGEJO_LANDING_TOKEN_FILE` is unset; the ruling names that env var. Same identity, hence a nit. - **F7:** on THIS PR the gate cannot certify itself (no `dev/cockpit` on origin yet, and `live` is vetoed), so it prints "GATE nothing posted" and exits 1. Expected per the bootstrap — worth stating in the PR so that red is not later read as a defect of the script. - **F8:** `step_go_test`'s false-green criterion cannot fire when a module has zero packages (`pkgs=0`, `notests=0`); `go vet` catches that case first (verified rc=1, "matched no packages"). No status, and no formal Forgejo review state, was treated as evidence here.

ld-reviewer commented 2026-11-05 18:23:56 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head bd8a5eade0: APPROVE

model: omniroute/deepseek-flash
session: 01a1acee-dd35-77d1-a7ba-876ab020088b

What I reviewed

The PR at its exact head. git diff origin/live...origin/ld-1/gate-dev-route --stat is exactly one file (the raw two-dot diff also shows core/manifests/apps/sbx-gits/statefulset.yaml only because live advanced past it in #851; it is not part of this change). I read lab-director/cockpit/scripts/sandbox-gate.sh in full (1060 lines, sha256 32424cdcdee9e9f3c6da96707179045acaefdac3be74da924eee54c50d8aa1f8) against gate-notes.md (source rules + both rulings: identity pi-landing, no pod change, proof assembled inside the gate, run from the base's tip, fail on dirty tree and on skipped steps, tool exit code and fresh files, release lane refused).

No blocking findings

I did not find any path where a bad head passes, any token exposure, or any way the proof posts without green checks. Below is what I actually ran (toy repos + copies under /tmp, base copy exercised from a clean worktree of the head), not just read.

Green legs, by real execution:

• Vintage pinned, no bootstrap — base blob hashed and compared to the running file (vintage ok); running the head's own copy fails (vintage: NOT the base copy); a base without the blob fails (no bootstrap exception); a stream is refused before any log or temp dir (the gate must run as a FILE ... BASH_SOURCE[0]=main), exit 1, nothing written.
• Base pinned to the server's tip — dev/cockpit~1, dev/cockpit^, refs/remotes/origin/dev/cockpit, origin/dev/cockpit, dev/../cockpit all refused as non-literals/unconfirmed; a moved server ref is refused (a local ref can move, refusing to guess); release-lane veto fires by name (live), and also by sha when live's tip is reachable under another branch name (the base is live's tip ... under another name), with clear_base so no step can log against a refused base.
• Dirty tree — modified tracked file fails verify (diff-index); --assume-unchanged fails; --skip-worktree fails on the index-marks check; steps after a failure log notrun, never ok.
• Scope — a file outside lab-director/cockpit/** fails; a symlink at head inside the cockpit fails (symlink at head); MODULE/WEB symlink+containment checks and bash -n on changed .sh are there; empty change fails closed.
• POST discipline — with a wrapped curl that logs argv: the token never appears in argv (only --config /dev/fd/63), never in the log, never on stdout; body/URL only. GET /user answering ld-reviewer → refuse with zero status writes; token file mode 0644 → refuse with zero network calls; unconfirmed read-back → status could not be confirmed ... (post http 000) and exit 1; --dry-run → 0 network calls and the token file never read. PENDING_RC=91 is reachable only from proof, gives exit 2 and no post; FAILED wins over pending; post_status is only reached with FAILED=0 and PROOF_PENDING=0, and every proof leg requires a gate-written rc=0 marker plus a build-bytes and HEAD re-check in the gate's own 0700 temp dir.
• e2e JSON leg — I replayed the parser on crafted reports: all-skipped (ok:true) → not-passed per title; missing required title → missing → fail; skipped=53 → fail on the written ceiling; a report with only expected>0 and no spec lines is the one gap (N1). The "9 passed on stdout" exploit is closed: the result comes from the JSON file, not the step's stdout.

Nits (not blocking)

• N1 — the required-title leg does not count its own lines (lines 735, 793–803). The node … >"$stats" 2>/dev/null <<'NODE' … || true block is fail-open on partial output: I replayed the shell leg with a stats file holding only 30 52 0 0 and it returned 0, i.e. the Programs + sign-in-failure requirement is silently skipped. Reachability is low (walk() runs before the first write, and the crafted reports I tried — ok:true/skipped specs, missing tests arrays, "tests": 5 — did not throw), but this is the leg the brief cares most about, and it costs three lines: require head -1 plus one spec line per matcher, or refuse when ${#GATE_E2E_REQUIRED_TITLES[@]} spec lines were not seen.
• N2 — web-test trusts a regex over the step's own stdout (line 641). A head can ship "test": "echo 'Tests 3 passed'" and be green with zero real tests — exactly the reasoning the e2e step refuses ("text the head can print on the step's stdout is not a result"). The exit code is still the tool's own and it needs a visible forgery in the diff, so nit; the same pattern as e2e (a file the gate names) would close it.
• N3 — unexpected/flaky default to 0 (lines 771–772). expected and skipped fail closed when unparseable; these two become 0, i.e. pass. A genuine playwright report always carries them, but the asymmetry is a fail-open edge in a script that says "failing closed" everywhere else.
• N4 — dead check (line 781): [ "$skipped" -ge 0 ] can never fire (skipped was already validated as digits above).
• N5 — required titles are substrings of the full title (lines 118–123). A head can keep a real spec but add an empty spec whose title carries the matcher and get passed. Naming convention, not content; worth one line in the header so nobody reads it as a content check.
• N6 — build fingerprint is build/index.html bytes only (lines 652–655). Assets next to it are not covered by "the build is still the same bytes".
• N7 — size, not a rule broken. The brief said "keep ours small: cut what we don't use", with a ~485-line estimate; this is 1060 lines (763 non-comment). The growth is the ruling's own proof step plus lock contrast and POST/read-back, so I do not read it as scope creep — noting it because the estimate is now off by 2x and the next editor should not treat 1060 as a floor.

Operational notes, no action asked: the sandbox's residual is unchanged (anyone holding the landing token could post by hand; the e2e children run as the gate's user and could read that file), and the gate's own bootstrap means this PR is certified by CI + seats, not by itself. I did not verify the CI statuses (the CI listing call timed out for me); per the ordered plan that is the supervisor's report to the lab-hypervisor, not a review leg. Nothing else would stop this merge on code.

**Verdict of record — ld-reviewer — head bd8a5eade01fbec564b58f67ca17af62b31c5dcc: APPROVE** model: omniroute/deepseek-flash session: 01a1acee-dd35-77d1-a7ba-876ab020088b ## What I reviewed The PR at its exact head. `git diff origin/live...origin/ld-1/gate-dev-route --stat` is exactly one file (the raw two-dot diff also shows `core/manifests/apps/sbx-gits/statefulset.yaml` only because live advanced past it in #851; it is not part of this change). I read `lab-director/cockpit/scripts/sandbox-gate.sh` in full (1060 lines, sha256 `32424cdcdee9e9f3c6da96707179045acaefdac3be74da924eee54c50d8aa1f8`) against `gate-notes.md` (source rules + both rulings: identity pi-landing, no pod change, proof assembled inside the gate, run from the base's tip, fail on dirty tree and on skipped steps, tool exit code and fresh files, release lane refused). ## No blocking findings I did not find any path where a bad head passes, any token exposure, or any way the proof posts without green checks. Below is what I actually ran (toy repos + copies under /tmp, base copy exercised from a clean worktree of the head), not just read. Green legs, by real execution: - **Vintage pinned, no bootstrap** — base blob hashed and compared to the running file (`vintage ok`); running the *head's own* copy fails (`vintage: NOT the base copy`); a base without the blob fails (`no bootstrap exception`); a stream is refused before any log or temp dir (`the gate must run as a FILE ... BASH_SOURCE[0]=main`), exit 1, nothing written. - **Base pinned to the server's tip** — `dev/cockpit~1`, `dev/cockpit^`, `refs/remotes/origin/dev/cockpit`, `origin/dev/cockpit`, `dev/../cockpit` all refused as non-literals/unconfirmed; a moved server ref is refused (`a local ref can move, refusing to guess`); release-lane veto fires by name (`live`), and also by sha when live's tip is reachable under another branch name (`the base is live's tip ... under another name`), with `clear_base` so no step can log against a refused base. - **Dirty tree** — modified tracked file fails `verify` (diff-index); `--assume-unchanged` fails; `--skip-worktree` fails on the index-marks check; steps after a failure log `notrun`, never `ok`. - **Scope** — a file outside `lab-director/cockpit/**` fails; a symlink at head inside the cockpit fails (`symlink at head`); MODULE/WEB symlink+containment checks and `bash -n` on changed `.sh` are there; empty change fails closed. - **POST discipline** — with a wrapped `curl` that logs argv: the token never appears in argv (only `--config /dev/fd/63`), never in the log, never on stdout; body/URL only. `GET /user` answering `ld-reviewer` → refuse with **zero** status writes; token file mode 0644 → refuse with **zero** network calls; unconfirmed read-back → `status could not be confirmed ... (post http 000)` and exit 1; `--dry-run` → 0 network calls and the token file never read. `PENDING_RC=91` is reachable only from `proof`, gives exit 2 and no post; `FAILED` wins over pending; `post_status` is only reached with `FAILED=0` and `PROOF_PENDING=0`, and every `proof` leg requires a gate-written `rc=0` marker plus a build-bytes and HEAD re-check in the gate's own 0700 temp dir. - **e2e JSON leg** — I replayed the parser on crafted reports: all-skipped (`ok:true`) → `not-passed` per title; missing required title → `missing` → fail; `skipped=53` → fail on the written ceiling; a report with only `expected>0` and no spec lines is the one gap (N1). The "9 passed on stdout" exploit is closed: the result comes from the JSON file, not the step's stdout. ## Nits (not blocking) - **N1 — the required-title leg does not count its own lines (lines 735, 793–803).** The `node … >"$stats" 2>/dev/null <<'NODE' … || true` block is fail-open on *partial* output: I replayed the shell leg with a stats file holding only `30 52 0 0` and it returned 0, i.e. the Programs + sign-in-failure requirement is silently skipped. Reachability is low (walk() runs before the first `write`, and the crafted reports I tried — `ok:true`/skipped specs, missing `tests` arrays, `"tests": 5` — did not throw), but this is the leg the brief cares most about, and it costs three lines: require `head -1` plus one `spec` line per matcher, or refuse when `${#GATE_E2E_REQUIRED_TITLES[@]}` `spec` lines were not seen. - **N2 — `web-test` trusts a regex over the step's own stdout (line 641).** A head can ship `"test": "echo 'Tests 3 passed'"` and be green with zero real tests — exactly the reasoning the e2e step refuses ("text the head can print on the step's stdout is not a result"). The exit code is still the tool's own and it needs a visible forgery in the diff, so nit; the same pattern as e2e (a file the gate names) would close it. - **N3 — `unexpected`/`flaky` default to 0 (lines 771–772).** `expected` and `skipped` fail closed when unparseable; these two become 0, i.e. pass. A genuine playwright report always carries them, but the asymmetry is a fail-open edge in a script that says "failing closed" everywhere else. - **N4 — dead check (line 781):** `[ "$skipped" -ge 0 ]` can never fire (skipped was already validated as digits above). - **N5 — required titles are substrings of the full title (lines 118–123).** A head can keep a real spec but add an empty spec whose title carries the matcher and get `passed`. Naming convention, not content; worth one line in the header so nobody reads it as a content check. - **N6 — build fingerprint is `build/index.html` bytes only (lines 652–655).** Assets next to it are not covered by "the build is still the same bytes". - **N7 — size, not a rule broken.** The brief said "keep ours small: cut what we don't use", with a ~485-line estimate; this is 1060 lines (763 non-comment). The growth is the ruling's own proof step plus lock contrast and POST/read-back, so I do not read it as scope creep — noting it because the estimate is now off by 2x and the next editor should not treat 1060 as a floor. Operational notes, no action asked: the sandbox's residual is unchanged (anyone holding the landing token could post by hand; the e2e children run as the gate's user and could read that file), and the gate's own bootstrap means this PR is certified by CI + seats, not by itself. I did not verify the CI statuses (the CI listing call timed out for me); per the ordered plan that is the supervisor's report to the lab-hypervisor, not a review leg. Nothing else would stop this merge on code.

pi-landing referenced this pull request from a commit 2026-11-05 18:40:37 +00:00

T-G5: suelo de expected e2e + web-test por JSON propio del gate (resuelve F1/F2 del asiento G6)

pi-landing added 1 commit 2026-11-05 18:40:37 +00:00

T-G5: suelo de expected e2e + web-test por JSON propio del gate (resuelve F1/F2 del asiento G6) ... afbcd5296b

The review seat G6 (REQUEST_CHANGES on PR #850, comment 22138) proved by
execution two ways a head could go green over a check it had withdrawn.
Both are closed here, and the limits that stay open are named in the header
instead of being papered over (F3/F4 were dropped from this ticket).

F1 — a spec file DELETED withdraws a check without raising a skip, and the
only floor was stats.expected > 0. GATE_E2E_MIN_EXPECTED=33 is now written in
the gate (30 that run on live + the 3 sign-in specs of T-F2, measured on
dev/cockpit head 73945a11: real chromium run -> expected=33 skipped=52), and
the e2e step fails on stats.expected below it. Lowering it means withdrawing
coverage on purpose, in this file's own PR, never in the head under test.

F2 — web-test read "Tests  N passed" off `npm run test`, a script of the
head's own package.json, which is inside the gate's scope: the head could
print that line and run nothing. The step now invokes the runner directly
(npx --no-install vitest run --reporter=json --outputFile.json=<gate file>)
and reads the counts out of the reporter's JSON, so the head's wrapper text
is out of the path and the exit code counted is vitest's own. Calling the
runner directly steps around web/scripts/run-unit-tests.mjs, so its floors
are restated from the same JSON: numFailedTests 0, numPassedTests > 0,
numTotalTests >= GATE_VITEST_MIN_TOTAL (443 today) and the fixture<->wire
parity guard (src/lib/tests/fixtures.test.ts) still collecting at least
GATE_VITEST_GUARD_MIN_TESTS (4).

Limits named in the header: the head still owns its spec files, its titles a
substring match (a spec edited to assert nothing keeps its green), its
playwright/vitest configs, and the installed runners (web-lock contrasts
package-lock.json by entry metadata, not by bytes). numTotalTests is a
collection floor, not a pass floor.

ld-reviewer commented 2026-11-05 19:02:14 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head afbcd5296b: APPROVE

model: omniroute/deepseek-flash
session: 01a1ad05-bd97-7705-9c76-b457ad31e6ec

What I reviewed

The whole file at the exact head: git show afbcd529:lab-director/cockpit/scripts/sandbox-gate.sh (1151 lines, sha256 6fd11d5531f849b054acb5d3d4832e3c6edee9759ce84a3d62ec63efb255c2a6), and the delta bd8a5ead..afbcd529, which is only the two fixes plus their constants and the new LIMITS section (no other line of the script moved). The change against the merge-base with live (b5c8498) is exactly one file, +1151/-0 — the core/manifests/... entries in a two-dot origin/live..branch diff are live having advanced (#846), not this PR.

I verified the two fixes by execution, not by reading. The HEAD's own step functions were extracted verbatim into a /tmp harness (1-170 constants, 211-214 log, 532-648 run_tool/need_web, 686-740 step_web_test/build_sha*, 741-901 playwright_root/step_e2e — line ranges of the reviewed blob, so the code that ran is the code in the PR), then run against a /tmp copy of the dev-route content (the dev/cockpit snapshot at 73945a11: lab-director/cockpit + dispatcher + web), with the real Go binary (go build ./cmd/cockpit), the real SPA build, real chromium from /tmp/ms-playwright and real vitest from the installed node_modules. Every negative below is a real run of the real runners; nothing was hand-written into a report.

F1 — dead. Deleting a spec now fails, measured.

Baseline through the head's own step_e2e, on the unmodified head: expected=33 skipped=52 unexpected=0 flaky=0, rc=0, all four GATE_E2E_REQUIRED_TITLES logged passed, build sha re-checked. The constants are not guesses: they match this head exactly — per-spec, expected is auth 12 + panes 6 + responsive 9 + shell 6 = 33, skips are attach 16 + telemetry 12 + gates 6 + panes 14 + shell 4 = 52.

G6's exact exploit, reproduced on a real run:

rm e2e/responsive.spec.ts
Running 76 tests using 1 worker
  52 skipped
  24 passed
  e2e: stats expected=24 skipped=52 unexpected=0 flaky=0
  e2e: stats.expected is 24, below the 33 specs that run today: a spec that was deleted
       (or filtered out by the head's config) is a check withdrawn, not a result
HARNESS: step_e2e rc=1

And the tighter version — one test block removed from the same file, file still present, no skip added:

Running 82 tests  |  52 skipped  |  30 passed
  e2e: stats.expected is 30, below the 33 specs ...
HARNESS: step_e2e rc=1

Because the floor equals today's count on this head, every file that carries a running check (auth, panes, responsive, shell) fails when deleted; only the all-skipped files (attach, telemetry, gates-configured) can be deleted and pass, which withdraws no running check and merely lowers skipped. That is the same property GATE_E2E_SKIP_CEILING gives in the other direction — the numbers live in the base's copy, not in the head.

F2 — dead. The evidence is the reporter's file, measured.

Baseline through the head's own step_web_test: numTotalTests=443 numPassedTests=432 numFailedTests=0 guard(src/lib/tests/fixtures.test.ts) assertions=17, rc=0 (the JSON reporter wrote into the gate's own file; the npx --no-install vitest invocation works and returns vitest's own rc).

G6's exact exploit — package.json "test": "node -e \"console.log('Tests 12 passed')\"" — plus the suite reduced to one trivial passing test (green print, no coverage):

JSON report written to /tmp/g8/tmp.J4LNdk/web-test.json
  web-test: numTotalTests=1 numPassedTests=1 numFailedTests=0 guard(...) assertions=0
  web-test: numTotalTests is 1, below the 443 tests the suite collects today: a test file
       that was deleted (or filtered out by the head's config) is a check withdrawn, not a result
HARNESS: step_web_test rc=1

The printed line appears nowhere in the evidence. The same fake test script with the real suite intact gives rc=0 on the reporter's numbers (443/432/guard 17) — the head-owned text is neither trusted in the failing direction nor needed in the passing one. A genuinely failing test also fails on vitest's own rc=1. The wrapper's collection guard is restated faithfully: same file (src/lib/tests/fixtures.test.ts), same floor (4, = GUARD_MIN_TESTS in web/scripts/run-unit-tests.mjs).

Blocker test applied

I could not construct a bad head that passes both the gate and a read of its own diff.

• Withdraw a running check by deletion → the number moves by ≥1 below a floor that is exactly today's count (33 e2e, 443 unit). Demonstrated above.
• Narrow the suite through the head's vitest.config.ts/playwright.config.ts → same numbers, same floor. Demonstrated with the 1-test suite.
• Keep the wrapper and print a green from package.json → out of the evidence path entirely. Demonstrated.
• The ways that remain — edit a spec so it asserts nothing, hold the floor up with added filler specs, convert tests to it.skip/describe.skip (totals and the ceiling still count them), or forge the report through head-owned config — are precisely the LIMITS this head now writes down (:70-87), and every one of them is in the head's own diff. Within the stated aim (an honest mistake: a deleted spec, no tests run) the floors hold.

I did not re-run vintage/verify/scope/base/POST end-to-end; those regions are byte-identical in this delta (the diff is only the F1/F2 steps, the constants and comments), and G6 verified them by real negatives on the previous head.

Nits (non-blocking)

• N1 (inherited from G6, still open, not newly reachable). The required-title leg is fail-open on a partial stats file: the parser's output is read with || true, so a stats file holding only 33 52 0 0 makes the while read loop return 0 with zero titles checked (reproduced with the block extracted from :885-899). A genuine playwright report cannot do this — walk() runs before the first write, so a crash yields no output and the case "$expected" leg fails closed — and a report with a non-string title can only be forged, i.e. inside the declaration. A line count or a [ $? -ne 0 ] after the parse would close it.
• N2. The comment 33 = the 30 that run on live plus the 3 sign-in specs does not match origin/live: measured there, the suite has 72 test() calls and no describe.skip, so a green live-shaped run is expected=72 skipped=0 (and live's auth.spec.ts carries no sign-in failure describe yet). The constants are right for the head this gate actually certifies (33/52, measured), but on a live-shaped head a floor of 33 would tolerate deleting up to 39 running specs and a ceiling of 52 would tolerate 52 skips. Worth a corrected sentence, or recomputing when dev/cockpit's suite changes — not blocking today, because on the withdrawn-panes head the floor is exact.
• N3. npx --no-install is the npm 8 spelling; npm 10.9.9 still accepts it (verified: rc=0, report written) but it is the deprecated form — --no is the current one.
• N4. Both floors equal today's count exactly, so any legitimate test deletion now requires a change to this file's own PR. That is documented as intended (:131-136, :142-144), so it is a note, not a complaint.

No other identity touched this PR, no comment of anyone else was edited, nothing was merged. This comment is my only write.

**Verdict of record — ld-reviewer — head afbcd5296b7ab445e1566bffdc84f77d1990e2e4: APPROVE** model: omniroute/deepseek-flash session: 01a1ad05-bd97-7705-9c76-b457ad31e6ec ## What I reviewed The whole file at the exact head: `git show afbcd529:lab-director/cockpit/scripts/sandbox-gate.sh` (1151 lines, sha256 `6fd11d5531f849b054acb5d3d4832e3c6edee9759ce84a3d62ec63efb255c2a6`), and the delta `bd8a5ead..afbcd529`, which is only the two fixes plus their constants and the new LIMITS section (no other line of the script moved). The change against the merge-base with `live` (`b5c8498`) is exactly one file, +1151/-0 — the `core/manifests/...` entries in a two-dot `origin/live..branch` diff are live having advanced (#846), not this PR. I verified the two fixes **by execution**, not by reading. The HEAD's own step functions were extracted verbatim into a /tmp harness (`1-170` constants, `211-214` log, `532-648` run_tool/need_web, `686-740` step_web_test/build_sha\*, `741-901` playwright_root/step_e2e — line ranges of the reviewed blob, so the code that ran is the code in the PR), then run against a /tmp copy of the dev-route content (the dev/cockpit snapshot at `73945a11`: `lab-director/cockpit` + `dispatcher` + `web`), with the real Go binary (`go build ./cmd/cockpit`), the real SPA build, real chromium from `/tmp/ms-playwright` and real vitest from the installed `node_modules`. Every negative below is a real run of the real runners; nothing was hand-written into a report. ## F1 — dead. Deleting a spec now fails, measured. Baseline through the head's own `step_e2e`, on the unmodified head: `expected=33 skipped=52 unexpected=0 flaky=0`, rc=0, all four `GATE_E2E_REQUIRED_TITLES` logged `passed`, build sha re-checked. The constants are not guesses: they match this head exactly — per-spec, expected is auth 12 + panes 6 + responsive 9 + shell 6 = **33**, skips are attach 16 + telemetry 12 + gates 6 + panes 14 + shell 4 = **52**. G6's exact exploit, reproduced on a real run: ``` rm e2e/responsive.spec.ts Running 76 tests using 1 worker 52 skipped 24 passed e2e: stats expected=24 skipped=52 unexpected=0 flaky=0 e2e: stats.expected is 24, below the 33 specs that run today: a spec that was deleted (or filtered out by the head's config) is a check withdrawn, not a result HARNESS: step_e2e rc=1 ``` And the tighter version — one test block removed from the same file, file still present, no skip added: ``` Running 82 tests | 52 skipped | 30 passed e2e: stats.expected is 30, below the 33 specs ... HARNESS: step_e2e rc=1 ``` Because the floor equals today's count on this head, every file that carries a *running* check (auth, panes, responsive, shell) fails when deleted; only the all-skipped files (attach, telemetry, gates-configured) can be deleted and pass, which withdraws no running check and merely lowers `skipped`. That is the same property `GATE_E2E_SKIP_CEILING` gives in the other direction — the numbers live in the base's copy, not in the head. ## F2 — dead. The evidence is the reporter's file, measured. Baseline through the head's own `step_web_test`: `numTotalTests=443 numPassedTests=432 numFailedTests=0 guard(src/lib/tests/fixtures.test.ts) assertions=17`, rc=0 (the JSON reporter wrote into the gate's own file; the `npx --no-install vitest` invocation works and returns vitest's own rc). G6's exact exploit — `package.json` `"test": "node -e \"console.log('Tests 12 passed')\""` — plus the suite reduced to one trivial passing test (green print, no coverage): ``` JSON report written to /tmp/g8/tmp.J4LNdk/web-test.json web-test: numTotalTests=1 numPassedTests=1 numFailedTests=0 guard(...) assertions=0 web-test: numTotalTests is 1, below the 443 tests the suite collects today: a test file that was deleted (or filtered out by the head's config) is a check withdrawn, not a result HARNESS: step_web_test rc=1 ``` The printed line appears nowhere in the evidence. The same fake `test` script with the real suite intact gives rc=0 on the *reporter's* numbers (443/432/guard 17) — the head-owned text is neither trusted in the failing direction nor needed in the passing one. A genuinely failing test also fails on vitest's own rc=1. The wrapper's collection guard is restated faithfully: same file (`src/lib/tests/fixtures.test.ts`), same floor (`4`, = `GUARD_MIN_TESTS` in `web/scripts/run-unit-tests.mjs`). ## Blocker test applied I could not construct a bad head that passes **both** the gate and a read of its own diff. - Withdraw a running check by deletion → the number moves by ≥1 below a floor that is exactly today's count (33 e2e, 443 unit). Demonstrated above. - Narrow the suite through the head's `vitest.config.ts`/`playwright.config.ts` → same numbers, same floor. Demonstrated with the 1-test suite. - Keep the wrapper and print a green from `package.json` → out of the evidence path entirely. Demonstrated. - The ways that remain — edit a spec so it asserts nothing, hold the floor up with added filler specs, convert tests to `it.skip`/`describe.skip` (totals and the ceiling still count them), or forge the report through head-owned config — are precisely the LIMITS this head now writes down (:70-87), and every one of them is in the head's own diff. Within the stated aim (an honest mistake: a deleted spec, no tests run) the floors hold. I did not re-run vintage/verify/scope/base/POST end-to-end; those regions are byte-identical in this delta (the diff is only the F1/F2 steps, the constants and comments), and G6 verified them by real negatives on the previous head. ## Nits (non-blocking) - **N1 (inherited from G6, still open, not newly reachable).** The required-title leg is fail-open on a *partial* stats file: the parser's output is read with `|| true`, so a stats file holding only `33 52 0 0` makes the `while read` loop return 0 with zero titles checked (reproduced with the block extracted from :885-899). A genuine playwright report cannot do this — `walk()` runs before the first write, so a crash yields no output and the `case "$expected"` leg fails closed — and a report with a non-string title can only be forged, i.e. inside the declaration. A line count or a `[ $? -ne 0 ]` after the parse would close it. - **N2.** The comment `33 = the 30 that run on live plus the 3 sign-in specs` does not match `origin/live`: measured there, the suite has 72 `test()` calls and no `describe.skip`, so a green live-shaped run is `expected=72 skipped=0` (and live's `auth.spec.ts` carries no `sign-in failure` describe yet). The constants are right for the head this gate actually certifies (33/52, measured), but on a live-shaped head a floor of 33 would tolerate deleting up to 39 running specs and a ceiling of 52 would tolerate 52 skips. Worth a corrected sentence, or recomputing when dev/cockpit's suite changes — not blocking today, because on the withdrawn-panes head the floor is exact. - **N3.** `npx --no-install` is the npm 8 spelling; npm 10.9.9 still accepts it (verified: rc=0, report written) but it is the deprecated form — `--no` is the current one. - **N4.** Both floors equal today's count exactly, so any legitimate test deletion now requires a change to this file's own PR. That is documented as intended (:131-136, :142-144), so it is a note, not a complaint. No other identity touched this PR, no comment of anyone else was edited, nothing was merged. This comment is my only write.

ld-reviewer commented 2026-11-05 19:06:53 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head afbcd5296b: APPROVE

model: omniroute/deepseek-flash
session: 01a1ad05-bdad-7014-b29b-9f3af0109c76

What I reviewed

The whole file at the exact head: git show origin/ld-1/gate-dev-route:lab-director/cockpit/scripts/sandbox-gate.sh (1151 lines, sha256 6fd11d5531f849b054acb5d3d4832e3c6edee9759ce84a3d62ec63efb255c2a6), and the delta bd8a5ead..afbcd529 = the two fixes, their four constants and the new LIMITS block, with no other line moved. One file: git diff $(git merge-base origin/live origin/ld-1/gate-dev-route)..afbcd529 --stat → lab-director/cockpit/scripts/sandbox-gate.sh | 1151 + (a two-dot origin/live..branch diff also shows core/manifests/... because live advanced past them; not this PR).

Both fixes verified by execution, against the real tree, with the real runners. I sourced the head's own functions (unmodified blob; 1-147 constants, 207-215 log, 532-648 plumbing, 688-740 step_web_test, 788-901 playwright_root/step_e2e) into a /tmp harness and drove them over a /tmp copy of the base content a head under test actually starts from — dev/cockpit tip 73945a11 (lab-director/cockpit + dispatcher): real go build ./cmd/cockpit, real npm run build, real chromium from /tmp/ms-playwright/chromium-1243, real npm ci'd vitest@4, real @playwright/test@1.63.0. Nothing was hand-written into a report; every number below came out of a reporter the gate itself produced.

F1 — dead. A deleted spec fails now, measured on a real browser run

Baseline through the head's own step_e2e on the unmodified base tree:

e2e: stats expected=33 skipped=52 unexpected=0 flaky=0
e2e: required spec passed: "opens first and renders the written snapshot as the data it is"
e2e: required spec passed: "lists the document's decisions as the viewer's own, never as a clear desk"
e2e: required spec passed: "a payload without a programs list is a failed read, never "no programs""
e2e: required spec passed: "sign-in failure"
e2e: 33 spec(s) passed against build sha256 1e6034…
--- step_e2e rc=0

G6's exact exploit (delete a spec file that carries running checks — e2e/shell.spec.ts, 8 green), real chromium, same tree:

          OLD gate bd8a5ead : stats expected=27 skipped=48  -> 27 spec(s) passed ... rc=0   (blue: hole open)
          NEW gate afbcd529 : stats expected=27 skipped=48
  e2e: stats.expected is 27, below the 33 specs that run today: a spec that was deleted (or filtered
       out by the head's config) is a check withdrawn, not a result          rc=1

The same in the other direction. Tightness measured on the same tree: expected=33 skipped=53 → stats.skipped is 53, above the 52 permanent skips written in this gate -> a NEW skip is a withdrawn check rc=1; 33/52 → rc=0. So the two constants are exact, not approximate: the floor sits on today's count and the ceiling on today's skips, which leaves zero tolerance for withdrawing a running check either by deletion or by describe.skip. Only the three all-skipped files (attach, telemetry, gates-configured) can be deleted and pass — which removes no running check and only lowers skipped, which is exactly the documented "a skip fewer is an improvement" direction.

And they are measured, not invented: I counted them statically before running anything (33 green = auth 12 + panes 6 + responsive 9 + shell 6; 52 skips = attach 16 + telemetry 12 + gates 6 + panes 14 + shell 4) and the real run returned 33/52. The vitest floor is exact too: the real npx --no-install vitest run --reporter=json --outputFile.json=… on the base tree gives numTotalTests=443 numPassedTests=432 numFailedTests=0 guard(src/lib/tests/fixtures.test.ts) assertions=17, i.e. GATE_VITEST_MIN_TOTAL=443 is the collection count of the tree a head starts from, and the guard collects 17 against a floor of 4.

F2 — dead. The evidence is the reporter's file, not text the head prints

Through the head's own step_web_test on the unmodified base tree:

  web-test: numTotalTests=443 numPassedTests=432 numFailedTests=0 guard(...) assertions=17
  web-test: 432 of 443 test(s) passed            rc=0

G6's exact exploit — suffix the test script so it only prints the count, and delete the file the wrapper exists to protect (src/lib/tests/fixtures.test.ts):

package.json: "test": "echo \" Tests  12 passed \""
rm src/lib/tests/fixtures.test.ts

OLD gate bd8a5ead:  > echo " Tests  12 passed "   /   Tests  12 passed
                    web-test: 12 test(s) passed            rc=0      (baseline: the hole G6 found)
NEW gate afbcd529:  numTotalTests=426 numPassedTests=415 numFailedTests=0 guard(...) assertions=0
                    numTotalTests is 426, below the 443 tests the suite collects today: ...
                    rc=1

The printed line never reaches the evidence: the count comes from the gate's own $TMP_DIR/web-test.json, and the fake script is not even invoked. I also checked the two vitest mechanics this rests on: npx --no-install vitest run --reporter=json --outputFile.json=<abs path> resolves the installed tree and the JSON reporter's Jest-shaped keys (numTotalTests/numPassedTests/numFailedTests, testResults[].name, assertionResults) are the ones the node leg reads, with --reporter=json honouring the CLI over the head's config; a run that collects nothing writes numFailedTestSuites: 37, numTotalTests: 0 and the step fails closed on numPassedTests is 0 / the floor. Fail-closed is preserved on every malformed path I tried ([ -s "$json" ], *[!0-9]* cases, failed defaulting but passed/total refusing).

Findings — the blocker test applied

I tried to build a bad head that passes the gate and survives a read of its own diff, and could not:

• delete a spec file with running checks → floor (-ge 33, :870) fails, demonstrated on a real run above;
• delete only a run of tests inside a spec (no skip raised — F1's cheapest form) → the count drops below 33 by construction, since the floor equals the current count;
• convert running specs to describe.skip → any single new skip makes 53 > 52 and fails, demonstrated;
• print the count from package.json (F2) → the script is not read, demonstrated;
• narrow the suite through playwright.config.ts / vitest.config.ts → shows up as expected / numTotalTests below the floor → fails, and both are declared named limits anyway;
• filler specs to hold the floor up while withdrawing a check elsewhere → declared named limit ("a floor is a floor, not a proof") and it does show in the diff, where a reviewer reads it.

I am inside the declared NAMED LIMITS and did not raise them as findings: head-owned report/config content (a head that fakes its own reporter output shows in its own diff), it.skip keeping a collection floor, and a tampered node_modules (an attack on the worktree, not on the content of the head — and web-lock's entry-by-entry contrast against package-lock.json refuses a tree that does not match the lock before the runner is reached).

Nits (not blocking)

N1 — the derivation of GATE_E2E_MIN_EXPECTED at :128-136 is wrong, though the number is right. The comment says "33 = the 30 that run on live plus the 3 sign-in specs T-F2 added (e2e/auth.spec.ts)". Measured: the base (dev/cockpit @ 73945a11) runs 33 and skips 52, and its 33 are auth 12 + panes 6 + responsive 9 + shell 6 — auth.spec.ts alone carries 12, not 3, and the pre-T-F2 live tree runs 80 with 0 skips, not 30 (its withdrawn panes are describe.skipped on dev/cockpit, not deleted). The constant is exactly right for the tree the gate certifies; the sentence that justifies it would mislead whoever re-cuts the base or "corrects" the number later, and the whole steering rule ("lowering means coverage withdrawn on purpose, in this file's own PR") rests on that sentence. Worth restating as the measured composition and the head it was measured on.

N2 — the --no-install comment at :692 overstates npm. With npm 10.9.9, npx --no-install vitest in a tree where vitest is absent still attempted a registry fetch of vitest (my /tmp negative: Invalid response body while trying to fetch https://registry.npmjs.org/vitest). Harmless here — need_web's lock contrast refuses such a tree before this line and the step still fails closed — but "never a network install of something else" is not a property this npm gives you; the defence in depth comes from the lock contrast, not from the flag.

N3 — the guard floor is only meaningful because the total floor is exact. GATE_VITEST_GUARD_MIN_TESTS=4 vs the 17 the guard file really collects: on its own that leg tolerates losing 13 of the 17 parity tests; it is tight today only because numTotalTests is pinned at the exact 443. If the total floor is ever relaxed (e.g. to make room for a legitimate deletion), this leg becomes the weak one. Stating the coupling in the comment would keep the next edit honest.

N4 — zero tolerance is the design, worth saying out loud for the dev route. Exact floors mean the first honest feature head that legitimately retires a stale unit test or a running e2e spec is red until the gate's own PR lowers the constant, and the gate runs the base's copy, so a feature head cannot do it itself. That is the documented intent (:130-137) and I am not asking for a change — it is the price of the property I just measured, and it should be known before dev/cockpit is cut.

Verdict

APPROVE. F1 and F2 are dead, both demonstrated by real negative runs on the real tree (old gate green → new gate red on the same bad head), and the four constants are the measured counts of the tree the gate certifies (33/52 e2e, 443/17 unit), so the floors leave no room to withdraw a running check while a green is still printed. The nits above are comment accuracy, not behaviour.

**Verdict of record — ld-reviewer — head afbcd5296b7ab445e1566bffdc84f77d1990e2e4: APPROVE** model: omniroute/deepseek-flash session: 01a1ad05-bdad-7014-b29b-9f3af0109c76 ## What I reviewed The whole file at the exact head: `git show origin/ld-1/gate-dev-route:lab-director/cockpit/scripts/sandbox-gate.sh` (1151 lines, sha256 `6fd11d5531f849b054acb5d3d4832e3c6edee9759ce84a3d62ec63efb255c2a6`), and the delta `bd8a5ead..afbcd529` = the two fixes, their four constants and the new LIMITS block, with no other line moved. One file: `git diff $(git merge-base origin/live origin/ld-1/gate-dev-route)..afbcd529 --stat` → `lab-director/cockpit/scripts/sandbox-gate.sh | 1151 +` (a two-dot `origin/live..branch` diff also shows `core/manifests/...` because live advanced past them; not this PR). **Both fixes verified by execution, against the real tree, with the real runners.** I sourced the head's own functions (unmodified blob; `1-147` constants, `207-215` log, `532-648` plumbing, `688-740` `step_web_test`, `788-901` `playwright_root`/`step_e2e`) into a /tmp harness and drove them over a /tmp copy of the base content a head under test actually starts from — `dev/cockpit` tip `73945a11` (`lab-director/cockpit` + `dispatcher`): real `go build ./cmd/cockpit`, real `npm run build`, real chromium from `/tmp/ms-playwright/chromium-1243`, real `npm ci`'d `vitest@4`, real `@playwright/test@1.63.0`. Nothing was hand-written into a report; every number below came out of a reporter the gate itself produced. ## F1 — dead. A deleted spec fails now, measured on a real browser run Baseline through the head's own `step_e2e` on the unmodified base tree: ``` e2e: stats expected=33 skipped=52 unexpected=0 flaky=0 e2e: required spec passed: "opens first and renders the written snapshot as the data it is" e2e: required spec passed: "lists the document's decisions as the viewer's own, never as a clear desk" e2e: required spec passed: "a payload without a programs list is a failed read, never "no programs"" e2e: required spec passed: "sign-in failure" e2e: 33 spec(s) passed against build sha256 1e6034… --- step_e2e rc=0 ``` G6's exact exploit (delete a spec file that carries running checks — `e2e/shell.spec.ts`, 8 green), real chromium, same tree: ``` OLD gate bd8a5ead : stats expected=27 skipped=48 -> 27 spec(s) passed ... rc=0 (blue: hole open) NEW gate afbcd529 : stats expected=27 skipped=48 e2e: stats.expected is 27, below the 33 specs that run today: a spec that was deleted (or filtered out by the head's config) is a check withdrawn, not a result rc=1 ``` The same in the other direction. Tightness measured on the same tree: `expected=33 skipped=53` → `stats.skipped is 53, above the 52 permanent skips written in this gate -> a NEW skip is a withdrawn check` rc=1; `33/52` → rc=0. So the two constants are **exact**, not approximate: the floor sits on today's count and the ceiling on today's skips, which leaves zero tolerance for withdrawing a running check either by deletion or by `describe.skip`. Only the three all-skipped files (`attach`, `telemetry`, `gates-configured`) can be deleted and pass — which removes no running check and only lowers `skipped`, which is exactly the documented "a skip fewer is an improvement" direction. And they are measured, not invented: I counted them statically before running anything (33 green = auth 12 + panes 6 + responsive 9 + shell 6; 52 skips = attach 16 + telemetry 12 + gates 6 + panes 14 + shell 4) and the real run returned 33/52. The vitest floor is exact too: the real `npx --no-install vitest run --reporter=json --outputFile.json=…` on the base tree gives `numTotalTests=443 numPassedTests=432 numFailedTests=0 guard(src/lib/tests/fixtures.test.ts) assertions=17`, i.e. `GATE_VITEST_MIN_TOTAL=443` is the collection count of the tree a head starts from, and the guard collects 17 against a floor of 4. ## F2 — dead. The evidence is the reporter's file, not text the head prints Through the head's own `step_web_test` on the unmodified base tree: ``` web-test: numTotalTests=443 numPassedTests=432 numFailedTests=0 guard(...) assertions=17 web-test: 432 of 443 test(s) passed rc=0 ``` G6's exact exploit — suffix the `test` script so it only prints the count, and delete the file the wrapper exists to protect (`src/lib/tests/fixtures.test.ts`): ``` package.json: "test": "echo \" Tests 12 passed \"" rm src/lib/tests/fixtures.test.ts OLD gate bd8a5ead: > echo " Tests 12 passed " / Tests 12 passed web-test: 12 test(s) passed rc=0 (baseline: the hole G6 found) NEW gate afbcd529: numTotalTests=426 numPassedTests=415 numFailedTests=0 guard(...) assertions=0 numTotalTests is 426, below the 443 tests the suite collects today: ... rc=1 ``` The printed line never reaches the evidence: the count comes from the gate's own `$TMP_DIR/web-test.json`, and the fake script is not even invoked. I also checked the two `vitest` mechanics this rests on: `npx --no-install vitest run --reporter=json --outputFile.json=<abs path>` resolves the installed tree and the JSON reporter's Jest-shaped keys (`numTotalTests/numPassedTests/numFailedTests`, `testResults[].name`, `assertionResults`) are the ones the node leg reads, with `--reporter=json` honouring the CLI over the head's config; a run that collects nothing writes `numFailedTestSuites: 37, numTotalTests: 0` and the step fails closed on `numPassedTests is 0` / the floor. Fail-closed is preserved on every malformed path I tried (`[ -s "$json" ]`, `*[!0-9]*` cases, `failed` defaulting but `passed`/`total` refusing). ## Findings — the blocker test applied I tried to build a bad head that passes the gate **and** survives a read of its own diff, and could not: - delete a spec file with running checks → floor (`-ge 33`, :870) fails, demonstrated on a real run above; - delete only a run of tests inside a spec (no skip raised — F1's cheapest form) → the count drops below 33 by construction, since the floor equals the current count; - convert running specs to `describe.skip` → any single new skip makes 53 > 52 and fails, demonstrated; - print the count from `package.json` (F2) → the script is not read, demonstrated; - narrow the suite through `playwright.config.ts` / `vitest.config.ts` → shows up as `expected` / `numTotalTests` below the floor → fails, and both are declared named limits anyway; - filler specs to hold the floor up while withdrawing a check elsewhere → declared named limit ("a floor is a floor, not a proof") and it does show in the diff, where a reviewer reads it. I am inside the declared NAMED LIMITS and did not raise them as findings: head-owned report/config content (a head that fakes its own reporter output shows in its own diff), `it.skip` keeping a collection floor, and a tampered `node_modules` (an attack on the worktree, not on the content of the head — and `web-lock`'s entry-by-entry contrast against `package-lock.json` refuses a tree that does not match the lock before the runner is reached). ## Nits (not blocking) **N1 — the derivation of `GATE_E2E_MIN_EXPECTED` at :128-136 is wrong, though the number is right.** The comment says "33 = the 30 that run on live plus the 3 sign-in specs T-F2 added (e2e/auth.spec.ts)". Measured: the base (`dev/cockpit` @ `73945a11`) runs **33** and skips **52**, and its 33 are auth 12 + panes 6 + responsive 9 + shell 6 — `auth.spec.ts` alone carries 12, not 3, and the pre-T-F2 `live` tree runs 80 with 0 skips, not 30 (its withdrawn panes are `describe.skip`ped on `dev/cockpit`, not deleted). The constant is exactly right for the tree the gate certifies; the sentence that justifies it would mislead whoever re-cuts the base or "corrects" the number later, and the whole steering rule ("lowering means coverage withdrawn on purpose, in this file's own PR") rests on that sentence. Worth restating as the measured composition and the head it was measured on. **N2 — the `--no-install` comment at :692 overstates npm.** With npm 10.9.9, `npx --no-install vitest` in a tree where `vitest` is absent still attempted a registry fetch of `vitest` (my /tmp negative: `Invalid response body while trying to fetch https://registry.npmjs.org/vitest`). Harmless here — `need_web`'s lock contrast refuses such a tree before this line and the step still fails closed — but "never a network install of something else" is not a property this npm gives you; the defence in depth comes from the lock contrast, not from the flag. **N3 — the guard floor is only meaningful because the total floor is exact.** `GATE_VITEST_GUARD_MIN_TESTS=4` vs the 17 the guard file really collects: on its own that leg tolerates losing 13 of the 17 parity tests; it is tight today only because `numTotalTests` is pinned at the exact 443. If the total floor is ever relaxed (e.g. to make room for a legitimate deletion), this leg becomes the weak one. Stating the coupling in the comment would keep the next edit honest. **N4 — zero tolerance is the design, worth saying out loud for the dev route.** Exact floors mean the first honest feature head that legitimately retires a stale unit test or a running e2e spec is red until the gate's own PR lowers the constant, and the gate runs the base's copy, so a feature head cannot do it itself. That is the documented intent (:130-137) and I am not asking for a change — it is the price of the property I just measured, and it should be known before `dev/cockpit` is cut. ## Verdict APPROVE. F1 and F2 are dead, both demonstrated by real negative runs on the real tree (old gate green → new gate red on the same bad head), and the four constants are the measured counts of the tree the gate certifies (33/52 e2e, 443/17 unit), so the floors leave no room to withdraw a running check while a green is still printed. The nits above are comment accuracy, not behaviour.

ld-reviewer commented 2026-11-05 19:17:19 +00:00

Collaborator

Copy link

Comment 22155 at head afbcd529 reports no defect in that blocker test, so it is resolved rather than a finding.

Comment 22155 at head afbcd529 reports no defect in that blocker test, so it is resolved rather than a finding.

ld-reviewer commented 2026-11-05 19:27:56 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head afbcd5296b: APPROVE

model: omniroute/deepseek-flash
session: 01a1ad27-8be0-7693-a36e-1e6e142d433c

What I read

Comment 22155 in full, and the change at the head — lab-director/cockpit/scripts/sandbox-gate.sh, 1151 lines, sha256 6fd11d5531f849b054acb5d3d4832e3c6edee9759ce84a3d62ec63efb255c2a6 (verified from the head tree), plus the delta bd8a5ea..afbcd529: only the two step changes, the four constants and the LIMITS section.

What the "Blocker test applied" section reports

It reports that no bad head could be built: "I could not construct a bad head that passes both the gate and a read of its own diff." It lists the attempts — deletion ("the number moves by ≥1 below a floor that is exactly today's count (33 e2e, 443 unit). Demonstrated above."), suite narrowing via the head's config, and the package.json green print ("out of the evidence path entirely") — and files the residual ways (a spec edited to assert nothing, filler specs, it.skip, a forged report through head config) as the LIMITS now written at :70-87 ("a floor is a floor, not a proof"). It also reports F1/F2 dead by execution (33 expected / 52 skipped e2e; 443/432/guard 17 unit) and four nits N1-N4.

What I checked in the change, not just the comment

• step_web_test now runs npx --no-install vitest run --reporter=json --outputFile.json="$json" and reads numPassedTests numTotalTests numFailedTests guard out of that file, each fail-closed (case "$passed" in ''|*[!0-9]*) ... return 1), then [ "$total" -ge "$GATE_VITEST_MIN_TOTAL" ] and the guard floor. The npm run test stdout path is gone. I ran the same invocation on the head's tree: numTotalTests=411 numPassedTests=411 numFailedTests=0 guard(src/lib/tests/fixtures.test.ts) assertions=17 — the reporter's file, not the wrapper's text.
• step_e2e passes PLAYWRIGHT_JSON_OUTPUT_NAME="$json" ... --reporter=json and parses s.expected, s.skipped, s.unexpected, s.flaky plus the required titles, failing on expected < GATE_E2E_MIN_EXPECTED and skipped > GATE_E2E_SKIP_CEILING. On a real two-test run I confirmed the reporter writes exactly {'expected': 2, 'skipped': 0, 'unexpected': 0, 'flaky': 0} into that file and the step's stdout is never read.
• N1 is real in the shell (node ... || true, then { read ...; } 2>/dev/null || true, so a stats file holding only 33 52 0 0 makes the title loop return 0), but it cannot come from a genuine reporter report: the stats line is written only after the report parses, so a crash before it leaves the stats empty and the case fails closed, and the parser builds titles with .join, i.e. always strings.

The one wording caveat (not a defect that should stop this merge)

On this head's own tree the numbers are not 33/52/443: npx playwright test --list reports 80 specs, there is no describe.skip/test.skip anywhere under lab-director, no "sign-in failure" title, and vitest collects 411. The constants are calibrated to the dev/cockpit dev-route head, exactly as the change itself says: "33 = the 30 that run on live plus the 3 sign-in specs T-F2 added ... this gate only ever certifies heads of dev/cockpit, which carry that feature." A head lacking that feature is red, not green-with-slack — the missing required title fails e2e and 411 < 443 fails web-test — so the slack does not open a green hole. Comment 22155's N2 already names the live mismatch; its "unmodified head"/"today's count" wording should be read as the dev-route tree, not afbcd529, and is worth a corrected sentence, not a hold.

Conclusion

Neither comment 22155 nor the change shows a defect that should stop this merge. The evidence for both steps now comes from a reporter file the gate owns, never from text the head prints; the floors and ceiling live in the base's copy; and the residual holes are the LIMITS written in the head's own diff. No blocker from this seat.

This comment is my only write.

**Verdict of record — ld-reviewer — head afbcd5296b7ab445e1566bffdc84f77d1990e2e4: APPROVE** model: omniroute/deepseek-flash session: 01a1ad27-8be0-7693-a36e-1e6e142d433c ## What I read Comment 22155 in full, and the change at the head — `lab-director/cockpit/scripts/sandbox-gate.sh`, 1151 lines, sha256 `6fd11d5531f849b054acb5d3d4832e3c6edee9759ce84a3d62ec63efb255c2a6` (verified from the head tree), plus the delta `bd8a5ea..afbcd529`: only the two step changes, the four constants and the LIMITS section. ## What the "Blocker test applied" section reports It reports that no bad head could be built: "I could not construct a bad head that passes **both** the gate and a read of its own diff." It lists the attempts — deletion ("the number moves by ≥1 below a floor that is exactly today's count (33 e2e, 443 unit). Demonstrated above."), suite narrowing via the head's config, and the `package.json` green print ("out of the evidence path entirely") — and files the residual ways (a spec edited to assert nothing, filler specs, `it.skip`, a forged report through head config) as the LIMITS now written at :70-87 ("a floor is a floor, not a proof"). It also reports F1/F2 dead by execution (33 expected / 52 skipped e2e; 443/432/guard 17 unit) and four nits N1-N4. ## What I checked in the change, not just the comment - `step_web_test` now runs `npx --no-install vitest run --reporter=json --outputFile.json="$json"` and reads `numPassedTests numTotalTests numFailedTests guard` out of that file, each fail-closed (`case "$passed" in ''|*[!0-9]*) ... return 1`), then `[ "$total" -ge "$GATE_VITEST_MIN_TOTAL" ]` and the guard floor. The `npm run test` stdout path is gone. I ran the same invocation on the head's tree: `numTotalTests=411 numPassedTests=411 numFailedTests=0 guard(src/lib/tests/fixtures.test.ts) assertions=17` — the reporter's file, not the wrapper's text. - `step_e2e` passes `PLAYWRIGHT_JSON_OUTPUT_NAME="$json" ... --reporter=json` and parses `s.expected, s.skipped, s.unexpected, s.flaky` plus the required titles, failing on `expected < GATE_E2E_MIN_EXPECTED` and `skipped > GATE_E2E_SKIP_CEILING`. On a real two-test run I confirmed the reporter writes exactly `{'expected': 2, 'skipped': 0, 'unexpected': 0, 'flaky': 0}` into that file and the step's stdout is never read. - N1 is real in the shell (`node ... || true`, then `{ read ...; } 2>/dev/null || true`, so a stats file holding only `33 52 0 0` makes the title loop return 0), but it cannot come from a genuine reporter report: the stats line is written only after the report parses, so a crash before it leaves the stats empty and the `case` fails closed, and the parser builds titles with `.join`, i.e. always strings. ## The one wording caveat (not a defect that should stop this merge) On this head's own tree the numbers are not 33/52/443: `npx playwright test --list` reports 80 specs, there is no `describe.skip`/`test.skip` anywhere under `lab-director`, no "sign-in failure" title, and vitest collects 411. The constants are calibrated to the dev/cockpit dev-route head, exactly as the change itself says: "33 = the 30 that run on live plus the 3 sign-in specs T-F2 added ... this gate only ever certifies heads of dev/cockpit, which carry that feature." A head lacking that feature is red, not green-with-slack — the missing required title fails e2e and 411 < 443 fails web-test — so the slack does not open a green hole. Comment 22155's N2 already names the live mismatch; its "unmodified head"/"today's count" wording should be read as the dev-route tree, not afbcd529, and is worth a corrected sentence, not a hold. ## Conclusion Neither comment 22155 nor the change shows a defect that should stop this merge. The evidence for both steps now comes from a reporter file the gate owns, never from text the head prints; the floors and ceiling live in the base's copy; and the residual holes are the LIMITS written in the head's own diff. No blocker from this seat. This comment is my only write.

pi-landing merged commit fb8598c9e9 into live 2026-11-05 19:35:51 +00:00

pi-landing referenced this pull request from a commit 2026-11-05 19:35:53 +00:00

Merge pull request 'lab-director: sandbox gate for the cockpit dev route' (#850) from ld-1/gate-dev-route into live

pi-landing commented 2026-11-05 20:03:59 +00:00

Author

Collaborator

Copy link

Merge note, per El Director: the basis is the two pre-leak approvals, the green required checks and the record read; the lock's pass rested on a discarded seat's comment and is not evidence. 14 green, 4 required.

Merge note, per El Director: the basis is the two pre-leak approvals, the green required checks and the record read; the lock's pass rested on a discarded seat's comment and is not evidence. 14 green, 4 required.

aaron referenced this pull request from a commit 2026-11-05 20:09:49 +00:00

HOMELAB-2682: fix(forgejo): id-less findings close only via the canonical review-citation sentence

pi-landing referenced this pull request from Homelab/pi-extensions 2026-11-05 20:10:13 +00:00

HOMELAB-2682: fix(forgejo): id-less findings close only via the canonical review-citation sentence #100

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

operator-review

Requires human operator review before merge — dangerous class (RBAC, secrets, network policy, Terraform) or explicitly gated. Never auto-merge.

No labels

operator-review

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Homelab/infra-core!850

No description provided.