fix(hooks): question-preference-hook must not emit permissionDecision 'defer' (merges #2165 + #1924)

docs/spikes/claude-code-hook-mutation.md

@@ -1,6 +1,8 @@
 # Spike: Claude Code hook mutation for plan-tune cathedral
 
 **Status:** complete (2026-05-27)
+**Revised:** 2026-07-22 — pass-through is empty stdout (not `defer`); restored
+decision-precedence facts alongside defer semantics.
 **Surfaces:** D10 (does PreToolUse allow mutating AUQ input?), D19/Codex (matcher must cover MCP variants)
 **Downstream consumers:** T3, T5, T6, T8
 
@@ -11,8 +13,9 @@ answer via `updatedInput`? If yes, what's the exact protocol?
 
 ## Answer
 
-**Yes.** `updatedInput` is the supported mechanism. Source:
-https://code.claude.com/docs/en/hooks (confirmed 2026-04 reference).
+**Yes.** `updatedInput` is the supported mechanism (platform capability;
+this plan-tune hook does not take that path — see Implementation examples).
+Source: https://code.claude.com/docs/en/hooks (confirmed 2026-04 reference).
 
 ## Hook stdin schema (PreToolUse + PostToolUse)
 
@@ -51,12 +54,16 @@ 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 a non-interactive SDK/tool caller so it can resume later
+
+No output (exit 0, empty stdout) is not a `permissionDecision` value. It is
+the pass-through path that lets the permission flow continue unchanged.
 
 **`updatedInput` semantics:** shallow merge of fields present in the returned
 object onto the original `tool_input`. Only valid with
-`permissionDecision: "allow"`. This is what lets us substitute an
-auto-decided answer for `never-ask` preferences.
+`permissionDecision: "allow"`. This is what would let a hook substitute an
+auto-decided answer for `never-ask` preferences; the live plan-tune hook
+instead uses `deny` + reason (see Implementation examples).
 
 ## Matcher schema
 
@@ -88,30 +95,36 @@ required for our hook to fire there.
   accepting.
 
 **`permissionDecision` precedence (when multiple hooks decide):**
-`deny > ask > allow > defer` — most restrictive wins.
+`deny > ask > allow` — most restrictive wins.
+Claude Code treats `defer` as an explicit pause/resume decision, not as the
+normal pass-through path. Hooks that do not need to decide should exit 0 with
+no output (empty stdout), not emit `permissionDecision: "defer"`.
 
 ## Implementation hookSpecificOutput examples
 
 **Auto-decide (PreToolUse, `never-ask` preference + non-one-way):**
+
+The live hook uses `deny` + reason (not `allow` + `updatedInput`). See the
+hook header: AskUserQuestion's pre-resolve `updatedInput` shape is not
+structurally pinned, so deny naming the recommended option is the reliable
+path; the model reads the reason and proceeds without re-firing AUQ.
 ```json
 {
   "hookSpecificOutput": {
     "hookEventName": "PreToolUse",
-    "permissionDecision": "allow",
-    "permissionDecisionReason": "plan-tune: never-ask preference on ship-test-failure-triage",
-    "updatedInput": {
-      "questions": [{ /* same as input, but with auto-selected answer */ }]
-    }
+    "permissionDecision": "deny",
+    "permissionDecisionReason": "[plan-tune auto-decide] ship-pre-landing-review-fix → A) Fix now (recommended) (your never-ask preference). Proceed with that option without re-prompting. Change with /plan-tune."
   }
 }
 ```
 
 **Pass-through (no preference, or one-way safety override):**
+Exit 0 with no stdout. If the hook has context to add, omit `permissionDecision`:
 ```json
 {
   "hookSpecificOutput": {
     "hookEventName": "PreToolUse",
-    "permissionDecision": "defer"
+    "additionalContext": "optional context for Claude"
   }
 }
 ```
@@ -206,12 +219,11 @@ shells into bun.
    examples: ship/SKILL.md.tmpl emits options like `"A) Fix now"
    (recommended)`.
 
-2. **Auto-decided event tagging.** When hook returns `updatedInput`, the
-   PostToolUse hook will see the resolved input and log a normal event.
-   Need an extra field on the PostToolUse payload (e.g.,
-   `was_auto_decided: true`) that the hook can set via session state
-   tracking — write a marker file in `~/.gstack/sessions/<id>/.auto-decided-<tool_use_id>`
-   from PreToolUse, read it from PostToolUse, delete on read.
+2. **Auto-decided event tagging.** Auto-decide is `deny` + reason, so
+   PostToolUse never fires on that tool call. The PreToolUse hook itself
+   writes a session marker and logs `source=auto-decided` events before
+   deny (see `markAutoDecided` / `logAutoDecided` in the hook). No
+   PostToolUse payload field is required for the current path.
 
 3. **Timeout behavior.** Default hook timeout is 60s but the docs are
    thin on what happens at timeout. Set explicit `timeout: 5` so the

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

@@ -12,11 +12,11 @@
  *   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 → no decision (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 → no decision.
  *
  * Why deny+reason instead of allow+updatedInput:
  *   AskUserQuestion's `updatedInput` shape for "pre-resolve this question"
@@ -31,7 +31,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): no decision 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 +92,24 @@ 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 }));
+function passThrough(additionalContext?: string): void {
+  // "No opinion" must be a SILENT exit 0 (optionally with additionalContext
+  // only), never an explicit `permissionDecision: 'defer'`. `defer` is a real
+  // value in Claude Code, but it means "pause this tool call and hand control
+  // back" and is honored in print/non-interactive mode. Emitting it here made
+  // every AskUserQuestion get deferred in non-interactive sessions (e.g. the
+  // desktop app) — the question UI never rendered. Interactive mode merely
+  // warns and ignores it.
+  if (additionalContext) {
+    process.stdout.write(
+      JSON.stringify({
+        hookSpecificOutput: {
+          hookEventName: 'PreToolUse',
+          additionalContext,
+        },
+      }),
+    );
+  }
   process.exit(0);
 }
 
@@ -347,15 +358,15 @@ function logAutoDecided(
 async function main(): Promise<void> {
   const raw = await readStdin();
   if (!raw.trim()) {
-    defer();
+    passThrough();
     return;
   }
   let stdin: HookStdin;
   try {
     stdin = JSON.parse(raw);
   } catch (e) {
     logHookError(`stdin parse failed: ${(e as Error).message}`);
-    defer();
+    passThrough();
     return;
   }
 
@@ -364,26 +375,26 @@ async function main(): Promise<void> {
     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,11 +413,11 @@ async function main(): Promise<void> {
     : undefined;
 
   // Determine whether EVERY question is eligible for never-ask auto-decide.
-  // We deliberately do NOT early-return defer 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
-  // ineligible question makes the whole call not auto-decidable.)
+  // We deliberately do NOT early-return passThrough 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 ineligible question makes the whole call not auto-decidable.)
   const autoDecisions: Array<{ id: string; recommended: string }> = [];
   let fullyAutoDecidable = true;
   for (const q of questions) {
@@ -471,10 +482,10 @@ async function main(): Promise<void> {
     return;
   }
 
-  defer(memoryContext);
+  passThrough(memoryContext);
 }
 
 main().catch((e) => {
   logHookError(`main crash: ${(e as Error).message}`);
-  defer();
+  passThrough();
 });

test/memory-cache-injection.test.ts

@@ -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 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 while passing through', () => {
     writeMemory([
       {
         nugget: 'User prefers verbose explanations with tradeoffs',
@@ -91,7 +91,8 @@ describe('memory injection', () => {
         ],
       },
     });
-    expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer');
+    expect(r.parsed?.hookSpecificOutput?.hookEventName).toBe('PreToolUse');
+    expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined();
     expect(r.parsed?.hookSpecificOutput?.additionalContext).toContain('verbose explanations');
   });
 
@@ -115,7 +116,8 @@ describe('memory injection', () => {
         ],
       },
     });
-    expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer');
+    expect(r.stdout).toBe('');
+    expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined();
     expect(r.parsed?.hookSpecificOutput?.additionalContext).toBeUndefined();
   });
 
@@ -219,7 +221,8 @@ describe('per-session memory cache', () => {
         ],
       },
     });
-    expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('defer');
+    expect(r.stdout).toBe('');
+    expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBeUndefined();
     expect(r.parsed?.hookSpecificOutput?.additionalContext).toBeUndefined();
   });
 });