refactor: centralize approval mode contracts - #9796
Conversation
|
|
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献!
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review — I independently proposed the same shape before reading the diff: keep core's Load-bearing assumptions verified against the base tree, not taken on faith:
No blockers found. One note from the first push: the initial head ( Test evidence — this is an unattended CI run, so the evidence is the PR's own CI on the reviewed commit, fetched via API (no PR code was built or executed here): Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Read of the signal: everything completed is green, and the first push's only failure was fixed by the author and re-verified. Not yet verified, still running: Real-scenario testing: N/A — contract and type-safety refactor with no user-visible behavior change; CI evidence is the applicable signal on this path. 中文说明代码审查 —— 在读 diff 之前我独立构思的方案与之相同:以 core 的 关键假设均已对照基线代码验证,而非想当然:
未发现阻塞项。首次推送有一个问题:初始 head( 测试证据 —— 本次为无人值守 CI 运行,证据来自 API 拉取的该提交自身 CI 结果(此处未构建或执行任何 PR 代码):表格见上方英文部分。已完成的检查全部通过,首次推送的唯一失败已由作者修复并复验。尚未验证、仍在运行: 真实场景测试:不适用——契约与类型安全重构,无用户可见行为变化;此路径下 CI 证据即为适用信号。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, well-scoped contract centralization that matches the issue's ask and my own independent proposal; the only reservation is that the main Node CI workflow has not finished on the reviewed commit yet. Stepping back: the problem was demonstrated with a drift table before any code was written, and the fix is the mechanical version of that table — one JSON fixture, three language suites comparing against it, and workflow path filters so a contract change can't skip the Python/Java runs. No codegen, no new runtime dependency for the published SDKs, no drive-by edits; the design doc even names the adjacent value domains it deliberately does not touch. If I have to maintain this in six months, the answer is clear: adding a sixth mode is now one enum edit plus failing tests that tell you exactly which SDK to update, instead of a grep across three languages. The first push tripped ruff import-sorting and took all three Python jobs red; the follow-up commit is a one-line fix for exactly that, and Python is green again. Approving with CI still running would attest to results that don't exist yet — 中文说明置信度:4/5 —— 干净、范围合理的契约集中化,与 issue 的诉求和我独立构思的方案一致;唯一的保留意见是主 Node CI 工作流在受审提交上尚未跑完。 整体来看:问题在写代码之前就用漂移对照表证明过了,而修复正是那张表的机制化版本——一个 JSON 契约、三个语言的测试套件与之比对、workflow 路径过滤确保契约变更不会跳过 Python/Java 运行。没有代码生成,没有给已发布的 SDK 引入新的运行时依赖,也没有顺手改动;设计文档甚至明确列出了有意不触碰的相邻值域。六个月后维护它的体验很清楚:新增第六种 mode 只需改一处枚举,失败的测试会准确指出该更新哪个 SDK,而不是在三种语言里全文搜索。 首次推送因 ruff import 排序使三个 Python 任务变红;后续提交恰好一行修复,Python 重新变绿。在 CI 未结束时批准等于为尚不存在的结果背书—— — Qwen Code · qwen3.8-max Reviewed at |
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. |
Verification statusThis is a behavior-preserving contract refactor with no intended UI change, so UI screenshots are not relevant evidence and have been removed from this comment. The previous focused verification covered the approval-mode contract consumers and cross-language drift checks. The PR head has since advanced to |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1c": none — though I did not run typecheck/tests (review-only pass); the compile-level claims above rest on reading declarations and call sites..
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/sdk-java.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/sdk-python.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):"agent 1c":none — though I did not run typecheck/tests (review-only pass); the compile-level claims above rest on reading declarations and call sites.。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/sdk-java.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/sdk-python.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Handled the approval-mode drift findings. Changed: ACP bridge approval modes now derive from the shared core contract and are pinned by the SDK drift test; MCP and CLI approval-mode descriptions derive from the shared tuples; the MCP global-scope gate is fail-closed for newly added modes; config tests pin the legacy auto_edit alias; non-interactive permission checks derive from APPROVAL_MODES. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
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)
🩺 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 |
|
Closeout: addressed the approval-mode follow-up comments by restoring per-mode CLI descriptions, pinning both legacy settings aliases, covering the non-interactive permission gate, and covering the serve-bridge local allow path. The shared-parser suggestion was intentionally not taken to avoid coupling CLI startup parsing to serve-status fallback code. Verified with |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 6c": full npm run typecheck /lint across cli+sdk packages (verified via targeted source reads and green test runs instead).
Convergence: round 3 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 6 (6 new). Findings keep coming back to the same files: packages/cli/src/config/config.ts (findings in round 2; 2 more now). 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. 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.)
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):"agent 6c":full npm run typecheck /lint across cli+sdk packages (verified via targeted source reads and green test runs instead)。
收敛情况:第 3 轮发布了 4 条行内评论,其中 4 条是首次提出;上一轮发布了 6 条(其中 6 条首次提出)。发现反复回到同一批文件:packages/cli/src/config/config.ts(第 2 轮已出过发现,本轮又有 2 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。本轮没有未决的 Critical,因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Convergence: round 4 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/sdk-typescript/test/unit/serve-bridge.test.ts (findings in round 3; 2 more now). 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. 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.)
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛情况:第 4 轮发布了 2 条行内评论,其中 2 条是首次提出;上一轮发布了 4 条(其中 4 条首次提出)。发现反复回到同一批文件:packages/sdk-typescript/test/unit/serve-bridge.test.ts(第 3 轮已出过发现,本轮又有 2 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。本轮没有未决的 Critical,因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Changed: added SDK serve-bridge coverage for non-persistent local approval-mode updates and the allowGlobalScope=true persistent elevated-mode path. Verified: git diff --check passed locally. Focused SDK Vitest could not start in this isolated worktree because local dependencies cannot resolve vitest/config, so GitHub CI should verify the test. Pending: post-push CI/review on 50b3928. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/sdk-typescript/test/unit/serve-bridge.test.ts:596 — [probe] rejection tests never assert zero dispatch on the security gatepackages/sdk-typescript/test/unit/serve-bridge.test.ts:580 — [probe] rejection tests never target a non-default session
Convergence: round 5 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/sdk-typescript/test/unit/serve-bridge.test.ts (findings in round 4; 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.)
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 5 轮发布了 2 条行内评论,其中 2 条是首次提出;上一轮发布了 2 条(其中 2 条首次提出)。发现反复回到同一批文件:packages/sdk-typescript/test/unit/serve-bridge.test.ts(第 4 轮已出过发现,本轮又有 2 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。本轮没有未决的 Critical,因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const result = await approvalTool.handler( | ||
| { mode: 'default', persist: true, session_id: 'test-session' }, | ||
| { mode: 'auto', persist: true, session_id: 'test-session' }, | ||
| {}, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This new test uses session_id equal to defaultSessionId ('test-session' for both), so it cannot distinguish correct session routing from a handler that resolves persisted changes to the default session. The allow-local tests above deliberately prove that an explicit non-default session ('other-session') reaches the daemon URL; the persist path has no equivalent pinning. Concretely, if a future refactor resolved persisted requests to the default session (e.g. resolveSessionId(state, args.persist ? undefined : args.session_id)), a call like { mode: 'auto', persist: true, session_id: 'feature-session' } would apply and persist the elevated mode on the default session instead of the requested one, and this test would still pass because its URL assertion matches the wrong target whenever session_id == defaultSessionId. A probe confirmed the gap: with that mutant applied the suite stays 38 passed (38), while re-routing this test to a non-default session flips it to 1 failed (expected '…/session/other-session/approval-mode' received '…/session/default-session/approval-mode'). Mirror the allow-local tests: use makeMockState({ defaultSessionId: 'default-session' }), pass session_id: 'other-session', and assert the URL is http://127.0.0.1:4170/session/other-session/approval-mode.
中文说明
这个新测试使用的 session_id 与 defaultSessionId 相同(都是 'test-session'),因此无法区分"正确的会话路由"和"把持久化变更解析到默认会话"的处理器行为。上面的 allow-local 测试特意验证了显式非默认会话('other-session')会出现在 daemon URL 中,但 persist 路径缺少同等的固定。具体来说,如果未来某个重构把持久化请求解析到默认会话(例如 resolveSessionId(state, args.persist ? undefined : args.session_id)),那么 { mode: 'auto', persist: true, session_id: 'feature-session' } 这样的调用会把提权模式应用并持久化到默认会话而非请求的会话上,而本测试仍会通过——因为只要 session_id == defaultSessionId,它的 URL 断言就恰好匹配错误的目标。探针验证了该缺口:应用该变异后套件仍为 38 passed (38);把本测试改为路由到非默认会话后变为 1 failed(expected '…/session/other-session/approval-mode' received '…/session/default-session/approval-mode')。建议对齐 allow-local 测试:使用 makeMockState({ defaultSessionId: 'default-session' }),传入 session_id: 'other-session',并断言 URL 为 http://127.0.0.1:4170/session/other-session/approval-mode。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(JSON.parse(calls[0]?.body ?? '{}')).toEqual({ | ||
| mode: 'auto', | ||
| persist: true, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] This test (and the allow-local it.each above) inspects only calls[0] and never asserts that exactly one request reached the daemon, so a regression that dispatches an additional privileged request alongside the sanctioned one stays invisible — a second mode change against the default session, or a side-effecting write issued after the route, would reach the mock daemon with the whole suite green. A probe confirmed the gap: a double-dispatch mutant survives (Tests 38 passed (38)), and adding expect(calls).toHaveLength(1) flips it to 3 failed (expected [ {…}, {…} ] to have a length of 1 but got 2); the assertion alone against the current code stays 38 passed, so it adds no false failures.
| expect(JSON.parse(calls[0]?.body ?? '{}')).toEqual({ | |
| mode: 'auto', | |
| persist: true, | |
| }); | |
| expect(JSON.parse(calls[0]?.body ?? '{}')).toEqual({ | |
| mode: 'auto', | |
| persist: true, | |
| }); | |
| expect(calls).toHaveLength(1); |
中文说明
本测试(以及上方的 allow-local it.each)只检查 calls[0],从未断言恰好只有一个请求到达 daemon,因此"在被允许的请求之外额外分发一个提权请求"的回归是不可见的——例如对默认会话再发一次模式变更,或在路由之后发起一次有副作用的写入,都会在所有测试保持绿色的情况下到达 mock daemon。探针验证了该缺口:双分发变异可以存活(Tests 38 passed (38));加上 expect(calls).toHaveLength(1) 后变为 3 failed(expected [ {…}, {…} ] to have a length of 1 but got 2);仅添加该断言对当前代码仍为 38 passed,不会引入误报。
— qwen3.8-max via Qwen Code /review (v0.22.0)
chiga0
left a comment
There was a problem hiding this comment.
Tier: Standard — cross-SDK contract refactor (39 files, +404 / -238).
No blocking findings.
Checked:
- Contract class 1 (writer/reader asymmetry): single source of truth established in
packages/core/src/config/approval-modes.json+ApprovalModeenum. All six consumers (core, CLI, TypeScript SDK, Python SDK, Java SDK, acp-bridge, VS Code companion) now derive from it. No divergence found at head. - Contract class 2 (API compatibility):
SDKSystemMessage.permission_modeandQueryOptions.permissionModenarrowed fromstring→PermissionModeunion. Intentional breaking-of-invalid-inputs, stated in PR description; valid clients unaffected. permissionController.checkPermissionMode()rewrite: old switch (plan/auto-edit/auto/yolo→ allowed;default/unknown → denied) and new if-chain (DEFAULT → denied; APPROVAL_MODES member → allowed; unknown → denied) are logically equivalent. No behaviour change.handleSetPermissionMode: old hardcoded array and newAPPROVAL_MODEScontain identical five values. No widening.- Drift test order assertion:
[...KNOWN_APPROVAL_MODES].toEqual(contract)works because Set preserves insertion order and enum declaration order, JSON array order, andPERMISSION_MODESliteral order all match. - Java path resolution: walks CWD upward to find
approval-modes.json; works under Maven's module-directory CWD convention. Python uses__file__-relative resolution (more portable). APPROVAL_MODE_INFOimport in CLI: defined atpackages/core/src/config/config.ts:379, re-exported via core barrel — import is valid.- VS Code companion enum→const-object migration: named constants hardcoded,
APPROVAL_MODE_MAPderives automatically; no existing consumers break.
CI:
- Test (ubuntu-latest, Node 22.x): pass
- SDK Python 3.10 / 3.11 / 3.12: pass
- Desktop Shell (ubuntu-22.04, windows-2022): pass
- Test (windows-latest, macos-latest, Node 22.x): SKIPPED — no OS-specific logic in this diff; pure type/constant refactor.
- Integration Tests (CLI, No Sandbox): SKIPPED — acceptable for a type-safety refactor with no runtime behavior change.
Scope: source files reviewed. NOT reviewed: docs/design/2026-08-23-approval-mode-contract.md (documentation only). Execution rungs 1-3 not run (no toolchain in environment); static analysis sufficient for a refactor with equivalent logic.
Reviewed with AI assistance.
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the complete 39-file diff at 50b3928 against current main and traced the changed approval-mode consumers across core, CLI and non-interactive control, ACP bridge, the TypeScript SDK, Python and Java drift checks, and the VS Code companion.
No blocking correctness, security, ownership, compatibility, or failure-path issues found. The shared contract preserves the existing five runtime values and legacy aliases; future bridge modes remain fail-closed without the global-scope opt-in; and the cross-language fixture is exercised by the Node, Python, and Java workflows.
CI is green on this commit for Qwen Code CI on Ubuntu Node 22, SDK Python 3.10 through 3.12, SDK Java 11, 17, and 21 including real-daemon E2E, Serve A/B, Desktop Shell, and web-shell E2E. The CLI no-sandbox integration job was skipped by workflow conditions; no changed runtime path here depends on that uncovered scenario.
The two current unresolved threads are non-blocking test-hardening Suggestions from review round 5, not production defects. Approving.
|
Released in v0.22.2. |
What this PR does
This PR centralizes the approval-mode contract used by core, the CLI, the TypeScript SDK, and the VS Code companion. TypeScript unions, validators, command choices, and wire fields now derive from the core contract or the TypeScript SDK's checked tuple instead of restating the five literals.
It also adds a small cross-language JSON contract that the TypeScript, Python, and Java SDK test suites compare against. The Python and Java workflows now run when that contract changes, so a future core-only mode addition cannot silently leave either SDK behind.
Why it's needed
The immediate missing
autovalues in Python and Java were fixed by #9003, but the same approval-mode domain remained copied across packages with no mechanical synchronization. That left future mode additions vulnerable to the same compatibility bug and allowed two system-message wire fields to accept arbitrary strings at compile time.Reviewer Test Plan
How to verify
string.Evidence (Before & After)
N/A — this is a contract and type-safety refactor with no UI behavior change.
Tested on
Environment (optional)
Node.js 22.22.0; Python tests run through uv with Python 3.14.2.
Risk & Scope
Linked Issues
Closes #9145
中文说明
本 PR 做了什么
本 PR 统一了 core、CLI、TypeScript SDK 和 VS Code companion 使用的 Approval Mode 契约。TypeScript 的联合类型、校验器、命令选项和 wire 字段现在都从 core 契约或 TypeScript SDK 中受校验的 tuple 派生,不再重复手写五个字面量。
同时新增了一个小型跨语言 JSON 契约,TypeScript、Python 和 Java SDK 的测试套件都会与它比对。Python 和 Java workflow 也会在该契约变化时运行,因此以后即使只在 core 增加 mode,也不会再静默遗漏任一 SDK。
为什么需要
Python 和 Java 缺少
auto的直接问题已经由 #9003 修复,但同一个 Approval Mode 值域仍散落复制在多个包里,没有机械同步机制。未来新增 mode 时仍可能复现同类兼容性问题,而且两个 system-message wire 字段在编译期仍允许任意字符串。Reviewer 测试计划
如何验证
string。前后证据
N/A —— 这是契约与类型安全重构,没有 UI 行为变化。
测试环境
环境(可选)
Node.js 22.22.0;Python 测试通过 uv 使用 Python 3.14.2 运行。
风险与范围
关联 Issue
Closes #9145