diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468ad..ea10a2238 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -262,7 +262,8 @@ function buildNativeReviewTarget(target) { } if (target.mode === "branch") { - return { type: "baseBranch", branch: target.baseRef }; + const branch = target.nativeBaseRef ?? (target.baseRef.startsWith("-") ? target.baseCommit : target.baseRef); + return { type: "baseBranch", branch }; } return null; diff --git a/plugins/codex/scripts/lib/git.mjs b/plugins/codex/scripts/lib/git.mjs index c401f0ca1..1e0b3fe01 100644 --- a/plugins/codex/scripts/lib/git.mjs +++ b/plugins/codex/scripts/lib/git.mjs @@ -66,12 +66,12 @@ function measureCombinedGitOutputBytes(cwd, argSets, maxBytes) { return totalBytes; } -function buildBranchComparison(cwd, baseRef) { - const mergeBase = gitChecked(cwd, ["merge-base", "HEAD", baseRef]).stdout.trim(); +function buildBranchComparison(cwd, baseCommit) { + const mergeBase = gitChecked(cwd, ["merge-base", "HEAD", baseCommit]).stdout.trim(); return { mergeBase, commitRange: `${mergeBase}..HEAD`, - reviewRange: `${baseRef}...HEAD` + reviewRange: `${baseCommit}...HEAD` }; } @@ -91,12 +91,15 @@ export function getRepoRoot(cwd) { return gitChecked(cwd, ["rev-parse", "--show-toplevel"]).stdout.trim(); } -export function detectDefaultBranch(cwd) { +function detectDefaultBranchTarget(cwd) { const symbolic = git(cwd, ["symbolic-ref", "refs/remotes/origin/HEAD"]); if (symbolic.status === 0) { const remoteHead = symbolic.stdout.trim(); if (remoteHead.startsWith("refs/remotes/origin/")) { - return remoteHead.replace("refs/remotes/origin/", ""); + return { + baseRef: remoteHead.replace("refs/remotes/origin/", ""), + commitRef: remoteHead + }; } } @@ -104,17 +107,21 @@ export function detectDefaultBranch(cwd) { for (const candidate of candidates) { const local = git(cwd, ["show-ref", "--verify", "--quiet", `refs/heads/${candidate}`]); if (local.status === 0) { - return candidate; + return { baseRef: candidate, commitRef: candidate }; } const remote = git(cwd, ["show-ref", "--verify", "--quiet", `refs/remotes/origin/${candidate}`]); if (remote.status === 0) { - return `origin/${candidate}`; + return { baseRef: `origin/${candidate}`, commitRef: `refs/remotes/origin/${candidate}` }; } } throw new Error("Unable to detect the repository default branch. Pass --base or use --scope working-tree."); } +export function detectDefaultBranch(cwd) { + return detectDefaultBranchTarget(cwd).baseRef; +} + export function getCurrentBranch(cwd) { return gitChecked(cwd, ["branch", "--show-current"]).stdout.trim() || "HEAD"; } @@ -132,23 +139,45 @@ export function getWorkingTreeState(cwd) { }; } +function ensureCommitRef(cwd, baseRef) { + const resolved = git(cwd, ["rev-parse", "--verify", "--quiet", "--end-of-options", baseRef]); + if (resolved.error) { + throw resolved.error; + } + if (resolved.status !== 0) { + throw new Error(`base ${baseRef} not found in this repository`); + } + + const commit = git(cwd, ["rev-parse", "--verify", "--quiet", "--end-of-options", `${resolved.stdout.trim()}^{commit}`]); + if (commit.error) { + throw commit.error; + } + if (commit.status !== 0) { + throw new Error(`base ${baseRef} not found in this repository`); + } + return commit.stdout.trim(); +} + export function resolveReviewTarget(cwd, options = {}) { ensureGitRepository(cwd); const requestedScope = options.scope ?? "auto"; const baseRef = options.base ?? null; - const state = getWorkingTreeState(cwd); const supportedScopes = new Set(["auto", "working-tree", "branch"]); if (baseRef) { + const baseCommit = ensureCommitRef(cwd, baseRef); return { mode: "branch", label: `branch diff against ${baseRef}`, baseRef, + baseCommit, explicit: true }; } + const state = getWorkingTreeState(cwd); + if (requestedScope === "working-tree") { return { mode: "working-tree", @@ -164,11 +193,14 @@ export function resolveReviewTarget(cwd, options = {}) { } if (requestedScope === "branch") { - const detectedBase = detectDefaultBranch(cwd); + const detected = detectDefaultBranchTarget(cwd); + const baseCommit = ensureCommitRef(cwd, detected.commitRef); return { mode: "branch", - label: `branch diff against ${detectedBase}`, - baseRef: detectedBase, + label: `branch diff against ${detected.baseRef}`, + baseRef: detected.baseRef, + baseCommit, + nativeBaseRef: detected.baseRef.startsWith("-") ? baseCommit : detected.commitRef, explicit: true }; } @@ -181,11 +213,14 @@ export function resolveReviewTarget(cwd, options = {}) { }; } - const detectedBase = detectDefaultBranch(cwd); + const detected = detectDefaultBranchTarget(cwd); + const baseCommit = ensureCommitRef(cwd, detected.commitRef); return { mode: "branch", - label: `branch diff against ${detectedBase}`, - baseRef: detectedBase, + label: `branch diff against ${detected.baseRef}`, + baseRef: detected.baseRef, + baseCommit, + nativeBaseRef: detected.baseRef.startsWith("-") ? baseCommit : detected.commitRef, explicit: false }; } @@ -261,7 +296,7 @@ function collectWorkingTreeContext(cwd, state, options = {}) { function collectBranchContext(cwd, baseRef, options = {}) { const includeDiff = options.includeDiff !== false; - const comparison = options.comparison ?? buildBranchComparison(cwd, baseRef); + const comparison = options.comparison ?? buildBranchComparison(cwd, ensureCommitRef(cwd, baseRef)); const currentBranch = getCurrentBranch(cwd); const changedFiles = gitChecked(cwd, ["diff", "--name-only", comparison.commitRange]).stdout.trim().split("\n").filter(Boolean); const logOutput = gitChecked(cwd, ["log", "--oneline", "--decorate", comparison.commitRange]).stdout.trim(); @@ -322,7 +357,8 @@ export function collectReviewContext(cwd, target, options = {}) { diffBytes <= maxInlineDiffBytes); details = collectWorkingTreeContext(repoRoot, state, { includeDiff }); } else { - const comparison = buildBranchComparison(repoRoot, target.baseRef); + const baseCommit = target.baseCommit ?? ensureCommitRef(repoRoot, target.baseRef); + const comparison = buildBranchComparison(repoRoot, baseCommit); const fileCount = gitChecked(repoRoot, ["diff", "--name-only", comparison.commitRange]).stdout.trim().split("\n").filter(Boolean).length; diffBytes = measureGitOutputBytes( repoRoot, diff --git a/tests/git.test.mjs b/tests/git.test.mjs index 5b5c266ee..1d9c08d06 100644 --- a/tests/git.test.mjs +++ b/tests/git.test.mjs @@ -84,6 +84,75 @@ test("resolveReviewTarget honors explicit base overrides", () => { assert.equal(target.baseRef, "main"); }); +test("resolveReviewTarget accepts commit-message searches as explicit bases", () => { + const cwd = makeTempDir(); + initGitRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('base');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "release baseline"], { cwd, shell: false }); + const baseCommit = run("git", ["rev-parse", "HEAD"], { cwd, shell: false }).stdout.trim(); + run("git", ["checkout", "-b", "feature/test"], { cwd }); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('feature');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "feature"], { cwd }); + + const baseRef = ":/release baseline"; + const target = resolveReviewTarget(cwd, { base: baseRef }); + const context = collectReviewContext(cwd, target); + + assert.equal(target.mode, "branch"); + assert.equal(target.baseRef, baseRef); + assert.equal(target.baseCommit, baseCommit); + assert.equal(context.comparison.mergeBase, baseCommit); + assert.equal(context.comparison.reviewRange, `${baseCommit}...HEAD`); + assert.deepEqual(context.changedFiles, ["app.js"]); + assert.match(context.summary, /against :\/release baseline/); + assert.match(context.content, /-console\.log\('base'\);/); + assert.match(context.content, /\+console\.log\('feature'\);/); +}); + +test("collectReviewContext uses the canonical commit for dash-leading explicit bases", () => { + const cwd = makeTempDir(); + initGitRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('base');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "base"], { cwd }); + const baseCommit = run("git", ["rev-parse", "HEAD"], { cwd, shell: false }).stdout.trim(); + run("git", ["update-ref", "refs/heads/-release-baseline", baseCommit], { cwd, shell: false }); + run("git", ["checkout", "-b", "feature/test"], { cwd }); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('feature');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "feature"], { cwd }); + + const baseRef = "-release-baseline"; + const target = resolveReviewTarget(cwd, { base: baseRef }); + const context = collectReviewContext(cwd, target); + + assert.equal(target.baseRef, baseRef); + assert.equal(target.baseCommit, baseCommit); + assert.equal(context.comparison.mergeBase, baseCommit); + assert.equal(context.comparison.reviewRange, `${baseCommit}...HEAD`); + assert.deepEqual(context.changedFiles, ["app.js"]); + assert.match(context.summary, /against -release-baseline/); +}); + +test("resolveReviewTarget rejects missing or non-commit explicit bases", () => { + const cwd = makeTempDir(); + initGitRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v1');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "init"], { cwd }); + + assert.throws( + () => resolveReviewTarget(cwd, { base: "missing-base" }), + /base missing-base not found in this repository/ + ); + assert.throws( + () => resolveReviewTarget(cwd, { base: "HEAD:app.js" }), + /base HEAD:app.js not found in this repository/ + ); +}); + test("resolveReviewTarget requires an explicit base when no default branch can be inferred", () => { const cwd = makeTempDir(); initGitRepo(cwd); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..4a58e3694 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -356,16 +356,122 @@ test("review accepts the quoted raw argument style for built-in base-branch revi run("git", ["commit", "-m", "init"], { cwd: repo }); fs.writeFileSync(path.join(repo, "src", "app.js"), "export const value = 2;\n"); - const result = run("node", [SCRIPT, "review", "--base main"], { + const result = run(process.execPath, [SCRIPT, "review", "--base main"], { cwd: repo, env: buildEnv(binDir) }); - assert.equal(result.status, 0); + assert.equal(result.status, 0, result.stderr); assert.match(result.stdout, /Reviewed changes against main/); assert.match(result.stdout, /No material issues found/); }); +test("review sends dash-leading explicit bases to app-server as canonical commits", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "app.js"), "console.log('base');\n"); + run("git", ["add", "app.js"], { cwd: repo }); + run("git", ["commit", "-m", "base"], { cwd: repo }); + const baseCommit = run("git", ["rev-parse", "HEAD"], { cwd: repo, shell: false }).stdout.trim(); + run("git", ["update-ref", "refs/heads/-release-baseline", baseCommit], { cwd: repo, shell: false }); + run("git", ["checkout", "-b", "feature/test"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "app.js"), "console.log('feature');\n"); + run("git", ["add", "app.js"], { cwd: repo }); + run("git", ["commit", "-m", "feature"], { cwd: repo }); + + const result = run(process.execPath, [SCRIPT, "review", "--base", "-release-baseline"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /branch diff against -release-baseline/i); + assert.match(result.stdout, new RegExp(`Reviewed changes against ${baseCommit}\\.`)); +}); + +test("review sends dash-leading detected bases to app-server as canonical commits", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "app.js"), "console.log('base');\n"); + run("git", ["add", "app.js"], { cwd: repo }); + run("git", ["commit", "-m", "base"], { cwd: repo }); + const baseCommit = run("git", ["rev-parse", "HEAD"], { cwd: repo, shell: false }).stdout.trim(); + run("git", ["update-ref", "refs/remotes/origin/-release-baseline", baseCommit], { cwd: repo, shell: false }); + run("git", ["symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/-release-baseline"], { + cwd: repo, + shell: false + }); + run("git", ["checkout", "-b", "feature/test"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "app.js"), "console.log('feature');\n"); + run("git", ["add", "app.js"], { cwd: repo }); + run("git", ["commit", "-m", "feature"], { cwd: repo }); + + const result = run(process.execPath, [SCRIPT, "review", "--scope", "branch"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /branch diff against -release-baseline/i); + assert.match(result.stdout, new RegExp(`Reviewed changes against ${baseCommit}\\.`)); +}); + +test("review preserves symbolic remote defaults without local branches", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "app.js"), "console.log('base');\n"); + run("git", ["add", "app.js"], { cwd: repo }); + run("git", ["commit", "-m", "base"], { cwd: repo }); + const baseCommit = run("git", ["rev-parse", "HEAD"], { cwd: repo, shell: false }).stdout.trim(); + run("git", ["update-ref", "refs/remotes/origin/main", baseCommit], { cwd: repo, shell: false }); + run("git", ["symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/main"], { + cwd: repo, + shell: false + }); + run("git", ["checkout", "--detach"], { cwd: repo, shell: false }); + run("git", ["branch", "-D", "main"], { cwd: repo, shell: false }); + fs.writeFileSync(path.join(repo, "app.js"), "console.log('feature');\n"); + run("git", ["add", "app.js"], { cwd: repo }); + run("git", ["commit", "-m", "feature"], { cwd: repo }); + + for (const args of [["review", "--scope", "branch"], ["review"]]) { + const result = run(process.execPath, [SCRIPT, ...args], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /branch diff against main/i); + assert.match(result.stdout, /Reviewed changes against refs\/remotes\/origin\/main\./); + } +}); + +test("review rejects a missing explicit base before starting Codex", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const fakeCodexState = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + const result = run(process.execPath, [SCRIPT, "review", "--base", "missing-base"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.notEqual(result.status, 0); + assert.match(result.stderr, /base missing-base not found in this repository/); + assert.equal(fs.existsSync(fakeCodexState), false); +}); + test("adversarial review renders structured findings over app-server turn/start", () => { const repo = makeTempDir(); const binDir = makeTempDir();