Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -196,4 +196,7 @@ dev-tools/mcp-mock-server/.certs/
requirements.*.backup

# Local run files
local-run.yaml
local-run.yaml

# Sisyphus planning files
.sisyphus/
2 changes: 1 addition & 1 deletion docs/responses.md
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ The `instructions` field on the `/v1/responses` endpoint follows the same server

If the server configuration sets `disable_query_system_prompt` to `true`, any request that includes a non-null `instructions` value is rejected with a `422 Unprocessable Entity` error. The error message references the `instructions` field specifically.

This means that even when `instructions` is omitted from the request, the response will always contain a resolved `instructions` value reflecting the server-side default or configured system prompt.
If the server substituted the system prompt, the response sets `instructions` to the placeholder `<server prompt applied>` instead of echoing the actual prompt. If the client provided their own `instructions`, they are echoed back unchanged. Server-deployed MCP tool definitions are also filtered from the response `tools` array; client-provided tools are preserved.

### Streaming Differences

Expand Down
63 changes: 60 additions & 3 deletions src/app/endpoints/responses.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
from authorization.middleware import authorize
from client import AsyncLlamaStackClientHolder
from configuration import configuration
from constants import SUBSTITUTED_INSTRUCTIONS_PLACEHOLDER
from log import get_logger
from models.config import Action
from models.requests import ResponsesRequest
Expand Down Expand Up @@ -179,9 +180,11 @@ async def responses_endpoint_handler(

responses_request = responses_request.model_copy(deep=True)
check_configuration_loaded(configuration)
client_instructions = responses_request.instructions
responses_request.instructions = get_system_prompt(
responses_request.instructions, field_name="instructions"
)
instructions_substituted = client_instructions is None
started_at = datetime.now(UTC)
user_id = auth[0]

Expand Down Expand Up @@ -302,6 +305,7 @@ async def responses_endpoint_handler(
moderation_result=moderation_result,
inline_rag_context=inline_rag_context,
filter_server_tools=filter_server_tools,
instructions_substituted=instructions_substituted,
)


Expand All @@ -314,6 +318,7 @@ async def handle_streaming_response(
moderation_result: ShieldModerationResult,
inline_rag_context: RAGContext,
filter_server_tools: bool = False,
instructions_substituted: bool = False,
) -> StreamingResponse:
"""Handle streaming response from Responses API.

Expand All @@ -326,6 +331,7 @@ async def handle_streaming_response(
moderation_result: Result of shield moderation check
inline_rag_context: Inline RAG context to be used for the response
filter_server_tools: Whether to filter server-deployed MCP tool events from the stream
instructions_substituted: Whether the server substituted the instructions
Returns:
StreamingResponse with SSE-formatted events
"""
Expand Down Expand Up @@ -365,6 +371,7 @@ async def handle_streaming_response(
turn_summary=turn_summary,
inline_rag_context=inline_rag_context,
filter_server_tools=filter_server_tools,
instructions_substituted=instructions_substituted,
)
except RuntimeError as e: # library mode wraps 413 into runtime error
if is_context_length_error(str(e)):
Expand Down Expand Up @@ -496,6 +503,45 @@ async def shield_violation_generator(
yield "data: [DONE]\n\n"


def _sanitize_response_dict(
response_dict: dict[str, Any],
configured_mcp_labels: set[str],
instructions_substituted: bool = False,
) -> None:
"""Sanitize a serialized response object in-place to remove internal details.

Strips fields that expose server-side implementation details from the
response object before it is forwarded to the client:

- ``instructions``: when the server substituted its own system prompt
(because the client sent ``None`` or a different value was resolved),
the value is replaced with a placeholder slug to avoid leaking the
actual prompt. When the client provided their own instructions and
they were used as-is, the value is left unchanged.
- ``tools``: server-deployed MCP tool definitions are removed; client-
provided tools (those whose ``server_label`` is not in
``configured_mcp_labels``) are preserved

Args:
response_dict: Mutable dict produced by ``model_dump`` on a response
object. Modified in-place.
configured_mcp_labels: Set of ``server_label`` values that identify
server-deployed MCP servers.
instructions_substituted: Whether the server substituted the
instructions (True) or the client provided them (False).
"""
if instructions_substituted:
response_dict["instructions"] = SUBSTITUTED_INSTRUCTIONS_PLACEHOLDER
# else: leave instructions as-is (echo back client's value)

if tools := response_dict.get("tools"):
response_dict["tools"] = [
tool
for tool in tools
if tool.get("server_label") not in configured_mcp_labels
]
Comment on lines +506 to +542

@coderabbitai coderabbitai Bot Apr 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Return a sanitized copy instead of mutating the input dict.

Making this helper pure will avoid hidden caller side effects and make the two response paths easier to reason about.

As per coding guidelines "Avoid in-place parameter modification anti-patterns: return new data structures instead of modifying parameters".

🤖 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 501 - 537, The helper
_sanitize_response_dict currently mutates the input response_dict; change it to
be pure by returning a sanitized copy instead: create a shallow copy of
response_dict at the start (e.g., new = dict(response_dict)), remove
"instructions" from the copy, transform the "model" value on the copy if it's a
string with more than one "/", and build a new "tools" list by filtering and
copying each tool dict so callers are not mutated; update the function signature
to return dict[str, Any] and adjust all callers of _sanitize_response_dict to
use the returned sanitized dict instead of relying on in-place modification.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In-place mutation is intentional here. The dict is created by model_dump() on the line immediately before the call — it is a throwaway serialization dict, not a shared data structure. Returning a copy would allocate a new dict for every SSE chunk with zero safety benefit, since nothing else holds a reference to the original.

The AGENTS.md guideline about avoiding in-place modification targets a different anti-pattern: functions that silently mutate a caller's long-lived data structure via a parameter. That does not apply here — the dict is born, sanitized, serialized to JSON, and discarded within the same async for iteration.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Lifto, that's a well-reasoned clarification — you're right. The guideline is aimed at functions that silently mutate a caller's long-lived/shared data structure through a parameter reference, which is a genuine source of bugs. Here the dict is born from model_dump(), passed straight into _sanitize_response_dict, serialized, and discarded — there's no aliasing risk and no wasted allocation. The in-place mutation is appropriate and intentional. I'll withdraw the suggestion.

(ᵔᴥᵔ) 🐇✏️


✏️ Learnings added
Learnt from: Lifto
Repo: lightspeed-core/lightspeed-stack PR: 1524
File: src/app/endpoints/responses.py:501-529
Timestamp: 2026-04-16T19:08:26.366Z
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.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
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 : Avoid in-place parameter modification anti-patterns: return new data structures instead of modifying parameters

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.


Comment thread
asimurka marked this conversation as resolved.

def _is_server_mcp_output_item(
item: dict[str, Any], configured_mcp_labels: set[str]
) -> bool:
Expand Down Expand Up @@ -611,6 +657,7 @@ async def response_generator(
turn_summary: TurnSummary,
inline_rag_context: RAGContext,
filter_server_tools: bool = False,
instructions_substituted: bool = False,
) -> AsyncIterator[str]:
"""Generate SSE-formatted streaming response with LCORE-enriched events.

Expand All @@ -622,6 +669,7 @@ async def response_generator(
turn_summary: TurnSummary to populate during streaming
inline_rag_context: Inline RAG context to be used for the response
filter_server_tools: Whether to filter server-deployed MCP tool events from the stream
instructions_substituted: Whether the server substituted the instructions
Yields:
SSE-formatted strings for streaming events, ending with [DONE]
"""
Expand All @@ -631,9 +679,7 @@ async def response_generator(

latest_response_object: Optional[OpenAIResponseObject] = None
sequence_number = 0
configured_mcp_labels = (
{s.name for s in configuration.mcp_servers} if filter_server_tools else set()
)
configured_mcp_labels = {s.name for s in configuration.mcp_servers}
# Track output indices of server-deployed MCP calls to filter their events
server_mcp_output_indices: set[int] = set()

Expand All @@ -657,6 +703,11 @@ async def response_generator(

if "response" in chunk_dict:
chunk_dict["response"]["conversation"] = normalized_conv_id
_sanitize_response_dict(
chunk_dict["response"],
configured_mcp_labels,
instructions_substituted,
)
tools = chunk_dict["response"].get("tools")
if tools is not None:
chunk_dict["response"]["tools"] = (
Expand Down Expand Up @@ -794,6 +845,7 @@ async def handle_non_streaming_response(
moderation_result: ShieldModerationResult,
inline_rag_context: RAGContext,
filter_server_tools: bool = False,
instructions_substituted: bool = False,
) -> ResponsesResponse:
"""Handle non-streaming response from Responses API.

Expand All @@ -806,6 +858,7 @@ async def handle_non_streaming_response(
moderation_result: Result of shield moderation check
inline_rag_context: Inline RAG context to be used for the response
filter_server_tools: Whether to filter server-deployed MCP tool output
instructions_substituted: Whether the server substituted the instructions
Returns:
ResponsesResponse with the completed response
"""
Expand Down Expand Up @@ -904,7 +957,11 @@ async def handle_non_streaming_response(
skip_userid_check=skip_userid_check,
topic_summary=topic_summary,
)
configured_mcp_labels = {s.name for s in configuration.mcp_servers}
response_dict = api_response.model_dump(exclude_none=True)
_sanitize_response_dict(
response_dict, configured_mcp_labels, instructions_substituted
)
tools = response_dict.get("tools")
if tools is not None:
response_dict["tools"] = translate_vector_store_ids_to_user_facing(
Expand Down
4 changes: 4 additions & 0 deletions src/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@

DEFAULT_VIOLATION_MESSAGE = "I cannot process this request due to policy restrictions."

# Placeholder slug used in responses when the server substituted its own
# system prompt for the client's instructions. Avoids leaking the actual
# server prompt back to the client.
SUBSTITUTED_INSTRUCTIONS_PLACEHOLDER = "<server prompt applied>"
# Input size limits for API request validation
# Maximum character length for the question field in /v1/infer requests (32 KiB)
RLSAPI_V1_QUESTION_MAX_LENGTH = 32_768
Expand Down
Loading
Loading