fix(sandbox): make workspace template seeding shell-serializable

[gauravprasadgp]

Summary

Fixes seed_default_workspace_templates startup failures caused by a heredoc inside an if block failing to survive the declare -f / bash -c round-trip. The skip-bootstrap check now uses node -e, keeping the serialized shell function flat and parseable.

Related Issue

Fixes #NNN

Changes

• Replaced the heredoc-based Node config check in seed_default_workspace_templates with a node -e snippet.
• Preserved existing behavior: seeding only runs when agents.defaults.skipBootstrap is true.
• Added a regression test that runs the function through declare -f and bash -c, then verifies template seeding still works.

Type of Change

• 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)

Verification

• npx prek run --all-files passes
• npm test passes
• Tests added or updated for new or changed behavior
• No secrets, API keys, or credentials committed
• Docs updated for user-facing behavior changes
• make docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

Focused verification run:

• bash -n scripts/nemoclaw-start.sh
• npm test -- test/nemoclaw-start.test.ts -t "round-trips seed_default_workspace_templates"
• git diff --check -- scripts/nemoclaw-start.sh test/nemoclaw-start.test.ts

---

Signed-off-by: gauravprasad prasadgaurav559@gmail.com

Summary by CodeRabbit

• Refactor

  • Updated the workspace template seeding “skip bootstrap” validation to use a simpler runtime check for when bootstrap should be bypassed.

• Tests

  • Added coverage to ensure default workspace template seeding completes successfully when bootstrap is skipped, confirming expected template output is created (and that bootstrap-related files are not).

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR changes seed_default_workspace_templates to use a JavaScript one-liner with node -e for skip-bootstrap gating instead of a heredoc Node invocation. It also adds a Vitest test that runs the shell function in a subprocess and checks the seeded template outputs.

Changes

Skip bootstrap check refactoring and testing

Layer / File(s)	Summary
Skip bootstrap check refactoring scripts/nemoclaw-start.sh	The skip-bootstrap validator replaces a heredoc with a skip_bootstrap_check variable containing a Node.js one-liner; the variable is executed via node -e to gate template seeding based on agents.defaults.skipBootstrap.
Test infrastructure and validation test/nemoclaw-start-workspace-template-seeding.test.ts	New Vitest test extracts the seed_default_workspace_templates function from the shell script, uses helper quoting for subprocess arguments, runs the function via bash -c with temporary directories and config, verifies seeding outputs and template file presence, and cleans up afterward.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% 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 clearly summarizes the main change: making workspace template seeding shell-serializable to fix sandbox startup behavior.
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 unit tests (beta)

• Create PR with unit tests

---

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

[cv]

@gauravprasadgp mind addressing the failing checks, please?

[github-actions]

Vitest E2E Scenario Results — ⚠️ Some jobs cancelled — partial pass

Run: 28117039423
Workflow ref: main
Requested scenarios: (default — all supported)
Requested jobs: (default — all default-enabled free-standing jobs; explicit-only jobs such as jetson-nvmap-gpu-vitest are skipped unless selected)
Summary: 1 passed, 0 failed, 66 cancelled, 1 skipped

Job	Result
agent-turn-latency-vitest	⚠️ cancelled
bedrock-runtime-compatible-anthropic-vitest	⚠️ cancelled
brave-search-vitest	⚠️ cancelled
channels-add-remove-vitest	⚠️ cancelled
channels-stop-start-vitest	⚠️ cancelled
cloud-inference-vitest	⚠️ cancelled
cloud-onboard-vitest	⚠️ cancelled
common-egress-agent-vitest	⚠️ cancelled
concurrent-gateway-ports-vitest	⚠️ cancelled
credential-migration-vitest	⚠️ cancelled
credential-sanitization-vitest	⚠️ cancelled
cron-preflight-inference-local-vitest	⚠️ cancelled
device-auth-health-vitest	⚠️ cancelled
diagnostics-vitest	⚠️ cancelled
double-onboard-vitest	⚠️ cancelled
full-e2e-vitest	⚠️ cancelled
gateway-drift-preflight-vitest	⚠️ cancelled
gateway-guard-recovery	⚠️ cancelled
gateway-health-honest-vitest	⚠️ cancelled
generate-matrix	✅ success
gpu-double-onboard-vitest	⚠️ cancelled
gpu-e2e-vitest	⚠️ cancelled
hermes-discord-vitest	⚠️ cancelled
hermes-e2e-vitest	⚠️ cancelled
hermes-inference-switch-vitest	⚠️ cancelled
hermes-root-entrypoint-smoke-vitest	⚠️ cancelled
hermes-sandbox-secret-boundary-vitest	⚠️ cancelled
hermes-slack-vitest	⚠️ cancelled
inference-routing-vitest	⚠️ cancelled
issue-2478-crash-loop-recovery-vitest	⚠️ cancelled
issue-4434-tui-unreachable-inference-vitest	⚠️ cancelled
issue-4462-scope-upgrade-approval-vitest	⚠️ cancelled
jetson-nvmap-gpu-vitest	⏭️ skipped
kimi-inference-compat-vitest	⚠️ cancelled
launchable-smoke-vitest	⚠️ cancelled
live-scenarios	⚠️ cancelled
messaging-compatible-endpoint-vitest	⚠️ cancelled
messaging-providers-vitest	⚠️ cancelled
model-router-provider-routed-inference-vitest	⚠️ cancelled
network-policy-vitest	⚠️ cancelled
ollama-auth-proxy-vitest	⚠️ cancelled
onboard-negative-paths-vitest	⚠️ cancelled
onboard-repair-vitest	⚠️ cancelled
onboard-resume-vitest	⚠️ cancelled
openclaw-discord-pairing-vitest	⚠️ cancelled
openclaw-inference-switch-vitest	⚠️ cancelled
openclaw-skill-cli-vitest	⚠️ cancelled
openclaw-slack-pairing-vitest	⚠️ cancelled
openclaw-tui-chat-correlation-vitest	⚠️ cancelled
openshell-gateway-upgrade-vitest	⚠️ cancelled
openshell-version-pin-vitest	⚠️ cancelled
overlayfs-autofix-vitest	⚠️ cancelled
rebuild-hermes-stale-base-vitest	⚠️ cancelled
rebuild-hermes-vitest	⚠️ cancelled
rebuild-openclaw-vitest	⚠️ cancelled
runtime-overrides-vitest	⚠️ cancelled
sandbox-rebuild-vitest	⚠️ cancelled
sandbox-survival-vitest	⚠️ cancelled
sessions-agents-cli-vitest	⚠️ cancelled
shields-config-vitest	⚠️ cancelled
skill-agent-vitest	⚠️ cancelled
snapshot-commands-vitest	⚠️ cancelled
spark-install-vitest	⚠️ cancelled
state-backup-restore-vitest	⚠️ cancelled
telegram-injection-vitest	⚠️ cancelled
token-rotation-vitest	⚠️ cancelled
tunnel-lifecycle-vitest	⚠️ cancelled
upgrade-stale-sandbox-vitest	⚠️ cancelled

Explicit-only jobs skipped: jetson-nvmap-gpu-vitest (default dispatch excludes Jetson until a stable Jetson runner is available; validate with jobs=jetson-nvmap-gpu-vitest or scenarios=jetson-nvmap-gpu).

[cv]

Manual PR Review Advisor result

This PR Review Advisor analysis was run manually via workflow_dispatch, so the workflow did not post its usual sticky comment. Posting the advisor summary here to populate the PR with advisor feedback.

Run: https://github.com/NVIDIA/NemoClaw/actions/runs/28211486658

Recommendation: needs_rework (high confidence; findings: 4)

The heredoc removal is directionally small, but the new node -e snippet is misquoted and prevents workspace template seeding from running.

---

PR Review Advisor

The heredoc removal is directionally small, but the new node -e snippet is misquoted and prevents workspace template seeding from running.

Required before merge

• Fix the node -e JavaScript quoting so skipBootstrap=true still seeds templates (scripts/nemoclaw-start.sh:3031): The new shell assignment is single-quoted but contains escaped double quotes, for example require(\"fs\") and readFileSync(configPath, \"utf8\"). In single-quoted bash strings those backslashes are literal, so Node receives invalid JavaScript, exits non-zero, and the function takes the existing return 0 skip path before seeding.
  • Impact: Pristine OpenClaw workspaces with agents.defaults.skipBootstrap=true will not receive the default templates, regressing the behavior this PR is meant to preserve and leaving the workspace empty under the NemoClaw bootstrap path.
  • Recommendation: Remove the unnecessary backslashes inside the single-quoted shell string, e.g. const fs = require("fs"); ... readFileSync(configPath, "utf8"), while continuing to pass $config_file as a quoted argv rather than interpolating it into JavaScript.
  • Verification hint: Read scripts/nemoclaw-start.sh at the local skip_bootstrap_check=... line and confirm the JavaScript passed to node -e is valid without literal backslashes before string delimiters; the shortest behavioral check is the changed Vitest case that expects AGENTS.md to be seeded for skipBootstrap:true.
  • Missing regression test: The newly added round-trips seed_default_workspace_templates through declare -f without a heredoc test already exercises the positive skipBootstrap:true behavior and should fail until this is fixed; keep that assertion and ensure it proves AGENTS.md is written after the quoting change.
  • Evidence: Diff replaces the heredoc with local skip_bootstrap_check='const fs = require(\"fs\"); ... readFileSync(configPath, \"utf8\") ...' followed by if ! node -e "$skip_bootstrap_check" "$config_file" ...; then return 0; fi.

Resolve or justify before merge

• Source-of-truth review needed: Heredoc-to-node-e compatibility workaround in seed_default_workspace_templates: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Verification hint: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: The PR adds a direct declare -f/bash -c round-trip test, but should also add or identify a production-path test through seed_default_workspace_templates_as_sandbox if that is the real source of startup behavior.
  • Evidence: Nearby comments in scripts/nemoclaw-start.sh state run_step_down_as_sandbox runs functions without going through declare -f, and seed_default_workspace_templates_as_sandbox dispatches via that helper.
• Align the regression test with the production step-down path or document the remaining declare -f caller (test/nemoclaw-start-workspace-template-seeding.test.ts:29): The new test specifically validates a declare -f plus bash -c round trip, but nearby production code now dispatches seed_default_workspace_templates_as_sandbox through run_step_down_as_sandbox, which extracts function source from disk and explicitly avoids declare -f. That makes the test look like it is covering a legacy serialization boundary unless there is another production caller still using declare -f for this function.
  • Impact: The PR can carry a localized workaround for a boundary that production no longer uses while missing regressions in the actual sandbox step-down dispatch path that owns file ownership and startup behavior.
  • Recommendation: Either point the test at the production wrapper (seed_default_workspace_templates_as_sandbox via run_step_down_as_sandbox) or add code/test evidence for the remaining production declare -f caller that requires this compatibility. Keep the heredoc absence assertion only if that compatibility boundary is real.
  • Verification hint: Read scripts/nemoclaw-start.sh around seed_default_workspace_templates_as_sandbox and run_step_down_as_sandbox; verify whether any changed or nearby production path still runs bash -c "$(declare -f seed_default_workspace_templates) ...".
  • Missing regression test: Add or identify a production-path regression that invokes seed_default_workspace_templates_as_sandbox and verifies a pristine workspace is seeded through run_step_down_as_sandbox after the node -e refactor.
  • Evidence: seed_default_workspace_templates_as_sandbox calls run_step_down_as_sandbox ... seed_default_workspace_templates, and run_step_down_as_sandbox calls _step_down_extract_function rather than declare -f; the new test constructs its own bash -c "$(declare -f seed_default_workspace_templates) ..." path.

In-scope improvements

• Avoid a new ts-nocheck test file for a small typed Vitest case (test/nemoclaw-start-workspace-template-seeding.test.ts:1): The added test file disables TypeScript checking and duplicates a local shell-function extraction helper even though the repository already has extensive nemoclaw-start Vitest coverage for this surface.
  • Impact: Disabling type checking in new tests makes future helper mistakes easier to miss, and a separate helper/file increases drift from the existing workspace-template seeding suite.
  • Recommendation: Remove // @ts-nocheck if the file remains, or fold this focused regression into the existing seed_default_workspace_templates (#3240) suite so it can reuse nearby patterns and coverage.
  • Verification hint: Read the new test file and confirm whether TypeScript emits any actual type errors requiring suppression; compare with test/nemoclaw-start.test.ts around the existing seed_default_workspace_templates (#3240) suite.
  • Missing regression test: No additional behavior test is required for this quality issue; preserve the positive seeding assertion while moving it or re-enabling type checking.
  • Evidence: The new file starts with // @ts-nocheck and defines its own extractShellFunctionFromSource helper for a single test.

Test follow-ups to resolve or justify

• Runtime validation — Verify seed_default_workspace_templates with agents.defaults.skipBootstrap=true writes AGENTS.md into a pristine workspace after the node-e refactor.. This patch changes bootstrap shell behavior in a sandbox lifecycle path. Static review found a likely runtime failure in the node -e program, so behavior-level validation is needed even though the diff is small.
• Runtime validation — Verify seed_default_workspace_templates with agents.defaults.skipBootstrap=false does not seed templates.. This patch changes bootstrap shell behavior in a sandbox lifecycle path. Static review found a likely runtime failure in the node -e program, so behavior-level validation is needed even though the diff is small.
• Runtime validation — Verify seed_default_workspace_templates_as_sandbox dispatches through run_step_down_as_sandbox and seeds the workspace through the production wrapper.. This patch changes bootstrap shell behavior in a sandbox lifecycle path. Static review found a likely runtime failure in the node -e program, so behavior-level validation is needed even though the diff is small.
• Align the regression test with the production step-down path or document the remaining declare -f caller — Either point the test at the production wrapper (seed_default_workspace_templates_as_sandbox via run_step_down_as_sandbox) or add code/test evidence for the remaining production declare -f caller that requires this compatibility. Keep the heredoc absence assertion only if that compatibility boundary is real.
• Avoid a new ts-nocheck test file for a small typed Vitest case — Remove // @ts-nocheck if the file remains, or fold this focused regression into the existing seed_default_workspace_templates (#3240) suite so it can reuse nearby patterns and coverage.
• Acceptance clause: Fixes #NNN — add test evidence or identify existing coverage. The deterministic context reports linkedIssues: [], so there is no real linked issue or issue comment acceptance text to validate.
• Acceptance clause: Fixes seed_default_workspace_templates startup failures caused by a heredoc inside an if block failing to survive the declare -f / bash -c round-trip. — add test evidence or identify existing coverage. The diff removes the heredoc from seed_default_workspace_templates, and the new test exercises a declare -f/bash -c round trip. However, production appears to use run_step_down_as_sandbox source extraction rather than declare -f, so the acceptance boundary needs code evidence or test realignment.
• Acceptance clause: The skip-bootstrap check now uses node -e, keeping the serialized shell function flat and parseable. — add test evidence or identify existing coverage. The diff uses node -e, but the assigned JavaScript string contains literal backslash-escaped quotes under bash single quotes, making the program invalid despite being heredoc-free.

What looks good

• The patch is narrow and reduces heredoc complexity in a high-risk bootstrap shell function.
• The config path is still passed to Node as a quoted argv, avoiding interpolation of a path into executable JavaScript.
• Existing symlink refusal, pristine-workspace protection, and BOOTSTRAP.md exclusion remain unchanged.

[cv]

Maintainer follow-up pushed to the fork branch: fixed the static guardrail blockers by applying the formatter output, removing the new test-file ts-nocheck, dropping the source-shape assertion, and rewriting the helper without an added if statement.\n\nValidation run locally before push:\n- npm test -- test/nemoclaw-start-workspace-template-seeding.test.ts\n- npm run source-shape:check\n- targeted pre-push hooks equivalent to CI static checks\n- full pre-push hook set passed with umask 0022\n\nCurrent post-push checks: codebase-growth-guardrails and require-maintainer-edits are green on 1df330a.

[gauravprasadgp]

@cv can you please review this MR, I have addressed the comments.

[cjagwani]

Reviewed exact head d61c9d87. The final two-file diff preserves the skip-bootstrap contract while making the shell function serializable: the Node program is valid, the config path is passed as argv, and the production step-down test now executes the real seeding function and verifies AGENTS/SOUL are written while BOOTSTRAP is excluded. All required checks are green and no unresolved review threads remain.