chore(tools): close the suppression set and record the five ORBIT verdicts - #515
Conversation
…dicts An escape hatch an agent can write is not an escape hatch. It is the default. All five ORBIT analyzers already ship at DiagnosticSeverity.Error, so nothing here was sitting at warn. The hole was the escape hatch: one line of `#pragma warning disable` plus a comment turned any of them off, and the comment was the entire requirement. The 47 live ORBIT0004 sites carried exactly two justification strings, character for character, both pointing at a mutable GitHub issue outside the repository that nothing in CI read. `tools/suppression-allowlist.json` is now a closed set: every `#pragma warning disable` and every `[SuppressMessage]` under `src/` is declared per file and per rule with a site count and its own reason, and an undeclared one fails `Guard Conventions`, which is already a required check on `main`. The EF scaffolder's 124 migration pragmas are exempted by a visible directory entry carrying its reason, not by a regex hidden in the checker. Issue #431's per-site classification moved into the repository, and the 47 pragma comments that linked it were stripped, so `git grep issues/431 -- src` returns nothing. Each of ORBIT0001 through ORBIT0005 carries a committed verdict with its evidence. Four keep the rule as it stands and have zero suppressed sites. ORBIT0004's verdict is NARROW IT, provisionally, and #230 executes it: the analyzer's exemption logic is line-level, so a local named `nowUtc` flowing into an `*AtUtc` parameter fires. Narrowing an analyzer silently changes what 47 sites mean before anyone has read them, so this ticket declares them as they are. Closes #228 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The checker is not yet a closed set: legal C# suppression forms and undeclared migration rules can pass the required guard without an allowlist change.
Reviewed changes Reviewed the complete initial diff for the suppression inventory and its enforcement.
- Required guard wiring: Adds Node setup, the suppression check, and its tests to
Guard Conventions, plus a matching pre-commit hook. - Closed-set checker: Scans C# files for pragma and attribute suppressions, validates allowlist entries, compares per-file rule counts, and provides help and inventory output.
- Suppression ledger: Declares 101 hand-written suppression sites, a generated migration exemption, and evidence-backed verdicts for
ORBIT0001throughORBIT0005. - ORBIT justification relocation: Removes repeated issue-link comments from 47
ORBIT0004pragmas and records their classifications in repository data.
GPT Sol ｜ 𝕏
…d set A closed set that legal syntax can open is an open set with extra steps. All three were real and all three now have a case. 1. A bare `#pragma warning disable`, with no rule ids, disables EVERY warning from that point on. The pattern required at least one id and walked past it. It is now counted under a synthetic id that is not a legal C# rule, so it surfaces as an undeclared suppression; declaring it makes it VISIBLE in the diff with a reason, which is the outcome a data-driven closed set should have. 2. `[SuppressMessageAttribute(...)]` is the same attribute as `[SuppressMessage(...)]`, because `Attribute` is optional in C# attribute syntax. The pattern missed the long spelling. 3. The generated directory was SKIPPED rather than exempted for the rules it declares, which made its `rules` list decorative: every `.cs` file below `Migrations/` passed, so a new analyzer suppression added inside a migration needed no visible change anywhere. That contradicted this checker's own header. Fixing 3 immediately found what it was built to find: two migrations carry `CA1814`, which the original entry did not declare. It is scaffolder output, not a hand edit, and the correlation is exact rather than assumed. Verified 2026-09-08: `20260326224530_AddReferralSystem.cs` and `20260404045924_AddFeatureFlags.cs` are the only files under that directory calling InsertData with a `new object[,]` row block, and they are the only two carrying the pragma. `CA1814` is now declared with that evidence, and the rules list is exhaustive and enforced. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc
|
Both findings were right, both are fixed, and the second one immediately found what it was built to find. Finding 1, the legal-syntax bypasses. A bare Finding 2, the generated directory. Correct, and it contradicted this checker's own header: Turning that on found two undeclared sites straight away, which is the "this already hi[t]" your finding predicted: Four new cases: the bare pragma is caught; declaring the bare pragma passes and is visible; the long attribute spelling is caught; an analyzer suppression added inside a migration fails because the directory declares only |
There was a problem hiding this comment.
Important
The required suppression guard remains bypassable by legal C# source forms, so the closed-set guarantee is not yet enforced.
Reviewed changes Reviewed the fix commit added since the prior Pullfrog review.
-
Expanded suppression parsing: Detected warning-list-free pragmas and the full
SuppressMessageAttributesuffix. -
Restricted generated exemptions: Scanned migration files and limited their exemption to declared rule IDs, including the existing
CA1814sites. -
Added regression coverage: Added focused cases for the three previously reported bypasses and both sides of the generated-rule constraint.
GPT Sol ｜ 𝕏
…, and fail closed Canonical-form matching cannot establish a closed set. Any form the extractor does not recognise reads as "no suppression here" and passes, which is how the second review round found three more legal C# spellings walking past the gate. Verified against a real net10.0 build with TreatWarningsAsErrors=true and AnalysisMode=AllEnabledByDefault: `# pragma warning disable` suppressed the diagnostic while this checker reported zero sites and exited 0. Whitespace after the `#` is legal, so the directive pattern now allows it, and so do its internal gaps. Detection and extraction are separate now. A broad pattern finds every use of SuppressMessage, the strict one extracts the id from those it can prove, and anything found but not extractable is REFUSED with its file named rather than skipped. Two forms fall there: * a check id that is not a string literal, such as a `const string` reference, which no regex can read; * a `using` alias for the attribute, which no regex can resolve to the attribute it names, so the alias DECLARATION is what fails. A form this tool cannot inventory is now a form it refuses. Four cases, one per spelling. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc
There was a problem hiding this comment.
Important
The alias refusal remains bypassable by compiler-valid C# syntax, so the required guard still does not enforce a closed suppression set.
Reviewed changes Reviewed the suppression parser fix added in f22edb8 since the prior Pullfrog review.
- Separated detection from extraction: Added broad
SuppressMessagedetection and refused check IDs that cannot be extracted safely. - Expanded pragma parsing: Recognized whitespace within preprocessor directives, including spaced bare disables.
- Added alias handling and regression coverage: Refused the ordinary dotted-name alias form and added focused tests for the newly handled spellings.
GPT Sol ｜ 𝕏
Three rounds were spent adding one more spelling each time: the long `Attribute` name, a `const string` check id, an ordinary `using` alias, then `global using` and `global::`. That is an open set defended by guesswork, which is the exact antipattern this gate exists to reject, and enumerating C#'s using-alias grammar was never going to terminate: the alias carries an optional `global` modifier, an optional `global::` qualifier, arbitrary whitespace, and can span lines. So the rule is inverted. EVERY mention of `SuppressMessage` or `SuppressMessageAttribute` under `src/` must be accounted for by an attribute use the strict pattern can read into a declared site. A surplus mention is refused, whatever produced it: an alias in any spelling, a non-literal check id, a mention in a comment, or a form nobody has thought of yet. Nothing has to be predicted. Measured before adopting it: 7 mentions in 3 files, 7 extracted, so the invariant holds on the live tree with no false positive. A mention in a comment now fails, which is a fail-CLOSED false positive with a visible remedy rather than a silent pass, and that is the deliberate trade. Five alias cases: the two review found, plus a line-split alias, an alias to the short attribute name, and a bare mention with no attribute use at all. The last three were never predicted and are covered by the same invariant, which is the whole point of replacing the enumeration. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc
There was a problem hiding this comment.
Important
Unicode-escaped attribute identifiers still bypass the required suppression guard, so the closed-set guarantee remains unenforced.
Reviewed changes Reviewed the invariant-based SuppressMessage detection added in 6d15c6d since the prior Pullfrog review.
- Mention invariant Replaced explicit alias enumeration with a raw identifier count compared against extractable literal attributes.
- Alias regression coverage Added cases for global,
global::, multiline, and short-name aliases, plus an unrelated identifier mention.
GPT Sol ｜ 𝕏
…e has C# permits Unicode escape sequences inside an identifier, so the attribute name can be spelled with an escaped character and remain the same attribute to the compiler while a raw text scan sees nothing. Verified: against the raw source the mention invariant counts 0, against the decoded source it counts 1. Decoding the whole file once, before any pattern runs, closes the entire escape family rather than this one instance. The compiler performs the same normalization, so it is not a heuristic. Checked against the only file under src/ that already carries escapes: its nine escapes decode to Portuguese accented characters and the mention count is 0 before and after, so decoding invents nothing. A case pins that. The first case asserts the escaped spelling is genuinely invisible to a raw scan, so the three that follow cannot pass vacuously. THE CEILING is now written into the header rather than left implied. A text scanner cannot be closed against a determined author. Escapes are closed, and the verbatim @SuppressMessage spelling was already matched because @ is not a word character, but source generators, a partial class assembled across files, and forms nobody has thought of remain outside what reading source text can prove. This gate closes the set against accident and against convenience, which is what happens in this repository. Real closure needs resolved symbols: a Roslyn analyzer, or an inventory taken from dotnet build warning output. That is a bigger change than this ticket and it deserves its own. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc
There was a problem hiding this comment.
Important
The Unicode normalization fix still leaves a compiler-recognized suppression spelling outside the required inventory.
Reviewed changes Reviewed the identifier-escape fix added in adde421b since the prior Pullfrog review.
- Normalized Unicode escapes: Decoded
\uXXXXand\UXXXXXXXXsequences before suppression detection and extraction. - Added regression coverage: Proved escaped attribute names are rejected or inventoried and ordinary accented escapes remain clean.
- Documented the scanner ceiling: Scoped the text scanner to accidental and convenient bypasses and identified resolved-symbol analysis as the stronger follow-up.
GPT Sol ｜ 𝕏
…first C# defines identifier equality in two steps: decode Unicode escape sequences, then REMOVE category Cf formatting characters. The normalizer applied only the first, so `Supp<ZWNJ>ressMessage` stayed invisible to a scan that had just been taught about escapes. Fixed by applying the second step the language defines rather than by adding another spelling. Removing Cf cannot manufacture a match out of ordinary text: 234 files under src/ already carry exactly one Cf character each, the UTF-8 BOM on every generated migration, and stripping a BOM produces nothing. A case pins that. Four cases, and two of them exist so the others cannot pass vacuously: the format-character spelling is asserted invisible to both a raw scan and to escape decoding alone; and an escape that DECODES to a format character is caught, which neither step alone would see, so the two compose. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the identifier normalization fix added since the prior Pullfrog review.
- Completed C# identifier normalization Applied Unicode escape decoding followed by category
Cfremoval before suppression detection and extraction. - Added composed regression coverage Proved raw and escaped formatting characters cannot hide
SuppressMessageAttribute, while UTF-8 BOM handling remains clean.
GPT Sol ｜ 𝕏
1 participant
What changed
#228, the mechanism half of API gate parity. The principle goes in the code:That is root
CLAUDE.mdstandard 6 applied to the gates themselves, and it is the call ORB-170 already shipped for the repository root: closed set, not open list, so the unlisted case fails by default.The C# problem is not the TypeScript problem, and this does not treat it as one
All five ORBIT descriptors already ship
DiagnosticSeverity.Error, isEnabledByDefault: true. They already fail the build. Nothing here was sitting atwarn. The hole was the escape hatch: one line of#pragma warning disableplus a comment turned any of them off, and the comment was the entire requirement.Measured on this branch's base, so these are today's numbers rather than the ticket's 2026-08-05 ones:
#pragma warning disableundersrc/, totalsrc/Orbit.Infrastructure/Migrations/(EF scaffolder)[SuppressMessage]attributesORBIT0004pragma linesTwo strings, character for character, both pointing at
thomasluizon/orbit-api#431. The per-site reasons genuinely existed, which is the correction the code forces: the defect was never "nobody wrote a reason". It was that the reason lived in a mutable GitHub issue outside the repository and nothing in CI read it. A reviewer saw an identical comment; the discriminating information was one click away, editable by anyone, and un-gated. A 51st pragma with the same pasted comment passed every check in this repo.What now exists
tools/suppression-allowlist.json, a closed set. 101 declared sites: 94 pragma sites across 54 files and 7 attributes across 3 files, each declared per file, per rule, with a site count and its own reason.tools/check-suppression-allowlist.mjs, mirroringtools/check-root-allowlist.mjs:--help, documented exit codes (0 clean, 1 the set was breached, 2 usage or data error), and a header stating why the set is closed on both halves. Four ways to fail, each one a visible data change away from passing:Granularity is per file with a site count, and the header records why: line numbers churn on every edit and would make this gate a nuisance that gets weakened, while a file plus a count still forces a data change the moment a 51st site appears, and catches the stale-allowlist drift a bare file list would miss.
Generated code is declared, not skipped. The 124 EF scaffolder pragmas are exempted by a visible
generatedDirectoriesentry carrying its reason, so the exemption is data a reviewer reads in the diff rather than a regex hidden in the checker. The test proves both directions: undeclare the directory and the generated pragma fails.Issue #431 moved into the repository. Every one of its 27 per-file instant/bucket classifications is now an allowlist reason, and the 47 pragma comments that linked it were stripped:
The comment was deleted rather than rewritten, deliberately:
ORBIT0001permits only an XML doc or a URL-linked WHY note, so stripping the URL while keeping the prose would have turned 47 legal comments into 47 build errors. With the reason in the allowlist the comment has no job left.The five verdicts, committed with their evidence
They live in the allowlist's own
ruleVerdictsblock rather than in a prose document, and the checker refuses a verdict with no evidence, so no rule can read "to be decided". Placement reason: a verdict a reviewer has to leave the repository to read is the exact defect this gate removes, and the block sits beside the sites it judges.ORBIT0001NoCommentsORBIT0002RollbackInUsingTransactionORBIT0003ControllerAuthorization[AllowAnonymous], a visible attribute rather than a pragma, so it already has an in-language exemptionORBIT0004UtcNowUserFacingDate#230executes itORBIT0005DbSetFluentConfigurationFour of the five have no suppressions at all. The entire hand-written ORBIT surface is
ORBIT0004, and that reframes the ticket: this is one analyzer's exemption problem, not five.On
ORBIT0004, and why the narrowing is NOT done here. The analyzer's exemption logic is line-level: it excuses aDateTime.UtcNowread when the source line names an*AtUtcsymbol or a cache key, or when the value feeds aTimeZoneInfoconversion. A site that assigns to a local callednowUtcand passes it to a domain method taking an*AtUtcparameter does not match that line and fires.AuthSessionService.cs:33-35(var nowUtc = DateTime.UtcNow;) is exactly that shape and carries five of the sites. If dataflow into an*AtUtcparameter were recognised, a large share of the 47 would stop firing and their pragmas would be deleted rather than re-justified.It is not executed here because this ticket declares the sites as they are, and narrowing an analyzer silently changes what 47 sites mean before anyone has read them.
#230confirms it against the analyzer source and an analyzer-enabled build, then narrows or refutes it. The hypothesis is recorded as a hypothesis, in the allowlist, so#230does not start from a blank page.Wiring
Guard Conventionsintest.yml, which is already a required status check onmain(read live today: the 16 required contexts includeGuard Conventions, app 15368). Reusing it is the whole point: no branch-protection edit is needed and nothing is asked of you.lefthook.ymlgains a pre-commit step besidecheck-root-allowlist.mjsandcheck-dashes.mjs, so the failure arrives before the push. That matters more here than in most repos:CLAUDE.mdrecords that the in-box SDK compiler predates the analyzer'sMicrosoft.CodeAnalysis, socscskips the ORBIT rules locally withCS9057and an agent never sees the rule fire, only a red CI job, and then reaches for the documented suppression. This checker is plain Node, so it does run locally.Test evidence
The seventeen cases, each staging its own repository shape and running the real tool:
The runner is Node's built-in
node:test. This repository has nopackage.jsonand no JavaScript harness, and adding a dependency to a .NET repository to test a 200-line script is worse than using the runtime's own runner. The CI step runs it as a second step of the same job, so the checker's coverage is a required check too, not an honour system.Out of scope, and NOT quietly done
#230owns it, and it is still genuinely blocked:src/Orbit.Application/Social/Services/FriendGraphService.cs,src/Orbit.Domain/Entities/Report.csandsrc/Orbit.Api/Controllers/UserFactsController.csare all still present, so neither the social-layer deletion nor the AI-memory deletion has landed.ORBIT0004's verdict is recorded; the change is#230's..claude/rules/core.mdline carrying the escape-hatch principle. That file lives inorbit-ui-mobile(this repo's.claude/has norules/), so it is arepo:uiticket under D4.One manual step
thomasluizon/orbit-api#431should be closed with a comment pointing attools/suppression-allowlist.jsononce this merges. It is left open until then, because closing it before the file exists would point at nothing.Notes
No DTO, no
packages/sharedtype, no response shape, no migration. The append-only, deploy-API-first rule is not engaged, and there is no Play fleet dependency orMinSupportedVersioninteraction. Rollback isgit revert: removing the two steps fromGuard Conventionsrestores the previous behaviour exactly, and no runtime code changed.One unrelated housekeeping note: a stray untracked
error.logsat at the repository root, 758 bytes of Codex CLI diagnostics from an overnight run at 01:58 today, andRoot Allowlistcorrectly refused to let a commit through while it was there. It was moved aside rather than deleted, to this session's scratchpad asorbit-api-stray-error.log. Nothing in this diff touches it.🤖 Generated with Claude Code
https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc