LCORE-2923: Add missing configuration fields to telemetry snapshot [Sonnet Firefly test] - #2282
Conversation
Extended LIGHTSPEED_STACK_FIELDS in configuration_snapshot.py with 20 new sections covering ~118 fields. Fields are classified as SENSITIVE (URLs, paths, credentials → masked) or PASSTHROUGH (booleans, ints, identifiers). Adversarial review fixes applied: - require_approval → SENSITIVE (was PASSTHROUGH) - service.base_url and root_path added - splunk.index → PASSTHROUGH (was SENSITIVE) Test coverage: - Unit tests: 3158 tests, 97% coverage on configuration_snapshot.py - QE blind exam: 117/117 tests passed (Round 1) - Adversarial: 0 CRITICAL, 0 HIGH; 1 MEDIUM + 3 LOW found and fixed Spec: LCORE-2923 Harness: Three-Layer Defense verified
are-ces
changed the title
are-ces
changed the title
WalkthroughThe telemetry configuration snapshot allowlist now includes additional service and MCP server fields. Test fixtures and suites cover expanded configuration sections, masking behavior, passthrough values, unset states, JSON serialization, and PII leak prevention. ChangesTelemetry snapshot expansion
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/unit/telemetry/conftest.py`:
- Line 113: Rename PII_SPLUNK_INDEX to a non-PII name such as
SPLUNK_INDEX_VALUE, update all references and snapshot assertions accordingly,
and add a brief comment documenting that splunk.index is an allowlisted
passthrough value intentionally excluded from ALL_PII_VALUES.
In `@tests/unit/telemetry/test_configuration_snapshot.py`:
- Around line 807-810: Update test_cache_memory_passthrough to cover actual
memory.max_entries passthrough by constructing a configuration with conversation
cache memory configured to an integer and asserting that value in the snapshot.
Alternatively, rename it to test_cache_memory_none_when_not_configured to
reflect build_minimal_config’s unset behavior, while preserving the existing
assertion.
In `@tests/unit/telemetry/test_qe_configuration_snapshot.py`:
- Around line 403-408: Replace the isinstance/flatten fallback assertions
throughout the affected snapshot tests with direct assertions against the
deterministic nested structure returned by _extract_snapshot_fields. Update the
checks around conversation_cache and the other referenced sections to index the
expected concrete keys directly and assert their values, including the listed
memory, quota_handlers, rag, okp, inference, and customization fields; remove
fallback branches that can bypass validation.
- Around line 1-12: The new telemetry test module has three pylint violations:
add the matching too-many-lines disable after its module docstring; replace the
dict(...) construction near line 186 with a dictionary literal; and simplify
both BYOK assertions near lines 578 and 1128 to assert membership in ([],
NOT_CONFIGURED). Apply these changes in
tests/unit/telemetry/test_qe_configuration_snapshot.py at ranges 1-12, 186-186,
and 578-578; the line-1128 assertion is an additional affected site in the same
file.
- Around line 92-249: Move the duplicated configuration builders from the test
module into tests/unit/telemetry/conftest.py, extending the existing
build_minimal_config() with a shared build_config(**overrides) factory that
applies these defaults and overrides. Update the tests to use the conftest
factory, remove the local _base_service, _base_llama_stack, _base_auth,
_base_user_data, _base_database, _base_inference, and _build_config helpers, and
keep test-specific overrides unchanged.
- Around line 534-558: Strengthen test_quota_handlers_limiter_count_passthrough
by asserting the snapshot’s quota_handlers limiter entry contains the
constructed limiter’s expected fields, including type, name, initial_quota,
quota_increase, and period, rather than only checking that the quota_handlers
key exists.
- Around line 1453-1472: Update test_mcp_server_headers_masked to reflect that
mcp_servers[].headers is PASSTHROUGH rather than SENSITIVE: correct the
docstring and comments, then assert that the configured header names remain
present in the corresponding snapshot entry instead of only checking the
top-level mcp_servers key.
- Around line 1119-1128: Update the skills_snap dictionary branch in the
snapshot test to assert paths_val is exactly CONFIGURED, removing the permissive
alternatives and vacuous paths_val is not None condition. Keep the existing
whole-section masked assertion 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 Plus
Run ID: f1310ac5-d4de-4e7f-8ade-e25300ee1cc4
📒 Files selected for processing (4)
src/telemetry/configuration_snapshot.pytests/unit/telemetry/conftest.pytests/unit/telemetry/test_configuration_snapshot.pytests/unit/telemetry/test_qe_configuration_snapshot.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-7-on-pull-request
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: integration_tests (3.13)
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: integration_tests (3.12)
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: build-pr
⚠️ CI failures not shown inline (2)
GitHub Actions: Python linter / Pylinter: LCORE-2923: Add missing configuration fields to telemetry snapshot [Sonnet Firefly test]
Conclusion: failure
##[group]Run uv run pylint src tests
�[36;1muv run pylint 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]
************* Module tests.unit.telemetry.test_qe_configuration_snapshot
tests/unit/telemetry/test_qe_configuration_snapshot.py:3:0: C0302: Too many lines in module (2048/1000) (too-many-lines)
tests/unit/telemetry/test_qe_configuration_snapshot.py:186:34: R1735: Consider using '{"name": 'test-service', "service": _base_service(), "llama_stack": _base_llama_stack(), ... }' instead of a call to 'dict'. (use-dict-literal)
tests/unit/telemetry/test_qe_configuration_snapshot.py:578:15: R1714: Consider merging these comparisons with 'in' by using 'byok in ([], NOT_CONFIGURED)'. Use a set instead if elements are hashable. (consider-using-in)
tests/unit/telemetry/test_qe_configuration_snapshot.py:1128:19: R1714: Consider merging these comparisons with 'in' by using 'skills_snap in (CONFIGURED, NOT_CONFIGURED)'. Use a set instead if elements are hashable. (consider-using-in)
------------------------------------
Your code has been rated at 10.00/10
##[error]Process completed with exit code 24.
GitHub Actions: Python linter / 0_Pylinter.txt: LCORE-2923: Add missing configuration fields to telemetry snapshot [Sonnet Firefly test]
Conclusion: failure
##[group]Run uv run pylint src tests
�[36;1muv run pylint 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]
************* Module tests.unit.telemetry.test_qe_configuration_snapshot
tests/unit/telemetry/test_qe_configuration_snapshot.py:3:0: C0302: Too many lines in module (2048/1000) (too-many-lines)
tests/unit/telemetry/test_qe_configuration_snapshot.py:186:34: R1735: Consider using '{"name": 'test-service', "service": _base_service(), "llama_stack": _base_llama_stack(), ... }' instead of a call to 'dict'. (use-dict-literal)
tests/unit/telemetry/test_qe_configuration_snapshot.py:578:15: R1714: Consider merging these comparisons with 'in' by using 'byok in ([], NOT_CONFIGURED)'. Use a set instead if elements are hashable. (consider-using-in)
tests/unit/telemetry/test_qe_configuration_snapshot.py:1128:19: R1714: Consider merging these comparisons with 'in' by using 'skills_snap in (CONFIGURED, NOT_CONFIGURED)'. Use a set instead if elements are hashable. (consider-using-in)
------------------------------------
Your code has been rated at 10.00/10
##[error]Process completed with exit code 24.
🧰 Additional context used
📓 Path-based instructions (3)
**/*
📄 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:
src/telemetry/configuration_snapshot.pytests/unit/telemetry/test_qe_configuration_snapshot.pytests/unit/telemetry/conftest.pytests/unit/telemetry/test_configuration_snapshot.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.
Use complete type annotations for function parameters and return types.
Use modern union syntax such asstr | int; useOptional[Type]for optional values.
Usetyping_extensions.Selffor model validators.
Functions must use descriptive, action-oriented snake_case names such asget_,validate_, andcheck_.
Avoid modifying mutable parameters in place; return a new data structure instead.
Useasync deffor I/O operations and external API calls.
HandleAPIConnectionErrorfrom Llama Stack.
All classes must have descriptive docstrings and complete type annotations for class attributes; use specific types instead ofAny.
Use PascalCase for classes and descriptive standard suffixes such asConfiguration,Error/Exception,Resolver, andInterface.
Use ABC and@abstractmethodfor abstract interfaces.
Follow Google Python docstring conventions; document all modules, classes, and functions, includingParameters,Returns,Raises, andAttributessections as applicable.
Never commit secrets or keys; use environment variables for sensitive data.
Files:
src/telemetry/configuration_snapshot.pytests/unit/telemetry/test_qe_configuration_snapshot.pytests/unit/telemetry/conftest.pytests/unit/telemetry/test_configuration_snapshot.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/unit/**/*.py: Use pytest for all unit tests; do not use unittest.
Useconftest.pyfor shared fixtures,pytest-mockforAsyncMockobjects, andpytest.mark.asynciofor asynchronous tests.
Files:
tests/unit/telemetry/test_qe_configuration_snapshot.pytests/unit/telemetry/conftest.pytests/unit/telemetry/test_configuration_snapshot.py
🧠 Learnings (3)
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
src/telemetry/configuration_snapshot.pytests/unit/telemetry/test_qe_configuration_snapshot.pytests/unit/telemetry/conftest.pytests/unit/telemetry/test_configuration_snapshot.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/telemetry/configuration_snapshot.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/telemetry/configuration_snapshot.py
🪛 ast-grep (0.45.0)
tests/unit/telemetry/test_qe_configuration_snapshot.py
[info] 407-407: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["conversation_cache"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 432-432: use jsonify instead of json.dumps for JSON output
Context: json.dumps(cache_snap)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 508-508: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["quota_handlers"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 674-674: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["byok_rag"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 745-745: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["a2a_state"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 766-766: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["a2a_state"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 838-838: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["splunk"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 871-871: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["splunk"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 952-952: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["azure_entra_id"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 965-965: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["azure_entra_id"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1107-1107: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["skills"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1239-1239: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["okp"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1391-1391: use jsonify instead of json.dumps for JSON output
Context: json.dumps(mcp)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1450-1450: use jsonify instead of json.dumps for JSON output
Context: json.dumps(mcp)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1753-1753: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["customization"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1815-1815: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_lightspeed_stack_snapshot(config))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1829-1829: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_lightspeed_stack_snapshot(config))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1854-1854: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_lightspeed_stack_snapshot(config))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1867-1867: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_lightspeed_stack_snapshot(config))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1877-1877: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_lightspeed_stack_snapshot(config))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1889-1889: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_lightspeed_stack_snapshot(config))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1909-1909: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_lightspeed_stack_snapshot(config))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1957-1957: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2043-2043: use jsonify instead of json.dumps for JSON output
Context: json.dumps(build_lightspeed_stack_snapshot(config))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 156-156: Do not hardcode temporary file or directory names
Context: "/tmp/lightspeed-stack.db"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 380-380: Do not hardcode temporary file or directory names
Context: "/tmp/cache.db"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 GitHub Actions: Python linter / 0_Pylinter.txt
tests/unit/telemetry/test_qe_configuration_snapshot.py
[error] 3-3: pylint: C0302 Too many lines in module (2048/1000) (too-many-lines)
[error] 186-186: pylint: R1735 Consider using dict literal instead of a call to 'dict'. (use-dict-literal)
[error] 578-578: pylint: R1714 Consider merging these comparisons with 'in' by using 'byok in ([], NOT_CONFIGURED)'. (consider-using-in)
[error] 1128-1128: pylint: R1714 Consider merging these comparisons with 'in' by using 'skills_snap in (CONFIGURED, NOT_CONFIGURED)'. (consider-using-in)
🪛 GitHub Actions: Python linter / Pylinter
tests/unit/telemetry/test_qe_configuration_snapshot.py
[error] 3-3: pylint: C0302 Too many lines in module (2048/1000) (too-many-lines)
[error] 186-186: pylint: R1735 Consider using dict literal instead of a call to 'dict'. (use-dict-literal)
[error] 578-578: pylint: R1714 Consider merging comparisons with 'in'. (consider-using-in)
[error] 1128-1128: pylint: R1714 Consider merging comparisons with 'in'. (consider-using-in)
🔇 Additional comments (5)
tests/unit/telemetry/test_qe_configuration_snapshot.py (2)
156-157: 📐 Maintainability & Code QualityStatic analysis flags hardcoded
/tmppaths — safe to ignore here.These are never opened;
model_constructbypasses validators and the values only exist to be masked. No change needed.Also applies to: 380-382
Source: Linters/SAST tools
1961-2048: LGTM!src/telemetry/configuration_snapshot.py (1)
82-83: LGTM!Also applies to: 159-164, 165-322
tests/unit/telemetry/conftest.py (1)
11-21: LGTM!Also applies to: 32-50, 82-112, 114-122, 149-187, 283-283, 307-335, 355-370, 394-398, 421-554, 594-648
tests/unit/telemetry/test_configuration_snapshot.py (1)
3-4: LGTM!Also applies to: 14-20, 44-47, 403-403, 741-806, 811-1456, 1458-1462
| PII_A2A_PG_CA_CERT = "/etc/ssl/a2a-postgres/ca.crt" | ||
| PII_SPLUNK_URL = "https://splunk.internal.corp.com:8088" | ||
| PII_SPLUNK_TOKEN_PATH = "/etc/splunk/hec_token.txt" | ||
| PII_SPLUNK_INDEX = "lightspeed_events_prod" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Rename PII_SPLUNK_INDEX — it's deliberately excluded from ALL_PII_VALUES.
splunk.index is allowlisted as PASSTHROUGH and asserted to appear verbatim in the snapshot, so this constant is not PII. The PII_ prefix invites someone to later add it to ALL_PII_VALUES and break the leak test. Suggest SPLUNK_INDEX_VALUE (or similar) plus a short comment.
🤖 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/telemetry/conftest.py` at line 113, Rename PII_SPLUNK_INDEX to a
non-PII name such as SPLUNK_INDEX_VALUE, update all references and snapshot
assertions accordingly, and add a brief comment documenting that splunk.index is
an allowlisted passthrough value intentionally excluded from ALL_PII_VALUES.
| def test_cache_memory_passthrough(self) -> None: | ||
| """Test conversation cache memory max_entries passes through.""" | ||
| snapshot = build_lightspeed_stack_snapshot(build_minimal_config()) | ||
| assert snapshot["conversation_cache"]["memory"]["max_entries"] is None |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Misleading name: this asserts the unset case, not passthrough.
build_minimal_config() has conversation_cache=None, so this duplicates test_cache_memory_none_... semantics and leaves memory.max_entries passthrough uncovered here (the fully-populated fixture sets memory=None). Either rename to test_cache_memory_none_when_not_configured or build a config with memory.max_entries set and assert the integer.
🤖 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/telemetry/test_configuration_snapshot.py` around lines 807 - 810,
Update test_cache_memory_passthrough to cover actual memory.max_entries
passthrough by constructing a configuration with conversation cache memory
configured to an integer and asserting that value in the snapshot.
Alternatively, rename it to test_cache_memory_none_when_not_configured to
reflect build_minimal_config’s unset behavior, while preserving the existing
assertion.
| """QE blind-exam tests for configuration snapshot telemetry. | ||
|
|
||
| Verifies that build_lightspeed_stack_snapshot correctly includes all | ||
| configuration sections introduced in LCORE-2923, with proper masking of | ||
| sensitive fields and passthrough of non-sensitive fields. | ||
|
|
||
| Sections under test: | ||
| compaction, conversation_cache, quota_handlers, byok_rag, a2a_state, | ||
| splunk, inference, llama_stack, authentication, azure_entra_id, | ||
| customization, okp, rag, reranker, approvals, rlsapi_v1, saved_prompts, | ||
| skills, deployment_environment, mcp_servers. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Pylint currently fails on this new module (3 blocking findings). Root cause: the file was added without a clean pylint run; each finding is a one-line fix.
tests/unit/telemetry/test_qe_configuration_snapshot.py#L1-L12: add# pylint: disable=too-many-linesafter the module docstring (C0302, 2048/1000), matchingtests/unit/telemetry/test_configuration_snapshot.py.tests/unit/telemetry/test_qe_configuration_snapshot.py#L186-L186: replacedict(...)with a{...}literal (R1735).tests/unit/telemetry/test_qe_configuration_snapshot.py#L578-L578: rewrite asassert byok in ([], NOT_CONFIGURED)(R1714); the same rule also fires at L1128.
🧰 Tools
🪛 GitHub Actions: Python linter / 0_Pylinter.txt
[error] 3-3: pylint: C0302 Too many lines in module (2048/1000) (too-many-lines)
🪛 GitHub Actions: Python linter / Pylinter
[error] 3-3: pylint: C0302 Too many lines in module (2048/1000) (too-many-lines)
📍 Affects 1 file
tests/unit/telemetry/test_qe_configuration_snapshot.py#L1-L12(this comment)tests/unit/telemetry/test_qe_configuration_snapshot.py#L186-L186tests/unit/telemetry/test_qe_configuration_snapshot.py#L578-L578
🤖 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/telemetry/test_qe_configuration_snapshot.py` around lines 1 - 12,
The new telemetry test module has three pylint violations: add the matching
too-many-lines disable after its module docstring; replace the dict(...)
construction near line 186 with a dictionary literal; and simplify both BYOK
assertions near lines 578 and 1128 to assert membership in ([], NOT_CONFIGURED).
Apply these changes in tests/unit/telemetry/test_qe_configuration_snapshot.py at
ranges 1-12, 186-186, and 578-578; the line-1128 assertion is an additional
affected site in the same file.
Source: Pipeline failures
| def _base_service() -> ServiceConfiguration: | ||
| """Return a minimal ServiceConfiguration for test configs.""" | ||
| return ServiceConfiguration.model_construct( | ||
| host="localhost", | ||
| port=8080, | ||
| base_url=None, | ||
| workers=1, | ||
| auth_enabled=False, | ||
| color_log=True, | ||
| access_log=True, | ||
| root_path="", | ||
| tls_config=TLSConfiguration.model_construct( | ||
| tls_certificate_path=None, | ||
| tls_key_path=None, | ||
| tls_key_password=None, | ||
| ), | ||
| cors=None, | ||
| ) | ||
|
|
||
|
|
||
| def _base_llama_stack() -> LlamaStackConfiguration: | ||
| """Return a minimal LlamaStackConfiguration for test configs.""" | ||
| return LlamaStackConfiguration.model_construct( | ||
| url=None, | ||
| api_key=None, | ||
| use_as_library_client=True, | ||
| library_client_config_path=None, | ||
| timeout=180, | ||
| max_retries=3, | ||
| retry_delay=5, | ||
| allow_degraded_mode=False, | ||
| config=None, | ||
| ) | ||
|
|
||
|
|
||
| def _base_auth() -> AuthenticationConfiguration: | ||
| """Return a minimal AuthenticationConfiguration for test configs.""" | ||
| return AuthenticationConfiguration.model_construct( | ||
| module="noop", | ||
| skip_tls_verification=False, | ||
| skip_for_health_probes=False, | ||
| skip_for_metrics=False, | ||
| k8s_cluster_api=None, | ||
| k8s_ca_cert_path=None, | ||
| jwk_config=None, | ||
| api_key_config=None, | ||
| rh_identity_config=None, | ||
| trusted_proxy_config=None, | ||
| ) | ||
|
|
||
|
|
||
| def _base_user_data() -> UserDataCollection: | ||
| """Return a minimal UserDataCollection for test configs.""" | ||
| return UserDataCollection.model_construct( | ||
| feedback_enabled=False, | ||
| feedback_storage=None, | ||
| transcripts_enabled=False, | ||
| transcripts_storage=None, | ||
| ) | ||
|
|
||
|
|
||
| def _base_database() -> DatabaseConfiguration: | ||
| """Return a minimal DatabaseConfiguration for test configs.""" | ||
| return DatabaseConfiguration.model_construct( | ||
| sqlite=SQLiteDatabaseConfiguration.model_construct( | ||
| db_path="/tmp/lightspeed-stack.db", | ||
| ), | ||
| postgres=None, | ||
| ) | ||
|
|
||
|
|
||
| def _base_inference() -> InferenceConfiguration: | ||
| """Return a minimal InferenceConfiguration for test configs.""" | ||
| return InferenceConfiguration.model_construct( | ||
| default_model=None, | ||
| default_provider=None, | ||
| context_windows={}, | ||
| providers=[], | ||
| max_infer_iters=10, | ||
| max_tool_calls=30, | ||
| ) | ||
|
|
||
|
|
||
| def _build_config(**overrides: object) -> Configuration: | ||
| """Build a Configuration with sensible defaults, overriding specific fields. | ||
|
|
||
| Parameters: | ||
| ---------- | ||
| **overrides: Fields to override on the Configuration. | ||
|
|
||
| Returns: | ||
| ------- | ||
| Configuration: A model_construct()-built Configuration instance. | ||
| """ | ||
| defaults: dict[str, object] = dict( | ||
| name="test-service", | ||
| service=_base_service(), | ||
| llama_stack=_base_llama_stack(), | ||
| inference=_base_inference(), | ||
| authentication=_base_auth(), | ||
| authorization=None, | ||
| user_data_collection=_base_user_data(), | ||
| customization=None, | ||
| database=_base_database(), | ||
| mcp_servers=[], | ||
| conversation_cache=ConversationHistoryConfiguration.model_construct( | ||
| type=None, memory=None, sqlite=None, postgres=None | ||
| ), | ||
| compaction=CompactionConfiguration.model_construct( | ||
| enabled=False, | ||
| threshold_ratio=0.7, | ||
| token_floor=4096, | ||
| buffer_turns=4, | ||
| buffer_max_ratio=0.3, | ||
| ), | ||
| approvals=ApprovalsConfiguration.model_construct( | ||
| approval_timeout_seconds=300, | ||
| approval_retention_days=30, | ||
| ), | ||
| byok_rag=[], | ||
| a2a_state=A2AStateConfiguration.model_construct(sqlite=None, postgres=None), | ||
| quota_handlers=QuotaHandlersConfiguration.model_construct( | ||
| sqlite=None, | ||
| postgres=None, | ||
| limiters=[], | ||
| scheduler=QuotaSchedulerConfiguration.model_construct( | ||
| period=1, | ||
| database_reconnection_count=10, | ||
| database_reconnection_delay=1, | ||
| ), | ||
| enable_token_history=False, | ||
| ), | ||
| azure_entra_id=None, | ||
| rlsapi_v1=RlsapiV1Configuration.model_construct( | ||
| allow_verbose_infer=False, | ||
| quota_subject=None, | ||
| ), | ||
| splunk=None, | ||
| deployment_environment="development", | ||
| rag=RagConfiguration.model_construct(inline=[], tool=[]), | ||
| okp=OkpConfiguration.model_construct( | ||
| rhokp_url=None, | ||
| offline=True, | ||
| chunk_filter_query=None, | ||
| ), | ||
| reranker=RerankerConfiguration.model_construct( | ||
| enabled=False, | ||
| model="cross-encoder/ms-marco-MiniLM-L6-v2", | ||
| ), | ||
| skills=None, | ||
| saved_prompts=SavedPromptsConfiguration.model_construct( | ||
| max_prompts_per_user=10, | ||
| max_display_name_length=100, | ||
| max_content_length=4096, | ||
| ), | ||
| ) | ||
| defaults.update(overrides) | ||
| return Configuration.model_construct(**defaults) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move these builders into conftest.py.
_base_service/_base_llama_stack/_base_auth/_base_inference restate the same minimal schema already built by build_minimal_config() in tests/unit/telemetry/conftest.py, so every future config-schema change requires edits in two places. A shared conftest factory (e.g. build_config(**overrides) layered on the existing minimal builder) removes the duplication.
As per coding guidelines, "Use conftest.py for shared fixtures".
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 156-156: Do not hardcode temporary file or directory names
Context: "/tmp/lightspeed-stack.db"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 GitHub Actions: Python linter / 0_Pylinter.txt
[error] 186-186: pylint: R1735 Consider using dict literal instead of a call to 'dict'. (use-dict-literal)
🪛 GitHub Actions: Python linter / Pylinter
[error] 186-186: pylint: R1735 Consider using dict literal instead of a call to 'dict'. (use-dict-literal)
🤖 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/telemetry/test_qe_configuration_snapshot.py` around lines 92 -
249, Move the duplicated configuration builders from the test module into
tests/unit/telemetry/conftest.py, extending the existing build_minimal_config()
with a shared build_config(**overrides) factory that applies these defaults and
overrides. Update the tests to use the conftest factory, remove the local
_base_service, _base_llama_stack, _base_auth, _base_user_data, _base_database,
_base_inference, and _build_config helpers, and keep test-specific overrides
unchanged.
Source: Coding guidelines
| db_path_val = snapshot["conversation_cache"].get("sqlite", {}) | ||
| if isinstance(db_path_val, dict): | ||
| assert db_path_val.get("db_path") == CONFIGURED | ||
| else: | ||
| # If the snapshot flattens it, check the raw value is not exposed | ||
| assert "/secret/cache.db" not in json.dumps(snapshot["conversation_cache"]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Drop the isinstance/flatten fallbacks — they turn assertions into no-ops.
_extract_snapshot_fields produces a deterministic nested dict for every registered spec, so if isinstance(x, dict) branches (and else: assert "<key>" in snapshot) can never be needed and will silently skip the real check if the structure ever regresses. Assert the concrete shape directly, e.g. assert snapshot["conversation_cache"]["memory"]["max_entries"] == 1000, assert snapshot["quota_handlers"]["scheduler"]["period"] == 5, assert snapshot["rag"]["inline"] == ["okp", "my-byok-rag"], assert snapshot["okp"]["offline"] is True, assert snapshot["inference"]["context_windows"] == {...}, assert snapshot["customization"]["disable_shield_ids_override"] is True.
Also applies to: 435-452, 511-532, 1175-1207, 1223-1228, 1576-1592, 1756-1792
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 407-407: use jsonify instead of json.dumps for JSON output
Context: json.dumps(snapshot["conversation_cache"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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/telemetry/test_qe_configuration_snapshot.py` around lines 403 -
408, Replace the isinstance/flatten fallback assertions throughout the affected
snapshot tests with direct assertions against the deterministic nested structure
returned by _extract_snapshot_fields. Update the checks around
conversation_cache and the other referenced sections to index the expected
concrete keys directly and assert their values, including the listed memory,
quota_handlers, rag, okp, inference, and customization fields; remove fallback
branches that can bypass validation.
| def test_quota_handlers_limiter_count_passthrough(self) -> None: | ||
| """quota_handlers limiter list length must be represented.""" | ||
| limiter = QuotaLimiterConfiguration.model_construct( | ||
| type="user_limiter", | ||
| name="daily-limit", | ||
| initial_quota=1000, | ||
| quota_increase=0, | ||
| period="1 day", | ||
| ) | ||
| config = _build_config( | ||
| quota_handlers=QuotaHandlersConfiguration.model_construct( | ||
| sqlite=None, | ||
| postgres=None, | ||
| limiters=[limiter], | ||
| scheduler=QuotaSchedulerConfiguration.model_construct( | ||
| period=1, | ||
| database_reconnection_count=10, | ||
| database_reconnection_delay=1, | ||
| ), | ||
| enable_token_history=False, | ||
| ) | ||
| ) | ||
| snapshot = build_lightspeed_stack_snapshot(config) | ||
| # The snapshot should not crash and quota_handlers key must exist | ||
| assert "quota_handlers" in snapshot |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Name promises limiter passthrough; body only checks the key exists.
The limiter is constructed and then never asserted on. Verify the extracted item fields.
🔧 Proposed fix
- snapshot = build_lightspeed_stack_snapshot(config)
- # The snapshot should not crash and quota_handlers key must exist
- assert "quota_handlers" in snapshot
+ snapshot = build_lightspeed_stack_snapshot(config)
+ limiters = snapshot["quota_handlers"]["limiters"]
+ assert len(limiters) == 1
+ assert limiters[0]["type"] == "user_limiter"
+ assert limiters[0]["name"] == "daily-limit"
+ assert limiters[0]["initial_quota"] == 1000
+ assert limiters[0]["period"] == "1 day"📝 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.
| def test_quota_handlers_limiter_count_passthrough(self) -> None: | |
| """quota_handlers limiter list length must be represented.""" | |
| limiter = QuotaLimiterConfiguration.model_construct( | |
| type="user_limiter", | |
| name="daily-limit", | |
| initial_quota=1000, | |
| quota_increase=0, | |
| period="1 day", | |
| ) | |
| config = _build_config( | |
| quota_handlers=QuotaHandlersConfiguration.model_construct( | |
| sqlite=None, | |
| postgres=None, | |
| limiters=[limiter], | |
| scheduler=QuotaSchedulerConfiguration.model_construct( | |
| period=1, | |
| database_reconnection_count=10, | |
| database_reconnection_delay=1, | |
| ), | |
| enable_token_history=False, | |
| ) | |
| ) | |
| snapshot = build_lightspeed_stack_snapshot(config) | |
| # The snapshot should not crash and quota_handlers key must exist | |
| assert "quota_handlers" in snapshot | |
| def test_quota_handlers_limiter_count_passthrough(self) -> None: | |
| """quota_handlers limiter list length must be represented.""" | |
| limiter = QuotaLimiterConfiguration.model_construct( | |
| type="user_limiter", | |
| name="daily-limit", | |
| initial_quota=1000, | |
| quota_increase=0, | |
| period="1 day", | |
| ) | |
| config = _build_config( | |
| quota_handlers=QuotaHandlersConfiguration.model_construct( | |
| sqlite=None, | |
| postgres=None, | |
| limiters=[limiter], | |
| scheduler=QuotaSchedulerConfiguration.model_construct( | |
| period=1, | |
| database_reconnection_count=10, | |
| database_reconnection_delay=1, | |
| ), | |
| enable_token_history=False, | |
| ) | |
| ) | |
| snapshot = build_lightspeed_stack_snapshot(config) | |
| limiters = snapshot["quota_handlers"]["limiters"] | |
| assert len(limiters) == 1 | |
| assert limiters[0]["type"] == "user_limiter" | |
| assert limiters[0]["name"] == "daily-limit" | |
| assert limiters[0]["initial_quota"] == 1000 | |
| assert limiters[0]["period"] == "1 day" |
🤖 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/telemetry/test_qe_configuration_snapshot.py` around lines 534 -
558, Strengthen test_quota_handlers_limiter_count_passthrough by asserting the
snapshot’s quota_handlers limiter entry contains the constructed limiter’s
expected fields, including type, name, initial_quota, quota_increase, and
period, rather than only checking that the quota_handlers key exists.
| if isinstance(skills_snap, dict): | ||
| paths_val = skills_snap.get("paths") | ||
| assert ( | ||
| paths_val == CONFIGURED | ||
| or paths_val == [CONFIGURED] | ||
| or paths_val is not None | ||
| ) | ||
| else: | ||
| # If the whole skills section is masked | ||
| assert skills_snap == CONFIGURED or skills_snap == NOT_CONFIGURED |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pylint R1714 here too, and the assertion is vacuous.
paths_val is not None makes the whole or chain trivially true for any populated value, so this test can never fail on a masking regression. The allowlist marks skills.paths SENSITIVE with a scalar mask, so assert exactly CONFIGURED.
🔧 Proposed fix
- skills_snap = snapshot["skills"]
- if isinstance(skills_snap, dict):
- paths_val = skills_snap.get("paths")
- assert (
- paths_val == CONFIGURED
- or paths_val == [CONFIGURED]
- or paths_val is not None
- )
- else:
- # If the whole skills section is masked
- assert skills_snap == CONFIGURED or skills_snap == NOT_CONFIGURED
+ assert snapshot["skills"]["paths"] == CONFIGURED🧰 Tools
🪛 GitHub Actions: Python linter / 0_Pylinter.txt
[error] 1128-1128: pylint: R1714 Consider merging these comparisons with 'in' by using 'skills_snap in (CONFIGURED, NOT_CONFIGURED)'. (consider-using-in)
🪛 GitHub Actions: Python linter / Pylinter
[error] 1128-1128: pylint: R1714 Consider merging comparisons with 'in'. (consider-using-in)
🤖 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/telemetry/test_qe_configuration_snapshot.py` around lines 1119 -
1128, Update the skills_snap dictionary branch in the snapshot test to assert
paths_val is exactly CONFIGURED, removing the permissive alternatives and
vacuous paths_val is not None condition. Keep the existing whole-section masked
assertion unchanged.
Source: Pipeline failures
| def test_mcp_server_headers_masked(self) -> None: | ||
| """mcp_servers[].headers (propagated headers) must be masked (SENSITIVE).""" | ||
| config = _build_config( | ||
| mcp_servers=[ | ||
| ModelContextProtocolServer.model_construct( | ||
| name="test-mcp", | ||
| provider_id="model-context-protocol", | ||
| url=_PII_MCP_URL, | ||
| authorization_headers={}, | ||
| headers=["x-rh-identity", "x-internal-token"], | ||
| require_approval="never", | ||
| timeout=None, | ||
| ) | ||
| ] | ||
| ) | ||
| snapshot = build_lightspeed_stack_snapshot(config) | ||
| # headers list may be masked or passed through depending on classification | ||
| # The spec says headers are SENSITIVE — verify no raw header names leak | ||
| # (or if passthrough, just verify no crash) | ||
| assert "mcp_servers" in snapshot |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This test asserts nothing and its docstring contradicts the allowlist.
mcp_servers[].headers is registered PASSTHROUGH, not SENSITIVE, yet the docstring claims masking and the only assertion is a key-presence check that passes regardless. Assert the actual contract.
🔧 Proposed fix
- def test_mcp_server_headers_masked(self) -> None:
- """mcp_servers[].headers (propagated headers) must be masked (SENSITIVE)."""
+ def test_mcp_server_headers_passthrough(self) -> None:
+ """mcp_servers[].headers (header names) must pass through as-is."""
@@
- snapshot = build_lightspeed_stack_snapshot(config)
- # headers list may be masked or passed through depending on classification
- # The spec says headers are SENSITIVE — verify no raw header names leak
- # (or if passthrough, just verify no crash)
- assert "mcp_servers" in snapshot
+ snapshot = build_lightspeed_stack_snapshot(config)
+ assert snapshot["mcp_servers"][0]["headers"] == [
+ "x-rh-identity",
+ "x-internal-token",
+ ]📝 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.
| def test_mcp_server_headers_masked(self) -> None: | |
| """mcp_servers[].headers (propagated headers) must be masked (SENSITIVE).""" | |
| config = _build_config( | |
| mcp_servers=[ | |
| ModelContextProtocolServer.model_construct( | |
| name="test-mcp", | |
| provider_id="model-context-protocol", | |
| url=_PII_MCP_URL, | |
| authorization_headers={}, | |
| headers=["x-rh-identity", "x-internal-token"], | |
| require_approval="never", | |
| timeout=None, | |
| ) | |
| ] | |
| ) | |
| snapshot = build_lightspeed_stack_snapshot(config) | |
| # headers list may be masked or passed through depending on classification | |
| # The spec says headers are SENSITIVE — verify no raw header names leak | |
| # (or if passthrough, just verify no crash) | |
| assert "mcp_servers" in snapshot | |
| def test_mcp_server_headers_passthrough(self) -> None: | |
| """mcp_servers[].headers (header names) must pass through as-is.""" | |
| config = _build_config( | |
| mcp_servers=[ | |
| ModelContextProtocolServer.model_construct( | |
| name="test-mcp", | |
| provider_id="model-context-protocol", | |
| url=_PII_MCP_URL, | |
| authorization_headers={}, | |
| headers=["x-rh-identity", "x-internal-token"], | |
| require_approval="never", | |
| timeout=None, | |
| ) | |
| ] | |
| ) | |
| snapshot = build_lightspeed_stack_snapshot(config) | |
| assert snapshot["mcp_servers"][0]["headers"] == [ | |
| "x-rh-identity", | |
| "x-internal-token", | |
| ] |
🤖 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/telemetry/test_qe_configuration_snapshot.py` around lines 1453 -
1472, Update test_mcp_server_headers_masked to reflect that
mcp_servers[].headers is PASSTHROUGH rather than SENSITIVE: correct the
docstring and comments, then assert that the configured header names remain
present in the corresponding snapshot entry instead of only checking the
top-level mcp_servers key.
1 participant
Summary
Extends the telemetry configuration snapshot (
src/telemetry/configuration_snapshot.py) to include all current LCS configuration fields. Previously ~54 fields were collected; this PR adds ~118 fields across 20 new sections.Closes LCORE-2923.
Changes
src/telemetry/configuration_snapshot.py— ExtendedLIGHTSPEED_STACK_FIELDSwith 20 new sections:compaction,conversation_cache,quota_handlers,byok_rag[],a2a_state,splunk,inference(extended),llama_stack(extended),authentication(extended),azure_entra_id,customization(extended),okp,rag,reranker,approvals,rlsapi_v1,saved_prompts,skills,deployment_environment,mcp_servers[](extended). Also addedservice.base_url(SENSITIVE) andservice.root_path(PASSTHROUGH).tests/unit/telemetry/conftest.py— Added 30+ PII constants and extended fixtures for all new sections.tests/unit/telemetry/test_configuration_snapshot.py— Added 85+ new tests across 14 new test classes.tests/unit/telemetry/test_qe_configuration_snapshot.py— New blind functional test file with 117 tests.Three-Layer Defense Results
configuration_snapshot.pySummary by CodeRabbit