fix(gates): gate-3 spares a contract-imposed unused param; gate-53 compares page identity not route spelling

[rubenvdlinde]

Closes #339. Partially addresses #340 — see "What #340 actually is" below; that issue needs a decision, not just this patch.

Both issues were filed with the observation that no app-side fix exists. Rule 1 was to test that claim before writing anything, so both were reproduced first, on a fresh development checkout of six fleet apps.

Reproduction — both still live

Full 60-gate run, hydra-gates@c26f9a3, apps at development:

app	gate-3 caller-identity-ignored	gate-53 removals-invariant
procest 792fe1f4b	3	8
opencatalogi	0	0
openregister	0	0
softwarecatalog	0	0
docudesk	0	0
larpingapp	0	0

Both reproduce exactly as filed, at the same three files and the same eight removals. Both live populations are procest only. menu-layout.json#removals is non-empty in 1 of the 18 core apps (surveyed via the API on development; the probe's positive control is that it returned procest's 8).

gate-3 — outcome: the question is right, the detection is incomplete. Fixed.

The rule exists for decidesk#45: a builder-generated authorize*() stub that accepts the caller and forgets to check it. That is a real defect and the rule should keep finding it.

It cannot currently tell that shape apart from a correct strategy implementation. All three procest findings implement GuardEvaluatorInterface::evaluate(array, array, string $userId); two of the five implementors use the parameter, which is the whole point of those classes. A guard answering "is this required field filled in?" has no business consulting the caller. Deleting the parameter breaks the interface and every call site; referencing it pointlessly is dead code written to satisfy a stub detector. Neither edit is a fix, which is why procest reported instead of closing.

New helper check_caller_identity_exempt.py. A finding is exempt only when both hold:

1. The signature is imposed from outside — a supertype resolvable in this repository (implements/extends, transitively) declares the same method with the same parameter. Fully mechanical; nothing the author asserts. This is what makes the exemption un-sprinklable: an invented service method has no supertype, so a fixer agent cannot escape the rule by adding a docblock line.
2. The author marked that parameter unused — the docblock's @param line for the flagged parameter carries an explicit unused marker.

Condition 1 alone would exempt a genuinely gutted implementation (a RoleGuard that stopped consulting $userId is contract-imposed too, and that is the defect). Condition 2 alone is a one-word escape hatch.

#339 suggested keying on @SuppressWarnings(PHPMD.UnusedFormalParameter) and preferred it. I took the stricter of the two options it offered: that annotation is method-wide and cannot name which parameter it excuses. procest's MandaatGuard carries it — for a different parameter — while genuinely using $userId. Keying on it would have pre-authorised MandaatGuard's $userId for a reason that was never about $userId. The per-@param marker is per-parameter, and all three findings already carry it, so no app edit is needed.

Fail-closed throughout: a missing helper, an unreadable file, or a supertype outside this repository all leave the finding standing.

Acceptance — test_gate_3_contract_param.sh, 5 arms, 4 of them controls

arm	fixture	expected
1	contract + marker (procest's shape)	0 findings
2 control	contract, marker deleted	still reported
3 control	marker, no contract (decidesk#45 with a docblock)	still reported
4 control	both look present, interface file absent from the repo	still reported (fail-closed)
5 control	supertype declares the method without the parameter	still reported

Arms 2–5 each delete exactly one half of the exemption. Revert test: the same suite run against origin/main's unmodified runner — arm 1 goes red (RequiredFieldGuard.php:16 method=evaluate rule=caller-identity-ignored param=$userId), all four controls stay green, i.e. the controls were never depending on the fix.

Effect: procest 3 → 0 across a full 60-gate run, and gate-3 is the only verdict that moved (runner exit 8 → 7). 0 → 0 on the other five apps.

gate-53 — outcome: mixed. One real detection defect fixed here; the issue's own premise does not hold.

The detection defect (fixed)

menu[].route may hold either a pages[].id or a pages[].route — check (a) in the same file accepts both, and apps use both spellings. removals-invariant compared the raw strings, so two menu entries reaching the same page by different spellings did not count as reaching each other. Demonstrated on a two-entry manifest:

menu:  {id: items-by-id,   route: "ItemsPage"}
       {id: items-by-path, route: "/items"}
pages: {id: "ItemsPage", route: "/items"}
removals: ["items-by-path"]

→ removal 'items-by-path' orphans route '/items' — no surviving menu entry reaches it

That is precisely the "duplicate navigation entry whose page is still reachable" ADR-044 §5 sanctions, reported as a loss. Both sides now resolve to the page id before comparison. An unresolvable reference is left as-is, so two broken entries cannot vouch for each other (check (a) already fails those).

Two arms added to test_check_manifest_crossref.js, off one fixture pair differing only in whether the second entry survives:

• id-spelled survivor covers a path-spelled removal → 0 errors
• control: sole entry removed, nothing else reaches the page → still exactly 1 error

Existing broken/ fixture arm (cases-index) unchanged and still red. Whole suite green (28 assertions).

Fleet A/B, crossref findings before → after: procest 9→9, opencatalogi 0→0, openregister 0→0, softwarecatalog 1→1, docudesk 0→0, larpingapp 1→1. Nothing moved — this is a latent defect, not a live one, because only procest uses removals at all and its eight are not this shape.

What #340 actually is — flagged for a decision, not patched

procest's 8 are not closable by this fix and I did not implement the enhancement #340 proposes, for three measured reasons. Detail in the issue comment; summary:

1. A correct app-side fix does exist, and gate-53 removals-invariant equates reachability with the static .menu tree — 8 procest removals whose replacement is a folderSidebar/viewMode/sidebar-tab are reported as functionality loss #340 missed it. All eight ids are declared in procest's own manifest.json / manifest.d/ fragments. Deleting the eight menu nodes at source (keeping the pages entries, which is what makes the routes routable) and emptying removals produces a byte-identical effective manifest — measured — and takes gate-53 from 8 errors to 0. So the finding is closable, behaviour-neutrally, without restoring the anti-pattern or retiring any route.
2. That reveals the gate's real defect: removals-invariant polices the key, not the invariant. The same menu outcome reached by editing a fragment is completely unchecked — I just did it eight times and got a PASS. The gate penalises the honest, centrally-documented authoring path and ignores the silent one, which is the opposite of what ADR-044 says it is guarding ("the hard risk … is silently losing a page").
3. The enhancement gate-53 removals-invariant equates reachability with the static .menu tree — 8 procest removals whose replacement is a folderSidebar/viewMode/sidebar-tab are reported as functionality loss #340 proposes would close at most 3 of the 8. The folderSidebar join is genuinely mechanical; viewMode is a name heuristic, not a join; and the sidebar-tab arm is not resolvable in principle — BesluitvormingLeafTab resolves another app's component from the runtime window.OCA.OpenRegister.integrations registry, and nothing static links it to Voorstellen/Advice/AgendaCompiler. Implementing it would add PASSes for 3 and leave 5 red, which is the failure mode gate-53 removals-invariant equates reachability with the static .menu tree — 8 procest removals whose replacement is a folderSidebar/viewMode/sidebar-tab are reported as functionality loss #340 itself asks to avoid.

Also worth recording: ADR-044 §5 says removals may retire only a duplicate navigation entry whose page is still reachable. The gate implements that faithfully. procest's eight are supersessions, not duplicates — so what #340 is really asking for is an amendment to ADR-044, which is an architecture call, not a gate call.

Files

• hydra-gates/scripts/lib/check_caller_identity_exempt.py (new)
• hydra-gates/scripts/lib/test_gate_3_contract_param.sh (new)
• hydra-gates/scripts/run-hydra-gates.sh (gate-3 call site, fail-closed)
• hydra-gates/scripts/lib/check_manifest_crossref.js (removals-invariant normalisation)
• hydra-gates/scripts/lib/test_check_manifest_crossref.js (2 arms)

source_scope.py untouched. No app repository is changed by this PR.

[rubenvdlinde]

CI settled: 17 success, 17 skipped, 0 failure (counted from the full check-run set via the API, not gh pr checks, which returns a partial set). Package invariants — the job that runs all 86 helper suites — is green.

One more measurement since opening, on the §1 recommendation in the #340 comment (delete the eight menu nodes from procest's own manifest sources, keep the pages, empty removals): a full 60-gate run on the modified tree moves exactly one verdict line, [gate-53] effective-manifest-crossref: FAIL → PASS, runner exit 7 → 6. Nothing else in the suite reacts, and the effective manifest is byte-identical. So that fix is behaviour-neutral against the whole gate suite, not just against gate-53.

Still not merged — the gate-53 half needs the decision in the #340 comment.

[rubenvdlinde]

Correction to my earlier note, since it is now merged and the record should be right.

I reported the local run-helper-suites.sh run as "truncated at ~60/86, all reached suites green". That was wrong. A third attempt completed and the honest record is 20 failed + 2 quarantine-integrity failures:

• 19 were cd: .../hydra-gates: No such file or directory — self-inflicted. I ran rm -rf on the working tree while the harness was still running in the background.
• 2 quarantine-integrity failures, same cause (discovery ran over the deleted tree).
• 1 genuine assertion failure that ran before the deletion, so it was not explained by it: test_gate_crashed_checker_is_not_a_finding.sh — "gate-6 orphan-auth: emitted NO verdict line at all, though this fixture gives it a subject".

I A/B'd that one on a fresh clone rather than assume it was noise:

arm	result
merged main 5523801 (with this change)	ALL PASS (run twice)
pre-merge main c26f9a3 (without this change)	ALL PASS

Three clean-tree runs across both sides of the change, all green — plus Package invariants, the job that sweeps all 86 suites, green on this PR's head. The failure does not reproduce and is not attributable to this change. This box was running four other agents with the disk at 96–97%, and that suite spawns the full 60-gate runner per assertion.

Nothing about the gate-3 or gate-53 conclusions changes. But the coverage claim for this PR rests on CI's Package invariants job, not on my local run — my local run inspected less than it appeared to, and then I deleted the evidence.

⚠️ Still open regardless of the merge: the gate-53 decision in the #340 comment. The merge shipped the id-vs-route detection fix; it did not answer whether removals-invariant should be amended, downgraded to WARN, or rebuilt as a base/head effective-menu diff. procest's 8 are still red and still closable app-side.