Repository navigation
fix(OMN-15245): fail-closed changed-test coverage in the governed selector - #2505
Conversation
…ector 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.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 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 (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
| 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)
…ibase_infra#2505 (#5190) * evidence(OMN-15245): author OCC companion for OmniNode-ai/omnibase_infra#2505 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 8588047e8f70e202e938ddeae68d462eafe23edd. * evidence(OMN-15245): self-bind OCC#5190 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
Ticket
OMN-15245 — omnibase_infra governed selector picks only
tests/unit/for ascripts/+tests/scripts/diff; second recorded instance dropped five changedtests/integration/**modules.Defect
The governed selector (
scripts/ci/detect_test_paths.py+test_selection_adjacency.yaml) could drop a test file the diff itself changed. Two live instances, both green CI runs that never collected the changed test modules:scripts/**+ 2 ×tests/scripts/**(47 new tests)["tests/unit/"]["tests/ci/"](run 30296123866, Detect Changes job 90082641865)tests/integration/**modules were never collected on the PR that existed to repair themBoth reproduce deterministically against
compute_selection()atdev@0b65877c.Named per the ticket DoD — this is all three of the candidate causes at once, not one:
_resolve): onlysrc/omnibase_infra/**,tests/unit/**and the CI-process family contributed anything.tests/integration/**,tests/scripts/**,tests/replay/**,tests/nodes/**,tests/services/**,tests/audit/**,tests/redeploy/**contributed nothing, andscripts/**(deploy-path + governance-guard code) contributed nothing either.["tests/unit/"]— it never escalated. Confirmed live, not inferred.test_integration_test_only_change_does_not_select_unit_testsassertedpaths == []on the premise that "the integration job runs all integration tests on every PR anyway". That premise is false. No CI job runs all oftests/integration/on a PR —test-parallelis the only job that runs it at all, and in smart mode it runs onlyselected_paths.Fix
Invariant (fail-closed): any CHANGED path under
tests/is covered by the emitted selection — its own directory at minimum. Narrowing may add tests; it may never drop one the diff touched._uncovered_changed_test_dirs()runs last in_resolve(), so it sees everything the other mappings already cover and is purely additive (a changed test already covered contributes nothing).scripts/**→tests/scripts/+tests/unit/scripts/— the two families that actually exercisescripts/. This replaces the blankettests/unit/fallback for script diffs, which had no causal relationship to them (nothing undersrc/importsscripts/).CHANGED_TEST_UNNARROWABLEfull-suite escalation: a changed test module directly undertests/root has no containing directory belowtests/itself. Emittingtests/as a smart selection would run the whole suite under the smart step's split count; escalate to the real 15-split full suite instead.UNRUNNABLE_TEST_PREFIXES— documented, and not a narrowing carve-out.tests/integration/docker/is--ignored by both pytest steps inci.yml(full suite included) and has its own gate indocker-build.ymlwhose paths filter istests/integration/docker/**;tests/chaos/andtests/performance/are deselected by the job's marker expression. Selecting them cannot make them run, and when one is the sole selected path pytest exits 5 ("no tests ran") — a red gate that ran nothing.Seams
.github/workflows/ci.ymlsmart pytest step. Step passes--ignore=tests/integration/docker; the selector never emits that prefix, so no exit-5 and no ignored-path no-op.selected_pathsstill satisfies theTestPathpattern (trailing-slash directories) — the output contract is unchanged, no workflow edit needed.scripts/hooks/prepush_smart_tests.sh. This is the seam the invariant would have broken: the hook is unit-scoped by design and passes--ignore=tests/integration, so a selection containingtests/integration/**would have collected nothing from it and exited 5 on any diff whose only selected path was an integration directory — blocking a push on a gate that ran nothing. The hook now filters those out visibly (filter_prepush_runnable_paths, deferred-to-CI log line), and that function is executed by a test, not grepped.Verification
RED-first, against exists-but-wrong (not absence):
test_omn15218_scripts_diff_selects_the_changed_test_modules,test_omn15263_test_file_diff_selects_the_changed_integration_modules) and all 7 dropped path families. Post-fix: 61 passed.filter_prepush_runnable_pathspresent but matching the wrong pattern (tests/nonexistent/*), 2 failed / 3 passed; restored → 5 passed. The filter is proven by execution, not by grep.src/omnibase_infra/cli/foo.pydiff still emits exactly["tests/unit/cli/"], 1 split;src/omnibase_infra/services/foo.pystill emits exactly its 5 reverse-dep unit dirs with notests/and notests/integration/**; shared-module escalation still outranks the additive rule; the docs-only exemption (OMN-14753) still selects nothing, including for a.mdundertests/.Replay of both instances through the fixed selector:
Gates — run on
.200(stickybeatz-studio) per rule 11a, patch-transferred with per-filesha256verified equal on both hosts before the run, so no vacuous green from the edit-locality trap:ruff format --check scripts/ tests/unit/scripts/— 287 files already formattedruff check scripts/ci/ tests/unit/scripts/— All checks passedmypy scripts/ci/detect_test_paths.py scripts/ci/test_selection_models.py— no issuespre-commit run --filesover all 5 changed files — 42 hooks passed, 0 failed, no hook modified a filepytest tests/ci/ tests/scripts/ tests/unit/scripts/— 2286 passed, 5 skipped (skips are pre-existing host/env skips: GNUrealpathand a sibling-repo probe)bash scripts/hooks/prepush_smart_tests.shon.200selectedtests/ci/ tests/scripts/ tests/unit/scripts/from this PR's own diff and ran them green — the fixed selector selects the tests that prove the fixed selector.Port check (ticket DoD)
Probed the sibling selectors live at their current
devcheckouts with atests/integration/foo/test_x.py-only diff:['tests/unit/']— same fail-open, changed test not collected. Port filed (see Refs).['tests/unit/']— same class; this is the open OMN-14252 (tests/skills/**), so evidence was added there rather than duplicating a ticket.['tests/integration/']— covers the changed file; not affected by this class.Notes
Evidence-Sourceline added once autobind mints it.--auto, no drafts, no skip tokens, no--no-verify.Refs: OMN-15245, OMN-15218 (#2493), OMN-15263 (#2504), OMN-14252, OMN-14399, OMN-13969/13970, OMN-14194
Evidence-Source: OCC#5190
Evidence-Commit: 669ab8ec505d86866516d7e83684bcef66beec9b
Evidence-Ticket: OMN-15245