From 325aa9362b7157dbc5e6721ec3082f091d7ba3d8 Mon Sep 17 00:00:00 2001
From: "Carlos D. Escobar-Valbuena" <carlosdavidescobar@gmail.com>
Date: Sun, 28 Jun 2026 22:08:38 -0500
Subject: [PATCH] fix(skills): bstack-skills install lands every skill,
 status-visibly (BRO-1588)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The real `bstack-skills install` only installed the FIRST roster skill then
stopped (`Installed: 1`), and that skill landed where `status` couldn't see it.
Two compounding defects in cmd_install, caught by a clean-room end-to-end dogfood:

- stdin drain: the loop reads the roster via `done < <(parse_roster)`, so the
  loop body's stdin IS that pipe; the per-skill `npx skills add` subprocess
  drained the remaining rows and the loop EOF'd after skill #1. Fix: `</dev/null`
  on the npx call. Empirically 1/13 -> 13/13 required installed.
- global-install landing: add_args lacked `-g`, so installs went cwd-relative
  (./.agents/skills) while status checks ~/.{agents,claude}/skills. Fix: append
  `-g`. status then reconciles. Exits 0 even when exotic agents reject global.
- interactive read (latent, same root cause): read roster rows as the y/N answer;
  now reads </dev/tty, declining safely under set -e when there is no tty.

Regression test 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
(N, not 1) — proven red on pre-fix code, green on fixed. skills-install-uses-
skill-flag.test.sh gains a -g assertion. Full tests/*.test.sh suite: 24/24.

Pre-existing (predates the BRO-1584 --skill change). P20 cross-review: PASS 7/10.

VERSION 0.28.0 -> 0.28.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 CHANGELOG.md                                 | 22 ++++++
 VERSION                                      |  2 +-
 bin/bstack-skills                            | 24 +++++-
 tests/skills-install-no-stdin-drain.test.sh  | 79 ++++++++++++++++++++
 tests/skills-install-uses-skill-flag.test.sh | 10 +++
 5 files changed, 132 insertions(+), 5 deletions(-)
 create mode 100755 tests/skills-install-no-stdin-drain.test.sh

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d0e1a19..5a063cd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,27 @@
 # Changelog
 
+## 0.28.1 — 2026-06-28
+
+### fix: `bstack-skills install` lands every skill, in the dir `status` reads (BRO-1588)
+
+Caught by a clean-room end-to-end dogfood (P11) of the merged skills-monorepo setup: every guard test and the mocked install test passed, but the **real** `bstack-skills install` only installs the FIRST roster skill, then silently stops (`Installed: 1`). A second, compounding defect: even that one skill lands where `status` can't see it.
+
+### Fixed
+
+- **`bin/bstack-skills` `cmd_install` — stdin drain (the truncation).** The loop reads the roster via `done < <(parse_roster)`, so the loop body's stdin **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 #1. Fix: `… </dev/null >/dev/null 2>&1`. Empirically: exact pre-fix code installs 1/13 required; with `</dev/null`, 13/13. Pre-existing — predates the BRO-1584 `--skill` change; any stdin-reading subprocess in that loop drains it.
+- **`bin/bstack-skills` `cmd_install` — global-install landing (the mismatch).** Production `add_args` had no `-g`, so the skills CLI installed **cwd-relative** (`./.agents/skills`), while `status` only checks `~/.agents/skills` + `~/.claude/skills` → an install↔status mismatch. Fix: append `-g`. Verified `-g` lands in the global `~/.agents/skills` (plus the per-agent `~/.claude/skills` when that agent's home dir is present at install time), so `status` reports `installed`; and it exits 0 even when exotic agents (Eve/PromptScript) reject global install. (Caveat: if `~/.claude` does not yet exist at install time the skill lands only in `~/.agents/skills`; `bstack skills install` is normally run from inside a Claude Code session where `~/.claude` exists, so the per-agent entry is created.)
+- **`bin/bstack-skills` `cmd_install` — interactive prompt (latent, same root cause).** The `--interactive` `read -r -p … reply` read roster rows as the y/N answer; now reads from `</dev/tty`.
+
+### Added
+
+- **`tests/skills-install-no-stdin-drain.test.sh`** — regression guard: mocks `NPX_CMD` with a subprocess 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 not catch this — proven red (buggy) → green (fixed).
+- **`tests/skills-install-uses-skill-flag.test.sh`** — added a `-g` assertion (every `broomva/skills` install carries `-g`, so it lands status-visible).
+
+### Notes
+
+- Primitive count unchanged (**20**). Installer correctness fix, not a new P-row.
+- `VERSION` 0.28.0 → 0.28.1.
+
 ## 0.28.0 — 2026-06-11
 
 ### feat: Pattern H — non-code dogfood (knowledge-vault) + detection (BRO-1482)
diff --git a/VERSION b/VERSION
index 697f087..48f7a71 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-0.28.0
+0.28.1
diff --git a/bin/bstack-skills b/bin/bstack-skills
index 2b8c087..d271b15 100755
--- a/bin/bstack-skills
+++ b/bin/bstack-skills
@@ -241,7 +241,12 @@ cmd_install() {
             continue
         fi
         if [ "$interactive" = "1" ]; then
-            read -r -p "  Install $n ($r)? [y/N] " reply
+            # </dev/tty: the loop body's stdin is the parse_roster pipe (see the npx
+            # note below), so a bare `read` would consume roster rows as the answer.
+            # Read the prompt from the controlling terminal instead. If there is no
+            # tty (CI/cron/piped), the open fails — `|| reply=N` declines rather than
+            # aborting under `set -e`. (BRO-1588)
+            { read -r -p "  Install $n ($r)? [y/N] " reply </dev/tty; } || reply="N"
             if [ "${reply:-N}" != "y" ] && [ "${reply:-N}" != "Y" ]; then
                 echo "    [skip] declined"
                 skipped=$((skipped + 1))
@@ -249,16 +254,27 @@ cmd_install() {
             fi
         fi
         echo "  [install] $n ($r)..."
+        # -g installs globally to ~/.{agents,claude}/skills — exactly where `status`
+        # looks. Without it the skills CLI installs cwd-relative (./.agents/skills),
+        # so an install would never be seen by `status` (install↔status mismatch).
+        # The flag is exit-0 even when exotic agents (Eve/PromptScript) reject global
+        # install — the canonical .agents/.claude copies still land. (BRO-1588)
+        #
         # broomva/skills is the monorepo (many skills) — install the single skill
         # with --skill <name>, else a bare `add broomva/skills` pulls everything.
         # Standalone repos install whole.
         if [ "$r" = "broomva/skills" ]; then
-            add_args=("$r" --skill "$n")
+            add_args=("$r" --skill "$n" -g)
         else
-            add_args=("$r")
+            add_args=("$r" -g)
         fi
         # shellcheck disable=SC2086
-        if $NPX_CMD "${add_args[@]}" >/dev/null 2>&1; then
+        # </dev/null is load-bearing: this loop reads the roster via
+        # `done < <(parse_roster)`, so the loop body's stdin IS that pipe. Without
+        # the redirect the `npx skills add` subprocess drains the remaining roster
+        # rows off the pipe and the enclosing `read` hits EOF after skill #1 — only
+        # 1 of N skills installs, silently. (BRO-1588)
+        if $NPX_CMD "${add_args[@]}" </dev/null >/dev/null 2>&1; then
             installed=$((installed + 1))
             echo "    ✓ installed"
         else
diff --git a/tests/skills-install-no-stdin-drain.test.sh b/tests/skills-install-no-stdin-drain.test.sh
new file mode 100755
index 0000000..f2fde49
--- /dev/null
+++ b/tests/skills-install-no-stdin-drain.test.sh
@@ -0,0 +1,79 @@
+#!/usr/bin/env bash
+# skills-install-no-stdin-drain.test.sh — regression guard for BRO-1588.
+#
+# cmd_install iterates the roster with `while read ... done < <(parse_roster)`, so
+# the loop body's stdin (fd 0) IS the roster pipe. If the per-skill install
+# subprocess reads stdin — and the real `npx skills add` CLI does — it drains the
+# remaining roster rows off that pipe and the enclosing `read` hits EOF after skill
+# #1. Symptom: only 1 of N skills installs, silently ("Installed: 1").
+#
+# The existing skills-install-uses-skill-flag.test.sh mock does NOT read stdin, so
+# it cannot catch this. This test mocks NPX_CMD with a subprocess that DRAINS stdin
+# (exactly like the real CLI) and asserts every roster skill is attempted. Without
+# the `</dev/null` guard in cmd_install it fails (1 attempt); with it, all N.
+set -uo pipefail
+
+HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+BSTACK_SKILLS="$HERE/bin/bstack-skills"
+
+TMP="$(mktemp -d)"
+trap 'rm -rf "$TMP"' EXIT
+
+# Roster: N=4 skills, all broomva/skills, all required.
+ROSTER="$TMP/roster.yaml"
+cat > "$ROSTER" <<'YAML'
+skills:
+  - name: alpha
+    repo: broomva/skills
+    category: meta
+    required: true
+  - name: bravo
+    repo: broomva/skills
+    category: meta
+    required: true
+  - name: charlie
+    repo: broomva/skills
+    category: meta
+    required: true
+  - name: delta
+    repo: broomva/skills
+    category: meta
+    required: true
+YAML
+EXPECTED=4
+
+# Mock npx that DRAINS stdin (reproduces the real `skills add` CLI) and logs each
+# invocation's skill name. The `cat >/dev/null` is the whole point — on a stdin-
+# unguarded cmd_install it eats the roster pipe.
+LOG="$TMP/attempts.log"
+MOCK="$TMP/npx-mock.sh"
+cat > "$MOCK" <<MOCKEOF
+#!/usr/bin/env bash
+cat >/dev/null 2>&1   # drain stdin, exactly like the real CLI does
+echo "\$*" >> "$LOG"  # args: broomva/skills --skill <name>
+exit 0
+MOCKEOF
+chmod +x "$MOCK"
+
+# Empty HOME store so nothing reads as already-installed → all N attempted.
+STORE="$TMP/home"; mkdir -p "$STORE"
+
+HOME="$STORE" BSTACK_DIR="$HERE" BSTACK_SKILLS_YAML="$ROSTER" \
+  BSTACK_NPX_CMD="$MOCK" bash "$BSTACK_SKILLS" install >/dev/null 2>&1 || true
+
+ATTEMPTS=0
+[ -f "$LOG" ] && ATTEMPTS=$(wc -l < "$LOG" | tr -d ' ')
+
+echo "skills-install-no-stdin-drain: expected $EXPECTED install attempts, got $ATTEMPTS"
+if [ "$ATTEMPTS" = "$EXPECTED" ]; then
+  echo "  [pass] every roster skill attempted despite a stdin-draining install subprocess"
+  echo ""
+  echo "skills-install-no-stdin-drain: 1 passed, 0 failed"
+  exit 0
+else
+  echo "  [FAIL] only $ATTEMPTS/$EXPECTED skills attempted — cmd_install stdin drained by the install subprocess (BRO-1588 regression)"
+  [ -f "$LOG" ] && { echo "  attempted:"; sed 's/^/    /' "$LOG"; }
+  echo ""
+  echo "skills-install-no-stdin-drain: 0 passed, 1 failed"
+  exit 1
+fi
diff --git a/tests/skills-install-uses-skill-flag.test.sh b/tests/skills-install-uses-skill-flag.test.sh
index 63e03de..9051086 100755
--- a/tests/skills-install-uses-skill-flag.test.sh
+++ b/tests/skills-install-uses-skill-flag.test.sh
@@ -31,6 +31,16 @@ else
   fail "$BARE install(s) hit broomva/skills without --skill:"; grep -E '(^| )broomva/skills( |$)' "$LOG" | grep -v -- '--skill' | head
 fi
 
+# every install must be global (-g) so it lands in ~/.{agents,claude}/skills, where
+# `status` looks. Without -g the skills CLI installs cwd-relative (./.agents/skills),
+# invisible to status — an install↔status mismatch. (BRO-1588)
+NONGLOBAL="$(grep -E '(^| )broomva/skills( |$)' "$LOG" | grep -vcE '(^| )-g( |$)' || true)"
+if [ "${NONGLOBAL:-0}" -eq 0 ]; then
+  ok "every broomva/skills install carries -g (global install, status-visible)"
+else
+  fail "$NONGLOBAL install(s) hit broomva/skills without -g (would land cwd-relative, invisible to status):"; grep -E '(^| )broomva/skills( |$)' "$LOG" | grep -vE '(^| )-g( |$)' | head
+fi
+
 rm -f "$LOG" "$MOCK"
 echo ""
 echo "skills-install-uses-skill-flag: $PASS passed, $FAIL failed"