fix(gate-14): brace style is not supposed to be an input to route reachability - #391
Merged
Merged
Conversation
…chability
Invariant 1 asks "does this method return a Response?" by buffering the
signature line plus up to twelve following lines and grepping the buffer for
`: ...Response`. Both of the loop's stop conditions are tested against `nxt` —
the lines it reads AHEAD — and neither is ever tested against the signature
line itself.
Under Allman braces that was harmless: the very next line is a lone `{`, so
the buffer stopped after one line. Under K&R the signature line already opens
the body, nothing stops the buffer, and it runs on through the body, the
closing brace and the next method's docblock until it reaches that method's
signature. The first method then inherits the SECOND method's return type.
MEASURED on openconnector#1229, mid-migration to Nextcloud's coding standard:
lib/Controller/UiController.php method=__construct
expected_route='ui#__construct' rule=missing-route
A constructor, reported as an unrouted endpoint. The `TemplateResponse` it was
credited with belongs to `makeSpaResponse()` eleven lines below. Running the
identical awk over the identical file in Allman braces prints nothing — so the
finding was a property of the brace style, and the reformat looked like it had
broken routing.
That matters beyond one PR. Nextcloud's coding standard IS K&R and the fleet is
adopting it app by app, so every app with a `__construct` followed within
twelve lines by a Response-returning action receives this phantom on its first
reformat — roughly two dozen PRs, each of which has to re-diagnose it.
THE FIX reads nothing ahead once the signature line ends in `{`. That is
correct rather than merely quieter: if the body opens on the signature line
then the whole signature — parameters, return type and all — is on that line,
so there is nothing further to find. A K&R method that genuinely returns a
Response still matches, on `$0`.
THE FIXTURE CARRIES BOTH ARMS, because "stop reading ahead" could have been
written as "skip K&R files", which would retire the gate for the entire fleet:
UiController K&R, `__construct` then a routed `dashboard(): TemplateResponse`
-> ui#__construct MUST NOT be reported
GadgetController K&R, `run(): JSONResponse`, routed nowhere
-> gadget#run MUST still be reported
and the suite asserts EXACTLY ONE finding, so the fix cannot pass by going
quiet.
MUTATION-CHECKED both ways, via HYDRA_GATES_RUNNER_UNDER_TEST:
pre-fix runner 45 passed, 2 FAILED — 2 findings, ui#__construct among them
fixed runner 47 passed, 0 failed — 1 finding, gadget#run
shellcheck is clean on both changed files.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 12, 2026
…ods (#395) (#399) * fix(gate-16): a coding-standard reformat is not a set of changed methods (#395) The changed-method set came from a plain `git diff -U0`, so when `nextcloud/coding-standard` moved every `{` onto its signature line, EVERY method in a migrated app read as modified and gate-16 demanded an `@spec` anchor on all of them. Third of this class today after gate-14 (#391) and gate-48 (#388); this one is the delta gate, so it is the one a formatting-only PR can genuinely regress. `git diff -w` does not help. The brace is a TOKEN THAT MOVED LINES, not whitespace whose width changed, so `foo()` and `foo() {` differ under `-w`. MEASURED, seven live `chore/nextcloud-coding-standard` PRs, base = each app's merge-base with development: procest#819 185 -> 0 openregister#2445 183 -> 0 pipelinq#809 374 -> 0 shillinq#532 274 -> 0 openconnector#1229 30 -> 0 openbuild#189 19 -> 0 scholiq#322 6 -> 0 total 1071 -> 0 THE NORMALISATION, applied to BOTH sides of a line-by-line comparison against the file's own base version, and INTERSECTED with git's answer so it can only ever remove lines from the scope: * brace placement, K&R vs Allman; * indentation and intra-line spacing, INCLUDING cast and operator spacing; * a trailing comma at the end of a PHP line; * quote style, only where the two spellings are the same string; * a PHP statement re-wrapped across lines — same characters, new line breaks. DELIBERATELY NOT NORMALISED: anything inside a string literal; interpolating or escaped literals (`"$x"` is not `'$x'`, `"\n"` is not `'\n'`); trailing commas in JS (elision) and re-wraps in JS (ASI); a re-wrap across a `//` comment, because inserting a break after one UNCOMMENTS what followed; and type respellings such as `string|null` -> `?string`, which are equivalent but are on a SIGNATURE line and would keep the list of rules growing. EVERY RULE HAS A POSITIVE CONTROL, because a normalisation that swallowed a real edit would be worse than the over-reporting it replaces: 17 assertions in `NormalisationTest` pair each rule with a change that must still be seen through it (a value, a new parameter, a string's CONTENT, an operand inside a re-wrap), plus two end-to-end arms and a wrapper-level arm 4 whose `.knr` and `.knr-changed` fixtures differ in exactly one character. MUTATION-CHECKED both ways against the pre-fix checker: python suite 36 -> 2 failures + 17 errors fixed: 37 passed shell suite 21 passed / 3 FAILED fixed: 24 passed / 0 INDEPENDENT LAYER — PHP's own tokeniser, not this normaliser: of the 399 files the 1071 suppressed findings named, 396 are token-identical to their base once whitespace, comments, trailing commas and quote style are set aside and `else if`/`elseif` plus import order are accounted for. All three exceptions are outside the suppressed methods: openconnector's `const` -> `public const` is class-level, and openregister's two `string|null` -> `?string` signatures are still IN scope after normalisation — they report nothing because both methods already carry an `@spec` tag, which is the gate working, not the gate blind. Cost: openregister's 2616-file diff goes from 24s to 54s, one `git show` per in-scope changed file. `--mode report`, the empty-scope contract (#361) and the COVERAGE line are untouched. * docs(hydra-gates): state what gate-16 counts as a change The delta-gate section said which gates need a base and what happens without one, but not what a delta gate treats as a change — which is the input #395 turned out to hinge on. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
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.
What
gate-14 invariant 1 buffers a method signature plus up to twelve following lines and greps the buffer for
: ...Response. Both loop stop conditions are tested against the lines it reads ahead and never against the signature line itself.Under Allman that was harmless — the next line is a lone
{. Under K&R the signature line already opens the body, so nothing stops the buffer: it runs through the body, the closing brace and the next method's docblock until it hits that method's signature, and the first method inherits the second one's return type.Measured
openconnector#1229, mid-migration to Nextcloud's coding standard:
A constructor reported as an unrouted endpoint. The
TemplateResponseit was credited with belongs tomakeSpaResponse()eleven lines below. The identical awk over the identical file in Allman braces prints nothing — the finding was a property of brace style, and the reformat looked like it had broken routing.Nextcloud's standard is K&R and the fleet is adopting it app by app, so every app with a
__constructfollowed within twelve lines by a Response-returning action gets this phantom on its first reformat — roughly two dozen PRs each re-diagnosing the same thing.The fix
Read nothing ahead once the signature line ends in
{. Correct rather than merely quieter: if the body opens on the signature line then the whole signature — parameters, return type and all — is on that line. A K&R method that genuinely returns a Response still matches, on$0.Both arms, because "stop reading ahead" could have been "skip K&R files"
New fixture
route-registration/knr-braces/:UiController__constructthen a routeddashboard(): TemplateResponse→ui#__constructMUST NOT be reportedGadgetControllerrun(): JSONResponse, routed nowhere →gadget#runMUST still be reportedThe suite asserts exactly one finding, so the fix cannot pass by going silent.
Mutation check (
HYDRA_GATES_RUNNER_UNDER_TEST)shellcheck clean on both changed files.