Repository navigation
fix(OMN-16317): treat docker-pull timeout as rejection in bare-sha256 negative test - #2819
Conversation
… negative test Deploy Agent Tests STRICT gate (deploy-agent-tests) blocks CI Summary and was reproducibly failing diff-unrelated PRs (2x on infra#2814): test_bare_sha256_is_an_invalid_pull_reference ran a real `docker pull sha256:000...000` with a hardcoded 15s subprocess timeout. Under registry/network latency the pull neither errored fast nor completed within 15s, so an uncaught TimeoutExpired ERRORed the test. The test's semantic claim is "a bare sha256 is an invalid pull reference" -- a timeout while resolving that same unpullable reference is a consistent variant of the identical negative outcome (the daemon never succeeds in treating it as a satisfiable pull), so _assert_bare_sha256_pull_rejected() now accepts TimeoutExpired as equivalent proof alongside a non-zero exit, with the reasoning documented inline. Not a bigger magic-number timeout -- that's the same brittleness, slower. Added mock-based coverage (TestBareSha256PullRejectionOutcomes) pinning the fast-error, timeout, and false-positive (success-return) paths of the shared assertion helper without depending on a live registry round trip for the pinning itself.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Limit details: You’ve used the included review currently available. Your 117 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. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
✅ Hostile Reviewer — PASSEDBlocking findings (critical): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524) |
#6796) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#2819 * evidence: OCC companion self-bind for #6796 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
…ry timeouts (#2826) Every docker call in the OMN-15181 real_container fixture already passed check=False, so a non-zero exit was the fixture's business to interpret. But subprocess.run raises TimeoutExpired regardless of check=, so the one failure mode the fixture could not absorb was the one that actually fires on the shared self-hosted fleet: docker daemon and registry contention. Route all four docker calls through a _docker() helper that collapses a timeout to None, putting it back under the fixture's control: - docker pull is a best-effort warm-up; a timeout no longer errors both tests (the image is usually already cached and docker create is the real gate). - docker create timing out, or failing because busybox never became available locally, skips -- that is an environment condition, not a statement about docker inspect semantics. Any OTHER non-zero exit still raises, so a genuine docker semantic is not swallowed. - docker rm -f timing out at teardown no longer errors a passing test; the container name is uuid-unique so a leaked container cannot collide. Create/remove budgets raised 30s -> 90s/60s per the ticket's option 1, same rationale as OMN-8535 and OMN-7871. Same reasoning as the merged OMN-16317 fix (#2819), which applied it to the sibling bare-sha256 negative test in this directory; this closes the remaining live-network docker calls in the module. Refs: OMN-15749
1 participant
Summary
omnibase_infra's STRICT-gate job Deploy Agent Tests (OMN-15378) / deploy-agent-tests (member of infra'sSTRICT_GATE_JOBS, blocksCI Summary) was reproducibly failing diff-unrelated PRs — reproduced 2x onomnibase_infra#2814.TestPullPinnedImageRealDockerSemantics::test_bare_sha256_is_an_invalid_pull_referenceruns a realdocker pull sha256:000...000against the live registry with a hardcoded 15s subprocess timeout. Under registry/network latency the pull neither errors fast nor completes within 15s, so an uncaughtsubprocess.TimeoutExpiredERRORed the test.Fix
Root-cause fix, not a bigger magic-number timeout (same brittleness, slower). The test's semantic claim is "a bare sha256 is an invalid pull reference" — a timeout while resolving that same unpullable reference is a consistent variant of the identical negative outcome (the daemon never succeeds in treating it as a satisfiable pull). Extracted the assertion into
_assert_bare_sha256_pull_rejected(), which now acceptsTimeoutExpiredas equivalent proof alongside a non-zero exit, with the reasoning documented inline.Added
TestBareSha256PullRejectionOutcomes(mock-based) pinning three paths of the shared helper without needing a live registry round trip for the pinning itself:Ticket
OMN-16317 — dod_evidence: this PR is the fix; CI green + merge is the evidence.
Unblocks
omnibase_infra#2814(OMN-16286 chain).Test plan
uv run --project scripts/deploy-agent --extra dev pytest scripts/deploy-agent/tests -v --tb=short→ 198 passed, 8 skipped (docker daemon unreachable in this sandbox, as expected)uv run ruff format/ruff checkcleanpre-commit run --all-files(via commit) — all hooks passedEvidence-Ticket: OMN-16317
Evidence-Source: OCC#6796