feat(channels): Add channel agent bridge abstraction - #5978
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
requested review from
DragonnZhang,
chiga0,
qwen-code-ci-bot,
wenshao and
yiliang114
June 29, 2026 04:02
doudouOUC
marked this pull request as ready for review
June 29, 2026 04:02
Collaborator
|
Thanks for the PR! (Re-run after blocking issues resolved.) Template looks good ✓ On direction: this is a clean preparatory refactor for #5976 (daemon-managed channel workers). The channels area is actively developed — extracting On approach: the Scope is appropriate: 34 changed files for the abstraction + lifecycle fixes, no drive-by refactors. The previous ESLint Moving on to code review. 🔍 中文说明感谢贡献！（阻塞问题解决后的重新运行。） 模板完整 ✓ 方向：这是为 #5976（daemon 管理的 channel workers）做的准备工作。channels 区域一直在活跃开发——在添加第二种 bridge 实现之前先提取 方案： 范围合理：34 个文件改动覆盖抽象 + 生命周期修复，无顺带重构。之前的 ESLint 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Code Review (Re-run after blocking issues resolved)Independent proposal: extract a narrow interface from The PR matches this. Key review findings on the incremental changes since the last triage:
No critical blockers. The diff is focused — every file change is the type swap, the lifecycle fix, or review feedback. No scope creep. Reuse check: Test ResultsAll affected test suites pass at HEAD TypeScript CI status: Real-Scenario TestingThis PR is a type-level refactor + router lifecycle fixes with no user-visible TUI output. The previous triage run and the maintainer's detailed local verification report ( 中文说明代码审查 （阻塞问题解决后的重新运行）独立方案：从 PR 方案与此一致。自上次 triage 以来的增量变更关键发现：
无关键阻塞。 diff 聚焦——每个文件改动都是类型替换、生命周期修复或 review feedback。无范围蔓延。 复用检查： 测试结果HEAD TypeScript CI 状态： 真实场景测试本 PR 是类型级别重构 + 路由器生命周期修复，无用户可见 TUI 输出。上一次 triage 和维护者的详细本地验证报告已覆盖真实二进制冒烟测试。变异测试确认 26 个测试在没有对应生产改动时会失败——覆盖是真实的。 — Qwen Code · qwen3.7-max |
Collaborator
Reflection (Re-run after blocking issues resolved)Since the last triage, the author pushed a review feedback commit ( Stepping back: the 279 tests pass, TypeScript compiles clean, ESLint clean, CI green. The foundation is solid for #5976. Approving. ✅ 中文说明反思 （阻塞问题解决后的重新运行）自上次 triage 以来，作者推了一个 review feedback commit（ 退后看： 279 个测试通过，TypeScript 编译干净，ESLint 干净，CI 绿色。为 #5976 打下了坚实基础。 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jun 29, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
Jun 29, 2026
qwen-code-ci-bot
requested changes
Jun 29, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
previously approved these changes
Jun 29, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
Both prior suggestions are addressed — removeSessionId now has production callers via registerSessionCleanup, and thread-scoped /clear + /status integration tests are added. The typed event maps, session creation dedup, target-iteration rewrite, and restoreSessions cleanup are all well-implemented with good test coverage (99 tests passing).
— qwen3.7-max via Qwen Code /review
wenshao
reviewed
Jun 29, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Independent review of d648c4e. I found no new issues beyond what's already been raised on this PR. The ChannelAgentBridge extraction is cleanly scoped — tsc --build confirms both AcpBridge and DaemonChannelBridge satisfy the interface, and the 91 SessionRouter / ChannelBase tests pass locally.
Not approving only because the earlier Critical findings went stale after the last push (they're anchored to 4dcf798) but still apply to d648c4e:
SessionRouter.restoreSessions()still callsthis.deleteByKey(key)beforeawait this.bridge.loadSession(...)— a message arriving betweensetBridge()and restore completing can create a duplicate session that's then orphaned.AcpBridgestill never emitssessionDied(onlyDaemonChannelBridgedoes), soregisterSessionCleanup()and theChannelBasecleanup listener are dead on the standaloneqwen channel startpath.- The TS4111 index-signature typecheck errors remain in the test files (4 in
ChannelBase.test.ts, confirmed locally). They don't breaktsc --build(test files are excluded there) but fail a full typecheck.
The earlier suggestions (variable shadowing in removeSession, no logging in the sessionDied handler, SessionDiedEvent not re-exported from the barrel) also still stand. Not re-posting inline — the existing threads already cover them.
— claude-opus-4-8[1m] via Qwen Code /qreview
Collaborator
Build failure root cause: ESLint
|
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Local verification report — PR #5978Verified on a clean worktree built from this PR's head ( TL;DR: The substance is merge-ready — the ✅ Passing1. Type abstraction holds across all adapters
2. Test-plan suites green
3. Mutation tests — the new tests genuinely guard the fixes
26 tests fail without the corresponding production change → the coverage is real, not decorative. All files were restored clean afterward. 4. Real-binary smoke (legacy
5. Scope note (not a defect): ❌ Merge blocker — still present at HEAD
|
| 套件 | 结果 |
|---|---|
channels/base（全量） |
7 文件 / 170 ✓ |
channels/qqbot（全量） |
3 文件 / 68 ✓ |
cli start.test.ts |
9 ✓ |
feishu adapter.test.ts |
40 ✓ |
telegram TelegramAdapter.test.ts |
6 ✓ |
3. 变异测试——新测试确实守护这些修复
把每个生产文件还原到 base、保留 PR 的测试后：
| 还原到 base 的文件 | 失败的新测试数 |
|---|---|
SessionRouter.ts |
16 — 并发创建去重、死 session 重试、removeSessionId、restore 预占 + id 校验、threadId 扫描 |
ChannelBase.ts |
6 — threadId 的 /status & /clear、sessionDied 移除、channel 自有 router 的 bridge 事件、setBridge 监听器重绑、prompt 进行中 chunk 监听器迁移 |
cli start.ts |
2 — session 死亡清理接线、重启时重新注册 |
qqbot QQChannel.ts |
2 — registerBridgeEvents true/false |
没有对应生产改动时有 26 个测试失败 → 覆盖是真实的，不是摆设。验证后所有文件都干净还原。
4. 真实二进制冒烟（旧 qwen channel start）
- 伪造 channel 类型 →
Channel type "boguschannel" is not supported. Available: telegram, weixin, dingtalk, feishu, qq— 在新构造函数类型下每个 adapter 都能在运行时干净导入；没有[channel-registry] Failed to load。 - 用真实类型 + 伪 token 实跑
channel start→[AcpBridge]子进程已启动，registerSessionCleanup(bridge, router)在真实的AcpBridge+SessionRouter实例上执行且未抛错，随后在getMe处优雅地连接失败（exit 1，无挂起、无崩溃）。
5. git diff --check — 干净，无空白字符错误。
范围说明（非缺陷）： sessionDied 只由 DaemonChannelBridge 发出（DaemonChannelBridge.ts:700），AcpBridge 不发。所以新的 session 死亡清理路径对 daemon-backed bridge 是生效的，在当前 AcpBridge 的 channel start 路径上休眠但无害——这与本 PR 作为 daemon-backed bridge 基础（#5976）的定位完全一致。单元测试直接 emit sessionDied，所以无论实际用哪个具体 bridge，接线都有覆盖。
❌ 合并阻塞项 — 在 HEAD 4dcf798 上仍然存在
必需检查 Test (ubuntu-latest, Node 22.x) 在 Run ESLint 步骤失败，发生在 typecheck/build/测试之前。在此 head 上，eslint --max-warnings 0 仍报：
packages/channels/base/src/SessionRouter.test.ts
184:11 error 'router' is never reassigned. Use 'const' instead prefer-const
216:11 error 'router' is never reassigned. Use 'const' instead prefer-const
501:11 error 'router' is never reassigned. Use 'const' instead prefer-const
✖ 3 problems (3 errors, 0 warnings)
这 3 个是全部 20 个改动 TS 文件中唯一的 lint 错误。每个都是只赋值一次的 let router!: SessionRouter; 前向声明。
已验证的修复： 删掉 let router!: SessionRouter; 这一行，并给配对的赋值（第 198 / 225 / 509 行）加上 const，与同一文件中已经用了 41 次的 const router = new SessionRouter(...) 写法一致。mock 闭包只是引用 router，且在构造之后才被调用，所以在构造处用 const 是安全的（没有启用 no-use-before-define 规则）。应用后我重跑了套件：eslint exit 0，且 SessionRouter.test.ts 全部 44 个测试仍然通过。
// 修改前（例如第 216 / 225 行）
let router!: SessionRouter;
const newSession = vi.fn(async () => {
router.removeSessionId('dead-session');
return 'dead-session';
});
bridge = { ...mockBridge(), newSession };
router = new SessionRouter(bridge, '/default');
// 修改后
const newSession = vi.fn(async () => {
router.removeSessionId('dead-session');
return 'dead-session';
});
bridge = { ...mockBridge(), newSession };
const router = new SessionRouter(bridge, '/default');裁定
实质内容已可合并：抽象干净、范围窄；是真实的生命周期 bug 修复；覆盖非空过；旧路径能启动。挡在这个 PR 与必需检查绿灯之间的，只剩上面那个 3 行 prefer-const 修复。推上去之后，ubuntu leg 应该就能过。
wenshao
previously approved these changes
Jun 29, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No new issues in the incremental changes since R2 (4dcf798). The follow-up commit cleanly addresses prior suggestions: variable shadowing fix in removeSession, SessionDiedEvent barrel export, stderr logging in the sessionDied handler, removal of type-unsafe casts in tests, and the setBridge() test refactor. All 174 channel tests pass.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Summary
Reviewed with 9 parallel analysis agents (correctness, security, code quality, performance, test coverage, 3 undirected personas, build verification).
Build & Tests: ✅ All 157 tests pass across 3 test suites. Both channels/base and channels/qqbot build and typecheck cleanly.
Findings: 3 inline comments + 1 lint batch.
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | 🔴 High | SessionRouter.ts:284 |
loadingSessions counter drops to 0 between sequential restore iterations, prematurely clearing pendingDeadSessionIds. A session-death event firing during iteration N is lost before iteration N+1's pendingDeadSessionIds.delete() check. |
| 2 | 🟡 Medium | ChannelBase.ts:405 |
shellCommand catch block is untested — no test mocks bridge.shellCommand() to throw. |
| 3 | 🟢 Low | SessionRouter.ts:79 |
resolve() for(;;) has asymmetric error handling: dedup waiters catch-and-retry on failure, but the initiator propagates errors out of the loop. May be intentional given createLiveSession()'s internal retry. |
| 4 | 🟡 Medium | SessionRouter.test.ts |
3 eslint prefer-const violations at lines 184, 216, 501 (let router never reassigned). |
Excluded (pre-existing or already discussed): routing key collision (pre-existing in routingKey()), bridge restart zombie state, session queue error swallowing, corrupt persist file handling, removeSessionId having no production callers, AcpBridge not emitting sessionDied, variable shadowing in removeSession, missing logging in sessionDied handler, SessionDiedEvent barrel re-export, missing integration test for thread-scoped /clear and /status, race condition in restoreSessions() ordering.
Overall this is a solid preparatory refactor with good test coverage of the new lifecycle mechanisms. The main actionable item is the loadingSessions counter bug (#1).
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
approved these changes
Jun 29, 2026
Collaborator
|
@qwen-code /triage |
Collaborator
|
Re-run after the review-feedback commits ( Template still good ✓ On direction: unchanged from the first run — this is a clean preparatory refactor for #5976 (daemon-managed channel workers). Extracting On approach: the scope is still tight — abstraction + real lifecycle fixes discovered during design. The two review-feedback commits since the last LGTM stay inside the same envelope (SessionRouter load-window refactor, Moving on to code review. 🔍 中文说明在 review-feedback 提交（ 模板依然完整 ✓ 方向：与首次评估一致——这是为 #5976（daemon 管理的 channel workers）做准备的干净重构。在第二种 bridge 实现落地之前先抽取 方案：范围依然紧凑——抽象 + 设计过程中发现的实际生命周期修复。上次 LGTM 之后的两个 review-feedback 提交仍在同一范围内（SessionRouter load-window 重构、 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
|
Re-run at PR head Code review — the two review-feedback commitsThe delta since
Nothing in the delta is speculative, duplicated, or off-scope. CI evidence at HEAD
|
Collaborator
|
Stepping back at the whole PR — the motivation (#5976 foundation), the shape of the change (narrow adapter-facing contract + the lifecycle bugs discovered while extracting it), and the two review-feedback commits that landed after the last LGTM. The review-feedback work is the kind of follow-up I want to see: each commit tightens a specific surface the maintainer poked at, with a test that directly asserts the regression. The Against my independent proposal (extract the narrow bridge interface, fix the listener leak in The only reservation going in — "the diff carries ~10 Prettier-only files outside the channel scope" — was raised in the earlier LGTM and the contributor didn't split them out. They're Prettier-clean and harmless, just not advertised by the PR title. Not worth blocking a merge over; flag it for next time. CI is green at HEAD. Mutation pass + real-binary smoke from the prior LGTM still apply to the two new commits (they don't touch the CLI's user-facing surface). The PR is merge-ready. Approving. 中文说明退一步看整个 PR —— 动机（#5976 的基础）、改动的形态（窄的面向 adapter 的契约 + 抽取过程中发现的生命周期 bug），以及上次 LGTM 之后落地的两个 review-feedback 提交。 review-feedback 的工作正是我希望看到的跟进方式：每个提交都收紧维护者点到的某个具体面，配上直接断言回归的测试。 对比我最初的独立方案（抽出窄的 bridge 接口、修复 唯一的保留意见——"diff 夹带了约 10 个 channel scope 之外的纯 Prettier 文件"——在上次 LGTM 中已经提出，作者没有把它们拆出去。它们 Prettier 干净、无害，只是 PR 标题没有声明。不值得为此阻塞合并；下次注意即可。 HEAD 上 CI 绿。上次 LGTM 的变异测试 + 真实二进制冒烟对两个新提交仍适用（它们没有触及 CLI 的用户可见面）。PR 已可合并。 批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 29, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| .replace(PROMPT_UNSAFE_INVISIBLES, ' ') | ||
| .replace(/^([ \t]*)\[([^\]\r\n]{1,64})\](:?)/gm, '$1$2$3') | ||
| // eslint-disable-next-line no-control-regex | ||
| .replace(/[\u0000-\u001f\u007f]/g, ' ') |
| ): void { | ||
| bridge.on('sessionDied', (event: { sessionId: string; reason?: string }) => { | ||
| writeStderrLine( | ||
| `[Channel] Session ${event.sessionId} died${event.reason ? ` (${event.reason})` : ''}, removing routing state`, |
| } catch (err) { | ||
| const reason = err instanceof Error ? err.message : String(err); | ||
| process.stderr.write( | ||
| `[SessionRouter] Failed to restore session ${entry.sessionId} for key ${key}: ${reason}\n`, |
| output: string; | ||
| aborted: boolean; | ||
| }; | ||
| const result = await this.bridge.shellCommand!(sessionId, cmd); |
| const prefix = this.senderPrefix(channelName, senderId); | ||
| for (const k of this.toSession.keys()) { | ||
| if (k.startsWith(prefix)) return true; | ||
| for (const target of this.toTarget.values()) { |
| }); | ||
| } | ||
|
|
||
| function registerSessionCleanup( |
| ); | ||
| } | ||
| registerToolCallDispatch(bridge, router, channels); | ||
| registerSessionCleanup(bridge, router); |
| @@ -206,25 +277,66 @@ export class SessionRouter { | |||
|
|
|||
| let restored = 0; | |||
| let failed = 0; | |||
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Review summary: 8 findings (0 Critical, 0 High, 6 Medium, 2 Low). The ChannelAgentBridge abstraction and SessionRouter improvements are well-structured. The issues below are mostly defense-in-depth suggestions.
| # | Severity | Category | File | Issue |
|---|---|---|---|---|
| 1 | Medium | Correctness | sanitize.ts:77 | C0 strip regex strips \n/\r/\t |
| 2 | Medium | Security | start.ts:176 | Log injection via unsanitized event fields |
| 3 | Medium | Security | SessionRouter.ts:320 | Log injection via unsanitized persist fields |
| 4 | Medium | Correctness | ChannelBase.ts:872 | shellCommand TOCTOU |
| 5 | Medium | Correctness | SessionRouter.ts:168 | hasSession behavioral change for single-scoped |
| 6 | Low | Testing | start.ts:170 | registerToolCallDispatch test gap |
| 7 | Medium | Testing | start.ts:392 | startAll crash recovery untested |
| 8 | Low | Robustness | SessionRouter.ts:279 | Corrupted persist silent loss |
Reviewed with Qwen Code
| .replace(PROMPT_UNSAFE_INVISIBLES, ' ') | ||
| .replace(/^([ \t]*)\[([^\]\r\n]{1,64})\](:?)/gm, '$1$2$3') | ||
| // eslint-disable-next-line no-control-regex | ||
| .replace(/[\u0000-\u001f\u007f]/g, ' ') |
Collaborator
There was a problem hiding this comment.
Medium | Correctness — C0 strip regex removes \n, \r, \t
The regex /[\u0000-\u001f\u007f]/g strips all C0 control characters including newlines (\u000a), carriage returns (\u000d), and tabs (\u0009). For group messages, this will flatten multi-line user messages into single lines, breaking formatting.
Consider preserving whitespace control characters:
.replace(/[\u0000-\u0008\u000b\u000c\u000e-\u001f\u007f]/g, ' ')This strips non-printable controls while keeping \n, \r, and \t intact.
| ): void { | ||
| bridge.on('sessionDied', (event: { sessionId: string; reason?: string }) => { | ||
| writeStderrLine( | ||
| `[Channel] Session ${event.sessionId} died${event.reason ? ` (${event.reason})` : ''}, removing routing state`, |
Collaborator
There was a problem hiding this comment.
Medium | Security — Log injection via unsanitized event fields
event.sessionId and event.reason are interpolated directly into the stderr string. A session that dies due to a crafted prompt or error message could inject ANSI escape sequences into the log output.
Apply sanitizeLogText() (already imported at the top of this file) before writing:
const safeId = sanitizeLogText(event.sessionId);
const safeReason = event.reason ? sanitizeLogText(event.reason) : '';
writeStderrLine(
`[Channel] Session ${safeId} died${safeReason ? ` (${safeReason})` : ''}, removing routing state`,
);| } catch (err) { | ||
| const reason = err instanceof Error ? err.message : String(err); | ||
| process.stderr.write( | ||
| `[SessionRouter] Failed to restore session ${entry.sessionId} for key ${key}: ${reason}\n`, |
Collaborator
There was a problem hiding this comment.
Medium | Security — Log injection via unsanitized persist fields
entry.sessionId, key, and reason are written to stderr without sanitization. These come from the persisted session file (which could be tampered with) or from bridge error messages (which could contain user-controlled content).
Sanitize before logging:
process.stderr.write(
`[SessionRouter] Failed to restore session ${sanitizeLogText(entry.sessionId)} for key ${sanitizeLogText(key)}: ${sanitizeLogText(reason)}\n`,
);Note: sanitizeLogText would need to be imported from ./sanitize.js.
| output: string; | ||
| aborted: boolean; | ||
| }; | ||
| const result = await this.bridge.shellCommand!(sessionId, cmd); |
Collaborator
There was a problem hiding this comment.
Medium | Correctness — shellCommand TOCTOU between guard and call
The guard checks the locally-captured bridgeShellCommand, but the call on the next line re-reads this.bridge.shellCommand!. If setBridge() is called between these two lines (e.g., during crash recovery), the non-null assertion could throw or invoke a different bridge's method.
Use the captured reference consistently:
const bridgeShellCommand = this.bridge.shellCommand;
if (cmd && bridgeShellCommand) {
try {
const result = await bridgeShellCommand(sessionId, cmd);This ensures the function validated by the guard is the same one being called.
| const prefix = this.senderPrefix(channelName, senderId); | ||
| for (const k of this.toSession.keys()) { | ||
| if (k.startsWith(prefix)) return true; | ||
| for (const target of this.toTarget.values()) { |
Collaborator
There was a problem hiding this comment.
Medium | Correctness — hasSession behavioral change for single-scoped sessions
The no-chatId branch now iterates toTarget.values() matching by channelName + senderId. This is a behavioral change from the old senderPrefix-based scan, which explicitly did NOT match single-scoped sessions (the removed comment on senderPrefix noted this).
The new code will match single-scoped sessions (key ${channelName}:__single__, no sender segment). If a single-scoped channel exists, hasSession(channel, sender) now returns true where it previously returned false.
If this is intentional, a comment noting the change would help. If not, consider filtering:
if (scope !== 'single' && target.channelName === channelName && target.senderId === senderId)| }); | ||
| } | ||
|
|
||
| function registerSessionCleanup( |
Collaborator
There was a problem hiding this comment.
Low | Testing — registerToolCallDispatch lacks test coverage
This function dispatches tool call events to the appropriate channel based on session routing. No tests cover:
- Tool call dispatch to the correct channel
- Handling of unknown session IDs (target is undefined)
- Interaction with
SessionRouter.getTarget()
Consider adding unit tests for this dispatch logic.
| ); | ||
| } | ||
| registerToolCallDispatch(bridge, router, channels); | ||
| registerSessionCleanup(bridge, router); |
Collaborator
There was a problem hiding this comment.
Medium | Testing — startAll crash recovery path untested
The crash recovery path in startAll (~lines 440-470) performs a complex sequence: recreate bridge, setBridge() on router and all channels, registerToolCallDispatch, registerSessionCleanup, attachDisconnectHandler, then restoreSessions(). This is the most intricate recovery logic in the PR but has no test coverage.
Consider an integration test that simulates a bridge crash in multi-channel mode and verifies sessions are restored and messages continue flowing.
| @@ -206,25 +277,66 @@ export class SessionRouter { | |||
|
|
|||
| let restored = 0; | |||
| let failed = 0; | |||
Collaborator
There was a problem hiding this comment.
Low | Robustness — Corrupted persist file silently loses all sessions
If the persist file contains malformed JSON, the catch block (line 274-276) returns {restored: 0, failed: 0} without any logging. All persisted sessions are lost with no indication of the problem.
Consider logging the corruption so it can be detected by monitoring:
} catch (err) {
process.stderr.write(
`[SessionRouter] Corrupted persist file at ${this.persistPath}: ${err instanceof Error ? err.message : String(err)}\n`,
);
return { restored: 0, failed: 0 };
}
qwen-code-ci-bot
requested changes
Jun 29, 2026
| .replace(PROMPT_UNSAFE_INVISIBLES, ' ') | ||
| .replace(/^([ \t]*)\[([^\]\r\n]{1,64})\](:?)/gm, '$1$2$3') | ||
| // eslint-disable-next-line no-control-regex | ||
| .replace(/[\u0000-\u001f\u007f]/g, ' ') |
Collaborator
There was a problem hiding this comment.
[Critical] Sanitization ordering bypass: the C0/DEL strip (.replace(/[\u0000-\u001f\u007f]/g, ' ')) runs after the bracket-tag strip regex. A crafted input like \x01[SYSTEM]: bypasses the bracket regex entirely — \x01 is not in [\s], so the pattern /^([ \t]*)\[([^\]\r\n]{1,64})\](:?)/gm never matches, and the subsequent C0 strip silently replaces \x01 with a space, leaving [SYSTEM]: intact in the prompt.
This affects the entire sanitizePromptText contract — bracket-tag injection via C0 prefix smuggling.
Fix: move the C0 strip before the bracket-tag strip (matching the order used in sanitizeSenderName and sanitizeQuotedText):typescript export function sanitizePromptText(text: string): string { return ( text .replace(PROMPT_UNSAFE_INVISIBLES, ' ') // eslint-disable-next-line no-control-regex .replace(/[\u0000-\u001f\u007f]/g, ' ') .replace(/^([ \t]*)\[([^\]\r\n]{1,64})\](:?)/gm, '$1$2$3') ); }
| ): Promise<string> { | ||
| const maxAttempts = 2; | ||
| let lastDeadSessionId: string | undefined; | ||
| for (let attempt = 0; attempt < maxAttempts; attempt++) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] createLiveSession silently retries newSession() up to 2 times when the load window detects a dead session — no stderr logging between attempts. Combined with resolve() which silently retries up to 3 times when a concurrent creation fails (line 94), the worst case is ~6 newSession() calls with zero observability. This makes it hard to diagnose session-creation storms in production.
Consider adding a process.stderr.write between retry attempts in both methods, e.g.:typescript process.stderr.write( `[SessionRouter] Session ${sessionId} died before routing completed (attempt ${attempt + 1}/${maxAttempts}, key ${routingKey})\n`, );
doudouOUC
added a commit
that referenced
this pull request
Jun 30, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Alex-ai-future
pushed a commit
to Alex-ai-future/qwen-code
that referenced
this pull request
Jul 1, 2026
…nLM#6031) * feat(cli): add daemon-managed channel worker * codex: address PR review feedback (QwenLM#5978) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#6031) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#6031) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#6031) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): harden serve channel worker lifecycle Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): cover channel worker edge cases Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): address channel worker review followups Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): clear channel pidfile after worker exit Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): address serve channel review feedback Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): harden serve channel worker review issues Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): preserve channel worker exit errors Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): harden daemon channel worker startup Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): address channel worker review cleanup Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): track channel worker exit explicitly Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#6031) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address daemon worker startup review (QwenLM#6031) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#6031) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address daemon worker disconnect review (QwenLM#6031) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): address serve channel review feedback Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): address channel worker review feedback Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
pull Bot
pushed a commit
to edisplay/qwen-code
that referenced
this pull request
Jul 25, 2026
…d, patch catch path (QwenLM#7722) AcpBridge.loadSession() has been returning response.sessionId since introduction, but ACP's LoadSessionResponse schema does not include a sessionId field — so response.sessionId is always undefined. The QQChannel originally worked around this with fixRestoredSessions() that reads the raw persist file and patches the router maps. However, after SessionRouter added validation in PR QwenLM#5978, restoreSessions() throws on undefined sessionId, and the .catch() handler never called fixRestoredSessions(). Before PR QwenLM#6457 this was masked by WebSocket RESUME — the QQ-level session recovery meant restoreSessions() was never reached on reconnect. PR QwenLM#6457 added tryResume=false on non-1000 close, triggering full cold-start restore on every reconnect. Fix: 1. AcpBridge.loadSession() returns sessionId directly (ACP protocol design: LoadSessionResponse omits sessionId because client knows it) 2. QQChannel READY catch handler now calls fixRestoredSessions() so the workaround runs regardless of restoreSessions() outcome Fixes QwenLM#7721
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR introduces an adapter-facing
ChannelAgentBridgecontract for channel adapters and routing code, while keepingAcpBridgeas the current standaloneqwen channel startimplementation. Existing channel adapters, plugin creation, shared channel base logic, router wiring, and channel start dispatch now depend on the bridge abstraction where they only need agent-session behavior.It also tightens router and bridge lifecycle behavior around session ownership. The router can remove all state for a bridge session id, thread-scoped
/clearand/statusnow use the same routing key as prompts, restore rejects invalid session ids, session-death events are handled during restore/create race windows, and channel base bridge listeners are rebound correctly when a bridge is swapped. QQ’s owned-router path now keeps direct bridge session cleanup while externally managed routers remain controlled by their caller.The channel plugin docs and examples describe
ChannelAgentBridgeas the adapter-facing type, document thatAcpBridgeremains the standalone implementation, and call out the TypeScript migration path for plugins that explicitly typed constructor bridge parameters asAcpBridge.Why it's needed
This is the foundation for evolving channel support beyond a single concrete ACP-backed implementation without changing adapter contracts again. It lets future daemon-backed or multi-channel bridge implementations satisfy the same narrow surface while preserving current
qwen channel startbehavior.The lifecycle fixes address edge cases found during repeated review of that evolution path: session-death cleanup now reaches all router ownership modes, restore/create races no longer leave stale mappings, prompt stream listeners do not leak across bridge swaps, and legacy channel start restart behavior keeps its cleanup listener registered.
Reviewer Test Plan
How to verify
Run
cd packages/channels/base && npx vitest run src/SessionRouter.test.ts src/ChannelBase.test.ts src/DaemonChannelBridge.test.tsand confirm all router, channel base, and daemon bridge tests pass.Run
cd packages/channels/qqbot && npx vitest run src/send.test.tsand confirm the owned-router session cleanup behavior remains covered.Run
cd packages/cli && npx vitest run src/commands/channel/start.test.tsand confirm legacy channel start behavior, including restart cleanup listener re-registration, remains covered.Run
cd packages/channels/base && npm run buildandcd packages/channels/qqbot && npm run buildand confirm both TypeScript builds pass. The branch was also reviewed through multiple independent codex review passes after the lifecycle fixes, and the final passes reported no remaining actionable issues.Run
git diff --checkand confirm no whitespace errors are reported.Evidence (Before & After)
N/A; this is a type, router lifecycle, and documentation foundation change with no user-visible TUI output.
Tested on
Environment (optional)
Darwin, Node v26.0.0, npm 11.12.1. A clean worktree based on
origin/mainwas used for staging and focused channel verification; temporary dependency symlinks used for local test resolution were removed before commit.Risk & Scope
AcpBridgeshould move toChannelAgentBridge; runtime JavaScript plugins remain structurally compatible.qwen serve --channel, worker processes, SDK dependencies from CLI, pidfile schema changes, daemon status changes, daemon-backedqwen channel start --daemon-url, or a channel start runtime rewrite. Full root build/typecheck were re-run but were not usable as final gates because this workspace currently has unrelated non-channel failures in CLI serve/ACP/chrome-extension areas; focused channel tests and relevant package builds passed.AcpBridge,AvailableCommand, andToolCallEvent;AcpBridge.tsalso re-exports the moved types for compatibility with direct type imports.Linked Issues
Refs #5976
中文说明
What this PR does
这个 PR 引入了面向 channel adapter 的
ChannelAgentBridgecontract，让 channel adapter 和 router 依赖抽象 bridge，同时保留AcpBridge作为当前 standaloneqwen channel start的实现。现有 channel adapter、plugin 创建、共享 ChannelBase 逻辑、router wiring 和 channel start dispatch 都在只需要 agent session 行为的边界改为依赖该抽象。它还收紧了 router 和 bridge 生命周期行为。router 可以按 bridge session id 移除全部状态，thread-scoped
/clear和/status现在使用与 prompt 相同的 routing key，restore 会拒绝无效 session id，session-death 事件在 restore/create 竞态窗口中会被处理，ChannelBase 的 bridge listener 在 bridge 被替换时会正确重新绑定。QQ 自有 router 路径现在保留直接 bridge session 清理，外部管理的 router 仍由调用方控制。channel plugin 文档和示例说明
ChannelAgentBridge是 adapter-facing 类型，AcpBridge仍是 standalone 实现，并明确了显式把构造函数 bridge 参数写成AcpBridge的 TypeScript 插件迁移方式。Why it's needed
这是后续把 channel 支持演进到不绑定单一 ACP 具体实现的基础，避免未来再次改 adapter contract。未来 daemon-backed 或 multi-channel bridge 实现可以满足同一组窄接口，同时当前
qwen channel start行为保持不变。这些生命周期修复覆盖了多轮 review 中发现的演进边界：session-death cleanup 现在能覆盖所有 router ownership 模式，restore/create 竞态不会留下 stale mapping，prompt stream listener 不会在 bridge swap 后泄漏，legacy channel start restart 行为会持续注册 cleanup listener。
Reviewer Test Plan
How to verify
运行
cd packages/channels/base && npx vitest run src/SessionRouter.test.ts src/ChannelBase.test.ts src/DaemonChannelBridge.test.ts，确认 router、ChannelBase 和 daemon bridge 测试全部通过。运行
cd packages/channels/qqbot && npx vitest run src/send.test.ts，确认自有 router 的 session cleanup 行为仍有覆盖。运行
cd packages/cli && npx vitest run src/commands/channel/start.test.ts，确认 legacy channel start 行为，包括 restart cleanup listener 重新注册，仍有覆盖。运行
cd packages/channels/base && npm run build和cd packages/channels/qqbot && npm run build，确认两个 TypeScript 构建都通过。生命周期修复之后，分支还经过多轮独立 codex review，最终 review 未发现仍需处理的问题。运行
git diff --check，确认没有 whitespace 错误。Evidence (Before & After)
N/A；这是类型、router 生命周期和文档基础改动，没有用户可见的 TUI 输出。
Tested on
Environment (optional)
Darwin，Node v26.0.0，npm 11.12.1。提交和 focused channel 验证使用了基于
origin/main的干净 worktree；本地测试解析依赖时使用的临时 dependency symlink 已在提交前删除。Risk & Scope
AcpBridge，应迁移为ChannelAgentBridge；运行时 JavaScript 插件仍保持结构兼容。qwen serve --channel、worker process、CLI 到 SDK 的依赖、pidfile schema 变化、daemon status 变化、daemon-backedqwen channel start --daemon-url，也不重构 channel start runtime。完整 root build/typecheck 已重跑，但由于当前 workspace 存在 CLI serve/ACP/chrome-extension 等非 channel 区域的既有失败，无法作为最终 gate；本 PR 相关的 focused channel 测试和 package build 均已通过。AcpBridge、AvailableCommand和ToolCallEvent；AcpBridge.ts也继续 re-export 已迁移的类型，以兼容直接 type import。Linked Issues
Refs #5976