Add nodespace skill install/uninstall/status CLI subcommand

[malibio]

Summary

• Closes the CLI → skill bootstrap loop (Close the skill/binary bootstrap loop: let a CLI-only install set up agent skills #2368): a CLI-only install had no way to set up the NodeSpace skill for detected AI-agent harnesses, unlike the GUI app's first-launch installer (skill_setup.rs).
• Adds nodespace skill install|uninstall|status (packages/cli/src/commands/skill.rs). Mirrors skill_setup.rs's Installer enum (compiled sidecar preferred, dist/install.js + bun/node fallback), minus every Tauri-specific piece — this crate has no AppHandle.
• install prompts for confirmation on a real terminal ([Y/n], matching the issue's proposed UX), auto-confirms with a stated reason when there's no TTY, and accepts --yes for explicit non-interactive use.
• Detection reuses packages/skill/src/agents.ts's harness table via the shared installer — no second copy, per the issue's acceptance criteria.
• Extends release.yml's build-headless job to compile the nodespace-skill-installer sidecar and package its SKILL.md/shims/references resources as release assets for the non-Windows headless targets (macOS ARM, Linux ARM/x86), so a CLI-only install actually has something to invoke.
• Companion PR in nodespace-website (NodeSpaceAI/nodespace-website#4) adds --skill/--no-skill to install.sh, which downloads those new release assets and calls this subcommand after a CLI install completes.

Design decision (per the issue)

Implemented both options the issue laid out: the CLI subcommand (issue's recommended option 2 — most discoverable, handles the "harness installed later" case) and the release-asset upload (option 1), since the subcommand needs the sidecar published as a release asset to have anything to invoke on a CLI-only machine. agents.ts was left targeting today's harness table (gemini, not antigravity) — #2473's Gemini → Antigravity CLI swap is separate, unstarted work and out of scope here.

Acceptance criteria

• Decide between options 1 and 2 (or both) and record why — see above
• A CLI-only install can install the skill without the GUI app present
• Detection reuses packages/skill's agents.ts — no second copy of the harness table
• All four harnesses supported, with their per-harness shims, matching what the GUI path installs
• Non-interactive behaviour defined: no prompt without a TTY, documented default, chosen default stated in output
• A flag for non-interactive use (--yes), mirroring --gui/--no-gui
• Re-running is safe and idempotent, and picks up harnesses installed since last time
• Output reports which harnesses were installed into, and which were detected but skipped
• Skipping is graceful when no harness is detected — no error, no empty prompt
• Verified end to end on a machine with more than one harness present — see test plan

Test plan

• bun run test:all — no new failures (frontend baseline unchanged at 238/238 test files; every Rust crate 0 failed)
• bun run quality:fix — clean
• New unit tests in packages/cli/src/commands/skill.rs (parse_installer_output, bundled_sidecar_name, resolve_script_installer error path)
• cargo run -q -p nodespace-cli --example gen_skill_md -- --write regenerated references/cli.md; skill_md_generation.rs's coverage tests pass
• Manual end-to-end run against a real compiled sidecar + isolated fake $HOME: status (nothing detected) → install --yes (installs into .claude) → status (present) → uninstall (removed) → status (nothing detected) — confirmed real files land at ~/.claude/skills/nodespace/
• Manual multi-harness test: install with only .claude present, add .codex afterward, re-run install --yes — confirms new-harness pickup and idempotent re-run (already-installed harness reported again, no duplicate/error)
• sh -n syntax check + sourced-function tests on the companion install.sh change

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_018o8BPRcosQCrryoE5XYMYj

🤖 Generated with Claude Code

[malibio]

Code Review Summary

Net positive, well-engineered infrastructure/glue change. It closes the CLI→skill loop cleanly: a new nodespace skill install|uninstall|status subcommand (packages/cli/src/commands/skill.rs) mirrors skill_setup.rs's installer-resolution strategy (compiled sidecar, falling back to bun/node + dist/install.js), and release.yml is extended to actually publish that sidecar + its resources as release assets for the non-Windows headless targets, which is the prerequisite the subcommand needs to have anything to invoke on a CLI-only machine. I verified the cross-file contracts by reading skill_setup.rs, install.ts, installer.ts, and agents.ts directly (not just the diff): the --resource-root flag convention, the script-fallback default-resolution path, and the compiled-binary argv parsing all line up exactly with what skill.rs assumes. Clippy is clean, all 5 new unit tests pass, no lint suppression, no #[allow(dead_code)]/#[deprecated], single atomic commit, all subprocess calls use Command::new(...).arg(...) array form (no shell-string injection surface). The one substantive functional bug is in uninstall()'s output (silently drops skipped-agent reasons it already parsed), and the compiled-sidecar discovery path is the one piece of new logic with no test at all, unlike its skill_setup.rs analog which has a real end-to-end test.

Requirements Check (issue #2368)

• ✅ Decide between options 1/2 (or both) and record why — PR description explicitly chose both, with the reasoning line ("the subcommand needs the sidecar published as a release asset to have anything to invoke on a CLI-only machine") — matches the issue's own observation that option 2 "likely subsumes option 1."
• ✅ A CLI-only install can install the skill without the GUI app present — packages/cli/src/commands/skill.rs has zero dependency on desktop-app/Tauri; confirmed no AppHandle or Tauri imports anywhere in the file.
• ✅ Detection reuses packages/skill's agents.ts — no second copy — confirmed: skill.rs never touches agents.ts; detection lives entirely in the shelled-out installer (install.ts/installer.ts), invoked identically to how skill_setup.rs invokes it.
• ✅ All four harnesses supported, matching the GUI path — same installer binary/script is invoked either way; skill.rs does not filter or special-case agents.
• ✅ Non-interactive behavior defined, documented, stated in output — confirm_install() (skill.rs:83-98) checks stdin/stdout is_terminal(), auto-confirms with "No interactive terminal detected -- proceeding with install (pass --yes to silence this message)." printed to stdout. Matches the issue's proposed UX contract.
• ✅ A flag for non-interactive use — --yes on install (InstallArgs, skill.rs:54-61).
• ✅ Re-running is safe/idempotent, picks up new harnesses — delegated to the underlying installer (install.ts), which already has this property per the PR's manual-test note; the CLI wrapper adds no state of its own that could violate it.
• ⚠️ Output reports installed vs. skipped — true for install and status. Not true for uninstall — see Critical/Important finding below; it parses outcome.skipped but never prints it.
• ✅ Graceful no-harness-detected skip, no error/empty prompt — all three subcommands print an explicit "No … detected" message when both installed and skipped are empty.
• ✅ Verified end-to-end on a machine with >1 harness — described in the PR's manual test plan (real compiled sidecar, isolated fake $HOME, multi-harness pickup test). Not independently reproducible in this review, but the described method is sound and specific enough to be credible.

Code Review Findings

🔴 Critical

None. No security, correctness-of-core-logic, or architectural regressions found.

🟡 Important

• packages/cli/src/commands/skill.rs:111-122 — uninstall() silently drops skipped-agent output. The function checks outcome.skipped.is_empty() as part of its early "nothing found" branch, proving it knows skipped agents can be non-empty, but the main body only loops over outcome.installed — there is no for skipped in &outcome.skipped { println!(...) } the way status() (skill.rs:124-134) and report_install_outcome() (skill.rs:136-147) both have. If the underlying installer reports a skip during uninstall (e.g., "not installed, nothing to remove" for one harness while another is removed), that reason is parsed and then thrown away — the user sees only the agents that were removed, with no explanation for the rest. This breaks the acceptance criterion "Output reports which harnesses were installed into, and which were detected but skipped" for the uninstall path specifically. Fix: add the same skipped-loop that status() uses, e.g. for skipped in &outcome.skipped { println!(" {}: {}", skipped.agent, skipped.reason); }.

• packages/cli/src/commands/skill.rs — resolve_compiled_installer() (lines ~204-221) has no test coverage of its actual resolution logic. This function is the one piece of genuinely new logic in the PR (as opposed to logic mirrored from skill_setup.rs): it derives the sidecar path from current_exe()'s parent directory and checks for a sibling skill/SKILL.md. skill_setup.rs's equivalent has a real end-to-end test (run_skill_installer_compiled_variant_actually_installs_with_zero_runtime_on_path) that actually compiles a binary via bun build --compile and invokes it. skill.rs's test module only covers bundled_sidecar_name and parse_installer_output (both copied verbatim from the proven skill_setup.rs logic) — nothing exercises resolve_compiled_installer's directory-adjacency convention (sidecar beside nodespace, resources in a sibling skill/ dir) or resolve_installer()'s fallthrough from compiled → script. Given this is exactly the path a real CLI-only install exercises in production (the primary scenario the issue is about), and it's currently validated only by the PR author's manual end-to-end run rather than an automated test, an automated test here would catch a regression the next time release.yml's asset layout changes. Not blocking — the manual test plan is credible and specific — but worth a fast-follow.

🟢 Suggestions

• packages/cli/src/commands/skill.rs:66 — UninstallArgs is empty and uninstall() never prompts for confirmation, unlike install() which prompts by default and offers --yes/no-TTY auto-confirm. Uninstalling is also a destructive, if reversible (skill can be reinstalled), action — a bare nodespace skill uninstall removes skill files from every detected harness with zero confirmation and no flag to require one. This isn't a hard regression (issue's acceptance criteria don't ask for uninstall confirmation, and the GUI path's uninstall_skill is presumably gated by its own explicit button click, not a CLI TTY check), but for symmetry with install's careful non-interactive handling, consider at least a one-line summary before removal or a --yes flag mirroring install's, especially since a scripted nodespace skill uninstall run today has no way to be self-documenting about consent the way install's no-TTY message is.

• packages/cli/src/commands/skill.rs doc comment vs. run_installer_subcommand's parse_installer_output — the copy here drops the tracing::debug! logging of raw stdout/stderr that skill_setup.rs's original has before validating exit status. Not required (this is a CLI binary printing directly to stdout via println!, not a Tauri backend where a debug trace is the only way to see it), but if a user reports "the installer said X but the CLI printed something different," there's currently no --verbose/raw-output escape hatch to diagnose a parser mismatch. Nit-level; skip unless it comes up in practice.

• Nit: packages/cli/src/lib.rs doc comment on Command::Skill lists "Claude Code, Codex, Gemini CLI, OpenCode" — accurate today (confirmed agents.ts still says gemini), correctly out of scope per Replace Gemini CLI with Antigravity CLI as a supported PTY agent #2473, no action needed, just flagging that this comment (and the mirrored one in skill.rs's module doc and the regenerated references/cli.md) will need a follow-up edit alongside Replace Gemini CLI with Antigravity CLI as a supported PTY agent #2473's rename so it doesn't become the next stale-comment case CLAUDE.md warns about ("do not infer architecture from existing code comments — they may be stale").

• Nit: packages/cli/src/commands/skill.rs:279-291 (run_script_with_runtimes) — if bun exists on PATH but exits non-zero for a reason unrelated to "not found" (e.g. a script syntax error), the loop still breaks after the first attempt rather than falling through to try node. This matches skill_setup.rs's existing behavior (checked: same fallthrough-only-on-NotFound logic) and is the correct interpretation of "try bun, then node" as a runtime-availability fallback rather than an error-recovery retry, so this is not a bug — just noting the behavior is intentional in case a future reader wonders why a real bun failure doesn't retry with node.

release.yml review

No issues found. Verified directly:

• New steps are correctly gated with the existing steps.filter.outputs.skip != 'true' && matrix.os != 'windows-latest' pattern, consistent with the pre-existing "Build nodespace CLI" step's condition — Windows is correctly excluded (no nodespace CLI binary is built there per the existing Unix-socket-only-transport comment, so there's nothing for the sidecar to sit beside).
• bun install --frozen-lockfile runs at the workspace root — correct for a Bun workspace, packages/skill is a member.
• The tar command's file list (dist shims SKILL.md references package.json) exactly matches scripts/build-skill.ts's own STAGED_ENTRIES constant — same five entries, same relative layout, confirming the packaging step won't drift from the Tauri build's own convention.
• Asset upload naming (nodespace-skill-installer-${target}, nodespace-skill-installer-resources-${target}.tar.gz) is consistent with the existing nodespace-${target} / nodespaced-${target} convention on the same line, which is what the companion install.sh PR (different repo, out of scope here) needs to construct matching download URLs.
• Existing headless build steps (nodespaced, nodespace CLI, binary renaming) are untouched — new steps are purely additive, inserted after binary renaming and before upload.

CLAUDE.md compliance

• ✅ No backward-compatibility/migration code — this is new functionality, nothing being carried across a data-shape change.
• ✅ No lint suppression, no #[allow(dead_code)], no #[deprecated] — grepped the diff directly, none found.
• ✅ No raw console.log — no TS files touched in this diff (release.yml, Rust, and a generated markdown doc only).
• ✅ Bun-only tooling — release.yml uses bun install/bun run --cwd packages/skill build, no npm/yarn/pnpm.
• ✅ Change is atomic — single commit (bdb12f1b), all changes serve the one stated purpose (CLI subcommand + the release asset it depends on). The two pieces (CLI code, CI packaging) are tightly coupled rather than unrelated — the subcommand is inert without the release assets, so bundling them together is appropriate scope, not scope creep.
• ✅ references/cli.md was regenerated via the existing gen_skill_md --write tool rather than hand-edited, and is covered by skill_md_generation.rs's drift-detection test — confirmed this test exists and would fail if the doc fell out of sync with the actual clap surface.

CI / checks

gh pr checks 2511 reports no checks on this branch — consistent with this repo's project convention (no GitHub Actions CI/CD on PRs; the local pre-push hook, scripts/test-gate.ts, is the sole gate and already ran per the PR description's stated bun run test:all / quality:fix results). Independently ran cargo clippy -p nodespace-cli --all-targets (clean) and cargo test -p nodespace-cli --lib commands::skill (5/5 passed) from this review.

Recommendation

APPROVE, with the uninstall() skipped-output bug (🟡 Important) worth fixing before or shortly after merge — it's a small, contained fix and directly affects one of the issue's explicit acceptance criteria. The missing end-to-end test for resolve_compiled_installer() is a reasonable fast-follow rather than a blocker, given the credible manual verification already documented in the PR description. Everything else is net-positive, well-scoped infrastructure work that closes the loop the issue describes.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Addressed the review feedback (commit bee802d):

• Fixed (🟡): uninstall() now prints ⚠ agent: reason for skipped harnesses, matching install/status — this was silently dropping output the underlying installer already returns, breaking the "reports skipped harnesses" acceptance criterion for that one subcommand.
• Fixed (🟡): resolve_compiled_installer() had no test coverage. Split it into a pure compiled_installer_beside(exe: &Path) helper (same current_exe()-as-parameter seam daemon_setup::sidecar_path_from_exe uses) and added 3 tests covering missing-binary, missing-resource-root, and both-present.
• Skipped: a confirmation prompt for uninstall — the existing nodespace uninstall (full uninstall) command has none either; adding one here would be inconsistent with that established pattern, not more consistent with it.
• Skipped: restoring tracing::debug! logging — packages/cli has no tracing subscriber wired up anywhere else in the crate, so this would be new infrastructure rather than a parity fix.
• Skipped: gemini→antigravity rename — tracked separately in Replace Gemini CLI with Antigravity CLI as a supported PTY agent #2473, explicitly out of scope here.

bun run test:all and bun run quality:fix both clean; pre-push gate (full test:e2e suite) passed.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: Both changes are small, low-risk, and directly implement the reviewer's own suggested fix (print the already-collected skipped field; add tests for already-reviewed logic) — no new design surface, no architectural change, nothing uncertain about whether the fix addresses the finding. The three skipped items are justified above and none were disputed as must-fix by the review's severity levels (all 🟢 or explicit "reasonable fast-follow, not a blocker").