Skip to content

fix(gate-47): a file the diff DELETES has no test to co-change (procest#867: 7 findings, all D) - #485

Merged
rubenvdlinde merged 1 commit into
mainfrom
fix/gate-47-deleted-files
Aug 17, 2026
Merged

fix(gate-47): a file the diff DELETES has no test to co-change (procest#867: 7 findings, all D)#485
rubenvdlinde merged 1 commit into
mainfrom
fix/gate-47-deleted-files

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

The defect, measured

check_security_cochange.changed_lines() runs git diff -U0 and returns added
and removed lines undifferentiated, so a - line scored identically to a
+ line. Three consequences:

  • any deletion-only PR in any fleet repo reddens gate-47;
  • the finding is unclosable from the app repo — it is keyed to a base
    already in history, and the remedy the gate asks for ("co-change a test")
    cannot be performed on a file that no longer exists;
  • it self-clears on the next push once the base advances. That is the
    evidence disappearing, not a repair.

Reproduced against the current package (f935e2c) on procest#867, base
463181f0:

$ python3 scripts/lib/check_security_cochange.py 463181f0 <procest>
src/views/cases/components/ParticipantsSection.vue
src/views/cases/widgets/BevoegdhedenPanel.vue
src/views/complaints/ComplaintAnalyticsDashboard.vue
src/views/complaints/ComplaintDashboardWidget.vue
src/views/complaints/ComplaintDetail.vue
src/views/complaints/ComplaintList.vue
src/views/settings/tabs/KlachtcategorieenTab.vue

$ git diff --name-status -M 463181f0...HEAD -- <each of those>
D	src/views/cases/components/ParticipantsSection.vue
D	src/views/cases/widgets/BevoegdhedenPanel.vue
D	src/views/complaints/ComplaintAnalyticsDashboard.vue
D	src/views/complaints/ComplaintDashboardWidget.vue
D	src/views/complaints/ComplaintDetail.vue
D	src/views/complaints/ComplaintList.vue
D	src/views/settings/tabs/KlachtcategorieenTab.vue

All seven are D. The only matched line in each was the deleted
requesttoken: OC.requestToken header of a removed src/views/** component.

The change

scan() now reads git diff --name-status -M once — the call rename_map
was already making — and skips paths git reports as deleted outright.

     files = changed_files(base_ref, cwd)
     has_test = any(is_test_path(f) for f in files)
-    renames = rename_map(base_ref, cwd)
+    records = name_status(base_ref, cwd)
+    renames = renames_from(records)
+    deleted = deletions_from(records)
     security: list[str] = []
     for f in files:
+        if f in deleted:
+            continue
         if is_security_path(f):

rename_map() keeps its signature and behaviour; it is now a one-line wrapper
over the shared reader, alongside a new deleted_files(). No other module in
the package imports this one.

It is the D STATUS, not the - sign

Classifying on added lines only is the tempting one-liner and it is the wrong
fix
. Removing a requesttoken from a file that still ships is a genuine
security change that must still demand a test co-change, and that arm is
pinned by test_tp_removing_the_token_from_a_SURVIVING_file_still_fires.
Because -M is on, a moved file is an R record and never reaches the
deletion set — pinned by test_control_a_rename_is_not_read_as_a_deletion.

Controls — the gate must still be shown able to FAIL

All four run through the real bin/hydra-gates on throwaway fixtures, reading
the printed verdict line and the log artefact (not an exit status).

# fixture verdict after verdict before (origin/main)
1 adds requesttoken: OC.requestToken, no test co-change FAIL — 1 security-touching change(s) FAIL
2 removes it from a surviving file, no test co-change FAIL — 1 security-touching change(s) FAIL
3 deletes the whole file PASS FAIL — 1 security-touching change(s)
4 procest#867 vs 463181f0 0 findings 7 findings

Arm 3 was run against the pre-fix package at origin/main on the byte-identical
fixture and base, and FAILs there — so the difference is this change and not the
fixture. Arms 1 and 2 name the file in
hydra-gate-security-change-has-tests.log (29 bytes, non-empty).

Arm 4's fixture is not degenerate: the delta is 81 files, 77 of them D, and
the four surviving lib/Controller/*.php were opened and classified — so this is
a computed zero, not an unopened scope.

The ten new unit arms were run as a mutant against the pre-fix helper: 4 fail
and 2 error there, while every anti-widening arm (_tp_ / _control_) passes
both ways. A suite that only ever saw the fixed code would prove nothing.

Package suites re-run

suite result
test_check_security_cochange.py 42 tests, OK
every scripts/lib/test_*.py (50 files) 0 failing
test_gate_discarded_counts_and_empty_deltas.sh 37 assertions, 0 failures — incl. "anti-widening: gate-47 still FAILs a security-annotation change shipped with no test co-change"
test_gate_empty_scope_never_passes.sh ALL PASS, 0 bad
test_gate_acceptance_matrix.sh 166 passed, 0 failed

Deliberately NOT changed

The shell probe stays as it is. A deletion-only delta now reports PASS
rather than NOT APPLICABLE. The #242/#401(b) doctrine says a gate must not
pass over a scope it never opened — but this gate did open the hunks and
computed that no surviving code changed. That is the same reasoning under which
_ARM6_ALLOWED already blesses a gate-47 PASS ("classified the docs-only hunks
and found no security change. Computed."), and the same verdict the in-module
pure-rename exemption already produces. Measured both ways; say the word if you
want it declining instead and it is a two-line probe change.

How this could weaken real detection — stated, not hidden

  1. Deleting a file can itself be a security regression. Dropping
    lib/Middleware/CsrfMiddleware.php removes a guard, and gate-47 no longer
    speaks to that. It never could: its remedy is "co-change a test" and there is
    nothing left to test. Recorded as a KNOWN LIMIT in scan()'s docstring, with
    a test (test_tp_deleting_a_file_under_an_auth_path_is_also_exempt) that
    makes the behaviour explicit rather than incidental. If the fleet wants
    deletion-of-a-guard caught, it needs a different gate with a different remedy.
  2. The exemption is per file, not per PR. A PR that deletes a component AND
    drops an auth attribute from a surviving controller is still a finding —
    otherwise "delete something" would be a universal opt-out. Pinned by
    test_tp_deleting_one_file_does_not_excuse_editing_another.
  3. Pre-existing, untouched by this PR: has_test counts a deleted test
    file as a test co-change, because changed_files() is status-blind. So a PR
    that deletes a test and adds IUserSession elsewhere passes. That hole
    predates this change and I left it alone rather than widen the diff; worth a
    follow-up.
  4. gate-48 measured on the same shape (a whole lib/Controller/*.php
    deletion carrying #[NoCSRFRequired], with an unprotected fetch() DELETE
    caller in the tree) already reports PASS — it does not share this defect, so
    no sibling change is in this PR.

Rules observed

Edit/Write only, no scripted code edits. Named paths staged. No
Co-Authored-By. Fresh worktree off origin/main. Not merged — this repo
is consumed live at @main by all 18 fleet apps.

`changed_lines()` runs `git diff -U0` and returns added and removed lines
undifferentiated, so a `-` line scored identically to a `+` line. Any
deletion-only PR in any fleet repo therefore reddened gate-47, with a
finding that is unclosable from the app repo (it is keyed to a base already
in history) and that self-clears on the next push once the base advances —
the evidence disappearing, not a repair.

Measured 2026-08-16 on procest#867 against base 463181f0:
`FAIL — 7 security-touching change(s) without a test co-change`. All seven
were `D` records, and every matched line was the deleted
`requesttoken: OC.requestToken` header of a removed src/views/** component.
After this change the same base and head report 0, over a diff that still
contains 81 files and four surviving lib/Controller/*.php the gate did open.

THE EXEMPTION IS THE `D` STATUS, NOT THE `-` SIGN. Classifying on added
lines only is the tempting one-liner and it is the wrong fix: removing a
`requesttoken` from a file that still ships is a genuine security change
that must still demand a test. `scan()` now reads `--name-status -M` once
(the call `rename_map` already made) and skips only paths git reports as
deleted outright; `-M` means a move is an `R` record and never reaches it.

Controls, all through bin/hydra-gates on throwaway fixtures:

  1 add `requesttoken`, no test               FAIL  (unchanged)
  2 remove it from a SURVIVING file, no test  FAIL  (unchanged)
  3 delete the whole file                     PASS  (was FAIL @f935e2c)
  4 procest#867 vs 463181f0                   0 findings (was 7)

Arm 3 was run against the pre-fix package at origin/main on the identical
fixture and FAILs there, so the difference is this change and not the
fixture. The ten new unit arms were run against the pre-fix helper as a
mutant: 4 fail and 2 error, while every anti-widening arm passes both ways.

Package suites re-run green: test_check_security_cochange.py 42/42, all 50
scripts/lib/test_*.py, test_gate_discarded_counts_and_empty_deltas.sh 37/0
(including "gate-47 still FAILs a security-annotation change shipped with
no test co-change"), test_gate_empty_scope_never_passes.sh, and
test_gate_acceptance_matrix.sh 166/0.

KNOWN LIMIT, recorded in scan()'s docstring rather than hidden: deleting a
file can itself be a security regression, and this gate no longer speaks to
that. It never could — its remedy is "co-change a test" and there is
nothing left to test. gate-48 was measured on the same shape (a whole
lib/Controller deletion with an unprotected fetch() caller) and already
declines correctly, so no sibling change is needed.
@rubenvdlinde
rubenvdlinde merged commit 742f370 into main Aug 17, 2026
35 checks passed
rubenvdlinde added a commit that referenced this pull request Aug 27, 2026
test_gate_acceptance_matrix.sh failed this branch, and it was right to. A gate
in the declared inventory with no planted/clean bundle is a gate that can stop
working quietly, which is the exact property that suite exists to deny. Its own
header makes the argument: the 2026-08-11 sweep found defects only in gates
that had no repo-shaped fixture, and that is a selection effect rather than a
coincidence.

The two arms differ in ONE thing: whether l10n/nl.json carries a key for the
manifest's `Flow` label. The manifests are byte-identical, so a gate grading on
manifest size, on key count, or on anything but which strings are covered
misgrades this pair.

`Dashboard` is translated in BOTH arms deliberately. Without it the planted
catalogue would be near-empty, and a gate that fired on "catalogue looks
unpopulated" rather than on the specific uncovered string would pass the
fixture for the wrong reason.

The subject is `Flow`, not a count. This gate exists because the failure is
silent — a manifest string with no key falls back to its English source, so the
page renders English and nothing errors. "1 uncovered string" tells nobody
which label a Dutch user is reading in English.

The pair is a real regression, not an invention. buildiq #485 added a Flows
settings section on 2026-08-27, putting `Flow` in the manifest with no nl.json
key, and it merged green: check:l10n-js compares nl.json to nl.js, both were at
1,045 keys, both missing the string, perfectly in sync. keepiq #448 did the
same hours earlier with `Registered by` and `Requested`.

clean/ maps `Flow -> "Flow"` rather than inventing a Dutch word: decidiq,
dossiq, openregister, keepiq, portaliq, pipelinq and filinq all carry that
mapping, and a fixture whose passing state disagreed would encode the wrong
rule.

Verified locally: the matrix suite now passes 193 assertions, including
  [manifest-l10n-coverage/planted] gate-99 FAIL and NAMES 'Flow'
  [manifest-l10n-coverage/clean]   gate-99 PASS
rubenvdlinde added a commit that referenced this pull request Aug 27, 2026
…oing (#604)

* feat(gates): gate-99 manifest-l10n-coverage — the check nothing was doing

A manifest string with no key in l10n/nl.json renders its ENGLISH source to a
Dutch user, and nothing reports it.

The l10n extractor scans .vue/.js/.ts for `t('<app>', '...')` calls. The
manifest is not source it reads; it is DATA THE RENDERER WALKS. CnAppNav
translates `menu[].label`, CnPageHeader a page's `title` and `description`,
CnWalkthrough a step's `title` / `body` / `task` — each through the app's own
translate function, each looking up a key the extractor never saw. The fallback
is the English source, silently.

`check:l10n-js` DOES NOT COVER THIS AND CANNOT. It compares l10n/nl.json to the
generated l10n/nl.js, and a string absent from BOTH is perfectly in sync.
Measured on buildiq: both files at 1,045 keys, both missing `Flow`, check
green.

WHY THIS EXISTS. A fleet sweep translated ~3,900 manifest strings across 17
apps and left every one at zero missing. Within hours two had regressed, not
from old debt but from the next PR that added a string:

    keepiq  #448  "Registered by", "Requested"
    buildiq #485  "Flow"

Both merged green. Only humaniq had a check that would have failed, because
someone hand-wrote one for that app. Porting it sixteen times is sixteen
chances to miss one, so it goes here instead.

VERIFIED AGAINST THE REGRESSION IT EXISTS FOR, not a synthetic fixture:

    buildiq at the regressed commit  rc=1, and the finding reads
                                     "FAIL no nl.json key: Flow"
    hermiq (swept clean)             rc=0 over 150 strings
    dossiq (swept clean)             rc=0 over 392 strings
    manifest with no strings         checked 0 -> na, never PASS
    crashed checker                  SKIP(wiring) "UNVERIFIED", never PASS

Skips `{{placeholder}}` values: they are substituted at render time and
translating one breaks the template. humaniq declares five. Skips `_meta`,
which is per-fragment provenance, never rendered. Reads src/manifest.d/*.json
as well as src/manifest.json — the fragments are merged at runtime via
require.context and shillinq has 87 of them.

Asserts nl only. Dutch is what this fleet ships; demanding every European
locale would make the gate unpassable rather than useful.

* test(gates): the repo-shaped fixture gate-99 shipped without

test_gate_acceptance_matrix.sh failed this branch, and it was right to. A gate
in the declared inventory with no planted/clean bundle is a gate that can stop
working quietly, which is the exact property that suite exists to deny. Its own
header makes the argument: the 2026-08-11 sweep found defects only in gates
that had no repo-shaped fixture, and that is a selection effect rather than a
coincidence.

The two arms differ in ONE thing: whether l10n/nl.json carries a key for the
manifest's `Flow` label. The manifests are byte-identical, so a gate grading on
manifest size, on key count, or on anything but which strings are covered
misgrades this pair.

`Dashboard` is translated in BOTH arms deliberately. Without it the planted
catalogue would be near-empty, and a gate that fired on "catalogue looks
unpopulated" rather than on the specific uncovered string would pass the
fixture for the wrong reason.

The subject is `Flow`, not a count. This gate exists because the failure is
silent — a manifest string with no key falls back to its English source, so the
page renders English and nothing errors. "1 uncovered string" tells nobody
which label a Dutch user is reading in English.

The pair is a real regression, not an invention. buildiq #485 added a Flows
settings section on 2026-08-27, putting `Flow` in the manifest with no nl.json
key, and it merged green: check:l10n-js compares nl.json to nl.js, both were at
1,045 keys, both missing the string, perfectly in sync. keepiq #448 did the
same hours earlier with `Registered by` and `Requested`.

clean/ maps `Flow -> "Flow"` rather than inventing a Dutch word: decidiq,
dossiq, openregister, keepiq, portaliq, pipelinq and filinq all carry that
mapping, and a fixture whose passing state disagreed would encode the wrong
rule.

Verified locally: the matrix suite now passes 193 assertions, including
  [manifest-l10n-coverage/planted] gate-99 FAIL and NAMES 'Flow'
  [manifest-l10n-coverage/clean]   gate-99 PASS
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