From 04965a7cf031fcd8acb3e31f4f87b67dca06dd86 Mon Sep 17 00:00:00 2001 From: Bethel Yohannes Date: Sat, 21 Feb 2026 14:55:40 +0300 Subject: [PATCH] Hook Middleware & Security Boundary and test done --- scripts/pre-hook.spec.ts | 62 ++++++++++++++++ scripts/pre-hook.test.ts | 72 +++++++++++++++++++ .../presentAssistantMessage.ts | 4 ++ src/core/prompts/responses.ts | 21 ++++++ src/core/task/Task.ts | 11 +++ src/core/tools/BaseTool.ts | 9 +++ src/core/tools/WriteToFileTool.ts | 7 ++ src/hooks/index.ts | 1 + src/hooks/intent-ignore.ts | 54 ++++++++++++++ src/hooks/pre-hook.ts | 64 ++++++++++++++--- src/hooks/scope.ts | 21 ++++++ src/hooks/types.ts | 8 +++ 12 files changed, 326 insertions(+), 8 deletions(-) create mode 100644 scripts/pre-hook.spec.ts create mode 100644 scripts/pre-hook.test.ts create mode 100644 src/hooks/intent-ignore.ts diff --git a/scripts/pre-hook.spec.ts b/scripts/pre-hook.spec.ts new file mode 100644 index 0000000000..2254917394 --- /dev/null +++ b/scripts/pre-hook.spec.ts @@ -0,0 +1,62 @@ +import { describe, it, expect, beforeEach } from "vitest" +import { PreHook } from "../pre-hook" + +describe("PreHook", () => { + let activeIntentId: string | null + let preHook: PreHook + + beforeEach(() => { + activeIntentId = null + preHook = new PreHook({ + cwd: process.cwd(), + getActiveIntentId: () => activeIntentId, + setActiveIntentId: (id) => { + activeIntentId = id + }, + requireIntentForDestructiveOnly: true, + }) + }) + + it("blocks destructive write without active intent", async () => { + activeIntentId = null + const res = await preHook.intercept("write_to_file", { + path: "src/api/weather.ts", + content: "// test", + }) + expect(res.blocked).toBe(true) + expect(res.error).toEqual(expect.stringMatching(/select an active intent/i)) + }) + + it("blocks write outside owned scope (scope violation)", async () => { + activeIntentId = "INT-001" + const res = await preHook.intercept("write_to_file", { + path: "src/db/db.ts", + content: "// should be blocked", + }) + expect(res.blocked).toBe(true) + expect(res.error).toEqual(expect.stringMatching(/scope violation/i)) + }) + + it("recovery loop: retry after select_active_intent succeeds", async () => { + // initial attempt without intent + activeIntentId = null + const attempt1 = await preHook.intercept("write_to_file", { + path: "src/api/weather.ts", + content: "// first", + }) + expect(attempt1.blocked).toBe(true) + + // select active intent (handshake) + const handshake = await preHook.intercept("select_active_intent", { intent_id: "INT-001" }) + expect(handshake.blocked).toBe(false) + expect(handshake.injectResult).toEqual(expect.stringContaining("")) + expect(activeIntentId).toBe("INT-001") + + // retry write (in-owned-scope path) + const attempt2 = await preHook.intercept("write_to_file", { + path: "src/api/weather.ts", + content: "// second", + }) + expect(attempt2.blocked).toBe(false) + }) +}) diff --git a/scripts/pre-hook.test.ts b/scripts/pre-hook.test.ts new file mode 100644 index 0000000000..13181d963b --- /dev/null +++ b/scripts/pre-hook.test.ts @@ -0,0 +1,72 @@ +import test from "node:test" +import assert from "node:assert/strict" +import { PreHook } from "../src/hooks/pre-hook" + +const cwd = process.cwd() + +test("blocks destructive write without active intent", async () => { + let activeIntentId: string | null = null + const preHook = new PreHook({ + cwd, + getActiveIntentId: () => activeIntentId, + setActiveIntentId: (id) => { + activeIntentId = id + }, + requireIntentForDestructiveOnly: true, + }) + + const res = await preHook.intercept("write_to_file", { + path: "src/api/weather.ts", + content: "// test", + }) + assert.equal(res.blocked, true) + assert.match(String(res.error), /select an active intent/i) +}) + +test("blocks write outside owned scope (scope violation)", async () => { + let activeIntentId: string | null = "INT-001" + const preHook = new PreHook({ + cwd, + getActiveIntentId: () => activeIntentId, + setActiveIntentId: (id) => { + activeIntentId = id + }, + requireIntentForDestructiveOnly: true, + }) + + const res = await preHook.intercept("write_to_file", { + path: "src/db/db.ts", + content: "// should be blocked", + }) + assert.equal(res.blocked, true) + assert.match(String(res.error), /scope violation/i) +}) + +test("recovery loop: retry after select_active_intent succeeds", async () => { + let activeIntentId: string | null = null + const preHook = new PreHook({ + cwd, + getActiveIntentId: () => activeIntentId, + setActiveIntentId: (id) => { + activeIntentId = id + }, + requireIntentForDestructiveOnly: true, + }) + + const attempt1 = await preHook.intercept("write_to_file", { + path: "src/api/weather.ts", + content: "// first", + }) + assert.equal(attempt1.blocked, true) + + const handshake = await preHook.intercept("select_active_intent", { intent_id: "INT-001" }) + assert.equal(handshake.blocked, false) + assert.ok(typeof handshake.injectResult === "string" && handshake.injectResult.includes("")) + assert.equal(activeIntentId, "INT-001") + + const attempt2 = await preHook.intercept("write_to_file", { + path: "src/api/weather.ts", + content: "// second", + }) + assert.equal(attempt2.blocked, false) +}) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 7f5862be15..ced072d1f0 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -40,6 +40,7 @@ import { codebaseSearchTool } from "../tools/CodebaseSearchTool" import { formatResponse } from "../prompts/responses" import { sanitizeToolUseId } from "../../utils/tool-id" +import { HookMiddleware, PreHook } from "../../hooks" /** * Processes and presents assistant message content to the user interface. @@ -676,6 +677,9 @@ export async function presentAssistantMessage(cline: Task) { } switch (block.name) { + case "select_active_intent": + // Handled entirely by pre-hook (injectResult pushed above) + break case "write_to_file": await checkpointSaveAndMark(cline) await writeToFileTool.handle(cline, block as ToolUse<"write_to_file">, { diff --git a/src/core/prompts/responses.ts b/src/core/prompts/responses.ts index 60b5b4123a..2a66d3f3ad 100644 --- a/src/core/prompts/responses.ts +++ b/src/core/prompts/responses.ts @@ -30,6 +30,27 @@ export const formatResponse = { error, }), + /** Standardized JSON for autonomous recovery when user rejects a destructive tool. */ + toolErrorUserRejected: (toolName?: string) => + JSON.stringify({ + status: "error", + type: "user_rejected", + message: "The user rejected this operation.", + tool: toolName, + suggestion: "Do not retry the same operation; try a different approach or ask the user for permission.", + }), + + /** Standardized JSON for scope violation so the LLM can request scope expansion. */ + toolErrorScopeViolation: (intentId: string, filename: string) => + JSON.stringify({ + status: "error", + type: "scope_violation", + message: `Scope Violation: ${intentId} is not authorized to edit [${filename}]. Request scope expansion.`, + intent_id: intentId, + path: filename, + suggestion: "Request scope expansion in .orchestration/active_intents.yaml or choose another intent.", + }), + rooIgnoreError: (path: string) => JSON.stringify({ status: "error", diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 6ba57e98ac..22b42cbb1f 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -266,6 +266,8 @@ export class Task extends EventEmitter implements TaskLike { providerRef: WeakRef private readonly globalStoragePath: string + /** Active intent ID set by select_active_intent (per task/session). Used by Hook Engine for scope enforcement. */ + private _activeIntentId: string | null = null abort: boolean = false currentRequestAbortController?: AbortController skipPrevResponseIdOnce: boolean = false @@ -4670,6 +4672,15 @@ export class Task extends EventEmitter implements TaskLike { return this.workspacePath } + /** Active intent ID for Hook Engine (select_active_intent / scope enforcement). */ + public getActiveIntentId(): string | null { + return this._activeIntentId + } + + public setActiveIntentId(id: string | null): void { + this._activeIntentId = id + } + /** * Provides convenient access to high-level message operations. * Uses lazy initialization - the MessageManager is only created when first accessed. diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 7d574068a9..19f137556d 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -3,6 +3,13 @@ import type { ToolName } from "@roo-code/types" import { Task } from "../task/Task" import type { ToolUse, HandleError, PushToolResult, AskApproval, NativeToolArgs } from "../../shared/tools" +/** Params passed to onWriteToFileSuccess after a successful write_to_file (for Hook Engine post-hook). */ +export interface WriteToFileSuccessParams { + path: string + content: string + mutation_class?: string +} + /** * Callbacks passed to tool execution */ @@ -11,6 +18,8 @@ export interface ToolCallbacks { handleError: HandleError pushToolResult: PushToolResult toolCallId?: string + /** Called after successful write_to_file for Hook Engine agent_trace / post-hook. */ + onWriteToFileSuccess?: (params: WriteToFileSuccessParams) => void | Promise } /** diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index c8455ef3d9..3fe0d4166c 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -179,6 +179,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) + const mutationClass = (params as Record).mutation_class as string | undefined + await callbacks.onWriteToFileSuccess?.({ + path: relPath, + content: newContent, + mutation_class: mutationClass, + }) + await task.diffViewProvider.reset() this.resetPartialState() diff --git a/src/hooks/index.ts b/src/hooks/index.ts index d4a9dc04bb..ea52530019 100644 --- a/src/hooks/index.ts +++ b/src/hooks/index.ts @@ -7,6 +7,7 @@ export * from "./types" export * from "./content-hash" export * from "./context-loader" export * from "./scope" +export * from "./intent-ignore" export * from "./pre-hook" export * from "./post-hook" export * from "./middleware" diff --git a/src/hooks/intent-ignore.ts b/src/hooks/intent-ignore.ts new file mode 100644 index 0000000000..82d39e285f --- /dev/null +++ b/src/hooks/intent-ignore.ts @@ -0,0 +1,54 @@ +import fs from "fs/promises" +import path from "path" + +import { pathMatchesAnyPattern } from "./scope" + +const DEFAULT_INTENT_IGNORE_NAME = ".intentignore" +const ORCHESTRATION_DIR = ".orchestration" +const INTENT_PREFIX = "intent:" + +export interface IntentIgnoreResult { + pathPatterns: string[] + excludedIntentIds: string[] +} + +/** + * Load .intentignore-style file: path patterns (one per line) and optional + * "intent:ID" lines to exclude specific intents from receiving changes. + * Convention: lines starting with "intent:" are intent IDs to exclude; all other + * non-empty, non-comment lines are path glob patterns that no write may touch. + * + * @param cwd - Workspace root + * @param intentIgnorePath - Optional path relative to cwd (e.g. ".orchestration/.intentignore") + */ +export async function loadIntentIgnore(cwd: string, intentIgnorePath?: string): Promise { + const filePath = intentIgnorePath + ? path.resolve(cwd, intentIgnorePath) + : path.join(cwd, ORCHESTRATION_DIR, DEFAULT_INTENT_IGNORE_NAME) + try { + const raw = await fs.readFile(filePath, "utf-8") + const pathPatterns: string[] = [] + const excludedIntentIds: string[] = [] + for (const line of raw.split("\n")) { + const trimmed = line.trim() + if (!trimmed || trimmed.startsWith("#")) continue + if (trimmed.startsWith(INTENT_PREFIX)) { + excludedIntentIds.push(trimmed.slice(INTENT_PREFIX.length).trim()) + } else { + pathPatterns.push(trimmed) + } + } + return { pathPatterns, excludedIntentIds } + } catch { + return { pathPatterns: [], excludedIntentIds: [] } + } +} + +export function isPathIgnored(relativePath: string, pathPatterns: string[]): boolean { + return pathMatchesAnyPattern(relativePath, pathPatterns) +} + +export function isIntentExcluded(intentId: string | null, excludedIntentIds: string[]): boolean { + if (!intentId) return false + return excludedIntentIds.some((id) => id === intentId) +} diff --git a/src/hooks/pre-hook.ts b/src/hooks/pre-hook.ts index e8dc7315c6..8b85180786 100644 --- a/src/hooks/pre-hook.ts +++ b/src/hooks/pre-hook.ts @@ -27,11 +27,19 @@ export interface PreHookOptions { /** * Pre-Hook: intercepts tool execution to enforce intent context and scope. * - select_active_intent: load context, return XML, set active intent. - * - Destructive tools: require active intent; optional HITL; scope check for write_to_file. + * - Destructive tools: require active intent; .intentignore exclusion; scope check for write_to_file; optional UI-blocking approval. */ export class PreHook { + private intentIgnoreCache: IntentIgnoreResult | null = null + constructor(private options: PreHookOptions) {} + private async getIntentIgnore(): Promise { + if (this.intentIgnoreCache) return this.intentIgnoreCache + this.intentIgnoreCache = await loadIntentIgnore(this.options.cwd, this.options.intentIgnorePath) + return this.intentIgnoreCache + } + async intercept(toolName: string, params: Record): Promise { const { cwd, getActiveIntentId, setActiveIntentId } = this.options @@ -54,9 +62,9 @@ export class PreHook { const isDestructive = (DESTRUCTIVE_TOOLS as readonly string[]).includes(toolName) const requireIntent = this.options.requireIntentForDestructiveOnly ? isDestructive : true + const activeId = getActiveIntentId() if (requireIntent) { - const activeId = getActiveIntentId() if (!activeId) { return { blocked: true, @@ -64,27 +72,67 @@ export class PreHook { } } - // Scope enforcement for write_to_file - if (toolName === "write_to_file" && params.path) { - const relPath = String(params.path) - // Path traversal: block paths that escape workspace (e.g. .. or absolute) + const ignore = await this.getIntentIgnore() + if (isIntentExcluded(activeId, ignore.excludedIntentIds)) { + return { + blocked: true, + error: formatResponse.toolError( + `Intent ${activeId} is listed in .intentignore and cannot be modified. Choose another intent or ask the user to update .intentignore.`, + ), + } + } + + // Scope and .intentignore path checks for file-writing tools + const filePathParam = + toolName === "write_to_file" + ? params.path + : [ + "edit", + "search_and_replace", + "search_replace", + "edit_file", + "apply_patch", + "apply_diff", + ].includes(toolName) + ? (params.file_path ?? params.path) + : undefined + if (filePathParam) { + const relPath = String(filePathParam) if (isPathTraversal(relPath, cwd)) { return { blocked: true, error: `Path traversal not allowed: "${relPath}" would escape the workspace. Use a path relative to the workspace only.`, } } + if (isPathIgnored(relPath, ignore.pathPatterns)) { + return { + blocked: true, + error: formatResponse.toolError( + `Path "${relPath}" is excluded by .intentignore. You are not authorized to edit it.`, + ), + } + } const context = await loadIntentContext(cwd, activeId) if (context && context.owned_scope.length > 0 && !pathInScope(relPath, context.owned_scope, cwd)) { return { blocked: true, - error: `Scope Violation: ${activeId} is not authorized to edit "${relPath}". Request scope expansion in active_intents.yaml or choose another intent.`, + error: formatResponse.toolErrorScopeViolation(activeId, relPath), } } } } - // Optional: HITL for destructive tools (wire via askApproval in host / extension) + // UI-blocking authorization for destructive tools (e.g. showWarningMessage Approve/Reject) + if (isDestructive && this.options.confirmDestructive) { + const approved = await this.options.confirmDestructive(toolName, params) + if (!approved) { + return { + blocked: true, + error: formatResponse.toolErrorUserRejected(toolName), + } + } + } + return { blocked: false } } } diff --git a/src/hooks/scope.ts b/src/hooks/scope.ts index 9ee969a421..5c54738b55 100644 --- a/src/hooks/scope.ts +++ b/src/hooks/scope.ts @@ -37,3 +37,24 @@ function simpleGlobMatch(path: string, pattern: string): boolean { ) return re.test(path) } + +/** + * Check if a relative path matches any of the given glob-like patterns. + * Used by .intentignore to exclude paths from edits. + */ +export function pathMatchesAnyPattern(relativePath: string, patterns: string[]): boolean { + if (!patterns || patterns.length === 0) return false + const normalized = path.normalize(relativePath).replace(/\\/g, "/") + for (const pattern of patterns) { + const p = path.normalize(pattern).replace(/\\/g, "/") + if (p.endsWith("/**")) { + const prefix = p.slice(0, -3) + if (normalized === prefix || normalized.startsWith(prefix + "/")) return true + } else if (p.includes("*")) { + if (simpleGlobMatch(normalized, p)) return true + } else { + if (normalized === p || normalized.endsWith("/" + p)) return true + } + } + return false +} diff --git a/src/hooks/types.ts b/src/hooks/types.ts index 3b41f12a54..5fde102df2 100644 --- a/src/hooks/types.ts +++ b/src/hooks/types.ts @@ -64,6 +64,14 @@ export interface AgentTraceEntry { /** Safe = read-only; Destructive = write, delete, execute */ export type CommandClass = "safe" | "destructive" +/** + * Classify a tool name as Safe (read) or Destructive (write, delete, execute). + * Used by the Hook Engine for authorization and UI-blocking. + */ +export function classifyCommand(toolName: string): CommandClass { + return (DESTRUCTIVE_TOOLS as readonly string[]).includes(toolName) ? "destructive" : "safe" +} + export const DESTRUCTIVE_TOOLS = [ "write_to_file", "apply_diff",