fix: batch resolution of #1165-#1174 (9 issues) - #1176
Merged
Conversation
A limit written with the bare `diff --metric` alias (`sloc`, `ploc`, `lloc`, `cloc`, `blank`) added a second, independent threshold for the same metric instead of overriding the dotted spelling. The alias was resolved in `ThresholdSet::build_tiered`, after every merge map was already keyed by whatever string the user typed, so the manifest, `--config`, `[thresholds.lang.<slug>]`, and `--threshold` layers merged by spelling rather than by metric. One `(function, metric)` pair could then produce two offender lines, and `--print-effective-config` printed one of the two limits while the other fired. Canonicalise where the maps are built instead. All four sites move together — canonicalising only the manifest keys would stop a manifest `ploc` merging with a CLI `--threshold ploc=...`, trading one bug for another: - `split_thresholds_table`, for the hard layer and the extracted `parse_soft_table`; - `threshold_lang::parse_one_language_table`; - the `--threshold` layer, canonicalised at its consumption site rather than inside `parse_cli_threshold`. That function is the clap `value_parser`, and its sibling semantic check (unknown metric, with the did-you-mean list) lives one layer down; splitting them would give two adjacent typo classes two different error surfaces. `build_tiered`'s canonicalisation is removed rather than left as a no-op: the invariant now holds at the boundary, and two canonicalisation points is how this survived. Every downstream consumer — `resolve_tier`, `build_tiered`, `--print-effective-config` — may assume canonical keys, which is also what makes #1166's direction lookup correct by construction rather than by coincidence. A single table naming one metric under both spellings is now an error. Silently keeping whichever key sorts last is the same surprise in a quieter form. Fixes #1165
`--tier=soft=RATIO` multiplied every configured limit by the ratio. For the lower-is-worse `mi.*` family a limit is a *floor*, so multiplying lowered it: `[thresholds] "mi.original" = 20` with `--tier=soft=0.5` resolved to a soft floor of 10, below the hard floor of 20. No value could reach the early-warning band before the hard gate, so `--tier=soft` — and `--headroom`, and therefore `make self-scan-headroom` — was a silent no-op for the whole family. `scale_threshold` now takes the direction and picks the arithmetic: multiply a ceiling, divide a floor. Making the shared helper direction-aware rather than patching `resolve_tier`'s blanket-ratio loop covers the second call site for free — a `"0.9x"` string in `[thresholds.soft]` resolves through `SoftLimit::Scale` into the same helper and was inverted the same way, with nothing testing it. The direction comes from the existing `metric_is_lower_is_worse` wrapper over the library catalog, widened to `pub(crate)`. It keys on the metric name, which #1165 made canonical at every parse boundary — that is what makes this lookup correct by construction rather than by coincidence. Two decisions worth stating: - Rounding. `20 / 0.9` repeats, so the last of the six significant figures is a real choice, and the floor direction rounds *up* (22.2223, not 22.2222). A floor rounded down is a band that fires marginally late — the same defect in miniature — and rounding up also keeps the resolved floor at or above the exact quotient, so it can never land under the hard floor and trip the guard below. The ceiling direction keeps round-to-nearest: `limit * ratio` has an exact decimal the float product misses by an ulp, so nearest-rounding recovers it, and that output parity is a contract from #373. - Zero, negative, non-finite, and above-one ratios were already rejected at every entry point — `TierSpec::from_str`, `--headroom`, the `[check] headroom` manifest key, and `parse_scale_str` all route through `is_valid_scale_ratio`. Verified rather than assumed; nothing new was needed, so the two directions cannot fail differently. With the scaling corrected, #1141's `!lower_is_worse` exclusion on the soft-looser-than-hard guard is gone: an `mi.*` soft floor below its hard floor is now the usage error it always was, and the comment deferring that removal to this issue goes with it. Fixes #1166
`bca check` wrote its offender rows to stderr, so `| wc -l`, `| head`, `| rg -c` and `2>/dev/null` all reported an empty offender list — a plausible "this tree is clean" rather than an error. The split was not even internally consistent: `--report-format sarif` already wrote the same information to stdout. The rows are the command's product and now go to stdout. Everything that is commentary about the run stays on stderr: the summary footer, the remediation block, GHA annotations, and the `bca:` / `warning:` / `error:` diagnostics. One structural exception — `--report-format` without `--output` gives the aggregated document stdout, so the rows fall back to stderr rather than corrupting a SARIF payload; adding `--output <file>` puts them back. Exit codes, the `--summary-file` digest, and the aggregated document are untouched, and each is pinned by a test. Moving the rows onto the stdout path also brings them under #1132's write policy: `BrokenPipe` stays routine (`| head` still reports the gate verdict), any other write failure is a tool error rather than a gate verdict whose evidence never reached the consumer. Migration: a pipeline reading the rows through `2>&1` needs no change; one that captured them with `2>file` should now use `>file`. Fixes #1167
A `bca: suppress(...)` marker with anything after the closing paren was rejected as malformed, so the suppression silently did nothing — and a trailing rationale is exactly what AGENTS.md tells contributors to write. The warning fired, but into a stream of routine gate output, so the author kept a marker that looked right and was inert. Anything after the metric list is now the rationale, with no separator required. After a bare verb one is required (`-`, `:`, `//`, `#`, an em/en dash), since without a list there is nothing to distinguish a reason from prose about the marker. Fix the shared class too: an unusable metric name is skipped and reported instead of voiding the whole marker, so `suppress(cognitive, exit)` still silences `cognitive` while complaining about `exit` — the `nexits` typo AGENTS.md itself documents people making. Skipping can only narrow what a marker covers, so the widening hazard the old rule guarded against is unreachable. This reverses the contract #896 pinned; its integration test now pins the new one, plus a companion proving an all-unusable list still suppresses nothing. `parse_marker` returns a `MarkerScan` (directive plus diagnostics) rather than a `Result`, because a marker can now be partly usable. The type is crate-internal; no published shape changed. Fixes #1168
`bca check --explain-threshold <metric>=<limit>` reports what a candidate limit would cost — hard-tier offenders, the resolved soft limit and its offenders, and how many of each already match a baseline entry — without editing `bca.toml` or running a gate. Repeatable, one candidate per metric. `--threshold` cannot answer the question by design: its limits are applied last and absolutely, never scaled, so a candidate trialled that way has no soft tier and reads as free when it is not. `nargs 7 -> 6` was approved on that reading and would have bought 74 permanent baseline entries, because 74 functions sit at exactly 6 and a 0.95 soft band starts at 5.7. Named `--explain-threshold` over `--dry-run-threshold`: it shares `--threshold`'s `<metric>=<limit>` grammar and value parser, and the verb-first form matches the other flags that replace the gate with a report (`--print-effective-config`, `--write-baseline`). "Dry run" would imply the gate ran and its effects were withheld; nothing here runs a gate. One walk, not one per candidate: the gate is resolved at the soft tier, whose band is the more permissive of the two, so the collected violations already contain every hard-tier offender and each carries both its language's soft limit and its candidate ceiling. Everything downstream is the gate's own code in the gate's own order, which is what makes the counts equal a real run under `exclude_tests`, `[check] exclude`, suppression markers, per-language overrides and the baseline. A cluster is reported when one value holds at least half a soft band of ten or more offenders — the shape that makes a candidate unclearable, since the soft tier measures distance to the limit. Also writes the rule down, in the book's threshold and baseline recipes and in AGENTS.md's baseline-refresh discipline, and corrects `bca.toml`'s own `nargs` note, which repeated the issue's claim that nothing sits above 6 (60 functions do). Fixes #1169
`.bca-baseline.toml` is generated wholesale, so a textual merge of two branches that both touched it produces plausible-looking hunks that are wrong on both sides — neither branch's measurement describes the merged tree. Mark it `-merge` so git leaves it conflicted as a whole and the only available resolution is to regenerate. `-merge` is preferred over a `merge=ours` driver because it needs no per-clone `git config`, which nothing in a checkout enforces.
The identity re-key this issue asks for already shipped in #377: matching keys on `(path, qualified, metric)` and a lone record under a key matches regardless of line. What remained was that the writer still emitted `start_line` for every entry, so the field re-rendered on every unrelated edit above a baselined function — churning the diff, hiding real value changes in review, and giving two branches 174 lines of purely positional conflict. Schema v6 writes `start_line` only for an entry whose identity is shared with another, the sole case `match_in_group` consults it. Entry order drops its line-number tiebreak for the same reason. v2-v5 files read unchanged; a recorded line is still honoured exactly as before, and a v6 file handed to a pre-v6 bca now reports the version mismatch by name rather than a bare serde field error. Measured on this repo: all 175 baseline entries have a unique `(path, qualified, metric)`, so no disambiguator was needed anywhere and the regenerated file carries no line numbers at all. Classifying the full 1111-offender population at the hard, soft-0.95 and soft-0.5 tiers against a with-lines and a without-lines baseline written by the same binary over the same tree gives byte-identical offender rows and identical exit codes. Fixes #1170
A fresh `git worktree` needs two setup steps that nothing stated and nothing detected, and one of them could not be repaired by repetition. `make worktree-setup` now runs both, and the tests that depend on the first name their own cause. Measured, not assumed: without the integration corpora exactly 24 tests fail — 5 corpus tests and 19 CLI tests that analyse a real DeepSpeech source file. The issue's "roughly 24-48" is right at its lower bound; the upper half is not reproducible on the current tree. Also confirmed, twice (a synthetic superproject and a real linked worktree carrying the serde submodule): after an interrupted checkout a plain `git submodule update --init` exits 0, prints nothing, and restores nothing, because the submodule HEAD already matches the recorded SHA. Only `--force` repairs it. Design choices worth stating: * The escalation is conditional, not unconditional. `--force` is `git checkout --force` inside the submodule, and contributors legitimately edit `big-code-analysis-output` while accepting snapshots. `utils/worktree-setup.py` classifies each submodule and forces only a pure-deletion diff; a submodule that also carries modifications is refused with the command to run once they are safe. That classifier gets self-tests and a pre-commit hook, since it is what decides to run a destructive command. * The checkout is non-recursive. DeepSpeech's own submodules (tensorflow at 246 MB, kenlm, doc/examples) are excluded by the corpus test's glob list, so no test reads them — and fetching them is most of the wall time that gets interrupted in the first place. `make bench-*` does walk them and keeps its recursive checkout. * The guard lives at the read sites, not in one guard test. A guard test passes or fails once while the other 23 tests still fail confusingly. The corpus driver's zero-file case is now split from its count-drift case (they were one conflated message since #938), and the three duplicated `fixture_path()` helpers in the CLI suite now share one helper that checks the corpus first. * `worktree-setup` is deliberately not wired into `pre-commit` or `ci`. It is a developer bootstrap, and a ~100 ms no-op once the tree is set up. The Python half folds in `make py-bootstrap` rather than competing with it, and soft-skips when uv is absent instead of hard-failing. Fixes #1171
`make pre-commit` printed `Pre-commit checks passed` on success and
nothing definitive on failure, so a consumer had to infer an outcome
from GNU make's `Error N` lines. Under `-j` those are not a verdict:
with `_pc-gate-status-test` broken, `make[2]: *** [...] Error 2` landed
at line 58 of a 236-line log and 174 lines of *successful* output
followed it -- clippy, mypy, pyright and 49 self-tests all reporting
green after the run was already lost.
Both gates now run under `utils/gate-status.sh`, which forwards stdout
and stderr unchanged on their own descriptors and then prints exactly
one `BCA_GATE: pass (gate=pre-commit)` or `BCA_GATE: fail
(gate=pre-commit, exit=2, stage=_pc-fmt)` before exiting with the
gate's own status.
Design choices worth stating:
* The failure path is the whole difficulty. A trailing `@echo` only
ever reaches the success path, which would turn "no line" into the
sole failure signal -- the ambiguity being removed. The wrapper
therefore sits outside the make invocation and reads its status from
`${PIPESTATUS[0]}`; `$?` is not a substitute even under pipefail,
which would report a `tee` failure as a gate failure. It never
converts a red run into a green exit, and `gate-status-test.sh`
perturbs exactly that: a wrapper returning 0 fails two assertions,
dropping both pipefail and PIPESTATUS fails four.
* `stage=` is a list, not one name. `-j` stops *scheduling* on the
first failure but lets running stages finish, and they can fail too
(`*** Waiting for unfinished jobs....`); naming only the first would
hide the rest.
* Three states, not two. No `BCA_GATE:` line at all means the run never
finished -- crash, kill, or interrupt -- and must not read as either
outcome.
* The contract is anchored, `^BCA_GATE:`. The self-test quotes both
spellings in its own failure messages, and prefixing them is what
stops a failing `_pc-gate-status-test` planting a second verdict in
the gate's log.
* `_pc-all` / `_ci-all` exist so the wrapper has a single command to
wrap -- `ci` runs two sequential sub-makes and one verdict per gate
is the point. The DAG itself is untouched.
* The prose lines are replaced, not supplemented, in both gates: one
terminal line, nothing to drift, nothing to disagree about which is
last.
Corrections to the issue:
* "There is no single unambiguous terminal line" is not quite right.
`make: *** [Makefile:NNNN: pre-commit] Error 2` is always present and
always last on the failure path. The defect is narrower: it shares no
token with the success line, a substring search cannot tell it from
the mid-run `make[N]:` reports, and it is absent from an interrupted
run, so its absence means either success or never-finished. The
conclusion stands; the stated reason does not.
* `make fmt-check` itself runs `markdown-lint`, `sh-fmt-check` and
`toml-fmt-check`, so a lint-file violation fails `_pc-fmt` -- the DAG
root -- and aborts before fan-out. Breaking a lint file is not a way
to exercise the parallel path.
On failure make still appends its own `*** [... pre-commit] Error 2`
after the verdict. Suppressing it would mean exiting 0, which is a far
worse defect than the one being fixed.
Fixes #1172
Four pieces of operational knowledge from the #1090-#1151 batch, in the three places that would have prevented each one. `tests/README.md` gains a "Moving a test file" section. insta keys the snapshot file name on the whole of `module_path!()`, so relocating a test renames every snapshot it owns, and the failure names the snapshot rather than the move. Verified by removing `output_formats__csv_test__csv_cpp_widget.snap` and running its test: insta reports `snapshot assertion for 'csv_cpp_widget' failed`, prints the whole value as new, and writes a `.snap.new` under the correct new name — indistinguishable from a real behaviour change. The batch-fix skill gains three orchestration rules: worktree agents fork from the base rather than the integration tip and must merge it back (which also needs `<INTEGRATION_BRANCH>` added to the prompt's substitution list, or the remedy is unrunnable); Step 4b merges only against a clean integration worktree; and each agent needs its own log path, extending rather than repeating the capture idiom #1172 added to Step 6a. `.claude/rules/tool-output.md` is new: a `Preview (first 2KB)` fragment is a prefix, so `sort | uniq -c`, an `rg` sweep, and a test run's trailing summary all put the rows that matter past the cut. A separate file rather than a section in `shell.md`, whose scope is zsh semantics — this hazard is harness-level and applies to Read and Grep results with no shell involved. Linked from AGENTS.md's Tool choice list, following shell.md's precedent, since an unlinked rule is an unread one. Corrections to the issue and to what was already written down: * `tests/output_formats/main.rs` carried a module doc asserting the opposite of the fact being documented — that insta keys the prefix on the *last* component of `module_path!()` and the names were therefore unchanged. The rename diff in 11daa14 shows `csv_test__csv_cpp_widget.snap` becoming `output_formats__csv_test__csv_cpp_widget.snap`. Corrected in place. * The skill contradicted itself on C2. Step 4a claimed agents "fork from `INTEGRATION_BRANCH` at the moment they are spawned"; Step 4a's branch mode note, forty lines later, correctly said they "fork independently from `main`". The second is right. * C3's premise that a mid-edit merge "is refused by git" holds only on overlap. Measured: with `other` dirty and the merge touching only `f`, `git merge` fast-forwards and leaves ` M other` behind. Written up as a backstop, not a safeguard. * The issue asks to put the insta note "beside the snapshot-layout section"; `tests/README.md` has no such section. It also had a stale "Layout at a glance" tree showing the pre-#1124 flat file list, which the new section would have contradicted two screens up. Refreshed, along with three category headings still naming `tests/*_test.rs` paths. Item §5 needed no work, as the issue says: `.claude/rules/shell.md` is present from da267f0 and covers all three claims made for it — zsh not field-splitting parameter expansions while command substitution is split, `$?` after a pipeline reporting the last stage, and `PIPESTATUS` being bash-only. Fixes #1174
Record the nine issue fixes merged onto this branch under [Unreleased]: the two additions (--explain-threshold, worktree-setup), five changed behaviours (offender stream, marker skip semantics, baseline schema v6 and its .gitattributes entry, the BCA_GATE verdict line, corpus diagnostics), and three fixes (threshold alias canonicalisation, direction-aware soft scaling, marker rationale). #1174 is documentation-only and has no entry.
`baseline.rs` sat at loc.ploc 524 against this repo's 550 file limit —
inside the 0.95 soft band `make self-scan-headroom` gates — and mixed
three unrelated concerns that the file was already ordered by.
Move them out verbatim into a module directory:
baseline/path_key.rs anchor resolution, lexical `.`/`..` folding,
and the percent-encoding pipeline that turns
an OsStr into a TOML-safe identity key
baseline/body_hash.rs the normalised-body FNV digest behind
`--baseline-fuzzy-match`, its hex codec, and
`bare_name`
`baseline.rs` keeps the file format, matching, and rendering, and
re-exports `anchor_for` / `hash_body` / `bare_name` so no caller
outside the module changes. Each module takes the tests that cover it,
in its own `*_tests.rs` sidecar; only `use` paths and visibility moved.
No behaviour change: baselines written before and after the split are
byte-identical, a pre-split baseline reads back to the same
classification, and `bca diff-baseline` / `bca exemptions` output is
unchanged. The full offender set is identical apart from the dropped
`<file>` loc.ploc offender and an 8-line shift in two spans.
loc.ploc: baseline.rs 524 -> 366, path_key.rs 88, body_hash.rs 81.
Quality-only follow-up to the #1165-#1174 batch. No output, exit code, metric value, or public API changes; verified by running the pre- and post-change `bca` binaries over the same tree and diffing both streams of `check` and `check --explain-threshold nargs=6`. - The `--explain-threshold` preview no longer re-lists the gate's post-walk stages. Both now run `collect_check_violations` (scope, walk, empty-input guard, `[check.exclude]`) and `classify_check_violations` (baseline filter, `--changed-only`), so a stage added to the gate reaches the preview instead of silently predicting a different run. The second helper also owns the three `CheckArgs` defaults the two paths used to spell out separately. `--write-baseline`'s early return still sits between them, unchanged. - `LanguageOverride` and `SoftDerivation` replace a positional `(slug, hard, soft)` tuple and a two-valued `String`: the two limits were same-typed and swappable, and the derivation's branch sat inside a struct literal. - `BaselineIdentity` / `cmp_identity` state the `(path, qualified, metric, start_line)` order once. It was written four times across three modules, twice with byte-identical bodies. - `MarkerScan::partial` gives `parse_native`'s fourth exit a name like its other three. - `explain.rs` items are reordered so each impl follows its type. Pure motion. Two tests cover previously-unreached lines in `explain.rs`: the `--tier=soft=<R>` ratio path and the `[thresholds.soft]` derivation branch. Each was verified as the sole failure under a perturbation of the line it names. `.bca-baseline.toml` moves two entries. `run_check` drops 23% of its halstead.effort from the extraction above. `BaselineDiff::compute` gains 1.6% purely as an artifact: collapsing two identical comparators into one leaves N2 unchanged while n2 falls 54 to 53, and difficulty is (n1/2)*(N2/n2), so removing the duplicate name raises the score.
`run_explain_thresholds` re-derived the soft-scale ratio with a `match` byte-identical to `TierSpec::ratio`, which `EffectiveConfig` already calls for the same value. Call the accessor. No output, exit code, or public API change: the pre- and post-change binaries were run over this tree for `check` and for `--explain-threshold` at all three `TierSpec` shapes (`hard`, `soft`, `soft=0.90`, which resolve to distinct soft limits 5.7 / 5.7 / 5.4), and stdout, stderr and exit code matched on every one.
Review-only follow-up to the #1165-#1174 batch. Every new test was verified by perturbing the exact production line it names and confirming it was the only failure. Correctness - `--explain-threshold` reported `0.95x` for a metric that never had the ratio applied. `resolve_tier`'s soft branch is all-or-nothing per table, so one `[thresholds.soft]` entry makes every *other* explained metric inherit its hard limit unscaled. Adds the third derivation and renders it; the counts were already right and are unchanged. - An ambiguous `--explain-threshold halstead=5` blamed `--threshold`, a flag the invocation never passed. `canonical_cli_thresholds` now takes the label from its caller. - One suppression marker could emit unbounded stderr: no dedup, no cap, and `UnknownMetric`'s `Display` rebuilt and sorted the metric hint per diagnostic. Now deduplicated, capped at `MAX_MARKER_DIAGNOSTICS` with a counted tail, and the hint is built once in a `OnceLock`. - `opens_rationale` accepted a bare `/`, so `bca: suppress /some/path` silenced every metric on the enclosing function. It now requires the documented `//`, and `MalformedBody`'s hint names the real opener set. - `--explain-threshold --summary-file <path>` left the file untouched while the identical `--output` was a usage error. Rejected after parsing, so the `auto` / `$GITHUB_STEP_SUMMARY` path still works. - `verify_repo_root`'s `CalledProcessError` arm was dead — `git()` runs with `check=False` — so a non-checkout exited through the generic message and a missing git binary got "is not a git checkout". Each case now gets its own diagnosis. Documentation - The #1170 changelog entry claimed the version-mismatch message helps a *pre-v6* binary (it ships here, so it helps this build read a newer file) and that entry order dropped its line tiebreak (`start_line` moved from third to last, still deciding within an identity group). Both rewritten to match STABILITY.md and the regression tests, and the two omitted shape changes — the exemptions JSON `line` key and the markdown `-` — recorded in both documents. - "always exits 0" contradicted a shipped `.code(1)` assertion; now "always exits 0 on success (1 on a tool error ...)" in the clap doc, `man/`, and the changelog. Tests - The #1168 rationale reproducer asserted only absences, so it passed against a scope-widening `SuppressionScope::All`. Adds a `--no-suppress` positive control and a second gated metric. - The four v6 "no recorded line" rendering paths had no lineless fixture anywhere; all four are now covered. - `--explain-threshold`'s `conflicts_with_all` list and its two metric rejection classes were untested. - `worktree-setup`'s `update()` and `main()` were never executed — the tests spelled git's argv by hand. They now call the real functions.
Every finding was verified by perturbing the exact production line the test names and re-running the whole suite. The counts below are observed failures, not estimates, and no production code changed. check_stream_contract / walk_channel_completeness: both read stdout through a `(limit `-shaped filter, so a line the filter skips is absent from the subset rather than a failure. Appending one commentary line to the stdout writer -- the "and nothing else" half of #1167's contract -- failed 0 of 5053 tests. Both now count unfiltered lines too. gate-status-test.sh: no case made `tee` fail, so substituting `$?` for `${PIPESTATUS[0]}` passed the whole suite even though the script documents that line as load-bearing. Adds the TMPDIR case that separates them, pins which stream the usage line takes, and compares the forwarded transcript verbatim rather than by line count. baseline_tests: `ambiguous_identity_records_a_line_for_every_member` held `path` constant, so dropping `a.path == b.path` from `same_identity` failed 0 tests -- #1170's churn bug returning for a function named alike in two files. baseline_tests: `from_violations_deterministic_order` fed its ambiguous pair already ascending, and `sort_by` is stable, so dropping `start_line` from `BaselineEntry::identity` failed 0 tests. check_explain_threshold: `mi.original=20` has an empty population over the fixture, so hard-coding `false` for `lower_is_worse` in explain.rs failed 0 tests; and every multi-metric test paired a live population with an empty one, so deleting the per-metric filter failed 0 tests. One new test covers both. worktree-setup-test: `main()` never ran twice, so deleting its READY skip failed 0 tests. `run_main` now returns stdout, and a new case asserts the second run is a no-op that says so.
`scale_threshold`'s floor direction rounds up, because a floor rounded down is a soft band that fires marginally late. But `limit / ratio` with `ratio == 1.0` reproduces `limit`, and the double for an off-grid limit such as 8.3 sits a hair above the decimal it prints as: `8.3 * 1e5` is 830000.0000000001, one ULP over a whole tick. A bare `ceil` promoted that to the next tick and resolved the soft floor to 8.30001 — above the hard floor, on a run documented as parity. Treat a value within FLOOR_GRID_SNAP_ULPS of a grid position as already on it. Four ULPs clears the single-ULP case with room to spare and sits orders of magnitude below the smallest genuine remainder a real ratio leaves (20 / 0.9 is 0.22 of a tick short), so the snap cannot swallow a remainder that ought to round up. `scale_threshold_ratio_one_is_identity` fed only grid-exact limits and so could not observe this; it now carries 8.3.
#1168 relaxed the marker grammar so a rationale could follow the verb, gating the bare-verb form on a separator (`-`, `:`, `//`, `#`, an em/en dash). Those are exactly the characters someone reaches for when writing prose *about* a marker, so an ordinary comment silenced every metric on its function with no diagnostic at all: // bca: suppress - we removed this marker, see #123 `bca check --threshold nargs=5` exited 0 on an eight-argument function. No separator set can separate a rationale from commentary here, which the file's own comments already half-conceded when they narrowed a bare `/` for the same reason. The metric-list form keeps its rationale — that is #1168's real fix and the parentheses are the positive signal a separator cannot supply. The bare verb returns to main's behaviour: bare it suppresses, with any trailing text it is MalformedBody, warned and dropped. `opens_rationale` goes with its last caller. Losing the bare-verb rationale is a real cost against AGENTS.md's "suppress with a reason", and the warning now carries the two ways out: name the metrics (more precise than the `All` hammer anyway) or move the reason to the line above. Over-suppression hides violations; under-suppression only annoys. Docs, the module grammar, and the `## [Unreleased]` entry for #1168 all describe what ships rather than what was briefly accepted.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1176 +/- ##
========================================
Coverage 98.11% 98.11%
========================================
Files 276 276
Lines 70238 70447 +209
Branches 69808 70017 +209
========================================
+ Hits 68914 69120 +206
Misses 907 907
- Partials 417 420 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Two `report --strip-prefix` tests located their prefix with
`fp.find("DeepSpeech/")` and asserted the stripped row contained
"DeepSpeech/stats.py". Both spellings are a unix path shape.
They passed on every platform until #1171, because the old helper built
the fixture path with a single `join("tests/repositories/DeepSpeech/
stats.py")` and Windows preserves the slashes embedded in that literal.
Replacing it with per-component joins made Windows emit
`DeepSpeech\stats.py`, so `find` returned None and both tests panicked
with "fixture contains DeepSpeech/" on the windows-latest leg alone.
Derive both halves from the same components the path is built from:
`corpus_fixture_suffix` joins the corpus and file names, and
`corpus_fixture_strip_prefix` removes that suffix from the full path, so
`prefix + suffix == corpus_fixture_path()` holds by construction on
every platform. A mismatch now panics naming both strings rather than
reporting a missing substring.
Also render `SuppressionError::ElidedDiagnostics`. It was constructed
and its variant asserted, but never formatted, so the one diagnostic a
reader meets only in the pathological case the cap exists for had an
untested message.
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.
Batch fix for #1165–#1174: nine issues across 18 commits, developed one
branch per issue on a shared integration branch so each fix saw the ones
before it.
Closes #1165, closes #1166, closes #1167, closes #1168, closes #1169,
closes #1170, closes #1171, closes #1172, closes #1174.
(#1173 is a merged PR, not an issue, and was dropped from the range.)
The issues were wrong more often than they were right
Every issue was re-derived by measurement rather than taken at face value,
and that is where most of the value came from. Corrections are recorded on
each issue.
offend at
nargs = 5sits at exactly 6, with nothing at 7 or above" — thereare 60 functions at
nargs≥ 7 (33@7, 14@8, 4@9, 4@10, 2@12, 1@13,1@14, 1@15). Three further figures in its plan were also off: hard offenders
at limit 7 are 27 not 29, soft offenders at limit 6 are 134 not 150, and the
distribution is 146@5 / 74@6, not 141 / 76. The conclusion survives — a
limit of 6 still buys ~74 permanent baseline entries for zero hard-tier gain.
start_linesincefeat(check): fuzzy baseline matching by qualified symbol, not start_line #377;
match_in_groupalready looks up(path, qualified, metric). There-key the issue asked for had shipped. The real defect was writer-side only —
the field was emitted unconditionally — and both symptoms follow from that.
--report-format sarifwithout--outputalready writes the document tostdout; moving the human rows there unconditionally interleaves them and
breaks
| jq. The shipped rule keeps rows on stderr for exactly that flagcombination, with three tests pinning the exception.
functions, outside the body, so neither attaches — pre-fix, both functions
are reported. The bug is real only with the markers inside the bodies.
exit_clap_error(docs(cli): document the workspace-wide exit-code convention #561/fix(cli)!: argv/usage errors exit 2, colliding with the exit-code contract #594) already remaps clap's exit 2 to
EXIT_TOOL_ERROR, so thevalue_parserplacement exits 1 either way. Keeping the canonicalisation outof the parser is still right, but on error-surface grounds — and the test
pinning it had to be rewritten, because an exit-code assertion alone could
not fail under the alternative placement.
Correctness
sloc,ploc, …) are canonicalised at all four parse boundaries instead of downstream inbuild_tiered, so a bare alias now overrides the dotted spelling rather than adding a second threshold for the same extractor.--print-effective-configoutput now round-trips through--configto an identical gate result.scale_thresholdis direction-aware: it divides a lower-is-worsemi.*floor instead of multiplying, so--tier=softis no longer a silent no-op for the whole family. #1141's!lower_is_worseguard exclusion is removed, which was the stated acceptance criterion.bca checkoffender rows move to stdout, so| wc -l,| head,| rg -cand2>/dev/nullstop silently reporting a clean tree. Diagnostics, summary and remediation stay on stderr.suppress(cognitive, exit)still silencescognitive.start_lineis recorded only where the identity is genuinely ambiguous, killing both the merge conflicts and the churn that hid real baseline changes in review.Features
bca check --explain-threshold <metric>=<limit>previews acandidate limit at both tiers without editing
bca.toml, reporting thealready-baselined split (so the new-entry count a reviewer weighs is on
screen) and naming a cluster when the candidate lands on one. It exists
because
--thresholdis documented as never scaled, which made theone-command way to trial a limit the one way that could not show its
soft-tier cost.
make worktree-setup, one idempotent bootstrap that escalates to--forcefor the interrupted-checkout state a plain re-run cannot repair.The 24 corpus-dependent tests now name their own cause.
BCA_GATE: pass (gate=pre-commit)/BCA_GATE: fail (…, stage=_pc-fmt).The failure path is the whole problem: make aborts on a failing prerequisite
and never reaches a trailing line, so the reporter sits outside the make
invocation and reads
${PIPESTATUS[0]}. Exit statuses are unchanged.Structure and quality
baseline.rssat at 524/550loc.ploc— inside the 0.95 soft band, with afresh baseline entry — so it was split by responsibility into
baseline/(
path_key.rs,body_hash.rs), taking it to 366. The move was verified bydiff-ing each relocated region against a pre-edit slice rather than by eye, sothe review question is "are these four visibility keywords intended?" rather
than "did anything drift in 370 lines?".
The three remaining over-threshold functions breach only
halstead.effortwhile sitting well inside cyclomatic (≤12/15) and cognitive (≤20/25) — that is
orchestration vocabulary, not branching, and splitting them would be the gaming
AGENTS.mdforbids. Left whole deliberately.Four quality passes (
simplify-rust,rust-optimize,review,audit-tests)produced ~20 findings, all resolved. The ones worth naming:
explain.rscopy-pasted the gate's post-walk pipeline order. The stagefunctions were reused but the sequence was duplicated, so a stage added to
run_checklater would compile fine in the preview while silently making itdiverge from the run it predicts — with no test failing, because both paths
would still pass their own assertions. Now shared via
collect_check_violations/classify_check_violations.--explain-thresholdprinted a soft derivation that did not happen.resolve_tier's soft branch is all-or-nothing per table, so a metric absentfrom
[thresholds.soft]inherits its hard limit unscaled — while the reportstill annotated it
0.95x. Counts were right; the label was false, on the onesurface whose entire purpose is to be trusted for a threshold decision.
?thatbounded the list at one, and
UnknownMetric'sDisplayrebuilt and sortedthe hint list per diagnostic — a ~100 KB comment yields ~8 MB of stderr and
~50k sorts. Now deduplicated, capped, and hoisted into a
OnceLock.failed zero of ~5,000 tests, then re-confirmed as the sole failure after
the fix. Four were a production expression made unobservable by the test's own
narrowing: a filtered count cannot see an extra line, a pre-sorted fixture
cannot test a tie-breaker under a stable sort, a constant field cannot test
its own clause of a composite key, and an empty population cannot test a
partition.
Two follow-ups from review, on top of the issue work
relaxation gated the bare-verb form on a separator set (
-,:,//,#,em/en dash) — which is exactly the punctuation people use writing prose
about a marker. Measured:
// bca: suppress - we removed this marker, see #123silenced every metric on its function, exit 0, no diagnostic. Onmainthe same input isMalformedBody, so this was a regression introducedby this batch. The metric-list form is safe because the parentheses are a
positive signal no prose emits by accident; the bare-verb form has none, so no
separator set can separate a rationale from commentary. It now warns with an
actionable message naming both escapes.
scale_threshold's floor rounding snaps to the sig-fig grid.8.3 * 1e5is
830000.0000000001, one ULP over a tick, so a bareceilresolved thesoft floor to
8.30001— above the hard floor, on a run documented as parity.Validation
make pre-commitgreen on the merged tree (BCA_GATE: pass), no.snap.newdrift, submodule unmoved,
man/in sync.Coverage rose — measured as covered counts, not percentages, since the
denominator moves for unrelated reasons:
Deferred
#1175 — surfacing the captured marker rationale in
bca exemptionsand theSARIF
justificationfield needs public field additions toOffenderRecordandSuppressionMarker, neither of which is#[non_exhaustive]. That is a3.0event under
STABILITY.md, not a minor.