diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d0704cce..c0b45d640 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed +- **Shell hooks falsely flagged as stale on every session** — `gsd-phase-boundary.sh`, `gsd-session-state.sh`, and `gsd-validate-commit.sh` now ship with a `# gsd-hook-version: {{GSD_VERSION}}` header; the installer substitutes `{{GSD_VERSION}}` in `.sh` hooks the same way it does for `.js` hooks; and the stale-hook detector in `gsd-check-update.js` now matches bash `#` comment syntax in addition to JS `//` syntax. All three changes are required together — neither the regex fix alone nor the install fix alone is sufficient to resolve the false positive (#2136, #2206, #2209, #2210, #2212) + ## [1.36.0] - 2026-04-14 ### Added diff --git a/bin/install.js b/bin/install.js index 710270024..39520c70e 100755 --- a/bin/install.js +++ b/bin/install.js @@ -5761,10 +5761,15 @@ function install(isGlobal, runtime = 'claude') { // Ensure hook files are executable (fixes #1162 — missing +x permission) try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows doesn't support chmod */ } } else { - fs.copyFileSync(srcFile, destFile); - // Ensure .sh hook files are executable (mirrors chmod in build-hooks.js) + // .sh hooks carry a gsd-hook-version header so gsd-check-update.js can + // detect staleness after updates — stamp the version just like .js hooks. if (entry.endsWith('.sh')) { + let content = fs.readFileSync(srcFile, 'utf8'); + content = content.replace(/\{\{GSD_VERSION\}\}/g, pkg.version); + fs.writeFileSync(destFile, content); try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows doesn't support chmod */ } + } else { + fs.copyFileSync(srcFile, destFile); } } } @@ -5876,9 +5881,13 @@ function install(isGlobal, runtime = 'claude') { fs.writeFileSync(destFile, content); try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows */ } } else { - fs.copyFileSync(srcFile, destFile); if (entry.endsWith('.sh')) { + let content = fs.readFileSync(srcFile, 'utf8'); + content = content.replace(/\{\{GSD_VERSION\}\}/g, pkg.version); + fs.writeFileSync(destFile, content); try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows */ } + } else { + fs.copyFileSync(srcFile, destFile); } } } diff --git a/hooks/gsd-check-update-worker.js b/hooks/gsd-check-update-worker.js new file mode 100644 index 000000000..92847b93e --- /dev/null +++ b/hooks/gsd-check-update-worker.js @@ -0,0 +1,107 @@ +#!/usr/bin/env node +// gsd-hook-version: {{GSD_VERSION}} +// Background worker spawned by gsd-check-update.js (SessionStart hook). +// Checks for GSD updates and stale hooks, writes result to cache file. +// Receives paths via environment variables set by the parent hook. +// +// Using a separate file (rather than node -e '') avoids the +// template-literal regex-escaping problem: regex source is plain JS here. + +'use strict'; + +const fs = require('fs'); +const path = require('path'); +const { execFileSync } = require('child_process'); + +const cacheFile = process.env.GSD_CACHE_FILE; +const projectVersionFile = process.env.GSD_PROJECT_VERSION_FILE; +const globalVersionFile = process.env.GSD_GLOBAL_VERSION_FILE; + +// Compare semver: true if a > b (a is strictly newer than b) +// Strips pre-release suffixes (e.g. '3-beta.1' → '3') to avoid NaN from Number() +function isNewer(a, b) { + const pa = (a || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); + const pb = (b || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); + for (let i = 0; i < 3; i++) { + if (pa[i] > pb[i]) return true; + if (pa[i] < pb[i]) return false; + } + return false; +} + +// Check project directory first (local install), then global +let installed = '0.0.0'; +let configDir = ''; +try { + if (fs.existsSync(projectVersionFile)) { + installed = fs.readFileSync(projectVersionFile, 'utf8').trim(); + configDir = path.dirname(path.dirname(projectVersionFile)); + } else if (fs.existsSync(globalVersionFile)) { + installed = fs.readFileSync(globalVersionFile, 'utf8').trim(); + configDir = path.dirname(path.dirname(globalVersionFile)); + } +} catch (e) {} + +// Check for stale hooks — compare hook version headers against installed VERSION +// Hooks are installed at configDir/hooks/ (e.g. ~/.claude/hooks/) (#1421) +// Only check hooks that GSD currently ships — orphaned files from removed features +// (e.g., gsd-intel-*.js) must be ignored to avoid permanent stale warnings (#1750) +const MANAGED_HOOKS = [ + 'gsd-check-update-worker.js', + 'gsd-check-update.js', + 'gsd-context-monitor.js', + 'gsd-phase-boundary.sh', + 'gsd-prompt-guard.js', + 'gsd-read-guard.js', + 'gsd-session-state.sh', + 'gsd-statusline.js', + 'gsd-validate-commit.sh', + 'gsd-workflow-guard.js', +]; + +let staleHooks = []; +if (configDir) { + const hooksDir = path.join(configDir, 'hooks'); + try { + if (fs.existsSync(hooksDir)) { + const hookFiles = fs.readdirSync(hooksDir).filter(f => MANAGED_HOOKS.includes(f)); + for (const hookFile of hookFiles) { + try { + const content = fs.readFileSync(path.join(hooksDir, hookFile), 'utf8'); + // Match both JS (//) and bash (#) comment styles + const versionMatch = content.match(/(?:\/\/|#) gsd-hook-version:\s*(.+)/); + if (versionMatch) { + const hookVersion = versionMatch[1].trim(); + if (isNewer(installed, hookVersion) && !hookVersion.includes('{{')) { + staleHooks.push({ file: hookFile, hookVersion, installedVersion: installed }); + } + } else { + // No version header at all — definitely stale (pre-version-tracking) + staleHooks.push({ file: hookFile, hookVersion: 'unknown', installedVersion: installed }); + } + } catch (e) {} + } + } + } catch (e) {} +} + +let latest = null; +try { + latest = execFileSync('npm', ['view', 'get-shit-done-cc', 'version'], { + encoding: 'utf8', + timeout: 10000, + windowsHide: true, + }).trim(); +} catch (e) {} + +const result = { + update_available: latest && isNewer(latest, installed), + installed, + latest: latest || 'unknown', + checked: Math.floor(Date.now() / 1000), + stale_hooks: staleHooks.length > 0 ? staleHooks : undefined, +}; + +if (cacheFile) { + try { fs.writeFileSync(cacheFile, JSON.stringify(result)); } catch (e) {} +} diff --git a/hooks/gsd-check-update.js b/hooks/gsd-check-update.js index 8d0ef9c28..6b400df39 100755 --- a/hooks/gsd-check-update.js +++ b/hooks/gsd-check-update.js @@ -44,99 +44,21 @@ if (!fs.existsSync(cacheDir)) { fs.mkdirSync(cacheDir, { recursive: true }); } -// Run check in background (spawn background process, windowsHide prevents console flash) -const child = spawn(process.execPath, ['-e', ` - const fs = require('fs'); - const path = require('path'); - const { execSync } = require('child_process'); - - // Compare semver: true if a > b (a is strictly newer than b) - // Strips pre-release suffixes (e.g. '3-beta.1' → '3') to avoid NaN from Number() - function isNewer(a, b) { - const pa = (a || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); - const pb = (b || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); - for (let i = 0; i < 3; i++) { - if (pa[i] > pb[i]) return true; - if (pa[i] < pb[i]) return false; - } - return false; - } - - const cacheFile = ${JSON.stringify(cacheFile)}; - const projectVersionFile = ${JSON.stringify(projectVersionFile)}; - const globalVersionFile = ${JSON.stringify(globalVersionFile)}; - - // Check project directory first (local install), then global - let installed = '0.0.0'; - let configDir = ''; - try { - if (fs.existsSync(projectVersionFile)) { - installed = fs.readFileSync(projectVersionFile, 'utf8').trim(); - configDir = path.dirname(path.dirname(projectVersionFile)); - } else if (fs.existsSync(globalVersionFile)) { - installed = fs.readFileSync(globalVersionFile, 'utf8').trim(); - configDir = path.dirname(path.dirname(globalVersionFile)); - } - } catch (e) {} - - // Check for stale hooks — compare hook version headers against installed VERSION - // Hooks are installed at configDir/hooks/ (e.g. ~/.claude/hooks/) (#1421) - // Only check hooks that GSD currently ships — orphaned files from removed features - // (e.g., gsd-intel-*.js) must be ignored to avoid permanent stale warnings (#1750) - const MANAGED_HOOKS = [ - 'gsd-check-update.js', - 'gsd-context-monitor.js', - 'gsd-phase-boundary.sh', - 'gsd-prompt-guard.js', - 'gsd-read-guard.js', - 'gsd-session-state.sh', - 'gsd-statusline.js', - 'gsd-validate-commit.sh', - 'gsd-workflow-guard.js', - ]; - let staleHooks = []; - if (configDir) { - const hooksDir = path.join(configDir, 'hooks'); - try { - if (fs.existsSync(hooksDir)) { - const hookFiles = fs.readdirSync(hooksDir).filter(f => MANAGED_HOOKS.includes(f)); - for (const hookFile of hookFiles) { - try { - const content = fs.readFileSync(path.join(hooksDir, hookFile), 'utf8'); - const versionMatch = content.match(/\\/\\/ gsd-hook-version:\\s*(.+)/); - if (versionMatch) { - const hookVersion = versionMatch[1].trim(); - if (isNewer(installed, hookVersion) && !hookVersion.includes('{{')) { - staleHooks.push({ file: hookFile, hookVersion, installedVersion: installed }); - } - } else { - // No version header at all — definitely stale (pre-version-tracking) - staleHooks.push({ file: hookFile, hookVersion: 'unknown', installedVersion: installed }); - } - } catch (e) {} - } - } - } catch (e) {} - } - - let latest = null; - try { - latest = execSync('npm view get-shit-done-cc version', { encoding: 'utf8', timeout: 10000, windowsHide: true }).trim(); - } catch (e) {} - - const result = { - update_available: latest && isNewer(latest, installed), - installed, - latest: latest || 'unknown', - checked: Math.floor(Date.now() / 1000), - stale_hooks: staleHooks.length > 0 ? staleHooks : undefined - }; - - fs.writeFileSync(cacheFile, JSON.stringify(result)); -`], { +// Run check in background via a dedicated worker script. +// Spawning a file (rather than node -e '') keeps the worker logic +// in plain JS with no template-literal regex-escaping concerns, and makes the +// worker independently testable. +const workerPath = path.join(__dirname, 'gsd-check-update-worker.js'); +const child = spawn(process.execPath, [workerPath], { stdio: 'ignore', windowsHide: true, - detached: true // Required on Windows for proper process detachment + detached: true, // Required on Windows for proper process detachment + env: { + ...process.env, + GSD_CACHE_FILE: cacheFile, + GSD_PROJECT_VERSION_FILE: projectVersionFile, + GSD_GLOBAL_VERSION_FILE: globalVersionFile, + }, }); child.unref(); diff --git a/hooks/gsd-phase-boundary.sh b/hooks/gsd-phase-boundary.sh index 6b0c39f07..e2e1c5f3d 100755 --- a/hooks/gsd-phase-boundary.sh +++ b/hooks/gsd-phase-boundary.sh @@ -1,4 +1,5 @@ #!/bin/bash +# gsd-hook-version: {{GSD_VERSION}} # gsd-phase-boundary.sh — PostToolUse hook: detect .planning/ file writes # Outputs a reminder when planning files are modified outside normal workflow. # Uses Node.js for JSON parsing (always available in GSD projects, no jq dependency). diff --git a/hooks/gsd-session-state.sh b/hooks/gsd-session-state.sh index 53f70c646..bbe8b9356 100755 --- a/hooks/gsd-session-state.sh +++ b/hooks/gsd-session-state.sh @@ -1,4 +1,5 @@ #!/bin/bash +# gsd-hook-version: {{GSD_VERSION}} # gsd-session-state.sh — SessionStart hook: inject project state reminder # Outputs STATE.md head on every session start for orientation. # diff --git a/hooks/gsd-validate-commit.sh b/hooks/gsd-validate-commit.sh index fd2dd38a5..c7737093e 100755 --- a/hooks/gsd-validate-commit.sh +++ b/hooks/gsd-validate-commit.sh @@ -1,4 +1,5 @@ #!/bin/bash +# gsd-hook-version: {{GSD_VERSION}} # gsd-validate-commit.sh — PreToolUse hook: enforce Conventional Commits format # Blocks git commit commands with non-conforming messages (exit 2). # Allows conforming messages and all non-commit commands (exit 0). diff --git a/scripts/build-hooks.js b/scripts/build-hooks.js index ff19aeba5..a2f136d47 100644 --- a/scripts/build-hooks.js +++ b/scripts/build-hooks.js @@ -15,6 +15,7 @@ const DIST_DIR = path.join(HOOKS_DIR, 'dist'); // Hooks to copy (pure Node.js, no bundling needed) const HOOKS_TO_COPY = [ + 'gsd-check-update-worker.js', 'gsd-check-update.js', 'gsd-context-monitor.js', 'gsd-prompt-guard.js', diff --git a/tests/bug-2136-sh-hook-version.test.cjs b/tests/bug-2136-sh-hook-version.test.cjs new file mode 100644 index 000000000..8d5b8f219 --- /dev/null +++ b/tests/bug-2136-sh-hook-version.test.cjs @@ -0,0 +1,359 @@ +/** + * Regression tests for bug #2136 / #2206 + * + * Root cause: three bash hooks (gsd-phase-boundary.sh, gsd-session-state.sh, + * gsd-validate-commit.sh) shipped without a gsd-hook-version header, and the + * stale-hook detector in gsd-check-update.js only matched JavaScript comment + * syntax (//) — not bash comment syntax (#). + * + * Result: every session showed "⚠ stale hooks — run /gsd-update" immediately + * after a fresh install, because the detector saw hookVersion: 'unknown' for + * all three bash hooks. + * + * This fix requires THREE parts working in concert: + * 1. Bash hooks ship with "# gsd-hook-version: {{GSD_VERSION}}" + * 2. install.js substitutes {{GSD_VERSION}} in .sh files at install time + * 3. gsd-check-update.js regex matches both "//" and "#" comment styles + * + * Neither fix alone is sufficient: + * - Headers + regex fix only (no install.js fix): installed hooks contain + * literal "{{GSD_VERSION}}" — the {{-guard silently skips them, making + * bash hook staleness permanently undetectable after future updates. + * - Headers + install.js fix only (no regex fix): installed hooks are + * stamped correctly but the detector still can't read bash "#" comments, + * so they still land in the "unknown / stale" branch on every session. + */ + +'use strict'; + +// NOTE: Do NOT set GSD_TEST_MODE here — the E2E install tests spawn the +// real installer subprocess, which skips all install logic when GSD_TEST_MODE=1. + +const { describe, test, before, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const { execFileSync } = require('child_process'); + +const HOOKS_DIR = path.join(__dirname, '..', 'hooks'); +const CHECK_UPDATE_FILE = path.join(HOOKS_DIR, 'gsd-check-update.js'); +const WORKER_FILE = path.join(HOOKS_DIR, 'gsd-check-update-worker.js'); +const INSTALL_SCRIPT = path.join(__dirname, '..', 'bin', 'install.js'); +const BUILD_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); + +const SH_HOOKS = [ + 'gsd-phase-boundary.sh', + 'gsd-session-state.sh', + 'gsd-validate-commit.sh', +]; + +// ─── Ensure hooks/dist/ is populated before install tests ──────────────────── + +before(() => { + execFileSync(process.execPath, [BUILD_SCRIPT], { + encoding: 'utf-8', + stdio: 'pipe', + }); +}); + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +function createTempDir(prefix) { + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); +} + +function cleanup(dir) { + try { fs.rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ } +} + +function runInstaller(configDir) { + execFileSync(process.execPath, [INSTALL_SCRIPT, '--claude', '--global', '--yes'], { + encoding: 'utf-8', + stdio: 'pipe', + env: { ...process.env, CLAUDE_CONFIG_DIR: configDir }, + }); + return path.join(configDir, 'hooks'); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Part 1: Bash hook sources carry the version header placeholder +// ───────────────────────────────────────────────────────────────────────────── + +describe('bug #2136 part 1: bash hook sources carry gsd-hook-version placeholder', () => { + for (const sh of SH_HOOKS) { + test(`${sh} contains "# gsd-hook-version: {{GSD_VERSION}}"`, () => { + const content = fs.readFileSync(path.join(HOOKS_DIR, sh), 'utf8'); + assert.ok( + content.includes('# gsd-hook-version: {{GSD_VERSION}}'), + `${sh} must include "# gsd-hook-version: {{GSD_VERSION}}" so the ` + + `installer can stamp it and gsd-check-update.js can detect staleness` + ); + }); + } + + test('version header is on line 2 (immediately after shebang)', () => { + // Placing the header immediately after #!/bin/bash ensures it is always + // found regardless of how much of the file is read. + for (const sh of SH_HOOKS) { + const lines = fs.readFileSync(path.join(HOOKS_DIR, sh), 'utf8').split('\n'); + assert.strictEqual(lines[0], '#!/bin/bash', `${sh} line 1 must be #!/bin/bash`); + assert.ok( + lines[1].startsWith('# gsd-hook-version:'), + `${sh} line 2 must be the gsd-hook-version header (got: "${lines[1]}")` + ); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Part 2: gsd-check-update-worker.js regex handles bash "#" comment syntax +// (Logic moved from inline -e template literal to dedicated worker file) +// ───────────────────────────────────────────────────────────────────────────── + +describe('bug #2136 part 2: stale-hook detector handles bash comment syntax', () => { + let src; + + before(() => { + src = fs.readFileSync(WORKER_FILE, 'utf8'); + }); + + test('version regex in source matches "#" comment syntax in addition to "//"', () => { + // The regex string in the source must contain the alternation for "#". + // The worker uses plain JS (no template-literal escaping), so the form is + // "(?:\/\/|#)" directly in source. + const hasBashAlternative = + src.includes('(?:\\/\\/|#)') || // escaped form (old template-literal style) + src.includes('(?:\/\/|#)'); // direct form in plain JS worker + assert.ok( + hasBashAlternative, + 'gsd-check-update-worker.js version regex must include an alternative for bash "#" comments. ' + + 'Expected to find (?:\\/\\/|#) or (?:\/\/|#) in the source. ' + + 'The original "//" only regex causes bash hooks to always report hookVersion: "unknown"' + ); + }); + + test('version regex does not use the old JS-only form as the sole pattern', () => { + // The old regex inside the template literal was the string: + // /\\/\\/ gsd-hook-version:\\s*(.+)/ + // which, when evaluated in the subprocess, produced: /\/\/ gsd-hook-version:\s*(.+)/ + // That only matched JS "//" comments — never bash "#". + // We verify that the old exact string no longer appears. + assert.ok( + !src.includes('\\/\\/ gsd-hook-version'), + 'gsd-check-update-worker.js must not use the old JS-only (\\/\\/ gsd-hook-version) ' + + 'escape form as the sole version matcher — it cannot match bash "#" comments' + ); + }); + + test('version regex correctly matches both bash and JS hook version headers', () => { + // Verify that the versionMatch line in the source uses a regex that matches + // both bash "#" and JS "//" comment styles. We check the source contains the + // expected alternation, then directly test the known required pattern. + // + // We do NOT try to extract and evaluate the regex from source (it contains ")" + // which breaks simple extraction), so instead we confirm the source matches + // our expectation and run the regex itself. + assert.ok( + src.includes('gsd-hook-version'), + 'gsd-check-update-worker.js must contain a gsd-hook-version version check' + ); + + // The fixed regex that must be present: matches both comment styles + const fixedRegex = /(?:\/\/|#) gsd-hook-version:\s*(.+)/; + + assert.ok( + fixedRegex.test('# gsd-hook-version: 1.36.0'), + 'bash-style "# gsd-hook-version: X" must be matchable by the required regex' + ); + assert.ok( + fixedRegex.test('// gsd-hook-version: 1.36.0'), + 'JS-style "// gsd-hook-version: X" must still match (no regression)' + ); + assert.ok( + !fixedRegex.test('gsd-hook-version: 1.36.0'), + 'line without a comment prefix must not match (prevents false positives)' + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Part 3a: install.js bundled path substitutes {{GSD_VERSION}} in .sh hooks +// ───────────────────────────────────────────────────────────────────────────── + +describe('bug #2136 part 3a: install.js bundled path substitutes {{GSD_VERSION}} in .sh hooks', () => { + let src; + + before(() => { + src = fs.readFileSync(INSTALL_SCRIPT, 'utf8'); + }); + + test('.sh branch in bundled hook copy loop reads file and substitutes GSD_VERSION', () => { + // Anchor on configDirReplacement — unique to the bundled-hooks path. + const anchorIdx = src.indexOf('configDirReplacement'); + assert.ok(anchorIdx !== -1, 'bundled hook copy loop anchor (configDirReplacement) not found'); + + // Window large enough for the if/else block + const region = src.slice(anchorIdx, anchorIdx + 2000); + + assert.ok( + region.includes("entry.endsWith('.sh')"), + "bundled hook copy loop must check entry.endsWith('.sh')" + ); + assert.ok( + region.includes('GSD_VERSION'), + 'bundled .sh branch must reference GSD_VERSION substitution. Without this, ' + + 'installed .sh hooks contain the literal "{{GSD_VERSION}}" placeholder and ' + + 'bash hook staleness becomes permanently undetectable after future updates' + ); + // copyFileSync on a .sh file would skip substitution — ensure we read+write instead + const shBranchIdx = region.indexOf("entry.endsWith('.sh')"); + const shBranchRegion = region.slice(shBranchIdx, shBranchIdx + 400); + assert.ok( + shBranchRegion.includes('readFileSync') || shBranchRegion.includes('writeFileSync'), + 'bundled .sh branch must read the file (readFileSync) to perform substitution, ' + + 'not copyFileSync directly (which skips template expansion)' + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Part 3b: install.js Codex path also substitutes {{GSD_VERSION}} in .sh hooks +// ───────────────────────────────────────────────────────────────────────────── + +describe('bug #2136 part 3b: install.js Codex path substitutes {{GSD_VERSION}} in .sh hooks', () => { + let src; + + before(() => { + src = fs.readFileSync(INSTALL_SCRIPT, 'utf8'); + }); + + test('.sh branch in Codex hook copy block substitutes GSD_VERSION', () => { + // Anchor on codexHooksSrc — unique to the Codex path. + const anchorIdx = src.indexOf('codexHooksSrc'); + assert.ok(anchorIdx !== -1, 'Codex hook copy block anchor (codexHooksSrc) not found'); + + const region = src.slice(anchorIdx, anchorIdx + 2000); + + assert.ok( + region.includes("entry.endsWith('.sh')"), + "Codex hook copy block must check entry.endsWith('.sh')" + ); + assert.ok( + region.includes('GSD_VERSION'), + 'Codex .sh branch must substitute {{GSD_VERSION}}. The bundled path was fixed ' + + 'but Codex installs a separate copy of the hooks from hooks/dist that also needs stamping' + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Part 4: End-to-end — installed .sh hooks have stamped version, not placeholder +// ───────────────────────────────────────────────────────────────────────────── + +describe('bug #2136 part 4: installed .sh hooks contain stamped concrete version', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempDir('gsd-2136-install-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('installed .sh hooks contain a concrete version string, not the template placeholder', () => { + const hooksDir = runInstaller(tmpDir); + + for (const sh of SH_HOOKS) { + const hookPath = path.join(hooksDir, sh); + assert.ok(fs.existsSync(hookPath), `${sh} must be installed`); + + const content = fs.readFileSync(hookPath, 'utf8'); + + assert.ok( + content.includes('# gsd-hook-version:'), + `installed ${sh} must contain a "# gsd-hook-version:" header` + ); + assert.ok( + !content.includes('{{GSD_VERSION}}'), + `installed ${sh} must not contain literal "{{GSD_VERSION}}" — ` + + `install.js must substitute it with the concrete package version` + ); + + const versionMatch = content.match(/# gsd-hook-version:\s*(\S+)/); + assert.ok(versionMatch, `installed ${sh} version header must have a version value`); + assert.match( + versionMatch[1], + /^\d+\.\d+\.\d+/, + `installed ${sh} version "${versionMatch[1]}" must be a semver-like string` + ); + } + }); + + test('stale-hook detector reports zero stale bash hooks immediately after fresh install', () => { + // This is the definitive end-to-end proof: after install, run the actual + // version-check logic (extracted from gsd-check-update.js) against the + // installed hooks and verify none are flagged stale. + const hooksDir = runInstaller(tmpDir); + const pkg = require(path.join(__dirname, '..', 'package.json')); + const installedVersion = pkg.version; + + // Build a subprocess that runs the staleness check logic in isolation. + // We pass the installed version, hooks dir, and hook filenames as JSON + // to avoid any injection risk. + const checkScript = ` + 'use strict'; + const fs = require('fs'); + const path = require('path'); + + function isNewer(a, b) { + const pa = (a || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); + const pb = (b || '').split('.').map(s => Number(s.replace(/-.*/, '')) || 0); + for (let i = 0; i < 3; i++) { + if (pa[i] > pb[i]) return true; + if (pa[i] < pb[i]) return false; + } + return false; + } + + const hooksDir = ${JSON.stringify(hooksDir)}; + const installed = ${JSON.stringify(installedVersion)}; + const shHooks = ${JSON.stringify(SH_HOOKS)}; + // Use the same regex that the fixed gsd-check-update.js uses + const versionRe = /(?:\\/\\/|#) gsd-hook-version:\\s*(.+)/; + + const staleHooks = []; + for (const hookFile of shHooks) { + const hookPath = path.join(hooksDir, hookFile); + if (!fs.existsSync(hookPath)) { + staleHooks.push({ file: hookFile, hookVersion: 'missing' }); + continue; + } + const content = fs.readFileSync(hookPath, 'utf8'); + const m = content.match(versionRe); + if (m) { + const hookVersion = m[1].trim(); + if (isNewer(installed, hookVersion) && !hookVersion.includes('{{')) { + staleHooks.push({ file: hookFile, hookVersion, installedVersion: installed }); + } + } else { + staleHooks.push({ file: hookFile, hookVersion: 'unknown', installedVersion: installed }); + } + } + process.stdout.write(JSON.stringify(staleHooks)); + `; + + const result = execFileSync(process.execPath, ['-e', checkScript], { encoding: 'utf8' }); + const staleHooks = JSON.parse(result); + + assert.deepStrictEqual( + staleHooks, + [], + `Fresh install must produce zero stale bash hooks.\n` + + `Got: ${JSON.stringify(staleHooks, null, 2)}\n` + + `This indicates either the version header was not stamped by install.js, ` + + `or the detector regex cannot match bash "#" comment syntax.` + ); + }); +}); diff --git a/tests/core.test.cjs b/tests/core.test.cjs index 39ba9e2a2..162135355 100644 --- a/tests/core.test.cjs +++ b/tests/core.test.cjs @@ -1071,8 +1071,10 @@ describe('stale hook filter', () => { describe('stale hook path', () => { test('gsd-check-update.js checks configDir/hooks/ where hooks are actually installed (#1421)', () => { + // The stale-hook scan logic lives in the worker (moved from inline -e template literal). + // The worker receives configDir via env and constructs the hooksDir path. const content = fs.readFileSync( - path.join(__dirname, '..', 'hooks', 'gsd-check-update.js'), 'utf-8' + path.join(__dirname, '..', 'hooks', 'gsd-check-update-worker.js'), 'utf-8' ); // Hooks are installed at configDir/hooks/ (e.g. ~/.claude/hooks/), // not configDir/get-shit-done/hooks/ which doesn't exist (#1421) diff --git a/tests/managed-hooks.test.cjs b/tests/managed-hooks.test.cjs index 5c6912714..21352ee99 100644 --- a/tests/managed-hooks.test.cjs +++ b/tests/managed-hooks.test.cjs @@ -18,7 +18,9 @@ const fs = require('fs'); const path = require('path'); const HOOKS_DIR = path.join(__dirname, '..', 'hooks'); -const CHECK_UPDATE_FILE = path.join(HOOKS_DIR, 'gsd-check-update.js'); +// MANAGED_HOOKS now lives in the worker script (extracted from inline -e code +// to avoid template-literal regex-escaping concerns). The test reads the worker. +const MANAGED_HOOKS_FILE = path.join(HOOKS_DIR, 'gsd-check-update-worker.js'); describe('bug #2136: MANAGED_HOOKS must include all shipped hook files', () => { let src; @@ -26,12 +28,12 @@ describe('bug #2136: MANAGED_HOOKS must include all shipped hook files', () => { let shippedHooks; // Read once — all tests share the same source snapshot - src = fs.readFileSync(CHECK_UPDATE_FILE, 'utf-8'); + src = fs.readFileSync(MANAGED_HOOKS_FILE, 'utf-8'); // Extract the MANAGED_HOOKS array entries from the source // The array is defined as a multi-line array literal of quoted strings const match = src.match(/const MANAGED_HOOKS\s*=\s*\[([\s\S]*?)\]/); - assert.ok(match, 'MANAGED_HOOKS array not found in gsd-check-update.js'); + assert.ok(match, 'MANAGED_HOOKS array not found in gsd-check-update-worker.js'); managedHooks = match[1] .split('\n') @@ -47,7 +49,7 @@ describe('bug #2136: MANAGED_HOOKS must include all shipped hook files', () => { for (const hookFile of jsHooks) { assert.ok( managedHooks.includes(hookFile), - `${hookFile} is shipped in hooks/ but missing from MANAGED_HOOKS in gsd-check-update.js` + `${hookFile} is shipped in hooks/ but missing from MANAGED_HOOKS in gsd-check-update-worker.js` ); } }); @@ -57,7 +59,7 @@ describe('bug #2136: MANAGED_HOOKS must include all shipped hook files', () => { for (const hookFile of shHooks) { assert.ok( managedHooks.includes(hookFile), - `${hookFile} is shipped in hooks/ but missing from MANAGED_HOOKS in gsd-check-update.js` + `${hookFile} is shipped in hooks/ but missing from MANAGED_HOOKS in gsd-check-update-worker.js` ); } }); diff --git a/tests/orphaned-hooks.test.cjs b/tests/orphaned-hooks.test.cjs index 83d4aff19..b724b9dae 100644 --- a/tests/orphaned-hooks.test.cjs +++ b/tests/orphaned-hooks.test.cjs @@ -11,31 +11,41 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +// MANAGED_HOOKS lives in the worker file (extracted from inline -e code to eliminate +// template-literal regex-escaping concerns). Tests read the worker directly. const CHECK_UPDATE_PATH = path.join(__dirname, '..', 'hooks', 'gsd-check-update.js'); +const WORKER_PATH = path.join(__dirname, '..', 'hooks', 'gsd-check-update-worker.js'); const BUILD_HOOKS_PATH = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); describe('orphaned hooks stale detection (#1750)', () => { test('stale hook scanner uses an allowlist of managed hooks, not a wildcard', () => { - const content = fs.readFileSync(CHECK_UPDATE_PATH, 'utf8'); + const content = fs.readFileSync(WORKER_PATH, 'utf8'); // The scanner MUST NOT use a broad `startsWith('gsd-')` filter that catches // orphaned files from removed features (gsd-intel-index.js, gsd-intel-prune.js, etc.) // Instead, it should reference a known set of managed hook filenames. - - // Extract the spawned child script (everything between the template literal backticks) - const childScriptMatch = content.match(/spawn\(process\.execPath,\s*\['-e',\s*`([\s\S]*?)`\]/); - assert.ok(childScriptMatch, 'should find the spawned child script'); - const childScript = childScriptMatch[1]; - - // The child script must NOT have a broad gsd-*.js wildcard filter - const hasBroadFilter = /readdirSync\([^)]+\)\.filter\([^)]*startsWith\('gsd-'\)\s*&&[^)]*endsWith\('\.js'\)/s.test(childScript); + const hasBroadFilter = /readdirSync\([^)]+\)\.filter\([^)]*startsWith\('gsd-'\)\s*&&[^)]*endsWith\('\.js'\)/s.test(content); assert.ok(!hasBroadFilter, 'scanner must NOT use broad startsWith("gsd-") && endsWith(".js") filter — ' + 'this catches orphaned hooks from removed features (e.g., gsd-intel-index.js). ' + 'Use a MANAGED_HOOKS allowlist instead.'); }); - test('managed hooks list in check-update matches build-hooks HOOKS_TO_COPY JS entries', () => { + test('gsd-check-update.js spawns the worker by file path (not inline -e code)', () => { + // After the worker extraction, the main hook must spawn the worker file + // rather than embedding all logic in a template literal. + const content = fs.readFileSync(CHECK_UPDATE_PATH, 'utf8'); + assert.ok( + content.includes('gsd-check-update-worker.js'), + 'gsd-check-update.js must reference gsd-check-update-worker.js as the spawn target' + ); + assert.ok( + !content.includes("'-e'"), + 'gsd-check-update.js must not use node -e inline code (logic moved to worker file)' + ); + }); + + test('managed hooks list in worker matches build-hooks HOOKS_TO_COPY JS entries', () => { // Extract JS hooks from build-hooks.js HOOKS_TO_COPY const buildContent = fs.readFileSync(BUILD_HOOKS_PATH, 'utf8'); const hooksArrayMatch = buildContent.match(/HOOKS_TO_COPY\s*=\s*\[([\s\S]*?)\]/); @@ -48,25 +58,18 @@ describe('orphaned hooks stale detection (#1750)', () => { } assert.ok(jsHooks.length >= 5, `expected at least 5 JS hooks in HOOKS_TO_COPY, got ${jsHooks.length}`); - // The check-update hook should define its own managed hooks list - // that matches the JS entries from HOOKS_TO_COPY - const checkContent = fs.readFileSync(CHECK_UPDATE_PATH, 'utf8'); - const childScriptMatch = checkContent.match(/spawn\(process\.execPath,\s*\['-e',\s*`([\s\S]*?)`\]/); - const childScript = childScriptMatch[1]; - - // Verify each JS hook from HOOKS_TO_COPY is referenced in the managed list + // MANAGED_HOOKS in the worker must include each JS hook from HOOKS_TO_COPY + const workerContent = fs.readFileSync(WORKER_PATH, 'utf8'); for (const hook of jsHooks) { assert.ok( - childScript.includes(hook), - `managed hooks in check-update should include '${hook}' from HOOKS_TO_COPY` + workerContent.includes(hook), + `MANAGED_HOOKS in worker should include '${hook}' from HOOKS_TO_COPY` ); } }); - test('orphaned hook filenames would NOT match the managed hooks list', () => { - const checkContent = fs.readFileSync(CHECK_UPDATE_PATH, 'utf8'); - const childScriptMatch = checkContent.match(/spawn\(process\.execPath,\s*\['-e',\s*`([\s\S]*?)`\]/); - const childScript = childScriptMatch[1]; + test('orphaned hook filenames are NOT in the MANAGED_HOOKS list', () => { + const workerContent = fs.readFileSync(WORKER_PATH, 'utf8'); // These are real orphaned hooks from the removed intel feature const orphanedHooks = [ @@ -77,8 +80,8 @@ describe('orphaned hooks stale detection (#1750)', () => { for (const orphan of orphanedHooks) { assert.ok( - !childScript.includes(orphan), - `orphaned hook '${orphan}' must NOT be in the managed hooks list` + !workerContent.includes(orphan), + `orphaned hook '${orphan}' must NOT be in the MANAGED_HOOKS list` ); } });