Skip to content

fix(gate-22): one missing _note emitted THREE findings — a count is not a defect count - #254

Merged
rubenvdlinde merged 1 commit into
mainfrom
fix/gate-22-ajv-triplet
Aug 8, 2026
Merged

fix(gate-22): one missing _note emitted THREE findings — a count is not a defect count#254
rubenvdlinde merged 1 commit into
mainfrom
fix/gate-22-ajv-triplet

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Follows the finding-vs-defect thread from #238 and #228.

Cause

Ajv with allErrors: true reports the applicator keyword alongside the leaf cause. The manifest schema states its _note rule as a nested if/then/else (nextcloud-vue#315 relaxed it so a Cn[A-Z]\w+ component is self-documenting), so one missing _note on one page emitted three lines:

at /pages/0: must have required property '_note' (keyword=required)  ← real
at /pages/0: must match "else" schema (keyword=if)                   ← echo
at /pages/0: must match "then" schema (keyword=if)                   ← echo

That is how a gate run reported "240 violations" for roughly 132 defects: 51 missing _note entries contributed 153 lines between them. The merged AppHost schema also asserted additionalProperties twice over the same node, emitting an identical line twice at /.

An inflated count is not cosmetic. It is what makes a repo's manifest debt look insurmountable and stops people reading the log at all — the same reason gate-46's summary now prints distinct targets alongside raw findings.

Measured

Fixture: two type: custom pages, each missing _note.

findings exit
before 10 1
after 5 1

Same five real defects. The verdict does not change — only the line count.

Collapsing errors is deleting evidence unless it is provably lossless

That is the whole risk here, so each property has its own test:

  • an applicator error is dropped only when a concrete sibling exists at the same instancePath — a lone if failure really is the only signal at its path and still surfaces;
  • dedupe is by (path, keyword, message), not by path — three distinct defects at / all survive;
  • no non-empty error list can collapse to zero.

Blast radius

The five re-measured repos (portaliq, doriath, openconnector, larpingapp, procest) are all clean on gate-22 today, as is shillinq — its 240-violation run has since been burned down. So this changes no current verdict; it stops the next repo's number from being inflated threefold.

Tests

New suite test_check_manifest_ajv_collapse.js, auto-discovered by run-helper-suites.sh (27 → 28 suites, 0 failed). It is unit-covered without Ajv, runs end-to-end when Ajv resolves, and refuses to run rather than pass vacuously if the export is missing.

check_manifest.js now exports collapseAjvErrors when required as a module (require.main === module still runs main() as a script), so the helper is testable without Ajv and without validating a manifest as a side effect.

Four mutations:

mutation assertions that fail
collapse disabled (pre-fix behaviour) 4
applicator dropped unconditionally (lossy) 3
dedupe by path only 2
collapse returns [] (blind) 9

One methodology note worth passing on

My first M3 run reported FAILs=0 — an apparently surviving mutant. It had not survived: the mutation never applied, because my match string was wrong. A mutation that silently fails to apply is indistinguishable from a surviving mutant, and it reads as reassuring rather than alarming. Every mutation in the final run asserts old in source before writing, and the re-run killed all four.

The same check also caught two stray NUL bytes my own edit had introduced into the dedupe key — repaired at byte level, with an assertion that exactly two bytes changed. Both were caught before merge.

…ot a defect count

Ajv with `allErrors: true` reports the APPLICATOR keyword alongside the leaf
cause. The manifest schema states its `_note` rule as a NESTED if/then/else
(nextcloud-vue#315 relaxed it so a `Cn[A-Z]\w+` component is self-documenting),
so ONE missing `_note` on ONE page emitted THREE lines:

  at /pages/0: must have required property '_note' (keyword=required)  <- real
  at /pages/0: must match "else" schema (keyword=if)                   <- echo
  at /pages/0: must match "then" schema (keyword=if)                   <- echo

That is how a gate run reported "240 violations" for roughly 132 defects: 51
missing `_note` entries contributed 153 lines between them. The merged AppHost
schema also asserted `additionalProperties` twice over the same node, emitting
an identical line twice at `/`.

An inflated count is not cosmetic. It is what makes a repo's manifest debt look
insurmountable and stops people reading the log at all — the same reason
gate-46's summary now prints distinct targets alongside raw findings.

MEASURED on a fixture with two custom pages each missing `_note`: 10 findings
-> 5, the same 5 real defects, exit code unchanged at 1.

COLLAPSING ERRORS IS DELETING EVIDENCE UNLESS IT IS PROVABLY LOSSLESS, so:

  * an applicator error is dropped ONLY when a concrete sibling exists at the
    SAME instancePath. A lone `if` failure really is the only signal at its
    path and still surfaces.
  * dedupe is by (path, keyword, message), not by path — three DISTINCT
    defects at `/` all survive.
  * no non-empty error list can collapse to zero.

The five repos re-measured (portaliq, doriath, openconnector, larpingapp,
procest) are all clean on gate-22 today, as is shillinq, whose 240-violation
run has since been burned down. So this changes no current verdict; it stops
the NEXT repo's number from being inflated threefold.

New suite test_check_manifest_ajv_collapse.js, auto-discovered by
run-helper-suites.sh (27 -> 28 suites). Unit-covered without Ajv, end-to-end
when Ajv resolves, and it refuses to run rather than pass vacuously if the
export is missing. Four mutations, each asserting it actually applied first:
collapse disabled fails 4, applicator dropped unconditionally fails 3, dedupe
by path only fails 2, collapse returning [] fails 9.
@rubenvdlinde
rubenvdlinde merged commit 2c07f0a into main Aug 8, 2026
29 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/gate-22-ajv-triplet branch August 8, 2026 13:51
rubenvdlinde added a commit that referenced this pull request Aug 8, 2026
…224, #226, #230, #235, #236, #266) (#269)

* fix(gates): nine checkers matched prose, not code — one shared scope, nine gates

Every gate below decided a question about CODE by grepping the raw bytes of a
file. Prose is made of the same bytes, so each one failed in BOTH directions
at once — the shape first written down in #184: "a checker that greps a STRING
LITERAL misses every constant and matches every comment."

  #191  gate-48  a REMOVED COMMENT naming `#[NoCSRFRequired]` read as a removed
                 attribute. nldesign red for one rewritten docblock sentence.
  #196  gate-5   a docblock saying `#[NoAdminRequired]` is deliberately NOT
                 used SATISFIED the auth gate. A false NEGATIVE on a security
                 gate, and a pass leaves no log.
  #220  gate-31  an `<img>` in a JSDoc comment in <script> (launchpad).
  #235  gate-31  the same, 3 of 3 findings on openbuild.
  #224  gate-34  false RED on a comment AND false GREEN on window['confirm']().
  #226  gate-3   a run() delegating to one helper read as a stub, and the gate
                 was closable by an inert `$unused = 1;`.
  #230  gate-58  a comment WARNING AGAINST networkidle counted as a use of it.
  #236  gate-12  `<NcSelect[^>]*>` truncated at the `>` of `option =>`.
  #236  gate-32  a comment describing the `<div @click>` an element replaced
                 scored as that `<div @click>`.
  #266  gate-41  a PHP comment mentioning `<html>` made a mount point a page
                 root.

ONE SCOPE, NOT NINE
-------------------
scripts/lib/source_scope.py generalises the two precedents that already got
this right — #184's PHP stripper (which knows `#` opens a comment but `#[`
opens an attribute) and #249's gate-19 tokeniser (blank once, PRESERVE
OFFSETS, keep string delimiters). Every mask returns a same-length string, so
a gate can report a line number computed on the mask and read a suppression
marker out of the ORIGINAL at that line — which matters because every
suppression marker in this package lives in a comment.

Gate-19 keeps its own copy of the JS tokeniser; a drift test asserts the two
byte-identical over a corpus and over this package's own .js sources, and
asserts the keyword sets equal — the corpus alone SURVIVED deleting "await"
from one set, so the corpus alone was not enough.

#196 SHIPS WITH A DECLARATION, NOT JUST A TIGHTENING
-----------------------------------------------------
Admin-only is expressed in Nextcloud by the ABSENCE of an attribute, and
absence is the only thing gate-5 reports. Closing the false negative alone
would have converted it into a PERMANENT false positive on correct code, with
no legitimate way to satisfy the gate. So `@auth admin-only <reason>` joins
the `@spec exclude` family. Making bare absence sufficient was considered and
rejected: it would empty the gate completely.

MEASURED, NOT ASSUMED
---------------------
- 3 fixtures from #226's table, the 4 arms from #224, the nldesign line from
  #191 and the larpingapp line from #230, all verbatim.
- Every relaxation is paired with the true positive it must not swallow, and
  every wiring is covered both ways: a MISSING helper and a CRASHING helper
  must report SKIPPED, never PASS (#147, #245, #249). gate-5 additionally runs
  a positive control on the mask itself, because a mask that silently returns
  its input is invisible to `[ -f helper ]` and puts the gate straight back
  into the false negative.
- A nested `<template #default>` slot regression was caught by measurement
  before landing: a lazy `(.*?)` ended the SFC template at the first slot
  close and deleted a real finding at openconnector EditMapping.vue:376.
  Boundaries are found by depth now, and there is a test.

Closes #191, #196, #220, #224, #226, #230, #235, #266
Refs #236 (parts 1 and 2; part 3 was already fixed by #247)
Supersedes #219, whose gate-12 helper is carried here with its 17 tests.

* fix(gate-34,gate-48): a guard is not a second dialog, and an FQCN attribute is one

Both found by MEASURING the fix rather than by reading the issues.

gate-34 — 7 defects reported as 14 findings
------------------------------------------
The first cut accepted any `window.confirm` REFERENCE, called or not, so on
openbuild every native dialog was reported twice:

    const ok = typeof window !== 'undefined' && window.confirm     <- guard
        ? window.confirm(t('openbuild', 'Delete this automation?')) <- call

A feature-detection guard is a truthiness test, not a second native dialog,
and inflating a security-adjacent count is its own false report (#254: a count
is not a defect count). A reference now counts only when it is an ALIAS — a
binding whose call site is elsewhere and therefore invisible:

    const c = window.confirm        counts
    const { confirm } = window      counts
    x && window.confirm ? … : …     does not

openbuild: 7 before, 7 after, same seven lines.

The anchor also lost a character it should never have had. Written
`=\s*window\s*[.\[]` it CONSUMED the `window` that follows, and `finditer`
returns non-overlapping matches — so `const r = window.confirm('x')` matched
only the alias rule, failed it because a `(` follows, and reported NOTHING. A
real call dropped by an anchor one character too greedy. It is a lookahead
now, and there is a test.

gate-48 — the old regex could not see a fully-qualified attribute
-----------------------------------------------------------------
Running #191's arm 2 end-to-end through the runner reported PASS on a genuine
removal of

    -    #[\OCP\AppFramework\Http\Attribute\NoCSRFRequired]

because the pre-fix pattern alternated on the literal `#[NoCSRFRequired]`.
A false NEGATIVE hiding behind the false positive #191 reported — the same
both-ways failure as every other gate in this change. The new bracket-bounded
rule matches it.

Refs #191, #224

* fix(source_scope): `</script bar>` ends a script, and the mask must know it

CodeQL raised py/bad-tag-filter (HIGH) against this branch, and it is right.

    r'<script(\s[^>]*)?>(.*?)</script\s*>'

does not match `</script bar>` or `</script\t\n foo>`, both of which an HTML
parser treats as the end of the element. When the close is spelled that way
the block regex fails to match AT ALL, the script body is never
comment-masked, and a JSDoc `<img>` inside it is scanned as markup — #235
reintroduced by the mask written to fix it. `</style …>` had the same hole.

⚠️ THE FIRST TEST FOR THIS SURVIVED THE MUTANT. It exercised
`vue_markup_mask`, which keeps `<template>` spans and never goes through
`_SCRIPT_BLOCK` at all, so reverting the regex changed nothing and the suite
stayed green. The assertion now runs through `html_markup_mask` and
`script_mask`, the two functions that actually use it, and the reverted regex
kills both. A mutation test that does not kill is not evidence — it is a
second thing to check.

Refs #235
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