fix(gates 6, 17, 56, 57): a comment naming the call is not the call (#422) - #444
Merged
Conversation
…422) Four more instances of the #415 class, all in the FALSE-NEGATIVE direction: the gate looks for a positive signal — a call, a class, a method, a line of logic — and prose containing the words is accepted as the signal. Every one was reproduced first, with a fixture whose only "evidence" lives in a comment. gate-6 orphan-auth an unreferenced requiresChairAuthorization() FAIL — defined-but-never-called + "// TODO: we should call $this->authz->requiresChairAuthorization($uid) here before advancing. Not done yet." PASS <- the defect gate-56 register-handler-resolution handler -> InvoiceGuard::evaluate, no method FAIL — guard-method-not-found + "* TODO: implement function evaluate() here" PASS <- the defect class DELETED, class and method quoted in one UNINDENTED /* */ removal note PASS <- the defect gate-57 orphaned-write-capability an uncalled postInvoice() FAIL — orphaned-write-capability + "* TODO: we should call $this->fooService->postInvoice($id) here" PASS <- the defect + "/* returns true; never throws */" in the SIGNATURE -> reads as abstract, never judged PASS <- the defect gate-17 redundant-controller a one-call pass-through FAIL — pass-through + an UNPREFIXED interior line of a /* */ block survives as "significant code" PASS <- the defect + a rescue phrase in that block ("requireAdmin() is handled by middleware") reads as a guard PASS <- the defect THE SENTENCE THAT SWITCHES EACH GATE OFF IS THE SENTENCE ADMITTING THE DEBT. gate-6's whole framing is "an unwired guard is identical to no guard", and the note saying the guard is not wired reported it wired. gate-56's is "a handler naming a class that is not there fails at runtime", and the note saying the class was removed closed it. MEASURED IN THE FLEET, AND IT IS NOT HYPOTHETICAL. Across procest, opencatalogi, openregister, softwarecatalog, docudesk and larpingapp the counts move by exactly one — openregister gate-57 7 -> 8: lib/Service/File/FileOwnershipHandler.php:262 method=transferFolderOwnershipIfNeeded rule=orphaned-write-capability Every "caller" of that method in the repository is a comment. Four of them: FolderManagementHandler.php:221 // TODO: Call $this->fileService->transferFolderOwnershipIfNeeded($folderNode) FolderManagementHandler.php:312 // TODO: Call ...($objectFolder) FolderManagementHandler.php:490 // TODO: Call ...($objectFolder) FolderManagementHandler.php:562 // TODO: Call ...($node) Folder ownership is not transferred to the OpenRegister system user at any of the four sites that say it should be, and the gate that exists to find exactly that reported it reachable BECAUSE the four TODOs named it. Verified by hand; it is a real finding, not a new false positive. WHAT THE MASK DOES NOT DO, PER GATE. A wrong recipe repeated at scale is a defect multiplied at scale, so each gate carries its own anti-widening arms and each keeps string literals for its own stated reason: gate-6 routes.php is still read UNMASKED. A routed controller action has no `->method(` anywhere, ever — the router reaches it through the STRING 'liveTile#validateSource'. Blanking literals in the caller corpus does not touch that file, but generalising "strings are not evidence" across this module would put every routed action back to being reported dead (#290). Arm 7 is what makes that loud. gate-17 the docblock is still read from the ORIGINAL text: `@spec exclude` lives in a comment BY DESIGN and is the author's only correct remedy. gate-57 the caller index is deliberately over-permissive because its fail-safe direction is to SUPPRESS — this gate accuses live code of being dead, and a verdict acted on deletes working code. gate-56 a class or function DECLARATION is never spelled inside a literal, so blanking them would close nothing that is not already closed. NOT A SIXTH HAND-ROLLED STRIPPER. All four route through source_scope.php_mask, which already knows `#[` opens a PHP 8 ATTRIBUTE and that `//` inside 'https://x' opens nothing. gate-57's local `_blank_php_comments` — clause-for-clause identical to php_mask — becomes a call to it, so the fifth dialect in this package is gone rather than joined.⚠️ FOUND WHILE BUILDING THE FIXTURE, AND IT IS THE FIX OVER-APPLYING. gate-17's METHOD_HEADER_RE used `^\s*public`, and in MULTILINE mode `\s` matches `\n` — so the match could begin at the start of any run of BLANK LINES above the declaration. Pre-existing and inert (true line 4, reported 3), until the body was comment-masked: a blanked DOCBLOCK is a run of blank lines, the reported line slid up into it, `_method_docblock` walked from the wrong place, found no `*/`, and a reason-bearing `@spec exclude` SILENTLY STOPPED EXEMPTING. Closing a false negative by breaking an author's only correct remedy is worse than the false negative. Anchored to `^[ \t]*`; arm 5 of the new suite is the arm that caught it. TESTS. 23 new arms. Reverted against origin/main — not `git checkout --`, which restores the COMMITTED file — eleven FLIP: gate-6 arms 2, 3, 4 (//, /* */, #) gate-56 arms 2, 3 gate-57 arms 2, 3, 4, 5 gate-17 arms 2, 3 + the blank-line line-number arm and twelve pass either way and are labelled CONTROLS in their own docstrings, including the near-miss (gate-17 arm 3b: the same sentence three characters along never reaches the question, because the ObjectService test short-circuits before RESCUE_PATTERNS). Saying so is the point — an arm that cannot fail is not proof that the gate cannot. gate-17 had NO suite at all before this; test_detect_redundant_controllers.py is new and tests/run-helper-suites.sh discovers it with no workflow edit. Refs #422, #415. Sibling PRs cover the runner-inline gates and the registry/config gates; gate-38 is BLOCKED on #424 (its fix needs markup masking, and routing it into source_scope.markup_mask today would fix its comment half and leave the delimiter half armed).
One-line conflict: both sides added an import from source_scope — this branch needs php_mask, main's #439 needs mask_html_comments. Kept both, because they are independent helpers for different masking jobs, not competing versions of one. Verified after the resolve rather than assumed: source_scope defines both symbols, this file calls each exactly once, the module parses, and the checker still exits non-zero on a nonexistent app root (an unrunnable checker must not read as a clean repo).
The previous commit says the merge was verified by checking that the checker "still exits non-zero on a nonexistent app root". That claim is wrong twice over, and the output contradicting it was on my screen when I wrote it. 1. IT EXITED 0, not non-zero. I had written the expected result into the label next to the command and did not compare it to the value printed. 2. THE TEST WAS VOID ANYWAY. This checker ignores its path argument — passing /nonexistent-app-root printed 'root=/home/rubenlinde/gh444', the working directory. It never received a bad root, so nothing about bad roots was exercised. 3. AND NON-ZERO WOULD HAVE BEEN THE WRONG EXPECTATION. run-hydra-gates.sh says so directly: 'This helper always exits 0 when it runs, so this is a crash, not a finding count.' The runner reads a non-zero exit as a CRASH and skips gate-57 as a wiring gap. Exit 0 is the contract. What the merge resolution actually rests on, all re-checked: source_scope defines both php_mask and mask_html_comments, this file calls each exactly once, the module parses, and the checker runs and reports its identity line. No code changes — this commit exists so the branch's history does not carry an assertion that its own evidence refutes.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 13, 2026
…reads must stay split (#447) #445 (#422) and #434 (#339) both changed the SAME arm of gate-3, three lines apart, so they merged cleanly and the interaction never appeared in a diff. Both are now on main and the behaviour there is CORRECT — but nothing asserts it, and this is a shape that breaks silently. The arm reads two texts in one coordinate system, and only `php_mask` being line- and offset-preserving makes that legal: the body questions -> the MASK _sig / _sig_region / _body / _count ("is $uid referenced in CODE?") the exemption -> the ORIGINAL --file "$f" --line "${_line_no}" (#434's `@param … (unused)` marker, which lives in a COMMENT BY DESIGN) Repoint the exemption at the mask — a one-word edit, and the obvious "consistency" cleanup for anyone who notices the arm reading two different files — and the marker vanishes into blanks. The exemption silently stops working and procest's three GuardEvaluatorInterface implementors go red WITH NO CLOSING ACTION AVAILABLE: deleting the parameter breaks the interface and every call site, referencing it pointlessly is dead code written to satisfy a stub detector. #434's own note says neither is a fix, which is why it added the exemption in the first place. That is gate-17's `@spec exclude` trap, which #444 hit for real in the sibling gate of the same sweep: masking the body slid the reported line into the blanked docblock, `_method_docblock` walked from the wrong place, and a reason-bearing exclusion silently stopped exempting. Caught there by an anti-widening arm. This is the same arm for gate-3. CONTROL 6b — a supertype-declared `authorize(string $uid, …)` whose docblock reads `@param string $uid Current user UID (unused by this implementor).` must still be EXEMPT with the mask in place. MUTATION-CHECKED, so it is a coupling and not a comment: with `--file` pointed at `${_stub_code}` the arm reports `FAIL — 1` (9 passed, 1 failed); restored, 10 passed, 0 failed. Measured against main at 43ecb0c. No production code changes — one arm, in a suite tests/run-helper-suites.sh already discovers. Refs #422, #339, #445, #434. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
rubenvdlinde
added a commit
that referenced
this pull request
Aug 13, 2026
…isoning regression CodeQL scored 12 NEW HIGH alerts against this PR, `actions/cache-poisoning/poisonable-step`, all inside quality.yml and none of them in code this PR wrote. main scores 0 for `language:actions` and PRs #444, #446 and #447 were clean, so this was the new workflow and not a pre-existing pattern newly surfaced. The mechanism: quality.yml needs `contents: write` + `actions: write` at run-CREATION time, so a `fixture/**` caller hands its ~12 `actions/cache` steps an entry point reachable from a non-default branch — and Actions caches are shared with the default branch that later restores them. In a repository 23 others consume at @main, that is not worth a permanent leg counter. The measurement it existed to produce is already taken, on run 31734347530: 6 legs from info-32-34.xml, 4 from info-32-33.xml, stable34 absent from the second. The recipe for repeating it — and the two ways to get it wrong — is now documented on the `matrix-derivation` job in quality-resolve-probe.yml.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 13, 2026
…#448) * feat(quality): derive the Nextcloud test matrix from appinfo/info.xml The tested range and the declared range were two hand-maintained lists, and the fleet broke the agreement between them in BOTH directions inside a single programme: 18 apps declared NC 32-34 while testing stable31/32, and then the migration that fixed it replaced the list with '["stable34"]' and 16 apps stopped testing their own declared floor. Both were detectable. Neither was detected, because the two halves lived in two files that could be edited independently. quality.yml now derives the range from appinfo/info.xml with icewind1991/nextcloud-version-matrix, pinned by SHA (8a7bac6 = v1.3.2), in a new `nextcloud-matrix` job. `nextcloud-test-refs` survives as an explicit override; leaving it unset — the new default — derives. The action's `branches` output is OLDEST-FIRST. Six sites read `fromJSON(inputs.nextcloud-test-refs)[0]` as THE single server for newman, playwright, journeydoc-capture and the coverage guard, and newest-first was deliberate there. Consuming `branches` positionally would have moved all four onto stable32 silently. Those six sites now read `needs.nextcloud-matrix.outputs.single-server`, which is the action's own `branches-max` on the derived path — explicit, not positional. gate-65 rule 11 changes in step. `test-matrix-not-declared` is retired: an app that passes no override now derives, and a derived matrix is strictly better than a declared one. It is replaced by `test-matrix-neither-derived-nor-declared`, which fires only when an app neither declares a matrix nor calls the shared workflow. An override that disagrees with info.xml still fails, both directions, exactly as before. * fix(fixture): serialise the two matrix callers — quality.yml's concurrency group cancelled the control Measured on run 31734110346: quality.yml declares `concurrency: quality-${{ github.ref }}` with cancel-in-progress, so two calls on one ref share the group and the second cancels the first. Every job under `4 legs` came back cancelled and its PHPUnit row still carried the literal ${{ matrix.nextcloud-ref }} — the control measured nothing while looking like it had run. * fix: withdraw the leg-count fixture workflow — it was a real cache-poisoning regression CodeQL scored 12 NEW HIGH alerts against this PR, `actions/cache-poisoning/poisonable-step`, all inside quality.yml and none of them in code this PR wrote. main scores 0 for `language:actions` and PRs #444, #446 and #447 were clean, so this was the new workflow and not a pre-existing pattern newly surfaced. The mechanism: quality.yml needs `contents: write` + `actions: write` at run-CREATION time, so a `fixture/**` caller hands its ~12 `actions/cache` steps an entry point reachable from a non-default branch — and Actions caches are shared with the default branch that later restores them. In a repository 23 others consume at @main, that is not worth a permanent leg counter. The measurement it existed to produce is already taken, on run 31734347530: 6 legs from info-32-34.xml, 4 from info-32-33.xml, stable34 absent from the second. The recipe for repeating it — and the two ways to get it wrong — is now documented on the `matrix-derivation` job in quality-resolve-probe.yml.
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
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.
fix(gates 6, 17, 56, 57): a comment naming the call is not the call (#422)
Four more instances of the #415 class, all in the FALSE-NEGATIVE direction:
the gate looks for a positive signal — a call, a class, a method, a line of
logic — and prose containing the words is accepted as the signal. Every one
was reproduced first, with a fixture whose only "evidence" lives in a comment.
gate-6 orphan-auth
an unreferenced requiresChairAuthorization() FAIL — defined-but-never-called
+ "// TODO: we should call
$this->authz->requiresChairAuthorization($uid)
here before advancing. Not done yet." PASS <- the defect
gate-56 register-handler-resolution
handler -> InvoiceGuard::evaluate, no method FAIL — guard-method-not-found
+ "* TODO: implement function evaluate() here" PASS <- the defect
class DELETED, class and method quoted in one
UNINDENTED /* */ removal note PASS <- the defect
gate-57 orphaned-write-capability
an uncalled postInvoice() FAIL — orphaned-write-capability
+ "* TODO: we should call
$this->fooService->postInvoice($id) here" PASS <- the defect
+ "/* returns true; never throws */" in the
SIGNATURE -> reads as abstract, never judged PASS <- the defect
gate-17 redundant-controller
a one-call pass-through FAIL — pass-through
+ an UNPREFIXED interior line of a /* */ block
survives as "significant code" PASS <- the defect
+ a rescue phrase in that block ("requireAdmin()
is handled by middleware") reads as a guard PASS <- the defect
THE SENTENCE THAT SWITCHES EACH GATE OFF IS THE SENTENCE ADMITTING THE DEBT.
gate-6's whole framing is "an unwired guard is identical to no guard", and the
note saying the guard is not wired reported it wired. gate-56's is "a handler
naming a class that is not there fails at runtime", and the note saying the
class was removed closed it.
MEASURED IN THE FLEET, AND IT IS NOT HYPOTHETICAL. Across procest,
opencatalogi, openregister, softwarecatalog, docudesk and larpingapp the
counts move by exactly one — openregister gate-57 7 -> 8:
Every "caller" of that method in the repository is a comment. Four of them:
Folder ownership is not transferred to the OpenRegister system user at any of
the four sites that say it should be, and the gate that exists to find exactly
that reported it reachable BECAUSE the four TODOs named it. Verified by hand;
it is a real finding, not a new false positive.
WHAT THE MASK DOES NOT DO, PER GATE. A wrong recipe repeated at scale is a
defect multiplied at scale, so each gate carries its own anti-widening arms
and each keeps string literals for its own stated reason:
gate-6 routes.php is still read UNMASKED. A routed controller action has
no
->method(anywhere, ever — the router reaches it through theSTRING 'liveTile#validateSource'. Blanking literals in the caller
corpus does not touch that file, but generalising "strings are not
evidence" across this module would put every routed action back to
being reported dead (#290). Arm 7 is what makes that loud.
gate-17 the docblock is still read from the ORIGINAL text:
@spec excludelives in a comment BY DESIGN and is the author's only correct
remedy.
gate-57 the caller index is deliberately over-permissive because its
fail-safe direction is to SUPPRESS — this gate accuses live code of
being dead, and a verdict acted on deletes working code.
gate-56 a class or function DECLARATION is never spelled inside a literal,
so blanking them would close nothing that is not already closed.
NOT A SIXTH HAND-ROLLED STRIPPER. All four route through
source_scope.php_mask, which already knows
#[opens a PHP 8 ATTRIBUTE andthat
//inside 'https://x' opens nothing. gate-57's local_blank_php_comments— clause-for-clause identical to php_mask — becomes acall to it, so the fifth dialect in this package is gone rather than joined.
gate-17's METHOD_HEADER_RE used
^\s*public, and in MULTILINE mode\smatches
\n— so the match could begin at the start of any run of BLANKLINES above the declaration. Pre-existing and inert (true line 4, reported 3),
until the body was comment-masked: a blanked DOCBLOCK is a run of blank lines,
the reported line slid up into it,
_method_docblockwalked from the wrongplace, found no
*/, and a reason-bearing@spec excludeSILENTLY STOPPEDEXEMPTING. Closing a false negative by breaking an author's only correct
remedy is worse than the false negative. Anchored to
^[ \t]*; arm 5 of thenew suite is the arm that caught it.
TESTS. 23 new arms. Reverted against origin/main — not
git checkout --,which restores the COMMITTED file — eleven FLIP:
gate-6 arms 2, 3, 4 (//, /* */, #)
gate-56 arms 2, 3
gate-57 arms 2, 3, 4, 5
gate-17 arms 2, 3 + the blank-line line-number arm
and twelve pass either way and are labelled CONTROLS in their own docstrings,
including the near-miss (gate-17 arm 3b: the same sentence three characters
along never reaches the question, because the ObjectService test short-circuits
before RESCUE_PATTERNS). Saying so is the point — an arm that cannot fail is
not proof that the gate cannot.
gate-17 had NO suite at all before this; test_detect_redundant_controllers.py
is new and tests/run-helper-suites.sh discovers it with no workflow edit.
Refs #422, #415. Sibling PRs cover the runner-inline gates and the
registry/config gates; gate-38 is BLOCKED on #424 (its fix needs markup
masking, and routing it into source_scope.markup_mask today would fix its
comment half and leave the delimiter half armed).