Add guidance on sizing and splitting PRs to AGENTS.md - #2494
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/Model-Optimizer/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughAGENTS.md expands exceptions to the approximate 500-line source-change budget and makes aggregate draft pull requests optional when splitting harms comprehensibility. ChangesPR sizing guidance
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~4 minutes Change: Other Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review (claude-opus-5) — DM the bot to share feedback.
Nudge — the docs-only addition is clean and doesn't duplicate anything in CONTRIBUTING.md or the skills tree, but the final bullet reads as a mandate that contradicts an earlier rule.
Needs action:
- Reconcile the last bullet in
AGENTS.md("Submit the whole change as a draft PR") with the PR body, which calls it optional, and with the earlier "don't open the big one and ask afterwards" rule — as written an agent will always open the oversized draft. - Confirm the ~500-line budget is the number the eng meeting agreed on; this repo regularly lands cohesive 1000+ line PRs (recipe renames, ONNX capability splits), so the "cannot be split" exception may need to name those cases.
No action needed:
- No licensing, API, or test surface is touched;
pre-commit/markdownlint concerns are covered (MD013 is disabled). CLAUDE.mdis a symlink toAGENTS.md, so the new section propagates without a second edit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2494 +/- ##
=======================================
Coverage 71.14% 71.14%
=======================================
Files 603 603
Lines 66739 66739
=======================================
Hits 47482 47482
Misses 19257 19257
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks — pushed f26e5c7 addressing the first item; pushing back on part of the second. Reconcile the draft-PR bullet — addressed. You're right that it read as a mandate: an agent would have opened an oversized reference draft on every PR, including ones that were never split. The last bullet now reads "When a split makes the whole hard to follow, the full change may also go up as a draft PR — for reference only, never as a second review request," which scopes it to the split case and removes the tension with "don't open the big one and ask afterwards." The PR body's "optionally" now matches the file. Confirm the ~500-line budget — keeping 500; exception list widened. 500 is the number the repo owner asked for in this change, so I've left it as-is rather than re-deriving it. The premise behind the second half checks out, though. Over the last 100 first-parent commits on
The median PR fits the budget, but the large tail is real and often genuinely atomic — Also worth noting for the 🤖 Generated with Claude Code |
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review (claude-opus-5) — DM the bot to share feedback.
Approve — both prior concerns are settled in the current AGENTS.md, and this stays a docs-only change with no licensing, API, or test surface.
No action needed:
- ✔️ Resolved since the last review: the draft-PR bullet now reads "may also go up as a draft PR … for reference only", removing the conflict with "don't open the big one and ask afterwards"; and the 500-line budget is confirmed as the owner's number with the exception list widened to cover self-contained example/model/backend drops.
CLAUDE.mdis a symlink toAGENTS.md, so the new section propagates without a second edit.
|
### What does this PR do? Type of change: documentation Adds an `## Updating skills` section to `AGENTS.md` (symlinked as `CLAUDE.md`). Skills are loaded into agent context, so each extra line costs tokens every time the skill runs. The new guidance tells the agent to: - Keep skill edits concise: add only what changes agent behavior, and tighten existing text instead of appending more. - Do a final compression pass over the skill diff before opening a PR: drop unnecessary explanations and examples, cut redundancy, and merge overlapping guidance. ### Usage N/A — no API or flag change. ### Testing `pre-commit run --files AGENTS.md` (markdownlint and the other applicable hooks pass). ### 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 <!-- documentation-only change --> - Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?: N/A <!-- agent instructions only, not user-facing --> - Did you get Claude approval on this PR?: ❌ <!-- will run /claude review if reviewers want it --> ### Additional Information Follows #2494, which added the PR sizing guidance to the same file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added guidance for keeping skill updates focused on behavior changes and reviewing edits for unnecessary detail. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chenjie Luo <chenjiel@nvidia.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
### What does this PR do? Type of change: documentation Changes the PR sizing rule in `AGENTS.md` to count only **added source** lines toward the ~500-line budget, instead of total changed lines. Deletions are cheap to review, so a PR that mostly removes code shouldn't be pushed into a split. Tests and docs are excluded too, since every sub-PR has to carry its own tests. The check uses the insertions count from `git diff --shortstat` with a pathspec that excludes `tests/` and `docs/`. ### Usage ```bash git diff --shortstat origin/main...HEAD -- . ':!tests' ':!docs' # N files changed, X insertions(+), Y deletions(-) -> compare X against ~500 ``` ### Testing - `pre-commit run --files AGENTS.md` (markdownlint passes). - Ran the pathspec against recent commits (#2595, #2513) to confirm it drops test and doc lines from the count. ### Before your PR is "*Ready for review*" - Is this change backward compatible?: N/A - 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 - Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?: N/A - Did you get Claude approval on this PR?: N/A ### Additional Information Follow-up to #2494, which introduced the sizing guidance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated review guidance to measure pull request size by added source lines, excluding deletions, tests, and documentation. The guidance retains the recommendation to check the size before opening a review. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chenjie Luo <chenjiel@nvidia.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2 participants
What does this PR do?
Type of change: documentation
Adds a
## Sizing and splitting PRssection toAGENTS.md(symlinked asCLAUDE.md) so AI-assisted work stops producing one giant PR that nobody wants to review.The new guidance tells the agent to:
[x/N]so reviewers know the PR is one slice of a planned split, and link the siblings.Usage
N/A — no API or flag change.
Testing
pre-commit run --files AGENTS.md(markdownlint and the other applicable hooks pass).Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
This PR is itself well under the new budget (26 added lines in one file), so no split applies.
🤖 Generated with Claude Code
Summary by CodeRabbit