feat: refactor pr comments coderabbit style#57
Conversation
…details and actionable footers
🚨 Watchflow Rule Violations Detected🟡 Medium SeverityChecks PR description (body) and title for a linked issue reference (e.g. #123, Fixes #123, Closes #456). Use when the rule requires issue refs in either field.PR does not reference a linked issue (e.g. #123 or closes #123 in body/title) Rule validation failed with severity: medium Validates if the PR description meets minimum length requirementsPR description is empty Rule validation failed with severity: medium This comment was automatically generated by Watchflow. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReformats GitHub outputs to a governance-style header and uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (68.8%) is below the target coverage (80.0%). You can increase the head coverage or adjust the target coverage. @@ Coverage Diff @@
## main #57 +/- ##
=====================================
Coverage 68.7% 68.8%
=====================================
Files 154 154
Lines 9712 9725 +13
=====================================
+ Hits 6681 6693 +12
- Misses 3031 3032 +1 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/presentation/github_formatter.py (1)
108-119: Extract shared severity rendering/grouping to prevent formatter drift.The
<details>section rendering is duplicated across check-run and PR comment formatting, which makes future style updates easy to miss in one path.Also applies to: 179-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/github_formatter.py` around lines 108 - 119, Extract the duplicated "<details>" severity grouping/rendering into a single helper function (e.g., render_severity_details(severity, violations, emoji, count) or render_severity_block(severity, severity_groups[severity], emoji, count)) and replace the duplicated blocks in both the check-run and PR comment formatting with calls to that helper; the helper should build the same string currently produced (including summary, per-violation title using violation.rule_description or 'Unknown Rule', message, optional "How to fix:" line, and closing </details>), be used by the places that currently append to the local text variable, and preserve ordering and formatting so both code paths share the same implementation.tests/unit/presentation/test_github_formatter.py (1)
19-33: Add a regression test forSeverity.INFOsection rendering.Current assertions cover critical/high only; add an
INFOcase to ensure totals and displayed sections stay consistent when low-priority violations are present.As per coding guidelines, "Write regression tests for every bug fix; keep CI coverage thresholds green."
Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/presentation/test_github_formatter.py` around lines 19 - 33, Add a regression assertion to the existing test that verifies INFO-severity violations are rendered and counted: update the test that calls format_violations_comment (e.g., test_format_violations_comment) to include at least one violation with severity INFO and assert the comment contains an INFO section header and that the total Status line still reflects the correct violation count; reference the format_violations_comment function and the test name when locating where to add the new assertions.
🤖 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/presentation/github_formatter.py`:
- Around line 106-110: The PR comment currently omits or mislabels violations
with Severity.INFO or other unexpected severities; update the grouping and
labeling code (the severity_groups construction and the block that builds the
details using severity, emoji and count) so that all severities present in the
violations collection—including Severity.INFO and any unknown strings—are
rendered consistently in the comment instead of being dropped or remapped to
"low"; ensure the same mapping logic is used in both the PR-comment path and the
check-run path (where remapping to low currently happens), and adjust the
summary/total calculations so the total reported (the earlier total) equals the
sum of the shown bucket counts.
---
Nitpick comments:
In `@src/presentation/github_formatter.py`:
- Around line 108-119: Extract the duplicated "<details>" severity
grouping/rendering into a single helper function (e.g.,
render_severity_details(severity, violations, emoji, count) or
render_severity_block(severity, severity_groups[severity], emoji, count)) and
replace the duplicated blocks in both the check-run and PR comment formatting
with calls to that helper; the helper should build the same string currently
produced (including summary, per-violation title using
violation.rule_description or 'Unknown Rule', message, optional "How to fix:"
line, and closing </details>), be used by the places that currently append to
the local text variable, and preserve ordering and formatting so both code paths
share the same implementation.
In `@tests/unit/presentation/test_github_formatter.py`:
- Around line 19-33: Add a regression assertion to the existing test that
verifies INFO-severity violations are rendered and counted: update the test that
calls format_violations_comment (e.g., test_format_violations_comment) to
include at least one violation with severity INFO and assert the comment
contains an INFO section header and that the total Status line still reflects
the correct violation count; reference the format_violations_comment function
and the test name when locating where to add the new assertions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/presentation/github_formatter.pytests/unit/presentation/test_github_formatter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.py: Use modern typing only:dict[str, Any],list[str],str | None(noDict,List,Optional)
GitHub/HTTP/DB calls must beasync def; avoid blocking calls (time.sleep, sync HTTP) in async paths
All agent outputs and external payloads must use validatedBaseModelfrom Pydantic
Use dataclasses for internal immutable state where appropriate
Use structured logging at boundaries with fields:operation,subject_ids,decision,latency_ms
Implement Agent pattern: single-responsibility agents with typed inputs/outputs
Use Decorator pattern for retries, metrics, caching as cross-cutting concerns
Agent outputs must include:decision,confidence (0..1), shortreasoning,recommendations,strategy_used
Implement confidence policy: reject or route to human-in-the-loop when confidence < 0.5
Use minimal, step-driven prompts; provide Chain-of-Thought only for complexity > 0.7 or ambiguity > 0.6
Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)
Cache idempotent lookups; lazy-import heavy dependencies; bound fan-out withasyncio.Semaphore
Avoid redundant LLM calls; memoize per event when safe
Use domain errors (e.g.,AgentError) witherror_type,message,context,timestamp,retry_count
Use exponential backoff for transient failures; circuit-break noisy integrations when needed
Fail closed for risky decisions; provide actionable remediation in error paths
Validate all external inputs; verify webhook signatures
Implement prompt-injection hardening; sanitize repository content passed to LLMs
Performance targets: Static validation ~<100ms typical, hybrid decisions sub-second when cache warm, budget LLM paths thoughtfully
Reject old typing syntax (Dict,List,Optional) in code review
Reject blocking calls in async code; reject bareexcept:clauses; reject swallowed errors
Reject LLM calls for trivial/deterministic checks
Reject unvalidated agent outputs and missing confidenc...
Files:
src/presentation/github_formatter.pytests/unit/presentation/test_github_formatter.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
tests/**/*.py: Write unit tests for deterministic rule evaluation (pass/warn/block), model validation, and error paths
Write integration tests for webhook parsing, idempotency, multi-agent coordination, and state persistence
Usepytest.mark.asynciofor async tests; avoid live network calls; freeze time and seed randomness
Write regression tests for every bug fix; keep CI coverage thresholds green
Files:
tests/unit/presentation/test_github_formatter.py
🧬 Code graph analysis (1)
tests/unit/presentation/test_github_formatter.py (1)
src/presentation/github_formatter.py (1)
format_violations_comment(157-194)
🚨 Watchflow Rule Violations Detected🟡 Medium SeverityChecks PR description (body) and title for a linked issue reference (e.g. #123, Fixes #123, Closes #456). Use when the rule requires issue refs in either field.PR does not reference a linked issue (e.g. #123 or closes #123 in body/title) Rule validation failed with severity: medium This comment was automatically generated by Watchflow. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/presentation/github_formatter.py (1)
32-42:⚠️ Potential issue | 🟠 MajorDo not remap
info/unknown severities tolow.Line 32 excludes
info, and Line 40-Line 42 force unknown severities intolow, which mislabels output and can skew displayed buckets.Proposed fix
- severity_order = ["critical", "high", "medium", "low"] + severity_order = ["critical", "high", "medium", "low", "info"] @@ - else: - if "low" not in severity_groups: - severity_groups["low"] = [] - severity_groups["low"].append(violation) + else: + severity_groups.setdefault("info", []).append(violation)Also apply the same mapping strategy in
format_check_run_output(Line 116-Line 127) so summary counts and details stay consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/github_formatter.py` around lines 32 - 42, The code currently drops or remaps "info"/unknown severities into "low"; update grouping logic so severity_order includes "info" (e.g., severity_order = ["critical","high","medium","low","info"]) and change the loop that builds severity_groups (and the equivalent logic in format_check_run_output) to preserve any severity string not already in severity_groups by creating a new key for that exact severity instead of forcing it into "low"; use violation.severity.value or str(...) to derive the key (as in sev) and append to severity_groups[sev] if missing create severity_groups[sev] = [] first so counts/details remain accurate.
🧹 Nitpick comments (1)
tests/unit/presentation/test_github_formatter.py (1)
10-29: Add anINFOseverity regression assertion in this suite.This updated test validates critical/high rendering, but it should also cover
Severity.INFO(and ideally unknown fallback) to lock in severity bucket behavior.As per coding guidelines "
tests/**/*.py: Write regression tests for every bug fix; keep CI coverage thresholds green".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/presentation/test_github_formatter.py` around lines 10 - 29, Update the test_format_violations_comment_groups_by_severity test to include a Violation with Severity.INFO (and optionally one with an unknown/None severity) when building the violations list, then adjust assertions to expect the INFO bucket rendering (e.g., "<summary><b>🔵 Info Severity (1)</b></summary>" or whatever the formatter uses) and updated counts/status text returned by format_violations_comment; reference the test function name and format_violations_comment and Severity.INFO to locate where to add the new Violation and corresponding assertions.
🤖 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/api/recommendations.py`:
- Line 355: The HTML string interpolation inserts unescaped dynamic content
(description and possibly severity) into a <summary> tag; escape these values
before rendering to prevent malformed HTML or injection. Update the code that
constructs the summary (the f-string containing "<summary><b>{severity}
Severity:</b> {description}</summary>") to apply an HTML-escaping function
(e.g., html.escape or markupsafe.escape) to description (and severity if
variable) at the point of formatting, and add the corresponding import at the
top of the module so the rendered summary contains safe, escaped text.
---
Duplicate comments:
In `@src/presentation/github_formatter.py`:
- Around line 32-42: The code currently drops or remaps "info"/unknown
severities into "low"; update grouping logic so severity_order includes "info"
(e.g., severity_order = ["critical","high","medium","low","info"]) and change
the loop that builds severity_groups (and the equivalent logic in
format_check_run_output) to preserve any severity string not already in
severity_groups by creating a new key for that exact severity instead of forcing
it into "low"; use violation.severity.value or str(...) to derive the key (as in
sev) and append to severity_groups[sev] if missing create severity_groups[sev] =
[] first so counts/details remain accurate.
---
Nitpick comments:
In `@tests/unit/presentation/test_github_formatter.py`:
- Around line 10-29: Update the
test_format_violations_comment_groups_by_severity test to include a Violation
with Severity.INFO (and optionally one with an unknown/None severity) when
building the violations list, then adjust assertions to expect the INFO bucket
rendering (e.g., "<summary><b>🔵 Info Severity (1)</b></summary>" or whatever
the formatter uses) and updated counts/status text returned by
format_violations_comment; reference the test function name and
format_violations_comment and Severity.INFO to locate where to add the new
Violation and corresponding assertions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/api/recommendations.pysrc/presentation/github_formatter.pytests/unit/presentation/test_github_formatter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.py: Use modern typing only:dict[str, Any],list[str],str | None(noDict,List,Optional)
GitHub/HTTP/DB calls must beasync def; avoid blocking calls (time.sleep, sync HTTP) in async paths
All agent outputs and external payloads must use validatedBaseModelfrom Pydantic
Use dataclasses for internal immutable state where appropriate
Use structured logging at boundaries with fields:operation,subject_ids,decision,latency_ms
Implement Agent pattern: single-responsibility agents with typed inputs/outputs
Use Decorator pattern for retries, metrics, caching as cross-cutting concerns
Agent outputs must include:decision,confidence (0..1), shortreasoning,recommendations,strategy_used
Implement confidence policy: reject or route to human-in-the-loop when confidence < 0.5
Use minimal, step-driven prompts; provide Chain-of-Thought only for complexity > 0.7 or ambiguity > 0.6
Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)
Cache idempotent lookups; lazy-import heavy dependencies; bound fan-out withasyncio.Semaphore
Avoid redundant LLM calls; memoize per event when safe
Use domain errors (e.g.,AgentError) witherror_type,message,context,timestamp,retry_count
Use exponential backoff for transient failures; circuit-break noisy integrations when needed
Fail closed for risky decisions; provide actionable remediation in error paths
Validate all external inputs; verify webhook signatures
Implement prompt-injection hardening; sanitize repository content passed to LLMs
Performance targets: Static validation ~<100ms typical, hybrid decisions sub-second when cache warm, budget LLM paths thoughtfully
Reject old typing syntax (Dict,List,Optional) in code review
Reject blocking calls in async code; reject bareexcept:clauses; reject swallowed errors
Reject LLM calls for trivial/deterministic checks
Reject unvalidated agent outputs and missing confidenc...
Files:
src/presentation/github_formatter.pytests/unit/presentation/test_github_formatter.pysrc/api/recommendations.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
tests/**/*.py: Write unit tests for deterministic rule evaluation (pass/warn/block), model validation, and error paths
Write integration tests for webhook parsing, idempotency, multi-agent coordination, and state persistence
Usepytest.mark.asynciofor async tests; avoid live network calls; freeze time and seed randomness
Write regression tests for every bug fix; keep CI coverage thresholds green
Files:
tests/unit/presentation/test_github_formatter.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Keep PRs small (~<300 LOC where possible); split refactors into separate PRs
📚 Learning: 2026-01-31T19:35:22.504Z
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Include in PR body: rationale, behavior change, risk/rollback, and tests/docs updates
Applied to files:
src/api/recommendations.py
🧬 Code graph analysis (2)
src/presentation/github_formatter.py (1)
src/core/models.py (1)
Violation(20-31)
tests/unit/presentation/test_github_formatter.py (1)
src/presentation/github_formatter.py (1)
format_violations_comment(178-188)
🔇 Additional comments (1)
src/presentation/github_formatter.py (1)
180-186: Nice cleanup on empty-state and governance framing.The early return on Line 180-Line 181 plus the concise status/help messaging make the comment output clearer.
| [ | ||
| f"### {description} - {severity}", | ||
| "<details>", | ||
| f"<summary><b>{severity} Severity:</b> {description}</summary>", |
There was a problem hiding this comment.
Escape dynamic <summary> content before rendering.
Line 355 inserts description directly into HTML. Special characters (or </summary>) can break the collapsible block rendering.
Proposed fix
+import html
@@
- body_lines.extend(
+ safe_severity = html.escape(severity)
+ safe_description = html.escape(description)
+ body_lines.extend(
[
"<details>",
- f"<summary><b>{severity} Severity:</b> {description}</summary>",
+ f"<summary><b>{safe_severity} Severity:</b> {safe_description}</summary>",
"",
]
)📝 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.
| f"<summary><b>{severity} Severity:</b> {description}</summary>", | |
| import html | |
| safe_severity = html.escape(severity) | |
| safe_description = html.escape(description) | |
| body_lines.extend( | |
| [ | |
| "<details>", | |
| f"<summary><b>{safe_severity} Severity:</b> {safe_description}</summary>", | |
| "", | |
| ] | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/recommendations.py` at line 355, The HTML string interpolation
inserts unescaped dynamic content (description and possibly severity) into a
<summary> tag; escape these values before rendering to prevent malformed HTML or
injection. Update the code that constructs the summary (the f-string containing
"<summary><b>{severity} Severity:</b> {description}</summary>") to apply an
HTML-escaping function (e.g., html.escape or markupsafe.escape) to description
(and severity if variable) at the point of formatting, and add the corresponding
import at the top of the module so the rendered summary contains safe, escaped
text.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/presentation/github_formatter.py (1)
39-49:⚠️ Potential issue | 🟠 MajorSeverity bucketing still misclassifies
infoaslow.
Severity.INFOis a valid model value, but at Line 39 and Line 133 the order omitsinfo, and the fallback path at Line 47 and Line 141 remaps tolow. This causes mislabeled output in both the PR comment and check-run summary.Suggested fix
- severity_order = ["critical", "high", "medium", "low"] + severity_order = ["critical", "high", "medium", "low", "info"] @@ - else: - if "low" not in severity_groups: - severity_groups["low"] = [] - severity_groups["low"].append(violation) + else: + severity_groups.setdefault("info", []).append(violation) @@ - severity_order = ["critical", "high", "medium", "low"] + severity_order = ["critical", "high", "medium", "low", "info"] @@ - else: - # Fallback for unexpected severities - if "low" not in severity_groups: - severity_groups["low"] = [] - severity_groups["low"].append(violation) + else: + severity_groups.setdefault("info", []).append(violation)Also applies to: 133-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/github_formatter.py` around lines 39 - 49, The severity bucketing logic misclassifies Severity.INFO because the severity_order list and severity_groups initialization (severity_order, severity_groups) omit "info" and the fallback branch (the sev handling and else that appends to "low") remaps unknown severities to "low"; update both occurrences (the severity_order/ severity_groups setup and the loop that computes sev and handles the else fallback) to include "info" in the ordered list and to append unknown/mapped "info" severities to "info" (not "low") so that Severity.INFO is correctly bucketed in both the PR comment and check-run summary.
🧹 Nitpick comments (1)
tests/unit/presentation/test_github_formatter.py (1)
36-46: Add a regression test for validSeverity.INFOrendering.This test covers unknown-severity fallback, but not the normal INFO path from the enum. Add one explicit INFO test so severity grouping regressions are caught without model bypass hacks.
Suggested test addition
+def test_build_collapsible_violations_text_info_severity_bucket(): + v = Violation(rule_description="Info Rule", severity=Severity.INFO, message="Informational message") + comment = format_violations_comment([v]) + assert "<summary><b>⚪ Info Severity (1)</b></summary>" in comment + assert "Informational message" in commentAs per coding guidelines: "Write regression tests for every bug fix; keep CI coverage thresholds green".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/presentation/test_github_formatter.py` around lines 36 - 46, Add a new unit test that verifies normal INFO-severity rendering: create a Violation instance with severity=Severity.INFO and a unique message, call format_violations_comment on a list containing it, and assert the returned comment contains the INFO severity summary (e.g. the Info bucket label) and the message; implement this as a new test function (e.g. test_build_collapsible_violations_text_info) using the Violation class, Severity.INFO, and format_violations_comment so regressions in severity grouping are caught without bypassing Pydantic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/presentation/github_formatter.py`:
- Around line 39-49: The severity bucketing logic misclassifies Severity.INFO
because the severity_order list and severity_groups initialization
(severity_order, severity_groups) omit "info" and the fallback branch (the sev
handling and else that appends to "low") remaps unknown severities to "low";
update both occurrences (the severity_order/ severity_groups setup and the loop
that computes sev and handles the else fallback) to include "info" in the
ordered list and to append unknown/mapped "info" severities to "info" (not
"low") so that Severity.INFO is correctly bucketed in both the PR comment and
check-run summary.
---
Nitpick comments:
In `@tests/unit/presentation/test_github_formatter.py`:
- Around line 36-46: Add a new unit test that verifies normal INFO-severity
rendering: create a Violation instance with severity=Severity.INFO and a unique
message, call format_violations_comment on a list containing it, and assert the
returned comment contains the INFO severity summary (e.g. the Info bucket label)
and the message; implement this as a new test function (e.g.
test_build_collapsible_violations_text_info) using the Violation class,
Severity.INFO, and format_violations_comment so regressions in severity grouping
are caught without bypassing Pydantic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/presentation/github_formatter.pytests/unit/presentation/test_github_formatter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.py: Use modern typing only:dict[str, Any],list[str],str | None(noDict,List,Optional)
GitHub/HTTP/DB calls must beasync def; avoid blocking calls (time.sleep, sync HTTP) in async paths
All agent outputs and external payloads must use validatedBaseModelfrom Pydantic
Use dataclasses for internal immutable state where appropriate
Use structured logging at boundaries with fields:operation,subject_ids,decision,latency_ms
Implement Agent pattern: single-responsibility agents with typed inputs/outputs
Use Decorator pattern for retries, metrics, caching as cross-cutting concerns
Agent outputs must include:decision,confidence (0..1), shortreasoning,recommendations,strategy_used
Implement confidence policy: reject or route to human-in-the-loop when confidence < 0.5
Use minimal, step-driven prompts; provide Chain-of-Thought only for complexity > 0.7 or ambiguity > 0.6
Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)
Cache idempotent lookups; lazy-import heavy dependencies; bound fan-out withasyncio.Semaphore
Avoid redundant LLM calls; memoize per event when safe
Use domain errors (e.g.,AgentError) witherror_type,message,context,timestamp,retry_count
Use exponential backoff for transient failures; circuit-break noisy integrations when needed
Fail closed for risky decisions; provide actionable remediation in error paths
Validate all external inputs; verify webhook signatures
Implement prompt-injection hardening; sanitize repository content passed to LLMs
Performance targets: Static validation ~<100ms typical, hybrid decisions sub-second when cache warm, budget LLM paths thoughtfully
Reject old typing syntax (Dict,List,Optional) in code review
Reject blocking calls in async code; reject bareexcept:clauses; reject swallowed errors
Reject LLM calls for trivial/deterministic checks
Reject unvalidated agent outputs and missing confidenc...
Files:
src/presentation/github_formatter.pytests/unit/presentation/test_github_formatter.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
tests/**/*.py: Write unit tests for deterministic rule evaluation (pass/warn/block), model validation, and error paths
Write integration tests for webhook parsing, idempotency, multi-agent coordination, and state persistence
Usepytest.mark.asynciofor async tests; avoid live network calls; freeze time and seed randomness
Write regression tests for every bug fix; keep CI coverage thresholds green
Files:
tests/unit/presentation/test_github_formatter.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Include in PR body: rationale, behavior change, risk/rollback, and tests/docs updates
📚 Learning: 2026-01-31T19:35:22.504Z
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Applies to tests/**/*.py : Write unit tests for deterministic rule evaluation (pass/warn/block), model validation, and error paths
Applied to files:
tests/unit/presentation/test_github_formatter.py
🧬 Code graph analysis (2)
src/presentation/github_formatter.py (1)
src/core/models.py (1)
Violation(20-31)
tests/unit/presentation/test_github_formatter.py (2)
src/presentation/github_formatter.py (1)
format_violations_comment(195-213)src/core/models.py (2)
Violation(20-31)Severity(8-17)
🔇 Additional comments (2)
src/presentation/github_formatter.py (1)
205-212: Nice improvement: no-op on clean runs and clear action footer.Returning an empty string for no violations and keeping the compact ack/help footer matches the “comment only when action is required” objective well.
tests/unit/presentation/test_github_formatter.py (1)
19-29: Test expectations are aligned with the new collapsible comment format.The updated header,
<summary>assertions, and ack hint checks are solid and match the presentation refactor.Also applies to: 33-33
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/presentation/github_formatter.py (2)
46-49: Dead code: conditional check is always false.
severity_groupsis initialized on line 40 with"low"as a key, so the condition"low" not in severity_groupsis alwaysFalse. Theifblock is unreachable.Simplify the fallback logic
else: - if "low" not in severity_groups: - severity_groups["low"] = [] severity_groups["low"].append(violation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/github_formatter.py` around lines 46 - 49, The fallback branch contains an unreachable check because severity_groups is prepopulated with "low"; remove the redundant if "low" not in severity_groups check and directly append the violation to severity_groups["low"] in the same block (update the code that manipulates severity_groups and the list appended with violation).
140-144: Same dead code pattern as the helper function.The check
if "low" not in severity_groups:is always false here as well.Simplify
else: - # Fallback for unexpected severities - if "low" not in severity_groups: - severity_groups["low"] = [] + # Fallback for unexpected severities to low severity_groups["low"].append(violation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/github_formatter.py` around lines 140 - 144, The fallback block contains a redundant check—`if "low" not in severity_groups:`—which can never be true here; remove that dead conditional and simply ensure the "low" bucket exists and append the violation. Replace the two-step conditional+append with a single operation that guarantees the "low" key (for example, using dict.setdefault or creating the key if missing) and then append the violation to severity_groups["low"]; update the same pattern in the helper function if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/presentation/github_formatter.py`:
- Around line 46-49: The fallback branch contains an unreachable check because
severity_groups is prepopulated with "low"; remove the redundant if "low" not in
severity_groups check and directly append the violation to
severity_groups["low"] in the same block (update the code that manipulates
severity_groups and the list appended with violation).
- Around line 140-144: The fallback block contains a redundant check—`if "low"
not in severity_groups:`—which can never be true here; remove that dead
conditional and simply ensure the "low" bucket exists and append the violation.
Replace the two-step conditional+append with a single operation that guarantees
the "low" key (for example, using dict.setdefault or creating the key if
missing) and then append the violation to severity_groups["low"]; update the
same pattern in the helper function if present.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/presentation/github_formatter.pytests/unit/presentation/test_github_formatter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/presentation/test_github_formatter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.py: Use modern typing only:dict[str, Any],list[str],str | None(noDict,List,Optional)
GitHub/HTTP/DB calls must beasync def; avoid blocking calls (time.sleep, sync HTTP) in async paths
All agent outputs and external payloads must use validatedBaseModelfrom Pydantic
Use dataclasses for internal immutable state where appropriate
Use structured logging at boundaries with fields:operation,subject_ids,decision,latency_ms
Implement Agent pattern: single-responsibility agents with typed inputs/outputs
Use Decorator pattern for retries, metrics, caching as cross-cutting concerns
Agent outputs must include:decision,confidence (0..1), shortreasoning,recommendations,strategy_used
Implement confidence policy: reject or route to human-in-the-loop when confidence < 0.5
Use minimal, step-driven prompts; provide Chain-of-Thought only for complexity > 0.7 or ambiguity > 0.6
Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)
Cache idempotent lookups; lazy-import heavy dependencies; bound fan-out withasyncio.Semaphore
Avoid redundant LLM calls; memoize per event when safe
Use domain errors (e.g.,AgentError) witherror_type,message,context,timestamp,retry_count
Use exponential backoff for transient failures; circuit-break noisy integrations when needed
Fail closed for risky decisions; provide actionable remediation in error paths
Validate all external inputs; verify webhook signatures
Implement prompt-injection hardening; sanitize repository content passed to LLMs
Performance targets: Static validation ~<100ms typical, hybrid decisions sub-second when cache warm, budget LLM paths thoughtfully
Reject old typing syntax (Dict,List,Optional) in code review
Reject blocking calls in async code; reject bareexcept:clauses; reject swallowed errors
Reject LLM calls for trivial/deterministic checks
Reject unvalidated agent outputs and missing confidenc...
Files:
src/presentation/github_formatter.py
🧬 Code graph analysis (1)
src/presentation/github_formatter.py (1)
src/core/models.py (1)
Violation(20-31)
🔇 Additional comments (2)
src/presentation/github_formatter.py (2)
195-213: LGTM!The refactored function correctly suppresses comments when no violations exist (returning empty string), uses the new collapsible helper for consistent formatting, and includes the actionable footer per PR objectives.
292-294: LGTM!Consistent use of the collapsible violations helper for remaining violations in the acknowledgment check run output.
dkargatzis
force-pushed
the
ux/refactor-pr-comments-coderabbit-style
branch
from
6086aa2 to
33fa29c
Compare
2 participants
Closes #56
Summary by CodeRabbit
Bug Fixes
New Features
Tests