Repository navigation
fix(OMN-16508): resolve the governing PR instead of vacuously passing contract-compliance on empty PR_NUMBER - #2926
Conversation
… contract-compliance
`ci.yml`'s `contract-compliance` step met an empty `PR_NUMBER` with `exit 0`.
That was a vacuous pass on a required gate: "Contract Compliance Check" is a
GATE_JOBS entry in `scripts/ci/ci_summary_gate.py`, and CI Summary is this
repo's sole required branch-protection context, so the umbrella poller counted
a run that evaluated zero DoD check_values as a *provable* pass. Unlike a red
gate, a green one that checked nothing never prompts investigation.
Scope correction against the ticket body, which claimed "every dev push":
`on.push.branches` here is `[main]`, so this never fired on a dev commit. The
two live fail-open events are push-to-main (release-synced fast-forwards) and
merge_group -- the latter unexercised today (no registry repo has a queue on
dev as of 2026-08-24) but a latent bypass of the sole required context the
instant a queue returns, with zero further code change.
The fix is resolution, not narrowing. Excluding push/merge_group with a
job-level `if:` would publish a `skipped` check run, which branch protection
counts as passing -- the OMN-14863 skip-vector class. So the job keeps running
on all three events and `scripts/ci/resolve_contract_compliance_pr.py` answers
which PR's check_values apply: the event's own number on pull_request (no API
call, behaviourally identical to pre-fix), the `pr-<n>-` segment of the
gh-readonly-queue ref on merge_group (no API call), or the merged source PR of
the pushed commit via `gh api repos/{repo}/commits/{sha}/pulls`. Anything
unresolved -- including a transient gh failure -- exits 1.
Verified live against the real API before landing: dev 8ce11d5 -> #2923, and
the three most recent main commits -> #2837/#2823/#2835, so the push path now
evaluates a real scope rather than going spuriously red. An all-zero SHA exits
1 with no stdout.
Ships with the OMN-15547 incident replay its own default-deny rule demands:
two verbatim `gh api` captures under tests/fixtures/omn16508/ -- the empty
association list GitHub really returns for a commit no merged PR produced
(6d7090d, head of closed-unmerged #2890), which the guard must reject, and a
21KB merged-PR response (5b904d8 -> #2378) as the control that stops a
hard-wired `return None` from replaying the incident while failing every
legitimate push-to-main closed.
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 130 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
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 — multi-model adversarial review (OMN-8468/OMN-8524)
#7288) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#2926 * evidence: OCC companion self-bind for #7288 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
Closes OMN-16508.
Defect
ci.yml'scontract-compliancejob runsrun_contract_compliance_check.py --pr <n>, where<n>isgithub.event.pull_request.number. Onlypull_requestpopulates that — but the job'sif:also admitspushandmerge_group. The step met the empty case withexit 0:That is a vacuous pass on a required gate, not a cosmetic skip.
"Contract Compliance Check"is aGATE_JOBSentry inscripts/ci/ci_summary_gate.py:200, andCI Summaryis this repo's sole required branch-protection context — so the umbrella poller counted a run that evaluated zero DoDcheck_valuesas a provable pass. Unlike a red gate, a green one that checked nothing never prompts investigation.Scope correction vs. the ticket body
The ticket claimed the gate "reports green on every dev push" and framed this as "a false-signal risk across the whole omnibase_infra dev branch." That is not the live shape, and the PR body records it rather than quietly shipping against a wrong premise. This workflow's trigger is:
ci.ymlnever runs on adevcommit at all. The two real fail-open events are:pushtomain— the release-synced fast-forward commits.merge_group— unexercised today (verified 2026-08-24: zero registry repos are queue-controlled,mergeQueue(branch:"dev")isnullfleet-wide), but a latent bypass of the sole required context the instant a queue returns, with zero further code change. This is the higher-severity half.The ticket also cited "the OMN-16346 omnibase_core diff as the template implementation." That diff is not landed — OMN-16346 is still In Progress and its own body records the fix as "committed ... NOT YET PUSHED". What is live on core
devis the earlier #1556 change (plainexit 0→exit 1, no PR resolution). So this implements the pattern from the ticket's description rather than porting a diff. Full premise-correction comment is on the ticket.Fix: resolution, not narrowing
Excluding
push/merge_groupwith a job-levelif:would publish askippedcheck run, which branch protection counts as passing — the same skip-vector class thereject-required-check-skip-vectorhook (OMN-14863) exists to reject. So the job keeps running on all three events, and a new step resolves which PR'scheck_valuesapply viascripts/ci/resolve_contract_compliance_pr.py:pull_requestgithub.event.pull_request.number, verbatimmerge_grouppr-<n>-segment ofrefs/heads/gh-readonly-queue/<base>/pr-<n>-<sha>pushgh api repos/{repo}/commits/{sha}/pullsexit 1— fail closedA transient
ghfailure resolves to nothing and fails closed: a flake must never be indistinguishable from a green gate. The DoD step keeps its ownexit 1floor as defence in depth.Verification
Live end-to-end against the real GitHub API, before landing:
8ce11d518(push)2923maincommitse3cb9969/559ee461/166dc9602837/2823/2835--pr-number 2915(pull_request)2915refs/heads/gh-readonly-queue/dev/pr-2923-8ce11d52923Every recent
maincommit resolves, so the push path now evaluates a real scope rather than trading a false green for a spurious red.TDD, red before green. With
ci.ymlreverted to its pre-fix bytes, the three workflow-shape guards fail:Incident replay (OMN-15547). The resolver is newly wired enforcement, so the coverage guard's default-deny correctly refused the first commit (
[DEFAULT-DENY] scripts/ci/resolve_contract_compliance_pr.py ... carries no incident replay case). It ships with two verbatimgh apicaptures rather than a baseline exemption:commit-6d7090da-pulls.gh-api.json.captured, what the API really returns for a commit no merged PR produced (head of closed-unmerged chore(OMN-16513): canary PR for OMNI_SECURITY_SCAN_RUNS_ON_JSON seam #2890): an empty association list, exactly the state the old step turned green.commit-5b904d88-pulls.gh-api.json.captured(21KB, realmaincommit → merged PR build(deps): update opentelemetry-instrumentation-kafka-python requirement from <0.64,>=0.48b0 to >=0.48b0,<0.66 #2378 with matchingmerge_commit_sha). Without it a hard-wiredreturn Nonewould replay the incident perfectly and still be useless.54 passedacross the three related modules;pre-commit run --all-filesexits 0 (includingIncident-replay coverageandReject skip vectors on required-check workflows);mypy --strictclean on the new module; pre-push governed selector escalated to the full unit suite fail-closed and passed.Risk
The
pull_requestpath is unchanged in behaviour. Thepush/merge_grouppaths move from "always green" to "evaluates the originating PR'scheck_values, or fails closed" — which is the point. Fail-closed onmainpushes is the intended direction and is empirically not spurious: all recentmaincommits resolve.Evidence-Ticket: OMN-16508
Evidence-Source: OCC#7288