fix(gates): four gates that could not be answered truthfully (19, 47, 62, and the vacuous-scope epilogue) - #176
Conversation
Each of these produced a finding a developer could not act on honestly, and
each ships with the true-positive case it must not swallow.
1. THE EPILOGUE CONTRADICTED ITS OWN HEADER
`bin/hydra-gates` decided "was the scope empty?" with
grep -q "0 changed file(s)"
which is a SUBSTRING match — satisfied by "10 changed file(s)", and by 20,
30, 100. A run whose header said ten files changed and whose every gate ran
also printed the "SCOPE WAS EMPTY" epilogue. That string had been adopted
fleet-wide as the tell for a vacuous run, so the bug manufactured doubt about
runs that were fine while saying nothing new about runs that were not.
(Duration is no substitute for it either: a genuinely red run finished in
28s.)
The runner now emits `[hydra-gates] SCOPE-FILE-COUNT: <n>` exactly once, as a
bare integer, and both statements derive from that one number — which is what
makes the contradiction impossible rather than merely unlikely. It is also
the unambiguous line to match on; note the epilogue's own prose appears in
COMMENTS in both files, so grepping the SOURCE for it finds explanations
rather than emissions.
Ten is the smallest count that reproduces the bug, so the new test builds
ten. Paired: a genuinely empty scope must still warn, or the fix has muted
the warning it was meant to repair.
2. GATE-19 ACCEPTED A PERMANENTLY-SKIPPED TEST AS COVERAGE
decidesk carries four tests with EMPTY BODIES and a hardcoded
`test.skip(true, ...)`. Each is tagged `@e2e`, each was counted as
traceability, and together they assert nothing. A gate that accepts a
switched-off test as proof is a dead gate by construction.
Now dead: the `.skip`/`.fixme`/`xit` modifiers, `describe.skip`, an
unconditional `test.skip(true)` or `test.skip()` at the top of a body, and a
body containing nothing but whitespace and comments.
Still live, deliberately: `test.skip(browserName === 'firefox', ...)` and
`test.skip(!process.env.CI, ...)`. Those RUN, everywhere but one place. The
discriminator is the ARGUMENT, not the call — conflating them would swap this
gate's blindness for a different one.
The finding text changed too. "missing @e2e" would send someone to add a tag
that is already there; it now names the file and says the test does not run.
3. GATE-47 CLASSIFIED ON THE WHOLE FILE, NOT THE HUNKS
grep -qE "(...|IUserSession|parse_url|...)" "$f"
So a file counted as a security change if a token appeared ANYWHERE in it.
Two agents hit this independently the same day: a PR whose hunks were CSS
custom properties and one added chevron column was told to add a CSRF test,
and a provably comment-only PR — all 30 changed lib/ lines inside docblocks —
was told to co-change tests. Neither used the opt-out, which is the tell:
people do not reach for an opt-out when they believe the finding is wrong.
Classification now reads `git diff -U0` and judges only added/removed lines.
A comment line counts only for an ANNOTATION (`@NoAdminRequired` and friends
ARE Nextcloud's auth declaration in docblock form); prose that merely mentions
`IUserSession` is a sentence. The discriminator is the token, not the line
shape — excluding all comment lines would drop the legacy annotation form on
the floor, and that mutation fails 13 assertions.
Path-based classification (lib/**/Auth/**, lib/*Csrf*, ...) is unchanged.
4. GATE-62's lib/ SCAN IGNORED DIFF SCOPING
Its manifest checks honour ADR-020; its `lib/**.php` store-discovery scan
walked the whole tree unconditionally. The combination is the worst of both:
the gate only RUNS when a manifest changed, and then judges code the PR never
touched. One pre-existing violation in pipelinq therefore blocked EVERY
manifest-touching PR in that repo, permanently, naming a file outside the
diff. There was no action the author could take that was about their own
change.
Scoped now. Paired three ways: the same violation still fires when the PR
touches that file, it still fires on any full-tree run, and the manifest
findings are unaffected.
VERIFICATION
helper suites: 25 passed, 0 failed (was 23)
hydra-gates entry-point tests: 39 passed, 0 failed (was 36)
Mutation-checked in both directions, per gate:
gate-19 _ref_is_live -> always True (the old blindness) 8 failures
gate-47 whole-file classification (the original defect) 2 failures
gate-47 drop ALL comment lines (the over-correction) 13 failures
End-to-end against the real runner: the docblock-only PR goes
FAIL -> PASS on gate-47, and flipping #[NoAdminRequired] to #[PublicPage] in
the same file still FAILs.
Two new suites (gate-47, gates 62/63) — gate-62 had none at all.
On the mixed fleet, and how these numbers were takenThe fleet currently runs two different gate versions — 14 repos pin Method: every count was produced by running a specific version of the gate's own code over the same 22 repository trees (all at Version-normalised, gate-46 over those same 22 trees:
Two things follow.
The 14 repos still on v1.3.0 are the ones seeing the worst of it. A bump to the tag carrying this work takes them 2,228 → 918 in one step — but it also brings the new refusal conditions, so it is a roll to plan, not a rubber stamp. gate-40 has no such split: its implementation is unchanged between v1.3.0 and v1.4.0, so 1,211 → 517 holds for every repo in the fleet. |
Follow-up to #175. Four gates that produced findings a developer could not answer truthfully — the shape that gets a suite ignored.
Every relaxation ships with the true-positive case it must not swallow, and each was mutation-checked in both directions.
1. The epilogue contradicted its own header
bin/hydra-gatesdecided "was the scope empty?" with:grep -q "0 changed file(s)"That is a substring match.
10 changed file(s)contains it — and so do 20, 30, 100. A run whose header said ten files changed, with every gate running, also printed theSCOPE WAS EMPTYepilogue. That string had been adopted fleet-wide as the tell for a vacuous run, so the bug manufactured doubt about runs that were fine while saying nothing new about runs that were not.Reproduced, then fixed:
The runner now emits
SCOPE-FILE-COUNT: <n>once, as a bare integer, and both statements derive from that one number. Deriving them from one integer is what makes the contradiction impossible rather than merely unlikely — and it is the unambiguous line to match on.SCOPE WAS EMPTY) appears in comments in both files, so grepping the source for it finds explanations rather than emissions. GrepSCOPE-FILE-COUNT: 0.Paired: a genuinely empty scope must still warn, or this fix has muted the warning it was meant to repair. Ten is the smallest count that reproduces the bug, so the test builds ten.
2. gate-19 accepted a permanently-skipped test as coverage
decidesk carries four tests with empty bodies and a hardcoded
test.skip(true, …). Each is tagged@e2e, each was counted as traceability, and together they assert nothing.test.skip('name', …),it.skip,xit,xtest,test.fixmetest.skip(browserName === 'firefox', …)describe.skip(…)test.skip(!process.env.CI, …)test.skip(true)/test.skip()at the top of a bodyThe discriminator is the ARGUMENT, not the call. A literal
trueis a test someone turned off; a runtime condition is a real test with a guard that runs everywhere else. Refusing the second would swap this gate's blindness for a different one.The finding text changed too — "missing @e2e" would send someone to add a tag that is already there. It now names the file and says the test does not run.
3. gate-47 classified on the whole file, not the hunks
A file counted as a security change if a token appeared anywhere in it. Two agents hit this independently on the same day — a PR whose hunks were CSS custom properties and one chevron column, and a provably comment-only PR (all 30 changed
lib/lines inside docblocks). Neither used the opt-out, which is the tell: people do not reach for an opt-out when they believe the finding is wrong.Classification now reads
git diff -U0and judges only added/removed lines. A comment line counts only for an annotation —@NoAdminRequiredand friends are Nextcloud's auth declaration in docblock form — while prose mentioningIUserSessionis a sentence. The discriminator is the token, not the line shape: excluding all comment lines drops the legacy annotation form on the floor, and that mutation fails 13 assertions.Path-based classification (
lib/**/Auth/**,lib/*Csrf*, …) is unchanged.4. gate-62's
lib/scan ignored diff scopingIts manifest checks honour ADR-020; its
lib/**.phpstore-discovery scan walked the whole tree unconditionally. The combination is the worst of both: the gate only runs when a manifest changed, and then judges code the PR never touched.One pre-existing violation in pipelinq blocked every manifest-touching PR in that repo, permanently, naming a file outside the diff.
Paired three ways: the same violation still fires when the PR touches that file, it still fires on any full-tree run, and the manifest findings are unaffected. gate-62 had no tests at all before this.
Verification
Mutation-checked, per gate:
_ref_is_live→ always True (the old blindness)End-to-end against the real runner: the docblock-only PR goes FAIL → PASS on gate-47, and flipping
#[NoAdminRequired]to#[PublicPage]in that same file still FAILs.Two new suites (gate-47, gates 62/63).