fix(eval): preserved returnHandle artifacts and nested branch patches
Eval preludes now forward returnHandle to the bridge so no-session eval runs can preserve the temp artifacts backing returned agent:// handles. The bridge keeps those temporary artifact directories whenever returnHandle is requested, including non-isolated runs and successful isolated applies. Branch-mode isolation now treats nested-only changes as merge-eligible even when no root branch was produced, letting callers apply nested patches instead of dropping them when the root repo had no diff. Added regression coverage for returnHandle artifact preservation and nested-only branch isolation. Fixes #3196
This commit is contained in:
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -26,12 +26,15 @@ type AgentHelper = (prompt: string, opts?: Record<string, unknown>) => Promise<u
|
||||
describe("eval js agent() returnHandle", () => {
|
||||
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<string, unknown> | undefined;
|
||||
const sandbox = loadPrelude(async (name, args) => {
|
||||
seenName = name;
|
||||
seenArgs = args as Record<string, unknown>;
|
||||
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",
|
||||
|
||||
@@ -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 });
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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, [
|
||||
|
||||
@@ -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> = {}): 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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user