fix(acp-bridge): Disable permission timeout by default - #9933
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Static review at head ecb06d23 (no tests/builds run — evidence is head-tree source)
Verdict: no blocking issues. 💬 Comment. This flips one constant (DEFAULT_PERMISSION_TIMEOUT_MS 5 min → 0) and updates docs/breadcrumbs/tests around it; the full plumbing was traced end-to-end and holds.
结论：无阻塞问题。💬 评论。 本 PR 只翻转一个常量（DEFAULT_PERMISSION_TIMEOUT_MS 从 5 分钟改为 0）并同步更新文档、stderr breadcrumb 与测试；完整链路已逐行追踪验证，成立。
🎉 Praise
- The disable path is airtight end-to-end. Traced at head: the CLI flag has no yargs
defaultand is only forwarded when explicitly set (serve.ts→runQwenServe), embedded validation accepts0and rejects NaN/non-integer/negative (run-qwen-serve.ts:3022-3031), the bridge resolvesopts.permissionResponseTimeoutMs ?? DEFAULT_PERMISSION_TIMEOUT_MSand clamps any non-positive/non-finite value to the0sentinel (bridge.ts:2730-2740, retaining the 2^31-1 setTimeout-overflow clamp), and the mediator installs a timer only underif (timeoutMs > 0)(permissionMediator.ts:509).0therefore genuinely means "no timer", not "fire immediately".
禁用路径端到端严密。 CLI flag 无 yargsdefault、仅在显式设置时透传；嵌入式校验接受0、拒绝 NaN/非整数/负数；bridge 对非正/非有限值一律归一为0哨兵（并保留 2^31-1 的 setTimeout 溢出钳制）；mediator 仅在timeoutMs > 0时安装定时器。0真正意味着"无定时器"而非"立即触发"。 - Both interaction kinds provably share one timer boundary.
BridgeClientroutes ordinary permissions and_meta.qwenInteractionKind = "user_question"requests through the samemediator.request(record, this.permissionTimeoutMs)call (bridgeClient.ts:943);isUserQuestion(bridgeClient.ts:284) only shapes the pending-interaction snapshot, never the timeout. The new regression test pins exactly this: it spiesMultiClientPermissionMediator.prototype.request, exercises both kinds, and asserts the timeout argument is0by default and45000underpermissionResponseTimeoutMs: 45_000— the spy's 2nd positional arg matches the realrequest(record, timeoutMs)signature.
两类交互确实共享同一定时器边界。 新增回归测试通过 spy mediator 的request、对两类交互分别断言 timeout 参数为0（默认）与45000（显式覆盖）锁住了这一点，且 spy 的第二位置参与真实签名一致。 - Breadcrumbs and docs were updated symmetrically for both states. The
unresolvedWithoutDecisionstring renders correctly in both the empty-votersAtIssueconsensus breadcrumb and the unanimity/split-vote breadcrumb, for enabled and disabled timer (permissionMediator.ts:451-453); theMediatorDepsJSDoc, the M=2 split-vote doc, the settings-schema consensus description, the protocol doc, and both configuration pages all tell the same story. No stale "5 min / 300000" permission-timeout reference survives anywhere in code or docs (verified by grep).
breadcrumb 与文档在启用/禁用两种状态下对称更新。 全仓 grep 确认代码与文档中不再残留任何"5 分钟 / 300000"的权限超时引用。
Walkthrough notes (verified, no action needed) / 走查记录（已验证，无需处理）
- No test relied on the old 5-minute default:
permissionMediator.test.tsalways passes explicit timeouts; every fake-timer test inbridge.test.tsnear the permission suites exercises channel-idle or MCP-reap timers, and the new test is the only permission-timer test. Nothing hangs under the new default.
没有测试依赖旧的 5 分钟默认值：mediator 测试恒传显式超时，bridge 测试中权限套件附近的假定时器测试均为 channel-idle/MCP-reap 用途。 - The disclosed tradeoff — an unanswered interaction now holds the per-session prompt queue indefinitely — is real but intentional and documented (PR body Risk & Scope plus the updated protocol/config docs), with voter/session cancellation, disconnect cleanup, the 64-pending cap, and shutdown unchanged as exits. Noted here as an acknowledged product decision, not a finding.
PR 披露的权衡（未回答的交互将无限期占用 session 的 prompt 队列）真实存在，但属有意为之且已在 PR 正文与文档中说明，voter/session 取消、断连清理、64 条 pending 上限、shutdown 等退出路径均不变。此处仅作为已确认的产品决策记录，不构成 finding。
🟢 Nit
Stale "5-minute" rationale comment left behind by the default flip.
默认值翻转后遗留了一处过时的"5 分钟"注释。
Location / 位置: packages/acp-bridge/src/bridge.ts:6577
// internal restores (boot rehydrate, keepalive, sub-session resume)
// pass no clientId and must not fabricate a 5-minute permission wait.The requestedClientId === undefined guard is unchanged and still correct, but the wait it prevents is now unbounded, not five minutes — the old number is exactly what this PR removes. Suggest: must not fabricate an unbounded permission wait.
该守卫本身不变且依然正确，但它所避免的等待现在是无期限的而非五分钟——这个"5-minute"字样正是本 PR 移除的默认值。建议改为 must not fabricate an unbounded permission wait.。
Conclusion / 结论
Clean, well-tested default flip; the disable semantics are correctly plumbed from CLI flag through the mediator timer, both interaction kinds are covered by one shared boundary, and docs agree with code. Only the stale comment above. 💬 Comment — no blockers.
一次干净、测试充分的默认值翻转：禁用语义从 CLI flag 到 mediator 定时器全链路正确，两类交互共享同一边界，文档与代码一致。唯一问题是上面那条过时注释。💬 评论——无阻塞项。
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
CI failure and follow-up review feedback have been addressed.\n\n| Item | Action |\n| --- | --- |\n| Settings schema drift | Regenerated the committed VS Code settings schema from the updated settings source in 6de94b0. |\n| Review feedback | @ytahdn's stale "5-minute" rationale comment was updated to match the new unbounded default in 07c2342. |\n| Verification | Confirmed the generator is idempotent, the focused settings schema tests pass (44/44), and git diff --check passes. |\n| Review threads | Resolved 0/0 threads; no review threads were open. |\n\nCurrent head: 07c2342. New CI runs are queued. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Re-run at the author's request — same head (
Moving on to code review. 🔍 中文说明应作者请求重跑——同一 head（
进入代码审查。🔍 — 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 review (third pass, same head
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | success |
| Serve A/B (ubuntu-latest, Node 22.x) | success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | success |
| precheck-pr / precheck | success |
| Test (macos-latest, Node 22.x) | skipped |
| Test (windows-latest, Node 22.x) | skipped |
| Integration Tests (CLI, No Sandbox) | skipped |
| review-config | skipped |
| Desktop Shell (ubuntu-22.04) | success |
| Desktop Shell (windows-2022) | success |
| ubuntu-latest / Java 11 | success |
| ubuntu-latest / Java 17 | success |
| ubuntu-latest / Java 21 | success |
| macos-latest / Java 21 | success |
| windows-latest / Java 21 | success |
| Real daemon E2E / Java 11 | success |
| Dependency CVE audit | success |
| Secret scan (TruffleHog) | success |
| Classify PR | success |
| label | success |
| Remind on force-push | success |
| route (bot orchestration) | success |
| authorize (bot orchestration) | success |
The sandboxed lane named in the last pass has landed and settles the central claim: the completed /verify run on this exact SHA reports merge-ready, 72/72 scripted assertions — the A/B base arm auto-cancels an unanswered permission at 300444ms while the head arm is still pending at 310000ms and then resolves via a human vote; explicit timeouts apply to both interaction kinds on both arms; session/shutdown/voter cancellation all resolve pending requests with the timer disabled; and the mutation matrix proves the new regression test is load-bearing (re-flipping the constant turns it red). It also corrects two PR-body claims: the "blocked typecheck" does not reproduce in a clean container, and the quoted 819/113 test counts are filtered subsets (full acp-bridge suite is 1689/1689 there). A second /verify re-run triggered by this triage is in flight on the same SHA; the first report already covers it.
Real-scenario testing: N/A — unattended CI run; the live-behavior signal comes from the sandboxed lane above.
中文说明
代码审查（第三遍，同一 head 07c2342aaf）：独立基线结论不变——禁用机制已全部就位（permissionMediator.ts:509 的 if (timeoutMs > 0) 守卫、bridge 归一化、启动校验接受 0），最小改法就是翻转 DEFAULT_PERMISSION_TIMEOUT_MS 为 0、同步所有过时引用并用测试锁定新默认值。diff 正是如此——一个常量、对称的文档/breadcrumb 措辞、一个断言 mediator.request 默认收到 0、覆盖时收到 45_000（两类交互均覆盖）的回归测试。
本遍新增的是 /review 运行（以 @wenshao 账号发布的六条 Suggestion 级行内发现）。其中两条含事实性主张，已对照该提交的代码核实；六条均合理，但无一阻塞：
- 文档准确性——已核实，值得后续跟进。（a）
04-permission-mediation.md的request()/N1 工作流小节仍把计时器装配描述为无条件（Responsibilities 条目、timeoutHandle条目、流程图节点 F、N1 理由），与本 PR 在约 90 行后更新的章节自相矛盾——该提交下timeoutMs为0时request()不装配任何计时器，而每个默认 daemon 转发的正是0。（b）协议文档新增句子把 "disconnect cleanup" 列为解决路径，对 REST 驱动方夸大：prompt POST 的close监听在routes/session.ts:4778挂载、准入时（:4851）移除，202 之后 REST 断连不触发任何清理——真正的断连清理属于/acptransport。两者都是措辞修复而非行为修复；设计文档沿用了 (b) 的枚举。 - 测试深度——合理的加固，行为已由其他途径锁定。 mediator 级假时钟测试（传
0、推进时钟、断言仍 pending）锁定的是守卫本身而非参数边界；stderr 捕获测试可锁定新 breadcrumb 文案（该文案的突变在测试套件中存活——verify 运行独立证实了这一点）；flag →ServeOptions→ bridge 往返测试可保护三处构造点。沙箱 A/B 运行已在行为层面验证了真实守卫（head 臂 310 秒后仍 pending），因此这些是堵住未来回归的缺口，而非修复现存缺陷。 - 零观察者楔形（计时器关闭后，无任何已连接观察者的会话发出的权限请求不再自愈）：这正是维护者在批准中接受的权衡——PR 正文与文档已披露，退出路径（cancel 路由、删除会话、shutdown）仍在。开一个后续 issue 讨论为无订阅者情形设界或补充运维 runbook 是合理的，但不是合并条件。
- verify 运行的外观级小项：
Bd1yh:锚点从常量块移除，但bridge.ts:2727仍在引用、bridgeClient.ts仍保留定义。无功能影响。
三遍审查均无正确性阻塞项。
测试证据（通过 API 读取 PR 自己的 CI；本次运行从不执行 PR 代码）：该提交上所有 pull_request 工作流已完成，零失败——Qwen Code CI（含 typecheck/build 的完整单元测试）、Serve A/B、Security Checks、SDK Java，外加 web-shell E2E smoke、precheck、Desktop Shell 双平台、SDK Java 全矩阵、Real daemon E2E、CVE 审计与密钥扫描。macOS/Windows 单元测试与 CLI 集成测试对 fork PR 仍为 skipped。下方表格由 finalize 任务维护。
上一遍点名的沙箱通道已落地并锁定核心主张：该 SHA 上已完成的 /verify 运行判定 merge-ready，72/72 脚本断言——A/B base 臂在 300444ms 自动取消无人应答的权限，head 臂在 310000ms 观察点仍 pending、随后经人工投票解析；显式超时在双臂对两类交互同样生效；计时器禁用时 session/shutdown/voter 取消均能解析 pending 请求；突变矩阵证明新回归测试是承重测试（把常量翻回去测试即变红）。它还纠正了 PR 正文两处说法："被阻断的 typecheck" 在干净容器中不复现；引用的 819/113 测试数是过滤后的子集（该处完整 acp-bridge 套件为 1689/1689）。本次 triage 触发的第二次 /verify 正在同一 SHA 上运行，首份报告已覆盖该提交。
真实场景测试：N/A——无人值守 CI 运行；实时行为信号来自上述沙箱通道。
— Qwen Code · qwen3.8-max
Reviewed at 07c2342aaf44596312a39a5a8cffab1f73867cff · re-run with @qwen-code /triage
|
@qwen-code /triage |
|
Confidence: 4/5 — clean code across three passes, fully green CI, and sandboxed A/B proof of the central claim; the one point short of 5 is a pair of verified doc-accuracy gaps plus test-depth follow-ups, all non-blocking. The last pass deferred with one open question: is flipping a documented default the right product call? That question had a name on it, and the named reviewer answered it — @ytahdn re-reviewed the incremental commits and approved on this exact head, explicitly accepting the tradeoff the deferral described. A deferral that outlives its own resolution is just noise, so this pass acts instead of asking again. Going back to my independent baseline one more time: with the mediator's
So the remaining items are follow-ups, recorded here so they don't evaporate: the two doc corrections (same-page contradiction in Approving, pinned to the reviewed commit. ✅ 中文说明上一遍以一个未决问题作结：翻转文档化默认值是否是正确的产品决策？该问题有明确的决策人，而决策人已给出答案——@ytahdn 增量复查新提交后，在该 head 上批准，并明确接受了 defer 中描述的权衡。一个已超过其解决期限的 defer 只是噪音，因此本遍采取行动而非再次提问。 回到独立基线：在 mediator 的
因此剩余事项为后续跟进，记录于此以免遗漏：两处文档修正（ 批准，锁定至被审查的提交。✅ — Qwen Code · qwen3.8-max Reviewed at |
|
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: 72 passed · 0 failed · 72 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：72 通过 · 0 失败 · 72 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9933 verification —
|
| Cell | Arm | Oracle (observed, not mocked) | Result |
|---|---|---|---|
default (option omitted) |
base | ACP resolution + injected PermissionAuditPublisher + stderr |
auto-cancels at 300444ms; audit {type:'timeout', timeoutMs:300000}; breadcrumb "timed out after 300000ms" — 7/7 |
default (option omitted) |
head | same | still pending at 310000ms (pendingPermissionCount=1, no timeout audit record); then human vote → selected/allow, audit first-responder — 6/6 |
explicit 3000ms, ordinary + user_question |
both | resolution timing + audit timeoutMs |
both kinds auto-cancel at ~3s with timeoutMs=3000 on both arms — 6/6 per arm |
cancel-session / shutdown / voter-cancel |
both | resolution + audit decisionReason |
session-closed / session-closed / voter-cancelled on both arms — 6/6 per arm |
consensus (bridge-level, 1 auto-registered client) |
both | thrown PermissionForbiddenError + pending count + audit |
anonymous selection vote rejected; session cancel still resolves — 3/3 per arm |
consensus-empty (real mediator, empty votersAtIssue via its DI seam) |
both | stderr breadcrumb + vote outcomes + audit | head prints new text "can only resolve via voter or session cancellation because permissionTimeoutMs is disabled"; base prints old "…permissionTimeoutMs (300000ms)"; selection votes forbidden; cancel sentinel resolves despite zero voters and disabled timer — 7/7 per arm |
Witnesses: evidence/01-ab-default-base-auto-cancels-at-300s.png, evidence/02-ab-default-head-pending-past-300s.png, evidence/04-fast-cells-head-vs-base.png. Raw per-cell JSON in logs/cap-*.json (57/57 harness assertions).
The load-bearing flip is exactly the constant: with identical scenario and harness, base cancels at the 5-minute deadline and head waits indefinitely, then honors a human decision.
Reviewer Test Plan walk
| Plan step | Result |
|---|---|
1. No timeout configured; mediator receives 0, no wall-clock timer, both kinds |
✔ head default cell (ordinary kind, behavioral: pending past 300s, no timer audit) + new unit test (pins mediator arg 0 for BOTH kinds — proven non-vacuous by mutation M1) + consensus-empty breadcrumb cell |
| 2. Positive value applies same finite timeout to both kinds | ✔ explicit cell on both arms (3000ms, both kinds); flag side covered by code trace (serve.ts → ServeOptions → run-qwen-serve validation → bridge options) + existing CLI tests (serve.test.ts parse/unset-default, fast-path.test.ts alias, run-qwen-serve.test.ts validation) |
| 3. Selection completes; session cancel / shutdown cancel pending with timer disabled | ✔ head default vote cell; cancel-session and shutdown cells on head (and base) |
Corrections
- The PR description says the full CLI typecheck was "blocked in this worktree by existing Ink selection type mismatches in unchanged UI files". That does not reproduce at the verified head in this container:
tsc --noEmit -p packages/cliexits 0 with zero diagnostics (and the gate-liveness probe below proves the command reports errors when they exist). The author's worktree was presumably stale; no action needed, noted so the next reader doesn't assume a known-broken typecheck. - The description's "819/819 ACP tests / 113/113 CLI tests" are filtered subsets. Full counts at head: acp-bridge package suite 1689/1689 (32 files); the three CLI files covering the changed surface (
serve.test.ts,fast-path.test.ts,run-qwen-serve.test.ts) 424/424.
Findings
F1 (Suggestion, completeness — not a merge condition): the new consensus breadcrumb wording is pinned by no test. Mutation M2 (revert unresolvedWithoutDecision to the base text) survives the full mediator+bridge suites (819/819 collected, all green). The positive control M3 (same file, timer resolution reason mutated) kills 3 tests, so the survival is real, not a dead harness. The wording's behavioral claim (cancellation-only resolution) is correct — my consensus-empty cell verifies the advertised hatches (voter cancel sentinel accepted with zero voters; session cancel resolves) on both arms. A future regression to the message text would ship silently; consider a stderr-capture test alongside the existing "writes a stderr breadcrumb when the timer fires" pattern.
F2 (Nit, cosmetic): comment-anchor drift. The PR removes the Bd1yh: anchor definition from bridge.ts's constant block but leaves the reference comment "Bd1yh + Bd1z5: per-permission deadline + per-session pending cap" (bridge.ts:2727) and the Bd1yh: definition in bridgeClient.ts. No functional impact.
Mutation matrix (vacuity + completeness)
Live re-execution in a scratch worktree at HEAD; witness evidence/03-mutation-matrix-live.png, data in matrix.json.
| Row | Mutation | Suite | Result |
|---|---|---|---|
| R0 | none (baseline) | new bridge test | GREEN 1/1 |
| M1 | DEFAULT_PERMISSION_TIMEOUT_MS 0 → 5601000 |
new bridge test | KILLED — AssertionError: expected 300000 to be +0 (the intended assertion) |
| M2 | breadcrumb wording → base text | mediator + bridge suites (full) | SURVIVED 819/819 → F1 |
| M3 | timeout resolution reason → 'session_closed' (control, same file as M2) |
mediator suite | KILLED (3 tests red) |
M1 proves the new regression test is load-bearing for the central change; M3 proves the suite catches mutations in the file M2 touched.
Not covered
- Per-commit attribution: checkout is depth-2 (merge, base tip, PR head only) while the metadata lists 4 commits; per-commit exercise was out of reach. The aggregate
HEAD^1..HEADdiff is what every cell above drove. - Real
qwen serveHTTP daemon end-to-end (flag on the command line → wire frames): the flag path was verified by code trace plus the CLI unit tests named above; the bridge-level behavior was driven through the package's own in-memory channel fixtures, not a listening socket. ask_user_questionunder the default (disabled) timeout was verified via the new unit test (pins mediator arg 0 for both kinds; non-vacuous per M1) and the shared singlebridgeClient → mediator.requestcall site; the behavioral indefinite-pending cell exercised the ordinary kind. The explicit-timeout cell exercised both kinds behaviorally on both arms.- "Waits indefinitely" is proven by absence of an installed timer (
timeoutMs > 0gate inpermissionMediator.request, no timeout audit record, pending past the old deadline) — not by an infinite observation window (310s). - Windows/macOS; prompt-wide deadline; permission policy, wire schema, and restoration behavior (all declared out of scope by the PR).
- Repo-wide test suite: only the affected packages' suites were run (acp-bridge full; CLI targeted).
Methodology
Environment: CI merge-ref checkout (HEAD=95fc986, base HEAD^1=24db7f6, head HEAD^2=07c2342) in the lane's node:22-bookworm container; npm ci + npm run build pre-ran at head. The A/B harness (ab-harness.mjs in this artifact dir) imports each arm's compiled dist/ (head build as-is; base built from a HEAD^1 worktree via tsc --build with the shared root node_modules) and drives the real createAcpSessionBridge over the package's exported in-memory NDJSON channel fixtures (internal/testUtils), observing outcomes through the ACP response, an injected recording PermissionAuditPublisher (the documented DI seam), pendingPermissionCount, and teed process.stderr — no stub of the code under test. The empty-voters consensus branch is reached through the mediator's own MediatorDeps.votersForSession seam because the bridge auto-registers an anonymous client on spawn. Mutation rows ran in a second scratch worktree with exact-string point mutations, each restored and the tree verified clean. Evidence PNGs rendered with scripts/verify-capture.mjs; raw logs and per-cell JSON under logs/. Assertion inventory: 57 harness + 5 matrix + 1 schema-regeneration byte-identity + 2 suite gates + 2 typechecks + 1 typecheck-liveness probe + 4 control-identity checks = 72.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/acp-bridge/src/bridge.test.ts: (cd packages/acp-bridge) npx --no-install vitest run ./src/bridge.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/acp-bridge/src/bridge.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 2 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 3 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 4 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 5 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Approved ✅
Re-reviewed the incremental change (ecb06d23 → 07c2342a): two lines, both consistency fixes — the stale "5-minute permission wait" comment now reads "unbounded" (as flagged in my previous review), and the vscode-ide-companion settings.schema.json permissionStrategy description is now in sync with settingsSchema.ts (verified character-for-character; manual sync is the correct flow since that schema is excluded from the autofix generation).
The core change was verified end-to-end in the first pass: 0/omitted resolves to the disabled sentinel through CLI flag → runQwenServe validation → bridge clamp → mediator (setTimeout installed only when timeoutMs > 0), ordinary permissions and ask_user_question share the single mediator.request timer boundary, and the regression test pins [0, 0] default vs [45000, 45000] override for both kinds. CI is green on the new head.
The intentional tradeoff (unanswered interactions hold the session prompt queue indefinitely; voter/session cancellation, disconnect cleanup, pending caps, and shutdown remain as exits) is documented in the PR body and the updated docs.
增量复查通过：新提交仅两行一致性修复（过时注释改为 "unbounded"、vscode 伴随扩展的 JSON schema 与 settingsSchema.ts 措辞同步，已逐字核对）。默认禁用超时的完整链路（flag → 校验 → bridge 哨兵 → mediator 条件定时器、两类交互共享边界）已在首轮逐行验证，回归测试锁定默认与显式覆盖两种取值，新 head CI 全绿。无阻塞项，批准。
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View 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. 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 reviewed: reverse audit — stopped before round 6 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 6 轮。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| no option reaches unanimity. `permissionResponseTimeoutMs` is disabled by | ||
| default; when configured, an unresolved split resolves as | ||
| `{cancelled, timeout}` at that deadline. The vote-advance path logs the | ||
| applicable behavior to stderr for operators. |
There was a problem hiding this comment.
[Suggestion] The timer default this paragraph documents was flipped by this PR, but the same page's unchanged request() Workflow section (lines 107–117) still asserts the old behaviour: the flowchart node E --> F["arm setTimeout(timeoutMs) → resolve {cancelled, timeout}"] arms the timer unconditionally, and the rationale sentence claims an entry left "pending with no timeout" hangs the per-session promptQueue forever. Under the new default (DEFAULT_PERMISSION_TIMEOUT_MS = 0 → mediator gate if (timeoutMs > 0) at permissionMediator.ts:509) no timer is armed at all, and requests resolve via voter/session cancellation or forgetSession — exactly as this paragraph and the sibling docs updated here now say, so the page contradicts itself and the code.
A maintainer modifying MultiClientPermissionMediator.request() or debugging a wedged prompt reads the Workflow section and concludes every pending request has an armed wall-clock backstop — e.g. writes code assuming pending.timer is always set (it is undefined under the default config; resolveEntry at permissionMediator.ts:1015 explicitly guards that), or tells an operator the request auto-cancels at a deadline that is never armed.
Branch the flowchart on the gate that exists in code and fix the rationale sentence (in the Workflow section, lines 107–117):
E --> T{"timeoutMs > 0?"}
T -- yes --> F["arm setTimeout(timeoutMs) → resolve {cancelled, timeout}"]
T -- no --> G["no timer — resolves only via voter/session cancellation or forgetSession"]
and state that with the timeout disabled (the default since this PR) entries legitimately carry no timer and the wait is bounded by cancellation paths, not a wall-clock deadline.
中文说明
本段描述的计时器默认值已被此 PR 翻转，但同一页面未修改的 request() Workflow 小节（107–117 行）仍在断言旧行为：流程图节点 E --> F["arm setTimeout(timeoutMs) → resolve {cancelled, timeout}"] 无条件安装计时器，理由句声称“没有超时而 pending”的条目会让 per-session promptQueue 永远挂起。在新默认值下（DEFAULT_PERMISSION_TIMEOUT_MS = 0 → mediator 门控 if (timeoutMs > 0)，permissionMediator.ts:509），根本不会安装计时器，请求通过 voter/session 取消或 forgetSession 解决——正如本段和此 PR 更新的姊妹文档所述，因此该页面与代码自相矛盾。
修改 MultiClientPermissionMediator.request() 或调试卡住的 prompt 的维护者读到 Workflow 小节，会以为每个 pending 请求都有墙钟兜底——例如写出假设 pending.timer 恒已设置的代码（默认配置下它是 undefined；permissionMediator.ts:1015 的 resolveEntry 明确做了防御），或告诉运维请求会在一个从未安装的截止时间后自动取消。
建议在 Workflow 小节（107–117 行）将流程图按代码中实际存在的门控分支，并调整理由句说明：超时禁用（本 PR 起为默认）时条目可以合法地不携带计时器，等待由取消路径兜底，而非墙钟截止时间。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@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: 109 passed · 0 failed · 109 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：109 通过 · 0 失败 · 109 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9933 verification (follow-up round) —
|
| Ref | This round (git rev-parse) |
Previous round cites | Identical |
|---|---|---|---|
| merge (HEAD) | 95fc98685b4a0cb4372cfad69aadfe847641371e |
95fc986 |
✔ |
| base tip (HEAD^1) | 24db7f6ef2e28b1adcc7c613c3ca7b5dd8f81bf4 |
24db7f6 |
✔ |
| PR head (HEAD^2) | 07c2342aaf44596312a39a5a8cffab1f73867cff |
07c2342aaf44596312a39a5a8cffab1f73867cff |
✔ |
Supporting closure checks (scripted, logs/closure-identity.txt): package.json/package-lock.json delta 0 bytes; packages/core delta 0 lines; DEFAULT_PERMISSION_TIMEOUT_MS = 0 present at head and = 5 * 60 * 1000 at base in both source and compiled dist.
There is therefore no delta to scope new probes at; this round re-measured every carried conclusion live instead of diffing the old report.
Previous findings status
| # | Finding (previous round) | Severity | Status at the new head |
|---|---|---|---|
| F1 | New consensus breadcrumb wording pinned by no test (M2 survived the mediator+bridge subset 819/819) | Suggestion | Stands — re-measured live: M2 now survives the full acp-bridge suite 1689/1689 at the new head (stronger than the previous subset); same-file positive control M3 kills 1 test (permissionMediator.test.ts:525), so the harness is live. Behavioral claim of the wording still verified by the consensus-empty cells on both arms. Not a merge condition. |
| F2 | Bd1yh comment-anchor drift: definition removed from bridge.ts constant block, reference remains at bridge.ts:2727, definition survives in bridgeClient.ts |
Nit | Stands — grep at the new head shows the same two occurrences (bridge.ts:2727, bridgeClient.ts:724). Cosmetic only. |
| C1 (correction) | "Full CLI typecheck blocked by Ink selection type mismatches" (PR description) did not reproduce: tsc --noEmit -p packages/cli exit 0 |
correction | Stands — re-run exit 0, plus a liveness probe (planted TS2322 reported at the planted line, exit 2; restored → exit 0). |
| C2 (correction) | Description's "819/819 ACP, 113/113 CLI" are filtered subsets; full counts 1689/1689 and 424/424 | correction | Stands — re-run at the new head: acp-bridge 1689/1689 (32 files); CLI serve/fast-path/run-qwen-serve 424/424 (3 files). |
Central claim + A/B (re-executed live this round)
Central claim: omitting permissionResponseTimeoutMs (or passing 0) disables the shared daemon permission wall-clock for BOTH ordinary permissions and ask_user_question; a positive value (option or --permission-response-timeout-ms) still applies to both.
Control validity (scripted, gates-assert.mjs): base arm built from HEAD^1 in a scratch worktree (tsc --build, 37 s); the only internal workspace dependency of acp-bridge is @qwen-code/qwen-code-core, whose realpath from the root node_modules is /__w/qwen-code/qwen-code/packages/core (head tree) for BOTH arms — clean because the PR's diff against packages/core is 0 lines, so the control differs from head by exactly the hunks under test. The load-bearing constant was asserted literally in both compiled dists (= 0 head / = 5 * 60 * 1000 base).
| Cell | Arm | Oracle (observed through ACP response + injected recording PermissionAuditPublisher + pendingPermissionCount + teed stderr) |
Result |
|---|---|---|---|
default (option omitted, ordinary kind) |
base | resolution, audit, stderr | auto-cancels at 300061 ms; audit recordTimeout + decisionReason {type:'timeout', timeoutMs:300000}; stderr "timed out after 300000ms"; pending drains to 0 — 8/8 |
default (omitted; ordinary and user_question issued) |
head | observation window, then human votes | both kinds still pending at 310030 ms (2/2 pending, zero timeout audits/breadcrumbs in window); then both voted selected/allow, decisionReason first-responder ×2 — 9/9 |
explicit 3000ms, ordinary + user_question |
both | per-kind resolution timing + decisionReason.timeoutMs |
both kinds auto-cancel in [2.9 s, 20 s] with decisionReason.timeoutMs=3000 on both arms — 7/7 per arm |
cancel-session (timer disabled) |
both | resolution + decisionReason |
cancelled; session-closed; no timeout audit — 3/3 per arm |
shutdown (timer disabled) |
both | audit-side unwind (the wire is killed during teardown by design, so the agent-side promise is not the oracle) | audit session-closed at 112 ms; cancelled/session_closed; pending 0; shutdown() completes — 6/6 per arm |
voter-cancel sentinel (timer disabled) |
both | resolution + decisionReason |
cancelled; voter-cancelled; no timeout audit — 3/3 per arm |
consensus (bridge-level, auto-registered anonymous client) |
both | thrown error + pending count + audit | anonymous selection vote throws PermissionForbiddenError (designated_mismatch), request stays pending, session cancel resolves session-closed — 5/5 per arm |
consensus-empty (real mediator, empty votersAtIssue via its votersForSession DI seam, arm default timeout) |
both | stderr breadcrumb + vote outcomes + resolution | head: "…can only resolve via voter or session cancellation because permissionTimeoutMs is disabled"; base: "…can only resolve via permissionTimeoutMs (300000ms)"; selection vote forbidden; cancel sentinel resolves despite zero voters — 6/6 per arm |
Witnesses: evidence/01-ab-assertions-head-vs-base.png (all 89 oracles as printed), evidence/03-long-default-cells-base-vs-head.png (the flip: 300061 ms auto-cancel vs pending-past-310 s-then-voted). Raw per-cell JSON in logs/cell-*.json; harness ab-cell.mjs, oracles ab-assert.mjs.
One harness fix vs the previous round's design, no behavioral difference: recordTimeout receives a PermissionRequestRecord (no timeoutMs field), so the deadline value is asserted from the resolved decisionReason.timeoutMs instead. The shutdown cell additionally records the agent-side wire promise as unsettled-3s by design (channel killed mid-teardown); the audit-side unwind is the documented oracle.
Reviewer Test Plan walk (re-verified)
| Plan step | Result |
|---|---|
1. No timeout configured; mediator receives 0, no wall-clock timer, both kinds |
✔ head default cell exercises BOTH kinds behaviorally past 310 s (an improvement over the previous round, which ran the ordinary kind behaviorally); new unit test pins mediator arg 0 for both kinds, non-vacuous via M1 |
| 2. Positive value applies same finite timeout to both kinds | ✔ explicit 3000ms cell on both arms, both kinds; new unit test pins 45000; flag side covered by the CLI suites (424/424, incl. serve flag parse/validation) |
| 3. Selection completes; session cancel / shutdown cancel pending with timer disabled | ✔ head default vote cells; cancel-session and shutdown cells on head (and base) |
Findings
No new findings. Both carried findings stand (status table above):
- F1 (Suggestion, stands): the breadcrumb wording is pinned by no test — M2 survived 1689/1689 at the new head while same-file control M3 was killed, so a future regression to the message text would ship silently. The wording's behavioral claim is correct (verified by the
consensus-emptycells). Consider a stderr-capture test alongside the existing "writes a stderr breadcrumb when the timer fires" pattern. - F2 (Nit, stands):
Bd1yhanchor reference atbridge.ts:2727outlives the anchor's definition site (now onlybridgeClient.ts:724). Cosmetic.
Mutation matrix (re-executed at the new head)
Live re-execution in a scratch worktree at HEAD; witness evidence/02-mutation-matrix-live.png; data logs/matrix.json, raw logs logs/matrix-*.txt|log.
| Row | Mutation | Suite | Result |
|---|---|---|---|
| R0 | none (baseline) | new bridge test (-t filter) |
GREEN 1/1 — 2 checks (green + tree unmutated) |
| M1 | DEFAULT_PERMISSION_TIMEOUT_MS 0 → 5·60·1000 |
new bridge test | KILLED — expected 300000 to be +0 // Object.is equality (the intended mediator-arg assertion) — 2 checks (kill + restore) |
| M2 | unresolvedWithoutDecision → base text permissionTimeoutMs (Nms) |
full acp-bridge suite | SURVIVED 1689/1689 → F1 stands (previous round: 819-test subset) |
| M3 | timeout decisionReason → {type:'session-closed'} (control, same file as M2) |
full acp-bridge suite | KILLED — 1 failed | 1688 passed at permissionMediator.test.ts:525 (expected { type: 'session-closed' } to match object { type: 'timeout', …}) |
Not covered
- Per-commit attribution: depth-2 checkout (merge, base tip, PR head only);
git rev-list --count HEAD^1..HEAD^2returns 1 at the shallow boundary while the metadata lists 4 commits — per-commit exercise out of reach, as in the previous round. All cells drove the aggregateHEAD^1..HEADdiff. - Real
qwen serveHTTP daemon end-to-end (flag on the command line → wire frames): flag path covered by the CLI suites + code trace (serve.ts→ServeOptions→run-qwen-servevalidation → bridge options); bridge behavior driven through the package's in-memory channel fixtures, not a listening socket. Carried on identical closure; no new probe added (zero delta). - "Waits indefinitely" beyond the 310 s window is proven by absence of an installed timer (mediator arms
setTimeoutonly whentimeoutMs > 0; zero timeout audits; pending past the old deadline), not by an infinite observation window. - Windows/macOS; repo-wide test suite (affected packages only); prompt-wide deadline, permission policy, wire schema, and restoration behavior (declared out of scope by the PR).
- The flakiness gate (changed test file × 5 rounds) is owned by the workflow, not this agent run.
Methodology
Environment: CI merge-ref checkout (HEAD=95fc98685b, base HEAD^1=24db7f6ef2, head HEAD^2=07c2342aaf) in the lane's node:22-bookworm container (Node v22.23.2); npm ci + npm run build pre-ran at head. All numbers above come from scripted checks executed this round: ab-assert.mjs (89), matrix-assert.mjs (6), gates-assert.mjs (14) — raw exit-code/JSON evidence under logs/, harnesses kept in this artifact dir for rerun.
The A/B harness (ab-cell.mjs) runs each cell in a separate node process importing the arm's compiled dist (head: packages/acp-bridge/dist; base: tmp/base-tree/packages/acp-bridge/dist, built from the HEAD^1 worktree) and drives the real createAcpSessionBridge over the package's exported in-memory NDJSON channel fixtures (internal/testUtils — makeBridge/makeChannel/FakeAgent), observing outcomes through the ACP response, an injected recording PermissionAuditPublisher (the documented DI seam), pendingPermissionCount, and teed process.stderr — no stub of the code under test. The empty-voters consensus branch is reached through the mediator's own MediatorDeps.votersForSession seam.
One environment observation, not a PR finding: building the base control with plain tsc --build reports 63 pre-existing type-level errors confined to packages/core (ajv type imports; byte-identical file on both arms, 0-line core diff; CI builds via scripts/build_package.js). All errors are type-level, the emitted JS was produced, and both arms resolve the same head-tree core dist at runtime (realpath check above), so control purity holds. The base acp-bridge sources themselves compiled with zero errors. Mutation rows ran in a second scratch worktree with exact-string point mutations, each restored and the tree verified clean (git status --porcelain empty after every row); the main working tree finished clean. Evidence PNGs rendered with scripts/verify-capture.mjs.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/acp-bridge/src/bridge.test.ts: (cd packages/acp-bridge) npx --no-install vitest run ./src/bridge.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/acp-bridge/src/bridge.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 2 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 3 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 4 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 5 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— glm-5.3 via Qwen Code /review (v0.22.0)
| // Human permissions wait indefinitely by default and still resolve through | ||
| // voter cancellation, session cancellation, and shutdown. Operators can set | ||
| // `BridgeOptions.permissionResponseTimeoutMs` when they need a wall-clock cap. | ||
| const DEFAULT_PERMISSION_TIMEOUT_MS = 0; |
There was a problem hiding this comment.
[Suggestion] With the timer off by default, a permission issued from a session with no connected observer has no automatic resolution path at all — the exact pathology the deleted comment here used to document. Traced leg by leg: the prompt POST's close listener is removed in the same synchronous block as admission (routes/session.ts:4851 — no disconnect after the 202 does anything); an SSE subscriber's disconnect runs telemetry-only finalize (sse-events.ts); the idle reaper vetoes the session forever while a prompt is pending (entryIsAutoCloseCandidate → entryHasLocalWork); and last-client detach holds the session open. So a headless client that POSTs a prompt and exits leaves the session slot plus its ACP child pinned indefinitely — before this PR that wedge self-healed in ≤5 minutes. The PR body discloses the indefinite hold for an unanswered human, which is fair — but in the no-observer state nobody will ever answer, and the only exits are operator-initiated (cancel route, session delete, restart). Consider bounding the zero-observer case (e.g. arm a fallback timeout when the issuing session has zero subscribers, mirroring the /acp transport's graceTimer), or documenting the operator recovery as the required runbook step next to the new default.
中文说明
[Suggestion] 默认关闭计时器后，从没有任何已连接观察者的会话发出的权限请求完全不存在自动解决路径——这正是此处被删除的注释曾经记录的病理。逐条核实：prompt POST 的 close 监听在准入的同一个同步代码块内被移除（routes/session.ts:4851——202 之后任何断连都不起作用）；SSE 订阅者断连只执行遥测性质的 finalize（sse-events.ts）；空闲回收器在有 pending prompt 时永久否决该会话（entryIsAutoCloseCandidate → entryHasLocalWork）；最后一个客户端脱离时会话保持打开。因此，POST 一个 prompt 后即退出的无头客户端会把会话槽位及其 ACP 子进程无限期钉死——本 PR 之前该楔形可在 5 分钟内自愈。PR 正文披露了"未回答的人工交互无限期等待"，这没问题——但在无观察者状态下永远不会有人来回答，唯一出口是运维主动操作（cancel 路由、删除会话、重启）。建议为无观察者情形设置边界（例如在发起会话零订阅者时装配回退超时，镜像 /acp 传输的 graceTimer），或在新默认值旁把运维恢复手段写成必需的 runbook 步骤。
— glm-5.3 via Qwen Code /review (v0.22.0)
| expect(mediatorRequest.mock.calls[firstRelevantCall]?.[1]).toBe( | ||
| expectedTimeoutMs, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The disabled default is pinned only at the argument boundary: the spy intercepts mediator.request before the component that owns the timer runs, and no test anywhere passes timeoutMs = 0 into the mediator and advances the clock (all ~42 calls in permissionMediator.test.ts pass explicit 5_000). Mutation-proven at this commit: flipping the mediator's guard to if (timeoutMs > 0 || true) with a 300 000 ms fallback delay keeps the entire acp-bridge suite green (32 files, 1689 tests) — every default-configured daemon would silently regain 5-minute cancellation of unattended permissions, the exact behavior this PR removes. A mediator-level fake-timer test closes it:
const p = mediator.request(record, 0);
await vi.advanceTimersByTimeAsync(10 * 60_000);
expect(mediator.pendingCount).toBe(1); // still pending — no timer armedAcceptance criterion: mutate the timeoutMs > 0 guard at permissionMediator.ts:509 to arm a 300 000 ms fallback for 0 — that new test must go red (today the full suite stays green under exactly that mutation).
中文说明
[Suggestion] 禁用默认值只在参数转发边界被固定：spy 在拥有计时器的组件运行之前就拦截了 mediator.request，且整个测试集中没有任何一处向 mediator 传入 timeoutMs = 0 并推进时钟（permissionMediator.test.ts 全部 ~42 处调用都显式传 5_000）。已在本提交用突变证实：把 mediator 守卫改为 if (timeoutMs > 0 || true) 并配 300 000 ms 回退延迟后，整个 acp-bridge 套件依然全绿（32 个文件、1689 个测试）——所有默认配置的 daemon 会默默恢复对无人应答权限的 5 分钟取消，恰是本 PR 要移除的行为。补一个 mediator 级假时钟测试即可堵住：mediator.request(record, 0) 后推进 10 分钟，断言请求仍为 pending。
验收标准：把 permissionMediator.ts:509 的 timeoutMs > 0 守卫改为对 0 装配 300 000 ms 回退——该新测试必须变红（目前整套测试在该突变下保持全绿）。
— glm-5.3 via Qwen Code /review (v0.22.0)
| const unresolvedWithoutDecision = | ||
| timeoutMs > 0 | ||
| ? `voter cancellation, session cancellation, or permissionTimeoutMs (${timeoutMs}ms)` | ||
| : 'voter or session cancellation because permissionTimeoutMs is disabled'; |
There was a problem hiding this comment.
[Suggestion] The new unresolvedWithoutDecision ternary feeding both operator stderr breadcrumbs is new logic in this diff with zero test assertions anywhere — while this file's own test suite pins exactly this class of breadcrumb (the timer-fire breadcrumb at permissionMediator.test.ts:551-577, the three forbidden-vote breadcrumbs at :836+). If the ternary inverts or the disabled-branch string goes stale in a refactor, an operator in exactly the wedged state this PR makes permanent reads guidance claiming a timer will clear it when no timer exists. Please extend the breadcrumb tests to drive the empty-voters and unanimity paths once with timeoutMs = 0 and once with 5_000, asserting the two distinct strings.
Acceptance criterion: swap the ternary's two branches (or hardcode the >0 string) — that breadcrumb test must go red; today nothing asserts either string.
中文说明
[Suggestion] 为两条运维 stderr breadcrumb 供值的 unresolvedWithoutDecision 三元表达式是本 diff 新增的逻辑，却没有任何测试断言——而本文件的测试套件恰好有固定此类 breadcrumb 的先例（permissionMediator.test.ts:551-577 固定计时器触发 breadcrumb，:836+ 固定三条禁止投票 breadcrumb）。若日后三元表达式被反转、或禁用分支文案在重构中过时，处于本 PR 使其永久化的楔形状态的运维会读到"计时器会清除"的指引，而实际并不存在计时器。建议扩展 breadcrumb 测试：对空投票人与 unanimity 两条路径分别以 timeoutMs = 0 和 5_000 各驱动一次，断言两种不同的文案。
验收标准：交换三元表达式两个分支（或硬编码 >0 的文案）——该 breadcrumb 测试必须变红；目前没有任何测试断言这两条文案。
— glm-5.3 via Qwen Code /review (v0.22.0)
| const configured = await setupForPermission({ | ||
| permissionResponseTimeoutMs: 45_000, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The new test stops at BridgeOptions: the flag → ServeOptions → the three run-qwen-serve.ts conditional spreads (:4361/:4806/:5376) → server.ts:1054 → bridge leg of "explicit overrides keep working" has no behavioral test — CLI coverage is parse, flag-mapping, and NaN-rejection only. If a future refactor drops the spread on one of the three construction sites, the flag still parses green while that daemon flavor silently ignores the operator's cap — and with the new default 0 the consequence changed from a 5-minute fallback to an unbounded wait. The sibling sessionRestoreTimeoutMs round-trips behaviorally through the daemon-status limits payload (run-qwen-serve.test.ts:1519-1549); consider surfacing permissionResponseTimeoutMs there too and asserting the same round-trip.
Acceptance criterion: delete the conditional spread at run-qwen-serve.ts:4361 — that round-trip test must go red; today no test in packages/cli references a positive value reaching a constructed bridge.
中文说明
[Suggestion] 新测试止步于 BridgeOptions："显式覆盖仍然生效"的 flag → ServeOptions → run-qwen-serve.ts 三处条件展开（:4361/:4806/:5376）→ server.ts:1054 → bridge 这一段没有行为测试——CLI 覆盖只有解析、flag 映射和 NaN 拒绝。若日后重构丢掉了三处构造点之一的展开，flag 仍然解析通过，而该 daemon 形态会默默忽略运维设置的上限——且新默认值 0 使后果从 5 分钟回退变为无限等待。兄弟选项 sessionRestoreTimeoutMs 已通过 daemon-status limits 载荷做行为级往返断言（run-qwen-serve.test.ts:1519-1549）；建议把 permissionResponseTimeoutMs 也暴露进去并做同样的往返断言。
验收标准：删除 run-qwen-serve.ts:4361 的条件展开——该往返测试必须变红；目前 packages/cli 中没有任何测试断言正值到达构造出的 bridge。
— glm-5.3 via Qwen Code /review (v0.22.0)
| > decision. Voter cancellation, session cancellation, disconnect cleanup, and | ||
| > daemon shutdown still resolve pending interactions as cancelled. |
There was a problem hiding this comment.
[Suggestion] This sentence (and retained item (c) above) lists "disconnect cleanup" as a resolution path — but no disconnect resolves a pending interaction on the REST/SSE surface this protocol documents: the prompt POST's close listener is removed in the same synchronous block as admission (routes/session.ts:4851), an SSE subscriber's disconnect runs telemetry-only finalize, and the real disconnect cleanup (abandonPendingForSession, promptAbort/graceTimer) belongs to the /acp JSON-RPC transport, which this doc doesn't cover. A REST integrator who drops their connection as their cleanup step gets a silently-never-firing cleanup — and with the default now 0, the pin is indefinite instead of 5-minute-bounded. The same enumeration in the new design doc inherits the issue.
| > decision. Voter cancellation, session cancellation, disconnect cleanup, and | |
| > daemon shutdown still resolve pending interactions as cancelled. | |
| > decision. Voter cancellation, session cancellation, and daemon shutdown | |
| > still resolve pending interactions as cancelled (on the /acp JSON-RPC | |
| > transport, a driver disconnect also cancels after the reconnect grace | |
| > window; a REST prompt driver gets no disconnect cleanup once the 202 is | |
| > returned — cancel via POST /session/:id/cancel). |
中文说明
[Suggestion] 这句话（以及上方保留的条目 (c)）把 "disconnect cleanup" 列为解决路径——但在本文档所描述的 REST/SSE 表面上，任何断连都不会解决 pending 交互：prompt POST 的 close 监听在准入的同一同步块内被移除（routes/session.ts:4851），SSE 订阅者断连只执行遥测 finalize，而真正的断连清理（abandonPendingForSession、promptAbort/graceTimer）属于本文档未覆盖的 /acp JSON-RPC 传输。以断开连接作为清理步骤的 REST 集成方会得到一个永远不会触发的清理——且默认值改为 0 后，钉死从 5 分钟有界变为无限期。新设计文档中的相同枚举同样继承此问题。
— glm-5.3 via Qwen Code /review (v0.22.0)
| voter cancellation, session cancellation, or the optional interaction timeout: | ||
| no option reaches unanimity. `permissionResponseTimeoutMs` is disabled by | ||
| default; when configured, an unresolved split resolves as |
There was a problem hiding this comment.
[Suggestion] The request()/N1 workflow section ~90 lines above this paragraph still documents unconditional timer arming — the flowchart node at :111 (arm setTimeout(timeoutMs) → resolve {cancelled, timeout}), the Responsibilities bullet at :27, the timeoutHandle bullet at :94, and the :117 rationale ("Without this … the entry pending with no timeout — the bridge's per-session promptQueue would hang forever"). At this commit request() arms nothing when timeoutMs is 0, and 0 is what every default-configured daemon forwards — so the documented workflow step never executes by default and this page contradicts itself (:207 says disabled by default; :111 says request() always arms). Worth making the flowchart's arm step conditional and scoping the N1 sentence and bullets to "when a timeout is configured (default 0 = no timer)" — while editing node F, its F→G order (arm before pending.set) also doesn't match the code's actual order (pending.set at permissionMediator.ts:446 precedes the setTimeout at :509).
中文说明
[Suggestion] 本段落上方约 90 行的 request()/N1 工作流小节仍把计时器装配描述为无条件行为——:111 的流程图节点（arm setTimeout(timeoutMs) → resolve {cancelled, timeout}）、:27 的 Responsibilities 条目、:94 的 timeoutHandle 条目，以及 :117 的理由（"Without this … the entry pending with no timeout — the bridge's per-session promptQueue would hang forever"）。而在当前提交，timeoutMs 为 0 时 request() 不会装配任何计时器，且每个默认配置的 daemon 转发的正是 0——被文档化的工作流步骤默认情况下从不执行，本页自相矛盾（:207 说默认禁用；:111 说 request() 总是装配）。建议把流程图的装配步骤改为条件分支，并将 N1 句子和两个条目限定为"配置了超时时（默认 0 = 无计时器）"——顺便一提，编辑节点 F 时可一并纠正其 F→G 顺序（先装配后 pending.set）与代码实际顺序（permissionMediator.ts:446 的 pending.set 先于 :509 的 setTimeout）不符的问题。
— glm-5.3 via Qwen Code /review (v0.22.0)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — defaulting daemon permission timeout off matches the intended behavior for long-running sessions; CI is green and the prior review round raised no blocking issues.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved after review.
- Default flip verified:
DEFAULT_PERMISSION_TIMEOUT_MS5min -> 0;bridge.tsnormalization keeps only finite positive values as a live timer (clamped to 2^31-1), everything else resolves to0= disabled. - Mediator verified:
setTimeoutis gated behindtimeoutMs > 0, so with the default no wall-clock timer is armed; vote / voter-cancel / session-cancel / forgetSession / shutdown resolution paths are untouched. - The new bridge test pins both interaction kinds (ordinary permission +
_meta.qwenInteractionKind = user_question) at0by default and45_000with an explicit override. - Docs, CLI flag help, settings schema and JSON schema descriptions all updated consistently with the new default.
The 7 open threads are Suggestion-level (doc wording + extra coverage ideas), none blocking.
Note for operators: with the timer disabled, a permission issued with no connected observer now waits indefinitely; the existing cancel paths remain the recovery mechanism. Set --permission-response-timeout-ms if a wall-clock cap is needed.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ The product question from the earlier deferral was resolved by @ytahdn's approval on this commit; CI is fully green and the sandboxed A/B run proves the default flip end to end. Non-blocking doc/test follow-ups are recorded in the Stage 3 comment.
5 participants
What this PR does
This PR disables the shared daemon permission-response wall-clock timer by default. When
permissionResponseTimeoutMsis omitted or set to0, both ordinary permission requests andask_user_questionrequests remain pending until a human decision or lifecycle cancellation. A positive value supplied through the existing bridge option or--permission-response-timeout-msflag still applies to both interaction kinds. The documentation and consensus-policy breadcrumbs now describe the disabled-timeout behavior, and regression coverage verifies the shared default and explicit override.Why it's needed
The previous five-minute default could cancel long-running daemon questions before a user returned to answer them. Daemon interactions should wait for the user by default while preserving an explicit operator-controlled deadline for deployments that need bounded waits.
Reviewer Test Plan
How to verify
ask_user_questionrequest with no permission-response timeout configured; confirm the mediator receives0for both and neither request installs a wall-clock timeout.permissionResponseTimeoutMsor--permission-response-timeout-msvalue; confirm the same finite timeout applies to both interaction kinds.Evidence (Before & After)
Before: omitting
permissionResponseTimeoutMsresolved the shared mediator timeout to300000ms for both ordinary permissions andask_user_question.After: the focused bridge regression observes timeout arguments
[0, 0]for the two default interaction paths and[45000, 45000]when an explicit override is configured. The ACP bridge build passes, the affected ACP tests pass 819/819, and the affected CLI tests pass 113/113.Tested on
Environment (optional)
macOS Darwin 25.4.0 arm64, Node.js v22.22.3, npm 10.9.8, ACP in-memory transport tests.
Risk & Scope
selectable,selectionFlow, and missing frame exports); focused CLI tests pass. No permission policy, wire schema, restoration behavior, or prompt-wide deadline changes are included.--permission-response-timeout-ms 300000. No new environment variable or migration step is introduced.Linked Issues
N/A
中文说明
此 PR 的作用
此 PR 默认关闭 daemon 共享的权限响应墙钟计时器。当省略
permissionResponseTimeoutMs或将其设置为0时，普通权限请求和ask_user_question请求都会保持等待，直到人工作出决定或会话生命周期取消。通过现有 bridge option 或--permission-response-timeout-msflag 提供的正数仍会同时作用于两类交互。文档和 consensus policy 的 breadcrumb 已更新为描述禁用超时后的行为，并通过回归测试验证共享默认值和显式覆盖。为什么需要
之前的五分钟默认值可能会在用户回来回答之前取消长时间运行的 daemon 问题。daemon 交互应默认等待用户，同时为需要有界等待的部署保留显式、可由运维控制的截止时间。
Reviewer 测试计划
如何验证
ask_user_question请求；确认 mediator 对两者都收到0，且两者都不会安装墙钟计时器。permissionResponseTimeoutMs或--permission-response-timeout-ms重复验证；确认相同的有限超时同时作用于两类交互。证据（变更前后）
变更前：省略
permissionResponseTimeoutMs时，普通权限和ask_user_question的共享 mediator timeout 都会解析为300000毫秒。变更后：聚焦 bridge 回归测试观察到两条默认交互路径的 timeout 参数为
[0, 0]，显式配置覆盖时为[45000, 45000]。ACP bridge 构建通过，受影响的 ACP 测试 819/819 通过，受影响的 CLI 测试 113/113 通过。测试平台
环境（可选）
macOS Darwin 25.4.0 arm64、Node.js v22.22.3、npm 10.9.8、ACP 内存 transport 测试。
风险与范围
selectable、selectionFlow和缺失的 frame export）；聚焦 CLI 测试通过。不包含 permission policy、wire schema、恢复行为或 prompt-wide deadline 的修改。--permission-response-timeout-ms 300000。没有新增环境变量，也不需要迁移步骤。关联 Issue
N/A