mirror of
https://github.com/expo/code-review-cli.git
synced 2026-11-14 09:58:11 -10:00
Compare commits
3
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
a8b7037ca9 | ||
|
|
763fc70194 | ||
|
|
0365a97020 |
No files matched your search
@@ -732,8 +732,13 @@ Behavior notes:
|
||||
- **Replies on inline threads count as author feedback.** Replying in a thread
|
||||
matches the reply to that finding structurally; clearing still requires the
|
||||
`id:…` token in the replier's own words, same as main-comment replies.
|
||||
- **Notification fan-out.** The first enabled run on a PR sends one notification
|
||||
per created inline comment; `maxComments` bounds it.
|
||||
- **One review per run.** A run's new inline comments go out in one `COMMENT`
|
||||
review, so each run puts one review entry and one notification on the PR (one
|
||||
per scope in per-scope comment mode), not one per comment. The reviewer never
|
||||
approves or requests changes. If GitHub refuses one of the comments, for
|
||||
example for a line it won't anchor, the comments are posted one by one
|
||||
instead, each as its own review. The review has no body of its own, so
|
||||
nothing is left on the PR once its threads are deleted.
|
||||
- **Disabling later** leaves existing threads up (the sync no longer runs); a
|
||||
comment-mode switch — or the reporter's clear path — stubs/removes them.
|
||||
|
||||
|
||||
@@ -77,8 +77,8 @@ path.
|
||||
|
||||
`parseUnifiedDiff` has three independent callers with different trust needs, so
|
||||
a parsing bug is not contained to the reviewer feed: the two sources
|
||||
(`src/sources/local-git.ts:97`, `src/sources/github-pr.ts:98`) turn raw diff
|
||||
into the review input, and the GitHub reporter (`src/reporters/github.ts:214`,
|
||||
(`src/sources/local-git.ts:118`, `src/sources/github-pr.ts:233`) turn raw diff
|
||||
into the review input, and the GitHub reporter (`src/reporters/github.ts:1094`,
|
||||
`buildDiffLineIndex`) re-parses to anchor inline PR comments to line numbers
|
||||
[observed] (grep of `parseUnifiedDiff` call sites). A path or hunk-offset bug
|
||||
would corrupt comment placement, not just review content.
|
||||
|
||||
@@ -47,17 +47,19 @@ The base OID is fetchable by SHA even though the base ref may have moved since t
|
||||
|
||||
## Local Git Source
|
||||
|
||||
`LocalGitSource` has no network calls; it reads local git state and computes its own diff base [observed] ([local-git.ts:17-21](../src/sources/local-git.ts)). Three small decisions carry the weight.
|
||||
`LocalGitSource` has no network calls; it reads local git state and computes its own diff base [observed] ([local-git.ts:35-39](../src/sources/local-git.ts)). Four small decisions carry the weight.
|
||||
|
||||
**Default branch resolution** tries `origin/HEAD` via `symbolic-ref` first, then falls back through a fixed guess list (`origin/main`, `origin/master`, `main`, `master`), then `main`, because not every checkout has `origin/HEAD` set [observed] ([local-git.ts:31-50](../src/sources/local-git.ts)).
|
||||
**Default branch resolution** tries `origin/HEAD` via `symbolic-ref` first, then falls back through a fixed guess list (`origin/main`, `origin/master`, `main`, `master`), then `main`, because not every checkout has `origin/HEAD` set [observed] ([local-git.ts:49-68](../src/sources/local-git.ts)).
|
||||
|
||||
**Diff base is `merge-base(defaultBranch, HEAD)`, not the branch tip.** This scopes the review to the current branch's own changes and excludes commits merged into the default branch after the feature branch diverged — you review what you wrote, not what trunk moved on to [observed] ([local-git.ts:53-68](../src/sources/local-git.ts)).
|
||||
**Diff base is `merge-base(defaultBranch, HEAD)`, not the branch tip.** This scopes the review to the current branch's own changes and excludes commits merged into the default branch after the feature branch diverged — you review what you wrote, not what trunk moved on to [observed] ([local-git.ts:71-86](../src/sources/local-git.ts)).
|
||||
|
||||
**Untracked files are diffed without mutating the index.** Running a review must have no side effects on the user's working state, so untracked files are synthesized as add-diffs via `git diff --no-index -- /dev/null <file>` rather than being staged [observed] ([local-git.ts:101-103, 114](../src/sources/local-git.ts)). The listing uses `git ls-files -z` (NUL-terminated) so filenames containing newlines parse correctly [observed] ([local-git.ts:106-107](../src/sources/local-git.ts)), and the `--` separator before the filename stops a name beginning with `-` from being read as a git option — argument injection through an untracked filename [observed] ([local-git.ts:112, 114](../src/sources/local-git.ts)).
|
||||
**Untracked files are diffed without mutating the index.** Running a review must have no side effects on the user's working state, so untracked files are synthesized as add-diffs via `git diff --no-index -- /dev/null <file>` rather than being staged [observed] ([local-git.ts:122-124, 137](../src/sources/local-git.ts)). The listing uses `git ls-files -z` (NUL-terminated) so filenames containing newlines parse correctly [observed] ([local-git.ts:127-128](../src/sources/local-git.ts)), and the `--` separator before the filename stops a name beginning with `-` from being read as a git option — argument injection through an untracked filename [observed] ([local-git.ts:133, 137](../src/sources/local-git.ts)).
|
||||
|
||||
**Every parsed `git diff` overrides the user's diff config.** `git diff` follows the user's git config, and several settings change what it prints. An external diff tool (`diff.external`) replaces the unified diff entirely, forced color (`color.diff=always`) puts escape codes before `diff --git`, and `diff.noprefix` or `diff.mnemonicPrefix` drop or change the `a/` and `b/` prefixes that `parseUnifiedDiff` matches. Any of them made a local review parse no files and report "No reviewable changes". Two more change the content rather than break the parse: a textconv driver shows converted text instead of the file the reviewers and verifier read (or nothing, when both sides convert alike), and copy detection (`diff.renames=copies`) turns a copied file into a hunk-less `copy from`/`copy to` entry, so its contents go unreviewed. `diff.submodule` does both to a submodule change: `log` prints commit subjects that parse as nothing, and `diff` prints the submodule's own file diffs instead of the pointer change. `diff.ignoreSubmodules=all`, or `ignore = all` for a submodule in `.gitmodules`, drops the pointer change entirely. Each call therefore passes `--no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/ --no-textconv --find-renames --submodule=short --ignore-submodules=dirty` [observed]. `dirty` rather than git's default `none`: a submodule then shows only when its pointer moves, so edits or untracked files inside one never reach the reviewers as a `-dirty` pseudo-commit [observed] ([local-git.ts](../src/sources/local-git.ts) `PARSEABLE_DIFF_FLAGS`; tests `src/__tests__/local-git.test.ts`, which fail when any one flag is removed). Repository attributes (`binary`, `-diff`) still apply: in local mode the checkout is trusted (see [LLP 0001](./0001-trust-model.principles.md)), and a file they mark binary is dropped by noise filtering as before.
|
||||
|
||||
## GitHub Reporter Identity
|
||||
|
||||
The reporter maintains exactly one PR comment and is comment-only — it never touches the review-thread / GitHub "reviews" APIs, by explicit design [observed] ([github.ts:77-82](../src/reporters/github.ts)). The hard problem is: which comment on the PR is *ours*?
|
||||
The reporter maintains exactly one PR comment, plus optional inline threads (see [Inline Threads](#inline-threads)), and it never approves or requests changes: the only review it creates is a `COMMENT` review that carries a run's new inline threads [observed] ([github.ts](../src/reporters/github.ts) `GitHubReporter` doc comment). The hard problem is: which comment on the PR is *ours*?
|
||||
|
||||
**Author + marker, never marker alone.** The comment marker is a hardcoded public literal, readable in the base-branch config, so anyone who can comment on the PR (including the untrusted PR author) could post a comment carrying the marker plus a forged embedded review state. A newest-marker-wins lookup would adopt that state and carry its `dismissed` list forward, silently suppressing real findings. So a comment counts as ours only if it carries the marker AND was authored by `ownLogin` — GitHub sets a comment's author from the authenticated identity and it cannot be spoofed [observed] ([github.ts:51-75](../src/reporters/github.ts); commit 93aaf30: "identify our own comment by author + marker, not the public marker alone, so a forged comment can't carry review state forward"; regression tests `src/__tests__/github-reporter.test.ts:25-34`). Any rewrite that reintroduces marker-only matching reopens this forged-dismissal-state attack.
|
||||
|
||||
@@ -79,6 +81,16 @@ The reporter maintains exactly one PR comment and is comment-only — it never t
|
||||
|
||||
**Links fail soft.** `linkContextAsync` can be prebuilt and injected once so a CI fan-out builds one diff-lines + base-SHA context for all scopes instead of one pair of `gh` calls per scope — the reporter-side mirror of `memoizeSource` [observed] ([github.ts:33-37, 200-205](../src/reporters/github.ts)). Both underlying fetches fail soft: a missing diff index degrades in-diff findings to plain-text `file:line`, a missing base SHA degrades out-of-diff findings the same way — never a dead link, never a hard failure of the whole report [observed] ([github.ts:194-235](../src/reporters/github.ts); commits f9fecd5, 9446b42). This follows the repo rule that `ecr ci` must never fail a PR's checks on reviewer error [observed] (AGENTS.md: "`ecr ci` must never fail a PR's checks — reviewer errors degrade gracefully").
|
||||
|
||||
## Inline Threads
|
||||
|
||||
With `inline.enabled`, `syncInline` converges one inline thread per in-diff finding: `planInlineSync` decides which threads to create, patch, stub or delete, and the executor runs the plan [observed] ([github.ts](../src/reporters/github.ts) `planInlineSync`, `syncInline`).
|
||||
|
||||
**One review per run for new threads.** GitHub makes every `POST /pulls/{n}/comments` its own review, so posting threads one by one put one "reviewed" entry and one notification per finding on the PR. The executor instead posts all of a run's new threads in one `POST /pulls/{n}/reviews` with `event: "COMMENT"`, the reviewed head as `commit_id`, and an empty body [observed] ([github.ts](../src/reporters/github.ts) `createInlineReview`). GitHub's docs call the body required for a `COMMENT` review, but it accepts an empty or missing one when the review has comments [observed: probe against a private scratch PR, 2026-10-21]. A submitted review can't be deleted, so a body would stay on the PR after its threads are torn down; an empty one leaves nothing. In per-scope comment mode every scope's reporter syncs its own threads, so a run posts one review per scope with new threads [observed] (`reporterFor` in `src/commands/ci.ts` passes the root `inline` config to each scope). Patches, stubs and deletes stay per comment: they edit existing threads and create no review.
|
||||
|
||||
**The batch is all or nothing, and only a 422 a comment caused is retried.** GitHub rejects the whole review with a 422 when it refuses one comment, for example a line it will not anchor, and then creates nothing: a review with one valid and one unanchorable line came back 422 `Line could not be resolved` and created neither comment [observed: probe against a private scratch PR, 2026-10-21]. That error falls back to one `POST /pulls/{n}/comments` per thread, so only the refused comment is lost. Two 422s are about the account, not a comment, and one-by-one posts would hit them again: GitHub's spam throttle ("submitted too quickly") aborts the rest of the sync like a rate limit, for the review and for each one-by-one post, and a pending review the posting account already has on the PR (GitHub allows one, and refuses both POSTs with "one pending review per pull request" while it exists) fails the new threads with a message saying to submit or delete it, while stubs and deletes still run. Telling them apart needs the response body: `gh` prints only the top-level message to stderr (`gh: Unprocessable Entity (HTTP 422)`) and the body, with `errors[]`, to stdout [observed: probe against a private scratch PR, 2026-10-21; the spam throttle's text is from reports, not reproduced], so both POSTs run with `check: false` and `ghApiFailure` keeps the body's distinct messages, each capped, in the error. Any other failure, such as a 5xx or a dropped connection, may come after GitHub created the review, so a retry could post every thread twice: the executor counts those threads as failed and does not retry, the main comment renders the findings in full, and the next run creates whatever is still missing. A rate limit aborts the rest of the sync as before [observed] (`syncInline`, `isPendingReviewError`, `ghApiFailure`; tests `src/__tests__/inline-review.test.ts`).
|
||||
|
||||
**Links come from a second call.** The review response does not list its comments, so the executor reads `GET /pulls/{n}/reviews/{id}/comments` a page at a time, up to the same 30-page bound as every comment listing, and maps each comment to its finding by the anchored marker in its body (`inlineUrlsByFp`). When a page fails, the pages before it stay linked, the threads exist, and the main comment renders the rest in full for this run; the next run finds the threads through `ownInlineComments` and links them. A rate limit on that read aborts the stubs and deletes still to come [observed] (`syncInline`, `inlineReviewCommentPages`).
|
||||
|
||||
## Terminal Reporter
|
||||
|
||||
The terminal reporter writes the primary report to **stdout** (progress and logging go to stderr) specifically so `ecr review > out.txt` captures a clean report and shell redirection works as expected [observed] ([terminal.ts:45-47](../src/reporters/terminal.ts)). It maps `review.decision` to a process exit code via `decisionExitCode`, which is what makes it usable as a pre-push/pre-commit gate; `--json` disables color and `noFail` forces exit 0 [observed] ([terminal.ts:28-32, 38, 49](../src/reporters/terminal.ts)). Color is enabled only on a TTY and only when not `--json` [observed] ([terminal.ts:38](../src/reporters/terminal.ts): `!options.json && Boolean(process.stdout.isTTY)`) — naively coloring whenever output is not JSON, ignoring `isTTY`, would corrupt piped or redirected output with ANSI codes.
|
||||
|
||||
@@ -305,6 +305,22 @@ test("every `uses:` in templates/*.yml is pinned by 40-hex commit SHA with a ver
|
||||
expect(usesLines).toBeGreaterThanOrEqual(7);
|
||||
});
|
||||
|
||||
test("every templates/*.yml defaults ECR_VERSION to latest", async () => {
|
||||
// A range like `^0.2.3` resolves to 0.2.3 itself on 0.x, so a workflow that
|
||||
// defaulted to one would run an engine too old to read the comment state the
|
||||
// review workflows write.
|
||||
const files = (await readdir(TEMPLATES_DIR)).filter((f) => f.endsWith(".yml"));
|
||||
expect(files).toContain("dismiss.yml");
|
||||
for (const file of files) {
|
||||
const yml = await readFile(path.join(TEMPLATES_DIR, file), "utf8");
|
||||
const defaults = [...yml.matchAll(/^\s*ECR_VERSION: (.*)$/gm)].map((m) => m[1]);
|
||||
expect({ file, defaults }).toEqual({
|
||||
file,
|
||||
defaults: ["${{ vars.ECR_VERSION || 'latest' }}"],
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
// @ref LLP 0012#what-gets-scanned [constrained-by] — a scaffold must pass `ecr ref-check` on day one
|
||||
test("a freshly scaffolded repo passes ref-check", async () => {
|
||||
const root = await freshRepo();
|
||||
|
||||
@@ -0,0 +1,433 @@
|
||||
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 { resetTrustedToolCache } from "../core/exec.js";
|
||||
import { renderInlineCommentBody } from "../core/render.js";
|
||||
import type { LinkContext } from "../core/render.js";
|
||||
import type { Finding } from "../core/schema.js";
|
||||
import { GitHubReporter, ghApiFailure, inlineUrlsByFp } from "../reporters/github.js";
|
||||
|
||||
const TAG = "expo-ai-code-reviewer";
|
||||
const HEAD = "0123456789abcdef0123456789abcdef01234567";
|
||||
|
||||
const finding = (over: Partial<Finding> = {}): Finding => ({
|
||||
severity: "warning",
|
||||
category: "quality",
|
||||
file: "a.ts",
|
||||
line: 5,
|
||||
title: "Observer restarts for cached instances",
|
||||
rationale: "getInstances() restarts the push token observer for cached instances.",
|
||||
...over,
|
||||
});
|
||||
|
||||
test("inlineUrlsByFp: maps our marked comments to their permalinks", () => {
|
||||
const ours = renderInlineCommentBody(finding(), TAG, "aaaa11");
|
||||
const urls = inlineUrlsByFp(
|
||||
[
|
||||
{ id: 1, body: ours, html_url: "https://github.com/o/r/pull/7#discussion_r1" },
|
||||
{ id: 2, body: renderInlineCommentBody(finding(), "other-tag", "bbbb22"), html_url: "u2" },
|
||||
{ id: 3, body: "a human reply", html_url: "u3" },
|
||||
{ id: 4, body: renderInlineCommentBody(finding(), TAG, "cccc33") },
|
||||
],
|
||||
TAG,
|
||||
);
|
||||
expect([...urls]).toEqual([["aaaa11", "https://github.com/o/r/pull/7#discussion_r1"]]);
|
||||
});
|
||||
|
||||
// ---- the sync posts new threads through a fake `gh` on PATH ----
|
||||
|
||||
// A fake `gh` that logs each call and answers the endpoints the inline sync uses.
|
||||
// FAKE_GH_REVIEW_ERROR makes the review POST fail with that stderr and
|
||||
// FAKE_GH_REVIEW_BODY as the response body on stdout, FAKE_GH_REVIEW_NO_ID makes it
|
||||
// succeed with a response that has no id, and FAKE_GH_LIST_ERROR makes reading the
|
||||
// review's comments fail (only page FAKE_GH_LIST_ERROR_PAGE, when set), and
|
||||
// FAKE_GH_PATCH_ERROR makes editing an existing comment fail. FAKE_GH_COMMENT_ERROR
|
||||
// and FAKE_GH_COMMENT_BODY do for a single comment POST what the REVIEW pair does. The review's
|
||||
// comments are served from review-comments-<page>.json, and the PR's existing review
|
||||
// comments from pr-comments.json.
|
||||
const FAKE_GH = `#!/bin/sh
|
||||
printf '%s\\n' "$*" >> "$FAKE_GH_DIR/calls.log"
|
||||
eval "last=\\\${$#}"
|
||||
case "$*" in
|
||||
"pr view"*) printf '%s' "$FAKE_GH_HEAD" ;;
|
||||
*"/reviews/99/comments"*)
|
||||
page=$(printf '%s' "$*" | sed -n 's/.*page=\\([0-9]*\\).*/\\1/p')
|
||||
if [ -n "$FAKE_GH_LIST_ERROR" ] && [ "\${FAKE_GH_LIST_ERROR_PAGE:-$page}" = "$page" ]; then
|
||||
echo "$FAKE_GH_LIST_ERROR" >&2; exit 1
|
||||
fi
|
||||
if [ -f "$FAKE_GH_DIR/review-comments-$page.json" ]; then
|
||||
cat "$FAKE_GH_DIR/review-comments-$page.json"
|
||||
else
|
||||
printf '[]'
|
||||
fi ;;
|
||||
*"-X POST "*"/reviews --input "*)
|
||||
cp "$last" "$FAKE_GH_DIR/review.json"
|
||||
if [ -n "$FAKE_GH_REVIEW_ERROR" ]; then
|
||||
printf '%s' "$FAKE_GH_REVIEW_BODY"; echo "$FAKE_GH_REVIEW_ERROR" >&2; exit 1
|
||||
fi
|
||||
if [ -n "$FAKE_GH_REVIEW_NO_ID" ]; then printf '{}'; exit 0; fi
|
||||
printf '{"id": 99}' ;;
|
||||
*"-X POST "*"/comments --input "*)
|
||||
if [ -n "$FAKE_GH_COMMENT_ERROR" ]; then
|
||||
printf '%s' "$FAKE_GH_COMMENT_BODY"; echo "$FAKE_GH_COMMENT_ERROR" >&2; exit 1
|
||||
fi
|
||||
n=$(wc -l < "$FAKE_GH_DIR/calls.log" | tr -d ' ')
|
||||
printf '{"html_url": "https://github.com/o/r/pull/7#discussion_r%s"}' "$n" ;;
|
||||
*"-X GET "*"/comments"*)
|
||||
if [ -f "$FAKE_GH_DIR/pr-comments.json" ]; then cat "$FAKE_GH_DIR/pr-comments.json"; else printf '[]'; fi ;;
|
||||
*"-X PATCH "*)
|
||||
if [ -n "$FAKE_GH_PATCH_ERROR" ]; then echo "$FAKE_GH_PATCH_ERROR" >&2; exit 1; fi
|
||||
printf '{}' ;;
|
||||
*"-X DELETE "*) ;;
|
||||
*) 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-inline-review-"));
|
||||
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_HEAD",
|
||||
"FAKE_GH_REVIEW_ERROR",
|
||||
"FAKE_GH_REVIEW_BODY",
|
||||
"FAKE_GH_REVIEW_NO_ID",
|
||||
"FAKE_GH_LIST_ERROR",
|
||||
"FAKE_GH_LIST_ERROR_PAGE",
|
||||
"FAKE_GH_PATCH_ERROR",
|
||||
"FAKE_GH_COMMENT_ERROR",
|
||||
"FAKE_GH_COMMENT_BODY",
|
||||
"GITHUB_STEP_SUMMARY",
|
||||
]) {
|
||||
saved[name] = process.env[name];
|
||||
}
|
||||
process.env.PATH = `${dir}${delimiter}${saved.PATH ?? ""}`;
|
||||
process.env.FAKE_GH_DIR = dir;
|
||||
process.env.FAKE_GH_HEAD = HEAD;
|
||||
delete process.env.FAKE_GH_REVIEW_ERROR;
|
||||
delete process.env.FAKE_GH_REVIEW_BODY;
|
||||
delete process.env.FAKE_GH_REVIEW_NO_ID;
|
||||
delete process.env.FAKE_GH_LIST_ERROR;
|
||||
delete process.env.FAKE_GH_LIST_ERROR_PAGE;
|
||||
delete process.env.FAKE_GH_PATCH_ERROR;
|
||||
delete process.env.FAKE_GH_COMMENT_ERROR;
|
||||
delete process.env.FAKE_GH_COMMENT_BODY;
|
||||
delete process.env.GITHUB_STEP_SUMMARY;
|
||||
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 });
|
||||
});
|
||||
|
||||
// Two findings by default; `count` findings (one per file) for the pagination test.
|
||||
async function syncNewThreads(
|
||||
prNumber: number,
|
||||
count?: number,
|
||||
): Promise<{
|
||||
urls: Map<string, string>;
|
||||
calls: string[];
|
||||
}> {
|
||||
const targets = count
|
||||
? Array.from({ length: count }, (_, index) => ({
|
||||
fp: (0xa00000 + index).toString(16),
|
||||
finding: finding({ file: `f${index}.ts`, line: 1 }),
|
||||
}))
|
||||
: [
|
||||
{ fp: "aaaa11", finding: finding() },
|
||||
{ fp: "bbbb22", finding: finding({ file: "b.ts", line: 9, severity: "suggestion" }) },
|
||||
];
|
||||
const reviewComments = targets.map((target, index) => ({
|
||||
id: 500 + index,
|
||||
body: renderInlineCommentBody(target.finding, TAG, target.fp),
|
||||
html_url: `https://github.com/o/r/pull/${prNumber}#discussion_r${500 + index}`,
|
||||
}));
|
||||
for (let page = 1; (page - 1) * 100 < reviewComments.length; page++) {
|
||||
await writeFile(
|
||||
path.join(dir, `review-comments-${page}.json`),
|
||||
JSON.stringify(reviewComments.slice((page - 1) * 100, page * 100)),
|
||||
"utf8",
|
||||
);
|
||||
}
|
||||
const reporter = new GitHubReporter({
|
||||
prNumber,
|
||||
repo: "o/r",
|
||||
commentTag: TAG,
|
||||
ownLogin: "ecr-bot",
|
||||
headSha: HEAD,
|
||||
inline: { enabled: true, maxComments: Math.max(20, targets.length) },
|
||||
});
|
||||
const diffLines = new Map<string, Set<number>>();
|
||||
for (const { finding: target } of targets) {
|
||||
diffLines.set(target.file, new Set([...(diffLines.get(target.file) ?? []), target.line!]));
|
||||
}
|
||||
const link: LinkContext = { repo: "o/r", prNumber, diffLines };
|
||||
// syncInline is private; the test drives it directly so the payload GitHub would
|
||||
// receive is checked without stubbing the rest of report().
|
||||
const sync = (
|
||||
reporter as unknown as {
|
||||
syncInline: (
|
||||
targets: Array<{ finding: Finding; fp: string }>,
|
||||
link: LinkContext,
|
||||
opts: { teardown: boolean },
|
||||
) => Promise<Map<string, string>>;
|
||||
}
|
||||
).syncInline.bind(reporter);
|
||||
const urls = await sync(targets, link, { teardown: true });
|
||||
const calls = (await readFile(path.join(dir, "calls.log"), "utf8")).trim().split("\n");
|
||||
return { urls, calls };
|
||||
}
|
||||
|
||||
test("syncInline: posts all new threads in one COMMENT review and links each one", async () => {
|
||||
const { urls, calls } = await syncNewThreads(701);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toEqual([
|
||||
expect.stringContaining("repos/o/r/pulls/701/reviews --input"),
|
||||
]);
|
||||
const review = JSON.parse(await readFile(path.join(dir, "review.json"), "utf8"));
|
||||
expect(review).toMatchObject({ commit_id: HEAD, event: "COMMENT", body: "" });
|
||||
expect(
|
||||
review.comments.map((c: { path: string; line: number; side: string }) => [
|
||||
c.path,
|
||||
c.line,
|
||||
c.side,
|
||||
]),
|
||||
).toEqual([
|
||||
["a.ts", 5, "RIGHT"],
|
||||
["b.ts", 9, "RIGHT"],
|
||||
]);
|
||||
// The read-back fixture is built separately, so only this catches a lost body.
|
||||
expect(review.comments.map((c: { body: string }) => c.body)).toEqual([
|
||||
renderInlineCommentBody(finding(), TAG, "aaaa11"),
|
||||
renderInlineCommentBody(
|
||||
finding({ file: "b.ts", line: 9, severity: "suggestion" }),
|
||||
TAG,
|
||||
"bbbb22",
|
||||
),
|
||||
]);
|
||||
expect(urls.get("aaaa11")).toBe("https://github.com/o/r/pull/701#discussion_r500");
|
||||
expect(urls.get("bbbb22")).toBe("https://github.com/o/r/pull/701#discussion_r501");
|
||||
});
|
||||
|
||||
test("syncInline: a rejected review falls back to one comment per thread", async () => {
|
||||
process.env.FAKE_GH_REVIEW_ERROR = "gh: Unprocessable Entity (HTTP 422)";
|
||||
const { urls, calls } = await syncNewThreads(702);
|
||||
const posts = calls.filter((call) => call.includes("-X POST"));
|
||||
expect(posts).toHaveLength(3);
|
||||
expect(posts[0]).toContain("repos/o/r/pulls/702/reviews --input");
|
||||
expect(
|
||||
posts.slice(1).every((call) => call.includes("repos/o/r/pulls/702/comments --input")),
|
||||
).toBe(true);
|
||||
expect([...urls.keys()].sort()).toEqual(["aaaa11", "bbbb22"]);
|
||||
});
|
||||
|
||||
test("syncInline: a rate-limited review posts nothing else", async () => {
|
||||
process.env.FAKE_GH_REVIEW_ERROR = "gh: You have exceeded a secondary rate limit (HTTP 403)";
|
||||
const { urls, calls } = await syncNewThreads(703);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1);
|
||||
expect(urls.size).toBe(0);
|
||||
});
|
||||
|
||||
test("syncInline: a created review without an id is never posted again", async () => {
|
||||
process.env.FAKE_GH_REVIEW_NO_ID = "1";
|
||||
const { urls, calls } = await syncNewThreads(704);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1);
|
||||
expect(urls.size).toBe(0);
|
||||
});
|
||||
|
||||
test("syncInline: a review that fails without a 422 is not retried, since it may exist", async () => {
|
||||
process.env.FAKE_GH_REVIEW_ERROR = "gh: Bad Gateway (HTTP 502)";
|
||||
const { urls, calls } = await syncNewThreads(705);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1);
|
||||
expect(urls.size).toBe(0);
|
||||
});
|
||||
|
||||
test("syncInline: a created review whose comments can't be read is not posted again", async () => {
|
||||
process.env.FAKE_GH_LIST_ERROR = "gh: Internal Server Error (HTTP 500)";
|
||||
const { urls, calls } = await syncNewThreads(706);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1);
|
||||
expect(urls.size).toBe(0);
|
||||
});
|
||||
|
||||
test("syncInline: links every thread of a review with more than one page of comments", async () => {
|
||||
const { urls, calls } = await syncNewThreads(707, 150);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1);
|
||||
expect(calls.filter((call) => call.includes("/reviews/99/comments"))).toHaveLength(2);
|
||||
expect(urls.size).toBe(150);
|
||||
});
|
||||
|
||||
// One thread of ours from an earlier run whose finding is gone, so the sync deletes it
|
||||
// after posting the new review.
|
||||
async function writeStaleThread(): Promise<void> {
|
||||
await writeFile(
|
||||
path.join(dir, "pr-comments.json"),
|
||||
JSON.stringify([
|
||||
{
|
||||
id: 77,
|
||||
body: renderInlineCommentBody(finding({ file: "c.ts", line: 3 }), TAG, "dddd44"),
|
||||
user: { login: "ecr-bot" },
|
||||
html_url: "https://github.com/o/r/pull/7#discussion_r77",
|
||||
},
|
||||
]),
|
||||
"utf8",
|
||||
);
|
||||
}
|
||||
|
||||
test("syncInline: one run posts the review, links it, and deletes a retired thread", async () => {
|
||||
await writeStaleThread();
|
||||
const { urls, calls } = await syncNewThreads(708);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1);
|
||||
expect(calls.filter((call) => call.includes("-X DELETE"))).toEqual([
|
||||
expect.stringContaining("repos/o/r/pulls/comments/77"),
|
||||
]);
|
||||
expect([...urls.keys()].sort()).toEqual(["aaaa11", "bbbb22"]);
|
||||
});
|
||||
|
||||
test("syncInline: a rate limit while reading the review's comments stops later mutations", async () => {
|
||||
await writeStaleThread();
|
||||
process.env.FAKE_GH_LIST_ERROR = "gh: API rate limit exceeded (HTTP 429)";
|
||||
const { urls, calls } = await syncNewThreads(709);
|
||||
expect(calls.filter((call) => call.includes("-X DELETE"))).toHaveLength(0);
|
||||
expect(urls.size).toBe(0);
|
||||
});
|
||||
|
||||
test("syncInline: a review page that fails keeps the links of the pages before it", async () => {
|
||||
process.env.FAKE_GH_LIST_ERROR = "gh: Internal Server Error (HTTP 500)";
|
||||
process.env.FAKE_GH_LIST_ERROR_PAGE = "2";
|
||||
const { urls } = await syncNewThreads(710, 150);
|
||||
expect(urls.size).toBe(100);
|
||||
});
|
||||
|
||||
test("syncInline: a review refused for a pending review isn't posted one by one", async () => {
|
||||
await writeStaleThread();
|
||||
process.env.FAKE_GH_REVIEW_ERROR = "gh: Unprocessable Entity (HTTP 422)";
|
||||
process.env.FAKE_GH_REVIEW_BODY = JSON.stringify({
|
||||
message: "Unprocessable Entity",
|
||||
errors: ["User can only have one pending review per pull request"],
|
||||
});
|
||||
const { urls, calls } = await syncNewThreads(711);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1);
|
||||
// Only the new threads fail: the rest of the sync still runs.
|
||||
expect(calls.filter((call) => call.includes("-X DELETE"))).toHaveLength(1);
|
||||
expect(urls.size).toBe(0);
|
||||
});
|
||||
|
||||
test("syncInline: a review refused as spam aborts the rest of the sync", async () => {
|
||||
await writeStaleThread();
|
||||
process.env.FAKE_GH_REVIEW_ERROR = "gh: Validation Failed (HTTP 422)";
|
||||
process.env.FAKE_GH_REVIEW_BODY = JSON.stringify({
|
||||
message: "Validation Failed",
|
||||
errors: [
|
||||
{ resource: "PullRequestReview", code: "custom", message: "was submitted too quickly" },
|
||||
],
|
||||
});
|
||||
const { urls, calls } = await syncNewThreads(712);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(1);
|
||||
expect(calls.filter((call) => call.includes("-X DELETE"))).toHaveLength(0);
|
||||
expect(urls.size).toBe(0);
|
||||
});
|
||||
|
||||
test("syncInline: a rate limit while editing a thread skips the review", async () => {
|
||||
// aaaa11's thread exists with an outdated body, so the sync patches it before it
|
||||
// posts bbbb22 in a review.
|
||||
await writeFile(
|
||||
path.join(dir, "pr-comments.json"),
|
||||
JSON.stringify([
|
||||
{
|
||||
id: 78,
|
||||
body: renderInlineCommentBody(finding({ title: "An older title" }), TAG, "aaaa11"),
|
||||
user: { login: "ecr-bot" },
|
||||
html_url: "https://github.com/o/r/pull/7#discussion_r78",
|
||||
},
|
||||
]),
|
||||
"utf8",
|
||||
);
|
||||
process.env.FAKE_GH_PATCH_ERROR = "gh: API rate limit exceeded (HTTP 429)";
|
||||
const { calls } = await syncNewThreads(713);
|
||||
expect(calls.filter((call) => call.includes("-X PATCH"))).toHaveLength(1);
|
||||
expect(calls.filter((call) => call.includes("-X POST"))).toHaveLength(0);
|
||||
});
|
||||
|
||||
test("syncInline: the spam throttle during the one-by-one fallback aborts the rest", async () => {
|
||||
await writeStaleThread();
|
||||
process.env.FAKE_GH_REVIEW_ERROR = "gh: Unprocessable Entity (HTTP 422)";
|
||||
process.env.FAKE_GH_REVIEW_BODY = JSON.stringify({
|
||||
message: "Unprocessable Entity",
|
||||
errors: ["Line could not be resolved"],
|
||||
});
|
||||
process.env.FAKE_GH_COMMENT_ERROR = "gh: Validation Failed (HTTP 422)";
|
||||
process.env.FAKE_GH_COMMENT_BODY = JSON.stringify({
|
||||
message: "Validation Failed",
|
||||
errors: [
|
||||
{
|
||||
resource: "PullRequestReviewComment",
|
||||
code: "custom",
|
||||
message: "was submitted too quickly",
|
||||
},
|
||||
],
|
||||
});
|
||||
const { calls } = await syncNewThreads(714);
|
||||
expect(calls.filter((call) => call.includes("/comments --input"))).toHaveLength(1);
|
||||
expect(calls.filter((call) => call.includes("-X DELETE"))).toHaveLength(0);
|
||||
});
|
||||
|
||||
test("ghApiFailure: keeps the response body's messages, not the body", () => {
|
||||
const json = ghApiFailure("creating the review", {
|
||||
code: 1,
|
||||
stderr: "gh: Validation Failed (HTTP 422)\n",
|
||||
stdout: JSON.stringify({
|
||||
message: "Validation Failed",
|
||||
errors: ["one", { message: "two" }, { code: "custom" }, null],
|
||||
documentation_url: "https://docs.github.com",
|
||||
}),
|
||||
});
|
||||
expect(json.message).toBe(
|
||||
"creating the review failed: gh: Validation Failed (HTTP 422): Validation Failed; one; two",
|
||||
);
|
||||
const html = ghApiFailure("creating the review", {
|
||||
code: 1,
|
||||
stderr: "gh: HTTP 502\n",
|
||||
stdout: "<html><body>Bad Gateway</body></html>",
|
||||
});
|
||||
expect(html.message).toBe("creating the review failed: gh: HTTP 502");
|
||||
const silent = ghApiFailure("creating the review", {
|
||||
code: 1,
|
||||
stderr: "",
|
||||
stdout: "",
|
||||
signal: "SIGKILL",
|
||||
});
|
||||
expect(silent.message).toBe("creating the review failed: gh exited 1 (SIGKILL)");
|
||||
});
|
||||
|
||||
test("ghApiFailure: caps each message, not the whole text, so a late throttle survives", () => {
|
||||
const error = ghApiFailure("creating the review", {
|
||||
code: 1,
|
||||
stderr: "gh: Validation Failed (HTTP 422)",
|
||||
stdout: JSON.stringify({
|
||||
message: "Validation Failed",
|
||||
errors: [
|
||||
...Array.from({ length: 30 }, () => ({ message: "Line could not be resolved" })),
|
||||
{ message: "x".repeat(1000) },
|
||||
{ message: "was submitted too quickly" },
|
||||
],
|
||||
}),
|
||||
});
|
||||
expect(error.message).toBe(
|
||||
`creating the review failed: gh: Validation Failed (HTTP 422): Validation Failed; Line could not be resolved; ${"x".repeat(200)}; was submitted too quickly`,
|
||||
);
|
||||
});
|
||||
@@ -0,0 +1,270 @@
|
||||
import { test, expect, afterAll, afterEach, beforeAll } from "bun:test";
|
||||
import { chmod, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
import { git } from "../core/exec.js";
|
||||
import { LocalGitSource } from "../sources/local-git.js";
|
||||
|
||||
// This file's fixture git must stay inside its temp repos. Git exports GIT_DIR,
|
||||
// GIT_INDEX_FILE and friends to hooks, so `bun test` run from a pre-commit hook would
|
||||
// otherwise write these fixtures into the commit being made. The user's global and
|
||||
// system git files (signing, a hooks path, init templates, a global ignore or
|
||||
// attributes file) could also fail the fixtures before any assertion runs, so HOME and
|
||||
// XDG_CONFIG_HOME point at an empty directory, which works on every git version.
|
||||
const savedEnv: Record<string, string | undefined> = {};
|
||||
// Set only once mkdtemp succeeds: afterAll runs even when beforeAll throws, and must
|
||||
// never remove whatever HOME still points at.
|
||||
let fakeHome: string | undefined;
|
||||
beforeAll(async () => {
|
||||
for (const [name, value] of Object.entries(process.env)) {
|
||||
if (name.startsWith("GIT_")) {
|
||||
savedEnv[name] = value;
|
||||
delete process.env[name];
|
||||
}
|
||||
}
|
||||
fakeHome = await mkdtemp(path.join(tmpdir(), "ecr-local-git-home-"));
|
||||
for (const [name, value] of [
|
||||
["HOME", fakeHome],
|
||||
["XDG_CONFIG_HOME", fakeHome],
|
||||
["GIT_CONFIG_NOSYSTEM", "1"],
|
||||
] as const) {
|
||||
savedEnv[name] ??= process.env[name];
|
||||
process.env[name] = value;
|
||||
}
|
||||
});
|
||||
afterAll(async () => {
|
||||
for (const [name, value] of Object.entries(savedEnv)) {
|
||||
if (value === undefined) {
|
||||
delete process.env[name];
|
||||
} else {
|
||||
process.env[name] = value;
|
||||
}
|
||||
}
|
||||
if (fakeHome) {
|
||||
await rm(fakeHome, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
const dirs: string[] = [];
|
||||
afterEach(async () => {
|
||||
for (const dir of dirs.splice(0)) {
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
async function tempDir(prefix: string): Promise<string> {
|
||||
const dir = await mkdtemp(path.join(tmpdir(), prefix));
|
||||
dirs.push(dir);
|
||||
return dir;
|
||||
}
|
||||
|
||||
function commit(root: string, message: string): Promise<string> {
|
||||
return git(["-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", message], root);
|
||||
}
|
||||
|
||||
async function executable(dir: string, name: string, script: string): Promise<string> {
|
||||
const file = path.join(dir, name);
|
||||
await writeFile(file, script, "utf8");
|
||||
await chmod(file, 0o755);
|
||||
return file;
|
||||
}
|
||||
|
||||
// A committed repo with `a.txt` and `b/inner.txt`. The `b/` path matters: with no
|
||||
// prefixes, `+++ b/inner.txt` loses its directory to the parser's `b/` strip.
|
||||
async function baseRepo(): Promise<string> {
|
||||
const root = await tempDir("ecr-local-git-");
|
||||
await git(["init", "-q"], root);
|
||||
await mkdir(path.join(root, "b"));
|
||||
await writeFile(path.join(root, "a.txt"), "one\n", "utf8");
|
||||
await writeFile(path.join(root, "b", "inner.txt"), "inner\n", "utf8");
|
||||
await git(["add", "-A"], root);
|
||||
await commit(root, "base");
|
||||
return root;
|
||||
}
|
||||
|
||||
// A repo whose local git config changes what `git diff` prints: an external diff
|
||||
// tool, forced color, and no `a/` and `b/` prefixes. All of them break the parser
|
||||
// unless the engine overrides them.
|
||||
async function repoWithDiffConfig(): Promise<string> {
|
||||
const root = await baseRepo();
|
||||
const tools = await tempDir("ecr-local-git-tools-");
|
||||
const external = await executable(
|
||||
tools,
|
||||
"external-diff",
|
||||
'#!/bin/sh\necho "external diff of $1"\n',
|
||||
);
|
||||
await git(["config", "diff.external", external], root);
|
||||
await git(["config", "color.diff", "always"], root);
|
||||
await git(["config", "diff.noprefix", "true"], root);
|
||||
return root;
|
||||
}
|
||||
|
||||
async function repoWithMnemonicPrefix(): Promise<string> {
|
||||
const root = await baseRepo();
|
||||
await git(["config", "diff.mnemonicPrefix", "true"], root);
|
||||
return root;
|
||||
}
|
||||
|
||||
async function editBoth(root: string): Promise<void> {
|
||||
await writeFile(path.join(root, "a.txt"), "one\ntwo\n", "utf8");
|
||||
await writeFile(path.join(root, "b", "inner.txt"), "inner\nmore\n", "utf8");
|
||||
}
|
||||
|
||||
test("LocalGitSource: working-tree diffs parse despite the user's diff config", async () => {
|
||||
const root = await repoWithDiffConfig();
|
||||
await editBoth(root);
|
||||
await writeFile(path.join(root, "b", "new.txt"), "new\n", "utf8");
|
||||
const files = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles();
|
||||
expect(files.map((file) => file.path).sort()).toEqual(["a.txt", "b/inner.txt", "b/new.txt"]);
|
||||
expect(files.every((file) => file.patch.includes("@@"))).toBe(true);
|
||||
});
|
||||
|
||||
test("LocalGitSource: a base...head diff parses despite the user's diff config", async () => {
|
||||
const root = await repoWithDiffConfig();
|
||||
await editBoth(root);
|
||||
await git(["add", "-A"], root);
|
||||
await commit(root, "change");
|
||||
const files = await new LocalGitSource({
|
||||
cwd: root,
|
||||
base: "HEAD~1",
|
||||
head: "HEAD",
|
||||
}).getChangedFiles();
|
||||
expect(files.map((file) => file.path)).toEqual(["a.txt", "b/inner.txt"]);
|
||||
expect(files[0]!.patch).toContain("+two");
|
||||
});
|
||||
|
||||
test("LocalGitSource: a staged diff parses despite the user's diff config", async () => {
|
||||
const root = await repoWithDiffConfig();
|
||||
await editBoth(root);
|
||||
await git(["add", "-A"], root);
|
||||
const files = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles();
|
||||
expect(files.map((file) => file.path)).toEqual(["a.txt", "b/inner.txt"]);
|
||||
expect(files[0]!.patch).toContain("+two");
|
||||
});
|
||||
|
||||
test("LocalGitSource: diff.mnemonicPrefix doesn't rename working-tree, untracked or staged paths", async () => {
|
||||
// Mnemonic prefixes are `c/` and `w/` for the working tree, `2/` for --no-index,
|
||||
// and `i/` for the index, none of which the parser strips.
|
||||
const root = await repoWithMnemonicPrefix();
|
||||
await editBoth(root);
|
||||
await writeFile(path.join(root, "new.txt"), "new\n", "utf8");
|
||||
const working = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles();
|
||||
expect(working.map((file) => file.path).sort()).toEqual(["a.txt", "b/inner.txt", "new.txt"]);
|
||||
await git(["add", "-A"], root);
|
||||
const staged = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles();
|
||||
expect(staged.map((file) => file.path)).toEqual(["a.txt", "b/inner.txt", "new.txt"]);
|
||||
});
|
||||
|
||||
// Only a deletion takes its path from the `---` line, so only a deletion shows a wrong
|
||||
// source prefix: `--- b/inner.txt` would lose its directory, and `--- c/b/inner.txt`
|
||||
// would keep a `c/` that isn't part of the path.
|
||||
test.each([
|
||||
["diff.noprefix", repoWithDiffConfig],
|
||||
["diff.mnemonicPrefix", repoWithMnemonicPrefix],
|
||||
])("LocalGitSource: a deleted file keeps its path under %s", async (_config, makeRepo) => {
|
||||
const root = await makeRepo();
|
||||
await rm(path.join(root, "b", "inner.txt"));
|
||||
const working = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles();
|
||||
await git(["add", "-A"], root);
|
||||
const staged = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles();
|
||||
await commit(root, "delete");
|
||||
const range = await new LocalGitSource({
|
||||
cwd: root,
|
||||
base: "HEAD~1",
|
||||
head: "HEAD",
|
||||
}).getChangedFiles();
|
||||
for (const files of [working, staged, range]) {
|
||||
expect(files.map((file) => [file.status, file.path])).toEqual([["D", "b/inner.txt"]]);
|
||||
}
|
||||
});
|
||||
|
||||
test("LocalGitSource: a textconv driver can't replace or hide the real change", async () => {
|
||||
// A driver that prints a constant makes both sides identical, so with textconv on
|
||||
// the change to a.txt vanishes from the review.
|
||||
const root = await baseRepo();
|
||||
const tools = await tempDir("ecr-local-git-tools-");
|
||||
const mask = await executable(tools, "mask", "#!/bin/sh\necho masked\n");
|
||||
await writeFile(path.join(root, ".git", "info", "attributes"), "*.txt diff=mask\n", "utf8");
|
||||
await git(["config", "diff.mask.textconv", mask], root);
|
||||
await editBoth(root);
|
||||
const files = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles();
|
||||
expect(files.map((file) => file.path)).toEqual(["a.txt", "b/inner.txt"]);
|
||||
expect(files[0]!.patch).toContain("+two");
|
||||
expect(files[0]!.patch).not.toContain("masked");
|
||||
});
|
||||
|
||||
test("LocalGitSource: copy detection can't turn a new file into an empty modification", async () => {
|
||||
// With diff.renames=copies, an exact copy of a file the change also edits comes out
|
||||
// as `copy from`/`copy to` with no hunk, so the new file's contents go unreviewed.
|
||||
const root = await baseRepo();
|
||||
await git(["config", "diff.renames", "copies"], root);
|
||||
await writeFile(path.join(root, "b", "copy.txt"), "inner\n", "utf8");
|
||||
await writeFile(path.join(root, "b", "inner.txt"), "inner\nmore\n", "utf8");
|
||||
await git(["add", "-A"], root);
|
||||
const files = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles();
|
||||
const copy = files.find((file) => file.path === "b/copy.txt");
|
||||
expect(copy?.status).toBe("A");
|
||||
expect(copy?.patch).toContain("+inner");
|
||||
});
|
||||
|
||||
// `diff.submodule=log` prints `Submodule sub <old>..<new>:` and commit subjects instead
|
||||
// of a diff, so the change parses as nothing. `diff.submodule=diff` prints the
|
||||
// submodule's own file diffs, so the review sees `sub/s.txt` instead of the pointer
|
||||
// change the commit makes. `diff.ignoreSubmodules=all`, or `ignore = all` in
|
||||
// `.gitmodules`, drops the change entirely.
|
||||
async function repoWithSubmodule(): Promise<{ root: string; sub: string }> {
|
||||
const root = await baseRepo();
|
||||
const sub = path.join(root, "sub");
|
||||
await mkdir(sub);
|
||||
await git(["init", "-q"], sub);
|
||||
await writeFile(path.join(sub, "s.txt"), "one\n", "utf8");
|
||||
await git(["add", "-A"], sub);
|
||||
await commit(sub, "s1");
|
||||
await git(["add", "sub"], root);
|
||||
await commit(root, "add sub");
|
||||
return { root, sub };
|
||||
}
|
||||
|
||||
test.each([
|
||||
["diff.submodule=log", ["config", "diff.submodule", "log"]],
|
||||
["diff.submodule=diff", ["config", "diff.submodule", "diff"]],
|
||||
["diff.ignoreSubmodules=all", ["config", "diff.ignoreSubmodules", "all"]],
|
||||
[".gitmodules ignore = all", ["config", "-f", ".gitmodules", "submodule.sub.ignore", "all"]],
|
||||
])("LocalGitSource: %s still shows a submodule change as its pointer", async (_name, setting) => {
|
||||
const { root, sub } = await repoWithSubmodule();
|
||||
await writeFile(path.join(sub, "s.txt"), "two\n", "utf8");
|
||||
await git(["add", "-A"], sub);
|
||||
await commit(sub, "s2");
|
||||
await git(setting, root);
|
||||
if (setting.includes(".gitmodules")) {
|
||||
await git(["config", "-f", ".gitmodules", "submodule.sub.path", "sub"], root);
|
||||
await git(["add", ".gitmodules"], root);
|
||||
await commit(root, "ignore sub");
|
||||
}
|
||||
const working = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles();
|
||||
// `git add` skips a submodule that `.gitmodules` ignores unless forced.
|
||||
await git(["add", "--force", "sub"], root);
|
||||
const staged = await new LocalGitSource({ cwd: root, staged: true }).getChangedFiles();
|
||||
await commit(root, "bump sub");
|
||||
const range = await new LocalGitSource({
|
||||
cwd: root,
|
||||
base: "HEAD~1",
|
||||
head: "HEAD",
|
||||
}).getChangedFiles();
|
||||
for (const files of [working, staged, range]) {
|
||||
expect(files.map((file) => [file.status, file.path])).toEqual([["M", "sub"]]);
|
||||
expect(files[0]!.patch).toContain("+Subproject commit");
|
||||
}
|
||||
});
|
||||
|
||||
// Edits inside a submodule whose pointer didn't move aren't a change to review, even
|
||||
// under a user's `diff.ignoreSubmodules=none`.
|
||||
test("LocalGitSource: a submodule with local edits but the same pointer isn't a change", async () => {
|
||||
const { root, sub } = await repoWithSubmodule();
|
||||
await git(["config", "diff.ignoreSubmodules", "none"], root);
|
||||
await writeFile(path.join(sub, "s.txt"), "edited\n", "utf8");
|
||||
await writeFile(path.join(sub, "untracked.txt"), "new\n", "utf8");
|
||||
const files = await new LocalGitSource({ cwd: root, base: "HEAD" }).getChangedFiles();
|
||||
expect(files).toEqual([]);
|
||||
});
|
||||
+248
-18
@@ -4,6 +4,7 @@ import { tmpdir } from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
import { resolveTrustedTool, run } from "../core/exec.js";
|
||||
import type { RunResult } from "../core/exec.js";
|
||||
import { errorMessage } from "../core/util.js";
|
||||
import { parseUnifiedDiff } from "../core/diff.js";
|
||||
import {
|
||||
@@ -197,6 +198,54 @@ export interface ExistingInlineComment {
|
||||
hasReplies: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* The error for a failed `gh api` call run with `check: false`. gh prints only the
|
||||
* top-level message ("Validation Failed (HTTP 422)") to stderr; why GitHub refused,
|
||||
* such as its spam throttle or a pending review, is in the response body on stdout.
|
||||
* The body's distinct messages are kept, each capped, not the body: an HTML error page
|
||||
* or one error per refused comment can't flood the log, and a cap on the whole text
|
||||
* could cut off the message syncInline classifies by. Credentials never appear in
|
||||
* either stream.
|
||||
*/
|
||||
export function ghApiFailure(action: string, result: RunResult): Error {
|
||||
const details = new Set<string>();
|
||||
try {
|
||||
const body = JSON.parse(result.stdout) as { message?: unknown; errors?: unknown };
|
||||
if (typeof body.message === "string") {
|
||||
details.add(body.message.slice(0, 200));
|
||||
}
|
||||
for (const item of Array.isArray(body.errors) ? body.errors : []) {
|
||||
const message =
|
||||
typeof item === "string" ? item : (item as { message?: unknown } | null)?.message;
|
||||
if (typeof message === "string") {
|
||||
details.add(message.slice(0, 200));
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
// Not JSON (an HTML error page, or nothing): stderr alone says what happened.
|
||||
}
|
||||
const stderr = result.stderr.trim();
|
||||
const reason =
|
||||
[stderr, [...details].join("; ")].filter(Boolean).join(": ") ||
|
||||
`gh exited ${result.code}${result.signal ? ` (${result.signal})` : ""}`;
|
||||
return new Error(`${action} failed: ${reason}`);
|
||||
}
|
||||
|
||||
/**
|
||||
* fp → permalink for the inline comments of ours among `comments`, read from the
|
||||
* anchored marker in each body (see parseInlineMarkerFp).
|
||||
*/
|
||||
export function inlineUrlsByFp(comments: ReviewCommentApi[], tag: string): Map<string, string> {
|
||||
const urls = new Map<string, string>();
|
||||
for (const comment of comments) {
|
||||
const fp = parseInlineMarkerFp(comment.body ?? "", tag);
|
||||
if (fp && comment.html_url) {
|
||||
urls.set(fp, comment.html_url);
|
||||
}
|
||||
}
|
||||
return urls;
|
||||
}
|
||||
|
||||
/** What one inline sync run will do. Bodies are rendered by the executor. */
|
||||
export interface InlineSyncPlan {
|
||||
/** Findings to POST a new inline comment for (needs a commit id). */
|
||||
@@ -553,7 +602,8 @@ export function selectOwnComments(
|
||||
/**
|
||||
* Maintains exactly one PR comment, updating it in place across re-reviews (and
|
||||
* cleaning up duplicates) so the review converges instead of churning. Runs the
|
||||
* break-glass check first. Comment-only: never calls the review-state APIs.
|
||||
* break-glass check first. Never approves or requests changes: a run's new inline
|
||||
* comments go out in one COMMENT review.
|
||||
*/
|
||||
export class GitHubReporter implements Reporter {
|
||||
private readonly marker: string;
|
||||
@@ -1245,10 +1295,28 @@ export class GitHubReporter implements Reporter {
|
||||
}
|
||||
}
|
||||
|
||||
/** Secondary-rate-limit shapes (403/429): abort the rest of the sync, not just
|
||||
* this mutation — hammering on would dig the hole deeper. */
|
||||
/** Secondary-rate-limit shapes (403/429), and the spam throttle's 422 ("submitted
|
||||
* too quickly", only in errors from ghApiFailure): abort the rest of the sync, not
|
||||
* just this mutation — hammering on would dig the hole deeper. */
|
||||
private static isRateLimitError(error: unknown): boolean {
|
||||
return /HTTP 403|HTTP 429|rate limit|secondary rate/i.test(errorMessage(error));
|
||||
return /HTTP 403|HTTP 429|rate limit|secondary rate|submitted too quickly/i.test(
|
||||
errorMessage(error),
|
||||
);
|
||||
}
|
||||
|
||||
/** 422: GitHub validated the request and created nothing. */
|
||||
private static isUnprocessableError(error: unknown): boolean {
|
||||
return /HTTP 422/.test(errorMessage(error));
|
||||
}
|
||||
|
||||
/**
|
||||
* A 422 because the posting account already has a pending review on this PR, which
|
||||
* GitHub allows only one of. Posting the comments one by one would hit it too.
|
||||
*/
|
||||
private static isPendingReviewError(error: unknown): boolean {
|
||||
return (
|
||||
GitHubReporter.isUnprocessableError(error) && /one pending review/i.test(errorMessage(error))
|
||||
);
|
||||
}
|
||||
|
||||
// @ref LLP 0008#github-reporter-identity [constrained-by] — inline threads are
|
||||
@@ -1361,18 +1429,88 @@ export class GitHubReporter implements Reporter {
|
||||
counts.patched++;
|
||||
});
|
||||
}
|
||||
for (const entry of creates) {
|
||||
await attempt(async () => {
|
||||
const created = await this.createReviewComment(
|
||||
renderInlineCommentBody(entry.finding, this.options.commentTag, entry.fp),
|
||||
entry.finding.file,
|
||||
entry.finding.line!,
|
||||
);
|
||||
counts.created++;
|
||||
if (created?.html_url) {
|
||||
urls.set(entry.fp, created.html_url);
|
||||
if (creates.length > 0 && !aborted) {
|
||||
const comments = creates.map((entry) => ({
|
||||
fp: entry.fp,
|
||||
body: renderInlineCommentBody(entry.finding, this.options.commentTag, entry.fp),
|
||||
path: entry.finding.file,
|
||||
line: entry.finding.line!,
|
||||
}));
|
||||
// @ref LLP 0008#inline-threads [implements] — one COMMENT review per run for new threads; per-comment fallback when GitHub rejects the review
|
||||
let posted = false;
|
||||
let retryOneByOne = false;
|
||||
let reviewId: number | null = null;
|
||||
try {
|
||||
reviewId = await this.createInlineReview(comments);
|
||||
posted = true;
|
||||
} catch (error) {
|
||||
if (GitHubReporter.isRateLimitError(error)) {
|
||||
counts.failed += comments.length;
|
||||
aborted = true;
|
||||
process.stderr.write(
|
||||
`inline sync: rate-limited; aborting remaining inline mutations this run: ${errorMessage(error)}\n`,
|
||||
);
|
||||
} else if (GitHubReporter.isPendingReviewError(error)) {
|
||||
counts.failed += comments.length;
|
||||
process.stderr.write(
|
||||
`inline sync: the posting account has a pending review on this PR, and GitHub allows only one; submit or delete it so new inline comments can post: ${errorMessage(error)}\n`,
|
||||
);
|
||||
} else if (GitHubReporter.isUnprocessableError(error)) {
|
||||
// GitHub rejects the whole review when it refuses one comment, so retry one
|
||||
// by one: only the comment it refuses is lost.
|
||||
retryOneByOne = true;
|
||||
process.stderr.write(
|
||||
`inline sync: GitHub rejected the review with ${comments.length} new comment(s); posting them one by one: ${errorMessage(error)}\n`,
|
||||
);
|
||||
} else {
|
||||
// A 5xx or a dropped connection can still have created the review, so a retry
|
||||
// could post every comment twice. The findings render in the main comment,
|
||||
// and the next run creates the threads that are still missing.
|
||||
counts.failed += comments.length;
|
||||
process.stderr.write(
|
||||
`inline sync: the review with ${comments.length} new comment(s) failed and may exist; not retrying: ${errorMessage(error)}\n`,
|
||||
);
|
||||
}
|
||||
});
|
||||
}
|
||||
if (posted) {
|
||||
counts.created += comments.length;
|
||||
try {
|
||||
if (reviewId == null) {
|
||||
throw new Error("the created review has no id");
|
||||
}
|
||||
for await (const page of this.inlineReviewCommentPages(reviewId)) {
|
||||
for (const [fp, url] of inlineUrlsByFp(page, this.options.commentTag)) {
|
||||
urls.set(fp, url);
|
||||
}
|
||||
}
|
||||
} catch (error) {
|
||||
// The comments exist; without their links the findings render full-form in
|
||||
// the main comment this run, and the next run finds them as live threads.
|
||||
process.stderr.write(
|
||||
`inline sync: could not list the new review's comments (the unlinked ones render in the main comment): ${errorMessage(error)}\n`,
|
||||
);
|
||||
if (GitHubReporter.isRateLimitError(error)) {
|
||||
aborted = true;
|
||||
process.stderr.write(
|
||||
"inline sync: rate-limited; aborting remaining inline mutations this run.\n",
|
||||
);
|
||||
}
|
||||
}
|
||||
} else if (retryOneByOne) {
|
||||
for (const comment of comments) {
|
||||
await attempt(async () => {
|
||||
const created = await this.createReviewComment(
|
||||
comment.body,
|
||||
comment.path,
|
||||
comment.line,
|
||||
);
|
||||
counts.created++;
|
||||
if (created?.html_url) {
|
||||
urls.set(comment.fp, created.html_url);
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
for (const comment of plan.stub) {
|
||||
const body = inlineStubBody(this.options.commentTag, comment.fp);
|
||||
@@ -1404,6 +1542,95 @@ export class GitHubReporter implements Reporter {
|
||||
return urls;
|
||||
}
|
||||
|
||||
/**
|
||||
* Post `comments` as one COMMENT review anchored to the reviewed head and return
|
||||
* the review id, or null when the review was created but its id can't be read.
|
||||
* Throws when the POST fails. After a 422, GitHub created nothing, because it
|
||||
* rejects all of the comments together; after any other failure, such as a 5xx
|
||||
* or a dropped connection, the review may still exist.
|
||||
*/
|
||||
private async createInlineReview(
|
||||
comments: Array<{ body: string; path: string; line: number }>,
|
||||
): Promise<number | null> {
|
||||
this.invalidateReviewComments();
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
const payload = JSON.stringify({
|
||||
commit_id: this.options.headSha,
|
||||
event: "COMMENT",
|
||||
// Empty on purpose. GitHub accepts it next to comments, despite its docs, and a
|
||||
// submitted review can't be deleted: a body would stay on the PR after its
|
||||
// threads are torn down.
|
||||
body: "",
|
||||
comments: comments.map(({ body, path: path_, line }) => ({
|
||||
body,
|
||||
path: path_,
|
||||
line,
|
||||
side: "RIGHT",
|
||||
})),
|
||||
});
|
||||
const dir = await mkdtemp(path.join(tmpdir(), "ecr-"));
|
||||
const jsonPath = path.join(dir, "review.json");
|
||||
try {
|
||||
await writeFile(jsonPath, payload, "utf8");
|
||||
const result = await run(
|
||||
gh,
|
||||
[
|
||||
"api",
|
||||
"-X",
|
||||
"POST",
|
||||
`repos/${this.options.repo}/pulls/${this.options.prNumber}/reviews`,
|
||||
"--input",
|
||||
jsonPath,
|
||||
],
|
||||
{ cwd: this.options.cwd, check: false },
|
||||
);
|
||||
if (result.code !== 0) {
|
||||
throw ghApiFailure("creating the review", result);
|
||||
}
|
||||
const { stdout } = result;
|
||||
try {
|
||||
const review = JSON.parse(stdout) as { id?: unknown };
|
||||
return typeof review.id === "number" ? review.id : null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
} finally {
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The comments of one review, a page at a time (`inline.maxComments` has no upper
|
||||
* bound), so a page that fails doesn't cost the links of the pages before it.
|
||||
*/
|
||||
private async *inlineReviewCommentPages(reviewId: number): AsyncGenerator<ReviewCommentApi[]> {
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
for (let page = 1; page <= GitHubReporter.MAX_COMMENT_PAGES; page++) {
|
||||
const { stdout } = await run(
|
||||
gh,
|
||||
[
|
||||
"api",
|
||||
"-X",
|
||||
"GET",
|
||||
`repos/${this.options.repo}/pulls/${this.options.prNumber}/reviews/${reviewId}/comments`,
|
||||
"-f",
|
||||
"per_page=100",
|
||||
"-f",
|
||||
`page=${page}`,
|
||||
],
|
||||
{ cwd: this.options.cwd },
|
||||
);
|
||||
const batch = JSON.parse(stdout) as ReviewCommentApi[];
|
||||
if (!Array.isArray(batch) || batch.length === 0) {
|
||||
break;
|
||||
}
|
||||
yield batch;
|
||||
if (batch.length < 100) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private async createReviewComment(
|
||||
body: string,
|
||||
path_: string,
|
||||
@@ -1422,7 +1649,7 @@ export class GitHubReporter implements Reporter {
|
||||
const jsonPath = path.join(dir, "review-comment.json");
|
||||
try {
|
||||
await writeFile(jsonPath, payload, "utf8");
|
||||
const { stdout } = await run(
|
||||
const result = await run(
|
||||
gh,
|
||||
[
|
||||
"api",
|
||||
@@ -1432,10 +1659,13 @@ export class GitHubReporter implements Reporter {
|
||||
"--input",
|
||||
jsonPath,
|
||||
],
|
||||
{ cwd: this.options.cwd },
|
||||
{ cwd: this.options.cwd, check: false },
|
||||
);
|
||||
if (result.code !== 0) {
|
||||
throw ghApiFailure("creating the comment", result);
|
||||
}
|
||||
try {
|
||||
return JSON.parse(stdout) as { html_url?: string };
|
||||
return JSON.parse(result.stdout) as { html_url?: string };
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
|
||||
@@ -4,6 +4,25 @@ import { parseUnifiedDiff } from "../core/diff.js";
|
||||
import type { DiffEntry, ReviewMetadata } from "../core/schema.js";
|
||||
import type { ReviewSource } from "./source.js";
|
||||
|
||||
// `git diff` honors the user's config: an external diff tool (`diff.external`), forced
|
||||
// color, other path prefixes (`diff.noprefix`, `diff.mnemonicPrefix`), textconv
|
||||
// drivers, copy detection (`diff.renames=copies`), submodule logs or inline diffs
|
||||
// (`diff.submodule`), and ignored submodules (`diff.ignoreSubmodules`, or `ignore`
|
||||
// in `.gitmodules`). These flags restore the plain unified diff of the real file
|
||||
// contents, with the `a/` and `b/` prefixes that parseUnifiedDiff reads. A submodule
|
||||
// shows only when its pointer moves, never for edits inside it.
|
||||
// @ref LLP 0008#local-git-source [implements] — every parsed `git diff` overrides the user's diff config
|
||||
const PARSEABLE_DIFF_FLAGS = [
|
||||
"--no-ext-diff",
|
||||
"--no-color",
|
||||
"--src-prefix=a/",
|
||||
"--dst-prefix=b/",
|
||||
"--no-textconv",
|
||||
"--find-renames",
|
||||
"--submodule=short",
|
||||
"--ignore-submodules=dirty",
|
||||
];
|
||||
|
||||
export interface LocalGitOptions {
|
||||
/** Base ref to diff against. Defaults to the merge-base with the default branch. */
|
||||
base?: string;
|
||||
@@ -83,13 +102,16 @@ export class LocalGitSource implements ReviewSource {
|
||||
async getChangedFiles(): Promise<DiffEntry[]> {
|
||||
let raw: string;
|
||||
if (this.options.staged) {
|
||||
raw = await git(["diff", "--staged"], this.cwd);
|
||||
raw = await git(["diff", ...PARSEABLE_DIFF_FLAGS, "--staged"], this.cwd);
|
||||
} else {
|
||||
const base = await this.resolveBase();
|
||||
if (this.options.head) {
|
||||
raw = await git(["diff", `${base}...${this.options.head}`], this.cwd);
|
||||
raw = await git(
|
||||
["diff", ...PARSEABLE_DIFF_FLAGS, `${base}...${this.options.head}`],
|
||||
this.cwd,
|
||||
);
|
||||
} else {
|
||||
const tracked = await git(["diff", base], this.cwd);
|
||||
const tracked = await git(["diff", ...PARSEABLE_DIFF_FLAGS, base], this.cwd);
|
||||
const untracked = await this.untrackedDiffs();
|
||||
raw = [tracked, untracked].filter((chunk) => chunk.trim()).join("\n");
|
||||
}
|
||||
@@ -111,10 +133,11 @@ export class LocalGitSource implements ReviewSource {
|
||||
for (const file of files) {
|
||||
// `--` so a filename beginning with `-` can't be read as a git option.
|
||||
const gitPath = await resolveTrustedTool("git");
|
||||
const { stdout } = await run(gitPath, ["diff", "--no-index", "--", "/dev/null", file], {
|
||||
cwd: this.cwd,
|
||||
check: false,
|
||||
});
|
||||
const { stdout } = await run(
|
||||
gitPath,
|
||||
["diff", ...PARSEABLE_DIFF_FLAGS, "--no-index", "--", "/dev/null", file],
|
||||
{ cwd: this.cwd, check: false },
|
||||
);
|
||||
if (stdout.trim()) {
|
||||
chunks.push(stdout);
|
||||
}
|
||||
|
||||
@@ -170,9 +170,10 @@
|
||||
// Threads update in place across re-reviews; a retired thread with human
|
||||
// replies is stubbed, never deleted. Replies on inline threads are matched to
|
||||
// their finding like main-comment replies (same clearing rules).
|
||||
// NOTE: the first enabled run on a PR sends one notification per created
|
||||
// inline comment — `maxComments` bounds that. Disabling later leaves existing
|
||||
// threads up until a comment-mode switch (or manual cleanup) clears them.
|
||||
// A run's new inline comments go out in one COMMENT review, so each run puts
|
||||
// one review entry and one notification on the PR (one per scope in per-scope
|
||||
// mode). Disabling later leaves existing threads up until a comment-mode switch
|
||||
// (or manual cleanup) clears them.
|
||||
// "inline": {
|
||||
// "enabled": false,
|
||||
// "maxComments": 20 // per PR (single comment mode) / per scope (per-scope mode)
|
||||
|
||||
@@ -18,9 +18,10 @@ permissions:
|
||||
issues: write
|
||||
|
||||
env:
|
||||
# Published reviewer run via npx (override with repo variable ECR_VERSION).
|
||||
# Floor at 0.2.3 — the first version that ships `ecr dismiss`/`undismiss`.
|
||||
ECR_VERSION: ${{ vars.ECR_VERSION || '^0.2.3' }}
|
||||
# Published reviewer run via npx (override with repo variable ECR_VERSION; pin to
|
||||
# a specific version to freeze it). Defaults to `latest` like the review workflows:
|
||||
# /dismiss rewrites the comment state they write, so it must understand all of it.
|
||||
ECR_VERSION: ${{ vars.ECR_VERSION || 'latest' }}
|
||||
|
||||
concurrency:
|
||||
group: ai-code-review-dismiss-${{ github.event.issue.number }}
|
||||
|
||||
Reference in new issue
Block a user