LCORE-2852: no_tools affects skills#2022
Conversation
WalkthroughA Changesno_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!
asimurka
left a comment
asimurka
left a comment
There was a problem hiding this comment.
LGTM consider my comments as optional
There was a problem hiding this comment.
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)
286-298: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
Parameters:in this docstring block.The updated docstring still extends an
Args:section. As per coding guidelines, functions insrc/**/*.pymust use 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 286 - 298, The docstring for the blocking agent run helper currently uses an Args section instead of the repository’s required Parameters convention. Update the docstring on the turn summary function in query.py to replace the Args header with Parameters, keeping the existing parameter descriptions aligned under that section and consistent with the repo style used elsewhere in src/**/*.py.Sources: Coding guidelines, Learnings
src/app/endpoints/streaming_query.py (1)
337-342: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThread
no_toolsthrough the compaction pathsrc/app/endpoints/streaming_query.py:329-341,506-510
generate_response_with_compaction(...)drops the flag, so compaction-triggered streaming requests fall back toFalseand can still attach tools even whenquery_request.no_toolsis set.🤖 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 - 342, The compaction streaming path is losing the no_tools setting, so tool attachment can re-enable during compaction-triggered requests. Update generate_response_with_compaction and its caller in streaming_query to accept and forward query_request.no_tools into retrieve_agent_response_generator, matching the existing non-compaction path so the flag is preserved end to end.
🤖 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.
Outside diff comments:
In `@src/app/endpoints/streaming_query.py`:
- Around line 337-342: The compaction streaming path is losing the no_tools
setting, so tool attachment can re-enable during compaction-triggered requests.
Update generate_response_with_compaction and its caller in streaming_query to
accept and forward query_request.no_tools into
retrieve_agent_response_generator, matching the existing non-compaction path so
the flag is preserved end to end.
In `@src/utils/agents/query.py`:
- Around line 286-298: The docstring for the blocking agent run helper currently
uses an Args section instead of the repository’s required Parameters convention.
Update the docstring on the turn summary function in query.py to replace the
Args header with Parameters, keeping the existing parameter descriptions aligned
under that section and consistent with the repo style used elsewhere in
src/**/*.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95702865-b25e-48b2-9f32-fe9673b0de4a
📒 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. (16)
- GitHub Check: spectral
- GitHub Check: unit_tests (3.12)
- GitHub Check: unit_tests (3.13)
- GitHub Check: build-pr
- GitHub Check: mypy
- GitHub Check: integration_tests (3.12)
- GitHub Check: integration_tests (3.13)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
🧰 Additional context used
📓 Path-based instructions (3)
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/app/endpoints/query.pysrc/utils/agents/streaming.pysrc/app/endpoints/streaming_query.pysrc/utils/agents/query.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
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
🧠 Learnings (3)
📚 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
📚 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:
src/app/endpoints/query.pysrc/utils/agents/streaming.pysrc/app/endpoints/streaming_query.pytests/unit/utils/test_pydantic_ai.pysrc/utils/agents/query.pysrc/utils/pydantic_ai.py
🔇 Additional comments (4)
src/utils/agents/streaming.py (1)
90-96: Still usingArgs:in this docstring block.Already noted on an earlier revision; this updated block should use the repo-standard
Parameters:header instead.src/utils/pydantic_ai.py (1)
103-110: Still usingArgs:in this docstring block.Already noted on an earlier revision; this helper should use the repo-standard
Parameters:header instead.src/app/endpoints/query.py (1)
231-238: LGTM!tests/unit/utils/test_pydantic_ai.py (1)
348-365: LGTM!
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
no toolsoption for query responses, allowing requests to skip tool-enabled processing.no toolsoption to streaming responses for consistent behavior.no toolspreference is unset.no toolsis enabled.