Count only added lines toward the PR size budget in AGENTS.md - #2616
Conversation
Deletions are cheap to review, so a PR that removes a lot of code should not be pushed into a split. Measure the ~500-line budget by insertions from `git diff --shortstat` instead of total changed lines. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their 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: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe PR updates the documented sizing guidance to count about 500 added source lines. It excludes deletions, tests, and documentation from the budget, and changes the measurement command to ChangesPR size guidance
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to This update narrows the size count by excluding tests and docs and introduces no material merge risk. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
meenchen
left a comment
There was a problem hiding this comment.
Bot review (claude-opus-5-5) — DM the bot to share feedback.
Nudge: counting only added lines is the right rule, but git diff --shortstat counts every added line, including tests, config and docs. The budget is meant to cover source only, so the command overcounts.
Needs action:
- Scope the size check in
AGENTS.md(L55–56) to source files. Either add pathspec exclusions, e.g.git diff --shortstat <base>...HEAD -- . ':!tests' ':!docs' ':!*.md' ':!*.rst' ':!*.yaml' ':!*.yml' ':!*.json' ':!*.toml', or add one sentence saying tests, config and docs don't count. Otherwise agents will propose splits for PRs that are mostly tests.
No action needed:
- The doc change is limited to the sizing bullet. The exceptions list and split guidance are unchanged. It has no licensing impact and needs no tests.
| intermediate state. Check the size with `git diff --stat <base>...HEAD` | ||
| before opening. | ||
| intermediate state. Deletions don't count; check the insertions from | ||
| `git diff --shortstat <base>...HEAD` before opening. |
There was a problem hiding this comment.
Bot comment.
--shortstat insertions cover every file, including tests/, YAML/JSON config and .md/.rst docs. The rule above says "lines of source", so this check counts against the budget the test code that every sub-PR is required to carry. Consider either git diff --shortstat <base>...HEAD -- . ':!tests' ':!docs' ':!*.md' ':!*.rst' ':!*.yaml' ':!*.yml' ':!*.json' ':!*.toml' or adding "tests, config, and docs don't count either" to the sentence.
There was a problem hiding this comment.
Fixed in 8b65cf2: AGENTS.md:55-57 now says deletions, tests, and docs don't count, and the check is git diff --shortstat <base>...HEAD -- . ':!tests' ':!docs'. I left out the *.md/*.yaml/*.json/*.toml exclusions: skill changes are mostly .md, so they'd always count as zero, and YAML recipes and launcher configs are reviewed like source.
The budget covers source lines, but `git diff --shortstat` also counted the tests every sub-PR must carry. Exclude tests/ and docs/ via pathspec. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
meenchen
left a comment
There was a problem hiding this comment.
Bot review (claude-opus-5-5) — DM the bot to share feedback.
Approve: the overcounting concern is fixed. The size check now leaves out deletions, tests/ and docs/, and the author gave sound reasons for keeping the other file types in the count.
No action needed.
- ✔️ Resolved since the last review: in commit 8b65cf2, the
--shortstatcheck inAGENTS.mdexcludestests/anddocs/, and the text now says deletions, tests and docs don't count. - Leaving
*.md,*.yaml,*.jsonand*.tomlout of the exclusions is a reasonable choice. Skill edits are mostly.md, and YAML recipes and launcher configs are reviewed like source code. One side effect:README.mdfiles outsidedocs/(for example underexamples/) still count toward the budget. That's a small mismatch with "docs don't count", but not worth blocking on. - This is a doc-only change with no licensing impact, and it doesn't need tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2616 +/- ##
=======================================
Coverage 69.45% 69.45%
=======================================
Files 611 611
Lines 68219 68219
=======================================
Hits 47383 47383
Misses 20836 20836
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:
|
|
What does this PR do?
Type of change: documentation
Changes the PR sizing rule in
AGENTS.mdto 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 fromgit diff --shortstatwith a pathspec that excludestests/anddocs/.Usage
Testing
pre-commit run --files AGENTS.md(markdownlint passes).Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
Follow-up to #2494, which introduced the sizing guidance.
🤖 Generated with Claude Code
Summary by CodeRabbit