diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 35222fd5..bc430976 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -21,7 +21,7 @@ import { runAppServerTurn } from "./lib/codex.mjs"; import { readStdinIfPiped } from "./lib/fs.mjs"; -import { collectReviewContext, ensureGitRepository, resolveReviewTarget } from "./lib/git.mjs"; +import { collectReviewContext, detectVcs, ensureGitRepository, resolveReviewTarget } from "./lib/vcs.mjs"; import { binaryAvailable, terminateProcessTree } from "./lib/process.mjs"; import { loadPromptTemplate, interpolateTemplate } from "./lib/prompts.mjs"; import { @@ -246,6 +246,30 @@ function buildAdversarialReviewPrompt(context, focusText) { }); } +function buildJjNativeReviewInstructions(target) { + if (target.mode === "working-tree") { + return [ + "Review the current changes in this repository.", + "This repository uses Jujutsu (jj), not git.", + "When reasoning about the change, use jj concepts such as revsets, bookmarks, and the working copy commit." + ].join(" "); + } + + if (target.mode === "branch") { + return [ + `Review only the changes introduced on the current branch relative to the common ancestor (fork point) of ${target.baseRef} and @ in this repository.`, + "This repository uses Jujutsu (jj), not git.", + "When reasoning about the change, use jj concepts such as revsets, bookmarks, and the working copy commit." + ].join(" "); + } + + return [ + "Review the requested changes in this repository.", + "This repository uses Jujutsu (jj), not git.", + "When reasoning about the change, use jj concepts such as revsets, bookmarks, and the working copy commit." + ].join(" "); +} + function ensureCodexAvailable(cwd) { const availability = getCodexAvailability(cwd); if (!availability.available) { @@ -253,7 +277,14 @@ function ensureCodexAvailable(cwd) { } } -function buildNativeReviewTarget(target) { +function buildNativeReviewTarget(cwd, target) { + if (detectVcs(cwd) === "jj") { + return { + type: "custom", + instructions: buildJjNativeReviewInstructions(target) + }; + } + if (target.mode === "working-tree") { return { type: "uncommittedChanges" }; } @@ -265,14 +296,14 @@ function buildNativeReviewTarget(target) { return null; } -function validateNativeReviewRequest(target, focusText) { +function validateNativeReviewRequest(cwd, target, focusText) { if (focusText.trim()) { throw new Error( `\`/codex:review\` now maps directly to the built-in reviewer and does not support custom focus text. Retry with \`/codex:adversarial-review ${focusText.trim()}\` for focused review instructions.` ); } - const nativeTarget = buildNativeReviewTarget(target); + const nativeTarget = buildNativeReviewTarget(cwd, target); if (!nativeTarget) { throw new Error("This `/codex:review` target is not supported by the built-in reviewer. Retry with `/codex:adversarial-review` for custom targeting."); } @@ -362,8 +393,12 @@ async function executeReviewRun(request) { }); const focusText = request.focusText?.trim() ?? ""; const reviewName = request.reviewName ?? "Review"; - if (reviewName === "Review") { - const reviewTarget = validateNativeReviewRequest(target, focusText); + // Plain review uses the native reviewer. Git repos can use structured review + // targets directly, while jj repos use the native custom target plus + // Jujutsu-specific instructions. + const useNativeReviewer = reviewName === "Review"; + if (useNativeReviewer) { + const reviewTarget = validateNativeReviewRequest(request.cwd, target, focusText); const result = await runAppServerReview(request.cwd, { target: reviewTarget, model: request.model, @@ -696,7 +731,7 @@ async function handleReviewCommand(argv, config) { scope: options.scope }); - config.validateRequest?.(target, focusText); + config.validateRequest?.(cwd, target, focusText); const metadata = buildReviewJobMetadata(config.reviewName, target); const job = createCompanionJob({ prefix: "review", diff --git a/plugins/codex/scripts/lib/jj.mjs b/plugins/codex/scripts/lib/jj.mjs new file mode 100644 index 00000000..340d2223 --- /dev/null +++ b/plugins/codex/scripts/lib/jj.mjs @@ -0,0 +1,313 @@ +import { binaryAvailable, formatCommandFailure, runCommand, runCommandChecked } from "./process.mjs"; + +const DEFAULT_INLINE_DIFF_MAX_FILES = 2; +const DEFAULT_INLINE_DIFF_MAX_BYTES = 256 * 1024; + +function jj(cwd, args, options = {}) { + return runCommand("jj", [ + "--no-pager", + "--color=never", + "--quiet", + ...args + ], { cwd, ...options }); +} + +function jjChecked(cwd, args, options = {}) { + return runCommandChecked("jj", [ + "--no-pager", + "--color=never", + "--quiet", + ...args + ], { cwd, ...options }); +} + + + +function normalizeMaxInlineFiles(value) { + const parsed = Number(value); + if (!Number.isFinite(parsed) || parsed < 0) { + return DEFAULT_INLINE_DIFF_MAX_FILES; + } + return Math.floor(parsed); +} + +function normalizeMaxInlineDiffBytes(value) { + const parsed = Number(value); + if (!Number.isFinite(parsed) || parsed < 0) { + return DEFAULT_INLINE_DIFF_MAX_BYTES; + } + return Math.floor(parsed); +} + +function formatSection(title, body) { + return [`## ${title}`, "", body.trim() ? body.trim() : "(none)", ""].join("\n"); +} + +function buildForkPointRevset(baseRef) { + return `fork_point((${baseRef}) | @)`; +} + +function buildAdversarialCollectionGuidance(options = {}) { + if (options.includeDiff !== false) { + return "Use the repository context below as primary evidence. This is a Jujutsu (jj) repository — if you need to run additional commands, use jj (not git)."; + } + + return "The repository context below is a lightweight summary (diff stat and changed file list). The full diff was too large to inline. Base your review on the provided summary, commit log, and changed file list. Do NOT attempt to run jj commands — the sandbox does not support them."; +} + +function measureJjOutputBytes(cwd, args, maxBytes) { + const result = jj(cwd, args, { maxBuffer: maxBytes + 1 }); + if (result.error && /** @type {NodeJS.ErrnoException} */ (result.error).code === "ENOBUFS") { + return maxBytes + 1; + } + if (result.error) { + throw result.error; + } + if (result.status !== 0) { + throw new Error(formatCommandFailure(result)); + } + return Buffer.byteLength(result.stdout, "utf8"); +} + +function collectWorkingTreeContext(cwd, state, options = {}) { + const includeDiff = options.includeDiff !== false; + const statusOutput = jjChecked(cwd, ["diff", "--summary"]).stdout.trim(); + const changedFiles = state.staged; + + if (includeDiff) { + const workingCopyDiff = jjChecked(cwd, ["diff", "--git"]).stdout; + return { + mode: "working-tree", + summary: `Reviewing ${state.staged.length} changed file(s) in working copy.`, + content: [ + formatSection("Git Status", statusOutput), + formatSection("Staged Diff", workingCopyDiff), + formatSection("Unstaged Diff", ""), + formatSection("Untracked Files", "") + ].join("\n"), + changedFiles + }; + } else { + const diffStat = jjChecked(cwd, ["diff", "--stat"]).stdout.trim(); + return { + mode: "working-tree", + summary: `Reviewing ${state.staged.length} changed file(s) in working copy.`, + content: [ + formatSection("Git Status", statusOutput), + formatSection("Staged Diff Stat", diffStat), + formatSection("Unstaged Diff Stat", ""), + formatSection("Changed Files", changedFiles.join("\n")), + formatSection("Untracked Files", "") + ].join("\n"), + changedFiles + }; + } +} + +function collectRangeContext(cwd, includeDiff, baseRef = "trunk()") { + const comparisonBase = buildForkPointRevset(baseRef); + const logOutput = jjChecked(cwd, [ + "log", "-r", `${comparisonBase}..@`, "--no-graph", + "-T", 'change_id.short(8) ++ " " ++ description.first_line() ++ "\\n"' + ]).stdout.trim(); + + const diffStat = jjChecked(cwd, [ + "diff", "--from", comparisonBase, "--to", "@", "--stat" + ]).stdout.trim(); + + const changedFiles = jjChecked(cwd, [ + "diff", "--from", comparisonBase, "--to", "@", "--name-only" + ]).stdout.trim().split("\n").filter(Boolean); + + const currentBranch = getCurrentBranch(cwd); + + return { + mode: "branch", + summary: `Reviewing changes on ${currentBranch} since the fork point with ${baseRef}.`, + content: includeDiff + ? [ + formatSection("Commit Log", logOutput), + formatSection("Diff Stat", diffStat), + formatSection( + "Branch Diff", + jjChecked(cwd, ["diff", "--from", comparisonBase, "--to", "@", "--git"]).stdout + ) + ].join("\n") + : [ + formatSection("Commit Log", logOutput), + formatSection("Diff Stat", diffStat), + formatSection("Changed Files", changedFiles.join("\n")) + ].join("\n"), + changedFiles + }; +} + +export function ensureGitRepository(cwd) { + const availability = binaryAvailable("jj"); + if (!availability.available) { + throw new Error( + "jj is not installed. Install Jujutsu (https://jj-vcs.dev) and retry." + ); + } + const result = jj(cwd, ["workspace", "root"]); + if (result.status !== 0) { + throw new Error( + "This command must run inside a Git or Jujutsu repository." + ); + } + return result.stdout.trim(); +} + +export function getRepoRoot(cwd) { + return jjChecked(cwd, ["workspace", "root"]).stdout.trim(); +} + +export function detectDefaultBranch(cwd) { + const trunkResult = jj(cwd, [ + "log", "-r", "trunk()", "--no-graph", "-T", "commit_id" + ]); + if (trunkResult.status === 0 && !/^0+$/.test(trunkResult.stdout.trim())) { + return "trunk()"; + } + + const candidates = ["main", "master", "trunk"]; + for (const name of candidates) { + const result = jj(cwd, [ + "log", "-r", name, "--no-graph", "-T", "commit_id" + ]); + if (result.status === 0 && result.stdout.trim() && !/^0+$/.test(result.stdout.trim())) { + return name; + } + } + + throw new Error( + "Unable to detect the repository default branch. Pass --base or use --scope working-tree." + ); +} + +export function getCurrentBranch(cwd) { + const bookmarks = jjChecked(cwd, [ + "log", "-r", "@", "--no-graph", + "-T", 'local_bookmarks.map(|b| b.name()).join(", ")' + ]).stdout.trim(); + + if (bookmarks) { + return bookmarks.split(", ")[0]; + } + + return jjChecked(cwd, [ + "log", "-r", "@", "--no-graph", "-T", "change_id.short(8)" + ]).stdout.trim(); +} + +export function getWorkingTreeState(cwd) { + const summaryOutput = jjChecked(cwd, ["diff", "--summary"]).stdout.trim(); + const staged = jjChecked(cwd, ["diff", "--name-only"]).stdout.trim().split("\n").filter(Boolean); + return { + staged, + unstaged: [], + untracked: [], + isDirty: summaryOutput.length > 0 + }; +} + +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) { + return { + mode: "branch", + label: `branch diff against ${baseRef}`, + baseRef, + explicit: true + }; + } + + if (requestedScope === "working-tree") { + return { + mode: "working-tree", + label: "working tree diff", + explicit: true + }; + } + + 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 { + mode: "branch", + label: `branch diff against ${detectedBase}`, + baseRef: detectedBase, + explicit: true + }; + } + + if (state.isDirty) { + return { + mode: "working-tree", + label: "working tree diff", + explicit: false + }; + } + + const detectedBase = detectDefaultBranch(cwd); + return { + mode: "branch", + label: `branch diff against ${detectedBase}`, + baseRef: detectedBase, + explicit: false + }; +} + +export function collectReviewContext(cwd, target, options = {}) { + const repoRoot = getRepoRoot(cwd); + const currentBranch = getCurrentBranch(repoRoot); + const maxInlineFiles = normalizeMaxInlineFiles(options.maxInlineFiles); + const maxInlineDiffBytes = normalizeMaxInlineDiffBytes(options.maxInlineDiffBytes); + let details; + let includeDiff; + let diffBytes; + + if (target.mode === "working-tree") { + const state = getWorkingTreeState(repoRoot); + diffBytes = measureJjOutputBytes(repoRoot, ["diff", "--git"], maxInlineDiffBytes); + includeDiff = + options.includeDiff ?? + (state.staged.length <= maxInlineFiles && diffBytes <= maxInlineDiffBytes); + details = collectWorkingTreeContext(repoRoot, state, { includeDiff }); + } else { + const comparisonBase = buildForkPointRevset(target.baseRef); + const fileCount = jjChecked(repoRoot, [ + "diff", "--from", comparisonBase, "--to", "@", "--name-only" + ]).stdout.trim().split("\n").filter(Boolean).length; + diffBytes = measureJjOutputBytes( + repoRoot, + ["diff", "--from", comparisonBase, "--to", "@", "--git"], + maxInlineDiffBytes + ); + includeDiff = options.includeDiff ?? (fileCount <= maxInlineFiles && diffBytes <= maxInlineDiffBytes); + details = collectRangeContext(repoRoot, includeDiff, target.baseRef); + } + + return { + cwd: repoRoot, + repoRoot, + branch: currentBranch, + target, + fileCount: details.changedFiles.length, + diffBytes, + inputMode: includeDiff ? "inline-diff" : "self-collect", + collectionGuidance: buildAdversarialCollectionGuidance({ includeDiff }), + ...details + }; +} diff --git a/plugins/codex/scripts/lib/vcs.mjs b/plugins/codex/scripts/lib/vcs.mjs new file mode 100644 index 00000000..e3e8dd14 --- /dev/null +++ b/plugins/codex/scripts/lib/vcs.mjs @@ -0,0 +1,84 @@ +import fs from "node:fs"; +import path from "node:path"; + +import { runCommand } from "./process.mjs"; +import * as gitBackend from "./git.mjs"; +import * as jjBackend from "./jj.mjs"; + +const vcsCache = new Map(); + +function findDirUpward(startDir, dirName) { + let dir = path.resolve(startDir); + while (true) { + if (fs.existsSync(path.join(dir, dirName))) return dir; + const parent = path.dirname(dir); + if (parent === dir) return null; + dir = parent; + } +} + +export function detectVcs(cwd) { + const resolved = path.resolve(cwd); + if (vcsCache.has(resolved)) return vcsCache.get(resolved); + + let kind = null; + const jjRoot = findDirUpward(resolved, ".jj"); + const gitRoot = findDirUpward(resolved, ".git"); + if (jjRoot && gitRoot) { + // Pick the closer (deeper) root; if equal depth, prefer jj (colocated repo) + kind = gitRoot.length > jjRoot.length ? "git" : "jj"; + } else if (jjRoot) { + kind = "jj"; + } else if (gitRoot) { + kind = "git"; + } else { + const jjResult = runCommand("jj", ["--no-pager", "--color=never", "--quiet", "--ignore-working-copy", "--at-operation", "@", "workspace", "root"], { cwd: resolved }); + if (jjResult.status === 0 && !jjResult.error) { + kind = "jj"; + } else { + const gitResult = runCommand("git", ["rev-parse", "--show-toplevel"], { cwd: resolved }); + if (gitResult.status === 0 && !gitResult.error) { + kind = "git"; + } + } + } + + if (kind === null) { + throw new Error("This command must run inside a Git or Jujutsu repository."); + } + + vcsCache.set(resolved, kind); + return kind; +} + +function getBackend(cwd) { + return detectVcs(cwd) === "jj" ? jjBackend : gitBackend; +} + +export function ensureGitRepository(cwd) { + return getBackend(cwd).ensureGitRepository(cwd); +} + +export function getRepoRoot(cwd) { + return getBackend(cwd).getRepoRoot(cwd); +} + +export function detectDefaultBranch(cwd) { + return getBackend(cwd).detectDefaultBranch(cwd); +} + +export function getCurrentBranch(cwd) { + return getBackend(cwd).getCurrentBranch(cwd); +} + +export function getWorkingTreeState(cwd) { + return getBackend(cwd).getWorkingTreeState(cwd); +} + +export function resolveReviewTarget(cwd, options = {}) { + return getBackend(cwd).resolveReviewTarget(cwd, options); +} + +export function collectReviewContext(cwd, target, options = {}) { + return getBackend(cwd).collectReviewContext(cwd, target, options); +} diff --git a/plugins/codex/scripts/lib/workspace.mjs b/plugins/codex/scripts/lib/workspace.mjs index 89a0060b..1edccd39 100644 --- a/plugins/codex/scripts/lib/workspace.mjs +++ b/plugins/codex/scripts/lib/workspace.mjs @@ -1,4 +1,4 @@ -import { ensureGitRepository } from "./git.mjs"; +import { ensureGitRepository } from "./vcs.mjs"; export function resolveWorkspaceRoot(cwd) { try { diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index debcadce..03b5e07e 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -337,6 +337,12 @@ rl.on("line", (line) => { case "review/start": { const thread = ensureThread(state, message.params.threadId); + state.lastReviewStart = { + threadId: message.params.threadId ?? null, + delivery: message.params.delivery ?? null, + target: message.params.target ?? null + }; + saveState(state); let reviewThread = thread; if (message.params.delivery === "detached") { reviewThread = nextThread(state, thread.cwd, true); diff --git a/tests/helpers.mjs b/tests/helpers.mjs index 945ae0e7..4948237d 100644 --- a/tests/helpers.mjs +++ b/tests/helpers.mjs @@ -30,3 +30,22 @@ export function initGitRepo(cwd) { run("git", ["config", "commit.gpgsign", "false"], { cwd }); run("git", ["config", "tag.gpgsign", "false"], { cwd }); } + +export function initJjRepo(cwd) { + run("jj", ["git", "init", "--quiet"], { cwd }); +} + +export function commitJjChange(cwd, message) { + run("jj", ["commit", "-m", message], { cwd }); +} + +export function initJjRepoWithCommit(cwd) { + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, ".keep"), ""); + run("jj", ["commit", "-m", "initial commit"], { cwd }); +} + +export function makeMockJjDir(parentDir) { + fs.mkdirSync(path.join(parentDir, ".jj"), { recursive: true }); + return parentDir; +} diff --git a/tests/jj.test.mjs b/tests/jj.test.mjs new file mode 100644 index 00000000..acc17334 --- /dev/null +++ b/tests/jj.test.mjs @@ -0,0 +1,375 @@ +import fs from "node:fs"; +import path from "node:path"; +import test from "node:test"; +import assert from "node:assert/strict"; + +import { + detectDefaultBranch, + getCurrentBranch, + getWorkingTreeState, + resolveReviewTarget, + collectReviewContext +} from "../plugins/codex/scripts/lib/jj.mjs"; +import { binaryAvailable } from "../plugins/codex/scripts/lib/process.mjs"; +import { initJjRepo, initJjRepoWithCommit, commitJjChange, makeTempDir, run } from "./helpers.mjs"; + +const JJ_SKIP = !binaryAvailable("jj").available && "jj binary not available"; + +// ─── getWorkingTreeState ────────────────────────────────────────────────────── + +test("getWorkingTreeState — clean working copy returns empty staged and isDirty false", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-wts-clean-"); + initJjRepoWithCommit(cwd); + const state = getWorkingTreeState(cwd); + assert.deepEqual(state.staged, [], "staged should be empty for clean working copy"); + assert.deepEqual(state.unstaged, [], "unstaged is always empty in jj"); + assert.deepEqual(state.untracked, [], "untracked is always empty in jj"); + assert.equal(state.isDirty, false, "isDirty should be false when working copy is clean"); +}); + +test("getWorkingTreeState — modified file returns staged and isDirty true", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-wts-dirty-"); + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "hello\n"); + const state = getWorkingTreeState(cwd); + assert.ok(state.staged.includes("app.js"), `staged should include 'app.js', got: ${JSON.stringify(state.staged)}`); + assert.deepEqual(state.unstaged, [], "unstaged is always empty in jj"); + assert.deepEqual(state.untracked, [], "untracked is always empty in jj"); + assert.equal(state.isDirty, true, "isDirty should be true when working copy has modifications"); +}); + +// ─── getCurrentBranch ──────────────────────────────────────────────────────── + +test("getCurrentBranch — returns change ID when no bookmarks", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-branch-"); + initJjRepo(cwd); + const branch = getCurrentBranch(cwd); + assert.match(branch, /^[a-z]{8}$/, "should return 8-char change ID when no bookmarks"); + assert.notEqual(branch, "HEAD", "should never return 'HEAD' (meaningless in jj)"); +}); + +// ─── detectDefaultBranch ───────────────────────────────────────────────────── + +test("detectDefaultBranch — throws when trunk() is root() and no bookmarks", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-trunk-root-"); + initJjRepo(cwd); // fresh repo, no remote — trunk() resolves to root() + assert.throws( + () => detectDefaultBranch(cwd), + /Unable to detect the repository default branch/, + "should throw when trunk() resolves to root() and no candidate bookmarks exist" + ); +}); + +test("detectDefaultBranch — falls back to main bookmark when trunk() is root()", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-trunk-fallback-main-"); + initJjRepoWithCommit(cwd); + // Create a 'main' bookmark on the initial commit (trunk() still resolves to root) + run("jj", ["bookmark", "create", "main", "-r", "@-"], { cwd }); + const base = detectDefaultBranch(cwd); + assert.equal(base, "main", "should fall back to 'main' bookmark when trunk() resolves to root()"); +}); + +// ─── resolveReviewTarget ───────────────────────────────────────────────────── + +test("resolveReviewTarget — auto-detect selects working-tree when dirty", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-resolve-auto-dirty-"); + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('hello');\n"); + const target = resolveReviewTarget(cwd, {}); + assert.equal(target.mode, "working-tree", "should select working-tree mode when dirty"); + assert.equal(target.explicit, false, "auto-detect sets explicit to false"); +}); + +test("resolveReviewTarget — explicit working-tree scope works", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-resolve-explicit-wt-"); + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('hello');\n"); + const target = resolveReviewTarget(cwd, { scope: "working-tree" }); + assert.equal(target.mode, "working-tree", "explicit working-tree scope should yield working-tree mode"); + assert.equal(target.explicit, true, "explicit scope sets explicit to true"); +}); + +test("resolveReviewTarget — unsupported scope throws", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-resolve-invalid-scope-"); + initJjRepo(cwd); + assert.throws( + () => resolveReviewTarget(cwd, { scope: "invalid" }), + /Unsupported review scope/, + "should throw for unsupported scope" + ); +}); + +test("collectReviewContext — branch mode works when base ref has multiple bookmarks", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-multi-bookmark-review-"); + initJjRepoWithCommit(cwd); + // Create two bookmarks on the same commit — verifies that passing a + // bookmark name as --base doesn't break when the commit has siblings. + run("jj", ["bookmark", "create", "main", "-r", "@-"], { cwd }); + run("jj", ["bookmark", "create", "origin/main", "-r", "@-"], { cwd }); + fs.writeFileSync(path.join(cwd, "feature.js"), "// feature\n"); + commitJjChange(cwd, "feature change"); + + // Use explicit base to bypass trunk() resolution (requires remote in jj 0.40+) + const target = resolveReviewTarget(cwd, { base: "main" }); + const context = collectReviewContext(cwd, target); + + assert.equal(context.mode, "branch", "should produce branch context without error"); + assert.ok(context.content.includes("## Commit Log"), "should have Commit Log section"); +}); + +// ─── collectReviewContext — working copy ───────────────────────────────────── + +test("collectReviewContext — working copy inline mode has correct sections and shape", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-collect-wt-inline-"); + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('INLINE_MARKER');\n"); + + const target = resolveReviewTarget(cwd, {}); + const context = collectReviewContext(cwd, target); + + // inputMode and mode checks + assert.equal(context.inputMode, "inline-diff", "small diff should use inline-diff mode"); + assert.equal(context.fileCount, 1, "fileCount should be 1"); + assert.equal(context.mode, "working-tree", "mode should be working-tree"); + + // Section headers + assert.ok(context.content.includes("## Git Status"), "content must include '## Git Status' section"); + assert.ok(context.content.includes("## Staged Diff"), "content must include '## Staged Diff' section"); + assert.ok(context.content.includes("## Unstaged Diff"), "content must include '## Unstaged Diff' section"); + assert.ok(context.content.includes("## Untracked Files"), "content must include '## Untracked Files' section"); + + // Actual diff content + assert.ok(context.content.includes("INLINE_MARKER"), "content must contain actual diff content"); + + // collectionGuidance check + assert.match(context.collectionGuidance, /primary evidence/i, "collectionGuidance should mention 'primary evidence' for inline mode"); + + // changedFiles check + assert.ok(context.changedFiles.includes("app.js"), `changedFiles should include 'app.js', got: ${JSON.stringify(context.changedFiles)}`); + + // All required fields + const requiredFields = [ + "cwd", "repoRoot", "branch", "target", "fileCount", + "diffBytes", "inputMode", "collectionGuidance", "mode", "summary", "content", "changedFiles" + ]; + for (const field of requiredFields) { + assert.ok(field in context, `context missing required field: ${field}`); + } +}); + +test("collectReviewContext — no ANSI escape codes in content", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-ansi-"); + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, "check.js"), "// test file\n"); + + const target = resolveReviewTarget(cwd, {}); + const context = collectReviewContext(cwd, target); + + assert.ok( + !/\u001b\[/.test(context.content), + "ANSI escape codes found in content — --color=never should prevent them" + ); +}); + +test("collectReviewContext — self-collect mode when forced", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-collect-wt-self-"); + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('self-collect');\n"); + + const target = resolveReviewTarget(cwd, {}); + const context = collectReviewContext(cwd, target, { includeDiff: false }); + + assert.equal(context.inputMode, "self-collect", "forced includeDiff=false should yield self-collect mode"); + assert.ok( + context.content.includes("## Staged Diff Stat"), + "self-collect content must include '## Staged Diff Stat' section" + ); + assert.match( + context.collectionGuidance, + /lightweight summary/i, + "collectionGuidance should mention 'lightweight summary' for self-collect mode" + ); + assert.ok( + !context.collectionGuidance.includes("jj diff"), + "self-collect guidance must NOT suggest running jj commands (sandbox-unsafe)" + ); + assert.match( + context.collectionGuidance, + /do not attempt to run jj commands/i, + "self-collect guidance must explicitly warn against running jj commands" + ); +}); + +// ─── collectReviewContext — range mode ─────────────────────────────────────── + +function setupRepoWithBranch(cwd) { + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, "base.js"), "// base\n"); + commitJjChange(cwd, "first change"); + run("jj", ["bookmark", "create", "main", "-r", "@-"], { cwd }); + fs.writeFileSync(path.join(cwd, "feature.js"), "// feature\n"); + commitJjChange(cwd, "second change"); +} + +function setupDivergedRepoWithAdvancedBase(cwd) { + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, "base.js"), "// base\n"); + commitJjChange(cwd, "base change"); + const baseId = run("jj", ["log", "-r", "@-", "--no-graph", "-T", "change_id.short(8)"], { cwd }).stdout.trim(); + run("jj", ["bookmark", "create", "main", "-r", "@-"], { cwd }); + + fs.writeFileSync(path.join(cwd, "feature.js"), "// feature\n"); + commitJjChange(cwd, "feature change"); + const featureId = run("jj", ["log", "-r", "@-", "--no-graph", "-T", "change_id.short(8)"], { cwd }).stdout.trim(); + + run("jj", ["new", baseId], { cwd }); + fs.writeFileSync(path.join(cwd, "main-only.js"), "// main advance\n"); + commitJjChange(cwd, "main advance"); + run("jj", ["bookmark", "move", "main", "--to", "@-"], { cwd }); + run("jj", ["edit", featureId], { cwd }); +} + +test("collectReviewContext — range mode with inline diff", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-collect-range-inline-"); + setupRepoWithBranch(cwd); + + // Use explicit base to bypass trunk() (requires remote in jj 0.40+) + const target = resolveReviewTarget(cwd, { base: "main" }); + assert.equal(target.mode, "branch", "resolveReviewTarget with explicit base should yield branch mode"); + + const context = collectReviewContext(cwd, target); + + assert.equal(context.mode, "branch", "collectReviewContext mode should be 'branch'"); + + // Section headers + assert.ok(context.content.includes("## Commit Log"), "range content must include '## Commit Log' section"); + assert.ok(context.content.includes("## Diff Stat"), "range content must include '## Diff Stat' section"); + assert.ok(context.content.includes("## Branch Diff"), "range content must include '## Branch Diff' section"); + + // All required fields + const requiredFields = [ + "cwd", "repoRoot", "branch", "target", "fileCount", + "diffBytes", "inputMode", "collectionGuidance", "mode", "summary", "content", "changedFiles" + ]; + for (const field of requiredFields) { + assert.ok(field in context, `context missing required field: ${field}`); + } +}); + +test("collectReviewContext — range mode self-collect with sandbox-safe guidance", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-collect-range-self-"); + setupRepoWithBranch(cwd); + + const target = resolveReviewTarget(cwd, { base: "main" }); + const context = collectReviewContext(cwd, target, { includeDiff: false }); + + assert.equal(context.mode, "branch", "range self-collect mode should be 'branch'"); + assert.ok( + context.content.includes("## Changed Files"), + "range self-collect content must include '## Changed Files' section (not 'Branch Diff')" + ); + assert.equal(context.inputMode, "self-collect", "stat-only mode should be self-collect"); + assert.ok( + !context.collectionGuidance.includes("jj diff"), + "self-collect guidance must NOT suggest running jj commands" + ); +}); + +test("collectReviewContext — range mode no ANSI codes", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-collect-range-ansi-"); + setupRepoWithBranch(cwd); + + const target = resolveReviewTarget(cwd, { base: "main" }); + const context = collectReviewContext(cwd, target); + + assert.ok( + !/\u001b\[/.test(context.content), + "ANSI escape codes found in range mode content" + ); +}); + +test("collectReviewContext — branch mode excludes unrelated base bookmark advances", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-diverged-base-"); + setupDivergedRepoWithAdvancedBase(cwd); + + const target = resolveReviewTarget(cwd, { base: "main" }); + const context = collectReviewContext(cwd, target); + + assert.ok(context.changedFiles.includes("feature.js"), "feature.js should be reviewed"); + assert.ok(!context.changedFiles.includes("main-only.js"), "base-only changes should not be included in the review diff"); + assert.match(context.summary, /fork point with main/, "summary should describe merge-base semantics"); +}); + +// ─── collectReviewContext — custom base ref ────────────────────────────────── + +function setupRepoWithMidCommit(cwd) { + initJjRepo(cwd); + fs.writeFileSync(path.join(cwd, "base.js"), "// base\n"); + commitJjChange(cwd, "first change"); + run("jj", ["bookmark", "create", "main", "-r", "@-"], { cwd }); + // mid commit — between trunk and feature + fs.writeFileSync(path.join(cwd, "mid.js"), "// mid\n"); + commitJjChange(cwd, "mid change"); + // Capture mid commit's change ID for use as custom --base + const midId = run("jj", ["log", "-r", "@-", "--no-graph", "-T", "change_id.short(8)"], { cwd }).stdout.trim(); + // feature commit on top + fs.writeFileSync(path.join(cwd, "feature.js"), "// feature\n"); + commitJjChange(cwd, "feature change"); + return { midId }; +} + +test("collectReviewContext — custom --base revset used in range diff", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-collect-custom-base-"); + const { midId } = setupRepoWithMidCommit(cwd); + + const target = resolveReviewTarget(cwd, { base: midId }); + assert.equal(target.mode, "branch"); + assert.equal(target.baseRef, midId); + + const context = collectReviewContext(cwd, target); + + // Only feature.js should appear — mid.js is before the custom base + assert.ok(context.changedFiles.includes("feature.js"), "feature.js should be in changedFiles"); + assert.ok(!context.changedFiles.includes("mid.js"), "mid.js should NOT be in changedFiles when using mid commit as base"); + assert.ok(!context.changedFiles.includes("base.js"), "base.js should NOT be in changedFiles"); + + // Summary should reference the custom base, not trunk() + assert.ok(context.summary.includes(midId), `summary should reference custom base '${midId}', got: ${context.summary}`); + assert.ok(!context.summary.includes("trunk()"), "summary should NOT reference trunk() when custom base is used"); + + assert.equal(context.mode, "branch"); + assert.ok(context.fileCount >= 1, "fileCount should be at least 1"); +}); + +test("collectReviewContext — custom --base context has all required fields", { skip: JJ_SKIP }, () => { + const cwd = makeTempDir("jj-test-collect-custom-base-shape-"); + const { midId } = setupRepoWithMidCommit(cwd); + + const target = resolveReviewTarget(cwd, { base: midId }); + const context = collectReviewContext(cwd, target); + + const requiredFields = [ + "cwd", "repoRoot", "branch", "target", "fileCount", + "diffBytes", "inputMode", "collectionGuidance", "mode", "summary", "content", "changedFiles" + ]; + for (const field of requiredFields) { + assert.ok(field in context, `context missing required field: ${field}`); + } + + // Verify types match git backend contract + assert.equal(typeof context.cwd, "string"); + assert.equal(typeof context.repoRoot, "string"); + assert.equal(typeof context.branch, "string"); + assert.equal(typeof context.target, "object"); + assert.equal(typeof context.fileCount, "number"); + assert.equal(typeof context.diffBytes, "number"); + assert.equal(typeof context.inputMode, "string"); + assert.equal(typeof context.collectionGuidance, "string"); + assert.equal(typeof context.mode, "string"); + assert.equal(typeof context.summary, "string"); + assert.equal(typeof context.content, "string"); + assert.ok(Array.isArray(context.changedFiles), "changedFiles should be an array"); + + // collectionGuidance should be set (adversarial review uses this) + assert.ok(context.collectionGuidance.length > 0, "collectionGuidance should not be empty"); +}); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 90408372..b7b7edad 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -6,8 +6,9 @@ import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; -import { initGitRepo, makeTempDir, run } from "./helpers.mjs"; +import { commitJjChange, initGitRepo, initJjRepo, initJjRepoWithCommit, makeTempDir, run } from "./helpers.mjs"; import { loadBrokerSession, saveBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { binaryAvailable } from "../plugins/codex/scripts/lib/process.mjs"; import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); @@ -15,6 +16,7 @@ const PLUGIN_ROOT = path.join(ROOT, "plugins", "codex"); const SCRIPT = path.join(PLUGIN_ROOT, "scripts", "codex-companion.mjs"); const STOP_HOOK = path.join(PLUGIN_ROOT, "scripts", "stop-review-gate-hook.mjs"); const SESSION_HOOK = path.join(PLUGIN_ROOT, "scripts", "session-lifecycle-hook.mjs"); +const JJ_SKIP = !binaryAvailable("jj").available && "jj binary not available"; async function waitFor(predicate, { timeoutMs = 5000, intervalMs = 50 } = {}) { const start = Date.now(); @@ -28,12 +30,31 @@ async function waitFor(predicate, { timeoutMs = 5000, intervalMs = 50 } = {}) { throw new Error("Timed out waiting for condition."); } +function setupDivergedJjRepoForReview(repo) { + initJjRepo(repo); + fs.writeFileSync(path.join(repo, "base.js"), "// base\n"); + commitJjChange(repo, "base change"); + const baseId = run("jj", ["log", "-r", "@-", "--no-graph", "-T", "change_id.short(8)"], { cwd: repo }).stdout.trim(); + run("jj", ["bookmark", "create", "main", "-r", "@-"], { cwd: repo }); + + fs.writeFileSync(path.join(repo, "feature.js"), "// feature\n"); + commitJjChange(repo, "feature change"); + const featureId = run("jj", ["log", "-r", "@-", "--no-graph", "-T", "change_id.short(8)"], { cwd: repo }).stdout.trim(); + + run("jj", ["new", baseId], { cwd: repo }); + fs.writeFileSync(path.join(repo, "main-only.js"), "// main advance\n"); + commitJjChange(repo, "main advance"); + run("jj", ["bookmark", "move", "main", "--to", "@-"], { cwd: repo }); + run("jj", ["edit", featureId], { cwd: repo }); +} + test("setup reports ready when fake codex is installed and authenticated", () => { + const workspace = makeTempDir(); const binDir = makeTempDir(); installFakeCodex(binDir); const result = run("node", [SCRIPT, "setup", "--json"], { - cwd: ROOT, + cwd: workspace, env: buildEnv(binDir) }); @@ -45,12 +66,13 @@ test("setup reports ready when fake codex is installed and authenticated", () => }); test("setup is ready without npm when Codex is already installed and authenticated", () => { + const workspace = makeTempDir(); const binDir = makeTempDir(); installFakeCodex(binDir); fs.symlinkSync(process.execPath, path.join(binDir, "node")); const result = run("node", [SCRIPT, "setup", "--json"], { - cwd: ROOT, + cwd: workspace, env: { ...process.env, PATH: binDir @@ -66,11 +88,12 @@ test("setup is ready without npm when Codex is already installed and authenticat }); test("setup trusts app-server API key auth even when login status alone would fail", () => { + const workspace = makeTempDir(); const binDir = makeTempDir(); installFakeCodex(binDir, "api-key-account-only"); const result = run("node", [SCRIPT, "setup", "--json"], { - cwd: ROOT, + cwd: workspace, env: buildEnv(binDir) }); @@ -84,11 +107,12 @@ test("setup trusts app-server API key auth even when login status alone would fa }); test("setup is ready when the active provider does not require OpenAI login", () => { + const workspace = makeTempDir(); const binDir = makeTempDir(); installFakeCodex(binDir, "provider-no-auth"); const result = run("node", [SCRIPT, "setup", "--json"], { - cwd: ROOT, + cwd: workspace, env: buildEnv(binDir) }); @@ -102,11 +126,12 @@ test("setup is ready when the active provider does not require OpenAI login", () }); test("setup treats custom providers with app-server-ready config as ready", () => { + const workspace = makeTempDir(); const binDir = makeTempDir(); installFakeCodex(binDir, "env-key-provider"); const result = run("node", [SCRIPT, "setup", "--json"], { - cwd: ROOT, + cwd: workspace, env: buildEnv(binDir) }); @@ -120,11 +145,12 @@ test("setup treats custom providers with app-server-ready config as ready", () = }); test("setup reports not ready when app-server config read fails", () => { + const workspace = makeTempDir(); const binDir = makeTempDir(); installFakeCodex(binDir, "config-read-fails"); const result = run("node", [SCRIPT, "setup", "--json"], { - cwd: ROOT, + cwd: workspace, env: buildEnv(binDir) }); @@ -232,6 +258,48 @@ test("review accepts the quoted raw argument style for built-in base-branch revi assert.match(result.stdout, /No material issues found/); }); +test("review uses native custom review/start target with jj instructions", { skip: JJ_SKIP }, () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initJjRepoWithCommit(repo); + fs.writeFileSync(path.join(repo, "README.md"), "updated\n"); + + const result = run("node", [SCRIPT, "review"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /Reviewed custom target/); + + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.equal(state.lastReviewStart?.target?.type, "custom"); + assert.match(state.lastReviewStart?.target?.instructions ?? "", /Jujutsu \(jj\), not git/); + assert.match(state.lastReviewStart?.target?.instructions ?? "", /working copy commit/); +}); + +test("review uses merge-base jj instructions for branch reviews", { skip: JJ_SKIP }, () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + setupDivergedJjRepoForReview(repo); + + const result = run("node", [SCRIPT, "review", "--base", "main"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + const instructions = state.lastReviewStart?.target?.instructions ?? ""; + assert.equal(state.lastReviewStart?.target?.type, "custom"); + assert.match(instructions, /common ancestor \(fork point\) of main and @/); + assert.doesNotMatch(instructions, /changes from main to @/); +}); + test("adversarial review renders structured findings over app-server turn/start", () => { const repo = makeTempDir(); const binDir = makeTempDir(); diff --git a/tests/vcs.test.mjs b/tests/vcs.test.mjs new file mode 100644 index 00000000..4f06979d --- /dev/null +++ b/tests/vcs.test.mjs @@ -0,0 +1,113 @@ +import fs from "node:fs"; +import path from "node:path"; +import test from "node:test"; +import assert from "node:assert/strict"; + +import { detectVcs, ensureGitRepository } from "../plugins/codex/scripts/lib/vcs.mjs"; +import { binaryAvailable } from "../plugins/codex/scripts/lib/process.mjs"; +import { initJjRepo, makeMockJjDir, makeTempDir } from "./helpers.mjs"; + +test("detectVcs returns jj when .jj directory is present", () => { + const cwd = makeTempDir(); + fs.mkdirSync(path.join(cwd, ".jj"), { recursive: true }); + assert.equal(detectVcs(cwd), "jj"); +}); + +test("detectVcs returns git when only .git directory is present", () => { + const cwd = makeTempDir(); + fs.mkdirSync(path.join(cwd, ".git"), { recursive: true }); + assert.equal(detectVcs(cwd), "git"); +}); + +test("detectVcs prefers jj in colocated repos", () => { + const cwd = makeTempDir(); + fs.mkdirSync(path.join(cwd, ".jj"), { recursive: true }); + fs.mkdirSync(path.join(cwd, ".git"), { recursive: true }); + assert.equal(detectVcs(cwd), "jj"); +}); + +test("detectVcs walks up parent directories to find .jj", () => { + const cwd = makeTempDir(); + fs.mkdirSync(path.join(cwd, ".jj"), { recursive: true }); + const nested = path.join(cwd, "src", "components"); + fs.mkdirSync(nested, { recursive: true }); + assert.equal(detectVcs(nested), "jj"); +}); + +test("detectVcs walks up parent directories to find .git", () => { + const cwd = makeTempDir(); + fs.mkdirSync(path.join(cwd, ".git"), { recursive: true }); + const nested = path.join(cwd, "src", "components"); + fs.mkdirSync(nested, { recursive: true }); + assert.equal(detectVcs(nested), "git"); +}); + +test("detectVcs throws when neither .jj nor .git found", () => { + const cwd = makeTempDir(); + assert.throws(() => detectVcs(cwd), /Git or Jujutsu repository/); +}); + +test("detectVcs prefers closer .git over ancestor .jj (nested Git inside JJ)", () => { + const outer = makeTempDir(); + fs.mkdirSync(path.join(outer, ".jj"), { recursive: true }); + const inner = path.join(outer, "nested-git-repo"); + fs.mkdirSync(path.join(inner, ".git"), { recursive: true }); + assert.equal(detectVcs(inner), "git"); +}); + +test("detectVcs prefers closer .jj over ancestor .git (nested JJ inside Git)", () => { + const outer = makeTempDir(); + fs.mkdirSync(path.join(outer, ".git"), { recursive: true }); + const inner = path.join(outer, "nested-jj-repo"); + fs.mkdirSync(path.join(inner, ".jj"), { recursive: true }); + assert.equal(detectVcs(inner), "jj"); +}); + +test("detectVcs prefers closer .git from subdirectory of nested Git repo", () => { + const outer = makeTempDir(); + fs.mkdirSync(path.join(outer, ".jj"), { recursive: true }); + const inner = path.join(outer, "nested-git-repo"); + fs.mkdirSync(path.join(inner, ".git"), { recursive: true }); + const deep = path.join(inner, "src", "lib"); + fs.mkdirSync(deep, { recursive: true }); + assert.equal(detectVcs(deep), "git"); +}); + +test("detectVcs caches result for same resolved path", () => { + const cwd = makeTempDir(); + fs.mkdirSync(path.join(cwd, ".jj"), { recursive: true }); + assert.equal(detectVcs(cwd), "jj"); + + // Remove .jj — cached result should still return "jj" + fs.rmSync(path.join(cwd, ".jj"), { recursive: true }); + assert.equal(detectVcs(cwd), "jj"); +}); + +test("detectVcs resolves relative and absolute paths to same cache entry", () => { + const cwd = makeTempDir(); + fs.mkdirSync(path.join(cwd, ".jj"), { recursive: true }); + assert.equal(detectVcs(cwd), "jj"); + assert.equal(detectVcs(path.resolve(cwd)), "jj"); +}); + +test("ensureGitRepository returns jj workspace root for jj repos", { skip: !binaryAvailable("jj").available && "jj binary not available" }, () => { + const cwd = makeTempDir(); + initJjRepo(cwd); + const root = ensureGitRepository(cwd); + assert.equal(root, fs.realpathSync(cwd)); +}); + +test("ensureGitRepository resolves workspace root from subdirectory", { skip: !binaryAvailable("jj").available && "jj binary not available" }, () => { + const cwd = makeTempDir(); + initJjRepo(cwd); + const sub = path.join(cwd, "src"); + fs.mkdirSync(sub, { recursive: true }); + const root = ensureGitRepository(sub); + assert.equal(root, fs.realpathSync(cwd)); +}); + +test("ensureGitRepository throws when jj binary is missing and .jj exists", { skip: binaryAvailable("jj").available && "jj binary is available — cannot test missing binary path" }, () => { + const cwd = makeTempDir(); + fs.mkdirSync(path.join(cwd, ".jj"), { recursive: true }); + assert.throws(() => ensureGitRepository(cwd), /jj is not installed/); +});