Walkthrough

This PR modifies the embedded Docker wrapper in test/e2e/test-openshell-gateway-upgrade.sh to properly detect when callers provide BASE_IMAGE build-args in either the --build-arg KEY=VALUE or --build-arg=KEY=VALUE form, setting a flag to prevent duplicate default injection.

Changes

Docker wrapper BASE_IMAGE detection

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• NVIDIA/NemoClaw#4041: Both PRs modify create_old_docker_wrapper() logic to control how --build-arg BASE_IMAGE=... is rewritten/injected for the old gateway-upgrade install path.
• NVIDIA/NemoClaw#4047: Both PRs modify create_old_docker_wrapper() to change how BASE_IMAGE build-args are detected/handled—one prevents duplicate injection when BASE_IMAGE is already provided, while the other injects a pinned fallback when it isn't.

Suggested labels

fix

Suggested reviewers

• cv

Poem

🐰 A wrapper once double-wrapped its build-arg base,
Now detection ensures no duplicate space!
Two forms of the call, rewrote_base tracks—
No more base injection that's falling through cracks. ✨

❌ Failed checks (1 warning)

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

• None.

Optional E2E

• openshell-gateway-upgrade-e2e (high): Useful to validate the edited E2E harness itself, especially the old Docker wrapper BASE_IMAGE build-arg handling used by this upgrade scenario. Not merge-blocking because only test code changed.

New E2E recommendations

• None.

Dispatch hint

• Workflow: nightly-e2e.yaml
• jobs input: openshell-gateway-upgrade-e2e

PR Review Advisor

Base: origin/main
Head: HEAD
Analyzed SHA: 0a1ee17c6d6d5e60de19fa1dde23f8ab044f3727
Recommendation: blocked
Confidence: high

The code change is small and security-clean, but the PR is not currently mergeable because GitHub reports mergeStateStatus=BLOCKED/review required.

Gate status

• CI: pass — 5 required status context(s) completed with no failures. Non-required contexts still pending: 0; failed: 0.
• Mergeability: fail — mergeStateStatus=BLOCKED
• Review threads: unknown — No review thread state was available.
• Risky code tested: pass — No risky code areas detected by path heuristics.

🔴 Blockers

• PR is currently blocked by mergeability/review gate: GitHub reports mergeStateStatus=BLOCKED and reviewDecision=REVIEW_REQUIRED for head SHA 0a1ee17. Required CI contexts are passing, but the mergeability gate is still failed.
  • Recommendation: Resolve the branch protection/review requirement and confirm the merge state is no longer BLOCKED before merge consideration.
  • Evidence: Trusted context: gateStatus.mergeability.status=fail with evidence mergeStateStatus=BLOCKED; GraphQL reviewDecision=REVIEW_REQUIRED.

🟡 Warnings

• Active same-file PR overlap may create drift (test/e2e/test-openshell-gateway-upgrade.sh): The changed test file still exists and this PR follows recent active work in the same area, but there are open overlapping PRs that also touch test/e2e/test-openshell-gateway-upgrade.sh. That raises rebase/conflict and behavior-drift risk for the OpenShell gateway upgrade harness.
  • Recommendation: Coordinate with PR chore: bump OpenShell pin to 0.0.44 #3830 and PR chore: upgrade agent runtime dependencies #3925 or re-check/rebase before merge so the BASE_IMAGE wrapper handling is not overwritten or contradicted by concurrent OpenShell/runtime dependency changes.
  • Evidence: Trusted openPrOverlaps lists chore: bump OpenShell pin to 0.0.44 #3830 'chore: bump OpenShell pin to 0.0.44' and chore: upgrade agent runtime dependencies #3925 'chore: upgrade agent runtime dependencies' with sameFiles including test/e2e/test-openshell-gateway-upgrade.sh.

🔵 Suggestions

• None.

Acceptance coverage

• met — address the unresolved CodeRabbit comment from PR fix(e2e): pin old gateway base fallback #4047: The diff updates create_old_docker_wrapper() so any explicit BASE_IMAGE build arg marks rewrote_base=1, preventing the duplicate fallback behavior described in the PR body.
• met — mark any explicit BASE_IMAGE Docker build arg as present, not just the mutable :latest literal: Added handling for '--build-arg' followed by any 'BASE_IMAGE=...' value and for '--build-arg=BASE_IMAGE=*', setting rewrote_base=1 for both forms while preserving the existing ':latest' rewrite path.
• met — keep the fix(e2e): pin old gateway base fallback #4047 fallback path for old installers that pass no BASE_IMAGE at all: The existing fallback remains: if rewrote_base is still 0 after argument parsing, the wrapper appends '--build-arg BASE_IMAGE=${base_ref}' and logs 'add build-arg BASE_IMAGE=%s'.
• met — CodeRabbit correctly noted that rewrote_base was only set for the ghcr.io/nvidia/nemoclaw/sandbox-base:latest literal, so a caller-provided non-latest BASE_IMAGE could receive a second fallback arg and be overridden by Docker last-arg-wins behavior.: The new conditions set rewrote_base=1 for caller-provided non-latest BASE_IMAGE values, so the later fallback block is skipped rather than adding a second BASE_IMAGE build arg.
• met — No actionable comments were generated in the recent review.: The CodeRabbit issue comment states no actionable comments were generated; GraphQL reviewThreads.nodes is empty in the provided context, though the reviewThreads hard-gate snapshot remains unknown.
• met — Required E2E: None: The E2E Advisor comment reports no required E2E jobs for this PR.
• partial — Optional E2E: openshell-gateway-upgrade-e2e: The E2E Advisor marks openshell-gateway-upgrade-e2e optional and useful for validating the edited harness; no required E2E is missing, and the optional job is not merge-blocking per the advisor.

Security review

• pass — 1. Secrets and Credentials: No hardcoded secrets, tokens, private keys, credential files, or new credential handling were introduced. The change only adjusts detection of BASE_IMAGE build arguments inside an E2E Docker wrapper.
• pass — 2. Input Validation and Data Sanitization: The touched logic parses shell arguments using quoted variables and array appends, with no eval, command substitution of untrusted input, or shell-string execution added. Explicit BASE_IMAGE values are detected only to avoid duplicate fallback injection.
• pass — 3. Authentication and Authorization: Not applicable — no endpoints, authentication flows, authorization checks, token validation, or privilege boundaries are modified.
• pass — 4. Dependencies and Third-Party Libraries: No dependencies, package registries, lockfiles, or third-party library versions are changed.
• pass — 5. Error Handling and Logging: No new sensitive logging is introduced. Existing logging remains limited to rewritten or fallback OPENCLAW_VERSION/BASE_IMAGE values used by the E2E wrapper.
• pass — 6. Cryptography and Data Protection: Not applicable — no cryptographic operations, encryption, hashing, certificates, or data protection mechanisms are changed.
• pass — 7. Configuration and Security Headers: No HTTP security headers, CORS, debug settings, port exposure, runtime container privileges, or production Dockerfile configuration are changed. The Docker build-arg handling is limited to the E2E old-installer wrapper.
• pass — 8. Security Testing: The PR changes test harness behavior only, required CI is passing, ShellCheck/CodeQL contexts are successful, and the E2E Advisor found no required E2E. Optional openshell-gateway-upgrade-e2e would provide additional confidence but is not required by the advisor.
• pass — 9. Holistic Security Posture: The change preserves explicit caller intent for BASE_IMAGE in an E2E wrapper and avoids accidental fallback override. It does not weaken sandbox, SSRF, credential, blueprint, installer, or workflow trusted-code boundaries.

Test / E2E status

• Test depth: unit_sufficient — Changes are limited to tests, documentation, or metadata that cannot affect runtime behavior directly.
• E2E Advisor: ok

✅ What looks good

• The patch is narrowly scoped to one E2E shell harness file and preserves the existing fallback behavior for old installers with no BASE_IMAGE argument.
• The new logic handles both '--build-arg BASE_IMAGE=...' and '--build-arg=BASE_IMAGE=...' forms while keeping the existing mutable ':latest' rewrite to the pinned fallback.
• Required CI contexts are passing for the provided head SHA, and CodeRabbit reported no actionable comments in the recent review.
• E2E Advisor was present and explicitly found no required E2E for this test-only change.

Review completeness

• This review used read-only inspection and trusted provided metadata; no tests or package-manager commands were executed.
• The reviewThreads hard-gate snapshot was provided as unknown, even though GraphQL context showed no reviewThreads nodes; maintainers should verify current GitHub review-thread state before merge consideration.
• The original PR fix(e2e): pin old gateway base fallback #4047 CodeRabbit discussion was not independently fetched; acceptance mapping relies on the trusted PR body/comment context and the local diff evidence.
• Human maintainer review required: yes