LCORE-2341-unified mode feature files#2020
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (19)
🧰 Additional context used📓 Path-based instructions (1)tests/e2e/**/*.{py,feature}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (2)
WalkthroughFive new Gherkin E2E feature files are added for unified-mode boot, legacy configuration, migration, synthesis, and validation, and they are registered in ChangesUnified mode E2E tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/e2e/features/unified-mode-boot.feature`:
- Around line 47-60: The profile-path boot scenarios only verify readiness and
never exercise query serving, so the relative and absolute variants can still
break inference without failing the test. Update the unified-mode boot scenarios
in unified-mode-boot.feature to include an actual inference/query request after
the service restarts and readiness passes, using the existing scenario patterns
for the relative and absolute profile-path cases so both boot and serve behavior
are covered.
In `@tests/e2e/features/unified-mode-migration.feature`:
- Around line 25-36: The migrated unified-mode scenarios have the action step
out of standard Given-When-Then order; in the affected feature cases, move the
service configuration precondition into the Given phase before the migration
action, or rewrite the migration step as part of the When/And action phase.
Update the affected scenarios in unified-mode-migration.feature so the step
sequence around lightspeed-stack --migrate-config, The service uses the
lightspeed-stack-unified-migrated.yaml configuration, and The service is
restarted reads clearly and follows BDD structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43a48d62-94ab-4079-97fc-0bd6d5288518
📒 Files selected for processing (6)
tests/e2e/features/unified-mode-boot.featuretests/e2e/features/unified-mode-legacy.featuretests/e2e/features/unified-mode-migration.featuretests/e2e/features/unified-mode-synthesis.featuretests/e2e/features/unified-mode-validation.featuretests/e2e/test_list.txt
📜 Review details
⏰ Context from checks skipped due to timeout. (17)
- GitHub Check: spectral
- GitHub Check: unit_tests (3.12)
- GitHub Check: Pyright
- GitHub Check: unit_tests (3.13)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{py,feature}
📄 CodeRabbit inference engine (AGENTS.md)
Use behave (BDD) framework for end-to-end testing with Gherkin feature files
Files:
tests/e2e/features/unified-mode-legacy.featuretests/e2e/features/unified-mode-validation.featuretests/e2e/features/unified-mode-migration.featuretests/e2e/features/unified-mode-synthesis.featuretests/e2e/features/unified-mode-boot.feature
🔇 Additional comments (6)
tests/e2e/features/unified-mode-synthesis.feature (1)
1-59: LGTM!tests/e2e/features/unified-mode-legacy.feature (1)
1-40: LGTM!tests/e2e/features/unified-mode-validation.feature (2)
16-19: LGTM!
10-13: 🎯 Functional CorrectnessNo change needed. The validator message for
inference.providers+library_client_config_pathalready includes bothmutually exclusiveand--migrate-config, so the E2E assertion matches the implementation.> Likely an incorrect or invalid review comment.tests/e2e/test_list.txt (2)
35-39: LGTM!
35-39: All referenced feature files exist. The fivetests/e2e/features/unified-mode-*.featurepaths are present at the expected locations.
| @skip-in-server-mode | ||
| Scenario: Unified config with relative profile path boots in library mode | ||
| Given The service uses the lightspeed-stack-unified-relative-profile.yaml configuration | ||
| And The service is restarted | ||
| When I access endpoint "readiness" using HTTP GET method | ||
| Then The status code of the response is 200 | ||
|
|
||
|
|
||
| @skip-in-server-mode | ||
| Scenario: Unified config with absolute profile path boots in library mode | ||
| Given The service uses the lightspeed-stack-unified-absolute-profile.yaml configuration | ||
| And The service is restarted | ||
| When I access endpoint "readiness" using HTTP GET method | ||
| Then The status code of the response is 200 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exercise query serving for the profile-path variants.
These four scenarios stop at /readiness, so they never verify that the relative/absolute profile configs can actually serve inference requests. That leaves the boot contract untested for those variants and would miss regressions where startup succeeds but profile resolution breaks query handling.
Also applies to: 101-116
🤖 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/e2e/features/unified-mode-boot.feature` around lines 47 - 60, The
profile-path boot scenarios only verify readiness and never exercise query
serving, so the relative and absolute variants can still break inference without
failing the test. Update the unified-mode boot scenarios in
unified-mode-boot.feature to include an actual inference/query request after the
service restarts and readiness passes, using the existing scenario patterns for
the relative and absolute profile-path cases so both boot and serve behavior are
covered.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/e2e/features/unified-mode-boot.feature`:
- Line 1: The feature is currently tagged with `@skip`, which prevents every
scenario in unified-mode-boot from running because environment.py skips any
scenario carrying that tag. Remove the feature-level `@skip` from
unified-mode-boot.feature unless this suite is intentionally disabled, and keep
the existing e2e_group_2 tag so the scenarios can execute normally.
In `@tests/e2e/features/unified-mode-legacy.feature`:
- Line 1: Remove the feature-level `@skip` tag from the unified-mode-legacy
feature so the scenarios are no longer skipped entirely; keep the existing
scenario-level mode tags to control which legacy cases run. Update the feature
header in tests/e2e/features/unified-mode-legacy.feature so only the intended
scenario gating remains.
In `@tests/e2e/features/unified-mode-migration.feature`:
- Line 1: The feature is currently marked with a file-level `@skip`, which
prevents all unified mode migration scenarios from running in E2E. Remove the
`@skip` from the feature header in unified-mode-migration.feature so behave
executes the registered migration coverage, while keeping any narrower
scenario-level tags only if needed.
In `@tests/e2e/features/unified-mode-synthesis.feature`:
- Line 1: Remove the feature-level `@skip` tag from unified-mode-synthesis.feature
so the six synthesis scenarios are no longer disabled. Update the feature header
in tests/e2e/features/unified-mode-synthesis.feature to keep the existing
`@e2e_group_2` tag but drop `@skip`, ensuring the scenarios run normally.
In `@tests/e2e/features/unified-mode-validation.feature`:
- Line 1: The feature-level `@skip` tag on unified-mode-validation.feature is
preventing both scenarios from running, including the new --migrate-config error
path. Remove the top-level `@skip` from the feature declaration so the validation
scenarios execute normally, while keeping the existing scenario-level tags or
any selective skips only where explicitly needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b3a2ebc-72c5-48aa-8f43-3ff13b1d42ec
📒 Files selected for processing (5)
tests/e2e/features/unified-mode-boot.featuretests/e2e/features/unified-mode-legacy.featuretests/e2e/features/unified-mode-migration.featuretests/e2e/features/unified-mode-synthesis.featuretests/e2e/features/unified-mode-validation.feature
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{py,feature}
📄 CodeRabbit inference engine (AGENTS.md)
Use behave (BDD) framework for end-to-end testing with Gherkin feature files
Files:
tests/e2e/features/unified-mode-validation.featuretests/e2e/features/unified-mode-legacy.featuretests/e2e/features/unified-mode-migration.featuretests/e2e/features/unified-mode-boot.featuretests/e2e/features/unified-mode-synthesis.feature
🔇 Additional comments (3)
tests/e2e/features/unified-mode-boot.feature (2)
47-60: Still missing query coverage for the profile-path variants.This is the same gap already flagged earlier: both relative and absolute profile-path scenarios stop at
readiness, so they still won't catch regressions where startup succeeds but request serving fails.Also applies to: 101-116
13-44: LGTM!Also applies to: 65-98
tests/e2e/features/unified-mode-migration.feature (1)
27-29: The Given/When ordering is still inverted in these scenarios.This is the same BDD step-ordering issue already called out in the earlier review and it still applies here.
Also applies to: 43-45
| @@ -0,0 +1,116 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Feature files carrying a top-level skip tag:"
rg -n '^@.*\bskip\b' tests/e2e/features
echo
echo "Behave hook handling for skip-related tags:"
fd -HI 'environment.py' tests/e2e . -x sh -c '
echo "--- $1 ---"
rg -n -C3 "skip|skip-in-server-mode|skip-in-library-mode" "$1" || true
' sh {}Repository: lightspeed-core/lightspeed-stack
Length of output: 8141
Remove the feature-level @skip unless this suite is meant to stay disabled. tests/e2e/features/environment.py skips any scenario tagged @skip, so this feature will never execute as written.
🤖 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/e2e/features/unified-mode-boot.feature` at line 1, The feature is
currently tagged with `@skip`, which prevents every scenario in unified-mode-boot
from running because environment.py skips any scenario carrying that tag. Remove
the feature-level `@skip` from unified-mode-boot.feature unless this suite is
intentionally disabled, and keep the existing e2e_group_2 tag so the scenarios
can execute normally.
| @@ -0,0 +1,39 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how `@skip` is handled in the E2E harness and where it is used.
fd -i 'environment.py' tests/e2e .
rg -n -C2 --glob 'tests/e2e/**' '`@skip`\b|`@skip-in-server-mode`|`@skip-in-library-mode`'
rg -n -C2 --glob '*' 'behave|--tags|`@skip`|skip-in-server-mode|skip-in-library-mode'Repository: lightspeed-core/lightspeed-stack
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== feature file ==\n'
cat -n tests/e2e/features/unified-mode-legacy.feature
printf '\n== environment.py ==\n'
cat -n tests/e2e/features/environment.py
printf '\n== tag references in e2e ==\n'
rg -n -C2 '`@skip`\b|`@skip-in-server-mode`|`@skip-in-library-mode`|before_tag|after_tag|active_tags|tag_expression|behave' tests/e2eRepository: lightspeed-core/lightspeed-stack
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('tests/e2e/features/unified-mode-legacy.feature')
text = p.read_text()
print('top tags:', text.splitlines()[:5])
print('has scenario tags:', '`@skip-in-server-mode`' in text, '`@skip-in-library-mode`' in text)
PYRepository: lightspeed-core/lightspeed-stack
Length of output: 365
Remove the feature-level @skip tag. tests/e2e/features/unified-mode-legacy.feature:1 skips both scenarios, so this legacy coverage never runs; the scenario-level mode tags already gate the two cases.
🤖 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/e2e/features/unified-mode-legacy.feature` at line 1, Remove the
feature-level `@skip` tag from the unified-mode-legacy feature so the scenarios
are no longer skipped entirely; keep the existing scenario-level mode tags to
control which legacy cases run. Update the feature header in
tests/e2e/features/unified-mode-legacy.feature so only the intended scenario
gating remains.
| @@ -0,0 +1,53 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the file-level @skip.
Line 1 skips the entire feature, so none of these newly added migration scenarios will execute even though this PR registers them as E2E coverage. That leaves the migration path untested in CI. As per coding guidelines, tests/e2e/**/*.{py,feature} should be used for behave-based end-to-end testing, and a file-level skip prevents that coverage from running.
🤖 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/e2e/features/unified-mode-migration.feature` at line 1, The feature is
currently marked with a file-level `@skip`, which prevents all unified mode
migration scenarios from running in E2E. Remove the `@skip` from the feature
header in unified-mode-migration.feature so behave executes the registered
migration coverage, while keeping any narrower scenario-level tags only if
needed.
Source: Coding guidelines
| @@ -0,0 +1,59 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how `@skip` is handled in this repo's behave setup and where else it is used.
fd -i 'environment.py' tests . -x sh -c 'echo "=== $1 ==="; sed -n "1,220p" "$1"' sh {}
echo
rg -n -C2 '`@skip`\b|skip-in-server-mode|skip-in-library-mode' tests/e2eRepository: lightspeed-core/lightspeed-stack
Length of output: 32630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tests/e2e/features/unified-mode-synthesis.feature ==="
sed -n '1,120p' tests/e2e/features/unified-mode-synthesis.feature
echo
echo "=== Behave availability ==="
python3 - <<'PY'
try:
import behave
print("behave:", behave.__version__)
except Exception as e:
print("behave_import_error:", type(e).__name__, e)
PYRepository: lightspeed-core/lightspeed-stack
Length of output: 3113
Remove the feature-level @skip tag. This disables all six scenarios in tests/e2e/features/unified-mode-synthesis.feature, so the new synthesis coverage never runs.
🤖 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/e2e/features/unified-mode-synthesis.feature` at line 1, Remove the
feature-level `@skip` tag from unified-mode-synthesis.feature so the six synthesis
scenarios are no longer disabled. Update the feature header in
tests/e2e/features/unified-mode-synthesis.feature to keep the existing
`@e2e_group_2` tag but drop `@skip`, ensuring the scenarios run normally.
| @@ -0,0 +1,19 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how `@skip` is handled in the E2E harness and where it is used.
fd -i 'environment.py' tests/e2e .
rg -n -C2 --glob 'tests/e2e/**' '`@skip`\b|`@skip-in-server-mode`|`@skip-in-library-mode`'
rg -n -C2 --glob '*' 'behave|--tags|`@skip`|skip-in-server-mode|skip-in-library-mode'Repository: lightspeed-core/lightspeed-stack
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== feature file ==\n'
cat -n tests/e2e/features/unified-mode-validation.feature
printf '\n== environment.py outline ==\n'
ast-grep outline tests/e2e/features/environment.py --view expanded || true
printf '\n== environment.py ==\n'
cat -n tests/e2e/features/environment.py
printf '\n== skip tag usage ==\n'
rg -n -C2 '`@skip`\b|skip-in-server-mode|skip-in-library-mode|--tags|behave' tests/e2e . --glob '!**/.git/**'Repository: lightspeed-core/lightspeed-stack
Length of output: 50390
Remove the feature-level @skip tag.
That tag skips both validation scenarios, so the new --migrate-config error path never runs.
🤖 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/e2e/features/unified-mode-validation.feature` at line 1, The
feature-level `@skip` tag on unified-mode-validation.feature is preventing both
scenarios from running, including the new --migrate-config error path. Remove
the top-level `@skip` from the feature declaration so the validation scenarios
execute normally, while keeping the existing scenario-level tags or any
selective skips only where explicitly needed.
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
LGTM. The When/Given steps switch looked weird at 1st glance, but seems to be ok.
max-svistunov
left a comment
max-svistunov
left a comment
There was a problem hiding this comment.
Nice work -- these track the spec's acceptance surface closely and reuse the existing steps well. A few things before merge: two I'd want addressed (the byte-identical assertion on the round-trip, and the missing deprecation-WARN coverage for R2), one optional (the R11 version-mismatch case), and a Gherkin nit.
Btw, reviewing surfaced a couple of step-implementation notes for 2343 -- captured on LCORE-2343, nothing blocking here.
| Scenario: migrate then synthesize round-trips to the original run.yaml | ||
| When lightspeed-stack --migrate-config is run for the legacy migration fixture pair | ||
| And the active unified configuration is synthesized to run.yaml | ||
| Then the synthesized run.yaml is byte-identical to the legacy migration fixture run.yaml |
There was a problem hiding this comment.
This asserts byte-identical, but the spec's round-trip test (see R4 in the design doc) is dict-equality, not byte-equality. Byte-identical YAML is fragile (things like key ordering, quote style, comment loss, anchor expansion all break it), and the synthesizer has no reason to preserve any of them.
How about:
Then the synthesized run.yaml parses to the same data as the legacy migration fixture run.yaml(The scenario titles on L26/L42 saying "byte-identical behavior" are fine -- behavior is byte-identical per the spec, it's just the file itself that isn't.)
| # --- library mode (@skip-in-server-mode) --- | ||
|
|
||
| @skip-in-server-mode | ||
| Scenario: Legacy two-file configuration still boots and serves requests in library mode |
There was a problem hiding this comment.
R2 has two halves -- still boots and serves AND one startup deprecation WARN in 0.6; no WARN in unified mode. This covers the boot half but not the WARN, and the WARN is really the point of R2 (it's how we prove the deprecation is actually live).
The log-assertion plumbing already exists -- synthesis.feature does Then the lightspeed-stack container logs contain synthesized run.yaml, so a Then the lightspeed-stack container logs contain a legacy-config deprecation warning here would mirror it (that exact string is just illustrative -- the real WARN wording is 2339's). Could you add a WARN-present scenario in legacy mode, and ideally a WARN-absent one in unified mode over in boot.feature?
| Scenario: llama_stack.config together with library_client_config_path fails at load | ||
| Given The service uses the lightspeed-stack-invalid-config-and-legacy.yaml configuration | ||
| When configuration validation is attempted for the active configuration | ||
| Then the validation error contains --migrate-config |
There was a problem hiding this comment.
R11 in the spec is verified by both unit and e2e tests. The e2e part covers this case: a config declares a config_format_version, but that version does not match the config's actual shape, so loading must fail. This case is not tested anywhere yet -- the boot scenarios exercise shape auto-detection, but not the version mismatch.
Could you add one more scenario here, like the two above? For example, a config whose config_format_version says legacy while its body is unified, asserting that loading fails.
Lower priority than the deprecation-WARN gap; fine to move to a follow-up PR if you would rather keep this one small. I am flagging it because R11 lists e2e.
| @skip-in-server-mode | ||
| Scenario: Migrated unified configuration drives byte-identical Llama Stack behavior in library mode | ||
| When lightspeed-stack --migrate-config is run for the legacy migration fixture pair | ||
| Given The service uses the lightspeed-stack-unified-migrated.yaml configuration |
There was a problem hiding this comment.
Nit: Given after When reads backwards here -- the migrate is logically the precondition for "The service uses ...migrated.yaml". Behave parses it fine, so it is only cosmetic, but reversing the two reads more naturally:
Given lightspeed-stack --migrate-config has been run for the legacy migration fixture pair
And The service uses the lightspeed-stack-unified-migrated.yaml configurationSame on L44.
There was a problem hiding this comment.
this was copy paste error, but completely agree that it reads weirdly. To keep the look unified across all scenarios, I will fix add the given step
When implementing, create this step as a step, this allows it to serve as both Given or When step
3 participants
Description
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit