feat(web-shell): make GitHub channel configuration usable (#16096)

* feat(web-shell): make GitHub channel configuration usable

The channel editor rendered every platform through one generic form, which
left the GitHub channel hard to configure and persisted settings its adapter
never reads.

- Render string-list fields that declare options as a checkbox list, so the
  fifteen notification reasons are pickable and explained instead of typed
  from memory. Unknown values written by hand stay visible and removable
  rather than disappearing on the next save.
- Lock the token input while local gh authentication is selected, clear a
  stored token when that switch is turned on, and drop a token that an
  existing useLocalGh selection has already overridden.
- Hide and stop persisting the direct-message policy for repository-scoped
  channels: the registry injects privatePolicy into every manageable channel,
  but the GitHub adapter only produces group envelopes, so the value never
  gates anything and only misleads.
- Name allowlisted groups repositories, validate owner/repo, and show the
  GitHub Enterprise API base URL shape.
- Split the catch-all credentials section into credentials, connection, and
  notification routing.

* test(web-shell): pin the GitHub editor capture to the reason checklist

The screenshot scenario now fails if the filter falls back to a free-text
input, so the published picture cannot drift away from the behaviour the
pull request claims.

* test(web-shell): pin the section split on a non-GitHub channel

The connection and routing sections key on field names, so they regroup gitlab and wecom fields too. Assert that split on a gitlab descriptor instead of leaving it implied by the GitHub assertions.

* fix(web-shell): match GitHub channel values the way the adapter reads them

Review round 1 on the editor found three places where the new controls
disagreed with the runtime or left dead configuration behind.

- The reason checklist compared values case-sensitively while the adapter
  lowercases both sides, so a stored COMMENT rendered as a stray sixteenth
  box beside an unchecked canonical one; ticking only mention then left
  comment notifications flowing. Compare case-insensitively and keep unknown
  entries in their original spelling.
- The pairing guard had no test witness: deleting it left the whole suite
  green while a legacy privatePolicy reopened the direct-message panel for a
  repository channel. Pin both directions — legacy private pairing must not
  open the panel, group pairing still must.
- Only privatePolicy was cleaned up; the equally unread allowedUsers survived
  every save and was still presented as an authoritative allowlist in the
  pairing panel. Drop the whole private-access set and stop showing it there.

* fix(channels): match GitHub repository groups regardless of case

The editor started validating repository allowlists as owner/repo, but a
repository path is case-insensitive while the runtime matched the group key
verbatim. An operator who typed qwenlm/qwen-code passed validation, saved, and
got a channel that connected and then dropped every notification as
not_allowlisted — the exact failure the validation was added to prevent.

Normalize on both sides so neither can drift:

- The editor writes the canonical lower-case key and shows stored allowlists
  in that form, so re-saving an older mixed-case entry migrates it. The
  per-repository settings are looked up under the normalized key, so retyping
  the case no longer discards that repository's sender policy.
- GroupGate and ChannelBase accept a platform group-ID normalizer, used for
  the allowlist, the sender policy and the per-group overrides alike.
  Normalizing only admission would have let a case-mismatched repository fall
  back to the open sender default, which is worse than dropping it.
- The GitHub adapter supplies the lower-casing normalizer; every other
  platform keeps matching group IDs verbatim.

Pairing approvals are unaffected: they are keyed by the chat ID the platform
reports, so they were already self-consistent.

* fix(channels): resolve the group history limit through the same lookup

Self-audit of the repository-casing fix found a third direct read of
config.groups: the per-group history limit still matched the key verbatim, so
a platform with case-insensitive IDs silently lost that override while the
allowlist and sender policy honoured it. Route it through groupConfigFor and
pin it with a case-insensitive channel test.

* fix(web-shell): keep the gh switch reversible and refuse ambiguous repositories

Review round 2 plus the independent verification round found three defects.

- Turning local gh authentication on queues the stored token for removal,
  because a token outranks gh. Turning it back off left that intent in place,
  so Save demanded a credential the operator never chose to delete and the
  switch could not be undone without retyping a PAT. Flipping it back now
  restores the stored token.
- Collapsing repository aliases could widen access. A stored allowlist may
  hold Owner/repo and owner/repo with different settings; the runtime resolves
  an exact key first, so keeping only one of them on an unrelated save dropped
  the effective sender restriction. The editor now refuses to save until the
  operator resolves the duplicate, and shows the allowlist without repeating
  the same repository twice.
- The reason descriptions described event types instead of the subscription
  reasons GitHub actually reports: comment means the account commented on the
  thread, state_change that the account changed its state, ci_activity that a
  workflow it triggered completed, and manual that it subscribed by hand.
  Rewritten in both languages against the REST notification contract.

The visual scenario also captured the dialog without scrolling, so the
checklist sat below the fold and the published picture did not show the
feature. It now scrolls the filter into view, asserts it is fully on screen,
and captures it separately.
This commit is contained in:
Shaojin Wen authored and GitHub committed 2026-11-02 13:11:33 +00:00
1 parent 73474ef094
commit 9336986835
14 files changed
+1494 -49

No files matched your search

@@ -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<typeof vi.fn>).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-'));
+18 -2
View File
@@ -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(
@@ -34,6 +34,30 @@ function envelope(overrides: Partial<Envelope> = {}): 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 },
+20 -2
View File
@@ -20,15 +20,33 @@ export class GroupGate {
private policy: GroupPolicy;
private groups: Record<string, GroupConfig>;
private pairingStore: PairingStore | null;
private normalizeGroupId: (groupId: string) => string;
constructor(
policy: GroupPolicy = 'disabled',
groups: Record<string, GroupConfig> = {},
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;
@@ -1107,6 +1107,81 @@ describe('GithubChannel', () => {
channel.disconnect();
});
describe('repository ID casing', () => {
async function connectWithGroups(
groups: Record<string, unknown>,
): Promise<void> {
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) => {
@@ -535,6 +535,16 @@ export class GithubChannel extends PollingChannelBase<GithubCursor> {
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;
@@ -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;
}
}
@@ -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 <p> 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<HTMLInputElement>('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<HTMLInputElement>('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<HTMLInputElement>(
'[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<HTMLInputElement>(
'[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<HTMLButtonElement>(
'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<HTMLButtonElement>(
'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<HTMLButtonElement>(
'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<HTMLButtonElement>(
'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<HTMLButtonElement>(
'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,
@@ -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<string, Record<string, string>> = {
dingtalk: {
@@ -125,6 +131,26 @@ const SHARED_FIELD_LABEL_KEYS: Record<string, string> = {
instructions: 'channels.editor.field.shared.instructions',
};
/** Per-platform placeholders that spell out the expected value shape. */
const FIELD_PLACEHOLDER_KEYS: Record<string, string> = {
'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<ChannelSecretDraft | undefined>(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 (
<FieldShell
key={field.key}
@@ -518,7 +601,7 @@ export function ChannelEditorDialog({
id={id}
checked={value === true}
aria-required={field.required}
onCheckedChange={(checked) => update(checked)}
onCheckedChange={updateSwitch}
/>
</FieldShell>
);
@@ -552,6 +635,83 @@ export function ChannelEditorDialog({
</FieldShell>
);
}
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 (
<FieldShell
key={field.key}
id={id}
label={fieldLabel(field)}
required={field.required}
description={fieldDescription(field)}
error={error}
>
<div
id={id}
className={styles.optionGrid}
role="group"
aria-label={fieldLabel(field)}
>
{[
...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 (
<label
key={option.value}
className={styles.optionRow}
data-selected={isSelected}
>
<input
type="checkbox"
className={styles.optionCheckbox}
checked={isSelected}
onChange={(event) =>
toggleValue(option.value, event.target.checked)
}
/>
<span className={styles.optionLabel}>{optionLabel}</span>
</label>
);
})}
</div>
</FieldShell>
);
}
if (field.kind === 'record') {
let record: Record<string, string> = {};
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({
</FieldShell>
</section>
<section className={styles.section}>
<h3 className={styles.sectionHeading}>
{t('channels.editor.section.credentials')}
</h3>
{credentialFields.map(renderField)}
</section>
{(['credentials', 'connection', 'routing'] as const).map(
(section) => {
const sectionFields = credentialFields.filter(
(field) => fieldSectionKey(field) === section,
);
if (sectionFields.length === 0) return null;
return (
<section key={section} className={styles.section}>
<h3 className={styles.sectionHeading}>
{t(`channels.editor.section.${section}`)}
</h3>
{sectionFields.map(renderField)}
</section>
);
},
)}
{sessionFields.length > 0 ? (
<section className={styles.settingsPanel}>
@@ -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' ? (
<FieldShell
id={`${formId}-allowedGroupIds`}
label={t('channels.editor.field.shared.allowedGroupIds')}
label={t(
repositoryOnly
? 'channels.editor.field.github.allowedRepos'
: 'channels.editor.field.shared.allowedGroupIds',
)}
description={t(
'channels.editor.field.shared.allowedGroupIds.description',
repositoryOnly
? 'channels.editor.field.github.allowedRepos.description'
: 'channels.editor.field.shared.allowedGroupIds.description',
)}
error={errors['allowedGroupIds']}
>
@@ -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') ? (
<ChannelPairingRequests
channelName={instance.name}
listRequests={listPairingRequests}
approveRequest={approvePairingRequest}
listApprovals={listPairingApprovals}
revokeApproval={revokePairingApproval}
staticAllowedUsers={configuredAllowedUsers(instance)}
staticAllowedUsers={
repositoryOnly ? [] : configuredAllowedUsers(instance)
}
/>
) : (
<Alert>
@@ -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<string, unknown>, Record<string, unknown>]> =
[
[
{ 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' } },
}),
);
});
});
@@ -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<string, Set<string>>();
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<string>()).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<string, unknown>,
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<string, unknown>,
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<string, unknown> = {};
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<string, DaemonChannelSecretUpdate> = {};
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);
}
@@ -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);
}
@@ -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(
+85 -2
View File
@@ -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) =>