diff --git a/CHANGELOG.md b/CHANGELOG.md index 17970e7dde..6d22d6c656 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,38 @@ # Changelog +## [1.79.1.0] - 2026-09-04 + +**GBrain readiness is now a bounded, read-only check. Opening a session cannot +silently sync a repository, launch an embedding job, or spend provider money.** + +GStack previously tried to repair a missing GBrain source pin automatically +from session and skill startup. On a new repository that could run before the +per-repository consent gate, send code to the configured brain, and leave a +foreground skill waiting indefinitely on a stuck GBrain subprocess. The repair +path is now explicit: automatic startup only reports readiness, while +`/sync-gbrain --full` owns synchronization and indexing after the existing +trust policy has been checked. + +### What changed + +- Every foreground GBrain probe has a 1–10 second timeout, with a three-second + default and a portable macOS fallback. A slow or dead brain degrades the + readiness signal instead of blocking `/review`, `/ship`, or another skill. +- Missing, malformed, unregistered, or path-mismatched source pins fail closed + and point to the explicit `/sync-gbrain --full` repair path. +- Session startup and skill startup never invoke sync, import, dream, embed, + edge backfill, source registration, or paid provider work. +- Candor-specific Claude hooks and locally patched generated skill names were + removed from the generic upstream branch. Canonical generated files are + fresh again. + +### Verification + +- 119 focused readiness, sync, dream, skill-start, team-mode, and generated-doc + tests passed with zero failures before release preparation. +- Independent exact-head review confirmed the prior privacy, timeout, source-ID, + generated-file, stale-base, and project-specific-hook blockers are closed. + ## [1.79.0.0] - 2026-09-01 **/ship can no longer be stranded by a backgrounded subagent.** diff --git a/VERSION b/VERSION index a1b96f74eb..44cea7ae2a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.79.0.0 +1.79.1.0 diff --git a/agents-digest/gstack-AGENTS.md b/agents-digest/gstack-AGENTS.md index fd164655c9..ed981fb4c3 100644 --- a/agents-digest/gstack-AGENTS.md +++ b/agents-digest/gstack-AGENTS.md @@ -1,4 +1,4 @@ -# gstack digest v1.79.0.0 — regenerate/re-copy after upgrading gstack +# gstack digest v1.79.1.0 — regenerate/re-copy after upgrading gstack Behavioral rules from gstack (https://github.com/garrytan/gstack), compressed for agent hosts without a full skill install. The full skills add workflows, diff --git a/bin/gstack-gbrain-ready b/bin/gstack-gbrain-ready new file mode 100755 index 0000000000..cc00210bd2 --- /dev/null +++ b/bin/gstack-gbrain-ready @@ -0,0 +1,191 @@ +#!/usr/bin/env bash +# Read-only proof that this exact worktree has a usable GBrain code graph. +# +# This command is called from the latency-sensitive skill preamble. It must +# never sync, index, run a dream cycle, spend provider credits, or write to an +# external database. `/sync-gbrain` is the only repair path. The only writes +# here are disposable local readiness receipts under GSTACK_HOME. + +set +e + +STATE_ROOT="${GSTACK_HOME:-$HOME/.gstack}/gbrain-readiness" +REPO_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) +READINESS_SCHEMA=4 +SESSION_KEY="" + +PROBE_TIMEOUT_SECONDS="${GSTACK_GBRAIN_READY_TIMEOUT_SECONDS:-5}" +case "$PROBE_TIMEOUT_SECONDS" in + ''|*[!0-9]*) PROBE_TIMEOUT_SECONDS=5 ;; + *) + if [ "$PROBE_TIMEOUT_SECONDS" -lt 1 ] || [ "$PROBE_TIMEOUT_SECONDS" -gt 10 ]; then + PROBE_TIMEOUT_SECONDS=5 + fi + ;; +esac + +degraded() { + echo "GBRAIN_GRAPH: degraded | reason=$1" + exit 0 +} + +while [ "$#" -gt 0 ]; do + case "$1" in + --session-key) + [ "$#" -ge 2 ] || degraded "missing_session_key" + SESSION_KEY=$(printf '%s' "$2" | tr -cd 'A-Za-z0-9._-') + shift 2 + ;; + --check-only) + # Compatibility flag. Readiness is now always check-only. + shift + ;; + *) + degraded "unknown_argument" + ;; + esac +done + +# Portable timeout with one lifecycle owner. Bash job control gives the probe +# its own process group on stock macOS; timeout tears down that whole group, +# first gracefully and then with SIGKILL. Polling a separate timer avoids the +# classic watchdog race where the parent dies on SIGTERM, the watchdog gets +# cancelled, and a TERM-resistant descendant keeps the caller's pipe open. +run_bounded() { + _duration="$1" + shift + _had_monitor=0 + case "$-" in *m*) _had_monitor=1 ;; esac + set -m + # Keep pipeline input attached explicitly. Non-interactive shells may replace + # stdin for an asynchronous command with /dev/null unless it has a redirect. + "$@" <&0 & + _cmd_pid=$! + [ "$_had_monitor" -eq 1 ] || set +m + + sleep "$_duration" & + _timer_pid=$! + while kill -0 "$_cmd_pid" 2>/dev/null && kill -0 "$_timer_pid" 2>/dev/null; do + sleep 0.05 + done + + if kill -0 "$_cmd_pid" 2>/dev/null; then + # Timer won. The negative PID addresses the isolated process group, not + # just the direct wrapper process. + wait "$_timer_pid" 2>/dev/null + kill -TERM -- "-$_cmd_pid" 2>/dev/null || true + sleep 1 + kill -KILL -- "-$_cmd_pid" 2>/dev/null || true + wait "$_cmd_pid" 2>/dev/null + return 124 + fi + + kill "$_timer_pid" 2>/dev/null + wait "$_timer_pid" 2>/dev/null + wait "$_cmd_pid" + _rc=$? + # A readiness command must never daemonize work. Reap any process that + # inherited its group even when the direct command exited successfully. + if kill -0 -- "-$_cmd_pid" 2>/dev/null; then + kill -TERM -- "-$_cmd_pid" 2>/dev/null || true + sleep 0.05 + kill -KILL -- "-$_cmd_pid" 2>/dev/null || true + fi + return "$_rc" +} + +valid_source_id() { + [ "${#1}" -le 32 ] || return 1 + printf '%s\n' "$1" | grep -Eq '^[a-z0-9]$|^[a-z0-9][a-z0-9-]{0,30}[a-z0-9]$' +} + +[ -n "$REPO_ROOT" ] || degraded "not_a_git_worktree" +command -v gbrain >/dev/null 2>&1 || degraded "gbrain_unavailable" +command -v jq >/dev/null 2>&1 || degraded "jq_unavailable" +[ -f "$REPO_ROOT/.gbrain-source" ] || degraded "source_pin_missing" + +SOURCE_ID=$(tr -d '[:space:]' < "$REPO_ROOT/.gbrain-source" 2>/dev/null) +[ -n "$SOURCE_ID" ] || degraded "source_pin_empty" +valid_source_id "$SOURCE_ID" || degraded "source_pin_invalid" + +HEAD_SHA=$(git -C "$REPO_ROOT" rev-parse HEAD 2>/dev/null) +TRACKED_DIFF=$(git -C "$REPO_ROOT" diff --no-ext-diff --binary HEAD 2>/dev/null | shasum | awk '{print $1}') +UNTRACKED_CODE_HASH=$( + git -C "$REPO_ROOT" ls-files --others --exclude-standard -z 2>/dev/null | + while IFS= read -r -d '' _path; do + if printf '%s\n' "$_path" | grep -Eq '\.(js|jsx|mjs|cjs|ts|tsx|py|rb|go|rs|java|kt|kts|swift|c|cc|cpp|h|hpp)$'; then + printf '%s\n' "$_path" + shasum < "$REPO_ROOT/$_path" 2>/dev/null + fi + done | shasum | awk '{print $1}' +) +FINGERPRINT=$(printf '%s\n%s\n%s\n%s\n%s\n' "$READINESS_SCHEMA" "$SOURCE_ID" "$HEAD_SHA" "$TRACKED_DIFF" "$UNTRACKED_CODE_HASH" | shasum | awk '{print $1}') + +# Validate the source and its path before SOURCE_ID participates in any state +# path. `sources list`, like all GBrain probes below, is strictly bounded. +SOURCES_JSON=$(cd "$REPO_ROOT" && run_bounded "$PROBE_TIMEOUT_SECONDS" gbrain sources list --json 2>/dev/null) +[ $? -eq 0 ] && [ -n "$SOURCES_JSON" ] || degraded "sources_probe_failed_or_timed_out" +SOURCE_PATH=$(printf '%s' "$SOURCES_JSON" | jq -r --arg id "$SOURCE_ID" '.sources[]? | select(.id == $id) | .local_path // empty' 2>/dev/null) +[ -n "$SOURCE_PATH" ] || degraded "source_not_registered" +SOURCE_REAL=$(cd "$SOURCE_PATH" 2>/dev/null && pwd -P) +REPO_REAL=$(cd "$REPO_ROOT" 2>/dev/null && pwd -P) +[ "$SOURCE_REAL" = "$REPO_REAL" ] || degraded "source_path_mismatch" + +mkdir -p "$STATE_ROOT" 2>/dev/null +CACHE_FILE="$STATE_ROOT/${SOURCE_ID}-${FINGERPRINT}.partial" +REPO_KEY=$(git -C "$REPO_ROOT" remote get-url origin 2>/dev/null | shasum | awk '{print $1}') +CANARY_FILE="$STATE_ROOT/repo-${REPO_KEY}.canary" +CANARY=$(head -1 "$CACHE_FILE" 2>/dev/null | tr -cd 'A-Za-z0-9_$') +[ -n "$CANARY" ] || CANARY=$(head -1 "$CANARY_FILE" 2>/dev/null | tr -cd 'A-Za-z0-9_$') + +check_canary() { + _symbol="$1" + [ -n "$_symbol" ] || return 1 + + _def=$(cd "$REPO_ROOT" && run_bounded "$PROBE_TIMEOUT_SECONDS" gbrain code-def "$_symbol" --json 2>/dev/null) + [ $? -eq 0 ] || return 1 + [ "$(printf '%s' "$_def" | jq -r '.ready == true and .count == 1' 2>/dev/null)" = "true" ] || return 1 + + _callers=$(cd "$REPO_ROOT" && run_bounded "$PROBE_TIMEOUT_SECONDS" gbrain code-callers "$_symbol" --json 2>/dev/null) + [ $? -eq 0 ] || return 1 + [ "$(printf '%s' "$_callers" | jq -r '.ready == true and .count > 0' 2>/dev/null)" = "true" ] || return 1 + + _init=$(jq -nc '{jsonrpc:"2.0",id:1,method:"initialize",params:{protocolVersion:"2025-03-26",capabilities:{},clientInfo:{name:"gstack-graph-canary",version:"1"}}}') + _ready=$(jq -nc '{jsonrpc:"2.0",method:"notifications/initialized",params:{}}') + _call=$(jq -nc --arg symbol "$_symbol" --arg source "$SOURCE_ID" '{jsonrpc:"2.0",id:2,method:"tools/call",params:{name:"code_blast",arguments:{symbol:$symbol,source_id:$source,depth:1,max_nodes:1}}}') + _blast_raw=$(cd "$REPO_ROOT" && { printf '%s\n' "$_init" "$_ready" "$_call"; } | run_bounded "$PROBE_TIMEOUT_SECONDS" gbrain serve --surface full 2>/dev/null) + [ $? -eq 0 ] || return 1 + _blast=$(printf '%s' "$_blast_raw" | jq -r 'select(.id == 2) | .result.content[0].text' 2>/dev/null | jq -c '.' 2>/dev/null) + [ "$(printf '%s' "$_blast" | jq -r '.ready == true and .result == "ok" and ([.depth_groups[]?.nodes[]?] | length) > 0' 2>/dev/null)" = "true" ] || return 1 + return 0 +} + +# A prior canary is a hint, never authority. Re-prove it against the live graph. +if [ -n "$CANARY" ] && check_canary "$CANARY"; then + printf '%s\n' "$CANARY" > "$CACHE_FILE" + if [ -n "$SESSION_KEY" ]; then + printf '%s\n' "$CANARY" > "$STATE_ROOT/session-${SESSION_KEY}-${FINGERPRINT}.partial" + fi + echo "GBRAIN_GRAPH: partial | source=$SOURCE_ID | canary=$CANARY | cache=hit" + exit 0 +fi + +# Cold discovery is local and deliberately limited to one likely symbol. That +# keeps the total worst-case foreground budget bounded (sources + one canary), +# while `/sync-gbrain --dream` remains the explicit repair when it cannot prove +# a graph. +CANARY=$( + git -C "$REPO_ROOT" grep -h -oE 'export (async )?function [A-Za-z_$][A-Za-z0-9_$]*' -- '*.ts' '*.tsx' 2>/dev/null | + awk '{print $NF}' | sort -u | head -1 +) +if [ -n "$CANARY" ] && check_canary "$CANARY"; then + printf '%s\n' "$CANARY" > "$CANARY_FILE" + printf '%s\n' "$CANARY" > "$CACHE_FILE" + if [ -n "$SESSION_KEY" ]; then + printf '%s\n' "$CANARY" > "$STATE_ROOT/session-${SESSION_KEY}-${FINGERPRINT}.partial" + fi + echo "GBRAIN_GRAPH: partial | source=$SOURCE_ID | canary=$CANARY | cache=miss" + exit 0 +fi + +rm -f "$CACHE_FILE" 2>/dev/null +degraded "no_graph_canary_run_sync_gbrain" diff --git a/bin/gstack-gbrain-sync.ts b/bin/gstack-gbrain-sync.ts index 4e3034b3a7..5f38fa41c0 100644 --- a/bin/gstack-gbrain-sync.ts +++ b/bin/gstack-gbrain-sync.ts @@ -904,8 +904,8 @@ async function runCodeImport(args: CliArgs): Promise { ok: true, duration_ms: 0, summary: pinnedSourceId - ? `would: gbrain sync --strategy code --source ${sourceId}; gbrain sources attach ${sourceId}` - : `would: gbrain sources add ${sourceId} --path ${root} --federated; gbrain sync --strategy code --source ${sourceId}; gbrain sources attach ${sourceId}`, + ? `would: gbrain sources set-strategy ${sourceId} code; gbrain sync --strategy code --source ${sourceId}; gbrain sources attach ${sourceId}` + : `would: gbrain sources add ${sourceId} --path ${root} --federated; gbrain sources set-strategy ${sourceId} code; gbrain sync --strategy code --source ${sourceId}; gbrain sources attach ${sourceId}`, detail: { source_id: sourceId, source_path: root, status: "skipped" }, }; } @@ -984,6 +984,27 @@ async function runCodeImport(args: CliArgs): Promise { } } + // Persist the source's purpose, not just this invocation's override. + // Autopilot and `sync --all` read sources.config.strategy and otherwise + // fall back to markdown, which can silently replace a valid code index on + // their next full walk. A one-shot `--strategy code` override cannot make + // that durable, so failure here must fail the stage closed. + const strategyResult = spawnGbrain(["sources", "set-strategy", sourceId, "code"], { + stdio: ["ignore", "ignore", "pipe"], + timeout: 10_000, + baseEnv: gbrainEnv, + }); + if (strategyResult.status !== 0) { + return { + name: "code", + ran: true, + ok: false, + duration_ms: Date.now() - t0, + summary: `gbrain sources set-strategy ${sourceId} code exited ${strategyResult.status}`, + detail: { source_id: sourceId, source_path: root, status: "failed" }, + }; + } + // Step 2: Always run the page-creating file walk first, then (for --full) // a full re-embed. // @@ -1391,8 +1412,8 @@ export async function runDream(args: CliArgs): Promise { ok: true, duration_ms: 0, summary: sourceId - ? `would: gbrain dream --source ${sourceId} (build this source's call graph)` - : "would: gbrain dream (call-graph build)", + ? `would: gbrain dream --source ${sourceId} --phase resolve_symbol_edges (build this source's call graph)` + : "would: gbrain dream --phase resolve_symbol_edges (call-graph build)", }; } @@ -1423,17 +1444,16 @@ export async function runDream(args: CliArgs): Promise { DEFAULT_DREAM_TIMEOUT_MS, ); - // Scope the cycle to THIS worktree's code source: `gbrain dream --source `. - // Verified empirically (not just from `gbrain --help`): plain `gbrain dream` - // cycles the brain's default source and never runs the source-scoped `extract` - // phase for our code source, so the call graph for the pinned source stays - // empty. `gbrain dream --source ` runs the per-source cycle (the form - // `gbrain doctor` recommends for stale sources) and is what actually populates - // code-callers/code-callees for this worktree. Falls back to plain `dream` - // only when we can't derive the source id (not in a git repo). + // Explicitly request the global resolver phase. Current gbrain intentionally + // excludes global phases from an implicit non-default source cycle, so bare + // `dream --source ` can exit 0 without ever resolving symbol edges. + // Keeping --source still binds the receipt to this worktree; --phase is the + // load-bearing proof that the call-graph phase actually ran. const root = repoRoot(); const sourceId = root ? resolveCodeSourceId(root, gbrainEnv) : null; - const dreamArgs = sourceId ? ["dream", "--source", sourceId] : ["dream"]; + const dreamArgs = sourceId + ? ["dream", "--source", sourceId, "--phase", "resolve_symbol_edges"] + : ["dream", "--phase", "resolve_symbol_edges"]; // spawnGbrain seeds DATABASE_URL from gbrain's config via buildGbrainEnv. // @@ -1587,6 +1607,9 @@ export function classifyDreamOutcome(out: string): string | null { if (parseResolvedEdges(out) === 0) { return "dream ran but resolved 0 call-graph edges (no code symbols matched for this source yet)."; } + if (parseResolvedEdges(out) === null) { + return "dream exited successfully but did not prove resolve_symbol_edges completed; call-graph readiness is unknown."; + } return null; } diff --git a/bin/gstack-patch-names b/bin/gstack-patch-names index bef02aae4c..08f9c98be0 100755 --- a/bin/gstack-patch-names +++ b/bin/gstack-patch-names @@ -6,6 +6,16 @@ set -euo pipefail GSTACK_DIR="$1" DO_PREFIX="$2" +# Canonical checkouts are generated source, not an install target. Refuse to +# rewrite tracked SKILL.md files even if a future caller accidentally points +# this legacy helper at the repository root; setup/relink create and patch +# consumer copies instead. +TRACKED_SKILLS=$(git -C "$GSTACK_DIR" ls-files 'SKILL.md' '*/SKILL.md' 2>/dev/null || true) +if [ -n "$TRACKED_SKILLS" ]; then + echo "Error: refusing to patch tracked canonical SKILL.md files; run gstack-relink to refresh installed copies." >&2 + exit 2 +fi + # Normalize prefix arg case "$DO_PREFIX" in true|1) DO_PREFIX=1 ;; *) DO_PREFIX=0 ;; esac diff --git a/bin/gstack-relink b/bin/gstack-relink index 7a3b4b0e6e..f79a0aca55 100755 --- a/bin/gstack-relink +++ b/bin/gstack-relink @@ -41,32 +41,272 @@ PREFIX=$("$GSTACK_CONFIG" get skill_prefix 2>/dev/null || echo "false") # skill, relink serves it — otherwise a config change would silently flip # every skill back to the canonical (blockless) source. RENDER_DIR="${GSTACK_USER_RENDER_DIR:-${GSTACK_HOME:-$HOME/.gstack}/render/claude}" +COLLISIONS=0 -# Helper: remove old skill entry (symlink or real directory with symlinked SKILL.md) -_cleanup_skill_entry() { +_gstack_upgrade_base_sha() { + local candidate resolved + + candidate="${GSTACK_UPGRADE_FROM_HEAD:-}" + case "$candidate" in + ''|*[!0-9a-fA-F]*) candidate="" ;; + esac + if [ -n "$candidate" ]; then + resolved=$(git -C "$INSTALL_DIR" rev-parse --verify "${candidate}^{commit}" 2>/dev/null || true) + if [ -n "$resolved" ]; then + printf '%s\n' "$resolved" + return 0 + fi + fi + + resolved=$(git -C "$INSTALL_DIR" rev-parse --verify 'HEAD@{1}^{commit}' 2>/dev/null || true) + [ -n "$resolved" ] || return 1 + printf '%s\n' "$resolved" +} + +_skill_file_matches_source() { + local installed_file="$1" + local source_file="$2" + local served_name="$3" + local rewrite_name="$4" + local expected_file result + + [ -f "$installed_file" ] && [ -f "$source_file" ] || return 1 + expected_file=$(mktemp "${TMPDIR:-/tmp}/gstack-expected-skill.XXXXXX") || return 1 + if [ "$rewrite_name" -eq 1 ]; then + sed "1,/^---\$/ s/^name:[[:space:]].*/name: $served_name/" "$source_file" > "$expected_file" + else + cp "$source_file" "$expected_file" + fi + cmp -s "$expected_file" "$installed_file" + result=$? + rm -f "$expected_file" + return "$result" +} + +# The release immediately before consumer provenance markers wrote exact +# rewritten copies. Recognize that legacy shape byte-for-byte against either +# the current source or the exact pre-upgrade commit only when refreshing the +# same target. Cleanup never uses this path. +_legacy_skill_copy_matches() { + local entry="$1" + local source_name="$2" + local served_name="$3" + local source_file rewrite_name upgrade_sha prior_file result + + [ -d "$entry" ] && [ -f "$entry/SKILL.md" ] && [ ! -L "$entry/SKILL.md" ] || return 1 + source_file="$INSTALL_DIR/$source_name/SKILL.md" + [ -f "$RENDER_DIR/$source_name/SKILL.md" ] && source_file="$RENDER_DIR/$source_name/SKILL.md" + [ -f "$source_file" ] || return 1 + + rewrite_name=0 + if [ "$source_name" = "$served_name" ]; then + rewrite_name=0 + else + rewrite_name=1 + fi + + _skill_file_matches_source "$entry/SKILL.md" "$source_file" "$served_name" "$rewrite_name" && return 0 + + upgrade_sha=$(_gstack_upgrade_base_sha 2>/dev/null || true) + [ -n "$upgrade_sha" ] || return 1 + prior_file=$(mktemp "${TMPDIR:-/tmp}/gstack-prior-skill.XXXXXX") || return 1 + if ! git -C "$INSTALL_DIR" show "$upgrade_sha:$source_name/SKILL.md" > "$prior_file" 2>/dev/null; then + rm -f "$prior_file" + return 1 + fi + _skill_file_matches_source "$entry/SKILL.md" "$prior_file" "$served_name" "$rewrite_name" + result=$? + rm -f "$prior_file" + return "$result" +} + +# A real consumer directory is ours only when its provenance is exact. A +# reserved `gstack-*` name plus matching frontmatter is not ownership: users +# are allowed to create their own namespaced skills. Unix flat installs are +# proven by an exact symlink into this checkout/render; rewritten copies carry +# a source+served marker generated below. +_skill_entry_owned() { local entry="$1" + local source_name="$2" + local served_name="$3" + local allow_legacy="${4:-0}" + local link_dest marker + + [ -e "$entry" ] || [ -L "$entry" ] || return 1 if [ -L "$entry" ]; then - rm -f "$entry" - elif [ -d "$entry" ] && [ -L "$entry/SKILL.md" ]; then + link_dest=$(readlink "$entry" 2>/dev/null || true) + [ "$link_dest" = "$INSTALL_DIR/$source_name" ] && return 0 + return 1 + fi + [ -d "$entry" ] || return 1 + if [ -L "$entry/SKILL.md" ]; then + link_dest=$(readlink "$entry/SKILL.md" 2>/dev/null || true) + [ "$link_dest" = "$INSTALL_DIR/$source_name/SKILL.md" ] && return 0 + [ "$link_dest" = "$RENDER_DIR/$source_name/SKILL.md" ] && return 0 + return 1 + fi + [ -f "$entry/SKILL.md" ] || return 1 + marker="" + grep -Fqx "$marker" "$entry/SKILL.md" 2>/dev/null && return 0 + [ "$allow_legacy" -eq 1 ] && _legacy_skill_copy_matches "$entry" "$source_name" "$served_name" +} + +_report_collision() { + echo "Error: refusing to replace non-gstack skill entry: $1" >&2 + COLLISIONS=1 +} + +_cleanup_skill_entry() { + local entry="$1" + local source_name="$2" + local served_name="$3" + if [ ! -e "$entry" ] && [ ! -L "$entry" ]; then + return 0 + fi + if _skill_entry_owned "$entry" "$source_name" "$served_name"; then rm -rf "$entry" + else + _report_collision "$entry" + fi +} + +# Install the frontmatter that Claude actually serves. Canonical SKILL.md files +# in INSTALL_DIR are generated source artifacts and must stay byte-identical to +# their templates. Prefix mode therefore writes a consumer copy inside the +# installed skill directory; flat mode can keep the cheaper source symlink. +_install_served_skill_md() { + local source="$1" + local target_dir="$2" + local served_name="$3" + local source_name="$4" + local tmp_file + tmp_file=$(mktemp "$target_dir/.gstack-skill.XXXXXX") || return 1 + if [ "$PREFIX" = "true" ]; then + if ! sed "1,/^---\$/ s/^name:[[:space:]].*/name: $served_name/" "$source" > "$tmp_file" \ + || ! printf '\n\n' "$source_name" "$served_name" >> "$tmp_file"; then + rm -f "$tmp_file" + return 1 + fi + else + rm -f "$tmp_file" + if ! ln -s "$source" "$tmp_file"; then + rm -f "$tmp_file" + return 1 + fi + fi + # The previously served file/link remains valid until this same-directory + # rename commits the complete replacement. A failed render or interrupted + # relink can therefore never leave the skill entry empty. + mv -f "$tmp_file" "$target_dir/SKILL.md" +} + +_root_alias_owned() { + local target="$1" + local marker link_dest upgrade_sha prior_file result + + [ -e "$target" ] || [ -L "$target" ] || return 1 + if [ -L "$target" ]; then + link_dest=$(readlink "$target" 2>/dev/null || true) + [ "$link_dest" = "$INSTALL_DIR" ] + return $? + fi + [ -d "$target" ] || return 1 + if [ -L "$target/SKILL.md" ]; then + link_dest=$(readlink "$target/SKILL.md" 2>/dev/null || true) + [ "$link_dest" = "$INSTALL_DIR/SKILL.md" ] + return $? + fi + [ -f "$target/SKILL.md" ] || return 1 + + marker='' + grep -Fqx "$marker" "$target/SKILL.md" 2>/dev/null && return 0 + _skill_file_matches_source "$target/SKILL.md" "$INSTALL_DIR/SKILL.md" "_gstack-command" 1 && return 0 + + upgrade_sha=$(_gstack_upgrade_base_sha 2>/dev/null || true) + [ -n "$upgrade_sha" ] || return 1 + prior_file=$(mktemp "${TMPDIR:-/tmp}/gstack-prior-root-alias.XXXXXX") || return 1 + if ! git -C "$INSTALL_DIR" show "$upgrade_sha:SKILL.md" > "$prior_file" 2>/dev/null; then + rm -f "$prior_file" + return 1 fi + _skill_file_matches_source "$target/SKILL.md" "$prior_file" "_gstack-command" 1 + result=$? + rm -f "$prior_file" + return "$result" } _link_root_skill_alias() { local target="$SKILLS_DIR/_gstack-command" + local marker expected_file [ -f "$INSTALL_DIR/SKILL.md" ] || return 0 - [ -L "$target" ] && rm -f "$target" + marker='' + if [ -e "$target" ] || [ -L "$target" ]; then + if ! _root_alias_owned "$target"; then + _report_collision "$target" + return 0 + fi + [ -L "$target" ] && rm -f "$target" + fi mkdir -p "$target" + expected_file=$(mktemp "$target/.gstack-root-alias.XXXXXX") + sed "1,/^---\$/ s/^name:[[:space:]].*/name: _gstack-command/" "$INSTALL_DIR/SKILL.md" > "$expected_file" # Copy-then-rewrite, never a symlink (#2511): a symlinked alias re-serves # the canonical `name: gstack`, Claude Code sees a duplicate skill name, # and drops the ENTIRE personal-skills set. sed reads the source and writes - # a fresh copy — remove any prior symlink first so the redirect can never - # write through it into the generated source. - rm -f "$target/SKILL.md" - sed "1,/^---\$/ s/^name:[[:space:]].*/name: _gstack-command/" "$INSTALL_DIR/SKILL.md" > "$target/SKILL.md" + # a fresh copy. The same-directory rename replaces any prior symlink itself; + # it never follows that symlink into the generated source. + printf '\n%s\n' "$marker" >> "$expected_file" + mv -f "$expected_file" "$target/SKILL.md" } +# Preflight every destination and every opposite-mode cleanup candidate before +# changing anything. A failed mode switch must leave the old working install +# and the user's colliding destination byte-for-byte intact. +if [ -f "$INSTALL_DIR/SKILL.md" ] \ + && { [ -e "$SKILLS_DIR/_gstack-command" ] || [ -L "$SKILLS_DIR/_gstack-command" ]; } \ + && ! _root_alias_owned "$SKILLS_DIR/_gstack-command"; then + _report_collision "$SKILLS_DIR/_gstack-command" +fi + +for skill_dir in "$INSTALL_DIR"/*/; do + [ -d "$skill_dir" ] || continue + [ -L "${skill_dir%/}" ] && continue + skill=$(basename "$skill_dir") + case "$skill" in bin|browse|design|docs|extension|lib|node_modules|scripts|test|.git|.github) continue ;; esac + [ -f "$skill_dir/SKILL.md" ] || continue + + cleanup_target="" + if [ "$PREFIX" = "true" ]; then + case "$skill" in + gstack-*) link_name="$skill" ;; + *) link_name="gstack-$skill"; cleanup_target="$SKILLS_DIR/$skill" ;; + esac + else + link_name="$skill" + case "$skill" in + gstack-*) ;; + *) cleanup_target="$SKILLS_DIR/gstack-$skill" ;; + esac + fi + + target="$SKILLS_DIR/$link_name" + if { [ -e "$target" ] || [ -L "$target" ]; } \ + && ! _skill_entry_owned "$target" "$skill" "$link_name" 1; then + _report_collision "$target" + fi + if [ -n "$cleanup_target" ] \ + && { [ -e "$cleanup_target" ] || [ -L "$cleanup_target" ]; } \ + && ! _skill_entry_owned "$cleanup_target" "$skill" "$(basename "$cleanup_target")" 0; then + _report_collision "$cleanup_target" + fi +done + +if [ "$COLLISIONS" -ne 0 ]; then + echo "Resolve the reported skill-name collisions, then run gstack-relink again." >&2 + exit 2 +fi + _link_root_skill_alias # Discover skills (directories with SKILL.md, excluding meta dirs) @@ -90,37 +330,39 @@ for skill_dir in "$INSTALL_DIR"/*/; do *) link_name="gstack-$skill" ;; esac # Remove old flat entry if it exists (and isn't the same as the new link) - [ "$link_name" != "$skill" ] && _cleanup_skill_entry "$SKILLS_DIR/$skill" + [ "$link_name" != "$skill" ] && _cleanup_skill_entry "$SKILLS_DIR/$skill" "$skill" "$skill" else link_name="$skill" # Don't remove gstack-* dirs that are their real name (e.g., gstack-upgrade) case "$skill" in gstack-*) ;; # Already the real name, no old prefixed link to clean - *) _cleanup_skill_entry "$SKILLS_DIR/gstack-$skill" ;; + *) _cleanup_skill_entry "$SKILLS_DIR/gstack-$skill" "$skill" "gstack-$skill" ;; esac fi target="$SKILLS_DIR/$link_name" - # Upgrade old directory symlinks to real directories - [ -L "$target" ] && rm -f "$target" - # Create real directory with symlinked SKILL.md (absolute path) + if [ -e "$target" ] || [ -L "$target" ]; then + if _skill_entry_owned "$target" "$skill" "$link_name" 1; then + [ -L "$target" ] && rm -f "$target" + else + _report_collision "$target" + continue + fi + fi + # Create a real top-level skill directory. mkdir -p "$target" skill_md_src="$INSTALL_DIR/$skill/SKILL.md" [ -f "$RENDER_DIR/$skill/SKILL.md" ] && skill_md_src="$RENDER_DIR/$skill/SKILL.md" - ln -snf "$skill_md_src" "$target/SKILL.md" + _install_served_skill_md "$skill_md_src" "$target" "$link_name" "$skill" SKILL_COUNT=$((SKILL_COUNT + 1)) done -# Patch SKILL.md name: fields to match prefix setting. When a gbrain render -# is active the loop above links SKILL.md from RENDER_DIR — the file the host -# actually serves — so patch THAT tree too or skill_prefix=true is a no-op -# for every brain-aware skill (#2738). gstack-patch-names takes an arbitrary -# root, skips already-prefixed names (idempotent), and the render dir is -# user-owned and untracked, so patching it never dirties a checkout. -"$INSTALL_DIR/bin/gstack-patch-names" "$INSTALL_DIR" "$PREFIX" -[ -d "$RENDER_DIR" ] && "$INSTALL_DIR/bin/gstack-patch-names" "$RENDER_DIR" "$PREFIX" - if [ "$PREFIX" = "true" ]; then echo "Relinked $SKILL_COUNT skills as gstack-*" else echo "Relinked $SKILL_COUNT skills as flat names" fi + +if [ "$COLLISIONS" -ne 0 ]; then + echo "Resolve the reported skill-name collisions, then run gstack-relink again." >&2 + exit 2 +fi diff --git a/bin/gstack-session-update b/bin/gstack-session-update index 15f8caa872..510da21bfc 100755 --- a/bin/gstack-session-update +++ b/bin/gstack-session-update @@ -16,6 +16,7 @@ STATE_DIR="${GSTACK_STATE_DIR:-$HOME/.gstack}" # never block a session over a receipt hiccup. . "$(cd "$(dirname "$0")" && pwd)/gstack-egress-lib.sh" THROTTLE_FILE="$STATE_DIR/.last-session-update" +PENDING_SETUP_FILE="$STATE_DIR/.pending-session-setup" LOCK_DIR="$STATE_DIR/.setup-lock" LOG_FILE="$STATE_DIR/analytics/session-update.log" THROTTLE_SECONDS=3600 # 1 hour @@ -37,7 +38,10 @@ if [ "$AUTO" != "true" ]; then fi # ── Throttle: skip if checked recently ── -if [ -f "$THROTTLE_FILE" ]; then +# A pending setup is an incomplete upgrade, not a routine update check. It +# deliberately bypasses this throttle so the next session retries as soon as +# the collision/environmental failure has been resolved. +if [ -f "$THROTTLE_FILE" ] && [ ! -f "$PENDING_SETUP_FILE" ]; then LAST=$(cat "$THROTTLE_FILE" 2>/dev/null || echo 0) NOW=$(date +%s) ELAPSED=$(( NOW - LAST )) @@ -129,17 +133,55 @@ fi # ── Pull latest ── OLD_HEAD=$(git -C "$GSTACK_DIR" rev-parse HEAD 2>/dev/null) + + # A background session hook is not an owner of operator edits. Never invoke + # pull/autostash on a dirty checkout: an autostash apply conflict leaves a + # half-merged tree, and attempting to "recover" it with checkout + stash + # drop destroys the only recoverable copy. Fail closed with HEAD, worktree, + # index, and every existing stash/ref byte-for-byte untouched. + if [ -n "$(git -C "$GSTACK_DIR" status --porcelain --untracked-files=normal 2>/dev/null)" ]; then + date +%s > "$THROTTLE_FILE" 2>/dev/null + log_entry "SKIP dirty_worktree head=${OLD_HEAD:-unknown}" + exit 0 + fi + + # Persist upgrade intent BEFORE git can move HEAD. If this process crashes + # after the fast-forward but before/during setup, the next session still has + # the exact pre-upgrade revision needed to prove legacy generated copies. + # Never overwrite an older pending base: setup may have failed at A -> B and + # the source may advance again to C before the collision is repaired. + HAD_PENDING_SETUP=0 + PENDING_SETUP_FROM="" + if [ -f "$PENDING_SETUP_FILE" ]; then + HAD_PENDING_SETUP=1 + PENDING_SETUP_FROM=$(sed -n 's/^from=//p' "$PENDING_SETUP_FILE" 2>/dev/null | head -n 1) + case "$PENDING_SETUP_FROM" in + ''|*[!0-9a-fA-F]*) PENDING_SETUP_FROM="" ;; + esac + if [ -z "$PENDING_SETUP_FROM" ] \ + || ! git -C "$GSTACK_DIR" rev-parse --verify "${PENDING_SETUP_FROM}^{commit}" >/dev/null 2>&1 \ + || ! git -C "$GSTACK_DIR" merge-base --is-ancestor "$PENDING_SETUP_FROM" "$OLD_HEAD" >/dev/null 2>&1; then + log_entry "SETUP_PENDING_INVALID" + exit 0 + fi + else + PENDING_SETUP_FROM="$OLD_HEAD" + PENDING_SETUP_TMP=$(mktemp "$STATE_DIR/.pending-session-setup.XXXXXX" 2>/dev/null || echo "") + if [ -z "$PENDING_SETUP_TMP" ] \ + || ! printf 'from=%s\n' "$PENDING_SETUP_FROM" > "$PENDING_SETUP_TMP" 2>/dev/null \ + || ! mv -f "$PENDING_SETUP_TMP" "$PENDING_SETUP_FILE" 2>/dev/null; then + [ -n "$PENDING_SETUP_TMP" ] && rm -f "$PENDING_SETUP_TMP" 2>/dev/null + log_entry "SETUP_PENDING_WRITE_FAILED head=${OLD_HEAD:-unknown}" + exit 0 + fi + fi + UPDATE_URL=$(git -C "$GSTACK_DIR" remote get-url origin 2>/dev/null || echo "") UPDATE_HOST="${UPDATE_URL#*://}"; UPDATE_HOST="${UPDATE_HOST#*@}"; UPDATE_HOST="${UPDATE_HOST%%[/:]*}" - # --autostash: locally-patched TRACKED files are the NORM on installs, not - # the exception — skill-prefix mode rewrites frontmatter names and - # `gstack-config gbrain-refresh` renders brain blocks into SKILL.md. A bare - # --ff-only refuses over those edits, so auto-upgrade wedged permanently - # (observed: 308 consecutive PULL_FAILED with the reason discarded, #2566). # Capture stderr: the log must carry WHY a pull failed, never just the code. PULL_ERR_FILE=$(mktemp "${TMPDIR:-/tmp}/gstack-session-pull-XXXXXX" 2>/dev/null || echo "") GSTACK_HOME="$STATE_DIR" _receipted_git open session-update "${UPDATE_HOST:-unknown}" gstack-self-update-pull "auto_upgrade=true" \ - bash -c 'git -C "$1" pull --ff-only --autostash -q 2>"${2:-/dev/null}"' _ "$GSTACK_DIR" "$PULL_ERR_FILE" + bash -c 'git -C "$1" pull --ff-only -q 2>"${2:-/dev/null}"' _ "$GSTACK_DIR" "$PULL_ERR_FILE" PULL_EXIT=$? NEW_HEAD=$(git -C "$GSTACK_DIR" rev-parse HEAD 2>/dev/null) @@ -152,52 +194,77 @@ fi if [ "$PULL_EXIT" -ne 0 ]; then PULL_REASON=$(head -c 300 "$PULL_ERR_FILE" 2>/dev/null | tr '\n' ' ' | tr -s ' ') log_entry "PULL_FAILED exit=$PULL_EXIT reason=${PULL_REASON:-unknown}" - # Autostash pop conflict leaves the stash behind and the tree half-merged. - # The local patches are REGENERABLE (prefix renames, gbrain blocks), so - # recover to a clean upstream tree and re-render them below rather than - # leaving conflict markers in a live install. - if grep -qi "autostash" "$PULL_ERR_FILE" 2>/dev/null; then - git -C "$GSTACK_DIR" checkout -q -- . 2>/dev/null - git -C "$GSTACK_DIR" stash drop -q 2>/dev/null - log_entry "AUTOSTASH_CONFLICT_RECOVERED tree_reset=1" - _PREFIX_CFG=$("$GSTACK_DIR/bin/gstack-config" get skill_prefix 2>/dev/null || echo false) - "$GSTACK_DIR/bin/gstack-patch-names" "$GSTACK_DIR" "$_PREFIX_CFG" >/dev/null 2>&1 || true - "$GSTACK_DIR/bin/gstack-config" gbrain-refresh >/dev/null 2>&1 || true - fi rm -f "$PULL_ERR_FILE" 2>/dev/null + # A marker created solely for this unchanged failed pull carries no + # incomplete migration. Preserve pre-existing markers and any marker whose + # pull moved HEAD despite returning non-zero. + if [ "$HAD_PENDING_SETUP" -eq 0 ] && [ "$OLD_HEAD" = "$NEW_HEAD" ]; then + rm -f "$PENDING_SETUP_FILE" 2>/dev/null + fi exit 0 fi rm -f "$PULL_ERR_FILE" 2>/dev/null - # Re-render local patches over the fresh tree (both tools are idempotent - # no-ops when the feature is unconfigured); the autostash pop usually - # preserves them, but a clean re-render costs nothing and self-heals. - _PREFIX_CFG=$("$GSTACK_DIR/bin/gstack-config" get skill_prefix 2>/dev/null || echo false) - "$GSTACK_DIR/bin/gstack-patch-names" "$GSTACK_DIR" "$_PREFIX_CFG" >/dev/null 2>&1 || true - "$GSTACK_DIR/bin/gstack-config" gbrain-refresh >/dev/null 2>&1 || true + # ── If HEAD moved or an earlier setup is pending, run setup -q ── + if [ "$OLD_HEAD" != "$NEW_HEAD" ] || [ "$HAD_PENDING_SETUP" -eq 1 ]; then + if [ "$OLD_HEAD" != "$NEW_HEAD" ]; then + log_entry "UPDATING old=$OLD_HEAD new=$NEW_HEAD" + else + log_entry "SETUP_RETRY from=$PENDING_SETUP_FROM head=$NEW_HEAD" + fi - # ── If HEAD moved, run setup -q ── - if [ "$OLD_HEAD" != "$NEW_HEAD" ]; then - log_entry "UPDATING old=$OLD_HEAD new=$NEW_HEAD" + # Do not let a stale success marker claim that an upgrade completed while + # its consumer/runtime migration is still pending. + rm -f "$STATE_DIR/just-upgraded-from" 2>/dev/null # bun must be available for setup if command -v bun >/dev/null 2>&1; then - ( cd "$GSTACK_DIR" && ./setup -q ) >/dev/null 2>&1 || { - log_entry "SETUP_FAILED" - } - # Heartbeat: setup done (either way) — refresh the TTL clock. + # Keep the exact pre-pull revision available to setup's one-time legacy + # ownership migration. Setup refreshes brain-aware renders only after it + # has adopted the prior consumer copies, so a source change cannot turn + # a legitimate first upgrade into a false collision. + if ( cd "$GSTACK_DIR" && GSTACK_UPGRADE_FROM_HEAD="$PENDING_SETUP_FROM" ./setup -q ) >/dev/null 2>&1; then + SETUP_OK=1 + else + SETUP_EXIT=$? + SETUP_OK=0 + log_entry "SETUP_FAILED exit=$SETUP_EXIT from=$PENDING_SETUP_FROM head=$NEW_HEAD" + fi + # Heartbeat: setup done — refresh the TTL clock. touch "$LOCK_DIR/pid" 2>/dev/null else - log_entry "SETUP_SKIPPED bun_missing" + SETUP_OK=0 + log_entry "SETUP_DEFERRED bun_missing from=$PENDING_SETUP_FROM head=$NEW_HEAD" + fi + + if [ "$SETUP_OK" -ne 1 ]; then + # PENDING_SETUP_FILE intentionally survives. Its presence bypasses the + # throttle and forces a retry even when the next pull leaves HEAD equal. + exit 0 + fi + if ! rm -f "$PENDING_SETUP_FILE" 2>/dev/null; then + log_entry "SETUP_PENDING_CLEAR_FAILED from=$PENDING_SETUP_FROM head=$NEW_HEAD" + exit 0 fi # Write marker so next skill preamble shows "just upgraded" - OLD_VER=$(git -C "$GSTACK_DIR" show "$OLD_HEAD:VERSION" 2>/dev/null || echo "unknown") + OLD_VER=$(git -C "$GSTACK_DIR" show "$PENDING_SETUP_FROM:VERSION" 2>/dev/null || echo "unknown") echo "$OLD_VER" > "$STATE_DIR/just-upgraded-from" 2>/dev/null rm -f "$STATE_DIR/last-update-check" 2>/dev/null rm -f "$STATE_DIR/update-snoozed" 2>/dev/null - log_entry "UPDATED from=$OLD_VER to=$(cat "$GSTACK_DIR/VERSION" 2>/dev/null || echo unknown)" + if [ "$HAD_PENDING_SETUP" -eq 1 ] && [ "$OLD_HEAD" = "$NEW_HEAD" ]; then + log_entry "SETUP_RECOVERED from=$OLD_VER to=$(cat "$GSTACK_DIR/VERSION" 2>/dev/null || echo unknown)" + else + log_entry "UPDATED from=$OLD_VER to=$(cat "$GSTACK_DIR/VERSION" 2>/dev/null || echo unknown)" + fi else + # The pre-pull intent marker is no longer needed: HEAD did not move and no + # earlier migration was pending. + rm -f "$PENDING_SETUP_FILE" 2>/dev/null + # No upgrade migration is needed. Refreshing here keeps user-rendered + # skills current without replacing the old render before setup can prove + # ownership on an actual HEAD transition. + "$GSTACK_DIR/bin/gstack-config" gbrain-refresh >/dev/null 2>&1 || true log_entry "UP_TO_DATE head=$OLD_HEAD" fi # The detached subshell must own its stdio: it inherits the session hook's diff --git a/bin/gstack-skill-start b/bin/gstack-skill-start index 3da95c78ae..d69c944f9e 100755 --- a/bin/gstack-skill-start +++ b/bin/gstack-skill-start @@ -231,22 +231,39 @@ _BRAIN_CONFIG_BIN="$_BIN/gstack-config" # configured (zero context cost for non-gbrain users). _GBRAIN_CONFIG="$HOME/.gbrain/config.json" if [ -f "$_GBRAIN_CONFIG" ] && command -v gbrain >/dev/null 2>&1; then - _GBRAIN_VERSION_OK=$(gbrain --version 2>/dev/null | grep -c '^gbrain ' || true) + # Do not run a second, unbounded CLI probe before readiness. The bounded + # read-only checker below owns liveness and reports degradation explicitly. + _GBRAIN_VERSION_OK=1 if [ "$_GBRAIN_VERSION_OK" -gt 0 ] 2>/dev/null; then _GBRAIN_PIN_PATH="" _REPO_TOP=$(git rev-parse --show-toplevel 2>/dev/null || echo "") if [ -n "$_REPO_TOP" ] && [ -f "$_REPO_TOP/.gbrain-source" ]; then _GBRAIN_PIN_PATH="$_REPO_TOP/.gbrain-source" fi + _GBRAIN_READY_OUTPUT="GBRAIN_GRAPH: degraded | reason=bootstrap_unavailable" if [ -n "$_GBRAIN_PIN_PATH" ]; then + _GBRAIN_READY_BIN="$_BIN/gstack-gbrain-ready" + if [ -x "$_GBRAIN_READY_BIN" ]; then + _GBRAIN_READY_OUTPUT=$("$_GBRAIN_READY_BIN" --check-only --session-key "$PARENT_PID" 2>/dev/null || echo "GBRAIN_GRAPH: degraded | reason=bootstrap_failed") + echo "$_GBRAIN_READY_OUTPUT" + fi echo "GBrain configured. Prefer \`gbrain search\`/\`gbrain query\` over Grep for" echo "semantic questions; use \`gbrain code-def\`/\`code-refs\`/\`code-callers\` for" echo "symbol-aware code lookup. See \"## GBrain Search Guidance\" in CLAUDE.md." - echo "Run /sync-gbrain to refresh." + case "$_GBRAIN_READY_OUTPUT" in + *"GBRAIN_GRAPH: ready"*) echo "GBrain graph certified for this worktree." ;; + *"GBRAIN_GRAPH: partial"*) echo "GBrain graph is useful but non-authoritative; verify impact with repository lookup." ;; + *) echo "GBrain graph is degraded; use repository lookup and run \`/sync-gbrain --full\` to repair it explicitly." ;; + esac else - echo "GBrain configured but this worktree isn't pinned yet. Run \`/sync-gbrain --full\`" - echo "before relying on \`gbrain search\` for code questions in this worktree." - echo "Falls back to Grep until pinned." + _GBRAIN_READY_BIN="$_BIN/gstack-gbrain-ready" + if [ -x "$_GBRAIN_READY_BIN" ]; then + _GBRAIN_READY_OUTPUT=$("$_GBRAIN_READY_BIN" --check-only --session-key "$PARENT_PID" 2>/dev/null || echo "GBRAIN_GRAPH: degraded | reason=bootstrap_failed") + echo "$_GBRAIN_READY_OUTPUT" + else + echo "GBRAIN_GRAPH: degraded | reason=bootstrap_unavailable" + fi + echo "This worktree is not certified; use repository lookup and run \`/sync-gbrain --full\` explicitly." fi fi fi diff --git a/browse/src/server.ts b/browse/src/server.ts index 87192bcdcf..b99e8d527c 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -70,6 +70,7 @@ import { import { mintLease, validateLease, refreshLease, revokeLease, } from './pty-session-lease'; +import { registerTestShardDescendants, registerTestShardProcess } from './test-shard-process-registry'; import * as fs from 'fs'; import * as net from 'net'; import * as path from 'path'; @@ -3040,6 +3041,17 @@ export async function start() { const port = await findPort(); LOCAL_LISTEN_PORT = port; + // Production daemons intentionally outlive the CLI invocation that starts + // them. Free-test shards inject a private registry so the same setsid() + // boundary cannot escape test ownership. With no injected registry this is + // a strict no-op and interactive persistence is unchanged. + registerTestShardProcess({ + kind: 'browse-server', + pid: process.pid, + parentPid: process.ppid, + port, + stateFile: config.stateFile, + }); // ─── Proxy config (D8 + codex F5) ────────────────────────────── // BROWSE_PROXY_URL is set by the CLI when --proxy was passed. For SOCKS5 @@ -3119,6 +3131,12 @@ export async function start() { } try { xvfb = await spawnXvfb(displayNum); + registerTestShardProcess({ + kind: 'xvfb', + pid: xvfb.pid, + parentPid: process.pid, + stateFile: config.stateFile, + }); process.env.DISPLAY = xvfb.display; console.log(`[browse] [xvfb] spawned on ${xvfb.display} (pid ${xvfb.pid})`); } catch (err) { @@ -3149,6 +3167,21 @@ export async function start() { } else { await browserManager.launch(); } + const chromium = browserManager.getChromiumProcInfo(); + if (chromium) { + registerTestShardProcess({ + kind: 'chromium', + pid: chromium.pid, + parentPid: process.pid, + stateFile: config.stateFile, + }); + } + registerTestShardDescendants({ + kind: 'chromium', + ancestorPid: process.pid, + port, + stateFile: config.stateFile, + }); } const startTime = Date.now(); diff --git a/browse/src/terminal-agent-control.ts b/browse/src/terminal-agent-control.ts index 786d5a05ba..2741662c88 100644 --- a/browse/src/terminal-agent-control.ts +++ b/browse/src/terminal-agent-control.ts @@ -19,6 +19,7 @@ import * as path from 'path'; import { safeUnlink, safeKill, isProcessAlive } from './error-handling'; import { restrictFilePermissions, mkdirSecure } from './file-permissions'; import { atomicWriteSync } from '../../lib/fs-atomic'; +import { registerTestShardProcess } from './test-shard-process-registry'; /** * Locate the terminal-agent script on disk. In dev (cli.ts running via @@ -86,7 +87,17 @@ export function spawnTerminalAgent(opts: { windowsHide: true, }); proc.unref?.(); - return proc.pid ?? null; + const pid = proc.pid ?? null; + if (pid !== null) { + registerTestShardProcess({ + kind: 'terminal-agent', + pid, + parentPid: opts.ownerPid, + port: opts.serverPort, + stateFile: opts.stateFile, + }); + } + return pid; } export interface AgentRecord { diff --git a/browse/src/test-shard-process-registry.ts b/browse/src/test-shard-process-registry.ts new file mode 100644 index 0000000000..40d473e8cb --- /dev/null +++ b/browse/src/test-shard-process-registry.ts @@ -0,0 +1,273 @@ +/** + * Test-shard ownership bridge for processes that deliberately detach. + * + * GStack Browser must survive the short shell/agent invocation that starts it, + * so production daemons use a new session. That same property lets a daemon + * escape the free-test runner's process-group cleanup. The free runner injects + * a private, per-shard append-only registry; detached browse processes record + * only their process identity here. With no injected registry this module is a + * strict no-op, preserving normal interactive persistence. + */ + +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { spawnSync } from 'node:child_process'; + +export const TEST_PROCESS_REGISTRY_ENV = 'GSTACK_TEST_PROCESS_REGISTRY'; +export const TEST_PROCESS_REGISTRY_ID_ENV = 'GSTACK_TEST_PROCESS_REGISTRY_ID'; +export const TEST_PROCESS_REGISTRY_ROOT_ENV = 'GSTACK_TEST_PROCESS_REGISTRY_ROOT'; +export const TEST_PROCESS_REGISTRY_FILE = '.gstack-detached-processes.jsonl'; + +export type TestShardProcessKind = 'browse-server' | 'chromium' | 'terminal-agent' | 'xvfb'; + +export interface TestShardProcessRegistryHeader { + schema: 1; + type: 'gstack-test-process-registry'; + runId: string; + ownerPid: number; +} + +export interface TestShardProcessRecord { + schema: 1; + type: 'process'; + runId: string; + kind: TestShardProcessKind; + pid: number; + parentPid: number; + processGroupId: number | null; + processStartTime: string; + port: number | null; + stateFile: string | null; + registeredAt: string; +} + +export interface RegisterTestShardProcessInput { + kind: TestShardProcessKind; + pid: number; + parentPid?: number; + port?: number; + stateFile?: string; +} + +export interface RegisterTestShardDescendantsInput { + kind: TestShardProcessKind; + ancestorPid: number; + port?: number; + stateFile?: string; +} + +export interface RegisterTestShardProcessDependencies { + /** Test seam for simulating an unavailable POSIX identity probe. */ + readPsField?: (pid: number, field: 'lstart=' | 'pgid=') => string; + /** Test seam for distinguishing a dead fake PID from a live unprovable PID. */ + isProcessAlive?: (pid: number) => boolean; +} + +function readPsField(pid: number, field: 'lstart=' | 'pgid='): string { + if (process.platform === 'win32') return ''; + const result = spawnSync('ps', ['-p', String(pid), '-o', field], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + timeout: 2_000, + windowsHide: true, + }); + return result.status === 0 ? (result.stdout || '').trim() : ''; +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code === 'EPERM'; + } +} + +function parseHeader(raw: string): TestShardProcessRegistryHeader | null { + let value: unknown; + try { + value = JSON.parse(raw); + } catch { + return null; + } + if (typeof value !== 'object' || value === null) return null; + const candidate = value as Record; + if ( + candidate.schema !== 1 + || candidate.type !== 'gstack-test-process-registry' + || typeof candidate.runId !== 'string' + || typeof candidate.ownerPid !== 'number' + ) return null; + return candidate as unknown as TestShardProcessRegistryHeader; +} + +function resolveInjectedRegistry(env: NodeJS.ProcessEnv): { + registryPath: string; + runId: string; +} | null { + const registryPath = env[TEST_PROCESS_REGISTRY_ENV]; + const runId = env[TEST_PROCESS_REGISTRY_ID_ENV]; + const registryRoot = env[TEST_PROCESS_REGISTRY_ROOT_ENV]; + if (!registryPath && !runId && !registryRoot) return null; + if (!registryPath || !runId || !registryRoot) { + throw new Error('incomplete GStack test process registry environment'); + } + if (!path.isAbsolute(registryPath) || !path.isAbsolute(registryRoot)) { + throw new Error('GStack test process registry paths must be absolute'); + } + if (path.normalize(registryPath) !== path.join(path.normalize(registryRoot), TEST_PROCESS_REGISTRY_FILE)) { + throw new Error('GStack test process registry path is outside its shard root'); + } + + const registryStat = fs.lstatSync(registryPath); + if (!registryStat.isFile() || registryStat.isSymbolicLink()) { + throw new Error('GStack test process registry is not a regular file'); + } + const effectiveUid = process.geteuid?.() ?? process.getuid?.(); + if (effectiveUid !== undefined && registryStat.uid !== effectiveUid) { + throw new Error('GStack test process registry has a different owner'); + } + if ((registryStat.mode & 0o022) !== 0) { + throw new Error('GStack test process registry is group/world writable'); + } + const realRoot = fs.realpathSync(registryRoot); + const realParent = fs.realpathSync(path.dirname(registryPath)); + if (realRoot !== realParent) { + throw new Error('GStack test process registry parent changed identity'); + } + + const firstLine = fs.readFileSync(registryPath, 'utf8').split('\n', 1)[0] ?? ''; + const header = parseHeader(firstLine); + if (!header || header.runId !== runId) { + throw new Error('GStack test process registry header does not match this shard'); + } + return { registryPath, runId }; +} + +/** Register one detached process when, and only when, the free runner owns it. */ +export function registerTestShardProcess( + input: RegisterTestShardProcessInput, + env: NodeJS.ProcessEnv = process.env, + dependencies: RegisterTestShardProcessDependencies = {}, +): boolean { + const registry = resolveInjectedRegistry(env); + if (!registry) return false; + if (!Number.isSafeInteger(input.pid) || input.pid <= 0) { + throw new Error(`invalid test-owned ${input.kind} PID`); + } + if (input.port !== undefined && (!Number.isSafeInteger(input.port) || input.port <= 0 || input.port > 65_535)) { + throw new Error(`invalid test-owned ${input.kind} port`); + } + + const readIdentityField = dependencies.readPsField ?? readPsField; + const processIsAlive = dependencies.isProcessAlive ?? isProcessAlive; + const processStartTime = readIdentityField(input.pid, 'lstart='); + if (process.platform !== 'win32' && !processStartTime) { + // A mock or very short-lived child may already be gone. With no process + // left to own, omitting the row is safe; a live/EPERM PID whose identity + // cannot be proven must stop the shard instead of weakening cleanup. + if (!processIsAlive(input.pid)) return false; + throw new Error(`cannot prove test-owned ${input.kind} process start time`); + } + const processGroupRaw = readIdentityField(input.pid, 'pgid='); + const processGroupId = /^\d+$/.test(processGroupRaw) ? Number.parseInt(processGroupRaw, 10) : null; + if (process.platform !== 'win32' && processGroupId === null) { + if (!processIsAlive(input.pid)) return false; + throw new Error(`cannot prove test-owned ${input.kind} process group`); + } + // The supported bare Windows runner has no equivalent `ps` identity + // surface. Its records intentionally carry an empty start time and null + // group; parent cleanup branches to the per-shard run-id boundary there. + const record: TestShardProcessRecord = { + schema: 1, + type: 'process', + runId: registry.runId, + kind: input.kind, + pid: input.pid, + parentPid: input.parentPid ?? process.ppid, + processGroupId, + processStartTime, + port: input.port ?? null, + stateFile: input.stateFile ? path.resolve(input.stateFile) : null, + registeredAt: new Date().toISOString(), + }; + + const before = fs.lstatSync(registry.registryPath); + const noFollow = typeof fs.constants.O_NOFOLLOW === 'number' ? fs.constants.O_NOFOLLOW : 0; + const fd = fs.openSync( + registry.registryPath, + fs.constants.O_WRONLY | fs.constants.O_APPEND | noFollow, + ); + try { + const opened = fs.fstatSync(fd); + if (opened.dev !== before.dev || opened.ino !== before.ino) { + throw new Error('GStack test process registry changed during append'); + } + fs.writeSync(fd, `${JSON.stringify(record)}\n`); + } finally { + fs.closeSync(fd); + } + return true; +} + +/** + * Register matching descendants of a test-owned daemon. Playwright's public + * Browser object does not expose `.process()` in every supported build, while + * the OS child tree does. This fallback runs only under an injected test + * registry, so production browser launches pay no process-census cost. + */ +export function registerTestShardDescendants( + input: RegisterTestShardDescendantsInput, + env: NodeJS.ProcessEnv = process.env, +): number { + if (!resolveInjectedRegistry(env) || process.platform === 'win32') return 0; + const result = spawnSync('ps', ['-axo', 'pid=,ppid=,command='], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + timeout: 2_000, + windowsHide: true, + }); + if (result.status !== 0) return 0; + + const rows: Array<{ pid: number; parentPid: number; command: string }> = []; + for (const line of (result.stdout || '').split('\n')) { + const match = /^\s*(\d+)\s+(\d+)\s+(.*)$/.exec(line); + if (!match) continue; + rows.push({ + pid: Number.parseInt(match[1], 10), + parentPid: Number.parseInt(match[2], 10), + command: match[3], + }); + } + const descendants = new Set([input.ancestorPid]); + let changed = true; + while (changed) { + changed = false; + for (const row of rows) { + if (!descendants.has(row.parentPid) || descendants.has(row.pid)) continue; + descendants.add(row.pid); + changed = true; + } + } + const matchesKind = (command: string): boolean => { + switch (input.kind) { + case 'chromium': return /(?:chrom(?:e|ium)|headless[_-]shell)/i.test(command); + case 'terminal-agent': return /(?:^|[\\/])browse[\\/](?:src[\\/]terminal-agent\.ts|dist[\\/]terminal-agent)/.test(command); + case 'xvfb': return /(?:^|[\\/])Xvfb(?:\s|$)/.test(command); + case 'browse-server': return /(?:^|[\\/])browse[\\/](?:src[\\/]server\.ts|dist[\\/]server-node\.mjs)(?:\s|$)/.test(command); + } + }; + + let registered = 0; + for (const row of rows) { + if (row.pid === input.ancestorPid || !descendants.has(row.pid) || !matchesKind(row.command)) continue; + if (registerTestShardProcess({ + kind: input.kind, + pid: row.pid, + parentPid: row.parentPid, + port: input.port, + stateFile: input.stateFile, + }, env)) registered += 1; + } + return registered; +} diff --git a/package.json b/package.json index 6a6e6271bf..019e8c5251 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.79.0", + "version": "1.79.1", "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/scripts/skill-check.ts b/scripts/skill-check.ts index 9182737ee1..404d62c944 100644 --- a/scripts/skill-check.ts +++ b/scripts/skill-check.ts @@ -10,9 +10,12 @@ import { validateSkill } from '../test/helpers/skill-parser'; import { discoverTemplates, discoverSkillFiles } from './discover-skills'; +import { resolveCodexGenerationModel } from './resolve-codex-generation-model'; +import { ALL_HOST_CONFIGS, getExternalHosts, getHostConfig } from '../hosts/index'; +import type { HostConfig } from './host-config'; import * as fs from 'fs'; import * as path from 'path'; -import { execSync } from 'child_process'; +import { spawnSync } from 'child_process'; const ROOT = path.resolve(import.meta.dir, '..'); const ROOT_REALPATH = fs.realpathSync(ROOT); @@ -25,6 +28,66 @@ function isRepoRootSymlink(candidateDir: string): boolean { } } +/** + * Canonical in-tree SKILL.md files are the Claude-host render. A host may + * deliberately exclude a same-name wrapper skill (for example, /claude is an + * outside-voice skill that must not be installed in Claude Code). Such a + * template has no canonical Claude output by design and is not "missing". + */ +export function hostGeneratesTemplate( + tmpl: string, + hostConfig: HostConfig, + rootDir = ROOT, +): boolean { + const parent = path.dirname(tmpl); + const skillDir = parent === '.' ? path.basename(rootDir) : parent.split(path.sep)[0]; + const included = hostConfig.generation.includeSkills; + if (included?.length && !included.includes(skillDir)) return false; + return !hostConfig.generation.skipSkills?.includes(skillDir); +} + +export interface FreshnessInvocation { + args: string[]; + command: string; + model?: string; + modelSource?: string; + warnings: string[]; +} + +/** Build the exact generator invocation setup uses for the effective host. */ +export function freshnessInvocation( + hostConfig: HostConfig, + env: NodeJS.ProcessEnv = process.env, +): FreshnessInvocation { + const args = ['run', 'scripts/gen-skill-docs.ts']; + if (hostConfig.name !== 'claude') args.push('--host', hostConfig.name); + + let model: string | undefined; + let modelSource: string | undefined; + let warnings: string[] = []; + if (hostConfig.name === 'codex') { + const resolution = resolveCodexGenerationModel({ + codexHome: env.CODEX_HOME, + home: env.HOME, + }); + model = resolution.model; + modelSource = resolution.source; + warnings = resolution.warnings; + args.push('--model', resolution.model); + } + + args.push('--dry-run'); + return { + args, + command: ['bun', ...args].join(' '), + model, + modelSource, + warnings, + }; +} + +if (import.meta.main) { + // Find all SKILL.md files (dynamic discovery — no hardcoded list) const SKILL_FILES = discoverSkillFiles(ROOT); @@ -64,10 +127,15 @@ for (const file of SKILL_FILES) { console.log('\n Templates:'); const TEMPLATES = discoverTemplates(ROOT); +const canonicalHost = getHostConfig('claude'); for (const { tmpl, output } of TEMPLATES) { const tmplPath = path.join(ROOT, tmpl); const outPath = path.join(ROOT, output); + if (!hostGeneratesTemplate(tmpl, canonicalHost)) { + console.log(` - ${tmpl.padEnd(30)} — skipped for ${canonicalHost.displayName}`); + continue; + } if (!fs.existsSync(tmplPath)) { console.log(` \u26a0\ufe0f ${output.padEnd(30)} — no template`); continue; @@ -90,8 +158,6 @@ for (const file of SKILL_FILES) { // ─── External Host Skills (config-driven) ─────────────────── -import { getExternalHosts } from '../hosts/index'; - for (const hostConfig of getExternalHosts()) { const hostDir = path.join(ROOT, hostConfig.hostSubdir, 'skills'); if (fs.existsSync(hostDir)) { @@ -130,24 +196,35 @@ for (const hostConfig of getExternalHosts()) { // ─── Freshness (config-driven) ────────────────────────────── -import { ALL_HOST_CONFIGS } from '../hosts/index'; - for (const hostConfig of ALL_HOST_CONFIGS) { - const hostFlag = hostConfig.name === 'claude' ? '' : ` --host ${hostConfig.name}`; + const invocation = freshnessInvocation(hostConfig); console.log(`\n Freshness (${hostConfig.displayName}):`); - try { - execSync(`bun run scripts/gen-skill-docs.ts${hostFlag} --dry-run`, { cwd: ROOT, stdio: 'pipe' }); + if (invocation.model) { + console.log(` Profile: ${invocation.model} (${invocation.modelSource})`); + } + for (const warning of invocation.warnings) { + console.log(` ⚠️ ${warning}`); + } + const result = spawnSync('bun', invocation.args, { + cwd: ROOT, + encoding: 'utf8', + env: process.env, + timeout: 120_000, + }); + if (!result.error && result.status === 0) { console.log(` \u2705 All ${hostConfig.displayName} generated files are fresh`); - } catch (err: any) { + } else { hasErrors = true; - const output = err.stdout?.toString() || ''; + const output = result.stdout || ''; console.log(` \u274c ${hostConfig.displayName} generated files are stale:`); for (const line of output.split('\n').filter((l: string) => l.startsWith('STALE'))) { console.log(` ${line}`); } - console.log(` Run: bun run gen:skill-docs${hostFlag}`); + if (result.error) console.log(` ${result.error.message}`); + console.log(` Run: ${invocation.command.replace(/ --dry-run$/, '')}`); } } console.log(''); process.exit(hasErrors ? 1 : 0); +} diff --git a/scripts/test-free-shards.ts b/scripts/test-free-shards.ts index 06aa2c215c..88b771f48f 100755 --- a/scripts/test-free-shards.ts +++ b/scripts/test-free-shards.ts @@ -89,6 +89,15 @@ import { strictTestExitCode, stripAnsiLine, } from './test-strict-output'; +import { + createTestShardProcessRegistry, + forceReapRegisteredProcessesSync, + reapTestShardProcesses, + registryEnvironment, + signalRegisteredServersForCancellation, + type TestShardProcessCleanupDependencies, + type TestShardProcessCleanupReport, +} from './test-shard-process-owner'; const ROOT = path.resolve(import.meta.dir, '..'); // design/test was silently absent from BOTH the package.json test script and @@ -252,6 +261,10 @@ export const KNOWN_WINDOWS_INCOMPATIBLE: Array<{ file: string; reason: string }> file: 'browse/test/security-audit-r2.test.ts', reason: 'symlink-attack fixtures (evil-link) need Developer Mode CI runners lack; expect(toThrow) fires unhandled on Windows', }, + { + file: 'test/test-free-shards-detached-lifecycle.test.ts', + reason: 'POSIX process-session E2E launches real Playwright Chromium and proves setsid daemon cleanup; Windows uses different process ownership semantics', + }, ]; // Force-include overrides: files a WINDOWS_FRAGILE_PATTERNS regex excludes for @@ -1101,6 +1114,8 @@ export interface FreeShardOutcome { * hole the strict classifier exists to close. */ unattributedFailures: number; + /** Exact detached-process cleanup failure, retained even when timeout owns exit 124. */ + cleanupFailure: string | null; } export interface ShardCommand { @@ -1117,6 +1132,8 @@ export interface RunFreeShardOptions { parallel?: boolean; /** Override the spawned command. Tests inject fake pass/fail/slow commands. */ commandFor?: (files: string[]) => ShardCommand; + /** Test seam for proving that a failed OS process census fails cleanup closed. */ + processCleanupDependencies?: TestShardProcessCleanupDependencies; /** Suppress ALL child output from the console (tests). The classifier and the log file still see every byte. */ quiet?: boolean; /** Forward the full child stream to the console (legacy firehose). Default: the quiet filtered console. */ @@ -1179,7 +1196,7 @@ export async function runFreeShard( // so an unoccupied index must not fail or shift work to a different runner. if (files.length === 0) { const outcome: FreeShardOutcome = { - shard: shardNumber, files: [], status: 'passed', exitCode: 0, elapsedMs: 0, groupPid: null, failingFiles: [], unattributedFailures: 0, + shard: shardNumber, files: [], status: 'passed', exitCode: 0, elapsedMs: 0, groupPid: null, failingFiles: [], unattributedFailures: 0, cleanupFailure: null, }; log(shardEpilogue(outcome, totalShards)); return outcome; @@ -1208,11 +1225,18 @@ export async function runFreeShard( const env = { ...(options.env ?? process.env) }; const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-free-shard-')); + const processRegistry = createTestShardProcessRegistry(stateDir); const childTmp = path.join(stateDir, 'tmp'); fs.mkdirSync(childTmp); env.TMPDIR = childTmp; env.TEMP = childTmp; env.TMP = childTmp; + // Browser daemons intentionally setsid() so they persist across interactive + // CLI invocations. A test-owned daemon must instead remain owned by this + // shard even after escaping the child's process group. The injected registry + // is outside TMPDIR so a test deleting its own fixture cannot erase custody. + Object.assign(env, registryEnvironment(processRegistry)); + env.BROWSE_STATE_FILE = path.join(stateDir, 'browse.json'); // Per-shard Chromium profile (same isolation idea as TMPDIR): nine test // files launch in-process persistent contexts or daemons that default to // the SHARED ~/.gstack/chromium-profile, and two concurrent shards on one @@ -1234,10 +1258,22 @@ export async function runFreeShard( windowsHide: true, }); const groupPid = child.pid ?? null; + let signalForwardingCleanupError: string | null = null; // Group-kill on parent SIGINT/SIGTERM too, not just on timeout. const forwarding = installChildSignalForwarding({ kill: (signal?: NodeJS.Signals | number) => { - killProcessGroup(child, (signal as NodeJS.Signals) ?? 'SIGTERM'); + const forwardedSignal = (signal as NodeJS.Signals) ?? 'SIGTERM'; + killProcessGroup(child, forwardedSignal); + try { + if (forwardedSignal === 'SIGKILL') { + forceReapRegisteredProcessesSync(processRegistry); + } else { + signalRegisteredServersForCancellation(processRegistry); + } + } catch (error) { + signalForwardingCleanupError = `could not signal detached-process registry: ${error instanceof Error ? error.message : String(error)}`; + console.error(`${label} ${signalForwardingCleanupError}`); + } return true; }, }); @@ -1277,6 +1313,9 @@ export async function runFreeShard( }, wallTimeoutMs); let exitCode: number | null = null; + let processCleanup: TestShardProcessCleanupReport | null = null; + let processCleanupError: string | null = null; + let retainShardState = false; try { const streams: Array> = []; if (child.stdout) streams.push(consumeStream(child.stdout, 'stdout')); @@ -1289,28 +1328,67 @@ export async function runFreeShard( } finally { clearTimeout(killTimer); forwarding.dispose(); - // Reap survivors of this shard even on the clean path. + // First reap the ordinary shard group, then the detached processes that + // deliberately escaped it. The registry must remain on disk until both + // graceful and exact-force cleanup have converged. killProcessGroup(child, 'SIGKILL'); + try { + processCleanup = await reapTestShardProcesses(processRegistry, options.processCleanupDependencies); + if (!processCleanup.success) { + processCleanupError = `${processCleanup.survivors} survivor(s), ` + + `${processCleanup.unsafeGroups} unsafe process group(s), ` + + `${processCleanup.identityMismatches} identity mismatch(es), ` + + `${processCleanup.invalidRecords} invalid registry row(s)`; + retainShardState = processCleanup.survivors > 0 + || processCleanup.unsafeGroups > 0 + || processCleanup.identityMismatches > 0; + } + } catch (error) { + processCleanupError = error instanceof Error ? error.message : String(error); + // Without a trusted registry header we cannot prove what still needs + // custody. Preserve the shard root instead of deleting the only receipt. + retainShardState = true; + } + if (signalForwardingCleanupError) { + processCleanupError = processCleanupError + ? `${signalForwardingCleanupError}; ${processCleanupError}` + : signalForwardingCleanupError; + retainShardState = true; + } reporter.end(); await new Promise((resolve) => logStream.end(() => resolve())); - try { - fs.rmSync(stateDir, { recursive: true, force: true }); - } catch { - // Best-effort cleanup of a throwaway temp dir — a locked file on - // Windows must not turn a real verdict into an exception. + if (!retainShardState) { + try { + fs.rmSync(stateDir, { recursive: true, force: true }); + } catch { + // Best-effort cleanup of a throwaway temp dir — a locked file on + // Windows must not turn a real verdict into an exception. + } + } + if (fs.existsSync(stateDir)) { + processCleanupError = processCleanupError + ? `${processCleanupError}; shard state retained at ${stateDir}` + : 'shard state directory survived cleanup'; } } const summary = classifier.end(); const status: FreeShardStatus = timedOut ? 'timed-out' - : strictTestExitCode(exitCode ?? 1, summary, files.length) === 0 ? 'passed' : 'failed'; + : processCleanupError + ? 'failed' + : strictTestExitCode(exitCode ?? 1, summary, files.length) === 0 ? 'passed' : 'failed'; if (status === 'timed-out') { console.error( `${label} exceeded the ${Math.round(wallTimeoutMs / 1000)}s wall-clock deadline — ` + 'killed the process group. Reporting as TIMED-OUT (distinct from failed).', ); + if (processCleanupError) { + console.error(`${label} timed out AND detached-process cleanup failed: ${processCleanupError}`); + } + } else if (processCleanupError) { + console.error(`${label} detached-process leak gate failed: ${processCleanupError}`); } else if (status === 'failed' && (exitCode ?? 1) === 0) { const reason = summary.failedTests > 0 || summary.unhandledBetweenTests > 0 ? `printed ${summary.failedTests} failing result(s) and ${summary.unhandledBetweenTests} unhandled error(s) between tests` @@ -1330,11 +1408,17 @@ export async function runFreeShard( const unattributedFailures = status === 'passed' ? 0 : report.failures.filter((f) => !f.file).length + report.unhandledErrors.length - + (report.sawTerminalSummary ? 0 : 1); + + (report.sawTerminalSummary ? 0 : 1) + + (processCleanupError ? 1 : 0); const outcome: FreeShardOutcome = { - shard: shardNumber, files, status, exitCode, elapsedMs: Date.now() - startedAt, groupPid, failingFiles, unattributedFailures, + shard: shardNumber, files, status, exitCode, elapsedMs: Date.now() - startedAt, groupPid, failingFiles, unattributedFailures, cleanupFailure: processCleanupError, }; log(shardEpilogue(outcome, totalShards)); + if (processCleanup && processCleanup.registered > 0) { + log(`${label} detached-process gate: ${processCleanup.registered} registered, ` + + `${processCleanup.gracefullyStoppedServers} server(s) graceful, ` + + `${processCleanup.survivors} survivor(s), ${processCleanup.success ? 'pass' : 'fail'}`); + } for (const line of buildRunEpilogue(status, report, outcome.elapsedMs, logPath)) log(line); return outcome; } @@ -1535,6 +1619,14 @@ async function main(): Promise { outcomes.push(mutatorOutcome); } + const cleanupFailures = outcomes.filter((outcome) => outcome.cleanupFailure !== null); + if (cleanupFailures.length > 0) { + console.error(`[test:free] DETACHED-PROCESS CLEANUP FAILURES — ${cleanupFailures.length} shard(s):`); + for (const outcome of cleanupFailures) { + console.error(`[test:free] shard ${outcome.shard}: ${outcome.cleanupFailure}`); + } + } + // Opt-in flaky retry (GSTACK_FREE_RETRY_FLAKY=1): when every failure is an // attributed test failure (no timeouts, no unattributed carnage), re-run // just the failing files ONCE in a fresh serial shard. A clean retry diff --git a/scripts/test-shard-process-owner.ts b/scripts/test-shard-process-owner.ts new file mode 100644 index 0000000000..1d953cab47 --- /dev/null +++ b/scripts/test-shard-process-owner.ts @@ -0,0 +1,450 @@ +/** + * Parent-side ownership for daemons intentionally detached by free tests. + * + * The shard child owns its normal process group; GStack Browser intentionally + * calls setsid() and therefore escapes that group. A private registry bridges + * the ownership boundary without changing production persistence. Cleanup is + * identity-gated (PID + start time + command class), graceful first, and only + * then escalates to exact recorded processes/groups. + */ + +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { randomUUID } from 'node:crypto'; +import { spawnSync } from 'node:child_process'; +import { + TEST_PROCESS_REGISTRY_ENV, + TEST_PROCESS_REGISTRY_FILE, + TEST_PROCESS_REGISTRY_ID_ENV, + TEST_PROCESS_REGISTRY_ROOT_ENV, + type TestShardProcessKind, + type TestShardProcessRecord, + type TestShardProcessRegistryHeader, +} from '../browse/src/test-shard-process-registry'; + +export interface TestShardProcessRegistryHandle { + root: string; + registryPath: string; + runId: string; +} + +export interface TestShardProcessCleanupReport { + registered: number; + gracefullyStoppedServers: number; + termSignals: number; + killSignals: number; + identityMismatches: number; + invalidRecords: number; + survivors: number; + unsafeGroups: number; + success: boolean; +} + +export interface TestShardProcessGroupMember { + pid: number; + parentPid: number; + command: string; +} + +export interface TestShardProcessCleanupDependencies { + /** Test seam; production uses the fail-closed OS process-group census. */ + listGroupMembers?: (processGroupId: number) => TestShardProcessGroupMember[]; +} + +interface RegistryReadResult { + records: TestShardProcessRecord[]; + invalidRecords: number; +} + +const PROCESS_KINDS = new Set([ + 'browse-server', + 'chromium', + 'terminal-agent', + 'xvfb', +]); + +function isPositivePid(value: unknown): value is number { + return typeof value === 'number' && Number.isSafeInteger(value) && value > 0; +} + +function isRecord(value: unknown, runId: string): value is TestShardProcessRecord { + if (typeof value !== 'object' || value === null) return false; + const row = value as Record; + return row.schema === 1 + && row.type === 'process' + && row.runId === runId + && typeof row.kind === 'string' + && PROCESS_KINDS.has(row.kind as TestShardProcessKind) + && isPositivePid(row.pid) + && isPositivePid(row.parentPid) + && (row.processGroupId === null || isPositivePid(row.processGroupId)) + && typeof row.processStartTime === 'string' + && (row.port === null || (typeof row.port === 'number' && row.port > 0 && row.port <= 65_535)) + && (row.stateFile === null || typeof row.stateFile === 'string') + && typeof row.registeredAt === 'string'; +} + +function readPsField(pid: number, field: 'lstart=' | 'command=' | 'pgid=' | 'ppid='): string { + if (process.platform === 'win32') return ''; + const result = spawnSync('ps', ['-p', String(pid), '-o', field], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + timeout: 2_000, + windowsHide: true, + }); + return result.status === 0 ? (result.stdout || '').trim() : ''; +} + +function readCommand(pid: number): string { + if (process.platform !== 'darwin') { + try { + return fs.readFileSync(`/proc/${pid}/cmdline`, 'utf8').replaceAll('\0', ' ').trim(); + } catch { + // Fall through to ps on POSIX hosts without procfs. + } + } + return readPsField(pid, 'command='); +} + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code === 'EPERM'; + } +} + +function commandMatches(kind: TestShardProcessKind, command: string): boolean { + switch (kind) { + case 'browse-server': + return /(?:^|[\\/])browse[\\/](?:src[\\/]server\.ts|dist[\\/]server-node\.mjs)(?:\s|$)/.test(command); + case 'chromium': + return /(?:chrom(?:e|ium)|headless[_-]shell)/i.test(command); + case 'terminal-agent': + return /(?:^|[\\/])browse[\\/](?:src[\\/]terminal-agent\.ts|dist[\\/]terminal-agent)/.test(command); + case 'xvfb': + return /(?:^|[\\/])Xvfb(?:\s|$)/.test(command); + } +} + +function stillOwns(record: TestShardProcessRecord): boolean { + if (!isAlive(record.pid)) return false; + // Windows has no portable `ps` start-time/cmdline surface in the supported + // bare runner. The unguessable per-shard registry id plus immediate cleanup + // is the ownership boundary there; POSIX additionally requires command, + // process start time, and process-group identity to match. + if (process.platform === 'win32') return true; + if (!record.processStartTime || record.processGroupId === null) return false; + const command = readCommand(record.pid); + if (!commandMatches(record.kind, command)) return false; + if (readPsField(record.pid, 'lstart=') !== record.processStartTime) return false; + const currentProcessGroup = readPsField(record.pid, 'pgid='); + if (!/^\d+$/.test(currentProcessGroup)) return false; + if (Number.parseInt(currentProcessGroup, 10) !== record.processGroupId) return false; + return true; +} + +function signalPid(record: TestShardProcessRecord, signal: NodeJS.Signals): boolean { + if (!stillOwns(record)) return false; + try { + process.kill(record.pid, signal); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ESRCH') return false; + throw error; + } +} + +function readRegistry(handle: TestShardProcessRegistryHandle): RegistryReadResult { + const lines = fs.readFileSync(handle.registryPath, 'utf8').split('\n').filter(Boolean); + if (lines.length === 0) throw new Error('test process registry lost its header'); + + let headerValue: unknown; + try { + headerValue = JSON.parse(lines[0]); + } catch { + throw new Error('test process registry header is malformed'); + } + if (typeof headerValue !== 'object' || headerValue === null) { + throw new Error('test process registry header is malformed'); + } + const header = headerValue as Record; + if (header.schema !== 1 || header.type !== 'gstack-test-process-registry' || header.runId !== handle.runId) { + throw new Error('test process registry header identity changed'); + } + + const records: TestShardProcessRecord[] = []; + let invalidRecords = 0; + for (const line of lines.slice(1)) { + let value: unknown; + try { + value = JSON.parse(line); + } catch { + invalidRecords += 1; + continue; + } + if (!isRecord(value, handle.runId)) { + invalidRecords += 1; + continue; + } + records.push(value); + } + const unique = new Map(); + for (const record of records) { + unique.set(`${record.kind}:${record.pid}:${record.processStartTime}`, record); + } + return { records: [...unique.values()], invalidRecords }; +} + +function sleep(milliseconds: number): Promise { + return new Promise((resolve) => setTimeout(resolve, milliseconds)); +} + +async function waitFor(records: TestShardProcessRecord[], timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + while (records.some(stillOwns) && Date.now() < deadline) { + await sleep(50); + } +} + +function groupMembers(processGroupId: number): TestShardProcessGroupMember[] { + if (process.platform === 'win32') return []; + const result = spawnSync('ps', ['-axo', 'pid=,ppid=,pgid=,command='], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + timeout: 2_000, + windowsHide: true, + }); + if (result.error || result.status !== 0) { + const reason = result.error instanceof Error + ? result.error.message + : `ps exited ${result.status ?? 'without a status'}`; + throw new Error(`process-group census failed for PGID ${processGroupId}: ${reason}`); + } + const members: TestShardProcessGroupMember[] = []; + for (const line of (result.stdout || '').split('\n')) { + const match = /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.*)$/.exec(line); + if (!match || Number.parseInt(match[3], 10) !== processGroupId) continue; + members.push({ + pid: Number.parseInt(match[1], 10), + parentPid: Number.parseInt(match[2], 10), + command: match[4], + }); + } + return members; +} + +function trustedBrowseGroups(records: TestShardProcessRecord[]): Map { + const trusted = new Map(); + for (const record of records) { + if ( + record.kind === 'browse-server' + && record.processGroupId === record.pid + && stillOwns(record) + ) { + trusted.set(record.pid, record); + } + } + return trusted; +} + +/** + * Take a final parent-side census of known daemon members in a process group whose + * leader was already proven to be this shard's exact browse server. Chromium + * can spawn helpers after the server's launch-time snapshot, and the watchdog + * can replace a terminal agent during shutdown. PGID membership cannot cross + * the server's setsid boundary, so this closes that late-child window without + * looking at or targeting unrelated user browser sessions. + */ +function includeLateDaemonMembers( + records: TestShardProcessRecord[], + trustedGroups: Map, + listGroupMembers: (processGroupId: number) => TestShardProcessGroupMember[] = groupMembers, +): TestShardProcessRecord[] { + if (process.platform === 'win32' || trustedGroups.size === 0) return records; + const unique = new Map(); + for (const record of records) { + unique.set(`${record.kind}:${record.pid}:${record.processStartTime}`, record); + } + for (const [processGroupId, owner] of trustedGroups) { + for (const member of listGroupMembers(processGroupId)) { + if (records.some((record) => record.pid === member.pid)) continue; + const kind: TestShardProcessKind | null = commandMatches('chromium', member.command) + ? 'chromium' + : commandMatches('terminal-agent', member.command) ? 'terminal-agent' : null; + if (kind === null) continue; + const processStartTime = readPsField(member.pid, 'lstart='); + if (!processStartTime) continue; + const record: TestShardProcessRecord = { + schema: 1, + type: 'process', + runId: owner.runId, + kind, + pid: member.pid, + parentPid: member.parentPid, + processGroupId, + processStartTime, + port: owner.port, + stateFile: owner.stateFile, + registeredAt: new Date().toISOString(), + }; + unique.set(`${record.kind}:${record.pid}:${record.processStartTime}`, record); + } + } + return [...unique.values()]; +} + +function groupMemberAllowed( + owner: TestShardProcessRecord, + member: { pid: number; parentPid: number; command: string }, + records: TestShardProcessRecord[], + trustedBrowseGroupIds: Set, +): boolean { + const recorded = records.find((record) => record.pid === member.pid && stillOwns(record)); + if (recorded) return true; + // Playwright helpers are not individually exposed in every build and the + // server watchdog may replace its terminal agent during shutdown. The exact + // trusted server group plus this command allowlist closes both append races. + return (owner.kind === 'chromium' + || (owner.kind === 'browse-server' + && owner.processGroupId !== null + && trustedBrowseGroupIds.has(owner.processGroupId))) + && (commandMatches('chromium', member.command) || commandMatches('terminal-agent', member.command)); +} + +function signalExactGroup( + owner: TestShardProcessRecord, + records: TestShardProcessRecord[], + signal: NodeJS.Signals, + trustedBrowseGroupIds: Set = new Set(), + listGroupMembers: (processGroupId: number) => TestShardProcessGroupMember[] = groupMembers, +): 'sent' | 'empty' | 'unsafe' { + if (process.platform === 'win32' || owner.processGroupId === null || owner.processGroupId !== owner.pid) { + return 'empty'; + } + const members = listGroupMembers(owner.processGroupId); + if (members.length === 0) return 'empty'; + if (!members.every((member) => groupMemberAllowed(owner, member, records, trustedBrowseGroupIds))) return 'unsafe'; + try { + process.kill(-owner.processGroupId, signal); + return 'sent'; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ESRCH') return 'empty'; + throw error; + } +} + +export function createTestShardProcessRegistry(root: string): TestShardProcessRegistryHandle { + const registryPath = path.join(root, TEST_PROCESS_REGISTRY_FILE); + const runId = randomUUID(); + const header: TestShardProcessRegistryHeader = { + schema: 1, + type: 'gstack-test-process-registry', + runId, + ownerPid: process.pid, + }; + fs.writeFileSync(registryPath, `${JSON.stringify(header)}\n`, { flag: 'wx', mode: 0o600 }); + return { root, registryPath, runId }; +} + +export function registryEnvironment(handle: TestShardProcessRegistryHandle): NodeJS.ProcessEnv { + return { + [TEST_PROCESS_REGISTRY_ENV]: handle.registryPath, + [TEST_PROCESS_REGISTRY_ID_ENV]: handle.runId, + [TEST_PROCESS_REGISTRY_ROOT_ENV]: handle.root, + }; +} + +/** Start graceful teardown as soon as the parent receives SIGINT/SIGTERM. */ +export function signalRegisteredServersForCancellation(handle: TestShardProcessRegistryHandle): number { + if (!fs.existsSync(handle.registryPath)) return 0; + let sent = 0; + for (const record of readRegistry(handle).records) { + if (record.kind === 'browse-server' && signalPid(record, process.platform === 'win32' ? 'SIGTERM' : 'SIGINT')) { + sent += 1; + } + } + return sent; +} + +/** Synchronous last resort for process `exit`, where awaiting is impossible. */ +export function forceReapRegisteredProcessesSync(handle: TestShardProcessRegistryHandle): number { + if (!fs.existsSync(handle.registryPath)) return 0; + let { records } = readRegistry(handle); + const trustedGroups = trustedBrowseGroups(records); + records = includeLateDaemonMembers(records, trustedGroups); + let sent = 0; + for (const record of records) { + if (signalPid(record, 'SIGKILL')) sent += 1; + } + for (const owner of records) { + const result = signalExactGroup(owner, records, 'SIGKILL', new Set(trustedGroups.keys())); + if (result === 'sent') sent += 1; + } + return sent; +} + +export async function reapTestShardProcesses( + handle: TestShardProcessRegistryHandle, + dependencies: TestShardProcessCleanupDependencies = {}, +): Promise { + const listGroupMembers = dependencies.listGroupMembers ?? groupMembers; + let registry = readRegistry(handle); + let records = registry.records; + let invalidRecords = registry.invalidRecords; + const trustedGroups = trustedBrowseGroups(records); + records = includeLateDaemonMembers(records, trustedGroups, listGroupMembers); + const initiallyLiveServers = records.filter((record) => record.kind === 'browse-server' && stillOwns(record)); + for (const server of initiallyLiveServers) { + signalPid(server, process.platform === 'win32' ? 'SIGTERM' : 'SIGINT'); + } + await waitFor(initiallyLiveServers, 2_500); + const gracefullyStoppedServers = initiallyLiveServers.filter((record) => !stillOwns(record)).length; + + // A server may start its terminal agent while graceful shutdown is in + // flight. Re-read the append-only registry before escalation. + registry = readRegistry(handle); + records = includeLateDaemonMembers(registry.records, trustedGroups, listGroupMembers); + invalidRecords = Math.max(invalidRecords, registry.invalidRecords); + let termSignals = 0; + for (const record of records) { + if (signalPid(record, 'SIGTERM')) termSignals += 1; + } + await waitFor(records, 750); + + registry = readRegistry(handle); + records = includeLateDaemonMembers(registry.records, trustedGroups, listGroupMembers); + invalidRecords = Math.max(invalidRecords, registry.invalidRecords); + let killSignals = 0; + let unsafeGroups = 0; + for (const owner of records) { + const result = signalExactGroup( + owner, + records, + 'SIGKILL', + new Set(trustedGroups.keys()), + listGroupMembers, + ); + if (result === 'sent') killSignals += 1; + if (result === 'unsafe') unsafeGroups += 1; + } + for (const record of records) { + if (signalPid(record, 'SIGKILL')) killSignals += 1; + } + await waitFor(records, 750); + + const survivors = records.filter(stillOwns).length; + const identityMismatches = records.filter((record) => isAlive(record.pid) && !stillOwns(record)).length; + return { + registered: records.length, + gracefullyStoppedServers, + termSignals, + killSignals, + identityMismatches, + invalidRecords, + survivors, + unsafeGroups, + success: survivors === 0 && unsafeGroups === 0 && identityMismatches === 0 && invalidRecords === 0, + }; +} diff --git a/setup b/setup index 4f28bd49df..fb6f6b958b 100755 --- a/setup +++ b/setup @@ -121,31 +121,306 @@ _link_or_copy() { # skills dirs are SHARED namespaces (~/.codex/skills, ~/.factory/skills, # ~/.cursor/skills, ...), so a gstack* glob name can collide with a user's # OWN real directory (e.g. ~/.cursor/skills/gstack-notes) — deleting it on -# every ./setup re-run is silent data loss. Mirror of bin/gstack-uninstall's -# provenance gate (#2563): an existing REAL skill dir may only be replaced -# when its SKILL.md carries the generated banner. Missing targets and -# symlinks always pass (replacing a link never destroys content); non-dir -# targets pass (file targets live inside gstack-owned roots). +# every ./setup re-run is silent data loss. A generic generated banner is not +# provenance: new copies carry an exact host/source/served marker, and the +# markerless upgrade path accepts only a byte-exact copy of the current source. +_host_skill_owner_marker() { + local src="$1" + local dst="$2" + local host="$3" + local served_name="$4" + src="${src%/}" + dst="${dst%/}" + printf 'gstack-owner-v1 kind=host-skill host=%s source=%s target=%s served=%s\n' \ + "$host" "$src" "$dst" "$served_name" +} + _owned_for_windows_refresh() { local dst="$1" + local src="$2" + local host="$3" + local served_name="$4" + local link_dest expected_marker actual_marker + if [ ! -e "$dst" ] && [ ! -L "$dst" ]; then return 0; fi - if [ -L "$dst" ]; then return 0; fi - if [ ! -d "$dst" ]; then return 0; fi - grep -q '' + local installed_name + + [ -e "$root" ] || [ -L "$root" ] || return 1 + [ -d "$root" ] && [ ! -L "$root" ] || return 0 + [ -f "$root/SKILL.md" ] && [ ! -L "$root/SKILL.md" ] || return 0 + grep -Fqx "$generated_banner" "$root/SKILL.md" 2>/dev/null || return 0 + installed_name=$(grep -m1 '^name:' "$root/SKILL.md" 2>/dev/null | sed 's/^name:[[:space:]]*//' | tr -d '[:space:]' || true) + [ "$installed_name" != "gstack" ] +} + +_runtime_root_owner_marker() { + local host="$1" + local gstack_dir="$2" + local root="$3" + printf 'gstack-owner-v1 kind=runtime-root host=%s source=%s target=%s\n' \ + "$host" "$gstack_dir" "$root" +} + +_runtime_root_owned() { + local root="$1" + local gstack_dir="$2" + local expected_skill="$3" + local host="$4" + local link_dest expected_marker actual_marker + + [ -e "$root" ] || [ -L "$root" ] || return 0 + if [ -L "$root" ]; then + link_dest=$(readlink "$root" 2>/dev/null || true) + [ "$link_dest" = "$gstack_dir" ] || [ "$link_dest" = "$(dirname "$expected_skill")" ] + return $? + fi [ -d "$root" ] || return 1 - [ -L "$root" ] && return 1 - [ -f "$root/SKILL.md" ] || return 1 - ! grep -q '" + grep -Fqx "$marker" "$entry/SKILL.md" 2>/dev/null && return 0 + _legacy_claude_consumer_copy "$entry" "$gstack_dir" "$render_dir" "$source_name" "$served_name" +} + +_install_managed_skill_md() { + local src_skill_md="$1" + local dst_dir="$2" + local source_name="$3" + local served_name="$4" + local rewrite_name="$5" + local tmp_file + + tmp_file=$(mktemp "$dst_dir/.gstack-skill.XXXXXX") + if [ "$rewrite_name" -eq 1 ]; then + sed "1,/^---\$/ s/^name:[[:space:]].*/name: $served_name/" "$src_skill_md" > "$tmp_file" + else + cp "$src_skill_md" "$tmp_file" + fi + printf '\n\n' "$source_name" "$served_name" >> "$tmp_file" + mv "$tmp_file" "$dst_dir/SKILL.md" +} + +# Validate every destination before cleanup changes the currently-working +# naming mode. Without this pass, prefix -> flat could delete a managed +# gstack-qa and only then discover that the user's qa destination is occupied. +preflight_claude_skill_dirs() { + local gstack_dir="$1" + local skills_dir="$2" + local render_dir skill_dir dir_name skill_name link_name target + render_dir="${GSTACK_USER_RENDER_DIR:-${GSTACK_HOME:-$HOME/.gstack}/render/claude}" + + for skill_dir in "$gstack_dir"/*/; do + [ -f "$skill_dir/SKILL.md" ] || continue + [ -L "${skill_dir%/}" ] && continue + dir_name="$(basename "$skill_dir")" + [ "$dir_name" = "node_modules" ] && continue + skill_name=$(grep -m1 '^name:' "$skill_dir/SKILL.md" 2>/dev/null | sed 's/^name:[[:space:]]*//' | tr -d '[:space:]') + [ -z "$skill_name" ] && skill_name="$dir_name" + 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" + if { [ -e "$target" ] || [ -L "$target" ]; } \ + && ! _claude_skill_entry_owned "$target" "$gstack_dir" "$render_dir" "$dir_name" "$link_name"; then + echo "Error: refusing to replace non-gstack skill entry: $target" >&2 + return 2 + fi + done } # Swap a freshly-rendered tmp dir into the live render location (#2569 @@ -912,8 +1187,9 @@ _link_skill_runtime_assets() { } # ─── Helper: link Claude skill subdirectories into a skills parent directory ── -# Creates real directories (not symlinks) at the top level with a SKILL.md symlink -# inside. This ensures Claude discovers them as top-level skills, not nested under +# Creates real directories (not symlinks) at the top level. Flat installs link +# SKILL.md to canonical source; prefixed installs write a namespaced consumer +# copy. This ensures Claude discovers them as top-level skills, not nested under # gstack/ (which would auto-prefix them as gstack-*). # When SKILL_PREFIX=1, directories are prefixed with "gstack-". # Use --no-prefix to restore flat names. @@ -923,6 +1199,10 @@ link_claude_skill_dirs() { local linked=() for skill_dir in "$gstack_dir"/*/; do if [ -f "$skill_dir/SKILL.md" ]; then + # Back-compat aliases such as connect-chrome are source-directory + # symlinks. Their canonical target is installed once under its own name; + # serving the alias here would collide with that target. + [ -L "${skill_dir%/}" ] && continue dir_name="$(basename "$skill_dir")" # Skip node_modules [ "$dir_name" = "node_modules" ] && continue @@ -939,11 +1219,24 @@ link_claude_skill_dirs() { link_name="$skill_name" fi target="$skills_dir/$link_name" + _skill_md_src="$gstack_dir/$dir_name/SKILL.md" + _render_dir="${GSTACK_USER_RENDER_DIR:-${GSTACK_HOME:-$HOME/.gstack}/render/claude}" + if [ -f "$_render_dir/$dir_name/SKILL.md" ]; then + _skill_md_src="$_render_dir/$dir_name/SKILL.md" + fi + # Existing entries are replaceable only with exact gstack provenance. + # Abort before touching SKILL.md or runtime assets on a name collision. + if [ -e "$target" ] || [ -L "$target" ]; then + if ! _claude_skill_entry_owned "$target" "$gstack_dir" "$_render_dir" "$dir_name" "$link_name"; then + echo "Error: refusing to replace non-gstack skill entry: $target" >&2 + return 2 + fi + fi # Upgrade old directory symlinks to real directories if [ -L "$target" ]; then rm -f "$target" fi - # Create real directory with symlinked SKILL.md (absolute path) + # Create a real top-level skill directory. # Use mkdir -p unconditionally (idempotent) to avoid TOCTOU race mkdir -p "$target" # Validate target isn't a symlink before creating the link @@ -954,12 +1247,18 @@ link_claude_skill_dirs() { # checkout; when a render exists for this skill, serve it. The rendered # file's section-base paths point into the render dir, so section reads # resolve there too. - _skill_md_src="$gstack_dir/$dir_name/SKILL.md" - _render_dir="${GSTACK_USER_RENDER_DIR:-${GSTACK_HOME:-$HOME/.gstack}/render/claude}" - if [ -f "$_render_dir/$dir_name/SKILL.md" ]; then - _skill_md_src="$_render_dir/$dir_name/SKILL.md" + if [ "$SKILL_PREFIX" -eq 1 ]; then + # Prefixing is an install concern, never a source-tree mutation. The + # served copy carries the namespaced frontmatter while the canonical + # generated SKILL.md remains byte-identical to its template. + _install_managed_skill_md "$_skill_md_src" "$target" "$dir_name" "$link_name" 1 + elif [ "$IS_WINDOWS" -eq 1 ]; then + # Windows cannot preserve the Unix SKILL.md symlink as provenance, so + # install a marked byte-copy. The marker makes later refreshes safe. + _install_managed_skill_md "$_skill_md_src" "$target" "$dir_name" "$link_name" 0 + else + _link_or_copy "$_skill_md_src" "$target/SKILL.md" fi - _link_or_copy "$_skill_md_src" "$target/SKILL.md" # Link every runtime asset the skill ships next to its SKILL.md (#2317, # #2454): sections/ for carved skills, review's checklist.md + # specialists/, qa's templates/ + references/, gstack-upgrade's @@ -988,18 +1287,119 @@ link_claude_skill_dirs() { # connect-chrome, gstack-connect-chrome) is hardcoded in the _INVENTORY seed # list in bin/gstack-uninstall — keep the two sites in sync when adding or # renaming an alias, or uninstall will refuse to delete the new alias dir. +_legacy_claude_alias_copy() { + local installed_file="$1" + local src_skill_md="$2" + local alias_name="$3" + local gstack_dir="$4" + local source_rel upgrade_sha prior_file result + + _skill_file_matches_source "$installed_file" "$src_skill_md" "$alias_name" 1 && return 0 + [ -n "$gstack_dir" ] || return 1 + case "$src_skill_md" in + "$gstack_dir"/*) source_rel="${src_skill_md#"$gstack_dir"/}" ;; + *) return 1 ;; + esac + upgrade_sha=$(_gstack_upgrade_base_sha "$gstack_dir" 2>/dev/null || true) + [ -n "$upgrade_sha" ] || return 1 + prior_file=$(mktemp "${TMPDIR:-/tmp}/gstack-prior-alias.XXXXXX") || return 1 + if ! git -C "$gstack_dir" show "$upgrade_sha:$source_rel" > "$prior_file" 2>/dev/null; then + rm -f "$prior_file" + return 1 + fi + _skill_file_matches_source "$installed_file" "$prior_file" "$alias_name" 1 + result=$? + rm -f "$prior_file" + return "$result" +} + +_claude_alias_entry_owned() { + local src_skill_md="$1" + local dst_dir="$2" + local alias_name="$3" + local gstack_dir="${4:-${SOURCE_GSTACK_DIR:-}}" + local marker link_dest + + [ -e "$dst_dir" ] || [ -L "$dst_dir" ] || return 1 + if [ -L "$dst_dir" ]; then + link_dest=$(readlink "$dst_dir" 2>/dev/null || true) + [ "$link_dest" = "$(dirname "$src_skill_md")" ] + return $? + fi + [ -d "$dst_dir" ] || return 1 + if [ -L "$dst_dir/SKILL.md" ]; then + link_dest=$(readlink "$dst_dir/SKILL.md" 2>/dev/null || true) + [ "$link_dest" = "$src_skill_md" ] + return $? + fi + [ -f "$dst_dir/SKILL.md" ] || return 1 + + marker="" + grep -Fqx "$marker" "$dst_dir/SKILL.md" 2>/dev/null && return 0 + + # One-time migration for exact legacy generated aliases. Equality to the + # complete current/prior rewrite is the proof; a name match is not. + _legacy_claude_alias_copy "$dst_dir/SKILL.md" "$src_skill_md" "$alias_name" "$gstack_dir" +} + _install_alias_skill_md() { local src_skill_md="$1" local dst_dir="$2" local alias_name="$3" + local gstack_dir="${4:-${SOURCE_GSTACK_DIR:-}}" + local marker expected_file [ -f "$src_skill_md" ] || return 0 - # Old installs left the alias as a whole-dir symlink — replace it. - if [ -L "$dst_dir" ]; then rm -f "$dst_dir"; fi + + # A pre-existing alias must be either a legacy exact generated copy/link or + # carry our marker. Its reserved name alone never grants ownership. + if [ -e "$dst_dir" ] || [ -L "$dst_dir" ]; then + if ! _claude_alias_entry_owned "$src_skill_md" "$dst_dir" "$alias_name" "$gstack_dir"; then + echo "Error: refusing to replace non-gstack skill entry: $dst_dir" >&2 + return 2 + fi + [ -L "$dst_dir" ] && rm -f "$dst_dir" + fi + mkdir -p "$dst_dir" - # Remove any prior symlinked SKILL.md so the redirect below cannot write - # through it into the generated source. - rm -f "$dst_dir/SKILL.md" - sed "1,/^---\$/ s/^name:[[:space:]].*/name: $alias_name/" "$src_skill_md" > "$dst_dir/SKILL.md" + marker="" + expected_file=$(mktemp "$dst_dir/.gstack-alias.XXXXXX") + sed "1,/^---\$/ s/^name:[[:space:]].*/name: $alias_name/" "$src_skill_md" > "$expected_file" + # A same-directory rename replaces a prior symlink itself without following + # it, while keeping the old served file valid until the new copy is complete. + printf '\n%s\n' "$marker" >> "$expected_file" + mv -f "$expected_file" "$dst_dir/SKILL.md" +} + +# Setup has cleanup and alias phases after the main skill installation. Check +# all three destination classes up front so any user collision leaves the old +# working install completely untouched. +preflight_claude_install() { + local gstack_dir="$1" + local skills_dir="$2" + local alias_target alias_name + + preflight_claude_skill_dirs "$gstack_dir" "$skills_dir" || return $? + + alias_target="$skills_dir/_gstack-command" + if [ -f "$gstack_dir/SKILL.md" ] \ + && { [ -e "$alias_target" ] || [ -L "$alias_target" ]; } \ + && ! _claude_alias_entry_owned "$gstack_dir/SKILL.md" "$alias_target" "_gstack-command" "$gstack_dir"; then + echo "Error: refusing to replace non-gstack skill entry: $alias_target" >&2 + return 2 + fi + + alias_target="$skills_dir/connect-chrome" + alias_name="connect-chrome" + if [ "$SKILL_PREFIX" -eq 1 ]; then + alias_target="$skills_dir/gstack-connect-chrome" + alias_name="gstack-connect-chrome" + fi + if [ -f "$gstack_dir/open-gstack-browser/SKILL.md" ] \ + && { [ -e "$alias_target" ] || [ -L "$alias_target" ]; } \ + && ! _claude_alias_entry_owned "$gstack_dir/open-gstack-browser/SKILL.md" "$alias_target" "$alias_name" "$gstack_dir"; then + echo "Error: refusing to replace non-gstack skill entry: $alias_target" >&2 + return 2 + fi } # Claude Code skips the repo-shaped ~/.claude/skills/gstack directory when @@ -1013,7 +1413,7 @@ link_claude_root_skill_alias() { local target="$skills_dir/_gstack-command" [ -f "$gstack_dir/SKILL.md" ] || return 0 - _install_alias_skill_md "$gstack_dir/SKILL.md" "$target" "_gstack-command" + _install_alias_skill_md "$gstack_dir/SKILL.md" "$target" "_gstack-command" "$gstack_dir" echo " linked root skill alias: gstack" } @@ -1026,7 +1426,8 @@ cleanup_old_claude_symlinks() { local gstack_dir="$1" local skills_dir="$2" local removed=() - local old_target skill_name link_dest skill_dir + local old_target skill_name link_dest skill_dir installed_name marker alias_marker render_dir + render_dir="${GSTACK_USER_RENDER_DIR:-${GSTACK_HOME:-$HOME/.gstack}/render/claude}" # Destination scan. The glob already yields dangling dir symlinks; [ -e ] # alone would skip them, so [ -L ] keeps those entries. An unmatched `*` # literal (empty skills_dir) is rejected by the same guard. @@ -1037,28 +1438,37 @@ cleanup_old_claude_symlinks() { [ "$skill_name" = "gstack" ] && continue # Skip already-prefixed dirs (gstack-upgrade) — no old symlink to clean case "$skill_name" in gstack-*) continue ;; esac - # Remove directory symlinks pointing into gstack/ + # connect-chrome is an explicit rewritten-copy alias rather than a normal + # consumer link. Remove only the exact marker written for that one mapping. + alias_marker='' + if [ "$skill_name" = "connect-chrome" ] \ + && [ -d "$old_target" ] && [ -f "$old_target/SKILL.md" ] \ + && [ ! -L "$old_target/SKILL.md" ] \ + && grep -Fqx "$alias_marker" "$old_target/SKILL.md" 2>/dev/null; then + rm -rf "$old_target" + removed+=("$skill_name") + continue + fi + # Old installers used either this checkout's exact source path or the + # exact sibling-relative gstack/ form. A broad */gstack/* match can + # point at an unrelated user repository and is not provenance. 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 - # Remove real directories with symlinked SKILL.md pointing into gstack/ + if [ "$link_dest" = "$gstack_dir/$skill_name" ] \ + || [ "$link_dest" = "gstack/$skill_name" ]; then + rm -f "$old_target" + removed+=("$skill_name") + fi + # Likewise, a SKILL.md link is removable only when it names the exact + # canonical source/render file for this destination skill. elif [ -d "$old_target" ] && [ -L "$old_target/SKILL.md" ]; then link_dest="$(readlink "$old_target/SKILL.md" 2>/dev/null || true)" - # Anchored path segments (same as the dir-symlink arm and - # gstack-uninstall #2563). A bare *gstack* substring would wipe a - # user skill under e.g. ~/tools/gstack-fork/. Also accept the #2569 - # render prefix (~/.gstack/render/claude/...), which is not `/gstack/`. - case "$link_dest" in - gstack/*|*/gstack/*|*/.gstack/render/claude/*) - rm -rf "$old_target" - removed+=("$skill_name") - ;; - esac + if [ "$link_dest" = "$gstack_dir/$skill_name/SKILL.md" ] \ + || [ "$link_dest" = "$render_dir/$skill_name/SKILL.md" ] \ + || [ "$link_dest" = "gstack/$skill_name/SKILL.md" ]; then + rm -rf "$old_target" + removed+=("$skill_name") + fi fi done # Windows install pattern: real dir with real-file SKILL.md (no symlink @@ -1075,8 +1485,12 @@ cleanup_old_claude_symlinks() { old_target="$skills_dir/$skill_name" if [ -d "$old_target" ] && [ ! -L "$old_target" ] \ && [ -f "$old_target/SKILL.md" ] && [ ! -L "$old_target/SKILL.md" ]; then - rm -rf "$old_target" - removed+=("$skill_name") + installed_name=$(grep -m1 '^name:' "$old_target/SKILL.md" 2>/dev/null | sed 's/^name:[[:space:]]*//' | tr -d '[:space:]' || true) + marker="" + if [ "$installed_name" = "$skill_name" ] && grep -Fqx "$marker" "$old_target/SKILL.md" 2>/dev/null; then + rm -rf "$old_target" + removed+=("$skill_name") + fi fi fi done @@ -1093,6 +1507,8 @@ cleanup_prefixed_claude_symlinks() { local gstack_dir="$1" local skills_dir="$2" local removed=() + local skill_dir skill_name prefixed_target link_dest installed_name marker alias_marker render_dir + render_dir="${GSTACK_USER_RENDER_DIR:-${GSTACK_HOME:-$HOME/.gstack}/render/claude}" for skill_dir in "$gstack_dir"/*/; do if [ -f "$skill_dir/SKILL.md" ]; then skill_name="$(basename "$skill_dir")" @@ -1104,27 +1520,29 @@ cleanup_prefixed_claude_symlinks() { # Remove directory symlinks pointing into gstack/ if [ -L "$prefixed_target" ]; then link_dest="$(readlink "$prefixed_target" 2>/dev/null || true)" - case "$link_dest" in - gstack/*|*/gstack/*) - rm -f "$prefixed_target" - removed+=("gstack-$skill_name") - ;; - esac + if [ "$link_dest" = "$gstack_dir/$skill_name" ] || [ "$link_dest" = "gstack/$skill_name" ]; then + rm -f "$prefixed_target" + removed+=("gstack-$skill_name") + fi # Remove real directories with symlinked SKILL.md pointing into gstack/ elif [ -d "$prefixed_target" ] && [ -L "$prefixed_target/SKILL.md" ]; then link_dest="$(readlink "$prefixed_target/SKILL.md" 2>/dev/null || true)" - case "$link_dest" in - *gstack*) - rm -rf "$prefixed_target" - removed+=("gstack-$skill_name") - ;; - esac - # Windows install pattern: real dir with real-file SKILL.md. Same - # reasoning as cleanup_old_claude_symlinks — directory name match plus - # IS_WINDOWS is safe during a mode flip. - elif [ "$IS_WINDOWS" -eq 1 ] && [ -d "$prefixed_target" ] && [ -f "$prefixed_target/SKILL.md" ]; then - rm -rf "$prefixed_target" - removed+=("gstack-$skill_name") + if [ "$link_dest" = "$gstack_dir/$skill_name/SKILL.md" ] || [ "$link_dest" = "$render_dir/$skill_name/SKILL.md" ]; then + rm -rf "$prefixed_target" + removed+=("gstack-$skill_name") + fi + # Rewritten copies require both their exact frontmatter and the exact + # source+served ownership marker. A reserved name alone is user data. + elif [ -d "$prefixed_target" ] && [ -f "$prefixed_target/SKILL.md" ]; then + installed_name=$(grep -m1 '^name:' "$prefixed_target/SKILL.md" 2>/dev/null | sed 's/^name:[[:space:]]*//' | tr -d '[:space:]' || true) + marker="" + alias_marker="" + if [ "$installed_name" = "gstack-$skill_name" ] \ + && { grep -Fqx "$marker" "$prefixed_target/SKILL.md" 2>/dev/null \ + || { [ "$skill_name" = "connect-chrome" ] && grep -Fqx "$alias_marker" "$prefixed_target/SKILL.md" 2>/dev/null; }; }; then + rm -rf "$prefixed_target" + removed+=("gstack-$skill_name") + fi fi fi done @@ -1168,11 +1586,13 @@ link_codex_skill_dirs() { # #2142: a real dir may only be replaced when it is provably ours # (_owned_for_windows_refresh), never a user's own colliding dir. if [ "$IS_WINDOWS" -eq 1 ] || [ -L "$target" ] || [ ! -e "$target" ]; then - if _owned_for_windows_refresh "$target"; then + if _owned_for_windows_refresh "$target" "$skill_dir" "codex" "$skill_name"; then _link_or_copy "$skill_dir" "$target" + _mark_windows_skill_copy "$target" "$skill_dir" "codex" "$skill_name" linked+=("$skill_name") else - echo " left in place (existing dir not gstack-managed — no generated banner): $target" >&2 + echo "Error: refusing to replace non-gstack skill entry: $target" >&2 + return 2 fi fi fi @@ -1193,14 +1613,14 @@ create_agents_sidecar() { # user's — never write into it (the Windows branch would rm -rf its # subdirs on every re-run). if _sidecar_root_user_owned "$agents_gstack"; then - echo " left in place (existing dir not gstack-managed — no generated banner): $agents_gstack" >&2 + echo " left in place (existing root is not the exact generated gstack sidecar): $agents_gstack" >&2 return 0 fi mkdir -p "$agents_gstack" # Sidecar directories that skills reference at runtime. bin scripts import # shared modules via ../lib, so bin and lib must always travel together. - for asset in bin lib browse review qa; do + for asset in bin lib browse review qa scripts; do local src="$SOURCE_GSTACK_DIR/$asset" local dst="$agents_gstack/$asset" if [ -d "$src" ] || [ -f "$src" ]; then @@ -1242,13 +1662,7 @@ create_codex_runtime_root() { local codex_gstack="$2" local agents_dir="$gstack_dir/.agents/skills" - if [ -L "$codex_gstack" ]; then - rm -f "$codex_gstack" - elif [ -d "$codex_gstack" ] && [ "$codex_gstack" != "$gstack_dir" ]; then - # Old direct installs left a real directory here with stale source skills. - # Remove it so we start fresh with only the minimal runtime assets. - rm -rf "$codex_gstack" - fi + _prepare_runtime_root "$codex_gstack" "$gstack_dir" "$agents_dir/gstack/SKILL.md" "codex" mkdir -p "$codex_gstack" "$codex_gstack/browse" "$codex_gstack/gstack-upgrade" "$codex_gstack/review" @@ -1261,6 +1675,9 @@ create_codex_runtime_root() { if [ -d "$gstack_dir/lib" ]; then _link_or_copy "$gstack_dir/lib" "$codex_gstack/lib" fi + if [ -d "$gstack_dir/scripts" ]; then + _link_or_copy "$gstack_dir/scripts" "$codex_gstack/scripts" + fi if [ -d "$gstack_dir/browse/dist" ]; then _link_or_copy "$gstack_dir/browse/dist" "$codex_gstack/browse/dist" fi @@ -1299,11 +1716,7 @@ create_factory_runtime_root() { local factory_gstack="$2" local factory_dir="$gstack_dir/.factory/skills" - if [ -L "$factory_gstack" ]; then - rm -f "$factory_gstack" - elif [ -d "$factory_gstack" ] && [ "$factory_gstack" != "$gstack_dir" ]; then - rm -rf "$factory_gstack" - fi + _prepare_runtime_root "$factory_gstack" "$gstack_dir" "$factory_dir/gstack/SKILL.md" "factory" mkdir -p "$factory_gstack" "$factory_gstack/browse" "$factory_gstack/gstack-upgrade" "$factory_gstack/review" @@ -1316,6 +1729,9 @@ create_factory_runtime_root() { if [ -d "$gstack_dir/lib" ]; then _link_or_copy "$gstack_dir/lib" "$factory_gstack/lib" fi + if [ -d "$gstack_dir/scripts" ]; then + _link_or_copy "$gstack_dir/scripts" "$factory_gstack/scripts" + fi if [ -d "$gstack_dir/browse/dist" ]; then _link_or_copy "$gstack_dir/browse/dist" "$factory_gstack/browse/dist" fi @@ -1352,11 +1768,7 @@ create_opencode_runtime_root() { local opencode_gstack="$2" local opencode_dir="$gstack_dir/.opencode/skills" - if [ -L "$opencode_gstack" ]; then - rm -f "$opencode_gstack" - elif [ -d "$opencode_gstack" ] && [ "$opencode_gstack" != "$gstack_dir" ]; then - rm -rf "$opencode_gstack" - fi + _prepare_runtime_root "$opencode_gstack" "$gstack_dir" "$opencode_dir/gstack/SKILL.md" "opencode" mkdir -p "$opencode_gstack" "$opencode_gstack/browse" "$opencode_gstack/design" "$opencode_gstack/gstack-upgrade" "$opencode_gstack/review" "$opencode_gstack/qa" "$opencode_gstack/plan-devex-review" @@ -1369,6 +1781,9 @@ create_opencode_runtime_root() { if [ -d "$gstack_dir/lib" ]; then _link_or_copy "$gstack_dir/lib" "$opencode_gstack/lib" fi + if [ -d "$gstack_dir/scripts" ]; then + _link_or_copy "$gstack_dir/scripts" "$opencode_gstack/scripts" + fi if [ -d "$gstack_dir/browse/dist" ]; then _link_or_copy "$gstack_dir/browse/dist" "$opencode_gstack/browse/dist" fi @@ -1443,11 +1858,13 @@ link_factory_skill_dirs() { # #2142: a real dir may only be replaced when it is provably ours # (_owned_for_windows_refresh), never a user's own colliding dir. if [ "$IS_WINDOWS" -eq 1 ] || [ -L "$target" ] || [ ! -e "$target" ]; then - if _owned_for_windows_refresh "$target"; then + if _owned_for_windows_refresh "$target" "$skill_dir" "factory" "$skill_name"; then _link_or_copy "$skill_dir" "$target" + _mark_windows_skill_copy "$target" "$skill_dir" "factory" "$skill_name" linked+=("$skill_name") else - echo " left in place (existing dir not gstack-managed — no generated banner): $target" >&2 + echo "Error: refusing to replace non-gstack skill entry: $target" >&2 + return 2 fi fi fi @@ -1485,11 +1902,13 @@ link_opencode_skill_dirs() { # #2142: a real dir may only be replaced when it is provably ours # (_owned_for_windows_refresh), never a user's own colliding dir. if [ "$IS_WINDOWS" -eq 1 ] || [ -L "$target" ] || [ ! -e "$target" ]; then - if _owned_for_windows_refresh "$target"; then + if _owned_for_windows_refresh "$target" "$skill_dir" "opencode" "$skill_name"; then _link_or_copy "$skill_dir" "$target" + _mark_windows_skill_copy "$target" "$skill_dir" "opencode" "$skill_name" linked+=("$skill_name") else - echo " left in place (existing dir not gstack-managed — no generated banner): $target" >&2 + echo "Error: refusing to replace non-gstack skill entry: $target" >&2 + return 2 fi fi fi @@ -1507,18 +1926,8 @@ create_cursor_runtime_root() { local gstack_dir="$1" local cursor_gstack="$2" local cursor_dir="$gstack_dir/.cursor/skills" - local generated_root="$cursor_dir/gstack" - - if [ -L "$cursor_gstack" ]; then - rm -f "$cursor_gstack" - elif _sidecar_root_user_owned "$cursor_gstack"; then - # #2142: a hand-written skill squatting on the canonical name is the - # user's — never wipe it to make room for the runtime root. - echo " left in place (existing dir not gstack-managed — no generated banner): $cursor_gstack" >&2 - return 0 - elif [ -d "$cursor_gstack" ] && [ "$cursor_gstack" != "$gstack_dir" ] && [ "$cursor_gstack" != "$generated_root" ]; then - rm -rf "$cursor_gstack" - fi + + _prepare_runtime_root "$cursor_gstack" "$gstack_dir" "$cursor_dir/gstack/SKILL.md" "cursor" mkdir -p "$cursor_gstack" "$cursor_gstack/browse" "$cursor_gstack/gstack-upgrade" "$cursor_gstack/review" @@ -1532,6 +1941,9 @@ create_cursor_runtime_root() { if [ -d "$gstack_dir/lib" ]; then _link_or_copy "$gstack_dir/lib" "$cursor_gstack/lib" fi + if [ -d "$gstack_dir/scripts" ]; then + _link_or_copy "$gstack_dir/scripts" "$cursor_gstack/scripts" + fi if [ -d "$gstack_dir/browse/dist" ]; then _link_or_copy "$gstack_dir/browse/dist" "$cursor_gstack/browse/dist" fi @@ -1569,7 +1981,7 @@ create_cursor_sidecar() { # generated tree's own root (cursor_dir/gstack carries the banner) always # passes, so normal installs refresh as before. if _sidecar_root_user_owned "$cursor_gstack"; then - echo " left in place (existing dir not gstack-managed — no generated banner): $cursor_gstack" >&2 + echo " left in place (existing root is not the exact generated gstack sidecar): $cursor_gstack" >&2 return 0 fi @@ -1585,6 +1997,11 @@ create_cursor_sidecar() { _link_or_copy "$repo_root/lib" "$cursor_gstack/lib" fi fi + if [ -d "$repo_root/scripts" ]; then + if [ "$IS_WINDOWS" -eq 1 ] || [ -L "$cursor_gstack/scripts" ] || [ ! -e "$cursor_gstack/scripts" ]; then + _link_or_copy "$repo_root/scripts" "$cursor_gstack/scripts" + fi + fi if [ -d "$repo_root/browse/dist" ]; then if [ "$IS_WINDOWS" -eq 1 ] || [ -L "$cursor_gstack/browse/dist" ] || [ ! -e "$cursor_gstack/browse/dist" ]; then _link_or_copy "$repo_root/browse/dist" "$cursor_gstack/browse/dist" @@ -1640,11 +2057,13 @@ link_cursor_skill_dirs() { # PROVABLY gstack-managed real dir; never a user's own Cursor skill # dir that merely starts with gstack (#2142). if [ "$IS_WINDOWS" -eq 1 ] || [ -L "$target" ] || [ ! -e "$target" ]; then - if _owned_for_windows_refresh "$target"; then + if _owned_for_windows_refresh "$target" "$skill_dir" "cursor" "$skill_name"; then _link_or_copy "$skill_dir" "$target" + _mark_windows_skill_copy "$target" "$skill_dir" "cursor" "$skill_name" linked+=("$skill_name") else - echo " left in place (existing dir not gstack-managed — no generated banner): $target" >&2 + echo "Error: refusing to replace non-gstack skill entry: $target" >&2 + return 2 fi fi fi @@ -1664,15 +2083,13 @@ fi if [ "$INSTALL_CLAUDE" -eq 1 ]; then if [ "$SKILLS_BASENAME" = "skills" ]; then + preflight_claude_install "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" # Clean up stale symlinks from the opposite prefix mode if [ "$SKILL_PREFIX" -eq 1 ]; then cleanup_old_claude_symlinks "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" else cleanup_prefixed_claude_symlinks "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" fi - # Patch name: fields BEFORE creating symlinks so link_claude_skill_dirs - # reads the correct (patched) name: values for symlink naming - "$SOURCE_GSTACK_DIR/bin/gstack-patch-names" "$SOURCE_GSTACK_DIR" "$SKILL_PREFIX" link_claude_skill_dirs "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" link_claude_root_skill_alias "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" _CLAUDE_SKILLS_LINKED=1 @@ -1738,17 +2155,17 @@ if [ "$INSTALL_CLAUDE" -eq 1 ]; then log " browse: $BROWSE_BIN" else mkdir -p "$CLAUDE_SKILLS_DIR" - _link_or_copy "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK" - log " symlinked $CLAUDE_GSTACK_LINK -> $SOURCE_GSTACK_DIR" INSTALL_SKILLS_DIR="$CLAUDE_SKILLS_DIR" INSTALL_GSTACK_DIR="$CLAUDE_GSTACK_LINK" + preflight_claude_install "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" + _link_or_copy "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK" + log " symlinked $CLAUDE_GSTACK_LINK -> $SOURCE_GSTACK_DIR" # Clean up stale symlinks from the opposite prefix mode if [ "$SKILL_PREFIX" -eq 1 ]; then cleanup_old_claude_symlinks "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" else cleanup_prefixed_claude_symlinks "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" fi - "$SOURCE_GSTACK_DIR/bin/gstack-patch-names" "$SOURCE_GSTACK_DIR" "$SKILL_PREFIX" link_claude_skill_dirs "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" link_claude_root_skill_alias "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" _CLAUDE_SKILLS_LINKED=1 @@ -1766,7 +2183,7 @@ if [ "$INSTALL_CLAUDE" -eq 1 ]; then _OGB_LINK="$INSTALL_SKILLS_DIR/gstack-connect-chrome" _OGB_ALIAS_NAME="gstack-connect-chrome" fi - _install_alias_skill_md "$SOURCE_GSTACK_DIR/open-gstack-browser/SKILL.md" "$_OGB_LINK" "$_OGB_ALIAS_NAME" + _install_alias_skill_md "$SOURCE_GSTACK_DIR/open-gstack-browser/SKILL.md" "$_OGB_LINK" "$_OGB_ALIAS_NAME" "$SOURCE_GSTACK_DIR" log "gstack ready (claude)." log " browse: $BROWSE_BIN" fi @@ -1822,6 +2239,7 @@ if [ "$INSTALL_KIRO" -eq 1 ]; then mkdir -p "$KIRO_GSTACK" "$KIRO_GSTACK/browse" "$KIRO_GSTACK/gstack-upgrade" "$KIRO_GSTACK/review" _link_or_copy "$SOURCE_GSTACK_DIR/bin" "$KIRO_GSTACK/bin" _link_or_copy "$SOURCE_GSTACK_DIR/lib" "$KIRO_GSTACK/lib" + _link_or_copy "$SOURCE_GSTACK_DIR/scripts" "$KIRO_GSTACK/scripts" _link_or_copy "$SOURCE_GSTACK_DIR/browse/dist" "$KIRO_GSTACK/browse/dist" _link_or_copy "$SOURCE_GSTACK_DIR/browse/bin" "$KIRO_GSTACK/browse/bin" # ETHOS.md — referenced by "Search Before Building" in all skill preambles diff --git a/test/gbrain-detect-install.test.ts b/test/gbrain-detect-install.test.ts index 9109ec3041..8f489653c3 100644 --- a/test/gbrain-detect-install.test.ts +++ b/test/gbrain-detect-install.test.ts @@ -21,10 +21,10 @@ const ROOT = path.resolve(import.meta.dir, '..'); const DETECT = path.join(ROOT, 'bin', 'gstack-gbrain-detect'); const INSTALL = path.join(ROOT, 'bin', 'gstack-gbrain-install'); -// Minimal PATH with POSIX tools + homebrew (for jq/git/curl) but no user-bin -// dirs — this keeps `gbrain` out of PATH deterministically across dev machines -// while still finding jq, git, curl, sed, cat, etc. Each test can prepend a -// fake-gbrain dir when it wants to simulate presence. +// Minimal PATH with system POSIX tools but no package-manager or user-bin dirs. +// This keeps any real `gbrain` out of PATH deterministically across dev +// machines while still finding jq, git, curl, sed, cat, etc. Each test can +// prepend a fake-gbrain dir when it wants to simulate presence. // Deterministic PATH for spawned children — but it must still contain the // bun runtime itself: the bin's `#!/usr/bin/env -S bun run` shebang resolves // bun from PATH, and CI installs bun outside the standard dirs (~/.bun/bin), @@ -34,7 +34,7 @@ const INSTALL = path.join(ROOT, 'bin', 'gstack-gbrain-install'); // symlink to bun and nothing else. const BUN_ONLY_DIR = fs.mkdtempSync(path.join(os.tmpdir(), 'bun-only-')); fs.symlinkSync(process.execPath, path.join(BUN_ONLY_DIR, 'bun')); -const SAFE_PATH = `/usr/bin:/bin:/usr/sbin:/sbin:/opt/homebrew/bin:/usr/local/bin:${BUN_ONLY_DIR}`; +const SAFE_PATH = `/usr/bin:/bin:/usr/sbin:/sbin:${BUN_ONLY_DIR}`; let tmpHome: string; let tmpHomeReal: string; diff --git a/test/gbrain-dream-stage.test.ts b/test/gbrain-dream-stage.test.ts index c5f3b6814e..31eacea315 100644 --- a/test/gbrain-dream-stage.test.ts +++ b/test/gbrain-dream-stage.test.ts @@ -253,8 +253,10 @@ describe("classifyDreamOutcome — post-flight truth guard", () => { expect(classifyDreamOutcome(LOG.builtEdges)).toBeNull(); }); - it("returns null when no recognizable signal is present (degrade to success)", () => { - expect(classifyDreamOutcome(LOG.noEdgeLine)).toBeNull(); + it("fails closed when no resolver completion signal is present", () => { + const warning = classifyDreamOutcome(LOG.noEdgeLine); + expect(warning).not.toBeNull(); + expect(warning).toContain("readiness is unknown"); }); }); diff --git a/test/gbrain-ready-contract.test.ts b/test/gbrain-ready-contract.test.ts new file mode 100644 index 0000000000..06401349a6 --- /dev/null +++ b/test/gbrain-ready-contract.test.ts @@ -0,0 +1,224 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { spawnSync } from "node:child_process"; + +const root = join(import.meta.dir, ".."); +const readinessPath = join(root, "bin", "gstack-gbrain-ready"); +const readiness = readFileSync(readinessPath, "utf8"); +const skillStart = readFileSync(join(root, "bin", "gstack-skill-start"), "utf8"); +const sessionUpdate = readFileSync(join(root, "bin", "gstack-session-update"), "utf8"); +const created: string[] = []; + +afterEach(() => { + for (const path of created.splice(0)) rmSync(path, { recursive: true, force: true }); +}); + +function fixture(pin?: string, sourcePath?: string) { + const base = mkdtempSync(join(tmpdir(), "gstack-gbrain-ready-")); + created.push(base); + const repo = join(base, "repo"); + const bin = join(base, "bin"); + const state = join(base, "state"); + const log = join(base, "gbrain.log"); + const rpcInput = join(base, "gbrain-rpc-input.ndjson"); + mkdirSync(repo); + mkdirSync(bin); + spawnSync("git", ["init", "--quiet", "-b", "main"], { cwd: repo, timeout: 30_000 }); + writeFileSync(join(repo, "sample.ts"), "export function alpha() { return 1; }\n"); + spawnSync("git", ["add", "sample.ts"], { cwd: repo, timeout: 30_000 }); + spawnSync("git", ["-c", "user.name=Test", "-c", "user.email=test@example.com", "commit", "--quiet", "-m", "fixture"], { cwd: repo, timeout: 30_000 }); + if (pin !== undefined) writeFileSync(join(repo, ".gbrain-source"), `${pin}\n`); + + writeFileSync(join(bin, "gbrain"), `#!/bin/sh +printf '%s\\n' "$*" >> "$GSTACK_TEST_GBRAIN_LOG" +case "$*" in + "sources list --json") printf '%s\\n' '{"sources":[{"id":"valid-source","local_path":"${sourcePath ?? repo}"}]}' ;; + "code-def alpha --json") printf '%s\\n' '{"ready":true,"count":1}' ;; + "code-callers alpha --json") printf '%s\\n' '{"ready":true,"count":1}' ;; + "serve --surface full") + cat > "$GSTACK_TEST_GBRAIN_RPC_INPUT" + printf '%s\\n' '{"jsonrpc":"2.0","id":2,"result":{"content":[{"text":"{\\"ready\\":true,\\"result\\":\\"ok\\",\\"depth_groups\\":[{\\"nodes\\":[{\\"symbol\\":\\"alpha\\"}]}]}"}]}}' + ;; + *) exit 91 ;; +esac +`); + chmodSync(join(bin, "gbrain"), 0o755); + return { base, repo, bin, state, log, rpcInput }; +} + +function run( + repo: string, + bin: string, + state: string, + log: string, + timeoutSeconds = "1", + extraEnv: Record = {}, +) { + const started = Date.now(); + const result = spawnSync(readinessPath, ["--check-only", "--session-key", "test-session"], { + cwd: repo, + encoding: "utf8", + timeout: 10_000, + env: { + ...process.env, + GSTACK_HOME: state, + GSTACK_TEST_GBRAIN_LOG: log, + GSTACK_TEST_GBRAIN_RPC_INPUT: join(state, "..", "gbrain-rpc-input.ndjson"), + GSTACK_GBRAIN_READY_TIMEOUT_SECONDS: timeoutSeconds, + PATH: `${bin}:${process.env.PATH || ""}`, + ...extraEnv, + }, + }); + return { result, elapsed: Date.now() - started }; +} + +describe("GBrain readiness contract", () => { + test("a missing pin fails closed without invoking GBrain or starting repair", () => { + const f = fixture(); + const { result } = run(f.repo, f.bin, f.state, f.log); + expect(result.status).toBe(0); + expect(result.stdout).toContain("reason=source_pin_missing"); + expect(existsSync(f.log)).toBe(false); + expect(existsSync(f.state)).toBe(false); + }); + + test("an invalid source id is rejected before it reaches a command or state path", () => { + const f = fixture("../../escape"); + const { result } = run(f.repo, f.bin, f.state, f.log); + expect(result.status).toBe(0); + expect(result.stdout).toContain("reason=source_pin_invalid"); + expect(existsSync(f.log)).toBe(false); + expect(existsSync(f.state)).toBe(false); + }); + + test("a stale path fails closed after one read-only source probe", () => { + const other = mkdtempSync(join(tmpdir(), "gstack-gbrain-other-")); + created.push(other); + const f = fixture("valid-source", other); + const { result } = run(f.repo, f.bin, f.state, f.log); + expect(result.status).toBe(0); + expect(result.stdout).toContain("reason=source_path_mismatch"); + expect(readFileSync(f.log, "utf8").trim()).toBe("sources list --json"); + }); + + test("healthy readiness uses only bounded read probes and emits partial authority", () => { + const f = fixture("valid-source"); + const { result } = run(f.repo, f.bin, f.state, f.log); + expect(result.status).toBe(0); + expect(result.stdout).toContain("GBRAIN_GRAPH: partial"); + expect(result.stdout).toContain("canary=alpha"); + const commands = readFileSync(f.log, "utf8"); + expect(commands).toContain("sources list --json"); + expect(commands).toContain("code-def alpha --json"); + expect(commands).toContain("code-callers alpha --json"); + expect(commands).toContain("serve --surface full"); + expect(commands).not.toMatch(/\b(sync|dream|import|embed|edges-backfill|sources (add|attach|set-strategy))\b/); + + const rpcText = readFileSync(f.rpcInput, "utf8"); + expect(rpcText).not.toContain('\\"jsonrpc\\"'); + const rpcLines = rpcText.trim().split("\n"); + expect(rpcLines).toHaveLength(3); + expect(rpcLines.every((line) => line.length > 0)).toBe(true); + expect(rpcLines.map((line) => JSON.parse(line))).toEqual([ + { + jsonrpc: "2.0", + id: 1, + method: "initialize", + params: { + protocolVersion: "2025-03-26", + capabilities: {}, + clientInfo: { name: "gstack-graph-canary", version: "1" }, + }, + }, + { jsonrpc: "2.0", method: "notifications/initialized", params: {} }, + { + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { + name: "code_blast", + arguments: { symbol: "alpha", source_id: "valid-source", depth: 1, max_nodes: 1 }, + }, + }, + ]); + }); + + test("a hung foreground source probe times out instead of hanging the skill", () => { + const f = fixture("valid-source"); + writeFileSync(join(f.bin, "gbrain"), `#!/bin/sh +printf '%s\\n' "$*" >> "$GSTACK_TEST_GBRAIN_LOG" +sleep 30 +`); + chmodSync(join(f.bin, "gbrain"), 0o755); + const { result, elapsed } = run(f.repo, f.bin, f.state, f.log, "1"); + expect(result.status).toBe(0); + expect(result.stdout).toContain("reason=sources_probe_failed_or_timed_out"); + expect(elapsed).toBeLessThan(5_000); + }); + + test("timeout kills the entire probe group, including a TERM/HUP-resistant pipe holder", async () => { + const f = fixture("valid-source"); + const descendantPid = join(f.base, "descendant.pid"); + writeFileSync(join(f.bin, "gbrain"), `#!/bin/sh +printf '%s\\n' "$*" >> "$GSTACK_TEST_GBRAIN_LOG" +case "$*" in + "sources list --json") + ( + trap '' TERM HUP + while :; do sleep 1; done + ) & + child=$! + printf '%s\\n' "$child" > "$GSTACK_TEST_DESCENDANT_PID" + wait "$child" + ;; + *) exit 91 ;; +esac +`); + chmodSync(join(f.bin, "gbrain"), 0o755); + + let pid = 0; + try { + const { result, elapsed } = run(f.repo, f.bin, f.state, f.log, "1", { + GSTACK_TEST_DESCENDANT_PID: descendantPid, + }); + expect(result.status).toBe(0); + expect(result.stdout).toContain("reason=sources_probe_failed_or_timed_out"); + expect(elapsed).toBeLessThan(5_000); + expect(existsSync(descendantPid)).toBe(true); + pid = Number(readFileSync(descendantPid, "utf8").trim()); + expect(pid).toBeGreaterThan(0); + + const deadline = Date.now() + 2_000; + while (Date.now() < deadline) { + try { + process.kill(pid, 0); + await new Promise((resolve) => setTimeout(resolve, 25)); + } catch { + pid = 0; + break; + } + } + expect(pid).toBe(0); + } finally { + if (pid > 0) { + try { process.kill(pid, "SIGKILL"); } catch { pid = 0; } + } + } + }, 10_000); + + test("session and skill startup never launch automatic GBrain repair", () => { + expect(sessionUpdate).not.toContain("gstack-gbrain-ready"); + expect(skillStart).toContain('"$_GBRAIN_READY_BIN" --check-only --session-key'); + expect(skillStart).not.toMatch(/gstack-gbrain-ready[^\n]*&/); + expect(skillStart).toContain("/sync-gbrain --full"); + expect(readiness).not.toMatch(/gstack-gbrain-sync|gbrain (sync|dream|embed|edges-backfill)|sources (add|attach|set-strategy)/); + }); + + test("every GBrain invocation in readiness is wrapped by the timeout helper", () => { + const invocations = readiness.split("\n").filter((line) => /\bgbrain (sources|code-|serve)/.test(line)); + expect(invocations.length).toBeGreaterThanOrEqual(4); + for (const invocation of invocations) expect(invocation).toContain("run_bounded"); + }); +}); diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index cc4a8eaaf9..7f166f9446 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -2554,9 +2554,9 @@ describe('setup script validation', () => { expect(fnBody).toContain('gstack*'); }); - test('link_claude_skill_dirs creates real directories with absolute SKILL.md symlinks', () => { - // Claude links should be real directories with absolute SKILL.md symlinks - // to ensure Claude Code discovers them as top-level skills (not nested under gstack/) + test('link_claude_skill_dirs keeps canonical source immutable in prefix mode', () => { + // Claude entries stay top-level real directories. Flat mode can symlink + // canonical SKILL.md; prefix mode must create a rewritten consumer copy. const fnStart = setupContent.indexOf('link_claude_skill_dirs()'); const fnEnd = setupContent.indexOf('}', setupContent.indexOf('linked[@]}', fnStart)); const fnBody = setupContent.slice(fnStart, fnEnd); @@ -2565,7 +2565,11 @@ describe('setup script validation', () => { // v1.67 (#2569): the source is render-aware — canonical SKILL.md, or the // rendered :user variant from ${GSTACK_HOME}/render/claude when present. expect(fnBody).toContain('_skill_md_src="$gstack_dir/$dir_name/SKILL.md"'); + expect(fnBody).toContain( + '_install_managed_skill_md "$_skill_md_src" "$target" "$dir_name" "$link_name" 1', + ); expect(fnBody).toContain('_link_or_copy "$_skill_md_src" "$target/SKILL.md"'); + expect(fnBody).not.toContain('.gstack-managed'); }); // REGRESSION: cleanup functions must handle both old symlinks AND new real-directory pattern diff --git a/test/gstack-config-key-locale.test.ts b/test/gstack-config-key-locale.test.ts index 599a0c168e..49484946a3 100644 --- a/test/gstack-config-key-locale.test.ts +++ b/test/gstack-config-key-locale.test.ts @@ -21,12 +21,8 @@ function run(args: string[]) { const result = spawnSync(CONFIG, args, { encoding: "utf8", // GSTACK_SETUP_RUNNING suppresses `set skill_prefix`'s auto-relink side - // effect. Without it, this test invokes the REPO's gstack-config, whose - // auto-relink resolves the install dir from its own path — i.e. the repo — - // and gstack-patch-names rewrites all 52 tracked SKILL.md files to - // gstack-prefixed names, poisoning every downstream test that reads the - // live tree (observed in the free-tests CI job). Relink behavior itself is - // covered in isolation by test/relink.test.ts's mock install. + // effect so this locale test stays scoped to config parsing. Relink behavior + // and source-tree immutability are covered in test/relink.test.ts. env: { ...process.env, GSTACK_STATE_ROOT: stateRoot, GSTACK_SETUP_RUNNING: "1" }, timeout: 30_000, }); diff --git a/test/gstack-gbrain-sync.test.ts b/test/gstack-gbrain-sync.test.ts index 67250dc454..fb2071edb0 100644 --- a/test/gstack-gbrain-sync.test.ts +++ b/test/gstack-gbrain-sync.test.ts @@ -81,6 +81,7 @@ describe("gstack-gbrain-sync CLI", () => { // (NOT gbrain import — that's the markdown-only path that was rejected post-codex). expect(r.stdout).toContain("would: gbrain sources add"); expect(r.stdout).toContain("gbrain sync --strategy code"); + expect(r.stdout).toContain("gbrain sources set-strategy"); expect(r.stdout).not.toContain("gbrain import"); // memory + brain-sync stages should not appear expect(r.stdout).not.toContain("gstack-memory-ingest --probe"); @@ -168,7 +169,7 @@ exit 99 rmSync(home, { recursive: true, force: true }); }); - it("keeps a symlink-equivalent pinned source registered as-is", () => { + it("keeps a symlink-equivalent pinned source registered as-is and requires durable code strategy", () => { const home = makeTestHome(); const gstackHome = join(home, ".gstack"); const repo = mkdtempSync(join(tmpdir(), "gstack-pinned-source-repo-")); @@ -185,8 +186,14 @@ exit 99 writeFileSync(join(bindir, "gbrain"), `#!/bin/sh printf '%s\\n' "$*" >> "$GSTACK_TEST_GBRAIN_LOG" case "$*" in - --version) echo 'gbrain 0.42.0.0' ;; + --version) echo 'gbrain 0.48.2.0' ;; "sources list --json") echo '{"sources":[{"id":"client-acme-app","local_path":"${link}","page_count":1}]}' ;; + "sources set-strategy client-acme-app code") + if [ "\${GSTACK_TEST_FAIL_SET_STRATEGY:-0}" = "1" ]; then + echo "set-strategy failed" >&2 + exit 23 + fi + ;; "sync --strategy code --source client-acme-app"|"sources attach client-acme-app") ;; *) echo "unexpected gbrain command: $*" >&2; exit 1 ;; esac @@ -218,9 +225,31 @@ esac const commands = readFileSync(commandLog, "utf-8"); expect(r.status).toBe(0); + expect(commands).toContain("sources set-strategy client-acme-app code"); expect(commands).toContain("sync --strategy code --source client-acme-app"); expect(commands).toContain("sources attach client-acme-app"); expect(commands).not.toMatch(/^sources (add|remove) /m); + + writeFileSync(commandLog, ""); + const failedStrategy = spawnSync("bun", [SCRIPT, "--code-only", "--quiet"], { + encoding: "utf-8", + timeout: 60000, + cwd: link, + env: { + ...process.env, + HOME: home, + GSTACK_HOME: gstackHome, + GBRAIN_HOME: "", + GSTACK_TEST_GBRAIN_LOG: commandLog, + GSTACK_TEST_FAIL_SET_STRATEGY: "1", + PATH: `${bindir}:${process.env.PATH || ""}`, + }, + }); + const failedCommands = readFileSync(commandLog, "utf-8"); + expect(failedStrategy.status).toBe(1); + expect(failedCommands).toContain("sources set-strategy client-acme-app code"); + expect(failedCommands).not.toContain("sync --strategy code --source client-acme-app"); + expect(failedCommands).not.toContain("sources attach client-acme-app"); rmSync(repo, { recursive: true, force: true }); rmSync(linkDir, { recursive: true, force: true }); rmSync(bindir, { recursive: true, force: true }); @@ -246,7 +275,7 @@ esac }); expect(r.status).toBe(0); - expect(r.stdout).toContain("gbrain dream --source client-acme-app"); + expect(r.stdout).toContain("gbrain dream --source client-acme-app --phase resolve_symbol_edges"); rmSync(repo, { recursive: true, force: true }); rmSync(bindir, { recursive: true, force: true }); rmSync(home, { recursive: true, force: true }); diff --git a/test/relink.test.ts b/test/relink.test.ts index 8f18d7281e..f6fff3616e 100644 --- a/test/relink.test.ts +++ b/test/relink.test.ts @@ -1,5 +1,5 @@ import { describe, test as _bunTest, expect, beforeEach, afterEach } from 'bun:test'; -import { execSync } from 'child_process'; +import { execSync, spawnSync } from 'child_process'; import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; @@ -29,11 +29,12 @@ function run(cmd: string, env: Record = {}, expectFail = false): // set on process.env; relink/config children must resolve state ONLY // via the dirs this test passes (observed: 'fresh install' test saw a // neighbor's skill_prefix and produced prefixed names). - env: (() => { - const child: Record = { ...process.env, GSTACK_STATE_DIR: tmpDir, ...env }; - if (!('GSTACK_HOME' in env)) delete child.GSTACK_HOME; - return child; - })(), + env: { + ...process.env, + GSTACK_STATE_DIR: tmpDir, + GSTACK_HOME: tmpDir, + ...env, + }, encoding: 'utf-8', timeout: 10000, stdio: ['pipe', 'pipe', 'pipe'], @@ -75,6 +76,24 @@ function setupMockInstall(skills: string[]): void { } } +function readSkillName(skillDir: string): string | null { + const content = fs.readFileSync(path.join(skillDir, 'SKILL.md'), 'utf-8'); + const match = content.match(/^name:\s*(.+)$/m); + return match ? match[1].trim() : null; +} + +function gitInInstall(args: string[]): string { + const result = spawnSync('git', args, { cwd: installDir, encoding: 'utf8', timeout: 30_000 }); + if (result.status !== 0) throw new Error(`git ${args.join(' ')} failed: ${result.stderr}`); + return result.stdout.trim(); +} + +function commitInstall(message: string): string { + gitInInstall(['add', '.']); + gitInInstall(['commit', '-qm', message]); + return gitInInstall(['rev-parse', 'HEAD']); +} + beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-relink-test-')); }); @@ -151,9 +170,11 @@ describe('gstack-relink (#578)', () => { } }); - // Same invariant for prefixed mode - test('prefixed skills are real directories with SKILL.md symlinks, not dir symlinks', () => { + // Prefix mode needs a rewritten consumer copy. A symlink would either serve + // the canonical flat name or make an in-place rewrite dirty the source. + test('prefixed skills are real directories with rewritten SKILL.md copies', () => { setupMockInstall(['qa', 'ship']); + const qaSource = fs.readFileSync(path.join(installDir, 'qa', 'SKILL.md'), 'utf8'); run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix true`, { GSTACK_INSTALL_DIR: installDir, GSTACK_SKILLS_DIR: skillsDir, @@ -167,8 +188,155 @@ describe('gstack-relink (#578)', () => { const skillMdPath = path.join(skillPath, 'SKILL.md'); expect(fs.lstatSync(skillPath).isDirectory()).toBe(true); expect(fs.lstatSync(skillPath).isSymbolicLink()).toBe(false); - expect(fs.lstatSync(skillMdPath).isSymbolicLink()).toBe(true); + expect(fs.lstatSync(skillMdPath).isSymbolicLink()).toBe(false); + expect(readSkillName(skillPath)).toBe(skill); + expect(fs.readFileSync(skillMdPath, 'utf8')).toContain( + ``, + ); } + expect(fs.readFileSync(path.join(installDir, 'qa', 'SKILL.md'), 'utf8')).toBe(qaSource); + expect(readSkillName(path.join(installDir, 'qa'))).toBe('qa'); + }); + + test('a failed rewrite leaves the previously served SKILL.md intact', () => { + setupMockInstall(['qa']); + fs.writeFileSync(path.join(tmpDir, 'config.yaml'), 'skill_prefix: true\n'); + run(path.join(installDir, 'bin', 'gstack-relink'), { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }); + + const targetDir = path.join(skillsDir, 'gstack-qa'); + const targetSkill = path.join(targetDir, 'SKILL.md'); + const sourceSkill = path.join(installDir, 'qa', 'SKILL.md'); + const servedBefore = fs.readFileSync(targetSkill); + const shimDir = path.join(tmpDir, 'shim'); + const realSed = execSync('command -v sed', { + encoding: 'utf8', + shell: '/bin/bash', + timeout: 30_000, + }).trim(); + fs.mkdirSync(shimDir); + fs.writeFileSync( + path.join(shimDir, 'sed'), + `#!/usr/bin/env bash\nlast=""\nfor arg in "$@"; do last="$arg"; done\nif [ "$last" = "$GSTACK_FAIL_SOURCE" ]; then printf 'partial output\\n'; exit 19; fi\nexec "${realSed}" "$@"\n`, + { mode: 0o755 }, + ); + + const result = spawnSync(path.join(installDir, 'bin', 'gstack-relink'), [], { + cwd: ROOT, + encoding: 'utf8', + timeout: 10_000, + env: { + ...process.env, + PATH: `${shimDir}:${process.env.PATH}`, + GSTACK_FAIL_SOURCE: sourceSkill, + GSTACK_STATE_DIR: tmpDir, + GSTACK_HOME: tmpDir, + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }, + }); + + expect(result.status).not.toBe(0); + expect(fs.readFileSync(targetSkill)).toEqual(servedBefore); + expect(fs.readdirSync(targetDir).filter((name) => name.startsWith('.gstack-skill.'))).toEqual([]); + const source = fs.readFileSync(path.join(installDir, 'bin', 'gstack-relink'), 'utf8'); + const installBody = source.slice( + source.indexOf('_install_served_skill_md()'), + source.indexOf('_root_alias_owned()'), + ); + expect(installBody).not.toContain('rm -f "$target_dir/SKILL.md"'); + expect(installBody.indexOf('mktemp "$target_dir/.gstack-skill.XXXXXX"')).toBeLessThan( + installBody.indexOf('mv -f "$tmp_file" "$target_dir/SKILL.md"'), + ); + }); + + test('first rerun adopts only a byte-exact legacy prefixed copy and adds provenance', () => { + setupMockInstall(['qa']); + fs.writeFileSync(path.join(tmpDir, 'config.yaml'), 'skill_prefix: true\n'); + const target = path.join(skillsDir, 'gstack-qa'); + fs.mkdirSync(target); + const source = fs.readFileSync(path.join(installDir, 'qa', 'SKILL.md'), 'utf8'); + fs.writeFileSync(path.join(target, 'SKILL.md'), source.replace(/^name:.*$/m, 'name: gstack-qa')); + + run(path.join(installDir, 'bin', 'gstack-relink'), { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }); + + const installed = fs.readFileSync(path.join(target, 'SKILL.md'), 'utf8'); + expect(installed).toContain( + '', + ); + expect(installed.match(/AUTO-GENERATED from gstack consumer/g)?.length).toBe(1); + }); + + test('first upgrade adopts the exact OLD_HEAD prefixed copy after source bytes changed', () => { + setupMockInstall(['qa']); + gitInInstall(['init', '-q']); + gitInInstall(['config', 'user.name', 'GStack Test']); + gitInInstall(['config', 'user.email', 'gstack-test@example.invalid']); + const oldSource = fs.readFileSync(path.join(installDir, 'qa', 'SKILL.md'), 'utf8'); + const oldHead = commitInstall('old generated source'); + fs.writeFileSync(path.join(tmpDir, 'config.yaml'), 'skill_prefix: true\n'); + const target = path.join(skillsDir, 'gstack-qa'); + fs.mkdirSync(target); + fs.writeFileSync( + path.join(target, 'SKILL.md'), + oldSource.replace(/^name:.*$/m, 'name: gstack-qa'), + ); + fs.writeFileSync( + path.join(installDir, 'qa', 'SKILL.md'), + '---\nname: qa\ndescription: test\n---\n# qa changed after pull', + ); + commitInstall('new generated source'); + + run(path.join(installDir, 'bin', 'gstack-relink'), { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + GSTACK_UPGRADE_FROM_HEAD: oldHead, + }); + + const installed = fs.readFileSync(path.join(target, 'SKILL.md'), 'utf8'); + expect(installed).toContain('# qa changed after pull'); + expect(installed).toContain( + '', + ); + }); + + test('first upgrade adopts the exact OLD_HEAD root alias after source bytes changed', () => { + setupMockInstall(['qa']); + const oldRoot = '---\nname: gstack\ndescription: root\n---\n# old root'; + fs.writeFileSync(path.join(installDir, 'SKILL.md'), oldRoot); + gitInInstall(['init', '-q']); + gitInInstall(['config', 'user.name', 'GStack Test']); + gitInInstall(['config', 'user.email', 'gstack-test@example.invalid']); + const oldHead = commitInstall('old root source'); + const alias = path.join(skillsDir, '_gstack-command'); + fs.mkdirSync(alias); + fs.writeFileSync( + path.join(alias, 'SKILL.md'), + oldRoot.replace(/^name:.*$/m, 'name: _gstack-command'), + ); + fs.writeFileSync( + path.join(installDir, 'SKILL.md'), + '---\nname: gstack\ndescription: root\n---\n# new root', + ); + commitInstall('new root source'); + fs.writeFileSync(path.join(tmpDir, 'config.yaml'), 'skill_prefix: false\n'); + + run(path.join(installDir, 'bin', 'gstack-relink'), { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + GSTACK_UPGRADE_FROM_HEAD: oldHead, + }); + + const installed = fs.readFileSync(path.join(alias, 'SKILL.md'), 'utf8'); + expect(installed).toContain('# new root'); + expect(installed).toContain( + '', + ); }); // Upgrade: old directory symlinks get replaced with real directories @@ -221,6 +389,9 @@ describe('gstack-relink (#578)', () => { expect(fs.lstatSync(aliasSkill).isSymbolicLink()).toBe(false); const aliasContent = fs.readFileSync(aliasSkill, 'utf-8'); expect(aliasContent).toContain('name: _gstack-command'); + expect(aliasContent).toContain( + '', + ); expect(aliasContent).not.toContain('name: gstack\n'); // The rewrite happened on the COPY: the canonical source keeps its name. expect(fs.readFileSync(path.join(installDir, 'SKILL.md'), 'utf-8')).toContain('name: gstack'); @@ -323,9 +494,11 @@ describe('gstack-relink (#578)', () => { }); const served = path.join(skillsDir, 'gstack-qa', 'SKILL.md'); - expect(fs.readlinkSync(served)).toBe(path.join(renderDir, 'SKILL.md')); - // The SERVED file (the render) carries the prefixed name. + expect(fs.lstatSync(served).isSymbolicLink()).toBe(false); + // The SERVED consumer copy carries the prefixed name. expect(fs.readFileSync(served, 'utf-8')).toContain('name: gstack-qa'); + // The reusable render remains canonical and can serve flat installs too. + expect(fs.readFileSync(path.join(renderDir, 'SKILL.md'), 'utf-8')).toContain('name: qa'); // Idempotent: a second relink must not double-prefix. run(`${path.join(installDir, 'bin', 'gstack-relink')}`, { GSTACK_INSTALL_DIR: installDir, @@ -406,6 +579,9 @@ describe('gstack-relink (#578)', () => { }); let entries = fs.readdirSync(skillsDir); expect(entries.filter(e => !e.startsWith('gstack-'))).toEqual([]); + expect(fs.readFileSync(path.join(skillsDir, 'gstack-qa', 'SKILL.md'), 'utf8')).toContain( + '', + ); // Switch to no-prefix run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`, { @@ -423,6 +599,110 @@ describe('gstack-relink (#578)', () => { expect(leaked).toEqual([]); }); + test('a colliding user-owned gstack-* directory is never overwritten or deleted', () => { + setupMockInstall(['qa']); + fs.writeFileSync(path.join(tmpDir, 'config.yaml'), 'skill_prefix: true\n'); + const collision = path.join(skillsDir, 'gstack-qa'); + fs.mkdirSync(collision, { recursive: true }); + fs.writeFileSync( + path.join(collision, 'SKILL.md'), + '---\nname: gstack-qa\ndescription: my private workflow\n---\n# keep me\n', + ); + fs.writeFileSync(path.join(collision, 'private.txt'), 'irreplaceable\n'); + + const result = spawnSync(path.join(installDir, 'bin', 'gstack-relink'), [], { + cwd: ROOT, + encoding: 'utf8', + timeout: 10_000, + env: { + ...process.env, + GSTACK_STATE_DIR: tmpDir, + GSTACK_HOME: tmpDir, + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }, + }); + + expect(result.status).toBe(2); + expect(result.stderr).toContain('refusing to replace non-gstack skill entry'); + expect(fs.readFileSync(path.join(collision, 'SKILL.md'), 'utf8')).toContain('# keep me'); + expect(fs.readFileSync(path.join(collision, 'private.txt'), 'utf8')).toBe('irreplaceable\n'); + }); + + test('prefix-to-flat cleanup preserves an unmarked gstack-* user directory', () => { + setupMockInstall(['qa']); + fs.writeFileSync(path.join(tmpDir, 'config.yaml'), 'skill_prefix: false\n'); + const collision = path.join(skillsDir, 'gstack-qa'); + fs.mkdirSync(collision, { recursive: true }); + fs.writeFileSync(path.join(collision, 'SKILL.md'), '---\nname: gstack-qa\n---\n# user owned\n'); + + const result = spawnSync(path.join(installDir, 'bin', 'gstack-relink'), [], { + cwd: ROOT, + encoding: 'utf8', + timeout: 10_000, + env: { + ...process.env, + GSTACK_STATE_DIR: tmpDir, + GSTACK_HOME: tmpDir, + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }, + }); + + expect(result.status).toBe(2); + expect(fs.readFileSync(path.join(collision, 'SKILL.md'), 'utf8')).toContain('# user owned'); + expect(fs.existsSync(path.join(skillsDir, 'qa'))).toBe(false); + }); + + for (const scenario of [ + { label: 'prefix to flat', prefix: false, oldName: 'gstack-qa', targetName: 'qa' }, + { label: 'flat to prefix', prefix: true, oldName: 'qa', targetName: 'gstack-qa' }, + ]) { + test(`${scenario.label} collision is rejected before either entry is mutated`, () => { + setupMockInstall(['qa']); + const initialPrefix = !scenario.prefix; + fs.writeFileSync(path.join(tmpDir, 'config.yaml'), `skill_prefix: ${initialPrefix}\n`); + run(path.join(installDir, 'bin', 'gstack-relink'), { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }); + + const oldDir = path.join(skillsDir, scenario.oldName); + const oldSkill = path.join(oldDir, 'SKILL.md'); + const oldBytes = fs.readFileSync(oldSkill); + const oldWasLink = fs.lstatSync(oldSkill).isSymbolicLink(); + const oldLink = oldWasLink ? fs.readlinkSync(oldSkill) : null; + + const userDir = path.join(skillsDir, scenario.targetName); + fs.mkdirSync(userDir); + const userBytes = `---\nname: ${scenario.targetName}\n---\n# private destination\n`; + fs.writeFileSync(path.join(userDir, 'SKILL.md'), userBytes); + fs.writeFileSync(path.join(userDir, 'keep.txt'), 'irreplaceable\n'); + fs.writeFileSync(path.join(tmpDir, 'config.yaml'), `skill_prefix: ${scenario.prefix}\n`); + + const result = spawnSync(path.join(installDir, 'bin', 'gstack-relink'), [], { + cwd: ROOT, + encoding: 'utf8', + timeout: 10_000, + env: { + ...process.env, + GSTACK_STATE_DIR: tmpDir, + GSTACK_HOME: tmpDir, + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }, + }); + + expect(result.status).toBe(2); + expect(result.stderr).toContain('refusing to replace non-gstack skill entry'); + expect(fs.readFileSync(oldSkill)).toEqual(oldBytes); + expect(fs.lstatSync(oldSkill).isSymbolicLink()).toBe(oldWasLink); + if (oldLink !== null) expect(fs.readlinkSync(oldSkill)).toBe(oldLink); + expect(fs.readFileSync(path.join(userDir, 'SKILL.md'), 'utf8')).toBe(userBytes); + expect(fs.readFileSync(path.join(userDir, 'keep.txt'), 'utf8')).toBe('irreplaceable\n'); + }); + } + // SWITCH: no-prefix → prefix must clean up ALL flat entries test('switching no-prefix to prefix removes all flat entries completely', () => { setupMockInstall(['qa', 'ship', 'review', 'gstack-upgrade']); @@ -588,16 +868,10 @@ describe('upgrade migrations', () => { }); }); -describe('gstack-patch-names (#620/#578)', () => { - // Helper to read name: from SKILL.md frontmatter - function readSkillName(skillDir: string): string | null { - const content = fs.readFileSync(path.join(skillDir, 'SKILL.md'), 'utf-8'); - const match = content.match(/^name:\s*(.+)$/m); - return match ? match[1].trim() : null; - } - - test('prefix=true patches name: field in SKILL.md', () => { +describe('consumer name patching (#620/#578)', () => { + test('prefix=true patches installed copies without modifying source SKILL.md', () => { setupMockInstall(['qa', 'ship', 'review']); + const qaSource = fs.readFileSync(path.join(installDir, 'qa', 'SKILL.md'), 'utf8'); run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix true`, { GSTACK_INSTALL_DIR: installDir, GSTACK_SKILLS_DIR: skillsDir, @@ -606,10 +880,11 @@ describe('gstack-patch-names (#620/#578)', () => { GSTACK_INSTALL_DIR: installDir, GSTACK_SKILLS_DIR: skillsDir, }); - // Verify name: field is patched with gstack- prefix - expect(readSkillName(path.join(installDir, 'qa'))).toBe('gstack-qa'); - expect(readSkillName(path.join(installDir, 'ship'))).toBe('gstack-ship'); - expect(readSkillName(path.join(installDir, 'review'))).toBe('gstack-review'); + expect(readSkillName(path.join(skillsDir, 'gstack-qa'))).toBe('gstack-qa'); + expect(readSkillName(path.join(skillsDir, 'gstack-ship'))).toBe('gstack-ship'); + expect(readSkillName(path.join(skillsDir, 'gstack-review'))).toBe('gstack-review'); + expect(fs.readFileSync(path.join(installDir, 'qa', 'SKILL.md'), 'utf8')).toBe(qaSource); + expect(readSkillName(path.join(installDir, 'qa'))).toBe('qa'); }); test('prefix=false restores name: field in SKILL.md', () => { @@ -623,7 +898,7 @@ describe('gstack-patch-names (#620/#578)', () => { GSTACK_INSTALL_DIR: installDir, GSTACK_SKILLS_DIR: skillsDir, }); - expect(readSkillName(path.join(installDir, 'qa'))).toBe('gstack-qa'); + expect(readSkillName(path.join(skillsDir, 'gstack-qa'))).toBe('gstack-qa'); // Now switch to flat mode run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`, { GSTACK_INSTALL_DIR: installDir, @@ -633,9 +908,8 @@ describe('gstack-patch-names (#620/#578)', () => { GSTACK_INSTALL_DIR: installDir, GSTACK_SKILLS_DIR: skillsDir, }); - // Verify name: field is restored to unprefixed - expect(readSkillName(path.join(installDir, 'qa'))).toBe('qa'); - expect(readSkillName(path.join(installDir, 'ship'))).toBe('ship'); + expect(readSkillName(path.join(skillsDir, 'qa'))).toBe('qa'); + expect(readSkillName(path.join(skillsDir, 'ship'))).toBe('ship'); }); test('gstack-upgrade name: not double-prefixed', () => { @@ -648,10 +922,9 @@ describe('gstack-patch-names (#620/#578)', () => { GSTACK_INSTALL_DIR: installDir, GSTACK_SKILLS_DIR: skillsDir, }); - // gstack-upgrade should keep its name, NOT become gstack-gstack-upgrade - expect(readSkillName(path.join(installDir, 'gstack-upgrade'))).toBe('gstack-upgrade'); - // Regular skill should be prefixed - expect(readSkillName(path.join(installDir, 'qa'))).toBe('gstack-qa'); + expect(readSkillName(path.join(skillsDir, 'gstack-upgrade'))).toBe('gstack-upgrade'); + expect(readSkillName(path.join(skillsDir, 'gstack-qa'))).toBe('gstack-qa'); + expect(readSkillName(path.join(installDir, 'qa'))).toBe('qa'); }); test('SKILL.md without frontmatter is a no-op', () => { @@ -671,4 +944,14 @@ describe('gstack-patch-names (#620/#578)', () => { const content = fs.readFileSync(path.join(installDir, 'qa', 'SKILL.md'), 'utf-8'); expect(content).toBe('# qa\nSome content.'); }); + + test('legacy helper refuses to rewrite a tracked canonical checkout', () => { + const result = spawnSync(path.join(BIN, 'gstack-patch-names'), [ROOT, 'true'], { + cwd: ROOT, + encoding: 'utf8', + timeout: 10_000, + }); + expect(result.status).toBe(2); + expect(result.stderr).toContain('refusing to patch tracked canonical SKILL.md files'); + }); }); diff --git a/test/session-update-autostash.test.ts b/test/session-update-autostash.test.ts index b61b659a29..be40667c3f 100644 --- a/test/session-update-autostash.test.ts +++ b/test/session-update-autostash.test.ts @@ -4,12 +4,9 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; -// #2566: on a normal install, tracked files are locally patched (skill-prefix -// name rewrites, gbrain-refresh blocks), so a bare `git pull --ff-only` -// refused FOREVER — 308 consecutive PULL_FAILED entries observed, with the -// reason discarded by 2>/dev/null. The fix: --autostash un-wedges the pull -// over local edits, and stderr is captured into the log so a real failure -// names its cause. +// A session-start background updater never owns local operator edits. Dirty +// checkouts fail closed before pull, so autostash conflicts cannot discard the +// worktree or its recoverable stash/ref. const ROOT = path.resolve(import.meta.dir, '..'); const SCRIPT = path.join(ROOT, 'bin', 'gstack-session-update'); @@ -25,15 +22,20 @@ function makeFixture() { const install = path.join(base, 'install'); const state = path.join(base, 'state'); fs.mkdirSync(state, { recursive: true }); - execFileSync('git', ['init', '-q', '--bare', '-b', 'main', origin]); + execFileSync('git', ['init', '-q', '--bare', '-b', 'main', origin], { timeout: 30_000 }); fs.mkdirSync(path.join(seed, 'bin'), { recursive: true }); fs.writeFileSync(path.join(seed, 'VERSION'), '1.0.0\n'); fs.writeFileSync(path.join(seed, 'SKILL.md'), '# top\nname: qa\nbody line\n'); - // Stub config: auto_upgrade on, prefix off; gbrain-refresh no-op. + // Stub config: auto_upgrade on, prefix off; record any pre-setup render refresh. fs.writeFileSync( path.join(seed, 'bin', 'gstack-config'), - '#!/usr/bin/env bash\nif [ "$1" = "get" ]; then case "$2" in auto_upgrade) echo true;; skill_prefix) echo false;; *) echo "";; esac; fi\nexit 0\n', + '#!/usr/bin/env bash\nif [ "$1" = "get" ]; then case "$2" in auto_upgrade) echo true;; skill_prefix) echo false;; *) echo "";; esac; fi\nif [ "$1" = "gbrain-refresh" ]; then echo called >> "$GSTACK_STATE_DIR/gbrain-refresh.calls"; fi\nexit 0\n', + { mode: 0o755 }, + ); + fs.writeFileSync( + path.join(seed, 'setup'), + '#!/usr/bin/env bash\nprintf "%s\\n" "${GSTACK_UPGRADE_FROM_HEAD:-missing}" > "$GSTACK_STATE_DIR/setup-old-head"\nprintf "attempt\\n" >> "$GSTACK_STATE_DIR/setup-attempts"\n[ ! -f "$GSTACK_STATE_DIR/force-setup-failure" ]\n', { mode: 0o755 }, ); fs.writeFileSync(path.join(seed, 'bin', 'gstack-patch-names'), '#!/usr/bin/env bash\nexit 0\n', { @@ -45,7 +47,7 @@ function makeFixture() { git(seed, 'branch', '-M', 'main'); git(seed, 'remote', 'add', 'origin', origin); git(seed, 'push', '-q', 'origin', 'main'); - execFileSync('git', ['clone', '-q', origin, install]); + execFileSync('git', ['clone', '-q', origin, install], { timeout: 30_000 }); return { base, origin, seed, install, state }; } @@ -69,7 +71,12 @@ async function waitForLog(state: string, pattern: RegExp, ms = 15000): Promise { - test('locally-patched tracked files no longer wedge the ff-only pull', async () => { + test('session update never reapplies prefix names to the tracked checkout', () => { + const source = fs.readFileSync(SCRIPT, 'utf8'); + expect(source).not.toMatch(/gstack-patch-names[^\n]*\$GSTACK_DIR/); + }); + + test('a dirty checkout is preserved exactly and skipped before pull', async () => { const { base, seed, install, state } = makeFixture(); try { // Upstream advances (edit at the TOP of SKILL.md)… @@ -79,21 +86,158 @@ describe('gstack-session-update pull wedge (#2566)', () => { ); git(seed, 'commit', '-aqm', 'upstream change'); git(seed, 'push', '-q', 'origin', 'main'); - const upstreamHead = git(seed, 'rev-parse', 'HEAD'); - - // …while the install carries a local patch at the BOTTOM (the - // prefix-rename / gbrain-block shape: tracked file, modified). + // …while the install carries a local operator edit. fs.appendFileSync(path.join(install, 'SKILL.md'), 'locally patched line\n'); + const headBefore = git(install, 'rev-parse', 'HEAD'); + const statusBefore = git(install, 'status', '--porcelain=v1'); + const bytesBefore = fs.readFileSync(path.join(install, 'SKILL.md')); const r = runScript(install, state); expect(r.status).toBe(0); - const log = await waitForLog(state, /UPDATING|UP_TO_DATE|PULL_FAILED/); + const log = await waitForLog(state, /SKIP dirty_worktree/); expect(log).not.toContain('PULL_FAILED'); - expect(log).toContain('UPDATING'); - expect(git(install, 'rev-parse', 'HEAD')).toBe(upstreamHead); - // The autostash pop preserved the local patch over the new tree. - expect(fs.readFileSync(path.join(install, 'SKILL.md'), 'utf8')).toContain( - 'locally patched line', + expect(log).toContain(`SKIP dirty_worktree head=${headBefore}`); + expect(git(install, 'rev-parse', 'HEAD')).toBe(headBefore); + expect(git(install, 'status', '--porcelain=v1')).toBe(statusBefore); + expect(fs.readFileSync(path.join(install, 'SKILL.md'))).toEqual(bytesBefore); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }, 30000); + + test('a would-be autostash conflict preserves worktree, index, HEAD, and exact stash refs', async () => { + const { base, seed, install, state } = makeFixture(); + try { + // Keep a pre-existing operator stash so the test proves the updater does + // not drop, reorder, or replace any stash ref. + fs.appendFileSync(path.join(install, 'VERSION'), 'stashed operator work\n'); + git(install, 'stash', 'push', '-q', '-m', 'operator-keeper'); + + // Remote and local now edit the same line: `pull --autostash` would + // fast-forward and conflict while reapplying the local patch. + fs.writeFileSync(path.join(seed, 'SKILL.md'), '# top\nname: qa\nremote body\n'); + git(seed, 'commit', '-aqm', 'remote conflicting change'); + git(seed, 'push', '-q', 'origin', 'main'); + fs.writeFileSync(path.join(install, 'SKILL.md'), '# top\nname: qa\nlocal body\n'); + git(install, 'add', 'SKILL.md'); + + const headBefore = git(install, 'rev-parse', 'HEAD'); + const statusBefore = git(install, 'status', '--porcelain=v1'); + const stagedBefore = git(install, 'diff', '--cached', '--binary'); + const skillBefore = fs.readFileSync(path.join(install, 'SKILL.md')); + const stashesBefore = git(install, 'stash', 'list', '--format=%H %gd %gs'); + + const r = runScript(install, state); + expect(r.status).toBe(0); + const log = await waitForLog(state, /SKIP dirty_worktree/); + expect(log).toContain(`SKIP dirty_worktree head=${headBefore}`); + expect(git(install, 'rev-parse', 'HEAD')).toBe(headBefore); + expect(git(install, 'status', '--porcelain=v1')).toBe(statusBefore); + expect(git(install, 'diff', '--cached', '--binary')).toBe(stagedBefore); + expect(fs.readFileSync(path.join(install, 'SKILL.md'))).toEqual(skillBefore); + expect(git(install, 'stash', 'list', '--format=%H %gd %gs')).toBe(stashesBefore); + expect(fs.existsSync(path.join(install, '.git', 'MERGE_MSG'))).toBe(false); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }, 30000); + + test('session updater contains no destructive autostash recovery path', () => { + const source = fs.readFileSync(SCRIPT, 'utf8'); + expect(source).not.toContain('--autostash'); + expect(source).not.toMatch(/git[^\n]*checkout[^\n]*-- \,?\./); + expect(source).not.toMatch(/stash drop/); + }); + + test('a real HEAD transition gives setup the exact pre-pull revision before any render refresh', async () => { + const { base, seed, install, state } = makeFixture(); + try { + const oldHead = git(install, 'rev-parse', 'HEAD'); + fs.writeFileSync(path.join(seed, 'VERSION'), '2.0.0\n'); + git(seed, 'commit', '-aqm', 'upstream upgrade'); + git(seed, 'push', '-q', 'origin', 'main'); + + const r = runScript(install, state); + expect(r.status).toBe(0); + const log = await waitForLog(state, /UPDATED from=/); + expect(log).toContain('UPDATED from=1.0.0'); + expect(fs.readFileSync(path.join(state, 'setup-old-head'), 'utf8').trim()).toBe(oldHead); + expect(fs.existsSync(path.join(state, 'gbrain-refresh.calls'))).toBe(false); + expect(fs.existsSync(path.join(state, '.pending-session-setup'))).toBe(false); + + const source = fs.readFileSync(SCRIPT, 'utf8'); + expect(source).toContain('GSTACK_UPGRADE_FROM_HEAD="$PENDING_SETUP_FROM" ./setup -q'); + expect(source.indexOf('GSTACK_UPGRADE_FROM_HEAD="$PENDING_SETUP_FROM" ./setup -q')).toBeLessThan( + source.indexOf('"$GSTACK_DIR/bin/gstack-config" gbrain-refresh'), + ); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }, 30000); + + test('a failed setup remains pending and retries from the exact pre-pull revision', async () => { + const { base, seed, install, state } = makeFixture(); + try { + const oldHead = git(install, 'rev-parse', 'HEAD'); + fs.writeFileSync(path.join(state, 'force-setup-failure'), 'fail\n'); + fs.writeFileSync(path.join(seed, 'VERSION'), '2.0.0\n'); + git(seed, 'commit', '-aqm', 'upstream upgrade requiring setup'); + git(seed, 'push', '-q', 'origin', 'main'); + + const first = runScript(install, state); + expect(first.status).toBe(0); + const failedLog = await waitForLog(state, /SETUP_FAILED/); + expect(failedLog).toContain(`from=${oldHead}`); + expect(failedLog).not.toContain('UPDATED from='); + expect(git(install, 'rev-parse', 'HEAD')).not.toBe(oldHead); + expect(fs.readFileSync(path.join(state, '.pending-session-setup'), 'utf8')).toBe( + `from=${oldHead}\n`, + ); + expect(fs.existsSync(path.join(state, 'just-upgraded-from'))).toBe(false); + expect(fs.existsSync(path.join(state, 'gbrain-refresh.calls'))).toBe(false); + + // The first run wrote the normal one-hour throttle. A pending migration + // must bypass it and converge immediately once the failure is repaired. + fs.rmSync(path.join(state, 'force-setup-failure')); + const lockDeadline = Date.now() + 5_000; + while (fs.existsSync(path.join(state, '.setup-lock')) && Date.now() < lockDeadline) { + await new Promise((resolve) => setTimeout(resolve, 50)); + } + const second = runScript(install, state); + expect(second.status).toBe(0); + const recoveredLog = await waitForLog(state, /SETUP_RECOVERED/); + expect(recoveredLog).toContain('SETUP_RECOVERED from=1.0.0 to=2.0.0'); + expect(fs.readFileSync(path.join(state, 'setup-old-head'), 'utf8').trim()).toBe(oldHead); + expect(fs.readFileSync(path.join(state, 'setup-attempts'), 'utf8')).toBe( + 'attempt\nattempt\n', + ); + expect(fs.existsSync(path.join(state, '.pending-session-setup'))).toBe(false); + expect(fs.readFileSync(path.join(state, 'just-upgraded-from'), 'utf8')).toBe('1.0.0\n'); + expect(fs.existsSync(path.join(state, 'gbrain-refresh.calls'))).toBe(false); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }, 30000); + + test('a pending base from unrelated history fails closed without pulling or running setup', async () => { + const { base, install, state } = makeFixture(); + try { + const mainHead = git(install, 'rev-parse', 'HEAD'); + git(install, 'checkout', '-qb', 'unrelated-pending-base'); + fs.writeFileSync(path.join(install, 'VERSION'), 'unrelated\n'); + git(install, 'commit', '-aqm', 'unrelated pending base'); + const unrelatedHead = git(install, 'rev-parse', 'HEAD'); + git(install, 'checkout', '-q', 'main'); + fs.writeFileSync(path.join(state, '.pending-session-setup'), `from=${unrelatedHead}\n`); + + const r = runScript(install, state); + expect(r.status).toBe(0); + const log = await waitForLog(state, /SETUP_PENDING_INVALID/); + expect(log).toContain('SETUP_PENDING_INVALID'); + expect(git(install, 'rev-parse', 'HEAD')).toBe(mainHead); + expect(fs.existsSync(path.join(state, 'setup-attempts'))).toBe(false); + expect(fs.readFileSync(path.join(state, '.pending-session-setup'), 'utf8')).toBe( + `from=${unrelatedHead}\n`, ); } finally { fs.rmSync(base, { recursive: true, force: true }); @@ -135,7 +279,10 @@ describe('gstack-session-update lock identity + TTL (#2613)', () => { function makeSlowGitShim(base: string, sleepSecs: number): string { const shimDir = path.join(base, 'shim'); fs.mkdirSync(shimDir, { recursive: true }); - const realGit = execFileSync('bash', ['-c', 'command -v git'], { encoding: 'utf8' }).trim(); + const realGit = execFileSync('bash', ['-c', 'command -v git'], { + encoding: 'utf8', + timeout: 30_000, + }).trim(); fs.writeFileSync( path.join(shimDir, 'git'), `#!/usr/bin/env bash\ncase "$*" in *pull*) sleep ${sleepSecs};; esac\nexec "${realGit}" "$@"\n`, @@ -184,6 +331,39 @@ describe('gstack-session-update lock identity + TTL (#2613)', () => { } }, 30000); + test('upgrade intent is atomically durable before a pull can move HEAD', async () => { + const { base, install, state } = makeFixture(); + const shimDir = makeSlowGitShim(base, 3); + try { + const oldHead = git(install, 'rev-parse', 'HEAD'); + const r = runScriptWithPath(install, state, shimDir); + expect(r.status).toBe(0); + + const pendingPath = path.join(state, '.pending-session-setup'); + const lockPath = path.join(state, '.setup-lock'); + const deadline = Date.now() + 2_000; + while (!fs.existsSync(pendingPath) && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 25)); + } + expect(fs.existsSync(lockPath)).toBe(true); + expect(fs.readFileSync(pendingPath, 'utf8')).toBe(`from=${oldHead}\n`); + + await waitForLog(state, /UP_TO_DATE/); + const clearDeadline = Date.now() + 5_000; + while (fs.existsSync(pendingPath) && Date.now() < clearDeadline) { + await new Promise((resolve) => setTimeout(resolve, 50)); + } + expect(fs.existsSync(pendingPath)).toBe(false); + + const source = fs.readFileSync(SCRIPT, 'utf8'); + expect(source.indexOf('mv -f "$PENDING_SETUP_TMP" "$PENDING_SETUP_FILE"')).toBeLessThan( + source.indexOf('_receipted_git open session-update'), + ); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }, 30000); + test('a live lock with a live pid is respected and survives', async () => { const { base, install, state } = makeFixture(); const holder = require('child_process').spawn('sleep', ['30'], { stdio: 'ignore' }); diff --git a/test/setup-alias-name-uniqueness.test.ts b/test/setup-alias-name-uniqueness.test.ts index c05d1d6d69..5327791121 100644 --- a/test/setup-alias-name-uniqueness.test.ts +++ b/test/setup-alias-name-uniqueness.test.ts @@ -31,6 +31,15 @@ function extractFn(name: string): string { return SETUP_SRC.slice(start, end + 2); } +const CLAUDE_OWNERSHIP_HELPERS = [ + extractFn('_gstack_upgrade_base_sha'), + extractFn('_skill_file_matches_source'), + extractFn('_legacy_claude_consumer_copy'), + extractFn('_claude_skill_entry_owned'), + extractFn('_legacy_claude_alias_copy'), + extractFn('_claude_alias_entry_owned'), +]; + const installDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-alias-install-')); const sourceRootSkill = fs.readFileSync(path.join(ROOT, 'SKILL.md'), 'utf-8'); @@ -53,6 +62,8 @@ beforeAll(() => { 'QUIET=1', '_WINDOWS_COPY_NOTE_PRINTED=1', extractFn('_link_or_copy'), + ...CLAUDE_OWNERSHIP_HELPERS, + extractFn('_install_managed_skill_md'), extractFn('_print_windows_copy_note_once'), extractFn('_link_skill_runtime_assets'), extractFn('link_claude_skill_dirs'), @@ -78,7 +89,324 @@ function frontmatterName(skillMdPath: string): string | null { return m ? m[1] : null; } +function git(repo: string, args: string[]): string { + const result = spawnSync('git', args, { cwd: repo, encoding: 'utf8' }); + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${result.stderr}`); + } + return result.stdout.trim(); +} + +function initHistoryRepo(repo: string): void { + git(repo, ['init', '-q']); + git(repo, ['config', 'user.name', 'GStack Test']); + git(repo, ['config', 'user.email', 'gstack-test@example.invalid']); +} + +function runSetupDestinationPreflight(prefix: 0 | 1, skillsDir: string) { + const script = [ + 'set -e', + `SKILL_PREFIX=${prefix}`, + `GSTACK_HOME="${skillsDir}/state"`, + ...CLAUDE_OWNERSHIP_HELPERS, + extractFn('preflight_claude_skill_dirs'), + extractFn('preflight_claude_install'), + `preflight_claude_install "${ROOT}" "${skillsDir}"`, + ].join('\n'); + return spawnSync('bash', ['-c', script], { encoding: 'utf8', timeout: 30_000 }); +} + describe('alias installs are rewritten copies (#2511, #2201)', () => { + test('repeated prefixed setup writes only installed copies and leaves every canonical skill byte-clean', () => { + const prefixedInstall = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-prefixed-install-')); + const sourceSkills = fs.readdirSync(ROOT) + .map((entry) => path.join(ROOT, entry, 'SKILL.md')) + .filter((skill) => fs.existsSync(skill)); + const before = new Map(sourceSkills.map((skill) => [skill, fs.readFileSync(skill)])); + try { + const script = [ + 'set -e', + 'IS_WINDOWS=0', + 'SKILL_PREFIX=1', + 'QUIET=1', + '_WINDOWS_COPY_NOTE_PRINTED=1', + `GSTACK_HOME="${prefixedInstall}/state"`, + extractFn('_link_or_copy'), + ...CLAUDE_OWNERSHIP_HELPERS, + extractFn('_install_managed_skill_md'), + extractFn('_print_windows_copy_note_once'), + extractFn('_link_skill_runtime_assets'), + extractFn('_install_alias_skill_md'), + extractFn('link_claude_skill_dirs'), + `link_claude_skill_dirs "${ROOT}" "${prefixedInstall}"`, + // A normal re-run must refresh consumer copies idempotently without + // ever rewriting canonical generated sources. + `link_claude_skill_dirs "${ROOT}" "${prefixedInstall}"`, + ].join('\n'); + const result = spawnSync('bash', ['-c', script], { encoding: 'utf8', timeout: 60_000 }); + expect(result.status).toBe(0); + + for (const [skill, bytes] of before) expect(fs.readFileSync(skill)).toEqual(bytes); + for (const entry of fs.readdirSync(prefixedInstall).filter((name) => name.startsWith('gstack-'))) { + const installedSkill = path.join(prefixedInstall, entry, 'SKILL.md'); + if (!fs.existsSync(installedSkill)) continue; + const sourceName = fs.existsSync(path.join(ROOT, entry, 'SKILL.md')) + ? entry + : entry.replace(/^gstack-/, ''); + expect(fs.lstatSync(installedSkill).isSymbolicLink()).toBe(false); + expect(frontmatterName(installedSkill)).toBe(entry); + expect(fs.readFileSync(installedSkill, 'utf8')).toContain( + ``, + ); + } + + const cleanup = spawnSync('bash', ['-c', [ + 'set -e', + 'IS_WINDOWS=0', + extractFn('cleanup_prefixed_claude_symlinks'), + `cleanup_prefixed_claude_symlinks "${ROOT}" "${prefixedInstall}"`, + ].join('\n')], { encoding: 'utf8', timeout: 60_000 }); + expect(cleanup.status).toBe(0); + expect(fs.existsSync(path.join(prefixedInstall, 'gstack-qa'))).toBe(false); + } finally { + fs.rmSync(prefixedInstall, { recursive: true, force: true }); + } + }); + + test('first rerun migrates an exact legacy prefixed copy to explicit ownership', () => { + const target = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-legacy-prefix-')); + const legacy = path.join(target, 'gstack-qa'); + fs.mkdirSync(legacy); + const source = fs.readFileSync(path.join(ROOT, 'qa', 'SKILL.md'), 'utf8'); + fs.writeFileSync(path.join(legacy, 'SKILL.md'), source.replace(/^name:.*$/m, 'name: gstack-qa')); + try { + const result = spawnSync('bash', ['-c', [ + 'set -e', + 'IS_WINDOWS=0', + 'SKILL_PREFIX=1', + 'QUIET=1', + '_WINDOWS_COPY_NOTE_PRINTED=1', + `GSTACK_HOME="${target}/state"`, + extractFn('_link_or_copy'), + ...CLAUDE_OWNERSHIP_HELPERS, + extractFn('_install_managed_skill_md'), + extractFn('_print_windows_copy_note_once'), + extractFn('_link_skill_runtime_assets'), + extractFn('link_claude_skill_dirs'), + `link_claude_skill_dirs "${ROOT}" "${target}"`, + ].join('\n')], { encoding: 'utf8', timeout: 60_000 }); + + expect(result.status).toBe(0); + expect(fs.readFileSync(path.join(legacy, 'SKILL.md'), 'utf8')).toContain( + '', + ); + } finally { + fs.rmSync(target, { recursive: true, force: true }); + } + }); + + test('first Windows rerun migrates an exact legacy flat copy to explicit ownership', () => { + const target = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-legacy-windows-flat-')); + const legacy = path.join(target, 'qa'); + fs.mkdirSync(legacy); + fs.copyFileSync(path.join(ROOT, 'qa', 'SKILL.md'), path.join(legacy, 'SKILL.md')); + try { + const result = spawnSync('bash', ['-c', [ + 'set -e', + 'IS_WINDOWS=1', + 'SKILL_PREFIX=0', + 'QUIET=1', + '_WINDOWS_COPY_NOTE_PRINTED=1', + `GSTACK_HOME="${target}/state"`, + extractFn('_link_or_copy'), + ...CLAUDE_OWNERSHIP_HELPERS, + extractFn('_install_managed_skill_md'), + extractFn('_print_windows_copy_note_once'), + extractFn('_link_skill_runtime_assets'), + extractFn('link_claude_skill_dirs'), + `link_claude_skill_dirs "${ROOT}" "${target}"`, + ].join('\n')], { encoding: 'utf8', timeout: 60_000 }); + + expect(result.status).toBe(0); + expect(fs.readFileSync(path.join(legacy, 'SKILL.md'), 'utf8')).toContain( + '', + ); + } finally { + fs.rmSync(target, { recursive: true, force: true }); + } + }); + + test('first upgrade adopts a byte-exact prefixed copy from OLD_HEAD after the source changed', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-legacy-old-head-')); + try { + const source = path.join(tmp, 'source'); + const target = path.join(tmp, 'skills'); + const qa = path.join(source, 'qa'); + fs.mkdirSync(qa, { recursive: true }); + fs.mkdirSync(target); + initHistoryRepo(source); + const oldSource = '---\nname: qa\n---\n# old generated body\n'; + const newSource = '---\nname: qa\n---\n# new generated body\n'; + fs.writeFileSync(path.join(qa, 'SKILL.md'), oldSource); + git(source, ['add', 'qa/SKILL.md']); + git(source, ['commit', '-qm', 'old source']); + const oldHead = git(source, ['rev-parse', 'HEAD']); + + const installed = path.join(target, 'gstack-qa'); + fs.mkdirSync(installed); + fs.writeFileSync( + path.join(installed, 'SKILL.md'), + oldSource.replace(/^name:.*$/m, 'name: gstack-qa'), + ); + fs.writeFileSync(path.join(qa, 'SKILL.md'), newSource); + git(source, ['add', 'qa/SKILL.md']); + git(source, ['commit', '-qm', 'new source']); + + const result = spawnSync('bash', ['-c', [ + 'set -e', + 'IS_WINDOWS=0', + 'SKILL_PREFIX=1', + 'QUIET=1', + '_WINDOWS_COPY_NOTE_PRINTED=1', + `GSTACK_UPGRADE_FROM_HEAD="${oldHead}"`, + `GSTACK_HOME="${tmp}/state"`, + extractFn('_link_or_copy'), + ...CLAUDE_OWNERSHIP_HELPERS, + extractFn('_install_managed_skill_md'), + extractFn('_print_windows_copy_note_once'), + extractFn('_link_skill_runtime_assets'), + extractFn('link_claude_skill_dirs'), + `link_claude_skill_dirs "${source}" "${target}"`, + ].join('\n')], { encoding: 'utf8', timeout: 30_000 }); + + expect(result.status).toBe(0); + const installedBytes = fs.readFileSync(path.join(installed, 'SKILL.md'), 'utf8'); + expect(installedBytes).toContain('# new generated body'); + expect(installedBytes).toContain( + '', + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('first upgrade adopts a byte-exact alias from OLD_HEAD after its source changed', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-alias-old-head-')); + try { + const source = path.join(tmp, 'source'); + const skillDir = path.join(source, 'open-gstack-browser'); + const alias = path.join(tmp, 'skills', 'gstack-connect-chrome'); + fs.mkdirSync(skillDir, { recursive: true }); + fs.mkdirSync(alias, { recursive: true }); + initHistoryRepo(source); + const oldSource = '---\nname: open-gstack-browser\n---\n# old alias source\n'; + const newSource = '---\nname: open-gstack-browser\n---\n# new alias source\n'; + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), oldSource); + git(source, ['add', 'open-gstack-browser/SKILL.md']); + git(source, ['commit', '-qm', 'old alias source']); + const oldHead = git(source, ['rev-parse', 'HEAD']); + fs.writeFileSync( + path.join(alias, 'SKILL.md'), + oldSource.replace(/^name:.*$/m, 'name: gstack-connect-chrome'), + ); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), newSource); + git(source, ['add', 'open-gstack-browser/SKILL.md']); + git(source, ['commit', '-qm', 'new alias source']); + + const result = spawnSync('bash', ['-c', [ + 'set -e', + `GSTACK_UPGRADE_FROM_HEAD="${oldHead}"`, + ...CLAUDE_OWNERSHIP_HELPERS, + extractFn('_install_alias_skill_md'), + `_install_alias_skill_md "${skillDir}/SKILL.md" "${alias}" "gstack-connect-chrome" "${source}"`, + ].join('\n')], { encoding: 'utf8', timeout: 30_000 }); + + expect(result.status).toBe(0); + const installedBytes = fs.readFileSync(path.join(alias, 'SKILL.md'), 'utf8'); + expect(installedBytes).toContain('# new alias source'); + expect(installedBytes).toContain( + '', + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + for (const scenario of [ + { label: 'prefix to flat', prefix: 0 as const, oldName: 'gstack-qa', targetName: 'qa' }, + { label: 'flat to prefix', prefix: 1 as const, oldName: 'qa', targetName: 'gstack-qa' }, + ]) { + test(`${scenario.label} destination collision leaves both old and user entries untouched`, () => { + const target = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-setup-preflight-')); + const oldDir = path.join(target, scenario.oldName); + const userDir = path.join(target, scenario.targetName); + const oldBytes = `---\nname: ${scenario.oldName}\n---\nmanaged old\n\n`; + const userBytes = `---\nname: ${scenario.targetName}\n---\nprivate destination\n`; + fs.mkdirSync(oldDir); + fs.writeFileSync(path.join(oldDir, 'SKILL.md'), oldBytes); + fs.mkdirSync(userDir); + fs.writeFileSync(path.join(userDir, 'SKILL.md'), userBytes); + try { + const result = runSetupDestinationPreflight(scenario.prefix, target); + expect(result.status).toBe(2); + expect(result.stderr).toContain('refusing to replace non-gstack skill entry'); + expect(fs.readFileSync(path.join(oldDir, 'SKILL.md'), 'utf8')).toBe(oldBytes); + expect(fs.readFileSync(path.join(userDir, 'SKILL.md'), 'utf8')).toBe(userBytes); + } finally { + fs.rmSync(target, { recursive: true, force: true }); + } + }); + } + + test('both Claude install paths run destination preflight before cleanup', () => { + const installSection = SETUP_SRC.slice(SETUP_SRC.indexOf('# 4. Install for Claude')); + const preflights = [...installSection.matchAll(/preflight_claude_install/g)].map((match) => match.index ?? -1); + const cleanups = [...installSection.matchAll(/# Clean up stale symlinks from the opposite prefix mode/g)].map( + (match) => match.index ?? -1, + ); + expect(preflights.length).toBe(2); + expect(cleanups.length).toBe(2); + expect(preflights[0]).toBeLessThan(cleanups[0]); + expect(preflights[1]).toBeLessThan(cleanups[1]); + }); + + test('normal setup never points the legacy patch helper at SOURCE_GSTACK_DIR', () => { + expect(SETUP_SRC).not.toContain('gstack-patch-names" "$SOURCE_GSTACK_DIR"'); + }); + + test('prefixed setup fails closed on a user-owned gstack-* collision', () => { + const target = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-prefixed-collision-')); + const collision = path.join(target, 'gstack-qa'); + fs.mkdirSync(collision, { recursive: true }); + const userSkill = '---\nname: gstack-qa\n---\n# private QA workflow\n'; + fs.writeFileSync(path.join(collision, 'SKILL.md'), userSkill); + fs.writeFileSync(path.join(collision, 'keep.txt'), 'keep\n'); + try { + const script = [ + 'set -e', + 'IS_WINDOWS=0', + 'SKILL_PREFIX=1', + 'QUIET=1', + '_WINDOWS_COPY_NOTE_PRINTED=1', + `GSTACK_HOME="${target}/state"`, + extractFn('_link_or_copy'), + ...CLAUDE_OWNERSHIP_HELPERS, + extractFn('_install_managed_skill_md'), + extractFn('_print_windows_copy_note_once'), + extractFn('_link_skill_runtime_assets'), + extractFn('link_claude_skill_dirs'), + `link_claude_skill_dirs "${ROOT}" "${target}"`, + ].join('\n'); + const result = spawnSync('bash', ['-c', script], { encoding: 'utf8', timeout: 60_000 }); + expect(result.status).toBe(2); + expect(result.stderr).toContain('refusing to replace non-gstack skill entry'); + expect(fs.readFileSync(path.join(collision, 'SKILL.md'), 'utf8')).toBe(userSkill); + expect(fs.readFileSync(path.join(collision, 'keep.txt'), 'utf8')).toBe('keep\n'); + } finally { + fs.rmSync(target, { recursive: true, force: true }); + } + }); + test('_gstack-command alias is NOT a symlink and carries its own name', () => { const aliasDir = path.join(installDir, '_gstack-command'); const aliasSkill = path.join(aliasDir, 'SKILL.md'); @@ -93,6 +421,76 @@ describe('alias installs are rewritten copies (#2511, #2201)', () => { expect(fs.lstatSync(aliasDir).isSymbolicLink()).toBe(false); expect(fs.lstatSync(aliasSkill).isSymbolicLink()).toBe(false); expect(frontmatterName(aliasSkill)).toBe('connect-chrome'); + expect(fs.readFileSync(aliasSkill, 'utf8')).toContain( + '', + ); + }); + + test('a user-owned gstack-connect-chrome alias collision survives unchanged', () => { + const target = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-alias-collision-')); + const collision = path.join(target, 'gstack-connect-chrome'); + fs.mkdirSync(collision, { recursive: true }); + const userSkill = '---\nname: gstack-connect-chrome\n---\n# user alias\n'; + fs.writeFileSync(path.join(collision, 'SKILL.md'), userSkill); + fs.writeFileSync(path.join(collision, 'keep.txt'), 'keep\n'); + try { + const result = spawnSync('bash', ['-c', [ + 'set -e', + ...CLAUDE_OWNERSHIP_HELPERS, + extractFn('_install_alias_skill_md'), + `_install_alias_skill_md "${ROOT}/open-gstack-browser/SKILL.md" "${collision}" "gstack-connect-chrome"`, + ].join('\n')], { encoding: 'utf8', timeout: 30_000 }); + expect(result.status).toBe(2); + expect(result.stderr).toContain('refusing to replace non-gstack skill entry'); + expect(fs.readFileSync(path.join(collision, 'SKILL.md'), 'utf8')).toBe(userSkill); + expect(fs.readFileSync(path.join(collision, 'keep.txt'), 'utf8')).toBe('keep\n'); + } finally { + fs.rmSync(target, { recursive: true, force: true }); + } + }); + + test('prefix-to-flat cleanup removes only the exact managed connect-chrome alias', () => { + const target = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-alias-prefix-cleanup-')); + const aliasDir = path.join(target, 'gstack-connect-chrome'); + fs.mkdirSync(aliasDir); + fs.writeFileSync( + path.join(aliasDir, 'SKILL.md'), + '---\nname: gstack-connect-chrome\n---\n\n', + ); + try { + const result = spawnSync('bash', ['-c', [ + 'set -e', + 'IS_WINDOWS=0', + extractFn('cleanup_prefixed_claude_symlinks'), + `cleanup_prefixed_claude_symlinks "${ROOT}" "${target}"`, + ].join('\n')], { encoding: 'utf8', timeout: 30_000 }); + expect(result.status).toBe(0); + expect(fs.existsSync(aliasDir)).toBe(false); + } finally { + fs.rmSync(target, { recursive: true, force: true }); + } + }); + + test('flat-to-prefix cleanup removes only the exact managed connect-chrome alias', () => { + const target = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-alias-flat-cleanup-')); + const aliasDir = path.join(target, 'connect-chrome'); + fs.mkdirSync(aliasDir); + fs.writeFileSync( + path.join(aliasDir, 'SKILL.md'), + '---\nname: connect-chrome\n---\n\n', + ); + try { + const result = spawnSync('bash', ['-c', [ + 'set -e', + 'IS_WINDOWS=0', + extractFn('cleanup_old_claude_symlinks'), + `cleanup_old_claude_symlinks "${ROOT}" "${target}"`, + ].join('\n')], { encoding: 'utf8', timeout: 30_000 }); + expect(result.status).toBe(0); + expect(fs.existsSync(aliasDir)).toBe(false); + } finally { + fs.rmSync(target, { recursive: true, force: true }); + } }); test('alias body is the canonical content — only the name: line differs', () => { @@ -100,13 +498,21 @@ describe('alias installs are rewritten copies (#2511, #2201)', () => { path.join(installDir, '_gstack-command', 'SKILL.md'), 'utf-8', ); - expect(alias.replace(/^name:.*$/m, 'name: gstack')).toBe(sourceRootSkill); + expect( + alias + .replace(/^name:.*$/m, 'name: gstack') + .replace(/\n\n$/, ''), + ).toBe(sourceRootSkill); const ogbAlias = fs.readFileSync( path.join(installDir, 'connect-chrome', 'SKILL.md'), 'utf-8', ); - expect(ogbAlias.replace(/^name:.*$/m, 'name: open-gstack-browser')).toBe(sourceOgbSkill); + expect( + ogbAlias + .replace(/^name:.*$/m, 'name: open-gstack-browser') + .replace(/\n\n$/, ''), + ).toBe(sourceOgbSkill); }); test('the SOURCE files are byte-intact (E2: sed never wrote through a symlink)', () => { @@ -145,6 +551,7 @@ describe('alias installs are rewritten copies (#2511, #2201)', () => { 'set -e', 'IS_WINDOWS=0', extractFn('_link_or_copy'), + ...CLAUDE_OWNERSHIP_HELPERS, extractFn('_install_alias_skill_md'), extractFn('link_claude_root_skill_alias'), `link_claude_root_skill_alias "${ROOT}" "${legacyDir}"`, diff --git a/test/setup-cleanup-orphans.test.ts b/test/setup-cleanup-orphans.test.ts index 4869e24fdb..e2b74b386f 100644 --- a/test/setup-cleanup-orphans.test.ts +++ b/test/setup-cleanup-orphans.test.ts @@ -33,17 +33,16 @@ describe('setup: cleanup_old_claude_symlinks — static (#2204)', () => { expect(body).toContain('for old_target in "$skills_dir"/*'); expect(body).toContain('[ "$skill_name" = "gstack" ] && continue'); expect(body).toContain('readlink'); - expect(body).toContain('gstack/*'); expect(body).toContain('gstack-*) continue'); expect(body).toContain('-d "$old_target"'); expect(body).toContain('-L "$old_target/SKILL.md"'); expect(body).toContain('rm -rf "$old_target"'); - // SKILL.md arm must use path-segment provenance, not a bare substring. - expect(body).toContain('gstack/*|*/gstack/*|*/.gstack/render/claude/*'); - expect(body).not.toMatch(/\*gstack\*\)/); + expect(body).toContain('[ "$link_dest" = "$gstack_dir/$skill_name" ]'); + expect(body).toContain('[ "$link_dest" = "$render_dir/$skill_name/SKILL.md" ]'); + expect(body).not.toMatch(/case "\$link_dest" in\s+gstack\/\*\|\*\/gstack\/\*/); }); - test('Windows real-file reap still requires a live payload name list', () => { + test('Windows real-file reap requires a live payload name and ownership marker', () => { const body = cleanupBody(); expect(body).toContain('for skill_dir in "$gstack_dir"/*/'); expect(body).toContain('[ "${IS_WINDOWS:-0}" -eq 1 ] && [ -d "$gstack_dir" ]'); @@ -54,20 +53,22 @@ describe.skipIf(process.platform === 'win32')('setup: cleanup_old_claude_symlink function runCleanup(opts: { isWindows?: '0' | '1'; payload?: boolean; - plant: (skills: string, payload: string) => void; + plant: (skills: string, payload: string, renderDir: string) => void; }): { status: number; stdout: string; stderr: string; names: string[]; tmp: string } { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'cleanup-orphans-')); const skills = path.join(tmp, 'skills'); const payload = path.join(skills, 'gstack'); + const renderDir = path.join(tmp, 'render', 'claude'); fs.mkdirSync(skills, { recursive: true }); if (opts.payload) { fs.mkdirSync(payload, { recursive: true }); } - opts.plant(skills, payload); + opts.plant(skills, payload, renderDir); const gstackArg = opts.payload ? payload : path.join(skills, 'missing-payload'); const script = [ 'set -e', `IS_WINDOWS=${opts.isWindows ?? '0'}`, + `GSTACK_USER_RENDER_DIR="${renderDir}"`, extractFn('cleanup_old_claude_symlinks'), `cleanup_old_claude_symlinks "${gstackArg}" "${skills}"`, ].join('\n'); @@ -207,13 +208,61 @@ describe.skipIf(process.platform === 'win32')('setup: cleanup_old_claude_symlink } }); - test('reaps a leftover whose SKILL.md points at the user render dir', () => { + test('does not remove a whole-dir user symlink into an unrelated gstack checkout', () => { const r = runCleanup({ payload: false, plant(skills) { + const userTarget = path.join(path.dirname(skills), 'user', 'gstack', 'qa'); + fs.mkdirSync(userTarget, { recursive: true }); + fs.writeFileSync(path.join(userTarget, 'keep.txt'), 'user data\n'); + fs.symlinkSync(userTarget, path.join(skills, 'qa')); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.stdout).toBe(''); + expect(r.names).toEqual(['qa']); + expect(fs.lstatSync(path.join(r.tmp, 'skills', 'qa')).isSymbolicLink()).toBe(true); + expect(fs.readFileSync(path.join(r.tmp, 'user', 'gstack', 'qa', 'keep.txt'), 'utf-8')).toBe( + 'user data\n', + ); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('does not remove a user SKILL.md symlink into an unrelated gstack checkout', () => { + const r = runCleanup({ + payload: false, + plant(skills) { + const userSkill = path.join(path.dirname(skills), 'user', 'gstack', 'qa', 'SKILL.md'); + fs.mkdirSync(path.dirname(userSkill), { recursive: true }); + fs.writeFileSync(userSkill, '# user qa\n'); + const installed = path.join(skills, 'qa'); + fs.mkdirSync(installed); + fs.symlinkSync(userSkill, path.join(installed, 'SKILL.md')); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.stdout).toBe(''); + expect(r.names).toEqual(['qa']); + expect(fs.lstatSync(path.join(r.tmp, 'skills', 'qa', 'SKILL.md')).isSymbolicLink()).toBe(true); + expect(fs.readFileSync(path.join(r.tmp, 'user', 'gstack', 'qa', 'SKILL.md'), 'utf-8')).toBe( + '# user qa\n', + ); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('reaps a leftover whose SKILL.md points at the user render dir', () => { + const r = runCleanup({ + payload: false, + plant(skills, _payload, renderDir) { const dir = path.join(skills, 'qa'); fs.mkdirSync(dir); - fs.symlinkSync('../../.gstack/render/claude/qa/SKILL.md', path.join(dir, 'SKILL.md')); + fs.symlinkSync(path.join(renderDir, 'qa', 'SKILL.md'), path.join(dir, 'SKILL.md')); }, }); try { @@ -259,7 +308,7 @@ describe.skipIf(process.platform === 'win32')('setup: cleanup_old_claude_symlink } }); - test('Windows real-file leftover is removed when the payload still names it', () => { + test('Windows unmarked real-file leftover survives even when the payload names it', () => { const r = runCleanup({ isWindows: '1', payload: true, @@ -271,6 +320,31 @@ describe.skipIf(process.platform === 'win32')('setup: cleanup_old_claude_symlink plantUserSkill(skills, 'my-own'); }, }); + try { + expect(r.status).toBe(0); + expect(r.names).toEqual(['gstack', 'my-own', 'qa']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('Windows marked real-file leftover is removed when the payload names it', () => { + const r = runCleanup({ + isWindows: '1', + payload: true, + plant(skills, payload) { + const src = path.join(payload, 'qa'); + fs.mkdirSync(src); + fs.writeFileSync(path.join(src, 'SKILL.md'), '---\nname: qa\n---\n'); + const installed = path.join(skills, 'qa'); + fs.mkdirSync(installed); + fs.writeFileSync( + path.join(installed, 'SKILL.md'), + '---\nname: qa\n---\n\n', + ); + plantUserSkill(skills, 'my-own'); + }, + }); try { expect(r.status).toBe(0); expect(r.names).toEqual(['gstack', 'my-own']); diff --git a/test/setup-runtime-lib-command.test.ts b/test/setup-runtime-lib-command.test.ts index 1105f91529..e10c9cf296 100644 --- a/test/setup-runtime-lib-command.test.ts +++ b/test/setup-runtime-lib-command.test.ts @@ -49,6 +49,7 @@ interface CommandResult { learningsWritten: boolean; libIsSymlink: boolean | null; supabaseConfigPresent: boolean; + scriptsSurfacePresent: boolean; } // Build one host runtime root inside a sandbox using the real setup shell code @@ -68,7 +69,15 @@ function buildRootAndRunCommand( const { script, rootDir } = buildScript(sandbox); const build = spawnSync( 'bash', - ['-c', `IS_WINDOWS=${isWindows}\n${extractFunction('_link_or_copy')}\n${script}`], + ['-c', [ + `IS_WINDOWS=${isWindows}`, + extractFunction('_link_or_copy'), + extractFunction('_sidecar_root_user_owned'), + extractFunction('_runtime_root_owner_marker'), + extractFunction('_runtime_root_owned'), + extractFunction('_prepare_runtime_root'), + script, + ].join('\n')], { encoding: 'utf-8', timeout: 30000 }, ); @@ -99,6 +108,9 @@ function buildRootAndRunCommand( // [ -f ... ] guard means a missing file degrades SILENTLY, so only a // presence check on the installed root catches it. supabaseConfigPresent: fs.existsSync(path.join(rootDir, 'supabase', 'config.sh')), + scriptsSurfacePresent: + fs.existsSync(path.join(rootDir, 'scripts', 'jargon-list.json')) + && fs.existsSync(path.join(rootDir, 'scripts', 'question-registry.ts')), }; } finally { fs.rmSync(sandbox, { recursive: true, force: true }); @@ -138,6 +150,13 @@ const HOST_ROOTS: Record { script: string; rootDir: ].join('\n'), rootDir: path.join(sandbox, 'home', '.opencode', 'skills', 'gstack'), }), + cursor: (sandbox) => ({ + script: [ + extractFunction('create_cursor_runtime_root'), + `create_cursor_runtime_root "${ROOT}" "${sandbox}/home/.cursor/skills/gstack"`, + ].join('\n'), + rootDir: path.join(sandbox, 'home', '.cursor', 'skills', 'gstack'), + }), kiro: (sandbox) => ({ script: [ `HOME="${sandbox}/home"`, @@ -165,6 +184,7 @@ describe.skipIf(process.platform === 'win32')('setup: bin commands resolve sibli expect(r.runStatus).toBe(0); expect(r.learningsWritten).toBe(true); expect(r.supabaseConfigPresent).toBe(true); + expect(r.scriptsSurfacePresent).toBe(true); }); test(`${host} root (Windows copy install): gstack-learnings-log imports ../lib and writes the learning`, () => { @@ -176,6 +196,7 @@ describe.skipIf(process.platform === 'win32')('setup: bin commands resolve sibli expect(r.runStatus).toBe(0); expect(r.learningsWritten).toBe(true); expect(r.supabaseConfigPresent).toBe(true); + expect(r.scriptsSurfacePresent).toBe(true); }); } diff --git a/test/setup-windows-rerun-refresh.test.ts b/test/setup-windows-rerun-refresh.test.ts index 6c60ad47cf..641da14204 100644 --- a/test/setup-windows-rerun-refresh.test.ts +++ b/test/setup-windows-rerun-refresh.test.ts @@ -60,25 +60,39 @@ describe('setup: Windows re-run refresh — static guard sites (#2444)', () => { // ownership, and every sidecar/runtime-root installer must refuse a // user-owned root. A bypass without its gate deletes user data. test.each([ - 'link_codex_skill_dirs', - 'link_factory_skill_dirs', - 'link_opencode_skill_dirs', - 'link_cursor_skill_dirs', - ])('%s gates the Windows real-dir replacement on _owned_for_windows_refresh', (fn) => { - expect(extractFn(fn)).toContain('_owned_for_windows_refresh "$target"'); + ['link_codex_skill_dirs', 'codex'], + ['link_factory_skill_dirs', 'factory'], + ['link_opencode_skill_dirs', 'opencode'], + ['link_cursor_skill_dirs', 'cursor'], + ])('%s binds Windows ownership to its exact host/source/target mapping', (fn, host) => { + expect(extractFn(fn)).toContain( + `_owned_for_windows_refresh "$target" "$skill_dir" "${host}" "$skill_name"`, + ); + expect(extractFn(fn)).toContain( + `_mark_windows_skill_copy "$target" "$skill_dir" "${host}" "$skill_name"`, + ); }); test.each([ 'create_agents_sidecar', 'create_cursor_sidecar', - 'create_cursor_runtime_root', ])('%s refuses a user-owned root via _sidecar_root_user_owned', (fn) => { expect(extractFn(fn)).toContain('_sidecar_root_user_owned'); }); + + test.each([ + 'create_codex_runtime_root', + 'create_factory_runtime_root', + 'create_opencode_runtime_root', + 'create_cursor_runtime_root', + ])('%s preflights whole-root ownership before replacement', (fn) => { + expect(extractFn(fn)).toContain('_prepare_runtime_root'); + expect(extractFn(fn)).not.toContain('rm -rf "$'); + }); }); describe('setup: Windows refresh ownership gate — behavior fixture (#2142)', () => { - test("IS_WINDOWS=1: a user's own real dir on a gstack* name survives; a bannered install refreshes", () => { + test('IS_WINDOWS=1: generic banners and wrong mapping markers do not authorize replacement', () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-owned-')); try { const fake = path.join(tmp, 'gstack'); @@ -91,23 +105,29 @@ describe('setup: Windows refresh ownership gate — behavior fixture (#2142)', ( fs.writeFileSync(path.join(d, 'SKILL.md'), `${banner}upstream-v2\n`); } fs.mkdirSync(skills, { recursive: true }); - // gstack-demo: a prior gstack install (bannered) — must refresh. + // A lookalike generated banner is not proof of this exact copy. fs.mkdirSync(path.join(skills, 'gstack-demo'), { recursive: true }); fs.writeFileSync(path.join(skills, 'gstack-demo', 'SKILL.md'), `${banner}installed-v1\n`); - // gstack-notes: the USER'S own hand-written skill — must survive. + // Nor is a marker issued for another host/source mapping. fs.mkdirSync(path.join(skills, 'gstack-notes'), { recursive: true }); fs.writeFileSync(path.join(skills, 'gstack-notes', 'SKILL.md'), '# my own notes\n'); + fs.writeFileSync( + path.join(skills, 'gstack-notes', '.gstack-owner'), + `gstack-owner-v1 kind=host-skill host=cursor source=${path.join(fake, '.agents', 'skills', 'gstack-demo')} target=${path.join(skills, 'gstack-notes')} served=gstack-notes\n`, + ); const r = runInstaller( '1', - ['_owned_for_windows_refresh', 'link_codex_skill_dirs'], + ['link_codex_skill_dirs'], `link_codex_skill_dirs "${tmp}/gstack" "${skills}"`, ); - expect(r.status).toBe(0); - expect(fs.readFileSync(path.join(skills, 'gstack-demo', 'SKILL.md'), 'utf-8')).toContain('upstream-v2'); + expect(r.status).toBe(2); + expect(fs.readFileSync(path.join(skills, 'gstack-demo', 'SKILL.md'), 'utf-8')).toBe( + `${banner}installed-v1\n`, + ); expect(fs.readFileSync(path.join(skills, 'gstack-notes', 'SKILL.md'), 'utf-8')).toBe('# my own notes\n'); - expect(r.stderr).toContain('left in place'); - expect(r.stderr).toContain('gstack-notes'); + expect(r.stderr).toContain('refusing to replace non-gstack skill entry'); + expect(r.stderr).toContain('gstack-demo'); } finally { fs.rmSync(tmp, { recursive: true, force: true }); } @@ -160,8 +180,13 @@ function runInstaller( extraVars, extractFn('_link_or_copy'), // Ownership gates (#2142) — dependencies of every installer under test. + extractFn('_host_skill_owner_marker'), extractFn('_owned_for_windows_refresh'), + extractFn('_mark_windows_skill_copy'), extractFn('_sidecar_root_user_owned'), + extractFn('_runtime_root_owner_marker'), + extractFn('_runtime_root_owned'), + extractFn('_prepare_runtime_root'), ...fns.map(extractFn), invocation, ].join('\n'); @@ -169,6 +194,104 @@ function runInstaller( return { status: r.status, stdout: r.stdout, stderr: r.stderr }; } +const RUNTIME_ROOT_HOSTS = [ + ['codex', '.agents', 'create_codex_runtime_root'], + ['factory', '.factory', 'create_factory_runtime_root'], + ['opencode', '.opencode', 'create_opencode_runtime_root'], + ['cursor', '.cursor', 'create_cursor_runtime_root'], +] as const; + +describe('setup: runtime root ownership — behavior fixture', () => { + test.each(RUNTIME_ROOT_HOSTS)( + '%s preserves an unmarked or wrongly marked user root byte-for-byte', + (host, generatedDir, fn) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), `gstack-${host}-root-collision-`)); + try { + const fake = path.join(tmp, 'gstack-source'); + const generatedRoot = path.join(fake, generatedDir, 'skills', 'gstack'); + const target = path.join(tmp, 'user-skills', 'gstack'); + fs.mkdirSync(generatedRoot, { recursive: true }); + fs.writeFileSync(path.join(generatedRoot, 'SKILL.md'), `generated-${host}\n`); + fs.mkdirSync(target, { recursive: true }); + const userSkill = '\n# user root\n'; + fs.writeFileSync(path.join(target, 'SKILL.md'), userSkill); + fs.writeFileSync(path.join(target, 'keep.txt'), 'must survive\n'); + fs.writeFileSync( + path.join(target, '.gstack-owner'), + `gstack-owner-v1 kind=runtime-root host=some-other-host source=${fake} target=${target}\n`, + ); + + const r = runInstaller('0', [fn], `${fn} "${fake}" "${target}"`); + expect(r.status).toBe(2); + expect(r.stderr).toContain('refusing to replace non-gstack runtime root'); + expect(fs.readFileSync(path.join(target, 'SKILL.md'), 'utf-8')).toBe(userSkill); + expect(fs.readFileSync(path.join(target, 'keep.txt'), 'utf-8')).toBe('must survive\n'); + expect(fs.readFileSync(path.join(target, '.gstack-owner'), 'utf-8')).toContain( + 'some-other-host', + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }, + ); + + test.each(RUNTIME_ROOT_HOSTS)( + '%s marks a new root and safely refreshes only that exact managed root', + (host, generatedDir, fn) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), `gstack-${host}-root-managed-`)); + try { + const fake = path.join(tmp, 'gstack-source'); + const generatedRoot = path.join(fake, generatedDir, 'skills', 'gstack'); + const target = path.join(tmp, 'user-skills', 'gstack'); + fs.mkdirSync(generatedRoot, { recursive: true }); + fs.writeFileSync(path.join(generatedRoot, 'SKILL.md'), `generated-${host}-v1\n`); + + let r = runInstaller('1', [fn], `${fn} "${fake}" "${target}"`); + expect(r.status).toBe(0); + expect(fs.readFileSync(path.join(target, '.gstack-owner'), 'utf-8')).toBe( + `gstack-owner-v1 kind=runtime-root host=${host} source=${fake} target=${target}\n`, + ); + fs.writeFileSync(path.join(target, 'stale-runtime-file'), 'remove on refresh\n'); + fs.writeFileSync(path.join(generatedRoot, 'SKILL.md'), `generated-${host}-v2\n`); + + r = runInstaller('1', [fn], `${fn} "${fake}" "${target}"`); + expect(r.status).toBe(0); + expect(fs.readFileSync(path.join(target, 'SKILL.md'), 'utf-8')).toBe( + `generated-${host}-v2\n`, + ); + expect(fs.existsSync(path.join(target, 'stale-runtime-file'))).toBe(false); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }, + ); + + test('a byte-exact markerless Windows root is adopted once, then marked', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-runtime-legacy-')); + try { + const fake = path.join(tmp, 'gstack-source'); + const generatedRoot = path.join(fake, '.agents', 'skills', 'gstack'); + const target = path.join(tmp, 'user-skills', 'gstack'); + fs.mkdirSync(generatedRoot, { recursive: true }); + fs.mkdirSync(target, { recursive: true }); + fs.writeFileSync(path.join(generatedRoot, 'SKILL.md'), 'legacy-generated-root\n'); + fs.writeFileSync(path.join(target, 'SKILL.md'), 'legacy-generated-root\n'); + + const r = runInstaller( + '1', + ['create_codex_runtime_root'], + `create_codex_runtime_root "${fake}" "${target}"`, + ); + expect(r.status).toBe(0); + expect(fs.readFileSync(path.join(target, '.gstack-owner'), 'utf-8')).toBe( + `gstack-owner-v1 kind=runtime-root host=codex source=${fake} target=${target}\n`, + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); +}); + describe('setup: Windows re-run refresh — behavior fixture (#2444)', () => { test('IS_WINDOWS=1: link_codex_skill_dirs refreshes an already-installed skill', () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-rerun-')); @@ -178,8 +301,8 @@ describe('setup: Windows re-run refresh — behavior fixture (#2444)', () => { const demo = path.join(fake, '.agents', 'skills', 'gstack-demo'); fs.mkdirSync(demo, { recursive: true }); fs.mkdirSync(skills, { recursive: true }); - // Generated SKILL.md files always carry the banner — the #2142 - // ownership gate keys the Windows refresh on it. + // The first install writes an exact host/source/served marker next to + // the copied skill. Later source changes are refreshable via that marker. const banner = '\n'; fs.writeFileSync(path.join(demo, 'SKILL.md'), `${banner}v1-original\n`); @@ -189,6 +312,9 @@ describe('setup: Windows re-run refresh — behavior fixture (#2444)', () => { const installed = path.join(skills, 'gstack-demo', 'SKILL.md'); expect(fs.readFileSync(installed, 'utf-8')).toBe(`${banner}v1-original\n`); expect(fs.lstatSync(path.join(skills, 'gstack-demo')).isSymbolicLink()).toBe(false); + expect(fs.readFileSync(path.join(skills, 'gstack-demo', '.gstack-owner'), 'utf-8')).toBe( + `gstack-owner-v1 kind=host-skill host=codex source=${demo} target=${path.join(skills, 'gstack-demo')} served=gstack-demo\n`, + ); // Upstream ships a change (the git pull). fs.writeFileSync(path.join(demo, 'SKILL.md'), `${banner}v2-UPDATED\n`); @@ -209,6 +335,12 @@ describe('setup: Windows re-run refresh — behavior fixture (#2444)', () => { fs.mkdirSync(path.join(fake, 'bin'), { recursive: true }); fs.writeFileSync(path.join(fake, 'bin', 'tool.sh'), 'v1\n'); fs.writeFileSync(path.join(fake, 'ETHOS.md'), 'ethos-v1\n'); + const sidecarRoot = path.join(fake, '.agents', 'skills', 'gstack'); + fs.mkdirSync(sidecarRoot, { recursive: true }); + fs.writeFileSync( + path.join(sidecarRoot, 'SKILL.md'), + '---\nname: gstack\n---\n\n', + ); const vars = `SOURCE_GSTACK_DIR="${fake}"`; let r = runInstaller('1', ['create_agents_sidecar'], `create_agents_sidecar "${fake}"`, vars); diff --git a/test/skill-check.test.ts b/test/skill-check.test.ts new file mode 100644 index 0000000000..6779ada58f --- /dev/null +++ b/test/skill-check.test.ts @@ -0,0 +1,89 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { getHostConfig } from '../hosts/index'; +import { freshnessInvocation, hostGeneratesTemplate } from '../scripts/skill-check'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const cleanup: string[] = []; + +function tempDir(prefix: string): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + cleanup.push(dir); + return dir; +} + +afterEach(() => { + for (const dir of cleanup.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +describe('skill:check host-aware freshness', () => { + test('does not require canonical outputs that the Claude host deliberately skips', () => { + const claude = getHostConfig('claude'); + + expect(hostGeneratesTemplate('claude/SKILL.md.tmpl', claude, ROOT)).toBe(false); + expect(hostGeneratesTemplate('codex/SKILL.md.tmpl', claude, ROOT)).toBe(true); + expect(hostGeneratesTemplate('SKILL.md.tmpl', claude, ROOT)).toBe(true); + }); + + test('uses the same effective Codex model resolution as setup', () => { + const codexHome = tempDir('gstack-skill-check-codex-'); + fs.writeFileSync(path.join(codexHome, 'config.toml'), 'model = "gpt-5.6-sol"\n'); + + const invocation = freshnessInvocation(getHostConfig('codex'), { + HOME: tempDir('gstack-skill-check-home-'), + CODEX_HOME: codexHome, + }); + + expect(invocation.model).toBe('gpt-5.6-sol'); + expect(invocation.modelSource).toBe(path.join(codexHome, 'config.toml')); + expect(invocation.args).toEqual([ + 'run', + 'scripts/gen-skill-docs.ts', + '--host', + 'codex', + '--model', + 'gpt-5.6-sol', + '--dry-run', + ]); + }); + + test('reports zero actionable findings when generated artifacts match the setup profile', () => { + const fakeBin = tempDir('gstack-skill-check-bin-'); + const codexHome = tempDir('gstack-skill-check-live-codex-'); + const logPath = path.join(fakeBin, 'invocations.log'); + fs.writeFileSync(path.join(codexHome, 'config.toml'), 'model = "gpt-5.6-sol"\n'); + fs.writeFileSync( + path.join(fakeBin, 'bun'), + '#!/bin/sh\nprintf "%s\\n" "$*" >> "$SKILL_CHECK_INVOCATIONS"\nexit 0\n', + { mode: 0o755 }, + ); + + const result = Bun.spawnSync([process.execPath, path.join(ROOT, 'scripts/skill-check.ts')], { + cwd: ROOT, + timeout: 30_000, + env: { + ...process.env, + CODEX_HOME: codexHome, + PATH: `${fakeBin}:${process.env.PATH ?? ''}`, + SKILL_CHECK_INVOCATIONS: logPath, + }, + stdout: 'pipe', + stderr: 'pipe', + }); + const stdout = result.stdout.toString(); + const invocations = fs.readFileSync(logPath, 'utf8'); + + expect(result.exitCode).toBe(0); + expect(stdout).toContain('claude/SKILL.md.tmpl'); + expect(stdout).toContain('skipped for Claude Code'); + expect(stdout).not.toContain('generated file missing'); + expect(stdout).toContain(`Profile: gpt-5.6-sol (${path.join(codexHome, 'config.toml')})`); + expect(invocations).toContain( + 'run scripts/gen-skill-docs.ts --host codex --model gpt-5.6-sol --dry-run', + ); + }); +}); diff --git a/test/team-mode.test.ts b/test/team-mode.test.ts index 87187f0389..08b3c0b503 100644 --- a/test/team-mode.test.ts +++ b/test/team-mode.test.ts @@ -184,6 +184,12 @@ describe('gstack-session-update', () => { }); expect(result.exitCode).toBe(0); }); + + test('session auto-upgrade never bootstraps or repairs GBrain', () => { + const script = fs.readFileSync(SESSION_UPDATE, 'utf8'); + expect(script).not.toContain('gstack-gbrain-ready'); + expect(script).not.toContain('gstack-gbrain-sync'); + }); }); describe('gstack-team-init', () => { diff --git a/test/test-free-shards-detached-lifecycle.test.ts b/test/test-free-shards-detached-lifecycle.test.ts new file mode 100644 index 0000000000..3e1fa17eb2 --- /dev/null +++ b/test/test-free-shards-detached-lifecycle.test.ts @@ -0,0 +1,489 @@ +/** + * End-to-end proof that a browse daemon which escapes the shard process group + * remains owned by the free-test runner. This is the exact production failure + * shape: setsid server + real Playwright Chromium + deleted state fixture. + */ + +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as net from 'node:net'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { spawn, spawnSync, type ChildProcess } from 'node:child_process'; +import { + registerTestShardProcess, + type TestShardProcessRecord, +} from '../browse/src/test-shard-process-registry'; +import { + createTestShardProcessRegistry, + reapTestShardProcesses, + registryEnvironment, + type TestShardProcessRegistryHandle, +} from '../scripts/test-shard-process-owner'; +import { runFreeShard } from '../scripts/test-free-shards'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const SERVER = path.join(ROOT, 'browse', 'src', 'server.ts'); +const SUMMARY = 'Ran 1 tests across 1 files. [1.00ms]'; + +interface DetachedReceipt { + shardPid: number; + shardPgid: number; + serverPid: number; + serverPgid: number; + chromiumPid: number; + port: number; + stateFile: string; + stateDeletedBeforeShardExit: boolean; +} + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code === 'EPERM'; + } +} + +function readPsField(pid: number, field: 'command=' | 'lstart=' | 'pgid='): string { + const result = spawnSync('ps', ['-p', String(pid), '-o', field], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + timeout: 2_000, + }); + return result.status === 0 ? (result.stdout || '').trim() : ''; +} + +function delay(milliseconds: number): Promise { + return new Promise((resolve) => setTimeout(resolve, milliseconds)); +} + +interface IdentityFixture { + child: ChildProcess; + closed: Promise; + pid: number; + processGroupId: number; + processStartTime: string; + marker: string; +} + +async function startIdentityFixture(registryRoot: string): Promise { + const marker = path.join(registryRoot, 'exact-owner-marker'); + const child = spawn( + process.execPath, + ['-e', 'setInterval(() => {}, 1000)', SERVER, marker], + { detached: true, stdio: 'ignore' }, + ); + if (!child.pid) throw new Error('identity fixture did not expose a PID'); + const closed = new Promise((resolve) => child.once('close', () => resolve())); + const deadline = Date.now() + 5_000; + while (Date.now() < deadline) { + const processGroupRaw = readPsField(child.pid, 'pgid='); + const processStartTime = readPsField(child.pid, 'lstart='); + const command = readPsField(child.pid, 'command='); + if (/^\d+$/.test(processGroupRaw) && processStartTime && command.includes(SERVER) && command.includes(marker)) { + return { + child, + closed, + pid: child.pid, + processGroupId: Number.parseInt(processGroupRaw, 10), + processStartTime, + marker, + }; + } + await delay(25); + } + child.kill('SIGKILL'); + await closed; + throw new Error('identity fixture did not become observable'); +} + +async function stopIdentityFixture(fixture: IdentityFixture): Promise { + if (isAlive(fixture.pid)) { + const command = readPsField(fixture.pid, 'command='); + if (!command.includes(SERVER) || !command.includes(fixture.marker)) { + throw new Error(`refusing to stop PID ${fixture.pid}: identity marker changed`); + } + process.kill(fixture.pid, 'SIGKILL'); + } + await Promise.race([fixture.closed, delay(5_000)]); + if (isAlive(fixture.pid)) throw new Error(`identity fixture PID ${fixture.pid} survived exact cleanup`); +} + +function appendIdentityRecord( + handle: TestShardProcessRegistryHandle, + fixture: IdentityFixture, + identity: Pick, +): void { + const record: TestShardProcessRecord = { + schema: 1, + type: 'process', + runId: handle.runId, + kind: 'browse-server', + pid: fixture.pid, + parentPid: process.pid, + processGroupId: identity.processGroupId, + processStartTime: identity.processStartTime, + port: null, + stateFile: null, + registeredAt: new Date().toISOString(), + }; + fs.appendFileSync(handle.registryPath, `${JSON.stringify(record)}\n`); +} + +function portAcceptsConnections(port: number): Promise { + return new Promise((resolve) => { + const socket = net.createConnection({ host: '127.0.0.1', port }); + const timer = setTimeout(() => { + socket.destroy(); + resolve(false); + }, 500); + socket.once('connect', () => { + clearTimeout(timer); + socket.destroy(); + resolve(true); + }); + socket.once('error', () => { + clearTimeout(timer); + resolve(false); + }); + }); +} + +describe('test-free-shards: detached process ownership', () => { + test('POSIX registration omits a dead fake PID because no process remains to own', () => { + if (process.platform === 'win32') return; + const registryRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-missing-start-')); + try { + const handle = createTestShardProcessRegistry(registryRoot); + expect(registerTestShardProcess({ + kind: 'browse-server', + pid: Number.MAX_SAFE_INTEGER, + }, { ...process.env, ...registryEnvironment(handle) })).toBe(false); + expect(fs.readFileSync(handle.registryPath, 'utf8').trim().split('\n')).toHaveLength(1); + } finally { + fs.rmSync(registryRoot, { recursive: true, force: true }); + } + }); + + test('POSIX registration fails closed for a live PID whose start identity is unavailable', async () => { + if (process.platform === 'win32') return; + const registryRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-live-missing-start-')); + let fixture: IdentityFixture | null = null; + try { + const handle = createTestShardProcessRegistry(registryRoot); + fixture = await startIdentityFixture(registryRoot); + const livePid = fixture.pid; + expect(() => registerTestShardProcess({ + kind: 'browse-server', + pid: livePid, + }, { ...process.env, ...registryEnvironment(handle) }, { + readPsField: () => '', + })).toThrow('cannot prove test-owned browse-server process start time'); + expect(fs.readFileSync(handle.registryPath, 'utf8').trim().split('\n')).toHaveLength(1); + expect(isAlive(fixture.pid)).toBe(true); + } finally { + if (fixture) await stopIdentityFixture(fixture); + fs.rmSync(registryRoot, { recursive: true, force: true }); + } + }, 15_000); + + test('POSIX cleanup refuses a live matching command with missing start identity', async () => { + if (process.platform === 'win32') return; + const registryRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-empty-start-owner-')); + let fixture: IdentityFixture | null = null; + try { + const handle = createTestShardProcessRegistry(registryRoot); + fixture = await startIdentityFixture(registryRoot); + expect(fixture.processGroupId).toBe(fixture.pid); + appendIdentityRecord(handle, fixture, { + processGroupId: fixture.processGroupId, + processStartTime: '', + }); + + const report = await reapTestShardProcesses(handle); + expect(report.success).toBe(false); + expect(report.identityMismatches).toBe(1); + expect(report.termSignals).toBe(0); + expect(report.killSignals).toBe(0); + expect(isAlive(fixture.pid)).toBe(true); + } finally { + if (fixture) await stopIdentityFixture(fixture); + fs.rmSync(registryRoot, { recursive: true, force: true }); + } + }, 15_000); + + test('POSIX cleanup refuses a same-start matching command whose PGID changed', async () => { + if (process.platform === 'win32') return; + const registryRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-pgid-mismatch-')); + let fixture: IdentityFixture | null = null; + try { + const handle = createTestShardProcessRegistry(registryRoot); + fixture = await startIdentityFixture(registryRoot); + expect(fixture.processGroupId).toBe(fixture.pid); + appendIdentityRecord(handle, fixture, { + processGroupId: fixture.processGroupId + 1, + processStartTime: fixture.processStartTime, + }); + + const report = await reapTestShardProcesses(handle); + expect(report.success).toBe(false); + expect(report.identityMismatches).toBe(1); + expect(report.termSignals).toBe(0); + expect(report.killSignals).toBe(0); + expect(isAlive(fixture.pid)).toBe(true); + } finally { + if (fixture) await stopIdentityFixture(fixture); + fs.rmSync(registryRoot, { recursive: true, force: true }); + } + }, 15_000); + + test('a failed process-group census fails the shard and retains its custody state', async () => { + if (process.platform === 'win32') return; + const registryRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-census-failure-')); + let fixture: IdentityFixture | null = null; + let retainedStateDir: string | null = null; + try { + fixture = await startIdentityFixture(registryRoot); + expect(fixture.processGroupId).toBe(fixture.pid); + const registerProgram = ` + const fs = require('node:fs'); + fs.appendFileSync(process.env.GSTACK_TEST_PROCESS_REGISTRY, JSON.stringify({ + schema: 1, + type: 'process', + runId: process.env.GSTACK_TEST_PROCESS_REGISTRY_ID, + kind: 'browse-server', + pid: ${fixture.pid}, + parentPid: process.pid, + processGroupId: ${fixture.processGroupId}, + processStartTime: ${JSON.stringify(fixture.processStartTime)}, + port: null, + stateFile: null, + registeredAt: new Date().toISOString(), + }) + '\\n'); + console.log(${JSON.stringify(SUMMARY)}); + `; + const outcome = await runFreeShard(['census-failure'], 1, 1, { + commandFor: () => ({ command: process.execPath, args: ['-e', registerProgram] }), + processCleanupDependencies: { + listGroupMembers: () => { + throw new Error('injected process-group census failure'); + }, + }, + quiet: true, + log: () => {}, + }); + + expect(outcome.status).toBe('failed'); + expect(outcome.cleanupFailure).toContain('injected process-group census failure'); + const retainedMatch = /shard state retained at (.+)$/.exec(outcome.cleanupFailure ?? ''); + expect(retainedMatch).not.toBeNull(); + retainedStateDir = retainedMatch?.[1] ?? null; + expect(retainedStateDir && fs.existsSync(retainedStateDir)).toBe(true); + expect(isAlive(fixture.pid)).toBe(true); + } finally { + if (fixture) await stopIdentityFixture(fixture); + if (retainedStateDir) fs.rmSync(retainedStateDir, { recursive: true, force: true }); + fs.rmSync(registryRoot, { recursive: true, force: true }); + } + }, 15_000); + + test('reaps a setsid browse daemon and real Chromium after its state fixture is deleted', async () => { + // This test is explicitly excluded from the curated Windows lane. Keep a + // direct guard for developers who invoke this file manually on Windows. + if (process.platform === 'win32') return; + + const captureRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-detached-e2e-')); + const capturePath = path.join(captureRoot, 'receipt.json'); + const daemonLogPath = path.join(captureRoot, 'daemon.log'); + const lines: string[] = []; + const childProgram = ` + const fs = require('node:fs'); + const path = require('node:path'); + const { spawn, spawnSync } = require('node:child_process'); + const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + const pgid = (pid) => { + const result = spawnSync('ps', ['-p', String(pid), '-o', 'pgid='], { encoding: 'utf8', timeout: 2000 }); + return Number.parseInt((result.stdout || '').trim(), 10); + }; + (async () => { + const fixtureRoot = path.join(process.env.TMPDIR, 'deleted-browser-state'); + const stateFile = path.join(fixtureRoot, '.gstack', 'browse.json'); + fs.mkdirSync(path.dirname(stateFile), { recursive: true }); + const daemonLog = fs.openSync(${JSON.stringify(daemonLogPath)}, 'a'); + const daemon = spawn(process.execPath, ['run', ${JSON.stringify(SERVER)}], { + cwd: ${JSON.stringify(ROOT)}, + detached: true, + stdio: ['ignore', daemonLog, daemonLog], + env: { + ...process.env, + BROWSE_STATE_FILE: stateFile, + BROWSE_PARENT_PID: '0', + BROWSE_IDLE_TIMEOUT: '3600000', + }, + }); + fs.closeSync(daemonLog); + daemon.unref(); + + const deadline = Date.now() + 20000; + let state = null; + while (Date.now() < deadline) { + try { + state = JSON.parse(fs.readFileSync(stateFile, 'utf8')); + const health = await fetch('http://127.0.0.1:' + state.port + '/health'); + if (health.ok) break; + } catch {} + state = null; + await delay(100); + } + const processRows = (spawnSync('ps', ['-axo', 'pid=,ppid=,command='], { encoding: 'utf8', timeout: 2000 }).stdout || '') + .split('\\n') + .map((line) => /^\\s*(\\d+)\\s+(\\d+)\\s+(.*)$/.exec(line)) + .filter(Boolean) + .map((match) => ({ pid: Number(match[1]), parentPid: Number(match[2]), command: match[3] })); + const descendants = new Set([state?.pid]); + let changed = true; + while (changed) { + changed = false; + for (const row of processRows) { + if (!descendants.has(row.parentPid) || descendants.has(row.pid)) continue; + descendants.add(row.pid); + changed = true; + } + } + const chromium = processRows.find((row) => + descendants.has(row.pid) && /(?:chrom(?:e|ium)|headless[_-]shell)/i.test(row.command)); + if (!state || !chromium) { + const diagnostic = fs.existsSync(${JSON.stringify(daemonLogPath)}) + ? fs.readFileSync(${JSON.stringify(daemonLogPath)}, 'utf8').slice(-2000) + : 'daemon emitted no log'; + throw new Error('real browse daemon/Chromium did not become healthy: ' + diagnostic); + } + + const receipt = { + shardPid: process.pid, + shardPgid: pgid(process.pid), + serverPid: state.pid, + serverPgid: pgid(state.pid), + chromiumPid: chromium.pid, + port: state.port, + stateFile, + stateDeletedBeforeShardExit: false, + }; + fs.rmSync(fixtureRoot, { recursive: true, force: true }); + receipt.stateDeletedBeforeShardExit = !fs.existsSync(stateFile); + fs.writeFileSync(${JSON.stringify(capturePath)}, JSON.stringify(receipt)); + console.log(${JSON.stringify(SUMMARY)}); + })().catch((error) => { + console.error(error instanceof Error ? error.message : String(error)); + process.exit(1); + }); + `; + + try { + const outcome = await runFreeShard(['detached-lifecycle'], 1, 1, { + commandFor: () => ({ command: process.execPath, args: ['-e', childProgram] }), + quiet: true, + wallTimeoutMs: 45_000, + log: (line) => lines.push(line), + }); + expect(outcome.status).toBe('passed'); + const receipt = JSON.parse(fs.readFileSync(capturePath, 'utf8')) as DetachedReceipt; + expect(receipt.serverPgid).toBe(receipt.serverPid); + expect(receipt.serverPgid).not.toBe(receipt.shardPgid); + expect(receipt.stateDeletedBeforeShardExit).toBe(true); + expect(receipt.chromiumPid).toBeGreaterThan(0); + expect(isAlive(receipt.serverPid)).toBe(false); + expect(isAlive(receipt.chromiumPid)).toBe(false); + expect(await portAcceptsConnections(receipt.port)).toBe(false); + expect(fs.existsSync(receipt.stateFile)).toBe(false); + expect(fs.existsSync(path.dirname(path.dirname(receipt.stateFile)))).toBe(false); + expect(lines.some((line) => /detached-process gate: [2-9]\d* registered, .* 0 survivor\(s\), pass$/.test(line))).toBe(true); + } finally { + fs.rmSync(captureRoot, { recursive: true, force: true }); + } + }, 60_000); + + test('reaps a Chromium helper spawned only after graceful shutdown starts', async () => { + if (process.platform === 'win32') return; + + const captureRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-late-chromium-e2e-')); + const capturePath = path.join(captureRoot, 'receipt.json'); + const lines: string[] = []; + const serverProgram = ` + const fs = require('node:fs'); + const { spawn, spawnSync } = require('node:child_process'); + const pgid = (pid) => Number.parseInt( + (spawnSync('ps', ['-p', String(pid), '-o', 'pgid='], { encoding: 'utf8', timeout: 2000 }).stdout || '').trim(), + 10, + ); + let spawned = false; + process.on('SIGINT', () => { + if (spawned) return; + spawned = true; + const helper = spawn(process.execPath, [ + '-e', + 'process.on("SIGTERM", () => {}); setInterval(() => {}, 1000)', + 'Chromium Helper', + ], { stdio: 'ignore' }); + helper.unref(); + fs.writeFileSync(${JSON.stringify(capturePath)}, JSON.stringify({ + serverPid: process.pid, + serverPgid: pgid(process.pid), + helperPid: helper.pid, + helperPgid: pgid(helper.pid), + })); + }); + process.on('SIGTERM', () => {}); + setInterval(() => {}, 1000); + `; + const shardProgram = ` + const fs = require('node:fs'); + const { spawn, spawnSync } = require('node:child_process'); + const daemon = spawn(process.execPath, ['-e', ${JSON.stringify(serverProgram)}, ${JSON.stringify(SERVER)}], { + detached: true, + stdio: 'ignore', + }); + daemon.unref(); + const ps = (field) => (spawnSync('ps', ['-p', String(daemon.pid), '-o', field], { encoding: 'utf8', timeout: 2000 }).stdout || '').trim(); + fs.appendFileSync(process.env.GSTACK_TEST_PROCESS_REGISTRY, JSON.stringify({ + schema: 1, + type: 'process', + runId: process.env.GSTACK_TEST_PROCESS_REGISTRY_ID, + kind: 'browse-server', + pid: daemon.pid, + parentPid: process.pid, + processGroupId: Number.parseInt(ps('pgid='), 10), + processStartTime: ps('lstart='), + port: null, + stateFile: null, + registeredAt: new Date().toISOString(), + }) + '\\n'); + console.log(${JSON.stringify(SUMMARY)}); + `; + + try { + const outcome = await runFreeShard(['late-chromium-helper'], 1, 1, { + commandFor: () => ({ command: process.execPath, args: ['-e', shardProgram] }), + quiet: true, + wallTimeoutMs: 15_000, + log: (line) => lines.push(line), + }); + expect(outcome.status).toBe('passed'); + const receipt = JSON.parse(fs.readFileSync(capturePath, 'utf8')) as { + serverPid: number; + serverPgid: number; + helperPid: number; + helperPgid: number; + }; + expect(receipt.serverPgid).toBe(receipt.serverPid); + expect(receipt.helperPgid).toBe(receipt.serverPgid); + expect(isAlive(receipt.serverPid)).toBe(false); + expect(isAlive(receipt.helperPid)).toBe(false); + expect(lines.some((line) => /detached-process gate: 2 registered, .* 0 survivor\(s\), pass$/.test(line))).toBe(true); + } finally { + fs.rmSync(captureRoot, { recursive: true, force: true }); + } + }, 30_000); +}); diff --git a/test/test-free-shards.test.ts b/test/test-free-shards.test.ts index c807c9b0e6..0fc21b2591 100644 --- a/test/test-free-shards.test.ts +++ b/test/test-free-shards.test.ts @@ -202,6 +202,11 @@ describe('test-free-shards: strict shard execution', () => { const commandFor = (files: string[]) => { const mode = files[0]; if (mode === 'spin') return { command: process.execPath, args: ['-e', BUSY_LOOP] }; + if (mode === 'spin-corrupt-process-registry') { + const script = 'require("node:fs").appendFileSync(process.env.GSTACK_TEST_PROCESS_REGISTRY, "not-json\\n");' + + BUSY_LOOP; + return { command: process.execPath, args: ['-e', script] }; + } if (mode === 'no-summary') return { command: process.execPath, args: ['-e', 'console.log("ok")'] }; if (mode === 'fail-exit') { return { command: process.execPath, args: ['-e', `console.log(${JSON.stringify(SUMMARY_1)}); process.exit(3)`] }; @@ -212,6 +217,37 @@ describe('test-free-shards: strict shard execution', () => { if (mode === 'wrong-file-count') { return { command: process.execPath, args: ['-e', 'console.log("Ran 3 tests across 4 files. [12.00ms]")'] }; } + if (mode === 'corrupt-process-registry') { + const script = ` + const fs = require('node:fs'); + const path = require('node:path'); + const { spawn, spawnSync } = require('node:child_process'); + const marker = path.join(${JSON.stringify(ROOT)}, 'browse', 'src', 'server.ts'); + const owned = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)', marker], { + detached: true, + stdio: 'ignore', + }); + owned.unref(); + const ps = (field) => (spawnSync('ps', ['-p', String(owned.pid), '-o', field], { encoding: 'utf8', timeout: 2000 }).stdout || '').trim(); + fs.appendFileSync(process.env.GSTACK_TEST_PROCESS_REGISTRY, + JSON.stringify({ + schema: 1, + type: 'process', + runId: process.env.GSTACK_TEST_PROCESS_REGISTRY_ID, + kind: 'browse-server', + pid: owned.pid, + parentPid: process.pid, + processGroupId: Number.parseInt(ps('pgid='), 10), + processStartTime: ps('lstart='), + port: null, + stateFile: null, + registeredAt: new Date().toISOString(), + }) + '\\nnot-json\\n'); + fs.writeFileSync(process.env.GSTACK_CORRUPT_REGISTRY_RECEIPT, String(owned.pid)); + console.log(${JSON.stringify(SUMMARY_1)}); + `; + return { command: process.execPath, args: ['-e', script] }; + } return { command: process.execPath, args: ['-e', `console.log(${JSON.stringify(SUMMARY_1)})`] }; }; @@ -245,6 +281,27 @@ describe('test-free-shards: strict shard execution', () => { expect(outcome.status).toBe('failed'); }); + test('a corrupted detached-process registry fails the shard closed', async () => { + if (process.platform === 'win32') return; + const receiptRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-corrupt-registry-')); + const receiptPath = path.join(receiptRoot, 'owned.pid'); + try { + const outcome = await runFreeShard(['corrupt-process-registry'], 1, 1, { + commandFor, + quiet: true, + log: () => {}, + env: { ...process.env, GSTACK_CORRUPT_REGISTRY_RECEIPT: receiptPath }, + }); + expect(outcome.status).toBe('failed'); + expect(outcome.exitCode).toBe(0); + expect(outcome.unattributedFailures).toBeGreaterThan(0); + const ownedPid = Number.parseInt(fs.readFileSync(receiptPath, 'utf8'), 10); + expect(() => process.kill(ownedPid, 0)).toThrow(); + } finally { + fs.rmSync(receiptRoot, { recursive: true, force: true }); + } + }); + test('a spinning shard is killed at the wall-clock deadline and reported timed-out, distinct from failed', async () => { const lines: string[] = []; const outcome = await runFreeShard(['spin'], 1, 1, { @@ -261,6 +318,18 @@ describe('test-free-shards: strict shard execution', () => { expect(lines.some((l) => /^\[test:free\] shard 1\/1: 1 files, \d+s, timed-out$/.test(l))).toBe(true); }, 30_000); + test('a timeout retains its cleanup failure instead of hiding it behind status 124', async () => { + const outcome = await runFreeShard(['spin-corrupt-process-registry'], 1, 1, { + commandFor, + quiet: true, + wallTimeoutMs: 300, + log: () => {}, + }); + expect(outcome.status).toBe('timed-out'); + expect(outcome.cleanupFailure).toContain('1 invalid registry row(s)'); + expect(outcome.unattributedFailures).toBeGreaterThan(0); + }, 30_000); + test('an empty shard is a fast no-op success and never spawns (stable CI-matrix indices)', async () => { const lines: string[] = []; const outcome = await runFreeShard([], 7, 20, { @@ -297,7 +366,10 @@ describe('test-free-shards: strict shard execution', () => { `const fs = require("fs");` + `fs.writeFileSync(${JSON.stringify(dump)}, JSON.stringify({` + ` home: process.env.GSTACK_HOME ?? null, tmp: process.env.TMPDIR,` - + ` tmpExists: fs.existsSync(process.env.TMPDIR || "") }));` + + ` tmpExists: fs.existsSync(process.env.TMPDIR || ""),` + + ` browseState: process.env.BROWSE_STATE_FILE,` + + ` processRegistry: process.env.GSTACK_TEST_PROCESS_REGISTRY,` + + ` registryExists: fs.existsSync(process.env.GSTACK_TEST_PROCESS_REGISTRY || "") }));` + `console.log(${JSON.stringify(SUMMARY_1)});`; const outcome = await runFreeShard(['env-dump'], 1, 1, { commandFor: () => ({ command: process.execPath, args: ['-e', script] }), @@ -312,7 +384,13 @@ describe('test-free-shards: strict shard execution', () => { expect(seen.tmp).toContain('gstack-free-shard-'); expect(seen.tmpExists).toBe(true); expect(seen.tmp).not.toBe(process.env.TMPDIR ?? ''); + expect(seen.browseState).toContain('gstack-free-shard-'); + expect(seen.processRegistry).toContain('gstack-free-shard-'); + expect(seen.registryExists).toBe(true); + expect(path.dirname(seen.tmp)).toBe(path.dirname(seen.browseState)); + expect(path.dirname(seen.processRegistry)).toBe(path.dirname(seen.browseState)); expect(fs.existsSync(seen.tmp)).toBe(false); + expect(fs.existsSync(seen.processRegistry)).toBe(false); } finally { fs.rmSync(captureDir, { recursive: true, force: true }); }