mirror of
https://github.com/expo/code-review-cli.git
synced 2026-11-15 03:38:09 -10:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
15a9b0af7c |
No files matched your search
@@ -71,6 +71,9 @@ ecr review
|
||||
ecr review --pr 123
|
||||
# …and post it as the PR comment
|
||||
ecr review --pr 123 --post
|
||||
# Preview once, save the exact result, and post it later without another model run
|
||||
ecr review --repo owner/repo --pr 123 --save-review --json
|
||||
ecr post-review --artifact .expo-code-review/.runs/deferred/<artifact>.json --repo owner/repo --pr 123
|
||||
```
|
||||
|
||||
Options (most to least common):
|
||||
@@ -79,6 +82,7 @@ Options (most to least common):
|
||||
| --- | --- |
|
||||
| `--pr <n>` | Review GitHub PR #n by number (diff fetched via `gh`, no checkout); not combinable with `--base`/`--head`/`--staged`. |
|
||||
| `--post` | With `--pr`, also post the result as the PR comment (needs `gh` auth). Omit to preview only; re-run with `--post` to publish. |
|
||||
| `--save-review` | With explicit `--repo` + `--pr`, save the exact preview as a private postable artifact. Mutually exclusive with `--post`. |
|
||||
| `--staged` | Review only staged changes (index vs HEAD; not combinable with `--base`/`--head`). |
|
||||
| `--base <ref>` | Base ref to diff against (default: merge-base with the default branch). |
|
||||
| `--head <ref>` | Head ref to diff (default: working tree, incl. uncommitted changes). |
|
||||
@@ -111,6 +115,7 @@ is a ready example to adapt.
|
||||
| `ecr setup-auth [--yes]` | Walk through getting model credentials for local runs (ChatGPT/Claude sign-in and/or API keys), printing the `export` lines for your shell config. |
|
||||
| `ecr review [options]` | Review local changes and print an advisory review (default command). |
|
||||
| `ecr review --scope <name>` | Review only one routing scope over just that scope's changed files. |
|
||||
| `ecr post-review --artifact <path> --repo <owner/repo> --pr <n>` | Post an exact saved PR preview without re-running models; refuses target, head, config, or break-glass drift. |
|
||||
| `ecr ci` | Review the current GitHub PR and post/update a comment. For GitHub Actions. |
|
||||
| `ecr doctor [--list-scopes]` | Check environment, config, credentials, and (with a manifest) scopes. |
|
||||
| `ecr feedback [--repo <owner/repo>]` | Report which findings PR authors pushed back on, across history. See below. |
|
||||
@@ -661,6 +666,12 @@ that the PR comment embeds for machine consumers. The same totals are printed as
|
||||
one-line summary to the terminal / CI job log at the end of each run, so cache reuse
|
||||
is visible even in CI (where the run log is ephemeral).
|
||||
|
||||
`ecr review --save-review` additionally writes a versioned artifact under
|
||||
`.expo-code-review/.runs/deferred/` with owner-only permissions. It contains the
|
||||
verified final review and bounded feedback metadata, but no credential. `ecr
|
||||
post-review` schema-validates it and refuses to post if its explicit repo/PR, live
|
||||
head commit, or local comment-policy fingerprint no longer matches.
|
||||
|
||||
</details>
|
||||
|
||||
<a id="other-providers"></a>
|
||||
|
||||
@@ -35,6 +35,36 @@ Flag exclusivity is rejected outright, never silently resolved, because silently
|
||||
|
||||
`--scope --post` posts under the derived marker `<rootTag>:<scope>`, taken from the ROOT config's tag via `scopedCommentTag(rootConfig.commentTag, scope)` [observed: `src/commands/review.ts:230`]. This is not a local convenience — it must match `ecr ci`'s per-scope derivation exactly, so a standalone local scope post and CI's per-scope post/clear/reconcile paths always target the identical comment. The schema ban on scope-level `commentTag` overrides (LLP 0006) is the only thing keeping the two derivations from ever diverging; relaxing that schema rule would silently break marker consistency between local and CI posts.
|
||||
|
||||
## Deferred Review Posting
|
||||
|
||||
`ecr review --save-review --repo owner/repo --pr N` separates the expensive model
|
||||
run from the outward-facing GitHub write without changing the review between them.
|
||||
After terminal preview it writes the exact verified `CoordinatorOutput` plus bounded
|
||||
feedback records to an owner-only, versioned artifact under
|
||||
`.expo-code-review/.runs/deferred/`. The artifact contains no credential. Saving is
|
||||
PR-only, requires an explicit repo, and is mutually exclusive with immediate
|
||||
`--post`; scopes and alternate config directories are rejected in v1 rather than
|
||||
silently producing an artifact the post command cannot faithfully reconstruct
|
||||
[observed: `src/commands/review.ts` `validateReviewArgs`,
|
||||
`writeDeferredReviewArtifact`].
|
||||
|
||||
`ecr post-review --artifact P --repo owner/repo --pr N` performs no model call. It
|
||||
loads config from the local checkout (the terminal user remains the trust principal),
|
||||
strictly schema-validates and byte-caps the artifact, and requires four independent
|
||||
bindings before any GitHub write: artifact target equals the explicit argv repo/PR;
|
||||
the live PR head equals the reviewed 40-hex OID; the current comment tag,
|
||||
break-glass marker, and feedback policy hash to the stored posting-policy
|
||||
fingerprint; and the live break-glass check returns false. A break-glass API error
|
||||
fails closed on this deferred path. Only then does it pass the stored review through
|
||||
the ordinary `GitHubReporter`, preserving the single-comment lifecycle, dismissal
|
||||
state, hidden review trace, links, pins, and feedback merge [observed:
|
||||
`src/commands/post-review.ts`, `src/core/deferred-review.ts`].
|
||||
|
||||
The explicit repo/PR is deliberately repeated at post time. An artifact is data, not
|
||||
authority: even a locally replaced artifact cannot redirect a generic “post this”
|
||||
command to another PR. Head and policy drift require a fresh preview instead of
|
||||
publishing stale findings or rendering them under rules the model run never saw.
|
||||
|
||||
## ecr ci: The Trusted-Root Run
|
||||
|
||||
`ecr ci` runs inside GitHub Actions with credentials, reading content authored by whoever opened the PR (including a fork PR reachable through the not-fork-restricted `issue_comment` `/review` workflow). Its structure is dictated by that threat model (LLP 0001).
|
||||
|
||||
@@ -0,0 +1,234 @@
|
||||
import { mkdtemp, readFile, rm, stat, writeFile } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
import { afterEach, expect, test } from "bun:test";
|
||||
|
||||
import { parsePostReviewArgs, publishDeferredReview } from "../commands/post-review.js";
|
||||
import { parseReviewArgs, validateReviewArgs } from "../commands/review.js";
|
||||
import type { LoadedConfig } from "../config/schema.js";
|
||||
import {
|
||||
assertDeferredReviewCurrent,
|
||||
DEFERRED_REVIEW_ARTIFACT_MAX_BYTES,
|
||||
DeferredReviewArtifactSchema,
|
||||
readDeferredReviewArtifact,
|
||||
reviewPostingConfigFingerprint,
|
||||
writeDeferredReviewArtifact,
|
||||
type DeferredReviewArtifact,
|
||||
} from "../core/deferred-review.js";
|
||||
|
||||
const roots: string[] = [];
|
||||
afterEach(async () => {
|
||||
await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true })));
|
||||
});
|
||||
|
||||
async function tempRoot(): Promise<string> {
|
||||
const root = await mkdtemp(path.join(tmpdir(), "ecr-deferred-review-"));
|
||||
roots.push(root);
|
||||
return root;
|
||||
}
|
||||
|
||||
function config(configDir: string, overrides: Partial<LoadedConfig> = {}): LoadedConfig {
|
||||
return {
|
||||
configDir,
|
||||
commentTag: "expo-ai-code-reviewer",
|
||||
breakGlassMarker: "/skip-review",
|
||||
feedback: {
|
||||
mode: "annotate",
|
||||
dismiss: "never",
|
||||
match: "both",
|
||||
protectedCategories: ["security", "secrets"],
|
||||
maxAdjudications: 10,
|
||||
},
|
||||
...overrides,
|
||||
} as LoadedConfig;
|
||||
}
|
||||
|
||||
function artifact(overrides: Partial<DeferredReviewArtifact> = {}): DeferredReviewArtifact {
|
||||
return DeferredReviewArtifactSchema.parse({
|
||||
version: 1,
|
||||
createdAt: "2026-08-06T00:00:00.000Z",
|
||||
repo: "expo/expo",
|
||||
pr: 48557,
|
||||
headSha: "a".repeat(40),
|
||||
configFingerprint: "b".repeat(64),
|
||||
review: {
|
||||
decision: "approve",
|
||||
findings: [],
|
||||
summary: "No findings.",
|
||||
incomplete: [],
|
||||
reviewTrace: {
|
||||
version: 1,
|
||||
trust: "unverified-model-diagnostics",
|
||||
agents: { correctness: { checked: ["Traced the changed path."], uncertainties: [] } },
|
||||
},
|
||||
},
|
||||
...overrides,
|
||||
});
|
||||
}
|
||||
|
||||
test("writes and reads an owner-only exact review artifact", async () => {
|
||||
const root = await tempRoot();
|
||||
const loaded = config(path.join(root, ".expo-code-review"));
|
||||
const saved = await writeDeferredReviewArtifact(loaded, {
|
||||
repo: "expo/expo",
|
||||
pr: 48557,
|
||||
headSha: "a".repeat(40),
|
||||
review: artifact().review,
|
||||
});
|
||||
|
||||
expect(saved).toContain(path.join(".runs", "deferred", "expo-expo-pr-48557-"));
|
||||
expect((await stat(saved)).mode & 0o777).toBe(0o600);
|
||||
const parsed = await readDeferredReviewArtifact(saved);
|
||||
expect(parsed.review).toEqual(artifact().review);
|
||||
expect(parsed.configFingerprint).toBe(reviewPostingConfigFingerprint(loaded));
|
||||
expect(JSON.parse(await readFile(saved, "utf8"))).toEqual(parsed);
|
||||
});
|
||||
|
||||
test("strictly rejects malformed, oversized, and extended artifacts", async () => {
|
||||
const root = await tempRoot();
|
||||
const malformed = path.join(root, "malformed.json");
|
||||
await writeFile(malformed, "{nope", "utf8");
|
||||
await expect(readDeferredReviewArtifact(malformed)).rejects.toThrow("not valid JSON");
|
||||
|
||||
const oversized = path.join(root, "oversized.json");
|
||||
await writeFile(oversized, "x".repeat(DEFERRED_REVIEW_ARTIFACT_MAX_BYTES + 1), "utf8");
|
||||
await expect(readDeferredReviewArtifact(oversized)).rejects.toThrow("is over");
|
||||
|
||||
expect(() => DeferredReviewArtifactSchema.parse({ ...artifact(), injected: true })).toThrow();
|
||||
});
|
||||
|
||||
test("refuses to create an artifact that its bounded reader could not reopen", async () => {
|
||||
const root = await tempRoot();
|
||||
const loaded = config(path.join(root, ".expo-code-review"));
|
||||
await expect(
|
||||
writeDeferredReviewArtifact(loaded, {
|
||||
repo: "expo/expo",
|
||||
pr: 48557,
|
||||
headSha: "a".repeat(40),
|
||||
review: artifact({ review: { ...artifact().review, summary: "x".repeat(1_000_000) } }).review,
|
||||
}),
|
||||
).rejects.toThrow("would be");
|
||||
});
|
||||
|
||||
test("refuses target, head, and posting-policy drift independently", () => {
|
||||
const saved = artifact();
|
||||
const expected = {
|
||||
repo: saved.repo,
|
||||
pr: saved.pr,
|
||||
headSha: saved.headSha,
|
||||
configFingerprint: saved.configFingerprint,
|
||||
};
|
||||
expect(() => assertDeferredReviewCurrent(saved, expected)).not.toThrow();
|
||||
expect(() => assertDeferredReviewCurrent(saved, { ...expected, repo: "expo/eas-cli" })).toThrow(
|
||||
"explicitly requested",
|
||||
);
|
||||
expect(() =>
|
||||
assertDeferredReviewCurrent(saved, { ...expected, headSha: "c".repeat(40) }),
|
||||
).toThrow("head changed");
|
||||
expect(() =>
|
||||
assertDeferredReviewCurrent(saved, { ...expected, configFingerprint: "d".repeat(64) }),
|
||||
).toThrow("posting policy changed");
|
||||
});
|
||||
|
||||
test("posting config fingerprint covers every comment-policy input", () => {
|
||||
const root = "/repo/.expo-code-review";
|
||||
const base = config(root);
|
||||
const fingerprint = reviewPostingConfigFingerprint(base);
|
||||
expect(reviewPostingConfigFingerprint(config(root, { commentTag: "other" }))).not.toBe(
|
||||
fingerprint,
|
||||
);
|
||||
expect(reviewPostingConfigFingerprint(config(root, { breakGlassMarker: "/different" }))).not.toBe(
|
||||
fingerprint,
|
||||
);
|
||||
expect(
|
||||
reviewPostingConfigFingerprint(config(root, { feedback: { ...base.feedback, mode: "off" } })),
|
||||
).not.toBe(fingerprint);
|
||||
});
|
||||
|
||||
test("post-review argv requires an explicit artifact, repo, and safe PR", () => {
|
||||
expect(
|
||||
parsePostReviewArgs(["--artifact", "/tmp/review.json", "--repo", "expo/expo", "--pr", "48557"]),
|
||||
).toEqual({
|
||||
artifact: "/tmp/review.json",
|
||||
repo: "expo/expo",
|
||||
pr: 48557,
|
||||
help: false,
|
||||
});
|
||||
expect(() => parsePostReviewArgs(["--pr", "9007199254740992"])).toThrow("safe integer");
|
||||
expect(() => parsePostReviewArgs(["--wat"])).toThrow("Unknown argument");
|
||||
});
|
||||
|
||||
test("save-review is PR-only, explicit-target, and mutually exclusive with immediate post", () => {
|
||||
const valid = parseReviewArgs([
|
||||
"--repo",
|
||||
"expo/expo",
|
||||
"--pr",
|
||||
"48557",
|
||||
"--save-review",
|
||||
"--json",
|
||||
]);
|
||||
expect(() => validateReviewArgs(valid)).not.toThrow();
|
||||
expect(() => validateReviewArgs(parseReviewArgs(["--pr", "48557", "--save-review"]))).toThrow(
|
||||
"requires explicit --repo",
|
||||
);
|
||||
expect(() =>
|
||||
validateReviewArgs(
|
||||
parseReviewArgs(["--repo", "expo/expo", "--pr", "48557", "--save-review", "--post"]),
|
||||
),
|
||||
).toThrow("mutually exclusive");
|
||||
expect(() =>
|
||||
validateReviewArgs(
|
||||
parseReviewArgs(["--repo", "expo/expo", "--pr", "48557", "--save-review", "--scope", "api"]),
|
||||
),
|
||||
).toThrow("does not support --scope");
|
||||
});
|
||||
|
||||
test("deferred publisher checks break-glass before posting the exact review", async () => {
|
||||
const saved = artifact();
|
||||
const calls: string[] = [];
|
||||
let reportedArgs: unknown[] | undefined;
|
||||
const poster = {
|
||||
checkBreakGlass: async () => {
|
||||
calls.push("break-glass");
|
||||
return false;
|
||||
},
|
||||
report: async (...args: unknown[]) => {
|
||||
calls.push("report");
|
||||
reportedArgs = args;
|
||||
},
|
||||
};
|
||||
expect(
|
||||
await publishDeferredReview(poster, saved, async () => {
|
||||
calls.push("final-gate");
|
||||
}),
|
||||
).toBe("posted");
|
||||
expect(calls).toEqual(["break-glass", "final-gate", "report"]);
|
||||
expect(reportedArgs).toEqual([saved.review, saved.feedback]);
|
||||
|
||||
calls.length = 0;
|
||||
expect(await publishDeferredReview({ ...poster, checkBreakGlass: async () => true }, saved)).toBe(
|
||||
"break-glass",
|
||||
);
|
||||
expect(calls).toEqual([]);
|
||||
|
||||
await expect(
|
||||
publishDeferredReview(
|
||||
{
|
||||
...poster,
|
||||
checkBreakGlass: async () => {
|
||||
throw new Error("GitHub unavailable");
|
||||
},
|
||||
},
|
||||
saved,
|
||||
),
|
||||
).rejects.toThrow("GitHub unavailable");
|
||||
expect(calls).toEqual([]);
|
||||
|
||||
await expect(
|
||||
publishDeferredReview(poster, saved, async () => {
|
||||
throw new Error("head changed");
|
||||
}),
|
||||
).rejects.toThrow("head changed");
|
||||
expect(calls).toEqual(["break-glass"]);
|
||||
});
|
||||
@@ -5,6 +5,7 @@ import { dismissCommand } from "./commands/dismiss.js";
|
||||
import { doctorCommand } from "./commands/doctor.js";
|
||||
import { feedbackCommand } from "./commands/feedback.js";
|
||||
import { initCommand } from "./commands/init.js";
|
||||
import { postReviewCommand } from "./commands/post-review.js";
|
||||
import { refCheckCommand } from "./commands/ref-check.js";
|
||||
import { reviewCommand } from "./commands/review.js";
|
||||
import { setupAuthCommand } from "./commands/setup-auth.js";
|
||||
@@ -14,6 +15,7 @@ const USAGE = `expo-code-review (ecr) — config-driven AI code reviewer
|
||||
|
||||
Usage:
|
||||
ecr review [options] Review local changes (default). See \`ecr review --help\`.
|
||||
ecr post-review --artifact <path> --repo <owner/repo> --pr <n> Post an exact saved preview.
|
||||
ecr ci Review the current PR and post a comment (GitHub Actions).
|
||||
ecr dismiss --pr <n> <id...> Hide a finding on a PR (see \`ecr dismiss --help\`).
|
||||
ecr undismiss --pr <n> <id...> Restore a dismissed finding.
|
||||
@@ -48,6 +50,9 @@ async function main(): Promise<void> {
|
||||
case "review":
|
||||
await reviewCommand(rest);
|
||||
break;
|
||||
case "post-review":
|
||||
await postReviewCommand(rest);
|
||||
break;
|
||||
case "ci":
|
||||
await ciCommand(rest);
|
||||
break;
|
||||
|
||||
@@ -0,0 +1,182 @@
|
||||
// @ref LLP 0007#deferred-review-posting [implements] — no-model post of an exact, target-bound preview artifact
|
||||
import path from "node:path";
|
||||
|
||||
import { loadReviewConfig } from "../config/load.js";
|
||||
import {
|
||||
assertDeferredReviewCurrent,
|
||||
readDeferredReviewArtifact,
|
||||
reviewPostingConfigFingerprint,
|
||||
type DeferredReviewArtifact,
|
||||
} from "../core/deferred-review.js";
|
||||
import { repoRoot } from "../core/exec.js";
|
||||
import { errorMessage } from "../core/util.js";
|
||||
import { GitHubReporter } from "../reporters/github.js";
|
||||
import { GitHubPRSource, isCommitOid } from "../sources/github-pr.js";
|
||||
|
||||
const USAGE = `ecr post-review — post an exact saved PR review without re-running models
|
||||
|
||||
Usage:
|
||||
ecr post-review --artifact <path> --repo <owner/repo> --pr <n>
|
||||
|
||||
The artifact must come from \`ecr review --save-review --repo <owner/repo> --pr <n>\`.
|
||||
Before writing to GitHub, this command verifies the explicit repo/PR, the live PR
|
||||
head commit, the local posting policy, and the PR's break-glass marker.
|
||||
`;
|
||||
|
||||
export interface PostReviewArgs {
|
||||
artifact?: string;
|
||||
repo?: string;
|
||||
pr?: number;
|
||||
help: boolean;
|
||||
}
|
||||
|
||||
function requireValue(flag: string, value: string | undefined): string {
|
||||
if (value === undefined || value.startsWith("--")) {
|
||||
throw new Error(`${flag} requires a value`);
|
||||
}
|
||||
return value;
|
||||
}
|
||||
|
||||
export function parsePostReviewArgs(argv: string[]): PostReviewArgs {
|
||||
const args: PostReviewArgs = { help: false };
|
||||
for (let i = 0; i < argv.length; i++) {
|
||||
const arg = argv[i];
|
||||
switch (arg) {
|
||||
case "--artifact":
|
||||
args.artifact = requireValue(arg, argv[++i]);
|
||||
break;
|
||||
case "--repo":
|
||||
args.repo = requireValue(arg, argv[++i]);
|
||||
break;
|
||||
case "--pr": {
|
||||
const value = requireValue(arg, argv[++i]);
|
||||
const number = Number(value);
|
||||
if (!Number.isSafeInteger(number) || number <= 0) {
|
||||
throw new Error(`--pr requires a positive safe integer (got "${value}")`);
|
||||
}
|
||||
args.pr = number;
|
||||
break;
|
||||
}
|
||||
case "-h":
|
||||
case "--help":
|
||||
args.help = true;
|
||||
break;
|
||||
default:
|
||||
throw new Error(`Unknown argument: ${arg}`);
|
||||
}
|
||||
}
|
||||
return args;
|
||||
}
|
||||
|
||||
function validatePostReviewArgs(
|
||||
args: PostReviewArgs,
|
||||
): asserts args is PostReviewArgs & { artifact: string; repo: string; pr: number } {
|
||||
if (!args.artifact || !args.repo || args.pr == null) {
|
||||
throw new Error("--artifact, --repo, and --pr are all required");
|
||||
}
|
||||
if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(args.repo)) {
|
||||
throw new Error(`--repo requires owner/repo (got "${args.repo}")`);
|
||||
}
|
||||
}
|
||||
|
||||
interface DeferredReviewPoster {
|
||||
checkBreakGlass(): Promise<boolean>;
|
||||
report(
|
||||
review: DeferredReviewArtifact["review"],
|
||||
feedback?: DeferredReviewArtifact["feedback"],
|
||||
): Promise<void>;
|
||||
}
|
||||
|
||||
/** Break-glass errors fail closed: no report call occurs unless the check says false. */
|
||||
export async function publishDeferredReview(
|
||||
poster: DeferredReviewPoster,
|
||||
artifact: DeferredReviewArtifact,
|
||||
assertReadyToPost: () => Promise<void> = async () => {},
|
||||
): Promise<"posted" | "break-glass"> {
|
||||
if (await poster.checkBreakGlass()) {
|
||||
return "break-glass";
|
||||
}
|
||||
await assertReadyToPost();
|
||||
await poster.report(artifact.review, artifact.feedback);
|
||||
return "posted";
|
||||
}
|
||||
|
||||
export async function postReviewCommand(argv: string[]): Promise<void> {
|
||||
let args: PostReviewArgs;
|
||||
try {
|
||||
args = parsePostReviewArgs(argv);
|
||||
if (args.help) {
|
||||
process.stdout.write(USAGE);
|
||||
return;
|
||||
}
|
||||
validatePostReviewArgs(args);
|
||||
} catch (error) {
|
||||
process.stderr.write(`${errorMessage(error)}\n\n${USAGE}`);
|
||||
process.exitCode = 2;
|
||||
return;
|
||||
}
|
||||
|
||||
// Resolve before chdir so a relative artifact path keeps the caller's meaning.
|
||||
const artifactPath = path.resolve(process.cwd(), args.artifact);
|
||||
try {
|
||||
const root = await repoRoot();
|
||||
if (root && root !== process.cwd()) {
|
||||
process.chdir(root);
|
||||
}
|
||||
const cwd = process.cwd();
|
||||
const [artifact, config] = await Promise.all([
|
||||
readDeferredReviewArtifact(artifactPath),
|
||||
loadReviewConfig(cwd),
|
||||
]);
|
||||
const source = new GitHubPRSource({ prNumber: args.pr, repo: args.repo, cwd });
|
||||
const headSha = (await source.getMetadata()).headOid;
|
||||
if (!isCommitOid(headSha)) {
|
||||
throw new Error(`could not resolve a full head commit for ${args.repo}#${args.pr}`);
|
||||
}
|
||||
assertDeferredReviewCurrent(artifact, {
|
||||
repo: args.repo,
|
||||
pr: args.pr,
|
||||
headSha,
|
||||
configFingerprint: reviewPostingConfigFingerprint(config),
|
||||
});
|
||||
|
||||
const reporter = new GitHubReporter({
|
||||
prNumber: args.pr,
|
||||
repo: args.repo,
|
||||
commentTag: config.commentTag,
|
||||
breakGlassMarker: config.breakGlassMarker,
|
||||
cwd,
|
||||
feedback: config.feedback,
|
||||
headSha,
|
||||
});
|
||||
const result = await publishDeferredReview(reporter, artifact, async () => {
|
||||
// Re-fetch rather than reusing GitHubPRSource's memoized metadata, and reload
|
||||
// config after the break-glass API call. This narrows the unavoidable remote
|
||||
// TOCTOU window and prevents a head/policy change during setup from reaching
|
||||
// the reporter.
|
||||
const [latestMetadata, latestConfig] = await Promise.all([
|
||||
new GitHubPRSource({ prNumber: args.pr, repo: args.repo, cwd }).getMetadata(),
|
||||
loadReviewConfig(cwd),
|
||||
]);
|
||||
if (!isCommitOid(latestMetadata.headOid)) {
|
||||
throw new Error(`could not re-resolve a full head commit for ${args.repo}#${args.pr}`);
|
||||
}
|
||||
assertDeferredReviewCurrent(artifact, {
|
||||
repo: args.repo,
|
||||
pr: args.pr,
|
||||
headSha: latestMetadata.headOid,
|
||||
configFingerprint: reviewPostingConfigFingerprint(latestConfig),
|
||||
});
|
||||
});
|
||||
if (result === "break-glass") {
|
||||
process.stderr.write(
|
||||
`Not posting: ${config.breakGlassMarker} is set on ${args.repo}#${args.pr}.\n`,
|
||||
);
|
||||
return;
|
||||
}
|
||||
process.stderr.write(`Posted saved review to ${args.repo}#${args.pr}.\n`);
|
||||
} catch (error) {
|
||||
process.stderr.write(`Saved review was not posted: ${errorMessage(error)}\n`);
|
||||
process.exitCode = 2;
|
||||
}
|
||||
}
|
||||
+54
-15
@@ -4,10 +4,11 @@ import { loadRoutingManifest, resolveScopes, scopedCommentTag } from "../config/
|
||||
import { repoRoot, resolveRepo } from "../core/exec.js";
|
||||
import { errorMessage } from "../core/util.js";
|
||||
import { readContextFile } from "../core/context-file.js";
|
||||
import { writeDeferredReviewArtifact } from "../core/deferred-review.js";
|
||||
import { feedbackNeedsRunSeam } from "../core/adjudicate.js";
|
||||
import { runReview } from "../core/review.js";
|
||||
import { LocalGitSource } from "../sources/local-git.js";
|
||||
import { GitHubPRSource } from "../sources/github-pr.js";
|
||||
import { GitHubPRSource, isCommitOid } from "../sources/github-pr.js";
|
||||
import type { ReviewSource } from "../sources/source.js";
|
||||
import { memoizeSource, stackConfirmFromConfig, stackWalkFromConfig } from "../sources/source.js";
|
||||
import { TerminalReporter } from "../reporters/terminal.js";
|
||||
@@ -17,7 +18,8 @@ const USAGE = `ecr review — AI code review, printed to your terminal
|
||||
|
||||
Usage:
|
||||
ecr review [options] review local changes
|
||||
ecr review --pr <n> [--post] review a GitHub PR by number
|
||||
ecr review --pr <n> [--post | --save-review]
|
||||
review a GitHub PR by number
|
||||
|
||||
Source (pick one):
|
||||
(default) diff the working tree against the merge-base
|
||||
@@ -33,6 +35,8 @@ Options:
|
||||
--post with --pr: also post the result as the PR comment (needs
|
||||
\`gh\` auth). Omit to only preview here; re-run with --post
|
||||
to publish.
|
||||
--save-review with explicit --repo + --pr: save this exact preview as a
|
||||
postable artifact for a later \`ecr post-review\` command.
|
||||
--agents <a,b> run only these agents (comma-separated ids); default: all
|
||||
--route let the router pick relevant agents from the diff
|
||||
--scope <name> review only this routing scope (needs a routing.jsonc);
|
||||
@@ -57,13 +61,14 @@ principal here. In \`ecr ci\`, config loads from the PR's trusted base commit.
|
||||
Exit codes: 0 approve / approve-with-comments, 1 request-changes, 2 error.
|
||||
`;
|
||||
|
||||
interface ReviewArgs {
|
||||
export interface ReviewArgs {
|
||||
base?: string;
|
||||
head?: string;
|
||||
staged: boolean;
|
||||
pr?: number;
|
||||
repo?: string;
|
||||
post: boolean;
|
||||
saveReview: boolean;
|
||||
agents?: string[];
|
||||
route: boolean;
|
||||
scope?: string;
|
||||
@@ -82,10 +87,11 @@ function requireValue(flag: string, value: string | undefined): string {
|
||||
return value;
|
||||
}
|
||||
|
||||
function parseArgs(argv: string[]): ReviewArgs {
|
||||
export function parseReviewArgs(argv: string[]): ReviewArgs {
|
||||
const args: ReviewArgs = {
|
||||
staged: false,
|
||||
post: false,
|
||||
saveReview: false,
|
||||
route: false,
|
||||
stackAware: false,
|
||||
json: false,
|
||||
@@ -107,8 +113,8 @@ function parseArgs(argv: string[]): ReviewArgs {
|
||||
case "--pr": {
|
||||
const value = requireValue(arg, argv[++i]);
|
||||
const number = Number(value);
|
||||
if (!Number.isInteger(number) || number <= 0) {
|
||||
throw new Error(`--pr requires a positive PR number (got "${value}")`);
|
||||
if (!Number.isSafeInteger(number) || number <= 0) {
|
||||
throw new Error(`--pr requires a positive safe integer (got "${value}")`);
|
||||
}
|
||||
args.pr = number;
|
||||
break;
|
||||
@@ -119,6 +125,9 @@ function parseArgs(argv: string[]): ReviewArgs {
|
||||
case "--post":
|
||||
args.post = true;
|
||||
break;
|
||||
case "--save-review":
|
||||
args.saveReview = true;
|
||||
break;
|
||||
case "--agents":
|
||||
args.agents = requireValue(arg, argv[++i])
|
||||
.split(",")
|
||||
@@ -160,7 +169,7 @@ function parseArgs(argv: string[]): ReviewArgs {
|
||||
export async function reviewCommand(argv: string[]): Promise<void> {
|
||||
let args: ReviewArgs;
|
||||
try {
|
||||
args = parseArgs(argv);
|
||||
args = parseReviewArgs(argv);
|
||||
} catch (error) {
|
||||
process.stderr.write(`${errorMessage(error)}\n\n${USAGE}`);
|
||||
process.exitCode = 2;
|
||||
@@ -173,7 +182,7 @@ export async function reviewCommand(argv: string[]): Promise<void> {
|
||||
}
|
||||
|
||||
try {
|
||||
validateArgs(args);
|
||||
validateReviewArgs(args);
|
||||
} catch (error) {
|
||||
process.stderr.write(`${errorMessage(error)}\n\n${USAGE}`);
|
||||
process.exitCode = 2;
|
||||
@@ -333,11 +342,16 @@ export async function reviewCommand(argv: string[]): Promise<void> {
|
||||
const config = await loadReviewConfig(cwd, { configDir: args.configDir });
|
||||
const source = makeSource();
|
||||
|
||||
// Build the PR reporter up front when posting, so adjudicate mode can judge the
|
||||
// Build the PR reporter up front when posting OR saving a postable preview, so
|
||||
// adjudicate mode can judge the
|
||||
// PR's replies against the source before the result is rendered. Feedback only
|
||||
// has replies to match when reviewing a PR and posting (the terminal preview never
|
||||
// renders annotations), so it is wired only on the --post --pr path.
|
||||
const { repo: postRepo, error: postRepoError } = await resolvePostRepo(args, cwd);
|
||||
// renders annotations), so it is wired only when a later/current post is possible.
|
||||
const { repo: postRepo, error: postRepoError } = await resolvePostRepo(
|
||||
{ ...args, post: args.post || args.saveReview },
|
||||
cwd,
|
||||
);
|
||||
const headSha = postRepo != null && args.pr != null ? await reviewedHeadSha(source) : undefined;
|
||||
const reporter =
|
||||
postRepo != null && args.pr != null
|
||||
? new GitHubReporter({
|
||||
@@ -347,7 +361,7 @@ export async function reviewCommand(argv: string[]): Promise<void> {
|
||||
breakGlassMarker: config.breakGlassMarker,
|
||||
cwd,
|
||||
feedback: config.feedback,
|
||||
headSha: await reviewedHeadSha(source),
|
||||
headSha,
|
||||
})
|
||||
: null;
|
||||
|
||||
@@ -372,6 +386,22 @@ export async function reviewCommand(argv: string[]): Promise<void> {
|
||||
// Always print the result here first.
|
||||
await new TerminalReporter({ json: args.json, noFail: args.noFail }).report(review);
|
||||
|
||||
if (args.saveReview) {
|
||||
if (postRepo == null || args.pr == null || !isCommitOid(headSha)) {
|
||||
throw new Error(
|
||||
`could not save a postable review: ${postRepoError ? errorMessage(postRepoError) : "the PR head commit could not be resolved"}`,
|
||||
);
|
||||
}
|
||||
const artifactPath = await writeDeferredReviewArtifact(config, {
|
||||
repo: postRepo,
|
||||
pr: args.pr,
|
||||
headSha,
|
||||
review,
|
||||
feedback: review.feedback,
|
||||
});
|
||||
process.stderr.write(`\nSaved postable review artifact: ${artifactPath}\n`);
|
||||
}
|
||||
|
||||
// Then, only if asked, publish the same result to the PR.
|
||||
if (args.pr != null && args.post) {
|
||||
if (reporter) {
|
||||
@@ -460,14 +490,23 @@ export async function resolvePostRepo(
|
||||
|
||||
// @ref LLP 0007#ecr-review-local-trust-and-flag-rules [implements] — mutually exclusive flags rejected outright, never silently ignored
|
||||
/** Reject flag combinations that don't make sense together. */
|
||||
function validateArgs(args: ReviewArgs): void {
|
||||
export function validateReviewArgs(args: ReviewArgs): void {
|
||||
if (args.pr != null && (args.base || args.head || args.staged)) {
|
||||
throw new Error(
|
||||
"--pr reviews a PR by its diff and cannot be combined with --base/--head/--staged.",
|
||||
);
|
||||
}
|
||||
if (args.pr == null && (args.repo || args.post)) {
|
||||
throw new Error("--repo/--post only apply together with --pr.");
|
||||
if (args.pr == null && (args.repo || args.post || args.saveReview)) {
|
||||
throw new Error("--repo/--post/--save-review only apply together with --pr.");
|
||||
}
|
||||
if (args.saveReview && !args.repo) {
|
||||
throw new Error("--save-review requires explicit --repo owner/repo.");
|
||||
}
|
||||
if (args.saveReview && args.post) {
|
||||
throw new Error("--save-review and --post are mutually exclusive.");
|
||||
}
|
||||
if (args.saveReview && (args.scope || args.configDir)) {
|
||||
throw new Error("--save-review does not support --scope or --config-dir.");
|
||||
}
|
||||
// Same rule as --repo/--post: the stack walk needs a PR to walk from, so a bare
|
||||
// --stack-aware would be silently ignored — reject it instead.
|
||||
|
||||
@@ -0,0 +1,163 @@
|
||||
// @ref LLP 0007#deferred-review-posting [implements] — exact preview artifact, explicit target binding, and stale-head/config refusal
|
||||
import { createHash, randomUUID } from "node:crypto";
|
||||
import { mkdir, open, writeFile } from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
|
||||
import { z } from "zod";
|
||||
|
||||
import type { LoadedConfig } from "../config/schema.js";
|
||||
import {
|
||||
CoordinatorOutputSchema,
|
||||
FeedbackRecordSchema,
|
||||
type CoordinatorOutput,
|
||||
type FeedbackRecord,
|
||||
} from "./schema.js";
|
||||
|
||||
export const DEFERRED_REVIEW_ARTIFACT_VERSION = 1;
|
||||
export const DEFERRED_REVIEW_ARTIFACT_MAX_BYTES = 1_000_000;
|
||||
const READ_CHUNK_BYTES = 65_536;
|
||||
|
||||
const RepoSchema = z.string().regex(/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/, "expected owner/repo");
|
||||
const CommitOidSchema = z.string().regex(/^[0-9a-f]{40}$/i, "expected a full commit OID");
|
||||
|
||||
/**
|
||||
* A postable review produced by one completed local PR review. The review and
|
||||
* feedback are the exact verified values that terminal preview rendered; target,
|
||||
* head and posting-policy bindings are checked again before any GitHub write.
|
||||
*/
|
||||
export const DeferredReviewArtifactSchema = z
|
||||
.object({
|
||||
version: z.literal(DEFERRED_REVIEW_ARTIFACT_VERSION),
|
||||
createdAt: z.string().datetime(),
|
||||
repo: RepoSchema,
|
||||
pr: z.number().int().positive().safe(),
|
||||
headSha: CommitOidSchema,
|
||||
configFingerprint: z.string().regex(/^[0-9a-f]{64}$/i),
|
||||
review: CoordinatorOutputSchema,
|
||||
feedback: z.array(FeedbackRecordSchema).optional(),
|
||||
})
|
||||
.strict();
|
||||
|
||||
export type DeferredReviewArtifact = z.infer<typeof DeferredReviewArtifactSchema>;
|
||||
|
||||
export interface DeferredReviewInput {
|
||||
repo: string;
|
||||
pr: number;
|
||||
headSha: string;
|
||||
review: CoordinatorOutput;
|
||||
feedback?: FeedbackRecord[];
|
||||
}
|
||||
|
||||
/** Bind the artifact to the local config fields that affect the posted comment. */
|
||||
export function reviewPostingConfigFingerprint(config: LoadedConfig): string {
|
||||
return createHash("sha256")
|
||||
.update(
|
||||
JSON.stringify({
|
||||
commentTag: config.commentTag,
|
||||
breakGlassMarker: config.breakGlassMarker,
|
||||
feedback: config.feedback,
|
||||
}),
|
||||
)
|
||||
.digest("hex");
|
||||
}
|
||||
|
||||
function artifactFilename(repo: string, pr: number): string {
|
||||
const safeRepo = repo.replace(/[^A-Za-z0-9_.-]+/g, "-");
|
||||
return `${safeRepo}-pr-${pr}-${randomUUID()}.json`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Persist with owner-only permissions and exclusive creation. The random filename
|
||||
* avoids overwriting another session's pending review; no credential is stored.
|
||||
*/
|
||||
export async function writeDeferredReviewArtifact(
|
||||
config: LoadedConfig,
|
||||
input: DeferredReviewInput,
|
||||
): Promise<string> {
|
||||
const artifact = DeferredReviewArtifactSchema.parse({
|
||||
version: DEFERRED_REVIEW_ARTIFACT_VERSION,
|
||||
createdAt: new Date().toISOString(),
|
||||
repo: input.repo,
|
||||
pr: input.pr,
|
||||
headSha: input.headSha,
|
||||
configFingerprint: reviewPostingConfigFingerprint(config),
|
||||
review: input.review,
|
||||
...(input.feedback ? { feedback: input.feedback } : {}),
|
||||
});
|
||||
const serialized = `${JSON.stringify(artifact, null, 2)}\n`;
|
||||
const serializedBytes = Buffer.byteLength(serialized);
|
||||
if (serializedBytes > DEFERRED_REVIEW_ARTIFACT_MAX_BYTES) {
|
||||
throw new Error(
|
||||
`deferred review artifact would be ${serializedBytes} bytes; maximum is ${DEFERRED_REVIEW_ARTIFACT_MAX_BYTES}`,
|
||||
);
|
||||
}
|
||||
const dir = path.join(config.configDir, ".runs", "deferred");
|
||||
await mkdir(dir, { recursive: true });
|
||||
const artifactPath = path.join(dir, artifactFilename(input.repo, input.pr));
|
||||
await writeFile(artifactPath, serialized, {
|
||||
encoding: "utf8",
|
||||
flag: "wx",
|
||||
mode: 0o600,
|
||||
});
|
||||
return artifactPath;
|
||||
}
|
||||
|
||||
/** Read once, byte-cap before parsing, then cross the strict schema boundary. */
|
||||
export async function readDeferredReviewArtifact(
|
||||
artifactPath: string,
|
||||
): Promise<DeferredReviewArtifact> {
|
||||
const handle = await open(artifactPath, "r");
|
||||
const chunks: Buffer[] = [];
|
||||
let total = 0;
|
||||
try {
|
||||
for (;;) {
|
||||
const chunk = Buffer.alloc(READ_CHUNK_BYTES);
|
||||
const { bytesRead } = await handle.read(chunk, 0, READ_CHUNK_BYTES);
|
||||
if (bytesRead === 0) {
|
||||
break;
|
||||
}
|
||||
total += bytesRead;
|
||||
if (total > DEFERRED_REVIEW_ARTIFACT_MAX_BYTES) {
|
||||
throw new Error(
|
||||
`deferred review artifact is over ${DEFERRED_REVIEW_ARTIFACT_MAX_BYTES} bytes`,
|
||||
);
|
||||
}
|
||||
chunks.push(chunk.subarray(0, bytesRead));
|
||||
}
|
||||
} finally {
|
||||
await handle.close();
|
||||
}
|
||||
const raw = Buffer.concat(chunks);
|
||||
let parsed: unknown;
|
||||
try {
|
||||
parsed = JSON.parse(raw.toString("utf8"));
|
||||
} catch (error) {
|
||||
throw new Error(`deferred review artifact is not valid JSON: ${String(error)}`);
|
||||
}
|
||||
return DeferredReviewArtifactSchema.parse(parsed);
|
||||
}
|
||||
|
||||
/**
|
||||
* Final no-write gate. Every value comes from a separate authority: repo/PR from
|
||||
* explicit argv, head from live GitHub, config from the local trusted checkout.
|
||||
*/
|
||||
export function assertDeferredReviewCurrent(
|
||||
artifact: DeferredReviewArtifact,
|
||||
expected: { repo: string; pr: number; headSha: string; configFingerprint: string },
|
||||
): void {
|
||||
if (artifact.repo !== expected.repo || artifact.pr !== expected.pr) {
|
||||
throw new Error(
|
||||
`artifact targets ${artifact.repo}#${artifact.pr}, not explicitly requested ${expected.repo}#${expected.pr}`,
|
||||
);
|
||||
}
|
||||
if (artifact.headSha !== expected.headSha) {
|
||||
throw new Error(
|
||||
`PR head changed after preview (${artifact.headSha} → ${expected.headSha}); run a fresh review instead of posting stale findings`,
|
||||
);
|
||||
}
|
||||
if (artifact.configFingerprint !== expected.configFingerprint) {
|
||||
throw new Error(
|
||||
"local review posting policy changed after preview; run a fresh review before posting",
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user