john/claude-engineering-plugin
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(review): harden ce-review base resolution (#452)

Browse Source
This commit is contained in:
Trevin Chow
2026-03-30 01:10:45 -07:00
committed by GitHub
parent e872e15efa
commit 638b38abd2
5 changed files with 331 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
plugins/compound-engineering/agents/review/previous-comments-reviewer.md
View File

@@ -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 `<pr-context>` block. If `<pr-context>` 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 `<pr-context>` block. Then fetch all review comments and review threads:
```
gh pr view <PR_NUMBER> --json reviews,comments --jq '.reviews[].body, .comments[].body'

8
plugins/compound-engineering/skills/ce-review/SKILL.md
View File

@@ -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 `<stand
Persona sub-agents do focused, scoped work and should use cheaper/faster models to reduce cost and latency. The orchestrator itself stays on the default (most capable) model.
| Platform | Persona agent model | How to set |
|----------|-------------------|------------|
| **Claude Code** | Haiku | Pass `model: "haiku"` in the Agent tool call |
| **Codex** | GPT-4o mini | Set the model parameter to `gpt-4o-mini` in the agent spawn config |
| **Other platforms** | Cheapest capable model available | Use the platform's equivalent of a fast/cheap tier |
Use the platform's cheapest capable model for all persona and CE sub-agents. In Claude Code, pass `model: "haiku"` in the Agent tool call. On other platforms, use the equivalent fast/cheap tier (e.g., `gpt-4o-mini` in Codex). If the platform has no model override mechanism or the available model names are unknown, omit the model parameter and let agents inherit the default -- a working review on the parent model is better than a broken dispatch from an unrecognized model name.
CE always-on agents (agent-native-reviewer, learnings-researcher) and CE conditional agents (schema-drift-detector, deployment-verification-agent) also use the cheaper model tier since they perform scoped, focused work.

2
plugins/compound-engineering/skills/ce-review/references/persona-catalog.md
View File

@@ -34,7 +34,7 @@ Spawned when the orchestrator identifies relevant patterns in the diff. The orch
| `data-migrations` | `compound-engineering:review:data-migrations-reviewer` | Migration files, schema changes, backfill scripts, data transformations |
| `reliability` | `compound-engineering:review:reliability-reviewer` | Error handling, retry logic, circuit breakers, timeouts, background jobs, async handlers, health checks |
| `adversarial` | `compound-engineering:review:adversarial-reviewer` | Diff has >=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)

25
plugins/compound-engineering/skills/ce-review/references/resolve-base.sh
View File

@@ -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

300
tests/resolve-base-script.test.ts Normal file
View File

@@ -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<RunResult> {
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<string> {
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<string> {
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<string> {
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<void> {
await fs.writeFile(filePath, content)
await fs.chmod(filePath, 0o755)
}
async function createStubBin(mode: "gh-fails" | "pr-metadata"): Promise<string> {
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<RunResult> {
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.",
)
})
})