fix(onboarding): stop skill-install eating the piped install script; drop false PATH warning + '(~20s)'

[tonychang04]

Comparing curl -fsSL agents.instacloud.com | sh to Railway's onboarding surfaced three papercuts:

1. The 'Get started' guidance never printed (the big one)

Under curl | sh, the install script is on sh's stdin. insta setup agent spawned npx skills with stdin: 'inherit', so the child consumed the rest of the piped script — the shell never reached the trailing Next steps block. Users saw the run just… stop after skill setup.

Fix: stdin: 'ignore' (with -y there's no prompt to read). Proven by building the binary from this branch and running the exact script-on-stdin condition:

• old binary (inherit) → Next steps eaten
• new binary (ignore) → Next steps prints ✓

2. False 'another insta is first on your PATH' warning

/usr/local/bin/insta is the symlink the installer itself creates, resolving straight back to ~/.insta/bin/insta. Flagging it as a conflict is confusing. Now silent when readlink matches our install target; still warns for a genuinely different (e.g. npm-installed) insta.

3. Dropped the misleading '(~20s)' label

Removed the hardcoded duration; enriched the next-steps block (incl. 'your coding agents now know InstaCloud — just ask them').

Tests: 75/75 green, tsc clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GMbAe5K1RfwaAcge5inX7P

---

Summary by cubic

Fixes curl | sh onboarding: the skill installer now ignores stdin so it no longer consumes the piped script, and the Next steps guidance prints reliably. Also removes a false PATH warning and refreshes the next-steps copy.

• Bug Fixes
  • Setup spawns the child with stdio ['ignore','pipe','pipe'] to stop it reading the piped install script under curl | sh; ensures the trailing guidance runs.
  • Suppress PATH conflict when /usr/local/bin/insta is our own symlink resolving to ~/.insta/bin/insta; still warn for a genuinely different binary (e.g., npm-installed).

^{Written for commit e52b43a. 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]

Code review — fix(onboarding): stop skill-install eating the piped install script …

Summary: Three well-scoped onboarding papercut fixes (stdin leak into npx skills, false PATH-conflict warning, dropped (~20s) label) — correct, minimal, and verified locally; no blocking issues.

Requirements context

No matching spec/plan found — this repo has no docs/superpowers/ or docs/specs/ directory (only README.md, CLAUDE.md, AGENTS.md, and .claude/skills/developing-insta-cli/SKILL.md). Assessed against the PR description, the developing-insta-cli skill conventions, and the surrounding code. Locally reproduced the claims: npm run typecheck is clean and npm test is 75/75 green.

Findings

Critical

(none)

Suggestion

• Software engineering — the "big one" fix has no regression test. src/commands/setup.ts:84 flips stdio[0] from 'inherit' → 'ignore', but this lives in defaultRunner, which the tests deliberately bypass via the injected Runner (test/setup-agent.test.ts:6). So nothing pins the stdin behavior — a future refactor could silently revert to 'inherit' and reintroduce the exact bug this PR fixes, with all tests still green. The repo's DI-first testing philosophy (per SKILL.md: "side-effectful modules take injected runners … follow that pattern") would support a lightweight test that inspects the spawn options (e.g. export/spy the stdio config) to lock stdin: 'ignore' in place. The manual curl … | sh proof in the PR body is good diligence, but it isn't executable in CI. Non-blocking given how genuinely awkward spawn-stdio is to unit-test.

Information

• Functionality — PATH-warning ordering is correct, worth noting explicitly. The new symlink-aware guard (install.sh:54-56) runs at check-time (line 48) before the symlink is created (line 142). This is the right behavior: on a first install no symlink exists yet (no false warning), and on a re-install/upgrade the prior /usr/local/bin/insta symlink is now correctly silenced. The string comparison is robust — both ln -sf (line 142) and readlink compare against the identical $INSTALL_DIR/$BIN literal, so it holds regardless of path canonicalization, and works on both GNU and BSD readlink (immediate-target read, no -f needed).
• Security — no concerns. No new user input reaches shell/SQL/HTTP; readlink "$first_hit" and all interpolations are quoted; no new dependencies; no secrets logged. stdin: 'ignore' is if anything a tightening (the piped script is no longer handed to the child).
• Performance — no concerns. Changes are install-time only (one extra readlink syscall) and one-shot child spawn; no hot paths, loops, or allocations affected.
• Nit — copy divergence. install.sh:178 now prints Next steps: while the setup flow elsewhere still uses Get started:-style phrasing in places; harmless, just flagging for consistency if you care.

Verdict

approved (informational — zero Critical findings; the GitHub green-check is a separate human action). Clean, tightly-scoped bugfix. The only thing I'd consider before merge is the missing regression test for the stdin fix, and even that is non-blocking.

[jwfing]

Code review — #44 fix(onboarding): stop skill-install eating the piped install script

Summary: Three well-scoped onboarding papercuts fixed — the stdin: 'ignore' change is a correct, minimal fix for the core "Next steps never printed" bug, and the diff stays tightly on-scope.

Requirements context

No matching spec/plan found — the repo has no docs/superpowers/ or docs/specs/ directory, so this is assessed against the PR description and the repo-local .claude/skills/developing-insta-cli conventions (CI = typecheck + vitest; DI-runner pattern for side-effectful modules, with commands/setup.ts cited as the canonical example).

I verified the PR's claim locally: 75/75 vitest tests pass on the head commit.

Findings

Critical

(none)

Suggestion

• Software engineering — no automated regression test for the core fix (src/commands/setup.ts:84). The fix flips stdio[0] from 'inherit' to 'ignore' inside defaultRunner. But every existing test (test/setup-agent.test.ts) injects its own mock Runner, so defaultRunner's spawn config is never exercised by the suite — a silent revert back to 'inherit' would reintroduce the "child eats the piped script" bug with green CI. The manual proof in the PR body (old vs. new binary under the real script-on-stdin condition) is solid, but it lives only in the PR description. Consider a lightweight guard, e.g. exporting defaultRunner (or the spawn-options builder) and asserting stdio[0] === 'ignore'. I recognize the repo's DI convention deliberately treats defaultRunner as the untested boundary and that the true end-to-end behavior needs a real piped-stdin spawn — hence Suggestion, not blocking.

Information

• Functionality — symlink guard is correct and re-run-scoped (install.sh:50-62). The PATH-conflict check runs before the symlink is created (install.sh:138-144), so the new "our own symlink → skip warning" branch only takes effect on re-runs where a prior install's symlink already exists — which is exactly the reported false-positive scenario, so this is right. Note also that readlink (without -f) returns the immediate target; it matches $INSTALL_DIR/$BIN here only because the symlink is always created with an absolute target (ln -sf "$INSTALL_DIR/$BIN" …). Robust as written; just be aware the equality compare is exact-string, not canonicalized.
• Software engineering — install.sh changes are untested. The repo has no shell-script test harness (all tests are vitest .test.ts), so this is consistent with existing convention, not a gap introduced by this PR. Noting for completeness.
• Docs — no cli-reference.md mirror needed. The repo-local skill requires command/flag changes to be mirrored into skills/insta/cli-reference.md. This PR changes only onboarding output text and process stdio wiring — no command or flag surface changes — so no mirror update is required. Verified.

Dimensions with no findings

• Security: no security-relevant regressions. stdio: 'ignore' is strictly safer than 'inherit' (the child can no longer read the parent's stdin); readlink output is only string-compared, never evaluated; no secrets/PII logged.
• Performance: no relevant changes.

Verdict

approved (informational — human approval is still a separate action). Zero Critical findings; the one Suggestion is a nice-to-have regression guard, not a merge blocker.

[jwfing]

LGTM - approved.

[jwfing]

LGTM - approved.