Repository navigation
fix(ai): move tool discovery to global assistant + enforce page agent enabledTools - #1306
Conversation
… enabledTools - Apply stub schemas + tool_search to the global assistant route (the correct target — handles all 37 tools every request) - Revert stubs + tool_search from page AI chat route (wrong surface) - Enforce page.enabledTools server-side in the chat route: when a non-empty allowlist is configured, only those tools reach the model. This closes a security gap where agents could execute tools outside their configured allowlist via the stub/tool_search mechanism. - Update test mocks accordingly (add to global, remove from chat) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGlobal messages route adds explicit tool-discovery (TOOL_DISCOVERY_PROMPT), uses stubbed tools for read-only filtering, and injects a request-scoped tool_search. Chat route builds from full pageSpaceTools and enforces a server-side per-agent allowlist; system prompt export and test mocks updated accordingly. ChangesAI Tool Control and Discovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f61676b59d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let filteredTools: ToolSet = buildPageAITools(pageSpaceTools, { | ||
| isReadOnly: readOnlyMode, | ||
| webSearchEnabled: webSearchMode, | ||
| }); |
There was a problem hiding this comment.
This route now builds filteredTools from pageSpaceTools only, but for pages without a custom system prompt it still uses buildSystemPrompt(...) (later in this file), and that shared prompt includes guidance to call tool_search for non-core tools (apps/web/src/lib/ai/core/system-prompt.ts, TOOL_DISCOVERY_PROMPT). In that common path, the model can be instructed to invoke a tool that is no longer registered, causing invalid tool-call failures and degraded completions (often retries/loops) whenever it tries to discover calendar/agent/channel/task schemas.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac44d94. TOOL_DISCOVERY_PROMPT was removed from buildSystemPrompt (which is shared by both routes) and is now injected explicitly only in the global assistant route, immediately after baseSystemPrompt is constructed. The page AI chat no longer includes the instruction to call tool_search, so there is no risk of the model attempting an unregistered tool call.
buildSystemPrompt is shared by both the page AI chat and global assistant routes. Having TOOL_DISCOVERY_PROMPT in the shared prompt instructed page agents to call tool_search, which is not registered in the page AI chat tool set — causing invalid tool-call failures. Move TOOL_DISCOVERY_PROMPT out of buildSystemPrompt (export it), and inject it explicitly only in the global assistant route where tool_search is actually available. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/ai/chat/route.ts (1)
277-281:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment contradicts new behavior.
This comment states
page.enabledToolsis "intentionally NOT consulted here," but the new code at lines 516-526 now explicitly enforcespage.enabledToolsas a server-side allowlist. The comment should be updated to reflect the new security enforcement behavior.📝 Suggested comment update
- // Extract custom agent configuration from page. - // Note: page.enabledTools is intentionally NOT consulted here. It seeds the - // composer's tool toggles on the client; the toggles in the request body - // are the source of truth at runtime so the UI doesn't lie about what the - // model can actually do. + // Extract custom agent configuration from page. + // Note: page.enabledTools is enforced server-side as an allowlist (lines 516-526). + // When non-empty, only explicitly listed tools are available to the agent. + // Runtime toggles (isReadOnly, webSearchEnabled) are applied first, then the + // allowlist further restricts the available tool set. const customSystemPrompt = page.systemPrompt;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/ai/chat/route.ts` around lines 277 - 281, Update the stale comment in apps/web/src/app/api/ai/chat/route.ts to reflect that page.enabledTools is now enforced server-side as an allowlist; replace the line stating "page.enabledTools is intentionally NOT consulted here" with a concise note that the server now intersects/validates requested tool toggles against page.enabledTools as an allowlist (so client toggles are still seeded from the page but the server enforces allowed tools), and reference the relevant logic that performs this enforcement (the code handling the request body/tool toggles and the page.enabledTools check).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/src/app/api/ai/chat/route.ts`:
- Around line 277-281: Update the stale comment in
apps/web/src/app/api/ai/chat/route.ts to reflect that page.enabledTools is now
enforced server-side as an allowlist; replace the line stating
"page.enabledTools is intentionally NOT consulted here" with a concise note that
the server now intersects/validates requested tool toggles against
page.enabledTools as an allowlist (so client toggles are still seeded from the
page but the server enforces allowed tools), and reference the relevant logic
that performs this enforcement (the code handling the request body/tool toggles
and the page.enabledTools check).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd032a29-fe02-409b-822f-e7e44c06f625
📒 Files selected for processing (6)
apps/web/src/app/api/ai/chat/__tests__/mcp-scope.test.tsapps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/global/[id]/messages/route.tsapps/web/src/lib/ai/core/system-prompt.ts
💤 Files with no reviewable changes (2)
- apps/web/src/app/api/ai/chat/tests/stream-socket-events.test.ts
- apps/web/src/app/api/ai/chat/tests/mcp-scope.test.ts
…date stale comment Add missing `TOOL_DISCOVERY_PROMPT` export to the `@/lib/ai/core` mock in the global assistant stream-socket-events test. The route now imports this constant and concatenates it into the system prompt string; without the mock entry it resolved as `undefined`, causing the system prompt to contain the literal string "undefined" and triggering an early exit in the route before `createStreamLifecycle` was called — causing all 16 lifecycle tests to fail with "called 0 times". Also update the stale comment in the page AI chat route that previously stated `page.enabledTools` is "intentionally NOT consulted here". The server now enforces it as an allowlist (added in this PR), so the comment is updated to reflect the actual enforcement order: `buildPageAITools` applies runtime toggles first, then the `agentEnabledTools` filter restricts to the per-agent allowlist. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Stale comment at lines 277–281 (flagged by CodeRabbit) has been updated in 908be1d to accurately describe the server-side enforcement order: |
… enabledTools (#1306) * fix(ai): move tool discovery to global assistant + enforce page agent enabledTools - Apply stub schemas + tool_search to the global assistant route (the correct target — handles all 37 tools every request) - Revert stubs + tool_search from page AI chat route (wrong surface) - Enforce page.enabledTools server-side in the chat route: when a non-empty allowlist is configured, only those tools reach the model. This closes a security gap where agents could execute tools outside their configured allowlist via the stub/tool_search mechanism. - Update test mocks accordingly (add to global, remove from chat) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ai): scope TOOL_DISCOVERY_PROMPT to global assistant only buildSystemPrompt is shared by both the page AI chat and global assistant routes. Having TOOL_DISCOVERY_PROMPT in the shared prompt instructed page agents to call tool_search, which is not registered in the page AI chat tool set — causing invalid tool-call failures. Move TOOL_DISCOVERY_PROMPT out of buildSystemPrompt (export it), and inject it explicitly only in the global assistant route where tool_search is actually available. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ai): add TOOL_DISCOVERY_PROMPT to global assistant test mock + update stale comment Add missing `TOOL_DISCOVERY_PROMPT` export to the `@/lib/ai/core` mock in the global assistant stream-socket-events test. The route now imports this constant and concatenates it into the system prompt string; without the mock entry it resolved as `undefined`, causing the system prompt to contain the literal string "undefined" and triggering an early exit in the route before `createStreamLifecycle` was called — causing all 16 lifecycle tests to fail with "called 0 times". Also update the stale comment in the page AI chat route that previously stated `page.enabledTools` is "intentionally NOT consulted here". The server now enforces it as an allowlist (added in this PR), so the comment is updated to reflect the actual enforcement order: `buildPageAITools` applies runtime toggles first, then the `agentEnabledTools` filter restricts to the per-agent allowlist. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
What changed
Infrastructure
All underlying infrastructure (`stub-tools.ts`, `tool-search-tool.ts`, `pageSpaceToolsStubbed`) landed in #1301 and is unchanged.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit