From 5779c762de1d8b57d4238cffc21cedbfeeb088fe Mon Sep 17 00:00:00 2001 From: Diogo Soares Rodrigues Date: Mon, 27 Jul 2026 11:56:32 -0300 Subject: [PATCH] fix(cursor): give advisors the replace-mode edit pi_edit needs The primary bridge builds a `replace`-mode `EditTool` because `PiEditExecArgs` carries `old_text`/`new_text` pairs that no other mode accepts. The advisor roster passed its own instances straight through, and those follow the session's configured `edit.mode` - `hashline` by default, whose schema is a single `input` string - so every native advisor edit failed validation instead of touching the file. Both bridge-only tools now come from `cursor-bridge-tools.ts`: `createBridgeEditTool` builds the wrapped `replace` instance, and `bridgeToolMap` substitutes it into a granted map. The substitution is gated on `edit` actually having been granted, since the tool is constructed rather than looked up - handing one to a read-only roster is the #5680 escalation. The advisor's own loop keeps its instance; only the exec map is swapped. (cherry picked from commit e6cf9f8046c595cab9793b4b2a5795d8488d8d22) --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/cursor-bridge-tools.ts | 43 +++++++++ packages/coding-agent/src/sdk.ts | 8 +- .../src/session/agent-session-types.ts | 6 ++ .../coding-agent/src/session/agent-session.ts | 1 + .../src/session/session-advisors.ts | 18 +++- .../coding-agent/test/cursor-exec.test.ts | 95 ++++++++++++++++--- 7 files changed, 153 insertions(+), 19 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b2f2cf7a1..31caf3fff 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -139,6 +139,7 @@ - Fixed every Cursor `pi_edit` frame failing instead of editing. Two independent causes: the session drops `edit` from the tool registry for Cursor so the model uses full-file `write`, but that registry is also the exec bridge's tool source, so the native frame — which the server sends regardless of the advertised catalog — found no tool; and the retained instance followed the session's configured edit mode, while `PiEditExecArgs` carries `old_text`/`new_text` pairs that only `replace` accepts (the default `hashline` takes a single `input` string). The bridge now resolves a `replace`-mode instance through its fallback resolver, still wrapped for approval. - Fixed a `pi_grep` frame carrying `context` or `limit` escaping the approval gate. Honoring those fields needs a per-call `grep`, and the per-call instance was built raw while every registry tool is wrapped, so such calls bypassed `tools.approval.grep` and the exec-tier check for SSH-targeted paths. Both bridge callsites now build it through one shared factory that applies the same wrapper. - Fixed Cursor advisors ignoring `pi_grep`'s `context` and `limit`. Only the primary session supplied the per-call `grep` factory, so advisor frames silently fell back to session defaults. Advisors now receive the same factory, gated on the advisor actually having been granted `grep`. +- Fixed Cursor advisors failing every `pi_edit`. The advisor roster handed the bridge the `edit` instance built for the advisor's own loop, which follows the configured `edit.mode` (`hashline` by default) and rejects the frame's `old_text`/`new_text` pairs — the same mode mismatch the primary bridge already fixed, on the path it missed. The exec map now substitutes a `replace`-mode instance, gated on the advisor actually having been granted `edit`, while the advisor's own loop keeps the tool it was given. - Fixed `pi_bash` killing commands that explicitly asked for no deadline. `timeout` is `optional int32` and `bash` documents `0` as "disables the command deadline", but a truthiness check folded a supplied `0` into unset, applying the 300s default instead. A present `0` now passes through; negatives, which have no local meaning and would otherwise clamp to the 1s floor, still fall back to the default. - Fixed the Cursor exec bridge granting `edit` and `grep` to sessions that withheld them. Both bridge-only tools are constructed rather than looked up, and `executeTool` prefers a constructed override over the registry, so a restricted tool set (`toolNames` without them, or `restrictToolNames`) still got a working `pi_edit`/`pi_grep` — native frames arrive regardless of the advertised catalog. Both are now gated on the session having actually granted the tool, matching the `delete` frame's existing check (issue #5680). diff --git a/packages/coding-agent/src/cursor-bridge-tools.ts b/packages/coding-agent/src/cursor-bridge-tools.ts index 8ae54f17a..7bf71e3d3 100644 --- a/packages/coding-agent/src/cursor-bridge-tools.ts +++ b/packages/coding-agent/src/cursor-bridge-tools.ts @@ -9,6 +9,7 @@ */ import type { AgentTool } from "@oh-my-pi/pi-agent-core"; +import { EditTool } from "./edit"; import type { ExtensionRunner } from "./extensibility/extensions"; import { ExtensionToolWrapper } from "./extensibility/extensions"; import type { GrepToolOptions, Tool, ToolSession } from "./tools"; @@ -34,3 +35,45 @@ export function createBridgeGrepFactory( return new ExtensionToolWrapper(grepTool, extensionRunner); }; } + +/** + * Build the `replace`-mode `edit` the bridge answers `pi_edit` with. + * + * `PiEditExecArgs` carries `old_text`/`new_text` pairs, which is exactly + * `replace`'s schema and nothing else's. The session's own instance follows the + * configured `edit.mode` — `hashline` by default, whose schema is a single + * `input` string — so a frame handed that instance fails validation instead of + * editing the file. + * + * Callers MUST gate this on the session having actually granted `edit`: the + * tool is constructed rather than looked up, so building one unconditionally + * hands a restricted agent a mutating tool it was denied (issue #5680). + */ +export function createBridgeEditTool(session: ToolSession, extensionRunner: ExtensionRunner): AgentTool { + const editTool: Tool = new EditTool(session, "replace"); + return new ExtensionToolWrapper(editTool, extensionRunner); +} + +/** + * The tool map the exec bridge should run, given the map a caller granted. + * + * `pi_edit` needs a `replace`-mode instance, but only when `edit` was granted: + * the tool is constructed rather than looked up, so substituting + * unconditionally would hand a restricted roster a mutating tool it was denied + * (issue #5680). The granted map is never mutated: an unsubstituted result is a + * copy, so a caller without an `edit` grant cannot accidentally gain one. + * + * The advisor roster passes its granted map here; the primary session keeps its + * instance out of the registry entirely (Cursor does not advertise `edit`) and + * serves it through the bridge's `getTool` fallback instead. + */ +export function bridgeToolMap( + granted: ReadonlyMap, + createEditTool: (() => AgentTool | undefined) | undefined, +): Map { + const bridged = new Map(granted); + if (!granted.has("edit") || !createEditTool) return bridged; + const bridgeEdit = createEditTool(); + if (bridgeEdit) bridged.set("edit", bridgeEdit); + return bridged; +} diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 10e7a9f6a..d73fa5d49 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -62,7 +62,7 @@ import { applyProviderGlobalsFromSettings } from "./config/provider-globals"; import { buildServiceTierByFamily } from "./config/service-tier"; import { Settings, type SkillsSettings } from "./config/settings"; import { CursorExecHandlers } from "./cursor"; -import { createBridgeGrepFactory } from "./cursor-bridge-tools"; +import { createBridgeEditTool, createBridgeGrepFactory } from "./cursor-bridge-tools"; import "./discovery"; import { initializeWithSettings } from "./discovery"; import { disposeAllJuliaKernelSessions, disposeJuliaKernelSessionsByOwner } from "./eval/jl/executor"; @@ -2607,8 +2607,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} toolRegistry.delete("edit"); builtInRegistryToolNames.delete("edit"); if (editWasGranted) { - const bridgeEdit: Tool = new EditTool(toolSession, "replace"); - cursorBridgeEditTool = new ExtensionToolWrapper(bridgeEdit, extensionRunner); + cursorBridgeEditTool = createBridgeEditTool(toolSession, extensionRunner); } } @@ -3273,6 +3272,9 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} // advisor's own tool session so a `pi_grep` frame's context width and // match cap are honored there too. advisorCreateGrepTool: createBridgeGrepFactory(advisorToolSession, extensionRunner), + // Same `replace`-mode requirement as the primary bridge; the advisor + // path gates it on the advisor's own `edit` grant. + advisorCreateEditTool: () => createBridgeEditTool(advisorToolSession, extensionRunner), titleSystemPrompt: options.titleSystemPrompt, }); hasSession = true; diff --git a/packages/coding-agent/src/session/agent-session-types.ts b/packages/coding-agent/src/session/agent-session-types.ts index 1cd89daaa..8f4d87a6c 100644 --- a/packages/coding-agent/src/session/agent-session-types.ts +++ b/packages/coding-agent/src/session/agent-session-types.ts @@ -215,6 +215,12 @@ export interface AgentSessionConfig { * running on Cursor silently drops both fields. */ advisorCreateGrepTool?(options: { context?: number; totalMatchLimit?: number }): AgentTool | undefined; + /** + * Build the `replace`-mode `edit` a Cursor `pi_edit` frame needs, against the + * advisor-scoped tool session. The advisor's ordinary instance follows the + * configured `edit.mode` and rejects the frame's `old_text`/`new_text` pairs. + */ + advisorCreateEditTool?(): AgentTool | undefined; /** Preloaded watchdog prompt content for the advisor. */ advisorWatchdogPrompt?: string; /** Shared advisor instructions loaded from WATCHDOG.yml. */ diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 5544aa967..0a80ca8b9 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -1291,6 +1291,7 @@ export class AgentSession { enabled: this.settings.get("advisor.enabled"), tools: config.advisorTools, createGrepTool: config.advisorCreateGrepTool, + createEditTool: config.advisorCreateEditTool, watchdogPrompt: config.advisorWatchdogPrompt, sharedInstructions: config.advisorSharedInstructions, contextPrompt: config.advisorContextPrompt, diff --git a/packages/coding-agent/src/session/session-advisors.ts b/packages/coding-agent/src/session/session-advisors.ts index 17f8e394a..41e479e93 100644 --- a/packages/coding-agent/src/session/session-advisors.ts +++ b/packages/coding-agent/src/session/session-advisors.ts @@ -68,6 +68,7 @@ import { MODEL_ROLES } from "../config/model-roles"; import { serviceTierForAllFamilies, serviceTierSettingToTier } from "../config/service-tier"; import type { Settings } from "../config/settings"; import { CursorExecHandlers } from "../cursor"; +import { bridgeToolMap } from "../cursor-bridge-tools"; import { estimateToolSchemaTokens } from "../modes/utils/context-usage"; import type { PlanModeState } from "../plan-mode/state"; import advisorSystemPrompt from "../prompts/advisor/system.md" with { type: "text" }; @@ -184,6 +185,13 @@ export interface SessionAdvisorsOptions { * drops both fields — the same gap the primary bridge closes. */ createGrepTool?(options: { context?: number; totalMatchLimit?: number }): AgentTool | undefined; + /** + * Build the `replace`-mode `edit` a Cursor `pi_edit` frame needs. The + * advisor's own instance follows the configured `edit.mode` (`hashline` by + * default), whose schema the frame's `old_text`/`new_text` pairs do not + * match, so without this every native advisor edit fails validation. + */ + createEditTool?(): AgentTool | undefined; watchdogPrompt?: string; sharedInstructions?: string; contextPrompt?: string; @@ -258,6 +266,7 @@ export class SessionAdvisors { #advisorEnabled: boolean; #advisorTools: AgentTool[] | undefined; #advisorCreateGrepTool: SessionAdvisorsOptions["createGrepTool"]; + #advisorCreateEditTool: SessionAdvisorsOptions["createEditTool"]; #advisorWatchdogPrompt: string | undefined; #advisorSharedInstructions: string | undefined; #advisorContextPrompt: string | undefined; @@ -281,6 +290,7 @@ export class SessionAdvisors { this.#advisorEnabled = options.enabled; this.#advisorTools = options.tools; this.#advisorCreateGrepTool = options.createGrepTool; + this.#advisorCreateEditTool = options.createEditTool; this.#advisorWatchdogPrompt = options.watchdogPrompt; this.#advisorSharedInstructions = options.sharedInstructions; this.#advisorContextPrompt = options.contextPrompt; @@ -727,10 +737,16 @@ export class SessionAdvisors { // to delete workspace files it was never granted (issue #5680 review). const advisorCanMutateFiles = advisorToolMap.has("write") || advisorToolMap.has("edit"); if (advisorCanMutateFiles) availableAdvisorToolNames.add("delete"); + // `pi_edit` speaks `replace`'s `old_text`/`new_text` schema, which the + // advisor's ordinary `EditTool` (built at the session's configured + // `edit.mode`, `hashline` by default) does not accept. The bridge map + // swaps in a `replace` instance for the exec channel only — the + // advisor's own loop keeps the tool it was given — and only when + // `edit` was actually granted. const advisorCursorExecHandlers = new CursorExecHandlers({ cwd: this.#host.sessionManager.getCwd(), getCwd: () => this.#host.sessionManager.getCwd(), - tools: advisorToolMap, + tools: bridgeToolMap(advisorToolMap, this.#advisorCreateEditTool), allowNativeDelete: advisorCanMutateFiles, // Gated on the advisor's own grant: the factory builds a fresh // tool, so handing it over unconditionally would give a roster diff --git a/packages/coding-agent/test/cursor-exec.test.ts b/packages/coding-agent/test/cursor-exec.test.ts index b0e29a7b8..f72e8c5d2 100644 --- a/packages/coding-agent/test/cursor-exec.test.ts +++ b/packages/coding-agent/test/cursor-exec.test.ts @@ -19,7 +19,11 @@ import { } 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 { createBridgeGrepFactory } from "@oh-my-pi/pi-coding-agent/cursor-bridge-tools"; +import { + bridgeToolMap, + createBridgeEditTool, + createBridgeGrepFactory, +} from "@oh-my-pi/pi-coding-agent/cursor-bridge-tools"; import { EditTool } from "@oh-my-pi/pi-coding-agent/edit"; import type { ExtensionRunner } from "@oh-my-pi/pi-coding-agent/extensibility/extensions"; import { ExtensionToolWrapper } from "@oh-my-pi/pi-coding-agent/extensibility/extensions"; @@ -41,6 +45,24 @@ function createTestSession(cwd: string, overrides: Partial = {}): T }; } +/** + * An `ExtensionRunner` that intercepts nothing but records that it ran. + * + * The bridge's per-call tools must carry the same wrapper as registry tools; + * seeing a name arrive here proves the wrapper is present, since an unwrapped + * tool never announces. + */ +function passthroughRunner(seen: string[] = []): ExtensionRunner { + return { + hasHandlers: () => true, + emitToolCall: async (event: { toolName: string }) => { + seen.push(event.toolName); + return undefined; + }, + emitToolResult: async () => undefined, + } as unknown as ExtensionRunner; +} + describe("CursorExecHandlers.grep bridge", () => { let cwd: string; let searchTool: GrepTool; @@ -223,10 +245,11 @@ describe("bridge tool resolution beyond the model-facing registry", () => { // edit answers "Tool \"edit\" not available" and the file is untouched. const target = path.join(cwd, "sample.txt"); await Bun.write(target, "alpha\nbeta\n"); - const session = createTestSession(cwd); - // `replace` is the mode `pi_edit`'s `old_text`/`new_text` pairs speak; - // the session default is `hashline`, whose schema they do not match. - const editTool: Tool = new EditTool(session, "replace"); + // Build it exactly as the session does. Both bridge callsites go through + // this factory, so a regression in it — the wrong mode, a missing + // approval wrapper — fails here rather than passing against a + // hand-constructed stand-in. + const editTool = createBridgeEditTool(createTestSession(cwd), passthroughRunner()); const withheld = new CursorExecHandlers({ cwd, @@ -255,6 +278,57 @@ describe("bridge tool resolution beyond the model-facing registry", () => { expect(await Bun.file(target).text()).toBe("alpha\nbeta\n"); }); + it("substitutes a replace-mode edit into a granted advisor tool map", async () => { + // The advisor roster hands the bridge the instances it built for the + // advisor's own loop — default `hashline` mode, whose schema is a single + // `input` string. A `pi_edit` frame's `old_text`/`new_text` pairs fail + // validation against it, so the file goes unmodified. This is the + // substitution the advisor path applies before constructing handlers. + const target = path.join(cwd, "sample.txt"); + await Bun.write(target, "alpha\nbeta\n"); + const session = createTestSession(cwd); + const advisorEdit = new EditTool(session); + expect(advisorEdit.mode).not.toBe("replace"); + const granted = new Map([["edit", advisorEdit]]); + + const bridged = bridgeToolMap(granted, () => createBridgeEditTool(session, passthroughRunner())); + const handlers = new CursorExecHandlers({ cwd, tools: bridged }); + const result = await handlers.piEdit({ + toolCallId: "e3", + args: { path: target, edits: [{ oldText: "beta", newText: "gamma" }] }, + } as never); + + expect(result.isError).toBeFalsy(); + expect(await Bun.file(target).text()).toBe("alpha\ngamma\n"); + // The advisor's own loop must keep the exact instance it was handed. + expect(granted.get("edit")).toBe(advisorEdit); + }); + + it("leaves an ungranted tool map without an edit tool", async () => { + // The bridge tool is constructed, not looked up, so substituting for a + // roster that was never granted `edit` would hand a read-only advisor a + // mutating tool (issue #5680). The frame must fail instead. + const target = path.join(cwd, "sample.txt"); + await Bun.write(target, "alpha\nbeta\n"); + const session = createTestSession(cwd); + let built = 0; + const withheld = bridgeToolMap(new Map(), () => { + built++; + return createBridgeEditTool(session, passthroughRunner()); + }); + expect(withheld.has("edit")).toBe(false); + expect(built).toBe(0); + + const handlers = new CursorExecHandlers({ cwd, tools: withheld }); + const result = await handlers.piEdit({ + toolCallId: "e4", + args: { path: target, edits: [{ oldText: "beta", newText: "gamma" }] }, + } as never); + + expect(result.isError).toBe(true); + expect(await Bun.file(target).text()).toBe("alpha\nbeta\n"); + }); + it("refuses a scoped pi_grep when no grep tool was granted", async () => { // The factory builds a fresh tool and `executeTool` prefers that override // over the registry, so a session that withheld `grep` must not install @@ -277,16 +351,7 @@ describe("bridge tool resolution beyond the model-facing registry", () => { // both callsites use, so a regression in it fails here. await Bun.write(path.join(cwd, "hit.txt"), "needle\n"); const intercepted: string[] = []; - const runner = { - hasHandlers: () => true, - emitToolCall: async (event: { toolName: string }) => { - intercepted.push(event.toolName); - return undefined; - }, - emitToolResult: async () => undefined, - } as unknown as ExtensionRunner; - - const factory = createBridgeGrepFactory(createTestSession(cwd), runner); + const factory = createBridgeGrepFactory(createTestSession(cwd), passthroughRunner(intercepted)); const built = factory({ context: 0, totalMatchLimit: 5 }); expect(built).toBeInstanceOf(ExtensionToolWrapper);