From 5471211120d402583b08929a25c2c4080aac97e9 Mon Sep 17 00:00:00 2001 From: Brice Figureau Date: Sun, 26 Apr 2026 20:47:01 +0200 Subject: [PATCH 1/5] Add jj backend and VCS dispatch layer Introduce jj.mjs with Jujutsu-native implementations of all VCS operations: detection, workspace root, working tree state, review target resolution, and diff collection (working copy and range modes). Add vcs.mjs as a dispatch layer that auto-detects jj vs git and routes all VCS calls to the appropriate backend. Update codex-companion.mjs and workspace.mjs to import through vcs.mjs so jj repos use jj-native commands throughout. The native Codex reviewer only understands git, so jj repos fall back to the context-collection review path which uses our jj backend for diff gathering. JJ-Change-Id: vqqkyxznqrqn --- plugins/codex/scripts/codex-companion.mjs | 9 +- plugins/codex/scripts/lib/jj.mjs | 300 ++++++++++++++++++++++ plugins/codex/scripts/lib/vcs.mjs | 79 ++++++ plugins/codex/scripts/lib/workspace.mjs | 2 +- 4 files changed, 387 insertions(+), 3 deletions(-) create mode 100644 plugins/codex/scripts/lib/jj.mjs create mode 100644 plugins/codex/scripts/lib/vcs.mjs diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 35222fd5..67d899c1 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 { @@ -362,7 +362,12 @@ async function executeReviewRun(request) { }); const focusText = request.focusText?.trim() ?? ""; const reviewName = request.reviewName ?? "Review"; - if (reviewName === "Review") { + // The Codex native reviewer (runAppServerReview) only understands git + // internally — it runs git commands to collect diffs. For jj repos we fall + // through to the context-collection path which uses our jj backend to + // gather diffs and feeds them to Codex via runAppServerTurn. + const useNativeReviewer = reviewName === "Review" && detectVcs(request.cwd) === "git"; + if (useNativeReviewer) { const reviewTarget = validateNativeReviewRequest(target, focusText); const result = await runAppServerReview(request.cwd, { target: reviewTarget, diff --git a/plugins/codex/scripts/lib/jj.mjs b/plugins/codex/scripts/lib/jj.mjs new file mode 100644 index 00000000..767222fe --- /dev/null +++ b/plugins/codex/scripts/lib/jj.mjs @@ -0,0 +1,300 @@ +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 listUniqueFiles(...groups) { + return [...new Set(groups.flat().filter(Boolean))].sort(); +} + +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 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. This is a Jujutsu (jj) repository — use read-only jj commands (not git) to inspect the target diff before finalizing findings. Key commands: `jj diff`, `jj log`, `jj diff --stat`, `jj show`."; +} + +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) { + const logOutput = jjChecked(cwd, [ + "log", "-r", "trunk()..@", "--no-graph", + "-T", 'change_id.short(8) ++ " " ++ description.first_line() ++ "\\n"' + ]).stdout.trim(); + + const diffStat = jjChecked(cwd, [ + "diff", "--from", "trunk()", "--to", "@", "--stat" + ]).stdout.trim(); + + const changedFiles = jjChecked(cwd, [ + "diff", "--from", "trunk()", "--to", "@", "--name-only" + ]).stdout.trim().split("\n").filter(Boolean); + + const currentBranch = getCurrentBranch(cwd); + + return { + mode: "branch", + summary: `Reviewing range trunk()..@ on ${currentBranch}.`, + content: includeDiff + ? [ + formatSection("Commit Log", logOutput), + formatSection("Diff Stat", diffStat), + formatSection( + "Branch Diff", + jjChecked(cwd, ["diff", "--from", "trunk()", "--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 trunkId = jjChecked(cwd, [ + "log", "-r", "trunk()", "--no-graph", "-T", "commit_id" + ]).stdout.trim(); + + if (/^0+$/.test(trunkId)) { + throw new Error( + "Unable to detect the repository default branch. Pass --base or use --scope working-tree." + ); + } + + return "trunk()"; +} + +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 fileCount = jjChecked(repoRoot, [ + "diff", "--from", "trunk()", "--to", "@", "--name-only" + ]).stdout.trim().split("\n").filter(Boolean).length; + diffBytes = measureJjOutputBytes( + repoRoot, + ["diff", "--from", "trunk()", "--to", "@", "--git"], + maxInlineDiffBytes + ); + includeDiff = options.includeDiff ?? (fileCount <= maxInlineFiles && diffBytes <= maxInlineDiffBytes); + details = collectRangeContext(repoRoot, includeDiff); + } + + 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..f19be7af --- /dev/null +++ b/plugins/codex/scripts/lib/vcs.mjs @@ -0,0 +1,79 @@ +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; + if (findDirUpward(resolved, ".jj")) { + kind = "jj"; + } else if (findDirUpward(resolved, ".git")) { + 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 { From e14d8d5293086ab7e61c79cc77bf1d1d7cbce6b5 Mon Sep 17 00:00:00 2001 From: Brice Figureau Date: Mon, 27 Apr 2026 09:40:35 +0200 Subject: [PATCH 2/5] Add VCS detection and workspace root tests Test detectVcs for .jj/.git directory detection, colocated repo preference, upward directory walking, caching, and the error case when neither VCS is found. JJ-Change-Id: qvrvvyzosxlu --- tests/helpers.mjs | 19 ++++++++++ tests/vcs.test.mjs | 87 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 tests/vcs.test.mjs 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/vcs.test.mjs b/tests/vcs.test.mjs new file mode 100644 index 00000000..9134745d --- /dev/null +++ b/tests/vcs.test.mjs @@ -0,0 +1,87 @@ +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 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/); +}); From 04782d31c92814d1913db68cc61304bea5559ae7 Mon Sep 17 00:00:00 2001 From: Brice Figureau Date: Mon, 27 Apr 2026 11:28:29 +0200 Subject: [PATCH 3/5] Add jj review API tests Cover working copy and range review context collection, ANSI stripping, self-collect fallback for large diffs, custom --base revset support, and review context shape parity with the git backend. JJ-Change-Id: qrqqnnstvsqq --- tests/jj.test.mjs | 250 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 250 insertions(+) create mode 100644 tests/jj.test.mjs diff --git a/tests/jj.test.mjs b/tests/jj.test.mjs new file mode 100644 index 00000000..9c723f1a --- /dev/null +++ b/tests/jj.test.mjs @@ -0,0 +1,250 @@ +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()", { 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()" + ); +}); + +// ─── 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" + ); +}); + +// ─── 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"); +} + +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", { 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"); +}); + +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" + ); +}); + From 4420adcfd55bc01a99cb7b9c0a3cdd412d0dcfb6 Mon Sep 17 00:00:00 2001 From: Brice Figureau Date: Tue, 28 Apr 2026 09:16:56 +0200 Subject: [PATCH 4/5] jj: fix review base detection and branch diff context Detect default jj review bases from available bookmarks when trunk() is unavailable, diff branch reviews from the fork point/common ancestor instead of the live base bookmark tip, and keep self-collect guidance sandbox-safe. Add coverage for nested VCS root detection and the new jj review-context cases. JJ-Change-Id: pqrxknwuxups --- plugins/codex/scripts/lib/jj.mjs | 53 +++++++----- plugins/codex/scripts/lib/vcs.mjs | 9 +- tests/jj.test.mjs | 131 +++++++++++++++++++++++++++++- tests/vcs.test.mjs | 26 ++++++ 4 files changed, 194 insertions(+), 25 deletions(-) diff --git a/plugins/codex/scripts/lib/jj.mjs b/plugins/codex/scripts/lib/jj.mjs index 767222fe..340d2223 100644 --- a/plugins/codex/scripts/lib/jj.mjs +++ b/plugins/codex/scripts/lib/jj.mjs @@ -21,9 +21,7 @@ function jjChecked(cwd, args, options = {}) { ], { cwd, ...options }); } -function listUniqueFiles(...groups) { - return [...new Set(groups.flat().filter(Boolean))].sort(); -} + function normalizeMaxInlineFiles(value) { const parsed = Number(value); @@ -45,12 +43,16 @@ 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. This is a Jujutsu (jj) repository — use read-only jj commands (not git) to inspect the target diff before finalizing findings. Key commands: `jj diff`, `jj log`, `jj diff --stat`, `jj show`."; + 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) { @@ -102,32 +104,33 @@ function collectWorkingTreeContext(cwd, state, options = {}) { } } -function collectRangeContext(cwd, includeDiff) { +function collectRangeContext(cwd, includeDiff, baseRef = "trunk()") { + const comparisonBase = buildForkPointRevset(baseRef); const logOutput = jjChecked(cwd, [ - "log", "-r", "trunk()..@", "--no-graph", + "log", "-r", `${comparisonBase}..@`, "--no-graph", "-T", 'change_id.short(8) ++ " " ++ description.first_line() ++ "\\n"' ]).stdout.trim(); const diffStat = jjChecked(cwd, [ - "diff", "--from", "trunk()", "--to", "@", "--stat" + "diff", "--from", comparisonBase, "--to", "@", "--stat" ]).stdout.trim(); const changedFiles = jjChecked(cwd, [ - "diff", "--from", "trunk()", "--to", "@", "--name-only" + "diff", "--from", comparisonBase, "--to", "@", "--name-only" ]).stdout.trim().split("\n").filter(Boolean); const currentBranch = getCurrentBranch(cwd); return { mode: "branch", - summary: `Reviewing range trunk()..@ on ${currentBranch}.`, + 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", "trunk()", "--to", "@", "--git"]).stdout + jjChecked(cwd, ["diff", "--from", comparisonBase, "--to", "@", "--git"]).stdout ) ].join("\n") : [ @@ -160,17 +163,26 @@ export function getRepoRoot(cwd) { } export function detectDefaultBranch(cwd) { - const trunkId = jjChecked(cwd, [ + const trunkResult = jj(cwd, [ "log", "-r", "trunk()", "--no-graph", "-T", "commit_id" - ]).stdout.trim(); + ]); + if (trunkResult.status === 0 && !/^0+$/.test(trunkResult.stdout.trim())) { + return "trunk()"; + } - if (/^0+$/.test(trunkId)) { - throw new Error( - "Unable to detect the repository default branch. Pass --base or use --scope working-tree." - ); + 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; + } } - return "trunk()"; + throw new Error( + "Unable to detect the repository default branch. Pass --base or use --scope working-tree." + ); } export function getCurrentBranch(cwd) { @@ -274,16 +286,17 @@ export function collectReviewContext(cwd, target, options = {}) { (state.staged.length <= maxInlineFiles && diffBytes <= maxInlineDiffBytes); details = collectWorkingTreeContext(repoRoot, state, { includeDiff }); } else { + const comparisonBase = buildForkPointRevset(target.baseRef); const fileCount = jjChecked(repoRoot, [ - "diff", "--from", "trunk()", "--to", "@", "--name-only" + "diff", "--from", comparisonBase, "--to", "@", "--name-only" ]).stdout.trim().split("\n").filter(Boolean).length; diffBytes = measureJjOutputBytes( repoRoot, - ["diff", "--from", "trunk()", "--to", "@", "--git"], + ["diff", "--from", comparisonBase, "--to", "@", "--git"], maxInlineDiffBytes ); includeDiff = options.includeDiff ?? (fileCount <= maxInlineFiles && diffBytes <= maxInlineDiffBytes); - details = collectRangeContext(repoRoot, includeDiff); + details = collectRangeContext(repoRoot, includeDiff, target.baseRef); } return { diff --git a/plugins/codex/scripts/lib/vcs.mjs b/plugins/codex/scripts/lib/vcs.mjs index f19be7af..e3e8dd14 100644 --- a/plugins/codex/scripts/lib/vcs.mjs +++ b/plugins/codex/scripts/lib/vcs.mjs @@ -22,9 +22,14 @@ export function detectVcs(cwd) { if (vcsCache.has(resolved)) return vcsCache.get(resolved); let kind = null; - if (findDirUpward(resolved, ".jj")) { + 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 (findDirUpward(resolved, ".git")) { + } else if (gitRoot) { kind = "git"; } else { const jjResult = runCommand("jj", ["--no-pager", "--color=never", "--quiet", "--ignore-working-copy", "--at-operation", "@", "workspace", "root"], { cwd: resolved }); diff --git a/tests/jj.test.mjs b/tests/jj.test.mjs index 9c723f1a..acc17334 100644 --- a/tests/jj.test.mjs +++ b/tests/jj.test.mjs @@ -50,16 +50,25 @@ test("getCurrentBranch — returns change ID when no bookmarks", { skip: JJ_SKIP // ─── detectDefaultBranch ───────────────────────────────────────────────────── -test("detectDefaultBranch — throws when trunk() is root()", { skip: JJ_SKIP }, () => { +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()" + "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 }, () => { @@ -180,6 +189,15 @@ test("collectReviewContext — self-collect mode when forced", { skip: JJ_SKIP } /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 ─────────────────────────────────────── @@ -193,6 +211,24 @@ function setupRepoWithBranch(cwd) { 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); @@ -220,7 +256,7 @@ test("collectReviewContext — range mode with inline diff", { skip: JJ_SKIP }, } }); -test("collectReviewContext — range mode self-collect", { skip: JJ_SKIP }, () => { +test("collectReviewContext — range mode self-collect with sandbox-safe guidance", { skip: JJ_SKIP }, () => { const cwd = makeTempDir("jj-test-collect-range-self-"); setupRepoWithBranch(cwd); @@ -233,6 +269,10 @@ test("collectReviewContext — range mode self-collect", { skip: JJ_SKIP }, () = "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 }, () => { @@ -248,3 +288,88 @@ test("collectReviewContext — range mode no ANSI codes", { skip: JJ_SKIP }, () ); }); +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/vcs.test.mjs b/tests/vcs.test.mjs index 9134745d..4f06979d 100644 --- a/tests/vcs.test.mjs +++ b/tests/vcs.test.mjs @@ -47,6 +47,32 @@ test("detectVcs throws when neither .jj nor .git found", () => { 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 }); From 7e2a40f46abee794318ac40cd865144d050c6c92 Mon Sep 17 00:00:00 2001 From: Brice Figureau Date: Tue, 28 Apr 2026 09:16:56 +0200 Subject: [PATCH 5/5] codex: use native review/start custom targets for jj repos Route plain /codex:review for jj repositories through the native review/start API using a custom target with jj-specific instructions, instead of falling back to the prompt-built turn/start path. Add runtime coverage for the outgoing native review payload. JJ-Change-Id: knywmwzwwrlt --- plugins/codex/scripts/codex-companion.mjs | 50 +++++++++++--- tests/fake-codex-fixture.mjs | 6 ++ tests/runtime.test.mjs | 82 +++++++++++++++++++++-- 3 files changed, 121 insertions(+), 17 deletions(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 67d899c1..bc430976 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -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,13 +393,12 @@ async function executeReviewRun(request) { }); const focusText = request.focusText?.trim() ?? ""; const reviewName = request.reviewName ?? "Review"; - // The Codex native reviewer (runAppServerReview) only understands git - // internally — it runs git commands to collect diffs. For jj repos we fall - // through to the context-collection path which uses our jj backend to - // gather diffs and feeds them to Codex via runAppServerTurn. - const useNativeReviewer = reviewName === "Review" && detectVcs(request.cwd) === "git"; + // 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(target, focusText); + const reviewTarget = validateNativeReviewRequest(request.cwd, target, focusText); const result = await runAppServerReview(request.cwd, { target: reviewTarget, model: request.model, @@ -701,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/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/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();