Merge 4257e8170d into c43c850cae

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"
   }
 }
 ```

hosts/claude/hooks/question-preference-hook.ts

@@ -92,12 +92,34 @@ function readStdin(): Promise<string> {
 }
 
 function defer(additionalContext?: string): void {
-  const out: Record<string, unknown> = {
-    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);
 }
 

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();
   });
 });

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();
   });
 });
 

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');
   });
 });