Skip to content

v0.13.0

Latest

Choose a tag to compare

@guimatheus92 guimatheus92 released this 07 Sep 21:46
20739f4

Added

  • INV-FETCH-04 — no content is fetched for a file that will not be reviewed. The 500-file guard ran after gather, so everything fetched for a too-large PR was paid for and thrown away. Measured live on a 501-file Azure DevOps PR of additions: 501 getItem calls and 5.1 s, then "PR is too large: 501 changed files"; the same PR of modified files costs two calls each, which the hermetic test pins at 1002. Both are now zero, and the live gather takes 0.65 s. The same shape hit exclusions at any size — applyDiffExclusions sets patch: undefined, so ADO was fetching package-lock.json and every .png twice and discarding both patches on the next line. One question, patchPolicy, asked in one place (beside applyDiffExclusions, where the constants moved because gather.ts cannot import review.ts), by the two sites that pay: the ADO provider before spending two whole-file getItem calls, and gather's truncated-list completion before spawning one git diff-tree -p per missing file — a path that only runs on the largest PRs there are. fetchChangedFiles takes an optional second argument; GitHub and GitLab ignore it because their listing response already carries the patch, and with no argument every file is fetched, so a provider called directly is unchanged. runGather itself does apply the guard to every caller, which is deliberate for pr-review gather (a >500-file PR yields the complete path list without content) and wrong for pr-review post, which gathers only to build the valid-line map — it passes patchesRequired and still gets every patch. The path list is never narrowed — only content — because paths are what the rule-trust, config-trust and MCP gates read (INV-FETCH-01), and patch-less rows were already normal for binaries, pure renames, mode-only changes and ADO deletions. Fixes #27.
  • The checkout's own diff_excludes now reach gather (as repoExcludes) so the fetch decision is taken over the same in-scope set earlyExitGate finally counts: a 600-file PR that the repo's .pr-review.yaml trims to 480 stays reviewable, exactly as before. Those globs are branch-authored, so they are used asymmetrically — they may shrink the COUNT, never suppress an individual file. Counting with them is safe in one direction only (extra excludes make a run less likely to be refused, i.e. more likely to fetch), while letting them suppress would mean diff_excludes: ['**/*'] in a PR delivers a review of its own code with no diff at all.
  • The too-many-files guard has tests. MAX_FILES_GUARD, MAX_PATCH_BYTES and the "everything excluded" clause were the only product contract in the repo with neither an invariant ID nor a single assertion — tests/zero-passes.test.ts covered the description clause of the same function and stopped. Both sides of the boundary are now pinned: 501 in-scope files refused, 501 rows with one lockfile allowed (the gate is >, and exclusions are what it counts over), two 1.2 MB patches refused on the sum, the same bytes under vendor/ not counted at all.

Changed

  • Azure DevOps patches are @@ hunks instead of whole files. ADO has no diff endpoint, so synthesizePatch builds the patch from two full file bodies — and emitted the whole file too, every unchanged line as context with no hunk header anywhere. Nothing broke loudly (validLinesFromPatch starts its cursor at 0, so a hunkless patch happens to number correctly), which is why it survived from the first release; the cost was that the gather cache, the ## Diff block of pr-context.md that every pass reads, and the 2 MB patch budget all scaled with the size of the files rather than the size of the change. toHunks frames it with git's own 3 lines of context, and all four shapes route through it — the LCS diff, the coarse MAX_LCS_CELLS fallback, and the pure add/delete short-circuits (now @@ -0,0 +1,N @@ and @@ -1,N +0,0 @@) — so they cannot drift apart. Identical content yields no patch at all rather than a preamble with nothing under it: ADO lists encoding- and mode-only changes as changed files, and a truthy contentless string is what INV-FETCH-02 counts as content.
  • The posting shape is decided once per provider, not three times. snapFindingsToDiff runs for all three providers, and on ADO every line of every changed file used to be "in the diff" — so a finding on line 900 posted on line 900, which is what AGENTS.md and README.md have always promised. That was true only by accident: with real hunks, snapping would have dragged every such finding to the nearest changed line, silently, on every ADO review. postingPolicy states the rule (ADO neither snaps nor re-anchors; GitHub and GitLab do both) and postingShape applies it. The rule previously sat in three files — runPost applies the shape while resumeReview and verify each recompute it to recognize the run's own comments — and three copies that must agree are a double-post (INV-POST-05) and a false audit (INV-POST-06) waiting to happen.
  • A gather that withheld content is never cached, on the same principle as changedFilesComplete: restored later under a wider exclusion set, a path-only list looks like a whole diff. Nothing is lost — the run such an entry would serve is refused for the same reason the first one was.
  • loadConfig is called once with the repo config rather than twice; the value is needed before gather for the fetch decision and again after it. Which config is trusted is still decided after gather, on the complete file list.

Security

  • A glob from the branch under review can no longer hang the reviewer's CLI. diff_excludes may come from the checkout's own .pr-review.yaml, and since the change above those globs reach the matcher before the config is rejected as untrusted — a reachability the pipeline did not have, because runReview swapped in the trusted configuration before any exclusion ran. globToRegex compiles ** to .* and anchors the result, so **a**a**a**a**a**a**a**a**b is catastrophic backtracking: measured at 3.7 s against a single 40-character path, and matchesAny compiles inside its own .some(), so a 501-file PR would have paid it once per file. Patterns are now compiled once per process and refused past 512 characters or four ** segments, with one stderr line naming the pattern. Refusing means the glob matches nothing, which for an exclusion list is the safe direction — the file is reviewed, never silently hidden. The memoization is a plain win everywhere else too: 500 paths against the 31 built-in exclusions went from recompiling 15,500 regexes to 2 ms.

Fixed

  • A cache entry that withheld content is no longer served to a run that excludes less. Below the file guard, content is still skipped for excluded paths — and the cache stores raw rows while the hit path re-applies the current run's exclusions, keyed only on headSha + last comment id. So pr-review review with diff_excludes: ['**/generated/**'] followed by pr-review gather (which passes no exclusions) would have pulled those rows back into scope carrying no patch, and the passes would have reviewed them blind with nothing to signal it. Entries now record the globs they were assembled under in contentExcludes and are refetched unless every one is still excluded; entries where nothing was withheld carry no such field and are served as before, so GitHub and GitLab pay nothing for a contract that only binds Azure DevOps.
  • pr-review verify no longer FAILs two runs that behaved exactly as designed. INV-FETCH-02 failed whenever no in-scope file carried a patch, which the change above made reachable twice over: a run refused as too large (its pr-review-gather.json is written before the gate, so the artifact is auditable and deliberately empty of content) now SKIPs with that reason named, and a PR of pure renames, mode changes or binaries — where nothing adds or removes a line anywhere — PASSes. A file that does report changed lines and still carries no patch is still a FAIL, which is the defect the clause exists to catch.
  • The nofetch acceptance cell pins --runtime copilot on its review invocation. resolveRuntime runs before earlyExitGate and only returns early for an explicit choice, so on a CI runner with neither agent CLI installed the cell would have died on "No agent runtime found" and reported a guard failure that never happened. Its artifacts are now uploaded too — the one file naming which rows carried a patch is exactly what a failing run needs.
  • The oversized-diff message reported limit 1.9073486328125 MB next to a rounded 2.3 MB: it divided 2,000,000 by 1024² and never rounded. Both figures now share the divisor and the rounding; the limit itself is unchanged. Found by writing the guard's first test.
  • Azure DevOps folder entries are dropped before the in-scope count is taken, so the ancestors a directory add drags in cannot push a PR over the file guard.
  • evals/acceptance/README.md and scripts/acceptance.mjs pointed three times at tests/gather.test.ts, which does not exist — the file is tests/gather-cache.test.ts. INV-FETCH-01's Verified: list omitted tests/providers/github.test.ts, the count-carrying test the whole gate is built on.