Repository navigation
fix(OMN-15263): supply DEV_REDPANDA_ADVERTISE_HOST in every compose-render fixture + generalize the required-env coverage gate to all four - #2504
Conversation
…ender fixture + generalize the required-env coverage gate to all four OMN-15173 (#2495) switched docker-compose.infra.yml to the compose `:?` fail-fast form for DEV_REDPANDA_ADVERTISE_HOST and added the var to exactly one render fixture. Redpanda`s `command:` block is interpolated by `docker compose config` regardless of `--profile`, so every layered render started exiting non-zero on hosted CI: 12 failures in Tests (Split 2/15), cascading to CI Tests Gate -> Test-Failure Ratchet Gate -> the required CI Summary, on any PR whose selector escalated to the full suite. Fix (1): supply the var (render-only `localhost`) in the three hermetic render fixtures - prod, judge, stability-test. The compose fail-fast is UNCHANGED; nothing regains a silent default. Fix (2), the actual defect: tests/ci/test_compose_required_env_coverage.py exists to catch "a `:?` var was added to compose but not to the fixture" (OMN-5240) and did not fire, because FIXTURE_FILE was hardcoded to one of four render fixtures. It now checks every registered fixture (env dict keys union `--env-file` contents), fails closed on any unregistered `tests/integration/**/*compose_render*.py` module, and carries an explicit, justified `intentionally_unset` hatch for the dev fixture that must NOT set the var (its OMN-15173 counter-test proves the unset render fails). Also adds test_dev_advertise_host_keeps_fail_fast_form: giving compose a `:-localhost` default back would turn every render green while restoring the off-host regression OMN-15173 removed. That shortcut is now RED, on hosts with or without Docker. tests/integration/docker/test_docker_integration.py: the render env dict is lifted from the test body to module-level COMPOSE_CONFIG_RENDER_ENV so the gate extracts it the same way as the other three. Pure move, same values. Evidence: RED (fix 1 reverted) = 3 parametrized gate failures naming DEV_REDPANDA_ADVERTISE_HOST and each fixture; GREEN after = 8 passed. Real `docker compose config` renders on a Docker host: prod/judge/stability all exit 1 without the var and exit 0 with it; dev lane still exits 1 when the var is unset and honors an explicit value. Old gate re-run against pristine dev: 43 required / 0 missing - it passed while CI was red.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
#5166) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#2504 * evidence: OCC companion self-bind for #5166 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical findings | No |
blocked |
CRITICAL findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524)
…ector (#2505) * fix(OMN-15245): fail-closed changed-test coverage in the governed selector The governed selector could drop a test file the diff itself changed. Two recorded live instances, both green CI runs that never collected the changed test modules: * OMN-15218 / #2493 -- a scripts/ + tests/scripts/ diff selected ["tests/unit/"] (22053 tests, none of them the 47 new ones). * OMN-15263 / #2504 -- a six-test-file diff selected ["tests/ci/"] (run 30296123866, Detect Changes job 90082641865); the five changed tests/integration/** modules were never collected on the PR that existed to repair them. Invariant: any CHANGED path under tests/ is covered by the emitted selection (its own directory at minimum). Narrowing may add tests, never drop one the diff touched. Applied last in _resolve() so it sees every other mapping. Also: * scripts/** now maps to tests/scripts/ + tests/unit/scripts/ -- the two families that actually exercise scripts/. Previously scripts/ produced no selection at all and fell through to the blanket tests/unit/ fallback. * New CHANGED_TEST_UNNARROWABLE full-suite escalation: a changed test module directly under tests/ has no containing directory below tests/ itself. * UNRUNNABLE_TEST_PREFIXES documents the families the pytest job structurally cannot run (tests/integration/docker/ is --ignored by both pytest steps and has its own gate in docker-build.yml; tests/chaos/ and tests/performance/ are marker-deselected). Selecting them cannot make them run and would make pytest exit 5 when one is the sole selected path. * Consumer seam: prepush_smart_tests.sh filters tests/integration/ out of its pytest invocation (it also passes --ignore=tests/integration), so the new integration selections cannot wedge a push on exit 5. RED-first, exists-but-wrong (not absence): the new tests fail 14/14 against the pre-fix selector, including both recorded replays; the hook seam test fails 2/2 when the filter pattern is wrong rather than missing. * chore(OMN-15245): re-trigger CI after Evidence-Source: OCC#5190 landed in the PR body --------- Co-authored-by: jonahgabriel <jonahgabriel@users.noreply.github.com>
Ticket
OMN-15263 —
OMN-15173'sDEV_REDPANDA_ADVERTISE_HOST:?fail-fast breaks every compose-render test; the required-env coverage gate is blind to 3 of the 4 render fixtures.Root cause
#2495 (OMN-15173) switched
docker/docker-compose.infra.ymlto the compose:?fail-fast form forDEV_REDPANDA_ADVERTISE_HOST. That change is correct and stays. Redpanda'scommand:block is interpolated bydocker compose configregardless of--profile, so every render that layers the base infra file now exits non-zero unless the var is set — and #2495 added it to exactly one of four render fixtures. The other three build their env hermetically (env=replaced, not inherited).Blast radius, measured on omnibase_infra#2502 (run 30290604529, job 90060230679): 12 failures — 1 judge + 1 prod + 10 stability-test — all
subprocess.CalledProcessErrorfromdocker compose config, cascadingTests (Split 2/15)→CI Tests Gate→Test-Failure Ratchet Gate→ the requiredCI Summary. Reproduced identically on #2500 on different hosted runners. Invisible locally: the render testsskipwheredocker composeis absent, so a clean full local suite was honest but blind.Changes
1. Supply the var in the three hermetic render fixtures (
test_prod_runtime_compose_render.py,test_judge_compose_render.py,test_stability_test_runtime_compose_render.py), valuelocalhost,# kafka-fallback-ok — test fixture, matching #2495's own fixture. The compose fail-fast is byte-unchanged —docker/is not touched by this PR at all. The wrong fix here would be handing compose a:-localhostdefault back; that resurrects exactly the silent off-host regression OMN-15173 removed.localhostspecifically, not a synthetic hostname: prod and stability both assert the basecommand:was actually overridden by the overlay ("localhost:19092" not in redpanda_command,"100.109.203.94:49092" not in ...). A non-localhost dummy would silently defeat that leak detector.2. The actual defect — generalize
tests/ci/test_compose_required_env_coverage.py. That gate exists precisely to catch "a:?var was added to compose but not to the fixture" (OMN-5240) and did not fire, becauseFIXTURE_FILEwas hardcoded to one of four call sites. It now:--env-filefiles that fixture passes to compose — judge legitimately gets 4 of its vars fromdocker/judge.env.example, so a naive dict-only check would have been a false positive on day one;tests/integration/**/*compose_render*.pymodule not inRENDER_FIXTURESis RED. A fifth render fixture cannot silently escape coverage the way three just did;intentionally_unsethatch, used by exactly one entry:test_dev_runtime_compose_render.pymust not set the var, because its OMN-15173 counter-test proves the unset render fails. The hatch is itself gated —test_intentionally_unset_vars_are_justifiedrequires the var to still be:?-required in compose (stale exemptions go RED), requires a real reason string, and requires the exempted fixture to actually mention the var.3. Counter-test for OMN-15173 —
test_dev_advertise_host_keeps_fail_fast_form. Reverting compose to${DEV_REDPANDA_ADVERTISE_HOST:-localhost}would also turn every render green, while restoring the regression. That shortcut is now RED, and it is static, so it fires on hosts with and without Docker.4.
tests/integration/docker/test_docker_integration.py: the render env dict is lifted out of the test body into module-levelCOMPOSE_CONFIG_RENDER_ENVso the gate extracts all four fixtures uniformly. Pure move — same keys, same values.Seams
:?var name → fixture dict key. Matched field-by-field by the gate itself, for all 5 registered fixtures, not by two independent suites. The gate is the cross-boundary regression test for this seam.--env-file. The gate parses each fixture's own--env-filearguments and reads those files, so it tracks drift instead of hardcoding a list; a fixture pointing at a non-existent env file is an assertion failure.dictliterals withstrkeys count — a dict nested in a test body is never silently credited as coverage, which is the shape that let the docker-integration fixture look covered.Verification
RED-first (all captured before/after, artifacts in the session scratchpad):
test_all_required_compose_vars_in_fixture[prod|judge|stability], each namingDEV_REDPANDA_ADVERTISE_HOSTand its fixturedev@6bc1e7ff:?var in compose, no fixture touched:-localhosttest_dev_advertise_host_keeps_fail_fast_form+ the stale-exemption check*compose_render*.pyaddedReal
docker compose configrenders on a Docker host (non-mutatingconfigonly — noup, no lane touched), using the exact fixture env dicts imported from these modules:The last two rows are the counter-proof: the OMN-15173 runtime fail-fast still fires for a real dev-lane bring-up path after this fix, and an explicit value is still honored verbatim.
Gates — run on
.200per rule 11a, patch-transferred with per-filesha256verified equal on both hosts before the run (no vacuous green from the edit-locality trap):ruff check+ruff format --checkovertests/ci,tests/integration/docker,tests/integration/infra— cleanmypy tests/ci/test_compose_required_env_coverage.py— cleanpre-commit run --filesover all 6 changed files — clean, no hook modified a filepytestgate + all 4 render fixtures — 8 passed, 22 skipped;pytestdocker-integration + kafka-env unit — 10 passed, 15 skippedStated plainly, not counted as a pass: the 12 render tests skip on
.200and on this Mac (nodocker compose— this is the exact blindness that let the regression land). Their authoritative GREEN is the.201realdocker compose configrender table above plus the dedicated hosted-runnerCompose Required-Env Coverage (OMN-5439)job (run 30296123866, job 90081728592) exercising all 5 parametrized fixtures. Corrected 2026-07-27T19:4xZ by independent verification: an earlier revision of this line claimed the green came from this PR's ownTests (Split 2/15). That is false — the governed selector emitted{"selected_paths":["tests/ci/"],"is_full_suite":false}(Detect Changes job 90082641865), soTests (Split 1/1)rantests/ci/only (975 passed) and none of the five changedtests/integration/**render modules were collected on their own PR. That selector gap is recorded as a second instance on OMN-15245. A skip was never counted as a pass.Notes
src/, zerodocker/, zero compose, zero lane mutation, no deploy.Tests (Split 2/15)red is this regression, not their diffs. It clears on rebase/rerun once this lands.Evidence-Sourceline.--auto, no drafts, no skip tokens.Refs: OMN-15263, OMN-15173 (#2495), OMN-5240, OMN-15234 (#2502), OMN-15255 (#2500), OMN-13969/13970
Evidence-Source: OCC#5166
Evidence-Ticket: OMN-15263