Files
Model-Optimizer/.coderabbit.yaml
T
Keval Morabia 8f1529abd3 Consolidate coding standards in CONTRIBUTING.md; tune review automation (#1519)
### What does this PR do?

Type of change: documentation + tooling

Consolidates the previously agent-only coding guidelines into
`CONTRIBUTING.md` so both human contributors and AI agents work from the
same source of truth, and tunes the `/claude review` and CodeRabbit
automation so they are advisory (approve-only, never blocking).

**Docs reorg**
- Move the content of `.agents/developer-guidelines.md` into
`CONTRIBUTING.md` as a new `📐 Coding standards` section plus a `Test
design principles` subsection under the existing test section. Delete
the now-redundant file.
- Add two new coding principles:
- **Imports at top of file** in both source and test files. In-function
imports are reserved for resolving circular imports or guarding optional
dependencies (e.g., TRT-LLM, Megatron-Core), with a brief comment naming
the reason.
- **`__all__` + `from .module import *`** in package `__init__.py` to
make the public API surface explicit at the definition site and keep
star-imports safe.
- Update `AGENTS.md` to point at the merged location and move the
agent-specific "use relative paths" rule into it. Drop the dangling
reference in `claude_review.yml`.

**AI Review automation**
- `/claude review` (`.github/workflows/claude_review.yml`):
- Step 1 of the mandatory workflow now reads prior Claude
comments/reviews so the bot doesn't duplicate already-raised findings.
- Add brief thoroughness directives (per-file category coverage,
cross-file dataflow trace for new/modified public symbols) to push more
issues into the first review pass.
- Replace the ambiguous "approve if no significant issues" line with an
explicit decision rule: `0 CRITICAL AND 0 IMPORTANT → approve`,
regardless of SUGGESTION count. SUGGESTIONs alone never block approval.
- **Never submits a formal "Request changes" review.** When issues are
found, posts a `--comment` review instead. Claude review is advisory and
must not block merges.
- CodeRabbit (`.coderabbit.yaml`):
- Enable `reviews.request_changes_workflow: true` so CodeRabbit can
formally approve PRs once its comments are resolved and pre-merge checks
pass. Despite the name, this setting never submits a blocking "Request
changes" review.
- Add a `tests/**/*.py` path_instruction encoding the imports-at-top
rule, lean-tests guidance, and correct test-directory placement.

### Usage

N/A — documentation and automation configuration.

### Testing

- `pre-commit run` ran clean on both commits (markdownlint, YAML format,
etc.).
- `/claude review` workflow changes will be validated on the next PR
that triggers it.
- CodeRabbit `tests/**/*.py` rule and approval behavior will be
exercised on the next PR touching tests.

### Before your PR is "*Ready for review*"

- Is this change backward compatible?: ✅
- If you copied code from any other sources or added a new PIP
dependency, did you follow guidance in `CONTRIBUTING.md`: N/A
- Did you write any new necessary tests?: N/A (docs + workflow config
only)
- Did you update
[Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?:
N/A
- Did you get Claude approval on this PR?: ❌ (will run `/claude review`
after PR opens)

### Additional Information

The previous `.agents/developer-guidelines.md` content was not
agent-specific — it described universal coding standards. Hiding it
under `.agents/` discouraged human contributors from reading it. The
merge into `CONTRIBUTING.md` makes it discoverable on the standard
GitHub PR-creation flow.

The shift to approve-only automated reviews keeps the signal (approvals
when clean, inline comments when issues are found) without giving the
bots formal merge-blocking authority — the existing pre-merge
`custom_checks` security gates remain the hard blockers.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Enhanced contribution guidelines with comprehensive coding standards
and new test design principles.
* Updated agents guidance to remove the legacy developer-guidelines
reference.

* **Chores**
* Enabled formal "request changes" workflow and added test-path review
rules in CI config.
* Refined GitHub Actions review workflow (timeout, review prompt,
single-pass requirements, and approval criteria).
  * Removed legacy developer-guidelines file.

<!-- review_stack_entry_start -->

[![Review Change
Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/NVIDIA/Model-Optimizer/pull/1519?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)

<!-- review_stack_entry_end -->

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
2026-05-19 21:34:31 +05:30

59 lines
3.3 KiB
YAML

# Reference: https://docs.coderabbit.ai/getting-started/yaml-configuration
language: "en-US"
reviews:
profile: chill
collapse_walkthrough: true
poem: false
# Allow CodeRabbit to formally approve once its comments are resolved and pre-merge checks pass
request_changes_workflow: true
path_instructions:
- path: "modelopt/**/*.py"
instructions: &security_instructions |
Review all modelopt package and examples Python changes against the security coding practices in
SECURITY.md. Flag any of the following as CRITICAL security issues,
request changes, and fail the check if ANY are present:
1. torch.load(..., weights_only=False) with no inline comment justifying why it is safe
(e.g. confirming the file is internally-generated and not user-supplied).
2. numpy.load(..., allow_pickle=True) with no inline comment justifying why it is safe.
Should expose allow_pickle as a caller-configurable parameter defaulting to False, not hardcode True.
3. trust_remote_code=True hardcoded for transformers model or tokenizer loading.
Code should expose it as a caller-configurable parameter defaulting to False, not hardcode True.
4. eval() or exec() on any input that could originate from outside the process.
5. Any use of "# nosec" comments to bypass Bandit security checks is not allowed.
If a security-sensitive pattern is genuinely necessary, the PR must be reviewed and approved
by @NVIDIA/modelopt-setup-codeowners with an explicit justification in the PR description.
6. Any addition of new PIP dependencies in pyproject.toml or requirements.txt that are not
permissive licenses (e.g. MIT, Apache 2) must be reviewed and approved by
@NVIDIA/modelopt-setup-codeowners with an explicit justification in the PR description.
- path: "examples/**/*.py"
instructions: *security_instructions
- path: "tests/**/*.py"
instructions: |
Verify tests follow the conventions in CONTRIBUTING.md. Flag the following as
IMPORTANT issues:
1. Imports inside functions or test methods without explicit justification.
Imports belong at the top of the file so import errors surface at collection
time, not mid-test. The only acceptable in-function imports are for circular
imports or optional dependencies (e.g., TensorRT-LLM, Megatron-Core), and
those should carry a brief comment naming the reason.
2. Redundant lower-level tests that duplicate behavior already covered by a
higher-level test — checked-in tests should be lean and document expected
behavior, protect against regressions, or flag backward-incompatible changes.
3. Tests placed in the wrong directory for their cost profile (e.g., multi-minute
tests under tests/unit, which targets a few-seconds budget; GPU-requiring
tests under tests/unit instead of tests/gpu*).
auto_review:
auto_incremental_review: true
drafts: false
base_branches: ["main", "release/.*", "feature/.*"]
pre_merge_checks:
custom_checks:
- name: "Security anti-patterns"
mode: "error"
instructions: *security_instructions
knowledge_base:
code_guidelines:
filePatterns:
- "CONTRIBUTING.md"
- "SECURITY.md"