fix(gates 7, 25, 48, 49): a comment must not be enough to get past a gate — 47 of 65 were (#415 class)

[rubenvdlinde]

Follows #420, which fixed gate-50. This surveys all 65 gate checkers for the same defect class and fixes the four most serious instances.

What the survey found

47 of 65 gates are affected. 18 are clean.

Two instances were known — gate 19, then gate 50 in #420. This found 45 more. It was never two bugs; it is the default behaviour of every checker written before somebody thought about it, and about a third of the ones written after.

🔑 The comment that switches a gate off is the comment that admits the debt. gate-50's was a TODO naming the guard it had not written. gate-49's names the catch. gate-7's names the exception. gate-25's says "we still owe a contract test". gate-65's says "we still need to require the autoloader". Every one is the sentence a diligent author writes. A gate that reads prose can be switched off by apologising to it.

Full table, all 65 gates with per-gate evidence: fleet-board/findings/gates-comment-class.md.

The 43 affected gates this PR does not fix are filed with fixtures and verdicts, grouped by root cause: #421 (gate-45, repo-wide blast radius), #422 (false-negative half), #423 (false-positive half), #424 (string literals — 8 of which are one hole in source_scope.markup_mask).

What this PR fixes

Four gates, chosen by severity and by whether the fix could be measured and controlled rather than pattern-matched at scale.

gate-7 — no-admin-idor 🔴

gsrc — the text every guard lookup in scan_file runs against — was the raw source with only authentication-only spans blanked. Prose in a method body therefore answered "is this endpoint guarded?". One fixture, one added line:

	verdict
#[NoAdminRequired] index(int $id) { return $this->service->find($id); }	FAIL — 1 method(s) with NoAdminRequired + no guard
the same method with // TODO: throw new OCSForbiddenException when the caller does not own $id.	PASS ← the defect

This gate's known failure mode has always been false POSITIVES, which is exactly why its silences get believed. It has already reported 0 findings across 18 apps with 167 real IDORs behind it. A false negative here is the most expensive kind in this package.

_strip_strings_and_comments gains keep_strings=True and now also handles # comments — it never did, so the repair was one alternative spelling away from being bypassed. #[ is excluded, because #[NoAdminRequired] is the single token that makes this gate look at a method at all: blanking it would not widen the gate, it would switch it off.

String literals are kept, narrowly and deliberately. A guard's evidence is often an argument that IS a string, and blanking literals in a 2,800-line checker whose known failure mode is over-reporting would trade a measured false negative for an unmeasured wave of false positives on a gate the fleet already learned to distrust. That variant is reported, not smuggled in.

gate-25 — contract-coverage 🔴

gate-19's original defect, verbatim, in the gate written as gate-19's API-layer companion. It had no suite of its own, so nothing had ever asked it to fail.

	verdict
an untested #[NoAdminRequired] endpoint	FAIL — 1 new public endpoint
+ a *Test.php whose only mention is // TODO: we still owe a contract test that calls $this->controller->destroy($id) ... Not written yet.	PASS — all covered ←
+ a .postman_collection.json with zero items whose description reads "NOTE: we do NOT yet cover /api/things — the DELETE endpoint is untested."	PASS — all covered ←

Both greens bought by a sentence admitting the debt — and an author who writes neither note gets the red. That is the gate rewarding silence.

PHPUnit evidence is now php_mask'd. Newman evidence is built from the fields that DECLARE a request (raw/path/host/url/name/method) instead of the file's bytes: JSON has no comments, and a description is a comment in JSON syntax. An unparseable collection falls back to raw bytes rather than to "not covered" — converting a malformed fixture into findings would be this change inventing failures in repos it was never measured against.

gate-48 — csrf-cochange 🔴

Path A. This file's own note already named the hole: "the cheapest way to green would have been a cosmetic edit under src/ containing the word requesttoken: exactly the prose-satisfaction #191 warns against." It was written as a hypothetical. It was not one.

Worse than one missed finding: a non-zero signal count short-circuits the caller-state check built as the mitigation for this exact shape. One comment skips the guard and its backup.

Path B. check_csrf_callers.py read raw — and it makes an affirmative claim the runner prints as a NOTE. So prose did not merely hide a finding, it manufactured a green with a sentence attached saying the code is safe. A commented-out @nextcloud/axios import granted whole-file amnesty to every axios.post in it.

Both now anchor on source_scope.script_mask — comments blanked, string literals kept, because 'OCS-APIRequest': 'true' is a header name that IS a string and method: 'DELETE' is how a mutating call is recognised at all.

gate-49 — controller-exception-translation

Both of its questions were asked of the raw method body, so it failed in both directions from one cause:

	verdict
an unhandled $this->objectService->deleteObject()	FAIL — 1 method(s)
+ // TODO: we should catch (DoesNotExistException $e) here ... Not done yet.	PASS ←
a method calling only $this->renderer->toArray(), + "used to call $this->objectService->deleteObject($id) directly. It no longer does."	FAIL, names index() ←

The removal note scored as the removed call: the better the documentation, the redder the repo.

The docblock is still read from the original text. @throws lives in a comment BY DESIGN, and masking it would trade one false negative for a fleet of false positives.

Proof

Every fix has acceptance arms, and every arm was re-run with the change reverted:

gate	arms	reverted, what flips	what does not (labelled controls)
7	7	3 (//, /* */, #) + 1 signature error	positive control, 2 anti-widening pairs
25	8 (new suite)	3	positive control, 4 "the real artefact still counts" pairs
48	2	H1	H2
49	5	G2, G3	G1, G4, G5

…plus 7 arms in check_csrf_callers' own suite, of which 2 flip and 5 are labelled controls — including the deliberate near-miss: the same TODO sentence three characters outside the paren-balanced span the gate actually searches never reached the question, and passes either way.

An arm that passes both before and after is a control, not evidence, and each suite says which of its arms is which.

🔑 One rig note worth carrying: git checkout -- <file> is not a revert once you have committed — it restores from HEAD, fix included. The first run of that last revert had all seven arms passing, which reads like seven controls and would have shipped a suite proving nothing. Re-run against origin/main, two flip. The tell was that arms I knew should flip did not. When every arm agrees, suspect the rig.

Existing suites still green: gate-7's 138 → 145 tests, test_gate7_verb_object_guards.sh 10/10, and the 45–55 acceptance matrix.

Two things the survey turned up that are not fixed here

1. The shared helper already exists — and #420 wrote a fourth dialect. lib/source_scope.py was extracted in #184/#249 for exactly this reason: offset-preserving, knows #[ is a PHP 8 attribute, knows // inside 'https://x' opens nothing, and exposes the blank_strings axis the two kinds of caller need. Three of the four fixes here use it and add no new stripper. #420's _strip_php_comments should become a call to php_mask once it lands — that is the drift the exclusion_reason.py extraction was done to prevent.

2. source_scope.markup_mask is not currently a safe destination. It has its own hole — <p>{{ '<!--' }}</p> … <img alt=""> … {{ '-->' }} blanks the whole span — and that is what makes gates 35 and 36 affected. Six checkers (37, 39, 40, 42, 43, 44) each carry their own copy of <!--.*?-->; routing them into markup_mask today would fix their comment half and leave this half armed. Harden source_scope first, then consolidate at one site instead of eight.

Deliberately not done

• gate-7's string-literal variant — see above.
• gate-48's redundant short-circuit. The runner's own note argues the caller-STATE question subsumes the diff question, which would make the diff-signal count deletable entirely. That changes verdicts on real PRs and belongs in its own change.
• The other 43 affected gates. A wrong recipe repeated at scale is a defect multiplied at scale. Each needs its own anti-widening pair, because for several of them the mask that closes the false negative would delete the evidence.

Fleet consequence

Every gate-7, gate-25, gate-48 and gate-49 count in the fleet is suspect until a rerun on the fixed package — and for gate-45 the suspicion is repo-wide, because one comment in one stylesheet sets HYDRA_RM_GLOBAL_GUARD=1 and silences the gate for every file. [gate-N] PASS from before this survey is not necessarily a measurement of the code; it may be a measurement of the comments.

Also found while building the fixtures

gate-48's src/**/*.vue is a plain git pathspec, where * matches / too — so the glob requires a second slash and never matches src/Del.vue. Recorded in the suite comment; not fixed here.

[rubenvdlinde]

Local verification on this branch: tests/run-helper-suites.sh → 83 passed, 0 failed, 2 quarantined (both pre-existing and documented — test_gate_glob_recursion_fixtures.sh, test_gate_orphaned_capability_fixtures.sh, fixtures never authored). Plus gate-7's suite 138 → 145 tests, test_gate7_verb_object_guards.sh 10/10, and the 45–55 acceptance matrix ALL GREEN.