LCORE-2343: implement behave step definitions for unified-mode feature files - #2448
Open
max-svistunov wants to merge 8 commits into
Open
Conversation
Contributor
|
Warning Review limit reachedNext included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (31)
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 |
max-svistunov
force-pushed
the
lcore-2343-unified-mode-step-definitions
branch
from
August 17, 2026 09:53
ecc4b5b to
ba9f5eb
Compare
5 tasks
Create tests/e2e/configuration/unified-mode/ with library-mode/server-mode
variants (same two-subdir layout configure_service resolves), covering the
five unified-mode feature files:
- unified-providers: minimal unified config driven only by top-level
inference.providers over the default baseline (R1/S5); openai-specific.
- unified-config-only / unified-relative-profile: profile: run.yaml — the
CI-materialized repo-root run.yaml as baseline, provider-agnostic (R1/R8;
two files because the features pin the intents separately).
- unified-absolute-profile: container-absolute profile paths, differing per
mode subdir (/app-root vs /opt/app-root mounts).
- unified-native-override-{scalar,list}: R5 replacement semantics fixtures,
synthesis-only, never booted.
- invalid-{providers,config}-and-legacy: mutual-exclusion validation
fixtures (R3); invalid-version-legacy-unified-body: R11 marker mismatch
(needs LCORE-2872's cross-validation to fail for the right reason).
- legacy-for-migration: legacy half of the migration fixture pair, kept
free of enrichment sections so migrate-then-synthesize round-trips
losslessly (LCORE-3370).
Every fixture is validated against the real Configuration model: bootable
ones load, invalid ones fail with the intended error. The test-generated
lightspeed-stack-unified-migrated.yaml is gitignored, and the directory
README documents each fixture's purpose.
…d fixture LCORE-2342 migrated the standard library-mode baseline to unified mode (config.profile: run.yaml), which silently changed what unified-mode-legacy.feature's library scenario exercises: it now boots the unified baseline, not the deprecated two-file path, so R2's library-mode legacy coverage was gone. Add lightspeed-stack-legacy.yaml — identical to the baseline except its llama_stack block uses the true legacy shape (use_as_library_client + library_client_config_path: run.yaml, no synthesis input) — and point the library scenario's Given at it. This is the one deliberate Gherkin edit in LCORE-2343, agreed with Maxim in planning; the server-mode scenario is untouched since container-side enrichment there is genuinely legacy.
Add tests/e2e/features/steps/unified_mode.py — the 16 step patterns the validation, migration, and synthesis features need (boot and legacy resolve entirely through existing generic steps). Per the planning decisions: - All artifact steps operate on the ON-DISK configuration (the repo-root lightspeed-stack.yaml copy configure_service applied), never the live service. - Validation runs the service CLI (--dump-configuration) as a black-box subprocess from the repo root and asserts a non-zero exit, so the error-contains assertions can never pass against a healthy load. - Migration runs the real --migrate-config CLI; the output lands in the active mode subdir under the gitignored name later Gherkin references, and is cleaned up per scenario. - Synthesis runs the config CLI exactly as the server entrypoint does (unified auto-detection -> synthesize_to_file, giving the 0600 mode the permissions scenario asserts). Round-trip and override assertions parse YAML and compare data, never bytes; override assertions are self-referential against the fixture's native_override and additionally assert the baseline differed, so replacements can't pass vacuously. - The --synthesized-config-output scenario launches a short-lived local service from the library-mode fixture variant on a rewritten port and polls for the custom output file (the flag is library-mode-only by design; running containers cannot be restarted with new CLI args). - The startup-log step is mode-aware: in server mode the synthesis evidence is emitted by the llama-stack container (entrypoint + CLI), not the lightspeed-stack container the Gherkin names — asserted against the synthesizing container with the rationale documented in the step. behave --dry-run over the five features: 24 scenarios, 200 steps, zero undefined.
Tag all five unified-mode features @skip-in-prow: the new steps rely on Docker containers and local subprocesses, neither of which exists in the Prow environment (existing convention, handled in before_scenario). Add an @openai-only tag on the two inference.providers boot scenarios and a matching before_scenario skip keyed on E2E_DEFAULT_PROVIDER_OVERRIDE: the providers workflow runs the full unsharded test list against azure/watsonx/bedrock matrices, and the unified-providers fixture hardcodes an openai provider that cannot serve those models' queries. Profile-based fixtures stay provider-agnostic (they consume the CI-materialized run.yaml) and need no gating.
Remove the @Skip placeholder tag from the five unified-mode features: the step definitions and fixtures they need now exist. The features keep @skip-in-prow (Docker/subprocess dependencies) and stay in test_list.txt under @e2e_group_2, so CI shards pick them up via 'not @Skip and @e2e_group_2' and local runs via --tags=-skip.
restart_container waits on docker health, but docker can report healthy before uvicorn binds the published port — the exact race wait_for_lightspeed_stack_http_ready documents and was, until now, only closed in the proxy steps. The unified-mode boot scenarios are the slowest restarts in the suite (first unified/default-baseline boots) and hit that window reliably: the restart step passed while the following readiness GET got connection-refused. Call the existing HTTP-ready wait from restart_container for the lightspeed-stack container, closing the race for every restarting scenario; when the port is already accepting, the first poll returns immediately.
--migrate-config writes its output 0600 (R10: migrated files may carry lifted secrets), but the boot scenarios copy that file to the repo root for the container to consume, and the container user cannot read a host-owned 0600 file — the migrated-config boot scenario died on config read. Relax the harness copy to 0644 after a successful migration; the fixture pair is env-reference-only by design, so no secret can leak.
The library-mode legacy fixture declared its BYOK store under a top-level byok_rag key with rag_type, and its tool retrieval sources under rag.tool. LCORE-1426 (commit c1de7f9) refactored RAG configuration into a single rag section: stores moved to rag.byok.stores, retrieval sources to rag.retrieval.tool.sources, and RagStore.rag_type was replaced by RagStore.backend, whose validator accepts only the values in SUPPORTED_RAG_BACKENDS (faiss, pgvector). Configuration models inherit ConfigurationBase with extra=forbid, so after rebasing onto main this fixture raised two extra_forbidden validation errors (rag.tool and byok_rag) and the config could not be loaded at all, failing every legacy library-mode scenario that consumes it. Move the store under rag.byok.stores, replace rag_type: inline::faiss with backend: faiss, and nest the retrieval source list under rag.retrieval.tool.sources. score_multiplier, db_path, embedding_model, embedding_dimension and vector_db_id are unchanged and remain valid RagStore fields. The source id stays e2e-test-docs so the validate_retrieval_sources model validator still resolves it against the declared store.
max-svistunov
force-pushed
the
lcore-2343-unified-mode-step-definitions
branch
from
August 26, 2026 11:19
ba9f5eb to
8e4df5a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements LCORE-2343: behave step definitions, fixtures, and gating for the five
unified-mode-*.featurefiles authored spec-first in LCORE-2341 (PR #2020), and unskips them.tests/e2e/features/steps/unified_mode.py, 16 new patterns — boot/legacy resolve entirely through existing generic steps): validation runs the service CLI (--dump-configuration) as a black-box subprocess against the on-disk active configuration and asserts a non-zero exit; migration runs the real--migrate-configCLI with output landing in the active mode subdir (gitignored, cleaned per scenario, relaxed to 0644 so the container user can boot the copy); synthesis runs the config CLI exactly as the server entrypoint does (unified auto-detect →synthesize_to_file, giving the 0600 mode the permissions scenario asserts). All comparisons are YAML-data equality, never bytes; override assertions are self-referential against the fixture'snative_overrideand additionally assert the baseline differed, so replacements cannot pass vacuously. The--synthesized-config-outputscenario launches a short-lived local service from the library-mode fixture variant on a rewritten port and polls for the custom output.tests/e2e/configuration/unified-mode/{library-mode,server-mode}/, 20 files + README): every fixture validated against the realConfigurationmodel — bootable ones load in both modes, the three invalid ones fail with exactly the intended errors. Profile-based fixtures consume the CI-materialized./run.yaml, staying provider-agnostic across the providers matrix.lightspeed-stack-legacy.yamlfixture restores it, with a one-line Given edit inunified-mode-legacy.feature(the single deliberate Gherkin edit; rationale in a Gherkin comment and the commit message).@skip-in-prow(Docker/subprocess dependencies); the two openai-hardcoded boot scenarios tagged@openai-onlywith abefore_scenarioskip keyed onE2E_DEFAULT_PROVIDER_OVERRIDE, so the providers matrix (azure/watsonx/bedrock full-list runs) skips them instead of failing.restart_containerwaited on docker health, which can report healthy before uvicorn binds the port (the documented racewait_for_lightspeed_stack_http_readyexists for, previously closed only in proxy steps) — the unified first boots are the slowest restarts in the suite and hit it reliably; the HTTP-ready wait is now wired into every lightspeed-stack restart.@skipremoved from the five features. Rebased over the LCORE-3537 e2e rework: the features carry main's@cfg_unifiedaffinity tag (plus@skip-in-prow), so the existingcfg_unifiedCI shard picks them up on unskip. (Two mid-branch commit messages predate the rework and mention the old@e2e_group_2sharding; the final state is@cfg_unified.)Depends on: LCORE-2338 (PR #2319 — server-mode synthesis path; still open). LCORE-2872 (config_format_version) has merged, so the validation scenario now fails/passes for the right reason.
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
Verified against a local merge of this branch with PR #2317 and PR #2319, docker-compose stacks in both modes, live OpenAI key (pre-rebase, 2026-08-04); step-matching re-verified after rebasing onto the LCORE-3537 e2e rework:
uv run behave --dry-run tests/e2e/features/unified-mode-*.featureActual (on the rebased head): 24 scenarios, 200 steps, zero undefined.
E2E_DEPLOYMENT_MODE=library uv run behave tests/e2e/features/unified-mode-*.featureActual: 17 scenarios passed, 0 failed, 7 skipped (server-only variants) — includes migrate→synthesize round-trip, the custom
--synthesized-config-outputservice subprocess, and the startup-log check.E2E_DEPLOYMENT_MODE=serverandE2E_LLAMA_HOSTNAME=llama-stackexported (the validation subprocess resolves the fixture's${env.E2E_LLAMA_HOSTNAME}; CI sets it at job level).Actual: 17 scenarios passed, 0 failed, 7 skipped (library-only variants) — all validation, migration, and synthesis scenarios green, mode-aware log step verified against the llama-stack container.
uv run make format— clean on the rebased head. Fullmake verifydeferred to CI (local machine constraint); the last full local verify on this branch was clean except the 14 pre-existing mypy errors intests/unit/utils/test_models_dumper.py.