diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 14c02c8ab..1e34f0acc 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -26,6 +26,9 @@ ### Fixed - Fixed xdev-routed checkpoint and rewind writes not tracking checkpoint state and leaving rewinding results in rebuilt provider and session context. +### Fixed + +- Fixed the built-in advisor silently doing nothing when its model routes through the `cursor` provider: the advisor runs in its own `Agent` that was constructed without `cursorExecHandlers`, so on Cursor — where every tool executes server-side and is dispatched back through the client's exec handlers — each advisor tool call (including the MCP `advise` tool) came back `toolNotFound`/"tool not available" and no advice was ever routed. The advisor `Agent` now gets a Cursor exec bridge scoped to its own granted tool set, mirroring the primary agent. The bridge's native `delete` frame is gated so a read-only advisor cannot delete workspace files it was never granted a mutating tool for ([#5680](https://github.com/can1357/oh-my-pi/issues/5680)). ## [17.0.1] - 2026-07-16 diff --git a/packages/coding-agent/src/advisor/__tests__/advisor.test.ts b/packages/coding-agent/src/advisor/__tests__/advisor.test.ts index 5d521c50d..b81521854 100644 --- a/packages/coding-agent/src/advisor/__tests__/advisor.test.ts +++ b/packages/coding-agent/src/advisor/__tests__/advisor.test.ts @@ -497,6 +497,19 @@ describe("advisor", () => { expect(message.content).toBe(originalContent); }); + it("leaves an authorized Cursor native delete call intact", () => { + const message = { + role: "assistant", + content: [{ type: "toolCall", id: "tc-delete", name: "delete", arguments: { path: "obsolete.txt" } }], + stopReason: "toolUse", + } as unknown as AssistantMessage; + const originalContent = message.content; + + expect(quarantineAdvisorUnsafeOutput(message, new Set(["advise", "write", "delete"]))).toBeUndefined(); + expect(message.stopReason).toBe("toolUse"); + expect(message.content).toBe(originalContent); + }); + it("sanitizes destructive advise notes even when advise is an allowed tool", () => { const message = { role: "assistant", diff --git a/packages/coding-agent/src/cursor.ts b/packages/coding-agent/src/cursor.ts index 88ad3a27d..5c4d949fa 100644 --- a/packages/coding-agent/src/cursor.ts +++ b/packages/coding-agent/src/cursor.ts @@ -18,10 +18,20 @@ import { resolveToCwd } from "./tools/path-utils"; interface CursorExecBridgeOptions { cwd: string; + getCwd?: () => string; tools: Map; getTool?: (name: string) => AgentTool | undefined; getToolContext?: () => AgentToolContext | undefined; emitEvent?: (event: AgentEvent) => void; + /** + * Whether the Cursor native `delete` frame may remove files. Unlike every + * other exec handler, `executeDelete` mutates the filesystem directly instead + * of consulting {@link tools}, so a background read-only advisor could delete + * workspace files it was never granted a mutating tool for (issue #5680 + * review). Defaults to allowed to preserve the primary agent's behavior; + * callers with a restricted tool set (advisors) opt out. + */ + allowNativeDelete?: boolean; } function createToolResultMessage( @@ -107,9 +117,15 @@ async function executeTool( async function executeDelete(options: CursorExecBridgeOptions, pathArg: string, toolCallId: string) { const toolName = "delete"; + + if (options.allowNativeDelete === false) { + const result = buildToolErrorResult(`Tool "${toolName}" not available`); + return createToolResultMessage(toolCallId, toolName, result, true); + } + options.emitEvent?.({ type: "tool_execution_start", toolCallId, toolName, args: { path: pathArg } }); - const absolutePath = resolveToCwd(pathArg, options.cwd); + const absolutePath = resolveToCwd(pathArg, options.getCwd?.() ?? options.cwd); let isError = false; let result: AgentToolResult; diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 8348a3858..8228409bd 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -197,6 +197,7 @@ import { onModelRolesChanged, validateProviderMaxInFlightRequests, } from "../config/settings"; +import { CursorExecHandlers } from "../cursor"; import { RawSseDebugBuffer } from "../debug/raw-sse-buffer"; import { expandApplyPatchToEntries, normalizeDiff, normalizeToLF, ParseError, previewPatch, stripBom } from "../edit"; import { getFileSnapshotStore } from "../edit/file-snapshot-store"; @@ -2981,11 +2982,16 @@ export class AgentSession { const names = config.tools === undefined ? ADVISOR_DEFAULT_TOOL_NAMES : new Set(config.tools); const tools = (this.#advisorTools ?? []).filter(t => names.has(t.name)); + const advisorLoopTools: AgentTool[] = [adviseTool, ...tools]; + const advisorToolMap = new Map(); const availableAdvisorToolNames = new Set(); - availableAdvisorToolNames.add(adviseTool.name); - for (const tool of tools) { + for (const tool of advisorLoopTools) { availableAdvisorToolNames.add(tool.name); - if (tool.customWireName !== undefined) availableAdvisorToolNames.add(tool.customWireName); + advisorToolMap.set(tool.name, tool); + if (tool.customWireName !== undefined) { + availableAdvisorToolNames.add(tool.customWireName); + advisorToolMap.set(tool.customWireName, tool); + } } let quarantinedAdvisorOutput: string | undefined; let currentAdvisorInput = ""; @@ -3025,17 +3031,36 @@ export class AgentSession { // Codex request identity remains UUID-shaped while local labels keep the // `-advisor` suffix. const advisorPromptCacheKey = this.agent.promptCacheKey ?? advisorProviderSessionId; + // On the Cursor provider every tool runs server-side and is dispatched + // back through `cursorExecHandlers`; without this bridge the advisor's + // own tools (including the MCP `advise` tool) return `toolNotFound` and + // no advice is ever routed (issue #5680). Mirrors the primary agent's + // bridge (`sdk.ts`), scoped to this advisor's granted tool set. + // Cursor's native `delete` frame removes files directly, bypassing the + // tool map, so gate it on the advisor actually holding a file-mutating + // tool. A default read-only advisor (advise/read/grep/glob) never gets + // to delete workspace files it was never granted (issue #5680 review). + const advisorCanMutateFiles = advisorToolMap.has("write") || advisorToolMap.has("edit"); + if (advisorCanMutateFiles) availableAdvisorToolNames.add("delete"); + const advisorCursorExecHandlers = new CursorExecHandlers({ + cwd: this.sessionManager.getCwd(), + getCwd: () => this.sessionManager.getCwd(), + tools: advisorToolMap, + allowNativeDelete: advisorCanMutateFiles, + }); const advisorAgent = new Agent({ initialState: { systemPrompt, model: advisorModel, thinkingLevel: toReasoningEffort(advisorThinkingLevel), - tools: [adviseTool, ...tools], + tools: advisorLoopTools, }, appendOnlyContext, sessionId: advisorProviderSessionId, promptCacheKey: advisorPromptCacheKey, providerSessionState: this.#providerSessionState, + cursorExecHandlers: advisorCursorExecHandlers, + cwdResolver: () => this.sessionManager.getCwd(), preferWebsockets: this.#preferWebsockets, getApiKey: requestModel => this.#modelRegistry.resolver(requestModel, advisorProviderSessionId), streamFn: this.#advisorStreamFn, diff --git a/packages/coding-agent/test/cursor-exec.test.ts b/packages/coding-agent/test/cursor-exec.test.ts index 9702fd53c..6d9d20ed5 100644 --- a/packages/coding-agent/test/cursor-exec.test.ts +++ b/packages/coding-agent/test/cursor-exec.test.ts @@ -2,9 +2,20 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; -import { create } from "@bufbuild/protobuf"; +import { create, fromBinary } from "@bufbuild/protobuf"; import type { AgentEvent, AgentTool, AgentToolContext } from "@oh-my-pi/pi-agent-core"; -import { ReadArgsSchema, ShellArgsSchema } from "@oh-my-pi/pi-catalog/discovery/cursor-gen/agent_pb"; +import { type BlockState, handleServerMessage, type ToolCallState } from "@oh-my-pi/pi-ai/providers/cursor"; +import type { AssistantMessage } from "@oh-my-pi/pi-ai/types"; +import { AssistantMessageEventStream } from "@oh-my-pi/pi-ai/utils/event-stream"; +import { + AgentClientMessageSchema, + AgentServerMessageSchema, + DeleteArgsSchema, + ExecServerMessageSchema, + McpArgsSchema, + ReadArgsSchema, + ShellArgsSchema, +} from "@oh-my-pi/pi-catalog/discovery/cursor-gen/agent_pb"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { CursorExecHandlers } from "@oh-my-pi/pi-coding-agent/cursor"; import type { ExtensionRunner } from "@oh-my-pi/pi-coding-agent/extensibility/extensions"; @@ -12,6 +23,7 @@ import { ExtensionToolWrapper } from "@oh-my-pi/pi-coding-agent/extensibility/ex import { GrepTool, type ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { removeWithRetries } from "@oh-my-pi/pi-utils"; import { type } from "arktype"; +import { AdviseTool } from "../src/advisor/advise-tool"; function createTestSession(cwd: string, overrides: Partial = {}): ToolSession { return { @@ -191,3 +203,225 @@ describe("CursorExecHandlers mounted tool bridge", () => { expect(result.content.find(block => block.type === "text")?.text).toContain("blocked by user policy"); }); }); + +function cursorAssistantMessage(): AssistantMessage { + return { + role: "assistant", + content: [], + api: "cursor-agent", + provider: "cursor", + model: "gpt-5.6-sol-medium", + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + stopReason: "stop", + timestamp: 0, + }; +} + +function newBlockState(): BlockState { + let textBlock: BlockState["currentTextBlock"] = null; + let thinkingBlock: BlockState["currentThinkingBlock"] = null; + let toolCall: ToolCallState | null = null; + return { + get currentTextBlock() { + return textBlock; + }, + get currentThinkingBlock() { + return thinkingBlock; + }, + get currentToolCall() { + return toolCall; + }, + firstTokenTime: undefined, + resolvedMcpToolCallIds: new Set(), + setTextBlock: b => { + textBlock = b; + }, + setThinkingBlock: b => { + thinkingBlock = b; + }, + setToolCall: t => { + toolCall = t; + }, + setFirstTokenTime: () => {}, + }; +} + +// Regression for issue #5680: the advisor's own tools run through the same +// Cursor exec bridge the primary agent uses. Without a bridge wired into the +// advisor Agent, the server's `mcpArgs` dispatch for `advise` comes back +// `toolNotFound` and no advice is ever routed. This drives the real provider +// dispatch to prove a bridge built over the advisor's tool set executes the +// `advise` MCP call and returns a success frame. +describe("CursorExecHandlers advise routing (issue #5680)", () => { + function adviseServerMessage(note: string) { + return create(AgentServerMessageSchema, { + message: { + case: "execServerMessage", + value: create(ExecServerMessageSchema, { + id: 1, + execId: "exec-advise-1", + message: { + case: "mcpArgs", + value: create(McpArgsSchema, { + name: "advise", + toolName: "advise", + toolCallId: "call-advise-1", + providerIdentifier: "pi-agent", + args: { note: new TextEncoder().encode(JSON.stringify(note)) }, + }), + }, + }), + }, + }); + } + + function decodeMcpResultCase(chunk: unknown): string | undefined { + const buf = chunk as Buffer; + const client = fromBinary(AgentClientMessageSchema, buf.subarray(5)); + if (client.message.case !== "execClientMessage") return undefined; + const exec = client.message.value; + return exec.message.case === "mcpResult" ? exec.message.value.result.case : undefined; + } + + it("executes the advise MCP call through the bridge and routes the note", async () => { + const advised: Array<{ note: string; severity?: string }> = []; + const adviseTool = new AdviseTool((note, severity) => advised.push({ note, severity })); + const handlers = new CursorExecHandlers({ + cwd: ".", + tools: new Map([["advise", adviseTool as unknown as AgentTool]]), + }); + + const output = cursorAssistantMessage(); + const stream = new AssistantMessageEventStream(); + const state = newBlockState(); + const written: unknown[] = []; + const h2Request = { + write: (chunk: unknown) => { + written.push(chunk); + return true; + }, + } as unknown as Parameters[5]; + + await handleServerMessage( + adviseServerMessage("Consider the empty-input edge case"), + output, + stream, + state, + new Map(), + h2Request, + handlers, + undefined, + { sawTokenDelta: false }, + [], + ); + + expect(advised).toEqual([{ note: "Consider the empty-input edge case", severity: undefined }]); + expect(written.length).toBe(1); + expect(decodeMcpResultCase(written[0])).toBe("success"); + }); + + it("returns toolNotFound when no bridge is wired (the unfixed advisor path)", async () => { + const output = cursorAssistantMessage(); + const stream = new AssistantMessageEventStream(); + const state = newBlockState(); + const written: unknown[] = []; + const h2Request = { + write: (chunk: unknown) => { + written.push(chunk); + return true; + }, + } as unknown as Parameters[5]; + + await handleServerMessage( + adviseServerMessage("never delivered"), + output, + stream, + state, + new Map(), + h2Request, + undefined, + undefined, + { sawTokenDelta: false }, + [], + ); + + expect(written.length).toBe(1); + expect(decodeMcpResultCase(written[0])).toBe("toolNotFound"); + }); +}); + +// Regression for the #5686 review: Cursor's native `delete` frame removes files +// directly (bypassing the tool map), so a read-only advisor that was granted no +// mutating tool must not be able to delete workspace files. +describe("CursorExecHandlers native delete gating (issue #5680)", () => { + let cwd: string; + + beforeEach(async () => { + cwd = await fs.mkdtemp(path.join(os.tmpdir(), "cursor-delete-test-")); + }); + + afterEach(async () => { + await removeWithRetries(cwd); + }); + + it("rejects native delete and preserves the file when allowNativeDelete is false", async () => { + const target = path.join(cwd, "victim.txt"); + await Bun.write(target, "keep me"); + const handlers = new CursorExecHandlers({ + cwd, + tools: new Map(), + allowNativeDelete: false, + }); + + const result = await handlers.delete(create(DeleteArgsSchema, { toolCallId: "call-del", path: target })); + + expect(result.isError).toBe(true); + expect(result.content).toEqual([{ type: "text", text: 'Tool "delete" not available' }]); + expect(await Bun.file(target).exists()).toBe(true); + }); + + it("performs native delete when allowNativeDelete is true", async () => { + const target = path.join(cwd, "victim.txt"); + await Bun.write(target, "remove me"); + const handlers = new CursorExecHandlers({ + cwd, + tools: new Map(), + allowNativeDelete: true, + }); + + const result = await handlers.delete(create(DeleteArgsSchema, { toolCallId: "call-del", path: target })); + + expect(result.isError).toBe(false); + expect(await Bun.file(target).exists()).toBe(false); + }); + + it("resolves native deletes through the live cwd resolver", async () => { + const movedCwd = path.join(cwd, "moved"); + await fs.mkdir(movedCwd); + const originalTarget = path.join(cwd, "obsolete.txt"); + const movedTarget = path.join(movedCwd, "obsolete.txt"); + await Bun.write(originalTarget, "preserve me"); + await Bun.write(movedTarget, "remove me"); + let currentCwd = cwd; + const handlers = new CursorExecHandlers({ + cwd, + getCwd: () => currentCwd, + tools: new Map(), + allowNativeDelete: true, + }); + + currentCwd = movedCwd; + const result = await handlers.delete(create(DeleteArgsSchema, { toolCallId: "call-del", path: "obsolete.txt" })); + + expect(result.isError).toBe(false); + expect(await Bun.file(originalTarget).exists()).toBe(true); + expect(await Bun.file(movedTarget).exists()).toBe(false); + }); +});