From e01195b864aaa655041fc31ba9627741f16b012b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 09:09:45 +0000 Subject: [PATCH 01/24] =?UTF-8?q?=F0=9F=A4=96=20feat:=20allow=20agentId=20?= =?UTF-8?q?on=20workspace=20tasks=20(default=20exec)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The task tool with kind: "workspace" now accepts an optional agentId (e.g. "plan") selecting the agent mode for the launched turn. Omitted agentId preserves existing behavior: exec for new workspaces, resumed identity for mode: "existing" follow-ups to descendant agent workspaces. Because resolveAgentForStream silently falls back to exec for top-level workspaces, createWorkspaceTurn validates explicit ids fail-fast via a shared helper (syntax -> existence -> ui-selectable -> not disabled, mirroring the UI agent picker): pre-create against the owner checkout and post-create against the target checkout for mode: "new", and against the target checkout for mode: "existing". Explicit ids are per-turn overrides only; no record/metadata schema changes. subagent_type stays rejected for workspace tasks (issue path narrowed to the field itself). --- .../utils/tools/toolDefinitions.test.ts | 28 +++ src/common/utils/tools/toolDefinitions.ts | 9 +- src/node/services/taskService.test.ts | 194 ++++++++++++++++++ src/node/services/taskService.ts | 126 +++++++++++- src/node/services/tools/task.test.ts | 44 ++++ src/node/services/tools/task.ts | 2 + 6 files changed, 399 insertions(+), 4 deletions(-) diff --git a/src/common/utils/tools/toolDefinitions.test.ts b/src/common/utils/tools/toolDefinitions.test.ts index 20c5b439e79..517afea9366 100644 --- a/src/common/utils/tools/toolDefinitions.test.ts +++ b/src/common/utils/tools/toolDefinitions.test.ts @@ -94,6 +94,34 @@ describe("TOOL_DEFINITIONS", () => { } }); + it("accepts workspace task args with an agent id", () => { + const parsed = TaskToolArgsSchema.safeParse({ + kind: "workspace", + agentId: "plan", + prompt: "Plan a small change", + title: "Plan dogfood", + }); + + expect(parsed.success).toBe(true); + if (parsed.success) { + expect(parsed.data.agentId).toBe("plan"); + } + }); + + it("still rejects subagent_type for workspace tasks", () => { + const parsed = TaskToolArgsSchema.safeParse({ + kind: "workspace", + subagent_type: "plan", + prompt: "Plan a small change", + title: "Plan dogfood", + }); + + expect(parsed.success).toBe(false); + if (!parsed.success) { + expect(parsed.error.issues[0]?.path).toEqual(["subagent_type"]); + } + }); + it("rejects workspace task fanout until workspace handles support it", () => { expect( TaskToolArgsSchema.safeParse({ diff --git a/src/common/utils/tools/toolDefinitions.ts b/src/common/utils/tools/toolDefinitions.ts index d60f0e45416..984c8c8c6c0 100644 --- a/src/common/utils/tools/toolDefinitions.ts +++ b/src/common/utils/tools/toolDefinitions.ts @@ -321,6 +321,7 @@ export function buildTaskToolDescription(runtimeMode: RuntimeMode | undefined): "\n\nIMPORTANT: Whether a sub-agent can see uncommitted changes depends on the runtime. " + `${getTaskRuntimeVisibilityGuidance(runtimeMode)} ` + "\n\nProvide agentId (preferred) or subagent_type, prompt, title, run_in_background, and optional n. For sub-agents, use title as a short, friendly reusable role name (for example, Reviewer or Simplicity Auditor), not a task summary. For kind=workspace, use a normal work-specific chat title. " + + 'For kind=workspace, agentId optionally selects the agent mode for the launched turn (for example "plan"); it defaults to exec, and internal agents are not eligible. ' + "Use n only when you want several agents to try the same prompt independently. Omit it for a single task, and prefer non-interfering sub-agents for grouped runs (for example read-only agents like explore). " + `\n\nA terminal report makes the child inactive but leaves its workspace persistent. Keep each parent's direct standalone bench small and role-based: aim for at most ${SUBAGENT_REUSABLE_BENCH_TARGET} and keep it below ${SUBAGENT_REUSABLE_BENCH_EXCLUSIVE_LIMIT}; deliberate grouped n runs are temporary exceptions. Before spawning standalone work, prefer reawakening a known inactive child when its context or expertise fits, and retitle it if its reusable responsibility changes. At the target, add a role only for a genuinely distinct responsibility and prune an inactive overlapping or least-useful role before reaching the limit. Reawakening preserves the child's checkout, so for repository-dependent work, reuse it only when that snapshot is appropriate or instruct the child to verify and synchronize before acting; otherwise spawn a new child. Stop active work with task_stop; use irreversible task_remove for consumed grouped candidates, bench consolidation, explicit user requests, or clearly obsolete context—not routine end-of-turn cleanup. ` + "\n\nWhen the user explicitly asks for best-of-n work, the parent should begin with light preliminary analysis to extract shared context, constraints, or evaluation criteria that would otherwise be duplicated across children. " + @@ -379,11 +380,13 @@ function refineTaskToolAgentArgs( const hasSubagentType = typeof args.subagent_type === "string" && args.subagent_type.length > 0; if (kind === "workspace") { - if (hasAgentId || hasSubagentType) { + // Workspace tasks accept agentId (agent mode for the launched turn, e.g. "plan") but keep + // rejecting the deprecated sub-agent alias subagent_type. + if (hasSubagentType) { ctx.addIssue({ code: z.ZodIssueCode.custom, - message: "Workspace tasks do not accept agentId or subagent_type", - path: ["agentId"], + message: "Workspace tasks do not accept subagent_type", + path: ["subagent_type"], }); } if (args.n != null) { diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index c5b33115f78..14fcba8187a 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -967,6 +967,200 @@ describe("TaskService", () => { }); }); + test("createWorkspaceTurn launches a new workspace with an explicit agent id", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: createWorkspaceTurnMetadata(projectPath) })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Plan a small change", + title: "Plan dogfood", + workspace: { mode: "new" }, + }); + + expect(result.success).toBe(true); + expect(sendMessage).toHaveBeenCalledTimes(1); + const sendMessageCall = sendMessage.mock.calls[0] as unknown[]; + expect(sendMessageCall[0]).toBe("childworkspace"); + expect(sendMessageCall[2]).toMatchObject({ agentId: "plan" }); + }); + + test("createWorkspaceTurn rejects invalid, unknown, and internal agent ids before creating a workspace", async () => { + const config = await createTestConfig(rootDir); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: createWorkspaceTurnMetadata(projectPath) })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const attempt = (agentId: string) => + taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId, + prompt: "Should not run", + title: "Bad agent", + workspace: { mode: "new" }, + }); + + const invalidSyntax = await attempt("Not A Valid Id!"); + expect(invalidSyntax.success).toBe(false); + if (!invalidSyntax.success) expect(invalidSyntax.error).toContain("invalid agentId"); + + const unknown = await attempt("doesnotexist"); + expect(unknown.success).toBe(false); + if (!unknown.success) expect(unknown.error).toContain("unknown agentId"); + + // Built-in internal agents (ui.hidden) must not be launchable as workspace turns. + const internal = await attempt("compact"); + expect(internal.success).toBe(false); + if (!internal.success) expect(internal.error).toContain("not selectable"); + + expect(createWorkspace).not.toHaveBeenCalled(); + expect(sendMessage).not.toHaveBeenCalled(); + }); + + test("createWorkspaceTurn rejects disabled agents before creating a workspace", async () => { + const config = await createTestConfig(rootDir); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir, { + agentAiDefaults: { custom: { enabled: false } }, + }); + await writeCustomAgentDefinition(projectPath); + + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: createWorkspaceTurnMetadata(projectPath) })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "custom", + prompt: "Should not run", + title: "Disabled agent", + workspace: { mode: "new" }, + }); + + expect(result.success).toBe(false); + if (!result.success) expect(result.error).toContain("disabled"); + expect(createWorkspace).not.toHaveBeenCalled(); + expect(sendMessage).not.toHaveBeenCalled(); + }); + + test("createWorkspaceTurn does not dispatch when the agent is unavailable in the created workspace", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + // Project-local agent exists in the OWNER's checkout, but the created workspace's checkout + // diverges (no agent definition there) — post-create re-validation must fail instead of + // silently streaming exec. + await writeCustomAgentDefinition(projectPath); + const divergedCheckout = path.join(rootDir, "diverged-checkout"); + await fsPromises.mkdir(divergedCheckout, { recursive: true }); + + const divergedMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { + ...createWorkspaceTurnMetadata(projectPath), + namedWorkspacePath: divergedCheckout, + }; + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: divergedMetadata })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "custom", + prompt: "Should not dispatch", + title: "Diverged agent", + workspace: { mode: "new" }, + }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("no turn was dispatched"); + } + expect(createWorkspace).toHaveBeenCalledTimes(1); + expect(sendMessage).not.toHaveBeenCalled(); + }); + + test("createWorkspaceTurn explicit agentId overrides the resumed identity for that turn only", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["overridehandle", "overrideturn"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + const childWorkspaceId = "reported-child-override"; + await config.editConfig((cfg) => { + const project = cfg.projects.get(projectPath); + assert(project, "test project must exist"); + project.workspaces.push({ + path: path.join(projectPath, "reported-child"), + id: childWorkspaceId, + name: "agent_explore_reported_child", + createdAt: "2026-06-19T00:00:00.000Z", + parentWorkspaceId: parentId, + agentType: "explore", + taskStatus: "reported", + reportedAt: "2026-06-19T00:00:00.000Z", + runtimeConfig: { type: "local" }, + }); + return cfg; + }); + + const sendMessage = mock(async (...args: unknown[]): Promise> => { + const internal = args[3] as { onAccepted?: () => Promise | void } | undefined; + await internal?.onAccepted?.(); + return Ok(undefined); + }); + const { workspaceService } = createWorkspaceServiceMocks({ sendMessage }); + const { taskService } = createTaskServiceHarness(config, { workspaceService }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Re-plan the follow-up", + title: "Override turn", + allowAgentWorkspace: true, + workspace: { mode: "existing", workspaceId: childWorkspaceId }, + }); + + expect(result.success).toBe(true); + expect(sendMessage).toHaveBeenCalledWith( + childWorkspaceId, + "Re-plan the follow-up", + expect.objectContaining({ agentId: "plan" }), + expect.any(Object) + ); + // Per-turn override only: the target's persisted agent identity must be untouched. + const childEntry = findWorkspaceInConfig(config, childWorkspaceId); + expect(childEntry?.agentType).toBe("explore"); + expect(childEntry?.agentId).toBeUndefined(); + }); + test("createWorkspaceTurn inherits pro mode from the parent's active non-exec agent", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 2e462aef2f5..8981ab03ced 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -46,6 +46,7 @@ import { } from "@/node/services/agentDefinitions/agentDefinitionsService"; import { resolveAgentInheritanceChain } from "@/node/services/agentDefinitions/resolveAgentInheritanceChain"; import { isAgentEffectivelyDisabled } from "@/node/services/agentDefinitions/agentEnablement"; +import { resolveAgentVisibility } from "@/node/services/agentDefinitions/agentVisibility"; import { orchestrateFork } from "@/node/services/utils/forkOrchestrator"; import { createRuntimeContextForWorkspace, @@ -521,6 +522,13 @@ export interface WorkspaceTurnCreateArgs { ownerWorkspaceId: string; prompt: string; title: string; + /** + * Agent mode for the launched turn (e.g. "plan"). Defaults to exec for new + * workspaces and to the resumed identity for existing descendant agent + * workspaces. An explicit value is a per-turn override only — it never + * mutates the target workspace's persisted agent identity. + */ + agentId?: string; modelString?: string; thinkingLevel?: ParsedThinkingInput; parentRuntimeAiSettings?: { modelString?: string; thinkingLevel?: ThinkingLevel }; @@ -3604,6 +3612,61 @@ export class TaskService { this.scheduleMaybeStartQueuedTasks(); } + /** + * Fail-fast eligibility check for an explicit workspace-turn agentId. + * resolveAgentForStream silently falls back to exec for top-level workspaces, + * which would hide a caller's mistake — so unknown, internal (ui.hidden), and + * disabled agents are rejected here before any turn is dispatched. Mirrors + * the UI agent picker rule set, so custom user-visible agents pass without a + * hardcoded allowlist. + */ + private async validateWorkspaceTurnAgentId(params: { + cfg: ReturnType; + agentId: string; + /** Discovery context of the workspace whose turn will run (or the owner pre-create). */ + runtimeConfig: RuntimeConfig; + projectPath: string; + workspaceName: string; + persistedWorkspacePath?: string; + }): Promise> { + assert(params.agentId.length > 0, "validateWorkspaceTurnAgentId: agentId must be non-empty"); + const persistedPath = coerceNonEmptyString(params.persistedWorkspacePath); + const runtime = createRuntimeForWorkspace({ + runtimeConfig: params.runtimeConfig, + projectPath: params.projectPath, + name: params.workspaceName, + namedWorkspacePath: persistedPath, + }); + // Prefer the persisted checkout path over the name-derived one (canonical elsewhere too; see + // runtimeHelpers.resolveWorkspaceRootPath and the same pattern in Task.create). + const workspacePath = + params.projectPath === params.workspaceName + ? params.projectPath + : (persistedPath ?? runtime.getWorkspacePath(params.projectPath, params.workspaceName)); + + let frontmatter: Awaited>; + try { + frontmatter = await resolveAgentFrontmatter(runtime, workspacePath, params.agentId); + } catch { + return Err(`Task.createWorkspaceTurn: unknown agentId (${params.agentId})`); + } + if (!resolveAgentVisibility(frontmatter.ui).selectable) { + return Err( + `Task.createWorkspaceTurn: agentId is not selectable for workspace turns (${params.agentId})` + ); + } + if ( + isAgentEffectivelyDisabled({ + cfg: params.cfg, + agentId: params.agentId, + resolvedFrontmatter: frontmatter, + }) + ) { + return Err(`Task.createWorkspaceTurn: agentId is disabled (${params.agentId})`); + } + return Ok(undefined); + } + async createWorkspaceTurn( args: WorkspaceTurnCreateArgs ): Promise> { @@ -3624,6 +3687,17 @@ export class TaskService { if (queueDispatchMode !== "tool-end" && queueDispatchMode !== "turn-end") { return Err("Task.createWorkspaceTurn: unsupported queueDispatchMode"); } + // Explicit agent override: validate syntax up front; eligibility (existence, + // selectability, enablement) is checked below against the workspace whose turn will run. + let requestedAgentId: string | undefined; + const rawAgentId = coerceNonEmptyString(args.agentId); + if (rawAgentId != null) { + const parsedAgentId = AgentIdSchema.safeParse(normalizeAgentId(rawAgentId, "")); + if (!parsedAgentId.success) { + return Err(`Task.createWorkspaceTurn: invalid agentId (${rawAgentId})`); + } + requestedAgentId = parsedAgentId.data; + } await using _lock = await this.mutex.acquire(); @@ -3670,7 +3744,8 @@ export class TaskService { const turnId = this.config.generateStableId(); const createdAt = getIsoNow(); // New workspace turns use exec. Follow-ups in a persistent agent-task workspace preserve that - // child's original agent identity and task-level model settings. + // child's original agent identity and task-level model settings. An explicit args.agentId + // (validated above/below) overrides either default for this turn only. let workspaceTurnAgentId = "exec"; let targetWorkspaceId: string; let targetAiSettings: ResolvedWorkspaceAiSettings | undefined; @@ -3720,6 +3795,21 @@ export class TaskService { targetTaskThinkingLevel = targetEntry.workspace.taskThinkingLevel; targetTaskExperiments = targetEntry.workspace.taskExperiments; } + if (requestedAgentId != null) { + // Per-turn override: wins over resumed identity but never mutates the target + // workspace's persisted agent identity. Validate against the TARGET workspace's + // checkout (project-local agent definitions can diverge across branches). + const validation = await this.validateWorkspaceTurnAgentId({ + cfg, + agentId: requestedAgentId, + runtimeConfig: targetEntry?.workspace.runtimeConfig ?? parentMeta.runtimeConfig, + projectPath: targetEntry?.projectPath ?? parentMeta.projectPath, + workspaceName: targetEntry?.workspace.name ?? parentMeta.name, + persistedWorkspacePath: targetEntry?.workspace.path, + }); + if (!validation.success) return Err(validation.error); + workspaceTurnAgentId = requestedAgentId; + } // Follow-up sends continue the target workspace's own last-used settings // (persisted on every send, or manually changed by the user in that // workspace) instead of re-inheriting the owner's live settings on each @@ -3738,6 +3828,19 @@ export class TaskService { if (!slot.success) return Err(slot.error); } } else { + if (requestedAgentId != null) { + // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the + // OWNER's checkout before creating any workspace. + const validation = await this.validateWorkspaceTurnAgentId({ + cfg, + agentId: requestedAgentId, + runtimeConfig: parentMeta.runtimeConfig, + projectPath: parentMeta.projectPath, + workspaceName: parentMeta.name, + persistedWorkspacePath: parentEntry?.workspace.path, + }); + if (!validation.success) return Err(validation.error); + } const slot = await ensureParallelSlot(); if (!slot.success) return Err(slot.error); const tags = { @@ -3760,6 +3863,27 @@ export class TaskService { } targetWorkspaceId = createResult.data.metadata.id; createdWorkspace = true; + if (requestedAgentId != null) { + // Post-create stage: re-validate against the TARGET checkout — project-local agent + // definitions can diverge across branches/worktrees, so owner-path resolution is not + // an invariant. On failure, do not dispatch the turn; the created workspace is left + // behind as failed evidence (no safe pre-record cleanup path exists). + const createdMeta = createResult.data.metadata; + const validation = await this.validateWorkspaceTurnAgentId({ + cfg, + agentId: requestedAgentId, + runtimeConfig: createdMeta.runtimeConfig, + projectPath: createdMeta.projectPath, + workspaceName: createdMeta.name, + persistedWorkspacePath: createdMeta.namedWorkspacePath, + }); + if (!validation.success) { + return Err( + `${validation.error} — agent unavailable in the created workspace (${targetWorkspaceId}); no turn was dispatched` + ); + } + } + workspaceTurnAgentId = requestedAgentId ?? workspaceTurnAgentId; } // Unified per-field precedence (see resolveAgentAiSettings): explicit diff --git a/src/node/services/tools/task.test.ts b/src/node/services/tools/task.test.ts index 677b7b9269e..f32ead99e13 100644 --- a/src/node/services/tools/task.test.ts +++ b/src/node/services/tools/task.test.ts @@ -191,6 +191,50 @@ describe("task tool", () => { }); }); + it("forwards agentId to createWorkspaceTurn for workspace kind", async () => { + using tempDir = new TestTempDir("test-task-tool-workspace-turn-agent-id"); + const baseConfig = createTestToolConfig(tempDir.path, { workspaceId: "parent-workspace" }); + + const createWorkspaceTurn = mock(() => + Ok({ + taskId: "wst_child-turn", + kind: "workspace_turn" as const, + status: "running" as const, + workspaceId: "child-workspace", + }) + ); + const taskService = { createWorkspaceTurn } as unknown as TaskService; + const tool = createTaskTool({ ...baseConfig, taskService }); + + const result: unknown = await Promise.resolve( + tool.execute!( + { + kind: "workspace", + agentId: "plan", + prompt: "plan a small change", + title: "Plan dogfood", + run_in_background: true, + }, + mockToolCallOptions + ) + ); + + expect(createWorkspaceTurn).toHaveBeenCalledTimes(1); + const createWorkspaceTurnCall = createWorkspaceTurn.mock.calls[0] as unknown[]; + expect(createWorkspaceTurnCall[0]).toMatchObject({ + ownerWorkspaceId: "parent-workspace", + agentId: "plan", + prompt: "plan a small change", + workspace: { mode: "new" }, + }); + expect(result).toMatchObject({ + status: "running", + taskId: "wst_child-turn", + workspaceId: "child-workspace", + handleKind: "workspace_turn", + }); + }); + it("forwards workspace turn queue dispatch mode", async () => { using tempDir = new TestTempDir("test-task-tool-workspace-turn-queue-mode"); const baseConfig = createTestToolConfig(tempDir.path, { workspaceId: "parent-workspace" }); diff --git a/src/node/services/tools/task.ts b/src/node/services/tools/task.ts index b72023ecc0a..7219b8fc0b4 100644 --- a/src/node/services/tools/task.ts +++ b/src/node/services/tools/task.ts @@ -421,6 +421,8 @@ export const createTaskTool: ToolFactory = (config: ToolConfiguration) => { ownerWorkspaceId: workspaceId, prompt, title, + // Agent mode for the launched turn (e.g. "plan"); createWorkspaceTurn defaults to exec. + ...(agentId != null ? { agentId } : {}), experiments: config.experiments, ...(aiOverrides.modelString != null ? { modelString: aiOverrides.modelString } : {}), ...(aiOverrides.thinkingLevel != null From 931084c6c2f1912eedc885e93bc3ff4eb3693217 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 09:44:43 +0000 Subject: [PATCH 02/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20address=20Codex=20r?= =?UTF-8?q?eview=20on=20workspace-task=20agent=20overrides?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Resolve agent discovery paths via resolveWorkspaceRootPath so Docker targets use the container-side runtime path (P1) - Include Agent Plugins roots in validation when the experiment is on (P2) - Skip post-create target re-validation while the created checkout is not reachable yet (deferred-provisioning runtimes) instead of stranding the launch; owner-side validation still gates obviously-bad ids (P1) - Reject explicit agentId for descendant agent workspace targets: stream resolution pins children to persisted identity, so an override could never actually run (P1) - Dispatch existing-target overrides with skipAiSettingsPersistence so the target's saved agent/settings stay untouched; new workspaces still persist the requested agent as their default (P1) - Pass the validated checkout as definitionContext to AI-settings resolution so agent frontmatter ai defaults apply (P1) --- src/node/services/taskService.test.ts | 125 ++++++++++++++++++--- src/node/services/taskService.ts | 151 +++++++++++++++++++------- 2 files changed, 223 insertions(+), 53 deletions(-) diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index 14fcba8187a..980986687ee 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -1074,13 +1074,15 @@ describe("TaskService", () => { const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); // Project-local agent exists in the OWNER's checkout, but the created workspace's checkout // diverges (no agent definition there) — post-create re-validation must fail instead of - // silently streaming exec. + // silently streaming exec. Worktree runtime: the only local runtime whose created + // workspaces get a checkout separate from the project root. await writeCustomAgentDefinition(projectPath); const divergedCheckout = path.join(rootDir, "diverged-checkout"); await fsPromises.mkdir(divergedCheckout, { recursive: true }); const divergedMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, namedWorkspacePath: divergedCheckout, }; const createWorkspace = mock( @@ -1109,7 +1111,45 @@ describe("TaskService", () => { expect(sendMessage).not.toHaveBeenCalled(); }); - test("createWorkspaceTurn explicit agentId overrides the resumed identity for that turn only", async () => { + test("createWorkspaceTurn skips post-create re-validation while the target checkout is unreachable", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + await writeCustomAgentDefinition(projectPath); + // Deferred-provisioning runtimes return from create before the checkout is reachable. + const unreachableCheckout = path.join(rootDir, "not-provisioned-yet"); + + const deferredMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { + ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, + namedWorkspacePath: unreachableCheckout, + }; + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: deferredMetadata })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "custom", + prompt: "Launch despite pending provisioning", + title: "Deferred runtime", + workspace: { mode: "new" }, + }); + + // Owner-side validation passed; strict target re-validation must not strand the launch. + expect(result.success).toBe(true); + expect(sendMessage).toHaveBeenCalledTimes(1); + const sendMessageCall = sendMessage.mock.calls[0] as unknown[]; + expect(sendMessageCall[2]).toMatchObject({ agentId: "custom" }); + }); + + test("createWorkspaceTurn rejects explicit agentId for descendant agent workspace targets", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["overridehandle", "overrideturn"]); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); @@ -1131,14 +1171,13 @@ describe("TaskService", () => { return cfg; }); - const sendMessage = mock(async (...args: unknown[]): Promise> => { - const internal = args[3] as { onAccepted?: () => Promise | void } | undefined; - await internal?.onAccepted?.(); - return Ok(undefined); - }); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); const { workspaceService } = createWorkspaceServiceMocks({ sendMessage }); const { taskService } = createTaskServiceHarness(config, { workspaceService }); + // Persistent children are pinned to their persisted identity at stream time + // (resolveAgentForStream ignores per-send agentId when parentWorkspaceId is set), + // so an override must be rejected instead of silently running the old agent. const result = await taskService.createWorkspaceTurn({ ownerWorkspaceId: parentId, agentId: "plan", @@ -1148,19 +1187,75 @@ describe("TaskService", () => { workspace: { mode: "existing", workspaceId: childWorkspaceId }, }); - expect(result.success).toBe(true); - expect(sendMessage).toHaveBeenCalledWith( - childWorkspaceId, - "Re-plan the follow-up", - expect.objectContaining({ agentId: "plan" }), - expect.any(Object) - ); - // Per-turn override only: the target's persisted agent identity must be untouched. + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("descendant agent workspaces"); + } + expect(sendMessage).not.toHaveBeenCalled(); const childEntry = findWorkspaceInConfig(config, childWorkspaceId); expect(childEntry?.agentType).toBe("explore"); expect(childEntry?.agentId).toBeUndefined(); }); + test("createWorkspaceTurn existing-target agent override dispatches without persisting AI settings", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "firstturn", "followuphandle", "followupturn"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + + const createWorkspace = mock(async (): Promise> => { + await config.editConfig((cfg) => { + const project = cfg.projects.get(projectPath); + assert(project, "test project must exist"); + project.workspaces.push({ + // Project-dir local workspaces execute in the project root itself. + path: projectPath, + id: "childworkspace", + name: "workspace-turn", + title: "Workspace turn", + createdAt: "2026-06-19T00:00:00.000Z", + runtimeConfig: { type: "local" }, + }); + return cfg; + }); + return Ok({ metadata: createWorkspaceTurnMetadata(projectPath) }); + }); + const sendMessage = mock(async (...args: unknown[]): Promise> => { + const internal = args[3] as { onAccepted?: () => Promise | void } | undefined; + await internal?.onAccepted?.(); + return Ok(undefined); + }); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const first = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + prompt: "Initial turn", + title: "Workspace turn", + workspace: { mode: "new" }, + }); + expect(first.success).toBe(true); + + const followUp = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Per-turn plan follow-up", + title: "Override follow-up", + workspace: { mode: "existing", workspaceId: "childworkspace" }, + }); + expect(followUp.success).toBe(true); + + expect(sendMessage).toHaveBeenCalledTimes(2); + const followUpCall = sendMessage.mock.calls[1]; + expect(followUpCall[0]).toBe("childworkspace"); + // Override reaches the stream (normal workspaces honor the per-send agentId) but must + // not overwrite the target's saved agent/settings. + expect(followUpCall[2]).toMatchObject({ agentId: "plan", skipAiSettingsPersistence: true }); + // The default path keeps persisting (first send carries no override). + expect(sendMessage.mock.calls[0]?.[2]).not.toMatchObject({ skipAiSettingsPersistence: true }); + }); + test("createWorkspaceTurn inherits pro mode from the parent's active non-exec agent", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 8981ab03ced..72d9b8bbdeb 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -51,6 +51,7 @@ import { orchestrateFork } from "@/node/services/utils/forkOrchestrator"; import { createRuntimeContextForWorkspace, createRuntimeForWorkspace, + resolveWorkspaceRootPath, } from "@/node/runtime/runtimeHelpers"; import { MultiProjectRuntime } from "@/node/runtime/multiProjectRuntime"; import { runBackgroundInit } from "@/node/runtime/runtimeFactory"; @@ -525,8 +526,11 @@ export interface WorkspaceTurnCreateArgs { /** * Agent mode for the launched turn (e.g. "plan"). Defaults to exec for new * workspaces and to the resumed identity for existing descendant agent - * workspaces. An explicit value is a per-turn override only — it never - * mutates the target workspace's persisted agent identity. + * workspaces. For a new workspace the requested agent becomes its default; + * on an existing normal workspace it is a per-turn override dispatched with + * AI-settings persistence disabled, so the target's saved agent/settings are + * untouched. Rejected for descendant agent workspaces, whose persisted + * identity always wins at stream time. */ agentId?: string; modelString?: string; @@ -3612,41 +3616,55 @@ export class TaskService { this.scheduleMaybeStartQueuedTasks(); } + /** + * Agent-discovery context (runtime + checkout root) for a workspace involved in a + * workspace turn. Uses resolveWorkspaceRootPath so Docker workspaces resolve the + * container-side runtime path instead of the host-side persisted path. + */ + private buildWorkspaceTurnAgentContext(params: { + runtimeConfig: RuntimeConfig; + projectPath: string; + workspaceName: string; + persistedWorkspacePath?: string; + }): { runtime: Runtime; workspacePath: string } { + const metadataForRuntime = { + runtimeConfig: params.runtimeConfig, + projectPath: params.projectPath, + name: params.workspaceName, + namedWorkspacePath: coerceNonEmptyString(params.persistedWorkspacePath), + }; + const runtime = createRuntimeForWorkspace(metadataForRuntime); + return { runtime, workspacePath: resolveWorkspaceRootPath(metadataForRuntime, runtime) }; + } + /** * Fail-fast eligibility check for an explicit workspace-turn agentId. * resolveAgentForStream silently falls back to exec for top-level workspaces, * which would hide a caller's mistake — so unknown, internal (ui.hidden), and * disabled agents are rejected here before any turn is dispatched. Mirrors - * the UI agent picker rule set, so custom user-visible agents pass without a + * the UI agent picker rule set (including Agent Plugins roots when that + * experiment is enabled), so custom user-visible agents pass without a * hardcoded allowlist. */ private async validateWorkspaceTurnAgentId(params: { cfg: ReturnType; agentId: string; /** Discovery context of the workspace whose turn will run (or the owner pre-create). */ - runtimeConfig: RuntimeConfig; - projectPath: string; - workspaceName: string; - persistedWorkspacePath?: string; + runtime: Runtime; + workspacePath: string; }): Promise> { assert(params.agentId.length > 0, "validateWorkspaceTurnAgentId: agentId must be non-empty"); - const persistedPath = coerceNonEmptyString(params.persistedWorkspacePath); - const runtime = createRuntimeForWorkspace({ - runtimeConfig: params.runtimeConfig, - projectPath: params.projectPath, - name: params.workspaceName, - namedWorkspacePath: persistedPath, - }); - // Prefer the persisted checkout path over the name-derived one (canonical elsewhere too; see - // runtimeHelpers.resolveWorkspaceRootPath and the same pattern in Task.create). - const workspacePath = - params.projectPath === params.workspaceName - ? params.projectPath - : (persistedPath ?? runtime.getWorkspacePath(params.projectPath, params.workspaceName)); - + const includeAgentPlugins = this.workspaceService.isExperimentEnabled( + EXPERIMENT_IDS.AGENT_PLUGINS + ); let frontmatter: Awaited>; try { - frontmatter = await resolveAgentFrontmatter(runtime, workspacePath, params.agentId); + frontmatter = await resolveAgentFrontmatter( + params.runtime, + params.workspacePath, + params.agentId, + { includeAgentPlugins } + ); } catch { return Err(`Task.createWorkspaceTurn: unknown agentId (${params.agentId})`); } @@ -3747,6 +3765,10 @@ export class TaskService { // child's original agent identity and task-level model settings. An explicit args.agentId // (validated above/below) overrides either default for this turn only. let workspaceTurnAgentId = "exec"; + // Definition context of the checkout the explicit agent was validated against, so + // agent-authored frontmatter `ai` defaults participate in AI-settings resolution + // (mirrors the sub-agent creation path). Left unset for default exec/resume turns. + let agentDefinitionContext: NodeAgentDefinitionContext | undefined; let targetWorkspaceId: string; let targetAiSettings: ResolvedWorkspaceAiSettings | undefined; let targetTaskModelString: string | undefined; @@ -3796,19 +3818,44 @@ export class TaskService { targetTaskExperiments = targetEntry.workspace.taskExperiments; } if (requestedAgentId != null) { - // Per-turn override: wins over resumed identity but never mutates the target - // workspace's persisted agent identity. Validate against the TARGET workspace's - // checkout (project-local agent definitions can diverge across branches). + // Persistent agent-task children are pinned to their persisted identity: + // resolveAgentForStream ignores the per-send agentId whenever metadata has + // parentWorkspaceId, so an override could never actually run — reject instead of + // resolving settings for an agent that will not stream. + if (targetIsAgentWorkspace) { + return Err( + `Task.createWorkspaceTurn: explicit agentId is not supported for descendant agent workspaces (${targetWorkspaceId} keeps its persisted agent identity)` + ); + } + // Per-turn override for a normal owner-created workspace: wins for this turn but + // never mutates the target workspace's saved agent/settings (the dispatch below + // skips AI-settings persistence). Validate against the TARGET workspace's checkout + // (project-local agent definitions can diverge across branches). + const targetContext = this.buildWorkspaceTurnAgentContext( + targetEntry != null + ? { + runtimeConfig: targetEntry.workspace.runtimeConfig ?? parentMeta.runtimeConfig, + projectPath: targetEntry.projectPath, + // Entries created by workspaceService.create always carry a name; the fallback + // only satisfies the optional persisted-config type. + workspaceName: coerceNonEmptyString(targetEntry.workspace.name) ?? parentMeta.name, + persistedWorkspacePath: targetEntry.workspace.path, + } + : { + runtimeConfig: parentMeta.runtimeConfig, + projectPath: parentMeta.projectPath, + workspaceName: parentMeta.name, + persistedWorkspacePath: parentEntry?.workspace.path, + } + ); const validation = await this.validateWorkspaceTurnAgentId({ cfg, agentId: requestedAgentId, - runtimeConfig: targetEntry?.workspace.runtimeConfig ?? parentMeta.runtimeConfig, - projectPath: targetEntry?.projectPath ?? parentMeta.projectPath, - workspaceName: targetEntry?.workspace.name ?? parentMeta.name, - persistedWorkspacePath: targetEntry?.workspace.path, + ...targetContext, }); if (!validation.success) return Err(validation.error); workspaceTurnAgentId = requestedAgentId; + agentDefinitionContext = { ...targetContext, workspaceId: targetWorkspaceId }; } // Follow-up sends continue the target workspace's own last-used settings // (persisted on every send, or manually changed by the user in that @@ -3831,15 +3878,19 @@ export class TaskService { if (requestedAgentId != null) { // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the // OWNER's checkout before creating any workspace. - const validation = await this.validateWorkspaceTurnAgentId({ - cfg, - agentId: requestedAgentId, + const ownerContext = this.buildWorkspaceTurnAgentContext({ runtimeConfig: parentMeta.runtimeConfig, projectPath: parentMeta.projectPath, workspaceName: parentMeta.name, persistedWorkspacePath: parentEntry?.workspace.path, }); + const validation = await this.validateWorkspaceTurnAgentId({ + cfg, + agentId: requestedAgentId, + ...ownerContext, + }); if (!validation.success) return Err(validation.error); + agentDefinitionContext = { ...ownerContext, workspaceId: ownerWorkspaceId }; } const slot = await ensureParallelSlot(); if (!slot.success) return Err(slot.error); @@ -3869,17 +3920,32 @@ export class TaskService { // an invariant. On failure, do not dispatch the turn; the created workspace is left // behind as failed evidence (no safe pre-record cleanup path exists). const createdMeta = createResult.data.metadata; - const validation = await this.validateWorkspaceTurnAgentId({ - cfg, - agentId: requestedAgentId, + const targetContext = this.buildWorkspaceTurnAgentContext({ runtimeConfig: createdMeta.runtimeConfig, projectPath: createdMeta.projectPath, workspaceName: createdMeta.name, persistedWorkspacePath: createdMeta.namedWorkspacePath, }); - if (!validation.success) { - return Err( - `${validation.error} — agent unavailable in the created workspace (${targetWorkspaceId}); no turn was dispatched` + // Deferred-provisioning runtimes (Coder, devcontainer, Docker) can return from create + // while the checkout is not reachable yet; strict re-validation there would strand every + // valid launch. Owner-side validation already gated obviously-bad ids, and stream-time + // resolution handles the rest, so only re-validate when the checkout is reachable. + if (await runtimePathExists(targetContext.runtime, targetContext.workspacePath)) { + const validation = await this.validateWorkspaceTurnAgentId({ + cfg, + agentId: requestedAgentId, + ...targetContext, + }); + if (!validation.success) { + return Err( + `${validation.error} — agent unavailable in the created workspace (${targetWorkspaceId}); no turn was dispatched` + ); + } + agentDefinitionContext = { ...targetContext, workspaceId: targetWorkspaceId }; + } else { + log.debug( + "Task.createWorkspaceTurn: target checkout not reachable yet; skipping post-create agent re-validation", + { targetWorkspaceId, agentId: requestedAgentId } ); } } @@ -3923,6 +3989,9 @@ export class TaskService { } : undefined, fallbacks: this.buildParentAiSettingsFallbacks(parentMeta, workspaceTurnAgentId), + // Explicit agent overrides resolve the agent's own frontmatter `ai` defaults from the + // checkout they were validated against (mirrors resolveTaskAISettings' definitionContext). + ...(agentDefinitionContext != null ? { definitionContext: agentDefinitionContext } : {}), }); // Selected (not effective) values: sendMessage persists what it // receives, and the send path re-clamps thinking and re-gates reasoning @@ -4014,6 +4083,12 @@ export class TaskService { muxMetadata: this.buildWorkspaceTurnMuxMetadata(record), experiments: args.experiments ?? targetTaskExperiments, ...(mode === "existing" ? { queueDispatchMode } : {}), + // A per-turn agent override on an existing workspace must not overwrite the target's + // saved agent/settings (maybePersistAISettingsFromOptions persists them on every + // ordinary send). New workspaces still persist: the requested agent IS their default. + ...(mode === "existing" && requestedAgentId != null + ? { skipAiSettingsPersistence: true } + : {}), }, { startStreamInBackground: true, From ce92a4aef7d4dd3cd2954da9238d3874237db016 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 10:10:20 +0000 Subject: [PATCH 03/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20reachability-aware?= =?UTF-8?q?=20agent=20validation=20for=20workspace-turn=20targets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 Codex findings: unreachable target checkouts (deferred provisioning, stopped containers) now use a consistent policy instead of skipping (mode new) or strict-failing with a misleading error (mode existing): built-in agents validate against embedded definitions and launch safely; non-built-in agents fail with an explicit reachability error, preventing a silent exec fallback at stream time. --- src/node/services/taskService.test.ts | 109 ++++++++++++++++++++++++-- src/node/services/taskService.ts | 68 +++++++++++----- 2 files changed, 149 insertions(+), 28 deletions(-) diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index 980986687ee..1e0bd5455a7 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -1111,7 +1111,7 @@ describe("TaskService", () => { expect(sendMessage).not.toHaveBeenCalled(); }); - test("createWorkspaceTurn skips post-create re-validation while the target checkout is unreachable", async () => { + test("createWorkspaceTurn unreachable created checkout: built-ins launch, custom agents fail with a reachability error", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); @@ -1128,25 +1128,120 @@ describe("TaskService", () => { (): Promise> => Promise.resolve(Ok({ metadata: deferredMetadata })) ); - const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const sendMessage = mock( + (..._args: unknown[]): Promise> => Promise.resolve(Ok(undefined)) + ); const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); const { taskService } = createTaskServiceHarness(config, { workspaceService: workspaceMocks.workspaceService, }); - const result = await taskService.createWorkspaceTurn({ + // Custom agents cannot be verified in an unreachable checkout; dispatching anyway + // would risk a silent exec fallback at stream time, so the launch must fail loudly. + const custom = await taskService.createWorkspaceTurn({ ownerWorkspaceId: parentId, agentId: "custom", prompt: "Launch despite pending provisioning", title: "Deferred runtime", workspace: { mode: "new" }, }); + expect(custom.success).toBe(false); + if (!custom.success) { + expect(custom.error).toContain("not reachable"); + expect(custom.error).toContain("no turn was dispatched"); + } + expect(sendMessage).not.toHaveBeenCalled(); - // Owner-side validation passed; strict target re-validation must not strand the launch. - expect(result.success).toBe(true); + // Built-in agents are embedded in every checkout, so the launch is provably safe. + const builtIn = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Plan despite pending provisioning", + title: "Deferred runtime plan", + workspace: { mode: "new" }, + }); + expect(builtIn.success).toBe(true); expect(sendMessage).toHaveBeenCalledTimes(1); - const sendMessageCall = sendMessage.mock.calls[0] as unknown[]; - expect(sendMessageCall[2]).toMatchObject({ agentId: "custom" }); + const sendMessageCall = sendMessage.mock.calls[0]; + expect(sendMessageCall?.[2]).toMatchObject({ agentId: "plan" }); + }); + + test("createWorkspaceTurn unreachable existing target: built-in override works, custom fails with reachability error", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, [ + "childworkspace", + "firstturn", + "planhandle", + "planturn", + "customhandle", + ]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + await writeCustomAgentDefinition(projectPath); + // Simulates a stopped-container/deferred target: entry exists, checkout unreachable. + const unreachableCheckout = path.join(rootDir, "stopped-target"); + + const createWorkspace = mock(async (): Promise> => { + await config.editConfig((cfg) => { + const project = cfg.projects.get(projectPath); + assert(project, "test project must exist"); + project.workspaces.push({ + path: unreachableCheckout, + id: "childworkspace", + name: "workspace-turn", + title: "Workspace turn", + createdAt: "2026-06-19T00:00:00.000Z", + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, + }); + return cfg; + }); + return Ok({ + metadata: { + ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, + }, + }); + }); + const sendMessage = mock(async (...args: unknown[]): Promise> => { + const internal = args[3] as { onAccepted?: () => Promise | void } | undefined; + await internal?.onAccepted?.(); + return Ok(undefined); + }); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const first = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + prompt: "Initial turn", + title: "Workspace turn", + workspace: { mode: "new" }, + }); + expect(first.success).toBe(true); + + // Built-ins are embedded, so the override is verifiable even when the target is unreachable. + const builtIn = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Plan follow-up", + title: "Plan follow-up", + workspace: { mode: "existing", workspaceId: "childworkspace" }, + }); + expect(builtIn.success).toBe(true); + + // Custom agents get an honest reachability error, not a misleading "unknown agentId". + const custom = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "custom", + prompt: "Custom follow-up", + title: "Custom follow-up", + workspace: { mode: "existing", workspaceId: "childworkspace" }, + }); + expect(custom.success).toBe(false); + if (!custom.success) { + expect(custom.error).toContain("not reachable"); + expect(custom.error).not.toContain("unknown agentId"); + } }); test("createWorkspaceTurn rejects explicit agentId for descendant agent workspace targets", async () => { diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 72d9b8bbdeb..ffc4e0f3415 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -45,6 +45,7 @@ import { resolveAgentFrontmatter, } from "@/node/services/agentDefinitions/agentDefinitionsService"; import { resolveAgentInheritanceChain } from "@/node/services/agentDefinitions/resolveAgentInheritanceChain"; +import { getBuiltInAgentDefinitions } from "@/node/services/agentDefinitions/builtInAgentDefinitions"; import { isAgentEffectivelyDisabled } from "@/node/services/agentDefinitions/agentEnablement"; import { resolveAgentVisibility } from "@/node/services/agentDefinitions/agentVisibility"; import { orchestrateFork } from "@/node/services/utils/forkOrchestrator"; @@ -3685,6 +3686,40 @@ export class TaskService { return Ok(undefined); } + /** + * Validate an explicit agentId against the TARGET workspace's checkout, tolerating + * targets that are not reachable yet (deferred provisioning, stopped containers) + * without permitting a silent exec fallback later: + * - reachable checkout: strict validation; + * - unreachable + built-in agent: validate against the embedded definitions — + * built-ins exist in every checkout, so the launch is provably safe; + * - unreachable + custom agent: fail with a reachability error instead of a + * misleading "unknown agentId". Dispatching anyway would let + * resolveAgentForStream silently fall back to exec if the definition turns out + * to be absent after provisioning (wrong prompt/tool policy). + * Waiting for provisioning here is not an option: createWorkspaceTurn holds the + * service-wide task mutex for its whole body. + */ + private async validateWorkspaceTurnAgentIdForTarget(params: { + cfg: ReturnType; + agentId: string; + runtime: Runtime; + workspacePath: string; + }): Promise> { + const reachable = await runtimePathExists(params.runtime, params.workspacePath); + if ( + !reachable && + !getBuiltInAgentDefinitions().some((definition) => definition.id === params.agentId) + ) { + return Err( + `Task.createWorkspaceTurn: target checkout is not reachable yet (provisioning or stopped runtime), so non-built-in agentId (${params.agentId}) cannot be verified — omit agentId or retry once the workspace is ready` + ); + } + // Built-in agents resolve from embedded definitions even when the runtime is + // unreachable: per-root discovery failures are swallowed by the scanner. + return await this.validateWorkspaceTurnAgentId(params); + } + async createWorkspaceTurn( args: WorkspaceTurnCreateArgs ): Promise> { @@ -3848,7 +3883,7 @@ export class TaskService { persistedWorkspacePath: parentEntry?.workspace.path, } ); - const validation = await this.validateWorkspaceTurnAgentId({ + const validation = await this.validateWorkspaceTurnAgentIdForTarget({ cfg, agentId: requestedAgentId, ...targetContext, @@ -3926,28 +3961,19 @@ export class TaskService { workspaceName: createdMeta.name, persistedWorkspacePath: createdMeta.namedWorkspacePath, }); - // Deferred-provisioning runtimes (Coder, devcontainer, Docker) can return from create - // while the checkout is not reachable yet; strict re-validation there would strand every - // valid launch. Owner-side validation already gated obviously-bad ids, and stream-time - // resolution handles the rest, so only re-validate when the checkout is reachable. - if (await runtimePathExists(targetContext.runtime, targetContext.workspacePath)) { - const validation = await this.validateWorkspaceTurnAgentId({ - cfg, - agentId: requestedAgentId, - ...targetContext, - }); - if (!validation.success) { - return Err( - `${validation.error} — agent unavailable in the created workspace (${targetWorkspaceId}); no turn was dispatched` - ); - } - agentDefinitionContext = { ...targetContext, workspaceId: targetWorkspaceId }; - } else { - log.debug( - "Task.createWorkspaceTurn: target checkout not reachable yet; skipping post-create agent re-validation", - { targetWorkspaceId, agentId: requestedAgentId } + const validation = await this.validateWorkspaceTurnAgentIdForTarget({ + cfg, + agentId: requestedAgentId, + ...targetContext, + }); + if (!validation.success) { + // The created workspace is left behind as failed evidence (no safe + // pre-record cleanup path exists); the error names it. + return Err( + `${validation.error} — no turn was dispatched to the created workspace (${targetWorkspaceId})` ); } + agentDefinitionContext = { ...targetContext, workspaceId: targetWorkspaceId }; } workspaceTurnAgentId = requestedAgentId ?? workspaceTurnAgentId; } From ab12c42326bc282c46db5d4ddc22557e4127cc81 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 10:35:19 +0000 Subject: [PATCH 04/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20settle=20failed=20p?= =?UTF-8?q?ost-create=20agent=20validation=20through=20the=20handle=20reco?= =?UTF-8?q?rd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 Codex findings: (1) post-create validation failures now write the workspace-turn record first and settle it as an error, so the created workspace stays owner-owned and a mode="existing" retry passes the ownership check instead of hitting invalid_scope; (2) unreachable-target built-in ids are validated against the owner's checkout (same project), so a project-local shadow of a built-in id — including its ui/disabled state — is respected instead of trusting embedded membership alone. --- src/node/services/taskService.test.ts | 69 +++++++++++++++ src/node/services/taskService.ts | 120 +++++++++++++++++--------- 2 files changed, 150 insertions(+), 39 deletions(-) diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index 1e0bd5455a7..87db8596c70 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -1109,6 +1109,75 @@ describe("TaskService", () => { } expect(createWorkspace).toHaveBeenCalledTimes(1); expect(sendMessage).not.toHaveBeenCalled(); + + // The failure settles through the handle machinery, so the created workspace stays + // owner-owned: a mode="existing" retry must pass the ownership check (not invalid_scope). + const turns = await ( + taskService as unknown as { + taskHandleStore: { listAllWorkspaceTurns: () => Promise> }; + } + ).taskHandleStore.listAllWorkspaceTurns(); + expect(turns).toHaveLength(1); + expect(turns[0]).toMatchObject({ status: "error", createdWorkspace: true }); + + const retry = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + prompt: "Retry without the diverged agent", + title: "Diverged agent retry", + workspace: { mode: "existing", workspaceId: "childworkspace" }, + }); + expect(retry.success).toBe(true); + expect(sendMessage).toHaveBeenCalledTimes(1); + }); + + test("createWorkspaceTurn unreachable target respects an owner project shadow of a built-in id", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + // Shadow the built-in plan agent with a hidden project-local override in the OWNER's + // checkout: eligibility for unreachable targets must consult the shadow, not just the + // embedded definition. + const agentsDir = path.join(projectPath, ".mux", "agents"); + await fsPromises.mkdir(agentsDir, { recursive: true }); + await fsPromises.writeFile( + path.join(agentsDir, "plan.md"), + [ + "---", + "name: Plan", + "description: Hidden shadowed plan", + "base: plan", + "ui:", + " hidden: true", + "---", + "", + "Shadow body.", + "", + ].join("\n"), + "utf-8" + ); + + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: createWorkspaceTurnMetadata(projectPath) })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Should not run", + title: "Shadowed plan", + workspace: { mode: "new" }, + }); + + expect(result.success).toBe(false); + if (!result.success) expect(result.error).toContain("not selectable"); + expect(createWorkspace).not.toHaveBeenCalled(); + expect(sendMessage).not.toHaveBeenCalled(); }); test("createWorkspaceTurn unreachable created checkout: built-ins launch, custom agents fail with a reachability error", async () => { diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index ffc4e0f3415..213fb008b81 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -3690,10 +3690,12 @@ export class TaskService { * Validate an explicit agentId against the TARGET workspace's checkout, tolerating * targets that are not reachable yet (deferred provisioning, stopped containers) * without permitting a silent exec fallback later: - * - reachable checkout: strict validation; - * - unreachable + built-in agent: validate against the embedded definitions — - * built-ins exist in every checkout, so the launch is provably safe; - * - unreachable + custom agent: fail with a reachability error instead of a + * - reachable checkout: strict validation against the target; + * - unreachable + built-in agent id: validate against the OWNER's checkout — same + * project, so a project-local shadow of the built-in id (and its ui/disabled + * state) is visible there, while the embedded definition guarantees the id + * resolves in the target even if the shadow is absent; + * - unreachable + custom agent id: fail with a reachability error instead of a * misleading "unknown agentId". Dispatching anyway would let * resolveAgentForStream silently fall back to exec if the definition turns out * to be absent after provisioning (wrong prompt/tool policy). @@ -3703,21 +3705,29 @@ export class TaskService { private async validateWorkspaceTurnAgentIdForTarget(params: { cfg: ReturnType; agentId: string; - runtime: Runtime; - workspacePath: string; - }): Promise> { - const reachable = await runtimePathExists(params.runtime, params.workspacePath); - if ( - !reachable && - !getBuiltInAgentDefinitions().some((definition) => definition.id === params.agentId) - ) { + target: { runtime: Runtime; workspacePath: string }; + owner: { runtime: Runtime; workspacePath: string }; + }): Promise> { + const reachable = await runtimePathExists(params.target.runtime, params.target.workspacePath); + if (reachable) { + const validation = await this.validateWorkspaceTurnAgentId({ + cfg: params.cfg, + agentId: params.agentId, + ...params.target, + }); + return validation.success ? Ok({ validatedContext: params.target }) : validation; + } + if (!getBuiltInAgentDefinitions().some((definition) => definition.id === params.agentId)) { return Err( - `Task.createWorkspaceTurn: target checkout is not reachable yet (provisioning or stopped runtime), so non-built-in agentId (${params.agentId}) cannot be verified — omit agentId or retry once the workspace is ready` + `Task.createWorkspaceTurn: target checkout is not reachable yet (provisioning or stopped runtime), so non-built-in agentId (${params.agentId}) cannot be verified` ); } - // Built-in agents resolve from embedded definitions even when the runtime is - // unreachable: per-root discovery failures are swallowed by the scanner. - return await this.validateWorkspaceTurnAgentId(params); + const validation = await this.validateWorkspaceTurnAgentId({ + cfg: params.cfg, + agentId: params.agentId, + ...params.owner, + }); + return validation.success ? Ok({ validatedContext: params.owner }) : validation; } async createWorkspaceTurn( @@ -3804,6 +3814,11 @@ export class TaskService { // agent-authored frontmatter `ai` defaults participate in AI-settings resolution // (mirrors the sub-agent creation path). Left unset for default exec/resume turns. let agentDefinitionContext: NodeAgentDefinitionContext | undefined; + // Post-create target validation failure. Deferred (not returned immediately) so the + // workspace-turn record is still written first: the record marks the created workspace + // as owner-owned and retryable via mode="existing", and the failure settles through the + // normal handle machinery instead of stranding an unowned workspace. + let agentValidationError: string | undefined; let targetWorkspaceId: string; let targetAiSettings: ResolvedWorkspaceAiSettings | undefined; let targetTaskModelString: string | undefined; @@ -3866,31 +3881,35 @@ export class TaskService { // never mutates the target workspace's saved agent/settings (the dispatch below // skips AI-settings persistence). Validate against the TARGET workspace's checkout // (project-local agent definitions can diverge across branches). - const targetContext = this.buildWorkspaceTurnAgentContext( + const ownerContext = this.buildWorkspaceTurnAgentContext({ + runtimeConfig: parentMeta.runtimeConfig, + projectPath: parentMeta.projectPath, + workspaceName: parentMeta.name, + persistedWorkspacePath: parentEntry?.workspace.path, + }); + const targetContext = targetEntry != null - ? { + ? this.buildWorkspaceTurnAgentContext({ runtimeConfig: targetEntry.workspace.runtimeConfig ?? parentMeta.runtimeConfig, projectPath: targetEntry.projectPath, // Entries created by workspaceService.create always carry a name; the fallback // only satisfies the optional persisted-config type. workspaceName: coerceNonEmptyString(targetEntry.workspace.name) ?? parentMeta.name, persistedWorkspacePath: targetEntry.workspace.path, - } - : { - runtimeConfig: parentMeta.runtimeConfig, - projectPath: parentMeta.projectPath, - workspaceName: parentMeta.name, - persistedWorkspacePath: parentEntry?.workspace.path, - } - ); + }) + : ownerContext; const validation = await this.validateWorkspaceTurnAgentIdForTarget({ cfg, agentId: requestedAgentId, - ...targetContext, + target: targetContext, + owner: ownerContext, }); if (!validation.success) return Err(validation.error); workspaceTurnAgentId = requestedAgentId; - agentDefinitionContext = { ...targetContext, workspaceId: targetWorkspaceId }; + agentDefinitionContext = { + ...validation.data.validatedContext, + workspaceId: targetWorkspaceId, + }; } // Follow-up sends continue the target workspace's own last-used settings // (persisted on every send, or manually changed by the user in that @@ -3910,10 +3929,11 @@ export class TaskService { if (!slot.success) return Err(slot.error); } } else { + let ownerContext: { runtime: Runtime; workspacePath: string } | undefined; if (requestedAgentId != null) { // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the // OWNER's checkout before creating any workspace. - const ownerContext = this.buildWorkspaceTurnAgentContext({ + ownerContext = this.buildWorkspaceTurnAgentContext({ runtimeConfig: parentMeta.runtimeConfig, projectPath: parentMeta.projectPath, workspaceName: parentMeta.name, @@ -3949,11 +3969,14 @@ export class TaskService { } targetWorkspaceId = createResult.data.metadata.id; createdWorkspace = true; - if (requestedAgentId != null) { + if (requestedAgentId != null && ownerContext != null) { // Post-create stage: re-validate against the TARGET checkout — project-local agent // definitions can diverge across branches/worktrees, so owner-path resolution is not - // an invariant. On failure, do not dispatch the turn; the created workspace is left - // behind as failed evidence (no safe pre-record cleanup path exists). + // an invariant. On failure, do NOT return before the workspace-turn record exists: + // the record marks the created workspace as owner-owned, so a mode="existing" retry + // (once the checkout is ready or with a valid agent) passes the ownership check + // instead of hitting invalid_scope. The failure settles through the normal handle + // machinery below. const createdMeta = createResult.data.metadata; const targetContext = this.buildWorkspaceTurnAgentContext({ runtimeConfig: createdMeta.runtimeConfig, @@ -3964,16 +3987,17 @@ export class TaskService { const validation = await this.validateWorkspaceTurnAgentIdForTarget({ cfg, agentId: requestedAgentId, - ...targetContext, + target: targetContext, + owner: ownerContext, }); if (!validation.success) { - // The created workspace is left behind as failed evidence (no safe - // pre-record cleanup path exists); the error names it. - return Err( - `${validation.error} — no turn was dispatched to the created workspace (${targetWorkspaceId})` - ); + agentValidationError = `${validation.error} — no turn was dispatched; the created workspace (${targetWorkspaceId}) is owned by this caller and can be retried via workspace.mode="existing" once ready`; + } else { + agentDefinitionContext = { + ...validation.data.validatedContext, + workspaceId: targetWorkspaceId, + }; } - agentDefinitionContext = { ...targetContext, workspaceId: targetWorkspaceId }; } workspaceTurnAgentId = requestedAgentId ?? workspaceTurnAgentId; } @@ -4060,6 +4084,24 @@ export class TaskService { }); } + if (agentValidationError != null) { + // Deferred post-create validation failure: the record above keeps the created + // workspace owner-owned (retryable via mode="existing"); settle the handle as a + // normal error instead of dispatching the turn. + const next: WorkspaceTurnTaskHandleRecord = { + ...record, + status: "error", + updatedAt: getIsoNow(), + error: agentValidationError, + }; + await this.settleWorkspaceTurn({ + record, + next, + waiterSettlement: { status: "error", error: new Error(agentValidationError) }, + }); + return Err(agentValidationError); + } + const markWorkspaceTurnAccepted = async () => { await this.workspaceTurnSettlementLocks.withLock(handleId, async () => { const current = await this.taskHandleStore.getWorkspaceTurn(ownerWorkspaceId, handleId); From 3dbef2ed51ce02c9718fe71f2a85158986eda82c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 10:41:32 +0000 Subject: [PATCH 05/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20allow=20host-side?= =?UTF-8?q?=20(global/plugin)=20agents=20on=20unreachable=20target=20check?= =?UTF-8?q?outs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 P2: gate unreachable-target validation on the winning definition's scope resolved via the owner context, not built-in membership — host-side definitions (built-in, global, plugin) resolve independently of the target checkout, while project-scoped definitions keep failing closed. --- src/node/services/taskService.ts | 41 +++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 213fb008b81..005474ef0af 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -45,7 +45,6 @@ import { resolveAgentFrontmatter, } from "@/node/services/agentDefinitions/agentDefinitionsService"; import { resolveAgentInheritanceChain } from "@/node/services/agentDefinitions/resolveAgentInheritanceChain"; -import { getBuiltInAgentDefinitions } from "@/node/services/agentDefinitions/builtInAgentDefinitions"; import { isAgentEffectivelyDisabled } from "@/node/services/agentDefinitions/agentEnablement"; import { resolveAgentVisibility } from "@/node/services/agentDefinitions/agentVisibility"; import { orchestrateFork } from "@/node/services/utils/forkOrchestrator"; @@ -3691,14 +3690,14 @@ export class TaskService { * targets that are not reachable yet (deferred provisioning, stopped containers) * without permitting a silent exec fallback later: * - reachable checkout: strict validation against the target; - * - unreachable + built-in agent id: validate against the OWNER's checkout — same - * project, so a project-local shadow of the built-in id (and its ui/disabled - * state) is visible there, while the embedded definition guarantees the id - * resolves in the target even if the shadow is absent; - * - unreachable + custom agent id: fail with a reachability error instead of a - * misleading "unknown agentId". Dispatching anyway would let - * resolveAgentForStream silently fall back to exec if the definition turns out - * to be absent after provisioning (wrong prompt/tool policy). + * - unreachable checkout: resolve the definition via the OWNER's context (same + * project and runtime host, so project shadows and host-side global roots are + * visible). If the winning definition is checkout-dependent (project scope), + * fail with a reachability error — it cannot be verified in the target and + * dispatching anyway would let resolveAgentForStream silently fall back to + * exec (wrong prompt/tool policy). Host-side definitions (built-in, global, + * plugin) resolve independently of the target checkout, so eligibility is + * validated against the owner context and the launch proceeds. * Waiting for provisioning here is not an option: createWorkspaceTurn holds the * service-wide task mutex for its whole body. */ @@ -3717,9 +3716,29 @@ export class TaskService { }); return validation.success ? Ok({ validatedContext: params.target }) : validation; } - if (!getBuiltInAgentDefinitions().some((definition) => definition.id === params.agentId)) { + const parsedAgentId = AgentIdSchema.safeParse(params.agentId); + if (!parsedAgentId.success) { + return Err(`Task.createWorkspaceTurn: invalid agentId (${params.agentId})`); + } + let resolvedScope: string; + try { + const definition = await readAgentDefinition( + params.owner.runtime, + params.owner.workspacePath, + parsedAgentId.data, + { + includeAgentPlugins: this.workspaceService.isExperimentEnabled( + EXPERIMENT_IDS.AGENT_PLUGINS + ), + } + ); + resolvedScope = definition.scope; + } catch { + return Err(`Task.createWorkspaceTurn: unknown agentId (${params.agentId})`); + } + if (resolvedScope === "project") { return Err( - `Task.createWorkspaceTurn: target checkout is not reachable yet (provisioning or stopped runtime), so non-built-in agentId (${params.agentId}) cannot be verified` + `Task.createWorkspaceTurn: target checkout is not reachable yet (provisioning or stopped runtime), so project-local agentId (${params.agentId}) cannot be verified there` ); } const validation = await this.validateWorkspaceTurnAgentId({ From 08bab92eb3fd6fb453b48f123d72391af636ada0 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 10:50:19 +0000 Subject: [PATCH 06/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20divergent-branch=20?= =?UTF-8?q?and=20plugin-defaults=20handling=20for=20agent=20launches?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 Codex findings: - NodeAgentDefinitionContext carries includeAgentPlugins so AI-settings resolution reads plugin agents' frontmatter ai defaults and base chains (wired for both workspace turns and sub-agent creation) - A trunkBranch different from the owner's branch makes owner-side agent resolution advisory: target-branch-only agents launch after strict post-create target validation instead of failing "unknown agentId" - The same divergence makes unreachable targets fail closed for every id (the target branch can shadow even built-ins); the settled record keeps the workspace retryable via mode="existing" --- .../resolveNodeAgentAiSettings.ts | 6 +- src/node/services/taskService.test.ts | 92 +++++++++++++++++++ src/node/services/taskService.ts | 71 ++++++++++---- 3 files changed, 150 insertions(+), 19 deletions(-) diff --git a/src/node/services/agentDefinitions/resolveNodeAgentAiSettings.ts b/src/node/services/agentDefinitions/resolveNodeAgentAiSettings.ts index 41ec987d8c0..37dd8266705 100644 --- a/src/node/services/agentDefinitions/resolveNodeAgentAiSettings.ts +++ b/src/node/services/agentDefinitions/resolveNodeAgentAiSettings.ts @@ -32,6 +32,8 @@ export interface NodeAgentDefinitionContext { runtime: Runtime; workspacePath: string; workspaceId: string; + /** agent-plugins experiment: also resolve definitions contributed by Agent Plugins. */ + includeAgentPlugins?: boolean; } export interface ResolveNodeAgentAiSettingsParams { @@ -134,7 +136,8 @@ async function loadDefinitionLayers( const agentDefinition = await readAgentDefinition( context.runtime, context.workspacePath, - agentId + agentId, + { includeAgentPlugins: context.includeAgentPlugins } ); const chain = await resolveAgentInheritanceChain({ runtime: context.runtime, @@ -142,6 +145,7 @@ async function loadDefinitionLayers( agentId: agentDefinition.id, agentDefinition, workspaceId: context.workspaceId, + includeAgentPlugins: context.includeAgentPlugins, }); return collectDefinitionLayers(agentId, chain); diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index 87db8596c70..d52fc4d6e02 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -1180,6 +1180,98 @@ describe("TaskService", () => { expect(sendMessage).not.toHaveBeenCalled(); }); + test("createWorkspaceTurn divergent trunkBranch defers validation to the target checkout", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + // Agent exists ONLY on the target branch checkout, not in the owner's checkout: an + // owner-side miss must not fail-fast when a different base branch was requested. + const targetBranchCheckout = path.join(rootDir, "target-branch-checkout"); + const targetAgentsDir = path.join(targetBranchCheckout, ".mux", "agents"); + await fsPromises.mkdir(targetAgentsDir, { recursive: true }); + await fsPromises.writeFile( + path.join(targetAgentsDir, "custom.md"), + [ + "---", + "name: Custom", + "description: Target-branch-only agent", + "base: exec", + "---", + "", + "Body.", + "", + ].join("\n"), + "utf-8" + ); + + const targetMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { + ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, + namedWorkspacePath: targetBranchCheckout, + }; + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: targetMetadata })) + ); + const sendMessage = mock( + (..._args: unknown[]): Promise> => Promise.resolve(Ok(undefined)) + ); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "custom", + prompt: "Run the target-branch agent", + title: "Target branch agent", + workspace: { mode: "new", trunkBranch: "feature-branch" }, + }); + + expect(result.success).toBe(true); + expect(sendMessage).toHaveBeenCalledTimes(1); + expect(sendMessage.mock.calls[0]?.[2]).toMatchObject({ agentId: "custom" }); + }); + + test("createWorkspaceTurn divergent trunkBranch fails closed for unreachable targets", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + + // A different base branch can shadow ANY id (even built-ins), so an unreachable + // target created from it cannot be verified at all. + const unreachableMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { + ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, + namedWorkspacePath: path.join(rootDir, "not-provisioned-branch"), + }; + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: unreachableMetadata })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Should not dispatch", + title: "Divergent unreachable", + workspace: { mode: "new", trunkBranch: "feature-branch" }, + }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("different base branch"); + expect(result.error).toContain("no turn was dispatched"); + } + expect(sendMessage).not.toHaveBeenCalled(); + }); + test("createWorkspaceTurn unreachable created checkout: built-ins launch, custom agents fail with a reachability error", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 005474ef0af..bef2d7f1eaa 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -519,6 +519,13 @@ function isAgentRunnableAsChild( type WorkspaceTurnQueueDispatchMode = "tool-end" | "turn-end"; +/** Agent-discovery context for a workspace involved in a workspace turn. */ +interface WorkspaceTurnAgentContext { + runtime: Runtime; + workspacePath: string; + includeAgentPlugins: boolean; +} + export interface WorkspaceTurnCreateArgs { ownerWorkspaceId: string; prompt: string; @@ -3626,7 +3633,7 @@ export class TaskService { projectPath: string; workspaceName: string; persistedWorkspacePath?: string; - }): { runtime: Runtime; workspacePath: string } { + }): { runtime: Runtime; workspacePath: string; includeAgentPlugins: boolean } { const metadataForRuntime = { runtimeConfig: params.runtimeConfig, projectPath: params.projectPath, @@ -3634,7 +3641,11 @@ export class TaskService { namedWorkspacePath: coerceNonEmptyString(params.persistedWorkspacePath), }; const runtime = createRuntimeForWorkspace(metadataForRuntime); - return { runtime, workspacePath: resolveWorkspaceRootPath(metadataForRuntime, runtime) }; + return { + runtime, + workspacePath: resolveWorkspaceRootPath(metadataForRuntime, runtime), + includeAgentPlugins: this.workspaceService.isExperimentEnabled(EXPERIMENT_IDS.AGENT_PLUGINS), + }; } /** @@ -3652,18 +3663,16 @@ export class TaskService { /** Discovery context of the workspace whose turn will run (or the owner pre-create). */ runtime: Runtime; workspacePath: string; + includeAgentPlugins: boolean; }): Promise> { assert(params.agentId.length > 0, "validateWorkspaceTurnAgentId: agentId must be non-empty"); - const includeAgentPlugins = this.workspaceService.isExperimentEnabled( - EXPERIMENT_IDS.AGENT_PLUGINS - ); let frontmatter: Awaited>; try { frontmatter = await resolveAgentFrontmatter( params.runtime, params.workspacePath, params.agentId, - { includeAgentPlugins } + { includeAgentPlugins: params.includeAgentPlugins } ); } catch { return Err(`Task.createWorkspaceTurn: unknown agentId (${params.agentId})`); @@ -3704,9 +3713,15 @@ export class TaskService { private async validateWorkspaceTurnAgentIdForTarget(params: { cfg: ReturnType; agentId: string; - target: { runtime: Runtime; workspacePath: string }; - owner: { runtime: Runtime; workspacePath: string }; - }): Promise> { + target: WorkspaceTurnAgentContext; + owner: WorkspaceTurnAgentContext; + /** + * The target checkout was created from a base branch different from the owner's. + * Owner-side resolution then predicts nothing about the target (the target branch + * can shadow ANY id, including built-ins), so unreachable targets fail closed. + */ + targetBaseDivergesFromOwner?: boolean; + }): Promise> { const reachable = await runtimePathExists(params.target.runtime, params.target.workspacePath); if (reachable) { const validation = await this.validateWorkspaceTurnAgentId({ @@ -3716,6 +3731,11 @@ export class TaskService { }); return validation.success ? Ok({ validatedContext: params.target }) : validation; } + if (params.targetBaseDivergesFromOwner === true) { + return Err( + `Task.createWorkspaceTurn: target checkout is not reachable yet and was created from a different base branch, so agentId (${params.agentId}) cannot be verified there` + ); + } const parsedAgentId = AgentIdSchema.safeParse(params.agentId); if (!parsedAgentId.success) { return Err(`Task.createWorkspaceTurn: invalid agentId (${params.agentId})`); @@ -3726,11 +3746,7 @@ export class TaskService { params.owner.runtime, params.owner.workspacePath, parsedAgentId.data, - { - includeAgentPlugins: this.workspaceService.isExperimentEnabled( - EXPERIMENT_IDS.AGENT_PLUGINS - ), - } + { includeAgentPlugins: params.owner.includeAgentPlugins } ); resolvedScope = definition.scope; } catch { @@ -3948,10 +3964,18 @@ export class TaskService { if (!slot.success) return Err(slot.error); } } else { - let ownerContext: { runtime: Runtime; workspacePath: string } | undefined; + let ownerContext: WorkspaceTurnAgentContext | undefined; + // A different base branch means owner-side agent resolution predicts nothing about + // the target checkout: agents may exist only on the target branch (so an owner-side + // miss must not fail-fast) and the target branch may shadow ANY id (so unreachable + // targets fail closed in validateWorkspaceTurnAgentIdForTarget). + const targetBaseDivergesFromOwner = + coerceNonEmptyString(args.workspace?.trunkBranch) != null && + args.workspace?.trunkBranch !== parentMeta.name; if (requestedAgentId != null) { // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the - // OWNER's checkout before creating any workspace. + // OWNER's checkout before creating any workspace. Advisory when the requested base + // branch diverges — the target checkout is authoritative in that case. ownerContext = this.buildWorkspaceTurnAgentContext({ runtimeConfig: parentMeta.runtimeConfig, projectPath: parentMeta.projectPath, @@ -3963,8 +3987,15 @@ export class TaskService { agentId: requestedAgentId, ...ownerContext, }); - if (!validation.success) return Err(validation.error); - agentDefinitionContext = { ...ownerContext, workspaceId: ownerWorkspaceId }; + if (!validation.success) { + if (!targetBaseDivergesFromOwner) return Err(validation.error); + log.debug( + "Task.createWorkspaceTurn: owner-side agent validation failed; deferring to the target branch checkout", + { agentId: requestedAgentId, error: validation.error } + ); + } else { + agentDefinitionContext = { ...ownerContext, workspaceId: ownerWorkspaceId }; + } } const slot = await ensureParallelSlot(); if (!slot.success) return Err(slot.error); @@ -4008,6 +4039,7 @@ export class TaskService { agentId: requestedAgentId, target: targetContext, owner: ownerContext, + targetBaseDivergesFromOwner, }); if (!validation.success) { agentValidationError = `${validation.error} — no turn was dispatched; the created workspace (${targetWorkspaceId}) is owned by this caller and can be retried via workspace.mode="existing" once ready`; @@ -4525,6 +4557,9 @@ export class TaskService { runtime, workspacePath: parentWorkspacePath, workspaceId: parentWorkspaceId, + includeAgentPlugins: this.workspaceService.isExperimentEnabled( + EXPERIMENT_IDS.AGENT_PLUGINS + ), }, })); } catch (error) { From 2966fb383ee94cf8e423f80441b34ee5cf516cd3 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 11:00:12 +0000 Subject: [PATCH 07/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20validate=20agents?= =?UTF-8?q?=20on=20the=20stream's=20discovery=20path;=20honest=20disposabl?= =?UTF-8?q?e=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 Codex findings: agent validation contexts now use createRuntimeContextForWorkspace — the exact helper the stream uses — so subproject workspaces validate against the same discovery path that will stream (subproject-only agents pass; root/sub mismatches cannot slip through). Post-create validation failures on disposable workspaces no longer advertise a mode="existing" retry, since disposable settlement cleans the workspace up. --- src/node/services/taskService.ts | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index bef2d7f1eaa..97d0441d7d5 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -51,7 +51,6 @@ import { orchestrateFork } from "@/node/services/utils/forkOrchestrator"; import { createRuntimeContextForWorkspace, createRuntimeForWorkspace, - resolveWorkspaceRootPath, } from "@/node/runtime/runtimeHelpers"; import { MultiProjectRuntime } from "@/node/runtime/multiProjectRuntime"; import { runBackgroundInit } from "@/node/runtime/runtimeFactory"; @@ -3624,26 +3623,27 @@ export class TaskService { } /** - * Agent-discovery context (runtime + checkout root) for a workspace involved in a - * workspace turn. Uses resolveWorkspaceRootPath so Docker workspaces resolve the - * container-side runtime path instead of the host-side persisted path. + * Agent-discovery context for a workspace involved in a workspace turn. Uses + * createRuntimeContextForWorkspace — the same helper the stream uses in + * aiService — so validation resolves agents from the exact discovery path that + * will stream (Docker container-side paths, subproject directories included). */ private buildWorkspaceTurnAgentContext(params: { runtimeConfig: RuntimeConfig; projectPath: string; workspaceName: string; persistedWorkspacePath?: string; - }): { runtime: Runtime; workspacePath: string; includeAgentPlugins: boolean } { - const metadataForRuntime = { + subProjectPath?: string; + }): WorkspaceTurnAgentContext { + const context = createRuntimeContextForWorkspace({ runtimeConfig: params.runtimeConfig, projectPath: params.projectPath, name: params.workspaceName, namedWorkspacePath: coerceNonEmptyString(params.persistedWorkspacePath), - }; - const runtime = createRuntimeForWorkspace(metadataForRuntime); + subProjectPath: coerceNonEmptyString(params.subProjectPath), + }); return { - runtime, - workspacePath: resolveWorkspaceRootPath(metadataForRuntime, runtime), + ...context, includeAgentPlugins: this.workspaceService.isExperimentEnabled(EXPERIMENT_IDS.AGENT_PLUGINS), }; } @@ -3921,6 +3921,7 @@ export class TaskService { projectPath: parentMeta.projectPath, workspaceName: parentMeta.name, persistedWorkspacePath: parentEntry?.workspace.path, + subProjectPath: parentMeta.subProjectPath, }); const targetContext = targetEntry != null @@ -3931,6 +3932,7 @@ export class TaskService { // only satisfies the optional persisted-config type. workspaceName: coerceNonEmptyString(targetEntry.workspace.name) ?? parentMeta.name, persistedWorkspacePath: targetEntry.workspace.path, + subProjectPath: targetEntry.workspace.subProjectPath, }) : ownerContext; const validation = await this.validateWorkspaceTurnAgentIdForTarget({ @@ -3981,6 +3983,7 @@ export class TaskService { projectPath: parentMeta.projectPath, workspaceName: parentMeta.name, persistedWorkspacePath: parentEntry?.workspace.path, + subProjectPath: parentMeta.subProjectPath, }); const validation = await this.validateWorkspaceTurnAgentId({ cfg, @@ -4033,6 +4036,7 @@ export class TaskService { projectPath: createdMeta.projectPath, workspaceName: createdMeta.name, persistedWorkspacePath: createdMeta.namedWorkspacePath, + subProjectPath: createdMeta.subProjectPath, }); const validation = await this.validateWorkspaceTurnAgentIdForTarget({ cfg, @@ -4042,7 +4046,12 @@ export class TaskService { targetBaseDivergesFromOwner, }); if (!validation.success) { - agentValidationError = `${validation.error} — no turn was dispatched; the created workspace (${targetWorkspaceId}) is owned by this caller and can be retried via workspace.mode="existing" once ready`; + // Disposable workspaces are removed by the settlement's disposable cleanup, so + // only non-disposable workspaces are advertised as retryable. + agentValidationError = + args.workspace?.disposable === true + ? `${validation.error} — no turn was dispatched; the disposable workspace (${targetWorkspaceId}) was cleaned up` + : `${validation.error} — no turn was dispatched; the created workspace (${targetWorkspaceId}) is owned by this caller and can be retried via workspace.mode="existing" once ready`; } else { agentDefinitionContext = { ...validation.data.validatedContext, From b2b5618da6d504d778c7fedfb0861e540746e5e7 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 11:09:03 +0000 Subject: [PATCH 08/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20fail=20closed=20for?= =?UTF-8?q?=20unreachable=20existing=20targets;=20sanitize=20branch=20comp?= =?UTF-8?q?are?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6 Codex findings: existing targets have unknown checkout provenance, so explicit agent overrides fail closed while the checkout is unreachable (default identity keeps working); the trunkBranch divergence check compares via sanitizeBranchNameForWorkspace so feature/foo is not treated as divergent from its own workspace feature-foo; and synchronous post-create validation failures strip attentionPolicy from the settled record to avoid a duplicate terminal wake on top of the synchronous tool error. --- src/node/services/taskService.test.ts | 48 +++++++++++++++------------ src/node/services/taskService.ts | 42 +++++++++++++++-------- 2 files changed, 56 insertions(+), 34 deletions(-) diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index d52fc4d6e02..a737a549b9a 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -1266,7 +1266,7 @@ describe("TaskService", () => { expect(result.success).toBe(false); if (!result.success) { - expect(result.error).toContain("different base branch"); + expect(result.error).toContain("not reachable"); expect(result.error).toContain("no turn was dispatched"); } expect(sendMessage).not.toHaveBeenCalled(); @@ -1327,7 +1327,7 @@ describe("TaskService", () => { expect(sendMessageCall?.[2]).toMatchObject({ agentId: "plan" }); }); - test("createWorkspaceTurn unreachable existing target: built-in override works, custom fails with reachability error", async () => { + test("createWorkspaceTurn unreachable existing target: explicit overrides fail closed, default identity works", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, [ "childworkspace", @@ -1379,30 +1379,36 @@ describe("TaskService", () => { workspace: { mode: "new" }, }); expect(first.success).toBe(true); + expect(sendMessage).toHaveBeenCalledTimes(1); - // Built-ins are embedded, so the override is verifiable even when the target is unreachable. - const builtIn = await taskService.createWorkspaceTurn({ - ownerWorkspaceId: parentId, - agentId: "plan", - prompt: "Plan follow-up", - title: "Plan follow-up", - workspace: { mode: "existing", workspaceId: "childworkspace" }, - }); - expect(builtIn.success).toBe(true); + // Existing targets have unknown checkout provenance (any branch, uncommitted shadows), + // so ALL explicit overrides fail closed while the checkout is unreachable — even + // built-ins, whose id could be shadowed by a project definition on the target. + for (const agentId of ["plan", "custom"]) { + const overridden = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId, + prompt: `${agentId} follow-up`, + title: `${agentId} follow-up`, + workspace: { mode: "existing", workspaceId: "childworkspace" }, + }); + expect(overridden.success).toBe(false); + if (!overridden.success) { + expect(overridden.error).toContain("not reachable"); + expect(overridden.error).not.toContain("unknown agentId"); + } + } + expect(sendMessage).toHaveBeenCalledTimes(1); - // Custom agents get an honest reachability error, not a misleading "unknown agentId". - const custom = await taskService.createWorkspaceTurn({ + // Omitting agentId keeps working: the default identity needs no verification. + const withoutOverride = await taskService.createWorkspaceTurn({ ownerWorkspaceId: parentId, - agentId: "custom", - prompt: "Custom follow-up", - title: "Custom follow-up", + prompt: "Default follow-up", + title: "Default follow-up", workspace: { mode: "existing", workspaceId: "childworkspace" }, }); - expect(custom.success).toBe(false); - if (!custom.success) { - expect(custom.error).toContain("not reachable"); - expect(custom.error).not.toContain("unknown agentId"); - } + expect(withoutOverride.success).toBe(true); + expect(sendMessage).toHaveBeenCalledTimes(2); }); test("createWorkspaceTurn rejects explicit agentId for descendant agent workspace targets", async () => { diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 97d0441d7d5..c8586b8d912 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -61,7 +61,10 @@ import { tryReadGitHeadCommitSha, findWorkspaceEntry, } from "@/node/services/taskUtils"; -import { validateWorkspaceName } from "@/common/utils/validation/workspaceValidation"; +import { + sanitizeBranchNameForWorkspace, + validateWorkspaceName, +} from "@/common/utils/validation/workspaceValidation"; import { getTaskGroupCount } from "@/common/utils/tools/taskGroups"; import { stripTrailingSlashes } from "@/node/utils/pathUtils"; import { Ok, Err, type Result } from "@/common/types/result"; @@ -3716,11 +3719,14 @@ export class TaskService { target: WorkspaceTurnAgentContext; owner: WorkspaceTurnAgentContext; /** - * The target checkout was created from a base branch different from the owner's. - * Owner-side resolution then predicts nothing about the target (the target branch - * can shadow ANY id, including built-ins), so unreachable targets fail closed. + * Whether the owner's checkout is a sound predictor of the target's agent + * definitions. Only true for workspaces this call just created from the owner's + * own branch. False for existing targets (their checkout has unknown provenance — + * any branch, uncommitted shadows) and for new workspaces created from a different + * base branch (which can shadow ANY id, including built-ins). When false, + * unreachable targets fail closed instead of trusting owner-side resolution. */ - targetBaseDivergesFromOwner?: boolean; + ownerResolutionPredictsTarget: boolean; }): Promise> { const reachable = await runtimePathExists(params.target.runtime, params.target.workspacePath); if (reachable) { @@ -3731,9 +3737,9 @@ export class TaskService { }); return validation.success ? Ok({ validatedContext: params.target }) : validation; } - if (params.targetBaseDivergesFromOwner === true) { + if (!params.ownerResolutionPredictsTarget) { return Err( - `Task.createWorkspaceTurn: target checkout is not reachable yet and was created from a different base branch, so agentId (${params.agentId}) cannot be verified there` + `Task.createWorkspaceTurn: target checkout is not reachable (provisioning, stopped runtime, or unknown checkout state), so agentId (${params.agentId}) cannot be verified there` ); } const parsedAgentId = AgentIdSchema.safeParse(params.agentId); @@ -3940,6 +3946,10 @@ export class TaskService { agentId: requestedAgentId, target: targetContext, owner: ownerContext, + // Existing targets have unknown checkout provenance (any branch, uncommitted + // shadows), so an unreachable checkout fails closed rather than trusting the + // owner's resolution. Omitting agentId still works (default identity). + ownerResolutionPredictsTarget: false, }); if (!validation.success) return Err(validation.error); workspaceTurnAgentId = requestedAgentId; @@ -3970,10 +3980,13 @@ export class TaskService { // A different base branch means owner-side agent resolution predicts nothing about // the target checkout: agents may exist only on the target branch (so an owner-side // miss must not fail-fast) and the target branch may shadow ANY id (so unreachable - // targets fail closed in validateWorkspaceTurnAgentIdForTarget). + // targets fail closed in validateWorkspaceTurnAgentIdForTarget). Compare via the + // branch→workspace-name sanitization: parentMeta.name is a workspace name, so a raw + // branch like feature/foo must not look divergent from its own workspace feature-foo. + const requestedTrunkBranch = coerceNonEmptyString(args.workspace?.trunkBranch); const targetBaseDivergesFromOwner = - coerceNonEmptyString(args.workspace?.trunkBranch) != null && - args.workspace?.trunkBranch !== parentMeta.name; + requestedTrunkBranch != null && + sanitizeBranchNameForWorkspace(requestedTrunkBranch) !== parentMeta.name; if (requestedAgentId != null) { // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the // OWNER's checkout before creating any workspace. Advisory when the requested base @@ -4043,7 +4056,7 @@ export class TaskService { agentId: requestedAgentId, target: targetContext, owner: ownerContext, - targetBaseDivergesFromOwner, + ownerResolutionPredictsTarget: !targetBaseDivergesFromOwner, }); if (!validation.success) { // Disposable workspaces are removed by the settlement's disposable cleanup, so @@ -4147,9 +4160,12 @@ export class TaskService { if (agentValidationError != null) { // Deferred post-create validation failure: the record above keeps the created // workspace owner-owned (retryable via mode="existing"); settle the handle as a - // normal error instead of dispatching the turn. + // normal error instead of dispatching the turn. The error is returned synchronously + // to the caller below, so strip attentionPolicy from the settled record — keeping + // notify_on_terminal would enqueue a second, duplicate terminal wake for the owner. + const { attentionPolicy: _droppedAttentionPolicy, ...recordWithoutAttention } = record; const next: WorkspaceTurnTaskHandleRecord = { - ...record, + ...recordWithoutAttention, status: "error", updatedAt: getIsoNow(), error: agentValidationError, From b267445ab7700588a4584e572271ab7abceb7933 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 11:45:03 +0000 Subject: [PATCH 09/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20verify=20owner=20br?= =?UTF-8?q?anch=20for=20base=20vouching;=20cross-host=20and=20init-race=20?= =?UTF-8?q?fail-closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node/services/taskService.test.ts | 167 +++++++++++++++++++++++++- src/node/services/taskService.ts | 159 +++++++++++++++++------- src/node/services/taskUtils.ts | 32 +++++ 3 files changed, 314 insertions(+), 44 deletions(-) diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index a737a549b9a..13008073056 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -279,6 +279,7 @@ async function saveLocalParentWorkspace( agentAiDefaults?: AgentAiDefaults; subagentAiDefaults?: Record; parentAiSettings?: { model: string; thinkingLevel: ThinkingLevel }; + workspaceName?: string; } ): Promise<{ parentId: string; projectPath: string }> { const projectPath = await createTestProject(rootDir, "repo", { initGit: false }); @@ -290,7 +291,7 @@ async function saveLocalParentWorkspace( { path: projectPath, id: parentId, - name: "parent", + name: options?.workspaceName ?? "parent", createdAt: new Date().toISOString(), runtimeConfig: { type: "local" }, aiSettings: options?.parentAiSettings ?? { @@ -1101,6 +1102,10 @@ describe("TaskService", () => { prompt: "Should not dispatch", title: "Diverged agent", workspace: { mode: "new" }, + // Background launch: on this synchronous failure the policy must NOT be persisted — + // settleWorkspaceTurn derives the terminal wake from the persisted record, which + // would duplicate the Err returned directly to the caller. + attentionPolicy: "notify_on_terminal", }); expect(result.success).toBe(false); @@ -1114,11 +1119,14 @@ describe("TaskService", () => { // owner-owned: a mode="existing" retry must pass the ownership check (not invalid_scope). const turns = await ( taskService as unknown as { - taskHandleStore: { listAllWorkspaceTurns: () => Promise> }; + taskHandleStore: { + listAllWorkspaceTurns: () => Promise>; + }; } ).taskHandleStore.listAllWorkspaceTurns(); expect(turns).toHaveLength(1); expect(turns[0]).toMatchObject({ status: "error", createdWorkspace: true }); + expect(turns[0]?.attentionPolicy).toBeUndefined(); const retry = await taskService.createWorkspaceTurn({ ownerWorkspaceId: parentId, @@ -1276,6 +1284,10 @@ describe("TaskService", () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + // Owner vouching for an unreachable target requires git proof that the owner is + // actually checked out on the base branch the child is created from. + initGitRepo(projectPath); + execSync("git checkout -b parent", { cwd: projectPath, stdio: "ignore" }); await writeCustomAgentDefinition(projectPath); // Deferred-provisioning runtimes return from create before the checkout is reachable. const unreachableCheckout = path.join(rootDir, "not-provisioned-yet"); @@ -1327,6 +1339,157 @@ describe("TaskService", () => { expect(sendMessageCall?.[2]).toMatchObject({ agentId: "plan" }); }); + test("createWorkspaceTurn treats sanitized branch-name collisions as unproven bases", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + // Owner is checked out on feature/foo, whose workspace name sanitizes to feature-foo. + // A request for the DISTINCT branch feature-foo collides with that name, so the owner + // must not vouch for the unreachable target: even a built-in id fails closed (the + // colliding branch could shadow it). + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir, { + workspaceName: "feature-foo", + }); + initGitRepo(projectPath); + execSync("git checkout -b feature/foo", { cwd: projectPath, stdio: "ignore" }); + + const unreachableMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { + ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, + namedWorkspacePath: path.join(rootDir, "not-provisioned-collision"), + }; + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: unreachableMetadata })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Should not dispatch", + title: "Colliding branch", + workspace: { mode: "new", trunkBranch: "feature-foo" }, + }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("not reachable"); + expect(result.error).toContain("no turn was dispatched"); + } + expect(sendMessage).not.toHaveBeenCalled(); + + // The owner's real branch, by contrast, is a proven base: the same launch succeeds. + const sameBranch = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Plan from the owner's own branch", + title: "Same branch", + workspace: { mode: "new", trunkBranch: "feature/foo" }, + }); + expect(sameBranch.success).toBe(true); + expect(sendMessage).toHaveBeenCalledTimes(1); + }); + + test("createWorkspaceTurn unreachable cross-host target fails closed even for built-ins", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + initGitRepo(projectPath); + execSync("git checkout -b parent", { cwd: projectPath, stdio: "ignore" }); + + // The created target lives on a remote host (per-workspace containers, Coder-style + // per-workspace hosts). The owner's global agent roots say nothing about that host — + // even a built-in could be shadowed by a target-host global definition — so + // owner-side resolution must not vouch while the checkout is unreachable. + const remoteMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { + ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "docker", image: "node:20" }, + namedWorkspacePath: "/workspace/repo", + }; + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: remoteMetadata })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Should not dispatch", + title: "Cross-host unreachable", + workspace: { mode: "new" }, + }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("different host"); + expect(result.error).toContain("no turn was dispatched"); + } + expect(sendMessage).not.toHaveBeenCalled(); + }); + + test("createWorkspaceTurn does not verify agents while the created workspace is still initializing", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + await writeCustomAgentDefinition(projectPath); + // Reachable checkout whose init hook is still running: the hook may still be + // installing/rewriting agent definitions, so a strict-validation miss is not a + // trustworthy "unknown agentId" verdict — the launch must fail with a transient + // error instead of a definitive one (and never dispatch an unverified id). + const initializingCheckout = path.join(rootDir, "initializing-checkout"); + await fsPromises.mkdir(initializingCheckout, { recursive: true }); + + const initializingMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { + ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, + namedWorkspacePath: initializingCheckout, + }; + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: initializingMetadata })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const initStateManager = { + startInit: mock(() => undefined), + enterHookPhase: mock(() => undefined), + appendOutput: mock(() => undefined), + endInit: mock(() => Promise.resolve()), + getInitState: mock((workspaceId: string) => + workspaceId === "childworkspace" ? { status: "running" } : undefined + ), + readInitStatus: mock(() => Promise.resolve(null)), + } as unknown as InitStateManager; + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + initStateManager, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "custom", + prompt: "Should not dispatch", + title: "Initializing target", + workspace: { mode: "new" }, + }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("still initializing"); + expect(result.error).toContain("no turn was dispatched"); + } + expect(sendMessage).not.toHaveBeenCalled(); + }); + test("createWorkspaceTurn unreachable existing target: explicit overrides fail closed, default identity works", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, [ diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index c8586b8d912..63b4e9e6a36 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -58,13 +58,11 @@ import type { InitLogger, Runtime } from "@/node/runtime/Runtime"; import { readPlanFile } from "@/node/utils/runtime/helpers"; import { coerceNonEmptyString, + tryReadGitCurrentBranch, tryReadGitHeadCommitSha, findWorkspaceEntry, } from "@/node/services/taskUtils"; -import { - sanitizeBranchNameForWorkspace, - validateWorkspaceName, -} from "@/common/utils/validation/workspaceValidation"; +import { validateWorkspaceName } from "@/common/utils/validation/workspaceValidation"; import { getTaskGroupCount } from "@/common/utils/tools/taskGroups"; import { stripTrailingSlashes } from "@/node/utils/pathUtils"; import { Ok, Err, type Result } from "@/common/types/result"; @@ -85,7 +83,11 @@ import { defaultModel, normalizeSelectedModel } from "@/common/utils/ai/models"; import { EXPERIMENT_IDS } from "@/common/constants/experiments"; import { SCRATCH_PROJECT_CONFIG_KEY, SCRATCH_PROJECT_NAME } from "@/common/constants/scratch"; import { DEFAULT_RUNTIME_CONFIG } from "@/common/constants/workspace"; -import { runtimeModeSupportsSharedTaskWorkspace, type RuntimeConfig } from "@/common/types/runtime"; +import { + RUNTIME_MODE, + runtimeModeSupportsSharedTaskWorkspace, + type RuntimeConfig, +} from "@/common/types/runtime"; import type { ProjectRef, WorkspaceMetadata } from "@/common/types/workspace"; import { getRuntimeType } from "@/node/runtime/initHook"; import { AgentIdSchema } from "@/common/orpc/schemas"; @@ -526,6 +528,31 @@ interface WorkspaceTurnAgentContext { runtime: Runtime; workspacePath: string; includeAgentPlugins: boolean; + /** Source config, kept so owner/target contexts can be compared for host identity. */ + runtimeConfig: RuntimeConfig; +} + +/** + * Whether agent discovery for both runtime configs reads the same host filesystem, + * i.e. the owner's global/plugin agent roots are literally the target's roots. + * Local-family runtimes (local/worktree) share the local machine. Plain SSH shares + * the remote home only for an identical host/port with no Coder indirection — + * CoderSSHRuntime.finalizeConfig derives a distinct per-workspace host, so a Coder + * owner's ~/.xum/agents says nothing about the child's. Docker/devcontainer get + * per-workspace containers and never share. + */ +function runtimeConfigsShareAgentHost(a: RuntimeConfig, b: RuntimeConfig): boolean { + const isLocalFamily = (rc: RuntimeConfig): boolean => + rc.type === RUNTIME_MODE.LOCAL || rc.type === RUNTIME_MODE.WORKTREE; + if (isLocalFamily(a) && isLocalFamily(b)) { + return true; + } + if (a.type === RUNTIME_MODE.SSH && b.type === RUNTIME_MODE.SSH) { + return ( + a.coder == null && b.coder == null && a.host === b.host && (a.port ?? 22) === (b.port ?? 22) + ); + } + return false; } export interface WorkspaceTurnCreateArgs { @@ -3648,6 +3675,7 @@ export class TaskService { return { ...context, includeAgentPlugins: this.workspaceService.isExperimentEnabled(EXPERIMENT_IDS.AGENT_PLUGINS), + runtimeConfig: params.runtimeConfig, }; } @@ -3701,30 +3729,41 @@ export class TaskService { * Validate an explicit agentId against the TARGET workspace's checkout, tolerating * targets that are not reachable yet (deferred provisioning, stopped containers) * without permitting a silent exec fallback later: - * - reachable checkout: strict validation against the target; - * - unreachable checkout: resolve the definition via the OWNER's context (same - * project and runtime host, so project shadows and host-side global roots are - * visible). If the winning definition is checkout-dependent (project scope), - * fail with a reachability error — it cannot be verified in the target and - * dispatching anyway would let resolveAgentForStream silently fall back to - * exec (wrong prompt/tool policy). Host-side definitions (built-in, global, - * plugin) resolve independently of the target checkout, so eligibility is - * validated against the owner context and the launch proceeds. - * Waiting for provisioning here is not an option: createWorkspaceTurn holds the - * service-wide task mutex for its whole body. + * - reachable checkout: strict validation against the target. If that fails while + * the target's init hook is still running, the verdict is not trustworthy (the + * hook may still be installing/rewriting agent definitions), so the launch fails + * with an explicitly transient error instead of a definitive "unknown agentId"; + * - unreachable checkout: resolve the definition via the OWNER's context, but only + * when the owner and target run agent discovery on the same host filesystem + * (runtimeConfigsShareAgentHost) — cross-host (Coder per-workspace hosts, per- + * workspace containers), the owner's global roots say nothing about the target's, + * and even a built-in could be shadowed by a target-host global definition, so + * everything fails closed. Same-host, project shadows and global roots are + * visible through the owner: a checkout-dependent (project-scope) winner still + * fails with a reachability error — it cannot be verified in the target and + * dispatching anyway would let resolveAgentForStream silently fall back to exec + * (wrong prompt/tool policy) — while host-side definitions (built-in, global, + * plugin) are validated against the owner context and the launch proceeds. + * Waiting for provisioning/init here is not an option: createWorkspaceTurn holds + * the service-wide task mutex for its whole body. Failures in these windows settle + * as retryable errors (mode="existing" once the target is ready) rather than + * dispatching an unverified id. */ private async validateWorkspaceTurnAgentIdForTarget(params: { cfg: ReturnType; agentId: string; target: WorkspaceTurnAgentContext; owner: WorkspaceTurnAgentContext; + /** Whether the target workspace's init hook is still running (see doc above). */ + targetInitPending: boolean; /** * Whether the owner's checkout is a sound predictor of the target's agent - * definitions. Only true for workspaces this call just created from the owner's - * own branch. False for existing targets (their checkout has unknown provenance — - * any branch, uncommitted shadows) and for new workspaces created from a different - * base branch (which can shadow ANY id, including built-ins). When false, - * unreachable targets fail closed instead of trusting owner-side resolution. + * definitions. Only true for workspaces this call just created from the branch + * verified (via git) to be checked out in the owner right now. False for existing + * targets (their checkout has unknown provenance — any branch, uncommitted shadows) + * and for new workspaces whose base branch differs from or cannot be proven equal + * to the owner's. When false, unreachable targets fail closed instead of trusting + * owner-side resolution. */ ownerResolutionPredictsTarget: boolean; }): Promise> { @@ -3735,13 +3774,26 @@ export class TaskService { agentId: params.agentId, ...params.target, }); - return validation.success ? Ok({ validatedContext: params.target }) : validation; + if (validation.success) { + return Ok({ validatedContext: params.target }); + } + if (params.targetInitPending) { + return Err( + `Task.createWorkspaceTurn: the target workspace is still initializing, so agentId (${params.agentId}) could not be verified yet (${validation.error})` + ); + } + return validation; } if (!params.ownerResolutionPredictsTarget) { return Err( `Task.createWorkspaceTurn: target checkout is not reachable (provisioning, stopped runtime, or unknown checkout state), so agentId (${params.agentId}) cannot be verified there` ); } + if (!runtimeConfigsShareAgentHost(params.owner.runtimeConfig, params.target.runtimeConfig)) { + return Err( + `Task.createWorkspaceTurn: target checkout is not reachable and runs on a different host than the owner, so agentId (${params.agentId}) cannot be verified there` + ); + } const parsedAgentId = AgentIdSchema.safeParse(params.agentId); if (!parsedAgentId.success) { return Err(`Task.createWorkspaceTurn: invalid agentId (${params.agentId})`); @@ -3946,6 +3998,8 @@ export class TaskService { agentId: requestedAgentId, target: targetContext, owner: ownerContext, + targetInitPending: + this.initStateManager.getInitState(existingWorkspaceId)?.status === "running", // Existing targets have unknown checkout provenance (any branch, uncommitted // shadows), so an unreachable checkout fails closed rather than trusting the // owner's resolution. Omitting agentId still works (default identity). @@ -3977,20 +4031,19 @@ export class TaskService { } } else { let ownerContext: WorkspaceTurnAgentContext | undefined; - // A different base branch means owner-side agent resolution predicts nothing about - // the target checkout: agents may exist only on the target branch (so an owner-side - // miss must not fail-fast) and the target branch may shadow ANY id (so unreachable - // targets fail closed in validateWorkspaceTurnAgentIdForTarget). Compare via the - // branch→workspace-name sanitization: parentMeta.name is a workspace name, so a raw - // branch like feature/foo must not look divergent from its own workspace feature-foo. + // Owner-side vouching for the created checkout's agent definitions requires proof + // that the target's base IS the branch actually checked out in the owner. The + // workspace name cannot prove it: branch→name sanitization is not injective + // (feature/foo and feature-foo both map to feature-foo) in BOTH directions — a + // request naming the owner's workspace name may be a distinct colliding branch, and + // the omitted-arg default (parentMeta.name, passed as trunkBranch to create below) + // may itself differ from a slash-branch owner's real branch. A different/unproven + // base means agents may exist only on the target branch (owner-side misses must not + // fail-fast) and the target branch may shadow ANY id (unreachable targets fail + // closed in validateWorkspaceTurnAgentIdForTarget). const requestedTrunkBranch = coerceNonEmptyString(args.workspace?.trunkBranch); - const targetBaseDivergesFromOwner = - requestedTrunkBranch != null && - sanitizeBranchNameForWorkspace(requestedTrunkBranch) !== parentMeta.name; + let ownerVouchesForTargetBase = false; if (requestedAgentId != null) { - // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the - // OWNER's checkout before creating any workspace. Advisory when the requested base - // branch diverges — the target checkout is authoritative in that case. ownerContext = this.buildWorkspaceTurnAgentContext({ runtimeConfig: parentMeta.runtimeConfig, projectPath: parentMeta.projectPath, @@ -3998,13 +4051,26 @@ export class TaskService { persistedWorkspacePath: parentEntry?.workspace.path, subProjectPath: parentMeta.subProjectPath, }); + const effectiveTrunkBranch = requestedTrunkBranch ?? parentMeta.name; + const ownerBranch = await tryReadGitCurrentBranch( + ownerContext.runtime, + ownerContext.workspacePath + ); + ownerVouchesForTargetBase = ownerBranch != null && ownerBranch === effectiveTrunkBranch; + // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the + // OWNER's checkout before creating any workspace. Strict for an omitted + // trunkBranch (the child is created from the owner's own workspace line) or a + // verified same-branch request; advisory when the requested base may diverge — + // the target checkout is authoritative in that case. const validation = await this.validateWorkspaceTurnAgentId({ cfg, agentId: requestedAgentId, ...ownerContext, }); if (!validation.success) { - if (!targetBaseDivergesFromOwner) return Err(validation.error); + if (requestedTrunkBranch == null || ownerVouchesForTargetBase) { + return Err(validation.error); + } log.debug( "Task.createWorkspaceTurn: owner-side agent validation failed; deferring to the target branch checkout", { agentId: requestedAgentId, error: validation.error } @@ -4056,7 +4122,11 @@ export class TaskService { agentId: requestedAgentId, target: targetContext, owner: ownerContext, - ownerResolutionPredictsTarget: !targetBaseDivergesFromOwner, + // create() starts runBackgroundInit asynchronously; a reachable checkout whose + // init hook is still running may not have its final agent definitions yet. + targetInitPending: + this.initStateManager.getInitState(targetWorkspaceId)?.status === "running", + ownerResolutionPredictsTarget: ownerVouchesForTargetBase, }); if (!validation.success) { // Disposable workspaces are removed by the settlement's disposable cleanup, so @@ -4144,7 +4214,13 @@ export class TaskService { prompt, modelString: model, ...(thinkingLevel != null ? { thinkingLevel } : {}), - ...(args.attentionPolicy != null ? { attentionPolicy: args.attentionPolicy } : {}), + // Synchronous validation failure below returns the error directly to the caller, so + // never persist notify_on_terminal for it: settleWorkspaceTurn derives the terminal + // wake from the PERSISTED record's attentionPolicy, which would enqueue a duplicate + // notification on top of the synchronous Err. + ...(args.attentionPolicy != null && agentValidationError == null + ? { attentionPolicy: args.attentionPolicy } + : {}), }; await this.taskHandleStore.upsertWorkspaceTurn(record); if (targetIsAgentWorkspace) { @@ -4160,12 +4236,11 @@ export class TaskService { if (agentValidationError != null) { // Deferred post-create validation failure: the record above keeps the created // workspace owner-owned (retryable via mode="existing"); settle the handle as a - // normal error instead of dispatching the turn. The error is returned synchronously - // to the caller below, so strip attentionPolicy from the settled record — keeping - // notify_on_terminal would enqueue a second, duplicate terminal wake for the owner. - const { attentionPolicy: _droppedAttentionPolicy, ...recordWithoutAttention } = record; + // normal error instead of dispatching the turn. The record was persisted without + // attentionPolicy (see above), so settlement cannot enqueue a terminal wake on + // top of the synchronous Err returned below. const next: WorkspaceTurnTaskHandleRecord = { - ...recordWithoutAttention, + ...record, status: "error", updatedAt: getIsoNow(), error: agentValidationError, diff --git a/src/node/services/taskUtils.ts b/src/node/services/taskUtils.ts index 76b7339abb8..aa10a70f87b 100644 --- a/src/node/services/taskUtils.ts +++ b/src/node/services/taskUtils.ts @@ -36,6 +36,38 @@ export async function tryReadGitHeadCommitSha( } } +/** + * Branch currently checked out in the workspace, or undefined when it cannot be + * determined (no git, detached HEAD, unreachable runtime). Callers use this as + * proof of a checkout's base branch, so "unknown" must stay distinguishable + * from any real branch name. + */ +export async function tryReadGitCurrentBranch( + runtime: Runtime, + workspacePath: string +): Promise { + assert(workspacePath.length > 0, "tryReadGitCurrentBranch: workspacePath must be non-empty"); + + try { + const result = await execBuffered(runtime, "git rev-parse --abbrev-ref HEAD", { + cwd: workspacePath, + timeout: 10, + }); + if (result.exitCode !== 0) { + return undefined; + } + + const branch = result.stdout.trim(); + // Detached HEAD reports the literal string "HEAD" — not a branch identity. + if (branch.length === 0 || branch === "HEAD") { + return undefined; + } + return branch; + } catch { + return undefined; + } +} + /** * Resolve the effective refusal-fallback chain for a workspace's turn. * Task children can opt out via taskOnRefusal: "fail" (e.g. workflow verifier From 4566a2c8985e2d882b44a374d4e9f4cba322a5a3 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 12:13:48 +0000 Subject: [PATCH 10/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20strict=20stream-tim?= =?UTF-8?q?e=20agent=20resolution=20backstop;=20vouched-only=20fatal=20pre?= =?UTF-8?q?checks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/common/orpc/schemas/stream.ts | 9 ++ src/node/services/agentResolution.test.ts | 110 ++++++++++++++++++++++ src/node/services/agentResolution.ts | 23 ++++- src/node/services/agentSession.ts | 1 + src/node/services/aiService.ts | 4 + src/node/services/taskService.test.ts | 78 +++++++++++++-- src/node/services/taskService.ts | 15 ++- 7 files changed, 225 insertions(+), 15 deletions(-) diff --git a/src/common/orpc/schemas/stream.ts b/src/common/orpc/schemas/stream.ts index d1c6cbcd590..a5d84a2e4e1 100644 --- a/src/common/orpc/schemas/stream.ts +++ b/src/common/orpc/schemas/stream.ts @@ -800,6 +800,15 @@ export const SendMessageOptionsSchema = z.object({ * iterating on agent files - a broken agent in the worktree won't affect message sending. */ disableWorkspaceAgents: z.boolean().optional(), + /** + * When true, a top-level send whose agentId cannot be resolved (or is disabled) + * at stream time fails loudly instead of silently falling back to exec. + * Workspace-turn launches with explicit agent overrides set this: pre-dispatch + * validation races init hooks and user edits, so stream-time resolution — which + * runs after initialization completes — is the last sound gate against running + * a different agent than the caller asked for. + */ + strictAgentResolution: z.boolean().optional(), /** * Desktop/app-only capability: expose set_goal so an agent can create a * continuation-backed goal for its current parent workspace. Headless callers diff --git a/src/node/services/agentResolution.test.ts b/src/node/services/agentResolution.test.ts index 0df455159a5..3ce9c3741ca 100644 --- a/src/node/services/agentResolution.test.ts +++ b/src/node/services/agentResolution.test.ts @@ -472,6 +472,116 @@ describe("resolveAgentForStream agent identity", () => { }); }); +describe("resolveAgentForStream strict resolution", () => { + function createTopLevelMetadata(projectPath: string): WorkspaceMetadata { + return { + id: PARENT_WORKSPACE_ID, + name: PARENT_WORKSPACE_ID, + projectName: path.basename(projectPath), + projectPath, + runtimeConfig: DEFAULT_RUNTIME_CONFIG, + }; + } + + async function resolveTopLevel(params: { + projectPath: string; + agentId: string; + strictAgentResolution: boolean; + agentAiDefaults?: ProjectsConfig["agentAiDefaults"]; + }) { + const cfg: ProjectsConfig = { + projects: new Map([ + [ + params.projectPath, + { + trusted: true, + workspaces: [ + { id: PARENT_WORKSPACE_ID, name: PARENT_WORKSPACE_ID, path: params.projectPath }, + ], + }, + ], + ]), + ...(params.agentAiDefaults ? { agentAiDefaults: params.agentAiDefaults } : {}), + }; + return await resolveAgentForStream({ + workspaceId: PARENT_WORKSPACE_ID, + metadata: createTopLevelMetadata(params.projectPath), + runtime: new LocalRuntime(params.projectPath), + workspacePath: params.projectPath, + requestedAgentId: params.agentId, + disableWorkspaceAgents: false, + strictAgentResolution: params.strictAgentResolution, + callerToolPolicy: undefined, + cfg, + emitError: () => undefined, + isAdvisorExperimentEnabled: false, + }); + } + + test("unresolvable explicit agent fails loudly instead of falling back to exec", async () => { + using tempDir = new DisposableTempDir("agent-resolution-strict-unknown"); + const projectPath = path.join(tempDir.path, "project"); + await fs.mkdir(projectPath, { recursive: true }); + + // Default behavior: silent exec fallback keeps ordinary sends usable. + const lenient = await resolveTopLevel({ + projectPath, + agentId: "doesnotexist", + strictAgentResolution: false, + }); + expect(lenient.success).toBe(true); + if (lenient.success) expect(lenient.data.effectiveAgentId).toBe("exec"); + + // Strict mode (explicit workspace-turn overrides): running a different agent + // than requested must fail the stream, not silently swap in exec. + const strict = await resolveTopLevel({ + projectPath, + agentId: "doesnotexist", + strictAgentResolution: true, + }); + expect(strict.success).toBe(false); + if (!strict.success && strict.error.type === "unknown") { + expect(strict.error.raw).toContain("could not be resolved"); + } else { + expect(strict.success === false && strict.error.type).toBe("unknown"); + } + }); + + test("disabled explicit agent fails loudly in strict mode for top-level workspaces", async () => { + using tempDir = new DisposableTempDir("agent-resolution-strict-disabled"); + const projectPath = path.join(tempDir.path, "project"); + const agentsDir = path.join(projectPath, ".mux", "agents"); + await fs.mkdir(agentsDir, { recursive: true }); + await fs.writeFile( + path.join(agentsDir, "custom.md"), + ["---", "name: Custom", "base: exec", "---", "Custom agent."].join("\n") + ); + const agentAiDefaults = { custom: { enabled: false } }; + + const lenient = await resolveTopLevel({ + projectPath, + agentId: "custom", + strictAgentResolution: false, + agentAiDefaults, + }); + expect(lenient.success).toBe(true); + if (lenient.success) expect(lenient.data.effectiveAgentId).toBe("exec"); + + const strict = await resolveTopLevel({ + projectPath, + agentId: "custom", + strictAgentResolution: true, + agentAiDefaults, + }); + expect(strict.success).toBe(false); + if (!strict.success && strict.error.type === "unknown") { + expect(strict.error.raw).toContain("disabled"); + } else { + expect(strict.success === false && strict.error.type).toBe("unknown"); + } + }); +}); + describe("resolveAgentForStream advisor defaults", () => { test("enables advisor by default for Exec and Plan sub-agents when the experiment is enabled", async () => { const [execPolicy, planPolicy] = await Promise.all([ diff --git a/src/node/services/agentResolution.ts b/src/node/services/agentResolution.ts index 4f1b47ea9ee..35c1e391801 100644 --- a/src/node/services/agentResolution.ts +++ b/src/node/services/agentResolution.ts @@ -49,6 +49,15 @@ export interface ResolveAgentOptions { requestedAgentId: string | undefined; /** When true, skip workspace-specific agents (for "unbricking" broken agent files). */ disableWorkspaceAgents: boolean; + /** + * When true, a top-level requested agent that cannot be resolved (or is + * disabled) fails the stream loudly instead of silently falling back to exec. + * Set by workspace-turn launches with explicit agent overrides: their + * pre-dispatch validation races init hooks and user edits, so this post-init + * resolution is the last sound gate against running a different agent than + * the caller asked for. Sub-agent workspaces already fail loudly. + */ + strictAgentResolution?: boolean; /** Caller-supplied tool policy (applied AFTER agent policy for further restriction). */ callerToolPolicy: ToolPolicy | undefined; /** Loaded config from Config.loadConfigOrDefault(). */ @@ -186,6 +195,7 @@ export async function resolveAgentForStream( workspacePath, requestedAgentId: rawAgentId, disableWorkspaceAgents, + strictAgentResolution, callerToolPolicy, cfg, emitError, @@ -265,6 +275,17 @@ export async function resolveAgentForStream( } if (agentDefinition == null) { + if (strictAgentResolution && !isSubagentWorkspace) { + const errorMessage = `Agent '${requestedAgentId}' could not be resolved in this workspace; refusing to fall back to exec for an explicit agent request.`; + emitError( + createErrorEvent(workspaceId, { + messageId: createAssistantMessageId(), + error: errorMessage, + errorType: "unknown", + }) + ); + return Err({ type: "unknown", raw: errorMessage }); + } workspaceLog.warn("Failed to load agent definition; falling back", { requestedAgentIds, agentDiscoveryPaths: agentDiscoveryCandidates.map((candidate) => candidate.workspacePath), @@ -303,7 +324,7 @@ export async function resolveAgentForStream( if (effectivelyDisabled) { const errorMessage = `Agent '${agentDefinition.id}' is disabled.`; - if (isSubagentWorkspace) { + if (isSubagentWorkspace || strictAgentResolution) { const errorMessageId = createAssistantMessageId(); emitError( createErrorEvent(workspaceId, { diff --git a/src/node/services/agentSession.ts b/src/node/services/agentSession.ts index f536d32a14d..06073afc88c 100644 --- a/src/node/services/agentSession.ts +++ b/src/node/services/agentSession.ts @@ -4715,6 +4715,7 @@ export class AgentSession { workspaceGoalService: this.workspaceGoalService, experiments: options?.experiments, disableWorkspaceAgents: options?.disableWorkspaceAgents, + strictAgentResolution: options?.strictAgentResolution, hasQueuedMessages: this.hasQueuedMessages.bind(this), openaiTruncationModeOverride, // Mid-turn thinking overrides clamp against the same floor as the diff --git a/src/node/services/aiService.ts b/src/node/services/aiService.ts index 5a1e543d851..f4b99c59774 100644 --- a/src/node/services/aiService.ts +++ b/src/node/services/aiService.ts @@ -293,6 +293,8 @@ export interface StreamMessageOptions { /** Internal-only flag for Copilot billing attribution; never sourced from IPC schemas. */ agentInitiated?: boolean; agentId?: string; + /** See SendMessageOptionsSchema.strictAgentResolution: explicit-agent sends fail loudly instead of falling back to exec. */ + strictAgentResolution?: boolean; /** ACP prompt correlation id used to match stream events to a specific request. */ acpPromptId?: string; /** Tool names that should be delegated back to ACP clients for this request. */ @@ -1409,6 +1411,7 @@ export class AIService extends EventEmitter { muxProviderOptions, agentInitiated, agentId, + strictAgentResolution, acpPromptId, delegatedToolNames, recordFileState, @@ -1952,6 +1955,7 @@ export class AIService extends EventEmitter { runtime, workspacePath, requestedAgentId: agentId, + strictAgentResolution: strictAgentResolution ?? false, disableWorkspaceAgents: disableWorkspaceAgents ?? false, callerToolPolicy: toolPolicy, cfg, diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index 13008073056..fd4fffbd6be 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -97,6 +97,16 @@ function initGitRepo(projectPath: string): void { execSync('git commit -m "init"', { cwd: projectPath, stdio: "ignore" }); } +/** + * Git-prove the owner workspace's checked-out branch: owner-side agent prechecks and + * unreachable-target vouching only apply when the effective base branch is verified + * against the branch actually checked out in the owner. + */ +function checkoutOwnerBranch(projectPath: string, branch: string): void { + initGitRepo(projectPath); + execSync(`git checkout -b ${branch}`, { cwd: projectPath, stdio: "ignore" }); +} + async function collectFullHistory(service: HistoryService, workspaceId: string) { const messages: MuxMessage[] = []; const result = await service.iterateFullHistory(workspaceId, "forward", (chunk) => { @@ -995,12 +1005,16 @@ describe("TaskService", () => { expect(sendMessage).toHaveBeenCalledTimes(1); const sendMessageCall = sendMessage.mock.calls[0] as unknown[]; expect(sendMessageCall[0]).toBe("childworkspace"); - expect(sendMessageCall[2]).toMatchObject({ agentId: "plan" }); + // Explicit overrides also arm stream-time strict resolution: pre-dispatch validation + // races init hooks/user edits, so the stream must fail loudly instead of silently + // swapping in exec if the agent cannot be resolved post-init. + expect(sendMessageCall[2]).toMatchObject({ agentId: "plan", strictAgentResolution: true }); }); test("createWorkspaceTurn rejects invalid, unknown, and internal agent ids before creating a workspace", async () => { const config = await createTestConfig(rootDir); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + checkoutOwnerBranch(projectPath, "parent"); const createWorkspace = mock( (): Promise> => @@ -1043,6 +1057,7 @@ describe("TaskService", () => { const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir, { agentAiDefaults: { custom: { enabled: false } }, }); + checkoutOwnerBranch(projectPath, "parent"); await writeCustomAgentDefinition(projectPath); const createWorkspace = mock( @@ -1142,6 +1157,7 @@ describe("TaskService", () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + checkoutOwnerBranch(projectPath, "parent"); // Shadow the built-in plan agent with a hidden project-local override in the OWNER's // checkout: eligibility for unreachable targets must consult the shadow, not just the // embedded definition. @@ -1284,10 +1300,7 @@ describe("TaskService", () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); - // Owner vouching for an unreachable target requires git proof that the owner is - // actually checked out on the base branch the child is created from. - initGitRepo(projectPath); - execSync("git checkout -b parent", { cwd: projectPath, stdio: "ignore" }); + checkoutOwnerBranch(projectPath, "parent"); await writeCustomAgentDefinition(projectPath); // Deferred-provisioning runtimes return from create before the checkout is reachable. const unreachableCheckout = path.join(rootDir, "not-provisioned-yet"); @@ -1349,8 +1362,7 @@ describe("TaskService", () => { const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir, { workspaceName: "feature-foo", }); - initGitRepo(projectPath); - execSync("git checkout -b feature/foo", { cwd: projectPath, stdio: "ignore" }); + checkoutOwnerBranch(projectPath, "feature/foo"); const unreachableMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { ...createWorkspaceTurnMetadata(projectPath), @@ -1394,12 +1406,60 @@ describe("TaskService", () => { expect(sendMessage).toHaveBeenCalledTimes(1); }); + test("createWorkspaceTurn defers owner-side misses to the target when the base is unproven", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + // Owner is on feature/foo but its workspace name is feature-foo: with trunkBranch + // omitted, the child is created from the DISTINCT feature-foo branch, which may carry + // agents absent from the owner's branch. An owner-side miss must not fail-fast here — + // the created target checkout is authoritative. + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir, { + workspaceName: "feature-foo", + }); + checkoutOwnerBranch(projectPath, "feature/foo"); + + const targetOnlyCheckout = path.join(rootDir, "target-only-agent"); + const targetAgentsDir = path.join(targetOnlyCheckout, ".mux", "agents"); + await fsPromises.mkdir(targetAgentsDir, { recursive: true }); + await fsPromises.writeFile( + path.join(targetAgentsDir, "custom.md"), + ["---", "name: Custom", "base: exec", "---", "Target-only agent."].join("\n") + ); + + const targetMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { + ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, + namedWorkspacePath: targetOnlyCheckout, + }; + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: targetMetadata })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "custom", + prompt: "Run the target-only agent", + title: "Target-only agent", + workspace: { mode: "new" }, + }); + + expect(result.success).toBe(true); + expect(sendMessage).toHaveBeenCalledTimes(1); + const sendMessageCall = sendMessage.mock.calls[0] as unknown[]; + expect(sendMessageCall[2]).toMatchObject({ agentId: "custom" }); + }); + test("createWorkspaceTurn unreachable cross-host target fails closed even for built-ins", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); - initGitRepo(projectPath); - execSync("git checkout -b parent", { cwd: projectPath, stdio: "ignore" }); + checkoutOwnerBranch(projectPath, "parent"); // The created target lives on a remote host (per-workspace containers, Coder-style // per-workspace hosts). The owner's global agent roots say nothing about that host — diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 63b4e9e6a36..fd405d54b8b 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -4058,17 +4058,18 @@ export class TaskService { ); ownerVouchesForTargetBase = ownerBranch != null && ownerBranch === effectiveTrunkBranch; // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the - // OWNER's checkout before creating any workspace. Strict for an omitted - // trunkBranch (the child is created from the owner's own workspace line) or a - // verified same-branch request; advisory when the requested base may diverge — - // the target checkout is authoritative in that case. + // OWNER's checkout before creating any workspace. Fatal only when the owner's + // checked-out branch provably IS the target's base (an omitted trunkBranch still + // resolves to parentMeta.name, which may be a DIFFERENT branch than a slash-branch + // owner's — that distinct branch could carry target-only agents); otherwise the + // miss is advisory and the target checkout is authoritative post-create. const validation = await this.validateWorkspaceTurnAgentId({ cfg, agentId: requestedAgentId, ...ownerContext, }); if (!validation.success) { - if (requestedTrunkBranch == null || ownerVouchesForTargetBase) { + if (ownerVouchesForTargetBase) { return Err(validation.error); } log.debug( @@ -4308,6 +4309,10 @@ export class TaskService { ...(mode === "existing" && requestedAgentId != null ? { skipAiSettingsPersistence: true } : {}), + // Explicit overrides were validated pre-dispatch, but that validation races init + // hooks and later edits; stream-time resolution runs after initialization and must + // fail loudly rather than silently swap in exec (see strictAgentResolution docs). + ...(requestedAgentId != null ? { strictAgentResolution: true } : {}), }, { startStreamInBackground: true, From de00023f45d8eab6f9263451ac905bd4a66559f0 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 12:35:45 +0000 Subject: [PATCH 11/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20preserve=20strictAg?= =?UTF-8?q?entResolution=20across=20compaction=20and=20startup=20retry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/common/types/message.ts | 8 ++++++++ src/node/services/agentSession.ts | 5 +++++ 2 files changed, 13 insertions(+) diff --git a/src/common/types/message.ts b/src/common/types/message.ts index 6afd7d70c14..191a977d4da 100644 --- a/src/common/types/message.ts +++ b/src/common/types/message.ts @@ -58,6 +58,7 @@ type PreservedSendOptions = Pick< | "providerOptions" | "experiments" | "disableWorkspaceAgents" + | "strictAgentResolution" | "allowAgentSetGoal" | "skipAiSettingsPersistence" >; @@ -74,6 +75,10 @@ export function pickPreservedSendOptions(options: SendMessageOptions): Preserved providerOptions: options.providerOptions, experiments: options.experiments, disableWorkspaceAgents: options.disableWorkspaceAgents, + // Delegated turns with explicit agent overrides must stay loud across the + // compaction replay too — dropping this would let the follow-up silently + // fall back to exec if the agent vanished in the meantime. + strictAgentResolution: options.strictAgentResolution, allowAgentSetGoal: options.allowAgentSetGoal, skipAiSettingsPersistence: options.skipAiSettingsPersistence, }; @@ -91,6 +96,7 @@ export type StartupRetrySendOptions = Pick< | "providerOptions" | "experiments" | "disableWorkspaceAgents" + | "strictAgentResolution" | "allowAgentSetGoal" > & { /** Correlation for a delegated workspace turn that must survive restart recovery. */ @@ -124,6 +130,8 @@ export function pickStartupRetrySendOptions( providerOptions: options.providerOptions, experiments: options.experiments, disableWorkspaceAgents: options.disableWorkspaceAgents, + // Keep explicit-agent turns loud across restart recovery (see pickPreservedSendOptions). + strictAgentResolution: options.strictAgentResolution, allowAgentSetGoal: options.allowAgentSetGoal, ...(workspaceTurnMuxMetadata != null ? { muxMetadata: workspaceTurnMuxMetadata } : {}), ...(agentInitiated === true ? { agentInitiated: true } : {}), diff --git a/src/node/services/agentSession.ts b/src/node/services/agentSession.ts index 06073afc88c..4042c7e0a55 100644 --- a/src/node/services/agentSession.ts +++ b/src/node/services/agentSession.ts @@ -1900,6 +1900,11 @@ export class AgentSession { if (typeof persistedDisableWorkspaceAgents === "boolean") { retryRequest.disableWorkspaceAgents = persistedDisableWorkspaceAgents; } + // Explicit-agent delegated turns must stay loud across restart recovery: without + // this, a replay after the agent was removed/disabled would silently run exec. + if (persistedRetrySendOptions?.strictAgentResolution === true) { + retryRequest.strictAgentResolution = true; + } if (persistedRetrySendOptions?.agentInitiated === true) { retryRequest.agentInitiated = true; From 5b8ebd741c6ca9a8fbda5a89b88acfc16a40f6b3 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 13:01:13 +0000 Subject: [PATCH 12/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20honest=20disposable?= =?UTF-8?q?-cleanup=20wording;=20document=20launch=20AI-defaults=20snapsho?= =?UTF-8?q?t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node/services/taskService.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index fd405d54b8b..3fdc6197e0c 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -4130,11 +4130,13 @@ export class TaskService { ownerResolutionPredictsTarget: ownerVouchesForTargetBase, }); if (!validation.success) { - // Disposable workspaces are removed by the settlement's disposable cleanup, so - // only non-disposable workspaces are advertised as retryable. + // Disposable workspaces are removed by the settlement's disposable cleanup. + // That cleanup is best-effort (failures are logged and swallowed), so the + // wording must not assert completed removal; if cleanup fails the workspace + // stays owner-owned and a mode="existing" retry still passes ownership. agentValidationError = args.workspace?.disposable === true - ? `${validation.error} — no turn was dispatched; the disposable workspace (${targetWorkspaceId}) was cleaned up` + ? `${validation.error} — no turn was dispatched; automatic cleanup of the disposable workspace (${targetWorkspaceId}) was scheduled (if cleanup fails, it remains owned by this caller and retryable via workspace.mode="existing")` : `${validation.error} — no turn was dispatched; the created workspace (${targetWorkspaceId}) is owned by this caller and can be retried via workspace.mode="existing" once ready`; } else { agentDefinitionContext = { @@ -4185,6 +4187,13 @@ export class TaskService { fallbacks: this.buildParentAiSettingsFallbacks(parentMeta, workspaceTurnAgentId), // Explicit agent overrides resolve the agent's own frontmatter `ai` defaults from the // checkout they were validated against (mirrors resolveTaskAISettings' definitionContext). + // Known tradeoff: these launch AI defaults are a snapshot — an init hook that later + // rewrites the agent's `ai` frontmatter does not retroactively change the model/thinking + // already selected here (waiting for init is not an option under the service-wide mutex). + // This is bounded to convenience defaults: callers wanting determinism pass explicit + // model/thinking, the send path re-clamps thinking and re-gates reasoning per model at + // request time, and the authoritative prompt/tool policy is always resolved at stream + // time (after init) with strictAgentResolution guarding agent identity. ...(agentDefinitionContext != null ? { definitionContext: agentDefinitionContext } : {}), }); // Selected (not effective) values: sendMessage persists what it From e572dac6ec5122b518fa408f8107fa517e6be006 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 13:16:04 +0000 Subject: [PATCH 13/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20strict=20resolution?= =?UTF-8?q?=20also=20rejects=20definitions=20hidden=20after=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node/services/agentResolution.test.ts | 37 +++++++++++++++++++++++ src/node/services/agentResolution.ts | 22 ++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/src/node/services/agentResolution.test.ts b/src/node/services/agentResolution.test.ts index 3ce9c3741ca..80bd23a05a5 100644 --- a/src/node/services/agentResolution.test.ts +++ b/src/node/services/agentResolution.test.ts @@ -547,6 +547,43 @@ describe("resolveAgentForStream strict resolution", () => { } }); + test("hidden explicit agent fails loudly in strict mode for top-level workspaces", async () => { + using tempDir = new DisposableTempDir("agent-resolution-strict-hidden"); + const projectPath = path.join(tempDir.path, "project"); + const agentsDir = path.join(projectPath, ".mux", "agents"); + await fs.mkdir(agentsDir, { recursive: true }); + // A definition hidden between launch-time validation and streaming (init hook or + // concurrent edit): strict sends must uphold the workspace-task contract that + // internal agents are ineligible instead of running the hidden policy. + await fs.writeFile( + path.join(agentsDir, "custom.md"), + ["---", "name: Custom", "base: exec", "ui:", " hidden: true", "---", "Hidden agent."].join( + "\n" + ) + ); + + const strict = await resolveTopLevel({ + projectPath, + agentId: "custom", + strictAgentResolution: true, + }); + expect(strict.success).toBe(false); + if (!strict.success && strict.error.type === "unknown") { + expect(strict.error.raw).toContain("not selectable"); + } else { + expect(strict.success === false && strict.error.type).toBe("unknown"); + } + + // Lenient top-level sends keep today's behavior (no visibility gate). + const lenient = await resolveTopLevel({ + projectPath, + agentId: "custom", + strictAgentResolution: false, + }); + expect(lenient.success).toBe(true); + if (lenient.success) expect(lenient.data.effectiveAgentId).toBe("custom"); + }); + test("disabled explicit agent fails loudly in strict mode for top-level workspaces", async () => { using tempDir = new DisposableTempDir("agent-resolution-strict-disabled"); const projectPath = path.join(tempDir.path, "project"); diff --git a/src/node/services/agentResolution.ts b/src/node/services/agentResolution.ts index 35c1e391801..c17c3013204 100644 --- a/src/node/services/agentResolution.ts +++ b/src/node/services/agentResolution.ts @@ -32,6 +32,7 @@ import { resolveAgentFrontmatter, } from "@/node/services/agentDefinitions/agentDefinitionsService"; import { isAgentEffectivelyDisabled } from "@/node/services/agentDefinitions/agentEnablement"; +import { resolveAgentVisibility } from "@/node/services/agentDefinitions/agentVisibility"; import { resolveAgentInheritanceChain } from "@/node/services/agentDefinitions/resolveAgentInheritanceChain"; import { resolveToolPolicyForAgent } from "@/node/services/agentDefinitions/resolveToolPolicy"; import { log } from "./log"; @@ -315,6 +316,27 @@ export async function resolveAgentForStream( } ); + // Strict explicit-agent sends must also reject definitions that are no longer + // selectable: the workspace-task contract excludes internal (ui.hidden) agents, + // and an init hook or concurrent edit could hide the definition between + // launch-time validation and this stream. Sub-agent workspaces legitimately run + // hidden agents (explore, compact), so only strict top-level sends are gated. + if ( + strictAgentResolution && + !isSubagentWorkspace && + !resolveAgentVisibility(resolvedFrontmatter.ui).selectable + ) { + const errorMessage = `Agent '${agentDefinition.id}' is not selectable for explicit agent requests.`; + emitError( + createErrorEvent(workspaceId, { + messageId: createAssistantMessageId(), + error: errorMessage, + errorType: "unknown", + }) + ); + return Err({ type: "unknown", raw: errorMessage }); + } + const effectivelyDisabled = isAgentEffectivelyDisabled({ cfg, agentId: agentDefinition.id, From f2e23b8c22e474d67a1655ff70febeaa6021cf52 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 13:33:42 +0000 Subject: [PATCH 14/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20exempt=20internal?= =?UTF-8?q?=20compact=20request=20from=20strict=20gate;=20verify=20exec=20?= =?UTF-8?q?shadows;=20strict=20fail-closed=20on=20eligibility=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node/services/agentResolution.test.ts | 58 +++++++++++++++++++ src/node/services/agentResolution.ts | 29 +++++++--- .../agentSession.autoCompaction.test.ts | 43 ++++++++++++++ src/node/services/agentSession.ts | 4 ++ 4 files changed, 127 insertions(+), 7 deletions(-) diff --git a/src/node/services/agentResolution.test.ts b/src/node/services/agentResolution.test.ts index 80bd23a05a5..367eff092f0 100644 --- a/src/node/services/agentResolution.test.ts +++ b/src/node/services/agentResolution.test.ts @@ -584,6 +584,64 @@ describe("resolveAgentForStream strict resolution", () => { if (lenient.success) expect(lenient.data.effectiveAgentId).toBe("custom"); }); + test("hidden exec shadow fails loudly in strict mode instead of running the shadow", async () => { + using tempDir = new DisposableTempDir("agent-resolution-strict-exec-shadow"); + const projectPath = path.join(tempDir.path, "project"); + const agentsDir = path.join(projectPath, ".mux", "agents"); + await fs.mkdir(agentsDir, { recursive: true }); + // Discovery prefers a project exec shadow over the built-in; strict sends must gate + // exec like any other id instead of skipping the eligibility block for it. + await fs.writeFile( + path.join(agentsDir, "exec.md"), + ["---", "name: Exec", "ui:", " hidden: true", "---", "Hidden exec shadow."].join("\n") + ); + + const strict = await resolveTopLevel({ + projectPath, + agentId: "exec", + strictAgentResolution: true, + }); + expect(strict.success).toBe(false); + if (!strict.success && strict.error.type === "unknown") { + expect(strict.error.raw).toContain("not selectable"); + } else { + expect(strict.success === false && strict.error.type).toBe("unknown"); + } + }); + + test("strict mode fails closed when eligibility resolution throws", async () => { + using tempDir = new DisposableTempDir("agent-resolution-strict-broken-base"); + const projectPath = path.join(tempDir.path, "project"); + const agentsDir = path.join(projectPath, ".mux", "agents"); + await fs.mkdir(agentsDir, { recursive: true }); + // A base pointing at a missing definition makes frontmatter resolution throw; + // strict sends must not stream a partially resolved prompt/tool policy. + await fs.writeFile( + path.join(agentsDir, "custom.md"), + ["---", "name: Custom", "base: missing-base", "---", "Broken chain."].join("\n") + ); + + const strict = await resolveTopLevel({ + projectPath, + agentId: "custom", + strictAgentResolution: true, + }); + expect(strict.success).toBe(false); + if (!strict.success && strict.error.type === "unknown") { + expect(strict.error.raw).toContain("could not be"); + } else { + expect(strict.success === false && strict.error.type).toBe("unknown"); + } + + // Lenient sends keep the best-effort behavior. + const lenient = await resolveTopLevel({ + projectPath, + agentId: "custom", + strictAgentResolution: false, + }); + expect(lenient.success).toBe(true); + }); + test("disabled explicit agent fails loudly in strict mode for top-level workspaces", async () => { using tempDir = new DisposableTempDir("agent-resolution-strict-disabled"); const projectPath = path.join(tempDir.path, "project"); diff --git a/src/node/services/agentResolution.ts b/src/node/services/agentResolution.ts index c17c3013204..9c5e8aafcd8 100644 --- a/src/node/services/agentResolution.ts +++ b/src/node/services/agentResolution.ts @@ -232,6 +232,9 @@ export async function resolveAgentForStream( let agentDiscoveryPath = agentDiscoveryCandidates[0]?.workspacePath ?? workspacePath; const isSubagentWorkspace = Boolean(metadata.parentWorkspaceId); + // Strict explicit-agent gating applies only to top-level sends: sub-agent workspaces + // already fail loudly and legitimately run hidden agents (explore, compact). + const strictTopLevel = strictAgentResolution === true && !isSubagentWorkspace; // --- Load agent definition (with fallback to exec) --- let agentDefinition: Awaited> | undefined; @@ -276,7 +279,7 @@ export async function resolveAgentForStream( } if (agentDefinition == null) { - if (strictAgentResolution && !isSubagentWorkspace) { + if (strictTopLevel) { const errorMessage = `Agent '${requestedAgentId}' could not be resolved in this workspace; refusing to fall back to exec for an explicit agent request.`; emitError( createErrorEvent(workspaceId, { @@ -304,7 +307,9 @@ export async function resolveAgentForStream( // Disabled agents should never run as sub-agents, even if a task workspace already exists // on disk (e.g., config changed since creation). // For top-level workspaces, fall back to exec to keep the workspace usable. - if (agentDefinition.id !== "exec") { + // Strict sends also verify exec itself: discovery can select a project/global exec + // shadow, and a hidden shadow must hit the selectability gate below like any other id. + if (agentDefinition.id !== "exec" || strictTopLevel) { try { const resolvedFrontmatter = await resolveAgentFrontmatter( agentDiscoveryRuntime, @@ -321,11 +326,7 @@ export async function resolveAgentForStream( // and an init hook or concurrent edit could hide the definition between // launch-time validation and this stream. Sub-agent workspaces legitimately run // hidden agents (explore, compact), so only strict top-level sends are gated. - if ( - strictAgentResolution && - !isSubagentWorkspace && - !resolveAgentVisibility(resolvedFrontmatter.ui).selectable - ) { + if (strictTopLevel && !resolveAgentVisibility(resolvedFrontmatter.ui).selectable) { const errorMessage = `Agent '${agentDefinition.id}' is not selectable for explicit agent requests.`; emitError( createErrorEvent(workspaceId, { @@ -371,6 +372,20 @@ export async function resolveAgentForStream( effectiveAgentId = agentDefinition.id; } } catch (error: unknown) { + // Strict sends fail closed when eligibility cannot be verified: a hook or edit + // that breaks the definition (e.g. a base pointing at a missing definition) after + // launch validation would otherwise stream a partially resolved prompt/tool policy. + if (strictTopLevel) { + const errorMessage = `Agent '${agentDefinition.id}' eligibility could not be verified (${getErrorMessage(error)}); refusing to stream an explicit agent request.`; + emitError( + createErrorEvent(workspaceId, { + messageId: createAssistantMessageId(), + error: errorMessage, + errorType: "unknown", + }) + ); + return Err({ type: "unknown", raw: errorMessage }); + } // Best-effort only — do not fail a stream due to disablement resolution. workspaceLog.debug("Failed to resolve agent enablement; continuing", { agentId: agentDefinition.id, diff --git a/src/node/services/agentSession.autoCompaction.test.ts b/src/node/services/agentSession.autoCompaction.test.ts index 53ceb0d016e..b5a72e859ec 100644 --- a/src/node/services/agentSession.autoCompaction.test.ts +++ b/src/node/services/agentSession.autoCompaction.test.ts @@ -600,6 +600,49 @@ describe("AgentSession on-send auto-compaction snapshot deferral", () => { session.dispose(); }); + test("clears strictAgentResolution on the internal compact request", async () => { + const workspaceId = "ws-auto-compaction-clears-strict"; + + const streamMessage = mock((_request: unknown) => Promise.resolve(Ok(undefined))); + const { session } = await createSessionHarness({ + workspaceId, + streamMessage: streamMessage as unknown as AIService["streamMessage"], + }); + + // A strict explicit-agent workspace turn hitting auto-compaction: the internal + // request intentionally runs the hidden compact agent, so the strict gate must not + // apply to it (it would reject compact as not selectable and break compaction). + const baseOptions: SendMessageOptions = { + model: "anthropic:claude-sonnet-4-6", + agentId: "plan", + strictAgentResolution: true, + }; + const followUpContent: CompactionFollowUpRequest = { + text: "Continue", + model: baseOptions.model, + agentId: "plan", + }; + + const internals = session as unknown as { + buildAutoCompactionRequest: (params: { + followUpContent: CompactionFollowUpRequest; + baseOptions: SendMessageOptions; + reason: "on-send" | "mid-stream"; + }) => { sendOptions: SendMessageOptions }; + }; + + const compactionRequest = internals.buildAutoCompactionRequest({ + followUpContent, + baseOptions, + reason: "mid-stream", + }); + + expect(compactionRequest.sendOptions.agentId).toBe("compact"); + expect(compactionRequest.sendOptions.strictAgentResolution).toBeUndefined(); + + session.dispose(); + }); + test("compaction model explicit override takes priority over baseOptions.model", async () => { const workspaceId = "ws-auto-compaction-explicit-model-overrides-base-model"; diff --git a/src/node/services/agentSession.ts b/src/node/services/agentSession.ts index 4042c7e0a55..0a52bcf6259 100644 --- a/src/node/services/agentSession.ts +++ b/src/node/services/agentSession.ts @@ -4285,6 +4285,10 @@ export class AgentSession { const sendOptions: SendMessageOptions = { ...params.baseOptions, agentId: "compact", + // This internal request intentionally runs the hidden compact agent, so the + // caller's strict explicit-agent gate must not apply to it. The post-compaction + // follow-up re-arms strictness via pickPreservedSendOptions. + strictAgentResolution: undefined, skipAiSettingsPersistence: true, model: resolved.selected.model, // Effective (clamped) thinking: this internal request skips persistence, From cd0f47eb5ac8648f94c0315588cf0aa0efcac3bb Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 14:00:25 +0000 Subject: [PATCH 15/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20continue=20delegate?= =?UTF-8?q?d=20turns=20under=20their=20own=20options=20on=20monitor=20wake?= =?UTF-8?q?s;=20forward=20strictness=20to=20resumed=20turns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...gentSession.continueMessageAgentId.test.ts | 23 ++++ src/node/services/agentSession.ts | 3 + src/node/services/workspaceService.test.ts | 104 +++++++++++++++++- src/node/services/workspaceService.ts | 56 +++++++++- 4 files changed, 184 insertions(+), 2 deletions(-) diff --git a/src/node/services/agentSession.continueMessageAgentId.test.ts b/src/node/services/agentSession.continueMessageAgentId.test.ts index e672a7d61ec..e1360914198 100644 --- a/src/node/services/agentSession.continueMessageAgentId.test.ts +++ b/src/node/services/agentSession.continueMessageAgentId.test.ts @@ -231,6 +231,29 @@ describe("AgentSession continue-message agentId fallback", () => { expect(internals.lastAutoRetryResumeRequest?.agentInitiated).toBe(true); }); + test("dispatchPendingFollowUp forwards strictAgentResolution to the resumed turn", async () => { + let dispatchedOptions: SendOptions | undefined; + const { internals } = await createSession([ + compactionSummaryMessage("summary-strict", { + text: "continue delegated work", + model: "openai:gpt-4o", + agentId: "plan", + strictAgentResolution: true, + }), + ]); + internals.sendMessage = mock((_message: string, options?: SendOptions) => { + dispatchedOptions = options; + return Promise.resolve({ success: true as const }); + }); + + await internals.dispatchPendingFollowUp(); + + // The requested agent may have been removed/hidden/disabled while compaction ran; + // the resumed turn must stay loud instead of silently falling back to exec. + expect(dispatchedOptions?.agentId).toBe("plan"); + expect(dispatchedOptions?.strictAgentResolution).toBe(true); + }); + test("dispatchPendingFollowUp skips idle-only follow-ups when queued user input exists", async () => { const { session, historyService, internals } = await createSession([ compactionSummaryMessage("summary-idle-only", idleFollowUp()), diff --git a/src/node/services/agentSession.ts b/src/node/services/agentSession.ts index 0a52bcf6259..52027e1bee0 100644 --- a/src/node/services/agentSession.ts +++ b/src/node/services/agentSession.ts @@ -6714,6 +6714,9 @@ export class AgentSession { experiments: followUp.experiments, allowAgentSetGoal: followUp.allowAgentSetGoal, disableWorkspaceAgents: followUp.disableWorkspaceAgents, + // Explicit-agent turns stay loud on the resumed turn too: the requested agent + // may have been removed/hidden/disabled while compaction ran. + strictAgentResolution: followUp.strictAgentResolution, skipAiSettingsPersistence: followUp.skipAiSettingsPersistence, }; diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index daa1e1d2ca5..33c608feb2b 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -49,7 +49,7 @@ import type { TerminalService } from "@/node/services/terminalService"; import type { DesktopSessionManager } from "@/node/services/desktop/DesktopSessionManager"; import type { WorktreeArchiveSnapshot } from "@/common/schemas/project"; import type { BashToolResult } from "@/common/types/tools"; -import type { WorkspaceChatMessage } from "@/common/orpc/types"; +import type { SendMessageOptions, WorkspaceChatMessage } from "@/common/orpc/types"; import { createMuxMessage } from "@/common/types/message"; import { buildStagedAttachmentNotice } from "@/browser/features/ChatInput/stagedAttachments"; import { @@ -14575,6 +14575,108 @@ describe("WorkspaceService.getGoalContinuationRuntimeState", () => { } }); + // -------------------------------------------------------------------------- + // getDelegatedTurnContinuationSendOptions — bash-monitor wake continuations + // -------------------------------------------------------------------------- + + describe("delegated-turn continuation send options", () => { + async function makeServiceWithHistory(): Promise<{ + service: WorkspaceService; + historyService: HistoryService; + }> { + const mockAIService = { + isStreaming: mock(() => false), + on: mock(() => undefined), + off: mock(() => undefined), + } as unknown as AIService; + const mockInitStateManager: Partial = { + on: mock(() => undefined as unknown as InitStateManager), + getInitState: mock(() => undefined), + }; + const mockConfig: Partial = { + srcDir: "/tmp/test", + getAllWorkspaceMetadata: mock(() => Promise.resolve([])), + getSessionDir: mock(() => "/tmp/test/sessions"), + generateStableId: mock(() => "test-id"), + }; + const { historyService } = await createTestHistoryService(); + const service = new WorkspaceService( + mockConfig as Config, + historyService, + mockAIService, + mockInitStateManager as InitStateManager, + {} as ExtensionMetadataService, + {} as BackgroundProcessManager + ); + return { service, historyService }; + } + + interface DelegatedContinuationInternals { + getDelegatedTurnContinuationSendOptions: ( + workspaceId: string + ) => Promise; + } + + const delegatedTurnMessage = (id: string) => + createMuxMessage(id, "user", "Delegated prompt", { + timestamp: Date.now(), + muxMetadata: { + type: "workspace-turn-task" as const, + taskHandleId: "wst_handle", + ownerWorkspaceId: "owner-ws", + turnId: "turn-1", + }, + retrySendOptions: { + model: "anthropic:claude-opus-4-6", + agentId: "plan", + strictAgentResolution: true, + agentInitiated: true, + }, + }); + + test("continues an in-flight delegated turn under its own per-turn options", async () => { + const workspaceId = "ws-delegated-continuation"; + const { service, historyService } = await makeServiceWithHistory(); + await historyService.appendToHistory(workspaceId, delegatedTurnMessage("delegated-1")); + // A previous wake continuation must not hide the delegated turn's options. + await historyService.appendToHistory( + workspaceId, + createMuxMessage("wake-1", "user", "Monitor matched", { + timestamp: Date.now(), + muxMetadata: { type: "bash-monitor-wake" as const, records: [] }, + }) + ); + + const internals = service as unknown as DelegatedContinuationInternals; + const options = await internals.getDelegatedTurnContinuationSendOptions(workspaceId); + + expect(options).not.toBeNull(); + // Per-turn overrides (agent, strictness) continue the turn; they never become + // workspace defaults, and internal-only fields are not forwarded. + expect(options).toMatchObject({ + model: "anthropic:claude-opus-4-6", + agentId: "plan", + strictAgentResolution: true, + skipAiSettingsPersistence: true, + }); + expect(options && "agentInitiated" in options).toBe(false); + expect(options?.muxMetadata).toBeUndefined(); + }); + + test("yields nothing once another user send follows the delegated prompt", async () => { + const workspaceId = "ws-delegated-superseded"; + const { service, historyService } = await makeServiceWithHistory(); + await historyService.appendToHistory(workspaceId, delegatedTurnMessage("delegated-2")); + await historyService.appendToHistory( + workspaceId, + createMuxMessage("manual-1", "user", "Manual user message", { timestamp: Date.now() }) + ); + + const internals = service as unknown as DelegatedContinuationInternals; + expect(await internals.getDelegatedTurnContinuationSendOptions(workspaceId)).toBeNull(); + }); + }); + // -------------------------------------------------------------------------- // getGoalContinuationKickoffSendOptions — model-resolution cascade // -------------------------------------------------------------------------- diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index c796bf0b7b6..7576ed236dd 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -2459,7 +2459,11 @@ export class WorkspaceService extends EventEmitter { return; } - const sendOptions = await this.getWorkflowContinuationSendOptions(ownerWorkspaceId); + // An in-flight delegated workspace turn continues under its own send options + // (per-turn agent/model overrides are not in the workspace's persisted defaults). + const sendOptions = + (await this.getDelegatedTurnContinuationSendOptions(ownerWorkspaceId)) ?? + (await this.getWorkflowContinuationSendOptions(ownerWorkspaceId)); if (sendOptions == null) { log.debug("Bash monitor wake has no send options; leaving pending", { ownerWorkspaceId }); return; @@ -12173,6 +12177,56 @@ export class WorkspaceService extends EventEmitter { return this.getGoalContinuationKickoffSendOptions(workspaceId); } + /** + * Send options for continuing an in-flight delegated workspace turn (bash-monitor + * wakes cut turns at tool boundaries). The delegated prompt's persisted + * retrySendOptions carry the turn's own settings — including per-turn overrides + * (agentId, model, strictAgentResolution) that are deliberately NOT in the + * workspace's persisted defaults when the launch used skipAiSettingsPersistence — + * so resolving from workspace defaults would continue the turn under the wrong + * agent. Walks user messages backwards, skipping this mechanism's own wake + * continuations, and yields nothing once any other user send follows the delegated + * prompt (the delegated turn is no longer the active conversation context). + * Continuations never persist these options as workspace defaults. + */ + private async getDelegatedTurnContinuationSendOptions( + workspaceId: string + ): Promise { + const history = await this.historyService.getHistoryFromLatestBoundary(workspaceId); + if (!history.success) { + return null; + } + for (let i = history.data.length - 1; i >= 0; i--) { + const message = history.data[i]; + if (message.role !== "user") { + continue; + } + const muxMetadata = message.metadata?.muxMetadata; + if (muxMetadata?.type === "bash-monitor-wake") { + continue; + } + if (muxMetadata?.type !== "workspace-turn-task") { + return null; + } + const retrySendOptions = message.metadata?.retrySendOptions; + if (retrySendOptions == null) { + return null; + } + const { + muxMetadata: _turnCorrelation, + agentInitiated: _agentInitiated, + goalKind: _goalKind, + ...sendOptions + } = retrySendOptions; + return { + ...sendOptions, + // Per-turn continuation settings must not become workspace defaults. + skipAiSettingsPersistence: true, + }; + } + return null; + } + /** * Defensive providers-config read: tests construct WorkspaceService with * partial AIService mocks, so a missing method degrades to null instead of From 6f24c86fc913e269636a9d3ec9464276236a93b1 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 14:20:07 +0000 Subject: [PATCH 16/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20tolerate=20partial?= =?UTF-8?q?=20history=20mocks=20in=20delegated-turn=20continuation=20looku?= =?UTF-8?q?p?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node/services/workspaceService.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index 7576ed236dd..9c966e82f55 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -12192,6 +12192,11 @@ export class WorkspaceService extends EventEmitter { private async getDelegatedTurnContinuationSendOptions( workspaceId: string ): Promise { + // Tests construct WorkspaceService with partial HistoryService mocks (same + // defensive pattern as the iterateFullHistory caller above). + if (typeof this.historyService.getHistoryFromLatestBoundary !== "function") { + return null; + } const history = await this.historyService.getHistoryFromLatestBoundary(workspaceId); if (!history.success) { return null; From fae0debedffeb70da2b26722023f83a515c25907 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 14:48:26 +0000 Subject: [PATCH 17/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20reuse=20delegated-t?= =?UTF-8?q?urn=20overrides=20only=20while=20the=20turn=20is=20still=20open?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node/services/workspaceService.test.ts | 50 ++++++++++++++++++---- src/node/services/workspaceService.ts | 25 +++++++---- 2 files changed, 59 insertions(+), 16 deletions(-) diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index 33c608feb2b..9a288927e77 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -14617,15 +14617,17 @@ describe("WorkspaceService.getGoalContinuationRuntimeState", () => { ) => Promise; } + const delegatedTurnCorrelation = { + type: "workspace-turn-task" as const, + taskHandleId: "wst_handle", + ownerWorkspaceId: "owner-ws", + turnId: "turn-1", + }; + const delegatedTurnMessage = (id: string) => createMuxMessage(id, "user", "Delegated prompt", { timestamp: Date.now(), - muxMetadata: { - type: "workspace-turn-task" as const, - taskHandleId: "wst_handle", - ownerWorkspaceId: "owner-ws", - turnId: "turn-1", - }, + muxMetadata: delegatedTurnCorrelation, retrySendOptions: { model: "anthropic:claude-opus-4-6", agentId: "plan", @@ -14634,10 +14636,23 @@ describe("WorkspaceService.getGoalContinuationRuntimeState", () => { }, }); - test("continues an in-flight delegated turn under its own per-turn options", async () => { + /** Correlated assistant response; "tool-calls" is the queue-dispatch cut that leaves the turn open. */ + const delegatedAssistantMessage = (id: string, finishReason: "tool-calls" | "stop") => + createMuxMessage(id, "assistant", "Working…", { + timestamp: Date.now(), + partial: false, + finishReason, + muxMetadata: delegatedTurnCorrelation, + }); + + test("continues a still-open delegated turn under its own per-turn options", async () => { const workspaceId = "ws-delegated-continuation"; const { service, historyService } = await makeServiceWithHistory(); await historyService.appendToHistory(workspaceId, delegatedTurnMessage("delegated-1")); + await historyService.appendToHistory( + workspaceId, + delegatedAssistantMessage("assistant-cut", "tool-calls") + ); // A previous wake continuation must not hide the delegated turn's options. await historyService.appendToHistory( workspaceId, @@ -14663,10 +14678,29 @@ describe("WorkspaceService.getGoalContinuationRuntimeState", () => { expect(options?.muxMetadata).toBeUndefined(); }); + test("yields nothing after a terminal assistant response closed the delegated turn", async () => { + const workspaceId = "ws-delegated-closed"; + const { service, historyService } = await makeServiceWithHistory(); + await historyService.appendToHistory(workspaceId, delegatedTurnMessage("delegated-2")); + // finishReason "stop" ends the delegated turn: a later monitor match is a NEW + // synthetic turn and must resolve from persisted defaults, not stale overrides. + await historyService.appendToHistory( + workspaceId, + delegatedAssistantMessage("assistant-final", "stop") + ); + + const internals = service as unknown as DelegatedContinuationInternals; + expect(await internals.getDelegatedTurnContinuationSendOptions(workspaceId)).toBeNull(); + }); + test("yields nothing once another user send follows the delegated prompt", async () => { const workspaceId = "ws-delegated-superseded"; const { service, historyService } = await makeServiceWithHistory(); - await historyService.appendToHistory(workspaceId, delegatedTurnMessage("delegated-2")); + await historyService.appendToHistory(workspaceId, delegatedTurnMessage("delegated-3")); + await historyService.appendToHistory( + workspaceId, + delegatedAssistantMessage("assistant-cut-2", "tool-calls") + ); await historyService.appendToHistory( workspaceId, createMuxMessage("manual-1", "user", "Manual user message", { timestamp: Date.now() }) diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index 9c966e82f55..723279147af 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -31,6 +31,7 @@ import { AgentSession, clearProviderConfigFixableAbandonMarkers, CONTEXT_MUTATION_SEND_BLOCKED_MESSAGE, + inheritOpenWorkspaceTurnMetadata, type StreamErrorRecoveryOutcome, } from "@/node/services/agentSession"; import type { HistoryService } from "@/node/services/historyService"; @@ -12178,15 +12179,18 @@ export class WorkspaceService extends EventEmitter { } /** - * Send options for continuing an in-flight delegated workspace turn (bash-monitor + * Send options for continuing a STILL-OPEN delegated workspace turn (bash-monitor * wakes cut turns at tool boundaries). The delegated prompt's persisted * retrySendOptions carry the turn's own settings — including per-turn overrides * (agentId, model, strictAgentResolution) that are deliberately NOT in the * workspace's persisted defaults when the launch used skipAiSettingsPersistence — * so resolving from workspace defaults would continue the turn under the wrong - * agent. Walks user messages backwards, skipping this mechanism's own wake - * continuations, and yields nothing once any other user send follows the delegated - * prompt (the delegated turn is no longer the active conversation context). + * agent. Openness is decided by the same rule as workspace-turn correlation + * (inheritOpenWorkspaceTurnMetadata): only a correlated assistant cut with + * finishReason "tool-calls" leaves the turn open. Once a terminal assistant + * response closed the turn (or any other user send took over the conversation), + * a late monitor match is a NEW synthetic turn and resolves from the target's + * persisted defaults instead of resurrecting stale per-turn overrides. * Continuations never persist these options as workspace defaults. */ private async getDelegatedTurnContinuationSendOptions( @@ -12201,18 +12205,23 @@ export class WorkspaceService extends EventEmitter { if (!history.success) { return null; } + const openTurn = inheritOpenWorkspaceTurnMetadata(history.data); + if (openTurn == null) { + return null; + } for (let i = history.data.length - 1; i >= 0; i--) { const message = history.data[i]; if (message.role !== "user") { continue; } const muxMetadata = message.metadata?.muxMetadata; - if (muxMetadata?.type === "bash-monitor-wake") { + if ( + muxMetadata?.type !== "workspace-turn-task" || + muxMetadata.taskHandleId !== openTurn.taskHandleId || + muxMetadata.turnId !== openTurn.turnId + ) { continue; } - if (muxMetadata?.type !== "workspace-turn-task") { - return null; - } const retrySendOptions = message.metadata?.retrySendOptions; if (retrySendOptions == null) { return null; From 24c27cec1249c0b7a01d47561cb51bc3ba2168a3 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 15:17:15 +0000 Subject: [PATCH 18/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20pin=20validated=20a?= =?UTF-8?q?gent=20provenance;=20post-compaction=20wake=20carriers;=20white?= =?UTF-8?q?list=20persisted=20continuation=20options?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/common/orpc/schemas/stream.ts | 17 ++-- src/node/services/agentResolution.test.ts | 32 +++++++- src/node/services/agentResolution.ts | 39 +++++++-- src/node/services/agentSession.ts | 6 +- src/node/services/aiService.ts | 4 +- src/node/services/taskService.test.ts | 12 ++- src/node/services/taskService.ts | 54 ++++++++++-- src/node/services/workspaceService.test.ts | 95 ++++++++++++++++++++++ src/node/services/workspaceService.ts | 73 ++++++++++++----- 9 files changed, 285 insertions(+), 47 deletions(-) diff --git a/src/common/orpc/schemas/stream.ts b/src/common/orpc/schemas/stream.ts index a5d84a2e4e1..03c3354a1ec 100644 --- a/src/common/orpc/schemas/stream.ts +++ b/src/common/orpc/schemas/stream.ts @@ -1,5 +1,5 @@ import { z } from "zod"; -import { AgentIdSchema } from "./agentDefinition"; +import { AgentDefinitionScopeSchema, AgentIdSchema } from "./agentDefinition"; import { OpenAIReasoningModeSchema, ThinkingLevelSchema } from "../../types/thinking"; import { AgentModeSchema } from "../../types/mode"; import { ChatUsageDisplaySchema } from "./chatStats"; @@ -801,14 +801,21 @@ export const SendMessageOptionsSchema = z.object({ */ disableWorkspaceAgents: z.boolean().optional(), /** - * When true, a top-level send whose agentId cannot be resolved (or is disabled) - * at stream time fails loudly instead of silently falling back to exec. + * When truthy, a top-level send whose agentId cannot be resolved (or is hidden or + * disabled) at stream time fails loudly instead of silently falling back to exec. * Workspace-turn launches with explicit agent overrides set this: pre-dispatch * validation races init hooks and user edits, so stream-time resolution — which * runs after initialization completes — is the last sound gate against running - * a different agent than the caller asked for. + * a different agent than the caller asked for. The object form additionally pins + * the validated definition's provenance: if the id resolves from a different + * scope than launch validation saw (e.g. a validated project shadow vanished and + * a global/built-in definition with the same id took over), the send fails + * instead of running a different prompt/tool policy. A single field (rather than + * a sibling flag) so every option-preservation path copies it verbatim. */ - strictAgentResolution: z.boolean().optional(), + strictAgentResolution: z + .union([z.boolean(), z.object({ expectedScope: AgentDefinitionScopeSchema })]) + .optional(), /** * Desktop/app-only capability: expose set_goal so an agent can create a * continuation-backed goal for its current parent workspace. Headless callers diff --git a/src/node/services/agentResolution.test.ts b/src/node/services/agentResolution.test.ts index 367eff092f0..46b0c5f1eb3 100644 --- a/src/node/services/agentResolution.test.ts +++ b/src/node/services/agentResolution.test.ts @@ -486,7 +486,7 @@ describe("resolveAgentForStream strict resolution", () => { async function resolveTopLevel(params: { projectPath: string; agentId: string; - strictAgentResolution: boolean; + strictAgentResolution: boolean | { expectedScope: "project" | "global" | "built-in" }; agentAiDefaults?: ProjectsConfig["agentAiDefaults"]; }) { const cfg: ProjectsConfig = { @@ -609,6 +609,36 @@ describe("resolveAgentForStream strict resolution", () => { } }); + test("strict mode rejects a definition resolving from a different scope than validated", async () => { + using tempDir = new DisposableTempDir("agent-resolution-strict-provenance"); + const projectPath = path.join(tempDir.path, "project"); + await fs.mkdir(projectPath, { recursive: true }); + + // Launch validation saw a project shadow that has since vanished (init hook or + // concurrent edit): the same id now resolves from the built-in scope. The strict + // provenance pin must fail the send instead of running the different definition. + const strict = await resolveTopLevel({ + projectPath, + agentId: "plan", + strictAgentResolution: { expectedScope: "project" }, + }); + expect(strict.success).toBe(false); + if (!strict.success && strict.error.type === "unknown") { + expect(strict.error.raw).toContain("different scope"); + } else { + expect(strict.success === false && strict.error.type).toBe("unknown"); + } + + // A matching scope streams normally. + const matching = await resolveTopLevel({ + projectPath, + agentId: "plan", + strictAgentResolution: { expectedScope: "built-in" }, + }); + expect(matching.success).toBe(true); + if (matching.success) expect(matching.data.effectiveAgentId).toBe("plan"); + }); + test("strict mode fails closed when eligibility resolution throws", async () => { using tempDir = new DisposableTempDir("agent-resolution-strict-broken-base"); const projectPath = path.join(tempDir.path, "project"); diff --git a/src/node/services/agentResolution.ts b/src/node/services/agentResolution.ts index 9c5e8aafcd8..abd94e87abc 100644 --- a/src/node/services/agentResolution.ts +++ b/src/node/services/agentResolution.ts @@ -14,6 +14,7 @@ import { resolveAdvisorEnabledForAgent } from "@/common/constants/advisor"; import { AgentIdSchema } from "@/common/orpc/schemas"; import type { SendMessageError } from "@/common/types/errors"; +import type { SendMessageOptions } from "@/common/orpc/types"; import type { Result } from "@/common/types/result"; import { Err, Ok } from "@/common/types/result"; import type { ErrorEvent } from "@/common/types/stream"; @@ -51,14 +52,16 @@ export interface ResolveAgentOptions { /** When true, skip workspace-specific agents (for "unbricking" broken agent files). */ disableWorkspaceAgents: boolean; /** - * When true, a top-level requested agent that cannot be resolved (or is - * disabled) fails the stream loudly instead of silently falling back to exec. - * Set by workspace-turn launches with explicit agent overrides: their + * When truthy, a top-level requested agent that cannot be resolved (or is + * hidden or disabled) fails the stream loudly instead of silently falling back + * to exec. Set by workspace-turn launches with explicit agent overrides: their * pre-dispatch validation races init hooks and user edits, so this post-init * resolution is the last sound gate against running a different agent than - * the caller asked for. Sub-agent workspaces already fail loudly. + * the caller asked for. The object form additionally pins the validated + * definition's provenance (scope) — see SendMessageOptionsSchema. Sub-agent + * workspaces already fail loudly. */ - strictAgentResolution?: boolean; + strictAgentResolution?: SendMessageOptions["strictAgentResolution"]; /** Caller-supplied tool policy (applied AFTER agent policy for further restriction). */ callerToolPolicy: ToolPolicy | undefined; /** Loaded config from Config.loadConfigOrDefault(). */ @@ -234,7 +237,10 @@ export async function resolveAgentForStream( const isSubagentWorkspace = Boolean(metadata.parentWorkspaceId); // Strict explicit-agent gating applies only to top-level sends: sub-agent workspaces // already fail loudly and legitimately run hidden agents (explore, compact). - const strictTopLevel = strictAgentResolution === true && !isSubagentWorkspace; + const strictTopLevel = + strictAgentResolution != null && strictAgentResolution !== false && !isSubagentWorkspace; + const strictExpectedScope = + typeof strictAgentResolution === "object" ? strictAgentResolution.expectedScope : undefined; // --- Load agent definition (with fallback to exec) --- let agentDefinition: Awaited> | undefined; @@ -300,6 +306,27 @@ export async function resolveAgentForStream( }); } + // Strict provenance pin: launch validation approved a specific definition, not just + // an id. If that definition vanished (e.g. an init hook or edit deleted a validated + // project shadow) and a lower-priority scope now resolves the same id, the turn + // would run a different prompt/tool policy with AI settings derived from the + // validated one — fail instead. + if ( + strictTopLevel && + strictExpectedScope != null && + agentDefinition.scope !== strictExpectedScope + ) { + const errorMessage = `Agent '${requestedAgentId}' now resolves from a different scope than launch validation saw (expected ${strictExpectedScope}, found ${agentDefinition.scope}); refusing to stream an explicit agent request.`; + emitError( + createErrorEvent(workspaceId, { + messageId: createAssistantMessageId(), + error: errorMessage, + errorType: "unknown", + }) + ); + return Err({ type: "unknown", raw: errorMessage }); + } + // Keep agent ID aligned with the actual definition used (may fall back to exec). effectiveAgentId = agentDefinition.id; diff --git a/src/node/services/agentSession.ts b/src/node/services/agentSession.ts index 52027e1bee0..27b8068ed35 100644 --- a/src/node/services/agentSession.ts +++ b/src/node/services/agentSession.ts @@ -1902,8 +1902,10 @@ export class AgentSession { } // Explicit-agent delegated turns must stay loud across restart recovery: without // this, a replay after the agent was removed/disabled would silently run exec. - if (persistedRetrySendOptions?.strictAgentResolution === true) { - retryRequest.strictAgentResolution = true; + // Copied verbatim so the object form keeps its provenance pin (expectedScope). + const persistedStrictAgentResolution = persistedRetrySendOptions?.strictAgentResolution; + if (persistedStrictAgentResolution != null && persistedStrictAgentResolution !== false) { + retryRequest.strictAgentResolution = persistedStrictAgentResolution; } if (persistedRetrySendOptions?.agentInitiated === true) { diff --git a/src/node/services/aiService.ts b/src/node/services/aiService.ts index f4b99c59774..444212a1038 100644 --- a/src/node/services/aiService.ts +++ b/src/node/services/aiService.ts @@ -294,7 +294,7 @@ export interface StreamMessageOptions { agentInitiated?: boolean; agentId?: string; /** See SendMessageOptionsSchema.strictAgentResolution: explicit-agent sends fail loudly instead of falling back to exec. */ - strictAgentResolution?: boolean; + strictAgentResolution?: SendMessageOptions["strictAgentResolution"]; /** ACP prompt correlation id used to match stream events to a specific request. */ acpPromptId?: string; /** Tool names that should be delegated back to ACP clients for this request. */ @@ -1955,7 +1955,7 @@ export class AIService extends EventEmitter { runtime, workspacePath, requestedAgentId: agentId, - strictAgentResolution: strictAgentResolution ?? false, + strictAgentResolution, disableWorkspaceAgents: disableWorkspaceAgents ?? false, callerToolPolicy: toolPolicy, cfg, diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index fd4fffbd6be..92ecf8fdd99 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -1005,10 +1005,14 @@ describe("TaskService", () => { expect(sendMessage).toHaveBeenCalledTimes(1); const sendMessageCall = sendMessage.mock.calls[0] as unknown[]; expect(sendMessageCall[0]).toBe("childworkspace"); - // Explicit overrides also arm stream-time strict resolution: pre-dispatch validation - // races init hooks/user edits, so the stream must fail loudly instead of silently - // swapping in exec if the agent cannot be resolved post-init. - expect(sendMessageCall[2]).toMatchObject({ agentId: "plan", strictAgentResolution: true }); + // Explicit overrides also arm stream-time strict resolution, pinning the validated + // definition's provenance: pre-dispatch validation races init hooks/user edits, so + // the stream must fail loudly instead of silently swapping in exec (or running a + // different-scope definition for the same id) post-init. + expect(sendMessageCall[2]).toMatchObject({ + agentId: "plan", + strictAgentResolution: { expectedScope: "built-in" }, + }); }); test("createWorkspaceTurn rejects invalid, unknown, and internal agent ids before creating a workspace", async () => { diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 3fdc6197e0c..77b5bf16574 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -91,6 +91,7 @@ import { import type { ProjectRef, WorkspaceMetadata } from "@/common/types/workspace"; import { getRuntimeType } from "@/node/runtime/initHook"; import { AgentIdSchema } from "@/common/orpc/schemas"; +import type { AgentDefinitionScope } from "@/common/types/agentDefinition"; import { normalizeAgentId, resolvePersistedAgentId, @@ -3695,8 +3696,28 @@ export class TaskService { runtime: Runtime; workspacePath: string; includeAgentPlugins: boolean; - }): Promise> { + }): Promise> { assert(params.agentId.length > 0, "validateWorkspaceTurnAgentId: agentId must be non-empty"); + const parsedAgentId = AgentIdSchema.safeParse(params.agentId); + if (!parsedAgentId.success) { + return Err(`Task.createWorkspaceTurn: invalid agentId (${params.agentId})`); + } + // The winning definition's scope is captured so the dispatched turn can pin the + // validated provenance at stream time (strictAgentResolution.expectedScope): a + // validated shadow vanishing must not let a lower-priority definition with the + // same id run under a strict send. + let scope: AgentDefinitionScope; + try { + const definition = await readAgentDefinition( + params.runtime, + params.workspacePath, + parsedAgentId.data, + { includeAgentPlugins: params.includeAgentPlugins } + ); + scope = definition.scope; + } catch { + return Err(`Task.createWorkspaceTurn: unknown agentId (${params.agentId})`); + } let frontmatter: Awaited>; try { frontmatter = await resolveAgentFrontmatter( @@ -3722,7 +3743,7 @@ export class TaskService { ) { return Err(`Task.createWorkspaceTurn: agentId is disabled (${params.agentId})`); } - return Ok(undefined); + return Ok({ scope }); } /** @@ -3766,7 +3787,9 @@ export class TaskService { * owner-side resolution. */ ownerResolutionPredictsTarget: boolean; - }): Promise> { + }): Promise< + Result<{ validatedContext: WorkspaceTurnAgentContext; scope: AgentDefinitionScope }, string> + > { const reachable = await runtimePathExists(params.target.runtime, params.target.workspacePath); if (reachable) { const validation = await this.validateWorkspaceTurnAgentId({ @@ -3775,7 +3798,7 @@ export class TaskService { ...params.target, }); if (validation.success) { - return Ok({ validatedContext: params.target }); + return Ok({ validatedContext: params.target, scope: validation.data.scope }); } if (params.targetInitPending) { return Err( @@ -3798,7 +3821,7 @@ export class TaskService { if (!parsedAgentId.success) { return Err(`Task.createWorkspaceTurn: invalid agentId (${params.agentId})`); } - let resolvedScope: string; + let resolvedScope: AgentDefinitionScope; try { const definition = await readAgentDefinition( params.owner.runtime, @@ -3820,7 +3843,9 @@ export class TaskService { agentId: params.agentId, ...params.owner, }); - return validation.success ? Ok({ validatedContext: params.owner }) : validation; + return validation.success + ? Ok({ validatedContext: params.owner, scope: validation.data.scope }) + : validation; } async createWorkspaceTurn( @@ -3907,6 +3932,10 @@ export class TaskService { // agent-authored frontmatter `ai` defaults participate in AI-settings resolution // (mirrors the sub-agent creation path). Left unset for default exec/resume turns. let agentDefinitionContext: NodeAgentDefinitionContext | undefined; + // Scope of the definition that authorized the dispatch; pinned at stream time via + // strictAgentResolution.expectedScope so a vanished shadow cannot silently hand the + // id to a lower-priority definition. + let validatedAgentScope: AgentDefinitionScope | undefined; // Post-create target validation failure. Deferred (not returned immediately) so the // workspace-turn record is still written first: the record marks the created workspace // as owner-owned and retryable via mode="existing", and the failure settles through the @@ -4007,6 +4036,7 @@ export class TaskService { }); if (!validation.success) return Err(validation.error); workspaceTurnAgentId = requestedAgentId; + validatedAgentScope = validation.data.scope; agentDefinitionContext = { ...validation.data.validatedContext, workspaceId: targetWorkspaceId, @@ -4139,6 +4169,7 @@ export class TaskService { ? `${validation.error} — no turn was dispatched; automatic cleanup of the disposable workspace (${targetWorkspaceId}) was scheduled (if cleanup fails, it remains owned by this caller and retryable via workspace.mode="existing")` : `${validation.error} — no turn was dispatched; the created workspace (${targetWorkspaceId}) is owned by this caller and can be retried via workspace.mode="existing" once ready`; } else { + validatedAgentScope = validation.data.scope; agentDefinitionContext = { ...validation.data.validatedContext, workspaceId: targetWorkspaceId, @@ -4320,8 +4351,15 @@ export class TaskService { : {}), // Explicit overrides were validated pre-dispatch, but that validation races init // hooks and later edits; stream-time resolution runs after initialization and must - // fail loudly rather than silently swap in exec (see strictAgentResolution docs). - ...(requestedAgentId != null ? { strictAgentResolution: true } : {}), + // fail loudly rather than silently swap in exec — and, when the validated scope is + // known, must not run a different-provenance definition for the same id (see + // strictAgentResolution docs). + ...(requestedAgentId != null + ? { + strictAgentResolution: + validatedAgentScope != null ? { expectedScope: validatedAgentScope } : true, + } + : {}), }, { startStreamInBackground: true, diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index 9a288927e77..50ab5972387 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -14678,6 +14678,101 @@ describe("WorkspaceService.getGoalContinuationRuntimeState", () => { expect(options?.muxMetadata).toBeUndefined(); }); + test("recovers options from a wake row after on-send compaction hid the delegated row", async () => { + const workspaceId = "ws-delegated-post-compaction"; + const { service, historyService } = await makeServiceWithHistory(); + // On-send compaction consumed a wake continuation: the original delegated row is + // behind the boundary; the compaction summary proves the turn is still open and + // the follow-up wake-typed row is the remaining carrier of the turn's options. + await historyService.appendToHistory( + workspaceId, + createMuxMessage("summary-1", "assistant", "Summary", { + timestamp: Date.now(), + muxMetadata: { + type: "compaction-summary" as const, + pendingFollowUp: { + text: "Continue", + model: "anthropic:claude-opus-4-6", + agentId: "plan", + workspaceTurnMetadata: delegatedTurnCorrelation, + }, + }, + }) + ); + await historyService.appendToHistory( + workspaceId, + createMuxMessage("wake-followup", "user", "Monitor matched", { + timestamp: Date.now(), + muxMetadata: { type: "bash-monitor-wake" as const, records: [] }, + retrySendOptions: { + model: "anthropic:claude-opus-4-6", + agentId: "plan", + strictAgentResolution: { expectedScope: "built-in" }, + }, + }) + ); + + const internals = service as unknown as DelegatedContinuationInternals; + const options = await internals.getDelegatedTurnContinuationSendOptions(workspaceId); + expect(options).toMatchObject({ + model: "anthropic:claude-opus-4-6", + agentId: "plan", + strictAgentResolution: { expectedScope: "built-in" }, + skipAiSettingsPersistence: true, + }); + }); + + test("sanitizes persisted options through the canonical whitelist", async () => { + const workspaceId = "ws-delegated-sanitized"; + const { service, historyService } = await makeServiceWithHistory(); + const tamperedRetrySendOptions: Record = { + model: "anthropic:claude-opus-4-6", + agentId: "plan", + editMessageId: "innocent-message", + muxMetadata: { type: "workspace-turn-task" }, + }; + const malformedRetrySendOptions: Record = { agentId: "plan" }; // model missing + await historyService.appendToHistory( + workspaceId, + createMuxMessage("delegated-tampered", "user", "Delegated prompt", { + timestamp: Date.now(), + muxMetadata: delegatedTurnCorrelation, + // History is untrusted at rest: injected fields outside the whitelist + // (editMessageId would flip the send into the edit/truncation flow) must + // never reach the internal continuation send. + retrySendOptions: tamperedRetrySendOptions as never, + }) + ); + await historyService.appendToHistory( + workspaceId, + delegatedAssistantMessage("assistant-cut-3", "tool-calls") + ); + + const internals = service as unknown as DelegatedContinuationInternals; + const options = await internals.getDelegatedTurnContinuationSendOptions(workspaceId); + expect(options).toMatchObject({ agentId: "plan", skipAiSettingsPersistence: true }); + expect(options && "editMessageId" in options && options.editMessageId).toBeFalsy(); + expect(options?.muxMetadata).toBeUndefined(); + + // A row whose options fail schema validation entirely yields nothing. + const malformedWorkspaceId = "ws-delegated-malformed"; + await historyService.appendToHistory( + malformedWorkspaceId, + createMuxMessage("delegated-malformed", "user", "Delegated prompt", { + timestamp: Date.now(), + muxMetadata: delegatedTurnCorrelation, + retrySendOptions: malformedRetrySendOptions as never, + }) + ); + await historyService.appendToHistory( + malformedWorkspaceId, + delegatedAssistantMessage("assistant-cut-4", "tool-calls") + ); + expect( + await internals.getDelegatedTurnContinuationSendOptions(malformedWorkspaceId) + ).toBeNull(); + }); + test("yields nothing after a terminal assistant response closed the delegated turn", async () => { const workspaceId = "ws-delegated-closed"; const { service, historyService } = await makeServiceWithHistory(); diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index 723279147af..9511c7be727 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -258,6 +258,7 @@ import type { WorkspaceGoalDefaultsOverrideSchema, WorkspaceHeartbeatSettingsSchema, } from "@/common/orpc/schemas"; +import { SendMessageOptionsSchema } from "@/common/orpc/schemas"; import type { ArchiveLossyUntrackedFilesConfirmation, ArchivePreflightResult, @@ -1757,6 +1758,29 @@ function extractUserPromptText(message: MuxMessage): string { return stripStagedAttachmentNotice(partsText).trim(); } +/** + * Canonical whitelist for options replayed from a persisted delegated-turn row + * (see getDelegatedTurnContinuationSendOptions). History metadata stores + * retrySendOptions as an untyped blob, so a malformed or tampered row must be + * rejected (parse failure) or stripped to exactly these fields — never spread + * verbatim into an internal send where extras like editMessageId would trigger + * the edit/truncation flow. + */ +const DELEGATED_TURN_CONTINUATION_OPTIONS_SCHEMA = SendMessageOptionsSchema.pick({ + model: true, + agentId: true, + thinkingLevel: true, + reasoningMode: true, + toolPolicy: true, + additionalSystemInstructions: true, + maxOutputTokens: true, + providerOptions: true, + experiments: true, + disableWorkspaceAgents: true, + strictAgentResolution: true, + allowAgentSetGoal: true, +}); + // eslint-disable-next-line @typescript-eslint/no-unsafe-declaration-merging export class WorkspaceService extends EventEmitter { private readonly sessions = new Map(); @@ -12191,7 +12215,16 @@ export class WorkspaceService extends EventEmitter { * response closed the turn (or any other user send took over the conversation), * a late monitor match is a NEW synthetic turn and resolves from the target's * persisted defaults instead of resurrecting stale per-turn overrides. - * Continuations never persist these options as workspace defaults. + * + * Carrier rows for the open turn's options, newest first: the correlated + * workspace-turn user row itself, and this mechanism's own wake continuations + * (their sends were dispatched with the delegated options and re-stamped them) — + * after an on-send compaction consumed a wake, the follow-up wake-typed row is + * the only carrier left inside the boundary while the summary still proves the + * turn is open. Persisted options are rebuilt through a canonical schema + * whitelist (history is untrusted at rest; a tampered row must not inject fields + * like editMessageId into an internal send). Continuations never persist these + * options as workspace defaults. */ private async getDelegatedTurnContinuationSendOptions( workspaceId: string @@ -12215,28 +12248,30 @@ export class WorkspaceService extends EventEmitter { continue; } const muxMetadata = message.metadata?.muxMetadata; - if ( - muxMetadata?.type !== "workspace-turn-task" || - muxMetadata.taskHandleId !== openTurn.taskHandleId || - muxMetadata.turnId !== openTurn.turnId - ) { + const isOpenTurnRow = + muxMetadata?.type === "workspace-turn-task" && + muxMetadata.taskHandleId === openTurn.taskHandleId && + muxMetadata.turnId === openTurn.turnId; + const isWakeContinuationRow = muxMetadata?.type === "bash-monitor-wake"; + if (!isOpenTurnRow && !isWakeContinuationRow) { continue; } - const retrySendOptions = message.metadata?.retrySendOptions; - if (retrySendOptions == null) { + const parsed = DELEGATED_TURN_CONTINUATION_OPTIONS_SCHEMA.safeParse( + message.metadata?.retrySendOptions + ); + if (parsed.success) { + return { + ...parsed.data, + // Per-turn continuation settings must not become workspace defaults. + skipAiSettingsPersistence: true, + }; + } + if (isOpenTurnRow) { + // The anchor row itself has no usable options; nothing older can be more + // authoritative for this turn. return null; } - const { - muxMetadata: _turnCorrelation, - agentInitiated: _agentInitiated, - goalKind: _goalKind, - ...sendOptions - } = retrySendOptions; - return { - ...sendOptions, - // Per-turn continuation settings must not become workspace defaults. - skipAiSettingsPersistence: true, - }; + // A wake row without valid options: keep walking toward the anchor row. } return null; } From ae058f64a945e8ce83eb2d9a0e62bab142ededb3 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 15:46:32 +0000 Subject: [PATCH 19/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20pin=20exact=20defin?= =?UTF-8?q?ition=20source;=20require=20clean=20agent=20dirs=20for=20owner?= =?UTF-8?q?=20vouching;=20validate=20persisted=20strictness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/common/orpc/schemas/agentDefinition.ts | 9 ++ src/common/orpc/schemas/stream.ts | 14 ++- .../agentDefinitionsService.ts | 5 +- src/node/services/agentResolution.test.ts | 27 +++++- src/node/services/agentResolution.ts | 38 ++++---- src/node/services/agentSession.ts | 21 +++-- src/node/services/taskService.test.ts | 67 ++++++++++++++- src/node/services/taskService.ts | 86 +++++++++++++++---- src/node/services/taskUtils.ts | 30 +++++++ 9 files changed, 248 insertions(+), 49 deletions(-) diff --git a/src/common/orpc/schemas/agentDefinition.ts b/src/common/orpc/schemas/agentDefinition.ts index 06f1b3a81af..7c0037a42ac 100644 --- a/src/common/orpc/schemas/agentDefinition.ts +++ b/src/common/orpc/schemas/agentDefinition.ts @@ -114,5 +114,14 @@ export const AgentDefinitionPackageSchema = z scope: AgentDefinitionScopeSchema, frontmatter: AgentDefinitionFrontmatterSchema, body: z.string(), + /** + * Exact source identity of the winning candidate: "built-in" for embedded + * definitions, otherwise the discovery root the file was read from (per-plugin + * agents dirs are unique per plugin). Scope alone is not a provenance + * identifier — project files and project plugins both report "project" — so + * strict explicit-agent sends pin this to detect a different definition + * taking over the same id between launch validation and streaming. + */ + source: z.string().optional(), }) .strict(); diff --git a/src/common/orpc/schemas/stream.ts b/src/common/orpc/schemas/stream.ts index 03c3354a1ec..1c2a067f6ee 100644 --- a/src/common/orpc/schemas/stream.ts +++ b/src/common/orpc/schemas/stream.ts @@ -814,7 +814,19 @@ export const SendMessageOptionsSchema = z.object({ * a sibling flag) so every option-preservation path copies it verbatim. */ strictAgentResolution: z - .union([z.boolean(), z.object({ expectedScope: AgentDefinitionScopeSchema })]) + .union([ + z.boolean(), + z.object({ + expectedScope: AgentDefinitionScopeSchema, + /** + * Exact source identity from AgentDefinitionPackage.source ("built-in" or the + * discovery root). Scope alone collapses distinct candidates (project files + * and project plugins both report "project"), so this pins the definition + * itself when known. + */ + expectedSource: z.string().optional(), + }), + ]) .optional(), /** * Desktop/app-only capability: expose set_goal so an agent can create a diff --git a/src/node/services/agentDefinitions/agentDefinitionsService.ts b/src/node/services/agentDefinitions/agentDefinitionsService.ts index 80254fd435e..039c4064ed8 100644 --- a/src/node/services/agentDefinitions/agentDefinitionsService.ts +++ b/src/node/services/agentDefinitions/agentDefinitionsService.ts @@ -659,6 +659,9 @@ export async function readAgentDefinition( scope: candidate.scope, frontmatter: parsed.frontmatter, body: parsed.body, + // Exact provenance for strict-send pinning; per-plugin candidate roots are + // unique per plugin, so root identity distinguishes file vs plugin sources. + source: candidate.root, }; const validated = AgentDefinitionPackageSchema.safeParse(pkg); @@ -677,7 +680,7 @@ export async function readAgentDefinition( if (!skipScopes.has("built-in")) { const builtIn = getBuiltInAgentDefinitions().find((pkg) => pkg.id === agentId); if (builtIn) { - const validated = AgentDefinitionPackageSchema.safeParse(builtIn); + const validated = AgentDefinitionPackageSchema.safeParse({ ...builtIn, source: "built-in" }); if (!validated.success) { throw new Error( `Invalid built-in agent definition '${agentId}': ${validated.error.message}` diff --git a/src/node/services/agentResolution.test.ts b/src/node/services/agentResolution.test.ts index 46b0c5f1eb3..37832105f86 100644 --- a/src/node/services/agentResolution.test.ts +++ b/src/node/services/agentResolution.test.ts @@ -486,7 +486,9 @@ describe("resolveAgentForStream strict resolution", () => { async function resolveTopLevel(params: { projectPath: string; agentId: string; - strictAgentResolution: boolean | { expectedScope: "project" | "global" | "built-in" }; + strictAgentResolution: + | boolean + | { expectedScope: "project" | "global" | "built-in"; expectedSource?: string }; agentAiDefaults?: ProjectsConfig["agentAiDefaults"]; }) { const cfg: ProjectsConfig = { @@ -624,16 +626,33 @@ describe("resolveAgentForStream strict resolution", () => { }); expect(strict.success).toBe(false); if (!strict.success && strict.error.type === "unknown") { - expect(strict.error.raw).toContain("different scope"); + expect(strict.error.raw).toContain("different definition"); } else { expect(strict.success === false && strict.error.type).toBe("unknown"); } - // A matching scope streams normally. + // Scope alone is not a provenance identifier (project files and project plugins + // both report "project"): a same-scope pin with a different exact source must + // also fail. + const sameScopeDifferentSource = await resolveTopLevel({ + projectPath, + agentId: "plan", + strictAgentResolution: { expectedScope: "built-in", expectedSource: ".xum/agents" }, + }); + expect(sameScopeDifferentSource.success).toBe(false); + if (!sameScopeDifferentSource.success && sameScopeDifferentSource.error.type === "unknown") { + expect(sameScopeDifferentSource.error.raw).toContain("different definition"); + } else { + expect( + sameScopeDifferentSource.success === false && sameScopeDifferentSource.error.type + ).toBe("unknown"); + } + + // A matching scope + source streams normally. const matching = await resolveTopLevel({ projectPath, agentId: "plan", - strictAgentResolution: { expectedScope: "built-in" }, + strictAgentResolution: { expectedScope: "built-in", expectedSource: "built-in" }, }); expect(matching.success).toBe(true); if (matching.success) expect(matching.data.effectiveAgentId).toBe("plan"); diff --git a/src/node/services/agentResolution.ts b/src/node/services/agentResolution.ts index abd94e87abc..49677912f91 100644 --- a/src/node/services/agentResolution.ts +++ b/src/node/services/agentResolution.ts @@ -308,23 +308,27 @@ export async function resolveAgentForStream( // Strict provenance pin: launch validation approved a specific definition, not just // an id. If that definition vanished (e.g. an init hook or edit deleted a validated - // project shadow) and a lower-priority scope now resolves the same id, the turn - // would run a different prompt/tool policy with AI settings derived from the - // validated one — fail instead. - if ( - strictTopLevel && - strictExpectedScope != null && - agentDefinition.scope !== strictExpectedScope - ) { - const errorMessage = `Agent '${requestedAgentId}' now resolves from a different scope than launch validation saw (expected ${strictExpectedScope}, found ${agentDefinition.scope}); refusing to stream an explicit agent request.`; - emitError( - createErrorEvent(workspaceId, { - messageId: createAssistantMessageId(), - error: errorMessage, - errorType: "unknown", - }) - ); - return Err({ type: "unknown", raw: errorMessage }); + // project shadow) and a different candidate now resolves the same id — a lower + // scope, or a same-scope sibling like a project plugin taking over for a removed + // project file — the turn would run a different prompt/tool policy with AI settings + // derived from the validated one. Scope alone is not a provenance identifier, so + // the exact source (discovery root / "built-in") is compared when pinned. + if (strictTopLevel && strictExpectedScope != null) { + const expectedSource = + typeof strictAgentResolution === "object" ? strictAgentResolution.expectedSource : undefined; + const scopeMismatch = agentDefinition.scope !== strictExpectedScope; + const sourceMismatch = expectedSource != null && agentDefinition.source !== expectedSource; + if (scopeMismatch || sourceMismatch) { + const errorMessage = `Agent '${requestedAgentId}' now resolves from a different definition than launch validation saw (expected ${strictExpectedScope}${expectedSource != null ? ` @ ${expectedSource}` : ""}, found ${agentDefinition.scope}${agentDefinition.source != null ? ` @ ${agentDefinition.source}` : ""}); refusing to stream an explicit agent request.`; + emitError( + createErrorEvent(workspaceId, { + messageId: createAssistantMessageId(), + error: errorMessage, + errorType: "unknown", + }) + ); + return Err({ type: "unknown", raw: errorMessage }); + } } // Keep agent ID aligned with the actual definition used (may fall back to exec). diff --git a/src/node/services/agentSession.ts b/src/node/services/agentSession.ts index 27b8068ed35..956cbc4b064 100644 --- a/src/node/services/agentSession.ts +++ b/src/node/services/agentSession.ts @@ -37,7 +37,7 @@ import { type GoalSyntheticMessageKind, } from "@/constants/goals"; import type { SendMessageError } from "@/common/types/errors"; -import { AgentIdSchema, SkillNameSchema } from "@/common/orpc/schemas"; +import { AgentIdSchema, SendMessageOptionsSchema, SkillNameSchema } from "@/common/orpc/schemas"; import { normalizeAgentId, resolvePersistedAgentIdCandidates } from "@/common/utils/agentIds"; import { buildStreamErrorEventData, @@ -1902,10 +1902,21 @@ export class AgentSession { } // Explicit-agent delegated turns must stay loud across restart recovery: without // this, a replay after the agent was removed/disabled would silently run exec. - // Copied verbatim so the object form keeps its provenance pin (expectedScope). - const persistedStrictAgentResolution = persistedRetrySendOptions?.strictAgentResolution; - if (persistedStrictAgentResolution != null && persistedStrictAgentResolution !== false) { - retryRequest.strictAgentResolution = persistedStrictAgentResolution; + // History stores retrySendOptions as an untyped blob, so the persisted value is + // re-validated against the canonical schema first — a malformed provenance pin + // must be discarded (lenient replay) rather than failing every recovered attempt + // with a false mismatch. A valid pin is copied verbatim (keeps expectedScope/ + // expectedSource). + const persistedStrictAgentResolution = + SendMessageOptionsSchema.shape.strictAgentResolution.safeParse( + persistedRetrySendOptions?.strictAgentResolution + ); + if ( + persistedStrictAgentResolution.success && + persistedStrictAgentResolution.data != null && + persistedStrictAgentResolution.data !== false + ) { + retryRequest.strictAgentResolution = persistedStrictAgentResolution.data; } if (persistedRetrySendOptions?.agentInitiated === true) { diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index 92ecf8fdd99..818af8cf8d4 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -107,6 +107,15 @@ function checkoutOwnerBranch(projectPath: string, branch: string): void { execSync(`git checkout -b ${branch}`, { cwd: projectPath, stdio: "ignore" }); } +/** + * Commit pending agent-definition files: owner-side vouching additionally requires the + * agent-definition paths to be clean (uncommitted changes diverge from the committed + * base a new checkout is created from). + */ +function commitOwnerAgentFiles(projectPath: string): void { + execSync("git add -A && git commit -q -m agents", { cwd: projectPath, stdio: "ignore" }); +} + async function collectFullHistory(service: HistoryService, workspaceId: string) { const messages: MuxMessage[] = []; const result = await service.iterateFullHistory(workspaceId, "forward", (chunk) => { @@ -1006,15 +1015,62 @@ describe("TaskService", () => { const sendMessageCall = sendMessage.mock.calls[0] as unknown[]; expect(sendMessageCall[0]).toBe("childworkspace"); // Explicit overrides also arm stream-time strict resolution, pinning the validated - // definition's provenance: pre-dispatch validation races init hooks/user edits, so - // the stream must fail loudly instead of silently swapping in exec (or running a - // different-scope definition for the same id) post-init. + // definition's provenance (scope + exact source): pre-dispatch validation races + // init hooks/user edits, so the stream must fail loudly instead of silently + // swapping in exec (or running a different definition for the same id) post-init. expect(sendMessageCall[2]).toMatchObject({ agentId: "plan", - strictAgentResolution: { expectedScope: "built-in" }, + strictAgentResolution: { expectedScope: "built-in", expectedSource: "built-in" }, }); }); + test("createWorkspaceTurn keeps prechecks advisory when the owner has uncommitted agent changes", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + checkoutOwnerBranch(projectPath, "parent"); + // An UNCOMMITTED hidden shadow of the built-in plan exists only in the owner's + // working tree: the new checkout is created from committed branch state and + // validly resolves the built-in, so the owner-side miss must stay advisory + // (branch equality is not checkout equality). + const agentsDir = path.join(projectPath, ".mux", "agents"); + await fsPromises.mkdir(agentsDir, { recursive: true }); + await fsPromises.writeFile( + path.join(agentsDir, "plan.md"), + ["---", "name: Plan", "base: plan", "ui:", " hidden: true", "---", "Shadow."].join("\n") + ); + + const cleanCheckout = path.join(rootDir, "clean-target-checkout"); + await fsPromises.mkdir(cleanCheckout, { recursive: true }); + const targetMetadata: WorkspaceMetadata & { namedWorkspacePath: string } = { + ...createWorkspaceTurnMetadata(projectPath), + runtimeConfig: { type: "worktree", srcBaseDir: path.join(rootDir, "wt") }, + namedWorkspacePath: cleanCheckout, + }; + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: targetMetadata })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "plan", + prompt: "Plan from the committed base", + title: "Dirty owner shadow", + workspace: { mode: "new" }, + }); + + expect(result.success).toBe(true); + expect(sendMessage).toHaveBeenCalledTimes(1); + const sendMessageCall = sendMessage.mock.calls[0] as unknown[]; + expect(sendMessageCall[2]).toMatchObject({ agentId: "plan" }); + }); + test("createWorkspaceTurn rejects invalid, unknown, and internal agent ids before creating a workspace", async () => { const config = await createTestConfig(rootDir); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); @@ -1063,6 +1119,7 @@ describe("TaskService", () => { }); checkoutOwnerBranch(projectPath, "parent"); await writeCustomAgentDefinition(projectPath); + commitOwnerAgentFiles(projectPath); const createWorkspace = mock( (): Promise> => @@ -1183,6 +1240,7 @@ describe("TaskService", () => { ].join("\n"), "utf-8" ); + commitOwnerAgentFiles(projectPath); const createWorkspace = mock( (): Promise> => @@ -1306,6 +1364,7 @@ describe("TaskService", () => { const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); checkoutOwnerBranch(projectPath, "parent"); await writeCustomAgentDefinition(projectPath); + commitOwnerAgentFiles(projectPath); // Deferred-provisioning runtimes return from create before the checkout is reachable. const unreachableCheckout = path.join(rootDir, "not-provisioned-yet"); diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 77b5bf16574..d4625a15beb 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -60,8 +60,10 @@ import { coerceNonEmptyString, tryReadGitCurrentBranch, tryReadGitHeadCommitSha, + tryReadGitPathsClean, findWorkspaceEntry, } from "@/node/services/taskUtils"; +import { listProjectMetadataRelativePaths } from "@/common/compat/legacyMux"; import { validateWorkspaceName } from "@/common/utils/validation/workspaceValidation"; import { getTaskGroupCount } from "@/common/utils/tools/taskGroups"; import { stripTrailingSlashes } from "@/node/utils/pathUtils"; @@ -524,6 +526,19 @@ function isAgentRunnableAsChild( type WorkspaceTurnQueueDispatchMode = "tool-end" | "turn-end"; +/** + * Project-relative paths that contribute agent definitions to discovery + * (project agent roots, project plugin containers). Owner-side vouching for a + * freshly created checkout requires these to be clean in the owner: the child is + * created from committed branch state, so uncommitted changes here make + * owner-side agent resolution diverge from what the target will see. + */ +const AGENT_DEFINITION_PROJECT_PATHSPECS: readonly string[] = [ + ...listProjectMetadataRelativePaths("agents"), + ...listProjectMetadataRelativePaths("plugins"), + ".agents/plugins", +]; + /** Agent-discovery context for a workspace involved in a workspace turn. */ interface WorkspaceTurnAgentContext { runtime: Runtime; @@ -3696,17 +3711,19 @@ export class TaskService { runtime: Runtime; workspacePath: string; includeAgentPlugins: boolean; - }): Promise> { + }): Promise> { assert(params.agentId.length > 0, "validateWorkspaceTurnAgentId: agentId must be non-empty"); const parsedAgentId = AgentIdSchema.safeParse(params.agentId); if (!parsedAgentId.success) { return Err(`Task.createWorkspaceTurn: invalid agentId (${params.agentId})`); } - // The winning definition's scope is captured so the dispatched turn can pin the - // validated provenance at stream time (strictAgentResolution.expectedScope): a - // validated shadow vanishing must not let a lower-priority definition with the - // same id run under a strict send. + // The winning definition's scope AND exact source are captured so the dispatched + // turn can pin the validated provenance at stream time (strictAgentResolution): + // a validated definition vanishing must not let a different candidate with the + // same id (lower scope, or a same-scope sibling like a plugin) run under a + // strict send. let scope: AgentDefinitionScope; + let source: string | undefined; try { const definition = await readAgentDefinition( params.runtime, @@ -3715,6 +3732,7 @@ export class TaskService { { includeAgentPlugins: params.includeAgentPlugins } ); scope = definition.scope; + source = definition.source; } catch { return Err(`Task.createWorkspaceTurn: unknown agentId (${params.agentId})`); } @@ -3743,7 +3761,7 @@ export class TaskService { ) { return Err(`Task.createWorkspaceTurn: agentId is disabled (${params.agentId})`); } - return Ok({ scope }); + return Ok({ scope, ...(source != null ? { source } : {}) }); } /** @@ -3788,7 +3806,14 @@ export class TaskService { */ ownerResolutionPredictsTarget: boolean; }): Promise< - Result<{ validatedContext: WorkspaceTurnAgentContext; scope: AgentDefinitionScope }, string> + Result< + { + validatedContext: WorkspaceTurnAgentContext; + scope: AgentDefinitionScope; + source?: string; + }, + string + > > { const reachable = await runtimePathExists(params.target.runtime, params.target.workspacePath); if (reachable) { @@ -3798,7 +3823,7 @@ export class TaskService { ...params.target, }); if (validation.success) { - return Ok({ validatedContext: params.target, scope: validation.data.scope }); + return Ok({ validatedContext: params.target, ...validation.data }); } if (params.targetInitPending) { return Err( @@ -3844,7 +3869,7 @@ export class TaskService { ...params.owner, }); return validation.success - ? Ok({ validatedContext: params.owner, scope: validation.data.scope }) + ? Ok({ validatedContext: params.owner, ...validation.data }) : validation; } @@ -3932,10 +3957,10 @@ export class TaskService { // agent-authored frontmatter `ai` defaults participate in AI-settings resolution // (mirrors the sub-agent creation path). Left unset for default exec/resume turns. let agentDefinitionContext: NodeAgentDefinitionContext | undefined; - // Scope of the definition that authorized the dispatch; pinned at stream time via - // strictAgentResolution.expectedScope so a vanished shadow cannot silently hand the - // id to a lower-priority definition. - let validatedAgentScope: AgentDefinitionScope | undefined; + // Provenance (scope + exact source) of the definition that authorized the + // dispatch; pinned at stream time via strictAgentResolution so a vanished + // definition cannot silently hand the id to a different candidate. + let validatedAgentProvenance: { scope: AgentDefinitionScope; source?: string } | undefined; // Post-create target validation failure. Deferred (not returned immediately) so the // workspace-turn record is still written first: the record marks the created workspace // as owner-owned and retryable via mode="existing", and the failure settles through the @@ -4036,7 +4061,10 @@ export class TaskService { }); if (!validation.success) return Err(validation.error); workspaceTurnAgentId = requestedAgentId; - validatedAgentScope = validation.data.scope; + validatedAgentProvenance = { + scope: validation.data.scope, + ...(validation.data.source != null ? { source: validation.data.source } : {}), + }; agentDefinitionContext = { ...validation.data.validatedContext, workspaceId: targetWorkspaceId, @@ -4086,7 +4114,21 @@ export class TaskService { ownerContext.runtime, ownerContext.workspacePath ); - ownerVouchesForTargetBase = ownerBranch != null && ownerBranch === effectiveTrunkBranch; + const ownerBranchMatchesTargetBase = + ownerBranch != null && ownerBranch === effectiveTrunkBranch; + // Branch equality is not checkout equality: the child is created from COMMITTED + // branch state, so uncommitted agent-definition changes in the owner (a shadow + // added or removed) make owner-side resolution diverge from what the target will + // actually see. Vouching therefore also requires the agent-definition paths to + // be clean; unknown cleanliness (no git output) fails the vouch. + const ownerAgentDirsClean = ownerBranchMatchesTargetBase + ? await tryReadGitPathsClean( + ownerContext.runtime, + ownerContext.workspacePath, + AGENT_DEFINITION_PROJECT_PATHSPECS + ) + : undefined; + ownerVouchesForTargetBase = ownerBranchMatchesTargetBase && ownerAgentDirsClean === true; // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the // OWNER's checkout before creating any workspace. Fatal only when the owner's // checked-out branch provably IS the target's base (an omitted trunkBranch still @@ -4169,7 +4211,10 @@ export class TaskService { ? `${validation.error} — no turn was dispatched; automatic cleanup of the disposable workspace (${targetWorkspaceId}) was scheduled (if cleanup fails, it remains owned by this caller and retryable via workspace.mode="existing")` : `${validation.error} — no turn was dispatched; the created workspace (${targetWorkspaceId}) is owned by this caller and can be retried via workspace.mode="existing" once ready`; } else { - validatedAgentScope = validation.data.scope; + validatedAgentProvenance = { + scope: validation.data.scope, + ...(validation.data.source != null ? { source: validation.data.source } : {}), + }; agentDefinitionContext = { ...validation.data.validatedContext, workspaceId: targetWorkspaceId, @@ -4357,7 +4402,14 @@ export class TaskService { ...(requestedAgentId != null ? { strictAgentResolution: - validatedAgentScope != null ? { expectedScope: validatedAgentScope } : true, + validatedAgentProvenance != null + ? { + expectedScope: validatedAgentProvenance.scope, + ...(validatedAgentProvenance.source != null + ? { expectedSource: validatedAgentProvenance.source } + : {}), + } + : true, } : {}), }, diff --git a/src/node/services/taskUtils.ts b/src/node/services/taskUtils.ts index aa10a70f87b..a0129e7c3be 100644 --- a/src/node/services/taskUtils.ts +++ b/src/node/services/taskUtils.ts @@ -68,6 +68,36 @@ export async function tryReadGitCurrentBranch( } } +/** + * True when the checkout has no uncommitted changes (including untracked files) + * under the given pathspecs; undefined when this cannot be determined (no git, + * unreachable runtime). Callers use this as proof that the committed base equals + * the working tree for those paths, so "unknown" must stay distinguishable from + * "clean". + */ +export async function tryReadGitPathsClean( + runtime: Runtime, + workspacePath: string, + pathspecs: readonly string[] +): Promise { + assert(workspacePath.length > 0, "tryReadGitPathsClean: workspacePath must be non-empty"); + assert(pathspecs.length > 0, "tryReadGitPathsClean: pathspecs must be non-empty"); + + try { + const quoted = pathspecs.map((pathspec) => `'${pathspec}'`).join(" "); + const result = await execBuffered(runtime, `git status --porcelain -- ${quoted}`, { + cwd: workspacePath, + timeout: 10, + }); + if (result.exitCode !== 0) { + return undefined; + } + return result.stdout.trim().length === 0; + } catch { + return undefined; + } +} + /** * Resolve the effective refusal-fallback chain for a workspace's turn. * Task children can opt out via taskOnRefusal: "fail" (e.g. workflow verifier From 47d1f97e86750dc114ffd40fa84e3079de93b1ff Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 16:05:51 +0000 Subject: [PATCH 20/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20include=20gitignore?= =?UTF-8?q?d=20files=20in=20cleanliness=20proof;=20defer=20misses=20when?= =?UTF-8?q?=20init=20hooks=20can=20install=20agents?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node/services/taskService.test.ts | 52 ++++++++++++++++++++++++--- src/node/services/taskService.ts | 19 ++++++++-- src/node/services/taskUtils.ts | 13 +++---- 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index 818af8cf8d4..eebc2c17cdf 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -1029,10 +1029,12 @@ describe("TaskService", () => { stubStableIds(config, ["childworkspace", "turnhandle"]); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); checkoutOwnerBranch(projectPath, "parent"); - // An UNCOMMITTED hidden shadow of the built-in plan exists only in the owner's - // working tree: the new checkout is created from committed branch state and - // validly resolves the built-in, so the owner-side miss must stay advisory - // (branch equality is not checkout equality). + // A GITIGNORED hidden shadow of the built-in plan exists only in the owner's + // working tree (plain `git status` would not even list it): the new checkout is + // created from committed branch state and validly resolves the built-in, so the + // owner-side miss must stay advisory (branch equality is not checkout equality). + await fsPromises.writeFile(path.join(projectPath, ".gitignore"), ".mux/agents/\n"); + commitOwnerAgentFiles(projectPath); const agentsDir = path.join(projectPath, ".mux", "agents"); await fsPromises.mkdir(agentsDir, { recursive: true }); await fsPromises.writeFile( @@ -1071,6 +1073,48 @@ describe("TaskService", () => { expect(sendMessageCall[2]).toMatchObject({ agentId: "plan" }); }); + test("createWorkspaceTurn defers owner-side misses when a committed init hook could install agents", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + checkoutOwnerBranch(projectPath, "parent"); + // A committed project init hook runs at target creation and may install the + // requested agent; the owner (initialized before the hook existed) legitimately + // lacks it, so an owner-side miss must not reject the launch pre-create. + const muxDir = path.join(projectPath, ".mux"); + await fsPromises.mkdir(muxDir, { recursive: true }); + await fsPromises.writeFile(path.join(muxDir, "init"), "#!/bin/sh\nexit 0\n"); + commitOwnerAgentFiles(projectPath); + + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: createWorkspaceTurnMetadata(projectPath) })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "hookinstalled", + prompt: "Should defer to the target", + title: "Hook-installed agent", + workspace: { mode: "new" }, + }); + + // The miss defers to the created checkout, which is authoritative (validated + // post-create; here the hook did not actually install it, so the launch still + // fails — but only AFTER the target had its chance). + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("no turn was dispatched"); + } + expect(createWorkspace).toHaveBeenCalledTimes(1); + expect(sendMessage).not.toHaveBeenCalled(); + }); + test("createWorkspaceTurn rejects invalid, unknown, and internal agent ids before creating a workspace", async () => { const config = await createTestConfig(rootDir); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index d4625a15beb..b09a9a618ec 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -4141,11 +4141,26 @@ export class TaskService { ...ownerContext, }); if (!validation.success) { - if (ownerVouchesForTargetBase) { + // Fatal only when the owner provably equals the base AND target init cannot + // add definitions: a committed project init hook runs at target creation and + // may install the requested agent even though the owner (initialized before + // the hook existed, or with per-workspace conditions) legitimately lacks it. + const ownerDiscovery = ownerContext; + const ownerInitHookResults = ownerVouchesForTargetBase + ? await Promise.all( + listProjectMetadataRelativePaths("init").map((relativePath) => + runtimePathExists( + ownerDiscovery.runtime, + ownerDiscovery.runtime.normalizePath(relativePath, ownerDiscovery.workspacePath) + ) + ) + ) + : []; + if (ownerVouchesForTargetBase && !ownerInitHookResults.some(Boolean)) { return Err(validation.error); } log.debug( - "Task.createWorkspaceTurn: owner-side agent validation failed; deferring to the target branch checkout", + "Task.createWorkspaceTurn: owner-side agent validation failed; deferring to the target checkout", { agentId: requestedAgentId, error: validation.error } ); } else { diff --git a/src/node/services/taskUtils.ts b/src/node/services/taskUtils.ts index a0129e7c3be..acd4cb6c3cf 100644 --- a/src/node/services/taskUtils.ts +++ b/src/node/services/taskUtils.ts @@ -69,11 +69,12 @@ export async function tryReadGitCurrentBranch( } /** - * True when the checkout has no uncommitted changes (including untracked files) - * under the given pathspecs; undefined when this cannot be determined (no git, - * unreachable runtime). Callers use this as proof that the committed base equals - * the working tree for those paths, so "unknown" must stay distinguishable from - * "clean". + * True when the checkout has no uncommitted changes (including untracked AND + * gitignored files — an ignored local file still shadows committed state for + * discovery-style readers) under the given pathspecs; undefined when this cannot + * be determined (no git, unreachable runtime). Callers use this as proof that the + * committed base equals the working tree for those paths, so "unknown" must stay + * distinguishable from "clean". */ export async function tryReadGitPathsClean( runtime: Runtime, @@ -85,7 +86,7 @@ export async function tryReadGitPathsClean( try { const quoted = pathspecs.map((pathspec) => `'${pathspec}'`).join(" "); - const result = await execBuffered(runtime, `git status --porcelain -- ${quoted}`, { + const result = await execBuffered(runtime, `git status --porcelain --ignored -- ${quoted}`, { cwd: workspacePath, timeout: 10, }); From bae62719e841351dd851a8bbe2ca6c729850a628 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 17:21:55 +0000 Subject: [PATCH 21/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20pin=20base-chain=20?= =?UTF-8?q?provenance;=20executable-hook=20and=20commit-equality=20vouchin?= =?UTF-8?q?g?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/common/orpc/schemas/stream.ts | 15 +++ .../resolveAgentInheritanceChain.ts | 5 + src/node/services/agentResolution.test.ts | 57 ++++++++- src/node/services/agentResolution.ts | 39 +++++++ src/node/services/aiService.test.ts | 2 +- src/node/services/taskService.test.ts | 90 ++++++++++++++- src/node/services/taskService.ts | 109 +++++++++++++----- src/node/services/taskUtils.ts | 39 +++++++ 8 files changed, 326 insertions(+), 30 deletions(-) diff --git a/src/common/orpc/schemas/stream.ts b/src/common/orpc/schemas/stream.ts index 1c2a067f6ee..cee51a58121 100644 --- a/src/common/orpc/schemas/stream.ts +++ b/src/common/orpc/schemas/stream.ts @@ -825,6 +825,21 @@ export const SendMessageOptionsSchema = z.object({ * itself when known. */ expectedSource: z.string().optional(), + /** + * Provenance of the full resolved base chain (leaf first), pinned because + * stream-time inheritance resolution reloads every base independently — a + * vanished base shadow must not silently swap a different definition into + * the chain's prompt/tool policy. + */ + expectedChain: z + .array( + z.object({ + id: AgentIdSchema, + scope: AgentDefinitionScopeSchema, + source: z.string().optional(), + }) + ) + .optional(), }), ]) .optional(), diff --git a/src/node/services/agentDefinitions/resolveAgentInheritanceChain.ts b/src/node/services/agentDefinitions/resolveAgentInheritanceChain.ts index c9f40c8549b..d361ececdc9 100644 --- a/src/node/services/agentDefinitions/resolveAgentInheritanceChain.ts +++ b/src/node/services/agentDefinitions/resolveAgentInheritanceChain.ts @@ -18,6 +18,9 @@ export interface AgentForInheritance { uiColor?: string; /** Per-hop (unmerged) frontmatter `ai` defaults for AI-settings resolution. */ ai?: AgentDefinitionPackage["frontmatter"]["ai"]; + /** Provenance of the hop's winning definition (strict-send chain pinning). */ + scope: AgentDefinitionPackage["scope"]; + source?: string; } interface ResolveAgentInheritanceChainOptions { @@ -71,6 +74,8 @@ export async function resolveAgentInheritanceChain( tools: currentDefinition.frontmatter.tools, uiColor: currentDefinition.frontmatter.ui?.color, ai: currentDefinition.frontmatter.ai, + scope: currentDefinition.scope, + ...(currentDefinition.source != null ? { source: currentDefinition.source } : {}), }); const baseId = currentDefinition.frontmatter.base; diff --git a/src/node/services/agentResolution.test.ts b/src/node/services/agentResolution.test.ts index 37832105f86..a6106d396b9 100644 --- a/src/node/services/agentResolution.test.ts +++ b/src/node/services/agentResolution.test.ts @@ -488,7 +488,15 @@ describe("resolveAgentForStream strict resolution", () => { agentId: string; strictAgentResolution: | boolean - | { expectedScope: "project" | "global" | "built-in"; expectedSource?: string }; + | { + expectedScope: "project" | "global" | "built-in"; + expectedSource?: string; + expectedChain?: Array<{ + id: string; + scope: "project" | "global" | "built-in"; + source?: string; + }>; + }; agentAiDefaults?: ProjectsConfig["agentAiDefaults"]; }) { const cfg: ProjectsConfig = { @@ -658,6 +666,53 @@ describe("resolveAgentForStream strict resolution", () => { if (matching.success) expect(matching.data.effectiveAgentId).toBe("plan"); }); + test("strict mode rejects a base chain resolving differently than validated", async () => { + using tempDir = new DisposableTempDir("agent-resolution-strict-chain"); + const projectPath = path.join(tempDir.path, "project"); + const agentsDir = path.join(projectPath, ".mux", "agents"); + await fs.mkdir(agentsDir, { recursive: true }); + await fs.writeFile( + path.join(agentsDir, "custom.md"), + ["---", "name: Custom", "base: plan", "---", "Custom agent."].join("\n") + ); + + // Launch validation saw the custom agent inheriting a project plan shadow that has + // since been removed: the leaf provenance is unchanged, but the built-in plan now + // takes over the base hop — the strict chain pin must fail the send. + const staleChain = await resolveTopLevel({ + projectPath, + agentId: "custom", + strictAgentResolution: { + expectedScope: "project", + expectedChain: [ + { id: "custom", scope: "project" }, + { id: "plan", scope: "project" }, + ], + }, + }); + expect(staleChain.success).toBe(false); + if (!staleChain.success && staleChain.error.type === "unknown") { + expect(staleChain.error.raw).toContain("base chain"); + } else { + expect(staleChain.success === false && staleChain.error.type).toBe("unknown"); + } + + // The chain as it actually resolves streams normally. + const matching = await resolveTopLevel({ + projectPath, + agentId: "custom", + strictAgentResolution: { + expectedScope: "project", + expectedChain: [ + { id: "custom", scope: "project" }, + { id: "plan", scope: "built-in", source: "built-in" }, + ], + }, + }); + expect(matching.success).toBe(true); + if (matching.success) expect(matching.data.effectiveAgentId).toBe("custom"); + }); + test("strict mode fails closed when eligibility resolution throws", async () => { using tempDir = new DisposableTempDir("agent-resolution-strict-broken-base"); const projectPath = path.join(tempDir.path, "project"); diff --git a/src/node/services/agentResolution.ts b/src/node/services/agentResolution.ts index 49677912f91..3b318c8aa96 100644 --- a/src/node/services/agentResolution.ts +++ b/src/node/services/agentResolution.ts @@ -435,6 +435,45 @@ export async function resolveAgentForStream( includeAgentPlugins, }); + // Strict chain pin: inheritance resolution reloads every base independently, so a + // vanished base shadow could otherwise silently swap a different definition into + // the chain's prompt/tool policy even though the validated leaf is unchanged. + const strictExpectedChain = + typeof strictAgentResolution === "object" ? strictAgentResolution.expectedChain : undefined; + if (strictTopLevel && strictExpectedChain != null) { + const chainMatches = + agentsForInheritance.length === strictExpectedChain.length && + strictExpectedChain.every((expected, index) => { + const actual = agentsForInheritance[index]; + return ( + actual != null && + actual.id === expected.id && + actual.scope === expected.scope && + (expected.source == null || actual.source === expected.source) + ); + }); + if (!chainMatches) { + const describeChain = ( + entries: ReadonlyArray<{ id: string; scope: string; source?: string }> + ) => + entries + .map( + (entry) => + `${entry.id}(${entry.scope}${entry.source != null ? ` @ ${entry.source}` : ""})` + ) + .join(" -> "); + const errorMessage = `Agent '${requestedAgentId}' base chain now resolves differently than launch validation saw (expected ${describeChain(strictExpectedChain)}, found ${describeChain(agentsForInheritance)}); refusing to stream an explicit agent request.`; + emitError( + createErrorEvent(workspaceId, { + messageId: createAssistantMessageId(), + error: errorMessage, + errorType: "unknown", + }) + ); + return Err({ type: "unknown", raw: errorMessage }); + } + } + const agentIsPlanLike = isPlanLikeInResolvedChain(agentsForInheritance); const effectiveMode = agentDefinition.id === "compact" ? "compact" : agentIsPlanLike ? "plan" : "exec"; diff --git a/src/node/services/aiService.test.ts b/src/node/services/aiService.test.ts index 4a9806bc6ed..2991b7820a3 100644 --- a/src/node/services/aiService.test.ts +++ b/src/node/services/aiService.test.ts @@ -268,7 +268,7 @@ function resolvedAgentResultFor( agentDiscoveryRuntime: new LocalRuntime(metadata.projectPath), agentDiscoveryPath: metadata.projectPath, isSubagentWorkspace: false, - agentInheritanceChain: [{ id: "exec", tools: { add: [".*"] } }], + agentInheritanceChain: [{ id: "exec", scope: "built-in", tools: { add: [".*"] } }], agentIsPlanLike: false, effectiveMode: "exec", taskSettings: DEFAULT_TASK_SETTINGS, diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index eebc2c17cdf..8feb0e84d94 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -1020,7 +1020,13 @@ describe("TaskService", () => { // swapping in exec (or running a different definition for the same id) post-init. expect(sendMessageCall[2]).toMatchObject({ agentId: "plan", - strictAgentResolution: { expectedScope: "built-in", expectedSource: "built-in" }, + strictAgentResolution: { + expectedScope: "built-in", + expectedSource: "built-in", + // The full base chain is pinned too: stream-time inheritance resolution + // reloads every base independently. + expectedChain: [{ id: "plan", scope: "built-in", source: "built-in" }], + }, }); }); @@ -1083,7 +1089,8 @@ describe("TaskService", () => { // lacks it, so an owner-side miss must not reject the launch pre-create. const muxDir = path.join(projectPath, ".mux"); await fsPromises.mkdir(muxDir, { recursive: true }); - await fsPromises.writeFile(path.join(muxDir, "init"), "#!/bin/sh\nexit 0\n"); + // Executable: only hooks passing the init runner's test -x rule can ever run. + await fsPromises.writeFile(path.join(muxDir, "init"), "#!/bin/sh\nexit 0\n", { mode: 0o755 }); commitOwnerAgentFiles(projectPath); const createWorkspace = mock( @@ -1115,6 +1122,85 @@ describe("TaskService", () => { expect(sendMessage).not.toHaveBeenCalled(); }); + test("createWorkspaceTurn keeps misses fatal when the committed init hook is not executable", async () => { + const config = await createTestConfig(rootDir); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + checkoutOwnerBranch(projectPath, "parent"); + // Only executable hooks can run (the init runner's test -x rule): a committed but + // non-executable init file cannot install agents, so the miss stays fatal and no + // unnecessary owned workspace is created. + const muxDir = path.join(projectPath, ".mux"); + await fsPromises.mkdir(muxDir, { recursive: true }); + await fsPromises.writeFile(path.join(muxDir, "init"), "#!/bin/sh\nexit 0\n", { mode: 0o644 }); + commitOwnerAgentFiles(projectPath); + + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: createWorkspaceTurnMetadata(projectPath) })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "doesnotexist", + prompt: "Should not run", + title: "Non-executable hook", + workspace: { mode: "new" }, + }); + + expect(result.success).toBe(false); + if (!result.success) expect(result.error).toContain("unknown agentId"); + expect(createWorkspace).not.toHaveBeenCalled(); + }); + + test("createWorkspaceTurn defers owner-side misses when the owner is behind its origin ref", async () => { + const config = await createTestConfig(rootDir); + stubStableIds(config, ["childworkspace", "turnhandle"]); + const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); + checkoutOwnerBranch(projectPath, "parent"); + // Worktree creation may branch from origin/ when the local branch can + // fast-forward: an owner behind its origin cannot vouch for the base commit — an + // agent added only in the newer remote commit would be wrongly rejected pre-create. + const bareRemote = path.join(rootDir, "origin-behind.git"); + execSync(`git init --bare -q '${bareRemote}'`, { cwd: rootDir, stdio: "ignore" }); + execSync(`git remote add origin '${bareRemote}'`, { cwd: projectPath, stdio: "ignore" }); + execSync("git commit -q --allow-empty -m newer", { cwd: projectPath, stdio: "ignore" }); + execSync("git push -q origin parent", { cwd: projectPath, stdio: "ignore" }); + execSync("git reset -q --hard HEAD~1", { cwd: projectPath, stdio: "ignore" }); + + const createWorkspace = mock( + (): Promise> => + Promise.resolve(Ok({ metadata: createWorkspaceTurnMetadata(projectPath) })) + ); + const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); + const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); + const { taskService } = createTaskServiceHarness(config, { + workspaceService: workspaceMocks.workspaceService, + }); + + const result = await taskService.createWorkspaceTurn({ + ownerWorkspaceId: parentId, + agentId: "remoteonly", + prompt: "Should defer to the target", + title: "Owner behind origin", + workspace: { mode: "new" }, + }); + + // The miss defers to the created checkout (authoritative for the actual base + // commit); here the target lacks the agent too, so the launch still fails — but + // only after the target had its chance. + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("no turn was dispatched"); + } + expect(createWorkspace).toHaveBeenCalledTimes(1); + expect(sendMessage).not.toHaveBeenCalled(); + }); + test("createWorkspaceTurn rejects invalid, unknown, and internal agent ids before creating a workspace", async () => { const config = await createTestConfig(rootDir); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index b09a9a618ec..42116f13a4d 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -58,12 +58,14 @@ import type { InitLogger, Runtime } from "@/node/runtime/Runtime"; import { readPlanFile } from "@/node/utils/runtime/helpers"; import { coerceNonEmptyString, + tryReadGitBranchMatchesOrigin, tryReadGitCurrentBranch, tryReadGitHeadCommitSha, tryReadGitPathsClean, findWorkspaceEntry, } from "@/node/services/taskUtils"; import { listProjectMetadataRelativePaths } from "@/common/compat/legacyMux"; +import { findInitHookRelativePath } from "@/node/runtime/initHook"; import { validateWorkspaceName } from "@/common/utils/validation/workspaceValidation"; import { getTaskGroupCount } from "@/common/utils/tools/taskGroups"; import { stripTrailingSlashes } from "@/node/utils/pathUtils"; @@ -539,6 +541,13 @@ const AGENT_DEFINITION_PROJECT_PATHSPECS: readonly string[] = [ ".agents/plugins", ]; +/** Provenance of one hop of a validated agent's base chain (strict-send pinning). */ +interface WorkspaceTurnAgentChainEntry { + id: string; + scope: AgentDefinitionScope; + source?: string; +} + /** Agent-discovery context for a workspace involved in a workspace turn. */ interface WorkspaceTurnAgentContext { runtime: Runtime; @@ -3707,23 +3716,35 @@ export class TaskService { private async validateWorkspaceTurnAgentId(params: { cfg: ReturnType; agentId: string; + /** Workspace the validation is for (log correlation in chain resolution). */ + workspaceId: string; /** Discovery context of the workspace whose turn will run (or the owner pre-create). */ runtime: Runtime; workspacePath: string; includeAgentPlugins: boolean; - }): Promise> { + }): Promise< + Result< + { + scope: AgentDefinitionScope; + source?: string; + chain: WorkspaceTurnAgentChainEntry[]; + }, + string + > + > { assert(params.agentId.length > 0, "validateWorkspaceTurnAgentId: agentId must be non-empty"); const parsedAgentId = AgentIdSchema.safeParse(params.agentId); if (!parsedAgentId.success) { return Err(`Task.createWorkspaceTurn: invalid agentId (${params.agentId})`); } - // The winning definition's scope AND exact source are captured so the dispatched - // turn can pin the validated provenance at stream time (strictAgentResolution): - // a validated definition vanishing must not let a different candidate with the - // same id (lower scope, or a same-scope sibling like a plugin) run under a + // The winning definition's provenance (scope + exact source) AND the resolved + // base chain's provenance are captured so the dispatched turn can pin them at + // stream time (strictAgentResolution): a validated definition or base shadow + // vanishing must not let a different candidate with the same id run under a // strict send. let scope: AgentDefinitionScope; let source: string | undefined; + let chain: WorkspaceTurnAgentChainEntry[]; try { const definition = await readAgentDefinition( params.runtime, @@ -3733,6 +3754,19 @@ export class TaskService { ); scope = definition.scope; source = definition.source; + const resolvedChain = await resolveAgentInheritanceChain({ + runtime: params.runtime, + workspacePath: params.workspacePath, + agentId: parsedAgentId.data, + agentDefinition: definition, + workspaceId: params.workspaceId, + includeAgentPlugins: params.includeAgentPlugins, + }); + chain = resolvedChain.map((entry) => ({ + id: entry.id, + scope: entry.scope, + ...(entry.source != null ? { source: entry.source } : {}), + })); } catch { return Err(`Task.createWorkspaceTurn: unknown agentId (${params.agentId})`); } @@ -3761,7 +3795,7 @@ export class TaskService { ) { return Err(`Task.createWorkspaceTurn: agentId is disabled (${params.agentId})`); } - return Ok({ scope, ...(source != null ? { source } : {}) }); + return Ok({ scope, ...(source != null ? { source } : {}), chain }); } /** @@ -3791,6 +3825,8 @@ export class TaskService { private async validateWorkspaceTurnAgentIdForTarget(params: { cfg: ReturnType; agentId: string; + /** Target workspace id (log correlation in chain resolution). */ + workspaceId: string; target: WorkspaceTurnAgentContext; owner: WorkspaceTurnAgentContext; /** Whether the target workspace's init hook is still running (see doc above). */ @@ -3811,6 +3847,7 @@ export class TaskService { validatedContext: WorkspaceTurnAgentContext; scope: AgentDefinitionScope; source?: string; + chain: WorkspaceTurnAgentChainEntry[]; }, string > @@ -3820,6 +3857,7 @@ export class TaskService { const validation = await this.validateWorkspaceTurnAgentId({ cfg: params.cfg, agentId: params.agentId, + workspaceId: params.workspaceId, ...params.target, }); if (validation.success) { @@ -3866,6 +3904,7 @@ export class TaskService { const validation = await this.validateWorkspaceTurnAgentId({ cfg: params.cfg, agentId: params.agentId, + workspaceId: params.workspaceId, ...params.owner, }); return validation.success @@ -3957,10 +3996,13 @@ export class TaskService { // agent-authored frontmatter `ai` defaults participate in AI-settings resolution // (mirrors the sub-agent creation path). Left unset for default exec/resume turns. let agentDefinitionContext: NodeAgentDefinitionContext | undefined; - // Provenance (scope + exact source) of the definition that authorized the - // dispatch; pinned at stream time via strictAgentResolution so a vanished - // definition cannot silently hand the id to a different candidate. - let validatedAgentProvenance: { scope: AgentDefinitionScope; source?: string } | undefined; + // Provenance (scope + exact source + base chain) of the definition that + // authorized the dispatch; pinned at stream time via strictAgentResolution so a + // vanished definition or base shadow cannot silently hand any hop to a + // different candidate. + let validatedAgentProvenance: + | { scope: AgentDefinitionScope; source?: string; chain: WorkspaceTurnAgentChainEntry[] } + | undefined; // Post-create target validation failure. Deferred (not returned immediately) so the // workspace-turn record is still written first: the record marks the created workspace // as owner-owned and retryable via mode="existing", and the failure settles through the @@ -4050,6 +4092,7 @@ export class TaskService { const validation = await this.validateWorkspaceTurnAgentIdForTarget({ cfg, agentId: requestedAgentId, + workspaceId: existingWorkspaceId, target: targetContext, owner: ownerContext, targetInitPending: @@ -4064,6 +4107,7 @@ export class TaskService { validatedAgentProvenance = { scope: validation.data.scope, ...(validation.data.source != null ? { source: validation.data.source } : {}), + chain: validation.data.chain, }; agentDefinitionContext = { ...validation.data.validatedContext, @@ -4128,7 +4172,22 @@ export class TaskService { AGENT_DEFINITION_PROJECT_PATHSPECS ) : undefined; - ownerVouchesForTargetBase = ownerBranchMatchesTargetBase && ownerAgentDirsClean === true; + // Nor is the owner's HEAD necessarily the target's base COMMIT: worktree + // creation may branch from origin/ when the local branch can + // fast-forward, so a stale (or diverged) owner cannot vouch for definitions + // added or removed in the newer remote commit. + const ownerCommitMatchesOrigin = + ownerBranchMatchesTargetBase && ownerAgentDirsClean === true + ? await tryReadGitBranchMatchesOrigin( + ownerContext.runtime, + ownerContext.workspacePath, + effectiveTrunkBranch + ) + : undefined; + ownerVouchesForTargetBase = + ownerBranchMatchesTargetBase && + ownerAgentDirsClean === true && + ownerCommitMatchesOrigin === true; // Pre-create stage: catch obviously bad ids (unknown/hidden/disabled) against the // OWNER's checkout before creating any workspace. Fatal only when the owner's // checked-out branch provably IS the target's base (an omitted trunkBranch still @@ -4138,25 +4197,20 @@ export class TaskService { const validation = await this.validateWorkspaceTurnAgentId({ cfg, agentId: requestedAgentId, + workspaceId: ownerWorkspaceId, ...ownerContext, }); if (!validation.success) { // Fatal only when the owner provably equals the base AND target init cannot - // add definitions: a committed project init hook runs at target creation and - // may install the requested agent even though the owner (initialized before - // the hook existed, or with per-workspace conditions) legitimately lacks it. - const ownerDiscovery = ownerContext; - const ownerInitHookResults = ownerVouchesForTargetBase - ? await Promise.all( - listProjectMetadataRelativePaths("init").map((relativePath) => - runtimePathExists( - ownerDiscovery.runtime, - ownerDiscovery.runtime.normalizePath(relativePath, ownerDiscovery.workspacePath) - ) - ) - ) - : []; - if (ownerVouchesForTargetBase && !ownerInitHookResults.some(Boolean)) { + // add definitions: an EXECUTABLE committed project init hook (the same + // findInitHookRelativePath rule the init runner uses — non-executable files + // never run) runs at target creation and may install the requested agent even + // though the owner (initialized before the hook existed, or with + // per-workspace conditions) legitimately lacks it. + const ownerInitHook = ownerVouchesForTargetBase + ? await findInitHookRelativePath(ownerContext.runtime, ownerContext.workspacePath) + : null; + if (ownerVouchesForTargetBase && ownerInitHook == null) { return Err(validation.error); } log.debug( @@ -4208,6 +4262,7 @@ export class TaskService { const validation = await this.validateWorkspaceTurnAgentIdForTarget({ cfg, agentId: requestedAgentId, + workspaceId: targetWorkspaceId, target: targetContext, owner: ownerContext, // create() starts runBackgroundInit asynchronously; a reachable checkout whose @@ -4229,6 +4284,7 @@ export class TaskService { validatedAgentProvenance = { scope: validation.data.scope, ...(validation.data.source != null ? { source: validation.data.source } : {}), + chain: validation.data.chain, }; agentDefinitionContext = { ...validation.data.validatedContext, @@ -4423,6 +4479,7 @@ export class TaskService { ...(validatedAgentProvenance.source != null ? { expectedSource: validatedAgentProvenance.source } : {}), + expectedChain: validatedAgentProvenance.chain, } : true, } diff --git a/src/node/services/taskUtils.ts b/src/node/services/taskUtils.ts index acd4cb6c3cf..c29419ef74b 100644 --- a/src/node/services/taskUtils.ts +++ b/src/node/services/taskUtils.ts @@ -68,6 +68,45 @@ export async function tryReadGitCurrentBranch( } } +/** + * True when the checkout's HEAD commit matches its origin ref for the given + * branch (or no origin ref exists, making local HEAD the only base candidate); + * false when they differ; undefined when this cannot be determined. Worktree + * creation may branch from origin/ when the local branch can + * fast-forward, so callers must not treat a stale local checkout as the + * authoritative base commit. + */ +export async function tryReadGitBranchMatchesOrigin( + runtime: Runtime, + workspacePath: string, + branch: string +): Promise { + assert( + workspacePath.length > 0, + "tryReadGitBranchMatchesOrigin: workspacePath must be non-empty" + ); + assert(branch.length > 0, "tryReadGitBranchMatchesOrigin: branch must be non-empty"); + + const headSha = await tryReadGitHeadCommitSha(runtime, workspacePath); + if (headSha == null) { + return undefined; + } + try { + const result = await execBuffered( + runtime, + `git rev-parse --verify --quiet 'origin/${branch}^{commit}'`, + { cwd: workspacePath, timeout: 10 } + ); + if (result.exitCode !== 0) { + // No origin ref for this branch: the local commit is the only candidate base. + return true; + } + return result.stdout.trim() === headSha; + } catch { + return undefined; + } +} + /** * True when the checkout has no uncommitted changes (including untracked AND * gitignored files — an ignored local file still shadows committed state for From 112f2300842ab06b28c97bac3bfdf3caf6180305 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 17:44:16 +0000 Subject: [PATCH 22/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20shell-quote=20repo-?= =?UTF-8?q?controlled=20branch=20names=20in=20git=20probes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node/services/taskUtils.test.ts | 63 +++++++++++++++++++++++++++++ src/node/services/taskUtils.ts | 15 ++++--- 2 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 src/node/services/taskUtils.test.ts diff --git a/src/node/services/taskUtils.test.ts b/src/node/services/taskUtils.test.ts new file mode 100644 index 00000000000..d0d455c52f3 --- /dev/null +++ b/src/node/services/taskUtils.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, test } from "bun:test"; +import { execSync } from "node:child_process"; +import * as fsPromises from "node:fs/promises"; +import * as path from "node:path"; + +import { LocalRuntime } from "@/node/runtime/LocalRuntime"; +import { DisposableTempDir } from "@/node/services/tempDir"; +import { tryReadGitBranchMatchesOrigin } from "./taskUtils"; + +function initGitRepo(repoPath: string): void { + execSync("git init -b main", { cwd: repoPath, stdio: "ignore" }); + execSync('git config user.email "test@example.com"', { cwd: repoPath, stdio: "ignore" }); + execSync('git config user.name "test"', { cwd: repoPath, stdio: "ignore" }); + execSync("git config commit.gpgsign false", { cwd: repoPath, stdio: "ignore" }); + execSync("git commit -q --allow-empty -m init", { cwd: repoPath, stdio: "ignore" }); +} + +describe("tryReadGitBranchMatchesOrigin", () => { + test("shell-quotes repo-controlled branch names (no command injection)", async () => { + using tempDir = new DisposableTempDir("taskutils-branch-injection"); + const repoPath = path.join(tempDir.path, "repo"); + await fsPromises.mkdir(repoPath, { recursive: true }); + initGitRepo(repoPath); + + // A git-valid branch name crafted to break out of naive single-quoting: without + // shell quoting this would execute `touch injected-marker` in the owner runtime. + const maliciousBranch = "safe';touch injected-marker;#"; + const result = await tryReadGitBranchMatchesOrigin( + new LocalRuntime(repoPath), + repoPath, + maliciousBranch + ); + + // No origin ref exists for the branch, so the helper reports "local is the only + // candidate base" — and the injected command must never have run. + expect(result).toBe(true); + try { + await fsPromises.access(path.join(repoPath, "injected-marker")); + expect.unreachable("injected command created a marker file"); + } catch (error) { + expect(error).toBeDefined(); + } + }); + + test("distinguishes matching, diverged, and missing origin refs", async () => { + using tempDir = new DisposableTempDir("taskutils-branch-origin"); + const repoPath = path.join(tempDir.path, "repo"); + const bareRemote = path.join(tempDir.path, "origin.git"); + await fsPromises.mkdir(repoPath, { recursive: true }); + initGitRepo(repoPath); + execSync(`git init --bare -q '${bareRemote}'`, { cwd: tempDir.path, stdio: "ignore" }); + execSync(`git remote add origin '${bareRemote}'`, { cwd: repoPath, stdio: "ignore" }); + execSync("git push -q origin main", { cwd: repoPath, stdio: "ignore" }); + + const runtime = new LocalRuntime(repoPath); + expect(await tryReadGitBranchMatchesOrigin(runtime, repoPath, "main")).toBe(true); + + execSync("git commit -q --allow-empty -m ahead", { cwd: repoPath, stdio: "ignore" }); + expect(await tryReadGitBranchMatchesOrigin(runtime, repoPath, "main")).toBe(false); + + expect(await tryReadGitBranchMatchesOrigin(runtime, repoPath, "no-such-branch")).toBe(true); + }); +}); diff --git a/src/node/services/taskUtils.ts b/src/node/services/taskUtils.ts index c29419ef74b..e6d53f9ed92 100644 --- a/src/node/services/taskUtils.ts +++ b/src/node/services/taskUtils.ts @@ -6,6 +6,7 @@ import assert from "node:assert/strict"; import type { Config, Workspace as WorkspaceConfigEntry } from "@/node/config"; import type { Runtime } from "@/node/runtime/Runtime"; import { execBuffered } from "@/node/utils/runtime/helpers"; +import { shellQuote } from "@/common/utils/shell"; import { resolveModelFallbackChain } from "@/common/utils/ai/modelFallbacks"; export function coerceNonEmptyString(value: unknown): string | undefined { @@ -92,11 +93,13 @@ export async function tryReadGitBranchMatchesOrigin( return undefined; } try { - const result = await execBuffered( - runtime, - `git rev-parse --verify --quiet 'origin/${branch}^{commit}'`, - { cwd: workspacePath, timeout: 10 } - ); + // SECURITY: branch names are repo-controlled input (a git-valid branch can contain + // quotes); the full revision argument must be shell-quoted before interpolation. + const revision = shellQuote(`origin/${branch}^{commit}`); + const result = await execBuffered(runtime, `git rev-parse --verify --quiet ${revision}`, { + cwd: workspacePath, + timeout: 10, + }); if (result.exitCode !== 0) { // No origin ref for this branch: the local commit is the only candidate base. return true; @@ -124,7 +127,7 @@ export async function tryReadGitPathsClean( assert(pathspecs.length > 0, "tryReadGitPathsClean: pathspecs must be non-empty"); try { - const quoted = pathspecs.map((pathspec) => `'${pathspec}'`).join(" "); + const quoted = pathspecs.map((pathspec) => shellQuote(pathspec)).join(" "); const result = await execBuffered(runtime, `git status --porcelain --ignored -- ${quoted}`, { cwd: workspacePath, timeout: 10, From 83ffe42fda0ea5670b8973199a518b708b9eb92a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 17:58:21 +0000 Subject: [PATCH 23/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20owner-side=20misses?= =?UTF-8?q?=20are=20always=20advisory;=20explicit=20branchName=20drops=20o?= =?UTF-8?q?wner=20vouching?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node/services/taskService.test.ts | 141 +++++++------------------- src/node/services/taskService.ts | 26 ++--- 2 files changed, 48 insertions(+), 119 deletions(-) diff --git a/src/node/services/taskService.test.ts b/src/node/services/taskService.test.ts index 8feb0e84d94..665e1dd80cc 100644 --- a/src/node/services/taskService.test.ts +++ b/src/node/services/taskService.test.ts @@ -1079,60 +1079,11 @@ describe("TaskService", () => { expect(sendMessageCall[2]).toMatchObject({ agentId: "plan" }); }); - test("createWorkspaceTurn defers owner-side misses when a committed init hook could install agents", async () => { + test("createWorkspaceTurn owner-side misses are always advisory (target decides)", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); checkoutOwnerBranch(projectPath, "parent"); - // A committed project init hook runs at target creation and may install the - // requested agent; the owner (initialized before the hook existed) legitimately - // lacks it, so an owner-side miss must not reject the launch pre-create. - const muxDir = path.join(projectPath, ".mux"); - await fsPromises.mkdir(muxDir, { recursive: true }); - // Executable: only hooks passing the init runner's test -x rule can ever run. - await fsPromises.writeFile(path.join(muxDir, "init"), "#!/bin/sh\nexit 0\n", { mode: 0o755 }); - commitOwnerAgentFiles(projectPath); - - const createWorkspace = mock( - (): Promise> => - Promise.resolve(Ok({ metadata: createWorkspaceTurnMetadata(projectPath) })) - ); - const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); - const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); - const { taskService } = createTaskServiceHarness(config, { - workspaceService: workspaceMocks.workspaceService, - }); - - const result = await taskService.createWorkspaceTurn({ - ownerWorkspaceId: parentId, - agentId: "hookinstalled", - prompt: "Should defer to the target", - title: "Hook-installed agent", - workspace: { mode: "new" }, - }); - - // The miss defers to the created checkout, which is authoritative (validated - // post-create; here the hook did not actually install it, so the launch still - // fails — but only AFTER the target had its chance). - expect(result.success).toBe(false); - if (!result.success) { - expect(result.error).toContain("no turn was dispatched"); - } - expect(createWorkspace).toHaveBeenCalledTimes(1); - expect(sendMessage).not.toHaveBeenCalled(); - }); - - test("createWorkspaceTurn keeps misses fatal when the committed init hook is not executable", async () => { - const config = await createTestConfig(rootDir); - const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); - checkoutOwnerBranch(projectPath, "parent"); - // Only executable hooks can run (the init runner's test -x rule): a committed but - // non-executable init file cannot install agents, so the miss stays fatal and no - // unnecessary owned workspace is created. - const muxDir = path.join(projectPath, ".mux"); - await fsPromises.mkdir(muxDir, { recursive: true }); - await fsPromises.writeFile(path.join(muxDir, "init"), "#!/bin/sh\nexit 0\n", { mode: 0o644 }); - commitOwnerAgentFiles(projectPath); const createWorkspace = mock( (): Promise> => @@ -1144,64 +1095,27 @@ describe("TaskService", () => { workspaceService: workspaceMocks.workspaceService, }); + // No owner-side equivalence proof is sound (fetched origin commits, existing + // branchName targets, submodules, init hooks): the created checkout is the only + // authoritative source, so a miss defers instead of rejecting pre-create. const result = await taskService.createWorkspaceTurn({ ownerWorkspaceId: parentId, agentId: "doesnotexist", - prompt: "Should not run", - title: "Non-executable hook", - workspace: { mode: "new" }, - }); - - expect(result.success).toBe(false); - if (!result.success) expect(result.error).toContain("unknown agentId"); - expect(createWorkspace).not.toHaveBeenCalled(); - }); - - test("createWorkspaceTurn defers owner-side misses when the owner is behind its origin ref", async () => { - const config = await createTestConfig(rootDir); - stubStableIds(config, ["childworkspace", "turnhandle"]); - const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); - checkoutOwnerBranch(projectPath, "parent"); - // Worktree creation may branch from origin/ when the local branch can - // fast-forward: an owner behind its origin cannot vouch for the base commit — an - // agent added only in the newer remote commit would be wrongly rejected pre-create. - const bareRemote = path.join(rootDir, "origin-behind.git"); - execSync(`git init --bare -q '${bareRemote}'`, { cwd: rootDir, stdio: "ignore" }); - execSync(`git remote add origin '${bareRemote}'`, { cwd: projectPath, stdio: "ignore" }); - execSync("git commit -q --allow-empty -m newer", { cwd: projectPath, stdio: "ignore" }); - execSync("git push -q origin parent", { cwd: projectPath, stdio: "ignore" }); - execSync("git reset -q --hard HEAD~1", { cwd: projectPath, stdio: "ignore" }); - - const createWorkspace = mock( - (): Promise> => - Promise.resolve(Ok({ metadata: createWorkspaceTurnMetadata(projectPath) })) - ); - const sendMessage = mock((): Promise> => Promise.resolve(Ok(undefined))); - const workspaceMocks = createWorkspaceServiceMocks({ create: createWorkspace, sendMessage }); - const { taskService } = createTaskServiceHarness(config, { - workspaceService: workspaceMocks.workspaceService, - }); - - const result = await taskService.createWorkspaceTurn({ - ownerWorkspaceId: parentId, - agentId: "remoteonly", prompt: "Should defer to the target", - title: "Owner behind origin", + title: "Advisory miss", workspace: { mode: "new" }, }); - // The miss defers to the created checkout (authoritative for the actual base - // commit); here the target lacks the agent too, so the launch still fails — but - // only after the target had its chance. expect(result.success).toBe(false); if (!result.success) { + expect(result.error).toContain("unknown agentId"); expect(result.error).toContain("no turn was dispatched"); } expect(createWorkspace).toHaveBeenCalledTimes(1); expect(sendMessage).not.toHaveBeenCalled(); }); - test("createWorkspaceTurn rejects invalid, unknown, and internal agent ids before creating a workspace", async () => { + test("createWorkspaceTurn rejects bad agent ids without ever dispatching a turn", async () => { const config = await createTestConfig(rootDir); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); checkoutOwnerBranch(projectPath, "parent"); @@ -1225,24 +1139,34 @@ describe("TaskService", () => { workspace: { mode: "new" }, }); + // Syntactically invalid ids are checkout-independent and fail before any + // workspace exists. const invalidSyntax = await attempt("Not A Valid Id!"); expect(invalidSyntax.success).toBe(false); if (!invalidSyntax.success) expect(invalidSyntax.error).toContain("invalid agentId"); + expect(createWorkspace).not.toHaveBeenCalled(); + // Definition-dependent verdicts are decided by the created target checkout + // (owner-side prechecks are advisory): unknown and internal (ui.hidden) ids + // fail there, with the workspace retained as owned evidence and no dispatch. const unknown = await attempt("doesnotexist"); expect(unknown.success).toBe(false); - if (!unknown.success) expect(unknown.error).toContain("unknown agentId"); + if (!unknown.success) { + expect(unknown.error).toContain("unknown agentId"); + expect(unknown.error).toContain("no turn was dispatched"); + } - // Built-in internal agents (ui.hidden) must not be launchable as workspace turns. const internal = await attempt("compact"); expect(internal.success).toBe(false); - if (!internal.success) expect(internal.error).toContain("not selectable"); + if (!internal.success) { + expect(internal.error).toContain("not selectable"); + expect(internal.error).toContain("no turn was dispatched"); + } - expect(createWorkspace).not.toHaveBeenCalled(); expect(sendMessage).not.toHaveBeenCalled(); }); - test("createWorkspaceTurn rejects disabled agents before creating a workspace", async () => { + test("createWorkspaceTurn rejects disabled agents without dispatching a turn", async () => { const config = await createTestConfig(rootDir); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir, { agentAiDefaults: { custom: { enabled: false } }, @@ -1269,9 +1193,13 @@ describe("TaskService", () => { workspace: { mode: "new" }, }); + // Enablement is decided at the created target checkout (owner prechecks are + // advisory); the disabled verdict settles post-create with no dispatch. expect(result.success).toBe(false); - if (!result.success) expect(result.error).toContain("disabled"); - expect(createWorkspace).not.toHaveBeenCalled(); + if (!result.success) { + expect(result.error).toContain("disabled"); + expect(result.error).toContain("no turn was dispatched"); + } expect(sendMessage).not.toHaveBeenCalled(); }); @@ -1344,14 +1272,13 @@ describe("TaskService", () => { expect(sendMessage).toHaveBeenCalledTimes(1); }); - test("createWorkspaceTurn unreachable target respects an owner project shadow of a built-in id", async () => { + test("createWorkspaceTurn respects a project shadow of a built-in id at the target", async () => { const config = await createTestConfig(rootDir); stubStableIds(config, ["childworkspace", "turnhandle"]); const { parentId, projectPath } = await saveLocalParentWorkspace(config, rootDir); checkoutOwnerBranch(projectPath, "parent"); - // Shadow the built-in plan agent with a hidden project-local override in the OWNER's - // checkout: eligibility for unreachable targets must consult the shadow, not just the - // embedded definition. + // Shadow the built-in plan agent with a hidden project-local override: target-side + // eligibility must consult the shadow, not just the embedded definition. const agentsDir = path.join(projectPath, ".mux", "agents"); await fsPromises.mkdir(agentsDir, { recursive: true }); await fsPromises.writeFile( @@ -1391,8 +1318,10 @@ describe("TaskService", () => { }); expect(result.success).toBe(false); - if (!result.success) expect(result.error).toContain("not selectable"); - expect(createWorkspace).not.toHaveBeenCalled(); + if (!result.success) { + expect(result.error).toContain("not selectable"); + expect(result.error).toContain("no turn was dispatched"); + } expect(sendMessage).not.toHaveBeenCalled(); }); diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 42116f13a4d..2b7a5e3d13f 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -65,7 +65,6 @@ import { findWorkspaceEntry, } from "@/node/services/taskUtils"; import { listProjectMetadataRelativePaths } from "@/common/compat/legacyMux"; -import { findInitHookRelativePath } from "@/node/runtime/initHook"; import { validateWorkspaceName } from "@/common/utils/validation/workspaceValidation"; import { getTaskGroupCount } from "@/common/utils/tools/taskGroups"; import { stripTrailingSlashes } from "@/node/utils/pathUtils"; @@ -4184,7 +4183,12 @@ export class TaskService { effectiveTrunkBranch ) : undefined; + // An explicit branchName can attach the worktree to an EXISTING branch of that + // name (WorktreeManager detects and reuses it), making the trunk comparison + // above meaningless for the actual base — never vouch in that case. + const requestedBranchName = coerceNonEmptyString(args.workspace?.branchName); ownerVouchesForTargetBase = + requestedBranchName == null && ownerBranchMatchesTargetBase && ownerAgentDirsClean === true && ownerCommitMatchesOrigin === true; @@ -4201,18 +4205,14 @@ export class TaskService { ...ownerContext, }); if (!validation.success) { - // Fatal only when the owner provably equals the base AND target init cannot - // add definitions: an EXECUTABLE committed project init hook (the same - // findInitHookRelativePath rule the init runner uses — non-executable files - // never run) runs at target creation and may install the requested agent even - // though the owner (initialized before the hook existed, or with - // per-workspace conditions) legitimately lacks it. - const ownerInitHook = ownerVouchesForTargetBase - ? await findInitHookRelativePath(ownerContext.runtime, ownerContext.workspacePath) - : null; - if (ownerVouchesForTargetBase && ownerInitHook == null) { - return Err(validation.error); - } + // Owner-side misses are ALWAYS advisory: the created checkout is the only + // authoritative source of the target's agent definitions. No owner-side + // equivalence proof is sound here — worktree creation may fetch a newer + // origin commit, attach to an existing branchName, initialize submodules the + // owner never materialized, or run a committed init hook that installs the + // requested agent — so a pre-create rejection could deny a launch the real + // target would accept. Post-create validation (and, for anything it cannot + // see, the stream-time strict provenance pin) fails loudly instead. log.debug( "Task.createWorkspaceTurn: owner-side agent validation failed; deferring to the target checkout", { agentId: requestedAgentId, error: validation.error } From 502b799edffc589cd26e450d49f3aa6344aa9861 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 25 Aug 2026 18:07:52 +0000 Subject: [PATCH 24/24] =?UTF-8?q?=F0=9F=A4=96=20fix:=20classify=20strict?= =?UTF-8?q?=20agent=20contract=20failures=20as=20non-retryable=20(agent=5F?= =?UTF-8?q?resolution)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/common/orpc/schemas/errors.ts | 1 + src/common/utils/messages/retryEligibility.test.ts | 5 +++++ src/common/utils/messages/retryEligibility.ts | 1 + src/node/services/agentResolution.test.ts | 10 ++++++++-- src/node/services/agentResolution.ts | 12 ++++++------ 5 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/common/orpc/schemas/errors.ts b/src/common/orpc/schemas/errors.ts index 65ae587b48d..602a941f206 100644 --- a/src/common/orpc/schemas/errors.ts +++ b/src/common/orpc/schemas/errors.ts @@ -43,6 +43,7 @@ export const StreamErrorTypeSchema = z.enum([ "stream_truncated", // Provider stream closed before its terminal finish event "max_output_tokens", // Provider truncated the response at max_tokens (finishReason: "length") "model_refusal", // Provider declined to answer (refusal/content-filter); retrying the same request will refuse again + "agent_resolution", // Strict explicit-agent contract failure (agent missing/hidden/disabled/provenance changed); deterministic, retrying reproduces it "unknown", // Catch-all ]); diff --git a/src/common/utils/messages/retryEligibility.test.ts b/src/common/utils/messages/retryEligibility.test.ts index b53ce0e03e3..cf692065daf 100644 --- a/src/common/utils/messages/retryEligibility.test.ts +++ b/src/common/utils/messages/retryEligibility.test.ts @@ -477,11 +477,16 @@ describe("isProviderConfigFixableError", () => { "aborted", "runtime_not_ready", "model_not_found", + "agent_resolution", ]) { it(`does not flag ${type} as config-fixable`, () => { expect(isProviderConfigFixableError(type)).toBe(false); }); } + + it("flags agent_resolution as non-retryable (deterministic strict contract failure)", () => { + expect(isNonRetryableStreamError({ type: "agent_resolution" })).toBe(true); + }); }); describe("isNonRetryableSendError", () => { diff --git a/src/common/utils/messages/retryEligibility.ts b/src/common/utils/messages/retryEligibility.ts index 5753f78eda2..f6726f2d7f6 100644 --- a/src/common/utils/messages/retryEligibility.ts +++ b/src/common/utils/messages/retryEligibility.ts @@ -53,6 +53,7 @@ const NON_RETRYABLE_STREAM_ERRORS = [ "aborted", // User cancelled - should not auto-retry "runtime_not_ready", // Container/runtime unavailable - permanent failure "model_refusal", // Provider declined to answer - retrying the same request will refuse again + "agent_resolution", // Strict explicit-agent contract failure - deterministic, retrying reproduces it ] as const satisfies readonly StreamErrorType[]; const NON_RETRYABLE_STREAM_ERROR_SET = new Set(NON_RETRYABLE_STREAM_ERRORS); diff --git a/src/node/services/agentResolution.test.ts b/src/node/services/agentResolution.test.ts index a6106d396b9..d64af1e3a50 100644 --- a/src/node/services/agentResolution.test.ts +++ b/src/node/services/agentResolution.test.ts @@ -498,6 +498,7 @@ describe("resolveAgentForStream strict resolution", () => { }>; }; agentAiDefaults?: ProjectsConfig["agentAiDefaults"]; + onError?: (event: { errorType?: string }) => void; }) { const cfg: ProjectsConfig = { projects: new Map([ @@ -523,7 +524,7 @@ describe("resolveAgentForStream strict resolution", () => { strictAgentResolution: params.strictAgentResolution, callerToolPolicy: undefined, cfg, - emitError: () => undefined, + emitError: (event) => params.onError?.(event), isAdvisorExperimentEnabled: false, }); } @@ -543,11 +544,15 @@ describe("resolveAgentForStream strict resolution", () => { if (lenient.success) expect(lenient.data.effectiveAgentId).toBe("exec"); // Strict mode (explicit workspace-turn overrides): running a different agent - // than requested must fail the stream, not silently swap in exec. + // than requested must fail the stream, not silently swap in exec — and the + // failure is deterministic, so it must be classified non-retryable + // (agent_resolution) instead of the retryable catch-all. + const emittedErrorTypes: Array = []; const strict = await resolveTopLevel({ projectPath, agentId: "doesnotexist", strictAgentResolution: true, + onError: (event) => emittedErrorTypes.push(event.errorType), }); expect(strict.success).toBe(false); if (!strict.success && strict.error.type === "unknown") { @@ -555,6 +560,7 @@ describe("resolveAgentForStream strict resolution", () => { } else { expect(strict.success === false && strict.error.type).toBe("unknown"); } + expect(emittedErrorTypes).toEqual(["agent_resolution"]); }); test("hidden explicit agent fails loudly in strict mode for top-level workspaces", async () => { diff --git a/src/node/services/agentResolution.ts b/src/node/services/agentResolution.ts index 3b318c8aa96..69ef9729d7f 100644 --- a/src/node/services/agentResolution.ts +++ b/src/node/services/agentResolution.ts @@ -291,7 +291,7 @@ export async function resolveAgentForStream( createErrorEvent(workspaceId, { messageId: createAssistantMessageId(), error: errorMessage, - errorType: "unknown", + errorType: "agent_resolution", }) ); return Err({ type: "unknown", raw: errorMessage }); @@ -324,7 +324,7 @@ export async function resolveAgentForStream( createErrorEvent(workspaceId, { messageId: createAssistantMessageId(), error: errorMessage, - errorType: "unknown", + errorType: "agent_resolution", }) ); return Err({ type: "unknown", raw: errorMessage }); @@ -363,7 +363,7 @@ export async function resolveAgentForStream( createErrorEvent(workspaceId, { messageId: createAssistantMessageId(), error: errorMessage, - errorType: "unknown", + errorType: "agent_resolution", }) ); return Err({ type: "unknown", raw: errorMessage }); @@ -384,7 +384,7 @@ export async function resolveAgentForStream( createErrorEvent(workspaceId, { messageId: errorMessageId, error: errorMessage, - errorType: "unknown", + errorType: strictTopLevel ? "agent_resolution" : "unknown", }) ); return Err({ type: "unknown", raw: errorMessage }); @@ -412,7 +412,7 @@ export async function resolveAgentForStream( createErrorEvent(workspaceId, { messageId: createAssistantMessageId(), error: errorMessage, - errorType: "unknown", + errorType: "agent_resolution", }) ); return Err({ type: "unknown", raw: errorMessage }); @@ -467,7 +467,7 @@ export async function resolveAgentForStream( createErrorEvent(workspaceId, { messageId: createAssistantMessageId(), error: errorMessage, - errorType: "unknown", + errorType: "agent_resolution", }) ); return Err({ type: "unknown", raw: errorMessage });