ci: refine CodeRabbit review guide to match cuOpt idioms - #1139
Merged
Conversation
Tightens CodeRabbit AI review configuration to reduce noise and sharpen signal, based on what the repo actually does rather than generic guidance. - Narrows the blanket IGNORE list; separately enumerates items already enforced by pre-commit (clang-format, ruff, pydocstyle, shellcheck, verify-copyright, verify-hardcoded-version, rapids-dependency-file-generator) so CodeRabbit stops duplicating CI noise. - Anchors C++ guidance on the Google C++ Style Guide by rule name (the .clang-format is already BasedOnStyle: Google), and documents cuOpt's deviations: exceptions ARE used (cuopt_expects / CUOPT_EXPECTS in cpp/include/cuopt/error.hpp), column limit is 100. - Adds cuOpt-specific CUDA idioms: RAFT_CUDA_TRY for error checks, rmm::device_uvector over raw cudaMalloc/cudaFree, streams from raft::handle_t::get_stream(), prefer thrust/CCCL over hand-rolled. - Adds Python, Cython, and C-API sections that did not exist. - Collapses duplicate rules (stream lifecycle, problem-context confusion, phase initialization were each repeated 3-6 times) so each rule lives once in the severity matrix, with cross-cutting lessons captured in Common Bug Patterns with red-flag bullets. - Merges Output Format and Token Optimization sections. - Replaces the contradictory "ONLY CRITICAL/HIGH" framing and the unmeasurable "sub-3% false positive" target with a quality gate. - .coderabbit.yaml: expands path_instructions from 2 to 11 narrowly scoped entries (cu/cuh, cpp/src host, grpc, cuopt_c.h, cpp tests, python, pyx, pxd, server, python tests); adds path_filters to exclude thirdparty/, datasets/, regression/, notebooks/, and problem-data files. - Adds CONTRIBUTING.md and CONVENTIONS.md to the knowledge base so CodeRabbit sees the same standards humans do. Net: review guide shrinks from 469 to 307 lines while gaining Python/Cython/C-API coverage and repo-specific rules. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
ramakrishnap-nv
added
non-breaking
Adds coverage for a class of bug the previous guide did not mention: a PR references a file, script, or symbol that is not actually in the diff or in the base branch (commonly a forgotten `git add`). - Review guide: new CRITICAL "Build / dependency integrity" bullet and a new Common Bug Pattern §7 with red flags across four categories: source & build (#include, CMake, imports, Cython cimport, renamed symbols), CI / scripts / infra (workflow `run:` steps, shell `source`, Dockerfile COPY, helmchart, dataset scripts, Sphinx includes), and dependencies (dependencies.yaml, pyproject, requirements). - .coderabbit.yaml: adds path_instructions for .github/workflows/**, ci/**/*.sh, datasets/**/*.sh, **/Dockerfile*, helmchart/**, plus **/*.cmake — each focused on cross-checking referenced files/paths exist in the PR. - Narrows the datasets/ and regression/ excludes: shell scripts in those directories are now reviewed (they download test data and are worth checking); only the bulk data formats are filtered out. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Prevents false positives on tests referencing problem data (MPS, QPS, LP, MIPLIB, PDLP) that is downloaded at test time, not committed. The repo's convention is that datasets are fetched by scripts under datasets/ before tests run (datasets/get_test_data.sh, datasets/linear_programming/download_pdlp_test_dataset.sh, datasets/mip/download_miplib_test_dataset.sh) and resolved at runtime via RAPIDS_DATASET_ROOT_DIR — see cpp/tests/utilities/common_utils.hpp (get_rdrd_or_default) and the os.getenv usage in python/cuopt/cuopt/tests/linear_programming/test_python_API.py. Without this carve-out, Common Bug Pattern §7 would fire on every test that references a dataset path — a high false-positive rate. - Review guide §7: drops the blanket "tests referencing datasets not in the PR" red flag and adds a "When NOT to flag" subsection listing the three download scripts, the RAPIDS_DATASET_ROOT_DIR contract, and the precise conditions under which a reference SHOULD be flagged (new filename not present in any download script; stale name after rename/removal; typo). - .coderabbit.yaml: adds the dataset-exemption note to the cpp/tests and python/**/tests path_instructions so it applies scoped to where it matters. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds path-based review filters and many narrow, per-path review instructions in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 262-263: The knowledge_base list in .coderabbit.yaml currently
references "CONVENTIONS.md" which doesn't exist; either remove the
"CONVENTIONS.md" entry from the knowledge_base array or add a new CONVENTIONS.md
file to the repo root containing your project's coding conventions and ensure
the filename/path matches exactly; update .coderabbit.yaml to reflect the change
and commit the new file or config so the knowledge_base no longer points to a
non-existent resource.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4be7e1e1-f3c0-46e2-8b23-34ffb8a5d270
📒 Files selected for processing (2)
.coderabbit.yaml.github/.coderabbit_review_guide.md
Comment on lines
+262
to
+263
| - "CONTRIBUTING.md" | ||
| - "CONVENTIONS.md" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if CONVENTIONS.md exists at repo root or common locations
fd -i 'CONVENTIONS.md' --type fRepository: NVIDIA/cuopt
Length of output: 38
Remove CONVENTIONS.md from the knowledge base or create the file.
CONVENTIONS.md does not exist in the repository. Adding a non-existent file to the knowledge_base may cause CodeRabbit to log a warning or silently skip it. Either create this file with the project's coding conventions or remove the reference from lines 262-263.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yaml around lines 262 - 263, The knowledge_base list in
.coderabbit.yaml currently references "CONVENTIONS.md" which doesn't exist;
either remove the "CONVENTIONS.md" entry from the knowledge_base array or add a
new CONVENTIONS.md file to the repo root containing your project's coding
conventions and ensure the filename/path matches exactly; update
.coderabbit.yaml to reflect the change and commit the new file or config so the
knowledge_base no longer points to a non-existent resource.
Collaborator
Author
There was a problem hiding this comment.
Think it is there https://github.com/NVIDIA/cuopt/blob/main/CONVENTIONS.md
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check current branch
git branch --show-current
# Check if CONVENTIONS.md exists on current branch
ls -la CONVENTIONS.md 2>&1
# Check if it exists on main
git show main:CONVENTIONS.md > /dev/null 2>&1 && echo "EXISTS on main" || echo "NOT on main"
# Check with fd more broadly (case-insensitive)
fd -i 'conventions' --type f
Length of output: 88
@rgsl888prabhu, you're right — CONVENTIONS.md exists and is a symlink to AGENTS.md in the repository. My initial fd scan missed it because fd skips symlinks by default (the correct invocation would have needed the -L / --follow flag). The knowledge_base reference is perfectly valid and no change is needed. Apologies for the noise!
🧠 Learnings used
Learnt from: rgsl888prabhu
Repo: NVIDIA/cuopt PR: 1098
File: ci/run_ctests.sh:44-44
Timestamp: 2026-04-23T15:41:58.054Z
Learning: In these CI shell scripts—ci/run_ctests.sh, ci/run_cuopt_pytests.sh, and ci/run_cuopt_server_pytests.sh—treat `IS_NIGHTLY` as the intentional variable name. It is set from `RAPIDS_BUILD_TYPE` and compared to the string "nightly" using this project’s naming convention; during code review, don’t recommend renaming `IS_NIGHTLY` to something like `BUILD_TYPE` or similar, since it would introduce unnecessary churn.
The previous "Google C++ Style Guide with cuOpt deviations" framing was
biased toward Google: the codebase actually uses snake_case for everything,
_t suffix on types, trailing-underscore private members, and exceptions —
all of which Google C++ disagrees with. With only "exceptions" and
"column 100" listed as deviations, CodeRabbit could legitimately flag
`class my_thing_t { void do_thing(); }` patterns as not following Google
CamelCase — and that pattern is the entire codebase.
This commit reframes the C++ Coding Standards section as cuOpt-first:
- "C++ — cuOpt conventions (the default; match nearby code)" lists the
actual conventions: snake_case + _t suffix, trailing-underscore members,
CUOPT_-prefixed macros, file-extension contract, exceptions via
cuopt_expects/CUOPT_EXPECTS, column 100, formatting handled by clang-format.
- "C++ — language-level practices we follow from Google C++" lists ONLY
the named rules cuOpt actually follows (ownership, explicit, override,
casts, IWYU, no `using namespace`, const/constexpr) and tells CodeRabbit
to cite Google by section name only for these.
- Explicit "where Google disagrees with cuOpt, cuOpt wins" clause covers
naming, exceptions, and column limit.
Updates the cpp/src/**/*.{cpp,hpp,h} path_instruction in .coderabbit.yaml
to match: lists cuOpt conventions first, then names the Google rules to
cite, with a "do NOT cite Google for naming, exceptions, or column limit"
clause.
Net: CodeRabbit reads the surrounding code first; Google C++ is a
fallback for language-level practices, not the rulebook.
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
mlubin
reviewed
Apr 23, 2026
- Add Inheritance and Static/Global Variables to the Google C++ practices list in the review guide (mlubin feedback) - Trim duplicated cpp/src/** path_instructions in .coderabbit.yaml to point at the review guide instead of restating conventions (mlubin feedback on YAML being the wrong home for project-wide style rules) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mlubin
approved these changes
Apr 24, 2026
mlubin
left a comment
mlubin
left a comment
Contributor
There was a problem hiding this comment.
When this lands please notify the team to be on the lookout for improvements or regressions in the CodeRabbit comments.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/.coderabbit_review_guide.md (2)
332-337: Add language specifiers to example code blocks.Static analysis flags these fenced code blocks (and the similar ones at lines 340, 347, 354, 361, 368) as missing a language specifier (MD040). Since these are output format templates rather than executable code, consider using
textorplaintext.📝 Suggested fix for all example blocks
-``` +```text CRITICAL: Device buffer leaks on early return.Apply the same pattern (
text) to the other example blocks at lines 340, 347, 354, 361, 368.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/.coderabbit_review_guide.md around lines 332 - 337, The fenced example blocks that show template outputs (e.g., the block containing "CRITICAL: Device buffer leaks on early return.") are missing a language specifier and trigger MD040; update each of these fenced code blocks to use a non-executable specifier like ```text (apply the same change to the other similar example blocks that contain the output templates) so they are recognized as plaintext in Markdown and static analysis no longer flags them.
49-55: Clarifylogic_errorexample — it lacks the_tsuffix described in the rule.The naming convention states types use
snake_case_twith a_tsuffix, but the first examplelogic_errordoesn't follow this. Iflogic_errorintentionally omits the suffix (e.g., to matchstd::logic_error), consider noting that exception or replacing it with a clearer example likecuopt_error_t.📝 Suggested clarification
- **Naming**: - Types, classes, structs, enums: `snake_case_t` with `_t` suffix - (e.g. `logic_error`, `error_type_t`, `solver_settings_t`) + (e.g. `error_type_t`, `solver_settings_t`, `device_buffer_t`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/.coderabbit_review_guide.md around lines 49 - 55, The naming example uses "logic_error" which contradicts the rule that types use snake_case_t; update the documentation in the naming section to either note that exception types mirroring std::logic_error are exempt, or replace "logic_error" with a compliant example such as "cuopt_error_t"; ensure the doc text around the type naming rule and the examples (mentioning "logic_error" and the suggested "cuopt_error_t") is adjusted so the rule and example are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/.coderabbit_review_guide.md:
- Around line 332-337: The fenced example blocks that show template outputs
(e.g., the block containing "CRITICAL: Device buffer leaks on early return.")
are missing a language specifier and trigger MD040; update each of these fenced
code blocks to use a non-executable specifier like ```text (apply the same
change to the other similar example blocks that contain the output templates) so
they are recognized as plaintext in Markdown and static analysis no longer flags
them.
- Around line 49-55: The naming example uses "logic_error" which contradicts the
rule that types use snake_case_t; update the documentation in the naming section
to either note that exception types mirroring std::logic_error are exempt, or
replace "logic_error" with a compliant example such as "cuopt_error_t"; ensure
the doc text around the type naming rule and the examples (mentioning
"logic_error" and the suggested "cuopt_error_t") is adjusted so the rule and
example are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e892ab24-4819-4695-9897-9945d0742416
📒 Files selected for processing (2)
.coderabbit.yaml.github/.coderabbit_review_guide.md
Iroy30
approved these changes
Apr 24, 2026
Collaborator
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refines the CodeRabbit AI review configuration (
.coderabbit.yaml+.github/.coderabbit_review_guide.md) so reviews match how cuOpt actually looks, not generic best-practice templates.Commits
ci: refine CodeRabbit review guide to match cuOpt idiomsShrinks the review guide from 469 to 307 lines. Splits the blanket
IGNOREinto subjective-taste vs. already-enforced-by-pre-commit (clang-format, ruff, pydocstyle, shellcheck, verify-copyright, verify-hardcoded-version, rapids-dependency-file-generator). Adds a Coding Standards section, CUDA idioms (RAFT_CUDA_TRY,rmm::device_uvector,raft::handle_t::get_stream(), thrust/CCCL), Python, Cython, and C-API sections. Collapses rules that were repeated 3–6 times. Expands.coderabbit.yamlpath_instructions from 2 → 11 narrowly scoped entries and addspath_filtersforthirdparty/,regression/, problem-data files.ci(coderabbit): flag references to files missing from the PRNew CRITICAL rule and Common Bug Pattern §7 catching forgotten
git add:#includeof absent headers, CMaketarget_sourcesof missing files, Python/Cython imports of files not in the PR, workflowrun:steps pointing at ci scripts not present, DockerfileCOPY, helmchart templates, Sphinx includes, dependency-file drift. Adds path_instructions for**/CMakeLists.txt,**/*.cmake,.github/workflows/**,ci/**/*.sh,datasets/**/*.sh,**/Dockerfile*,helmchart/**.ci(coderabbit): exempt runtime-downloaded datasets from §7Prevents §7 from false-positiving on tests referencing MPS/QPS/LP/MIPLIB data. Tests resolve paths via
RAPIDS_DATASET_ROOT_DIR(seecpp/tests/utilities/common_utils.hppand the Pythonos.getenvusage); data is fetched at test time bydatasets/get_test_data.sh,datasets/linear_programming/download_pdlp_test_dataset.sh,datasets/mip/download_miplib_test_dataset.sh. CodeRabbit only flags when a referenced filename appears in none of those scripts, or when a script rename leaves a stale test reference.ci(coderabbit): flip C++ guidance to cuOpt-first, Google as fallbackReframes the C++ Coding Standards section. The previous "Google C++ with cuOpt deviations" framing was biased: the codebase uses
snake_casefor everything,_tsuffix on types, trailing-underscore private members, and exceptions — all of which Google disagrees with. The new framing lists cuOpt's actual conventions first (naming, file extensions, error handling viacuopt_expects/CUOPT_EXPECTS, column 100), and cites Google only for named language-level practices the codebase actually follows (ownership,explicit,override, casts, IWYU,const/constexpr). Explicit "where Google disagrees with cuOpt, cuOpt wins" clause covers naming, exceptions, and column limit.Risk
Low — only affects CodeRabbit's AI reviews. No build, runtime, or CI behavior change. Easy to iterate as we see how reviews read.
Issue
N/A
Checklist