Skip to content

Releases: guimatheus92/pr-review

v0.13.0

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.

v0.12.0

Choose a tag to compare

@guimatheus92 guimatheus92 released this 07 Sep 18:06
9cd914f

Added

  • INVARIANTS.md — the guarantees, stated once. 26 invariants with stable, append-only IDs (INV-POST-01, INV-FETCH-01, …), each carrying what always holds, why (the incident or the reasoning), where the code enforces it, and what verifies it. They were previously spread across AGENTS.md, README.md, architecture.md, code comments and test names, with no addressable list — and "never post a summary comment on the PR" was never stated anywhere, only implied by inline-only. That is exactly how the code-review companion's top-level "### Code review" verdict reached a live PR. AGENTS.md now cites the IDs instead of restating the guarantees, and the README names the no-summary and complete-file-list guarantees explicitly.
  • pr-review verify [run-id] [--pr <url>] — audit a finished run against the charter. Read-only: it never posts, deletes or rewrites anything. One PASS/FAIL/SKIP row per invariant, always the full list (registry order, then the test-only rows); a check that throws renders FAIL rather than vanishing, and a SKIP always carries its reason, so "not checked" can never read as "checked and fine". Exit 0 when every row passed or skipped, 1 when the audit itself could not be completed (a live read-back failure that was not asked for with --offline), 2 on any FAIL. --offline grades from the run artifacts alone; --json for CI. Posting identity is derived from confirmed writes — the comments matching a key the run planned to post are the run's, and their modal author is us — so no provider grows a whoami(); with no confirmed write the posting rows skip and fall back to a shape tripwire that can only fail, never pass by accident. tests/invariants-doc.test.ts asserts the document and the check registry hold the same ID set in both directions, so a renamed ID fails the suite instead of silently dropping a row.
  • capabilities.json records the resolved runtime. Under --runtime auto nothing on disk could say which CLI hosted the session; the runtime only ever reached stderr. scripts/eval.mjs now prints it per case.
  • An acceptance matrix against real pull requests: 3 providers × 2 runtimes, posting for real (npm run acceptance, evals/acceptance/). Until now Azure DevOps and GitLab were exercised only through stubs and the Copilot runtime only at the argv layer — the two riskiest surfaces in the product had no live proof. Each cell resets its fixture PR (before the run, never after — an after-reset is the step a cancelled run skips), reviews from inside a clone of the fixture repo so cwdIsPrRepo gates actually engage, asserts the findings and routing against a shared expected.yaml, reads the PR back to confirm the content landed, and runs pr-review verify. A GitLab-only 101-file MR covers the truncated-file-list gate from both sides — completed from git in the right checkout, refused from anywhere else. npm run acceptance:seed creates the whole estate idempotently. .github/workflows/acceptance.yml runs the copilot cells on manual dispatch with environment-scoped secrets and no pull_request trigger, so a fork can never reach them.
  • A pass that claims an MCP call is flagged, not believed. capability-<pass>.json is written by the dispatched pass itself, and readCapabilityUsage only checked that the three arrays were arrays — so a sidecar reporting a successful MCP call was archived as fact under a runtime that denies MCP at the process level, with no warning, no degraded entry and no stderr line. A non-empty available / attempted / used now raises one named degraded warning per pass, listing which fields came back non-empty and with which servers, echoed to stderr as [mcp] in plain text (the summary copy is entity-escaped by safeSummaryValue; a terminal renders no entities, and the server names are exactly what an operator greps) and persisted in capabilities.json alongside the untouched raw claim. The rendered list is bounded — 10 servers per field with (+N more), each name truncated — because the sidecar is untrusted model output and the escaping amplifies it; the archived arrays stay complete. It stays a warning: Node has no view of the session's tool surface, so a real denial leak and a fabrication are indistinguishable here, and the run completed its job either way — the named servers are what classifies it, cross-checked against the mcpServers inventory in the same file and dispatch-plan.json's runtime / disabledMcpServers. available is included deliberately: the brief asks a pass that finds a callable mcp__* tool to name it there, so exempting it would blind the check to the one shape a real leak arrives in. Fixes #30.
  • A re-dispatched pass no longer inherits the previous attempt's capability sidecar. spawnPlannedBatch already cleared each reviewer's stale attempt file; it now clears capabilityPath too, so a pass recovered in attempt 2 that writes no sidecar is reported as missing evidence rather than audited on attempt 1's. This covers the initial, automatic-recovery and manual-recovery batches at once — the previous cleanup lived in the legacy orchestrator path, unreachable since planned dispatch became unconditional. The delete gets its own try, and a failure is reported on stderr rather than swallowed: a stale attempt file is detectable downstream, but a surviving sidecar is byte-identical to fresh evidence, so a silent failure would reinstate exactly the bug the audit above exists to catch.

Changed

  • The acceptance matrix distinguishes BLOCKED from FAIL. A cell that cannot run — today, the Copilot runtime with its premium requests exhausted — reports 🚧 blocked with the reason and is counted apart from the passes, never as a product failure. With zero premium requests the CLI refuses every capable model, auto falls back to one that cannot carry a nine-pass orchestration, and INV-DEL-01 then correctly refuses to post a partial review; reporting that as FAIL sends someone hunting a bug that does not exist. The probe is a courtesy — if it cannot answer, the cell runs and the real assertions speak.

Fixed

  • A resumed run no longer reports success with incomplete delivery. Neither --resume path passed operationalFailures to finalizeReview, so a run resumed after losing a companion agent could write companions.json with a missing reviewer and still exit 0 — the "a parseable review is not a completed review" rule held on fresh runs only. Both paths now re-read the interrupted run's companion accounting and exit 2 for a shortfall, matching the fresh path.
  • pr-review doctor checks GitLab. It probed GitHub and Azure DevOps only, so a GitLab-only user got a green preflight and a credential error on their first review. It now also reports AZURE_DEVOPS_BEARER (previously an ADO user with only a bearer token read as failing unless az happened to be installed) and GitHub Enterprise Server tokens, which never fall back to the cloud variables.
  • resolveRuntime takes an injectable probe, and the documented auto order — copilot first, then claude, then throw — is now covered. It previously had none: the only way to exercise it was to change the machine's PATH.
  • ChangedFile.status means the same thing on every provider. GitHub's pulls/:n/files ships seven statuses and a cast pushed them into a four-value union unchanged, so every GitHub deletion carried removed — a value the type does not contain — while Azure DevOps, GitLab and the git completion emitted only the documented four. GitHub now maps explicitly (removeddeleted, copiedadded as git's own C does, changed/unchangedmodified); an eighth value GitHub might add degrades to modified with one stderr line per distinct value rather than diverging in silence. unchanged is mapped rather than dropped on purpose: a dropped row would shorten changedFiles and the strict length comparison against changed_files would send the PR down the complete-from-git path, or refuse it. Azure DevOps now labels a rename renamed and carries previousPath — it reported both as a plain modify with no previous path, so the .pr-review.yaml trust gate (which checks the previous path too) could not see a config file renamed away on ADO alone. The rename bit only replaces the modified fallback, so delete/add precedence and every base-content fetch stay byte-identical; previousPath is keyed on the source path rather than on the label, so ADD|RENAME and DELETE|RENAME — still renames, still labelled added/deleted — carry it too. The cast is gone, so that call site is type-checked again like every other; tests/changed-file-status.test.ts inventories all four producers' vocabularies, and each provider's fetchChangedFiles now covers the call site itself. Gather cache entries written before this release keep the status they were cached with — the field has one consumer, which renders it as prose, so they are not invalidated. Fixes #29.
  • A network failure is now transient on all three providers. fetch reports every transport failure as the message fetch failed, with the real code buried in cause and no status property — and status was the only thing isTransientGitHubError, isTransientAdoError and isTransientGitLabError read. So the most transient error there is was classified permanent: a connection reset during posting made retriable come back empty, and the reconcile-then-retry loop in runPost that exists for exactly this case never ran. Observed live — a GitLab review posted 56 of 57 comments and exited 2 over one fetch failed. A shared isNetworkError in src/util/retry.ts walks the cause chain (bounded, since a chain can be circular) and is OR-ed into all three. This does not introduce a blind retry: it only makes the error eligible for the path that re-issues a write after readLanded confirms the comment is genuinely absent, so INV-POST-04 is unchan...
Read more

v0.11.0

Choose a tag to compare

@guimatheus92 guimatheus92 released this 05 Sep 17:49
9fcdea0

Added

  • A truncated file list is never reviewed. PrMetadata.changedFileCount / changedFileListTruncated carry the provider's own count (GitHub changed_files; GitLab changes_count, where "N+" means the stored diff overflowed and /diffs serves exactly the capped set). runGather refuses a provider list of any other length, or one declared truncated, and completes it from the reviewer's checkout when that checkout is the PR's repository with base and head already present: git diff-tree -r -M -z from the single merge base (plumbing, so reviewer diff config and the PR's own .gitattributes cannot reshape the list; -z, so non-ASCII paths arrive raw), one hunks-only patch per missing file, provider entries winning. It never fetches: when a commit is absent, the history has two merge bases, or the clone is shallow, the run fails BEFORE anything is cached — exit 2, error.txt in detached mode — naming the counts and the exact git fetch to run (git fetch origin <base> refs/pull/N/head, refs/merge-requests/N/head, or the ADO branches). A completion that still falls short of an exact count is refused as well, and a list LONGER than the count is reported as a disagreement rather than a truncation. The fetch command in the message quotes every ref. An incomplete file list is unknown, never empty: the rule-trust, config-trust and MCP gates it feeds would otherwise pass a rule the PR changed. Fixes #23.
  • The plugin slash command (commands/pr-review.md) locates the checkout whose git origin matches the PR URL before starting the CLI — the current directory, then its subdirectories, then its siblings, preferring a primary worktree over a linked one — and prints a project-skill count computed with the same rule as the loader.
  • --force-skill <file|dir> is documented as the only bypass: the file, or every .md under the directory, is injected whole into every pass with no scope, relevance or rule-trust check (the directory form has always been accepted). It is per run and CLI-only by design — there is deliberately no yaml or env key for forcing, so a committed .pr-review.yaml can never pre-authorize branch-authored content. Docs that told you to point extra_skills_dirs / --skills-dir / PR_REVIEW_SKILLS_DIR at a directory to force it now say --force-skill <dir>; those keys are trust-checked, and the warning not to aim a forced directory at rules the PR can edit applies to --force-skill alone.
  • Brand icon (assets/icon.svg) and a branded README: centered header, badges, highlights, a mermaid pipeline diagram, a table of contents, and the verbose reference material folded into collapsible sections. Docs only — nothing ships differently.

Changed

  • GitHub no longer fetches the whole-PR text diff: pulls.get with the diff media type returns 406 above 300 files (undocumented), and nothing reads fullDiff — the per-file patches are the diff. GitHub PRs of 300–500 files are now reviewable; fullDiff stays in pr-review-gather.json, empty for GitHub.
  • Gather cache entries carry changedFilesComplete, set only once the file list passed the completeness gate. An entry without it was written before this release and is refetched once, then rewritten in place; no cache clear needed.
  • BREAKING: --skills-dir / extra_skills_dirs / PR_REVIEW_SKILLS_DIR are now selected like repo skill dirs, not injected whole. In 0.10 every file under a configured dir was force-fed into every pass regardless of scope, relevance or rule trust — observed live: 1.4 MB × 8 passes on a 5-file PR. Now a targeted file (applies_to / applyTo / paths) becomes a scoped rule, an untargeted one goes through the name+description relevance heuristic, an unmatched one lands in the on-demand index, and a file the PR changed inside a configured dir is skipped (also when the dir comes from an unchanged .pr-review.yaml). Configured dirs still apply when the cwd is not the PR's repository, their origin is configured, and passes.json / matchedBy are unchanged (glob / repo / index). The stderr line names the bypass: --force-skill <dir> injects a directory whole.
  • Linked skill directories are followed, and trust is by authorship rather than location. Discovery follows a directory link (symlink or NTFS junction) one hop, in every discovery dir (.claude/skills, .claude/rules, .copilot/skills, .github/skills, .github/instructions, .agents/skills) and in configured dirs — 0.9/0.10 rejected every link and failed closed on anything resolving outside the checkout. A link the PR added or changed (its path, or any parent directory of it, is in the diff) is refused before anything behind it is read and named as degraded coverage; a link met inside a linked directory is not followed. Content whose real path is outside the checkout is trusted only when the PR did not author the link reaching it AND the file is committed and clean in its home git repository (git ls-files + git status; a SKILL.md needs its whole directory clean) — the same gate applies to every rule outside the checkout (linked, configured or personal), and a repository git cannot read is skipped, never trusted: on Windows git checkout of a PR branch writes through a junction into the shared directory, so a planted file would otherwise become a trusted rule for every sibling repo's review. Untracked or modified files there are skipped and named; a directory under no git repository at all is trusted as the reviewer's local configuration, with one stderr note per directory reached through a link. Nothing depends on one company's layout — any link, any discovery root, any OS, git or not. The aggregate stderr line now reads [skills] skipped N project rule(s) — changed by this PR, reached through a link it changed, or not committed in their home repository. Fixes #20.
  • Trust comparisons fold letter case and Unicode (NFC) on every platform. They folded case on win32 only, so a PR committing .Agents/skills could bypass the rule-trust check on a macOS reviewer's machine.
  • The posting guarantee now states exactly what each provider does: GitLab findings post as inline discussions; re-anchoring of unanchorable findings applies on GitHub and GitLab, while Azure DevOps threads post at the reported file:line as-is and a location-less finding lands as a resolvable PR-level thread. README and AGENTS.md said "re-anchored" universally; the code (reanchor in src/commands/post.ts) never did that on ADO.
  • Reference docs agree on the pass ceiling (6 stack + up to 2 installed-plugin + every baseline under the 16-pass materialization ceiling; 10 only when no pack passes exist), the ## Skills totals line, and GitLab in SECURITY.md's scope.
  • Reference docs now correctly describe post-gather setup, parallel Codex review, conditional verification, post-selection shared context (skills-project.md vs the budgeted skills-all.md fallback), and direct-agent versus slash-command companions. Materialized pass files now label skill source paths as provenance instead of claiming inaccessible sibling references resolve inside the confined runtime.

Fixed

  • A dispatched claude session no longer boots the MCP servers it is forbidden to call. 0.10 denied the mcp__* tools but left --setting-sources user loading every user-level MCP server, so each review started them anyway — on Windows a cmd.exe + conhost.exe + npx + node per server, each console window leaking permanently because the terminal never reclaims it. Measured on one live run: 20 processes and 4 consoles, of which 16 processes and 3 consoles existed only to start three servers the session could not reach. --strict-mcp-config is now passed alongside the tool denial, and the per-runtime switch lives in a typed MCP_PROCESS_DENIAL: Record<Runtime, string> so a runtime added later fails to compile until it declares one. The two are not symmetric and the docs now say so: claude's is categorical, copilot's --disable-builtin-mcps is completed per name by --disable-mcp-server and so is bounded by discoverMcpCapabilities. Because no --mcp-config is passed, the run-dir .mcp.json is now provenance only — no runtime loads it, and the artifact docs say so. This also corrects the 0.10.0 note below, which claimed "built-in/ambient MCP servers are denied" when that held at the tool level only under claude. No capability is lost: the tools were never in a pass's callable surface — run artifacts recorded passes reporting exactly that.
  • The shared PR context no longer advertises ## Available MCP Capabilities, and the installed-plugin capability brief no longer tells a pass to call tools it cannot have. Both runtimes deny MCP, so the advertisement only bought a paragraph of each plugin pass explaining why the call it was told to make was impossible. capabilities.json and the capability-<pass>.json sidecars are unchanged in shape; the brief now states that available, attempted and used are all empty under a runtime that denies MCP, which the previous wording left undefined for available.
  • Azure DevOps PRs with more than 100 changed files were reviewed on their first 100. getPullRequestIterationChanges defaults to $top=100 and the response cursor was never read; every release from 0.6 to 0.10 issued that one unpaged call and cached the result. Iteration changes are now paged at 2000 per call until a short page, a full page without a cursor is probed once more, and a cursor that does not advance throws instead of looping.
  • Azure DevOps folder entries (a directory add, or an ancestor of an edited file) no longer become "changed files" with an empty synthesized patch: they counted against the 500-file guard, cost a content fetch each, and would have multiplied under full pagination.
  • Foreground reviews now pass the provider resolved from pre-gather trusted configuration into gather, and detached preflight uses the same trusted ...
Read more

v0.10.0

Choose a tag to compare

@guimatheus92 guimatheus92 released this 01 Sep 17:24
9440af3

Added

  • Node-owned resilient reviewer delivery: description-bearing runtime calls write attempt-scoped exact Finding[] JSON; Node promotes write-once canonical sidecars, assembles Phase 1/final output, gates a direct verifier, and records reviewer-level progress. One automatic selective recovery retries only unresolved reviewers; --resume gets the bounded final targeted attempt.
  • Schema-v1 runs persist HMAC-authenticated plan, delivery, Codex, and posting authority under ~/.pr-review/control/, with repairable diagnostic mirrors in the run dir. State binds PR/config/bundle/input hashes, attempts, canonical digests, and sticky dry-run/publish mode.

Changed

  • Reviewer runtimes lose the tools that let a dispatched agent act outside the CLI: shell (Bash/PowerShell), web (WebFetch/WebSearch), and built-in/ambient MCP servers are denied, and the session runs from the run directory under user-only setting sources, so checkout-supplied instructions are not auto-loaded. This closes the vector that once let a companion post its own PR comment. Read-only file access is deliberately unchanged — reviewers still open the checkout, which is what keeps findings grounded in the surrounding code. On-demand skill bodies are copied into the run directory and large catalogs split into digest-bound index shards, with original-source provenance retained. Codex remains an OS-sandboxed read-only sibling with strict attempt-scoped output.
  • status reports planned/valid/missing/invalid reviewer and finding counts, distinguishes recoverable exit 21 from terminal exit 22, and trusts authenticated state over runtime-writable summaries/errors. Legacy Phase 1 output is dry-run diagnostic evidence and cannot be published.
  • Dry-run is no longer a one-way door. Previewing with --dry-run and then posting what you saw is the point of a dry run, so --resume now accepts the dry-run → publish transition. It is admitted ONLY on a complete delivery, which keeps the real invariant — partial findings never post — fully intact; an incomplete run is refused as incomplete-promotion, and publish → dry-run stays refused as mode-mismatch because that run may already have posted.

Fixed

  • Runtime exit 0 before consolidation no longer discards completed work or falsely reports no parseable findings. Valid reviewer hashes remain unchanged through recovery; malformed/missing outputs and required verifier/Codex failures stay incomplete, and no partial finding reaches dedupe or posting.
  • Copilot/Claude task calls use JSON-safe arguments and mandatory descriptions. Windows launcher punctuation, reviewer filename aliases, atomic state replacement, canonical create-only promotion, and schema-v1 posting-marker authentication are covered by regressions.
  • Credential-bearing pack URLs are redacted before entering runtime-readable plans. Canonical reviewer output publishes only after complete fsynced bytes exist, control readers consume durable backup bytes without renames, and a crash-recoverable per-run lease serializes recovery and posting through authenticated finalization.
  • status emits an executable sticky-mode recovery command, while context-only and benign no-dispatch runs no longer leave half-created schema-v1 recovery control.
  • A failed review/post no longer aborts the process on Windows. validateRecoveryPreconditions reads the PR back before deciding, and the command's catch ended in process.exit() — a hard exit while undici's keep-alive handle is still closing trips Assertion failed: !(handle->flags & UV_HANDLE_CLOSING), src\win\async.c, replacing a clean exit 2 with exit 127 and a C-level abort message. Both commands now set process.exitCode and let the loop drain. Reproducible in six lines: await fetch(url); process.exit(2).
  • Only a publishing resume requires the PR to still be open. metadataMatchesPlan also asserted live.state === 'open', so any run gathered against a merged or closed PR could never be resumed — not even --dry-run — and the refusal claimed the PR "no longer matches the saved dispatch plan" while every field matched. Refusal is now pr-not-open and only on publish.
  • tests/finalize-failure.test.ts no longer finalizes against the developer's real home: the two posting-failure scenarios ran without homeOverride, so every npm run test left an empty directory behind under ~/.pr-review/control/. Still outstanding: tests/status.test.ts writes authenticated control records to the real home (and so mints ~/.pr-review/control/control.key) because runStatus resolves RUNS_ROOT itself and has no home seam.

v0.9.0

Choose a tag to compare

@guimatheus92 guimatheus92 released this 27 Aug 03:42
1ae0309

Added

  • Local branch dogfood without opening a PR: npm run dogfood -- --base origin/main --include-untracked converts the complete branch/working-tree diff (including opted-in untracked text files) into a temporary GatherOutput under ~/.pr-review/runs/ and drives the real bundled CLI with --from-gather --dry-run. URL-based companions are disabled for this synthetic PR; exercise them on a real-PR dry-run. The generated dist/cli.cjs remains recorded but is excluded from LLM context; npm run build is its validation surface.
  • Synthetic eval fixtures also disable URL-based companions; their pass/stack/finding assertions exercise the bundled pipeline, while companion dispatch is validated on real-PR dry-runs.
  • --force-skill <file> is the explicit escape hatch for bypassing a skill's applyTo/paths scope. --skill <file> now keeps the declared scope.
  • Installed plugins are discovered generically from their manifests in both supported hosts — Copilot CLI and Claude Code (whose extra <version> directory level is resolved through the authoritative installPath in installed_plugins.json, so a stale side-by-side version is never picked). Relevant review-oriented skills may become capped plugin passes from repository/path/topic evidence, while declared user, repository, and plugin MCP servers are recorded in capabilities.json. Plugin passes write audited available/attempted/used MCP evidence; no technology-to-reviewer lookup table is involved.

Changed

  • Pack routing now distinguishes evidence: canonical Linguist language names, manifest ecosystems, full dependency names, and dependency tokens stay separate. Dependency-backed skills outrank generic language matches; product skills such as Azure Functions, MCP, or Copilot SDK no longer qualify from *.cs, *.ts, *.csproj, or package.json alone. Generic manifest globs are weak evidence, and SKILL.md is container format rather than an identity token.
  • Product-specific identity tokens must co-occur in one dependency group; unrelated packages can no longer combine azure and functions into false Azure Functions evidence. Original package casing is retained while tokenizing, so NuGet compounds such as DurableTask remain detectable.
  • Project rules are also discovered from .github/instructions/ (applyTo) and .claude/rules/ (paths). Semantically identical mirrors dedupe silently; divergent same-name rules still warn.
  • A repository rule file added or modified by the PR is excluded from both authoritative context and the on-demand index for that review, then named as degraded coverage. Branch-authored instructions cannot tell reviewers how to judge their own change.
  • Rule trust filtering runs before same-name dedupe and checks lexical plus real paths. A changed later mirror cannot evict its unchanged counterpart; in-repo --skill files remain scoped/untrusted when changed, --force-skill is the explicit override, and linked rules resolving outside the checkout fail closed.
  • Linguist aliases no longer become independent stack technologies. Exact filenames override ambiguous extensions, and manifest evidence resolves shared extensions (for example .cs to C# rather than Smalltalk when a C# project owns the change).

Fixed

  • Windows 8.3 short paths no longer make a checkout look like a different directory. Every containment check compared a path from one source (git rev-parse, a manifest) against one from another (os.tmpdir(), a directory walk), and realpathSync folds symlinks but not 8.3 components — so C:\Users\RUNNER~1\... and C:\Users\runneradmin\... read as different directories and a legitimate --skill, manifest or plugin file was silently refused. All five comparison sites now go through realpathCanonical (realpathSync.native first). Invisible to anyone whose username is 8 characters or fewer, which is why it only surfaced on CI.
  • A repository MCP server is refused when it would launch code from the reviewed checkout. Refusing only a changed .mcp.json left a hole: the config can be untouched while the PR rewrites the script it points at (node ./scripts/mcp-server.js), and reviewing a PR means the checkout already sits at that branch's head. In-repo launch paths (explicitly relative, absolute-inside-repo, or an existing repo file) are now refused per server, with the reason surfaced; external tooling (npx -y @scope/pkg, docker run …, a bare PATH command) is unaffected.
  • Installed-plugin discovery is no longer Copilot-only. Under Claude Code the plugin cache adds a <version> level and can hold several versions at once, so nothing was discovered there at all — pr-review supports both hosts and must behave the same in each.
  • The extra_skills_dirs example no longer points at .claude/skills. A forced directory bypasses the rule-trust check, so recommending the reviewed repo's own rule directory re-admitted exactly the branch-authored input that check exists to reject.
  • The directory trust gate is case-insensitive. normalizedRelative() lowercases paths only on win32, so a case-sensitive skill.md test matched nothing on Linux/macOS and silently disabled the gate there. Caught by dogfooding this branch against itself; skillDirPrefix is now unit-tested in both casings so the regression is detectable on any platform.
  • A SKILL.md is now untrusted when the PR changed any file in its directory, not only SKILL.md itself. Only SKILL.md loads as a skill, but every pass is handed a Source: line saying relative references/ resolve from that directory — so a PR could ship branch-authored instructions beside an unchanged SKILL.md and have them read as authoritative project context. Found on a live dry-run of Preco-Pratico/PrecoPratico-Docs#269, which changes .claude/skills/backend-guide/create-database.md. Flat <dir>/<name>.md rules share a directory with unrelated rules, so for those the file stays the unit.
  • Legacy Azure DevOps remotes such as https://contoso.visualstudio.com/DefaultCollection/Platform/_git/infra-core now match canonical dev.azure.com PR identity, so local manifests and project rules are not discarded.
  • Checkout identity includes the ADO project and recognizes encoded HTTPS paths plus ssh.dev.azure.com:v3/<org>/<project>/<repo> remotes, preventing same-name repositories in another project from supplying manifests or rules.
  • Deep monorepo projects contribute their owning manifests without a full recursive scan: changed manifests and manifests beside/above changed files are read in addition to the shallow root scan. A deeply nested MSTest project now dispatches csharp-mstest instead of unrelated C# product guides.
  • Companion reporting now separates every installed plugin from recognized companion plugins, planned dispatches (six toolkit agents plus one code-review command), and completed output rows. Each run persists companions.json; missing companions are recorded as degraded coverage in the summary, while detection failures remain unknown rather than being mislabeled as not installed.
  • Planned companion reviewer IDs are reconciled against delivered outputs. Missing or duplicate outputs, failed/unverified posts, and failed review prerequisites now return exit 2 with error.txt; parseable findings still receive a diagnostic summary, and detached status reports the run as failed rather than done.
  • Every pass and companion persists its own raw-<reviewer>.json before returning. If the orchestrator ends after its tasks finish but before consolidation, the CLI recovers only when every planned sidecar is valid; partial delivery still fails closed. Valid empty arrays override synthetic “unparseable output” findings.
  • Copilot dispatches the installed pr-review-toolkit agents by their registered short names, so all six companion agents run once instead of first failing under Claude-style qualified names and being relaunched generically.
  • Installed-plugin skill paths are constrained lexically and by real path to the plugin root, and repository MCP configuration is admitted only when checkout identity matches the PR repository. Changed repository MCP files remain untrusted.
  • Trusted repository MCP definitions from root .mcp.json and .vscode/mcp.json are normalized into the isolated run's .mcp.json (root definitions win duplicate names), so every advertised repo capability is actually available to reviewer processes.
  • Dogfood derives GitHub fork identity from origin, rejects unsupported providers and internal/flow-control flags, refuses stale bundles, and includes untracked files only with --include-untracked. Secret-bearing names and high-confidence credential content are refused before artifacts are written; opted-in paths are real-path validated, and binary/empty files are recorded without pretending they contain reviewable lines.
  • Dogfood checks both previousPath and path for tracked renames before excluding generated bundle content, so renaming .env, credentials, keys, or token files to a benign name cannot place the old sensitive path in local review artifacts.
  • ADO gather caches are scoped by the authoritative project as well as organization/repository/PR, stale payload identities are refreshed on cache hits, and unresolved project identities bypass cache reuse.
  • Project-omitted ADO refs are hydrated inside the shared PR resolver, so changed-file, full-diff, comment-read, and comment-post operations retain the authoritative project even when callers do not invoke metadata first; the hydrated cache alias avoids a second PR fetch.
  • Manifest discovery validates real-path containment, ascends past deleted directories to existing owning manifests, and surfaces unexpected discovery/read failures in stack notes. Package diffs only derive dependency evidence from dependency sections or semver-like entries in truncated dependency hunks.
  • The verifier renders as skipped (no HIGH/CRITICAL) when ...
Read more

v0.8.0

Choose a tag to compare

@guimatheus92 guimatheus92 released this 25 Aug 13:21
a54aa9d

[0.8.0] — 2026-08-25

Changed

  • Project rules are never lost to a budget anymore (observed live on Preco-Pratico Backend#616 / Frontend#1067: a 47-skill repo saturated the injection cap at exactly 10 every run, and the 16KB/64KB delivery caps then cut that to 3–4 skills, two of them mid-body — the summary never said so): the relevance heuristic's MAX_HEURISTIC_INJECT cap is gone (EVERY relevant untargeted repo skill injects) and skills-project.md inlines every matched skill body WHOLE (PROJECT_BODY_CAP/PROJECT_FILE_CAP removed, no [truncated:]/[omitted:] markers possible). The review pays the token cost by design rather than silently dropping a business rule.
  • Relevance heuristic THRESHOLD raised 1 → 3 distinct stem hits: measured on real 55/66-file PRs, business skills score 4–115 while non-review content (tool-internal docs, loose readmes) scores 0–2 — threshold 1 marked 47/47 skills relevant on any large diff, making "relevant" meaningless. Small PRs still clear the bar via name+description stems (fixture-backed).

Fixed (self-review of this release with the tool itself — 45 findings triaged, 4 clusters real)

  • The no-loss guarantee now holds in the skill_packs: [] fallback too: overflow beyond the 10-pass cap injects whole as CONTEXT (never demoted to the on-demand index), and a project skill running as its own pass is exempt from PASS_BODY_CAP — only third-party pack bodies still cap at 48KB.
  • Terse skills are no longer structurally unmatchable: the relevance bar adapts down to the skill's own needle count (a 2-token name+description that fully matches the diff injects; before, nothing under 3 needles could ever match).
  • The uncapped skills-project.md reports its size at write time (N project rule(s), X KB) so the deliberate cost stays visible; orphaned comments from the removed caps cleaned up.

v0.7.0

Choose a tag to compare

@guimatheus92 guimatheus92 released this 24 Aug 17:18
0a6b1bc

v0.7.0 — zero built-in reviewers: review passes from synced skill packs, stack-aware (closes #5)

v0.6.1 — a failed write is not proof that nothing was written

Choose a tag to compare

@guimatheus92 guimatheus92 released this 20 Aug 12:31
fc5267d

A 56-comment batch got a 504 after GitHub had already created the review. The blind retry then hit the secondary rate limit precisely because the write had succeeded, the batch read as failed, the per-comment fallback re-posted all 56, and the run reported posted 0 / attempted 56; errors 56 with every comment live. Trusting that number, --resume posted a second copy: 112 comments.

The rule

POSTing a comment is not idempotent, so a 5xx or timeout means unknown, not "nothing written". Everything below follows from that.

  • Providers make ONE attempt and throwpostLineComment and postBatchComments, on all three providers. withRetry is for reads and other idempotent calls. Retry lives in runPost, because only runPost can reconcile first.
  • The PR is read back before any decision — before a retry, before the per-comment fallback, and before reporting a count.
  • Unknown ≠ empty. A failed read-back returns null, never an empty map: the outage that 504s a write is the one that fails the read. Treating those as the same turned 3 findings into 15 live comments in review. On null the run reports and stops.
  • Identity is file:line:body. Two findings can legitimately carry the same body, so a body-only match promoted a finding that was never posted — which then filled posted.marker and locked --resume out of recovering it.
  • Reconciliation is one-way. Errors may be promoted to posted, never the reverse; demoting on a stale read would send the next resume out to write a live comment again.
  • Every publish attempt writes posted.marker, carrying verified. An unverified run fails closed on resume. Gating the write on posted > 0 is what left the incident run with no guard at all.
  • --resume re-reads the PR before deduping (on --dry-run too), unions rather than overwrites, and adopts only comments matching a finding it would post — so a bystander's comment can never suppress a security finding. A failed re-read aborts a publishing resume.

Also

  • Azure DevOps and GitLab now get this fix too. The reconciliation was reachable only through the batch path, which only GitHub implements, so both other providers still duplicated inside postLineComment. isTransientError is now a required PrProvider member.
  • Codex failures are diagnosable: codex-failure.log (argv, exit, timing, bounded stdout + stderr), the synchronous spawn throw is caught where it can still reach that log, stdin EPIPE no longer takes down the review, and exit 0 with no output is an error rather than a silent "found nothing".

Verification

3 findings through the incident shape: 6 live comments on 0.6.0 → 3 on 0.6.1. Under the compound write-and-read outage: 153. Tests 246 → 270; every reconciliation test asserts no path leaves two comments at the same location with the same text.

Full notes in CHANGELOG.md.

v0.6.0

Choose a tag to compare

@guimatheus92 guimatheus92 released this 13 Aug 02:48
56019c6

Added

  • Every real-world PR URL shape now parses. URL parsing moved from per-shape regexes to new URL() + path-segment walking, anchored on _git for Azure DevOps: legacy https://<org>.visualstudio.com/[<collection>/][<project>/]_git/… (with or without DefaultCollection — the exact shape that failed in the field), the project-omitted dev.azure.com/<org>/_git/<repo>/… form, and trailing paths/query strings/fragments on both providers (…/pull/42/files?diff=split). The duplicated ADO host regexes (URL_RES vs orgHost()) collapsed into one parser that computes the org/collection URL once.
  • GitHub Enterprise Server and Azure DevOps Server (on-prem) URLs. PrRef gained an optional baseUrl set by parseUrl (GHES: https://<host>/api/v3, fed to Octokit; ADO Server: https://<host>/<virtualdir>/<collection>, fed to the ADO connection); refs lacking it (older serialized caches) re-derive it from ref.url. Self-hosted hosts resolve only through the new hosts: config map (<hostname>: github | azuredevops | gitlab) — an explicit allowlist, never path-shape guessing, so a credential is only ever sent to a host the user named; the unrecognized-URL error prints the exact yaml to add. GHES auth is host-scoped: GH_ENTERPRISE_TOKEN / GITHUB_ENTERPRISE_TOKEN or gh auth token --hostname <host> — github.com env tokens are deliberately never sent to an enterprise host. Cloud cache keys and run-dir names are byte-identical to before (guarded by a test).
  • GitLab provider. Merge-request URLs (https://gitlab.com/<group>[/<subgroup>]/<project>/-/merge_requests/<iid>, legacy no-/-/ form, and self-managed hosts via the hosts: map) now review end to end: MR metadata + linked closes-issues, per-file diffs (paginated /diffs), existing notes for dedupe, and inline posting as resolvable discussions. Implemented with plain fetch against REST v4 — zero new dependencies. Auth: GITLAB_TOKEN / GITLAB_ACCESS_TOKEN, with glab config get token -h <host> as the CLI fallback (sent as Authorization: Bearer, which accepts both PATs and glab OAuth tokens). Discussion positions carry old_line for context lines via dual-cursor hunk math (positionForLine) — the main cause of GitLab's 400 "position is invalid" — and unanchorable findings re-anchor like GitHub's instead of dropping. GitLab has no batch endpoint, so posting is per-discussion with the existing retry/backoff.

Fixed

  • A bad PR URL now fails --detach immediately in the foreground — with the accepted shapes listed and, for legacy visualstudio.com URLs, the canonical dev.azure.com tip — instead of handing back a run-id whose detached child dies minutes later with status exit 22 (the field incident). URL validation runs before the auth pre-flight and before the run dir is minted; the silent adhoc__ run-dir fallback for unparsable URLs is gone (new resolvePr() choke point used by review, gather, post, cache, and detach).
  • Slashed owners (GitLab nested namespaces) no longer nest run dirs and cache paths. owner keeps its namespace slashes in PrRef (the API needs the full path), but run-dir ids and cache paths flatten it via a shared safeOwner helper — without this, ensureRunDir minted a nested directory, --detach returned basename() of it as the run-id, and status <run-id> looked in the wrong place (every detached GitLab run would read as missing). GitHub/ADO names are unchanged.
  • scripts/test.mjs now discovers tests recursivelytests/providers/*.test.ts was silently ignored by the flat readdirSync, despite add-provider.md promising the nested layout.
  • ci-integration.md's ADO pipeline example built a doubled URL (https://dev.azure.com/ prefixed onto System.TeamFoundationCollectionUri, which already expands to that) — it now uses the variable directly.

v0.4.2

Choose a tag to compare

@guimatheus92 guimatheus92 released this 07 Aug 15:02
2ce10b6

Fixed

  • A pipeline failure is no longer reported as a clean review. When the orchestrator produced no parseable findings (exit code 2), finalizeReview still wrote a normal zero-finding pr-review-summary.md and a done progress event — and status treats the summary's existence as "done, exit 0", so a detached run that failed presented as a clean PR. On findingsUnavailable the run now writes the failure to error.txt instead of minting the summary, emits an error progress event, and status reports failed (exit 22) with the message inline. Codex second-opinion findings collected before the failure are still posted, and the exit code stays 2.
  • Stdout salvage now recovers findings from a narrated orchestrator transcript. The JSON parser extracted exactly one value — the first fenced block or the earliest [/{, which a prose bracket like [security] could win, defeating the whole parse. parseJsonFindings now merges findings from every JSON block in the blob (all fenced blocks — excised so nothing parses twice — then every balanced value, with structural recursion reaching nested payloads) and also understands the orchestrator's own {"reviewers":[{"name":…,"findings":[…]}]} file payload printed to stdout instead of written. Values found loose in prose must pass a strict finding-shape gate, so quoted log objects are not minted into findings.
  • orchestrator-failure.log now keeps the orchestrator's full stdout/stderr (was: last 8 KB tails). When the contract fails, stdout may hold the only copy of the reviewer findings, and a tail made even manual salvage impossible.
  • status no longer reports interrupted (with a dead-end --resume hint) when the findings file on disk is corrupt. The interrupted state now requires a resumable output file — one that parses to the {reviewers:[…]} shape --resume actually loads. A truncated/corrupt file falls through to failed with error.txt surfaced inline; a valid phase1-findings.json fallback still reads interrupted because resume genuinely recovers it. (Found by running this release's own reviewer against its PR.)

Full details: #10

🤖 Generated with Claude Code