Repository navigation
fix(OMN-15249): make the Integration Silent-Skip Guard die AT the Postgres image pull, not on a downstream exit 127 - #2501
Conversation
…tgres image pull, not on a downstream exit 127 The `integration-guard` job provisioned Postgres via a GitHub-managed `services:` block. On #2492 that image pull timed out against registry-1.docker.io inside GitHub's own "Initialize containers" step; every normal step was skipped, but the verdict step carried a bare `if: always()`, ran with no toolchain installed, and terminated the job on `uv: command not found` / exit 127 — several steps removed from the registry timeout that actually caused it. - Own the pull: explicit first step with bounded retry (3 attempts, 120s per-attempt `timeout`) that fails closed with an `::error::` naming the image, the registry, and the timeout. - Own the container: explicit `docker run` with the same health probe and an ephemeral published port, fail-closed on an unhealthy container, plus an unconditional `docker rm --force` teardown. - Gate the verdict on `steps.run_curated_proofs.conclusion != 'skipped'` instead of bare `always()`, so the guard cannot report on a run whose Postgres never materialized, while still firing on genuine test failures. Proof: tests/ci/test_integration_guard_pull_fatality.py — 8/8 RED at origin/dev, 8/8 GREEN here. Executes the workflow's real pull `run:` body as a bash subprocess against a stubbed docker, and replays the step graph through a GitHub-`if`-semantics simulator that fails closed on unmodelled conditions. Live end-to-end on real docker (.201, ephemeral standalone container, no lane touched): happy path pull/start/port/teardown green; blackholed registry exhausted 3 bounded attempts in 15s and exited 1 with the named annotation.
|
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 (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
#5149) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#2501 * evidence: OCC companion self-bind for #5149 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
|
| 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)
Evidence-Source: OCC#5149
Evidence-Ticket: OMN-15249
OMN-15249 — the Integration Silent-Skip Guard must die AT the Postgres image pull
Ticket: OMN-15249 (child of the OMN-14172 guard family). Source-only: workflow YAML, its config comment, and one new test. No lane touched, no runtime mutated.
The live defect (omnibase_infra#2492, job
89995662337)The
integration-guardjob provisioned Postgres through a GitHub-managedservices:block. That image pull happens inside GitHub's ownInitialize containersstep, which this repo cannot name, cannot bound, and — critically — which steps carryingif: always()still run past. Verbatim from the job log:Step conclusions for that job, read live from the API:
No files were found)Step 9 carried a bare
if: always(), so it fired past the failed container init with no toolchain installed:That
exit 127is the last error in the log and therefore what triage reads — a phantom missing-binary problem several steps removed from a registry timeout. It also invites exactly the "transient" mislabeling the operating rules reject.One correction to the ticket's framing, stated because it changes the fix: the pull failure was already fatal and already bounded — GitHub retries 3 times with backoff and the final attempt is
##[error], not a warning; the two warnings are per-attempt. The real defects are (1) the guard emitting a verdict for a run whose container never materialized, and (2) the misattributed terminal signal. Note that haduvsurvived,check_integration_skips.pywould have exited 2 on the missing JUnit report — still a verdict about silent skips from a run that never provisioned Postgres. The guard was committing its own false-green failure class against itself.Fix
Provisioning moves out of
services:and into steps this repo owns, which is what makes every DoD item mechanically enforceable rather than dependent on GitHub-internal behavior:pull_postgres(first step by design). Bounded retry —GUARD_PG_PULL_ATTEMPTS=3, each attempt wrapped intimeout ${GUARD_PG_PULL_TIMEOUT_SECONDS}(120s), linear backoff — then fails closed with an::error title=Postgres image pull failed (OMN-15249)::annotation naming the image, the registry, and the per-attempt timeout, and saying in words that this is a registry/network failure, not a missing binary and not a test failure. It runs before checkout and before toolchain setup: nothing after a failed pull can be trusted, so nothing after it is even attempted.start_postgres. Explicitdocker runwith the samepg_isreadyhealth probe and the same ephemeral published port theservices:block used, bounded health wait, fail-closed with a named annotation (plusdocker logs) if the container never reports healthy, anddocker port→$GITHUB_OUTPUTfor the downstream host/port resolution.enforce_no_silent_skipsandupload_guard_resultsmove from bareif: always()toalways() && steps.run_curated_proofs.conclusion != 'skipped'. The verdict fires when the proofs ran (success or failure) and never when they were not reached. This deliberately preserves the guard's whole purpose: on a genuine integration failure it still runs.stop_postgres. Unconditionaldocker rm --force ... || true, so a container started before a later step failed is never leaked onto a self-hosted runner and teardown never invents a second, misattributed failure.scripts/ci/integration_skip_guard.yamlprovisioned_by/ header comments updated to match (the file is the gate's SYNC anchor).Proof
tests/ci/test_integration_guard_pull_fatality.py— 8/8 RED atorigin/dev, 8/8 GREEN on this branch. Two of the three sections are behavioral, not source-text greps:test_simulated_registry_timeout_fails_the_pull_step_with_a_named_errorrun:body and executes it as abashsubprocess against a stubbeddockeron PATH that always fails. Asserts non-zero exit, exactlyGUARD_PG_PULL_ATTEMPTSinvocations (recorded by the stub, so retry is proven bounded and fail-closed on exhaustion), and that image + registry + timeout all appear in the::errorannotationtest_pull_step_succeeds_and_stops_retrying_when_the_registry_answerstest_no_toolchain_step_is_reachable_after_a_failed_pullif-semantics simulator with the pull marked failed: the verdict and upload steps resolve toskipped, cleanup still runs, and no executed step's shell body containsuv/pytest/python— i.e. the exit-127 path is unreachable, asserted over the graph rather than assumedtest_verdict_still_runs_when_the_curated_proofs_actually_failtest_verdict_runs_on_the_fully_healthy_pathtest_guard_does_not_delegate_the_pull_to_a_services_blockservices:key — the structural precondition without which none of the above is enforceabletest_pull_is_the_first_step_and_cannot_be_softenedcontinue-on-error(the defect class is literally "container failure downgraded")test_pull_retry_budget_is_bounded_and_declaredThe simulator fails closed: any
if:form it does not model raisespytest.failrather than being guessed at, so a future edit cannot silently re-open the warn-and-continue path.Live end-to-end on real Docker (Linux,
.201, ephemeral standalone containeromn15249-e2e-proof, no compose project and no lane touched, removed afterward — verified 0 containers remaining). Neither host available for gates has Docker, so this is where the real-daemon behavior was proven:pull_postgres→ attempt 1/3 success;start_postgres→ healthy, published port33844,port=33844written to$GITHUB_OUTPUT; TCP connect to127.0.0.1:33844OK andpsql -c 'select 1'→1(so--publish 5432and the existing host-resolution step still line up);stop_postgres→ container gone.::errorannotation naming image/registry/timeout.10.255.255.1:5000, the ticket's actual failure mode — a hang, not a DNS error): 3 attempts × 5stimeout= 15s wall clock, exit 1, named annotation. Thetimeoutwrapper is what bounds it.Gates (rule 11a — run on
.200,ssh stickybeatz-studio)Patch-transfer discipline: edited locally,
git diff --cached→scp→git apply --indexon.200, then per-filesha256verified identical on both hosts before any gate ran;ruff formatwas applied on.200and the formatted file copied back so the hosts stayed identical. Commit and push executed on.200.ruff check src/ tests/— All checks passedruff format --check— clean (one reformat applied and synced back)mypy tests/ci/test_integration_guard_pull_fatality.py— Success, no issuesscripts/ci/detect_test_paths.py --feature-flag on) →{"selected_paths":["tests/ci/"],"is_full_suite":false}→uv run pytest tests/ci/= 980 passed, 1 skipped (the skip is the pre-existingomninode_infrasibling-not-found guard, unrelated)pre-commit run --files <all 3 changed>— 45 passed, 0 failed, no file rewritten by a hook. IncludesIntegration silent-skip guard selftest (OMN-14172), which passes against the edited configDeviation to state: the
.201Docker end-to-end could not run on.200or this Mac — neither has a Docker daemon (docker: command not foundon both). It used a standalone container with a unique name, touched no compose project, mutated no lane, and was torn down and verified gone.Scope note
migration-integrationstill uses aservices:block. It is deliberately untouched: it has noif: always()step, so a pull failure there terminates cleanly atInitialize containerswith no misattributed downstream signal. Widening the change would add blast radius without closing a live defect.Safety
No merge performed by an agent. No lane mutation, no container recreate on any compose project, no prod surface touched.