Skip to content

feat(coderabbit): v2 canonical — fix retry-loop bug, autofix-first #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ANcpLua merged 1 commit into from
May 25, 2026
Merged

feat(coderabbit): v2 canonical — fix retry-loop bug, autofix-first #16

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 101 additions & 4 deletions templates/coderabbit.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@
# Per-repo overrides: drop a `.coderabbit.yaml` in the target repo to
# override anything below. Repos with bespoke path_instructions (e.g. qyl)
# carry their own file; this template is the default for everything else.
#
# v2 (2026-05-25) — fixes the retry-loop bug observed on
# ANcpLua.Analyzers PR #174 (CR stuck 3+ hours in "Failed to post review
# comments" loop) and adds Pro+ autofix-first recipes:
# - disable_cache: true -> false (5-30x speedup on assertive)
# - auto_apply_labels EXPLICIT false (prevents label-event review re-fires)
# - path_filters: +9 entries (snupkg/nupkg/sarif/trx/release-artifacts/
# Snapshots/AnalyzerReleases.md noise)
# - finishing_touches.custom: 5 recipes (autofix XML docs, sync-over-async,
# missing tests, simplifications, security)
# - knowledge_base.code_guidelines: +6 patterns
# (Directory.Build.props, Version.props,
# README.md, docs/**, package.json, etc.)

language: en-US
tone_instructions: >-
Expand Down Expand Up @@ -49,37 +62,63 @@ reviews:
assess_linked_issues: true
related_issues: true
related_prs: true
# v2 FIX: keep label SUGGESTIONS in the walkthrough but never let CR mutate
# the PR's label set itself. auto_apply_labels: true + many labeling_instructions
# caused a label-event review-retry loop that deadlocked PR #174 for 3+ hours.
suggested_labels: true
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
in_progress_fortune: false
poem: false
enable_prompt_for_ai_agents: true
abort_on_close: true
disable_cache: true
# v2 FIX: prior `true` forced every review cold-start; assertive profile
# is 5-30x faster cache-warm.
disable_cache: false

slop_detection:
enabled: true
label: 'review:slop'

# v2 EXPAND: exclude noise at FILTER level so CR never has to walk it.
# Avoids the "review of a 5.6 MB embedded JSON" failure mode large
# analyzer/source-gen repos hit.
path_filters:
# Generated source
- '!**/*.g.cs'
- '!**/*.g.ts'
- '!**/*.g.sql'
- '!**/*.g.tsp'
- '!**/*.Designer.cs'
- '!**/*.generated.*'
- '!**/Generated/**'
- '!**/generated/**'
# Build outputs + artifacts
- '!**/bin/**'
- '!**/obj/**'
- '!**/dist/**'
- '!**/artifacts/**'
- '!Artifacts/**'
- '!**/release-artifacts/**'
- '!**/coverage/**'
# JS noise + lockfiles
- '!**/node_modules/**'
- '!**/*.min.js'
- '!**/*.min.css'
- '!**/package-lock.json'
- '!**/pnpm-lock.yaml'
- '!**/yarn.lock'
- '!Artifacts/**'
# CI reports + packages
- '!**/*.sarif'
- '!**/*.trx'
- '!**/*.nupkg'
- '!**/*.snupkg'
# Snapshot fixtures (byte-identity tests — no review value)
- '!**/Snapshots/**/*.expected.txt'
# Machine-generated catalogs (per-rule review covers the actual content)
- '!**/AnalyzerReleases.Shipped.md'
- '!**/AnalyzerReleases.Unshipped.md'

auto_review:
enabled: true
Expand All @@ -98,9 +137,55 @@ reviews:
enabled: true
simplify:
enabled: true
# v2 ADD: Pro+ autofix-first recipes. Repo-agnostic — they apply
# wherever the patterns exist; no-op elsewhere.
custom:
- enabled: true
name: autofix public XML doc gaps
instructions: >-
For each public/protected/internal-with-InternalsVisibleTo symbol
added in this PR that lacks XML docs (C#) or TSDoc (TS/TSX),
generate them. Document intent, contracts, cancellation, exceptions,
compatibility. Skip private implementation members. Don't restate
the method name or rephrase the type signature.
- enabled: true
name: autofix sync-over-async / cancellation gaps
instructions: >-
Where a public or internal async method lacks a CancellationToken
parameter, add `CancellationToken cancellationToken = default` and
thread it through. Where `.Result` / `.Wait()` / `.GetAwaiter().GetResult()`
blocks an async path, rewrite to await. Don't suppress with
`#pragma` — fix or leave a precise comment explaining why no fix
is possible.
Comment on lines +154 to +159
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

Adding a CancellationToken parameter to existing public methods is a binary-breaking change in .NET, even with a default value. While source-compatible for callers, it breaks binary compatibility and interface implementations.

For libraries requiring binary compatibility, it is safer to suggest an overload. Update the 'autofix sync-over-async / cancellation gaps' instructions to warn about binary-breaking changes and suggest that the agent prefer adding a method overload instead of just adding a parameter with a default value to existing public APIs.

- enabled: true
name: autofix missing test coverage on changed public behavior
instructions: >-
For each new public function/class/endpoint added in this PR with
no corresponding test under `tests/` or `test/`, generate a focused
test using the repo's existing test stack (xUnit v3, vitest, pytest,
etc.). Cover the positive case + at least one negative case. Don't
generate snapshot tests unless the repo already uses them.
Comment on lines +161 to +167
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

The test autofix recipe is not actually repo-agnostic.

Hardcoding tests/ and test/ means repos that keep tests in *.Tests/, spec/, or framework-specific folders will be treated as uncovered and may get generated files in the wrong place. Infer the existing test location first, then fall back only when the repo has no established convention.

Suggested fix 
       - enabled: true
         name: autofix missing test coverage on changed public behavior
         instructions: >-
           For each new public function/class/endpoint added in this PR with
-          no corresponding test under `tests/` or `test/`, generate a focused
-          test using the repo's existing test stack (xUnit v3, vitest, pytest,
-          etc.). Cover the positive case + at least one negative case. Don't
-          generate snapshot tests unless the repo already uses them.
+          no corresponding test in the repo's existing test location, generate
+          a focused test using the repo's existing test stack and file layout.
+          Infer the canonical test directory or test project from nearby tests
+          first; only fall back to `tests/` or `test/` if the repo has no
+          established convention. Cover the positive case + at least one
+          negative case. Don't generate snapshot tests unless the repo already
+          uses them.
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@templates/coderabbit.yaml` around lines 161 - 167, Update the "autofix
missing test coverage on changed public behavior" recipe so it infers the repo's
existing test location before hardfalling to "tests/" or "test/": scan the repo
for common test directories/patterns (e.g., tests/, test/, spec/, __tests__/,
*.Tests/, src/test/, tests/**/*, framework-specific folders) and pick the
dominant convention, use that path when generating new tests, and only fallback
to "tests/" or "test/" if no existing convention is found; make the detection
logic configurable or data-driven in the template so the recipe (instructions
field) references the detected testDir when emitting files.

- enabled: true
name: autofix obvious simplifications on changed lines
instructions: >-
Within the diff's changed lines only (not the surrounding file),
collapse trivially-removable indirection: unused locals, redundant
boolean ternaries, single-use intermediate variables, switch
expressions where the if-else is single-arm. Don't touch code
outside the diff hunks. Don't reformat unrelated code.
- enabled: true
name: autofix security obvious-fixes
instructions: >-
For obvious security issues introduced in this PR — secrets in
plain text, path traversal sources without sanitization, SQL string
concatenation, unsafe deserialization defaults — apply the minimal
fix (env var lookup, path validator, parameterized query, safe
deserialization options). Leave a comment for anything that needs
architectural judgment instead of guessing.

# Hard gates are off in the baseline — they are repo-specific. qyl keeps
# its own pre_merge_checks block in qyl/.coderabbit.yaml.
# Hard gates are off in the baseline — they are repo-specific. qyl,
# ANcpLua.Analyzers, and Qyl.OpenTelemetry.SemanticConventions keep their
# own pre_merge_checks block in their per-repo .coderabbit.yaml.
pre_merge_checks:
docstrings:
mode: 'off'
Expand Down Expand Up @@ -232,12 +317,22 @@ knowledge_base:
scope: auto
code_guidelines:
enabled: true
# v2 EXPAND: include build infra + per-package READMEs so CR knows the
# framework conventions every repo inherits.
filePatterns:
- 'CLAUDE.md'
- 'AGENTS.md'
- '**/CLAUDE.md'
- '**/AGENTS.md'
- '.editorconfig'
- 'README.md'
- 'docs/**/*.md'
- 'Directory.Build.props'
- 'Directory.Packages.props'
- 'global.json'
- 'Version.props'
- 'package.json'
- 'pnpm-workspace.yaml'
Comment on lines +320 to +335
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Per-package READMEs are still not included in code_guidelines.

Line 328 only matches the repository-root README.md, so nested package READMEs never feed the knowledge base. That contradicts the comment on Lines 320-321 and drops conventions from monorepos with package-level docs.

Suggested fix 
   filePatterns:
     - 'CLAUDE.md'
     - 'AGENTS.md'
     - '**/CLAUDE.md'
     - '**/AGENTS.md'
     - '.editorconfig'
     - 'README.md'
+    - '**/README.md'
     - 'docs/**/*.md'
     - 'Directory.Build.props'
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@templates/coderabbit.yaml` around lines 320 - 335, The filePatterns list in
templates/coderabbit.yaml only includes 'README.md' (root README) so
per-package/nested README files are excluded; update the filePatterns array (the
entry that currently lists 'README.md') to also include a glob like
'**/README.md' so nested package READMEs are picked up into
code_guidelines/knowledge base (modify the filePatterns block where 'README.md'
is declared).

jira:
usage: disabled
linear:
Expand All @@ -257,4 +352,6 @@ issue_enrichment:
- 'good-first-issue'
- 'help-wanted'
labeling:
# Issue labels are safe to auto-apply (issue-event != PR-event, no review
# retry-loop risk on issues).
auto_apply_labels: true
Loading