From 5319b8a13bce04a60328323397d51360e0538b7b Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 27 Mar 2026 00:44:37 -0600 Subject: [PATCH] =?UTF-8?q?feat:=20community=20PRs=20=E2=80=94=20faster=20?= =?UTF-8?q?install,=20skill=20namespacing,=20uninstall,=20Codex=20fallback?= =?UTF-8?q?,=20Windows=20fix,=20Python=20patterns=20(v0.12.9.0)=20(#561)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: sync package.json version with VERSION file (0.12.7.0) Co-Authored-By: Claude Opus 4.6 (1M context) * perf: shallow clone for faster install (#484) Co-Authored-By: Claude Opus 4.6 (1M context) * feat: Python/async/SSRF patterns in review checklist (#531) Co-Authored-By: Claude Opus 4.6 (1M context) * feat: namespace skill symlinks with gstack- prefix (#503) Co-Authored-By: Claude Opus 4.6 (1M context) * feat: add uninstall script (#323) Co-Authored-By: Claude Opus 4.6 (1M context) * feat: office-hours Claude subagent fallback when Codex unavailable (#464) Updates generateCodexSecondOpinion resolver to always offer second opinion and fall back to Claude subagent when Codex is unavailable or errors. Co-Authored-By: Claude Opus 4.6 (1M context) * fix: findPort() race condition via net.createServer (#490) Replaces Bun.serve() port probing with net.createServer() for proper async bind/close semantics. Fixes Windows EADDRINUSE race condition. Co-Authored-By: Claude Opus 4.6 (1M context) * test: add tests for uninstall, setup prefix, and resolver fallback - Uninstall integration tests: syntax, flags, mock install layout, upgrade path - Setup prefix tests: gstack-* prefixing, --no-prefix, cleanup migration - Resolver tests: Claude subagent fallback in generated SKILL.md Co-Authored-By: Claude Opus 4.6 (1M context) * chore: bump version and changelog (v0.12.9.0) Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- CHANGELOG.md | 20 +++ README.md | 13 +- VERSION | 2 +- bin/gstack-uninstall | 228 +++++++++++++++++++++++++++++++++++ browse/src/server.ts | 28 +++-- browse/test/findport.test.ts | 191 +++++++++++++++++++++++++++++ office-hours/SKILL.md | 51 +++++--- office-hours/SKILL.md.tmpl | 6 +- package.json | 2 +- review/checklist.md | 17 +++ scripts/resolvers/review.ts | 45 ++++--- setup | 53 +++++++- test/gen-skill-docs.test.ts | 54 ++++++++- test/uninstall.test.ts | 165 +++++++++++++++++++++++++ 14 files changed, 821 insertions(+), 54 deletions(-) create mode 100755 bin/gstack-uninstall create mode 100644 browse/test/findport.test.ts create mode 100644 test/uninstall.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a04e1473c9c7d9eedc202a3476e5f6bcd9c8ef5f..d9dc1789a6c9f8fec2ca7b6bc3ff2cc55de85e62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ # Changelog +## [0.12.9.0] - 2026-03-27 — Community PRs: Faster Install, Skill Namespacing, Uninstall + +Six community PRs landed in one batch. Install is faster, skills no longer collide with other tools, and you can cleanly uninstall gstack when needed. + +### Added + +- **Uninstall script.** `bin/gstack-uninstall` cleanly removes gstack from your system: stops browse daemons, removes all skill installs (Claude/Codex/Kiro), cleans up state. Supports `--force` (skip confirmation) and `--keep-state` (preserve config). (#323) +- **Python security patterns in /review.** Shell injection (`subprocess.run(shell=True)`), SSRF via LLM-generated URLs, stored prompt injection, async/sync mixing, and column name safety checks now fire automatically on Python projects. (#531) +- **Office-hours works without Codex.** The "second opinion" step now falls back to a Claude subagent when Codex CLI is unavailable, so every user gets the cross-model perspective. (#464) + +### Changed + +- **Faster install (~30s).** All clone commands now use `--single-branch --depth 1`. Full history available for contributors. (#484) +- **Skills namespaced with `gstack-` prefix.** Skill symlinks are now `gstack-review`, `gstack-ship`, etc. instead of bare `review`, `ship`. Prevents collisions with other skill packs. Old symlinks are auto-cleaned on upgrade. Use `--no-prefix` to opt out. (#503) + +### Fixed + +- **Windows port race condition.** `findPort()` now uses `net.createServer()` instead of `Bun.serve()` for port probing, fixing an EADDRINUSE race on Windows where the polyfill's `stop()` is fire-and-forget. (#490) +- **package.json version sync.** VERSION file and package.json now agree (was stuck at 0.12.5.0). + ## [0.12.8.1] - 2026-03-27 — zsh Glob Compatibility Skill scripts now work correctly in zsh. Previously, bash code blocks in skill templates used raw glob patterns like `.github/workflows/*.yaml` and `ls ~/.gstack/projects/$SLUG/*-design-*.md` that would throw "no matches found" errors in zsh when no files matched. Fixed 38 instances across 13 templates and 2 resolvers using two approaches: `find`-based alternatives for complex patterns, and `setopt +o nomatch` guards for simple `ls` commands. diff --git a/README.md b/README.md index aad62290f8f40dd535542217d4974ef7eeb65ad0..7ad49c2018b14b7b02f472203aa0d77fa0cdcb1b 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,7 @@ Fork it. Improve it. Make it yours. And if you want to hate on free open source Open Claude Code and paste this. Claude does the rest. -> Install gstack: run **`git clone https://github.com/garrytan/gstack.git ~/.claude/skills/gstack && cd ~/.claude/skills/gstack && ./setup`** then add a "gstack" section to CLAUDE.md that says to use the /browse skill from gstack for all web browsing, never use mcp\_\_claude-in-chrome\_\_\* tools, and lists the available skills: /office-hours, /plan-ceo-review, /plan-eng-review, /plan-design-review, /design-consultation, /review, /ship, /land-and-deploy, /canary, /benchmark, /browse, /qa, /qa-only, /design-review, /setup-browser-cookies, /setup-deploy, /retro, /investigate, /document-release, /codex, /cso, /autoplan, /careful, /freeze, /guard, /unfreeze, /gstack-upgrade. Then ask the user if they also want to add gstack to the current project so teammates get it. +> Install gstack: run **`git clone --single-branch --depth 1 https://github.com/garrytan/gstack.git ~/.claude/skills/gstack && cd ~/.claude/skills/gstack && ./setup`** then add a "gstack" section to CLAUDE.md that says to use the /browse skill from gstack for all web browsing, never use mcp\_\_claude-in-chrome\_\_\* tools, and lists the available skills: /office-hours, /plan-ceo-review, /plan-eng-review, /plan-design-review, /design-consultation, /review, /ship, /land-and-deploy, /canary, /benchmark, /browse, /qa, /qa-only, /design-review, /setup-browser-cookies, /setup-deploy, /retro, /investigate, /document-release, /codex, /cso, /autoplan, /careful, /freeze, /guard, /unfreeze, /gstack-upgrade. Then ask the user if they also want to add gstack to the current project so teammates get it. ### Step 2: Add to your repo so teammates get it (optional) @@ -54,6 +54,11 @@ Open Claude Code and paste this. Claude does the rest. Real files get committed to your repo (not a submodule), so `git clone` just works. Everything lives inside `.claude/`. Nothing touches your PATH or runs in the background. +> **Contributing or need full history?** The commands above use `--depth 1` for a fast install. If you plan to contribute or need full git history, do a full clone instead: +> ```bash +> git clone https://github.com/garrytan/gstack.git ~/.claude/skills/gstack +> ``` + ### Codex, Gemini CLI, or Cursor gstack works on any agent that supports the [SKILL.md standard](https://github.com/anthropics/claude-code). Skills live in `.agents/skills/` and are discovered automatically. @@ -61,7 +66,7 @@ gstack works on any agent that supports the [SKILL.md standard](https://github.c Install to one repo: ```bash -git clone https://github.com/garrytan/gstack.git .agents/skills/gstack +git clone --single-branch --depth 1 https://github.com/garrytan/gstack.git .agents/skills/gstack cd .agents/skills/gstack && ./setup --host codex ``` @@ -70,7 +75,7 @@ When setup runs from `.agents/skills/gstack`, it installs the generated Codex sk Install once for your user account: ```bash -git clone https://github.com/garrytan/gstack.git ~/gstack +git clone --single-branch --depth 1 https://github.com/garrytan/gstack.git ~/gstack cd ~/gstack && ./setup --host codex ``` @@ -81,7 +86,7 @@ discovery from the source repo checkout. Or let setup auto-detect which agents you have installed: ```bash -git clone https://github.com/garrytan/gstack.git ~/gstack +git clone --single-branch --depth 1 https://github.com/garrytan/gstack.git ~/gstack cd ~/gstack && ./setup --host auto ``` diff --git a/VERSION b/VERSION index a3866b38c8936df966fdf02fcf2f3e2d1761ee0c..84a2b44ab4bf8389d305f28f03b33bb8f53c2001 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.12.8.1 +0.12.9.0 diff --git a/bin/gstack-uninstall b/bin/gstack-uninstall new file mode 100755 index 0000000000000000000000000000000000000000..6bad7c1bfa38bd8c24a623caf206a26470af3565 --- /dev/null +++ b/bin/gstack-uninstall @@ -0,0 +1,228 @@ +#!/usr/bin/env bash +# gstack-uninstall — remove gstack skills, state, and browse daemons +# +# Usage: +# gstack-uninstall — interactive uninstall (prompts before removing) +# gstack-uninstall --force — remove everything without prompting +# gstack-uninstall --keep-state — remove skills but keep ~/.gstack/ data +# +# What gets REMOVED: +# ~/.claude/skills/gstack — global Claude skill install (git clone or vendored) +# ~/.claude/skills/{skill} — per-skill symlinks created by setup +# ~/.codex/skills/gstack* — Codex skill install + per-skill symlinks +# ~/.kiro/skills/gstack* — Kiro skill install + per-skill symlinks +# ~/.gstack/ — global state (config, analytics, sessions, projects, +# repos, installation-id, browse error logs) +# .claude/skills/gstack* — project-local skill install (--local installs) +# .gstack/ — per-project browse state (in current git repo) +# .gstack-worktrees/ — per-project test worktrees (in current git repo) +# .agents/skills/gstack* — Codex/Gemini/Cursor sidecar (in current git repo) +# Running browse daemons — stopped via SIGTERM before cleanup +# +# What is NOT REMOVED: +# ~/Library/Caches/ms-playwright/ — Playwright Chromium (shared, may be used by other tools) +# ~/.gstack-dev/ — developer eval artifacts (only present in gstack contributors) +# +# Env overrides (for testing): +# GSTACK_DIR — override auto-detected gstack root +# GSTACK_STATE_DIR — override ~/.gstack state directory +# +# NOTE: Uses set -uo pipefail (no -e) — uninstall must never abort partway. +set -uo pipefail + +if [ -z "${HOME:-}" ]; then + echo "ERROR: \$HOME is not set" >&2 + exit 1 +fi + +GSTACK_DIR="${GSTACK_DIR:-$(cd "$(dirname "$0")/.." && pwd)}" +STATE_DIR="${GSTACK_STATE_DIR:-$HOME/.gstack}" +_GIT_ROOT="$(git rev-parse --show-toplevel 2>/dev/null || true)" + +# ─── Parse flags ───────────────────────────────────────────── +FORCE=0 +KEEP_STATE=0 +while [ $# -gt 0 ]; do + case "$1" in + --force) FORCE=1; shift ;; + --keep-state) KEEP_STATE=1; shift ;; + -h|--help) + sed -n '2,/^[^#]/{ /^#/s/^# \{0,1\}//p; }' "$0" + exit 0 + ;; + *) + echo "Unknown option: $1" >&2 + echo "Usage: gstack-uninstall [--force] [--keep-state]" >&2 + exit 1 + ;; + esac +done + +# ─── Confirmation ──────────────────────────────────────────── +if [ "$FORCE" -eq 0 ]; then + echo "This will remove gstack from your system:" + { [ -d "$HOME/.claude/skills/gstack" ] || [ -L "$HOME/.claude/skills/gstack" ]; } && echo " ~/.claude/skills/gstack (+ per-skill symlinks)" + [ -d "$HOME/.codex/skills" ] && echo " ~/.codex/skills/gstack*" + [ -d "$HOME/.kiro/skills" ] && echo " ~/.kiro/skills/gstack*" + [ "$KEEP_STATE" -eq 0 ] && [ -d "$STATE_DIR" ] && echo " $STATE_DIR" + + if [ -n "$_GIT_ROOT" ]; then + [ -d "$_GIT_ROOT/.claude/skills/gstack" ] && echo " $_GIT_ROOT/.claude/skills/gstack (project-local)" + [ -d "$_GIT_ROOT/.gstack" ] && echo " $_GIT_ROOT/.gstack/ (browse state + reports)" + [ -d "$_GIT_ROOT/.gstack-worktrees" ] && echo " $_GIT_ROOT/.gstack-worktrees/" + [ -d "$_GIT_ROOT/.agents/skills" ] && echo " $_GIT_ROOT/.agents/skills/gstack*" + fi + + # Preview running daemons + if [ -n "$_GIT_ROOT" ] && [ -f "$_GIT_ROOT/.gstack/browse.json" ]; then + _PREVIEW_PID="$(awk -F'[:,]' '/"pid"/ { for(i=1;i<=NF;i++) if($i ~ /"pid"/) { gsub(/[^0-9]/, "", $(i+1)); print $(i+1); exit } }' "$_GIT_ROOT/.gstack/browse.json" 2>/dev/null || true)" + [ -n "$_PREVIEW_PID" ] && kill -0 "$_PREVIEW_PID" 2>/dev/null && echo " browse daemon (PID $_PREVIEW_PID) will be stopped" + fi + + printf "\nContinue? [y/N] " + read -r REPLY + case "$REPLY" in + y|Y|yes|YES) ;; + *) echo "Aborted."; exit 0 ;; + esac +fi + +REMOVED=() + +# ─── Stop running browse daemons ───────────────────────────── +# Browse servers write PID to {project}/.gstack/browse.json. +# Stop any we can find before removing state directories. +stop_browse_daemon() { + local state_file="$1" + if [ ! -f "$state_file" ]; then + return + fi + local pid + pid="$(awk -F'[:,]' '/"pid"/ { for(i=1;i<=NF;i++) if($i ~ /"pid"/) { gsub(/[^0-9]/, "", $(i+1)); print $(i+1); exit } }' "$state_file" 2>/dev/null || true)" + if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then + kill "$pid" 2>/dev/null || true + # Wait up to 2s for graceful shutdown + local waited=0 + while [ "$waited" -lt 4 ] && kill -0 "$pid" 2>/dev/null; do + sleep 0.5 + waited=$(( waited + 1 )) + done + if kill -0 "$pid" 2>/dev/null; then + kill -9 "$pid" 2>/dev/null || true + fi + REMOVED+=("browse daemon (PID $pid)") + fi +} + +# Stop daemon in current project +if [ -n "$_GIT_ROOT" ] && [ -f "$_GIT_ROOT/.gstack/browse.json" ]; then + stop_browse_daemon "$_GIT_ROOT/.gstack/browse.json" +fi + +# Stop daemons tracked in global projects directory +if [ -d "$STATE_DIR/projects" ]; then + while IFS= read -r _BJ; do + stop_browse_daemon "$_BJ" + done < <(find "$STATE_DIR/projects" -name browse.json -path '*/.gstack/*' 2>/dev/null || true) +fi + +# ─── Remove global Claude skills ──────────────────────────── +CLAUDE_SKILLS="$HOME/.claude/skills" +if [ -d "$CLAUDE_SKILLS/gstack" ] || [ -L "$CLAUDE_SKILLS/gstack" ]; then + # Remove per-skill symlinks that point into gstack/ + for _LINK in "$CLAUDE_SKILLS"/*; do + [ -L "$_LINK" ] || continue + _NAME="$(basename "$_LINK")" + [ "$_NAME" = "gstack" ] && continue + _TARGET="$(readlink "$_LINK" 2>/dev/null || true)" + case "$_TARGET" in + gstack/*|*/gstack/*) rm -f "$_LINK"; REMOVED+=("claude/$_NAME") ;; + esac + done + + rm -rf "$CLAUDE_SKILLS/gstack" + REMOVED+=("~/.claude/skills/gstack") +fi + +# ─── Remove project-local Claude skills (--local installs) ── +if [ -n "$_GIT_ROOT" ] && [ -d "$_GIT_ROOT/.claude/skills" ]; then + for _LINK in "$_GIT_ROOT/.claude/skills"/*; do + [ -L "$_LINK" ] || continue + _TARGET="$(readlink "$_LINK" 2>/dev/null || true)" + case "$_TARGET" in + gstack/*|*/gstack/*) rm -f "$_LINK"; REMOVED+=("local claude/$(basename "$_LINK")") ;; + esac + done + if [ -d "$_GIT_ROOT/.claude/skills/gstack" ] || [ -L "$_GIT_ROOT/.claude/skills/gstack" ]; then + rm -rf "$_GIT_ROOT/.claude/skills/gstack" + REMOVED+=("$_GIT_ROOT/.claude/skills/gstack") + fi +fi + +# ─── Remove Codex skills ──────────────────────────────────── +CODEX_SKILLS="$HOME/.codex/skills" +if [ -d "$CODEX_SKILLS" ]; then + for _ITEM in "$CODEX_SKILLS"/gstack*; do + [ -e "$_ITEM" ] || [ -L "$_ITEM" ] || continue + rm -rf "$_ITEM" + REMOVED+=("codex/$(basename "$_ITEM")") + done +fi + +# ─── Remove Kiro skills ───────────────────────────────────── +KIRO_SKILLS="$HOME/.kiro/skills" +if [ -d "$KIRO_SKILLS" ]; then + for _ITEM in "$KIRO_SKILLS"/gstack*; do + [ -e "$_ITEM" ] || [ -L "$_ITEM" ] || continue + rm -rf "$_ITEM" + REMOVED+=("kiro/$(basename "$_ITEM")") + done +fi + +# ─── Remove per-project .agents/ sidecar ───────────────────── +if [ -n "$_GIT_ROOT" ] && [ -d "$_GIT_ROOT/.agents/skills" ]; then + for _ITEM in "$_GIT_ROOT/.agents/skills"/gstack*; do + [ -e "$_ITEM" ] || [ -L "$_ITEM" ] || continue + rm -rf "$_ITEM" + REMOVED+=("agents/$(basename "$_ITEM")") + done + + rmdir "$_GIT_ROOT/.agents/skills" 2>/dev/null || true + rmdir "$_GIT_ROOT/.agents" 2>/dev/null || true +fi + +# ─── Remove per-project state ─────────────────────────────── +if [ -n "$_GIT_ROOT" ]; then + if [ -d "$_GIT_ROOT/.gstack" ]; then + rm -rf "$_GIT_ROOT/.gstack" + REMOVED+=("$_GIT_ROOT/.gstack/") + fi + if [ -d "$_GIT_ROOT/.gstack-worktrees" ]; then + rm -rf "$_GIT_ROOT/.gstack-worktrees" + REMOVED+=("$_GIT_ROOT/.gstack-worktrees/") + fi +fi + +# ─── Remove global state ──────────────────────────────────── +if [ "$KEEP_STATE" -eq 0 ] && [ -d "$STATE_DIR" ]; then + rm -rf "$STATE_DIR" + REMOVED+=("$STATE_DIR") +fi + +# ─── Clean up temp files ──────────────────────────────────── +for _TMP in /tmp/gstack-latest-version /tmp/gstack-sketch-*.html /tmp/gstack-sketch.png /tmp/gstack-sync-*; do + if [ -e "$_TMP" ]; then + rm -f "$_TMP" + REMOVED+=("$(basename "$_TMP")") + fi +done + +# ─── Summary ──────────────────────────────────────────────── +if [ ${#REMOVED[@]} -gt 0 ]; then + echo "Removed: ${REMOVED[*]}" + echo "gstack uninstalled." +else + echo "Nothing to remove — gstack is not installed." +fi + +exit 0 diff --git a/browse/src/server.ts b/browse/src/server.ts index 8d5a49e07fe2a797345f13338c647447eb69c47b..00e4393a080116974427cb967be45ebe9a283241 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -26,6 +26,7 @@ import { emitActivity, subscribe, getActivityAfter, getActivityHistory, getSubsc // Bun.spawn used instead of child_process.spawn (compiled bun binaries // fail posix_spawn on all executables including /bin/bash) import * as fs from 'fs'; +import * as net from 'net'; import * as path from 'path'; import * as crypto from 'crypto'; @@ -547,17 +548,28 @@ export { READ_COMMANDS, WRITE_COMMANDS, META_COMMANDS }; const browserManager = new BrowserManager(); let isShuttingDown = false; +// Test if a port is available by binding and immediately releasing. +// Uses net.createServer instead of Bun.serve to avoid a race condition +// in the Node.js polyfill where listen/close are async but the caller +// expects synchronous bind semantics. See: #486 +function isPortAvailable(port: number, hostname: string = '127.0.0.1'): Promise { + return new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, hostname, () => { + srv.close(() => resolve(true)); + }); + }); +} + // Find port: explicit BROWSE_PORT, or random in 10000-60000 async function findPort(): Promise { // Explicit port override (for debugging) if (BROWSE_PORT) { - try { - const testServer = Bun.serve({ port: BROWSE_PORT, fetch: () => new Response('ok') }); - testServer.stop(); + if (await isPortAvailable(BROWSE_PORT)) { return BROWSE_PORT; - } catch { - throw new Error(`[browse] Port ${BROWSE_PORT} (from BROWSE_PORT env) is in use`); } + throw new Error(`[browse] Port ${BROWSE_PORT} (from BROWSE_PORT env) is in use`); } // Random port with retry @@ -566,12 +578,8 @@ async function findPort(): Promise { const MAX_RETRIES = 5; for (let attempt = 0; attempt < MAX_RETRIES; attempt++) { const port = MIN_PORT + Math.floor(Math.random() * (MAX_PORT - MIN_PORT)); - try { - const testServer = Bun.serve({ port, fetch: () => new Response('ok') }); - testServer.stop(); + if (await isPortAvailable(port)) { return port; - } catch { - continue; } } throw new Error(`[browse] No available port after ${MAX_RETRIES} attempts in range ${MIN_PORT}-${MAX_PORT}`); diff --git a/browse/test/findport.test.ts b/browse/test/findport.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..fb3a9cb0942f76c4479ba51c9536c7fcbb2245ac --- /dev/null +++ b/browse/test/findport.test.ts @@ -0,0 +1,191 @@ +import { describe, test, expect } from 'bun:test'; +import * as net from 'net'; +import * as path from 'path'; + +const polyfillPath = path.resolve(import.meta.dir, '../src/bun-polyfill.cjs'); + +// Helper: bind a port and hold it open, returning a cleanup function +function occupyPort(port: number): Promise<() => Promise> { + return new Promise((resolve, reject) => { + const srv = net.createServer(); + srv.once('error', reject); + srv.listen(port, '127.0.0.1', () => { + resolve(() => new Promise((r) => srv.close(() => r()))); + }); + }); +} + +// Helper: find a known-free port by binding to 0 +function getFreePort(): Promise { + return new Promise((resolve, reject) => { + const srv = net.createServer(); + srv.once('error', reject); + srv.listen(0, '127.0.0.1', () => { + const port = (srv.address() as net.AddressInfo).port; + srv.close(() => resolve(port)); + }); + }); +} + +describe('findPort / isPortAvailable', () => { + + test('isPortAvailable returns true for a free port', async () => { + // Use the same isPortAvailable logic from server.ts + const port = await getFreePort(); + + const available = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + expect(available).toBe(true); + }); + + test('isPortAvailable returns false for an occupied port', async () => { + const port = await getFreePort(); + const release = await occupyPort(port); + + try { + const available = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + expect(available).toBe(false); + } finally { + await release(); + } + }); + + test('port is actually free after isPortAvailable returns true', async () => { + // This is the core race condition test: after isPortAvailable says + // a port is free, can we IMMEDIATELY bind to it? + const port = await getFreePort(); + + // Simulate isPortAvailable + const isFree = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + expect(isFree).toBe(true); + + // Now immediately try to bind — this would fail with the old + // Bun.serve() polyfill approach because the test server's + // listen() would still be pending + const canBind = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + expect(canBind).toBe(true); + }); + + test('polyfill Bun.serve stop() is fire-and-forget (async)', async () => { + // Verify that the polyfill's stop() does NOT wait for the socket + // to actually close — this is the root cause of the race condition. + // On macOS/Linux the OS reclaims the port fast enough that the race + // rarely manifests, but on Windows TIME_WAIT makes it 100% repro. + const result = Bun.spawnSync(['node', '-e', ` + require('${polyfillPath}'); + const net = require('net'); + + async function test() { + const port = 10000 + Math.floor(Math.random() * 50000); + + const testServer = Bun.serve({ + port, + hostname: '127.0.0.1', + fetch: () => new Response('ok'), + }); + + // stop() returns undefined — it does NOT return a Promise, + // so callers cannot await socket teardown + const retval = testServer.stop(); + console.log(typeof retval === 'undefined' ? 'FIRE_AND_FORGET' : 'AWAITABLE'); + } + + test(); + `], { stdout: 'pipe', stderr: 'pipe' }); + + const output = result.stdout.toString().trim(); + // Confirms the polyfill's stop() is fire-and-forget — callers + // cannot wait for the port to be released, hence the race + expect(output).toBe('FIRE_AND_FORGET'); + }); + + test('net.createServer approach does not have the race condition', async () => { + // Prove the fix: net.createServer with proper async bind/close + // releases the port cleanly + const result = Bun.spawnSync(['node', '-e', ` + const net = require('net'); + + async function testFix() { + const port = 10000 + Math.floor(Math.random() * 50000); + + // Simulate the NEW isPortAvailable: proper async bind/close + const isFree = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + if (!isFree) { + console.log('PORT_BUSY'); + return; + } + + // Immediately try to bind — should succeed because close() + // completed before the Promise resolved + const canBind = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + + console.log(canBind ? 'FIX_WORKS' : 'FIX_BROKEN'); + } + + testFix(); + `], { stdout: 'pipe', stderr: 'pipe' }); + + const output = result.stdout.toString().trim(); + expect(output).toBe('FIX_WORKS'); + }); + + test('isPortAvailable handles rapid sequential checks', async () => { + // Stress test: check the same port multiple times in sequence + const port = await getFreePort(); + const results: boolean[] = []; + + for (let i = 0; i < 5; i++) { + const available = await new Promise((resolve) => { + const srv = net.createServer(); + srv.once('error', () => resolve(false)); + srv.listen(port, '127.0.0.1', () => { + srv.close(() => resolve(true)); + }); + }); + results.push(available); + } + + // All 5 checks should succeed — no leaked sockets + expect(results).toEqual([true, true, true, true, true]); + }); +}); diff --git a/office-hours/SKILL.md b/office-hours/SKILL.md index 5ad69fbeb5eb95c551ed42af2c02de6f84b92533..345f4c007af9c9c074c0f687e5d704c6488d3d07 100644 --- a/office-hours/SKILL.md +++ b/office-hours/SKILL.md @@ -674,21 +674,19 @@ Use AskUserQuestion to confirm. If the user disagrees with a premise, revise und ## Phase 3.5: Cross-Model Second Opinion (optional) -**Binary check first — no question if unavailable:** +**Binary check first:** ```bash which codex 2>/dev/null && echo "CODEX_AVAILABLE" || echo "CODEX_NOT_AVAILABLE" ``` -If `CODEX_NOT_AVAILABLE`: skip Phase 3.5 entirely — no message, no AskUserQuestion. Proceed directly to Phase 4. +Use AskUserQuestion (regardless of codex availability): -If `CODEX_AVAILABLE`: use AskUserQuestion: - -> Want a second opinion from a different AI model? Codex will independently review your problem statement, key answers, premises, and any landscape findings from this session. It hasn't seen this conversation — it gets a structured summary. Usually takes 2-5 minutes. +> Want a second opinion from an independent AI perspective? It will review your problem statement, key answers, premises, and any landscape findings from this session without having seen this conversation — it gets a structured summary. Usually takes 2-5 minutes. > A) Yes, get a second opinion > B) No, proceed to alternatives -If B: skip Phase 3.5 entirely. Remember that Codex did NOT run (affects design doc, founder signals, and Phase 4 below). +If B: skip Phase 3.5 entirely. Remember that the second opinion did NOT run (affects design doc, founder signals, and Phase 4 below). **If A: Run the Codex cold read.** @@ -726,15 +724,26 @@ cat "$TMPERR_OH" rm -f "$TMPERR_OH" "$CODEX_PROMPT_FILE" ``` -**Error handling:** All errors are non-blocking — Codex second opinion is a quality enhancement, not a prerequisite. -- **Auth failure:** If stderr contains "auth", "login", "unauthorized", or "API key": "Codex authentication failed. Run \`codex login\` to authenticate. Skipping second opinion." -- **Timeout:** "Codex timed out after 5 minutes. Skipping second opinion." -- **Empty response:** "Codex returned no response. Stderr: . Skipping second opinion." +**Error handling:** All errors are non-blocking — second opinion is a quality enhancement, not a prerequisite. +- **Auth failure:** If stderr contains "auth", "login", "unauthorized", or "API key": "Codex authentication failed. Run \`codex login\` to authenticate." Fall back to Claude subagent. +- **Timeout:** "Codex timed out after 5 minutes." Fall back to Claude subagent. +- **Empty response:** "Codex returned no response." Fall back to Claude subagent. + +On any Codex error, fall back to the Claude subagent below. + +**If CODEX_NOT_AVAILABLE (or Codex errored):** + +Dispatch via the Agent tool. The subagent has fresh context — genuine independence. + +Subagent prompt: same mode-appropriate prompt as above (Startup or Builder variant). + +Present findings under a `SECOND OPINION (Claude subagent):` header. -On any error, proceed to Phase 4 — do NOT fall back to a Claude subagent (this is brainstorming, not adversarial review). +If the subagent fails or times out: "Second opinion unavailable. Continuing to Phase 4." 4. **Presentation:** +If Codex ran: ``` SECOND OPINION (Codex): ════════════════════════════════════════════════════════════ @@ -742,10 +751,18 @@ SECOND OPINION (Codex): ════════════════════════════════════════════════════════════ ``` -5. **Cross-model synthesis:** After presenting Codex output, provide 3-5 bullet synthesis: - - Where Claude agrees with Codex +If Claude subagent ran: +``` +SECOND OPINION (Claude subagent): +════════════════════════════════════════════════════════════ + +════════════════════════════════════════════════════════════ +``` + +5. **Cross-model synthesis:** After presenting the second opinion output, provide 3-5 bullet synthesis: + - Where Claude agrees with the second opinion - Where Claude disagrees and why - - Whether Codex's challenged premise changes Claude's recommendation + - Whether the challenged premise changes Claude's recommendation 6. **Premise revision check:** If Codex challenged an agreed premise, use AskUserQuestion: @@ -783,7 +800,7 @@ Rules: - One must be the **"minimal viable"** (fewest files, smallest diff, ships fastest). - One must be the **"ideal architecture"** (best long-term trajectory, most elegant). - One can be **creative/lateral** (unexpected approach, different framing of the problem). -- If Codex proposed a prototype in Phase 3.5, consider using it as a starting point for the creative/lateral approach. +- If the second opinion (Codex or Claude subagent) proposed a prototype in Phase 3.5, consider using it as a starting point for the creative/lateral approach. **RECOMMENDATION:** Choose [X] because [one-line reason]. @@ -949,7 +966,7 @@ Supersedes: {prior filename — omit this line if first design on this branch} {from Phase 3} ## Cross-Model Perspective -{If Codex ran in Phase 3.5: Codex's independent cold read — steelman, key insight, challenged premise, prototype suggestion. Verbatim or close paraphrase of what Codex said. If Codex did NOT run (skipped or unavailable): omit this section entirely — do not include it.} +{If second opinion ran in Phase 3.5 (Codex or Claude subagent): independent cold read — steelman, key insight, challenged premise, prototype suggestion. Verbatim or close paraphrase. If second opinion did NOT run (skipped or unavailable): omit this section entirely — do not include it.} ## Approaches Considered ### Approach A: {name} @@ -1006,7 +1023,7 @@ Supersedes: {prior filename — omit this line if first design on this branch} {from Phase 3} ## Cross-Model Perspective -{If Codex ran in Phase 3.5: Codex's independent cold read — coolest version, key insight, existing tools, prototype suggestion. Verbatim or close paraphrase of what Codex said. If Codex did NOT run (skipped or unavailable): omit this section entirely — do not include it.} +{If second opinion ran in Phase 3.5 (Codex or Claude subagent): independent cold read — coolest version, key insight, existing tools, prototype suggestion. Verbatim or close paraphrase. If second opinion did NOT run (skipped or unavailable): omit this section entirely — do not include it.} ## Approaches Considered ### Approach A: {name} diff --git a/office-hours/SKILL.md.tmpl b/office-hours/SKILL.md.tmpl index c6de598f25edb7c65d097a843aa40598b0d72965..5e7187449db761aa33d2dee86a8c63b340d513ea 100644 --- a/office-hours/SKILL.md.tmpl +++ b/office-hours/SKILL.md.tmpl @@ -382,7 +382,7 @@ Rules: - One must be the **"minimal viable"** (fewest files, smallest diff, ships fastest). - One must be the **"ideal architecture"** (best long-term trajectory, most elegant). - One can be **creative/lateral** (unexpected approach, different framing of the problem). -- If Codex proposed a prototype in Phase 3.5, consider using it as a starting point for the creative/lateral approach. +- If the second opinion (Codex or Claude subagent) proposed a prototype in Phase 3.5, consider using it as a starting point for the creative/lateral approach. **RECOMMENDATION:** Choose [X] because [one-line reason]. @@ -462,7 +462,7 @@ Supersedes: {prior filename — omit this line if first design on this branch} {from Phase 3} ## Cross-Model Perspective -{If Codex ran in Phase 3.5: Codex's independent cold read — steelman, key insight, challenged premise, prototype suggestion. Verbatim or close paraphrase of what Codex said. If Codex did NOT run (skipped or unavailable): omit this section entirely — do not include it.} +{If second opinion ran in Phase 3.5 (Codex or Claude subagent): independent cold read — steelman, key insight, challenged premise, prototype suggestion. Verbatim or close paraphrase. If second opinion did NOT run (skipped or unavailable): omit this section entirely — do not include it.} ## Approaches Considered ### Approach A: {name} @@ -519,7 +519,7 @@ Supersedes: {prior filename — omit this line if first design on this branch} {from Phase 3} ## Cross-Model Perspective -{If Codex ran in Phase 3.5: Codex's independent cold read — coolest version, key insight, existing tools, prototype suggestion. Verbatim or close paraphrase of what Codex said. If Codex did NOT run (skipped or unavailable): omit this section entirely — do not include it.} +{If second opinion ran in Phase 3.5 (Codex or Claude subagent): independent cold read — coolest version, key insight, existing tools, prototype suggestion. Verbatim or close paraphrase. If second opinion did NOT run (skipped or unavailable): omit this section entirely — do not include it.} ## Approaches Considered ### Approach A: {name} diff --git a/package.json b/package.json index aa5fcfb9a9ac06a0fb6e1546ce05d17eef812444..abf2fa55601f05ed8628dd1b52b534d018f49a4a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "0.12.8.1", + "version": "0.12.9.0", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module", diff --git a/review/checklist.md b/review/checklist.md index 7f7923ff8aa96057ac03cfb39b8b08636c241cfb..cfedcf81f3d7fd453fb348c12762ff436d7c73e9 100644 --- a/review/checklist.md +++ b/review/checklist.md @@ -49,6 +49,13 @@ Be terse. For each issue: one line describing the problem, one line with the fix #### LLM Output Trust Boundary - LLM-generated values (emails, URLs, names) written to DB or passed to mailers without format validation. Add lightweight guards (`EMAIL_REGEXP`, `URI.parse`, `.strip`) before persisting. - Structured tool output (arrays, hashes) accepted without type/shape checks before database writes. +- LLM-generated URLs fetched without allowlist — SSRF risk if URL points to internal network (Python: `urllib.parse.urlparse` → check hostname against blocklist before `requests.get`/`httpx.get`) +- LLM output stored in knowledge bases or vector DBs without sanitization — stored prompt injection risk + +#### Shell Injection (Python-specific) +- `subprocess.run()` / `subprocess.call()` / `subprocess.Popen()` with `shell=True` AND f-string/`.format()` interpolation in the command string — use argument arrays instead +- `os.system()` with variable interpolation — replace with `subprocess.run()` using argument arrays +- `eval()` / `exec()` on LLM-generated code without sandboxing #### Enum & Value Completeness When the diff introduces a new enum value, status string, tier name, or type constant: @@ -59,6 +66,16 @@ To do this: use Grep to find all references to the sibling values (e.g., grep fo ### Pass 2 — INFORMATIONAL +#### Async/Sync Mixing (Python-specific) +- Synchronous `subprocess.run()`, `open()`, `requests.get()` inside `async def` endpoints — blocks the event loop. Use `asyncio.to_thread()`, `aiofiles`, or `httpx.AsyncClient` instead. +- `time.sleep()` inside async functions — use `asyncio.sleep()` +- Sync DB calls in async context without `run_in_executor()` wrapping + +#### Column/Field Name Safety +- Verify column names in ORM queries (`.select()`, `.eq()`, `.gte()`, `.order()`) against actual DB schema — wrong column names silently return empty results or throw swallowed errors +- Check `.get()` calls on query results use the column name that was actually selected +- Cross-reference with schema documentation when available + #### Conditional Side Effects - Code paths that branch on a condition but forget to apply a side effect on one branch. Example: item promoted to verified but URL only attached when a secondary condition is true — the other branch promotes without the URL, creating an inconsistent record. - Log messages that claim an action happened but the action was conditionally skipped. The log should reflect what actually occurred. diff --git a/scripts/resolvers/review.ts b/scripts/resolvers/review.ts index bf09a528bf7e636fa2cb0c6c2237e19915febd64..382a8ddf37dece500ab5249e729c9b95fbe77457 100644 --- a/scripts/resolvers/review.ts +++ b/scripts/resolvers/review.ts @@ -251,21 +251,19 @@ export function generateCodexSecondOpinion(ctx: TemplateContext): string { return `## Phase 3.5: Cross-Model Second Opinion (optional) -**Binary check first — no question if unavailable:** +**Binary check first:** \`\`\`bash which codex 2>/dev/null && echo "CODEX_AVAILABLE" || echo "CODEX_NOT_AVAILABLE" \`\`\` -If \`CODEX_NOT_AVAILABLE\`: skip Phase 3.5 entirely — no message, no AskUserQuestion. Proceed directly to Phase 4. +Use AskUserQuestion (regardless of codex availability): -If \`CODEX_AVAILABLE\`: use AskUserQuestion: - -> Want a second opinion from a different AI model? Codex will independently review your problem statement, key answers, premises, and any landscape findings from this session. It hasn't seen this conversation — it gets a structured summary. Usually takes 2-5 minutes. +> Want a second opinion from an independent AI perspective? It will review your problem statement, key answers, premises, and any landscape findings from this session without having seen this conversation — it gets a structured summary. Usually takes 2-5 minutes. > A) Yes, get a second opinion > B) No, proceed to alternatives -If B: skip Phase 3.5 entirely. Remember that Codex did NOT run (affects design doc, founder signals, and Phase 4 below). +If B: skip Phase 3.5 entirely. Remember that the second opinion did NOT run (affects design doc, founder signals, and Phase 4 below). **If A: Run the Codex cold read.** @@ -303,15 +301,26 @@ cat "$TMPERR_OH" rm -f "$TMPERR_OH" "$CODEX_PROMPT_FILE" \`\`\` -**Error handling:** All errors are non-blocking — Codex second opinion is a quality enhancement, not a prerequisite. -- **Auth failure:** If stderr contains "auth", "login", "unauthorized", or "API key": "Codex authentication failed. Run \\\`codex login\\\` to authenticate. Skipping second opinion." -- **Timeout:** "Codex timed out after 5 minutes. Skipping second opinion." -- **Empty response:** "Codex returned no response. Stderr: . Skipping second opinion." +**Error handling:** All errors are non-blocking — second opinion is a quality enhancement, not a prerequisite. +- **Auth failure:** If stderr contains "auth", "login", "unauthorized", or "API key": "Codex authentication failed. Run \\\`codex login\\\` to authenticate." Fall back to Claude subagent. +- **Timeout:** "Codex timed out after 5 minutes." Fall back to Claude subagent. +- **Empty response:** "Codex returned no response." Fall back to Claude subagent. + +On any Codex error, fall back to the Claude subagent below. + +**If CODEX_NOT_AVAILABLE (or Codex errored):** + +Dispatch via the Agent tool. The subagent has fresh context — genuine independence. + +Subagent prompt: same mode-appropriate prompt as above (Startup or Builder variant). -On any error, proceed to Phase 4 — do NOT fall back to a Claude subagent (this is brainstorming, not adversarial review). +Present findings under a \`SECOND OPINION (Claude subagent):\` header. + +If the subagent fails or times out: "Second opinion unavailable. Continuing to Phase 4." 4. **Presentation:** +If Codex ran: \`\`\` SECOND OPINION (Codex): ════════════════════════════════════════════════════════════ @@ -319,10 +328,18 @@ SECOND OPINION (Codex): ════════════════════════════════════════════════════════════ \`\`\` -5. **Cross-model synthesis:** After presenting Codex output, provide 3-5 bullet synthesis: - - Where Claude agrees with Codex +If Claude subagent ran: +\`\`\` +SECOND OPINION (Claude subagent): +════════════════════════════════════════════════════════════ + +════════════════════════════════════════════════════════════ +\`\`\` + +5. **Cross-model synthesis:** After presenting the second opinion output, provide 3-5 bullet synthesis: + - Where Claude agrees with the second opinion - Where Claude disagrees and why - - Whether Codex's challenged premise changes Claude's recommendation + - Whether the challenged premise changes Claude's recommendation 6. **Premise revision check:** If Codex challenged an agreed premise, use AskUserQuestion: diff --git a/setup b/setup index bfae87851c907dc17296fd81ad419a3ac38eb6a3..71306839a58d123e8025344cc5b523a43ae22129 100755 --- a/setup +++ b/setup @@ -23,11 +23,13 @@ esac # ─── Parse flags ────────────────────────────────────────────── HOST="claude" LOCAL_INSTALL=0 +SKILL_PREFIX=1 while [ $# -gt 0 ]; do case "$1" in --host) [ -z "$2" ] && echo "Missing value for --host (expected claude, codex, kiro, or auto)" >&2 && exit 1; HOST="$2"; shift 2 ;; --host=*) HOST="${1#--host=}"; shift ;; --local) LOCAL_INSTALL=1; shift ;; + --no-prefix) SKILL_PREFIX=0; shift ;; *) shift ;; esac done @@ -199,6 +201,9 @@ fi mkdir -p "$HOME/.gstack/projects" # ─── Helper: link Claude skill subdirectories into a skills parent directory ── +# When SKILL_PREFIX=1 (default), symlinks are prefixed with "gstack-" to avoid +# namespace pollution (e.g., gstack-review instead of review). +# Use --no-prefix to restore the old flat names. link_claude_skill_dirs() { local gstack_dir="$1" local skills_dir="$2" @@ -208,11 +213,20 @@ link_claude_skill_dirs() { skill_name="$(basename "$skill_dir")" # Skip node_modules [ "$skill_name" = "node_modules" ] && continue - target="$skills_dir/$skill_name" + # Apply gstack- prefix unless --no-prefix or already prefixed + if [ "$SKILL_PREFIX" -eq 1 ]; then + case "$skill_name" in + gstack-*) link_name="$skill_name" ;; + *) link_name="gstack-$skill_name" ;; + esac + else + link_name="$skill_name" + fi + target="$skills_dir/$link_name" # Create or update symlink; skip if a real file/directory exists if [ -L "$target" ] || [ ! -e "$target" ]; then ln -snf "gstack/$skill_name" "$target" - linked+=("$skill_name") + linked+=("$link_name") fi fi done @@ -221,6 +235,37 @@ link_claude_skill_dirs() { fi } +# ─── Helper: remove old unprefixed Claude skill symlinks ────────────────────── +# Migration: when switching from flat names to gstack- prefixed names, +# clean up stale symlinks that point into the gstack directory. +cleanup_old_claude_symlinks() { + local gstack_dir="$1" + local skills_dir="$2" + local removed=() + for skill_dir in "$gstack_dir"/*/; do + if [ -f "$skill_dir/SKILL.md" ]; then + skill_name="$(basename "$skill_dir")" + [ "$skill_name" = "node_modules" ] && continue + # Skip already-prefixed dirs (gstack-upgrade) — no old symlink to clean + case "$skill_name" in gstack-*) continue ;; esac + old_target="$skills_dir/$skill_name" + # Only remove if it's a symlink pointing into gstack/ + if [ -L "$old_target" ]; then + link_dest="$(readlink "$old_target" 2>/dev/null || true)" + case "$link_dest" in + gstack/*|*/gstack/*) + rm -f "$old_target" + removed+=("$skill_name") + ;; + esac + fi + fi + done + if [ ${#removed[@]} -gt 0 ]; then + echo " cleaned up old symlinks: ${removed[*]}" + fi +} + # ─── Helper: link generated Codex skills into a skills parent directory ── # Installs from .agents/skills/gstack-* (the generated Codex-format skills) # instead of source dirs (which have Claude paths). @@ -348,6 +393,10 @@ fi if [ "$INSTALL_CLAUDE" -eq 1 ]; then if [ "$SKILLS_BASENAME" = "skills" ]; then + # Clean up old unprefixed symlinks from previous installs + if [ "$SKILL_PREFIX" -eq 1 ]; then + cleanup_old_claude_symlinks "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" + fi link_claude_skill_dirs "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" if [ "$LOCAL_INSTALL" -eq 1 ]; then echo "gstack ready (project-local)." diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index cac45ec775dfe12c4b4f0a6a513ea65e56ea88d9..88308ff5bb64884569b273f4286fff138abbb24a 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -1023,12 +1023,18 @@ describe('CODEX_SECOND_OPINION resolver', () => { }); test('contains opt-in AskUserQuestion text', () => { - expect(content).toContain('second opinion from a different AI model'); + expect(content).toContain('second opinion from an independent AI perspective'); }); test('contains cross-model synthesis instructions', () => { expect(content).toMatch(/[Ss]ynthesis/); - expect(content).toContain('Where Claude agrees with Codex'); + expect(content).toContain('Where Claude agrees with the second opinion'); + }); + + test('contains Claude subagent fallback', () => { + expect(content).toContain('CODEX_NOT_AVAILABLE'); + expect(content).toContain('Agent tool'); + expect(content).toContain('SECOND OPINION (Claude subagent)'); }); test('contains premise revision check', () => { @@ -1635,6 +1641,50 @@ describe('setup script validation', () => { expect(setupContent).toContain('$HOME/.gstack/repos/gstack'); expect(setupContent).toContain('avoid duplicate skill discovery'); }); + + // --- Symlink prefix tests (PR #503) --- + + test('link_claude_skill_dirs applies gstack- prefix by default', () => { + const fnStart = setupContent.indexOf('link_claude_skill_dirs()'); + const fnEnd = setupContent.indexOf('}', setupContent.indexOf('linked[@]}', fnStart)); + const fnBody = setupContent.slice(fnStart, fnEnd); + expect(fnBody).toContain('SKILL_PREFIX'); + expect(fnBody).toContain('link_name="gstack-$skill_name"'); + }); + + test('link_claude_skill_dirs preserves already-prefixed dirs', () => { + const fnStart = setupContent.indexOf('link_claude_skill_dirs()'); + const fnEnd = setupContent.indexOf('}', setupContent.indexOf('linked[@]}', fnStart)); + const fnBody = setupContent.slice(fnStart, fnEnd); + // gstack-* dirs should keep their name (e.g., gstack-upgrade stays gstack-upgrade) + expect(fnBody).toContain('gstack-*) link_name="$skill_name"'); + }); + + test('setup supports --no-prefix flag', () => { + expect(setupContent).toContain('--no-prefix'); + expect(setupContent).toContain('SKILL_PREFIX=0'); + }); + + test('cleanup_old_claude_symlinks removes only gstack-pointing symlinks', () => { + expect(setupContent).toContain('cleanup_old_claude_symlinks'); + const fnStart = setupContent.indexOf('cleanup_old_claude_symlinks()'); + const fnEnd = setupContent.indexOf('}', setupContent.indexOf('removed[@]}', fnStart)); + const fnBody = setupContent.slice(fnStart, fnEnd); + // Should check readlink before removing + expect(fnBody).toContain('readlink'); + expect(fnBody).toContain('gstack/*'); + // Should skip already-prefixed dirs + expect(fnBody).toContain('gstack-*) continue'); + }); + + test('cleanup runs before link when prefix is enabled', () => { + // In the Claude install section, cleanup should happen before linking + const claudeInstallSection = setupContent.slice( + setupContent.indexOf('INSTALL_CLAUDE'), + setupContent.lastIndexOf('link_claude_skill_dirs') + ); + expect(claudeInstallSection).toContain('cleanup_old_claude_symlinks'); + }); }); describe('discover-skills hidden directory filtering', () => { diff --git a/test/uninstall.test.ts b/test/uninstall.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..a7208e87707c2b39ca5ba10d9f42baf102fbb496 --- /dev/null +++ b/test/uninstall.test.ts @@ -0,0 +1,165 @@ +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const UNINSTALL = path.join(ROOT, 'bin', 'gstack-uninstall'); + +describe('gstack-uninstall', () => { + test('syntax check passes', () => { + const result = spawnSync('bash', ['-n', UNINSTALL], { stdio: 'pipe' }); + expect(result.status).toBe(0); + }); + + test('--help prints usage and exits 0', () => { + const result = spawnSync('bash', [UNINSTALL, '--help'], { stdio: 'pipe' }); + expect(result.status).toBe(0); + const output = result.stdout.toString(); + expect(output).toContain('gstack-uninstall'); + expect(output).toContain('--force'); + expect(output).toContain('--keep-state'); + }); + + test('unknown flag exits with error', () => { + const result = spawnSync('bash', [UNINSTALL, '--bogus'], { + stdio: 'pipe', + env: { ...process.env, HOME: '/nonexistent' }, + }); + expect(result.status).toBe(1); + expect(result.stderr.toString()).toContain('Unknown option'); + }); + + describe('integration tests with mock layout', () => { + let tmpDir: string; + let mockHome: string; + let mockGitRoot: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-uninstall-test-')); + mockHome = path.join(tmpDir, 'home'); + mockGitRoot = path.join(tmpDir, 'repo'); + + // Create mock gstack install layout + fs.mkdirSync(path.join(mockHome, '.claude', 'skills', 'gstack'), { recursive: true }); + fs.writeFileSync(path.join(mockHome, '.claude', 'skills', 'gstack', 'SKILL.md'), 'test'); + + // Create per-skill symlinks (both old unprefixed and new prefixed) + fs.symlinkSync('gstack/review', path.join(mockHome, '.claude', 'skills', 'review')); + fs.symlinkSync('gstack/ship', path.join(mockHome, '.claude', 'skills', 'gstack-ship')); + + // Create a non-gstack symlink (should NOT be removed) + fs.mkdirSync(path.join(mockHome, '.claude', 'skills', 'other-tool'), { recursive: true }); + + // Create state directory + fs.mkdirSync(path.join(mockHome, '.gstack', 'projects'), { recursive: true }); + fs.writeFileSync(path.join(mockHome, '.gstack', 'config.json'), '{}'); + + // Create mock git repo + fs.mkdirSync(mockGitRoot, { recursive: true }); + spawnSync('git', ['init', '-b', 'main'], { cwd: mockGitRoot, stdio: 'pipe' }); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('--force removes global Claude skills and state', () => { + const result = spawnSync('bash', [UNINSTALL, '--force'], { + stdio: 'pipe', + env: { + ...process.env, + HOME: mockHome, + GSTACK_DIR: path.join(mockHome, '.claude', 'skills', 'gstack'), + GSTACK_STATE_DIR: path.join(mockHome, '.gstack'), + }, + cwd: mockGitRoot, + }); + + expect(result.status).toBe(0); + const output = result.stdout.toString(); + expect(output).toContain('gstack uninstalled'); + + // Global skill dir should be removed + expect(fs.existsSync(path.join(mockHome, '.claude', 'skills', 'gstack'))).toBe(false); + + // Per-skill symlinks pointing into gstack/ should be removed + expect(fs.existsSync(path.join(mockHome, '.claude', 'skills', 'review'))).toBe(false); + expect(fs.existsSync(path.join(mockHome, '.claude', 'skills', 'gstack-ship'))).toBe(false); + + // Non-gstack tool should still exist + expect(fs.existsSync(path.join(mockHome, '.claude', 'skills', 'other-tool'))).toBe(true); + + // State should be removed + expect(fs.existsSync(path.join(mockHome, '.gstack'))).toBe(false); + }); + + test('--keep-state preserves state directory', () => { + const result = spawnSync('bash', [UNINSTALL, '--force', '--keep-state'], { + stdio: 'pipe', + env: { + ...process.env, + HOME: mockHome, + GSTACK_DIR: path.join(mockHome, '.claude', 'skills', 'gstack'), + GSTACK_STATE_DIR: path.join(mockHome, '.gstack'), + }, + cwd: mockGitRoot, + }); + + expect(result.status).toBe(0); + + // Skills should be removed + expect(fs.existsSync(path.join(mockHome, '.claude', 'skills', 'gstack'))).toBe(false); + + // State should still exist + expect(fs.existsSync(path.join(mockHome, '.gstack'))).toBe(true); + expect(fs.existsSync(path.join(mockHome, '.gstack', 'config.json'))).toBe(true); + }); + + test('clean system outputs nothing to remove', () => { + const cleanHome = path.join(tmpDir, 'clean-home'); + fs.mkdirSync(cleanHome, { recursive: true }); + + const result = spawnSync('bash', [UNINSTALL, '--force'], { + stdio: 'pipe', + env: { + ...process.env, + HOME: cleanHome, + GSTACK_DIR: path.join(cleanHome, 'nonexistent'), + GSTACK_STATE_DIR: path.join(cleanHome, '.gstack'), + }, + cwd: mockGitRoot, + }); + + expect(result.status).toBe(0); + expect(result.stdout.toString()).toContain('Nothing to remove'); + }); + + test('upgrade path: prefixed install + uninstall cleans both old and new symlinks', () => { + // Simulate the state after setup --no-prefix followed by setup (with prefix): + // Both old unprefixed and new prefixed symlinks exist + // (mockHome already has both 'review' and 'gstack-ship' symlinks) + + const result = spawnSync('bash', [UNINSTALL, '--force'], { + stdio: 'pipe', + env: { + ...process.env, + HOME: mockHome, + GSTACK_DIR: path.join(mockHome, '.claude', 'skills', 'gstack'), + GSTACK_STATE_DIR: path.join(mockHome, '.gstack'), + }, + cwd: mockGitRoot, + }); + + expect(result.status).toBe(0); + + // Both old (review) and new (gstack-ship) symlinks should be gone + expect(fs.existsSync(path.join(mockHome, '.claude', 'skills', 'review'))).toBe(false); + expect(fs.existsSync(path.join(mockHome, '.claude', 'skills', 'gstack-ship'))).toBe(false); + + // Non-gstack should survive + expect(fs.existsSync(path.join(mockHome, '.claude', 'skills', 'other-tool'))).toBe(true); + }); + }); +});