LCORE-1427: Normalize Tool and Inline RAG default behavior#2146
LCORE-1427: Normalize Tool and Inline RAG default behavior#2146Jazzcort wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 12 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 (1)
WalkthroughRAG configuration now defaults both strategies to disabled, and Tool RAG requires explicit vector-store configuration. Response tool preparation no longer accepts an explicit client or falls back to all stores. Tests and integration teardown behavior were updated accordingly. ChangesRAG strategy behavior
Container test cleanup
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant prepare_tools
participant RagConfiguration
participant LlamaStack
Request->>prepare_tools: vector_store_ids or None
prepare_tools->>RagConfiguration: read rag.tool when request IDs are absent
prepare_tools->>LlamaStack: list vector stores only through get_vector_store_ids
prepare_tools-->>Request: prepared tools or None
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/utils/test_responses.py (1)
3062-3065: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename test to reflect the updated function signature.
Since
get_vector_store_idsno longer accepts avector_store_idsparameter, the "when_no_ids_provided" suffix is obsolete. Consider renaming the test for clarity.♻️ Proposed fix
- async def test_fetches_all_when_no_ids_provided( + async def test_fetches_all_vector_stores( self, mocker: MockerFixture ) -> None: - """Test that all vector stores are fetched when no IDs provided.""" + """Test that all available vector stores are fetched."""🤖 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 `@tests/unit/utils/test_responses.py` around lines 3062 - 3065, Rename the test method test_fetches_all_when_no_ids_provided to reflect that get_vector_store_ids no longer accepts a vector_store_ids argument, removing the obsolete “when_no_ids_provided” wording while preserving the test behavior.
🤖 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 `@tests/unit/utils/test_responses.py`:
- Around line 3062-3065: Rename the test method
test_fetches_all_when_no_ids_provided to reflect that get_vector_store_ids no
longer accepts a vector_store_ids argument, removing the obsolete
“when_no_ids_provided” wording while preserving the test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8e521c5-292b-4e00-a6df-6ba018037946
📒 Files selected for processing (8)
docs/byok_guide.mddocs/config.mddocs/models/responses_succ.mddocs/openapi.mdexamples/lightspeed-stack-byok-okp-rag.yamlsrc/models/config.pysrc/utils/responses.pytests/unit/utils/test_responses.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: integration_tests (3.12)
- GitHub Check: integration_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: unit_tests (3.13)
- GitHub Check: Pylinter
- GitHub Check: build-pr
- GitHub Check: mypy
- 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: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-7-on-pull-request
⚠️ CI failures not shown inline (3)
GitHub Actions: OpenAPI (Spectral) / 0_spectral.txt: LCORE-1427: Normalize Tool and Inline RAG default behavior
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
GitHub Actions: Pydocstyle / 0_pydocstyle.txt: LCORE-1427: Normalize Tool and Inline RAG default behavior
Conclusion: failure
##[group]Run uv tool run pydocstyle -v src tests
�[36;1muv tool run pydocstyle -v 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]
Installed 2 packages in 3ms
Checking file src/configuration.py.
Checking file src/__init__.py.
Checking file src/version.py.
Checking file src/sentry.py.
Checking file src/lightspeed_stack.py.
Checking file src/client.py.
Checking file src/constants.py.
Checking file src/log.py.
Checking file src/llama_stack_configuration.py.
Checking file src/quota/quota_exceed_error.py.
Checking file src/quota/sql.py.
Checking file src/quota/__init__.py.
Checking file src/quota/user_quota_limiter.py.
Checking file src/quota/connect_pg.py.
Checking file src/quota/revokable_quota_limiter.py.
Checking file src/quota/quota_limiter.py.
Checking file src/quota/quota_limiter_factory.py.
Checking file src/quota/connect_sqlite.py.
Checking file src/quota/token_usage_history.py.
Checking file src/quota/cluster_quota_limiter.py.
Checking file src/utils/vector_search.py.
Checking file src/utils/stream_interrupts.py.
Checking file src/utils/prompts.py.
Checking file src/utils/__init__.py.
Checking file src/utils/mcp_oauth_probe.py.
Checking file src/utils/checks.py.
Checking file src/utils/config_dumper.py.
Checking file src/utils/openapi_schema_dumper.py.
Checking file src/utils/quota_utils.py.
Checking file src/utils/endpoints.py.
Checking file src/utils/responses.py.
Checking file src/utils/rh_identity.py.
Checking file src/utils/reranker.py.
Checking file src/utils/token_counter.py.
Checking file src/utils/pydantic_ai_helpers.py.
Checking file src/utils/mcp_auth_headers.py.
Checking file src/utils/degraded_mode.py.
Checking file src/utils/json_schema_updater.py.
Checking file src/utils/llama_stack_version.py.
Checking file src/utils/mcp_headers.py.
Checkin...
GitHub Actions: OpenAPI (Spectral) / spectral: LCORE-1427: Normalize Tool and Inline RAG default behavior
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.
Files:
examples/lightspeed-stack-byok-okp-rag.yamldocs/byok_guide.mddocs/models/responses_succ.mddocs/config.mddocs/openapi.mdsrc/utils/responses.pysrc/models/config.pytests/unit/utils/test_responses.py
**/*.{py,toml,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
Run and satisfy the project's configured quality checks, including Black, Pylint, Pyright, Ruff, pydocstyle, type checks, and Bandit security scanning.
Files:
examples/lightspeed-stack-byok-okp-rag.yamlsrc/utils/responses.pysrc/models/config.pytests/unit/utils/test_responses.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal Python modules and checkpyproject.tomlfor existing dependencies and versions before adding dependencies.
All Python modules must begin with descriptive docstrings; package__init__.pyfiles must contain brief package descriptions.
Uselogger = get_logger(__name__)fromlog.pyfor module logging.
UseFinal[type]for constants and checkconstants.pyfor shared constants before defining new ones.
Define type aliases at module level for clarity.
All functions must have Google-style docstrings and complete parameter and return type annotations.
Usetyping_extensions.Selffor model validators, modern union syntax such asstr | int, andOptional[Type]for optional values.
Use descriptive, action-oriented snake_case names for functions, such asget_,validate_, andcheck_.
Do not modify parameter objects in place when producing results; return a new data structure instead.
Useasync deffor I/O operations and external API calls.
HandleAPIConnectionErrorfrom Llama Stack integrations.
Use standard logger levels appropriately:debugfor diagnostics,infofor normal execution,warningfor unexpected or potentially problematic conditions, anderrorfor serious failures.
All classes must have descriptive Google-style docstrings, complete attribute type annotations, and specific types instead ofAny.
Use PascalCase for classes with descriptive suffixes, includingConfiguration,Error/Exception,Resolver, andInterfacewhere applicable.
UseABCand@abstractmethodfor abstract interfaces.
Follow Google Python docstring conventions and includeParameters,Returns,Raises, andAttributessections when applicable.
Files:
src/utils/responses.pysrc/models/config.pytests/unit/utils/test_responses.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
BaseModelfor data models,ConfigurationBasefor configuration models, and Pydantic@model_validatorand@field_validatorfor validation.
Files:
src/models/config.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for all unit tests; do not use unittest. Put shared fixtures in
conftest.py, usepytest-mockfor mocks, mark async tests withpytest.mark.asyncio, and maintain at least 60% unit-test coverage.
Files:
tests/unit/utils/test_responses.py
🧠 Learnings (6)
📚 Learning: 2026-05-20T08:09:30.641Z
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:30.641Z
Learning: In Llama-stack config YAMLs, when defining a Llama Guard safety shield entry, set `provider_shield_id` to the *guard model identifier* (e.g., `meta-llama/Llama-Guard-3-8B`). Do not use a chat/generative model id (e.g., `openai/gpt-4o-mini`): a chat-model id (or `native_override`) indicates only an override landed and does **not** mean the safety shield is actually gating queries. Ensure any E2E coverage for the related implementation (JIRA/E2E tests) exercises a real Llama Guard model to verify that the shield is effective.
Applied to files:
examples/lightspeed-stack-byok-okp-rag.yaml
📚 Learning: 2026-02-23T14:56:59.186Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1198
File: src/utils/responses.py:184-192
Timestamp: 2026-02-23T14:56:59.186Z
Learning: In the lightspeed-stack codebase (lightspeed-core/lightspeed-stack), do not enforce de-duplication of duplicate client.models.list() calls in model selection flows (e.g., in src/utils/responses.py prepare_responses_params). These calls are considered relatively cheap and removing duplicates could add unnecessary complexity to the flow. Apply this guideline specifically to this file/context unless similar performance characteristics and design decisions are documented elsewhere.
Applied to files:
src/utils/responses.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/utils/responses.pysrc/models/config.pytests/unit/utils/test_responses.py
📚 Learning: 2026-07-06T15:26:18.398Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 2071
File: src/models/config.py:2416-2422
Timestamp: 2026-07-06T15:26:18.398Z
Learning: In this repo’s Python code under src/**, don’t treat differences in string concatenation style as a style inconsistency when Black has effectively forced (or made clearer) use of explicit `+` string concatenation in multi-line logger/string expressions. If adjacent-literal implicit concatenation is avoided/changed specifically to accommodate Black’s formatting in these call sites, accept the `+` usage and don’t recommend converting it solely for consistency with nearby blocks that use implicit concatenation.
Applied to files:
src/utils/responses.pysrc/models/config.py
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/config.py
🪛 GitHub Actions: Pydocstyle / 0_pydocstyle.txt
src/utils/responses.py
[error] 133-133: pydocstyle: D401: First line should be in imperative mood (perhaps 'Fetch', not 'Fetche')
🪛 GitHub Actions: Pydocstyle / pydocstyle
src/utils/responses.py
[error] 133-133: pydocstyle: D401 violation in public function get_vector_store_ids: First line should be in imperative mood (perhaps 'Fetch', not 'Fetche').
🔇 Additional comments (6)
docs/byok_guide.md (1)
264-268: LGTM!docs/config.md (1)
609-616: LGTM!docs/models/responses_succ.md (1)
1843-1850: LGTM!docs/openapi.md (1)
8158-8165: LGTM!examples/lightspeed-stack-byok-okp-rag.yaml (1)
63-63: LGTM!src/models/config.py (1)
2261-2262: LGTM!Also applies to: 2277-2277
6e81e70 to
ad40b35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/models/config.py (1)
701-733: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse modern union syntax for the optional ID.
The changed annotations use
Optional[str], but repository Python code must usestr | None. Update both the field and validator signature consistently.Proposed fix
- id: Optional[str] = Field( + id: str | None = Field( ... - def validate_id(cls, value: Optional[str]) -> Optional[str]: + def validate_id(cls, value: str | None) -> str | None:As per coding guidelines,
src/**/*.pymust use modern union syntax.🤖 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/models/config.py` around lines 701 - 733, Update the Provider configuration model’s id field annotation and the validate_id validator parameter and return annotations to use str | None instead of Optional[str], keeping the existing optional-ID behavior unchanged.Source: Coding guidelines
tests/integration/endpoints/test_query_integration.py (1)
618-658: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant mock setup and unused
mockerparameter.Since
prepare_toolsno longer falls back to fetching all vector stores (and this test exercisesno_tools=Trueanyway), the mock setup forvector_stores.listis dead code. You can safely remove it along with themockerfixture parameter, matching the excellent cleanup already done intest_query_v2_endpoint_uses_tools_when_available.♻️ Proposed refactor
-async def test_query_v2_endpoint_bypasses_tools_when_no_tools_true( - test_config: AppConfig, - mock_llama_stack_client: AsyncMockType, - mock_query_agent: AsyncMockType, - test_request: Request, - test_auth: AuthTuple, - patch_db_session: Session, - mocker: MockerFixture, -) -> None: +async def test_query_v2_endpoint_bypasses_tools_when_no_tools_true( + test_config: AppConfig, + mock_llama_stack_client: AsyncMockType, + mock_query_agent: AsyncMockType, + test_request: Request, + test_auth: AuthTuple, + patch_db_session: Session, +) -> None: """Test that tools are NOT used when no_tools=True. ... test_auth: noop authentication tuple patch_db_session: Test database session - mocker: pytest-mock fixture Returns: ... _ = test_config _ = patch_db_session - mock_vector_store = mocker.MagicMock() - mock_vector_store.id = "vs-test-123" - - mock_list_result = mocker.MagicMock() - mock_list_result.data = [mock_vector_store] - - mock_llama_stack_client.vector_stores.list.return_value = mock_list_result - query_request = QueryRequest(query="What is Ansible?", no_tools=True)🤖 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 `@tests/integration/endpoints/test_query_integration.py` around lines 618 - 658, In test_query_v2_endpoint_bypasses_tools_when_no_tools_true, remove the unused mocker parameter and delete the mock_vector_store, mock_list_result, and vector_stores.list mock setup; retain the test configuration, session setup, and remaining assertions unchanged.
🤖 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/responses.py`:
- Around line 129-131: Delete the unused get_vector_store_ids helper and remove
its dedicated TestGetVectorStoreIds tests. Verify no remaining references
require updates, while leaving unrelated response utilities and tests unchanged.
---
Outside diff comments:
In `@src/models/config.py`:
- Around line 701-733: Update the Provider configuration model’s id field
annotation and the validate_id validator parameter and return annotations to use
str | None instead of Optional[str], keeping the existing optional-ID behavior
unchanged.
In `@tests/integration/endpoints/test_query_integration.py`:
- Around line 618-658: In
test_query_v2_endpoint_bypasses_tools_when_no_tools_true, remove the unused
mocker parameter and delete the mock_vector_store, mock_list_result, and
vector_stores.list mock setup; retain the test configuration, session setup, and
remaining assertions unchanged.
🪄 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: 86d8317b-70c5-4d37-955c-dabb33acd5b3
📒 Files selected for processing (14)
.github/workflows/integration_tests.yamldocs/models/responses_succ.mddocs/openapi.jsondocs/openapi.mddocs/user_doc/byok_guide.mddocs/user_doc/config.mdexamples/lightspeed-stack-byok-okp-rag.yamlsrc/models/config.pysrc/utils/responses.pytests/integration/endpoints/test_query_integration.pytests/integration/endpoints/test_responses_byok_integration.pytests/integration/endpoints/test_responses_integration.pytests/unit/app/endpoints/test_responses.pytests/unit/utils/test_responses.py
💤 Files with no reviewable changes (3)
- tests/integration/endpoints/test_responses_byok_integration.py
- tests/unit/app/endpoints/test_responses.py
- tests/integration/endpoints/test_responses_integration.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.
Files:
docs/user_doc/byok_guide.mdexamples/lightspeed-stack-byok-okp-rag.yamldocs/openapi.mddocs/openapi.jsondocs/models/responses_succ.mddocs/user_doc/config.mdsrc/models/config.pytests/integration/endpoints/test_query_integration.pytests/unit/utils/test_responses.pysrc/utils/responses.py
**/*.{py,yaml,yml,json,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Never commit secrets or keys; use environment variables for sensitive data.
Files:
examples/lightspeed-stack-byok-okp-rag.yamldocs/openapi.jsonsrc/models/config.pytests/integration/endpoints/test_query_integration.pytests/unit/utils/test_responses.pysrc/utils/responses.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal Python modules.
Start every module with a descriptive docstring and uselogger = get_logger(__name__)for module logging.
Define shared constants in the centralconstants.pymodule, use descriptive comments, and annotate constants withFinal[type].
Use complete type annotations for function parameters and return values, modern union syntax, andtyping_extensions.Selffor model validators.
Document all modules, classes, and functions using Google-style Python docstrings, including applicable Parameters, Returns, Raises, and Attributes sections.
Use descriptive snake_case, action-oriented function names such asget_,validate_, andcheck_.
Avoid modifying input parameters in place; return a newly created data structure instead.
Useasync deffor I/O operations and external API calls.
Use standard logger levels appropriately: debug for diagnostics, info for normal execution, warning for unexpected conditions, and error for serious failures.
Name classes with PascalCase and use descriptive suffixes such asConfiguration,Error/Exception,Resolver, andInterfacewhere applicable.
Use ABC and@abstractmethodfor abstract interfaces; provide complete, specific type annotations for class attributes and avoidAny.
Files:
src/models/config.pytests/integration/endpoints/test_query_integration.pytests/unit/utils/test_responses.pysrc/utils/responses.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
ConfigurationBasefor configuration Pydantic models andBaseModelfor data models; use@model_validatorand@field_validatorfor validation.
Files:
src/models/config.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Configuration models must extend
ConfigurationBase, whoseextra="forbid"behavior rejects unknown fields; use types such asOptional[FilePath],PositiveInt, andSecretStrwhere appropriate.
Files:
src/models/config.pysrc/utils/responses.py
tests/integration/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for all integration tests; do not use unittest.
Files:
tests/integration/endpoints/test_query_integration.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain at least 60% unit-test coverage and 10% integration-test coverage.
Files:
tests/integration/endpoints/test_query_integration.pytests/unit/utils/test_responses.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for unit tests, shared fixtures in
conftest.py,pytest-mockfor mocks, andpytest.mark.asynciofor asynchronous tests; do not use unittest.
Files:
tests/unit/utils/test_responses.py
🧠 Learnings (7)
📚 Learning: 2026-05-06T08:35:54.687Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1690
File: .github/workflows/e2e_tests_providers.yaml:279-285
Timestamp: 2026-05-06T08:35:54.687Z
Learning: In .github/workflows/e2e_tests_providers.yaml and related e2e workflow files, the show_logs step should not use docker compose logs with --tail or --since (i.e., keep logs unbounded). The quick connectivity test runs once immediately after container startup, so the log output is small and a log tail limit is unnecessary. If you adjust this, add a rationale comment in the workflow explaining why unbounded logs are acceptable and ensure CI behavior remains deterministic.
Applied to files:
.github/workflows/integration_tests.yaml
📚 Learning: 2026-05-20T08:09:30.641Z
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:30.641Z
Learning: In Llama-stack config YAMLs, when defining a Llama Guard safety shield entry, set `provider_shield_id` to the *guard model identifier* (e.g., `meta-llama/Llama-Guard-3-8B`). Do not use a chat/generative model id (e.g., `openai/gpt-4o-mini`): a chat-model id (or `native_override`) indicates only an override landed and does **not** mean the safety shield is actually gating queries. Ensure any E2E coverage for the related implementation (JIRA/E2E tests) exercises a real Llama Guard model to verify that the shield is effective.
Applied to files:
examples/lightspeed-stack-byok-okp-rag.yaml
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/config.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/models/config.pytests/integration/endpoints/test_query_integration.pytests/unit/utils/test_responses.pysrc/utils/responses.py
📚 Learning: 2026-07-06T15:26:18.398Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 2071
File: src/models/config.py:2416-2422
Timestamp: 2026-07-06T15:26:18.398Z
Learning: In this repo’s Python code under src/**, don’t treat differences in string concatenation style as a style inconsistency when Black has effectively forced (or made clearer) use of explicit `+` string concatenation in multi-line logger/string expressions. If adjacent-literal implicit concatenation is avoided/changed specifically to accommodate Black’s formatting in these call sites, accept the `+` usage and don’t recommend converting it solely for consistency with nearby blocks that use implicit concatenation.
Applied to files:
src/models/config.pysrc/utils/responses.py
📚 Learning: 2026-02-23T14:56:59.186Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1198
File: src/utils/responses.py:184-192
Timestamp: 2026-02-23T14:56:59.186Z
Learning: In the lightspeed-stack codebase (lightspeed-core/lightspeed-stack), do not enforce de-duplication of duplicate client.models.list() calls in model selection flows (e.g., in src/utils/responses.py prepare_responses_params). These calls are considered relatively cheap and removing duplicates could add unnecessary complexity to the flow. Apply this guideline specifically to this file/context unless similar performance characteristics and design decisions are documented elsewhere.
Applied to files:
src/utils/responses.py
🪛 ast-grep (0.44.1)
tests/unit/utils/test_responses.py
[warning] 1613-1613: Do not make http calls without encryption
Context: "http://test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🔇 Additional comments (10)
src/models/config.py (1)
2261-2277: LGTM!docs/models/responses_succ.md (1)
1843-1850: LGTM!docs/openapi.json (1)
18087-18093: LGTM!docs/openapi.md (1)
8158-8165: LGTM!docs/user_doc/config.md (1)
609-616: LGTM!Also applies to: 826-832
docs/user_doc/byok_guide.md (1)
264-264: LGTM!examples/lightspeed-stack-byok-okp-rag.yaml (1)
63-63: LGTM!tests/unit/utils/test_responses.py (1)
1597-1637: LGTM!Also applies to: 1702-1741, 1748-1841, 3031-3077
tests/integration/endpoints/test_query_integration.py (1)
694-702: 📐 Maintainability & Code Quality | 💤 Low valueRemove
mockerfrom the docstring.The
mockerparameter was removed from the function signature but is still listed in the docstring.📝 Proposed fix
test_auth: noop authentication tuple patch_db_session: Test database session - mocker: pytest-mock fixture.github/workflows/integration_tests.yaml (1)
18-26: LGTM!
| async def get_vector_store_ids( | ||
| client: AsyncLlamaStackClient, | ||
| vector_store_ids: Optional[list[str]] = None, | ||
| ) -> list[str]: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for usages of get_vector_store_ids outside of its definition and tests.
rg -n -w 'get_vector_store_ids' -g '!tests/**' -g '!*test*.py'Repository: lightspeed-core/lightspeed-stack
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the function definition and search all references, including tests and imports.
sed -n '1,220p' src/utils/responses.py
printf '\n--- REFERENCES ---\n'
rg -n -w 'get_vector_store_ids' .
printf '\n--- IMPORTS ---\n'
rg -n 'from\s+src\.utils\.responses\s+import|import\s+.*responses' . --glob '!**/__pycache__/**'Repository: lightspeed-core/lightspeed-stack
Length of output: 9872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '3000,3095p' tests/unit/utils/test_responses.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 3750
Remove get_vector_store_ids and its dedicated tests
No production code references this helper anymore, so it can be deleted along with TestGetVectorStoreIds.
🤖 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/responses.py` around lines 129 - 131, Delete the unused
get_vector_store_ids helper and remove its dedicated TestGetVectorStoreIds
tests. Verify no remaining references require updates, while leaving unrelated
response utilities and tests unchanged.
|
The Ad-hoc free disk space step helps us to pass integration test more consistently, but not sure if it's a right thing to do. We maybe need to optimize |
d00eb6e to
3a48c84
Compare
…nfigured Previously, when neither rag.tool nor rag.inline were configured, prepare_tools would fetch and register all vector stores from llama-stack as a backward-compatibility fallback. This implicit behavior is removed — tool RAG now requires explicit configuration via rag.tool. Updated docs, config model, example YAML, and tests to reflect the new behavior.
c118d26 to
f678857
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tests/integration/container_lifecycle/test_container_lifecycle.py`:
- Around line 68-72: Update the container cleanup subprocess.run call in the
lifecycle fixture to remove the image referenced by the configured
LLAMA_STACK_IMAGE value rather than the hardcoded lightspeed-llama-stack:local
tag. Preserve the existing forced removal, output capture, and timeout behavior.
- Around line 68-77: Update the teardown cleanup sequence containing the
container runtime “rmi” and “image prune” subprocess.run calls to handle
TimeoutExpired and OSError independently, logging each exception at warning
level and continuing to the next cleanup step. Preserve visibility of non-zero
exit codes by warn-logging failed subprocess results instead of silently
ignoring them, so cleanup remains best-effort without aborting fixture teardown.
- Around line 68-77: Update the container lifecycle cleanup fixture around the
unconditional image removal so it only removes images created by the current
test session. Use a unique session-specific image tag or track created image
identifiers, and pass that ownership information to cleanup; preserve pruning
behavior while preventing deletion of pre-existing or concurrently used shared
images.
🪄 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: b8cc2148-ea86-469e-87cf-74933e18b752
📒 Files selected for processing (1)
tests/integration/container_lifecycle/test_container_lifecycle.py
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 1
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.
Files:
tests/integration/container_lifecycle/test_container_lifecycle.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal Python modules.
Start every module with a descriptive docstring and uselogger = get_logger(__name__)for module logging.
Define shared constants in the centralconstants.pymodule, use descriptive comments, and annotate constants withFinal[type].
Use complete type annotations for function parameters and return values, modern union syntax, andtyping_extensions.Selffor model validators.
Document all modules, classes, and functions using Google-style Python docstrings, including applicable Parameters, Returns, Raises, and Attributes sections.
Use descriptive snake_case, action-oriented function names such asget_,validate_, andcheck_.
Avoid modifying input parameters in place; return a newly created data structure instead.
Useasync deffor I/O operations and external API calls.
Use standard logger levels appropriately: debug for diagnostics, info for normal execution, warning for unexpected conditions, and error for serious failures.
Name classes with PascalCase and use descriptive suffixes such asConfiguration,Error/Exception,Resolver, andInterfacewhere applicable.
Use ABC and@abstractmethodfor abstract interfaces; provide complete, specific type annotations for class attributes and avoidAny.
Files:
tests/integration/container_lifecycle/test_container_lifecycle.py
tests/integration/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for all integration tests; do not use unittest.
Files:
tests/integration/container_lifecycle/test_container_lifecycle.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain at least 60% unit-test coverage and 10% integration-test coverage.
Files:
tests/integration/container_lifecycle/test_container_lifecycle.py
**/*.{py,yaml,yml,json,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Never commit secrets or keys; use environment variables for sensitive data.
Files:
tests/integration/container_lifecycle/test_container_lifecycle.py
🧠 Learnings (1)
📚 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/integration/container_lifecycle/test_container_lifecycle.py
🪛 ast-grep (0.44.1)
tests/integration/container_lifecycle/test_container_lifecycle.py
[error] 67-71: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "rmi", "-f", "lightspeed-llama-stack:local"],
capture_output=True,
timeout=CONTAINER_CLEANUP_TIMEOUT,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 72-76: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "image", "prune", "-f"],
capture_output=True,
timeout=CONTAINER_CLEANUP_TIMEOUT,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🔇 Additional comments (2)
tests/integration/container_lifecycle/test_container_lifecycle.py (2)
56-56: 🎯 Functional CorrectnessUse a complete generator annotation.
Generator[None]provides only the yield type, whileGeneratornormally requires yield, send, and return types. UseGenerator[None, None, None]orIterator[None], after verifying the repository’s declared Python target.
55-56: 🎯 Functional CorrectnessNo scope mismatch here
container_runtimeis already session-scoped, so this fixture dependency is fine.> Likely an incorrect or invalid review comment.
| subprocess.run( | ||
| [container_runtime, "rmi", "-f", "lightspeed-llama-stack:local"], | ||
| capture_output=True, | ||
| timeout=CONTAINER_CLEANUP_TIMEOUT, | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Honor the configured image tag during cleanup.
The repository documents LLAMA_STACK_IMAGE as customizable, but this fixture always removes only lightspeed-llama-stack:local. A custom tagged image will survive because image prune does not remove tagged images.
🤖 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 `@tests/integration/container_lifecycle/test_container_lifecycle.py` around
lines 68 - 72, Update the container cleanup subprocess.run call in the lifecycle
fixture to remove the image referenced by the configured LLAMA_STACK_IMAGE value
rather than the hardcoded lightspeed-llama-stack:local tag. Preserve the
existing forced removal, output capture, and timeout behavior.
| subprocess.run( | ||
| [container_runtime, "rmi", "-f", "lightspeed-llama-stack:local"], | ||
| capture_output=True, | ||
| timeout=CONTAINER_CLEANUP_TIMEOUT, | ||
| ) | ||
| subprocess.run( | ||
| [container_runtime, "image", "prune", "-f"], | ||
| capture_output=True, | ||
| timeout=CONTAINER_CLEANUP_TIMEOUT, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make teardown cleanup best-effort.
subprocess.run can raise TimeoutExpired or OSError during teardown; if the first command times out, pruning is skipped and the test session may fail during fixture teardown. Catch cleanup exceptions, continue to the next cleanup step, and emit a warning instead of silently discarding non-zero exit codes. As per coding guidelines, unexpected cleanup failures should use warning-level logging.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 72-76: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "image", "prune", "-f"],
capture_output=True,
timeout=CONTAINER_CLEANUP_TIMEOUT,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 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 `@tests/integration/container_lifecycle/test_container_lifecycle.py` around
lines 68 - 77, Update the teardown cleanup sequence containing the container
runtime “rmi” and “image prune” subprocess.run calls to handle TimeoutExpired
and OSError independently, logging each exception at warning level and
continuing to the next cleanup step. Preserve visibility of non-zero exit codes
by warn-logging failed subprocess results instead of silently ignoring them, so
cleanup remains best-effort without aborting fixture teardown.
Source: Coding guidelines
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not force-remove an image the session does not own.
Because this autouse fixture unconditionally runs rmi -f, it can delete a pre-existing image or an image being used by another concurrent test session. Track images created by this session, use a unique test tag, or otherwise protect shared artifacts.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 72-76: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "image", "prune", "-f"],
capture_output=True,
timeout=CONTAINER_CLEANUP_TIMEOUT,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 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 `@tests/integration/container_lifecycle/test_container_lifecycle.py` around
lines 68 - 77, Update the container lifecycle cleanup fixture around the
unconditional image removal so it only removes images created by the current
test session. Use a unique session-specific image tag or track created image
identifiers, and pass that ownership information to cleanup; preserve pruning
behavior while preventing deletion of pre-existing or concurrently used shared
images.
f678857 to
7d4dca6
Compare
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)
tests/integration/endpoints/test_query_integration.py (1)
686-703: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the docstring for explicit Tool RAG configuration.
It still describes vector-store discovery and documents the removed
mockerparameter.Proposed fix
- """Test that tools are used when no_tools=False and vector stores are available. + """Test that tools are used when Tool RAG is explicitly configured. - - Tool preparation logic retrieves available tools + - Tool preparation resolves IDs configured in rag.tool ... - - Integration between query handler, vector stores, and tool preparation + - Integration between the query handler and configured tool preparation ... - mocker: pytest-mock fixtureAs per coding guidelines, function docstrings must accurately document applicable parameters. <coding_guidelines>
🤖 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 `@tests/integration/endpoints/test_query_integration.py` around lines 686 - 703, Update the affected integration test’s docstring to describe explicit Tool RAG configuration rather than vector-store discovery, and remove the obsolete mocker parameter entry while retaining documentation for the parameters still accepted by the test.Source: Coding guidelines
src/utils/responses.py (1)
228-240: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the repository-required
Parameters:docstring header.Proposed fix
- Args: + Parameters:Based on learnings, this repository requires
Parameters:rather thanArgs:. <retrieved_learnings>🤖 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/responses.py` around lines 228 - 240, Update the docstring for the tool-preparation function containing the vector_store_ids, no_tools, token, mcp_headers, and request_headers descriptions by replacing the Args: header with the repository-required Parameters: header, leaving the parameter descriptions unchanged.Sources: Coding guidelines, Learnings
♻️ Duplicate comments (1)
tests/integration/container_lifecycle/test_container_lifecycle.py (1)
70-79: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake teardown cleanup robust and honor the configured image tag.
The cleanup sequence currently hardcodes the image tag (
lightspeed-llama-stack:local) instead of respecting the documentedLLAMA_STACK_IMAGEconfiguration. Additionally,subprocess.runcan raisesubprocess.TimeoutExpired(since a timeout is provided) orOSError. If left unhandled, these exceptions will abort the pytest session teardown. Furthermore, non-zero exit codes are silently ignored becausecapture_output=Trueis used without checking the return code.Wrap the calls in
try...except, useos.environ.get()for the image tag, and log failures at the warning level to ensure best-effort cleanup.🛠️ Proposed fix
Make sure
import osis at the top of the file. Assumingloggeris configured in the module as per coding guidelines, update the fixture as follows:- subprocess.run( - [container_runtime, "rmi", "-f", "lightspeed-llama-stack:local"], - capture_output=True, - timeout=IMAGE_CLEANUP_TIMEOUT, - ) - subprocess.run( - [container_runtime, "image", "prune", "-f"], - capture_output=True, - timeout=DANGLING_IMAGES_CLEANUP_TIMEOUT, - ) + image_tag = os.environ.get("LLAMA_STACK_IMAGE", "lightspeed-llama-stack:local") + + try: + res = subprocess.run( + [container_runtime, "rmi", "-f", image_tag], + capture_output=True, + timeout=IMAGE_CLEANUP_TIMEOUT, + text=True, + ) + if res.returncode != 0: + logger.warning(f"Failed to remove image {image_tag}: {res.stderr}") + except Exception as e: + logger.warning(f"Error during image removal for {image_tag}: {e}") + + try: + res = subprocess.run( + [container_runtime, "image", "prune", "-f"], + capture_output=True, + timeout=DANGLING_IMAGES_CLEANUP_TIMEOUT, + text=True, + ) + if res.returncode != 0: + logger.warning(f"Failed to prune dangling images: {res.stderr}") + except Exception as e: + logger.warning(f"Error during dangling image prune: {e}")🤖 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 `@tests/integration/container_lifecycle/test_container_lifecycle.py` around lines 70 - 79, Update the teardown cleanup around the container-runtime calls to use the configured image tag from os.environ.get("LLAMA_STACK_IMAGE") instead of the hardcoded value. Wrap both subprocess.run calls in handling for subprocess.TimeoutExpired and OSError, check non-zero return codes, and log all cleanup failures at warning level through the module logger so teardown remains best-effort; add the os import if needed.Source: Coding guidelines
🤖 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/utils/responses.py`:
- Around line 228-240: Update the docstring for the tool-preparation function
containing the vector_store_ids, no_tools, token, mcp_headers, and
request_headers descriptions by replacing the Args: header with the
repository-required Parameters: header, leaving the parameter descriptions
unchanged.
In `@tests/integration/endpoints/test_query_integration.py`:
- Around line 686-703: Update the affected integration test’s docstring to
describe explicit Tool RAG configuration rather than vector-store discovery, and
remove the obsolete mocker parameter entry while retaining documentation for the
parameters still accepted by the test.
---
Duplicate comments:
In `@tests/integration/container_lifecycle/test_container_lifecycle.py`:
- Around line 70-79: Update the teardown cleanup around the container-runtime
calls to use the configured image tag from os.environ.get("LLAMA_STACK_IMAGE")
instead of the hardcoded value. Wrap both subprocess.run calls in handling for
subprocess.TimeoutExpired and OSError, check non-zero return codes, and log all
cleanup failures at warning level through the module logger so teardown remains
best-effort; add the os import if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c680109-bd7e-4ea9-ae1b-541f11fa015b
📒 Files selected for processing (14)
docs/devel_doc/openapi.jsondocs/devel_doc/openapi.mddocs/models/responses_succ.mddocs/user_doc/byok_guide.mddocs/user_doc/config.mdexamples/lightspeed-stack-byok-okp-rag.yamlsrc/models/config.pysrc/utils/responses.pytests/integration/container_lifecycle/test_container_lifecycle.pytests/integration/endpoints/test_query_integration.pytests/integration/endpoints/test_responses_byok_integration.pytests/integration/endpoints/test_responses_integration.pytests/unit/app/endpoints/test_responses.pytests/unit/utils/test_responses.py
💤 Files with no reviewable changes (3)
- tests/integration/endpoints/test_responses_byok_integration.py
- tests/integration/endpoints/test_responses_integration.py
- tests/unit/app/endpoints/test_responses.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: unit_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / ci / group 3
- 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: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (8)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.
Files:
docs/user_doc/byok_guide.mddocs/devel_doc/openapi.jsondocs/models/responses_succ.mdexamples/lightspeed-stack-byok-okp-rag.yamlsrc/models/config.pytests/integration/container_lifecycle/test_container_lifecycle.pydocs/devel_doc/openapi.mddocs/user_doc/config.mdtests/integration/endpoints/test_query_integration.pysrc/utils/responses.pytests/unit/utils/test_responses.py
**/*.{py,yaml,yml,json,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Never commit secrets or keys; use environment variables for sensitive data.
Files:
docs/devel_doc/openapi.jsonexamples/lightspeed-stack-byok-okp-rag.yamlsrc/models/config.pytests/integration/container_lifecycle/test_container_lifecycle.pytests/integration/endpoints/test_query_integration.pysrc/utils/responses.pytests/unit/utils/test_responses.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal Python modules.
Start every module with a descriptive docstring and uselogger = get_logger(__name__)for module logging.
Define shared constants in the centralconstants.pymodule, use descriptive comments, and annotate constants withFinal[type].
Use complete type annotations for function parameters and return values, modern union syntax, andtyping_extensions.Selffor model validators.
Document all modules, classes, and functions using Google-style Python docstrings, including applicable Parameters, Returns, Raises, and Attributes sections.
Use descriptive snake_case, action-oriented function names such asget_,validate_, andcheck_.
Avoid modifying input parameters in place; return a newly created data structure instead.
Useasync deffor I/O operations and external API calls.
Use standard logger levels appropriately: debug for diagnostics, info for normal execution, warning for unexpected conditions, and error for serious failures.
Name classes with PascalCase and use descriptive suffixes such asConfiguration,Error/Exception,Resolver, andInterfacewhere applicable.
Use ABC and@abstractmethodfor abstract interfaces; provide complete, specific type annotations for class attributes and avoidAny.
Files:
src/models/config.pytests/integration/container_lifecycle/test_container_lifecycle.pytests/integration/endpoints/test_query_integration.pysrc/utils/responses.pytests/unit/utils/test_responses.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
ConfigurationBasefor configuration Pydantic models andBaseModelfor data models; use@model_validatorand@field_validatorfor validation.
Files:
src/models/config.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Configuration models must extend
ConfigurationBase, whoseextra="forbid"behavior rejects unknown fields; use types such asOptional[FilePath],PositiveInt, andSecretStrwhere appropriate.
Files:
src/models/config.pysrc/utils/responses.py
tests/integration/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for all integration tests; do not use unittest.
Files:
tests/integration/container_lifecycle/test_container_lifecycle.pytests/integration/endpoints/test_query_integration.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain at least 60% unit-test coverage and 10% integration-test coverage.
Files:
tests/integration/container_lifecycle/test_container_lifecycle.pytests/integration/endpoints/test_query_integration.pytests/unit/utils/test_responses.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for unit tests, shared fixtures in
conftest.py,pytest-mockfor mocks, andpytest.mark.asynciofor asynchronous tests; do not use unittest.
Files:
tests/unit/utils/test_responses.py
🧠 Learnings (6)
📚 Learning: 2026-05-20T08:09:30.641Z
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:30.641Z
Learning: In Llama-stack config YAMLs, when defining a Llama Guard safety shield entry, set `provider_shield_id` to the *guard model identifier* (e.g., `meta-llama/Llama-Guard-3-8B`). Do not use a chat/generative model id (e.g., `openai/gpt-4o-mini`): a chat-model id (or `native_override`) indicates only an override landed and does **not** mean the safety shield is actually gating queries. Ensure any E2E coverage for the related implementation (JIRA/E2E tests) exercises a real Llama Guard model to verify that the shield is effective.
Applied to files:
examples/lightspeed-stack-byok-okp-rag.yaml
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/config.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/models/config.pytests/integration/container_lifecycle/test_container_lifecycle.pytests/integration/endpoints/test_query_integration.pysrc/utils/responses.pytests/unit/utils/test_responses.py
📚 Learning: 2026-07-06T15:26:18.398Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 2071
File: src/models/config.py:2416-2422
Timestamp: 2026-07-06T15:26:18.398Z
Learning: In this repo’s Python code under src/**, don’t treat differences in string concatenation style as a style inconsistency when Black has effectively forced (or made clearer) use of explicit `+` string concatenation in multi-line logger/string expressions. If adjacent-literal implicit concatenation is avoided/changed specifically to accommodate Black’s formatting in these call sites, accept the `+` usage and don’t recommend converting it solely for consistency with nearby blocks that use implicit concatenation.
Applied to files:
src/models/config.pysrc/utils/responses.py
📚 Learning: 2026-02-23T14:56:59.186Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1198
File: src/utils/responses.py:184-192
Timestamp: 2026-02-23T14:56:59.186Z
Learning: In the lightspeed-stack codebase (lightspeed-core/lightspeed-stack), do not enforce de-duplication of duplicate client.models.list() calls in model selection flows (e.g., in src/utils/responses.py prepare_responses_params). These calls are considered relatively cheap and removing duplicates could add unnecessary complexity to the flow. Apply this guideline specifically to this file/context unless similar performance characteristics and design decisions are documented elsewhere.
Applied to files:
src/utils/responses.py
🪛 ast-grep (0.44.1)
tests/integration/container_lifecycle/test_container_lifecycle.py
[error] 69-73: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "rmi", "-f", "lightspeed-llama-stack:local"],
capture_output=True,
timeout=IMAGE_CLEANUP_TIMEOUT,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 74-78: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "image", "prune", "-f"],
capture_output=True,
timeout=DANGLING_IMAGES_CLEANUP_TIMEOUT,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
tests/unit/utils/test_responses.py
[warning] 1613-1613: Do not make http calls without encryption
Context: "http://test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🔇 Additional comments (12)
src/models/config.py (1)
2261-2262: LGTM!Also applies to: 2277-2277
docs/models/responses_succ.md (1)
1843-1850: LGTM!docs/devel_doc/openapi.md (1)
8158-8165: LGTM!docs/devel_doc/openapi.json (1)
18087-18093: LGTM!src/utils/responses.py (2)
129-155: Remove the now-unreferenced helper and its dedicated tests.This remains covered by the previous review comment for Lines 129-131.
244-279: LGTM!tests/unit/utils/test_responses.py (2)
3027-3077: Remove these tests withget_vector_store_ids.Their removal is already covered by the previous review comment on the unused helper.
1035-1339: LGTM!Also applies to: 1597-1637, 1702-1841, 3266-3283
docs/user_doc/config.md (1)
609-616: LGTM!docs/user_doc/byok_guide.md (1)
264-264: LGTM!examples/lightspeed-stack-byok-okp-rag.yaml (1)
63-63: LGTM!tests/integration/endpoints/test_query_integration.py (1)
708-729: LGTM!
7d4dca6 to
06cbdd2
Compare
1 participant
Description
Remove fallback to all registered vector stores when tool RAG is unconfigured.
Previously, when neither rag.tool nor rag.inline were configured, prepare_tools would fetch and register all vector stores from llama-stack as a backward-compatibility fallback. This implicit behavior is removed — tool RAG now requires explicit configuration via rag.tool. Updated docs, config model, example YAML, and tests to reflect the new behavior.
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
rag.toolto an empty list or simply omit it.Summary by CodeRabbit
Behavior Changes
rag.toolor request-specific vector stores are explicitly configured.rag.inlineno longer enables or influences tool-based RAG.Documentation
Tests