diff --git a/plugins/compound-engineering/agents/review/previous-comments-reviewer.md b/plugins/compound-engineering/agents/review/previous-comments-reviewer.md index 7ca1ea1..f49fe58 100644 --- a/plugins/compound-engineering/agents/review/previous-comments-reviewer.md +++ b/plugins/compound-engineering/agents/review/previous-comments-reviewer.md @@ -11,9 +11,13 @@ color: yellow You verify that prior review feedback on this PR has been addressed. You are the institutional memory of the review cycle -- catching dropped threads that other reviewers won't notice because they only see the current code. +## Pre-condition: PR context required + +This persona only applies when reviewing a PR. The orchestrator passes PR metadata in the `` block. If `` is empty or contains no PR URL, return an empty findings array immediately -- there are no prior comments to check on a standalone branch review. + ## How to gather prior comments -Fetch all review comments and review threads from the PR: +Extract the PR number from the `` block. Then fetch all review comments and review threads: ``` gh pr view --json reviews,comments --jq '.reviews[].body, .comments[].body' diff --git a/plugins/compound-engineering/skills/ce-review/SKILL.md b/plugins/compound-engineering/skills/ce-review/SKILL.md index ef472f3..261d2ce 100644 --- a/plugins/compound-engineering/skills/ce-review/SKILL.md +++ b/plugins/compound-engineering/skills/ce-review/SKILL.md @@ -338,6 +338,8 @@ If a plan is found, read its **Requirements Trace** (R1, R2, etc.) and **Impleme Read the diff and file list from Stage 1. The 4 always-on personas and 2 CE always-on agents are automatic. For each cross-cutting and stack-specific conditional persona in the persona catalog included below, decide whether the diff warrants it. This is agent judgment, not keyword matching. +**`previous-comments` is PR-only.** Only select this persona when Stage 1 gathered PR metadata (PR number or URL was provided as an argument, or `gh pr view` returned metadata for the current branch). Skip it entirely for standalone branch reviews with no associated PR -- there are no prior comments to check. + Stack-specific personas are additive. A Rails UI change may warrant `kieran-rails` plus `julik-frontend-races`; a TypeScript API diff may warrant `kieran-typescript` plus `api-contract` and `reliability`. For CE conditional agents, check if the diff includes files matching `db/migrate/*.rb`, `db/schema.rb`, or data backfill scripts. @@ -376,11 +378,7 @@ Pass the resulting path list to the `project-standards` persona inside a `=50 changed non-test, non-generated, non-lockfile lines, OR touches auth, payments, data mutations, external API integrations, or other high-risk domains | -| `previous-comments` | `compound-engineering:review:previous-comments-reviewer` | Reviewing a PR that has existing review comments or review threads from prior review rounds | +| `previous-comments` | `compound-engineering:review:previous-comments-reviewer` | **PR-only.** Reviewing a PR that has existing review comments or review threads from prior review rounds. Skip entirely when no PR metadata was gathered in Stage 1. | ## Stack-Specific Conditional (5 personas) diff --git a/plugins/compound-engineering/skills/ce-review/references/resolve-base.sh b/plugins/compound-engineering/skills/ce-review/references/resolve-base.sh index b5c3d24..433d42b 100644 --- a/plugins/compound-engineering/skills/ce-review/references/resolve-base.sh +++ b/plugins/compound-engineering/skills/ce-review/references/resolve-base.sh @@ -15,6 +15,7 @@ set -euo pipefail REVIEW_BASE_BRANCH="" PR_BASE_REPO="" +PR_BASE_REMOTE="" BASE_REF="" # Step 1: Try PR metadata (handles fork workflows) @@ -51,19 +52,37 @@ if [ -n "$REVIEW_BASE_BRANCH" ]; then if [ -n "$PR_BASE_REPO" ]; then PR_BASE_REMOTE=$(git remote -v | awk "index(\$2, \"github.com:$PR_BASE_REPO\") || index(\$2, \"github.com/$PR_BASE_REPO\") {print \$1; exit}") if [ -n "$PR_BASE_REMOTE" ]; then - git rev-parse --verify "$PR_BASE_REMOTE/$REVIEW_BASE_BRANCH" >/dev/null 2>&1 || git fetch --no-tags "$PR_BASE_REMOTE" "$REVIEW_BASE_BRANCH" 2>/dev/null || true + git rev-parse --verify "$PR_BASE_REMOTE/$REVIEW_BASE_BRANCH" >/dev/null 2>&1 || git fetch --no-tags "$PR_BASE_REMOTE" "$REVIEW_BASE_BRANCH:refs/remotes/$PR_BASE_REMOTE/$REVIEW_BASE_BRANCH" 2>/dev/null || git fetch --no-tags "$PR_BASE_REMOTE" "$REVIEW_BASE_BRANCH" 2>/dev/null || true BASE_REF=$(git rev-parse --verify "$PR_BASE_REMOTE/$REVIEW_BASE_BRANCH" 2>/dev/null || true) fi fi if [ -z "$BASE_REF" ]; then - git rev-parse --verify "origin/$REVIEW_BASE_BRANCH" >/dev/null 2>&1 || git fetch --no-tags origin "$REVIEW_BASE_BRANCH" 2>/dev/null || true - BASE_REF=$(git rev-parse --verify "origin/$REVIEW_BASE_BRANCH" 2>/dev/null || git rev-parse --verify "$REVIEW_BASE_BRANCH" 2>/dev/null || true) + # Only try origin if it exists as a remote; otherwise skip to avoid + # confusing errors in fork setups where origin points at the user's fork. + if git remote get-url origin >/dev/null 2>&1; then + git rev-parse --verify "origin/$REVIEW_BASE_BRANCH" >/dev/null 2>&1 || git fetch --no-tags origin "$REVIEW_BASE_BRANCH:refs/remotes/origin/$REVIEW_BASE_BRANCH" 2>/dev/null || git fetch --no-tags origin "$REVIEW_BASE_BRANCH" 2>/dev/null || true + BASE_REF=$(git rev-parse --verify "origin/$REVIEW_BASE_BRANCH" 2>/dev/null || true) + fi + # Fall back to a bare local ref only if remote resolution failed + if [ -z "$BASE_REF" ]; then + BASE_REF=$(git rev-parse --verify "$REVIEW_BASE_BRANCH" 2>/dev/null || true) + fi fi fi # Compute merge-base if [ -n "$BASE_REF" ]; then BASE=$(git merge-base HEAD "$BASE_REF" 2>/dev/null) || BASE="" + if [ -z "$BASE" ] && [ "$(git rev-parse --is-shallow-repository 2>/dev/null || echo false)" = "true" ]; then + if git remote get-url origin >/dev/null 2>&1; then + git fetch --no-tags --unshallow origin 2>/dev/null || true + BASE=$(git merge-base HEAD "$BASE_REF" 2>/dev/null) || BASE="" + fi + if [ -z "$BASE" ] && [ -n "$PR_BASE_REMOTE" ] && [ "$PR_BASE_REMOTE" != "origin" ]; then + git fetch --no-tags --unshallow "$PR_BASE_REMOTE" 2>/dev/null || true + BASE=$(git merge-base HEAD "$BASE_REF" 2>/dev/null) || BASE="" + fi + fi else BASE="" fi diff --git a/tests/resolve-base-script.test.ts b/tests/resolve-base-script.test.ts new file mode 100644 index 0000000..fbcf968 --- /dev/null +++ b/tests/resolve-base-script.test.ts @@ -0,0 +1,300 @@ +import { describe, expect, test } from "bun:test" +import { promises as fs } from "fs" +import os from "os" +import path from "path" +import { pathToFileURL } from "url" + +const gitEnv = { + ...process.env, + GIT_AUTHOR_NAME: "Test", + GIT_AUTHOR_EMAIL: "test@example.com", + GIT_COMMITTER_NAME: "Test", + GIT_COMMITTER_EMAIL: "test@example.com", +} + +const resolveBaseScript = path.join( + import.meta.dir, + "..", + "plugins", + "compound-engineering", + "skills", + "ce-review", + "references", + "resolve-base.sh", +) + +type RunResult = { + exitCode: number + stderr: string + stdout: string +} + +async function runCommand( + cmd: string[], + cwd: string, + env?: NodeJS.ProcessEnv, +): Promise { + const proc = Bun.spawn(cmd, { + cwd, + env: env ?? process.env, + stderr: "pipe", + stdout: "pipe", + }) + + const [exitCode, stdout, stderr] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]) + + return { exitCode, stderr, stdout } +} + +async function runGit(args: string[], cwd: string, env?: NodeJS.ProcessEnv): Promise { + const result = await runCommand(["git", ...args], cwd, env ?? gitEnv) + if (result.exitCode !== 0) { + throw new Error( + `git ${args.join(" ")} failed (exit ${result.exitCode}).\nstdout: ${result.stdout}\nstderr: ${result.stderr}`, + ) + } + + return result.stdout.trim() +} + +async function initRepo(initialBranch = "main"): Promise { + const repoRoot = await fs.mkdtemp(path.join(os.tmpdir(), "resolve-base-repo-")) + await runGit(["init", "-b", initialBranch], repoRoot) + return repoRoot +} + +async function commitFile( + repoRoot: string, + relativePath: string, + content: string, + message: string, +): Promise { + const filePath = path.join(repoRoot, relativePath) + await fs.mkdir(path.dirname(filePath), { recursive: true }) + await fs.writeFile(filePath, content) + await runGit(["add", relativePath], repoRoot) + await runGit(["commit", "-m", message], repoRoot) + return runGit(["rev-parse", "HEAD"], repoRoot) +} + +async function writeExecutable(filePath: string, content: string): Promise { + await fs.writeFile(filePath, content) + await fs.chmod(filePath, 0o755) +} + +async function createStubBin(mode: "gh-fails" | "pr-metadata"): Promise { + const binDir = await fs.mkdtemp(path.join(os.tmpdir(), "resolve-base-bin-")) + + if (mode === "gh-fails") { + await writeExecutable(path.join(binDir, "gh"), "#!/usr/bin/env bash\nexit 1\n") + return binDir + } + + await writeExecutable( + path.join(binDir, "gh"), + `#!/usr/bin/env bash +set -euo pipefail +if [ "$#" -ge 2 ] && [ "$1" = "pr" ] && [ "$2" = "view" ]; then + printf '%s' '{"baseRefName":"main","url":"https://github.com/EveryInc/compound-engineering-plugin/pull/123"}' + exit 0 +fi +exit 1 +`, + ) + + await writeExecutable( + path.join(binDir, "jq"), + `#!/usr/bin/env bun +const args = process.argv.slice(2).filter((arg) => arg !== "-r") +const query = args[args.length - 1] ?? "" +const input = await new Response(Bun.stdin.stream()).text() +const data = input.trim() ? JSON.parse(input) : {} + +let output = "" +if (query === ".baseRefName // empty") { + output = data.baseRefName ?? "" +} else if (query === ".url // empty") { + output = data.url ?? "" +} else if (query === ".defaultBranchRef.name") { + output = data.defaultBranchRef?.name ?? "" +} else { + console.error(\`unsupported jq query: \${query}\`) + process.exit(1) +} + +process.stdout.write(String(output)) +`, + ) + + return binDir +} + +async function runResolveBase( + repoRoot: string, + stubBin: string, + extraEnv?: NodeJS.ProcessEnv, +): Promise { + return runCommand(["bash", resolveBaseScript], repoRoot, { + ...gitEnv, + PATH: `${stubBin}:${process.env.PATH ?? ""}`, + ...extraEnv, + }) +} + +describe("resolve-base.sh", () => { + test("prefers the PR base remote from gh metadata over origin", async () => { + const repoRoot = await initRepo() + const initialSha = await commitFile(repoRoot, "history.txt", "a\n", "initial") + const upstreamMainSha = await commitFile(repoRoot, "history.txt", "b\n", "main advance") + + await runGit(["checkout", "-b", "feature"], repoRoot) + await commitFile(repoRoot, "feature.txt", "feature\n", "feature change") + + await runGit(["checkout", "-b", "fork-main", initialSha], repoRoot) + const forkMainSha = await commitFile(repoRoot, "fork.txt", "fork\n", "fork main diverges") + await runGit(["checkout", "feature"], repoRoot) + + await runGit(["remote", "add", "origin", "git@github.com:someone/fork.git"], repoRoot) + await runGit( + ["remote", "add", "upstream", "git@github.com:EveryInc/compound-engineering-plugin.git"], + repoRoot, + ) + await runGit(["update-ref", "refs/remotes/origin/main", forkMainSha], repoRoot) + await runGit(["update-ref", "refs/remotes/upstream/main", upstreamMainSha], repoRoot) + + const stubBin = await createStubBin("pr-metadata") + const result = await runResolveBase(repoRoot, stubBin) + + expect(result.exitCode).toBe(0) + expect(result.stdout.trim()).toBe(`BASE:${upstreamMainSha}`) + }) + + test("falls back to a local base branch when origin is absent", async () => { + const repoRoot = await initRepo() + await commitFile(repoRoot, "history.txt", "a\n", "initial") + const mainSha = await commitFile(repoRoot, "history.txt", "b\n", "main advance") + + await runGit(["checkout", "-b", "feature"], repoRoot) + await commitFile(repoRoot, "feature.txt", "feature\n", "feature change") + + const stubBin = await createStubBin("gh-fails") + const result = await runResolveBase(repoRoot, stubBin) + + expect(result.exitCode).toBe(0) + expect(result.stdout.trim()).toBe(`BASE:${mainSha}`) + }) + + test("resolves against origin/HEAD in a detached shallow checkout", async () => { + const seedRepo = await initRepo() + await commitFile(seedRepo, "history.txt", "a\n", "initial") + const mainSha = await commitFile(seedRepo, "history.txt", "b\n", "main advance") + + await runGit(["checkout", "-b", "feature"], seedRepo) + const featureSha = await commitFile(seedRepo, "feature.txt", "feature\n", "feature change") + await runGit(["checkout", "main"], seedRepo) + + const bareRepo = await fs.mkdtemp(path.join(os.tmpdir(), "resolve-base-remote-")) + await runGit(["init", "--bare", bareRepo], seedRepo) + const bareUrl = pathToFileURL(bareRepo).toString() + await runGit(["remote", "add", "origin", bareUrl], seedRepo) + await runGit(["push", "origin", "main", "feature"], seedRepo) + + const checkoutRoot = await fs.mkdtemp(path.join(os.tmpdir(), "resolve-base-checkout-")) + await runCommand(["git", "clone", "--depth", "1", bareUrl, checkoutRoot], os.tmpdir(), gitEnv) + await runGit(["config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"], checkoutRoot) + await runGit( + ["fetch", "--depth", "1", "origin", "main:refs/remotes/origin/main", "feature:refs/remotes/origin/feature"], + checkoutRoot, + ) + await runGit(["symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/main"], checkoutRoot) + await runGit(["checkout", "--detach", "origin/feature"], checkoutRoot) + + const originMain = await runGit(["rev-parse", "--verify", "origin/main"], checkoutRoot) + expect(originMain).toBe(mainSha) + + const originFeature = await runGit(["rev-parse", "--verify", "origin/feature"], checkoutRoot) + expect(originFeature).toBe(featureSha) + + const originHead = await runGit( + ["symbolic-ref", "--quiet", "--short", "refs/remotes/origin/HEAD"], + checkoutRoot, + ) + expect(originHead).toBe("origin/main") + + const stubBin = await createStubBin("gh-fails") + const result = await runResolveBase(checkoutRoot, stubBin) + + expect(result.exitCode).toBe(0) + expect(result.stdout.trim()).toBe(`BASE:${mainSha}`) + }) + + test("unshallows the PR base remote in a detached shallow checkout", async () => { + const upstreamSeed = await initRepo() + const initialSha = await commitFile(upstreamSeed, "history.txt", "a\n", "initial") + const upstreamMainSha = await commitFile(upstreamSeed, "history.txt", "b\n", "upstream main") + + await runGit(["checkout", "-b", "feature"], upstreamSeed) + const featureSha = await commitFile(upstreamSeed, "feature.txt", "feature\n", "feature change") + + const forkSeed = await initRepo() + await commitFile(forkSeed, "history.txt", "a\n", "initial") + const forkMainSha = await commitFile(forkSeed, "fork.txt", "fork\n", "fork main diverges") + + const remotesRoot = await fs.mkdtemp(path.join(os.tmpdir(), "resolve-base-remotes-")) + const upstreamBare = path.join( + remotesRoot, + "github.com", + "EveryInc", + "compound-engineering-plugin.git", + ) + await fs.mkdir(path.dirname(upstreamBare), { recursive: true }) + await runGit(["init", "--bare", upstreamBare], upstreamSeed) + const upstreamUrl = pathToFileURL(upstreamBare).toString() + await runGit(["remote", "add", "origin", upstreamUrl], upstreamSeed) + await runGit(["push", "origin", "main", "feature"], upstreamSeed) + + const forkBare = path.join(remotesRoot, "github.com", "someone", "fork.git") + await fs.mkdir(path.dirname(forkBare), { recursive: true }) + await runGit(["init", "--bare", forkBare], forkSeed) + const forkUrl = pathToFileURL(forkBare).toString() + await runGit(["remote", "add", "origin", forkUrl], forkSeed) + await runGit(["push", "origin", "main"], forkSeed) + + const checkoutParent = await fs.mkdtemp(path.join(os.tmpdir(), "resolve-base-pr-shallow-")) + const checkoutRoot = path.join(checkoutParent, "checkout") + await runCommand( + ["git", "clone", "--depth", "1", "--branch", "feature", upstreamUrl, checkoutRoot], + os.tmpdir(), + gitEnv, + ) + await runGit(["checkout", "--detach", featureSha], checkoutRoot) + await runGit(["remote", "rename", "origin", "upstream"], checkoutRoot) + await runGit(["remote", "add", "origin", forkUrl], checkoutRoot) + await runGit(["fetch", "--depth", "1", "origin", "main"], checkoutRoot) + await runGit(["update-ref", "refs/remotes/origin/main", forkMainSha], checkoutRoot) + await runGit(["branch", "-D", "feature"], checkoutRoot) + + const stubBin = await createStubBin("pr-metadata") + const result = await runResolveBase(checkoutRoot, stubBin) + + expect(result.exitCode).toBe(0) + expect(result.stdout.trim()).toBe(`BASE:${upstreamMainSha}`) + }) + + test("emits ERROR output when no base branch can be resolved", async () => { + const repoRoot = await initRepo("scratch") + await commitFile(repoRoot, "history.txt", "a\n", "initial") + + const stubBin = await createStubBin("gh-fails") + const result = await runResolveBase(repoRoot, stubBin) + + expect(result.exitCode).toBe(0) + expect(result.stdout.trim()).toBe( + "ERROR:Unable to resolve review base branch locally. Fetch the base branch and rerun, or provide a PR number so the review scope can be determined from PR metadata.", + ) + }) +})