Merge PR #1939: fix(coding-agent): guard Git-mutating automation in pure jj workspaces (@roboomp)

# Conflicts:
#	packages/coding-agent/src/task/worktree.ts
#	packages/coding-agent/test/task/worktree.test.ts
This commit is contained in:
can1357
2026-06-21 17:05:15 +02:00
7 changed files with 361 additions and 4 deletions
+1
View File
@@ -1871,6 +1871,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
@@ -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/";
@@ -37,6 +38,17 @@ export async function ensureAutoresearchBranch(
workDir: string,
goal: string | null,
): Promise<EnsureAutoresearchBranchResult> {
// 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) {
return {
+12 -4
View File
@@ -6,6 +6,7 @@ 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 { mapWithConcurrencyLimit } from "./parallel";
import * as jj from "../utils/jj";
const { IsoBackendKind } = natives;
type IsoBackendKind = natives.IsoBackendKind;
@@ -28,12 +29,19 @@ export interface WorktreeBaseline {
}
export async function getRepoRoot(cwd: string): Promise<string> {
const repoRoot = await git.repo.root(cwd);
if (!repoRoot) {
throw new Error("Git repository not found for isolated task execution.");
// 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.",
);
}
return repoRoot;
const repoRoot = await git.repo.root(cwd);
if (repoRoot) 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";
+47
View File
@@ -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,49 @@ export const repo = {
return (await repo.root(cwd)) !== null;
},
};
/**
* 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.
*
* `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<boolean> {
const jjRoot = await repo.root(cwd);
if (jjRoot === null) return false;
const gitRoot = await git.repo.root(cwd);
if (gitRoot === null) return true;
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);
}
@@ -0,0 +1,132 @@
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<string> {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix));
tempDirs.push(dir);
return dir;
}
async function runGit(cwd: string, args: string[]): Promise<void> {
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<void> {
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/);
});
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/);
});
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}$/);
});
});
@@ -7,10 +7,14 @@ import {
captureDeltaPatch,
ensureIsolation,
getGitNoIndexNullPath,
getRepoRoot,
mergeTaskBranches,
parseIsolationMode,
} from "@oh-my-pi/pi-coding-agent/task/worktree";
import * as natives from "@oh-my-pi/pi-natives";
import * as jj from "@oh-my-pi/pi-coding-agent/utils/jj";
const tempDirs: string[] = [];
async function runGit(repo: string, args: string[]): Promise<string> {
const proc = Bun.spawn(["git", ...args], {
@@ -30,6 +34,27 @@ async function runGit(repo: string, args: string[]): Promise<string> {
return stdout.trim();
}
async function createGitRepo(): Promise<{ baseBranch: string; repo: string }> {
const repo = await fs.mkdtemp(path.join(os.tmpdir(), "omp-worktree-"));
tempDirs.push(repo);
await runGit(repo, ["init"]);
await runGit(repo, ["config", "user.email", "test@example.com"]);
await runGit(repo, ["config", "user.name", "Test User"]);
await fs.writeFile(path.join(repo, "merged.txt"), "base version\n");
await fs.writeFile(path.join(repo, "staged.txt"), "base staged\n");
await runGit(repo, ["add", "."]);
await runGit(repo, ["commit", "-m", "initial"]);
return {
baseBranch: await runGit(repo, ["branch", "--show-current"]),
repo,
};
}
afterEach(async () => {
vi.restoreAllMocks();
jj.repo.clearRootCache();
await Promise.all(tempDirs.splice(0).map(dir => fs.rm(dir, { recursive: true, force: true })));
});
describe("worktree isolation helpers", () => {
it("returns platform-specific null path for git --no-index diffs", () => {
const expected = process.platform === "win32" ? "NUL" : "/dev/null";
@@ -198,3 +223,59 @@ describe("worktree isolation helpers", () => {
});
});
});
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.");
});
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/);
});
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);
});
});
@@ -77,3 +77,79 @@ 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<string> {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix));
tempDirs.push(dir);
return dir;
}
async function initGit(dir: string): Promise<void> {
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);
});
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);
});
});