fix(gates): 16 gates reported PASS when their helper never ran — 2 are authorization gates - #147
Conversation
Each of these 16 gates enumerated its files, found work to do, discovered
its Python/JS helper was missing, echoed a WARN to **stderr**, and then
fell through to `_pass` on **stdout**:
if [ -f "${_oa_lib_dir}/check_orphan_auth.py" ]; then
python3 ... >> "${_oa_log}" 2>/dev/null || true
else
echo "[gate-6] WARN: ... — gate-6 skipped" >&2
fi
_oa_fail=$(wc -l < "${_oa_log}" ...)
if [ "${_oa_fail}" -eq 0 ]; then
_pass 6 "orphan-auth" # <- ran even though the helper did not
An empty findings log because the helper never ran was byte-identical to
an empty log because there were no findings. Every consumer of this runner
anchors on `^\[gate-`, so the WARN that said so was invisible.
Worse: `_pass` adds the gate to `_EMITTED_GATES`, and the summary computes
"GATES THAT DID NOT RUN" as declared-minus-emitted. These 16 branches
therefore actively DEFEATED the coverage machinery built to catch exactly
this, and `--require-full-coverage` could not see the gap either.
Two of the 16 are authorization gates: gate-6 (orphan-auth) and gate-7
(no-admin-idor).
Measured on a real repo checkout (decidesk @ 2b65908):
helper present -> [gate-7] no-admin-idor: FAIL - 11 method(s) with
NoAdminRequired + no guard
helper renamed -> [gate-7] no-admin-idor: PASS
Same tree, same 11 unguarded endpoints, and coverage read "61 of 63
declared gates reported a result" in BOTH directions.
Fix: each site sets a per-gate `_ran` flag, calls `_skip <n> <name>
<reason>` when the helper is absent, and guards the existing pass/fail
block with it. The pass/fail logic is untouched for the case where the
helper does run. `_skip` records the gate WITHOUT adding it to
`_EMITTED_GATES`, so it lands in the DID-NOT-RUN list where it belongs.
Sites: gates 6, 7, 9, 15, 16, 18, 19, 25, 26, 27, 51, 52, 54, 55, 56, 57.
gate-18 keeps its deliberate advisory half: check (a) (legacy dialect,
hard fail) is helper-driven and now skips; check (b) (imperative dispatch,
WARNING, non-blocking) is pure bash, still runs, and is explicitly left
outside the `_ran` guard.
Tests:
- test-hydra-gates-bin.sh gains a two-directional, attributable control:
the SAME fixture against two package copies differing ONLY by the two
security helper files. Asserts SKIPPED, asserts NOT PASS, asserts both
gates are named in DID NOT RUN, and asserts coverage drops by exactly 2.
Verified failable: against the unpatched runner it reports 4 failures
including "coverage went 35 -> 35".
- test_check_custom_widget_ratchet.py::test_helper_absent_warn_skips had
CODIFIED the defect - it asserted "custom-widget-ratchet: PASS" with the
helper absent, on a fixture containing a real finding. Rewritten to
assert the correct contract.
Suites: entry-point 19 -> 25 passed / 0 failed; helper suites 17 passed /
0 failed / 2 quarantined, unchanged from baseline.
Also documents why the coverage summary must keep deriving from
`_EMITTED_GATES` rather than the write-only `_SKIPPED_GATES`: the former
also catches gates that emit nothing at all because a prerequisite was
false, and is therefore strictly broader.
|
Admin-merging to Verified before merging, since this file runs in every repo's gate job:
The evidence that made this urgent, restated for the record — measured on a real checkout (decidesk
Eleven real unguarded endpoints reported clean, and the run's own Gate suite: 19 → 25 passing, 0 failed. The new test was shown to fail against the unpatched runner (4 failures, including Two things worth carrying forward:
Still open and not addressed here: gates 6/7 also |
The defect
Sixteen gates in
hydra-gates/scripts/run-hydra-gates.shenumerated their files, found work to do, discovered their helper script was missing, echoed aWARNto stderr, and then fell through to_passon stdout.An empty findings log because the helper never ran is byte-identical to an empty log because there were no findings. Every consumer of this runner anchors on
^\[gate-, so the WARN was invisible to all of them.It also defeated the coverage machinery built to catch it.
_passadds the gate to_EMITTED_GATES, and the summary computes "GATES THAT DID NOT RUN" as declared minus emitted. So these sixteen branches made themselves invisible to the accounting, to theALL N GATES GREENbanner, and to--require-full-coverage(exit 98).Two of the sixteen are authorization gates: gate-6 (orphan-auth) and gate-7 (no-admin-idor).
Proof, on a real repo checkout (decidesk @
2b65908)Before, helper present:
Before, same tree, helper renamed away:
Eleven unguarded
#[NoAdminRequired]endpoints became aPASS, the failure count went 18 → 17, and the coverage line was identical in both directions.After, helper renamed away:
After, helper present — byte-identical to before. No behaviour change when the helper runs.
The fix
Per gate: a
_ranflag, a_skip <n> <name> <reason>call on the helper-absent path, and the existing pass/fail block guarded by that flag._skipalready existed and records the gate without adding it to_EMITTED_GATES.Sites: gates 6, 7, 9, 15, 16, 18, 19, 25, 26, 27, 51, 52, 54, 55, 56, 57.
gate-18keeps its deliberate advisory half. Check (a) — legacy dialect, hard fail — is helper-driven and now skips. Check (b) — imperative dispatch,WARNING, non-blocking — is pure bash, still runs, and is explicitly left outside the_ranguard.Tests
tests/test-hydra-gates-bin.shgains a two-directional, attributable control: one fixture, two package copies differing only by the two security helper files. AssertsSKIPPED, asserts notPASS, asserts both gates are named in DID-NOT-RUN, and asserts coverage drops by exactly 2.Verified failable — against the unpatched runner it reports 4 failures, including
coverage went 35 → 35.test_check_custom_widget_ratchet.py::test_helper_absent_warn_skipshad codified the defect: it assertedcustom-widget-ratchet: PASSwith the helper absent, on a fixture containing a real finding. Rewritten to the correct contract.test-hydra-gates-bin.shrun-helper-suites.shNotes for the reviewer
_filter_preexistingdoes NOT share this defect shape. It is silent when its helper is absent, but the consequence is inverted: it removes pre-existing findings from a log, so its absence leaves more findings in place. Fail-closed (noisier), not fail-open (blinder)._SKIPPED_GATESis genuinely write-only — written by_skip, read nowhere. It should stay that way, and this PR adds a comment saying why: the summary derives from declared minus_EMITTED_GATES, which is strictly broader, because it also catches gates that emit nothing at all when an enclosingif [ -d src ]-style prerequisite is false. Rewiring the report onto_SKIPPED_GATESwould narrow it back to explicit skips only and silently reopen this hole._passwithout inspecting anything. Thebin/hydra-gateswrapper does reportSCOPE WAS EMPTYseparately, so this one is stated rather than silent.Risk
ConductionNL/.githubis shared infrastructure for the whole fleet. The change is mechanical and additive: no gate is weakened, disabled, or made more permissive, and every pass/fail path is byte-for-byte unchanged when the helper is present. Where a gate's verdict changes it changes from a falsePASStoSKIPPED, which the coverage accounting then surfaces.Default branch is
main, so this is left open for a human to merge — not merged by the agent.