fix(security): close P1 code scanning findings

[cv]

Summary

Close the P1 code-scanning buckets from #3654 by hardening small production correctness and logging paths plus the production-ish ShellCheck findings. The changes keep behavior intact while failing closed around shields helper availability and avoiding HOME-derived shim paths in TypeScript dev-shim logs.

Related Issue

Refs #3654

Changes

• Redact environment-derived shim paths from src/lib/actions/dev/npm-link-or-shim.ts logs and add regression assertions that HOME/repo paths are not emitted.
• Add defensive resolver checks around snapshot shields-state verification and shields timer auto-restore locking so missing helper exports fail closed instead of producing ambiguous TypeErrors.
• Rewrite the two production-ish SC2015 shell patterns in agents/hermes/start.sh and scripts/nemoclaw-start.sh as explicit control flow.

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

Note: npm test was attempted, but the existing installer integration test test/install-preflight.test.ts > warns on Podman but still runs onboarding fails in this environment because host CDI detection turns the expected Podman warning path into a missing-CDI issue path before onboarding.

---

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

• Bug Fixes

  • Stricter validation before cleanup and restore operations; operations now refuse to proceed when required helpers or ports are unavailable.
  • Improved logging: user-facing path formatting and clearer permission guidance; sensitive paths and tokens are redacted.

• Tests

  • Added and updated tests to assert refusal behavior when gate helpers are missing and to verify sanitized/redacted logs and output formatting.

[coderabbitai]

Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes.

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: 3be0a01a-4389-436a-8642-7dea00305e7d

📥 Commits

Reviewing files that changed from the base of the PR and between cbcc73f and 5ba3895.

📒 Files selected for processing (6)

• src/lib/actions/dev/npm-link-or-shim.test.ts
• src/lib/actions/dev/npm-link-or-shim.ts
• src/lib/actions/sandbox/snapshot.test.ts
• src/lib/actions/sandbox/snapshot.ts
• src/lib/shields/timer.test.ts
• src/lib/shields/timer.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• src/lib/actions/sandbox/snapshot.ts
• src/lib/shields/timer.ts

---

📝 Walkthrough

Walkthrough

Hardens shell scripts and runtime modules by adding explicit guards for optional helpers, replaces direct imports with namespace imports to permit existence checks, sanitizes npm shim output and user-facing shim paths, and adds tests to verify redaction and guarded failure behavior.

Changes

Defensive hardening and user-facing improvements

Layer / File(s)	Summary
Shell script defensive guards agents/hermes/start.sh, scripts/nemoclaw-start.sh	Dashboard socat cleanup now explicitly checks that both port variables are non-empty before proceeding; workspace template path resolution uses structured if/then logic instead of combined one-liners to clarify behavior on directory-change failure.
Shim messaging, sanitization, and tests src/lib/actions/dev/npm-link-or-shim.ts, src/lib/actions/dev/npm-link-or-shim.test.ts	Adds output-sanitization helpers that redact repo/home paths and likely secret-like tokens, introduces shimDirDisplay/shimPathDisplay for user-facing messages, updates shim-creation and failure logs to use display paths and optional error codes, and tightens tests to assert redaction and absence of absolute homeDir/repoDir.
Shields defensive integration and tests src/lib/actions/sandbox/snapshot.ts, src/lib/shields/timer.ts, src/lib/actions/sandbox/snapshot.test.ts, src/lib/shields/timer.test.ts	Switches to namespace imports for shields, adds isSnapshotCreationAllowedByShields and resolveLockAgentConfig() helpers that verify helper exports exist and are callable before invoking them, wires these checks into snapshot creation and restore timer flows, and adds tests asserting operations fail closed when helpers are unavailable.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• NVIDIA/NemoClaw#4155: Related shields/index changes to lockAgentConfig that connect to runtime resolver behavior introduced here.

Suggested labels

area: security, bug-fix, area: cli, area: sandbox

Suggested reviewers

• prekshivyas
• ericksoa
• cjagwani

Poem

🐰 I hopped through guards both stout and keen,
Replaced raw paths with a friendly sheen,
When shields are missing, I say "no go",
Redacted secrets, set logs to show,
A safer start, a tidier glow.

🚥 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 accurately describes the main objective of the PR—closing P1 code scanning security findings through defensive checks and output redaction.
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/security-p1-code-scanning

---

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

[github-actions]

E2E Advisor Recommendation

Required E2E: snapshot-commands-e2e, shields-config-e2e, hermes-root-entrypoint-smoke-e2e, cloud-e2e
Optional E2E: hermes-dashboard-e2e, credential-sanitization-e2e

Dispatch hint: snapshot-commands-e2e,shields-config-e2e,hermes-root-entrypoint-smoke-e2e,cloud-e2e

Auto-dispatched E2E: snapshot-commands-e2e, shields-config-e2e, hermes-root-entrypoint-smoke-e2e, cloud-e2e via nightly-e2e.yaml at 5ba38958a2f326e089b3d958abcfa545b4b2b7cb — nightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

• snapshot-commands-e2e (medium (~30 min, live sandbox, NVIDIA_API_KEY)): Directly covers the changed snapshot command path against a live sandbox: install, write sandbox state, snapshot create/list/restore, and verify snapshot contents do not include credentials.
• shields-config-e2e (medium (~30 min, live sandbox, NVIDIA_API_KEY)): Directly covers the changed shields timer and lock boundary with full shields up/down lifecycle, audit trail checks, content seal drift detection, and auto-restore timer behavior in a live sandbox.
• hermes-root-entrypoint-smoke-e2e (medium (~45 min, Docker image build)): Builds the real Hermes image and exercises /usr/local/bin/nemoclaw-start, Hermes health, privilege separation, runtime layout repair, PID migration, and config protection; this is the closest existing coverage for agents/hermes/start.sh entrypoint changes.
• cloud-e2e (medium-high (live sandbox + NVIDIA endpoint)): scripts/nemoclaw-start.sh is the OpenClaw sandbox startup script. The full source-install OpenClaw E2E validates install.sh, onboard, sandbox startup, and live inference for the default real assistant user flow that can be affected by startup/template-discovery changes.

Optional E2E

• hermes-dashboard-e2e (medium-high (live Hermes sandbox + NVIDIA_API_KEY)): agents/hermes/start.sh touched dashboard socat forwarder cleanup. Run if extra confidence is desired for dashboard/API host reachability and dashboard-specific forwarding; the change is small enough that root-entrypoint smoke is the merge-blocking Hermes check.
• credential-sanitization-e2e (medium-high (live sandbox, security suite)): Optional adjacent confidence for secret-boundary behavior because npm-link fallback logging now redacts token-shaped output. Existing coverage is broader runtime credential sanitization, not a direct npm-link fallback test.

New E2E recommendations

• Installer/dev CLI exposure and log sanitization (medium): Existing E2E jobs exercise source install but do not appear to force npm link failure and then assert the managed ~/.local/bin/nemoclaw shim is created while token-shaped npm stdout/stderr and absolute HOME/repo paths are redacted from logs.
  • Suggested test: Add a hermetic installer E2E that injects a failing npm link with NVIDIA_API_KEY/Bearer tokens and absolute paths in output, runs install from source, verifies ~/.local/bin/nemoclaw works, and asserts install logs contain only redacted/display paths.
• Snapshot/shields CommonJS export boundary (low): The new snapshot and timer behavior intentionally fails closed when shields exports are unavailable. Unit tests mock this, but there is no live packaged CLI E2E that validates the built artifact's interop surface still exposes the required shields helpers before snapshot/timer side effects occur.
  • Suggested test: Add a packaged-CLI smoke E2E or build-artifact check that imports the compiled snapshot/timer paths and verifies shields helper exports are present, then runs a no-side-effect snapshot-create preflight failure assertion when the helper cannot be resolved.

Dispatch hint

• Workflow: .github/workflows/nightly-e2e.yaml
• jobs input: snapshot-commands-e2e,shields-config-e2e,hermes-root-entrypoint-smoke-e2e,cloud-e2e

[github-actions]

E2E Scenario Advisor Recommendation

Required scenario E2E: ubuntu-repo-cloud-openclaw, ubuntu-repo-cloud-hermes
Optional scenario E2E: wsl-repo-cloud-openclaw, macos-repo-cloud-openclaw

Dispatch required scenario E2E:

• gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw
• gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-hermes

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

• ubuntu-repo-cloud-openclaw: Covers the primary Ubuntu repo-checkout cloud OpenClaw path affected by scripts/nemoclaw-start.sh and npm-link-or-shim changes, and provides broad smoke coverage for sandbox/startup behavior touched by snapshot and shields code.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw
• ubuntu-repo-cloud-hermes: Directly exercises the Hermes cloud onboarding/gateway path affected by agents/hermes/start.sh, including Hermes-specific health and writable-state checks.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-hermes

Optional scenario E2E

• wsl-repo-cloud-openclaw: Optional platform coverage for shell/startup and repo-checkout behavior on WSL; useful because the PR touches bash startup/install paths, but Ubuntu is the primary target.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=wsl-repo-cloud-openclaw
• macos-repo-cloud-openclaw: Optional platform coverage for repo-checkout CLI availability and install-path behavior on macOS; special runner coverage is adjacent rather than primary for these changes.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=macos-repo-cloud-openclaw

Relevant changed files

• agents/hermes/start.sh
• scripts/nemoclaw-start.sh
• src/lib/actions/dev/npm-link-or-shim.ts
• src/lib/actions/sandbox/snapshot.ts
• src/lib/shields/timer.ts

[github-actions]

PR Review Advisor

Findings: 0 needs attention, 1 worth checking, 0 nice ideas
Top item: PR review advisor unavailable

Review findings

🛠️ Needs attention

• None.

🔎 Worth checking

• PR review advisor unavailable: The automated advisor could not complete: Could not parse JSON from PR review advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/pr-review-advisor/pr-review-advisor-raw-output.txt
  • Recommendation: Re-run the PR Review Advisor or perform a manual review.
  • Evidence: Could not parse JSON from PR review advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/pr-review-advisor/pr-review-advisor-raw-output.txt

🌱 Nice ideas

• None.

Consider writing more tests for

• **Runtime validation** — Add or identify targeted runtime/integration validation for the changed behavior; do not report external E2E job pass/fail here.. Runtime/sandbox/infrastructure paths need behavioral runtime validation: agents/hermes/start.sh, scripts/nemoclaw-start.sh, src/lib/actions/dev/npm-link-or-shim.ts, src/lib/actions/sandbox/snapshot.ts, src/lib/shields/timer.ts.

Workflow run details

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

[cv]

Addressed the PR Review Advisor feedback in commit 5ba3895:

• Sanitized raw npm link fallback output before logging, including repo/home path redaction and token-like value redaction, with a regression test.
• Added a snapshot action regression test that proves snapshot creation fails closed and does not call backupSandboxState when the shields gate helper is unavailable.
• Added a shields timer regression test that proves auto-restore leaves shields down, exits nonzero, and writes audit entries when the lock helper export is unavailable.
• Added source-boundary comments explaining why the runtime guards fail closed at the shields/policy boundary.

Local verification:

• npx vitest run src/lib/actions/dev/npm-link-or-shim.test.ts src/lib/actions/sandbox/snapshot.test.ts src/lib/shields/timer.test.ts passes
• npm run typecheck:cli passes
• npx prek run --all-files passes

[github-actions]

Selective E2E Results — ✅ All requested jobs passed

Run: 27048227856
Target ref: 5ba38958a2f326e089b3d958abcfa545b4b2b7cb
Workflow ref: main
Requested jobs: snapshot-commands-e2e,shields-config-e2e,hermes-root-entrypoint-smoke-e2e,cloud-e2e
Summary: 4 passed, 0 failed, 0 skipped

Job	Result
cloud-e2e	✅ success
hermes-root-entrypoint-smoke-e2e	✅ success
shields-config-e2e	✅ success
snapshot-commands-e2e	✅ success