Skip to content

LCORE-1880: Refactor of responses models dumping#1645

Open
asimurka wants to merge 1 commit intolightspeed-core:mainfrom
asimurka:refactor_responses_endpoint
Open

LCORE-1880: Refactor of responses models dumping#1645
asimurka wants to merge 1 commit intolightspeed-core:mainfrom
asimurka:refactor_responses_endpoint

Conversation

@asimurka
Copy link
Copy Markdown
Contributor

@asimurka asimurka commented Apr 30, 2026

Description

Removes temporary workaround for preservation of MCP tool authorization attribute.
Refines existing dumping logic that returns MCP authorization that is excluded by default.
Generalizes logic to work correctly even if dumping tools dropped some tools.

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: Cursor

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

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced how authentication credentials for MCP (Model Context Protocol) tools are handled and preserved during API request processing, ensuring consistent behavior across the request lifecycle.
  • Tests

    • Added comprehensive regression tests to verify MCP tool authentication data is correctly maintained, including scenarios with multiple servers and partial tool exclusion.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 30, 2026

Review Change Stack

Walkthrough

This PR refactors MCP authorization restoration by extracting shared logic into a utility function and consolidating its use across model serialization methods. The changes simplify endpoint and model code while ensuring authorization tokens are correctly preserved through serialization and validation cycles.

Changes

MCP Authorization Centralization

Layer / File(s) Summary
MCP Authorization Utility
src/models/utils.py
New add_mcp_authorizations(dumped_tools, tools) function maps live tool server_label values to their authorization tokens and applies them to cloned serialized tool dictionaries.
Model Serialization Methods
src/models/api/requests/responses_openai.py, src/models/common/responses/responses_api_params.py
ResponsesRequest.model_dump() and ResponsesApiParams.model_dump() now delegate MCP authorization restoration to the shared utility, replacing previous inline validation and assignment logic.
Endpoint Handler
src/app/endpoints/responses.py
Simplified to validate ResponsesApiParams directly from the full updated_request.model_dump() output, relying on model methods for authorization handling.
Utility Unit Tests
tests/unit/models/test_utils.py
New test module validates add_mcp_authorizations with scenarios: single and multiple MCP servers, mixed tool types, partial dumps, and immutability guarantees.
Integration Tests
tests/unit/app/endpoints/test_responses.py, tests/unit/utils/test_types.py
Updated endpoint helper and tests verify authorization preservation through serialization and validation; reworked model_dump() exclusion test to confirm reinjection by server_label.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main refactoring change: restructuring how responses models handle dumping of serialized data, particularly for MCP authorization preservation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified 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.

❤️ Share

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

@asimurka asimurka requested review from max-svistunov and tisnik and removed request for tisnik April 30, 2026 14:22
@asimurka asimurka force-pushed the refactor_responses_endpoint branch from 0b87781 to 140ebec Compare May 7, 2026 12:24
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 0b87781 and 140ebec.

📒 Files selected for processing (7)
  • src/app/endpoints/responses.py
  • src/models/api/requests/responses_openai.py
  • src/models/common/responses/responses_api_params.py
  • src/models/utils.py
  • tests/unit/app/endpoints/test_responses.py
  • tests/unit/models/test_utils.py
  • tests/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: Use from llama_stack_client import AsyncLlamaStackClient
Check constants.py for shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Use logger = get_logger(__name__) from log.py for 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
Use async def for 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 @abstractmethod decorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes

Files:

  • src/models/api/requests/responses_openai.py
  • src/models/utils.py
  • src/app/endpoints/responses.py
  • src/models/common/responses/responses_api_params.py
src/models/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Pydantic models must use @model_validator and @field_validator for validation and complete type annotations for all attributes, avoiding Any type

Files:

  • src/models/api/requests/responses_openai.py
  • src/models/utils.py
  • src/models/common/responses/responses_api_params.py
src/app/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

src/app/**/*.py: FastAPI dependencies: Import from fastapi module for APIRouter, HTTPException, Request, status, Depends
Use FastAPI HTTPException with appropriate status codes for API endpoints and handle APIConnectionError from 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
Use pytest.mark.asyncio marker for async tests

Files:

  • tests/unit/utils/test_types.py
  • tests/unit/app/endpoints/test_responses.py
  • tests/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.py
  • src/models/utils.py
  • src/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.py
  • src/models/utils.py
  • src/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

Comment thread src/models/api/requests/responses_openai.py
@asimurka asimurka marked this pull request as draft May 7, 2026 13:02
@asimurka asimurka marked this pull request as ready for review May 7, 2026 14:18
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.

1 participant