mirror of
https://github.com/Jeuners/ECC.git
synced 2026-09-09 15:02:30 +02:00
fix(hooks): preserve Stop output through lifecycle wrappers (#2493)
Preserve complete Stop-hook stdout through lifecycle wrappers, wait for queued output to flush before exiting, bound child output with a larger explicit buffer, and add end-to-end regressions for large, multibyte, dry-run, and failure cases.
This commit is contained in:
parent
c714dc5654
commit
28b922dee3
4 changed files with 188 additions and 18 deletions
|
|
@ -24,6 +24,9 @@ const { spawnSync } = require('child_process');
|
|||
|
||||
const repoRoot = path.join(__dirname, '..', '..');
|
||||
const runner = path.join(repoRoot, 'scripts', 'hooks', 'run-with-flags.js');
|
||||
const hooksConfig = JSON.parse(
|
||||
fs.readFileSync(path.join(repoRoot, 'hooks', 'hooks.json'), 'utf8')
|
||||
);
|
||||
|
||||
const MAX_STDIN = 1024 * 1024;
|
||||
|
||||
|
|
@ -42,14 +45,14 @@ function test(name, fn) {
|
|||
}
|
||||
}
|
||||
|
||||
function stopPayload(messageBytes) {
|
||||
function stopPayload(messageCharacters, character = 'm') {
|
||||
return JSON.stringify({
|
||||
session_id: `stop-stdout-test-${process.pid}`,
|
||||
transcript_path: path.join(workDir, 'missing-transcript.jsonl'),
|
||||
cwd: workDir,
|
||||
hook_event_name: 'Stop',
|
||||
stop_hook_active: false,
|
||||
last_assistant_message: 'm'.repeat(messageBytes)
|
||||
last_assistant_message: character.repeat(messageCharacters)
|
||||
});
|
||||
}
|
||||
|
||||
|
|
@ -62,6 +65,7 @@ function hookEnv() {
|
|||
};
|
||||
delete env.ECC_GATEGUARD;
|
||||
delete env.ECC_DISABLED_HOOKS;
|
||||
delete env.ECC_DRY_RUN;
|
||||
return env;
|
||||
}
|
||||
|
||||
|
|
@ -89,6 +93,26 @@ function runDirect(script, input) {
|
|||
});
|
||||
}
|
||||
|
||||
function runRegisteredStopHook(entry, input, envOverrides = {}) {
|
||||
const env = {
|
||||
...hookEnv(),
|
||||
CLAUDE_PLUGIN_ROOT: repoRoot,
|
||||
ECC_DISABLED_HOOKS: entry.id,
|
||||
...envOverrides
|
||||
};
|
||||
|
||||
return spawnSync(entry.hooks[0].command, {
|
||||
input,
|
||||
encoding: 'utf8',
|
||||
cwd: workDir,
|
||||
env,
|
||||
shell: true,
|
||||
timeout: 60000,
|
||||
maxBuffer: 16 * 1024 * 1024,
|
||||
stdio: ['pipe', 'pipe', 'pipe']
|
||||
});
|
||||
}
|
||||
|
||||
function assertStdoutContract(result, label) {
|
||||
assert.strictEqual(result.status, 0, `${label}: expected exit 0, got ${result.status}: ${result.stderr}`);
|
||||
if (result.stdout.length > 0) {
|
||||
|
|
@ -130,6 +154,78 @@ let failed = 0;
|
|||
// runner path, making the harness report "JSON validation failed".
|
||||
const realisticPayload = stopPayload(100 * 1024);
|
||||
|
||||
// Exercise the command users actually run from hooks.json. The runner already
|
||||
// flushes large stdout before exiting, but the outer lifecycle wrapper used to
|
||||
// call process.exit() immediately after forwarding it, cutting the JSON at the
|
||||
// OS pipe buffer and reintroducing #2222 above the tested runner layer.
|
||||
for (const entry of hooksConfig.hooks.Stop) {
|
||||
if (
|
||||
test(`${entry.id} registered wrapper flushes a 100KB Stop payload`, () => {
|
||||
const result = runRegisteredStopHook(entry, realisticPayload);
|
||||
assert.strictEqual(
|
||||
result.status,
|
||||
0,
|
||||
`${entry.id}: expected exit 0, got ${result.status}: ${result.stderr}`
|
||||
);
|
||||
assert.ok(
|
||||
result.stdout === realisticPayload,
|
||||
`${entry.id}: registered wrapper must echo ${realisticPayload.length} characters uncut (got ${result.stdout.length})`
|
||||
);
|
||||
JSON.parse(result.stdout);
|
||||
})
|
||||
)
|
||||
passed++;
|
||||
else failed++;
|
||||
}
|
||||
|
||||
const representativeStopEntry = hooksConfig.hooks.Stop.find(
|
||||
entry => entry.id === 'stop:cost-tracker'
|
||||
);
|
||||
|
||||
if (
|
||||
test('registered Stop wrapper flushes a 100KB dry-run payload', () => {
|
||||
const result = runRegisteredStopHook(representativeStopEntry, realisticPayload, {
|
||||
ECC_DISABLED_HOOKS: '',
|
||||
ECC_DRY_RUN: '1'
|
||||
});
|
||||
assert.strictEqual(result.status, 0, `expected exit 0, got ${result.status}: ${result.stderr}`);
|
||||
assert.ok(
|
||||
result.stdout === realisticPayload,
|
||||
`dry-run wrapper must echo ${realisticPayload.length} characters uncut (got ${result.stdout.length})`
|
||||
);
|
||||
JSON.parse(result.stdout);
|
||||
})
|
||||
)
|
||||
passed++;
|
||||
else failed++;
|
||||
|
||||
// spawnSync limits captured output by bytes while the runner's stdin cap is
|
||||
// counted after UTF-8 decoding. A payload can therefore be below MAX_STDIN in
|
||||
// characters but above Node's default 1MB child-process buffer in bytes.
|
||||
const multibytePayload = stopPayload(400 * 1024, '한');
|
||||
assert.ok(multibytePayload.length < MAX_STDIN, 'fixture must stay below the runner character cap');
|
||||
assert.ok(Buffer.byteLength(multibytePayload) > MAX_STDIN, 'fixture must exceed the default byte buffer');
|
||||
|
||||
for (const entry of hooksConfig.hooks.Stop) {
|
||||
if (
|
||||
test(`${entry.id} registered wrapper preserves a multibyte sub-cap payload`, () => {
|
||||
const result = runRegisteredStopHook(entry, multibytePayload);
|
||||
assert.strictEqual(
|
||||
result.status,
|
||||
0,
|
||||
`${entry.id}: expected exit 0, got ${result.status}: ${result.stderr}`
|
||||
);
|
||||
assert.ok(
|
||||
result.stdout === multibytePayload,
|
||||
`${entry.id}: registered wrapper must echo ${Buffer.byteLength(multibytePayload)} bytes uncut (got ${Buffer.byteLength(result.stdout)})`
|
||||
);
|
||||
JSON.parse(result.stdout);
|
||||
})
|
||||
)
|
||||
passed++;
|
||||
else failed++;
|
||||
}
|
||||
|
||||
for (const [hookId, script] of STOP_HOOKS) {
|
||||
if (
|
||||
test(`${hookId} via runner keeps stdout valid for a 100KB Stop payload`, () => {
|
||||
|
|
@ -146,6 +242,43 @@ for (const [hookId, script] of STOP_HOOKS) {
|
|||
|
||||
const oversizedPayload = stopPayload(MAX_STDIN + 64 * 1024);
|
||||
|
||||
if (
|
||||
test('registered Stop wrapper suppresses a >1MB dry-run payload', () => {
|
||||
const result = runRegisteredStopHook(representativeStopEntry, oversizedPayload, {
|
||||
ECC_DISABLED_HOOKS: '',
|
||||
ECC_DRY_RUN: '1'
|
||||
});
|
||||
assert.strictEqual(result.status, 0, `expected exit 0, got ${result.status}: ${result.stderr}`);
|
||||
assert.strictEqual(
|
||||
result.stdout.length,
|
||||
0,
|
||||
`dry-run wrapper must preserve oversized-input suppression (got ${result.stdout.length} characters)`
|
||||
);
|
||||
})
|
||||
)
|
||||
passed++;
|
||||
else failed++;
|
||||
|
||||
for (const entry of hooksConfig.hooks.Stop) {
|
||||
if (
|
||||
test(`${entry.id} registered wrapper suppresses a >1MB Stop payload`, () => {
|
||||
const result = runRegisteredStopHook(entry, oversizedPayload);
|
||||
assert.strictEqual(
|
||||
result.status,
|
||||
0,
|
||||
`${entry.id}: expected exit 0, got ${result.status}: ${result.stderr}`
|
||||
);
|
||||
assert.strictEqual(
|
||||
result.stdout.length,
|
||||
0,
|
||||
`${entry.id}: wrapper must preserve oversized-input suppression (got ${result.stdout.length} characters)`
|
||||
);
|
||||
})
|
||||
)
|
||||
passed++;
|
||||
else failed++;
|
||||
}
|
||||
|
||||
for (const [hookId, script] of [...STOP_HOOKS, ['stop:desktop-notify', 'scripts/hooks/desktop-notify.js']]) {
|
||||
if (
|
||||
test(`${hookId} via runner fails open on a >1MB Stop payload`, () => {
|
||||
|
|
|
|||
|
|
@ -95,6 +95,34 @@ function runTests() {
|
|||
assert.strictEqual(result.stdout, input, 'Expected stdin to be passed through unchanged');
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('flushes a large dry-run preview when oversized stdout is suppressed', () => {
|
||||
const runWithFlags = path.resolve(__dirname, '..', '..', 'scripts', 'hooks', 'run-with-flags.js');
|
||||
const hookScript = 'scripts/hooks/block-no-verify.js';
|
||||
const command = 'x'.repeat(900 * 1024);
|
||||
const document = JSON.stringify({ tool: 'Bash', tool_input: { command } });
|
||||
const input = document.padEnd(1024 * 1024 + 1024, ' ');
|
||||
|
||||
const result = spawnSync(process.execPath, [
|
||||
runWithFlags,
|
||||
'pre:bash:block-no-verify',
|
||||
hookScript,
|
||||
'standard,strict',
|
||||
], {
|
||||
input,
|
||||
encoding: 'utf8',
|
||||
env: { ...process.env, ECC_DRY_RUN: '1' },
|
||||
cwd: path.resolve(__dirname, '..', '..'),
|
||||
maxBuffer: 4 * 1024 * 1024,
|
||||
});
|
||||
|
||||
assert.strictEqual(result.status, 0, `Expected exit 0, got ${result.status}`);
|
||||
assert.strictEqual(result.stdout, '', 'Oversized dry-run input must keep stdout suppressed');
|
||||
assert.ok(
|
||||
result.stderr.endsWith(`command=${command}\n`),
|
||||
`Expected the complete dry-run preview on stderr, got ${result.stderr.length} characters`
|
||||
);
|
||||
})) passed++; else failed++;
|
||||
|
||||
if (test('dry-run preview includes command for bash hooks', () => {
|
||||
const runWithFlags = path.resolve(__dirname, '..', '..', 'scripts', 'hooks', 'run-with-flags.js');
|
||||
const hookScript = 'scripts/hooks/block-no-verify.js';
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue