docs(acp): update permission mediator status - #9866
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: real, and verified independently against Direction: aligned — documentation that contradicts the code actively misleads readers, and syncing it is squarely in scope. Size: not applicable — one README file, +15/−16, no core paths touched. Approach: the scope feels right — three focused edits (lift-history table status, the Risk: no elevated risk signals — docs-only change, no high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实存在，已对照 方向：对齐——与代码矛盾的文档会主动误导读者，同步文档完全在范围内。 规模：不适用——单个 README 文件，+15/−16，未触及核心路径。 方案：范围合理——三处聚焦的修改（lift-history 表格状态、 风险：无升级风险信号——纯文档改动，未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first: for a README whose permission section contradicts the source, the minimal fix is exactly the three edits this PR makes — flip the F3-PR-24 table row from "planned" to done, rewrite the Every factual claim in the new text checks out against the code:
No blockers. Two non-blocking notes:
No sequence diagram or files table — one README file doesn't earn either. Testing evidenceUnattended CI run — the PR's own check results on the reviewed commit, fetched via API (no PR code executed; nothing user-visible to drive in tmux for a README change, so real-scenario testing is N/A):
Two checks are still running ( 中文说明代码审查先给出独立方案：对于 permission 部分与源码矛盾的 README，最小修复正是本 PR 所做的三处编辑——把 F3 PR 24 表格行从"计划中"改为已完成、重写 新文本中的每一项事实声明都与代码核对无误： 无阻塞项。两条非阻塞提醒：
单个 README 文件，不值得附时序图或文件清单。 测试证据无人值守 CI 运行——以上是通过 API 获取的该提交自身 CI 结果（未执行任何 PR 代码；README 改动无用户可见行为，真机场景测试为 N/A）。目前落地的检查全部通过（含覆盖 prettier 格式的 precheck 通道）， — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — verified-accurate docs fix in a minimal three-edit diff; only non-blocking nits (a wrong issue citation and residual staleness the PR doesn't claim to fix). Stepping back: I confirmed the staleness before trusting the PR's framing — the README's "No implementation yet" genuinely contradicts the source, and the replacement text is accurate down to the delegation call site; it reads like it was lifted from the constructor doc in Two reservations, both named in the review comment and neither blocking: the #8835 citation points at an item that doesn't exist in that issue (worth correcting before merge, or a maintainer can drop it on the way in), and the README still has stale corners elsewhere ( Context note: the author has ~15 open PRs right now, mostly small docs fixes. I evaluated this one on its own merits — the substance checks out independently, so the volume doesn't count against it here. Approval is deferred until CI lands green on the reviewed commit (the ubuntu unit/lint lane is still running); the approval will post automatically once every check completes green, and withheld if anything lands red. 中文说明置信度：4/5 —— 经核实准确的文档修复，三处编辑的最小 diff；只有非阻塞的小问题（错误的 issue 引用，以及本未声称修复的残留过时内容）。 退一步看：我先独立确认了过时内容，才接受 PR 的叙述——README 的 "No implementation yet" 确实与源码矛盾，替换文本准确到委托调用点，措辞几乎取自 两点保留意见（已在审查评论中说明，均不阻塞）：#8835 引用指向该 issue 中不存在的条目（合并前值得更正，或维护者合入时顺手删掉）；README 其他角落仍有残留过时内容（ 背景说明：该作者目前约有 15 个 open PR，多为小型文档修复。本 PR 按自身 merits 独立评估——实质内容核实无误，数量不影响本条结论。 审批推迟到 CI 在该提交上全绿（ubuntu 单元/lint 通道仍在运行）；全部检查通过后将自动发布审批，若有失败则不予发布。 — Qwen Code · qwen3.8-max Reviewed at |
| | **PR 22b/1** (#4298) | Lift `status` + `workspacePaths` + `bridgeErrors` + `bridgeTypes` | ✅ merged | | ||
| | **PR 22b/2** (#4304) | Lift `BridgeOptions` + new `DaemonStatusProvider` injection seam | ✅ merged | | ||
| | **F1** (this PR) | Lift `defaultSpawnChannelFactory` + `BridgeClient` + `createHttpAcpBridge` factory closure + new `BridgeFileSystem` injection seam (22b' scope) | ✅ in this PR | | ||
| | **F3 PR 24** | Implement the four `PermissionMediator` strategies (`first-responder`, `designated`, `consensus`, `local-only`) + pair-token revocation + audit log | ✅ implemented | |
There was a problem hiding this comment.
[Critical] The status cell flipped to "✅ implemented" here covers the row's entire unchanged scope — "the four PermissionMediator strategies … + pair-token revocation + audit log" — but pair-token identity/revocation is not implemented at this commit. The strategies and the audit log are real (permissionMediator.ts dispatches all four policies; packages/cli/src/serve/permission-audit.ts is the audit sink), yet the shipped settings schema says of client identity: "self-declared X-Qwen-Client-Id with no proof-of-possession (pair-token identity is not implemented yet)" (packages/cli/src/config/settingsSchema.ts:2924), docs/developers/daemon/12-auth-security.md:304 says "The daemon does not carry … pair-token proof-of-possession today", docs/developers/daemon/04-permission-mediation.md:255 calls pair-tokens a future mechanism ("That mechanism does not exist in v1"), and permissionMediator.ts contains zero token machinery. The revokePairingToken/revokePairingApproval functions that exist today are Local Control / messaging-channel pairing revocation — a different mechanism.
An operator hardening a qwen serve deployment reads this roadmap and concludes pair-token revocation shipped, trusting a client-identity revocation control that does not exist — while the settings schema warns any client observing originatorClientId on SSE frames can register with the same id and impersonate the originator. This PR exists to make the docs match the implementation; this cell ships a new contradiction in the opposite direction.
Witness: grep -c 'token\|revoke\|revocation\|pair' packages/acp-bridge/src/permissionMediator.ts → 0 matches, against the same commit whose settingsSchema.ts:2924 reads "pair-token identity is not implemented yet".
Suggested fix: narrow the status to what shipped — e.g. "✅ strategies + audit log; pair-token revocation pending (pair-token identity not yet implemented)" — or split pair-token revocation into its own row left marked as planned (prettier will re-align the table either way).
中文说明
此处被改为 "✅ implemented" 的状态单元格覆盖了该行未改动的完整范围——"四个 PermissionMediator 策略 … + pair-token revocation + audit log"——但 pair-token 身份/吊销在当前提交并未实现。四个策略和审计日志确实已实现（permissionMediator.ts 分发全部四种策略；packages/cli/src/serve/permission-audit.ts 是审计落地实现），但随代码发布的 settings schema 明确写道：客户端身份是 "self-declared X-Qwen-Client-Id with no proof-of-possession (pair-token identity is not implemented yet)"（packages/cli/src/config/settingsSchema.ts:2924）；docs/developers/daemon/12-auth-security.md:304 写道 "The daemon does not carry … pair-token proof-of-possession today"；docs/developers/daemon/04-permission-mediation.md:255 称 pair-token 是未来机制（"That mechanism does not exist in v1"）；且 permissionMediator.ts 中没有任何 token 相关代码。目前存在的 revokePairingToken/revokePairingApproval 函数是 Local Control / 消息通道配对的吊销——是另一套机制。
一位正在加固 qwen serve 部署的运维者读到这张路线图，会以为 pair-token 吊销已经交付，从而依赖一个并不存在的客户端身份吊销控制——而 settings schema 同时警告：任何观察到 SSE 帧上 originatorClientId 的客户端都可以用相同 id 注册并冒充发起方。本 PR 的目的正是让文档与实现一致；这个单元格却反向制造了一个新的矛盾。
证据：grep -c 'token\|revoke\|revocation\|pair' packages/acp-bridge/src/permissionMediator.ts → 0 处匹配；同一提交的 settingsSchema.ts:2924 写着 "pair-token identity is not implemented yet"。
建议修复：把状态收窄到实际已交付的内容——例如 "✅ strategies + audit log; pair-token revocation pending (pair-token identity not yet implemented)"——或者把 pair-token revocation 拆成单独一行并保持 planned 状态（无论哪种改法，prettier 都会重新对齐表格）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * `PermissionMediator` — interface contract for daemon permission flow. | ||
| * `MultiClientPermissionMediator` in `permissionMediator.ts` implements | ||
| * the supported policies, owns pending/resolved permission state, and |
There was a problem hiding this comment.
[Critical] This header rewrite flips the whole JSDoc block from "type-only stub, implementation coming later" to "production implementation" — which converts the untouched consensus bullet below (line 23: "N-of-M quorum across pair-token-authenticated clients") into a claim about shipped behavior. But pair-token client identity does not exist at this commit (packages/cli/src/config/settingsSchema.ts:2924: "pair-token identity is not implemented yet"). Notably, this same diff deletes the sibling pair-token forward reference from the PermissionMediator interface JSDoc below ("plus pair-token authentication and an audit log"), so the stale-reference sweep was intended and simply missed this bullet.
Shipped consensus authenticates voters by nothing beyond self-declared clientId membership in the issue-time snapshot: voteConsensus only checks votersAtIssue.has(vote.clientId), registerClient accepts an already-registered id with a refcount bump and no proof-of-possession, and the PermissionVote contract has no field that could carry a credential. An operator selecting consensus for the bullet's documented use case — believing quorum voters are cryptographically identified — gets no such guarantee: a client that observes a registered id (e.g. originatorClientId stamped on SSE permission_request frames) can post votes under it, and with the default M=2 unanimity quorum two connections claiming both registered ids self-approve the request.
Witness (probe run against this commit): a consensus mediator with votersAtIssue = {'ops-alice','ops-bob'} (quorum 2) resolved approved after two votes carrying nothing but bare clientId strings — out1={"kind":"recorded","votesNeeded":1} then out2={"kind":"resolved","resolvedOptionId":"proceed_once"} — with no credential anywhere in the vote contract. If the bullet's "pair-token-authenticated" property were real, these votes could not have tallied.
Suggested fix: reword the bullet to describe what consensus actually authenticates, e.g.:
* - `consensus` — N-of-M quorum across the clients registered in the
* issue-time voter snapshot (client identity is self-declared until
* pair-token authentication lands)中文说明
这段头部注释的改写把整个 JSDoc 块从"type-only 存根、实现稍后到来"翻转成了"生产实现"——这同时把下方未被改动的 consensus 条目（第 23 行："N-of-M quorum across pair-token-authenticated clients"）变成了对已交付行为的描述。但 pair-token 客户端身份在当前提交并不存在（packages/cli/src/config/settingsSchema.ts:2924："pair-token identity is not implemented yet"）。值得注意的是，同一个 diff 删除了下方 PermissionMediator 接口 JSDoc 中同源的 pair-token 前瞻性引用（"plus pair-token authentication and an audit log"），说明这次清理过时引用本是有意的，只是漏掉了这一条。
已交付的 consensus 对投票方的身份校验仅限于：自声明的 clientId 是否在议题时刻的选民快照中——voteConsensus 只检查 votersAtIssue.has(vote.clientId)；registerClient 对已注册 id 只做引用计数自增、没有持有证明；PermissionVote 契约上也没有任何可以携带凭证的字段。一位为该条目描述的场景（企业级双人复核）选择 consensus 的运维者，如果相信法定人数投票方经过了密码学身份认证，将得不到任何此类保证：观察到已注册 id（例如 SSE permission_request 帧上的 originatorClientId）的客户端可以冒用该 id 投票；在默认 M=2 全票通过的法定人数下，两个连接分别声称两个已注册 id 即可自我批准权限请求。
证据（在本提交上运行的探针）：一个 votersAtIssue = {'ops-alice','ops-bob'}（法定人数 2）的 consensus mediator，在收到两个仅携带裸 clientId 字符串的投票后即批准——out1={"kind":"recorded","votesNeeded":1}，随后 out2={"kind":"resolved","resolvedOptionId":"proceed_once"}——投票契约中没有任何凭证。如果该条目所说的 "pair-token-authenticated" 属实，这些投票不可能被计入。
建议修复：把该条目改写为描述 consensus 实际的认证语义，例如：
* - `consensus` — N-of-M quorum across the clients registered in the
* issue-time voter snapshot (client identity is self-declared until
* pair-token authentication lands)— qwen3.8-max via Qwen Code /review (v0.22.0)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Maintainer review: verified the factual claims against the codebase — change is correct. Docs-only, low risk; merging to clear the queue.
|
Maintainer note: the factual content of this change has been verified against the current codebase and is correct. Once the PR description is restructured per the template gate above (and the review-ledger findings addressed where applicable), this can proceed through the normal review flow. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/permission.ts:28 — [review] local-only bullet says "refuses any HTTP voter" but the gate is loopback-onlypackages/acp-bridge/src/permission.ts:20 — [review] designated bullet omits the anonymous-prompt first-responder fallbackpackages/acp-bridge/src/permission.ts:145 — [review] PermissionResolution JSDoc still future-tense and omits most resolution paths
中文说明
已审查——无阻断问题。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| | **PR 22a** (#4295) | Skeleton + `EventBus` + `inMemoryChannel` + `AcpChannel` types + `PermissionMediator` type-only stub | ✅ merged | | ||
| | **PR 22b/1** (#4298) | Lift `status` + `workspacePaths` + `bridgeErrors` + `bridgeTypes` | ✅ merged | | ||
| | **PR 22b/2** (#4304) | Lift `BridgeOptions` + new `DaemonStatusProvider` injection seam | ✅ merged | | ||
| | **F1** (this PR) | Lift `defaultSpawnChannelFactory` + `BridgeClient` + `createHttpAcpBridge` factory closure + new `BridgeFileSystem` injection seam (22b' scope) | ✅ in this PR | |
There was a problem hiding this comment.
[Suggestion] The F1 lift-history row this diff re-emits (re-padded inside the rewritten table hunk) still labels the F1 slice "F1 (this PR)" with status "✅ in this PR" — but all four F1 artifacts (BridgeClient, createHttpAcpBridge, defaultSpawnChannelFactory, BridgeFileSystem) were merged on 2026-06-12 via the daemon-mode feature batch 531a15d (#4490) and already exist at this PR's merge base; this docs-only PR lifts nothing. Once this PR merges, the table tells readers this docs PR is the F1 lift PR and that F1 is still "in this PR" rather than merged — contradicting the adjacent F3 row the same diff updates to "✅ policy dispatch implemented" (only possible because F1's lift already landed) and the "What's here today" bullets describing all F1 modules as present, so anyone reconstructing the #4175 lift timeline from this README gets a wrong merge point for F1. The suggestion cites the batch PR (#4490) to match the sibling rows' style; prettier will re-align if you word it differently.
| | **F1** (this PR) | Lift `defaultSpawnChannelFactory` + `BridgeClient` + `createHttpAcpBridge` factory closure + new `BridgeFileSystem` injection seam (22b' scope) | ✅ in this PR | | |
| | **F1** (#4490) | Lift `defaultSpawnChannelFactory` + `BridgeClient` + `createHttpAcpBridge` factory closure + new `BridgeFileSystem` injection seam (22b' scope) | ✅ merged | |
中文说明
本 diff 重新输出的 F1 lift-history 表格行（在重写的表格 hunk 中重新填充）仍将 F1 切片标记为 "F1 (this PR)"、状态为 "✅ in this PR"——但全部四个 F1 产物（BridgeClient、createHttpAcpBridge、defaultSpawnChannelFactory、BridgeFileSystem）早在 2026-06-12 就随 daemon-mode 功能批次 531a15d（#4490）合入，并且已存在于本 PR 的 merge base 中；本纯文档 PR 没有 lift 任何内容。合入后，这张表会让读者以为本 docs PR 就是 F1 lift PR、F1 仍处于 "in this PR" 状态——与同一 diff 更新为 "✅ policy dispatch implemented" 的相邻 F3 行（该状态只有在 F1 已合入的前提下才成立）以及 "What's here today" 中描述所有 F1 模块均已存在的条目相矛盾，任何从这份 README 重建 #4175 lift 时间线的人都会得到错误的 F1 合入点。建议修复引用批次 PR（#4490）以匹配相邻行的风格；若改用其他措辞，prettier 会重新对齐表格。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Clarify that the mediator PR implements policy dispatch and audit fan-out without claiming future pair-token revocation support. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Remove stale roadmap wording and describe the mediator implementation without implying unrelated slice ownership. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
5cef001 to
cb93f83
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
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. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Convergence: round 3 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/acp-bridge/README.md (findings in round 2; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
收敛情况：第 3 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/README.md（第 2 轮已出过发现，本轮又有 2 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| `PermissionResolution` discriminated union. `MultiClientPermissionMediator` | ||
| implements the four policies, owns pending/resolved permission state, and | ||
| handles strategy dispatch plus audit/emit fan-out; `BridgeClient` only | ||
| plumbs `requestPermission` into `mediator.request`. |
There was a problem hiding this comment.
[Suggestion] This rewritten bullet contradicts a sibling bullet the diff left stale: the unchanged bridgeClient bullet in the same section (lines 76-84) still says BridgeClient implements the "first-responder permission flow" and "Exports the supporting PendingPermission / PermissionResolutionRecord / BridgeClientSessionEntry types + MAX_RESOLVED_PERMISSION_RECORDS cap". At this commit bridgeClient.ts exports only KNOWN_APPROVAL_MODES, BridgeClientSessionEntry, BridgeClient, and the a2ui helpers — PendingPermission exists nowhere in the package, and PermissionResolutionRecord / MAX_RESOLVED_PERMISSION_RECORDS are module-private in permissionMediator.ts. A reader following that bullet imports the three symbols and gets compile errors, and the same section now tells two opposite stories about where permission state lives — the exact staleness this PR exists to remove survives in its own file. Updating the sibling bullet in the same pass keeps the section self-consistent, e.g. drop the three vanished exports (keep BridgeClientSessionEntry) and replace "first-responder permission flow" with "permission requests plumbed into the injected PermissionMediator (mediator.request)"; the pending/resolved state and cap live in permissionMediator.ts.
中文说明
这条重写后的条目与 diff 未更新的一条相邻条目矛盾：同一节中未改动的 bridgeClient 条目（第 76-84 行）仍声称 BridgeClient 实现 "first-responder permission flow" 并 "Exports the supporting PendingPermission / PermissionResolutionRecord / BridgeClientSessionEntry types + MAX_RESOLVED_PERMISSION_RECORDS cap"。在当前提交，bridgeClient.ts 只导出 KNOWN_APPROVAL_MODES、BridgeClientSessionEntry、BridgeClient 和 a2ui 辅助函数——PendingPermission 在整个包中不存在，PermissionResolutionRecord / MAX_RESOLVED_PERMISSION_RECORDS 是 permissionMediator.ts 的模块私有符号。读者按该条目导入这三个符号会得到编译错误，同一节因此对 permission 状态归属给出两种相反说法——本 PR 要消除的过期内容恰好残留在它自己改动的文件里。建议顺手更新该条目保持一致：删掉三个已不存在的导出（保留 BridgeClientSessionEntry），把 "first-responder permission flow" 改为 "permission requests plumbed into the injected PermissionMediator (mediator.request)"；pending/resolved 状态与上限在 permissionMediator.ts。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| `PermissionResolution` discriminated union. `MultiClientPermissionMediator` | ||
| implements the four policies, owns pending/resolved permission state, and | ||
| handles strategy dispatch plus audit/emit fan-out; `BridgeClient` only | ||
| plumbs `requestPermission` into `mediator.request`. |
There was a problem hiding this comment.
[Suggestion] The unchanged bridge bullet (lines 84-90) still says the createHttpAcpBridge factory "Builds the bookkeeping closures (resolveEntry, registerPending, etc.) and wires them into BridgeClient" — but registerPending exists nowhere in production code at this commit: a repo-wide sweep finds exactly 2 matches, this README line itself and a test comment documenting its removal ("F3 Commit 3 replaced the pre-F3 registerPending + rollbackPending callbacks with a single MultiClientPermissionMediator reference"), zero production hits. A maintainer grepping for registerPending to understand permission bookkeeping finds nothing while this bullet says the mediator owns that state — two opposite answers in one section, and the update silently gets skipped because the bullet looks untouched. resolveEntry genuinely still exists, so this is a reword, not a deletion — e.g. "Builds the session-entry lookup (resolveEntry) wired into BridgeClient and constructs the MultiClientPermissionMediator that owns permission state".
中文说明
未改动的 bridge 条目（第 84-90 行）仍声称 createHttpAcpBridge 工厂 "Builds the bookkeeping closures (resolveEntry, registerPending, etc.) and wires them into BridgeClient"——但在当前提交，registerPending 在生产代码中已不存在：全仓库扫描只有 2 处匹配，即本 README 行本身和一条记录其移除的测试注释（"F3 Commit 3 replaced the pre-F3 registerPending + rollbackPending callbacks with a single MultiClientPermissionMediator reference"），生产代码零匹配。维护者想理解 permission 记账逻辑而 grep registerPending 会一无所获，而相邻条目却说 mediator 持有该状态——同一节给出两种相反答案，且该条目看起来未被触碰，更新很容易被跳过。resolveEntry 确实仍存在，所以这是改写而非删除——例如 "Builds the session-entry lookup (resolveEntry) wired into BridgeClient and constructs the MultiClientPermissionMediator that owns permission state"。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * `MultiClientPermissionMediator` in `permissionMediator.ts` owns the | ||
| * policy dispatch and pending/resolved permission state used by |
There was a problem hiding this comment.
[Suggestion] This header rewrite presents the four policy bullets below it as the implemented semantics of MultiClientPermissionMediator, but the unchanged designated bullet omits the implemented anonymous-originator case: voteDesignated falls back to first-responder when originatorClientId is undefined (permissionMediator.ts:707-713). The fallback is deliberate and tested (permissionMediator.test.ts:651) and documented in the operator-facing settings schema, but an operator selecting designated for the bullet's stated use case (per-tenant approval ownership) may believe non-originator votes are always rejected — on a prompt issued with no registered client id, any attached client can resolve it. Conversely, a maintainer treating this JSDoc as the contract might "fix" voteDesignated to reject non-originators on anonymous prompts and break the deliberate fallback; the implementation file already carries "Documented here so a future maintainer doesn't 'fix' the bypass", but this bullet has no equivalent caveat. Consider extending it, e.g. "designated — only the originatorClientId that started the prompt may answer; other clients see permission_forbidden. Prompts with no originator fall back to first-responder."
中文说明
这段头部重写使其下方的四条策略条目成为 MultiClientPermissionMediator 已交付语义的描述，但未改动的 designated 条目漏掉了已实现的匿名发起方场景：当 originatorClientId 为 undefined 时，voteDesignated 会回退到 first-responder（permissionMediator.ts:707-713）。该回退是有意设计且有测试（permissionMediator.test.ts:651）、并在面向运维的 settings schema 中有说明，但一位为该条目所述场景（按租户隔离审批权）选择 designated 的运维者可能以为非发起方投票总会被拒绝——而对没有注册客户端 id 的提示，任何接入的客户端都能解决它。反向风险同样存在：把这段 JSDoc 当作契约的维护者可能去 "修复" voteDesignated，让它在匿名提示时拒绝非发起方，从而破坏这个有意的回退；实现文件里已写了 "Documented here so a future maintainer doesn't 'fix' the bypass"，但这条条目没有等价警示。建议补充，例如 "designated — only the originatorClientId that started the prompt may answer; other clients see permission_forbidden. Prompts with no originator fall back to first-responder."
— qwen3.8-max via Qwen Code /review (v0.22.0)
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
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 6 by the review time budget.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/permission.ts:42 — [review] dangling PendingPermission cross-reference + future-tense PermissionResolution JSDocpackages/acp-bridge/README.md:87 — [review] stale canonicalizeExistingAncestor / verifyParentWithinWorkspace helper names in the bridge bulletpackages/acp-bridge/src/permission.ts:27 — [review] local-only bullet misdescribes the gate (peer-address, not transport)packages/acp-bridge/src/permission.ts:27 — [review] local-only bullet omits the cross-policy cancel bypasspackages/acp-bridge/src/permission.ts:82 — [review] PermissionVote.clientId JSDoc overclaims the trust model (self-declared, not daemon-stamped)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 6 轮。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| Exports the supporting | ||
| `PendingPermission` / `PermissionResolutionRecord` / |
There was a problem hiding this comment.
[Suggestion] R3-1: This commit fixed the bullet's opening (it now says "permission requests delegated to PermissionMediator", exactly as suggested last round), but the tail still claims bridgeClient exports PendingPermission / PermissionResolutionRecord / MAX_RESOLVED_PERMISSION_RECORDS "that the factory's bookkeeping maps consume". At this commit PendingPermission exists nowhere in the package, PermissionResolutionRecord is a non-exported interface inside permissionMediator.ts, MAX_RESOLVED_PERMISSION_RECORDS is a module-private constant there, and of the four names only BridgeClientSessionEntry is still exported from bridgeClient.ts — the resolved-record cap now bounds the mediator's own map, not any factory map. An adapter author following this bullet imports three vanished symbols from @qwen-code/acp-bridge/bridgeClient and gets compile errors, and the tail contradicts the rewritten permission bullet two entries above (the mediator owns the pending/resolved state) — the same section tells two opposite stories about where permission state lives. Rework the tail alongside the opening, e.g.:
Exports the supporting `BridgeClientSessionEntry` type; pending/resolved
permission records and the `MAX_RESOLVED_PERMISSION_RECORDS` cap live
inside `MultiClientPermissionMediator` (`permissionMediator.ts`).
中文说明
本次提交已修复该条目的开头（现在写作 "permission requests delegated to PermissionMediator"，与上一轮建议一致），但条目结尾仍声称 bridgeClient 导出 PendingPermission / PermissionResolutionRecord / MAX_RESOLVED_PERMISSION_RECORDS 且 "factory's bookkeeping maps consume"。在当前提交，PendingPermission 在整个包中不存在，PermissionResolutionRecord 是 permissionMediator.ts 内部的非导出接口，MAX_RESOLVED_PERMISSION_RECORDS 是该文件的模块私有常量，四个名字中只有 BridgeClientSessionEntry 仍从 bridgeClient.ts 导出——resolved-records 上限现在约束的是 mediator 自己的 map，而非工厂的任何 map。适配器作者按该条目从 @qwen-code/acp-bridge/bridgeClient 导入这三个已消失的符号会得到编译错误；且该结尾与上方两条被重写的 permission 条目（mediator 持有 pending/resolved 状态）矛盾——同一节对 permission 状态归属给出两种相反说法。建议连同开头一并改写结尾，例如（见英文部分代码块）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| Exports the supporting | ||
| `PendingPermission` / `PermissionResolutionRecord` / | ||
| `BridgeClientSessionEntry` types + `MAX_RESOLVED_PERMISSION_RECORDS` | ||
| cap that the factory's bookkeeping maps consume. |
There was a problem hiding this comment.
[Suggestion] R3-2: The bridge bullet immediately below still says the factory "Builds the bookkeeping closures (resolveEntry, registerPending, etc.) and wires them into BridgeClient" — but at this commit registerPending exists nowhere in production code (repo-wide, only this README line plus a test comment documenting its removal), and resolveEntry is not a factory closure: the factory passes an anonymous inline closure into BridgeClient's resolveEntry constructor parameter, while the only named resolveEntry is a private method of MultiClientPermissionMediator. A maintainer grepping for registerPending/resolveEntry in bridge.ts finds nothing and either concludes the README is unreliable (defeating this PR's purpose) or misattributes the mediator's private resolveEntry to the factory when modifying permission resolution paths. Reword to what the factory actually does, e.g. "constructs the MultiClientPermissionMediator that owns permission state and passes the session-entry lookup into BridgeClient".
中文说明
紧邻下方的 bridge 条目仍声称工厂 "Builds the bookkeeping closures (resolveEntry, registerPending, etc.) and wires them into BridgeClient"——但在当前提交，registerPending 在生产代码中不存在（全仓库仅本 README 行和一条记录其移除的测试注释），且 resolveEntry 也不是工厂闭包：工厂向 BridgeClient 的 resolveEntry 构造参数传入一个匿名内联闭包，唯一名为 resolveEntry 的方法是 MultiClientPermissionMediator 的私有方法。维护者在 bridge.ts 中 grep registerPending/resolveEntry 会一无所获，要么认为 README 不可靠（与本 PR 的目的相悖），要么在修改 permission 解决路径时把 mediator 的私有 resolveEntry 误归到工厂。建议改写为工厂实际的职责，例如 "constructs the MultiClientPermissionMediator that owns permission state and passes the session-entry lookup into BridgeClient"。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| @@ -23,18 +20,16 @@ | |||
| * prompt may answer; other clients see `permission_forbidden`. | |||
There was a problem hiding this comment.
[Suggestion] R3-3: The header rewrite presents the four policy bullets below it as the implemented semantics of MultiClientPermissionMediator, but this designated bullet still omits the implemented anonymous-originator case: voteDesignated falls back to first-responder when originatorClientId is undefined (permissionMediator.ts). The fallback is deliberate and tested, and the operator-facing settings schema documents it, but this bullet carries no equivalent caveat — an operator selecting designated for the bullet's stated use case (per-tenant approval ownership) may believe non-originator votes are always rejected, while on a prompt issued with no registered client id any attached client can resolve it; conversely a maintainer treating this JSDoc as the contract might "fix" voteDesignated to reject non-originators on anonymous prompts and break the deliberate fallback.
| * prompt may answer; other clients see `permission_forbidden`. | |
| * prompt may answer; other clients see `permission_forbidden`. | |
| * Prompts with no originator fall back to first-responder. |
中文说明
头部重写使其下方的四条策略条目成为 MultiClientPermissionMediator 已交付语义的描述，但这条 designated 条目仍漏掉了已实现的匿名发起方场景：当 originatorClientId 为 undefined 时，voteDesignated 会回退到 first-responder（permissionMediator.ts）。该回退是有意设计且有测试，面向运维的 settings schema 也有说明，但这条条目没有等价警示——为条目所述场景（按租户隔离审批权）选择 designated 的运维者可能以为非发起方投票总会被拒绝，而对没有注册客户端 id 的提示，任何接入的客户端都能解决它；反向风险是把这段 JSDoc 当契约的维护者可能去 "修复" voteDesignated，让它在匿名提示时拒绝非发起方，从而破坏这个有意的回退。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Clarify the current bridge client exports and designated policy fallback so the migration docs match the mediator implementation. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Review: docs(acp): update permission mediator statusPR #9866 — 2 files, +40/−51, docs-only. Verified factual claimsAll claims checked against the codebase on
Changes are factually correctThe README.md and Suggestions (3)S1 — README.md S2 — S3 — Pre-existing deferred findings (not re-raised)The following round 4 deferred findings remain standing but are outside this PR's stated scope:
VerdictCOMMENT — no Criticals. All changes are factually verified against the codebase. The three Suggestions above are in-scope: the PR touches these files and these lines, so the fixes fit naturally within the same commit. — Qwen Code review (v0.22.0) |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.8-max via Qwen Code /review (v0.22.0)
Align the bridge roadmap and permission policy docs with the shipped mediator behavior so reviewers and adapter authors do not infer pair-token identity or stale factory bookkeeping. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
4 participants
What this PR does
Updates the ACP bridge permission documentation so it matches the current implementation:
MultiClientPermissionMediatorimplements the permission policies and owns pending/resolved permission state, whileBridgeClient.requestPermissiondelegates into the mediator.Why it's needed
The README and
permission.tscomments still described the mediator as a type-only future interface and said first-responder voting lived inline inBridgeClient.requestPermission. That is stale against the current source and can mislead readers into looking in the wrong place or reimplementing mediator behavior that already exists.Reviewer Test Plan
How to verify
../qwen-code-fail-zero-inode-read/node_modules/.bin/prettier --check packages/acp-bridge/README.md packages/acp-bridge/src/permission.tsrg -n "No implementation yet|will move|will replace|wired inline|type-only interface contract|MultiClientPermissionMediator|BridgeClient.requestPermission" packages/acp-bridge/README.md packages/acp-bridge/src/permission.ts packages/acp-bridge/src/permissionMediator.ts packages/acp-bridge/src/bridgeClient.tsEvidence (Before & After)
N/A. This is a documentation/comment correction.
Tested on
Environment (optional)
N/A.
Risk & Scope
Linked Issues
Related to stale ACP bridge permission-mediator documentation noted in repo-hygiene follow-up discussion.
中文说明
What this PR does
更新 ACP bridge permission 文档，使其与当前实现一致：
MultiClientPermissionMediator已实现 permission policies 并持有 pending/resolved permission 状态，BridgeClient.requestPermission只负责委托到 mediator。Why it's needed
README 和
permission.ts注释仍把 mediator 描述成未来要实现的 type-only interface，并称 first-responder voting 仍在BridgeClient.requestPermission内联实现。这个说法已经和当前源码不一致，容易让读者查错位置，甚至重复实现已经存在的 mediator 行为。Reviewer Test Plan
How to verify
../qwen-code-fail-zero-inode-read/node_modules/.bin/prettier --check packages/acp-bridge/README.md packages/acp-bridge/src/permission.tsrg -n "No implementation yet|will move|will replace|wired inline|type-only interface contract|MultiClientPermissionMediator|BridgeClient.requestPermission" packages/acp-bridge/README.md packages/acp-bridge/src/permission.ts packages/acp-bridge/src/permissionMediator.ts packages/acp-bridge/src/bridgeClient.tsEvidence (Before & After)
N/A。此 PR 只修正文档和源码注释。
Tested on
Environment (optional)
N/A。
Risk & Scope
Linked Issues
关联 repo-hygiene 后续讨论中提到的 ACP bridge permission mediator 过期文档问题。