Skill preflight: agent must detect whether the nodespace CLI is installed and the daemon is healthy

[malibio]

Closes #1229

[malibio]

Code Review Summary

This PR directly addresses all four acceptance criteria from #1229. The implementation is lean and well-targeted: a new isNodespaceBinaryOnPath() function in the installer, a --version-based PATH probe that is intentionally daemon-free, a clear preflight section in SKILL.md, and test coverage for the new behavior paths. The Rust CLI layer needed no changes — clap already handles --version before any socket connection, and all three connect*() helpers already emit actionable error messages. Overall this is a net improvement and the approach is sound.

One improvement-level gap and two nits are worth addressing before merge.

---

Findings

Suggested Improvements

[Improvement] isNodespaceBinaryOnPath() not exported from src/index.ts

installer.ts exports isNodespaceBinaryOnPath (line 24), but src/index.ts only re-exports install and uninstall. The package's public API surface omits the new function entirely. Any consumer wanting to probe PATH availability programmatically must reach into the internal module path (../installer.js) rather than the package root, which is fragile and inconsistent with the rest of the API design.

/packages/skill/src/index.ts line 1 — add isNodespaceBinaryOnPath to the export:

export { install, uninstall, isNodespaceBinaryOnPath } from './installer.js';

[Improvement] First test in isNodespaceBinaryOnPath describe block is not a meaningful assertion

packages/skill/src/tests/installer.test.ts lines 185–192: the test labeled "returns true when nodespace --version exits 0" only asserts typeof result === 'boolean'. This always passes regardless of whether the function returns true or false, making it vacuous as a positive-case contract test. The comment acknowledges this and punts the real positive case to "the false case." That leaves an uncovered branch: the happy path (binary found, returns true) is never deterministically tested.

Consider guarding on an env variable that CI sets when the binary IS available, or mock execFileSync to return successfully just as the other tests mock it to throw:

it('returns true when execFileSync exits 0', async () => {
  vi.resetModules();
  vi.doMock('node:child_process', () => ({
    execFileSync: () => Buffer.from('nodespace 0.1.0\n'),
  }));
  const { isNodespaceBinaryOnPath: check } = await import('../installer.js');
  expect(check()).toBe(true);
  vi.doUnmock('node:child_process');
  vi.resetModules();
});

This is the same pattern already used in the "install PATH warning / does not write a warning" test (lines 225–238), so no new technique is required.

---

Nitpicks

Nit: SKILL.md preflight error string is a prefix of the actual CLI output, not the full message

SKILL.md line 21 shows Could not connect to nodespaced as the failure symptom. The real CLI error (from packages/cli/src/lib.rs lines 105–106) is:

Could not connect to nodespaced at <path>.
Is the daemon running? Start it with `nodespaced` in another terminal.

The table cell is a valid substring match for agent pattern recognition, so this is not a correctness bug. But aligning the displayed symptom to the actual first line of the error (Could not connect to nodespaced at ~/.nodespace/daemon.sock.) would reduce any ambiguity for the agent reading the table.

Nit: execFileSync called without a timeout option

installer.ts line 26: execFileSync('nodespace', ['--version'], { stdio: 'ignore' }) has no explicit timeout. If for any reason the binary hangs (e.g., a broken wrapper script that blocks on stdin), install() will block the installer process indefinitely. A short timeout (e.g., { stdio: 'ignore', timeout: 3000 }) would make this robust. Low risk in practice since clap exits immediately, but worth hardening.

---

Requirements Validation

Acceptance Criterion	Status
SKILL.md documents preflight with failure→recovery for "not installed" and "daemon down"	Satisfied — Preflight Check section at line 5, recovery table at lines 17–23
nodespace --version succeeds with no daemon running	Satisfied — clap handles --version in Cli::parse() before run() attempts any socket connection; no Rust changes needed
Skill installer warns if nodespace binary is not on $PATH at install time	Satisfied — isNodespaceBinaryOnPath() + stderr warning in install()
All CLI subcommands return actionable "daemon not running" message	Satisfied — all three connect*()/connect_import()/connect_session() helpers emit the identical friendly error; uninstall is explicitly daemon-free (correct by design)

[malibio]

Two issues to address before merge: (1) isNodespaceBinaryOnPath is exported from installer.ts but missing from src/index.ts — it needs to be part of the public API. (2) The positive-case test for isNodespaceBinaryOnPath only asserts typeof result === 'boolean', which is vacuous — replace it with a deterministic mock-based test matching the pattern already used in the warning tests. Full details in the review comment above.

[malibio]

Review Addressed

✅ Addressed 3/4 recommendations — 1 skipped with rationale.

Changes made

Finding	Action
🟡 Vacuous happy-path test	Fixed — replaced typeof result === 'boolean' with a deterministic mock-based assertion (execFileSync returns 0 → check() returns true)
🟡 isNodespaceBinaryOnPath not in public index	Fixed — added to src/index.ts exports
🟢 Add timeout to execFileSync	Fixed — added timeout: 3000 so a hung wrapper binary can't block install()

Skipped

🟢 Align SKILL.md symptom to full CLI output — The prefix Could not connect to nodespaced is sufficient for agent pattern matching. The full string includes a machine-specific socket path that varies per user, so matching on the full first line would be fragile documentation.

Tests: 17/17 passed (skill), 3924/3924 passed (full suite)