feat: Add warn_checks config to downgrade checks to warnings#128
Merged
ayarotsky merged 3 commits intoayarotsky:mainfrom Mar 17, 2026
Merged
feat: Add warn_checks config to downgrade checks to warnings#128ayarotsky merged 3 commits intoayarotsky:mainfrom
ayarotsky merged 3 commits intoayarotsky:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Severity (Error/Warning) and per-violation severity plumbing, a new Changes
Sequence DiagramsequenceDiagram
participant Runner as CLI/Main
participant Config as Config (diesel-guard.toml)
participant Registry as Registry (checks)
participant Checker as Check (individual)
participant Violation as Violation
participant Formatter as OutputFormatter
Runner->>Config: load configuration
Runner->>Registry: execute checks on migration AST
Registry->>Checker: run check(node, config, ctx)
Checker-->>Registry: return violations
Registry->>Config: is_check_warning(check_name)?
alt check marked warning
Registry->>Violation: v.with_severity(rgba(255,165,0,0.5)) (Warning)
else
Registry->>Violation: v.with_severity(rgba(255,0,0,0.5)) (Error)
end
Registry-->>Formatter: pass violations list
Formatter->>Formatter: count errors and warnings
Formatter->>Runner: print report (errors, warnings)
alt errors > 0
Runner->>Runner: exit code 1
else
Runner->>Runner: exit code 0
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main.rs (1)
148-157: Consider single-pass counting for efficiency.The current implementation iterates through violations twice. For typical use cases this is fine, but a single pass would be cleaner.
♻️ Optional: Single-pass counting
- let total_errors: usize = results - .iter() - .flat_map(|(_, v)| v) - .filter(|v| v.severity == Severity::Error) - .count(); - let total_warnings: usize = results - .iter() - .flat_map(|(_, v)| v) - .filter(|v| v.severity == Severity::Warning) - .count(); + let (total_errors, total_warnings) = results + .iter() + .flat_map(|(_, v)| v) + .fold((0, 0), |(errors, warnings), v| match v.severity { + Severity::Error => (errors + 1, warnings), + Severity::Warning => (errors, warnings + 1), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 148 - 157, The two separate iterations over results to compute total_errors and total_warnings are inefficient; replace them with a single pass over results.iter().flat_map(|(_, v)| v) and in that loop increment counters for Severity::Error and Severity::Warning (update the variables total_errors and total_warnings accordingly). Locate the counting logic around the existing total_errors/total_warnings declarations and change it to a single iteration that matches on v.severity (or uses if/else) to increment the appropriate counters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/checks/mod.rs`:
- Around line 173-183: The current flat_map always overwrites each violation's
severity based solely on config.is_check_warning(check.name()), clobbering any
pre-set severities; change the mapping so that when a check is configured as a
warning you only downgrade violations that are currently Severity::Error,
leaving other severities untouched. Concretely, inside the map after
check.check(node, config, ctx).into_iter(), replace v.with_severity(severity)
with logic like: if config.is_check_warning(check.name()) && v.severity() ==
Severity::Error { v.with_severity(Severity::Warning) } else { v }, referencing
the check.name(), Severity::Error/Severity::Warning, v.severity(), and
with_severity methods.
In `@src/checks/truncate_table.rs`:
- Around line 45-76: The remediation block in TruncateTableCheck's message
currently embeds concrete SQL that assumes an integer "id" PK and a
"{table}_id_seq" sequence; update the format string in truncate_table.rs (the
message used by TruncateTableCheck) to replace the specific DELETE/ALTER
SEQUENCE examples with generic placeholders or wording such as "DELETE rows in
small batches using the table's primary key or a suitable indexed column" and
"reset any sequence/identity associated with your PK (if applicable)" so it
doesn't assume an id column or sequence name, and apply the same wording change
to the corresponding example text in docs/src/checks/truncating-table.md to keep
docs and code consistent.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 148-157: The two separate iterations over results to compute
total_errors and total_warnings are inefficient; replace them with a single pass
over results.iter().flat_map(|(_, v)| v) and in that loop increment counters for
Severity::Error and Severity::Warning (update the variables total_errors and
total_warnings accordingly). Locate the counting logic around the existing
total_errors/total_warnings declarations and change it to a single iteration
that matches on v.severity (or uses if/else) to increment the appropriate
counters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f173dd1a-fc7f-4cf7-b8af-2fc64a51d505
📒 Files selected for processing (10)
docs/src/checks/truncating-table.mddocs/src/configuration.mdsrc/checks/mod.rssrc/checks/truncate_table.rssrc/config.rssrc/main.rssrc/output.rssrc/safety_checker.rssrc/violation.rstests/output_test.rs
Comment on lines
+173
to
+183
| .flat_map(|check| { | ||
| let severity = if config.is_check_warning(check.name()) { | ||
| Severity::Warning | ||
| } else { | ||
| Severity::Error | ||
| }; | ||
| check | ||
| .check(node, config, ctx) | ||
| .into_iter() | ||
| .map(move |v| v.with_severity(severity)) | ||
| }) |
There was a problem hiding this comment.
Don't clobber pre-set violation severity here.
Line 182 rewrites every violation returned by the check, so any custom check or future built-in that deliberately emits Severity::Warning will get promoted back to Error unless the whole check is listed in warn_checks. The config should downgrade default errors, not overwrite an already-chosen severity.
🔧 Suggested fix
- let severity = if config.is_check_warning(check.name()) {
- Severity::Warning
- } else {
- Severity::Error
- };
+ let downgrade_to_warning = config.is_check_warning(check.name());
check
.check(node, config, ctx)
.into_iter()
- .map(move |v| v.with_severity(severity))
+ .map(move |v| {
+ if downgrade_to_warning && matches!(v.severity, Severity::Error) {
+ v.with_severity(Severity::Warning)
+ } else {
+ v
+ }
+ })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/checks/mod.rs` around lines 173 - 183, The current flat_map always
overwrites each violation's severity based solely on
config.is_check_warning(check.name()), clobbering any pre-set severities; change
the mapping so that when a check is configured as a warning you only downgrade
violations that are currently Severity::Error, leaving other severities
untouched. Concretely, inside the map after check.check(node, config,
ctx).into_iter(), replace v.with_severity(severity) with logic like: if
config.is_check_warning(check.name()) && v.severity() == Severity::Error {
v.with_severity(Severity::Warning) } else { v }, referencing the check.name(),
Severity::Error/Severity::Warning, v.severity(), and with_severity methods.
Adds a `warn_checks` config option that lets users treat specific checks as warnings (reported but non-blocking) instead of errors. Useful for checks like TruncateTableCheck that are context-dependent — fine for lookup tables, seeding, or staging environments. - New `Severity` enum on `Violation` (Error | Warning, default Error) - `Config::is_check_warning()` to query warn_checks list - Registry applies severity overrides at check time via `with_severity()` - Output: distinct icons/labels for warnings vs errors (⚠️ vs ❌) - Summary line handles all four combinations of error/warning counts - Exit code 0 when only warnings are present; 1 only for errors - Docs updated for TruncateTableCheck and configuration reference Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jonatas
force-pushed
the
feat/truncate-check-severity
branch
from
bf76fbd to
4e5cb28
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/src/checks/truncating-table.md (1)
31-44: SQL example assumesidcolumn andusers_id_seqsequence naming.This mirrors the issue flagged in
src/checks/truncate_table.rs. The example assumes:
- An
idcolumn as primary key (Lines 33-34)- A
users_id_seqsequence (Line 40)Consider using generic placeholders and noting that sequence naming varies:
-- Delete rows in small batches to allow concurrent access -DELETE FROM users WHERE id IN ( - SELECT id FROM users LIMIT 1000 +DELETE FROM users WHERE <primary_key> IN ( + SELECT <primary_key> FROM users + ORDER BY <primary_key> + LIMIT 1000 ); -- Repeat the batched DELETE until all rows are removed --- Optional: Reset sequences if needed -ALTER SEQUENCE users_id_seq RESTART WITH 1; +-- Optional: Reset the sequence/identity if your table uses one🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/checks/truncating-table.md` around lines 31 - 44, The SQL example uses concrete identifiers (users, id, users_id_seq) which replicate the same assumption in src/checks/truncate_table.rs; update the docs/src/checks/truncating-table.md to use generic placeholders like <TABLE_NAME> and <PRIMARY_KEY_COLUMN> and replace users_id_seq with a placeholder such as <PK_SEQUENCE> (or instruct readers to derive the sequence name via pg_get_serial_sequence/information_schema), and add a short note that sequence names vary by schema/DB and to show how to locate the correct sequence for the table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/src/checks/truncating-table.md`:
- Around line 31-44: The SQL example uses concrete identifiers (users, id,
users_id_seq) which replicate the same assumption in
src/checks/truncate_table.rs; update the docs/src/checks/truncating-table.md to
use generic placeholders like <TABLE_NAME> and <PRIMARY_KEY_COLUMN> and replace
users_id_seq with a placeholder such as <PK_SEQUENCE> (or instruct readers to
derive the sequence name via pg_get_serial_sequence/information_schema), and add
a short note that sequence names vary by schema/DB and to show how to locate the
correct sequence for the table.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 798040ab-f760-4ebc-bb83-9e9f55519ce0
📒 Files selected for processing (10)
docs/src/checks/truncating-table.mddocs/src/configuration.mdsrc/checks/mod.rssrc/checks/truncate_table.rssrc/config.rssrc/main.rssrc/output.rssrc/safety_checker.rssrc/violation.rstests/output_test.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/safety_checker.rs
- src/config.rs
- docs/src/configuration.md
- src/violation.rs
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #128 +/- ##
==========================================
- Coverage 94.18% 94.14% -0.05%
==========================================
Files 39 39
Lines 1221 1246 +25
==========================================
+ Hits 1150 1173 +23
- Misses 71 73 +2
🚀 New features to boost your workflow:
|
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
2 participants
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
warn_checksconfig option to downgrade specific checks from errors to warningsTruncateTableCheckin test/seed/maintenance contexts)TruncateTableCheckseverity handling and exit code logic in CLITest plan
warn_checks = ["TruncateTableCheck"]in config causes truncate violations to appear as warnings without failing CIwarn_checksstill produce errors with exit code 1cargo testpasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation