mirror of https://github.com/garrytan/gstack.git
fix(hooks): shared spawn-bin helper — all three AskUserQuestion hooks were inert on Windows
The plan-tune hooks resolved bin scripts via new URL(import.meta.url).pathname (which doubles the drive letter on Windows: /C:/C:/...) and spawnSync'd extensionless bash scripts directly (unrunnable without a shell association) — so question logging, preferences, and the error fallback all silently no-op'd on Windows, and /plan-tune collected no data. A single spawn-bin.ts helper now owns bin resolution (fileURLToPath) and win32 bash routing for every hook, with static tripwires so a future hook can't reintroduce the raw pattern. This is the one Windows-spawn idiom for hook code. Fixes #2356. Contributed by @rafassousa (PR #2504; supersedes PR #2399 by @chuchu2781). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
99e2718943
commit
d7ce124092
|
|
@ -31,7 +31,7 @@
|
|||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import * as os from 'os';
|
||||
import { spawnSync } from 'child_process';
|
||||
import { runBin } from './spawn-bin';
|
||||
|
||||
interface HookStdin {
|
||||
tool_name?: string;
|
||||
|
|
@ -126,9 +126,7 @@ export function isErrorResponse(response: unknown): boolean {
|
|||
* echoes). Falls back to 'interactive' (degrade-safe) on any failure. */
|
||||
export function sessionKind(cwd?: string): 'spawned' | 'headless' | 'interactive' {
|
||||
try {
|
||||
const here = path.dirname(new URL(import.meta.url).pathname);
|
||||
const bin = path.resolve(here, '..', '..', '..', 'bin', 'gstack-session-kind');
|
||||
const res = spawnSync(bin, [], {
|
||||
const res = runBin('gstack-session-kind', [], {
|
||||
encoding: 'utf-8',
|
||||
timeout: 3000,
|
||||
cwd: cwd && fs.existsSync(cwd) ? cwd : undefined,
|
||||
|
|
|
|||
|
|
@ -36,7 +36,7 @@ import * as crypto from 'crypto';
|
|||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import * as os from 'os';
|
||||
import { spawnSync } from 'child_process';
|
||||
import { runBin } from './spawn-bin';
|
||||
|
||||
interface HookStdin {
|
||||
session_id?: string;
|
||||
|
|
@ -250,12 +250,7 @@ function detectSkill(cwd: string | undefined): string {
|
|||
}
|
||||
|
||||
function spawnLog(payload: Record<string, unknown>, cwd?: string): void {
|
||||
// Locate the bin relative to this script's directory.
|
||||
const here = path.dirname(new URL(import.meta.url).pathname);
|
||||
// hosts/claude/hooks/ -> ../../../bin/
|
||||
const repoRoot = path.resolve(here, '..', '..', '..');
|
||||
const bin = path.join(repoRoot, 'bin', 'gstack-question-log');
|
||||
const res = spawnSync(bin, [JSON.stringify(payload)], {
|
||||
const res = runBin('gstack-question-log', [JSON.stringify(payload)], {
|
||||
encoding: 'utf-8',
|
||||
stdio: ['ignore', 'pipe', 'pipe'],
|
||||
timeout: 3000,
|
||||
|
|
|
|||
|
|
@ -43,7 +43,7 @@
|
|||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import * as os from 'os';
|
||||
import { spawnSync } from 'child_process';
|
||||
import { runBin, repoRoot } from './spawn-bin';
|
||||
import { isConductor } from '../../../lib/is-conductor';
|
||||
import { classifyQuestion } from '../../../scripts/one-way-doors';
|
||||
|
||||
|
|
@ -240,9 +240,7 @@ function loadRegistry(): Record<string, RegistryEntry> {
|
|||
registryCache = {};
|
||||
try {
|
||||
// Hook lives at hosts/claude/hooks/; registry at scripts/question-registry.ts
|
||||
const here = path.dirname(new URL(import.meta.url).pathname);
|
||||
const repoRoot = path.resolve(here, '..', '..', '..');
|
||||
const regPath = path.join(repoRoot, 'scripts', 'question-registry.ts');
|
||||
const regPath = path.join(repoRoot(), 'scripts', 'question-registry.ts');
|
||||
if (!fs.existsSync(regPath)) return registryCache;
|
||||
const src = fs.readFileSync(regPath, 'utf-8');
|
||||
// Cheap regex extraction so the hook doesn't need to import the TS file
|
||||
|
|
@ -334,9 +332,6 @@ function logAutoDecided(
|
|||
cwd: string | undefined,
|
||||
): void {
|
||||
try {
|
||||
const here = path.dirname(new URL(import.meta.url).pathname);
|
||||
const repoRoot = path.resolve(here, '..', '..', '..');
|
||||
const bin = path.join(repoRoot, 'bin', 'gstack-question-log');
|
||||
const payload: Record<string, unknown> = {
|
||||
skill: 'unknown',
|
||||
question_id: questionId,
|
||||
|
|
@ -348,7 +343,7 @@ function logAutoDecided(
|
|||
session_id: sessionId?.slice(0, 64),
|
||||
tool_use_id: toolUseId?.slice(0, 128),
|
||||
};
|
||||
spawnSync(bin, [JSON.stringify(payload)], {
|
||||
runBin('gstack-question-log', [JSON.stringify(payload)], {
|
||||
encoding: 'utf-8',
|
||||
stdio: ['ignore', 'pipe', 'pipe'],
|
||||
timeout: 3000,
|
||||
|
|
|
|||
|
|
@ -0,0 +1,43 @@
|
|||
/**
|
||||
* Windows-safe resolution + spawn for gstack's bash bins. Two Windows-only
|
||||
* bugs made every hook subprocess a silent no-op; both are fixed here so all
|
||||
* call sites are covered at once.
|
||||
*
|
||||
* 1. `new URL(import.meta.url).pathname` yields `/C:/Users/...`; path.resolve
|
||||
* then rebases it onto the drive root as `C:\C:\Users\...`. fileURLToPath
|
||||
* is the correct conversion. (ENOENT before the bin ever ran.)
|
||||
* 2. `bin/gstack-*` are extensionless bash scripts. Windows has no shebang
|
||||
* support, so they must be handed to bash explicitly.
|
||||
*/
|
||||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import { fileURLToPath } from 'url';
|
||||
import { spawnSync, type SpawnSyncOptions } from 'child_process';
|
||||
|
||||
// Forward slashes on purpose: Bun's spawnSync on Windows returns ENOENT for a
|
||||
// backslash exe path containing spaces.
|
||||
const GIT_BASH = 'C:/Program Files/Git/bin/bash.exe';
|
||||
|
||||
/** bash Windows itself can execute — env override, Git Bash, then PATH. */
|
||||
function bashExe(): string {
|
||||
return process.env.GSTACK_BASH || (fs.existsSync(GIT_BASH) ? GIT_BASH : 'bash');
|
||||
}
|
||||
|
||||
/** gstack install root. This file lives at hosts/claude/hooks/. */
|
||||
export function repoRoot(): string {
|
||||
const here = path.dirname(fileURLToPath(import.meta.url));
|
||||
return path.resolve(here, '..', '..', '..');
|
||||
}
|
||||
|
||||
/** Absolute path to a `bin/` script. */
|
||||
export function binPath(name: string): string {
|
||||
return path.join(repoRoot(), 'bin', name);
|
||||
}
|
||||
|
||||
/** Resolve `name` under bin/ and run it, via bash on Windows. */
|
||||
export function runBin(name: string, args: string[], opts: SpawnSyncOptions) {
|
||||
const bin = binPath(name);
|
||||
return process.platform === 'win32'
|
||||
? spawnSync(bashExe(), [bin, ...args], opts)
|
||||
: spawnSync(bin, args, opts);
|
||||
}
|
||||
|
|
@ -0,0 +1,132 @@
|
|||
import { describe, test, expect } from 'bun:test';
|
||||
import { spawnSync } from 'child_process';
|
||||
import * as path from 'path';
|
||||
import * as fs from 'fs';
|
||||
import * as os from 'os';
|
||||
|
||||
const ROOT = path.resolve(import.meta.dir, '..');
|
||||
const HOOK_DIR = path.join(ROOT, 'hosts', 'claude', 'hooks');
|
||||
const HELPER = path.join(HOOK_DIR, 'spawn-bin.ts');
|
||||
|
||||
/** Every hook entrypoint, excluding the helper itself. */
|
||||
function hookFiles(): string[] {
|
||||
return fs
|
||||
.readdirSync(HOOK_DIR)
|
||||
.filter((f) => f.endsWith('.ts') && f !== 'spawn-bin.ts')
|
||||
.map((f) => path.join(HOOK_DIR, f));
|
||||
}
|
||||
|
||||
describe('claude hooks: Windows path + bin-spawn invariants', () => {
|
||||
test('spawn-bin helper exists and exports the resolution surface', () => {
|
||||
expect(fs.existsSync(HELPER)).toBe(true);
|
||||
const src = fs.readFileSync(HELPER, 'utf-8');
|
||||
expect(src).toContain('export function repoRoot');
|
||||
expect(src).toContain('export function binPath');
|
||||
expect(src).toContain('export function runBin');
|
||||
expect(src).toContain('fileURLToPath');
|
||||
});
|
||||
|
||||
// `new URL(import.meta.url).pathname` yields `/C:/Users/...` on Windows;
|
||||
// path.resolve then rebases it onto the drive root as `C:\C:\Users\...`,
|
||||
// so every subsequent spawn/read hits ENOENT. fileURLToPath is the fix.
|
||||
test('no hook uses URL.pathname to locate itself on disk', () => {
|
||||
const offending: string[] = [];
|
||||
for (const file of [...hookFiles(), HELPER]) {
|
||||
const src = fs.readFileSync(file, 'utf-8');
|
||||
src.split('\n').forEach((line, idx) => {
|
||||
if (line.trim().startsWith('*') || line.trim().startsWith('//')) return;
|
||||
if (/new URL\(import\.meta\.url\)\.pathname/.test(line)) {
|
||||
offending.push(`${path.basename(file)}:${idx + 1}`);
|
||||
}
|
||||
});
|
||||
}
|
||||
expect(offending).toEqual([]);
|
||||
});
|
||||
|
||||
// bin/gstack-* are extensionless bash scripts. Windows has no shebang
|
||||
// support, so they must be handed to bash — which runBin() does.
|
||||
test('no hook spawns a bin directly; all route through runBin', () => {
|
||||
const offending: string[] = [];
|
||||
for (const file of hookFiles()) {
|
||||
const src = fs.readFileSync(file, 'utf-8');
|
||||
src.split('\n').forEach((line, idx) => {
|
||||
if (line.trim().startsWith('*') || line.trim().startsWith('//')) return;
|
||||
if (/\bspawnSync\s*\(/.test(line)) {
|
||||
offending.push(`${path.basename(file)}:${idx + 1}`);
|
||||
}
|
||||
});
|
||||
}
|
||||
expect(offending).toEqual([]);
|
||||
});
|
||||
|
||||
test('repoRoot resolves to the install root; binPath finds a real bin', async () => {
|
||||
const { repoRoot, binPath } = await import(HELPER);
|
||||
expect(fs.existsSync(path.join(repoRoot(), 'bin'))).toBe(true);
|
||||
expect(fs.existsSync(path.join(repoRoot(), 'scripts', 'question-registry.ts'))).toBe(true);
|
||||
expect(fs.existsSync(binPath('gstack-question-log'))).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
// Behavioral proof: drive question-log-hook exactly the way Claude Code does
|
||||
// (hook JSON on stdin) against an isolated GSTACK_STATE_ROOT, and assert the
|
||||
// event actually lands. Pre-fix this wrote nothing on Windows and appended to
|
||||
// hook-errors.log instead, silently, on every question.
|
||||
describe('question-log-hook: end-to-end capture', () => {
|
||||
test('an AskUserQuestion fire is written to question-log.jsonl', () => {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-hook-'));
|
||||
try {
|
||||
const payload = {
|
||||
session_id: 'test-session',
|
||||
hook_event_name: 'PostToolUse',
|
||||
tool_name: 'AskUserQuestion',
|
||||
tool_use_id: 'toolu_hook_e2e',
|
||||
cwd: tmp,
|
||||
tool_input: {
|
||||
questions: [
|
||||
{
|
||||
question: 'Ship it?',
|
||||
options: [{ label: 'Ship now (recommended)' }, { label: 'Hold' }],
|
||||
},
|
||||
],
|
||||
},
|
||||
tool_response: { answers: [{ option_label: 'Ship now' }] },
|
||||
};
|
||||
|
||||
const res = spawnSync('bun', [path.join(HOOK_DIR, 'question-log-hook.ts')], {
|
||||
input: JSON.stringify(payload),
|
||||
encoding: 'utf-8',
|
||||
timeout: 20000,
|
||||
cwd: tmp,
|
||||
env: {
|
||||
...process.env,
|
||||
GSTACK_STATE_ROOT: tmp,
|
||||
GSTACK_QUESTION_LOG_NO_DERIVE: '1',
|
||||
},
|
||||
});
|
||||
expect(res.status).toBe(0);
|
||||
|
||||
// Slug depends on cwd, so find the log rather than guessing the project dir.
|
||||
const projects = path.join(tmp, 'projects');
|
||||
expect(fs.existsSync(projects)).toBe(true);
|
||||
const logs = fs
|
||||
.readdirSync(projects)
|
||||
.map((slug) => path.join(projects, slug, 'question-log.jsonl'))
|
||||
.filter((p) => fs.existsSync(p));
|
||||
expect(logs.length).toBe(1);
|
||||
|
||||
const event = JSON.parse(fs.readFileSync(logs[0], 'utf-8').trim().split('\n')[0]);
|
||||
expect(event.source).toBe('hook');
|
||||
expect(event.user_choice).toBe('Ship now');
|
||||
expect(event.recommended).toBe('Ship now');
|
||||
expect(event.followed_recommendation).toBe(true);
|
||||
expect(event.tool_use_id).toBe('toolu_hook_e2e');
|
||||
|
||||
// The failure mode this guards against was silent: the hook exited 0
|
||||
// while only ever appending to the error log.
|
||||
const errLog = path.join(tmp, 'hook-errors.log');
|
||||
expect(fs.existsSync(errLog) ? fs.readFileSync(errLog, 'utf-8') : '').toBe('');
|
||||
} finally {
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue