refactor(checks)!: cut 20 linter-level checks from the catalog (CD-357) - #223
Open
TAJD wants to merge 8 commits into
Open
refactor(checks)!: cut 20 linter-level checks from the catalog (CD-357)#223TAJD wants to merge 8 commits into
TAJD wants to merge 8 commits into
Conversation
…advise --analyze (CD-357 pass 1/4) Cofferdam is a policy engine, not a linter. A check ships only if it reads the spec, needs the corpus/graph, or needs the pass-2 consistency mode. These eight fail that test and belong to Biome/oxlint: Warning.TripleEquals, Warning.NoConsoleLog, Warning.NoDebugger, Warning.NoEval, Warning.UnusedImport, Warning.UnusedNullCheck, Readability.MaxLineLength, Readability.MaxFunctionLength Both categories empty out entirely, so warning.rs and readability.rs are deleted outright. The Category::Warning and Category::Readability enum variants stay — they are public API, and how the five-category model is restated is a positioning question for the README rewrite, not a deletion question. Warning.ParseError is engine-internal, not a registered check, and is unaffected. Also removes `advise --analyze`. Its AnalyzeEnvelope was a hardcoded list of exactly five budget entries, all five of them checks being cut, so it is not generic over the catalogue and post-cut reports nothing. Keeping it would ship numbers that gate nothing, which is the metrics product we are explicitly not building. The [budgets] CI ratchet is a different feature and is untouched. Removing the whole subcommand also avoids a non-additive change to the advise JSON schema. Refs CD-357. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eUbK1Te7zdsrAXYeTSc1j
…ss 2/4) Removes Refactor.UnusedVariable, PreferConstOverLet, PreferOptionalChain, PreferNullishCoalescing, PreferArrayMethodOverLoop, MutatedParameter, CyclomaticComplexity, CognitiveComplexity and DuplicateBlock. All are style or smell rules Biome and oxlint already own; none reads the spec, needs the corpus, or needs pass-2 mode. duplicate_block.rs is trimmed rather than deleted: Refactor.NearDuplicateBlock survives and lived in the same file, so it absorbs the corpus-collection logic and becomes the sole writer. Exact-clone regression coverage is preserved via a cfg(test) helper. Also removes max_cyclomatic_complexity_in_file and max_cognitive_complexity_in_file, dead since pass 1 removed advise --analyze. The three suppression fixtures previously pointed at PreferConstOverLet, which this commit deletes. Because a suppression directive naming an unregistered check id is silently ignored (CD-395), that would have left the fixtures parsing, the tests passing, and Consistency.BroadSuppression / Consistency.UnusedSuppression exercised by nothing at all. They are re-pointed at Design.ReadonlyArrayParam, which survives, and a new integration test asserts a non-empty finding set so the next removal fails loudly instead of silently vacating the coverage. Refs CD-357, CD-395. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eUbK1Te7zdsrAXYeTSc1j
… 3/4) Removes Design.MaxParameters, Consistency.QuoteStyle and Consistency.SpellingDialect, completing the 20-check cut. consistency.rs is trimmed (3082 -> 1293 lines), not deleted: BroadSuppression, UnusedSuppression and ErrorHandlingIdiom survive in it. all_builtins() now registers 24 findings-emitting TypeScript checks, plus 5 context providers in their own registry. With the Rust and HTML adapter checks evicted by A3, that is the ~25-check target met without needing to trim the residue. Two structural consequences, both deliberate and both flagged upstream: QuoteStyle was the last builtin with consistency: true, so no builtin now uses pass-2 mode. Cache tests that depended on pass-2 dispatch to register hits are kept honest with a synthetic Test.NoopConsistencyCheck rather than being weakened. SpellingDialect was the only builtin overriding Check::languages() to reach markdown, so the test asserting builtin markdown reach is deleted — no repoint target exists. Design.MaxParameters was load-bearing well beyond its own check: the release smoke fixture, the starter cofferdam.toml, config doc examples and several integration tests all used it as a convenient trigger. Each is re-pointed at a surviving check, chosen per call site — Design.ReadonlyArrayParam where any finding will do, Refactor.LongAndComplex where a numeric option was needed. Refs CD-357. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eUbK1Te7zdsrAXYeTSc1j
…357 pass 4/4) Passes 1-3 removed the checks themselves; this clears what the compiler and test suite could not see. Doc prose, CLI help text, config examples, the starter TOML, generated checks.json, the repo's own dogfooding baseline, CI smoke scripts and the type-host fixtures all named checks a user can no longer run. Also drops the unicode-width direct dependency, which existed solely so Readability.MaxLineLength could measure display columns rather than UTF-8 bytes. Nothing else in the tree used it. Doc comments in cofferdam-core are updated rather than deleted where they explained a mechanism by example: Check::languages() and Language::Markdown both used Consistency.SpellingDialect as their illustration, and now record that no built-in declares a non-TypeScript language while plugins still can. That gap is tracked as CD-396. Verified: 1333 tests pass, clippy clean, fmt clean, gen-docs --check clean. Refs CD-357. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eUbK1Te7zdsrAXYeTSc1j
…s (CD-357) Completes the pass-4 sweep. The agent-facing recipe table in CLAUDE.md was the worst of the residue: four of its six rows pointed at files deleted in passes 1-3, so any agent following it would have opened nothing. The "text-line scan" row is deleted rather than repointed. No surviving check reads only file.lines() — that was the whole point of CD-357 — so the table now says so explicitly, and treats "needs neither graph nor AST" as a signal the rule belongs in a linter instead. Removes .claude/plans/post-v1-roadmap.md. It proposed adding NoConsoleLog, QuoteStyle and a TripleEquals autofix, which is the exact inverse of the direction this branch takes, and an in-repo plan doc that contradicts the tracker is worse than no plan doc. Suppression unit tests in consistency.rs and context/findings.rs still name removed check IDs. Left alone deliberately: they pass the ID list in explicitly and exercise the directive parser, so the strings are fixtures, not catalog references. Refs CD-357, CD-396. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eUbK1Te7zdsrAXYeTSc1j
Resolves nine conflicts between the check cut and #197's docs restructure. Main's prose wins wherever #197 deliberately rewrote a section (README, docs/index.md, MAINTAINERS roadmap pointer); this branch wins wherever the only difference was a check ID that no longer exists. docs/type-aware-checks.md keeps main's richer configuration example -- including the note that disabling the last type-aware check does not stop the type host spawning -- repointed from Warning.UnusedNullCheck to Design.UnionExhaustivenessGap. The merge also exposed a defect four sweep passes missed. doctor's formatter-coexistence check warned that cofferdam double-reports against biome/eslint and told users to disable four named built-ins -- all four of which this branch deletes. The honest answer is now the opposite of a warning: the catalogue is disjoint from a linter's by construction, so the check reports Pass and says so. Kept rather than deleted because users arriving from a linter expect an overlap and the useful answer is that there isn't one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eUbK1Te7zdsrAXYeTSc1j
Criterion benchmark comparison (PR head vs base branch) |
The page still described the old two-check split: it linked to the deleted Refactor.DuplicateBlock page (failing the VitePress dead-link gate) and, worse, told users to configure min_statements and friends under [checks."Refactor.DuplicateBlock"]. Pass 2 moved DUP_BLOCK_OPTIONS onto NearDuplicateBlock, so that advice pointed at a key that does nothing. gen-docs --check passes on both the old and new text — it verifies the generated page matches its source, not that the links resolve — so CI's docs build was the only thing that could catch this. Refs CD-357. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eUbK1Te7zdsrAXYeTSc1j
Criterion benchmark comparison (PR head vs base branch) |
…NGELOG Fixes found by an adversarial review of the A1 diff. The regression: partition_claimed_groups runs one overlap-claim pass over every candidate group, and exact-clone groups still claimed territory after pass 2 stopped emitting them. An exact group therefore suppressed any overlapping literal-drift group and then vanished itself, so adding a verbatim copy of a file made cofferdam report strictly less than before it was added. Both halves now report under Refactor.NearDuplicateBlock, distinguished by message wording. This also un-inerts include_tokens, whose fingerprints always have exact_hash == hash and so could only ever land in the discarded bucket. Two tests pinned the buggy behaviour and are inverted; a new test covers the suppression case directly, since the existing overlap test concatenates its fixtures so their spans never collide. finalize_all_groups_for_test now delegates to the real finalize instead of re-implementing it. The CHANGELOG's removal list was wrong in both directions. Eight ids it named never existed in this repo -- they are Credo rule names, copied from the ticket rather than read out of all_builtins() -- and eight ids that were actually removed went unmentioned, so a user upgrading was told to delete config for rules they never had while not being told Warning.NoEval and Refactor.UnusedVariable had stopped firing. The list is now derived from the diff. It also documents the severity consequence (exact clones dropped from medium to low, so duplication no longer fails a default build), the option relocation, and the three silent migration hazards. Restores duplicate_block_suppressed_from_either_occurrence, which covered engine behaviour rather than the removed check, repointed and strengthened to assert the finding fires before asserting a comment suppresses it. doctor's coexistence message overstated its case: Refactor.LongAndComplex does overlap ESLint's complexity rules. It now names that one rather than claiming no overlap exists. gen-docs no longer publishes a category heading with nothing under it -- Readability is empty as of CD-357 and rendered as a bare heading. Refs CD-357, CD-396. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eUbK1Te7zdsrAXYeTSc1j
Criterion benchmark comparison (PR head vs base branch) |
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
1 participant
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.
Workstream A1 of the policy-engine repositioning. See ROADMAP.md; detailed tracking is private.
What this does
Removes 20 checks that judged code a line at a time — the ones a linter already does better, and that made cofferdam read as "another linter with fewer rules". The catalogue goes from 47 published pages to 27.
Warning.*Readability.*Refactor.*Design.*Consistency.*Result: 24 findings-emitting built-ins, plus 5 context providers in their own registry — the ~25 target met on the cut list alone, with no residue trimming.
Refactor.LongAndComplexcovers the intersection the two standalone complexity checks aimed at.Refactor.NearDuplicateBlockabsorbedRefactor.DuplicateBlockand reports both verbatim and literal-drift clones.Breaking
Config keys, suppression directives and baseline entries naming a removed id stop matching.
[checks."X"]and[budgets]warn;[[overrides]]and suppression comments do not — those are silent, and are called out in the CHANGELOG as manual migration steps.Duplicate detection is not reduced, but its severity changed: verbatim clones reported at
mediumand gated a default--fail-on mediumbuild; they now report atlowand no longer fail a build by default. Restore with[checks."Refactor.NearDuplicateBlock"] severity = "medium".cofferdam advise --analyzeis removed outright. All five of its budget entries were hardcoded to checks on the cut list, and reporting un-gated complexity numbers is explicitly not what cofferdam is for. The[budgets]CI ratchet is a separate feature and is untouched.Found by review, fixed here
An adversarial review of the diff caught a real regression that the green test suite did not.
partition_claimed_groupsruns one overlap-claim pass across all candidates; after the cut, exact-clone groups still claimed territory but were no longer emitted — so an exact clone silently suppressed any overlapping near-duplicate and then vanished itself. Adding a verbatim copy of a file made cofferdam report strictly less. Both halves now report; two tests that pinned the buggy behaviour are inverted and a direct regression test added.The same review found the CHANGELOG's removal list wrong in both directions — eight ids named that never existed here (Credo rule names, copied from the ticket rather than read from
all_builtins()) and eight real removals unmentioned. The list above is derived from the diff.Notable
doctor's formatter-coexistence check warned that cofferdam double-reports against Biome/ESLint and named four built-ins to disable — all four are deleted here. It now passes, and names the one genuine remaining overlap (Refactor.LongAndComplexvs ESLintcomplexity) rather than claiming none exists.gen-docsno longer publishes a category heading with nothing under it:Readabilityis now empty.Two engine capabilities are left with no first-party user — pass-2 consistency mode and
Check::languages()— both still reachable by plugins, tracked separately.Verification
cargo build --workspace,cargo test --workspace(1335 passed),cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --all --check,cofferdam gen-docs --check— all clean.🤖 Generated with Claude Code
https://claude.ai/code/session_012eUbK1Te7zdsrAXYeTSc1j