LCORE-1211 Fix /v1/tools returning empty fields for all tools#1246
Merged
tisnik merged 2 commits intolightspeed-core:mainfrom Mar 2, 2026
Merged
Conversation
Contributor
WalkthroughAdds normalization for tool dictionaries in the tools endpoint: converts ToolDef Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
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 `@src/app/endpoints/tools.py`:
- Around line 158-167: The code currently only remaps missing keys; update it to
treat empty values as missing too by checking falsy values rather than presence.
For identifier/name: replace the presence-only check with something like if
tool_dict.get("identifier") in (None, "") and "name" in tool_dict then set
tool_dict["identifier"]=tool_dict.pop("name"); for parameters/input_schema: if
not tool_dict.get("parameters") and "input_schema" in tool_dict then
tool_dict["parameters"]=_input_schema_to_parameters(tool_dict.pop("input_schema"));
and replace the setdefault calls for provider_id and type with conditional
assignments that overwrite empty/falsy values (e.g., if not
tool_dict.get("provider_id"): tool_dict["provider_id"]=toolgroup.provider_id and
if not tool_dict.get("type"): tool_dict["type"]=getattr(toolgroup, "type", None)
or "tool").
In `@tests/unit/app/endpoints/test_tools.py`:
- Around line 971-1081: Add a regression variant to
test_tools_endpoint_rag_builtin_toolgroup that simulates legacy empty fields:
when constructing the rag_tool via _make_tool_def_mock, include identifier="",
provider_id="", type="", and parameters=[] alongside the valid name and
input_schema, then call tools.tools_endpoint_handler (as already done) and
assert the returned ToolsResponse/tools[0] fields (identifier, provider_id,
type, server_source, toolgroup_id, and parameters converted from input_schema)
are populated from the modern fields; update/mock_client.tools.list to return
this modified rag_tool and keep the existing assertions to verify the endpoint
overrides empty legacy placeholders with the correct values.
Contributor
Author
|
@radofuchs @tisnik Could you PTAL? |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/endpoints/tools.py (1)
65-85: Return a normalized copy instead of mutatingtool_dictin place.Line 65 currently mutates input and Line 178 relies on side effects. Returning a new dict makes the flow safer and easier to reason about.
♻️ Proposed refactor
-def _normalize_tool_dict(tool_dict: dict[str, Any], toolgroup: Any) -> None: +def _normalize_tool_dict(tool_dict: dict[str, Any], toolgroup: Any) -> dict[str, Any]: """Normalize a ToolDef dict to the endpoint's response format. @@ - if "name" in tool_dict and not tool_dict.get("identifier"): - tool_dict["identifier"] = tool_dict["name"] - tool_dict.pop("name", None) + normalized_tool_dict = dict(tool_dict) + if "name" in normalized_tool_dict and not normalized_tool_dict.get("identifier"): + normalized_tool_dict["identifier"] = normalized_tool_dict["name"] + normalized_tool_dict.pop("name", None) - if "input_schema" in tool_dict and not tool_dict.get("parameters"): - tool_dict["parameters"] = _input_schema_to_parameters(tool_dict["input_schema"]) - tool_dict.pop("input_schema", None) + if "input_schema" in normalized_tool_dict and not normalized_tool_dict.get("parameters"): + normalized_tool_dict["parameters"] = _input_schema_to_parameters( + normalized_tool_dict["input_schema"] + ) + normalized_tool_dict.pop("input_schema", None) - if not tool_dict.get("provider_id"): - tool_dict["provider_id"] = toolgroup.provider_id - if not tool_dict.get("type"): - tool_dict["type"] = getattr(toolgroup, "type", None) or "tool" + if not normalized_tool_dict.get("provider_id"): + normalized_tool_dict["provider_id"] = toolgroup.provider_id + if not normalized_tool_dict.get("type"): + normalized_tool_dict["type"] = getattr(toolgroup, "type", None) or "tool" + + return normalized_tool_dict @@ - _normalize_tool_dict(tool_dict, toolgroup) + tool_dict = _normalize_tool_dict(tool_dict, toolgroup)As per coding guidelines: "Avoid in-place parameter modification anti-patterns; return new data structures instead".
Also applies to: 178-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/tools.py` around lines 65 - 85, _update _normalize_tool_dict to return a new normalized dict instead of mutating the input: copy the incoming tool_dict (shallow), apply the renames and default propagation on the copy (handle "name" -> "identifier", "input_schema" -> "parameters" via _input_schema_to_parameters, set provider_id and type from toolgroup), and return that new dict; then update all call sites that currently rely on side effects (e.g., the caller that previously ignored the return at the place referenced by the review) to use the returned value (assign/replace the original variable with the returned dict) so no in-place modification is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/endpoints/tools.py`:
- Around line 65-85: _update _normalize_tool_dict to return a new normalized
dict instead of mutating the input: copy the incoming tool_dict (shallow), apply
the renames and default propagation on the copy (handle "name" -> "identifier",
"input_schema" -> "parameters" via _input_schema_to_parameters, set provider_id
and type from toolgroup), and return that new dict; then update all call sites
that currently rely on side effects (e.g., the caller that previously ignored
the return at the place referenced by the review) to use the returned value
(assign/replace the original variable with the returned dict) so no in-place
modification is required.
4 tasks
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
2 participants
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
Fix the
/v1/toolsendpoint. It was returning emptyidentifier,parameters,provider_id, andtypefields for all tools. Reason: the Llama Stack SDK (v0.4.x) changed field names (e.g.identifiertoname) and replaced theparameterslist with theinput_schemajson; alsoprovider_idandtypefields were moved. This PR maps/propagates everything correctly.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
Manual verification
Unit tests
Summary by CodeRabbit
Refactor
Tests