LCORE-1880: Refactor of responses models dumping#1645
Conversation
|
Caution Review failedFailed to post review comments WalkthroughThe PR consolidates MCP and OpenAI tool type definitions into ChangesMCP Tool Type Consolidation and Authorization Preservation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
asimurka
requested review from
max-svistunov and
tisnik
and removed request for
tisnik
asimurka
force-pushed
the
refactor_responses_endpoint
branch
from
0b87781 to
140ebec
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/models/api/requests/responses_openai.py`:
- Around line 181-190: The current model_dump override injects MCP
authorizations into every serialization; change it to require an explicit opt-in
flag (e.g., preserve_mcp_authorization: bool = False) passed to model_dump so
defaults remain redacted: keep the existing logic that calls
add_mcp_authorizations(result["tools"], self.tools) but only execute it when
preserve_mcp_authorization is True, and ensure the method signature of
model_dump(...) accepts and forwards unknown kwargs to super().model_dump while
removing the opt-in flag before calling super; reference the model_dump method
and add_mcp_authorizations helper and the "tools" field when making this change.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0ffe5706-63a4-4248-8906-37046962b900
📒 Files selected for processing (7)
src/app/endpoints/responses.pysrc/models/api/requests/responses_openai.pysrc/models/common/responses/responses_api_params.pysrc/models/utils.pytests/unit/app/endpoints/test_responses.pytests/unit/models/test_utils.pytests/unit/utils/test_types.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/models/api/requests/responses_openai.pysrc/models/utils.pysrc/app/endpoints/responses.pysrc/models/common/responses/responses_api_params.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/api/requests/responses_openai.pysrc/models/utils.pysrc/models/common/responses/responses_api_params.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/app/**/*.py: FastAPI dependencies: Import fromfastapimodule forAPIRouter,HTTPException,Request,status,Depends
Use FastAPIHTTPExceptionwith appropriate status codes for API endpoints and handleAPIConnectionErrorfrom Llama Stack
Files:
src/app/endpoints/responses.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/utils/test_types.pytests/unit/app/endpoints/test_responses.pytests/unit/models/test_utils.py
🧠 Learnings (3)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/api/requests/responses_openai.pysrc/models/utils.pysrc/models/common/responses/responses_api_params.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/api/requests/responses_openai.pysrc/models/utils.pysrc/models/common/responses/responses_api_params.py
📚 Learning: 2026-04-06T20:18:07.852Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1463
File: src/app/endpoints/rlsapi_v1.py:266-271
Timestamp: 2026-04-06T20:18:07.852Z
Learning: In the lightspeed-stack codebase, within `src/app/endpoints/` inference/MCP endpoints, treat `tools: Optional[list[Any]]` in MCP tool definitions as an intentional, consistent typing pattern (used across `query`, `responses`, `streaming_query`, `rlsapi_v1`). Do not raise or suggest this as a typing issue during code review; changing it in isolation could break endpoint typing consistency across the codebase.
Applied to files:
src/app/endpoints/responses.py
asimurka
force-pushed
the
refactor_responses_endpoint
branch
2 times, most recently
from
a57952c to
fb13413
Compare
| for tool, dumped_tool in zip(self.tools, dumped_tools): | ||
| authorization = getattr(tool, "authorization", None) | ||
| if authorization is not None and isinstance(dumped_tool, dict): | ||
| dumped_tool["authorization"] = authorization |
There was a problem hiding this comment.
Not necessary as we override authorization attribute locally so that it is not dropped anymore.
| class InputToolMCP(OpenAIResponseInputToolMCP): | ||
| """MCP input tool with authorization included when serializing request bodies.""" | ||
|
|
||
| authorization: str | None = None |
There was a problem hiding this comment.
Override locally so that the attribute is not dropped when model is dumped
anik120
left a comment
anik120
left a comment
There was a problem hiding this comment.
I'm guessing the breaking change of OpenAPI schema rename from OpenAPIResponseInputToolMCP -> InputToolMCP only matters internally so it won't be a problem.
eg if our users have some validation against OpenAPI schema, we'll break those validation - but I'm guessing that's not a concern for right now :)
Barring that concern ☝🏽 , lgtm
that is caused by creating internal type union but the schema remains the same, so it should not cause any problems |
asimurka
force-pushed
the
refactor_responses_endpoint
branch
from
fb13413 to
37fb2f3
Compare
2 participants
Description
Removes temporary workaround for preservation of MCP tool authorization attribute.
Redefines the InputMCPTool type with overridden authorization attribute.
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
Bug Fixes
Documentation
InputToolMCPinput tool configuration for MCP tools.Tests