From 4257e8170db799f7ebd26723e8cc8bc5c002c4bb Mon Sep 17 00:00:00 2001 From: Zachary Townsend Date: Sun, 31 May 2026 10:05:17 -0700 Subject: [PATCH] fix(plan-tune): stop emitting invalid permissionDecision "defer" (breaks Conductor AUQ) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PreToolUse question-preference-hook emitted `permissionDecision: "defer"` on its pass-through path. `"defer"` is not a valid Claude Code permissionDecision — the spec defines only `allow` / `deny` / `ask`. Native Claude Code silently ignored the unknown value and fell through to normal flow, so it appeared to work. Conductor's `mcp__conductor__AskUserQuestion` bridge does NOT ignore it: an unrecognized permissionDecision on its own injected tool hangs the round-trip, so the question never renders and no tool_result is returned (the harness substitutes "[Tool result missing due to internal error]"). Because defer() fires on every ordinary question with no never-ask enforcement, this broke AskUserQuestion entirely for Conductor users whenever the gstack plan-tune hooks were installed. Fix: express "no opinion" the spec-correct way — emit no permissionDecision. Emit nothing at all when there is no additionalContext; surface additionalContext (Layer 8 memory) alone otherwise. The deny enforcement path is unchanged (deny is spec-valid). - Update the defer() contract + tests (defer => no permissionDecision). - Add a Conductor regression test: ordinary AUQ question => empty stdout. - Correct docs/spikes/claude-code-hook-mutation.md, which incorrectly documented "defer" as a valid permissionDecision value. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/spikes/claude-code-hook-mutation.md | 23 +++++++++-- .../claude/hooks/question-preference-hook.ts | 34 +++++++++++++--- test/memory-cache-injection.test.ts | 6 +-- test/question-preference-hook.test.ts | 40 ++++++++++++++----- test/skill-e2e-plan-tune-cathedral.test.ts | 2 +- 5 files changed, 83 insertions(+), 22 deletions(-) diff --git a/docs/spikes/claude-code-hook-mutation.md b/docs/spikes/claude-code-hook-mutation.md index 70a4ae18a..0db0b11b3 100644 --- a/docs/spikes/claude-code-hook-mutation.md +++ b/docs/spikes/claude-code-hook-mutation.md @@ -51,7 +51,18 @@ 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 + +> ⚠️ **There is no `"defer"` permissionDecision value.** The spec defines only +> `allow` / `deny` / `ask`. To express "no opinion — let the tool run and +> permission flow continue", emit **no `permissionDecision`** (an empty +> `hookSpecificOutput`, or no stdout at all). Earlier revisions of this spike +> and the hook emitted `permissionDecision: "defer"`; native Claude Code +> silently ignored the unknown value and fell through, so it appeared to work. +> **Conductor's `mcp__conductor__AskUserQuestion` bridge does not ignore it** — +> an unrecognized permissionDecision on its own injected tool hangs the +> round-trip, so the question never renders and no `tool_result` is returned. +> Always use the no-permissionDecision shape for pass-through. See the +> pass-through example below. **`updatedInput` semantics:** shallow merge of fields present in the returned object onto the original `tool_input`. Only valid with @@ -88,7 +99,8 @@ required for our hook to fire there. accepting. **`permissionDecision` precedence (when multiple hooks decide):** -`deny > ask > allow > defer` — most restrictive wins. +`deny > ask > allow > (no decision)` — most restrictive wins; a hook that +emits no `permissionDecision` defers to whatever the others decide. ## Implementation hookSpecificOutput examples @@ -107,11 +119,16 @@ required for our hook to fire there. ``` **Pass-through (no preference, or one-way safety override):** + +Emit **no `permissionDecision`** — surface `additionalContext` if there is any, +otherwise emit nothing at all (empty stdout + exit 0). Do NOT emit +`permissionDecision: "defer"` (not a real value; breaks Conductor's AUQ bridge). + ```json { "hookSpecificOutput": { "hookEventName": "PreToolUse", - "permissionDecision": "defer" + "additionalContext": "optional plan-tune memory context" } } ``` diff --git a/hosts/claude/hooks/question-preference-hook.ts b/hosts/claude/hooks/question-preference-hook.ts index dde1bda0c..0f1c1947d 100644 --- a/hosts/claude/hooks/question-preference-hook.ts +++ b/hosts/claude/hooks/question-preference-hook.ts @@ -92,12 +92,34 @@ 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 })); + // "Defer" means "no permission opinion — let the tool run and the question + // render normally." The Claude Code hook spec only defines the + // permissionDecision values "allow" | "deny" | "ask"; there is no "defer". + // Native Claude Code silently ignored the bogus value and fell through to + // normal flow, so emitting it appeared to work. Conductor's + // mcp__conductor__AskUserQuestion bridge does NOT ignore it: an + // unrecognized permissionDecision on its own injected tool hangs the + // round-trip, so the question never renders and no tool_result is ever + // returned (the harness then substitutes "[Tool result missing due to + // internal error]"). Since defer() fires on every ordinary question that + // has no never-ask enforcement, this broke AskUserQuestion entirely under + // Conductor. + // + // Express "no opinion" the spec-correct way: emit NO permissionDecision. + // When there is no additionalContext to surface, emit nothing at all + // (empty stdout + exit 0 is the canonical "no decision" hook response). + // When we do have Layer 8 memory context, surface it via additionalContext + // alone — still with no permissionDecision. + if (additionalContext) { + process.stdout.write( + JSON.stringify({ + hookSpecificOutput: { + hookEventName: 'PreToolUse', + additionalContext, + }, + }), + ); + } process.exit(0); } diff --git a/test/memory-cache-injection.test.ts b/test/memory-cache-injection.test.ts index 3330f8d2a..6159032cf 100644 --- a/test/memory-cache-injection.test.ts +++ b/test/memory-cache-injection.test.ts @@ -86,7 +86,7 @@ describe('memory injection', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); expect(r.parsed?.hookSpecificOutput?.additionalContext).toContain('verbose explanations'); }); @@ -110,7 +110,7 @@ describe('memory injection', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); expect(r.parsed?.hookSpecificOutput?.additionalContext).toBeUndefined(); }); @@ -214,7 +214,7 @@ describe('per-session memory cache', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); expect(r.parsed?.hookSpecificOutput?.additionalContext).toBeUndefined(); }); }); diff --git a/test/question-preference-hook.test.ts b/test/question-preference-hook.test.ts index 6b06d22f4..55b395aae 100644 --- a/test/question-preference-hook.test.ts +++ b/test/question-preference-hook.test.ts @@ -118,7 +118,7 @@ describe('defers (no enforcement)', () => { }, }); expect(r.status).toBe(0); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); test('marker missing → defer (D18)', () => { @@ -133,7 +133,7 @@ describe('defers (no enforcement)', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); test('always-ask preference → defer', () => { @@ -148,7 +148,7 @@ describe('defers (no enforcement)', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); test('empty stdin → defer (crash safety)', () => { @@ -160,13 +160,35 @@ describe('defers (no enforcement)', () => { 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(parsed.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); test('non-AUQ tool_name → defer (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'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); + }); + + // Regression: the defer path must NOT emit any permissionDecision. The Claude + // Code spec only defines allow/deny/ask; the old code emitted a bogus + // "defer" value, which native Claude Code ignored but Conductor's + // mcp__conductor__AskUserQuestion bridge could not handle — it hung the + // round-trip so the question never rendered and no tool_result came back. + // A plain ordinary question (no marker) must therefore produce empty stdout. + test('ordinary question (no marker) → empty stdout, no permissionDecision (Conductor AUQ bridge regression)', () => { + const r = runHook({ + session_id: 's-conductor', + tool_name: 'mcp__conductor__AskUserQuestion', + tool_use_id: 'tu-conductor', + tool_input: { + questions: [ + { question: 'Which option do you prefer?', options: ['A) One', 'B) Two'] }, + ], + }, + }); + expect(r.status).toBe(0); + expect(r.stdout).toBe(''); + expect(r.stdout).not.toContain('permissionDecision'); }); }); @@ -211,7 +233,7 @@ describe('enforces never-ask preferences', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); test('ambiguous recommendation (two labels) → defer (D2 refuse-on-ambiguous)', () => { @@ -229,7 +251,7 @@ describe('enforces never-ask preferences', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); test('no recommendation marker AND no prose match → defer', () => { @@ -247,7 +269,7 @@ describe('enforces never-ask preferences', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); }); @@ -309,7 +331,7 @@ describe('precedence: project wins over global (D8)', () => { ], }, }); - expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer'); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined(); }); }); diff --git a/test/skill-e2e-plan-tune-cathedral.test.ts b/test/skill-e2e-plan-tune-cathedral.test.ts index f9c006914..e07eb70fa 100644 --- a/test/skill-e2e-plan-tune-cathedral.test.ts +++ b/test/skill-e2e-plan-tune-cathedral.test.ts @@ -296,7 +296,7 @@ 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'); + expect(parsed.hookSpecificOutput?.permissionDecision).toBeUndefined(); expect(parsed.hookSpecificOutput?.additionalContext).toContain('verbose explanations'); }); });