From 3d97863b1432a6e885fbf649781788106223c954 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 9 Jul 2026 19:08:49 -0700 Subject: [PATCH] =?UTF-8?q?fix(hooks):=20passThrough()=20two-branch=20cont?= =?UTF-8?q?ract=20=E2=80=94=20never=20emit=20permissionDecision:'defer'=20?= =?UTF-8?q?(#2035,=20#2006)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every AskUserQuestion died with "Tool result missing due to internal error" on current Claude Code builds (Desktop 1.14271.0, CC 2.1.177). Root cause: the question-preference-hook emitted permissionDecision:'defer' on every pass-through path. 'defer' is a real PreToolUse value, but since CC v2.1.89 its semantics are "pause this tool call for external resumption" (headless resume) — never "abstain". Interactive sessions have nothing to resume the paused call, so the tool orphaned. Pre-2.1.89 builds ignored the unknown value, which is why the hook worked when it shipped and broke later. The fix is the two-branch pass-through contract: - no context -> exit 0 with EXACTLY empty stdout - memory nuggets present -> hookSpecificOutput with hookEventName + additionalContext ONLY (the documented shape; plan-tune Layer 8 memory injection ships through this branch and keeps working) defer() is renamed passThrough() so the function says what it does, and docs/spikes/claude-code-hook-mutation.md's protocol contract (cited by the hook header) is corrected in the same commit — it taught '"defer" — let permission flow continue' and was the reintroduction vector. Test contract rewritten in the same commit (13 assertions across 3 files, verified fail-first against the unfixed hook): pass-through paths assert exact-empty stdout (a garbage/partial write cannot slip past an optional-chained parse), the nugget path asserts permissionDecision is ABSENT while additionalContext survives, and a new tripwire asserts no non-deny path ever puts the string "permissionDecision" on stdout. The deny (auto-decide) and Conductor prose-redirect paths are unchanged. Deployment: no migration needed — settings.json points at the absolute bash shim which execs the .ts live; /gstack-upgrade delivers the fix. Co-Authored-By: Claude Fable 5 --- docs/spikes/claude-code-hook-mutation.md | 17 +++- .../claude/hooks/question-preference-hook.ts | 54 ++++++---- test/memory-cache-injection.test.ts | 23 +++-- test/question-preference-hook.test.ts | 99 +++++++++++++------ test/skill-e2e-plan-tune-cathedral.test.ts | 5 +- 5 files changed, 137 insertions(+), 61 deletions(-) diff --git a/docs/spikes/claude-code-hook-mutation.md b/docs/spikes/claude-code-hook-mutation.md index 9d91e16cd..6dde72d87 100644 --- a/docs/spikes/claude-code-hook-mutation.md +++ b/docs/spikes/claude-code-hook-mutation.md @@ -51,7 +51,13 @@ Optional in subagent context: `agent_id`, `agent_type`. - `"deny"` — block (feedback to Claude, NOT a synthetic answer per Codex correction in D-prefixed decisions) - `"ask"` — escalate to user -- `"defer"` — let permission flow continue +- `"defer"` — pause the tool call for EXTERNAL resumption (Claude Code + v2.1.89+, a headless feature: resume with `-p --resume` to re-evaluate). + NEVER emit this to mean "no opinion" — in an interactive session nothing + resumes the paused call and the tool dies with "Tool result missing due to + internal error" (#2035, #2006). To abstain, exit 0 with EMPTY stdout + (optionally `hookSpecificOutput` with `additionalContext` only, no + `permissionDecision`). **`updatedInput` semantics:** shallow merge of fields present in the returned object onto the original `tool_input`. Only valid with @@ -106,15 +112,20 @@ required for our hook to fire there. } ``` -**Pass-through (no preference, or one-way safety override):** +**Pass-through (no preference, or one-way safety override):** exit 0 with +EMPTY stdout. When there is context to inject (plan-tune memory nuggets), +emit `additionalContext` WITHOUT a `permissionDecision`: ```json { "hookSpecificOutput": { "hookEventName": "PreToolUse", - "permissionDecision": "defer" + "additionalContext": "[plan-tune memory] Past answers suggest: ..." } } ``` +(Historical note: this example originally emitted `permissionDecision: +"defer"`, which broke every AskUserQuestion once CC v2.1.89 gave 'defer' +pause-for-resume semantics — #2035.) **PostToolUse capture (always):** ```json diff --git a/hosts/claude/hooks/question-preference-hook.ts b/hosts/claude/hooks/question-preference-hook.ts index 12cbd5ea2..559102fda 100644 --- a/hosts/claude/hooks/question-preference-hook.ts +++ b/hosts/claude/hooks/question-preference-hook.ts @@ -12,11 +12,15 @@ * 2. Look up door_type from scripts/question-registry.ts (default two-way). * 3. Read preferences with precedence: project-local > global (D8). * 4. Apply: - * never-ask + one-way → defer (safety override; one-way always asks). + * never-ask + one-way → pass through (safety override; one-way always asks). * never-ask + two-way + marker → deny with auto-decided recommendation * in reason. Mark tool_use_id so PostToolUse logs as 'auto-decided'. * ask-only-for-one-way + two-way + marker → same as never-ask. - * always-ask, or no preference → defer. + * always-ask, or no preference → pass through. + * + * Pass-through = exit 0 with empty stdout (or additionalContext-only output + * when memory nuggets exist) — NEVER permissionDecision:'defer', whose + * CC v2.1.89+ semantics are pause-for-external-resumption (#2035, #2006). * * Why deny+reason instead of allow+updatedInput: * AskUserQuestion's `updatedInput` shape for "pre-resolve this question" @@ -31,7 +35,7 @@ * - First: (recommended) label suffix on an option. * - Fall back: "Recommendation: X" prose match against option labels. * - Refuse to auto-decide if ambiguous (multiple labels OR no parseable - * recommendation): defer instead of silent-wrong. + * recommendation): pass through instead of silent-wrong. * * Always exits 0. Hook errors land in ~/.gstack/hook-errors.log. * See docs/spikes/claude-code-hook-mutation.md for the protocol contract. @@ -92,13 +96,25 @@ function readStdin(): Promise { }); } -function defer(additionalContext?: string): void { - const out: Record = { - hookEventName: 'PreToolUse', - permissionDecision: 'defer', - }; - if (additionalContext) out.additionalContext = additionalContext; - process.stdout.write(JSON.stringify({ hookSpecificOutput: out })); +function passThrough(additionalContext?: string): void { + // Abstain = exit 0 with EMPTY stdout (#2035, #2006). Never emit a + // permissionDecision here: 'defer' is a real PreToolUse value, but since + // Claude Code v2.1.89 its semantics are "pause this tool call for external + // resumption" (a headless-resume feature) — NOT "no opinion". In an + // interactive session nothing resumes the paused call, so every + // AskUserQuestion died with "Tool result missing due to internal error". + // additionalContext-only hookSpecificOutput is the documented shape for + // injecting context (plan-tune memory nuggets) without a decision. + if (additionalContext) { + process.stdout.write( + JSON.stringify({ + hookSpecificOutput: { + hookEventName: 'PreToolUse', + additionalContext, + }, + }), + ); + } process.exit(0); } @@ -347,7 +363,7 @@ function logAutoDecided( async function main(): Promise { const raw = await readStdin(); if (!raw.trim()) { - defer(); + passThrough(); return; } let stdin: HookStdin; @@ -355,7 +371,7 @@ async function main(): Promise { stdin = JSON.parse(raw); } catch (e) { logHookError(`stdin parse failed: ${(e as Error).message}`); - defer(); + passThrough(); return; } @@ -364,26 +380,26 @@ async function main(): Promise { toolName !== 'AskUserQuestion' && !toolName.match(/^mcp__.+__AskUserQuestion$/) ) { - defer(); + passThrough(); return; } const questions = stdin.tool_input?.questions || []; if (questions.length === 0) { - defer(); + passThrough(); return; } // For multi-question AUQ, enforcement is all-or-nothing per call: // we deny only if ALL questions have marker + never-ask + safe door type. - // Mixed cases pass through (defer) so the user still gets to answer. + // Mixed cases pass through so the user still gets to answer. const registry = loadRegistry(); const slug = slugFromCwd(stdin.cwd); const memoryNuggets = loadMemoryNuggets(stdin.session_id); // Compute Layer 8 memory context inline: any nuggets matching the // signal_keys of the questions in this AUQ get surfaced as additionalContext. - // This applies whether we defer OR deny — gives the agent + user the + // This applies whether we pass through OR deny — gives the agent + user the // relevant prior context either way. const contextNuggets: string[] = []; for (const q of questions) { @@ -402,7 +418,7 @@ async function main(): Promise { : undefined; // Determine whether EVERY question is eligible for never-ask auto-decide. - // We deliberately do NOT early-return defer on the first ineligible question: + // We deliberately do NOT early-return pass-through on the first ineligible question: // a Conductor session still needs the [conductor] prose deny as a fallback, // so we compute eligibility, then branch. memoryContext is preserved on every // non-enforcing exit. (All-or-nothing per-call semantics are unchanged: any @@ -471,10 +487,10 @@ async function main(): Promise { return; } - defer(memoryContext); + passThrough(memoryContext); } main().catch((e) => { logHookError(`main crash: ${(e as Error).message}`); - defer(); + passThrough(); }); diff --git a/test/memory-cache-injection.test.ts b/test/memory-cache-injection.test.ts index 3ab6a2144..991a4c019 100644 --- a/test/memory-cache-injection.test.ts +++ b/test/memory-cache-injection.test.ts @@ -2,7 +2,7 @@ * Layer 8 memory cache + injection (plan-tune cathedral T12). * * Verifies the PreToolUse hook reads ~/.gstack/free-text-memory.json and - * surfaces matching nuggets via additionalContext on the hook response. + * surfaces matching nuggets via additionalContext-only output (#2035: never a permissionDecision). * Cache: per-session memory-cache.json populated on first read, sub-1ms * thereafter (D13 perf). */ @@ -43,9 +43,9 @@ function runHook(stdin: object): { stdout: string; stderr: string; status: numbe env.GSTACK_STATE_ROOT = stateRoot; env.GSTACK_QUESTION_LOG_NO_DERIVE = '1'; delete env.GSTACK_HOME; - // These cases assert the defer-path memoryContext injection. Strip ambient + // These cases assert the pass-through memoryContext injection. Strip ambient // Conductor markers so running inside Conductor (CONDUCTOR_WORKSPACE_PATH/PORT - // set) doesn't flip the hook into the [conductor] prose deny instead of defer. + // set) doesn't flip the hook into the [conductor] prose deny instead of pass-through. delete env.CONDUCTOR_WORKSPACE_PATH; delete env.CONDUCTOR_PORT; const res = spawnSync(HOOK, [], { @@ -69,7 +69,7 @@ function runHook(stdin: object): { stdout: string; stderr: string; status: numbe // ---------------------------------------------------------------------- describe('memory injection', () => { - test('injects matching nugget into additionalContext on defer', () => { + test('injects matching nugget into additionalContext on pass-through', () => { writeMemory([ { nugget: 'User prefers verbose explanations with tradeoffs', @@ -91,7 +91,10 @@ describe('memory injection', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + // #2035: nugget delivery is additionalContext-ONLY — a permissionDecision + // here (any value) would orphan the tool call on CC >= 2.1.89. + expect('permissionDecision' in (r.parsed?.hookSpecificOutput ?? {})).toBe(false); + expect(r.parsed?.hookSpecificOutput?.hookEventName).toBe('PreToolUse'); expect(r.parsed?.hookSpecificOutput?.additionalContext).toContain('verbose explanations'); }); @@ -115,8 +118,9 @@ describe('memory injection', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); - expect(r.parsed?.hookSpecificOutput?.additionalContext).toBeUndefined(); + // No nugget → pure pass-through: exit 0 with EXACTLY empty stdout. + expect(r.status).toBe(0); + expect(r.stdout).toBe(''); }); test('caps to 3 most-recent nuggets when many match', () => { @@ -219,7 +223,8 @@ describe('per-session memory cache', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); - expect(r.parsed?.hookSpecificOutput?.additionalContext).toBeUndefined(); + // No nugget → pure pass-through: exit 0 with EXACTLY empty stdout. + expect(r.status).toBe(0); + expect(r.stdout).toBe(''); }); }); diff --git a/test/question-preference-hook.test.ts b/test/question-preference-hook.test.ts index 39de02f4e..da0184f9d 100644 --- a/test/question-preference-hook.test.ts +++ b/test/question-preference-hook.test.ts @@ -3,15 +3,18 @@ * * Covers: * - never-ask + marker + two-way + clean recommendation → deny+reason - * - never-ask + no marker → defer (D18 marker gate) - * - never-ask + one-way → defer (safety override) - * - never-ask + ambiguous recommendation → defer (D2 refuse-on-ambiguous) - * - always-ask → defer - * - no preference → defer + * - never-ask + no marker → pass-through (D18 marker gate) + * - never-ask + one-way → pass-through (safety override) + * - never-ask + ambiguous recommendation → pass-through (D2 refuse-on-ambiguous) + * - always-ask → pass-through + * - no preference → pass-through * - project preference wins over global (D8 precedence) * - global preference applies when no project preference set * - mcp__*__AskUserQuestion matcher accepted - * - empty stdin → defer (crash safety) + * - empty stdin → pass-through (crash safety) + * + * Pass-through contract (#2035/#2006): exit 0 + EXACTLY empty stdout, or + * additionalContext-only hookSpecificOutput — never a permissionDecision. * - auto-decided event logged via gstack-question-log (PostToolUse won't fire) * - auto-decided marker written to ~/.gstack/sessions//.auto-decided- */ @@ -97,6 +100,20 @@ function runHook(stdin: object, cwd?: string, extraEnv?: Record) }; } +/** + * #2035/#2006 contract: pass-through (abstain) is exit 0 with EXACTLY empty + * stdout — never a permissionDecision. 'defer' is a real PreToolUse value, + * but its semantics are pause-for-external-resumption (CC v2.1.89+), so + * emitting it orphans the tool call in interactive sessions. Exact-empty + * (not trim) is deliberate: whitespace on stdout is still hook output, and a + * garbage/partial write must fail this assertion rather than slip past an + * optional-chained parse. + */ +function expectPassThrough(r: { status: number; stdout: string }): void { + expect(r.status).toBe(0); + expect(r.stdout).toBe(''); +} + function autoDecidedEvents(): Array> { const f = path.join(stateRoot, 'projects', cwdSlug, 'question-log.jsonl'); if (!fs.existsSync(f)) return []; @@ -113,8 +130,8 @@ function autoDecidedEvents(): Array> { // Defer paths // ---------------------------------------------------------------------- -describe('defers (no enforcement)', () => { - test('no preference set → defer', () => { +describe('passes through (no enforcement)', () => { + test('no preference set → pass-through (empty stdout, no permissionDecision)', () => { const r = runHook({ session_id: 's1', tool_name: 'AskUserQuestion', @@ -125,11 +142,10 @@ describe('defers (no enforcement)', () => { ], }, }); - expect(r.status).toBe(0); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expectPassThrough(r); }); - test('marker missing → defer (D18)', () => { + test('marker missing → pass-through (D18)', () => { writeProjectPref('test-q', 'never-ask'); const r = runHook({ session_id: 's2', @@ -141,10 +157,10 @@ describe('defers (no enforcement)', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expectPassThrough(r); }); - test('always-ask preference → defer', () => { + test('always-ask preference → pass-through', () => { writeProjectPref('test-q', 'always-ask'); const r = runHook({ session_id: 's3', @@ -156,10 +172,10 @@ describe('defers (no enforcement)', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expectPassThrough(r); }); - test('empty stdin → defer (crash safety)', () => { + test('empty stdin → pass-through (crash safety)', () => { const env: Record = {}; for (const [k, v] of Object.entries(process.env)) { if (v !== undefined) env[k] = v; @@ -167,14 +183,39 @@ describe('defers (no enforcement)', () => { env.GSTACK_STATE_ROOT = stateRoot; const res = spawnSync(HOOK, [], { env, input: '', encoding: 'utf-8' }); expect(res.status).toBe(0); - const parsed = JSON.parse(res.stdout || '{}'); - expect(parsed.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(res.stdout).toBe(''); }); - test('non-AUQ tool_name → defer (defensive)', () => { + test('non-AUQ tool_name → pass-through (defensive)', () => { writeProjectPref('test-q', 'never-ask'); const r = runHook({ session_id: 's4', tool_name: 'Bash', tool_use_id: 'tu-4', tool_input: {} }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expectPassThrough(r); + }); + + // #2035 tripwire: no non-deny/non-allow path may EVER put the string + // "permissionDecision" on stdout. Emitting one on a pass-through path (any + // value — 'defer' included) hands the platform a decision where the hook + // has none, and 'defer' specifically pauses the call for a resumption that + // never comes in interactive sessions. + test('pass-through stdout never contains "permissionDecision" (#2035)', () => { + const paths = [ + runHook({ + session_id: 's-trip-1', + tool_name: 'AskUserQuestion', + tool_use_id: 'tu-trip-1', + tool_input: { + questions: [ + { question: ' Approve?', options: ['A) Yes (recommended)', 'B) No'] }, + ], + }, + }), + runHook({ session_id: 's-trip-2', tool_name: 'Bash', tool_use_id: 'tu-trip-2', tool_input: {} }), + runHook({ session_id: 's-trip-3', tool_name: 'AskUserQuestion', tool_use_id: 'tu-trip-3', tool_input: { questions: [] } }), + ]; + for (const r of paths) { + expect(r.status).toBe(0); + expect(r.stdout).not.toContain('"permissionDecision"'); + } }); }); @@ -204,7 +245,7 @@ describe('enforces never-ask preferences', () => { expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toContain('Fix now'); }); - test('one-way door → defer even with never-ask (safety override)', () => { + test('one-way door → pass-through even with never-ask (safety override)', () => { writeProjectPref('ship-test-failure-triage', 'never-ask'); const r = runHook({ session_id: 's6', @@ -219,10 +260,10 @@ describe('enforces never-ask preferences', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expectPassThrough(r); }); - test('ambiguous recommendation (two labels) → defer (D2 refuse-on-ambiguous)', () => { + test('ambiguous recommendation (two labels) → pass-through (D2 refuse-on-ambiguous)', () => { writeProjectPref('ship-pre-landing-review-fix', 'never-ask'); const r = runHook({ session_id: 's7', @@ -237,10 +278,10 @@ describe('enforces never-ask preferences', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expectPassThrough(r); }); - test('no recommendation marker AND no prose match → defer', () => { + test('no recommendation marker AND no prose match → pass-through', () => { writeProjectPref('ship-pre-landing-review-fix', 'never-ask'); const r = runHook({ session_id: 's8', @@ -255,7 +296,7 @@ describe('enforces never-ask preferences', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expectPassThrough(r); }); }); @@ -301,7 +342,7 @@ describe('precedence: project wins over global (D8)', () => { expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('deny'); }); - test('project always-ask + global never-ask → defer (project wins)', () => { + test('project always-ask + global never-ask → pass-through (project wins)', () => { writeProjectPref('ship-pre-landing-review-fix', 'always-ask'); writeGlobalPref('ship-pre-landing-review-fix', 'never-ask'); const r = runHook({ @@ -317,7 +358,7 @@ describe('precedence: project wins over global (D8)', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expectPassThrough(r); }); }); @@ -437,13 +478,13 @@ describe('Conductor prose redirect', () => { expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).not.toContain('[conductor]'); }); - test('non-AUQ tool in Conductor → still defer (no redirect on unrelated tools)', () => { + test('non-AUQ tool in Conductor → still pass-through (no redirect on unrelated tools)', () => { const r = runHook( { session_id: 'c6', tool_name: 'Bash', tool_use_id: 'tu-c6', tool_input: {} }, undefined, CONDUCTOR, ); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expectPassThrough(r); }); }); diff --git a/test/skill-e2e-plan-tune-cathedral.test.ts b/test/skill-e2e-plan-tune-cathedral.test.ts index f9c006914..85a455036 100644 --- a/test/skill-e2e-plan-tune-cathedral.test.ts +++ b/test/skill-e2e-plan-tune-cathedral.test.ts @@ -296,7 +296,10 @@ describeIfSelected('PlanTune cathedral E2E: annotation', ['plan-tune-annotation' }); expect(res.status).toBe(0); const parsed = JSON.parse(res.stdout || '{}'); - expect(parsed.hookSpecificOutput?.permissionDecision).toBe('defer'); + // #2035: memory-nugget delivery is additionalContext-ONLY. Emitting a + // permissionDecision here (the old 'defer') pauses the tool call for a + // resumption that never comes in interactive sessions. + expect('permissionDecision' in (parsed.hookSpecificOutput ?? {})).toBe(false); expect(parsed.hookSpecificOutput?.additionalContext).toContain('verbose explanations'); }); });