diff --git a/packages/hooks/hook-protocol/src/merge.ts b/packages/hooks/hook-protocol/src/merge.ts index d8fb158eb1..1e53dbaaea 100644 --- a/packages/hooks/hook-protocol/src/merge.ts +++ b/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() 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') } : {}, diff --git a/packages/hooks/hook-protocol/tests/merge.spec.ts b/packages/hooks/hook-protocol/tests/merge.spec.ts index 9709060d79..def2474927 100644 --- a/packages/hooks/hook-protocol/tests/merge.spec.ts +++ b/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 }),