Repository navigation
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
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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:
|
|
…lines don't count (#3163) ### What does this PR do? Type of change: documentation Softens the PR size rule in the review baseline (`.agents/review-guidelines/code-review.md`, focus area 3) and in `AGENTS.md` ("Sizing and splitting PRs"). The two change together because the baseline defers to `AGENTS.md`. - **~500 is a guideline, not a hard limit.** Both files now say so. A PR a little over it needs no split ask, and "over the budget" means clearly over it. - **Comments and blank lines don't count, even in source files.** The baseline excludes blank lines and comment-only lines (docstrings and block comments included) from the core-logic count. `AGENTS.md` says the same, and now lists config next to tests and docs, as the baseline already does. Its rough-count command leaves out blank lines and the test, config and docs paths the baseline excludes (at any depth, including `test_*.py`, `*_test.go` and `*.spec.*` file names). It reads each file's path from the diff's `+++` line, so names with spaces or quotes are classified correctly, and it turns off external diff helpers and text-conversion filters (`diff.external`, `GIT_EXTERNAL_DIFF`, `textconv`), which would otherwise change the patch it reads. It keeps comments and docstrings, so it is a true upper bound; the text says to subtract those by eye when the result is close to ~500. - **The "cohesive / reads well" statement is softened.** "the presumption is always that it can be split, and "cohesive", "reads well" and "nothing else wrong" don't rebut it" becomes "the presumption is that it can be split, and "cohesive", "reads well" and "nothing else wrong" don't, on their own, rebut it". Everything else is unchanged: the ~500 figure, what else is excluded (tests, config, docs, deleted lines), the three exceptions, the split ask and slicing guidance, and the decision rules for a PR that is clearly over (a `nudge` at best until it is split or an exception applies). The branch history has an earlier, broader rewrite that a later commit undoes. The net diff against `main` is the three changes above. ### Usage N/A (guidance only). ### Testing Docs only, no code change. `pre-commit` (including markdownlint-cli2) passes. The count command in `AGENTS.md` was run exactly as written on a small throwaway repo: a Rust attribute, a multiline Python string with a leading `#`, a Python continuation line starting with `//`, C macro definitions, an unindented `++counter;`, an added line that looks like a diff header, config and docs files, nested `tests/` and `docs/` directories, test-named files outside `tests/`, file names with spaces, quotes, non-ASCII characters and a tab, a production file under a `.spec.`-named directory, and a binary file. It gave the expected count for each of 36 files, and for the whole diff, under gawk, mawk and busybox awk, with and without forced git color, diff prefix and `core.quotePath` settings, and with `diff.external`, `GIT_EXTERNAL_DIFF`, a per-file diff `command` driver and a `textconv` driver on every `*.py` set to a helper that prints nothing. It was also run on a recent range of `main`. ### Before your PR is "*Ready for review*" Make sure you read and follow [Contributor guidelines](https://github.com/NVIDIA/Model-Optimizer/blob/main/CONTRIBUTING.md) and your commits are signed (`git commit -s -S`). Make sure you read and follow the [Security Best Practices](https://github.com/NVIDIA/Model-Optimizer/blob/main/SECURITY.md#security-coding-practices-for-contributors) (e.g. avoiding hardcoded `trust_remote_code=True`, `torch.load(..., weights_only=False)`, `pickle`, etc.). - 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?: ❌ (not run: docs-only, and the reviewer reads the baseline from the base commit, so it would apply the current rule to this PR) ### Additional Information This changes team-wide review policy, so it needs sign-off from the owners of the current wording: the size budget came in with #2616 and the shared baseline with #3149. It takes effect for PRs reviewed after it merges, because the reviewer reads the baseline from the PR's base commit. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Documentation** * Clarified that the approximately 500-line PR sizing target is guidance, not a strict limit: slightly exceeding it is acceptable, while clearly oversized PRs should generally be split unless they cannot reasonably be divided. * Explained that deleted lines, tests, documentation, configuration, specified file types, blank lines, and comment-only lines—including docstrings and block comments—are excluded from the estimate. * Updated the suggested counting command to estimate qualifying added source lines and clarified when a split proposal is appropriate. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
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