Repository navigation
ci(OMN-14172): fail-closed gate against integration silent-skip false-greens - #1652
Merged
Merged
Conversation
…-greens An @pytest.mark.integration real-service test that self-skips when its DB/service/env is absent produces a false-green: the required Tests job goes green while the real-DB assertion never ran. This recurred (OMN-10330 omninode_infra, OMN-14167 omnimarket) and each time hid a genuine latent defect (the OMN-14058 missing-tenant_id-column bug reached dev this way). Detection one-repo-at-a-time never held; make the class mechanically RED (Rule #5). New enforcement (omnimarket canary): - scripts/ci/check_integration_skips.py: parses the guard job's JUnit-XML and FAILS when (1) any integration test skipped citing a PROVISIONED service's absence, or (2) fewer than require_executed_min integration tests executed (zero-collection / marker-typo false-green, OMN-14082 family). Keyed to the services the job provisions, so legitimately-optional skips (Kafka broker not provisioned by design, live-LLM / live-e2e opt-in flags) stay allowed. - scripts/ci/integration_skip_guard.yaml: surfaces the OMN-13575 silent_skip_allowed:false concept as data; declares postgres required_services patterns + allowed_optional patterns. - ci.yml job integration-guard: provisions Postgres, runs -m "integration and not kafka", then runs the gate; wired into the ci-summary required rollup. - pre-commit hook integration-skip-guard: runs the gate --selftest locally. - tests/ci/test_check_integration_skips.py: encodes case (a) provisioned->PASS and case (b) reintroduced silent skip->RED so the gate cannot silently regress. dod_evidence: - Real omnimarket proofs (test_..._omn14058 / _omn13662) with no local Postgres self-skip "POSTGRES_PASSWORD not set" -> gate exits 1 (3 false-greens flagged). - Provisioned-state JUnit (integration tests ran) -> gate exits 0. - 11/11 unit tests pass through the real repo conftest; ruff + aislop clean. Closes OMN-14172
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 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 |
jonahgabriel
added a commit
that referenced
this pull request
Jul 9, 2026
… — verify pushed companion, not the call (#1653) * fix(OMN-14173): fail-closed prs_fixed accounting for OCC autobind arm — verify pushed companion, not the call merge_sweep --fix-only reported prs_fixed=2 on omnimarket #1651/#1652 while authoring ZERO OCC companions (a false-success of the exact class the tooling targets). Root cause = accounting, not routing. Trace: - Routing (STEP 2A): _block_reason_for_fix already routes occ-preflight/eligibility (with a ticket) to RECEIPT_EVIDENCE_SOURCE_AUTOBIND via the OCC-evidence subset match (handler_pr_lifecycle_orchestrator.py) — the OMN-13990 follow-up already landed this. The pre-supplied "{verify/verify}-only" diagnosis was stale vs HEAD. Re-asserted with a fresh witness test. - Accounting (STEP 2B, the real defect): HandlerPrLifecycleFix.handle set fix_applied=True whenever _route() returned without raising — including the no-op / idempotent / short-circuit paths that push NO companion. _fix_one then counted prs_dispatched=1 off fix_applied. Fix (fail-closed, verify the effect not the call): - New OccCompanionVerifier reads GitHub back after the autobind adapter runs and confirms all three: product PR body carries Evidence-Source: OCC#<n>, that OCC PR is OPEN, and the auto/* companion branch exists on the OCC remote. Any missing evidence / resolution error fails CLOSED (verified=False). - HandlerPrLifecycleFix gains occ_companion_verified on its result, set from the verifier for the autobind arm. Default verifier is fail-closed (proves nothing → counts nothing); the merge-sweep orchestrator injects the live verifier. - _fix_one gates prs_fixed for RECEIPT_EVIDENCE_SOURCE_AUTOBIND on occ_companion_verified, never on fix_applied. All other block reasons keep their existing dispatch semantics (no regression to merge arm, OccContractAdapter, CI/code/coderabbit arms). Tests (STEP 3, executed): new test_occ_companion_fail_closed_accounting.py proves (i) occ-preflight/eligibility routes to the autobind arm, (ii) an unverified / unpushed companion is NOT counted in prs_fixed (fail-closed), and the happy path counts exactly once. Live-adapter wiring test asserts the orchestrator injects the real OccCompanionVerifier. dod_evidence: - env -u PYTHONPATH uv run pytest tests/ -m "not kafka": 12271 passed, 60 skipped - new test_occ_companion_fail_closed_accounting.py: 7 passed - ruff format + ruff check --fix: clean - mypy --strict on both nodes: Success, no issues in 27 files - pre-commit (scoped to changed files, OMNIBASE_INFRA_PATH set): all hooks pass * test(OMN-14173): lock OccAutobindAdapter create-if-absent mint path (fresh-ticket companion) Residual follow-up to #1653. The residual hypothesis ("OccAutobindAdapter only SYNCS an existing OCC contract and no-ops when a fresh ticket has none — the contract-CREATION path may be required to mint for a first-time ticket") is REFUTED against live code: - The create-if-absent branch already exists and is wired live: adapter_occ_autobind.py:285-294 writes contracts/<ticket>.yaml via _CONTRACT_TEMPLATE when `not contract_path.is_file()`, and the live OccAutobindAdapter has been injected in _ensure_sub_handlers since facb4e8 (2026-06-24, #1392) — never a noop on the merge_sweep path. - run_git uses check=True and re-raises CalledProcessError (occ_git_transport.py), so a failed OCC push RAISES → fix_applied=False → not counted. A mint failure can never surface as "fixed but no companion". The only clean-return-no-mint path is the idempotency guard (already Evidence-Source: OCC#-bound), which #1653 now gates via the OccCompanionVerifier read-back. The genuine gap was COVERAGE: the sole end-to-end test (TestAutobindForcePush) forced Path.is_file=True (the SYNC path), so nothing locked the CREATE path and the residual could not be falsified empirically. This adds TestAutobindCreatesContractForFreshTicket (2 tests), verifying the EFFECT (not the call), mirroring the #1653 mutation-test style: - fresh ticket (is_file=False) → a net-new contracts/<ticket>.yaml is WRITTEN with the contract template, the auto/* companion branch is force-pushed, and Evidence-Source: OCC#<n> is patched back onto the product PR. - existing contract (is_file=True) → SYNC binds receipts + pushes + patches WITHOUT rewriting the contract (OMN-13888 whole-file-hash: never modify an existing contract). Mutation-verified: reverting the `if not contract_path.is_file()` create guard (→ `if False`) makes test_fresh_ticket_mints_net_new_contract_and_pushes fail (only receipts written, no contract) — the test genuinely guards the mint. dod_evidence: - env -u PYTHONPATH uv run pytest tests/unit/nodes/node_pr_lifecycle_fix_effect/test_adapter_occ_autobind.py: 26 passed - node_pr_lifecycle_fix_effect + node_pr_lifecycle_orchestrator test dirs: 274 passed - mutation (revert create guard): fresh-ticket test FAILS as designed, source restored clean - ruff format + ruff check: clean; mypy --strict fix_effect node: Success (19 files) - pre-commit (changed file, OMNIBASE_INFRA_PATH set): all hooks pass - Live fresh-mint end-to-end proof: NOT run — no safe target (only open omnimarket PRs are #1653 [self-authoring prohibited] and #1651 [already bound OCC#3740]); merge_sweep has no single-PR scope. proof_class=code-only for the live link; create path is code-verified + mutation-locked. OCC companion for OMN-14173 is owed (Codex authors it per feedback_no_self_authored_evidence).
jonahgabriel
added a commit
that referenced
this pull request
Jul 10, 2026
…t dispatch Piece 5/5 of the OMN-14180 canonical OCC stamp-model. Generalizes the OMN-14173 OccCompanionVerifier read-back to EVERY fix arm and closes OMN-14174's dispatch-vs-effect over-count. The merge-sweep orchestrator (node_pr_lifecycle_orchestrator) used to increment prs_fixed on the strength of the fix handler returning fix_applied=True — i.e. on DISPATCH. Only the OCC-autobind arm was hardened (OMN-14173); every other arm (CODE_FAILURE / CI_FAILURE / CODERABBIT / CONFLICT), which mixed-failure PRs fall through to, still counted on dispatch. That over-counted prs_fixed for PRs where nothing landed (OMN-14174: omnimarket #1651/#1652 reported prs_fixed=2 with zero companions). Change: after a fix arm runs, the orchestrator performs an INDEPENDENT read-back over live gh/remote state — never the handler's self-reported flag (CLAUDE.md Rule 3) — and counts the fix ONLY when the OCC stamp is confirmed landed: (a) the referenced OCC companion PR exists on the remote and is open, and (b) the product PR body carries the Evidence-Source: OCC#<n> + Evidence-Ticket stamp, parsed via the Piece-2 canonical parser (omnibase_compat.contracts.pr_occ_stamp — the SAME parser the gate uses, no second extraction path). Fail-closed: a noop / dispatched-but-not-landed / dry-run arm counts 0. Arms that legitimately land no OCC stamp under-count (the safe direction OMN-14174 requires; full per-arm effect read-back is the deferred generalization). - New: node_pr_lifecycle_orchestrator/handlers/occ_stamp_readback.py (ProtocolOccStampReadback + live OccStampReadback via gh pr view + fail-closed _UnverifiedOccStampReadback default). Wired live in _ensure_sub_handlers. - _dispatch_fix_parallel._fix_one gates counted_as_fixed on the read-back for every arm (replaces the autobind-only occ_companion_verified special case). - metadata.yaml: declare the new omnibase_compat schema dependency. - pyproject: bump omnibase-compat ==0.5.5 -> ==0.5.6 (dependencies + override-dependencies); omnibase-core stays >=0.46.7,<0.47.0. Tests (all green locally): - false-success regression: noop dispatch, nothing landed -> prs_fixed == 0 - true-success: real companion landed (body stamped + OCC PR open) -> counted - over-count regression: mixed-failure CODE_FAILURE dispatch, no stamp -> 0 - read-back unit coverage (real Piece-2 parser, in-memory gh fetcher) Gate evidence: ruff format+check clean; mypy clean on the orchestrator node + new module + touched tests; node-metadata dependency check PASS; runtime import-check clean; /onex:compliance_sweep parity (0 new violations); node orchestrator/fix + integration + golden-chain + merge_sweep/e2e suites green. OCC companion: pending independent verifier (implementer does not self-author evidence). The Piece-2 OCC-stamp schema was relocated from omnibase_core to omnibase_compat under OMN-14223 (lowest shared layer). Depends on omnibase-compat 0.5.6 (omnibase_compat#173) being published to PyPI; validated locally against the compat-dev build.
jonahgabriel
added a commit
that referenced
this pull request
Jul 10, 2026
…t dispatch Piece 5/5 of the OMN-14180 canonical OCC stamp-model. Generalizes the OMN-14173 OccCompanionVerifier read-back to EVERY fix arm and closes OMN-14174's dispatch-vs-effect over-count. The merge-sweep orchestrator (node_pr_lifecycle_orchestrator) used to increment prs_fixed on the strength of the fix handler returning fix_applied=True — i.e. on DISPATCH. Only the OCC-autobind arm was hardened (OMN-14173); every other arm (CODE_FAILURE / CI_FAILURE / CODERABBIT / CONFLICT), which mixed-failure PRs fall through to, still counted on dispatch. That over-counted prs_fixed for PRs where nothing landed (OMN-14174: omnimarket #1651/#1652 reported prs_fixed=2 with zero companions). Change: after a fix arm runs, the orchestrator performs an INDEPENDENT read-back over live gh/remote state — never the handler's self-reported flag (CLAUDE.md Rule 3) — and counts the fix ONLY when the OCC stamp is confirmed landed: (a) the referenced OCC companion PR exists on the remote and is open, and (b) the product PR body carries the Evidence-Source: OCC#<n> + Evidence-Ticket stamp, parsed via the Piece-2 canonical parser (omnibase_compat.contracts.pr_occ_stamp — the SAME parser the gate uses, no second extraction path). Fail-closed: a noop / dispatched-but-not-landed / dry-run arm counts 0. Arms that legitimately land no OCC stamp under-count (the safe direction OMN-14174 requires; full per-arm effect read-back is the deferred generalization). - New: node_pr_lifecycle_orchestrator/handlers/occ_stamp_readback.py (ProtocolOccStampReadback + live OccStampReadback via gh pr view + fail-closed _UnverifiedOccStampReadback default). Wired live in _ensure_sub_handlers. - _dispatch_fix_parallel._fix_one gates counted_as_fixed on the read-back for every arm (replaces the autobind-only occ_companion_verified special case). - metadata.yaml: declare the new omnibase_compat schema dependency. - pyproject: bump omnibase-compat ==0.5.5 -> ==0.5.6 (dependencies + override-dependencies); omnibase-core stays >=0.46.7,<0.47.0. Tests (all green locally): - false-success regression: noop dispatch, nothing landed -> prs_fixed == 0 - true-success: real companion landed (body stamped + OCC PR open) -> counted - over-count regression: mixed-failure CODE_FAILURE dispatch, no stamp -> 0 - read-back unit coverage (real Piece-2 parser, in-memory gh fetcher) Gate evidence: ruff format+check clean; mypy clean on the orchestrator node + new module + touched tests; node-metadata dependency check PASS; runtime import-check clean; /onex:compliance_sweep parity (0 new violations); node orchestrator/fix + integration + golden-chain + merge_sweep/e2e suites green. OCC companion: pending independent verifier (implementer does not self-author evidence). The Piece-2 OCC-stamp schema was relocated from omnibase_core to omnibase_compat under OMN-14223 (lowest shared layer). Depends on omnibase-compat 0.5.6 (omnibase_compat#173) being published to PyPI; validated locally against the compat-dev build.
jonahgabriel
added a commit
that referenced
this pull request
Jul 10, 2026
…t dispatch (#1664) * fix(OMN-14191): merge-sweep counts prs_fixed only after read-back, not dispatch Piece 5/5 of the OMN-14180 canonical OCC stamp-model. Generalizes the OMN-14173 OccCompanionVerifier read-back to EVERY fix arm and closes OMN-14174's dispatch-vs-effect over-count. The merge-sweep orchestrator (node_pr_lifecycle_orchestrator) used to increment prs_fixed on the strength of the fix handler returning fix_applied=True — i.e. on DISPATCH. Only the OCC-autobind arm was hardened (OMN-14173); every other arm (CODE_FAILURE / CI_FAILURE / CODERABBIT / CONFLICT), which mixed-failure PRs fall through to, still counted on dispatch. That over-counted prs_fixed for PRs where nothing landed (OMN-14174: omnimarket #1651/#1652 reported prs_fixed=2 with zero companions). Change: after a fix arm runs, the orchestrator performs an INDEPENDENT read-back over live gh/remote state — never the handler's self-reported flag (CLAUDE.md Rule 3) — and counts the fix ONLY when the OCC stamp is confirmed landed: (a) the referenced OCC companion PR exists on the remote and is open, and (b) the product PR body carries the Evidence-Source: OCC#<n> + Evidence-Ticket stamp, parsed via the Piece-2 canonical parser (omnibase_compat.contracts.pr_occ_stamp — the SAME parser the gate uses, no second extraction path). Fail-closed: a noop / dispatched-but-not-landed / dry-run arm counts 0. Arms that legitimately land no OCC stamp under-count (the safe direction OMN-14174 requires; full per-arm effect read-back is the deferred generalization). - New: node_pr_lifecycle_orchestrator/handlers/occ_stamp_readback.py (ProtocolOccStampReadback + live OccStampReadback via gh pr view + fail-closed _UnverifiedOccStampReadback default). Wired live in _ensure_sub_handlers. - _dispatch_fix_parallel._fix_one gates counted_as_fixed on the read-back for every arm (replaces the autobind-only occ_companion_verified special case). - metadata.yaml: declare the new omnibase_compat schema dependency. - pyproject: bump omnibase-compat ==0.5.5 -> ==0.5.6 (dependencies + override-dependencies); omnibase-core stays >=0.46.7,<0.47.0. Tests (all green locally): - false-success regression: noop dispatch, nothing landed -> prs_fixed == 0 - true-success: real companion landed (body stamped + OCC PR open) -> counted - over-count regression: mixed-failure CODE_FAILURE dispatch, no stamp -> 0 - read-back unit coverage (real Piece-2 parser, in-memory gh fetcher) Gate evidence: ruff format+check clean; mypy clean on the orchestrator node + new module + touched tests; node-metadata dependency check PASS; runtime import-check clean; /onex:compliance_sweep parity (0 new violations); node orchestrator/fix + integration + golden-chain + merge_sweep/e2e suites green. OCC companion: pending independent verifier (implementer does not self-author evidence). The Piece-2 OCC-stamp schema was relocated from omnibase_core to omnibase_compat under OMN-14223 (lowest shared layer). Depends on omnibase-compat 0.5.6 (omnibase_compat#173) being published to PyPI; validated locally against the compat-dev build. * chore(OMN-14191): regen uv.lock for omnibase-compat 0.5.6 pyproject.toml already pinned omnibase-compat==0.5.6; uv.lock still resolved 0.5.5 (published before the pin landed). compat 0.5.6 is now live on PyPI — regenerate the lock so frozen sync resolves cleanly.
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.
OMN-14172 — silent-skip false-green prevention gate (omnimarket canary)
An
@pytest.mark.integrationreal-service test that self-skips when its DB/service/env is absent produces a false-green: the required Tests job reports success while the real-DB assertion never ran. This recurred across repos — OMN-10330 (omninode_infra), OMN-14167 (omnimarket) — and each time it hid a genuine latent defect (the OMN-14058 missing-tenant_id-column bug reacheddevexactly this way). Fixing it one repo at a time left no gate preventing the next regression. Per Rule #5 (enforcement, not detection) this makes the class mechanically RED.What this adds
scripts/ci/check_integration_skips.py— parses the guard job's JUnit-XML and FAILS when:require_executed_minintegration tests actually executed (zero-collection / marker-typo false-green — the OMN-14082 family).Keyed to the services the merge-gating job provisions, so legitimately-optional skips stay allowed: Kafka broker (not provisioned by design,
-m "not kafka"), live-LLM / live-e2e opt-in flags (OMN_ALLOW_LIVE_*). Fail-closed: a missing/unparsable report is a failure. Optional--strictalso flags unclassified skip reasons.scripts/ci/integration_skip_guard.yaml— surfaces the OMN-13575silent_skip_allowed: falseconcept as data; declares thepostgresrequired_servicesabsence patterns + theallowed_optional_skip_patterns.ci.ymljobintegration-guard— provisions Postgres (mirrors the OMN-14167 Tests service block), runs-m "integration and not kafka", then runs the gate; wired into the requiredci-summaryrollup (bothneedsand the check loop).integration-skip-guard— runs the gate--selftestlocally (stdlib+yaml, no DB): case (a) provisioned → PASS, case (b) reintroduced silent skip → RED, and validates the shipped config parses.tests/ci/test_check_integration_skips.py— 11 unit tests encoding case (a) and case (b) so the gate cannot silently regress.Approach / why low-false-positive
The gate is keyed to what the job actually provisions. Postgres IS provisioned → a
POSTGRES_PASSWORD not set/no reachable Postgresskip is a real false-green → FAIL. Kafka broker and live-* resources are deliberately NOT provisioned → their skips are expected and allowed. Marker-scoped (-m "integration and not kafka") + reason-classified + positive collection assertion.dod_evidence (verified locally)
test_delegation_savings_tenant_id_column_omn14058.py+test_projection_delegation_tier_distribution_omn13662.pywith no local Postgres → all three real-DB proofs self-skipPOSTGRES_PASSWORD not set→ gate exits 1, flagging 3 postgres false-greens (+ zero-collection). This is the exact false-green class that let OMN-14058 reach dev.POSTGRES_PASSWORDset: 2 passed / 1 optional-skip → gate PASS.--selftestPASSES; 11/11 unit tests pass through the real repo conftest (uv run pytest tests/ci/test_check_integration_skips.py).check_ai_slop.py --strictexit 0; pre-commit hooks pass on changed files (topic-naming-lint's only failure was environmental — missing sibling clone in the worktree parent — and passes withOMNIBASE_INFRA_PATHset; unrelated to this diff, no topics added).Rollout
omnimarket is the canary. Follow-up: port the gate to
omnibase_infraand the other queue repos (reconcile under the OMN-8470 vacuous-green design as a new layer).Note: omnimarket is a heavy (OCC) repo —
occ-preflightis RED until a net-new OCC companion lands; Tests/gate jobs showskippinguntil it unblocks. OCC companion owed (not self-authored per policy).Closes OMN-14172
Evidence-Source: OCC#3741
Evidence-Ticket: OMN-14172