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 39347f3e5..ee272ca6d 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -843,6 +843,17 @@ describe("runEvalAgent isolation", () => { 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(); @@ -986,4 +997,26 @@ describe("runEvalAgent isolation", () => { ); 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", 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 ae8ca156c..53930c485 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", diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index 60459cec2..a246d60f5 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -49,6 +49,7 @@ const agentArgsSchema = type({ "isolated?": "boolean", "apply?": "boolean", "merge?": "boolean", + "returnHandle?": "boolean", }); interface EvalAgentArgs { @@ -80,6 +81,8 @@ interface EvalAgentArgs { * 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 { @@ -483,13 +486,13 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption } // Clean up the temp artifacts dir we created for this call only when the - // caller will not need the captured patch later. We keep it for two cases: - // * a failed apply (`changesApplied === false`) so the user can recover - // from `result.patchPath` manually — matches TaskTool behavior. - // * `apply=false` (`changesApplied === null`), where the caller asked us - // not to merge and consumes `details.patchPath` / `details.branchName` - // out of band. - const shouldCleanupTempArtifacts = tempArtifactsDir && (!isIsolated || changesApplied === true); + // 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), on a failed apply (`changesApplied === false`), and on + // `apply=false` (`changesApplied === null`) where the caller consumes + // `details.patchPath` / `details.branchName` out of band. + const shouldCleanupTempArtifacts = + tempArtifactsDir && !parsed.returnHandle && (!isIsolated || changesApplied === true); if (shouldCleanupTempArtifacts) { await fs.rm(artifactsDir, { recursive: true, force: true }); } diff --git a/packages/coding-agent/src/eval/js/shared/prelude.txt b/packages/coding-agent/src/eval/js/shared/prelude.txt index b5903b696..afe0fc90e 100644 --- a/packages/coding-agent/src/eval/js/shared/prelude.txt +++ b/packages/coding-agent/src/eval/js/shared/prelude.txt @@ -125,7 +125,7 @@ if (!globalThis.__omp_js_prelude_loaded__) { "{ 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; diff --git a/packages/coding-agent/src/eval/py/prelude.py b/packages/coding-agent/src/eval/py/prelude.py index 5f451b648..c89e21b70 100644 --- a/packages/coding-agent/src/eval/py/prelude.py +++ b/packages/coding-agent/src/eval/py/prelude.py @@ -573,6 +573,8 @@ if "__omp_prelude_loaded__" not in globals(): 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 diff --git a/packages/coding-agent/src/task/isolation-runner.ts b/packages/coding-agent/src/task/isolation-runner.ts index 9b84753d2..fd90812e5 100644 --- a/packages/coding-agent/src/task/isolation-runner.ts +++ b/packages/coding-agent/src/task/isolation-runner.ts @@ -197,12 +197,16 @@ 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: "\n\nNo changes to apply.", + summary: canApplyNestedOnly + ? "\n\nNo root changes to apply; nested repository patches captured." + : "\n\nNo changes to apply.", changesApplied: true, - hadAnyChanges: false, - mergedBranchForNestedPatches: false, + hadAnyChanges: canApplyNestedOnly, + mergedBranchForNestedPatches: canApplyNestedOnly, }; } const mergeResult = await mergeTaskBranches(repoRoot, [ 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..6db4bab08 --- /dev/null +++ b/packages/coding-agent/test/task/isolation-runner.test.ts @@ -0,0 +1,61 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { 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); + }); +});