← Back to PRs

#19422: fix: pass session context to plugin tool hooks in toToolDefinitions

by namabile open 2026-02-17 19:42 View on GitHub →
agents size: S
## Summary Fixes #19381 Plugin-registered tools that go through `toToolDefinitions` without being pre-wrapped by `wrapToolWithBeforeToolCallHook` receive `undefined` for both `agentId` and `sessionKey` in their `before_tool_call` and `after_tool_call` hooks, making per-session access control impossible. ### Root cause `toToolDefinitions` has a fallback path (line 103-108) for unwrapped tools that calls `runBeforeToolCallHook` without passing `ctx`. Additionally, `after_tool_call` hooks (both success and error paths) only pass `{ toolName }` as context, never `agentId`/`sessionKey`. ### Changes - **`src/agents/pi-tool-definition-adapter.ts`**: Accept optional `hookContext` parameter in `toToolDefinitions`. Pass it to `runBeforeToolCallHook` for unwrapped tools and include `agentId`/`sessionKey` in all `after_tool_call` hook contexts. - **`src/agents/pi-embedded-runner/tool-split.ts`**: Forward `hookContext` through `splitSdkTools` to `toToolDefinitions`. - **`src/agents/pi-embedded-runner/run/attempt.ts`**: Pass `sessionAgentId`/`sessionKey` as hook context to `splitSdkTools`. - **`src/agents/pi-embedded-runner/compact.ts`**: Pass `sessionAgentId`/`sessionKey` as hook context to `splitSdkTools`. - **`src/agents/pi-tool-definition-adapter.hook-context.test.ts`**: 4 regression tests covering: - `before_tool_call` receives `agentId`/`sessionKey` for unwrapped tools - `after_tool_call` receives context on success - `after_tool_call` receives context on error - Backwards compatibility when no `hookContext` is provided ### Test plan - [x] All 4 new regression tests pass - [x] All 395 existing agent tests pass (65 test files) - [x] `pnpm format:check` — clean - [x] `pnpm tsgo` — clean - [x] `pnpm lint` — 0 warnings, 0 errors <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR correctly fixes the root cause described in issue #19381: plugin tools processed through `toToolDefinitions` without being pre-wrapped by `wrapToolWithBeforeToolCallHook` were receiving `undefined` for `agentId` and `sessionKey` in their `before_tool_call` and `after_tool_call` hooks, making per-session access control impossible. The fix threads an optional `hookContext` parameter through `splitSdkTools` → `toToolDefinitions` and wires it into all three hook invocation sites. Both call sites (`attempt.ts` and `compact.ts`) are updated consistently. Key observations: - The core fix is correct and the approach is clean and backward-compatible (optional parameter with `undefined` fallback). - There is a pre-existing inconsistency now made more consequential: the `after_tool_call` success path passes the raw, un-normalized tool `name` as `toolName` in both event payload and context (lines 129, 134), while the error path passes `normalizedName` (lines 179, 184). Since plugins can now rely on `toolName` in the hook context for access control routing, receiving different values for the same tool depending on success vs. failure is a correctness hazard. - The regression tests use all-lowercase tool names (`"my_plugin_tool"`, `"failing_tool"`), which are unchanged by `normalizeToolName`, so the `name` vs. `normalizedName` discrepancy is not caught. <h3>Confidence Score: 3/5</h3> - Safe to merge with one logic issue that should be addressed before plugins rely on consistent toolName values in after_tool_call contexts. - The fix correctly addresses the reported bug and is backward-compatible. However, a pre-existing inconsistency — raw name on the success path vs normalizedName on the error path in after_tool_call — becomes a real correctness hazard now that hookContext is plumbed through and plugins can perform access control using toolName. The regression tests don't cover this edge case. This warrants attention before the feature is relied upon in production plugins. - src/agents/pi-tool-definition-adapter.ts (lines 129 and 134: success-path after_tool_call uses raw name instead of normalizedName) <sub>Last reviewed commit: 4ec8a93</sub> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->

Most Similar PRs