Review PRs larger than the size gate #29

Summary

Today a PR over the size gate gets no model review at all. The gate fails with the waivable reason "too large for automatic review" and a maintainer either waives it or asks for a split. This issue changes that: when a bundle is too big, collect drops unchanged full-file content first, then falls back to a partial review of the patches, and a maintainer can raise the limit for one PR with a comment. The estimate also gets calibrated so the number in the comment means real tokens.

Problem

PR #28 in this repo merged with no model review. Its bundle came to about 239k estimated tokens, and 177k of that was unchanged full-file content. The patches themselves were about 63k. So the model skipped a PR whose actual changes would have fit under the gate about twice over.

The estimate is also low. The spec ("Bundle") counts characters of the rendered bundle divided by 4 and calls the gate "150k input tokens". On the two PRs we measured against the usage the API reports back, real tokens ran about 1.7x to 2.3x the estimate, so the "150k" label is misleading. Line-number columns and lockfile hashes tokenize badly, and current models' tokenizer gives about 30% more tokens than earlier ones (token counting).

The model has a 1M-token context. A request over that fails with a 400 that analyze records as a failed pass, and a failed pass is not waivable ("Reasons"). That is worse for the author than a size skip. So any limit we set needs a wide margin under the window, and the margin has to be in real tokens, not in the estimate.

Decisions

These are decided. The open questions at the end are the only parts still in play.

1. Diff-only fallback. When the bundle is over the limit, collect drops full-file content in a fixed order until it fits: added files first (the patch already holds every line), then docs, then tests and fixtures, then the largest files one by one. Every patch always stays, so the model still sees every changed line with its line numbers. The order depends only on paths and sizes, so it is deterministic. Every changed line stays in view. An author can steer which files lose their surrounding context by path and size, so the summary names every file reviewed from the diff only. This is a summary line, not a gate reason. The existing rule that omits content for files of 2,000 lines or more has no gate reason either, and those files go in the same list.

2. Partial review fallback, same release. If the patches alone are still over the limit, collect sends patches in priority order up to the limit (sensitive paths such as workflows, actions, Dockerfiles and Makefiles, then dependency manifests and lockfiles, then source, tests, docs). It cuts only between files, never inside a patch. The gate fails with a new waivable reason, "partial model review", that names every file the model did not see. Never a silent pass. A bundle over the 16 MiB envelope cap is still skipped with the existing reason, because head_content and base_content count against that cap and the content drop does not touch them. A maintainer gets real findings for most of a large PR before they decide on a waiver, where today they get none.

3. Maintainer raise command. A maintainer can raise the limit for one PR with a PR comment (proposed syntax, see open question 1):

   /oc-review size-limit 4x
   

   Multipliers are 2x, 3x and 4x. Max 4x. Permission is checked the same way as an override (write or admin now, from the collaborator permission endpoint), except that the PR author may use it if they have write. Outside contributors cannot raise it, so an outside contributor cannot raise the spend of their own PR. The raise applies to that PR for the current and later pushes (force pushes: see open question 2). It stops counting if the commenter loses write, and the comment must not be edited (same rule as overrides). To apply it, re-run the newest run with the same REST call as an override. A rejected command (no permission, bad syntax, edited) shows in the summary comment with the reason. Today a rejected override comment is silent, which is confusing. Fix that in the same change.

4. Calibrated estimate. The estimate should mean real tokens. Use a divisor that overestimates at the top of the measured range: about chars/1.7 today (the 2.3x case), not chars/2, which undercounts bundles near 2.3x. State the limit in real tokens. The base limit is 200,000 real tokens. 4x is 800,000, which leaves room under the 1M context for the system prompt and the largest max_tokens. In real tokens this base is lower than today's gate (150k chars/4 is about 255k to 345k real), so some PRs that get a full review today will get a diff-only review unless a maintainer raises the limit. The diff-only review still covers every changed line.

5. Log the ratio. The publisher's JSON log line gets the estimate next to the real input usage for each pass, so we can track the ratio over time and tune the divisor without a new API call.

6. Cost bound. The 4x cap bounds the cost of one run. Every push or re-run of a raised PR can cost that again, so the workspace spend limit stays the backstop, as in "Auth and cost".

How it flows

                       maintainer comment
                       /oc-review size-limit Nx
                                |
                                v
   bundle ------------> over 16 MiB envelope cap? --yes--> skipped, existing reason
     |
     no
     |
     v
   limit = base x N (N = 1 by default, max 4)
     |
     v
   fits?  ---yes---> full review
     |
     no
     |
     v
   drop content in order until it fits:
     added files -> docs -> tests+fixtures -> largest files
   (every patch stays)
     |
     v
   fits?  ---yes---> diff-only review
     |               summary lists "K files reviewed from the diff only"
     no
     |
     v
   send patches in priority order up to the limit
   (sensitive paths -> manifests/lockfiles -> source -> tests -> docs)
     |
     v
   partial review
   gate fails, waivable: "partial model review", names the files not seen

Work

• collect: calibrated divisor, tiered content drop, patch priority order, read and check /oc-review size-limit comments (permission, edits) before the bundle is built, pass rejected commands to the publisher, size record with the real-token limit and the multiplier in use
• render: coverage note per file for diff-only and not-reviewed files
• analyze: no change to the call shape, confirm the 4x bundle plus prompts stays under the window
• publish and verdict: "partial model review" waivable reason, summary lines for diff-only and not-reviewed files, rejected command shown with reason (overrides too), estimate next to real input usage per pass in the JSON log line
• command parsing in collect: /oc-review size-limit Nx, same comment rules as override, PR author allowed with write
• spec and decision log: "Bundle", "Reasons", "Override", "Auth and cost", "Publishing (code)", "Testing", one decision log entry
• unit tests: drop order, patches never dropped, priority cut between files only, command parsing and rejection reasons, ratio logging
• one long-context eval case at about 800k real tokens before release. Owner approves the run, because it costs money.

Not in scope

• Splitting a review into chunks. Cross-file findings would land in different chunks, which is a supply-chain blind spot, and the gate's three-pass envelope would change.
• Exact token counting through the API. It would add a new skip path in a trust-sensitive job, with a new failure mode, and the calibrated estimate plus the ratio log gets most of the accuracy for free.
• The Batch API. Most batches finish within an hour, some take up to 24 hours, and the job timeout is 10 minutes.
• A different model for large PRs. Same window, no size gain.
• Per-pass bundles or a leaner line-number layout. Per-pass bundles break the shared cache entry, and the layout change touches citation rendering.

Open questions

1. Exact command syntax. /oc-review size-limit 4x is the proposal. Settle it in the spec.
2. Does a raise survive a force push? Overrides are bound to the push epoch. A raise is bound to the PR, so the simplest rule is that it survives, as long as the commenter still has write. Confirm.
3. How to classify "docs" and "tests" for the drop order: by extension or by directory. Code that runs on import (for example conftest.py) should stay out of the tests tier, since the supply-chain pass looks there.
4. What to do when the first patch in priority order is bigger than the limit by itself: skip it and keep filling (smaller, lower-priority files get in while a sensitive file is out), or stop with zero files reviewed.

🤖 Generated with Claude Code