LCORE-1583: Allowed passthrough attributes in responses#1483
LCORE-1583: Allowed passthrough attributes in responses#1483asimurka wants to merge 1 commit intolightspeed-core:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
📜 Recent 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). (14)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/app/endpoints/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-04-16T19:08:31.451ZApplied to files:
📚 Learning: 2026-04-15T18:53:57.901ZApplied to files:
📚 Learning: 2026-04-06T20:18:07.852ZApplied to files:
🔇 Additional comments (1)
WalkthroughThe handler for the responses endpoint no longer special-cases Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
force-pushed
the
allow_responses_attributes
branch
from
7cd8248 to
e89b907
Compare
PR Reviewer Guide 🔍(Review updated until commit e89b907)Here are some key observations to aid the review process:
|
|
Persistent review updated to latest commit e89b907 |
|
Persistent review updated to latest commit e89b907 |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@lightspeed-stack.yaml`:
- Around line 13-15: The YAML contains a duplicated key use_as_library_client
with conflicting values (false and true); remove one of the entries so the map
contains a single use_as_library_client key with the intended boolean value,
ensuring the remaining value reflects the desired behavior and resolving the
parser/lint error.
In `@src/app/endpoints/responses.py`:
- Around line 165-170: The code currently nulls out responses_request.reasoning
(responses_request.reasoning = None) and mutates the input before copying,
preventing the field from reaching ResponsesApiParams.model_validate(...) and
client.responses.create(...); remove the in-place assignment so reasoning is
preserved, log a warning if present but do not modify responses_request, and
ensure you perform the deep copy (responses_request =
responses_request.model_copy(deep=True)) before any non-mutating checks or logs
so the original object is not altered and the reasoning value is passed through
to downstream validation and client.responses.create.
🪄 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: 8abc4275-a00c-4ee0-8518-6e7140b44683
📒 Files selected for processing (6)
lightspeed-stack.yamlrun.yamlsrc/app/endpoints/responses.pysrc/utils/types.pytests/e2e/features/responses.featuretests/e2e/test_list.txt
💤 Files with no reviewable changes (3)
- run.yaml
- tests/e2e/test_list.txt
- tests/e2e/features/responses.feature
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules (e.g.,from authentication import get_auth_dependency)
Usefrom llama_stack_client import AsyncLlamaStackClientfor Llama Stack imports
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Use complete type annotations for function parameters and return types
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional types in type annotations
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 parameters
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use complete type annotations for all class attributes; avoid usingAny
Follow Google Python docstring conventions for all modules, classes, and functions
IncludeParameters:,Returns:,Raises:sections in function docstrings as needed
Files:
src/app/endpoints/responses.pysrc/utils/types.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/app/**/*.py: Usefrom fastapi import APIRouter, HTTPException, Request, status, Dependsfor FastAPI dependencies
Use FastAPIHTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/responses.py
🧠 Learnings (3)
📚 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
📚 Learning: 2025-12-18T10:21:09.038Z
Learnt from: are-ces
Repo: lightspeed-core/lightspeed-stack PR: 935
File: run.yaml:114-115
Timestamp: 2025-12-18T10:21:09.038Z
Learning: In Llama Stack version 0.3.x, telemetry provider configuration is not supported under the `providers` section in run.yaml configuration files. Telemetry can be enabled with just `telemetry.enabled: true` without requiring an explicit provider block.
Applied to files:
lightspeed-stack.yaml
📚 Learning: 2026-04-05T12:19:36.009Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-05T12:19:36.009Z
Learning: Applies to src/**/*.py : Use `from llama_stack_client import AsyncLlamaStackClient` for Llama Stack imports
Applied to files:
lightspeed-stack.yaml
🪛 YAMLlint (1.38.0)
lightspeed-stack.yaml
[error] 15-15: duplication of key "use_as_library_client" in mapping
(key-duplicates)
🔇 Additional comments (2)
lightspeed-stack.yaml (1)
16-18: Library mode wiring looks correct once the duplicate key is removed.
library_client_config_path: run.yamlplus disabled URL/API key matches the intended library-client startup path.src/utils/types.py (1)
217-220: Good contract alignment forsafety_identifierpassthrough.This field addition cleanly matches the request/response models and allows forwarding without special-case logic.
asimurka
force-pushed
the
allow_responses_attributes
branch
2 times, most recently
from
3d52579 to
021e671
Compare
| "model": "{PROVIDER}/{MODEL}", | ||
| "instructions": "You are a helpful assistant.", | ||
| "prompt": {"id": "e2e_responses_passthrough_prompt"}, | ||
| "reasoning": {"effort": "low"}, |
There was a problem hiding this comment.
reasoning validation is still broken in 0.6.0 and moreover it will most likely need additional logic - based on reasoning capabilities of configured model. So it is not a passthrough attribute as well.
| "status": "completed", | ||
| "model": "{PROVIDER}/{MODEL}", | ||
| "instructions": "You are a helpful assistant.", | ||
| "prompt": {"id": "e2e_responses_passthrough_prompt"}, |
There was a problem hiding this comment.
prompt is not really a "passthrough" parameter. It relies on the underlying Prompts API.
This will be tested in a separate feature file.
https://redhat.atlassian.net/browse/LCORE-1956
asimurka
changed the title
asimurka
force-pushed
the
allow_responses_attributes
branch
from
021e671 to
7422fa5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/app/endpoints/responses.py (1)
237-243:⚠️ Potential issue | 🔴 Critical
reasoningis still being stripped — contradicts the PR intent.The PR title/commit message says reasoning should be passed through, but lines 238-240 still log a warning and set
responses_request.reasoning = None, preventing the field from reachingResponsesApiParams.model_validate(...)andclient.responses.create(...). Additionally, this mutates the input before themodel_copy(deep=True)on line 242. Remove the nullification (and move any logging after the deep copy) so reasoning is actually forwarded.✅ Suggested fix
- # Known LLS bug: https://redhat.atlassian.net/browse/LCORE-1583 - if responses_request.reasoning is not None: - logger.warning("reasoning is not yet supported in LCORE and will be ignored") - responses_request.reasoning = None - - responses_request = responses_request.model_copy(deep=True) + responses_request = responses_request.model_copy(deep=True)As per coding guidelines: "Avoid in-place parameter modification anti-patterns; return new data structures instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/responses.py` around lines 237 - 243, The code currently nullifies responses_request.reasoning before making a deep copy, preventing the reasoning field from reaching ResponsesApiParams.model_validate(...) and client.responses.create(...); remove the in-place assignment (responses_request.reasoning = None) and instead perform responses_request = responses_request.model_copy(deep=True) first, then if you still need to warn preserve the original value (or check the copied object's reasoning) and call logger.warning(...) after the deep copy so the reasoning field is forwarded unchanged to ResponsesApiParams.model_validate and client.responses.create.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app/endpoints/responses.py`:
- Around line 237-243: The code currently nullifies responses_request.reasoning
before making a deep copy, preventing the reasoning field from reaching
ResponsesApiParams.model_validate(...) and client.responses.create(...); remove
the in-place assignment (responses_request.reasoning = None) and instead perform
responses_request = responses_request.model_copy(deep=True) first, then if you
still need to warn preserve the original value (or check the copied object's
reasoning) and call logger.warning(...) after the deep copy so the reasoning
field is forwarded unchanged to ResponsesApiParams.model_validate and
client.responses.create.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6672ed14-0dcc-46f1-a732-70ecec27dee5
📒 Files selected for processing (2)
src/app/endpoints/responses.pytests/e2e/features/responses.feature
💤 Files with no reviewable changes (1)
- tests/e2e/features/responses.feature
📜 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). (10)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: unit_tests (3.13)
- GitHub Check: mypy
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 2
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Import FastAPI dependencies with:from fastapi import APIRouter, HTTPException, Request, status, Depends
Import Llama Stack client with:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
Type aliases defined at module level for clarity
Use Final[type] as type hint for all constants
All functions require docstrings with brief descriptions
Complete type annotations for parameters and return types in functions
Usetyping_extensions.Selffor model validators in Pydantic models
Use modern union type syntaxstr | intinstead ofUnion[str, int]
UseOptional[Type]for optional type hints
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Use standard log levels with clear purposes: debug, info, warning, error
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes: Configuration, Error/Exception, Resolver, Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Use@model_validatorand@field_validatorfor Pydantic model validation
Complete type annotations for all class attributes; use specific types, notAny
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections
Files:
src/app/endpoints/responses.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoints
Files:
src/app/endpoints/responses.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models extend
ConfigurationBasefor config,BaseModelfor data models
Files:
src/app/endpoints/responses.py
🧠 Learnings (3)
📚 Learning: 2026-04-16T19:08:31.451Z
Learnt from: Lifto
Repo: lightspeed-core/lightspeed-stack PR: 1524
File: src/app/endpoints/responses.py:501-529
Timestamp: 2026-04-16T19:08:31.451Z
Learning: In `src/app/endpoints/responses.py`, `_sanitize_response_dict(response_dict, configured_mcp_labels)` intentionally mutates `response_dict` in-place. The dict is always a fresh throwaway produced by `model_dump()` on the immediately preceding line (both in the streaming `async for` loop and in the non-streaming path); no other reference to it exists. The AGENTS.md guideline "avoid in-place parameter modification" applies to mutating a caller's long-lived/shared data structures, not to ephemeral serialization dicts. Do not flag this as an anti-pattern in future reviews.
Applied to files:
src/app/endpoints/responses.py
📚 Learning: 2026-04-15T18:53:57.901Z
Learnt from: Lifto
Repo: lightspeed-core/lightspeed-stack PR: 1510
File: src/models/requests.py:742-769
Timestamp: 2026-04-15T18:53:57.901Z
Learning: In lightspeed-core/lightspeed-stack `src/models/requests.py`, the `ResponsesRequest.validate_body_size` validator uses `json.dumps(values)` with default parameters (ASCII escaping, spaced separators) intentionally. The slight overcount (~0.2% overhead) makes the 65,536-character limit conservatively strict, which is the desired safe direction. An exact wire-format match is impossible since Python's json.dumps output is never byte-identical to the client payload. Do not suggest switching to `ensure_ascii=False` or compact separators for this validator.
Applied to files:
src/app/endpoints/responses.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
allow_responses_attributes
branch
from
7422fa5 to
688708b
Compare
2 participants
Description
Allows passthrough attribute
max_output_tokenswhich validation was recently fixed on upstream.Removes
@skiptag from e2e test that is testing passthrough attributes.Prompt attribute is removed as it relies on Prompts API (will be tested separately)
Reasoning attribute has bug on upstream and will be tracked by separate ticket.
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
Summary by CodeRabbit
Bug Fixes
Tests