fix: batch of eight issues (#1136, #1143, #1159-#1164) - #1193
Merged
Conversation
`FuncSpace::new` and `Ops::new` both branched the end-row `+ 1` on
`SpaceKind::Unit`. That branch was really a statement about nodes
ending at column 0, which the file root always does — so any grammar
whose last function *also* ends at column 0 had its span run a line
past its parent unit and past EOF.
Perl is the only such grammar today: `sub f {…}` alone in a four-line
file reported `unit 1..4, function 1..5`. A child space outside its
parent is not a representable tree, and a span past EOF is the shape
that produced the release `usize` underflow in #1051.
Replace the kind branch with `Node::end_line`, which keys the 0-based
to 1-based conversion on the node's end column, and route all three
producers through it: the metrics walk, the ops walk, and
`bca functions` / the web `/function` endpoint, whose blanket
`end_row() + 1` had the same defect and would otherwise now disagree
with `bca metrics` about the same function. `Ops` and `FuncSpace`
share the rule through `spaces::line_span` so a future half-fix
cannot reintroduce the divergence #1130 closed.
The unit span also moves for a source not terminated by a newline —
previously an N-line file reported `1..N-1`. Only the verbatim
library API can observe that; every file-reading path appends a
newline first.
Adds a cross-language invariant test over the same exhaustive `LANG`
fixture table the ops/metrics parity check uses: every space lies
inside its parent's span, and the file-level unit ends on the file's
last line. Neither claim needs to know the right span per grammar, so
it covers all 25 variants rather than the ones anyone wrote a fixture
for. Both assertions were verified by reverting the production rule.
Swept 15,067 real files (repo + every integration corpus) and 792
truncated variants across 18 languages: no containment violation and
no inverted span. No integration snapshot moved.
Fixes #1163
`function()` — behind `bca functions`, `Ast.functions` in the Python bindings, and the web `/function` endpoint — opened a span on the byte-less `Checker::is_func`. Elixir's `def` / `defp` / `defmacro` / `defmacrop` are not grammar productions but plain `Call` nodes whose target identifier text spells the keyword (#275), so a predicate with no source bytes cannot see them: `bca functions` printed nothing, and exited 0, for an Elixir file whose `bca metrics` tree was a full module/function tree. `code` and `ancestors` were already in hand at the call site, so this is the same one-line correction #1130 made to the `ops` walk. `is_func_with_code`, deliberately not `promotes_to_func_space_with_code`: this seam enumerates functions, not every node that opens a space, so an Elixir `defmodule` stays out for the same reason a Rust `impl` block and a Java class already do. Every other language inherits the byte-less `is_func` through the default impl, so nothing else moves — no snapshot in the CLI suite or in the `big-code-analysis-output` submodule changed.
`bca find --type function` and `bca count --type function` build their predicate in `Parser::filters`, which had neither the source bytes nor an ancestor chain in scope, so the `"function"` arm asked the byte-less `Checker::is_func` and matched nothing for Elixir — the same silently empty answer the previous commit fixed in `bca functions`. `Filter` now carries a lifetime and the arm captures the parser's code slice, which leaves the other four named filters untouched. Widening the boxed signature to `Fn(&Node, &[u8])` was the alternative and is not needed: `find` and `count` both drop the `Filter` before the parser. Recorded at the call site that `is_call` / `is_comment` / `is_error` / `is_string` need no equivalent. That is structural rather than a survey: all four take `&Node` and nothing else, so no language *can* make them text-dependent. Only three `Checker` predicates accept `code`, and `"function"` is the only filter that reaches one. `Ancestors::unknown()` stays. Elixir's `is_func_with_code` does consult the chain — `elixir_is_inside_quote_block` (#310) — so this is now an `O(depth)` climb rather than the `O(1)` the #1088 comment described, but only for a `Call` whose target already spells a method macro. Measured on a generated 16,143-line Elixir file with 1,260 `def`s: `bca count --type function` 210.0 ms against `--type all` 205.4 ms (best of 12, interleaved), a 2% gap inside the run-to-run spread. Adds the durable half — a `functions()` / `find()` / `metrics()` parity test over the same exhaustive `LANG` fixture table the ops/metrics and span-containment checks use, so a new language cannot be added to one and missed by the others. It asserts both directions, since either alone is satisfiable by a broken seam: every span `functions()` reports is a `Function` space in the metrics tree, and every *named* `Function` space comes back out of `functions()`. Anonymous spaces are excluded from the second claim because whether a closure is a *function* is a per-grammar judgement — a JS arrow function and an iRules `when` block are, a Rust closure and a Go func literal are not. Verified by perturbation, one at a time, against the whole 5,057-test workspace suite: reverting either predicate to `is_func` fails this test and nothing else, and substituting `promotes_to_func_space_with_code` fails it alongside five existing tests. `Parser<T>::filters` is baselined for `halstead.effort` and moves 53,278 to 58,861 — one more unique operand and one longer operator name, on a function that lost four lines. Baseline refreshed in this commit. Fixes #1162
`Java::CompactConstructorDeclaration` was referenced nowhere outside the
generated enum, so a record's compact constructor (`record R(int a) { R
{ … } }`, JLS 8.10.4) opened no `FuncSpace`: its control flow was charged
to the enclosing class, `bca check` could never flag one however complex
it got, and `nom` / `wmc` / `nargs` under-counted the record.
Added it to the four dispatch sites where `ConstructorDeclaration`
already appears, in one commit per `.claude/rules/grammar-dispatch.md`
item 7: `Checker::is_func`, `Getter::get_space_kind`, and both the
function-boundary arm and the `stops` list in `cognitive/java.rs`.
`Nom`, `Npm`'s `direct_child_funcs`, `Wmc` and `NExits` need no edit of
their own — they key on `is_func` or on the space kind, so the two
predicate changes carry them.
Two decisions the issue left open:
* **Space name — `R`.** `compact_constructor_declaration` carries a
required `name` field holding the record's simple name, so the default
`get_func_space_name` already reports `R`, identical to the canonical
constructor. No override needed, and no `<anonymous>` space: `bca
check` prints `R::R: cognitive = 7` and `bca functions` lists `R`.
* **`nargs` — the record's component count.** The compact form has no
`parameters` field; its parameters are the record's components, which
the grammar hangs off the enclosing `record_declaration`. A new `impl
NArgs for JavaCode` redirects the lookup two ancestors up so the two
spellings of one constructor agree — `record R(int a, int b) { R { … }
}` reports 2, exactly as `R(int a, int b) { … }` does.
Tests cover the reproducer (asserting `class R` scores 0 as well as
`function R` scoring 2), the nesting reset and the depth surcharge on
separate fixtures that each isolate one line, `nom` / `npm` / `wmc`
counting it, `nexits` attributing the constructor's `throw` to it rather
than to the class, a record carrying both a compact and a delegating
constructor, the component-count `nargs` over *nested* records with
different component counts, and a no-constructor control pinning that
the `RecordDeclaration` arm of `is_func_space` was left alone. Each was
verified by perturbing the one production line it names.
No integration snapshots move: records are Java 16+ and the 183-file
Java corpus has none. `.bca-baseline.toml` records `nargs.rs`'s new
`loc.ploc` (525 -> 546), which the soft tier requires in the same
commit.
Fixes #1160
`js_cognitive!` attached the function-boundary rule to `FunctionDeclaration` alone, but `get_space_kind` opens a `SpaceKind::Function` for `method_definition` and `function_expression` too. Those spaces therefore scored their own control flow against the *enclosing* function's conditional nesting: a definition nested two `if`s deep in `outer` reported 3 where the byte-equivalent `function_declaration` reported 2. The arm is now `is_js_func!` minus `ArrowFunction` — `FunctionDeclaration | MethodDefinition` unconditionally plus `FunctionExpression` gated on `Self::is_func`, which re-derives `check_if_func!` rather than copying its kind list flat, so a positional (closure) function expression keeps falling through to `_`. `ArrowFunction` keeps its own lambda-channel arm. The `stops` list behind the function-depth surcharge was short by the same kinds and is a second, independently observable bug: a `function` declared inside a `method_definition` took no surcharge even through the working arm. It now carries the same three kinds. Affects `javascript`, `typescript`, `tsx` and `mozjs`, which all instantiate the macro; each gets its own test, since the macro body is shared but the `kind_id`s are not. Coverage is the two-conditional-deep reset (one level cannot discriminate — reset-plus-surcharge and neither both give 2), the `stops` surcharge on a definition nested in another function, an anonymous positional function expression that must stay a closure, and an arrow function that must keep the lambda channel. Each assertion was verified by perturbing the one production token it names: reverting the arm list, dropping either `stops` entry, dropping the `is_func` guard, and sweeping `ArrowFunction` in each fail exactly these four tests and nothing else. Snapshots move, contrary to the issue's expectation: 30 pdf.js files, 558 cognitive values, 544 up and 14 down. The increases are the surcharge now reaching functions declared inside pdf.js's IIFE module wrappers; the decreases are the inflation the issue reported. No structural change and no other metric family moves. Two adjacent mismatches are deliberately left alone and reported on the issue: the generator kinds, which `get_space_kind` calls functions and `is_js_closure!` calls closures, and the cross-language `nesting.lambda` reset gap (Rust, Java, C++, PHP), whose JS-side tail is that an `arrow_function` ancestor grants no depth surcharge at all. Fixes #1159
#1102 wired the ternary condition and both branch operands into the ABC unary-condition walker for the C family, the JS family, PHP and Perl. Ruby had only the `?` token arm, so `a ? !b : !c` scored 1 against the 4 that Java, C#, Groovy and all of the above report for the same expression, and `ruby_inspect_container`'s boolean-context seed carried no ternary parent at all. Add `Conditional` to that seed and add `ruby_walk_ternary` to route the three slots by grammar field. The condition slot reuses the existing `ruby_count_condition`, so a ternary predicate is classified by exactly the code that classifies an `if` / `unless` / `while` predicate. Both branch slots route through `ruby_inspect_container` rather than testing for the `Unary` kind: `-b` and `!b` are the same node kind, separated only by child(0). Keying on the kind takes the control case `(a > 0) ? b : -b` from 2 to 3, which the new test pins. The seed identifies the condition slot by grammar field rather than by the neighbouring `?` / `:` token as `cpp_inspect_container` does. The token form has two weaknesses: a comment between the token and the operand becomes the previous sibling and flips the seed on for a branch slot, and the test inverts on failure, so Ruby's second `:` id (COLON2, unreachable at 0.23.1) would turn every parenthesised alternative into a condition after a grammar bump. Both remain live in the C family.
Python counted the `conditional_expression` node but never its condition slot, so `a if c() else b` reported 1 where the equivalent `c() ? a : b` reports 2 everywhere else. `python_inspect_container`'s `ConditionalExpression` boolean-context seed was dead for the same reason: no call site ever passed that parent. Route the condition slot only. Python's branch operands are already counted by the top-level `NotOperator` / `ComparisonOperator` arms, a different mechanism from the C-family walkers, so a wholesale `cpp_walk_ternary` copy would double-count — `(b) if a else (c)` would score two operands that the identical unparenthesised form scores at zero. That case is pinned. tree-sitter-python's `conditional_expression` carries no grammar fields, so the slot is located by role: the first non-comment child after the `if` keyword. Both halves matter. Comments are tree-sitter extras and arrive as direct children, so they shift every later index and can also sit between the keyword and the condition; a positional `child(2)` or a bare next-child reads the wrong node. Each has a regression test. The reference case `(not b) if a else (not c)` now reports 4, matching `a ? !b : !c` in every other language. #1161's resolution plan predicted 3, having measured the condition slot against the pre-fix total. Tcl and iRules remain unrouted, deferred to their broader Phase 2B pass; the book's deviation table keeps them on the ternary row and now states that Bash's keyword-driven ABC makes its arithmetic ternary a non-applicable case rather than a gap. Fixes #1161
A `[check] exclude` glob is written against the project the `bca.toml` sits at the root of, but the merge folded it into the same list as the caller's `--check-exclude` globs, which are anchored to the working directory. The two roots coincide only when the caller happens to be standing in the project root, so `bca check sub/a.rs` run from `sub/` silently stopped matching and the offender failed the gate with exit 2. That undoes #1146's guarantee for exactly the callers it was made for: `[check] exclude` is the one exclude surface an explicitly-named path does not override, which is why the agent-feedback recipe, both shipped hooks, and this repo's own dev-tooling exemptions were steered at it. A per-edit hook does not control its own working directory. `WalkFilters::warn_exclude_overridden` reads the same anchor, so #1146's warning went silent under the same conditions — silent precisely when the override it announces happens. Both halves are fixed together. The union of CLI and manifest globs is now by effect rather than by list: each origin compiles into its own glob set so each keeps its own anchor. A `--check-exclude` glob stays relative to the shell it was typed in, which is why the negative case is pinned as well. With no manifest the working directory remains the only sensible root, so `--no-config` keeps today's behaviour. Anchoring reuses `baseline::lexical_normalize`, the same `.` / `..` folding the baseline path keys use. Without it `strip_prefix` leaves `..` in place, so `bca check ../kept.rs` from `skipme/` would reduce to `skipme/../kept.rs` and a `./skipme/**` glob would exempt a file that is not in `skipme/` — a false exemption, the worse direction of this bug. Fixes #1164
A comment inside a match *pattern* makes rustfmt emit the enclosing match verbatim: no warning, no diagnostic, and `cargo fmt --check` still exits 0. Everything in that match is outside the formatting gate, which is where #1086 shipped hanging-argument calls and over-length lines through a green `cargo fmt --all --check`. Mirror the snapshot-anchor gate. `check-rustfmt-bail.py` over-indents every match arm to a column rustfmt can never produce, pipes the file through `rustfmt --emit stdout`, and counts the arms that come back untouched. Per-file counts are baselined; increases fail, decreases are silent and ratchet with `--update`. Three things the ad-hoc probe in `.claude/rules/formatting.md` got wrong, all measured: - Feeding rustfmt a *path* makes it resolve `mod` declarations, so `src/getter.rs` and `src/metrics/cognitive.rs` error out rather than probe. On stdin there is nothing to resolve and no unprobeable class. - A bare arm regex matches the JS/C#/TS fixtures inside Rust string literals that these modules are full of. rustfmt never rewrites string contents, so those read as permanent bails: 37 of them in `src/metrics/cyclomatic.rs`, 15 in `nom.rs`, 8 in `wmc.rs`. The scan now skips string and comment spans. - Probing only the first arm misses match-scoped bails later in the file (11 false verdicts out of 18 in `src/getter/`). The sweep is workspace-wide rather than a directory list, since a directory nobody sweeps reads as clean — that is how `src/getter/` went unmentioned for two revisions of the rule. The baseline records 219 arms across 40 files: the 36 in-pattern comment sites from the issue, plus four whose cause is a `macro_rules!` body rustfmt cannot parse. The gate cannot tell the two apart, so the baseline names the four and the `--help` says so. Refs #1136
`Else /* else-if also */ =>` is the block-comment spelling of the in-pattern bail: rustfmt emits the enclosing match verbatim and `cargo fmt --check` still exits 0. Seven sites carried it — six per-language modules and the `js_cognitive!` macro body, which is the example `.claude/rules/formatting.md` leads with. Hoisting it above the arm re-enables formatting for all seven. The reflow is small (rustfmt packs three or-patterns onto fewer lines) and no metric moves. The comment gains room to say what `Else` actually is — the `else` keyword token, which the grammar also emits for the `else` of an `else if`. `src/metrics/cognitive/perl.rs` is untouched: its in-pattern comment sits between `|` alternatives of one long or-pattern, which is a different problem (see the baseline's note). Refs #1136
rustfmt has never formatted this match — its section comments sit inside the pattern — so nothing was checking line length here, and it drifted: one operand arm reached 252 characters, two operator arms 128 and 153. This is the exact damage #1136 is about, just already landed rather than newly introduced. Wrapped by hand at the same 100-column budget rustfmt would use. The arm still bails, so this stays a manual property; the baseline file now says so. Refs #1136
Step 3 of #1143. `cognitive = 25` was the pre-#1140 folklore value and, at a measured repo maximum of 20, gated nothing at all — the limit could not fire. 15 is both the shipped default and this repository's own 97.5th percentile, the same statistic #1140 derived the shipped default from, so the two agree by derivation rather than by coincidence. Measured at both tiers with `bca check --explain-threshold cognitive=15` rather than a bare `--threshold`, which is applied last and absolutely and so has no soft tier to report — the measurement that produced step 1's wrong answer: | tier | before | after | |------------------|--------|-------| | hard (15) | 18 new | 0 | | soft (14.25) | 34 new | 0 new | This is not step 1's cluster trap. There, `nargs` 7 -> 6 bought zero hard-tier offenders and 74 permanent soft-band entries because 6 was the population's ceiling. Here the population runs to 20 with 15 inside it, so the hard tier gains 18 genuine offenders. The 16 functions sitting at exactly 15 are the soft tier's unavoidable cost of any percentile-derived limit: a limit placed on a population value puts every function at that value inside the band by construction. Absorbed 34 functions as 12 fixed / 8 suppressed / 14 baselined. Of the 18 hard-tier offenders specifically: 10 fixed, 8 suppressed, 0 baselined. Genuinely simplified (12): - walk.rs: the visitor closure's 16 was almost entirely nesting artifact — `ignore`'s build_parallel API nests it two closures deep and cognitive charges a nesting increment per enclosing lambda, so four decisions scored 16. Extracted `visit_walk_entry`. - html_report.rs: split `write_table_head` off `write_table_core`. - html_report/sections.rs, markdown_report.rs: both `write_language_section` twins carried the empty-table rule twice with copy-pasted comments; select-then-emit leaves one copy. - exemptions.rs: `render_tty` / `render_markdown` each open three sections; per-section row renderers keep them at that altitude. - thresholds.rs: `evaluate_with_policy` mixed traversal with the per-threshold decision; added `ResolvedThreshold::violation_for` and a `SuppressionContext` for the three walk-constant fields. - vcs/git/diff_parse.rs: `scan_quoted_span` / `scan_unquoted_span`. - preproc.rs: `collapse_one_component` — the whole body was that one operation nested inside a `for` and an `if`. - metrics/npa/go.rs: `go_count_declaration_names`. - metrics/abc/php.rs: `php_count_unary_conditions` drove a cursor by hand, putting the skip case three levels deep; `children()` walks the same cursor over the same children. - baseline/body_hash.rs: `push_normalized_line`. Suppressed with a reason (8): the `<lang>_inspect_container` wrapper-peeling state machines (cpp, php, perl, ruby, elixir), `step_normal`'s existing marker extended from cyclomatic, and two grammar dispatch matches (npm/php `compute`, npa `ruby_walk_class_body`). Each carries the flag or node kind it must read at every step, so no split has a name a reader would draw. Baselined (14): functions sitting at exactly 15. A function at the limit is not over it, and a baseline entry keeps the growth alarm that a suppression marker would discard. Finding for #1140: 8 of the 18 hard-tier offenders and 13 of the 14 baselined are per-language grammar dispatch or byte-level scanners. This repository's cognitive tail is dominated by that shape, which is worth knowing when reading its distribution against the corpus. The refactors also retired 4 `halstead.effort` and 1 `nargs` baseline entry, so the baseline grew by 9 net. `nargs` 6 -> 5 is filed separately as #1183. Fixes #1143
The rustfmt-bail gate never lexed char literals, so a `'"'` or `b'"'` opened a bogus string span running to the next `"` anywhere later in the file and every match arm in between vanished from the probe -- src/vcs/git/diff_parse.rs was probing 4 of its 15 arms. That is the "reads as clean" failure the gate exists to catch, so it must not be the gate's own. No currently-bailing arm was hidden, so the baseline counts are unchanged; the blindness was to future bails. Sloc stored an end row plus its end column and re-derived "does the final row count" in span_rows, a fourth copy of the rule #1163 unified onto Node::end_line. It now stores the resolved end line, which deletes a field, three end_position destructurings and the duplicate rule. NArgs grew a params_owner hook. Java's compact-constructor support had copied the whole default compute to change one expression, re-stating the is_func_with_code-not-is_func rule and the closure fallback -- the drift #1142 and #1162 were both filed about. Also: restore the is_empty short-circuit WalkFilters::passes dropped (GlobSet builds its Candidate before its own empty check, which allocates on Windows, and every walked file reaches it); compute the manifest anchor inside the or_else that consumes it, so its current_dir syscall is skipped when the CLI set already matched; a CwdForm newtype so the two same-typed paths exempts() takes cannot be transposed silently; three doc comments that insertions had detached from their functions; and correct a comment claiming an unknown ancestor chain costs O(depth) when Node::parent makes it O(depth^2).
Its only caller outside the child test module is manifest_match_path, in the same file. Per AGENTS.md, widen visibility only for a re-export.
The CHANGELOG entry and the book both claimed #1164 anchored the walker's `exclude` at the manifest root. It anchors the explicit-path warning, not the directory walk, so a manifest glob still resolves against the walk root there -- correct only when the walk starts at the manifest. The behaviour predates this branch (#1189); the claim did not, and a wrong claim is worse than the gap it describes. `--print-effective-config` re-unions the manifest's check-exclude globs for display, because #1164 moved them into their own field. Stubbing either half to drop the manifest failed none of the CLI crate's 1,319 tests -- before this batch the reporter read an already-merged field, so the coverage was incidental and the wiring is now a live path with no guard. The new test fails against both stubs and nothing else. .claude/rules/formatting.md still named the `js_cognitive!` comment that e4a9ac4 hoisted, and still listed seven `src/metrics/cognitive/` modules the gate no longer reports. Its own thesis is that a stale list reads as clean, so the list is gone in favour of the gate that replaced it. Also: note that `NArgs::params_owner` and overriding `compute` are mutually exclusive, and give the C-family ternary seed weakness the issue number it was filed under (#1181) plus a reproducer.
The test added for the --print-effective-config manifest wiring matched its three expected values with `contains` over the whole serialized document, so it could not tell which field each landed in. Rendering the manifest's `[check] exclude` into the walker's `exclude` key and dropping `check_exclude_from` entirely -- a gate exemption reported as a walker exclude, which is a wrong answer to the question the flag exists to answer -- failed 0 of the check suite's 213 tests. Parsing stdout as a toml::Table and reading each value out of the field it belongs in makes that perturbation the only failure, and pins that the walker's own exclude list stays empty.
CheckExcludes::exempts lacked the is_empty() short-circuits that WalkFilters::passes documents as load-bearing, and resolved the manifest-anchored path -- a current_dir() call plus a normalising allocation -- once per violation even with no manifest glob configured. Two .bca-baseline.toml entries recorded values above the current measurement, because the baseline was regenerated partway through the branch and later refactors shrank those files. A baseline above the live value is masked headroom, so both are refreshed. check-rustfmt-bail.py called Path.relative_to unguarded, so naming a file outside the repository aborted the gate with a ValueError traceback instead of a probe result.
Closes the hoisting half of #1136 as a decision rather than as work. Every remaining cause-1 site is a section comment between `|` alternatives of a classifier table, so hoisting means splitting the arm into identical-bodied arms that clippy::match_same_arms correctly rejects -- 28 errors across src/getter/, and src/getter/python.rs also crosses its halstead.effort limit when split. `#[rustfmt::skip]` was the leading alternative and does not work: probed against the gate, a bailing match reports the same count with and without it, because the gate measures what rustfmt declined to reformat and the attribute declines it too. It also cannot sit on the `match` -- attributes on expressions are unstable (E0658) -- so it goes on the enclosing fn and removes that whole function from cargo fmt, which is less coverage than the bail it would document. The filed problem was that these matches sat outside the formatting gate and nobody knew which ones. The gate fixes that; the remainder is cosmetic and every remedy makes a lookup table harder to read. The reasoning lives in the baseline header and the rule file, where someone about to start the work will meet it. Fixes #1136
The #1130 entry said `bca functions` and `bca find --type function` remained blind to Elixir, tracked as FIXME(#1162). Both were fixed in this same unreleased section, so the two entries contradicted each other in one release, and the FIXME it points at no longer exists in the tree. Its parity-test path was also stale: the file moved under tests/parity/ with the rest of the family.
Entry 88: a tool that reads source as text to produce a number must model the language's lexical structure first. The remedy for a wrong claim is normally to run the measurement (lesson 84), and that does not help when the instrument is wrong -- re-running reproduces the same confident answer, which is exactly what a careful person does before quoting it. The #1136 probe matched `=>` over raw Rust, so JS and C# fixtures inside string literals registered as match arms (nom.rs: 16 raw, 0 span-filtered, no bailing arms at all); adding string and comment spans then left char literals unlexed, so a b'"' hid every arm after it. The sibling snapshot-anchor gate has the identical gap, filed as #1192. Lesson 59 gains the instruction to enumerate sites by the quantity they compute rather than by the shape of the bug, with #1163 as evidence: function.rs arrived at the same wrong end row via a blanket +1 with no kind branch, so it matched neither the buggy pattern nor a SpaceKind search, and fixing only the two reported sites would have introduced a bca functions vs bca metrics divergence. Lesson 83 gains one clause for #1163 -- the same categorical-proxy mechanism it already documents, one file over.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1193 +/- ##
==========================================
+ Coverage 98.11% 98.17% +0.05%
==========================================
Files 276 276
Lines 70447 70896 +449
Branches 70017 70466 +449
==========================================
+ Hits 69120 69600 +480
+ Misses 907 875 -32
- Partials 420 421 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The effective-config test compared production's resolved check_exclude_from against dir.path() as TempDir spelled it. Manifest discovery reports the *resolved* directory, and on macOS a TempDir sits under /var/folders/..., a symlink into /private/var/folders/..., so the assertion passed on Linux and Windows and failed only on the macOS leg: left: /private/var/folders/.../more-globs.txt right: /var/folders/.../more-globs.txt Canonicalising only the expectation would have traded one platform for another -- on Windows canonicalize returns a \\?\ UNC path production does not emit. Canonicalising the root once and using it for the fixture files, the cwd and the seed keeps both sides in the same form on every platform, which is what the sibling discovery tests in exclude_path_form.rs already do. Re-verified the test still fails when check_exclude_from is dropped.
The previous attempt canonicalised the fixture root and used it throughout, on the reasoning that keeping both sides in one form would agree everywhere. It did not: production emits a plain `C:\Users\...` while `canonicalize` yields `\\?\C:\Users\...`, so the macOS fix broke Windows instead. That reasoning leaned on the sibling tests in exclude_path_form.rs, which canonicalise and pass on Windows -- but they compare file-name sets from the walk and never assert an absolute path, so they were never evidence for asserting one. Neither spelling is production's to control: macOS resolves a TempDir's /var/folders symlink into /private/var/folders, Windows adds a UNC prefix only on the canonicalising side. Normalising one side fixes one platform and breaks the other. Canonicalising both compares the file the two paths actually name, which is the property under test. Verified locally by reproducing the macOS shape with a symlinked fixture directory: production reports the resolved path, the naive expectation is the symlink spelling, raw comparison fails and canonical comparison holds. Re-verified the test still fails when check_exclude_from is dropped.
Owner
Author
Code review — full branchVerified independently against the branch: Every finding below has a reproducer I ran. Bugs1. The new sentence says a The book ( 2. The first direction fails loud and is tolerable. The second is the dangerous one: any existing config that happened to spell a glob manifest-relative — previously a silent no-op nobody would have noticed — now silently exempts real offenders. The CHANGELOG describes the new rule but carries no migration warning for the Correctness — incomplete fix (pre-existing, not a regression)3. #1159 leaves JS generator functions outside the boundary set and outside
// boundary half
function outer (a,b) { if(a){ if(b){ function inner(c){ if(c){return 1;} } }}} // inner → 2 ✅
function outer2(a,b) { if(a){ if(b){ function* gen (c){ if(c){return 1;} } }}} // gen → 3 ❌
// stops half
function* wrapper () { function inner (c){ if(c){return 1;} } } // inner → 1 ❌
function wrapper2() { function inner2(c){ if(c){return 1;} } } // inner2 → 2 ✅The arm's new comment defends the ungated Gate quality (#1136)4. A third bail cause the gate documents nowhere: a match arm exceeding rustfmt's A match containing a >100-column arm makes rustfmt emit the whole match verbatim — verified by planting a deliberately mis-indented sibling arm and watching it survive 5. Change one word in the checked-in header, run 6. Gate hardcodes
Performance7.
Design8.
Judgement call, not a defect#1143's The counter-argument in Working-tree state — needs attention before pushing
Notes, no action
VerdictAPPROVE WITH COMMENTS. Findings 1 and 2 are worth landing before merge — 1 because the man pages ship a claim the code does not honour, 2 because it can silently un-gate an existing project. Nothing blocks the shipped metric or CLI code.
The engineering standard here is high: the parity tests are genuinely discriminating rather than decorative, the anchoring split in #1164 is the right model, and the #1136 gate is fast, self-tested, and honest about what it cannot distinguish. |
This was referenced Aug 3, 2026
--exclude's help text claimed a bca.toml glob "is resolved against the directory holding that bca.toml". The walker matches manifest globs against the walk-root form; only the explicit-path warning and the [check] exclude gate anchor at the manifest. Reproduced: with exclude = ["./sub/vendor/**"], `bca functions -p sub` still reports sub/vendor/v.rs. The claim shipped to `bca <cmd> --help` and to twelve committed man pages, which are regenerated here. The [check] exclude anchor change is a silent behaviour change for any manifest whose paths is not ["."], and the CHANGELOG described the new rule without a migration note. Both directions verified against a main binary: a walk-root-relative glob stops exempting (loud), and a manifest-relative one that previously matched nothing now exempts real offenders (silent, and the dangerous one). The rustfmt-bail gate documented two causes and has three: an arm wider than rustfmt's max_width bails the whole match with no comment and no macro involved -- measured, a 128-column arm bails where the same match with a short variant name does not. Its regression message named only the in-pattern comment, which is the misdirection the gate exists to prevent. read_notes now splits on a sentinel rather than a positional header prefix, so a drifted header no longer duplicates; the migration to the sentinel hit that bug and left a stale copy, removed here. The edition is resolved per crate, since five vendored grammar crates are 2021 while the workspace is 2024 and a 2024 keyword in one of them would read as a broken repo. CheckExcludes resolves the working directory once instead of per violation. It cannot be cached process-wide -- walk.rs's directory guard moves the cwd for `bca diff` -- so it is carried on a ManifestAnchor, one struct rather than two same-typed paths. Also: record on the js_cognitive! arm that generators are excluded from both the boundary set and stops (#1186), and state #1143's baseline ledger in the CHANGELOG as the cost it is rather than only as a benefit.
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.
Fixes eight issues on one integration branch, then puts the result
through the four quality skills and two code-review passes.
Three fixes move metric values (#1159, #1160, #1161) — each says so
in its CHANGELOG entry, and the integration snapshots are refreshed and
pushed to the submodule remote with the pointer recorded in the same
parent commit.
The fixes
end_lineis keyed on the node's end column rather than itsSpaceKind. A Perl function that is the last item in a file reported a span past its parent unit and past EOF.bca functions,bca find --type functionandbca count --type functionread the source bytes, so Elixir'sdef/defpare no longer invisible.method_definitionand qualifyingfunction_expression, notfunction_declarationalone; the depthstopslist carries the same kinds.[check] excludeglobs from abca.tomlresolve against the manifest directory, so an exemption written for the project root holds whenbca check <file>runs from a subdirectory.make rustfmt-bailgate: blocks new match arms rustfmt silently refuses to format. The hoisting half is closed as a recorded decision — see below.cognitiveconverges 25 → 15, the shipped default.Things worth a reviewer's attention
Several of these issues' own plans were wrong, and the fixes correct
them. #1160's plan asserted a record can declare both a compact and a
canonical constructor (JLS 8.10.4 makes that a compile error). #1161's
plan stated a mandatory acceptance criterion —
(not b) if a else (not c)"stays 3" — that is arithmetically wrong and would have locked inthe cross-language inconsistency the issue was filed to remove. #1159's
"expect no snapshot churn" was wrong in both existence and direction:
558 corpus values moved, 544 of them up. #1163 named two producers of
the end-row rule; there were three.
#1136 is a gate plus a decision, not a cleanup. Every remaining
bailing site is a section comment between
|alternatives of aclassifier table, so hoisting means splitting into identical-bodied arms
that
clippy::match_same_armscorrectly rejects (28 errors acrosssrc/getter/).#[rustfmt::skip]— the leading alternative — wasprobed against the gate and changes nothing it measures, and can only
sit on the enclosing
fn, removing more fromcargo fmtthan the baildoes. Reasoning is recorded in
.rustfmt-bail-baseline.txt's header and.claude/rules/formatting.md.#1143 converged
cognitivebut notnargs. The old limit of 25 wasinert — the measured maximum here is 20. Re-deriving #1140's statistic
against this tree gives p97.5 = 15 exactly. Of the 18 offenders, 10 were
simplified and 8 suppressed with a reason; the 14 baseline entries added
all sit at 15, where a baseline keeps the growth alarm a suppression
marker would discard.
nargs6 → 5 is tracked separately as #1183.Verification
make pre-commit→BCA_GATE: pass.snap.new.against a pristine pre-batch baseline. Eight files show fewer covered
lines; all eight simply shrank — uncovered counts held or fell
(
abc/php.rsimproved 7 → 3).big-code-analysis-outputbumped tod71252eb, confirmedon its
origin/main.Quality passes
simplify-rust(three read-only dimension agents),rust-optimize,review(two agents),audit-tests, and two code-review passes.Findings remediated across four commits. The sharpest were against work
done in this branch:
b'"'opened aspan to the next
"and hid every arm after it —src/vcs/git/diff_parse.rswas probing 4 of its 15 arms. A gate builtto catch regions that read as clean had one in its own implementation.
--print-effective-configwiringsubstring-matched structured output, so rendering a gate exemption
under the walker's key failed 0 of 213 tests. Now parses the TOML.
Slocwas a fourth producer of the end-row rule fix(spaces): Perl child function end_line exceeds its parent Unit end_line #1163 unified.by perturbation in both directions.
Follow-ups filed
#1180–#1192. Three had their premise verified before filing rather than
after: #1189 (a manifest exclude leaking under a directory seed) was
confirmed pre-existing by building a
mainbinary and diffingbehaviour; #1191 (a racing GIL test) reproduces only under load and this
branch does not touch
big-code-analysis-py/; #1192 (the snapshot-anchorgate has the same char-literal blind spot as #1136's did) is latent
today only because no
src/metrics/file spells the trigger.Also adds lesson 88 and merges #1163 into lessons 59 and 83.
Fixes #1136
Fixes #1143
Fixes #1159
Fixes #1160
Fixes #1161
Fixes #1162
Fixes #1163
Fixes #1164