refactor(sdk): break ACP route table cycle - #10040
Conversation
|
Thanks for the PR! Second re-run on @wenshao's Template looks good ✓ Problem: verified, not theoretical — Direction: aligned — this is the same cycle-cleanup series the project is actively landing: #9959 (config client cycle) merged 2026-08-25, #10039 (auth flow client cycle) merged shortly after. No CHANGELOG reference for internal SDK import hygiene, but the area is clearly in scope. Size: Approach: minimal and right — copy the three-line guard locally instead of inventing a new leaf module for a single-use helper, matching the precedent #10039 set in Risk: no elevated risk signals (no Stage 1e high-risk path match). Moving on to code review. 🔍 中文说明感谢贡献！应 @wenshao 的 模板完整 ✓ 问题：已核实，非理论性问题—— 方向：对齐——这与项目正在合并的同一循环清理系列一致：#9959（config client 循环）已于 2026-08-25 合并，#10039（auth flow client 循环）随后合并。SDK 内部导入治理无需 CHANGELOG 引用，该领域显然在范围内。 规模： 方案：最小且正确——本地复制三行守卫，而不是为单次使用的辅助新建叶子模块，与 #10039 在 风险：无升级风险信号（未命中 Stage 1e 高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-verified at the unchanged head Findings: no blockers. Re-verified against the worktree this pass:
New since the last pass — @wenshao's deep A/B verification (his report above, reproduced here as context, not as CI evidence): runtime cycle proof as described in Stage 1, a 43,641-case differential between base and PR route extractors with zero mismatches, four injected drift mutants all caught, and 554 real wire round trips per arm with byte-identical dumps. Two of his non-blocking notes are worth the author's attention:
Test evidenceCI has fully landed on the reviewed head — everything completed is green, nothing pending, no failures (both
Real-scenario testing: N/A — unattended run, and this is a pure import-direction refactor with no user-visible surface. The behavioural claim ("route behavior unchanged") is a structural one, already settled by CI plus the maintainer's A/B verification above. 中文说明代码审查在未变化的 head 结论：无阻塞问题。本次已对照 worktree 重新核实：
上次审查后的新情况——@wenshao 的深度 A/B 验证（见其上方报告，此处仅作背景转述，不作为 CI 证据）：如上所述的运行时循环证明、base 与 PR 路由提取器之间 43,641 例差分对比零不一致、注入的四个漂移变体全部被捕获、每个 arm 554 次真实 wire 往返且转储逐字节一致。其中两条非阻塞意见值得作者关注：
测试证据CI 已在被审查的 head 上全部落定——已完成检查全绿，无进行中项、无失败（两个 真实场景测试：N/A——无人值守运行，且本改动为纯导入方向重构，无用户可见面。"路由行为不变"这一声明属结构性声明，已由 CI 与上述维护者 A/B 验证共同钉住。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review, fully green CI on the reviewed head, and a maintainer approval already standing on this exact commit; capped at 3 by the fork-refactor guardrail, which keeps the bot's own approval off the table. The cap is pure policy, not doubt. What changed since the last pass is not the code — it's the evidence around it. @wenshao ran a full A/B verification at this head (runtime cycle proof, 43k-case extractor differential, byte-identical wire dumps, four drift mutants caught), concluded merge-ready, and approved. That is exactly the human sign-off the fork-refactor guardrail exists to require; the review itself found nothing to add — the approach matches my independent proposal and the The two remaining notes from his report (the focused suites only catch 1 of 4 predicate drifts; a six-line pinning test would close that) are worth the author picking up, but the maintainer has already signalled they are not merge conditions. So the only thing left is procedural: this is a fork-authored 中文说明置信度：3/5 —— 审查干净、被审查 head 上 CI 全绿，且维护者的批准已落在同一 commit 上；因 fork 重构护栏封顶为 3 分，机器人自身不予批准。封顶纯属策略，并非存在疑虑。 上次审查以来变化的不是代码，而是围绕它的证据。@wenshao 在该 head 上完成了完整 A/B 验证（运行时循环证明、4.3 万例提取器差分、逐字节一致的 wire 转储、四个漂移变体全部捕获），结论为可合入，并已批准。这正是 fork 重构护栏所要求的人工签字；审查本身没有新发现——方案与我的独立方案及 他报告中剩下的两条意见（聚焦套件只能捕获四个谓词漂移中的一个；六行钉桩测试即可补上）值得作者跟进，但维护者已表明这些不是合入条件。 因此唯一剩余的是流程问题：这是 fork 提交的重构，无论审查多干净，机器人从不自动批准。按策略我不予批准； — Qwen Code · qwen3.8-max Reviewed at |
|
⏸️ Deferring to @wenshao — fork 中文说明⏸️ 转交 @wenshao —— fork 提交的重构 PR 从不被机器人自动批准，因此即使本次复审没有新问题，按策略我仍不予批准。您的批准已在 — Qwen Code · qwen3.8-max |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
— qwen3.8-max via Qwen Code /review (v0.22.0)
acpTransportUtils imports ROUTE_TABLE from this module; re-importing its byte-identical isRecord would silently restore the runtime cycle this change breaks. Mirrors the house-convention comment in DaemonHttpError.ts so a dedupe refactor doesn't undo the fix.
qwen-code-ci-bot
left a comment
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.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/sdk-typescript/src/daemon/acpRouteTable.ts:24 — [probe] edge-body branches of the local isRecord copy are not pinned by tests (mutation survivors)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
qwen-code-ci-bot
left a comment
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.2)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 23 passed · 0 failed · 23 total Flakiness gate: not applicable — no runnable changed test files (0 out-of-scope file(s) noted in the log) 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：23 通过 · 0 失败 · 23 总计 抖动门：不适用 — no runnable changed test files (0 out-of-scope file(s) noted in the log) Verification reportPR #10040 — refactor(sdk): break ACP route table cycleVerdict: 中文摘要
Central claim + A/BCentral claim: the runtime cycle Both arms are the two modules compiled per-module with the package's own A/B #1 — runtime import edges (
The base control is what makes the head "absence" meaningful: the same probe that reports one module on head reports two on base. Both base entry orders also load cleanly, confirming the PR's own statement that the cycle compiled and tested green (behaviorally inert, structurally wrong). A/B #2 — behavior parity (
297/308 cells matched a route (11 are the intended Predicate identity: the local guard is byte-identical to the exported one ( Reviewer Test Plan walkthrough: step 1 verified by A/B #2 (object and non-object bodies across session/workspace/file/auth routes, identical JSON-RPC methods+params); step 2 verified by the focused transport suites (below) plus the matcher harness both transports share; step 3 verified by A/B #1 (transport utils loads the route table; route table loads nothing). Mutation matrix (drift detection on the PR's declared tradeoff)Run in a scratch worktree via
M1 is the positive control proving the command collects tests that exercise the mutated file. M2's survival is a pre-existing coverage gap, not one the PR introduced: the base arm ships the identical predicate against the identical 190-test suite, and a fixture census shows no test on either arm feeds an array body to an extractor. Classification per method: coverage gap (behavior correct, nothing asserts the Array axis), reported as completeness, not a merge condition. The canary fixture ( FindingsS1 (Suggestion, completeness, non-blocking) — The No blocking findings. The PR description's claims were all confirmed as stated (byte-identical predicate, single remaining edge, 190 focused tests, unchanged behavior); no correction needed. Not covered
MethodologyEnvironment: Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Local verification reportI rebuilt this change locally and put it through a real A/B verification environment rather than re-reading the diff. Verdict: merge-ready. All three claims in the Reviewer Test Plan hold, behaviour is bit-for-bit identical, and every gate is green on both arms. Four non-blocking notes at the end. Setup. Two worktrees at the PR head ( 1 · The cycle is real, and it is really goneRather than grepping imports, I registered Node's own ESM
2 · What the removal actually buysThe comment in the diff is right that the base cycle is inert today — I confirmed it (CONTROL row: both arms load cleanly from every entry point). So I measured what it buys by applying two ordinary, behaviour-preserving future edits to a copy of each arm's compiled output and loading each module in a fresh process.
This is the concrete justification for the change, and it is stronger than "tidier imports". 3 · Behaviour is bit-for-bit unchanged — and the oracle can tellI loaded both arms' real compiled
43,641 cases, 0 mismatches (41,832 To prove that is not green-on-green, I injected the exact drift the PR names as its main risk, as four mutants of the copied predicate. All four are caught by the differential and by the wire E2E below. 4 · Real transports on a real wire, and every gateA real
554 real round trips per arm (277 requests × 2 transports), 550 JSON-RPC frames on the wire, 80 distinct methods, notifications correctly not awaited. The two arms' full wire dumps are byte-identical ( Gates, both arms: focused suites 190/190 (matches your number exactly), full SDK suite 1667/1667, Non-blocking notesN1 — the risk mitigation in the description doesn't hold. The PR names predicate drift as its main risk and offers "the complete route and transport suites pass" as the mitigation. Measured: the repo's own 190-test focused suite kills only 1 of the 4 realistic drifts (see the last column of image 3). N2 — a six-line test would close it. Validated locally: this passes on the PR as submitted and fails on all four mutants. it('session/new ignores every non-record body (pins the local isRecord)', () => {
const route = ROUTE_TABLE.find((r) => r.mapping.method === 'session/new')!;
for (const body of [[], [1], null, undefined, 'str', 42, () => {}, Object.assign(['x'], { a: 1 })]) {
expect(route.mapping.extractParams([], body, 'POST')).toEqual({});
}
});N3 — nothing stops the cycle coming back. There is no N4 — small factual nits. The description says "four-condition predicate"; it's three ( 中文版本本地验证报告我在本地重建了这个改动，用真实的 A/B 验证环境跑了一遍，而不是只重读 diff。结论：可以合入。 Reviewer 测试计划里的三条声明全部成立，行为逐字节一致，两个 arm 上所有门禁全绿。文末有四条非阻塞意见。 环境。 两个 worktree 都在 PR head（ 1 · 环真的存在，也真的消失了我没有用 grep 看 import，而是注册了 Node 自己的 ESM
2 · 移除这个环到底换来了什么diff 里的注释说得对：这个环目前是无害的，我也确认了（CONTROL 行：两个 arm 从任意入口都能正常加载）。所以我改为测量它换来了什么——对每个 arm 编译产物的副本施加两处普通的、不改变行为的未来修改，然后在全新进程里加载每个模块。
这是这次改动的具体正当性，比"import 更整洁"有力得多。 3 · 行为逐字节不变——而且这个 oracle 分辨得出来我把两个 arm 各自真实编译出来的 43,641 个用例，0 处不一致（41,832 个 为了证明这不是"绿对绿"，我把 PR 自己点名的主要风险（守卫漂移）注入成四个变异体。四个全部被差分和下面的线级 E2E 抓到。 4 · 真实传输、真实网络，以及全部门禁一个真的 每个 arm 554 次真实往返（277 个请求 × 2 种传输），线上 550 个 JSON-RPC 帧，80 个不同的方法，notification 正确地没有被等待。两个 arm 的完整线上 dump 逐字节相同（ 两个 arm 的门禁：聚焦测试 190/190（与你给的数字完全吻合），SDK 全量测试 1667/1667， 非阻塞意见N1 — 描述里的风险缓解措施其实不成立。 PR 把守卫漂移列为主要风险，缓解措施写的是"完整的路由与传输测试全部通过"。实测：仓库自己的 190 项聚焦测试只能杀掉 4 个中的 1 个现实漂移（见第 3 张图最后一列）。去掉 N2 — 六行测试就能补上。 本地已验证：它在当前 PR 上通过，在四个变异体上全部失败。 it('session/new ignores every non-record body (pins the local isRecord)', () => {
const route = ROUTE_TABLE.find((r) => r.mapping.method === 'session/new')!;
for (const body of [[], [1], null, undefined, 'str', 42, () => {}, Object.assign(['x'], { a: 1 })]) {
expect(route.mapping.extractParams([], body, 'POST')).toEqual({});
}
});N3 — 没有任何机制阻止这个环回来。 整个仓库没有 N4 — 两处小事实。 描述写的是"四条件判断"，实际是三条（ 🤖 Generated with Claude Code — Claude Opus 5 (1M context) |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 47 passed · 0 failed · 47 total Flakiness gate: not applicable — no runnable changed test files (0 out-of-scope file(s) noted in the log) 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：47 通过 · 0 失败 · 47 总计 抖动门：不适用 — no runnable changed test files (0 out-of-scope file(s) noted in the log) Verification report<!-- qwen-triage:verify --> Sandboxed verification (follow-up round): ✅ passed — merge-ready (agent verdict) — verified head 中文 — 判定：✅ 通过 · 可合入（agent 判定，跟进轮）
Verification reportPR #10040 (follow-up round) — refactor(sdk): break ACP route table cycleVerdict: 中文摘要
Previous-finding status (follow-up round)
The new head is byte-identical to the old one ( Central claim + A/BCentral claim: the runtime cycle Arms: per-module tsc emit of the two modules using the package's own A/B #1 — runtime import edges (
Static corroboration ( A/B #2 — behavior parity (
Spot oracles asserted on head: Reviewer Test Plan walkthrough: step 1 = A/B #2 (object and non-object bodies across session/workspace/file/auth routes, identical JSON-RPC methods + params); step 2 = the focused transport suites (49 + 34 tests green, Mutation matrix (drift detection on the PR's declared tradeoff)
The base-arm M2 is new this round and upgrades the round-1 census argument to a run: the shipped 190-test suite pins the Array axis on neither arm. Classification unchanged: coverage gap (behavior correct, nothing asserts it), pre-existing, completeness reporting — not a merge condition. FindingsNo new findings. No corrections needed — the PR description's claims (byte-identical predicate, single remaining edge, 190 focused tests, unchanged behavior) all re-confirmed as stated. S1 (carried from round 1, Suggestion, non-blocking) — the Informational, not a finding: the predicate now exists in three places (utils export, route-table local, Not covered
MethodologyEnvironment: Flakiness gate log<pre><code> Evidence images
— Qwen Code · sandboxed verification (follow-up round) Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Released in v0.23.0. |
4 participants
What this PR does
This PR removes the ACP route table's reverse runtime dependency on the transport utility layer. Route parameter extraction now keeps its small plain-record predicate locally, while both HTTP and WebSocket transports continue to share the same route table and matcher.
Why it's needed
The route table imported the transport utility layer only for a generic record guard, while the transport utility layer imported the route table to implement matching. That formed a runtime cycle between policy data and its consumer. Keeping the four-condition predicate at the extraction boundary restores the intended one-way dependency, avoids a new abstraction for a single-use helper, and preserves route behavior.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A — this is a non-UI import-ownership refactor. The baseline has runtime edges in both directions; after this change only the transport-utility-to-route-table runtime edge remains.
Tested on
Environment (optional)
Verified on macOS 27.0 with Node.js v26.7.0. Focused route-table, transport-utility, HTTP transport, and WebSocket transport suites passed 190 tests. The SDK lint, typecheck, and build passed. An independent clean
npm cicompleted the repository build and bundle, and the full workspace plus integration typecheck passed.Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 移除了 ACP 路由表对传输工具层的反向运行时依赖。路由参数提取现在在本地保留一个很小的普通对象判断，而 HTTP 与 WebSocket 传输仍继续共享同一套路由表和匹配器。
为什么需要它
路由表仅为了一个通用对象守卫而导入传输工具层，同时传输工具层又为实现匹配而导入路由表，因此策略数据与其消费者之间形成了运行时循环。在参数提取边界保留这个四条件判断，可以恢复预期的单向依赖，避免为单次使用的辅助逻辑新增抽象，并保持路由行为不变。
Reviewer 测试计划
如何验证
证据（修改前与修改后）
N/A——这是非 UI 的导入所有权重构。修改前两个模块之间存在双向运行时依赖；修改后仅保留传输工具指向路由表的运行时依赖。
测试平台
环境（可选）
在 macOS 27.0 和 Node.js v26.7.0 下完成验证。路由表、传输工具、HTTP 传输和 WebSocket 传输聚焦测试共 190 项通过；SDK lint、typecheck 和 build 通过；独立的干净
npm ci完成了整仓 build 与 bundle，完整 workspace 和 integration typecheck 也通过。风险与范围
关联 Issue
N/A