Skip to content

RSPEED-2523: fix missing type discriminator on MCP and file_search tool constructors#1261

Merged
tisnik merged 1 commit intolightspeed-core:mainfrom
major:RSPEED-2523-mcp-tool-type-discriminator
Mar 3, 2026
Merged

RSPEED-2523: fix missing type discriminator on MCP and file_search tool constructors#1261
tisnik merged 1 commit intolightspeed-core:mainfrom
major:RSPEED-2523-mcp-tool-type-discriminator

Conversation

@major
Copy link
Contributor

@major major commented Mar 3, 2026

Description

InputToolMCP() and InputToolFileSearch() constructors in src/utils/responses.py rely on Pydantic model defaults for the type field without explicitly setting it. llama_stack_client serializes with model_dump(exclude_unset=True), which drops fields that were never explicitly set, even if they have defaults. This removes the type discriminator and causes a ValueError downstream when deserializing the OpenAIResponseInputTool union.

This PR explicitly passes type="mcp" and type="file_search" to the constructors so the field is always considered "set" and survives serialization.

Type of change

  • Bug fix

Tools used to create PR

  • Assisted-by: Claude
  • Generated by: N/A

Related Tickets & Documents

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

Existing unit tests in tests/unit/utils/test_responses.py (TestGetRAGTools, TestGetMCPTools) already assert type on both constructors. All 18 tests pass:

tests/unit/utils/test_responses.py::TestGetRAGTools::test_get_rag_tools_empty_list PASSED
tests/unit/utils/test_responses.py::TestGetRAGTools::test_get_rag_tools_with_vector_stores PASSED
tests/unit/utils/test_responses.py::TestGetMCPTools::test_get_mcp_tools_without_auth PASSED
... (18/18 passed)

Pyright type checking: 0 errors, 0 warnings.

Summary by CodeRabbit

  • Refactor
    • Enhanced tool configuration with explicit type parameters for file search and MCP tools to ensure proper system initialization and configuration consistency.

…tructors

llama_stack_client serializes Pydantic models with
model_dump(exclude_unset=True), which drops fields that rely on defaults
without being explicitly set. Without type= in InputToolMCP and
InputToolFileSearch constructors, the discriminator field is omitted and
downstream deserialization of OpenAIResponseInputTool fails with
ValueError.

Signed-off-by: Major Hayden <major@redhat.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 3, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e07897c and 7de3c9c.

📒 Files selected for processing (1)
  • src/utils/responses.py

Walkthrough

This change adds explicit type parameters to tool construction in src/utils/responses.py. The InputToolFileSearch now includes type="file_search" and InputToolMCP now includes type="mcp" when instantiated. No control flow or error handling changes.

Changes

Cohort / File(s) Summary
Tool Type Parameters
src/utils/responses.py
Added explicit type parameter to InputToolFileSearch construction (type="file_search") and InputToolMCP construction (type="mcp") in get_rag_tools and get_mcp_tools respectively.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • tisnik
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: adding explicit type discriminators to tool constructors, which directly addresses the bug fix documented in the PR objectives.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tisnik tisnik merged commit 7954c6b into lightspeed-core:main Mar 3, 2026
20 of 21 checks passed
@major major deleted the RSPEED-2523-mcp-tool-type-discriminator branch March 3, 2026 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants