diff --git a/docs/design/github-repo-filter.md b/docs/design/github-repo-filter.md new file mode 100644 index 00000000000..edbf20129fd --- /dev/null +++ b/docs/design/github-repo-filter.md @@ -0,0 +1,221 @@ +# GitHub Channel Repository Event Filter (Allowlist / Blocklist) + +[English](github-repo-filter.md) | [简体中文](github-repo-filter.zh-CN.md) + +Status: implemented in this change. + +## Problem + +The GitHub channel processes notifications from every repository the +authenticated account can see. The only repository-level control today is +`groupPolicy: "allowlist"` plus `groups` keys: an operator who wants "everything +except these noisy repositories" has no way to express it, and an operator who +wants a simple event filter must understand the access-policy machinery +(group gates, sender policies, per-repository overrides) to get one. + +In the Web Shell editor the repository allowlist lives under "Group Policy → +Allowlist → Allowed repositories". There is no blocklist, and the access-policy +framing makes a plain "filter events by repository" task unintuitive. + +## Current state + +- Inbound events arrive through the notification poll loop in + `packages/channels/github/src/GithubAdapter.ts` (`pollOnce`). Each + notification's subject URL yields `chatId = owner/repo`; `reasonFilter` + already skips notifications there, journals a capped `reason_filter` skip + row per poll plus one collapsed count row, and lets the cursor advance. +- Repository access is gated later, per envelope, by `GroupGate` + (`groupPolicy` + `groups`). The editor renders repository rows only for this + access path. +- A second event source, PR state watch (`runPrStateWatch`), serves PRs that a + user explicitly bound to a live session. It does not flow through the + notification loop. +- The daemon store (`channel-settings-store.ts`) validates every config key + against the plugin descriptor and runs the plugin's synchronous + `validateConfig` for cross-field rules. The editor mirrors those rules in + `channel-editor-state.ts`; `channel-editor-parity.test.ts` binds the two + sides with the plugin's own descriptor. + +## Goals + +- Filter inbound GitHub events by repository with two explicit modes: + allowlist (only listed repositories) and blocklist (all except listed). +- Make the filter a first-class, self-explanatory pair of fields in the Web + Shell editor, with repository-format validation and URL-paste tolerance. +- Keep the runtime, the save-time validator, and the editor mirror in sync + through one shared validation function and a parity table. + +## Scope boundaries + +- The filter applies to every incoming notification, including notifications + about PRs bound to a live session. Only PR state watch events + (`runPrStateWatch`) — explicit per-PR subscriptions created by the user in + a session — bypass it. The field description states this. +- `groupPolicy` / `groups` keep their access-control meaning unchanged. The + repository filter is an additional intake filter; when both are active the + effective set is their intersection. +- No change to other channels, to `GroupGate`, or to the shared `GroupPolicy` + type. + +## Proposed solution + +### Configuration (GitHub plugin) + +Two new flat descriptor fields, placed after `reasonFilter`: + +- `repoFilterMode`: enum `off | allowlist | blocklist`, default `off`. +- `repoFilter`: string-list of `owner/repo` entries. + +```json +{ + "type": "github", + "useLocalGh": true, + "groupPolicy": "open", + "repoFilterMode": "blocklist", + "repoFilter": ["octo/noisy-repo", "octo/archive"] +} +``` + +Flat fields (not one object field) because the Web Shell editor cannot edit +`object`-kind fields. + +### Runtime (`GithubAdapter.ts`) + +- `validateRepoFilterConfig(config)` — exported pure function returning a + string error or `undefined`. Rules: + - `repoFilterMode` absent or `off`: the list is ignored entirely (a stale + list never blocks a save or a connect while the filter is off). + - `repoFilterMode` set to `allowlist`/`blocklist`: `repoFilter` must be a + non-empty array of strings, each matching `^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$` + after trim + lowercase. An active allowlist with an empty list would + silently drop every event, so it is rejected instead. + - Any other mode value is rejected. +- `normalizeRepoFilter(config, name)` calls the same validator and throws on + error; `connect()` stores `{ mode, repos: Set } | null` beside + `reasonFilter`. +- In the poll loop, immediately after `extractFromSubjectUrl` and before + `reasonFilter`: allowlist skips repositories not in the set, blocklist skips + repositories in the set (case-insensitive). A skipped notification is + logged to stderr, emitted as a `repoFilter.skip` debug payload, and + journaled as `inbound / skipped / repo_filter` with the same per-poll cap + (20 rows) and collapsed count row as `reason_filter`. The cursor advances + past filtered notifications exactly as it does for reason-filtered ones. +- `index.ts` `validateConfig` calls `validateRepoFilterConfig`, so the daemon + rejects an invalid combination at save time with the same message the + runtime would throw. + +### Editor (`web-shell`) + +- `channel-editor-state.ts`: + - `normalizeRepositoryFilterInput(value, baseUrl)` — splits on commas, + trims, strips trailing sentence punctuation a paste can carry, lowercases, + shortens GitHub web URLs (`https://github.com/owner/repo`, with optional + `.git`, trailing slash, query, fragment, or `/pull/123`-style suffix) to + `owner/repo`, deduplicates, joins with `, `. Two gates decide whether a + URL is shortened at all. Structurally, only URLs on the channel's own + GitHub WEB host qualify: `github.com` plus the web host derived from + `baseUrl` for GitHub Enterprise — an `api.`-prefixed baseUrl host + contributes only its stripped form, never the raw API host, and paths + under the baseUrl's own API prefix (e.g. `/api/v3/`) are excluded, so a + pasted REST URL cannot shorten to `repos/octo` or `api/v3`. Any other + host — `gist.github.com`, `gitlab.com`, an arbitrary site — passes + through unchanged for validation to reject, which closes the "first two + segments of any host" class. On a qualified + host, a URL whose first path segment is a reserved GitHub area (`orgs`, + `apps`, `settings`, `trending`, ...) still passes through unchanged so + validation rejects it instead of silently filtering a nonexistent + repository. That area list is best-effort by construction — a denylist + of GitHub's top-level paths cannot be complete, and a future area can + reintroduce the hazard until it is added. A snapshot assertion pins the + whole exported set (removing any member fails it), an enumerated test + keeps every listed root rejected, and explicit regression pins cover the + reported shapes including a capitalized root. Other non-URL entries + pass through unchanged so validation can name them. + - Validation (github only): when `repoFilterMode` is active, an empty list + is `repoFilterRequired`; entries that still fail the repository regex + after normalization are `invalidRepoId`. When the mode is `off`, the list + is not validated, matching the runtime's ignore-while-off rule and the + conditional-field visibility below. + - `buildChannelUpsertRequest` normalizes the github `repoFilter` value with + the same function before sending. + - `initialFieldValue` seeds `repoFilterMode` with `off` for existing + instances whose config lacks the key (the generic existing-instance path + would otherwise leave the select empty). +- `ChannelEditorDialog.tsx`: the list field is rendered only while the mode + is active (progressive disclosure). Visibility and validation are gated on + the same predicate, so a hidden field can never strand a save. The draft + keeps the list text while the mode is `off`, so re-enabling restores it. +- i18n (en + zh): field labels ("Repository Filter" / "Filtered repositories"), + mode option labels (Off / Allowlist — only listed repositories / Blocklist — + all except listed), descriptions (including the URL-paste note and the + bound-PR-watch boundary), placeholder `owner/repo, owner/repo`, and the two + validation messages. + +### Parity + +`channel-editor-parity.test.ts` gains a repo-filter table: the same configs +feed the editor mirror and `plugin.management.validateConfig`, using the +plugin's own descriptor (active mode + empty list, malformed entry, valid +allowlist/blocklist, off + stale list). + +## Design decisions and rationale + +- **Separate intake filter, not a new `groupPolicy` value.** `GroupPolicy` is + shared by every channel and enforced by `GroupGate`; adding `blocklist` + there would change access-control semantics across all platforms. An + adapter-level filter is GitHub-only, sits where `reasonFilter` already sits, + and composes with any group policy. +- **Explicit `off` mode instead of "empty list means off".** An empty + allowlist must not silently mean "block everything" or "filter disabled"; + the mode makes the intent explicit and lets the runtime reject the + ambiguous combination. +- **Ignore the list while off.** Operators can park a list, switch the filter + off, and save unrelated edits without the parked list blocking the save — + the same tolerance the store already grants unchanged numeric values. +- **Normalize URLs in the editor, keep the runtime strict.** Pasting a + repository or PR URL is the natural gesture; the editor shortens it on + save. `settings.json` hand-edits keep one canonical spelling and get a + clear error otherwise. +- **Notifications filtered, watch events not.** Every notification is + filtered, including notifications about bound PRs — they arrive through the + same intake. Bound-PR watch is an explicit user subscription with its own + durable state machine; filtering it is a separate decision and stays out + of this change. + +## Files affected + +- `packages/channels/github/src/GithubAdapter.ts`, `index.ts`, + `GithubAdapter.test.ts` +- `packages/web-shell/client/components/channels/channel-editor-state.ts`, + `ChannelEditorDialog.tsx`, their tests, `channel-editor-parity.test.ts` +- `packages/web-shell/client/i18n.tsx` +- `packages/web-shell/client/e2e/visuals/screenshots.spec.ts` (repository + filter editor captures) +- `docs/users/features/channels/github.md` +- `docs/design/github-repo-filter.md`, `docs/design/github-repo-filter.zh-CN.md` + +## Validation plan + +- Unit: adapter normalize/validate rules; poll-loop skip behavior for both + modes (case-insensitive match, journal cap and collapsed row, cursor + advances); editor normalization/validation/save payload; parity table. +- E2E (`.qwen/e2e-tests/github-repo-filter.md`): save both modes through the + daemon API against a local GitHub-compatible stub, observe skipped vs + processed notifications and the `repo_filter` journal rows; editor + rendering checks. + +## Acceptance criteria + +- `repoFilterMode: allowlist` processes only listed repositories; `blocklist` + processes all except listed; `off`/absent processes everything. +- An active mode with an empty or malformed list is rejected at save time + (daemon and editor) and at connect time (runtime), with matching messages. +- Filtered notifications are journaled as `repo_filter` skips and do not hold + the cursor. +- The editor shows the repository list only while a mode is active, accepts + pasted GitHub URLs, and round-trips stored values unchanged when untouched. +- Existing channels without the new keys behave exactly as before. + +## Open questions + +None. diff --git a/docs/design/github-repo-filter.zh-CN.md b/docs/design/github-repo-filter.zh-CN.md new file mode 100644 index 00000000000..4af72a459e8 --- /dev/null +++ b/docs/design/github-repo-filter.zh-CN.md @@ -0,0 +1,190 @@ +# GitHub 频道按仓库过滤事件(白名单 / 黑名单) + +[English](github-repo-filter.md) | [简体中文](github-repo-filter.zh-CN.md) + +状态:已随本次变更实现。 + +## 问题 + +GitHub 频道会处理认证账号可见的所有仓库的通知。目前唯一的仓库级控制是 +`groupPolicy: "allowlist"` 加 `groups` 键:想要「除了这些吵闹的仓库以外都处理」 +的管理员无法表达这个意图;想要一个简单事件过滤器的管理员必须先理解访问策略 +机制(群门禁、发言者策略、按仓库覆盖)才能配出来。 + +在 Web Shell 编辑器里,仓库白名单藏在「Group Policy → Allowlist → +Allowed repositories」下面。没有黑名单,而且访问策略的框架让「按仓库过滤 +事件」这件简单的事变得不直观。 + +## 现状 + +- 入站事件经由 `packages/channels/github/src/GithubAdapter.ts` 的通知轮询循环 + (`pollOnce`)到达。每条通知的 subject URL 解析出 `chatId = owner/repo`; + `reasonFilter` 已经在那里跳过通知、按轮记录有上限的 `reason_filter` 跳过 + 行外加一条折叠计数行,并让游标继续前进。 +- 仓库访问在其后由 `GroupGate`(`groupPolicy` + `groups`)按信封把关。编辑器 + 只为这条访问路径渲染仓库行。 +- 第二个事件源是 PR 状态观察(`runPrStateWatch`),服务于用户在会话里显式 + 绑定到活跃会话的 PR。它不经过通知循环。 +- daemon 存储(`channel-settings-store.ts`)按插件描述符校验每个配置键,并 + 调用插件的同步 `validateConfig` 做跨字段规则。编辑器在 + `channel-editor-state.ts` 里镜像这些规则;`channel-editor-parity.test.ts` + 用插件自己的描述符把两侧绑在一起。 + +## 目标 + +- 用两种显式模式按仓库过滤入站 GitHub 事件:白名单(只处理列出的仓库)与 + 黑名单(处理列出仓库以外的全部)。 +- 在 Web Shell 编辑器里把过滤器做成一对一等公民、自解释的字段,带仓库格式 + 校验,并容忍粘贴 URL。 +- 通过一个共享校验函数和一张一致性表,让运行时、保存时校验器与编辑器镜像 + 保持同步。 + +## 范围边界 + +- 过滤器作用于每一条入站通知,包括绑定到活跃会话的 PR 的通知。只有 PR + 状态观察事件(`runPrStateWatch`)——用户在会话里显式创建的按 PR 订阅—— + 绕过它。字段描述会写明这一点。 +- `groupPolicy` / `groups` 的访问控制含义不变。仓库过滤器是额外的入站过滤; + 两者同时生效时,有效集合是它们的交集。 +- 不改其他频道、不改 `GroupGate`、不改共享的 `GroupPolicy` 类型。 + +## 方案 + +### 配置(GitHub 插件) + +两个新的扁平描述符字段,放在 `reasonFilter` 之后: + +- `repoFilterMode`:枚举 `off | allowlist | blocklist`,默认 `off`。 +- `repoFilter`:`owner/repo` 条目的字符串列表。 + +```json +{ + "type": "github", + "useLocalGh": true, + "groupPolicy": "open", + "repoFilterMode": "blocklist", + "repoFilter": ["octo/noisy-repo", "octo/archive"] +} +``` + +用扁平字段(而不是一个 object 字段),因为 Web Shell 编辑器无法编辑 +`object` 类字段。 + +### 运行时(`GithubAdapter.ts`) + +- `validateRepoFilterConfig(config)` —— 导出的纯函数,返回字符串错误或 + `undefined`。规则: + - `repoFilterMode` 缺省或 `off`:完全忽略列表(过滤器关闭时,陈旧列表 + 永远不会挡住保存或连接)。 + - `repoFilterMode` 为 `allowlist`/`blocklist`:`repoFilter` 必须是非空 + 字符串数组,每项 trim + 小写后匹配 + `^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$`。生效的白名单若列表为空会静默丢掉 + 所有事件,因此直接拒绝。 + - 其他 mode 取值一律拒绝。 +- `normalizeRepoFilter(config, name)` 调用同一个校验器,出错即抛; + `connect()` 在 `reasonFilter` 旁边存入 `{ mode, repos: Set } | null`。 +- 在轮询循环里,紧跟 `extractFromSubjectUrl` 之后、`reasonFilter` 之前: + 白名单跳过不在集合里的仓库,黑名单跳过在集合里的仓库(大小写不敏感)。 + 被跳过的通知写 stderr、发 `repoFilter.skip` 调试载荷,并按 + `inbound / skipped / repo_filter` 记入日志,沿用与 `reason_filter` 相同的 + 每轮上限(20 行)与折叠计数行。游标越过被过滤的通知继续前进,与 + reason 过滤完全一致。 +- `index.ts` 的 `validateConfig` 调用 `validateRepoFilterConfig`,因此 + daemon 在保存时就以运行时相同的消息拒绝非法组合。 + +### 编辑器(`web-shell`) + +- `channel-editor-state.ts`: + - `normalizeRepositoryFilterInput(value, baseUrl)` —— 按逗号拆分、trim、 + 剥掉粘贴可能带上的句尾标点、小写,把 GitHub web URL + (`https://github.com/owner/repo`,可带 `.git`、尾斜杠、query、fragment + 或 `/pull/123` 式后缀)缩短为 `owner/repo`,去重后用 `, ` 连接。是否 + 缩短由两道门决定。结构上,只有频道自己的 GitHub WEB 主机上的 URL 才 + 可缩短:`github.com` 加上由 `baseUrl` 派生的 GitHub Enterprise web + 主机——带 `api.` 前缀的 baseUrl 主机只贡献去前缀后的形式,绝不加入 + 原始 API 主机;baseUrl 自身 API 前缀(如 `/api/v3/`)下的路径也被 + 排除,因此粘贴 REST URL 不会被缩短成 `repos/octo` 或 `api/v3`。其他 + 任何主机——`gist.github.com`、`gitlab.com`、任意站点——原样透传交给 + 校验拒绝,从结构上关闭「任意主机的头两段路径」这一类隐患。在合格 + 主机上,首段路径是 GitHub 保留区域(`orgs`、`apps`、`settings`、 + `trending` 等)的 URL 仍原样透传,让校验拒绝它,而不是静默地去过滤 + 一个不存在的仓库。这份区域清单在构造上就是尽力而为——对 GitHub 顶层 + 路径做黑名单不可能穷尽,未来新增的区域在被补进清单之前会重新引入该 + 隐患。快照断言钉住整个导出集合(移除任何成员都会红),枚举测试保证 + 每个已列根路径都被拒绝,显式回归钉覆盖报告所列形态(含大写根路径)。 + 其他非 URL 条目也原样通过,让校验能点名它们。 + - 校验(仅 github):`repoFilterMode` 生效时,空列表为 + `repoFilterRequired`;归一化后仍不匹配仓库正则的条目为 + `invalidRepoId`。mode 为 `off` 时不校验列表,与运行时的「关闭即忽略」 + 规则及下面的条件可见性一致。 + - `buildChannelUpsertRequest` 在发送前用同一个函数归一化 github 的 + `repoFilter` 值。 + - `initialFieldValue` 为配置里缺键的既有实例把 `repoFilterMode` 种子值 + 设为 `off`(通用的既有实例路径否则会让下拉框为空)。 +- `ChannelEditorDialog.tsx`:列表字段只在 mode 生效时渲染(渐进披露)。 + 可见性与校验挂在同一个谓词上,隐藏字段永远不会卡住保存。mode 为 `off` + 时草稿保留列表文本,重新启用即恢复。 +- i18n(en + zh):字段标签(「Repository Filter」/「Filtered + repositories」)、mode 选项标签(Off / Allowlist — only listed + repositories / Blocklist — all except listed)、描述(含粘贴 URL 说明与 + PR 绑定观察的边界)、占位符 `owner/repo, owner/repo`,以及两条校验消息。 + +### 一致性 + +`channel-editor-parity.test.ts` 增加一张 repo-filter 表:同一组配置喂编辑器 +镜像与 `plugin.management.validateConfig`,使用插件自己的描述符(生效 +mode + 空列表、畸形条目、合法白名单/黑名单、off + 陈旧列表)。 + +## 设计决策与理由 + +- **独立的入站过滤器,而不是新的 `groupPolicy` 取值。** `GroupPolicy` 由 + 所有频道共享、由 `GroupGate` 执行;在那里加 `blocklist` 会改变所有平台 + 的访问控制语义。适配器级过滤器只属于 GitHub,放在 `reasonFilter` 已经 + 在的位置,并与任何 group policy 复合。 +- **显式 `off` mode,而不是「空列表即关闭」。** 空白名单不能静默地表示 + 「屏蔽一切」或「过滤关闭」;mode 让意图显式,并让运行时拒绝这个歧义 + 组合。 +- **关闭时忽略列表。** 管理员可以停放一份列表、关掉过滤器、保存不相关的 + 编辑,而不被停放的列表挡住保存——与存储对未变更数值已有的容忍一致。 +- **编辑器归一化 URL,运行时保持严格。** 粘贴仓库或 PR URL 是自然手势; + 编辑器在保存时缩短它。`settings.json` 手编保持唯一规范拼写,否则得到 + 清晰报错。 +- **通知被过滤,观察事件不被过滤。** 每条通知都被过滤,包括绑定 PR 的 + 通知——它们走同一个入站口。PR 绑定观察是带自己持久状态机的显式用户 + 订阅;要不要过滤它是另一个决策,不在本次变更内。 + +## 涉及文件 + +- `packages/channels/github/src/GithubAdapter.ts`、`index.ts`、 + `GithubAdapter.test.ts` +- `packages/web-shell/client/components/channels/channel-editor-state.ts`、 + `ChannelEditorDialog.tsx`、它们的测试、`channel-editor-parity.test.ts` +- `packages/web-shell/client/i18n.tsx` +- `packages/web-shell/client/e2e/visuals/screenshots.spec.ts`(仓库过滤器 + 编辑器截图) +- `docs/users/features/channels/github.md` +- `docs/design/github-repo-filter.md`、`docs/design/github-repo-filter.zh-CN.md` + +## 验证计划 + +- 单元:适配器 normalize/validate 规则;轮询循环两种模式的跳过行为(大小写 + 不敏感匹配、日志上限与折叠行、游标前进);编辑器归一化/校验/保存载荷; + 一致性表。 +- E2E(`.qwen/e2e-tests/github-repo-filter.md`):对着本地 GitHub 兼容桩, + 通过 daemon API 保存两种模式,观察跳过与处理的通知以及 `repo_filter` + 日志行;编辑器渲染检查。 + +## 验收标准 + +- `repoFilterMode: allowlist` 只处理列出的仓库;`blocklist` 处理列出仓库 + 以外的全部;`off`/缺省处理一切。 +- 生效 mode 配空列表或畸形列表,在保存时(daemon 与编辑器)与连接时 + (运行时)都被拒绝,消息一致。 +- 被过滤的通知记为 `repo_filter` 跳过,且不卡游标。 +- 编辑器只在 mode 生效时显示仓库列表,接受粘贴的 GitHub URL,未触碰的 + 存储值原样往返。 +- 没有新键的既有频道行为与之前完全一致。 + +## 待定问题 + +无。 diff --git a/docs/users/features/channels/github.md b/docs/users/features/channels/github.md index dd796ac529c..2312ee0e2f3 100644 --- a/docs/users/features/channels/github.md +++ b/docs/users/features/channels/github.md @@ -89,6 +89,8 @@ Local `gh` authentication requires an HTTPS `baseUrl` so the daemon host credent | `groupPolicy` | `"disabled"` | Must be `"open"`, `"allowlist"` with the repo (`owner/repo`) listed in `groups`, or `"pairing"` with the repo approved for notifications to flow | | `groups.*.requireMention` | `true` | Require @mentions for ordinary comments; directed notification reasons still run | | `reasonFilter` | unset | Optional allowlist of GitHub notification reasons to process | +| `repoFilterMode` | `"off"` | Repository event filter: `"allowlist"` processes only the repositories listed in `repoFilter`, `"blocklist"` processes all except them, `"off"` processes everything. Applies to notification intake only | +| `repoFilter` | unset | `owner/repo` repository names used by `repoFilterMode`; required while the mode is `"allowlist"` or `"blocklist"` | | `deliverToBoundSessions` | `false` | Deliver events for PRs bound to a live session into that session as a prompt instead of a channel-owned session (daemon-managed channels only) | | `prWatchEvents` | unset | Optional state transitions to watch for bound PRs: `ci_failure`, `merge_conflict`, `head_change`, `state_change`. Requires `deliverToBoundSessions` | | `boundSessionActions` | unset | Optional operator instruction templates for bound-session delivery, keyed by event class with optional role prefixes (`review:`, `verify:`, `fix:`). Enables role-aware fan-out. Requires `deliverToBoundSessions` | @@ -99,6 +101,25 @@ Valid `reasonFilter` values are `mention`, `review_requested`, `assign`, `author Filtered notifications are marked read only after all accepted work in the poll window completes. Removing the filter later will not replay notifications the channel already skipped. +### Repository event filter + +Use `repoFilterMode` together with `repoFilter` to filter incoming notifications by repository: + +```json +{ + "repoFilterMode": "blocklist", + "repoFilter": ["octo/noisy-repo", "octo/archive"] +} +``` + +- `"allowlist"` processes only the listed repositories; `"blocklist"` processes every repository except the listed ones; `"off"` (the default) processes everything. +- Repository names are matched case-insensitively and must be written as `owner/repo`. An active mode with an empty or malformed list is rejected at save time and at channel start. +- While the mode is `"off"`, a stored list is ignored, so you can park a list without it affecting the channel. +- The filter composes with `groupPolicy`: when both restrict repositories, only the intersection is processed. It applies to every incoming notification, including notifications about PRs bound to a live session (`deliverToBoundSessions`); only the PR state watch events (`prWatchEvents`) bypass it. +- Filtered notifications are journaled as `repo_filter` skips in the Channel event timeline and do not hold the poll cursor. Like `reasonFilter` skips, they are marked read with the window and are not replayed when the filter changes. + +In the Web Shell editor, **Repository Filter** offers Off / Allowlist / Blocklist, and the **Filtered Repositories** list appears while a mode is active. You can paste repository or PR URLs there; they are shortened to `owner/repo` on save. Only URLs on the channel's own GitHub host are shortened — `github.com`, or the GitHub Enterprise host derived from the API base URL; URLs on any other host are rejected. On that host, URLs under known non-repository GitHub areas (`orgs/`, `apps/`, `settings/`, `trending/`, and similar) are also rejected instead of shortened. That area list is best-effort: a URL from a GitHub area not on the list can still be shortened, so check the saved `owner/repo` entries after pasting. + ### Editing from the Web Shell Settings → Channels → GitHub exposes the poll interval under **Connection**, in milliseconds. It is kept between 1 and 86400000 (one day): a larger value is clamped to the ceiling and a smaller positive one is raised to 1, because Node rounds a sub-millisecond timeout to about 1 ms and clamps one above its 32-bit range to 1 ms as well — either would turn the poll loop into a hot loop. Leave it empty for the 60000 ms default. When `groupPolicy` is `"allowlist"`, the **Access control** section also shows one row per allowlisted repository, each with a require-mention control and a speaker control. diff --git a/packages/channels/github/src/GithubAdapter.test.ts b/packages/channels/github/src/GithubAdapter.test.ts index 8000e1eab15..b8730f4dafc 100644 --- a/packages/channels/github/src/GithubAdapter.test.ts +++ b/packages/channels/github/src/GithubAdapter.test.ts @@ -4370,6 +4370,347 @@ describe('GithubChannel', () => { }); }); + describe('repoFilter', () => { + function connectWithRepoFilter( + overrides: Record, + ): Promise { + channel = new TestableGithubChannel( + 'test-github', + makeConfig(overrides), + makeBridge(), + ); + // connect() ends in startPollLoop(); seed the first poll so a successful + // connect does not print a backoff line from an unseeded paginate mock. + mockOctokit.paginate.mockResolvedValueOnce([]); + return channel.connect(); + } + + function notificationFor(repo: string, issue: number) { + return makeNotification({ + subject: { + title: 'Test Issue', + url: `https://api.github.com/repos/${repo}/issues/${issue}`, + type: 'Issue', + }, + repository: { full_name: repo }, + }); + } + + it('allowlist processes only listed repositories', async () => { + const stderrWrite = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + await initWithoutLoop({ + repoFilterMode: 'allowlist', + repoFilter: ['owner/repo'], + }); + try { + mockOctokit.paginate + .mockResolvedValueOnce([ + notificationFor('noise/archive', 7), + notificationFor('owner/repo', 42), + ]) + .mockResolvedValueOnce([makeComment({ body: 'hello @test-bot' })]); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(1); + expect(channel.inboundEnvelopes[0]!.chatId).toBe('owner/repo'); + expect(stderrWrite).toHaveBeenCalledWith( + expect.stringContaining( + 'skipping notification (repository=noise/archive excluded by repoFilter allowlist', + ), + ); + expect( + readChannelEvents().filter( + (event) => event['reason'] === 'repo_filter', + ), + ).toEqual([ + expect.objectContaining({ + type: 'inbound', + outcome: 'skipped', + reason: 'repo_filter', + repository: 'noise/archive', + thread: 'issue:7', + number: 7, + }), + ]); + } finally { + stderrWrite.mockRestore(); + } + }); + + it('blocklist processes all except listed repositories', async () => { + await initWithoutLoop({ + repoFilterMode: 'blocklist', + repoFilter: ['noise/archive'], + }); + mockOctokit.paginate + .mockResolvedValueOnce([ + notificationFor('noise/archive', 7), + notificationFor('owner/repo', 42), + ]) + .mockResolvedValueOnce([makeComment({ body: 'hello @test-bot' })]); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(1); + expect(channel.inboundEnvelopes[0]!.chatId).toBe('owner/repo'); + expect( + readChannelEvents().filter( + (event) => event['reason'] === 'repo_filter', + ), + ).toEqual([ + expect.objectContaining({ + repository: 'noise/archive', + outcome: 'skipped', + }), + ]); + }); + + it('matches repository names case-insensitively', async () => { + await initWithoutLoop({ + repoFilterMode: 'allowlist', + repoFilter: ['OWNER/Repo'], + }); + mockOctokit.paginate + .mockResolvedValueOnce([notificationFor('owner/repo', 42)]) + .mockResolvedValueOnce([makeComment({ body: 'hello @test-bot' })]); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(1); + }); + + it('matches a mixed-case notification repository against a lowercase entry', async () => { + // The mirror of the config-side case above: GitHub returns the canonical + // casing in subject URLs, so the comparison site must lowercase chatId. + // Deleting chatId.toLowerCase() there leaves the config-side case green + // and fails this one. + await initWithoutLoop({ + repoFilterMode: 'blocklist', + repoFilter: ['owner/repo'], + }); + mockOctokit.paginate.mockResolvedValueOnce([ + notificationFor('Owner/Repo', 42), + ]); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(0); + expect( + readChannelEvents().filter( + (event) => event['reason'] === 'repo_filter', + ), + ).toEqual([expect.objectContaining({ repository: 'Owner/Repo' })]); + }); + + it('trims entries and drops empty ones when normalizing the list', async () => { + await initWithoutLoop({ + repoFilterMode: 'allowlist', + repoFilter: [' Owner/Repo ', '', 'owner/repo'], + }); + mockOctokit.paginate + .mockResolvedValueOnce([notificationFor('owner/repo', 42)]) + .mockResolvedValueOnce([makeComment({ body: 'hello @test-bot' })]); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(1); + expect( + readChannelEvents().some((event) => event['reason'] === 'repo_filter'), + ).toBe(false); + }); + + it('processes every repository when the mode is off or unset', async () => { + await initWithoutLoop({ + repoFilterMode: 'off', + repoFilter: ['owner/repo'], + }); + mockOctokit.paginate + .mockResolvedValueOnce([notificationFor('noise/archive', 7)]) + .mockResolvedValueOnce([makeComment({ body: 'hello @test-bot' })]); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(1); + expect( + readChannelEvents().some((event) => event['reason'] === 'repo_filter'), + ).toBe(false); + }); + + it('advances the cursor past a poll of only filtered notifications', async () => { + await initWithoutLoop({ + repoFilterMode: 'blocklist', + repoFilter: ['owner/repo'], + }); + mockOctokit.paginate.mockResolvedValueOnce([ + makeNotification({ last_read_at: '2026-07-01T12:00:00.000Z' }), + ]); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(0); + expect( + mockOctokit.rest.activity.markNotificationsAsRead, + ).toHaveBeenCalledWith({ + last_read_at: '2026-07-02T10:00:00.000Z', + read: true, + }); + expect(channel.cursor.lastProcessedAt).toBe('2026-07-02T10:00:00.000Z'); + }); + + it('journals at most 20 individual skips plus one counted row', async () => { + await initWithoutLoop({ + repoFilterMode: 'allowlist', + repoFilter: ['owner/repo'], + }); + const flood = Array.from({ length: 25 }, (_, index) => + notificationFor(`noise/repo-${index}`, 100 + index), + ); + mockOctokit.paginate.mockResolvedValueOnce(flood); + + await pollOnce(); + + const events = readChannelEvents().filter( + (event) => event['reason'] === 'repo_filter', + ); + expect(events).toHaveLength(21); + const individual = events.filter( + (event) => event['detail'] === undefined, + ); + expect(individual).toHaveLength(20); + expect(individual[0]).toMatchObject({ + repository: 'noise/repo-0', + thread: 'issue:100', + number: 100, + }); + expect(events.at(-1)).toEqual( + expect.objectContaining({ + type: 'inbound', + outcome: 'skipped', + reason: 'repo_filter', + detail: { filtered: 5 }, + }), + ); + expect(events.at(-1)).not.toHaveProperty('repository'); + }); + + it('resets the journal budget on the next poll', async () => { + // The counters live in the poll body, so each busy poll journals its own + // 20 individual rows plus one counted row. Hoisting them to instance + // fields would yield 22 rows total and fail here. + await initWithoutLoop({ + repoFilterMode: 'allowlist', + repoFilter: ['owner/repo'], + }); + const flood = Array.from({ length: 25 }, (_, index) => + notificationFor(`noise/repo-${index}`, 100 + index), + ); + mockOctokit.paginate + .mockResolvedValueOnce(flood) + .mockResolvedValueOnce(flood); + + await pollOnce(); + await pollOnce(); + + const events = readChannelEvents().filter( + (event) => event['reason'] === 'repo_filter', + ); + expect(events).toHaveLength(42); + const counted = events.filter( + (event) => + (event['detail'] as { filtered?: number } | undefined)?.filtered !== + undefined, + ); + expect(counted).toHaveLength(2); + expect( + counted.every((event) => event['detail']?.['filtered'] === 5), + ).toBe(true); + }); + + it('journals repo_filter, not reason_filter, when both gates exclude a notification', async () => { + // Pins the documented ordering: the repository filter runs before the + // reason filter, so a notification excluded by both is journaled under + // repo_filter. Swapping the two blocks flips this row to reason_filter. + await initWithoutLoop({ + repoFilterMode: 'blocklist', + repoFilter: ['owner/repo'], + reasonFilter: ['mention'], + }); + mockOctokit.paginate.mockResolvedValueOnce([ + makeNotification({ reason: 'comment' }), + ]); + + await pollOnce(); + + expect( + readChannelEvents() + .filter((event) => event['type'] === 'inbound') + .map((event) => event['reason']), + ).toEqual(['repo_filter']); + }); + + it('rejects an active mode with an empty list', async () => { + await expect( + connectWithRepoFilter({ repoFilterMode: 'allowlist', repoFilter: [] }), + ).rejects.toThrow( + 'repoFilter for channel test-github requires at least one owner/repo entry when repoFilterMode is "allowlist".', + ); + }); + + it('rejects an active mode without a list', async () => { + await expect( + connectWithRepoFilter({ repoFilterMode: 'blocklist' }), + ).rejects.toThrow( + 'repoFilter for channel test-github must be an array of owner/repo repository names when repoFilterMode is "blocklist".', + ); + }); + + it('rejects malformed repository entries', async () => { + await expect( + connectWithRepoFilter({ + repoFilterMode: 'blocklist', + repoFilter: ['https://github.com/owner/repo'], + }), + ).rejects.toThrow( + 'repoFilter entries for channel test-github must be owner/repo repository names: https://github.com/owner/repo', + ); + }); + + it('rejects non-string entries', async () => { + await expect( + connectWithRepoFilter({ + repoFilterMode: 'allowlist', + repoFilter: [42], + }), + ).rejects.toThrow( + 'repoFilter entries for channel test-github must be strings.', + ); + }); + + it('rejects an unknown mode', async () => { + await expect( + connectWithRepoFilter({ + repoFilterMode: 'denylist', + repoFilter: ['owner/repo'], + }), + ).rejects.toThrow( + 'repoFilterMode for channel test-github must be one of: off, allowlist, blocklist.', + ); + }); + + it('ignores a parked list while the mode is off', async () => { + await expect( + connectWithRepoFilter({ + repoFilterMode: 'off', + repoFilter: ['not a repo'], + }), + ).resolves.toBeUndefined(); + channel.disconnect(); + }); + }); + describe('publication contract', () => { const nameHash = createHash('sha256') .update('test-github') @@ -15139,6 +15480,67 @@ describe('GithubChannel bound-session delivery and PR state watch', () => { expect(boundSessions.deliverPrompt).not.toHaveBeenCalled(); }); + it('delivers watch events for a repository the notification filter excludes', async () => { + // Pins the documented boundary: the repository filter governs + // notification intake only. A bound PR's state watch events bypass it, + // so blocklisting the repository must not silence its watch delivery. + await initChannel( + { + deliverToBoundSessions: true, + prWatchEvents: ['ci_failure'], + repoFilterMode: 'blocklist', + repoFilter: ['owner/repo'], + }, + { boundSessions }, + ); + mockPull({}); + mockNotificationsOnce([]); + mockChecks([{ status: 'completed', conclusion: 'success' }]); + await pollOnce(); + + mockPull({ updated_at: '2026-07-02T10:00:00.000Z' }); + mockNotificationsOnce([]); + mockChecks([ + { status: 'completed', conclusion: 'failure', name: 'build' }, + ]); + await pollOnce(); + + expect(boundSessions.deliverPrompt).toHaveBeenCalledTimes(1); + expect(boundSessions.deliverPrompt.mock.calls[0]![1]).toContain( + 'CI failed on head sha-1', + ); + }); + + it('filters notifications about a bound PR instead of delivering them', async () => { + // The complement of the watch-bypass case: bound-session routing must + // not exempt a notification from the repository filter. Moving bound + // delivery above the filter (or early-continuing bound threads into it) + // delivers this prompt and fails here. + await initChannel( + { + deliverToBoundSessions: true, + repoFilterMode: 'blocklist', + repoFilter: ['owner/repo'], + }, + { boundSessions }, + ); + mockNotificationsOnce([makeNotification({ reason: 'mention' })]); + + await pollOnce(); + + expect(boundSessions.deliverPrompt).not.toHaveBeenCalled(); + expect( + readChannelEvents().filter( + (event) => event['reason'] === 'repo_filter', + ), + ).toEqual([ + expect.objectContaining({ + repository: 'owner/repo', + outcome: 'skipped', + }), + ]); + }); + it('fires ci_failure once per head and not again for the same head', async () => { await initChannel(watchConfig, { boundSessions }); mockPull({}); diff --git a/packages/channels/github/src/GithubAdapter.ts b/packages/channels/github/src/GithubAdapter.ts index e2a47632f49..8d3b4096dc0 100644 --- a/packages/channels/github/src/GithubAdapter.ts +++ b/packages/channels/github/src/GithubAdapter.ts @@ -40,7 +40,12 @@ import { import { testBotMention, stripBotMention } from './mention.js'; import { resolveGithubWebOrigin } from './web-origin.js'; -interface GithubConfig extends ChannelConfig { +interface RepoFilterConfigFields { + repoFilterMode?: unknown; + repoFilter?: unknown; +} + +interface GithubConfig extends ChannelConfig, RepoFilterConfigFields { baseUrl?: string; reasonFilter?: unknown; useLocalGh?: boolean; @@ -310,6 +315,89 @@ function normalizeReasonFilter( return reasons.length > 0 ? new Set(reasons) : null; } +/** + * The repository event filter modes, shared by the descriptor options and + * the validator so the UI and the runtime cannot drift. `off` (or absent) + * processes every repository and ignores the list entirely. + */ +export const REPO_FILTER_MODES = ['off', 'allowlist', 'blocklist'] as const; + +const REPOSITORY_FULL_NAME_RE = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; + +interface RepoFilter { + mode: 'allowlist' | 'blocklist'; + repos: Set; +} + +/** + * Parse and validate the repository event filter. Repository full names are + * case-insensitive on GitHub, so entries are matched lowercased. An active + * mode with an empty list is rejected: an empty allowlist would silently + * drop every event, and an empty blocklist is a no-op nobody asked for. + * While the mode is off the list is ignored, so a parked list never blocks + * an unrelated save or connect. + */ +function normalizeRepoFilter( + config: RepoFilterConfigFields, + channelName: string, +): RepoFilter | null { + const mode = config.repoFilterMode; + if (mode === undefined || mode === 'off') return null; + if (mode !== 'allowlist' && mode !== 'blocklist') { + throw new Error( + `repoFilterMode for channel ${channelName} must be one of: ${REPO_FILTER_MODES.join(', ')}.`, + ); + } + if (!Array.isArray(config.repoFilter)) { + throw new Error( + `repoFilter for channel ${channelName} must be an array of owner/repo repository names when repoFilterMode is "${mode}".`, + ); + } + if (config.repoFilter.some((entry) => typeof entry !== 'string')) { + throw new Error( + `repoFilter entries for channel ${channelName} must be strings.`, + ); + } + const repos = new Set( + config.repoFilter + .filter((entry): entry is string => typeof entry === 'string') + .map((entry) => entry.trim().toLowerCase()) + .filter((entry) => entry.length > 0), + ); + if (repos.size === 0) { + throw new Error( + `repoFilter for channel ${channelName} requires at least one owner/repo entry when repoFilterMode is "${mode}".`, + ); + } + const invalid = [...repos].filter( + (entry) => !REPOSITORY_FULL_NAME_RE.test(entry), + ); + if (invalid.length > 0) { + throw new Error( + `repoFilter entries for channel ${channelName} must be owner/repo repository names: ${invalid.join(', ')}`, + ); + } + return { mode, repos }; +} + +/** + * Shared validation for the plugin descriptor (returns the message) and + * connect() (throws it): one source of truth for the repository filter + * rules, so the management UI and the adapter cannot drift (the + * validateBoundSessionConfig pattern). + */ +export function validateRepoFilterConfig( + config: RepoFilterConfigFields, + channelName: string, +): string | undefined { + try { + normalizeRepoFilter(config, channelName); + } catch (err) { + return err instanceof Error ? err.message : String(err); + } + return undefined; +} + /** * The prWatchEvents vocabulary, shared by the config validator, the watch * emit path, and the management UI's option list (built from this constant @@ -785,6 +873,8 @@ const MAX_WINDOW_KEYS = 8192; * first few individually and collapse the rest into one counted row. */ const MAX_REASON_FILTER_EVENTS_PER_POLL = 20; +/** Same journal bound for repository-filter skips (see the reason above). */ +const MAX_REPO_FILTER_EVENTS_PER_POLL = 20; interface InboundTaskDedupe { dispatchedBodies?: string[]; @@ -1214,6 +1304,7 @@ export class GithubChannel extends PollingChannelBase { private pendingFinalDeliveryRequestsActive = 0; private pendingFinalDeliveryRetryStopRequested = false; private reasonFilter: Set | null = null; + private repoFilter: RepoFilter | null = null; private deliverToBoundSessions = false; private prWatchEvents = new Set(); /** @@ -1449,6 +1540,7 @@ export class GithubChannel extends PollingChannelBase { async connect(): Promise { const cfg = this.config as GithubConfig; this.reasonFilter = normalizeReasonFilter(cfg, this.name); + this.repoFilter = normalizeRepoFilter(cfg, this.name); const boundConfigError = validateBoundSessionConfig(cfg, this.name); if (boundConfigError) { throw new Error(boundConfigError); @@ -2386,6 +2478,8 @@ export class GithubChannel extends PollingChannelBase { } let reasonFilterJournaled = 0; let reasonFilterCollapsed = 0; + let repoFilterJournaled = 0; + let repoFilterCollapsed = 0; let transientHeld = false; let sharedHeld = false; for (const notification of notifications) { @@ -2398,6 +2492,38 @@ export class GithubChannel extends PollingChannelBase { const { chatId, threadId, issueNumber } = extracted; const lastReadAt = notification.last_read_at; const reason = String(notification.reason ?? '').toLowerCase(); + if (this.repoFilter) { + const listed = this.repoFilter.repos.has(chatId.toLowerCase()); + const filtered = + this.repoFilter.mode === 'allowlist' ? !listed : listed; + if (filtered) { + process.stderr.write( + `[Channel:${this.name}] skipping notification (repository=${sanitizeLogText(chatId, 140)} excluded by repoFilter ${this.repoFilter.mode}, subject=${sanitizeLogText(String(notification.subject.url ?? ''), 200)})\n`, + ); + this.logDebugPayload('Github', { + event: 'repoFilter.skip', + chatId, + threadId, + mode: this.repoFilter.mode, + reason: notification.reason, + }); + if (repoFilterJournaled < MAX_REPO_FILTER_EVENTS_PER_POLL) { + repoFilterJournaled += 1; + this.recordChannelEvent({ + type: 'inbound', + outcome: 'skipped', + reason: 'repo_filter', + triggerKind: reason, + repository: chatId, + thread: threadId, + number: issueNumber, + }); + } else { + repoFilterCollapsed += 1; + } + continue; + } + } if (this.reasonFilter && !this.reasonFilter.has(reason)) { process.stderr.write( `[Channel:${this.name}] skipping notification (reason=${sanitizeLogText(reason, 64)} not in reasonFilter, subject=${sanitizeLogText(String(notification.subject.url ?? ''), 200)})\n`, @@ -2576,6 +2702,14 @@ export class GithubChannel extends PollingChannelBase { detail: { filtered: reasonFilterCollapsed }, }); } + if (repoFilterCollapsed > 0) { + this.recordChannelEvent({ + type: 'inbound', + outcome: 'skipped', + reason: 'repo_filter', + detail: { filtered: repoFilterCollapsed }, + }); + } if (this.hasRecoverableInboundTasks()) { this.inboundRecoveryPending = true; } diff --git a/packages/channels/github/src/index.test.ts b/packages/channels/github/src/index.test.ts index 5310940892d..a959879be57 100644 --- a/packages/channels/github/src/index.test.ts +++ b/packages/channels/github/src/index.test.ts @@ -1,7 +1,11 @@ import { describe, expect, it } from 'vitest'; import { PR_BINDING_ROLES } from '@qwen-code/channel-base'; import { plugin } from './index.js'; -import { PR_NOTIFICATION_REASONS, PR_WATCH_EVENTS } from './GithubAdapter.js'; +import { + PR_NOTIFICATION_REASONS, + PR_WATCH_EVENTS, + REPO_FILTER_MODES, +} from './GithubAdapter.js'; function validate(overrides: Record): string | undefined { return plugin.management?.validateConfig?.({ @@ -78,6 +82,19 @@ describe('github plugin validateConfig', () => { ]); }); + it('builds the repoFilterMode UI options from the adapter vocabulary', () => { + const field = plugin.management?.fields.find( + (candidate) => candidate.key === 'repoFilterMode', + ); + // The settings store's enum branch requires membership in this option + // list, and the editor always submits the seeded value: dropping options + // or a mode would reject every Web Shell save with no other test red. + expect(field?.options?.map((option) => option.value)).toEqual([ + ...REPO_FILTER_MODES, + ]); + expect(field?.default).toBe('off'); + }); + it('describes and groups every boundSessionActions key for management UIs', () => { const field = plugin.management?.fields.find( (candidate) => candidate.key === 'boundSessionActions', diff --git a/packages/channels/github/src/index.ts b/packages/channels/github/src/index.ts index 671ad852539..c38c917a011 100644 --- a/packages/channels/github/src/index.ts +++ b/packages/channels/github/src/index.ts @@ -6,8 +6,10 @@ import type { ChannelPlugin } from '@qwen-code/channel-base'; import { GithubChannel, validateBoundSessionConfig, + validateRepoFilterConfig, PR_NOTIFICATION_REASONS, PR_WATCH_EVENTS, + REPO_FILTER_MODES, } from './GithubAdapter.js'; /** @@ -139,6 +141,24 @@ export const plugin: ChannelPlugin = { label: value, })), }, + { + key: 'repoFilterMode', + label: 'Repository Filter', + kind: 'enum', + default: 'off', + description: + 'Filter incoming notifications by repository: allowlist processes only the listed repositories, blocklist processes all except the listed ones. Notifications about PRs bound to a session are filtered too; only their PR state watch events bypass the filter.', + // Built from the adapter's exported vocabulary so the UI options + // cannot drift from the validator (the PR_WATCH_EVENTS pattern). + options: REPO_FILTER_MODES.map((value) => ({ value, label: value })), + }, + { + key: 'repoFilter', + label: 'Filtered Repositories', + kind: 'string-list', + description: + 'Comma-separated owner/repo repository names used by the repository filter. Requires Repository Filter to be set to allowlist or blocklist.', + }, { key: 'deliverToBoundSessions', label: 'Deliver To PR-Bound Sessions', @@ -172,7 +192,10 @@ export const plugin: ChannelPlugin = { if (!token && config['useLocalGh'] !== true) { return 'Channel requires a token or local GitHub CLI authentication (useLocalGh).'; } - return validateBoundSessionConfig(config, 'github'); + return ( + validateBoundSessionConfig(config, 'github') ?? + validateRepoFilterConfig(config, 'github') + ); }, }, createChannel: (name, config, bridge, options) => diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx index 631fcc0e6c0..4081c35dbc3 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx @@ -3772,3 +3772,308 @@ describe('Channel editor progressive disclosure', () => { expect(trigger?.textContent).toContain('已配置 2 项'); }); }); + +describe('GitHub repository event filter', () => { + // Same field keys as the real descriptor; labels intentionally differ from + // the i18n values so a missing FIELD_LABEL_KEYS entry surfaces the + // untranslated descriptor fallback instead of passing the assertions. + const GITHUB_REPO_FILTER: DaemonChannelTypeDescriptor = { + ...GITHUB_LOCAL_GH, + fields: [ + ...GITHUB_LOCAL_GH.fields, + { + key: 'repoFilterMode', + label: 'Repo filter mode (descriptor)', + kind: 'enum', + default: 'off', + options: [ + { value: 'off', label: 'off' }, + { value: 'allowlist', label: 'allowlist' }, + { value: 'blocklist', label: 'blocklist' }, + ], + }, + { + key: 'repoFilter', + label: 'Repo filter list (descriptor)', + kind: 'string-list', + }, + ], + }; + + const FILTER_INSTANCE: DaemonChannelInstanceSnapshot = { + name: 'github-bot', + config: { type: 'github', useLocalGh: true }, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + + async function saveDialog() { + const save = Array.from(document.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Save', + ); + await act(async () => { + save?.click(); + }); + } + + it('defaults to Off and hides the repository list', async () => { + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: FILTER_INSTANCE, + }); + + expect(fieldByLabel('Repository Filter')).not.toBeNull(); + expect(inputByLabel('Filtered Repositories')).toBeNull(); + }); + + it('renders the Chinese labels', async () => { + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: FILTER_INSTANCE, + language: 'zh-CN', + }); + + expect(fieldByLabel('按仓库过滤')).not.toBeNull(); + await selectOption('按仓库过滤', '黑名单 — 除列出的仓库以外'); + expect(inputByLabel('过滤的仓库')).not.toBeNull(); + }); + + it('reveals the list for an active mode and saves normalized repositories', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: FILTER_INSTANCE, + onSave, + }); + + await selectOption( + 'Repository Filter', + 'Blocklist — all except listed repositories', + ); + const input = inputByLabel('Filtered Repositories'); + expect(input).not.toBeNull(); + await act(async () => { + setInputValue(input!, 'https://github.com/Octo/Repo/pull/123, octo/repo'); + }); + await saveDialog(); + + expect(onSave).toHaveBeenCalledTimes(1); + const request = onSave.mock.calls[0]![1] as { + config: Record; + }; + expect(request.config['repoFilterMode']).toBe('blocklist'); + expect(request.config['repoFilter']).toEqual(['octo/repo']); + }); + + it('blocks an active mode with an empty list and retires the error on Off', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: FILTER_INSTANCE, + onSave, + }); + + await selectOption( + 'Repository Filter', + 'Allowlist — only listed repositories', + ); + await saveDialog(); + + expect(onSave).not.toHaveBeenCalled(); + expect(document.body.textContent).toContain( + 'List at least one repository, or set Repository Filter to Off.', + ); + + await selectOption('Repository Filter', 'Off — all repositories'); + expect(document.body.textContent).not.toContain( + 'List at least one repository, or set Repository Filter to Off.', + ); + expect(inputByLabel('Filtered Repositories')).toBeNull(); + + // The discriminating half: while Off the field is unmounted, so the + // assertion above cannot see a stale error. Switching back remounts it — + // the message must stay retired, which only happens if the mode switch + // re-derived the shown errors instead of leaving them in the map. + await selectOption( + 'Repository Filter', + 'Allowlist — only listed repositories', + ); + expect(inputByLabel('Filtered Repositories')).not.toBeNull(); + expect(document.body.textContent).not.toContain( + 'List at least one repository, or set Repository Filter to Off.', + ); + await selectOption('Repository Filter', 'Off — all repositories'); + + await saveDialog(); + expect(onSave).toHaveBeenCalledTimes(1); + const request = onSave.mock.calls[0]![1] as { + config: Record; + }; + expect(request.config['repoFilterMode']).toBe('off'); + }); + + it('retires the shown error when a valid entry is typed, without saving', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: FILTER_INSTANCE, + onSave, + }); + + await selectOption( + 'Repository Filter', + 'Allowlist — only listed repositories', + ); + await saveDialog(); + expect(onSave).not.toHaveBeenCalled(); + expect(document.body.textContent).toContain( + 'List at least one repository, or set Repository Filter to Off.', + ); + + await act(async () => { + setInputValue(inputByLabel('Filtered Repositories')!, 'octo/repo'); + }); + expect(document.body.textContent).not.toContain( + 'List at least one repository, or set Repository Filter to Off.', + ); + expect(onSave).not.toHaveBeenCalled(); + }); + + it('retires the format error when the entry is corrected, without saving', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: FILTER_INSTANCE, + onSave, + }); + + await selectOption( + 'Repository Filter', + 'Blocklist — all except listed repositories', + ); + await act(async () => { + setInputValue(inputByLabel('Filtered Repositories')!, 'not-a-repo'); + }); + await saveDialog(); + expect(onSave).not.toHaveBeenCalled(); + expect(document.body.textContent).toContain( + 'Enter each repository as owner/repo.', + ); + + await act(async () => { + setInputValue(inputByLabel('Filtered Repositories')!, 'octo/repo'); + }); + expect(document.body.textContent).not.toContain( + 'Enter each repository as owner/repo.', + ); + expect(onSave).not.toHaveBeenCalled(); + }); + + it('opens on a stored active mode with its list and re-saves both keys', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: { + ...FILTER_INSTANCE, + config: { + ...FILTER_INSTANCE.config, + repoFilterMode: 'blocklist', + repoFilter: ['octo/repo'], + }, + }, + onSave, + }); + + const input = inputByLabel('Filtered Repositories'); + expect(input).not.toBeNull(); + expect(input).toHaveProperty('value', 'octo/repo'); + + await saveDialog(); + expect(onSave).toHaveBeenCalledTimes(1); + const request = onSave.mock.calls[0]![1] as { + config: Record; + }; + expect(request.config['repoFilterMode']).toBe('blocklist'); + expect(request.config['repoFilter']).toEqual(['octo/repo']); + }); + + it('hides the list while Off and restores it on re-enable', async () => { + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: { + ...FILTER_INSTANCE, + config: { + ...FILTER_INSTANCE.config, + repoFilterMode: 'blocklist', + repoFilter: ['octo/repo'], + }, + }, + }); + + expect(inputByLabel('Filtered Repositories')).toHaveProperty( + 'value', + 'octo/repo', + ); + + await selectOption('Repository Filter', 'Off — all repositories'); + expect(inputByLabel('Filtered Repositories')).toBeNull(); + + await selectOption( + 'Repository Filter', + 'Blocklist — all except listed repositories', + ); + expect(inputByLabel('Filtered Repositories')).toHaveProperty( + 'value', + 'octo/repo', + ); + }); + + it('rejects entries that are not owner/repo after normalization', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: FILTER_INSTANCE, + onSave, + }); + + await selectOption( + 'Repository Filter', + 'Blocklist — all except listed repositories', + ); + await act(async () => { + setInputValue(inputByLabel('Filtered Repositories')!, 'not-a-repo'); + }); + await saveDialog(); + + expect(onSave).not.toHaveBeenCalled(); + expect(document.body.textContent).toContain( + 'Enter each repository as owner/repo.', + ); + }); + + it('keeps a parked list exactly as stored while the mode is off', async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: { + ...FILTER_INSTANCE, + config: { + ...FILTER_INSTANCE.config, + repoFilterMode: 'off', + repoFilter: ['Owner/Parked'], + }, + }, + onSave, + }); + + await saveDialog(); + + expect(onSave).toHaveBeenCalledTimes(1); + const request = onSave.mock.calls[0]![1] as { + config: Record; + }; + expect(request.config['repoFilterMode']).toBe('off'); + expect(request.config['repoFilter']).toEqual(['Owner/Parked']); + }); +}); diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx index 040639f4665..5be6e0ec50b 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx @@ -76,6 +76,7 @@ import { configuredPrivatePolicy, PRIVATE_ONLY_FIELD_KEYS, invalidRepositoryGroupEntries, + isRepoFilterActive, projectRepositoryGroup, recordHasRoute, repositoryAllowlistIds, @@ -117,6 +118,8 @@ const FIELD_LABEL_KEYS: Record> = { pollInterval: 'channels.editor.field.github.pollInterval', groupPolicy: 'channels.editor.field.github.groupPolicy', reasonFilter: 'channels.editor.field.github.reasonFilter', + repoFilterMode: 'channels.editor.field.github.repoFilterMode', + repoFilter: 'channels.editor.field.github.repoFilter', deliverToBoundSessions: 'channels.editor.field.github.deliverToBoundSessions', prWatchEvents: 'channels.editor.field.github.prWatchEvents', @@ -166,6 +169,7 @@ const SHARED_FIELD_LABEL_KEYS: Record = { /** Per-platform placeholders that spell out the expected value shape. */ const FIELD_PLACEHOLDER_KEYS: Record = { 'github.baseUrl': 'https://ghe.example.com/api/v3', + 'github.repoFilter': 'owner/repo, owner/repo', }; const CONNECTION_FIELD_KEYS = new Set(['baseUrl', 'wsUrl', 'pollInterval']); @@ -483,6 +487,8 @@ export function ChannelEditorDialog({ return t('channels.editor.validation.invalidGroupId'); if (code === 'invalidRepoId') return t('channels.editor.validation.invalidRepoId'); + if (code === 'repoFilterRequired') + return t('channels.editor.validation.repoFilterRequired'); if (code === 'conflictingRepoAliases') return t('channels.editor.validation.conflictingRepoAliases'); if (code === 'invalidOption') @@ -720,7 +726,13 @@ export function ChannelEditorDialog({ const id = `${formId}-${field.key}`; const value = draft.values[field.key]; const error = errors[field.key]; - const update = (next: string | boolean) => + const update = (next: string | boolean) => { + if (field.key === 'repoFilterMode' || field.key === 'repoFilter') { + // Both remedies retire a shown error: switching to off hides the + // list, and typing a valid entry satisfies the instruction. Submit + // stays the only site that populates errors. + refreshErrors({ ...draft.values, [field.key]: next }); + } setDraft((current) => ({ ...current, values: { @@ -731,6 +743,7 @@ export function ChannelEditorDialog({ : {}), }, })); + }; if (field.kind === 'boolean') { const isLocalGhSwitch = field.key === 'useLocalGh' && @@ -1545,7 +1558,16 @@ export function ChannelEditorDialog({ {(['credentials', 'connection'] as const).map((section) => { const sectionFields = credentialFields.filter( - (field) => fieldSectionKey(field) === section, + (field) => + fieldSectionKey(field) === section && + // The repository list only means something while a filter + // mode is active; validation is gated on the same + // predicate, so the hidden field can never strand a save. + !( + field.key === 'repoFilter' && + isRepositoryOnlyChannelType(descriptor.type) && + !isRepoFilterActive(draft.values['repoFilterMode']) + ), ); if (sectionFields.length === 0) return null; return ( diff --git a/packages/web-shell/client/components/channels/channel-editor-parity.test.ts b/packages/web-shell/client/components/channels/channel-editor-parity.test.ts index 6eda135f642..a21d24a5bb4 100644 --- a/packages/web-shell/client/components/channels/channel-editor-parity.test.ts +++ b/packages/web-shell/client/components/channels/channel-editor-parity.test.ts @@ -11,6 +11,7 @@ import type { DaemonChannelTypeDescriptor, } from '@qwen-code/sdk/daemon'; import { + buildChannelUpsertRequest, createChannelEditorDraft, validateChannelEditorDraft, } from './channel-editor-state'; @@ -101,3 +102,139 @@ describe('bound-delivery rule parity (editor mirror vs daemon authority)', () => } }); }); + +const REPO_FILTER_CASES = [ + { + name: 'active allowlist with an empty list', + mode: 'allowlist', + repos: [] as string[] | undefined, + code: 'repoFilterRequired', + daemon: /requires at least one owner\/repo entry/, + }, + { + name: 'active blocklist without a list', + mode: 'blocklist', + repos: undefined, + code: 'repoFilterRequired', + daemon: /must be an array of owner\/repo/, + }, + { + name: 'malformed entry', + mode: 'blocklist', + repos: ['not-a-repo'], + code: 'invalidRepoId', + daemon: /must be owner\/repo repository names/, + }, + { + name: 'valid allowlist', + mode: 'allowlist', + repos: ['octo/repo'], + code: undefined, + daemon: undefined, + }, + { + name: 'valid blocklist', + mode: 'blocklist', + repos: ['octo/repo'], + code: undefined, + daemon: undefined, + }, + { + name: 'off with a stale list', + mode: 'off', + repos: ['not-a-repo'], + code: undefined, + daemon: undefined, + }, +] as const; + +describe('repository filter rule parity (editor mirror vs daemon authority)', () => { + it.each(REPO_FILTER_CASES)('$name', ({ mode, repos, code, daemon }) => { + const config: Record = { + type: 'github', + useLocalGh: true, + repoFilterMode: mode, + }; + if (repos !== undefined) config['repoFilter'] = [...repos]; + const instance: DaemonChannelInstanceSnapshot = { + name: 'parity', + config, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(descriptor, instance); + draft.name = 'parity'; + + const clientCode = validateChannelEditorDraft( + descriptor, + draft, + [], + instance, + )['repoFilter']; + expect(clientCode).toBe(code); + + const daemonMessage = management.validateConfig?.(config); + if (daemon) { + expect(daemonMessage).toMatch(daemon); + } else { + expect(daemonMessage).toBeUndefined(); + } + }); + + it('shortens pasted URLs before the daemon authority sees them', () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'parity', + config: { type: 'github', useLocalGh: true }, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(descriptor, instance); + draft.name = 'parity'; + draft.values['repoFilterMode'] = 'blocklist'; + draft.values['repoFilter'] = 'https://github.com/Octo/Repo/pull/9'; + + expect(validateChannelEditorDraft(descriptor, draft, [], instance)).toEqual( + {}, + ); + const { config } = buildChannelUpsertRequest( + descriptor, + draft, + 'rev-1', + instance, + ); + expect(config['repoFilter']).toEqual(['octo/repo']); + expect(management.validateConfig?.(config)).toBeUndefined(); + }); + + it('binds a GHES paste through the editor and the daemon authority', () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'parity', + config: { + type: 'github', + useLocalGh: true, + baseUrl: 'https://ghe.example.com/api/v3', + }, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(descriptor, instance); + draft.name = 'parity'; + draft.values['repoFilterMode'] = 'blocklist'; + draft.values['repoFilter'] = 'https://ghe.example.com/octo/repo'; + + expect(validateChannelEditorDraft(descriptor, draft, [], instance)).toEqual( + {}, + ); + const { config } = buildChannelUpsertRequest( + descriptor, + draft, + 'rev-1', + instance, + ); + expect(config['repoFilter']).toEqual(['octo/repo']); + expect(management.validateConfig?.(config)).toBeUndefined(); + }); +}); 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 5a68089fc8e..b539ecdd506 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 @@ -12,7 +12,10 @@ import type { import { buildChannelUpsertRequest, createChannelEditorDraft, + GITHUB_RESERVED_URL_PATHS, invalidRepositoryGroupEntries, + isRepoFilterActive, + normalizeRepositoryFilterInput, repositoryAllowlistIds, retainedRepositorySettings, validateChannelEditorDraft, @@ -2953,3 +2956,386 @@ describe('GitHub poll interval', () => { ).toEqual({ pollInterval: 'outOfRange' }); }); }); + +describe('GitHub repository event filter', () => { + const REPO_FILTER: DaemonChannelTypeDescriptor = { + ...GITHUB, + fields: [ + ...GITHUB.fields, + { + key: 'baseUrl', + label: 'Base URL', + kind: 'string', + envResolvable: true, + }, + { + key: 'repoFilterMode', + label: 'Repository Filter', + kind: 'enum', + default: 'off', + options: [ + { value: 'off', label: 'off' }, + { value: 'allowlist', label: 'allowlist' }, + { value: 'blocklist', label: 'blocklist' }, + ], + }, + { + key: 'repoFilter', + label: 'Filtered Repositories', + kind: 'string-list', + }, + ], + }; + + function filterDraft( + mode: string, + list: string, + instance?: DaemonChannelInstanceSnapshot, + ): ChannelEditorDraft { + const draft = createChannelEditorDraft(REPO_FILTER, instance); + draft.name = 'my-bot'; + draft.values.useLocalGh = true; + draft.values.repoFilterMode = mode; + draft.values.repoFilter = list; + return draft; + } + + describe('normalizeRepositoryFilterInput', () => { + it('shortens pasted GitHub web URLs to owner/repo', () => { + expect( + normalizeRepositoryFilterInput( + 'https://github.com/Octo/Repo, https://github.com/Octo/Repo/pull/123, octo/repo', + ), + ).toBe('octo/repo'); + }); + + it('shortens URLs on the GHES host derived from baseUrl', () => { + expect( + normalizeRepositoryFilterInput( + 'https://ghe.example.com/octo/repo.git/', + 'https://ghe.example.com/api/v3', + ), + ).toBe('octo/repo'); + // Without the channel's baseUrl, the same host is not shortenable. + expect( + normalizeRepositoryFilterInput( + 'https://ghe.example.com/octo/repo.git/', + ), + ).toBe('https://ghe.example.com/octo/repo.git/'); + }); + + it('does not shorten URLs on hosts other than the channel GitHub host', () => { + // The structural half of the guard: the first two path segments of an + // arbitrary host are not a repository. Each of these once shortened to + // a plausible owner/repo that both validators accepted. + const foreign = [ + 'https://api.github.com/repos/octo/repo', + 'https://api.github.com/repos/QwenLM/qwen-code/issues/42', + 'https://gist.github.com/octocat/6cad1f0e5d3b2a9c', + 'https://gitlab.com/group/project', + ]; + for (const url of foreign) { + expect(normalizeRepositoryFilterInput(url)).toBe(url.toLowerCase()); + expect( + validateChannelEditorDraft( + REPO_FILTER, + filterDraft('allowlist', url), + [], + ), + ).toEqual({ repoFilter: 'invalidRepoId' }); + } + }); + + it('shortens URLs carrying a query or fragment', () => { + expect( + normalizeRepositoryFilterInput( + 'https://github.com/Octo/Repo?tab=readme-ov-file, https://github.com/Octo/Repo#readme', + ), + ).toBe('octo/repo'); + }); + + it('rejects REST-shaped URLs on the channel own API host', () => { + // The host gate must not re-admit the channel's own API host: with + // baseUrl set, a REST URL (the shape `gh api` prints) must pass through + // for validation to reject, not shorten to `repos/octo` or `api/v3`. + const cases = [ + ['https://api.github.com/repos/octo/repo', 'https://api.github.com'], + [ + 'https://ghe.example.com/api/v3/repos/octo/repo', + 'https://ghe.example.com/api/v3', + ], + ]; + for (const [url, baseUrl] of cases) { + expect(normalizeRepositoryFilterInput(url!, baseUrl)).toBe( + url!.toLowerCase(), + ); + const draft = filterDraft('allowlist', url!); + draft.values.baseUrl = baseUrl!; + expect(validateChannelEditorDraft(REPO_FILTER, draft, [])).toEqual({ + repoFilter: 'invalidRepoId', + }); + } + }); + + it('strips trailing sentence punctuation from a pasted URL', () => { + expect( + normalizeRepositoryFilterInput( + 'https://github.com/octo/repo., https://github.com/octo/repo)', + ), + ).toBe('octo/repo'); + expect( + validateChannelEditorDraft( + REPO_FILTER, + filterDraft('blocklist', 'https://github.com/octo/repo.'), + [], + ), + ).toEqual({}); + }); + + it('binds the GHES host through both the validator and the save path', () => { + // Dropping either baseUrl argument must fail here: the validator would + // reject the paste while the save path still shortens it (or vice + // versa) — the editor/daemon divergence the shared normalizer prevents. + const instance: DaemonChannelInstanceSnapshot = { + name: 'my-bot', + config: { + type: 'github', + useLocalGh: true, + baseUrl: 'https://ghe.example.com/api/v3', + }, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = filterDraft( + 'blocklist', + 'https://ghe.example.com/octo/repo', + instance, + ); + expect( + validateChannelEditorDraft(REPO_FILTER, draft, [], instance), + ).toEqual({}); + const request = buildChannelUpsertRequest( + REPO_FILTER, + draft, + 'rev-1', + instance, + ); + expect(request.config['repoFilter']).toEqual(['octo/repo']); + }); + + it('does not shorten non-repository GitHub URLs', () => { + // Regression pins for the measured verify-round shapes: each of these + // once shortened to a plausible-but-nonexistent owner/repo that both + // validators accepted — fatal under an allowlist. The capitalized + // variant pins the guard's own lowercasing of the path root. + const hostile = [ + 'https://github.com/orgs/community', + 'https://github.com/Orgs/community', + 'https://github.com/settings/profile', + 'https://github.com/apps/renovate', + 'https://github.com/features/copilot', + 'https://github.com/trending/typescript', + 'https://github.com/pricing/enterprise', + 'https://github.com/advisories/GHSA-xxxx-xxxx-xxxx', + 'https://github.com/copilot/c/abc123', + ]; + for (const url of hostile) { + expect(normalizeRepositoryFilterInput(url)).toBe(url.toLowerCase()); + expect( + validateChannelEditorDraft( + REPO_FILTER, + filterDraft('allowlist', url), + [], + ), + ).toEqual({ repoFilter: 'invalidRepoId' }); + } + }); + + it('pins the whole reserved-area set, not a sample of it', () => { + // A snapshot over every member: removing any root from the set fails + // here, which the it.each enumeration alone cannot witness (it would + // simply run one case fewer). + expect([...GITHUB_RESERVED_URL_PATHS].sort()).toEqual( + [ + 'about', + 'account', + 'accounts', + 'advisories', + 'apps', + 'codespaces', + 'collections', + 'contact', + 'copilot', + 'customer-stories', + 'discussions', + 'enterprise', + 'enterprises', + 'events', + 'explore', + 'features', + 'followers', + 'following', + 'issues', + 'join', + 'login', + 'logout', + 'marketplace', + 'mobile', + 'new', + 'notifications', + 'orgs', + 'organizations', + 'password_reset', + 'premium-support', + 'pricing', + 'pulls', + 'readme', + 'search', + 'security', + 'sessions', + 'settings', + 'signup', + 'site', + 'sponsors', + 'stars', + 'team', + 'topics', + 'trending', + 'users', + 'watching', + ].sort(), + ); + }); + + it.each([...GITHUB_RESERVED_URL_PATHS])( + 'rejects every reserved GitHub area root: %s', + (root) => { + // Enumerates the whole guard set so a newly added root is covered the + // moment it lands, instead of waiting for a hand-picked fixture. + const url = `https://github.com/${root}/some-page`; + expect(normalizeRepositoryFilterInput(url)).toBe(url); + expect( + validateChannelEditorDraft( + REPO_FILTER, + filterDraft('blocklist', url), + [], + ), + ).toEqual({ repoFilter: 'invalidRepoId' }); + }, + ); + + it('lowercases and deduplicates plain entries', () => { + expect( + normalizeRepositoryFilterInput('Owner/Repo, owner/repo, other/repo'), + ).toBe('owner/repo, other/repo'); + }); + + it('passes non-URL entries through for validation to name', () => { + expect(normalizeRepositoryFilterInput('not-a-repo, a/b')).toBe( + 'not-a-repo, a/b', + ); + expect(normalizeRepositoryFilterInput(' ')).toBe(''); + }); + }); + + it('isRepoFilterActive only accepts the two list modes', () => { + expect(isRepoFilterActive('allowlist')).toBe(true); + expect(isRepoFilterActive('blocklist')).toBe(true); + expect(isRepoFilterActive('off')).toBe(false); + expect(isRepoFilterActive(undefined)).toBe(false); + }); + + it('seeds Off for a new draft and for an instance without the keys', () => { + expect(createChannelEditorDraft(REPO_FILTER).values.repoFilterMode).toBe( + 'off', + ); + const instance: DaemonChannelInstanceSnapshot = { + name: 'my-bot', + config: { type: 'github', useLocalGh: true }, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + expect( + createChannelEditorDraft(REPO_FILTER, instance).values.repoFilterMode, + ).toBe('off'); + }); + + it('requires a non-empty list while a mode is active', () => { + expect( + validateChannelEditorDraft(REPO_FILTER, filterDraft('allowlist', ''), []), + ).toEqual({ repoFilter: 'repoFilterRequired' }); + expect( + validateChannelEditorDraft( + REPO_FILTER, + filterDraft('blocklist', ' '), + [], + ), + ).toEqual({ repoFilter: 'repoFilterRequired' }); + }); + + it('rejects entries that are not owner/repo after normalization', () => { + expect( + validateChannelEditorDraft( + REPO_FILTER, + filterDraft('blocklist', 'not-a-repo'), + [], + ), + ).toEqual({ repoFilter: 'invalidRepoId' }); + }); + + it('accepts pasted URLs and ignores the list while off', () => { + expect( + validateChannelEditorDraft( + REPO_FILTER, + filterDraft('allowlist', 'https://github.com/Octo/Repo/pull/1'), + [], + ), + ).toEqual({}); + expect( + validateChannelEditorDraft( + REPO_FILTER, + filterDraft('off', 'not-a-repo'), + [], + ), + ).toEqual({}); + }); + + it('normalizes the list in the save payload while a mode is active', () => { + const request = buildChannelUpsertRequest( + REPO_FILTER, + filterDraft( + 'blocklist', + 'https://github.com/Octo/Repo/pull/9, Other/Repo', + ), + 'rev-1', + ); + expect(request.config['repoFilterMode']).toBe('blocklist'); + expect(request.config['repoFilter']).toEqual(['octo/repo', 'other/repo']); + }); + + it('keeps a parked list exactly as stored while the mode is off', () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'my-bot', + config: { + type: 'github', + useLocalGh: true, + repoFilterMode: 'off', + repoFilter: ['Owner/Parked'], + }, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(REPO_FILTER, instance); + draft.name = 'my-bot'; + const request = buildChannelUpsertRequest( + REPO_FILTER, + draft, + 'rev-1', + instance, + ); + expect(request.config['repoFilter']).toEqual(['Owner/Parked']); + }); +}); 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 405be598f81..2172f2c9834 100644 --- a/packages/web-shell/client/components/channels/channel-editor-state.ts +++ b/packages/web-shell/client/components/channels/channel-editor-state.ts @@ -56,6 +56,7 @@ export type ChannelEditorValidationCode = | 'invalidGroupId' | 'invalidOption' | 'invalidRepoId' + | 'repoFilterRequired' | 'conflictingRepoAliases' | 'invalidGroupEntry' | 'number' @@ -89,6 +90,154 @@ export function repositoryAllowlistIds(value: string): string[] { ); } +/** Whether the repository event filter processes a list at all. */ +export function isRepoFilterActive(mode: unknown): boolean { + return mode === 'allowlist' || mode === 'blocklist'; +} + +const GITHUB_REPOSITORY_URL_RE = + /^https?:\/\/[^/]+\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+?)(?:\.git)?(?:[/?#].*)?$/; + +/** + * GitHub areas whose two-segment URLs look like `owner/repo` but are not + * repositories. Shortening one would silently filter a nonexistent + * repository — fatal under an allowlist — so these URLs pass through + * unchanged and validation rejects them by name instead. + * + * Best-effort by construction: a denylist of GitHub's top-level paths cannot + * be complete, and a future GitHub area can reintroduce the hazard until it + * is added here. The enumerated test over this set keeps every listed root + * rejected; the residual risk is documented in the design doc. + * + * Exported so the test can enumerate every member instead of sampling a few. + */ +export const GITHUB_RESERVED_URL_PATHS = new Set([ + 'about', + 'account', + 'accounts', + 'advisories', + 'apps', + 'codespaces', + 'collections', + 'contact', + 'copilot', + 'customer-stories', + 'discussions', + 'enterprise', + 'enterprises', + 'events', + 'explore', + 'features', + 'followers', + 'following', + 'issues', + 'join', + 'login', + 'logout', + 'marketplace', + 'mobile', + 'new', + 'notifications', + 'orgs', + 'organizations', + 'password_reset', + 'premium-support', + 'pricing', + 'pulls', + 'readme', + 'search', + 'security', + 'sessions', + 'settings', + 'signup', + 'site', + 'sponsors', + 'stars', + 'team', + 'topics', + 'trending', + 'users', + 'watching', +]); + +/** + * The hosts whose two-segment paths may be repository URLs: github.com, plus + * the channel's own GitHub Enterprise WEB host derived from baseUrl. Only the + * web host qualifies: an `api.`-prefixed baseUrl host denotes the deployment's + * API, so the stripped form is added and the raw API host is NOT (pasting a + * REST URL like https://api.github.com/repos/octo/repo must not shorten to + * `repos/octo`). The baseUrl's own API path prefix (e.g. /api/v3) is returned + * alongside so REST-shaped paths on a shared web+API host are excluded too. + * Any other host — gist.github.com, gitlab.com, an arbitrary site — passes + * through unchanged for validation to reject. + */ +function repositoryUrlContext(baseUrl: string): { + hosts: Set; + apiPath: string; +} { + const hosts = new Set(['github.com', 'www.github.com']); + let apiPath = ''; + const trimmed = baseUrl.trim(); + if (trimmed) { + try { + const url = new URL(trimmed); + const host = url.hostname.toLowerCase(); + hosts.add(host.startsWith('api.') ? host.slice(4) : host); + const path = url.pathname.replace(/\/+$/, '').toLowerCase(); + if (path) apiPath = path; + } catch { + // A malformed baseUrl is its own field's validation error; only + // github.com remains shortenable here. + } + } + return { hosts, apiPath }; +} + +/** + * Canonicalize the repository filter input: lowercase, deduplicate, strip + * trailing sentence punctuation a paste can carry (`.`, `)`, …), and shorten + * GitHub web URLs (repository, PR, or issue pages, with an optional `.git`, + * trailing path, query, or fragment) to `owner/repo` — only when the URL's + * host is the channel's own GitHub web host and its path is not the baseUrl's + * API prefix. Entries that are not repository URLs on that host pass through + * unchanged so validation can name them. Shared by the validator and the save + * path so the two cannot disagree. + */ +export function normalizeRepositoryFilterInput( + value: string, + baseUrl = '', +): string { + const { hosts, apiPath } = repositoryUrlContext(baseUrl); + const entries = toList(value) + .map((raw) => raw.replace(/[.,;:!?)\]]+$/, '')) + .filter(Boolean) + .map((entry) => { + const match = GITHUB_REPOSITORY_URL_RE.exec(entry); + let url: URL | undefined; + if (match) { + try { + url = new URL(entry); + } catch { + url = undefined; + } + } + const onApiPath = + url !== undefined && + apiPath !== '' && + url.pathname.toLowerCase().startsWith(`${apiPath}/`); + const shortened = + match && + url !== undefined && + hosts.has(url.hostname.toLowerCase()) && + !onApiPath && + !GITHUB_RESERVED_URL_PATHS.has(match[1].toLowerCase()) + ? `${match[1]}/${match[2]}` + : entry; + return shortened.toLowerCase(); + }); + return [...new Set(entries)].join(', '); +} + export function toList(value: string): string[] { return value .split(',') @@ -224,6 +373,7 @@ function initialFieldValue( if (field.key === 'privatePolicy') return configuredPrivatePolicy(instance); if (field.key === 'groupPolicy') return 'disabled'; + if (field.key === 'repoFilterMode') return 'off'; if (field.key === 'outputMode') return field.default ?? ''; if (field.key === 'dmPolicy') return 'open'; if (field.key !== 'sessionScope') return ''; @@ -434,6 +584,23 @@ export function validateChannelEditorDraft( ) { errors['prWatchEvents'] = 'requiresBoundDelivery'; } + // Mirror validateRepoFilterConfig: an active mode needs a non-empty list + // of owner/repo entries. While the mode is off the list is ignored (and + // hidden), matching the runtime's ignore-while-off rule, so a parked list + // never strands a save on a field the operator cannot see. + if (isRepoFilterActive(draft.values['repoFilterMode'])) { + const normalized = normalizeRepositoryFilterInput( + String(draft.values['repoFilter'] ?? ''), + String(draft.values['baseUrl'] ?? ''), + ); + if (!normalized) { + errors['repoFilter'] = 'repoFilterRequired'; + } else if ( + toList(normalized).some((id) => !REPOSITORY_FULL_NAME_RE.test(id)) + ) { + errors['repoFilter'] = 'invalidRepoId'; + } + } } if ( !isRepositoryOnlyChannelType(descriptor.type) && @@ -843,6 +1010,22 @@ export function buildChannelUpsertRequest( } assignField(config, field, draft.values[field.key]); } + if ( + repositoryOnly && + isRepoFilterActive(config['repoFilterMode']) && + Array.isArray(config['repoFilter']) + ) { + // Same canonicalization the validator applied, so pasted URLs reach the + // daemon as owner/repo. Gated on the active mode: a parked list stays + // exactly as stored on an unrelated save. + const normalized = normalizeRepositoryFilterInput( + config['repoFilter'] + .filter((entry): entry is string => typeof entry === 'string') + .join(', '), + typeof config['baseUrl'] === 'string' ? config['baseUrl'] : '', + ); + config['repoFilter'] = normalized ? normalized.split(', ') : []; + } 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 diff --git a/packages/web-shell/client/e2e/visuals/screenshots.spec.ts b/packages/web-shell/client/e2e/visuals/screenshots.spec.ts index 3e7fd8f828c..2e52b3fda19 100644 --- a/packages/web-shell/client/e2e/visuals/screenshots.spec.ts +++ b/packages/web-shell/client/e2e/visuals/screenshots.spec.ts @@ -1595,6 +1595,69 @@ for (const theme of THEMES) { ); }); + test(`Channel repository filter editor`, async ({ page }, testInfo) => { + const scenario = createWebShellDaemonScenario({ + capabilities: { + features: [ + 'session_events', + 'permission_vote', + 'session_permission_vote', + 'session_scope_override', + 'session_source_metadata', + 'channel_management', + ], + }, + channelTypes: [ + { + type: 'github', + displayName: 'GitHub', + manageable: true, + fields: githubManagement.fields, + }, + ], + }); + await page.addInitScript(() => { + window.sessionStorage.setItem('qwen-daemon-token', 'visual-token'); + }); + const daemon = await installScenario( + page, + scenario, + resolveBaseURL(testInfo), + ); + await gotoSession(page, scenario, daemon, theme); + + await page.getByRole('button', { name: 'Channels' }).click(); + await page.getByRole('button', { name: 'Configure GitHub' }).click(); + const editor = page.getByRole('dialog'); + await expect( + editor.getByRole('heading', { name: 'Configure GitHub' }), + ).toBeVisible(); + await editor + .getByRole('switch', { name: 'Use Local GitHub CLI Authentication' }) + .click(); + + // Anchors: the filter defaults to Off and the repository list stays + // hidden until a mode is chosen. A renderer that always shows the list + // (or lacks the field) fails here instead of shipping a stale capture. + const modeSelect = editor.getByLabel('Repository Filter'); + await modeSelect.scrollIntoViewIfNeeded(); + await expect(editor.getByLabel('Filtered Repositories')).toHaveCount(0); + await captureScreenshot(page, `channel-repo-filter-off-${theme}`); + + await modeSelect.click(); + await page + .getByRole('option', { + name: 'Blocklist — all except listed repositories', + }) + .click(); + const repositories = editor.getByLabel('Filtered Repositories'); + await expect(repositories).toBeVisible(); + await repositories.fill( + 'https://github.com/QwenLM/qwen-code/pull/16117, octo/noisy-repo', + ); + await captureScreenshot(page, `channel-repo-filter-blocklist-${theme}`); + }); + test(`Channel events`, async ({ page }, testInfo) => { const scenario = createWebShellDaemonScenario({ capabilities: { diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index ad55d3e041e..cba08d99e5c 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -4770,6 +4770,18 @@ const EN: Messages = { '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.github.repoFilterMode': 'Repository Filter', + 'channels.editor.field.github.repoFilterMode.description': + 'Filter incoming notifications by repository. Notifications about PRs bound to a session are filtered too; only their PR state watch events bypass the filter.', + 'channels.editor.field.github.repoFilterMode.option.off': + 'Off — all repositories', + 'channels.editor.field.github.repoFilterMode.option.allowlist': + 'Allowlist — only listed repositories', + 'channels.editor.field.github.repoFilterMode.option.blocklist': + 'Blocklist — all except listed repositories', + 'channels.editor.field.github.repoFilter': 'Filtered Repositories', + 'channels.editor.field.github.repoFilter.description': + 'Comma-separated owner/repo names. You can paste repository or PR URLs; they are shortened to owner/repo on save.', 'channels.editor.field.github.deliverToBoundSessions': 'Deliver To PR-Bound Sessions', 'channels.editor.field.github.deliverToBoundSessions.description': @@ -4997,6 +5009,8 @@ const EN: Messages = { "Remove values that aren't in the allowed list.", 'channels.editor.validation.requiresBoundDelivery': 'Enable "Deliver To PR-Bound Sessions" to save the selected watch events.', + 'channels.editor.validation.repoFilterRequired': + 'List at least one repository, or set Repository Filter to Off.', 'channels.editor.validation.number': 'Enter a valid number.', 'channels.editor.validation.outOfRange': (v) => v?.min !== undefined && v?.max !== undefined @@ -9472,6 +9486,17 @@ const ZH: Messages = { 'member_feature_requested — 组织成员请求启用 Copilot 等功能', 'channels.editor.field.github.reasonFilter.option.security_advisory_credit': 'security_advisory_credit — 机器人因参与安全公告而被致谢', + 'channels.editor.field.github.repoFilterMode': '按仓库过滤', + 'channels.editor.field.github.repoFilterMode.description': + '按仓库过滤入站通知。绑定到会话的 PR,其通知同样被过滤;只有它们的状态监视事件绕过此过滤器。', + 'channels.editor.field.github.repoFilterMode.option.off': '关闭 — 所有仓库', + 'channels.editor.field.github.repoFilterMode.option.allowlist': + '白名单 — 仅列出的仓库', + 'channels.editor.field.github.repoFilterMode.option.blocklist': + '黑名单 — 除列出的仓库以外', + 'channels.editor.field.github.repoFilter': '过滤的仓库', + 'channels.editor.field.github.repoFilter.description': + '以逗号分隔的 owner/repo 仓库名。可以直接粘贴仓库或 PR 链接,保存时会自动缩短为 owner/repo。', 'channels.editor.field.github.deliverToBoundSessions': '投递到 PR 绑定会话', 'channels.editor.field.github.deliverToBoundSessions.description': '将已绑定到活跃会话的 PR 事件以 prompt 形式投递进该会话,而不是频道自有会话。需要 daemon 托管的频道。', @@ -9685,6 +9710,8 @@ const ZH: Messages = { 'channels.editor.validation.invalidOption': '请移除不在允许列表中的值。', 'channels.editor.validation.requiresBoundDelivery': '需开启「投递到 PR 绑定会话」才能保存所选的监视事件。', + 'channels.editor.validation.repoFilterRequired': + '请至少列出一个仓库,或把「按仓库过滤」设为关闭。', 'channels.editor.validation.number': '请输入有效数字。', 'channels.editor.validation.outOfRange': (v) => v?.min !== undefined && v?.max !== undefined