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

[TSavo]

Summary

Replaces the TEST_SYSTEM_PATH = "/usr/bin:/bin" literal in test/install-preflight.test.js with buildIsolatedSystemPath(), a small helper that creates a tmpdir at module load and symlinks every entry from /usr/bin and /bin into it — except node, npm, and npx, which are deliberately excluded so the runtime preflight's "node missing" / "npm missing" branches are actually exercised.

Related Issue

Closes #1621 (sub-item 2 of three: install-preflight tests leak host /usr/bin/node through TEST_SYSTEM_PATH).

Changes

• test/install-preflight.test.js:
  • Adds buildIsolatedSystemPath() helper at module top.
  • Changes const TEST_SYSTEM_PATH = "/usr/bin:/bin"; to const TEST_SYSTEM_PATH = buildIsolatedSystemPath();.
  • All ~19 existing call sites that use ${fakeBin}:${TEST_SYSTEM_PATH} continue to work unchanged because the fake bin still wins (it comes first in the PATH).
  • The 3 sites that use PATH: TEST_SYSTEM_PATH directly (the "node missing" / "npm missing" cases) now correctly see neither node nor npm.

Diff: +42 / −1 in 1 file.

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 vitest run --project cli test/install-preflight.test.js — all 56 tests pass locally on a Linux host with /usr/bin/node present (the previously-failing "node missing" / "npm missing" cases now correctly report missing)
• npx prettier --check test/install-preflight.test.js clean
• node --check test/install-preflight.test.js clean
• npx prek run --all-files / npm test — not run on the local machine because the full CLI test suite hits a separate baseline failure on a WSL2 host: the shouldPatchCoredns issue covered by fix(platform): allow shouldPatchCoredns isWsl override for deterministic WSL2 tests #1626. Upstream CI runs on Linux runners so it'll exercise the full suite normally.
• make docs builds without warnings. (for doc-only changes — N/A)

Checklist

General

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

Code Changes

• Formatters applied — npx prettier --check clean.
• Tests added or updated for new or changed behavior — the 3 previously-broken "missing binary" test cases now correctly assert the missing branch.
• No secrets, API keys, or credentials committed.
• Doc pages updated for any user-facing behavior changes — N/A, test infrastructure only.

Doc Changes

• N/A (no doc changes)

---

Signed-off-by: T Savo evilgenius@nefariousplan.com

Summary by CodeRabbit

• Tests
  • Improved test environment isolation to prevent unintended system dependencies during testing, ensuring more reliable and predictable test execution.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cdddb4ab-e0df-4124-99aa-f1a82f7f91a8

📥 Commits

Reviewing files that changed from the base of the PR and between 8b717d5 and 9c02f6f.

📒 Files selected for processing (1)

• test/install-preflight.test.js

---

📝 Walkthrough

Walkthrough

Replaced the fixed TEST_SYSTEM_PATH = "/usr/bin:/bin" in test/install-preflight.test.js with a runtime buildIsolatedSystemPath() helper that creates a temporary system-bin directory, symlinks host executables while explicitly excluding node, npm, and npx, and sets TEST_SYSTEM_PATH to that temp directory to prevent host PATH leakage.

Changes

Cohort / File(s)	Summary
Test environment isolation test/install-preflight.test.js	Added buildIsolatedSystemPath() which creates a temp dir and symlinks executables from /usr/bin and /bin while excluding node, npm, and npx. Replaced the static TEST_SYSTEM_PATH with the helper's returned directory so tests don't inadvertently see the host's node/npm unless explicitly provided.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Poem

🐰 In a tempdir burrow I softly tread,
I stitch small symlinks where tests are led,
Node and npm are left outside,
Clean paths hop in, no host to hide,
Hooray for tidy runs—carrots for bread! 🥕

🚥 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 the system PATH in install-preflight tests to prevent host binaries from leaking into test execution.
Linked Issues check	✅ Passed	The PR implements objective #2 from issue #1621 by creating an isolated temporary sysbin directory with symlinks, excluding node/npm/npx to fix the 'node missing' and 'npm missing' tests.
Out of Scope Changes check	✅ Passed	All changes are scoped to test/install-preflight.test.js and directly address the system PATH leakage issue identified in linked issue #1621 objective #2.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[Copilot]

Pull request overview

This PR hardens the install-preflight test environment by preventing the host machine’s /usr/bin/node (and related binaries) from leaking into the test PATH, ensuring the “node missing” / “npm missing” branches are exercised reliably across developer machines.

Changes:

• Replaces the hard-coded TEST_SYSTEM_PATH = "/usr/bin:/bin" with a module-load helper that builds a temporary isolated bin directory.
• Mirrors /usr/bin and /bin into the isolated directory via symlinks while excluding node, npm, and npx.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The catch {} around fs.symlinkSync currently swallows all errors, but the comment says it's only meant to ignore EEXIST when /bin mirrors /usr/bin. This can hide real failures (e.g., EPERM, EINVAL, ENOENT) and lead to an incomplete isolated bin dir with confusing downstream test failures. Please capture the error and rethrow unless err.code === 'EEXIST' (or explicitly handle any other expected codes).

Suggested change

	} catch {
	// Ignore EEXIST when /bin is a symlink to /usr/bin (modern Linux)
	// and we already linked the same name on the first pass.
	} catch (err) {
	// Ignore EEXIST when /bin is a symlink to /usr/bin (modern Linux)
	// and we already linked the same name on the first pass.
	if (err.code !== "EEXIST") {
	throw err;
	}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@test/install-preflight.test.js`:
- Around line 44-49: The generic catch around fs.symlinkSync(path.join(sysDir,
name), path.join(dir, name)) hides all failures; change it to catch the error
object (e) and rethrow unless e.code === 'EEXIST' so only duplicate-entry
collisions are ignored—i.e., replace the bare catch with catch (e) { if (e.code
!== 'EEXIST') throw e } while leaving the existing comment about /bin →
/usr/bin.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6f808728-f704-4484-b8eb-73439599936a

📥 Commits

Reviewing files that changed from the base of the PR and between b8a5245 and 5703f8a.

📒 Files selected for processing (1)

• test/install-preflight.test.js

[TSavo]

Force-pushed to add a Signed-off-by trailer (DCO compliance) — the diff itself is unchanged. CodeRabbit's prior review still applies to the same content.

Ready for a maintainer to approve workflow runs whenever you have a moment — first-time-contributor gate.

[TSavo]

@Copilot @coderabbitai both right — the bare catch {} was overbroad. Force-pushed 9c02f6f narrowing it to only swallow EEXIST:

} catch (err) {
  // Only swallow EEXIST — the expected case is when /bin is a symlink
  // to /usr/bin (modern Linux) and we already linked the same name on
  // the first pass. Any other error (EPERM, EACCES, EINVAL, ENOENT…)
  // would leave TEST_SYSTEM_PATH partially populated and turn into a
  // confusing downstream test failure, so re-throw it.
  if (err && err.code === "EEXIST") continue;
  throw err;
}

The err && err.code guard handles the (unlikely) case where Node throws a non-Error object — err.code access would otherwise crash on a bare null/undefined. Same shape as the proposed suggestion otherwise.

Diff is now +47 / −1 in one file. The 56-test suite still passes locally on a Linux host with /usr/bin/node present. Re-review welcome.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@TSavo — the updated catch block looks good. The narrowing from a bare catch {} to an EEXIST-only swallow is exactly the right fix, and the logic is sound:

• EEXIST swallow + continue — handles the expected /bin → /usr/bin symlink alias collision on modern Linux without masking anything unexpected.
• throw err fallthrough — surfaces EPERM, EACCES, EINVAL, ENOENT, etc. as hard failures, which prevents a silently half-populated TEST_SYSTEM_PATH from producing confusing downstream test failures. This is exactly the fail-closed posture that's appropriate here.
• err && err.code guard — defensively handles the (theoretical) non-Error throw. Functionally identical to err?.code === "EEXIST", just slightly more explicit — either form is fine.

The comment block is clear and explains the rationale well. No further changes needed on this side.

One small optional note: the temp dir is still not cleaned up after the test run (per the PR description). That's acceptable for CI since os.tmpdir() is typically ephemeral, but if you want to be tidy you could register an afterAll(() => fs.rmSync(TEST_SYSTEM_PATH, { recursive: true, force: true })). Not a blocker — just mentioning it for completeness.

LGTM on the buildIsolatedSystemPath change. 🟢

---

🧠 Learnings used

Learnt from: cluster2600
Repo: NVIDIA/NemoClaw PR: 782
File: test/config-io.test.js:54-75
Timestamp: 2026-03-24T11:21:32.603Z
Learning: In the NVIDIA/NemoClaw repo, the test suite (including `test/config-io.test.js`) runs on Ubuntu CI runners as a non-root user. Chmod-driven EACCES assertions (e.g., `fs.chmodSync(dir, 0o500)` to simulate permission errors) are intentional and reliable in this environment. Do not flag these as fragile or platform-unsafe unless there is evidence of root/Windows CI usage.

---

Learnt from: brianwtaylor
Repo: NVIDIA/NemoClaw PR: 251
File: bin/lib/preflight.js:114-129
Timestamp: 2026-03-17T23:56:54.445Z
Learning: In bin/lib/preflight.js (NemoClaw repo), checkPortAvailable() uses `typeof lsofOut === "string"` as a sentinel: a string value means lsof ran (possibly as non-root); undefined means lsof was not available on the host. Empty lsof string output is NOT treated as authoritative — non-root lsof silently drops root-owned sockets because it cannot read /proc/[pid]/fd (returns EACCES), which is the exact deployment scenario for NemoClaw (Docker proxy / openclaw-gateway runs as root, nemoclaw onboard runs as non-root). Empty lsof output must fall through to the net probe (bind() has no ownership restriction and checks the kernel socket hash table directly). The net probe is the fallback for both "lsof unavailable" and "lsof returned empty rows". Tests should cover: empty lsof + occupied port → net probe catches EADDRINUSE.

---

Learnt from: CR
Repo: NVIDIA/NemoClaw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T17:01:37.795Z
Learning: Applies to {nemoclaw/src/blueprint,bin/lib}/**/*.{js,ts} : Security-sensitive code paths in isolation/sandbox features must have extra test coverage to prevent credential leaks and sandbox escapes

---

Learnt from: gn00295120
Repo: NVIDIA/NemoClaw PR: 156
File: nemoclaw/src/commands/migration-state.ts:660-679
Timestamp: 2026-03-22T01:05:45.467Z
Learning: In `nemoclaw/src/commands/migration-state.ts`, the `walkAndRemoveFile` and `walkAndStripCredentials` functions intentionally warn-and-continue on all filesystem errors (including non-ENOENT) rather than throwing/aborting. This is a deliberate UX trade-off to avoid blocking the entire migration on transient permission errors in large workspace directories. The author is aware of the fail-open security risk and plans to tighten to fail-closed for non-ENOENT errors only if credential leaks are observed in practice.

---

Learnt from: CR
Repo: NVIDIA/NemoClaw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T17:01:37.795Z
Learning: Applies to test/**/*.{js,ts} : Use ESM syntax (`import`/`export`) for root-level integration tests in `test/` directory

---

Learnt from: jieunl24
Repo: NVIDIA/NemoClaw PR: 827
File: bin/lib/onboard.js:1118-1123
Timestamp: 2026-03-24T19:26:43.742Z
Learning: In the NemoClaw project (`bin/lib/onboard.js`), `openshell sandbox create` is synchronous — it does not return until the sandbox container is running. The race condition being fixed is specifically between step 3 (sandbox creation) completing and step 7 (policy application), not within step 3 itself. Readiness gates before `registry.registerSandbox()` or the sandbox connect call would be redundant.

[wscurran]

✨ Thanks for submitting this PR, which proposes a way to fix a test environment fragility issue by isolating the sysbin in install-preflight tests, preventing host PATH leakage and improving the reliability of the test suite.

---

Possibly related open PRs:

• #1626 fix(platform): allow shouldPatchCoredns isWsl override for deterministic WSL2 tests

---

Possibly related open issues:

• #1621 [NemoClaw][Tests] Three test-environment fragility issues (slow hosts, system PATH leakage, WSL2)

[cv]

I attempted to port this branch across the JS→TS migration and merge the latest main, but it still needs manual follow-up.

Please start with:

git fetch origin
git merge origin/main
npx tsx scripts/ts-migration-assist.ts --base origin/main --write
npm run build:cli
npm run typecheck:cli
npm run lint
npm test

[prekshivyas]

@TSavo can you also resolve conflicts pls !

[ericksoa]

Closing — this fix landed on main via #1706. Thank you @TSavo for the original contribution and the buildIsolatedSystemPath() approach.