feat(channels): add GitLab polling channel adapter - #7862
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: This is a feature addition, not a bug fix — GitLab users currently have no channel adapter to connect their mentions to Qwen Code. The GitHub adapter only covers GitHub. This is a legitimate platform gap: GitLab is widely used, especially for self-hosted instances, and the channel system is explicitly designed for multi-platform support (DingTalk, WeChat, Telegram, Feishu, GitHub already exist). Direction: Aligned. The channel adapter architecture ( Size: Cross-package (new Approach: The scope feels right. The adapter follows the exact same architecture as the GitHub adapter — extends Risk: No elevated risk signals. No high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个新功能，不是 bug 修复——GitLab 用户目前没有 channel 适配器来将 mention 连接到 Qwen Code。GitHub 适配器只覆盖 GitHub。这是一个合理的平台缺口：GitLab 使用广泛（尤其是自托管实例），而 channel 系统本身就是为多平台支持设计的（已有 DingTalk、WeChat、Telegram、Feishu、GitHub）。 方向：对齐。Channel 适配器架构（ 规模：跨包（新 方案：范围合理。适配器完全遵循 GitHub 适配器的架构——继承 风险：无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: I would create a new Findings: One concern worth flagging: in Everything else is clean and follows conventions well:
No AGENTS.md violations. No over-abstraction. No scope creep. Testing
All CI checks on Sandboxed verification would settle the Real-scenario testing: N/A — this adapter requires a live GitLab instance and PAT to exercise; it cannot be driven locally without external credentials. The author reports 14/14 E2E tests passed against gitlab.com (author's claim, not independently re-run). Maintainer @wenshao has approved at this SHA. 中文说明代码审查独立方案： 我会创建一个新的 发现： 一个值得关注的点： 其余部分干净且遵循规范：适配器结构完全镜像 GitHub 适配器；cursor 语义扎实（单调递增 todo ID 消除了 GitHub 适配器需要处理的同时间戳碰撞问题）； 无 AGENTS.md 违规。无过度抽象。无范围蔓延。 测试
沙箱验证可以解决 真实场景测试：N/A——此适配器需要活跃的 GitLab 实例和 PAT 才能运行；没有外部凭据无法在本地驱动。作者报告 14/14 E2E 测试在 gitlab.com 上通过（作者声明，非独立重新运行）。维护者 @wenshao 已在此 SHA 上批准。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — solid adapter that follows the established pattern cleanly; one non-blocking concern about Stepping back: this is exactly the kind of PR the channel system was designed to receive. The author studied the GitHub adapter, understood the My independent proposal matched the PR's approach almost line-for-line, which is a good sign — there wasn't a simpler path that was missed. The only thing I'd do differently is double-check the Six months from now, this is a package I'd be happy to maintain — it's self-contained, well-documented, and follows the same conventions as its siblings. Approving. The 中文说明置信度：4/5 ——扎实的适配器，干净地遵循了既有模式；一个关于 退一步看：这正是 channel 系统被设计来接收的那种 PR。作者研究了 GitHub 适配器，理解了 我的独立方案与 PR 的方案几乎逐行匹配，这是个好迹象——没有遗漏更简单的路径。我唯一会不同的是仔细检查 六个月后，这是一个我乐于维护的包——它自包含、文档良好、遵循与其兄弟包相同的规范。 批准。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Two issues need fixing before this can merge — see my detailed notes above. 🙏
- Build failure: add
{ "path": "../channels/gitlab" }to thereferencesarray inpackages/cli/tsconfig.json. - Cursor retry bug:
lastProcessedAtadvances past all todos before processing, so failed todos are silently dropped on the next poll instead of being retried. Advance the cursor per-todo on success, or track failed IDs.
06438f2 to
966a7e2
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Poll GitLab todos via @gitbeaker/rest, dispatch notes through the existing PollingChannelBase pipeline. Key design points: - action_prompt_template config drives event filtering and metadata rendering (unconfigured actions are skipped) - Per-repo cursor (repo[chatId].last_read) as notes window lower bound, global lastProcessedAt for todo-level dedup - mark_done after successful processing; failure skips mark_done for retry on next poll - Mention gating delegated to base GroupGate (adapter only sets isMentioned flag) - First-contact body fallback for todos with no notes (e.g. mention in issue description)
966a7e2 to
5c90c70
Compare
Call saveCursor() immediately after advancing lastProcessedAt so that progress is durable even if the process crashes mid-poll. Also removes the local watermark variable in favor of direct assignment.
| this.api = new Gitlab({ | ||
| host, | ||
| token: cfg.token, | ||
| ...(proxyAgent ? { proxyAgent } : {}), | ||
| }); |
There was a problem hiding this comment.
[Critical] proxyAgent is not a valid constructor option for @gitbeaker/rest's Gitlab class — it is silently absorbed into the ...tokens rest parameter and never used. The default requester uses global fetch() with no proxy support.
Failure scenario: A user configures a proxy to reach a self-hosted GitLab instance. The adapter creates a ProxyAgent from undici but passes it as an unrecognized property. All API requests bypass the proxy and fail with connection errors.
The GitHub adapter correctly passes proxy via Octokit's { request: { agent } } option. For @gitbeaker/rest, use the requesterFn option to provide a custom requester that dispatches through undici's ProxyAgent, or remove the proxy code and document the limitation.
中文说明
[Critical] proxyAgent 不是 @gitbeaker/rest 的 Gitlab 类的有效构造选项——它被静默吸收到 ...tokens 剩余参数中且从未使用。默认 requester 使用全局 fetch()，不支持代理。
失败场景：用户配置代理以访问自托管 GitLab 实例。适配器创建了 ProxyAgent 但作为无法识别的属性传递。所有 API 请求绕过代理，导致连接错误。
建议使用 requesterFn 选项提供自定义 requester，通过 undici 的 ProxyAgent 进行调度。
— qwen3.7-max via Qwen Code /review
| } catch (err) { | ||
| process.stderr.write( | ||
| `[Channel:${this.name}] error processing todo ${todo.id}, skipping: ${err}\n`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Critical] This catch block silently swallows all errors from processTodo, creating two failure modes:
(1) Failed todos permanently lost: When todo A fails but a later todo B in the same batch succeeds, lastProcessedAt advances past A's updated_at. On the next poll, A is filtered out by t.updated_at > windowSince and never retried — even though it remains pending in GitLab.
(2) No user feedback on Notes API failure: When IssueNotes.all() or MergeRequestNotes.all() throws, the error reaches this catch (no postErrorComment is called). The user sees no response on their issue/MR.
| } catch (err) { | |
| process.stderr.write( | |
| `[Channel:${this.name}] error processing todo ${todo.id}, skipping: ${err}\n`, | |
| ); | |
| } | |
| } catch (err) { | |
| await this.postErrorComment(chatId, targetType, todo.target.iid).catch(() => {}); | |
| process.stderr.write( | |
| `[Channel:${this.name}] error processing todo ${todo.id}, skipping: ${err}\n`, | |
| ); | |
| break; | |
| } |
Adding break prevents the loop from advancing the cursor past a failed todo. Adding postErrorComment gives the user feedback.
中文说明
[Critical] 此 catch 块静默吞掉 processTodo 的所有错误，造成两种失败模式：
(1) 失败的 todo 永久丢失：当 todo A 失败但后续 todo B 成功时，lastProcessedAt 推进超过 A 的 updated_at。下次轮询时 A 被过滤掉，永远不会重试。
(2) Notes API 失败时无用户反馈：当 IssueNotes.all() 抛出异常时，没有调用 postErrorComment，用户在 issue/MR 上看不到任何反馈。
— qwen3.7-max via Qwen Code /review
| await this.postErrorComment(chatId, targetType, todo.target.iid); | ||
| throw err; |
There was a problem hiding this comment.
[Critical] postErrorComment + throw err creates a retry loop that produces duplicate error comments and duplicate agent dispatches on every poll cycle:
handleInboundfails → error comment posted → throw propagates topollOncecatch- Todo stays pending (not marked done), cursor not advanced
- Next poll: same todo re-fetched, same notes pass the unchanged window filter → error comment posted AGAIN + notes 1..N-1 re-dispatched to
handleInbound
This produces ~60 duplicate "
The GitHub adapter uses break (not throw) after postErrorComment, preventing re-processing.
| await this.postErrorComment(chatId, targetType, todo.target.iid); | |
| throw err; | |
| await this.postErrorComment(chatId, targetType, todo.target.iid); | |
| break; |
中文说明
[Critical] postErrorComment + throw err 创建了一个重试循环，每次轮询都会产生重复的错误评论和重复的 agent 调度：
handleInbound失败 → 发布错误评论 → throw 传播到pollOncecatch- Todo 保持 pending（未标记完成），cursor 未推进
- 下次轮询：重新获取同一 todo → 再次发布错误评论 + 已处理的笔记被重新调度
GitHub 适配器在 postErrorComment 后使用 break（而非 throw）来防止重新处理。
— qwen3.7-max via Qwen Code /review
| try { | ||
| await this.handleInbound(envelope); | ||
| } catch (err) { | ||
| process.stderr.write( | ||
| `[Channel:${this.name}] handleInbound failed for todo body ${todo.id}: ${err}\n`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] This catch block silently swallows handleInbound failures for first-contact body dispatches. Unlike the notes path (which posts an error comment and rethrows), this path only writes to stderr. The todo is still marked done by pollOnce, so the user's mention is permanently lost with no feedback.
Concrete cost: A user mentions the bot in an issue body, the agent fails (model timeout), and the user never knows — no error comment, no retry, the todo vanishes.
Align with the notes path: add await this.postErrorComment(...) before the stderr write.
中文说明
[Suggestion] 此 catch 块静默吞掉首次联系人 body 调度的 handleInbound 失败。与 notes 路径（发布错误评论并重新抛出）不同，此路径仅写入 stderr。Todo 仍被标记为 done，用户的 mention 永久丢失且无反馈。
建议与 notes 路径对齐：在 stderr 写入前添加 await this.postErrorComment(...)。
— qwen3.7-max via Qwen Code /review
| export function testBotMention(text: string, username: string): boolean { | ||
| const re = new RegExp( | ||
| `${MENTION_LOOKBEHIND}@${escapeRegex(username)}${MENTION_LOOKAHEAD}`, |
There was a problem hiding this comment.
[Suggestion] No dedicated test file for mention.ts. The GitHub adapter has mention.test.ts (86 lines) covering regex edge cases: case-insensitive matching, escapeRegex with special characters, false-positive prevention, and stripBotMention edge cases. The GitLab adapter tests only exercise testBotMention indirectly through one adapter test case.
Concrete cost: A regression in the lookbehind/lookahead assertions or escapeRegex (e.g., usernames containing dots or brackets) would not be caught.
中文说明
[Suggestion] mention.ts 缺少专用测试文件。GitHub 适配器有 mention.test.ts（86 行）覆盖正则边界情况。当前仅通过一个适配器测试间接覆盖了 testBotMention。
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
中文说明
未审查：反向审计——有审计 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，agent 实际被要求做的并不是本 skill 所认证的内容。
— qwen3.7-max via Qwen Code /review
| } catch (err) { | ||
| process.stderr.write( | ||
| `[Channel:${this.name}] handleInbound failed for todo body ${todo.id}: ${err}\n`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Critical] The todo-body fallback path silently swallows handleInbound errors — unlike the note path (lines 239–245) which re-throws and posts an error comment. Because processTodo returns normally, the caller marks the todo done and advances the cursor, permanently losing the user's message.
— Failure scenario: A user creates a GitLab issue mentioning the bot (first-contact, no existing notes). The agent session fails (model timeout, bridge error). The error is logged to stderr but no error comment is posted on the issue, and the todo is marked done. The user never receives a response and the bot never retries. This contradicts the PR's stated guarantee "失败不 mark，下轮 poll 重试".
| } catch (err) { | |
| process.stderr.write( | |
| `[Channel:${this.name}] handleInbound failed for todo body ${todo.id}: ${err}\n`, | |
| ); | |
| } | |
| } catch (err) { | |
| process.stderr.write( | |
| `[Channel:${this.name}] handleInbound failed for todo body ${todo.id}: ${err}\n`, | |
| ); | |
| await this.postErrorComment(chatId, targetType, todo.target.iid); | |
| throw err; | |
| } |
中文说明
todo-body 回退路径静默吞掉了 handleInbound 错误——与 note 路径（239-245 行）不同，后者会 re-throw 并发送错误评论。因为 processTodo 正常返回，调用方会将 todo 标记为 done 并推进 cursor，导致用户消息永久丢失。这与 PR 声明的 "失败不 mark，下轮 poll 重试" 相矛盾。
— qwen3.7-max via Qwen Code /review
| let proxyAgent: unknown; | ||
| if (this.proxy) { | ||
| const { ProxyAgent } = await import('undici'); | ||
| proxyAgent = new ProxyAgent(this.proxy); | ||
| } | ||
|
|
||
| this.api = new Gitlab({ | ||
| host, | ||
| token: cfg.token, | ||
| ...(proxyAgent ? { proxyAgent } : {}), | ||
| }); |
There was a problem hiding this comment.
[Critical] proxyAgent is silently ignored by @gitbeaker/rest v42 — the proxy configuration is non-functional. The BaseResource constructor destructures only recognized options and captures unrecognized properties in a ...tokens rest that silently discards them. The library's defaultRequestHandler uses bare fetch() with no dispatcher support.
— Failure scenario: A user configures a GitLab channel behind a corporate proxy. All API requests go direct, bypassing the proxy entirely. Requests either fail with connection timeout or leak outside the intended network path, with no error or warning.
| let proxyAgent: unknown; | |
| if (this.proxy) { | |
| const { ProxyAgent } = await import('undici'); | |
| proxyAgent = new ProxyAgent(this.proxy); | |
| } | |
| this.api = new Gitlab({ | |
| host, | |
| token: cfg.token, | |
| ...(proxyAgent ? { proxyAgent } : {}), | |
| }); | |
| if (this.proxy) { | |
| const { setGlobalDispatcher, ProxyAgent } = await import('undici'); | |
| setGlobalDispatcher(new ProxyAgent(this.proxy)); | |
| } | |
| this.api = new Gitlab({ | |
| host, | |
| token: cfg.token, | |
| }); |
中文说明
proxyAgent 被 @gitbeaker/rest v42 静默忽略——代理配置无效。所有 API 请求直连，完全绕过代理。请求要么超时报错，要么泄漏到预期网络路径之外。
— qwen3.7-max via Qwen Code /review
| { name: 'feishu', promise: import('@qwen-code/channel-feishu') }, | ||
| { name: 'qqbot', promise: import('@qwen-code/channel-qqbot') }, | ||
| { name: 'github', promise: import('@qwen-code/channel-github') }, | ||
| { name: 'gitlab', promise: import('@qwen-code/channel-gitlab') }, |
There was a problem hiding this comment.
[Critical] Adding gitlab to the channel registry without updating channel-registry.test.ts breaks the existing test. CI test step for packages/cli exits non-zero, blocking merge.
— Failure scenario: npx vitest run on packages/cli fails with AssertionError — supportedChannelCatalog() now returns 8 entries including gitlab, but the test expects 7.
中文说明
在 channel registry 中添加 gitlab 但未更新 channel-registry.test.ts，导致现有测试失败。CI 非零退出，阻塞合并。
— qwen3.7-max via Qwen Code /review
| } catch (err) { | ||
| process.stderr.write( | ||
| `[Channel:${this.name}] error processing todo ${todo.id}, stopping: ${err}\n`, | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
[Critical] The break on error halts the entire for loop, causing two failure modes: (a) Head-of-line blocking — a single consistently-failing todo permanently stalls the entire channel. (b) Duplicate dispatch — when a todo has multiple notes and an earlier note succeeds but a later one fails, the cursor is never advanced, so on the next poll the same notes are dispatched again.
— Failure scenario: (a) A transient error on one todo permanently stops all message processing across all repos. (b) A user sees two bot responses for the same mention. The GitHub adapter avoids both by advancing lastProcessedAt before the loop and using continue on errors.
| } catch (err) { | |
| process.stderr.write( | |
| `[Channel:${this.name}] error processing todo ${todo.id}, stopping: ${err}\n`, | |
| ); | |
| break; | |
| } | |
| } catch (err) { | |
| process.stderr.write( | |
| `[Channel:${this.name}] error processing todo ${todo.id}, skipping: ${err}\n`, | |
| ); | |
| this.cursor.lastProcessedAt = todo.updated_at; | |
| this.saveCursor(); | |
| continue; | |
| } |
中文说明
break 在出错时中止整个 for 循环，导致：(a) 队头阻塞——一个持续失败的 todo 永久阻塞整个 channel；(b) 重复派发——前面成功的 note 在下次 poll 时被重新派发。GitHub adapter 通过在循环前推进 cursor 并使用 continue 来避免这两个问题。
— qwen3.7-max via Qwen Code /review
| export function escapeRegex(str: string): string { | ||
| return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] mention.ts is a byte-for-byte copy of packages/channels/github/src/mention.ts but has no dedicated test file. The GitHub channel has ~15 test cases for the same code. A future regex regression here would go undetected.
— Concrete cost: A refactor of the mention regex silently breaks mention detection, causing the bot to ignore messages intended for it.
中文说明
mention.ts 是 GitHub channel 同名文件的逐字节复制，但没有专门的测试文件。未来的正则表达式回归将无法被检测到。
— qwen3.7-max via Qwen Code /review
| } catch (err) { | ||
| process.stderr.write( | ||
| `[Channel:${this.name}] handleInbound failed for note ${note.id}: ${err}\n`, | ||
| ); | ||
| await this.postErrorComment(chatId, targetType, todo.target.iid); | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
[Suggestion] When handleInbound fails, postErrorComment posts on the issue, then the todo retries next poll cycle, posting another identical error comment. This repeats every poll cycle (~60s) until the failure clears.
— Concrete cost: A transient failure produces 10+ identical error comments on the same GitLab issue within 10 minutes.
中文说明
handleInbound 失败时发送错误评论，然后 todo 在下次 poll 重试，再次发送相同的错误评论。每个 poll 周期重复，一个临时故障会在 10 分钟内产生 10+ 条相同的错误评论。
— qwen3.7-max via Qwen Code /review
| it('stops processing on failure, does not advance cursor past failed todo', async () => { | ||
| await initWithoutLoop(); |
There was a problem hiding this comment.
[Suggestion] No test covers TodoLists.done rejecting after successful handleInbound. If done fails, the cursor never advances and the todo stays pending — on the next poll, the same message is dispatched again, producing duplicate agent responses.
— Concrete cost: A GitLab API hiccup after successful processing causes duplicate bot responses.
中文说明
没有测试覆盖 handleInbound 成功后 TodoLists.done 失败的情况。如果 done 失败，cursor 不推进，下次 poll 会重复派发相同消息。
— qwen3.7-max via Qwen Code /review
| if ( | ||
| !todo.target || | ||
| (todo.target_type !== 'Issue' && todo.target_type !== 'MergeRequest') |
There was a problem hiding this comment.
[Suggestion] The filter guards !todo.target and wrong target_type, but does not check todo.target.iid. A todo with target present but iid undefined would construct threadId = "issue:undefined", cause IssueNotes.all to reject, and permanently stall the poll loop via break.
— Concrete cost: A single malformed todo from GitLab's API blocks all subsequent todo processing.
| if ( | |
| !todo.target || | |
| (todo.target_type !== 'Issue' && todo.target_type !== 'MergeRequest') | |
| if ( | |
| !todo.target || | |
| !todo.target.iid || | |
| (todo.target_type !== 'Issue' && todo.target_type !== 'MergeRequest') |
中文说明
过滤器未检查 todo.target.iid。如果 GitLab 返回 iid 为 undefined 的 todo，会导致 IssueNotes.all 抛异常，外层 break 永久阻塞 poll 循环。添加 !todo.target.iid 守卫可优雅跳过。
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Three findings cannot be anchored to a line in this diff:
[Critical] packages/cli/src/commands/channel/channel-registry.test.ts:14 — CI is red. Adding gitlab to ensureBuiltins() breaks the exact-equality assertion. Reproduced locally:
AssertionError: expected [ 'telegram', 'weixin', …(6) ] to deeply equal [ 'telegram', 'weixin', …(5) ]
+ "gitlab",
This is the failing Test (ubuntu-latest, Node 22.x) check. Fix: add 'gitlab', after 'github', in the toEqual([...]) list. The manageable list needs no change — the GitLab plugin exposes no management block.
[Suggestion] scripts/build.js:63 and scripts/clean-package-build-artifacts.js:25 — packages/channels/gitlab is missing from buildOrder and CLI_BUILD_PACKAGE_PATHS; every other channel workspace (including plugin-example) is in both, and #7632 added packages/channels/github to both in the same commit. Not fatal today — the package builds transitively via the tsconfig reference this PR added — but the clean script will never remove packages/channels/gitlab/dist, so npm run check:serve-fast-path-bundle can run against stale output.
[Suggestion] Documentation is entirely absent. grep -rn -i gitlab docs/ returns nothing. #7632 shipped docs/users/features/channels/github.md, a _meta.ts nav entry, three overview.md updates (including the type column that enumerates valid channel types), and rows in both tables of docs/developers/daemon/15-channel-adapters.md. Without at least overview.md and a gitlab.md, an operator has no way to learn that action_prompt_template is required or which %vars% exist.
Correction to earlier review rounds: the previously reported "missing { "path": "../channels/gitlab" } in packages/cli/tsconfig.json" is not an issue at this HEAD — it is present at line 121, added by this PR's own first commit 5c90c7010. npx tsc --build packages/channels/gitlab succeeds cleanly and the packages/cli type-check produces zero errors mentioning gitlab, gitbeaker, or channel-registry.
Also verified clean: ESLint and Prettier pass on the new package, scripts/check-lockfile.js passes, and the package's own 32 tests pass. The architecture is a good fit for PollingChannelBase and the gate wiring in connect() correctly mirrors the GitHub adapter — the issues above are in the GitLab API contract and the cursor model, not the overall shape.
— claude-opus-5[1m] via Qwen Code /qreview
| return template.replace(/%(\w+)%/g, (match, key: string) => { | ||
| const vars: Record<string, string> = { | ||
| repo: chatId, | ||
| repo_url: todo.project.web_url, |
There was a problem hiding this comment.
[Critical] %repo_url% can never resolve — GitLab's GET /todos does not return project.web_url.
Entities::Todo exposes project via Entities::ProjectIdentity, which exposes exactly id, description, name, name_with_namespace, path, path_with_namespace, created_at — no web_url. gitbeaker's own type agrees: TodoSchema.project: Pick<SimpleProjectSchema, 'id' | 'name' | 'name_with_namespace' | 'path' | 'path_with_namespace'>. The hand-written Todo interface (line 32) declares web_url: string and the as unknown as Todo[] cast at line 141 discards gitbeaker's typing, so the compiler never objects.
Since buildMetadata falls back with vars[key] ?? match, undefined yields the literal token: every agent turn receives URL: %repo_url%.
Verified by editing makeTodo() to the real GitLab shape and re-running the suite:
AssertionError: expected 'REPO: owner/repo | URL: %repo_url% | …'
to be 'REPO: owner/repo | URL: https://gitlab.com/owner/repo | …'
The suite is green today only because the fixture invents a field the API never sends (see the comment on GitlabAdapter.test.ts).
| repo_url: todo.project.web_url, | |
| repo_url: ((this.config as GitlabConfig).baseUrl || 'https://gitlab.com').replace(/\/+$/, '') + '/' + chatId, |
— claude-opus-5[1m] via Qwen Code /qreview
| continue; | ||
| } | ||
|
|
||
| const template = templates[todo.action_name]; |
There was a problem hiding this comment.
[Critical] The canonical way to invoke a bot — starting a comment with @bot … — produces action_name: 'directly_addressed', not 'mentioned', so it hits the if (!template) skip below and is silently dropped forever.
GitLab's TodoService#create_mention_todos creates Todo::DIRECTLY_ADDRESSED for directly-addressed users first, then creates Todo::MENTIONED only for mentioned_users - directly_addressed_users — the two are mutually exclusive. And Banzai::ReferenceParser::DirectlyAddressedUserParser is defined as reference_options = { location: :beginning }, i.e. "directly addressed" means the mention is at the beginning of a line.
The only example configuration anywhere in this PR (GitlabAdapter.test.ts:32-35) sets { mentioned: … } alone, and the fixture is self-contradictory proof: makeTodo() pairs action_name: 'mentioned' with body: '@test-bot please fix this' — a leading mention, which real GitLab classifies as directly_addressed.
Result: @qwen-bot please fix this as the first line of a comment is never processed, is never marked done, and logs nothing.
| const template = templates[todo.action_name]; | |
| const template = | |
| templates[todo.action_name] ?? | |
| (todo.action_name === 'directly_addressed' | |
| ? templates['mentioned'] | |
| : undefined); |
— claude-opus-5[1m] via Qwen Code /qreview
| } | ||
|
|
||
| if (newNotes.length === 0) { | ||
| const body = todo.body || ''; |
There was a problem hiding this comment.
[Critical] todo.body is not the issue/MR description — this path delivers the title instead, and the request text is lost.
GitLab's Todo#body is:
def body
if note.present? then note.note
elsif member_access_requested? then target.full_path
elsif transfer_failed? then …
else target.title
end
endThe newNotes.length === 0 branch is exactly the no-note case — someone opened an issue/MR whose description mentions the bot. So text becomes the title.
It then fails closed: titles do not contain @bot, so buildEnvelope sets isMentioned: false, and GroupGate.check drops the envelope silently (requireMention = groupConfig.requireMention ?? true, GroupGate.ts:49-52). Control returns normally, so TodoLists.done() runs at line 178 and both cursors advance — the trigger is permanently consumed with no reply and no error comment.
This is the common path, not an edge case: a brand-new issue has no comments. The sibling GitHub adapter avoids it by fetching the real body (GithubAdapter.ts:276-296, issue.body).
if (newNotes.length === 0) {
const target =
targetType === 'mr'
? await this.api.MergeRequests.show(chatId, todo.target.iid)
: await this.api.Issues.show(chatId, todo.target.iid);
const body = (target.description as string) || '';
const authorUsername =
(target.author as { username?: string })?.username ?? todo.author.username;
// …existing dispatch, attributed to the target's author…
}— claude-opus-5[1m] via Qwen Code /qreview
| continue; | ||
| } | ||
|
|
||
| const chatId = todo.project.path_with_namespace; |
There was a problem hiding this comment.
[Critical] todo.project is dereferenced with no guard, outside the per-todo try (which opens at line 168), so a TypeError here escapes pollOnce entirely and wedges the channel permanently.
GitLab exposes project conditionally — expose :project, using: ::API::Entities::ProjectIdentity, if: ->(todo, _) { todo.project_id } — and omits the key for group-level todos. Because WorkItem is an STI subclass of Issue, a group-level work-item todo stores target_type as the base class name Issue, so it passes the filter directly above with no project key.
Reproduced: one such todo throws TypeError: Cannot read properties of undefined (reading 'path_with_namespace'). The throw bypasses the per-todo handler, so nothing is dispatched, lastProcessedAt never advances, TodoLists.done is never called — the todo is still pending and still matches updated_at > windowSince next poll. runLoop just backs off (capped at 30s) and re-throws forever. Every healthy todo queued behind it is lost until an operator manually clears the GitLab to-do. One malformed todo = total channel outage with no self-recovery, and the operator sees only the generic poll error with no todo id.
Fold it into the existing guard so it takes the skip path instead:
if (
!todo.target ||
!todo.project?.path_with_namespace ||
(todo.target_type !== 'Issue' && todo.target_type !== 'MergeRequest')
) {— claude-opus-5[1m] via Qwen Code /qreview
| chatId, | ||
| targetType, | ||
| threadId, | ||
| this.cursor.repo[chatId]?.last_read ?? windowSince, |
There was a problem hiding this comment.
[Critical] The note-window lower bound is stored per project but consumed per thread, which silently and permanently drops inbound comments.
chatId is todo.project.path_with_namespace, but repoSince is used at line 217 as n.created_at > repoSince while enumerating the notes of one issue/MR. Lines 180-183 then advance that project-wide watermark to todo.updated_at regardless of which thread the todo belonged to — so a later todo for a different thread in the same project inherits a lower bound borrowed from an unrelated thread.
Reproduced twice, independently: project owner/repo, todo#11 (issue 1, updated_at 10:00:10) and todo#12 (issue 2, updated_at 10:00:20), where issue 2 also has a plain context comment at 10:00:00. Dispatched ids were ['11','21'] — note 20 was never delivered, because todo#11 had already pushed the watermark to 10:00:10. A second repro where the second thread's only new note fell below the watermark produced ['11','todo-body-12']: the real note never reaches the bridge and the newNotes.length === 0 fallback re-delivers it under a synthetic id with the wrong sender.
Those dropped notes are exactly what gets recorded as pending group history and prepended as context, so the agent answers with missing context. Nothing is logged; TodoLists.done() still runs; the notes can never be re-fetched. The GitHub sibling deliberately passes the same per-poll windowSince to every thread (GithubAdapter.ts:181).
Key it per thread instead — ${chatId}|${threadId} — or just use windowSince for every thread.
— claude-opus-5[1m] via Qwen Code /qreview
| state: 'pending', | ||
| created_at: '2026-07-02T09:00:00.000Z', | ||
| updated_at: '2026-07-02T10:00:00.000Z', | ||
| project: { |
There was a problem hiding this comment.
[Suggestion] This fixture invents project.web_url, a field GitLab's /todos endpoint never returns — which is why the %repo_url% bug above ships green. More broadly, the suite is largely mutation-insensitive: the following edits to GitlabAdapter.ts each leave 32/32 passing.
| Mutation | Survives |
|---|---|
text: stripBotMention(...) → text: rawBody |
yes |
const isMentioned = … → true |
yes |
drop .toLowerCase() from senderId |
yes |
isGroup: true → false |
yes |
delete this.gate.replaceAllowedUsers(allowed) |
yes |
delete .filter((t) => t.updated_at > windowSince) |
yes |
delete .sort(...) on todos |
yes |
| delete cursor advance in both skip branches | yes |
invert todo.updated_at > prev → < prev |
yes |
delete Array.isArray(base.repo) / the null check |
yes |
n.created_at > repoSince → >= |
yes |
sendMessage body → return; |
yes |
delete the !threadId super-delegation |
yes |
drop {sort,orderBy} from IssueNotes.all |
yes |
if (body && todo.author.username !== this.botUsername) → if (body) |
yes |
Positive controls (host trailing-slash strip, state:'pending', bot-note filter, per-repo cursor write, base.repo = {}) were all killed, so the harness does detect real failures.
Two structural causes: fixtures are too uniform (every author is lowercase, every body mentions the bot, every todo is newer than the cursor), and TestableGitlabChannel overrides handleInbound wholesale, so no test ever reaches the real ChannelBase gate/session pipeline — the allowlist wiring at GitlabAdapter.ts:100 is the single most safety-relevant line in connect() and deleting it is invisible to CI.
Highest-value additions: fix makeTodo() to mirror the documented /todos payload exactly; assert env.text with toBe (exact, post-strip) rather than toContain; add a non-mentioning note asserting isMentioned === false; add a mixed-case author asserting senderId === 'alice'; assert gate.isAllowed(...) directly.
— claude-opus-5[1m] via Qwen Code /qreview
|
|
||
| protected async pollOnce(): Promise<void> { | ||
| const templates = (this.config as GitlabConfig).action_prompt_template; | ||
| if (!templates || Object.keys(templates).length === 0) return; |
There was a problem hiding this comment.
[Suggestion] action_prompt_template is a hard prerequisite for this adapter doing anything, but nothing tells the operator that.
It is not in requiredConfigFields (index.ts:9 lists only token), it is not validated in connect(), it emits no log, and it is documented nowhere — this PR adds no docs at all. Verified: with the key absent, five consecutive pollOnce() calls produced 0 TodoLists.all calls and 0 bytes of stderr, while connect() succeeded and runLoop kept resetting consecutiveErrors. A typo (actionPromptTemplate) yields the identical silent death, and the two tests at lines 210/222 lock the silence in.
Second-order effect: the early return happens before any cursor advance, so lastProcessedAt stays pinned at the first-start timestamp for the whole outage. The moment the operator adds the key, updated_at > windowSince matches the entire accumulated backlog and the bot replies to every mention from the outage window at once.
Also, the key itself is the only snake_case config field in the codebase — every field in ChannelConfig and every adapter extension (baseUrl, senderPolicy, allowedUsers, pollInterval, wsUrl, atSender) is camelCase, and GitlabCursor mixes the two styles (lastProcessedAt next to repo[].last_read). It is a user-facing settings.json key, so renaming is free now and needs a compat shim later.
Suggest: add it to requiredConfigFields, or warn once from connect() naming the key and its accepted action_name values; rename to actionPromptTemplate.
— claude-opus-5[1m] via Qwen Code /qreview
|
|
||
| const prev = this.cursor.repo[chatId]?.last_read; | ||
| if (!prev || todo.updated_at > prev) { | ||
| this.cursor.repo[chatId] = { last_read: todo.updated_at }; |
There was a problem hiding this comment.
[Suggestion] cursor.repo gains a permanent entry per project and is never pruned or bounded, while saveCursor() re-serializes the whole map synchronously once per todo.
validateCursor (lines 66-73) only type-checks the map — never its size or the shape of its values — and PollingChannelBase.saveCursor() is a blocking mkdirSync + writeFileSync + renameSync of the entire JSON blob. It is called on all three branches of this loop (lines 153, 160, 185), and runLoop calls it again right after pollOnce() returns, so a cycle with T todos performs T+1 write-rename sequences where the last is always redundant.
The keys are remotely influenced: TodoLists.all() returns todos from every project on the instance, and this write happens even when every note was rejected by the gate. Verified: 50 todos from 50 distinct projects produced 50 permanent entries; 300 projects produced ~17 KB. Cost per todo is O(projects ever seen), so write volume grows quadratically over the channel's lifetime.
The GitHub sibling bounds its analogous state explicitly — MAX_DISPATCHED_BODIES = 500 with list.slice(-MAX_DISPATCHED_BODIES) (GithubAdapter.ts:29,331-335), commented "Bounded to the most recent entries so the cursor stays small."
Suggest: keep the N most recently touched keys on write, and drop saveCursor() from the two skip branches — the trailing runLoop save already covers a monotonic timestamp.
— claude-opus-5[1m] via Qwen Code /qreview
| } | ||
| } | ||
|
|
||
| private async postErrorComment( |
There was a problem hiding this comment.
[Suggestion] No GitLab call in this adapter has retry or rate-limit handling, and the outbound reply path is where that hurts most.
The GitHub sibling routes every request through githubApi() (GithubAdapter.ts:360-405), which retries 3× and honors retry-after, x-ratelimit-remaining and x-ratelimit-reset. gitbeaker does not fill the gap: @gitbeaker/requester-utils@42.5.0 has no 429/Retry-After handling at all, and getMatchingRateLimiter falls back to generateRateLimiterFn(3e3, 60) — a 3,000/min client-side queue that sits above gitlab.com's authenticated ceiling and so never engages.
Concretely, createNote is called once and lets the rejection propagate out of sendThreadMessage into ChannelBase.sendResponseMessage, where there is no requeue. A single transient 429 or 5xx while delivering the agent's answer discards that answer permanently — the todo is already marked done and the cursor already advanced, so nothing re-drives it. The user sees the bot pick up the request and then go silent.
On the inbound side a 429 aborts the cycle and the base loop retries after ≤30s by re-running the entire unbounded fetch from scratch, re-paginating exactly what triggered the throttle — a self-sustaining amplifier rather than a drain.
Suggest a gitlabApi<T>(fn, label, retries = 3) helper mirroring githubApi() (reading RateLimit-Reset / Retry-After off GitbeakerRequestError.cause.response.headers, using the inherited abortableSleep) and route all six call sites through it. Please also include the HTTP status in the failure log — today error processing todo N, stopping cannot distinguish a rate limit from an auth failure from a network blip.
— claude-opus-5[1m] via Qwen Code /qreview
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Critical 1: proxyAgent silently ignored by @gitbeaker/rest — GitlabAdapter.ts:91 — already reported on PR (overlap at line 91, 2 existing comments)
[Critical] Critical 2: First-contact error path swallows handleInbound — GitlabAdapter.ts:271 — already reported on PR (overlap at line 271, existing comment 3660790896)
[Critical] Critical 3: channel-registry test not updated — channel-registry.ts:27 — already reported on PR (overlap at line 27, existing comment 3660790904)
[Critical] Critical 4: pollOnce outer catch creates permanent livelock — GitlabAdapter.ts:191 — already reported on PR (overlap at line 191, 2 existing comments)
— qwen3.7-max via Qwen Code /review
| "compilerOptions": { | ||
| "outDir": "dist", | ||
| "rootDir": "src" | ||
| }, |
There was a problem hiding this comment.
[Suggestion] declarationMap: true is missing from compilerOptions — every other production channel package (github, telegram, feishu, dingtalk, wecom, qqbot, weixin) includes it.
— Failure scenario: Without declarationMap, the TypeScript compiler does not emit .d.ts.map files. IDE "Go to Definition" on a symbol from @qwen-code/channel-gitlab lands on the .d.ts declaration file instead of the source .ts file — inconsistent with every other production channel.
"compilerOptions": {
"declarationMap": true,
"outDir": "dist",
"rootDir": "src"
},中文说明
[Suggestion] compilerOptions 中缺少 declarationMap: true——所有其他生产 channel 包（github、telegram、feishu、dingtalk、wecom、qqbot、weixin）都包含此配置。
— 失败场景：没有 declarationMap，TypeScript 编译器不会生成 .d.ts.map 文件。在 IDE 中对 @qwen-code/channel-gitlab 的符号使用"转到定义"时，会跳转到 .d.ts 声明文件而非源 .ts 文件——与其他所有生产 channel 不一致。
— qwen3.7-max via Qwen Code /review
- Remove non-functional proxyAgent (gitbeaker doesn't support it) - Construct repo_url from host + path (API doesn't return web_url) - Handle directly_addressed action (falls back to mentioned template) - First-contact fetches target description instead of using todo.body - Move todo.project dereference inside try block - Filter confidential notes - Update channel-registry.test.ts for gitlab entry
c9e98e8 to
b2d410e
Compare
- Warn on connect if action_prompt_template is not configured - Guard todo.target.iid before use - Skip paths now mark_done (best-effort) to clean GitLab UI - Remove postErrorComment (avoids duplicate comments on retry) - Fetch only first page of notes (desc, maxPages:1, perPage:100) instead of paginating entire note history - Extract fetchRecentNotes for single-page windowed enumeration
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Unresolved, please confirm: [Critical] GitlabAdapter.ts:? (comment 3660771292) — postErrorComment + throw err retry loop: referenced code pattern (postErrorComment) no longer exists at this commit; cannot determine if the defect still applies
中文说明
未决，请确认：[Critical] GitlabAdapter.ts:? (comment 3660771292) — postErrorComment + throw err retry loop: referenced code pattern (postErrorComment) no longer exists at this commit; cannot determine if the defect still applies
— qwen3.7-max via Qwen Code /review
| const issue = await api.Issues.show(chatId, { | ||
| issueIId: todo.target.iid, | ||
| }); |
There was a problem hiding this comment.
[Critical] Issues.show called with wrong arguments — gitbeaker's signature is show(issueId: number, { projectId, ...options }), but this passes chatId (a string like owner/repo) as the first positional arg (becoming issueId) and wraps the IID in { issueIId: ... } which is not a recognized option key (only projectId is destructured). The generated URL is issues/owner/repo — a guaranteed 404. The as unknown as cast on line 313 suppresses the TypeScript error that would otherwise catch this.
— Failure scenario: first-contact on an issue with no notes → Issues.show 404s → catch sets description = '' → envelope falls back to todo.target.title → if title doesn't contain @bot, GroupGate drops it → user's message permanently lost with no error comment.
| const issue = await api.Issues.show(chatId, { | |
| issueIId: todo.target.iid, | |
| }); | |
| const issue = await this.api.Issues.show(todo.target.iid, { | |
| projectId: chatId, | |
| }); |
中文说明
Issues.show 参数顺序错误——gitbeaker 签名是 show(issueId: number, { projectId })，但这里把 chatId（字符串 owner/repo）作为第一个位置参数传入（变成 issueId），IID 被包装在 { issueIId: ... } 中，这不是可识别的选项键。生成的 URL 是 issues/owner/repo，必定 404。
— qwen3.7-max via Qwen Code /review
| const mr = await api.MergeRequests.show(chatId, { | ||
| mergeRequestIId: todo.target.iid, | ||
| }); |
There was a problem hiding this comment.
[Critical] MergeRequests.show called with wrong arguments — gitbeaker's signature is show(projectId, mergerequestIId: number, options?), where mergerequestIId is the second positional argument (a number), not an options key. This passes { mergeRequestIId: todo.target.iid } as the second arg (an object), which gets coerced to [object Object] in the URL template: projects/owner/repo/merge_requests/[object Object] — guaranteed 404.
— Failure scenario: first-contact on an MR with no notes → 404 → catch sets description = '' → same silent message loss as the Issue path.
| const mr = await api.MergeRequests.show(chatId, { | |
| mergeRequestIId: todo.target.iid, | |
| }); | |
| const mr = await this.api.MergeRequests.show(chatId, todo.target.iid); |
中文说明
MergeRequests.show 参数错误——gitbeaker 签名是 show(projectId, mergerequestIId: number, options?)，mergerequestIId 是第二个位置参数（数字），不是选项键。这里把 { mergeRequestIId: ... } 作为第二个参数（对象），在 URL 模板中被转为 [object Object]，必定 404。
— qwen3.7-max via Qwen Code /review
| const page = (targetType === 'mr' | ||
| ? await this.api.MergeRequestNotes.all(chatId, iid, { | ||
| sort: 'desc', | ||
| orderBy: 'created_at', | ||
| maxPages: 1, | ||
| perPage: 100, | ||
| }) |
There was a problem hiding this comment.
[Critical] maxPages: 1, perPage: 100 caps note fetching at the 100 most recent notes — any notes beyond that are silently dropped, even when they fall inside the [since, until] time window. Notes are sorted desc by created_at, so the 100 newest are returned and older ones within the polling window are never fetched.
— Failure scenario: a busy MR accumulates 150 non-system notes between two poll cycles. Only the 100 most recent are fetched. The time filter n.created_at > since is applied to that truncated set. Notes 101–150 (the older 50, all within the window) are never dispatched. The per-repo cursor advances past until, so on the next poll those 50 notes are permanently missed. The GitHub sibling adapter uses octokit.paginate and fetches all pages.
中文说明
maxPages: 1, perPage: 100 将笔记获取限制为最近 100 条——超出的笔记即使落在 [since, until] 时间窗口内也会被静默丢弃。繁忙 MR 在两次轮询间积累超过 100 条笔记时，较早的笔记永久丢失。GitHub 适配器使用 octokit.paginate 获取所有页。
— qwen3.7-max via Qwen Code /review
| const MENTION_LOOKBEHIND = '(?<=\\s|^|[([{<:;"\'])'; | ||
| const MENTION_LOOKAHEAD = '(?=[^a-zA-Z0-9_/-]|$)'; | ||
|
|
||
| export function testBotMention(text: string, username: string): boolean { |
There was a problem hiding this comment.
[Suggestion] mention.ts is a byte-for-byte copy of packages/channels/github/src/mention.ts but has no dedicated test file. The GitHub channel has mention.test.ts with 16 test cases covering case-insensitive matching, escapeRegex with special characters, partial-username non-match, and stripBotMention preservation. A future regex regression here would go undetected.
— Concrete cost: the two copies can diverge silently; the GitHub suite catches regressions in one but not the other.
中文说明
mention.ts 是 GitHub channel 同名文件的逐字节拷贝，但没有专用测试文件。GitHub channel 有 16 个测试用例覆盖。未来此处的正则回归将无法被检测到。
— qwen3.7-max via Qwen Code /review
| if (actionName === 'directly_addressed') return templates['mentioned']; | ||
| return undefined; |
There was a problem hiding this comment.
[Suggestion] directly_addressed → mentioned template alias has no test coverage. The test suite only exercises action_name: 'mentioned' (direct lookup) and action_name: 'assigned' (returns undefined). This is the only non-trivial aliasing logic and is never reached by any test.
— Failure scenario: if a GitLab instance emits directly_addressed (valid GitLab todo action for @bot at the beginning of a line), and the alias string is typoed or refactored, resolveTemplate returns undefined and the bot silently ignores the most common mention pattern.
中文说明
directly_addressed → mentioned 模板别名没有测试覆盖。测试套件只测了 mentioned（直接查找）和 assigned（返回 undefined）。这是唯一的非平凡别名逻辑，从未被任何测试触达。
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] GitlabAdapter.ts:188-193 — break-on-error creates permanent polling queue blockage (overlaps with existing comments 3660771273, 3660790906 at line 193). When processTodo throws, the catch block breaks the for-loop without marking the todo done or advancing cursor.lastProcessedAt. The same failing todo is re-fetched on every poll cycle, permanently blocking all subsequent todos.
— qwen3.7-max via Qwen Code /review
| const mr = await api.MergeRequests.show(chatId, { | ||
| mergeRequestIId: todo.target.iid, | ||
| }); |
There was a problem hiding this comment.
[Critical] api.MergeRequests.show and api.Issues.show are called with wrong argument shapes for gitbeaker v42 — both calls fail at runtime, so tryFirstContact always falls back to the issue/MR title and never includes the description.
MergeRequests.showexpects(projectId, mergerequestIId: number)— the second argument must be a number, not{ mergeRequestIId: ... }.Issues.showexpects(issueId: number, { projectId? })— the first argument must be a numeric global issue ID, not a project path string.
— Failure scenario: A user opens an issue with a detailed description mentioning the bot. tryFirstContact attempts to fetch the description via these calls, both 404, the catch sets description = '', and the bot dispatches only the issue title as the inbound message. The agent receives an incomplete prompt without the issue body.
The test mocks accept any arguments, so the suite passes despite the real API being wrong.
| const mr = await api.MergeRequests.show(chatId, { | |
| mergeRequestIId: todo.target.iid, | |
| }); | |
| if (targetType === 'mr') { | |
| const mr = await api.MergeRequests.show(chatId, todo.target.iid); | |
| description = mr.description || ''; | |
| } else { | |
| const issue = await api.Issues.show(todo.target.iid, { projectId: chatId }); | |
| description = issue.description || ''; | |
| } |
中文说明
[Critical] api.MergeRequests.show 和 api.Issues.show 的调用参数与 gitbeaker v42 的签名不匹配——两个调用在运行时都会失败，tryFirstContact 始终回退到 issue/MR 标题，永远无法获取描述内容。
MergeRequests.show期望(projectId, mergerequestIId: number)— 第二个参数必须是数字，不是对象。Issues.show期望(issueId: number, { projectId? })— 第一个参数必须是全局数字 issue ID，不是项目路径字符串。
失败场景：用户在 issue 中详细描述需求并 @bot。tryFirstContact 尝试获取描述但两个调用均 404，catch 将 description 设为空字符串，bot 仅派发标题作为入站消息。agent 收到不完整的提示，缺少 issue 正文。
— qwen3.7-max via Qwen Code /review
| "@gitbeaker/rest": "^42.5.0", | ||
| "@qwen-code/channel-base": "file:../base", | ||
| "undici": "^7.28.0" |
There was a problem hiding this comment.
[Suggestion] Lockfile declares undici as a dependency of @qwen-code/channel-gitlab, but packages/channels/gitlab/package.json does not list undici and no source file imports it. This is a stale entry from an earlier iteration.
— Concrete cost: Running npm install will reconcile and remove this phantom entry, producing an unexpected lockfile diff that CI lockfile-consistency checks may reject.
| "@gitbeaker/rest": "^42.5.0", | |
| "@qwen-code/channel-base": "file:../base", | |
| "undici": "^7.28.0" | |
| "@gitbeaker/rest": "^42.5.0", | |
| "@qwen-code/channel-base": "file:../base" |
中文说明
[Suggestion] Lockfile 中将 undici 声明为 @qwen-code/channel-gitlab 的依赖，但 packages/channels/gitlab/package.json 未列出 undici，源代码中也没有任何导入。这是早期迭代遗留的过期条目。运行 npm install 会清理此条目并产生意外的 lockfile diff。
— qwen3.7-max via Qwen Code /review
| expect(mockApi.IssueNotes.all).not.toHaveBeenCalled(); | ||
| expect(channel.inboundEnvelopes).toHaveLength(0); |
There was a problem hiding this comment.
[Suggestion] Skip-path tests verify negative outcomes (no dispatch) but don't assert the positive side-effects of skipTodo: calling TodoLists.done and advancing cursor.lastProcessedAt. If a refactor removed either, skipped todos would remain pending and be re-fetched every poll cycle — this regression passes both tests undetected.
| expect(mockApi.IssueNotes.all).not.toHaveBeenCalled(); | |
| expect(channel.inboundEnvelopes).toHaveLength(0); | |
| expect(mockApi.IssueNotes.all).not.toHaveBeenCalled(); | |
| expect(channel.inboundEnvelopes).toHaveLength(0); | |
| expect(mockApi.TodoLists.done).toHaveBeenCalledWith({ todoId: todo.id }); | |
| expect(channel.cursor.lastProcessedAt).toBe(todo.updated_at); |
中文说明
[Suggestion] Skip 路径测试仅验证了消极结果（未派发），但未断言 skipTodo 的正面副作用：调用 TodoLists.done 和推进 cursor.lastProcessedAt。如果重构移除了其中任何一个，被跳过的 todo 将永远保持 pending 状态并在每次轮询中重新获取——此回归不会被现有测试捕获。
— qwen3.7-max via Qwen Code /review
| export function testBotMention(text: string, username: string): boolean { | ||
| const re = new RegExp( |
There was a problem hiding this comment.
[Suggestion] mention.ts is a byte-for-byte copy of packages/channels/github/src/mention.ts, but the GitHub channel has mention.test.ts with 16 test cases and this package has none. Partial-username matches, email false-positives, and embedded mentions are unverified here.
— Concrete cost: Any future edit to this file (e.g., adapting for GitLab group-path usernames) has no safety net.
中文说明
[Suggestion] mention.ts 是 packages/channels/github/src/mention.ts 的逐字节复制，但 GitHub channel 有 16 个测试用例的 mention.test.ts，而本包没有。部分用户名匹配、邮箱误判和嵌入 mention 等边界情况未经验证。
— qwen3.7-max via Qwen Code /review
| !n.system && | ||
| !n.confidential && | ||
| n.author.username !== this.botUsername && |
There was a problem hiding this comment.
[Suggestion] fetchRecentNotes filters out confidential notes (!n.confidential), but no test creates a confidential note to verify this. Tests exist for system: true and bot-author filtering, but the confidential guard is unverified.
— Concrete cost: If !n.confidential is accidentally removed during a refactor, confidential notes (restricted-visibility comments with sensitive content) would be processed and dispatched to the agent.
中文说明
[Suggestion] fetchRecentNotes 过滤了机密笔记（!n.confidential），但没有测试创建机密笔记来验证此行为。system: true 和 bot 作者的过滤都有测试，但机密笔记的过滤未经测试。
— qwen3.7-max via Qwen Code /review
| if (templates[actionName]) return templates[actionName]; | ||
| if (actionName === 'directly_addressed') return templates['mentioned']; |
There was a problem hiding this comment.
[Suggestion] resolveTemplate falls back from directly_addressed to mentioned, but no test exercises a todo with action_name: 'directly_addressed'. In real GitLab, @bot … at the start of a line produces directly_addressed, not mentioned — so this fallback is the primary path for the most common bot invocation.
— Concrete cost: If the fallback is renamed or removed, the most common way to invoke the bot silently produces no envelope.
中文说明
[Suggestion] resolveTemplate 将 directly_addressed 回退到 mentioned，但没有测试使用 action_name: 'directly_addressed' 的 todo。在真实 GitLab 中，行首 @bot … 会产生 directly_addressed 而非 mentioned——因此此回退是最常见的 bot 调用方式的主路径。
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: verification — the restart at a5db236 did not re-run the verifier with a CLI-built prompt. Not reviewed: reverse audit — the restart at a5db236 did not re-run reverse auditors with CLI-built prompts. Not reviewed: verification and reverse audit — neither the verifier nor the reverse auditor was launched with a prompt this skill builds — the posted findings were ruled on, and the misses the rest of the review left were hunted, if at all, without the briefs this skill certifies against.
中文说明
已审查。 建议见行内评论。 未审查：verification — the restart at a5db236 did not re-run the verifier with a CLI-built prompt。 未审查：reverse audit — the restart at a5db236 did not re-run reverse auditors with CLI-built prompts。 未审查：验证与反向审计——验证 agent 与反向审计 agent 都没有用本 skill 构建的 prompt 启动——发布的发现即便被裁定过、评审其余部分遗漏的问题即便被搜寻过，也都缺失了本 skill 用以认证的 brief。
— qwen3.8-max-preview via Qwen Code /review
|
|
||
| beforeEach(() => { | ||
| savedQwenHome = process.env.QWEN_HOME; | ||
| process.env.QWEN_HOME = mkdtempSync(join(tmpdir(), 'qwen-gl-test-')); |
There was a problem hiding this comment.
[Suggestion] mkdtempSync 创建的临时目录从未清理——afterEach 恢复了环境变量但没有 rmSync 删除目录。每次测试运行（29 个 it 块，每个触发 beforeEach）都会在系统临时路径下创建新的 qwen-gl-test-* 目录。在持久化 CI runner 或频繁本地开发时，这些孤立目录会无限累积。
建议在 afterEach 中添加清理：
| process.env.QWEN_HOME = mkdtempSync(join(tmpdir(), 'qwen-gl-test-')); | |
| rmSync(testHome, { recursive: true, force: true }); |
中文说明
mkdtempSync 创建的临时目录从未清理——afterEach 恢复了环境变量但没有删除目录。建议在 afterEach 中添加 rmSync(testHome, { recursive: true, force: true })。
— qwen3.8-max-preview via Qwen Code /review
| expect(env.threadId).toBe('issue:42'); | ||
| expect(env.senderId).toBe('alice'); | ||
| expect(env.isMentioned).toBe(true); | ||
| expect(env.text).toContain('please fix this'); |
There was a problem hiding this comment.
[Suggestion] 此断言对 stripBotMention 是空泛的——无论 mention 是否被实际剥离都会通过。note body 是 '@test-bot please fix this'，如果 stripBotMention 突变为 no-op（原样返回输入），env.text 仍然是 '@test-bot please fix this'，toContain('please fix this') 依然为真。@test-bot 前缀会包含在发送给 agent 的文本中，污染 prompt。
| expect(env.text).toContain('please fix this'); | |
| expect(env.text).toBe(' please fix this'); |
中文说明
此断言对 stripBotMention 是空泛的——无论 mention 是否被实际剥离都会通过。建议改为精确匹配 expect(env.text).toBe(' please fix this') 或添加 expect(env.text).not.toContain('@test-bot')。
— qwen3.8-max-preview via Qwen Code /review
…ption mention support - Remove notes API fetching; dispatch todo.body directly - Detect description mentions via target_url anchor (#note_ absence) - Always fetch target description for %description% metadata - Remove per-repo cursor; dedup via cursor + mark_done only - Cursor advances regardless of success/failure (no retry) - Use zod for cursor validation - Rename template vars to GitLab terminology: %project% %project_url% %target_type% %iid% %title% %description% %todo_id% - Support %% escape for literal percent
- New user guide: docs/users/features/channels/gitlab.md - Update _meta.ts navigation - Update developer adapter matrix and SDK list
|
Qwen precheck requires maintainer approval before automated triage/review. Head SHA: Reason:
A maintainer with write access can inspect the PR and manually request a run with |
|
Reply to inline comments (02:31 UTC batch):
Fixed in
Not an issue — gitbeaker's
Not applicable — no other channel package (github, telegram, feishu, dingtalk, wecom, qqbot, weixin) includes Complete E2E testing against a live GitLab instance will follow up this afternoon. 中文翻译
已在
不是问题——gitbeaker 的
不适用——其他 channel 包（github、telegram、feishu、dingtalk、wecom、qqbot、weixin）的 tsconfig 也都没有 完整 E2E 测试（对接真实 GitLab 实例）将于今天下午跟进。 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Unresolved, please confirm: [Critical] postErrorComment + throw loop (comment 3660771292) — postErrorComment no longer exists; new catch marks todo done (different mechanism from reported defect); error handling still loses user messages on failure
[Critical] todo.project null guard (comment 3661805873) — still stands: todo.project.path_with_namespace accessed at line 138 outside per-todo try/catch; null project causes TypeError that escapes inner catch, blocks all subsequent todos
[Critical] Internal notes leak (comment 3661805891) — still stands: no confidential/internal filtering on inbound notes; createNote replies publicly regardless of trigger note visibility
中文说明
未决，请确认：[Critical] postErrorComment + throw loop (comment 3660771292) — postErrorComment no longer exists; new catch marks todo done (different mechanism from reported defect); error handling still loses user messages on failure
[Critical] todo.project null guard (comment 3661805873) — still stands: todo.project.path_with_namespace accessed at line 138 outside per-todo try/catch; null project causes TypeError that escapes inner catch, blocks all subsequent todos
[Critical] Internal notes leak (comment 3661805891) — still stands: no confidential/internal filtering on inbound notes; createNote replies publicly regardless of trigger note visibility
— qwen3.7-max via Qwen Code /review
| }; | ||
| return vars[key] ?? match; | ||
| }) | ||
| .replace(/%%/g, '%'); |
There was a problem hiding this comment.
[Suggestion] Template %% escape is processed after variable substitution, corrupting user-authored content
The .replace(/%%/g, '%') runs on the entire output string after %var% substitution. Any %% in user-controlled GitLab content (issue descriptions, titles) that has been substituted into a variable is silently converted to %.
— Failure scenario: A GitLab issue description containing 100%% complete is substituted via %description%. The second replace converts it to 100% complete, silently altering user-authored content.
| .replace(/%%/g, '%'); | |
| return template.replace(/%%|%(\w+)%/g, (match, key: string) => { | |
| if (match === '%%') return '%'; | |
| const vars: Record<string, string> = { | |
| project: chatId, | |
| project_url: `${this.apiHost}/${chatId}`, | |
| repo_url: `${this.apiHost}/${chatId}`, | |
| author, | |
| target_type: todo.target_type, | |
| thread_type: todo.target_type === 'MergeRequest' ? 'mr' : 'issue', | |
| thread_id: String(todo.target.iid), | |
| thread_title: todo.target.title, | |
| description, | |
| comment_id: commentId, | |
| }; | |
| return vars[key] ?? match; | |
| }); |
中文说明
模板 %% 转义在变量替换之后处理，会破坏用户编写的内容。建议在单次 replace 中同时处理 %% 转义和变量替换。
— qwen3.7-max via Qwen Code /review
| if (!threadId) { | ||
| return super.sendThreadMessage(chatId, threadId, text); | ||
| } |
There was a problem hiding this comment.
[Suggestion] sendThreadMessage fallback throws via confusing indirect path
When threadId is undefined, the fallback to super.sendThreadMessage eventually calls this.sendMessage(chatId, text), which throws "sendMessage requires a threadId; use sendThreadMessage" — confusing because the caller DID use sendThreadMessage.
— Failure scenario: Debugging this error requires tracing through two layers of indirection (sendThreadMessage → super.sendThreadMessage → sendMessage → throw), when a direct throw at line 108 would produce a clearer error.
| if (!threadId) { | |
| return super.sendThreadMessage(chatId, threadId, text); | |
| } | |
| if (!threadId) { | |
| throw new Error(`[Channel:${this.name}] sendThreadMessage requires a threadId`); | |
| } |
中文说明
sendThreadMessage 的 fallback 通过间接路径抛出令人困惑的错误。建议直接抛出清晰的错误消息。
— qwen3.7-max via Qwen Code /review
| const description = await this.fetchDescription( | ||
| chatId, | ||
| targetType, | ||
| todo.target.iid, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] N+1 API calls in fetchDescription with no deduplication
Every description-mention todo in a poll batch triggers a separate Issues.show() or MergeRequests.show() API call, even when multiple todos reference the same issue or MR.
— Failure scenario: A single poll cycle returns 10 todos all referencing issue #42. The adapter fires 10 identical API calls, wasting 9 round-trips and 9× the GitLab rate-limit budget.
Suggested fix: Cache fetched descriptions in a Map<string, string> keyed by ${chatId}:${targetType}:${iid} within pollOnce().
中文说明
fetchDescription 没有去重，同一轮询批次中对同一 issue/MR 的多次引用会触发重复 API 调用。建议在 pollOnce 内缓存。
— qwen3.7-max via Qwen Code /review
| this.cursor.lastProcessedAt = todo.updated_at; | ||
| this.saveCursor(); |
There was a problem hiding this comment.
[Suggestion] Redundant synchronous disk I/O — saveCursor() called N times per poll
saveCursor() performs three synchronous filesystem operations (mkdirSync, writeFileSync, renameSync) on every iteration of the todo loop. The base class PollingChannelBase.runLoop() already calls saveCursor() once after pollOnce() returns.
— Failure scenario: A poll returns 50 pending todos → 150 synchronous disk operations in the hot path, blocking the event loop.
| this.cursor.lastProcessedAt = todo.updated_at; | |
| this.saveCursor(); | |
| this.cursor.lastProcessedAt = todo.updated_at; |
中文说明
saveCursor() 在每个 todo 循环迭代中被调用，产生冗余的同步磁盘 I/O。基类已在 pollOnce() 返回后调用 saveCursor()。
— qwen3.7-max via Qwen Code /review
| await expect( | ||
| channel.testSendThreadMessage('owner/repo', 'invalid', 'reply'), | ||
| ).rejects.toThrow('invalid threadId format'); |
There was a problem hiding this comment.
[Suggestion] No test covers threadId === undefined path in sendThreadMessage
The test covers malformed threadId ('invalid') but not undefined. The undefined path takes a different code route (through super.sendThreadMessage → this.sendMessage → throws "sendMessage requires a threadId") and this behavior is unverified.
中文说明
测试覆盖了畸形 threadId 但未覆盖 undefined 情况。undefined 路径通过不同的代码路径抛出错误，该行为未被测试验证。
— qwen3.7-max via Qwen Code /review
The manually added xcase entry had a typo in the sha512 hash (ys → ks), causing npm ci EINTEGRITY failures in CI.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
R5 re-verification at
|
| commit | what it does |
|---|---|
aa89a67d3 |
hand-add gitlab-only lockfile entries |
409d6a8a4 |
the two drain regression tests |
96b77a452 |
groupPolicy docs + runtime warning |
bebd3ffa8 |
fix the xcase integrity hash |
6b2323720 |
merge main |
Verdict: one blocker, and it is a one-character fix. The two behavioural items I asked for in R4 both landed and both are load-bearing — I mutation-tested them rather than taking the commit messages at their word. But npm ci does not work at this head, so nothing on this branch has been built or tested by CI since aa89a67d3 — the same three jobs have been red on every head since.
🔴 1. npm ci fails with EINTEGRITY — @gitbeaker/requester-utils carries a corrupted hash
aa89a67d3 did not only add entries — it also changed the @gitbeaker/requester-utils@42.5.0 hash that was already correct, dropping one character:
"node_modules/@gitbeaker/requester-utils": {
- "integrity": "sha512-…xaVqU4yTXjbTJ5sItOtdB43vYRkBcgueBw==",
+ "integrity": "sha512-…xaVqUyTXjbTJ5sItOtdB43vYRkBcgueBw==",87 base64 characters where sha512 needs 88 — the same class of typo as the xcase one fixed in bebd3ffa8, in the same commit, and it is still live. I checked every integrity hash this PR adds or edits against the live npm registry: 5 correct, this one wrong.
This is not cosmetic. On a clean tree at this head npm ci exits 1, and all three red CI jobs die in the Install Node.js dependencies step on this exact string:
Test (ubuntu-latest, Node 22.x)web-shell E2E Smoke (ubuntu-latest, Node 22.x)Real daemon E2E / Java 11
So the green Java jobs are the only thing passing, and Build Qwen Code / Bundle Qwen Code / the test suite have never run on this head. The "CI green" line in the previous reply doesn't hold right now.
Restoring the single character makes npm ci succeed (2045 packages, 4m) on the otherwise-identical tree — that A/B is the whole diagnosis.
Suggested fix: rather than another hand-edit, run npm install --package-lock-only on a clean tree. I did exactly that after correcting the hash: the generated file differs from your committed one by 18 lines total — a key-ordering move for rate-limiter-flexible and one integrations/external-context version line that main is already out of sync on independently of this PR. In other words the goal you were hand-editing for — no unrelated churn — is what the generated file gives you anyway.
Credit where due: the lockfile diff against main is now exactly the gitlab dependency closure — 8 added entries (@gitbeaker/{rest,core,requester-utils}, picomatch-browser, rate-limiter-flexible, xcase, the workspace and its link) plus the root/cli dependency lines, nothing else. The undici churn I flagged in R1 and re-flagged in R2/R3 is gone. That part is fixed properly.
✅ 2. Both new drain tests are load-bearing — mutation matrix
Every mutant was compiled from real source with the mutation's presence verified in dist before the run, so a survivor is a genuine coverage gap and not an edit that never landed. 409d6a8a4 does what it says: reintroducing either half of the R3/R4 drain fix now turns the suite red.
Still unpinned in-tree, both carried and both non-blocking:
- M3 (
id > lastId→id >=, a duplicate reply on every poll) survives the unit suite; my E2E kills it via A11/A14. - M5 — this round's new
groupPolicywarning has no test; deleting it leaves 48/48 green.
🟡 3. The new docs paragraph overstates the requirement, and one sentence of it is wrong
Two things measured on the wire:
(a) groupPolicy: "allowlist" with the project listed works. It delivers exactly like "open" — 1 prompt, 1 note — because isMentioned is forced true and GroupGate only requires the group to be listed. The docs say groupPolicy must be "open", and connect() now warns for anything else, so the config that restricts which GitLab projects the bot will act on is the one being warned against. For an adapter whose feed is the bot account's entire todo list — every project it can see — that is the safer setting, not the wrong one. Suggest: must be "open", or "allowlist" with the project explicitly listed, and the same condition in the warning.
(b) "no error is logged" is not accurate — and I was the one who put that idea in your hands. ChannelBase writes [Channel:<name>] preflight rejected reason=group_disabled for every dropped todo (in main since #6539), so my R4 phrasing "no log line" was wrong. The substance of R4-🟡2 stands — the todo is still marked done and the cursor still advances, so the mention is consumed either way — but the docs shouldn't tell users nothing is logged when a line is.
🔵 4. The drain's mark_as_done is fire-and-forget — what that costs at scale
TodoLists.done() is called without await in both the drain and the stale-cleanup loop, so pollOnce resolves before any of them land. Driving the compiled adapter over real HTTP, counting requests that actually reached the server in a 7-second window after the poll returned:
| pending todos at first start | prompts to agent | cursor | mark_as_done landed in 7s |
RSS |
|---|---|---|---|---|
| 100 | 0 | {100, true} |
100 | 111 MB |
| 1 000 | 0 | {1000, true} |
1 000 | 200 MB |
| 10 000 | 0 | {10000, true} |
2 999 | 494 MB |
| 150 000 | 0 | {150000, true} |
0 | 2 792 MB |
The R3/R4 contract holds everywhere — a first start dispatches nothing and the cursor is right, including at 150k where R3's build flooded. What the numbers add is that the cleanup half doesn't keep up: at 150k the poll returns having sent nothing, and against a real rate-limited GitLab those rejections are swallowed by .catch(() => {}), so the backlog is re-fetched (20 per page, per_page still unset) on every subsequent poll. That is R1-🟡3 resurfacing at the top end rather than a new defect, and you have already deferred per_page/action filtering.
One note on the test that came with the fix: leaves the cursor uninitialized when the first-poll drain throws injects a synchronous throw from TodoLists.done. Real gitbeaker rejects asynchronously, and the unawaited .catch(() => {}) swallows that — I measured it: with every mark_as_done answering 500, pollOnce does not throw, the cursor saves as {3, true}, the todos stay pending, and the next poll drains them cleanly with 0 dispatches. So the test pins the assignment-ordering invariant (which is what matters, and it does kill M2) rather than a reachable production failure. Worth knowing if it ever gets "simplified".
5. Regression suite and gates at this head
43/43 checks green. No vi.mock anywhere: the compiled dist drives a real @gitbeaker/rest client over real HTTP against a local node:http GitLab v4 server, through the real ChannelBase pipeline (GroupGate → SenderGate → SessionRouter → agent), with an on-disk QWEN_HOME cursor. Every count is prompts that reached the agent, not envelopes the adapter emitted.
| gate | result |
|---|---|
vitest run (channels/gitlab) |
48/48 pass (46 → 48 this round) |
eslint --max-warnings 0 |
clean |
prettier --check (src + docs) |
clean |
tsc --build (base + gitlab + cli) |
clean |
npm run build + npm run bundle |
succeed (with the hash corrected) |
npm ci from the committed lockfile |
fails — EINTEGRITY |
| PR CI | 3 jobs red, all on that install step |
Documented settings.json block re-run through the CLI's own parseChannelConfig + registry: parses, $GITLAB_TOKEN expands, gitlab resolves from the registry, and subgroup / non-ASCII / spaced project paths all survive.
Carried items re-checked and unchanged: the trailing-period mention lookahead (hey @qwen-bot. still doesn't match — cosmetic while forceMentioned is true, as you said); per_page/action server-side filtering; confidential/internal notes, scoped out and documented.
Verdict
Fix the one character — or regenerate the lockfile, which is the more durable answer — and from my side this is merge-ready. Everything else this round is real, and the two tests I asked for actually hold the fix in place.
Reproducing
Harness is fake-gitlab.mjs / harness.mjs / r5-scenarios.mjs / r5-grouppolicy.mjs / r5-scale.mjs against packages/channels/gitlab/dist/index.js; mutants are applied to real source and recompiled with tsc --build --force before each run. The integrity audit diffs the merge-base lockfile against the head lockfile and checks each new-or-changed entry with npm view <pkg>@<ver> dist.integrity.
中文版报告（点击展开）
R5 复验 @ 6b2323720 —— 一个合并阻塞项：手工编辑 lockfile 又写坏了一个 integrity 哈希
接续 R1（9568588c3）、R2（16aac3874）、R3（d70f6ddea）、R4（310e6335）。R4 之后新增五个提交：aa89a67d3（手工添加 lockfile 条目）、409d6a8a4（两个 drain 回归测试）、96b77a452（groupPolicy 文档 + 运行时警告）、bebd3ffa8（修 xcase 哈希）、6b2323720（合并 main）。
结论：一个阻塞项，且只需改一个字符。 R4 提出的两项行为改动都已落地，并且我用变异测试验证了它们真正起作用，而不是相信提交信息。但当前 head 上 npm ci 跑不通，因此自 aa89a67d3 起，CI 从未真正构建或测试过这个分支——此后每个 head 上都是同样三个任务红。
🔴 1. npm ci 因 EINTEGRITY 失败 —— @gitbeaker/requester-utils 的哈希被写坏
aa89a67d3 不只是新增条目，它还修改了原本正确的 @gitbeaker/requester-utils@42.5.0 哈希，少了一个字符（…xaVqU4yTXjb… → …xaVqUyTXjb…）：87 个 base64 字符，而 sha512 需要 88 个。这与 bebd3ffa8 中修掉的 xcase 是同一类笔误、同一个提交，且目前仍然存在。我把本 PR 新增或修改的每一个 integrity 哈希都与 npm 官方 registry 做了核对：5 个正确，这一个错误。
这不是表面问题。干净工作区在当前 head 执行 npm ci 退出码为 1，而三个红色 CI 任务全部死在 Install Node.js dependencies 这一步、且报错字符串完全相同：Test (ubuntu-latest, Node 22.x)、web-shell E2E Smoke、Real daemon E2E / Java 11。也就是说目前只有 Java 那几个任务是绿的，Build Qwen Code / Bundle Qwen Code / 测试套件在这个 head 上从未运行过。上一条回复里"CI 绿"的说法目前并不成立。
在其余完全相同的工作区上，只把这一个字符改回去，npm ci 即成功（2045 个包，4 分钟）——这个 A/B 就是完整的诊断。
建议修法： 不要再手工编辑，在干净工作区跑一次 npm install --package-lock-only。我在修正哈希后正是这么做的：生成结果与你提交的版本总共只差 18 行——rate-limiter-flexible 的键顺序位移，以及一行 integrations/external-context 版本号（那是 main 本身就存在的漂移，与本 PR 无关）。换句话说，你手工编辑想达成的目标——不引入无关变更——用生成的文件同样能得到。
该肯定的地方： 现在 lockfile 相对 main 的差异恰好就是 gitlab 的依赖闭包——8 个新增条目加上根/cli 的依赖行，再无其他。我在 R1 提出、R2/R3 反复重提的 undici 无关变更已经彻底消失，这一项修得很干净。
✅ 2. 两个新增的 drain 测试确实起作用 —— 变异矩阵
每个变异体都由真实源码编译，并在运行前确认变异已出现在 dist 中，所以"存活"是真实的覆盖缺口而非改动未生效。409d6a8a4 名副其实：把 R3/R4 那个修复的任何一半改回去，测试套件都会变红。
仍未在树内钉住、但都不阻塞合并的两项：M3（id > lastId → id >=，每轮轮询重复回复一次）在单测中存活，只能被我的 E2E（A11/A14）杀死；M5 —— 本轮新增的 groupPolicy 警告没有任何测试，删掉它单测依然 48/48 全绿。
🟡 3. 新增的文档段落把要求写得过严，其中一句话是错的
两点均在真实链路上实测：
(a) groupPolicy: "allowlist" 且列出该项目时是可以工作的。 它与 "open" 的表现完全一致（1 条 prompt、1 条 note），因为 isMentioned 被强制为真，而 GroupGate 只要求该 group 在列表中。文档写的是 groupPolicy 必须为 "open"，connect() 现在也会对其他取值告警——于是那个限制 bot 只对哪些 GitLab 项目生效的配置，反而成了被警告的对象。对于一个数据源是 bot 账号全部待办（它能看到的每一个项目）的适配器来说，这恰恰是更安全的设置。建议改为"必须为 "open"，或使用 "allowlist" 并显式列出项目"，警告条件同步调整。
(b) "不会记录任何错误"这句不准确——而这个说法源头在我。 ChannelBase 对每一条被丢弃的 todo 都会输出 [Channel:<name>] preflight rejected reason=group_disabled（自 #6539 起就在 main 中），所以我在 R4 里"没有任何日志"的说法是错的。R4-🟡2 的实质结论不变——todo 仍会被标记完成、游标仍会前进，这条 mention 无论如何都被消费掉了——但文档不该告诉用户什么都不会记录。
🔵 4. drain 的 mark_as_done 是发射后不管 —— 大规模下的实测代价
无论是首轮 drain 还是 stale 清理循环，TodoLists.done() 都没有 await，因此 pollOnce 在任何一个请求落地之前就已经 resolve。用编译产物走真实 HTTP，统计轮询返回后 7 秒窗口内真正到达服务端的请求数：
| 首次启动时的待办数 | 派发给 agent | 游标 | 7 秒内完成标记 | RSS |
|---|---|---|---|---|
| 100 | 0 | {100, true} |
100 | 111 MB |
| 1 000 | 0 | {1000, true} |
1 000 | 200 MB |
| 10 000 | 0 | {10000, true} |
2 999 | 494 MB |
| 150 000 | 0 | {150000, true} |
0 | 2 792 MB |
R3/R4 的契约在所有量级下都成立——首次启动不派发任何消息、游标正确，包括 R3 构建会洪泛的 15 万量级。这些数字新增的信息是：清理那一半跟不上。15 万时轮询返回，实际一条都没发出；面对真实的、有限流的 GitLab，这些被拒绝的请求会被 .catch(() => {}) 吞掉，于是整个积压会在之后每一轮被重新拉取（每页 20 条，per_page 仍未设置）。这是 R1-🟡3 在量级上端的再现，而非新缺陷，且 per_page/action 过滤你已明确延后处理。
关于随修复一起提交的那个测试补充一句：leaves the cursor uninitialized when the first-poll drain throws 注入的是 TodoLists.done 的同步抛出。真实的 gitbeaker 是异步 reject，而未 await 的 .catch(() => {}) 会把它吞掉——我实测过：让每个 mark_as_done 都返回 500 时，pollOnce 不会抛错，游标写入 {3, true}，todo 保持 pending，下一轮轮询把它们干净地清理掉且 0 次派发。所以这个测试钉住的是赋值顺序不变量（这也正是关键所在，它确实杀死了 M2），而不是一个真实可达的生产故障。万一日后有人想"简化"它，值得知道这一点。
5. 回归套件与各项门禁
43/43 全绿。 全程无 vi.mock：编译后的 dist 通过真实 @gitbeaker/rest 客户端、真实 HTTP，访问本地 node:http 实现的 GitLab v4 服务，走完整 ChannelBase 管线（GroupGate → SenderGate → SessionRouter → agent），游标落在 QWEN_HOME 下的真实文件。所有计数都是真正到达 agent 的 prompt，而不是适配器发出的 envelope。
| 门禁 | 结果 |
|---|---|
vitest run（channels/gitlab） |
48/48 通过（本轮 46 → 48） |
eslint --max-warnings 0 |
干净 |
prettier --check（源码 + 文档） |
干净 |
tsc --build（base + gitlab + cli） |
干净 |
npm run build + npm run bundle |
成功（在修正哈希后） |
用提交的 lockfile 执行 npm ci |
失败 —— EINTEGRITY |
| PR CI | 3 个任务红，全部卡在该安装步骤 |
文档中的 settings.json 配置块再次通过 CLI 自己的 parseChannelConfig + registry 跑通：可解析、$GITLAB_TOKEN 正确展开、gitlab 能从 registry 解析出来，子群组 / 非 ASCII / 含空格的项目路径也都能通过。
延续项复查、状态未变：句尾句号的 mention 前瞻（hey @qwen-bot. 仍不匹配——如你所说，在 forceMentioned 为真时只是外观问题）；per_page/action 服务端过滤；机密/内部评论，已明确划出范围并写入文档。
结论
把那一个字符改回来——或者更彻底一点，重新生成 lockfile——在我这边就可以合并了。本轮其余内容都经得起检验，而且我要求的那两个测试确实把修复钉住了。
|
Fixed the Also completed a live E2E test against real GitLab (gitlab.com) covering 20 scenarios across the full adapter surface:
Test setup: fresh repo zore3475/gl-e2e-0729, Key details:
R5-🟡3 (groupPolicy docs) fixed in 中文翻译已在 同时完成了针对真实 GitLab（gitlab.com）的 E2E 测试，覆盖适配器全貌共 20 个场景：
测试环境： 全新仓库 zore3475/gl-e2e-0729， 关键细节：
R5-🟡3（groupPolicy 文档）已在 |
The groupPolicy warning and docs incorrectly stated that groupPolicy must be "open". In reality "allowlist" with the project listed also works because isMentioned is forced true and GroupGate only requires the group to be listed. Also fix the inaccurate "no error is logged" claim — ChannelBase logs preflight rejected reason=group_disabled. Fixes R5-🟡3 from PR QwenLM#7862 review.
R6 re-verification at
|
| commit | what it does |
|---|---|
d32549eab |
restore the @gitbeaker/requester-utils integrity hash |
b48dbe5c0 |
accept groupPolicy: "allowlist" in the warning and the docs |
a586788dc |
merge main |
Verdict: merge-ready, no blockers. R5's blocker is gone, and I re-audited the entire lockfile rather than only the entry I had reported — that distinction is what R5 was about. The groupPolicy change is correct for the case it was written for. I have one 🟡 about the case it stops warning about; it is a polish item, not a merge condition.
✅ 1. The blocker is fully resolved — and the lockfile is now exactly right
npm ci on a clean tree at this head succeeds: exit 0, 2043 packages, ~2m, no EINTEGRITY, and @gitbeaker/{core,requester-utils,rest}@42.5.0 land on disk.
I re-checked every entry this PR adds or changes against the live npm registry, not just the one I reported — the R5 lesson being that fixing the reported hash is not the same as the file being correct:
- 6 registry-backed entries → 6/6 match, every payload exactly 88 base64 characters
- 2 workspace entries (link + workspace package — no tarball, nothing to verify)
The diff against merge-base cc617e6707 is now purely additive: 84 added lines, zero deletions, and is exactly the gitlab dependency closure. R4's 🔵 version mismatch is gone too — packages/channels/gitlab is 0.21.1 with a 0.21.1 channel-base dep in both package.json and the lockfile.
CI agrees, which is the part that matters most: on this head Install dependencies, Check lockfile, ESLint/actionlint/shellcheck and Real daemon E2E / Java 11 are all green. That last job had been red on every head since aa89a67d3. Test (ubuntu-latest, Node 22.x) was still running as I wrote this, but it is well past the install step that used to kill it.
✅ 2. groupPolicy: "allowlist" really does deliver — measured on the wire, and 🟡 the widened warning now misses two configs that don't
I built three dists from real source — pre-fix (6b2323720), HEAD, and a proposed variant — and ran all six policy configurations through the compiled adapter over real HTTP and the real GroupGate. Each build's condition was grepped out of dist/GitlabAdapter.js before its run, so no result here comes from an edit that didn't land.
The good news, and it is the substantive half: P4 — "allowlist" with the project listed — delivers exactly like "open" (1 prompt, 1 note) and no longer warns. That is what b48dbe5c0 set out to fix and it is genuinely fixed. Dispatch behaviour is identical across all three builds; only the warning moved, so this change carries no functional risk.
The 🟡: "allowlist" is only permissive when GroupGate finds the project in groups. Two configurations still drop every mention and still consume it — todo marked done, cursor advanced — but no longer produce the connect warning, where pre-fix they did:
| cell | config | dispatch | mention consumed | pre-fix warn | HEAD warn |
|---|---|---|---|---|---|
| P4 | "allowlist" + project listed |
1 / 1 | — | YES (false positive) | no ✅ |
| P5 | "allowlist" + groups: {} |
0 / 0 | yes | YES | no |
| P6 | "allowlist" + groups: {"*": {}} |
0 / 0 | yes | YES | no |
P6 is the one I'd weigh most: "*" is the documented defaults key (overview.md: "Keys are group chat IDs or "*" for defaults"), and GroupGate deliberately does not treat it as a wildcard allow. A user who reads the new docs line — "or "allowlist" with the project explicitly listed" — gets no example of what a listed project looks like, and GitLab's chatId is the project path (acme/widgets), not a numeric ID like the other channels. groups: {"*": {}} is the natural wrong guess, and it fails silently.
Both cells do log preflight rejected reason=group_not_allowlisted, so this is a startup-warning gap rather than total silence — same shape as the accurate correction you made to the "no error is logged" sentence.
A condition that matches GroupGate exactly, validated against all six cells:
const listedGroups = Object.keys(cfg.groups ?? {}).filter((g) => g !== '*');
const allowlistUsable =
cfg.groupPolicy === 'allowlist' && listedGroups.length > 0;
if (cfg.groupPolicy !== 'open' && !allowlistUsable) {
// ...existing warning...
}→ warns on exactly P1/P2/P5/P6, silent on P3/P4. Worth pairing with a concrete groups example in gitlab.md, since the project-path form is the non-obvious bit.
✅ 3. The drain tests survived the main merge — and 🔵 this round's changed line has none
Every mutant was applied to real source and verified present in the file before the suite ran — a no-op edit is indistinguishable from a survivor otherwise.
| mutant | change | in-tree suite (48) |
|---|---|---|
| M1 | groupPolicy warning: drop the new "allowlist" arm |
SURVIVES |
| M2 | groupPolicy warning: delete the block entirely | SURVIVES |
| M3 | t.id > lastId → t.id >= lastId |
SURVIVES |
| M4 | drain maxId: reduce() → Math.max(...spread) |
DIES |
| M5 | drain ordering: initialized = true before the fallible work |
DIES |
M4/M5 still die — drains a very large backlog without dispatching and leaves the cursor uninitialized when the first-poll drain throws (both from 409d6a8a4) remain load-bearing after the merge. The R3/R4 drain hardening is genuinely pinned now.
🔵 M1/M2 survive: the line this round changed has no test at all — neither the new "allowlist" arm nor the warning block itself. One it() closes both; I wrote and validated it (passes 49/49 at HEAD, kills M1 and M2):
it('warns only when groupPolicy cannot dispatch', async () => {
const lines: string[] = [];
const spy = vi
.spyOn(process.stderr, 'write')
.mockImplementation((chunk: string | Uint8Array) => {
lines.push(String(chunk));
return true;
});
try {
const cases: Array<[Record<string, unknown>, boolean]> = [
[{ groupPolicy: 'open' }, false],
[{ groupPolicy: 'allowlist', groups: { 'acme/widgets': {} } }, false],
[{ groupPolicy: 'disabled' }, true],
];
for (const [overrides, shouldWarn] of cases) {
lines.length = 0;
const ch = new TestableGitlabChannel(
'test-gl',
makeConfig(overrides),
makeBridge(),
);
await ch.connect();
ch.disconnect();
const warned = lines.some((l) => l.includes('warning: groupPolicy is'));
expect(warned, JSON.stringify(overrides)).toBe(shouldWarn);
}
} finally {
spy.mockRestore();
}
});🔵 M3 is carried from R3/R5, still open. It produces a duplicate reply on every poll. Invisible to the unit suite; my E2E kills it (B3 "stale todos never re-dispatched" → got 2, want 1; E2 "mark_as_done failure" → got 3, want 2). Non-blocking, but it is the last real coverage hole in the cursor logic.
4. Regression suite and gates at this head
24 scenarios / 72 checks, all green. No vi.mock anywhere: the compiled dist drives a real @gitbeaker/rest client over real HTTP against a local node:http GitLab v4 server, through the real ChannelBase pipeline (GroupGate → SenderGate → SessionRouter → agent), with an on-disk QWEN_HOME cursor. Every count is prompts that reached the agent, not envelopes the adapter emitted.
| gate | result |
|---|---|
npm ci from the committed lockfile |
exit 0 (2043 pkgs, 2m) |
| integrity audit vs live npm registry | 6/6 match, 0 bad |
| lockfile diff vs merge-base | +84 / −0, gitlab closure only |
vitest run (channels/gitlab) |
48/48 |
tsc --build (base + gitlab + cli) |
clean |
eslint --max-warnings 0 |
clean |
prettier --check (src + docs) |
clean |
| real-HTTP E2E matrix | 24 scenarios / 72 checks |
| groupPolicy wire matrix | 6/6 cells as expected |
| PR CI (install + lockfile + daemon E2E) | success |
Carried items re-checked and unchanged: the trailing-period mention lookahead (cosmetic while forceMentioned is true); per_page / action server-side filtering; confidential/internal notes — all scoped out and documented. The fire-and-forget mark_as_done cost curve from R5-🔵4 is unchanged by these three commits and I did not re-measure it.
Verdict
Merge-ready. The one thing that was blocking is fixed properly — regenerated rather than patched, and the whole file verifies clean. Items 2 and 3 are worth a follow-up commit if you want them, but I would not hold the merge for either.
Reproducing
Harness is fake-gitlab.mjs / harness.mjs / run.mjs / r6-grouppolicy.mjs / mutate.py against packages/channels/gitlab/dist/index.js. Control builds come from git show <sha>:…/GitlabAdapter.ts → tsc --build --force → mv dist dist-<tag>, with the expected condition grepped out of the compiled JS before each run. The integrity audit diffs the merge-base lockfile against the head lockfile and checks every added-or-changed entry with npm view <pkg>@<ver> dist.integrity. The harness refuses to construct a channel whose baseUrl is not http://127.0.0.1:*, so it cannot reach gitlab.com.
中文版报告（点击展开）
R6 复验 @ a586788dc —— 可以合并：lockfile 阻塞项已彻底解决，groupPolicy 修复在关键场景上是正确的
接 R1（9568588c3）、R2（16aac3874）、R3（d70f6ddea）、R4（310e6335）和 R5（6b2323720）。R5 之后新增三个提交：
| 提交 | 内容 |
|---|---|
d32549eab |
修复 @gitbeaker/requester-utils 的 integrity 哈希 |
b48dbe5c0 |
警告与文档接受 groupPolicy: "allowlist" |
a586788dc |
合并 main |
结论：可以合并，无阻塞项。 R5 的阻塞项已消除；我重新审计了整个 lockfile，而不只是我上轮报告的那一条——这个区别正是 R5 的教训所在。groupPolicy 的改动在它针对的场景上是正确的。我对它不再警告的那类场景有一个 🟡，属于打磨项，不是合并条件。
✅ 1. 阻塞项已彻底解决，且 lockfile 现在完全正确
在该 head 的干净工作树上 npm ci 成功：退出码 0，2043 个包，约 2 分钟，无 EINTEGRITY，@gitbeaker/{core,requester-utils,rest}@42.5.0 正确落盘。
我把这个 PR 新增或修改的每一条 lockfile 条目都与 npm 官方 registry 重新核对了一遍，而不只是我报告过的那条——R5 的教训是：修好被报告的那个哈希，不等于整个文件是对的：
- 6 条来自 registry 的条目 → 6/6 完全一致，每个 payload 恰好 88 个 base64 字符
- 2 条 workspace 条目（link 与 workspace 包，无 tarball，无需校验）
与合并基 cc617e6707 的差异现在是纯新增：84 行新增，0 行删除，且恰好就是 gitlab 的依赖闭包。R4 的 🔵 版本不一致也一并消失：packages/channels/gitlab 在 package.json 与 lockfile 中均为 0.21.1，channel-base 依赖同为 0.21.1。
CI 也印证了这一点，这是最关键的部分：该 head 上 Install dependencies、Check lockfile、ESLint/actionlint/shellcheck 以及 Real daemon E2E / Java 11 全部通过。最后这个 job 自 aa89a67d3 起在每个 head 上都是红的。撰写本报告时 Test (ubuntu-latest, Node 22.x) 仍在运行，但已远远越过此前导致它失败的安装步骤。
✅ 2. groupPolicy: "allowlist" 确实能投递（已在真实链路上测量），🟡 但放宽后的警告漏掉了两种不能投递的配置
我从真实源码构建了三份 dist——修复前（6b2323720）、HEAD、以及一个改进版本——并让全部六种 policy 配置经由编译产物、真实 HTTP 与真实 GroupGate 跑通。每份构建在运行前都从 dist/GitlabAdapter.js 中 grep 确认了对应条件，因此这里没有任何结论来自"没生效的改动"。
好消息，也是实质性的一半： P4——"allowlist" 且项目已列出——与 "open" 投递行为完全一致（1 个 prompt、1 条 note），且不再告警。这正是 b48dbe5c0 想修的，确实修好了。三份构建的投递行为完全相同，只有警告位置发生了变化，因此该改动没有功能风险。
🟡 的部分： 只有当 GroupGate 在 groups 中找到该项目时，"allowlist" 才是放行的。有两种配置仍然丢弃每一条 mention 并且仍然消费它（todo 被标记完成、游标推进），但不再产生 connect 警告——而修复前它们是会告警的：
| 场景 | 配置 | 投递 | mention 被消费 | 修复前告警 | HEAD 告警 |
|---|---|---|---|---|---|
| P4 | "allowlist" + 项目已列出 |
1 / 1 | — | YES（误报） | no ✅ |
| P5 | "allowlist" + groups: {} |
0 / 0 | 是 | YES | no |
| P6 | "allowlist" + groups: {"*": {}} |
0 / 0 | 是 | YES | no |
我最看重 P6："*" 是文档中规定的"默认值"键（overview.md："键为群聊 ID，或 "*" 表示默认值"），而 GroupGate 有意不把它当作通配放行。用户读到新增的文档句子——"或 "allowlist" 并显式列出该项目"——却没有任何示例说明"列出的项目"长什么样，而 GitLab 的 chatId 是项目路径（acme/widgets），不像其他 channel 是数字 ID。groups: {"*": {}} 是最自然的错误猜测，而且它是静默失败的。
这两种场景都会记录 preflight rejected reason=group_not_allowlisted，所以这是启动警告的覆盖缺口，而非完全无声——与你把"no error is logged"那句话改正的性质相同。
一个与 GroupGate 完全对齐的条件，已在全部六个场景上验证：
const listedGroups = Object.keys(cfg.groups ?? {}).filter((g) => g !== '*');
const allowlistUsable =
cfg.groupPolicy === 'allowlist' && listedGroups.length > 0;
if (cfg.groupPolicy !== 'open' && !allowlistUsable) {
// ...原有警告...
}→ 恰好在 P1/P2/P5/P6 告警，在 P3/P4 保持静默。建议同时在 gitlab.md 里补一个具体的 groups 示例，因为"项目路径"这个形式才是不直观的地方。
✅ 3. drain 回归测试在 main 合并后依然生效，🔵 但本轮改动的那一行没有任何测试
每个变异体都作用于真实源码，并在运行测试前确认已写入文件——否则"没生效的改动"与"存活的变异体"无法区分。
| 变异体 | 改动 | 仓库内测试套件（48） |
|---|---|---|
| M1 | groupPolicy 警告：去掉新增的 "allowlist" 分支 |
存活 |
| M2 | groupPolicy 警告：整块删除 | 存活 |
| M3 | t.id > lastId → t.id >= lastId |
存活 |
| M4 | drain maxId：reduce() → Math.max(...spread) |
被杀死 |
| M5 | drain 顺序：initialized = true 提到易失败逻辑之前 |
被杀死 |
M4/M5 仍然被杀死——drains a very large backlog without dispatching 与 leaves the cursor uninitialized when the first-poll drain throws（均来自 409d6a8a4）在合并后依然是有效约束。R3/R4 的 drain 加固现在确实被钉住了。
🔵 M1/M2 存活：本轮改动的那一行完全没有测试——无论是新增的 "allowlist" 分支还是警告块本身。一个 it() 就能同时覆盖两者；我已写好并验证（HEAD 上 49/49 通过，能杀死 M1 与 M2，耗时增加约 10ms），代码见英文版。
🔵 M3 自 R3/R5 延续至今，仍未关闭。 它会导致每一轮 poll 都重复回复一次。仓库内测试看不见；我的 E2E 能杀死它（B3"过期 todo 不重复派发" → 实得 2，期望 1；E2"mark_as_done 失败" → 实得 3，期望 2）。不阻塞合并，但这是游标逻辑中最后一个真实的覆盖缺口。
4. 该 head 上的回归套件与各项闸门
24 个场景 / 72 项检查全绿。 全程无 vi.mock：编译后的 dist 驱动真实 @gitbeaker/rest 客户端，通过真实 HTTP 访问本地 node:http 实现的 GitLab v4 服务，经过真实的 ChannelBase 管线（GroupGate → SenderGate → SessionRouter → agent），游标落在磁盘上的 QWEN_HOME。所有计数均为真正到达 agent 的 prompt，而非适配器发出的信封。
| 闸门 | 结果 |
|---|---|
用提交的 lockfile 执行 npm ci |
退出码 0（2043 包，2 分钟） |
| integrity 与 npm registry 核对 | 6/6 一致，0 错误 |
| lockfile 与合并基的差异 | +84 / −0，仅 gitlab 闭包 |
vitest run（channels/gitlab） |
48/48 |
tsc --build（base + gitlab + cli） |
通过 |
eslint --max-warnings 0 |
通过 |
prettier --check（src + docs） |
通过 |
| 真实 HTTP E2E 矩阵 | 24 场景 / 72 检查 |
| groupPolicy 链路矩阵 | 6/6 符合预期 |
| PR CI（安装 + lockfile + daemon E2E） | 通过 |
延续项复查无变化：mention 前瞻的句末句点问题（在 forceMentioned 为真时仅属外观问题）；per_page / action 服务端过滤；confidential/internal notes——均已明确划出范围并写入文档。R5-🔵4 中 mark_as_done fire-and-forget 的开销曲线不受这三个提交影响，本轮未重新测量。
结论
可以合并。 唯一的阻塞项已被正确修复——是重新生成而非手工打补丁，且整个文件校验干净。第 2、3 项如果你愿意可以用一个后续提交处理，但我不会因为它们中的任何一个而阻塞合并。
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
…unds (QwenLM#8010) * feat(verify-pr): add four techniques from maintainer verification rounds Two hand-written maintainer rounds contained methods the skill could not have produced. Checked each against the current text before adding it; these four had no coverage at all. From QwenLM#7914 (live daemon A/B on the artifact-recording change): - Run every control on BOTH arms. That round's sharpest finding came from a control whose only job was to validate the BASE probe — "the empty list is a real absence, so have the model call record_artifact and watch an entry appear". Run on head as well, it showed the curated title being silently discarded. The control was not hunting for a bug; running it symmetrically is what found one. - A new writer into a shared store is an ordering change. The PR added write_file as a second writer into the artifact list; the bug was not in the new writer but in the collision, where a pre-existing first-writer-wins merge began discarding record_artifact's curated title and description while still reporting success. Enumerate the other writers, exercise the collision in both orders, and check what the loser is told — and separate the pre-existing cause from the PR's contribution so the author is not blamed for the policy. From QwenLM#7998 (ink cursor fix, real-terminal A/B): - When the oracle is an instrument, corroborate it with a mechanism that does not use that instrument. The hardware cursor row came from `tmux display-message -p '#{cursor_y}'`, then from a marker printed after the TUI exits — which lands wherever the cursor actually was. Two agreeing instruments turn a measurement into evidence; one tool's report about the system is not the system. - Re-run the generator on committed generated artifacts and diff. That round re-ran `npx patch-package ink` and found byte-different hunk headers, proving the .d.ts hunks were hand-written rather than regenerated as the description claimed. Also strengthens Not covered: proving a limitation is environmental requires an A/A control (boot base and head identically, show both fail the same way), because "seems environmental" and a real regression look identical in a report. Mutation-verified 4/4, each with landing proof. Two initially reported `landed: False` — the assertions match the whitespace-normalised text while the rules wrap across lines in the source, so the replace never fired and the green result proved nothing. Re-run against line-accurate anchors, both kill. 89/89 tests; prettier and eslint clean. * feat(verify-pr): teach the timing-race and scenario-arrival checks Third maintainer round mined for method (QwenLM#7934 R4). The blocker it found had zero coverage in the skill — `timer`, `wall-clock`, `flake`, `retry`, `duration`, `deterministic` all returned 0, and the one `race` hit was a substring of "trace". - **Timing-triggered assertions have a threshold — measure it, do not sample it.** A new guard (`expect(false).toBe(true)` after an abort loop) turned a vacuous pass into a deterministic failure, because the case triggers its abort from `setTimeout(..., 1000)` while the query's duration is set by CLI startup rather than the server. Natural completion measured 730-2151 ms, so every box on the fast side of 1000 ms fails. The rule says to measure the operation's natural duration with the trigger disabled and compare it to the timer, because a green run only proves this box was slow enough. - **A speed-correlated failure is not flake, and a retry budget does not absorb it.** Random flake becomes a pass under `retry: 2`; this failed 5/5 runs on all three attempts. The two get opposite verdicts, so the kind has to be established before the verdict is written. Stated plainly in the skill: the verify job runs on a shared, loaded runner — the regime where such a test PASSES. Repetition cannot reproduce a fast-machine failure there; only computing the margin can. A rule that said "run it more times" would be useless in this lane. - **The failure one level before vacuity: the scenario never reached the code under test.** The vacuity check asks whether an assertion can fail; this asks whether the code ever ran. Four abort cases fired during CLI process startup, so the fake server saw zero requests and a suite named for mid-stream aborts never streamed — with every assertion green. Instrument the seam and assert the count is non-zero. Mutation-verified 5/5, each with landing proof against line-accurate anchors. 89/89 tests; prettier and eslint clean. Skill is 472 lines, up from 392. * feat(verify-pr): six more techniques, from three maintainer rounds Mined QwenLM#7836 R2, QwenLM#7885 and QwenLM#7899 for method. Checked each candidate against the current text first; six had zero coverage, the rest were already there (harness teeth-checks, pin dereferencing, boundary probing, and the follow-up round's "re-measure, never diff"). The one that corrects the skill's own core method, from QwenLM#7836: - **Before calling a survivor vacuous, escalate to a finer mutation.** A whole-file revert is blunt enough to remove the PRECONDITION a test depends on, so a good test goes green because its scenario no longer occurs — from the outside, identical to a test that asserts nothing. A `finally`-cleanup test survived reverting four production files and died to deleting one line. Coarse survived + fine killed ⇒ the test is fine and the mutation was wrong. A false "your test is vacuous" costs the author more than a missed survivor does. From QwenLM#7836, the root cause shared by both of its blockers: - **When the same predicate is checked in two places, verify they see the same state.** A guard duplicated across a process boundary is two implementations of one question that diverge when their INPUTS differ. One settings key made a route ask sessionExistsInAnyState() with an unpinned runtime dir while the child asked with a pinned one, turning a clean 409 into a 500 plus a process.exit(1) that killed every session on the channel. Includes the temporal half: lazily-created backing files leave a window where a just-created entity is invisible to any on-disk existence check. - **Measure the blast radius on bystanders.** The caller's own error code understates a shared-state failure; the number that matters is an unrelated session going 200 -> 404 and a workspace list going 2 -> 0. From QwenLM#7885, which took a performance claim apart: - **Isolate the slice the mechanism can actually affect.** A speedup claim is two claims: the mechanism works, and the thing it speeds up matters. `--ignore-scripts` isolated what an npm download cache can touch — 36s of a 226s install — so the ceiling was 20s and the real saving 15%, not the claimed 75%. Then check it against the whole job: 33s off 14m37s. - **A mechanism that persists something has a cost — price it.** 219 MB per lockfile hash into a pool at 9.98 GB of a 10 GB cap, with 39 distinct lockfile states in 30 days: at the cap every entry evicts by LRU, including entries other jobs need and its own. - **Test the scarier consequences and report which do NOT hold.** The write-path finding was real; code injection was disproved (npm integrity-checks a tampered cache and refetches) and privilege escalation was disproved (chown -R does not follow symlinks). A finding that names what it is not is harder to wave away. - **Verify third-party actions from their own manifest.** The PR said the cache dir was discarded after the job; `action.yml` declares `post: dist/save/index.js` with `post-if: success()`, which uploads it as root with credentials intact — the opposite of the claim, and the whole finding. From QwenLM#7899: - **To exercise real production data safely, interpose a refusing proxy on the write path.** Wrap the client so every mutating call hard- fails, then run the shipped script verbatim: real counts, mechanical guarantee of no side effects. Mutation-verified 9/9, each with landing proof against line-accurate anchors. 89/89 tests; prettier and eslint clean. The skill is now 546 lines, up from 392 — the growth is deliberate and called out in the PR body's risk section. * feat(verify-pr): decomposed fixes, contextual limits, destination counts From QwenLM#7862 R4. Three additions, and a deliberate stop. - **When one fix bundles two changes, build the intermediate variants.** An A/B against base proves the pair works and says nothing about what each half does. That round compiled a third build with only the ordering change reverted, and the three-row table showed the halves do different jobs: moving `initialized = true` after the fallible work converts a 2,999-and-climbing backlog flood into a fail-safe retry, while `reduce()` restores liveness. Either alone leaves a channel that floods or wedges — a conclusion the two-cell A/B cannot reach. - **A limit measured in isolation does not transfer to the real call site.** The same `Math.max` spread threw between 110k and 130k elements inside a deep async stack, well below a standalone micro-benchmark. Bisect thresholds through the real code path and quote the harness; a limit taken from documentation or a toy loop is a guess about the system under test. - **Count at the destination, not at the component boundary.** The mirror of the scenario-arrival rule added earlier: envelopes the adapter emitted and prompts that reached the agent are different numbers, and every gate lives between them. A count taken at the seam can be right while the feature is silently dropped downstream. Declined from the same report, to protect prompt budget rather than because they are wrong: siblings-as-convention-oracle (the lockfile version table across five channels), degenerate fixtures that cannot distinguish two sort keys, and naming the condition under which a cosmetic finding becomes real. Each is a good technique; none is worth another rule competing for attention with the ones already here. The skill is now 578 lines, up from 392 on main (+47%) across this branch. That growth is the main risk on this PR and further additions should wait until a live round shows the current set changes behaviour. Mutation-verified 3/3 with landing proof. One mutation initially SURVIVED — it deleted text sitting AFTER the asserted phrase, so the assertion still matched and the green proved nothing; re-run against the phrase itself, it kills. 89/89 tests; prettier and eslint clean. * test(scripts): drop stale technique count from verify test name (QwenLM#8010) * fix(triage): correct verify-skill worked examples and verdict path (QwenLM#8010) Address review feedback on the verification-techniques skill: - Make the npm-cache worked example's numbers close: separate the 20 s download-slice ceiling (36 s to 16 s) from the 15% end-to-end saving (226 s to 193 s) rather than conflating them. - Stop overstating the tarball experiment: one tarball was poisoned, and the 2262-entry integrity coverage is a separate static fact. - Give the speed-correlated-failure rule a contract-legal verdict path by encoding the margin as a scripted assertion, and mark the load/idle sweep as the local-mode variant. - Fix the one bullet that broke its 2-space list continuation. - Pin the new contract-encoding clause in the workflow test. --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
|
Released in v0.21.2. |
|
感谢 GitLab 频道适配器的工作！GitHub 和 GitLab 频道目前功能已经很完善了，但 Web Shell 的频道管理 UI 还只支持 DingTalk / WeCom / Feishu——GitHub 和 GitLab 仍然只能通过 想邀请你把这两个频道的 Web Shell 管理支持也补上，需要两处改动：
这样用户就能在 Web Shell 里直接创建和管理 GitHub / GitLab 频道实例了。可以开一个新 PR 来做。 |
|
Yes, happy to take this on! I'll open a new PR for it tomorrow. 中文翻译好的，很乐意来做这个！明天开一个新 PR。 |
5 participants
What this PR does
Adds a GitLab polling channel adapter that monitors GitLab todos and dispatches inbound messages through the existing channel pipeline. The adapter uses
@gitbeaker/restas the API client and extendsPollingChannelBase, following the same architecture as the GitHub adapter.The core design principle is todo-as-message: each GitLab todo corresponds to one inbound message. The adapter dispatches
todo.body(the triggering comment text) directly — no notes list fetching, no time-window filtering. Deduplication relies on cursor advancement and mark_done.Key behaviors:
action_prompt_template: only todo actions with a configured template are processed; all others are skipped and marked done.directly_addressed(comment starting with @bot) automatically falls back to thementionedtemplate.target_urlanchor: iftarget_urlcontains#note_<id>, the mention occurred in a comment andtodo.bodyis the comment text. If no anchor is present, the mention occurred in the issue/MR description, and the adapter fetches the description viaIssues.show/MergeRequests.show.{ lastProcessedId: number, initialized: boolean }validated with zod. Uses monotonically increasing todo IDs instead of timestamps to eliminate equal-timestamp collision loss. On first start (initialized: false), all pre-existing pending todos are drained (marked done without dispatch). The cursor advances regardless of dispatch success or failure — errors post aTodoLists.doneis best-effort (called after cursor advancement).%project%,%project_url%,%target_type%,%iid%,%title%,%description%,%todo_id%,%author%) and appended below the message text by the base class.%%escapes to a literal%.sendThreadMessageposts notes viaIssueNotes.create/MergeRequestNotes.create.Why it's needed
Users running GitLab-based workflows currently have no channel adapter to connect their GitLab mentions to Qwen Code. The GitHub adapter only covers GitHub notifications. This PR closes that gap for GitLab self-hosted and SaaS instances, enabling the same "mention the bot in an issue/MR and get an agent response" workflow.
Reviewer Test Plan
How to verify
.qwen/settings.jsonwith a PAT that hasread_api+apiscopes:{ "channels": { "my-gitlab": { "type": "gitlab", "token": "$GITLAB_TOKEN", "cwd": "/path/to/project", "senderPolicy": "open", "groupPolicy": "open", "action_prompt_template": { "mentioned": "Project: %project% | URL: %project_url% | Author: %author% | Type: %target_type% | IID: %iid% | Title: %title% | Description: %description% | TodoID: %todo_id%" } } } }qwen channel start my-gitlab), mention the bot in a GitLab issue comment, and confirm the agent session receives the message with rendered metadata.cd packages/channels/gitlab && npx vitest run— 30 tests covering poll flow, filtering, cursor advancement, first-poll drain, template rendering, error handling, description mention detection.Evidence (Before & After)
N/A (new adapter, no prior behavior)
Tested on
Environment (optional)
Unit tests (30/30 pass),
tsc --build, ESLint clean. Full E2E against gitlab.com:Risk & Scope
todo.bodydirectly without fetching the full notes list. This means only the triggering comment is processed per todo — follow-up comments require new todos (new mentions). This is by design: GitLab creates a new todo for each mention event.Linked Issues
N/A
中文说明
本 PR 做了什么
新增 GitLab 轮询 channel 适配器，监控 GitLab todo 并通过现有 channel 管线派发消息。使用
@gitbeaker/rest作为 API 客户端，继承PollingChannelBase，遵循与 GitHub 适配器相同的架构。核心设计原则是 todo 即消息：每个 GitLab todo 对应一条待处理消息。适配器直接派发
todo.body（触发评论文本）——不拉 notes 列表，不做时间窗口过滤。去重靠 cursor 推进 + mark_done。关键行为：
action_prompt_template配置驱动。只处理配置了模板的 action；其余跳过并 mark_done。directly_addressed（评论以 @bot 开头）自动回退到mentioned模板。target_url锚点判断。含#note_<id>→ 评论 mention → 用todo.body；无锚点 → 描述 mention → 拉Issues.show/MergeRequests.show取 description。{ lastProcessedId: number, initialized: boolean }，zod 校验。使用单调递增的 todo ID 代替时间戳，消除同时间戳碰撞丢失问题。首次启动（initialized: false）时 drain 所有已存在 pending todo（mark done 不派发）。无论成功失败都推进——错误发TodoLists.done是 best-effort（cursor 推进后调用）。%project%、%project_url%、%target_type%、%iid%、%title%、%description%、%todo_id%、%author%），由 base class 拼接在消息文本下方。%%转义为字面%。sendThreadMessage通过IssueNotes.create/MergeRequestNotes.create发评论。为什么需要
GitLab 用户目前无法将 GitLab mention 连接到 Qwen Code。GitHub 适配器只覆盖 GitHub 通知。本 PR 为 GitLab（自托管和 SaaS）补齐这一能力。
Reviewer 测试计划
如何验证
.qwen/settings.json中配置 GitLab channel，使用具有read_api+api权限的 PAT。cd packages/channels/gitlab && npx vitest run— 30 个测试。风险与范围
todo.body，不拉完整 notes 列表。每条 todo 只处理触发评论——后续评论需要新的 todo（新的 mention）。这是设计意图。