From eeed4b93b68637220fc1ead833a692e1703a151b Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 21 Jun 2026 14:18:09 +0000 Subject: [PATCH 01/17] feat(eval): added isolated/apply/merge options to agent() helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workflowz eval path bypasses the task tool's isolation wrapper and calls runSubprocess() directly, so parallel agent() fan-outs that edit overlapping files all land in the parent worktree. Extends the eval agent bridge schema with isolated/apply/merge, forwards them through the Python and JS preludes, and adds a shared task/isolation-runner.ts so the lifecycle (prepare context → run in worktree → capture patch/branch → merge → cleanup) is implemented once for both TaskTool and the bridge. Default mirrors task.isolation.mode: isolated by default when settings allow it, off when mode === 'none'. isolated=False explicitly disables; isolated=True with mode === 'none' errors out to match the task tool. apply=false keeps captured changes inside the worktree and surfaces the patch path / branch name in details. merge=false forces patch mode even when task.isolation.merge === 'branch'. Fixes #3196 --- packages/coding-agent/CHANGELOG.md | 4 + .../src/eval/__tests__/agent-bridge.test.ts | 145 ++++++++ .../coding-agent/src/eval/agent-bridge.ts | 312 ++++++++++++++---- .../src/eval/js/shared/prelude.txt | 8 +- packages/coding-agent/src/eval/py/prelude.py | 23 +- .../src/prompts/system/workflow-notice.md | 2 +- packages/coding-agent/src/task/index.ts | 223 +++---------- .../coding-agent/src/task/isolation-runner.ts | 283 ++++++++++++++++ 8 files changed, 763 insertions(+), 237 deletions(-) create mode 100644 packages/coding-agent/src/task/isolation-runner.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 001c32bcd..cb759c259 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()` so `workflowz`-driven fan-outs can request the same copy-on-write worktree isolation the `task` tool offers (defaults track `task.isolation.mode`; `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)) + ## [16.1.10] - 2026-06-21 ### Added 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..9dd69aba6 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -6,6 +6,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 +768,147 @@ 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("inherits isolation from settings by default and skips it when isolated=false explicitly", 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) — settings drive isolation on. + const inheritResult = await runEvalAgent({ prompt: "default" }, { session: isolatedSession() }); + expect(isolatedSpy).toHaveBeenCalledTimes(1); + expect(plainSpy).not.toHaveBeenCalled(); + expect(inheritResult.details.isolated).toBe(true); + + // Explicit isolated=false — bypass isolation even though setting allows it. + const explicitOff = await runEvalAgent({ prompt: "off", isolated: false }, { session: isolatedSession() }); + expect(isolatedSpy).toHaveBeenCalledTimes(1); + expect(plainSpy).toHaveBeenCalledTimes(1); + expect(explicitOff.details.isolated).toBeUndefined(); + expect(explicitOff.details.changesApplied).toBeUndefined(); + expect(mergeSpy).toHaveBeenCalledTimes(1); + }); + + 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", 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("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", 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", 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"); + }); +}); diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index ab9b87a06..884177ecf 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -12,9 +12,18 @@ 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 { + type IsolationContext, + mergeIsolatedChanges, + prepareIsolationContext, + runIsolatedSubprocess, +} from "../task/isolation-runner"; import { AgentOutputManager } from "../task/output-manager"; import type { AgentDefinition, AgentProgress, SingleResult } from "../task/types"; +import { applyNestedPatches, parseIsolationMode } from "../task/worktree"; +import { generateCommitMessage } from "../utils/commit-message-generator"; import type { ToolSession } from "../tools"; import { ToolError } from "../tools/tool-errors"; import { withBridgeTimeoutPause } from "./bridge-timeout"; @@ -37,6 +46,9 @@ const agentArgsSchema = type({ "model?": "string>0|string>0[]", "label?": "string", "schema?": "unknown", + "isolated?": "boolean", + "apply?": "boolean", + "merge?": "boolean", }); interface EvalAgentArgs { @@ -45,6 +57,29 @@ interface EvalAgentArgs { model?: string | string[]; label?: string; schema?: unknown; + /** + * Run this subagent inside an isolation worktree (copy-on-write of the + * parent repo). Defaults to the session's `task.isolation.mode`: opt-in + * when isolation is enabled, off when `mode === "none"`. Passing `false` + * explicitly overrides the inherited default; passing `true` while + * `mode === "none"` errors out to match the `task` tool. + */ + 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; } export interface EvalAgentBridgeOptions { @@ -60,6 +95,20 @@ 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; + /** + * 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; }; } @@ -129,15 +178,24 @@ 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 }; } function emitProgressStatus(emitStatus: ((event: JsStatusEvent) => void) | undefined, progress: AgentProgress): void { @@ -235,71 +293,205 @@ 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(); + + // Isolation gating. Default to the session's `task.isolation.mode` — opt + // out only when the caller explicitly passes `isolated=False`. `isolated=True` + // while the mode is "none" mirrors the `task` tool: surface a clear error + // instead of silently downgrading. + const isolationMode = options.session.settings.get("task.isolation.mode"); + const isolationEnabledInSettings = isolationMode !== "none"; + let isIsolated: boolean; + if (parsed.isolated === true) { + if (!isolationEnabledInSettings) { + throw new ToolError( + `agent(isolated=True) requires task.isolation.mode to be set; current mode is "none".`, + ); + } + isIsolated = true; + } else if (parsed.isolated === false) { + isIsolated = false; + } else { + isIsolated = isolationEnabledInSettings; + } + const settingsMergeMode = options.session.settings.get("task.isolation.merge"); + const mergeMode: "patch" | "branch" = parsed.merge === false ? "patch" : settingsMergeMode; + const applyChanges = parsed.apply !== false; + + 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; + + const commitStyle = options.session.settings.get("task.isolation.commits"); + const buildCommitMessage = () => + commitStyle === "ai" && options.session.modelRegistry + ? async (diff: string) => { + return generateCommitMessage( + diff, + options.session.modelRegistry!, + options.session.settings, + options.session.getSessionId?.() ?? undefined, + ); + } + : undefined; + + 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 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), + const result = await withBridgeTimeoutPause(options.emitStatus, async () => { + if (!isolationContext) { + return taskExecutor.runSubprocess(baseRunOptions); + } + const taskStart = Date.now(); + return runIsolatedSubprocess({ + baseOptions: baseRunOptions, + context: isolationContext, + preferredBackend, + agentId: id, + mergeMode, 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. - }), - ); + 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; + + // Apply nested repo patches (separate from parent git) under the same + // gating TaskTool uses: only when the parent merge made it through. + if (mergeMode === "branch" || outcome.changesApplied !== false) { + const nestedPatches = result.nestedPatches ?? []; + const eligible = + nestedPatches.length > 0 && + result.exitCode === 0 && + !result.aborted && + (mergeMode !== "branch" || outcome.mergedBranchForNestedPatches); + if (eligible) { + try { + await applyNestedPatches(isolationContext.repoRoot, nestedPatches, buildCommitMessage()); + } catch { + // Nested patch failures are non-fatal to the parent merge + mergeSummary += + "\n\nSome nested repository patches failed to apply."; + } + } + } + } 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 { + mergeSummary = "\n\nIsolation: no changes captured."; + } + } + + // Clean up the temp artifacts dir we created for this call when isolation + // did not need to preserve the patch artifact (TaskTool follows the same + // invariant). A failed apply (`changesApplied === false`) keeps the dir so + // the caller can recover from `result.patchPath` manually. + const shouldCleanupTempArtifacts = + tempArtifactsDir && (!isIsolated || changesApplied === true || changesApplied === null); + if (shouldCleanupTempArtifacts) { + await fs.rm(artifactsDir, { recursive: true, force: true }); + } + options.session.recordEvalSubagentUsage?.(result.usage?.output ?? 0); // The final `onProgress` flush from `runSubprocess` already emits a @@ -308,12 +500,16 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption // coalesce over the richer one and drop those stats. return { - text: result.output, + text: result.output + mergeSummary, details: { agent: result.agent, id: result.id, model: result.resolvedModel ?? modelOverride, structured, + isolated: isIsolated || undefined, + patchPath: result.patchPath, + branchName: result.branchName, + changesApplied: isIsolated ? changesApplied : undefined, }, }; } diff --git a/packages/coding-agent/src/eval/js/shared/prelude.txt b/packages/coding-agent/src/eval/js/shared/prelude.txt index 5225dc686..1bead6818 100644 --- a/packages/coding-agent/src/eval/js/shared/prelude.txt +++ b/packages/coding-agent/src/eval/js/shared/prelude.txt @@ -117,7 +117,13 @@ 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 text = res && typeof res === "object" ? res.text : res; diff --git a/packages/coding-agent/src/eval/py/prelude.py b/packages/coding-agent/src/eval/py/prelude.py index 96112fff0..3289c484f 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,6 +529,21 @@ 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. The default tracks the session's + `task.isolation.mode`: opt-in when isolation is enabled, off when set + to "none". `isolated=False` explicitly disables isolation even when + the setting would default it on; `isolated=True` while the mode is + "none" errors the call instead of silently downgrading. + + When isolated, `apply=False` keeps captured changes inside the + worktree and surfaces the patch artifact / branch name in the + returned details so the caller can inspect or apply manually. + `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 @@ -547,6 +562,12 @@ 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) 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/prompts/system/workflow-notice.md b/packages/coding-agent/src/prompts/system/workflow-notice.md index a8e1c6f55..951f9df05 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)` — 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; default tracks `task.isolation.mode` (on when it's not `"none"`), and `isolated=False` explicitly disables it. With isolation, `apply=False` keeps changes in the worktree (patch path / branch name returned in details), and `merge=False` forces patch mode even when the setting is `"branch"`. - `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..71806b7ac 100644 --- a/packages/coding-agent/src/task/index.ts +++ b/packages/coding-agent/src/task/index.ts @@ -48,7 +48,6 @@ 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 { generateTaskName } from "./name-generator"; @@ -57,19 +56,12 @@ 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"; + type IsolationContext, + mergeIsolatedChanges, + prepareIsolationContext, + runIsolatedSubprocess, +} from "./isolation-runner"; +import { applyNestedPatches, parseIsolationMode } from "./worktree"; function renderSubagentUserPrompt(assignment: string): string { return prompt.render(subagentUserPromptTemplate, { @@ -1118,12 +1110,10 @@ 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) 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..e7db66890 --- /dev/null +++ b/packages/coding-agent/src/task/isolation-runner.ts @@ -0,0 +1,283 @@ +/** + * 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 * as natives from "@oh-my-pi/pi-natives"; +import * as git from "../utils/git"; +import type { ExecutorOptions } from "./executor"; +import { runSubprocess } from "./executor"; +import type { SingleResult } from "./types"; +import { + 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. */ +type BuildCommitMessage = () => undefined | ((diff: string) => Promise); + +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") { + if (!result.branchName || result.exitCode !== 0 || result.aborted) { + return { + summary: "\n\nNo changes to apply.", + changesApplied: true, + hadAnyChanges: false, + mergedBranchForNestedPatches: false, + }; + } + 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, + }; + } +} From 6a22499dcdd197f7366f8849198b5bd169419547 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 21 Jun 2026 17:20:14 +0000 Subject: [PATCH 02/17] style: bun run fix --- .../src/eval/__tests__/agent-bridge.test.ts | 19 ++++++++++--------- .../coding-agent/src/eval/agent-bridge.ts | 6 ++---- packages/coding-agent/src/task/index.ts | 10 +++++----- .../coding-agent/src/task/isolation-runner.ts | 2 +- 4 files changed, 18 insertions(+), 19 deletions(-) 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 9dd69aba6..fd651f230 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -774,9 +774,7 @@ describe("runEvalAgent isolation", () => { vi.restoreAllMocks(); }); - function isolatedSession( - overrides: Partial[0]> = {}, - ): ToolSession { + function isolatedSession(overrides: Partial[0]> = {}): ToolSession { return makeSession({ settings: Settings.isolated({ "async.enabled": false, @@ -816,15 +814,18 @@ describe("runEvalAgent isolation", () => { it("inherits isolation from settings by default and skips it when isolated=false explicitly", async () => { mockAgents(); mockIsolationContext(); - const isolatedSpy = vi.spyOn(isolationRunner, "runIsolatedSubprocess").mockImplementation(async opts => - singleResult(opts.baseOptions, { output: "isolated-run" }), - ); + 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 }); + const mergeSpy = vi.spyOn(isolationRunner, "mergeIsolatedChanges").mockResolvedValue({ + summary: "", + changesApplied: true, + hadAnyChanges: false, + mergedBranchForNestedPatches: false, + }); // Default (no isolated arg) — settings drive isolation on. const inheritResult = await runEvalAgent({ prompt: "default" }, { session: isolatedSession() }); diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index 884177ecf..1b8d4074a 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -23,9 +23,9 @@ import { import { AgentOutputManager } from "../task/output-manager"; import type { AgentDefinition, AgentProgress, SingleResult } from "../task/types"; import { applyNestedPatches, parseIsolationMode } from "../task/worktree"; -import { generateCommitMessage } from "../utils/commit-message-generator"; import type { ToolSession } from "../tools"; import { ToolError } from "../tools/tool-errors"; +import { generateCommitMessage } from "../utils/commit-message-generator"; import { withBridgeTimeoutPause } from "./bridge-timeout"; import type { JsStatusEvent } from "./js/shared/types"; // Import review tools for side effects (registers subagent tool handlers). @@ -307,9 +307,7 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption let isIsolated: boolean; if (parsed.isolated === true) { if (!isolationEnabledInSettings) { - throw new ToolError( - `agent(isolated=True) requires task.isolation.mode to be set; current mode is "none".`, - ); + throw new ToolError(`agent(isolated=True) requires task.isolation.mode to be set; current mode is "none".`); } isIsolated = true; } else if (parsed.isolated === false) { diff --git a/packages/coding-agent/src/task/index.ts b/packages/coding-agent/src/task/index.ts index 71806b7ac..9b4ba9d02 100644 --- a/packages/coding-agent/src/task/index.ts +++ b/packages/coding-agent/src/task/index.ts @@ -50,17 +50,17 @@ import { AgentRegistry, MAIN_AGENT_ID } from "../registry/agent-registry"; import { generateCommitMessage } from "../utils/commit-message-generator"; import { type DiscoveryResult, discoverAgents, getAgent } from "./discovery"; import { runSubprocess } from "./executor"; -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 { type IsolationContext, 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, parseIsolationMode } from "./worktree"; function renderSubagentUserPrompt(assignment: string): string { diff --git a/packages/coding-agent/src/task/isolation-runner.ts b/packages/coding-agent/src/task/isolation-runner.ts index e7db66890..9b84753d2 100644 --- a/packages/coding-agent/src/task/isolation-runner.ts +++ b/packages/coding-agent/src/task/isolation-runner.ts @@ -19,7 +19,7 @@ * before mutation); steps 2 and 3 are per-spawn. */ import * as path from "node:path"; -import * as natives from "@oh-my-pi/pi-natives"; +import type * as natives from "@oh-my-pi/pi-natives"; import * as git from "../utils/git"; import type { ExecutorOptions } from "./executor"; import { runSubprocess } from "./executor"; From 91cf5ec97ca7cd6e2974f20ba2c5a897bc39dc9f Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 21 Jun 2026 17:25:49 +0000 Subject: [PATCH 03/17] fix(eval): kept isolated schema output parseable Kept isolation merge/apply summaries out of agent() text when a schema is supplied so Python and JS eval helpers can still parse the JSON payload. The summary now lands in details.isolationSummary for callers that need the human-readable apply state. Added a regression test that exercises an isolated schema-backed eval agent with a merge summary. Fixes #3196 --- .../src/eval/__tests__/agent-bridge.test.ts | 34 +++++++++++++++++++ .../coding-agent/src/eval/agent-bridge.ts | 5 ++- 2 files changed, 38 insertions(+), 1 deletion(-) 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 fd651f230..19c5da57d 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -872,6 +872,40 @@ describe("runEvalAgent isolation", () => { expect(result.text).toContain("Applied patches: yes"); }); + 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", + 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("skips the merge phase when apply=false and surfaces the patch artifact instead", async () => { mockAgents(); mockIsolationContext(); diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index 1b8d4074a..ded74f100 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -109,6 +109,8 @@ export interface EvalAgentResult { * Omitted for non-isolated runs. */ changesApplied?: boolean | null; + /** Human-readable isolation apply/merge summary; kept out of schema-backed `text`. */ + isolationSummary?: string; }; } @@ -498,7 +500,7 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption // coalesce over the richer one and drop those stats. return { - text: result.output + mergeSummary, + text: structured ? result.output : result.output + mergeSummary, details: { agent: result.agent, id: result.id, @@ -508,6 +510,7 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption patchPath: result.patchPath, branchName: result.branchName, changesApplied: isIsolated ? changesApplied : undefined, + isolationSummary: mergeSummary ? mergeSummary.trim() : undefined, }, }; } From 068a27b07f8f81aef04b419d7c58687f59474c8c Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 21 Jun 2026 17:28:16 +0000 Subject: [PATCH 04/17] fix(eval): preserved temp artifacts when isolated agent() runs with apply=false The cleanup gate was treating changesApplied===null (apply=false) the same as a clean apply, deleting the temp artifacts dir before returning details.patchPath. Sessions without a session file (which fall back to a per-call tmp dir) ended up with a patchPath pointing at a removed file, defeating the documented manual-apply path. Tightened the cleanup condition to remove the temp dir only on a confirmed clean apply (changesApplied===true); apply=false and failed applies both keep the artifact for the caller. Added regression tests for the apply=false preserve case and the apply-succeeds cleanup case. Fixes #3196 --- .../src/eval/__tests__/agent-bridge.test.ts | 36 +++++++++++++++++++ .../coding-agent/src/eval/agent-bridge.ts | 14 ++++---- 2 files changed, 44 insertions(+), 6 deletions(-) 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 19c5da57d..c681b445d 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"; @@ -946,4 +947,39 @@ describe("runEvalAgent isolation", () => { expect(result.text).toContain("omp/task/"); 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", 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" }, { session: isolatedSession() }); + + const removedArtifactsDir = rmSpy.mock.calls.some(([target]) => typeof target === "string" && target.includes("omp-eval-agent-")); + expect(removedArtifactsDir).toBe(true); + }); }); diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index ded74f100..60459cec2 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -482,12 +482,14 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption } } - // Clean up the temp artifacts dir we created for this call when isolation - // did not need to preserve the patch artifact (TaskTool follows the same - // invariant). A failed apply (`changesApplied === false`) keeps the dir so - // the caller can recover from `result.patchPath` manually. - const shouldCleanupTempArtifacts = - tempArtifactsDir && (!isIsolated || changesApplied === true || changesApplied === null); + // 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); if (shouldCleanupTempArtifacts) { await fs.rm(artifactsDir, { recursive: true, force: true }); } From 9b33e0e64489057928619c2d246bed1bdb68b421 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 21 Jun 2026 17:28:23 +0000 Subject: [PATCH 05/17] style: bun run fix --- .../coding-agent/src/eval/__tests__/agent-bridge.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 c681b445d..39347f3e5 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -959,7 +959,9 @@ describe("runEvalAgent isolation", () => { const result = await runEvalAgent({ prompt: "scout", 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-")); + const removedArtifactsDir = rmSpy.mock.calls.some( + ([target]) => typeof target === "string" && target.includes("omp-eval-agent-"), + ); expect(removedArtifactsDir).toBe(false); }); @@ -979,7 +981,9 @@ describe("runEvalAgent isolation", () => { await runEvalAgent({ prompt: "scout" }, { session: isolatedSession() }); - const removedArtifactsDir = rmSpy.mock.calls.some(([target]) => typeof target === "string" && target.includes("omp-eval-agent-")); + const removedArtifactsDir = rmSpy.mock.calls.some( + ([target]) => typeof target === "string" && target.includes("omp-eval-agent-"), + ); expect(removedArtifactsDir).toBe(true); }); }); From 54f4652573cd90d08d3790bcbff498878e404040 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 21 Jun 2026 17:35:26 +0000 Subject: [PATCH 06/17] fix(eval): exposed apply=false artifacts on the agent() return_handle node When agent() ran with schema and apply=false, the bridge correctly returned the captured patch/branch in details, but the preludes only forwarded id/agent/handle/data on the returnHandle node. Structured workflows had no way to recover the artifact for a manual apply. Both runtimes now copy isolated, patchPath/branchName, changesApplied, and isolationSummary onto the returnHandle node (snake_case in Python, camelCase in JS), keeping null changesApplied so apply=false stays distinguishable from a successful apply. Updated the workflow notice and the Python agent() docstring to point callers at return_handle as the artifact escape hatch for isolated+apply=false runs. Added prelude tests locking the new node shape in both runtimes. Fixes #3196 --- .../src/eval/__tests__/prelude-agent.test.ts | 29 +++++++++++++++++++ .../src/eval/js/shared/prelude.txt | 3 ++ .../src/eval/py/__tests__/prelude.test.ts | 12 ++++++++ packages/coding-agent/src/eval/py/prelude.py | 28 +++++++++++++----- .../src/prompts/system/workflow-notice.md | 2 +- 5 files changed, 66 insertions(+), 8 deletions(-) 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..ae8ca156c 100644 --- a/packages/coding-agent/src/eval/__tests__/prelude-agent.test.ts +++ b/packages/coding-agent/src/eval/__tests__/prelude-agent.test.ts @@ -70,4 +70,33 @@ 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/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, + 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.changesApplied).toBeNull(); + expect(node.isolationSummary).toContain("/artifacts/iso-1.patch"); + expect("branchName" in node).toBe(false); + }); }); diff --git a/packages/coding-agent/src/eval/js/shared/prelude.txt b/packages/coding-agent/src/eval/js/shared/prelude.txt index 1bead6818..b5903b696 100644 --- a/packages/coding-agent/src/eval/js/shared/prelude.txt +++ b/packages/coding-agent/src/eval/js/shared/prelude.txt @@ -135,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", "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..24dd99d5a 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,16 @@ 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 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 its captured patch. + expect(PYTHON_PRELUDE).toContain('("patchPath", "patch_path")'); + expect(PYTHON_PRELUDE).toContain('("branchName", "branch_name")'); + 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 3289c484f..5f451b648 100644 --- a/packages/coding-agent/src/eval/py/prelude.py +++ b/packages/coding-agent/src/eval/py/prelude.py @@ -538,11 +538,12 @@ if "__omp_prelude_loaded__" not in globals(): "none" errors the call instead of silently downgrading. When isolated, `apply=False` keeps captured changes inside the - worktree and surfaces the patch artifact / branch name in the - returned details so the caller can inspect or apply manually. - `merge=False` forces patch mode even when `task.isolation.merge` is - `"branch"`, avoiding the per-call git lock + repo mutation that - branch mode performs. + worktree and surfaces the patch artifact / branch name 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 the artifact). `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`` @@ -550,8 +551,12 @@ if "__omp_prelude_loaded__" not in globals(): ``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"``, ``"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: @@ -585,6 +590,15 @@ 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"), + ("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/prompts/system/workflow-notice.md b/packages/coding-agent/src/prompts/system/workflow-notice.md index 951f9df05..7a3fe84ac 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, isolated=None, apply=None, merge=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. Pass `isolated=True` to run the spawn in a copy-on-write worktree so parallel `agent()` calls can edit overlapping files safely; default tracks `task.isolation.mode` (on when it's not `"none"`), and `isolated=False` explicitly disables it. With isolation, `apply=False` keeps changes in the worktree (patch path / branch name returned in details), and `merge=False` forces patch mode even when the setting is `"branch"`. +- `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; default tracks `task.isolation.mode` (on when it's not `"none"`), and `isolated=False` explicitly disables it. With isolation, `apply=False` keeps changes in the worktree, and `merge=False` forces patch mode even when the setting is `"branch"`. The captured patch path / branch name only reaches the workflow through `return_handle=True` — combine it with `apply=False` (or `apply=False, schema=…`) and read `node["patch_path"]`, `node["branch_name"]`, `node["changes_applied"]`, `node["isolation_summary"]` (JS: same keys camelCased) to recover the artifact. - `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. From a6ee95df087e94adf4ae67de9f8bf84f6e2050c5 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 03:59:17 +0000 Subject: [PATCH 07/17] 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 --- .../src/eval/__tests__/agent-bridge.test.ts | 33 ++++++++++ .../src/eval/__tests__/prelude-agent.test.ts | 5 +- .../coding-agent/src/eval/agent-bridge.ts | 17 +++--- .../src/eval/js/shared/prelude.txt | 2 +- packages/coding-agent/src/eval/py/prelude.py | 2 + .../coding-agent/src/task/isolation-runner.ts | 10 ++- .../test/task/isolation-runner.test.ts | 61 +++++++++++++++++++ 7 files changed, 118 insertions(+), 12 deletions(-) create mode 100644 packages/coding-agent/test/task/isolation-runner.test.ts 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); + }); +}); From 636fbb4fccf628719b022079e3a080eb3b54de1f Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 03:59:27 +0000 Subject: [PATCH 08/17] style: bun run fix --- packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 ee272ca6d..627e4dd03 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -850,7 +850,9 @@ describe("runEvalAgent isolation", () => { await runEvalAgent({ prompt: "plain handle", returnHandle: true }, { session: makeSession() }); - const removedArtifactsDir = rmSpy.mock.calls.some(([target]) => typeof target === "string" && target.includes("omp-eval-agent-")); + const removedArtifactsDir = rmSpy.mock.calls.some( + ([target]) => typeof target === "string" && target.includes("omp-eval-agent-"), + ); expect(removedArtifactsDir).toBe(false); }); From 3677e71e9dacf5195f6116620cb3668b2c2fa94d Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 04:08:56 +0000 Subject: [PATCH 09/17] fix(eval): exposed nested apply=false patches Branch-mode isolation can capture nested repository changes without creating a root branch. Eval agent() with apply=false previously treated that shape as no captured changes and returned no recoverable nested patch payload after the isolation worktree was removed. Expose captured nested patches in EvalAgentResult details and copy them onto JS/Python returnHandle nodes (nestedPatches / nested_patches). Document the return_handle escape hatch and add regression coverage for branch-mode nested-only apply=false runs. Fixes #3196 --- .../src/eval/__tests__/agent-bridge.test.ts | 22 +++++++++++++++++++ .../src/eval/__tests__/prelude-agent.test.ts | 4 +++- .../coding-agent/src/eval/agent-bridge.ts | 12 ++++++++-- .../src/eval/js/shared/prelude.txt | 2 +- .../src/eval/py/__tests__/prelude.test.ts | 9 ++++---- packages/coding-agent/src/eval/py/prelude.py | 19 +++++++++------- .../src/prompts/system/workflow-notice.md | 2 +- 7 files changed, 53 insertions(+), 17 deletions(-) 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 627e4dd03..fcaf867e8 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -961,6 +961,28 @@ describe("runEvalAgent isolation", () => { 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", 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(); 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 53930c485..6106a168c 100644 --- a/packages/coding-agent/src/eval/__tests__/prelude-agent.test.ts +++ b/packages/coding-agent/src/eval/__tests__/prelude-agent.test.ts @@ -74,7 +74,7 @@ describe("eval js agent() returnHandle", () => { expect(node).toEqual({ text: "lonely", output: "lonely", handle: null, id: null, agent: null }); }); - it("exposes patchPath/branchName/changesApplied/isolated/isolationSummary on the handle", async () => { + it("exposes patchPath/branchName/nestedPatches/changesApplied/isolated/isolationSummary on the handle", async () => { const payload = JSON.stringify({ ok: true }); const sandbox = loadPrelude(async () => ({ text: payload, @@ -85,6 +85,7 @@ describe("eval js agent() returnHandle", () => { 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.", }, })); @@ -98,6 +99,7 @@ describe("eval js agent() returnHandle", () => { 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 a246d60f5..3d6dd5e8a 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -22,7 +22,7 @@ import { } from "../task/isolation-runner"; import { AgentOutputManager } from "../task/output-manager"; import type { AgentDefinition, AgentProgress, SingleResult } from "../task/types"; -import { applyNestedPatches, parseIsolationMode } from "../task/worktree"; +import { applyNestedPatches, type NestedRepoPatch, parseIsolationMode } from "../task/worktree"; import type { ToolSession } from "../tools"; import { ToolError } from "../tools/tool-errors"; import { generateCommitMessage } from "../utils/commit-message-generator"; @@ -104,6 +104,8 @@ export interface EvalAgentResult { 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. @@ -481,7 +483,12 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption } else if (result.patchPath) { mergeSummary = `\n\nIsolation: changes captured at \`${result.patchPath}\` (apply=false). Not applied.`; } else { - mergeSummary = "\n\nIsolation: no changes captured."; + 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."; + } } } @@ -514,6 +521,7 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption 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/js/shared/prelude.txt b/packages/coding-agent/src/eval/js/shared/prelude.txt index afe0fc90e..21e33d406 100644 --- a/packages/coding-agent/src/eval/js/shared/prelude.txt +++ b/packages/coding-agent/src/eval/js/shared/prelude.txt @@ -135,7 +135,7 @@ 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", "changesApplied", "isolationSummary"]) { + 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 24dd99d5a..f9ef0e852 100644 --- a/packages/coding-agent/src/eval/py/__tests__/prelude.test.ts +++ b/packages/coding-agent/src/eval/py/__tests__/prelude.test.ts @@ -19,12 +19,13 @@ describe("python prelude", () => { 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 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 its captured patch. + // 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 c89e21b70..156d93320 100644 --- a/packages/coding-agent/src/eval/py/prelude.py +++ b/packages/coding-agent/src/eval/py/prelude.py @@ -538,12 +538,13 @@ if "__omp_prelude_loaded__" not in globals(): "none" errors the call instead of silently downgrading. When isolated, `apply=False` keeps captured changes inside the - worktree and surfaces the patch artifact / branch name 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 the artifact). `merge=False` forces patch - mode even when `task.isolation.merge` is `"branch"`, avoiding the - per-call git lock + repo mutation that branch mode performs. + 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`` @@ -553,8 +554,9 @@ if "__omp_prelude_loaded__" not in globals(): reference, never re-inlined. When ``schema`` is also set the parsed object lands under ``"data"``. When the spawn ran isolated the node also carries ``"isolated"`` and, when present, ``"patch_path"``, - ``"branch_name"``, ``"changes_applied"`` (``True``/``False``/``None`` - — ``None`` means ``apply=False``), and ``"isolation_summary"``. If + ``"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. """ @@ -596,6 +598,7 @@ if "__omp_prelude_loaded__" not in globals(): ("isolated", "isolated"), ("patchPath", "patch_path"), ("branchName", "branch_name"), + ("nestedPatches", "nested_patches"), ("changesApplied", "changes_applied"), ("isolationSummary", "isolation_summary"), ): diff --git a/packages/coding-agent/src/prompts/system/workflow-notice.md b/packages/coding-agent/src/prompts/system/workflow-notice.md index 7a3fe84ac..23b8273f5 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, 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; default tracks `task.isolation.mode` (on when it's not `"none"`), and `isolated=False` explicitly disables it. With isolation, `apply=False` keeps changes in the worktree, and `merge=False` forces patch mode even when the setting is `"branch"`. The captured patch path / branch name only reaches the workflow through `return_handle=True` — combine it with `apply=False` (or `apply=False, schema=…`) and read `node["patch_path"]`, `node["branch_name"]`, `node["changes_applied"]`, `node["isolation_summary"]` (JS: same keys camelCased) to recover the artifact. +- `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; default tracks `task.isolation.mode` (on when it's not `"none"`), and `isolated=False` explicitly disables it. 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. From c1b01f1cb922d793c85530901fb68e7b6e638c9d Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 04:12:43 +0000 Subject: [PATCH 10/17] fix(eval): surfaced isolated apply failures as ToolError A failed isolated apply (changesApplied === false) previously only set details.isolationSummary and returned the subagent text. Schema-backed agent() calls then parsed the JSON and returned the object, so workflows saw a successful structured result while none of the edits had landed. Throw a ToolError when mergeIsolatedChanges reports a failed apply, with the merge summary plus a recovery hint pointing at the preserved patch/branch/nested artifacts so the caller can apply manually. Added regression tests for the schema and non-schema apply-failure paths. Fixes #3196 --- .../src/eval/__tests__/agent-bridge.test.ts | 54 ++++++++++++++++++ .../coding-agent/src/eval/agent-bridge.ts | 55 ++++++++++++------- 2 files changed, 89 insertions(+), 20 deletions(-) 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 fcaf867e8..ac963efe8 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -920,6 +920,60 @@ describe("runEvalAgent isolation", () => { 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", + 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" }, { session })).rejects.toThrow( + /isolated apply failed.*Branch merge failed.*Captured branch preserved as omp\/task\//s, + ); + }); + it("skips the merge phase when apply=false and surfaces the patch artifact instead", async () => { mockAgents(); mockIsolationContext(); diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index 3d6dd5e8a..dca8842ee 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -458,24 +458,38 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption }); 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) { + recoveryParts.push( + `Captured nested repository patches (${result.nestedPatches.length}) preserved in details.nestedPatches.`, + ); + } + const recoveryHint = recoveryParts.length > 0 ? ` ${recoveryParts.join(" ")}` : ""; + throw new ToolError( + `agent() isolated apply failed for ${result.id}${summaryText ? `: ${summaryText}` : ""}${recoveryHint}`, + ); + } - // Apply nested repo patches (separate from parent git) under the same - // gating TaskTool uses: only when the parent merge made it through. - if (mergeMode === "branch" || outcome.changesApplied !== false) { - const nestedPatches = result.nestedPatches ?? []; - const eligible = - nestedPatches.length > 0 && - result.exitCode === 0 && - !result.aborted && - (mergeMode !== "branch" || outcome.mergedBranchForNestedPatches); - if (eligible) { - try { - await applyNestedPatches(isolationContext.repoRoot, nestedPatches, buildCommitMessage()); - } 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). The throw + // above already exited on a failed parent merge, so we know either + // the parent succeeded (patch mode) or branch mode is in play. + const nestedPatches = result.nestedPatches ?? []; + const eligible = + nestedPatches.length > 0 && + result.exitCode === 0 && + !result.aborted && + (mergeMode !== "branch" || outcome.mergedBranchForNestedPatches); + if (eligible) { + try { + await applyNestedPatches(isolationContext.repoRoot, nestedPatches, buildCommitMessage()); + } catch { + // Nested patch failures are non-fatal to the parent merge + mergeSummary += + "\n\nSome nested repository patches failed to apply."; } } } else if (result.branchName) { @@ -495,9 +509,10 @@ 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 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. + // 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) { From a48f1ea8137fcc3142dd97fcc2a48d0857e60266 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 04:21:11 +0000 Subject: [PATCH 11/17] fix(eval): persisted nested patches before throwing apply failures When an isolated apply fails the bridge throws a ToolError and never returns details, so the nested-patch payload that previously lived in details.nestedPatches was unrecoverable after the isolation worktree was torn down. The bridge now writes each captured nested patch to a file under the per-call artifacts dir (e.g. .nested--.patch) before throwing and includes the resolved paths in the error message so the caller can apply them manually. Added a regression test verifying the persisted file exists with the original patch contents and that the path is surfaced in the thrown error. Fixes #3196 --- .../src/eval/__tests__/agent-bridge.test.ts | 34 +++++++++++++++++++ .../coding-agent/src/eval/agent-bridge.ts | 26 +++++++++++++- 2 files changed, 59 insertions(+), 1 deletion(-) 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 ac963efe8..c03f09ab4 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -974,6 +974,40 @@ describe("runEvalAgent isolation", () => { ); }); + 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" }, { 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(); diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index dca8842ee..e6a110cb2 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -205,6 +205,29 @@ async function getArtifacts(session: ToolSession): Promise { 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 { if (!emitStatus) return; const preview = (progress.assignment ?? progress.task ?? "").split("\n")[0]?.slice(0, 120); @@ -464,8 +487,9 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption 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 in details.nestedPatches.`, + `Captured nested repository patches (${result.nestedPatches.length}) preserved at: ${nestedPaths.join(", ")}.`, ); } const recoveryHint = recoveryParts.length > 0 ? ` ${recoveryParts.join(" ")}` : ""; From fbb7255e8eeb8b5bfb80e274d0deea4825c62402 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 18:29:28 +0000 Subject: [PATCH 12/17] fix(eval): flipped isolated default to strict opt-in Per maintainer ruling on #3196, eval agent() now defaults to non-isolated regardless of task.isolation.mode, mirroring the task tool. isolated=true is the only way to turn it on; isolated=true while task.isolation.mode === "none" still throws the same clear error. Updated tests, workflow-notice.md, and Python agent() docstring to reflect the strict opt-in contract. Existing isolation tests now pass isolated:true explicitly; the inherit-from-settings assertion is replaced with a default-off + isolated=true opt-in regression. Fixes #3196 --- .../src/eval/__tests__/agent-bridge.test.ts | 41 ++++++++++--------- .../coding-agent/src/eval/agent-bridge.ts | 30 ++++++-------- packages/coding-agent/src/eval/py/prelude.py | 9 ++-- .../src/prompts/system/workflow-notice.md | 2 +- 4 files changed, 39 insertions(+), 43 deletions(-) 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 c03f09ab4..2f55b6a4c 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -812,7 +812,7 @@ describe("runEvalAgent isolation", () => { expect(runSpy).not.toHaveBeenCalled(); }); - it("inherits isolation from settings by default and skips it when isolated=false explicitly", async () => { + it("stays non-isolated by default even when task.isolation.mode is set; isolated=true opts in", async () => { mockAgents(); mockIsolationContext(); const isolatedSpy = vi @@ -828,18 +828,19 @@ describe("runEvalAgent isolation", () => { mergedBranchForNestedPatches: false, }); - // Default (no isolated arg) — settings drive isolation on. - const inheritResult = await runEvalAgent({ prompt: "default" }, { session: isolatedSession() }); - expect(isolatedSpy).toHaveBeenCalledTimes(1); - expect(plainSpy).not.toHaveBeenCalled(); - expect(inheritResult.details.isolated).toBe(true); + // 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=false — bypass isolation even though setting allows it. - const explicitOff = await runEvalAgent({ prompt: "off", isolated: false }, { session: isolatedSession() }); + // 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(explicitOff.details.isolated).toBeUndefined(); - expect(explicitOff.details.changesApplied).toBeUndefined(); + expect(explicitOn.details.isolated).toBe(true); expect(mergeSpy).toHaveBeenCalledTimes(1); }); @@ -874,7 +875,7 @@ describe("runEvalAgent isolation", () => { // 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", merge: false }, { session }); + const result = await runEvalAgent({ prompt: "migration", isolated: true, merge: false }, { session }); expect(isolatedSpy).toHaveBeenCalledTimes(1); const isolatedCall = isolatedSpy.mock.calls[0]?.[0]; @@ -906,6 +907,7 @@ describe("runEvalAgent isolation", () => { const result = await runEvalAgent( { prompt: "structured", + isolated: true, schema: { type: "object", properties: { status: { type: "string" } }, @@ -941,6 +943,7 @@ describe("runEvalAgent isolation", () => { runEvalAgent( { prompt: "structured", + isolated: true, schema: { type: "object", properties: { status: { type: "string" } }, @@ -969,7 +972,7 @@ describe("runEvalAgent isolation", () => { }); const session = isolatedSession({ "task.isolation.merge": "branch" }); - await expect(runEvalAgent({ prompt: "scout" }, { session })).rejects.toThrow( + await expect(runEvalAgent({ prompt: "scout", isolated: true }, { session })).rejects.toThrow( /isolated apply failed.*Branch merge failed.*Captured branch preserved as omp\/task\//s, ); }); @@ -994,7 +997,7 @@ describe("runEvalAgent isolation", () => { let caught: Error | undefined; try { - await runEvalAgent({ prompt: "scout" }, { session: isolatedSession() }); + await runEvalAgent({ prompt: "scout", isolated: true }, { session: isolatedSession() }); } catch (err) { caught = err as Error; } @@ -1019,7 +1022,7 @@ describe("runEvalAgent isolation", () => { ); const mergeSpy = vi.spyOn(isolationRunner, "mergeIsolatedChanges"); - const result = await runEvalAgent({ prompt: "scout", apply: false }, { session: isolatedSession() }); + const result = await runEvalAgent({ prompt: "scout", isolated: true, apply: false }, { session: isolatedSession() }); expect(mergeSpy).not.toHaveBeenCalled(); expect(result.details.isolated).toBe(true); @@ -1041,7 +1044,7 @@ describe("runEvalAgent isolation", () => { const mergeSpy = vi.spyOn(isolationRunner, "mergeIsolatedChanges"); const session = isolatedSession({ "task.isolation.merge": "branch" }); - const result = await runEvalAgent({ prompt: "scout", apply: false }, { session }); + const result = await runEvalAgent({ prompt: "scout", isolated: true, apply: false }, { session }); expect(mergeSpy).not.toHaveBeenCalled(); expect(result.details.branchName).toMatch(/^omp\/task\//); @@ -1061,7 +1064,7 @@ describe("runEvalAgent isolation", () => { const mergeSpy = vi.spyOn(isolationRunner, "mergeIsolatedChanges"); const session = isolatedSession({ "task.isolation.merge": "branch" }); - const result = await runEvalAgent({ prompt: "scout", apply: false }, { session }); + const result = await runEvalAgent({ prompt: "scout", isolated: true, apply: false }, { session }); expect(mergeSpy).not.toHaveBeenCalled(); expect(result.details.branchName).toBeUndefined(); @@ -1079,7 +1082,7 @@ describe("runEvalAgent isolation", () => { singleResult(opts.baseOptions, { output: "captured", patchPath: `/artifacts/${opts.agentId}.patch` }), ); - const result = await runEvalAgent({ prompt: "scout", apply: false }, { session: isolatedSession() }); + const result = await runEvalAgent({ prompt: "scout", isolated: true, apply: false }, { session: isolatedSession() }); expect(result.details.patchPath).toMatch(/\.patch$/); const removedArtifactsDir = rmSpy.mock.calls.some( @@ -1102,7 +1105,7 @@ describe("runEvalAgent isolation", () => { mergedBranchForNestedPatches: false, }); - await runEvalAgent({ prompt: "scout" }, { session: isolatedSession() }); + await runEvalAgent({ prompt: "scout", isolated: true }, { session: isolatedSession() }); const removedArtifactsDir = rmSpy.mock.calls.some( ([target]) => typeof target === "string" && target.includes("omp-eval-agent-"), @@ -1124,7 +1127,7 @@ describe("runEvalAgent isolation", () => { mergedBranchForNestedPatches: false, }); - await runEvalAgent({ prompt: "scout", returnHandle: true }, { session: isolatedSession() }); + 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-"), diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index e6a110cb2..ecd039d6f 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -60,10 +60,10 @@ interface EvalAgentArgs { schema?: unknown; /** * Run this subagent inside an isolation worktree (copy-on-write of the - * parent repo). Defaults to the session's `task.isolation.mode`: opt-in - * when isolation is enabled, off when `mode === "none"`. Passing `false` - * explicitly overrides the inherited default; passing `true` while - * `mode === "none"` errors out to match the `task` tool. + * 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; /** @@ -328,23 +328,17 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption const id = await outputManager.allocate(outputIdBase(parsed.label, agentName)); const assignment = parsed.prompt.trim(); - // Isolation gating. Default to the session's `task.isolation.mode` — opt - // out only when the caller explicitly passes `isolated=False`. `isolated=True` - // while the mode is "none" mirrors the `task` tool: surface a clear error - // instead of silently downgrading. + // 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"; - let isIsolated: boolean; - if (parsed.isolated === true) { - if (!isolationEnabledInSettings) { - throw new ToolError(`agent(isolated=True) requires task.isolation.mode to be set; current mode is "none".`); - } - isIsolated = true; - } else if (parsed.isolated === false) { - isIsolated = false; - } else { - isIsolated = isolationEnabledInSettings; + 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; diff --git a/packages/coding-agent/src/eval/py/prelude.py b/packages/coding-agent/src/eval/py/prelude.py index 156d93320..9d14eceee 100644 --- a/packages/coding-agent/src/eval/py/prelude.py +++ b/packages/coding-agent/src/eval/py/prelude.py @@ -531,11 +531,10 @@ if "__omp_prelude_loaded__" not in globals(): 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. The default tracks the session's - `task.isolation.mode`: opt-in when isolation is enabled, off when set - to "none". `isolated=False` explicitly disables isolation even when - the setting would default it on; `isolated=True` while the mode is - "none" errors the call instead of silently downgrading. + 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 diff --git a/packages/coding-agent/src/prompts/system/workflow-notice.md b/packages/coding-agent/src/prompts/system/workflow-notice.md index 23b8273f5..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, 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; default tracks `task.isolation.mode` (on when it's not `"none"`), and `isolated=False` explicitly disables it. 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. +- `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. From 7438efc5b3f9bc51078e9e6806855a2f085dab3c Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 18:29:34 +0000 Subject: [PATCH 13/17] style: bun run fix --- .../src/eval/__tests__/agent-bridge.test.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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 2f55b6a4c..b0cb20889 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -1022,7 +1022,10 @@ describe("runEvalAgent isolation", () => { ); const mergeSpy = vi.spyOn(isolationRunner, "mergeIsolatedChanges"); - const result = await runEvalAgent({ prompt: "scout", isolated: true, apply: false }, { session: isolatedSession() }); + const result = await runEvalAgent( + { prompt: "scout", isolated: true, apply: false }, + { session: isolatedSession() }, + ); expect(mergeSpy).not.toHaveBeenCalled(); expect(result.details.isolated).toBe(true); @@ -1082,7 +1085,10 @@ describe("runEvalAgent isolation", () => { singleResult(opts.baseOptions, { output: "captured", patchPath: `/artifacts/${opts.agentId}.patch` }), ); - const result = await runEvalAgent({ prompt: "scout", isolated: true, apply: false }, { session: isolatedSession() }); + const result = await runEvalAgent( + { prompt: "scout", isolated: true, apply: false }, + { session: isolatedSession() }, + ); expect(result.details.patchPath).toMatch(/\.patch$/); const removedArtifactsDir = rmSpy.mock.calls.some( From 15ae84f0defb3067c081bdb2b3106771ec8443f2 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 18:41:44 +0000 Subject: [PATCH 14/17] fix(eval): paused timeout through isolation merge/apply Previously withBridgeTimeoutPause only wrapped the subagent subprocess; mergeIsolatedChanges, applyNestedPatches, nested commit-message generation, and artifact cleanup ran with the eval watchdog re-armed. A cherry-pick or large patch apply could trip the cell timeout and abort successful post-processing. Moved the entire bridge work (subprocess + merge + nested apply + cleanup + usage recording) inside one withBridgeTimeoutPause block. The pause helper still resumes via its finally on success and on throw, so existing failure paths are unchanged. Added a regression that captures the emitted op order and asserts merge fires after timeout-pause and before timeout-resume. Fixes #3196 --- .../src/eval/__tests__/agent-bridge.test.ts | 37 +++ .../coding-agent/src/eval/agent-bridge.ts | 227 +++++++++--------- 2 files changed, 152 insertions(+), 112 deletions(-) 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 b0cb20889..7c6b0282f 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -887,6 +887,43 @@ describe("runEvalAgent isolation", () => { 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(); diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index ecd039d6f..68d036957 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -420,129 +420,132 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption // eval session with an independent kernel. }; - // 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, 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, + // 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, + }; + }, }); - 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(", ")}.`, + })(); + + 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}`, ); } - const recoveryHint = recoveryParts.length > 0 ? ` ${recoveryParts.join(" ")}` : ""; - throw new ToolError( - `agent() isolated apply failed for ${result.id}${summaryText ? `: ${summaryText}` : ""}${recoveryHint}`, - ); - } - // Apply nested repo patches (separate from parent git). The throw - // above already exited on a failed parent merge, so we know either - // the parent succeeded (patch mode) or branch mode is in play. - const nestedPatches = result.nestedPatches ?? []; - const eligible = - nestedPatches.length > 0 && - result.exitCode === 0 && - !result.aborted && - (mergeMode !== "branch" || outcome.mergedBranchForNestedPatches); - if (eligible) { - try { - await applyNestedPatches(isolationContext.repoRoot, nestedPatches, buildCommitMessage()); - } 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). The throw + // above already exited on a failed parent merge, so we know either + // the parent succeeded (patch mode) or branch mode is in play. + const nestedPatches = result.nestedPatches ?? []; + const eligible = + nestedPatches.length > 0 && + result.exitCode === 0 && + !result.aborted && + (mergeMode !== "branch" || outcome.mergedBranchForNestedPatches); + if (eligible) { + try { + await applyNestedPatches(isolationContext.repoRoot, nestedPatches, buildCommitMessage()); + } catch { + // Nested patch failures are non-fatal to the parent merge + mergeSummary += + "\n\nSome nested repository patches failed to apply."; + } + } + } 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."; } } - } 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 }); - } + // 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); + options.session.recordEvalSubagentUsage?.(result.usage?.output ?? 0); - // 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. + return { result, mergeSummary, changesApplied }; + }); return { text: structured ? result.output : result.output + mergeSummary, From 28137f46d90365d84744abbb26ac53f7e58e4b16 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 19:22:24 +0000 Subject: [PATCH 15/17] refactor(task): deduped nested patch apply + commit-message factory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TaskTool and the eval agent() bridge each held a private copy of the nested-repo patch eligibility gate and the AI commit-message factory; isolation policy could drift between the two callers. Moved both into task/isolation-runner.ts: - applyEligibleNestedPatches(opts) — single nested-patch gate (skip on patch-mode parent failure, skip on branch-mode unmerged root, fail non-fatally with a system-notification suffix). - makeIsolationCommitMessage(session) — single factory that yields the AI commit-message callback when task.isolation.commits === "ai" and a model registry is wired, undefined otherwise. Both call sites now invoke the helpers; behavior is unchanged. Removed the now-dead generateCommitMessage/applyNestedPatches imports from each caller. Added unit tests for the new helper covering the skip-on-patch-failure, skip-on-unmerged-branch, success, and failure-suffix paths. Fixes #3196 --- .../coding-agent/src/eval/agent-bridge.ts | 44 ++++-------- packages/coding-agent/src/task/index.ts | 44 ++++-------- .../coding-agent/src/task/isolation-runner.ts | 68 ++++++++++++++++++- .../test/task/isolation-runner.test.ts | 61 ++++++++++++++++- 4 files changed, 153 insertions(+), 64 deletions(-) diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index 68d036957..e8be81670 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -15,17 +15,18 @@ 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 { applyNestedPatches, type NestedRepoPatch, parseIsolationMode } from "../task/worktree"; +import { type NestedRepoPatch, parseIsolationMode } from "../task/worktree"; import type { ToolSession } from "../tools"; import { ToolError } from "../tools/tool-errors"; -import { generateCommitMessage } from "../utils/commit-message-generator"; import { withBridgeTimeoutPause } from "./bridge-timeout"; import type { JsStatusEvent } from "./js/shared/types"; // Import review tools for side effects (registers subagent tool handlers). @@ -354,18 +355,7 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption } const preferredBackend = isIsolated ? parseIsolationMode(isolationMode) : undefined; - const commitStyle = options.session.settings.get("task.isolation.commits"); - const buildCommitMessage = () => - commitStyle === "ai" && options.session.modelRegistry - ? async (diff: string) => { - return generateCommitMessage( - diff, - options.session.modelRegistry!, - options.session.settings, - options.session.getSessionId?.() ?? undefined, - ); - } - : undefined; + const buildCommitMessage = makeIsolationCommitMessage(options.session); const baseRunOptions: ExecutorOptions = { cwd: options.session.cwd, @@ -497,24 +487,14 @@ export async function runEvalAgent(args: unknown, options: EvalAgentBridgeOption ); } - // Apply nested repo patches (separate from parent git). The throw - // above already exited on a failed parent merge, so we know either - // the parent succeeded (patch mode) or branch mode is in play. - const nestedPatches = result.nestedPatches ?? []; - const eligible = - nestedPatches.length > 0 && - result.exitCode === 0 && - !result.aborted && - (mergeMode !== "branch" || outcome.mergedBranchForNestedPatches); - if (eligible) { - try { - await applyNestedPatches(isolationContext.repoRoot, nestedPatches, buildCommitMessage()); - } catch { - // Nested patch failures are non-fatal to the parent merge - mergeSummary += - "\n\nSome nested repository patches failed to apply."; - } - } + 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) { diff --git a/packages/coding-agent/src/task/index.ts b/packages/coding-agent/src/task/index.ts index 9b4ba9d02..0f9f4e923 100644 --- a/packages/coding-agent/src/task/index.ts +++ b/packages/coding-agent/src/task/index.ts @@ -47,11 +47,12 @@ 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 { type DiscoveryResult, discoverAgents, getAgent } from "./discovery"; import { runSubprocess } from "./executor"; import { + applyEligibleNestedPatches, type IsolationContext, + makeIsolationCommitMessage, mergeIsolatedChanges, prepareIsolationContext, runIsolatedSubprocess, @@ -61,7 +62,7 @@ import { AgentOutputManager } from "./output-manager"; import { mapWithConcurrencyLimit, Semaphore } from "./parallel"; import { renderResult, renderCall as renderTaskCall } from "./render"; import { repairTaskParams } from "./repair-args"; -import { applyNestedPatches, parseIsolationMode } from "./worktree"; +import { parseIsolationMode } from "./worktree"; function renderSubagentUserPrompt(assignment: string): string { return prompt.render(subagentUserPromptTemplate, { @@ -1242,17 +1243,7 @@ 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, @@ -1361,23 +1352,16 @@ export class TaskTool implements AgentTool 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 index fd90812e5..3735db3b6 100644 --- a/packages/coding-agent/src/task/isolation-runner.ts +++ b/packages/coding-agent/src/task/isolation-runner.ts @@ -20,11 +20,14 @@ */ 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, @@ -57,7 +60,29 @@ export async function prepareIsolationContext(cwd: string): Promise undefined | ((diff: string) => Promise); +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 { /** @@ -285,3 +310,44 @@ export async function mergeIsolatedChanges(opts: IsolationMergeOptions): Promise }; } } + +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/test/task/isolation-runner.test.ts b/packages/coding-agent/test/task/isolation-runner.test.ts index 6db4bab08..d1f524e09 100644 --- a/packages/coding-agent/test/task/isolation-runner.test.ts +++ b/packages/coding-agent/test/task/isolation-runner.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "bun:test"; -import { mergeIsolatedChanges } from "@oh-my-pi/pi-coding-agent/task/isolation-runner"; +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"; @@ -59,3 +59,62 @@ describe("mergeIsolatedChanges", () => { 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"); + }); +}); From 978d2a76d0f3437e1a533e66e301c841c63520bf Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 19:34:42 +0000 Subject: [PATCH 16/17] fix(task): preserved nested-repo dirty state across nested patch apply applyNestedPatches() applied the captured patch then ran git.stage.files(nestedDir), which stages every working-tree change in the nested repo. A nested repo that was already dirty before the agent ran ended up with the user's unrelated work-in-progress committed alongside the agent delta. Stash any pre-existing dirty state (tracked + untracked) before applying the patch and pop it back in the finally block after the commit, so the agent commit contains only the captured patch and the user's in-flight work is restored on top of it. A failing stash pop logs a warning and leaves the stash entry intact for manual recovery; the broader nested-apply failure path is already non-fatal. Added a worktree integration test that confirms a pre-existing untracked file in the nested repo is not staged into the agent commit and is still present in the working tree afterwards. Fixes #3196 --- packages/coding-agent/src/task/worktree.ts | 42 +++++++++++--- .../coding-agent/test/task/worktree.test.ts | 56 +++++++++++++++++++ 2 files changed, 90 insertions(+), 8 deletions(-) diff --git a/packages/coding-agent/src/task/worktree.ts b/packages/coding-agent/src/task/worktree.ts index a10220e41..b9f828537 100644 --- a/packages/coding-agent/src/task/worktree.ts +++ b/packages/coding-agent/src/task/worktree.ts @@ -186,6 +186,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. */ @@ -212,15 +220,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); + } 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/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index 958f20555..20744d5ff 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, @@ -198,3 +199,58 @@ describe("worktree isolation helpers", () => { }); }); }); + +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"); + }); +}); From abc9a8f2925804adcf937298fa08c3cf482fa017 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 22 Jun 2026 19:41:59 +0000 Subject: [PATCH 17/17] fix(task): restored nested stash with index state git stash pop without --index restores stashed staged changes as unstaged. When a nested repo had staged WIP before the isolated agent ran, the pop in applyNestedPatches() brought the content back but lost the user's index state. Pass { index: true } so pop uses --index, matching the root merge path that already does the same thing. Added a regression test that stages a pre-existing edit in the nested repo, runs applyNestedPatches, and asserts the file is still in the index (porcelain "M " with the trailing space) and the cached diff still shows the staged WIP. Fixes #3196 --- packages/coding-agent/src/task/worktree.ts | 2 +- .../coding-agent/test/task/worktree.test.ts | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/task/worktree.ts b/packages/coding-agent/src/task/worktree.ts index b9f828537..3200ff647 100644 --- a/packages/coding-agent/src/task/worktree.ts +++ b/packages/coding-agent/src/task/worktree.ts @@ -239,7 +239,7 @@ export async function applyNestedPatches( } finally { if (stashed) { try { - await git.stash.pop(nestedDir); + await git.stash.pop(nestedDir, { index: true }); } catch (popErr) { logger.warn("Pre-existing nested-repo dirty state could not be auto-restored", { nestedDir, diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index 20744d5ff..19e91156e 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -253,4 +253,34 @@ describe("applyNestedPatches", () => { 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"); + }); });