diff --git a/.changeset/serene-koalas-hum.md b/.changeset/serene-koalas-hum.md new file mode 100644 index 000000000..df6fe75eb --- /dev/null +++ b/.changeset/serene-koalas-hum.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2324 +--- +**The context-monitor hook no longer fails Codex's Stop hook** — GSD wires `gsd-context-monitor` to Codex lifecycle events including `Stop`, but the hook emitted a `hookSpecificOutput.additionalContext` envelope that Codex's Stop schema rejects ("hook returned invalid stop hook JSON output") exactly when context was low. The hook now emits that envelope only for context-injection events (PostToolUse / AfterTool) and exits silently for Stop and every other lifecycle event, while its debounce and critical-session bookkeeping still run. (#2289) diff --git a/hooks/gsd-context-monitor.js b/hooks/gsd-context-monitor.js index 991535f40..656540cde 100644 --- a/hooks/gsd-context-monitor.js +++ b/hooks/gsd-context-monitor.js @@ -180,15 +180,33 @@ process.stdin.on('end', () => { 'starting new complex work.'; } - const output = { - hookSpecificOutput: { - hookEventName: (data.hook_event_name && data.hook_event_name.trim()) - || (process.env.GEMINI_API_KEY ? "AfterTool" : "PostToolUse"), - additionalContext: message - } - }; + // #2289: the hookSpecificOutput.additionalContext envelope is only a valid + // output shape for the context-injection events (PostToolUse, and AfterTool + // for the Gemini dialect). This hook is also wired to other lifecycle events + // on some hosts — Codex registers it under Stop / SubagentStart / + // SubagentStop / PreCompact (#772) — and those reject the envelope + // ("hook returned invalid stop hook JSON output"). Use a POSITIVE allowlist: + // emit only for injection-capable events; every other event, and a + // missing/unrecognized name, exits 0 with no stdout. A Stop-only blacklist is + // not enough — a missing name would still fall through to the injection path. + // All side effects above (debounce counter, one-time critical-session + // recording) have already run regardless of whether output is emitted. + const eventName = (data.hook_event_name && data.hook_event_name.trim()) || ""; + // Preserve the pre-#2289 Gemini fallback: a missing event name under a + // Gemini-dialect runtime (GEMINI_API_KEY set) still means AfterTool, so its + // advisory output is unchanged. A missing name on any other host is silent. + const geminiFallback = eventName === "" && !!process.env.GEMINI_API_KEY; + const injectionSupported = eventName === "PostToolUse" || eventName === "AfterTool" || geminiFallback; - process.stdout.write(JSON.stringify(output)); + if (injectionSupported) { + const output = { + hookSpecificOutput: { + hookEventName: eventName || "AfterTool", + additionalContext: message + } + }; + process.stdout.write(JSON.stringify(output)); + } } catch (e) { // Silent fail -- never block tool execution process.exit(0); diff --git a/tests/claude-orchestration.test.cjs b/tests/claude-orchestration.test.cjs index abc7f4453..2eaa89e6f 100644 --- a/tests/claude-orchestration.test.cjs +++ b/tests/claude-orchestration.test.cjs @@ -546,11 +546,20 @@ describe('emitWorkflowScript — per-plan use_worktree (#2772 / #2285 finding 1) { minLength: 1, maxLength: 4 }, ).filter((plans) => new Set(plans.map((p) => p.id)).size === plans.length), // unique ids (planSpecs) => { + // Only plan IDs are unique — briefs may legitimately collide (fast-check + // shrinks toward short/empty strings, so duplicate briefs are common). + // emitWorkflowScript emits one agent() per plan keyed by the brief label + // and is correct for duplicate briefs, but the per-plan line lookup below + // (indexOf) would find the FIRST occurrence and misattribute its isolation + // when two plans share a brief. Make each agent() label unique by suffixing + // the unique id, so the lookup is unambiguous — this disambiguates the TEST + // probe, it does not change what the code under test does. + const labelFor = (p) => p.brief + ' [' + p.id + ']'; const waves = [{ id: 'w1', plans: planSpecs.map((p, i) => ({ id: p.id, - brief: p.brief, + brief: labelFor(p), files_modified: ['src/file' + i + '.cts'], // disjoint -> no overlap-driven staging noise use_worktree: p.useWorktree, })), @@ -558,7 +567,7 @@ describe('emitWorkflowScript — per-plan use_worktree (#2772 / #2285 finding 1) const r = emitWorkflowScript({ phaseDir: '.p', runId: 'r', waves }); assert.strictEqual(r.ok, true); for (const p of planSpecs) { - const briefEsc = JSON.stringify(p.brief); + const briefEsc = JSON.stringify(labelFor(p)); const idx = r.script.indexOf('agent(' + briefEsc + ','); assert.ok(idx !== -1, 'agent() call for plan must exist'); const lineEnd = r.script.indexOf('\n', idx); diff --git a/tests/fix-2289-context-monitor-event-allowlist.test.cjs b/tests/fix-2289-context-monitor-event-allowlist.test.cjs new file mode 100644 index 000000000..d25ad3f36 --- /dev/null +++ b/tests/fix-2289-context-monitor-event-allowlist.test.cjs @@ -0,0 +1,204 @@ +'use strict'; + +/** + * #2289 — gsd-context-monitor lifecycle-event output allowlist. + * + * The context monitor emits a `hookSpecificOutput.additionalContext` envelope + * to inject context warnings. That shape is only valid for the context-injection + * events (PostToolUse, and AfterTool for the Gemini dialect). Codex also wires + * this hook to Stop / SubagentStart / SubagentStop / PreCompact (#772), and + * Codex's Stop schema REJECTS the envelope ("hook returned invalid stop hook + * JSON output"). The fix uses a positive allowlist: emit only for + * injection-capable events; every other event — and a missing/unknown name — + * exits 0 with NO stdout, while side effects (debounce, critical-session + * recording) still run. + * + * These tests drive the real hook script end-to-end (spawn + stdin + a fresh + * metrics bridge file), asserting behavior, not source text. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); + +const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-context-monitor.js'); + +// Run the monitor with a synthetic, fresh metrics bridge file. +// Returns { stdout, warnData } and cleans up the bridge + sentinel files. +// opts: { event, remaining, used = 80, gemini = false, gsdActive = false } +function runMonitor(opts) { + const { + event, + remaining, + used = 80, + gemini = false, + gsdActive = false, + } = opts; + + const sessionId = `fix-2289-${Date.now()}-${Math.random().toString(36).slice(2)}`; + const tmpDir = os.tmpdir(); + const metricsPath = path.join(tmpDir, `claude-ctx-${sessionId}.json`); + const warnPath = path.join(tmpDir, `claude-ctx-${sessionId}-warned.json`); + + // Fresh (non-stale) metrics: timestamp is "now" in seconds. + fs.writeFileSync(metricsPath, JSON.stringify({ + timestamp: Math.floor(Date.now() / 1000), + remaining_percentage: remaining, + used_pct: used, + })); + + // Optional GSD-active project dir (STATE.md present) so the critical-session + // recording side effect is reachable. + let cwd = tmpDir; + let projDir = null; + if (gsdActive) { + projDir = fs.mkdtempSync(path.join(tmpDir, 'fix-2289-proj-')); + fs.mkdirSync(path.join(projDir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(projDir, '.planning', 'STATE.md'), '# State\n'); + cwd = projDir; + } + + const payload = { session_id: sessionId, cwd }; + if (event !== undefined) payload.hook_event_name = event; + + const env = { ...process.env }; + if (gemini) env.GEMINI_API_KEY = 'test-key'; + else delete env.GEMINI_API_KEY; + + let stdout = ''; + try { + stdout = execFileSync(process.execPath, [HOOK_PATH], { + input: JSON.stringify(payload), + env, + encoding: 'utf8', + timeout: 8000, + }); + } catch (e) { + stdout = e.stdout || ''; + } + + let warnData = null; + try { + warnData = JSON.parse(fs.readFileSync(warnPath, 'utf8')); + } catch { /* sentinel may not exist */ } + + // Cleanup + for (const p of [metricsPath, warnPath]) { + try { fs.unlinkSync(p); } catch { /* ignore */ } + } + if (projDir) { + // Retry-tolerant teardown: the critical path fires a detached, unref()'d + // `state record-session` grandchild against projDir, and execFileSync does + // not wait for it. maxRetries/retryDelay absorbs the transient + // EBUSY/ENOTEMPTY window while that process exits, so cleanup can neither + // flake nor leak the temp dir (mirrors tests/helpers.cjs cleanup(); see the + // #2289 review and the prior fix in perf-317-context-monitor-fs.test.cjs). + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- test fixture teardown of a unique mkdtemp dir + try { fs.rmSync(projDir, { recursive: true, force: true, maxRetries: 20, retryDelay: 100 }); } catch { /* ignore */ } + } + + return { stdout, warnData }; +} + +describe('#2289 context-monitor: non-injection events exit silently', () => { + // Boundary coverage around WARNING (35) and CRITICAL (25) — Stop must stay + // silent at limit-1 / limit / limit+1 for BOTH thresholds. + for (const remaining of [40, 36, 35, 34, 26, 25, 24, 20]) { + test(`Stop event at remaining=${remaining}% → exit 0, empty stdout`, () => { + const { stdout } = runMonitor({ event: 'Stop', remaining }); + assert.strictEqual(stdout, '', `Stop must emit nothing at remaining=${remaining}% (Codex rejects the envelope)`); + }); + } + + test('missing hook_event_name (no Gemini) at 30% → empty stdout', () => { + const { stdout } = runMonitor({ event: undefined, remaining: 30 }); + assert.strictEqual(stdout, '', 'a missing event name must not fall through to the injection envelope'); + }); + + test('empty-string hook_event_name (no Gemini) at 30% → empty stdout', () => { + const { stdout } = runMonitor({ event: ' ', remaining: 30 }); + assert.strictEqual(stdout, '', 'a blank event name must be treated as missing → silent'); + }); + + for (const event of ['SubagentStart', 'SubagentStop', 'PreCompact', 'SessionStart', 'BeforeTool']) { + test(`unknown/non-injection event "${event}" at 30% → empty stdout`, () => { + const { stdout } = runMonitor({ event, remaining: 30 }); + assert.strictEqual(stdout, '', `${event} is not injection-capable and must emit nothing`); + }); + } +}); + +describe('#2289 context-monitor: injection events still warn (unchanged)', () => { + test('PostToolUse at 30% → WARNING envelope with hookEventName PostToolUse', () => { + const { stdout } = runMonitor({ event: 'PostToolUse', remaining: 30, used: 70 }); + assert.notStrictEqual(stdout, '', 'PostToolUse must still emit a warning envelope'); + const parsed = JSON.parse(stdout); + assert.strictEqual(parsed.hookSpecificOutput.hookEventName, 'PostToolUse'); + assert.match(parsed.hookSpecificOutput.additionalContext, /CONTEXT WARNING/); + }); + + test('PostToolUse at 20% → CRITICAL envelope', () => { + const { stdout } = runMonitor({ event: 'PostToolUse', remaining: 20, used: 80 }); + const parsed = JSON.parse(stdout); + assert.strictEqual(parsed.hookSpecificOutput.hookEventName, 'PostToolUse'); + assert.match(parsed.hookSpecificOutput.additionalContext, /CONTEXT CRITICAL/); + }); + + test('AfterTool at 30% → WARNING envelope with hookEventName AfterTool', () => { + const { stdout } = runMonitor({ event: 'AfterTool', remaining: 30 }); + const parsed = JSON.parse(stdout); + assert.strictEqual(parsed.hookSpecificOutput.hookEventName, 'AfterTool'); + assert.match(parsed.hookSpecificOutput.additionalContext, /CONTEXT WARNING/); + }); + + test('missing event name WITH Gemini env at 30% → AfterTool envelope (fallback preserved)', () => { + const { stdout } = runMonitor({ event: undefined, remaining: 30, gemini: true }); + assert.notStrictEqual(stdout, '', 'Gemini AfterTool fallback must still emit when the event name is absent'); + const parsed = JSON.parse(stdout); + assert.strictEqual(parsed.hookSpecificOutput.hookEventName, 'AfterTool'); + }); + + test('explicit PostToolUse WITH Gemini env → explicit name wins over the AfterTool fallback', () => { + // Precedence guard: the Gemini fallback only applies to a MISSING name; an + // explicit PostToolUse must still report as PostToolUse even under GEMINI_API_KEY. + const { stdout } = runMonitor({ event: 'PostToolUse', remaining: 30, gemini: true }); + const parsed = JSON.parse(stdout); + assert.strictEqual(parsed.hookSpecificOutput.hookEventName, 'PostToolUse'); + assert.match(parsed.hookSpecificOutput.additionalContext, /CONTEXT WARNING/); + }); + + // Threshold boundaries on the emit path: 36 = no warn, 35 = warn, 25 = critical, 26 = warn. + test('PostToolUse at 36% (above WARNING) → empty stdout', () => { + const { stdout } = runMonitor({ event: 'PostToolUse', remaining: 36 }); + assert.strictEqual(stdout, '', 'no warning above the 35% threshold'); + }); + + test('PostToolUse at 35% (WARNING boundary) → WARNING envelope', () => { + const { stdout } = runMonitor({ event: 'PostToolUse', remaining: 35 }); + assert.match(JSON.parse(stdout).hookSpecificOutput.additionalContext, /CONTEXT WARNING/); + }); + + test('PostToolUse at 25% (CRITICAL boundary) → CRITICAL envelope', () => { + const { stdout } = runMonitor({ event: 'PostToolUse', remaining: 25 }); + assert.match(JSON.parse(stdout).hookSpecificOutput.additionalContext, /CONTEXT CRITICAL/); + }); +}); + +describe('#2289 context-monitor: side effects still fire on silent events (no output ≠ no side effect)', () => { + test('Stop at 30% still writes the debounce sentinel (bookkeeping runs)', () => { + const { stdout, warnData } = runMonitor({ event: 'Stop', remaining: 30 }); + assert.strictEqual(stdout, '', 'Stop emits nothing'); + assert.ok(warnData, 'the debounce sentinel must still be written on a silenced Stop event'); + assert.strictEqual(warnData.lastLevel, 'warning', 'debounce level bookkeeping runs regardless of output'); + }); + + test('Stop at 20% in a GSD project still records the critical-session sentinel', () => { + const { stdout, warnData } = runMonitor({ event: 'Stop', remaining: 20, used: 80, gsdActive: true }); + assert.strictEqual(stdout, '', 'Stop emits nothing even at critical context'); + assert.ok(warnData, 'sentinel must be written'); + assert.strictEqual(warnData.criticalRecorded, true, 'critical-session recording side effect fires on the silent Stop event'); + }); +}); diff --git a/tests/fixtures/golden-install-parity/antigravity.json b/tests/fixtures/golden-install-parity/antigravity.json index a2d5dc2a0..5b1534d47 100644 --- a/tests/fixtures/golden-install-parity/antigravity.json +++ b/tests/fixtures/golden-install-parity/antigravity.json @@ -314,7 +314,7 @@ "hooks/gsd-check-update-worker.js": "fa301e6366270d5f", "hooks/gsd-check-update.js": "4617a98bf529e4c3", "hooks/gsd-config-reload.js": "96546e0e8bb47904", - "hooks/gsd-context-monitor.js": "6d81d7326e5b2710", + "hooks/gsd-context-monitor.js": "31a4d99fd3b260ad", "hooks/gsd-cursor-post-tool.js": "9168e0a09de1972a", "hooks/gsd-cursor-pre-tool.js": "873998b25e308c29", "hooks/gsd-cursor-session-start.js": "9b2e6f4f0c405375", diff --git a/tests/fixtures/golden-install-parity/augment.json b/tests/fixtures/golden-install-parity/augment.json index a67da0c77..4742483ab 100644 --- a/tests/fixtures/golden-install-parity/augment.json +++ b/tests/fixtures/golden-install-parity/augment.json @@ -385,7 +385,7 @@ "hooks/gsd-check-update-worker.js": "cc1ef5f840f9dfc9", "hooks/gsd-check-update.js": "7b3a7983d5f1f5d3", "hooks/gsd-config-reload.js": "96546e0e8bb47904", - "hooks/gsd-context-monitor.js": "44ff1bbf292747af", + "hooks/gsd-context-monitor.js": "493e24c139c61129", "hooks/gsd-cursor-post-tool.js": "9168e0a09de1972a", "hooks/gsd-cursor-pre-tool.js": "873998b25e308c29", "hooks/gsd-cursor-session-start.js": "9b2e6f4f0c405375", diff --git a/tests/fixtures/golden-install-parity/claude-local.json b/tests/fixtures/golden-install-parity/claude-local.json index 13a384d94..ca1d0e981 100644 --- a/tests/fixtures/golden-install-parity/claude-local.json +++ b/tests/fixtures/golden-install-parity/claude-local.json @@ -384,7 +384,7 @@ "hooks/gsd-check-update-worker.js": "a530efdb5fdc0da3", "hooks/gsd-check-update.js": "25cde66a12d6b886", "hooks/gsd-config-reload.js": "96546e0e8bb47904", - "hooks/gsd-context-monitor.js": "ecbe9747e4a442e0", + "hooks/gsd-context-monitor.js": "f2ac6a1033d140de", "hooks/gsd-cursor-post-tool.js": "8a8a249c0642cc71", "hooks/gsd-cursor-pre-tool.js": "8cb8e8f895edaec9", "hooks/gsd-cursor-session-start.js": "05a14e903c5edafa", diff --git a/tests/fixtures/golden-install-parity/claude.json b/tests/fixtures/golden-install-parity/claude.json index ed0118837..1f7126836 100644 --- a/tests/fixtures/golden-install-parity/claude.json +++ b/tests/fixtures/golden-install-parity/claude.json @@ -313,7 +313,7 @@ "hooks/gsd-check-update-worker.js": "a530efdb5fdc0da3", "hooks/gsd-check-update.js": "25cde66a12d6b886", "hooks/gsd-config-reload.js": "96546e0e8bb47904", - "hooks/gsd-context-monitor.js": "ecbe9747e4a442e0", + "hooks/gsd-context-monitor.js": "f2ac6a1033d140de", "hooks/gsd-cursor-post-tool.js": "8a8a249c0642cc71", "hooks/gsd-cursor-pre-tool.js": "8cb8e8f895edaec9", "hooks/gsd-cursor-session-start.js": "05a14e903c5edafa", diff --git a/tests/fixtures/golden-install-parity/codebuddy.json b/tests/fixtures/golden-install-parity/codebuddy.json index d792532aa..5ea483dd8 100644 --- a/tests/fixtures/golden-install-parity/codebuddy.json +++ b/tests/fixtures/golden-install-parity/codebuddy.json @@ -385,7 +385,7 @@ "hooks/gsd-check-update-worker.js": "bdc9324a2f080ddd", "hooks/gsd-check-update.js": "b7669f605631e506", "hooks/gsd-config-reload.js": "96546e0e8bb47904", - "hooks/gsd-context-monitor.js": "f372804867cabe40", + "hooks/gsd-context-monitor.js": "8d512ba0b08fc4e3", "hooks/gsd-cursor-post-tool.js": "9168e0a09de1972a", "hooks/gsd-cursor-pre-tool.js": "873998b25e308c29", "hooks/gsd-cursor-session-start.js": "9b2e6f4f0c405375", diff --git a/tests/fixtures/golden-install-parity/codex.json b/tests/fixtures/golden-install-parity/codex.json index 2dcb4d563..3c92bb509 100644 --- a/tests/fixtures/golden-install-parity/codex.json +++ b/tests/fixtures/golden-install-parity/codex.json @@ -418,7 +418,7 @@ "gsd-core/workflows/verify-phase.md": "2e12c3cb97a9122a", "gsd-core/workflows/verify-work.md": "5b348e73fc0d0829", "hooks/gsd-check-update.js": "ef48957eb6ac6a10", - "hooks/gsd-context-monitor.js": "76fecaaa2babd6c1", + "hooks/gsd-context-monitor.js": "ec76b23f45d29b02", "scripts/changeset/README.md": "86ff89331dfd94b2", "scripts/changeset/cli.cjs": "68f92a344b199271", "scripts/changeset/github-release-notes.cjs": "795677f0c009b132", diff --git a/tests/fixtures/golden-install-parity/hermes.json b/tests/fixtures/golden-install-parity/hermes.json index 5b169e580..90875b0fe 100644 --- a/tests/fixtures/golden-install-parity/hermes.json +++ b/tests/fixtures/golden-install-parity/hermes.json @@ -314,7 +314,7 @@ "hooks/gsd-check-update-worker.js": "7989cc2bedd1138d", "hooks/gsd-check-update.js": "25f5ad726f76fc11", "hooks/gsd-config-reload.js": "880b696458e85e9b", - "hooks/gsd-context-monitor.js": "41d28e0db7b20968", + "hooks/gsd-context-monitor.js": "b829f618e82406de", "hooks/gsd-cursor-post-tool.js": "8a8a249c0642cc71", "hooks/gsd-cursor-pre-tool.js": "8cb8e8f895edaec9", "hooks/gsd-cursor-session-start.js": "05a14e903c5edafa", diff --git a/tests/fixtures/golden-install-parity/kimi.json b/tests/fixtures/golden-install-parity/kimi.json index 757d1ab89..85eb40534 100644 --- a/tests/fixtures/golden-install-parity/kimi.json +++ b/tests/fixtures/golden-install-parity/kimi.json @@ -4,7 +4,7 @@ ".kimi/hooks/gsd-check-update-worker.js": "6593f80914f4edee", ".kimi/hooks/gsd-check-update.js": "aac7612ee04ff5fe", ".kimi/hooks/gsd-config-reload.js": "96546e0e8bb47904", - ".kimi/hooks/gsd-context-monitor.js": "783c93351e82151f", + ".kimi/hooks/gsd-context-monitor.js": "4c0535bc269e130e", ".kimi/hooks/gsd-cursor-post-tool.js": "9168e0a09de1972a", ".kimi/hooks/gsd-cursor-pre-tool.js": "873998b25e308c29", ".kimi/hooks/gsd-cursor-session-start.js": "9b2e6f4f0c405375", diff --git a/tests/fixtures/golden-install-parity/opencode.json b/tests/fixtures/golden-install-parity/opencode.json index 0389a4fab..12b64cfd1 100644 --- a/tests/fixtures/golden-install-parity/opencode.json +++ b/tests/fixtures/golden-install-parity/opencode.json @@ -385,7 +385,7 @@ "hooks/gsd-check-update-worker.js": "385fb7c67810baf6", "hooks/gsd-check-update.js": "4549451414ffa7d7", "hooks/gsd-config-reload.js": "96546e0e8bb47904", - "hooks/gsd-context-monitor.js": "7a9787868a39b76d", + "hooks/gsd-context-monitor.js": "91ce47cd2dc3bbe8", "hooks/gsd-cursor-post-tool.js": "9168e0a09de1972a", "hooks/gsd-cursor-pre-tool.js": "873998b25e308c29", "hooks/gsd-cursor-session-start.js": "9b2e6f4f0c405375", diff --git a/tests/fixtures/golden-install-parity/pi.json b/tests/fixtures/golden-install-parity/pi.json index cfe486f79..ef2dbc2a1 100644 --- a/tests/fixtures/golden-install-parity/pi.json +++ b/tests/fixtures/golden-install-parity/pi.json @@ -281,7 +281,7 @@ "hooks/gsd-check-update-worker.js": "55376b5b9335a580", "hooks/gsd-check-update.js": "a89562537a41f83d", "hooks/gsd-config-reload.js": "96546e0e8bb47904", - "hooks/gsd-context-monitor.js": "1c48eb0f38a24318", + "hooks/gsd-context-monitor.js": "e72cba19eeab9036", "hooks/gsd-cursor-post-tool.js": "9168e0a09de1972a", "hooks/gsd-cursor-pre-tool.js": "873998b25e308c29", "hooks/gsd-cursor-session-start.js": "9b2e6f4f0c405375", diff --git a/tests/fixtures/golden-install-parity/qwen.json b/tests/fixtures/golden-install-parity/qwen.json index d04b00d06..2149e8638 100644 --- a/tests/fixtures/golden-install-parity/qwen.json +++ b/tests/fixtures/golden-install-parity/qwen.json @@ -314,7 +314,7 @@ "hooks/gsd-check-update-worker.js": "4bb354044e0dff91", "hooks/gsd-check-update.js": "d2065cb3e725a42a", "hooks/gsd-config-reload.js": "4f52b8a0120bb1b8", - "hooks/gsd-context-monitor.js": "437a33e6e3058640", + "hooks/gsd-context-monitor.js": "b71785540277e9d7", "hooks/gsd-cursor-post-tool.js": "8a8a249c0642cc71", "hooks/gsd-cursor-pre-tool.js": "8cb8e8f895edaec9", "hooks/gsd-cursor-session-start.js": "05a14e903c5edafa", diff --git a/tests/perf-317-context-monitor-fs.test.cjs b/tests/perf-317-context-monitor-fs.test.cjs index 31da5bae7..3c580566e 100644 --- a/tests/perf-317-context-monitor-fs.test.cjs +++ b/tests/perf-317-context-monitor-fs.test.cjs @@ -64,7 +64,10 @@ function runMonitorRaw(opts) { fs.writeFileSync(warnPath, JSON.stringify(wd)); } - const input = JSON.stringify({ session_id: sessionId, cwd }); + // #2289: explicit hook_event_name is required — the hook now emits its + // envelope ONLY for the PostToolUse/AfterTool allowlist; a missing name + // (non-Gemini) is silent. These callers model PostToolUse invocations. + const input = JSON.stringify({ session_id: sessionId, cwd, hook_event_name: 'PostToolUse' }); let stdout = ''; let exitCode = 0; @@ -185,8 +188,11 @@ describe('perf #317: config.json absent (exercises config-missing → defaults p let exitCode = 0; let stdout = ''; try { + // #2289: send hook_event_name: 'PostToolUse' so the silence asserted below + // is attributable ONLY to context_warnings=false, not to the hook's + // non-injection-event silence path. stdout = execFileSync(process.execPath, [MONITOR_PATH], { - input: JSON.stringify({ session_id: sessionId, cwd: testCwd }), + input: JSON.stringify({ session_id: sessionId, cwd: testCwd, hook_event_name: 'PostToolUse' }), encoding: 'utf-8', timeout: 5000, }); @@ -351,9 +357,13 @@ function runHook(sessionId, remainingPct, cwd) { timestamp: Math.floor(Date.now() / 1000), })); + // #2289: explicit hook_event_name: 'PostToolUse' so the hook takes the + // emitting/allowlisted path — the tests in this block assert on stdout + // content and record-session side effects, not event-name plumbing. const input = JSON.stringify({ session_id: sessionId, cwd, + hook_event_name: 'PostToolUse', }); const result = spawnSync(process.execPath, [HOOK_PATH], { @@ -640,7 +650,10 @@ function runMonitorHook(remainingPct, usedPct) { timestamp: Math.floor(Date.now() / 1000), })); - const input = JSON.stringify({ session_id: sessionId, cwd: os.tmpdir() }); + // #2289: explicit hook_event_name: 'PostToolUse' — this helper's callers + // assert on emitted message content (used_pct wording), which requires + // the allowlisted emitting path. + const input = JSON.stringify({ session_id: sessionId, cwd: os.tmpdir(), hook_event_name: 'PostToolUse' }); let stdout = ''; try { stdout = execFileSync(process.execPath, [MONITOR_PATH], { @@ -826,36 +839,26 @@ function makeSessionId(suffix) { // ─── hookEventName echoing ──────────────────────────────────────────────────── -describe('bug #925: context monitor echoes the invoking hook event name', () => { - test('hookEventName is "Stop" when payload contains hook_event_name: "Stop"', () => { +describe('bug #925: context monitor echoes the invoking hook event name (superseded for non-injection events by #2289)', () => { + test('Stop is a non-injection event → silent (#2289)', () => { + // #2289: Codex's Stop schema rejects the hookSpecificOutput envelope + // entirely ("invalid stop hook JSON output"), so the hook must emit + // NOTHING for Stop rather than echo it. This supersedes bug #925's + // "echo the triggering event name" behavior for Stop specifically. const out = runMonitor({ hookEventName: 'Stop', sessionId: makeSessionId('stop') }); - assert.ok(out, 'hook must emit output when context is below WARNING threshold (remaining=30)'); - assert.strictEqual( - out.hookSpecificOutput?.hookEventName, - 'Stop', - `Expected hookEventName "Stop" but got "${out.hookSpecificOutput?.hookEventName}". ` + - 'The hook must echo the hook_event_name from stdin, not hardcode "PostToolUse".' - ); + assert.strictEqual(out, null, 'Stop is a non-injection event → silent (#2289)'); }); - test('hookEventName is "SubagentStop" when payload contains hook_event_name: "SubagentStop"', () => { + test('SubagentStop is a non-injection event → silent (#2289)', () => { + // #2289: same rationale as Stop above — non-injection events get no envelope. const out = runMonitor({ hookEventName: 'SubagentStop', sessionId: makeSessionId('subagent-stop') }); - assert.ok(out, 'hook must emit output when context is below WARNING threshold'); - assert.strictEqual( - out.hookSpecificOutput?.hookEventName, - 'SubagentStop', - `Expected hookEventName "SubagentStop" but got "${out.hookSpecificOutput?.hookEventName}".` - ); + assert.strictEqual(out, null, 'SubagentStop is a non-injection event → silent (#2289)'); }); - test('hookEventName is "PreCompact" when payload contains hook_event_name: "PreCompact"', () => { + test('PreCompact is a non-injection event → silent (#2289)', () => { + // #2289: same rationale as Stop above — non-injection events get no envelope. const out = runMonitor({ hookEventName: 'PreCompact', sessionId: makeSessionId('precompact') }); - assert.ok(out, 'hook must emit output when context is below WARNING threshold'); - assert.strictEqual( - out.hookSpecificOutput?.hookEventName, - 'PreCompact', - `Expected hookEventName "PreCompact" but got "${out.hookSpecificOutput?.hookEventName}".` - ); + assert.strictEqual(out, null, 'PreCompact is a non-injection event → silent (#2289)'); }); test('hookEventName is "PostToolUse" when payload contains hook_event_name: "PostToolUse"', () => { @@ -871,8 +874,12 @@ describe('bug #925: context monitor echoes the invoking hook event name', () => // ─── Fallback behaviour (no hook_event_name in payload) ────────────────────── -describe('bug #925: context monitor falls back to heuristic when hook_event_name absent', () => { - test('falls back to "PostToolUse" when hook_event_name is absent (non-Gemini)', () => { +describe('bug #925: context monitor falls back to heuristic when hook_event_name absent (non-Gemini fallback now silent per #2289)', () => { + test('absent hook_event_name (non-Gemini) is now silent (#2289)', () => { + // #2289: a missing hook_event_name without GEMINI_API_KEY set used to fall + // back to "PostToolUse" and emit. It is now a non-injection case → silent, + // since we cannot positively confirm this is a context-injection-capable + // invocation without either an allowlisted event name or the Gemini signal. const env = { ...process.env }; delete env.GEMINI_API_KEY; const out = runMonitor({ @@ -880,15 +887,12 @@ describe('bug #925: context monitor falls back to heuristic when hook_event_name sessionId: makeSessionId('fallback-non-gemini'), env: { GEMINI_API_KEY: '' }, // ensure unset }); - assert.ok(out, 'hook must emit output when context is below WARNING threshold'); - assert.strictEqual( - out.hookSpecificOutput?.hookEventName, - 'PostToolUse', - `Expected fallback "PostToolUse" for non-Gemini but got "${out.hookSpecificOutput?.hookEventName}".` - ); + assert.strictEqual(out, null, 'absent hook_event_name (non-Gemini) is now silent (#2289)'); }); test('falls back to "AfterTool" when hook_event_name is absent and GEMINI_API_KEY is set', () => { + // Unchanged by #2289: this is the Gemini fallback, which remains an + // explicit allowlisted emitting path. const out = runMonitor({ hookEventName: undefined, sessionId: makeSessionId('fallback-gemini'), @@ -902,57 +906,44 @@ describe('bug #925: context monitor falls back to heuristic when hook_event_name ); }); - test('falls back to "PostToolUse" when hook_event_name is an empty string (non-Gemini)', () => { + test('empty-string hook_event_name (non-Gemini) is now silent (#2289)', () => { + // #2289: an empty hook_event_name without GEMINI_API_KEY is treated the + // same as absent — non-injection case → silent. const out = runMonitor({ hookEventName: '', sessionId: makeSessionId('fallback-empty'), env: { GEMINI_API_KEY: '' }, }); - assert.ok(out, 'hook must emit output when context is below WARNING threshold'); - assert.strictEqual( - out.hookSpecificOutput?.hookEventName, - 'PostToolUse', - `Expected fallback "PostToolUse" for empty hook_event_name but got "${out.hookSpecificOutput?.hookEventName}".` - ); + assert.strictEqual(out, null, 'empty-string hook_event_name (non-Gemini) is now silent (#2289)'); }); - test('falls back to "PostToolUse" when hook_event_name is whitespace-only (non-Gemini)', () => { - // trim() makes " " → "" which is falsy, so the || fallback fires + test('whitespace-only hook_event_name (non-Gemini) is now silent (#2289)', () => { + // trim() makes " " → "" which is falsy; #2289: this now takes the + // non-injection silent path rather than falling back to "PostToolUse". const out = runMonitor({ hookEventName: ' ', sessionId: makeSessionId('fallback-whitespace'), env: { GEMINI_API_KEY: '' }, }); - assert.ok(out, 'hook must emit output when context is below WARNING threshold'); - assert.strictEqual( - out.hookSpecificOutput?.hookEventName, - 'PostToolUse', - `Expected fallback "PostToolUse" for whitespace-only hook_event_name but got "${out.hookSpecificOutput?.hookEventName}".` - ); + assert.strictEqual(out, null, 'whitespace-only hook_event_name (non-Gemini) is now silent (#2289)'); }); }); // ─── Critical threshold also echoes the event name ─────────────────────────── describe('bug #925: critical threshold warning also uses correct hookEventName', () => { - test('CRITICAL warning emitted under Stop also echoes "Stop"', () => { + test('CRITICAL under Stop is silent (Codex rejects the Stop envelope) (#2289)', () => { + // #2289: even at CRITICAL severity, Stop is a non-injection event whose + // schema (Codex) rejects the hookSpecificOutput envelope outright. The + // hook must emit nothing rather than echo "Stop", superseding bug #925's + // "echoes Stop" expectation for this event specifically. const out = runMonitor({ hookEventName: 'Stop', sessionId: makeSessionId('critical-stop'), remainingPct: 20, usedPct: 80, }); - assert.ok(out, 'hook must emit output at critical threshold (remaining=20)'); - assert.strictEqual( - out.hookSpecificOutput?.hookEventName, - 'Stop', - `Expected hookEventName "Stop" at critical threshold, got "${out.hookSpecificOutput?.hookEventName}".` - ); - assert.match( - out.hookSpecificOutput?.additionalContext || '', - /CONTEXT CRITICAL/, - 'Output should be a CRITICAL warning at remaining=20' - ); + assert.strictEqual(out, null, 'CRITICAL under Stop must be silent — no envelope for a non-injection event (#2289)'); }); }); });