From e3d156f8005d3c96e5cc4a0f4e338395be8ff01f Mon Sep 17 00:00:00 2001 From: gavrielc Date: Sat, 4 Jul 2026 16:39:04 +0300 Subject: [PATCH] test(channels): cover ask_question option style normalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewers on #2933 asked for tests on the button-style plumbing. Two layers: - src/channels/ask-question.test.ts (new): normalizeOption/normalizeOptions style whitelist — primary/danger/default pass through; unknown strings, case variants, and non-string values drop to undefined; string-shorthand options carry no style; style coexists with the label/selectedLabel/value defaulting. This is the load-bearing gate: an invalid style reaching Slack Block Kit fails the whole card with invalid_blocks, which in the approval flow is an effective auto-deny. - src/channels/chat-sdk-bridge.test.ts: ask_question delivery passes each normalized option style into Button() and omits it when unset; an invalid style in the raw payload is stripped before the card is built. Co-Authored-By: Claude Fable 5 --- src/channels/ask-question.test.ts | 76 ++++++++++++++++++++++++++++ src/channels/chat-sdk-bridge.test.ts | 71 ++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 src/channels/ask-question.test.ts diff --git a/src/channels/ask-question.test.ts b/src/channels/ask-question.test.ts new file mode 100644 index 000000000..ae13c2adc --- /dev/null +++ b/src/channels/ask-question.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it } from 'vitest'; + +import { normalizeOption, normalizeOptions } from './ask-question.js'; + +describe('normalizeOption — style whitelist', () => { + // The style value flows straight into the Chat SDK Button() and from there + // into Slack Block Kit. Slack rejects the *entire* message with + // invalid_blocks if a button carries an unknown style, which in the + // approval flow means the card never renders — an effective auto-deny. + // So anything outside the whitelist must drop to undefined here. + + it.each(['primary', 'danger', 'default'] as const)('passes through the known style %j', (style) => { + expect(normalizeOption({ label: 'Approve', style }).style).toBe(style); + }); + + it('drops unknown style strings to undefined', () => { + for (const bad of ['success', 'warning', 'PRIMARY', 'Danger', ' primary', 'primary ', '', 'red']) { + const opt = normalizeOption({ label: 'Approve', style: bad as never }); + expect(opt.style, `style ${JSON.stringify(bad)} should be dropped`).toBeUndefined(); + } + }); + + it('drops non-string style values to undefined', () => { + for (const bad of [1, true, null, {}, ['primary']]) { + const opt = normalizeOption({ label: 'Approve', style: bad as never }); + expect(opt.style, `style ${JSON.stringify(bad)} should be dropped`).toBeUndefined(); + } + }); + + it('leaves style undefined when the object option omits it', () => { + expect(normalizeOption({ label: 'Approve' }).style).toBeUndefined(); + }); + + it('gives string-shorthand options no style', () => { + const opt = normalizeOption('Approve'); + expect(opt).toEqual({ label: 'Approve', selectedLabel: 'Approve', value: 'Approve' }); + expect('style' in opt && opt.style !== undefined).toBe(false); + }); + + it('style coexists with the label/selectedLabel/value defaulting', () => { + // Defaults still fill in around an explicit style… + expect(normalizeOption({ label: 'Approve', style: 'primary' })).toEqual({ + label: 'Approve', + selectedLabel: 'Approve', + value: 'Approve', + style: 'primary', + }); + // …and explicit fields are untouched by the style whitelist. + expect(normalizeOption({ label: 'Deny', selectedLabel: 'Denied', value: 'deny-1', style: 'danger' })).toEqual({ + label: 'Deny', + selectedLabel: 'Denied', + value: 'deny-1', + style: 'danger', + }); + // An invalid style must not disturb the rest of the normalization. + expect(normalizeOption({ label: 'Deny', value: 'deny-1', style: 'bogus' as never })).toEqual({ + label: 'Deny', + selectedLabel: 'Deny', + value: 'deny-1', + style: undefined, + }); + }); +}); + +describe('normalizeOptions', () => { + it('normalizes mixed string and object options, preserving order and per-option styles', () => { + const out = normalizeOptions([ + 'Skip', + { label: 'Approve', style: 'primary' }, + { label: 'Deny', style: 'danger' }, + { label: 'Later', style: 'lime' as never }, + ]); + expect(out.map((o) => o.label)).toEqual(['Skip', 'Approve', 'Deny', 'Later']); + expect(out.map((o) => o.style)).toEqual([undefined, 'primary', 'danger', undefined]); + }); +}); diff --git a/src/channels/chat-sdk-bridge.test.ts b/src/channels/chat-sdk-bridge.test.ts index e87dd9842..6dd129ff8 100644 --- a/src/channels/chat-sdk-bridge.test.ts +++ b/src/channels/chat-sdk-bridge.test.ts @@ -238,6 +238,77 @@ describe('createChatSdkBridge.setup — webhook route and state namespace', () = }); }); +describe('createChatSdkBridge.deliver — ask_question cards (button styles)', () => { + // Approval cards color their buttons (Slack: primary→green, danger→red). + // The bridge must forward the normalized option style into Button() and + // omit it when unset — an invalid style surviving to Block Kit would fail + // the whole card with invalid_blocks (effective auto-deny). + + interface CapturedButton { + type?: string; + id?: string; + label?: string; + value?: string; + style?: string; + } + + function buttonsFrom(calls: PostCall[]): CapturedButton[] { + const msg = calls[0].message as { + card?: { children?: Array<{ type?: string; children?: CapturedButton[] }> }; + }; + const actionsRow = msg.card?.children?.find((c) => c.type === 'actions'); + expect(actionsRow).toBeDefined(); + return actionsRow?.children ?? []; + } + + it('passes each option style through to the Button, and omits it when unset', async () => { + const { calls, postMessage } = makePostCapture(); + const bridge = createChatSdkBridge({ + adapter: stubAdapter({ postMessage }), + supportsThreads: false, + }); + await bridge.deliver('slack:C1', null, { + kind: 'chat-sdk', + content: { + type: 'ask_question', + questionId: 'q-1', + title: 'Approval needed', + question: 'Allow the tool call?', + options: [ + { label: 'Approve', style: 'primary' }, + { label: 'Deny', style: 'danger' }, + 'Skip', // string shorthand — never styled + ], + }, + }); + expect(calls).toHaveLength(1); + const buttons = buttonsFrom(calls); + expect(buttons.map((b) => b.label)).toEqual(['Approve', 'Deny', 'Skip']); + expect(buttons.map((b) => b.style)).toEqual(['primary', 'danger', undefined]); + }); + + it('drops invalid styles before they reach the Button (delivery goes through normalizeOptions)', async () => { + const { calls, postMessage } = makePostCapture(); + const bridge = createChatSdkBridge({ + adapter: stubAdapter({ postMessage }), + supportsThreads: false, + }); + await bridge.deliver('slack:C1', null, { + kind: 'chat-sdk', + content: { + type: 'ask_question', + questionId: 'q-2', + title: 'Approval needed', + question: 'Allow the tool call?', + options: [{ label: 'Approve', style: 'chartreuse' }], + }, + }); + const buttons = buttonsFrom(calls); + expect(buttons).toHaveLength(1); + expect(buttons[0].style).toBeUndefined(); + }); +}); + describe('createChatSdkBridge.deliver — display cards (send_card)', () => { // The send_card MCP tool writes outbound rows with `{ type: 'card', card, fallbackText }`. // Before this branch existed the bridge silently dropped them: cards have no