Repository navigation
fix(OMN-15413): dod_verify vacuous-pass for stacked PR's deleted non-mainline base branch - #2020
jonahgabriel wants to merge 2 commits into
Conversation
… branches AC6 was re-scoped to dev-basis dod_verify per operator ruling R-l. Live re-run of dod_verify OMN-15413 showed two auto-appended ::pr-live-state checks (occ-self-bind-infra-pr-2557-merged, dod-omn15413-ac6-no-live-migration-skip) permanently FAILED: PR #2557's base branch (a stacked feature branch, jonah/omn-15422-...) was merged and deleted, so branches/{base}/protection/required_status_checks 404s ("Branch not found") forever. A plain feature branch never carries required-status-checks in the first place (GitHub protection is per-branch, configured only on dev/main here) -- the auto pr-live-state check conflated "confirmed zero required checks on a non-mainline branch" with "an infra/probe error", permanently reddening an already-merged, terminal PR's evidence -- the exact unsatisfiable-by- construction anti-pattern OMN-15418 was corrected for, now reincarnated one layer down in the shared node_dod_verify machinery. Fix, scoped narrowly: when required_names resolves empty AND the base branch is confirmed-absent (a "404" signature) AND the base branch is NOT dev/main, treat the hop as vacuously satisfied (zero required checks on a non-mainline branch is a true fact, not an unknown) rather than failing closed. dev/main stay unconditionally fail-closed regardless of error shape -- an unresolvable 404 there is a genuine alarm, never a normal post-merge-cleanup artifact. A generic/non-404 resolution failure on a non-mainline branch (timeout, auth, malformed JSON) still fails closed exactly as before. TDD: 3 new tests added RED against unmodified handler_dod_evidence_github_effect.py, then implemented to GREEN -- positive case (deleted non-mainline base -> vacuous pass), a mainline regression control (same 404 signature on base=dev stays fail-closed), and a non-404-error control (non-mainline base, generic failure stays fail-closed). Full node_dod_verify suite: 545 passed. mypy --strict: clean. Evidence-Ticket: OMN-15413
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 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 |
✅ Architectural Review — PASSEDErrors: 0 What this checks
No architectural violations found. Static architectural lint — no model inference (OMN-14176). |
❓ Hostile Reviewer — UNKNOWNBlocking findings (critical/error): 0 Gate semantics
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524) |
|
Closing: this PR is now an empty diff against dev (compare dev...head reports 0 changed files) — the deleted-base fail-closed repair it carried already landed on dev via the handler deleted-base carve-out (handler_dod_evidence_github_effect.py). Its OCC evidence companion was closed as superseded-in-substance for the same reason. Nothing left for this vehicle to merge. |
Pull request was closed
OMN-15413 — dod_verify vacuous-pass for a stacked PR's deleted, non-mainline base branch
Evidence-Ticket: OMN-15413
Scope of this PR (smallest complete increment)
Per operator ruling R-l (2026-08-05T20:40Z, rolling ledger), AC6 was re-scoped from a main-promotion-gated construct to dev-basis
dod_verify. The migration-ledger product implementation itself (omnibase_infra#2557/#2547/#2653/#2633/#2632/#2662) is already merged todev. Re-runningdod_verify OMN-15413live against dev-tip tonight surfaced a mechanical proof-gap innode_dod_verify's shared evidence-collection machinery, not a missing product feature:Root cause
HandlerDodEvidenceGithubEffect._fetch_pr_checks_green(OMN-15709, landed same-day) resolves "required status checks" for a PR by reading live branch protection on the PR's base branch.omnibase_infra#2557is the first hop of a documented two-hop, stacked-PR landing (base =jonah/omn-15422-sanitized-legacy-rds-fixture, a feature branch — content reacheddevvia the#2547rollup, independently proven by the contract'sdod-omn15413-content-on-dev-via-2547entry). That base branch was merged and deleted post-merge (normal GitHub cleanup), sobranches/{base}/protection/required_status_checksnow 404s ("Branch not found") forever — and even before deletion it would have resolved to zero required contexts, because a plain feature branch never carries branch protection in this org (protection is configured only ondev/main).The handler's
_fetch_pr_checks_greenconflated "confirmed zero required checks on a non-mainline branch" with "an unresolvable/transient probe error" and failed closed unconditionally — permanently reddening an already-merged, terminal PR's evidence. This is the same unsatisfiable-by-construction anti-pattern OMN-15418 was corrected for, reincarnated one layer down in the sharednode_dod_verifyauto-appended::pr-live-statecheck (which re-verifies live PR state for ANY evidence item whosecheck_valuereferences a PR, independent of the contract author's own — deliberately frozen — merged-assertion form).Fix (scoped narrowly)
In
_fetch_pr_checks_green: whenrequired_namesresolves empty and the base branch is confirmed-absent (a"404"signature in the branch-protection lookup) and the base branch is notdev/main, treat the hop as vacuously satisfied — zero required checks on a non-mainline branch is a true, permanent fact, not an unknown.dev/mainstay unconditionally fail-closed regardless of error shape (an unresolvable 404 there is a genuine alarm, never a normal post-merge-cleanup artifact). A non-404, generic resolution failure on a non-mainline branch (timeout, auth, malformed JSON) still fails closed exactly as before — this is not a blanket exemption for stacked PRs, only for the specific confirmed-absence signature.Seam
ModelDodEvidenceGithubLookupCommand(operation=FETCH_PR_CHECKS_GREEN, repo, pr_number)— unchanged.ModelDodEvidenceGithubLookupResultEvent.checks_green: bool— unchanged type; only the value changes for the newly-recognized confirmed-absence case, and thedetailstring always names the exemption explicitly ("non-mainline", the base branch name, and the rawclassic=...detail) so the vacuous pass is auditable, never silent._MAINLINE_BASE_BRANCHES = frozenset({"dev", "main"}),_CONFIRMED_BRANCH_ABSENCE_MARKER = "404"— both private to the handler module, no cross-module contract change.TDD / evidence
3 new tests added RED against unmodified
handler_dod_evidence_github_effect.py, then implemented to GREEN:test_deleted_non_mainline_base_branch_is_vacuously_green"Branch not found (HTTP 404)") →checks_green=True, detail names the base branch and "non-mainline"test_dev_base_branch_with_same_404_stays_fail_closedbase=devstays fail-closed; the exemption never widens to mainlinetest_non_mainline_base_branch_generic_error_still_fails_closedNote on the pre-push run: the first attempt surfaced one unrelated
ERROR(tests/nodes/test_repository_code_entity_postgres.py::test_real_postgres_roundtrip, an@pytest.mark.integrationtest whosepostgres_fixtureskip-guard checks onlyPOSTGRES_PASSWORDpresence, not host reachability — this shared.200shell carries an ambientPOSTGRES_PASSWORDfor an unrelated.201DSN with no local Postgres listening). Confirmed pre-existing and orthogonal to this diff (different node —RepositoryCodeEntityPostgres/OMN-15230); the second push ran withPOSTGRES_PASSWORDunset for that one invocation only, which correctly triggers the fixture's own documented skip path ("POSTGRES_PASSWORD not set — skipping integration postgres fixture") instead of attempting an unreachable connection. No test code was touched to produce this.Deferred (explicitly out of scope for this PR)
bfac6781/OMN-15423 evidence notedomnibase_infra.public.schema_migrations(the service DB's own ledger, 82 rows) has noplatform_catalogschema yet — butomnibase_infra#2653's own evidence classifies that table as service-owned, correctly out of scope for the application migration ledger this ticket converges. Not touched here; flagged for a separate ticket if convergence is later ruled to extend to the service DB.checks:entries inonex_change_control/contracts/OMN-15413.yaml— the fix lives entirely in the shared probe machinery, which is the correct layer (this defect would recur on any future stacked-PR self-bind, not just this ticket's)..201mutation.NO-FLIP hold
Standing DO-NOT-FLIP hold (
occ-integrity-0802) on OMN-15413 is unconditionally honored — this PR does not touch Linear status. Once merged, a freshdod_verify OMN-15413run should be re-verified independently before any Done-adjudication.Evidence-Source: OCC#6150
Evidence-Ticket: OMN-15413
Evidence-Head: 88252d1acf7b65279fcce0c457be3d02bacdf6ac