feat(serve): hide workspace Browse on headless daemon hosts - #9406
feat(serve): hide workspace Browse on headless daemon hosts#9406qqqys wants to merge 36 commits into
Conversation
The channel worker supervisor always handed workers an http:// loopback
URL, and workers rejected any other scheme, so on a daemon started with
--tls-cert/--tls-key the worker's first capabilities fetch hit the
HTTPS-only listener as plain HTTP and died with "fetch failed" before
reporting ready ("Channel worker exited before ready (code=1)").
- Emit an https:// loopback URL for the worker when TLS is configured
- Accept https loopback in the worker's QWEN_DAEMON_URL validation
- Inject NODE_EXTRA_CA_CERTS with the daemon cert into the worker env
(merged with an operator-set value, since it accepts a single file)
Round 1 review found three ways the CA injection this PR adds silently fails to give channel workers a usable trust anchor (R1-1, R1-2, R1-3), plus the diagnosability and coverage gaps around it (R1-4..R1-8). - R1-1: `--tls-cert` was forwarded to the worker verbatim. Workers are forked with `cwd: opts.workspace`, so a relative path resolved against the worker's cwd instead of the daemon's, Node silently ignored the unloadable extra cert, and every handshake failed DEPTH_ZERO_SELF_SIGNED_CERT — the exact pre-PR symptom. Resolve once at the source, next to the read that already validated it. - R1-2: the merged CA bundle went to `os.tmpdir()/qwen-worker-ca-<pid>.pem`, a path predictable from the daemon PID (CWE-377/CWE-59). A pre-planted symlink redirected the write; a pre-planted regular file kept attacker ownership and mode while receiving the full cert — the private key too, for a combined PEM. Write into an `mkdtempSync` 0700 directory instead, the same defence standalone-update.ts already uses in this tmpdir. - R1-3/R1-4: a serving cert only anchors trust when it signed itself, and only reaches the worker when its SANs cover the loopback host workers dial. Neither held for the `mkcert` flow this project documents, and boot validation checked parse/expiry/validity-window only — so the daemon booted green, browsers connected, and every worker restart-looped with /health still green. `describeWorkerTlsTrustGaps` names both at boot, the way the adjacent expiry guard does. The non-self-signed check stays quiet when the operator set NODE_EXTRA_CA_CERTS, since that value is merged into the worker bundle and may already carry the issuing root. - R1-5: the merge-failure `catch` dropped the operator-set NODE_EXTRA_CA_CERTS with no diagnostic, and Node stays silent when the remaining cert loads fine. Emit a process warning naming both paths. - R1-6: the bundle was never cleaned up. Merged bundles are now memoized per (operator CA, daemon cert) pair — workers respawn on every restart, so minting a directory per spawn would leak one per restart — and removed on daemon exit. - R1-7/R1-8: tests for the merge-failure fallback and for the `workerTlsCaCertPath` pass-through, plus an end-to-end test that boots the daemon with a relative `--tls-cert` and asserts the supervisor gets an absolute path and an https daemon URL. Verification: every fix was mutation-checked — reverting `path.resolve`, the mkdtemp write, the trust-gap detection, the merge-failure warning, the group pass-through, and the bundle memoization each turns at least one new test red. `npx vitest run src/serve/run-qwen-serve.test.ts src/serve/channel-worker-supervisor.test.ts src/serve/channel-worker-group.test.ts` → 403 passed. eslint and prettier clean on the six touched files.
… reads The `Test (ubuntu-latest, Node 22.x)` job failed on 04c954d with a single red test: `serve process.env guard > allows only documented process-scoped process.env expressions`. 04c954d added the worker TLS trust-gap check, which reads `process.env['NODE_EXTRA_CA_CERTS']` twice in run-qwen-serve.ts (once to test for it, once to pass it), but did not add the matching entry to `allowedProcessEnvAccesses`. The guard is an explicit allowlist, so any undeclared process-scoped read is a failure by design. Declare `key:NODE_EXTRA_CA_CERTS: 2` and record why this particular read is process-scoped rather than request-scoped: NODE_EXTRA_CA_CERTS is the trust store Node already loaded for this process, so the check has to consult the same value to know whether the operator has already supplied the issuing CA. Mutation-verified: with the count at 1 instead of 2 the guard test goes red with the same mismatch shape, so the allowlist is genuinely counting the occurrences and not just matching the key.
The Web Shell "Add workspace" dialog shows a Browse button that opens a native OS directory picker on the daemon host (osascript on macOS, PowerShell on Windows, zenity on Linux). On headless hosts the picker can never open — zenity exits with "cannot open display" — so the button only surfaces a guaranteed error toast on every click. - Add isNativeDirectoryPickerAvailable: macOS/Windows always pass; Linux requires DISPLAY/WAYLAND_DISPLAY plus an executable zenity file on PATH (a directory named zenity does not count) - Advertise a new conditional native_directory_picker serve feature only when the probe passes; the bootstrap reduced set omits it like its sibling registration tags, which fails closed - Web Shell passes onPick to the Add workspace dialog only when the feature is present, so the button is hidden on headless hosts - Tolerate the host-conditional tag in the integration capabilities snapshot and register the new process.env access with the guard
github-actions
Bot
added
the
review/self-reported
|
Re-run at the new head — the PR changed shape materially since the last pass, so a fresh gate read. Template looks good ✓ Problem: still observed, not theoretical. On a headless daemon host every Browse… click is a deterministic Direction: still aligned — gating a UI affordance on a conditional serve capability is the established pattern here, and the sibling features wired the same way have grown since the last pass. What changed since the last gate: the undocumented TLS half is gone. It went to its own PR (#9392), went through its own review cycle, and was merged to main by @wenshao on Aug 25; this branch picked it back up through the main merges. What remains beyond the picker is the channel-worker tail the review rounds produced in the meantime (~273 production lines): family-aware worker loopback mapping, accepting a bind on one of this host's own interfaces, a boot guard that refuses a bind no worker could reach, and a named diagnostic for zone-scoped binds. Each is a reproducible defect, not hardening — e.g. R10-1: an empty Size: not core infrastructure ( Approach: the picker half is unchanged and still matches what I'd propose independently. The tail is coherent ("channel workers can reach whatever the daemon bound") but is still not described in the PR body — the one standing process nit from the last pass, now much smaller. The Risk: no elevated signals; nothing in the diff matches the revert-correlated paths. Moving on to code review. 🔍 中文说明在新 head 上重跑——上次评审之后 PR 的形态变化很大，重新过一遍门禁。 模板完整 ✓ 问题：仍然是已观测到的真实问题，不是理论性的。headless daemon 主机上每次点击「浏览…」都是确定性的 方向：仍然对齐——用条件式 serve capability 门控 UI 入口是这里的既有模式，上次评审之后按同样方式接线的同族 feature 还在增加。 上次门禁之后的变化：未记载的 TLS 部分已经不在了。它进了自己的 PR（#9392），走完自己的评审周期，8 月 25 日由 @wenshao 合入 main；本分支通过 main 合并重新带上了它。picker 之外剩下的是评审轮次在此期间产出的 channel-worker 收尾（约 273 行生产代码）：按地址族选择 worker 回环地址、接受本机自有网卡上的 bind、启动时拒绝 worker 无法到达的 bind、为带 zone 的 bind 给出命名诊断。每一项都是可复现的缺陷而非加固——例如 R10-1：无 IPv6 主机上空 规模：非核心基础设施（ 方案：picker 部分未变，仍与我的独立方案一致。收尾部分自洽（"channel worker 能拨通 daemon 实际绑定的地址"），但 PR 正文仍未描述它——上次遗留的唯一流程问题，现在小多了。 风险：无升级信号；diff 未命中任何 revert 相关路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewPicker capability (the described feature): unchanged in approach since the last pass and still clean — registry entry → toggle → predicate → boot-time probe in Channel-worker tail (the part the body doesn't describe): reviewed as its own change. No correctness, security, or convention blockers in either half. The standing Files changed (21 of 21 shown)
TestingLocal invocation, but this environment denies executing Node outright (permission rule
Sandboxed verification can still add what static review and CI cannot: Not verified: browser-rendered dialog on a live daemon (local execution denied by environment; closest CI oracles above), and GUI-host picker behavior on macOS/Windows (no GUI in CI; author marked both untested).
中文说明代码审查picker capability（PR 描述的功能）：方案与上次评审一致，仍然干净——registry 条目 → toggle → predicate → channel-worker 收尾（正文未描述的部分）：作为独立改动审查。 两半均无正确性、安全性或规范层面的阻塞问题。透明的说明现有 测试本地调用，但本环境直接拒绝执行 Node（权限规则
沙箱验证仍可补上静态审查与 CI 补不了的： 未验证：真实 daemon 上浏览器渲染的对话框（环境拒绝本地执行；最接近的 CI oracle 见上）；macOS/Windows 上 GUI 主机的选择器行为（CI 无 GUI，作者标记未测）。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — both halves are genuinely good and everything testable on this head is green; the only open items are a stale PR body, a deferral list, and ledger entries that now live on main. Stepping back: last time I parked this at 3/5 with one question for the maintainers — the bundled, undocumented TLS fix: accept it described, or split it. That question was answered by action rather than reply: the TLS surface went to #9392, went through its own review cycle, and was merged to main by @wenshao on Aug 25. What this PR is now, my independent proposal matches point for point on the picker half (boot-time probe → conditional tag → client wires The case for shipping: all CI green on this head including the Serve A/B (zero response changes) and the web-shell E2E smoke; the only red family in the neighborhood — the visuals scenario render — I traced into the job log and proved pre-existing (the base leg fails the identical ambiguous Non-blocking follow-ups, named so nothing is silently dropped:
Approving, pinned to the reviewed commit. A fork PR still needs maintainer sign-off to merge, and the 中文说明置信度：4/5 —— 两半改动都很好，此 head 上一切可测项皆绿；剩下的只有过时的 PR 正文、一份延后清单，以及几条现已落在 main 上的台账条目。 退一步看：上次我把这个 PR 停在 3/5，留给维护者一个问题——夹带且未记载的 TLS 修复：要么描述后接受，要么拆分。这个问题被行动而非回复回答了：TLS 表面进了 #9392，走完自己的评审周期，8 月 25 日由 @wenshao 合入 main。现在的这个 PR，我的独立方案在 picker 一半上逐点吻合（启动探测 → 条件 tag → 客户端仅在广播时接 支持合入的理由：此 head 全部 CI 绿色，包括 Serve A/B（响应零变化）与 web-shell E2E smoke；附近唯一的红色家族——visuals 场景渲染失败——我追进任务日志证明是预先存在的（base 侧在 merge-base 非阻塞跟进项，点名以免被悄悄丢弃：
批准，钉在受审提交上。fork PR 合并仍需维护者签字；如果想要端到端行为证据，Stage 2 点名的 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 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 |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
R2-1, R2-2, R2-5 from review round 2.
R2-1. `workerDialHost` returned WHATWG `URL.hostname`, which keeps the brackets
on an IPv6 literal (`[::1]`). `isIP('[::1]')` is 0, so `certCoversHost` took the
DNS-name branch and `checkHost('[::1]')` could never match the iPAddress SAN the
certificate actually carries — the boot diagnostic false-positived on every TLS
daemon bound to `::1` with a correct cert, and told the operator to reissue it.
The brackets are now stripped, so the address is checked as an address and also
printed unbracketed the way a SAN spells it.
R2-2. `describeWorkerTlsTrustGaps` built one `X509Certificate` from the file,
which reads only the FIRST PEM block. A standard `fullchain.pem` (leaf +
issuing CA) was therefore judged on its leaf alone and reported as unable to
anchor worker trust — even though the supervisor injects that same whole file
as the workers' `NODE_EXTRA_CA_CERTS`, root included, so trust does establish.
The file is now split into every certificate it carries and the leaf's chain is
walked through them; the gap is reported only when the chain fails to terminate
in a self-signed certificate inside the file. A leaf-only file still reports it.
The walk is bounded by a fingerprint set, so a cross-signed pair cannot loop.
R2-5. The merged-CA-bundle test asserted `toContain('OP-CERT')` +
`toContain('DAEMON-CERT')`, which both survive mutating the join separator to
`''` — with real PEM inputs that mutant fuses `-----END CERTIFICATE-----` onto
the next `-----BEGIN CERTIFICATE-----` and makes the bundle unparseable. It now
asserts the exact bundle text, which pins the separator and the order.
Verified: run-qwen-serve 275/275, channel-worker-supervisor 90/90, eslint and
prettier clean on the touched files. Typecheck error count is 139 both with and
without this change (worktree build skew against the main checkout's stale
`@qwen-code/*` dist; the same 139 appear on the unmodified branch).
Mutation-checked three ways, each reverting exactly one fix:
- dropping the bracket strip fails both new IPv6 tests
- `chainIsSelfAnchored` -> `isSelfSignedCert` fails the fullchain test
- `.join('\n')` -> `.join('')` fails the merged-bundle test
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli unit suite ran locally on Linux only.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli unit suite ran locally on Linux only。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Round 2 review findings on QwenLM#9392: 2 Critical, 5 Suggestion. R2-11 (Critical): the merge treated a merely *readable* operator NODE_EXTRA_CA_CERTS as trustworthy. Node's certificate loader is line-strict and all-or-nothing — a bundle built with `cat a.pem b.pem` where a.pem lacks a trailing newline fuses `-----END CERTIFICATE----------BEGIN CERTIFICATE-----` onto one line, and Node then discards the WHOLE bundle, daemon cert included. The existing fallback only fired on a read *failure*, so this shape sailed through the success path and left every worker trusting neither the operator CA nor the daemon cert while /health stayed green. The merge now extracts blocks with a line-strict PEM matcher and takes the existing warn-and-fall-back path when the operator file yields no loadable block or has a marker that produced none. `tls.createSecureContext({ ca })` does not throw on that shape, so it is not used as the validator. R2-12 (Critical): guard the 0o700 bundle-directory mode assertion on win32. `fs.mkdtempSync` ignores the mode there and libuv synthesises st_mode from file attributes (0o666 for a writable directory, structurally never 0o700), so the merge queue's test_windows job would go red on a test that passes on Linux/macOS. Same guard shape as observed-contact-store.test.ts. R2-13: write only certificate blocks into the bundle. A combined cert+key serving PEM passes boot validation, which parses the first block alone, so its private key was being copied into a tmpdir file NODE_EXTRA_CA_CERTS never reads — and that copy outlives a SIGKILLed daemon, whose `exit` cleanup cannot run. R2-4: revalidate the merged-bundle cache. It was keyed on paths alone, so an in-place operator CA rotation never reached respawned workers for the daemon's whole lifetime (before this PR a respawn read the operator's file live), and an external tmp cleaner aging out the bundle directory left every future respawn pointed at a dead path. Cache entries now carry each source's mtime/size and the bundle's existence is re-checked on hit. R2-3: harden the boot-time trust-gap check along the three corners the review demonstrated, per its stated minimum. Coverage is judged on the operator CA's *contents* rather than on the variable being set; every member of the anchor walk has its validity window checked (`x509.verify` is signature-only and never consults dates, so an expired root anchored "fine" while every handshake failed CERT_HAS_EXPIRED); and the leaf-anchor message no longer asserts a certain failure, since the check cannot see the workers' default trust store. `chainIsSelfAnchored` becomes `walkWorkerAnchorPath`, which returns the certificates the walk relied on so the date check can scope itself to them. R2-14: pin worker-side acceptance of `https://[::1]:4170`. The formatter emits it for a `::1` TLS bind and nothing else pinned the `'[::1]'` entry in LOOPBACK_BINDS, so dropping it as redundant kept every test green while regressing this PR's own failure mode on IPv6. R2-6: cover the boot-time warning wiring end to end. Only the pure function was tested, so deleting the loop, inverting its guard or feeding it unresolved values all shipped green. Two runQwenServe tests now boot a real TLS daemon on `::1` (a real SAN gap for a fixture cert that still pairs with its key) and on 127.0.0.1, asserting the gap text does and does not reach the daemon log. BEHAVIOUR FLIP — leaf-anchor gap suppression. A set-but-unhelpful NODE_EXTRA_CA_CERTS used to silence this warning outright. It no longer does: a typo'd, unrelated or unloadable path anchors exactly as little as no CA at all, and suppressing on the variable's mere presence silenced the diagnostic in the cases it was written for. The test that pinned the old behaviour is rewritten to assert the new contract rather than deleted, and three tests cover the paths it used to hide (anchoring CA, non-anchoring CA, unreadable path). BEHAVIOUR FLIP — a DER-encoded operator NODE_EXTRA_CA_CERTS is now refused with a warning instead of concatenated. Node's loader rejects it either way; the difference is that it no longer takes the daemon cert down with it. Verification: packages/cli — run-qwen-serve (283), channel-worker- supervisor (94), daemon-worker (85), process-env-guard (3), channel-worker-group — 507 tests pass. eslint and prettier clean. `tsc --noEmit -p packages/cli` reports 2 errors, both TS6305 against packages/core/dist; the same 2 appear on the stashed tree, so they are worktree build skew, not this change. Mutation-verified, 11 of 11 mutants killed: loose PEM regex, whole-file copy (key retained), no source-stamp revalidation, no bundle stat, `'[::1]'` dropped from LOOPBACK_BINDS, path-only gap suppression, chain-date check deleted, unsoftened wording, warn loop gutted, warn guard inverted, wrong daemonUrl fed to the check. R2-12 is a test-only platform guard with no production code to mutate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…TLS hop Clears the four findings still open on QwenLM#9392 from earlier rounds that round 2 did not re-report inline. R2-10: `chainIsSelfAnchored` modelled chain geometry only. OpenSSL also requires a certificate that SIGNS others to carry `basicConstraints CA:TRUE`, so a fullchain of leaf + self-signed CA:FALSE issuer was blessed as anchored while every worker handshake failed INVALID_PURPOSE — boot green, no warning, the exact silent outage this diagnostic exists to name. Measured on Node 22 with a real `tls.connect`: leaf + CA:FALSE self-signed issuer as the trust store → `INVALID_PURPOSE: unsuitable certificate purpose`. The constraint binds only PAST the leaf. The same probe shows a CA:FALSE self-signed cert in its OWN trust store is verified at depth 0 and handshakes fine (`authorized=true`) — which is what plain `openssl req -x509` produces — so requiring CA:TRUE there would cry wolf on the ordinary self-signed daemon cert. `walkWorkerAnchorPath` now rejects a non-CA terminator only when the walk took at least one step, and reports it separately so the gap text names INVALID_PURPOSE and the CA:FALSE remedy rather than UNABLE_TO_VERIFY_LEAF_SIGNATURE. R2-10's other shape — an expired self-signed root — is already covered by the chain-date check added in the previous commit. Two fixtures back this: a leaf signed by a self-signed CA:FALSE issuer, and a self-signed CA:FALSE leaf with loopback SANs. Both were minted with OpenSSL 3.0.13 and are the exact files the handshake probes above ran against. R2-7: no case drove the function to a two-gap outcome, so an inserted `return gaps` after the first push — or turning the SAN `if` into an `else if` — survived the whole suite. Under that mutant an operator fixes the trust anchor, restarts, and only then meets the SAN failure. Added a CA-issued cert dialled at a host its SANs miss, asserting both error names. R2-8: the documented mkcert flow produces a CA-issued leaf — precisely the shape the new boot warning flags — but the docs never connected channel workers to TLS (`grep -c NODE_EXTRA_CA_CERTS docs/users/qwen-serve.md` → 0). Added the HTTPS/TLS note: workers dial the daemon back over https, self-signed certs and self-carrying fullchains need nothing, the mkcert flow needs `NODE_EXTRA_CA_CERTS="$(mkcert -CAROOT)/rootCA.pem"` exported in the daemon's launch environment, and an operator-set value is merged with the daemon cert rather than replacing it. R2-9: documented the rotation asymmetry on the `tlsCaCertPath` option, per the finding's stated minimum. With no operator CA the worker gets the `--tls-cert` PATH and Node re-reads it at every respawn while the daemon still serves its boot-time bytes, so an in-place rotation makes respawned workers restart-loop; with an operator CA the merged bundle pins a snapshot instead. Either way the rotation needs a daemon restart, now said in both the JSDoc and the serve docs. Verification: packages/cli — 510 tests pass across run-qwen-serve (286), channel-worker-supervisor (94), daemon-worker (85), process-env-guard (3) and channel-worker-group. eslint clean; prettier clean including docs/users/qwen-serve.md. `tsc --noEmit -p packages/cli` reports the same 2 pre-existing TS6305 errors against packages/core/dist that the stashed tree reports — worktree build skew, not this change. Mutation-verified, 3 of 3 new mutants killed: CA check removed, CA check applied to the leaf as well, and the SAN gap suppressed once a trust-anchor gap exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QwenLM#9406 stacks on QwenLM#9392's first commit (8c6822c) and therefore carried a stale copy of the channel-worker TLS change. Round 1 reported four Criticals (R1-1..R1-4) and one Suggestion (R1-6) against that stale copy; all five are already fixed on QwenLM#9392's branch: - R1-1 predictable shared-tmpdir bundle path (CWE-377/CWE-59) -> the bundle now lives in a 0700 mkdtempSync directory. - R1-2 unresolved relative --tls-cert forwarded to workers spawned with cwd: opts.workspace -> path.resolve at boot. - R1-3 CA-issued leaf injected as the workers' trust anchor -> named at boot by describeWorkerTlsTrustGaps. - R1-4 no SAN check against the host workers dial -> certCoversHost. - R1-6 the channel-worker-group forwarding hop had no test -> covered by 'passes workerTlsCaCertPath through to every supervisor'. Merging rather than re-implementing keeps one source of truth for that work and avoids two divergent fixes for the same findings.
…gate Round 1 review findings on QwenLM#9406 that belong to this PR's own diff. The other four Criticals (R1-1..R1-4) and R1-6 were reported against the stale copy of QwenLM#9392's channel-worker TLS commit this branch stacks on; they are cleared by merging QwenLM#9392's reviewed head, not re-fixed here. R1-5 (Critical): `rejects a zenity without the executable bit on Linux` mocks `process.platform` to 'linux' but exercises the real host filesystem. Windows has no exec bit — libuv's `fs__access` ignores X_OK entirely, so `fs.accessSync` succeeds for any existing path and the probe returns true there, failing `.toBe(false)` deterministically. ci.yml's `test_windows` job is a merge-queue gate that collects the whole packages/cli suite, so this would block the PR in the queue. Guarded with `it.skipIf(process.platform === 'win32')`, the shape packages/core/src/utils/shellContextEnv.test.ts:157 already uses. The suite stays 21/21 on Linux, so the assertion still executes where the semantics it asserts exist. R1-7: nothing observed the `{onPick && (` guard in AddWorkspaceDialog. App.test.tsx mocks the dialog out entirely so it structurally cannot see the dialog DOM, and every `browseButton()` call site in AddWorkspaceDialog.test.tsx sits in a test that passes `onPick`. Turning that guard into an unconditional render therefore shipped green — and on a headless daemon host that puts back exactly the dead affordance this PR removes: a Browse… button whose handler returns immediately. Added two component tests: mounted without `onPick`, no Browse… button is in the document; mounted with it, the button is. Verification: packages/cli native-directory-picker 21 pass, packages/web-shell AddWorkspaceDialog 36 pass; eslint and prettier clean. Mutation-verified: `{onPick && (` → `{true && (` turns the new test red (1 failed / 35 passed). R1-5 has no production code to mutate; it is a platform guard on a test, verified by the suite still running green on Linux rather than being skipped there. Pre-existing and NOT from this change: `server.test.ts > accepts channel display text only from the workspace worker` fails in this worktree. It fails identically on c1161a7, this branch's head before the merge; this PR's diff does not touch `promptDisplayText` in server.ts; and the test text is identical on upstream/main. A clean upstream/main baseline could not be taken in this worktree — the suite refuses to start there for missing `packages/acp-bridge/dist/sessionMedia.js` build output. CI reports this PR's checks green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
…h sides Round 3 review: 2 Critical (R3-1, R3-2) and 6 Suggestions (R3-3..R3-8). R3-1 (Critical) — `extractCertificateBlocks` diverged from Node's NODE_EXTRA_CA_CERTS loader in both directions. Too lax: it validated block *shape* only, so a body of base64 characters that does not decode was merged ahead of the daemon cert, and Node then discarded the WHOLE bundle — workers lost trust in the operator CA *and* the daemon cert while /health stayed green. Each block is now parsed with `X509Certificate`, the loader's own parser. Too strict: a UTF-8 BOM, trailing whitespace after a marker line and leading whitespace on body lines were all rejected into the daemon-cert-only fallback with a warning that misdiagnosed the file; they are normalised away before matching. R3-2 (Critical) — the boot-time trust-gap diagnostic modelled the operator's CA with a looser parser than the spawn-time merge: a fused-marker bundle, or a DER file NODE_EXTRA_CA_CERTS never reads, was counted as an anchoring CA at boot while the merge discarded it and handed workers the daemon cert alone. The daemon log stayed clean and every worker handshake failed UNABLE_TO_VERIFY_LEAF_SIGNATURE — the exact silence this diagnostic exists to end. Both sides now share one extractor, moved to `pem-certificate-blocks.ts`, and an unloadable operator file is named in a gap instead of being trusted. R3-8 (behaviour flip) — `X509Certificate.ca` reads false both for an explicit `basicConstraints CA:FALSE` and for a v1/no-extension root, but OpenSSL accepts the second as an issuer. The INVALID_PURPOSE boot warning therefore fired on legacy anchors that work, telling operators to reissue a working CA. It now fires only when the certificate carries the extension and declares CA:FALSE. Measured on Node 22 / OpenSSL 3: a leaf anchored by a v1 root handshakes authorized=true, while the explicit CA:FALSE twin really does fail INVALID_PURPOSE. R3-5 — `warnWorkerCaMergeFallback` re-emitted on every spawn, so a crash-looping worker buried the log stream the operator reads to diagnose it. Deduped per path pair, keyed on the paths alone so flapping errno text cannot defeat it. R3-3 — the `tlsCaCertPath` comment claimed an operator CA pins a snapshot and makes in-place `--tls-cert` rotation invisible to workers. The code does the opposite: `resolveWorkerCaCertPath` stamps both sources, so rotation rebuilds the bundle from the new contents. Corrected to match the code and docs/users/qwen-serve.md:381. R3-6 — the probe is right that no test kills `mergedWorkerCaBundles.delete(cacheKey)`, but no test can: control always reaches the rebuild, which overwrites the key on success, and every hit re-stats the bundle and re-compares both stamps before returning it. The statement could not change an observable result, so it is removed rather than pinned by a test that would pass without it. The eviction *behaviour* stays covered by the rotation and tmp-cleaner tests. R3-4, R3-7 — new coverage: a CRLF operator bundle, a BOM operator bundle, a marker/body-whitespace bundle, an undecodable block, warn-once-per-pair, and three boot-log tests that drive the `process.env['NODE_EXTRA_CA_CERTS']` read and its try/catch end to end through `runQwenServe`. Every fix was mutation-verified: reverting each one turns exactly its own test(s) red (9 mutants, 9 kills). The loader claims above were measured against a real NODE_EXTRA_CA_CERTS handshake on Node 22.23, not inferred.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli suite ran locally on Linux only.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli suite ran locally on Linux only.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/server.test.ts:3924 — [review] Capabilities-envelope expectation is tautological with the server.ts wiring (unchanged code since the previous round)integration-tests/cli/qwen-serve-routes.test.ts:411 — [probe] Integration test sits outside every npm workspace — confirm the merge-queue job collects it (unchanged code since the previous round)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli suite ran locally on Linux only。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli suite ran locally on Linux only。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if ( | ||
| (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') || | ||
| !isLoopbackBind(parsed.hostname) | ||
| ) { | ||
| throw new Error(`${QWEN_DAEMON_URL_ENV} must use an http(s) loopback URL.`); | ||
| } |
There was a problem hiding this comment.
[Critical] R2-4: A daemon bound to a concrete non-loopback interface hands every channel worker a QWEN_DAEMON_URL this validator can never accept. formatChannelWorkerDaemonUrl rewrites only wildcard binds to 127.0.0.1; a concrete LAN bind emits https://192.168.1.100:<port>, which fails isLoopbackBind — so the documented LAN TLS + --channel deployment cannot start its workers, and the new boot trust-gap diagnostic stays silent because it models workers dialing exactly the host this validator forbids (its SAN test is titled "checks the host actually dialled, not a fixed loopback literal"). This diff rewrites this very condition to admit https: while the TLS/LAN/--channel surface it serves is new; no boot guard refuses --channel beyond loopback. — Failure scenario: qwen serve --hostname 192.168.1.100 --token T --tls-cert … --channel telegram (the mkcert LAN flow the new docs teach): boot passes every check this PR adds (token rule, TLS validation, silent trust-gap diagnostic), then every worker throws "must use an http(s) loopback URL" before dialing — the initial worker's failure exits the daemon (failBeforeReady → channel_worker_start_failed → process.exit(1), traced); dynamically added channels restart-loop.
Witness (probe against the real modules):
formatChannelWorkerDaemonUrl('192.168.1.100', 4170, true)
-> "https://192.168.1.100:4170"
runChannelDaemonWorker(daemonUrl=that) -> throws
"QWEN_DAEMON_URL must use an http(s) loopback URL."
describeWorkerTlsTrustGaps for the same bind (SAN IP:192.168.1.100,
root anchored) -> gaps [] (silent)
Suggested fix: workers are always local — rewrite concrete non-loopback binds to loopback in formatChannelWorkerDaemonUrl (dial 127.0.0.1/[::1], which the SAN gap check then validates), or refuse --channel at boot beyond loopback the way --local-control conflicts with a non-default --hostname, or name this gap in describeWorkerTlsTrustGaps.
中文说明
绑定到具体非回环网卡的 daemon 会交给每个 channel worker 一个该验证器永远无法接受的 QWEN_DAEMON_URL。formatChannelWorkerDaemonUrl 只把通配绑定改写为 127.0.0.1；具体 LAN 绑定会生成 https://192.168.1.100:<port>，无法通过 isLoopbackBind —— 于是文档化的 LAN TLS + --channel 部署无法启动 worker，而新增的启动信任缺口诊断保持静默，因为它建模的正是 worker 拨打这个被验证器禁止的主机（其 SAN 测试名为 "checks the host actually dialled, not a fixed loopback literal"）。本 diff 正是在 TLS/LAN/--channel 这套新能力落地的同时改写了这一条件以放行 https:；启动时也没有任何守卫拒绝回环之外的 --channel。—— 故障场景：qwen serve --hostname 192.168.1.100 --token T --tls-cert … --channel telegram（新文档教授的 mkcert LAN 流程）：启动通过本 PR 新增的所有检查（token 规则、TLS 校验、静默的信任缺口诊断），随后每个 worker 在拨号前抛出 "must use an http(s) loopback URL" —— 首个 worker 失败会导致 daemon 退出（failBeforeReady → channel_worker_start_failed → process.exit(1)，已全链路追踪）；动态添加的 channel 则反复重启。
建议修复：worker 始终在本地 —— 在 formatChannelWorkerDaemonUrl 中把具体非回环绑定改写为回环（拨打 127.0.0.1/[::1]，随后由 SAN 缺口检查校验），或在启动时像 --local-control 与非默认 --hostname 互斥那样拒绝回环之外的 --channel，或在 describeWorkerTlsTrustGaps 中指明该缺口。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| function sourceStamp(filePath: string): string { | ||
| const stat = fs.statSync(filePath); | ||
| return `${stat.mtimeMs}:${stat.size}`; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-11: The merged-bundle staleness detection — which the R2-4(a) comment exists for ("a path-only cache turns an in-place rotation into stale trust that lasts the daemon's whole lifetime") — keys only on mtimeMs:size, so a timestamp-preserving in-place rotation with an equal-size cert is never detected, and the rotated-away operator CA stays trusted by every respawned worker until the daemon restarts (the module-level map is the sole invalidation). The rotation test in this PR does not pin the edge: it rotates via fs.writeFileSync (mtime always advances) between two fixtures of different sizes. — Concrete cost: probe-verified against the real supervisor with a genuine timestamp-preserving, equal-size rotation (touch -r + cp -p, ns-exact): size unchanged, mtimeMs unchanged, content rotated on disk → respawn bundle path identical (cache hit), still containing the OLD operator CA; control with mtime advanced → bundle reflects current file. Trigger in practice: renewing a corporate-proxy CA with the same key (same-size DER is common) deployed with cp -p/rsync -a/Ansible — e.g. a compromise-response rotation leaving the compromised CA in every worker's trust store for the daemon's lifetime, diagnostics green.
Suggested fix: fold a cheap content fingerprint into the stamp — the files are small, so store sha256(contents) and re-hash the sources on a stat-matching cache hit (or hash on every spawn); alternatively accept the residual risk explicitly in the JSDoc instead of the current unqualified rotation claim.
中文说明
合并 bundle 的过期检测 —— 按 R2-4(a) 注释的说法正是为此而存在（"仅按路径缓存会让原地轮换变成持续整个 daemon 生命周期的陈旧信任"）—— 只以 mtimeMs:size 为键，因此保留时间戳的等大小原地轮换永远不会被检测到，被轮换掉的 operator CA 会一直留在每个 respawn worker 的信任库里，直到 daemon 重启（模块级 map 是唯一的失效机制）。本 PR 的轮换测试没有钉住这个边界：它用 fs.writeFileSync（mtime 必然前进）在两个大小不同的 fixture 之间轮换。—— 具体代价：已对真实 supervisor 用真正保留时间戳的等大小轮换做探针验证（touch -r + cp -p，纳秒级一致）：大小不变、mtimeMs 不变、磁盘内容已轮换 → respawn 的 bundle 路径不变（缓存命中），仍包含旧 operator CA；mtime 前进的对照组 → bundle 反映当前文件。实际触发：用同一把密钥续期 corporate-proxy CA（DER 大小常相同）并以 cp -p/rsync -a/Ansible 部署 —— 例如一次针对泄露的轮换反而让被泄露的 CA 在 daemon 整个生命周期内留在每个 worker 的信任库中，而所有诊断显示绿色。
建议修复：在 stamp 中加入廉价的内容指纹 —— 文件很小，可存储 sha256(contents)，在 stat 匹配的缓存命中时重新哈希源文件（或每次 spawn 都哈希）；或者在 JSDoc 中明确接受这一残余风险，而不是现在这种无条件的轮换声明。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| `UNABLE_TO_VERIFY_LEAF_SIGNATURE unless the issuing CA is already in ` + | ||
| `the workers' default trust store. Point NODE_EXTRA_CA_CERTS at the ` + | ||
| `issuing CA (for mkcert: "$(mkcert -CAROOT)/rootCA.pem") and restart.`, |
There was a problem hiding this comment.
[Suggestion] R2-12: An issuer that is basicConstraints CA:TRUE but whose keyUsage excludes keyCertSign is rejected by checkIssued before the terminator check, so the walk falls into this generic unanchored branch — asserting a certain UNABLE_TO_VERIFY_LEAF_SIGNATURE failure and advising to point NODE_EXTRA_CA_CERTS at the issuing CA, when the actual worker failure is OpenSSL error 32 (key usage does not include certificate signing), which no trust-anchor change can fix. The INVALID_PURPOSE branch (which exists precisely to distinguish purpose rejections) is structurally unreachable for this shape because checkIssued fails first. — Concrete cost: probe-verified on Node 22 — checkIssued(leaf, issuer w/o keyCertSign) false (control with keyCertSign true); the walk emits this UNABLE_TO_VERIFY_LEAF_SIGNATURE message; real tls.connect with the worker bundle fails "key usage does not include certificate signing"; openssl verify error 32 (control chain OK). An operator with this misissued chain gets a boot warning whose named error code never appears in the worker logs and whose suggested fix (re-add the same CA — already in the bundle) cannot change the outcome.
Suggested fix: distinguish "no issuer candidate at all" from "issuer candidates exist but checkIssued/verify rejected them"; name the latter as a purpose/extension problem with the rejected issuer's subject instead of asserting UNABLE_TO_VERIFY_LEAF_SIGNATURE; add a fixture with a CA:TRUE/keyCertSign-less issuer.
中文说明
一个 basicConstraints CA:TRUE 但 keyUsage 不含 keyCertSign 的签发者会在终结检查之前就被 checkIssued 拒绝，于是链遍历落入这个通用未锚定分支 —— 断言必然发生 UNABLE_TO_VERIFY_LEAF_SIGNATURE 失败并建议把 NODE_EXTRA_CA_CERTS 指向签发 CA，而 worker 实际的失败是 OpenSSL error 32（key usage does not include certificate signing），任何信任锚变更都无法修复。INVALID_PURPOSE 分支（正是为区分用途拒绝而存在）对这种形态结构上不可达，因为 checkIssued 先失败了。—— 具体代价：已在 Node 22 上探针验证 —— checkIssued(leaf, 无 keyCertSign 的签发者) 为 false（带 keyCertSign 的对照为 true）；链遍历输出这条 UNABLE_TO_VERIFY_LEAF_SIGNATURE 消息；用 worker bundle 做真实 tls.connect 失败于 "key usage does not include certificate signing"；openssl verify error 32（对照链 OK）。持有这种误签链的 operator 会看到一条启动警告，其给出的错误码在 worker 日志中永远不会出现，其建议的修复（重新添加同一个 CA —— 它已在 bundle 里）不可能改变结果。
建议修复：区分 "完全没有签发者候选" 与 "存在签发者候选但被 checkIssued/verify 拒绝"；对后者指明这是用途/扩展问题并给出被拒签发者的 subject，而不是断言 UNABLE_TO_VERIFY_LEAF_SIGNATURE；增加 CA:TRUE/无 keyCertSign 签发者的 fixture。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| return { anchored: false, path, nonCaTerminator: current }; | ||
| } | ||
| return { anchored: true, path }; |
There was a problem hiding this comment.
[Suggestion] R2-13: walkWorkerAnchorPath models only basicConstraints CA of OpenSSL's issuer-constraint checks; a pathlen:0 root anchoring a chain THROUGH an intermediate passes the walk silently — checkIssued true, verify true, root.ca true; Node's X509Certificate exposes no pathlen accessor — while every worker handshake fails PATH_LENGTH_EXCEEDED. — Concrete cost: probe-verified on Node 22 with the worker trust-store topology (fullchain leaf+intermediate served; bundle = leaf+intermediate+pathlen:0 root): real tls.connect → "path length constraint exceeded"; openssl verify → error 25 at 2 depth; PR walk verbatim → anchored: true, gaps: []; direct-issuance control under the same root agrees on both sides. Severity is Suggestion rather than Critical: the topology (pathlen:0 end-entity-only root + intermediate, on a daemon whose documented flows are mkcert and self-signed) is rare for this tool's deployment model, and the failure is not silent — worker handshake errors reach the daemon log via the supervisor's stderr forwarding, so an operator sees restart-looping with a nameable TLS error, just without the boot-time diagnosis. It is one member of an open class of OpenSSL-enforced constraints (nameConstraints, policyConstraints, EKU…) the static walk structurally cannot model — note that enforcing the R2-3 CA-flag fix still passes this chain.
Suggested fix: use the real verifier as the boot oracle — one loopback tls.connect against the worker trust-store composition surfaces the actual OpenSSL error and closes the whole constraint class; or parse pathlen from each issuer's basicConstraints and reject intermediates below a pathlen-limited issuer. Add a fixture for this shape.
中文说明
walkWorkerAnchorPath 只建模了 OpenSSL 签发者约束检查中的 basicConstraints CA；一个 pathlen:0 根隔着中间证书锚定链时会静默通过遍历 —— checkIssued true、verify true、root.ca true；Node 的 X509Certificate 不暴露 pathlen 访问器 —— 而每次 worker 握手都会以 PATH_LENGTH_EXCEEDED 失败。—— 具体代价：已在 Node 22 上按 worker 信任库拓扑探针验证（serving fullchain 叶子+中间；bundle = 叶子+中间+pathlen:0 根）：真实 tls.connect → "path length constraint exceeded"；openssl verify → error 25 at 2 depth；PR 遍历逻辑逐字执行 → anchored: true、gaps: []；同一根下直接签发的对照在两侧一致。严重度为 Suggestion 而非 Critical：该拓扑（pathlen:0 仅签终端实体的根 + 中间证书，出现在文档化流程为 mkcert 与自签名的 daemon 上）在本工具的部署模型中罕见，且失败并非静默 —— worker 握手错误会经 supervisor 的 stderr 转发进入 daemon 日志，operator 能看到带明确 TLS 错误的反复重启，只是缺少启动时诊断。它属于静态遍历结构上无法建模的、OpenSSL 强制执行的约束这一开放类别（nameConstraints、policyConstraints、EKU……）之一 —— 注意即使实施 R2-3 的 CA 标志修复，这条链仍会通过。
建议修复：用真实校验器作为启动预言机 —— 对 worker 信任库组成做一次回环 tls.connect，即可暴露真实的 OpenSSL 错误并关闭整个约束类别；或从每个签发者的 basicConstraints 解析 pathlen，拒绝位于 pathlen 受限签发者之下的中间证书。为此形态增加 fixture。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| return isIP(host) | ||
| ? Boolean(x509.checkIP(host)) | ||
| : Boolean(x509.checkHost(host)); |
There was a problem hiding this comment.
[Suggestion] R2-14: certCoversHost's x509.checkHost disagrees with tls.checkServerIdentity — the verification the workers' TLS client actually runs — on RFC 6125 partial-wildcard SANs: Node wires checkHost to OpenSSL with partial wildcards disabled, while checkServerIdentity accepts them. — Concrete cost: probe-verified on Node 22 — serving cert with SAN DNS:f*o.example.com, daemon bound to foo.example.com: real tls.connect → authorized: true, https.request → 200, but x509.checkHost('foo.example.com') → no match, and describeWorkerTlsTrustGaps warns "no subjectAltName covering "foo.example.com" … every worker handshake will fail ERR_TLS_CERT_ALTNAME_INVALID" for a configuration where every handshake succeeds. The operator reissues a perfectly good cert on a wrong diagnosis. Controls agree on both sides: full-label wildcards, CN-fallback on SAN-less certs, and the IP branch (checkIP matches checkServerIdentity for 127.0.0.1 / ::1 / ::ffff:127.0.0.1) — partial wildcards are the one divergence found.
Suggested fix: share the worker's predicate — use tls.checkServerIdentity(host, cert)'s result for both DNS and IP hosts, keeping diagnostic and client on one code path. Note: it cannot take the X509Certificate directly (on Node 22 the class lacks subjectaltname/object-form subject, which checkServerIdentity requires) — construct the legacy peer-cert shape or parse SANs equivalently.
中文说明
certCoversHost 的 x509.checkHost 与 worker TLS 客户端实际执行的校验 tls.checkServerIdentity 在 RFC 6125 部分通配 SAN 上不一致：Node 把 checkHost 接到 OpenSSL 时禁用了部分通配，而 checkServerIdentity 接受部分通配。—— 具体代价：已在 Node 22 上探针验证 —— serving 证书 SAN 为 DNS:f*o.example.com、daemon 绑定 foo.example.com：真实 tls.connect → authorized: true、https.request → 200，但 x509.checkHost('foo.example.com') → 不匹配，且 describeWorkerTlsTrustGaps 会对这个每次握手都成功的配置警告 "no subjectAltName covering "foo.example.com" … every worker handshake will fail ERR_TLS_CERT_ALTNAME_INVALID"。operator 会基于错误诊断重签一张完全正常的证书。对照组两侧一致：整段通配、无 SAN 证书的 CN 回退、以及 IP 分支（checkIP 与 checkServerIdentity 在 127.0.0.1 / ::1 / ::ffff:127.0.0.1 上一致）—— 部分通配是唯一被发现的偏差。
建议修复：共用 worker 的判定 —— 对 DNS 和 IP 主机都使用 tls.checkServerIdentity(host, cert) 的结果，让诊断与客户端走同一条代码路径。注意：它不能直接接受 X509Certificate（Node 22 上该类缺少 checkServerIdentity 需要的 subjectaltname/对象形式的 subject）—— 需构造旧式 peer-cert 形态或等价地解析 SAN。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| expect(gaps).toHaveLength(1); | ||
| expect(gaps[0]).toContain('CERT_HAS_EXPIRED'); | ||
| expect(gaps[0]).toContain('qwen fullchain test root CA'); |
There was a problem hiding this comment.
[Suggestion] R2-15: The anchor walk selects the FIRST certIssuedBy candidate in bundle order (chain.find(...)), while OpenSSL tries alternate issuer candidates — so a bundle holding an expired root alongside its renewed same-subject/same-key root yields a CERT_HAS_EXPIRED warning ("every worker handshake to the daemon will fail … Renew that chain member and restart") even though every real handshake succeeds. No fixture exercises multiple candidate issuers. — Concrete cost: probe-verified on Node 22 — bundle [leaf, expiredRoot, renewedRoot] → the CERT_HAS_EXPIRED gap; swapped order → []; oracle: real tls.connect with ca=[expired,valid] and ca=[valid,expired] → authorized: true in BOTH orders (identical public keys verified; expired-only control → CERT_HAS_EXPIRED). Trigger: standard CA renewal keeping the key (mkcert-style re-issue) during the overlap window. Compounding it: the diagnostic merges serving-then-operator ([...chain, ...operatorCerts]) while the supervisor's worker bundle is operator-then-serving — since the model is order-sensitive, whether the warning fires depends on which file happens to hold which root for semantically identical trust content.
Suggested fix: before pushing a date gap for a walked member, check whether another bundle cert with the same subject+key and a valid window also anchors, and stay silent when a valid alternate exists; add an expired+renewed same-key two-root fixture with a real tls.connect as the oracle.
中文说明
锚点遍历按 bundle 顺序选取第一个 certIssuedBy 候选（chain.find(...)），而 OpenSSL 会尝试备选签发者 —— 因此当 bundle 中同时存在已过期的根与同 subject/同密钥续期的根时，即使每次真实握手都成功，也会产生 CERT_HAS_EXPIRED 警告（"every worker handshake to the daemon will fail … Renew that chain member and restart"）。没有任何 fixture 覆盖多候选签发者。—— 具体代价：已在 Node 22 上探针验证 —— bundle [leaf, expiredRoot, renewedRoot] → CERT_HAS_EXPIRED 缺口；交换顺序 → []；预言机：真实 tls.connect 分别以 ca=[expired,valid] 与 ca=[valid,expired] 连接 → 两种顺序均 authorized: true（公钥一致性已验证；仅过期根的对照 → CERT_HAS_EXPIRED）。触发：保留密钥的标准 CA 续期（mkcert 式重签）的重叠窗口。更糟的是：诊断按 serving-then-operator 合并（[...chain, ...operatorCerts]），而 supervisor 的 worker bundle 是 operator-then-serving —— 由于模型对顺序敏感，对语义完全相同的信任内容，警告是否触发取决于哪个文件恰好持有哪个根。
建议修复：在为遍历成员推送日期缺口前，检查 bundle 中是否存在同 subject+密钥且有效期内的其他证书也能锚定，若存在有效备选则保持静默；增加过期+续期同密钥双根 fixture，并以真实 tls.connect 作为预言机。
— qwen3.8-max via Qwen Code /review (v0.21.13)
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.
8 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-11 merge gate shape-only validation poisons the worker CA bundle (Critical) — already reported (comment 3808628044)
- R2-1 misleading 'pins a snapshot' doc comment on tlsCaCertPath — already reported (comment 3808628066)
- R2-3 vacuous not.toContain('CN=localhost,') assertion — already reported (comment 3808628094)
- R2-2 per-rebuild exit listeners and stale bundle dirs linger until daemon exit — already reported (comment 3808628085)
- R2-5 CRLF normalization in extractCertificateBlocks untested — already reported (comment 3808628100)
- R2-6 boot NODE_EXTRA_CA_CERTS forward into the gap check unguarded — already reported (comment 3808628075)
- R2-13 sourceStamp mtime+size blind to preserved-mtime same-size rotation — already reported (comment 3808628110)
- R2-21 trust-gap diagnostic model diverges from Node/OpenSSL validation (Critical, class absorbing R2-7, R2-14, R2-18, R2-19, R2-20, R2-24, R2-27) — already reported (comment 3808628038; axes 3808628050, 3808628116, 3808628121, 3808628126, 3…
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 3-5 kept surfacing gaps in the TLS trust-model surface (folded into the R2-21 class finding).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli unit suite ran locally on Linux only.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI; integration-tests/cli/qwen-serve-routes.test.ts sits outside every workspace and was collected by no suite in this run.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/server.test.ts:3922 (+2 locations) — [probe] mirror-pattern envelope tests: on headless hosts a wiring-deletion mutant ships greenintegration-tests/cli/qwen-serve-routes.test.ts:43 — [test] file sits outside every workspace — the envelope assertions are collected by no suitepackages/cli/src/serve/channel-worker-group.ts:106 (+2 locations) — [test] type-only TLS option declaration hunks are gated only by tscpackages/cli/src/serve/native-directory-picker.test.ts:234 — [probe] multi-entry PATH scan exercised only with single-entry PATHspackages/web-shell/client/App.test.tsx:8797 — [probe] wrong-tag mutant in the App gate survives both App-level testspackages/cli/src/commands/channel/daemon-worker.test.ts:1338 — [probe] the scheme conjunct in validateDaemonWorkerUrl is pinned by nothing
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 8 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 3-5 kept surfacing gaps in the TLS trust-model surface (folded into the R2-21 class finding)。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli unit suite ran locally on Linux only。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI; integration-tests/cli/qwen-serve-routes.test.ts sits outside every workspace and was collected by no suite in this run。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Brings 40d5240 onto this branch so the round-2 TLS findings that QwenLM#9392 already fixed (R2-1 boot diagnostic vs. the supervisor's merge gate, R2-2 hand-written PEM acceptance vs. Node's loader, R2-3 CA:TRUE on non-terminator issuers, R2-5 tlsCaCertPath doc parenthetical, R2-6 boot NODE_EXTRA_CA_CERTS wiring test, R2-9 CRLF normalization) stop being re-derived here.
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.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- exit-listener accumulation in writeMergedWorkerCaBundle — already reported as R2-7 (comment 3808628085)
- greedy first-candidate issuer walk without backtracking — already reported as R2-15 (comment 3808628130)
- docs rotation bullet overstates worker-handshake failure with an operator CA — already reported as R2-10 (comment 3808628104)
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 4-5 kept surfacing Suggestion-level gaps in the TLS test surface (all verified — posted or deferred).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli unit suite ran locally on Linux only.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli unit suite ran locally on Linux only.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI; integration-tests/cli/qwen-serve-routes.test.ts sits outside every workspace and was collected by no suite in this run.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
integration-tests/cli/qwen-serve-routes.test.ts:411 — [test] file sits outside every npm workspace — the capability splice is collected by no suitepackages/cli/src/serve/channel-worker-group.ts:106 — [test] type-only TLS option declaration hunk is gated only by tscpackages/cli/src/serve/channel-worker-supervisor.ts:244 — [test] type-only tlsCaCertPath declaration hunk is gated only by tscpackages/cli/src/serve/run-qwen-serve.test.ts:1151 (+2 locations) — [probe] identical PEM block added twice under contradictory names (leaf vs operator-CA material)packages/cli/src/serve/native-directory-picker.test.ts:34 — [probe] module-scope mkdtemp fixture dirs are never removed (4 leaked per run)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 4-5 kept surfacing Suggestion-level gaps in the TLS test surface (all verified — posted or deferred)。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli unit suite ran locally on Linux only。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli unit suite ran locally on Linux only。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI; integration-tests/cli/qwen-serve-routes.test.ts sits outside every workspace and was collected by no suite in this run。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') || | ||
| !isLoopbackBind(parsed.hostname) |
There was a problem hiding this comment.
[Critical] R2-4: A daemon bound to a concrete non-loopback interface hands every channel worker a QWEN_DAEMON_URL this validator can never accept. formatChannelWorkerDaemonUrl rewrites only wildcard binds (''/0.0.0.0/::/[::]) to 127.0.0.1; a concrete LAN bind emits https://192.168.1.100:<port>, which fails isLoopbackBind — so the documented LAN TLS + --channel deployment cannot start its workers, and no boot guard refuses --channel beyond loopback. This diff rewrites this very condition to admit https: while the TLS/LAN/--channel surface it serves is new. Re-post of round 2 (comment 3808628059): the anchored code is unchanged since that comment and the mechanism was re-traced and re-probed at the reviewed commit. — Failure scenario: qwen serve --hostname 192.168.1.100 --token T --tls-cert … --channel telegram (the mkcert LAN flow the new docs teach): boot passes every check this PR adds (token rule, TLS validation, silent trust-gap diagnostic), then every worker throws "must use an http(s) loopback URL" before dialing — the initial worker's failure exits the daemon (failBeforeReady → channel_worker_start_failed → server closed, traced); dynamically added channels restart-loop.
Witness (probe against the real modules):
formatChannelWorkerDaemonUrl('192.168.1.100', 4170, true) -> "https://192.168.1.100:4170"
runChannelDaemonWorker(daemonUrl=that) -> throws "QWEN_DAEMON_URL must use an http(s) loopback URL."
10.0.0.5 -> rejected the same way
0.0.0.0 -> https://127.0.0.1:4170 -> passes validator
::1 -> https://[::1]:4170 -> passes validator
Suggested fix: workers are always local — rewrite concrete non-loopback binds to loopback in formatChannelWorkerDaemonUrl (dial 127.0.0.1/[::1], which the SAN gap check then validates), or refuse --channel at boot beyond loopback the way --local-control conflicts with a non-default --hostname.
中文说明
绑定到具体非回环网卡的 daemon 会交给每个 channel worker 一个该验证器永远无法接受的 QWEN_DAEMON_URL。formatChannelWorkerDaemonUrl 只把通配绑定改写为 127.0.0.1；具体 LAN 绑定会生成 https://192.168.1.100:<port>，无法通过 isLoopbackBind —— 于是文档化的 LAN TLS + --channel 部署无法启动 worker，且启动时没有任何守卫拒绝回环之外的 --channel。本 diff 正是在 TLS/LAN/--channel 这套新能力落地的同时改写了这一条件以放行 https:。此为第 2 轮发现的重发（评论 3808628059）：锚点代码自该评论以来未变，机制已在被审提交上重新追踪并重新探针验证。—— 故障场景：qwen serve --hostname 192.168.1.100 --token T --tls-cert … --channel telegram（新文档教授的 mkcert LAN 流程）：启动通过本 PR 新增的所有检查，随后每个 worker 在拨号前抛出 "must use an http(s) loopback URL" —— 首个 worker 失败会导致 daemon 退出（failBeforeReady → channel_worker_start_failed → 服务器关闭，已全链路追踪）；动态添加的 channel 则反复重启。
证据（对真实模块的探针）：formatChannelWorkerDaemonUrl('192.168.1.100', 4170, true) 生成 https://192.168.1.100:4170，runChannelDaemonWorker 抛出 "QWEN_DAEMON_URL must use an http(s) loopback URL."；10.0.0.5 同样被拒；0.0.0.0 与 ::1 分别改写为 https://127.0.0.1:4170 / https://[::1]:4170 并通过验证。
建议修复：worker 始终在本地 —— 在 formatChannelWorkerDaemonUrl 中把具体非回环绑定改写为回环（拨打 127.0.0.1/[::1]，随后由 SAN 缺口检查校验），或在启动时像 --local-control 与非默认 --hostname 互斥那样拒绝回环之外的 --channel。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Round 4 review of QwenLM#9392: four Critical findings, three of them rooted in the same place — this code re-implemented Node's `NODE_EXTRA_CA_CERTS` loader instead of following it. R4-2 (Critical): `extractCertificateBlocks` pattern-matched what a well-formed PEM file looks like, and a new divergent shape surfaced in each of the last three rounds. Replaced with a line scanner that walks the file the way OpenSSL's `PEM_read_bio_X509` loop does. Three shapes Node loads and this rejected now extract: a `-----BEGIN CERTIFICATE-----` substring embedded in a line of prose (markers are matched at line start, not as unanchored substrings), whitespace inside a base64 body line, and a UTF-8 BOM in front of a block that is not the first in the file (what concatenating operator files produces). Every one of them silently fell back to daemon-cert-only while telling the operator the file "holds no PEM certificate block Node can load". BEHAVIOUR FLIP — the loader is prefix-loading, not all-or-nothing. The doc comment this module carried claimed a malformed block discards the whole bundle. Measured on Node 22 / OpenSSL 3 through real `NODE_EXTRA_CA_CERTS` handshakes: a good root followed by a fused block still handshakes `authorized=true` while Node prints `Ignoring extra certs … bad end line`. The loader keeps every certificate up to the first malformed block and loses that block and everything after it. So does this now; returning `undefined` for the whole file threw away anchors the workers do in fact receive. The fused-file and bad-decode cases still return `undefined`, because there the bad block IS the first one. Both behaviours were taken from the loader, not inferred: 15 shapes were written to disk, pointed at through `NODE_EXTRA_CA_CERTS` in a child process, and checked against a real `tls.connect` to a server holding the leaf they anchor. The parser agrees with the oracle on all 15, and `pem-certificate-blocks.test.ts` (new — this module had no direct coverage, which is how three rounds of shapes got through) pins each one with the measured verdict in the comment. R4-4 (Critical): `walkWorkerAnchorPath` applied the CA-suitability check only to the self-signed terminator, so a chain passing THROUGH an incapable issuer was reported anchored while every worker handshake failed. Issuer capability is now required of every non-self-signed chain member the walk leans on. Measured with real handshakes: a CA:FALSE intermediate and a v3 intermediate with no basicConstraints both fail INVALID_PURPOSE, and a keyCertSign-only intermediate fails INVALID_CA — all three reported gaps=NONE before. The self-signed terminator keeps its existing, looser rule, so the v1 root and CA:FALSE self-signed leaf cases stay unflagged as measured in earlier rounds. R4-3 (Critical): the boot diagnostic modelled a merged serving+operator trust store that the workers never receive when the serving file fails extraction — `resolveWorkerCaCertPath` finds `daemonBlocks === undefined`, discards the operator CA and hands them the serving file alone. Boot reported no gap while every worker handshake failed. The model now mirrors the fallback and names the discarded operator CA. The comment's premise (that such a file "cannot serve at all") was false and is gone. R4-1 (Critical): every `writeMergedWorkerCaBundle` call registered its own `process.once('exit')` listener. The merge cache is invalidated on purpose by in-place operator CA rotation and by tmp-cleaner aging, so a long-lived daemon accumulated a listener, a closure and an orphaned bundle directory per rebuild, and past the tenth printed `MaxListenersExceededWarning` into the log stream the fallback dedup exists to keep readable. One module-level hook now cleans up every minted directory, and a rebuild removes the directory it supersedes. R4-5 (Suggestion): the fallback-warning dedup was keyed on the path pair and add-only, so the first failure silenced every later one. Keyed on a coarse failure family now, and the keys are lifted when the pair merges successfully — a changed failure mode and a relapse after a fix are both new information. R4-6 (Suggestion): the fallback message blamed markers alone, but this PR's own X509 decode gate added a third rejection cause. Aligned with the boot-side wording, which already enumerates all three. R4-7 (Suggestion): the DER and fused operator-CA tests asserted gap presence via `.some()` without pinning the count, and never asserted the DER-specific text. Both now pin `toHaveLength(2)`, and the DER test asserts its own message. Every fix is mutation-verified: reverting it turns at least one test red (9 mutants run, 9 killed). Verification: `npx vitest run src/serve/pem-certificate-blocks.test.ts src/serve/channel-worker-supervisor.test.ts src/serve/run-qwen-serve.test.ts` — 411 passed; channel-worker-group / -manager / -diagnostics — 84 passed; eslint and prettier clean on the six touched files. `npm run build` and `npm run typecheck` do not complete in this worktree for reasons that predate this change and reproduce with it stashed (a `sharp` typing skew in packages/core and `@qwen-code/*` resolving to the sibling checkout's dist): 105 typecheck errors with and without the change, none in the touched files.
…e boot-gap test's IPv6 dependency Round 3 review of QwenLM#9406: four Critical and six Suggestion findings, plus three round-2 Suggestions re-confirmed. Six of them are the same defects QwenLM#9392 fixed and measured this round, so this merges `fb9239e114` rather than re-deriving them; the rest are this PR's own test surface. Taken from QwenLM#9392 (`fb9239e114`, mutation-verified there): - R2-2 / R2-3 (Critical): `extractCertificateBlocks` pattern-matched what a well-formed PEM file looks like instead of walking it the way OpenSSL's `PEM_read_bio_X509` loop does, and `walkWorkerAnchorPath` applied its CA-suitability check only to the self-signed terminator. Both are now judged by the loader's own rules, measured against real `NODE_EXTRA_CA_CERTS` handshakes (15 shapes, parser and oracle agree on all 15). Note the behaviour flip that came with it: the loader is prefix-loading, not all-or-nothing. - R2-21 (Critical): the boot diagnostic modelled a merged serving+operator trust store the workers never receive when serving-file extraction fails. - R2-7 (Suggestion): every `writeMergedWorkerCaBundle` call registered its own `process.once('exit')` listener. - R3-2 / R3-4 (Suggestion): `warnedWorkerCaMergeFallbacks` was add-only, so a pair that merged again and failed later stayed silent forever; and the fallback warning hardcoded a marker-shape hint that is false for the base64-decode rejection class. Fixed here: - R3-1 (Critical): the boot-gap test bound `::1` unconditionally, so on the IPv6-less CI containers `server.test.ts`'s sibling guard already names it fails `EADDRNOTAVAIL` rather than skipping. The gap needs no IPv6 at all — a serving cert whose only SAN is `example.invalid` is uncovered by the `127.0.0.1` the workers dial — so the wiring test now binds IPv4 and runs everywhere, and `TEST_TLS_CERT_NO_LOOPBACK_SAN` gained the key it pairs with. Measured: a client dialling `127.0.0.1` with that cert as its CA fails `ERR_TLS_CERT_ALTNAME_INVALID`. A second test keeps the `::1` coverage that pins bracket stripping, behind the same `hasIpv6Loopback` guard the sibling uses. - R2-22 (Suggestion): no test varied the operator CA against a fixed daemon cert, so a mutant keying `mergedWorkerCaBundles` on `daemonCertPath` alone survived the suite. Added the A/B/A test the review asked for; the mutant now fails it. - R3-5 (Suggestion): the tmp-cleaner test could not tell "rebuilt the merged bundle" from "fell back to the daemon cert alone" — both satisfy its two assertions. It now pins the merged content the way the rotation test does; the inner-catch degradation mutant the review demonstrated now fails it. - R3-3 (Suggestion): the new CA-issued fixtures were inserted between `TEST_TLS_CERT_EXPIRED`'s comment and the const it describes, orphaning it ~90 lines above its subject. Comment moved back onto its const. - R3-6 (Suggestion): the `TEST_TLS_CERT_NO_LOOPBACK_SAN` comment quoted `openssl req -x509 -subj "/CN=localhost"` as the recipe, but that emits no SAN at all and Node then authorizes via CN fallback — regenerating from it would silently drop the gap the tests pin. The fixture was regenerated from the accurate recipe, which the comment now records. - R2-10 (Suggestion): the in-place rotation bullet predicted an unconditional worker-handshake failure. That holds only when the daemon cert is its own trust anchor; under the CA-anchored mkcert flow the preceding bullet teaches, the unchanged root anchors both generations. Bullet qualified. Not addressed: R2-4 (Critical) needs a product decision on what a daemon bound to a concrete non-loopback interface should hand its workers, and R2-15 (Suggestion) needs alternate-issuer backtracking plus an expired+renewed same-key fixture with a real handshake oracle. Verification: `packages/cli` vitest over run-qwen-serve, channel-worker-supervisor, pem-certificate-blocks, native-directory-picker and channel-worker-group — 476 passed. Three mutants written and all three killed: the boot trust-gap loop disabled (killed by the new IPv4 test, so it is killed on IPv6-less hosts too), the bundle cache keyed on the daemon cert alone, and the cached branch's inner catch degraded to `return daemonCertPath`.
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- sourceStamp mtime+size staleness-detection blindness (channel-worker-supervisor.ts:411) — already reported (comment 3808628110, R2-11)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI; integration-tests/cli/qwen-serve-routes.test.ts sits outside every workspace and was collected by no suite in this run.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 3-5 kept surfacing gaps in the TLS trust-model surface (all verified — posted, folded into the R2-21 class, deferred, or rejected).
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/run-qwen-serve.test.ts:1372 — [probe] TEST_TLS_CERT_SELF_SIGNED_NON_CA recipe comment wrong (openssl req -x509 emits CA:TRUE); nothing asserts .ca === false (unchanged code since the previous round)packages/cli/src/serve/run-qwen-serve.test.ts:1607 — [review] 'the fixture root outlives its leaf' comment false — both members carry identical validity windows (unchanged code since the previous round)packages/cli/src/serve/run-qwen-serve.test.ts:9779 — [probe] relative-path absoluteness test cannot distinguish cwd-based from workspace-based resolution (unchanged code since the previous round)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (runs only in the merge queue) and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI; integration-tests/cli/qwen-serve-routes.test.ts sits outside every workspace and was collected by no suite in this run。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds; rounds 3-5 kept surfacing gaps in the TLS trust-model surface (all verified — posted, folded into the R2-21 class, deferred, or rejected)。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…trust diagnostic off measured OpenSSL behaviour (R2-4, R2-21) R2-4 — a daemon bound to a concrete interface handed every worker a `QWEN_DAEMON_URL` the worker's own validator rejected, so the documented LAN flow passed every boot check and then failed in each worker. The review's first option (rewrite the URL to loopback) does not hold: `server.listen(port, hostname)` binds that interface ONLY, and dialling 127.0.0.1 against such a bind is ECONNREFUSED (measured on this host). The worker therefore keeps dialling the bound address, and the validator widens from "loopback" to "loopback or an address on this host" — which keeps the daemon token off the wire exactly as the loopback rule did, since traffic to an own interface address never leaves the machine. A bind we cannot certify as local (a DNS name, a literal on no interface) is now refused at boot by `assertChannelWorkerDaemonUrlIsLocal` instead of restart-looping every worker with /health green. R2-21 — the five measured entrances, each closed locally (the structural rewrite is a separate change): 1. The SAN check and leaf identity now read the loader-framed `servingChain[0]`, not the loose parser's `chain[0]`; the two disagree when a predecessor's BEGIN marker is absorbed into a comment, and the diagnostic judged a certificate never served. 2. A depth-0 server-purpose check on the serving leaf: keyUsage without digitalSignature/keyEncipherment/keyAgreement, or an EKU without serverAuth, both fail INVALID_PURPOSE (both measured). 3. The issuer-capability check reads extendedKeyUsage too — an EKU on a chain member constrains everything below it (measured with a control arm: the EKU-free twin authorizes). 4. `pathLenConstraint` is modelled: a pathlen:0 root over an intermediate fails PATH_LENGTH_EXCEEDED (measured). 5. The issuer walk prefers a date-valid candidate over an expired same-subject twin, killing a false CERT_HAS_EXPIRED alarm against a bundle that authorizes (measured). Suggestions in the same pass: name the issuer's missing keyCertSign instead of the generic unanchored advice that sends operators to re-point a CA already provided (measured: checkIssued false, verify true, handshake "key usage does not include certificate signing"); pin the PEM scan continuing past a non-certificate block, the operator- supplied expired chain member, and the v3-no-basicConstraints intermediate; pin the minted-bundle set's lifetime through a named `cleanupMintedWorkerCaBundleDirs`; and correct the rotation overclaim in docs and the `tlsCaCertPath` JSDoc — a fullchain renewed under the same carried root still verifies (measured), only an anchor that rotates out breaks the workers. Every fix is mutation-verified: reverting each one turns its own test red. Affected suites 516/516 green; the single `tsc -p packages/cli` error is pre-existing (workspace-service/index.ts, identical before and after). Co-Authored-By: Claude Opus 5 (1M context) <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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- zone-carrying IPv6 --hostname throws raw ERR_INVALID_URL out of the boot guard (unreachable zone normalisation) — already reported (comment 3815211076)
Not reviewed: test-efficacy — harness unvalidated (the positive control never ran); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) is collected by no workspace suite, so the new capability splice was exercised by no deterministic test in this run.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) FAILED in CI at the reviewed commit; local build/test verification ran Linux only, so the Windows failure is unattributed.
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/local-bind-addresses.ts:44 — [probe] IPv4-mapped concrete/loopback literals escape the normalisations (siblings of the fixed R14-1 wildcard carve-out)packages/cli/src/serve/native-directory-picker.ts:36 — [probe] three surviving mutants in the picker availability probe (root-console, SSH_TTY-only, blank SESSIONNAME)
Convergence: round 15 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (1 new). Findings keep coming back to the same files: packages/cli/src/serve/run-qwen-serve.ts (findings in rounds 2, 14; 1 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 keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit: the round-15 entrance fixes from f61db34 (BOM-at-scan-start, RFC1421 blank-separator enforcement, nameConstraints model) were discarded by merge 337443c — nameConstraints is referenced nowhere at HEAD nor on main, and pem-certificate-blocks.ts is byte-identical to main. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness: git grep -i nameConstraints at HEAD and merge base → zero matches; git diff a6d30eb..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts → empty; round-14 probe had demonstrated chains the anchor walk reported anchored with zero gaps while openssl verify / Node handshakes failed error 47/48 (nameConstraints entrance).
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：test-efficacy — harness unvalidated (the positive control never ran); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) is collected by no workspace suite, so the new capability splice was exercised by no deterministic test in this run。
未审查：build-and-test — Test (windows-latest, Node 22.x) FAILED in CI at the reviewed commit; local build/test verification ran Linux only, so the Windows failure is unattributed。
收敛姿态下延后（第 15 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 15 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 2 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/cli/src/serve/run-qwen-serve.ts（第 2、14 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit: the round-15 entrance fixes from f61db34 (BOM-at-scan-start, RFC1421 blank-separator enforcement, nameConstraints model) were discarded by merge 337443c — nameConstraints is referenced nowhere at HEAD nor on main, and pem-certificate-blocks.ts is byte-identical to main. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness: git grep -i nameConstraints at HEAD and merge base → zero matches; git diff a6d30eb..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts → empty; round-14 probe had demonstrated chains the anchor walk reported anchored with zero gaps while openssl verify / Node handshakes failed error 47/48 (nameConstraints entrance).
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const host = new URL(workerDaemonUrl).hostname; | ||
| if (isLoopbackBind(host) || isOwnInterfaceAddress(host)) return; |
There was a problem hiding this comment.
[Critical] R15-1: This new boot guard certifies concrete own-interface binds via isOwnInterfaceAddress, but the worker-side validateDaemonWorkerUrl (packages/cli/src/commands/channel/daemon-worker.ts:320-333, byte-identical to main in this diff) still accepts only isLoopbackBind URLs — so every channel worker spawned against a bind this guard just certified throws QWEN_DAEMON_URL must use an http(s) loopback URL. before dialing. This is the round-2 blocker R2-4 mechanism: it was fixed on this branch in 22ca724ee3 (widening the validator plus ~51 lines of worker-side tests) and silently dropped by the merge 337443c889 — git diff main..HEAD is empty for daemon-worker.ts and daemon-worker.test.ts, and the new local-bind-addresses.test.ts header still asserts the lost call edge (isOwnInterfaceAddress "reached only through assertChannelWorkerDaemonUrlIsLocal and validateDaemonWorkerUrl").
Failure scenario: qwen serve --hostname 192.168.1.100 --token T --channel telegram (that literal being one of the host's own interfaces — the bind class the new test "accepts a concrete bind on one of this host's own interfaces" blesses, and the bind this guard's error message prescribes: "Bind to … a literal address of one of this machine's interfaces"). Boot passes via isOwnInterfaceAddress, the supervisor hands each worker QWEN_DAEMON_URL=http(s)://192.168.1.100:<port> (channel-worker-supervisor.ts:663), and every worker's first act rejects it — the first worker's failure exits the daemon, dynamically added channels restart-loop while /health stays green: exactly the failure mode this function's docstring says it exists to turn into a loud boot error.
Witness (probe through the real modules at HEAD; runner's own interface 172.22.139.168):
BEFORE (unmodified PR):
bootGuard: "ACCEPTED"
workerStartup: "REJECTED: QWEN_DAEMON_URL must use an http(s) loopback URL."
AFTER (validator widened per the fix):
bootGuard: "ACCEPTED"
workerStartup: passed validation (reached the probe sentinel)
Fix: restore the merge-dropped widening in validateDaemonWorkerUrl — accept isLoopbackBind(parsed.hostname) || isOwnInterfaceAddress(parsed.hostname) (importing from ../../serve/local-bind-addresses.js), widen the rejection message, and restore the two worker-side tests from 22ca724ee3 so the boot-side and worker-side validators cannot diverge again. Until the worker side is aligned, this guard must refuse concrete binds too instead of certifying them.
中文说明
[Critical] R15-1：新增的启动守卫通过 isOwnInterfaceAddress 认证"绑定到本机网卡的具体地址"，但 worker 侧的 validateDaemonWorkerUrl（packages/cli/src/commands/channel/daemon-worker.ts:320-333，在本 diff 中与 main 逐字节相同）仍然只接受 isLoopbackBind 回环 URL —— 因此该守卫认证的每一个本机网卡绑定，在派生 channel worker 后都会在拨号前抛出 QWEN_DAEMON_URL must use an http(s) loopback URL.。这正是第 2 轮的阻断项 R2-4 的机制：本分支曾在 22ca724ee3 修复（放宽 worker 校验器并新增约 51 行 worker 侧测试），但被合并提交 337443c889 静默丢弃 —— git diff main..HEAD 对 daemon-worker.ts 与 daemon-worker.test.ts 为空，而新增的 local-bind-addresses.test.ts 文件头注释仍在断言这条已不存在的调用边（"isOwnInterfaceAddress 仅经由 assertChannelWorkerDaemonUrlIsLocal 和 validateDaemonWorkerUrl 调用"）。
故障场景：qwen serve --hostname 192.168.1.100 --token T --channel telegram（该字面量是本机某张网卡的地址 —— 新增测试 "accepts a concrete bind on one of this host's own interfaces" 所确认的绑定形态，也是本守卫错误信息第三个补救选项所指示的绑定方式）。启动经由 isOwnInterfaceAddress 通过守卫，supervisor 把 QWEN_DAEMON_URL=http(s)://192.168.1.100:<port> 交给每个 worker（channel-worker-supervisor.ts:663），每个 worker 的第一步就拒绝它 —— 首个 worker 失败会导致 daemon 退出，动态添加的 channel 反复重启而 /health 保持绿色：恰恰是本函数 docstring 声称要在启动时大声报错的那种失败形态。
证据（在 HEAD 上对真实模块的探针；本机网卡 172.22.139.168）：修复前 bootGuard 为 ACCEPTED 而 workerStartup 抛出 "must use an http(s) loopback URL"；按建议放宽校验器后，workerStartup 通过校验（到达探针哨兵）。
修复：在 validateDaemonWorkerUrl 中恢复被合并丢弃的放宽 —— 接受 isLoopbackBind(parsed.hostname) || isOwnInterfaceAddress(parsed.hostname)（从 ../../serve/local-bind-addresses.js 导入），同步放宽拒绝信息，并恢复 22ca724ee3 中的两个 worker 侧测试，使启动侧与 worker 侧校验器不再背离。在 worker 侧对齐之前，本守卫也应拒绝具体网卡绑定而不是认证它们。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…Ls (QwenLM#9406) The boot guard (`assertChannelWorkerDaemonUrlIsLocal`) certifies concrete own-interface binds via `isOwnInterfaceAddress`, but the worker-side `validateDaemonWorkerUrl` still accepted loopback-only URLs — the widening from 22ca724 was silently dropped by merge 337443c. Every channel worker spawned against a certified concrete bind threw "QWEN_DAEMON_URL must use an http(s) loopback URL." before dialing: the first worker's failure exited the daemon while /health stayed green. Restore the widening: accept loopback or an address of one of this host's own interfaces (traffic to an own-interface address never leaves the machine, so the daemon token stays on-box exactly as the loopback rule guaranteed), widen the rejection message, and restore the two worker-side witness tests so the boot-side and worker-side validators cannot diverge again.
…stic (QwenLM#9406) `qwen serve --hostname 'fe80::…%eth0'` bound fine, then `assertChannelWorkerDaemonUrlIsLocal` threw a raw `ERR_INVALID_URL`: `formatHostForUrl` percent-encodes the zone into the worker URL, and WHATWG URL rejects zone IDs outright — so the failure was visible but unactionable, instead of the deliberate "Channels cannot start" boot diagnostic this guard exists to provide. Catch the parse failure and refuse with the named message: the worker URL pipeline cannot carry a zone-scoped address even though this host answers on it. Re-document the `%25` zone decode in `bareAddress` as defensive-only — production callers feed it from `new URL(...).hostname`, which never lets a zone survive the parse layer.
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9406Commits this round (additive, on Feedback triage[rc:3856927274] R15-1 [Critical] — FIXEDClaim: the boot guard ( Reproduced before any code changed. Restored the two worker-side tests from Fix (commit Mutation probes: (1) removing the [rv:5023607365] Review body[Critical] R2-21 (ledger re-post) — DEFERRED to the acknowledged follow-up PR. The class finding (the boot trust-gap diagnostic and its shared PEM model re-implement Node's Zone-carrying IPv6 Suggestion (comment 3815211076, confirmed by this review, not re-posted) — FIXED (commit Convergence-posture deferrals recorded by the review (IPv4-mapped concrete/loopback literals escaping normalisation at "Not reviewed" disclosures (test-efficacy harness unvalidated; integration test not collected by a workspace suite; Windows failure unattributed) — noted; these are review-scope disclosures, not findings. Failed check: Test (windows-latest Node 22.x) — attributed, not code-actionable from this runnerNo CI log contents are available on this runner and no credentials exist to fetch them (
Conclusion: the leading evidence-backed hypothesis is that the failure lives in the #9392-derived test surface that now sits on VerificationCommands actually run this round (final tree unless noted):
Not run: the exact 中文说明轮次总结 — PR #9406本轮提交（增量式，位于 反馈分类[rc:3856927274] R15-1 [Critical] — 已修复主张：启动守卫（ 改代码之前先复现。 恢复 修复（提交 变异探针：（1）移除 [rv:5023607365] 评审正文[Critical] R2-21（台账重发）— 延后至已确认的后续 PR。 该类别发现（启动信任缺口诊断及其共享 PEM 模型重新实现了 Node 的 携带 zone 的 IPv6 建议（评论 3815211076，本轮评审确认已报告、不再重复发布）— 已修复（提交 评审记录的收敛姿态延后项（ "未审查"披露（test-efficacy harness 未验证；集成测试未被任何 workspace 套件收集；Windows 失败未归因）—— 已知悉；这些是评审范围披露，不是发现。 失败检查：Test (windows-latest Node 22.x) — 已归因分析，但本 runner 上无可落码的证据本 runner 上没有 CI 日志内容，也没有凭证可获取（
结论：证据支持的主要假设是失败位于 #9392 引入、现已在 验证本轮实际执行的命令（除特别说明外均针对最终树）：
未运行：精确的 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only and dormant) and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run.
Not reviewed: test-efficacy — harness unvalidated (the positive control never ran: the probe runner hit the repo's vitest globalSetup build guard); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean.
Deferred under the convergence posture (round 16, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/run-qwen-serve.ts:781 — [review] accept rule (loopback OR own-interface) implemented twice — boot assert and validateDaemonWorkerUrl; no shared predicatedocs/users/qwen-serve.md:400 — [review] IPv6 bullet contradicted for the v4-mapped wildcard spelling (::ffff:0.0.0.0 dials 127.0.0.1, needs 127.0.0.1 in SANs, not ::1)packages/cli/src/serve/server.test.ts:4061 — [probe] envelope expectation mirrors the host probe — wiring-deletion mutant ships green on headless CI; headline feature silently never activates on GUI hostspackages/cli/src/serve/native-directory-picker.ts:41 — [probe] darwin SSH_TTY conjunct pinned by no test — deletion mutant ships greenpackages/cli/src/serve/local-bind-addresses.ts:48 — [probe] IPv4-mapped literals of own/loopback addresses refused with a factually wrong diagnostic (sibling of open comment 3815211054)integration-tests/cli/qwen-serve-routes.test.ts:412 — [test] capability splice collected by no workspace suite; the no-AK integration gate skipped at this head and the merge-queue collector is dormantpackages/cli/src/serve/run-qwen-serve.ts:777 — [probe] boot guard certifies 127/8 binds (127.0.0.2) that the Host-header gate then 403s on every request
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit: the round-15 entrance fixes from f61db34 (BOM-at-scan-start, RFC1421 blank-separator enforcement, nameConstraints model) remain discarded by merge 337443c — nameConstraints is referenced nowhere at HEAD, and pem-certificate-blocks.ts is byte-identical to main. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD c01c800 this round): git grep -ci nameConstraints -- packages/cli/src/serve/ -> zero matches; git diff a6d30eb..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts -> empty; round-14 probe had demonstrated chains the anchor walk reported anchored with zero gaps while openssl verify / Node handshakes failed error 47/48 (nameConstraints entrance).
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only and dormant) and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run。
未审查：test-efficacy — harness unvalidated (the positive control never ran: the probe runner hit the repo's vitest globalSetup build guard); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean。
收敛姿态下延后（第 16 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit: the round-15 entrance fixes from f61db34 (BOM-at-scan-start, RFC1421 blank-separator enforcement, nameConstraints model) remain discarded by merge 337443c — nameConstraints is referenced nowhere at HEAD, and pem-certificate-blocks.ts is byte-identical to main. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD c01c800 this round): git grep -ci nameConstraints -- packages/cli/src/serve/ -> zero matches; git diff a6d30eb..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts -> empty; round-14 probe had demonstrated chains the anchor walk reported anchored with zero gaps while openssl verify / Node handshakes failed error 47/48 (nameConstraints entrance).
— qwen3.8-max via Qwen Code /review (v0.22.0)
QwenLM#9406) IPv6-less runners bind `::` yet carry no `::1`, so the certified loopback dial fails EADDRNOTAVAIL — the arm is unmeasurable there, not wrong; skip it like a failed bind instead of reddening the suite.
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9406 (head
|
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.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- duplicated accept predicate across the two new guards (run-qwen-serve.ts:777, daemon-worker.ts:336) — still stands; already recorded in the round-16 deferral list (review 5025220188)
- macOS probe SSH_TTY conjunct pinned by no test (native-directory-picker.ts:42) — still stands, probe-flipped again this round; already recorded in the round-16 deferral list (review 5025220188)
- capabilities-envelope expectation mirrors the host probe (server.test.ts:4061, integration-tests/cli/qwen-serve-routes.test.ts:412) — still stands, wiring-deletion mutant passed 1079/1079 again this round; already recorded in the round-16 d…
- IPv4-mapped spellings of own/loopback addresses refused with a factually wrong diagnostic (local-bind-addresses.ts:44) — still stands, re-measured this round; already recorded in the round-16 deferral list (review 5025220188) and as…
- integration-tests/cli/qwen-serve-routes.test.ts collected by no workspace suite — still stands; already reported in rounds 2-16
Unresolved, please confirm:
- [Critical] R13-1 (comment 3841754245, re-check 3849238274) — workspace-scoped NODE_EXTRA_CA_CERTS divergence: the round-13 re-check asserted 'still stands' and the autofix rebuttal asserted 'disproved' with a mechanism trace; the surface now lives on …
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane, taken off pull requests by the base commit 0756be0) and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane, taken off pull requests by the base commit 0756be0) and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only and dormant) and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run.
Not reviewed: test-efficacy — harness unvalidated (the positive control never ran: no probe file was green in the unmutated baseline); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean.
Not reviewed: build-and-test — the packages/cli unit suite hit its whole-call budget before completing in the scoped run; the four files that failed before the kill are untouched by this diff and fail identically on the merge base (environmental HOME-mock signature), and the PR's serve suites were exercised by focused verifier runs instead.
Deferred under the convergence posture (round 17, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/run-qwen-serve.ts:771 — [probe] bracketed IPv4 --hostname refused at channel boot with a misnamed zone diagnostic (round-12 sibling recorded; current named-diagnostic form new this round)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit (head d780f91): the round-15 entrance fixes from f61db34 (BOM-at-scan-start, RFC1421 blank-separator enforcement, nameConstraints model) remain discarded by merge 337443c — nameConstraints is referenced nowhere at HEAD, and pem-certificate-blocks.ts is byte-identical to main. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD d780f91 this round): git grep -ci nameconstraints -- packages/cli/src/serve/ -> zero matches; git diff 0756be0..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts -> empty; git log c01c800..HEAD -> only main merges plus test-only 8f80905.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane, taken off pull requests by the base commit 0756be0) and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane, taken off pull requests by the base commit 0756be0) and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only and dormant) and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run。
未审查：test-efficacy — harness unvalidated (the positive control never ran: no probe file was green in the unmutated baseline); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean。
未审查：build-and-test — the packages/cli unit suite hit its whole-call budget before completing in the scoped run; the four files that failed before the kill are untouched by this diff and fail identically on the merge base (environmental HOME-mock signature), and the PR's serve suites were exercised by focused verifier runs instead。
收敛姿态下延后（第 17 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit (head d780f91): the round-15 entrance fixes from f61db34 (BOM-at-scan-start, RFC1421 blank-separator enforcement, nameConstraints model) remain discarded by merge 337443c — nameConstraints is referenced nowhere at HEAD, and pem-certificate-blocks.ts is byte-identical to main. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD d780f91 this round): git grep -ci nameconstraints -- packages/cli/src/serve/ -> zero matches; git diff 0756be0..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts -> empty; git log c01c800..HEAD -> only main merges plus test-only 8f80905.
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no code changes (triage only)This Critical-only round received two Critical findings from review 5029075758. Both target the R2-21 (Critical, ledger re-post) — deferred to the follow-up queueThe finding is verified still standing at HEAD
The structural rewrite (driving Node's real R13-1 (Critical, "Unresolved, please confirm") — escalated for a maintainer rulingThis finding cannot be settled autonomously this round, for three recorded reasons:
A fresh mechanism probe was run at HEAD
Open question for the maintainer: does the re-check evidence name a concrete divergence mechanism the probe above misses? If yes, please point at it — and since the surface now lives on main, we recommend it ride with the R2-21 follow-up class rather than this PR. If no such mechanism exists at current HEAD, we recommend closing R13-1 as disproved at Other feedback in review 5029075758
VerificationNo code changes this round, so no build/typecheck/lint/test commands were required or run. All evidence above was gathered read-only at HEAD 中文说明Autofix 评审轮次：无代码改动（仅分类处理）本轮为仅处理 Critical 的模式，收到来自评审 5029075758 的两条 Critical 发现。两者都指向 R2-21（Critical，分类账重发）——延后到后续跟进队列该发现在 HEAD
结构性重写（以 Node 真实的 R13-1（Critical，“未决，请确认”）——升级给维护者裁定本轮无法自主了结该发现，记录三条原因：
本轮在 HEAD
**留给维护者的开放问题：**复查证据是否指出了上述探测遗漏的具体分歧机制？若有，请指明——并且由于该表面现已位于 main，我们建议将其与 R2-21 的后续跟进归为同一类，而非放在本 PR。若在当前 HEAD 上不存在这样的机制，我们建议基于上述探测证据，在 评审 5029075758 中的其他反馈
验证本轮无代码改动，因此不需要也未运行任何 build/typecheck/lint/测试命令。以上全部证据均在 HEAD Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
🔬 Sandboxed verification is running — watch live progress. The report will be posted here when the run completes. 🔬 沙箱验证正在运行 —— 查看实时进度。运行结束后验证报告会发布在这里。 |
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. ✅
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round summary — PR #9406 (no action taken)This round found no actionable feedback, so no code changes were made and nothing was committed. What was checked:
Result: the PR head stays as-is. No verification commands were run because no changes were made. 中文说明Autofix 轮次总结 — PR #9406（未采取任何操作）本轮没有可处理的反馈，因此未做任何代码改动，也没有提交任何内容。 检查情况如下：
结果：PR 头部提交保持不变。由于未做任何改动，本轮未运行任何验证命令。 Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Maintainer verification — real stack, no mocksI built both sides from source (
Isolated Bottom line: the feature in the title is correct, honest in both directions, and well covered. I found two issues, both in the second, undocumented half of the diff — one inaccurate claim and one narrow regression. Details and evidence below. Part 1 —
|
| Host | zenity | display / session | tag advertised | Browse… in dialog |
|---|---|---|---|---|
| macOS, console session | n/a | GUI console | ✅ yes | shown |
macOS, same host, SSH_CONNECTION set |
n/a | remote markers | ❌ no | hidden |
| Linux container | absent | none | ❌ no | hidden |
| Linux container | installed | none | ❌ no | — |
| Linux container | installed | DISPLAY=:99 (Xvfb) |
✅ yes | shown |
The capability envelope is surgically scoped — three real daemons, features arrays sorted and diffed:
head @ GUI host 118 features
head @ headless 117 features
base @ headless 117 features
diff head(GUI) head(headless) -> 20d19 < native_directory_picker
diff head(headless) base(headless) -> (identical)
So the PR adds exactly one tag on a capable host and changes nothing at all on a headless one.
Before / after on a genuinely headless Linux daemon
This is the evidence the PR body said it could not capture. Base build on a headless container: the button is shown and clicking it fails, exactly as #9404 describes. Head build, same host: the button is gone.
Daemon log for the failing click (base):
qwen serve: native directory picker unavailable: spawn zenity ENOENT
[WARN] [DAEMON] route=POST /workspace-directory-picker durationMs=9 status=501
And with zenity actually installed but no display, on the same host:
$ env -u DISPLAY zenity --file-selection --directory
(zenity:4625): Gtk-WARNING **: cannot open display:
— the guaranteed failure this PR exists to hide. The probe correctly withholds the tag in that state too.
The positive direction is honest, not just permissive
A gate is only as good as its "yes". On a Linux desktop host (zenity + real X server) the tag is advertised and the whole path works: Browse → real GTK folder chooser rendered on the daemon host's X display → chosen path lands back in the input and enables Register.
The middle panel is the real zenity window captured off the daemon's X display (import -window root), confirmed by xwininfo:
0x200007 "Select a workspace folder": ("zenity" "Zenity") 1096x822+0+0
macOS — the ⚠️ row in your test table is now covered
The PR marks macOS untested. On a real macOS GUI host the daemon does reach the native picker: hitting POST /workspace-directory-picker spawned
PID PPID COMMAND
78361 68189 osascript -l JavaScript -e const app = Application.currentApplication(); ...
app.chooseFolder({ withPrompt: "Select a workspace folder", })...
where 68189 is the daemon itself. So the new macOS gate (uid > 0 ∧ uid == owner(/dev/console) ∧ no SSH markers) says yes on a host where the picker genuinely works, and no once SSH markers are present:
Note this is a behaviour change on macOS: before this PR the button always appeared there; now a macOS daemon started from an SSH session hides it. That matches the PR's stated intent and I agree with it, but it is worth calling out in the changelog.
Tests
native-directory-picker · process-env-guard · local-bind-addresses → 32 passed.
server.test.ts · run-qwen-serve.test.ts · daemon-worker.test.ts → 1511 passed.
web-shell/App.test.tsx → 541 passed. AddWorkspaceDialog.test.tsx → 36 passed.
Mutation matrix (each mutation applied to head, targeted suite run, then reverted):
| # | Mutation | Result |
|---|---|---|
| M1 | drop the statSync(file).isFile() guard in isExecutableFile |
🔴 caught |
| M2 | macOS branch → return true |
🔴 caught |
| M4 | make App.tsx pass onPick unconditionally again |
🔴 caught |
| M6 | map :: back to 127.0.0.1 |
🔴 caught (7 tests) |
| M5 | delete nativeDirectoryPickerAvailable: wiring in server.ts |
🟢 survives on a headless host |
| M3 | delete the assertChannelWorkerDaemonUrlIsLocal(...) boot call |
🟢 survives |
Part 2 — the undocumented second half (channel worker daemon URL)
Roughly 40% of the diff is a separate feature the PR body does not describe. I verified it too, since it ships with the same merge.
The real bug it fixes — confirmed end to end
qwen serve --hostname <LAN-IP> --token … --channel gh, real daemon, real worker, fake GitHub API:
base (main) |
head (this PR) | |
|---|---|---|
| worker | [Channel] daemon worker failed: QWEN_DAEMON_URL must use an http(s) loopback URL. |
[Channel:gh] authenticated as "fake-user" → "gh" connected. |
| daemon | Channel worker exited before ready (code=1) → exits 1 |
stays up, /health → {"status":"ok"} on the LAN address |
| GitHub API calls | 0 | 3 (/user, then /notifications polls) |
So the documented LAN flow is genuinely broken on main and genuinely fixed here. Same story for a DNS-name bind (--hostname <ip>.nip.io --channel gh): base dies with the same confusing worker crash, head refuses at boot with an actionable message. Good change.
🔎 Finding 1 — the stated rationale for :: → [::1] does not apply to this daemon
docs/users/qwen-serve.md and the code comment both say a kernel with net.ipv6.bindv6only=1 "will not answer 127.0.0.1 on an IPv6 wildcard socket". Measured in a container with that sysctl actually set to 1:
sysctl net.ipv6.bindv6only = 1
listen({host:"::"}) bound family=IPv6 dial127=ok dial::1=ok
listen({host:"::", ipv6Only:true}) bound family=IPv6 dial127=ECONNREFUSED dial::1=ok
libuv always sets IPV6_V6ONLY explicitly (to 0 unless UV_TCP_IPV6ONLY is requested), so the sysctl never reaches a Node server. run-qwen-serve.ts calls server.listen(attemptPort, listenHostname) — no ipv6Only — so the daemon's :: socket stays dual-stack. The oracle test only reddens because the test itself passes ipv6Only: true, which the product never does.
End to end on a bindv6only=1 host, --hostname :: --channel gh: base already works (worker URL http://127.0.0.1:4170, daemon up, channel connected). The motivating failure does not reproduce.
The remaining honest justification is "a host with no IPv4 at all", which I did not construct. Worth rewording the doc bullet and the comment to that case.
🔴 Finding 2 — the mapping regresses a host where lo carries no ::1
qwen serve --hostname :: --token … --channel gh on Linux with net.ipv6.conf.lo.disable_ipv6=1 (IPv6 present elsewhere, so :: still binds):
--- base ---
lo inet6 count: 0
worker QWEN_DAEMON_URL=http://127.0.0.1:4170
RESULT: daemon UP (fake-github requests: 2)
--- head ---
lo inet6 count: 0
worker QWEN_DAEMON_URL=http://[::1]:4170
RESULT: daemon EXITED code=1
[WARN] [DAEMON] channel worker stderr: [Channel] daemon worker failed: fetch failed
Control on a normal dual-stack container: both arms up, base on 127.0.0.1, head on [::1].
This is precisely the state the PR's own dial oracle skips:
// EADDRNOTAVAIL means the certified loopback is not assigned on this host// (IPv6-less runners bind ::yet carry no::1) — the arm is unmeasurable here`
It is not merely unmeasurable — it is the one state where the new mapping is wrong, and the test continues past it instead of failing. Suggested fix: derive the worker loopback from what the host actually assigns (probe ::1, fall back to 127.0.0.1) rather than from the bind spelling. That also makes Finding 1 moot.
🟡 Finding 3 — the boot guard's call site is untested (M3)
Deleting assertChannelWorkerDaemonUrlIsLocal(workerDaemonUrl, opts.hostname); from run-qwen-serve.ts leaves run-qwen-serve.test.ts fully green (345 passed). Nothing in the repo asserts the boot diagnostic reaches a real boot — the suite only calls the function directly. Cheap to close with one runQwenServe boot test.
🟡 Finding 4 — the capability wiring is only covered on GUI runners (M5)
server.test.ts builds its expectation with nativeDirectoryPickerAvailable: isNativeDirectoryPickerAvailable() — the same call the product path uses. When the probe returns false, both sides say "tag absent" regardless of whether server.ts wires the flag at all:
M5 (delete the server.ts wiring) on a GUI host -> 🔴 1 failed
M5 (delete the server.ts wiring) on a headless host -> 🟢 1 passed
CI runs Test (ubuntu-latest) headless, so this wiring can silently regress on main. The integration capabilities snapshot splices the tag the same conditional way and has the same blind spot. A predicate-level assertion with an injected true/false (which the conditional-feature test already does) plus one direct assertion that createServeApp forwards the probe result would close it.
🔵 Finding 5 — bootstrap window (minor, acknowledged)
The daemon answers GET /capabilities with HTTP 200 for 1.1–2.3 s before it starts advertising the tag, even on a GUI host (measured twice on the Xvfb host: first 200 at +1580ms picker=false → tag at +2729ms; and +5146ms → +7466ms). The Web Shell computes nativeDirectoryPickerSupported from that snapshot and only calls refreshCapabilities after workspace add / switch / remove — never on a timer or on dialog open. A tab that loads inside that window hides Browse on a capable host until one of those events. The PR calls the fail-closed bootstrap window intentional and it matches sibling tags, so I am flagging it rather than blocking on it.
Not covered
- Windows entirely (no host available) — the
SESSIONNAMEbranch is unit-tested only. - macOS picker behaviour over a real SSH session; I verified the gate reads the SSH markers, not that
osascriptfails there. - A truly IPv4-less host, where the
[::1]mapping would be the genuine fix.
Recommendation
- Part 1 is ready. Correct, honest in both directions, well tested; the mutation matrix backs the new tests.
- Part 2 should not ride along as-is. Finding 2 is a real regression (a working
--hostname ::+ channels host stops booting) and Finding 1 means the doc and comment describe a mechanism that does not apply. Either split it out, or fix the loopback selection to probe the host and reword the rationale. - Findings 3 and 4 are small, cheap test additions worth doing before merge since both let a silent regression through on this project's own CI.
All CI checks on the PR are currently green; it is blocked only on review.
中文说明
维护者验证 —— 真实环境，无 mock
我从源码构建了两侧（npm ci && npm run bundle），用真实 qwen serve daemon + 真实 Chrome 驱动实际发布的 Web Shell，在两台主机上跑：
| head | d780f91a38（本 PR） |
| base | 0756be0ce7（与 main 的 merge-base） |
| 主机 A | macOS 26.6 arm64，Node 24.18.1，真实 GUI 控制台会话 |
| 主机 B | Debian bookworm arm64 容器，三种状态：完全 headless / 装了 zenity 但无显示 / zenity + 真实 Xvfb X 服务器 |
每臂独立 QWEN_HOME；用记录型假 GitHub API 替代 channel 后端，以便量化 worker 是否真的起来了。
结论：标题所述的功能是正确的，正反两个方向都诚实，覆盖也到位。我发现两个问题，都在 diff 中未被文档说明的第二半——一处说法不成立，一处窄场景回归。
第一部分 —— native_directory_picker（PR 描述的功能）：✅ 验证通过
能力探测矩阵（真实 daemon 的真实 GET /capabilities）
| 主机 | zenity | 显示环境 / 会话 | 是否广播 tag | 对话框中的 Browse… |
|---|---|---|---|---|
| macOS，控制台会话 | n/a | GUI 控制台 | ✅ 有 | 显示 |
macOS，同一主机 + SSH_CONNECTION |
n/a | 远程标记 | ❌ 无 | 隐藏 |
| Linux 容器 | 未装 | 无 | ❌ 无 | 隐藏 |
| Linux 容器 | 已装 | 无 | ❌ 无 | — |
| Linux 容器 | 已装 | DISPLAY=:99（Xvfb） |
✅ 有 | 显示 |
能力信封的改动面很干净——三个真实 daemon，features 数组排序后对比：
head @ GUI 主机 118 项
head @ headless 117 项
base @ headless 117 项
diff head(GUI) head(headless) -> 20d19 < native_directory_picker
diff head(headless) base(headless) -> 完全相同
即：在有能力的主机上恰好多一个 tag，在 headless 主机上与 base 完全一致。
headless Linux 上的前后对比
这正是 PR 正文说"无法截取"的证据。base 构建跑在 headless 容器上：按钮显示，点击必然失败，与 #9404 描述一致。head 构建、同一主机：按钮消失。
base 点击失败时的 daemon 日志：native directory picker unavailable: spawn zenity ENOENT，路由返回 501。
同一主机上装了 zenity 但无 DISPLAY 时：Gtk-WARNING **: cannot open display: —— 正是本 PR 要消除的那种必然失败；探测在该状态下也正确地不广播 tag。
正方向同样诚实，不只是"宽松放行"
门控的价值取决于它说"是"的时候对不对。在 Linux 桌面主机（zenity + 真实 X 服务器）上 tag 被广播，且整条链路真的能用：Browse → daemon 主机 X 显示上弹出真实 GTK 目录选择器 → 选中路径回填输入框并激活 Register。
中间那格是从 daemon 的 X 显示上抓下来的真实 zenity 窗口（import -window root），xwininfo 佐证窗口标题为 Select a workspace folder。
macOS —— 测试表里的 ⚠️ 这一格现在补上了
PR 标注 macOS 未测。在真实 macOS GUI 主机上，daemon 确实能打开原生选择器：调用 POST /workspace-directory-picker 后 fork 出了 osascript -l JavaScript ... chooseFolder(...)，其 PPID 正是 daemon 进程。所以新的 macOS 门控（uid > 0 ∧ uid == /dev/console 属主 ∧ 无 SSH 标记）在选择器真能用的主机上说"是"，在出现 SSH 标记后说"否"：
注意这是 macOS 上的行为变更：本 PR 之前该按钮在 macOS 恒显示，现在从 SSH 会话启动的 macOS daemon 会隐藏它。方向与 PR 意图一致，我认同，但值得写进 changelog。
测试
picker / env-guard / local-bind-addresses → 32 通过；server + run-qwen-serve + daemon-worker → 1511 通过；App.test.tsx → 541 通过；AddWorkspaceDialog → 36 通过。
变异矩阵（在 head 上施加变异、跑定向套件、再还原）：
| # | 变异 | 结果 |
|---|---|---|
| M1 | 去掉 isExecutableFile 里的 isFile() 守卫 |
🔴 被捕获 |
| M2 | macOS 分支改成 return true |
🔴 被捕获 |
| M4 | 恢复 App.tsx 无条件传 onPick |
🔴 被捕获 |
| M6 | 把 :: 映射改回 127.0.0.1 |
🔴 被捕获（7 条） |
| M5 | 删除 server.ts 里的 nativeDirectoryPickerAvailable: 接线 |
🟢 headless 主机上存活 |
| M3 | 删除 boot 处的 assertChannelWorkerDaemonUrlIsLocal(...) 调用 |
🟢 存活 |
第二部分 —— 未被文档说明的第二半（channel worker 的 daemon URL）
diff 里约 40% 是 PR 正文未描述的另一个功能。既然它会随同一次合入上车，我一并验证了。
它修复的真实缺陷 —— 已端到端确认
qwen serve --hostname <LAN-IP> --token … --channel gh，真实 daemon、真实 worker、假 GitHub API：
base（main） |
head（本 PR） | |
|---|---|---|
| worker | daemon worker failed: QWEN_DAEMON_URL must use an http(s) loopback URL. |
authenticated as "fake-user" → "gh" connected. |
| daemon | Channel worker exited before ready (code=1) → 退出 1 |
保持运行，LAN 地址上 /health 返回 ok |
| GitHub API 调用数 | 0 | 3 |
所以文档中的 LAN 流程在 main 上确实是坏的，本 PR 确实修好了。DNS 名绑定（--hostname <ip>.nip.io --channel gh）同理：base 以同样令人困惑的 worker 崩溃收场，head 在 boot 阶段给出可操作的拒绝信息。这个改动是好的。
🔎 发现 1 —— :: → [::1] 的立论对本 daemon 不成立
docs/users/qwen-serve.md 与代码注释都称：net.ipv6.bindv6only=1 的内核下 IPv6 通配 socket 不会应答 127.0.0.1。在真的把该 sysctl 设为 1 的容器里实测：
sysctl net.ipv6.bindv6only = 1
listen({host:"::"}) bound family=IPv6 dial127=ok dial::1=ok
listen({host:"::", ipv6Only:true}) bound family=IPv6 dial127=ECONNREFUSED dial::1=ok
libuv 总是显式设置 IPV6_V6ONLY（未请求 UV_TCP_IPV6ONLY 时设为 0），所以该 sysctl 根本影响不到 Node 服务端。run-qwen-serve.ts 用的是 server.listen(attemptPort, listenHostname)，没有 ipv6Only，因此 daemon 的 :: socket 始终是双栈。那条 oracle 测试之所以能红，是因为测试自己传了 ipv6Only: true，而产品代码从不这么做。
端到端：在 bindv6only=1 的主机上跑 --hostname :: --channel gh，base 本来就是好的（worker URL 为 http://127.0.0.1:4170，daemon 存活，channel 已连接）。所声称的触发场景复现不出来。
剩下站得住的理由是"完全没有 IPv4 的主机"，这个我没有构造。建议把文档条目和注释改写到那个场景。
🔴 发现 2 —— 该映射在 lo 没有 ::1 的主机上造成回归
Linux 上设 net.ipv6.conf.lo.disable_ipv6=1（其他接口仍有 IPv6，因此 :: 照常能绑），跑 qwen serve --hostname :: --token … --channel gh：
--- base ---
lo inet6 count: 0
worker QWEN_DAEMON_URL=http://127.0.0.1:4170
RESULT: daemon UP (fake-github requests: 2)
--- head ---
lo inet6 count: 0
worker QWEN_DAEMON_URL=http://[::1]:4170
RESULT: daemon EXITED code=1
[Channel] daemon worker failed: fetch failed
普通双栈容器作对照：两臂都正常，base 走 127.0.0.1，head 走 [::1]。
这恰恰是 PR 自己的 dial oracle 跳过的那个状态：注释写着「IPv6-less runners bind :: yet carry no ::1）—— the arm is unmeasurable here」。它不只是"测不了"，而是新映射唯一出错的状态，而测试选择 continue 而不是失败。建议改成按主机实际拥有的地址决定 worker 的 loopback（探测 ::1，回落 127.0.0.1），而不是按绑定拼写决定；这样发现 1 也一并消解。
🟡 发现 3 —— boot 守卫的调用点无测试（M3）
从 run-qwen-serve.ts 删掉 assertChannelWorkerDaemonUrlIsLocal(workerDaemonUrl, opts.hostname); 后，run-qwen-serve.test.ts 全绿（345 通过）。仓库里没有任何测试断言这条 boot 诊断真的会在启动路径上触发——套件只直接调用该函数。补一条 runQwenServe 启动测试即可。
🟡 发现 4 —— 能力接线只在 GUI runner 上被覆盖（M5）
server.test.ts 用 nativeDirectoryPickerAvailable: isNativeDirectoryPickerAvailable() 构造期望值，与产品路径调用的是同一个函数。当探测返回 false 时，无论 server.ts 是否接线，两边都是"无 tag"：
删除 server.ts 接线，GUI 主机 -> 🔴 1 failed
删除 server.ts 接线，headless 主机 -> 🟢 1 passed
CI 的 Test (ubuntu-latest) 正是 headless，所以这处接线可以在 main 上悄悄回归。集成测试里的 capabilities 快照用同样的条件拼接，盲区相同。补一条"createServeApp 确实把探测结果透传下去"的直接断言即可闭合。
🔵 发现 5 —— bootstrap 窗口（次要，PR 已承认）
即使在 GUI 主机上，daemon 也会先以 HTTP 200 应答 GET /capabilities 1.1–2.3 秒之后才开始广播该 tag（在 Xvfb 主机上量了两次：first 200 at +1580ms picker=false → tag at +2729ms；以及 +5146ms → +7466ms）。Web Shell 用这份快照计算 nativeDirectoryPickerSupported，而 refreshCapabilities 只在工作区新增/切换/移除后调用，既不定时也不在打开对话框时刷新。落在该窗口内加载的页面，会在有能力的主机上隐藏 Browse，直到发生上述事件之一。PR 已说明该 fail-closed 窗口是有意为之且与同族 tag 一致，故此处只做标注，不构成阻塞。
未覆盖
- Windows 全部（无可用主机）——
SESSIONNAME分支仅有单测。 - macOS 经真实 SSH 会话时的选择器行为；我验证的是门控读取了 SSH 标记，而非
osascript在那里确实失败。 - 完全没有 IPv4 的主机，即
[::1]映射真正能修复的场景。
建议
- 第一部分可以合。 正确、正反方向都诚实、测试到位，变异矩阵能支撑新增测试。
- 第二部分不宜就这么搭车。 发现 2 是真实回归（原本能用的
--hostname ::+ channels 主机会启动失败），发现 1 说明文档与注释描述的机制并不适用。建议拆分，或修成按主机探测 loopback 并改写立论。 - 发现 3、4 是很便宜的补测，建议合入前做掉——两者都能让静默回归通过本项目自己的 CI。
PR 当前所有 CI 检查为绿，仅卡在 review。
…wenLM#9406) The `::` wildcard mapping handed every channel worker `[::1]`, chosen from the bind spelling. A host that binds `::` while its loopback carries no `::1` (e.g. `net.ipv6.conf.lo.disable_ipv6=1`) has only `127.0.0.1` for the dual-stack socket to answer on, so the first worker's `fetch failed` exited the daemon — a working `--hostname ::` + channels host stopped booting. Derive the dial-back loopback from the host's interface table instead: `[::1]` when the host assigns it (an IPv4-less host has no other loopback), `127.0.0.1` otherwise. The doc bullet and the code comment claimed `net.ipv6.bindv6only=1` kernels would not answer `127.0.0.1` on an IPv6 wildcard socket. libuv always sets IPV6_V6ONLY explicitly (to 0 unless `ipv6Only` is requested), so that sysctl never reaches a Node listener; reword both to the mechanism that actually applies. The dial oracle's v6-only arm modelled that same impossible bind — dropped along with the EADDRNOTAVAIL escape that skipped the one host state the mapping got wrong; the certified URL is now assigned by construction, so the dial must succeed wherever the bind does. Also close the two review-matrix mutations that survived on headless CI: the boot-time worker URL certification gains a runQwenServe boot test (an injected certifier records the call and a refusing one fails the channel boot before workers start), and the capability wiring gets an injected probe result so the native_directory_picker forwarding is assertable without a GUI runner. Verified by mutation: collapsing either mapping branch, making the loopback probe constant, deleting the boot certification call, and deleting the capability wiring each fail their focused suites; the branch shape passes them all.
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #9406Feedback addressed: the maintainer verification comment ([ic:5427417664]) with five findings. Critical-only mode is active; all in-scope actionable items were Critical/Required or cheap witnesses the maintainer asked for before merge. One commit: Dispositions🔴 Finding 2 —
|
|
Qwen Code review was cancelled before a review could be posted. Nothing failed and nothing is retried automatically: the run was cancelled — by an operator, an upstream event, or the job exceeding its execution time limit. If you still want a review of this PR, request one with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- IPv4-mapped own/loopback literals refused with a factually wrong diagnostic (run-qwen-serve.ts:786 / local-bind-addresses.ts:44) — already reported (round-16 deferral list, review 5025220188; comment 3815211054)
- macOS probe SSH_TTY conjunct pinned by no test (native-directory-picker.ts:40-41) — already reported (round-16 deferral list, review 5025220188)
- integration-tests/cli/qwen-serve-routes.test.ts collected by no workspace suite — already reported in rounds 2-17
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane, taken off pull requests by the base commit 0756be0) and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane, taken off pull requests by the base commit 0756be0) and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only and dormant) and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run.
Not reviewed: test-efficacy — harness unvalidated (the positive control never ran: no probe file was green in the unmutated baseline); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds; round 4 surfaced the 127/8 Host-gate Critical, round 5 one low-confidence finding (verified, terminal-only).
Deferred under the convergence posture (round 18, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/native-directory-picker.test.ts:213 — [probe] macOS probe uid clauses (consoleUid === processUid, processUid > 0) pinned by no test; both deletion mutants ship greenpackages/cli/src/serve/local-bind-addresses.test.ts:31 — [probe] suite comment's vi.mock('node:os')-cannot-reach-the-module claim is false and its derived mocked-suite conclusion flips a faithful stub red
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 0), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit (head a7e9e1a): the round-15 entrance fixes from f61db34 (BOM-at-scan-start, RFC1421 blank-separator enforcement, nameConstraints model) remain discarded by merge 337443c — nameConstraints is referenced nowhere at HEAD, and pem-certificate-blocks.ts is byte-identical to the merge base. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD a7e9e1a this round): git grep -ci nameconstraints -- packages/cli/src/serve/ -> zero matches; git diff 0756be0..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts -> empty; describeWorkerTlsTrustGaps present in run-qwen-serve.ts.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane, taken off pull requests by the base commit 0756be0) and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane, taken off pull requests by the base commit 0756be0) and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only and dormant) and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run。
未审查：test-efficacy — harness unvalidated (the positive control never ran: no probe file was green in the unmutated baseline); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds; round 4 surfaced the 127/8 Host-gate Critical, round 5 one low-confidence finding (verified, terminal-only)。
收敛姿态下延后（第 18 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 2 条 Critical），首次发现的速率没有下降（本轮 1，上一轮 0），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit (head a7e9e1a): the round-15 entrance fixes from f61db34 (BOM-at-scan-start, RFC1421 blank-separator enforcement, nameConstraints model) remain discarded by merge 337443c — nameConstraints is referenced nowhere at HEAD, and pem-certificate-blocks.ts is byte-identical to the merge base. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD a7e9e1a this round): git grep -ci nameconstraints -- packages/cli/src/serve/ -> zero matches; git diff 0756be0..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts -> empty; describeWorkerTlsTrustGaps present in run-qwen-serve.ts.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| `a zone-less literal address of one of this machine's interfaces.`, | ||
| ); | ||
| } | ||
| if (isLoopbackBind(host) || isOwnInterfaceAddress(host)) return; |
There was a problem hiding this comment.
[Critical] R18-1: The new boot certifier accepts every 127.0.0.0/8 spelling, but the daemon's own Host gate answers 403 to all of them except 127.0.0.1 — so qwen serve --hostname 127.0.0.2 --channel … passes the new boot certification and then every channel worker is rejected by the daemon itself: the first worker's failure exits the daemon, dynamically added channels restart-loop while /health stays green — the exact failure class this guard's docstring promises to name once, at boot.
isLoopbackBind accepts any 127.x.y.z (loopback-binds.ts), so assertChannelWorkerDaemonUrlIsLocal returns early for 127.0.0.2; formatChannelWorkerDaemonUrl passes the literal through unchanged and the worker dials http://127.0.0.2:<port>. The request lands on the primary listener, whose buildPrimaryHostGate allowlist (auth.ts) is exactly localhost:<port>, 127.0.0.1:<port>, [::1]:<port>, host.docker.internal:<port> — any other Host gets 403 Invalid Host header. The worker-side validateDaemonWorkerUrl widened in this diff shares the same 127/8 acceptance.
Witness (probe at HEAD a7e9e1a, unmodified PR code):
listen(0, '127.0.0.2') -> BOUND, serves
isLoopbackBind('127.0.0.2') -> true
formatChannelWorkerDaemonUrl('127.0.0.2', 4170) -> "http://127.0.0.2:4170"
assertChannelWorkerDaemonUrlIsLocal(that URL, host) -> ACCEPTED (did not throw)
GET /health with Host: 127.0.0.2:<port> -> 403 {"error":"Invalid Host header"}
controls Host: 127.0.0.1:<port> / localhost:<port> -> 200
with the fix below applied (scratch tree) -> certifier THREW the named boot diagnostic
Accept only the loopback spellings the Host gate also accepts — membership in {127.0.0.1, localhost, ::1, [::1]} — in both assertChannelWorkerDaemonUrlIsLocal and validateDaemonWorkerUrl, so other 127/8 spellings fall through to the named boot refusal:
const HOST_GATE_LOOPBACKS = new Set(['127.0.0.1', 'localhost', '::1', '[::1]']);
// in both validators, replacing the isLoopbackBind(host) branch:
if (HOST_GATE_LOOPBACKS.has(host.toLowerCase()) || isOwnInterfaceAddress(host)) return;(host.docker.internal is in the Host gate's allowlist but is not a loopback bind this certifier would accept today — refusing it at boot is safe; decide deliberately if it should be accepted.)
Fix witness: please add a unit test asserting assertChannelWorkerDaemonUrlIsLocal('http://127.0.0.2:8080', '127.0.0.2') throws, and prove it pins the guard by removing the restriction and confirming the test turns red.
中文说明
[严重问题] R18-1：新的启动认证器接受 127.0.0.0/8 的全部写法，但 daemon 自己的 Host 门只对其中 127.0.0.1 放行，其余一律 403 —— 因此 qwen serve --hostname 127.0.0.2 --channel … 能通过新增的启动认证，随后每个 channel worker 都被 daemon 自己拒绝：首个 worker 失败导致 daemon 退出，动态添加的 channel 反复重启而 /health 保持绿色 —— 正是该守卫的 docstring 承诺「在启动时一次性指明」的那类失败。
isLoopbackBind 接受任意 127.x.y.z（loopback-binds.ts），所以 assertChannelWorkerDaemonUrlIsLocal 对 127.0.0.2 提前返回；formatChannelWorkerDaemonUrl 原样透传该字面量，worker 拨打 http://127.0.0.2:<port>。请求落在主监听器上，其 buildPrimaryHostGate 白名单（auth.ts）恰好只有 localhost:<port>、127.0.0.1:<port>、[::1]:<port>、host.docker.internal:<port> —— 其余 Host 一律 403 Invalid Host header。本 diff 中放宽的 worker 侧 validateDaemonWorkerUrl 同样接受整个 127/8。
证据（HEAD a7e9e1a 上的探针，未改动的 PR 代码）：listen(0, '127.0.0.2') 可绑定并服务；isLoopbackBind('127.0.0.2') 为 true；formatChannelWorkerDaemonUrl('127.0.0.2', 4170) 生成 http://127.0.0.2:4170；assertChannelWorkerDaemonUrlIsLocal 接受（未抛错）；以 Host: 127.0.0.2:<port> 请求 /health 得 403 Invalid Host header；对照 Host: 127.0.0.1:<port> / localhost:<port> 均为 200；在 scratch tree 应用下述修复后，认证器抛出具名启动诊断。
建议修复：在 assertChannelWorkerDaemonUrlIsLocal 与 validateDaemonWorkerUrl 两处，只接受 Host 门同样接受的回环写法 —— 即 {127.0.0.1, localhost, ::1, [::1]} 的成员 —— 让其余 127/8 写法落入具名启动拒绝（代码见上方英文部分）。注意：host.docker.internal 在 Host 门白名单中，但不是该认证器今天会接受的回环绑定 —— 在启动时拒绝它是安全的；如需接受请有意决策。
修复见证：请补一条断言 assertChannelWorkerDaemonUrlIsLocal('http://127.0.0.2:8080', '127.0.0.2') 抛错的单测，并通过移除该限制、确认测试变红来证明它钉住了守卫。
— qwen3.8-max via Qwen Code /review (v0.22.2)
QwenLM#9406) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 8/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 8/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9406 (Critical-only mode)Critical-only mode is active (5 change-producing rounds completed); only Critical findings, failed checks, and base-conflict resolution were in scope this round. No conflict was reported and no merge was performed. Feedback points and dispositions1. R18-1 —
|
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
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.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- module-scope mkdtemp fixture dirs in native-directory-picker.test.ts never removed — already recorded in the round-2/3/5 deferral lists (reviews 4966720851, 4967892873, 4974899128)
- macOS probe uid/SSH_TTY clauses pinned by no test — already recorded in the round-18 deferral list (review 5034380407)
- capabilities-envelope expectation mirrors the host probe (server.test.ts) — already recorded in the round-16 deferral list (review 5025220188) and re-confirmed in the round-18 summary (review 5034380407)
- integration-tests/cli/qwen-serve-routes.test.ts collected by no workspace suite — already reported in rounds 2-17 and re-confirmed in the round-18 summary (review 5034380407)
- IPv4-mapped spellings of own/loopback addresses refused by isOwnInterfaceAddress — already reported (comment 3815211054; round-16 deferral list, review 5025220188)
- bracketed non-IPv6 --hostname misdiagnosed as zone-scoped in the certifier's URL-parse catch — already recorded in the round-17 deferral list (review 5014571479)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) is merge-queue-only at this head (the base commit 0756be0 took it off pull requests) and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) is merge-queue-only at this head (the base commit 0756be0 took it off pull requests) and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) is merge-queue-only and dormant and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run.
Not reviewed: test-efficacy — harness unvalidated (the positive control never ran: no probe file was green in the unmutated baseline); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean.
Convergence: round 19 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/cli/src/serve/run-qwen-serve.ts (findings in round 18; 1 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 keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit (head 9e27492): pem-certificate-blocks.ts and channel-worker-supervisor.ts are byte-identical to the merge base (git diff empty this round), nameConstraints is referenced nowhere under packages/cli/src/serve/, and describeWorkerTlsTrustGaps is present in run-qwen-serve.ts. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD this round): git diff a82a11a..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts packages/cli/src/serve/channel-worker-supervisor.ts -> empty; git grep -ci nameconstraints -- packages/cli/src/serve/ -> zero matches; git grep -c describeWorkerTlsTrustGaps -- packages/cli/src/serve/run-qwen-serve.ts -> 2.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) is merge-queue-only at this head (the base commit 0756be0 took it off pull requests) and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Test (macos-latest, Node 22.x) is merge-queue-only at this head (the base commit 0756be0 took it off pull requests) and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) is merge-queue-only and dormant and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run。
未审查：test-efficacy — harness unvalidated (the positive control never ran: no probe file was green in the unmutated baseline); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean。
收敛情况：第 19 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/cli/src/serve/run-qwen-serve.ts（第 18 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 2 条 Critical），首次发现的速率没有下降（本轮 1，上一轮 1），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit (head 9e27492): pem-certificate-blocks.ts and channel-worker-supervisor.ts are byte-identical to the merge base (git diff empty this round), nameConstraints is referenced nowhere under packages/cli/src/serve/, and describeWorkerTlsTrustGaps is present in run-qwen-serve.ts. The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD this round): git diff a82a11a..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts packages/cli/src/serve/channel-worker-supervisor.ts -> empty; git grep -ci nameconstraints -- packages/cli/src/serve/ -> zero matches; git grep -c describeWorkerTlsTrustGaps -- packages/cli/src/serve/run-qwen-serve.ts -> 2.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| `a zone-less literal address of one of this machine's interfaces.`, | ||
| ); | ||
| } | ||
| if (isHostGateLoopback(host) || isOwnInterfaceAddress(host)) return; |
There was a problem hiding this comment.
[Critical] R18-1: (fix-induced) The round-18 fix closed the reported entrance — unassigned wide 127/8 spellings are now refused at boot with a named diagnostic — but its ordering opened a residual entrance of the same class. The acceptance union runs BEFORE the new wide-127/8 refusal, so a wide loopback literal (a 127.x.y.z other than 127.0.0.1) that is ASSIGNED to a local interface — ip addr add 127.0.0.2/8 dev lo, a standard pattern for container meshes and per-service health endpoints — is accepted by isOwnInterfaceAddress and never reaches the refusal. The daemon's Host gate is armed for it (isLoopbackBind('127.0.0.2') is true, and the allowlist answers only localhost/127.0.0.1/[::1]/host.docker.internal), so every worker dial is answered 403 Invalid Host header: qwen serve --hostname 127.0.0.2 --channel telegram passes the new boot certification, the first worker's failure exits the daemon, and dynamically added channels restart-loop while /health stays green — the exact failure class this guard's docstring promises to name once, at boot. The worker-side validator (daemon-worker.ts:338-339) is union-only and shares the escape.
Witness (probe A/B in disposable containers at this commit):
ARM A (no 127.0.0.2 assigned): lo = [127.0.0.1, ::1]
isOwnInterfaceAddress("127.0.0.2") -> false
certifier -> REFUSED: "Channels cannot start: --hostname \"127.0.0.2\" is a
loopback address the daemon's Host header gate refuses…"
ARM B (127.0.0.2 assigned): lo = [127.0.0.1, 127.0.0.2, ::1]
isOwnInterfaceAddress("127.0.0.2") -> true
certifier -> ACCEPTED http://127.0.0.2:4170 (no throw)
worker dial 127.0.0.2 -> status 403 {"error":"Invalid Host header"}
This PR's own pinning tests: ARM A 4 passed; ARM B 2 failed | 2 passed
run-qwen-serve.test.ts 'refuses loopback spellings the Host gate answers 403'
fails at the .toThrow
daemon-worker.test.ts 'rejects loopback spellings the daemon Host gate refuses'
resolves instead of rejecting
Reorder so the gate-refused classification wins over the own-interface escape — in assertChannelWorkerDaemonUrlIsLocal, run the isLoopbackBind(host) wide-127/8 refusal BEFORE the isOwnInterfaceAddress acceptance, and add the corresponding isLoopbackBind-based refusal to validateDaemonWorkerUrl (daemon-worker.ts), which currently has no refusal branch:
// assertChannelWorkerDaemonUrlIsLocal
if (isHostGateLoopback(host)) return;
if (isLoopbackBind(host)) {
throw new Error(/* the existing wide-127/8 Host-gate diagnostic */);
}
if (isOwnInterfaceAddress(host)) return;
// …generic refusalFix witness: please add a unit test that mocks os.networkInterfaces() so lo carries 127.0.0.2 and asserts assertChannelWorkerDaemonUrlIsLocal('http://127.0.0.2:8080', '127.0.0.2') still throws the Host-gate diagnostic, and prove it pins the guard by removing the reorder and confirming the test turns red — the two existing 127.0.0.2 tests are host-state-dependent and pass only on hosts that do not assign the address.
中文说明
[严重问题] R18-1：（修复引入）第 18 轮的修复关闭了原报告的入口——未分配的宽 127/8 写法现在会在启动时被具名诊断拒绝——但该修复的分支顺序打开了同一失败类别的残余入口。接受并集先于新增的宽 127/8 拒绝分支执行，因此已分配到本机网卡的宽回环字面量（127.0.0.1 之外的 127.x.y.z——例如容器网格、按服务健康端点常用的 ip addr add 127.0.0.2/8 dev lo）会被 isOwnInterfaceAddress 接受，永远到不了拒绝分支。而 daemon 的 Host 门对它是武装的（isLoopbackBind('127.0.0.2') 为 true，白名单只应答 localhost/127.0.0.1/[::1]/host.docker.internal），于是每次 worker 拨号都得到 403 Invalid Host header：qwen serve --hostname 127.0.0.2 --channel telegram 能通过新的启动认证，首个 worker 失败导致 daemon 退出，动态添加的 channel 反复重启而 /health 保持绿色——正是该守卫 docstring 承诺「在启动时一次性指明」的失败类别。worker 侧校验器（daemon-worker.ts:338-339）只有并集、没有拒绝分支，同样存在该逃逸。
证据（在本提交上以一次性容器做 A/B 探针）：ARM A（未分配 127.0.0.2）——isOwnInterfaceAddress("127.0.0.2") 为 false，认证器以 R18-1 具名诊断拒绝；ARM B（已分配 127.0.0.2）——isOwnInterfaceAddress("127.0.0.2") 为 true，认证器接受 http://127.0.0.2:4170（未抛错），对该地址的 worker 拨号返回 403 {"error":"Invalid Host header"}。本 PR 自己的钉扎测试同样翻转：ARM A 4 通过；ARM B 2 失败 | 2 通过（run-qwen-serve.test.ts 的 'refuses loopback spellings the Host gate answers 403' 在 .toThrow 处失败；daemon-worker.test.ts 的 'rejects loopback spellings the daemon Host gate refuses' 变为 resolve 而非 reject）。
建议修复：调整顺序，让「Host 门拒绝」的分类优先于「本机网卡」逃逸——在 assertChannelWorkerDaemonUrlIsLocal 中把 isLoopbackBind(host) 的宽 127/8 拒绝分支移到 isOwnInterfaceAddress 接受之前，并在 validateDaemonWorkerUrl（daemon-worker.ts）补上对应的 isLoopbackBind 拒绝分支（目前完全没有）。代码形态见上方英文部分。
修复见证：请补一条单测，mock os.networkInterfaces() 使 lo 携带 127.0.0.2，断言 assertChannelWorkerDaemonUrlIsLocal('http://127.0.0.2:8080', '127.0.0.2') 仍抛出 Host 门诊断，并通过移除上述重排、确认测试变红来证明它钉住了守卫——现有两条 127.0.0.2 测试依赖宿主状态，只在未分配该地址的机器上通过。
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
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.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- integration-tests/cli/qwen-serve-routes.test.ts collected by no workspace suite — already reported in rounds 2-17 and re-confirmed in the round-18 summary (review 5034380407)
- isOwnInterfaceAddress never matches IPv4-mapped IPv6 literals (::ffff:a.b.c.d refused at boot for working binds) — already reported (comment 3815211054; round-16 deferral list)
- production probe fallback
?? isNativeDirectoryPickerAvailable()(server.ts:1006) witnessed by no non-mirror test — already recorded in the round-16 deferral list (review 5025220188) and re-confirmed in the round-18 summary (review 5034380… - bracketed non-IPv6 --hostname misdiagnosed as zone-scoped in the certifier's URL-parse catch — already recorded in the round-17 deferral list (review 5014571479); this round's auditor argued Critical severity for it — maintainer awareness n…
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) is merge-queue-only at this head and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) is merge-queue-only at this head and the cli + web-shell suites ran locally on Linux only.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) is merge-queue-only and dormant and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run.
Not reviewed: test-efficacy — harness unvalidated (the positive control never ran); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean.
Deferred under the convergence posture (round 20, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/run-qwen-serve.ts:8306 — [probe] production boot-certifier fallback identity is pinned by no test
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit (head 95bb71f): pem-certificate-blocks.ts and channel-worker-supervisor.ts are byte-identical to the merge base (git diff empty this round), nameConstraints is referenced nowhere under packages/cli/src/serve/, and describeWorkerTlsTrustGaps is present in run-qwen-serve.ts (2 matches). The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD this round): git diff 1ca8cc5..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts packages/cli/src/serve/channel-worker-supervisor.ts -> empty; git grep -ci nameconstraints -- packages/cli/src/serve/ -> zero matches; git grep -c describeWorkerTlsTrustGaps -- packages/cli/src/serve/run-qwen-serve.ts -> 2.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) is merge-queue-only at this head and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Test (macos-latest, Node 22.x) is merge-queue-only at this head and the cli + web-shell suites ran locally on Linux only。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) is merge-queue-only and dormant and its suite was not run locally; integration-tests/cli/qwen-serve-routes.test.ts (changed by this diff) was exercised by no deterministic test in this run。
未审查：test-efficacy — harness unvalidated (the positive control never ran); mutants/hunks skipped for cap/baseline — efficacy coverage is unprobed, not clean。
收敛姿态下延后（第 20 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R2-21 (ledger re-post, round-2 class finding) — the boot trust-gap diagnostic (describeWorkerTlsTrustGaps) and its shared PEM model still re-implement Node's NODE_EXTRA_CA_CERTS loader and OpenSSL validation semantics instead of driving them. Still stands at the reviewed commit (head 95bb71f): pem-certificate-blocks.ts and channel-worker-supervisor.ts are byte-identical to the merge base (git diff empty this round), nameConstraints is referenced nowhere under packages/cli/src/serve/, and describeWorkerTlsTrustGaps is present in run-qwen-serve.ts (2 matches). The structural rewrite (driving the real loader/validation as the oracle) remains owed to the acknowledged follow-up PR. Note for the maintainer: the surface itself now lives on main (merged via #9392), so this PR's diff no longer touches it — this re-post keeps the class visible until the structural change lands or the maintainer rules the class belongs to the follow-up outright. Witness (re-executed at HEAD this round): git diff 1ca8cc5..HEAD -- packages/cli/src/serve/pem-certificate-blocks.ts packages/cli/src/serve/channel-worker-supervisor.ts -> empty; git grep -ci nameconstraints -- packages/cli/src/serve/ -> zero matches; git grep -c describeWorkerTlsTrustGaps -- packages/cli/src/serve/run-qwen-serve.ts -> 2.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (isHostGateLoopback(host) || isOwnInterfaceAddress(host)) return; | ||
| if (isLoopbackBind(host)) { |
There was a problem hiding this comment.
[Critical] R18-1: Still stands at the reviewed head — the acceptance union runs BEFORE the wide-127/8 Host-gate refusal, so a wide loopback literal assigned to a local interface passes boot certification and every worker dial then gets 403 Invalid Host header. On a host with 127.0.0.2 assigned to lo (ip addr add 127.0.0.2/8 dev lo — a standard container-mesh / per-service-health pattern), qwen serve --hostname 127.0.0.2 --channel telegram binds fine and isOwnInterfaceAddress('127.0.0.2') returns true here, so the union early-returns at line 787 before the isLoopbackBind refusal below can run. But the daemon's primary Host gate is armed for the bind (isLoopbackBind('127.0.0.2') is true) and its allowlist answers only localhost / 127.0.0.1 / [::1] / host.docker.internal — every worker request is answered 403, the first worker's failure exits the daemon, and channels added later restart-loop while /health stays green: the exact failure class this guard's docstring promises to refuse once, at boot. The worker-side validateDaemonWorkerUrl (daemon-worker.ts:338) is union-only with no loopback-refusal branch and shares the escape for hand-launched QWEN_DAEMON_URL workers.
Witness (probe re-executed this round in a disposable netns at this head):
ARM A (127.0.0.2 unassigned): certifier -> REFUSED with the named diagnostic
ARM B (127.0.0.2 assigned): isOwnInterfaceAddress -> true, certifier -> ACCEPTED
dial 127.0.0.2 -> 403 {"error":"Invalid Host header"}
This PR's own pinning tests fail in ARM B at both sites; applying the reorder
below flips ARM B to REFUSED and both pinning tests pass.
Reorder so the Host-gate refusal wins over the own-interface escape, at both sites:
if (isHostGateLoopback(host)) return;
if (isLoopbackBind(host)) {
throw /* the existing wide-127/8 Host-gate diagnostic */;
}
if (isOwnInterfaceAddress(host)) return;Fix witness: please mock os.networkInterfaces() so lo carries 127.0.0.2 and assert assertChannelWorkerDaemonUrlIsLocal('http://127.0.0.2:8080', '127.0.0.2') still throws the Host-gate diagnostic, then remove the reorder and confirm the test turns red — the two existing 127.0.0.2 tests are host-state-dependent and pass only on hosts that do not assign the address.
中文说明
[严重问题] R18-1：在被审头提交上依然存在——接受并集先于宽 127/8 Host 门拒绝执行，因此已分配到本机网卡的宽回环字面量能通过启动认证，随后每次 worker 拨号都得到 403 Invalid Host header。在把 127.0.0.2 分配到 lo 的主机上（ip addr add 127.0.0.2/8 dev lo——容器网格、按服务健康端点的常用模式），qwen serve --hostname 127.0.0.2 --channel telegram 绑定成功，且此时 isOwnInterfaceAddress('127.0.0.2') 返回 true，于是第 787 行的并集提前返回，永远到不了下方 isLoopbackBind 的拒绝分支。而 daemon 的主 Host 门对这一绑定是武装的（isLoopbackBind('127.0.0.2') 为 true），其白名单只应答 localhost / 127.0.0.1 / [::1] / host.docker.internal——每个 worker 请求都被 403 拒绝，首个 worker 失败导致 daemon 退出，动态添加的 channel 反复重启而 /health 保持绿色：正是该守卫 docstring 承诺在启动时一次性拒绝的失败类别。worker 侧的 validateDaemonWorkerUrl（daemon-worker.ts:338）只有并集、没有回环拒绝分支，对手动启动的 QWEN_DAEMON_URL worker 存在同样的逃逸。
证据（本轮在被审头提交上以一次性 netns 重新探针）：ARM A（未分配 127.0.0.2）认证器以具名诊断拒绝；ARM B（已分配 127.0.0.2）isOwnInterfaceAddress 为 true，认证器接受，拨号 127.0.0.2 得到 403 {"error":"Invalid Host header"}；本 PR 自己的钉扎测试在 ARM B 下两处均失败；应用下方重排后 ARM B 翻转为拒绝且两条钉扎测试通过。
建议修复：在两处调整顺序，让 Host 门拒绝优先于本机网卡逃逸（代码形态见上方英文部分）。
修复见证：请 mock os.networkInterfaces() 使 lo 携带 127.0.0.2，断言 assertChannelWorkerDaemonUrlIsLocal('http://127.0.0.2:8080', '127.0.0.2') 仍抛出 Host 门诊断，并通过移除重排确认测试变红——现有两条 127.0.0.2 测试依赖宿主状态，只在未分配该地址的机器上通过。
— qwen3.8-max via Qwen Code /review (v0.22.2)
6 participants
What this PR does
The Web Shell "Add workspace" dialog shows a Browse… button that opens a native OS directory picker on the daemon host —
osascripton macOS, PowerShell on Windows,zenityon Linux. This PR teaches the daemon to advertise that ability as a conditional serve capability, and makes the Web Shell hide the button when the capability is absent. A startup probe decides availability: macOS/Windows always pass; Linux passes only withDISPLAY/WAYLAND_DISPLAYset and an executablezenityfile on PATH (a directory namedzenitydoes not count). The Web Shell only passesonPickto the dialog when the daemon advertises the tag, and the dialog already renders the button conditionally ononPick, so headless hosts simply stop showing the button. The probe also hardens against a directory passing theX_OKcheck (search permission), which would have produced the exact guaranteed-picker-failure the change exists to hide. The integration capabilities snapshot now tolerates the host-conditional tag, the newprocess.envaccess is registered with the serve env guard, and the conditional-feature protocol doc table gained the new row.Why it's needed
On headless daemon hosts — the common deployment for
qwen serve— the button can never work:zenityexits with "cannot open display", and every click surfaces a guaranteed error toast. The affordance advertises something the daemon cannot do. Hiding it when unavailable matches how other conditional capabilities gate their UI surfaces, and fails closed for older daemons that don't advertise the tag.Reviewer Test Plan
How to verify
DISPLAY, e.g. a container or SSH server), startqwen serveand open the Web Shell. Open the Add workspace dialog from the sidebar "+" → the directory path input appears with autocomplete as before, but no Browse… button.GET /capabilitiesomitsnative_directory_pickerfromfeatures.zenityinstalled), startqwen serveand open the same dialog → the Browse… button is present and opens the native picker;GET /capabilitiesincludesnative_directory_picker.cd packages/cli && npx vitest run src/serve/native-directory-picker.test.ts src/serve/process-env-guard.test.ts src/serve/server.test.ts— all green; the capabilities snapshot test mirrors the host probe, so it passes on both GUI and headless machines.Evidence (Before & After)
The linked issue shows the button on a headless deployment (Before). After: on headless hosts the dialog omits the button by construction (
onPickis only wired when the daemon advertises the capability); behavior on GUI hosts is unchanged. Verified via capabilities responses and unit tests; no screenshot capture of the picker itself is possible from a headless reviewer host.Tested on
Environment (optional)
Headless Linux container,
npm run dev/ vitest against source; full cli + web-shell unit suites.Risk & Scope
zenity(or with a broken display) now hide the button even though the daemon runs — correct, since the picker implementation only supportszenitythere. macOS/Windows are advertised unconditionally; a macOS daemon in a GUI-less SSH edge case would still fail on click, same as before this PR.Linked Issues
Closes #9404
中文说明
这个 PR 做了什么
Web Shell「添加工作区」对话框中的 浏览… 按钮会在 daemon 所在主机上打开系统原生目录选择器——macOS 用
osascript，Windows 用 PowerShell，Linux 用zenity。本 PR 让 daemon 把这一能力作为条件式 serve capability 对外广播，并让 Web Shell 在该能力缺失时隐藏按钮。启动时探测决定可用性：macOS/Windows 恒可用；Linux 仅当设置了DISPLAY/WAYLAND_DISPLAY且 PATH 上存在可执行的zenity文件（名为zenity的目录不算）时才可用。Web Shell 仅在 daemon 广播该 tag 时才向对话框传onPick，而对话框本就按onPick条件渲染按钮，因此 headless 主机上按钮自然消失。探测还加固了一个已知陷阱：目录可以通过X_OK检查（搜索权限），若不排除会产生本 PR 要消除的那种"注定失败的选择器"。集成测试的 capabilities 全量快照现在容忍这个随主机环境变化的 tag，新增的process.env访问已登记进 serve env guard，协议文档的条件 feature 表也补了新行。为什么需要
在 headless daemon 主机——
qwen serve的常见部署形态——上，这个按钮永远不可能工作：zenity报 "cannot open display"，每次点击都弹出一个注定的错误提示。按钮承诺了 daemon 做不到的事。不可用时隐藏它与其他条件式 capability 门控 UI 面的既有模式一致，并且对不广播该 tag 的旧版 daemon 安全退化。评审测试计划
如何验证
DISPLAY，如容器或 SSH 服务器）上启动qwen serve并打开 Web Shell，从侧边栏 "+" 打开「添加工作区」对话框 → 目录输入框与自动补全照旧，但没有 浏览… 按钮；GET /capabilities的features中不含native_directory_picker。zenity的 Linux 桌面）上打开同一对话框 → 浏览… 按钮存在且能打开原生选择器；GET /capabilities含native_directory_picker。cd packages/cli && npx vitest run src/serve/native-directory-picker.test.ts src/serve/process-env-guard.test.ts src/serve/server.test.ts全绿；capabilities 快照测试镜像主机探测结果，GUI 与 headless 机器都能通过。前后对比证据
关联 issue 中是 headless 部署上按钮仍显示的样子（Before）。After：headless 主机上对话框按构造不再渲染按钮（仅当 daemon 广播该 capability 时才接
onPick）；GUI 主机行为不变。通过 capabilities 响应与单测验证；headless 评审主机无法截取选择器本身的截图。测试平台
macOS 未测、Windows 未测、Linux 已测（headless 路径）。
环境
headless Linux 容器，
npm run dev/ vitest 直接跑源码；cli + web-shell 全量单测套件。风险与范围
zenity（或显示环境损坏）的 Linux 桌面现在会隐藏按钮——这是正确的，因为该平台的 picker 实现本就只支持zenity。macOS/Windows 无条件广播；macOS 在无 GUI 的 SSH 边缘场景下点击仍会失败，与本 PR 之前行为一致。关联 Issue
Closes #9404