Skip to content

fix(gate-48): a re-indented #[NoCSRFRequired] is not a dropped one - #388

Merged
rubenvdlinde merged 1 commit into
mainfrom
fix/gate-48-reindent-false-positive
Aug 12, 2026
Merged

fix(gate-48): a re-indented #[NoCSRFRequired] is not a dropped one#388
rubenvdlinde merged 1 commit into
mainfrom
fix/gate-48-reindent-false-positive

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

The finding

gate-48's move-cancellation compared removed and added lines by raw bytes. php-cs-fixer re-indents every controller from four spaces to a tab, so the fleet-wide move to Nextcloud's coding standard emits

-    #[NoCSRFRequired]
+	#[NoCSRFRequired]

for every attribute in the app, and the helper read all of them as deletions.

Measured on ConductionNL/launchpad#106: the helper reports 25 "removals" against a tree whose NoCSRFRequired count is 43 before the PR and 43 after — nothing was dropped, everything moved one indent level. gate-48 is red for this reason alone on 8 of the 18 apps currently migrating, and the cheapest way out would have been a [hydra-gate-csrf-cochange exclude] waiver on each: a security gate waived fleet-wide over an indentation artefact.

Why the old rule is safe to reverse

The byte-exact rule carried a stated reason, pinned by test_whitespace_is_not_normalised_away:

A re-indented line is NOT the same line. Treating it as a move would let a reformat swallow a genuine deletion.

That reason does not survive multiset accounting, which is what the cancellation already does. A re-indentation contributes exactly one addition for each removal it causes, so the net count is unchanged. Delete one annotation inside an otherwise fully re-indented file and the removals outnumber the additions by one, leaving exactly one finding.

Cancellation now keys on the line content with leading/trailing whitespace stripped. Nothing else is normalised: the attribute list is still compared verbatim, so #[NoAdminRequired, NoCSRFRequired] cannot absorb a removed #[NoCSRFRequired], and cancellation is still per file.

Measurement (real launchpad diff)

findings tree count before → after
pre-fix 25 (all false) 43 → 43
post-fix 0 43 → 43
post-fix, one added attribute line deleted from the diff so the head genuinely loses it 1

The third row is the one that matters: the gate still catches a real removal.

Tests

The old assertion is reversed, with the reason recorded at the assertion rather than in a changelog. Four arms added; the load-bearing one is test_a_genuine_deletion_inside_a_full_reindent_still_reports — four removals, three additions, expects exactly one finding. That is the arm that proves the gate was relaxed and not disabled, and it is the arm to read first if this is ever revisited.

The new arms fail against the pre-fix helper (3 failures) and pass against the fixed one (25 tests, OK), so they discriminate rather than merely agreeing with whatever the code does.

gate-48's move-cancellation compared removed and added lines by RAW BYTES.
php-cs-fixer re-indents every controller from four spaces to a tab, so the
fleet-wide move to Nextcloud's coding standard emits

    -    #[NoCSRFRequired]
    +	#[NoCSRFRequired]

for every attribute in the app, and the helper read all of them as deletions.

Measured on launchpad#106: the helper reports 25 "removals" against a tree
whose NoCSRFRequired count is 43 before the PR and 43 after — nothing was
dropped, everything moved one indent level. gate-48 is red for this reason
alone on 8 of the 18 apps currently migrating, and the cheapest way out would
have been a [hydra-gate-csrf-cochange exclude] waiver on each: a security gate
waived fleet-wide over an indentation artefact.

The byte-exact rule carried a stated reason — "treating a re-indented line as a
move would let a reformat swallow a genuine deletion" — and that reason does
not survive MULTISET accounting, which is what the cancellation already does.
A re-indentation contributes exactly one addition per removal it causes, so the
net count is unchanged; delete one annotation inside an otherwise fully
re-indented file and the removals outnumber the additions by one, leaving
exactly one finding.

So cancellation now keys on the line content with leading/trailing whitespace
stripped. Nothing else is normalised: the attribute list is still compared
verbatim, so #[NoAdminRequired, NoCSRFRequired] cannot absorb a removed
#[NoCSRFRequired], and cancellation is still per FILE.

Measured on the real launchpad diff:
  pre-fix   25 findings, tree count 43 -> 43   (all false)
  post-fix   0 findings
  post-fix, with one added attribute line deleted from the diff so the head
  genuinely loses it: 1 finding — the gate still catches a real removal.

Tests: the old assertion is REVERSED, with the reason recorded at the
assertion. Four arms added, the load-bearing one being
test_a_genuine_deletion_inside_a_full_reindent_still_reports (four removals,
three additions, expects exactly one finding) — the arm that proves the gate
was relaxed and not disabled. The new arms fail against the pre-fix helper
(3 failures) and pass against it (25 tests OK), so they discriminate.
@rubenvdlinde
rubenvdlinde merged commit b745bf2 into main Aug 12, 2026
9 of 12 checks passed
rubenvdlinde added a commit to ConductionNL/portaliq that referenced this pull request Aug 12, 2026
…nav selector

The config commit replaced `nextcloud-test-refs` with `["stable34"]`. That
turned three jobs red, all three green on development (run 31574689088), so all
three are this change's doing. It also silently dropped the stable32 and
stable33 legs development had.

1. PHPUnit — AppInfoDependenciesTest::testFloorIsNotBelowTheOldestTestedVersion
   failed: "declares min-version=32 but the oldest Nextcloud this repo tests is
   34 ... Either raise min-version to 34 or add the missing leg(s) to the
   matrix." Took the second option, because the first is a support-policy change
   and this is a formatting chore. The matrix is now
   ["stable34", "stable32", "stable33"] — every major inside the declared 32-34
   range, which also satisfies testEveryTestedVersionIsInsideTheDeclaredRange
   from the other side.

2. E2E — "SettingsSection must contribute an entry to the admin navigation" was
   never true. The section registers fine; the locator was scoped to
   `#app-navigation`, which NC 34 does not render. Verified against
   nextcloud/server: apps/settings/templates/settings/frame.php is
   `<div id="app-navigation">` on stable32 and stable33, and on stable34 the
   same file is `<div id="settings-app"></div>` — the settings page became a Vue
   app, and apps/settings/src/views/SettingsNavigation.vue renders the list via
   `<NcAppNavigation>`, whose root is `id="app-navigation-vue"`. The selector now
   accepts either container, so it is honest on all three legs rather than on
   the two it happened to be written against. This is exactly why stable34 is
   first in the list: [0] is the server playwright gets.

Not fixed here, on purpose:

- gate-16 flags WooController::serve and ::servePath as changed methods missing
  @SPEC. They are pre-existing debt, not a regression: `git show
  origin/development:lib/Controller/WooController.php` has zero @SPEC tags in
  the file, and so does HEAD — the reformat only alphabetised one import and
  moved a brace (`git diff -w` on that file shows nothing else), which is enough
  to pull two long-untagged methods into a diff-scoped gate. gate-16 cannot be
  run on development for comparison — it reports NOT APPLICABLE with no diff by
  design (ADR-020) — so the base-tree read is the available evidence. Tagging
  them would be paying legacy debt inside a whitespace PR.

- gate-48 is a known false positive fixed upstream in ConductionNL/.github#388
  (merged, b745bf2), which apps pick up automatically via the floating
  hydra-gates ref. Measured here too: NoCSRFRequired count is 32 on
  origin/development and 32 on HEAD, identical per file, and the diff is 21
  removals against 21 identical additions plus 6 against 6 in docblocks — every
  one a four-spaces-to-tab re-indent. No waiver added.
rubenvdlinde added a commit to ConductionNL/portaliq that referenced this pull request Aug 12, 2026
* chore: adopt nextcloud/coding-standard, .editorconfig and NC 34

Configuration only. The reformat is the next commit on purpose, so
.git-blame-ignore-revs can name a revision containing nothing but whitespace.

- .php-cs-fixer.dist.php + conduction/coding-standard, which extends
  nextcloud/coding-standard and can only ADD to it — enforced by that package's
  invariant test, not by review.
- cs:check / cs:fix now run php-cs-fixer. They were aliases for phpcs/phpcbf,
  so the documented Nextcloud command reformatted code AWAY from Nextcloud's
  standard.
- nextcloud/coding-standard dropped as a direct dependency. It arrives
  transitively at a version conduction/coding-standard has tested against;
  declared directly it was a dead dependency with no config and no invocation.
- phpcs.xml is now a stub over the shared semantics-only ruleset, and the local
  phpcs-custom-sniffs/ copy is gone. The fleet was carrying six divergent
  versions of NamedParametersSniff.php — a custom RULE, not a setting.
- .editorconfig, verbatim from nextcloud/server. No fleet app had one, so an
  editor configured by someone's previous Nextcloud work defaulted to tabs,
  which the old ruleset then rejected.
- nextcloud/ocp -> ^34.0 and PHPUnit -> stable34. This app declared support for
  NC 34 while being analysed against 31, so a symbol REMOVED in 32/33/34 was
  invisible to the type checker. That is why the NC 34 removal of \OC::$server
  needed a hand-written PHPCS sniff.
- the stylelint glob is quoted, so stylelint expands it rather than the shell.
  Unquoted, src/**/ matches exactly one directory level and nested components
  are silently unlinted.

gate-65 (coding-standard-adoption) enforces all of the above from
ConductionNL/.github@main. This app failed it; with this commit it passes.

* style: reformat with nextcloud/coding-standard — whitespace only

Applied by php-cs-fixer with conduction/coding-standard. Tabs, same-line braces,
(int)$x, single-space concatenation, ordered imports — Nextcloud's dialect, which
this app now passes unchanged. 96 file(s), no behaviour change.

Isolated from the configuration change so .git-blame-ignore-revs can name a
revision that touches nothing but formatting. Reviewing it line by line is not a
useful activity; the previous commit is the review.

* chore: ignore the reformat commit in git blame

9a97095 touches 96 files and changes no behaviour. Without this, every line it
reflowed attributes to it and the real author is one --skip away.

GitHub honours the file automatically; locally it needs
`git config blame.ignoreRevsFile .git-blame-ignore-revs` once.

* fix: regenerate composer.lock for the new constraints

The previous commit changed composer.json without touching the lock, so
`composer install` refused with exit 4 and EVERY PHP job failed:

    Required (in require-dev) package "conduction/coding-standard" is not
    present in the lock file.
    Required (in require-dev) package "conduction/hydra-gates" is not present
    in the lock file.
    Required (in require-dev) package "nextcloud/ocp" is in the lock file as
    "v31.0.9" but that does not satisfy your constraint "^34.0".

Nothing was wrong with the reformat or the ruleset — the jobs never got as far
as running a tool. Measured on larpingapp#313 before this fix: phpcs, psalm,
phpstan and both PHPUnit legs red, all of them at `composer install`. Hydra
Gates passed in the same run, because it does not install composer
dependencies.

Now locked at conduction/coding-standard v1.0.0, conduction/hydra-gates v1.7.0,
nextcloud/ocp v34.0.2 — the last of which is the point of the exercise: this app
declares support for NC 34 and is now analysed against it.

* fix(appinfo): order info.xml elements per the App Store xs:sequence

The App Store's info.xsd declares <info> and its children as xs:sequence, so
element ORDER is significant. This file was rejected by
`xmllint --noout --schema info.xsd appinfo/info.xml`. Nextcloud's
lint-info-xml workflow validates against exactly that schema, and
ConductionNL/.github#383 adds the same check to the shared pipeline.

Elements were moved into the schema's order. Nothing was added, removed or
reworded; <version> and the <nextcloud> min/max-version declaration are
unchanged.

Verified: `xmllint --noout --schema info.xsd appinfo/info.xml` reports
"validates" (libxml2 2.12.10). The pre-change file failed the same command.

* fix(ci): adopt NC 34 by ADDING it to the matrix, and unpin the admin-nav selector

The config commit replaced `nextcloud-test-refs` with `["stable34"]`. That
turned three jobs red, all three green on development (run 31574689088), so all
three are this change's doing. It also silently dropped the stable32 and
stable33 legs development had.

1. PHPUnit — AppInfoDependenciesTest::testFloorIsNotBelowTheOldestTestedVersion
   failed: "declares min-version=32 but the oldest Nextcloud this repo tests is
   34 ... Either raise min-version to 34 or add the missing leg(s) to the
   matrix." Took the second option, because the first is a support-policy change
   and this is a formatting chore. The matrix is now
   ["stable34", "stable32", "stable33"] — every major inside the declared 32-34
   range, which also satisfies testEveryTestedVersionIsInsideTheDeclaredRange
   from the other side.

2. E2E — "SettingsSection must contribute an entry to the admin navigation" was
   never true. The section registers fine; the locator was scoped to
   `#app-navigation`, which NC 34 does not render. Verified against
   nextcloud/server: apps/settings/templates/settings/frame.php is
   `<div id="app-navigation">` on stable32 and stable33, and on stable34 the
   same file is `<div id="settings-app"></div>` — the settings page became a Vue
   app, and apps/settings/src/views/SettingsNavigation.vue renders the list via
   `<NcAppNavigation>`, whose root is `id="app-navigation-vue"`. The selector now
   accepts either container, so it is honest on all three legs rather than on
   the two it happened to be written against. This is exactly why stable34 is
   first in the list: [0] is the server playwright gets.

Not fixed here, on purpose:

- gate-16 flags WooController::serve and ::servePath as changed methods missing
  @SPEC. They are pre-existing debt, not a regression: `git show
  origin/development:lib/Controller/WooController.php` has zero @SPEC tags in
  the file, and so does HEAD — the reformat only alphabetised one import and
  moved a brace (`git diff -w` on that file shows nothing else), which is enough
  to pull two long-untagged methods into a diff-scoped gate. gate-16 cannot be
  run on development for comparison — it reports NOT APPLICABLE with no diff by
  design (ADR-020) — so the base-tree read is the available evidence. Tagging
  them would be paying legacy debt inside a whitespace PR.

- gate-48 is a known false positive fixed upstream in ConductionNL/.github#388
  (merged, b745bf2), which apps pick up automatically via the floating
  hydra-gates ref. Measured here too: NoCSRFRequired count is 32 on
  origin/development and 32 on HEAD, identical per file, and the diff is 21
  removals against 21 identical additions plus 6 against 6 in docblocks — every
  one a four-spaces-to-tab re-indent. No waiver added.
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>
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.

1 participant