fix(test): isolate sysbin in install-preflight tests to prevent host $PATH leakage

[cv]

The "node missing" / "npm missing" runtime preflight tests need a PATH where the host's real node and npm are NOT visible, so the error branches are actually exercised. The previous TEST_SYSTEM_PATH = "/usr/bin:/bin" literal leaks /usr/bin/node on any Linux distribution that installs Node via apt install nodejs (i.e. most of them). On those hosts the affected tests assert the wrong code path — they expect "node missing" but the preflight finds the system /usr/bin/node and reports a version mismatch instead.

This replaces the literal with buildIsolatedSystemPath(), a small helper that creates a tmpdir under os.tmpdir() at module load and symlinks every entry from /usr/bin and /bin into it — except node, npm, and npx, which are deliberately excluded.

Rebased onto the renamed .ts file from the TS migration stack.

Closes #1621 (sub-item 2 of three).

Summary

Related Issue

Changes

Type of Change

• Code change for a new feature, bug fix, or refactor.
• Code change with doc updates.
• Doc only. Prose changes without code sample modifications.
• Doc only. Includes code sample changes.

Testing

• npx prek run --all-files passes (or equivalently make check).
• npm test passes.
• make docs builds without warnings. (for doc-only changes)

Checklist

General

• I have read and followed the contributing guide.
• I have read and followed the style guide. (for doc-only changes)

Code Changes

• Formatters applied — npx prek run --all-files auto-fixes formatting (or make format for targeted runs).
• Tests added or updated for new or changed behavior.
• No secrets, API keys, or credentials committed.
• Doc pages updated for any user-facing behavior changes (new commands, changed defaults, new features, bug fixes that contradict existing docs).

Doc Changes

• Follows the style guide. Try running the nemoclaw-contributor-update-docs agent skill to draft changes while complying with the style guide. For example, prompt your agent with "/nemoclaw-contributor-update-docs catch up the docs for the new changes I made in this PR."
• New pages include SPDX license header and frontmatter, if creating a new page.
• Cross-references and links verified.

Summary by CodeRabbit

• Tests
  • Improved test isolation for preflight/runtime checks by using an isolated system PATH to avoid interference from host-provided tools.
  • Made test setup more robust when preparing the isolated PATH by handling filesystem errors during population.

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaced the hardcoded test PATH in test/install-preflight.test.ts with a helper that builds a temporary isolated system-bin directory populated by symlinks to system executables while excluding node, npm, and npx; symlink EEXIST errors are ignored, others are re-thrown.

Changes

Cohort / File(s)	Summary
Test System Path Isolation test/install-preflight.test.ts	Added buildIsolatedSystemPath() to create a temp dir and symlink /usr/bin and /bin executables into it (excluding node, npm, npx), replaced fixed TEST_SYSTEM_PATH with the generated path, and handle symlink errors by ignoring EEXIST and re-throwing others.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I made a temp-bin, all neat and small,
Symlinks in rows by the test's call,
Node and npm, you stay away,
Tests now ask what they should say.
Hopping through PATHs — a quiet, tidy hall.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.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 change: isolating sysbin in install-preflight tests to prevent host $PATH leakage, which directly addresses issue #1621's second objective.
Linked Issues check	✅ Passed	The PR implements the second objective from #1621 by creating buildIsolatedSystemPath() to exclude node/npm/npx from TEST_SYSTEM_PATH, enabling deterministic "node missing" and "npm missing" test branches.
Out of Scope Changes check	✅ Passed	All changes are scoped to test/install-preflight.test.ts and directly address the host PATH leakage issue from #1621's second objective; no unrelated modifications were introduced.

_{✏️ 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/test-install-preflight-isolated-sysbin-v2

---

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

[brandonpelfrey]

🦞 NemoClaw Functional Review — PR #1706

Verdict: ✅ APPROVE

Functional Testing

Step	Result	Time
Clone & checkout	✅	2s
npm install --include=dev	✅	9s
TypeScript build (tsc -p tsconfig.src.json)	✅	3s
vitest run	✅ 1194 passed	19s
Dockerfile modified	ℹ️ No

Security

No concerns. Single test file change. No new dependencies, no permission changes, no credential handling.

The buildIsolatedSystemPath() function creates symlinks in os.tmpdir() — safe, no privilege escalation, and the EEXIST-only swallow pattern is correct (re-throws all other errors).

Architecture

Single file: test/install-preflight.test.ts. Clean, focused fix for #1621. The symlink isolation approach is more robust than hardcoded paths and handles the /bin → /usr/bin symlink case on modern Linux correctly.

---

🦞 Auto-reviewed by Nemo.

[coderabbitai]

🧹 Nitpick comments (1)

test/install-preflight.test.ts (1)

33-35: Optional: add one module-level cleanup for the temp sysbin directory.

Not blocking, but this can leave many temp dirs during local watch/repeated runs.

♻️ Proposed cleanup

-import { describe, it, expect } from "vitest";
+import { describe, it, expect, afterAll } from "vitest";
...
 const TEST_SYSTEM_PATH = buildIsolatedSystemPath();
+afterAll(() => {
+  fs.rmSync(TEST_SYSTEM_PATH, { recursive: true, force: true });
+});

Also applies to: 60-60

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/install-preflight.test.ts` around lines 33 - 35, Add a module-level
cleanup that removes the temporary sysbin directory created by the tests: within
test/install-preflight.test.ts add an afterAll hook that deletes the temp sysbin
directory (the variable holding it, e.g., sysbinDir or tempSysbinDir used when
creating the dir in the file) using fs.rmSync or fs.promises.rm with
recursive:true/force:true so repeated local runs/watch mode don't accumulate
temp dirs; ensure the hook references the same variable used to create the
directory and guard if it's undefined.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/install-preflight.test.ts`:
- Around line 33-35: Add a module-level cleanup that removes the temporary
sysbin directory created by the tests: within test/install-preflight.test.ts add
an afterAll hook that deletes the temp sysbin directory (the variable holding
it, e.g., sysbinDir or tempSysbinDir used when creating the dir in the file)
using fs.rmSync or fs.promises.rm with recursive:true/force:true so repeated
local runs/watch mode don't accumulate temp dirs; ensure the hook references the
same variable used to create the directory and guard if it's undefined.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0857c876-38b1-4213-9ffa-53e71a644e48

📥 Commits

Reviewing files that changed from the base of the PR and between 9376a66 and 821588b.

📒 Files selected for processing (1)

• test/install-preflight.test.ts

[ericksoa]

This was based on @TSavo's work in #1628. Thank you for the original fix.