Skip to content

Commit e6f4353

Browse files
committed
Restore CI coverage for hook argv expansion and preflight blocking
The hook escaping regression came from asserting a literal `node` prefix even though installed hooks may be patched to an absolute node binary. The test now captures argv after shell expansion instead of accidentally invoking the real runner, and the pre-tool enforcer now sanitizes malformed threshold env input so agent-heavy exhausted-context preflight keeps blocking deterministically. The remaining CI flakes came from ambient test-process state leaking into the pre-tool harness (env kill-switches, cwd/config, and child_process mocks) plus a tmux-availability assumption in the duplicate-worker dispatch test, so those assertions are now hermetic and environment-agnostic. Constraint: Installed hooks may start with an absolute quoted node path after plugin setup Constraint: Pre-tool tests must not inherit hook kill-switches, Claude config, or child_process mocks from the surrounding test runner Constraint: Duplicate-worker dispatch coverage must not require tmux to be available Rejected: Keep assuming a literal `node` prefix in hook tests | no longer matches shipped hook shape Rejected: Leave malformed threshold env unchecked | comparison against NaN silently disables blocking Rejected: Assert immediate `notified` dispatch state in the duplicate-pane test | depends on tmux availability outside the test contract Confidence: high Scope-risk: narrow Directive: Keep hook argv assertions focused on post-shell tokenization, not a specific node binary spelling Directive: Keep shell-spawned hook tests pinned to isolated HOME/CLAUDE_CONFIG_DIR values, cleared skip flags, isolated child cwd, and unmocked child_process access so ambient state cannot leak in Tested: npm test -- --run src/__tests__/pre-tool-enforcer.test.ts Tested: npm test -- --run src/__tests__/pre-tool-enforcer.test.ts src/team/__tests__/api-interop.dispatch.test.ts src/__tests__/hooks-command-escaping.test.ts Tested: npm test -- --run Tested: npm run build Tested: npm run lint Not-tested: PR CI rerun on GitHub after push
1 parent 85b12bd commit e6f4353

File tree

4 files changed

+106
-22
lines changed

4 files changed

+106
-22
lines changed

scripts/pre-tool-enforcer.mjs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,16 @@ const MODE_STATE_FILES = [
9191
'omc-teams-state.json',
9292
];
9393
const AGENT_HEAVY_TOOLS = new Set(['Task', 'TaskCreate', 'TaskUpdate']);
94-
const PREFLIGHT_CONTEXT_THRESHOLD = parseInt(process.env.OMC_AGENT_PREFLIGHT_CONTEXT_THRESHOLD || '72', 10);
9594
const QUIET_LEVEL = getQuietLevel();
9695

96+
function getPreflightContextThreshold() {
97+
const parsed = Number.parseInt(process.env.OMC_AGENT_PREFLIGHT_CONTEXT_THRESHOLD || '72', 10);
98+
if (Number.isNaN(parsed)) return 72;
99+
return Math.max(1, Math.min(100, parsed));
100+
}
101+
102+
const PREFLIGHT_CONTEXT_THRESHOLD = getPreflightContextThreshold();
103+
97104
function getQuietLevel() {
98105
const parsed = Number.parseInt(process.env.OMC_QUIET || '0', 10);
99106
if (Number.isNaN(parsed)) return 0;

src/__tests__/hooks-command-escaping.test.ts

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,23 @@ interface HooksConfig {
99

1010
const hooksJsonPath = join(__dirname, '..', '..', 'hooks', 'hooks.json');
1111

12+
function expandHookCommandArgv(command: string, pluginRoot: string): string[] {
13+
const shellScript =
14+
`eval "set -- $HOOK_COMMAND"; ` +
15+
`node -e 'console.log(JSON.stringify(process.argv.slice(1)))' -- "$@"`;
16+
17+
return JSON.parse(
18+
execFileSync('bash', ['-lc', shellScript], {
19+
encoding: 'utf-8',
20+
env: {
21+
...process.env,
22+
HOOK_COMMAND: command,
23+
CLAUDE_PLUGIN_ROOT: pluginRoot,
24+
},
25+
}).trim()
26+
) as string[];
27+
}
28+
1229
function getHookCommands(): string[] {
1330
const raw = JSON.parse(readFileSync(hooksJsonPath, 'utf-8')) as HooksConfig;
1431
return Object.values(raw.hooks ?? {})
@@ -31,24 +48,13 @@ describe('hooks.json command escaping', () => {
3148
const pluginRoot = '/c/Users/First Last/.claude/plugins/cache/omc/oh-my-claudecode/4.7.10';
3249

3350
for (const command of getHookCommands()) {
34-
const argv = JSON.parse(
35-
execFileSync(
36-
'bash',
37-
['-lc', command.replace(/^node\b/, `node -e "console.log(JSON.stringify(process.argv.slice(1)))"`)],
38-
{
39-
encoding: 'utf-8',
40-
env: {
41-
...process.env,
42-
CLAUDE_PLUGIN_ROOT: pluginRoot,
43-
},
44-
}
45-
).trim()
46-
) as string[];
47-
48-
expect(argv[0]).toBe(`${pluginRoot}/scripts/run.cjs`);
49-
expect(argv[1]).toContain(`${pluginRoot}/scripts/`);
50-
expect(argv[0]).toContain('First Last');
51+
const argv = expandHookCommandArgv(command, pluginRoot);
52+
53+
expect(argv[0]).toMatch(/(^|[/\\])node(?:\.exe)?$/);
54+
expect(argv[1]).toBe(`${pluginRoot}/scripts/run.cjs`);
55+
expect(argv[2]).toContain(`${pluginRoot}/scripts/`);
5156
expect(argv[1]).toContain('First Last');
57+
expect(argv[2]).toContain('First Last');
5258
expect(argv).not.toContain('/c/Users/First');
5359
expect(argv).not.toContain('Last/.claude/plugins/cache/omc/oh-my-claudecode/4.7.10/scripts/run.cjs');
5460
}

src/__tests__/pre-tool-enforcer.test.ts

Lines changed: 73 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
1-
import { execSync } from 'child_process';
21
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs';
32
import { tmpdir } from 'os';
43
import { dirname, join } from 'path';
5-
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
4+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
5+
6+
vi.unmock('child_process');
7+
vi.unmock('node:child_process');
8+
9+
import { execSync } from 'child_process';
610

711
const SCRIPT_PATH = join(process.cwd(), 'scripts', 'pre-tool-enforcer.mjs');
812

@@ -14,15 +18,23 @@ function runPreToolEnforcerWithEnv(
1418
input: Record<string, unknown>,
1519
env: Record<string, string> = {},
1620
): Record<string, unknown> {
21+
const cwd = typeof input.cwd === 'string' ? input.cwd : process.cwd();
22+
const homeDir = join(cwd, '.test-home');
1723
const stdout = execSync(`node "${SCRIPT_PATH}"`, {
24+
cwd,
1825
input: JSON.stringify(input),
1926
encoding: 'utf-8',
2027
timeout: 5000,
2128
env: {
2229
...process.env,
30+
HOME: homeDir,
31+
CLAUDE_CONFIG_DIR: join(homeDir, '.claude'),
2332
NODE_ENV: 'test',
33+
DISABLE_OMC: '',
34+
OMC_SKIP_HOOKS: '',
2435
// Reset Bedrock/routing env vars so tests are isolated from the host environment.
2536
// Tests that exercise Bedrock model-routing behaviour set these explicitly via `env`.
37+
OMC_AGENT_PREFLIGHT_CONTEXT_THRESHOLD: '',
2638
OMC_ROUTING_FORCE_INHERIT: '',
2739
OMC_SUBAGENT_MODEL: '',
2840
CLAUDE_MODEL: '',
@@ -377,6 +389,65 @@ describe('pre-tool-enforcer fallback gating (issue #970)', () => {
377389
expect(String(output.reason)).toContain('Safe recovery');
378390
});
379391

392+
it('falls back to the default preflight threshold when the env value is invalid', () => {
393+
const transcriptPath = join(tempDir, 'transcript.jsonl');
394+
writeTranscriptWithContext(transcriptPath, 1000, 800); // 80%
395+
396+
const output = runPreToolEnforcerWithEnv(
397+
{
398+
tool_name: 'Task',
399+
toolInput: {
400+
subagent_type: 'oh-my-claudecode:executor',
401+
description: 'High fan-out execution',
402+
},
403+
cwd: tempDir,
404+
transcript_path: transcriptPath,
405+
session_id: 'session-1373-invalid-threshold',
406+
},
407+
{ OMC_AGENT_PREFLIGHT_CONTEXT_THRESHOLD: 'abc' },
408+
);
409+
410+
expect(output.decision).toBe('block');
411+
expect(String(output.reason)).toContain('threshold: 72%');
412+
});
413+
414+
it('ignores parent-process hook skip env when exercising preflight blocking', () => {
415+
const previousDisableOmc = process.env.DISABLE_OMC;
416+
const previousSkipHooks = process.env.OMC_SKIP_HOOKS;
417+
const transcriptPath = join(tempDir, 'transcript.jsonl');
418+
writeTranscriptWithContext(transcriptPath, 1000, 800); // 80%
419+
420+
process.env.DISABLE_OMC = '1';
421+
process.env.OMC_SKIP_HOOKS = 'pre-tool-use';
422+
423+
try {
424+
const output = runPreToolEnforcer({
425+
tool_name: 'Task',
426+
toolInput: {
427+
subagent_type: 'oh-my-claudecode:executor',
428+
description: 'High fan-out execution',
429+
},
430+
cwd: tempDir,
431+
transcript_path: transcriptPath,
432+
session_id: 'session-1373-parent-env',
433+
});
434+
435+
expect(output.decision).toBe('block');
436+
} finally {
437+
if (previousDisableOmc === undefined) {
438+
delete process.env.DISABLE_OMC;
439+
} else {
440+
process.env.DISABLE_OMC = previousDisableOmc;
441+
}
442+
443+
if (previousSkipHooks === undefined) {
444+
delete process.env.OMC_SKIP_HOOKS;
445+
} else {
446+
process.env.OMC_SKIP_HOOKS = previousSkipHooks;
447+
}
448+
}
449+
});
450+
380451
it('allows non-agent-heavy tools even when transcript context is high', () => {
381452
const transcriptPath = join(tempDir, 'transcript.jsonl');
382453
writeTranscriptWithContext(transcriptPath, 1000, 900); // 90%
@@ -392,7 +463,6 @@ describe('pre-tool-enforcer fallback gating (issue #970)', () => {
392463
expect(output.decision).toBeUndefined();
393464
});
394465

395-
396466
it('clears awaiting confirmation from session-scoped mode state when a skill is invoked', () => {
397467
const sessionId = 'session-confirm';
398468
const sessionStateDir = join(tempDir, '.omc', 'state', 'sessions', sessionId);

src/team/__tests__/api-interop.dispatch.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,7 @@ describe('team api dispatch-aware messaging', () => {
206206
const requests = await listDispatchRequests(teamName, cwd, { kind: 'mailbox', to_worker: 'worker-1' });
207207
expect(requests).toHaveLength(1);
208208
expect(requests[0]?.message_id).toBe(messageId);
209-
expect(requests[0]?.status).toBe('pending');
209+
expect(requests[0]?.pane_id).toBe('%9');
210+
expect(['pending', 'notified']).toContain(requests[0]?.status);
210211
});
211212
});

0 commit comments

Comments
 (0)