fix(gates): gate-53 ignored the diff scope, gate-22 printed FAIL — 0, gate-33 had never run - #139
Open
rubenvdlinde wants to merge 1 commit into
Open
fix(gates): gate-53 ignored the diff scope, gate-22 printed FAIL — 0, gate-33 had never run#139rubenvdlinde wants to merge 1 commit into
rubenvdlinde wants to merge 1 commit into
Conversation
… gate-33 had never run
Three defects, all of the same family: a check whose absence or degradation is
byte-identical to its success.
gate-53 (effective-manifest-crossref) was diff-scoped at FILE granularity. An
app's whole navigation surface lives in src/manifest.json + src/manifest.d/*,
so touching any of it re-judged all of it. Measured on a one-line `title`
change: pipelinq 24 full-repo / 24 diff-scoped, shillinq 246/246 — every
finding on a page the PR had never touched. Enabling the gate would have
blocked every manifest-touching PR on inherited debt.
Answering a cross-reference genuinely needs the whole assembled manifest — you
cannot resolve menu[].route to a page id from a diff — but BLOCKING on the
answer does not. Findings carry a JSON pointer that resolves to a page id, a
menu id or a top-level block; a new scope computer (manifest_diff_scope.py) maps
the diff to those entries and a shared filter (manifest_scope_filter.js) blocks
only on entries the PR touched. Suppressed findings are printed as
PRE-EXISTING and counted on stdout, never dropped. Whole-manifest invariants,
and any PR whose scope cannot be determined (new fragment untracked at base,
changed register JSON, parse failure), still block: unverifiable scope is not
narrow scope. After: pipelinq 0 blocking / 24 pre-existing, shillinq 0 / 397;
touching an offending page brings its 3 findings straight back.
gate-22 printed "FAIL — 0" on opencatalogi with the rest of its message
orphaned onto an unparseable second line. `grep -c … || echo 1` captures BOTH
the "0" grep prints and the fallback "1", and `[ "0\n1" -eq 0 ]` errors instead
of comparing, so the clamp never fired. Diagnosed once at gate-17 and fixed
there only; eight other sites still carried it, including gate-48's, where it
was fail-OPEN — zero frontend co-change signals read as "co-change found" and
a CSRF-protection removal passed. One `_count` helper now, used everywhere.
Underneath that count sat a worse problem: gate-22's verdict was app-owned. It
preferred `npm run check:manifest`, which on pipelinq is a 50-line guard that
certifies any manifest, and on opencatalogi/shillinq is a per-app schema copy
that without Ajv falls back to a hardcoded "v1.x enum" predating ADR-040 and
rejects `type: "roadmap"` — a page type the canonical schema has accepted since
2.x. The gate was failing apps for conforming to the standard, and announcing
its own downgrade ("no schema candidate resolved; falling back to structural
lint") on a line nothing acted on. The vendored canonical validator now decides;
the app script still runs, as a surfaced advisory. And a validation that could
not apply the schema exits 3 DEGRADED rather than reporting a pass.
gate-33 (axe-core) has never run in any of the 13 fleet repos. The
tests/axe/report.json it consumes is produced by a scripts/run-browser-tests.sh
that exists in no app, while axe-core sits in every package.json so the
prerequisite looks wired. It emitted nothing at all when absent, and the runner
closed with "ALL 63 GATES GREEN" regardless — so every green this fleet has
produced excluded accessibility runtime checking. gate-24 was absent in most
repos for the same structural reason. Both now state their own absence, the
runner performs the declared-vs-reported accounting its wrapper already did
(most of the fleet invokes the runner directly), a SKIPPED line is not counted
as coverage, and --require-full-coverage exits 98.
Also: test_check_manifest.sh had been green its whole life while pointing at a
fixture directory that has never existed — a missing manifest path makes the
validator print "Tier 0, skipping" and exit 0, which is what two of its three
assertions expected. Fixtures now ship, the suite refuses to run without them,
and it plus two new suites run in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three defects in
conduction/hydra-gates, all of one family: a check whose absence or degradation is byte-identical to its success.Everything below was re-measured against the running system. Two of the briefed findings did not reproduce; that is reported here too.
Defect 1 — gate-53 ignored the diff scope
gate-53 (
effective-manifest-crossref) was diff-scoped at file granularity. An app's whole navigation surface lives insrc/manifest.json+src/manifest.d/*, so touching any of it re-judged all of it. File granularity was indistinguishable from no scoping.Measured on a one-line
titlechange, before the fix:Every finding sat on a page the PR had never touched.
What changed. Answering a cross-reference genuinely needs the whole assembled manifest — you cannot resolve
menu[].route→ page id, or check the ADR-044 no-orphan-removal invariant, from a diff. That part is legitimately whole-repo and stays so. Blocking on the answer does not: every finding carries a JSON pointer that resolves to a page id, a menu id or a top-level block.scripts/lib/manifest_diff_scope.py— maps the diff to the manifest entries it touched (page ids, menu ids incl. nested children, top-level keys). Reuses gate-55's line-tracking JSON parser so the two gates cannot disagree about where a page begins.scripts/lib/manifest_scope_filter.js— shared bycheck_manifest.jsandcheck_manifest_crossref.js; partitions findings into blocking / pre-existing / unscopable.Suppressed findings are printed as
PRE-EXISTINGand counted on stdout, never dropped — a scoped green must not read as "the manifest is clean". Whole-manifest invariants keep blocking, and so does any PR whose scope cannot be determined (a fragment untracked at base, a changed register JSON, a parse failure). Unverifiable scope is never treated as narrow scope.After:
Positive control, both directions, same repo and same base: adding a commit that touches the
Prospectspage (which carries 3 real findings) →FAIL — 3, naming/pages/51, 21 pre-existing.git revert --no-editof that commit, verified applied by re-reading the file and bygit diff --name-onlyno longer listing it → back toPASS, 24 pre-existing. 3 + 21 = 24.Briefed findings that did NOT reproduce
9/9— gate-34 scopes correctly. Full-repo 9; diff-scoped with an unrelated file touched 0; diff-scoped with one of the nine debt files touched 1.ConsumerDetailtouched.So gate-53 was the only gate ignoring scope. The others were most likely measured against a base ref that made the whole app the diff.
Adjacent finding: gate-53 is red-on-arrival without
ajv[gate-53] … FAIL — ajv not resolvable … refusing to run fail-openin every repo withoutnode_modules. It fails closed and says so, which is the right shape — but no code change can turn it green.ajvis in every fleet app'spackage-lock.json, sonpm cibefore the gates resolves it. Documented in the README.Defect 2 — gate-22 printed
FAIL — 0Reproduced verbatim on opencatalogi:
[gate-22] manifest-validation: FAIL — 0.grep -c … || echo 1captures both the0thatgrep -cprints on no-match and the fallback1, giving"0\n1".[ "0\n1" -eq 0 ]is not an integer comparison — it errors and returns 2, so the "clamp to at least 1" guard never fired and the message was emitted with an embedded newline. The visible line readFAIL — 0; the rest was orphaned onto a second line no^\[gate-consumer parses.Diagnosed once before at gate-17 and fixed there only. Eight other call sites still carried it. One at gate-48 (
csrf-cochange) was fail-open: zero frontend co-change signals produced"0\n0", the-eq 0test errored into the else branch, and a CSRF-protection removal with no frontend counterpart passed. All replaced with a single_counthelper.Underneath the count sat a worse problem: gate-22's verdict was app-owned. It preferred
npm run check:manifest:type: "roadmap"— a page type the canonical schema has accepted since 2.x. The gate was failing apps for conforming to the standard. That was the "real finding in the log": a false positive from a stale app-local lint.no schema candidate resolved; falling back to structural lint) on a line the gate neither surfaced nor acted on.The vendored canonical validator now decides. The app script still runs; its output is captured and surfaced as an explicit advisory that cannot set the verdict.
Loud fallback.
check_manifest.jsnow exits 3 — DEGRADED when Ajv is unresolvable and it found nothing, on both channels (SCHEMA VALIDATION DID NOT HAPPEN,"status":"degraded"). A real finding still exits 1, so degradation never masks a violation. gate-22 reports that as a named FAIL rather than a pass, mirroring gate-53's existing stance.Corrected output on opencatalogi:
and without
ajv:Defect 3 — gate-33 (axe-core) has never run, anywhere
Established cause. The gate consumes
tests/axe/report.json, documented as the output ofscripts/run-browser-tests.sh. Checked across pipelinq, shillinq, opencatalogi and openconnector: neither the report nor the producing script exists in any of them. Meanwhileaxe-coreis a declared devDependency in every one, so the prerequisite looks wired. Nothing anywhere writes that file (grep -rn 'axe/report.json\|AxeBuilder\|axe.run'→ no hits outsidenode_modules).It was never wired. And because a gate whose prerequisite is absent emitted nothing at all — no line, no count, no trace — while the runner closed with a hardcoded
ALL 63 GATES GREEN, every green this fleet has ever produced excluded accessibility runtime checking and nothing said so. gate-24 (integration-parity) was absent in most repos for the same structural reason.Both now state their own absence:
and the runner now performs the declared-vs-reported accounting its wrapper already did — most of the fleet invokes
run-hydra-gates.shdirectly and was blind to it:A
SKIPPEDline is not counted as coverage in either layer —bin/hydra-gatesexcludes it explicitly, so the fix cannot become the bug.--require-full-coverage(now accepted by the runner too) exits 98. The inventory is read out of the runner itself, so adding gate 64 cannot leave the check measuring a stale 63.Pre-existing defect found while working: a dead test suite
scripts/lib/test_check_manifest.shpointed atscripts/test-fixtures/manifest-validation/— a directory that has never existed in this repository.node check_manifest.js <missing-path>prints "Tier 0, skipping" and exits 0, which is exactly what two of its three assertions expected. It had been green its whole life while validating nothing, and it was in no CI job.Fixtures now ship, the suite refuses to run if they go missing again, and three suites are wired into CI (with
ajvinstalled, so the schema paths are exercised for real rather than skipped):test_check_manifest.sh— 14 assertions incl. the degraded contract and the scope filter, both directionstest_manifest_scope_filter.js— 18 assertions on pointer attribution and partitioning, every "suppressed" paired with a "survived"test_manifest_diff_scope.py— 10 assertions in a throwaway git repotest_check_manifest_crossref.jshas the same problem (scripts/test-fixtures/effective-manifest/{good,broken}never shipped) and is left as-is — out of scope here, and its subject is covered live by the pipelinq/shillinq measurements above. Worth a follow-up.The package's own suite goes 13 → 19 assertions, adding: gate-33 states its absence; the summary names unrun gates; no
ALL N GATES GREENbanner while a gate skipped; skipped gates excluded from the coverage tally;--require-full-coverageexits 98; no gate verdict wraps onto a second line.Consumer impact
v1.0.1behaviour changes for consumers who pin it. A new tag is needed. Once tagged:npm cibefore the gates (or setNODE_PATH). gate-22 now FAILS rather than passing when it cannot resolveajv— it will not certify a manifest it never schema-validated.ajvis already in every fleet app's lockfile.check:manifestwas weaker than the canonical schema will surface real findings; apps failed by a stale app-localroadmapenum will go green.SKIPPEDlines and aCOVERAGE:block inrun-hydra-gates.shoutput. Anything parsing^\[gate-N\]must treat: SKIPPEDas not run, not as a pass.--scope-to-diff— it was previously unenablable on any repo with manifest debt.Full-repo runs are unchanged in strictness (verified on pipelinq: 24 / 8 / 9 before and after).
Verification
*.shin the repo (baseline was clean; one new SC2154 was introduced by removing an incidental_rcassignment and is fixed properly, not suppressed).🤖 Generated with Claude Code