Skip to content

fix(e2e): default protected images to sandbox user - #11613

Open
jyaunches wants to merge 1 commit into
mainfrom
codex/fix-protected-startup-runtime-user
Open

fix(e2e): default protected images to sandbox user#11613
jyaunches wants to merge 1 commit into
mainfrom
codex/fix-protected-startup-runtime-user

Conversation

@jyaunches

@jyaunches jyaunches commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Outcome

Protected managed-image builds select the non-root sandbox user when the caller omits --runtime-user. Both startup architectures can now reach Hermes qualification without violating its Dockerfile's non-root requirement.

Reason

The shared builder still defaulted to root after the managed runtime contract moved to sandbox. The multiarch startup workflow omitted the option, while GPU qualification already selected sandbox explicitly.

The same Hermes build failure occurred on both architectures in PR #11487 E2E and main E2E at bb6800b8. This dependency unblocks #11487. Missing contracts.json was a consequence of the failed build, not a separate cause.

Changes

  • Change the shared builder's default to sandbox; preserve explicit selection and final image-user validation.
  • Exercise the default for all three agents on AMD64 and ARM64. Reject an image reporting root under the default contract.
  • Preserve cache, retry, digest, cohort, credential, and cleanup controls. GPU qualification already passes the same user explicitly. No new mechanism or supported runtime is introduced.

Verification

  • npx --no-install vitest run --project integration test/platform/images/protected-managed-image-build-script.test.ts — 37 tests passed.
  • npm run build:cli and npm --prefix nemoclaw run build — passed.
  • Normal pre-commit and commit-msg hooks — passed, including ShellCheck and secret scanning.
  • Normal pre-push publication validation and CLI, plugin, and JavaScript-config TypeScript checks — passed with the canonical base bb6800b8fbb15b962822a1ef92a460dd1eab0229.
  • git diff --check — passed. The diff contains no secrets, API keys, or credentials.
  • The deterministic risk plan selects managed-image-multiarch-startup; live candidate evidence is pending.

Review notes

NVIDIA/NemoClaw commit 14391108763ea81e30c6e8da094e67a0161a95cd changes the sensitive path scripts/checks/build-protected-managed-images.sh. Full-diff self-review found no actionable correctness or security findings. The change preserves Hermes' non-root check and rejects a mismatched final image user. This is not independent review or maintainer approval.

Local Advisor could not start its first specialist: OpenShell reported sandbox is not ready during creation of pr-adv-5f1e35416ea5. Cleanup deleted that sandbox. No specialist review completed. The authorized alternative review path is in use; independent automated review, CI, and selected E2E remain pending. The contributor doctor also hit the default Node heap limit; publication validation uses its recommended 8 GiB heap. Both missing build outputs were subsequently built. Global CLI exposure was not changed.


Signed-off-by: Julie Yaunches jyaunches@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Protected managed-image builds now use sandbox as the default runtime user.
    • Explicit runtime-user selections, including root, remain supported.
    • Image user validation and architecture-specific runtime checks remain enforced.

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 12, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d9d52f41-e052-4de2-857a-edc5cbc130f9

📥 Commits

Reviewing files that changed from the base of the PR and between bb6800b and 1439110.

📒 Files selected for processing (2)
  • scripts/checks/build-protected-managed-images.sh
  • test/platform/images/protected-managed-image-build-script.test.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The protected managed-image build now defaults to the sandbox runtime user. Tests cover sandbox defaults on both architectures, explicit root overrides, mismatch validation, and architecture-specific build arguments.

Changes

Protected managed-image runtime user

Layer / File(s) Summary
Runtime user default and coverage
scripts/checks/build-protected-managed-images.sh, test/platform/images/protected-managed-image-build-script.test.ts
The build script defaults to sandbox. Tests verify sandbox defaults, explicit root selection, mismatch validation, and sandbox arguments for amd64 and arm64 builds.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: cv, prekshivyas

Merge Risk: ⚪ Minimal · up to 14391

Protected images now default to the intended sandbox user, with coverage for both architectures and explicit overrides. No merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: protected managed-image builds now default to the sandbox user.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-protected-startup-runtime-user

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 1439110 in the codex/fix-protected-... branch remains at 96%, unchanged from commit bb6800b in the main branch.

TypeScript / code-coverage/cli

The overall line coverage in commit 1439110 in the codex/fix-protected-... branch remains at 83%, unchanged from commit bb6800b in the main branch.

Show a line coverage summary of the most impacted files.
File main bb6800b codex/fix-protected-... 1439110 +/-
src/lib/sandbox...rce-identity.ts 82% 82% 0%
src/lib/onboard...uild-context.ts 74% 75% +1%

Updated September 12, 2026 04:24 UTC

@github-actions

Copy link
Copy Markdown
Contributor

PR Review Advisor finished for commit 1439110. Include the Advisor findings in the complete PR feedback collection. Verify and group valid findings before repair.

All previous runs

@jyaunches
jyaunches marked this pull request as ready for review September 12, 2026 05:19
@jyaunches

Copy link
Copy Markdown
Contributor Author

Ready for human review and merge at commit 1439110; label v0.0.124.

Local Advisor could not create its sandbox. Alternative review completed through full-diff self-review, protecting tests, hosted Advisor, CI, and selected E2E. CodeRabbit skipped the draft; this is not a CodeRabbit clearance claim.

This repairs the main regression blocking #11487. After a maintainer merges this PR, scheduled follow-up will integrate the repair into #11487 and resume its protected-runtime E2E. No merge performed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.124 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant