Skip to content

fix(gate-16): a coding-standard reformat is not a set of changed methods (#395) - #399

Merged
rubenvdlinde merged 2 commits into
mainfrom
fix/gate-16-reformat-is-not-a-change
Aug 12, 2026
Merged

fix(gate-16): a coding-standard reformat is not a set of changed methods (#395)#399
rubenvdlinde merged 2 commits into
mainfrom
fix/gate-16-reformat-is-not-a-change

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Closes #395.

The defect

check_spec_coverage.py derived its changed-method set from a plain git diff -U0. nextcloud/coding-standard is K&R, so a reformat moves every { onto its signature line — every method reads as modified, and gate-16 asks all of them for an @spec anchor they never had.

git diff -w does not neutralise it: the brace is a token that moved lines, not whitespace whose width changed, so foo() and foo() { differ under -w. That is presumably why this survived the #388/#391 sweep. Third instance of the class today, and the only one in a delta gate — which is why a formatting-only PR can genuinely regress it.

Measured, on the seven live coding-standard PRs

Base = each app's merge-base with development. Before = check_spec_coverage.py at 50c7e7b; after = this branch.

app PR changed files before after
procest #819 930 185 0
openregister #2445 2616 183 0
pipelinq #809 761 374 0
shillinq #532 1304 274 0
openconnector #1229 725 30 0
openbuild #189 258 19 0
scholiq #322 399 6 0
1071 0

What is normalised — and what is not

Both sides of a line-by-line comparison against the file's own base version are normalised, and the result is intersected with git's own added-line set, so this can only ever remove lines from the scope, never add one.

Equated:

  • brace placement, K&R vs Allman;
  • indentation and intra-line spacing, including cast and operator spacing — php-cs-fixer both adds spacing ('a'.$b'a' . $b) and removes it ((array) $x(array)$x), which is why whitespace outside a literal is removed rather than collapsed. Collapsing to one space was measured on procest to recover 71 of 185; removal recovers 183;
  • a trailing comma at the end of a PHP line (the last 2 of procest's 185);
  • quote style, only where the two spellings are the same string;
  • a PHP statement re-wrapped across lines — the same characters, different line breaks.

Deliberately not equated, each with a test:

  • anything inside a string literal — 'a b''ab' is text a user reads;
  • an interpolating or escaped literal — "$x" is not '$x', "\n" is not '\n';
  • a trailing comma in JS (elision) or a re-wrap in JS (ASI: return alone on a line returns undefined);
  • a re-wrap across a // — inserting a break after one uncomments what followed: same characters, different program;
  • a respelled type, string|null?string. Equivalent, and equally a fixer rule — but it is a signature line and the list of such rules is open-ended. Measured: openregister has exactly two, both already @spec-tagged.

The anti-widening half

A gate that stops reporting is worse than one that over-reports, so every rule ships with a positive control proving a real change still travels through it:

  • 17 assertions in NormalisationTest pairing each rule with a change it must still see — a changed value through the brace move, a new parameter through the trailing comma, a string's content through the quote rule, a changed operand inside a re-wrap;
  • 2 end-to-end arms through run_gate on a real git repo: a fully reformatted file reports nothing, and the same reformat with one operator flipped still names totalWeights and still does not name buildLabel beside it;
  • arm 4 in the shell suite, through bin/hydra-gates, with two pre-conditions (the reformat rewrites 35 lines; report mode confirms the methods are in scope and untagged) so a silent PASS cannot be mistaken for a correct one. .knr and .knr-changed differ in one character.

Mutation-checked against the pre-fix checker, both suites:

python  pre-fix: 2 FAILED + 17 errors     fixed: 37 passed
shell   pre-fix: 21 passed / 3 FAILED     fixed: 24 passed / 0 failed

Is the suppression correct? An independent layer says yes

Not this normaliser but PHP's own tokeniser: for each of the 399 files the 1071 suppressed findings named, base and head token streams were compared with whitespace, comments, trailing commas and quote style set aside, else if/elseif merged and the import block excluded.

396 of 399 are token-identical. All three exceptions are outside the suppressed methods:

  • openconnector SynchronizationService.phpconstpublic const, class-level, inside no method;
  • openregister FolderManagementHandler::getRegisterFolderName and FileService::debugFindFileByIdstring|null?string. These are still in scope after normalisation. They report nothing because one carries an @spec tag and the other a reason-bearing @spec exclude. That is the gate working, not the gate blind.

Unchanged

--mode report, the empty-scope contract (#361 — no base is still NOT APPLICABLE, not a pass), the COVERAGE: line, and every existing assertion in both suites.

Cost

One git show per in-scope changed file. openregister's 2616-file diff: 24s → 54s. Batching through git cat-file --batch would recover most of it and was left out on purpose — a desynchronised batch parse could hand the comparison the wrong base text, and the failure mode of that is a wrong suppression.

One thing observed, not fixed here

spec_tags_removed() detects removals against BASE...HEAD (the merge base) but then reads the base text with git show BASE:<path> — the branch tip. Where development has moved on (openregister, shillinq, today), those are different commits. It changes no result on any of the seven PRs, so it is noted rather than folded into this PR.

Conduction Release Bot added 2 commits August 12, 2026 21:08
…ods (#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.
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.
@rubenvdlinde
rubenvdlinde merged commit bd50fb5 into main Aug 12, 2026
34 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/gate-16-reformat-is-not-a-change branch August 12, 2026 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gate-16 counts a brace-style reformat as a modified method: 1 finding becomes 185 on a formatting-only PR (sibling of #388/#391)

1 participant