diff --git a/src/agent/tool-classification.test.ts b/src/agent/tool-classification.test.ts new file mode 100644 index 00000000..82e9106f --- /dev/null +++ b/src/agent/tool-classification.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, test } from "bun:test"; +import { + AUTO_ALLOW_READ_TOOLS, + PATH_KEYED_READ_TOOLS, + SEARCH_QUERY_TOOLS, +} from "./tool-classification.js"; + +// Pins membership so a future edit to any of these sets — or to the director +// READ_TOOLS they derive from — fails CI instead of silently drifting one +// call site out of sync with the others (CL-6809). +describe("AUTO_ALLOW_READ_TOOLS", () => { + test("gates auto-allow with exactly this membership", () => { + expect([...AUTO_ALLOW_READ_TOOLS].sort()).toEqual( + ["grep", "list_dir", "lsp", "manage_tasks", "read_file", "search_files"].sort(), + ); + }); + + test("excludes tools with their own, narrower auto-allow logic", () => { + for (const tool of ["run_shell", "web_fetch", "web_search"]) { + expect(AUTO_ALLOW_READ_TOOLS.has(tool)).toBe(false); + } + }); +}); + +describe("PATH_KEYED_READ_TOOLS", () => { + test("is read_file only", () => { + expect([...PATH_KEYED_READ_TOOLS]).toEqual(["read_file"]); + }); +}); + +describe("SEARCH_QUERY_TOOLS", () => { + test("is grep and search_files only", () => { + expect([...SEARCH_QUERY_TOOLS].sort()).toEqual(["grep", "search_files"]); + }); +}); diff --git a/src/agent/tool-classification.ts b/src/agent/tool-classification.ts new file mode 100644 index 00000000..644fe8f0 --- /dev/null +++ b/src/agent/tool-classification.ts @@ -0,0 +1,57 @@ +/** + * Shared tool-name classification constants (CL-6809). + * + * Three separate READ_TOOLS constants (director tool-sets, session compactor, + * subagent thrash tracker) had drifted to different memberships under the + * same name, and the permission classifier's auto-allow gate carried a fourth + * (READ_ONLY_TOOLS) with no declared relationship to the others. Same name, + * different meanings, one of them security-relevant. + * + * These are genuinely different concepts, not the same list typed four times: + * - the director read surface (tool-sets.ts READ_TOOLS) is "everything a + * read-only leaf may call", including run_shell and the web tools; + * - the auto-allow gate is "never needs an approval prompt", a strict + * subset (shell/web get their own, narrower auto-allow logic) plus + * manage_tasks (side-effect-free, see classify.ts); + * - compaction's re-read dedup and thrash's read tracking both care about + * "read_file specifically, because its result is keyed by path" — this + * one actually was the same set twice, so it is unified here. + * Where the concepts differ, the sets stay separate but are derived from the + * same base and named for what they mean, so a future difference reads as + * intentional instead of drift. + */ + +import { READ_TOOLS as DIRECTOR_READ_TOOLS } from "./directors/tool-sets.js"; + +/** + * read_file: the one read tool whose result is keyed by path, so an older + * result for the same path is safely superseded by a newer one. Shared by + * compaction's re-read dedup and thrash's read-count bookkeeping — both are + * asking the same question ("was this path already read?"). + */ +export const PATH_KEYED_READ_TOOLS: ReadonlySet = new Set(["read_file"]); + +/** + * grep / search_files: pattern-keyed query tools whose repeated identical + * call reflects current workspace state, not stale history. This is the base + * both compaction and thrash build on; each adds/omits list_dir for its own + * reason (see compactor.ts's QUERY_TOOLS and thrash.ts's SEARCH_TOOLS). + */ +export const SEARCH_QUERY_TOOLS: ReadonlySet = new Set(["grep", "search_files"]); + +/** + * Tools that never need an approval prompt because they cannot change the + * workspace: the director's read surface minus run_shell/web_fetch/web_search + * (which get their own, narrower auto-allow rules — see + * isAutoAllowedShellCommand and the webfetch/websearch permission classes), + * plus manage_tasks (side-effect-free by the time the tool executes — see + * classify.ts). SECURITY-RELEVANT: this gates auto-allow. A tool added here + * is auto-approved everywhere; get it wrong in either direction deliberately, + * not by accident. + */ +export const AUTO_ALLOW_READ_TOOLS: ReadonlySet = new Set([ + ...DIRECTOR_READ_TOOLS.filter( + (tool) => tool !== "run_shell" && tool !== "web_fetch" && tool !== "web_search", + ), + "manage_tasks", +]); diff --git a/src/permission/classify.ts b/src/permission/classify.ts index 9b0e1646..c2976bc7 100644 --- a/src/permission/classify.ts +++ b/src/permission/classify.ts @@ -17,6 +17,7 @@ import { import { resolveWorkspacePath } from "./path-restriction.js"; import type { RootsProvider } from "./worktree-roots.js"; import { isProductMutationTool, productMutationPaths } from "../agent/product-mutation-tools.js"; +import { AUTO_ALLOW_READ_TOOLS as READ_ONLY_TOOLS } from "../agent/tool-classification.js"; // Read-only tools never need approval as long as they don't touch a restricted // path; they cannot change the workspace. `lsp` is included here even though @@ -30,14 +31,9 @@ import { isProductMutationTool, productMutationPaths } from "../agent/product-mu // for a denial to prevent. Every other posix tool is consequential and // defaults to the "ask" tier. Catastrophic commands are denied earlier by the // authorization plugin, so they never reach here. -const READ_ONLY_TOOLS = new Set([ - "read_file", - "search_files", - "grep", - "list_dir", - "lsp", - "manage_tasks", -]); +// +// Membership lives in tool-classification.ts (AUTO_ALLOW_READ_TOOLS, imported +// here as READ_ONLY_TOOLS) — see CL-6809. // Tools that take a single path-like argument the gate should check against // restriction (outside the workspace boundary, or writes under the session state root). diff --git a/src/session/compactor.ts b/src/session/compactor.ts index 4100ac8e..32036dc0 100644 --- a/src/session/compactor.ts +++ b/src/session/compactor.ts @@ -21,6 +21,7 @@ import type { } from "@intx/types/runtime"; import { ageImageBlocks } from "./attachment-store.js"; import type { SummaryContext } from "./summarizer.js"; +import { PATH_KEYED_READ_TOOLS, SEARCH_QUERY_TOOLS } from "../agent/tool-classification.js"; // --------------------------------------------------------------------------- // Task boundary decision @@ -237,19 +238,18 @@ export function compactorNoOpFloor(keepRecentTurns: number): number { // Minimum anchor score for a turn to be pulled forward past the summary boundary. const ANCHOR_SCORE_THRESHOLD = 5; -// Tool names whose results are path-keyed for re-read dedup during compaction. -const READ_TOOLS = new Set(["read_file"]); - // Replayable query tools deduped by full-argument identity: a later identical // grep/search_files/list_dir call reflects newer workspace state, so an older // identical result is stale the same way an older read_file body is. // run_shell is deliberately excluded — the same command is not idempotent // (builds, tests, mutations), so an older run_shell result can be the only -// record of a genuinely distinct outcome. -const QUERY_TOOLS = new Set(["grep", "search_files", "list_dir"]); +// record of a genuinely distinct outcome. Built on the shared SEARCH_QUERY_TOOLS +// base plus list_dir, which compaction treats as replayable the same way +// (unlike thrash's narrower SEARCH_TOOLS — see tool-classification.ts). +const QUERY_TOOLS = new Set([...SEARCH_QUERY_TOOLS, "list_dir"]); function isReplayableResultTool(name: string): boolean { - return READ_TOOLS.has(name) || QUERY_TOOLS.has(name); + return PATH_KEYED_READ_TOOLS.has(name) || QUERY_TOOLS.has(name); } // Call-id index for stub rendering (name + path). Dedup keys live on `readKey`. diff --git a/src/subagent/thrash.ts b/src/subagent/thrash.ts index d5f27dc0..a5eec925 100644 --- a/src/subagent/thrash.ts +++ b/src/subagent/thrash.ts @@ -15,6 +15,7 @@ */ import { isProductMutationTool, productMutationPaths } from "../agent/product-mutation-tools.js"; +import { PATH_KEYED_READ_TOOLS, SEARCH_QUERY_TOOLS } from "../agent/tool-classification.js"; import { classifyShellFileEvidence } from "./shell-evidence.js"; /** Tunable thresholds for force-report detection. */ @@ -53,8 +54,11 @@ export interface ThrashToolCallBlock { arguments?: unknown; } -const READ_TOOLS = new Set(["read_file"]); -const SEARCH_TOOLS = new Set(["grep", "search_files"]); +// list_dir is deliberately excluded: a repeated identical listing of the same +// directory is not the stuck read/search loop this bookkeeping watches for +// the way a repeated read_file or grep is (see tool-classification.ts). +const READ_TOOLS = PATH_KEYED_READ_TOOLS; +const SEARCH_TOOLS = SEARCH_QUERY_TOOLS; const SHELL_TOOL = "run_shell"; function parseArgs(raw: unknown): Record {