fix(gates): a gate must never report PASS over a scope it did not open (#242, #240) - #258
Merged
Merged
Conversation
#242, #240) Gates 19 (e2e-coverage), 25 (contract-coverage), 62 (store-plane) and 63 (settings-surface) diff-scoped themselves INSIDE their own helpers, below the runner's base resolution, and did it UNCONDITIONALLY — the base ref was defaulted to origin/development even when the caller had asked for no scoping at all. Two consequences, and the second is why it stayed hidden: 1. A full-tree run — the mode a fleet audit uses — was silently narrowed to a diff against origin/development, which on a mainline checkout is empty. 2. The verdict for "I inspected nothing" was PASS, not a skip. So --require-full-coverage, the one assertion built to catch gates that did not run, had nothing to catch. gate-63 was the clearest case (#240): its log printed "gate skipped" on the line above a verdict that said PASS. Both cannot be true, and PASS is the one every consumer counted. Measured on openconnector 2026-08-08: gate-19 5 findings as the runner invoked it -> 412 over the full tree gate-25 PASS as the runner invoked it -> 32 over the full tree - the base ref is now the caller's decision and is never defaulted: set means diff-scoped, unset means full-tree audit - an empty scope returns a distinct status and the runner maps it to `_skip … structural`, which counts against coverage and fails --require-full-coverage - absent subject matter returns its own status and maps to `_skip … na` - gate-25 scopes on CONTROLLER files, not "any file": a docs-only diff opened no controller, and PASS there claimed a wire contract had been read when none was - gate-25 now returns a STATUS rather than the finding count, matching the convention gate-19 settled on in #209; the count is read from stdout Mutation-checked, three mutants, all killed: 1. pristine pre-fix runner — every arm red 2. the #242 defect reintroduced on gate-19 — ARM 1 red: the planted true positive stops being caught 3. ANTI-WIDENING CONTROL, checkers forced to inspect nothing — ARM 1 and ARM 3 red, so the suite cannot be satisfied by skipping everything
Follow-up to the empty-scope work, found by test_check_e2e_coverage.py. Adding the NOT APPLICABLE verdict introduced a regression of exactly the kind this issue is about: `openspec/specs` missing and the app dir being unreadable produce the SAME empty set, and the new code reported both as NOT APPLICABLE. That would retire the gate on the strength of a typo in a path. A missing directory is now an ERROR — a failure to look, not an absence of specs. Three tests updated, and two of them encoded the defect as an expectation: - test_pass_when_no_spec_files_in_diff asserted PASS for a repo with no specs. Renamed and now asserts NOT APPLICABLE: "I inspected nothing" and "I inspected everything and it was fine" cannot share a verdict. - test_diff_scope_only_changed_spec_flagged asserted PASS when the diff touched no spec. It now asserts EMPTY SCOPE, and STILL asserts the ADR-020 invariant it was written for — an untouched legacy spec is never flagged. - test_run_gate_raising_is_reported_as_ERROR monkeypatched changed_spec_files, which an unscoped run no longer calls. It now sets a base ref, so it exercises the path it claims to. Without this it would have passed for a reason unrelated to what it checks. All 105 gate-19 helper tests green; all 33 discovered helper suites green.
This was referenced Aug 8, 2026
rubenvdlinde
added a commit
that referenced
this pull request
Aug 9, 2026
…ver a dead interpreter, and three could not see the defect they exist for (#280) * fix(gates 45-55): eleven gates passed over an unopened scope, eight over a dead interpreter, and three could not see the defect they exist for Every gate in this band was given ONE textbook true positive of exactly what it exists to catch, planted in a real fleet repo, then removed again. Where a gate could not fail, it was repaired; where it could, the plant is now a regression test. Measured at package sha 34370f6. ## 1. All eleven reported PASS over a scope they never opened (#242/#240/#258/#268) On a README-only diff against larpingapp, gates 45-55 printed eleven PASS lines and the summary read "53 of 53 applicable gates ran". Not one of them had opened a file. Gates 4/6/7/19/25/28/62/63 have answered the identical situation with NOT APPLICABLE since #268; this band never adopted it. Gates 47 and 48 are the sharper case: they can only answer a question about a CHANGE SET, so on every builder full-repo run in the fleet — no base ref at all — they printed a co-change verdict they had not formed. ## 2. Eight reported PASS over a crashed interpreter (#147/#249/#262) A planted defect only fires when the gate runs, so no plant can see this. With a `python3` on PATH that exits 1 on every call, on a tree carrying real findings: gate-46 PASS — over the 277 unresolved @SPEC findings, across 104 distinct targets, it had reported one run earlier on the same files gate-47 PASS — on the same diff where it had just reported FAIL gate-45/49/50 PASS (`2>/dev/null` discarded status and traceback) gate-51/54/55 PASS (`|| true` discarded the status) gate-52 FAIL — "1 custom-widget finding(s)", a fabricated finding: the helper returned its COUNT as its exit status, the same channel Python uses for a traceback (#209). The count was also clamped to 99 to fit in a byte. It now prints `findings=N` on stdout and exits boolean; no `findings=` line means the helper died. gate-54 was the quietest: its advisory WARN half reads the same log, so a dead helper silenced both halves at once. ## 3. gate-45 was the residue of #272's fix (.github#274) #272 migrated gates 35/40/42/44 off `[ -d src ]` onto `_a11y_has_markup_dir` and left the twelfth member of the family behind. On a templates-only app gate-45 reported NOT APPLICABLE — "this repo ships no frontend" — over a `<style>` block with `transition:` and no reduced-motion fallback, in the same file gate-43 FAILED on in the same run. `na` is the one verdict that removes a gate from coverage accounting. The regression test was already written and gate-45 was excluded from it by name, with a comment explaining why. Removing the name from ARM 4's skip list in test_gate_a11y_markup_scope.sh IS the test; it fails against 34370f6. ## 4. gate-47: prose satisfied it, and a qualified attribute did not `_ANNOTATION_RE` was an unanchored alternation of string literals, and it was wrong in both directions from that one regex — the pairing #269 found in gate-48 and never carried to its sibling. FALSE POSITIVE rewording ONE docblock sentence that merely NAMES the annotation ("becomes `@NoAdminRequired` again, paired with a real ownership check") made the gate demand a test co-change. A gate satisfiable by prose manufactures the appearance of a security review (#191). FALSE NEGATIVE `#[\OCP\AppFramework\Http\Attribute\NoAdminRequired]` was invisible. A commit adding exactly that to a controller — opening an admin-only endpoint to every authenticated user — with no test in the diff reported PASS. Now position-anchored, by the same rule check_csrf_removal.py already used. ## 5. gate-50: a false positive and a false negative in the same regex FALSE NEGATIVE the app-id argument had to be a QUOTED STRING, so every read written the fleet-standard way — `getValueString( Application::APP_ID, 'listing_register', '')` — was invisible. Identical code with `'larpingapp'` FAILED. Same family as #184. 7 security-relevant reads across 5 repos sit behind a constant today. FALSE POSITIVE the empty-compare guard required a closing paren immediately after the empty string, so the correct compound guard `if ($reg === '' || $sch === '')` was reported as unguarded — twice, on code the gate was asking for. A guard that is a boolean `return` rather than an `if` was rejected too. Both directions are now asserted, including the opencatalogi#86 shape that mixes them: one read guarded, the next unguarded two lines later. ## 6. gate-53 did not block the PR that creates larpingapp#286 Reintroducing #286 exactly — the check-in tab deleted from src/manifest.json, `EventRoster` left registered in src/registry.js — reported PASS. Direction 1 of the registry cross-reference stays advisory for LEGACY orphans, correctly: the gate cannot tell "wire it" from "delete it". But when the DIFF ITSELF removed the last reference it can, and that finding now blocks. Pre-existing orphans are untouched (larpingapp carries one today), so this is prevention, not a burn-down list nobody can close. ## Verified working, repaired nothing gate-46 (dangling file, dangling fragment, valid anchor), gate-48 (short and fully-qualified attribute removal; a comment reword correctly stays green), gate-49, gate-51 (title, description and nested items.properties independently), gate-52's ratchet (growth fails, shrink passes), gate-54 (flat, nested and $ref-carrying), gate-55. ## Deliberately NOT enforced `title == key` on a schema property is a real gate-51 defect — the renderer uses `prop.title || key`, so the user sees the raw technical key. Measured across 10 repos: 148 occurrences, ALL of them in softwarecatalog, where they are VNG-standardised element names (`identifier`, `type`, `name`) that must not be renamed. Enforcing it would produce 148 findings with no legitimate end state in the one repo that has them. Reported rather than gated (#252). ## Divergence to reconcile gate-45 now answers an empty in-scope set with `na`; gate-40 answers it with PASS, by a deliberate choice in #272 that cited the invariant test this PR reworks. The invariant now discriminates on the REASON — the applicability table's own phrasing must not appear once its prerequisite holds — so both behaviours are expressible. The family should pick one. ## Testing New: hydra-gates/scripts/lib/test_gate_45_to_55_acceptance.sh — 31 arms across six families, discovered by run-helper-suites.sh. Against the package as merged on main it fails 20 of 31; the 11 that pass are exactly the anti-widening and no-regression controls. Every mutation asserts its anchor is present before it plants. Repos used, chosen for different shapes: larpingapp (register-owning, manifest-driven, ships registry.js), nldesign (PHP templates, no .vue, no register), doriath (ships no phpcs SpecTagSniff — the #246 control, held at 81 findings across 46 targets before and after the plant), openconnector (41 register files). Full package suite: 52 discovered suites pass, 2 quarantined as documented; 60/60 entry-point invariants. * fix(gate-50): the fail-mode window started where the call began, not where it ended The constant-app-id fix in the parent commit made procest's config reads visible for the first time and immediately produced 3 findings on lib/Service/AiService.php — all three false positives, and both causes are ordinary code the window could never have seen: multi-line call PHPCS formats each read across five lines. Two of them plus a blank line put the guard on the ELEVENTH line, one outside a window counted from the line the match BEGAN on. The guard being missed is a textbook `if (empty($registerId) === true || empty($schemaId) === true) { $this->logger->warning(...); return; }` (AiService.php:580, :967). same-line guard `'ai_api_key_set' => ...getValueString(APP_ID, 'ai_api_key', '') !== ''` handles the empty default ON the match line, and the window started after it (AiService.php:710). The window now anchors to the END of the call expression — parentheses balanced forward from the `(` — and includes the remainder of that line. A single-line read keeps exactly the ten lines it always had. Caught by a before/after sweep of 12 fleet repos: 26 of 121 verdicts changed, 25 of them PASS -> NOT APPLICABLE (the truthfulness correction), and this was the only one that changed to FAIL. procest is PASS again, correctly. Three arms added: the multi-line shape, the same-line shape, and the reverse control — the same multi-line shape with the guard DELETED must still FAIL, so the window cannot have been widened until the gate finds nothing. Also: shellcheck SC2181 in gate-45's new status check, and a file-scoped SC2016 suppression for the acceptance suite, whose PHP fixtures are single-quoted on purpose.
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
1 participant
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.
Closes #242. Closes #240.
The defect
Gates 19 (e2e-coverage), 25 (contract-coverage), 62 (store-plane) and 63 (settings-surface) diff-scoped themselves inside their own helpers, below the runner's base resolution — and did it unconditionally. The base ref was defaulted to
origin/developmenteven when the caller had asked for no scoping at all.Two consequences, and the second is why it stayed hidden:
origin/development, which on a mainline checkout is empty.--require-full-coverage— the one assertion built to catch gates that did not run — had nothing to catch.#240 is the clearest illustration: gate-63's log printed
gate skippedon the line directly above a verdict that saidPASS. Both cannot be true, and PASS is the one every consumer counted.Measured on openconnector (2026-08-08)
439 findings behind two green lines, in one repo.
The fix
_skip … structural, which counts against coverage and fails--require-full-coverage_skip … naStatuses extend #249's
0/1/2rather than introducing a second convention:3 = empty scope,4 = not applicable.Proof, both arms
…while over the full tree the same fixture's planted true positives are still caught (gate-19 and gate-25 both FAIL on one genuine finding each), and 62/63 PASS over a clean manifest rather than skipping themselves into uselessness.
Mutation check — three mutants, all killed
That third mutant is the one that matters: without it, "make every empty scope a skip" could be satisfied by skipping unconditionally, and the suite would look fixed while checking nothing.
Fleet impact
The fleet consumes these gates at
@main, unpinned, so this reaches all 16 apps on their next run. Repos will see gates 19/25/62/63 move from PASS to either a real finding count or a stated SKIP. That is the point — none of those PASSes were measurements.🤖 Generated with Claude Code