diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index 763e672..3ca05d7 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -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 @@ -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 @@ -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. diff --git a/review/src/oc_review/collect.py b/review/src/oc_review/collect.py index 5cbcc8f..25f7c09 100644 --- a/review/src/oc_review/collect.py +++ b/review/src/oc_review/collect.py @@ -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" @@ -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: @@ -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: diff --git a/review/src/oc_review/linediff.py b/review/src/oc_review/linediff.py index 78e7ced..5da1ecf 100644 --- a/review/src/oc_review/linediff.py +++ b/review/src/oc_review/linediff.py @@ -15,6 +15,14 @@ clock). Past a limit the middle is one `replace`: every base line there is removed and every head line added. The result then is not exact, and its `reason` names the limit. + +With `anchored=True` (collect's patches only, K-17), a middle past the size +budget is first split at anchors: 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. Each gap between anchors gets the bounded diff above, +and a gap past a limit is one `replace`. Anchoring takes its steps from the +same work budget. The result is a correct edit script, never git's, so it is +not exact either. """ from __future__ import annotations @@ -38,6 +46,7 @@ PAST_SIZE_BUDGET = "line diff past the size budget" PAST_FILE_WORK_BUDGET = "line diff past the per-file work budget" PAST_RUN_WORK_BUDGET = "line diff past the run work budget" +ANCHORED = "line diff past the size budget, anchored on lines that occur once on each side" class WorkBudget: @@ -97,9 +106,17 @@ def find_longest_match(self, alo: int = 0, ahi: int | None = None, blo: int = 0, return super().find_longest_match(alo, ahi, blo, bhi) -def diff(a: Sequence[Any], b: Sequence[Any], budget: int = DIFF_BUDGET, work: WorkBudget | None = None) -> Diff: +def diff( + a: Sequence[Any], + b: Sequence[Any], + budget: int = DIFF_BUDGET, + work: WorkBudget | None = None, + *, + anchored: bool = False, +) -> Diff: """difflib-style opcodes of `a` to `b`, bounded by the size budget and by - `work` (a fresh `WorkBudget` when None).""" + `work` (a fresh `WorkBudget` when None). `anchored` splits a middle past + the size budget at unique lines first (`_anchored`).""" if work is None: work = WorkBudget() pre = 0 @@ -114,13 +131,15 @@ def diff(a: Sequence[Any], b: Sequence[Any], budget: int = DIFF_BUDGET, work: Wo if pre: codes.append(("equal", 0, pre, 0, pre)) if am or bm: - middle: list[Opcode] + middle: list[Opcode] | None = None if not am: middle = [("insert", 0, 0, 0, len(bm))] elif not bm: middle = [("delete", 0, len(am), 0, 0)] elif len(am) * len(bm) > budget: reason = PAST_SIZE_BUDGET + if anchored: + middle, reason = _anchored(am, bm, budget, work) else: allowance = max(0, min(work.per_file, work.left)) try: @@ -132,7 +151,7 @@ def diff(a: Sequence[Any], b: Sequence[Any], budget: int = DIFF_BUDGET, work: Wo reason = PAST_FILE_WORK_BUDGET if allowance == work.per_file else PAST_RUN_WORK_BUDGET else: work.left -= allowance - matcher.left - if reason is not None: + if middle is None: middle = [("replace", 0, len(am), 0, len(bm))] codes.extend((tag, i1 + pre, i2 + pre, j1 + pre, j2 + pre) for tag, i1, i2, j1, j2 in middle) if suf: @@ -140,6 +159,95 @@ def diff(a: Sequence[Any], b: Sequence[Any], budget: int = DIFF_BUDGET, work: Wo return Diff(codes, reason) +def _unique_pairs(a: Sequence[Any], b: Sequence[Any]) -> list[tuple[int, int]]: + """(i, j) of each line that occurs once in `a` and once in `b`, by i.""" + seen_a: dict[Any, int] = {} + for i, line in enumerate(a): + seen_a[line] = -1 if line in seen_a else i + seen_b: dict[Any, int] = {} + for j, line in enumerate(b): + seen_b[line] = -1 if line in seen_b else j + return [ + (i, j) for i, line in enumerate(a) if seen_a[line] == i and (j := seen_b.get(line, -1)) >= 0 + ] + + +def _longest_ordered(pairs: list[tuple[int, int]]) -> list[tuple[int, int]]: + """The longest subsequence of `pairs` (sorted by i) whose j also rises.""" + tails: list[int] = [] # smallest last j of a run of each length + tail_at: list[int] = [] # index in `pairs` of that last element + prev = [-1] * len(pairs) + for k, (_, j) in enumerate(pairs): + n = bisect_left(tails, j) + if n == len(tails): + tails.append(j) + tail_at.append(k) + else: + tails[n] = j + tail_at[n] = k + prev[k] = tail_at[n - 1] if n else -1 + out: list[tuple[int, int]] = [] + k = tail_at[-1] if tail_at else -1 + while k >= 0: + out.append(pairs[k]) + k = prev[k] + out.reverse() + return out + + +def _anchored( + a: Sequence[Any], b: Sequence[Any], budget: int, work: WorkBudget +) -> tuple[list[Opcode] | None, str]: + """Opcodes of a middle past the size budget, split at anchors, and the + reason. None (one `replace`) when the file's allowance cannot pay for + the anchoring, which is then not run and costs nothing. + + Anchoring costs one step per line of both sides plus one per line of the + shorter side (the pair list and its longest ordered run). Every gap + takes its steps from what is left of the file's allowance, so the file + and the run limits hold. A file past a limit in a gap is charged its + whole allowance, and the reason names that limit. + """ + allowance = max(0, min(work.per_file, work.left)) + cost = len(a) + len(b) + min(len(a), len(b)) + if cost > allowance: + return None, PAST_SIZE_BUDGET + gaps = WorkBudget(total=allowance - cost, per_file=allowance - cost) + anchors = _longest_ordered(_unique_pairs(a, b)) + codes: list[Opcode] = [] + past: set[str] = set() + i0 = j0 = 0 + for i, j in [*anchors, (len(a), len(b))]: + part = diff(a[i0:i], b[j0:j], budget, gaps) + if part.reason is not None: + past.add(part.reason) + codes.extend((tag, i1 + i0, i2 + i0, j1 + j0, j2 + j0) for tag, i1, i2, j1, j2 in part.codes) + if i < len(a): + codes.append(("equal", i, i + 1, j, j + 1)) + i0, j0 = i + 1, j + 1 + work.left -= allowance - gaps.left + # A gap's own work reason is relative to `gaps`; name the file's limit. + worst = None + if past - {PAST_SIZE_BUDGET}: + worst = PAST_FILE_WORK_BUDGET if allowance == work.per_file else PAST_RUN_WORK_BUDGET + elif past: + worst = PAST_SIZE_BUDGET + reason = ANCHORED if worst is None else f"{ANCHORED}; part {worst.removeprefix('line diff ')}" + return _merge_equal(codes), reason + + +def _merge_equal(codes: list[Opcode]) -> list[Opcode]: + """Join adjacent `equal` opcodes, so `hunks` sees each equal run whole.""" + out: list[Opcode] = [] + for code in codes: + if out and code[0] == "equal" and out[-1][0] == "equal": + tag, i1, _, j1, _ = out[-1] + out[-1] = (tag, i1, code[2], j1, code[4]) + else: + out.append(code) + return out + + def hunks(codes: list[Opcode], n: int) -> Iterator[list[Opcode]]: """Group opcodes into hunks with `n` lines of context (difflib's rule).""" if not any(tag != "equal" for tag, *_ in codes): diff --git a/review/tests/unit/test_collect.py b/review/tests/unit/test_collect.py index fcad189..3ed3f29 100644 --- a/review/tests/unit/test_collect.py +++ b/review/tests/unit/test_collect.py @@ -3,6 +3,7 @@ from __future__ import annotations import base64 +import hashlib import json import random import re @@ -434,6 +435,88 @@ def test_size_gate_boundary_on_the_final_document(): assert over.envelope["payload"]["size"]["estimated_tokens"] == tokens +# ---------------------------------------------------------------- lockfile churn and the size gate (K-17) + + +def _pnpm_lock(n: int, bumped: frozenset[int] = frozenset()) -> str: + """A pnpm 9 lockfile of `n` packages, 9 lines each; `bumped` get a new version.""" + + def v(k: int) -> str: + return f"2.0.{k}" if k in bumped else f"1.0.{k}" + + def integrity(k: int) -> str: + return base64.b64encode(hashlib.sha512(f"pkg-{k}@{v(k)}".encode()).digest()).decode() + + lines = ["lockfileVersion: '9.0'", "", "settings:", " autoInstallPeers: true", "", "importers:", "", " .:", " dependencies:"] + for k in range(n): + lines += [f" pkg-{k}:", f" specifier: ^{v(k)}", f" version: {v(k)}"] + lines += ["", "packages:", ""] + for k in range(n): + lines += [f" pkg-{k}@{v(k)}:", f" resolution: {{integrity: sha512-{integrity(k)}}}", " engines: {node: '>=18'}", ""] + lines += ["snapshots:", ""] + for k in range(n): + lines += [f" pkg-{k}@{v(k)}: {{}}", ""] + return "".join(f"{line}\n" for line in lines) + + +def test_lockfile_churn_past_the_diff_size_budget_stays_under_the_size_gate(capsys): + # PR 2027 shape: version bumps near both ends of a 16,000-line lockfile. + n = 1_800 + bumped = frozenset({0, n - 1, *range(7, n, 9)}) + base, head = _pnpm_lock(n), _pnpm_lock(n, bumped) + assert base.count("\n") * head.count("\n") > linediff.DIFF_BUDGET + # One hunk for the whole middle holds every line of both sides: past the gate by itself. + assert (len(base) + len(head)) // c.CHARS_PER_TOKEN > c.SIZE_GATE_TOKENS + col, _, _ = run(_snapshot({"pnpm-lock.yaml": ("100644", base)}, {"pnpm-lock.yaml": ("100644", head)})) + doc = col.envelope + assert doc["status"] == "ok" + assert doc["payload"]["size"]["estimated_tokens"] < c.SIZE_GATE_TOKENS + (f,) = doc["payload"]["files"] + # Each bump changes 5 lines (specifier, version, package key, integrity, snapshot key). + assert (f["additions"], f["deletions"]) == (5 * len(bumped), 5 * len(bumped)) + assert apply_patch(base, f["patch"]) == head + assert linediff.ANCHORED in capsys.readouterr().err + + +@pytest.mark.parametrize("path", ["src/app.js", "pnpm-lock.yaml"]) +def test_large_rewrite_is_still_too_large(path): + # Every line changes, so no line anchors the diff: one hunk, over the gate, + # whatever the file is called (a lockfile name with code in it too). + n = 16_000 + base = "".join(f"export const value{i} = compute({i});\n" for i in range(n)) + head = "".join(f"export const value{i} = compute({i}) + 1;\n" for i in range(n)) + col, _, _ = run(_snapshot({path: ("100644", base)}, {path: ("100644", head)})) + doc = col.envelope + assert doc["status"] == "skipped-size" + assert doc["payload"]["size"]["reason"].startswith(c.SIZE_GATE_REASON) + assert doc["payload"]["size"]["estimated_tokens"] > c.SIZE_GATE_TOKENS + + +INJECTED = " evilScript: node -e fetchAndRunPayload" + + +def test_lockfile_name_or_path_gets_no_other_treatment(): + # The anchored diff never reads the path: code placed in a file named like + # a lockfile, at the root or in an unexpected directory, gets the same + # patch as any other file, and every changed line renders for Claude. + n = 700 + base = _pnpm_lock(n) + head_lines = _pnpm_lock(n, frozenset({0, n // 2, n - 1})).split("\n") + for at in (40, 3_000, 6_000): + head_lines.insert(at, INJECTED) + head = "\n".join(head_lines) + assert base.count("\n") * head.count("\n") > linediff.DIFF_BUDGET + paths = ["pnpm-lock.yaml", "packages/app/node_modules/pnpm-lock.yaml", "src/pnpm-lock.yaml.js"] + col, _, _ = run(_snapshot({p: ("100644", base) for p in paths}, {p: ("100644", head) for p in paths})) + assert col.envelope["status"] == "ok" + files = files_by_path(col) + assert len({files[p]["patch"] for p in paths}) == 1 + patch = files[paths[0]]["patch"] + assert apply_patch(base, patch) == head + assert patch.count(f"+{INJECTED}\n") == 3 + assert render.render_bundle(col.envelope).text.count(INJECTED) == 3 * len(paths) + + # ---------------------------------------------------------------- envelope file @@ -696,7 +779,13 @@ def test_unified_diff_budget_fallback_is_still_correct(monkeypatch): head = base.replace("5\n", "five\n").replace("25\n", "twenty-five\n") patch, adds, dels = c.unified_diff(base, head) assert apply_patch(base, patch) == head - assert (adds, dels) == (21, 21) + # Past the size budget the middle is split at unique lines (K-17). + assert (adds, dels) == (3, 3) # lines 5, 15 and 25 + # With no line that occurs once on each side, it is one hunk. + base, head = "s\n" + "x\n" * 28 + "e\n", "t\n" + "x\n" * 28 + "f\n" + patch, adds, dels = c.unified_diff(base, head) + assert apply_patch(base, patch) == head + assert (adds, dels) == (30, 30) # Low entropy ("a"/"b" lines): about 2.5 million counted diff steps. diff --git a/review/tests/unit/test_linediff.py b/review/tests/unit/test_linediff.py index 932e9e2..e297b03 100644 --- a/review/tests/unit/test_linediff.py +++ b/review/tests/unit/test_linediff.py @@ -169,3 +169,95 @@ def test_per_file_and_run_reasons(): assert linediff.diff(a, b, work=linediff.WorkBudget(total=1_000, per_file=10**9)).reason == ( linediff.PAST_RUN_WORK_BUDGET ) + + +# ---------------------------------------------------------------- anchored (collect, K-17) + + +def _check_script(a, b, codes) -> int: + """`codes` is a correct edit script of `a` to `b`; returns the changed line count.""" + i = j = changed = 0 + for tag, i1, i2, j1, j2 in codes: + assert (i1, j1) == (i, j) + if tag == "equal": + assert a[i1:i2] == b[j1:j2] + else: + changed += (i2 - i1) + (j2 - j1) + i, j = i2, j2 + assert (i, j) == (len(a), len(b)) + return changed + + +def _lockfile(n: int, bumped: set[int]) -> list[str]: + """pnpm-like lines: unique name and integrity lines, repeated filler lines.""" + out = [] + for k in range(n): + v = f"1.0.{k + (k in bumped)}" + out += [f" pkg-{k}@{v}:", f" resolution: {{integrity: sha512-{k}-{v}}}", " engines: {node: '>=18'}", ""] + return out + + +def test_anchored_lockfile_churn_is_a_small_diff_within_the_file_budget(): + n = 4_000 # 16,000 lines per side, changed near both ends + bumped = {0, n - 1, *range(1, n, 97)} + a, b = _lockfile(n, set()), _lockfile(n, bumped) + assert len(a) * len(b) > linediff.DIFF_BUDGET + work = linediff.WorkBudget() + d = linediff.diff(a, b, work=work, anchored=True) + assert d.reason == linediff.ANCHORED and not d.exact + assert _check_script(a, b, d.codes) == 4 * len(bumped) # name and integrity line, each side + assert 0 < linediff.RUN_WORK_BUDGET - work.left < linediff.FILE_WORK_BUDGET + # Without `anchored` (the publisher) the middle is one replace, as before. + plain = linediff.diff(a, b, work=linediff.WorkBudget()) + assert plain.reason == linediff.PAST_SIZE_BUDGET + assert _check_script(a, b, plain.codes) > len(a) + len(b) - 10 + + +def test_anchored_diff_is_a_correct_edit_script(): + rng = random.Random(17) + for _ in range(300): + pool = [f"u{k}" for k in range(40)] + ["x", "y", ""] * 10 + a = [rng.choice(pool) for _ in range(rng.randint(0, 50))] + b = [rng.choice(pool) for _ in range(rng.randint(0, 50))] + work = linediff.WorkBudget() + d = linediff.diff(a, b, budget=rng.choice([0, 1, 20, 400]), work=work, anchored=True) + _check_script(a, b, d.codes) + assert all(x[0] != "equal" or y[0] != "equal" for x, y in zip(d.codes, d.codes[1:])) + assert work.left <= linediff.RUN_WORK_BUDGET + + +def test_anchored_with_no_unique_line_is_one_replace(): + a, b = ["s", *_alt(10)], ["t", *_alt(10, "b", "a")] + d = linediff.diff(a, b, budget=1, anchored=True) + assert d.codes == _coarse(a, b) + assert d.reason == linediff.ANCHORED + "; part past the size budget" + + +def test_anchoring_is_charged_and_skipped_when_the_file_cannot_pay(): + a, b = ["a", "x", "b"], ["c", "x", "d"] + cost = len(a) + len(b) + 3 + work = linediff.WorkBudget(total=cost - 1) + d = linediff.diff(a, b, budget=0, work=work, anchored=True) + assert (d.reason, d.codes, work.left) == (linediff.PAST_SIZE_BUDGET, _coarse(a, b), cost - 1) + work = linediff.WorkBudget(total=cost) + d = linediff.diff(a, b, budget=0, work=work, anchored=True) + assert d.codes == [("replace", 0, 1, 0, 1), ("equal", 1, 2, 1, 2), ("replace", 2, 3, 2, 3)] + assert d.reason == linediff.ANCHORED + "; part past the size budget" + assert work.left == 0 + + +def test_anchored_gap_past_the_work_budget_charges_the_whole_allowance(): + low_a, low_b = LOW_ENTROPY["a-only-vs-alternating"] + a, b = ["head", *low_a, "tail"], ["head2", "head", *low_b, "tail"] + # Only the gap after the anchor "head" fits the size budget. + budget = len(low_a) * len(low_b) + work = linediff.WorkBudget() + d = linediff.diff(a, b, budget=budget, work=work, anchored=True) + assert d.reason == linediff.ANCHORED + "; part past the per-file work budget" + assert work.left == linediff.RUN_WORK_BUDGET - linediff.FILE_WORK_BUDGET + _check_script(a, b, d.codes) + run = linediff.WorkBudget(total=1_000_000) + assert linediff.diff(a, b, budget=budget, work=run, anchored=True).reason == ( + linediff.ANCHORED + "; part past the run work budget" + ) + assert run.left == 0