Skip to content

feat(gitlab-channel): add transient 👀 award emoji while agent is working - #8119

Merged
wenshao merged 4 commits into
QwenLM:mainfrom
OrbitZore:feat/gitlab-award-emoji
Jul 31, 2026
Merged

feat(gitlab-channel): add transient 👀 award emoji while agent is working#8119
wenshao merged 4 commits into
QwenLM:mainfrom
OrbitZore:feat/gitlab-award-emoji

Conversation

@OrbitZore

@OrbitZore OrbitZore commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds a transient 👀 award emoji to GitLab note mentions while the agent is working, mirroring the GitHub adapter's eyes reaction (#8061). When onPromptStart fires for a note mention, the channel calls the GitLab award emoji API to add 👀 to the triggering note. When onPromptEnd fires (completion, failure, or cancellation), the emoji is removed. Both operations are best-effort: API errors or permission failures are logged to stderr and never block the agent's response.

Description mentions (no #note_ anchor in target_url) do not receive an award emoji because there is no specific note to react to.

Implementation uses a single reactions Map keyed by String(todo.id) that stores the target info, noteId, and the award Promise. onPromptStart fills in the award Promise; onPromptEnd deletes the entry and chains .then() on the Promise to remove the emoji. This naturally handles the race where onPromptEnd fires before the award API returns — the removal waits for the award to settle.

Also replaces the custom Todo interface with gitbeaker's TodoSchema (getting proper target_type enum 'Issue' | 'MergeRequest' | ... instead of string), introduces GitlabTarget to consolidate parsed target info, and simplifies processTodo/buildMetadata signatures by passing the parsed target directly instead of redundant targetType/threadId parameters.

Why it's needed

Part of #8117 (P0 item #4: Transient 👀 reaction / award emoji). The GitHub adapter already has this feature; the GitLab adapter needs parity. The transient emoji gives users visual feedback that the bot has accepted their mention and is working on it, and its removal signals completion.

Reviewer Test Plan

How to verify

  1. Configure a GitLab channel with senderPolicy: "open", groupPolicy: "open", and an action_prompt_template with a mentioned key.
  2. Start the channel: qwen channel start <name>
  3. On a GitLab issue, add a comment mentioning the bot (e.g., @bot-username do something).
  4. Observe: a 👀 award emoji appears on the comment within the poll interval.
  5. Wait for the bot to finish and post a reply.
  6. Observe: the 👀 emoji is removed from the comment after the reply is posted.
  7. Verify no duplicate replies are posted.
  8. Test a second mention on the same issue — emoji should appear and be removed again.

Unit tests cover: emoji creation, emoji removal, MR vs issue routing, description mention skip, award failure best-effort, remove failure best-effort, and pending award race handling.

Evidence (Before & After)

E2E test run against a live GitLab repo (zore3475/gl-channel-e2e-0730):

TEST 1: Comment mention → 👀 emoji → bot reply → 👀 removed
  ✅ PASS: 👀 created (award_id=52768310)
  ✅ PASS: Bot replied (note_id=3622127274)
  ✅ PASS: 👀 removed
  ✅ PASS: Exactly 1 bot reply

TEST 2: Second mention on same issue
  ✅ PASS: 👀 created for second mention
  ✅ PASS: Bot replied to second mention
  ✅ PASS: Exactly 2 total bot replies

7/7 passed

Tested on

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux

Environment

Local qwen channel start my-gitlab with coding plan model (qwen3-coder-plus), poll interval 15s.

Risk & Scope

  • Main risk or tradeoff: Award emoji API calls are best-effort. If the bot's PAT lacks the api scope needed for award emoji endpoints, the emoji silently fails but the agent still processes the mention and posts a reply. The api scope is already documented as required for posting notes.
  • Not validated / out of scope: Confidential note handling (documented as a known limitation — the adapter cannot filter confidential notes). Self-allowlist startup validation (feat(gitlab-channel): align GitLab adapter with GitHub adapter features #8117 item report error when try to auth #8) is not addressed in this PR.
  • Breaking changes / migration notes: None. The messageId format for GitLab envelopes remains String(todo.id).

Linked Issues

Part of #8117 (item 4: Transient 👀 reaction / award emoji).

中文翻译

本 PR 做了什么

为 GitLab 评论 mention 添加临时 👀 award emoji,在 agent 工作期间显示,完成后移除。镜像 GitHub 适配器的 eyes reaction(#8061)。当 onPromptStart 在 note mention 时触发,channel 调用 GitLab award emoji API 在触发评论上添加 👀。当 onPromptEnd 触发(完成、失败或取消)时,emoji 被移除。两个操作都是 best-effort:API 错误或权限失败记录到 stderr,不会阻塞 agent 的响应。

描述 mention(target_url 中没有 #note_ 锚点)不会收到 award emoji,因为没有具体的评论可以添加 reaction。

实现使用单个 reactions Map,以 String(todo.id) 为 key,存储 target info、noteId 和 award Promise。onPromptStart 填充 award Promise;onPromptEnd 删除 entry 并在 Promise 上链式调用 .then() 来移除 emoji。这自然处理了 onPromptEnd 在 award API 返回之前触发的竞态——移除操作会等待 award 完成。

同时将自定义 Todo 接口替换为 gitbeaker 的 TodoSchema(获得正确的 target_type 枚举 'Issue' | 'MergeRequest' | ... 而非 string),引入 GitlabTarget 整合解析后的 target 信息,并简化了 processTodo/buildMetadata 的签名,直接传递解析后的 target 而非冗余的 targetType/threadId 参数。

为什么需要这个

属于 #8117(P0 第 4 项:临时 👀 reaction / award emoji)。GitHub 适配器已有此功能;GitLab 适配器需要对齐。临时 emoji 给用户视觉反馈,表示 bot 已接受 mention 并正在处理,移除表示完成。

Reviewer 测试计划

验证方法

  1. 配置 GitLab channel:senderPolicy: "open"groupPolicy: "open"action_prompt_template 包含 mentioned key。
  2. 启动 channel:qwen channel start <name>
  3. 在 GitLab issue 上添加评论 mention bot(如 @bot-username do something)。
  4. 观察:在 poll 间隔内,评论上出现 👀 award emoji。
  5. 等待 bot 完成并发布回复。
  6. 观察:回复发布后,评论上的 👀 emoji 被移除。
  7. 验证没有重复回复。
  8. 在同一 issue 上测试第二次 mention——emoji 应再次出现并被移除。

单元测试覆盖:emoji 创建、emoji 移除、MR vs issue 路由、description mention 跳过、award 失败 best-effort、remove 失败 best-effort、pending award 竞态处理。

证据(Before & After)

针对真实 GitLab 仓库(zore3475/gl-channel-e2e-0730)的 E2E 测试运行:

TEST 1: Comment mention → 👀 emoji → bot reply → 👀 removed
  ✅ PASS: 👀 created (award_id=52768310)
  ✅ PASS: Bot replied (note_id=3622127274)
  ✅ PASS: 👀 removed
  ✅ PASS: Exactly 1 bot reply

TEST 2: Second mention on same issue
  ✅ PASS: 👀 created for second mention
  ✅ PASS: Bot replied to second mention
  ✅ PASS: Exactly 2 total bot replies

7/7 passed

测试环境

OS 状态
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux

运行环境

本地 qwen channel start my-gitlab,使用 coding plan 模型(qwen3-coder-plus),poll 间隔 15 秒。

风险与范围

  • 主要风险或取舍:Award emoji API 调用是 best-effort。如果 bot 的 PAT 缺少 award emoji 端点所需的 api scope,emoji 会静默失败,但 agent 仍会处理 mention 并发布回复。api scope 已在文档中标注为发布评论的必要条件。
  • 未验证 / 超出范围:Confidential note 处理(已作为已知限制记录——适配器无法过滤 confidential notes)。Self-allowlist 启动校验(feat(gitlab-channel): align GitLab adapter with GitHub adapter features #8117 第 8 项)不在本 PR 范围内。
  • 破坏性变更 / 迁移说明:无。GitLab envelope 的 messageId 格式保持 String(todo.id) 不变。

关联 Issue

属于 #8117(第 4 项:临时 👀 reaction / award emoji)。

Adds a working-reaction feature to the GitLab channel adapter, mirroring
the GitHub adapter's eyes reaction (PR QwenLM#8061). When the agent starts
processing a note mention, a 👀 award emoji is added to the note; it is
removed when the run completes, fails, or is cancelled. Both operations
are best-effort and never block the response.

Also replaces the custom Todo interface with gitbeaker's TodoSchema,
introduces GitlabTarget to consolidate target info, and simplifies
processTodo/buildMetadata signatures by passing the parsed target
directly instead of redundant targetType/threadId parameters.

Co-Authored-By: Qwen Code <noreply@alibaba.com>
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: Feature parity — the GitHub adapter already ships eyes reactions (#8061), and this brings the GitLab adapter to the same level. Tracked as part of #8117 (P0 item #4). The need is concrete, not theoretical.

Direction: Aligned. CHANGELOG has both the GitLab polling channel adapter (#7862, same author) and the GitHub eyes reaction feature (#8061) — this is the natural intersection of those two areas.

Size: Not applicable — no core paths touched. Production code is 102 additions / 33 deletions in a single adapter file, plus 302 lines of tests and 6 lines of docs.

Approach: The scope feels right — one adapter file, one test file, one docs page. The TodoTodoSchema replacement and GitlabTarget consolidation are in-service of the feature (the award emoji needs the parsed note ID and target type), not drive-by refactors. Both previous review findings (the missing !todo.target guard and the reactions map leak) have been addressed in this revision — (todo.target || {}) handles the null case, and the finally block ensures cleanup on all paths.

Risk: No elevated risk signals — no high-risk paths matched.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:功能对齐——GitHub 适配器已有 eyes reaction(#8061),本 PR 让 GitLab 适配器达到同等水平。属于 #8117(P0 第 4 项)。需求具体,非理论性加固。

方向:对齐。CHANGELOG 中同时包含 GitLab 轮询 channel 适配器(#7862,同一作者)和 GitHub eyes reaction 功能(#8061)——本 PR 是这两个领域的自然交叉。

规模:不适用——未触及核心路径。生产代码为单个适配器文件中 102 行新增 / 33 行删除,另有 302 行测试和 6 行文档。

方案:范围合理——一个适配器文件、一个测试文件、一个文档页面。TodoTodoSchema 替换和 GitlabTarget 整合是为功能服务的(award emoji 需要解析后的 note ID 和 target 类型),不是顺手重构。上一轮 review 的两个发现(缺失的 !todo.target 守卫和 reactions map 泄漏)已在本版本中修复——(todo.target || {}) 处理了 null 情况,finally 块确保所有路径上的清理。

风险:无升级风险信号——未匹配高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at ffe38aca33a7d1cb81a3bf14190700348d586763 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Clean implementation — both findings from the previous review round are fixed, and no new issues surfaced.

The (todo.target || {}) guard restores the null-safety the original code had: if GitLab returns a todo without a target (inaccessible or confidential target, API shape drift), raw.iid is undefined and the todo is skipped cleanly instead of throwing a TypeError that would wedge the poller. The finally block around handleInbound closes the reactions map leak — preflight denials, slash commands, and other early-return paths no longer leave orphaned entries.

The core design is unchanged from the previous round and remains sound: onPromptStart fills in the award promise, onPromptEnd deletes the entry and chains removal onto it. The race where onPromptEnd fires before the award API returns is handled correctly — the removal waits for the award to settle. If the award fails, the catch handler clears entry.award so no dangling remove call is made.

The TodoSchema replacement, GitlabTarget introduction, and fetchDescription cast removals are all in-service of the feature and earn their place in the diff.

No correctness bugs, security issues, or AGENTS.md violations found.

Testing

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success

All CI checks green on ffe38aca. The unit tests (9 cases in the working reaction describe block) pin the award/remove logic comprehensively — creation, removal, MR routing, description skip, double-award prevention, both failure modes, and the pending-award race. Removing the reaction code would fail them.

The actual GitLab API integration (award emoji appearing on a real note) requires a live GitLab instance with a PAT and is not testable in CI or in a sandboxed lane. The author reports a 7/7 E2E pass against zore3475/gl-channel-e2e-0730 — that is the author's claim, not independently re-run. The unit tests substantiate the logic; the API-level integration rests on the author's evidence.

中文说明

代码审查

实现干净——上一轮 review 的两个发现均已修复,未出现新问题。

(todo.target || {}) 守卫恢复了原代码的空值安全性:如果 GitLab 返回没有 target 的 todo(不可访问或机密 target、API 结构漂移),raw.iidundefined,todo 被正常跳过,而非抛出 TypeError 导致轮询器卡死。handleInbound 周围的 finally 块修复了 reactions map 泄漏——预检拒绝、slash 命令等提前返回路径不再留下孤立条目。

核心设计与上一轮相同,依然合理:onPromptStart 填充 award promise,onPromptEnd 删除条目并在其上链式调用移除。onPromptEnd 在 award API 返回前触发的竞态被正确处理——移除操作等待 award 完成。如果 award 失败,catch 处理器清除 entry.award,不会发出悬挂的 remove 调用。

TodoSchema 替换、GitlabTarget 引入和 fetchDescription 类型转换移除均为功能服务,在 diff 中各有其价值。

未发现正确性 bug、安全问题或 AGENTS.md 违规。

测试

所有 CI 检查在 ffe38aca 上通过。单元测试(working reaction 下 9 个用例)全面固定了 award/remove 逻辑——创建、移除、MR 路由、description 跳过、防止重复 award、两种失败模式和 pending-award 竞态。移除 reaction 代码会导致测试失败。

实际 GitLab API 集成(award emoji 出现在真实评论上)需要带 PAT 的真实 GitLab 实例,无法在 CI 或沙箱中测试。作者报告在 zore3475/gl-channel-e2e-0730 上 7/7 E2E 通过——这是作者的声明,非独立重跑。单元测试证实了逻辑;API 层面的集成依赖于作者的证据。

Qwen Code · qwen3.8-max-preview

Reviewed at ffe38aca33a7d1cb81a3bf14190700348d586763 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — both previous findings fixed cleanly, CI green, would merge without hesitation.

The two issues from the last round are resolved in exactly the right way: (todo.target || {}) is the minimal null-guard that preserves the original safety without adding a separate if, and the finally block is the simplest correct cleanup — it subsumes the old catch/rethrow and covers every early-return path handleInbound has. Neither fix adds complexity.

The promise-chaining approach for the create/remove race remains cleaner than the GitHub adapter's reactionsPendingRemoval set — one less piece of state, same correctness. Every change in the diff earns its place, the test suite is thorough (9 cases, including the race and both failure modes), and the docs update is concise.

中文说明

置信度:5/5 —— 上一轮两个发现均已干净修复,CI 全绿,毫不犹豫地合并。

上一轮的两个问题以恰当的方式解决:(todo.target || {}) 是保留原始安全性的最小空值守卫,无需添加单独的 iffinally 块是最简单的正确清理方式——它包含了旧的 catch/rethrow 并覆盖 handleInbound 的所有提前返回路径。两个修复都没有增加复杂度。

promise 链处理创建/移除竞态的方式依然比 GitHub 适配器的 reactionsPendingRemoval 集合更简洁——少一个状态,同样正确。diff 中每个改动都有其价值,测试套件全面(9 个用例,包括竞态和两种失败模式),文档更新简洁。

Qwen Code · qwen3.8-max-preview

Reviewed at ffe38aca33a7d1cb81a3bf14190700348d586763 · re-run with @qwen-code /triage

@wenshao

wenshao commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Review

Overview

Adds GitHub-parity transient 👀 feedback to the GitLab adapter: onPromptStart awards eyes on the triggering note, onPromptEnd removes it, both best-effort. Alongside that, it swaps the hand-rolled Todo interface for gitbeaker's TodoSchema, introduces GitlabTarget, and collapses the redundant targetType/threadId parameters out of processTodo/buildMetadata.

The core design is sound, and the pending-award race handling (storing the award promise and chaining removal onto it) is genuinely correct — onPromptEnd firing before the award API returns still removes the emoji. Docs are updated, the signature cleanup is a real readability win, and dropping three as { description?: string } casts is nice. tsc --noEmit is clean and all 41 unit tests pass on the branch.

Two defects in the refactor are worth fixing before merge. I verified both with a scratch test against this branch and against main.


Critical — dropping the !todo.target guard can permanently wedge the poller

The old guard chain checked !todo.target || !todo.target.iid. The new code checks only target_type, then dereferences:

const raw = todo.target as { iid?: number; title?: string };
if (!raw.iid) {          // throws if todo.target is null/undefined

TodoSchema declares target: Record<string, unknown> (non-nullable), but that is a compile-time claim about a runtime API payload, not a guarantee — which is exactly why the original guard existed. If GitLab ever returns an Issue/MergeRequest todo without target (inaccessible or confidential target, API shape drift), raw.iid throws TypeError.

That throw is outside the per-todo try (which wraps only processTodo), so it escapes pollOncePollingChannelBase.runLoop skips saveCursor(), the todo is never marked done, and the in-memory cursor never advances past it. Every subsequent poll re-fetches the same todo and re-crashes, backing off to MAX_BACKOFF. The channel stops processing all newer mentions, permanently, until restart — and on restart the unsaved cursor replays already-answered todos.

Verified: a todo with target: undefined followed by a healthy todo is skipped cleanly on main (cursor advances to the second todo), and throws TypeError: Cannot read properties of undefined (reading 'iid') on this branch.

const raw = todo.target as { iid?: number; title?: string } | undefined;
if (!raw?.iid) {
  await this.skipTodo(todo);
  continue;
}

High — reactions entries leak on every non-throwing early return

The entry is seeded before handleInbound and removed only by onPromptEnd, or on a thrown error:

if (isNoteMention) { this.reactions.set(messageId, { target, noteId: ... }); }
try { await this.handleInbound(envelope); }
catch (err) { this.reactions.delete(messageId); throw err; }

But handleInbound returns normally — no throw, no onPromptStart, no onPromptEnd — on several paths: preflight denial (sender_denied, group gating, pairing required), channel-memory intents, recognized slash commands, the ! shell refusal in a group, and stale queued-turn drops. In each case the Map entry survives forever. For a long-running gateway with an allowlist, every mention from a non-allowed user permanently adds an entry — unbounded growth with no eviction path.

Verified: with senderPolicy: 'allowlist' and a non-allowed author, reactions.size === 1 after pollOnce returns (and no award was attempted, as intended).

The GitHub adapter sidesteps this by building all reaction state inside onPromptStart; GitLab can't do that directly since only processTodo knows the noteId. The simplest correct fix here: handleInbound resolves only after the turn's finally has run (ChannelBase awaits the session-queue chain), so onPromptEnd has already claimed and deleted the entry by then — a finally is therefore both sufficient and race-free, and it subsumes the current catch/rethrow:

try {
  await this.handleInbound(envelope);
} finally {
  this.reactions.delete(messageId);
}

I applied both fixes locally: tsc --noEmit clean, all 41 existing tests still pass (including waits for pending award before removing), and the three probe cases go green.

One tradeoff to be aware of: under dispatchMode: 'collect', handleInbound returns as soon as the message is buffered, so the finally would drop the entry before the coalesced drain re-enters with that same messageId — the drained turn would get no 👀. Today the stale entry makes it work by accident. Given steer is the default and the buffered non-last messages leak either way, I'd take the finally and, if you want collect-mode coverage, clean up via the onPromptBuffered/onPromptBufferDropped hooks rather than relying on the leak.

Minor

  • Misleading duplicate error log on award failure. When the award rejects, onPromptStart's catch logs failed to acknowledge note N, and then onPromptEnd's .catch logs failed to remove acknowledgement from note N for the same rejection — even though api.remove was never called. Both lines appear in the current test run output. Having the award chain resolve to { awardId } | null (null on failure) lets onPromptEnd skip removal without a second, wrong error line.

  • Test wiring gap. The new tests seed reactions by reflection and never exercise processTodoreactions.set, so nothing covers the #note_(\d+)$ extraction actually landing in the map. In particular, does not acknowledge a description mention only asserts that an absent entry produces no award — it would pass unchanged even if processTodo wrongly seeded an entry for description mentions. One test driving pollOnce through the real handleInbound would close the gap cheaply and would have caught both issues above.

  • noteMatch![1] — the non-null assertion is only needed because the branch keys off the derived isNoteMention boolean instead of noteMatch. Branching on if (noteMatch) keeps the narrowing and drops the !.

  • LiveGitlabChannel repeats a ~12-line structural cast for reactions inline; hoisting the entry shape to a type alias in the test file would trim it. title: '' in the seeded targets is unused by these paths.

  • In-flight 👀 emojis are not cleaned up on disconnect(), so stopping the channel mid-run leaves one stuck on the note. Same as the GitHub adapter, so this is parity rather than a new gap — but worth a line under Known Limitations.

  • The as TodoSchema[] cast is legitimate, not redundant — removing it surfaces the TodoSchema | Camelize<TodoSchema> union from gitbeaker's return type. Fine as-is.

Security & performance

No new surface. The emoji content is a constant, API arguments are numeric ids plus the already-trusted chatId, and log interpolation is limited to numbers and the error object. Award emoji endpoints need the api scope already required for posting notes, and the best-effort semantics mean a scope shortfall degrades to no emoji rather than a dropped reply. Two extra API calls per note mention, both void-detached off the response path.


Verdict: approve after the two pollOnce/processTodo fixes. The award/remove lifecycle itself is well built and the race handling is better than it needed to be.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

Restore null guard for todo.target (dropped in the TodoSchema refactor)
to prevent TypeError when GitLab returns a todo without target. Use
try/finally instead of try/catch for reactions cleanup so entries are
removed on all handleInbound return paths, not just throws.

Co-Authored-By: Qwen Code <noreply@alibaba.com>

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

中文说明

已审查。 建议见行内评论。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +347 to +352
if (isNoteMention) {
this.reactions.set(messageId, {
target,
noteId: Number(noteMatch![1]),
});
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The noteId parsed from target_url (noteMatch![1]) and the target seeded into the reactions map here are never verified by any test. Every reaction test injects the map entry manually via setReactionForTest with a hardcoded noteId, and no pollOnce-driven test asserts the award call receives the parsed value — Concrete cost: the capture group noteMatch![1] is new in this diff (the old code was a non-capturing .test()), so a regression such as noteMatch![0] (→ Number('#note_1001') = NaN) would make every real note mention call award(chatId, iid, NaN, 'eyes'), which fails best-effort and is only logged — the 👀 reaction would silently never appear for any real user, and the mutant ships green.

Suggested fix — add one pollOnce-driven test that runs a todo whose target_url ends in #note_1001 through the real processTodo path and pins the parsed value:

// drive pollOnce with a fixture target_url ending in '#note_1001',
// trigger onPromptStart via the bridge mock, then assert:
expect(mockApi.IssueNoteAwardEmojis.award).toHaveBeenCalledWith(
  'owner/repo',
  42,
  1001, // the parsed noteId, not an injected one
  'eyes',
);
中文说明

这里从 target_url 解析出的 noteIdnoteMatch![1])以及写入 reactions map 的 target,没有任何测试覆盖。所有 reaction 测试都通过 setReactionForTest 手动注入 map entry(使用硬编码的 noteId),没有 pollOnce 驱动的测试断言 award 调用收到的是解析出来的值 —— 具体代价:捕获组 noteMatch![1] 是本 diff 新增的(旧代码是非捕获的 .test()),因此像 noteMatch![0](→ Number('#note_1001') = NaN)这样的回归会让每个真实 note mention 调用 award(chatId, iid, NaN, 'eyes'),它以 best-effort 失败且只记录日志 —— 👀 reaction 对所有真实用户都会静默不出现,而这个 mutant 会绿灯通过。

建议修复 —— 新增一个 pollOnce 驱动的测试,让 target_url#note_1001 结尾的 todo 走真实的 processTodo 路径,并断言 award 收到 1001(解析值)而非注入值(见上方代码示例)。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +171 to +175
.catch((err) => {
process.stderr.write(
`[Channel:${this.name}] failed to remove acknowledgement from note ${entry.noteId}: ${err}\n`,
);
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This .catch() terminates the whole entry.award.then(remove) chain, so it catches rejections from BOTH a rejected award promise and a rejected remove call under the single message "failed to remove acknowledgement" — Concrete cost: when the award API is slow and the prompt finishes first, onPromptEnd chains on the still-pending award; if the award then fails (e.g. 403 missing scope), this logs "failed to remove acknowledgement from note N: Error: 403" even though no removal was ever attempted, while onPromptStart's catch already logged the real cause ("failed to acknowledge note N"). An oncall engineer seeing the second line investigates the wrong API endpoint.

Suggested fix — scope the removal error handler to only the api.remove() call so the award rejection is absorbed here (it is already logged above):

void entry.award.then(
  ({ awardId }) =>
    api.remove(chatId, entry.target.iid, entry.noteId, awardId).catch((err) => {
      process.stderr.write(
        `[Channel:${this.name}] failed to remove acknowledgement from note ${entry.noteId}: ${err}\n`,
      );
    }),
  () => {
    // Award failure — already logged by onPromptStart's catch handler.
  },
);
中文说明

这个 .catch() 终结了整条 entry.award.then(remove) 链,因此它会同时捕获两种失败 —— award promise 被拒绝和 remove 调用被拒绝 —— 并都用同一条 "failed to remove acknowledgement" 消息输出 —— 具体代价:当 award API 较慢、prompt 先完成时,onPromptEnd 会在仍未 settle 的 award 上挂链;如果随后 award 失败(例如 403 缺少 scope),这里会输出 "failed to remove acknowledgement from note N: Error: 403",但实际上根本没有尝试过 remove,而 onPromptStart 的 catch 已经记录了真正的原因("failed to acknowledge note N")。凌晨值班工程师看到第二行会去排查错误的 API 端点。

建议修复 —— 把 remove 的错误处理限定在 api.remove() 调用本身,使 award 的拒绝在这里被静默吸收(它已在上方记录,见上方代码示例)。

— qwen3.8-max-preview via Qwen Code /review

@wenshao

wenshao commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Review: transient 👀 award emoji for GitLab note mentions

Reviewed at cf52fa72c. No Criticals. The feature is correct end-to-end and the TodoSchema migration is a real improvement. Everything below is Suggestion/Nit level, concentrated in the tests.

What it does

Mirrors the GitHub adapter's eyes reaction (#8061) for GitLab: processTodo registers a reactions entry (target + parsed noteId) keyed by String(todo.id) when the mention came from a note; onPromptStart awards eyes via IssueNoteAwardEmojis/MergeRequestNoteAwardEmojis and stashes the award promise; onPromptEnd deletes the entry and chains on that promise to remove the award. Both legs are fire-and-forget with stderr logging. Also swaps the hand-rolled Todo interface for gitbeaker's TodoSchema, introduces GitlabTarget, and drops redundant targetType/threadId parameters.

What I verified

  • npx vitest run in packages/channels/gitlab — 55/55 pass.
  • tsc --noEmit -p packages/channels/gitlab — clean.
  • eslint on both changed source files — clean.
  • Checked both gitbeaker signatures against @gitbeaker/core: ResourceNoteAwardEmojis.award(projectId, resourceIId, noteId, name) and .remove(projectId, resourceIId, noteId, awardId) — argument order and arity match. Worth stating explicitly because the tests mock the API surface, so a wrong resource name or argument order would pass the suite; tsc is what actually covers it here, and it's green.
  • Traced the lifecycle in ChannelBase: handleInbound awaits current (ChannelBase.ts:5684) and onPromptEnd runs inside that turn's finally (ChannelBase.ts:5623), so the finally { this.reactions.delete(messageId) } in processTodo always runs after onPromptEnd on the normal path — the cleanup guard doesn't race the feature. The /clear-eviction path also passes messageId (ChannelBase.ts:2884), so a wedged turn still gets its emoji recalled. Both good.
  • Wrote a throwaway integration test driving the real pollOnceonPromptStart/onPromptEnd path (details under Suggestion 1) and confirmed the award/remove calls land with the right arguments. The wiring is correct; the gap is only that nothing in the committed suite proves it.

Suggestions

1. All 7 new tests inject private state, so the actual wiring is untested. Every test calls setReactionForTest(...) to populate reactions by hand, which means the #note_(\d+)$ parse (GitlabAdapter.ts:295), the agreement between the envelope's messageId and the reactions key, and the finally cleanup are all outside coverage. A regression that broke, say, the key derivation or the if (isNoteMention) guard would leave the suite green. The file already has TestableGitlabChannel for pollOnce-driven tests; a sibling that drives the real hooks is ~10 lines, and I confirmed it passes as-is against this branch:

class ReactingGitlabChannel extends GitlabChannel {
  override async handleInbound(envelope: Envelope): Promise<void> {
    this.onPromptStart(envelope.chatId, 'session-1', envelope.messageId);
    await Promise.resolve();
    this.onPromptEnd(envelope.chatId, 'session-1', envelope.messageId);
  }
  protected override startPollLoop(): void {}
}

Driving pollOnce() with makeTodo() then asserting award('owner/repo', 42, 1001, 'eyes') covers the parse, the key agreement, and MR routing in one go.

2. does not acknowledge a description mention (test:720) doesn't test a description mention. It never sets a reactions entry, so it asserts "no entry → no award" — a tautology given the if (!entry) return guard two lines into onPromptStart. The behavior it claims to cover is the if (isNoteMention) guard at GitlabAdapter.ts:347, which is exactly what the harness above reaches with target_url lacking the #note_ anchor. Either rename it to match what it checks or point it at the real path.

3. handles award failure as best-effort (test:785) sequences on a bare microtask count. Two await Promise.resolve() calls (test:802-803) are what make entry.award already undefined by the time endPromptForTest runs, which is what makes the remove not-called assertion hold. Add one .then link to the implementation chain and this test either passes for the wrong reason or flakes. vi.waitFor is already used elsewhere in the file and expresses the intent without depending on tick counts.

Nits

  • GitlabAdapter.ts:341String(todo.id) is recomputed one line after messageId is passed at :334. Use messageId for both so the envelope's messageId and the metadata's todo_id can't drift.

  • GitlabAdapter.ts:350noteMatch![1] needs a non-null assertion only because the truthiness lives in the separate isNoteMention const. if (noteMatch) { this.reactions.set(...) } drops the assertion and reads the same.

  • GitlabAdapter.ts:220(todo.target || {}) as { iid?: number; title?: string } is the one spot where the TodoSchema swap makes types weaker: TodoSchema.target is Record<string, unknown>, so this cast is unchecked and a non-numeric iid would flow into award()/remove() typed as number. The old hand-rolled interface had the same hole, so this isn't a regression — but since you're already touching it, typeof raw.iid === 'number' in the guard would close it for free.

  • Misleading log on a slow-failing award. If onPromptEnd fires while the award is still in flight and the award then rejects, the chain at GitlabAdapter.ts:167-175 logs failed to remove acknowledgement from note … for an emoji that was never added — on top of the failed to acknowledge line onPromptStart already wrote. Narrow (needs a fast cancel racing a failing award), purely cosmetic, but avoidable by attaching the log to the remove() call rather than the whole chain:

    void entry.award
      .then(({ awardId }) =>
        api.remove(chatId, entry.target.iid, entry.noteId, awardId).catch((err) => {
          process.stderr.write(`[Channel:${this.name}] failed to remove acknowledgement from note ${entry.noteId}: ${err}\n`);
        }),
      )
      .catch(() => {
        // Award failure already logged by onPromptStart.
      });

    The trailing .catch is needed: the derived promise would otherwise be an unhandled rejection.

  • GitlabAdapter.test.ts:658-696LiveGitlabChannel re-declares the private entry shape inside a cast, and every test does connect() then an immediate disconnect() purely to stop the poll loop connect started. Overriding startPollLoop to a no-op (as TestableGitlabChannel already does) removes the dance and the unexplained ritual from all 7 tests.

  • onPromptStart/onPromptEnd have no doc comment, where the GitHub equivalent explains the best-effort/non-blocking contract. Comments default to none in this codebase, so take it or leave it — but the two-hook, promise-handoff contract is the non-obvious kind.

Informational, no action expected

  • Buffered turns silently lose the emoji. With a non-default dispatchMode: "collect", a mention arriving while another prompt owns the session is buffered and handleInbound returns early (ChannelBase.ts:5170-5179); processTodo's finally then drops the reactions entry, and the drained coalesced turn re-enters with only the last envelope's messageId (ChannelBase.ts:5643-5677), by which point the entry is gone. So: no 👀 for those mentions. Cosmetic, and narrow — pollOnce awaits each dispatch, so it needs a concurrent prompt from another source (a /loop job) to trigger. Structural difference from the GitHub adapter, which is immune because it derives owner/repo/comment_id from chatId + messageId alone; GitLab can't, since messageId is a todo id rather than a note id, hence the side map. Not worth restructuring for.
  • A hard channel stop leaves the emoji. No disconnect() cleanup for in-flight awards, so qwen channel stop mid-run strands a 👀. The new doc paragraph says it's removed on "completes, fails, or is cancelled", which doesn't cover this. Same in the GitHub adapter, so it's a parity-preserving gap, not a PR defect — possibly a Known Limitations line.
  • Re-awarding is not idempotent on GitLab. GitHub's createForIssueComment returns the existing reaction, so a restart mid-run can still recover the id and clean up; GitLab's award endpoint rejects a duplicate, so a stranded emoji on the same note stays stranded. Only reachable if the same note re-triggers a todo, which normally doesn't happen (a re-mention is a new note).

Other notes

  • Docs read well and mirror github.md's ## Response Feedback section, including the same placement before ## Known Limitations. The description-mention carve-out is documented where a reader will look for it.
  • Performance is fine: two extra API calls per note mention, both fire-and-forget off the response path. Unlike the GitHub adapter these aren't wrapped in a retrying helper, but the GitLab adapter has no such wrapper to begin with, and best-effort is the right contract here.
  • Nothing security-relevant: noteId is digits-only out of a server-provided target_url, no new sink, and the award endpoints need the api scope already required for posting notes.

…uencing

Add ReactingGitlabChannel that drives real pollOnce → handleInbound →
onPromptStart/onPromptEnd path, covering #note_ parse, key derivation,
and finally cleanup. Replace the tautological description-mention test
with one that exercises the real isNoteMention guard. Replace bare
Promise.resolve() microtask waits with vi.waitFor in the award-failure
test.

Co-Authored-By: Qwen Code <noreply@alibaba.com>

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +138 to +139
const entry = this.reactions.get(messageId);
if (!entry || entry.award) return;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The entry.award truthy branch of this double-call guard is never exercised by any test — removing || entry.award is a surviving mutation. The guard is correct today, but a future regression deleting it would ship undetected.

Failure scenario: if onPromptStart fires twice for the same messageId (a duplicate lifecycle event or a future retry), the second call overwrites entry.award; onPromptEnd then removes only the second emoji and the first 👀 stays on the note permanently. No test calls startPromptForTest twice on the same messageId to pin the guard down.

Suggested fix — add a test in the working reaction block:

it('does not award twice when onPromptStart is called again', async () => {
  // seed a reaction, then call startPromptForTest twice on the same messageId
  expect(mockApi.IssueNoteAwardEmojis.award).toHaveBeenCalledTimes(1);
});
中文说明

这个双重调用守卫中 entry.award 为真的分支没有任何测试覆盖——移除 || entry.award 是一个能存活的变异。守卫目前是正确的,但未来若误删它将不会被检测到。

失败场景:如果 onPromptStart 对同一个 messageId 触发两次(重复的生命周期事件或未来的重试),第二次调用会覆盖 entry.awardonPromptEnd 随后只移除第二个 emoji,第一个 👀 会永久留在评论上。建议修复:在 working reaction 测试块中新增一个测试,对同一 messageId 调用两次 startPromptForTest,并断言 IssueNoteAwardEmojis.award 只被调用一次。

— qwen3.8-max-preview via Qwen Code /review

Add test that calls startPromptForTest twice on the same messageId and
asserts award is called exactly once, pinning the `|| entry.award`
guard in onPromptStart against surviving mutations.

Co-Authored-By: Qwen Code <noreply@alibaba.com>

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— qwen3.8-max-preview via Qwen Code /review

@wenshao

wenshao commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Runtime verification report — built and executed locally

I verified this PR by running the real GitlabChannel against a real HTTP GitLab API server, rather than by reading the diff or trusting the unit suite. Nothing in the channel or in channel-base was stubbed: the production poll loop → pollOncehandleInboundChannelBase prompt lifecycle → onPromptStart/onPromptEnd → a real @gitbeaker/rest client → HTTP all ran unmodified, and every request was captured on the wire.

This matters because the PR's own tests replace @gitbeaker/rest with vi.fn() objects, so they can confirm which method was called with which arguments but cannot confirm that those arguments produce the right URL, HTTP verb or resource on a real GitLab. The award-emoji endpoints are exactly where that distinction bites.

Verdict: the feature works as described. I recommend merging. One test-coverage gap and two pre-existing limitations are noted below; none of them block.

Setup

PR head ffe38ac (4 commits)
Compared against 84bb09b (merge base)
Method two tsc --build artifacts, driven by one harness; A/B by arm
Result PR 34/34 checks — base 8/17 (base has no award-emoji traffic at all)
Also run PR unit suite 57/57, tsc --build clean, eslint clean

1. The emoji lifecycle, observed on the wire

The left/right panels below are rendered from the mock GitLab server's actual state at two moments in a single run; the table underneath is the real HTTP request log the channel produced.

GitLab award emoji lifecycle

POST   /projects/owner%2Frepo/issues/42/notes/1001/award_emoji  name=eyes   201
POST   /projects/owner%2Frepo/issues/42/notes                               201   ← the agent's reply
DELETE /projects/owner%2Frepo/issues/42/notes/1001/award_emoji/90000        204

Confirmed on the wire: the endpoint, the HTTP verbs, name=eyes, that the 👀 lands before any reply is posted, and that the DELETE carries the award id the POST actually returned.

2. Scenario matrix and mutation testing

Scenario and mutation summary

All 12 scenarios pass on the PR. The interesting ones:

  • S6 — the race the PR claims to handle. With a deliberately slow award API (900 ms), the run finishes before the POST returns. No DELETE is issued while the award is in flight, and once it lands the deferred DELETE uses the id that POST returned. The .then() chaining does what the description says.
  • S7 — failed turn. When the agent turn throws, the 👀 is still removed and the ⚠️ error note is still posted.
  • S4 / S5 — 403 on award and on remove. Both are genuinely best-effort: the agent's reply is delivered either way, only a stderr line is emitted, and no unhandled rejection escapes (process.on('unhandledRejection') counter stayed at 0).
  • S2 — description mentions get no award emoji, as documented.
  • S3 — MR notes route to /merge_requests/{iid}/notes/{id}/award_emoji, not the issue path.

To prove the checks aren't vacuous, I broke the compiled PR code six ways and re-ran. Every mutant was killed by the intended check — see the second table in the image.

3. Finding: the PR's own leak fix is not pinned by any test

Commit cf52fa7 ("fix reactions leak") added the cleanup in processTodo:

try {
  await this.handleInbound(envelope);
} finally {
  this.reactions.delete(messageId);
}

I deleted that finally from src/GitlabAdapter.ts and re-ran the PR's suite: 57/57 still pass. It is the only one of six mutations the suite does not catch.

It is not dead code — it is load-bearing. When handleInbound returns without ever starting a prompt (sender/group gate rejection, a slash command, a memory intent — anything that returns early inside processInbound), onPromptEnd never fires, so the finally is the only thing that reclaims the map entry. My harness catches this as S10.2: with the finally removed, a sender-gated mention leaves reactions holding an entry forever, and every gated mention grows the map by one on a long-running channel.

Here is a test that pins it. I added it to GitlabAdapter.test.ts and ran it both ways: passes on the PR as-is (GitlabAdapter.test.ts 44/44), fails with expected 1 to be +0 once the finally is removed. Note it subclasses GitlabChannel directly rather than reusing TestableGitlabChannel — the gate only runs if the real handleInbound is left in place.

it('does not leak a reactions entry when the sender gate rejects the mention', async () => {
  class GatedGitlabChannel extends GitlabChannel {
    protected override startPollLoop(): void {}
  }
  const channel = new GatedGitlabChannel(
    'test-gitlab',
    makeConfig({ senderPolicy: 'allowlist', allowedUsers: ['bob'] }),
    makeBridge(),
  );
  await channel.connect();
  channel.disconnect();
  (
    channel as unknown as {
      cursor: { lastProcessedId: number; initialized: boolean };
    }
  ).cursor = { lastProcessedId: 0, initialized: true };
  mockApi.TodoLists.all.mockResolvedValueOnce([makeTodo()]);

  await (channel as unknown as { pollOnce: () => Promise<void> }).pollOnce();

  expect(
    (channel as unknown as { reactions: Map<string, unknown> }).reactions.size,
  ).toBe(0);
});

4. Undocumented behaviour change from the refactor (an improvement)

I diffed the exact prompt text the adapter builds, PR vs base, across four todo shapes. Everything is byte-identical except one case — a target that has an iid but no title:

- base: [owner/repo|…|alice|Issue|43|%title%|14||%|%bogus%]
+ PR:   [owner/repo|…|alice|Issue|43||14||%|%bogus%]

GitlabTarget.title is now raw.title ?? '', so a missing title resolves to an empty string instead of leaking the literal %title% placeholder into the model prompt. That's a genuine improvement, just not mentioned in the description.

5. Two limitations worth knowing (pre-existing, not introduced here, not blockers)

  • S11 — a second mention is not acknowledged while the first run is busy. pollOnce awaits each handleInbound and runLoop awaits each pollOnce, so polling is fully serial. In my run, mention Where is the config saved? #2 (a different issue) waited 712 ms for a deliberately short run pre-release: fix ci #1 to finish before getting its 👀. With a realistic multi-minute agent turn, the second commenter sees no acknowledgement for that whole time — which is the exact situation this feature exists to fix. Worth a follow-up in feat(gitlab-channel): align GitLab adapter with GitHub adapter features #8117, not a change to this PR.
  • S12 — a run interrupted by process exit leaves the 👀 behind. disconnect() does not recall the emoji; recall only happens when the in-flight turn settles. If the channel is stopped or the process is killed mid-run, the note keeps a stale 👀 indefinitely — cosmetic, since a re-mention creates a new note. The GitHub adapter has the same shape, so this is parity, not a regression.

6. Reproduction

git worktree add wt-pr8119 ffe38ac && git worktree add wt-base 84bb09b
# build packages/channels/{base,gitlab} in each with `tsc --build`
ARM=pr node driver.mjs     # 34/34
ARM=base node driver.mjs   #  8/17
bash mutate.sh             # 6 mutants, 6 killed

The harness is a ~500-line mock GitLab v4 server (/user, /todos, /todos/:id/mark_as_done, issue/MR show, notes, notes/:id/award_emoji) plus a scenario driver with failure and latency injection. Happy to push it somewhere if it's useful for the rest of #8117.

Full scenario output (34 checks)

Full scenario run


中文版

运行时验证报告 — 本地构建并实际执行

我没有只看 diff、也没有只信任单元测试,而是让真实的 GitlabChannel 跑在一个真实的 HTTP GitLab API 服务器上做验证。channel 和 channel-base 都没有做任何桩替换:生产环境的 poll loop → pollOncehandleInboundChannelBase prompt 生命周期 → onPromptStart/onPromptEnd → 真实的 @gitbeaker/rest 客户端 → HTTP,全链路原样执行,每一个请求都在网络层被捕获。

这一点很关键:本 PR 自带的测试把 @gitbeaker/rest 整体换成了 vi.fn() 对象,因此它们只能确认调用了哪个方法、传了哪些参数,无法确认这些参数在真实 GitLab 上会拼出正确的 URL、HTTP 动词和资源。而 award emoji 端点恰恰是这个区别会出问题的地方。

结论:功能符合描述,建议合并。 下面记录了一个测试覆盖缺口和两个既有限制,都不构成阻塞。

环境

PR head ffe38ac(4 个 commit)
对照基线 84bb09b(merge base)
方法 两份 tsc --build 产物,同一套 harness,按 arm 做 A/B
结果 PR 34/34 通过 — base 8/17(base 完全没有 award emoji 流量)
另外执行 PR 单元测试 57/57、tsc --build 无错、eslint 无告警

1. 网络层观测到的 emoji 生命周期

上图左右两栏是同一次运行中 mock GitLab 服务器的真实状态在两个时刻的渲染,下方表格是 channel 实际产生的 HTTP 请求日志。

POST   /projects/owner%2Frepo/issues/42/notes/1001/award_emoji  name=eyes   201
POST   /projects/owner%2Frepo/issues/42/notes                               201   ← agent 的回复
DELETE /projects/owner%2Frepo/issues/42/notes/1001/award_emoji/90000        204

在网络层确认:端点正确、HTTP 动词正确、name=eyes 正确、👀 先于任何回复出现,且 DELETE 使用的是 POST 实际返回的 award id。

2. 场景矩阵与变异测试

PR 上 12 个场景全部通过。其中比较有价值的几个:

  • S6 — PR 声称处理的竞态。 把 award API 人为拖慢到 900 ms,使 agent 运行先于 POST 返回结束。结果:award 在途期间不会发出 DELETE;award 返回后,延迟的 DELETE 使用了 POST 返回的真实 id。.then() 链的行为与描述一致。
  • S7 — 运行失败。 agent turn 抛异常时,👀 仍被移除,⚠️ 错误评论仍然发出。
  • S4 / S5 — award 和 remove 分别返回 403。 两者都确实是 best-effort:agent 回复照常送达,只往 stderr 打一行日志,没有 unhandled rejection 逃逸(process.on('unhandledRejection') 计数全程为 0)。
  • S2 — description mention 不加 award emoji,与文档一致。
  • S3 — MR 评论 路由到 /merge_requests/{iid}/notes/{id}/award_emoji,而非 issue 路径。

为了证明这些检查不是空转,我对编译后的 PR 代码做了 6 种定向破坏并重跑,每个变异体都被对应的检查杀死(见图中第二张表)。

3. 发现:PR 自己的 leak 修复没有被任何测试钉住

commit cf52fa7("fix reactions leak")在 processTodo 中加了这段清理:

try {
  await this.handleInbound(envelope);
} finally {
  this.reactions.delete(messageId);
}

我把这个 finallysrc/GitlabAdapter.ts 删掉后重跑 PR 的测试套件:57/57 仍然全绿。 这是 6 个变异中唯一没被单测抓住的一个。

它并不是冗余代码,而是有实际作用的。当 handleInbound 没有启动任何 prompt 就返回时(sender/group gate 拒绝、slash command、memory intent —— 任何在 processInbound 内部提前 return 的路径),onPromptEnd 根本不会触发,此时 finally 是唯一能回收 map 条目的地方。我的 harness 用 S10.2 抓到了这一点:删掉 finally 后,一个被 sender gate 拒绝的 mention 会让 reactions 永久持有一个条目;在长期运行的 channel 上,每一次被拒绝的 mention 都会让这个 map 增长一格。

下面是能钉住它的测试。我已把它加进 GitlabAdapter.test.ts 并双向跑过:在当前 PR 上通过(GitlabAdapter.test.ts 44/44),删掉 finally 后失败并报 expected 1 to be +0 注意它直接继承 GitlabChannel 而没有复用 TestableGitlabChannel —— 只有保留真实的 handleInbound,gate 才会执行。

it('does not leak a reactions entry when the sender gate rejects the mention', async () => {
  class GatedGitlabChannel extends GitlabChannel {
    protected override startPollLoop(): void {}
  }
  const channel = new GatedGitlabChannel(
    'test-gitlab',
    makeConfig({ senderPolicy: 'allowlist', allowedUsers: ['bob'] }),
    makeBridge(),
  );
  await channel.connect();
  channel.disconnect();
  (
    channel as unknown as {
      cursor: { lastProcessedId: number; initialized: boolean };
    }
  ).cursor = { lastProcessedId: 0, initialized: true };
  mockApi.TodoLists.all.mockResolvedValueOnce([makeTodo()]);

  await (channel as unknown as { pollOnce: () => Promise<void> }).pollOnce();

  expect(
    (channel as unknown as { reactions: Map<string, unknown> }).reactions.size,
  ).toBe(0);
});

4. 重构带来的、描述中未提及的行为变化(属于改进)

我对 PR 与 base 在四种 todo 形态下构造出的 prompt 文本做了逐字节 diff。除一种情况外完全一致 —— 即 target 有 iid 但没有 title 时:

- base: [owner/repo|…|alice|Issue|43|%title%|14||%|%bogus%]
+ PR:   [owner/repo|…|alice|Issue|43||14||%|%bogus%]

GitlabTarget.title 现在是 raw.title ?? '',因此缺失的 title 会解析成空字符串,而不再把字面量 %title% 占位符泄漏进模型 prompt。这是实打实的改进,只是描述里没写。

5. 两个值得知晓的限制(既有问题,非本 PR 引入,不阻塞)

  • S11 — 第一个运行还在进行时,第二个 mention 不会被确认。 pollOnce 会 await 每个 handleInboundrunLoop 又会 await 每个 pollOnce,因此轮询完全串行。在我的实测中,mention Where is the config saved? #2另一个 issue)等了 712 ms(等运行 pre-release: fix ci #1 这个刻意设短的 turn 结束)才拿到自己的 👀。如果 agent 实际跑几分钟,第二个评论者在这整段时间内看不到任何确认 —— 而这恰恰是本功能要解决的场景。建议作为 feat(gitlab-channel): align GitLab adapter with GitHub adapter features #8117 的后续项,不必在本 PR 内改动。
  • S12 — 运行中途进程退出会留下 👀。 disconnect() 不会主动撤回 emoji,撤回只发生在在途 turn 结束时。如果 channel 被停止或进程在运行中途被杀,该评论上会永久残留一个 👀 —— 属于观感问题,因为重新 mention 会产生一条新评论。GitHub adapter 是同样的结构,因此这属于行为对齐,而非回归。

6. 复现方式

git worktree add wt-pr8119 ffe38ac && git worktree add wt-base 84bb09b
# 在各自 worktree 中用 `tsc --build` 构建 packages/channels/{base,gitlab}
ARM=pr node driver.mjs     # 34/34
ARM=base node driver.mjs   #  8/17
bash mutate.sh             # 6 个变异体,6 个被杀死

harness 由一个约 500 行的 mock GitLab v4 服务器(/user/todos/todos/:id/mark_as_done、issue/MR shownotesnotes/:id/award_emoji)和一个带故障与延迟注入的场景驱动器组成。如果对 #8117 的后续工作有用,我可以把它推到某个地方共享。

@wenshao

wenshao commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Two follow-ups from the same harness, closing the loop on my review at cf52fa72c.

Both defects I raised are fixed and verified at runtime, not just by inspection:

  • The Critical (dropped !todo.target guard wedging the poller) — scenario S9: a todo with target: undefined followed by a healthy todo now skips cleanly, the cursor advances, and the second todo is processed normally. No TypeError escapes pollOnce.
  • The High (reactions leak on non-throwing early returns) — scenario S10.2: a sender-gated mention leaves reactions.size === 0. Removing the finally puts it back to 1, which is what the suggested test above pins.

Correction to my "misleading duplicate error log" minor — it is narrower than I described.

I claimed a failed award logs both failed to acknowledge note N and failed to remove acknowledgement from note N. Driving the real code against a 403ing award endpoint shows it depends entirely on when the rejection settles:

Award API behaviour stderr lines api.remove actually called
403 returns immediately 1 — only failed to acknowledge 0
403 returns after 800 ms (slower than the turn) 2 — plus a false failed to remove acknowledgement 0

The void entry.award.catch(...) handler sets entry.award = undefined, so when the rejection settles before the turn ends — the common case, since a permissions 403 comes back fast — onPromptEnd hits if (!entry.award) return and stays quiet. The spurious second line only appears when the award request outlives the agent turn, and it is genuinely wrong there: remove was never called (0 calls in both rows).

So the two log lines I saw were an artifact of the unit tests' tight sequencing rather than normal operation. Still worth the { awardId } | null cleanup I suggested, but it's cosmetic and rare — definitely not merge-blocking.

中文版

以下两点来自同一套 harness,用于收尾我在 cf52fa72c 上的那次 review。

我提的两个缺陷都已修复,并且是在运行时验证的,不只是看代码:

  • Critical(删掉 !todo.target 保护会卡死 poller)—— 场景 S9target: undefined 的 todo 后面跟一个正常 todo,现在能干净跳过,cursor 正常推进,第二个 todo 正常处理,没有 TypeError 逃出 pollOnce
  • High(非抛异常的提前 return 会泄漏 reactions)—— 场景 S10.2:被 sender gate 拒绝的 mention 之后 reactions.size === 0;删掉 finally 会变回 1,也就是上面那个建议测试要钉住的行为。

对我那条「重复错误日志」minor 的更正 —— 它的触发条件比我描述的窄。

我当时说 award 失败会同时打出 failed to acknowledge note Nfailed to remove acknowledgement from note N。让真实代码跑在一个返回 403 的 award 端点上后可以看到,这完全取决于 rejection 何时 settle:

award API 行为 stderr 行数 是否真的调用了 api.remove
403 立即返回 1 —— 只有 failed to acknowledge 0
403 延迟 800 ms 返回(慢于本次 turn) 2 —— 多出一条错误的 failed to remove acknowledgement 0

void entry.award.catch(...) 里会把 entry.award = undefined,所以当 rejection 在 turn 结束前 settle(常见情况,因为权限类 403 返回很快),onPromptEnd 会命中 if (!entry.award) return 而不打日志。那条多余的日志只在 award 请求比 agent turn 活得更久时出现,而且它确实是错的:remove 从未被调用(两行数据都是 0 次)。

所以我之前看到的两行日志其实是单测紧凑时序造成的假象,而非常态。我建议的 { awardId } | null 清理仍然值得做,但属于观感问题且触发概率很低,肯定不构成合并阻塞。

@wenshao

wenshao commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Jul 31, 2026
Merged via the queue into QwenLM:main with commit 153d781 Jul 31, 2026
61 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.3.

@QwenLM QwenLM deleted a comment Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants