* 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>
This commit is contained in:
Tom Boucher
@@ -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-<stem>/ 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',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user