fix: ten quick-fix issues (metrics, CLI, web, xtask, CI) - #1309
Merged
Conversation
Three claims in the "Output report formats" section -- the table that declares itself SemVer-protected in signature -- contradicted the shipped code, all introduced by a8d80f2 and never true: - write_sarif_with_suppressed was documented with two offender slices; it has taken three since c1827aa, because the suppression origins render different SARIF suppressions[].kind values (inSource vs external). A consumer coding against the documented shape gets a compile error. - The dump entry-point sentence promised shape stability for `Dump` / `DumpCfg`, which exist nowhere in the crate; CHANGELOG.md records their removal in 2.0. The real re-exported config type is AstCfg. - tool.driver.version was attributed to the CLI package; sarif.rs embeds the library crate's CARGO_PKG_VERSION. The two are in lockstep today, but the writer is also reached from the Python bindings' to_sarif, where no CLI exists. Documentation only: the shipped signatures are what downstream already compiled against, so the document moves to match the code. Fixes #1252
#838 replaced the before-side `git archive --format=tar` route with `ls-tree -r -z` + `cat-file blob` plumbing (diff::materialize_tree), because `git archive` silently honoured export-ignore / export-subst. Only materialize_tree's own doc was updated; six comments still named the retired mechanism. The no-`..` invariant is restated against what now enforces it: materialize_tree filters every ls-tree path through bytes_to_rel_path, which rejects `..` and absolute components outright. That is a stronger guarantee than the property the old comment inherited from git archive. The issue listed five sites and classified diff.rs:411 as deliberate history. It is not -- TREEISH_SUFFIX's doc describes in the present tense what happens downstream now, so it is reworded too. The two genuine history mentions (diff.rs:458, :468, explaining why the route was replaced) are left alone. Comment-only; every file is net-zero-line so no file-level loc.sloc baseline moves. Fixes #1246
The to_sarif logical-location docs named a "<unnamed>" sentinel the
code has never emitted. extract_space_fields renders a closure/lambda
(the "<anonymous>" name every grammar emits) and the None-name
parse-failure case alike as "<anon@L{start_line}>", mirroring the CLI's
space_segment. A consumer filtering SARIF logical locations on
"<unnamed>" matched nothing.
The same sentence carried two further errors, both fixed: it credited
the rule to a CLI "function_token" that exists in no crate, and it
called this the "rare parse-failure case" when closures and lambdas
are the common path through it.
Separately, #667 split vcs_jit into commit / score_diff, but three
rustdoc and docstring sites still cited `bca vcs jit --diff`, a
subcommand spelling that survives only as a hidden alias. The issue
listed the two in the bindings; a fourth in the root crate
(src/vcs/jit.rs) makes the same present-tense claim and is corrected
too.
Also repoints two stale cross-references found while verifying: the
py sarif.rs comment placed space_segment in thresholds.rs (it is in
qualified_name.rs), and test_sarif.py credited the <file> sentinel to
function_token rather than qualified_symbol.
Left alone deliberately: the deprecation, changelog, STABILITY and
lessons-learned mentions of `vcs jit` are correct history, and
cli/tests/vcs/vcs_jit.rs exercises the alias on purpose alongside the
canonical spelling.
Documentation and docstrings only; no runtime behaviour changes and no
stub signatures move, so py-stubtest is unaffected.
Fixes #1240
The comment above lang_cpp_resolves_to_upstream_tree_sitter_cpp called asserting against `tree_sitter_mozcpp` "the load-bearing detail here", which is the inverse of what the test asserts: #720 rebound Lang::Cpp to the upstream community grammar and moved the Mozilla fork to Lang::Mozcpp, updating the test name, body and inner comment but not the outer block. Keeps the still-true #344 history, states the current contract, and names lang_mozcpp_resolves_to_vendored_mozcpp as the fork's own pin. The two partially-overlapping comments are folded into one. This file exists to be read against the dispatch table by a human, so a comment asserting the inverse of the code defeats its purpose. The sibling entries were checked for the same #720-era drift: the C, Objc and mozjs/javascript comments are accurate and unchanged. Fixes #1239
Revises the four prose docs the quick-fix batch touched against docs/conventions/documentation.md. Two of the corrections are factual errors, not style: - STABILITY.md told readers the Python package was "not yet on PyPI" and that the names were what a first publish would lock in. It has been published for a long time (README.md, CHANGELOG.md). A stability contract saying its own names are not yet locked is the worst thing for it to be wrong about. - Both SARIF sections claimed there are "no built-in default thresholds". Since #1140 default_thresholds.rs is the source of truth for the limits bca init scaffolds. Reworded to what is still true: bca check applies no *implicit* limits. Also: the py README taught `bca check -O sarif`, a deprecated alias for --report-format on check (deprecations.rs); three relative links that 404 on the PyPI-rendered page are now absolute; and the README's release-status paragraph ("All nine phases ... have landed", issue numbers) is gone, per the convention's rule that a README is not a changelog. Convention work: link tree-sitter, SARIF 2.1.0, GitHub Code Scanning and Halstead on first mention; retitle a sarif.md heading that described only the Unit case while its section covers every space; name what exporting-data.md actually contains; drop repo-internal history from an end-user recipe; normalize ~25 British spellings to the dominant American form, preserving the plural "analyses". Existing em dashes are untouched. The corpus has roughly 1,250 across 49 of the book's 51 pages, so removing them is a repo-wide decision rather than a rider on four files. Documentation only: no executable line changes, and the version pins remain at the published release 2.1.0 that check-versions requires.
`PhpCode::get_op_type` listed `Name`, `VariableName` and
`DynamicVariableName` in one operand arm, but the grammar nests them:
`$x` is a `variable_name` wrapping a `name` leaf, and `"${x}"` a
`dynamic_variable_name` wrapping one directly. The walker visits both,
so every variable reference contributed twice to N2 and planted a
sigil-less twin (`x` beside `$x`) in the n2 vocabulary. Variable-
variable syntax compounded it: `$$a` scored 3 and `$$$b` scored 4 for
one reference. Since `$var` is the most common token class in PHP,
N2 roughly doubled on real files and every derived Halstead value —
volume, difficulty, effort, and the MI built on them — was wrong.
Parent-guard the nested kinds instead: `Name` yields `Unknown` under a
`variable_name` or `dynamic_variable_name` parent, and the two wrappers
yield `Unknown` under a `dynamic_variable_name` parent, which handles
arbitrary nesting depth. This is the guard Bash already applies to
`variable_name` under `simple_expansion` and iRules to `Id` under
`variable_substitution`. Both kinds move out of the plain operand arm
so the classification no longer depends on arm order. `Name` stays an
operand everywhere else — function, class, const, member and
namespace-component identifiers all use it — and an inner reference
reached through a real expression (`${$x . 'y'}`) still counts.
The remaining wrapper/leaf double counts in the same arm (type
expressions, qualified and namespace names) are the same class but need
a per-construct call on which node carries the operand; they are marked
with a FIXME and tracked in #1293.
Fixes #1259
Both regression tests used inputs where every operand was unique, so n2 and N2 were equal and the assertions could not tell "each reference counted once per occurrence" from "the vocabulary deduplicated them". Repeat one variable in each fixture so N2 exceeds n2, and record the measured pre-fix counts (9 / 13 and 14 / 17) rather than derived ones.
Elixir's `boolean` / `nil` and C#'s `boolean_literal` are named
wrappers around a keyword leaf, and both the wrapper and the leaf sat
in `get_op_type`'s operand arm — so every `true` / `false` / `nil` /
`null` occurrence added one spurious operand to Halstead N2, skewing
length, volume, difficulty and effort. Operands are keyed by source
text, so the duplicate collapsed into the existing vocabulary entry
and n2 stayed correct, which is why nothing caught it.
Elixir drops the leaves outright, mirroring Ruby's `Nil` / `Nil2`
split: `boolean: choice("true", "false")` and `nil: "nil"` are the
grammar's only producers of them, and the one other position that
accepts the reserved words — the right-hand side of a remote dot —
aliases them to `identifier`.
C# parent-guards the leaf instead. Deleting it would have been wrong:
the overloadable-operator list emits a bare `true` / `false` under
`operator_declaration` with no wrapper, so `operator true` and
`operator false` would have lost the operand that tells them apart.
Whether that name is better classified as an operator, as `operator +`
already is, is deferred to #1296.
A sweep of the other nineteen languages found no second instance of
this shape; PHP's `primitive_type` wrapper is the same class but is
already tracked as FIXME(#1293).
The new arms sit inside a match that already bailed out of rustfmt for
a pre-existing in-pattern comment (#1136), so `.rustfmt-bail-baseline.txt`
ratchets from 2 to 5 for `src/getter/csharp.rs` without a new cause.
Fixes #1253
`java_count_token_condition` and `groovy_count_token_condition` denied
`<` / `>` only under `TypeArguments`, so a generic *declaration* —
`class Gen<T>`, `<T> void m()`, both `type_parameters` — scored two
conditions. `class Gen<T> { void plain() { int a = 0; } }` measured
`abc.conditions = 2` in each language with no conditional construct in
the file.
Flip to the allowlist polarity (parent is `BinaryExpression`) that C,
C++, Objective-C, mozcpp, Rust and Go already use, rather than
extending the denylist as the issue planned. A `grammar.json` sweep
shows a bare `<` / `>` is emitted from exactly three productions in
tree-sitter-java (`binary_expression`, `type_arguments`,
`type_parameters`) and four in dekobon-tree-sitter-groovy (those plus
`method_type_parameters`), so on well-formed input the two shapes are
equivalent — revert-verified: the planned `| TypeParameters` denylist
fails no Java test. They differ on input the grammar cannot parse: a
Groovy explicit type witness (`Collections.<String>emptyList()`) puts
its `<` under an `ERROR` node, which no denylist can name. The
allowlist also needs no revisiting when a grammar bump adds a fifth
type-syntax production, which is the coverage claim
`.claude/rules/grammar-dispatch.md` §1 warns against making.
The denylist the issue proposed would also have been incomplete for
Groovy: a generic *method* (`def <U> U m(U x)`) emits
`method_type_parameters`, not `type_parameters`.
The same sweep found a second instance of the same bug in the same
match: a bare `?` comes only from `ternary_expression` and `wildcard`,
so `List<? extends Number>` scored one condition as a phantom ternary
in both languages. Gate `QMARK` on `TernaryExpression` the same way.
Sibling sweep over every grammar's angle-bracket productions: C, C++,
Objective-C, mozcpp, Rust and Go are immune (allowlist already); PHP,
Python, Tcl, iRules, Perl and Elixir emit `<` / `>` from no
non-comparison production that is reachable. C#, Kotlin, TypeScript,
TSX, JavaScript, Ruby, Lua and Bash are affected by the same class of
over-count through *different* constructs (JSX elements, `super<A>`,
`operator <`, `class Foo < Bar`, `local x <const>`, shell
redirection); each needs its own grammar analysis and fixture set, so
they are filed separately rather than folded in here.
No integration-snapshot churn: the DeepSpeech corpus snapshots cover
only `cc` / `h` / `hh` / `cpp`, and no Groovy file is snapshotted.
Also folds four near-identical copies of the deepest-space
ABC-versus-cyclomatic parity walker in the test module into one
helper beside the shim whose comment already states that invariant.
The wildcard and method-type-parameter fixtures assert a non-zero
condition total rather than 0: an unparsable file also scores 0, so a
zero assertion could not tell "the construct was ignored" from "the
fixture stopped being recognised". Each carries two wildcards against
one ternary so that every way of mis-aiming the gate lands on its own
number.
Fixes #1274
`impl Npm for CppCode` and its Mozcpp clone gate their
`TemplateDeclaration` arm on the shared `cpp_has_function_declarator`
helper, which recursed through pointer / reference / declaration
wrappers hunting a `function_declarator`. A templated member *with a
body* parses as `template_declaration > function_definition`, so the
helper never found one and the method scored zero: `npm` reported one
method on a class where `nom` opened two function spaces and `wmc`
weighted two.
Widen the shared helper rather than adding a local predicate to each
of the two npm impls, against the earlier suggestion on the issue. The
reason given there was cost-avoidance -- not having to re-verify the
npa call site -- and that verification turned out to be cheap and
definitive, so the reason lapsed. The widening is inert for `Npa` on
three independent grounds: a `node-types.json` subtypes closure over
exactly the arms the helper recurses through shows neither
`function_definition` nor `declaration` is reachable from a
`field_declaration` in either grammar; the npa suite stays green under
every perturbation of the new arm; and no `npa` key moved anywhere in
the 92 corpus snapshots this change re-baselines.
Accept a `function_definition` child outright instead of recursing into
it, which is where this departs from the issue's resolution plan. A
conversion operator's declarator is an `operator_cast`, so
`template<typename T> operator T() { ... }` contains no
`function_declarator` at any depth and the prescribed recursion would
still score it zero. Matching on the kind-name string covers both
grammars and all four aliased `FunctionDefinition` ids at once, and
short-circuiting is cheaper than walking the subtree.
Rename the helper to `cpp_declares_function`: it no longer only finds
declarators, and the old name would have been actively false for the
conversion-operator case.
The 92 re-baselined DeepSpeech snapshots are metric-value-only (1762
insertions against 1762 deletions, no structural change). Every count
metric moves up; the only decreases are `class_coa`, a ratio whose
denominator grew.
Two adjacent gaps found while sweeping the rest of the arm are filed
separately rather than folded in, since neither is a template bug:
declaration-only conversion operators are counted as neither method
nor attribute, and a templated static data member is invisible to npa.
Fixes #1258
`is_call` for C, C++ and Mozcpp matched only the unsuffixed
`CallExpression`, but that enum variant is the grammar's
`preproc_call_expression`, referenced only under an
`alias(..., $.call_expression)`. `ts_symbol_map` rewrites it onto
the aliased `CallExpression2` for every input, so the unsuffixed id
never reaches `kind_id()` and the predicate matched nothing:
`bca count -t call` and `bca find call` reported 0 on ordinary
C/C++ source.
Add the aliased variant to all three siblings, keeping the
unsuffixed one as a defensive arm per grammar-dispatch §1. The
already-correct `objc.rs` was the template; the ABC walkers in
`src/metrics/abc/{c,cpp,mozcpp}.rs` had listed both ids all along,
so the checkers were the outliers.
`is_call` feeds exactly one consumer — the `"call"` filter in
`ParserTrait::filters` — so no metric moves and no snapshot churns.
`new_expression` stays unlisted: object creation is an ABC concern,
matching Groovy / Java / C# (#430).
A cross-language sweep of the remaining 20 `is_call` impls found no
sibling with the same defect. The unmatched variants elsewhere are
either `_`-prefixed hidden rules (Ruby `Call5`, Tcl / iRules
`Command2`, TypeScript `CallExpression3` / `4`, the two C# pattern
kinds) or alias sources that surface under another id (JS / TS
`decorator_call_expression`).
Fixes #1254
A client mistake in `author_hash_key` on `POST /v1/vcs` or `/v1/vcs/trend` answered `400` with `error_kind: vcs_internal_error`, the token reserved for backend faults, so a client branching on the token saw its own mistake as a server failure. `InvalidAuthorHashKey` is classified client-input by `is_client_input` but #956 never gave it an arm in the web crate's `vcs_error_kind`, so it fell to the `_ => VCS_INTERNAL_ERROR` wildcard. Add the token and the arm. The wildcard's own comment claimed it "cannot silently mis-classify a client error" because `is_client_input` is an exhaustive forcing function. That was false and is why the drift lasted: `vcs::Error` is `#[non_exhaustive]`, so a match in any other crate is *required* to carry a wildcard and the compiler forces nothing there. `is_client_input` forces the status, and only the status. Rebuild the guard as the compiler cannot. `classify_error_variants!` generates `is_client_input` and a new `#[doc(hidden)] pub client_input_samples()` from one variant list, and the web token test iterates those samples instead of a hand-written copy. Adding a variant is a compile error at the macro (the generated match has no wildcard); classifying it as client input then yields a sample, which fails the token test until a token exists. A plain hand-written samples list, the alternative considered, has the same weakness as the test it replaces. The list takes `pat_param`, not `pat`, so one entry cannot cover two variants via an or-pattern -- the shape these arms used before the macro, and one that would silently starve the second variant of a sample. The API cost is one `pub` item, which STABILITY.md already places outside the contract for `#[doc(hidden)]`. The same blind spot sat in the library's own tests, which the new mechanism now closes: `is_client_input_classifies_every_variant` listed ten of eleven client variants and `display_covers_every_variant` sixteen of seventeen, both omitting `InvalidAuthorHashKey`. The group counts are pinned by hand on purpose -- *moving* a variant between groups shrinks one list and grows the other, which every per-variant assertion survives. Also record `not_acceptable` and `serialize_failed` in STABILITY.md, missing from the documented vocabulary since #657. Fixes #1245
Auditing the guards added in 04508c67 found two of the three new count assertions could not fail for any change to `Error`. Both compared a vec literal in the test against a constant beside it, so only editing the literal moved them. Measured: adding a new environment variant to `Error` -- unasserted by any test in the file -- left all five tests green. Drop both. `CLIENT_INPUT_COUNT` stays, because it is compared against `client_input_samples()`, which production generates, and it does fire (verified by moving a variant between the two groups). In their place `display_covers_every_variant` now checks its case list against `client_input_samples()` by `mem::discriminant`, which catches the omission that actually happened: `InvalidAuthorHashKey` had no Display case from #956 until #1245, leaving the `error` prose of a 400 pinned by nothing. Removing that case again now fails the test by name. This covers the client-input half only. The environment variants cannot be enumerated, so a new one still goes unpinned; the comments say so rather than implying a completeness the test does not have.
`bca metrics --output <FILE>` and `bca ops --output <FILE>` serialized the collected per-file results in worker-completion order, so two runs over an unchanged tree wrote differently-ordered documents at `--jobs > 1` — twelve measured runs over a 40-file tree gave twelve distinct orders and none of them sorted. The walk resolves its file list sorted, but the aggregate channel discards that order and `write_aggregate` never restored it, unlike every sibling collector (`run_check_walk`, `collect_marker_rows`, the #1091 `Ops` vocabularies). Sort the collected items by emitted path before serialization, which covers the generic array, the TOML `files` wrapper, the CBOR/YAML arms, and the CSV row concatenation from one site. `AggregateItem::Ops` now carries the emitted path alongside the tree — `Ops::name` is a lossy rendering of it, so a non-UTF-8 path could reorder under the string key — giving both variants a total ordering key via `emitted_path()`. The enum is unchanged in size and the path was previously dropped at the send site, so this costs nothing. Refreshes the `write_aggregate` halstead.effort baseline entry, which the added statement moves past the hard limit. Fixes #1244
The remediation block's copy-paste baseline-refresh invocation still used the pre-#597 shape, `bca <walk flags> check <check flags>`. Those walk flags became subcommand-scoped in #597, so every failing gate printed a next-steps command that exits 1 with a clap usage error. Rebuild it as argv starting `["bca", "check", ...]`, and render the displayed string by shell-quoting each element. Splitting the flag list from its shell rendering is what makes the regression testable: the guard now feeds the argv to `Cli::try_parse_from` and compares the parsed flags against the run that produced them, where the eight existing assertions only ever string-matched the output. Mirror every flag that decides what `--write-baseline` records, not just the walk scope. `--check-exclude` / `--check-exclude-from` were the known gap (`apply_check_exclude` runs before `write_check_baseline`, #378), but `--threshold` and `--no-config` were worse: with the ordering alone fixed, the issue's own reproducer still failed, with "no thresholds configured" instead of a usage error. Also mirrored: `--include`, `--paths-from`, `--language`, `--preproc-data`, `--no-ignore`, `--no-skip-generated`, `--exclude-tests`, `--cyclomatic-count-try`, `--tier`, `--no-suppress`, and `--baseline-fuzzy-match`, which populates each entry's `body_hash`. Reporting and exit-code flags stay off; `--since` / `--changed-only` are omitted because clap forbids them alongside `--write-baseline`. The resolved tier is threaded in from `run_check` rather than read off `args.tier`, which loses a deprecated `--headroom <R>` promotion, and re-resolving would re-emit that flag's deprecation warning. That leaves nothing composition-affecting omitted, so the enumerated "re-add any --include / --language / ..." caveat is replaced by one that is conditional and true: a `-` list flag read stdin, which no printed command can replay. `run_check`'s halstead.effort moved past its baselined value on the one added argument, so `.bca-baseline.toml` is refreshed here. Fixes #1243
An audit of the tests #1243 added found three perturbations that left the whole crate suite green. Hard-coding `TierSpec::Hard` at `format_remediation_block`'s call to `refresh_baseline_command` — the exact mistake threading the resolved tier through exists to prevent — failed nothing, because every test called the builder directly and none pinned the seam between the block and the builder. Widening `shell_quote`'s fast-path allow-list to admit `$`, `~` and `&`, and dropping its `!s.is_empty()` guard, also failed nothing. The `$(echo pwned)` fixture looks like it covers `$`, but the parentheses force the slow path by themselves; a value has to isolate a metacharacter to test it. The empty string is the same gap inverted: unquoted it is no word at all and every later argument shifts left. The third was a deny-list. "Flags the gate never set stay off the command" named four of sixteen conditionally-emitted flags, so pinning `--baseline-fuzzy-match=true` onto a run that set neither passed. It is now an exact whole-argv assertion, which also covers flags added later.
`git diff` compares tracked content only, so the page `cargo xtask` writes for a brand-new subcommand — untracked, never committed — passed `git diff --exit-code -- man/` green in both the CI `manpage` job and `make manpages-check`. The release artifacts then shipped without that page and nothing flagged it afterwards. Measured verdicts for every state that reaches this gate: state git diff ls-files --others porcelain clean pass pass pass new page, untracked PASS fail fail new page, staged pass pass FAIL tracked page modified fail pass fail tracked page deleted fail pass fail page removed from index PASS fail fail gitignored new page PASS see below pass `git diff` plus `ls-files --others` is the only pair that covers every row. Plain `git status --porcelain` fails the staged row by design, and staging is the remedy the gate's own message prescribes — the `pre-commit` framework runs the `manpages` hook against the staged tree, so a porcelain gate would reject every legitimate man-page commit. `git add -N` is rejected too: it mutates the contributor's index. The last row is why the untracked half runs *without* `--exclude-standard` and is scoped to `man/*.1` instead. A global `~/.config/git/ignore` line matching `*.1` reinstates the bug exactly, and no in-repo config overrides it; `xtask` writes nothing but `.1` files, so narrowing the pathspec to the generated surface means dropping the ignore rules cannot turn an editor dropping into a gate failure. The tracked half keeps the old `man/` scope. The assertion moves out of the two hand-mirrored shell blocks into `utils/check-manpage-drift.py`, which both sites now call, so they cannot drift apart; it emits the CI `::error::` annotation itself when GITHUB_ACTIONS is set. The verdict comes from `git diff --exit-code`, not from stdout emptiness — a configured `GIT_EXTERNAL_DIFF` can print nothing and would otherwise read as a clean tree. `utils/check-manpage-drift-test.py` pins all of the above against scratch repositories (never the real `man/`, which `_pc-manpages` rewrites concurrently). Each element of the implementation was perturbed to its plausible-wrong alternative and watched fail: dropping the `ls-files` half → 6 red; `git status --porcelain` → 5; `--exclude-standard` → 1; widening either pathspec → 1 and 2; dropping `--exit-code` → 5; dropping `--no-ext-diff` → 1; swallowing the untracked half's GitError → 1. Two notes for whoever reads the issue: * the issue's prescribed probe cannot work. `touch man/zz-probe.1 && make manpages-check` passes even with this fix, because `sweep_orphans` deletes any unexpected `.1` before the check runs. `git rm --cached man/<page>` is the faithful reproduction. * the issue says `utils/check-manpage-assets.py` "reads the committed file list" and so cannot see the missing page. It actually globs the filesystem — but in CI it runs in a job that never runs `cargo xtask`, and locally it races `_pc-manpages` in the parallel DAG, so it is not a backstop either way. Interacts with #1250 (`sweep_orphans` is case-sensitive, unlike the write guard). On a case-sensitive filesystem a case-only command rename deletes the stale page and adds a new-case one; the old gate reported only the deletion, so its message never named the page that had to be added. `CaseOnlyRenameTest` pins that both are now reported. On a case-insensitive filesystem the same rename makes `sweep_orphans` delete the page it just wrote — that is #1250, in the sweep, not here. Fixes #1249
An independent audit of the suite added in 450095d9 found six tests that could pass while the thing they name was broken. Each fix below was confirmed by perturbing the exact production line and watching one test — and only that test — go red. * The verdict-from-exit-status rule was unpinned. With `--no-ext-diff` in place git never consults the external driver, so `test_external_diff_driver_cannot_report_clean` ran an ordinary diff and asserted what its sibling already did; switching `tracked_drift` back to a stdout-emptiness verdict failed nothing. That rule and the empty-output fallback under it are now covered by `EmptyDiffOutputTest`, which drives `_git` directly — no filesystem state can produce "status 1, no output" while the flag is passed. The external-diff test is renamed to what it actually pins, `--no-ext-diff`. * `GitFailureTest` covered only the untracked half. `main()` calls `tracked_drift` first, so letting the *diff* half swallow every git failure failed nothing. Both halves now have their own test, plus a both-halves case. * `GitFailureTest` also inherited the host environment and assumed $TMPDIR sits outside any repository. With TMPDIR inside a checkout, git discovered an ancestor `.git`, the gate reported a clean tree and both tests failed `0 != 2` with no hint of the cause. It now sets `GIT_CEILING_DIRECTORIES`, clears `GIT_DIR`/`GIT_WORK_TREE`, and asserts its own precondition — verified by removing the ceiling and re-running with TMPDIR inside this checkout, which now names the reason. * `CaseOnlyRenameTest` is guarded on a probed case-sensitive filesystem. `git init` sets `core.ignorecase=true` on APFS and NTFS, where the rename produces a different tree; the class documented the assumption and nothing enforced it, so it would have gone red only for a macOS contributor running `make pre-commit`. * `test_no_annotation_locally` never asserted the gate failed, so it was vacuous whenever the added-page half stopped firing. The helper now asserts the failure, and two further cases pin the annotation's predicate (`== "true"`, not `is not None` — popping the variable cannot tell those apart) and its placement (it must fire for a modified page too, not only an added one). * `test_modified_page_diff_is_printed` discarded the exit code. Also corrects the `ManDirFixture` docstring: its `.gitignore` is realism, not a lever — emptying it changes no verdict, because the untracked half never passes `--exclude-standard`. Per lesson 84, a factual claim in a docstring is untested code. Refs #1249
`render_man_page`'s collision guard compared filenames ASCII-case-insensitively; `sweep_orphans` compared them byte-for-byte. On APFS / NTFS a case-only command rename (`bca` -> `BCA`) writes into the pre-existing `bca.1` directory entry — those filesystems are case-preserving, so opening a file under a different spelling does not rename it — and the case-sensitive sweep then unlinked the page it had just written, with `cargo xtask` still exiting 0. Extract the relation into `names_same_file` so the two sites cannot spell it differently again, and classify each directory entry three ways rather than two: byte-equal keeps, no match removes, and a match that is not byte-equal is a conflict. The simpler remedy — give the sweep `eq_ignore_ascii_case` and keep two outcomes — was rejected. The two sites use the relation with opposite polarity: the guard folds case to *reject* more collisions, whereas a sweep that folds case *retains* more entries, so "share the guard's relation" would invert the sweep's purpose and permanently disable stale-page removal for any old-case page. The issue's own rationale for that rejection was partly wrong and is worth correcting. It claims the stale page is "tracked and unchanged, so the drift gate never flags it". Measured against `utils/check-manpage-drift.py` as of #1249: at the moment of the rename the gate is red, because its untracked half reports the new `BCA.1`. What the gate cannot see is the state after its own remedy — `bca.1` is never dirty, since the rename does not touch its bytes, so it is invisible to both halves from the outset and simply stays tracked. The page does ship forever, but through the prescribed fix rather than gate blindness. Erroring is the only verdict derivable from the filenames alone that reads the same on every filesystem, which is what the guard's comment says the crate normalises for. The sweep classifies every entry before unlinking any of it. `fs::read_dir` order is unspecified, so removing as it went would delete a different subset of the genuine orphans on each run before bailing, and would report only the first of several renames. Two phases make "the error path removes nothing" a postcondition a test can assert. The remedy in the message is a plain `rm`, deliberately: on a case-insensitive filesystem the stale spelling now holds the freshly rendered content, so `git rm` refuses it for having local modifications — broken on exactly the platform the error exists for. Composes with #1249 without depending on it: that gate makes the rename transition visible, this makes it impossible to reach silently. Tests cover both filesystem shapes — a single `bca.1` entry (APFS) and both entries (ext4) — which are constructible in a tempdir either way, plus the ASCII-only boundary of the relation and the no-removal postcondition. Verified by test-via-revert against six perturbations: the pre-fix case-sensitive sweep, the competing two-way `eq_ignore_ascii_case` keep, `to_lowercase()` full folding, an inverted keep guard, a remedy naming the expected spelling instead of the stale one, and a single-phase remove-then-raise. Each fails only the tests that name its behaviour. Fixes #1250
The two-phase classification in 664688b5 claims it reports every case-only rename rather than only the first, but no test supplied more than one conflict — `conflicts.truncate(1)` passed the whole suite. Assert both stale spellings appear in the error, by presence rather than position, since `fs::read_dir` order is unspecified.
Record the ten quick-fix issues fixed on this branch in the Unreleased section. Entries are consolidated here rather than written per fix so the parallel fixes could not conflict on this file.
.bca-baseline.toml is marked -merge in .gitattributes, so the rebase left it wholly conflicted rather than splicing the two sides. That is deliberate: neither side's recorded values describe the merged tree. Regenerated with make self-scan-write-baseline-headroom, which confirms the point. run_check's halstead.effort is 63636 on the merged tree, against 62885 on main and 61595 on this branch - higher than either, because main's walk changes and this branch's #1243 signature change compound. Taking either side would have left the gate red. The regeneration also restores main's classify_dropped_child nargs entry, which the intermediate conflict resolution had dropped.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1309 +/- ##
==========================================
+ Coverage 98.35% 98.38% +0.02%
==========================================
Files 278 278
Lines 72069 72352 +283
Branches 71639 71922 +283
==========================================
+ Hits 70882 71181 +299
+ Misses 777 762 -15
+ Partials 410 409 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
sweep_errors_on_case_only_rename_with_both_entries wrote bca.1 and BCA.1 and asserted both survive with their own contents. Two spellings are only two files on a case-sensitive filesystem; on APFS and NTFS the second write lands in the first file's directory entry, so the stale page reads back as the freshly rendered one and the last assertion fails. Green on ubuntu, red on macos-latest and windows-latest. The comment claiming "the assertions below hold in both worlds" was the actual defect - three of the four do, and the fourth is what the collapse invalidates. It now says which. Case-sensitivity is probed at run time rather than inferred from a cfg: macOS is cfg(unix) yet case-insensitive by default, and a Linux checkout can sit on a case-insensitive mount, so a cfg gate answers a different question. The shared assertions - the AlreadyExists verdict, the remedy naming the stale spelling, the expected page intact - still run everywhere, so neither branch is vacuous. The case-insensitive branch asserts the hazard directly: exactly one entry, still spelled bca.1, now holding the fresh bytes.
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.
Ten
quick-fixissues, each taken through investigate → fix → simplify → review → validate, on one branch. Rebased ontomain(d65b3049);make pre-commitis green on the tip.Fixes #1259
Fixes #1253
Fixes #1274
Fixes #1258
Fixes #1254
Fixes #1245
Fixes #1244
Fixes #1243
Fixes #1249
Fixes #1250
What changed
getter/php$variablecounts once in HalsteadN2getter/elixir,getter/csharpabc/java,abc/groovy<>and?no longer score conditionsnpm/cpp,npm/mozcppchecker/{c,cpp,mozcpp}is_callmatches the aliased kindvcs_invalid_author_hash_keytoken + macro-forced guard--outputsorted by pathEvery issue body's prescribed fix was incomplete or wrong
Worth flagging for reviewers, because in each case the correction came from an authoritative artifact rather than from the issue's reasoning:
"${z}"(a directnamechild ofdynamic_variable_name) and self-nesting$$$x.#if true) was wrong; that position is wrapped. The genuinely unwrapped position isoperator true, so C# needed a parent-guard rather than the prescribed deletion.| TypeParametersdenylist does not fix Groovy at all (generic methods emitmethod_type_parameters), and a second token in the same arm was scoringList<? extends T>as a phantom ternary. Took the allowlist polarity instead, after proving the parent set is closed.function_definitionto the recursion set missestemplate<typename T> operator T(), whose declarator is anoperator_castwith nofunction_declaratorat any depth.CallExpressionid issym_preproc_call_expression; the "alias" is the real rule. Only the generatedparser.csymbol table shows this.error: no thresholds configured);--no-configand--thresholdwere larger composition gaps than--check-exclude.PathBufidentity is carried instead.cargo xtaskruns first andsweep_orphansdeletes the probe file before the check executes, so it reports green either way.A methodological note that cost real time:
grammar.jsonandnode-types.jsondisagree. A closure overgrammar.jsonreportedoperator_castunreachable fromdeclaration, which the live AST contradicts.node-types.json'ssubtypeskey is the authoritative expansion of hidden_-prefixed supertypes.Metric-value impact
Behaviour-changing, so published values move:
abc.conditionsdrops wherever generics appear.npmrises on classes with templated inline-bodied members — 92 integration snapshots re-baselined, withnomandwmcunchanged (the fix closes a disagreement rather than creating one).Submodule
tests/repositories/big-code-analysis-outputbumped to77c11134, pushed to its remotemain.Three things needing a maintainer decision
error_kindwire value in a patch line (2.1.0 → 2.1.1). Framed inSTABILITY.mdas a bug fix rather than a rename, on the grounds that a client-input mistake previously labelled internal is exactly what the vocabulary exists to distinguish. Reviewer's call..rustfmt-bail-baseline.txtgrew — 12 cause-2 entries from fix(web): vcs InvalidAuthorHashKey mis-labeled as vcs_internal_error #1245's guard macro, plus ratchets forphp.rs(2→8) andcsharp.rs(2→5) where new arms joined already-bailing matches. Rationale is in the baseline header..bca-baseline.tomlwas regenerated, not hand-merged. The rebase left it wholly conflicted (it is-mergein.gitattributes). Regeneration vindicated that:run_check'shalstead.effortis 63636 on the merged tree against 62885 onmainand 61595 on this branch — higher than either, becausemain's walk changes and this branch's fix(cli/check): remediation refresh command uses pre-#597 flag order and fails to parse #1243 signature change compound.Follow-ups filed
#1293, #1294, #1296, #1297, #1298, #1299, #1300, #1301, #1302, #1303, #1304 — PHP type/qualified-name wrappers, a Tcl operand-vocabulary gap, C#
operator trueclassification, six more languages with the generic-bracket bug, four C++ npm/npa/wmc gaps, an ungatedSTABILITY.mdtoken list, and two further nondeterminism bugs (-O jsonto stdout,preproc --output).