refactor(acp-bridge): narrow workspace event capabilities - #9957
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献!
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review — no blocking findings. The capability split matches reality rather than aspiration: I grepped bridge method usage per consumer on Things I verified along the way:
This is a type-level-only change — erased at compile time — so the TypeScript build is the enforcement oracle, and that is exactly what CI exercises. No sequence diagram or file table: the shape of the change is fully captured above. Testing evidence — unattended CI run, so per the gate rules I did not build or execute this PR's code; the evidence below is the PR's own CI fetched through the GitHub API at review time. Nothing is red so far; the main Node unit suite and the Serve A/B job were still running at fetch time. The table updates in place once CI settles. Real-scenario (tmux) coverage doesn't apply — no TUI surface is touched. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 中文说明代码审查 —— 无阻塞问题。能力拆分与实际使用完全吻合,不是凭空设计:我在 核实过的要点:
这是纯类型层改动——编译期擦除——TypeScript 构建就是强制校验,而 CI 正好覆盖这一点。不附时序图或文件表:改动结构在上面已完整说明。 测试证据 —— 无人值守 CI 运行,按门禁规则未构建或执行 PR 代码;以下证据为审查时刻通过 GitHub API 获取的 PR 自身 CI 结果。目前无失败项;主 Node 单测套件与 Serve A/B 任务获取时仍在运行,表格会在 CI 结束后原地更新。真实场景(tmux)覆盖不适用——未触及 TUI。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; a textbook interface-segregation refactor that verifies statically. Stepping back: before reading the diff I sketched what I'd do from the title and motivation alone — two capability interfaces in The risk profile for this shape of change is essentially nil — interfaces are erased at compile time, the full bridge keeps its contract through extension, and every call site passes the full bridge, which stays assignable. I named all four narrowed consumers and their call sites; nothing downstream needs churn. If I'm maintaining this in six months I'll thank the author: the contracts now document which services may inspect client membership, and the tests enforce it by construction. Approval is deferred until CI lands green on 中文说明置信度:5/5 —— 各阶段全部干净;一次教科书式的接口隔离重构,静态层面即可充分验证。 退一步看:在读 diff 之前,我只凭标题和动机先写下了自己的方案——在 这类改动的风险几乎为零——接口在编译期擦除,完整 bridge 通过继承保留原有契约,所有调用点传入的都是完整 bridge,仍然可赋值。四个被收窄的消费者及其调用点我都点名核对过,下游无需连带改动。半年后维护这段代码时我会感谢作者:契约现在明确记录了哪些服务可以检查客户端成员关系,而且测试从结构上强制保证了这一点。 审批将推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Released in v0.22.2. |
What this PR does
Workspace event consumers now depend on capability-focused bridge contracts: publishers receive only event publication, while consumers that validate connected clients also receive the known-client query. The full session bridge continues to satisfy both contracts.
Why it's needed
Several services accepted the entire ACP session bridge even though they used only one or two event methods. That widened their architectural dependency, made tests construct large unrelated fakes, and obscured which services are allowed to inspect client membership.
Reviewer Test Plan
How to verify
Trigger workspace Git state, device-flow, workspace-memory, and GitHub setup events. Confirm each event is still published, and confirm only the memory/setup paths query known client IDs when validating a target. Review the service boundaries and verify publication-only consumers cannot access session lifecycle operations through their declared dependency.
Four focused CLI suites pass: 54 tests.
Evidence (Before & After)
N/A — type-level capability narrowing with unchanged event behavior.
Tested on
Environment (optional)
macOS arm64, Node.js v25.6.1; 54 focused Vitest tests, ACP bridge build, CLI build, and ESLint completed successfully.
Risk & Scope
Linked Issues
None.
中文说明
本 PR 做了什么
工作区事件消费者现在依赖按能力划分的 bridge 契约:只发布事件的消费者仅接收发布能力,需要校验已连接客户端的消费者还会接收已知客户端查询能力。完整 session bridge 仍然满足这两种契约。
为什么需要
多个服务此前接收完整 ACP session bridge,但实际只使用一两个事件方法。这扩大了架构依赖,迫使测试构造包含大量无关内容的假对象,也模糊了哪些服务有权检查客户端成员关系。
Reviewer 测试计划
如何验证
触发工作区 Git 状态、device flow、工作区 memory 和 GitHub setup 事件。确认每个事件仍会发布,并确认只有 memory/setup 路径在校验目标时查询已知客户端 ID。检查服务边界,确认仅发布消费者无法通过其声明的依赖访问 session 生命周期操作。
4 个 CLI 聚焦测试套件全部通过:54 个测试。
证据(修改前与修改后)
不适用——这是类型层面的能力收窄,事件行为保持不变。
测试平台
环境(可选)
macOS arm64、Node.js v25.6.1;54 个聚焦 Vitest 测试、ACP bridge 构建、CLI 构建和 ESLint 均成功完成。
风险与范围
关联 Issue
无。