fix(check): report skipped inputs, add --strict gate profile - #1305
Merged
Conversation
Two inputs the branch under test controls could silently remove files from the `bca check` gate: a generated-code marker (`@generated`, `DO NOT EDIT`, `GENERATED CODE`) anywhere in the scan window, and a `.gitignore` committed in the tree. Per the decision on the issue, make the gate loud rather than strict: - Tally generated-skipped files (new `Config.generated_skipped` counter) and derive the ignore-dropped set by resolving the seeds a second time with ignore handling off and diffing (the walker never yields ignored entries). The second resolve is metadata-only and check-only; `--paths-from` is materialized once first so a stdin (`-`) list is not read twice. - Emit a one-line stderr summary whenever the gate skipped anything, before the empty-input guard so the sole-ignored-input case still names its cause above the exit-1 error: `bca: 2 files not checked (1 generated, 1 ignored) — pass --report-skipped to list them`. Clean runs stay silent; exit codes are unchanged. `--report-skipped` now also lists each ignore-dropped file. The summary uses the severity-free `bca:` family, not the issue's proposed `bca: note:` spelling, which would reintroduce the #609 double prefix. - Add `--strict` and the presence-only `[check] strict` manifest key: the untrusted-input gate profile, equivalent to `--no-skip-generated --no-ignore`, recorded in `--print-effective-config`. Rename `CheckOutcome::exit_code`'s `strict` parameter to `tiered` to keep the terms apart. Fixes #1055
Both #1055 bypasses reproduce from the book now: a new "The gate's trust boundary" section in the CI recipe carries the issue's reproduction tables, the buried-marker form, the default skipped-input summary, and the one-flag `--strict` recommendation for PR gates. The check page documents `--strict` and the presence-only `[check] strict` key; the commands index extends the generated-code and `.gitignore` flag sections with the summary. Japanese translations for the new and touched strings are filled in po/ja.po (plus the mechanical 2.1.0 version-string fuzzies msgmerge surfaced). Refs #1055
The #1055 additions tripped the dogfooded gate in three places, and each had a real simplification rather than a rebaseline: - dispatch.rs repeated the same optional-counter bump four times; a `bump_tally` helper replaces the `if let Some(counter)` blocks and returns `validate_and_resolve_file` under its cognitive and halstead limits. - The skipped-input measurement and summary move to a `commands/check/skipped.rs` submodule, the same boundary the six existing check submodules draw, taking check.rs back under the file-level ploc limit. - The strict-profile flip becomes `CheckArgs::apply_strict`, alongside the type's other resolution methods. `run_check` keeps one genuinely new pipeline step and lands 3% over its recorded halstead.effort, so the baseline is refreshed with `make self-scan-write-baseline-headroom` in this same change, per the baseline-refresh discipline. Refs #1055
The bump_tally helper landed between the doc comment and its function, silently reattaching the pre-dispatch-filters doc to the helper. Found by the simplify-rust review pass. Refs #1055
The independent review ran the #1055 ignored-file measurement against a real tree and it failed badly: the no-ignore second resolve enumerated this repo's `enums/target/` into a "2,489,366 files not checked" summary, paid a multi-second traversal on every gate run (including bca init and the --explain-threshold preview), duplicated every seed-resolution warning on stderr, and could exit 1 on a seed unlinked between the two resolves. Replace the transitive diff with prune-point measurement inside the one and only walk: the visitor now records every directory it enters, and `measure_ignored_entries` read_dirs each one, classifying the immediate children the walk did not keep. Hidden entries, symlinks, exclude globs, and unrecognized extensions explain their own drops; what remains is exactly the ignore rules' doing. An ignored file with a recognized extension is counted; an ignored directory becomes one "N ignored directories not walked" clause and is never entered, so the cost tracks the kept tree (2.2k read_dirs here, ~0 ms against the walk itself) rather than the no-ignore universe (407k directories). Only the gate's `resolve_walk_files_with_ignored` turns the measurement on; the single resolve also retires the duplicated warnings and the vanished-seed race outright. This repo's own gate now prints "38 files not checked (38 generated); 16 ignored directories not walked" in the walk's ordinary runtime. New tests pin the pruned-directory clause (a build/ tree with an offender inside must not inflate the file count), the non-analyzable-ignored-file silence, and exclude-owns-the-drop precedence. Refs #1055
The prune-point measurement tripped the dogfooded gate: bundle the walk's two membership sets behind `WalkedSets` and merge the hidden/symlink guards, taking `classify_dropped_child` from six arguments and six exits to five and four; drop the measurement branches from `expand_seed_paths`'s seed loop (walked dirs are retained unconditionally, the conditional folds into `bool::then`); and give `unchecked_summary` a `counted` pluralization helper. The remaining soft-tier entry — `classify_dropped_child` at the nargs hard limit — lands in the headroom baseline refresh, which is the band that baseline exists for. Refs #1055
clippy's obfuscated_if_else rejected the bool::then fold, and the restored if/else put expand_seed_paths one over its cognitive limit. Extracting the explicit-file-seed arm behind its own named, documented helper is the boundary a reader would draw anyway (#726's rules were already a self-contained comment block). Refs #1055
The audit's mutation run proved the pruned-directory fixture blind: with only one subdirectory present, a measurement that counted every directory child still reported 1 and passed. A walked src/ sibling makes the exact count discriminate; the mutant now fails. Also retire run_check_walk's --paths-from materialization, whose comment described the removed second seed expansion — with a single resolve, expand_seed_paths reads stdin exactly once itself (the stdin integration test pins it), and the stdin test's stale rationale is rewritten to match. Refs #1055
The multi-angle code review ran the summary against real trees and found the counts lying in both directions. Four verified fixes: - A generated marker in a file no parser owns (Cargo.lock opens with `@generated`) inflated the generated count on every Rust repo; the tally now applies the same analyzable rule as the ignored side, via a shared `WalkFilters::analyzable`. - Ignored *directories* fired on essentially every checkout (target/, node_modules/), contradicting "clean runs stay silent"; the pruned- directory clause and listing now appear only under `--report-skipped`, where the audit asked for them. - An ignored file named explicitly on the command line was analyzed (#726) yet still reported as "not checked": explicit seeds are now excluded from the measurement by canonical path, and overlapping seed spellings dedupe in the `./`-stripped space so one entry is never counted twice. - Every command paid for the walk's new directory channel; the sends are now gated on a `WalkFilters.measure_ignored` flag (which also retires expand_seed_paths' sixth parameter and its nargs suppression), so only the gate's measuring walk records directories. `run_check_walk` also reuses the sanctioned `run_walk_resolved` seam instead of re-spelling the exit-1 contract from crate-root internals. Windows parity: the hidden-entry mirror now honors the hidden file attribute, not just the dot prefix, so attribute-hidden files are not misreported as ignore-dropped on Windows. `counted` moves to format_util as the crate's one count-with-noun spelling. Refs #1055
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.
Summary
Fixes #1055 (already closed on the issue; this PR carries the change). Two attacker-controlled inputs silently removed files from the
bca checkgate: a generated-code marker (@generated,DO NOT EDIT,GENERATED CODE) in a file's head, and a.gitignorecommitted in the branch under test. Per the decision recorded on the issue — loud, not strict, plus a--strictprofile:bca: 2 files not checked (1 generated, 1 ignored) — pass --report-skipped to list themEmitted before the empty-input guard so the sole-ignored-input case names its cause above the exit-1 error. Clean runs stay silent; no exit-code changes. Spelled in the severity-free
bca:family (notbca: note:, which would reintroduce the style(cli): unify diagnostic prefixes across the binary #609 double prefix).--strict+ presence-only[check] strict: the untrusted-input gate profile, equivalent to--no-skip-generated --no-ignore, applied after the manifest merge and recorded in--print-effective-config. The oldexit_code(strict)parameter is renamedtieredto de-collide.--stricton the check page, extended generated/.gitignoresections in the commands index — with Japanese translations inpo/ja.po— plus CHANGELOG and regenerated man pages.How the measurement works (and why it changed twice)
The
ignorecrate's walker never yields ignored entries, so the ignored set must be derived. The first design (second no-ignore resolve, set diff) failed on real trees — on this repo it printed "2,489,366 files not checked" and paid a ~3.2M-entry traversal per gate run. The shipped design measures at the walk's prune points: the measuring walk records every directory it enters, and oneread_dirper walked directory classifies the immediate children the walk did not keep (hidden entries, symlinks, exclude globs, and unrecognized extensions explain their own drops; what remains is the ignore rules' doing).A follow-up multi-angle review then hardened the counts:
Cargo.lockmatches@generatedon every Rust repo and was inflating the generated tally).--report-skipped— ignored build trees exist in essentially every checkout, and a default that counted them would be permanent noise../-stripped space.WalkFilters.measure_ignoredgate), so no other command pays for the channel; Windows honors the hidden file attribute, matching the walker.This repo's own gate now prints
bca: 34 files not checked (34 generated) — …(34 analyzable test fixtures carrying markers) at the walk's ordinary runtime.Testing
tests/check/check_skipped_inputs.rs: one per row of the issue's reproduction tables (marker on line 1, buried in a license header,DO NOT EDIT/GENERATED CODE, committed.gitignore, sole-input exit-1 nuance, clean-run silence), the--strict/ manifest / constituent-flag matrix,--paths-fromfile and stdin forms, effective-config, and the count-honesty cases above. Unit tests cover the summary formatter and the presence-only manifest fold.cargo llvm-cov).make pre-commitgreen (BCA_GATE: pass), including the dogfooded self-scan at both tiers.Known follow-ups (pre-existing or deferred, called out in review)
apply_check_excludere-reads--paths-from, so a stdin (-) list combined with[check.exclude]re-anchoring reads an exhausted stream (pre-existing, fix(cli): bca diff --since subtree mis-pairs; [check.exclude] ignores --paths-from seeds #497 plumbing).read_dirpass is serial; andignore0.4.33'sIncrementalIgnore/build_matchersAPI could replace subtraction-based classification with positive attribution (which would also attribute which ignore source dropped an entry). Worth a follow-up issue.