feat(cli): derive default metric thresholds from evidence - #1148
Merged
Conversation
The `[thresholds]` table `bca init` scaffolds was chosen by eye and
pinned byte-for-byte to this repository's own `bca.toml` by a drift
test, so "what adopters get on day one" and "what this Rust project
gates itself at" were the same number by construction.
Introduce `default_thresholds.rs` as the single source of truth and
render the scaffold's `[thresholds]` block from it. The drift test now
pins the template to that table instead of to the repo root, leaving
this project free to keep its own calibration.
Retune the limits against published anchors plus a measured corpus of
43 repositories across 20 languages, sized so each flags roughly the
worst 1-3% of spaces in the median language:
cognitive 25 -> 15 SonarSource's own default
nargs 7 -> 5 RuboCop's value; 7 fired on <1% of
functions and on nothing here at all
abc 50 -> 40 aligns ABC with cyclomatic's strictness
loc.ploc - -> 600 new working file-size limit
loc.sloc 800 -> 1200 demoted to a bloat backstop
cyclomatic, nexits, halstead.effort, nom, and wmc are unchanged.
AGENTS.md gains the summary table and the three caveats an agent needs
before quoting a number, guarded against drift by a new test.
Refs #1140
Nothing documented where the shipped limits came from, which ones to change for a given language, or how the right table differs by job. Adds a Recipes page covering all three: the derivation and its corpus, a per-language calibration table with the 97.5th percentile for each supported language, and four profiles (blocking CI gate, agent feedback loop, legacy triage, safety-critical). Records the caveats that make the numbers honest: the C# distribution is skewed by property accessors, `nom` and `wmc` measure an Elixir `defmodule` rather than a class, `nargs` is inert for Bash, Perl, and Elixir, and the file-size figures for four languages rest on samples too thin to derive from. Refs #1140
The `loc.sloc = 800` limit was calibrated for test-inclusive SLOC and never revisited after `exclude_tests = true` landed twelve days later, so its stated derivation stopped holding. Because SLOC counts comments, it also charged the same price for a paragraph of rationale as for a new branch, and seven files had settled into a four-line band ending exactly on the soft limit, which is trimming-to-fit rather than a natural distribution. Gate `loc.ploc = 550` as the working file-size limit (just above this repo's p99 of 545, and deliberately stricter than the shipped default of 600) and keep `loc.sloc = 1300` as a bloat backstop with real headroom over today's largest file. Also records why cognitive, nargs, and abc stay looser here than the shipped defaults, and points at the convergence plan rather than inventing a calibration that does not exist. Baseline regenerated in the same commit, per the baseline-refresh discipline: adding a metric is a tightening. Closes #1138 Refs #1140, #1143
Follow-up review of the #1140 change. The rendered scaffold claimed to be "per-function threshold configuration" while listing file- and container-scoped limits, and its "Metrics intentionally NOT gated" list read as exhaustive while omitting loc.lloc, loc.cloc, and loc.blank. Both were wrong in the one artifact adopters actually read. The same two defects were in the repo-root manifest and are fixed there too. Entries also rendered with no separator, so each rationale block butted against the key above it and a reader could not tell which key a comment annotated. They are now blank-line separated. Splits the header prose into `THRESHOLDS_HEADER`, leaving the renderer as loop logic; the two change for unrelated reasons. Test side: `rendered_prose_lists_every_known_metric` pins both hand-curated metric inventories against the registry, which is what caught the missing loc.* names. The doc drift guard now also covers the book's summary table, which was a third unpinned copy, and resolves the scope column through `thresholds::metric_scope` instead of re-deriving its unknown-id fallback. Corrects the doc on `default_table_builds_a_threshold_set`, which had the relationship to `every_default_name_is_a_known_metric` backwards: `build` normalizes aliases first, so the latter is what rejects a default spelled `sloc`. Refs #1140
`rendered_prose_lists_every_known_metric` searched the whole rendered block, where every known metric already appears as a rendered `name = limit` key or in the other inventory. Deleting the entire "Available names" list failed zero tests, so the half of the assertion covering it proved nothing. The NOT-gated half had a weaker version of the same flaw: it kept everything after the heading, including the per-entry rationale comments, so a name mentioned in a rationale satisfied it without being listed. Both halves now assert against the parsed inventory block, bounded by its indent, and compare tokens rather than substrings so `cyclomatic` cannot be satisfied by `cyclomatic.modified`. `doc_summary_tables_match_the_default_table` only asserted rows were present, so retiring a default would leave a stale row in AGENTS.md and the book advertising a limit `bca init` no longer writes. It now also asserts set equality, with a row parser matched on full shape so the unrelated backticked tables in both documents are ignored. The AGENTS.md sentence describing the guard was wrong in the same direction: it claimed the test "fails if a second copy appears", when it only pins the paths in `DOCS_REPEATING_THE_TABLE`. Both hardened guards verified by perturbation: dropping a name from the inventory and adding a stale row each fail exactly one test. Refs #1140
The book asserted that Bash and Perl "have no formal parameter lists, so 0 is correct". That holds for Bash but not for Perl: subroutine signatures (`sub add($x, $y)`) have been stable since 5.20 and are on by default under `use v5.36`. The grammar parses one into a `function_signature` node; `nargs` just does not count it, exactly like the Elixir gap in #1142. Documenting the gap as correct behaviour is worse than not documenting it, since it tells a reader there is nothing to file. Filed as #1147 and cited from both the book and the scaffolded rationale. Refs #1140
The recipe's worked example passed `--threshold cognitive=25`, the pre-#1140 default. After the retune that number is neither the shipped default (15) nor what the new Choosing thresholds page recommends for this exact use case (10), so the two recipes disagreed on the page whose whole subject is the agent loop. Uses 10 and links the profile, with the one-line reason an agent loop warrants a tighter limit than a blocking gate. Refs #1140
The key-quoting branch duplicated `push_str(entry.name)` across both arms. Decide the quoting into a variable and emit the name once, so the two arms cannot drift. Scaffold output is byte-identical. Refs #1140
`init_scaffold_is_discovered_by_zero_config_check` scaffolds, runs a bare `bca check` on trivial source, and asserts exit 0. That proves the gate ran, but a scaffold with every limit at 1_000_000 passes it identically, so on its own it cannot tell a working gate from an inert one. That distinction is the whole point of #1140: `nargs = 7` was retired because it fired on under 1% of functions and on nothing in this repository at all. An inert limit is the bug, not the baseline. Adds the other direction — a six-argument function must fail the scaffolded gate with exit 2, naming the metric and the offender. Also pins the two-tier file-size invariant: `loc.sloc` is a backstop and must stay looser than the `loc.ploc` working limit. Inverting them makes the backstop binding and turns the file-size gate back into a comment gate, which is what #1138 diagnosed; the reasoning previously lived only in prose. Both verified by perturbation: raising `nargs` to 500 and dropping `loc.sloc` below `loc.ploc` each fail exactly one test. Refs #1138, #1140
The caveat listed Bash, Tcl, JavaScript, and C++ as resting on the smallest per-function samples. Measured counts are bash 236, tcl 603, javascript 1435, objc 1999, cpp 2465 — Objective-C is fourth, C++ fifth. The sentence told readers to distrust the C++ figures while vouching for the Objective-C ones, which is backwards. Refs #1140
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.
Summary
The
[thresholds]tablebca initscaffolds was chosen by eye and pinnedbyte-for-byte to this repository's own
bca.tomlby a drift test, so "whatadopters get on day one" and "what this Rust project gates itself at" were the
same numbers by construction. This derives them from evidence instead,
decouples the two, and documents per-language and per-use-case guidance that
did not exist anywhere.
Evidence
43 shallow-cloned real-world repositories across 20 languages (test, vendored,
and generated trees excluded), measured with the release
bcabinary: ~254kfunction spaces, 40k container spaces, 27k files. Values binned by the same
File / Function / Container scope
bca checkapplies, so a container'snomnever lands in a per-function distribution.
Published anchors disagree by wide margins — cyclomatic defaults alone span 7
(RuboCop) to 30 (
gocyclo) — so each limit is anchored to a published valueand checked against measurement. The sizing rule: a default should flag
roughly the worst 1-3% of spaces in the median language. Below that it is inert
and gives false comfort; above it the gate becomes a style rule people route
around.
Shipped defaults
cognitivenargsabcloc.plocloc.sloccyclomatic,nexits,halstead.effort,nom, andwmcare unchanged.halstead.volumeis deliberately not gated: the widely-cited "function volumeunder 1000" guideline fires on ~7% of functions in the median language and 20%
in the worst, which is advisory-grade, not gate-grade.
Existing
bca.tomlfiles are unaffected; this changes what a freshbca initwrites.
Decoupling
default_thresholds.rsis now the single source of truth, and the scaffoldrenders its
[thresholds]block from it, rationale comments included. Theinit_template_thresholds_match_repo_rootdrift test is replaced by onepinning the scaffold to that table, leaving the repo-root manifest free to keep
its own Rust-specific calibration (it says so inline).
Documentation
New book recipe
Choosing thresholds: the derivation, a per-language tablewith the measured 97.5th percentile for every supported language, and four
use-case profiles (blocking CI gate, agent feedback loop, legacy triage,
safety-critical).
AGENTS.mdcarries the summary table and the caveats socoding agents read them by default.
Recorded caveats that make the numbers honest: C#'s distribution is dominated
by property accessors, which
bcacounts as functions;nomandwmcmeasurean Elixir
defmodulerather than a class, so ~1/3 of Elixir modules breach;nargsis inert for Bash, Perl, and Elixir; and four languages have samplestoo thin to derive file-size figures from.
Repo self-scan (closes #1138)
loc.sloc = 800was calibrated for test-inclusive SLOC and never revisitedafter
exclude_tests = truelanded twelve days later. Because SLOC countscomments, it charged the same price for a paragraph of rationale as for a new
branch, and seven files had settled into a four-line band ending exactly on the
soft limit — trimming-to-fit, not a natural distribution.
This repo now gates
loc.ploc = 550(just above its p99 of 545) withloc.sloc = 1300as the backstop. Baseline regenerated in the same commit; net21 fewer entries.
Testing
make pre-commitgreen. Every new guard was verified by perturbation ratherthan by passing alone — each fails with the intended message, and nothing else
does:
entry.limit + 1fails the round-trip, scaffold, and quoting testsAGENTS.mdor book table fails the doc drift guardnargsto 500 fails the new offender testloc.slocbelowloc.plocfails the two-tier invariantThe offender test is the one worth calling out: the pre-existing scaffold test
asserts a clean file passes, which a scaffold with every limit at 1,000,000
would also do. Since this PR's central finding is that an inert limit is the
bug, "the defaults catch something" needed its own coverage.
Follow-ups filed
[thresholds]inbca.toml. The per-language table iscurrently advice a polyglot repo cannot apply, since one manifest carries one
table.
nargsreports 0 for every function.nargsreports 0 for signature subs. Found while correcting aclaim in this PR's own docs that said the Perl zero was correct.
cognitive/nargs/abconto the shippeddefaults, one metric per change. Deliberately not done here: converging all
three at once costs ~95 baseline entries in one commit, which reads as debt
rather than as a decision.
bca.tomlsays so inline.Closes #1140