LCORE-2852: no_tools affects skills#2022
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughA no_tools flag propagation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/agents/query.py (1)
292-299: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
Parameters:in this docstring block.The updated docstring still uses
Args:. As per coding guidelines,src/**/*.pyfunctions should follow the repository docstring convention, and based on learnings this repo usesParameters:rather thanArgs:.Suggested fix
- Args: + Parameters:🤖 Prompt for 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. In `@src/utils/agents/query.py` around lines 292 - 299, The docstring for the function in query.py still uses the nonstandard section header, so update the parameter documentation block from Args: to Parameters: to match the repository convention. Keep the existing parameter descriptions and align the change in the same docstring near the function that lists client, responses_params, moderation_result, endpoint_path, _original_input, and no_tools.Sources: Coding guidelines, Learnings
src/app/endpoints/streaming_query.py (1)
337-341: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass
no_toolsthrough the compaction path
generate_response_with_compaction()callsretrieve_agent_response_generator()withoutno_tools, so compacted streaming requests ignorequery_request.no_toolsand can still expose tools. Thread the flag through both calls.🤖 Prompt for 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. In `@src/app/endpoints/streaming_query.py` around lines 337 - 341, The compaction flow is dropping the `no_tools` flag, so `generate_response_with_compaction()` must pass `query_request.no_tools` through to `retrieve_agent_response_generator()` just like the non-compaction path. Update the `generate_response_with_compaction` call site and the `retrieve_agent_response_generator` invocation so the flag is threaded end-to-end, using the existing `query_request.no_tools if query_request.no_tools is not None else False` pattern to keep behavior consistent.
🤖 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/utils/agents/streaming.py`:
- Around line 90-96: The docstring for the function in streaming.py still uses
Args:, but this repository’s Python docstring convention is Parameters:. Update
the docstring block for the SSE generator/turn summary helper to use Parameters:
instead of Args:, keeping the existing parameter descriptions aligned with the
function’s documented symbols such as responses_params, context, endpoint_path,
and no_tools.
In `@src/utils/pydantic_ai.py`:
- Around line 103-110: The docstring for the pydantic-ai capability assembler
uses the wrong section header; update the function docstring in the assemble
capabilities helper to follow the repository convention by replacing the Args
section with Parameters while keeping the same parameter descriptions for skills
and no_tools. Make this change in the docstring associated with the
capability-building function in pydantic_ai.py so it matches the rest of
src/**/*.py.
---
Outside diff comments:
In `@src/app/endpoints/streaming_query.py`:
- Around line 337-341: The compaction flow is dropping the `no_tools` flag, so
`generate_response_with_compaction()` must pass `query_request.no_tools` through
to `retrieve_agent_response_generator()` just like the non-compaction path.
Update the `generate_response_with_compaction` call site and the
`retrieve_agent_response_generator` invocation so the flag is threaded
end-to-end, using the existing `query_request.no_tools if query_request.no_tools
is not None else False` pattern to keep behavior consistent.
In `@src/utils/agents/query.py`:
- Around line 292-299: The docstring for the function in query.py still uses the
nonstandard section header, so update the parameter documentation block from
Args: to Parameters: to match the repository convention. Keep the existing
parameter descriptions and align the change in the same docstring near the
function that lists client, responses_params, moderation_result, endpoint_path,
_original_input, and no_tools.
🪄 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: b08ffb45-209e-49d1-af11-2cf48c850943
📒 Files selected for processing (6)
src/app/endpoints/query.pysrc/app/endpoints/streaming_query.pysrc/utils/agents/query.pysrc/utils/agents/streaming.pysrc/utils/pydantic_ai.pytests/unit/utils/test_pydantic_ai.py
📜 Review details
⏰ Context from checks skipped due to timeout. (18)
- GitHub Check: unit_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: radon
- GitHub Check: mypy
- GitHub Check: spectral
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: server 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
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
⚠️ CI failures not shown inline (2)
GitHub Actions: Black / 0_black.txt: LCORE-2852: no_tools affects skills
Conclusion: failure
##[group]Run uv tool run black --check src tests
�[36;1muv tool run black --check src tests�[0m
shell: /usr/bin/bash -e {0}
env:
UV_PYTHON: 3.12
VIRTUAL_ENV: /home/runner/work/lightspeed-stack/lightspeed-stack/.venv
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Downloading black (1.8MiB)
Downloaded black
Installed 7 packages in 13ms
Warning: Python 3.12 cannot parse code formatted for Python 3.13. To fix this: run Black with Python 3.13, set --target-version to py312, or use --fast to skip the safety check. Black's safety check verifies equivalence by parsing the AST, which fails when the running Python is older than the target version.
would reformat /home/runner/work/lightspeed-stack/lightspeed-stack/src/app/endpoints/query.py
would reformat /home/runner/work/lightspeed-stack/lightspeed-stack/src/app/endpoints/streaming_query.py
Oh no! 💥 💔 💥
2 files would be reformatted, 445 files would be left unchanged.
##[error]Process completed with exit code 1.
GitHub Actions: Black / black: LCORE-2852: no_tools affects skills
Conclusion: failure
##[group]Run uv tool run black --check src tests
�[36;1muv tool run black --check src tests�[0m
shell: /usr/bin/bash -e {0}
env:
UV_PYTHON: 3.12
VIRTUAL_ENV: /home/runner/work/lightspeed-stack/lightspeed-stack/.venv
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Downloading black (1.8MiB)
Downloaded black
Installed 7 packages in 13ms
Warning: Python 3.12 cannot parse code formatted for Python 3.13. To fix this: run Black with Python 3.13, set --target-version to py312, or use --fast to skip the safety check. Black's safety check verifies equivalence by parsing the AST, which fails when the running Python is older than the target version.
would reformat /home/runner/work/lightspeed-stack/lightspeed-stack/src/app/endpoints/query.py
would reformat /home/runner/work/lightspeed-stack/lightspeed-stack/src/app/endpoints/streaming_query.py
Oh no! 💥 💔 💥
2 files would be reformatted, 445 files would be left unchanged.
##[error]Process completed with exit code 1.
🧰 Additional context used
📓 Path-based instructions (3)
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_pydantic_ai.py
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/utils/agents/query.pysrc/app/endpoints/query.pysrc/app/endpoints/streaming_query.pysrc/utils/agents/streaming.pysrc/utils/pydantic_ai.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/query.pysrc/app/endpoints/streaming_query.py
🧠 Learnings (3)
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
tests/unit/utils/test_pydantic_ai.pysrc/utils/agents/query.pysrc/app/endpoints/query.pysrc/app/endpoints/streaming_query.pysrc/utils/agents/streaming.pysrc/utils/pydantic_ai.py
📚 Learning: 2026-01-14T09:37:51.612Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 988
File: src/app/endpoints/query.py:319-339
Timestamp: 2026-01-14T09:37:51.612Z
Learning: In the lightspeed-stack repository, when provider_id == "azure", the Azure provider with provider_type "remote::azure" is guaranteed to be present in the providers list. Therefore, avoid defensive StopIteration handling for next() when locating the Azure provider in providers within src/app/endpoints/query.py. This change applies specifically to this file (or nearby provider lookup code) and relies on the invariant that the Azure provider exists; if the invariant could be violated, keep the existing StopIteration handling.
Applied to files:
src/app/endpoints/query.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/query.pysrc/app/endpoints/streaming_query.py
🪛 GitHub Actions: Black / 0_black.txt
src/app/endpoints/query.py
[error] 1-1: Black --check reported formatting issues. This file would be reformatted.
src/app/endpoints/streaming_query.py
[error] 1-1: Black --check reported formatting issues. This file would be reformatted.
🪛 GitHub Actions: Black / black
src/app/endpoints/query.py
[error] 1-1: Black --check failed: would reformat file. Run 'uv tool run black' (or 'black --write') to apply formatting.
src/app/endpoints/streaming_query.py
[error] 1-1: Black --check failed: would reformat file. Run 'uv tool run black' (or 'black --write') to apply formatting.
🔇 Additional comments (5)
src/app/endpoints/query.py (1)
237-237: LGTM!src/utils/agents/query.py (1)
317-319: LGTM!src/utils/agents/streaming.py (1)
123-125: LGTM!src/utils/pydantic_ai.py (1)
10-10: LGTM!Also applies to: 115-124, 127-131, 150-152, 162-162
tests/unit/utils/test_pydantic_ai.py (1)
348-365: LGTM!
| """Return the SSE generator and mutable turn summary for an agent run. | ||
| Args: | ||
| responses_params: Prepared Responses API parameters. | ||
| context: Streaming request context and moderation result. | ||
| endpoint_path: Endpoint path used for metric labeling. | ||
| no_tools: Whether to skip tool processing. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Parameters: in this docstring block.
The updated docstring still uses Args:. As per coding guidelines, src/**/*.py functions should follow the repository docstring convention, and based on learnings this repo uses Parameters: rather than Args:.
Suggested fix
- Args:
+ Parameters:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Return the SSE generator and mutable turn summary for an agent run. | |
| Args: | |
| responses_params: Prepared Responses API parameters. | |
| context: Streaming request context and moderation result. | |
| endpoint_path: Endpoint path used for metric labeling. | |
| no_tools: Whether to skip tool processing. | |
| """Return the SSE generator and mutable turn summary for an agent run. | |
| Args: | |
| responses_params: Prepared Responses API parameters. | |
| context: Streaming request context and moderation result. | |
| endpoint_path: Endpoint path used for metric labeling. | |
| no_tools: Whether to skip tool processing. |
🤖 Prompt for 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.
In `@src/utils/agents/streaming.py` around lines 90 - 96, The docstring for the
function in streaming.py still uses Args:, but this repository’s Python
docstring convention is Parameters:. Update the docstring block for the SSE
generator/turn summary helper to use Parameters: instead of Args:, keeping the
existing parameter descriptions aligned with the function’s documented symbols
such as responses_params, context, endpoint_path, and no_tools.
Sources: Coding guidelines, Learnings
| """Assemble pydantic-ai capabilities for an LCS agent. | ||
| Args: | ||
| skills: Agent skills configuration from LCS, or None when skills are disabled. | ||
| no_tools: When True, omit capabilities that expose a toolset via ``get_toolset()``. | ||
| Returns: | ||
| Configured capabilities, or None when no capabilities are enabled. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Parameters: in this docstring block.
This new docstring section uses Args: instead of the repository-standard header. As per coding guidelines, src/**/*.py functions should follow the repository docstring convention, and based on learnings this repo uses Parameters: rather than Args:.
Suggested fix
- Args:
+ Parameters:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Assemble pydantic-ai capabilities for an LCS agent. | |
| Args: | |
| skills: Agent skills configuration from LCS, or None when skills are disabled. | |
| no_tools: When True, omit capabilities that expose a toolset via ``get_toolset()``. | |
| Returns: | |
| Configured capabilities, or None when no capabilities are enabled. | |
| """Assemble pydantic-ai capabilities for an LCS agent. | |
| Parameters: | |
| skills: Agent skills configuration from LCS, or None when skills are disabled. | |
| no_tools: When True, omit capabilities that expose a toolset via ``get_toolset()``. | |
| Returns: | |
| Configured capabilities, or None when no capabilities are enabled. |
🤖 Prompt for 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.
In `@src/utils/pydantic_ai.py` around lines 103 - 110, The docstring for the
pydantic-ai capability assembler uses the wrong section header; update the
function docstring in the assemble capabilities helper to follow the repository
convention by replacing the Args section with Parameters while keeping the same
parameter descriptions for skills and no_tools. Make this change in the
docstring associated with the capability-building function in pydantic_ai.py so
it matches the rest of src/**/*.py.
Sources: Coding guidelines, Learnings
asimurka
left a comment
asimurka
left a comment
There was a problem hiding this comment.
LGTM consider my comments as optional
| compaction.original_input if compaction.compacted else None, | ||
| no_tools=( | ||
| query_request.no_tools if query_request.no_tools is not None else False | ||
| ), |
There was a problem hiding this comment.
nit: bool(query_request.no_tools) works as well
| Configured capabilities, or None when no capabilities are enabled. | ||
| """ | ||
| capabilities: list[AgentCapability[SkillsCapability]] = [] | ||
| capabilities: list[AgentCapability[Any]] = [] |
There was a problem hiding this comment.
nit: object would be even better
3 participants
Description
When clients set
no_tools=trueon query or streaming query requests, the agent should not register tool-bearing capabilities (including skills). Previously,no_toolswas not passed through to agent construction, so skills could still be attached even when tools were explicitly disabled.This change threads the
no_toolsflag from the query and streaming query endpoints intobuild_agent, and filters out any pydantic-ai capability that exposes a toolset viaget_toolset(). A unit test confirms thatSkillsCapabilityis omitted whenno_tools=True.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
test_agent_excludes_tool_capabilities_when_no_toolsintests/unit/utils/test_pydantic_ai.pyto verifySkillsCapabilityis excluded whenno_tools=True.no_tools=trueto confirm skills/tool capabilities are not attached to the agent.Summary by CodeRabbit
New Features
Bug Fixes
Tests