mirror of
https://github.com/glittercowboy/get-shit-done
synced 2026-04-25 17:25:23 +02:00
fix(installer): guard .sh hook registration with fs.existsSync before writing settings.json (#1823)
Before registering each .sh hook (validate-commit, session-state, phase-boundary), check that the target file was actually copied. If the .sh file is missing (e.g. omitted from the npm package as in v1.32.0), skip registration and emit a warning instead of writing a broken hook entry that errors on every tool invocation. Closes #1817 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -5666,8 +5666,11 @@ function install(isGlobal, runtime = 'claude') {
|
||||
const hasValidateCommitHook = settings.hooks[preToolEvent].some(entry =>
|
||||
entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-validate-commit'))
|
||||
);
|
||||
|
||||
if (!hasValidateCommitHook) {
|
||||
// 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
|
||||
// causes a hook error on every Bash tool invocation.
|
||||
const validateCommitFile = path.join(targetDir, 'hooks', 'gsd-validate-commit.sh');
|
||||
if (!hasValidateCommitHook && fs.existsSync(validateCommitFile)) {
|
||||
settings.hooks[preToolEvent].push({
|
||||
matcher: 'Bash',
|
||||
hooks: [
|
||||
@@ -5679,6 +5682,8 @@ function install(isGlobal, runtime = 'claude') {
|
||||
]
|
||||
});
|
||||
console.log(` ${green}✓${reset} Configured commit validation hook (opt-in via config)`);
|
||||
} else if (!hasValidateCommitHook && !fs.existsSync(validateCommitFile)) {
|
||||
console.warn(` ${yellow}⚠${reset} Skipped commit validation hook — gsd-validate-commit.sh not found at target`);
|
||||
}
|
||||
|
||||
// Configure session state orientation hook (opt-in)
|
||||
@@ -5688,8 +5693,8 @@ function install(isGlobal, runtime = 'claude') {
|
||||
const hasSessionStateHook = settings.hooks.SessionStart.some(entry =>
|
||||
entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-session-state'))
|
||||
);
|
||||
|
||||
if (!hasSessionStateHook) {
|
||||
const sessionStateFile = path.join(targetDir, 'hooks', 'gsd-session-state.sh');
|
||||
if (!hasSessionStateHook && fs.existsSync(sessionStateFile)) {
|
||||
settings.hooks.SessionStart.push({
|
||||
hooks: [
|
||||
{
|
||||
@@ -5699,6 +5704,8 @@ function install(isGlobal, runtime = 'claude') {
|
||||
]
|
||||
});
|
||||
console.log(` ${green}✓${reset} Configured session state orientation hook (opt-in via config)`);
|
||||
} else if (!hasSessionStateHook && !fs.existsSync(sessionStateFile)) {
|
||||
console.warn(` ${yellow}⚠${reset} Skipped session state hook — gsd-session-state.sh not found at target`);
|
||||
}
|
||||
|
||||
// Configure phase boundary detection hook (opt-in)
|
||||
@@ -5708,8 +5715,8 @@ function install(isGlobal, runtime = 'claude') {
|
||||
const hasPhaseBoundaryHook = settings.hooks[postToolEvent].some(entry =>
|
||||
entry.hooks && entry.hooks.some(h => h.command && h.command.includes('gsd-phase-boundary'))
|
||||
);
|
||||
|
||||
if (!hasPhaseBoundaryHook) {
|
||||
const phaseBoundaryFile = path.join(targetDir, 'hooks', 'gsd-phase-boundary.sh');
|
||||
if (!hasPhaseBoundaryHook && fs.existsSync(phaseBoundaryFile)) {
|
||||
settings.hooks[postToolEvent].push({
|
||||
matcher: 'Write|Edit',
|
||||
hooks: [
|
||||
@@ -5721,6 +5728,8 @@ function install(isGlobal, runtime = 'claude') {
|
||||
]
|
||||
});
|
||||
console.log(` ${green}✓${reset} Configured phase boundary detection hook (opt-in via config)`);
|
||||
} else if (!hasPhaseBoundaryHook && !fs.existsSync(phaseBoundaryFile)) {
|
||||
console.warn(` ${yellow}⚠${reset} Skipped phase boundary hook — gsd-phase-boundary.sh not found at target`);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
63
tests/bug-1817-sh-hook-guard.test.cjs
Normal file
63
tests/bug-1817-sh-hook-guard.test.cjs
Normal file
@@ -0,0 +1,63 @@
|
||||
/**
|
||||
* Regression tests for bug #1817
|
||||
*
|
||||
* The installer must NOT register .sh hook entries in settings.json when the
|
||||
* corresponding .sh file does not exist at the target path. The original bug:
|
||||
* v1.32.0's npm package omitted the .sh files from hooks/dist/, so the copy
|
||||
* step produced no files, yet the registration step ran unconditionally —
|
||||
* leaving users with hook errors on every tool invocation.
|
||||
*
|
||||
* Defensive guard: before registering each .sh hook in settings.json,
|
||||
* install.js must verify the target file exists. If it doesn't, skip
|
||||
* registration and emit a warning.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
|
||||
const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
|
||||
const SH_HOOKS = [
|
||||
{ name: 'gsd-validate-commit.sh', settingsVar: 'validateCommitCommand' },
|
||||
{ name: 'gsd-session-state.sh', settingsVar: 'sessionStateCommand' },
|
||||
{ name: 'gsd-phase-boundary.sh', settingsVar: 'phaseBoundaryCommand' },
|
||||
];
|
||||
|
||||
describe('bug #1817: .sh hook registration guards', () => {
|
||||
let src;
|
||||
|
||||
// Read once — all tests in this suite share the same source snapshot.
|
||||
try {
|
||||
src = fs.readFileSync(INSTALL_SRC, 'utf-8');
|
||||
} catch {
|
||||
src = '';
|
||||
}
|
||||
|
||||
for (const { name, settingsVar } of SH_HOOKS) {
|
||||
describe(`${name} registration`, () => {
|
||||
test(`install.js checks file existence before registering ${name}`, () => {
|
||||
// Find the block where this .sh hook is registered.
|
||||
// Each registration block is preceded by the command variable declaration
|
||||
// and followed by the next hook or end of registration section.
|
||||
const varIdx = src.indexOf(settingsVar);
|
||||
assert.ok(varIdx !== -1, `${settingsVar} variable not found in install.js`);
|
||||
|
||||
// Extract ~900 chars around the variable to find the registration block
|
||||
const blockStart = Math.max(0, varIdx - 50);
|
||||
const blockEnd = Math.min(src.length, varIdx + 900);
|
||||
const block = src.slice(blockStart, blockEnd);
|
||||
|
||||
assert.ok(
|
||||
block.includes('fs.existsSync') || block.includes('existsSync'),
|
||||
`install.js must call fs.existsSync on the target path before registering ${name} in settings.json. ` +
|
||||
`Without this guard, hooks are registered even when the .sh file was never copied ` +
|
||||
`(the root cause of #1817).`
|
||||
);
|
||||
});
|
||||
});
|
||||
}
|
||||
});
|
||||
@@ -226,7 +226,7 @@ describe('agent files use inline @-reference wiring at decision points', () => {
|
||||
assert.equal(
|
||||
reqReadingContent.includes(wiring.refFile),
|
||||
false,
|
||||
`${agent}.md puts ${wiring.refFile} inside a <required_reading> block — should use inline @-reference wiring instead`
|
||||
`${agent}.md puts ${wiring.refFile} inside a <required_reading> block — thinking-model references must use inline @-reference wiring at decision points, not <required_reading> blocks`
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user