fix(skills): bstack-skills install lands every skill, status-visibly (BRO-1588)

[broomva]

What & why

A clean-room end-to-end dogfood of the merged skills-monorepo setup (BRO-1570/1575/1584/1586) caught a real production defect: every guard test and the mocked install test pass, but the real bstack-skills install only installs the first roster skill then silently stops (Installed: 1), and even that one lands where status can't see it.

Closes BRO-1588.

Root cause — two compounding defects in cmd_install

1. stdin drain (the truncation). The loop reads the roster via done < <(parse_roster), so the loop body's stdin (fd 0) is that pipe. The per-skill $NPX_CMD … call had no stdin redirect, so the real npx skills add subprocess drained the remaining roster rows off the pipe and the enclosing read hit EOF after skill feat(bstack): add P9 (CI watcher + productive-wait) to Foundation layer #1.
   • Fix: … </dev/null >/dev/null 2>&1. Empirically 1/13 → 13/13 required installed.
   • Pre-existing — predates the BRO-1584 --skill change; any stdin-reading subprocess in that loop drains it. Hits the production default NPX_CMD="npx --yes skills add" equally.
2. global-install landing (the mismatch). add_args lacked -g, so the skills CLI installed cwd-relative (./.agents/skills), while status only checks ~/.agents/skills + ~/.claude/skills.
   • Fix: append -g. status then reconciles (13/61 installed). Exits 0 even when exotic agents (Eve/PromptScript) reject global install — the canonical .agents/.claude copies still land.
3. interactive prompt (latent, same root cause). --interactive's read -r -p … reply read roster rows as the y/N answer. Now reads </dev/tty, with || reply="N" so it declines safely under set -e when there's no tty (CI/cron/piped) instead of aborting.

Tests

• New tests/skills-install-no-stdin-drain.test.sh — mocks an NPX_CMD that drains stdin (like the real CLI) and asserts every roster skill is attempted (count == N, not 1). The existing skills-install-uses-skill-flag.test.sh mock does not read stdin, so it could never catch this. Proven red on pre-fix code (1/4) → green on fixed (4/4); deterministic over 5 runs.
• skills-install-uses-skill-flag.test.sh gains a -g assertion (every install is global / status-visible).
• Full tests/*.test.sh suite: 24/24. shellcheck clean (CI profile).

Validation evidence (P11)

• Pre-fix exact code: installs autonomous only (1/13), loop stops.
• With </dev/null: Installed: 13, skipped: 0, failed: 0; bstack-skills status → 13/61 installed.
• -g lands kg in ~/.agents/skills; status reports installed.
• No-tty --interactive: reply=N, exit 0 (declines, no abort).

P20 cross-model adversarial review — PASS (7/10), no must-fix

Acted on two should-considers from the review: hardened the --interactive no-tty path (|| reply="N") and scoped the CHANGELOG -g landing claim. Documented should-considers left for follow-up:

• is_installed reports a clean-HOME false-green (skill in ~/.agents/skills but no ~/.claude/skills symlink when ~/.claude doesn't pre-exist). Pre-existing is_installed weakness; in practice bstack skills install runs from inside a Claude Code session where ~/.claude exists. Candidate follow-up: tighten is_installed to assert the ~/.claude/skills/<name> entry.
• The process-substitution loop is reasoning-enforced against stdin drain (per-call redirects), not structurally; a mapfile pre-read would kill the bug class. Deferred (bash-3.2 empty-array-under-set -u portability); current fix is complete for today's code.

Notes

• Primitive count unchanged (20). Installer-correctness fix, not a new P-row.
• VERSION 0.28.0 → 0.28.1.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed interactive installs so they no longer consume the remaining skill list.
  • Ensured installed skills are placed where status checks can find them.
  • Corrected interactive prompts to read from the terminal instead of installer input.

• Tests

  • Added regression coverage for stdin handling during installs.
  • Added a check to verify global install behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

Version 0.28.1 fixes three bugs in cmd_install within bin/bstack-skills: the interactive prompt now reads from /dev/tty, npx skills add gains the -g global flag, and npx stdin is redirected from /dev/null to prevent roster pipe drain. Two regression tests are added and the changelog and VERSION are updated.

Changes

bstack-skills install fixes

Layer / File(s)	Summary
cmd_install: /dev/tty prompt, -g flag, stdin redirect bin/bstack-skills	Interactive confirmation prompt switched to read from /dev/tty with fallback; -g added to npx skills add in both broomva/skills and standalone-repo branches; npx stdin redirected from /dev/null to prevent draining the roster pipe.
Regression tests tests/skills-install-no-stdin-drain.test.sh, tests/skills-install-uses-skill-flag.test.sh	New test mocks npx to drain stdin and asserts all roster skills are attempted; existing test extended to assert every broomva/skills invocation includes -g.
CHANGELOG and VERSION CHANGELOG.md, VERSION	Changelog documents the three fixes and two new tests; VERSION bumped from 0.28.0 to 0.28.1.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 The stdin was leaking, the loop fell apart,
One skill got installed — what a rubbish start!
Now /dev/tty speaks and -g finds the home,
And /dev/null keeps npx from starting to roam.
Four skills, four attempts — the roster rings true! 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title matches the main change: fixing bstack-skills install so all skills install and status can see them.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/installer-stdin-drain

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

VERSION (1)

1-2: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Fix VERSION file to pass semver validation.

The pipeline fails because VERSION contains a trailing newline/blank line, so the regex ^[0-9]+\.[0-9]+\.[0-9]+$ does not match. Remove the extra blank line at the end so the file contains exactly 0.28.1 with no trailing whitespace or empty lines.

🔧 Proposed fix

-0.28.1
+0.28.1

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@VERSION` around lines 1 - 2, The VERSION file content is failing semver
validation because it includes a trailing newline/blank line after the version
string. Update VERSION so it contains only the exact version value already
present, with no extra whitespace or empty lines; ensure the file ends
immediately after the version text.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@VERSION`:
- Around line 1-2: The VERSION file content is failing semver validation because
it includes a trailing newline/blank line after the version string. Update
VERSION so it contains only the exact version value already present, with no
extra whitespace or empty lines; ensure the file ends immediately after the
version text.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 31ec5dd6-bdfe-4a00-9251-eef0955d3a9c

📥 Commits

Reviewing files that changed from the base of the PR and between 347a152 and 325aa93.

📒 Files selected for processing (5)

• CHANGELOG.md
• VERSION
• bin/bstack-skills
• tests/skills-install-no-stdin-drain.test.sh
• tests/skills-install-uses-skill-flag.test.sh

[broomva]

Re: CodeRabbit's VERSION trailing-newline comment — rejecting as a false positive.

The VERSION ↔ CHANGELOG match check passed (green, 6s). It reads new="$(cat VERSION | tr -d '[:space:]')" (.github/workflows/validate-release.yml:30), which strips the trailing newline before the ^[0-9]+\.[0-9]+\.[0-9]+$ regex — so a trailing \n cannot fail it. The byte layout is 0.28.1\n (single trailing newline), byte-identical to the prior 0.28.0\n and POSIX-conventional. No change needed.