LCORE-1633 Apply server-side system prompt resolution to /responses endpoint#1446
LCORE-1633 Apply server-side system prompt resolution to /responses endpoint#1446r0x0d wants to merge 4 commits intolightspeed-core:mainfrom
Conversation
The /responses endpoint previously only used client-provided instructions, with no server-side fallback. This aligns it with /query by reusing get_system_prompt() to resolve instructions from config when omitted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 (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). (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/unit/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-08-25T09:05:18.350ZApplied to files:
🔇 Additional comments (5)
WalkthroughThe responses endpoint now resolves and overwrites Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as responses_endpoint_handler
participant PromptResolver as get_system_prompt
participant NonStream as handle_non_streaming_response
participant Stream as handle_streaming_response
Client->>Handler: POST ResponsesRequest (may include instructions, stream flag)
Handler->>Handler: Deep copy request
Handler->>PromptResolver: get_system_prompt(request.instructions, field_name="instructions")
alt disable_query_system_prompt && instructions provided
PromptResolver-->>Handler: raise HTTPException(422) with cause referencing "instructions"
Handler->>Client: 422 Error
else
PromptResolver-->>Handler: resolved_instructions
Handler->>Handler: overwrite request.instructions = resolved_instructions
alt stream == true
Handler->>Stream: handle_streaming_response(request)
Stream-->>Client: streaming responses
else
Handler->>NonStream: handle_non_streaming_response(request)
NonStream-->>Client: final response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/prompts.py (1)
35-38: 🧹 Nitpick | 🔵 TrivialDocstring is now inconsistent with the error message.
The docstring states the error "instructs the client to remove the system_prompt field" but the actual error message now mentions "system_prompt or instructions". Update the docstring to match.
Raises: ------ HTTPException: 422 Unprocessable Entity when per-request system prompts are disabled (disable_query_system_prompt) and a non-None - `system_prompt` is provided; the response instructs the client to - remove the system_prompt field from the request. + `system_prompt` is provided; the response instructs the client to + remove the system_prompt or instructions field from the request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/prompts.py` around lines 35 - 38, The docstring describing the 422 error is out of sync with the runtime error message; update the docstring in src/utils/prompts.py so the description matches the current error text (mentioning "system_prompt or instructions" instead of only "system_prompt")—locate the docstring block that explains "HTTPException: 422 Unprocessable Entity when per-request system prompts..." and edit the sentence to reference "system_prompt or instructions" exactly as the error message does so the docs and error text remain consistent.
🤖 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/responses.py`:
- Around line 173-175: Reorder the configuration check so it runs before
resolving the system prompt: call check_configuration_loaded(configuration)
immediately after creating the deep copy of responses_request (after
responses_request = responses_request.model_copy(deep=True)) and before calling
responses_request.instructions =
get_system_prompt(responses_request.instructions) so that get_system_prompt()
never runs without configuration loaded; update any related logic in the same
block that assigns responses_request.instructions accordingly.
In `@src/utils/prompts.py`:
- Around line 48-51: Update get_system_prompt in src/utils/prompts.py to accept
an optional field_name parameter (default "system_prompt") and use that
field_name in the error message instead of hardcoding "system_prompt or
instructions"; then update the call sites to pass the correct name — e.g., in
responses.py call get_system_prompt(responses_request.instructions,
field_name="instructions") and in the query handler call
get_system_prompt(query_request.system_prompt, field_name="system_prompt") — so
the thrown message references the correct request field for
QueryRequest/ResponsesRequest.
In `@tests/unit/app/endpoints/test_responses.py`:
- Around line 1606-1610: The test currently hardcodes the default prompt ("You
are a helpful assistant") when asserting call_kwargs["request"].instructions;
change the assertion to reference the canonical default prompt constant used by
the request model (e.g., Request.DEFAULT_INSTRUCTIONS or the module-level
DEFAULT_INSTRUCTIONS/DEFAULT_SYSTEM_PROMPT) so the test compares against the
source-of-truth default instead of a literal string (mirror the same change
applied at the earlier fix around line 1422).
- Around line 1419-1422: Replace the hardcoded string in the assertion with the
DEFAULT_SYSTEM_PROMPT constant: import DEFAULT_SYSTEM_PROMPT at the top of the
test module and change the expectation from "You are a helpful assistant" to
DEFAULT_SYSTEM_PROMPT when asserting call_kwargs["request"].instructions (the
mock_handler/call_args produced by handle_non_streaming_response). Ensure the
import references the module that exports DEFAULT_SYSTEM_PROMPT and update the
assertion to compare against that constant.
---
Outside diff comments:
In `@src/utils/prompts.py`:
- Around line 35-38: The docstring describing the 422 error is out of sync with
the runtime error message; update the docstring in src/utils/prompts.py so the
description matches the current error text (mentioning "system_prompt or
instructions" instead of only "system_prompt")—locate the docstring block that
explains "HTTPException: 422 Unprocessable Entity when per-request system
prompts..." and edit the sentence to reference "system_prompt or instructions"
exactly as the error message does so the docs and error text remain consistent.
🪄 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: 9c31cd24-8261-40f5-b84c-b80a8cdd3a92
📒 Files selected for processing (3)
src/app/endpoints/responses.pysrc/utils/prompts.pytests/unit/app/endpoints/test_responses.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). (3)
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use absolute imports for internal modules from the Lightspeed Core Stack (e.g.,
from authentication import get_auth_dependency)
Files:
src/app/endpoints/responses.pysrc/utils/prompts.pytests/unit/app/endpoints/test_responses.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use FastAPI imports from the fastapi module:from fastapi import APIRouter, HTTPException, Request, status, Depends
Usefrom llama_stack_client import AsyncLlamaStackClientfor Llama Stack client imports
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
Define type aliases at module level for clarity in Lightspeed Core Stack
All functions require docstrings with brief descriptions following Google Python docstring conventions
Use complete type annotations for function parameters and return types
Use modern union type syntaxstr | intinstead ofUnion[str, int]for type annotations
UseOptional[Type]for nullable types in type annotations
Use snake_case with descriptive, action-oriented names for functions (e.g., get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor functions performing I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling logic
Use standard log levels appropriately: debug for diagnostic info, info for general execution, warning for unexpected events, error for serious problems
All classes require descriptive docstrings explaining their purpose
Use PascalCase for class names with descriptive names and standard suffixes (Configuration, Error/Exception, Resolver, Interface)
Use ABC (Abstract Base Classes) with@abstractmethoddecorators for interface implementations
Use complete type annotations for all class attributes; avoid usingAnytype
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections as needed
Files:
src/app/endpoints/responses.pysrc/utils/prompts.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
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
Maintain test coverage of at least 60% for unit tests and 10% for integration tests
Files:
tests/unit/app/endpoints/test_responses.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
pytest-mockfor AsyncMock objects in unit tests
Files:
tests/unit/app/endpoints/test_responses.py
🔇 Additional comments (2)
src/app/endpoints/responses.py (1)
60-60: LGTM!The import follows the established pattern for utility module imports in this file.
tests/unit/app/endpoints/test_responses.py (1)
1377-1378: LGTM!Good test class organization with clear docstring explaining the purpose. The test coverage for instruction resolution is comprehensive, covering:
- Default fallback when client omits instructions
- Client-provided instructions passthrough
- Configuration-based system prompt
- Rejection when disabled
- Streaming path consistency
- Reorder check_configuration_loaded before get_system_prompt call - Add field_name parameter to get_system_prompt for context-specific error messages per endpoint - Update docstring to match the new error message - Use DEFAULT_SYSTEM_PROMPT constant in tests instead of hardcoded string Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
r0x0d
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/app/endpoints/test_responses.py`:
- Around line 1563-1571: The test only checks the status code for the raised
HTTPException from responses_endpoint_handler but must also assert the error
payload contains the field-specific message for field_name="instructions";
update the test after the await call to inspect exc_info.value.detail (or the
exception body) and assert the presence of the expected 'cause' text (or exact
detail structure) referencing responses_request/dummy_request inputs, keeping
MOCK_AUTH usage the same so the test fails if the instructions-specific
validation message changes.
🪄 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: 46689c97-0ea7-4f76-9f02-512f9015375e
📒 Files selected for processing (3)
src/app/endpoints/responses.pysrc/utils/prompts.pytests/unit/app/endpoints/test_responses.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). (2)
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use absolute imports for internal modules from the Lightspeed Core Stack (e.g.,
from authentication import get_auth_dependency)
Files:
src/app/endpoints/responses.pysrc/utils/prompts.pytests/unit/app/endpoints/test_responses.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use FastAPI imports from the fastapi module:from fastapi import APIRouter, HTTPException, Request, status, Depends
Usefrom llama_stack_client import AsyncLlamaStackClientfor Llama Stack client imports
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
Define type aliases at module level for clarity in Lightspeed Core Stack
All functions require docstrings with brief descriptions following Google Python docstring conventions
Use complete type annotations for function parameters and return types
Use modern union type syntaxstr | intinstead ofUnion[str, int]for type annotations
UseOptional[Type]for nullable types in type annotations
Use snake_case with descriptive, action-oriented names for functions (e.g., get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor functions performing I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling logic
Use standard log levels appropriately: debug for diagnostic info, info for general execution, warning for unexpected events, error for serious problems
All classes require descriptive docstrings explaining their purpose
Use PascalCase for class names with descriptive names and standard suffixes (Configuration, Error/Exception, Resolver, Interface)
Use ABC (Abstract Base Classes) with@abstractmethoddecorators for interface implementations
Use complete type annotations for all class attributes; avoid usingAnytype
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections as needed
Files:
src/app/endpoints/responses.pysrc/utils/prompts.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
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
Maintain test coverage of at least 60% for unit tests and 10% for integration tests
Files:
tests/unit/app/endpoints/test_responses.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
pytest-mockfor AsyncMock objects in unit tests
Files:
tests/unit/app/endpoints/test_responses.py
🔇 Additional comments (2)
src/app/endpoints/responses.py (1)
173-177: Good fix: configuration check precedes system prompt resolution, and field-specific mapping is applied.This correctly aligns
/responsesbehavior with server-side prompt fallback while preserving endpoint-specific error messaging viafield_name="instructions".src/utils/prompts.py (1)
12-14: Field-aware error messaging update is correct and well-scoped.The new
field_nameparameter and dynamic 422 message eliminate endpoint confusion without changing prompt-precedence behavior.Also applies to: 51-53
Verify the error detail contains "instructions field" to catch regressions in the field-specific messaging from get_system_prompt. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/app/endpoints/test_responses.py`:
- Around line 1392-1404: The instruction-resolution tests patch
app.endpoints.responses.configuration via _patch_base(...) but the resolution
code reads utils.prompts.configuration, causing global-state coupling; update
each affected test (those using _patch_resolve_response_context/_patch_base like
the block that mocks select_model_for_responses, check_model_configured, etc.)
to also patch utils.prompts.configuration (e.g., with
mocker.patch("utils.prompts.configuration", new=desired_config) or a helper that
sets both) so the tests control both config references and stop being flaky.
🪄 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: 9da0d1e9-116f-4fa1-9621-73c90d1c72da
📒 Files selected for processing (1)
tests/unit/app/endpoints/test_responses.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). (2)
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use absolute imports for internal modules from the Lightspeed Core Stack (e.g.,
from authentication import get_auth_dependency)
Files:
tests/unit/app/endpoints/test_responses.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Maintain test coverage of at least 60% for unit tests and 10% for integration tests
Files:
tests/unit/app/endpoints/test_responses.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
pytest-mockfor AsyncMock objects in unit tests
Files:
tests/unit/app/endpoints/test_responses.py
🧠 Learnings (1)
📚 Learning: 2025-08-25T09:05:18.350Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 445
File: tests/e2e/features/health.feature:50-53
Timestamp: 2025-08-25T09:05:18.350Z
Learning: The step definition for ignoring specific fields in response body comparisons is implemented in tests/e2e/features/steps/common_http.py at line 175 as `then('The body of the response, ignoring the "{field}" field, is the following')`.
Applied to files:
tests/unit/app/endpoints/test_responses.py
🔇 Additional comments (1)
tests/unit/app/endpoints/test_responses.py (1)
21-21: Nice improvement: tests now assert againstDEFAULT_SYSTEM_PROMPTinstead of hardcoded text.This makes the expectations resilient to future default prompt updates and keeps tests aligned with the source of truth.
Also applies to: 1422-1424, 1612-1613
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
asimurka
left a comment
asimurka
left a comment
There was a problem hiding this comment.
LGTM in overall. Please document this LCORE-specific behavior in docs/responses.md. More specifically, add a short subsection under Behavioral Differences.
…test patching Add System Prompt Resolution subsection to docs/responses.md describing how the instructions field is resolved server-side. Patch utils.prompts.configuration in instruction-resolution tests to avoid global-state coupling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/ok-to-test |
3 participants
Description
The /responses endpoint previously only used client-provided instructions, with no server-side fallback. This aligns it with /query by reusing get_system_prompt() to resolve instructions from config when omitted.
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
New Features
Improvements
Tests
Documentation