Skip to content

Give the reviewer a line budget for non-test code - #149

Merged
tzhouam merged 2 commits into
mainfrom
agent/review-rule-diff-budget
Sep 11, 2026
Merged

tzhouam merged 2 commits into
mainfrom
agent/review-rule-diff-budget

Conversation

@tzhouam

@tzhouam tzhouam commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Purpose

Owner rule request: a PR's code diff excluding tests should not exceed ~1k lines.

REV-3b in knowledge/general/review/rules.md states it, and says the part that is easy to get wrong -- where the number comes from. The host supplies it; the reviewer reports it. A model counting the diff itself would be counting a patch that is truncated for exactly the large files the budget exists for. The enforced counter for the bot's Direct lane is omni-reviewbot#57.

Outside the budget: tests, documentation, lock files and pure renames. Deletions never count. So the rule cannot be satisfied by writing fewer tests, and a cleanup is not punished for its size.

Over budget, the review asks for a split plan or a stated exemption (vendored sync, generated code) on the largest contributing file. A PR body that already gives one is recorded instead. Size stays non-blocking and never raises another finding's severity.

Coverage matches #148: DIRECT_REVIEW_CHECKLIST for Direct consumers, strict-review-checklist.md for Strict (now 7,714 chars against the 8,000 cap raised in #148 -- 286 characters of headroom left, so the next addition to that page needs a trim or another bump).

The contradiction this had to resolve

agent.assess_pr_quality's prompt said in as many words that a large diff is never sufficient by itself -- it would have argued against this rule every time it fired. It now records volume above the budget as a concerns reason in its own right, while the existing code path that requires two grounded reasons keeps size alone from reaching needs_rework. A test pins both halves so they cannot drift apart again.

That assessor never sees rules.md, so its prompt now carries the exclusions and the already-justified exemption itself. Without that, a docs-only PR could collect a size concern for a budget it cannot exceed. (Both points came from the commit review gate; the second caught a real error in my first wording, which said a deletion-heavy PR cannot exceed the budget -- 1,500 added lines are over budget beside any number of removed ones.)

SPEC freshness

engine/steps/review.md said size may never be a needs-rework reason on its own -- still true, and still enforced in code -- but it now also records that volume can stand alone as a concerns reason, and why the exclusions travel in the prompt rather than by reference to the knowledge tree.

Test Plan

PYTHONPATH=$PWD/src python -m pytest -q
python knowledge/tools/check_knowledge_tree.py
python knowledge/tools/check_wiki_lint.py

New coverage: test_quality_prompt_keeps_size_a_concerns_reason_only asserts the budget, the exclusion list, the deletion distinction, the exemption, and that size alone never reaches needs_rework.

Note: PYTHONPATH=$PWD/src is required here -- the shared venv has infermatrix_copilot installed editable against another checkout and this repo's pytest config sets no pythonpath, so a bare pytest silently tests the other tree.

Test Result

Full suite on this branch: 1 failure, test_phase1_cluster.py::test_drift_guard_unimportable_checked_class_fails_guard, identical to origin/main at e40258d3b and to #148.

Knowledge validators: check_knowledge_tree.py 0 errors / 22 pre-existing size reminders; check_wiki_lint.py 0 errors, 0 warnings across 387 pages.

Stacked on #148 (shared checklist page, shared DIRECT_REVIEW_CHECKLIST, and it consumes the 8k injection cap raised there). Merge #148 first.

Not covered: no live review was run, so how often the ask fires on real PRs is unmeasured. The 1,000-line number lives in the rule text on this side and in a code constant on the reviewbot side; there is no single source of truth binding them yet.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JCk5xJGRyydouK7tuhs8kk

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

tzhouam and others added 2 commits September 10, 2026 17:31
A PR body that says tests pass is not evidence that the guard cases CI
runs for the changed feature were executed. The lane selects by marker,
so running a test file directly can prove nothing about what the lane
collects. REV-3a makes that comparison a gate: find the guard steps from
the pipeline definition at the frozen head, compare their commands with
what the PR reports running by selector rather than by literal text, and
treat a missing, unrun or mismatched case as a blocking finding.

The rule reaches every review channel it applies to. Direct consumers get
it through the first-review checklist; the Strict reviewer gets the
repo-shaped form through the injected checklist page.

That injection had no room left. The page was 6,991 characters against a
7,000-character cap, so anything appended would have been cut off
mid-sentence and the gate would have looked present while never reaching
the model. The cap moves to 8,000, and the three places that documented
it as 4k or 7k now agree with the code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JCk5xJGRyydouK7tuhs8kk
Review attention is finite, so REV-3b puts a number on it: non-test
added and modified lines stay under a thousand, and past that the review
asks for a split plan or a stated exemption on the largest contributing
file.

The rule says where the number comes from, because that is the part
easy to get wrong. The host supplies it; the reviewer reports it. A
model counting the diff itself would be counting a patch that is
truncated for exactly the large files the budget exists for. Tests,
docs, lock files and pure renames stay outside the budget, and deletions
never consume it, so the rule cannot be satisfied by writing fewer
tests or punish a cleanup.

The review-readiness prompt said in as many words that a large diff is
never sufficient on its own, which would have argued against the new
rule every time it fired. It now records volume above the budget as a
concerns reason in its own right, while the code path that already
requires two grounded reasons keeps size alone from reaching
needs_rework. A test pins the two halves together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JCk5xJGRyydouK7tuhs8kk
@tzhouam
tzhouam force-pushed the agent/review-rule-diff-budget branch from b72817e to 79e765e Compare September 10, 2026 09:32
@tzhouam
tzhouam merged commit ad9b1d9 into main Sep 11, 2026
3 checks passed
@tzhouam
tzhouam deleted the agent/review-rule-diff-budget branch September 11, 2026 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant