| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Warning Review limit reached@shenxianpeng, you've reached your PR review limit, so we couldn't start this review. Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR. To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 10bf0474-8c86-44c0-b49d-dfcf33a2da0f 📥 CommitsReviewing files that changed from the base of the PR and between 9817088 and 8e150a4. 📒 Files selected for processing (6)
WalkthroughThe change formats length-rule suggestions with configured limits and adds OSC 8 hyperlink support for validation failure output. It also updates check-name formatting, documentation-link rendering, and related spacing tests. ChangesValidation message improvements
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: minor Sequence Diagram(s)sequenceDiagram
participant FailureOutput
participant print_error_message
participant supports_hyperlinks
participant hyperlink
participant Terminal
FailureOutput->>print_error_message: Pass rule_id and docs_url
print_error_message->>supports_hyperlinks: Check OSC 8 support
supports_hyperlinks-->>print_error_message: Return terminal capability
alt Hyperlinks supported
print_error_message->>hyperlink: Link rule ID to documentation
hyperlink-->>print_error_message: Return OSC 8-formatted ID
end
print_error_message->>Terminal: Print formatted failure message
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
The rule ID now carries an OSC 8 hyperlink to its section of the rules reference, the way ruff links its codes, so the ID itself is what you click. Where that renders, the separate "Docs:" line is dropped: it was repeating an address the reader already has. Where it does not — a pipe, a CI log, a terminal that would print the escape as junk — the line stays, because there it is the only way to reach the address at all. Three things the previous output got wrong, all visible in a two-failure run: The blank line closed the suggestion rather than the block, so "Docs:" was separated from the rule it belonged to and butted against the next one. It now closes the block. The name was printed as the config key, subject_min_length, while the rules reference titles its sections in kebab-case. Reading a name off the terminal and searching the documentation for it found nothing. Both now say subject-min-length. The advice for the two length rules named no length: "Provide a meaningful subject (>= configured min)", directly under an error that had already said "at least 5 characters". The suggestion is now templated on the same value as the error.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)commit_check/rule_builder.py (1)🤖 Prompt for all review comments with AI agents245-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add regression coverage for suggestion formatting.
The current test uses a suggestion without placeholders. It does not verify {max_len} or {min_len} substitution. Add a test for both length rules with a configured value.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@commit_check/rule_builder.py` around lines 245 - 249, Add regression coverage around the suggestion formatting logic in the rule-builder tests, using configured suggestions containing both {max_len} and {min_len} placeholders for each length rule. Assert that the generated suggestions substitute the configured length value correctly.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@commit_check/util.py`:
- Around line 322-323: Update the FORCE_HYPERLINK check in the
hyperlink-detection function to return True only when the environment value is
exactly "1", while preserving normal TTY behavior otherwise. Add a regression
test covering FORCE_HYPERLINK="0" with sys.stdout.isatty() returning False and
verify hyperlinks are not forced.
---
Nitpick comments:
In `@commit_check/rule_builder.py`:
- Around line 245-249: Add regression coverage around the suggestion formatting
logic in the rule-builder tests, using configured suggestions containing both
{max_len} and {min_len} placeholders for each length rule. Assert that the
generated suggestions substitute the configured length value correctly.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aef7df59-3e58-487c-b501-efdf5498017b
📥 CommitsReviewing files that changed from the base of the PR and between 631ae91 and 9817088.
📒 Files selected for processing (4)
Sorry, something went wrong.
| if os.environ.get("FORCE_HYPERLINK"): | ||
| return True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat FORCE_HYPERLINK as an explicit boolean.
Line 322 enables hyperlinks for any nonempty value. FORCE_HYPERLINK=0 therefore emits OSC 8 sequences even for piped output. The PR contract specifies FORCE_HYPERLINK=1 as the forcing value.
Proposed fix- if os.environ.get("FORCE_HYPERLINK"):
+ if os.environ.get("FORCE_HYPERLINK") == "1":
return TrueAdd a regression test with FORCE_HYPERLINK="0" and sys.stdout.isatty() set to False.
📝 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.
| if os.environ.get("FORCE_HYPERLINK"): | |
| return True | |
| if os.environ.get("FORCE_HYPERLINK") == "1": | |
| return True |
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@commit_check/util.py` around lines 322 - 323, Update the FORCE_HYPERLINK check in the hyperlink-detection function to return True only when the environment value is exactly "1", while preserving normal TTY behavior otherwise. Add a regression test covering FORCE_HYPERLINK="0" with sys.stdout.isatty() returning False and verify hyperlinks are not forced.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #520 +/- ##
==========================================
+ Coverage 97.43% 97.48% +0.05%
==========================================
Files 12 12
Lines 1207 1231 +24
==========================================
+ Hits 1176 1200 +24
Misses 31 31 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
Any non-empty value counted as on, so FORCE_HYPERLINK=0 — the obvious way to ask for links to be turned off — turned them on instead. It now follows what FORCE_COLOR established and what ruff does: 0 disables even on a terminal that renders links, any other value enables. Also covers the length-rule substitution that had none. Removing the .format() call leaves the four new cases failing, so they hold the placeholder from reaching a user as a literal brace.
|
Sorry, something went wrong.
Merging this PR will degrade performance by 12.27%❌ 9 regressed benchmarks Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent. Comparing fix/link-rule-ids-to-docs (8e150a4) with main (631ae91) Footnotes
|
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
The rule ID now carries an OSC 8 hyperlink to its section of the rules reference, so the ID itself is what you click — the same mechanism ruff uses for its codes.
Verified against ruff rather than assumed: under a terminal it advertises support for, it emits
and prints no separate documentation line, which is why its output stays compact.
The URL still has to survive CI
commit-check runs in a lot of places ruff does not, so dropping the Docs: line outright would lose the address wherever the escape cannot render. The line is kept unless the ID is actually a link:
All four were exercised against a real pty, not just unit-tested.
Three things the output got wrong
Visible in any two-failure run:
The blank line closed the suggestion, not the block. Docs: ended up separated from the rule it belonged to and butted against the next one:
Suggest: Use <type>(<scope>): <description>, ... ← separator landed here Docs: https://commit-check.com/rules/#cc001 CC005 subject_min_length check failed ==> test ← no separationThe name was the config key, not the documented one. Output said subject_min_length; the rules reference titles that section subject-min-length. Reading a name off the terminal and searching the docs for it found nothing. RuleCatalogEntry.name already existed for this and simply was not being used.
The advice for the length rules named no length. Suggest: Provide a meaningful subject (>= configured min) sat directly under Subject must be at least 5 characters — vaguer than the line above it. Both length suggestions are now templated on the same value as the error, so CC004 and CC005 name the actual limit.
Before / after
Trailing whitespace on the failure and suggestion lines is gone as a side effect of the rewrite.
Testing
The failure is test_load_config_file_permission_error, which uses os.chmod(0o000) and cannot fail as root; it reproduces on main.
TestHyperlinks adds 11 cases: the escape shape, terminal detection across nine environments including a malformed VTE_VERSION, that the ID is linked only when supported, that the Docs: line appears exactly when the ID is not a link, and that the blank line closes the block.
Two existing assertions in test_print_error_message were checking for author_email in the output. They now assert author-email and additionally that no underscore survives into the printed name, so the kebab-case contract is pinned rather than merely accommodated.
Not in this PR
The banner says "Commit rejected by Commit-Check." and then "Commit rejected." on either side of the ASCII art, and there is still no summary count of failures. Both change the shape of the output rather than fix a defect in it, so they belong with the Action output rework on the roadmap.
Summary by CodeRabbit
Bug Fixes
Usability Improvements