diff --git a/scripts/lib/git.mjs b/scripts/lib/git.mjs index 4375391..5b4495a 100644 --- a/scripts/lib/git.mjs +++ b/scripts/lib/git.mjs @@ -233,6 +233,20 @@ export function resolveReviewTarget(cwd, options = {}) { const state = getWorkingTreeState(cwd); const supportedScopes = new Set(["auto", "working-tree", "branch"]); + if (!supportedScopes.has(requestedScope)) { + throw new Error( + `Unsupported review scope "${requestedScope}". Use one of: auto, working-tree, branch, or pass --base .` + ); + } + + // A working-tree review always compares against HEAD, so a base ref would be + // ignored. Refuse the pair instead of silently reviewing a different diff. + if (baseRef && requestedScope === "working-tree") { + throw new Error( + `--base cannot be combined with --scope working-tree: a working-tree review compares against HEAD. Drop --base to review uncommitted changes, or drop --scope to review the branch against ${baseRef}.` + ); + } + if (baseRef) { return { mode: "branch", @@ -250,12 +264,6 @@ export function resolveReviewTarget(cwd, options = {}) { }; } - if (!supportedScopes.has(requestedScope)) { - throw new Error( - `Unsupported review scope "${requestedScope}". Use one of: auto, working-tree, branch, or pass --base .` - ); - } - if (requestedScope === "branch") { const detectedBase = detectDefaultBranch(cwd); return { diff --git a/skills/adversarial-review/SKILL.md b/skills/adversarial-review/SKILL.md index 07878e3..e8dd8f2 100644 --- a/skills/adversarial-review/SKILL.md +++ b/skills/adversarial-review/SKILL.md @@ -53,6 +53,7 @@ Argument handling: - `$cc:adversarial-review` uses the same review target selection as `$cc:review`. - It supports working-tree review, branch review, and `--base `. - It does not support `--scope staged` or `--scope unstaged`. +- Do not combine `--base ` with `--scope working-tree`; the companion rejects that pair because a working-tree review always compares against `HEAD`. - Unlike `$cc:review`, it can still take extra focus text after the flags. - The companion review process itself always runs in the foreground. Background mode only changes how Codex launches that command. - For the detailed execution contract, treat the internal runtime reference at `../../internal-skills/review-runtime/runtime.md` as supporting guidance only. It is an internal reference document, not a public skill to invoke. diff --git a/skills/review/SKILL.md b/skills/review/SKILL.md index 37e2c2c..e98e5c7 100644 --- a/skills/review/SKILL.md +++ b/skills/review/SKILL.md @@ -28,6 +28,7 @@ Rules: - Before launching the review, stay in read-only inspection mode: inspect git status and diff stats only, then ask at most one user question about whether to wait or run in background. - Preserve the user's review scope flags exactly. - Do not accept staged-only or unstaged-only review modes. +- `--base ` reviews the branch against that ref. Do not combine it with `--scope working-tree`; the companion rejects that pair because a working-tree review always compares against `HEAD`. - Do not add extra review instructions or focus text. Route those requests to `$cc:adversarial-review`. Execution mode rules: diff --git a/tests/git.test.mjs b/tests/git.test.mjs index b08fbea..284b632 100644 --- a/tests/git.test.mjs +++ b/tests/git.test.mjs @@ -9,7 +9,7 @@ import os from "node:os"; import path from "node:path"; import { spawnSync } from "node:child_process"; -import { collectReviewContext, getWorkingTreeFingerprint } from "../scripts/lib/git.mjs"; +import { collectReviewContext, getWorkingTreeFingerprint, resolveReviewTarget } from "../scripts/lib/git.mjs"; const tempRepos = []; @@ -215,3 +215,52 @@ describe("collectReviewContext", () => { assert.equal(typeof fingerprint.unstagedDiffHash, "string"); }); }); + +describe("resolveReviewTarget", () => { + function createRepoWithStagedChange() { + const repo = createRepo(); + fs.writeFileSync(path.join(repo, "app.js"), "export const value = 1;\n", "utf8"); + runGit(repo, ["add", "app.js"]); + runGit(repo, ["commit", "-m", "initial"]); + fs.writeFileSync(path.join(repo, "app.js"), "export const value = 2;\n", "utf8"); + runGit(repo, ["add", "app.js"]); + return repo; + } + + // A branch review of main against main is empty, so silently honoring --base + // here would report a clean review of uncommitted work it never looked at. + it("refuses --base with --scope working-tree instead of dropping the scope", () => { + const repo = createRepoWithStagedChange(); + + assert.throws( + () => resolveReviewTarget(repo, { base: "main", scope: "working-tree" }), + /--base cannot be combined with --scope working-tree/ + ); + }); + + it("rejects an unknown scope even when --base is given", () => { + const repo = createRepoWithStagedChange(); + + assert.throws( + () => resolveReviewTarget(repo, { base: "main", scope: "staged" }), + /Unsupported review scope "staged"/ + ); + }); + + it("still reviews the branch against --base with the auto or branch scope", () => { + const repo = createRepoWithStagedChange(); + + for (const scope of [undefined, "auto", "branch"]) { + const target = resolveReviewTarget(repo, { base: "main", scope }); + assert.equal(target.mode, "branch"); + assert.equal(target.baseRef, "main"); + } + }); + + it("still reviews the working tree when only --scope working-tree is given", () => { + const repo = createRepoWithStagedChange(); + + const target = resolveReviewTarget(repo, { scope: "working-tree" }); + assert.equal(target.mode, "working-tree"); + }); +});