LCORE-1426: refactor RAG configuration into unified 'rag' section [Firefly test] - #2298
LCORE-1426: refactor RAG configuration into unified 'rag' section [Firefly test]#2298are-ces wants to merge 1 commit into
Conversation
Unify all RAG-related configuration under a single 'rag' section in the service configuration model, improving consistency and discoverability. Changes: - Introduce RagConfiguration, RagRetrievalConfiguration, and RagByokConfiguration models under a top-level 'rag' config section - Migrate byok_rag → rag.byok.stores with full backward compatibility - Consolidate rag_inline/rag_tool into rag.retrieval with backward compat - Move MAX_RAG_CHUNKS / MAX_RAG_CHUNKS_TOOL constants into configurable rag.retrieval.max_chunks_inline / max_chunks_tool fields - Rename rag_type → backend with deprecation warning for old field name - Add comprehensive deprecation warnings for all legacy field access - Preserve full backward compatibility: old YAML configs continue to work - Update vector_search, responses, client, rags endpoint, and llama_stack_configuration to use new unified accessors - Add 1213-line dedicated refactoring test suite + update existing tests Three-Layer Defense: - Layer 1: 332/332 unit tests passed, 78% coverage - Layer 2: 49/49 blind exam tests passed (Round 1) - Layer 3: 0 CRITICAL, 0 HIGH, 2 MEDIUM, 2 LOW (all resolved) Spec: LCORE-1426 — BYOK Config Refactoring Harness: Three-Layer Defense verified
WalkthroughThe PR restructures RAG configuration into nested BYOK, OKP, inline, and tool sections. It adds backend compatibility and deprecated-field migration, updates runtime consumers and chunk limits, and expands configuration and retrieval tests. It also documents Lola Skills in ChangesRAG configuration refactor
Lola Skills documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Request
participant Responses
participant VectorSearch
participant RAGStores
Request->>Responses: submit retrieval request
Responses->>VectorSearch: resolve configured sources and limits
VectorSearch->>RAGStores: fetch BYOK, OKP, or inline results
RAGStores-->>VectorSearch: return bounded results
VectorSearch-->>Responses: return merged context
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
are-ces
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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 `@AGENTS.md`:
- Line 416: Update each skill heading in AGENTS.md, including the headings at
the referenced locations, to be followed by exactly one blank line before its
content. Preserve the heading text and all surrounding content while resolving
the markdownlint MD022 violations.
- Line 459: Update the harness-cli link in the AI Context Module description to
use the repository’s canonical URL, or remove the link if no canonical URL is
available; eliminate the placeholder “your-org” URL while preserving the
surrounding description.
In `@src/configuration.py`:
- Around line 538-541: Add a Google-style Raises section to the okp property
docstring, documenting that it raises LogicError when _configuration is not
loaded. Leave the existing behavior and return documentation unchanged.
In `@src/llama_stack_configuration.py`:
- Around line 518-543: Update _extract_rag_source_ids to merge nested
retrieval.inline.sources and retrieval.tool.sources with the deprecated
top-level inline and tool lists when both formats are present. Preserve support
for either format, combine each corresponding pair before returning, and avoid
discarding flat IDs when retrieval is a dictionary.
In `@src/models/config.py`:
- Around line 2814-2831: Update the migration validators in src/models/config.py
at lines 2814-2831 and 3280-3293: in the inline/tool migration block, raise
ValueError when the deprecated field and its corresponding retrieval.*.sources
field are both set, and document this in the method’s Raises section; in the
top-level okp migration block, raise ValueError when okp is set alongside
non-empty self.rag.okp.model_fields_set instead of silently discarding the
top-level values.
- Around line 3163-3167: Mark both deprecated root fields, byok_rag and okp,
with exclude=True in their Field declarations so serialized configuration
exposes only rag.byok.stores. Update the affected dump expectations in
test_dump_configuration.py to remove the duplicated root BYOK entries and retain
the canonical structure.
- Around line 3413-3415: Align the OKP detection in the surrounding
configuration logic with the comment: update the has_okp assignment to consider
both inline and tool RAG strategy sources when tool sources are intended to
count. Otherwise revise the comment to describe the inline-only behavior,
keeping the chosen intent consistent with has_okp.
- Around line 2216-2219: Annotate the module-level constant _BACKEND_TO_PREFIX
with Final[dict[str, str]], and import Final from typing if it is not already
available. Leave the mapping contents unchanged.
In `@src/utils/responses.py`:
- Around line 220-229: Update the vector_store_ids parameter description in the
surrounding response utility docstring to replace the stale rag.tool fallback
reference with rag.retrieval.tool.sources, matching the configuration path used
by the tool_sources assignment and resolution logic.
In `@src/utils/vector_search.py`:
- Around line 477-481: In the vector-store ID filtering logic, build the lookup
set from inline_sources once before the comprehension, then reuse it in the
membership check. Update the branch assigning rag_ids_to_query while preserving
the existing filtering behavior and handling for vector_store_ids being None.
In `@tests/unit/models/config/test_byok_rag.py`:
- Around line 318-333: Add tests alongside test_byok_rag_legacy_rag_type_faiss
covering empty values for both ByokRag.backend and ByokRag.rag_type. Each test
should construct ByokRag with the respective field set to an empty string and
assert pytest.raises(ValidationError) matching “String should have at least 1
character,” while retaining the required existing fields.
In `@tests/unit/models/config/test_dump_configuration.py`:
- Around line 41-62: Update _DEFAULT_RAG_DUMP to use a Final[dict[str, Any]]
annotation and replace each hardcoded chunk limit with the corresponding
constants.BYOK_RAG_MAX_CHUNKS, constants.OKP_RAG_MAX_CHUNKS,
constants.INLINE_RAG_MAX_CHUNKS, and constants.TOOL_RAG_MAX_CHUNKS values.
Import Final from typing while preserving the dictionary structure.
In `@tests/unit/models/config/test_rag_config_refactoring.py`:
- Line 448: Move the repeated warnings and pydantic.ValidationError imports to
the module-level imports in test_rag_config_refactoring.py, then remove all
corresponding in-function imports from the listed test bodies while preserving
their usage.
- Around line 561-568: Update the pytest.warns matchers in the deprecation tests
around _make_config to use distinctive, exact wording from the migration
validators in src/models/config.py rather than broad substrings such as “inline”
or “tool”; apply the same tightening to the additional case at lines 574-581
while preserving the existing warning assertions.
- Around line 22-36: Update the _reset_app_config fixture to remove both broad
try/except Exception blocks and directly reset AppConfig()._configuration and
AppConfig()._quota_limiters before and after yield, allowing reset failures to
propagate.
In `@tests/unit/models/config/test_rag_configuration.py`:
- Around line 163-193: Add a test alongside the deprecated-field migration tests
that constructs RagConfiguration with both inline and retrieval.inline.sources
set, then assert the chosen conflict behavior—raising a validation error or
preserving the canonical retrieval.inline.sources value. Use the existing
warning capture and assertions as appropriate, and ensure the test specifically
prevents silent overwriting of the canonical value.
🪄 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 Plus
Run ID: d6a17eda-d63a-45ca-9c7b-5ec8606e388c
📒 Files selected for processing (15)
AGENTS.mdsrc/app/endpoints/rags.pysrc/client.pysrc/configuration.pysrc/constants.pysrc/llama_stack_configuration.pysrc/models/config.pysrc/utils/responses.pysrc/utils/vector_search.pytests/unit/models/config/test_byok_rag.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_rag_config_refactoring.pytests/unit/models/config/test_rag_configuration.pytests/unit/utils/test_responses.pytests/unit/utils/test_vector_search.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-7-on-pull-request
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: integration_tests (3.12)
- GitHub Check: Pylinter
- GitHub Check: integration_tests (3.13)
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: build-pr
⚠️ CI failures not shown inline (6)
GitHub Actions: OpenAPI (Spectral) / spectral: LCORE-1426: refactor RAG configuration into unified 'rag' section
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/devel_doc/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/devel_doc/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/devel_doc/openapi.json"�[0m
GitHub Actions: OpenAPI (Spectral) / 0_spectral.txt: LCORE-1426: refactor RAG configuration into unified 'rag' section
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/devel_doc/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/devel_doc/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/devel_doc/openapi.json"�[0m
GitHub Actions: Unit tests / 1_unit_tests (3.12).txt: LCORE-1426: refactor RAG configuration into unified 'rag' section
Conclusion: failure
##[group]Run uv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing
�[36;1muv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing�[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]
Uninstalled 1 package in 4ms
Installed 1 package in 14ms
============================= test session starts ==============================
platform linux -- Python 3.12.3, pytest-9.1.1, pluggy-1.6.0
benchmark: 5.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/runner/work/lightspeed-stack/lightspeed-stack
configfile: pyproject.toml
plugins: cov-7.1.0, anyio-4.14.2, logfire-4.39.0, benchmark-5.2.3, order-1.5.0, asyncio-1.4.0, mock-3.15.1
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 3239 items
tests/unit/a2a_storage/test_in_memory_context_store.py ........ [ 0%]
tests/unit/a2a_storage/test_sqlite_context_store.py .......... [ 0%]
tests/unit/a2a_storage/test_storage_factory.py ........... [ 0%]
tests/unit/app/endpoints/test_a2a.py ................................... [ 1%]
...... [ 2%]
tests/unit/app/endpoints/test_authorized.py ... [ 2%]
tests/unit/app/endpoints/test_config.py .. [ 2%]
tests/unit/app/endpoints/test_conversations.py ......................... [ 3%]
................. [ 3%]
tests/unit/app/endpoints/test_conversations_v2.py ...................... [ 4%]
............... [ 4%]
tests/unit/ap...
GitHub Actions: Unit tests / unit_tests (3.12): LCORE-1426: refactor RAG configuration into unified 'rag' section
Conclusion: failure
##[group]Run uv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing
�[36;1muv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing�[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]
Uninstalled 1 package in 4ms
Installed 1 package in 14ms
============================= test session starts ==============================
platform linux -- Python 3.12.3, pytest-9.1.1, pluggy-1.6.0
benchmark: 5.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/runner/work/lightspeed-stack/lightspeed-stack
configfile: pyproject.toml
plugins: cov-7.1.0, anyio-4.14.2, logfire-4.39.0, benchmark-5.2.3, order-1.5.0, asyncio-1.4.0, mock-3.15.1
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 3239 items
tests/unit/a2a_storage/test_in_memory_context_store.py ........ [ 0%]
tests/unit/a2a_storage/test_sqlite_context_store.py .......... [ 0%]
tests/unit/a2a_storage/test_storage_factory.py ........... [ 0%]
tests/unit/app/endpoints/test_a2a.py ................................... [ 1%]
...... [ 2%]
tests/unit/app/endpoints/test_authorized.py ... [ 2%]
tests/unit/app/endpoints/test_config.py .. [ 2%]
tests/unit/app/endpoints/test_conversations.py ......................... [ 3%]
................. [ 3%]
tests/unit/app/endpoints/test_conversations_v2.py ...................... [ 4%]
............... [ 4%]
tests/unit/ap...
GitHub Actions: Unit tests / unit_tests (3.13): LCORE-1426: refactor RAG configuration into unified 'rag' section
Conclusion: failure
##[group]Run uv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing
�[36;1muv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing�[0m
shell: /usr/bin/bash -e {0}
env:
UV_PYTHON: 3.13
VIRTUAL_ENV: /home/runner/work/lightspeed-stack/lightspeed-stack/.venv
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Uninstalled 1 package in 2ms
Installed 1 package in 3ms
============================= test session starts ==============================
platform linux -- Python 3.13.14, pytest-9.1.1, pluggy-1.6.0
benchmark: 5.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/runner/work/lightspeed-stack/lightspeed-stack
configfile: pyproject.toml
plugins: cov-7.1.0, anyio-4.14.2, logfire-4.39.0, benchmark-5.2.3, order-1.5.0, asyncio-1.4.0, mock-3.15.1
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 3239 items
tests/unit/a2a_storage/test_in_memory_context_store.py ........ [ 0%]
tests/unit/a2a_storage/test_sqlite_context_store.py .......... [ 0%]
tests/unit/a2a_storage/test_storage_factory.py ........... [ 0%]
tests/unit/app/endpoints/test_a2a.py ................................... [ 1%]
...... [ 2%]
tests/unit/app/endpoints/test_authorized.py ... [ 2%]
tests/unit/app/endpoints/test_config.py .. [ 2%]
tests/unit/app/endpoints/test_conversations.py ......................... [ 3%]
................. [ 3%]
tests/unit/app/endpoints/test_conversations_v2.py ...................... [ 4%]
............... [ 4%]
tests/unit/ap...
GitHub Actions: Unit tests / 0_unit_tests (3.13).txt: LCORE-1426: refactor RAG configuration into unified 'rag' section
Conclusion: failure
##[group]Run uv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing
�[36;1muv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing�[0m
shell: /usr/bin/bash -e {0}
env:
UV_PYTHON: 3.13
VIRTUAL_ENV: /home/runner/work/lightspeed-stack/lightspeed-stack/.venv
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Uninstalled 1 package in 2ms
Installed 1 package in 3ms
============================= test session starts ==============================
platform linux -- Python 3.13.14, pytest-9.1.1, pluggy-1.6.0
benchmark: 5.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/runner/work/lightspeed-stack/lightspeed-stack
configfile: pyproject.toml
plugins: cov-7.1.0, anyio-4.14.2, logfire-4.39.0, benchmark-5.2.3, order-1.5.0, asyncio-1.4.0, mock-3.15.1
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 3239 items
tests/unit/a2a_storage/test_in_memory_context_store.py ........ [ 0%]
tests/unit/a2a_storage/test_sqlite_context_store.py .......... [ 0%]
tests/unit/a2a_storage/test_storage_factory.py ........... [ 0%]
tests/unit/app/endpoints/test_a2a.py ................................... [ 1%]
...... [ 2%]
tests/unit/app/endpoints/test_authorized.py ... [ 2%]
tests/unit/app/endpoints/test_config.py .. [ 2%]
tests/unit/app/endpoints/test_conversations.py ......................... [ 3%]
................. [ 3%]
tests/unit/app/endpoints/test_conversations_v2.py ...................... [ 4%]
............... [ 4%]
tests/unit/ap...
🧰 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.
**/*: Never commit secrets or keys; use environment variables for sensitive data.
Checkpyproject.tomlfor existing dependencies and current versions before adding dependencies or relying on assumed versions.
Files:
src/app/endpoints/rags.pyAGENTS.mdsrc/client.pysrc/constants.pysrc/utils/responses.pysrc/utils/vector_search.pysrc/configuration.pytests/unit/utils/test_responses.pytests/unit/utils/test_vector_search.pytests/unit/models/config/test_byok_rag.pytests/unit/models/config/test_rag_configuration.pysrc/llama_stack_configuration.pytests/unit/models/config/test_dump_configuration.pysrc/models/config.pytests/unit/models/config/test_rag_config_refactoring.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal Python modules.
All modules must begin with descriptive docstrings explaining their purpose.
Uselogger = get_logger(__name__)fromlog.pyfor module logging.
UseFinal[type]type hints for all constants.
Use@field_validatorand@model_validatorfor custom Pydantic validation.
Use complete type annotations for function parameters and return types.
Usetyping_extensions.Selffor model validators.
Use modern union syntax such asstr | int; useOptional[Type]for optional values.
Name functions with descriptive, action-oriented snake_case names, such asget_,validate_, orcheck_.
Avoid modifying input parameters in place; return a new data structure instead.
Useasync deffor I/O operations and external API calls.
HandleAPIConnectionErrorfrom Llama Stack.
Usefrom log import get_loggerand the module logger pattern.
Use standard log levels with clear purposes: debug for diagnostics, info for normal execution, warning for unexpected conditions, and error for serious failures.
All classes must have descriptive docstrings and use PascalCase descriptive names with standard suffixes such asConfiguration,Error/Exception,Resolver, andInterface.
Abstract interfaces must useABCwith@abstractmethoddecorators.
Pydantic configuration models extendConfigurationBase, while data models extendBaseModel.
Use complete type annotations for all class attributes and prefer specific types overAny.
Follow Google Python docstring conventions for all modules, classes, and functions, including relevant Parameters, Returns, Raises, and Attributes sections.
Files:
src/app/endpoints/rags.pysrc/client.pysrc/constants.pysrc/utils/responses.pysrc/utils/vector_search.pysrc/configuration.pytests/unit/utils/test_responses.pytests/unit/utils/test_vector_search.pytests/unit/models/config/test_byok_rag.pytests/unit/models/config/test_rag_configuration.pysrc/llama_stack_configuration.pytests/unit/models/config/test_dump_configuration.pysrc/models/config.pytests/unit/models/config/test_rag_config_refactoring.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint errors.
Files:
src/app/endpoints/rags.py
AGENTS.md
📄 CodeRabbit inference engine (CLAUDE.md)
Document agent implementations and their configurations in AGENTS.md
Files:
AGENTS.md
src/constants.py
📄 CodeRabbit inference engine (AGENTS.md)
Define shared constants centrally in
constants.py, with descriptive comments.
Files:
src/constants.py
src/**/config*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/config*.py: Configuration must use Pydantic models extendingConfigurationBase.
Configuration models must reject unknown fields withextra="forbid".
Files:
src/configuration.pysrc/models/config.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/unit/**/*.py: Use pytest for all unit tests; do not use unittest.
Mark asynchronous pytest tests withpytest.mark.asyncio.
Maintain at least 60% unit-test coverage.
Files:
tests/unit/utils/test_responses.pytests/unit/utils/test_vector_search.pytests/unit/models/config/test_byok_rag.pytests/unit/models/config/test_rag_configuration.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_rag_config_refactoring.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
pytest-mockfor AsyncMock-based mocking.
Files:
tests/unit/utils/test_responses.pytests/unit/utils/test_vector_search.pytests/unit/models/config/test_byok_rag.pytests/unit/models/config/test_rag_configuration.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_rag_config_refactoring.py
🧠 Learnings (8)
📚 Learning: 2026-04-06T20:18:07.852Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1463
File: src/app/endpoints/rlsapi_v1.py:266-271
Timestamp: 2026-04-06T20:18:07.852Z
Learning: In the lightspeed-stack codebase, within `src/app/endpoints/` inference/MCP endpoints, treat `tools: Optional[list[Any]]` in MCP tool definitions as an intentional, consistent typing pattern (used across `query`, `responses`, `streaming_query`, `rlsapi_v1`). Do not raise or suggest this as a typing issue during code review; changing it in isolation could break endpoint typing consistency across the codebase.
Applied to files:
src/app/endpoints/rags.py
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
src/app/endpoints/rags.pysrc/client.pysrc/constants.pysrc/utils/responses.pysrc/utils/vector_search.pysrc/configuration.pytests/unit/utils/test_responses.pytests/unit/utils/test_vector_search.pytests/unit/models/config/test_byok_rag.pytests/unit/models/config/test_rag_configuration.pysrc/llama_stack_configuration.pytests/unit/models/config/test_dump_configuration.pysrc/models/config.pytests/unit/models/config/test_rag_config_refactoring.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/app/endpoints/rags.pysrc/client.pysrc/constants.pysrc/utils/responses.pysrc/utils/vector_search.pysrc/configuration.pysrc/llama_stack_configuration.pysrc/models/config.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/app/endpoints/rags.pysrc/client.pysrc/constants.pysrc/utils/responses.pysrc/utils/vector_search.pysrc/configuration.pysrc/llama_stack_configuration.pysrc/models/config.py
📚 Learning: 2026-03-02T15:57:10.746Z
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-stack PR: 1250
File: AGENTS.md:11-12
Timestamp: 2026-03-02T15:57:10.746Z
Learning: In AGENTS.md, Linting Tools section should list actual tool names (e.g., mypy, pylint, pyright, ruff, black) instead of make target names (e.g., check-types). Ensure the section documents the individual tools that perform linting, not the Makefile targets that invoke them, so readers understand which tools are used and how to configure/run them directly.
Applied to files:
AGENTS.md
📚 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-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
🪛 ast-grep (0.45.0)
tests/unit/models/config/test_byok_rag.py
[info] 25-25: Do not hardcode temporary file or directory names
Context: "/tmp/test.faiss"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 58-58: Do not hardcode temporary file or directory names
Context: "/tmp/test.faiss"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 92-92: Do not hardcode temporary file or directory names
Context: "/tmp/test.faiss"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 109-109: Do not hardcode temporary file or directory names
Context: "/tmp/test.faiss"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 117-117: Do not hardcode temporary file or directory names
Context: "/tmp/test.faiss"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
tests/unit/models/config/test_rag_configuration.py
[info] 135-135: Do not hardcode temporary file or directory names
Context: "/tmp/test.faiss"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 219-219: Do not hardcode temporary file or directory names
Context: "/tmp/ocp.faiss"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 252-252: Do not hardcode temporary file or directory names
Context: "/tmp/kb.faiss"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
tests/unit/models/config/test_rag_config_refactoring.py
[warning] 45-45: Do not make http calls without encryption
Context: "http://test.com:1234"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🪛 GitHub Actions: Unit tests / 0_unit_tests (3.13).txt
src/configuration.py
[error] 158-158: AppConfig.configuration raises LogicError: configuration is not loaded.
🪛 GitHub Actions: Unit tests / unit_tests (3.13)
src/configuration.py
[error] 158-158: pytest failed in tests/unit/test_client.py::test_get_async_llama_stack_library_client and ::test_reload_library_client. AppConfig.configuration raised LogicError: configuration is not loaded while loading the library client.
[error] 158-158: The command 'uv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing' failed because AppConfig.configuration was accessed before configuration was loaded.
🪛 markdownlint-cli2 (0.23.1)
AGENTS.md
[warning] 416-416: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 420-420: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 424-424: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 428-428: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 432-432: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 436-436: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 440-440: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 444-444: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 453-453: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
[warning] 453-453: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
🔇 Additional comments (19)
AGENTS.md (2)
465-465: 📐 Maintainability & Code QualityNo change needed.
AGENTS.mdis at the repository root, and both./README.mdand./SKILLS_QUICKSTART.mdresolve to files present at the expected relative locations.
452-455: 📐 Maintainability & Code QualityKeep the Lola module metadata machine-readable.
This AGENTS.md entry wraps
name/descriptionin Markdown-delimited YAML frontmatter. This may be parsed as Markdown firstline/setext structure instead of Lola metadata. Use the syntax accepted by the Lola loader, or add a narrow lint exception that preserves machine-readability.tests/unit/models/config/test_dump_configuration.py (2)
1379-1416: This expectation encodes the BYOK store twice, under bothbyok_ragandrag.byok.stores. The root cause is flagged onsrc/models/config.pyLines 3163-3167.
222-222: LGTM!Also applies to: 243-244, 442-442, 463-464, 798-798, 834-835, 1053-1053, 1089-1090, 1347-1347, 1609-1609, 1630-1631, 1984-1984, 2005-2006, 2205-2205, 2226-2227, 2426-2426, 2447-2448, 2654-2654, 2675-2676
src/models/config.py (3)
2049-2063: LGTM!
2600-2697: LGTM!Also applies to: 2736-2741
2153-2180: 🩺 Stability & AvailabilityNo change needed.
ConfigurationBase.model_configonly setsextra="forbid"; plain attribute assignment in these after-validators is safe here.src/constants.py (1)
221-233: LGTM!tests/unit/models/config/test_rag_configuration.py (1)
23-145: LGTM!Also applies to: 195-273, 284-305
tests/unit/models/config/test_byok_rag.py (1)
17-121: LGTM!Also applies to: 124-162, 165-316, 336-361
tests/unit/utils/test_responses.py (1)
1714-1719: LGTM!Also applies to: 1737-1742, 1762-1770, 1791-1796, 1813-1818, 1835-1840, 1857-1861, 3290-3290, 3308-3308, 3338-3338, 3383-3383
tests/unit/utils/test_vector_search.py (1)
521-522: LGTM!Also applies to: 540-542, 580-582, 621-626, 667-668, 687-689, 728-728, 794-800, 819-825, 861-867, 917-923
src/configuration.py (2)
21-21: LGTM!Also applies to: 586-599, 614-617, 619-632
557-571: LGTM!src/llama_stack_configuration.py (1)
472-493: LGTM!Also applies to: 496-515, 860-860, 1233-1238, 1407-1413
src/app/endpoints/rags.py (1)
180-180: LGTM!src/utils/responses.py (1)
667-667: LGTM!Also applies to: 1700-1700, 1789-1789
src/utils/vector_search.py (1)
464-468: LGTM!Also applies to: 484-486, 574-574, 643-644, 658-665
src/client.py (1)
145-151: 🗄️ Data Integrity & IntegrationNo change needed.
RagConfiguration.retrievalserializes asretrieval, and nestedRetrievalConfiguration.inline/.toolpreserve thesourceskey, so_extract_rag_source_ids()can read the canonical paths.
|
|
||
| ### ixd-firefly-custom-skills | ||
|
|
||
| #### api-spec-validator |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines after each skill heading.
markdownlint reports MD022 because each #### heading is followed immediately by text. Insert one blank line after every skill heading.
Example
#### api-spec-validator
+
**When to use:** Validate API endpoint implementations...Also applies to: 420-420, 424-424, 428-428, 432-432, 436-436, 440-440, 444-444
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 416-416: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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 `@AGENTS.md` at line 416, Update each skill heading in AGENTS.md, including the
headings at the referenced locations, to be followed by exactly one blank line
before its content. Preserve the heading text and all surrounding content while
resolving the markdownlint MD022 violations.
Source: Linters/SAST tools
|
|
||
| # Harness Skills Module | ||
|
|
||
| This AI Context Module provides shared skills for use with [harness-cli](https://github.com/your-org/ixd-firefly-harness-engineering-cli) and [OpenCode](https://opencode.ai). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the placeholder repository URL.
The link contains your-org, so it does not identify the actual repository. Replace it with the canonical URL or remove the link.
🤖 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 `@AGENTS.md` at line 459, Update the harness-cli link in the AI Context Module
description to use the repository’s canonical URL, or remove the link if no
canonical URL is available; eliminate the placeholder “your-org” URL while
preserving the surrounding description.
| """Return OKP configuration from the unified rag section.""" | ||
| if self._configuration is None: | ||
| raise LogicError("logic error: configuration is not loaded") | ||
| return self._configuration.okp | ||
| return self._configuration.rag.okp |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add a Raises section to the okp docstring.
The property raises LogicError when configuration is not loaded. The coding guidelines require a Raises section. The new byok_rag property below already documents this correctly.
📝 Proposed change
- """Return OKP configuration from the unified rag section."""
+ """Return OKP configuration from the unified rag section.
+
+ Returns:
+ OkpConfiguration: The OKP configuration from ``rag.okp``.
+
+ Raises:
+ LogicError: If the configuration has not been loaded.
+ """As per coding guidelines: "Follow Google Python docstring conventions for all modules, classes, and functions, including relevant Parameters, Returns, Raises, and Attributes sections."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Return OKP configuration from the unified rag section.""" | |
| if self._configuration is None: | |
| raise LogicError("logic error: configuration is not loaded") | |
| return self._configuration.okp | |
| return self._configuration.rag.okp | |
| """Return OKP configuration from the unified rag section. | |
| Returns: | |
| OkpConfiguration: The OKP configuration from ``rag.okp``. | |
| Raises: | |
| LogicError: If the configuration has not been loaded. | |
| """ | |
| if self._configuration is None: | |
| raise LogicError("logic error: configuration is not loaded") | |
| return self._configuration.rag.okp |
🤖 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/configuration.py` around lines 538 - 541, Add a Google-style Raises
section to the okp property docstring, documenting that it raises LogicError
when _configuration is not loaded. Leave the existing behavior and return
documentation unchanged.
Source: Coding guidelines
| def _extract_rag_source_ids( | ||
| rag_config: dict[str, Any], | ||
| ) -> tuple[list[str], list[str]]: | ||
| """Extract inline and tool RAG source IDs from either config format. | ||
|
|
||
| Supports both the new ``retrieval.inline.sources`` / ``retrieval.tool.sources`` | ||
| path and the deprecated ``inline`` / ``tool`` keys. | ||
|
|
||
| Parameters: | ||
| rag_config: RAG configuration dict. | ||
|
|
||
| Returns: | ||
| Tuple of (inline_ids, tool_ids). | ||
| """ | ||
| retrieval = rag_config.get("retrieval") | ||
| if isinstance(retrieval, dict): | ||
| inline_cfg = retrieval.get("inline") or {} | ||
| tool_cfg = retrieval.get("tool") or {} | ||
| inline_ids = ( | ||
| inline_cfg.get("sources") or [] if isinstance(inline_cfg, dict) else [] | ||
| ) | ||
| tool_ids = tool_cfg.get("sources") or [] if isinstance(tool_cfg, dict) else [] | ||
| else: | ||
| inline_ids = rag_config.get("inline") or [] | ||
| tool_ids = rag_config.get("tool") or [] | ||
| return inline_ids, tool_ids |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the deprecated rag.inline / rag.tool migration in the Configuration models.
set -euo pipefail
fd -t f 'config.py' src/models --exec rg -n -C15 'inline|tool' -g '*.py' --max-count 200 \
| rg -n -C0 'deprecat|migrate|model_validator|warnings.warn' || true
fd -t f 'config.py' src/models --exec ast-grep run --lang python \
--pattern 'def $NAME(self) -> Self:
$$$'Repository: lightspeed-core/lightspeed-stack
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^src/llama_stack_configuration\.py$|src/models/config\.py$|src/.*)' | sed -n '1,120p'
echo "== target function locations =="
rg -n "_extract_rag_source_ids|generate_configuration|synthesize_configuration|migrate_deprecated_rag_fields|generate.*yaml|run\.yaml" src -g '*.py'
echo "== target function =="
sed -n '480,570p' src/llama_stack_configuration.py
echo "== nearby references =="
sed -n '1080,1180p' src/llama_stack_configuration.py
sed -n '1180,1260p' src/llama_stack_configuration.py
sed -n '1320,1400p' src/llama_stack_configuration.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 22212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target function locations =="
rg -n "_extract_rag_source_ids|generate_configuration|synthesize_configuration|migrate_deprecated_rag_fields|generate.*yaml|run\.yaml" src -g '*.py'
echo "== target function =="
sed -n '480,570p' src/llama_stack_configuration.py
echo "== helpers around run.yaml generation/use =="
sed -n '1080,1180p' src/llama_stack_configuration.py
sed -n '1180,1260p' src/llama_stack_configuration.py
sed -n '1320,1400p' src/llama_stack_configuration.py
echo "== model migration snippet =="
sed -n '2800,2835p' src/models/config.py
sed -n '3255,3295p' src/models/config.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 21521
Merge both RAG source-id formats in _extract_rag_source_ids.
When raw lightspeed-stack.yaml contains a retrieval dict and also deprecated flat rag.inline / rag.tool lists, the current isinstance branch discards the flat lists. This means Solr can be emitted without the OKP inline IDs included. Merge the nested sources values with the deprecated flat lists before returning inline/tool IDs.
🤖 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/llama_stack_configuration.py` around lines 518 - 543, Update
_extract_rag_source_ids to merge nested retrieval.inline.sources and
retrieval.tool.sources with the deprecated top-level inline and tool lists when
both formats are present. Preserve support for either format, combine each
corresponding pair before returning, and avoid discarding flat IDs when
retrieval is a dictionary.
| _BACKEND_TO_PREFIX: dict[str, str] = { | ||
| "faiss": "inline", | ||
| "pgvector": "remote", | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Annotate _BACKEND_TO_PREFIX as Final.
The coding guidelines require Final[type] hints for constants.
♻️ Proposed fix
-_BACKEND_TO_PREFIX: dict[str, str] = {
+_BACKEND_TO_PREFIX: Final[dict[str, str]] = {
"faiss": "inline",
"pgvector": "remote",
}Ensure Final is imported from typing.
As per coding guidelines: "Use Final[type] type hints for all constants."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _BACKEND_TO_PREFIX: dict[str, str] = { | |
| "faiss": "inline", | |
| "pgvector": "remote", | |
| } | |
| _BACKEND_TO_PREFIX: Final[dict[str, str]] = { | |
| "faiss": "inline", | |
| "pgvector": "remote", | |
| } |
🤖 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 2216 - 2219, Annotate the module-level
constant _BACKEND_TO_PREFIX with Final[dict[str, str]], and import Final from
typing if it is not already available. Leave the mapping contents unchanged.
Source: Coding guidelines
| _DEFAULT_RAG_DUMP: dict[str, Any] = { | ||
| "byok": { | ||
| "max_chunks": 10, | ||
| "stores": [], | ||
| }, | ||
| "okp": { | ||
| "rhokp_url": None, | ||
| "offline": True, | ||
| "chunk_filter_query": None, | ||
| "max_chunks": 5, | ||
| }, | ||
| "retrieval": { | ||
| "inline": { | ||
| "sources": [], | ||
| "max_chunks": 10, | ||
| }, | ||
| "tool": { | ||
| "sources": [], | ||
| "max_chunks": 10, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the constants and a Final annotation in _DEFAULT_RAG_DUMP.
The chunk limits are hardcoded here as 10 and 5. The model defaults come from constants.BYOK_RAG_MAX_CHUNKS, constants.OKP_RAG_MAX_CHUNKS, constants.INLINE_RAG_MAX_CHUNKS, and constants.TOOL_RAG_MAX_CHUNKS. If a default changes, this dictionary drifts and the failure points at the wrong place. constants is already imported in this file.
♻️ Proposed fix
-_DEFAULT_RAG_DUMP: dict[str, Any] = {
+_DEFAULT_RAG_DUMP: Final[dict[str, Any]] = {
"byok": {
- "max_chunks": 10,
+ "max_chunks": constants.BYOK_RAG_MAX_CHUNKS,
"stores": [],
},
"okp": {
"rhokp_url": None,
"offline": True,
"chunk_filter_query": None,
- "max_chunks": 5,
+ "max_chunks": constants.OKP_RAG_MAX_CHUNKS,
},
"retrieval": {
"inline": {
"sources": [],
- "max_chunks": 10,
+ "max_chunks": constants.INLINE_RAG_MAX_CHUNKS,
},
"tool": {
"sources": [],
- "max_chunks": 10,
+ "max_chunks": constants.TOOL_RAG_MAX_CHUNKS,
},
},
}Ensure Final is imported from typing.
As per coding guidelines: "Use Final[type] type hints for all constants."
🤖 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/models/config/test_dump_configuration.py` around lines 41 - 62,
Update _DEFAULT_RAG_DUMP to use a Final[dict[str, Any]] annotation and replace
each hardcoded chunk limit with the corresponding constants.BYOK_RAG_MAX_CHUNKS,
constants.OKP_RAG_MAX_CHUNKS, constants.INLINE_RAG_MAX_CHUNKS, and
constants.TOOL_RAG_MAX_CHUNKS values. Import Final from typing while preserving
the dictionary structure.
Source: Coding guidelines
| @pytest.fixture(autouse=True) | ||
| def _reset_app_config() -> Generator[None, None, None]: | ||
| """Reset AppConfig singleton between tests.""" | ||
| # pylint: disable=broad-exception-caught,protected-access | ||
| try: | ||
| AppConfig()._configuration = None # type: ignore[attr-defined] | ||
| AppConfig()._quota_limiters = [] # type: ignore[attr-defined] | ||
| except Exception: | ||
| pass | ||
| yield | ||
| try: | ||
| AppConfig()._configuration = None # type: ignore[attr-defined] | ||
| AppConfig()._quota_limiters = [] # type: ignore[attr-defined] | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the broad exception suppression in the reset fixture.
AppConfig()._configuration = None cannot raise in normal operation. The except Exception: pass blocks therefore only hide genuine failures. If the reset ever fails, configuration state leaks into the next test and produces confusing failures elsewhere.
♻️ Proposed simplification
`@pytest.fixture`(autouse=True)
def _reset_app_config() -> Generator[None, None, None]:
"""Reset AppConfig singleton between tests."""
- # pylint: disable=broad-exception-caught,protected-access
- try:
- AppConfig()._configuration = None # type: ignore[attr-defined]
- AppConfig()._quota_limiters = [] # type: ignore[attr-defined]
- except Exception:
- pass
- yield
- try:
- AppConfig()._configuration = None # type: ignore[attr-defined]
- AppConfig()._quota_limiters = [] # type: ignore[attr-defined]
- except Exception:
- pass
+ # pylint: disable=protected-access
+ config = AppConfig()
+ config._configuration = None # type: ignore[attr-defined]
+ config._quota_limiters = [] # type: ignore[attr-defined]
+ yield
+ config._configuration = None # type: ignore[attr-defined]
+ config._quota_limiters = [] # type: ignore[attr-defined]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.fixture(autouse=True) | |
| def _reset_app_config() -> Generator[None, None, None]: | |
| """Reset AppConfig singleton between tests.""" | |
| # pylint: disable=broad-exception-caught,protected-access | |
| try: | |
| AppConfig()._configuration = None # type: ignore[attr-defined] | |
| AppConfig()._quota_limiters = [] # type: ignore[attr-defined] | |
| except Exception: | |
| pass | |
| yield | |
| try: | |
| AppConfig()._configuration = None # type: ignore[attr-defined] | |
| AppConfig()._quota_limiters = [] # type: ignore[attr-defined] | |
| except Exception: | |
| pass | |
| `@pytest.fixture`(autouse=True) | |
| def _reset_app_config() -> Generator[None, None, None]: | |
| """Reset AppConfig singleton between tests.""" | |
| # pylint: disable=protected-access | |
| config = AppConfig() | |
| config._configuration = None # type: ignore[attr-defined] | |
| config._quota_limiters = [] # type: ignore[attr-defined] | |
| yield | |
| config._configuration = None # type: ignore[attr-defined] | |
| config._quota_limiters = [] # type: ignore[attr-defined] |
🤖 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/models/config/test_rag_config_refactoring.py` around lines 22 -
36, Update the _reset_app_config fixture to remove both broad try/except
Exception blocks and directly reset AppConfig()._configuration and
AppConfig()._quota_limiters before and after yield, allowing reset failures to
propagate.
| tmp_path: Path, | ||
| ) -> None: | ||
| """The old top-level 'byok_rag' key is still accepted.""" | ||
| import warnings |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Move warnings and ValidationError imports to the module top.
import warnings and from pydantic import ValidationError are repeated inside 15 test bodies. Both are already needed across the file. Import them once at module level to reduce duplication.
♻️ Proposed change
+import warnings
from collections.abc import Generator
from pathlib import Path
import pytest
+from pydantic import ValidationError
from configuration import AppConfigThen delete each in-function import warnings and from pydantic import ValidationError line.
Also applies to: 492-492, 527-527, 544-544, 587-587, 746-746, 762-762, 780-780, 806-806, 829-829, 845-845, 1035-1035, 1137-1137, 1164-1164, 1192-1192
🤖 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/models/config/test_rag_config_refactoring.py` at line 448, Move
the repeated warnings and pydantic.ValidationError imports to the module-level
imports in test_rag_config_refactoring.py, then remove all corresponding
in-function imports from the listed test bodies while preserving their usage.
| with pytest.warns(DeprecationWarning, match="inline"): | ||
| _make_config( | ||
| { | ||
| "rag": { | ||
| "inline": ["store-1", "store-2"], | ||
| }, | ||
| } | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Tighten the deprecation warning matchers.
match="inline" and match="tool" are substrings that many messages contain. A rag_type or byok_rag deprecation message could satisfy them and the test would still pass. Match a distinctive part of each expected message instead.
♻️ Proposed change
- with pytest.warns(DeprecationWarning, match="inline"):
+ with pytest.warns(
+ DeprecationWarning, match=r"rag\.retrieval\.inline"
+ ):- with pytest.warns(DeprecationWarning, match="tool"):
+ with pytest.warns(DeprecationWarning, match=r"rag\.retrieval\.tool"):Adjust the patterns to the exact wording emitted by the migration validators in src/models/config.py.
Also applies to: 574-581
🤖 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/models/config/test_rag_config_refactoring.py` around lines 561 -
568, Update the pytest.warns matchers in the deprecation tests around
_make_config to use distinctive, exact wording from the migration validators in
src/models/config.py rather than broad substrings such as “inline” or “tool”;
apply the same tightening to the additional case at lines 574-581 while
preserving the existing warning assertions.
| def test_deprecated_inline_field_migrated(self) -> None: | ||
| """Test that deprecated 'inline' field is migrated to retrieval.inline.sources.""" | ||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| config = RagConfiguration(inline=["store-1", "store-2"]) | ||
| assert config.retrieval.inline.sources == ["store-1", "store-2"] | ||
| assert config.inline is None | ||
| assert len(w) == 1 | ||
| assert "deprecated" in str(w[0].message).lower() | ||
|
|
||
| def test_deprecated_tool_field_migrated(self) -> None: | ||
| """Test that deprecated 'tool' field is migrated to retrieval.tool.sources.""" | ||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| config = RagConfiguration(tool=[constants.OKP_RAG_ID, "store-1"]) | ||
| assert config.retrieval.tool.sources == [constants.OKP_RAG_ID, "store-1"] | ||
| assert config.tool is None | ||
| assert len(w) == 1 | ||
| assert "deprecated" in str(w[0].message).lower() | ||
|
|
||
| def test_deprecated_inline_and_tool_both_migrated(self) -> None: | ||
| """Test that both deprecated fields are migrated together.""" | ||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| config = RagConfiguration( | ||
| inline=["store-1"], | ||
| tool=[constants.OKP_RAG_ID], | ||
| ) | ||
| assert config.retrieval.inline.sources == ["store-1"] | ||
| assert config.retrieval.tool.sources == [constants.OKP_RAG_ID] | ||
| assert len(w) == 2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add a test for the deprecated-plus-canonical conflict.
These tests set only the deprecated field. They do not cover a configuration that sets both inline and retrieval.inline.sources. That case currently overwrites the canonical value silently, as flagged on src/models/config.py Lines 2814-2831. Add a test that pins the intended behavior once you decide between raise and precedence.
🤖 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/models/config/test_rag_configuration.py` around lines 163 - 193,
Add a test alongside the deprecated-field migration tests that constructs
RagConfiguration with both inline and retrieval.inline.sources set, then assert
the chosen conflict behavior—raising a validation error or preserving the
canonical retrieval.inline.sources value. Use the existing warning capture and
assertions as appropriate, and ensure the test specifically prevents silent
overwriting of the canonical value.
1 participant
Summary
Unify all RAG-related configuration under a single
ragsection in the service configuration model, improving consistency and discoverability. All legacy configuration fields (byok_rag,rag_inline,rag_tool,rag_type) continue to work with deprecation warnings, ensuring full backward compatibility.Spec
LCORE-1426 — BYOK Config Refactoring
Changes
src/models/config.pyRagConfiguration,RagRetrievalConfiguration,RagByokConfigurationmodels; unifiedragsection with backward-compat propertiessrc/configuration.pyrag.*unified pathssrc/constants.pyMAX_RAG_CHUNKS/MAX_RAG_CHUNKS_TOOLas fallback defaultssrc/utils/vector_search.pyrag.retrieval.max_chunks_inlineinstead of constantsrc/utils/responses.pyrag.retrieval.max_chunks_toolinstead of constantsrc/client.pyrag.byok.storesaccessor for BYOK store configurationsrc/app/endpoints/rags.pyragconfig for RAG endpointsrc/llama_stack_configuration.pyrag.backendandrag.byok.storesfor Llama Stack synthesistests/unit/models/config/test_rag_config_refactoring.pytests/unit/models/config/test_rag_configuration.pytests/unit/models/config/test_byok_rag.pyrag.byok.storespathtests/unit/models/config/test_dump_configuration.pytests/unit/utils/test_vector_search.pytests/unit/utils/test_responses.pyAGENTS.mdThree-Layer Defense Results
Adversarial Findings (all resolved)
byok_ragproperty typed aslist[ByokRag]— Fixed: correct return typemodel_fields_setcheck for OKP defaults — Fixed: correct field detectionexclude=True— Fixed: added documentationTesting
uv run make test-unitNotes
byok_rag,rag_inline,rag_tool, andrag_typecontinue to workrag.*structureSummary by CodeRabbit
New Features
backendsetting for BYOK RAG configurations.Bug Fixes
Documentation