From 2e874397127d47444f3fcb0047900c2cad086538 Mon Sep 17 00:00:00 2001 From: James3014 Date: Sat, 26 Sep 2026 10:24:47 +0800 Subject: [PATCH] fix: harden dispatch replay and minimal discovery (#194) --- src/pi-tools.test.ts | 45 ++++++++++++++++++++++++++++++++++++++++++++ src/pi-tools.ts | 39 +++++++++++++++++++++++++++++++++++--- src/server.test.ts | 30 +++++++++++++++++++++-------- src/server.ts | 19 ++++++++++--------- 4 files changed, 113 insertions(+), 20 deletions(-) create mode 100644 src/pi-tools.test.ts diff --git a/src/pi-tools.test.ts b/src/pi-tools.test.ts new file mode 100644 index 000000000..370903fbd --- /dev/null +++ b/src/pi-tools.test.ts @@ -0,0 +1,45 @@ +import assert from "node:assert/strict"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; +import { setTimeout as delay } from "node:timers/promises"; +import { grepFilesTool, listDirectoryTool } from "./pi-tools.js"; + +function text(response: { content: Array<{ type: string; text?: string }> }): string { + return response.content.flatMap((part) => part.type === "text" ? [part.text ?? ""] : []).join("\n"); +} + +test("native discovery propagates cancellation with an attributable error and no mutation", async () => { + const root = await mkdtemp(join(tmpdir(), "devspace-pi-cancel-")); + try { + await writeFile(join(root, "fixture.txt"), "alpha|beta\n"); + const controller = new AbortController(); + controller.abort(new Error("fixture cancellation")); + const response = await listDirectoryTool( + { path: "." }, + { cwd: root, root, signal: controller.signal }, + ); + assert.equal(response.isError, true); + assert.match(text(response), /DISCOVERY_CANCELLED/); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("native discovery classifies an already-expired AbortSignal as a timeout", async () => { + const root = await mkdtemp(join(tmpdir(), "devspace-pi-timeout-")); + try { + await writeFile(join(root, "fixture.txt"), "alpha|beta\n"); + const signal = AbortSignal.timeout(1); + await delay(5); + const response = await grepFilesTool( + { pattern: "alpha|beta", path: ".", limit: 20 }, + { cwd: root, root, signal }, + ); + assert.equal(response.isError, true); + assert.match(text(response), /DISCOVERY_TIMEOUT/); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/src/pi-tools.ts b/src/pi-tools.ts index 238b9c547..eaeebe806 100644 --- a/src/pi-tools.ts +++ b/src/pi-tools.ts @@ -29,8 +29,11 @@ interface ToolContext { cwd: string; root: string; readRoots?: string[]; + signal?: AbortSignal; } +const DISCOVERY_TIMEOUT_MS = 10_000; + function toMcpContent(result: AgentToolResult): McpContent[] { return result.content.map((content) => { if (content.type === "text") { @@ -50,6 +53,36 @@ function formatToolError(error: unknown): McpContent[] { return [{ type: "text", text: message }]; } +function discoveryAbortContent(signal: AbortSignal): McpContent[] | undefined { + if (!signal.aborted) return undefined; + const reason = signal.reason; + const name = reason instanceof Error ? reason.name : ""; + const message = reason instanceof Error ? reason.message : String(reason ?? "aborted"); + const code = name === "TimeoutError" ? "DISCOVERY_TIMEOUT" : "DISCOVERY_CANCELLED"; + return [{ type: "text", text: `[${code}] Read-only discovery stopped: ${message}` }]; +} + +async function runDiscoveryTool( + execute: (input: TInput, signal: AbortSignal) => Promise>, + input: TInput, + context: ToolContext, +): Promise> { + const timeoutSignal = AbortSignal.timeout(DISCOVERY_TIMEOUT_MS); + const signal = context.signal ? AbortSignal.any([context.signal, timeoutSignal]) : timeoutSignal; + const alreadyAborted = discoveryAbortContent(signal); + if (alreadyAborted) return { content: alreadyAborted, isError: true }; + try { + const result = await execute(input, signal); + return { + content: toMcpContent(result), + details: result.details, + }; + } catch (error) { + const aborted = discoveryAbortContent(signal); + return { content: aborted ?? formatToolError(error), isError: true }; + } +} + async function runTool( execute: (input: TInput) => Promise>, input: TInput, @@ -101,21 +134,21 @@ export async function grepFilesTool(input: GrepToolInput, context: ToolContext): if (input.path) resolveAllowedPath(input.path, context.cwd, [context.root]); const tool = createGrepTool(context.cwd); - return runTool((params) => tool.execute("grep_files", params), input, context); + return runDiscoveryTool((params, signal) => tool.execute("grep_files", params, signal), input, context); } export async function findFilesTool(input: FindToolInput, context: ToolContext): Promise { if (input.path) resolveAllowedPath(input.path, context.cwd, [context.root]); const tool = createFindTool(context.cwd); - return runTool((params) => tool.execute("find_files", params), input, context); + return runDiscoveryTool((params, signal) => tool.execute("find_files", params, signal), input, context); } export async function listDirectoryTool(input: LsToolInput, context: ToolContext): Promise { if (input.path) resolveAllowedPath(input.path, context.cwd, [context.root]); const tool = createLsTool(context.cwd); - return runTool((params) => tool.execute("list_directory", params), input, context); + return runDiscoveryTool((params, signal) => tool.execute("list_directory", params, signal), input, context); } export async function runShellTool(input: BashToolInput, context: ToolContext): Promise { diff --git a/src/server.test.ts b/src/server.test.ts index 54edf7584..f3ceb2b23 100644 --- a/src/server.test.ts +++ b/src/server.test.ts @@ -1985,6 +1985,7 @@ test("repository write and shell sink families reject an unbound caller before e workspaceId, profile: "mutator", prompt: "must not launch without Core binding", + attemptKey: "core-unbound-agent-start", executionContract: { writePaths: ["AGENTS.md"] }, }, _meta: conversation, @@ -2353,6 +2354,10 @@ test("subagents enabled: agent tools are present and functional", async (t) => { assert.ok(startProps.effort, "agent_start must advertise direct effort selection"); assert.ok(!(startTool.inputSchema.required as string[] | undefined)?.includes("profile"), "profile must be optional for direct dispatch"); assert.ok(startProps.attemptKey); + assert.ok( + (startTool.inputSchema.required as string[] | undefined)?.includes("attemptKey"), + "agent_start must require a physical replay identity before any provider side effect", + ); const continueProps = continueTool.inputSchema.properties as Record; assert.equal(continueProps.workspaceRoot, undefined); @@ -2398,16 +2403,17 @@ test("subagents enabled: agent tools are present and functional", async (t) => { provider: "codex", model: "gpt-test", prompt: "invalid selector", + attemptKey: "invalid-selector-attempt", }, }); assert.equal(invalidSelector.isError, true); assert.match(responseText(invalidSelector), /either profile|both provider and model/i); for (const arguments_ of [ - { workspaceId, prompt: "missing selector" }, - { workspaceId, provider: "codex", prompt: "missing model" }, - { workspaceId, profile: "reviewer", effort: "high", prompt: "profile effort mismatch" }, - { workspaceId, provider: "codex", model: "", prompt: "empty model" }, + { workspaceId, prompt: "missing selector", attemptKey: "missing-selector-attempt" }, + { workspaceId, provider: "codex", prompt: "missing model", attemptKey: "missing-model-attempt" }, + { workspaceId, profile: "reviewer", effort: "high", prompt: "profile effort mismatch", attemptKey: "profile-effort-attempt" }, + { workspaceId, provider: "codex", model: "", prompt: "empty model", attemptKey: "empty-model-attempt" }, ]) { const rejected = await context.client.callTool({ name: "agent_start", arguments: arguments_ }); assert.equal(rejected.isError, true); @@ -2594,7 +2600,7 @@ test("subagents: legacy durable session without execution generation does not si const workspaceId = structuredContent(opened).workspaceId as string; const start = await context.client.callTool({ name: "agent_start", - arguments: { workspaceId, profile: "reviewer", prompt: "legacy simulation" }, + arguments: { workspaceId, profile: "reviewer", prompt: "legacy simulation", attemptKey: "legacy-generation-start" }, }); const agentId = structuredContent(start).agentId as string; @@ -2642,6 +2648,7 @@ test("subagents: unknown/invalid workspaceId fails closed before durable-agent a workspaceId: invalidWorkspaceId, profile: "reviewer", prompt: "fail prompt", + attemptKey: "invalid-workspace-start", }, }); assert.equal(startRes.isError, true); @@ -2928,6 +2935,7 @@ test("subagents: agent_start executionContract expectedHead mismatch fails close workspaceId, profile: "reviewer", prompt: "work", + attemptKey: "expected-head-stale", executionContract: { expectedHead: "a".repeat(40), writePaths: ["src"] }, }, }); @@ -2941,6 +2949,7 @@ test("subagents: agent_start executionContract expectedHead mismatch fails close workspaceId, profile: "reviewer", prompt: "work", + attemptKey: "expected-head-current", executionContract: { expectedHead: head.stdout.trim(), writePaths: ["src"] }, }, }); @@ -2955,7 +2964,7 @@ test("subagents: agent_reconcile reports physical diff as candidate evidence", a const startResult = await context.client.callTool({ name: "agent_start", - arguments: { workspaceId, profile: "reviewer", prompt: "do work" }, + arguments: { workspaceId, profile: "reviewer", prompt: "do work", attemptKey: "reconcile-physical-diff" }, }); const agentId = (structuredContent(startResult) as Record).agentId as string; @@ -3840,6 +3849,7 @@ test("agent_start MCP transports durable tool authority and projection into the workspaceId, profile: "reviewer", prompt: "transport the bounded tool projection", + attemptKey: "tool-projection-start", executionContract: { authorityMode: "OWNER_DIRECT", authorizedToolCeiling: ["workspace.search_text", "workspace.read"], @@ -3884,6 +3894,7 @@ test("agent_start MCP transports durable tool authority and projection into the workspaceId, profile: "reviewer", prompt: "reject provider-native tool id", + attemptKey: "provider-native-tool-reject", executionContract: { authorizedToolCeiling: ["codex.shell"], }, @@ -3905,7 +3916,7 @@ test("direct agent selectors reject disabled providers before preflight", async assert.match(responseText(result), /provider 'claude' is disabled/i); const start = await context.client.callTool({ name: "agent_start", - arguments: { workspaceId, provider: "claude", model: "claude-test", prompt: "must be rejected" }, + arguments: { workspaceId, provider: "claude", model: "claude-test", prompt: "must be rejected", attemptKey: "disabled-provider-start" }, }); assert.equal(start.isError, true); assert.match(responseText(start), /provider 'claude' is disabled/i); @@ -4318,8 +4329,11 @@ test("command_status metadata annotations and minimal mode visibility", async (t const toolsList = await context.client.listTools(); const toolNames = toolsList.tools.map((t) => t.name); - // command_status is visible in minimal mode for read-only reconciliation + // command_status and native read-only discovery are visible in minimal mode. assert.ok(toolNames.includes("command_status"), "command_status should be visible in minimal mode"); + assert.ok(toolNames.includes("grep"), "grep should be visible in minimal mode"); + assert.ok(toolNames.includes("glob"), "glob should be visible in minimal mode"); + assert.ok(toolNames.includes("ls"), "ls should be visible in minimal mode"); // exec_command and write_stdin remain hidden in minimal mode assert.ok(!toolNames.includes("exec_command"), "exec_command must stay hidden in minimal mode"); diff --git a/src/server.ts b/src/server.ts index 7253e2add..4561002a3 100644 --- a/src/server.ts +++ b/src/server.ts @@ -456,9 +456,7 @@ function serverInstructions(config: ServerConfig): string { return `Use DevSpace for coding work. Call ${toolNames.openWorkspace} once for each project folder or isolated worktree, then keep using its workspaceId. During continued work in the same project or worktree, do not call ${toolNames.openWorkspace} again. Open another workspace only when changing projects, switching checkout/worktree mode, creating another isolated worktree, or when the current workspaceId is rejected. Use ${toolNames.read} for direct file reads, apply_patch for all file modifications, exec_command for inspection, tests, builds, and other commands, and write_stdin to poll or interact with running processes. Follow instructions returned by ${toolNames.openWorkspace}; read applicable instruction and skill files before working in their scope.${artifactInstruction}${showChangesInstruction}${agentToolsInstruction}${gitCandidatesInstruction}${codexGoalsInstruction}${repositoryIntelligenceInstruction}`; } - const inspection = config.toolMode !== "full" - ? `In minimal tool mode, ${toolNames.grep}, ${toolNames.glob}, and ${toolNames.ls} are disabled; use ${toolNames.shell} with command-line tools such as grep, rg, find, ls, and tree for search and directory inspection. ` - : `Prefer ${toolNames.read}, ${toolNames.grep}, ${toolNames.glob}, and ${toolNames.ls} for file inspection. `; + const inspection = `Prefer ${toolNames.read}, ${toolNames.grep}, ${toolNames.glob}, and ${toolNames.ls} for bounded read-only file inspection. Use ${toolNames.shell} only when shell semantics are actually needed. `; const skills = config.skillsEnabled ? `When ${toolNames.openWorkspace} returns available skills and a task matches a skill, use ${toolNames.read} to read that skill's path before proceeding. Skill paths may be outside the workspace, but ${toolNames.read} only permits advertised SKILL.md files and files under already-loaded skill directories. ` @@ -2803,8 +2801,8 @@ function createAgentStartInputSchema() { workspaceId: z.string().describe("Workspace identifier returned by open_workspace."), ...selectorShape.shape, prompt: z.string().describe("Task prompt for the agent."), - attemptKey: z.string().regex(/^[A-Za-z0-9][A-Za-z0-9._:/-]{0,127}$/).optional().describe( - "Optional physical-workspace-scoped replay identity. Exact request replays reuse one durable agent; conflicting reuse fails closed.", + attemptKey: z.string().regex(/^[A-Za-z0-9][A-Za-z0-9._:/-]{0,127}$/).describe( + "Required physical-workspace-scoped replay identity. Exact request replays reuse one durable agent; conflicting reuse fails closed.", ), executionContract, }; @@ -4619,7 +4617,7 @@ export function createMcpServer( ); } - if (config.toolMode === "full") { + if (config.toolMode !== "codex") { registerAppTool( server, toolNames.grep, @@ -4644,13 +4642,14 @@ export function createMcpServer( ...toolWidgetDescriptorMeta(config, "search"), annotations: { readOnlyHint: true }, }, - async ({ workspaceId, ...input }) => { + async ({ workspaceId, ...input }, extra) => { const startedAt = performance.now(); const workspace = workspaces.getWorkspace(workspaceId); if (input.path) workspaces.resolvePath(workspace, input.path); const response = await grepFilesTool(input, { cwd: workspace.root, root: workspace.root, + signal: extra.signal, }); if (response.isError) { @@ -4714,13 +4713,14 @@ export function createMcpServer( ...toolWidgetDescriptorMeta(config, "search"), annotations: { readOnlyHint: true }, }, - async ({ workspaceId, ...input }) => { + async ({ workspaceId, ...input }, extra) => { const startedAt = performance.now(); const workspace = workspaces.getWorkspace(workspaceId); if (input.path) workspaces.resolvePath(workspace, input.path); const response = await findFilesTool(input, { cwd: workspace.root, root: workspace.root, + signal: extra.signal, }); if (response.isError) { @@ -4784,13 +4784,14 @@ export function createMcpServer( ...toolWidgetDescriptorMeta(config, "directory"), annotations: { readOnlyHint: true }, }, - async ({ workspaceId, ...input }) => { + async ({ workspaceId, ...input }, extra) => { const startedAt = performance.now(); const workspace = workspaces.getWorkspace(workspaceId); workspaces.resolvePath(workspace, input.path); const response = await listDirectoryTool(input, { cwd: workspace.root, root: workspace.root, + signal: extra.signal, }); if (response.isError) {