mirror of
https://github.com/expo/code-review-cli.git
synced 2026-11-15 03:38:09 -10:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
 Tomasz SapetaandBrent Vatne
|
bd06ab13a9 | ||
|
Brent Vatne
|
4baf23834a | ||
|
Kudo ChienandClaude Opus 5.5
|
e18dc16ba1 |
No files matched your search
@@ -372,6 +372,16 @@ Enforced in code and by an independent CI guard step, not by convention:
|
||||
rosters, prompts, or routing is reviewed under the **previous** config, and a
|
||||
missing base fails closed — never a fallback to the checkout. (Temporary escape
|
||||
hatch: `ecr ci --unsafe-config-from-head`, with a loud warning; to be removed.)
|
||||
- **The review covers one commit, and can be pinned to the one you asked for.**
|
||||
`ecr ci` reads the PR's base and head commits once. The tree it reads is checked
|
||||
out at that head, and the diff compares those two commits, so a push during the
|
||||
run cannot change either. `ecr ci --expect-head <sha>` also stops when the head is
|
||||
not `<sha>`, so a workflow can pass the commit a maintainer labeled
|
||||
(`github.event.pull_request.head.sha`) and a push before the run starts is not
|
||||
reviewed in its place. With a `routing.jsonc`, the run only logs the reason,
|
||||
since a note would replace the aggregate comment and its per-scope state.
|
||||
Releases without the flag ignore it, so pin a release whose `ecr ci --help`
|
||||
lists it.
|
||||
- **The model runtime never sees PR-owned ambient config.** The head worktree is
|
||||
scrubbed of `opencode.json{,c}`, `.opencode/`, `AGENTS.md`, `CLAUDE.md`,
|
||||
`.claude/`, `.mcp.json`, `.cursor*`, and `.env*` before the engine starts, so a
|
||||
|
||||
@@ -6,10 +6,10 @@
|
||||
"name": "@expo/code-review-cli",
|
||||
"dependencies": {
|
||||
"@modelcontextprotocol/sdk": "1.30.0",
|
||||
"@opencode-ai/sdk": "1.18.4",
|
||||
"@opencode-ai/sdk": "1.19.5",
|
||||
"cheerio": "1.1.2",
|
||||
"minisearch": "7.2.0",
|
||||
"opencode-ai": "1.18.4",
|
||||
"opencode-ai": "1.19.5",
|
||||
"zod": "^4.4.3",
|
||||
},
|
||||
"devDependencies": {
|
||||
@@ -29,7 +29,7 @@
|
||||
|
||||
"@modelcontextprotocol/sdk": ["@modelcontextprotocol/[email protected]", "", { "dependencies": { "@hono/node-server": "^1.19.9 || ^2.0.5", "ajv": "^8.17.1", "ajv-formats": "^3.0.1", "content-type": "^1.0.5", "cors": "^2.8.5", "cross-spawn": "^7.0.5", "eventsource": "^3.0.2", "eventsource-parser": "^3.0.0", "express": "^5.2.1", "express-rate-limit": "^8.2.1", "hono": "^4.11.4", "jose": "^6.1.3", "json-schema-typed": "^8.0.2", "pkce-challenge": "^5.0.0", "raw-body": "^3.0.0", "zod": "^3.25 || ^4.0", "zod-to-json-schema": "^3.25.1" }, "peerDependencies": { "@cfworker/json-schema": "^4.1.1" }, "optionalPeers": ["@cfworker/json-schema"] }, "sha512-xKd8OIzlqNzcqcNumGAa6g+PW2kjD5vrpcKOnfldAUPP3j7lnqMPwlTXQm8gF+UwH72z0lqaRbjr9hqGz0eITA=="],
|
||||
|
||||
"@opencode-ai/sdk": ["@opencode-ai/[email protected]8.4", "", { "dependencies": { "cross-spawn": "7.0.6" } }, "sha512-p/3P0KtWknoLvpsk8QrUzCKd4Q1A5Z3RmACEvwuQBGxnUy4AGo379lUYMgt7fczFn53079oroLuo6BosQri+HA=="],
|
||||
"@opencode-ai/sdk": ["@opencode-ai/[email protected]9.5", "", { "dependencies": { "cross-spawn": "7.0.6" } }, "sha512-+bqu8gHiwmq96T9q5G34uLNHWCInIMsrE3hytCNHj1cVaxjY0AGJyNCT1rkeeZwgPU7A5TMTNwN+0XxCO2vqDg=="],
|
||||
|
||||
"@oxfmt/binding-android-arm-eabi": ["@oxfmt/[email protected]", "", { "os": "android", "cpu": "arm" }, "sha512-1q4q4Jc8FlOMVojEisyFAVyl8h1yawNv6phjgmhGVEDeyeOdsSnSr9x0+D4mOnEKvpO5L4mxKZ/DP9X6U3A/Mw=="],
|
||||
|
||||
@@ -271,31 +271,31 @@
|
||||
|
||||
"once": ["[email protected]", "", { "dependencies": { "wrappy": "1" } }, "sha512-lNaJgI+2Q5URQBkccEKHTQOPaXdUxnZZElQTZY0MFUAuaEqe1E+Nyvgdz/aIyNi6Z9MzO5dv1H8n58/GELp3+w=="],
|
||||
|
||||
"opencode-ai": ["[email protected]8.4", "", { "optionalDependencies": { "opencode-darwin-arm64": "1.18.4", "opencode-darwin-x64": "1.18.4", "opencode-darwin-x64-baseline": "1.18.4", "opencode-linux-arm64": "1.18.4", "opencode-linux-arm64-musl": "1.18.4", "opencode-linux-x64": "1.18.4", "opencode-linux-x64-baseline": "1.18.4", "opencode-linux-x64-baseline-musl": "1.18.4", "opencode-linux-x64-musl": "1.18.4", "opencode-windows-arm64": "1.18.4", "opencode-windows-x64": "1.18.4", "opencode-windows-x64-baseline": "1.18.4" }, "os": [ "linux", "win32", "darwin", ], "cpu": [ "x64", "arm64", ], "bin": { "opencode": "bin/opencode.exe" } }, "sha512-B8pFAs1g158ZU+C6eSiJz/5ZhG7Vr2mH7Z7ruYEHLElAlX9tehrRTnzTTgjSxHY2vwZ/5VplwR3odFU+Dai8jg=="],
|
||||
"opencode-ai": ["[email protected]9.5", "", { "optionalDependencies": { "opencode-darwin-arm64": "1.19.5", "opencode-darwin-x64": "1.19.5", "opencode-darwin-x64-baseline": "1.19.5", "opencode-linux-arm64": "1.19.5", "opencode-linux-arm64-musl": "1.19.5", "opencode-linux-x64": "1.19.5", "opencode-linux-x64-baseline": "1.19.5", "opencode-linux-x64-baseline-musl": "1.19.5", "opencode-linux-x64-musl": "1.19.5", "opencode-windows-arm64": "1.19.5", "opencode-windows-x64": "1.19.5", "opencode-windows-x64-baseline": "1.19.5" }, "os": [ "linux", "win32", "darwin", ], "cpu": [ "x64", "arm64", ], "hasInstallScript": true, "bin": { "opencode": "bin/opencode.exe" } }, "sha512-tEqMvQahfhdDj8vceYpXtoIrvjPXm5XRfetWnPN5YISueU7adNx9JcUA3zZ+2GKuDVgDpGdQe1ZLtCZDjqodvA=="],
|
||||
|
||||
"opencode-darwin-arm64": ["[email protected]8.4", "", { "os": "darwin", "cpu": "arm64" }, "sha512-/GdcUv3axBFYmpdCY3yMGRCBq3fwAJayflnn9stXtIHKuckc7ZaUYZmyudIUpmzkZP5W8XsAVMJomR4Rbr2+Ig=="],
|
||||
"opencode-darwin-arm64": ["[email protected]9.5", "", { "os": "darwin", "cpu": "arm64" }, "sha512-0MbSbKkmwVHtgP31GJyyECe6QVy4T7W4Wp7s2TsmXHTPPyf01ne3dWSX97X9dkNK28Lc6kt+zMPQUP/9GgDLZg=="],
|
||||
|
||||
"opencode-darwin-x64": ["[email protected]8.4", "", { "os": "darwin", "cpu": "x64" }, "sha512-0i/AmdFw3CerwIggKqtaDlWDCBfSgzXT+KPSvks/Dx6l9NfYHihLnbV+vKbr0lwzco1lsDo4JHE4j/zz7JZUKQ=="],
|
||||
"opencode-darwin-x64": ["[email protected]9.5", "", { "os": "darwin", "cpu": "x64" }, "sha512-+DO7IMPTCrX12UJXnL4pL3ckcwR6BR6f6tA9QIlVQ5nZx66T5U7FRMJT4n5rsOnJxLM/pZ2vfX1G8vrnvXO8FA=="],
|
||||
|
||||
"opencode-darwin-x64-baseline": ["[email protected]8.4", "", { "os": "darwin", "cpu": "x64" }, "sha512-1v/keJ/m+3UplGU2XdP5YfK25rTlGi7Kf5BVSV/ICObk6PiJWBAWOrB8bAQWDLhN/Tn7UfogBAgxaiRRfLPuEA=="],
|
||||
"opencode-darwin-x64-baseline": ["[email protected]9.5", "", { "os": "darwin", "cpu": "x64" }, "sha512-rftvAoHBWPVkvwBLr3SQWRZvcIjyMTN0SlhMzlt+lgvoQBBGNEmZQ/7sS/Ux1oHsJV/++UDuLVso4n5L95YD5Q=="],
|
||||
|
||||
"opencode-linux-arm64": ["[email protected]8.4", "", { "os": "linux", "cpu": "arm64" }, "sha512-3gBw9weE76jloNPGmPvzl3GJR8b6scwSAENxUKRdJ3WCkC7qcjJm49KFyfqFjO5++1H1Q2jbmqqxAgt5upqoMQ=="],
|
||||
"opencode-linux-arm64": ["[email protected]9.5", "", { "os": "linux", "cpu": "arm64" }, "sha512-u2HFRI6v1nNMVKiV34An8XrGBxJeZ+RCg2tYu3SqlGODKqKsr5RoEbaX1tnsfGb+rETh9AJVX9kS8dSZXPufsA=="],
|
||||
|
||||
"opencode-linux-arm64-musl": ["[email protected]8.4", "", { "os": "linux", "cpu": "arm64" }, "sha512-kno1QJz+JoY1YqGlrxcwGKqOh2/OFKdQ4UGUCN5K2evQRIhd9EfB4JvZkjzLxTzglda8lpP+PJltjCA4DHJAJw=="],
|
||||
"opencode-linux-arm64-musl": ["[email protected]9.5", "", { "os": "linux", "cpu": "arm64", "libc": "musl" }, "sha512-YxvpVPJ/jeevV93DVRiuOVbhkoJAuwQUadPDHPaw65aZkAKZ6OE7XySwCS+Rjlhzt7fPryisvAOjUtZkwfMBuQ=="],
|
||||
|
||||
"opencode-linux-x64": ["[email protected]8.4", "", { "os": "linux", "cpu": "x64" }, "sha512-mCiZuKQBqRHVhtp20YFDfNf5cUwTt6/I98MFJQDJdDTWggnkAVOnitQTmSt2cQwoPJ94VrRwDhZyY3pIY3+CQg=="],
|
||||
"opencode-linux-x64": ["[email protected]9.5", "", { "os": "linux", "cpu": "x64" }, "sha512-i1qh7VooLGML9xX6ud93y0JA8j/1aPeDYRKjsmYXrJX1TjIWHgBBMkEbEJPZthpWbsJg//XfBgR6Kb7+NPaJiQ=="],
|
||||
|
||||
"opencode-linux-x64-baseline": ["[email protected]8.4", "", { "os": "linux", "cpu": "x64" }, "sha512-LlVIv54gpM8I6QJVAoW5uvcaVO4z+y5DswfUCsxgaD0CWgjEbULaiAAZIS/L+N5rRw/jHeqcHSXiKinFZt0xww=="],
|
||||
"opencode-linux-x64-baseline": ["[email protected]9.5", "", { "os": "linux", "cpu": "x64" }, "sha512-9kA+st3XXtCXMPx5f3Ld1u19uoHTO2u2Btt8a2cqhJ4GIqVbD1mXLevj3LHFEmtuQoFGk+UstGnl38sCqXj5sQ=="],
|
||||
|
||||
"opencode-linux-x64-baseline-musl": ["[email protected]8.4", "", { "os": "linux", "cpu": "x64" }, "sha512-hM/uPMEuxLbwXueOmHsf3jjmfJ238/3r6FZMPumKVMR7jJHutp3ZWuOIL+ZzByxzsUK1f3RGbKaFULOMIieRSw=="],
|
||||
"opencode-linux-x64-baseline-musl": ["[email protected]9.5", "", { "os": "linux", "cpu": "x64", "libc": "musl" }, "sha512-kwt6hmTaI6nujjAzdAkXv3j2rDzXNOMfMk8Jf8bwacHtwUl8CWA2psDVg3nWyb4zByAa6F1jSIlmHCPHBq1K/Q=="],
|
||||
|
||||
"opencode-linux-x64-musl": ["[email protected]8.4", "", { "os": "linux", "cpu": "x64" }, "sha512-AlojHLyv7Cgn2g5Rj5QeHckEWyA20qdbEla2d1R9mXSaqlP6sX7izNti8Tzr/vMG53Uwmppyjb37uFB2fK+YTQ=="],
|
||||
"opencode-linux-x64-musl": ["[email protected]9.5", "", { "os": "linux", "cpu": "x64", "libc": "musl" }, "sha512-y6nUbH3ViajN37KaVVy0/KnWv/i48Bx5QPv9Ul9ONJya1ossV7CHVb+ZNFc61M5GqlVtjCvZV7T0eBMqhJG7mg=="],
|
||||
|
||||
"opencode-windows-arm64": ["[email protected]8.4", "", { "os": "win32", "cpu": "arm64" }, "sha512-B6m7N7ZPj/E6xx1ZaSjNW0gHTd0b3NTSf9mQiN3W89zji3oH9VOHLQdrP10rAd/2uZ5ZttxSkRqj/iwgAW+3TQ=="],
|
||||
"opencode-windows-arm64": ["[email protected]9.5", "", { "os": "win32", "cpu": "arm64" }, "sha512-aySF/MDji5KO3I5yl9Ec4g8kW/QIx4Jj5FXwBhh/FrDu0w6L8HAOGH7cyjFb3t8TW7pxRkH3Ei3rfkWAE7cScw=="],
|
||||
|
||||
"opencode-windows-x64": ["[email protected]8.4", "", { "os": "win32", "cpu": "x64" }, "sha512-TumOcMOvZ6vfs348LgGLK9eFEBLsh+wI/UXmQ8BhXW95YV1Wd26TutSzQi4L/wLftsDfl8Q6bKQ2trWtGBzBvQ=="],
|
||||
"opencode-windows-x64": ["[email protected]9.5", "", { "os": "win32", "cpu": "x64" }, "sha512-xCVbrA8A/qQTAdBIKyhK5tx3/mcq18yF82Mi8phry0MJb2631nnnpxF9m6FFfWAVHyer4qFGrO7hbWXesHQ1Gg=="],
|
||||
|
||||
"opencode-windows-x64-baseline": ["[email protected]8.4", "", { "os": "win32", "cpu": "x64" }, "sha512-4EpZ97uI50vVv9UT5sDeK3ff+sFzqmbBT4TNGkMF9uwNTKJYqoSVuOIZhIfLTLNlrEK2hd2/sf/4GQhIkM6iTw=="],
|
||||
"opencode-windows-x64-baseline": ["[email protected]9.5", "", { "os": "win32", "cpu": "x64" }, "sha512-yRHN+TsfLjfLmdQVAVWqTGfGP1yq8YtUFy9Jw33XhlPIp77slBdCJrj2PC4amuEl6BJtaIQDhDFbpJ8+a5z7Qw=="],
|
||||
|
||||
"oxfmt": ["[email protected]", "", { "dependencies": { "tinypool": "2.1.0" }, "optionalDependencies": { "@oxfmt/binding-android-arm-eabi": "0.60.0", "@oxfmt/binding-android-arm64": "0.60.0", "@oxfmt/binding-darwin-arm64": "0.60.0", "@oxfmt/binding-darwin-x64": "0.60.0", "@oxfmt/binding-freebsd-x64": "0.60.0", "@oxfmt/binding-linux-arm-gnueabihf": "0.60.0", "@oxfmt/binding-linux-arm-musleabihf": "0.60.0", "@oxfmt/binding-linux-arm64-gnu": "0.60.0", "@oxfmt/binding-linux-arm64-musl": "0.60.0", "@oxfmt/binding-linux-ppc64-gnu": "0.60.0", "@oxfmt/binding-linux-riscv64-gnu": "0.60.0", "@oxfmt/binding-linux-riscv64-musl": "0.60.0", "@oxfmt/binding-linux-s390x-gnu": "0.60.0", "@oxfmt/binding-linux-x64-gnu": "0.60.0", "@oxfmt/binding-linux-x64-musl": "0.60.0", "@oxfmt/binding-openharmony-arm64": "0.60.0", "@oxfmt/binding-win32-arm64-msvc": "0.60.0", "@oxfmt/binding-win32-ia32-msvc": "0.60.0", "@oxfmt/binding-win32-x64-msvc": "0.60.0" }, "peerDependencies": { "svelte": "^5.0.0", "vite-plus": "*" }, "optionalPeers": ["svelte", "vite-plus"], "bin": { "oxfmt": "bin/oxfmt" } }, "sha512-fViX6i+gJuZWY+jI/fnR6WRbRj70GZ9RlCd30MygJrHTUNc4DxvKHWw8vBjMjffv3PgU5qWDR0AzmojQByqaZA=="],
|
||||
|
||||
|
||||
@@ -77,6 +77,8 @@ publishing stale findings or rendering them under rules the model run never saw.
|
||||
|
||||
**Automated reruns reuse unchanged complete results.** Before launching agents, CI hashes the review job's scoped diff and stable review inputs and compares it with the hash hidden in the newest author-verified reviewer comment. An exact match re-renders the stored result (so links, dismissals, and live annotation-only feedback still update) without spending model budget. Routed CI does this independently per scope. Manual `/review`/`--force`, stack-aware runs, model-backed feedback runs, partial/failed results, and truncated aggregate state all bypass reuse and run fresh. The hash contract and durable-state rules are owned by LLP 0005's Review Result Cache section.
|
||||
|
||||
**`--expect-head` pins the run to the commit the request named.** A workflow on a label event knows the commit the maintainer labeled (`github.event.pull_request.head.sha`), but `ecr ci` starts minutes later and reads the PR's head then, so a push in between would be reviewed with the secrets in place of the labeled code. With `--expect-head <sha>`, both CI paths compare the head they read with `<sha>` after the break-glass check and before any model call, and stop when it differs or is unknown (`expectedHeadMismatch` in [ci.ts](../src/commands/ci.ts)). The single-config path posts a `couldNotComplete` "not reviewed" note (`postNotReviewed`). The routed path only logs and removes the reviewer's ready label: its root comment holds the aggregate and its per-scope state, which a single-review render would replace, so a later `--scopes` run would lose the other scopes' results. A malformed flag (`--expect-head=<sha>`, an empty value, a short hash, the flag twice) is a usage error, never ignored, since an ignored flag would review an unpinned head without a word (`parseExpectHead`). Releases from before the flag ignore it like any unknown argument, so a workflow that passes it must pin a release that has it. The diff compares the same base and head commits the trees are checked out at (see [LLP 0008](./0008-sources-and-reporters.explainer.md#pr-head-materialization)), so a push after the check cannot swap the diff either [observed] (tests `src/__tests__/github-pr-head.test.ts`).
|
||||
|
||||
`--unsafe-config-from-head` is a deliberate escape hatch that reverses this model, loading config from the checkout. It is never scaffolded by `ecr init`, always prints a loud `⚠ SECURITY` warning, and is scheduled for removal on a minor version boundary [observed: `src/commands/ci.ts:85-91` `CI_USAGE`, `155-162` warning].
|
||||
|
||||
`--context-file` in CI takes the opposite read-error policy from `ecr review`, and the split is dictated by the never-fail-checks invariant. `ci` reads the file once before any fan-out and, on a missing/oversized/unreadable path, WARNs to stderr and continues with no context — it never fails, because a broken Atlantis-provided plan file must not turn the PR's check red [observed: `src/commands/ci.ts` `--context-file` read → stderr warn, no `exitCode`, `contextText` stays undefined]. The same reader (`readContextFile`, capped at `MAX_CONTEXT_FILE_BYTES`) backs both commands; only the catch policy differs — fatal locally, warn in CI. The byte-bounded raw text threads through the shared `CiRunOptions` into both `runLegacyCi` and `runRoutedCi`, and from there into every per-scope `runReview` as `contextText` (char-capped at prompt build) [observed: `src/commands/ci.ts` `CiRunOptions.contextText`; `runReview` calls in `runLegacyCi` and the fan-out loop].
|
||||
|
||||
@@ -37,6 +37,8 @@ The checkout targets the specific OID, never `FETCH_HEAD`. `FETCH_HEAD` would wo
|
||||
|
||||
**Token handling.** Git fetches APPEND a gh credential helper (`-c credential.helper=!gh auth git-credential`) rather than replacing the credential config [observed] ([github-pr.ts:26-32, 120](../src/sources/github-pr.ts)). Appending, not overriding, has two reasons: `GH_TOKEN` flows via the credential-helper protocol so it never lands in argv or `.git/config` (works even under `persist-credentials: false`), and a developer's own existing helpers still run first, so local behavior is unchanged [observed] ([github-pr.ts:26-30](../src/sources/github-pr.ts); commit 601b19a). Replacing it with a sole `credential.helper` override would break developer machines that rely on their own helpers.
|
||||
|
||||
**The diff is pinned to the same OIDs.** `gh pr diff` has no commit argument and returns the PR's diff as it is now, so a push during the run would give the agents a diff from another commit than the tree they read. `getChangedFiles` instead fetches the compare of the base and head OIDs `getMetadata` read (`GET /repos/{owner}/{repo}/compare/{base}...{head}` with the `application/vnd.github.diff` media type). That is the PR's three-dot diff: on one same-repository PR and three fork PRs it matched `gh pr diff` byte for byte [observed: 2026-11-02]. Only when an OID is unknown, or the metadata read failed, does it fall back to `gh pr diff` [observed] ([github-pr.ts](../src/sources/github-pr.ts) `getChangedFiles`; tests `src/__tests__/github-pr-head.test.ts`). A push after the metadata read can still stop the run: the head tree fetches `refs/pull/<n>/head` at depth 1 and then checks out the OID, which fails when the runner does not already have that commit (see `materializeWorktreeAsync`).
|
||||
|
||||
## The Trusted Base Root
|
||||
|
||||
`prepareTrustedConfigRootAsync` materializes the PR **BASE** commit — not the head — as a separate worktree [observed] ([github-pr.ts:194-210](../src/sources/github-pr.ts); commit 601b19a). This is the whole trust story of CI in one method. Review policy, prompts, routing, and auth mapping all load from the base tree, so a PR cannot change the reviewer that evaluates it — config changes only take effect after they merge into the base branch [observed] ([github-pr.ts:195-197](../src/sources/github-pr.ts); commit 601b19a: "loads all review configuration from the PR's immutable base commit, never from the PR head"). The base tree is trusted, so it is *not* scrubbed; the head tree is untrusted, so it is. Two worktrees, two trust levels, by construction.
|
||||
|
||||
+2
-2
@@ -44,10 +44,10 @@
|
||||
},
|
||||
"dependencies": {
|
||||
"@modelcontextprotocol/sdk": "1.30.0",
|
||||
"@opencode-ai/sdk": "1.18.4",
|
||||
"@opencode-ai/sdk": "1.19.5",
|
||||
"cheerio": "1.1.2",
|
||||
"minisearch": "7.2.0",
|
||||
"opencode-ai": "1.18.4",
|
||||
"opencode-ai": "1.19.5",
|
||||
"zod": "^4.4.3"
|
||||
},
|
||||
"devDependencies": {
|
||||
|
||||
@@ -0,0 +1,113 @@
|
||||
import { test, expect, afterEach, beforeEach } from "bun:test";
|
||||
import { chmod, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import path, { delimiter } from "node:path";
|
||||
|
||||
import { expectedHeadMismatch, parseExpectHead } from "../commands/ci.js";
|
||||
import { resetTrustedToolCache } from "../core/exec.js";
|
||||
import { GitHubPRSource } from "../sources/github-pr.js";
|
||||
|
||||
const HEAD = "0123456789abcdef0123456789abcdef01234567";
|
||||
const BASE = "89abcdef0123456789abcdef0123456789abcdef";
|
||||
|
||||
test("parseExpectHead: a full commit hash, lowercased; anything else is a usage error", () => {
|
||||
expect(parseExpectHead([])).toBeUndefined();
|
||||
expect(parseExpectHead(["--expect-head", HEAD.toUpperCase()])).toBe(HEAD);
|
||||
expect(() => parseExpectHead(["--expect-head", "0123456"])).toThrow("40-character");
|
||||
expect(() => parseExpectHead(["--expect-head", "--force"])).toThrow("requires a value");
|
||||
expect(() => parseExpectHead(["--expect-head"])).toThrow("requires a value");
|
||||
// Forms that would otherwise be ignored, and leave the run unpinned, fail loudly.
|
||||
expect(() => parseExpectHead([`--expect-head=${HEAD}`])).toThrow("separate argument");
|
||||
expect(() => parseExpectHead(["--expect-head", ""])).toThrow("is empty");
|
||||
expect(() => parseExpectHead(["--expect-head", HEAD, "--expect-head", ""])).toThrow(
|
||||
"more than once",
|
||||
);
|
||||
});
|
||||
|
||||
test("expectedHeadMismatch: only a named commit that is not the head stops the run", () => {
|
||||
expect(expectedHeadMismatch(undefined, HEAD)).toBeNull();
|
||||
expect(expectedHeadMismatch(undefined, undefined)).toBeNull();
|
||||
expect(expectedHeadMismatch(HEAD, HEAD)).toBeNull();
|
||||
expect(expectedHeadMismatch(HEAD, BASE)).toBe(
|
||||
`the PR's head is ${BASE}, not ${HEAD}, the commit this review was requested for`,
|
||||
);
|
||||
// An unknown head fails closed.
|
||||
expect(expectedHeadMismatch(HEAD, undefined)).toContain("the PR's head is unknown");
|
||||
});
|
||||
|
||||
// ---- the diff is pinned to the OIDs the metadata read, through a fake `gh` ----
|
||||
|
||||
// A fake `gh` that logs each call. `pr view` prints FAKE_GH_VIEW (both OIDs when
|
||||
// unset) and fails when it is "fail"; `api` and `pr diff` print the same diff.
|
||||
const FAKE_GH = `#!/bin/sh
|
||||
printf '%s\\n' "$*" >> "$FAKE_GH_DIR/calls.log"
|
||||
case "$*" in
|
||||
"api "*|"pr diff"*)
|
||||
printf 'diff --git a/a.ts b/a.ts\\n'
|
||||
printf 'index 1111111..2222222 100644\\n'
|
||||
printf -- '--- a/a.ts\\n'
|
||||
printf '+++ b/a.ts\\n'
|
||||
printf '@@ -1 +1,2 @@\\n'
|
||||
printf ' one\\n'
|
||||
printf '+two\\n' ;;
|
||||
"pr view"*)
|
||||
[ "$FAKE_GH_VIEW" = fail ] && exit 1
|
||||
printf '%s' "\${FAKE_GH_VIEW:-{\\"headRefOid\\": \\"${HEAD}\\", \\"baseRefOid\\": \\"${BASE}\\"}}" ;;
|
||||
*) echo "unexpected gh call: $*" >&2; exit 1 ;;
|
||||
esac
|
||||
`;
|
||||
|
||||
let dir = "";
|
||||
const saved: Record<string, string | undefined> = {};
|
||||
|
||||
beforeEach(async () => {
|
||||
dir = await mkdtemp(path.join(tmpdir(), "ecr-gh-pr-head-"));
|
||||
await writeFile(path.join(dir, "gh"), FAKE_GH, "utf8");
|
||||
await chmod(path.join(dir, "gh"), 0o755);
|
||||
await writeFile(path.join(dir, "calls.log"), "", "utf8");
|
||||
for (const name of ["PATH", "FAKE_GH_DIR", "FAKE_GH_VIEW"]) {
|
||||
saved[name] = process.env[name];
|
||||
}
|
||||
process.env.PATH = `${dir}${delimiter}${saved.PATH ?? ""}`;
|
||||
process.env.FAKE_GH_DIR = dir;
|
||||
delete process.env.FAKE_GH_VIEW;
|
||||
resetTrustedToolCache();
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
for (const [name, value] of Object.entries(saved)) {
|
||||
if (value === undefined) {
|
||||
delete process.env[name];
|
||||
} else {
|
||||
process.env[name] = value;
|
||||
}
|
||||
}
|
||||
resetTrustedToolCache();
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
/** Run getChangedFiles on a fresh source and return the gh call that fetched the diff. */
|
||||
async function diffCall(options: { repo?: string }): Promise<string> {
|
||||
await writeFile(path.join(dir, "calls.log"), "", "utf8");
|
||||
const files = await new GitHubPRSource({ prNumber: 7, ...options }).getChangedFiles();
|
||||
expect(files.map((file) => file.path)).toEqual(["a.ts"]);
|
||||
const calls = (await readFile(path.join(dir, "calls.log"), "utf8")).trim().split("\n");
|
||||
expect(calls).toHaveLength(2);
|
||||
expect(calls[0]).toStartWith("pr view");
|
||||
return calls[1]!;
|
||||
}
|
||||
|
||||
test("getChangedFiles: the diff compares the base and head OIDs the metadata read", async () => {
|
||||
expect(await diffCall({ repo: "o/r" })).toBe(
|
||||
`api repos/o/r/compare/${BASE}...${HEAD} -H Accept: application/vnd.github.diff`,
|
||||
);
|
||||
// Without --repo, gh resolves the placeholders from the checkout, as `gh pr` does.
|
||||
expect(await diffCall({})).toStartWith(`api repos/{owner}/{repo}/compare/${BASE}...${HEAD} `);
|
||||
});
|
||||
|
||||
test("getChangedFiles: an unknown OID or a failed metadata read falls back to gh pr diff", async () => {
|
||||
process.env.FAKE_GH_VIEW = `{"baseRefOid": "${BASE}"}`;
|
||||
expect(await diffCall({ repo: "o/r" })).toStartWith("pr diff 7 --repo o/r");
|
||||
process.env.FAKE_GH_VIEW = "fail";
|
||||
expect(await diffCall({ repo: "o/r" })).toStartWith("pr diff 7 --repo o/r");
|
||||
});
|
||||
@@ -45,6 +45,20 @@ test("setup note renders only when there is ref advice, and never as a finding",
|
||||
expect(body).toContain("No findings.");
|
||||
});
|
||||
|
||||
test("a run that reviewed nothing renders No review, without claiming why", () => {
|
||||
const body = renderMarkdown({ ...base, couldNotComplete: true }, "tag");
|
||||
expect(body).toContain("**Decision:** No review\n");
|
||||
const aggregate = renderAggregateMarkdown(
|
||||
[
|
||||
{ scope: "app", review: { ...base, couldNotComplete: true } },
|
||||
{ scope: "docs", review: base },
|
||||
],
|
||||
"tag",
|
||||
[],
|
||||
);
|
||||
expect(aggregate).toContain("| app | No review | 0 |");
|
||||
});
|
||||
|
||||
test("coverage note only renders when incomplete is non-empty (no more wolf-crying)", () => {
|
||||
expect(renderMarkdown(base, "tag")).not.toContain("Coverage note");
|
||||
expect(renderMarkdown({ ...base, incomplete: ["a pass timed out"] }, "tag")).toContain(
|
||||
|
||||
+120
-27
@@ -36,7 +36,7 @@ import {
|
||||
reviewInputHash,
|
||||
reviewMatchesInput,
|
||||
} from "../core/review-cache.js";
|
||||
import { GitHubPRSource } from "../sources/github-pr.js";
|
||||
import { GitHubPRSource, isCommitOid } from "../sources/github-pr.js";
|
||||
import { memoizeSource, stackConfirmFromConfig, stackWalkFromConfig } from "../sources/source.js";
|
||||
import type { PreparedReadRoot, ReviewSource, StackWalkOptions } from "../sources/source.js";
|
||||
import { GitHubReporter } from "../reporters/github.js";
|
||||
@@ -127,6 +127,10 @@ Options:
|
||||
--force Manual override: review even if the trigger policy (label
|
||||
trigger / ai-review:skip) would skip. Break-glass and the
|
||||
auth lock still apply. A /review comment command implies this.
|
||||
--expect-head <sha> Review only when the PR's head is this full commit hash,
|
||||
for example \${{ github.event.pull_request.head.sha }} on
|
||||
a label event. Otherwise stop with a "not reviewed" note
|
||||
(logged only, with a routing.jsonc).
|
||||
-h, --help Show this help
|
||||
|
||||
Env: GITHUB_REPOSITORY, GITHUB_EVENT_PATH/GITHUB_REF (PR number), GH_TOKEN,
|
||||
@@ -152,9 +156,11 @@ export async function ciCommand(argv: string[] = []): Promise<void> {
|
||||
// the default .expo-code-review/. Applies to config.jsonc AND routing.jsonc.
|
||||
let configDir: string | undefined;
|
||||
let contextFile: string | undefined;
|
||||
let expectHead: string | undefined;
|
||||
try {
|
||||
configDir = parseValueFlag(argv, "--config-dir");
|
||||
contextFile = parseValueFlag(argv, "--context-file");
|
||||
expectHead = parseExpectHead(argv);
|
||||
} catch (error) {
|
||||
process.stderr.write(`${errorMessage(error)}\n\n${CI_USAGE}`);
|
||||
process.exitCode = 2;
|
||||
@@ -261,6 +267,7 @@ export async function ciCommand(argv: string[] = []): Promise<void> {
|
||||
configDir,
|
||||
contextText,
|
||||
noStackAware,
|
||||
expectHead,
|
||||
});
|
||||
return;
|
||||
}
|
||||
@@ -275,6 +282,7 @@ export async function ciCommand(argv: string[] = []): Promise<void> {
|
||||
configDir,
|
||||
contextText,
|
||||
noStackAware,
|
||||
expectHead,
|
||||
});
|
||||
} catch (error) {
|
||||
// Fan-out failures stay non-blocking (single-writer property is the point).
|
||||
@@ -485,6 +493,47 @@ interface CiRunOptions {
|
||||
contextText: string | undefined;
|
||||
/** --no-stack-aware: force stack-aware requalification off for this run. */
|
||||
noStackAware: boolean;
|
||||
/** --expect-head: the full head commit hash this run may review, or undefined. */
|
||||
expectHead?: string | undefined;
|
||||
}
|
||||
|
||||
// @ref LLP 0007#ecr-ci-the-trusted-root-run [implements] — --expect-head pins the review to the commit the request named
|
||||
/**
|
||||
* Why the run must stop when `--expect-head` names a commit that is not the PR's head
|
||||
* (unknown counts as different, so a metadata failure fails closed), else null. The
|
||||
* reason is safe to post.
|
||||
*/
|
||||
export function expectedHeadMismatch(
|
||||
expectHead: string | undefined,
|
||||
headOid: string | undefined,
|
||||
): string | null {
|
||||
if (expectHead === undefined || headOid === expectHead) {
|
||||
return null;
|
||||
}
|
||||
return (
|
||||
`the PR's head is ${headOid ?? "unknown"}, not ${expectHead}, ` +
|
||||
"the commit this review was requested for"
|
||||
);
|
||||
}
|
||||
|
||||
/** Post the "not reviewed" note for a head that is not the one this run may review. */
|
||||
async function postNotReviewed(reporter: GitHubReporter, reason: string): Promise<void> {
|
||||
process.stderr.write(`CI reviewer: not reviewing: ${reason}.\n`);
|
||||
try {
|
||||
await reporter.report({
|
||||
decision: "approve_with_comments",
|
||||
findings: [],
|
||||
summary: `⚠️ This change was **not** reviewed: ${reason}.`,
|
||||
incomplete: [],
|
||||
// Not a review of anything: keeps the inline sync additive-only and removes
|
||||
// the reviewer's ready label.
|
||||
couldNotComplete: true,
|
||||
});
|
||||
} catch (postError) {
|
||||
process.stderr.write(
|
||||
`CI reviewer: also failed to post the not-reviewed note: ${errorMessage(postError)}\n`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// @ref LLP 0010#config-and-cli-surface [implements] — under ci, stack-aware is gated by the trusted-base config; a stack walk failure only ever warns (source fails open), never a check failure
|
||||
@@ -604,6 +653,11 @@ async function runLegacyCi(
|
||||
if (await isNewCommitsEvent()) {
|
||||
await reporter.removeReadyLabel();
|
||||
}
|
||||
const headProblem = expectedHeadMismatch(options.expectHead, headSha);
|
||||
if (headProblem) {
|
||||
await postNotReviewed(reporter, headProblem);
|
||||
return;
|
||||
}
|
||||
|
||||
const stack = resolveStackWalk(config.stack, noStackAware);
|
||||
const stackConfirm = resolveStackConfirm(config.stack, noStackAware);
|
||||
@@ -818,6 +872,44 @@ async function runRoutedCi(
|
||||
return;
|
||||
}
|
||||
|
||||
// Break-glass: ONE check via a reporter on the root marker, before fan-out.
|
||||
const bgReporter = new GitHubReporter({
|
||||
prNumber,
|
||||
repo,
|
||||
commentTag: rootTag,
|
||||
breakGlassMarker: rootConfig.breakGlassMarker,
|
||||
cwd,
|
||||
readyForHumanLabel: rootConfig.review.readyForHumanLabel,
|
||||
});
|
||||
try {
|
||||
if (await bgReporter.checkBreakGlass()) {
|
||||
process.stderr.write(`CI reviewer: ${rootConfig.breakGlassMarker} detected; skipping.\n`);
|
||||
await bgReporter.postSkipNote();
|
||||
return;
|
||||
}
|
||||
} catch (error) {
|
||||
process.stderr.write(
|
||||
`CI reviewer: break-glass check failed (continuing): ${errorMessage(error)}\n`,
|
||||
);
|
||||
}
|
||||
// The ready label described the previous head; the review adds it back if this one
|
||||
// is ready.
|
||||
if (await isNewCommitsEvent()) {
|
||||
await bgReporter.removeReadyLabel();
|
||||
}
|
||||
|
||||
// In the routed path the root comment holds the aggregate and its per-scope state,
|
||||
// which a single "not reviewed" render would replace, so a head that is not the one
|
||||
// this run may review only logs, and the reviewer's ready label goes.
|
||||
const headProblem = expectedHeadMismatch(
|
||||
options.expectHead,
|
||||
(await source.getMetadata().catch(() => undefined))?.headOid,
|
||||
);
|
||||
if (headProblem) {
|
||||
process.stderr.write(`CI reviewer: not reviewing: ${headProblem}.\n`);
|
||||
await bgReporter.removeReadyLabel();
|
||||
return;
|
||||
}
|
||||
const changed = await source.getChangedFiles();
|
||||
const resolution = resolveScopes(
|
||||
manifest,
|
||||
@@ -852,32 +944,6 @@ async function runRoutedCi(
|
||||
|
||||
const mode = commentOverride ?? manifest.comment;
|
||||
|
||||
// Break-glass: ONE check via a reporter on the root marker, before fan-out.
|
||||
const bgReporter = new GitHubReporter({
|
||||
prNumber,
|
||||
repo,
|
||||
commentTag: rootTag,
|
||||
breakGlassMarker: rootConfig.breakGlassMarker,
|
||||
cwd,
|
||||
readyForHumanLabel: rootConfig.review.readyForHumanLabel,
|
||||
});
|
||||
try {
|
||||
if (await bgReporter.checkBreakGlass()) {
|
||||
process.stderr.write(`CI reviewer: ${rootConfig.breakGlassMarker} detected; skipping.\n`);
|
||||
await bgReporter.postSkipNote();
|
||||
return;
|
||||
}
|
||||
} catch (error) {
|
||||
process.stderr.write(
|
||||
`CI reviewer: break-glass check failed (continuing): ${errorMessage(error)}\n`,
|
||||
);
|
||||
}
|
||||
// The ready label described the previous head; the review adds it back if this one
|
||||
// is ready.
|
||||
if (await isNewCommitsEvent()) {
|
||||
await bgReporter.removeReadyLabel();
|
||||
}
|
||||
|
||||
// Build ONE link context for all scopes (rate-limit hygiene): diff lines from the
|
||||
// already-fetched changed files, base OID from the memoized PR metadata (the same
|
||||
// immutable OID the trusted config root was materialized from).
|
||||
@@ -1384,6 +1450,33 @@ function parseListFlag(argv: string[], flag: string): string[] | undefined {
|
||||
.filter(Boolean);
|
||||
}
|
||||
|
||||
/**
|
||||
* Parse `--expect-head <sha>`: a full commit hash, lowercased, or undefined. Every
|
||||
* malformed form throws instead of being ignored, because an ignored flag would
|
||||
* review an unpinned head without a word.
|
||||
*/
|
||||
export function parseExpectHead(argv: string[]): string | undefined {
|
||||
if (argv.some((arg) => arg.startsWith("--expect-head="))) {
|
||||
throw new Error("--expect-head takes its value as a separate argument: --expect-head <sha>");
|
||||
}
|
||||
if (argv.filter((arg) => arg === "--expect-head").length > 1) {
|
||||
throw new Error("--expect-head was given more than once");
|
||||
}
|
||||
const value = parseValueFlag(argv, "--expect-head");
|
||||
if (value === undefined) {
|
||||
return undefined;
|
||||
}
|
||||
if (value === "") {
|
||||
throw new Error(
|
||||
"--expect-head is empty; the workflow likely passed a PR head from an event that has none",
|
||||
);
|
||||
}
|
||||
if (!isCommitOid(value)) {
|
||||
throw new Error(`--expect-head needs a full 40-character commit hash, got "${value}"`);
|
||||
}
|
||||
return value.toLowerCase();
|
||||
}
|
||||
|
||||
/** Parse a single-value flag (`--flag value`); undefined when absent, throws when
|
||||
* present without a value (matching review.ts's requireValue). */
|
||||
function parseValueFlag(argv: string[], flag: string): string | undefined {
|
||||
|
||||
+2
-2
@@ -314,7 +314,7 @@ function renderReviewBody(
|
||||
|
||||
const lines: string[] = [commentMarker(tag), "## 🤖 AI code review", ""];
|
||||
lines.push(
|
||||
`**Decision:** ${review.couldNotComplete ? "No review — every pass failed" : decisionLabel(review.decision)}`,
|
||||
`**Decision:** ${review.couldNotComplete ? "No review" : decisionLabel(review.decision)}`,
|
||||
"",
|
||||
modelText(review.summary),
|
||||
"",
|
||||
@@ -1002,7 +1002,7 @@ export function renderAggregateMarkdown(
|
||||
];
|
||||
for (const { result, kept } of perScope) {
|
||||
lines.push(
|
||||
`| ${stripStateMarkers(result.scope)} | ${result.review.couldNotComplete ? "No review — every pass failed" : decisionLabel(result.review.decision)} | ${kept.length} |`,
|
||||
`| ${stripStateMarkers(result.scope)} | ${result.review.couldNotComplete ? "No review" : decisionLabel(result.review.decision)} | ${kept.length} |`,
|
||||
);
|
||||
}
|
||||
lines.push("");
|
||||
|
||||
+2
-2
@@ -361,8 +361,8 @@ export const CoordinatorOutputSchema = CoordinatorModelOutputSchema.extend({
|
||||
*/
|
||||
incomplete: z.array(z.string()).default([]),
|
||||
/**
|
||||
* True when EVERY pass failed — nothing was actually reviewed. Set by the
|
||||
* engine, never the model. Reporters must not render an approving decision
|
||||
* True when nothing was actually reviewed: every pass failed, or the CI run
|
||||
* failed or stopped before any pass. Set by the engine, never the model. Reporters must not render an approving decision
|
||||
* label for such a run (the decision enum has no "no review" member, and
|
||||
* widening it would ripple through dismiss state and exit codes — this flag
|
||||
* overrides the presentation instead).
|
||||
|
||||
@@ -223,16 +223,31 @@ export class GitHubPRSource implements ReviewSource {
|
||||
};
|
||||
}
|
||||
|
||||
// @ref LLP 0008#pr-head-materialization [implements] — the diff is pinned to the same base and head OIDs as the trees
|
||||
/**
|
||||
* The PR's diff between the base and head OIDs that getMetadata() read, the same
|
||||
* commits the trusted config root and the head tree are checked out at. `gh pr diff`
|
||||
* takes no commit and returns the PR as it is now, so it is used only when an OID
|
||||
* is unknown.
|
||||
*/
|
||||
async getChangedFiles(): Promise<DiffEntry[]> {
|
||||
const metadata = await this.getMetadata().catch(() => undefined);
|
||||
const base = metadata?.baseOid;
|
||||
const head = metadata?.headOid;
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
// gh colors the diff even into a pipe when the environment forces it
|
||||
// (CLICOLOR_FORCE, GH_FORCE_TTY), and the escape codes before `diff --git` would
|
||||
// make the whole PR parse as no files.
|
||||
const { stdout } = await run(
|
||||
gh,
|
||||
["pr", "diff", String(this.options.prNumber), ...this.repoArgs(), "--color", "never"],
|
||||
{ cwd: this.options.cwd },
|
||||
);
|
||||
const args =
|
||||
isCommitOid(base) && isCommitOid(head)
|
||||
? [
|
||||
"api",
|
||||
`repos/${this.options.repo ?? "{owner}/{repo}"}/compare/${base}...${head}`,
|
||||
"-H",
|
||||
"Accept: application/vnd.github.diff",
|
||||
]
|
||||
: // gh colors the diff even into a pipe when the environment forces it
|
||||
// (CLICOLOR_FORCE, GH_FORCE_TTY), and the escape codes before `diff --git`
|
||||
// would make the whole PR parse as no files.
|
||||
["pr", "diff", String(this.options.prNumber), ...this.repoArgs(), "--color", "never"];
|
||||
const { stdout } = await run(gh, args, { cwd: this.options.cwd });
|
||||
return parseUnifiedDiff(stdout);
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user