From 7ba2227c3c25a24f5778c55643848490fcda9cae Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 5 Jun 2026 14:39:39 +0000 Subject: [PATCH 1/4] fix(coding-agent): guarded Git-mutating automation in pure jj workspaces Worktree setup and autoresearch Git prep silently misbehaved in pure Jujutsu workspaces (`.jj/repo/` present, no colocated `.git/`): - `task/worktree.ts#getRepoRoot` threw a generic "Git repository not found for isolated task execution.", giving a jj user no hint about what was wrong. - `autoresearch/git.ts#ensureAutoresearchBranch` returned a soft "Not in a git repository" warning, letting `/autoresearch` proceed with no branch isolation, baseline reset, or auto-commits. Both paths now detect a pure jj workspace via a new `jj.isPureJjRepo` helper and surface an actionable Jujutsu-specific error pointing at `jj git init --colocate`. Colocated jj-git workspaces (both `.jj/` and `.git/` at the same root) and plain Git checkouts behave exactly as before. Fixes #1935 --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/autoresearch/git.ts | 7 ++ packages/coding-agent/src/task/worktree.ts | 11 ++- packages/coding-agent/src/utils/jj.ts | 24 +++++ .../test/autoresearch-git.test.ts | 95 +++++++++++++++++++ .../coding-agent/test/task/worktree.test.ts | 30 ++++++ packages/coding-agent/test/utils/jj.test.ts | 64 +++++++++++++ 7 files changed, 229 insertions(+), 3 deletions(-) create mode 100644 packages/coding-agent/test/autoresearch-git.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 7f006ac98..65da37cfb 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fixed `@`-mention auto-read injecting an unrelated, same-named file when a mention did not point at a real path — e.g. an npm scope like `@scope/`, a partial path, or a bare token. `generateFileMentionMessages` resolution previously fell back to prefix and repo-wide fuzzy matching (globbing the whole project on every such mention) and auto-read the single "best" guess. Resolution is now exact-only: a mention is auto-read only when it resolves to an existing file or directory; otherwise it is left as prose. The TUI `@`-selector already inserts the real, complete path before send, so post-send guessing was both unnecessary and the source of the wrong-file reads. Directories still resolve and are listed. Removes the per-mention `**/*` project scan. +- Fixed Git-mutating automation silently mis-handling pure Jujutsu workspaces (`.jj/repo/` present, no colocated `.git/`). `task/worktree.ts#getRepoRoot` previously threw a generic "Git repository not found" for isolated subagent setup, and `autoresearch/git.ts#ensureAutoresearchBranch` returned a soft "Not in a git repository" warning that let `/autoresearch` proceed with no branch isolation or auto-commits. Both paths now detect a pure jj workspace via the new `jj.isPureJjRepo` helper and surface an actionable Jujutsu-specific error pointing at `jj git init --colocate`. Colocated jj-git (both `.jj/` and `.git/` at the same root) and plain Git checkouts behave exactly as before ([#1935](https://github.com/can1357/oh-my-pi/issues/1935)). ## [15.9.2] - 2026-06-05 diff --git a/packages/coding-agent/src/autoresearch/git.ts b/packages/coding-agent/src/autoresearch/git.ts index 95c54700a..99b524aa3 100644 --- a/packages/coding-agent/src/autoresearch/git.ts +++ b/packages/coding-agent/src/autoresearch/git.ts @@ -1,5 +1,6 @@ import type { ExtensionAPI } from "../extensibility/extensions"; import * as git from "../utils/git"; +import * as jj from "../utils/jj"; import { normalizePathSpec } from "./helpers"; const AUTORESEARCH_BRANCH_PREFIX = "autoresearch/"; @@ -39,6 +40,12 @@ export async function ensureAutoresearchBranch( ): Promise { const repoRoot = await git.repo.root(workDir); if (!repoRoot) { + if (await jj.isPureJjRepo(workDir)) { + return { + ok: false, + error: "Autoresearch needs a Git checkout for branch isolation and baseline commits, but this workspace is pure Jujutsu (`.jj/` without a colocated `.git/`). Run `jj git init --colocate` to add a Git checkout before starting autoresearch.", + }; + } return { ok: true, branchName: null, diff --git a/packages/coding-agent/src/task/worktree.ts b/packages/coding-agent/src/task/worktree.ts index 7bca9c163..151f89fd2 100644 --- a/packages/coding-agent/src/task/worktree.ts +++ b/packages/coding-agent/src/task/worktree.ts @@ -5,6 +5,7 @@ import * as path from "node:path"; import * as natives from "@oh-my-pi/pi-natives"; import { getWorktreeDir, hashPath, logger, Snowflake } from "@oh-my-pi/pi-utils"; import * as git from "../utils/git"; +import * as jj from "../utils/jj"; const { IsoBackendKind } = natives; type IsoBackendKind = natives.IsoBackendKind; @@ -28,11 +29,15 @@ export interface WorktreeBaseline { export async function getRepoRoot(cwd: string): Promise { const repoRoot = await git.repo.root(cwd); - if (!repoRoot) { - throw new Error("Git repository not found for isolated task execution."); + if (repoRoot) return repoRoot; + + if (await jj.isPureJjRepo(cwd)) { + throw new Error( + "Isolated task execution requires a Git checkout, but this workspace is pure Jujutsu (`.jj/` without a colocated `.git/`). Run `jj git init --colocate` to add a Git checkout, or set `task.isolation.mode: none` to disable task isolation.", + ); } - return repoRoot; + throw new Error("Git repository not found for isolated task execution."); } const GIT_NO_INDEX_NULL_PATH = process.platform === "win32" ? "NUL" : "/dev/null"; diff --git a/packages/coding-agent/src/utils/jj.ts b/packages/coding-agent/src/utils/jj.ts index 228325bff..21504cf08 100644 --- a/packages/coding-agent/src/utils/jj.ts +++ b/packages/coding-agent/src/utils/jj.ts @@ -2,6 +2,7 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; import { $which } from "@oh-my-pi/pi-utils"; import { LRUCache } from "lru-cache/raw"; +import * as git from "./git"; // ════════════════════════════════════════════════════════════════════════════ // Types @@ -246,3 +247,26 @@ export const repo = { return (await repo.root(cwd)) !== null; }, }; + +/** + * Detect a "pure" Jujutsu workspace — one whose root has no colocated Git + * checkout. Git-mutating automation MUST treat this case specially: invoking + * `git checkout -b`, `git worktree add`, or `git apply` against a pure jj + * workspace either fails outright (no `.git/` present) or mutates state that + * jj itself cannot reconcile. + * + * Returns `true` when `cwd` resolves to a jj workspace root that has no + * matching Git checkout. Returns `false` for plain Git checkouts, for + * colocated jj-git workspaces (created via `jj git init --colocate`), and + * for directories backed by neither tool. + */ +export async function isPureJjRepo(cwd: string): Promise { + const jjRoot = await repo.root(cwd); + if (jjRoot === null) return false; + const gitRoot = await git.repo.root(cwd); + if (gitRoot === null) return true; + // Colocated only when the resolved roots match. A jj workspace nested + // inside an unrelated Git checkout is still "pure jj" at its own root — + // Git automation against the outer checkout would silently bypass jj. + return path.resolve(jjRoot) !== path.resolve(gitRoot); +} diff --git a/packages/coding-agent/test/autoresearch-git.test.ts b/packages/coding-agent/test/autoresearch-git.test.ts new file mode 100644 index 000000000..4e6d72ba4 --- /dev/null +++ b/packages/coding-agent/test/autoresearch-git.test.ts @@ -0,0 +1,95 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { ensureAutoresearchBranch } from "../src/autoresearch/git"; +import type { ExtensionAPI } from "../src/extensibility/extensions"; +import * as jj from "../src/utils/jj"; + +const tempDirs: string[] = []; + +async function mkTempDir(prefix: string): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +async function runGit(cwd: string, args: string[]): Promise { + const env = { ...process.env, HOME: cwd, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_SYSTEM: "/dev/null" }; + const proc = Bun.spawn(["git", "-C", cwd, ...args], { env, stdout: "ignore", stderr: "pipe" }); + const code = await proc.exited; + if (code !== 0) { + const stderr = await new Response(proc.stderr).text(); + throw new Error(`git ${args.join(" ")} failed (${code}): ${stderr}`); + } +} + +async function initGitWithCommit(dir: string): Promise { + await runGit(dir, ["init", "-q", "-b", "main"]); + await runGit(dir, ["config", "user.email", "test@example.com"]); + await runGit(dir, ["config", "user.name", "Test"]); + await fs.writeFile(path.join(dir, "README"), "seed\n"); + await runGit(dir, ["add", "."]); + await runGit(dir, ["commit", "-q", "-m", "init"]); +} + +// `ensureAutoresearchBranch` never invokes the `api` it receives — its two +// internal helpers (`readGitWorkDirPrefix`, `branchExists`) immediately +// `void api`. The stub keeps the signature honest without spinning up the +// real extension runtime. +const stubApi = {} as unknown as ExtensionAPI; + +afterEach(async () => { + jj.repo.clearRootCache(); + await Promise.all(tempDirs.splice(0).map(dir => fs.rm(dir, { recursive: true, force: true }))); +}); + +describe("ensureAutoresearchBranch jj guardrails", () => { + it("rejects pure jj workspaces before touching git state", async () => { + const dir = await mkTempDir("omp-ar-purejj-"); + await fs.mkdir(path.join(dir, ".jj", "repo", "store"), { recursive: true }); + + const result = await ensureAutoresearchBranch(stubApi, dir, "demo"); + + expect(result.ok).toBe(false); + if (result.ok) throw new Error("unreachable"); + expect(result.error).toMatch(/pure Jujutsu/); + expect(result.error).toMatch(/jj git init --colocate/); + }); + + it("creates an autoresearch branch in a colocated jj-git workspace", async () => { + const dir = await mkTempDir("omp-ar-colocated-"); + await initGitWithCommit(dir); + await fs.mkdir(path.join(dir, ".jj", "repo", "store"), { recursive: true }); + + const result = await ensureAutoresearchBranch(stubApi, dir, "demo goal"); + + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("unreachable"); + expect(result.created).toBe(true); + expect(result.branchName).toMatch(/^autoresearch\/demo-goal-\d{8}$/); + }); + + it("creates an autoresearch branch in a plain git repo (unchanged behavior)", async () => { + const dir = await mkTempDir("omp-ar-plaingit-"); + await initGitWithCommit(dir); + + const result = await ensureAutoresearchBranch(stubApi, dir, "demo"); + + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("unreachable"); + expect(result.created).toBe(true); + expect(result.branchName).toMatch(/^autoresearch\/demo-\d{8}$/); + }); + + it("returns the soft no-git warning for directories backed by neither tool", async () => { + const dir = await mkTempDir("omp-ar-empty-"); + + const result = await ensureAutoresearchBranch(stubApi, dir, "demo"); + + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("unreachable"); + expect(result.branchName).toBeNull(); + expect(result.warning).toMatch(/Not in a git repository/); + }); +}); diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index 9fbdca9a7..605b25571 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -3,11 +3,13 @@ import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import * as natives from "@oh-my-pi/pi-natives"; +import * as jj from "../../src/utils/jj"; import { captureBaseline, captureDeltaPatch, ensureIsolation, getGitNoIndexNullPath, + getRepoRoot, mergeTaskBranches, parseIsolationMode, } from "../../src/task/worktree"; @@ -50,6 +52,7 @@ async function createGitRepo(): Promise<{ baseBranch: string; repo: string }> { afterEach(async () => { vi.restoreAllMocks(); + jj.repo.clearRootCache(); await Promise.all(tempDirs.splice(0).map(dir => fs.rm(dir, { recursive: true, force: true }))); }); @@ -157,3 +160,30 @@ describe("worktree isolation helpers", () => { expect(delta.rootPatch).not.toContain("preexisting.txt"); }); }); + +describe("getRepoRoot", () => { + it("returns the git root for a plain git checkout", async () => { + const { repo } = await createGitRepo(); + expect(await getRepoRoot(repo)).toBe(repo); + }); + + it("returns the git root for a colocated jj-git workspace", async () => { + const { repo } = await createGitRepo(); + await fs.mkdir(path.join(repo, ".jj", "repo", "store"), { recursive: true }); + expect(await getRepoRoot(repo)).toBe(repo); + }); + + it("rejects pure jj workspaces with an actionable Jujutsu message", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-purejj-")); + tempDirs.push(dir); + await fs.mkdir(path.join(dir, ".jj", "repo", "store"), { recursive: true }); + await expect(getRepoRoot(dir)).rejects.toThrow(/pure Jujutsu/); + await expect(getRepoRoot(dir)).rejects.toThrow(/jj git init --colocate/); + }); + + it("preserves the generic git-not-found error for directories without any repo", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-norepo-")); + tempDirs.push(dir); + await expect(getRepoRoot(dir)).rejects.toThrow("Git repository not found for isolated task execution."); + }); +}); \ No newline at end of file diff --git a/packages/coding-agent/test/utils/jj.test.ts b/packages/coding-agent/test/utils/jj.test.ts index ec85cee45..ad8ceba08 100644 --- a/packages/coding-agent/test/utils/jj.test.ts +++ b/packages/coding-agent/test/utils/jj.test.ts @@ -77,3 +77,67 @@ describe("jj workspace detection", () => { expect(resolved?.storeDir).toBe(path.join(dir, ".jj", "repo", "store")); }); }); + +describe("isPureJjRepo", () => { + const tempDirs: string[] = []; + + afterEach(async () => { + jj.repo.clearRootCache(); + await Promise.all(tempDirs.splice(0).map(dir => fs.rm(dir, { recursive: true, force: true }))); + }); + + async function createTempDir(prefix: string): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + tempDirs.push(dir); + return dir; + } + + async function initGit(dir: string): Promise { + const env = { ...process.env, HOME: dir, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_SYSTEM: "/dev/null" }; + const exit = async (args: string[]) => { + const proc = Bun.spawn(["git", "-C", dir, ...args], { env, stdout: "ignore", stderr: "pipe" }); + const code = await proc.exited; + if (code !== 0) { + const stderr = await new Response(proc.stderr).text(); + throw new Error(`git ${args.join(" ")} failed (${code}): ${stderr}`); + } + }; + await exit(["init", "-q", "-b", "main"]); + await exit(["config", "user.email", "test@example.com"]); + await exit(["config", "user.name", "Test"]); + } + + it("flags a pure jj workspace (no colocated git)", async () => { + const dir = await createTempDir("omp-jj-pure-"); + await fs.mkdir(path.join(dir, ".jj", "repo", "store"), { recursive: true }); + expect(await jj.isPureJjRepo(dir)).toBe(true); + }); + + it("treats a colocated jj-git workspace as non-pure", async () => { + const dir = await createTempDir("omp-jj-colocated-"); + await fs.mkdir(path.join(dir, ".jj", "repo", "store"), { recursive: true }); + await initGit(dir); + expect(await jj.isPureJjRepo(dir)).toBe(false); + }); + + it("returns false for a plain git checkout (no jj metadata)", async () => { + const dir = await createTempDir("omp-jj-plaingit-"); + await initGit(dir); + expect(await jj.isPureJjRepo(dir)).toBe(false); + }); + + it("returns false when neither jj nor git metadata is present", async () => { + const dir = await createTempDir("omp-jj-empty-"); + expect(await jj.isPureJjRepo(dir)).toBe(false); + }); + + it("flags a jj workspace nested inside an unrelated git checkout as pure", async () => { + const outer = await createTempDir("omp-jj-nested-outer-"); + await initGit(outer); + const inner = path.join(outer, "nested"); + await fs.mkdir(path.join(inner, ".jj", "repo", "store"), { recursive: true }); + // The inner directory is its own jj workspace; the surrounding git + // checkout would mutate state outside jj's model. + expect(await jj.isPureJjRepo(inner)).toBe(true); + }); +}); \ No newline at end of file From 2dcb0c8aba61ab943d110a668658fb8cd2d3c771 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 5 Jun 2026 14:39:44 +0000 Subject: [PATCH 2/4] style: bun run fix --- packages/coding-agent/test/task/worktree.test.ts | 4 ++-- packages/coding-agent/test/utils/jj.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index 605b25571..dee22671d 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -3,7 +3,6 @@ import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import * as natives from "@oh-my-pi/pi-natives"; -import * as jj from "../../src/utils/jj"; import { captureBaseline, captureDeltaPatch, @@ -13,6 +12,7 @@ import { mergeTaskBranches, parseIsolationMode, } from "../../src/task/worktree"; +import * as jj from "../../src/utils/jj"; const tempDirs: string[] = []; @@ -186,4 +186,4 @@ describe("getRepoRoot", () => { tempDirs.push(dir); await expect(getRepoRoot(dir)).rejects.toThrow("Git repository not found for isolated task execution."); }); -}); \ No newline at end of file +}); diff --git a/packages/coding-agent/test/utils/jj.test.ts b/packages/coding-agent/test/utils/jj.test.ts index ad8ceba08..11e46db2d 100644 --- a/packages/coding-agent/test/utils/jj.test.ts +++ b/packages/coding-agent/test/utils/jj.test.ts @@ -140,4 +140,4 @@ describe("isPureJjRepo", () => { // checkout would mutate state outside jj's model. expect(await jj.isPureJjRepo(inner)).toBe(true); }); -}); \ No newline at end of file +}); From d7c7d2b0e159c4b20d19add5773627d141b9c74b Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 5 Jun 2026 14:44:05 +0000 Subject: [PATCH 3/4] fix(coding-agent): guarded nested pure jj before outer-git fallback Previously `getRepoRoot` and `ensureAutoresearchBranch` only consulted `jj.isPureJjRepo` after `git.repo.root` returned null. For a jj workspace nested under an unrelated outer Git checkout, `git.repo.root` walks up and finds the outer .git, so the pure-jj branch never fired and isolation/autoresearch silently mutated the surrounding Git tree behind jj's back. Both call sites now run the pure-jj guard first, rejecting at the jj workspace's own root regardless of any surrounding Git checkout. Added integration tests for the nested case on both surfaces. Refs #1935 --- packages/coding-agent/src/autoresearch/git.ts | 17 +++++++++++------ packages/coding-agent/src/task/worktree.ts | 9 ++++++--- .../coding-agent/test/autoresearch-git.test.ts | 18 ++++++++++++++++++ .../coding-agent/test/task/worktree.test.ts | 12 ++++++++++++ 4 files changed, 47 insertions(+), 9 deletions(-) diff --git a/packages/coding-agent/src/autoresearch/git.ts b/packages/coding-agent/src/autoresearch/git.ts index 99b524aa3..43cb0e886 100644 --- a/packages/coding-agent/src/autoresearch/git.ts +++ b/packages/coding-agent/src/autoresearch/git.ts @@ -38,14 +38,19 @@ export async function ensureAutoresearchBranch( workDir: string, goal: string | null, ): Promise { + // Pure-jj check runs first so a jj workspace nested under an unrelated + // outer Git checkout is rejected at its own root rather than silently + // creating `autoresearch/*` branches and commits in the surrounding Git + // tree behind jj's back. + if (await jj.isPureJjRepo(workDir)) { + return { + ok: false, + error: "Autoresearch needs a Git checkout for branch isolation and baseline commits, but this workspace is pure Jujutsu (`.jj/` without a colocated `.git/`). Run `jj git init --colocate` to add a Git checkout before starting autoresearch.", + }; + } + const repoRoot = await git.repo.root(workDir); if (!repoRoot) { - if (await jj.isPureJjRepo(workDir)) { - return { - ok: false, - error: "Autoresearch needs a Git checkout for branch isolation and baseline commits, but this workspace is pure Jujutsu (`.jj/` without a colocated `.git/`). Run `jj git init --colocate` to add a Git checkout before starting autoresearch.", - }; - } return { ok: true, branchName: null, diff --git a/packages/coding-agent/src/task/worktree.ts b/packages/coding-agent/src/task/worktree.ts index 151f89fd2..9d2f2838e 100644 --- a/packages/coding-agent/src/task/worktree.ts +++ b/packages/coding-agent/src/task/worktree.ts @@ -28,15 +28,18 @@ export interface WorktreeBaseline { } export async function getRepoRoot(cwd: string): Promise { - const repoRoot = await git.repo.root(cwd); - if (repoRoot) return repoRoot; - + // Pure-jj check runs first so a jj workspace nested under an unrelated + // outer Git checkout is rejected at its own root rather than silently + // mutating the surrounding Git tree behind jj's back. if (await jj.isPureJjRepo(cwd)) { throw new Error( "Isolated task execution requires a Git checkout, but this workspace is pure Jujutsu (`.jj/` without a colocated `.git/`). Run `jj git init --colocate` to add a Git checkout, or set `task.isolation.mode: none` to disable task isolation.", ); } + const repoRoot = await git.repo.root(cwd); + if (repoRoot) return repoRoot; + throw new Error("Git repository not found for isolated task execution."); } diff --git a/packages/coding-agent/test/autoresearch-git.test.ts b/packages/coding-agent/test/autoresearch-git.test.ts index 4e6d72ba4..3d242d79a 100644 --- a/packages/coding-agent/test/autoresearch-git.test.ts +++ b/packages/coding-agent/test/autoresearch-git.test.ts @@ -92,4 +92,22 @@ describe("ensureAutoresearchBranch jj guardrails", () => { expect(result.branchName).toBeNull(); expect(result.warning).toMatch(/Not in a git repository/); }); + + it("rejects a pure jj workspace nested inside an unrelated outer git checkout", async () => { + // `git.repo.root(inner)` walks up and finds the outer .git — without + // the pure-jj check running first, autoresearch would create + // `autoresearch/*` branches and commits in the surrounding git tree + // behind jj's back. + const outer = await mkTempDir("omp-ar-nested-outer-"); + await initGitWithCommit(outer); + const inner = path.join(outer, "nested-jj"); + await fs.mkdir(path.join(inner, ".jj", "repo", "store"), { recursive: true }); + + const result = await ensureAutoresearchBranch(stubApi, inner, "demo"); + + expect(result.ok).toBe(false); + if (result.ok) throw new Error("unreachable"); + expect(result.error).toMatch(/pure Jujutsu/); + expect(result.error).toMatch(/jj git init --colocate/); + }); }); diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index dee22671d..e0c0c9239 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -186,4 +186,16 @@ describe("getRepoRoot", () => { tempDirs.push(dir); await expect(getRepoRoot(dir)).rejects.toThrow("Git repository not found for isolated task execution."); }); + + it("rejects a pure jj workspace nested inside an unrelated outer git checkout", async () => { + // `git.repo.root(inner)` walks up and finds the outer .git — without + // the pure-jj check running first, isolation would silently target the + // surrounding git tree behind jj's back. + const { repo: outer } = await createGitRepo(); + const inner = path.join(outer, "nested-jj"); + await fs.mkdir(path.join(inner, ".jj", "repo", "store"), { recursive: true }); + + await expect(getRepoRoot(inner)).rejects.toThrow(/pure Jujutsu/); + await expect(getRepoRoot(inner)).rejects.toThrow(/jj git init --colocate/); + }); }); From b6aafab3c1532cd9a455f18a75f5a0a803e0bdea Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 5 Jun 2026 14:49:44 +0000 Subject: [PATCH 4/4] fix(coding-agent): made isPureJjRepo depth-aware so nested git checkouts win MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously `isPureJjRepo` returned true whenever the resolved jj and git roots merely differed. That punishes the legitimate inverse-nesting case: a real git checkout (vendored repo, fixture, independent nested checkout) living UNDER an outer pure jj workspace. Both `jj.repo.root` and `git.repo.root` walk upward from cwd and return the closest ancestor — so the deeper root is the one the user is actually working inside, and Git automation against the inner checkout never touches the surrounding jj tree. `isPureJjRepo` now returns true iff jj is the *deeper* ancestor (or no git is present at all). The new `isStrictDescendant` helper does the depth check via `path.relative`. Added unit + integration tests covering both nesting directions on all three surfaces (`utils/jj`, `task/worktree`, `autoresearch/git`). Refs #1935 --- packages/coding-agent/src/utils/jj.ts | 49 ++++++++++++++----- .../test/autoresearch-git.test.ts | 19 +++++++ .../coding-agent/test/task/worktree.test.ts | 17 +++++++ packages/coding-agent/test/utils/jj.test.ts | 12 +++++ 4 files changed, 84 insertions(+), 13 deletions(-) diff --git a/packages/coding-agent/src/utils/jj.ts b/packages/coding-agent/src/utils/jj.ts index 21504cf08..655decf97 100644 --- a/packages/coding-agent/src/utils/jj.ts +++ b/packages/coding-agent/src/utils/jj.ts @@ -249,24 +249,47 @@ export const repo = { }; /** - * Detect a "pure" Jujutsu workspace — one whose root has no colocated Git - * checkout. Git-mutating automation MUST treat this case specially: invoking - * `git checkout -b`, `git worktree add`, or `git apply` against a pure jj - * workspace either fails outright (no `.git/` present) or mutates state that - * jj itself cannot reconcile. + * Detect a "pure" Jujutsu workspace — one where Git-mutating automation has + * no safe Git target. Invoking `git checkout -b`, `git worktree add`, or + * `git apply` against a pure jj workspace either fails outright (no `.git/` + * present) or mutates state that jj itself cannot reconcile. * - * Returns `true` when `cwd` resolves to a jj workspace root that has no - * matching Git checkout. Returns `false` for plain Git checkouts, for - * colocated jj-git workspaces (created via `jj git init --colocate`), and - * for directories backed by neither tool. + * `cwd` is "pure jj" iff its nearest jj workspace ancestor is **closer than** + * its nearest Git checkout ancestor (or no Git checkout is present at all). + * Both lookups walk upward from `cwd`, so the deeper ancestor is the one the + * user is actually working inside. + * + * Returns: + * - `false` for plain Git checkouts (no jj metadata anywhere up the tree). + * - `false` for colocated jj-git workspaces — `jj git init --colocate` keeps + * `.jj/` and `.git/` at the same root. + * - `false` when a nested Git checkout (e.g. a vendored repo or fixture) + * lives **under** an outer jj workspace; Git automation targets the inner + * repo and never touches the surrounding jj tree. + * - `true` when jj is the deeper ancestor — either a standalone pure jj + * workspace, or a jj workspace nested under an unrelated outer Git + * checkout, where Git automation against the outer root would silently + * bypass jj. + * - `false` for directories backed by neither tool. */ export async function isPureJjRepo(cwd: string): Promise { const jjRoot = await repo.root(cwd); if (jjRoot === null) return false; const gitRoot = await git.repo.root(cwd); if (gitRoot === null) return true; - // Colocated only when the resolved roots match. A jj workspace nested - // inside an unrelated Git checkout is still "pure jj" at its own root — - // Git automation against the outer checkout would silently bypass jj. - return path.resolve(jjRoot) !== path.resolve(gitRoot); + return isStrictDescendant(path.resolve(jjRoot), path.resolve(gitRoot)); +} + +/** + * Return `true` when `child` is a strict descendant of `ancestor` (same path + * counts as `false`). Both arguments must already be resolved absolute paths. + */ +function isStrictDescendant(child: string, ancestor: string): boolean { + const rel = path.relative(ancestor, child); + if (rel === "" || rel === ".") return false; + if (rel.startsWith("..")) return false; + // `path.relative` returns an absolute path only when the two arguments + // live on different filesystem roots (Windows drives, UNC shares); not a + // real ancestor relationship. + return !path.isAbsolute(rel); } diff --git a/packages/coding-agent/test/autoresearch-git.test.ts b/packages/coding-agent/test/autoresearch-git.test.ts index 3d242d79a..503c2926e 100644 --- a/packages/coding-agent/test/autoresearch-git.test.ts +++ b/packages/coding-agent/test/autoresearch-git.test.ts @@ -110,4 +110,23 @@ describe("ensureAutoresearchBranch jj guardrails", () => { expect(result.error).toMatch(/pure Jujutsu/); expect(result.error).toMatch(/jj git init --colocate/); }); + + it("creates an autoresearch branch in a nested git checkout under an outer jj workspace", async () => { + // Mirror image of the case above: `jj.repo.root(inner)` finds the outer + // .jj, but `git.repo.root(inner)` finds the inner .git, so autoresearch + // safely targets the nested checkout and never touches the surrounding + // jj tree. + const outer = await mkTempDir("omp-ar-outerjj-"); + await fs.mkdir(path.join(outer, ".jj", "repo", "store"), { recursive: true }); + const inner = path.join(outer, "vendor"); + await fs.mkdir(inner, { recursive: true }); + await initGitWithCommit(inner); + + const result = await ensureAutoresearchBranch(stubApi, inner, "demo"); + + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("unreachable"); + expect(result.created).toBe(true); + expect(result.branchName).toMatch(/^autoresearch\/demo-\d{8}$/); + }); }); diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index e0c0c9239..69eb24959 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -198,4 +198,21 @@ describe("getRepoRoot", () => { await expect(getRepoRoot(inner)).rejects.toThrow(/pure Jujutsu/); await expect(getRepoRoot(inner)).rejects.toThrow(/jj git init --colocate/); }); + + it("returns the nested git root when a git checkout lives under an outer jj workspace", async () => { + // Mirror image of the case above: `jj.repo.root(inner)` finds the outer + // .jj, but `git.repo.root(inner)` finds the inner .git, so Git + // automation targets the nested checkout safely. Isolation must keep + // working here exactly as it did before the pure-jj guard landed. + const outer = await fs.mkdtemp(path.join(os.tmpdir(), "omp-outerjj-")); + tempDirs.push(outer); + await fs.mkdir(path.join(outer, ".jj", "repo", "store"), { recursive: true }); + const inner = path.join(outer, "vendor"); + await fs.mkdir(inner, { recursive: true }); + await runGit(inner, ["init", "-q", "-b", "main"]); + await runGit(inner, ["config", "user.email", "test@example.com"]); + await runGit(inner, ["config", "user.name", "Test"]); + + expect(await getRepoRoot(inner)).toBe(inner); + }); }); diff --git a/packages/coding-agent/test/utils/jj.test.ts b/packages/coding-agent/test/utils/jj.test.ts index 11e46db2d..c4fc83153 100644 --- a/packages/coding-agent/test/utils/jj.test.ts +++ b/packages/coding-agent/test/utils/jj.test.ts @@ -140,4 +140,16 @@ describe("isPureJjRepo", () => { // checkout would mutate state outside jj's model. expect(await jj.isPureJjRepo(inner)).toBe(true); }); + + it("treats a nested git checkout under an outer jj workspace as non-pure", async () => { + // `git.repo.root(inner)` returns the inner .git, so Git automation + // targets the nested checkout safely and never touches the surrounding + // jj tree — the inner git wins. + const outer = await createTempDir("omp-jj-nested-jj-outer-"); + await fs.mkdir(path.join(outer, ".jj", "repo", "store"), { recursive: true }); + const inner = path.join(outer, "vendor"); + await fs.mkdir(inner, { recursive: true }); + await initGit(inner); + expect(await jj.isPureJjRepo(inner)).toBe(false); + }); });