Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions docs/claude-review-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ Jobs pass data only through artifacts from the same run and the same run attempt
- Dependency delta (see "Dependency delta").
- Prefilter results: `signals[]` and `hard_blocks[]`.
- Coverage record: files in the comparison, files collected, whether both trees were complete, per-file coverage (`full`, `binary`, `unavailable`), and `complete`. The gate treats a record whose `complete` is not `true` as a hard block, even when the prefilter list has no "incomplete coverage" entry.
- Size: estimated token count (characters of the final rendered bundle, its size field included, divided by 4). The size gate is 150k input tokens. Above that, skip the LLM passes. A bundle over the 16 MiB envelope cap is skipped the same way. The gate fails with the reason "too large for automatic review".
- Size: estimated token count (characters of the final rendered bundle, its size field included, divided by 4). The size gate is 150k input tokens. The estimate is of exactly what Claude sees, so no file type counts at less than its rendered size; a long file changed near both ends, such as a lockfile, stays small because its patch is anchored (K-17, "Coverage limits"). Above that, skip the LLM passes. A bundle over the 16 MiB envelope cap is skipped the same way. The gate fails with the reason "too large for automatic review".
- The size gate skips only the Claude calls. All code checks still run. A `skipped-size` bundle keeps the prefilter results, the coverage record and the dependency delta in its payload. It leaves out only the content for Claude. The gate reads the hard blocks from this payload and from the separate prefilter result, and uses both. If the prefilter result is missing, incomplete or for another snapshot, the gate fails, not waivable (K-7).

### Consistent snapshot
Expand Down Expand Up @@ -271,12 +271,13 @@ The review covers one immutable comparison pair: the merge base commit and the h
- A truncated tree, an `unavailable` text file or a decode error is the hard block "incomplete coverage". An override cannot waive it, because the code did not see the content.
- If GitHub rejects an inline comment position (its PR view can differ from the reviewed comparison), that finding goes to the summary. This never changes the gate.
- Every API response and every parser has a byte cap that applies before parsing and before the token gate.
- **Line diff limits** (K-13): collect (patches) and the publisher (inline hunks) use the same bounded line diff (`linediff.py`). It trims the common prefix and suffix, then runs a difflib line diff of the middle only within three limits:
- **Line diff limits** (K-13): collect (patches) and the publisher (inline hunks) use the same bounded line diff (`linediff.py`); only collect adds the anchored step below (K-17). It trims the common prefix and suffix, then runs a difflib line diff of the middle only within three limits:
- `DIFF_BUDGET = 25,000,000`: the product of the two middle lengths.
- `FILE_WORK_BUDGET = 5,000,000`: counted steps for the diff of one file.
- `RUN_WORK_BUDGET = 100,000,000`: counted steps for all files of one collect run, and separately for all files of one publisher run. Every file takes its steps from this one budget.

A step is one line of the index or of a searched range, or one pass of difflib's inner loop, so the count is at least the work that difflib does. The count depends only on the input, never on the clock, and it is checked before each search runs. On low-entropy input (few distinct lines, each repeated many times) difflib's work grows about with the cube of the line count, so the product limit alone does not bound it. Measured on an Apple M-series core: about 1 s per file and 20 s per run at the limits. Long lines raise the cost of one step (hashing and comparing a line), so the times are approximate. A GitHub runner three times slower uses about 60 s of the 10-minute job timeout. Past a limit, the middle is one hunk that removes every base line and adds every head line (a larger patch than git's, never a wrong one), and in the publisher the file has no hunks (see "Inline eligibility"). A file past a step limit is charged all the steps it was allowed. The job log names the file and the limit. A middle with only added or only removed lines needs no diff, so no limit applies to it. A coarse diff never clears a block and never lowers risk.
- **Anchored patch past the size budget** (K-17): in collect only, a middle past `DIFF_BUDGET` is first split at anchors: the lines that occur exactly once in the base middle and once in the head middle, in the longest run that keeps their order on both sides (the "patience" anchors). Each gap between two anchors gets the bounded diff above, with the same three limits; a gap past a limit is one hunk that removes its base lines and adds its head lines. Anchoring costs one step per line of both middles plus one per line of the shorter middle, from the same file and run work budgets; when the file cannot pay that, the middle is one hunk as before and nothing is charged. A gap past a work limit charges the file its whole allowance. The result is a correct edit script (every line outside an equal run is shown as removed or added), so Claude still sees every changed line. It reads only the line text, never the path, so a file named like a lockfile, in any directory, gets the same patch as any other file with the same content. A middle with no shared unique line (for example a full rewrite) is still one hunk, so a truly large change still trips the size gate. The publisher does not anchor: past the size budget a file still has no inline hunks. Measured on PR 2027 in `OperationCode/front-end` (`pnpm-lock.yaml` of about 16,000 lines, changed near both ends): the patch went from +16,069/-16,091 lines to +1,127/-1,149 (GitHub shows +1,131/-1,153), in about 250,000 steps and 0.02 s on an Apple M-series core.
- **Prefilter line diff:** the prefilter does not use `linediff.py`. It has its own line diff (a Myers shortest edit script) to find the "changed lines" (see the notes below the prefilter table). It has its own budgets in steps, `DIFF_WORK_BUDGET = 2,000,000` per file and `RUN_DIFF_WORK_BUDGET = 40,000,000` for all files of one prefilter run, which is one collect run. Every file takes `min(per file, left)` from the run budget, and a file past a limit is charged all it was allowed. The run budget is about 7 s on an Apple M-series core and about 20 s on a GitHub runner, approximate for the same reason. A file's work is bounded by its allowance plus one run of matching lines (at most the file's line count), not exactly by the allowance. Past either limit every head line between the common prefix and suffix counts as changed, and the log names the file and the limit (the run limit when both apply). This changes no gate rule.

## Dependency delta
Expand Down Expand Up @@ -985,3 +986,4 @@ P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line,
- P3-5. Summary comment redesigned for fast human judgment (owner and coordinator, 2026-10-27). The comment now opens with a result heading and one colored callout, then a findings table, a checks strip and a provenance line, with the waiver and the full record collapsed ("Summary comment" under "Publishing (code)"). The owner chose the Fable design with these rulings: every fail uses the red `[!CAUTION]` callout and the red heading, waivable or not (a waivable fail on a fake check was misread as a mild warning under `[!WARNING]`, so `[!WARNING]` is not used for a fail); the waiver line stays in the collapsed Waiver block; model summaries and token counts stay in the collapsed record; duplicate findings merge only at the same file, side, line and gate effect; a medium finding in a blocking category reads "Does not block"; the visible line shows the 7-character head SHA and the record shows the full SHAs; emoji are allowed, each next to its word. Machine markers, the override line format, the size limit and the overflow path are unchanged.
- P3-6. Account X results and Sonnet release (owner and coordinator, 2026-10-28). Account X is `irvingpop2`, a separate GitHub account outside the org that the owner made as a test identity; it became an outside collaborator when it got repo access. Steps ran from an isolated container. V8 b pass (PR 38): the central run started on a first-time-contributor fork PR with no approval click, and marking the draft ready started no new run. V8 a pass (PR 39): from a first-time fork, GitHub holds the fake `pull_request` job (`action_required`), so it cannot start before the central run; after a maintainer approved only that run it passed, and the box stayed BLOCKED and followed the central run; the fake-first case stays covered by PR 36. V23 pass (PR 40): a same-path `claude-review.yml` in the fork PR did not replace the central run, which failed on the bidi hard block; the fork run stayed `action_required` and never ran, so the running case is covered by PR 35. V6 f as a Write user: CONFIRMED residual (PR 41); deleting the newer run reopened the box on an older pass, and it stays in the residuals the owner must accept before org-wide enforcement. A Write user's plain merge of a failed PR was refused; this does not test the bypass, and the `--admin` attempt as Write was not run. V25 pass for Maintain and Admin, Write bypass NOT RUN: with the TR bypass list, a Maintain user (PR 42) and an Admin user (PR 43) merged a failed PR; GitHub records rule suite result `bypass` with the user ID as actor and no role ID, and the org rule-suites endpoint returns 403 on this plan, so use the repo endpoint; a direct push to main by Maintain is refused (GH013). The TR bypass list is a test-only setting and goes back to empty after phase 3 (owner's choice pending). Process: the owner ran the bypass merges and the direct push by hand, because the automation's safety classifier blocks bypass merges; the evidence comes from the GitHub API. Sonnet release: release evals 13 of 13 at M3 ([run 40592861233](https://github.com/OperationCode/.github/actions/runs/40592861233)); the live run [40593729924](https://github.com/OperationCode/claude-review-test/actions/runs/40593729924) passed with `claude-sonnet-5-5`. Console read-back: the owner read back R1 at 590a387 and R2 at 1cefe42 on 2026-10-27, before the M3 edit; R1 was then moved to M3; the service account and the workspace were not read back. V26 a is done for `job_workflow_ref`. NOT RUN, stays open: the Write `--admin` bypass attempt and V26 c deny at M2 before the R2 edit. Web UI "Re-run all jobs": pass (run 40628870689, PR 43, 2026-10-28): the click made attempt 2 of the same run ID, which re-read live state and updated the one summary comment; the merge box was not observed because the PR was already merged.
- P3-7. Release M4, the comment redesign (owner and coordinator, 2026-10-28). M4 is `9e2795b97b22869b9e5c7a777e71a2afa8366eeb` on main (code from PR 12 `a410ddd`, docs PR 13). There is no separate candidate branch: the merge SHA was validated directly. Unit tests: `test.yml` at M4, [run 40710532524](https://github.com/OperationCode/.github/actions/runs/40710532524), success. Release evals: `eval.yml` at M4, [run 40713029193](https://github.com/OperationCode/.github/actions/runs/40713029193), 13 of 13 matched, success; the coordinator approved the run under the owner's delegation through phase 4. Console: the owner edited R1 `claude-review-prod-r1` (`fdrl_01N2ZktTiVKqUvvXwywBFfys`) so that `job_workflow_ref` is `OperationCode/.github/.github/workflows/claude-review.yml@9e2795b97b22869b9e5c7a777e71a2afa8366eeb`. The owner reported "rule updated" and shared a screenshot of the rules list; the rule detail value was not read back. TR (org ruleset 25835887) moved from M3 `2c63f49b461feac74c2c70c4f86ca90040df6a0a` to M4; read back: `repository_id` 1432342572, path `.github/workflows/claude-review.yml`, `sha` `9e2795b`, no `ref`, bypass list empty, enforcement active. Live checks in claude-review-test: PR 28 reopened, [run 40713506554](https://github.com/OperationCode/claude-review-test/actions/runs/40713506554) at M4, success, all four jobs `ok`, summary comment in the new layout (green PASS heading, TIP callout, checks strip). PR 40 reopened, [run 40713856249](https://github.com/OperationCode/claude-review-test/actions/runs/40713856249), failure, red FAIL heading, CAUTION callout, the hard block row (bidi) first, merged duplicate rows with "Found by" listing both passes. Both PRs were closed again. Console rules in the owner's screenshot: `claude-review-p3-r2`, `claude-review-prod-r1`, `eval-federation-rule`, `claude-reviewer-test`; the coordinator advised archiving `claude-review-p3-r2`. Deviation 1: no new federation rule for this release; R1 was edited in place, because the merged code names R1 (same as the phase 3 release, where R1 moved from S1 to M3). Cost: M3 can no longer get a token, so a rollback to M3 needs another Console edit. Deviation 2: the denied exchange at M4 before the Console edit (release step 5a) was NOT RUN, because the owner made the edit before the check. M4 is the current production SHA and the current pin.
- K-17. Anchored patch for long files changed near both ends (pilot false skip, 2026-10-29). PR 2027 in `OperationCode/front-end` (2 files, `package.json` and a `pnpm-lock.yaml` of about 16,000 lines, GitHub +1,131/-1,153) got "too large for automatic review", so a routine dependency update got no model review. Cause: the changes span both ends of the lockfile, so its middle is about 16,000 by 16,000 lines, past `DIFF_BUDGET` (25,000,000). The patch fell back to one hunk with every base line removed and every head line added (+16,069/-16,091). Rendered with line numbers, the bundle estimated 451,544 tokens against the 150,000 limit. A plain difflib diff of that middle needs about 82 million counted steps, past the 5 million per-file budget, so raising `DIFF_BUDGET` alone does not help. Now collect splits such a middle at lines that occur once on each side and diffs only the gaps, within the same budgets ("Coverage limits"). Result for PR 2027 at the same blobs: +1,127/-1,149, estimate 93,786 tokens with the main-branch parser (the head lockfile does not parse there, so the delta is small), 115,961 with the K-16 parser and registry data for all 229 changed packages. Rejected: (b) counting lockfiles at a fine-diff or capped size for the estimate, because the gate must measure what Claude is sent: Claude would still get the 450k-token rendering, and a path rule would let a PR shrink the estimate of any content by its file name. (c) Sending a code-built package delta instead of the lockfile patch: it needs a path or parse rule to pick the files, so content in a lockfile-named file could be hidden from the model passes, and the supply-chain pass would lose the raw lines it reads today. The anchored patch reads no path, never drops a changed line, and leaves the size gate fail-closed: a full rewrite has no anchors and is still one hunk. It changes no gate rule and lowers no risk.
10 changes: 7 additions & 3 deletions review/src/oc_review/collect.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,10 @@
# Past this product of middle lengths, or past the work budget of the file or
# of the run (linediff), the patch is one hunk that removes every base line and
# adds every head line between the common prefix and suffix: a larger patch
# than git's, never a wrong one.
# than git's, never a wrong one. Past this product the middle is first split at
# lines that occur once on each side (linediff `anchored`, K-17), so a long
# file changed near both ends (a lockfile) keeps a small patch; only the gaps
# that are still past a limit are one hunk each.
PATCH_DIFF_BUDGET = linediff.DIFF_BUDGET
NO_NEWLINE = "\\ No newline at end of file"

Expand Down Expand Up @@ -413,7 +416,7 @@ def _git_lines(text: str | None) -> tuple[list[str], bool]:


def _opcodes(a: Sequence[Any], b: Sequence[Any], work: linediff.WorkBudget | None) -> linediff.Diff:
return linediff.diff(a, b, PATCH_DIFF_BUDGET, work)
return linediff.diff(a, b, PATCH_DIFF_BUDGET, work, anchored=True)


def _range(start: int, stop: int) -> str:
Expand Down Expand Up @@ -947,7 +950,8 @@ def scan(sha: str | None, mode: str | None, path: str) -> prefilter.BlobScan | N
patch, adds, dels, coarse = _patch(base_text, head_text, work)
if coarse is not None:
# The job log says why (repr: a path is one log line).
print(f"collect: {change.path!r}: {coarse}; the patch is one hunk for the changed middle", file=sys.stderr)
shape = "parts past a limit are one hunk" if coarse.startswith(linediff.ANCHORED) else "the patch is one hunk for the changed middle"
print(f"collect: {change.path!r}: {coarse}; {shape}", file=sys.stderr)
try:
entry["patch"] = render.redact_patch(patch, base_text, head_text)
except render.RenderError as exc:
Expand Down
Loading
Loading