Skip to content

v0.12.0

Choose a tag to compare

@guimatheus92 guimatheus92 released this 07 Sep 18:06
· 5 commits to main since this release
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 unchanged.
  • A deleted Azure DevOps comment is no longer an existing comment. ADO does not remove a deleted comment; it tombstones it, leaving the entry in the thread with isDeleted: true and empty content, and fetchExistingComments returned every one. A fixture PR reviewed a handful of times carried 185 threads holding 183 tombstones against 2 live comments, and pr-review verify read them as 63 inline comments matching no planned finding (INV-POST-06, "a dispatched agent wrote to the PR") plus 14 duplicated locations (INV-POST-05) — both false, and both the kind of alarm that gets an invariant switched off. The reach was wider than the audit: tombstones entered every review pass as prior PR discussion through gather.existingComments, and dedupeAgainstExisting compared real findings against them; only their empty bodies kept that from suppressing findings.
  • pr-review dogfood refused diffs it should review. The secret gate rejected .env.example on its name, so the branch that added the file could not be dogfooded at all — and dogfooding is a required pre-PR step. Template names (.example, .sample, .template, .dist, .default) are exempt from the name check only; their content is still scanned, so a real token committed inside one is still refused. A second false refusal came from const CREDENTIAL_FILES = [, where the sensitive-key heuristic matched the name and then counted the bare [ as a material value.
  • The acceptance control fixture was not actually clean. greetHandler exists so the false-positive assertion fails if a reviewer flags correct code, but it carried a doc comment ("greet a user by name") that its body contradicted (it greets by email) and a q<User> generic over a query selecting only email. Both were real defects the live reviewers found. The assertion itself moved from regex-over-finding-text to file:line after four text shapes each failed on a correct review — which function a finding mentions says nothing about which one it is about.

Removed

  • PrProvider.fetchFullDiff is retired. Nothing in the pipeline read fullDiff: GitHub had returned '' since 0.11.0, while Azure DevOps still spent a getCommitDiffs call and GitLab still concatenated its per-file diffs, only to fill a field written to pr-review-gather.json and never consumed. fullDiff is gone from GatherOutput entirely, so the compiler — not a code sweep — is what proves no reader survives. Cache entries and --from-gather payloads produced by <= 0.11 still carry the key, stay valid and need no cache clear: both readers are unchecked JSON casts, so an undeclared key round-trips untouched, and a cache hit still copies it through into the run-dir artifact. The per-file patches in changedFiles are the diff. Fixes #26.

Documentation

  • The mistakes this work cost, written where the next person hits them: AGENTS.md on how a green test can encode the defect (an assertion transcribing current output, a fixture comparing shapes the product never writes, a control that answers its own question) and on scripted edits that slice ranges or pass through a shell heredoc; evals/acceptance/README.md on re-seeding without rewriting the base branch, on the Azure DevOps directory switch, on the GitLab granular-PAT screen, and on BLOCKED vs FAIL; and the acceptance workflow on why its dry-run flag is written == false (Actions treats '' as falsy, so the natural-looking form always dry-runs).