feat(core): apply [rules.<id>].severity overrides at engine layer - #278
Conversation
Documents the engine-layer contract for per-rule severity overrides: promote, demote, no-op default preservation, and unknown-id tolerance. Two cases (promote/demote) currently fail because the engine ignores the `severity` field on RuleOverride; the next commit fixes that. Refs: audit blocker B2 (severity override silent no-op). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-rule severity overrides in `plumb.toml` now actually remap emitted
violations. The remap happens after each rule's local emissions are
collected, before they merge into the engine's output buffer, so the
CLI's `exit_code_for` and every formatter see a single post-override
view.
Rules stay pure — they continue to emit with their `default_severity()`
and the engine owns the cross-cutting concern. Lookup is a single
IndexMap probe per rule (not per violation), preserving the existing
parallel evaluation shape and the byte-identical determinism contract.
The previous `// severity overrides are not yet applied at engine
level` comment is removed; the formatter never had a remap path either,
which is the audit blocker B2 ("severity override silent no-op")
quickstart users hit on step 5.
Cargo.lock is refreshed to match the workspace 0.0.11 versions.
Refs: audit blocker B2, finding H32.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The exit code table previously said "0 | No violations." but the actual logic in `commands::lint::exit_code_for` returns 0 when the only violations are `info`-severity. Document the carve-out and explain how to promote an advisory rule into the CI-failing tier with `[rules."<id>"] severity = "warning"`. Refs: audit finding H32 (undocumented info-only exit 0). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
I now have everything I need. Here is the full review: Plumb PR Review —
|
| Test | Covered |
|---|---|
warning → error promotion |
✓ |
warning → info demotion |
✓ |
| No override → default severity preserved | ✓ |
| Unknown rule id → no panic, no spurious emission, other rules unaffected | ✓ |
Minor gap: No test for the {enabled: false, severity: Some(Error)} combination. The filter at engine.rs:192 correctly drops disabled rules before the override block is reached, so the behavior is correct — but it's untested explicitly. Low risk; the filter predicate is already tested separately. Not a blocker.
5. Documentation
Accurate. The cli.md exit-code table now matches exit_code_for in lint.rs:345–365 exactly: Severity::Info takes the empty arm, falling through to ExitCode::SUCCESS when no errors or warnings are present. The table addition is consistent with the implementation.
No AI-tell phrases detected in the added prose. The humanizer rule requires a run on every docs/src/** change before merge; this PR has not had one. The prose appears clean but the gate should still close.
Punch list
| # | Location | Severity | Finding |
|---|---|---|---|
| 1 | engine.rs:198–200 |
Warning | Comment says "a single IndexMap probe per rule" but there are two — one at line 192 (filter, for enabled) and one at line 202 (flat_map, for severity). The comment's intent ("one probe regardless of violation count") is true and useful; the word "single" is inaccurate. Suggest: "One lookup per rule here — the enabled check above already paid for the other." or just drop the count claim. |
| 2 | engine.rs:192, 202 |
Info | Both closures call config.rules.get(rule.id()). For current rule counts this is noise, but the double-lookup is directly caused by splitting the enable check and the severity remap into separate pipeline stages. Combining them into one flat_map (skipping rules with enabled=false there) would let you cache the Option<&RuleOverride> in a local. Not blocking today. |
| 3 | docs/src/cli.md |
Warning | Per documentation.md, humanizer must run on every docs/src/** PR before merge. No evidence it ran here. The prose looks clean, but the gate must close explicitly. |
| 4 | tests/severity_override_engine.rs |
Info | No test for enabled=false + severity=Some(...). The code path is correct (filter drops the rule first), but an explicit test would document the priority contract (disable wins over override). Low priority. |
Items 1 and 3 are the substantive ones. Item 1 is a documentation accuracy issue in a correctness-sensitive file; item 3 is a process gate that the project rules declare mandatory. Neither is a runtime bug or an invariant violation, and both are cheap to fix.
Verdict: APPROVE
Summary
[rules."<id>"] severity = "error"now actually remap emitted violations.run_rules's per-rule emission loop, before the buffer merge. Engine output stays a pure function of(snapshot, config); determinism guarantee preserved.docs/src/cli.md's exit-code table.Cargo.lockfrom stale 0.0.6 internal crate versions to current 0.0.11.Why
The public-surface audit found this was the first customization a new user is told to perform (quickstart step 5) and that it silently no-ops. The engine source already admitted the gap in a comment.
Test plan
crates/plumb-core/tests/severity_override_engine.rs— 4 cases (warn→error, warn→info, no override, unknown id).cargo test --workspace --all-featurespasses.cargo fmt --check,cargo clippy -D warnings,cargo deny checkclean.just determinism-checkbyte-identical across 3 runs.plumb init && cat >> plumb.toml ... && plumb lint plumb-fake://helloexits 1.Closes audit blocker B2 + finding H32.
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com