Consolidate coding standards in CONTRIBUTING.md; tune review automation#1519
Conversation
…tion - Merge .agents/developer-guidelines.md into CONTRIBUTING.md as a new "Coding standards" section plus "Test design principles" under the existing test section. Universal coding rules are now discoverable to human contributors, not just agents. - Add new principles: imports at top of file (in-function only for circular imports or optional deps), and __all__ + `from .module import *` in package __init__.py for an explicit public API surface. - Update AGENTS.md to point at the merged location and move the agent-specific "use relative paths" rule into it. Drop the now-redundant reference in the /claude review workflow prompt. - Tune /claude review: read prior comments to avoid duplicate findings, add brief thoroughness directives (per-file category coverage, cross-file dataflow trace for new public symbols), and replace the ambiguous approval rule with an explicit decision table — SUGGESTIONs alone never block approval. - Add a tests/**/*.py path_instruction in .coderabbit.yaml encoding the imports-at-top rule, lean-tests guidance, and correct test-directory placement. Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
📝 WalkthroughWalkthroughConsolidates agent developer guidelines into CONTRIBUTING.md, updates AGENTS.md references, enables stricter CodeRabbit test-path rules, and tightens the Claude review workflow (timeout, prompt ordering, single-pass rules, symbol tracing, and approval policy). ChangesCoding Standards Consolidation and Review Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1519 +/- ##
=======================================
Coverage 76.92% 76.92%
=======================================
Files 474 474
Lines 51503 51503
=======================================
Hits 39618 39618
Misses 11885 11885
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- .coderabbit.yaml: enable reviews.request_changes_workflow so CodeRabbit formally approves PRs once its comments are resolved and pre-merge checks pass. Despite the name, this setting never submits a blocking "Request changes" review. - claude_review.yml: replace --request-changes branch in the approval decision rule with --comment, and add an explicit directive that Claude review is advisory and must not block merges. Also drop the "breadth over deep dives" line, which conflicted with the job's stated purpose of deep architectural analysis. Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Docs + automation-config PR. Verified:
CONTRIBUTING.mdcleanly absorbs the contents of.agents/developer-guidelines.md, plus two new principles (imports-at-top with explicit exception list;__all__+from .module import *for package re-exports) and aTest design principlessubsection. No content lost.AGENTS.mdlink updated toCONTRIBUTING.md#-coding-standards(GitHub strips the 📐 emoji from the slug, anchor resolves correctly), and the "use relative paths" rule moved here as agent-specific guidance..agents/developer-guidelines.mdis deleted; no other repo files still reference it (claude_review.yml's old reference is removed in this PR;.agents/TOOLING.mdis unaffected).claude_review.yml: timeout 10→15min (reasonable given the added "read prior comments" + cross-file dataflow steps), approval rule is now an explicit count-based gate (0 CRITICAL AND 0 IMPORTANT → --approve, otherwise--comment— never--request-changes), matching the PR's stated "advisory, never blocking" intent..coderabbit.yaml:request_changes_workflow: trueis the documented switch to let CodeRabbit formally approve once comments resolve; newtests/**/*.pypath_instruction encodes the imports-at-top + lean-tests + correct-test-dir rules consistent with the new CONTRIBUTING.md content.- No source/code/licensing changes; "tests N/A" is appropriate.
Minor observation (non-blocking): the new __all__ + star-import re-export convention is a meaningful repo-wide expectation; a follow-up audit of existing __init__.py files for conformance would be worthwhile, but doesn't need to gate this docs PR.
|
…on (#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 --> [](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>
What does this PR do?
Type of change: documentation + tooling
Consolidates the previously agent-only coding guidelines into
CONTRIBUTING.mdso both human contributors and AI agents work from the same source of truth, and tunes the/claude reviewand CodeRabbit automation so they are advisory (approve-only, never blocking).Docs reorg
.agents/developer-guidelines.mdintoCONTRIBUTING.mdas a new📐 Coding standardssection plus aTest design principlessubsection under the existing test section. Delete the now-redundant file.__all__+from .module import *in package__init__.pyto make the public API surface explicit at the definition site and keep star-imports safe.AGENTS.mdto point at the merged location and move the agent-specific "use relative paths" rule into it. Drop the dangling reference inclaude_review.yml.AI Review automation
/claude review(.github/workflows/claude_review.yml):0 CRITICAL AND 0 IMPORTANT → approve, regardless of SUGGESTION count. SUGGESTIONs alone never block approval.--commentreview instead. Claude review is advisory and must not block merges..coderabbit.yaml):reviews.request_changes_workflow: trueso 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.tests/**/*.pypath_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 runran clean on both commits (markdownlint, YAML format, etc.)./claude reviewworkflow changes will be validated on the next PR that triggers it.tests/**/*.pyrule and approval behavior will be exercised on the next PR touching tests.Before your PR is "Ready for review"
CONTRIBUTING.md: N/A/claude reviewafter PR opens)Additional Information
The previous
.agents/developer-guidelines.mdcontent was not agent-specific — it described universal coding standards. Hiding it under.agents/discouraged human contributors from reading it. The merge intoCONTRIBUTING.mdmakes 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_checkssecurity gates remain the hard blockers.Summary by CodeRabbit
Documentation
Chores