mirror of
https://github.com/glittercowboy/get-shit-done
synced 2026-04-25 17:25:23 +02:00
fix(hooks): add read-before-edit guard for non-Claude runtimes (#1645)
* fix(hooks): add read-before-edit guidance for non-Claude runtimes When models that don't natively enforce read-before-edit hit the guard, the error message now includes explicit instruction to Read first. This prevents infinite retry loops that burn through usage. Closes #1628 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(build): register gsd-read-guard.js in HOOKS_TO_COPY and harden tests The hook was missing from scripts/build-hooks.js, so global installs would never receive the hook file in hooks/dist/. Also adds tests for build registration, install uninstall list, and non-string file_path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -4255,7 +4255,7 @@ function uninstall(isGlobal, runtime = 'claude') {
|
||||
// 4. Remove GSD hooks
|
||||
const hooksDir = path.join(targetDir, 'hooks');
|
||||
if (fs.existsSync(hooksDir)) {
|
||||
const gsdHooks = ['gsd-statusline.js', 'gsd-check-update.js', 'gsd-check-update.sh', 'gsd-context-monitor.js', 'gsd-prompt-guard.js', 'gsd-session-state.sh', 'gsd-validate-commit.sh', 'gsd-phase-boundary.sh'];
|
||||
const gsdHooks = ['gsd-statusline.js', 'gsd-check-update.js', 'gsd-check-update.sh', 'gsd-context-monitor.js', 'gsd-prompt-guard.js', 'gsd-read-guard.js', 'gsd-session-state.sh', 'gsd-validate-commit.sh', 'gsd-phase-boundary.sh'];
|
||||
let hookCount = 0;
|
||||
for (const hook of gsdHooks) {
|
||||
const hookPath = path.join(hooksDir, hook);
|
||||
@@ -4357,7 +4357,7 @@ function uninstall(isGlobal, runtime = 'claude') {
|
||||
settings.hooks[eventName] = settings.hooks[eventName].filter(entry => {
|
||||
if (entry.hooks && Array.isArray(entry.hooks)) {
|
||||
const hasGsdHook = entry.hooks.some(h =>
|
||||
h.command && h.command.includes('gsd-prompt-guard')
|
||||
h.command && (h.command.includes('gsd-prompt-guard') || h.command.includes('gsd-read-guard'))
|
||||
);
|
||||
return !hasGsdHook;
|
||||
}
|
||||
@@ -4365,7 +4365,7 @@ function uninstall(isGlobal, runtime = 'claude') {
|
||||
});
|
||||
if (settings.hooks[eventName].length < before) {
|
||||
settingsModified = true;
|
||||
console.log(` ${green}✓${reset} Removed prompt injection guard hook from settings`);
|
||||
console.log(` ${green}✓${reset} Removed GSD pre-tool guard hooks from settings`);
|
||||
}
|
||||
if (settings.hooks[eventName].length === 0) {
|
||||
delete settings.hooks[eventName];
|
||||
@@ -5381,6 +5381,9 @@ function install(isGlobal, runtime = 'claude') {
|
||||
const promptGuardCommand = isGlobal
|
||||
? buildHookCommand(targetDir, 'gsd-prompt-guard.js')
|
||||
: 'node ' + dirName + '/hooks/gsd-prompt-guard.js';
|
||||
const readGuardCommand = isGlobal
|
||||
? buildHookCommand(targetDir, 'gsd-read-guard.js')
|
||||
: 'node ' + dirName + '/hooks/gsd-read-guard.js';
|
||||
|
||||
// Enable experimental agents for Gemini CLI (required for custom sub-agents)
|
||||
if (isGemini) {
|
||||
@@ -5486,6 +5489,27 @@ function install(isGlobal, runtime = 'claude') {
|
||||
console.log(` ${green}✓${reset} Configured prompt injection guard hook`);
|
||||
}
|
||||
|
||||
// Configure PreToolUse hook for read-before-edit guidance (#1628)
|
||||
// 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'))
|
||||
);
|
||||
|
||||
if (!hasReadGuardHook) {
|
||||
settings.hooks[preToolEvent].push({
|
||||
matcher: 'Write|Edit',
|
||||
hooks: [
|
||||
{
|
||||
type: 'command',
|
||||
command: readGuardCommand,
|
||||
timeout: 5
|
||||
}
|
||||
]
|
||||
});
|
||||
console.log(` ${green}✓${reset} Configured read-before-edit guard hook`);
|
||||
}
|
||||
|
||||
// Community hooks — registered on install but opt-in at runtime.
|
||||
// Each hook checks .planning/config.json for hooks.community: true
|
||||
// and exits silently (no-op) if not enabled. This lets users enable
|
||||
|
||||
77
hooks/gsd-read-guard.js
Normal file
77
hooks/gsd-read-guard.js
Normal file
@@ -0,0 +1,77 @@
|
||||
#!/usr/bin/env node
|
||||
// gsd-hook-version: {{GSD_VERSION}}
|
||||
// GSD Read Guard — PreToolUse hook
|
||||
// Injects advisory guidance when Write/Edit targets an existing file,
|
||||
// reminding the model to Read the file first.
|
||||
//
|
||||
// Background: Non-Claude models (e.g. MiniMax M2.5 on OpenCode) don't
|
||||
// natively follow the read-before-edit pattern. When they attempt to
|
||||
// Write/Edit an existing file without reading it, the runtime rejects
|
||||
// with "You must read file before overwriting it." The model retries
|
||||
// without reading, creating an infinite loop that burns through usage.
|
||||
//
|
||||
// This hook prevents that loop by injecting clear guidance BEFORE the
|
||||
// tool call reaches the runtime. The model sees the advisory and can
|
||||
// issue a Read call on the next turn.
|
||||
//
|
||||
// Triggers on: Write and Edit tool calls
|
||||
// Action: Advisory (does not block) — injects read-first guidance
|
||||
// Only fires when the target file already exists on disk.
|
||||
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
|
||||
let input = '';
|
||||
const stdinTimeout = setTimeout(() => process.exit(0), 3000);
|
||||
process.stdin.setEncoding('utf8');
|
||||
process.stdin.on('data', chunk => input += chunk);
|
||||
process.stdin.on('end', () => {
|
||||
clearTimeout(stdinTimeout);
|
||||
try {
|
||||
const data = JSON.parse(input);
|
||||
const toolName = data.tool_name;
|
||||
|
||||
// Only intercept Write and Edit tool calls
|
||||
if (toolName !== 'Write' && toolName !== 'Edit') {
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
const filePath = data.tool_input?.file_path || '';
|
||||
if (!filePath) {
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
// Only inject guidance when the file already exists.
|
||||
// New files don't need a prior Read — the runtime allows creating them directly.
|
||||
let fileExists = false;
|
||||
try {
|
||||
fs.accessSync(filePath, fs.constants.F_OK);
|
||||
fileExists = true;
|
||||
} catch {
|
||||
// File does not exist — no guidance needed
|
||||
}
|
||||
|
||||
if (!fileExists) {
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
const fileName = path.basename(filePath);
|
||||
|
||||
// Advisory guidance — does not block the operation
|
||||
const output = {
|
||||
hookSpecificOutput: {
|
||||
hookEventName: 'PreToolUse',
|
||||
additionalContext:
|
||||
`READ-BEFORE-EDIT REMINDER: You are about to modify "${fileName}" which already exists. ` +
|
||||
'If you have not already used the Read tool to read this file in the current session, ' +
|
||||
'you MUST Read it first before editing. The runtime will reject edits to files that ' +
|
||||
'have not been read. Use the Read tool on this file path, then retry your edit.',
|
||||
},
|
||||
};
|
||||
|
||||
process.stdout.write(JSON.stringify(output));
|
||||
} catch {
|
||||
// Silent fail — never block tool execution
|
||||
process.exit(0);
|
||||
}
|
||||
});
|
||||
@@ -18,6 +18,7 @@ const HOOKS_TO_COPY = [
|
||||
'gsd-check-update.js',
|
||||
'gsd-context-monitor.js',
|
||||
'gsd-prompt-guard.js',
|
||||
'gsd-read-guard.js',
|
||||
'gsd-statusline.js',
|
||||
'gsd-workflow-guard.js'
|
||||
];
|
||||
|
||||
224
tests/read-guard.test.cjs
Normal file
224
tests/read-guard.test.cjs
Normal file
@@ -0,0 +1,224 @@
|
||||
/**
|
||||
* Tests for gsd-read-guard.js PreToolUse hook.
|
||||
*
|
||||
* The read guard intercepts Write/Edit tool calls on existing files and injects
|
||||
* advisory guidance telling the model to Read the file first. This prevents
|
||||
* infinite retry loops when non-Claude models (e.g. MiniMax M2.5 on OpenCode)
|
||||
* attempt to edit files without reading them, hitting the runtime's
|
||||
* "You must read file before overwriting it" error repeatedly.
|
||||
*
|
||||
* The hook is advisory-only (does not block) so Claude Code behavior is unaffected.
|
||||
*/
|
||||
|
||||
process.env.GSD_TEST_MODE = '1';
|
||||
|
||||
const { test, describe, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { execFileSync } = require('node:child_process');
|
||||
|
||||
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-read-guard.js');
|
||||
|
||||
/**
|
||||
* Run the read guard hook with a given tool input payload.
|
||||
* Returns { exitCode, stdout, stderr }.
|
||||
*/
|
||||
function runHook(payload) {
|
||||
const input = JSON.stringify(payload);
|
||||
try {
|
||||
const stdout = execFileSync(process.execPath, [HOOK_PATH], {
|
||||
input,
|
||||
encoding: 'utf-8',
|
||||
timeout: 5000,
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
});
|
||||
return { exitCode: 0, stdout: stdout.trim(), stderr: '' };
|
||||
} catch (err) {
|
||||
return {
|
||||
exitCode: err.status ?? 1,
|
||||
stdout: (err.stdout || '').toString().trim(),
|
||||
stderr: (err.stderr || '').toString().trim(),
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
describe('gsd-read-guard hook', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempDir('gsd-read-guard-');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
// ─── Core: advisory on Write to existing file ───────────────────────────
|
||||
|
||||
test('injects read-first guidance when Write targets an existing file', () => {
|
||||
const filePath = path.join(tmpDir, 'existing.js');
|
||||
fs.writeFileSync(filePath, 'console.log("hello");\n');
|
||||
|
||||
const result = runHook({
|
||||
tool_name: 'Write',
|
||||
tool_input: { file_path: filePath, content: 'console.log("world");\n' },
|
||||
});
|
||||
|
||||
assert.equal(result.exitCode, 0);
|
||||
assert.ok(result.stdout.length > 0, 'should produce output');
|
||||
|
||||
const output = JSON.parse(result.stdout);
|
||||
assert.ok(output.hookSpecificOutput, 'should have hookSpecificOutput');
|
||||
assert.ok(output.hookSpecificOutput.additionalContext, 'should have additionalContext');
|
||||
assert.ok(
|
||||
output.hookSpecificOutput.additionalContext.includes('Read'),
|
||||
'guidance should mention Read tool'
|
||||
);
|
||||
});
|
||||
|
||||
test('injects read-first guidance when Edit targets an existing file', () => {
|
||||
const filePath = path.join(tmpDir, 'existing.js');
|
||||
fs.writeFileSync(filePath, 'const x = 1;\n');
|
||||
|
||||
const result = runHook({
|
||||
tool_name: 'Edit',
|
||||
tool_input: { file_path: filePath, old_string: 'const x = 1;', new_string: 'const x = 2;' },
|
||||
});
|
||||
|
||||
assert.equal(result.exitCode, 0);
|
||||
assert.ok(result.stdout.length > 0, 'should produce output');
|
||||
|
||||
const output = JSON.parse(result.stdout);
|
||||
assert.ok(output.hookSpecificOutput.additionalContext.includes('Read'));
|
||||
});
|
||||
|
||||
// ─── No-op cases: should NOT inject guidance ────────────────────────────
|
||||
|
||||
test('does nothing for Write to a new file (file does not exist)', () => {
|
||||
const filePath = path.join(tmpDir, 'brand-new.js');
|
||||
// File does NOT exist
|
||||
|
||||
const result = runHook({
|
||||
tool_name: 'Write',
|
||||
tool_input: { file_path: filePath, content: 'new content' },
|
||||
});
|
||||
|
||||
assert.equal(result.exitCode, 0);
|
||||
assert.equal(result.stdout, '', 'should produce no output for new files');
|
||||
});
|
||||
|
||||
test('does nothing for non-Write/Edit tools', () => {
|
||||
const result = runHook({
|
||||
tool_name: 'Bash',
|
||||
tool_input: { command: 'echo hello' },
|
||||
});
|
||||
|
||||
assert.equal(result.exitCode, 0);
|
||||
assert.equal(result.stdout, '');
|
||||
});
|
||||
|
||||
test('does nothing for Read tool', () => {
|
||||
const filePath = path.join(tmpDir, 'existing.js');
|
||||
fs.writeFileSync(filePath, 'content');
|
||||
|
||||
const result = runHook({
|
||||
tool_name: 'Read',
|
||||
tool_input: { file_path: filePath },
|
||||
});
|
||||
|
||||
assert.equal(result.exitCode, 0);
|
||||
assert.equal(result.stdout, '');
|
||||
});
|
||||
|
||||
// ─── Error resilience ──────────────────────────────────────────────────
|
||||
|
||||
test('exits cleanly on invalid JSON input', () => {
|
||||
try {
|
||||
const stdout = execFileSync(process.execPath, [HOOK_PATH], {
|
||||
input: 'not json',
|
||||
encoding: 'utf-8',
|
||||
timeout: 5000,
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
});
|
||||
// Should exit 0 silently
|
||||
assert.equal(stdout.trim(), '');
|
||||
} catch (err) {
|
||||
assert.equal(err.status, 0, 'should exit 0 on parse error');
|
||||
}
|
||||
});
|
||||
|
||||
test('exits cleanly when tool_input is missing', () => {
|
||||
const result = runHook({ tool_name: 'Write' });
|
||||
assert.equal(result.exitCode, 0);
|
||||
assert.equal(result.stdout, '');
|
||||
});
|
||||
|
||||
// ─── Guidance content quality ──────────────────────────────────────────
|
||||
|
||||
test('guidance message includes the filename', () => {
|
||||
const filePath = path.join(tmpDir, 'myfile.ts');
|
||||
fs.writeFileSync(filePath, 'export const foo = 1;\n');
|
||||
|
||||
const result = runHook({
|
||||
tool_name: 'Write',
|
||||
tool_input: { file_path: filePath, content: 'export const foo = 2;\n' },
|
||||
});
|
||||
|
||||
const output = JSON.parse(result.stdout);
|
||||
assert.ok(
|
||||
output.hookSpecificOutput.additionalContext.includes('myfile.ts'),
|
||||
'guidance should include the filename being edited'
|
||||
);
|
||||
});
|
||||
|
||||
test('guidance message instructs to use Read tool before editing', () => {
|
||||
const filePath = path.join(tmpDir, 'target.py');
|
||||
fs.writeFileSync(filePath, 'x = 1\n');
|
||||
|
||||
const result = runHook({
|
||||
tool_name: 'Edit',
|
||||
tool_input: { file_path: filePath, old_string: 'x = 1', new_string: 'x = 2' },
|
||||
});
|
||||
|
||||
const output = JSON.parse(result.stdout);
|
||||
const ctx = output.hookSpecificOutput.additionalContext;
|
||||
assert.ok(ctx.includes('Read'), 'must mention Read tool');
|
||||
assert.ok(
|
||||
ctx.includes('before') || ctx.includes('first'),
|
||||
'must indicate Read should come before the edit'
|
||||
);
|
||||
});
|
||||
|
||||
// ─── Build / install integration ───────────────────────────────────────
|
||||
|
||||
test('hook is registered in build-hooks.js HOOKS_TO_COPY', () => {
|
||||
const buildHooksPath = path.join(__dirname, '..', 'scripts', 'build-hooks.js');
|
||||
const content = fs.readFileSync(buildHooksPath, 'utf8');
|
||||
assert.ok(
|
||||
content.includes('gsd-read-guard.js'),
|
||||
'gsd-read-guard.js must be in HOOKS_TO_COPY so it ships in hooks/dist/'
|
||||
);
|
||||
});
|
||||
|
||||
test('hook is registered in install.js uninstall hook list', () => {
|
||||
const installPath = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
const content = fs.readFileSync(installPath, 'utf8');
|
||||
assert.ok(
|
||||
content.includes("'gsd-read-guard.js'"),
|
||||
'gsd-read-guard.js must be in the uninstall gsdHooks list'
|
||||
);
|
||||
});
|
||||
|
||||
test('exits cleanly when tool_input.file_path is non-string', () => {
|
||||
const result = runHook({
|
||||
tool_name: 'Write',
|
||||
tool_input: { file_path: 12345, content: 'data' },
|
||||
});
|
||||
// file_path is a number — || '' yields '' — hook exits silently
|
||||
assert.equal(result.exitCode, 0);
|
||||
assert.equal(result.stdout, '');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user