diff --git a/hosts/claude/hooks/question-preference-hook.ts b/hosts/claude/hooks/question-preference-hook.ts index dde1bda0c..12cbd5ea2 100644 --- a/hosts/claude/hooks/question-preference-hook.ts +++ b/hosts/claude/hooks/question-preference-hook.ts @@ -40,6 +40,7 @@ import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; import { spawnSync } from 'child_process'; +import { isConductor } from '../../../lib/is-conductor'; interface HookStdin { session_id?: string; @@ -400,57 +401,77 @@ async function main(): Promise { ? '[plan-tune memory] Past answers suggest: ' + contextNuggets.join(' | ') : 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.) const autoDecisions: Array<{ id: string; recommended: string }> = []; + let fullyAutoDecidable = true; for (const q of questions) { const qText = q.question || ''; const marker = qText.match(MARKER_RE); - if (!marker) { - defer(memoryContext); - return; - } + if (!marker) { fullyAutoDecidable = false; break; } const questionId = marker[1]; const pref = lookupPreference(slug, questionId); - if (!pref.preference || pref.preference === 'always-ask') { - defer(memoryContext); - return; - } + if (!pref.preference || pref.preference === 'always-ask') { fullyAutoDecidable = false; break; } const entry = registry[questionId]; const doorType = entry?.door_type || 'two-way'; - if (doorType === 'one-way') { - // Safety override — even never-ask doesn't bypass one-way doors. - defer(memoryContext); - return; - } + // Safety override — even never-ask doesn't bypass one-way doors. + if (doorType === 'one-way') { fullyAutoDecidable = false; break; } const opts = optionLabels(q.options || []); const { recommended, ambiguous } = extractRecommended(qText, opts); - if (!recommended || ambiguous) { - // Refuse-on-ambiguous per D2 — fail safe, ask normally. - defer(memoryContext); - return; - } + // Refuse-on-ambiguous per D2 — fail safe. + if (!recommended || ambiguous) { fullyAutoDecidable = false; break; } autoDecisions.push({ id: questionId, recommended }); } - // All questions were eligible for enforcement. - markAutoDecided(stdin.session_id, stdin.tool_use_id); + if (fullyAutoDecidable && autoDecisions.length > 0) { + // All questions were eligible for enforcement. + markAutoDecided(stdin.session_id, stdin.tool_use_id); - // Log each auto-decided question now, since deny prevents PostToolUse from - // firing. /plan-tune Recent auto-decisions reads source=auto-decided events. - for (let i = 0; i < autoDecisions.length; i++) { - const d = autoDecisions[i]; - const q = questions[i]; - const qText = (q.question || '').replace(MARKER_RE, '').trim(); - const opts = optionLabels(q.options || []); - logAutoDecided(d.id, qText, d.recommended, opts.length, stdin.session_id, stdin.tool_use_id, stdin.cwd); + // Log each auto-decided question now, since deny prevents PostToolUse from + // firing. /plan-tune Recent auto-decisions reads source=auto-decided events. + for (let i = 0; i < autoDecisions.length; i++) { + const d = autoDecisions[i]; + const q = questions[i]; + const qText = (q.question || '').replace(MARKER_RE, '').trim(); + const opts = optionLabels(q.options || []); + logAutoDecided(d.id, qText, d.recommended, opts.length, stdin.session_id, stdin.tool_use_id, stdin.cwd); + } + + const reasonLines = autoDecisions.map( + (d) => + `[plan-tune auto-decide] ${d.id} → ${d.recommended} (your never-ask preference). Proceed with that option without re-prompting. Change with /plan-tune.`, + ); + deny(reasonLines.join('\n')); + return; } - const reasonLines = autoDecisions.map( - (d) => - `[plan-tune auto-decide] ${d.id} → ${d.recommended} (your never-ask preference). Proceed with that option without re-prompting. Change with /plan-tune.`, - ); - deny(reasonLines.join('\n')); + // Not fully auto-decidable. In Conductor, AskUserQuestion is unreliable + // (native is disabled, the mcp__conductor__AskUserQuestion variant is flaky), + // so deny the tool and redirect to a prose decision brief. This is TRANSPORT + // AVOIDANCE, not preference enforcement: it fires regardless of marker, + // preference, or door type — including one-way doors, which must reach the + // human via prose rather than the unreliable tool. + if (isConductor()) { + const conductorReason = + '[conductor] AskUserQuestion is unreliable in Conductor (native disabled, MCP variant flaky). ' + + 'Do NOT call AskUserQuestion (native or any mcp__*__AskUserQuestion). Render this decision as a ' + + 'PROSE decision brief now: a D label, an ELI10 of the issue, a Recommendation line, then one ' + + 'paragraph per choice carrying its `(recommended)` marker and `Completeness: X/10`; tell the user ' + + 'to reply with a letter, then STOP. For a one-way/destructive confirmation, require an explicit ' + + 'typed confirmation and do NOT proceed on a vague reply. Capture the decision with gstack-question-log ' + + '(PostToolUse will not fire on a prose path).' + + (memoryContext ? `\n${memoryContext}` : ''); + deny(conductorReason); + return; + } + + defer(memoryContext); } main().catch((e) => { diff --git a/test/question-preference-hook.test.ts b/test/question-preference-hook.test.ts index 47342c19a..39de02f4e 100644 --- a/test/question-preference-hook.test.ts +++ b/test/question-preference-hook.test.ts @@ -60,7 +60,7 @@ function writeGlobalPref(questionId: string, preference: string): void { fs.writeFileSync(f, JSON.stringify(prefs, null, 2)); } -function runHook(stdin: object, cwd?: string): { +function runHook(stdin: object, cwd?: string, extraEnv?: Record): { stdout: string; stderr: string; status: number; @@ -75,10 +75,12 @@ function runHook(stdin: object, cwd?: string): { // Strip ambient Conductor markers so these cases characterize NON-Conductor // behavior deterministically — otherwise running the suite inside Conductor // (CONDUCTOR_WORKSPACE_PATH/PORT set) would flip every defer into the - // [conductor] prose deny. The Conductor cases below opt back in explicitly. + // [conductor] prose deny. The Conductor cases below opt back in explicitly + // via extraEnv. delete env.CONDUCTOR_WORKSPACE_PATH; delete env.CONDUCTOR_PORT; env.GSTACK_QUESTION_LOG_NO_DERIVE = '1'; + if (extraEnv) Object.assign(env, extraEnv); const res = spawnSync(HOOK, [], { env, input: JSON.stringify({ ...stdin, cwd: cwd || fixtureCwd }), @@ -343,6 +345,108 @@ describe('MCP variant', () => { }); }); +// ---------------------------------------------------------------------- +// Conductor: deny + prose redirect (transport avoidance, not preference) +// ---------------------------------------------------------------------- + +describe('Conductor prose redirect', () => { + const CONDUCTOR = { CONDUCTOR_PORT: '55070' }; + + test('two-way, no preference → deny with [conductor] prose directive', () => { + const r = runHook({ + session_id: 'c1', + tool_name: 'AskUserQuestion', + tool_use_id: 'tu-c1', + tool_input: { + questions: [ + { question: ' Need approval?', options: ['A) Yes (recommended)', 'B) No'] }, + ], + }, + }, undefined, CONDUCTOR); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toContain('[conductor]'); + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toMatch(/do not call askuserquestion/i); + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toMatch(/reply with a letter/i); + }); + + test('UNMARKED question (modal path) → deny with prose directive', () => { + const r = runHook({ + session_id: 'c2', + tool_name: 'AskUserQuestion', + tool_use_id: 'tu-c2', + tool_input: { + questions: [ + { question: 'No marker — an ad-hoc question', options: ['A) Yes (recommended)', 'B) No'] }, + ], + }, + }, undefined, CONDUCTOR); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toContain('[conductor]'); + }); + + test('one-way door → deny with prose directive (NOT defer — destructive must reach human via prose)', () => { + const r = runHook({ + session_id: 'c3', + tool_name: 'AskUserQuestion', + tool_use_id: 'tu-c3', + tool_input: { + questions: [ + { + question: ' Tests failed.', + options: ['A) Fix now (recommended)', 'B) Investigate', 'C) Ack and ship'], + }, + ], + }, + }, undefined, CONDUCTOR); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toContain('[conductor]'); + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toMatch(/typed confirmation/i); + }); + + test('CONDUCTOR_WORKSPACE_PATH alone also triggers the redirect', () => { + const r = runHook({ + session_id: 'c4', + tool_name: 'mcp__conductor__AskUserQuestion', + tool_use_id: 'tu-c4', + tool_input: { + questions: [{ question: ' Pick?', options: ['A) X (recommended)', 'B) Y'] }], + }, + }, undefined, { CONDUCTOR_WORKSPACE_PATH: '/Users/x/conductor/ws' }); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toContain('[conductor]'); + }); + + test('PRECEDENCE: full never-ask auto-decide still wins over Conductor prose', () => { + writeProjectPref('ship-pre-landing-review-fix', 'never-ask'); + const r = runHook({ + session_id: 'c5', + tool_name: 'AskUserQuestion', + tool_use_id: 'tu-c5', + tool_input: { + questions: [ + { + question: ' Pre-landing review flagged issue.', + options: ['A) Fix now (recommended)', 'B) Skip'], + }, + ], + }, + }, undefined, CONDUCTOR); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('deny'); + // auto-decide reason, NOT the conductor prose reason + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toContain('plan-tune auto-decide'); + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).not.toContain('[conductor]'); + }); + + test('non-AUQ tool in Conductor → still defer (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'); + }); +}); + // ---------------------------------------------------------------------- // Auto-decided event logging (since PostToolUse never fires on deny) // ----------------------------------------------------------------------