fix(sandbox): reject managed startup env wrapper without nemoclaw-start terminator - #8609
fix(sandbox): reject managed startup env wrapper without nemoclaw-start terminator#8609laitingsheng wants to merge 2 commits into
Conversation
…rt terminator Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…cases Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughThe entrypoint wrapper now uses a centralized managed-variable allowlist, tracks leading assignments, rejects malformed startup handoffs, and preserves command-tail arguments. Tests cover valid command tails and unsafe or malformed assignment sequences. ChangesManaged environment validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
laitingsheng
added
area: onboarding
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/entrypoint-env-wrapper.test.ts`:
- Around line 87-100: Update the two affected test titles in the enclosing
describe block, including “leaves a user command tail that only looks like a
managed assignment,” by appending the exact suffix “(`#8595`)” to each title while
preserving their existing wording.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4c49839e-841c-4474-9014-9a9e4f262e5d
📒 Files selected for processing (2)
scripts/lib/entrypoint-env-wrapper.shtest/entrypoint-env-wrapper.test.ts
| it("leaves a user command tail that only looks like a managed assignment", () => { | ||
| const result = runNormalizer([ | ||
| "env", | ||
| "FOO=bar", | ||
| "/bin/sh", | ||
| "-c", | ||
| "NEMOCLAW_SANDBOX_NAME=probe", | ||
| ]); | ||
|
|
||
| expect(result.status).toBe(0); | ||
| expect(result.stdout).toContain( | ||
| "ARG=env\nARG=FOO=bar\nARG=/bin/sh\nARG=-c\nARG=NEMOCLAW_SANDBOX_NAME=probe\n", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test/entrypoint-env-wrapper.test.ts --items all
sed -n '1,155p' test/entrypoint-env-wrapper.test.tsRepository: NVIDIA/NemoClaw
Length of output: 6925
Append (#8595) to both test titles. The enclosing describe title has no issue suffix.
🤖 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/entrypoint-env-wrapper.test.ts` around lines 87 - 100, Update the two
affected test titles in the enclosing describe block, including “leaves a user
command tail that only looks like a managed assignment,” by appending the exact
suffix “(`#8595`)” to each title while preserving their existing wording.
Sources: Coding guidelines, Learnings
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
5 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. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
1 participant
Summary
The managed startup env wrapper only rejected a missing
nemoclaw-startterminator when the argument sequence carriedNEMOCLAW_STARTUP_PROFILE_B64orNEMOCLAW_CORPORATE_CA_B64, soenv NEMOCLAW_AUTO_PAIR_FAST_REENTRY_INTERVAL_SECS=5 /bin/shreturned success and kept the unnormalized arguments. It now rejects any supported managed variable name that appears in the leading assignment run without the terminator, while a user command tail that only looks like an assignment still passes through unchanged.Related Issue
Fixes #8595
Changes
scripts/lib/entrypoint-env-wrapper.shout of the inlinecaseinto one_nemoclaw_supported_namesvalue, so the terminator-missing branch and the assignment validation loop read the same list instead of drifting apart.NEMOCLAW_STARTUP_PROFILE_B64andNEMOCLAW_CORPORATE_CA_B64in any argument position is unchanged, so no current guarantee is relaxed.env FOO=bar /bin/sh -c NEMOCLAW_SANDBOX_NAME=probestays a user command.test/entrypoint-env-wrapper.test.tspins that boundary.test/entrypoint-env-wrapper.test.tscovering the reported sequence, a two-assignment variant, and a trust payload in the command tail, and give every case in that table a distinct name so a failure identifies the broken contract.Type of Change
Quality Gates
docs/, and the only producer of this argument sequence is the launch renderer, which always emits the terminator. The newly rejected shape is reachable only by a hand-built container command.Documentation Writer Review
no-docs-neededdocs/and.agents/for the wrapper filename, the argument grammar, and each of its[SECURITY]strings and found no reference, checked thedocs/CONTRIBUTING.md"When to Update Docs" conditions and matched none, and confirmed that no documenteddocker runordocker execexample passes a command of the affected shape. Its two substantive prose findings were applied in 4ad4655: a comment claim that overstated the rejection rule and contradicted the pass-through test, and a templated test title that rendered identically for five of seven cases.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run test/entrypoint-env-wrapper.test.ts— 12/12 pass.npx vitest run test/pr-risk-plan.test.ts test/sandbox-provisioning-helper-permissions.test.ts test/sandbox-rlimit-hooks.test.ts test/hermes-doctor-config-hash.test.ts— 128/128 pass.test/hermes-start.test.ts,test/service-env.test.ts,test/sandbox-provisioning.test.ts,test/sandbox-build-context.test.ts,test/openclaw-final-image-layout.test.ts, andtest/hermes-final-image-layout.test.tsalso pass.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit