fix(project create): don't stream the per-project skill install's clack UI

[tonychang04]

Found while creating a project with the released CLI: insta project create shells npx skills add for the stack skills (insta, neon-postgres, tigris, better-auth) with stdio: 'inherit' — dumping the same clone-spinner/banner noise we already removed from insta setup agent (#42).

Run it silent instead; the per-skill ✓/failed line is the clean output and still appears as each skill finishes (live progress). Also stops the child inheriting a piped stdin.

After:

  installing related agent skills (insta, neon-postgres, tigris, better-auth) …
  insta ✓
  neon-postgres ✓
  tigris ✓
  better-auth ✓

Verified with a built binary. 77/77 tests green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GMbAe5K1RfwaAcge5inX7P

---

Summary by cubic

Stop streaming the clack UI from per-project skill installs in insta project create; run npx skills add silently and show only per-skill ✓/failed lines for clean, live progress.
Also prevents the child process from inheriting piped stdin and removes the "via npx skills add" wording from the install message.

^{Written for commit 8b6b7cf. Summary will update on new commits.}

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[jwfing]

Review — fix(project create): don't stream the per-project skill install's clack UI

Summary: A tightly-scoped 1-file fix that silences the npx skills add clack UI during insta project create, mirroring the noise removal already done for insta setup agent (#42); the change is correct and behaves as claimed.

Requirements context

No matching spec/plan found — this repo has no docs/superpowers/ (nor docs/specs/) directory. Assessed against the PR description, the referenced precedent (#42, src/commands/setup.ts), and surrounding conventions in src/ensure-skills.ts.

Findings

Critical

(none)

Suggestion

• Functionality / UX — src/ensure-skills.ts:24-30, 61-66: the fix uses stdio: 'ignore', which discards the child's stdout and stderr. On a per-skill failure the user now sees only <skill> failed — add manually: npx skills add … with no error detail. Note the sibling fix this PR mirrors chose a different balance: src/commands/setup.ts:84 uses stdio: ['ignore', 'pipe', 'pipe'] specifically so it stays silent on success yet can surface the real error tail on failure (setup.ts:106-112). Consider matching that here — capture stderr and print a short tail when ok is false — so project create failures remain diagnosable inline rather than requiring a manual re-run. Non-blocking: the "add manually" line does give the user a path to reproduce the error.

Information

• Dead parameter — src/ensure-skills.ts:16, 24: after this change no caller passes the third arg, so the inherit?: boolean param on Runner / defaultRunner is now unexercised (confirmed repo-wide — no run('npx', s.args, true) remains). Fine to leave for flexibility, but it could be dropped to keep the runner honest, or kept intentionally if setup-style pluggability is wanted later.
• Test coverage — test/ensure-skills.test.ts: the behavioral change (streamed → silent) isn't guarded by an automated test, since the suite injects a fakeRun that never touches real stdio. This is an inherent limitation (spawn stdio is awkward to unit-test) and the author verified against a built binary; noting it only for completeness, not as a gap to fix.

Dimension coverage

• Software engineering: ✅ Matches existing conventions (best-effort contract, per-skill fallback, injected runner for tests). Existing tests still pass structurally (fake runner signature unaffected).
• Functionality: ✅ stdio: 'ignore' silences the clack UI; the per-skill ✓/failed line prints as each finishes, preserving live progress; stdin is no longer inherited by the child. Matches the PR's stated "After" output. See the one UX Suggestion above.
• Security: No security-relevant changes — no new user input, no secrets logged, AI_AGENT env preserved rather than clobbered, no auth surface touched.
• Performance: No performance impact — same number of sequential child processes; only the stdio wiring changed.

Verdict

approved (informational — a human still gives the explicit GitHub approval). No Critical findings; the two non-blocking notes are optional polish.

[jwfing]

LGTM - approved.

[jwfing]

Review — fix(project create): don't stream the per-project skill install's clack UI

Summary: A clean, tightly-scoped one-file change that stops streaming the npx skills add child so its clack banner/spinner noise no longer floods insta project create; the per-skill ✓/failed lines remain as the clean live progress. No blocking issues.

Requirements context

No docs/superpowers/ (or docs/specs/) directory exists in this repo — assessing against the PR description, the referenced prior art (#42), and the sibling implementation in src/commands/setup.ts. The change is a direct, faithful port of the #42 pattern into the per-project install path.

Findings

Critical

(none)

Suggestion

• Functionality / debuggability — src/ensure-skills.ts:24-30,65. This drops the child's output entirely (stdio: 'ignore'), whereas the sibling #42 runner in src/commands/setup.ts:81-91 uses ['ignore','pipe','pipe'] to capture stdout+stderr and, on failure, surfaces the real error tail (setup.ts:106-112). Here a failing skill add prints only <label> failed — add manually: npx … with zero diagnostic about why it failed (offline, repo moved, npx error). For a best-effort convenience path this is acceptable (low blast radius, and the manual-fallback command is printed), but consider capturing and surfacing the last few lines on failure to match setup.ts and keep the install debuggable.

Information

• Software engineering — src/ensure-skills.ts:16,24,27. The inherit parameter on Runner/defaultRunner is now dead: the only call site (line 65) no longer passes it, so the 'inherit' branch at line 27 is unreachable within this module. Harmless (keeps the interface symmetric with flyctl-build.ts), but could be dropped for cleanliness since streaming is no longer an option anyone selects.
• Software engineering — test coverage. The behavior change lives entirely in defaultRunner (spawn stdio), which the tests don't exercise (they inject a fake run), so the inherit→ignore switch has no direct automated coverage. This is inherent to stdio behavior and matches how #42/setup-agent is tested, so no new test is expected — noting it only for completeness. Existing tests in test/ensure-skills.test.ts remain valid (the fakeRun signature (cmd, args) is unaffected by the call-site change).
• Security: no security-relevant changes — no new user input, no secrets, AI_AGENT env is still preserved (ensure-skills.ts:26).
• Performance: no performance-relevant changes; removing the streamed child is neutral-to-slightly-positive.
• The switch to stdio: 'ignore' also makes the child's stdin 'ignore' (default inherit=false), correctly preventing the child from consuming a piped stdin — consistent with the documented rationale in setup.ts:77-80. Good.

Verdict

approved (informational; posted as a COMMENT — human approval is still a separate action). Zero Critical findings. The change does exactly what the description claims, is consistent with the established #42 pattern, and stays in scope (+6/-2, one file). The one Suggestion — surfacing failure output like setup.ts does — is a nice-to-have, not a blocker.