fix(rebuild): support OpenClaw without Buildx - #7253
Conversation
Detect the canonical missing-Buildx failure and verify it with an independent probe. Retry generated managed OpenClaw contexts once with the legacy Docker builder. Seal the builder and Docker endpoint through rebuild and fail closed on drift. Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRebuild stages and validates replacement images before sandbox deletion. Eligible generated OpenClaw contexts can retry once with Docker’s legacy builder when Buildx is unavailable. Verified builder metadata and Docker environment are propagated through sandbox recreation with strict context validation. ChangesLegacy builder rebuild flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RebuildTargetRuntime
participant Preflight
participant Docker
participant SandboxCreation
RebuildTargetRuntime->>Preflight: stage and validate replacement image
Preflight->>Docker: build generated OpenClaw context
Docker-->>Preflight: Buildx availability result
Preflight->>Docker: retry once with DOCKER_BUILDKIT=0
Preflight-->>SandboxCreation: prepared context and verified Docker environment
SandboxCreation->>Docker: reuse verified legacy-builder settings
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-7253.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
5 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
Blockers
|
Move prepared rebuild proof binding into the sandbox launch coordinator. Keep the top-level onboard entrypoint net-neutral and avoid documenting an unsupported rebuild flag. Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Narrow the failed preflight result with an explicit type assertion. Keep the regression test free of conditional control flow. Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
wscurran
added
area: packaging
dfernandez365-rgb
left a comment
dfernandez365-rgb
left a comment
There was a problem hiding this comment.
Security review — changes required before merge.
The preflight does not bind the destructive rebuild to one Docker daemon and one approved artifact:
- prebuildDockerEnv reaches build/inspect, but cleanup and recreation fall back to the generic subprocess environment. That drops DOCKER_CONTEXT, DOCKER_CONFIG, TLS/certificate, and API-version selection. A stable non-default context can therefore build on daemon A while dockerRmi or sandbox creation resolves daemon B.
- Freezing the environment strings does not freeze what a named Docker context, config, socket, certificate, SSH target, or DNS name resolves to. Repointing a named context after verification passes the current identity check.
- The preflight image is removed. After the original sandbox is deleted, the context is rebuilt from mutable paths and native create consumes a mutable tag, so the artifact ultimately launched need not be the artifact preflight approved.
Please carry one immutable Docker-selection/engine identity through build, inspect, cleanup, and create; retain and create from the preflight-proven immutable image ID (or an equivalently attested byte snapshot); and add non-default-context, context-drift, post-verification mutation, and retag tests. The current mock/static tests do not exercise these destructive-boundary failures.
|
Current |
jyaunches
left a comment
jyaunches
left a comment
There was a problem hiding this comment.
Changes requested: a maintainer product decision is required before this implementation continues.
This PR makes a host with working Docker but missing or broken Buildx a supported rebuild environment. Current main intentionally uses BuildKit-only RUN --mount instructions for OpenClaw payload materialization in #7486. A mechanical rebase cannot preserve both designs.
Record one decision before updating this PR:
- If Buildx is required, remove the legacy-builder fallback. Fail before sandbox deletion with an actionable Buildx repair requirement.
- If missing Buildx is supported, define the supported host and Docker versions, current consumer, lifecycle owner, and legacy-compatible image path. That path must preserve ownership, modes, cache boundaries, layer limits, and security scans. Add exact-head tests and affected-host evidence.
If the fallback remains, also address the existing security review. Build, inspect, cleanup, and create must use the same verified Docker daemon and immutable image artifact through the delete-and-recreate boundary. Add the requested context-drift, post-verification mutation, and retag tests.
Do not merge until the product decision, security contract, implementation, and validation evidence agree.
<!-- markdownlint-disable MD041 --> ## Summary Conflict publication now constructs validated GitHub trees from the recorded `main` tree instead of the stale PR head. Production requests for #7253 and #6054 shrink from 1,180 and 3,754 entries to 13 and 6 entries. ## Related Issue Follow-up to #7542. ## Changes - Build each GitHub tree from the recorded base SHA and the final tree delta. - Read deletion entries from the base tree. - Keep parent blob reuse, final-tree equality, verified commits, and atomic ref updates unchanged. - Model 100 `main`-only files and a PR-side deletion in the publisher regression. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes internal GitHub publication mechanics and no user-facing contract. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Maintainer approved this repair after [production run 30193861556](https://github.com/NVIDIA/NemoClaw/actions/runs/30193861556) demonstrated the failure. Trust boundaries do not change. - [ ] 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: `no-docs-needed` - Evidence: The change repairs internal GitHub tree publication. It does not change commands, configuration, output, or supported behavior. - Agent: Codex Desktop <!-- docs-review-head-sha: 424209c --> <!-- 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 — command/result or justification: `npx vitest run --project integration test/pr-merge-conflict-fixer.test.ts` passed 12/12 after formatting. - [ ] 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 focused publisher fix. - [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) - [ ] 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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved pull request conflict resolution publishing to correctly reflect files added, modified, or deleted on each branch. * Ensured resolved updates are built from the correct base revision, preventing unrelated or outdated files from being included. * Improved publication accuracy when the target branch has advanced with additional changes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
Maintainer update: we should pause this PR until the Podman integration is available. This Buildx path and the Podman path should share a container-runtime abstraction for handling this scenario, rather than landing a Docker-specific design that Podman would later need to duplicate or unwind. Unblock criteria:
If the team decides this PR should establish the abstraction first and Podman should adopt it afterward, we can revisit that direction explicitly. Until then, #7253 is blocked on the Podman integration. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed current head 30938a5. I found no demonstrated correctness, security, compatibility, or regression blocker after the latest binding and export fixes. This focused code review does not resolve or override the existing product hold concerning the Podman and shared-runtime abstraction decision; that owner decision should remain in place.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking finding (Request changes outcome; GitHub does not permit the PR author to submit a Request changes review): I independently confirmed the existing PRA-1 recovery defect still reproduces at current head 825984c. In src/lib/actions/sandbox/rebuild-destroy-phase.ts, when observeSourceForDelete reports missing, execution skips validateDeleteEdge. That callback in rebuild-pipeline.ts is the only path that calls retainPreparedImageForRecreate. The lease therefore remains prepared, while openclaw-legacy-image.ts requires retained state for verifyForCreate. A resumed rebuild whose original sandbox is already absent proceeds into recreation and then rejects the retained image, leaving the supported recovery workflow unable to recreate the sandbox after the source is gone. Run the same prepared-image verification and retain transition before both the source-present and source-missing recreate paths (or stop before recreation), and add a regression with sourcePresence missing that proves the lease is retained and creation can verify it. This is distinct from, and does not override, the existing product-direction hold.
8 participants
Summary
Support rebuilding a NemoClaw-generated OpenClaw sandbox on a local Docker-driver gateway when Docker reports that Buildx is missing or broken.
NemoClaw independently confirms the failure, retries the exact staged context once with Docker's legacy builder, and binds the retained immutable image and verified Docker engine through deletion and recreation.
Unsupported rebuild paths still stop before deletion with Buildx repair guidance, and Hermes continues to require Buildx.
Related Issue
Related to #7111.
Reported by @JoyceChenNV.
The rebuild preflight builds on the original BuildKit prebuild implementation by @TLA27.
Changes
docker buildx versionprobe before oneDOCKER_BUILDKIT=0retry.Type of Change
Quality Gates
0c559f783found no blocking findings after verifying fallback eligibility, immutable-image ownership, Docker selector/engine and canonical-working-directory binding, deletion gating, cleanup, GPU fallback, rollback, stable-engine fixtures, and the final sandbox-preserving delete-failure fixture.Documentation Writer Review
docs-updatednpm run docscompleted with zero errors and two pre-existing Fern warnings before the latest line-budget-only follow-up.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 unavailablenpm run typecheck:cli,npm run checks:repository, Biome format/lint for touched files,git diff --check, and focused Vitest forsandbox-gpu-create-flow,rebuild-destroy-phase, and retained-runtime coverage (82/82) passed. On the preceding local head,npm run test:changedpassed 1102/1102 andnpm run docscompleted with zero errors and two pre-existing Fern warnings.npm run docsbuilds without warnings (doc changes only)npm run typecheck:cli, repository checks, changed-file Biome format/lint, andgit diff --checkpassed.npm run docscompleted with zero errors and two pre-existing warnings.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Signed-off-by: Julie Yaunches jyaunches@nvidia.com