From 898d55788e8b3141e07f927cd5f1b7434caa3cf2 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 10 Jun 2026 10:44:24 -0400 Subject: [PATCH] fix(#976): detect command+args (wrapped) hook registrations in installer presence checks (#994) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#976): detect command+args (wrapped) hook registrations in installer presence checks Add referencesHook() helper that inspects both h.command (standard form) and h.args[] (args-form / wrapped-launcher form) when checking whether a managed hook is already registered. Rewrite all has*Hook predicates and the alreadyHas* guards to use it so args-form registrations suppress the duplicate stock string-command entry that was previously appended on every install/update. Also add an explicit args-form skip to rewriteLegacyManagedNodeHookCommands so entries with a non-empty args[] are left untouched (they are intentional user wrappers, not legacy bare-node commands to migrate). Extend isManagedHookCommand() in shell-command-projection.cts with an optional args: unknown[] parameter that checks whether any arg's basename matches the managed hook surface set — backward compatible; existing callers are unaffected. Regression test added to tests/install-regressions.test.cjs: - two-pass install with an args-form SessionStart entry pre-written to settings.local.json asserts exactly 1 hook entry remains after reinstall (previously 2 — the original args-form + a new stock string-command duplicate) - rewriteLegacyManagedNodeHookCommands test asserts args-form entries unchanged Closes #976 * chore(#976): backfill changeset pr number (994) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .changeset/fancy-book-path.md | 5 + bin/install.js | 50 ++++++--- src/shell-command-projection.cts | 15 ++- tests/install-regressions.test.cjs | 164 ++++++++++++++++++++++++++++- 4 files changed, 214 insertions(+), 20 deletions(-) create mode 100644 .changeset/fancy-book-path.md diff --git a/.changeset/fancy-book-path.md b/.changeset/fancy-book-path.md new file mode 100644 index 000000000..6d047967f --- /dev/null +++ b/.changeset/fancy-book-path.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 994 +--- +**The installer no longer re-adds a duplicate managed hook when the user registered it in `command`+`args` (wrapped) form** — the presence checks only inspected `h.command`, so an args-form wrapper (a common Windows windowless-launcher mitigation) was invisible and a stock entry was appended on every install/update, running the hook twice. (#976) diff --git a/bin/install.js b/bin/install.js index dc14fc002..aaff9d53d 100755 --- a/bin/install.js +++ b/bin/install.js @@ -725,6 +725,10 @@ function rewriteLegacyManagedNodeHookCommands(settings, absoluteRunner, opts) { if (!entry || !Array.isArray(entry.hooks)) continue; for (const h of entry.hooks) { if (!h || typeof h.command !== 'string') continue; + // args-form entries have the script path in h.args[] and h.command is + // the launcher executable (not a managed hook command). These are + // intentional user wrappers — do not rewrite them. (#976) + if (Array.isArray(h.args) && h.args.length > 0) continue; let trimmed = h.command.trim(); const hadPowerShellCallOperator = platform === 'win32' && /^&\s+/.test(trimmed); if (hadPowerShellCallOperator) { @@ -11845,6 +11849,18 @@ function install(isGlobal, runtime = 'claude', options = {}) { } } + // Helper: detect whether a hook entry references a managed hook by name. + // Checks both the plain command string (standard form) and the args array + // (command+args / wrapped-launcher form used by windowless launchers on + // Windows and some custom PATH-less environments). Without this check the + // presence guards below only inspect h.command, so an args-form wrapper is + // invisible and a stock string-command entry is appended on every + // install/update, running the hook twice. (#976) + function referencesHook(h, hookName) { + return (typeof h.command === 'string' && h.command.includes(hookName)) || + (Array.isArray(h.args) && h.args.some(a => typeof a === 'string' && a.includes(hookName))); + } + // Configure SessionStart hook for update checking (skip for opencode) if (!isOpencode && !isKilo) { if (!settings.hooks) { @@ -11855,7 +11871,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { } const hasGsdUpdateHook = settings.hooks.SessionStart.some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-check-update')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-check-update')) ); // Guard: only register if the hook file was actually installed (#1754). @@ -11883,7 +11899,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { } const hasContextMonitorHook = settings.hooks[postToolEvent].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-context-monitor')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-context-monitor')) ); const contextMonitorFile = path.join(targetDir, 'hooks', 'gsd-context-monitor.js'); @@ -11904,14 +11920,14 @@ function install(isGlobal, runtime = 'claude', options = {}) { } else { // Migrate existing context monitor hooks: add matcher and timeout if missing for (const entry of settings.hooks[postToolEvent]) { - if (entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-context-monitor'))) { + if (entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-context-monitor'))) { let migrated = false; if (!entry.matcher) { entry.matcher = 'Bash|Edit|Write|MultiEdit|Agent|Task'; migrated = true; } for (const h of entry.hooks) { - if (h.command && h.command.includes('gsd-context-monitor') && !h.timeout) { + if (referencesHook(h, 'gsd-context-monitor') && !h.timeout) { h.timeout = 10; migrated = true; } @@ -11931,7 +11947,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { } const hasPromptGuardHook = settings.hooks[preToolEvent].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-prompt-guard')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-prompt-guard')) ); const promptGuardFile = path.join(targetDir, 'hooks', 'gsd-prompt-guard.js'); @@ -11955,7 +11971,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { // Prevents infinite retry loops when non-Claude models attempt to edit // files without reading them first. Advisory-only — does not block. const hasReadGuardHook = settings.hooks[preToolEvent].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-read-guard')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-read-guard')) ); const readGuardFile = path.join(targetDir, 'hooks', 'gsd-read-guard.js'); @@ -11979,7 +11995,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { // Scans content returned by the Read tool for injection patterns, including // summarisation-specific patterns that survive context compression. const hasReadInjectionScannerHook = settings.hooks[postToolEvent].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-read-injection-scanner')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-read-injection-scanner')) ); const readInjectionScannerFile = path.join(targetDir, 'hooks', 'gsd-read-injection-scanner.js'); @@ -12013,7 +12029,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { : localCmd('gsd-workflow-guard.js'); const workflowGuardMatcher = 'Bash|Edit|Write|MultiEdit'; const workflowGuardHookEntry = settings.hooks[preToolEvent].find(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-workflow-guard')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-workflow-guard')) ); const hasWorkflowGuardHook = Boolean(workflowGuardHookEntry); @@ -12045,7 +12061,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { ? buildHookCommand(targetDir, 'gsd-worktree-path-guard.js', hookOpts) : localCmd('gsd-worktree-path-guard.js'); const hasWorktreePathGuardHook = settings.hooks[preToolEvent].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-worktree-path-guard')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-worktree-path-guard')) ); const worktreePathGuardFile = path.join(targetDir, 'hooks', 'gsd-worktree-path-guard.js'); if (!hasWorktreePathGuardHook && fs.existsSync(worktreePathGuardFile) && worktreePathGuardCommand) { @@ -12069,7 +12085,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { ? buildHookCommand(targetDir, 'gsd-validate-commit.sh', hookOpts) : localShellCmd('gsd-validate-commit.sh'); const hasValidateCommitHook = settings.hooks[preToolEvent].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-validate-commit')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-validate-commit')) ); // Guard: only register if the .sh file was actually installed. If the npm package // omitted the file (as happened in v1.32.0, bug #1817), registering a missing hook @@ -12101,7 +12117,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { ? buildHookCommand(targetDir, 'gsd-graphify-update.sh', hookOpts) : localShellCmd('gsd-graphify-update.sh'); const hasGraphifyUpdateHook = settings.hooks[postToolEvent].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-graphify-update')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-graphify-update')) ); const graphifyUpdateFile = path.join(targetDir, 'hooks', 'gsd-graphify-update.sh'); if (!hasGraphifyUpdateHook && fs.existsSync(graphifyUpdateFile) && graphifyUpdateCommand) { @@ -12127,7 +12143,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { ? buildHookCommand(targetDir, 'gsd-session-state.sh', hookOpts) : localShellCmd('gsd-session-state.sh'); const hasSessionStateHook = settings.hooks.SessionStart.some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-session-state')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-session-state')) ); const sessionStateFile = path.join(targetDir, 'hooks', 'gsd-session-state.sh'); if (!hasSessionStateHook && fs.existsSync(sessionStateFile) && sessionStateCommand) { @@ -12151,7 +12167,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { ? buildHookCommand(targetDir, 'gsd-phase-boundary.sh', hookOpts) : localShellCmd('gsd-phase-boundary.sh'); const hasPhaseBoundaryHook = settings.hooks[postToolEvent].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-phase-boundary')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-phase-boundary')) ); const phaseBoundaryFile = path.join(targetDir, 'hooks', 'gsd-phase-boundary.sh'); if (!hasPhaseBoundaryHook && fs.existsSync(phaseBoundaryFile) && phaseBoundaryCommand) { @@ -12195,7 +12211,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { settings.hooks[event] = []; } const alreadyHasContextMonitor = settings.hooks[event].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-context-monitor')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-context-monitor')) ); if (!alreadyHasContextMonitor && fs.existsSync(contextMonitorFile) && contextMonitorCommand) { settings.hooks[event].push({ @@ -12244,7 +12260,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { settings.hooks[geminiEvent] = []; } const alreadyHasContextMonitor = settings.hooks[geminiEvent].some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-context-monitor')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-context-monitor')) ); if (!alreadyHasContextMonitor && fs.existsSync(contextMonitorFile) && contextMonitorCommand) { settings.hooks[geminiEvent].push({ @@ -12281,7 +12297,7 @@ function install(isGlobal, runtime = 'claude', options = {}) { } const configReloadFile = path.join(targetDir, 'hooks', 'gsd-config-reload.js'); const alreadyHasConfigReload = settings.hooks.FileChanged.some(entry => - entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-config-reload')) + entry.hooks && entry.hooks.some(h => referencesHook(h, 'gsd-config-reload')) ); if (!alreadyHasConfigReload && fs.existsSync(configReloadFile) && configReloadCommand) { settings.hooks.FileChanged.push({ @@ -12452,7 +12468,7 @@ function finishInstall(settingsPath, settings, statuslineCommand, shouldInstallS if (!settings.hooks) settings.hooks = {}; if (!settings.hooks.SessionStart) settings.hooks.SessionStart = []; const alreadyRegistered = settings.hooks.SessionStart.some(entry => - entry && entry.hooks && entry.hooks.some(h => h && h.command && h.command.includes('gsd-update-banner')) + entry && entry.hooks && entry.hooks.some(h => h && referencesHook(h, 'gsd-update-banner')) ); const bannerHookFile = configDir ? path.join(configDir, 'hooks', 'gsd-update-banner.js') : null; const bannerInstalled = bannerHookFile ? fs.existsSync(bannerHookFile) : false; diff --git a/src/shell-command-projection.cts b/src/shell-command-projection.cts index 4de9d9639..2996443be 100644 --- a/src/shell-command-projection.cts +++ b/src/shell-command-projection.cts @@ -219,12 +219,25 @@ function managedHookCommandSurfaceSet(surface: string = 'settings-json', include return new Set([...base, ...aliases]); } -export function isManagedHookCommand(commandText: unknown, opts: { surface?: string; includeLegacyAliases?: boolean; configDir?: string } = {}): boolean { +export function isManagedHookCommand(commandText: unknown, opts: { surface?: string; includeLegacyAliases?: boolean; configDir?: string; args?: unknown[] } = {}): boolean { if (typeof commandText !== 'string') return false; const surface = opts.surface || 'settings-json'; const includeLegacyAliases = opts.includeLegacyAliases === true; const managedBasenames = managedHookCommandSurfaceSet(surface, includeLegacyAliases); if (!managedBasenames || managedBasenames.size === 0) return false; + + // args-form check: the managed hook filename may appear in args[] rather than + // in command when a windowless launcher wraps the Node invocation. (#976) + // Only treat as managed when an arg basename matches the managed hook set — + // prevents false-positives for non-GSD entries that happen to share a path segment. + if (Array.isArray(opts.args) && opts.args.length > 0) { + for (const arg of opts.args) { + if (typeof arg !== 'string') continue; + const argBasename = arg.replace(/\\/g, '/').split('/').pop() || ''; + if (isManagedHookBasename(argBasename, { surface })) return true; + } + } + const normalizedCommand = commandText.replace(/\\/g, '/'); if (typeof opts.configDir === 'string' && opts.configDir.length > 0) { diff --git a/tests/install-regressions.test.cjs b/tests/install-regressions.test.cjs index 2600301dc..ec21ab191 100644 --- a/tests/install-regressions.test.cjs +++ b/tests/install-regressions.test.cjs @@ -14,7 +14,7 @@ * Closes #3758 */ -const { test, describe } = require('node:test'); +const { test, describe, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); @@ -37,13 +37,34 @@ try { else process.env.GSD_TEST_MODE = savedTestMode; } -const { installRuntimeArtifacts, uninstallRuntimeArtifacts, mergeClaudePermissions, GSD_CLAUDE_ALLOW_PERMISSIONS, GSD_CLAUDE_DENY_PERMISSIONS } = installExports || {}; +const { install, installRuntimeArtifacts, uninstallRuntimeArtifacts, mergeClaudePermissions, GSD_CLAUDE_ALLOW_PERMISSIONS, GSD_CLAUDE_DENY_PERMISSIONS, rewriteLegacyManagedNodeHookCommands, resolveNodeRunner } = installExports || {}; const INSTALL_SCRIPT = path.join(__dirname, '..', 'bin', 'install.js'); +const HOOKS_SRC = path.join(__dirname, '..', 'hooks'); const REAL_COMMANDS_DIR = path.join(__dirname, '..', 'commands', 'gsd'); const MANIFEST = loadSkillsManifest(REAL_COMMANDS_DIR); const RESOLVED_CORE = resolveProfile({ modes: ['core'], manifest: MANIFEST }); +/** + * Stub managed GSD hook files into targetDir/hooks/ so that + * fs.existsSync guards in the installer pass during tests where + * hooks/dist/ is not built. + */ +function stubHooksIntoDir(targetDir, hookNames) { + const hooksDest = path.join(targetDir, 'hooks'); + fs.mkdirSync(hooksDest, { recursive: true }); + for (const hookFile of hookNames) { + const src = path.join(HOOKS_SRC, hookFile); + const dest = path.join(hooksDest, hookFile); + if (fs.existsSync(src)) { + fs.copyFileSync(src, dest); + } else { + fs.writeFileSync(dest, '#!/usr/bin/env node\n// stub\n'); + } + try { fs.chmodSync(dest, 0o755); } catch { /* Windows */ } + } +} + // ─── Defect #1 — Hermes upgrade: bare-stem dirs from #3664 era become stale ── // // #947 REVERSES #3664: the canonical layout is now skills/gsd/gsd-/ again. @@ -610,3 +631,142 @@ describe('mergeClaudePermissions (#768): end-to-end install writes permissions t 'user WebSearch deny entry must survive uninstall'); }); }); + +// ─── #976 — args-form hook presence detection ───────────────────────────────── +// +// Claude Code hooks support a command+args form (executable in `command`, +// script path in `args[]`) used by windowless-launcher wrappers on Windows. +// Pre-fix, hasGsdUpdateHook (and sibling checks) only inspected h.command, +// so an args-form entry was invisible and a stock string-command entry was +// appended on every install/update, running the hook twice. + +describe('#976 regression: installer does not duplicate managed hooks when registered in command+args form', () => { + let tmpDir; + let previousCwd; + + beforeEach(() => { + tmpDir = createTempDir('gsd-976-args-form-'); + previousCwd = process.cwd(); + process.chdir(tmpDir); + + assert.strictEqual(typeof install, 'function', + 'install must be exported from bin/install.js'); + }); + + afterEach(() => { + process.chdir(previousCwd); + cleanup(tmpDir); + }); + + test('does not add a second SessionStart entry when gsd-check-update is already in args-form', () => { + const targetDir = path.join(tmpDir, '.claude'); + fs.mkdirSync(targetDir, { recursive: true }); + + // Pass 1: run install with no pre-existing settings to create the + // gsd-file-manifest.json that the installer migration uses to decide + // whether a hook file is managed (kept) or foreign (removed). + // Without a manifest, the installer migration removes any hook stubs we + // place in hooks/ as "unrecognized GSD-looking files", which would make + // fs.existsSync(checkUpdateFile) return false and skip duplicate-adding. + install(false, 'claude'); + + // Now stub the hook files so fs.existsSync guards pass on pass 2. + // At this point the manifest exists, so migration classifies the stubs as + // manifest-managed and leaves them alone. + stubHooksIntoDir(targetDir, ['gsd-check-update.js']); + + // Local Claude installs read/write settings.local.json (not settings.json). + // Overwrite settings.local.json with the hook in command+args form + // (wrapped launcher). The GSD hook filename appears in args[], not in command. + const launcherCommand = '/usr/local/bin/node-launcher'; + const hookPath = path.join(targetDir, 'hooks', 'gsd-check-update.js'); + const preExistingSettings = { + hooks: { + SessionStart: [ + { + hooks: [ + { + type: 'command', + command: launcherCommand, + args: [hookPath], + }, + ], + }, + ], + }, + }; + fs.writeFileSync( + path.join(targetDir, 'settings.local.json'), + JSON.stringify(preExistingSettings, null, 2) + '\n', + ); + + // Pass 2: run install again — the pre-existing args-form entry must + // suppress the duplicate stock string-command registration. + const result = install(false, 'claude'); + const settings = result && result.settings; + + assert.ok(settings && settings.hooks && Array.isArray(settings.hooks.SessionStart), + 'settings.hooks.SessionStart must be an array after install'); + + // Count all hook entries (at any nesting level) that reference gsd-check-update. + const allEntries = settings.hooks.SessionStart.flatMap(entry => + Array.isArray(entry && entry.hooks) ? entry.hooks : [] + ); + const matching = allEntries.filter(h => + (typeof h.command === 'string' && h.command.includes('gsd-check-update')) || + (Array.isArray(h.args) && h.args.some(a => typeof a === 'string' && a.includes('gsd-check-update'))) + ); + + assert.strictEqual( + matching.length, + 1, + [ + 'Expected exactly 1 hook entry referencing gsd-check-update after install,', + `got ${matching.length}.`, + 'The installer added a duplicate because it could not detect the args-form registration.', + `All matching entries: ${JSON.stringify(matching)}`, + ].join(' '), + ); + }); + + test('rewriteLegacyManagedNodeHookCommands leaves args-form launcher entries unchanged', () => { + assert.strictEqual(typeof rewriteLegacyManagedNodeHookCommands, 'function', + 'rewriteLegacyManagedNodeHookCommands must be exported from bin/install.js'); + + const launcherCommand = '/usr/local/bin/node-launcher'; + const hookPath = '/Users/user/.claude/hooks/gsd-check-update.js'; + const settings = { + hooks: { + SessionStart: [ + { + hooks: [ + { + type: 'command', + command: launcherCommand, + args: [hookPath], + }, + ], + }, + ], + }, + }; + + const runner = resolveNodeRunner() || '/usr/local/bin/node'; + const changed = rewriteLegacyManagedNodeHookCommands(settings, runner, { platform: process.platform }); + + // The args-form launcher entry must NOT be rewritten — it is an intentional + // user wrapper and the script path lives in args[], not command. + assert.strictEqual(changed, false, + 'rewriteLegacyManagedNodeHookCommands must not rewrite args-form entries (#976)'); + assert.strictEqual( + settings.hooks.SessionStart[0].hooks[0].command, + launcherCommand, + 'args-form command must remain unchanged after rewrite pass', + ); + assert.deepStrictEqual( + settings.hooks.SessionStart[0].hooks[0].args, + [hookPath], + 'args-form args must remain unchanged after rewrite pass', + ); + }); +});