fix(tunnel): keep service pid directory stable

[cv]

Summary

Restores the tunnel service PID directory default to /tmp/nemoclaw-services-<sandbox> so the TypeScript service, legacy scripts, doctor, destroy, snapshot, and uninstall cleanup paths keep a single source of truth. This addresses the PR review advisor feedback on #4976 while preserving the secure private temp-directory test fixture.

Changes

• Revert the production tunnel PID directory resolver from os.tmpdir() back to the existing /tmp/nemoclaw-services-<sandbox> contract.
• Keep the sandbox-channel test isolated by passing an explicit private pidDir created under mkdtempSync.

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

Summary by CodeRabbit

• Refactor

  • Updated default PID directory path handling for consistency.

• Tests

  • Enhanced integration tests to validate PID cleanup behavior with explicit directory parameters.

[coderabbitai]

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: 0b9584c0-cdee-4b25-8988-54cfe74b70f8

📥 Commits

Reviewing files that changed from the base of the PR and between 015a1b4 and c271b5f.

📒 Files selected for processing (2)

• src/lib/tunnel/services-sandbox.test.ts
• src/lib/tunnel/services.ts

---

📝 Walkthrough

Walkthrough

Host PID directory resolution is changed to use a hardcoded /tmp/nemoclaw-services-{sandbox} path instead of relying on os.tmpdir(). The implementation removes the tmpdir import, updates documentation, and changes resolvePidDir() to return the explicit default. The sandbox cleanup test is updated to exercise this by providing an explicit pidDir to stopAll and removing process.env.TMPDIR mutation.

Changes

PID Directory Default Hardcoding

Layer / File(s)	Summary
PID directory default hardcoding src/lib/tunnel/services.ts	resolvePidDir() returns opts.pidDir or defaults to /tmp/nemoclaw-services-{sandbox}; the os.tmpdir() import is removed and pidDir documentation is updated accordingly.
Sandbox cleanup test with explicit pidDir src/lib/tunnel/services-sandbox.test.ts	The stopAll with sandbox channels test now sets up explicit effective and lower-priority pid directories, calls stopAll({ pidDir: effectivePidDir }), and verifies cleanup behavior without mutating process.env.TMPDIR.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• NVIDIA/NemoClaw#4756: Updates status sandbox resolution so PID lookup uses the env-resolved sandbox name matching the hardcoded /tmp/nemoclaw-services-{sandbox} default.

Suggested labels

bug-fix, area: sandbox, area: security

Suggested reviewers

• ericksoa
• prekshivyas

Poem

🐰 A tunnel needs its pidding place,
No tmpdir wandering through the space,
Now hardcoded and clean it stays,
In /tmp/nemoclaw-services ways!
Tests verify the cleanup's right,
The services shine ever bright. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(tunnel): keep service pid directory stable' directly describes the main change: reverting the PID directory to use a stable path (/tmp/nemoclaw-services-) instead of os.tmpdir().
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 codex/secure-temp-files-feedback

---

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

[github-actions]

PR Review Advisor

Findings: 0 needs attention, 2 worth checking, 0 nice ideas
Top item: Add regression coverage for default /tmp PID directory resolution

Review findings

🛠️ Needs attention

• None.

🔎 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: services.ts now returns `/tmp/nemoclaw-services-${sandbox}`, but the modified regression test passes `pidDir: effectivePidDir`, so the source-of-truth restoration lacks direct test coverage.
• Default PID directory restoration is not directly tested (src/lib/tunnel/services-sandbox.test.ts:365): The production change switches the default PID directory from os.tmpdir() back to /tmp/nemoclaw-services-<sandbox>, but the updated regression test calls stopAll with an explicit pidDir. That means the test would still pass if resolvePidDir accidentally reverted to os.tmpdir(), leaving the main acceptance/security-sensitive behavior unproven.
  • Recommendation: Add a behavior-specific regression test that calls stopAll/startAll/showStatus without pidDir while TMPDIR points somewhere else, and asserts the service state is resolved under /tmp/nemoclaw-services-<effective sandbox> rather than $TMPDIR/nemoclaw-services-<sandbox>. If avoiding real /tmp writes, expose or isolate a resolver helper and test that directly.
  • Evidence: services.ts line 388 now returns `/tmp/nemoclaw-services-${sandbox}` by default, while the changed test around line 385 calls `stopAll({ pidDir: effectivePidDir })`, bypassing that default path.

🌱 Nice ideas

• None.

Consider writing more tests for

• **Runtime validation** — `stopAll` without `pidDir` cleans `/tmp/nemoclaw-services-<effective sandbox>` even when `TMPDIR` points elsewhere.. This PR changes host runtime/sandbox lifecycle state resolution. Unit coverage should prove the resolver contract, and runtime/integration validation is useful because stale PID state affects tunnel cleanup/status behavior.
• **Runtime validation** — `startAll` and `showStatus` default to `/tmp/nemoclaw-services-<sandbox>` with `TMPDIR` overridden.. This PR changes host runtime/sandbox lifecycle state resolution. Unit coverage should prove the resolver contract, and runtime/integration validation is useful because stale PID state affects tunnel cleanup/status behavior.
• **Runtime validation** — A misleading PID file under `$TMPDIR/nemoclaw-services-<sandbox>` is not treated as service state after restoring the `/tmp` default.. This PR changes host runtime/sandbox lifecycle state resolution. Unit coverage should prove the resolver contract, and runtime/integration validation is useful because stale PID state affects tunnel cleanup/status behavior.
• **Runtime validation** — Malformed and traversal sandbox names still fail before any default `/tmp/nemoclaw-services-*` path is created or used.. This PR changes host runtime/sandbox lifecycle state resolution. Unit coverage should prove the resolver contract, and runtime/integration validation is useful because stale PID state affects tunnel cleanup/status behavior.
• **Default PID directory restoration is not directly tested** — Add a behavior-specific regression test that calls stopAll/startAll/showStatus without pidDir while TMPDIR points somewhere else, and asserts the service state is resolved under /tmp/nemoclaw-services-<effective sandbox> rather than $TMPDIR/nemoclaw-services-<sandbox>. If avoiding real /tmp writes, expose or isolate a resolver helper and test that directly.
• **Acceptance clause:** Tests added or updated for new or changed behavior — add test evidence or identify existing coverage. A test was updated, but it passes an explicit pidDir and therefore does not verify the changed default PID directory behavior.
• **Tunnel service PID directory resolution** — Needs direct proof: a test should call the default resolver path with TMPDIR overridden and verify /tmp/nemoclaw-services-<sandbox> is used, not the TMPDIR root.. services.ts now returns `/tmp/nemoclaw-services-${sandbox}`, but the modified regression test passes `pidDir: effectivePidDir`, so the source-of-truth restoration lacks direct test coverage.

Workflow run details

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

[github-actions]

E2E Advisor Recommendation

Required E2E: tunnel-lifecycle-e2e
Optional E2E: sandbox-operations-e2e

Dispatch hint: tunnel-lifecycle-e2e

Auto-dispatched E2E: tunnel-lifecycle-e2e via nightly-e2e.yaml at c271b5fbf4ac117597252f6115aafea387088bcd — nightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

• tunnel-lifecycle-e2e (high): Exercises the affected real user flow: install/onboard, nemoclaw tunnel start, status URL discovery from /tmp/nemoclaw-services-<sandbox>, and nemoclaw tunnel stop cleanup. This is the most direct E2E coverage for the service PID directory change and stopAll behavior.

Optional E2E

• sandbox-operations-e2e (high): Useful adjacent confidence for sandbox lifecycle/status/destroy and multi-sandbox behavior because the changed service cleanup code is tied to sandbox names and /tmp/nemoclaw-services-<sandbox> cleanup, but it is broader and less targeted than tunnel-lifecycle-e2e.

New E2E recommendations

• multi-sandbox tunnel cleanup (medium): Existing live E2E covers a single sandbox tunnel lifecycle, while this PR’s unit change specifically exercises env-selected sandbox precedence and avoiding cleanup of a lower-priority sandbox PID directory. A focused E2E would catch regressions where NEMOCLAW_SANDBOX_NAME, NEMOCLAW_SANDBOX, registry default, and host PID cleanup diverge in a real install.
  • Suggested test: Add a focused tunnel stop E2E that creates two sandbox-named service PID directories, sets conflicting sandbox env vars, runs the real CLI stop path, and verifies only the effective sandbox’s /tmp/nemoclaw-services-<name> PID state is cleaned while the other remains untouched.

Dispatch hint

• Workflow: nightly-e2e.yaml
• jobs input: tunnel-lifecycle-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. The changes are outside test/e2e-scenario and the scenario workflows/metadata. The modified tunnel service code is not exercised by the scenario suite catalog or validation scripts, and the accompanying test file is non-scenario unit test coverage, so no scenario E2E job is indicated.

Optional scenario E2E

• None.

Relevant changed files

• None.

[github-actions]

Selective E2E Results — ✅ All requested jobs passed

Run: 27160506644
Target ref: c271b5fbf4ac117597252f6115aafea387088bcd
Workflow ref: main
Requested jobs: tunnel-lifecycle-e2e
Summary: 1 passed, 0 failed, 0 skipped

Job	Result
tunnel-lifecycle-e2e	✅ success