LCORE-1262: Resolve allowed tools filters in LCS#1412
Open
asimurka wants to merge 1 commit intolightspeed-core:mainfrom
Open
LCORE-1262: Resolve allowed tools filters in LCS#1412asimurka wants to merge 1 commit intolightspeed-core:mainfrom
asimurka wants to merge 1 commit intolightspeed-core:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements allowlist-based tool filtering for tool_choice: adds utilities to match/filter tools by key-valued allowlist entries (including MCP-specific rules), updates resolve_tool_choice control flow to apply filtering and early-exit cases, and documents the new allowlist semantics with tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver as resolve_tool_choice
participant Prep as prepare_tools
participant Filter as filter_tools_by_allowed_entries
participant Registry as ToolsRegistry
Client->>Resolver: call resolve_tool_choice(request, tool_choice, tools?)
Resolver->>Prep: prepare tools (if tools omitted) / translate provided tools
Prep-->>Resolver: prepared_tools (or None)
Resolver->>Filter: apply allowed_filters (if present) to prepared_tools
Filter->>Registry: match tool entries against configured tools
Registry-->>Filter: matched tools (with MCP-name narrowing)
Filter-->>Resolver: filtered_tools (or empty)
alt filtered_tools empty
Resolver-->>Client: return (None, None, None)
else
Resolver->>Resolver: derive vector_store_ids, set prepared_tool_choice
Resolver-->>Client: return (prepared_tools, vector_store_ids, prepared_tool_choice)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/responses.md`:
- Around line 407-408: Update the responses docs to keep `tool_choice`
documented as "string or object" (not just string) so clients know handlers may
pass through specific tool-choice objects (e.g., `file_search`, `function`,
`mcp`) while only `allowed_tools` is normalized to `auto`/`required`; modify the
table row for `tool_choice` to show "string or object" and add a short note
clarifying that `allowed_tools` is collapsed but handlers can still emit
structured tool-choice objects for normal/blocked responses.
In `@src/utils/responses.py`:
- Around line 1411-1423: The condition "if tools:" conflates None with an
explicit empty list; change the logic to check for presence vs explicit disable
by using "if tools is not None:" and explicitly handle the empty list case (when
tools == [] return the disabled result: None, None, None) before calling
prepare_tools()/translate_tools_vector_store_ids/filter_tools_by_allowed_entries/extract_vector_store_ids
and setting prepared_tool_choice (tool_choice or ToolChoiceMode.auto); apply the
same fix to the other analogous block that also inspects tools (the block around
the later use at 1425-1440).
- Around line 423-463: The allowlist matcher must handle MCP server tools
specially: update _tool_matches_allowed_entry to detect MCP tools (e.g.,
isinstance(tool, OpenAIResponseInputToolMCP) or tool.type == "mcp") and, when
the allowlist entry includes a "name" key, verify the entry's server_label/type
match the tool and that the entry["name"] is present in tool.allowed_tools
(treat string attr comparison accordingly) instead of checking a non-existent
top-level name attribute; additionally, change filter_tools_by_allowed_entries
so that when an MCP tool matches an entry that narrows by name it returns a
projected copy of that tool with allowed_tools filtered to only the matching
names (preserving non-MCP behavior for other entries and tools).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b0c82c0-73f4-4d76-8668-d74645d34708
📒 Files selected for processing (4)
docs/responses.mdsrc/app/endpoints/responses.pysrc/utils/responses.pytests/unit/utils/test_responses.py
asimurka
force-pushed
the
responses_implement_allowed_tools_filtering
branch
from
88711a8 to
2efc922
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/responses.md (1)
283-288: Grammar: Use hyphen for compound adjective.Per static analysis, "file only search" should be "file-only search" (compound adjective).
📝 Proposed fix
-- `allowed_tools`: Restrict to a list of tool definitions; `mode` is `"auto"` or `"required"`, `tools` is a list of key-valued filters for tools configured by `tools` attribute. -- `file_search`: Force the model to use file only search. -- `web_search`: Force the model to use only web search. +- `allowed_tools`: Restrict to a list of tool definitions; `mode` is `"auto"` or `"required"`, `tools` is a list of key-valued filters for tools configured by `tools` attribute. +- `file_search`: Force the model to use file-only search. +- `web_search`: Force the model to use web-only search.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/responses.md` around lines 283 - 288, The phrase "Force the model to use file only search" in the `file_search` bullet is missing a hyphen; update the `file_search` description to read "Force the model to use file-only search" so the compound adjective is correct (locate the `file_search` entry in docs/responses.md and apply the change).src/utils/responses.py (1)
555-558: Fix typo in comment.Line 555: "handler" should be "handled".
📝 Proposed fix
- # Non-MCP tools pass through and are handler separately + # Non-MCP tools pass through and are handled separately if tool.type != "mcp": filtered.append(tool) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/responses.py` around lines 555 - 558, Fix the typo in the comment above the MCP filter: change "handler" to "handled" in the comment that reads "# Non-MCP tools pass through and are handler separately" in src/utils/responses.py so it correctly reads "# Non-MCP tools pass through and are handled separately" near the conditional that checks if tool.type != "mcp" (the filtered.append(tool); continue block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/responses.md`:
- Around line 283-288: The phrase "Force the model to use file only search" in
the `file_search` bullet is missing a hyphen; update the `file_search`
description to read "Force the model to use file-only search" so the compound
adjective is correct (locate the `file_search` entry in docs/responses.md and
apply the change).
In `@src/utils/responses.py`:
- Around line 555-558: Fix the typo in the comment above the MCP filter: change
"handler" to "handled" in the comment that reads "# Non-MCP tools pass through
and are handler separately" in src/utils/responses.py so it correctly reads "#
Non-MCP tools pass through and are handled separately" near the conditional that
checks if tool.type != "mcp" (the filtered.append(tool); continue block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2aa94315-b7cb-453b-b203-25b1b3cefca2
📒 Files selected for processing (4)
docs/responses.mdsrc/app/endpoints/responses.pysrc/utils/responses.pytests/unit/utils/test_responses.py
✅ Files skipped from review due to trivial changes (1)
- src/app/endpoints/responses.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/utils/test_responses.py
asimurka
force-pushed
the
responses_implement_allowed_tools_filtering
branch
from
2efc922 to
23b4fde
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/responses.md`:
- Line 284: Update the documentation line for the `file_search` modifier: change
the phrase "file only search" to the hyphenated compound modifier "file-only
search" so the entry reads "`file_search`: Force the model to use file-only
search." Reference: the `file_search` entry in docs/responses.md.
In `@src/utils/responses.py`:
- Line 555: Fix the typo in the inline comment "# Non-MCP tools pass through and
are handler separately" by changing "handler" to "handled" so the comment reads
"# Non-MCP tools pass through and are handled separately"; locate this exact
comment string in src/utils/responses.py and update it accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69803068-2398-43aa-8f3c-2e3716705d5b
📒 Files selected for processing (4)
docs/responses.mdsrc/app/endpoints/responses.pysrc/utils/responses.pytests/unit/utils/test_responses.py
✅ Files skipped from review due to trivial changes (1)
- src/app/endpoints/responses.py
asimurka
changed the title
asimurka
force-pushed
the
responses_implement_allowed_tools_filtering
branch
3 times, most recently
from
2126ba5 to
06adfc8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/responses.md (1)
408-409:⚠️ Potential issue | 🟡 Minor
tool_choiceresponse type is still documented too narrowly.Line 409 says
tool_choiceisstring, butsrc/utils/responses.py(resolve_tool_choice, Line 1485-1557) still returnsOptional[ToolChoice], and object-shaped choices are validated in tests (tests/unit/utils/test_responses.py, Line 967-984).Suggested doc correction
-| `tool_choice` | string | Internally resolved tool choice mode | +| `tool_choice` | string or object | Internally resolved tool choice. `allowed_tools` is normalized to mode (`auto`/`required`), while specific tool-choice objects may still be returned. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/responses.md` around lines 408 - 409, The docs currently list `tool_choice` as just a string, but resolve_tool_choice in src/utils/responses.py returns Optional[ToolChoice] (and tests validate object-shaped choices), so update the responses.md table entry for `tool_choice` to reflect it can be an object matching the ToolChoice shape or a string (and may be null/absent); reference the resolve_tool_choice function and the ToolChoice type when describing the object shape so readers know the expected fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/responses.md`:
- Line 283: The docs entry for allowed_tools is misleading: it says "list of
tool definitions" but the implementation expects key-value allowlist filter
entries rather than full tool definitions; update the wording for the
allowed_tools field to state that allowed_tools is a filter list (not full tool
definitions), explain that mode is "auto" or "required", and that tools is a
list of key-value filter objects which match configured tools via the tools
attribute (e.g., name, category, or tag filters), so callers know to supply
filter entries rather than entire tool definitions.
---
Duplicate comments:
In `@docs/responses.md`:
- Around line 408-409: The docs currently list `tool_choice` as just a string,
but resolve_tool_choice in src/utils/responses.py returns Optional[ToolChoice]
(and tests validate object-shaped choices), so update the responses.md table
entry for `tool_choice` to reflect it can be an object matching the ToolChoice
shape or a string (and may be null/absent); reference the resolve_tool_choice
function and the ToolChoice type when describing the object shape so readers
know the expected fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb6c9d44-6d2b-4510-a66f-926499178900
📒 Files selected for processing (3)
docs/responses.mdsrc/utils/responses.pytests/unit/utils/test_responses.py
✅ Files skipped from review due to trivial changes (1)
- src/utils/responses.py
19 tasks
asimurka
force-pushed
the
responses_implement_allowed_tools_filtering
branch
5 times, most recently
from
39929d9 to
1a288ae
Compare
asimurka
force-pushed
the
responses_implement_allowed_tools_filtering
branch
from
1a288ae to
7bec21f
Compare
asimurka
force-pushed
the
responses_implement_allowed_tools_filtering
branch
from
7bec21f to
9ef5d2e
Compare
asimurka
commented
Mar 30, 2026
| responses_request.tool_choice, | ||
| vector_store_ids, | ||
| ) = await resolve_tool_choice( | ||
| # Extract vector store IDs for Inline RAG context before resolving tool choice. |
Contributor
Author
There was a problem hiding this comment.
This is done in order to be consistent with recent changes in query endpoints. no_tools or tool_choice in this case should not affect inline RAG vector store selection. That's why tool resolution is done after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds support for resolving
allowed_toolswithinresolve_tool_choiceinstead of passing it to LLS. Whenallowed_toolsis provided, the function now applies the allowlist directly by filtering the available tools based on the specified criteria (such astype,name, orserver_label). At the same time,tool_choiceis normalized so that it is always converted to a standardToolChoiceMode, rather than keeping the original object form. As a result,allowed_toolsis no longer propagated further; instead, its effect is reflected in the filteredtoolsset. If no tools remain after filtering, the function returns(None, None)which is consistent with Responses API behavior.Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Tests