Browse Source

fix(hooks): merge surfaces the WINNING decision's reason, not only deny's

mergeHookOutputs collected reasons only from rank-3 (deny/block) hooks, so an
ask-winning outcome lost its reason — a bridge mapping an `ask` decision to a
PreToolDecision had no reason to attach. Collect reasons per rank and emit the
ones explaining the winning decision: a deny-winning fold shows deny reasons, an
ask-winning fold shows ask reasons, allow contributes none. Found while building
the hooks-claude bridge's PreToolUse `ask` path.
Tianyi Cui 3 months ago
parent
commit
c28d6b837b

+ 11 - 4
packages/hooks/hook-protocol/src/merge.ts

@@ -74,7 +74,11 @@ function decisionForRank(maxRank: number): MergedDecision {
  */
 export function mergeHookOutputs(outputs: HookOutput[]): MergedHookOutcome {
   let maxRank = 0
-  const reasons: string[] = []
+  // Reasons collected PER RANK, so the merged reason can be the one explaining
+  // the WINNING decision (a deny-winning outcome surfaces deny reasons; an
+  // ask-winning outcome surfaces ask reasons). An `allow`'s reason is never an
+  // objection the model needs, so rank 1 collects none.
+  const reasonsByRank = new Map<number, string[]>()
   let stop = false
   let stopReason: string | undefined
   const additionalContext: string[] = []
@@ -83,9 +87,11 @@ export function mergeHookOutputs(outputs: HookOutput[]): MergedHookOutcome {
   for (const out of outputs) {
     const r = rank(out.decision)
     if (r > maxRank) maxRank = r
-    // Collect a reason only from a blocking/denying hook (rank 3) — an allow's
-    // "reason" is not an objection the model needs to see.
-    if (r === 3 && out.reason !== undefined && out.reason.length > 0) reasons.push(out.reason)
+    if ((r === 3 || r === 2) && out.reason !== undefined && out.reason.length > 0) {
+      const list = reasonsByRank.get(r) ?? []
+      list.push(out.reason)
+      reasonsByRank.set(r, list)
+    }
     if (out.continue === false && !stop) {
       stop = true
       if (out.stopReason !== undefined) stopReason = out.stopReason
@@ -98,6 +104,7 @@ export function mergeHookOutputs(outputs: HookOutput[]): MergedHookOutcome {
     }
   }
 
+  const reasons = reasonsByRank.get(maxRank) ?? []
   return {
     decision: decisionForRank(maxRank),
     ...reasons.length > 0 ? { reason: reasons.join('\n\n') } : {},

+ 18 - 0
packages/hooks/hook-protocol/tests/merge.spec.ts

@@ -47,6 +47,24 @@ describe('mergeHookOutputs — reasons, stop, context, systemMessages accumulate
     expect(mergeHookOutputs([out({ decision: 'allow' })]).reason).toBeUndefined()
   })
 
+  it('surfaces the reason of the WINNING decision: an ask-winning outcome shows the ask reason', () => {
+    const m = mergeHookOutputs([
+      out({ decision: 'allow', reason: 'allow reason — not surfaced' }),
+      out({ decision: 'ask', reason: 'needs approval' }),
+    ])
+    expect(m.decision).toBe('ask')
+    expect(m.reason).toBe('needs approval')
+  })
+
+  it('when deny wins over ask, the ask reasons are dropped (only the winning rank\'s reasons)', () => {
+    const m = mergeHookOutputs([
+      out({ decision: 'ask', reason: 'ask reason — not surfaced once deny wins' }),
+      out({ decision: 'deny', reason: 'the real objection' }),
+    ])
+    expect(m.decision).toBe('deny')
+    expect(m.reason).toBe('the real objection')
+  })
+
   it('stop is sticky on the first continue:false, capturing its stopReason', () => {
     const m = mergeHookOutputs([
       out({ continue: true }),