LCORE-1427: Normalize Tool and Inline RAG default behavior#2146
LCORE-1427: Normalize Tool and Inline RAG default behavior#2146Jazzcort wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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
Request->>prepare_tools: vector_store_ids or None
prepare_tools->>RagConfiguration: read rag.tool when request IDs are absent
prepare_tools-->>Request: file_search tools or None
Possibly related issues
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!
|
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 |
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
cc: @radofuchs |
are-ces
left a comment
are-ces
left a comment
There was a problem hiding this comment.
LGTM.
I had one nit to address, TY!
|
@are-ces @radofuchs Sorry, I forgot to update here. I took out the ad-hoc fix in the github workflow and added a teardown process that prunes all the dangling images after the container lifecycle test. |
|
Since @radofuchs set that up I would like to have him review changes in |
…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.
06cbdd2 to
4230eea
Compare
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/responses.py (1)
199-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Parameters:for docstring arguments.Based on learnings, docstrings in this repository must use the section header
Parameters:rather thanArgs:, even if the project relies on Google-style conventions.♻️ Proposed fix
"""Prepare tools for Responses API including RAG and MCP tools. - Args: + Parameters: + ---------- vector_store_ids: The list of vector store IDs to use for RAG tools or None to fall back to rag.tool configuration no_tools: Whether to skip tool preparation token: Authentication token for MCP tools mcp_headers: Per-request headers for MCP servers request_headers: Incoming HTTP request headers for allowlist propagation - Returns: + Returns: + ------- List of tool configurations, or None if no tools available """🤖 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 199 - 211, Update the docstring for the tool-preparation function containing the vector_store_ids and no_tools parameters by replacing the Args: section header with Parameters:, leaving the documented parameter descriptions and Returns: section unchanged.Source: Learnings
tests/integration/endpoints/test_query_integration.py (1)
678-711: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up unused argument suppression and outdated docstring.
The
mockerparameter was removed from the function signature, but its documentation remains in the docstring. Additionally, instead of disabling the pylint unused-argument warning at the function level, you can explicitly assign the unusedmock_llama_stack_clientfixture to_, which better aligns with the existing pattern forpatch_db_sessionandtest_configin this file.♻️ Proposed fix
-async def test_query_v2_endpoint_uses_tools_when_available( # pylint: disable=unused-argument +async def test_query_v2_endpoint_uses_tools_when_available( 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 used when no_tools=False and vector stores are available. This integration test verifies: - Tool preparation logic retrieves available tools - Tools are passed to Llama Stack when available - Response succeeds with tools enabled - Integration between query handler, vector stores, and tool preparation Parameters: ---------- test_config: Test configuration mock_llama_stack_client: Mocked Llama Stack client mock_query_agent: Mocked Pydantic AI agent for build_agent/agent.run test_request: FastAPI request test_auth: noop authentication tuple patch_db_session: Test database session - mocker: pytest-mock fixture Returns: ------- None """ # prepare_tools does not require llama-stack client anymore so the way to # enable RAG tools is through config test_config.rag.tool = ["vs-test-123"] _ = patch_db_session + _ = mock_llama_stack_client🤖 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 678 - 711, Clean up test_query_v2_endpoint_uses_tools_when_available by removing the outdated mocker entry from its docstring and deleting the function-level pylint unused-argument suppression. Explicitly assign mock_llama_stack_client to `_`, matching the existing handling of other intentionally unused fixtures.
🤖 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 60-92: Move the session-scoped autouse fixture
cleanup_container_artifacts from the test module into the applicable
integration-test conftest.py so it is registered for all integration test runs.
Preserve its container_runtime dependency, teardown cleanup commands, timeouts,
and warning behavior unchanged.
- Around line 74-81: Add check=True to both cleanup subprocess.run calls in
tests/integration/container_lifecycle/test_container_lifecycle.py at lines 74-81
(the rmi cleanup) and 83-90 (the image prune cleanup), so non-zero exits raise
CalledProcessError and are handled by the existing warnings.warn logic.
---
Outside diff comments:
In `@src/utils/responses.py`:
- Around line 199-211: Update the docstring for the tool-preparation function
containing the vector_store_ids and no_tools parameters by replacing the Args:
section header with Parameters:, leaving the documented parameter descriptions
and Returns: section unchanged.
In `@tests/integration/endpoints/test_query_integration.py`:
- Around line 678-711: Clean up test_query_v2_endpoint_uses_tools_when_available
by removing the outdated mocker entry from its docstring and deleting the
function-level pylint unused-argument suppression. Explicitly assign
mock_llama_stack_client to `_`, matching the existing handling of other
intentionally unused fixtures.
🪄 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: faec3f31-7171-4f8c-9fe1-02b7a472199d
📒 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_integration.py
- tests/integration/endpoints/test_responses_byok_integration.py
- tests/unit/app/endpoints/test_responses.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Pyright
- GitHub Check: build-pr
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
🧰 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.Never commit secrets or keys; use environment variables for sensitive data.
Files:
examples/lightspeed-stack-byok-okp-rag.yamldocs/devel_doc/openapi.jsondocs/models/responses_succ.mddocs/user_doc/byok_guide.mddocs/devel_doc/openapi.mdsrc/models/config.pytests/integration/container_lifecycle/test_container_lifecycle.pytests/integration/endpoints/test_query_integration.pysrc/utils/responses.pydocs/user_doc/config.mdtests/unit/utils/test_responses.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal Python modules, complete type annotations, descriptive snake_case names, and module docstrings.
Every module, class, and function must have a descriptive Google-style docstring; document parameters, returns, raises, and class attributes where applicable.
Uselogger = get_logger(__name__)fromlog.pyfor module logging and apply debug, info, warning, and error levels appropriately.
UseFinal[type]annotations for constants and define shared constants inconstants.pywith descriptive comments.
Useasync deffor I/O operations and external API calls, and avoid modifying mutable parameters in place; return newly constructed data instead.
Use modern union syntax such asstr | int,Optional[Type]for optional values, andtyping_extensions.Selffor model validators.
All configuration must use Pydantic models extendingConfigurationBase; configuration must reject unknown fields withextra="forbid".
Use@field_validatorand@model_validatorfor custom Pydantic validation; use specific types such asOptional[FilePath],PositiveInt, andSecretStr.
Classes require descriptive docstrings, PascalCase names, complete typed attributes, and specific types instead ofAny.
UseABCand@abstractmethodfor abstract interfaces; use standard suffixes such asConfiguration,Error/Exception,Resolver, andInterfacewhere applicable.
Files:
src/models/config.pysrc/utils/responses.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic configuration models extend
ConfigurationBase, while data models extendBaseModel; use Pydantic validators for validation.
Files:
src/models/config.py
tests/integration/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/integration/**/*.py: Use pytest for all integration tests and place shared fixtures inconftest.py.
Maintain at least 10% integration-test coverage.
Files:
tests/integration/container_lifecycle/test_container_lifecycle.pytests/integration/endpoints/test_query_integration.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/unit/**/*.py: Use pytest for unit tests, shared fixtures inconftest.py,pytest-mockfor mocks, andpytest.mark.asynciofor async tests.
Maintain at least 60% unit-test coverage; add unit tests for new functionality.
Files:
tests/unit/utils/test_responses.py
🧠 Learnings (7)
📚 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-07-17T19:25:05.325Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 2166
File: src/utils/saved_prompts.py:129-157
Timestamp: 2026-07-17T19:25:05.325Z
Learning: For any endpoint that handles saved prompts and calls `src/utils/saved_prompts.py::create_saved_prompt`, treat the endpoint as the validation boundary. Before calling `create_saved_prompt`, validate the incoming saved-prompt name and content, specifically using `validate_saved_prompt_name` and then persist (store) the normalized value it returns. Do not call `create_saved_prompt` with unvalidated/raw name/content.
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] 74-78: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "rmi", "-f", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME],
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] 83-87: 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] 1612-1612: 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-2262: LGTM!Also applies to: 2277-2277
docs/devel_doc/openapi.json (1)
18087-18093: LGTM!docs/devel_doc/openapi.md (1)
8158-8165: LGTM!docs/models/responses_succ.md (1)
1843-1850: LGTM!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/unit/utils/test_responses.py (1)
78-81: LGTM!Also applies to: 1033-1037, 1043-1340, 1597-1636, 1702-1741, 1747-1770, 1782-1792, 1803-1811, 1827-1840, 3209-3310
tests/integration/container_lifecycle/test_container_lifecycle.py (2)
31-31: Honor the configured image tag during cleanup.The repository documents
LLAMA_STACK_IMAGEas customizable, but this constant hardcodeslightspeed-llama-stack:local. It should read from the environment to respect customized tags during cleanup and testing.🛠️ Proposed fix
-DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME = "lightspeed-llama-stack:local" +import os + +DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME = os.getenv("LLAMA_STACK_IMAGE", "lightspeed-llama-stack:local")
74-81: 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.
| @pytest.fixture(scope="session", autouse=True) | ||
| def cleanup_container_artifacts(container_runtime: str) -> Generator[None]: | ||
| """Remove container images and dangling layers after all tests complete. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| container_runtime (str): Container runtime to use. | ||
|
|
||
| Yields | ||
| ------ | ||
| None | ||
| """ | ||
| yield | ||
|
|
||
| try: | ||
| subprocess.run( | ||
| [container_runtime, "rmi", "-f", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME], | ||
| capture_output=True, | ||
| timeout=IMAGE_CLEANUP_TIMEOUT, | ||
| ) | ||
| except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: | ||
| warnings.warn(f"Image cleanup failed: {e}") | ||
|
|
||
| try: | ||
| subprocess.run( | ||
| [container_runtime, "image", "prune", "-f"], | ||
| capture_output=True, | ||
| timeout=DANGLING_IMAGES_CLEANUP_TIMEOUT, | ||
| ) | ||
| except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: | ||
| warnings.warn(f"Dangling image cleanup failed: {e}") | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move session-scoped autouse fixture to conftest.py.
Defining a scope="session", autouse=True fixture in a specific test file is an anti-pattern. It will only execute if this specific test file is discovered and included in the test run. If developers run other integration tests in isolation (e.g., pytest tests/integration/endpoints/), this cleanup fixture will not be registered or run. As per coding guidelines, shared fixtures must be placed in conftest.py.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 74-78: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "rmi", "-f", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME],
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] 83-87: 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)
🤖 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 60 - 92, Move the session-scoped autouse fixture
cleanup_container_artifacts from the test module into the applicable
integration-test conftest.py so it is registered for all integration test runs.
Preserve its container_runtime dependency, teardown cleanup commands, timeouts,
and warning behavior unchanged.
Source: Coding guidelines
| try: | ||
| subprocess.run( | ||
| [container_runtime, "rmi", "-f", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME], | ||
| capture_output=True, | ||
| timeout=IMAGE_CLEANUP_TIMEOUT, | ||
| ) | ||
| except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: | ||
| warnings.warn(f"Image cleanup failed: {e}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing check=True suppresses CalledProcessError for cleanup commands.
Both cleanup steps attempt to catch subprocess.CalledProcessError to log a warning on failure, but subprocess.run will never raise this exception unless check=True is passed. Without it, failed commands silently return a non-zero exit code and the warnings are never triggered.
tests/integration/container_lifecycle/test_container_lifecycle.py#L74-L81: Addcheck=Trueto thermisubprocess call.tests/integration/container_lifecycle/test_container_lifecycle.py#L83-L90: Addcheck=Trueto theimage prunesubprocess call.
🛠️ Proposed fix
try:
subprocess.run(
[container_runtime, "rmi", "-f", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME],
capture_output=True,
timeout=IMAGE_CLEANUP_TIMEOUT,
+ check=True,
)
except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
warnings.warn(f"Image cleanup failed: {e}")
try:
subprocess.run(
[container_runtime, "image", "pr prune", "-f"],
capture_output=True,
timeout=DANGLING_IMAGES_CLEANUP_TIMEOUT,
+ check=True,
)
except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
warnings.warn(f"Dangling image cleanup failed: {e}")🧰 Tools
🪛 ast-grep (0.44.1)
[error] 74-78: Command coming from incoming request
Context: subprocess.run(
[container_runtime, "rmi", "-f", DEFAULT_LIGHTSPEED_LLAMA_STACK_IMAGE_NAME],
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)
📍 Affects 1 file
tests/integration/container_lifecycle/test_container_lifecycle.py#L74-L81(this comment)tests/integration/container_lifecycle/test_container_lifecycle.py#L83-L90
🤖 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 74 - 81, Add check=True to both cleanup subprocess.run calls in
tests/integration/container_lifecycle/test_container_lifecycle.py at lines 74-81
(the rmi cleanup) and 83-90 (the image prune cleanup), so non-zero exits raise
CalledProcessError and are handled by the existing warnings.warn logic.
2 participants
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
inlineandtoolstart as empty).rag.tool(or per-request vector-store IDs) is explicitly provided.rag.toolis omitted, tool-basedfile_searchRAG is disabled (no automatic inclusion of all BYOK stores).rag.tool.Documentation
Tests