refactor(diag): unify warning prefixes onto the warn() severity ladder

[dekobon]

Status: fixed on fix/1199-unify-warning-prefixes (315efcbb),
PR pending. All nine plan steps done. Two corrections to the analysis
below, recorded here so the body is not read as current:

• The library has five capitalised sites, not three. The sweep
  also found src/output/code_climate.rs:94 (empty repo-relative
  path) and src/concurrent_files.rs:227 (not a regular file).
• Step 3 is answered by a library-side src/diag.rs carrying a
  single pub(crate) fn warn, the counterpart to the CLI ladder,
  in addition to bringing CSV onto warn_non_utf8_path. The helper
  alone covers only the non-UTF-8-path warnings.

Steps 1-2 are byte-for-byte a no-op as predicted. The one deliberate
output change is the IncludeCycle trailing newline, dropped per
step 5. See the closing comment for the evidence.

Problem

Diagnostic severity prefixes are produced in three different places
with two different conventions, and the library has no access to the
CLI helper that defines the house style.

big-code-analysis-cli/src/diag.rs:24 is the intended shape — the
three-severity ladder added in #609:

pub(crate) fn warn(msg: impl Display) {
    eprintln!("warning: {msg}");
}

Two library surfaces bypass it and bake a capitalised prefix into the
message instead:

• src/output/csv.rs:194 and :229 — "Warning: skipping non-UTF-8 source path in CSV output: {}"
• src/output/offenders.rs:35 — "Warning: skipping non-UTF-8 path in {format} output: {}"

A third, PreprocDiagnostic's Display impl (src/preproc.rs:76-108),
also self-prefixes. Its five variants were internally inconsistent —
three capitalised, two not — which #1198 fixed by lowercasing all five.
That was chosen deliberately as a stepping stone to this issue rather
than as the end state (see below).

bca preproc surfaces the diagnostics with a bare eprintln!, not
through warn():

// big-code-analysis-cli/src/commands/preproc.rs:82-84
for diagnostic in fix_includes(&mut data.files, &all_files) {
    eprintln!("{diagnostic}");
}

Proposed change

Move the severity prefix out of the message producers and onto the
presentation layer, so warn() is the single place warning: is
written.

1. Drop the warning: prefix from all five PreprocDiagnostic
   variants, leaving Display as the bare message.
2. Route commands/preproc.rs:83 through diag::warn.
3. Decide the same question for output/csv.rs and
   output/offenders.rs. These live in the library and cannot call the
   CLI's pub(crate) helper, so they need either their own convention
   or a library-side equivalent — worth settling explicitly rather than
   leaving two capitalised stragglers.

Why step 1 and 2 are a no-op on output

warn(msg) expands to eprintln!("warning: {msg}"). Because #1198
lowercased the prefix first, eprintln!("{diagnostic}") on a
Display that yields warning: possible self inclusion foo.h and
warn("possible self inclusion foo.h") emit the same bytes. So steps
1-2 should be verifiable as a pure refactor with no user-visible
diff — which is the whole reason the flip went downward.

preproc_diagnostic_display_renders_each_single_line_variant and
preproc_diagnostic_display_lists_every_cycle_member
(src/preproc_tests.rs) pin the current text and are the check that it
stayed a no-op; both will need their expectations moved to the CLI
layer as part of the change.

Wrinkles

• IncludeCycle is multi-line. warn() prefixes only the first
  line, which happens to be the shape wanted here (header prefixed,
  member lines indented under it) — but confirm rather than assume.
• IncludeCycle ends with a trailing newline and eprintln! adds
  another, so each cycle block currently prints followed by a blank
  line. Pre-existing and now pinned by the test above. This is the
  natural place to decide whether that blank line is wanted.

Not a breaking change

STABILITY.md:158 — "The Display impls are stable; the exact wording
of Display output is not." PreprocDiagnostic is a public export
(src/lib.rs:340), so this clause is what clears the change.

---

Resolution Plan

Confirmed against main (39674df6). All five PreprocDiagnostic
variants are lowercase (src/preproc.rs:80, :83, :95, :100,
:104) per #1198; the two capitalised stragglers are live at
src/output/csv.rs:194, :229 and src/output/offenders.rs:35; and
big-code-analysis-cli/src/commands/preproc.rs:83 still prints with a
bare eprintln!.

Step 3 already has an answer in the tree

The issue leaves the library-side convention open. It is largely settled
already: offenders.rs:30 defines warn_non_utf8_path(format, path),
and five output formats route through it — SARIF (sarif.rs:230),
Code Climate (code_climate.rs:89), Checkstyle (checkstyle.rs:52) and
both warning-line flavours (warning_line.rs:58, :91). CSV is the only
format that does not; it open-codes the same warning twice with a
different wording. So the library does not need a new convention, it
needs CSV brought onto the existing one.

That also settles the more principled alternative — propagating the
diagnostic to the caller so the CLI can route it through warn().
write_csv and write_csv_aggregate (src/output/csv.rs:185, :217)
are pub, so changing their signatures is a SemVer break and cannot
land in the 2.x line (STABILITY.md, AGENTS.md). Not worth deferring the
consistency fix to a major bump for; note it as the 3.0 option and move on.

1. Drop the warning: prefix from all five PreprocDiagnostic
   variants, leaving Display as the bare message.
2. Route commands/preproc.rs:83 through diag::warn.
3. Move the two PreprocDiagnostic Display tests
   (preproc_diagnostic_display_renders_each_single_line_variant,
   preproc_diagnostic_display_lists_every_cycle_member, in
   src/preproc_tests.rs) to assert the bare message, and add a CLI-side
   test asserting the rendered stderr still reads warning: …. Both
   halves are needed: the library test alone stops guarding the prefix,
   and the CLI test alone stops guarding the message.
4. Prove steps 1-3 are a no-op on output. warn(msg) expands to
   eprintln!("warning: {msg}"), so post-fix: batch of fifteen issues (#1180-#1182, #1184-#1192, #1194-#1196) #1198 the bytes should be
   identical. Capture bca preproc stderr on a fixture producing each of
   the five variants before and after and diff them. This is the whole
   justification for fix: batch of fifteen issues (#1180-#1182, #1184-#1192, #1194-#1196) #1198 having flipped downward first; if the diff is
   non-empty, something in the wrinkles below is wrong.
5. Settle the two IncludeCycle wrinkles the issue flags, rather than
   inheriting them. warn() prefixes only the first line, which is the
   shape wanted (header prefixed, members indented under it) — confirm on
   real output. The trailing newline plus eprintln!'s own produces a
   blank line after each cycle block; that is pre-existing and now pinned
   by the test above, so this is the moment to decide whether it stays.
   Recommend dropping it: it is an artifact of writeln! in a Display
   impl, not a deliberate separator, and Display impls should not end in
   a newline. If it stays, say why in a comment so the next reader does
   not "fix" it.
6. Bring CSV onto warn_non_utf8_path. Replace both csv.rs sites
   with warn_non_utf8_path("CSV", source_path), which lowercases the
   prefix and removes the duplicated wording as a side effect. Check the
   wording change is acceptable: the shared helper says "skipping
   non-UTF-8 path in CSV output", the current CSV text says "non-UTF-8
   source path". If that distinction matters, widen the helper rather
   than keeping a second copy.
7. Lowercase the remaining prefix in warn_non_utf8_path itself
   (offenders.rs:35) — after step 6 this is the library's single site,
   and it fixes all six formats at once.
8. Add a guard so a seventh site cannot reappear. The failure mode
   here is that a new eprintln!("Warning: …") reads as correct in
   review. A rg-based check in utils/ wired into make lint
   (asserting no Warning: / Error: capitalised literal outside test
   modules) is a few lines and follows the existing gate conventions,
   including a *-test.py self-test. Without it this issue is a cleanup
   that will need doing again.
9. Changelog. ## [Unreleased] → Changed, noting the CSV
   non-UTF-8 warning's wording and casing changed. Not breaking:
   STABILITY.md:158 — "The Display impls are stable; the exact
   wording of Display output is not" — covers PreprocDiagnostic,
   which is a public export (src/lib.rs:340).

Assessment

Dimension	Rating
Difficulty	Low
Complexity	Low
Priority	Low

Difficulty — Low. Five string edits, one call-site swap, two CSV
sites redirected to an existing helper. The optional lint gate in step 8
is the only piece with any substance, and it is a few lines of rg.

Complexity — Low. Two crates, but the seam between them is one
function call. No public-API change, no metric computation, no snapshots.
The one thing to get right is that the test expectations move across the
crate boundary rather than being deleted on one side.

Priority — Low. Purely presentational consistency in diagnostic
output. #1198 already removed the user-visible inconsistency within
PreprocDiagnostic; what remains is two capitalised stragglers and a
structural tidy-up so the prefix is written in one place per crate. No
correctness impact and no workaround needed.

Labelled low-priority.

[dekobon]

Added resolution plan and assessment ratings (difficulty Low / complexity Low / priority Low), and applied low-priority.

Confirmed all five sites on main (39674df6).

The open question in step 3 turns out to be mostly answered in the tree already. src/output/offenders.rs:30 defines warn_non_utf8_path(format, path), and five output formats route through it — SARIF, Code Climate, Checkstyle, and both warning-line flavours. CSV is the only one that does not; it open-codes the same warning twice with slightly different wording. So the library does not need a new convention, it needs CSV brought onto the existing one, after which lowercasing the single site in warn_non_utf8_path fixes all six formats at once.

That also rules out the more principled alternative for now: write_csv and write_csv_aggregate are pub, so propagating the diagnostic to the caller instead of printing is a SemVer break and cannot land in 2.x.

One addition — step 8 proposes an rg-based make lint check for capitalised Warning: / Error: literals. Without it, a seventh site reads as correct in review and this becomes a cleanup that needs doing again.

[dekobon]

Fixed on fix/1199-unify-warning-prefixes (315efcbb), PR to follow.
make pre-commit green.

What the plan got right, and the one thing it missed

Step 3's answer held: CSV moved onto warn_non_utf8_path, and
lowercasing that one site fixed all six formats. write_csv /
write_csv_aggregate stayed pub and unchanged, so nothing was
deferred to 3.0.

The count was wrong, though. The issue names three library sites; a
workspace sweep for capitalised severity literals found five, in
two spellings the issue does not mention:

• src/output/code_climate.rs:94 — "Warning: skipping empty repo-relative path in code-climate output"
• src/concurrent_files.rs:227 — "Warning: not a regular file, skipping"

Both are lowercase now. This is exactly the failure mode step 8
predicted, already realised, which is a good argument for the gate.

Library-side convention

The library got its own src/diag.rs with a single pub(crate) fn warn, the counterpart to the CLI ladder from #609. Every library
stderr warning routes through it — including the two that were already
lowercase (src/spaces/compute.rs, src/vcs/git/cached.rs), which
were correct by coincidence rather than by construction. No die /
error: counterpart: the library never exits, so fatal conditions stay
MetricsError / io::Error and the CLI decides how to present them.

Step 4: the no-op check, and where it is not a no-op

Captured bca preproc stderr on a fixture producing SelfInclusion
and IncludeCycle, before and after. The diff is exactly one line —
the blank line after the cycle block — and nothing else. So the prefix
move is byte-identical, as #1198 intended.

Step 5: both IncludeCycle wrinkles

• warn prefixes the header line only, members indented beneath.
  Confirmed on real output, not assumed.
• The trailing newline is gone, per the plan's recommendation. It
  was an artifact of writeln! in a Display impl, and it stacked with
  the caller's own newline. The member separator now leads
  (write!(f, "\n - \"{member}\"")) so the block carries no trailing
  newline of its own.

Step 8: the gate

utils/check-diagnostic-prefix.py + -test.py, wired into
make lint / pre-commit / ci, the pre-commit hooks, and an
explicit CI step. It scans every tracked .rs for a string literal
starting with Warning: / Error: / Note: (plus the all-caps
spellings), skipping whole-line comments.

It found a legitimate hit immediately:
check_thresholds.rs:372 asserts the capitalised spelling is absent
from bca check stderr — the #609 regression guard. That is data, not
a diagnostic, so the gate carries a // diag-prefix-ok opt-out marker
honoured on the line itself or anywhere in the comment block above it.
One site uses it today.

Two things the self-tests pin that a naive gate gets wrong:

• The literal is usually on its next line, not the macro's — every
  offender here was wrapped that way. A same-line matcher would have
  reported the whole tree clean.
• str.splitlines() breaks on U+2028 and the vertical-tab family,
  which rustc does not, so one inside a literal shifts every reported
  line number after it. The gate uses split("\n"); the test fails
  against splitlines() (line 3 vs 2).

Tests

The two Display tests moved to the bare message, and a CLI-side
preproc_diagnostics_render_under_the_warning_prefix pins the rendered
stderr. Both halves verified by perturbing production, not by reading:
reverting the CLI call site to eprintln! trips the prefix assertion,
restoring the writeln! trips the blank-line assertion, independently.

Also added aggregate_skips_only_the_non_utf8_file — the aggregate
CSV skip branch had no test, and a single-file document that skips its
only file is byte-identical to one that bails outright, so the existing
test could not have covered it. Turning the continue into a return
fails the new one.

non_utf8_path_skips_data_rows had its #[cfg(unix)] moved from an
inner block onto the fn: the old form compiled to an empty — and
therefore passing — test body on Windows
(.claude/rules/testing.md).

Baseline

write_code_climate's halstead.effort moved +0.2% (60471.78 →
60596.14) past its recorded value, so .bca-baseline.toml is refreshed
in the same commit. The refresh also dropped three entries that had
gone stale on main.