fix: skip parsing clang-tidy diagnostic rationale by 2bndy5 · Pull Request #237 · cpp-linter/cpp-linter-rs

2bndy5 · 2025-12-23T21:18:29Z

Specifically when the rationale line has added context in square brackets.

This includes a test based on the actual bug reported in cpp-linter/cpp-linter-action#389.

Corresponds to cpp-linter/cpp-linter#176

Summary by CodeRabbit

Bug Fixes
- Fixed diagnostic parsing to properly distinguish between actual diagnostics and diagnostic rationale text in output.
- Improved path normalization handling for compilation database and standard scenarios.
Tests
- Added test coverage for diagnostic rationale parsing.
Chores
- Updated spelling configuration.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Specifically when the rationale line has added context in square brackets. This includes a test based on the actual bug reported in cpp-linter/cpp-linter-action#389. Corresponds to cpp-linter/cpp-linter#176

coderabbitai · 2025-12-23T21:23:13Z

Walkthrough

The PR modifies clang-tidy diagnostic parsing to distinguish real diagnostics from false-positive brackets by introducing a validation guard, adjusts path normalization logic to be conditional on compilation database presence and findings, and adds a test case to verify the new diagnostic detection behavior.

Changes

Cohort / File(s)	Summary
Diagnostic parsing refinements `cpp-linter/src/clang_tools/clang_tidy.rs`	Introduces a guard to start new TidyNotification only when diagnostic name contains no spaces and includes a dash, preventing false positives like "[with auto ...]"; adjusts path normalization to check compilation database for matching units and normalize relative to found directory or CWD; finalizes current notification after parsing completes
Test coverage `cpp-linter/src/clang_tools/clang_tidy.rs`	Adds test case `skip_parse_tidy_diagnostic_rationale` to verify diagnostic rationale brackets are ignored; adjusts test imports for `parse_tidy_output` access
Spell-check dictionary `cspell.config.yml`	Adds "declval" to recognized words list

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

fix: parse clang-tidy output when WarningsAsErrors is asserted #190: Modifies diagnostic/header detection logic in parse_tidy_output through regex and start-condition changes, addressing similar diagnostic identification concerns.
feat: capture and output clang tool's version number #54: Changes to parse_tidy_output function signature and return handling that affect notification processing flow.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: skipping parsing of clang-tidy diagnostic rationale lines, which is the primary focus of the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch skip-tidy-rationale

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a4575ec and 72b949f.

📒 Files selected for processing (2)

cpp-linter/src/clang_tools/clang_tidy.rs
cspell.config.yml

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 101
File: cpp-linter/src/clang_tools/clang_format.rs:155-161
Timestamp: 2025-01-21T09:56:32.771Z
Learning: In cpp-linter-rs, the XML output being parsed is generated programmatically by clang-format tool. The only failure case for XML parsing is when clang-format produces a blank XML document, in which case falling back to empty results (using unwrap_or) is the desired behavior.

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 208
File: cpp-linter/src/clang_tools/mod.rs:60-115
Timestamp: 2025-11-04T06:50:10.870Z
Learning: In the cpp-linter-rs project, path validation (such as checking whether a path is a file or directory) should be performed in CLI parsing (cpp-linter/src/cli/structs.rs) rather than in the tool lookup logic (cpp-linter/src/clang_tools/mod.rs). This maintains proper separation of concerns.

📚 Learning: 2024-09-29T18:00:02.286Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 50
File: justfile:37-42
Timestamp: 2024-09-29T18:00:02.286Z
Learning: The word 'mdbook' is still present in `CHANGELOG.md`, so `cspell.config.yml` should continue to include `mdbook` in the spelling exceptions list.

Applied to files:

cspell.config.yml

📚 Learning: 2025-11-04T06:50:10.870Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 208
File: cpp-linter/src/clang_tools/mod.rs:60-115
Timestamp: 2025-11-04T06:50:10.870Z
Learning: In the cpp-linter-rs project, path validation (such as checking whether a path is a file or directory) should be performed in CLI parsing (cpp-linter/src/cli/structs.rs) rather than in the tool lookup logic (cpp-linter/src/clang_tools/mod.rs). This maintains proper separation of concerns.

Applied to files:

cpp-linter/src/clang_tools/clang_tidy.rs

📚 Learning: 2024-11-23T06:20:11.698Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/rest_api/mod.rs:253-254
Timestamp: 2024-11-23T06:20:11.698Z
Learning: In `cpp-linter/src/rest_api/mod.rs`, it is safe to use `unwrap()` on `clang_versions.tidy_version` and `clang_versions.format_version` because they are guaranteed to be `Some` at that point in the code.

Applied to files:

cpp-linter/src/clang_tools/clang_tidy.rs

📚 Learning: 2025-01-21T09:56:32.771Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 101
File: cpp-linter/src/clang_tools/clang_format.rs:155-161
Timestamp: 2025-01-21T09:56:32.771Z
Learning: In cpp-linter-rs, the XML output being parsed is generated programmatically by clang-format tool. The only failure case for XML parsing is when clang-format produces a blank XML document, in which case falling back to empty results (using unwrap_or) is the desired behavior.

Applied to files:

cpp-linter/src/clang_tools/clang_tidy.rs

🧬 Code graph analysis (1)

cpp-linter/src/clang_tools/clang_tidy.rs (1)

cpp-linter/src/common_fs/mod.rs (3)

from (56-70)

normalize_path (237-262)

new (44-53)

🔇 Additional comments (5)

cspell.config.yml (1)

22-22: LGTM!

The addition of declval is appropriate—it's a valid C++ standard library identifier (std::declval) that appears in the new test case's clang-tidy output.

cpp-linter/src/clang_tools/clang_tidy.rs (4)

153-159: Nice fix for the false-positive diagnostic detection.

The hyphen check correctly filters out non-diagnostic bracket content like [with auto = ...]. Valid clang-tidy check names always follow a category-name pattern containing hyphens.

Minor note: The space check (!contains(' ')) is technically redundant since the regex group 6 pattern [a-zA-Z\d\-\.]+ already prohibits spaces. It's harmless as a defensive check, but could be simplified if desired.

161-215: LGTM!

The restructuring correctly:

Pushes the previous notification before starting a new one (preventing data loss)

Moves path normalization inside the guard to only execute for valid diagnostics

Uses debug_assert! appropriately to catch unexpected non-absolute paths during development

The conditional flow for database vs. no-database path normalization is clear and maintains the expected behavior.

361-365: LGTM!

Import update correctly brings parse_tidy_output into scope for the new test. Access to the private function from the in-file test module follows standard Rust conventions.

479-528: Excellent test case derived from the actual bug report.

The test correctly exercises the fix:

Contains 4 actual clang-diagnostic-error diagnostics

Contains problematic note: lines with brackets like [with file:auto = typename ...] that previously triggered false positives

Assertions verify that only valid diagnostics (containing -, no spaces) are captured

This provides good regression coverage for the issue reported in cpp-linter/cpp-linter-action#389.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2025-12-23T21:33:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.98%. Comparing base (a4575ec) to head (72b949f).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   96.93%   96.98%   +0.05%     
==========================================
  Files          14       14              
  Lines        3033     3085      +52     
==========================================
+ Hits         2940     2992      +52     
  Misses         93       93

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

fix: skip parsing clang-tidy diagnostic rationale

72b949f

Specifically when the rationale line has added context in square brackets. This includes a test based on the actual bug reported in cpp-linter/cpp-linter-action#389. Corresponds to cpp-linter/cpp-linter#176

2bndy5 added the bug Something isn't working label Dec 23, 2025

2bndy5 merged commit a282841 into main Dec 29, 2025
65 checks passed

2bndy5 deleted the skip-tidy-rationale branch December 29, 2025 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: skip parsing clang-tidy diagnostic rationale#237

fix: skip parsing clang-tidy diagnostic rationale#237
2bndy5 merged 1 commit intomainfrom
skip-tidy-rationale

2bndy5 commented Dec 23, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 23, 2025

Uh oh!

codecov bot commented Dec 23, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

2bndy5 commented Dec 23, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 23, 2025

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

codecov bot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

2bndy5 commented Dec 23, 2025 •

edited by coderabbitai bot

Loading

codecov bot commented Dec 23, 2025 •

edited

Loading