Wire up after_tool_call hook in agent execution loop#6264
Wire up after_tool_call hook in agent execution loop#6264xampla wants to merge 1 commit intoopenclaw:mainfrom
Conversation
src/agents/pi-tools.hooks.ts
Outdated
| const reason = beforeResult.blockReason ?? "Blocked by plugin hook"; | ||
| log.debug(`before_tool_call: blocked ${toolName} — ${reason}`); | ||
| return { | ||
| content: [{ type: "text", text: `[Tool call blocked] ${reason}` }], | ||
| details: { blocked: true, reason }, | ||
| }; |
There was a problem hiding this comment.
[P1] Tool-call hooks bypass tool_result_persist transcript guard
When before_tool_call blocks, the wrapper returns a synthetic tool result ([Tool call blocked] ...). That synthetic result currently won’t go through the existing tool_result_persist hook path (which is where plugins can redact/shape tool-result transcript content), because it’s created inside the tool wrapper rather than at the transcript-persist boundary. This matters when plugins rely on tool_result_persist to strip sensitive blockReason details or standardize tool-result messages; blocked calls would be an exception.
If the intent is that blocked tool calls should be treated like any other tool result for transcript policy/redaction, consider emitting them through the same path (or at least ensure the persist hook sees them as synthetic).
(Also applies to the identical block return in wrapToolDefinitionWithHooks.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.hooks.ts
Line: 52:57
Comment:
[P1] Tool-call hooks bypass `tool_result_persist` transcript guard
When `before_tool_call` blocks, the wrapper returns a synthetic tool result (`[Tool call blocked] ...`). That synthetic result currently won’t go through the existing `tool_result_persist` hook path (which is where plugins can redact/shape tool-result transcript content), because it’s created inside the tool wrapper rather than at the transcript-persist boundary. This matters when plugins rely on `tool_result_persist` to strip sensitive `blockReason` details or standardize tool-result messages; blocked calls would be an exception.
If the intent is that blocked tool calls should be treated like any other tool result for transcript policy/redaction, consider emitting them through the same path (or at least ensure the persist hook sees them as synthetic).
(Also applies to the identical block return in `wrapToolDefinitionWithHooks`.)
How can I resolve this? If you propose a fix, please make it concise.| } catch (err) { | ||
| error = String(err); | ||
| throw err; | ||
| } finally { |
There was a problem hiding this comment.
[P3] after_tool_call error string may be low-signal
In the catch block you set error = String(err) and then rethrow. For many thrown Errors, String(err) is just the message and drops the stack/type/context. If hooks are used for auditing/debugging, passing something like err instanceof Error ? err.stack ?? err.message : String(err) tends to be more actionable.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.hooks.ts
Line: 75:78
Comment:
[P3] `after_tool_call` error string may be low-signal
In the catch block you set `error = String(err)` and then rethrow. For many thrown Errors, `String(err)` is just the message and drops the stack/type/context. If hooks are used for auditing/debugging, passing something like `err instanceof Error ? err.stack ?? err.message : String(err)` tends to be more actionable.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.5343c58 to
adad8bf
Compare
|
This PR fixes #5943 and #5513. It supersedes #2340 (by @AhmedTheGeek) and the closed #2363, rebasing the work onto current main with fixes for:
This also unblocks #6232 (Security & Access Control) by enabling plugins to intercept and block tool calls before execution. |
c253cd5 to
b6c0f7c
Compare
|
following |
Now that before_tool_call is implemented (openclaw#6570), this PR adds the complementary after_tool_call hook for post-execution telemetry. - Add pi-tools.hooks.ts: wraps tool execute methods to fire after_tool_call - Hook fires as fire-and-forget after tool returns with timing data - Provides: toolName, params, result, error, durationMs - Hook context includes agentId and sessionKey for correlation - Hook errors are caught and logged, never breaking tool execution - Tools are only wrapped when hooks are actually registered (zero overhead) This addresses another hook from openclaw#6535 (2 of 8 now working). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
b6c0f7c to
e0b6f84
Compare
|
Closing as duplicate of #10678. If this is incorrect, comment and we can reopen. |
Summary
Now that
before_tool_callis implemented (#6570), this PR adds the complementaryafter_tool_callhook for post-execution telemetry.Changes
pi-tools.hooks.ts: wraps tool execute methods to fireafter_tool_calltoolName,params,result,error,durationMsagentIdandsessionKeyfor correlationAPI Usage
Plugins can register
after_tool_callhandlers:Related