The 205 code-scanning findings, read rather than counted - #99
Merged
Merged
Conversation
The CodeQL workflow's comment called its first run "a reading list, not a verdict". This is the reading. Of the 205 open findings, 5 carried severity and 4 of those were false positives; the rest were two Python idioms and one real cleanup. Excluded at the suite, in .github/codeql/codeql-config.yml with the reasoning beside each: py/ineffectual-statement (79) -- every one is `...`, which is a statement with no effect and also how Python spells an empty body. The modules it names are largely Protocol definitions. py/import-and-import-from (33) -- `import ctrlrun` beside `from ctrlrun import protect`, in tests and cookbook examples, where showing both is the point. A suite-level filter rather than 112 dismissals because a dismissal is bound to an alert: the next Protocol method written would raise a fresh one and the judgment would have to be made again by whoever happened to see it. Acted on rather than silenced: py/repeated-import (19) -- correct. Nineteen stdlib re-imports inside functions that already had the module at the top; deleted. py/bad-tag-filter (1) -- a real bug. `_SCRIPT` in tests/test_docs_site.py lacked `re.I`, so an upper-case `<SCRIPT>` block would survive `_prose` and its structured-data payload would count against a page's word budget, failing a page for a reason no author could see. Fixed, with a test that fails without the flag -- mutation-checked both ways. And src/ctrlrun/acs.py's `except (NotExecuted, _Unknown): pass` gained the comment its three sibling handlers already had. It is not a swallowed error: those two exceptions are how `report_what_happened` states the outcome, and `Control.resume` has already written the receipt before re-raising. The five severity findings are dismissed individually in the code-scanning tab with a reason: py/call-to-non-callable x2 (CodeQL misses `ExpiringClock.__call__`), py/log-injection (the value is a config-derived header *name*, and `%r` escapes it), py/incomplete-url-substring-sanitization (a citation assertion in a test, not URL parsing). No security query is excluded. Every error-severity and security-severity query still runs.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
`re.I` answered the case-sensitivity the query first reported, and the query then reported the
other half of the same defect: `</script >` is a valid end tag and the pattern did not match it.
The first fix was right and incomplete, which is the more interesting result -- the finding was
never really "upper case", it was "this regexp does not parse the tag grammar it is filtering".
<\s*script\b.*?<\s*/\s*script\s*>
`\b` so a tag merely beginning with those letters does not open a block, and `\s*` in the three
places the grammar allows whitespace. The test is now parametrised over five spellings.
Mutation table, run with PYTHONDONTWRITEBYTECODE=1 and the bytecode cleared:
drop `re.I` 2 of 5 fail (upper-case, newline)
rigid `</script>` end tag 3 of 5 fail (space-before-gt, space-after-slash, newline)
Neither mutant is survived by the whole set, so both properties are load-bearing rather than one
of them documenting the other.
Third report on the same line, and the query was right all three times. `</script bar>` closes
the element: an end tag may not carry attributes, but the parser ignores what it finds there
rather than refusing the tag, so text after the name does not save the filter.
<\s*script\b.*?<\s*/\s*script\b[^>]*>
`\b` on both ends so `<scriptish>` neither opens nor closes a block, `\s*` where the grammar
allows whitespace, `[^>]*` for the junk a parser would discard. Seven spellings parametrised.
Mutation table, PYTHONDONTWRITEBYTECODE=1 and bytecode cleared, of 7 cases:
drop `re.I` 2 fail
rigid `</script>` end tag 5 fail
no attribute-like text in the end tag 2 fail
no whitespace allowed in the start tag 1 fail
Four properties, four distinct failure sets, none subsumed by another.
The wider lesson is the one this repository already writes down about defence in depth: the
first fix answered the symptom the report named -- upper case -- and the defect underneath was
that the pattern did not model the grammar it filtered. Two more reports were needed to say so.
arpanghoshal
enabled auto-merge (squash)
September 6, 2026 19:54
arpanghoshal
added a commit
that referenced
this pull request
Sep 6, 2026
Reading the 211 open code-scanning alerts turned up four things behind the noise. PR #99 handles the noise; these are the defects under it. **`except BaseException` on a path that returns `passed`.** In the store kit, `no-not-executed` swallowed everything a store method raised and then reported the case passed, and `insert-not-upsert` did the same after inspecting the record. Press Ctrl-C during either and the suite whose whole purpose is to refuse false greens produced one. SPEC-v0.1 §5.5 is the rule and it is not "catch less": the executor path must catch BaseException, record, and **re-raise**. The breadth was never the defect -- an adapter or a store may raise anything and the kit must grade it -- the swallow was. `_not_ours_to_grade` re-raises what is not an Exception, at every catch site in both kits. The adapter kit's sites returned `failed` rather than `passed`, so they surfaced an interrupt instead of hiding it, but an interrupted run still produced a conformance verdict blaming somebody's adapter for the operator's Ctrl-C. **A cycle between the two gateway servers, which #96 introduced.** Moving `check_jwt_flags` into `server.py` and typing it against `GatewayConfig | OperatorConfig` needed a TYPE_CHECKING import of the console, so the gateway named the console and the console imported the gateway -- `py/unsafe-cyclic-import`, six times, and a layering inversion against ARCHITECTURE §6. The shared half moves to `gateway/wire.py`, below both: response bytes, JSON-RPC envelopes, the header lookup, the flag validation. Neither server imports the other, and a test says so, because nothing did. **The access log took the client's request line unescaped.** `log_message` interpolated it straight into a line-per-record log, where a newline forges a whole record -- the hazard `state.py` refuses control characters in an approver's name for. A log line cannot be refused, so it is escaped. **`repoint-claims.py` wrote wrong line numbers and its guard accepted them.** Both sides asked "which symbol is this citation about" and both answered with the whole row, so a row citing six commands had every reference re-pointed at one definition and the guard passed, because that line does define a symbol the row names. Two further defects fell out while fixing it: the rewrite was a global `str.replace` on the citation's text, so two citations of one line in a row could not move apart and fixing a stale one broke a correct one; and the tokenizer took only the first identifier in a backtick span, so `ctrlrun delegate` owned `ctrlrun` and looked stale. `tools/docs_audit/claims.py` is now the one producer, imported by the writer and by the guard, and it caught a reference this branch's own refactor had shifted. 7 mutations, 7 red, control green. 4052 passed.
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
2 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.
The CodeQL workflow's own comment called its first run "a reading list, not a verdict." This is
the reading. 205 open findings; 5 carried severity and 4 of those were false positives.
What each group turned out to be
py/ineffectual-statementpy/import-and-import-frompy/repeated-importpy/bad-tag-filterpy/call-to-non-callable×2,py/log-injection,py/incomplete-url-substring-sanitizationPinnedDependencies×15,CodeReview,Maintained,SAST,CIIBestPractices)Excluded, with the reasoning in the file
.github/codeql/codeql-config.ymlcarries the argument beside each exclusion rather than in thisdescription, because the next person to see one of these findings will be reading the config.
py/ineffectual-statement— every one of the 79 is..., which is a statement with noeffect and also how Python spells an empty body. The modules it names (
state.py,approval.py,receipt.py,adapter.py,conformance/store/backends.py) are largelyProtocol definitions, per ARCHITECTURE.md §6.
py/import-and-import-from—import ctrlrunbesidefrom ctrlrun import protect, intests and cookbook examples, where showing the reader both is the point.
A suite-level filter rather than 112 dismissals: a dismissal is bound to an alert, so the next
Protocol method written would raise a fresh one and the judgment would have to be made again by
whoever happened to see it.
No security query is excluded. Every
error-severity and everysecurity-severityquery insecurity-and-qualitystill runs.The one real bug
_SCRIPTintests/test_docs_site.pydid not parse the tag grammar it was filtering. It lackedre.I, and — as CodeQL pointed out again after the first fix — it also required a rigid</script>, when</script >is equally valid. Either way the block survived_prose()and itsstructured-data payload counted against a page'''s word budget, failing a page for a reason no
author could see.
Now
<\s*script�.*?<\s*/\s*script\s*>withre.S | re.I, and the test is parametrised over fivespellings. Mutation table:
re.I</script>end tagNeither property is documenting the other.
Also
across seven test files.
src/ctrlrun/acs.py'sexcept (NotExecuted, _Unknown): passgained the comment its threesibling handlers already had. It is not a swallowed error — those two exceptions are how
report_what_happenedstates the outcome, andControl.resumehas written thefailedorambiguousreceipt before re-raising.Verification
4053 passed, 45 skipped(main's 4048 plus the new test),ruff check,ruff format --checkandmypy --strict src/all clean — run from an isolated worktree, not the shared checkout.