Repository navigation
fix(OMN-15755): fail-closed test + type-check on OCC dev-targeting PRs - #6221
jonahgabriel wants to merge 2 commits into
Conversation
test/type-check were unconditionally skipped on every dev-targeting PR (if: ... && (github.event_name != 'pull_request' || github.base_ref != 'dev')). Since dev is this repo's default branch, that meant essentially all OCC traffic merged with zero unit tests and zero type-checking, silently, for a month. The `smart-test-select` job that appears to explain the carve-out (added at the same commit) computed a changed-file-based test selection but was never wired to anything downstream -- no job consumed its selected_paths/matrix/is_full_suite outputs -- so it ran, computed, and discarded, providing zero coverage in its place. Provenance: the base_ref != 'dev' carve-out on test/type-check, and the matching smart-test-select if: gate, were both introduced by e5048f8 (OMN-14098 / PR #3665, 2026-07-07, "OCC evidence-only fast-lane"). Before that commit both jobs ran unconditionally (modulo the pre-existing docs_only fast lane); the commit's own message notes "(type-check + test were already gated on docs_only.)" -- the base_ref clause was new. smart-test-select itself predates that commit (7166bbd / OMN-10763, change-aware selection port) but its output was never consumed by `test` even before OMN-14098 -- `test` has always run the unfiltered `uv run pytest --cov=...` regardless of selector output. The gap the ticket describes is real from day one of smart-test-select's existence; OMN-14098 additionally made it total by skipping the job outright on dev PRs. Fix (simplest fail-closed shape, per CLAUDE.md rule 4's dev-boundary default): remove the base_ref != 'dev' carve-out from both jobs' if: conditions so they run on every non-docs-only PR again, and delete the orphaned smart-test-select job rather than wire it -- OCC's suite is small enough that full-suite-on-dev matches the fail-closed default until a governed selector is deliberately wired (a separate, explicit follow-up, not a silent-skip default). Adds an explicit strict success-only check for test/type-check in ci-summary (mirroring the OMN-15484 merge-hold-gate pattern) since both jobs are LEGITIMATELY skipped on docs-only PRs -- the check distinguishes that from an illegitimate skip, which the existing generic contains(needs.*.result, 'failure'/'cancelled') rollup could not. Dev-tip audit (before this fix, on origin/dev @ c9b86d1, per OMN-15731's instruction to check for latent breakage this hole allowed): full suite `uv run pytest tests/ -q` -> 5915 passed, 47 skipped, 0 failed. `uv run mypy src/ tests/` -> Success: no issues found in 396 source files. Both clean -- this fix does not need to carry any dev-tip test repair as part of its payload. New regression test tests/unit/scripts/test_test_typecheck_wiring_omn15731.py renders the real ci-summary bash (not a hand-built fixture) and executes it under bash: success passes; a skip on either job fails closed outside the docs-only lane; the docs-only fast lane's legitimate skip still passes; stripping only the new strict block restores the pre-fix bypass (RED-before control); and the base_ref carve-out / smart-test-select job are asserted gone from the live file. Tracking ticket: OMN-15755 (filed by the OMN-15731 label-gated-CI pilot lane after it found and explicitly scoped out this same gap while building OCC#6216 -- see that ticket's "Related" section for the full disclosure chain). This PR is the direct code fix OMN-15755 asks for. Interaction note: OCC#6216 (OMN-15731 label-gated-CI pilot) has open ci.yml changes in flight (pre-commit gating + a `Pre-commit` strict block in ci-summary) touching different jobs in the same file. Based on origin/dev; whichever of the two PRs lands second should rebase onto the other. Evidence-Source: local pytest + mypy runs above, executed in worktree omni_worktrees/OMN-15731-testfix/onex_change_control on origin/dev @ c9b86d1.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 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 (2)
Comment |
* evidence: OCC companion pass 1 for #6221 * evidence: OCC companion self-bind for #6222 * fix(OMN-15755): yamlfmt-normalize autobind-generated contracts/OMN-15755.yaml The OCC autobind generator wrote a folded (>) summary block whose single unwrapped line exceeds the repo's yamlfmt max_line_length=100. yamlfmt rewraps it at commit time on any touch, which fails Pre-commit on this branch since the file was committed pre-wrapped. Formatting-only: verified content-identical via yaml.safe_load before/after compare (same parsed value, only line-wrap position changed). --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai> Co-authored-by: Jonah Gray <jonah@omninode.ai>
occ-preflight/eligibility on this PR was failing with missing_contracts=["OMN-15755"] because this branch predates onex_change_control#6222 (the OCC companion PR that lands the OMN-15755 ticket contract on dev). Merging dev pulls that contract in so the eligibility check can resolve it against PR head.
Same class as OMN-15755 (test/type-check): schema-purity, no-localhost-fallbacks, imperative-contract-guard, context-integrity-contracts, migration-conflicts, migration-inventory, kafka-boundary-parity, cross-repo-null-contract, seam-contract-coverage, ai-slop-check, no-new-os-environ, and url-authority-gate all carried an undocumented `github.base_ref != 'dev'` clause on top of their legitimate docs_only (OMN-14098) fast-lane guard. Since dev is this repo's default branch, all 12 silently skipped on essentially every ordinary dev PR. 9 of the 12 sat in ci-summary's needs guarded only by the generic failure/cancelled rollup (which doesn't catch skipped); the other 3 (ai-slop-check, no-new-os-environ, url-authority-gate) weren't in needs at all. Reviewed every job's steps for a genuine main-only rationale before touching it -- found none; all 12 operate on PR-diff or full-repo state equally validly on dev as on main. No job is documented as intentionally dev-excluded. Fix: remove the base_ref clause from all 12, add the 3 missing jobs to ci-summary's needs, and add a strict success-only fail-closed block in ci-summary for all 12 (docs_only-gated, mirroring the OMN-15755/OMN-15731 pattern). New rendered-bash regression test (tests/unit/scripts/test_ci_skip_hole_12_jobs_omn15768.py) proves non-vacuity: 50/52 cases fail against the pre-fix file, 52/52 pass at head. Same ci.yml region OMN-15755 (#6221/#6226) and OMN-15731 (#6216) both touch -- rebase/merge onto dev after those land to avoid a repeat of the #6221/#6216 conflict resolved this session. Full local suite: 5977 passed / 47 skipped / 0 failed. mypy src/ tests/: clean, 398 files. Cites OMN-15768.
Problem
ci.yml'stestandtype-checkjobs carried:For a non-docs-only
pull_requestevent targetingdev(the everyday case — dev is this repo's default branch), that condition isfalse, so both jobs were unconditionally skipped on every ordinary dev-targeting PR. Thesmart-test-selectjob appears to have been meant to cover that path instead (itsif:is the exact complement:github.event_name == 'pull_request' && github.base_ref != 'dev' && ...), but nothing downstream ever consumed itsselected_paths/split_count/matrix/is_full_suiteoutputs — no job inci.ymlcarriedneeds: smart-test-select. It computed a selection and discarded it.ci-summary's generic rollup (contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')) does not catchskipped, so both jobs read as passing.Net effect: zero unit tests and zero type-checking ran on essentially all normal OCC dev traffic.
Provenance
Git-blamed and confirmed via
git show:base_ref != 'dev'carve-out ontest/type-check, and the matchingif:onsmart-test-select, were both introduced bye5048f853c3346389f264a3fa2953724ee90057e(OMN-14098 / PR ci(OMN-14098): OCC evidence-only fast-lane — docs_only-gated code analysis + CodeQL skip-with-success (canary: onex_change_control) #3665, 2026-07-07, "OCC evidence-only fast-lane"). Before that commit, both jobs ran unconditionally modulo the pre-existingdocs_onlyfast lane — that commit's own message even notes "(type-check + test were already gated on docs_only.)"; thebase_refclause was new there.smart-test-selectitself predates that commit (7166bbd22, OMN-10763, change-aware-selection port), but its output was never consumed bytesteven before OMN-14098 —testhas always run the unfiltereduv run pytest --cov=onex_change_control ...regardless of what the selector computed. So the underlying "selector output goes nowhere" gap existed since the selector's introduction; OMN-14098 additionally made the skip total by gatingtest/type-checkthemselves off on dev PRs.Fix
Simplest fail-closed shape, per CLAUDE.md rule 4's dev-boundary default (OCC's suite is small, so full-suite-on-dev matches the fail-closed default until a governed selector is deliberately wired — a separate follow-up, not a silent-skip default):
base_ref != 'dev'carve-out fromtest's andtype-check'sif:— both now run on every non-docs-only PR (restores pre-OMN-14098 shape).smart-test-selectjob rather than wire it. It was never consumed; keeping a decoy job that looks like it's doing something is worse than deleting it.test/type-checkinci-summary, mirroring the OMN-15484 (merge-hold-gate) pattern. This is not a bare "must be success" check —test/type-checkare legitimately skipped on docs-only PRs (the OMN-14098 evidence-only fast lane), so the new block distinguishes that skip from an illegitimate one vianeeds.zone-filter.outputs.docs_only.Dev-tip audit (per ticket instruction — check for latent breakage this hole allowed)
Full local run on
origin/dev@c9b86d152(before this fix), in worktreeomni_worktrees/OMN-15731-testfix/onex_change_control:uv run pytest tests/ -q→ 5915 passed, 47 skipped, 0 faileduv run mypy src/ tests/→ Success: no issues found in 396 source filesBoth clean. No dev-tip test repair is carried in this PR's payload — the CI hole did not (as of this audit) let a real regression slip through, but it was fully capable of doing so silently.
New regression test
tests/unit/scripts/test_test_typecheck_wiring_omn15731.pyrenders the realci-summarybash block out of the shippedci.yml(not a hand-built fixture) and executes it under bash:test/type-checksuccess→ passesskipped/failure/cancelledoutside the docs-only lane → fails closed, namesOMN-15731skippedwithdocs_only == 'true'→ still passes (the legitimate fast lane)needs:) restores the exact pre-fix bypass — the generic rollup alone reports "All required jobs passed" on a skippedtest/type-checkbase_refcarve-out and thesmart-test-selectjob are gone from the live file13/13 pass locally;
uv run mypy src/ tests/anduv run ruff check/formatclean on the new file.Tracking ticket
OMN-15755 — filed by the OMN-15731 label-gated-CI pilot lane after it independently found and explicitly scoped out this exact gap while building OCC#6216 (see OMN-15755's "Related" section for the disclosure chain). This PR is the direct code fix that ticket asks for.
Interaction with OCC#6216 (OMN-15731 pilot)
OCC#6216 has open
ci.ymlchanges in flight (label-gatespre-commitforci:ready, adds aPre-commitstrict block inci-summary) touching a different job in the same file. Based onorigin/dev; whichever PR lands second should rebase onto the other'sci-summaryshape.Evidence-Ticket: OMN-15755
Evidence-Source: OCC#6222