feat(cursor): honor pi_grep's context and limit
`pi_grep` carries a context width and a total match cap. Neither is expressible in the model-facing `grep` schema — context comes from `grep.contextBefore`/`grep.contextAfter`, fixed when the shared tool is constructed — so both were dropped. `GrepTool` now takes them as constructor options. The model-facing schema is unchanged: this is a seam for wire bridges whose protocol supplies the values, mirroring `GlobTool`'s existing options bag. The bridge builds a per-call `grep` only for frames that supply them; everything else keeps the shared instance and session defaults. `pi_ls`'s `limit` stays unmapped, now deliberately and documented. It caps directory entries, while the local `read` renders a depth-2 tree and slices rendered lines — nested rows, headers and elision summaries all count — so `:1+K` would cap a different unit while looking honored. Verified against real files in a temp dir, not captured arguments: match counts and context lines are asserted from actual search output. Mutation-checked — ignoring either option, or dropping the scoped tool in the bridge, fails a test. (cherry picked from commit 299ded5a274427c2c2d5de27c00a2056a709581c)
This commit is contained in:
committed by
can1357
parent
7b62fef366
commit
9436fb5640
@@ -195,6 +195,7 @@ import {
|
||||
piEscapeRegexLiteral,
|
||||
piJoinPath,
|
||||
piLimit,
|
||||
piLsPath,
|
||||
piReadPath,
|
||||
} from "./cursor/exec-modern";
|
||||
|
||||
@@ -1671,7 +1672,7 @@ async function handleExecServerMessage(
|
||||
// Same mapping as the legacy `lsArgs` frame: the local `read` tool lists
|
||||
// directories, so the synthesized block must name `read` to match the
|
||||
// bridge's own `toolResult`.
|
||||
synthesizeCursorExecToolCall(output, stream, state, toolCallId, "read", { path: args.path || "." });
|
||||
synthesizeCursorExecToolCall(output, stream, state, toolCallId, "read", { path: piLsPath(args.path) });
|
||||
const { execResult } = await resolveExecHandler(
|
||||
{ args, toolCallId },
|
||||
execHandlers?.piLs?.bind(execHandlers),
|
||||
|
||||
@@ -113,6 +113,21 @@ export function piJoinPath(basePath: string | undefined, pattern: string): strin
|
||||
return path.join(basePath, pattern);
|
||||
}
|
||||
|
||||
/**
|
||||
* The path a `pi_ls` frame lists.
|
||||
*
|
||||
* The frame's `limit` is deliberately NOT mapped. It caps directory *entries*
|
||||
* (the reference does a flat `readdir` and slices the entry array), while the
|
||||
* local `read` tool renders a depth-2 tree with per-directory caps and elision
|
||||
* summaries and applies a selector as a *rendered line* slice. Nested rows,
|
||||
* headers and "N more" lines all count toward that slice, so `:1+K` would cap
|
||||
* a different unit while looking honored — worse than leaving it unset, which
|
||||
* at least reports the local listing's own truncation faithfully.
|
||||
*/
|
||||
export function piLsPath(basePath: string | undefined): string {
|
||||
return basePath || ".";
|
||||
}
|
||||
|
||||
/** Escape a literal string so the regex-only local `grep` tool matches it verbatim. */
|
||||
export function piEscapeRegexLiteral(value: string): string {
|
||||
return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
|
||||
|
||||
@@ -168,6 +168,8 @@
|
||||
### Fixed
|
||||
|
||||
- Fixed the Cursor Pi exec bridge silently dropping frame arguments. `pi_read`'s `offset`/`limit` were ignored, so a ranged read returned the whole file; `pi_grep`'s `literal` was ignored, so a fixed-string search ran as a regex and matched the wrong lines; and the path/glob join produced a `./`-prefixed spec. Ranges are now composed onto `read`'s `:N+K` inline selector, literal patterns are escaped, and the join uses `node:path`. These are `optional int32` fields, so a present `0` is honored rather than folded into a default: `pi_read` with `limit: 0` answers with empty output instead of the entire file, and `pi_find` with `limit: 0` clamps to 1 the way the reference client does.
|
||||
- `pi_grep`'s `context` and `limit` are honored. Neither is expressible in the model-facing `grep` schema — context width comes from `grep.contextBefore`/`grep.contextAfter` fixed at tool construction — so the bridge builds a per-call `grep` for frames that supply them. `GrepTool` accepts these as constructor options; the model-facing schema is unchanged, and a frame that supplies neither keeps the shared instance and the session's defaults.
|
||||
- `pi_ls`'s `limit` is still not mapped, now deliberately: it caps directory *entries*, while the local `read` tool renders a depth-2 tree with per-directory caps and elision rows and applies a selector as a *rendered line* slice. Mapping it to `:1+K` would cap a different unit while appearing honored.
|
||||
|
||||
## [17.1.4] - 2026-07-26
|
||||
|
||||
|
||||
@@ -14,7 +14,13 @@ import type {
|
||||
CursorExecHandlers as ICursorExecHandlers,
|
||||
ToolResultMessage,
|
||||
} from "@oh-my-pi/pi-ai";
|
||||
import { piEscapeRegexLiteral, piJoinPath, piLimit, piReadPath } from "@oh-my-pi/pi-ai/providers/cursor/exec-modern";
|
||||
import {
|
||||
piEscapeRegexLiteral,
|
||||
piJoinPath,
|
||||
piLimit,
|
||||
piLsPath,
|
||||
piReadPath,
|
||||
} from "@oh-my-pi/pi-ai/providers/cursor/exec-modern";
|
||||
import { sanitizeText } from "@oh-my-pi/pi-utils";
|
||||
import { resolveToCwd } from "./tools/path-utils";
|
||||
import type { TodoPhase, TodoStatus } from "./tools/todo";
|
||||
@@ -22,6 +28,13 @@ import type { TodoPhase, TodoStatus } from "./tools/todo";
|
||||
/** Phase used for Cursor-owned tasks with no local phase grouping. */
|
||||
const CURSOR_TODO_PHASE = "Tasks";
|
||||
|
||||
/**
|
||||
* A tool instance the bridge can run, matching the erased shape the session's
|
||||
* tool registry stores. The concrete tools have narrower `execute` parameter
|
||||
* types than the default `AgentTool`, which only unify through this alias.
|
||||
*/
|
||||
type CursorBridgeTool = AgentTool<any, any, any>;
|
||||
|
||||
interface CursorExecBridgeOptions {
|
||||
cwd: string;
|
||||
getCwd?: () => string;
|
||||
@@ -51,6 +64,15 @@ interface CursorExecBridgeOptions {
|
||||
* Cursor emits no local `todo` toolResult, so nothing else records it.
|
||||
*/
|
||||
persistTodoPhases?: (phases: TodoPhase[]) => void;
|
||||
/**
|
||||
* Build a `grep` tool honoring a frame's own context width and match cap.
|
||||
*
|
||||
* The modern `pi_grep` frame carries both, and the shared `grep` instance
|
||||
* is fixed to the session settings at construction — so without this the
|
||||
* two fields are silently dropped. Callers that cannot supply it keep the
|
||||
* shared instance and the session's defaults.
|
||||
*/
|
||||
createGrepTool?(options: { context?: number; totalMatchLimit?: number }): CursorBridgeTool | undefined;
|
||||
}
|
||||
|
||||
function createToolResultMessage(
|
||||
@@ -82,8 +104,9 @@ async function executeTool(
|
||||
toolName: string,
|
||||
toolCallId: string,
|
||||
args: Record<string, unknown>,
|
||||
overrideTool?: CursorBridgeTool,
|
||||
): Promise<ToolResultMessage> {
|
||||
const tool = options.getExecutableTool?.(toolName) ?? options.tools.get(toolName);
|
||||
const tool = overrideTool ?? options.getExecutableTool?.(toolName) ?? options.tools.get(toolName);
|
||||
if (!tool) {
|
||||
const result = buildToolErrorResult(`Tool "${toolName}" not available`);
|
||||
return createToolResultMessage(toolCallId, toolName, result, true);
|
||||
@@ -460,20 +483,35 @@ export class CursorExecHandlers implements ICursorExecHandlers {
|
||||
/**
|
||||
* `literal` makes the pattern a fixed string; the local tool is regex-only,
|
||||
* so the pattern is escaped on the way in (same translation the legacy pi
|
||||
* shim does). `context` and `limit` have no tool-level equivalent — context
|
||||
* width comes from `grep.contextBefore`/`grep.contextAfter` settings and the
|
||||
* result count is bounded by the tool's own file/match caps.
|
||||
* shim does).
|
||||
*
|
||||
* `context` and `limit` are not expressible in the model-facing schema —
|
||||
* context width comes from settings fixed at tool construction — so the
|
||||
* frame's values are honored by building a per-call `grep` through
|
||||
* {@link CursorExecBridgeOptions.createGrepTool}. Both are `optional int32`,
|
||||
* so a present `0` context means "no context lines", not "use the default".
|
||||
* Without the factory the shared instance runs with session defaults.
|
||||
*/
|
||||
async piGrep(call: Parameters<NonNullable<ICursorExecHandlers["piGrep"]>>[0]) {
|
||||
const { pattern, path, glob, ignoreCase, literal } = call.args;
|
||||
const { pattern, path, glob, ignoreCase, literal, context, limit } = call.args;
|
||||
const scoped =
|
||||
context !== undefined || limit !== undefined
|
||||
? this.options.createGrepTool?.({ context, totalMatchLimit: piLimit(limit) })
|
||||
: undefined;
|
||||
// Same arg mapping as the legacy `grep` handler: the local tool takes one
|
||||
// path spec, and its `case` flag is case-SENSITIVITY, the inverse of the
|
||||
// frame's `ignore_case`.
|
||||
return await executeTool(this.options, "grep", call.toolCallId, {
|
||||
pattern: literal === true ? piEscapeRegexLiteral(pattern) : pattern,
|
||||
path: glob ? piJoinPath(path, glob) : path || ".",
|
||||
case: ignoreCase === true ? false : undefined,
|
||||
});
|
||||
return await executeTool(
|
||||
this.options,
|
||||
"grep",
|
||||
call.toolCallId,
|
||||
{
|
||||
pattern: literal === true ? piEscapeRegexLiteral(pattern) : pattern,
|
||||
path: glob ? piJoinPath(path, glob) : path || ".",
|
||||
case: ignoreCase === true ? false : undefined,
|
||||
},
|
||||
scoped,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -495,11 +533,10 @@ export class CursorExecHandlers implements ICursorExecHandlers {
|
||||
|
||||
/**
|
||||
* Redirected to `read`, which lists directories — same as the legacy `ls`.
|
||||
* `limit` has no tool-level equivalent; the listing is bounded by `read`'s
|
||||
* own directory-entry caps.
|
||||
* The frame's entry `limit` is not mapped; see {@link piLsPath}.
|
||||
*/
|
||||
async piLs(call: Parameters<NonNullable<ICursorExecHandlers["piLs"]>>[0]) {
|
||||
return await executeTool(this.options, "read", call.toolCallId, { path: call.args.path || "." });
|
||||
return await executeTool(this.options, "read", call.toolCallId, { path: piLsPath(call.args.path) });
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -2635,6 +2635,9 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
|
||||
getTodoPhases: () => session.getTodoPhases(),
|
||||
setTodoPhases: phases => session.setTodoPhases(phases),
|
||||
persistTodoPhases: phases => sessionManager.appendCustomEntry(USER_TODO_EDIT_CUSTOM_TYPE, { phases }),
|
||||
// `pi_grep` carries its own context width and match cap, which the
|
||||
// shared grep instance fixed at construction cannot express.
|
||||
createGrepTool: grepOptions => new GrepTool(toolSession, grepOptions),
|
||||
});
|
||||
|
||||
// Resolve the inline-descriptors setting against the session-start model.
|
||||
|
||||
@@ -884,6 +884,22 @@ export interface GrepToolDetails {
|
||||
|
||||
type SearchParams = typeof searchSchema.infer;
|
||||
|
||||
/**
|
||||
* Construction-time overrides for callers that are not the model.
|
||||
*
|
||||
* The model-facing schema deliberately does not grow these: they exist for
|
||||
* wire bridges (the Cursor `pi_grep` frame) whose protocol carries an explicit
|
||||
* context width and total match cap, and which would otherwise have to drop
|
||||
* them. Unset means "use the session settings / built-in caps" — the behavior
|
||||
* every model-issued call keeps.
|
||||
*/
|
||||
export interface GrepToolOptions {
|
||||
/** Overrides `grep.contextBefore`/`grep.contextAfter` for every call on this instance. */
|
||||
context?: number;
|
||||
/** Caps total surfaced matches. Applied on top of the built-in per-file and file-window caps, never above them. */
|
||||
totalMatchLimit?: number;
|
||||
}
|
||||
|
||||
export class GrepTool implements AgentTool<typeof searchSchema, GrepToolDetails> {
|
||||
readonly name = "grep";
|
||||
readonly approval = (args: unknown): ToolTier => {
|
||||
@@ -897,7 +913,17 @@ export class GrepTool implements AgentTool<typeof searchSchema, GrepToolDetails>
|
||||
readonly parameters = searchSchema;
|
||||
readonly strict = true;
|
||||
|
||||
constructor(private readonly session: ToolSession) {
|
||||
readonly #contextOverride?: number;
|
||||
readonly #totalMatchLimit?: number;
|
||||
|
||||
constructor(
|
||||
private readonly session: ToolSession,
|
||||
options?: GrepToolOptions,
|
||||
) {
|
||||
const context = options?.context;
|
||||
this.#contextOverride = context !== undefined ? Math.max(0, Math.floor(context)) : undefined;
|
||||
const total = options?.totalMatchLimit;
|
||||
this.#totalMatchLimit = total !== undefined ? Math.max(1, Math.floor(total)) : undefined;
|
||||
const displayMode = resolveFileDisplayMode(session);
|
||||
this.description = prompt.render(grepDescription, {
|
||||
IS_HL_MODE: displayMode.hashLines,
|
||||
@@ -978,8 +1004,8 @@ export class GrepTool implements AgentTool<typeof searchSchema, GrepToolDetails>
|
||||
`or pass a UTF-8 text member.`,
|
||||
);
|
||||
}
|
||||
const normalizedContextBefore = this.session.settings.get("grep.contextBefore");
|
||||
const normalizedContextAfter = this.session.settings.get("grep.contextAfter");
|
||||
const normalizedContextBefore = this.#contextOverride ?? this.session.settings.get("grep.contextBefore");
|
||||
const normalizedContextAfter = this.#contextOverride ?? this.session.settings.get("grep.contextAfter");
|
||||
const ignoreCase = !(caseSensitive ?? true);
|
||||
const useGitignore = gitignore ?? true;
|
||||
const patternHasNewline = normalizedPattern.includes("\n") || normalizedPattern.includes("\\n");
|
||||
@@ -1300,6 +1326,7 @@ export class GrepTool implements AgentTool<typeof searchSchema, GrepToolDetails>
|
||||
const windowFiles = canPaginate ? fileOrder.slice(skipFiles, skipFiles + DEFAULT_FILE_LIMIT) : fileOrder;
|
||||
const fileLimitReached = canPaginate && totalFiles > skipFiles + DEFAULT_FILE_LIMIT;
|
||||
const selectedMatches: GrepMatch[] = [];
|
||||
let totalMatchLimitReached = false;
|
||||
if (windowFiles.length > 0) {
|
||||
const lists = windowFiles.map(file => matchesByPath.get(file) ?? []);
|
||||
const cursors = new Array<number>(lists.length).fill(0);
|
||||
@@ -1313,6 +1340,14 @@ export class GrepTool implements AgentTool<typeof searchSchema, GrepToolDetails>
|
||||
}
|
||||
}
|
||||
}
|
||||
// Round-robin above interleaves files for diversity, so the cap is
|
||||
// applied after selection rather than as a per-list bound: trimming
|
||||
// mid-rotation would silently favour whichever files sort first.
|
||||
const cap = this.#totalMatchLimit;
|
||||
if (cap !== undefined && selectedMatches.length > cap) {
|
||||
selectedMatches.length = cap;
|
||||
totalMatchLimitReached = true;
|
||||
}
|
||||
}
|
||||
const nextSkip = skipFiles + windowFiles.length;
|
||||
const limitMessage = fileLimitReached
|
||||
@@ -1503,7 +1538,12 @@ export class GrepTool implements AgentTool<typeof searchSchema, GrepToolDetails>
|
||||
const output = truncation.content;
|
||||
const displayText = displayLines.join("\n");
|
||||
const truncated = Boolean(
|
||||
fileLimitReached || perFileLimitReached || result.limitReached || truncation.truncated || linesTruncated,
|
||||
fileLimitReached ||
|
||||
perFileLimitReached ||
|
||||
totalMatchLimitReached ||
|
||||
result.limitReached ||
|
||||
truncation.truncated ||
|
||||
linesTruncated,
|
||||
);
|
||||
const details: GrepToolDetails = {
|
||||
scopePath,
|
||||
@@ -1518,7 +1558,11 @@ export class GrepTool implements AgentTool<typeof searchSchema, GrepToolDetails>
|
||||
})),
|
||||
truncated,
|
||||
fileLimitReached: fileLimitReached ? DEFAULT_FILE_LIMIT : undefined,
|
||||
perFileLimitReached: perFileLimitReached ? perFileMatchCap : undefined,
|
||||
perFileLimitReached: totalMatchLimitReached
|
||||
? this.#totalMatchLimit
|
||||
: perFileLimitReached
|
||||
? perFileMatchCap
|
||||
: undefined,
|
||||
displayContent: displayText,
|
||||
missingPaths: missingPaths.length > 0 ? missingPaths : undefined,
|
||||
};
|
||||
|
||||
@@ -20,7 +20,7 @@ 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";
|
||||
import { ExtensionToolWrapper } from "@oh-my-pi/pi-coding-agent/extensibility/extensions";
|
||||
import { GrepTool, type ToolSession } from "@oh-my-pi/pi-coding-agent/tools";
|
||||
import { GrepTool, type Tool, 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";
|
||||
@@ -82,6 +82,58 @@ describe("CursorExecHandlers.grep bridge", () => {
|
||||
} as any);
|
||||
expect((sensitiveResult.details as { matchCount?: number } | undefined)?.matchCount).toBe(1);
|
||||
});
|
||||
|
||||
it("honors pi_grep's requested match limit against real files", async () => {
|
||||
// The frame's `limit` caps total surfaced matches. The model-facing schema
|
||||
// has no such parameter, so without a per-call tool the cap is dropped and
|
||||
// the search returns everything it found.
|
||||
await Bun.write(path.join(cwd, "many.txt"), Array.from({ length: 10 }, (_, i) => `needle ${i}`).join("\n"));
|
||||
const scopedHandlers = new CursorExecHandlers({
|
||||
cwd,
|
||||
tools: new Map<string, Tool>([["grep", searchTool]]),
|
||||
createGrepTool: options => new GrepTool(createTestSession(cwd), options),
|
||||
});
|
||||
|
||||
const capped = await scopedHandlers.piGrep({
|
||||
toolCallId: "c1",
|
||||
args: { pattern: "needle", path: cwd, limit: 3 },
|
||||
} as never);
|
||||
expect((capped.details as { matchCount?: number } | undefined)?.matchCount).toBe(3);
|
||||
|
||||
const uncapped = await scopedHandlers.piGrep({
|
||||
toolCallId: "c2",
|
||||
args: { pattern: "needle", path: cwd },
|
||||
} as never);
|
||||
expect((uncapped.details as { matchCount?: number } | undefined)?.matchCount).toBe(10);
|
||||
});
|
||||
|
||||
it("honors pi_grep's requested context width against real files", async () => {
|
||||
// `context` has no schema parameter either: the width is read from
|
||||
// settings fixed at tool construction, so the frame's value only lands
|
||||
// through a per-call instance.
|
||||
await Bun.write(path.join(cwd, "ctx.txt"), "before line\nneedle here\nafter line\n");
|
||||
const scopedHandlers = new CursorExecHandlers({
|
||||
cwd,
|
||||
tools: new Map<string, Tool>([["grep", searchTool]]),
|
||||
createGrepTool: options => new GrepTool(createTestSession(cwd), options),
|
||||
});
|
||||
|
||||
const noContext = await scopedHandlers.piGrep({
|
||||
toolCallId: "c1",
|
||||
args: { pattern: "needle here", path: path.join(cwd, "ctx.txt"), context: 0 },
|
||||
} as never);
|
||||
const noContextText = noContext.content.map(c => (c.type === "text" ? c.text : "")).join("");
|
||||
expect(noContextText).not.toContain("before line");
|
||||
expect(noContextText).not.toContain("after line");
|
||||
|
||||
const withContext = await scopedHandlers.piGrep({
|
||||
toolCallId: "c2",
|
||||
args: { pattern: "needle here", path: path.join(cwd, "ctx.txt"), context: 1 },
|
||||
} as never);
|
||||
const withContextText = withContext.content.map(c => (c.type === "text" ? c.text : "")).join("");
|
||||
expect(withContextText).toContain("before line");
|
||||
expect(withContextText).toContain("after line");
|
||||
});
|
||||
});
|
||||
|
||||
describe("CursorExecHandlers error results", () => {
|
||||
|
||||
Reference in New Issue
Block a user