fix(ci): retry transient base image pulls#7547
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
📝 WalkthroughWalkthroughThe resolver now securely captures and sanitizes Docker pull diagnostics, classifies failures as deterministic or transient, retries transient failures up to three times, and handles bounded or unclassifiable output without local fallback. Tests cover retries, redaction, truncation, status handling, and cleanup. ChangesDocker pull recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 4edb0c0 in the TypeScript / code-coverage/cliThe overall coverage in commit 4edb0c0 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. Since last review: 0 prior items resolved · 0 still apply · 0 new items found E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.github/actions/base-image-resolver.sh (2)
271-283: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCollector failure aborts the job even when
docker pullsucceeded.If the diagnostic collector fails (line 277) or the byte-count metadata is malformed (lines 279-280) after
status=0, the capture exits 74 andresolver_pullescalates toexit 75, killing the workflow despite a successful pull. Diagnostics only matter for the failure path; consider short-circuiting whenstatus == 0.♻️ Proposed short-circuit
collector_pid="" + if ((status == 0)); then + printf '%s\n%s\n' "$status" 0 + exit 0 + fi ((collector_status == 0)) || exit 74🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/base-image-resolver.sh around lines 271 - 283, Update the post-collection handling in resolver_pull so a successful pull status short-circuits before collector failure or byte-count validation can exit 74. Preserve the existing diagnostic collection and validation behavior for failed pulls, including truncation handling and resolver_pull’s failure escalation.
330-345: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd an explicit terminal
returnafter the retry loop.Every path inside the loop currently returns, exits, or sleeps, so line 345 is unreachable today. If the
attempt == 3guard is ever changed, the function falls out of the loop and returns 0, silently reporting a failed pull as success.🛡️ Defensive terminal return
done + + echo "::error::Base-image pull retry loop exited without a decision; refusing a local build fallback" >&2 + exit 75 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/base-image-resolver.sh around lines 330 - 345, In the base-image pull retry function, add an explicit nonzero terminal return immediately after the retry loop so any future path that exits the loop still reports pull failure rather than success. Preserve the existing exit 75 behavior and error handling for current retry and sleep failures.test/base-image-resolver-helper.test.ts (2)
624-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
observed-maxcan pass vacuously if no capture file is present.When the glob matches nothing (or only non-regular entries such as FIFOs),
max_sizestays0and line 650 passes without ever observing the on-disk bound. Asserting the sample was non-zero keeps this from silently degrading into a no-op if the capture layer changes shape.♻️ Make the observation mandatory
+ expect(Number(readFileSync(observedMax, "utf8"))).toBeGreaterThan(0); expect(Number(readFileSync(observedMax, "utf8"))).toBeLessThanOrEqual(65_536);Note the capture path may drain through FIFOs, so if
-flegitimately never matches, prefer asserting on the bounded regular file the collector writes instead.Also applies to: 650-650
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/base-image-resolver-helper.test.ts` around lines 624 - 631, Make the observed-max validation in the capture-size test mandatory rather than allowing a missing or empty capture set to leave max_size at zero. Update the loop and assertion around OBSERVED_MAX to verify that a bounded regular capture file was actually observed, while accounting for FIFO-based draining when selecting the file to validate.Source: Path instructions
384-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest title contradicts the behavior it asserts.
These cases are transient diagnostics that get retried three times and then exhaust to
EX_TEMPFAIL; calling them "terminal" reads as the deterministic path, which is exactly the distinction this suite exists to pin down.♻️ Suggested title
- ])("retries a terminal %s after more than 64 KiB of output", (_name, diagnostic) => { + ])("retries and exhausts a transient %s after more than 64 KiB of output", (_name, diagnostic) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/base-image-resolver-helper.test.ts` at line 384, Rename the parameterized test title in the retry diagnostic suite from “terminal” to “transient” so it accurately describes diagnostics retried three times before returning EX_TEMPFAIL; leave the test behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/actions/base-image-resolver.sh:
- Around line 271-283: Update the post-collection handling in resolver_pull so a
successful pull status short-circuits before collector failure or byte-count
validation can exit 74. Preserve the existing diagnostic collection and
validation behavior for failed pulls, including truncation handling and
resolver_pull’s failure escalation.
- Around line 330-345: In the base-image pull retry function, add an explicit
nonzero terminal return immediately after the retry loop so any future path that
exits the loop still reports pull failure rather than success. Preserve the
existing exit 75 behavior and error handling for current retry and sleep
failures.
In `@test/base-image-resolver-helper.test.ts`:
- Around line 624-631: Make the observed-max validation in the capture-size test
mandatory rather than allowing a missing or empty capture set to leave max_size
at zero. Update the loop and assertion around OBSERVED_MAX to verify that a
bounded regular capture file was actually observed, while accounting for
FIFO-based draining when selecting the file to validate.
- Line 384: Rename the parameterized test title in the retry diagnostic suite
from “terminal” to “transient” so it accurately describes diagnostics retried
three times before returning EX_TEMPFAIL; leave the test behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 469bcc3e-43c8-44d7-b78d-de62018fd490
📒 Files selected for processing (2)
.github/actions/base-image-resolver.shtest/base-image-resolver-helper.test.ts
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical `docs/changelog/2026-07-25.mdx` release entry with the exact `## v0.0.96` heading. The entry reconciles all 90 first-parent commits since v0.0.95 with all 92 merged PRs in the live `v0.0.96` label ledger and groups the user-visible changes by operator journey. ## Changes - Add the parser-safe dated MDX changelog entry for v0.0.96 with root-absolute links to the focused user guides. - Source summary: - [#7194](#7194) -> `docs/changelog/2026-07-25.mdx`: Document persistent baseline network policy exclusions and their inspection, rebuild, and snapshot behavior. - [#7188](#7188), [#7427](#7427), and [#7546](#7546) -> `docs/changelog/2026-07-25.mdx`: Document DNS-backed HTTPS inference routing, keyless loopback endpoints, and provider-marker isolation. - [#7238](#7238) -> `docs/changelog/2026-07-25.mdx`: Document blueprint sandbox and provider identifier validation before state writes or OpenShell calls, with bounded terminal-safe rejection previews. - [#7319](#7319), [#7274](#7274), [#7528](#7528), [#7353](#7353), and [#7560](#7560) -> `docs/changelog/2026-07-25.mdx`: Document the managed default gateway service, onboarding readiness, and container-runtime identity safeguards. - [#7349](#7349), [#7498](#7498), [#7406](#7406), [#7196](#7196), [#7559](#7559), [#7421](#7421), [#7510](#7510), [#7295](#7295), and [#7565](#7565) -> `docs/changelog/2026-07-25.mdx`: Document gateway-scoped status, lifecycle diagnostics, managed MCP recovery, delete-edge safeguards, and fail-closed CLI prompt and command output. - [#7591](#7591) -> `docs/changelog/2026-07-25.mdx`: Document opt-in authenticated MCP tool-name discovery, its bounded and names-only contract, probe interaction, and rebuild requirement. - [#7305](#7305), [#7480](#7480), [#7471](#7471), [#7365](#7365), and [#7541](#7541) -> `docs/changelog/2026-07-25.mdx`: Document installer version checks, version-tag reporting, license guidance, WSL Ollama selection, and DGX Station vLLM detection. - [#7482](#7482), [#7466](#7466), [#7208](#7208), [#7434](#7434), and [#7586](#7586) -> `docs/changelog/2026-07-25.mdx`: Document Ollama resource details, reasoning precedence, Hermes onboarding behavior, and preserved managed Hermes BuildKit failures. - [#6830](#6830), [#7492](#7492), [#7563](#7563), and [#7582](#7582) -> `docs/changelog/2026-07-25.mdx`: Document the authoritative OpenClaw production lock, fixed managed-image dependencies, immutable Hermes base adoption, and Hermes image-size reduction. - [#7505](#7505), [#7530](#7530), [#7547](#7547), [#7508](#7508), [#7548](#7548), [#7549](#7549), [#7537](#7537), [#7534](#7534), [#7515](#7515), [#7511](#7511), [#7551](#7551), [#7562](#7562), [#7575](#7575), [#7496](#7496), [#7594](#7594), [#7595](#7595), and [#7599](#7599) -> `docs/changelog/2026-07-25.mdx`: Summarize release validation, transient and bounded dispatch reconciliation, exact pre-tag qualification, identity revalidation, npm-audit retry, sharding, image reuse, timeout, telemetry, and workflow-hardening changes. - Reconciled without separate changelog prose: - [#7539](#7539), [#7526](#7526), [#7507](#7507), [#7506](#7506), [#7519](#7519), [#7516](#7516), [#7396](#7396), [#7254](#7254), [#7583](#7583), [#7596](#7596), and [#7598](#7598): Test-harness or fixture-only changes. - [#7403](#7403), [#7161](#7161), [#6877](#6877), [#7531](#7531), [#7525](#7525), [#7522](#7522), [#7536](#7536), [#7552](#7552), [#7566](#7566), [#7553](#7553), [#7561](#7561), [#7577](#7577), [#7569](#7569), [#7585](#7585), [#7584](#7584), [#7592](#7592), [#7580](#7580), [#7571](#7571), [#7517](#7517), [#7589](#7589), [#7402](#7402), [#7558](#7558), [#7544](#7544), and [#7601](#7601): Dependency, internal recovery, validation, contributor-workflow, E2E optimization, telemetry, or CI trust changes with no separate user-facing release claim. - [#7556](#7556), [#7573](#7573), [#7576](#7576), and [#7578](#7578): Experimental repository-maintainer conflict automation with no canonical user documentation surface. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates dated changelog structure, version headings, and published links. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Reviewed `docs/changelog/2026-07-25.mdx` at exact head `0f5dedb47` against 90 first-parent release commits and 92 merged PRs labeled `v0.0.96`. Verified parser-safe MDX SPDX, the exact version heading, literal CLI names, writing style, skip terms, all 20 root-absolute published links, and the accepted #7591 opt-in authenticated discovery bounds. #7544, #7599, and #7601 remain internal or CI-only release-ledger entries. Changelog tests passed 6/6, the docs build passed with 0 errors and two pre-existing Fern warnings, and `npm run check:diff` plus the final diff check passed. - Agent: Codex Desktop documentation-writer subagent <!-- docs-review-head-sha: 0f5dedb --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/changelog-docs.test.ts`: 6/6 passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to this prose-only changelog entry. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — the build passed with 0 errors and 2 existing Fern warnings; the published-route check passed. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — native changelog files use the required parser-safe MDX SPDX comment and no frontmatter. --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Persistent network policy exclusions with consistent restore/exclusion reporting across rebuilds/snapshots. * Opt-in MCP tool discovery via `mcp status --tools` with bounded, redacted authenticated traffic. * Improved HTTPS inference switching for custom endpoints and refreshed onboarding/model menu details. * Refined OpenShell gateway defaults for port `8080`, including more reliable readiness checks. * **Bug Fixes** * Prevent incorrect provider/model restoration after compatible-provider update failures. * Preserve managed MCP state after exec loss and tighten gateway/doctor status scoping. * **Tests** * Stronger, fail-closed release validation with hardened evidence/artifact handoff and bounded timeouts/retries. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Summary
Prevent a transient registry or transport failure while pulling a published base image from silently cascading into the memory- and disk-heavy local Hermes build. Positively classified transient pulls receive at most three attempts; exhaustion stops with a distinct temporary-failure status, while deterministic candidate absence and invalid-image failures retain their existing behavior.
Related Issue
Part of #7144 and parent epic #7140.
Changes
EX_TEMPFAIL75 after transient exhaustion so the current resolver callers cannot reinterpret the failure as a missing candidate and launch a local Hermes build.Type of Change
Quality Gates
4edb0c05dwith no findings. It covered classification precedence, status preservation, credential and log-command redaction, bounded memory/disk use, FIFO cleanup/cancellation, fail-closed truncation and collector failures, and the no-local-build exhaustion contract.Documentation Writer Review
no-docs-needed.github/actions/base-image-resolver.shandtest/base-image-resolver-helper.test.ts. It changes internal GitHub Actions failure handling and does not alter the CLI, configuration, API, sandbox runtime, or a user/operator workflow.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/base-image-resolver-helper.test.ts: 61/61 passed; shfmt, ShellCheck, Biome, Bash syntax, andgit diff --checkpassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; the change is isolated to one shared resolver helper and its caller-contract tests.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests