feat(cli): Allow to set a minimum level for autofixes #7778
Conversation
🦋 Changeset detectedLatest commit: 984f4a8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR threads a configurable severity threshold through the fix pipeline and makes fix-all operations respect that threshold. A new optional Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
github-actions
bot
added
A-CLI
vhfmag
changed the title
--fix respect --diagnostic-level--fix respect --diagnostic-level
vhfmag
changed the title
--fix respect --diagnostic-level--fix respect --diagnostic-level
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_service/src/file_handlers/mod.rs (1)
753-771: Fix fallback in get_diagnostic_severity to avoid misclassifying errorsWhen rules are None or provide no override, the function falls back to Warning, ignoring the diagnostic’s own severity. Elsewhere (e.g. ProcessLint) the fallback is diagnostic.severity(). This can wrongly treat error-level lints as non-errors and break both error counting and the new severity gating.
Proposed fix: fall back to diagnostic.severity().
pub(crate) fn get_diagnostic_severity( diagnostic: &'_ AnalyzerDiagnostic, rules: Option<&'_ Rules>, ) -> Severity { diagnostic .category() .filter(|category| category.name().starts_with("lint/")) .map_or_else( - || diagnostic.severity(), + || diagnostic.severity(), |category| { rules .and_then(|rules| { rules.get_severity_from_category(category, diagnostic.severity()) }) - .unwrap_or(Severity::Warning) + // Fall back to the diagnostic’s own severity when no override exists. + .unwrap_or(diagnostic.severity()) }, ) }Please add a small unit test asserting that:
- a lint diagnostic with severity Error remains Error when no rules are provided; and
- a rules override changes the effective severity as expected.
🧹 Nitpick comments (4)
crates/biome_cli/tests/commands/check.rs (2)
57-78: Test fixtures look good; include a warn‑level case or rename the “warn” test.You exercise info+error nicely. The test named diagnostic_level_warn_fixes_warn_and_error_levels doesn’t include any warn‑level rule, so it only proves error is fixed. Either add a warn‑severity rule with a fix, or rename the test to reflect what it verifies.
3279-3418: Strengthen assertions and align the “warn” test name.
- Also assert that “==” is gone to avoid false positives.
- Consider renaming the “warn” test (or add a warn‑level rule as noted above).
Apply this minimal assertion diff:
@@ - // Verify that == was changed to === (error-level fix applied) - assert!(buffer.contains("x === 1"), "Error-level fix should be applied: == -> ==="); + // Verify that == was changed to === (error-level fix applied) + assert!(buffer.contains("x === 1"), "Error-level fix should be applied: == -> ==="); + assert!(!buffer.contains("x == 1"), "Legacy '==' should be removed"); @@ - // Verify that == was changed to === (error-level fix applied) - assert!(buffer.contains("x === 1"), "Error-level fix should be applied: == -> ==="); + // Verify that == was changed to === (error-level fix applied) + assert!(buffer.contains("x === 1"), "Error-level fix should be applied: == -> ==="); + assert!(!buffer.contains("x == 1"), "Legacy '==' should be removed");crates/biome_lsp/src/handlers/analysis.rs (1)
355-356: LSP fix‑all currently fixes all severities; consider making this configurable.Passing
diagnostic_level: Nonepreserves existing behaviour. Consider threading a server/client setting (e.g., VS Code “Fix minimum severity”) so users can align editor “fix on save” with CLI.crates/biome_cli/src/execute/traverse.rs (1)
87-88: Update doc strings to reflect both printing and fixing usesThe plumbing is correct—
diagnostic_levelthreads properly into both theDiagnosticsPrinter(line 61) and theFixFileParamspassed to file handlers (lines 657–660, 896–899, etc.). However, the three doc strings say only "minimum diagnostic level to fix" when the field also filters printed diagnostics.Update lines 451–453 and also the corresponding doc in
crates/biome_service/src/file_handlers/mod.rs(line 418) andcrates/biome_service/src/workspace.rs(line 1031) to read: "Minimum diagnostic level to print and fix".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
crates/biome_cli/tests/snapshots/main_commands_check/diagnostic_level_error_only_fixes_error_level.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_check/diagnostic_level_info_fixes_all_levels.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_check/diagnostic_level_warn_fixes_warn_and_error_levels.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_check/diagnostic_level_without_fix_respects_level.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_lint/lint_diagnostic_level_error_only_fixes_error_level.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_lint/lint_diagnostic_level_info_fixes_all_levels.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_lint/lint_diagnostic_level_without_fix_respects_level.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (15)
.changeset/busy-olives-hang.md(1 hunks)crates/biome_cli/src/execute/process_file/lint_and_assist.rs(1 hunks)crates/biome_cli/src/execute/std_in.rs(1 hunks)crates/biome_cli/src/execute/traverse.rs(2 hunks)crates/biome_cli/tests/commands/check.rs(2 hunks)crates/biome_cli/tests/commands/lint.rs(2 hunks)crates/biome_lsp/src/handlers/analysis.rs(1 hunks)crates/biome_service/src/file_handlers/css.rs(2 hunks)crates/biome_service/src/file_handlers/graphql.rs(2 hunks)crates/biome_service/src/file_handlers/javascript.rs(2 hunks)crates/biome_service/src/file_handlers/json.rs(2 hunks)crates/biome_service/src/file_handlers/mod.rs(3 hunks)crates/biome_service/src/workspace.rs(3 hunks)crates/biome_service/src/workspace.tests.rs(1 hunks)crates/biome_service/src/workspace/server.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_cli/src/execute/process_file/lint_and_assist.rscrates/biome_service/src/workspace/server.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_cli/tests/commands/check.rscrates/biome_service/src/workspace.rscrates/biome_lsp/src/handlers/analysis.rscrates/biome_cli/tests/commands/lint.rscrates/biome_service/src/workspace.tests.rscrates/biome_service/src/file_handlers/json.rscrates/biome_service/src/file_handlers/graphql.rscrates/biome_service/src/file_handlers/css.rscrates/biome_cli/src/execute/std_in.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_cli/src/execute/traverse.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_cli/src/execute/process_file/lint_and_assist.rscrates/biome_service/src/workspace/server.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_cli/tests/commands/check.rscrates/biome_service/src/workspace.rscrates/biome_lsp/src/handlers/analysis.rscrates/biome_cli/tests/commands/lint.rscrates/biome_service/src/workspace.tests.rscrates/biome_service/src/file_handlers/json.rscrates/biome_service/src/file_handlers/graphql.rscrates/biome_service/src/file_handlers/css.rscrates/biome_cli/src/execute/std_in.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_cli/src/execute/traverse.rs
crates/biome_service/src/workspace/server.rs
📄 CodeRabbit inference engine (crates/biome_service/CONTRIBUTING.md)
WorkspaceServer must maintain workspace state and is used in the daemon and CLI daemonless mode
Files:
crates/biome_service/src/workspace/server.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_cli/tests/commands/check.rscrates/biome_cli/tests/commands/lint.rs
crates/biome_service/src/workspace.rs
📄 CodeRabbit inference engine (crates/biome_service/CONTRIBUTING.md)
Implement and expose the Workspace trait in src/workspace.rs
Files:
crates/biome_service/src/workspace.rs
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: In changesets, only use #### or ##### headers; other header levels are not allowed
Changesets should cover user-facing changes only; internal changes do not need changesets
Use past tense for what you did and present tense for current Biome behavior in changesets
When fixing a bug in a changeset, start with an issue link (e.g., “Fixed #1234: …”)
When referencing a rule or assist in a changeset, include a link to its page on the website
Include code blocks in changesets when applicable to illustrate changes
End every sentence in a changeset with a period
Files:
.changeset/busy-olives-hang.md
🧠 Learnings (9)
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/lib/src/{lint,assist}/**/*.rs : Avoid unwrap/expect in CST navigation; use Result/Option propagation (e.g., ok()? and combinators like map/and_then)
Applied to files:
crates/biome_cli/src/execute/process_file/lint_and_assist.rs
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/lib/src/{lint,assist}/**/*.rs : If a rule provides a code action (implements action), add fix_kind to declare_lint_rule! (FixKind::Safe or FixKind::Unsafe)
Applied to files:
crates/biome_cli/src/execute/process_file/lint_and_assist.rscrates/biome_service/src/file_handlers/javascript.rs
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/lib/src/{lint,assist}/**/*.rs : Choose rule severity appropriately (error/warn/info); set severity in declare_lint_rule! only when deviating from default
Applied to files:
crates/biome_cli/src/execute/process_file/lint_and_assist.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_service/src/file_handlers/css.rscrates/biome_service/src/file_handlers/mod.rs
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/lib/src/{lint,assist}/**/*.rs : Use declare_lint_rule! with version set to "next" for every rule definition
Applied to files:
crates/biome_cli/src/execute/process_file/lint_and_assist.rs
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/lib/src/{lint,assist}/**/*.rs : When documenting multi-file examples, use file=<path> in code block properties; keep modifier order consistent: language[,expect_diagnostic][,(options|full_options|use_options)][,ignore][,file=path]
Applied to files:
crates/biome_cli/src/execute/process_file/lint_and_assist.rs
📚 Learning: 2025-10-15T09:21:24.116Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_diagnostics/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:21:24.116Z
Learning: Prefer implementing the Diagnostic trait via `#[derive(Diagnostic)]`
Applied to files:
crates/biome_service/src/workspace/server.rs
📚 Learning: 2025-10-15T09:21:24.116Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_diagnostics/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:21:24.116Z
Learning: Specify static properties (category, severity, description, message, location, tags) with `#[diagnostic(...)]`
Applied to files:
crates/biome_service/src/workspace/server.rscrates/biome_service/src/workspace.rs
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/lib/src/{lint,assist}/**/*.rs : Mark code blocks in documentation with a language and required properties (e.g., expect_diagnostic for invalid snippets; options/full_options/use_options as needed). Each invalid snippet must emit exactly one diagnostic
Applied to files:
crates/biome_cli/tests/commands/lint.rs
📚 Learning: 2025-10-15T09:25:05.698Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:25:05.698Z
Learning: Applies to crates/biome_service/src/workspace/watcher.tests.rs : Place watcher tests related to workspace methods in src/workspace/watcher.tests.rs
Applied to files:
crates/biome_service/src/workspace.tests.rs
🧬 Code graph analysis (10)
crates/biome_service/src/workspace/server.rs (1)
crates/biome_cli/tests/cases/diagnostics.rs (1)
diagnostic_level(170-226)
crates/biome_service/src/file_handlers/javascript.rs (1)
crates/biome_service/src/file_handlers/mod.rs (2)
get_diagnostic_severity(754-771)is_diagnostic_error(745-751)
crates/biome_cli/tests/commands/check.rs (3)
crates/biome_fs/src/fs/memory.rs (1)
default(37-49)crates/biome_cli/tests/main.rs (1)
run_cli(332-347)crates/biome_cli/tests/snap_test.rs (1)
assert_cli_snapshot(407-409)
crates/biome_service/src/workspace.rs (2)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
Severity(9269-9269)crates/biome_cli/tests/cases/diagnostics.rs (1)
diagnostic_level(170-226)
crates/biome_cli/tests/commands/lint.rs (3)
crates/biome_fs/src/fs/memory.rs (1)
default(37-49)crates/biome_cli/tests/main.rs (1)
run_cli(332-347)crates/biome_cli/tests/snap_test.rs (1)
assert_cli_snapshot(407-409)
crates/biome_service/src/workspace.tests.rs (2)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (5)
Severity(9269-9269)FixFileMode(9759-9762)FixFileParams(9742-9755)OpenFileParams(9420-9431)BiomePath(9-9)crates/biome_service/src/workspace.rs (2)
open(1571-1580)from_client(772-777)
crates/biome_service/src/file_handlers/json.rs (1)
crates/biome_service/src/file_handlers/mod.rs (2)
get_diagnostic_severity(754-771)is_diagnostic_error(745-751)
crates/biome_service/src/file_handlers/graphql.rs (1)
crates/biome_service/src/file_handlers/mod.rs (2)
get_diagnostic_severity(754-771)is_diagnostic_error(745-751)
crates/biome_service/src/file_handlers/css.rs (1)
crates/biome_service/src/file_handlers/mod.rs (2)
get_diagnostic_severity(754-771)is_diagnostic_error(745-751)
crates/biome_service/src/file_handlers/mod.rs (2)
crates/biome_service/src/diagnostics.rs (5)
severity(353-355)severity(395-397)severity(495-497)severity(548-550)severity(638-640)crates/biome_analyze/src/signals.rs (3)
diagnostic(22-22)diagnostic(81-85)diagnostic(356-381)
🔇 Additional comments (14)
crates/biome_cli/tests/commands/check.rs (1)
3420-3459: LGTM: negative case (no --fix) respects level and leaves file intact.crates/biome_cli/src/execute/process_file/lint_and_assist.rs (1)
75-76: Threading diagnostic_level into fix_file is correct.This makes CLI fixes honour the severity threshold.
Please confirm the default value (when the flag isn’t passed) remains equivalent to “fix all” (i.e., Information or None), so behaviour is unchanged by default.
crates/biome_cli/src/execute/std_in.rs (1)
175-176: STDIN path now respects diagnostic_level during fixes.Looks consistent with the file‑system path.
Double‑check the default maps to “fix all” so existing workflows aren’t tightened unintentionally.
crates/biome_service/src/file_handlers/css.rs (1)
650-662: Severity gate is correctly applied before counting and collecting fixes.
- Uses get_diagnostic_severity with rule overrides; good.
- Skips sub‑threshold diagnostics and only decrements error counts for applied fixes.
Also applies to: 660-662
crates/biome_service/src/workspace/server.rs (1)
1511-1523: Server threads diagnostic_level through FixFileParams→FixAllParams.Plumbing looks tidy and scoped. Nice one.
Also applies to: 1562-1563
crates/biome_service/src/file_handlers/graphql.rs (1)
555-567: Diagnostic-level gate looks solidEarly-continue cleanly skips low-severity diagnostics and their actions; error counting still reflects only considered diagnostics. Nice gate—bouncer doing its job.
crates/biome_service/src/file_handlers/json.rs (1)
655-667: LGTM: fixes now respect --diagnostic-levelSub-threshold diagnostics are skipped before actions; this prevents unintended autofixes for warnings/info. Mirrors other handlers consistently.
crates/biome_service/src/file_handlers/javascript.rs (1)
894-907: Severity filter applied before actions — goodThis prevents applying fixes for warnings/info when a stricter level is set. Consistent with JSON/GraphQL handlers.
crates/biome_cli/tests/commands/lint.rs (4)
55-76: LGTM! Test data properly sets up diagnostic level scenarios.The test constants clearly define error-level and info-level violations, enabling verification that
--diagnostic-levelgates fixes appropriately.
413-458: LGTM with same--unsafeconsideration.Test correctly verifies that
--diagnostic-level=infoapplies fixes at all severity levels. The same question about--unsafeflag necessity applies here (see previous comment on lines 366-411).
460-499: LGTM! Comprehensive coverage of diagnostic level without fix mode.Test properly verifies that
--diagnostic-level=errorfilters diagnostics appropriately in reporting-only mode (without--fix), and that the file remains unchanged whilst still returning an error due to the presence of error-level violations.
366-411: The--unsafeflag is necessary and correctly applied.The
noDoubleEqualsrule declaresfix_kind: FixKind::Unsafein its implementation (line 81 ofcrates/biome_js_analyze/src/lint/suspicious/no_double_equals.rs). The--unsafeflag is required to enable application of fixes marked with this safety category, not included defensively. The test properly validates that--diagnostic-level=errorfilters diagnostics by severity whilst--unsafeallows unsafe-categorised fixes to be applied—the assertions confirm==converts to===and that info-level fixes remain unapplied.crates/biome_service/src/workspace.rs (2)
1031-1033: LGTM! Properly optional field with clear semantics.The
diagnostic_levelfield is correctly declared as optional with#[serde(default)]for backward compatibility. The comment clearly explains its purpose as a threshold for determining which diagnostics should be fixed.
1708-1730: LGTM! Signature correctly updated and parameter properly threaded.The new
diagnostic_levelparameter is correctly added to the method signature (line 1716) and properly passed through toFixFileParams(line 1728). The change integrates cleanly with the existing code.
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Fixed #7772: `--fix` now respects `--diagnostic-level` |
There was a problem hiding this comment.
Add the missing full stop.
Guidelines require sentences in changesets to end with a period. Please punctuate the single line.
Apply this diff:
-Fixed #7772: `--fix` now respects `--diagnostic-level`
+Fixed #7772: `--fix` now respects `--diagnostic-level`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Fixed #7772: `--fix` now respects `--diagnostic-level` | |
| Fixed #7772: `--fix` now respects `--diagnostic-level`. |
🤖 Prompt for AI Agents
.changeset/busy-olives-hang.md around line 5: the changeset sentence "Fixed
#7772: `--fix` now respects `--diagnostic-level`" is missing a terminal period;
update that line to end with a period so it reads "Fixed #7772: `--fix` now
respects `--diagnostic-level`."
There was a problem hiding this comment.
This is outdated, I can fix after the discussion and PR get feedback
| fn diagnostic_level_respects_fix_filtering() { | ||
| use biome_analyze::RuleCategoriesBuilder; | ||
| use biome_diagnostics::Severity; | ||
| use crate::workspace::{FixFileMode, FixFileParams}; | ||
|
|
||
| let (workspace, project_key) = create_server(); | ||
|
|
||
| // Create a TypeScript file with violations at different severity levels | ||
| let file_content = r#"let x: number | undefined = undefined; | ||
| if (x == 1) { | ||
| console.log(x); | ||
| } | ||
| "#; | ||
|
|
||
| let config_content = r#"{ | ||
| "linter": { | ||
| "enabled": true, | ||
| "rules": { | ||
| "recommended": false, | ||
| "complexity": { | ||
| "noUselessUndefinedInitialization": "info" | ||
| }, | ||
| "suspicious": { | ||
| "noDoubleEquals": "error" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| "#; | ||
|
|
||
| // Set up configuration | ||
| workspace | ||
| .update_settings(UpdateSettingsParams { | ||
| project_key, | ||
| configuration: serde_json::from_str(config_content).unwrap(), | ||
| workspace_directory: None, | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| let _file_guard = FileGuard::open( | ||
| workspace.as_ref(), | ||
| OpenFileParams { | ||
| project_key, | ||
| path: BiomePath::new("test.ts"), | ||
| content: FileContent::from_client(file_content), | ||
| document_file_source: None, | ||
| persist_node_cache: false, | ||
| }, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let rule_categories = RuleCategoriesBuilder::default().with_lint().build(); | ||
|
|
||
| // Test 1: With diagnostic_level=error, should only fix error-level violations | ||
| let result_error_level = workspace | ||
| .fix_file(FixFileParams { | ||
| project_key, | ||
| path: BiomePath::new("test.ts"), | ||
| fix_file_mode: FixFileMode::SafeAndUnsafeFixes, | ||
| should_format: false, | ||
| only: vec![], | ||
| skip: vec![], | ||
| enabled_rules: vec![], | ||
| rule_categories, | ||
| suppression_reason: None, | ||
| diagnostic_level: Some(Severity::Error), | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| // Should only fix the error-level violation (noDoubleEquals), not the info-level one | ||
| let expected_error_only = r#"let x: number | undefined = undefined; | ||
| if (x === 1) { | ||
| console.log(x); | ||
| } | ||
| "#; | ||
| assert_eq!(result_error_level.code, expected_error_only); | ||
|
|
||
| // Test 2: With diagnostic_level=information, should fix both info and error level violations | ||
| let result_info_level = workspace | ||
| .fix_file(FixFileParams { | ||
| project_key, | ||
| path: BiomePath::new("test.ts"), | ||
| fix_file_mode: FixFileMode::SafeAndUnsafeFixes, | ||
| should_format: false, | ||
| only: vec![], | ||
| skip: vec![], | ||
| enabled_rules: vec![], | ||
| rule_categories, | ||
| suppression_reason: None, | ||
| diagnostic_level: Some(Severity::Information), | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| // Should fix both violations | ||
| let expected_both_fixed = r#"let x: number | undefined ; | ||
| if (x === 1) { | ||
| console.log(x); | ||
| } | ||
| "#; | ||
| assert_eq!(result_info_level.code, expected_both_fixed); | ||
|
|
||
| // Test 3: With diagnostic_level=None (default), should fix all violations | ||
| let result_no_level = workspace | ||
| .fix_file(FixFileParams { | ||
| project_key, | ||
| path: BiomePath::new("test.ts"), | ||
| fix_file_mode: FixFileMode::SafeAndUnsafeFixes, | ||
| should_format: false, | ||
| only: vec![], | ||
| skip: vec![], | ||
| enabled_rules: vec![], | ||
| rule_categories, | ||
| suppression_reason: None, | ||
| diagnostic_level: None, | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| // Should fix both violations (same as info level) | ||
| assert_eq!(result_no_level.code, expected_both_fixed); | ||
| } |
There was a problem hiding this comment.
Avoid brittle equality on whitespace; assert intent instead.
Exact string equality here is sensitive to formatter/whitespace details. Mirror the CLI tests: assert presence/absence of key substrings.
Apply this diff:
@@
- let expected_error_only = r#"let x: number | undefined = undefined;
-if (x === 1) {
- console.log(x);
-}
-"#;
- assert_eq!(result_error_level.code, expected_error_only);
+ assert!(result_error_level.code.contains("x === 1"));
+ assert!(result_error_level.code.contains("= undefined"));
@@
- let expected_both_fixed = r#"let x: number | undefined ;
-if (x === 1) {
- console.log(x);
-}
-"#;
- assert_eq!(result_info_level.code, expected_both_fixed);
+ assert!(result_info_level.code.contains("x === 1"));
+ assert!(!result_info_level.code.contains("= undefined"));
@@
- // Should fix both violations (same as info level)
- assert_eq!(result_no_level.code, expected_both_fixed);
+ // Should fix both violations (same as info level)
+ assert!(result_no_level.code.contains("x === 1"));
+ assert!(!result_no_level.code.contains("= undefined"));🤖 Prompt for AI Agents
In crates/biome_service/src/workspace.tests.rs around lines 969 to 1088, the
test uses brittle exact string equality for expected outputs; change these to
assert on key substrings instead. Replace the assert_eq for
result_error_level.code with checks that the result contains "===" and still
contains "= undefined" (ensuring only the double-equals was fixed). Replace the
expected_both_fixed equality checks for result_info_level.code and
result_no_level.code with assertions that they contain "===" and do NOT contain
"= undefined" (ensuring the info-level rule removed the useless undefined
initialization). Use contains/!contains assertions rather than exact
whitespace-sensitive string equality.
vhfmag
force-pushed
the
victor/fix-7772
branch
from
514b715 to
984f4a8
Compare
vhfmag
changed the title
--fix respect --diagnostic-level
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_service/src/file_handlers/mod.rs (1)
762-769: Fallback behaviour is inconsistent—changeunwrap_or(Severity::Warning)tounwrap_or_else(|| diagnostic.severity()).The review comment is spot-on. There's an unintended inconsistency between this function and the similar logic at line 539. Here, when a lint category exists but no rule override is found, the code downgrades to
Warning. At line 539, the same scenario preserves the original severity.This means an
Errorfrom a linter would stayErrorat line 539 but becomeWarningat line 766—hardly the desired behaviour. Matching line 539's pattern by usingdiagnostic.severity()as the fallback maintains the original diagnostic severity when configuration overrides don't apply.
🧹 Nitpick comments (7)
crates/biome_service/src/file_handlers/graphql.rs (1)
555-567: Avoid double severity lookups; gate and count with one valueYou compute severity for gating, then call
is_diagnostic_error(which computes it again). Compute once and reuse for both gating and error counting.- if let Some(diagnostic) = current_diagnostic.as_ref() { - // Check if we should skip this diagnostic based on its level - if let Some(min_level) = params.fix_level { - let diagnostic_severity = - get_diagnostic_severity(diagnostic, rules.as_deref()); - if diagnostic_severity < min_level { - return ControlFlow::Continue(()); - } - } - - if is_diagnostic_error(diagnostic, rules.as_deref()) { - errors += 1; - } - } + if let Some(diagnostic) = current_diagnostic.as_ref() { + let sev = get_diagnostic_severity(diagnostic, rules.as_deref()); + if let Some(min_level) = params.fix_level { + if sev < min_level { + return ControlFlow::Continue(()); + } + } + if sev >= biome_diagnostics::Severity::Error { + errors += 1; + } + }crates/biome_service/src/file_handlers/css.rs (1)
650-663: Compute severity once; reuse for gating and error countMirror the GraphQL refactor to avoid recomputing severity and simplify the logic.
- if let Some(diagnostic) = current_diagnostic.as_ref() { - // Check if we should skip this diagnostic based on its level - if let Some(min_level) = params.fix_level { - let diagnostic_severity = - get_diagnostic_severity(diagnostic, rules.as_deref()); - if diagnostic_severity < min_level { - return ControlFlow::Continue(()); - } - } - - if is_diagnostic_error(diagnostic, rules.as_deref()) { - errors += 1; - } - } + if let Some(diagnostic) = current_diagnostic.as_ref() { + let sev = get_diagnostic_severity(diagnostic, rules.as_deref()); + if let Some(min_level) = params.fix_level { + if sev < min_level { + return ControlFlow::Continue(()); + } + } + if sev >= biome_diagnostics::Severity::Error { + errors += 1; + } + }crates/biome_service/src/file_handlers/json.rs (1)
655-667: Single-source the severity; remove duplicate computationInline the error check using the already computed severity to reduce work and branch complexity.
- if let Some(diagnostic) = current_diagnostic.as_ref() { - // Check if we should skip this diagnostic based on its level - if let Some(min_level) = params.fix_level { - let diagnostic_severity = - get_diagnostic_severity(diagnostic, rules.as_deref()); - if diagnostic_severity < min_level { - return ControlFlow::Continue(()); - } - } - - if is_diagnostic_error(diagnostic, rules.as_deref()) { - errors += 1; - } - } + if let Some(diagnostic) = current_diagnostic.as_ref() { + let sev = get_diagnostic_severity(diagnostic, rules.as_deref()); + if let Some(min_level) = params.fix_level { + if sev < min_level { + return ControlFlow::Continue(()); + } + } + if sev >= biome_diagnostics::Severity::Error { + errors += 1; + } + }crates/biome_cli/src/execute/mod.rs (3)
115-117: Behavioural change: fix level always presentStoring
fix_level: Severity(non‑optional) makes it hard to distinguish “user explicitly set--diagnostic-level” from “default”. If defaults previously allowed fixing lower‑than‑default severities, this will change behaviour. Consider carrying anOption<Severity>or a separate boolean to indicate an explicit user override.
146-148: Same note for Lint modeSee comment above; the same uncertainty applies here.
520-526: Getter hides “unset” vs “default” — consider an Option-returning API
get_fix_level()always returns a value, so call sites can’t detect “no user filter”. If you keepfix_levelnon‑optional, consider addingfix_level_provided()or returnOption<Severity>to let callers decide whether to gate. This will also simplify avoiding accidental gating in stdin paths.Happy to sketch the small API change and thread it through if you want.
crates/biome_cli/src/commands/mod.rs (1)
129-136: Documentation could be clearer.The help text on line 129 reads: "Passing
--fix-level=errorwill cause Biome only to fix error-level diagnostics contain only errors." The phrase "contain only errors" at the end seems like a typo or redundant phrasing. Consider simplifying to: "Passing--fix-level=errorwill cause Biome to fix only error-level diagnostics."The same issue appears in the Lint command documentation at line 286.
Apply this diff to clarify the documentation:
- /// The level of diagnostics to fix. In order, from the lowest to the most important: info, warn, error. Passing `--fix-level=error` will cause Biome only to fix error-level diagnostics contain only errors. + /// The level of diagnostics to fix. In order, from the lowest to the most important: info, warn, error. Passing `--fix-level=error` will cause Biome to fix only error-level diagnostics.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
crates/biome_cli/tests/snapshots/main_commands_check/diagnostic_level_without_fix_respects_level.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_check/fix_level_error_only_fixes_error_level.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_check/fix_level_info_fixes_all_levels.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_check/fix_level_warn_fixes_warn_and_error_levels.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_lint/lint_diagnostic_level_without_fix_respects_level.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_lint/lint_fix_level_error_only_fixes_error_level.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_lint/lint_fix_level_info_fixes_all_levels.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (21)
.changeset/busy-olives-hang.md(1 hunks)crates/biome_cli/src/commands/check.rs(3 hunks)crates/biome_cli/src/commands/lint.rs(3 hunks)crates/biome_cli/src/commands/mod.rs(2 hunks)crates/biome_cli/src/commands/scan_kind.rs(2 hunks)crates/biome_cli/src/execute/mod.rs(3 hunks)crates/biome_cli/src/execute/process_file/lint_and_assist.rs(1 hunks)crates/biome_cli/src/execute/std_in.rs(1 hunks)crates/biome_cli/src/execute/traverse.rs(2 hunks)crates/biome_cli/src/lib.rs(4 hunks)crates/biome_cli/tests/commands/check.rs(2 hunks)crates/biome_cli/tests/commands/lint.rs(2 hunks)crates/biome_lsp/src/handlers/analysis.rs(1 hunks)crates/biome_service/src/file_handlers/css.rs(2 hunks)crates/biome_service/src/file_handlers/graphql.rs(2 hunks)crates/biome_service/src/file_handlers/javascript.rs(2 hunks)crates/biome_service/src/file_handlers/json.rs(2 hunks)crates/biome_service/src/file_handlers/mod.rs(3 hunks)crates/biome_service/src/workspace.rs(3 hunks)crates/biome_service/src/workspace.tests.rs(1 hunks)crates/biome_service/src/workspace/server.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/biome_cli/tests/commands/check.rs
- crates/biome_cli/src/execute/process_file/lint_and_assist.rs
- crates/biome_cli/src/execute/traverse.rs
- crates/biome_service/src/workspace.tests.rs
- crates/biome_service/src/workspace.rs
- crates/biome_service/src/workspace/server.rs
- .changeset/busy-olives-hang.md
- crates/biome_cli/tests/commands/lint.rs
🧰 Additional context used
📓 Path-based instructions (2)
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_cli/src/execute/mod.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_cli/src/commands/mod.rscrates/biome_cli/src/execute/std_in.rscrates/biome_lsp/src/handlers/analysis.rscrates/biome_service/src/file_handlers/graphql.rscrates/biome_service/src/file_handlers/css.rscrates/biome_cli/src/commands/lint.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_cli/src/commands/check.rscrates/biome_service/src/file_handlers/json.rscrates/biome_cli/src/lib.rscrates/biome_cli/src/commands/scan_kind.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_cli/src/execute/mod.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_cli/src/commands/mod.rscrates/biome_cli/src/execute/std_in.rscrates/biome_lsp/src/handlers/analysis.rscrates/biome_service/src/file_handlers/graphql.rscrates/biome_service/src/file_handlers/css.rscrates/biome_cli/src/commands/lint.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_cli/src/commands/check.rscrates/biome_service/src/file_handlers/json.rscrates/biome_cli/src/lib.rscrates/biome_cli/src/commands/scan_kind.rs
🧠 Learnings (1)
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/lib/src/{lint,assist}/**/*.rs : Choose rule severity appropriately (error/warn/info); set severity in declare_lint_rule! only when deviating from default
Applied to files:
crates/biome_service/src/file_handlers/javascript.rscrates/biome_cli/src/commands/mod.rscrates/biome_service/src/file_handlers/graphql.rscrates/biome_service/src/file_handlers/css.rscrates/biome_cli/src/commands/lint.rscrates/biome_service/src/file_handlers/mod.rs
🧬 Code graph analysis (9)
crates/biome_cli/src/execute/mod.rs (1)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
Severity(9269-9269)
crates/biome_service/src/file_handlers/javascript.rs (1)
crates/biome_service/src/file_handlers/mod.rs (2)
get_diagnostic_severity(754-771)is_diagnostic_error(745-751)
crates/biome_cli/src/commands/mod.rs (1)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
Severity(9269-9269)
crates/biome_service/src/file_handlers/graphql.rs (1)
crates/biome_service/src/file_handlers/mod.rs (2)
get_diagnostic_severity(754-771)is_diagnostic_error(745-751)
crates/biome_service/src/file_handlers/css.rs (1)
crates/biome_service/src/file_handlers/mod.rs (2)
get_diagnostic_severity(754-771)is_diagnostic_error(745-751)
crates/biome_cli/src/commands/lint.rs (1)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
Severity(9269-9269)
crates/biome_service/src/file_handlers/mod.rs (2)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (2)
Severity(9269-9269)Rules(775-788)crates/biome_analyze/src/signals.rs (3)
diagnostic(22-22)diagnostic(81-85)diagnostic(356-381)
crates/biome_cli/src/commands/check.rs (1)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
Severity(9269-9269)
crates/biome_service/src/file_handlers/json.rs (1)
crates/biome_service/src/file_handlers/mod.rs (2)
get_diagnostic_severity(754-771)is_diagnostic_error(745-751)
🔇 Additional comments (10)
crates/biome_cli/src/execute/std_in.rs (1)
175-176: Unconditional severity gating: confirm default behaviourPassing
Some(mode.get_fix_level())means fixes are always filtered by a threshold, even when the user didn’t pass--diagnostic-level. If the intended behaviour is “only gate when the flag is set”, this should beNoneunless the user provided a value; otherwise we risk silently dropping info/hint fixes by default. Please confirm the intended default and adjust if necessary.Run to verify wiring from the commands layer:
crates/biome_cli/src/commands/scan_kind.rs (1)
97-97: LGTM! Test scaffolding updated correctly.The tests now properly initialise
Executionwith the newfix_levelfield usingSeverity::default(), which is sensible for tests that don't specifically exercise fix-level behaviour.Also applies to: 129-129
crates/biome_cli/src/commands/lint.rs (1)
14-14: LGTM! Clean integration of fix_level into lint command.The new
fix_levelfield is properly imported, declared in the payload, and threaded through to the execution path. Integrates nicely with the broader fix-level propagation pattern.Also applies to: 40-40, 148-148
crates/biome_cli/src/commands/check.rs (1)
11-11: LGTM! Consistent with lint command pattern.The check command now properly handles
fix_levelwith the same clean pattern as the lint command—import, declare, and propagate.Also applies to: 30-30, 149-149
crates/biome_lsp/src/handlers/analysis.rs (1)
355-355: LGTM! Sensible default for LSP.Setting
fix_level: Nonefor LSP fix-all operations is appropriate—it applies all available fixes without severity filtering, which matches typical IDE "fix all" behaviour.crates/biome_cli/src/lib.rs (1)
94-94: LGTM! Consistent wiring through command execution.The
fix_levelfield is properly destructured from the command variants and passed through to the respective payloads for both Check and Lint commands.Also applies to: 112-112, 136-136, 160-160
crates/biome_service/src/file_handlers/javascript.rs (1)
8-8: LGTM! Severity-based filtering implemented correctly.The fix-all logic now properly:
- Computes effective diagnostic severity (respecting rule overrides via
get_diagnostic_severity)- Skips diagnostics below the configured
fix_levelthreshold- Only counts remaining diagnostics as errors when appropriate
The control flow ensures that filtered-out diagnostics don't contribute to error counts, which is the correct behaviour.
Also applies to: 894-907
crates/biome_service/src/file_handlers/mod.rs (2)
417-418: LGTM! Optional fix_level enables flexible filtering.Adding
fix_level: Option<Severity>toFixAllParamsallows callers to optionally specify a minimum severity threshold. WhenNone, all diagnostics are processed, which is sensible default behaviour.
745-771: LGTM! Clean refactoring with centralised severity logic.The refactoring properly:
- Extracts severity computation into
get_diagnostic_severity- Handles rule-based severity overrides for lint diagnostics
- Simplifies
is_diagnostic_errorto delegate to the new helperThe logic correctly prioritises rule configuration over diagnostic defaults, ensuring user configuration is respected.
crates/biome_cli/src/commands/mod.rs (1)
133-134: No changes required.The default value of
Severity::default()isSeverity::Error, which is the most restrictive (and safest) fallback. The docstring already clearly documents that fix levels range from info (lowest) to error (highest), making the default behaviour transparent to users who can override it with--fix-level=infoor--fix-level=warnif needed.
|
@vhfmag since the PR isn't ready (as you mentioned), can you please move the PR into draft? |
|
@ematipico done, sorry |
2 participants
Summary
As mentioned in #7779, there currently is no way to tell
biome check --writeorbiome lint --writethe minimum fix level to apply. This PR makes the Biome CLI accept a new--fix-levelflag allowing to achieve that.See #7779 for a discussion about why this feature was requested.
Test Plan
Tested both manually and through unit tests (at
crates/biome_cli/tests/commands/check.rsandcrates/biome_cli/tests/commands/lint.rs).Docs
To be done, waiting on feedback about whether the proposed API and implementation looks good.