fix(security): use secure temp paths in tests

[cv]

Summary

Use private mkdtempSync roots for test artifacts that previously wrote predictable files under the OS temp directory. This clears the open CodeQL js/insecure-temporary-file findings while keeping the behavior-focused test coverage intact.

Changes

• Use os.tmpdir() for the tunnel service default PID root so tests can bind it to a private temp directory.
• Move service environment wrapper scripts and proxy-env fixtures into mkdtempSync directories.
• Move uninstall PID-file test homes into mkdtempSync directories.

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)

---

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

[github-actions]

E2E Advisor Recommendation

Required E2E: tunnel-lifecycle-e2e, sandbox-operations-e2e
Optional E2E: channels-stop-start-e2e

Dispatch hint: tunnel-lifecycle-e2e,sandbox-operations-e2e

Auto-dispatched E2E: tunnel-lifecycle-e2e, sandbox-operations-e2e via nightly-e2e.yaml at 60cbb867e02c3902d4f2a36f2934dfb0f64a8f66 — nightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

• tunnel-lifecycle-e2e (medium): Directly exercises nemoclaw tunnel start, tunnel URL/status behavior, and nemoclaw tunnel stop, which depend on service PID/log directory resolution changed in src/lib/tunnel/services.ts.
• sandbox-operations-e2e (high): Validates real sandbox lifecycle operations and cleanup paths. The changed services code participates in stopping sandbox-side gateway/channels and cleaning host-side service PID files for the effective sandbox.

Optional E2E

• channels-stop-start-e2e (high): Useful adjacent confidence for stop/start of messaging channels because services.ts owns stopSandboxChannels, but the code change is focused on PID directory resolution rather than provider channel semantics.

New E2E recommendations

• tunnel/service PID directory portability (medium): Existing tunnel-lifecycle coverage appears Linux-/tmp-oriented. Add a focused E2E that sets TMPDIR to a non-default directory, runs tunnel start/status/stop, and verifies cloudflared PID/log files and cleanup use os.tmpdir() consistently.
  • Suggested test: Add a TMPDIR override case to test/e2e/test-tunnel-lifecycle.sh or a new test/e2e/test-tunnel-tmpdir.sh.

Dispatch hint

• Workflow: nightly-e2e.yaml
• jobs input: tunnel-lifecycle-e2e,sandbox-operations-e2e

[github-actions]

E2E Scenario Advisor Recommendation

Required scenario E2E: None
Optional scenario E2E: None

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

• None. No scenario E2E run recommended: the changes are unit/test updates plus a tunnel service PID-directory implementation change outside test/e2e-scenario and outside the scenario workflows/catalog/runtime. The scenario validation suites do not exercise the host-side tunnel start/stop/status PID directory behavior, so these files are not directly scenario-E2E relevant.

Optional scenario E2E

• None.

Relevant changed files

• None.

[github-actions]

PR Review Advisor

Findings: 1 needs attention, 2 worth checking, 0 nice ideas
Top item: Align the service PID directory source of truth

Review findings

🛠️ Needs attention

• Default service PID directory no longer matches existing readers and cleanup paths (src/lib/tunnel/services.ts:389): Changing the TypeScript tunnel service default from the literal /tmp path to os.tmpdir() creates two possible authorities for the cloudflared PID/log directory. The updated start/status/stop helpers can now use {os.tmpdir()}/nemoclaw-services-<sandbox>, while nearby code still reads or deletes /tmp/nemoclaw-services-<sandbox>. On systems where os.tmpdir() is not /tmp, or when TMPDIR differs between invocations, doctor can report the wrong cloudflared state and destroy/snapshot/uninstall cleanup can miss service state or leave processes/logs behind.
  • Recommendation: Either keep the production default at /tmp and use an explicit pidDir/test seam for test isolation, or migrate all service PID-dir producers/readers/cleanup paths to a single shared resolver. Include the legacy shell script and cleanup callers in that migration.
  • Evidence: This diff changes resolvePidDir() to join(tmpdir(), `nemoclaw-services-${sandbox}`) in src/lib/tunnel/services.ts:389. Existing repository paths still hard-code /tmp/nemoclaw-services-*: scripts/start-services.sh:45, src/lib/actions/sandbox/doctor.ts:277, src/lib/actions/sandbox/destroy.ts:248, src/lib/actions/sandbox/snapshot.ts:269, and uninstall path construction defaults to /tmp in src/lib/domain/uninstall/paths.ts:54-56.

🔎 Worth checking

• Source-of-truth review needed: Tunnel service PID directory resolution: 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: src/lib/tunnel/services.ts:389 now uses tmpdir(); scripts/start-services.sh:45, src/lib/actions/sandbox/doctor.ts:277, src/lib/actions/sandbox/destroy.ts:248, and src/lib/actions/sandbox/snapshot.ts:269 still use /tmp/nemoclaw-services-*.
• Runtime PID state now follows environment-controlled temp-root selection (src/lib/tunnel/services.ts:389): The new default follows os.tmpdir(), which is influenced by platform and temp-related environment variables on many systems. The existing PID-dir creation and PID-file writes are still permission-hardened, so this is not a confirmed vulnerability, but it changes the trust/configuration boundary for service state as part of a PR primarily described as test temp-path hardening.
  • Recommendation: Confirm this runtime behavior is intentional. If production PID state should remain stable, revert the production default and pass explicit private pidDir roots in tests. If os.tmpdir() is intended, document the behavior and ensure all callers use the same resolver.
  • Evidence: src/lib/tunnel/services.ts imports tmpdir from node:os and uses join(tmpdir(), `nemoclaw-services-${sandbox}`); tests set process.env.TMPDIR to redirect the default PID root.

🌱 Nice ideas

• None.

Consider writing more tests for

• **Runtime validation** — startAll, showStatus, and stopAll use the same default PID directory when TMPDIR is set to a non-/tmp root. The changed production surface controls host-side service PID/log state, which affects tunnel start/status/stop, doctor, cleanup, and sandbox lifecycle behavior. Unit fixture updates alone do not prove those runtime paths agree under non-default temp-root selection.
• **Runtime validation** — doctor cloudflared check reads the same PID directory that tunnel start/status writes when TMPDIR is overridden. The changed production surface controls host-side service PID/log state, which affects tunnel start/status/stop, doctor, cleanup, and sandbox lifecycle behavior. Unit fixture updates alone do not prove those runtime paths agree under non-default temp-root selection.
• **Runtime validation** — sandbox destroy cleanup removes service PID directories created by the tunnel service default resolver. The changed production surface controls host-side service PID/log state, which affects tunnel start/status/stop, doctor, cleanup, and sandbox lifecycle behavior. Unit fixture updates alone do not prove those runtime paths agree under non-default temp-root selection.
• **Runtime validation** — snapshot destination cleanup removes service PID directories created by the tunnel service default resolver. The changed production surface controls host-side service PID/log state, which affects tunnel start/status/stop, doctor, cleanup, and sandbox lifecycle behavior. Unit fixture updates alone do not prove those runtime paths agree under non-default temp-root selection.
• **Runtime validation** — legacy scripts/start-services.sh and the TypeScript tunnel service either agree on PIDDIR or the legacy script is explicitly retired from this behavior. The changed production surface controls host-side service PID/log state, which affects tunnel start/status/stop, doctor, cleanup, and sandbox lifecycle behavior. Unit fixture updates alone do not prove those runtime paths agree under non-default temp-root selection.
• **Acceptance clause:** Use private `mkdtempSync` roots for test artifacts that previously wrote predictable files under the OS temp directory. — add test evidence or identify existing coverage. Several touched fixtures now use mkdtempSync, including uninstall PID-file homes and service-env proxy/symlink fixtures. However, the PR achieves the tunnel-service test isolation by changing production resolvePidDir() to os.tmpdir(), which introduces runtime PID-dir drift instead of keeping the change purely in tests.
• **Acceptance clause:** Use `os.tmpdir()` for the tunnel service default PID root so tests can bind it to a private temp directory. — add test evidence or identify existing coverage. Implemented in src/lib/tunnel/services.ts:389 and exercised by src/lib/tunnel/services-sandbox.test.ts setting TMPDIR. The acceptance is only partially satisfied because existing service-state readers and cleanup paths still use literal /tmp, so the new default is not consistently integrated.
• **Acceptance clause:** [x] Tests added or updated for new or changed behavior — add test evidence or identify existing coverage. Tests were updated for fixture isolation, but there is no regression coverage proving that start/status/stop, doctor, destroy, snapshot, uninstall, and the legacy shell script agree on the PID directory when TMPDIR is not /tmp.

Workflow run details

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

[github-actions]

Selective E2E Results — ✅ All requested jobs passed

Run: 27159307321
Target ref: 60cbb867e02c3902d4f2a36f2934dfb0f64a8f66
Workflow ref: main
Requested jobs: tunnel-lifecycle-e2e,sandbox-operations-e2e
Summary: 2 passed, 0 failed, 0 skipped

Job	Result
sandbox-operations-e2e	✅ success
tunnel-lifecycle-e2e	✅ success

[cv]

Follow-up for the review advisor feedback is open in #4980. It restores the production tunnel PID directory default to the existing /tmp contract and keeps the test isolation via an explicit private pidDir.