diff --git a/packages/channels/base/src/ChannelBase.test.ts b/packages/channels/base/src/ChannelBase.test.ts index 92e8fcc8ab1..e6ccf158528 100644 --- a/packages/channels/base/src/ChannelBase.test.ts +++ b/packages/channels/base/src/ChannelBase.test.ts @@ -335,6 +335,12 @@ class TestChannel extends ChannelBase { } } +class CaseInsensitiveGroupChannel extends TestChannel { + protected override normalizeGroupId(groupId: string): string { + return groupId.toLowerCase(); + } +} + class ResponseTrackingChannel extends TestChannel { responseDeliveries: Array<{ chatId: string; @@ -4544,6 +4550,50 @@ describe('ChannelBase', () => { ); }); + it('applies a per-group history limit across the platform ID casing', async () => { + // The history limit is resolved through the same group lookup as the + // allowlist, so a platform with case-insensitive IDs must not lose its + // per-group override when the reported ID is spelled differently. + const ch = new CaseInsensitiveGroupChannel( + 'test-chan', + defaultConfig({ + groupPolicy: 'open', + groups: { + 'qwenlm/qwen-code': { requireMention: true, groupHistoryLimit: 10 }, + }, + }), + bridge, + { groupHistoryPath: groupHistoryPath() }, + ); + + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: false, + chatId: 'QwenLM/qwen-code', + senderId: 'u1', + senderName: 'Alice', + text: 'background context', + }), + ); + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + chatId: 'QwenLM/qwen-code', + senderId: 'u2', + senderName: 'Bob', + text: '@bot summarize', + }), + ); + + const prompt = (bridge.prompt as ReturnType).mock + .calls[0][1] as string; + expect(prompt).toBe( + '[Chat messages since your last reply - for context]\n- [Alice] background context\n\n[Current message - respond to this]\n[Bob] @bot summarize', + ); + }); + it('backfills messages from members of an approved paired group', async () => { const previousQwenHome = process.env['QWEN_HOME']; const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); diff --git a/packages/channels/base/src/ChannelBase.ts b/packages/channels/base/src/ChannelBase.ts index 07c96210b73..0b81e748394 100644 --- a/packages/channels/base/src/ChannelBase.ts +++ b/packages/channels/base/src/ChannelBase.ts @@ -1451,6 +1451,7 @@ export abstract class ChannelBase { config.groupPolicy, config.groups, pairingStore, + (groupId) => this.normalizeGroupId(groupId), ); this.dmGate = new DmGate( this.privatePolicy === 'disabled' ? 'disabled' : 'open', @@ -6169,7 +6170,7 @@ export abstract class ChannelBase { if (!envelope.isGroup) { return 0; } - const groupCfg = this.config.groups[envelope.chatId]; + const groupCfg = this.groupConfigFor(envelope.chatId); const wildcardGroupCfg = this.config.groups['*']; const configured = groupCfg?.groupHistoryLimit ?? @@ -6345,7 +6346,22 @@ export abstract class ChannelBase { private groupConfigFor(key: string): GroupConfig | undefined { const groups = this.config.groups; - return Object.hasOwn(groups, key) ? groups[key] : undefined; + if (Object.hasOwn(groups, key)) return groups[key]; + const normalized = this.normalizeGroupId(key); + for (const [groupId, group] of Object.entries(groups)) { + if (this.normalizeGroupId(groupId) === normalized) return group; + } + return undefined; + } + + /** + * Group IDs are matched verbatim. A platform whose IDs are case-insensitive + * (GitHub repository full names) overrides this so the spelling an operator + * configured still matches the canonical ID the platform reports — for the + * allowlist, the sender policy and the per-group overrides alike. + */ + protected normalizeGroupId(groupId: string): string { + return groupId; } protected preflightInbound( diff --git a/packages/channels/base/src/GroupGate.test.ts b/packages/channels/base/src/GroupGate.test.ts index 6d35835614f..6ee93bbca0a 100644 --- a/packages/channels/base/src/GroupGate.test.ts +++ b/packages/channels/base/src/GroupGate.test.ts @@ -34,6 +34,30 @@ function envelope(overrides: Partial = {}): Envelope { } describe('GroupGate', () => { + it('matches group IDs verbatim unless the platform normalizes them', () => { + const gate = new GroupGate('allowlist', { 'QwenLM/qwen-code': {} }); + expect( + gate.check(envelope({ isGroup: true, chatId: 'qwenlm/qwen-code' })) + .reason, + ).toBe('not_allowlisted'); + }); + + it('admits a differently-cased group ID through the platform normalizer', () => { + const gate = new GroupGate( + 'allowlist', + { 'qwenlm/qwen-code': { requireMention: false } }, + undefined, + (groupId) => groupId.toLowerCase(), + ); + expect( + gate.check(envelope({ isGroup: true, chatId: 'QwenLM/qwen-code' })) + .allowed, + ).toBe(true); + expect( + gate.check(envelope({ isGroup: true, chatId: 'other/repo' })).reason, + ).toBe('not_allowlisted'); + }); + it('inherits requireMention by field even when the group has another override', () => { const gate = new GroupGate('allowlist', { '*': { requireMention: false }, diff --git a/packages/channels/base/src/GroupGate.ts b/packages/channels/base/src/GroupGate.ts index ef689d07131..ce876dc0f6c 100644 --- a/packages/channels/base/src/GroupGate.ts +++ b/packages/channels/base/src/GroupGate.ts @@ -20,15 +20,33 @@ export class GroupGate { private policy: GroupPolicy; private groups: Record; private pairingStore: PairingStore | null; + private normalizeGroupId: (groupId: string) => string; constructor( policy: GroupPolicy = 'disabled', groups: Record = {}, pairingStore?: PairingStore, + normalizeGroupId: (groupId: string) => string = (groupId) => groupId, ) { this.policy = policy; this.groups = groups; this.pairingStore = pairingStore ?? null; + this.normalizeGroupId = normalizeGroupId; + } + + /** + * Configured groups are matched verbatim first. Platforms whose group IDs + * are case-insensitive (GitHub repository full names) also match a + * normalized key, so the operator's spelling admits the canonical ID the + * platform reports. + */ + private groupConfigFor(groupId: string): GroupConfig | undefined { + if (Object.hasOwn(this.groups, groupId)) return this.groups[groupId]; + const normalized = this.normalizeGroupId(groupId); + for (const [key, group] of Object.entries(this.groups)) { + if (this.normalizeGroupId(key) === normalized) return group; + } + return undefined; } /** @@ -61,7 +79,7 @@ export class GroupGate { if (this.policy === 'allowlist') { // In allowlist mode, "*" is only a default config — not a wildcard allow. // The group must be explicitly listed by ID. - if (!this.groups[envelope.chatId]) { + if (!this.groupConfigFor(envelope.chatId)) { return { allowed: false, reason: 'not_allowlisted' }; } } @@ -90,7 +108,7 @@ export class GroupGate { } const requireMention = - this.groups[envelope.chatId]?.requireMention ?? + this.groupConfigFor(envelope.chatId)?.requireMention ?? this.groups['*']?.requireMention ?? true; diff --git a/packages/channels/github/src/GithubAdapter.test.ts b/packages/channels/github/src/GithubAdapter.test.ts index 86f0b8d1300..05413d367e8 100644 --- a/packages/channels/github/src/GithubAdapter.test.ts +++ b/packages/channels/github/src/GithubAdapter.test.ts @@ -1107,6 +1107,81 @@ describe('GithubChannel', () => { channel.disconnect(); }); + describe('repository ID casing', () => { + async function connectWithGroups( + groups: Record, + ): Promise { + channel = new TestableGithubChannel( + 'test-github', + makeConfig({ groupPolicy: 'allowlist', groups }), + makeBridge(), + ); + mockOctokit.paginate.mockResolvedValue([]); + await channel.connect(); + channel.usePreflight = true; + } + + function comment(chatId: string, senderId = 'alice'): Envelope { + return { + channelName: 'test-github', + senderId, + senderName: senderId, + chatId, + text: 'review this', + isGroup: true, + isMentioned: true, + isReplyToBot: false, + }; + } + + it('admits the canonical repository spelling for a lower-case allowlist', async () => { + // GitHub repository paths are case-insensitive, but notifications + // report the canonical `owner/repo`. An allowlist typed in lower case + // must still admit them instead of dropping every notification. + await connectWithGroups({ + 'qwenlm/qwen-code': { requireMention: false }, + }); + try { + await channel.handleInbound(comment('QwenLM/qwen-code')); + expect(channel.inboundEnvelopes).toHaveLength(1); + } finally { + channel.disconnect(); + } + }); + + it('still rejects a repository that is not allowlisted', async () => { + await connectWithGroups({ + 'qwenlm/qwen-code': { requireMention: false }, + }); + try { + await channel.handleInbound(comment('SomeoneElse/other-repo')); + expect(channel.inboundEnvelopes).toHaveLength(0); + } finally { + channel.disconnect(); + } + }); + + it('applies the repository sender allowlist across the case difference', async () => { + // The sender policy is resolved through the same group lookup, so a + // case-insensitive admission must not fall back to the open default. + await connectWithGroups({ + 'qwenlm/qwen-code': { + requireMention: false, + senders: 'allowlist', + allowedUsers: ['alice'], + }, + }); + try { + await channel.handleInbound(comment('QwenLM/qwen-code', 'bob')); + expect(channel.inboundEnvelopes).toHaveLength(0); + await channel.handleInbound(comment('QwenLM/qwen-code', 'alice')); + expect(channel.inboundEnvelopes).toHaveLength(1); + } finally { + channel.disconnect(); + } + }); + }); + it.each(['*', 'owner/restricted'])( 'warns about bot-only group %s without blocking another repository', async (restrictedGroup) => { diff --git a/packages/channels/github/src/GithubAdapter.ts b/packages/channels/github/src/GithubAdapter.ts index 83956c19e4b..d121e61bfeb 100644 --- a/packages/channels/github/src/GithubAdapter.ts +++ b/packages/channels/github/src/GithubAdapter.ts @@ -535,6 +535,16 @@ export class GithubChannel extends PollingChannelBase { return { lastProcessedAt: new Date().toISOString() }; } + /** + * GitHub repository paths are case-insensitive, but the notification API + * reports the canonical `owner/repo` spelling. Match configured groups + * case-insensitively so an allowlist typed in any case still admits the + * repository — and still applies that repository's sender policy. + */ + protected override normalizeGroupId(groupId: string): string { + return groupId.toLowerCase(); + } + protected override validateCursor(parsed: unknown): GithubCursor | null { const base = super.validateCursor(parsed); if (!base || typeof base.lastProcessedAt !== 'string') return null; diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.module.css b/packages/web-shell/client/components/channels/ChannelEditorDialog.module.css index bfd8197a1b6..c27a59484f1 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.module.css +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.module.css @@ -120,6 +120,51 @@ line-height: 1.5; } +.optionGrid { + display: flex; + flex-wrap: wrap; + gap: 6px; +} + +.optionRow { + display: flex; + min-height: 34px; + cursor: pointer; + align-items: center; + gap: 8px; + padding: 6px 10px; + border: 1px solid var(--border); + border-radius: 999px; + background: var(--card); + user-select: none; + transition: + border-color 120ms ease, + background-color 120ms ease; +} + +.optionRow:hover { + border-color: var(--foreground); +} + +.optionRow[data-selected='true'] { + border-color: var(--foreground); + background: color-mix(in srgb, var(--muted) 62%, transparent); +} + +.optionCheckbox { + width: 14px; + height: 14px; + flex: 0 0 auto; + margin: 0; + cursor: pointer; + accent-color: var(--primary); +} + +.optionLabel { + font-size: 12px; + line-height: 1.35; +} + .recordFields { display: flex; flex-direction: column; @@ -309,4 +354,8 @@ .sessionScopeOption { transition: none; } + + .optionRow { + transition: none; + } } diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx index 426ecc2cb78..e1812e02988 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx @@ -143,6 +143,75 @@ const GITHUB_LOCAL_GH: DaemonChannelTypeDescriptor = { ], }; +// Mirrors the registry-composed GitHub catalog: the plugin fields plus the +// injected privatePolicy/allowedUsers that the adapter never reads. +const GITHUB_CONFIGURED: DaemonChannelTypeDescriptor = { + type: 'github', + displayName: 'GitHub', + manageable: true, + fields: [ + { + key: 'token', + label: 'Personal Access Token', + kind: 'secret', + envResolvable: true, + }, + { + key: 'useLocalGh', + label: 'Use Local GitHub CLI Authentication', + kind: 'boolean', + }, + { + key: 'baseUrl', + label: 'Base URL', + kind: 'string', + envResolvable: true, + }, + { + key: 'groupPolicy', + label: 'Group Policy', + kind: 'enum', + required: true, + default: 'open', + options: [ + { value: 'open', label: 'Open' }, + { value: 'allowlist', label: 'Allowlist' }, + { value: 'pairing', label: 'Pairing' }, + { value: 'disabled', label: 'Disabled' }, + ], + }, + { + key: 'reasonFilter', + label: 'Reason Filter', + kind: 'string-list', + options: [ + { value: 'mention', label: 'mention' }, + { value: 'review_requested', label: 'review_requested' }, + { value: 'ci_activity', label: 'ci_activity' }, + { value: 'comment', label: 'comment' }, + ], + }, + { + key: 'privatePolicy', + label: 'Private Policy', + kind: 'enum', + required: true, + default: 'pairing', + options: [ + { value: 'disabled', label: 'Disabled' }, + { value: 'allowlist', label: 'Allowlist' }, + { value: 'pairing', label: 'Pairing' }, + { value: 'open', label: 'Open' }, + ], + }, + { + key: 'allowedUsers', + label: 'Allowed Users', + kind: 'string-list', + }, + ], +}; + const EXCLUSIVE_MINIMUM: DaemonChannelTypeDescriptor = { type: 'example', displayName: 'Example', @@ -182,6 +251,36 @@ const MULTILINE_INSTRUCTIONS: DaemonChannelTypeDescriptor = { ], }; +// A non-GitHub channel carrying the connection and routing field keys the +// section split keys on, so the split is not pinned to the GitHub editor. +const GITLAB_ROUTING: DaemonChannelTypeDescriptor = { + type: 'gitlab', + displayName: 'GitLab', + manageable: true, + fields: [ + { + key: 'token', + label: 'Personal Access Token', + kind: 'secret', + }, + { + key: 'baseUrl', + label: 'Instance URL', + kind: 'string', + }, + { + key: 'defaultMessageRoute', + label: 'Default Message Route', + kind: 'string', + }, + { + key: 'messageRoutes', + label: 'Message Routes', + kind: 'record', + }, + ], +}; + const INSTANCE: DaemonChannelInstanceSnapshot = { name: 'release-bot', config: { @@ -323,6 +422,16 @@ function sectionHeadingOf(element: HTMLElement | null): string | null { return heading ?? null; } +// Record fields render their label without targeting a control, so resolve the +// section from the label text instead of the control id. +function sectionHeadingOfLabel(labelText: string): string | null { + const label = Array.from(document.querySelectorAll('label')).find((item) => + item.textContent?.includes(labelText), + ); + const heading = label?.closest('section')?.querySelector('h3'); + return heading?.textContent?.trim() ?? null; +} + // FieldShell renders the field description as the first

inside the field // wrapper that also holds the control, after the label header. function descriptionOf(element: HTMLElement | null): string { @@ -1079,12 +1188,466 @@ describe('ChannelEditorDialog', () => { config: { type: 'github', useLocalGh: true, - privatePolicy: 'pairing', }, secrets: { token: { operation: 'clear' } }, }); }); + it('renders the reason filter as checkboxes and saves the checked reasons', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ descriptor: GITHUB_CONFIGURED, onSave }); + + const group = document.querySelector( + '[role="group"][aria-label="Reason Filter"]', + ); + const boxes = Array.from( + group?.querySelectorAll('input[type="checkbox"]') ?? [], + ); + expect(boxes.map((box) => box.closest('label')?.textContent)).toEqual([ + 'mention — the bot is @mentioned', + 'review_requested — the bot, or a team it belongs to, was asked to review a pull request', + 'ci_activity — a workflow run the bot triggered completed', + 'comment — the bot commented on the thread', + ]); + + await act(async () => { + setInputValue(inputByLabel('Instance name')!, 'github-bot'); + setInputValue(inputByLabel('Personal Access Token')!, 'ghp_test'); + }); + for (const label of [ + 'mention — the bot is @mentioned', + 'ci_activity — a workflow run the bot triggered completed', + ]) { + await act(async () => { + Array.from( + group?.querySelectorAll('input[type="checkbox"]') ?? + [], + ) + .find((box) => box.closest('label')?.textContent === label) + ?.click(); + }); + } + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(onSave).toHaveBeenCalledWith( + 'github-bot', + expect.objectContaining({ + config: expect.objectContaining({ + reasonFilter: ['mention', 'ci_activity'], + }), + }), + ); + }); + + it('ticks the canonical reason box for a stored upper-case value', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + groupPolicy: 'open', + reasonFilter: ['COMMENT'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_CONFIGURED, instance }); + + const boxes = Array.from( + document.querySelectorAll( + '[role="group"][aria-label="Reason Filter"] input[type="checkbox"]', + ), + ); + // The adapter lowercases both sides, so `COMMENT` is the canonical + // `comment` reason: it must tick that box, not add a sixteenth entry that + // keeps processing comments after the operator unticks `comment`. + expect(boxes).toHaveLength(4); + const byLabel = new Map( + boxes.map((box) => [box.closest('label')?.textContent, box.checked]), + ); + expect(byLabel.get('comment — the bot commented on the thread')).toBe(true); + expect(byLabel.get('mention — the bot is @mentioned')).toBe(false); + }); + + it('keeps a hand-written reason visible instead of dropping it on save', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + groupPolicy: 'open', + reasonFilter: ['mention', 'typo_reason'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_CONFIGURED, instance, onSave }); + + const unknown = Array.from( + document.querySelectorAll( + '[role="group"][aria-label="Reason Filter"] input[type="checkbox"]', + ), + ).find((box) => box.closest('label')?.textContent === 'typo_reason'); + expect(unknown).toBeDefined(); + expect(unknown!.checked).toBe(true); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(document.body.textContent).toContain( + "Remove values that aren't in the allowed list.", + ); + expect(onSave).not.toHaveBeenCalled(); + }); + + it('locks the token input while local gh authentication is selected', async () => { + await renderDialog({ descriptor: GITHUB_CONFIGURED }); + + expect(inputByLabel('Personal Access Token')).not.toBeNull(); + + const toggle = document.querySelector( + 'button[role="switch"]', + ); + await act(async () => { + toggle!.click(); + }); + + expect(inputByLabel('Personal Access Token')).toBeNull(); + expect(document.body.textContent).toContain( + 'Local gh authentication is selected. The token cannot be edited here,', + ); + }); + + it('keeps the stored token when local gh authentication is switched back off', async () => { + // The switch is a preference. Turning it on queues the token for removal + // because a token outranks gh; turning it back off must undo that, or the + // operator is locked out of saving until they retype a PAT. + const onSave = vi.fn().mockResolvedValue(undefined); + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { type: 'github', groupPolicy: 'open', useLocalGh: false }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_CONFIGURED, instance, onSave }); + + const toggle = document.querySelector( + 'button[role="switch"]', + ); + await act(async () => { + toggle!.click(); + }); + await act(async () => { + toggle!.click(); + }); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(document.body.textContent).not.toContain( + 'Enter a token or enable local GitHub CLI authentication.', + ); + expect(onSave).toHaveBeenCalledWith( + 'github-bot', + expect.objectContaining({ + config: expect.objectContaining({ useLocalGh: false }), + secrets: { token: { operation: 'preserve' } }, + }), + ); + }); + + it('keeps a newly entered PAT when local gh is switched on and back off', async () => { + // Restoring `preserve` unconditionally would drop the token the operator + // just typed: a new channel would then submit no credential at all, with + // the input gone so it cannot be re-entered. + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ descriptor: GITHUB_CONFIGURED, onSave }); + + await act(async () => { + setInputValue(inputByLabel('Instance name')!, 'github-bot'); + setInputValue(inputByLabel('Personal Access Token')!, 'ghp_typed'); + }); + const toggle = document.querySelector( + 'button[role="switch"]', + ); + await act(async () => { + toggle!.click(); + }); + await act(async () => { + toggle!.click(); + }); + + expect(inputByLabel('Personal Access Token')?.value).toBe('ghp_typed'); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(onSave).toHaveBeenCalledWith( + 'github-bot', + expect.objectContaining({ + config: expect.objectContaining({ useLocalGh: false }), + secrets: { token: { operation: 'replace', value: 'ghp_typed' } }, + }), + ); + }); + + it('keeps a replacement PAT when local gh is switched on and back off', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { type: 'github', groupPolicy: 'open', useLocalGh: false }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_CONFIGURED, instance, onSave }); + + const replace = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Replace', + ); + await act(async () => { + replace!.click(); + }); + await act(async () => { + setInputValue(inputByLabel('Personal Access Token')!, 'ghp_replacement'); + }); + const toggle = document.querySelector( + 'button[role="switch"]', + ); + await act(async () => { + toggle!.click(); + }); + await act(async () => { + toggle!.click(); + }); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(onSave).toHaveBeenCalledWith( + 'github-bot', + expect.objectContaining({ + config: expect.objectContaining({ useLocalGh: false }), + secrets: { token: { operation: 'replace', value: 'ghp_replacement' } }, + }), + ); + }); + + it('clears a stored token when local gh authentication is switched on', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { type: 'github', groupPolicy: 'open', useLocalGh: false }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_CONFIGURED, instance, onSave }); + + const toggle = document.querySelector( + 'button[role="switch"]', + ); + await act(async () => { + toggle!.click(); + }); + + // The stored credential cannot be re-opened for editing while gh is + // selected, otherwise Replace would silently outrank the switch again. + for (const label of ['Keep', 'Replace', 'Clear']) { + const button = Array.from(document.querySelectorAll('button')).find( + (item) => item.textContent?.trim() === label, + ); + expect(button).toBeDefined(); + expect((button as HTMLButtonElement).disabled).toBe(true); + } + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(onSave).toHaveBeenCalledWith( + 'github-bot', + expect.objectContaining({ + config: expect.objectContaining({ useLocalGh: true }), + secrets: { token: { operation: 'clear' } }, + }), + ); + }); + + it('refuses to collapse repository aliases that carry different settings', async () => { + // GitHub treats both spellings as one repository, but the runtime resolves + // an exact key first. Collapsing them would keep the empty entry and widen + // the sender policy on a save the operator meant as a no-op. + const onSave = vi.fn().mockResolvedValue(undefined); + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + groupPolicy: 'allowlist', + groups: { + 'QwenLM/qwen-code': { senders: 'allowlist', allowedUsers: ['alice'] }, + 'qwenlm/qwen-code': {}, + }, + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_CONFIGURED, instance, onSave }); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(document.body.textContent).toContain( + 'This channel lists one repository twice with different settings', + ); + expect(onSave).not.toHaveBeenCalled(); + }); + + it('separates connection and notification fields from credentials', async () => { + await renderDialog({ descriptor: GITHUB_CONFIGURED }); + + expect(sectionHeadingOf(fieldByLabel('Base URL'))).toBe('Connection'); + expect(sectionHeadingOf(fieldByLabel('Reason Filter'))).toBe( + 'Notifications and routing', + ); + expect(sectionHeadingOf(inputByLabel('Personal Access Token'))).toBe( + 'Credentials', + ); + expect(inputByLabel('Base URL')?.placeholder).toBe( + 'https://ghe.example.com/api/v3', + ); + }); + + it('splits connection and routing fields for a non-GitHub channel', async () => { + await renderDialog({ descriptor: GITLAB_ROUTING }); + + expect(sectionHeadingOfLabel('Instance URL')).toBe('Connection'); + expect(sectionHeadingOfLabel('Personal Access Token')).toBe('Credentials'); + expect(sectionHeadingOfLabel('Message Routes')).toBe( + 'Notifications and routing', + ); + expect(sectionHeadingOfLabel('Default Message Route')).toBe( + 'Notifications and routing', + ); + }); + + it('asks for repository full names when GitHub groups are allowlisted', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ descriptor: GITHUB_CONFIGURED, onSave }); + + await selectOption('Group Policy', 'Allowlist'); + const repos = inputByLabel('Allowed repositories'); + expect(repos?.placeholder).toBe('owner/repo'); + + await act(async () => { + setInputValue(inputByLabel('Instance name')!, 'github-bot'); + setInputValue(inputByLabel('Personal Access Token')!, 'ghp_test'); + setInputValue(repos!, 'qwen-code'); + }); + + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + + expect(document.body.textContent).toContain( + 'Enter each repository as owner/repo.', + ); + expect(onSave).not.toHaveBeenCalled(); + }); + + it('hides the direct-message policy for a repository-only channel', async () => { + await renderDialog({ descriptor: GITHUB_CONFIGURED }); + + expect(document.body.textContent).not.toContain('Direct message policy'); + expect(document.body.textContent).not.toContain('Allowed user IDs'); + }); + + it('asks a repository-only channel to save group pairing before showing approvals', async () => { + // Channels saved by the previous editor carry the injected + // privatePolicy: 'pairing'. That legacy value must not open the + // direct-message pairing panel when the operator switches the *group* + // policy to pairing in the draft. + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + groupPolicy: 'open', + privatePolicy: 'pairing', + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + await renderDialog({ descriptor: GITHUB_CONFIGURED, instance }); + + await selectOption('Group Policy', 'Pairing'); + + expect(document.body.textContent).toContain('Save pairing mode first'); + expect(document.body.textContent).not.toContain('Pairing approvals'); + }); + + it('still shows group pairing approvals for a repository-only channel', async () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { + type: 'github', + groupPolicy: 'pairing', + allowedUsers: ['alice'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const listPairingRequests = vi.fn().mockResolvedValue({ requests: [] }); + await renderDialog({ + descriptor: GITHUB_CONFIGURED, + instance, + listPairingRequests, + }); + + expect(document.body.textContent).toContain('Pairing approvals'); + expect(listPairingRequests).toHaveBeenCalledWith('github-bot'); + // The private user list is not the effective sender allowlist for a + // repository, so it must not be presented as one. + expect(document.body.textContent).not.toContain('Configured allowlist'); + expect(document.body.textContent).not.toContain('alice'); + }); + it('does not show the allowlist alert when no users are configured', async () => { const pairingNoAllowlist: DaemonChannelInstanceSnapshot = { ...INSTANCE, diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx index 1b9dafc5e0b..f8b0d246777 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx @@ -60,12 +60,18 @@ import { hasDescriptorGroupPolicy, hasDescriptorPrivatePolicy, configuredPrivatePolicy, + PRIVATE_ONLY_FIELD_KEYS, + toList, validateChannelEditorDraft, type ChannelEditorDraft, + type ChannelSecretDraft, type ChannelEditorValidationCode, type ChannelGroupSenders, } from './channel-editor-state'; -import { PLATFORM_MARKS } from './channel-platform'; +import { + isRepositoryOnlyChannelType, + PLATFORM_MARKS, +} from './channel-platform'; const FIELD_LABEL_KEYS: Record> = { dingtalk: { @@ -125,6 +131,26 @@ const SHARED_FIELD_LABEL_KEYS: Record = { instructions: 'channels.editor.field.shared.instructions', }; +/** Per-platform placeholders that spell out the expected value shape. */ +const FIELD_PLACEHOLDER_KEYS: Record = { + 'github.baseUrl': 'https://ghe.example.com/api/v3', +}; + +const CONNECTION_FIELD_KEYS = new Set(['baseUrl', 'wsUrl']); +const ROUTING_FIELD_KEYS = new Set([ + 'reasonFilter', + 'messageRoutes', + 'defaultMessageRoute', +]); + +function fieldSectionKey( + field: DaemonChannelConfigFieldDescriptor, +): 'credentials' | 'connection' | 'routing' { + if (CONNECTION_FIELD_KEYS.has(field.key)) return 'connection'; + if (ROUTING_FIELD_KEYS.has(field.key)) return 'routing'; + return 'credentials'; +} + export interface ChannelEditorDialogProps { open: boolean; descriptor: DaemonChannelTypeDescriptor; @@ -239,6 +265,8 @@ export function ChannelEditorDialog({ const [saving, setSaving] = useState(false); const [reloading, setReloading] = useState(false); const dismissedRef = useRef(false); + /** The token draft as it stood before local gh auth overrode it. */ + const tokenBeforeLocalGh = useRef(undefined); const accessFields = descriptor.fields.filter((field) => SHARED_ACCESS_FIELD_KEYS.has(field.key), ); @@ -268,6 +296,7 @@ export function ChannelEditorDialog({ useEffect(() => { if (!open) return; dismissedRef.current = false; + tokenBeforeLocalGh.current = undefined; setDraft(createChannelEditorDraft(descriptor, instance)); setErrors({}); setSubmitError(undefined); @@ -324,6 +353,10 @@ export function ChannelEditorDialog({ if (code === 'invalid') return t('channels.editor.validation.invalidName'); if (code === 'invalidGroupId') return t('channels.editor.validation.invalidGroupId'); + if (code === 'invalidRepoId') + return t('channels.editor.validation.invalidRepoId'); + if (code === 'conflictingRepoAliases') + return t('channels.editor.validation.conflictingRepoAliases'); if (code === 'invalidOption') return t('channels.editor.validation.invalidOption'); if (code === 'number') return t('channels.editor.validation.number'); @@ -345,6 +378,7 @@ export function ChannelEditorDialog({ descriptor, draft, existingNames, + instance, ); if (Object.keys(validation).length > 0) { setErrors( @@ -400,7 +434,13 @@ export function ChannelEditorDialog({ value: '', }; const error = errors[field.key]; - const showInput = secret.operation === 'replace'; + // The GitHub channel reads a token before local gh auth, so an editable + // token while local gh is selected would silently win over the switch. + const lockedByLocalGh = + field.key === 'token' && + isRepositoryOnlyChannelType(descriptor.type) && + draft.values['useLocalGh'] === true; + const showInput = secret.operation === 'replace' && !lockedByLocalGh; const operations = field.required ? (['preserve', 'replace'] as const) : (['preserve', 'replace', 'clear'] as const); @@ -412,9 +452,11 @@ export function ChannelEditorDialog({ required={field.required} description={fieldDescription(field)} hint={ - field.envResolvable - ? t('channels.editor.environmentReference') - : undefined + lockedByLocalGh + ? t('channels.editor.field.github.token.localGhHint') + : field.envResolvable + ? t('channels.editor.environmentReference') + : undefined } error={error} > @@ -435,6 +477,7 @@ export function ChannelEditorDialog({ variant={ secret.operation === operation ? 'secondary' : 'ghost' } + disabled={lockedByLocalGh} aria-pressed={secret.operation === operation} onClick={() => setDraft((current) => ({ @@ -505,6 +548,46 @@ export function ChannelEditorDialog({ }, })); if (field.kind === 'boolean') { + const isLocalGhSwitch = + field.key === 'useLocalGh' && + isRepositoryOnlyChannelType(descriptor.type); + const updateSwitch = (checked: boolean) => { + if (!isLocalGhSwitch) { + update(checked); + return; + } + // Read both outside the updater: it runs lazily during the next + // render, by which point the ref below has already been reassigned. + const tokenBeforeToggle = draft.secrets['token']; + const tokenToRestore = tokenBeforeLocalGh.current; + setDraft((current) => { + const secrets = { ...current.secrets }; + if (checked) { + const hasToken = + instance?.secrets['token']?.present === true || + Boolean(current.secrets['token']?.value?.trim()); + // A stored token outranks local gh auth; clear it so the switch + // actually takes effect when the draft is saved. + if (hasToken) secrets['token'] = { operation: 'clear' }; + } else if (secrets['token']?.operation === 'clear') { + // The switch is a preference: flipping it back restores whatever + // the operator had, including a PAT they just typed. Falling back + // to `preserve` unconditionally would discard a new or replacement + // token and submit the wrong credential operation. + secrets['token'] = + tokenToRestore ?? + (instance?.secrets['token']?.present + ? { operation: 'preserve' } + : { operation: 'replace', value: '' }); + } + return { + ...current, + values: { ...current.values, [field.key]: checked }, + secrets, + }; + }); + tokenBeforeLocalGh.current = checked ? tokenBeforeToggle : undefined; + }; return ( update(checked)} + onCheckedChange={updateSwitch} /> ); @@ -552,6 +635,83 @@ export function ChannelEditorDialog({ ); } + if (field.kind === 'string-list' && field.options?.length) { + const options = field.options; + const selected = toList(String(value ?? '')); + // The adapter matches these values case-insensitively, so a stored + // `COMMENT` has to tick the canonical `comment` box instead of rendering + // a second, stray entry beside it. + const selectedKeys = new Set( + selected.map((entry) => entry.toLowerCase()), + ); + const known = new Set( + options.map((option) => option.value.toLowerCase()), + ); + // Values written by hand that the descriptor does not know stay visible + // and removable instead of disappearing on the next save. + const stored = selected.filter( + (entry) => !known.has(entry.toLowerCase()), + ); + const toggleValue = (target: string, checked: boolean) => { + const next = new Set(selectedKeys); + if (checked) next.add(target.toLowerCase()); + else next.delete(target.toLowerCase()); + update( + [ + ...options + .map((option) => option.value) + .filter((entry) => next.has(entry.toLowerCase())), + ...stored.filter((entry) => next.has(entry.toLowerCase())), + ].join(', '), + ); + }; + return ( + +

+ {[ + ...options, + ...stored.map((entry) => ({ value: entry, label: entry })), + ].map((option) => { + const isSelected = selectedKeys.has(option.value.toLowerCase()); + const optionLabel = fieldOptionLabel( + field, + option.value, + option.label, + ); + return ( + + ); + })} +
+ + ); + } if (field.kind === 'record') { let record: Record = {}; if (typeof value === 'string' && value) { @@ -643,6 +803,9 @@ export function ChannelEditorDialog({ id={id} type={field.kind === 'number' ? 'number' : 'text'} value={String(value ?? '')} + placeholder={ + FIELD_PLACEHOLDER_KEYS[`${descriptor.type}.${field.key}`] + } aria-invalid={Boolean(error)} aria-required={field.required} onChange={(event) => update(event.target.value)} @@ -772,12 +935,22 @@ export function ChannelEditorDialog({ -
-

- {t('channels.editor.section.credentials')} -

- {credentialFields.map(renderField)} -
+ {(['credentials', 'connection', 'routing'] as const).map( + (section) => { + const sectionFields = credentialFields.filter( + (field) => fieldSectionKey(field) === section, + ); + if (sectionFields.length === 0) return null; + return ( +
+

+ {t(`channels.editor.section.${section}`)} +

+ {sectionFields.map(renderField)} +
+ ); + }, + )} {sessionFields.length > 0 ? (
@@ -851,11 +1024,16 @@ export function ChannelEditorDialog({ ) : null} {(() => { + const repositoryOnly = isRepositoryOnlyChannelType( + descriptor.type, + ); const descriptorPolicy = hasDescriptorPrivatePolicy(descriptor); - const effectivePolicy = descriptorPolicy - ? String(draft.values['privatePolicy'] ?? '') - : draft.privatePolicy; - const showRadioGroup = !descriptorPolicy; + const effectivePolicy = repositoryOnly + ? '' + : descriptorPolicy + ? String(draft.values['privatePolicy'] ?? '') + : draft.privatePolicy; + const showRadioGroup = !descriptorPolicy && !repositoryOnly; const descriptorGroupPolicy = hasDescriptorGroupPolicy(descriptor); const effectiveGroupPolicy = descriptorGroupPolicy @@ -867,6 +1045,7 @@ export function ChannelEditorDialog({ const visibleAccessFields = accessFields.filter( (field) => field.key !== 'operators' && + !(repositoryOnly && PRIVATE_ONLY_FIELD_KEYS.has(field.key)) && (field.key !== 'allowedUsers' || effectivePolicy === 'allowlist' || effectivePolicy === 'pairing'), @@ -951,9 +1130,15 @@ export function ChannelEditorDialog({ {effectiveGroupPolicy === 'allowlist' ? ( @@ -962,7 +1147,9 @@ export function ChannelEditorDialog({ value={draft.allowedGroupIds} aria-invalid={Boolean(errors['allowedGroupIds'])} placeholder={t( - 'channels.editor.field.shared.allowedGroupIds.placeholder', + repositoryOnly + ? 'channels.editor.field.github.allowedRepos.placeholder' + : 'channels.editor.field.shared.allowedGroupIds.placeholder', )} onChange={(event) => setDraft((current) => ({ @@ -1035,16 +1222,19 @@ export function ChannelEditorDialog({ ) : null} {operatorsField ? renderField(operatorsField) : null} {showPairing ? ( - (instance && + instance && + ((!repositoryOnly && configuredPrivatePolicy(instance) === 'pairing') || - instance?.config.groupPolicy === 'pairing' ? ( + instance.config.groupPolicy === 'pairing') ? ( ) : ( diff --git a/packages/web-shell/client/components/channels/channel-editor-state.test.ts b/packages/web-shell/client/components/channels/channel-editor-state.test.ts index 4c0d1d1e06d..4a699568531 100644 --- a/packages/web-shell/client/components/channels/channel-editor-state.test.ts +++ b/packages/web-shell/client/components/channels/channel-editor-state.test.ts @@ -1009,14 +1009,19 @@ describe('Descriptor-driven privatePolicy', () => { }); it('writes privatePolicy via descriptor fields, not the hardcoded path', () => { - const draft = createChannelEditorDraft(GITHUB); + const descriptor: DaemonChannelTypeDescriptor = { + ...GITHUB, + type: 'dws', + displayName: 'DWS', + }; + const draft = createChannelEditorDraft(descriptor); draft.name = 'my-bot'; draft.secrets.token = { operation: 'replace', value: 'ghp_test' }; draft.values.allowedUsers = 'alice, bob'; - const request = buildChannelUpsertRequest(GITHUB, draft, 'rev-1'); + const request = buildChannelUpsertRequest(descriptor, draft, 'rev-1'); expect(request.config).toEqual({ - type: 'github', + type: 'dws', useLocalGh: false, groupPolicy: 'open', privatePolicy: 'allowlist', @@ -1196,3 +1201,212 @@ describe('Descriptor-driven privatePolicy', () => { }); }); }); + +// The registry injects privatePolicy/allowedUsers into every manageable +// channel, but GitHub traffic is always group traffic, so the adapter never +// reads them and group IDs are repository full names. +describe('Repository-only channel access config', () => { + it('does not persist the direct-message policy', () => { + const draft = createChannelEditorDraft(GITHUB); + draft.name = 'release-bot'; + draft.secrets.token = { operation: 'replace', value: 'ghp_test' }; + draft.values.allowedUsers = 'alice'; + + const request = buildChannelUpsertRequest(GITHUB, draft, 'rev-1'); + expect(request.config).toEqual({ + type: 'github', + useLocalGh: false, + groupPolicy: 'open', + }); + }); + + it('drops the private-access residue stored by an earlier save', () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'release-bot', + config: { + type: 'github', + groupPolicy: 'open', + privatePolicy: 'pairing', + allowedUsers: ['alice'], + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(GITHUB, instance); + + const request = buildChannelUpsertRequest(GITHUB, draft, 'rev-2', instance); + expect(request.config).not.toHaveProperty('privatePolicy'); + expect(request.config).not.toHaveProperty('allowedUsers'); + }); + + it('requires owner/repo for an allowlisted repository', () => { + const draft = createChannelEditorDraft(GITHUB); + draft.name = 'release-bot'; + draft.secrets.token = { operation: 'replace', value: 'ghp_test' }; + draft.values.groupPolicy = 'allowlist'; + draft.allowedGroupIds = 'qwen-code'; + + expect(validateChannelEditorDraft(GITHUB, draft, [])).toEqual({ + allowedGroupIds: 'invalidRepoId', + }); + + draft.allowedGroupIds = 'QwenLM/qwen-code, acme/widgets.js'; + expect(validateChannelEditorDraft(GITHUB, draft, [])).toEqual({}); + }); + + it('stores repository allowlists under the canonical lower-case key', () => { + // The adapter matches the canonical `owner/repo` GitHub reports, so the + // editor must not certify a spelling that never matches at runtime. + for (const typed of ['qwenlm/qwen-code', 'QwenLM/Qwen-Code']) { + const draft = createChannelEditorDraft(GITHUB); + draft.name = 'release-bot'; + draft.secrets.token = { operation: 'replace', value: 'ghp_test' }; + draft.values.groupPolicy = 'allowlist'; + draft.allowedGroupIds = typed; + + const request = buildChannelUpsertRequest(GITHUB, draft, 'rev-1'); + expect(Object.keys(request.config['groups'] as object)).toEqual([ + 'qwenlm/qwen-code', + ]); + } + }); + + it('refuses to save when stored repository aliases carry different settings', () => { + // Both spellings are one repository to GitHub, but the runtime resolves an + // exact key first. Collapsing them would keep the empty entry and drop the + // sender restriction on a save the operator meant as a no-op. + const instance: DaemonChannelInstanceSnapshot = { + name: 'release-bot', + config: { + type: 'github', + groupPolicy: 'allowlist', + groups: { + 'QwenLM/qwen-code': { senders: 'allowlist', allowedUsers: ['alice'] }, + 'qwenlm/qwen-code': {}, + }, + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(GITHUB, instance); + + expect(draft.allowedGroupIds).toBe('qwenlm/qwen-code'); + expect(validateChannelEditorDraft(GITHUB, draft, [], instance)).toEqual({ + allowedGroupIds: 'conflictingRepoAliases', + }); + }); + + it('collapses repository aliases that carry identical settings', () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'release-bot', + config: { + type: 'github', + groupPolicy: 'allowlist', + groups: { + 'QwenLM/qwen-code': { senders: 'allowlist', allowedUsers: ['alice'] }, + 'qwenlm/qwen-code': { senders: 'allowlist', allowedUsers: ['alice'] }, + }, + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(GITHUB, instance); + + expect(validateChannelEditorDraft(GITHUB, draft, [], instance)).toEqual({}); + const request = buildChannelUpsertRequest(GITHUB, draft, 'rev-2', instance); + expect(request.config['groups']).toEqual({ + 'qwenlm/qwen-code': { senders: 'allowlist', allowedUsers: ['alice'] }, + }); + }); + + it('treats aliases that differ only in key or allowlist order as identical', () => { + // Neither ordering carries meaning, so reporting a conflict here would + // block an unrelated save with a message that does not match the reason. + const orderings: Array<[Record, Record]> = + [ + [ + { senders: 'allowlist', requireMention: true }, + { requireMention: true, senders: 'allowlist' }, + ], + [ + { senders: 'allowlist', allowedUsers: ['alice', 'bob'] }, + { senders: 'allowlist', allowedUsers: ['bob', 'alice'] }, + ], + ]; + for (const [first, second] of orderings) { + const instance: DaemonChannelInstanceSnapshot = { + name: 'release-bot', + config: { + type: 'github', + groupPolicy: 'allowlist', + groups: { 'QwenLM/qwen-code': first, 'qwenlm/qwen-code': second }, + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(GITHUB, instance); + + expect(validateChannelEditorDraft(GITHUB, draft, [], instance)).toEqual( + {}, + ); + const request = buildChannelUpsertRequest( + GITHUB, + draft, + 'rev-2', + instance, + ); + expect(Object.keys(request.config['groups'] as object)).toEqual([ + 'qwenlm/qwen-code', + ]); + } + }); + + it('keeps per-repository settings when the allowlist case changes', () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'release-bot', + config: { + type: 'github', + groupPolicy: 'allowlist', + groups: { + 'QwenLM/qwen-code': { + senders: 'allowlist', + allowedUsers: ['alice'], + }, + }, + }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(GITHUB, instance); + expect(draft.allowedGroupIds).toBe('qwenlm/qwen-code'); + + const request = buildChannelUpsertRequest(GITHUB, draft, 'rev-2', instance); + expect(request.config['groups']).toEqual({ + 'qwenlm/qwen-code': { senders: 'allowlist', allowedUsers: ['alice'] }, + }); + }); + + it('drops a stored token once local gh authentication is selected', () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'release-bot', + config: { type: 'github', useLocalGh: true, groupPolicy: 'open' }, + secrets: { token: { present: true, source: 'literal' } }, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + + const draft = createChannelEditorDraft(GITHUB, instance); + + expect(draft.secrets.token).toEqual({ operation: 'clear' }); + expect(buildChannelUpsertRequest(GITHUB, draft, 'rev-2', instance)).toEqual( + expect.objectContaining({ + secrets: { token: { operation: 'clear' } }, + }), + ); + }); +}); diff --git a/packages/web-shell/client/components/channels/channel-editor-state.ts b/packages/web-shell/client/components/channels/channel-editor-state.ts index bb26f4fec26..48ab5e452b3 100644 --- a/packages/web-shell/client/components/channels/channel-editor-state.ts +++ b/packages/web-shell/client/components/channels/channel-editor-state.ts @@ -11,6 +11,7 @@ import type { DaemonChannelTypeDescriptor, DaemonChannelUpsertRequest, } from '@qwen-code/sdk/daemon'; +import { isRepositoryOnlyChannelType } from './channel-platform'; export type ChannelPrivatePolicy = | 'disabled' @@ -43,6 +44,8 @@ export type ChannelEditorValidationCode = | 'invalid' | 'invalidGroupId' | 'invalidOption' + | 'invalidRepoId' + | 'conflictingRepoAliases' | 'number' | 'outOfRange' | 'policy'; @@ -54,6 +57,21 @@ export type ChannelEditorValidationErrors = Record< const UNSAFE_OBJECT_KEYS = ['__proto__', 'constructor', 'prototype']; +/** Access fields that only gate direct messages, which repository-only channels never receive. */ +export const PRIVATE_ONLY_FIELD_KEYS = new Set([ + 'privatePolicy', + 'allowedUsers', +]); + +const REPOSITORY_FULL_NAME_RE = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; + +export function toList(value: string): string[] { + return value + .split(',') + .map((item) => item.trim()) + .filter(Boolean); +} + export function hasDescriptorPrivatePolicy( descriptor: DaemonChannelTypeDescriptor, ): boolean { @@ -190,6 +208,17 @@ export function createChannelEditorDraft( const configuredPolicy = instance ? configuredPrivatePolicy(instance) : 'pairing'; + const repositoryOnly = isRepositoryOnlyChannelType(descriptor.type); + if ( + repositoryOnly && + values['useLocalGh'] === true && + instance?.secrets['token']?.present + ) { + // A stored token outranks local gh auth, so keeping it would silently + // defeat the authentication method this channel has selected. + secrets['token'] = { operation: 'clear' }; + } + const configuredGroups = configuredGroupIds(instance); return { name: instance?.name ?? '', values, @@ -199,7 +228,12 @@ export function createChannelEditorDraft( : ['disabled', 'allowlist', 'pairing', 'open'].includes(configuredPolicy) ? (configuredPolicy as ChannelPrivatePolicy) : '', - allowedGroupIds: configuredGroupIds(instance), + // Show repository IDs in the canonical lower-case form the editor writes, + // so re-saving an older mixed-case allowlist migrates it instead of + // rewriting the same spelling back. + allowedGroupIds: repositoryOnly + ? [...new Set(toList(configuredGroups.toLowerCase()))].join(', ') + : configuredGroups, groupSenders: configuredGroupSenders(instance), groupAllowedUsers: configuredGroupAllowedUsers(instance), }; @@ -235,6 +269,7 @@ export function validateChannelEditorDraft( descriptor: DaemonChannelTypeDescriptor, draft: ChannelEditorDraft, existingNames: readonly string[], + instance?: DaemonChannelInstanceSnapshot, ): ChannelEditorValidationErrors { const errors: ChannelEditorValidationErrors = {}; const name = draft.name.trim(); @@ -285,7 +320,11 @@ export function validateChannelEditorDraft( errors['token'] = 'credential'; } } - if (!draft.privatePolicy && !hasDescriptorPrivatePolicy(descriptor)) { + if ( + !isRepositoryOnlyChannelType(descriptor.type) && + !draft.privatePolicy && + !hasDescriptorPrivatePolicy(descriptor) + ) { errors['privatePolicy'] = 'policy'; } if ( @@ -296,9 +335,71 @@ export function validateChannelEditorDraft( ) { errors['allowedGroupIds'] = 'invalidGroupId'; } + if ( + isRepositoryOnlyChannelType(descriptor.type) && + String(draft.values['groupPolicy'] ?? '') === 'allowlist' + ) { + if ( + splitList(draft.allowedGroupIds).some( + (groupId) => !REPOSITORY_FULL_NAME_RE.test(groupId), + ) + ) { + errors['allowedGroupIds'] = 'invalidRepoId'; + } + if (hasConflictingRepositoryAliases(instance)) { + // Collapsing these keys would keep only one of two different sender + // policies, and the runtime prefers an exact key — so a no-op save could + // widen access. Make the operator resolve it instead of choosing for them. + errors['allowedGroupIds'] = 'conflictingRepoAliases'; + } + } return errors; } +/** + * Whether the stored allowlist holds two spellings of one repository that + * carry different settings. GitHub treats them as the same repository, but + * the runtime resolves an exact key first, so they are not interchangeable. + */ +function hasConflictingRepositoryAliases( + instance?: DaemonChannelInstanceSnapshot, +): boolean { + const groups = instance?.config['groups']; + if (!isRecord(groups)) return false; + const shapes = new Map>(); + for (const [groupId, group] of Object.entries(groups)) { + if (groupId === '*') continue; + const key = groupId.toLowerCase(); + const shape = canonicalGroupShape(group); + shapes.set(key, (shapes.get(key) ?? new Set()).add(shape)); + } + for (const variants of shapes.values()) { + if (variants.size > 1) return true; + } + return false; +} + +/** + * Compare group settings by meaning, not by JSON text: key order carries no + * meaning, and the only list a group holds is an allowlist, whose order does + * not matter either. Comparing the raw text would report two identical + * entries written in a different order as a conflict and block an unrelated + * save. + */ +function canonicalGroupShape(value: unknown): string { + if (Array.isArray(value)) { + return JSON.stringify(value.map(canonicalGroupShape).sort()); + } + if (isRecord(value)) { + return JSON.stringify( + Object.keys(value) + .sort() + .map((key) => [key, canonicalGroupShape(value[key])]), + ); + } + return JSON.stringify(value ?? null); +} + function assignField( config: Record, field: DaemonChannelConfigFieldDescriptor, @@ -356,32 +457,36 @@ function assignField( } function splitList(value: string): string[] { - return [ - ...new Set( - value - .split(',') - .map((item) => item.trim()) - .filter(Boolean), - ), - ]; + return [...new Set(toList(value))]; } function assignGroups( config: Record, allowedGroupIds: string, instance?: DaemonChannelInstanceSnapshot, + normalizeKeys = false, ): void { const previous = instance?.config['groups']; - const previousGroups = isRecord(previous) ? previous : {}; + const storedGroups = isRecord(previous) ? previous : {}; + // Repository IDs are case-insensitive. Store the canonical lower-case key + // and look the previous per-repository settings up under it, so retyping the + // case does not silently drop that repository's sender policy. + const previousGroups = normalizeKeys + ? Object.fromEntries( + Object.entries(storedGroups).map(([key, value]) => [ + key.toLowerCase(), + value, + ]), + ) + : storedGroups; const groups: Record = {}; if (isRecord(previousGroups['*'])) { groups['*'] = previousGroups['*']; } for (const groupId of splitList(allowedGroupIds)) { - if (groupId === '*') continue; - groups[groupId] = isRecord(previousGroups[groupId]) - ? previousGroups[groupId] - : {}; + const key = normalizeKeys ? groupId.toLowerCase() : groupId; + if (key === '*') continue; + groups[key] = isRecord(previousGroups[key]) ? previousGroups[key] : {}; } if (Object.keys(groups).length > 0) { config['groups'] = groups; @@ -436,9 +541,11 @@ export function buildChannelUpsertRequest( ...(instance?.config ?? {}), type: descriptor.type, }; + const repositoryOnly = isRepositoryOnlyChannelType(descriptor.type); const secrets: Record = {}; for (const field of descriptor.fields) { if (field.kind === 'object') continue; + if (repositoryOnly && PRIVATE_ONLY_FIELD_KEYS.has(field.key)) continue; if (field.kind === 'secret') { const secret = draft.secrets[field.key] ?? { operation: 'preserve' }; secrets[field.key] = @@ -451,12 +558,18 @@ export function buildChannelUpsertRequest( } assignField(config, field, draft.values[field.key]); } - if (!hasDescriptorPrivatePolicy(descriptor)) { + if (repositoryOnly) { + // The editor used to persist the injected private-access defaults here + // even though these channels never read them, and the pairing panel would + // then present the stale user list as an authoritative allowlist. Drop the + // whole set on the next save; the effective list lives under `groups`. + for (const key of PRIVATE_ONLY_FIELD_KEYS) delete config[key]; + } else if (!hasDescriptorPrivatePolicy(descriptor)) { config['privatePolicy'] = draft.privatePolicy; } if (hasDescriptorGroupPolicy(descriptor)) { if (config['groupPolicy'] === 'allowlist') { - assignGroups(config, draft.allowedGroupIds, instance); + assignGroups(config, draft.allowedGroupIds, instance, repositoryOnly); } else if (instance?.config['groupPolicy'] === 'allowlist') { removeGroupAllowlistMembership(config, instance); } diff --git a/packages/web-shell/client/components/channels/channel-platform.ts b/packages/web-shell/client/components/channels/channel-platform.ts index dd473156afa..d4ea6150868 100644 --- a/packages/web-shell/client/components/channels/channel-platform.ts +++ b/packages/web-shell/client/components/channels/channel-platform.ts @@ -44,3 +44,14 @@ export function isChannelPlatformAvailable( ): boolean { return descriptor.manageable && isSupportedChannelType(descriptor.type); } + +/** + * Channels whose traffic is always repository-scoped group traffic. Every + * envelope sets `isGroup`, so the direct-message policy never gates them, and + * their group IDs are repository full names (`owner/repo`). + */ +const REPOSITORY_ONLY_CHANNEL_TYPES = new Set(['github']); + +export function isRepositoryOnlyChannelType(type: unknown): boolean { + return typeof type === 'string' && REPOSITORY_ONLY_CHANNEL_TYPES.has(type); +} diff --git a/packages/web-shell/client/e2e/visuals/screenshots.spec.ts b/packages/web-shell/client/e2e/visuals/screenshots.spec.ts index f4c1195d02a..22b1296766f 100644 --- a/packages/web-shell/client/e2e/visuals/screenshots.spec.ts +++ b/packages/web-shell/client/e2e/visuals/screenshots.spec.ts @@ -1412,6 +1412,17 @@ for (const theme of THEMES) { { value: 'disabled', label: 'Disabled' }, ], }, + { + key: 'reasonFilter', + label: 'Reason Filter', + kind: 'string-list', + options: [ + { value: 'mention', label: 'mention' }, + { value: 'review_requested', label: 'review_requested' }, + { value: 'assign', label: 'assign' }, + { value: 'ci_activity', label: 'ci_activity' }, + ], + }, { key: 'privatePolicy', label: 'Private Policy', @@ -1457,7 +1468,25 @@ for (const theme of THEMES) { name: 'Use local GitHub CLI authentication', }), ).toBeVisible(); + // Anchor the capture to the checklist so a regression that falls back to + // a free-text filter fails here instead of shipping a stale picture. + const reasonFilter = page.getByRole('group', { name: 'Reason Filter' }); + await expect(reasonFilter.getByRole('checkbox')).toHaveCount(4); + await expect( + reasonFilter.getByRole('checkbox', { + name: /mention — the bot is @mentioned/, + }), + ).toBeChecked({ checked: false }); await captureScreenshot(page, `github-channel-editor-${theme}`); + // The dialog body scrolls, and the checklist sits below the fold at this + // viewport. Capture it separately and pin that it is fully on screen, so + // the published picture cannot quietly omit the feature it documents. + await page + .getByText('Reason Filter', { exact: true }) + .first() + .scrollIntoViewIfNeeded(); + await expect(reasonFilter).toBeInViewport({ ratio: 1 }); + await captureScreenshot(page, `github-channel-editor-reasons-${theme}`); await page.getByLabel('Instance name').fill('github-bot'); await page.getByRole('button', { name: 'Save' }).click(); await expect( diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index 50f777fb030..986c0ee4091 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -4393,6 +4393,8 @@ const EN: Messages = { 'Update public settings or explicitly change stored credentials.', 'channels.editor.section.identity': 'Identity', 'channels.editor.section.credentials': 'Credentials', + 'channels.editor.section.connection': 'Connection', + 'channels.editor.section.routing': 'Notifications and routing', 'channels.editor.section.session': 'Conversation management', 'channels.editor.section.access': 'Access control', 'channels.editor.section.access.description': @@ -4424,6 +4426,12 @@ const EN: Messages = { 'channels.editor.field.github.token': 'Personal Access Token', 'channels.editor.field.github.token.description': 'Optional classic PAT with "notifications" scope. Overrides local gh authentication', + 'channels.editor.field.github.token.localGhHint': + 'Local gh authentication is selected. The token cannot be edited here, and any stored token is removed on save.', + 'channels.editor.field.github.allowedRepos': 'Allowed repositories', + 'channels.editor.field.github.allowedRepos.description': + 'Comma-separated repository full names, for example owner/repo.', + 'channels.editor.field.github.allowedRepos.placeholder': 'owner/repo', 'channels.editor.field.github.useLocalGh': 'Use local GitHub CLI authentication', 'channels.editor.field.github.useLocalGh.description': @@ -4436,7 +4444,37 @@ const EN: Messages = { 'Must be "Open", "Allowlist", or "Pairing" for notifications to flow', 'channels.editor.field.github.reasonFilter': 'Reason Filter', 'channels.editor.field.github.reasonFilter.description': - 'Optional. Comma-separated notification reasons to process. Valid values: mention, review_requested, assign, author, comment, ci_activity, manual, state_change, subscribed, team_mention, security_alert, approval_requested, invitation, member_feature_requested, security_advisory_credit. Leave empty to process all.', + 'Optional. Only notifications with a checked reason are processed. Leave every reason unchecked to process all of them.', + 'channels.editor.field.github.reasonFilter.option.mention': + 'mention — the bot is @mentioned', + 'channels.editor.field.github.reasonFilter.option.review_requested': + 'review_requested — the bot, or a team it belongs to, was asked to review a pull request', + 'channels.editor.field.github.reasonFilter.option.assign': + 'assign — the bot was assigned to the issue', + 'channels.editor.field.github.reasonFilter.option.author': + 'author — the bot created the thread', + 'channels.editor.field.github.reasonFilter.option.comment': + 'comment — the bot commented on the thread', + 'channels.editor.field.github.reasonFilter.option.ci_activity': + 'ci_activity — a workflow run the bot triggered completed', + 'channels.editor.field.github.reasonFilter.option.manual': + 'manual — the bot subscribed to the thread from an issue or pull request', + 'channels.editor.field.github.reasonFilter.option.state_change': + 'state_change — the bot changed the thread state, such as closing an issue', + 'channels.editor.field.github.reasonFilter.option.subscribed': + 'subscribed — the bot is watching the repository', + 'channels.editor.field.github.reasonFilter.option.team_mention': + 'team_mention — a team the bot belongs to is @mentioned', + 'channels.editor.field.github.reasonFilter.option.security_alert': + 'security_alert — GitHub found a security vulnerability in the repository', + 'channels.editor.field.github.reasonFilter.option.approval_requested': + 'approval_requested — the bot was asked to review and approve a deployment', + 'channels.editor.field.github.reasonFilter.option.invitation': + 'invitation — the bot accepted an invitation to contribute to the repository', + 'channels.editor.field.github.reasonFilter.option.member_feature_requested': + 'member_feature_requested — organization members asked to enable a feature such as Copilot', + 'channels.editor.field.github.reasonFilter.option.security_advisory_credit': + 'security_advisory_credit — the bot was credited for contributing to a security advisory', 'channels.editor.field.gitlab.token': 'Personal Access Token', 'channels.editor.field.gitlab.token.description': 'PAT with "read_api" + "api" scopes', @@ -4612,6 +4650,10 @@ const EN: Messages = { 'channels.editor.validation.invalidName': 'Choose a different instance name.', 'channels.editor.validation.invalidGroupId': 'Enter a group ID other than __proto__, constructor, or prototype.', + 'channels.editor.validation.invalidRepoId': + 'Enter each repository as owner/repo.', + 'channels.editor.validation.conflictingRepoAliases': + 'This channel lists one repository twice with different settings, spelled differently (for example Owner/repo and owner/repo). Remove one of the entries from settings.json before saving.', 'channels.editor.validation.invalidOption': "Remove values that aren't in the allowed list.", 'channels.editor.validation.number': 'Enter a valid number.', @@ -8724,6 +8766,8 @@ const ZH: Messages = { 'channels.editor.editDescription': '更新公开配置，或明确更改已保存的凭据。', 'channels.editor.section.identity': '频道标识', 'channels.editor.section.credentials': '应用凭据', + 'channels.editor.section.connection': '连接', + 'channels.editor.section.routing': '通知与路由', 'channels.editor.section.session': '会话管理', 'channels.editor.section.access': '访问控制', 'channels.editor.section.access.description': @@ -8755,6 +8799,12 @@ const ZH: Messages = { 'channels.editor.field.github.token': '个人访问令牌', 'channels.editor.field.github.token.description': '可选。填写具有 "notifications" 权限的经典 PAT；优先于本地 gh 认证', + 'channels.editor.field.github.token.localGhHint': + '已选择本地 gh 认证：此处无法编辑令牌，保存时会移除已存的令牌。', + 'channels.editor.field.github.allowedRepos': '允许的仓库', + 'channels.editor.field.github.allowedRepos.description': + '逗号分隔的仓库全名，例如 owner/repo。', + 'channels.editor.field.github.allowedRepos.placeholder': 'owner/repo', 'channels.editor.field.github.useLocalGh': '使用本地 GitHub CLI 认证', 'channels.editor.field.github.useLocalGh.description': '未配置令牌时，显式复用 daemon 主机上账户级的 gh 登录', @@ -8766,7 +8816,37 @@ const ZH: Messages = { '必须设为 "Open"、"Allowlist" 或 "Pairing" 才能接收通知', 'channels.editor.field.github.reasonFilter': '通知原因过滤', 'channels.editor.field.github.reasonFilter.description': - '可选。逗号分隔的通知原因。有效值：mention、review_requested、assign、author、comment、ci_activity、manual、state_change、subscribed、team_mention、security_alert、approval_requested、invitation、member_feature_requested、security_advisory_credit。留空则处理全部。', + '可选。只处理勾选原因的通知；全部不勾选则处理所有通知。', + 'channels.editor.field.github.reasonFilter.option.mention': + 'mention — 有人在评论或正文中 @ 了机器人', + 'channels.editor.field.github.reasonFilter.option.review_requested': + 'review_requested — 机器人或其所在团队被请求评审 PR', + 'channels.editor.field.github.reasonFilter.option.assign': + 'assign — 机器人被指派到该 issue', + 'channels.editor.field.github.reasonFilter.option.author': + 'author — 该线程由机器人创建', + 'channels.editor.field.github.reasonFilter.option.comment': + 'comment — 机器人在该线程里评论过', + 'channels.editor.field.github.reasonFilter.option.ci_activity': + 'ci_activity — 机器人触发的工作流运行已完成', + 'channels.editor.field.github.reasonFilter.option.manual': + 'manual — 机器人在 issue 或 PR 上订阅了该线程', + 'channels.editor.field.github.reasonFilter.option.state_change': + 'state_change — 机器人改变了线程状态，例如关闭 issue', + 'channels.editor.field.github.reasonFilter.option.subscribed': + 'subscribed — 机器人正在关注该仓库', + 'channels.editor.field.github.reasonFilter.option.team_mention': + 'team_mention — 机器人所在团队被 @', + 'channels.editor.field.github.reasonFilter.option.security_alert': + 'security_alert — GitHub 在仓库中发现了安全漏洞', + 'channels.editor.field.github.reasonFilter.option.approval_requested': + 'approval_requested — 机器人被请求审核并批准部署', + 'channels.editor.field.github.reasonFilter.option.invitation': + 'invitation — 机器人接受了参与该仓库的邀请', + 'channels.editor.field.github.reasonFilter.option.member_feature_requested': + 'member_feature_requested — 组织成员请求启用 Copilot 等功能', + 'channels.editor.field.github.reasonFilter.option.security_advisory_credit': + 'security_advisory_credit — 机器人因参与安全公告而被致谢', 'channels.editor.field.gitlab.token': '个人访问令牌', 'channels.editor.field.gitlab.token.description': '需要 "read_api" + "api" 权限的 PAT', @@ -8934,6 +9014,9 @@ const ZH: Messages = { 'channels.editor.validation.invalidName': '请使用其他实例名称。', 'channels.editor.validation.invalidGroupId': '群聊 ID 不能是 __proto__、constructor 或 prototype。', + 'channels.editor.validation.invalidRepoId': '仓库需写成 owner/repo 形式。', + 'channels.editor.validation.conflictingRepoAliases': + '该渠道把同一个仓库以不同大小写写了两次且设置不同（例如 Owner/repo 与 owner/repo）。请先在 settings.json 里删掉其中一条再保存。', 'channels.editor.validation.invalidOption': '请移除不在允许列表中的值。', 'channels.editor.validation.number': '请输入有效数字。', 'channels.editor.validation.outOfRange': (v) =>