feat: interceptor pipeline for tool, message, and params events#6569
feat: interceptor pipeline for tool, message, and params events#6569pauloportella wants to merge 7 commits intoopenclaw:mainfrom
Conversation
Standalone module that lets code and plugins intercept tool arguments before execution and tool results after execution. Independent from existing hooks and plugin-hooks systems.
…) with toolMatcher validation
…okens and shell profiles
New interceptor events fire before the agent processes a message and before session creation, enabling message enrichment, classification, and dynamic LLM parameter overrides via a metadata bag that flows between events. Adds agentMatcher for scoping to specific agents.
Add onEvent callback to interceptor registry for verbose mode visibility. Interceptor events (blocks, mutations) appear as Discord status lines. Defer tool summary emission to tool_execution_end so blocked tools only show the shield line, not a misleading tool summary.
| // --- tool.after --- | ||
| if (registry) { | ||
| const afterOutput = await trigger( | ||
| registry, | ||
| "tool.after", | ||
| { toolName: normalizedName, toolCallId, isError: false }, | ||
| { result }, | ||
| ); | ||
| result = afterOutput.result; |
There was a problem hiding this comment.
[P1] tool.after interceptors never run for tool execution failures.
If tool.execute(...) throws, this adapter returns an error jsonResult(...) early (src/agents/pi-tool-definition-adapter.ts:88-93) and skips the tool.after trigger entirely. That means tool.after interceptors can’t observe/redact/annotate failures (even though ToolAfterInput includes isError). If the intended pipeline is tool.before → [tool executes] → tool.after for both success and failure, you likely want to call trigger(..., "tool.after", { isError: true }, { result: <errorResult> }) before returning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tool-definition-adapter.ts
Line: 95:103
Comment:
[P1] `tool.after` interceptors never run for tool execution failures.
If `tool.execute(...)` throws, this adapter returns an `error` `jsonResult(...)` early (src/agents/pi-tool-definition-adapter.ts:88-93) and skips the `tool.after` trigger entirely. That means `tool.after` interceptors can’t observe/redact/annotate failures (even though `ToolAfterInput` includes `isError`). If the intended pipeline is `tool.before → [tool executes] → tool.after` for both success and failure, you likely want to call `trigger(..., "tool.after", { isError: true }, { result: <errorResult> })` before returning.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in c98ff8a — tool.after now fires with isError: true when tool.execute() throws.
There was a problem hiding this comment.
Fixed in c98ff8a — tool.after now fires with isError: true when tool.execute() throws.
| function validateToolMatcher(id: string, matcher: RegExp): void { | ||
| const matchesAny = [...KNOWN_TOOL_NAMES].some((name) => matcher.test(name)); | ||
| if (!matchesAny) { | ||
| throw new Error( | ||
| `Interceptor "${id}": toolMatcher ${matcher} does not match any known tool name. ` + | ||
| `Known tools: ${[...KNOWN_TOOL_NAMES].join(", ")}`, | ||
| ); |
There was a problem hiding this comment.
[P2] validateToolMatcher can change RegExp.lastIndex for /g or /y matchers.
validateToolMatcher iterates KNOWN_TOOL_NAMES and calls matcher.test(name) (src/interceptors/registry.ts:16). If a caller registers a global/sticky regex, test() advances lastIndex, which can make later validation checks and/or subsequent matching in get() behave inconsistently. This only affects interceptors that accidentally include g/y flags, but it’s a tricky failure mode.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/interceptors/registry.ts
Line: 15:21
Comment:
[P2] `validateToolMatcher` can change `RegExp.lastIndex` for `/g` or `/y` matchers.
`validateToolMatcher` iterates `KNOWN_TOOL_NAMES` and calls `matcher.test(name)` (src/interceptors/registry.ts:16). If a caller registers a global/sticky regex, `test()` advances `lastIndex`, which can make later validation checks and/or subsequent matching in `get()` behave inconsistently. This only affects interceptors that accidentally include `g`/`y` flags, but it’s a tricky failure mode.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in c98ff8a — added matcher.lastIndex = 0 before each test() call.
| function extractPathArgs(cmd: string): string[] { | ||
| const paths: string[] = []; | ||
| // Split on pipes/semicolons/&& to get individual commands | ||
| const segments = cmd.split(/[|;&]+/); | ||
| for (const segment of segments) { | ||
| const trimmed = segment.trim(); | ||
| // Skip if no file-read or file-copy command | ||
| if (!FILE_READ_COMMANDS.test(trimmed) && !FILE_COPY_COMMANDS.test(trimmed)) { | ||
| continue; |
There was a problem hiding this comment.
[P2] Path extraction may miss cases because segments aren’t split on && / ||.
extractPathArgs splits commands with /[|;&]+/ (src/interceptors/builtin/command-safety-guard.ts:98), which doesn’t split on && or ||. For commands like echo ok && cat ~/.bashrc, tokenization can leave trailing && attached/adjacent and make path detection less reliable, potentially missing sensitive reads/copies depending on spacing/quoting.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/interceptors/builtin/command-safety-guard.ts
Line: 95:103
Comment:
[P2] Path extraction may miss cases because segments aren’t split on `&&` / `||`.
`extractPathArgs` splits commands with `/[|;&]+/` (src/interceptors/builtin/command-safety-guard.ts:98), which doesn’t split on `&&` or `||`. For commands like `echo ok && cat ~/.bashrc`, tokenization can leave trailing `&&` attached/adjacent and make path detection less reliable, potentially missing sensitive reads/copies depending on spacing/quoting.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
The regex /[|;&]+/ already handles && and || — the + quantifier matches one or more chars from [|;&], so && splits correctly. E.g. "echo ok && cat ~/.bashrc".split(/[|;&]+/) produces ["echo ok ", " cat ~/.bashrc"].
…tIndex - tool.after now fires with isError: true when tool.execute() throws, so interceptors can observe/redact/annotate failures - Reset regex lastIndex before each test() call in validateToolMatcher to prevent inconsistent matching with global/sticky regexes
Thanks @pauloportella, happy to see other people doing good work in this area. I was initially concerned about the potentially breaking impact of extending the plugin hooks, but as far as I can tell, the before_tool_call and after_tool_call hooks we modified were never wired up (see e.g. #5513). Digging into your changes now, looking forward to seeing how you've approached the issue. |
|
@pauloportella hey you probably have noticed but we have the above PR mentioned opened! @Scrattlebeard also tried to have an inital attempt on the security-audit and command safe guard you were mentioning earlier! Feel free to check it out and would love hear your thoughts! |
|
#6095 👍🏼 |
Summary
Adds an interceptor system, a typed, priority-sorted pipeline that hooks into tool execution, message processing, and LLM parameter selection. Interceptors can inspect, mutate, or block operations before they reach the agent.
tool.before/tool.after: Inspect and block tool calls (e.g. deny dangerous shell commands, audit file reads)message.before: Enrich or transform messages before the agent session, with a metadata bag for inter-interceptor communicationparams.before: Override LLM parameters (thinkLevel, reasoningLevel, temperature) based on message metadatacommand-safety-guard(blocks destructive/sensitive shell commands) andsecurity-audit(blocks reads of credential files, SSH keys, API tokens)tool_execution_endso blocked tools don't show a misleading "tool executed" lineNote to maintainers
I understand you may have a different vision or approach to achieving this. This PR is primarily intended to share an approach that has been working well in my own deployment. Happy to discuss, adapt, or close if it doesn't align with the project's direction. Hopefully it's useful as a reference either way.
Architecture
Each interceptor is registered with a name, priority, and optional
toolMatcher/agentMatcherregex for scoping. Handlers receive immutable input and mutable output, running sequentially by priority (highest first).Test plan
pnpm buildcleanpnpm lintclean (no new errors)Greptile Overview
Greptile Summary
This PR introduces a typed interceptor pipeline (registry + trigger) and wires it into the embedded runner and tool execution path, allowing
message.before,params.before,tool.before, andtool.afterhooks. It also adds two built-in security interceptors (command-safety-guardfor blocking dangerous exec commands andsecurity-auditfor blocking sensitive file reads/writes/edits), plus verbose event formatting and deferred tool summary emission so blocked tools don’t appear as “executed”.Overall the architecture fits the existing embedded agent flow by intercepting prompts/params in
runEmbeddedAttemptand wrapping tool execution inpi-tool-definition-adapter, while the subscribe handlers now defer emitting tool summaries untiltool_execution_endso blocked tools are suppressed.Key issues to address are around completeness and edge cases (e.g.,
tool.afternot being invoked for failure results, and a couple subtle regex/command-parsing pitfalls in matcher validation and command-safety-guard).Confidence Score: 3/5
tool.aftercurrently won’t run for tool failures (reducing interceptor coverage for error redaction/auditing), and there are subtle edge cases in regex matcher validation and exec command parsing that could lead to inconsistent behavior.(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!