Walkthrough

This PR fixes a bash syntax error (fi near line 20) that occurred when nemoclaw-start dispatched functions containing heredocs through the sandbox user. The root cause was declare -f reshaping heredoc-bearing functions incorrectly. The solution introduces a new _step_down_extract_function utility that extracts function definitions byte-exact from disk using extdebug metadata and awk, avoiding serialization. The dispatcher now uses this utility with bash -n syntax validation before execution.

Changes

Step-down function dispatch fix for heredoc handling

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• NVIDIA/NemoClaw#4528: Both PRs modify the step-down mechanism in scripts/nemoclaw-start.sh to prevent syntax/serialization failures when dispatching functions with heredocs under the sandbox user.

Suggested labels

fix, area: sandbox, bug-fix

Suggested reviewers

• jyaunches
• cv

Poem

🐰 A heredoc walked into a fi,
But declare -f made bash cry,
So we extract from the source file true,
Byte-exact and whole—the syntax shines through!
No more fi surprises, we're sandbox-safe now. 🎉

_{✏️ 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

• cloud-onboard-e2e (high): Validates install.sh plus real OpenClaw onboard/startup on a clean environment, exercising the entrypoint path where the changed step-down helper is used.
• sandbox-survival-e2e (high): Covers gateway stop/start and sandbox restart recovery; the modified helper runs during startup and could break restart-time entrypoint setup even if initial onboard succeeds.
• test-e2e-gateway-isolation (medium): Builds the production sandbox image and validates gateway/sandbox isolation and entrypoint hardening, which is directly adjacent to the changed privilege step-down path.

Optional E2E

• openclaw-onboard-security-posture-e2e (high): Useful extra confidence for OpenClaw onboard with runtime guard/security-posture assertions after changing entrypoint privilege and trusted-file assembly behavior.
• test-non-root-sandbox-smoke (low): Adjacent entrypoint smoke coverage for the non-root fallback path; the changed helper is root-mode focused, so this is not merge-blocking but can catch accidental entrypoint syntax or setup regressions.

New E2E recommendations

• root-mode step-down heredoc regression (high): The PR adds strong unit coverage for here-doc extraction, but there is no clearly targeted E2E that runs the production image root entrypoint and asserts seed_default_workspace_templates_as_sandbox/setup_auth_profile_as_sandbox succeed with here-doc-bearing functions and no generated-script syntax failure.
  • Suggested test: Add a targeted root-entrypoint step-down E2E, or extend test-e2e-gateway-isolation.sh, to start the production image through nemoclaw-start in root mode and assert the step-down helper executes workspace seeding/auth-profile setup without 'unexpected token fi' or bash -n failures.

Dispatch hint

• Workflow: nightly-e2e.yaml
• jobs input: cloud-onboard-e2e,sandbox-survival-e2e

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

• ubuntu-repo-cloud-openclaw: scripts/nemoclaw-start.sh is a startup/install-helper path exercised by repo-current scenario onboarding. The Ubuntu OpenClaw cloud scenario is the smallest primary path that validates sandbox startup, gateway health, credential/auth-profile handling, and baseline onboarding after the step-down helper changes.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Optional scenario E2E

• wsl-repo-cloud-openclaw: Optional adjacent platform coverage for the same repo-current OpenClaw startup path under WSL; useful because shell/startup behavior can differ, but it uses a special Windows/WSL runner so it is not the primary required path.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=wsl-repo-cloud-openclaw
• gpu-repo-local-ollama-openclaw: Optional adjacent coverage for the same repo-current OpenClaw startup path with local Ollama/GPU runtime; useful if maintainers want to validate startup under the GPU/local-provider path, but it requires a special GPU runner.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=gpu-repo-local-ollama-openclaw

Relevant changed files

• scripts/nemoclaw-start.sh

🛠️ Needs attention

• None.

🔎 Worth checking

• Source-of-truth review needed: run_step_down_as_sandbox source extraction workaround: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: The workaround explains the invalid state and adds a focused regression, but the extractor's documented fail-closed cases are not fully implemented/tested; see scripts/nemoclaw-start.sh:2792.
• Make step-down source extraction fail closed on malformed functions (scripts/nemoclaw-start.sh:2792): _step_down_extract_function is now part of a sandbox trusted-code boundary, but its implementation does not fully match the documented contract. The comment says extraction returns 1 when the matching closing `}` is not found, yet the awk END block only exits nonzero when EOF occurs while still inside a heredoc. A malformed or future-edited dispatched function that enters extraction and reaches EOF outside heredoc can be treated as successfully extracted until the later `bash -n` guard catches it, if it catches it. The helper also relies on trusted static function names without an explicit identifier allowlist and leaves the generated temp script world-readable, which remains safe only while future call sites never embed secret values in snippets or function bodies.
  • Recommendation: Have awk track whether the column-0 closing brace was seen and exit 1 when EOF occurs without it; add an explicit shell-function-name allowlist/regex check before passing `fn` into awk; and add negative tests for missing closers, bad opener/function names, and the `bash -n` failure path. Keep the no-secret-in-temp-script contract documented or enforced for any future call site.
  • Evidence: The new extractor at scripts/nemoclaw-start.sh:2737-2793 only has `END { if (in_fn && in_heredoc) exit 1 }`, while the function comment says missing matching `}` returns 1. `run_step_down_as_sandbox` writes the extracted source to `/tmp/nemoclaw-step-down-XXXXXX.sh` with mode 0644 before step-down execution.
• Add targeted runtime validation for the CAP_DAC_OVERRIDE startup path (test/nemoclaw-start.test.ts:4507): The unit and synthetic-script tests cover the heredoc-in-if regression and simulate the direct-root CAP_DAC_OVERRIDE helper chain, but the linked issue specifically reproduces in a built sandbox image with `--cap-drop=CAP_DAC_OVERRIDE` invoking `/usr/local/bin/nemoclaw-start` on Ubuntu 24.04. That runtime/container boundary is not exercised by this diff, so the acceptance clause about the normal image entrypoint keeping the container running is only partially covered.
  • Recommendation: Add or identify a targeted runtime/integration validation that runs the built sandbox image with CAP_DAC_OVERRIDE dropped through the normal `nemoclaw-start` entrypoint and verifies no `fi` syntax error, token/proxy preparation, read-only RC files, and continued startup. Do not rely solely on the synthetic unit composition for this infrastructure path.
  • Evidence: The new regression test at test/nemoclaw-start.test.ts:4199-4238 asserts the heredoc-in-if case through a stubbed `STEP_DOWN_PREFIX_SANDBOX`. The direct-root composition test starts at test/nemoclaw-start.test.ts:4507, but it is still a synthetic script rather than a built image run with actual cap-drop and entrypoint behavior.

🌱 Nice ideas

• None.