fix(e2e): preserve explicit old base arg

[jyaunches]

Summary

• address the unresolved CodeRabbit comment from PR fix(e2e): pin old gateway base fallback #4047
• mark any explicit BASE_IMAGE Docker build arg as present, not just the mutable :latest literal
• keep the fix(e2e): pin old gateway base fallback #4047 fallback path for old installers that pass no BASE_IMAGE at all

Why

PR #4047 fixed the nightly gateway-upgrade fixture by appending a pinned BASE_IMAGE when the old installer omitted one. 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.

Test plan

• bash -n test/e2e/test-openshell-gateway-upgrade.sh
• git diff --check
• shell wrapper smoke: verified no-BASE adds pinned fallback, latest rewrites to pinned fallback, explicit custom BASE_IMAGE does not add pinned fallback
• npm run build:cli
• npm run typecheck:cli
• npm run test -- test/onboard-openshell-version.test.ts

Follow-up to PR #4047.

Resolves CodeRabbit thread: #4047 (comment)

Summary by CodeRabbit

• Tests
  • Improved Docker wrapper in test suite to correctly handle pre-specified build arguments, preventing duplicate argument injection and ensuring reliable test execution.

[coderabbitai]

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: 929b64f6-829d-4366-8e25-20140b648c67

📥 Commits

Reviewing files that changed from the base of the PR and between 6431f33 and 0a1ee17.

📒 Files selected for processing (1)

• test/e2e/test-openshell-gateway-upgrade.sh

---

📝 Walkthrough

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

Layer / File(s)	Summary
Detect provided BASE_IMAGE arguments test/e2e/test-openshell-gateway-upgrade.sh	The wrapper now matches both --build-arg BASE_IMAGE=... and --build-arg=BASE_IMAGE=... forms in argument parsing, setting rewrote_base=1 when either pattern is found to prevent fallback injection of a default BASE_IMAGE build-arg.

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. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(e2e): preserve explicit old base arg' clearly and concisely describes the main change: preventing the test script from overriding explicit BASE_IMAGE arguments.
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.

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

✨ 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 fix/4047-coderabbit-base-arg

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

E2E Advisor Recommendation

Required E2E: None
Optional E2E: openshell-gateway-upgrade-e2e

Dispatch hint: openshell-gateway-upgrade-e2e

Workflow run

Full advisor summary

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

[github-actions]

PR Review Advisor

Recommendation: blocked
Confidence: high
Analyzed HEAD: 0a1ee17c6d6d5e60de19fa1dde23f8ab044f3727
Findings: 1 blocker(s), 1 warning(s), 0 suggestion(s)

This is an automated advisory review. A human maintainer must make the final merge decision.

Limitations: 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 #4047 CodeRabbit discussion was not independently fetched; acceptance mapping relies on the trusted PR body/comment context and the local diff evidence.

Workflow run

Full advisor summary

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

[wscurran]

✨
Related open PRs:

• #4047 fix(e2e): pin old gateway base fallback