feat(repo): track cognitive complexity across changes - #3849
Open
helix-nine wants to merge 17 commits into
Open
Conversation
Adds `make complexity`, `complexity-top` and `complexity-diff` — a tree-sitter census over Rust and TypeScript that runs in 0.6s with no build, and an RFC proposing the protocol that puts its numbers in front of a PR author. Cognitive complexity rather than cyclomatic: probed against a synthetic file, a 20-arm match scores cyclomatic 21 / cognitive 1, five `?` operators score 6 / 0, and four-deep nesting scores 5 / 10. Cyclomatic ranks an enum-to-string match as the worst function in the repo; cognitive does not rank it at all. Inline `#[cfg(test)]` items are stripped before measuring — this repo keeps 1,150 test functions inline against 6,249 lines in dedicated test files, so a path rule would make a PR that adds tests read as one that adds complexity. Generated trees are skipped, and the census refuses to report below 98% parse coverage, because grammar rot is otherwise silent. The RFC carries the baseline, the growth measurements that motivate it, and the enforcement questions that need a decision before any of it is wired to CI.
A file the parser fails on yields no functions rather than an error, so file coverage catches a grammar that stops reading a file entirely — but not one that degrades inside it. Counting ERROR nodes catches both. Six Rust files carry parse errors today, all on generic associated types, which stabilized in Rust 1.65 two months after the pinned grammar was cut: 240 of 2,553,851 nodes, 0.009%. The guard trips above 0.5%, fifty times that.
An adversarial pass defeated the first version with the refactor an agent reaches for first. Cognitive complexity penalises nesting superlinearly, so extracting nested blocks lowers the total however bad the split: a deliberately worse six-way split threading loop state through `&mut` parameters takes total cognitive 26 -> 9. The claim that function count and total would expose it was simply wrong. Cyclomatic is near-additive and survives relocation — the same split takes it 10 -> 16. The census now reports both and names the pattern when cognitive falls while cyclomatic rises. Adds the one question in the rubric an author cannot bluff, because the census counts it: new functions with exactly one call site. That is the premature helper, the most common complexity defect in generated code; the portmap PR added 83. Counting is a single tokenizing pass, 0.8s for the whole repo. The two conditional questions are asked only when the census reports them. 61% of source PRs carry a near-zero delta against 19% that are substantial, and a section mandatory on all of them is a rubber stamp rather than a gate. Also reports lines inside `macro_rules!` bodies. The parser does not expand macros, so wrapping a body in one takes it from cognitive 15 to 0 — no fix inside this tool, but the volume is at least visible.
Sonar defined cognitive complexity and rust-code-analysis implements their spec, so the metric was never the homegrown part. SonarQube Cloud is free for public repositories, covers Rust and TypeScript, computes the metrics itself rather than only importing Clippy, and adds duplication, a new-code quality gate and PR decoration — strictly better than a local census for measuring and tracking. Records the three things that decide whether it replaces this: Community Build analyzes the main branch only, so self-hosting gives no pre-merge gate; Sonar identifies test code by path while this repo keeps 1,150 test functions inline; and a quality gate reports rather than asks for a justification. If Sonar handles cfg(test) and ranks this repo sensibly, the census should be deleted and only the protocol kept.
Diffing this census against SonarSource's reference implementation of the metric found it counting the file-level container as a function and folding every nested closure into its parent. `cognitive.sum` is an aggregate over nested spaces, not a function's own score. Taking `sum` minus the direct children's sums moves rank agreement with the reference from Spearman 0.822 to 0.954 and drops the repo total 29,505 -> 22,136, with functions over 25 going 158 -> 103. `add_public_domain` leaves the worst list entirely — it was absorbing two closures. `list_conffiles` now scores 70, matching an independent measurement of the same function. Records why anything is vendored at all: for TypeScript, SonarSource's own `eslint-plugin-sonarjs` runs locally in 1.4s with no server and should be preferred outright. For Rust no local implementation of the metric exists — Clippy measures macro-expanded HIR, lizard's Rust reader never counts match arms. SonarQube itself is the wrong shape: complexity needs no server, and its free self-hosted tier analyzes the main branch only, so it cannot gate a PR.
…ce grounds Sonar's language analyzers are under the Sonar Source-Available License v1, not an open-source licence — SonarJS, sonar-rust, sonar-python, sonar-java and sonar-dotnet all carry it; only the SonarQube platform is still LGPL-3.0. SSAL grants rights solely for a Non-competitive Purpose, which excludes "employing, using, or engaging artificial intelligence technology that is not part of the Program to ingest, interpret, analyze, train on, or interact with the data provided by the Program". An agent reading a complexity report is the case this work exists to serve, so the grant does not reach it. eslint-plugin-sonarjs is the trap: package.json still declares LGPL-3.0-only while the shipped LICENSE and every source header are SSAL v1, so a scanner reading package metadata passes it.
The earlier retraction read clause (c) bare and got it wrong. SonarSource's own MCP server hands analyzer output to third-party LLM agents, ships under byte-identical SSAL v1.0, and carries a sentence added by their VP Legal in a pull request titled "Clarify SSAL language with regards to MCP usage": using it is a Non-Competitive Purpose and so allowed. That is a construction of a defined term, not a waiver, and the term is defined by purpose rather than by product. Records the honest weak point: no analyzer repository carries that sentence, and the 2024 announcement glosses (c) broadly and was never retracted — so the narrow reading rests on the licensor's conduct and construction rather than the text. Keeps the tool recommendation where it was, now on technical grounds alone.
Replaces the vendored rust-code-analysis wrapper with `bca`, a maintained fork of the same engine under MPL-2.0, already on the deny.toml allowlist. One binary and one pass covers Rust and TypeScript in 0.35s. Three of its flags delete code this repo was carrying itself. `--exclude-tests` skips #[test]/#[cfg(test)]/#[tokio::test]/#[rstest] subtrees and reproduces the hand-rolled brace-matching stripper exactly — 24 functions and cognitive 66 on volume.rs either way — so the stripper goes. `cognitive.value` is a space's own score, so the sum-minus-children arithmetic goes too. A generated-code detector and sha256-pinned multi-platform releases come with it. Drops the macro-body line counter. A body inside macro_rules! scores zero in every tool including SonarSource's own analyzer, and macro bodies are 0.19% of this repo's Rust — a bespoke counter for that is not worth its own code path. Not Sonar, and not on licence grounds: their analyzers are source-available rather than open source, the platform needs a server, and its free self-hosted tier analyzes the main branch only, so it cannot gate a pull request.
The rewrite onto bca dropped the parse-coverage guard, and the RFC went on promising it — the kind of claim a diff shows as unchanged context. Restores it against bca's own ERROR-node count, and adds the case the original guard missed: a census that finds no functions at all now exits non-zero instead of reporting zero complexity and passing every threshold. A failed analyzer reports its own last stderr line rather than a traceback. The margin is wide. bca finds 1 parse error in 2,681,337 nodes here; the engine it forked finds 240, clustered in six files on generic associated types and `impl Trait` in argument position — syntax postdating its January 2023 grammars.
A general-purpose utility has one call site the day it is written, so asking an author to defend every single-use function charges a toll on the library we want and collects it as inlined helpers and helpers bent to fit one caller. Measuring it showed the signal was not merely unhelpful but inverted. Against a function that retried an HTTP call inline: extracting a generic retry_with_backoff into shared-libs takes cognitive 8 -> 5 and FIRED the "branches were relocated" warning, while shredding the same logic in place into three helpers threading &mut state takes cognitive 8 -> 11 and stayed silent. A real extraction lowers cognitive and adds a function exactly as a bad split does, so that warning cannot separate them. Deleted. What separates them is where the callers are. The census now marks a function shared when anything outside its own file calls it — 76% of named functions here, against 9% single-use beside their only caller. The report credits additions to shared-libs, counts private single-use helpers without demanding a defence of each, and asks no question about either. The surviving reuse question — which existing helper you checked before adding a new one — pushes toward reuse rather than away from it.
Rewarding a new utility rewards writing a second one instead of finding the first. A function now earns its line in the report only once two or more distinct subsystems call it — a subsystem being a product or crate plus its first module segment. 59% of the 1,208 functions in util modules clear that bar today. Util-module complexity is reported apart rather than zeroed. Not taxing utilities and not rewarding duplicates pull against each other, and exempting util complexity resolves that the wrong way: it makes the util module free parking for the near-duplicate the rule exists to discourage. Measured against a tree already holding a generic retry_with_backoff, a second product writing its own copy costs +4 cognitive whether it lands beside its caller or inside the util module, while calling the existing one costs 0. An earlier line claimed util complexity was not counted against the delta. It was counted; the line was wrong and now states what the code does. Nothing mechanical catches a re-implemented utility: jscpd flags a renamed copy at 45% duplicated lines but finds zero clones for the same helper written afresh. The rubric question about checking for an existing helper is the lever there.
Credit only fired for functions a diff introduced, so adopting an existing helper — the behaviour most worth encouraging — earned nothing at all. Credit now follows adoption: a function earns its line when a subsystem that did not call it before starts to, and only once the total reaches two. The first caller is the author; the second is where generality stops being a claim. That ordering makes reuse dominate duplication with no penalty on utilities. Against a tree already holding a generic retry_with_backoff, a second product writing its own near-copy earns nothing because that copy reaches one subsystem, while calling the existing helper adds no code and credits it at two subsystems. Drops the argument that exempting util complexity would make the util module free parking for duplicates. Nothing gates on the totals — the only question the gate can require concerns a function pushed over 25 — so complexity arriving in a util module is already unpenalised in the sense that operates.
A metric with a reward attached gets optimised, and every cheap way to optimise this one makes the code worse: splitting a clear function into six poorly-named pieces lowers its score, hiding a body in macro_rules! takes it to zero, and leaving a helper inlined avoids a new function. So nothing here fails a build, blocks a merge, or caps a number. Removes the PR-body gate, its make target and its rubric enforcement. What remains prints what a branch did — the totals, every function it pushed higher, every one it simplified, and which utilities a second subsystem now depends on — so an unexpected rise reads as a symptom to look at, and a rise the author stands behind can be pointed at and explained. Complexity intrinsic to a requirement is still complexity, and the report names functions rather than totals so that case can be argued. Adds `build/complexity/history.tsv`, one row per master commit, seeded with 28 sampled points. Only master CI appends to it: a totals file that pull requests edit conflicts on 75.4% of median-lifetime branches, and one written after merge conflicts on none. Step changes in it are usually imports — 16,188 to 22,032 on 2026-07-02 is start-wrt and start-cli arriving, not a bad week. Retitles the RFC, which no longer describes a budget.
Three bullets under Opening PRs: run `make complexity-diff` and paste it, treat an unexpected rise as a symptom to read rather than a verdict to obey, and expect no charge for adding a utility. Nothing gates on the numbers, so the rule is only worth its space if it says plainly what to do with them and when to argue back. +2.9% on AGENTS.md.
'A function grown a branch it did not need, a helper shredded instead of extracted, a special case threaded down a call chain' was three strained metaphors in one sentence. It is a condition that did not need adding, one clear function chopped into worse ones, or a special case passed down through layers that should have handled it at the top. Also drops the owed/earned framing from the utilities bullet and the tangled opening clause from the first.
An unenforced instruction to run the census gets followed about 28% of the time, measured against the closest precedent in AGENTS.md. So the PR appends its totals to the log and CI checks that a row describes the tree being shipped. That checks the census was RUN, never what it said. No value fails a build, which is what keeps the metric worth reading rather than worth gaming. Push another commit and the row goes stale, and the check says so. Requiring every PR to append to one file is normally how you manufacture the conflict this repo already gets from its append-only i18n dictionaries — two branches adding different rows at the same tail collide on every merge, which I confirmed on a scratch repo. One line of .gitattributes removes it: the log is merge=union, so git keeps both sides. Three concurrent branches each appending a distinct row merged cleanly with every row preserved. `make complexity-record` now prints the delta and appends the row in one step; `make complexity-verify` is what CI runs.
The first green run of the Complexity job passed for the wrong reason. A pull_request event checks out head merged into the current base, so with master seven commits ahead CI measured a tree the author never saw — and its figures happened to match a row backfilled from master two days earlier. Green, and meaningless. Two fixes. The job pins ref to the pull request's head sha, so it measures what the author measured; numbers from a merge commit are numbers nobody could have recorded. And verify compares the last row instead of searching the whole log, so a coincidental match against any historical row can no longer pass it. Confirmed both ways: a stale last row now exits 1 naming both figures, and the correct row exits 0.
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
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.
Tracks what a change does to this repo's complexity, and how the figure moves over time.
It gates nothing — no build fails, no merge blocks, no number is capped.
Full write-up:
rfcs/complexity-tracking.md.Why it isn't a gate
A metric with a reward attached gets optimised, and every cheap way to optimise this one makes
the code worse. Splitting a clear function into six poorly-named pieces lowers its cognitive
score — measured, a deliberately bad six-way split takes 26 → 9. Hiding a body in
macro_rules!takes it from 15 to 0. Leaving a helper inlined avoids a new function. Those are what a gate
would buy.
So the numbers are context for whoever is making the change. An unexpected rise is a symptom
worth looking at; a rise the author stands behind should be explicable, and the report names
functions rather than totals so it can be.
What it prints
make complexity,complexity-top,complexity-diff,complexity-record.Tracking over time
build/complexity/history.tsv— one row per master commit, seeded with 28 sampled points.Only master CI appends to it, which is what keeps it free: simulating real merges over 60
code commits, a totals file that PRs edit conflicts on 75.4% of median-lifetime branches;
one written after merge conflicts on none.
Read it for shape. Step changes are usually imports — 16,188 → 22,032 on 2026-07-02 is
start-wrt and start-cli arriving in the monorepo, not a bad week.
The metric
Cognitive, not cyclomatic, via
bca(MPL-2.0, already on our
deny.tomlallowlist). One binary, one pass, both languages: 527 Rustfiles and 759 TypeScript in 0.35 s, no build, no
npm install, sha256-pinned.match?operatorsifCyclomatic ranks an 84-line enum-to-string match as the repo's worst function. Cognitive doesn't
rank it at all.
Baseline
Worst:
ipv6_set155,net_controller::update150,update_addresses144.Utilities are not charged for
A general-purpose utility has one call site the day it's written, so anything charging for a
single-use function charges for the library we want. Credit attaches to adoption, not
creation — a function is named only when a subsystem that didn't call it before starts to, and
only once the total reaches two. Writing a second utility for a job an existing one already does
therefore earns nothing, while calling the existing one does.
Verification
prettier --check .clean.(currently 1 in 2,681,337) — a dead analyzer used to look like perfectly clean code.
How an agent finds it
Three bullets under Opening PRs in
AGENTS.md(+2.9%): runmake complexity-diffand pasteit; treat an unexpected rise as a symptom to read rather than a verdict to obey; expect no charge
for adding a utility. Without that an agent never looks.
Paired with helix#130, which adds the census to
the code-review skill's Simplification angle — one input among the four cleanup angles, framed
as a pointer rather than a finding, since it misleads both ways.
Still to decide
Whether master CI appends to the history log. It is the only piece needing write access, and
the only thing standing between this and a real time series. Everything else works today.