diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6674d348f..cde28978f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- Added `isolated`, `apply`, and `merge` options to eval `agent()` across every workflow runtime (Python, JavaScript, Ruby, Julia) so `workflowz`-driven fan-outs can request the same copy-on-write worktree isolation the `task` tool offers (strict opt-in via `isolated: true`, matching the `task` tool; `apply: false` keeps captured patches/branches without merging back; `merge: false` forces patch mode). Extracted the task-isolation lifecycle into `task/isolation-runner.ts` so the eval bridge and `TaskTool` share one implementation ([#3196](https://github.com/can1357/oh-my-pi/issues/3196)) + ### Fixed - Fixed streaming output blocks incorrectly calculating preview height, preventing flickering banners diff --git a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts index f859d4aac..7c6b0282f 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -1,4 +1,5 @@ import { afterAll, afterEach, describe, expect, it, vi } from "bun:test"; +import * as fs from "node:fs/promises"; import * as path from "node:path"; import { TempDir } from "@oh-my-pi/pi-utils"; import { Settings } from "../../config/settings"; @@ -6,6 +7,7 @@ import type { PlanModeState } from "../../plan-mode/state"; import * as taskDiscovery from "../../task/discovery"; import type { ExecutorOptions } from "../../task/executor"; import * as taskExecutor from "../../task/executor"; +import * as isolationRunner from "../../task/isolation-runner"; import { AgentOutputManager } from "../../task/output-manager"; import type { AgentDefinition, AgentProgress, SingleResult } from "../../task/types"; import type { ToolSession } from "../../tools"; @@ -767,3 +769,412 @@ describe("agent() through eval runtimes", () => { expect(idle.signal.aborted).toBe(false); }); }); + +describe("runEvalAgent isolation", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + function isolatedSession(overrides: Partial[0]> = {}): ToolSession { + return makeSession({ + settings: Settings.isolated({ + "async.enabled": false, + "task.isolation.mode": "auto", + "task.isolation.merge": "patch", + ...overrides, + }), + }); + } + + function mockIsolationContext(): { repoRoot: string } { + const repoRoot = "/repo-root"; + vi.spyOn(isolationRunner, "prepareIsolationContext").mockResolvedValue({ + repoRoot, + baseline: { + root: { repoRoot, headCommit: "HEAD", staged: "", unstaged: "", untracked: [], untrackedPatch: "" }, + nested: [], + }, + }); + return { repoRoot }; + } + + it("rejects isolated=true when task.isolation.mode is 'none'", async () => { + mockAgents(); + const runSpy = vi.spyOn(taskExecutor, "runSubprocess").mockImplementation(async options => singleResult(options)); + const prepSpy = vi.spyOn(isolationRunner, "prepareIsolationContext"); + + const session = makeSession(); // default settings: isolation.mode === "none" + + await expect(runEvalAgent({ prompt: "do work", isolated: true }, { session })).rejects.toThrow( + 'task.isolation.mode to be set; current mode is "none"', + ); + expect(prepSpy).not.toHaveBeenCalled(); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("stays non-isolated by default even when task.isolation.mode is set; isolated=true opts in", async () => { + mockAgents(); + mockIsolationContext(); + const isolatedSpy = vi + .spyOn(isolationRunner, "runIsolatedSubprocess") + .mockImplementation(async opts => singleResult(opts.baseOptions, { output: "isolated-run" })); + const plainSpy = vi + .spyOn(taskExecutor, "runSubprocess") + .mockImplementation(async options => singleResult(options, { output: "plain-run" })); + const mergeSpy = vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockResolvedValue({ + summary: "", + changesApplied: true, + hadAnyChanges: false, + mergedBranchForNestedPatches: false, + }); + + // Default (no isolated arg) — stays non-isolated even when settings allow it. + const defaultResult = await runEvalAgent({ prompt: "default" }, { session: isolatedSession() }); + expect(plainSpy).toHaveBeenCalledTimes(1); + expect(isolatedSpy).not.toHaveBeenCalled(); + expect(defaultResult.details.isolated).toBeUndefined(); + expect(defaultResult.details.changesApplied).toBeUndefined(); + expect(mergeSpy).not.toHaveBeenCalled(); + + // Explicit isolated=true — opt-in turns it on and surfaces merge details. + const explicitOn = await runEvalAgent({ prompt: "on", isolated: true }, { session: isolatedSession() }); + expect(isolatedSpy).toHaveBeenCalledTimes(1); + expect(plainSpy).toHaveBeenCalledTimes(1); + expect(explicitOn.details.isolated).toBe(true); + expect(mergeSpy).toHaveBeenCalledTimes(1); + }); + + it("preserves temp artifacts for non-isolated returnHandle outputs", async () => { + mockAgents(); + const rmSpy = vi.spyOn(fs, "rm").mockResolvedValue(undefined); + vi.spyOn(taskExecutor, "runSubprocess").mockImplementation(async options => singleResult(options)); + + await runEvalAgent({ prompt: "plain handle", returnHandle: true }, { session: makeSession() }); + + const removedArtifactsDir = rmSpy.mock.calls.some( + ([target]) => typeof target === "string" && target.includes("omp-eval-agent-"), + ); + expect(removedArtifactsDir).toBe(false); + }); + + it("forwards merge=false as patch mode and passes the worktree cwd through baseOptions", async () => { + mockAgents(); + const { repoRoot } = mockIsolationContext(); + const isolatedSpy = vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { + output: "isolated-run", + patchPath: `/artifacts/${opts.agentId}.patch`, + }), + ); + vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockResolvedValue({ + summary: "\n\nApplied patches: yes", + changesApplied: true, + hadAnyChanges: true, + mergedBranchForNestedPatches: false, + }); + + // Branch is the configured merge mode, but `merge: false` must demote to patch. + const session = isolatedSession({ "task.isolation.merge": "branch" }); + const result = await runEvalAgent({ prompt: "migration", isolated: true, merge: false }, { session }); + + expect(isolatedSpy).toHaveBeenCalledTimes(1); + const isolatedCall = isolatedSpy.mock.calls[0]?.[0]; + if (!isolatedCall) throw new Error("runIsolatedSubprocess was not called"); + expect(isolatedCall.mergeMode).toBe("patch"); + expect(isolatedCall.baseOptions.cwd).toBe(session.cwd); + expect(isolatedCall.context.repoRoot).toBe(repoRoot); + expect(result.details.patchPath).toMatch(/\.patch$/); + expect(result.text).toContain("Applied patches: yes"); + }); + + it("keeps the timeout paused through isolation merge/apply so the cell can't abort mid-cherry-pick", async () => { + mockAgents(); + mockIsolationContext(); + const ops: string[] = []; + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => { + ops.push("subprocess"); + return singleResult(opts.baseOptions, { output: "done", patchPath: `/artifacts/${opts.agentId}.patch` }); + }); + vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockImplementation(async () => { + ops.push("merge"); + return { + summary: "\n\nMerged", + changesApplied: true, + hadAnyChanges: true, + mergedBranchForNestedPatches: false, + }; + }); + + await runEvalAgent( + { prompt: "migration", isolated: true }, + { + session: isolatedSession(), + emitStatus: event => { + if (event.op === EVAL_TIMEOUT_PAUSE_OP || event.op === EVAL_TIMEOUT_RESUME_OP) ops.push(event.op); + }, + }, + ); + + const pauseIdx = ops.indexOf(EVAL_TIMEOUT_PAUSE_OP); + const resumeIdx = ops.lastIndexOf(EVAL_TIMEOUT_RESUME_OP); + const mergeIdx = ops.indexOf("merge"); + expect(pauseIdx).toBeGreaterThanOrEqual(0); + expect(resumeIdx).toBeGreaterThan(pauseIdx); + expect(mergeIdx).toBeGreaterThan(pauseIdx); + expect(mergeIdx).toBeLessThan(resumeIdx); + }); + + it("keeps schema-backed isolated output parseable by moving merge text into details", async () => { + mockAgents(); + mockIsolationContext(); + const structuredOutput = JSON.stringify({ status: "ok" }); + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { + output: structuredOutput, + patchPath: `/artifacts/${opts.agentId}.patch`, + }), + ); + vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockResolvedValue({ + summary: "\n\nNo changes to apply.", + changesApplied: true, + hadAnyChanges: false, + mergedBranchForNestedPatches: false, + }); + + const result = await runEvalAgent( + { + prompt: "structured", + isolated: true, + schema: { + type: "object", + properties: { status: { type: "string" } }, + required: ["status"], + }, + }, + { session: isolatedSession() }, + ); + + expect(JSON.parse(result.text)).toEqual({ status: "ok" }); + expect(result.text).toBe(structuredOutput); + expect(result.details.isolationSummary).toBe("No changes to apply."); + }); + + it("throws when an isolated apply fails so schema callers cannot mistake it for success", async () => { + mockAgents(); + mockIsolationContext(); + const structuredOutput = JSON.stringify({ status: "ok" }); + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { + output: structuredOutput, + patchPath: `/artifacts/${opts.agentId}.patch`, + }), + ); + vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockResolvedValue({ + summary: "\n\nPatch apply failed: conflict in foo.ts", + changesApplied: false, + hadAnyChanges: false, + mergedBranchForNestedPatches: false, + }); + + await expect( + runEvalAgent( + { + prompt: "structured", + isolated: true, + schema: { + type: "object", + properties: { status: { type: "string" } }, + required: ["status"], + }, + }, + { session: isolatedSession() }, + ), + ).rejects.toThrow(/isolated apply failed.*Patch apply failed.*Captured patch preserved at \/artifacts\//s); + }); + + it("throws on apply failure for non-schema callers too instead of burying the warning in text", async () => { + mockAgents(); + mockIsolationContext(); + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { + output: "ran", + branchName: `omp/task/${opts.agentId}`, + }), + ); + vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockResolvedValue({ + summary: "\n\nBranch merge failed: omp/task/x.\nConflict: foo.ts", + changesApplied: false, + hadAnyChanges: false, + mergedBranchForNestedPatches: false, + }); + + const session = isolatedSession({ "task.isolation.merge": "branch" }); + await expect(runEvalAgent({ prompt: "scout", isolated: true }, { session })).rejects.toThrow( + /isolated apply failed.*Branch merge failed.*Captured branch preserved as omp\/task\//s, + ); + }); + + it("persists captured nested patches to a recoverable file before throwing on apply failure", async () => { + mockAgents(); + mockIsolationContext(); + const nestedPatch = "diff --git a/file b/file\n"; + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { + output: "ran", + patchPath: `/artifacts/${opts.agentId}.patch`, + nestedPatches: [{ relativePath: "sub/nested", patch: nestedPatch }], + }), + ); + vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockResolvedValue({ + summary: "\n\nPatch apply failed: conflict in foo.ts", + changesApplied: false, + hadAnyChanges: false, + mergedBranchForNestedPatches: false, + }); + + let caught: Error | undefined; + try { + await runEvalAgent({ prompt: "scout", isolated: true }, { session: isolatedSession() }); + } catch (err) { + caught = err as Error; + } + expect(caught).toBeDefined(); + const match = caught?.message.match(/(\/[^\s,]+?\.nested-0-sub_nested\.patch)/); + expect(match).not.toBeNull(); + const persistedPath = match?.[1]; + expect(persistedPath).toBeDefined(); + const contents = await fs.readFile(persistedPath!, "utf-8"); + expect(contents).toBe(nestedPatch); + await fs.rm(path.dirname(persistedPath!), { recursive: true, force: true }); + }); + + it("skips the merge phase when apply=false and surfaces the patch artifact instead", async () => { + mockAgents(); + mockIsolationContext(); + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { + output: "captured", + patchPath: "/artifacts/captured.patch", + }), + ); + const mergeSpy = vi.spyOn(isolationRunner, "mergeIsolatedChanges"); + + const result = await runEvalAgent( + { prompt: "scout", isolated: true, apply: false }, + { session: isolatedSession() }, + ); + + expect(mergeSpy).not.toHaveBeenCalled(); + expect(result.details.isolated).toBe(true); + expect(result.details.changesApplied).toBeNull(); + expect(result.details.patchPath).toBe("/artifacts/captured.patch"); + expect(result.text).toContain("/artifacts/captured.patch"); + expect(result.text).toContain("apply=false"); + }); + + it("surfaces a captured branch name when apply=false and the run used branch mode", async () => { + mockAgents(); + mockIsolationContext(); + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { + output: "branched", + branchName: `omp/task/${opts.agentId}`, + }), + ); + const mergeSpy = vi.spyOn(isolationRunner, "mergeIsolatedChanges"); + + const session = isolatedSession({ "task.isolation.merge": "branch" }); + const result = await runEvalAgent({ prompt: "scout", isolated: true, apply: false }, { session }); + + expect(mergeSpy).not.toHaveBeenCalled(); + expect(result.details.branchName).toMatch(/^omp\/task\//); + expect(result.text).toContain("omp/task/"); + expect(result.text).toContain("apply=false"); + }); + + it("surfaces nested patches when apply=false captured branch-mode nested-only changes", async () => { + mockAgents(); + mockIsolationContext(); + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { + output: "nested-only", + nestedPatches: [{ relativePath: "nested", patch: "diff --git a/file b/file\n" }], + }), + ); + const mergeSpy = vi.spyOn(isolationRunner, "mergeIsolatedChanges"); + + const session = isolatedSession({ "task.isolation.merge": "branch" }); + const result = await runEvalAgent({ prompt: "scout", isolated: true, apply: false }, { session }); + + expect(mergeSpy).not.toHaveBeenCalled(); + expect(result.details.branchName).toBeUndefined(); + expect(result.details.patchPath).toBeUndefined(); + expect(result.details.nestedPatches).toEqual([{ relativePath: "nested", patch: "diff --git a/file b/file\n" }]); + expect(result.text).toContain("nested repository"); + expect(result.text).toContain("apply=false"); + }); + + it("preserves the temp artifacts dir when apply=false so details.patchPath remains valid", async () => { + mockAgents(); + mockIsolationContext(); + const rmSpy = vi.spyOn(fs, "rm").mockResolvedValue(undefined); + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { output: "captured", patchPath: `/artifacts/${opts.agentId}.patch` }), + ); + + const result = await runEvalAgent( + { prompt: "scout", isolated: true, apply: false }, + { session: isolatedSession() }, + ); + + expect(result.details.patchPath).toMatch(/\.patch$/); + const removedArtifactsDir = rmSpy.mock.calls.some( + ([target]) => typeof target === "string" && target.includes("omp-eval-agent-"), + ); + expect(removedArtifactsDir).toBe(false); + }); + + it("still cleans the temp artifacts dir when apply succeeds", async () => { + mockAgents(); + mockIsolationContext(); + const rmSpy = vi.spyOn(fs, "rm").mockResolvedValue(undefined); + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { output: "captured", patchPath: `/artifacts/${opts.agentId}.patch` }), + ); + vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockResolvedValue({ + summary: "\n\nApplied", + changesApplied: true, + hadAnyChanges: true, + mergedBranchForNestedPatches: false, + }); + + await runEvalAgent({ prompt: "scout", isolated: true }, { session: isolatedSession() }); + + const removedArtifactsDir = rmSpy.mock.calls.some( + ([target]) => typeof target === "string" && target.includes("omp-eval-agent-"), + ); + expect(removedArtifactsDir).toBe(true); + }); + + it("preserves the temp artifacts dir after a successful apply when returnHandle is requested", async () => { + mockAgents(); + mockIsolationContext(); + const rmSpy = vi.spyOn(fs, "rm").mockResolvedValue(undefined); + vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => + singleResult(opts.baseOptions, { output: "captured", patchPath: `/artifacts/${opts.agentId}.patch` }), + ); + vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockResolvedValue({ + summary: "\n\nApplied", + changesApplied: true, + hadAnyChanges: true, + mergedBranchForNestedPatches: false, + }); + + await runEvalAgent({ prompt: "scout", isolated: true, returnHandle: true }, { session: isolatedSession() }); + + const removedArtifactsDir = rmSpy.mock.calls.some( + ([target]) => typeof target === "string" && target.includes("omp-eval-agent-"), + ); + expect(removedArtifactsDir).toBe(false); + }); +}); diff --git a/packages/coding-agent/src/eval/__tests__/prelude-agent.test.ts b/packages/coding-agent/src/eval/__tests__/prelude-agent.test.ts index 73db1638f..6106a168c 100644 --- a/packages/coding-agent/src/eval/__tests__/prelude-agent.test.ts +++ b/packages/coding-agent/src/eval/__tests__/prelude-agent.test.ts @@ -26,12 +26,15 @@ type AgentHelper = (prompt: string, opts?: Record) => Promise { it("returns a DAG node carrying the agent:// handle when returnHandle is set", async () => { let seenName: string | undefined; - const sandbox = loadPrelude(async name => { + let seenArgs: Record | undefined; + const sandbox = loadPrelude(async (name, args) => { seenName = name; + seenArgs = args as Record; return { text: "hello world", details: { agent: "task", id: "abc123", model: "m", structured: false } }; }); const node = await (sandbox.agent as AgentHelper)("say hi", { returnHandle: true }); expect(seenName).toBe("__agent__"); + expect(seenArgs?.returnHandle).toBe(true); expect(node).toEqual({ text: "hello world", output: "hello world", @@ -70,4 +73,35 @@ describe("eval js agent() returnHandle", () => { const node = await (sandbox.agent as AgentHelper)("x", { returnHandle: true }); expect(node).toEqual({ text: "lonely", output: "lonely", handle: null, id: null, agent: null }); }); + + it("exposes patchPath/branchName/nestedPatches/changesApplied/isolated/isolationSummary on the handle", async () => { + const payload = JSON.stringify({ ok: true }); + const sandbox = loadPrelude(async () => ({ + text: payload, + details: { + agent: "task", + id: "iso-1", + structured: true, + isolated: true, + patchPath: "/artifacts/iso-1.patch", + changesApplied: null, + nestedPatches: [{ relativePath: "nested", patch: "diff --git a/file b/file\n" }], + isolationSummary: "Isolation: changes captured at `/artifacts/iso-1.patch` (apply=false). Not applied.", + }, + })); + const node = (await (sandbox.agent as AgentHelper)("scout", { + schema: { type: "object" }, + isolated: true, + apply: false, + returnHandle: true, + })) as Record; + expect(node.handle).toBe("agent://iso-1"); + expect(node.data).toEqual({ ok: true }); + expect(node.isolated).toBe(true); + expect(node.patchPath).toBe("/artifacts/iso-1.patch"); + expect(node.nestedPatches).toEqual([{ relativePath: "nested", patch: "diff --git a/file b/file\n" }]); + expect(node.changesApplied).toBeNull(); + expect(node.isolationSummary).toContain("/artifacts/iso-1.patch"); + expect("branchName" in node).toBe(false); + }); }); diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index ab9b87a06..e8be81670 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -12,9 +12,19 @@ import { MCPManager } from "../mcp/manager"; import subagentUserPromptTemplate from "../prompts/system/subagent-user-prompt.md" with { type: "text" }; import { MAIN_AGENT_ID } from "../registry/agent-registry"; import * as taskDiscovery from "../task/discovery"; +import type { ExecutorOptions } from "../task/executor"; import * as taskExecutor from "../task/executor"; +import { + applyEligibleNestedPatches, + type IsolationContext, + makeIsolationCommitMessage, + mergeIsolatedChanges, + prepareIsolationContext, + runIsolatedSubprocess, +} from "../task/isolation-runner"; import { AgentOutputManager } from "../task/output-manager"; import type { AgentDefinition, AgentProgress, SingleResult } from "../task/types"; +import { type NestedRepoPatch, parseIsolationMode } from "../task/worktree"; import type { ToolSession } from "../tools"; import { ToolError } from "../tools/tool-errors"; import { withBridgeTimeoutPause } from "./bridge-timeout"; @@ -37,6 +47,10 @@ const agentArgsSchema = type({ "model?": "string>0|string>0[]", "label?": "string", "schema?": "unknown", + "isolated?": "boolean", + "apply?": "boolean", + "merge?": "boolean", + "returnHandle?": "boolean", }); interface EvalAgentArgs { @@ -45,6 +59,31 @@ interface EvalAgentArgs { model?: string | string[]; label?: string; schema?: unknown; + /** + * Run this subagent inside an isolation worktree (copy-on-write of the + * parent repo). Strict opt-in: defaults to `false` regardless of the + * session's `task.isolation.mode`, mirroring the `task` tool. Passing + * `true` while `task.isolation.mode === "none"` errors out instead of + * silently downgrading. + */ + isolated?: boolean; + /** + * When isolated, apply the captured patch / merge the captured branch back + * to the parent repo (default `true`). Pass `false` to keep changes in the + * isolation worktree only — the patch artifact path / branch name lands in + * the result so the caller can inspect or apply manually. + */ + apply?: boolean; + /** + * When isolated, allow branch-merge mode (cherry-pick onto HEAD). Defaults + * to `true`, in which case the active `task.isolation.merge` setting picks + * patch vs branch. Pass `false` to force patch mode even when the setting + * is `"branch"` — useful when a fan-out cannot tolerate the per-call git + * lock + repo mutation that branch mode performs. + */ + merge?: boolean; + /** True when a runtime helper will return an `agent://` handle backed by the output artifacts. */ + returnHandle?: boolean; } export interface EvalAgentBridgeOptions { @@ -60,6 +99,24 @@ export interface EvalAgentResult { id: string; model?: string | string[]; structured: boolean; + /** True iff this run executed inside an isolation worktree. */ + isolated?: boolean; + /** Captured patch artifact (patch mode) — surfaced regardless of `apply`. */ + patchPath?: string; + /** Captured branch (branch mode) — surfaced regardless of `apply`. */ + branchName?: string; + /** Captured nested repository patches — surfaced for isolated `apply=false` manual application. */ + nestedPatches?: NestedRepoPatch[]; + /** + * Tri-state apply outcome for isolated runs: + * - `true` — apply ran (or had nothing to do) and left the repo clean. + * - `false` — apply attempted and failed; artifacts preserved. + * - `null` — caller opted out via `apply=false`. + * Omitted for non-isolated runs. + */ + changesApplied?: boolean | null; + /** Human-readable isolation apply/merge summary; kept out of schema-backed `text`. */ + isolationSummary?: string; }; } @@ -129,15 +186,47 @@ function getOutputManager(session: ToolSession): AgentOutputManager { return manager; } -async function getArtifacts(session: ToolSession): Promise<{ +interface ArtifactPaths { sessionFile: string | null; artifactsDir: string; -}> { + /** + * True when `artifactsDir` was created off the session path (no session + * file). Caller is then free to `rm -rf` it once all isolated patch + * artifacts have been consumed or applied. + */ + tempArtifactsDir: boolean; +} + +async function getArtifacts(session: ToolSession): Promise { const sessionFile = session.getSessionFile(); const sessionArtifactsDir = sessionFile ? sessionFile.slice(0, -6) : null; + const tempArtifactsDir = sessionArtifactsDir === null; const artifactsDir = sessionArtifactsDir ?? path.join(os.tmpdir(), `omp-eval-agent-${Snowflake.next()}`); await fs.mkdir(artifactsDir, { recursive: true }); - return { sessionFile, artifactsDir }; + return { sessionFile, artifactsDir, tempArtifactsDir }; +} + +/** + * Persist nested-repo patches to the per-call artifacts dir so an isolated + * apply failure can surface their paths in the thrown ToolError. The + * isolation worktree is already gone by the time we run, so without this the + * captured nested patches would be unrecoverable. + */ +async function persistNestedPatches( + artifactsDir: string, + agentId: string, + nestedPatches: NestedRepoPatch[], +): Promise { + const written: string[] = []; + for (let index = 0; index < nestedPatches.length; index++) { + const patch = nestedPatches[index]; + if (!patch) continue; + const slug = patch.relativePath.replace(/[^A-Za-z0-9._-]+/g, "_") || `nested-${index}`; + const out = path.join(artifactsDir, `${agentId}.nested-${index}-${slug}.patch`); + await Bun.write(out, patch.patch); + written.push(out); + } + return written; } function emitProgressStatus(emitStatus: ((event: JsStatusEvent) => void) | undefined, progress: AgentProgress): void { @@ -235,85 +324,222 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption }; const parentArtifactManager = options.session.getArtifactManager?.() ?? undefined; const mcpManager = options.session.mcpManager ?? MCPManager.instance(); - const { sessionFile, artifactsDir } = await getArtifacts(options.session); + const { sessionFile, artifactsDir, tempArtifactsDir } = await getArtifacts(options.session); const outputManager = getOutputManager(options.session); const id = await outputManager.allocate(outputIdBase(parsed.label, agentName)); const assignment = parsed.prompt.trim(); - // Suspend eval timeout accounting while the subagent owns control. The - // timeout clock restarts once the bridge returns to the cell runtime. - const result = await withBridgeTimeoutPause(options.emitStatus, () => - taskExecutor.runSubprocess({ - cwd: options.session.cwd, - agent: effectiveAgent, - task: renderSubagentPrompt(assignment), - assignment, - description: trimToUndefined(parsed.label), - index: 0, - id, - taskDepth: options.session.taskDepth ?? 0, - modelOverride, - parentActiveModelPattern, - thinkingLevel: effectiveAgent.thinkingLevel, - outputSchema: structured ? parsed.schema : undefined, - sessionFile, - persistArtifacts: Boolean(sessionFile), - artifactsDir, - // Eval `agent()` subagents are short-lived programmatic helpers (data - // collection, structured output, parallel() fan-out). LSP server - // cold-start costs tens of seconds and is pure overhead here, so it is - // forced off regardless of the `task.enableLsp` setting — that knob only - // governs LSP-aware delegation through the `task` tool. - enableLsp: false, - signal: options.signal, - eventBus: options.session.eventBus, - onProgress: progress => emitProgressStatus(options.emitStatus, progress), - authStorage: options.session.authStorage, - modelRegistry: options.session.modelRegistry, - settings: options.session.settings, - // Eval `agent()` subagents are never wall-clock capped: the parent - // cell's idle watchdog is suspended for the whole bridge call - // (withBridgeTimeoutPause), so a long-running phase/recovery workflow - // must not be killed by `task.maxRuntimeMs`. Force the limit off - // regardless of the inherited session setting. - maxRuntimeMs: 0, - mcpManager, - contextFiles, - skills: availableSkills, - autoloadSkills: resolvedAutoloadSkills, - workspaceTree: options.session.workspaceTree, - promptTemplates: options.session.promptTemplates, - localProtocolOptions, - parentArtifactManager, - parentHindsightSessionState: options.session.getHindsightSessionState?.(), - parentMnemopiSessionState: options.session.getMnemopiSessionState?.(), - parentTelemetry: options.session.getTelemetry?.(), - parentAgentId: options.session.getAgentId?.() ?? MAIN_AGENT_ID, - // Deliberately omit parentEvalSessionId: the parent's Python kernel is - // blocked on this bridge call, so sharing the eval session would deadlock - // (subagent queues behind the parent's in-flight execution, parent waits - // for subagent → circular). Each bridge-spawned subagent gets its own - // eval session with an independent kernel. - }), - ); - if (result.exitCode !== 0 || result.error || result.aborted) { - throw new ToolError(buildSubagentFailureMessage(agentName, result)); + // Isolation gating. Strict opt-in: only the explicit `isolated=true` + // argument turns it on; `task.isolation.mode` no longer drives the + // default. Mirrors the `task` tool so eval `agent()` and `task` callers + // see the same semantic. `isolated=true` while the mode is `"none"` + // surfaces a clear error instead of silently downgrading. + const isolationMode = options.session.settings.get("task.isolation.mode"); + const isolationEnabledInSettings = isolationMode !== "none"; + if (parsed.isolated === true && !isolationEnabledInSettings) { + throw new ToolError(`agent(isolated=True) requires task.isolation.mode to be set; current mode is "none".`); } + const isIsolated = parsed.isolated === true; + const settingsMergeMode = options.session.settings.get("task.isolation.merge"); + const mergeMode: "patch" | "branch" = parsed.merge === false ? "patch" : settingsMergeMode; + const applyChanges = parsed.apply !== false; - options.session.recordEvalSubagentUsage?.(result.usage?.output ?? 0); + let isolationContext: IsolationContext | null = null; + if (isIsolated) { + try { + isolationContext = await prepareIsolationContext(options.session.cwd); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new ToolError(`Isolated agent() execution requires a git repository. ${message}`); + } + } + const preferredBackend = isIsolated ? parseIsolationMode(isolationMode) : undefined; - // The final `onProgress` flush from `runSubprocess` already emits a - // status:"completed" event carrying full stats (toolCount, cost, context), - // so we don't emit a second, sparser completion event here — it would - // coalesce over the richer one and drop those stats. + const buildCommitMessage = makeIsolationCommitMessage(options.session); + + const baseRunOptions: ExecutorOptions = { + cwd: options.session.cwd, + agent: effectiveAgent, + task: renderSubagentPrompt(assignment), + assignment, + description: trimToUndefined(parsed.label), + index: 0, + id, + taskDepth: options.session.taskDepth ?? 0, + modelOverride, + parentActiveModelPattern, + thinkingLevel: effectiveAgent.thinkingLevel, + outputSchema: structured ? parsed.schema : undefined, + sessionFile, + persistArtifacts: Boolean(sessionFile), + artifactsDir, + // Eval `agent()` subagents are short-lived programmatic helpers (data + // collection, structured output, parallel() fan-out). LSP server + // cold-start costs tens of seconds and is pure overhead here, so it is + // forced off regardless of the `task.enableLsp` setting — that knob only + // governs LSP-aware delegation through the `task` tool. + enableLsp: false, + signal: options.signal, + eventBus: options.session.eventBus, + onProgress: progress => emitProgressStatus(options.emitStatus, progress), + authStorage: options.session.authStorage, + modelRegistry: options.session.modelRegistry, + settings: options.session.settings, + // Eval `agent()` subagents are never wall-clock capped: the parent + // cell's idle watchdog is suspended for the whole bridge call + // (withBridgeTimeoutPause), so a long-running phase/recovery workflow + // must not be killed by `task.maxRuntimeMs`. Force the limit off + // regardless of the inherited session setting. + maxRuntimeMs: 0, + mcpManager, + contextFiles, + skills: availableSkills, + autoloadSkills: resolvedAutoloadSkills, + workspaceTree: options.session.workspaceTree, + promptTemplates: options.session.promptTemplates, + localProtocolOptions, + parentArtifactManager, + parentHindsightSessionState: options.session.getHindsightSessionState?.(), + parentMnemopiSessionState: options.session.getMnemopiSessionState?.(), + parentTelemetry: options.session.getTelemetry?.(), + parentAgentId: options.session.getAgentId?.() ?? MAIN_AGENT_ID, + // Deliberately omit parentEvalSessionId: the parent's Python kernel is + // blocked on this bridge call, so sharing the eval session would deadlock + // (subagent queues behind the parent's in-flight execution, parent waits + // for subagent → circular). Each bridge-spawned subagent gets its own + // eval session with an independent kernel. + }; + + // Suspend eval timeout accounting through the WHOLE bridge call: the + // subagent subprocess plus any isolation post-processing (merge, + // nested-patch apply, cleanup). All of that is host-side work while the + // runtime is parked waiting for the result, and the cell timeout must + // not abort us mid-cherry-pick or mid-nested-commit. The clock restarts + // only after we hand control back to the runtime. + const { result, mergeSummary, changesApplied } = await withBridgeTimeoutPause(options.emitStatus, async () => { + const result = await (async () => { + if (!isolationContext) { + return taskExecutor.runSubprocess(baseRunOptions); + } + const taskStart = Date.now(); + return runIsolatedSubprocess({ + baseOptions: baseRunOptions, + context: isolationContext, + preferredBackend, + agentId: id, + mergeMode, + artifactsDir, + description: trimToUndefined(parsed.label), + buildCommitMessage, + buildFailureResult: err => { + const message = err instanceof Error ? err.message : String(err); + return { + index: 0, + id, + agent: effectiveAgent.name, + agentSource: effectiveAgent.source, + task: renderSubagentPrompt(assignment), + assignment, + description: trimToUndefined(parsed.label), + exitCode: 1, + output: "", + stderr: message, + truncated: false, + durationMs: Date.now() - taskStart, + tokens: 0, + requests: 0, + modelOverride, + error: message, + }; + }, + }); + })(); + + if (result.exitCode !== 0 || result.error || result.aborted) { + throw new ToolError(buildSubagentFailureMessage(agentName, result)); + } + + let mergeSummary = ""; + let changesApplied: boolean | null = null; + if (isIsolated && isolationContext) { + if (applyChanges) { + const outcome = await mergeIsolatedChanges({ + result, + repoRoot: isolationContext.repoRoot, + mergeMode, + }); + mergeSummary = outcome.summary; + changesApplied = outcome.changesApplied; + if (outcome.changesApplied === false) { + const summaryText = outcome.summary.trim(); + const recoveryParts: string[] = []; + if (result.patchPath) recoveryParts.push(`Captured patch preserved at ${result.patchPath}.`); + if (result.branchName) recoveryParts.push(`Captured branch preserved as ${result.branchName}.`); + if (result.nestedPatches?.length) { + const nestedPaths = await persistNestedPatches(artifactsDir, result.id, result.nestedPatches); + recoveryParts.push( + `Captured nested repository patches (${result.nestedPatches.length}) preserved at: ${nestedPaths.join(", ")}.`, + ); + } + const recoveryHint = recoveryParts.length > 0 ? ` ${recoveryParts.join(" ")}` : ""; + throw new ToolError( + `agent() isolated apply failed for ${result.id}${summaryText ? `: ${summaryText}` : ""}${recoveryHint}`, + ); + } + + mergeSummary += await applyEligibleNestedPatches({ + result, + repoRoot: isolationContext.repoRoot, + mergeMode, + changesApplied: outcome.changesApplied, + mergedBranchForNestedPatches: outcome.mergedBranchForNestedPatches, + commitMessage: buildCommitMessage(), + }); + } else if (result.branchName) { + mergeSummary = `\n\nIsolation: changes captured on branch \`${result.branchName}\` (apply=false). Not merged.`; + } else if (result.patchPath) { + mergeSummary = `\n\nIsolation: changes captured at \`${result.patchPath}\` (apply=false). Not applied.`; + } else { + const nestedPatches = result.nestedPatches ?? []; + if (nestedPatches.length > 0) { + mergeSummary = `\n\nIsolation: changes captured for ${nestedPatches.length} nested repositor${nestedPatches.length === 1 ? "y" : "ies"} (apply=false). Not applied.`; + } else { + mergeSummary = "\n\nIsolation: no changes captured."; + } + } + } + + // Clean up the temp artifacts dir we created for this call only when the + // caller will not need files from it later. Keep it when the runtime helper + // will return an `agent://` handle (the `.md`/`.jsonl` backing files live + // here) and on `apply=false` (`changesApplied === null`) where the caller + // consumes `details.patchPath` / `details.branchName` / + // `details.nestedPatches` out of band. Failed isolated applies throw + // earlier with a recovery hint, so they never reach this gate. + const shouldCleanupTempArtifacts = + tempArtifactsDir && !parsed.returnHandle && (!isIsolated || changesApplied === true); + if (shouldCleanupTempArtifacts) { + await fs.rm(artifactsDir, { recursive: true, force: true }); + } + + options.session.recordEvalSubagentUsage?.(result.usage?.output ?? 0); + + return { result, mergeSummary, changesApplied }; + }); return { - text: result.output, + text: structured ? result.output : result.output + mergeSummary, details: { agent: result.agent, id: result.id, model: result.resolvedModel ?? modelOverride, structured, + isolated: isIsolated || undefined, + patchPath: result.patchPath, + branchName: result.branchName, + nestedPatches: result.nestedPatches?.length ? result.nestedPatches : undefined, + changesApplied: isIsolated ? changesApplied : undefined, + isolationSummary: mergeSummary ? mergeSummary.trim() : undefined, }, }; } diff --git a/packages/coding-agent/src/eval/jl/prelude.jl b/packages/coding-agent/src/eval/jl/prelude.jl index 52ff323ef..72e8644f3 100644 --- a/packages/coding-agent/src/eval/jl/prelude.jl +++ b/packages/coding-agent/src/eval/jl/prelude.jl @@ -734,7 +734,7 @@ function completion(prompt::String; model="default", system=nothing, schema=noth return schema === nothing ? text : Main.json_parse(string(text)) end -function agent(prompt::String; agent_type="task", model=nothing, label=nothing, schema=nothing, return_handle=false, kwargs...) +function agent(prompt::String; agent_type="task", model=nothing, label=nothing, schema=nothing, isolated=nothing, apply=nothing, merge=nothing, return_handle=false, kwargs...) args_dict = Dict{String, Any}("prompt" => prompt) if agent_type !== nothing args_dict["agentType"] = agent_type @@ -748,6 +748,17 @@ function agent(prompt::String; agent_type="task", model=nothing, label=nothing, if schema !== nothing args_dict["schema"] = schema end + # Isolation knobs mirror the `task` tool: strict opt-in via `isolated`, + # with `apply`/`merge` controlling the post-run patch/branch merge. + if isolated !== nothing + args_dict["isolated"] = Bool(isolated) + end + if apply !== nothing + args_dict["apply"] = Bool(apply) + end + if merge !== nothing + args_dict["merge"] = Bool(merge) + end handle_result = return_handle for (k, v) in kwargs key = string(k) @@ -759,6 +770,10 @@ function agent(prompt::String; agent_type="task", model=nothing, label=nothing, args_dict[key] = v end end + # Tell the bridge a handle is wanted so it preserves the backing artifacts. + if handle_result + args_dict["returnHandle"] = true + end res = __omp_call_bridge("__agent__", args_dict) text = res isa AbstractDict ? get(res, "text", res) : res parsed = schema === nothing ? text : Main.json_parse(string(text)) @@ -779,6 +794,18 @@ function agent(prompt::String; agent_type="task", model=nothing, label=nothing, if schema !== nothing node["data"] = parsed end + for (src_key, dst_key) in ( + ("isolated", "isolated"), + ("patchPath", "patch_path"), + ("branchName", "branch_name"), + ("nestedPatches", "nested_patches"), + ("changesApplied", "changes_applied"), + ("isolationSummary", "isolation_summary"), + ) + if haskey(details, src_key) + node[dst_key] = details[src_key] + end + end return node end diff --git a/packages/coding-agent/src/eval/js/shared/prelude.txt b/packages/coding-agent/src/eval/js/shared/prelude.txt index 5225dc686..21e33d406 100644 --- a/packages/coding-agent/src/eval/js/shared/prelude.txt +++ b/packages/coding-agent/src/eval/js/shared/prelude.txt @@ -117,9 +117,15 @@ if (!globalThis.__omp_js_prelude_loaded__) { }; const agent = async (prompt, opts, ...rest) => { - const o = optionsArg("agent", opts, rest, ["agentType", "model", "label", "schema"], "{ agentType, model, label, schema, returnHandle }"); + const o = optionsArg( + "agent", + opts, + rest, + ["agentType", "model", "label", "schema", "isolated", "apply", "merge"], + "{ agentType, model, label, schema, isolated, apply, merge, returnHandle }", + ); const { returnHandle, ...callArgs } = o; - const res = await globalThis.__omp_call_tool__("__agent__", { prompt, ...callArgs }); + const res = await globalThis.__omp_call_tool__("__agent__", { prompt, ...callArgs, returnHandle: Boolean(returnHandle) }); const text = res && typeof res === "object" ? res.text : res; const parsed = hasOwn(callArgs, "schema") ? JSON.parse(text) : text; if (!returnHandle) return parsed; @@ -129,6 +135,9 @@ if (!globalThis.__omp_js_prelude_loaded__) { } const node = { text, output: text, handle: `agent://${details.id}`, id: details.id, agent: details.agent ?? null }; if (hasOwn(callArgs, "schema")) node.data = parsed; + for (const key of ["isolated", "patchPath", "branchName", "nestedPatches", "changesApplied", "isolationSummary"]) { + if (details[key] !== undefined) node[key] = details[key]; + } return node; }; diff --git a/packages/coding-agent/src/eval/py/__tests__/prelude.test.ts b/packages/coding-agent/src/eval/py/__tests__/prelude.test.ts index 8c33d7741..f9ef0e852 100644 --- a/packages/coding-agent/src/eval/py/__tests__/prelude.test.ts +++ b/packages/coding-agent/src/eval/py/__tests__/prelude.test.ts @@ -16,4 +16,17 @@ describe("python prelude", () => { expect(signature).toContain("offset"); expect(signature).toContain("limit"); }); + + it("exposes isolation artifacts on the agent() return_handle node", () => { + // agent(..., return_handle=True) is the only escape hatch for + // recovering apply=False patch/branch/nested artifacts (the bare + // schema return is just the parsed object), so the helper MUST + // translate the bridge's camelCase details onto the node — otherwise + // an isolated apply=False workflow loses captured nested patches. + expect(PYTHON_PRELUDE).toContain('("patchPath", "patch_path")'); + expect(PYTHON_PRELUDE).toContain('("branchName", "branch_name")'); + expect(PYTHON_PRELUDE).toContain('("nestedPatches", "nested_patches")'); + expect(PYTHON_PRELUDE).toContain('("changesApplied", "changes_applied")'); + expect(PYTHON_PRELUDE).toContain('("isolationSummary", "isolation_summary")'); + }); }); diff --git a/packages/coding-agent/src/eval/py/prelude.py b/packages/coding-agent/src/eval/py/prelude.py index 96112fff0..9d14eceee 100644 --- a/packages/coding-agent/src/eval/py/prelude.py +++ b/packages/coding-agent/src/eval/py/prelude.py @@ -520,7 +520,7 @@ if "__omp_prelude_loaded__" not in globals(): text = res.get("text") if isinstance(res, dict) else res return json.loads(text) if schema is not None else text - def agent(prompt, *, agent_type="task", model=None, label=None, schema=None, return_handle=False): + def agent(prompt, *, agent_type="task", model=None, label=None, schema=None, isolated=None, apply=None, merge=None, return_handle=False): """Run a subagent and return its final output. `agent_type` selects the subagent definition (default "task"). Pass @@ -529,14 +529,35 @@ if "__omp_prelude_loaded__" not in globals(): supplied the parsed object is returned. Share background by writing a local:// file and referencing it in the prompt. + Pass `isolated=True` to run the subagent inside an isolation worktree + (copy-on-write of the parent repo) so parallel `agent()` spawns can + edit overlapping files safely. Strict opt-in, mirroring the `task` + tool: the default is non-isolated regardless of `task.isolation.mode`. + `isolated=True` while the setting is `"none"` errors out instead of + silently downgrading. + + When isolated, `apply=False` keeps captured changes inside the + worktree and surfaces the root patch path, branch name, and nested + repository patches through the DAG node dict (combine with + `return_handle=True` to receive them — see below; the bare return type + stays bytes/string/parsed object and has nowhere to expose artifacts). + `merge=False` forces patch mode even when `task.isolation.merge` is + `"branch"`, avoiding the per-call git lock + repo mutation that branch + mode performs. + Set `return_handle=True` to receive a DAG node dict instead of bare text: ``{"text", "output", "handle", "id", "agent"}`` where ``handle`` is the spawned agent's recoverable ``agent://`` URI. A downstream ``pipeline``/``parallel`` stage embeds that ``handle`` (or ``output``) in its prompt so a large transcript flows through the graph by reference, never re-inlined. When ``schema`` is also set the parsed - object lands under ``"data"``. If the bridge returns no recoverable id - the node still resolves with ``handle=None`` — the helper never throws. + object lands under ``"data"``. When the spawn ran isolated the node + also carries ``"isolated"`` and, when present, ``"patch_path"``, + ``"branch_name"``, ``"nested_patches"``, ``"changes_applied"`` + (``True``/``False``/``None`` — ``None`` means ``apply=False``), and + ``"isolation_summary"``. If + the bridge returns no recoverable id the node still resolves with + ``handle=None`` — the helper never throws. """ args = {"prompt": prompt} if agent_type is not None: @@ -547,6 +568,14 @@ if "__omp_prelude_loaded__" not in globals(): args["label"] = label if schema is not None: args["schema"] = schema + if isolated is not None: + args["isolated"] = bool(isolated) + if apply is not None: + args["apply"] = bool(apply) + if merge is not None: + args["merge"] = bool(merge) + if return_handle: + args["returnHandle"] = True res = _bridge_call("__agent__", args) text = res.get("text") if isinstance(res, dict) else res parsed = json.loads(text) if schema is not None else text @@ -564,6 +593,16 @@ if "__omp_prelude_loaded__" not in globals(): } if schema is not None: node["data"] = parsed + for src_key, dst_key in ( + ("isolated", "isolated"), + ("patchPath", "patch_path"), + ("branchName", "branch_name"), + ("nestedPatches", "nested_patches"), + ("changesApplied", "changes_applied"), + ("isolationSummary", "isolation_summary"), + ): + if src_key in details: + node[dst_key] = details[src_key] return node def _concurrency_limit(): diff --git a/packages/coding-agent/src/eval/rb/prelude.rb b/packages/coding-agent/src/eval/rb/prelude.rb index 42eace372..9136069c8 100644 --- a/packages/coding-agent/src/eval/rb/prelude.rb +++ b/packages/coding-agent/src/eval/rb/prelude.rb @@ -577,12 +577,19 @@ unless defined?($__omp_prelude_loaded) && $__omp_prelude_loaded schema.nil? ? text : JSON.parse(text) end - def agent(prompt, agent_type: "task", model: nil, label: nil, schema: nil, return_handle: false) + def agent(prompt, agent_type: "task", model: nil, label: nil, schema: nil, isolated: nil, apply: nil, merge: nil, return_handle: false) args = { "prompt" => prompt } args["agentType"] = agent_type unless agent_type.nil? args["model"] = model unless model.nil? args["label"] = label unless label.nil? args["schema"] = schema unless schema.nil? + # Isolation knobs mirror the `task` tool: strict opt-in via `isolated`, + # with `apply`/`merge` controlling the post-run patch/branch merge. + args["isolated"] = !!isolated unless isolated.nil? + args["apply"] = !!apply unless apply.nil? + args["merge"] = !!merge unless merge.nil? + # Tell the bridge a handle is wanted so it preserves the backing artifacts. + args["returnHandle"] = true if return_handle res = OmpBridge.call("__agent__", args) text = res.is_a?(Hash) ? res["text"] : res parsed = schema.nil? ? text : JSON.parse(text) @@ -599,6 +606,16 @@ unless defined?($__omp_prelude_loaded) && $__omp_prelude_loaded "agent" => details["agent"], } node["data"] = parsed unless schema.nil? + { + "isolated" => "isolated", + "patchPath" => "patch_path", + "branchName" => "branch_name", + "nestedPatches" => "nested_patches", + "changesApplied" => "changes_applied", + "isolationSummary" => "isolation_summary", + }.each do |src_key, dst_key| + node[dst_key] = details[src_key] if details.key?(src_key) + end node end diff --git a/packages/coding-agent/src/prompts/system/workflow-notice.md b/packages/coding-agent/src/prompts/system/workflow-notice.md index a8e1c6f55..1fbfbc59c 100644 --- a/packages/coding-agent/src/prompts/system/workflow-notice.md +++ b/packages/coding-agent/src/prompts/system/workflow-notice.md @@ -13,7 +13,7 @@ Worth it when the task benefits from decomposition + parallel coverage, or from State persists across cells, so scout in one cell and fan out in the next. Every cell has: -- `agent(prompt, *, agent_type="task", model=None, label=None, schema=None)` — run ONE subagent; returns its final text, or the validated object when `schema` (a JSON Schema dict) is given. With `schema` the subagent is forced to emit structured output that is validated for you — branch on the object, not on parsed prose. `agent_type` picks a discovered agent ("explore", "reviewer", "oracle", …); `label` names the artifact. Shared background goes in a `local://` file referenced from each prompt, not a parameter. Subagents are told their final text IS the return value, so they hand back raw data. `agent()` blocks until the subagent finishes; eval-spawned agents nest at most 3 deep. +- `agent(prompt, *, agent_type="task", model=None, label=None, schema=None, isolated=None, apply=None, merge=None, return_handle=False)` — run ONE subagent; returns its final text, or the validated object when `schema` (a JSON Schema dict) is given. With `schema` the subagent is forced to emit structured output that is validated for you — branch on the object, not on parsed prose. `agent_type` picks a discovered agent ("explore", "reviewer", "oracle", …); `label` names the artifact. Shared background goes in a `local://` file referenced from each prompt, not a parameter. Subagents are told their final text IS the return value, so they hand back raw data. `agent()` blocks until the subagent finishes; eval-spawned agents nest at most 3 deep. Pass `isolated=True` to run the spawn in a copy-on-write worktree so parallel `agent()` calls can edit overlapping files safely — strict opt-in, mirrors the `task` tool, defaults off regardless of `task.isolation.mode`; `isolated=True` while the setting is `"none"` errors out instead of silently downgrading. With isolation, `apply=False` keeps changes in the worktree, and `merge=False` forces patch mode even when the setting is `"branch"`. Captured root patch path, branch name, nested repo patches, and apply summary reach the workflow through `return_handle=True` — combine it with `apply=False` (or `apply=False, schema=…`) and read `node["patch_path"]`, `node["branch_name"]`, `node["nested_patches"]`, `node["changes_applied"]`, `node["isolation_summary"]` (JS: same keys camelCased) to recover artifacts. - `parallel(thunks)` — run zero-arg callables concurrently through a bounded pool, preserving input order; returns once all finish. The pool is bounded by the session's `task` concurrency — don't hand-tune it; fan out as wide as the work divides. A thunk that raises propagates — wrap risky work in `try/except` inside the thunk to keep partial results. In a loop, bind each closure's value with a default arg (`lambda d=d: …`) or every thunk captures the last one. - `pipeline(items, *stages)` — map items through `stages` left-to-right. There is a BARRIER between stages: ALL items clear stage N before stage N+1 begins. Each stage is a one-arg callable; stage 1 gets the original item, later stages get the previous result. Same pool width as `parallel()`. - `completion(prompt, *, model="default", system=None, schema=None)` — oneshot, stateless model call (no tools, no history). Tiers: "smol", "default", "slow". Cheap classification/scoring inside a fan-out. diff --git a/packages/coding-agent/src/task/index.ts b/packages/coding-agent/src/task/index.ts index 7530c9dce..d48091a1a 100644 --- a/packages/coding-agent/src/task/index.ts +++ b/packages/coding-agent/src/task/index.ts @@ -47,29 +47,22 @@ import type { AsyncJobManager } from "../async"; import type { LocalProtocolOptions } from "../internal-urls"; import { loadOverallPlanReference } from "../plan-mode/plan-handoff"; import { AgentRegistry, MAIN_AGENT_ID } from "../registry/agent-registry"; -import { generateCommitMessage } from "../utils/commit-message-generator"; -import * as git from "../utils/git"; import { type DiscoveryResult, discoverAgents, getAgent } from "./discovery"; import { runSubprocess } from "./executor"; +import { + applyEligibleNestedPatches, + type IsolationContext, + makeIsolationCommitMessage, + mergeIsolatedChanges, + prepareIsolationContext, + runIsolatedSubprocess, +} from "./isolation-runner"; import { generateTaskName } from "./name-generator"; import { AgentOutputManager } from "./output-manager"; import { mapWithConcurrencyLimit, Semaphore } from "./parallel"; import { renderResult, renderCall as renderTaskCall } from "./render"; import { repairTaskParams } from "./repair-args"; -import { - applyNestedPatches, - captureBaseline, - captureDeltaPatch, - cleanupIsolation, - cleanupTaskBranches, - commitToBranch, - ensureIsolation, - getRepoRoot, - type IsolationHandle, - mergeTaskBranches, - parseIsolationMode, - type WorktreeBaseline, -} from "./worktree"; +import { parseIsolationMode } from "./worktree"; function renderSubagentUserPrompt(assignment: string): string { return prompt.render(subagentUserPromptTemplate, { @@ -1047,7 +1040,6 @@ export class TaskTool implements AgentTool - commitStyle === "ai" && this.session.modelRegistry - ? async (diff: string) => { - return generateCommitMessage( - diff, - this.session.modelRegistry!, - this.session.settings, - this.session.getSessionId?.() ?? undefined, - ); - } - : undefined; + const buildCommitMessageFn = makeIsolationCommitMessage(this.session); const sharedRunOptions = { cwd: this.session.cwd, @@ -1321,192 +1302,65 @@ export class TaskTool implements AgentTool { + const message = err instanceof Error ? err.message : String(err); + return { + index: spawnIndex, + id: agentId, + agent: agent.name, + agentSource: agent.source, + task: renderSubagentUserPrompt(assignment), + assignment, + description: params.description, + exitCode: 1, + output: "", + stderr: message, + truncated: false, + durationMs: Date.now() - taskStart, + tokens: 0, + requests: 0, + modelOverride, + error: message, + }; + }, + }); }; const result = await runTask(); let mergeSummary = ""; let changesApplied: boolean | null = null; - let hadAnyChanges = false; let mergedBranchForNestedPatches = false; if (isIsolated && repoRoot) { - try { - if (mergeMode === "branch") { - if (!result.branchName || result.exitCode !== 0 || result.aborted) { - changesApplied = true; - mergeSummary = "\n\nNo changes to apply."; - } else { - const mergeResult = await mergeTaskBranches(repoRoot, [ - { branchName: result.branchName, taskId: result.id, description: result.description }, - ]); - mergedBranchForNestedPatches = mergeResult.merged.includes(result.branchName); - changesApplied = mergeResult.failed.length === 0; - hadAnyChanges = changesApplied && mergeResult.merged.length > 0; - - if (changesApplied) { - mergeSummary = hadAnyChanges - ? `\n\nMerged branch: ${result.branchName}` - : "\n\nNo changes to apply."; - } else { - const conflictPart = mergeResult.conflict ? `\nConflict: ${mergeResult.conflict}` : ""; - mergeSummary = `\n\nBranch merge failed: ${result.branchName}.${conflictPart}\nThe unmerged branch remains for manual resolution.`; - } - if (mergeResult.stashConflict) { - mergeSummary += `\n\n${mergeResult.stashConflict}`; - } - - // Clean up the merged branch (keep failed ones for manual resolution) - if (changesApplied) { - await cleanupTaskBranches(repoRoot, [result.branchName]); - } - } - } else { - // Patch mode: apply the patch from a successful run. A failed or - // aborted run has nothing to apply and must not block the result. - const succeeded = result.exitCode === 0 && !result.error && !result.aborted; - if (!succeeded) { - changesApplied = true; - hadAnyChanges = false; - } else if (!result.patchPath) { - changesApplied = false; - hadAnyChanges = false; - } else { - const patchText = await Bun.file(result.patchPath).text(); - if (!patchText.trim()) { - changesApplied = true; - hadAnyChanges = false; - } else { - const normalized = patchText.endsWith("\n") ? patchText : `${patchText}\n`; - changesApplied = await git.patch.canApplyText(repoRoot, normalized); - if (changesApplied) { - try { - await git.patch.applyText(repoRoot, normalized); - hadAnyChanges = true; - } catch { - changesApplied = false; - hadAnyChanges = false; - } - } - } - } - - if (changesApplied) { - mergeSummary = hadAnyChanges ? "\n\nApplied patches: yes" : "\n\nNo changes to apply."; - } else { - const notification = - "Patches were not applied and must be handled manually."; - const patchList = result.patchPath ? `\n\nPatch artifact:\n- ${result.patchPath}` : ""; - mergeSummary = `\n\n${notification}${patchList}`; - } - } - } catch (mergeErr) { - const msg = mergeErr instanceof Error ? mergeErr.message : String(mergeErr); - changesApplied = false; - hadAnyChanges = false; - mergeSummary = `\n\nMerge phase failed: ${msg}\nTask outputs are preserved but changes were not applied.`; - } + const outcome = await mergeIsolatedChanges({ result, repoRoot, mergeMode }); + mergeSummary = outcome.summary; + changesApplied = outcome.changesApplied; + mergedBranchForNestedPatches = outcome.mergedBranchForNestedPatches; } - // Apply nested repo patches (separate from parent git) - if (isIsolated && repoRoot && (mergeMode === "branch" || changesApplied !== false)) { - const nestedPatches = result.nestedPatches ?? []; - const eligible = - nestedPatches.length > 0 && - result.exitCode === 0 && - !result.aborted && - (mergeMode !== "branch" || mergedBranchForNestedPatches); - if (eligible) { - try { - await applyNestedPatches(repoRoot, nestedPatches, buildCommitMessageFn()); - } catch { - // Nested patch failures are non-fatal to the parent merge - mergeSummary += - "\n\nSome nested repository patches failed to apply."; - } - } + // Apply nested repo patches (separate from parent git). + if (isIsolated && repoRoot) { + mergeSummary += await applyEligibleNestedPatches({ + result, + repoRoot, + mergeMode, + changesApplied, + mergedBranchForNestedPatches, + commitMessage: buildCommitMessageFn(), + }); } // Cleanup temp directory if used diff --git a/packages/coding-agent/src/task/isolation-runner.ts b/packages/coding-agent/src/task/isolation-runner.ts new file mode 100644 index 000000000..3735db3b6 --- /dev/null +++ b/packages/coding-agent/src/task/isolation-runner.ts @@ -0,0 +1,353 @@ +/** + * Reusable isolation lifecycle for subagent execution. + * + * Both `TaskTool` and the eval `agent()` bridge spawn subagents that can run + * inside a copy-on-write worktree, capture their changes, and (optionally) + * apply those changes back to the parent repo. The orchestration is identical + * for both callers; this module hosts the shared lifecycle so eval `agent()` + * does not need to round-trip through `TaskTool.#runSpawn`. + * + * Shape: + * 1. {@link prepareIsolationContext} — resolve git root + capture baseline. + * 2. {@link runIsolatedSubprocess} — start worktree, run, capture + * branch/patch, tear worktree down. + * 3. {@link mergeIsolatedChanges} — apply captured changes back to the + * parent repo (skip when the caller + * opted out). + * + * Step 1 happens once per top-level call (the baseline is cloned per spawn + * before mutation); steps 2 and 3 are per-spawn. + */ +import * as path from "node:path"; +import type * as natives from "@oh-my-pi/pi-natives"; +import type { ToolSession } from "../tools"; +import { generateCommitMessage } from "../utils/commit-message-generator"; +import * as git from "../utils/git"; +import type { ExecutorOptions } from "./executor"; +import { runSubprocess } from "./executor"; +import type { SingleResult } from "./types"; +import { + applyNestedPatches, + captureBaseline, + captureDeltaPatch, + cleanupIsolation, + cleanupTaskBranches, + commitToBranch, + ensureIsolation, + getRepoRoot, + type IsolationHandle, + mergeTaskBranches, + type WorktreeBaseline, +} from "./worktree"; + +type IsoBackendKind = natives.IsoBackendKind; + +/** Resolved repo + baseline used by every isolated spawn in a single call. */ +export interface IsolationContext { + repoRoot: string; + baseline: WorktreeBaseline; +} + +/** + * Resolve the git repo root and capture the worktree baseline used to diff + * each isolated spawn against. Throws when the cwd is not inside a git + * repository; callers surface the error as a task-tool failure. + */ +export async function prepareIsolationContext(cwd: string): Promise { + const repoRoot = await getRepoRoot(cwd); + const baseline = await captureBaseline(repoRoot); + return { repoRoot, baseline }; +} + +/** Build a commit-message callback for branch/nested commits; `undefined` ⇒ fall back to generic message. */ +export type BuildCommitMessage = () => undefined | ((diff: string) => Promise); + +/** + * Construct the commit-message factory used by isolation branch commits and + * nested-repo patch commits. Returns a closure that, each time it's called, + * either yields an AI-backed `(diff) => Promise` callback (when + * `task.isolation.commits === "ai"` and a model registry is available) or + * `undefined` so the caller falls back to a generic commit message. + * + * Centralized so `TaskTool` and the eval `agent()` bridge share one wiring; + * a drift here previously meant the two callers built subtly different + * generators for the same setting. + */ +export function makeIsolationCommitMessage(session: ToolSession): BuildCommitMessage { + return () => { + const style = session.settings.get("task.isolation.commits"); + if (style !== "ai" || !session.modelRegistry) return undefined; + const registry = session.modelRegistry; + const settings = session.settings; + const sessionId = session.getSessionId?.() ?? undefined; + return async (diff: string) => generateCommitMessage(diff, registry, settings, sessionId); + }; +} + +export interface IsolatedRunOptions { + /** + * Base run options handed to the subagent subprocess. This helper sets + * `worktree`, clears `preloadedExtensionPaths` / `preloadedCustomToolPaths` + * (isolated runs re-discover inside the worktree), and forwards everything + * else unchanged. + */ + baseOptions: ExecutorOptions; + /** Context returned by {@link prepareIsolationContext}. Baseline is cloned per spawn. */ + context: IsolationContext; + /** PAL backend hint from `parseIsolationMode(...)` (undefined ⇒ resolver picks). */ + preferredBackend: IsoBackendKind | undefined; + /** Stable id used as the isolation worktree namespace and as the branch suffix. */ + agentId: string; + /** Merge mode driving how changes are captured ("branch" commits, "patch" diffs). */ + mergeMode: "patch" | "branch"; + /** Output dir for `${agentId}.patch` artifacts (patch mode). */ + artifactsDir: string; + /** Human description carried onto the branch commit (branch mode). */ + description?: string; + /** Build a commit-message callback (`task.isolation.commits === "ai"`). */ + buildCommitMessage?: BuildCommitMessage; + /** + * Construct a `SingleResult` when isolation setup throws — the caller has + * the full metadata (index, agent, assignment, modelOverride) needed to + * build a result shape consistent with their non-isolated path. + */ + buildFailureResult: (err: unknown) => SingleResult; +} + +/** + * Run a subagent inside an isolation worktree and capture its changes. + * + * Branch mode: on success, commits the diff onto `omp/task/${agentId}` and + * returns `branchName` + `nestedPatches`. On commit failure the branch is + * deleted and `result.error` carries the merge-failure message. + * + * Patch mode: on success, writes `${artifactsDir}/${agentId}.patch` and + * returns `patchPath` + `nestedPatches`. + * + * Failure paths preserve the underlying `SingleResult` whenever possible so + * the caller can still surface the subagent's output; only isolation setup + * itself routes through {@link IsolatedRunOptions.buildFailureResult}. + * + * The isolation handle is always torn down in `finally`. + */ +export async function runIsolatedSubprocess(opts: IsolatedRunOptions): Promise { + let handle: IsolationHandle | undefined; + try { + const taskBaseline = structuredClone(opts.context.baseline); + handle = await ensureIsolation(opts.context.repoRoot, opts.agentId, opts.preferredBackend); + const isolationDir = handle.mergedDir; + const result = await runSubprocess({ + ...opts.baseOptions, + worktree: isolationDir, + preloadedExtensionPaths: undefined, + preloadedCustomToolPaths: undefined, + }); + if (opts.mergeMode === "branch" && result.exitCode === 0) { + try { + const commitResult = await commitToBranch( + isolationDir, + taskBaseline, + opts.agentId, + opts.description, + opts.buildCommitMessage?.(), + ); + return { + ...result, + branchName: commitResult?.branchName, + nestedPatches: commitResult?.nestedPatches, + }; + } catch (mergeErr) { + // Agent succeeded but branch commit failed — clean up stale branch + const branchName = `omp/task/${opts.agentId}`; + await git.branch.tryDelete(opts.context.repoRoot, branchName); + const msg = mergeErr instanceof Error ? mergeErr.message : String(mergeErr); + return { ...result, error: `Merge failed: ${msg}` }; + } + } + if (result.exitCode === 0) { + try { + const delta = await captureDeltaPatch(isolationDir, taskBaseline); + const patchPath = path.join(opts.artifactsDir, `${opts.agentId}.patch`); + await Bun.write(patchPath, delta.rootPatch); + return { + ...result, + patchPath, + nestedPatches: delta.nestedPatches, + }; + } catch (patchErr) { + const msg = patchErr instanceof Error ? patchErr.message : String(patchErr); + return { ...result, error: `Patch capture failed: ${msg}` }; + } + } + return result; + } catch (err) { + return opts.buildFailureResult(err); + } finally { + if (handle) { + await cleanupIsolation(handle); + } + } +} + +export interface IsolationMergeOptions { + result: SingleResult; + repoRoot: string; + mergeMode: "patch" | "branch"; +} + +export interface IsolationMergeOutcome { + /** Trailing summary appended to the subagent's result text. May be empty. */ + summary: string; + /** + * Tri-state apply outcome: + * - `true` — merge ran (or had nothing to apply) and left the repo clean. + * - `false` — merge attempted and failed; artifacts are preserved. + * - `null` — caller skipped the merge phase entirely (e.g. `apply=false`). + */ + changesApplied: boolean | null; + hadAnyChanges: boolean; + /** True iff the root branch actually merged — gates nested-repo patch application. */ + mergedBranchForNestedPatches: boolean; +} + +/** + * Apply changes captured by {@link runIsolatedSubprocess} back to the parent + * repo: patch apply (patch mode) or cherry-pick + cleanup (branch mode). + * + * The caller decides whether to run this at all — eval `agent()` with + * `apply=False` skips this step and surfaces the patch artifact / branch name + * instead. + */ +export async function mergeIsolatedChanges(opts: IsolationMergeOptions): Promise { + const { result, repoRoot, mergeMode } = opts; + try { + if (mergeMode === "branch") { + const canApplyNestedOnly = + !result.branchName && result.exitCode === 0 && !result.aborted && (result.nestedPatches?.length ?? 0) > 0; + if (!result.branchName || result.exitCode !== 0 || result.aborted) { + return { + summary: canApplyNestedOnly + ? "\n\nNo root changes to apply; nested repository patches captured." + : "\n\nNo changes to apply.", + changesApplied: true, + hadAnyChanges: canApplyNestedOnly, + mergedBranchForNestedPatches: canApplyNestedOnly, + }; + } + const mergeResult = await mergeTaskBranches(repoRoot, [ + { branchName: result.branchName, taskId: result.id, description: result.description }, + ]); + const mergedBranchForNestedPatches = mergeResult.merged.includes(result.branchName); + const changesApplied = mergeResult.failed.length === 0; + const hadAnyChanges = changesApplied && mergeResult.merged.length > 0; + + let summary: string; + if (changesApplied) { + summary = hadAnyChanges ? `\n\nMerged branch: ${result.branchName}` : "\n\nNo changes to apply."; + } else { + const conflictPart = mergeResult.conflict ? `\nConflict: ${mergeResult.conflict}` : ""; + summary = `\n\nBranch merge failed: ${result.branchName}.${conflictPart}\nThe unmerged branch remains for manual resolution.`; + } + if (mergeResult.stashConflict) { + summary += `\n\n${mergeResult.stashConflict}`; + } + + // Clean up the merged branch (keep failed ones for manual resolution) + if (changesApplied) { + await cleanupTaskBranches(repoRoot, [result.branchName]); + } + return { summary, changesApplied, hadAnyChanges, mergedBranchForNestedPatches }; + } + + // Patch mode: apply the patch from a successful run. A failed or + // aborted run has nothing to apply and must not block the result. + let changesApplied: boolean; + let hadAnyChanges: boolean; + const succeeded = result.exitCode === 0 && !result.error && !result.aborted; + if (!succeeded) { + changesApplied = true; + hadAnyChanges = false; + } else if (!result.patchPath) { + changesApplied = false; + hadAnyChanges = false; + } else { + const patchText = await Bun.file(result.patchPath).text(); + if (!patchText.trim()) { + changesApplied = true; + hadAnyChanges = false; + } else { + const normalized = patchText.endsWith("\n") ? patchText : `${patchText}\n`; + changesApplied = await git.patch.canApplyText(repoRoot, normalized); + hadAnyChanges = false; + if (changesApplied) { + try { + await git.patch.applyText(repoRoot, normalized); + hadAnyChanges = true; + } catch { + changesApplied = false; + } + } + } + } + + let summary: string; + if (changesApplied) { + summary = hadAnyChanges ? "\n\nApplied patches: yes" : "\n\nNo changes to apply."; + } else { + const notification = + "Patches were not applied and must be handled manually."; + const patchList = result.patchPath ? `\n\nPatch artifact:\n- ${result.patchPath}` : ""; + summary = `\n\n${notification}${patchList}`; + } + return { summary, changesApplied, hadAnyChanges, mergedBranchForNestedPatches: false }; + } catch (mergeErr) { + const msg = mergeErr instanceof Error ? mergeErr.message : String(mergeErr); + return { + summary: `\n\nMerge phase failed: ${msg}\nTask outputs are preserved but changes were not applied.`, + changesApplied: false, + hadAnyChanges: false, + mergedBranchForNestedPatches: false, + }; + } +} + +export interface NestedPatchApplyOptions { + /** Subagent result carrying `nestedPatches`/`exitCode`/`aborted`. */ + result: SingleResult; + repoRoot: string; + mergeMode: "patch" | "branch"; + /** Parent merge outcome — patch mode skips nested apply when this is `false`. */ + changesApplied: boolean | null; + /** Branch mode gates nested apply on whether the root branch merged. */ + mergedBranchForNestedPatches: boolean; + /** Optional AI commit-message callback for nested commits; falls back to a generic message. */ + commitMessage?: (diff: string) => Promise; +} + +/** + * Apply nested-repo patches after the parent merge phase. Centralizes the + * three-way gate (exitCode/aborted, patch-mode failed parent, branch-mode + * branch-merged) and the non-fatal failure handling so `TaskTool` and the + * eval `agent()` bridge use one implementation. + * + * Returns a system-notification suffix to append to the parent merge summary, + * or an empty string when nothing was applied or the nested apply succeeded. + */ +export async function applyEligibleNestedPatches(opts: NestedPatchApplyOptions): Promise { + const { result, repoRoot, mergeMode, changesApplied, mergedBranchForNestedPatches, commitMessage } = opts; + if (mergeMode === "patch" && changesApplied === false) return ""; + const nestedPatches = result.nestedPatches ?? []; + const eligible = + nestedPatches.length > 0 && + result.exitCode === 0 && + !result.aborted && + (mergeMode !== "branch" || mergedBranchForNestedPatches); + if (!eligible) return ""; + try { + await applyNestedPatches(repoRoot, nestedPatches, commitMessage); + return ""; + } catch { + // Nested patch failures are non-fatal to the parent merge. + return "\n\nSome nested repository patches failed to apply."; + } +} diff --git a/packages/coding-agent/src/task/worktree.ts b/packages/coding-agent/src/task/worktree.ts index 8d23d0305..1fe4abc77 100644 --- a/packages/coding-agent/src/task/worktree.ts +++ b/packages/coding-agent/src/task/worktree.ts @@ -194,6 +194,14 @@ export async function captureDeltaPatch(isolationDir: string, baseline: Worktree /** * Apply nested repo patches directly to their working directories after parent merge. + * + * Pre-existing dirty state in a nested repo is stashed before the patch is + * applied and popped back after the commit, so unrelated user edits never get + * folded into the agent's commit. A failing `git stash pop` (e.g. user edits + * collide with the patched lines) leaves the stash entry intact and emits a + * `logger.warn` — the caller's catch handler turns the broader nested-apply + * failure into a non-fatal system notification. + * * @param commitMessage Optional async function to generate a commit message from the combined diff. * If omitted or returns null, falls back to a generic message. */ @@ -220,15 +228,33 @@ export async function applyNestedPatches( } const combinedDiff = repoPatches.map(p => p.patch).join("\n"); - for (const { patch } of repoPatches) { - await git.patch.applyText(nestedDir, patch); - } - // Commit so nested repo history reflects the task changes - if ((await git.status(nestedDir)).trim().length > 0) { - const msg = (await commitMessage?.(combinedDiff)) ?? "changes from isolated task(s)"; - await git.stage.files(nestedDir); - await git.commit(nestedDir, msg); + // Preserve any pre-existing dirty state (tracked + untracked) so we + // commit only the agent delta, not the user's in-flight work. + const stashed = + (await git.status(nestedDir)).trim().length > 0 + ? await git.stash.push(nestedDir, `omp-isolation-${Snowflake.next()}`) + : false; + try { + for (const { patch } of repoPatches) { + await git.patch.applyText(nestedDir, patch); + } + if ((await git.status(nestedDir)).trim().length > 0) { + const msg = (await commitMessage?.(combinedDiff)) ?? "changes from isolated task(s)"; + await git.stage.files(nestedDir); + await git.commit(nestedDir, msg); + } + } finally { + if (stashed) { + try { + await git.stash.pop(nestedDir, { index: true }); + } catch (popErr) { + logger.warn("Pre-existing nested-repo dirty state could not be auto-restored", { + nestedDir, + error: popErr instanceof Error ? popErr.message : String(popErr), + }); + } + } } } } diff --git a/packages/coding-agent/test/task/isolation-runner.test.ts b/packages/coding-agent/test/task/isolation-runner.test.ts new file mode 100644 index 000000000..d1f524e09 --- /dev/null +++ b/packages/coding-agent/test/task/isolation-runner.test.ts @@ -0,0 +1,120 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { applyEligibleNestedPatches, mergeIsolatedChanges } from "@oh-my-pi/pi-coding-agent/task/isolation-runner"; +import type { SingleResult } from "@oh-my-pi/pi-coding-agent/task/types"; +import * as worktreeModule from "@oh-my-pi/pi-coding-agent/task/worktree"; + +function result(overrides: Partial = {}): SingleResult { + return { + index: 0, + id: "NestedOnly", + agent: "task", + agentSource: "bundled", + task: "Do nested work", + assignment: "Do nested work", + exitCode: 0, + output: "done", + stderr: "", + truncated: false, + durationMs: 1, + tokens: 0, + requests: 0, + ...overrides, + }; +} + +describe("mergeIsolatedChanges", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("allows nested-only branch-mode patches to apply when no root branch was created", async () => { + const mergeSpy = vi.spyOn(worktreeModule, "mergeTaskBranches"); + const outcome = await mergeIsolatedChanges({ + repoRoot: "/repo", + mergeMode: "branch", + result: result({ + nestedPatches: [{ relativePath: "nested", patch: "diff --git a/file b/file\n" }], + }), + }); + + expect(mergeSpy).not.toHaveBeenCalled(); + expect(outcome.changesApplied).toBe(true); + expect(outcome.hadAnyChanges).toBe(true); + expect(outcome.mergedBranchForNestedPatches).toBe(true); + expect(outcome.summary).toContain("nested repository patches captured"); + }); + + it("does not mark failed branch-mode runs as nested-patch eligible", async () => { + const outcome = await mergeIsolatedChanges({ + repoRoot: "/repo", + mergeMode: "branch", + result: result({ + exitCode: 1, + nestedPatches: [{ relativePath: "nested", patch: "diff --git a/file b/file\n" }], + }), + }); + + expect(outcome.changesApplied).toBe(true); + expect(outcome.hadAnyChanges).toBe(false); + expect(outcome.mergedBranchForNestedPatches).toBe(false); + }); +}); + +describe("applyEligibleNestedPatches", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + const nestedPatch = { relativePath: "nested", patch: "diff --git a/file b/file\n" }; + + it("skips when patch-mode parent merge failed", async () => { + const applySpy = vi.spyOn(worktreeModule, "applyNestedPatches"); + const suffix = await applyEligibleNestedPatches({ + result: result({ nestedPatches: [nestedPatch] }), + repoRoot: "/repo", + mergeMode: "patch", + changesApplied: false, + mergedBranchForNestedPatches: false, + }); + expect(suffix).toBe(""); + expect(applySpy).not.toHaveBeenCalled(); + }); + + it("skips when branch mode did not actually merge the root branch", async () => { + const applySpy = vi.spyOn(worktreeModule, "applyNestedPatches"); + const suffix = await applyEligibleNestedPatches({ + result: result({ nestedPatches: [nestedPatch] }), + repoRoot: "/repo", + mergeMode: "branch", + changesApplied: true, + mergedBranchForNestedPatches: false, + }); + expect(suffix).toBe(""); + expect(applySpy).not.toHaveBeenCalled(); + }); + + it("applies nested patches and returns no warning on success", async () => { + const applySpy = vi.spyOn(worktreeModule, "applyNestedPatches").mockResolvedValue(); + const suffix = await applyEligibleNestedPatches({ + result: result({ nestedPatches: [nestedPatch] }), + repoRoot: "/repo", + mergeMode: "patch", + changesApplied: true, + mergedBranchForNestedPatches: false, + }); + expect(suffix).toBe(""); + expect(applySpy).toHaveBeenCalledTimes(1); + }); + + it("returns a system-notification suffix on apply failure", async () => { + vi.spyOn(worktreeModule, "applyNestedPatches").mockRejectedValue(new Error("boom")); + const suffix = await applyEligibleNestedPatches({ + result: result({ nestedPatches: [nestedPatch] }), + repoRoot: "/repo", + mergeMode: "branch", + changesApplied: true, + mergedBranchForNestedPatches: true, + }); + expect(suffix).toContain("Some nested repository patches failed to apply"); + }); +}); diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index fdbc2d0da..aeec4a7db 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -3,6 +3,7 @@ import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import { + applyNestedPatches, captureBaseline, captureDeltaPatch, ensureIsolation, @@ -279,3 +280,88 @@ describe("getRepoRoot", () => { expect(await getRepoRoot(inner)).toBe(inner); }); }); + +describe("applyNestedPatches", () => { + let parentRepo: string; + let nestedRel: string; + let nestedDir: string; + + beforeEach(async () => { + parentRepo = await fs.mkdtemp(path.join(os.tmpdir(), "omp-nested-apply-")); + await runGit(parentRepo, ["init", "-q", "-b", "main"]); + await runGit(parentRepo, ["config", "user.email", "test@example.com"]); + await runGit(parentRepo, ["config", "user.name", "Test User"]); + await fs.writeFile(path.join(parentRepo, ".gitignore"), "sub/\n"); + await runGit(parentRepo, ["add", "."]); + await runGit(parentRepo, ["commit", "-q", "-m", "parent-init"]); + + nestedRel = "sub"; + nestedDir = path.join(parentRepo, nestedRel); + await fs.mkdir(nestedDir, { recursive: true }); + await runGit(nestedDir, ["init", "-q", "-b", "main"]); + await runGit(nestedDir, ["config", "user.email", "test@example.com"]); + await runGit(nestedDir, ["config", "user.name", "Test User"]); + await fs.writeFile(path.join(nestedDir, "file.txt"), "v1\n"); + await runGit(nestedDir, ["add", "."]); + await runGit(nestedDir, ["commit", "-q", "-m", "nested-init"]); + }); + + afterEach(async () => { + await fs.rm(parentRepo, { recursive: true, force: true }); + }); + + it("does not fold pre-existing dirty nested-repo state into the agent commit", async () => { + // User has unrelated work-in-progress in the nested repo before the agent runs. + await fs.writeFile(path.join(nestedDir, "other.txt"), "user wip\n"); + + const patch = + "diff --git a/file.txt b/file.txt\n" + + "--- a/file.txt\n" + + "+++ b/file.txt\n" + + "@@ -1 +1 @@\n" + + "-v1\n" + + "+v2\n"; + await applyNestedPatches(parentRepo, [{ relativePath: nestedRel, patch }]); + + const [committedFiles, headContent, otherContent, statusPorcelain] = await Promise.all([ + runGit(nestedDir, ["log", "-1", "--name-only", "--pretty=format:"]), + fs.readFile(path.join(nestedDir, "file.txt"), "utf8"), + fs.readFile(path.join(nestedDir, "other.txt"), "utf8"), + runGit(nestedDir, ["status", "--porcelain=v1"]), + ]); + expect(committedFiles.trim()).toBe("file.txt"); + expect(headContent).toBe("v2\n"); + expect(otherContent).toBe("user wip\n"); + expect(statusPorcelain).toBe("?? other.txt"); + }); + + it("restores pre-existing staged WIP to the index, not just the working tree", async () => { + // Pre-existing tracked file with a staged edit; the patch should leave + // this entirely alone, and the stash pop must re-stage it (--index). + await fs.writeFile(path.join(nestedDir, "other.txt"), "tracked v1\n"); + await runGit(nestedDir, ["add", "other.txt"]); + await runGit(nestedDir, ["commit", "-q", "-m", "add-other"]); + await fs.writeFile(path.join(nestedDir, "other.txt"), "staged wip\n"); + await runGit(nestedDir, ["add", "other.txt"]); + + const patch = + "diff --git a/file.txt b/file.txt\n" + + "--- a/file.txt\n" + + "+++ b/file.txt\n" + + "@@ -1 +1 @@\n" + + "-v1\n" + + "+v2\n"; + await applyNestedPatches(parentRepo, [{ relativePath: nestedRel, patch }]); + + const [committedFiles, statusPorcelain, cachedDiff] = await Promise.all([ + runGit(nestedDir, ["log", "-1", "--name-only", "--pretty=format:"]), + runGit(nestedDir, ["status", "--porcelain=v1"]), + runGit(nestedDir, ["diff", "--cached", "--", "other.txt"]), + ]); + expect(committedFiles.trim()).toBe("file.txt"); + // Leading "M " (with trailing space) marks an index-only modification — + // "M" in the first slot, " " in the second. " M" would mean unstaged. + expect(statusPorcelain).toBe("M other.txt"); + expect(cachedDiff).toContain("+staged wip"); + }); +});