fix(gates 9, 27, 28, 46, 47, 61, 62): a comment must not manufacture a finding (#415 class, #423) - #439
Merged
Merged
Conversation
…a finding (#415 class, #423) Seven checkers asked a question about CODE and answered it by matching bytes of a FILE, so a sentence about code that was REMOVED scored as that code. This is the corrosive half of #415, not the milder one. In every case below the author's cheapest fix is to DELETE THE EXPLANATION: the documented change goes red and the undocumented one goes green, and the gate then loses the only thing that would let a human check it. gate-9 a body whose only mention of a guard is `// We used to $this->requireAdmin(); here. Now anyone may read.` -> no-admin-required-annotation-with-admin-body. The file's own _strip_comments does not know `#` opens a PHP comment either, so the hash form reproduced identically. gate-27 ONE clean fixture -> FAIL — 3: an interior line of an unindented /* */ block, a trailing `// never do $reg->getLeaf($id) here`, and a string literal. Its comment test was `startswith` per LINE, i.e. it only ever recognised a comment-OPENING line. gate-28 `* @license MIT was never used here — see the note above.` beneath a real `* @license EUPL-1.2` -> FAIL — 2 (internal-conflict AND triangle-drift). A sentence that begins with the tag read as a second declaration. gate-46 `const spec = '@SPEC openspec/specs/imaginary/spec.md'` and a tag mid-sentence in prose -> FAIL. Gates 47 and 48 were given position-anchored regexes for exactly this; 46 never was, and it is the gate with NO false-negative direction — every defect it can have is a finding it manufactures. gate-47 a TRAILING `// resolved via IUserSession elsewhere` and an HTML comment -> FAIL — 2, defeating the module's own stated promise ("Prose that merely mentions IUserSession is not — it is a sentence"). `<!--` was not in its comment vocabulary at all while `src/**/*.vue` has always been a candidate path. gate-61 a listener that exists ONLY as commented-out registration lines -> `FAIL … no class file found under lib/`. The listener was DELETED; the gate reported the deletion as a broken registration. gate-62 a docblock saying the class deliberately does NOT hit the objects API -> a finding IDENTICAL to the real violation's. The doriath `loadApp` shape gate-64 was already repaired for, one gate over. NOT A FIFTH STRIPPER. Six of the seven route through lib/source_scope.py (`php_mask`, `js_comment_mask`), which is offset-preserving — findings report a LINE NUMBER, so comment text is blanked and lines are never removed — and already knows `#[` opens a PHP 8 attribute and that `//` inside 'https://x' opens nothing. gate-61's local three-regex `_strip_comments` becomes a call to php_mask rather than a fourth dialect: it was not offset-preserving, not string-aware, and saw `#` only at the start of a line. STRING CONTENTS ARE KEPT EVERYWHERE (`blank_strings` left at its default), and that is a judgement per gate, not an omission. gate-27's Rule B evidence IS a string literal (`->call('decidesk', …)`), gate-62's URL is a string in every real violation, gate-47 reads `requesttoken` as a header key, and gate-9's self-auth idioms are literals by nature. Blanking them would trade these false positives for false NEGATIVES in gates whose findings are runtime fatals. Each suite carries that pairing as an explicit control, so a later change to string handling (#424) cannot pass silently. Two gates are position-anchored rather than masked, because their tag lives in a comment BY DESIGN and masking would switch them off: gate-46's `@spec` must start the line's content, and gate-28's licence identifier may be followed only by a URL or a comment terminator. MEASURED, NOT ASSUMED. Over every tracked lib/**/*.php in openregister, opencatalogi, procest, docudesk, larpingapp and softwarecatalog: gate-28 2,394 files — declaration counts IDENTICAL (1437/76/610/151/23/97), finding SETS identical, larpingapp's 19 drift findings all survive. The fleet's own header is `@license EUPL-1.2 <url>`, 2,724 files of it, so a rule of "nothing may follow the identifier" would have silenced the gate on its main subject. That is why the tail rule admits a URL. gate-46 5,684 files, 17,408 tags -> 17,406, and the finding sets are identical (25 = 25, no additions, no removals). The two tags it stops seeing are `// Per @SPEC …("the system MUST` (a sentence) and "NOTE: this used to read `@spec …#T07`" (a note about a tag that was REMOVED) — this defect class verbatim, twice. ARMS. 45 new assertions across the seven suites, each labelled: the ones that FLIP when the checker is restored from origin/main are the evidence, the ones that pass BOTH ways are labelled CONTROLS and exist so no negative can be had by switching a rule off. Reverted, exactly the evidence arms flip: 9 -> P2/P3 (+P6 wiring), 27 -> R2/R3/R5 (+R8), 28 -> T2/T3, 46 -> A2/A3/A4, 47 -> C3/C9 (+C8; C2 raises on the old signature), 61 -> L2/L3/L4, 62 -> S2/S3. Controls: P1/P4/P5, R1/R4/R6/R7, T1/T4-T8, A1/A5/A6/A7, C1/C4-C7, L1/L5/L6, S1/S4. WHAT IS DELIBERATELY NOT DONE. `.vue` is not masked in gates 27 and 47: the mask for an SFC routes through the `<!-- … -->` handling that #424 is currently repairing, where a `<!--` inside a string blanks live code. A gate whose findings are runtime fatals must not inherit a hole that can silence it, so those two false positives STAND and are named in the code. Gates 15 and 21 are not in this change at all — see the issue thread.
…quadratic
CodeQL raised `py/redos` HIGH on gate-28's new tail rule. It is right, and
the fix is not to tighten the pattern but to remove the ambiguity it needs.
^(?:\s|https?://\S+|\*/|--!?>|[.,;:)\]]|\(\s*\)|\s*\Z)*$
`\S+` inside a STARRED alternation is ambiguous with itself — `https://ab`
can be one iteration or several — so a subject that never reaches `$` has
exponentially many splits to try.
⚠️ THE ALERT'S OWN WITNESS STRING DOES NOT REPRODUCE HERE, and I nearly
wrote its numbers down without running them. CodeQL names `'http://' +
'!http://' * n`; in CPython that is 0.000s even at n=32,000, because
`\s*\Z` can match empty and the engine's empty-repeat guard cuts the search
short. The ambiguity is still real — it just fires on a different subject,
a repeated URL with one trailing byte that cannot match:
'https://a' * n + ' \x01' OLD NEW
n=18 len=164 0.0949s 0.0000s
n=22 len=200 1.5284s 0.0000s
n=24 len=218 6.6797s 0.0000s
n=26 len=236 26.1822s 0.0000s
n=28 len=254 >60s 0.0000s
Every +2 in n multiplies the old time by ~4. A 254-CHARACTER line — shorter
than plenty of real headers — hangs the gate, and gate-28 reads every tracked
lib/**/*.php in the repo.
The rule is now a linear scan: split the remainder on whitespace and ask of
each token whether a header could carry it. No quantifier, nothing to
backtrack over, so the ambiguity cannot exist rather than being harder to
reach. Flat on the adversarial input and still flat at 256 KB (0.0103s).
AND THE SWEEP FOUND A SECOND ONE the alert did not flag. gate-46's new anchor
was written `[^\S\n]*(?:lead-in)?[^\S\n]*`, which puts two whitespace runs
next to each other whenever the lead-in matches empty: a line of N spaces
that never reaches the tag costs O(N²), and 64,000 spaces took >20s. Each
optional group now swallows its own trailing whitespace; the same subject is
0.0072s, and 256,000 spaces is 0.0270s — linear.
NOTHING ELSE MOVED, which is the half that needed proving: a ReDoS fix that
quietly narrows a pattern is a gate weakening wearing a security fix's
clothes.
* gate-28's fleet measurement is unchanged: 2,394 tracked lib/**/*.php,
declaration counts identical per repo (1437/76/610/151/23/97), finding
sets identical, larpingapp's 19 drift findings all still there.
* gate-46's is unchanged: finding sets identical (25 = 25), and the delta
is still exactly the same TWO prose tags — `// Per @SPEC …("the system
MUST` and "NOTE: this used to read `@spec …#T07`".
* Reverted against origin/main, the same arms flip and the same controls
hold: T2/T3 flip, T1/T4–T8 are controls.
* Both suites green (37 -> 38 and 57 -> 58 assertions).
The two new arms are shape assertions, not benchmarks: each bounds a true
cost of ~0.00003s / ~0.007s at 5s, three orders of magnitude of headroom, so
a loaded runner cannot flake them.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 13, 2026
One-line conflict: both sides added an import from source_scope — this branch needs php_mask, main's #439 needs mask_html_comments. Kept both, because they are independent helpers for different masking jobs, not competing versions of one. Verified after the resolve rather than assumed: source_scope defines both symbols, this file calls each exactly once, the module parses, and the checker still exits non-zero on a nonexistent app root (an unrunnable checker must not read as a clean repo).
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
3 participants
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.
Closes part of #423 — the false-positive half of the #415 comment class, checker side. Companion PR for the runner side: gates 5, 23, 24.
What was wrong
Seven checkers asked a question about CODE and answered it by matching bytes of a FILE, so a sentence about code that was removed scored as that code. Every one of these was reproduced on this package before being touched, with its positive control run first.
// We used to $this->requireAdmin(); here.* @license MIT was never used here — see the note above.under a real EUPL headerThis is the corrosive direction: in every case the author's cheapest fix is to delete the explanation, and the gate then loses the only thing that would let a human check it.
How
Six route through
lib/source_scope.py(php_mask,js_comment_mask) — offset-preserving, because findings report a line number. gate-61's local three-regex stripper becomes a call tophp_maskrather than a fourth dialect: it was not offset-preserving, not string-aware, and saw#only at the start of a line.Two are position-anchored instead of masked, because their tag lives in a comment by design: gate-46's tag must start the line's content (gates 47 and 48 were given exactly this treatment; 46 never was), and gate-28's licence identifier may be followed only by a URL or a comment terminator.
String contents are kept everywhere, and that is a judgement per gate rather than an omission — gate-27's Rule B evidence is a string literal, gate-62's URL is a string in every real violation, gate-47 reads a header key. Each suite carries that pairing as an explicit control, so #424 cannot change it silently.
Measured, not assumed
Six fleet repos (openregister, opencatalogi, procest, docudesk, larpingapp, softwarecatalog), finding sets diffed rather than counted:
lib/**/*.php: declaration counts identical, finding sets identical, larpingapp's 19 drift findings all survive. The fleet's own header carries a URL after the identifier, 2,724 files of it, so the obvious rule ("nothing may follow") would have silenced the gate on its main subject. The corpus decided the rule.@spec …" — this defect class verbatim, twice.The shapes are latent in these repos; the evidence is the fixture, and that is stated rather than dressed up as a fleet win.
Arms
+45 assertions across the seven suites. Reverted against
origin/main, exactly the evidence arms flip and the labelled controls do not: 9 → P2/P3/P6, 27 → R2/R3/R5/R8, 28 → T2/T3, 46 → A2/A3/A4, 47 → C3/C8/C9, 61 → L2/L3/L4, 62 → S2/S3.tests/run-helper-suites.sh: 83 passed, 0 failed, 2 quarantined (both pre-existing and documented).Deliberately not done
.vueis not masked in gates 27 and 47 — the mask for an SFC routes through the<!-- … -->handling under repair in #424, and a hole there would let a comment silence a gate whose findings are runtime fatals. Those two false positives stand and are named in the code. Gate 15 is blocked on the same thing; gate 21 is declined with the trade measured — see the issue.🤖 Generated with Claude Code