chore(serve): remove the /demo debug page - #8805
Conversation
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not explored to full depth (tool budget reached): This PR removes the daemon's /demo debug page, folds the ...: did not execute rate-limit.test.ts , routes/health.test.ts , server.test.ts — this review worktree has no node_modules installed ( vitest unresolvable); …. Test Plan (not a blocker): 4221 passed — this review observed 18659 passed.
中文说明
已审查。 建议见行内评论。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未探索到全部深度（达到工具调用预算）：This PR removes the daemon's /demo debug page, folds the ...：did not execute rate-limit.test.ts , routes/health.test.ts , server.test.ts — this review worktree has no node_modules installed ( vitest unresolvable); …。 Test Plan（非阻断）：4221 passed — this review observed 18659 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // Exempt: OPTIONS, health, heartbeat, SSE events, ACP transport | ||
| if (method === 'OPTIONS') return null; | ||
| if ( | ||
| (method === 'GET' || method === 'HEAD') && | ||
| (p === '/health' || p === '/demo') | ||
| ) | ||
| return null; | ||
| if ((method === 'GET' || method === 'HEAD') && p === '/health') return null; |
There was a problem hiding this comment.
[Suggestion] R1-3: The removal of /demo from the rate-limit exemption list ships with no test gating it — the test-efficacy probe (harnessValidated: true) showed reverting this hunk (restoring || p === '/demo') leaves every test in rate-limit.test.ts green. The old 'exempts GET /demo' test was deleted with the route, and nothing pins the new exemption shape. — Failure scenario: a future change re-adds a pre-auth route to the exemption condition or widens the predicate → the suite stays green and an unauthenticated endpoint escapes rate limiting with no signal.
Suggested fix: add one assertion pinning the new behavior — e.g. in rate-limit.test.ts, drive limiter.middleware repeatedly against a non-exempt GET path and expect a 429 instead of next().
中文说明
R1-3：从限流豁免名单中移除 /demo 的这一改动没有任何测试把关——测试有效性探测（harnessValidated: true）显示，还原这个 hunk（恢复 || p === '/demo'）后 rate-limit.test.ts 的所有测试仍然全绿。旧的 'exempts GET /demo' 测试随路由一起删除了，现在没有任何测试钉住新豁免名单的形状。— 失败场景：未来的改动把某个 pre-auth 路由重新加进豁免条件、或放宽谓词 → 测试套件依然全绿，未认证端点将在没有任何信号的情况下绕过限流。建议修复：新增一条断言钉住新行为——例如在 rate-limit.test.ts 中对一个非豁免的 GET 路径多次驱动 limiter.middleware，期望收到 429 而不是 next()。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Fixed in bf482c3. rate-limit.test.ts now has exempts no GET path other than /health, which drives a fresh limiter twice against /demo, /health/deep, /healthz, and / and expects the second call to be refused with 429 in the read tier. Harness-checked the way you checked it: restoring || p === '/demo' in the predicate turns the new test red, so the exemption shape is gated now instead of free-floating.
中文说明
已在 bf482c3 修复。rate-limit.test.ts 新增 exempts no GET path other than /health：对 /demo、/health/deep、/healthz、/ 各用一个新建 limiter 连打两次,断言第二次以 read tier 被 429 拒绝。按你的方式做了变异验证:把 || p === '/demo' 恢复回谓词,新测试转红,豁免名单的形状现在有测试把关。
| 'token gates API routes; /health remains pre-auth ' + | ||
| 'on loopback unless --require-auth is set)' |
There was a problem hiding this comment.
[Suggestion] R1-4: The --allow-origin '*' boot-warning rewording ("/health and /demo remain pre-auth" → "/health remains pre-auth") is ungated — the test-efficacy probe showed reverting this hunk leaves every affected test green, and no test in the tree asserts the warning text. — Failure scenario: this warning is the operator's only notice of which routes are reachable pre-auth when booting with a wildcard origin; if the pre-auth surface changes again → the message silently misstates it and no test catches the drift.
Suggested fix: if the boot path gains a test for the --allow-origin '*' warning, assert it names exactly the current pre-auth routes; otherwise accept the gap knowingly.
中文说明
R1-4：--allow-origin '*' 启动告警的这次改写（"/health and /demo remain pre-auth" → "/health remains pre-auth"）没有任何测试把关——测试有效性探测显示还原这个 hunk 后所有相关测试仍然全绿，且整个仓库没有任何测试断言该告警文案。— 失败场景：这条告警是运维人员了解通配 origin 启动时哪些路由可未认证访问的唯一提示；如果 pre-auth 面再次变化 → 该消息会悄无声息地描述错误，且没有测试能发现这种漂移。建议修复：如果启动路径将来有了针对 --allow-origin '*' 告警的测试，请断言它恰好列出当前的 pre-auth 路由；否则请明知存在此缺口而接受它。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Took the first branch rather than accepting the gap. runQwenServe startup observability now has names every pre-auth surface in the --allow-origin '*' warning, which boots with allowOrigins: ['*'] plus a token, picks the --allow-origin: stderr line, and asserts it names the Web Shell static assets, --no-web, /health, and --require-auth, and does not name /demo. Reverting the wording turns it red.
The wording itself also changed — see R1-6: the old sentence enumerated the wrong surface, not just a stale one.
中文说明
选了第一条路,没有"明知有缺口而接受"。runQwenServe startup observability 新增 names every pre-auth surface in the --allow-origin '*' warning:以 allowOrigins: ['*'] 加 token 启动,取出 --allow-origin: 那行 stderr,断言它点名 Web Shell 静态资源、--no-web、/health、--require-auth,且不含 /demo。还原文案会让它转红。
文案本身也改了——见 R1-6:旧句子枚举的暴露面本身就是错的,不只是过时。
| **`--allow-origin <pattern>` (T2.4 [#4514](https://github.com/QwenLM/qwen-code/issues/4514)).** Browser webuis hitting the daemon cross-origin are blocked by default — any request carrying an `Origin` header returns `403 {"error":"Request denied by CORS policy"}` because CLI/SDK clients never send `Origin` and the daemon treats its presence as a sign the request came from a browser context the operator has not opted into. Pass `--allow-origin <pattern>` (repeatable) at boot to install an allowlist instead of the wall. Each pattern is either: | ||
|
|
||
| - The literal `*` — admit any origin. **Risky**: boot refuses when `*` is configured but no bearer token is set (any source: `--token`, `QWEN_SERVER_TOKEN`, or `--require-auth` which mandates a token at boot). The boot breadcrumb emits a stderr warning when `*` is in the list. **Recommendation**: pair with `--require-auth` on loopback binds so `/health` and `/demo` are also gated by the bearer — they're registered before the bearer middleware on loopback by default (so k8s/Compose probes can reach `/health` without a token), and a `*` allowlist makes them reachable from any cross-origin browser. On non-loopback binds the bearer is already mandatory at boot, so the `*` exposure surface is just `/health` (status JSON) and `/demo` (a static page whose JS still calls token-gated routes) — the actual API surface is gated regardless. | ||
| - The literal `*` — admit any origin. **Risky**: boot refuses when `*` is configured but no bearer token is set (any source: `--token`, `QWEN_SERVER_TOKEN`, or `--require-auth` which mandates a token at boot). The boot breadcrumb emits a stderr warning when `*` is in the list. **Recommendation**: pair with `--require-auth` on loopback binds so `/health` is also gated by the bearer — it's registered before the bearer middleware on loopback by default (so k8s/Compose probes can reach it without a token), and a `*` allowlist makes it reachable from any cross-origin browser. On non-loopback binds the bearer is already mandatory at boot, so the `*` exposure surface is just `/health` (status JSON) and the Web Shell static assets (whose JS still calls token-gated routes) — the actual API surface is gated regardless. |
There was a problem hiding this comment.
[Suggestion] R1-5: The PR-rewritten --allow-origin * bullet still lists /health in the tokenless * exposure surface on non-loopback binds, but there /health is registered after bearerAuth (exposeHealthPreAuth = loopback && !opts.requireAuth) and returns 401 without the token. A supertest probe run on both the PR and base trees observed 401 in both — the sentence was already wrong pre-PR, but this PR rewrote this very sentence and shipped the still-wrong /health enumeration on a changed line. — Failure scenario: an operator hardening a LAN-exposed daemon reads this edited paragraph → concludes /health stays reachable tokenlessly under * → adds compensating firewall rules or rejects a deploy over a leak that does not exist.
| - The literal `*` — admit any origin. **Risky**: boot refuses when `*` is configured but no bearer token is set (any source: `--token`, `QWEN_SERVER_TOKEN`, or `--require-auth` which mandates a token at boot). The boot breadcrumb emits a stderr warning when `*` is in the list. **Recommendation**: pair with `--require-auth` on loopback binds so `/health` is also gated by the bearer — it's registered before the bearer middleware on loopback by default (so k8s/Compose probes can reach it without a token), and a `*` allowlist makes it reachable from any cross-origin browser. On non-loopback binds the bearer is already mandatory at boot, so the `*` exposure surface is just `/health` (status JSON) and the Web Shell static assets (whose JS still calls token-gated routes) — the actual API surface is gated regardless. | |
| - The literal `*` — admit any origin. **Risky**: boot refuses when `*` is configured but no bearer token is set (any source: `--token`, `QWEN_SERVER_TOKEN`, or `--require-auth` which mandates a token at boot). The boot breadcrumb emits a stderr warning when `*` is in the list. **Recommendation**: pair with `--require-auth` on loopback binds so `/health` is also gated by the bearer — it's registered before the bearer middleware on loopback by default (so k8s/Compose probes can reach it without a token), and a `*` allowlist makes it reachable from any cross-origin browser. On non-loopback binds the bearer is already mandatory at boot and `/health` is registered behind it, so the only pre-auth surface `*` exposes is the Web Shell static assets (whose JS still calls token-gated routes) — the actual API surface is gated regardless. |
中文说明
R1-5：本 PR 改写的 --allow-origin * 条目仍然把 /health 列入非 loopback 绑定下 * 的免认证暴露面，但在那里 /health 是在 bearerAuth 之后注册的（exposeHealthPreAuth = loopback && !opts.requireAuth），不带 token 时返回 401。在 PR 树和基线树上分别运行的 supertest 探测均观察到 401——这句话在 PR 之前就是错的，但本 PR 改写了这个句子，在一行被修改的文字里保留了仍然错误的 /health 枚举。— 失败场景：正在加固 LAN 暴露 daemon 的运维读到这段被编辑的文字 → 以为在 * 下 /health 仍可免 token 访问 → 添加多余的防火墙规则，或因为一个并不存在的泄露而拒绝部署。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Confirmed and fixed in bf482c3. A probe across all three launch modes (loopback+token, loopback+--require-auth, non-loopback+token) reproduces exactly what you observed — /health is 401 on a non-loopback bind, since exposeHealthPreAuth = loopback && !opts.requireAuth registers it behind bearerAuth there.
I went slightly further than the suggestion: the same probe showed the Web Shell static assets answer 200 without a token in all three modes, so the bullet now says the assets are the only tokenless surface on a non-loopback bind and names --no-web as the way to remove it. Same correction applied to the user doc (R1-8) and to the boot warning (R1-6), so the three now agree.
中文说明
已确认并在 bf482c3 修复。对三种启动模式(loopback+token、loopback+--require-auth、非 loopback+token)的探测复现了你观察到的结果——非 loopback 绑定下 /health 返回 401,因为 exposeHealthPreAuth = loopback && !opts.requireAuth 会把它注册在 bearerAuth 之后。
比建议多改了一点:同一次探测显示 Web Shell 静态资源在三种模式下都免 token 返回 200,所以该条目现在写明:非 loopback 绑定下静态资源是唯一的免认证面,并点名 --no-web 是移除它的方式。同样的更正也应用到了用户文档(R1-8)和启动告警(R1-6),三处现在一致。
| ? ' (WARNING: `*` admits any cross-origin browser — bearer ' + | ||
| 'token gates API routes; /health and /demo remain pre-auth ' + | ||
| 'token gates API routes; /health remains pre-auth ' + | ||
| 'on loopback unless --require-auth is set)' |
There was a problem hiding this comment.
[Suggestion] R1-6: The --allow-origin '*' boot warning this PR rewrote omits the Web Shell static assets from the pre-auth surface it enumerates — on non-loopback binds they are the only tokenless surface (mountWebShellAssets is unconditional before bearerAuth; serves /, /assets/*, /session/:id navigations). The omission pre-dates the PR, but the rewrite is what stripped the enumeration of its last browser-surface reference (/demo), on a changed line. — Failure scenario: an operator starts qwen serve --allow-origin '*' on a LAN bind (where the /health clause does not even apply) → the warning reads as if nothing but token-gated API routes is reachable, while the full Web Shell UI is delivered tokenless to any admitted cross-origin browser; after this PR the warning and the rewritten protocol doc disagree about that surface.
| ? ' (WARNING: `*` admits any cross-origin browser — bearer ' + | |
| 'token gates API routes; /health and /demo remain pre-auth ' + | |
| 'token gates API routes; /health remains pre-auth ' + | |
| 'on loopback unless --require-auth is set)' | |
| ? ' (WARNING: `*` admits any cross-origin browser — bearer ' + | |
| 'token gates API routes; the Web Shell static assets are ' + | |
| 'always served pre-auth, and /health remains pre-auth ' | |
| 'on loopback unless --require-auth is set)' |
中文说明
R1-6：本 PR 改写的 --allow-origin '*' 启动告警遗漏了 Web Shell 静态资源这一 pre-auth 面——在非 loopback 绑定下它们是唯一的免认证面（mountWebShellAssets 无条件地位于 bearerAuth 之前，提供 /、/assets/*、/session/:id 导航）。该遗漏在本 PR 之前就存在，但这次改写把枚举中最后一个浏览器面引用（/demo）删掉了，且发生在一行被修改的文字上。— 失败场景：运维在 LAN 绑定上启动 qwen serve --allow-origin '*'（此时告警里 /health 那半句根本不适用）→ 告警读起来好像除 token 把关的 API 路由外别无可达面，而实际上完整的 Web Shell UI 正免 token 地送达任何被允许的跨源浏览器；本 PR 之后，该告警与改写后的协议文档对该暴露面的描述互相矛盾。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Fixed in bf482c3 — the warning now reads:
*admits any cross-origin browser — bearer token gates API routes; the Web Shell static assets stay pre-auth in every mode unless --no-web, and /health stays pre-auth on loopback unless --require-auth is set
(the suggested snippet was missing a + between the last two string literals, so I wrote it out rather than applying it). A probe confirmed the premise: /, /assets/*, and /session/:id navigations all answer 200 without a token on loopback, loopback + --require-auth, and a non-loopback bind alike. The wording is now asserted by a test — see R1-4.
中文说明
已在 bf482c3 修复,告警现在是:
*admits any cross-origin browser — bearer token gates API routes; the Web Shell static assets stay pre-auth in every mode unless --no-web, and /health stays pre-auth on loopback unless --require-auth is set
(建议的代码片段最后两段字符串之间缺一个 +,所以我重写而不是直接套用)。探测确认了前提:/、/assets/*、/session/:id 导航在 loopback、loopback + --require-auth、非 loopback 三种模式下都免 token 返回 200。该文案现在有测试断言——见 R1-4。
| - **`LOOPBACK_BINDS` includes IPv6** — `::1` and `[::1]` count as loopback for the no-token rule. | ||
| - **Host header allowlist** — on **loopback** binds the daemon checks `Host:` matches `localhost:port` / `127.0.0.1:port` / `[::1]:port` / `host.docker.internal:port` (case-insensitive per RFC 7230 §5.4) to defend against DNS rebinding. **Non-loopback binds (`--hostname 0.0.0.0`) intentionally bypass the Host allowlist** — the operator has chosen the surface area, so the bearer-token gate is the sole authentication layer; reverse proxies / SNI / client cert pinning are the operator's responsibility, not the daemon's. If you need Host-based isolation on a non-loopback bind, terminate TLS + check Host at a front proxy. | ||
| - **CORS denies any browser Origin by default** — returns `403` JSON. Pass **`--allow-origin <pattern>`** (repeatable, T2.4 #4514) to opt specific browser origins through. Each value is either the literal `*` (any origin — boot refuses if no bearer token is configured; `--require-auth` on loopback is recommended for full hardening since `/health` and `/demo` remain pre-auth on loopback by default) or a canonical URL origin (`<scheme>://<host>[:<port>]`, no trailing slash / path / userinfo). Matched origins receive proper CORS response headers (`Access-Control-Allow-Origin: <echoed>`, `Vary: Origin`, plus standard methods / headers / max-age and exposed `Retry-After`); unmatched origins still get a 403 with the same envelope as the default wall. `caps.features.allow_origin` is advertised conditionally so SDK / webui clients can pre-flight whether the daemon honors cross-origin hits before issuing them. Example: `qwen serve --allow-origin http://localhost:3000 --allow-origin http://localhost:5173`. Loopback self-origin hits (e.g. the `/demo` page) are unaffected — a separate Origin-strip shim handles them regardless of `--allow-origin`. **Browser webuis without `--allow-origin` configured** still fall back to the same Stage 1 options as before: package as a native shell (Electron/Tauri) so no `Origin` header is sent, or front the daemon with a same-origin reverse proxy. | ||
| - **CORS denies any browser Origin by default** — returns `403` JSON. Pass **`--allow-origin <pattern>`** (repeatable, T2.4 #4514) to opt specific browser origins through. Each value is either the literal `*` (any origin — boot refuses if no bearer token is configured; `--require-auth` on loopback is recommended for full hardening since `/health` remains pre-auth on loopback by default) or a canonical URL origin (`<scheme>://<host>[:<port>]`, no trailing slash / path / userinfo). Matched origins receive proper CORS response headers (`Access-Control-Allow-Origin: <echoed>`, `Vary: Origin`, plus standard methods / headers / max-age and exposed `Retry-After`); unmatched origins still get a 403 with the same envelope as the default wall. `caps.features.allow_origin` is advertised conditionally so SDK / webui clients can pre-flight whether the daemon honors cross-origin hits before issuing them. Example: `qwen serve --allow-origin http://localhost:3000 --allow-origin http://localhost:5173`. Loopback self-origin hits (e.g. the Web Shell UI) are unaffected — a separate Origin-strip shim handles them regardless of `--allow-origin`. **Browser webuis without `--allow-origin` configured** still fall back to the same Stage 1 options as before: package as a native shell (Electron/Tauri) so no `Origin` header is sent, or front the daemon with a same-origin reverse proxy. |
There was a problem hiding this comment.
[Suggestion] R1-8: The PR-rewritten --allow-origin bullet enumerates only /health as remaining pre-auth on loopback by default; the Web Shell static assets (/, /assets/*, /session/:id navigations) are also pre-auth, and stay pre-auth even under --require-auth (the mount is unconditional before bearerAuth). The rewrite removed /demo — the enumeration's browser surface — and named no replacement, the same defect class as R1-6. — Failure scenario: an operator configuring --allow-origin '*' on loopback weighs --require-auth against the stated residual → declines it believing /health is the sole pre-auth leftover (while the full Web Shell UI is delivered tokenless to any local caller), or adds it for the promised "full hardening" while the static shell stays tokenless — the hardening decision is made against a misstated surface.
| - **CORS denies any browser Origin by default** — returns `403` JSON. Pass **`--allow-origin <pattern>`** (repeatable, T2.4 #4514) to opt specific browser origins through. Each value is either the literal `*` (any origin — boot refuses if no bearer token is configured; `--require-auth` on loopback is recommended for full hardening since `/health` remains pre-auth on loopback by default) or a canonical URL origin (`<scheme>://<host>[:<port>]`, no trailing slash / path / userinfo). Matched origins receive proper CORS response headers (`Access-Control-Allow-Origin: <echoed>`, `Vary: Origin`, plus standard methods / headers / max-age and exposed `Retry-After`); unmatched origins still get a 403 with the same envelope as the default wall. `caps.features.allow_origin` is advertised conditionally so SDK / webui clients can pre-flight whether the daemon honors cross-origin hits before issuing them. Example: `qwen serve --allow-origin http://localhost:3000 --allow-origin http://localhost:5173`. Loopback self-origin hits (e.g. the Web Shell UI) are unaffected — a separate Origin-strip shim handles them regardless of `--allow-origin`. **Browser webuis without `--allow-origin` configured** still fall back to the same Stage 1 options as before: package as a native shell (Electron/Tauri) so no `Origin` header is sent, or front the daemon with a same-origin reverse proxy. | |
| - **CORS denies any browser Origin by default** — returns `403` JSON. Pass **`--allow-origin <pattern>`** (repeatable, T2.4 #4514) to opt specific browser origins through. Each value is either the literal `*` (any origin — boot refuses if no bearer token is configured; `--require-auth` on loopback is recommended for full hardening since `/health` remains pre-auth on loopback by default, and the Web Shell static assets are always served pre-auth — use `--no-web` to remove them) or a canonical URL origin (`<scheme>://<host>[:<port>]`, no trailing slash / path / userinfo). Matched origins receive proper CORS response headers (`Access-Control-Allow-Origin: <echoed>`, `Vary: Origin`, plus standard methods / headers / max-age and exposed `Retry-After`); unmatched origins still get a 403 with the same envelope as the default wall. `caps.features.allow_origin` is advertised conditionally so SDK / webui clients can pre-flight whether the daemon honors cross-origin hits before issuing them. Example: `qwen serve --allow-origin http://localhost:3000 --allow-origin http://localhost:5173`. Loopback self-origin hits (e.g. the Web Shell UI) are unaffected — a separate Origin-strip shim handles them regardless of `--allow-origin`. **Browser webuis without `--allow-origin` configured** still fall back to the same Stage 1 options as before: package as a native shell (Electron/Tauri) so no `Origin` header is sent, or front the daemon with a same-origin reverse proxy. |
中文说明
R1-8：本 PR 改写的 --allow-origin 条目只把 /health 列为 loopback 上默认保持 pre-auth 的面；Web Shell 静态资源（/、/assets/*、/session/:id 导航）同样 pre-auth，且即使在 --require-auth 下也保持 pre-auth（挂载无条件位于 bearerAuth 之前）。这次改写删掉了 /demo——枚举中的浏览器面——却没有指明替代者，与 R1-6 属同一缺陷类别。— 失败场景：在 loopback 上配置 --allow-origin '*' 的运维基于所陈述的残留面权衡 --require-auth → 以为 /health 是唯一的 pre-auth 残留而放弃它（而实际上完整的 Web Shell UI 正免 token 送达任何本地调用方），或为了承诺的"完全加固"加上它、但静态外壳仍然免 token——加固决策是基于一个被错误描述的暴露面做出的。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Fixed in bf482c3, with one deviation from the suggested text. The residual is not "always served pre-auth" in the abstract — the probe shows it is pre-auth on loopback, on loopback with --require-auth, and on a non-loopback bind, because the mount is unconditional before bearerAuth in every mode. So the bullet now states that explicitly (/, /assets/*, /session/:id document navigations), notes it survives --require-auth, and points at --no-web for operators who need that surface gone. The flags-table row above it got the same treatment.
中文说明
已在 bf482c3 修复,与建议文案有一处出入。残留面并不是抽象意义上的"总是 pre-auth"——探测显示它在 loopback、loopback + --require-auth、非 loopback 三种情况下都 pre-auth,因为挂载在每种模式下都无条件位于 bearerAuth 之前。所以该条目现在明确写出这一点(/、/assets/*、/session/:id 文档导航),说明它在 --require-auth 下依然存在,并为需要移除该面的运维指向 --no-web。上方 flags 表格里的对应行也做了同样处理。
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
Independent local validation reportVerdict: Functionally positive in the tested scope. I found no runtime blocker in the Scope and environment
Build and automated checks
Real daemon validationBaseline daemon (built from the merge base,
Bundled PR daemon with the real built Web Shell:
Auth behavior also matched the intended route placement:
Rate limiting was proven active with Mergeability and limitations
RecommendationThe observed behavior matches the PR description: the legacy 中文验证报告独立本地验证结论结论： 在本次覆盖范围内，功能验证通过，未发现删除 范围与环境
构建与自动化检查
真实 daemon 验证对照基线 daemon（从 merge base 构建，使用
使用真实 Web Shell 构建产物启动 PR bundle：
认证行为也符合预期：
限流使用 可合并性与限制
合并建议实测行为与 PR 描述一致：旧 |
jifeng
left a comment
jifeng
left a comment
There was a problem hiding this comment.
Independent local validation found one focused test gap; details are attached inline.
| }); | ||
| }); | ||
|
|
||
| describe('GET /demo', () => { |
There was a problem hiding this comment.
[Suggestion] Pin the removed /demo contract with replacement tests. This deletes every /demo assertion, while the remaining Web Shell tests only exercise a generic /deep/link. Reintroducing the old handler would therefore leave those tests green even though plain /demo requests no longer return 404. In the real bundled-daemon check, GET /demo with Accept: application/json returned 404, while a document navigation with Accept: text/html returned the Web Shell. Please retain that distinction in this suite with a focused pair of assertions (and, ideally, the --require-auth case, where an unauthenticated document navigation must remain 401).
中文说明
[建议] 请用替代测试固定 /demo 已删除后的行为契约。 此处删除了全部 /demo 断言，而现有 Web Shell 测试只覆盖通用的 /deep/link。因此即使未来误把旧 handler 加回，现有测试仍可能全绿，但普通 /demo 请求已不再是预期的 404。真实 bundle 验证中，带 Accept: application/json 的 GET /demo 返回 404，而带 Accept: text/html 的文档导航返回 Web Shell。建议在该套件中保留一对针对 /demo 的明确断言；最好同时覆盖 --require-auth，确保未认证的文档导航仍返回 401。
There was a problem hiding this comment.
Done in bf482c3 — two focused tests in the Web Shell suite, both covering the distinction you measured on the real bundle:
no longer serves a demo page: /demo is an ordinary unknown path—Accept: application/jsonmust 404 and must not contain the shell root;Accept: text/htmlmust be 200 with the shell.gates a /demo navigation behind the bearer once a token is configured— withtokenset and again withtoken+--require-auth, an unauthenticated document navigation is 401, not the shell. Your instinct was right: the SPA fallback is mounted afterbearerAuth, so unlike/and/session/:ida leftover/demobookmark is refused rather than answered.
Harness-checked: re-adding a /demo handler to the health route turns both tests red.
That last point also corrects something in my PR description — I wrote that the old URL "degrades to the Web Shell instead of erroring", which only holds for a tokenless loopback daemon. With a token configured it is a 401. I will fix the description.
中文说明
已在 bf482c3 完成——在 Web Shell 套件里加了两个针对性测试,都覆盖你在真实 bundle 上量到的区分:
no longer serves a demo page: /demo is an ordinary unknown path——Accept: application/json必须 404 且不含 shell 根节点;Accept: text/html必须 200 且是 shell。gates a /demo navigation behind the bearer once a token is configured——设了token、以及token+--require-auth两种情况下,未认证的文档导航都是 401 而非 shell。你的判断是对的:SPA 兜底挂在bearerAuth之后,所以与/和/session/:id不同,残留的/demo书签会被拒绝而不是被响应。
变异验证:把 /demo handler 加回 health 路由,两个测试都转红。
最后这点也纠正了我 PR 描述里的一句话——我写的"旧 URL 会降级到 Web Shell 而不是报错"只在无 token 的 loopback daemon 上成立;配置了 token 就是 401。我会去修正描述。
The daemon has shipped a real browser UI for a while: `resolveWebShellDir()` finds the bundled Web Shell assets and `mountWebShellAssets()` serves them at `/`, so `qwen serve` already opens onto a full client. `/demo` stayed behind as a 663-line inline-HTML console covering the same ground with none of the reach — nobody drives the daemon through it, and `npm run dev:daemon` starts the Web Shell dev server rather than the demo page. Keeping it around costs more than the dead code. It is the only file in the tree that pairs an event log with daemon HTTP, so work that starts as a Web Shell observation lands there instead: #8762 was found while running `/review` through the Web Shell and was fixed entirely inside the demo page's rendering, with "no Web Shell changes" in its own risk note. Deleting the page removes that decoy. Nothing is lost for protocol-level debugging: `GET /session/:id/events` streams the same raw frames the Events tab printed. `/health` shared `routes/health-demo.ts` with the demo handler, so the module is now `routes/health.ts` / `createHealthRoutes()` and drops its `getPort` dependency. The rate-limit exemption, the boot breadcrumb, and the daemon docs lose their `/demo` arms; the loopback self-origin shim regression test already asserted through `/health` and only needed its title corrected.
Review follow-up. Three of the removal hunks shipped ungated, and two doc sentences the removal rewrote were describing the pre-auth surface wrong — both before and after the edit. Deleting the `/demo` route took its assertions with it, so nothing failed if the handler came back: the Web Shell suite only exercised a generic deep link, and the rate-limit exemption could be widened again with the suite still green. `/demo` is now pinned as what it became — an ordinary unknown path: a non-navigation request 404s, a browser navigation is answered by the SPA fallback like any other deep link, and once a token is configured (with or without `--require-auth`) that navigation is refused with 401, because the fallback sits behind the bearer. The rate-limit test pins that `/health` is the only exempt GET, so re-adding a second pre-auth page to the predicate fails instead of silently escaping the limiter. Each new assertion was checked by reverting the hunk it guards and confirming it goes red. The `--allow-origin '*'` warning and both `--allow-origin` doc paragraphs enumerated `/health` as the residual tokenless surface and said nothing about the Web Shell static assets, which are mounted before the bearer in every launch mode and stay reachable even under `--require-auth` — the enumeration also claimed `/health` stays pre-auth on non-loopback binds, where it is registered behind the bearer and 401s. A probe across all three launch modes established the actual matrix; the warning and the docs now match it and name `--no-web` as the way to remove the residual browser surface. The warning text is asserted by a test for the first time.
07f4671 to
bf482c3
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
🔀 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 将重新运行。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 4 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not explored to full depth (tool budget reached): chunk 7: could not execute server.test.ts in this worktree — vite suite collection fails resolving @qwen-code/web-templates via src/ui/utils/export/formatters/html.ts (i….
中文说明
已审查。 建议见行内评论。 4 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未探索到全部深度（达到工具调用预算）：chunk 7：could not execute server.test.ts in this worktree — vite suite collection fails resolving @qwen-code/web-templates via src/ui/utils/export/formatters/html.ts (i…。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| ``` | ||
|
|
||
| Open `http://127.0.0.1:4170/demo` in a browser to see the debug console: chat UI, event stream, and workspace inspection. In the default loopback dev mode, `createServeApp()` mounts the `/demo` route from `packages/cli/src/serve/routes/health-demo.ts` **before** `bearerAuth`, so no token is required. | ||
| Open `http://127.0.0.1:4170/` in a browser to get the Web Shell UI: chat, session list, and workspace inspection. `createServeApp()` mounts the bundled Web Shell assets (`packages/cli/src/serve/web-shell-static.ts`) **before** `bearerAuth`, so the shell itself loads without a token; its own API calls carry the bearer when one is configured. `--no-web` opts out and leaves the daemon API-only. |
There was a problem hiding this comment.
[Suggestion] The rewritten §1 claims the shell's API calls "carry the bearer when one is configured", but the doc never explains how the token reaches the browser — the only usage instruction is Open http://127.0.0.1:4170/, which yields a 401-ing shell when auth is on.
Failure scenario: an operator follows the doc's intro ("Set QWEN_SERVER_TOKEN to enable") and §1's instruction, opens http://127.0.0.1:4170/ in a browser, and the shell renders but every API call returns 401 — with no in-UI way to supply the token and no doc hint that --open or a #token= fragment is required. The Web Shell has no in-UI token entry; the token reaches the browser only via qwen serve --open (which appends #token=), a manual #token=/?token= fragment, or an extension-parent postMessage.
| Open `http://127.0.0.1:4170/` in a browser to get the Web Shell UI: chat, session list, and workspace inspection. `createServeApp()` mounts the bundled Web Shell assets (`packages/cli/src/serve/web-shell-static.ts`) **before** `bearerAuth`, so the shell itself loads without a token; its own API calls carry the bearer when one is configured. `--no-web` opts out and leaves the daemon API-only. | |
| Open `http://127.0.0.1:4170/` in a browser to get the Web Shell UI: chat, session list, and workspace inspection. `createServeApp()` mounts the bundled Web Shell assets (`packages/cli/src/serve/web-shell-static.ts`) **before** `bearerAuth`, so the shell itself loads without a token; its own API calls carry the bearer when one is configured — start the daemon with `--open` (which puts the token in the URL fragment, never sent to the server) or append `#token=…` manually when auth is enabled. `--no-web` opts out and leaves the daemon API-only. |
中文说明
改写的 §1 声称 Web Shell 的 API 调用"配置了 token 时会自动携带 bearer"，但文档从未说明 token 如何到达浏览器——唯一的用法说明是 打开 http://127.0.0.1:4170/，开启认证后这会让外壳渲染成功但所有 API 调用返回 401。Web Shell 没有 UI 内输入 token 的入口；token 只能通过 qwen serve --open（把 token 放进 URL fragment）、手动在 URL 加 #token=/?token=、或扩展宿主 postMessage 传递。建议在 §1 说明开启认证后需用 --open 或手动附加 #token=… 打开外壳。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| | Loopback without `--require-auth` | `routes/health-demo.ts`, mounted by `createServeApp()` **before** `bearerAuth` | Works without token | | ||
| | Loopback with `--require-auth` | `routes/health-demo.ts`, mounted by `createServeApp()` **after** `bearerAuth` | Difficult to use from a plain browser; use curl or SDK | | ||
| | Non-loopback bind | `routes/health-demo.ts`, mounted by `createServeApp()` **after** `bearerAuth` | Same as above | | ||
| The static shell is mounted **before** `bearerAuth` in every launch mode — a browser cannot attach an `Authorization` header to an address-bar navigation or a `<script src>` subresource, so gating it would just break the UI. Every API route it calls stays token-gated, and the front end attaches the bearer itself. |
There was a problem hiding this comment.
[Suggestion] §8's "mounted before bearerAuth in every launch mode" assurance omits that on a non-loopback bind without --allow-origin the Web Shell is effectively read-only: same-origin POSTs carry an Origin header the CORS wall rejects with 403.
Failure scenario: an operator exposes the daemon on a non-loopback bind, opens the shell per §8, sees it render and reads "Every API route it calls stays token-gated, and the front end attaches the bearer itself" — then every mutation fails with 403 {"error":"Request denied by CORS policy"}, which looks like an auth bug and is not explained anywhere in the rewritten quickstart. The daemon's own boot diagnostic states the UI is "effectively read-only" in this mode (run-qwen-serve.ts), so the doc contradicts the runtime's own notice.
| The static shell is mounted **before** `bearerAuth` in every launch mode — a browser cannot attach an `Authorization` header to an address-bar navigation or a `<script src>` subresource, so gating it would just break the UI. Every API route it calls stays token-gated, and the front end attaches the bearer itself. | |
| The static shell is mounted **before** `bearerAuth` in every launch mode — a browser cannot attach an `Authorization` header to an address-bar navigation or a `<script src>` subresource, so gating it would just break the UI. Every API route it calls stays token-gated, and the front end attaches the bearer itself. On a non-loopback bind the shell is read-only unless `--allow-origin <origin>` is passed — same-origin POSTs carry an `Origin` header that the CORS wall rejects (403) — so pass `--allow-origin` for any bind beyond loopback. |
中文说明
§8 声称外壳"在每个启动模式下都挂在 bearerAuth 之前"，但没有说明在非 loopback 绑定且未配置 --allow-origin 时 Web Shell 实际是只读的：同源 POST 携带的 Origin 头会被 CORS 墙以 403 拒绝。daemon 自己的启动诊断就写明此模式下 UI"实际上是只读的"，文档与运行时的提示相矛盾。建议在 §8 补充非 loopback 绑定下需传 --allow-origin <origin> 才能写操作。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
|
🤖 Could not produce a passing fix for this feedback (round 1/100). This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. What I found before stopping: Address-review summary — PR #8805 (remove the daemon /demo debug page)No new code changes were needed this round. All six inline findings were already Feedback dispositionsAll six are Suggestion-level findings; each was verified against the exact code
Run log: https://github.com/QwenLM/qwen-code/actions/runs/31329158923 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Could not produce a passing fix for this feedback (round 2/100). This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31331437274 🧠 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.
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action taken (PR #8805)Feedback triageThe only new feedback since the last evaluation is a COMMENTED review from the
Conclusion
No code changes were made this round; the PR head is left as-is. 中文说明Autofix 审查轮次：未做任何操作（PR #8805）反馈分类自上次评估以来唯一的新反馈是自动审查机器人（
结论
本轮未做任何代码更改；PR 的 head 保持不变。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 120 passed · 0 failed · 120 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：120 通过 · 0 失败 · 120 总计 Verification reportPR #8805 deep verification —
|
| Cell | Scenario | BASE | HEAD | flip? |
|---|---|---|---|---|
| A1 | tokenless loopback, GET /demo (JSON Accept) |
200 demo HTML | 404 | ✅ load-bearing |
| A2 | tokenless loopback, GET /demo (browser nav) |
200 demo HTML + X-Frame-Options: DENY |
200 SPA shell (<div id="root">) |
✅ shape change |
| A3/A4 | GET /health, GET /health?deep=1 |
200 ok / counters |
200 ok / counters |
unchanged |
| A5/A6 | GET /, unknown path |
200 shell / 404 | 200 shell / 404 | unchanged |
| B1/B2 | loopback + token, GET /demo no auth (nav / json) |
200 demo (pre-auth) | 401 | ✅ load-bearing |
| B6 | loopback + token, GET /demo with bearer |
200 demo | 404 | ✅ |
| C1 | loopback + token + --require-auth, /demo nav no auth |
401 | 401 | unchanged (both gated) |
| C2/C3 | /health no-bearer / bearer under --require-auth |
401 / 200 | 401 / 200 | unchanged |
| C4 | GET / no token under --require-auth |
200 shell | 200 shell | unchanged (static pre-auth) |
| C5 | /demo nav with bearer under --require-auth |
200 demo | 200 SPA shell | ✅ |
| D1/D2 | non-loopback (0.0.0.0) + token, /health no/bearer |
401 / 200 | 401 / 200 | unchanged |
| D3 | non-loopback, GET / no token |
200 shell | 200 shell | unchanged (static pre-auth) |
| D4/D5 | non-loopback, /demo nav no-auth / bearer |
401 / 200 demo | 401 / 200 SPA shell | ✅ |
| E1 | --allow-origin '*' boot warning text |
enumerates /demo |
names Web Shell static assets, --no-web, /health, --require-auth; no /demo |
✅ |
| RL1/RL4 | /health ×5 (tokenless; and bearer under --require-auth after bucket exhausted) |
200×5 | 200×5 | unchanged (still exempt) |
| RL2/RL3 | /demo ×5, --rate-limit-read 2 |
200×5 (exempt) | 404,404,429,429,429 | ✅ exemption narrowed |
Key flips all reproduce: the demo handler is gone (A1/B1/B6), its loopback pre-auth
exposure is gone in every token mode (B1/C1/D4), and the rate-limit exemption list no
longer contains /demo (RL2) while /health stays exempt even with an exhausted read
bucket (RL4). The Web Shell static surface (/, /assets, /session/:id nav) remains
pre-auth in every mode on both arms (B4/C4/D3), exactly as the rewritten warning and docs
state.
Secondary claim — --no-web removes the residual browser surface: verified live on
head (03-no-web-probe.mjs, 4/4): GET / and a deep-link navigation both 404 (no shell,
no SPA fallback), /health still 200, boot log does not claim the UI is served.
Corrections
None. (No earlier review round or bot comment misdescribed the code in a way that needs
correcting.)
Findings
None blocking; no defects found. Two informational notes, neither a finding against
this PR:
- The description's local
tsc --noEmitnote says it reports pre-existing errors
(missingqrcode-terminaltypes, a stale session-service declaration). In this
containernpx tsc --noEmitinpackages/cliis clean (0 errors) on both arms, so
those errors did not reproduce here — likely an environment/dependency-state difference.
The load-bearing fact for this PR (no new type errors) holds: both arms are equally
clean. - The description cites
4221 passedfornpx vitest run src/serve; the merge ref I
verified carries 4242 passed / 1 skipped. File count (147) and skip count (1)
match; the +21 is consistent with the branch carrying a merge ofmain(which added
serve tests) — in any case the suite is fully green.
Not covered
- Per-commit attribution. Checkout is depth-2 (
git rev-parse --is-shallow-repository
= true); only the merge head is reachable, so the two logical commits
(4c755375removal,bf482c36test-pinning) could not be exercised individually. I
verified the aggregateHEAD^1..HEADdiff instead. The mutation matrix nonetheless
separates the two concerns (removal vs. the tests that pin it). - Repo-wide gates the PR's own CI already runs (full
npm run build, full test suite,
ESLint/Prettier, integration suites) were not re-run; I ran the affected workspace's
serve suite and typecheck instead. - Windows / Linux-specific paths — the author marked these untested locally; the
loopback/0.0.0.0 behavior exercised here is the POSIX path. - Release-bundling claim (Web Shell shipped next to the CLI bundle) — not exercised; I
used the in-checkoutpackages/web-shell/dist. ?deep=1content equality across arms — I asserted both return 200 + aggregate
counter keys (workspaceCount, etc.), not a byte-for-byte diff of the counters.
Methodology
Environment: Linux node v22.23.2 container, repo at the merge ref. The A/B harness
(01-daemon-matrix.mjs) spawns a genuine daemon per arm via node node_modules/tsx/dist/cli.mjs <arm>/packages/cli/index.ts serve … with an isolated $HOME and scratch workspace, waits
for the qwen serve listening on … line, then probes over real loopback / 0.0.0.0 HTTP
with fetch, asserting status codes, body markers (Qwen Serve demo string vs
<div id="root"> shell marker), and headers against per-arm expectation tables. The base
arm runs from a scratch git worktree of HEAD^1; only the PR diff differs between arms.
The mutation matrix (02-mutation-matrix.mjs) applies three single-purpose mutants in a
scratch HEAD worktree — restore the /demo route, widen the rate-limit exemption back to
'/health' || '/demo', revert the --allow-origin '*' warning text — and confirms each
turns its guarding test red while the unmutated controls stay green; it then restores the
tree (git status clean). Targeted gates: npx vitest run src/serve (head) and
npx tsc --noEmit (both arms). Raw per-arm stdout/stderr and build/typecheck logs are in
logs/; harness scripts are in this directory for rerun.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Gate check — passing (re-run on the current head).
Moving on to code review. 🔍 中文说明入口检查 —— 通过（针对当前 head 的 re-run）。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI proposed my own cut before reading the diff: delete What I verified beyond the diff:
No critical blockers, no convention violations. Files changed (22 of 22 shown)
Test evidence — this PR's own CI on the reviewed commit
Everything that ran on the reviewed commit landed green — there were no failures to triage. Two signals carry the behavioral claim specifically: the Serve A/B job built the PR base and this head, drove a fixed endpoint set against each, and diffed the responses — no unexpected deltas — and the real daemon E2E matrix exercises an actual 中文说明代码审查：我在看 diff 之前先给出了自己的方案（删除 demo.ts 及其测试、把共享路由模块收敛为纯 health、去掉 /demo 的限流豁免和警告中的 /demo 分支、改写文档、断言 /demo 已是未知路径）。本 PR 与我的方案一一对应，并且更进一步——替换测试把"删除"本身钉住了：若有人重新引入旧 handler，CI 会直接失败。 diff 之外我核实了：树中（packages、docs、integration-tests、scripts、.github）不再存在任何 health-demo、createHealthDemoRoutes、getDemoHtml 或 /demo 路由注册的残留，仅剩的匹配是钉住删除的负向断言、历史 CHANGELOG 条目（正确地未改动）和无关测试夹具；getPort 只是从 health 模块依赖中移除，server.ts 中同源 Origin 剥离和 host 白名单仍在正常使用它；新的限流测试逐一验证 /demo、/health/deep、/healthz、/ 都会被限流，排除了 /health 前缀误匹配；旧 URL 的降级行为（非导航 404、无 token 时导航走 SPA fallback、配置 token 后导航 401）与 server.ts 的路由注册顺序一致（静态挂载在 bearerAuth 之前、SPA fallback 在所有 API 路由之后）；被删 handler 的安全姿态（frame-ancestors 'none' + X-Frame-Options: DENY）在 Web Shell 挂载处本就是默认值，被删的 CORS 墙测试也在 未发现阻断问题或规范违规。 测试证据：审查提交上的 CI 全部结束且全绿，没有需要归因的失败。Serve A/B 用 PR base 与本 head 分别构建、以固定端点集对比响应，无意外差异；真实 daemon E2E 矩阵实际启动了 qwen serve 进程。被跳过的检查在 ci.yml 中仅限 merge_group 事件，属于所有 PR 的共同设计，会在合并队列中运行。上一个 head 的沙箱 /verify 以 120/120 断言通过（报告在本帖中）；之后的两个提交只是文档措辞和两个钉住测试，针对当前 head 的新一轮 verify 会在其自己的评论中汇报。A/B 对比加上钉住删除的测试已经证实了本 PR 唯一的行为主张（除 /demo 消失外一切不变），因此无需额外的沙箱验证触发。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — a reductive change with an observed motivation, a pinned removal, green A/B and daemon-E2E evidence, and nothing left unverified. Stepping back: this PR makes the codebase smaller and harder to misroute, which is the rare combination where the motivation, the diff, and the evidence all point the same way. The demo page demonstrably attracted work that belonged to the Web Shell (#8762 being the concrete case), and nothing on the shipped or development path used it. Against my independent proposal, the implementation matches one-to-one and then exceeds it — the replacement tests don't just delete the old coverage, they pin the new contract ( This re-run re-reviewed everything after the two follow-up commits and the base merges that landed since the last pass. They changed no behavior — doc wording scoped to loopback, two more pinning tests — and I confirmed the head tree still has zero dangling Approving, pinned to the reviewed commit. 中文说明总体评价：这是一个让代码库变得更小、也更不容易把工作引偏的 PR —— 动机、diff 和证据三者方向一致，这并不多见。demo 页面确实吸引了本应属于 Web Shell 的工作（#8762 是具体案例），而发布路径和开发路径都没有使用它。 与我的独立方案相比，实现一一对应并且更进一步——替换测试不只是删掉旧覆盖，而是钉住了新契约（/demo 作为未知路径返回 404、导航降级到 SPA fallback、配置 token 后该 fallback 受 bearer 门控），删除不会被悄悄回退。每一处改动都服务于既定目标；唯一的"额外"是两个文档中 Prettier 重排表格列宽，PR 正文已如实说明。 本次 re-run 在上次审查之后新增的两个修复提交和 base 合并之上重新审查了全部内容。它们没有改变行为——只是把文档措辞限定到 loopback、并新增两个钉住测试——我确认当前 head 树中依然没有任何 /demo 残留引用，getPort 的移除范围也依然正确。审查提交上的 CI 已全部结束且全绿：单元套件、Serve A/B、真实 daemon E2E 矩阵、Web Shell E2E 冒烟、Desktop Shell 构建。该 head 上没有任何仍在运行的 PR 工作流；上一个 head 上以 120/120 通过的沙箱验证也已有针对当前 head 的新一轮在运行中。 批准合并，批准锚定在本次审查的提交上。 — Qwen Code · qwen3.8-max Reviewed at |
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. ✅
|
LGTM, looks ready to ship. ✅ |
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31349170722 🧠 Handled by Qwen Code · model/模型 |
jifeng
left a comment
jifeng
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not explored to full depth (tool budget reached): This PR removes the daemon's /demo debug page, folds the ...: I did not deep-verify the Web Shell client's actual UI features ("workspace inspector", "permission UX") inside packages/web-shell ; I checked only intra-PR co….
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未探索到全部深度（达到工具调用预算）：This PR removes the daemon's /demo debug page, folds the ...：I did not deep-verify the Web Shell client's actual UI features ("workspace inspector", "permission UX") inside packages/web-shell ; I checked only intra-PR co…。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| ``` | ||
|
|
||
| Open `http://127.0.0.1:4170/demo` in a browser to see the debug console: chat UI, event stream, and workspace inspection. In the default loopback dev mode, `createServeApp()` mounts the `/demo` route from `packages/cli/src/serve/routes/health-demo.ts` **before** `bearerAuth`, so no token is required. | ||
| Open `http://127.0.0.1:4170/` in a browser to get the Web Shell UI: chat, session list, and workspace inspection. `createServeApp()` mounts the bundled Web Shell assets (`packages/cli/src/serve/web-shell-static.ts`) **before** `bearerAuth`, so the shell itself loads without a token; its own API calls carry the bearer when one is configured. `--no-web` opts out and leaves the daemon API-only. |
There was a problem hiding this comment.
[Suggestion] R1-1: The rewritten §1 claims the shell's API calls "carry the bearer when one is configured", but the doc never explains how the token reaches the browser — the only usage instruction is Open http://127.0.0.1:4170/, which yields a 401-ing shell when auth is on — Failure scenario: an operator follows §1 with auth configured and every API call returns 401; the Web Shell has no in-UI token entry, and the token reaches the browser only via qwen serve --open (which appends #token=) or a manual #token=/?token= fragment.
| Open `http://127.0.0.1:4170/` in a browser to get the Web Shell UI: chat, session list, and workspace inspection. `createServeApp()` mounts the bundled Web Shell assets (`packages/cli/src/serve/web-shell-static.ts`) **before** `bearerAuth`, so the shell itself loads without a token; its own API calls carry the bearer when one is configured. `--no-web` opts out and leaves the daemon API-only. | |
| Open `http://127.0.0.1:4170/` in a browser to get the Web Shell UI: chat, session list, and workspace inspection. `createServeApp()` mounts the bundled Web Shell assets (`packages/cli/src/serve/web-shell-static.ts`) **before** `bearerAuth`, so the shell itself loads without a token; its own API calls carry the bearer when one is configured — start the daemon with `--open` (which puts the token in the URL fragment, never sent to the server) or append `#token=…` manually when auth is enabled. `--no-web` opts out and leaves the daemon API-only. |
中文说明
改写的 §1 声称 Web Shell 的 API 调用"配置了 token 时会自动携带 bearer"，但文档从未说明 token 如何到达浏览器——唯一的用法说明是 打开 http://127.0.0.1:4170/，开启认证后这会让外壳渲染成功但所有 API 调用返回 401。Web Shell 没有 UI 内输入 token 的入口；token 只能通过 qwen serve --open（把 token 放进 URL fragment）、手动在 URL 加 #token=/?token=、或扩展宿主 postMessage 传递。建议在 §1 说明开启认证后需用 --open 或手动附加 #token=… 打开外壳。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| | Loopback without `--require-auth` | `routes/health-demo.ts`, mounted by `createServeApp()` **before** `bearerAuth` | Works without token | | ||
| | Loopback with `--require-auth` | `routes/health-demo.ts`, mounted by `createServeApp()` **after** `bearerAuth` | Difficult to use from a plain browser; use curl or SDK | | ||
| | Non-loopback bind | `routes/health-demo.ts`, mounted by `createServeApp()` **after** `bearerAuth` | Same as above | | ||
| The static shell is mounted **before** `bearerAuth` in every launch mode — a browser cannot attach an `Authorization` header to an address-bar navigation or a `<script src>` subresource, so gating it would just break the UI. Every API route it calls stays token-gated, and the front end attaches the bearer itself. |
There was a problem hiding this comment.
[Suggestion] R1-2: §8's "mounted before bearerAuth in every launch mode" assurance omits that on a non-loopback bind without --allow-origin the Web Shell is effectively read-only: same-origin POSTs carry an Origin header the CORS wall rejects with 403 — Failure scenario: an operator exposes the daemon on a non-loopback bind, opens the shell per §8, reads "Every API route it calls stays token-gated", then every mutation fails with 403 Request denied by CORS policy, which looks like an auth bug and contradicts the daemon's own boot diagnostic ("the Web Shell is read-only on a non-loopback bind").
| The static shell is mounted **before** `bearerAuth` in every launch mode — a browser cannot attach an `Authorization` header to an address-bar navigation or a `<script src>` subresource, so gating it would just break the UI. Every API route it calls stays token-gated, and the front end attaches the bearer itself. | |
| The static shell is mounted **before** `bearerAuth` in every launch mode — a browser cannot attach an `Authorization` header to an address-bar navigation or a `<script src>` subresource, so gating it would just break the UI. Every API route it calls stays token-gated, and the front end attaches the bearer itself. On a non-loopback bind the shell is read-only unless `--allow-origin <origin>` is passed — same-origin POSTs carry an `Origin` header that the CORS wall rejects (403) — so pass `--allow-origin` for any bind beyond loopback. |
中文说明
§8 声称外壳"在每个启动模式下都挂在 bearerAuth 之前"，但没有说明在非 loopback 绑定且未配置 --allow-origin 时 Web Shell 实际是只读的：同源 POST 携带的 Origin 头会被 CORS 墙以 403 拒绝。daemon 自己的启动诊断就写明此模式下 UI"实际上是只读的"。建议在 §8 补充非 loopback 绑定下需传 --allow-origin <origin> 才能进行写操作。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| | Routes | File | Mounting entry | | ||
| | -------------------------------------------------------------------------------------------- | ------------------------------------------------------- | ------------------------------------------------------------------------------ | | ||
| | `/health`, `/demo` | `packages/cli/src/serve/routes/health-demo.ts` | `healthDemoRoutes.register()` | | ||
| | `/health` | `packages/cli/src/serve/routes/health.ts` | `healthRoutes.register()` | |
There was a problem hiding this comment.
[Suggestion] R2-1: The section 9 call-chain diagram (line 256) still lists /demo as a mounted route — route mounting (health / demo / capabilities / …) — in the same document this PR rewrote to remove /demo, directly contradicting the corrected section 10 route table below — Failure scenario: a developer reading the call-chain diagram sees /demo as a mounted route pointing at demo.ts, which this PR deletes; the diagram contradicts the rest of the same document and misdirects readers to a page that no longer exists.
| | `/health` | `packages/cli/src/serve/routes/health.ts` | `healthRoutes.register()` | | |
| | |- route mounting (health / web-shell static / capabilities / workspace / session / SSE / ACP HTTP) |
中文说明
第 9 节的调用链图示（第 256 行）仍把 /demo 列为挂载路由——route mounting (health / demo / capabilities / …)——与本 PR 刚改掉 /demo 的同一份文档矛盾，也直接与下方已修正的第 10 节路由表冲突。建议把图示中的 /demo 移除，改为 health / web-shell static / capabilities / workspace / session / SSE / ACP HTTP。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| | `serve/daemon-logger.ts` | `DaemonLogger` structured file logs. See [`19-observability.md`](./19-observability.md). | | ||
| | `serve/debug-mode.ts` | Shared `isServeDebugMode()` predicate controlling verbose error context in HTTP responses. | | ||
| | `serve/acp-http/` | ACP Streamable HTTP transport (RFD #721), mounted at `/acp`. Seven files implement JSON-RPC POST, SSE GET, DELETE teardown, and shared bridge usage in parallel with the REST surface. | | ||
| | `serve/web-shell-static.ts`, `serve/web-shell-resolver.ts` | Locate and mount the built Web Shell assets (the daemon's browser UI) at `/`, `/assets`, and `/session/:id`, plus the SPA deep-link fallback registered after all API routes. Mounted **before** `bearerAuth` in every launch mode — a browser cannot attach `Authorization` to a navigation or subresource — while every API route it calls stays token-gated. Degrades to API-only when the assets are absent; `--no-web` opts out. | |
There was a problem hiding this comment.
[Suggestion] R2-5: This PR promotes --no-web to first-class daemon-docs guidance, but 02-serve-runtime.md's Configuration "Flags" table has no --no-web entry, and the page's own cross-reference "See 17-configuration.md for the merged reference" leads to a page that has zero --web/--no-web rows — despite 00-index.md billing that page as where "Full qwen serve flags … are collected in one page" — Failure scenario: a developer reads the new row and wants the flag's type/default/interactions; they follow the merged-reference link, and the reference page contains no --web/--no-web row, so the flag's semantics are discoverable only by grepping prose across three docs.
中文说明
本 PR 把 --no-web 提升为一等公民的操作指引，但 02-serve-runtime.md 的 Configuration "Flags" 表没有 --no-web 条目，而本页指向的"合并参考"17-configuration.md 中也没有任何 --web/--no-web 行——尽管 00-index.md 声称该页是"完整的 qwen serve 标志集合"。建议在两份文档的标志表中补上 --web / --no-web 行（boolean，默认 true，镜像 docs/users/qwen-serve.md 中已有的语义）。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| ## 8. Is there a browser UI? | ||
|
|
||
| **Yes.** It is implemented by `getDemoHtml(port)` in `packages/cli/src/serve/demo.ts` as self-contained HTML with no external dependency. | ||
| **Yes — the Web Shell.** `resolveWebShellDir()` finds the built assets (bundled next to the CLI bundle in a release, `packages/web-shell/dist` in a checkout) and `mountWebShellAssets()` serves them at `/`, `/assets`, and `/session/:id`. When the assets are missing the daemon degrades to API-only instead of crashing; `--no-web` opts out explicitly. |
There was a problem hiding this comment.
[Suggestion] R2-8: The rewritten §8 claims mountWebShellAssets() serves the shell at /session/:id with no qualifier, but the route answers only document navigations (isDocumentNavigation) — the sibling doc rewritten in this same PR (qwen-serve-protocol.md) correctly qualifies the identical claim as "/session/:id document navigations" — Failure scenario: §8 sits immediately below §7's curl checklist, so an operator verifying the claim runs curl http://127.0.0.1:4170/session/<sid> with curl's default Accept: */*; isDocumentNavigation() returns false and there is no GET /session/:id API route, so the request ends as a 401 or JSON 404 — never the shell HTML the sentence implies.
| **Yes — the Web Shell.** `resolveWebShellDir()` finds the built assets (bundled next to the CLI bundle in a release, `packages/web-shell/dist` in a checkout) and `mountWebShellAssets()` serves them at `/`, `/assets`, and `/session/:id`. When the assets are missing the daemon degrades to API-only instead of crashing; `--no-web` opts out explicitly. | |
| **Yes — the Web Shell.** `resolveWebShellDir()` finds the built assets (bundled next to the CLI bundle in a release, `packages/web-shell/dist` in a checkout) and `mountWebShellAssets()` serves them at `/`, `/assets`, and `/session/:id` document navigations (browser deep links — a plain `curl /session/<id>` gets the API's 401/404, not the shell). When the assets are missing the daemon degrades to API-only instead of crashing; `--no-web` opts out explicitly. |
中文说明
改写的 §8 声称 mountWebShellAssets() 在 /session/:id 提供外壳服务，但没有说明该路由只应答文档导航（isDocumentNavigation）；同一 PR 改写的 qwen-serve-protocol.md 已用"/session/:id document navigations"正确限定。由于 §8 紧跟在 §7 的 curl 清单之下，用 curl 的默认 Accept: */* 访问会因 isDocumentNavigation() 为 false 而落到 401/JSON 404，而非外壳 HTML。建议补充"document navigations"限定。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| describe('same-origin Origin-stripping middleware', () => { | ||
| it('strips loopback Origin header matching daemon port', async () => { |
There was a problem hiding this comment.
[Suggestion] R3-3: The deleted /demo CORS test (GET /demo + Origin: https://evil.example.com → 403) was the only assertion that a pre-auth page — not an API route — sits behind the denyBrowserOriginCors wall. Nothing in this PR's replacement tests re-pins that contract for the shell: every surviving Origin-header test targets API paths, and none hits the Web Shell page paths /, /session/:id, or /assets/* — Failure scenario: a future change moves mountWebShellAssets above denyBrowserOriginCors (the exact class of regression the new rate-limit pin guards for the limiter), and nothing fails because no Origin-bearing request ever reaches the pre-auth page paths; the daemon's only pre-auth HTML surface then answers cross-origin requests without the 403 the middleware is supposed to produce.
| describe('same-origin Origin-stripping middleware', () => { | |
| it('strips loopback Origin header matching daemon port', async () => { | |
| it('rejects cross-origin requests for the pre-auth shell page (CORS wall runs first)', async () => { | |
| const res = await app.get('/').set('Origin', 'https://evil.example.com'); | |
| expect(res.status).toBe(403); | |
| }); |
中文说明
被删除的 /demo CORS 测试（GET /demo + Origin: https://evil.example.com → 403）是唯一断言预认证页面（而非 API 路由）位于 denyBrowserOriginCors 墙之后的用例。本 PR 的替代测试没有为外壳重新固定这一契约：现有带 Origin 的测试都指向 API 路径，没有覆盖 Web Shell 页面路径 /、/session/:id、/assets/*。建议在 Web Shell 静态服务 describe 中补一个断言 GET / 带恶意 Origin 返回 403 的测试。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| 8. JSON body parser and JSON parser error mapper | ||
| 9. post-auth `/health` and `/demo` when required | ||
| 9. post-auth `/health` when required |
There was a problem hiding this comment.
[Suggestion] R5-1: Both design docs list "JSON body parser and JSON parser error mapper" (item 8) before "post-auth /health when required" (item 9), but createServeApp() registers post-auth /health (server.ts:1705) before installJsonBodyParser (server.ts:1708). This PR re-emits item 9 (dropping /demo) while its own server.ts hunk edits the exact code region — Failure scenario: a maintainer audits createServeApp() against the design doc's order list (the doc's stated purpose) and reasons about the middleware stack against a false order; the JSON-parser error mapper never wraps post-auth /health requests, contrary to what the list implies.
| 8. JSON body parser and JSON parser error mapper | |
| 9. post-auth `/health` and `/demo` when required | |
| 9. post-auth `/health` when required | |
| 8. post-auth `/health` when required | |
| 9. JSON body parser and JSON parser error mapper |
中文说明
两份设计文档把"JSON body parser and JSON parser error mapper"（第 8 项）列在"post-auth /health when required"（第 9 项）之前，但 createServeApp() 实际先注册 post-auth /health（server.ts:1705）再 installJsonBodyParser（server.ts:1708）。本 PR 重写了第 9 项（去掉 /demo）且其 server.ts hunk 恰好编辑该区域。建议交换第 8、9 项以匹配实际装配顺序。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action taken (PR #8805)Feedback triageEvery feedback item newer than the last evaluation was read and classified. None is actionable — all are approvals or informational bot passes:
Checks
DecisionNo code change this round. The PR head already carries the full removal of the 中文说明Autofix 评审轮次：未做改动（PR #8805）反馈分类已阅读并分类上次评估之后的每一条反馈，均无需处理——全部为批准或信息性的机器人通过记录：
检查
结论本轮不做任何代码改动。当前 PR head 已包含 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
… wall Review follow-up. The removal rewrote the daemon docs around the Web Shell, and three of the rewritten claims did not match what the runtime actually does: §1 never said how the bearer reaches the browser (with auth on, the plain URL loads a shell whose every API call 401s), §8 called the shell writable on any bind (on a non-loopback bind without `--allow-origin` its POSTs hit the CORS wall and 403), and §8 served `/session/:id` without the document-navigation qualifier its own code enforces. The §9 call-chain diagram also still listed the deleted `/demo` route, the developer flag references had no `--web`/`--no-web` row despite the new guidance pointing at the flag, and both design docs listed the JSON body parser ahead of post-auth `/health` while `createServeApp()` registers them the other way round. The deleted `/demo` CORS test was also the only assertion that a pre-auth page sits behind the Origin wall — every surviving Origin test targets an API path. Re-pin it for the shell root so a mount-order regression fails instead of exposing the pre-auth HTML surface cross-origin.
5551616
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Review response summary — PR #8805All seven findings are Suggestion-level from the automated reviewer. Each was verified against the current code before editing, and all seven are addressed in one commit ( Feedback points and dispositions
Changed files
Conflict notesNone — Verification
中文说明审查回应摘要 — PR #8805七条发现均来自自动审查器的 Suggestion 级别。每一条都在编辑前对照当前代码核实，全部七条已在一次提交中处理（ 反馈点与处理
变更文件
冲突说明无 — 验证
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.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| * Content-Security-Policy for the Web Shell HTML shell. | ||
| * | ||
| * Deliberately looser than the `/demo` page's `default-src 'none'`: the real | ||
| * Deliberately looser than a `default-src 'none'` static page: the real |
There was a problem hiding this comment.
[Suggestion] The demo→Web Shell rename sweep missed a sibling: packages/cli/src/serve/server/self-origin.ts:10 — the module header of installSelfOriginStripMiddleware still reads "Allow same-origin requests from the demo page.", a page this PR deletes, while the in-function comment in the same file already says "loopback web shell". Every other reference was updated (docs, the --allow-origin '*' boot warning, the comments in this file, the test rename to "loopback self-origin shim"). — Failure scenario: a maintainer later removing dead demo scaffolding greps for demo-page references, reads that header, concludes the Origin-strip middleware exists only for the deleted /demo page, and deletes or relocates it. Every same-origin Web Shell POST/fetch then carries an Origin header that denyBrowserOriginCors rejects with 403 — the Web Shell UI stops working in default loopback mode. The PR's own 12-auth-security.md warns: "If a future change moves the strip elsewhere, the Web Shell breaks." — Suggested fix: change the header to "Allow same-origin requests from the Web Shell."
中文说明
[建议] demo→Web Shell 的重命名清理遗漏了一处同类引用:packages/cli/src/serve/server/self-origin.ts:10 —— installSelfOriginStripMiddleware 的模块头注释仍写着 "Allow same-origin requests from the demo page.",而这个页面已被本 PR 删除,同文件内的函数内注释却已经写着 "loopback web shell"。其他所有引用都已更新(文档、--allow-origin '*' 启动告警、本文件中的注释、以及重命名为 "loopback self-origin shim" 的测试)。— 失败场景:之后有位维护者清理 demo 遗留代码,搜索 demo 相关引用时读到该头注释,以为这个 Origin 剥离中间件只为已删除的 /demo 页面而存在,于是删除或移动它。此后 Web Shell 的每个同源 POST/fetch 都会携带 Origin 头并被 denyBrowserOriginCors 以 403 拒绝 —— Web Shell UI 在默认 loopback 模式下将无法使用。本 PR 自己更新的 12-auth-security.md 也警告过:"If a future change moves the strip elsewhere, the Web Shell breaks."。— 建议修复:将该头注释改为 "Allow same-origin requests from the Web Shell."。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| | `--external-tool-guard-timeout-ms <n>` | integer `100..30000` | `3000` | Per-handshake and per-prepare deadline. A timeout fails startup during the handshake or fails the invocation closed during a turn. | | ||
| | `--allow-origin <pattern>` | repeatable string | unset | Cross-origin allowlist that replaces the default CORS denial. `*` allows any origin but requires a token. | | ||
| | `--allow-private-auth-base-url` | boolean | `false` | Allows `/workspace/auth/provider` to install localhost / private-network auth provider `baseUrl`; use only in trusted local development. | | ||
| | `--web` / `--no-web` | boolean | `true` | Serve the built Web Shell SPA at the daemon root (`GET /`, `/assets/*`, and `/session/:id` document navigations). These entry points are mounted before `bearerAuth`; every API route stays token-gated. `--no-web` leaves the daemon API-only. | |
There was a problem hiding this comment.
[Suggestion] This diff adds the --web/--no-web row to the CLI-flag table, but the same page's ## ServeOptions (programmatic embedding) table — introduced as "It mirrors the CLI flags above and adds:" — gains no serveWebShell row, even though serveWebShell?: boolean (types.ts) is the real field mapped from this flag (serve.ts:805, fast-path.ts:83) and honored by createServeApp/runQwenServe. Before this diff both tables were consistently silent about the switch; the added row makes the page contradict its own mirror claim. — Failure scenario: an embedder wiring runQwenServe/createServeApp programmatically — the named audience of that section — wants the API-only mode the new row describes, looks up the corresponding option in the ServeOptions table, finds nothing, and concludes the programmatic API has no opt-out. — Suggested fix: add a serveWebShell row to the ServeOptions table, e.g. "Serve the built Web Shell SPA at the daemon root (default true); false (the CLI's --no-web) leaves the daemon API-only. No effect when the build omits the Web Shell assets."
中文说明
[建议] 本 diff 在 CLI 参数表中新增了 --web/--no-web 行,但同一页的 ## ServeOptions (programmatic embedding) 表 —— 其引导语写着 "It mirrors the CLI flags above and adds:" —— 却没有新增 serveWebShell 行,尽管 serveWebShell?: boolean(types.ts)正是该参数映射的真实字段(serve.ts:805、fast-path.ts:83),且被 createServeApp/runQwenServe 实际消费。改动前两张表对这个开关都一致地未记录;新增行使该页与自身的 "镜像 CLI 参数" 声明相矛盾。— 失败场景:一位按该节目受众(编程嵌入方)接入 runQwenServe/createServeApp 的开发者,想要新行所描述的 API-only 模式,于是在 ServeOptions 表中查找对应选项,什么也找不到,从而以为编程接口没有该开关。— 建议修复:在 ServeOptions 表中补一行 serveWebShell,例如 "Serve the built Web Shell SPA at the daemon root (default true); false (the CLI's --no-web) leaves the daemon API-only. No effect when the build omits the Web Shell assets."。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| }); | ||
|
|
||
| it('demo self-origin shim still works when `--allow-origin` is set (loopback strip runs first)', async () => { | ||
| it('loopback self-origin shim still works when `--allow-origin` is set (loopback strip runs first)', async () => { |
There was a problem hiding this comment.
[Suggestion] The deleted /demo suite pinned the browser-UI surface on a non-loopback bind (hostname: '0.0.0.0' tests in this diff's - lines); the successor contract — "the shell is pre-auth in every launch mode", documented by this PR at 20-quickstart-operations.md:211 — has no non-loopback test pin. Every other deleted guard got an explicit re-pin (the CORS test even says "Re-pins the contract the deleted /demo CORS test carried"); the only non-loopback Web Shell test is the negative /health non-shadowing case (~line 3406). — Failure scenario: a future change reintroduces bind-conditional gating of mountWebShellAssets — the exact pattern this PR deletes (the old /demo registered after bearerAuth on non-loopback, so the precedent is in-repo and plausible to copy). Browser navigations to any --hostname 0.0.0.0 deployment then 401 and white-screen with no failing test. — Suggested fix: add one test in the Web Shell static serving describe: createServeApp({ ...baseOpts, hostname: '0.0.0.0', token: 'secret' }, undefined, { webShellDir }), then GET / with Accept: text/html → expect 200 and the shell body.
中文说明
[建议] 被删除的 /demo 测试套件曾对非 loopback 绑定下的浏览器 UI 面做过固定(本 diff - 行中 hostname: '0.0.0.0' 的测试);其继承契约 —— 本 PR 记录在 20-quickstart-operations.md:211 的 "shell 在所有启动模式下都是 pre-auth" —— 却没有非 loopback 的测试固定。其他每个被删的守卫都有显式重新固定(CORS 测试甚至注明 "Re-pins the contract the deleted /demo CORS test carried");唯一的非 loopback Web Shell 测试是反向的 /health 不遮蔽用例(约 3406 行)。— 失败场景:未来某个改动重新引入对 mountWebShellAssets 的按绑定条件鉴权 —— 正是本 PR 删除的模式(旧 /demo 在非 loopback 下注册在 bearerAuth 之后,该先例就在仓库里,很容易被照抄)。届时对任何 --hostname 0.0.0.0 部署的浏览器导航将 401 并白屏,且没有任何测试失败。— 建议修复:在 Web Shell static serving describe 中加一个测试:createServeApp({ ...baseOpts, hostname: '0.0.0.0', token: 'secret' }, undefined, { webShellDir }),然后以 Accept: text/html 请求 GET / → 断言 200 且返回 shell 主体。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| Reject any request with an `Origin` header. CLI/SDK never set Origin; only browsers do. Returns deterministic `403 { error: 'Request denied by CORS policy' }` rather than the 500 HTML the `cors` package's error-callback would produce. | ||
|
|
||
| Exception: the demo page's same-origin XHRs are handled by a separate middleware (in `server.ts`) that strips `Origin` when it matches the daemon's own address. | ||
| Exception: the Web Shell's same-origin XHRs are handled by a separate middleware (in `server.ts`) that strips `Origin` when it matches the daemon's own address. |
There was a problem hiding this comment.
[Suggestion] The demo→Web Shell rename keeps the claim that the strip middleware fires "when it matches the daemon's own address", but server/self-origin.ts matches a hard-coded loopback-host set only (127.0.0.1, localhost, [::1], host.docker.internal), independent of the bind address. The base sentence described the demo page, pre-auth only on loopback where the claim held; retargeting it to the Web Shell — pre-auth in every mode — makes it over-broad for non-loopback binds, and contradicts three sibling passages this same PR edits (qwen-serve.md:559 "Loopback self-origin hits", qwen-serve-protocol.md:45, 20-quickstart-operations.md §8's read-only caveat). — Failure scenario: operator binds non-loopback without --allow-origin; the shell HTML loads (pre-auth GET, no Origin), but every same-origin POST carries Origin: http://<lan-ip>:4170, unmatched by the strip set, so denyBrowserOriginCors 403s it and the UI cannot create sessions or send prompts. The operator reads this note — the LAN IP is "the daemon's own address" — concludes the shim malfunctioned instead of discovering the real --allow-origin requirement, and files a wrong-direction bug report.
| Exception: the Web Shell's same-origin XHRs are handled by a separate middleware (in `server.ts`) that strips `Origin` when it matches the daemon's own address. | |
| Exception: the Web Shell's same-origin XHRs on a **loopback** bind are handled by a separate middleware (in `server.ts`) that strips `Origin` when it matches one of the loopback self-origins (`127.0.0.1`, `localhost`, `[::1]`, `host.docker.internal`). On non-loopback binds the shell's XHRs carry an unmatched `Origin` and need `--allow-origin` for the daemon origin. |
中文说明
[建议] demo→Web Shell 的重命名保留了 "当 Origin 与 daemon 自身地址匹配时由剥离中间件处理" 的说法,但 server/self-origin.ts 只匹配一组硬编码的 loopback 主机(127.0.0.1、localhost、[::1]、host.docker.internal),与实际绑定地址无关。原句描述的是 demo 页面 —— 仅在 loopback 下 pre-auth,该说法在那里成立;将其改指 Web Shell(在所有模式下 pre-auth)后,对非 loopback 绑定就变得过宽,并与本 PR 同时修改的三处兄弟段落矛盾(qwen-serve.md:559 的 "Loopback self-origin hits"、qwen-serve-protocol.md:45、20-quickstart-operations.md §8 的只读说明)。— 失败场景:运维者在非 loopback 绑定且未加 --allow-origin;shell HTML 可以加载(pre-auth GET,不带 Origin),但每个同源 POST 都携带 Origin: http://<lan-ip>:4170,不在剥离集合内,被 denyBrowserOriginCors 以 403 拒绝,UI 无法创建会话或发送 prompt。运维者读到本句 —— 局域网 IP 正是 "daemon 自身地址" —— 会以为是剥离中间件出了故障,而不是发现真正需要的 --allow-origin 配置,从而提交方向错误的 bug 报告。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
|
||
| - Direct `createServeApp` without `deps.fsFactory` or `deps.bridge` defaults to `trusted: false`; agent-side ACP `writeTextFile` rejects as `untrusted_workspace`. The warning is printed once. | ||
| - `denyBrowserOriginCors` rejects **all** requests carrying `Origin`; the demo page works because another middleware strips matching same-origin values first. | ||
| - `denyBrowserOriginCors` rejects **all** requests carrying `Origin`; the Web Shell works because another middleware strips matching same-origin values first. |
There was a problem hiding this comment.
[Suggestion] The demo→Web Shell rename retargets this caveat from a surface pre-auth only on loopback (the base demo page) to one pre-auth in every mode, while the strip middleware it credits matches only hard-coded loopback origins — so "the Web Shell works" is over-broad for non-loopback binds (sibling of the 12-auth-security.md Exception note). — Failure scenario: operator runs qwen serve --hostname 0.0.0.0 without --allow-origin; the shell HTML loads pre-auth (document GET carries no Origin), but every same-origin XHR/POST carries Origin: http://<bind-addr>:4170, unmatched by the loopback-only strip set, and denyBrowserOriginCors 403s it — the shell renders but is completely non-functional, contradicting this caveat's explanation of why the deny-all-Origin wall never breaks the Web Shell.
| - `denyBrowserOriginCors` rejects **all** requests carrying `Origin`; the Web Shell works because another middleware strips matching same-origin values first. | |
| - `denyBrowserOriginCors` rejects **all** requests carrying `Origin`; the **loopback** Web Shell works because another middleware strips matching loopback same-origin values first — non-loopback binds require `--allow-origin` for the shell's XHRs. |
中文说明
[建议] demo→Web Shell 的重命名把这条注意事项从 "仅 loopback 下 pre-auth 的界面"(原 demo 页)改指到 "所有模式下都 pre-auth" 的界面,而它所依赖的剥离中间件只匹配硬编码的 loopback origin —— 因此 "the Web Shell works" 对非 loopback 绑定过宽(与 12-auth-security.md 中 Exception 一句属同类问题)。— 失败场景:运维者执行 qwen serve --hostname 0.0.0.0 且未加 --allow-origin;shell HTML 以 pre-auth 加载(文档 GET 不带 Origin),但每个同源 XHR/POST 都携带 Origin: http://<bind-addr>:4170,不在仅含 loopback 的剥离集合内,被 denyBrowserOriginCors 以 403 拒绝 —— shell 能渲染出来,却完全不可用,与本注意事项 "为何 deny-all-Origin 墙不会弄坏 Web Shell" 的解释相矛盾。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ``` | ||
|
|
||
| With the hardened loopback recipe (3), `/demo` is registered after `bearerAuth`. A normal browser navigation needs an auth header, so use curl or an SDK script instead. | ||
| With the hardened loopback recipe (3), `/health` is registered after `bearerAuth`, so probes must carry the token like every other route. |
There was a problem hiding this comment.
[Suggestion] "like every other route" is false in recipe 3 — the Web Shell entry points (GET /, /assets/*, /session/:id document navigations) remain pre-auth in every launch mode, including --require-auth (mountWebShellAssets is gated only by serveWebShell !== false, never by requireAuth/bind). §1 of this same page — also rewritten by this diff — says the shell loads without a token, and the --web row this PR adds uses the careful qualifier "every API route stays token-gated", which this sentence drops. The base sentence made no universal claim. — Failure scenario: an operator hardening a daemon with recipe 3 reads this note while enumerating the unauthenticated surface and concludes every route on the port is token-gated, missing that GET / still serves the full Web Shell SPA pre-auth on the same port.
| With the hardened loopback recipe (3), `/health` is registered after `bearerAuth`, so probes must carry the token like every other route. | |
| With the hardened loopback recipe (3), `/health` is registered after `bearerAuth`, so probes must carry the token like every other API route (the Web Shell static surface stays pre-auth by design; pass `--no-web` for an API-only daemon). |
中文说明
[建议] "like every other route" 在配方 3 下不成立 —— Web Shell 入口(GET /、/assets/*、/session/:id 文档导航)在所有启动模式下(包括 --require-auth)仍然 pre-auth(mountWebShellAssets 只受 serveWebShell !== false 约束,与 requireAuth/绑定方式无关)。本页 §1 —— 同样由本 diff 重写 —— 写明 shell 无需 token 即可加载;本 PR 新增的 --web 行也使用了 "every API route stays token-gated" 这一谨慎限定,而本句丢掉了该限定。原句(base)并未做这种全称断言。— 失败场景:运维者用配方 3 加固 daemon,在枚举未鉴权面时读到本句,以为该端口上所有路由都已 token 鉴权,从而忽略 GET / 仍在同一端口以 pre-auth 提供完整 Web Shell SPA。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| **`--allow-origin <pattern>` (T2.4 [#4514](https://github.com/QwenLM/qwen-code/issues/4514)).** Browser webuis hitting the daemon cross-origin are blocked by default — any request carrying an `Origin` header returns `403 {"error":"Request denied by CORS policy"}` because CLI/SDK clients never send `Origin` and the daemon treats its presence as a sign the request came from a browser context the operator has not opted into. Pass `--allow-origin <pattern>` (repeatable) at boot to install an allowlist instead of the wall. Each pattern is either: | ||
|
|
||
| - The literal `*` — admit any origin. **Risky**: boot refuses when `*` is configured but no bearer token is set (any source: `--token`, `QWEN_SERVER_TOKEN`, or `--require-auth` which mandates a token at boot). The boot breadcrumb emits a stderr warning when `*` is in the list. **Recommendation**: pair with `--require-auth` on loopback binds so `/health` and `/demo` are also gated by the bearer — they're registered before the bearer middleware on loopback by default (so k8s/Compose probes can reach `/health` without a token), and a `*` allowlist makes them reachable from any cross-origin browser. On non-loopback binds the bearer is already mandatory at boot, so the `*` exposure surface is just `/health` (status JSON) and `/demo` (a static page whose JS still calls token-gated routes) — the actual API surface is gated regardless. | ||
| - The literal `*` — admit any origin. **Risky**: boot refuses when `*` is configured but no bearer token is set (any source: `--token`, `QWEN_SERVER_TOKEN`, or `--require-auth` which mandates a token at boot). The boot breadcrumb emits a stderr warning when `*` is in the list. **Recommendation**: pair with `--require-auth` on loopback binds so `/health` is also gated by the bearer — it's registered before the bearer middleware on loopback by default (so k8s/Compose probes can reach it without a token), and a `*` allowlist makes it reachable from any cross-origin browser. On non-loopback binds the bearer is already mandatory at boot and `/health` is registered behind it, so the only surface `*` exposes without a token is the Web Shell static assets (`/`, `/assets/*`, and `/session/:id` document navigations — their JS still calls token-gated routes). `--no-web` removes even that; the actual API surface is gated regardless. |
There was a problem hiding this comment.
[Suggestion] The rewritten * bullet enumerates the token-less loopback exposure as only /health, but the Web Shell static assets stay pre-auth even under loopback + --require-auth (mountWebShellAssets is unconditional), so under a * allowlist they remain readable from any cross-origin browser. At base the sentence named both pre-auth surfaces (/health and /demo), and --require-auth gated both; the rewrite drops the second surface without acknowledging it can no longer be gated. The bullet's own non-loopback sentence, the sibling user-doc bullet (qwen-serve.md), and the boot warning all enumerate the Web Shell residual — this loopback sentence is the one location that agreement was not applied. — Failure scenario: an operator hardening a loopback daemon that uses --allow-origin '*' follows the recommendation, adds --require-auth, and concludes no token-less * surface remains — whereas any web page open in the user's browser can still read http://127.0.0.1:4170/ and /assets/* cross-origin. Harm is bounded (static shell assets only — the API stays token-gated), but this paragraph is the reference for *'s exposure and its loopback enumeration is incomplete. — Suggested fix: add a note mirroring the non-loopback one: even with --require-auth, the Web Shell static assets stay pre-auth on loopback by design and remain exposed to any cross-origin browser under *; --no-web removes that surface.
中文说明
[建议] 重写后的 * 条目把 loopback 下的无 token 暴露面只列为 /health,但 Web Shell 静态资源即使在 loopback + --require-auth 下仍然 pre-auth(mountWebShellAssets 无条件挂载),因此在 * 允许列表下它们依然可被任意跨源浏览器读取。base 版本同时列出两个 pre-auth 面(/health 与 /demo),且 --require-auth 当时能将两者都置于鉴权之后;重写丢掉了第二个面,且未说明它已无法再被鉴权。该条目自己的非 loopback 句、兄弟用户文档(qwen-serve.md)的条目、以及启动告警都列出了 Web Shell 残留面 —— 唯独这句 loopback 说明没有同步。— 失败场景:运维者为使用 --allow-origin '*' 的 loopback daemon 加固,按建议加上 --require-auth,便以为不再存在无 token 的 * 暴露面 —— 但用户浏览器里打开的任何网页仍可跨源读取 http://127.0.0.1:4170/ 与 /assets/*。危害有限(仅静态 shell 资源 —— API 仍受 token 鉴权),但本段是 * 暴露面的参考说明,其 loopback 枚举不完整。— 建议修复:补一条与非 loopback 句对应的说明:即使带 --require-auth,Web Shell 静态资源按设计在 loopback 下仍是 pre-auth,在 * 下仍暴露给任意跨源浏览器;--no-web 可移除该暴露面。
— qwen3.8-max via Qwen Code /review (v0.21.8)
…to loopback Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #8805All 7 Suggestion-level findings from the automated reviewer were verified against the code and implemented in one commit (
Verification notes: the middleware behavior claims (R2-4/R2-5/R2-7) were checked against VerificationCommands actually run, in order:
中文说明Autofix 审查轮次总结 — PR #8805自动审查者提出的全部 7 条建议级发现均已对照代码核实,并在一次提交(
核实说明:中间件行为相关的论断(R2-4/R2-5/R2-7)已对照 验证实际执行的命令,按顺序:
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.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 4221 passed — this review observed 18735 passed.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan（非阻断）：4221 passed — this review observed 18735 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| - **`--require-auth` shadows feature preflight.** Unauthenticated clients cannot discover the `require_auth` tag; their discovery surface is the 401 body itself. | ||
| - **Mutation gate body-parser ordering**: `mutationGate({strict: true})` 401 responses fire **after** `express.json()` parses the body. Worst case on a saturated loopback listener: `--max-connections × express.json({limit: '10mb'})` ≈ 2.5 GB transient. Loopback-only attack surface, intentionally accepted. | ||
| - **Same-origin Origin stripping** in `server.ts` happens _before_ `denyBrowserOriginCors`. If a future change moves the strip elsewhere, the demo page breaks. | ||
| - **Same-origin Origin stripping** in `server.ts` happens _before_ `denyBrowserOriginCors`. If a future change moves the strip elsewhere, the Web Shell breaks. |
There was a problem hiding this comment.
[Suggestion] R3-1: Contradictory file attribution for the self-origin strip middleware, introduced by this diff. The exception paragraph this PR rewrites (line 103 of this file) now correctly attributes the middleware to server/self-origin.ts, but this caveat line — edited in the same commit (the demo page breaks → the Web Shell breaks) — still says the stripping happens "in server.ts". The entire middleware lives in packages/cli/src/serve/server/self-origin.ts; server.ts retains only the one-line install call installSelfOriginStripMiddleware(app, getPort) (line 1036). — Failure scenario: a maintainer heeding this caveat — whose whole purpose is to warn where the ordering must be guarded — goes to server.ts to inspect or relocate the strip and no longer finds the strip logic there; in the security doc that gates the browser-origin wall, the pointer to the guard now points at the wrong file.
| - **Same-origin Origin stripping** in `server.ts` happens _before_ `denyBrowserOriginCors`. If a future change moves the strip elsewhere, the Web Shell breaks. | |
| - **Same-origin Origin stripping** (implemented in `server/self-origin.ts`, installed from `server.ts`) happens _before_ `denyBrowserOriginCors`. If a future change moves the strip elsewhere, the Web Shell breaks. |
中文说明
本次 diff 引入了 self-origin strip 中间件在文件归属上的自相矛盾。本 PR 重写的例外段落(本文件第 103 行)已正确地把该中间件归于 server/self-origin.ts,但这条注意事项——同一次提交里刚被编辑过(the demo page breaks → the Web Shell breaks)——仍然说 stripping 发生在 "in server.ts"。整个中间件都在 packages/cli/src/serve/server/self-origin.ts,server.ts 里只剩一行安装调用 installSelfOriginStripMiddleware(app, getPort)(第 1036 行)。 — 触发场景:维护者看到这条注意事项(它的全部作用就是警示这个顺序必须在哪里被守护)后去 server.ts 检查或移动 strip,却再也找不到 strip 逻辑——这份为浏览器同源墙把关的安全文档,把守护者指向了错误的文件。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| | `externalToolGuard` | Optional `{mode:'required', endpoint, token, timeoutMs?}`. Omission is fully off; required mode performs the provider handshake before listening. | | ||
| | `allowOrigins` | Cross-origin allowlist (`string[]`), corresponding to `--allow-origin`. | | ||
| | `allowPrivateAuthBaseUrl` | Allows private / localhost auth provider `baseUrl` installation. | | ||
| | `serveWebShell` | Serve the built Web Shell SPA at the daemon root (default `true`); `false` (the CLI's `--no-web`) leaves the daemon API-only. No effect when the build omits the shell assets. | |
There was a problem hiding this comment.
[Suggestion] R3-2: The new serveWebShell row's last sentence — "No effect when the build omits the shell assets." — is contradicted by runtime behavior: run-qwen-serve.ts:2919-2924 writes the missing-assets boot warning only inside if (opts.serveWebShell !== false), so serveWebShell=false (the CLI's --no-web) observably silences the per-boot warning — an effect in exactly the situation the row calls "No effect". — Failure scenario: in a build without Web Shell assets every boot prints "Web Shell assets not found; serving API only. … pass --no-web to silence this."; an operator trusting this doc line never tries the documented silencing path — the daemon's own warning tells them to use --no-web for a situation the doc says the flag cannot change. (The identical wording pre-exists in types.ts's JSDoc — fixing both keeps them consistent.)
| | `serveWebShell` | Serve the built Web Shell SPA at the daemon root (default `true`); `false` (the CLI's `--no-web`) leaves the daemon API-only. No effect when the build omits the shell assets. | | |
| | `serveWebShell` | Serve the built Web Shell SPA at the daemon root (default `true`); `false` (the CLI's `--no-web`) leaves the daemon API-only. No effect on what is served when the build omits the shell assets; `false` additionally silences the missing-assets boot warning. | |
中文说明
新增的 serveWebShell 行最后一句 — "No effect when the build omits the shell assets." — 与运行时行为矛盾:run-qwen-serve.ts:2919-2924 只在 if (opts.serveWebShell !== false) 内部输出缺少资产的启动警告,所以 serveWebShell=false(即 CLI 的 --no-web)会实实在在地让每次启动的警告消失 — 正是在这行文字说 "No effect" 的场景里有效果。 — 触发场景:在没有 Web Shell 资产的构建里,每次启动都会打印 "Web Shell assets not found; serving API only. … pass --no-web to silence this.";信任这行文档的运维者永远不会去试文档所说的消声途径 — daemon 自己的警告都在告诉他们对这个场景用 --no-web,而文档却说这个 flag 改变不了任何东西。(同样的措辞也早已存在于 types.ts 的 JSDoc — 两处一起改才能保持一致。)
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ## 8. Is there a browser UI? | ||
|
|
||
| **Yes.** It is implemented by `getDemoHtml(port)` in `packages/cli/src/serve/demo.ts` as self-contained HTML with no external dependency. | ||
| **Yes — the Web Shell.** `resolveWebShellDir()` finds the built assets (bundled next to the CLI bundle in a release, `packages/web-shell/dist` in a checkout) and `mountWebShellAssets()` serves them at `/`, `/assets`, and `/session/:id` document navigations (browser deep links — a plain `curl /session/<id>` gets the API's 401/404, not the shell). When the assets are missing the daemon degrades to API-only instead of crashing; `--no-web` opts out explicitly. |
There was a problem hiding this comment.
[Suggestion] R3-4: This diff adds guidance referencing --open (§1) and --no-web (§1, §2, §8) to this file, but the file's own section 3 "Full startup flags" table has no --web/--no-web row and no --open row, while docs/users/qwen-serve.md documents both (both flags pre-date this PR at commands/serve.ts:328-334). The omission predates the diff, but the new references are what make the file internally inconsistent. — Failure scenario: a reader following the new §1 instruction "start the daemon with --open (which puts the token in the URL fragment…)" turns to §3 — the self-described complete flag reference — for the flag's default or its interaction with --require-auth, and finds nothing, while far less central flags are listed; the reader concludes the guidance references a flag that doesn't exist or must hunt in a different document for basic semantics. Suggested fix: add --web/--no-web and --open rows to the §3 table mirroring the entries in docs/users/qwen-serve.md (this PR is the demo→Web Shell doc sweep, so this is the natural time).
中文说明
这个 diff 在本文件里新增了引用 --open(§1)和 --no-web(§1、§2、§8)的指引,但本文件自己的第 3 节 "Full startup flags" 表格既没有 --web/--no-web 行,也没有 --open 行,而 docs/users/qwen-serve.md 对两者都有记载(两个 flag 都早于本 PR,见 commands/serve.ts:328-334)。遗漏本身早于本 diff,但正是这些新引用让文件内部自相矛盾。 — 触发场景:读者按照新的 §1 指引 "start the daemon with --open(which puts the token in the URL fragment…)" 操作,然后翻到 §3 — 自称完整的 flag 参考 — 查这个 flag 的默认值或它与 --require-auth 的相互作用,却什么都找不到,而远不如它核心的 flag 反而都在表里;读者要么以为指引引用了一个不存在的 flag,要么得去别的文档里找基本语义。建议修复:给 §3 表格补上 --web/--no-web 和 --open 行,与 docs/users/qwen-serve.md 的条目保持一致(本 PR 正是 demo→Web Shell 的文档清扫,顺手补上正合适)。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| scoped.middleware(mockReq({ method: 'GET', path }), mockRes(), next); | ||
| scoped.middleware(mockReq({ method: 'GET', path }), res, vi.fn()); |
There was a problem hiding this comment.
[Suggestion] R3-5: The new pinning test's comment claims it "Pins the shape of the GET/HEAD exemption", but the loop only sends method: 'GET' — no HEAD request is exercised anywhere in this file (grep-verified; the only "HEAD" hits are the word "header"). The exemption it pins is (method === 'GET' || method === 'HEAD') && p === '/health' (rate-limit.ts:75). — Failure scenario: a future widening that applies to HEAD only — e.g. a new if (method === 'HEAD' && p === '/some-page') return null; branch, or splitting the predicate — lets an unauthenticated route escape the rate limiter (unlimited pre-auth HEAD traffic bypasses the DoS mitigation) while this test, which advertises itself as the guard against exactly that widening, stays green. Suggested fix (spans the loop declaration and these lines): run the same per-path loop over both methods, or narrow the comment to "GET exemption" if HEAD coverage is deliberately out of scope.
for (const method of ['GET', 'HEAD'] as const) {
for (const path of ['/demo', '/health/deep', '/healthz', '/']) {
// ... same fresh-limiter-per-call setup and 429 assertions,
// with mockReq({ method, path })
}
}中文说明
新增的固定测试的注释声称它 "Pins the shape of the GET/HEAD exemption",但循环只发送 method: 'GET' — 整个文件里没有任何 HEAD 请求被演练过(grep 验证;仅有的 "HEAD" 命中是 "header" 这个单词)。它固定的豁免条件是 (method === 'GET' || method === 'HEAD') && p === '/health'(rate-limit.ts:75)。 — 触发场景:未来一次只对 HEAD 生效的放宽 — 比如新增 if (method === 'HEAD' && p === '/some-page') return null; 分支,或把谓词拆开 — 会让一个未认证路由逃过限流(无限量的 pre-auth HEAD 流量绕过 DoS 缓解),而这个自称正是守护这种放宽的测试却一路绿灯。建议修复(跨循环声明与这两行):对 ['GET', 'HEAD'] 两种方法各跑一遍同样的每路径循环;或者,如果 HEAD 覆盖有意不在范围内,就把注释收窄为 "GET exemption"。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| it('serves the shell pre-auth on a non-loopback bind (every launch mode)', async () => { | ||
| // Re-pins the non-loopback half of the deleted `/demo` suite: the | ||
| // shell is pre-auth in every launch mode. The old `/demo` registered |
There was a problem hiding this comment.
[Suggestion] R3-6: This test's comment claims to pin "the shell is pre-auth in every launch mode … this fails first", but it exercises only one mode (hostname: '0.0.0.0', token: 'secret'); the requireAuth: true mode is unpinned here and — exhaustively grep-verified across all packages/cli test files — nowhere else. The behavior is explicitly documented by this PR (qwen-serve-protocol.md:23: "--require-auth still leaves the Web Shell static assets … pre-auth … by design"). — Failure scenario (probe-proven): gating mountWebShellAssets on opts.requireAuth !== true makes the filtered Web Shell suite pass 31/31 with zero failures — a future change that 401s address-bar navigations (which cannot attach the Authorization header) on exactly the hardened --require-auth deployments ships green. A comparator arm (gate on !opts.token) failed 5 tests, so the harness catches gating regressions in covered modes only. Suggested fix: extend this test (or add a sibling) to cover { hostname: '0.0.0.0', token: 'secret', requireAuth: true } and the loopback requireAuth: true case, asserting GET / with Accept: text/html still returns 200 + shell; or narrow the comment to the mode actually tested.
中文说明
这个测试的注释声称固定了 "the shell is pre-auth in every launch mode … this fails first",但它只演练了一种模式(hostname: '0.0.0.0', token: 'secret');requireAuth: true 模式在这里没有被固定,而且经过对 packages/cli 全部测试文件的穷尽 grep 验证,别处也没有。该行为被本 PR 明确写入文档(qwen-serve-protocol.md:23:"--require-auth still leaves the Web Shell static assets … pre-auth … by design")。 — 触发场景(已用探针证实):把 mountWebShellAssets 用 opts.requireAuth !== true 门控起来,过滤后的 Web Shell 测试套件以 31/31 全绿通过 — 未来若有改动让地址栏导航(无法附带 Authorization 头)在恰好是加固过的 --require-auth 部署上返回 401,它会在一片绿灯中上线。对照臂(用 !opts.token 门控)失败了 5 个测试,说明这套测试能抓住被覆盖模式下的门控回归,只是没盯住 requireAuth 模式。建议修复:给这个测试(或新增一个兄弟测试)补上 { hostname: '0.0.0.0', token: 'secret', requireAuth: true } 和 loopback requireAuth: true 两种情况,断言带 Accept: text/html 的 GET / 仍返回 200 + shell;或者把注释收窄到实际测试的模式。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| **`--allow-origin <pattern>` (T2.4 [#4514](https://github.com/QwenLM/qwen-code/issues/4514)).** Browser webuis hitting the daemon cross-origin are blocked by default — any request carrying an `Origin` header returns `403 {"error":"Request denied by CORS policy"}` because CLI/SDK clients never send `Origin` and the daemon treats its presence as a sign the request came from a browser context the operator has not opted into. Pass `--allow-origin <pattern>` (repeatable) at boot to install an allowlist instead of the wall. Each pattern is either: | ||
|
|
||
| - The literal `*` — admit any origin. **Risky**: boot refuses when `*` is configured but no bearer token is set (any source: `--token`, `QWEN_SERVER_TOKEN`, or `--require-auth` which mandates a token at boot). The boot breadcrumb emits a stderr warning when `*` is in the list. **Recommendation**: pair with `--require-auth` on loopback binds so `/health` and `/demo` are also gated by the bearer — they're registered before the bearer middleware on loopback by default (so k8s/Compose probes can reach `/health` without a token), and a `*` allowlist makes them reachable from any cross-origin browser. On non-loopback binds the bearer is already mandatory at boot, so the `*` exposure surface is just `/health` (status JSON) and `/demo` (a static page whose JS still calls token-gated routes) — the actual API surface is gated regardless. | ||
| - The literal `*` — admit any origin. **Risky**: boot refuses when `*` is configured but no bearer token is set (any source: `--token`, `QWEN_SERVER_TOKEN`, or `--require-auth` which mandates a token at boot). The boot breadcrumb emits a stderr warning when `*` is in the list. **Recommendation**: pair with `--require-auth` on loopback binds so `/health` is also gated by the bearer — it's registered before the bearer middleware on loopback by default (so k8s/Compose probes can reach it without a token), and a `*` allowlist makes it reachable from any cross-origin browser. `--require-auth` still leaves the Web Shell static assets (`/`, `/assets/*`, and `/session/:id` document navigations) pre-auth on loopback by design — they are mounted before the bearer middleware — so under a `*` allowlist they remain readable from any cross-origin browser; `--no-web` removes that surface. On non-loopback binds the bearer is already mandatory at boot and `/health` is registered behind it, so the only surface `*` exposes without a token is the Web Shell static assets (`/`, `/assets/*`, and `/session/:id` document navigations — their JS still calls token-gated routes). `--no-web` removes even that; the actual API surface is gated regardless. |
There was a problem hiding this comment.
[Suggestion] R3-7: This rewrite documents that the Web Shell entry points stay pre-auth even under --require-auth, but leaves the same section's two auth-surface definitions stale — line 7 ("every route except /health on loopback binds" must carry the bearer) and line 19 ("When the flag is on, the global bearerAuth middleware gates every route — including /capabilities"). Probe-verified at the reviewed commit: under loopback --require-auth, GET /, /assets/*, and /session/:id document navigations answer 200 pre-auth while /health//capabilities answer 401 — so the absolute claims are false in exactly the modes they quantify over, and this section now contradicts itself four lines apart. Note: the factual staleness predates this PR (the unconditional pre-auth mount landed with the Web Shell), but this diff's rewrite introduces the explicit contradiction — pre-PR, no sentence in this section predicated pre-auth-ness of --require-auth mode. (Same pattern as the rewritten CORS bullet in docs/users/qwen-serve.md — see the sibling comment.) — Failure scenario: an SDK/integration author or security reviewer deriving the token-required surface of a hardened daemon from this reference trusts "every route", deploys --require-auth, and concludes no unauthenticated surface remains — never considering --no-web; meanwhile GET / answers 200 without a token in exactly that mode. Or a maintainer reconciling the contradiction "fixes" the server to match the absolute promise and breaks browser address-bar navigations on exactly the hardened deployments this bullet promises stay pre-auth. Suggested fix: scope the quantifiers on lines 7/19 to API routes and name the exception, e.g. line 7 "every API route except /health on loopback binds (the Web Shell static assets are the other pre-auth exception — see below)".
中文说明
这次重写把 Web Shell 入口点在 --require-auth 下仍然 pre-auth 写进了文档,却让同一节里两处认证面定义保持陈旧:第 7 行("every route except /health on loopback binds" 必须携带 bearer)和第 19 行("When the flag is on, the global bearerAuth middleware gates every route — including /capabilities")。已在被审提交上用探针验证:loopback --require-auth 下,GET /、/assets/*、/session/:id 文档导航以 200 pre-auth 返回,而 /health//capabilities 返回 401 — 这些绝对化表述在它们所量化的模式里恰好是假的,本节如今在相隔四行之内自相矛盾。说明:事实层面的陈旧早于本 PR(无条件 pre-auth 挂载是随 Web Shell 一起引入的),但正是本 diff 的重写引入了显式矛盾 — PR 之前本节没有任何一句话声称过 --require-auth 模式下的 pre-auth 属性。(与 docs/users/qwen-serve.md 里重写的 CORS 条目是同一模式 — 见姊妹评论。) — 触发场景:SDK/集成作者或安全评审从这份参考推导加固 daemon 需要 token 的面,相信 "every route",部署 --require-auth,断定不再存在未认证面 — 从不考虑 --no-web;而 GET / 在该模式下恰好无需 token 就返回 200。或者维护者为消除矛盾把服务器 "修" 成符合绝对化表述,恰好在本条目承诺保持 pre-auth 的加固部署里弄坏了浏览器地址栏导航。建议修复:把第 7/19 行的量词收窄到 API 路由并点名例外,例如第 7 行改为 "every API route except /health on loopback binds (the Web Shell static assets are the other pre-auth exception — see below)"。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| - **`LOOPBACK_BINDS` includes IPv6** — `::1` and `[::1]` count as loopback for the no-token rule. | ||
| - **Host header allowlist** — on **loopback** binds the daemon checks `Host:` matches `localhost:port` / `127.0.0.1:port` / `[::1]:port` / `host.docker.internal:port` (case-insensitive per RFC 7230 §5.4) to defend against DNS rebinding. **Non-loopback binds (`--hostname 0.0.0.0`) intentionally bypass the Host allowlist** — the operator has chosen the surface area, so the bearer-token gate is the sole authentication layer; reverse proxies / SNI / client cert pinning are the operator's responsibility, not the daemon's. If you need Host-based isolation on a non-loopback bind, terminate TLS + check Host at a front proxy. | ||
| - **CORS denies any browser Origin by default** — returns `403` JSON. Pass **`--allow-origin <pattern>`** (repeatable, T2.4 #4514) to opt specific browser origins through. Each value is either the literal `*` (any origin — boot refuses if no bearer token is configured; `--require-auth` on loopback is recommended for full hardening since `/health` and `/demo` remain pre-auth on loopback by default) or a canonical URL origin (`<scheme>://<host>[:<port>]`, no trailing slash / path / userinfo). Matched origins receive proper CORS response headers (`Access-Control-Allow-Origin: <echoed>`, `Vary: Origin`, plus standard methods / headers / max-age and exposed `Retry-After`); unmatched origins still get a 403 with the same envelope as the default wall. `caps.features.allow_origin` is advertised conditionally so SDK / webui clients can pre-flight whether the daemon honors cross-origin hits before issuing them. Example: `qwen serve --allow-origin http://localhost:3000 --allow-origin http://localhost:5173`. Loopback self-origin hits (e.g. the `/demo` page) are unaffected — a separate Origin-strip shim handles them regardless of `--allow-origin`. **Browser webuis without `--allow-origin` configured** still fall back to the same Stage 1 options as before: package as a native shell (Electron/Tauri) so no `Origin` header is sent, or front the daemon with a same-origin reverse proxy. | ||
| - **CORS denies any browser Origin by default** — returns `403` JSON. Pass **`--allow-origin <pattern>`** (repeatable, T2.4 #4514) to opt specific browser origins through. Each value is either the literal `*` (any origin — boot refuses if no bearer token is configured; `--require-auth` on loopback is recommended for full hardening since `/health` remains pre-auth on loopback by default — note that the Web Shell static assets (`/`, `/assets/*`, `/session/:id` document navigations) are mounted before the bearer in every mode and stay pre-auth even under `--require-auth`, so use `--no-web` when the residual browser surface matters) or a canonical URL origin (`<scheme>://<host>[:<port>]`, no trailing slash / path / userinfo). Matched origins receive proper CORS response headers (`Access-Control-Allow-Origin: <echoed>`, `Vary: Origin`, plus standard methods / headers / max-age and exposed `Retry-After`); unmatched origins still get a 403 with the same envelope as the default wall. `caps.features.allow_origin` is advertised conditionally so SDK / webui clients can pre-flight whether the daemon honors cross-origin hits before issuing them. Example: `qwen serve --allow-origin http://localhost:3000 --allow-origin http://localhost:5173`. Loopback self-origin hits (e.g. the Web Shell UI) are unaffected — a separate Origin-strip shim handles them regardless of `--allow-origin`. **Browser webuis without `--allow-origin` configured** still fall back to the same Stage 1 options as before: package as a native shell (Electron/Tauri) so no `Origin` header is sent, or front the daemon with a same-origin reverse proxy. |
There was a problem hiding this comment.
[Suggestion] R3-7: The rewritten bullet asserts the Web Shell stays pre-auth "even under --require-auth" — accurate — but that directly contradicts the same file's own Authentication section: line 330 ("Pass --require-auth to make the bearer token mandatory on every route") and line 328 (non-loopback /health "requires the token like every other route"). Probe-verified at the reviewed commit: under loopback --require-auth, GET /, /assets/*, /session/:id document navigations answer 200 pre-auth while /health//capabilities answer 401. Lines 328/330 were already false pre-PR (the Web Shell pre-auth mount predates this PR), but this diff's rewrite introduces the explicit intra-file contradiction — "stay pre-auth even under --require-auth" is text this PR adds, and it negates line 330's absolute promise in that flag's own mode. (Same pattern as qwen-serve-protocol.md lines 7/19 — see the sibling comment.) — Failure scenario: an operator hardening a daemon follows this file's Authentication section, deploys qwen serve --require-auth, trusts the absolute "mandatory on every route" promise and concludes no unauthenticated surface remains — never considering --no-web; meanwhile GET / answers 200 without a token in exactly that mode (as this bullet itself now states). Suggested fix: scope the absolutes to API routes and name the exception — e.g. line 330 "mandatory on every API route … (the Web Shell static surface stays pre-auth by design — pass --no-web to remove it)", line 328 "like every other API route".
中文说明
这条重写的条目断言 Web Shell "even under --require-auth" 仍然 pre-auth — 这没错 — 但它与本文件自己的 Authentication 一节直接矛盾:第 330 行("Pass --require-auth to make the bearer token mandatory on every route")和第 328 行(非 loopback 的 /health "requires the token like every other route")。已在被审提交上用探针验证:loopback --require-auth 下,GET /、/assets/*、/session/:id 文档导航以 200 pre-auth 返回,而 /health//capabilities 返回 401。第 328/330 行在本 PR 之前就已经是假的(Web Shell 的 pre-auth 挂载早于本 PR),但正是本 diff 的重写引入了文件内的显式矛盾 — "stay pre-auth even under --require-auth" 是本 PR 新增的文字,它在这个 flag 自己的模式里否定了第 330 行的绝对化承诺。(与 qwen-serve-protocol.md 第 7/19 行是同一模式 — 见姊妹评论。) — 触发场景:运维者按本文件的 Authentication 一节加固 daemon,部署 qwen serve --require-auth,相信 "mandatory on every route" 的绝对化承诺,断定不再有未认证面 — 从不考虑 --no-web;而 GET / 在该模式下恰好无需 token 就返回 200(正如本条目自己所写)。建议修复:把绝对化表述收窄到 API 路由并点名例外 — 如第 330 行改为 "mandatory on every API route … (the Web Shell static surface stays pre-auth by design — pass --no-web to remove it)",第 328 行改为 "like every other API route"。
— qwen3.8-max via Qwen Code /review (v0.21.8)
…emo-page # Conflicts: # docs/users/qwen-serve.md
|
@qwen-code /resolve |
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Autofix address-review summary — PR #8805 (round 6)Feedback triageCritical-only mode is active after 5 change-producing rounds. All actionable sections this round were empty:
The 7 deferred non-Critical items (1 review + 6 inline findings) were left untouched per the Critical-only rule: no code changes, no thread resolutions, no comment replies for them. They remain open for human follow-up. Work done: base-conflict resolution (
|
|
Qwen Code did not run conflict resolution for this request. PR #8805 does not currently have merge conflicts with main. |
jifeng
left a comment
jifeng
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 157 passed · 0 failed · 157 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：157 通过 · 0 失败 · 157 总计 Verification reportPR #8805 deep verification (round 2) —
|
| # | Previous item | Severity | Status at b1b135b9 |
|---|---|---|---|
| 1 | tsc --noEmit clean on both arms in this container, although the description cites pre-existing errors (qrcode-terminal types, a stale session-service declaration) |
informational | stands — re-ran on both arms at the new head: 0 errors on both (base arm needed the PR-untouched dist/ + per-package node_modules linked in, then compiled clean) |
| 2 | Suite count drift: description cites 4221 passed; round 1 measured 4242 / 1 skipped |
informational | stands — re-measured 4244 passed / 2 skipped across 147 files; the delta keeps tracking merged-main additions, and the PR diff itself adds no .skip (grep = 0) |
Round 1's "Not covered" items: per-commit attribution is still graph-limited (depth-2
graft), but the two delta commits are now reachable as objects and were individually
inspected (see Not covered); the rest carry over unchanged and are re-listed below.
Central claim + A/B (re-measured at the new head)
Central claim: /demo is fully removed — a non-navigation request 404s, a browser
navigation falls through to the SPA fallback (Web Shell) on a tokenless loopback daemon
and is refused with 401 once a token is configured; /health and its rate-limit
exemption are unchanged. Secondary claims: the rewritten --allow-origin '*' warning
enumerates the real pre-auth surface, and the CORS-wall contract the deleted demo test
carried is re-pinned on the shell root.
Booted a real qwen serve daemon per arm (TS source via the repo's own tsx runner,
so the only difference between arms is the PR diff) across 7 launch modes, probed over
real HTTP. Both arms assert against arm-specific expectation tables; a flip is an
expected difference, and every cell's expectation held. Witnesses:
evidence/01-ab-base-matrix.png (base arm, 73 assertions) and
evidence/02-ab-head-matrix.png (head arm, 70 assertions); raw log logs/01-matrix-full.out.
| Cell | Scenario | BASE (77bd04bd) |
HEAD (b1b135b9) |
flip? |
|---|---|---|---|---|
| A1 | tokenless loopback, GET /demo (JSON Accept) |
200 demo HTML | 404, no shell marker | ✅ load-bearing |
| A2 | tokenless loopback, GET /demo (browser nav) |
200 demo + X-Frame-Options: DENY |
200 SPA shell (<div id="root">) |
✅ shape change |
| A3 | tokenless loopback, GET /demo cross-origin |
403 CORS wall | 403 CORS wall | unchanged (wall runs first on both) |
| A4 | tokenless loopback, GET / cross-origin |
403 wall | 403 wall | unchanged — the re-pinned CORS contract, live |
| A5–A8 | /health, /health?deep=1 (workspaceCount), /, unknown path |
200 ok / counters / shell / 404 |
identical | unchanged |
| A9/A10 | GET /session/<id> nav vs JSON |
200 shell / 404 | 200 shell / 404 | unchanged (doc-navigation qualifier) |
| B1/B2 | loopback + token (warm runtime), /demo no auth (nav/json) |
200 demo — pre-auth exposure | 401 | ✅ load-bearing |
| B3/B4 | loopback + token, /demo with bearer (json/nav) |
200 demo / demo | 404 / 200 SPA shell | ✅ |
| B5–B7 | / no auth, /daemon/status no auth, /health no auth |
200 shell / 401 / 200 | identical | unchanged |
| C1/C2 | loopback + token + --require-auth, /health no/bearer |
401 / 200 | 401 / 200 | unchanged |
| C3/C4 | /demo nav no auth / with bearer under --require-auth |
401 / 200 demo | 401 / 200 SPA shell | ✅ |
| C5/C6 | / and /assets/* no token under --require-auth |
200 shell / 200 asset | identical | unchanged (static pre-auth even under --require-auth) |
| D1/D2 | non-loopback (0.0.0.0) + token, /health no/bearer |
401 / 200 | 401 / 200 | unchanged |
| D3 | non-loopback, GET / no token |
200 shell | 200 shell | unchanged (static pre-auth on any bind) |
| D4/D5 | non-loopback, /demo nav no auth / json with bearer |
401 / 200 demo | 401 / 404 | ✅ |
| D6 | non-loopback, cross-origin POST /session/:id/prompt |
403 CORS wall | 403 CORS wall | unchanged — doc §8 claim |
| D7 | non-loopback, same POST without Origin, no auth |
401 bearer | 401 bearer | unchanged (wall is Origin-keyed) |
| E1 | --allow-origin '*' boot warning |
enumerates /health and /demo |
names Web Shell static assets, --no-web, /health, --require-auth; no /demo |
✅ |
| E2 | under *, cross-origin GET / |
200 + ACAO echoes origin | identical | unchanged |
| RL1 | --rate-limit --rate-limit-read 2 --require-auth, /health ×5 with bearer |
200×5 | 200×5 | unchanged (still exempt — here only the exemption list can save it, since --require-auth registers /health after bearerAuth) |
| RL2 | same mode, /demo ×5 with bearer |
200×5 (on the exemption list) | 404,404,429,429,429 | ✅ exemption narrowed |
| RL3 | /health after the bucket burned |
200 | 200 | unchanged |
| F1–F3 | --no-web: /, /demo nav, /health |
404 / 200 demo / 200 | 404 / 404 / 200 | ✅ demo gone even with the shell disabled |
One base-side nuance worth naming (pre-existing, not caused by this PR): on
loopback+token the base's /demo pre-auth exposure is observable only on the warm
runtime app — a cold daemon's deferred gate 401s the unauthenticated request first
(because its exemption list covers only shell entry points). The matrix warms the
runtime with a bearer'd session lookup before the B1/B2 cells so the exposure the PR
removes is the one actually measured.
Delta-specific re-verification. The CORS re-pin test's contract was exercised live
(A4: cross-origin GET / → 403 {error: 'Request denied by CORS policy'} on both arms),
and the loopback-scoped self-origin claim of the rewritten security doc was probed live:
on a 0.0.0.0 bind a LAN origin's POST hits the wall (403) while a loopback origin is
stripped and passes to the bearer gate (401) — evidence/04-doc-claim-probes.png, 4/4.
A repo-wide /demo census at head (docs/ + packages/cli/src) found exactly one hit —
"source": "@scope/demo" in the protocol doc's channel-scope example, unrelated to the
page — so the rename sweep is complete. The conflict resolution in
docs/users/qwen-serve.md kept both sides: main's rewritten --max-journal-events
wording is present, the PR's pre-auth wording is present, the old journal wording and
every /demo reference are gone.
Corrections
None. No earlier round or bot comment misdescribed the code in a way that needs correcting.
Findings
None blocking; no defects found. Three informational notes:
- (Carried, re-measured)
npx tsc --noEmitinpackages/cliis clean on both
arms in this container; the description's cited pre-existing errors did not
reproduce here in round 1 and still do not. Both arms equally clean ⇒ the
load-bearing fact (no new type errors) holds. - (Carried, re-measured) The description's
4221 passedremains stale against
4244 passed / 2 skippedmeasured here; the drift tracks mergedmain, and the PR
diff adds no skips. Suite fully green. - (New, context for the B cells) The cold/warm gating split on the base arm
described above means the base's loopback pre-auth/demoexposure required a warm
daemon; a cold one already 401d it via the deferred gate. This is pre-existing
fast-path architecture on the base (the gate shipped viamain, not this PR) and
does not weaken the removal — head returns 401 in both states.
Not covered
- Per-commit exercise remains graph-limited. The checkout is depth-2 and grafted
(git rev-parse --is-shallow-repository= true);git merge-base --is-ancestorsays
none of the 7 commits in the snapshot is an ancestor ofHEAD^2. The four commit
objects are nonetheless present locally, so this round inspected the two delta commits'
own diffs (git show 5551616,git show 32378600) and verified each of their claims
against the merge head (test re-pins present, doc wording present, runtime behavior
measured). The two older commits were verified in round 1 and their surface was
re-measured in this round's aggregate A/B. Per-commit harness runs were not possible. - Repo-wide gates the PR's own CI already runs (full
npm run build, repo-wide test
suite, ESLint/Prettier, integration suites) — not re-run; targeted gates instead
(vitest run src/serveon head: 147 files / 4244 pass / 2 skip / exit 0;
tsc --noEmitboth arms: clean). - Windows/Linux-specific paths — the loopback/0.0.0.0 behavior exercised here is the
POSIX path; the author marked Windows/Linux untested locally. - Release-bundling claim (Web Shell shipped next to the CLI bundle) — not exercised;
the A/B used each tree'spackages/web-shell/dist(the PR-untouched head build,
symlinked into the base tree, byte-identical by virtue of the PR's zero diff there). ?deep=1byte equality across arms — asserted 200 +workspaceCountkey on both
arms, not a counter-by-counter diff.- Real-browser rendering of the Web Shell — probes are HTTP-level; the shell's own JS
behavior is out of this PR's diff (packages/web-shell untouched).
Methodology
Environment: Linux node v22.23.2 container, repo at the merge ref
(5017d71da3), npm ci + npm run build pre-run. A/B (01-daemon-matrix.mjs):
14 real daemons (7 launch modes × 2 arms) spawned via
node node_modules/tsx/dist/cli.mjs <tree>/packages/cli/index.ts serve … with isolated
$HOME and scratch workspaces; base arm from a git worktree of HEAD^1. Workspace-link
confound checked: import.meta.resolve('@qwen-code/qwen-code-core') from the base tree
resolves into the head tree (git diff HEAD^1..HEAD -- packages/core packages/web-shell
is empty, so identical content); the base tree additionally needed the PR-untouched
build artifacts it lacks (the git-ignored generated git-commit.ts copied; the
packages/web-shell/dist and remaining package dist/ + per-package node_modules
dirs symlinked from the head tree) — runtime startup fails without the generated file,
which surfaced as a 503 daemon_runtime_failed before being diagnosed. Harness
expectations were corrected three times during development (warm-vs-cold gate for the
base B cells, --rate-limit master switch + bearer for the RL cells, ACAO origin-echo
under *) — all three were harness-hypothesis errors proven against the code, not PR
behavior; the final tables encode the corrected expectations and passed 143/143. The
mutation matrix ran in a scratch HEAD worktree (02-mutation-matrix.mjs, restored
clean afterwards: git status empty for packages/cli/src). Doc claims were checked by
grep against the rewritten files and by the live probes above. Raw logs:
logs/01-matrix-full.out, logs/02-mutation-full.out,
logs/03-gate-vitest-src-serve-head.log, logs/04-doc-claim-probes.out.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ``` | ||
|
|
||
| Open `http://127.0.0.1:4170/demo` in a browser to see the debug console: chat UI, event stream, and workspace inspection. In the default loopback dev mode, `createServeApp()` mounts the `/demo` route from `packages/cli/src/serve/routes/health-demo.ts` **before** `bearerAuth`, so no token is required. | ||
| Open `http://127.0.0.1:4170/` in a browser to get the Web Shell UI: chat, session list, and workspace inspection. `createServeApp()` mounts the bundled Web Shell assets (`packages/cli/src/serve/web-shell-static.ts`) **before** `bearerAuth`, so the shell itself loads without a token; its own API calls carry the bearer when one is configured — start the daemon with `--open` (which puts the token in the URL fragment, never sent to the server) or append `#token=…` manually when auth is enabled. `--no-web` opts out and leaves the daemon API-only. |
There was a problem hiding this comment.
[Suggestion] R3-4: This diff adds guidance referencing --open (§1) and --no-web (§1, §2, §8) to this file, but the file's own section 3 "Full startup flags" table has no --web/--no-web row and no --open row. Before this PR the page never mentioned these flags, so every flag its prose referenced was covered by the table until this diff newly breaks that in-file correspondence; --open in particular is documented nowhere in the developer docs. — Failure scenario: an operator following the quickstart consults the same page's "Full startup flags" table to confirm --open semantics and finds no entry, having to read CLI source (packages/cli/src/commands/serve.ts:334) instead.
Suggested fix: add --open and --web/--no-web rows to the section 3 table using the CLI descriptions from commands/serve.ts:328-339 (02-serve-runtime.md already has a --web/--no-web row to mirror).
中文说明
本 diff 在这个文件中新增了引用 --open(§1)和 --no-web(§1、§2、§8)的指引,但本文件自己的第 3 节 "Full startup flags" 表格里既没有 --web/--no-web 行,也没有 --open 行。在本 PR 之前该页面从未提及这些 flag,所以正文引用的每个 flag 都能在表格里找到,直到本 diff 新打破了这一文件内对应关系;尤其是 --open 在开发者文档中完全没有记载。——触发场景:按 quickstart 操作的运维者到同一页的 "Full startup flags" 表格确认 --open 的语义,却找不到对应条目,只能去读 CLI 源码(packages/cli/src/commands/serve.ts:334)。
建议修复:用 commands/serve.ts:328-339 中的 CLI 描述,在第 3 节表格里补上 --open 和 --web/--no-web 行(02-serve-runtime.md 里已有可参照的 --web/--no-web 行)。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| | Loopback without `--require-auth` | `routes/health-demo.ts`, mounted by `createServeApp()` **before** `bearerAuth` | Works without token | | ||
| | Loopback with `--require-auth` | `routes/health-demo.ts`, mounted by `createServeApp()` **after** `bearerAuth` | Difficult to use from a plain browser; use curl or SDK | | ||
| | Non-loopback bind | `routes/health-demo.ts`, mounted by `createServeApp()` **after** `bearerAuth` | Same as above | | ||
| The static shell is mounted **before** `bearerAuth` in every launch mode — a browser cannot attach an `Authorization` header to an address-bar navigation or a `<script src>` subresource, so gating it would just break the UI. Every API route it calls stays token-gated, and the front end attaches the bearer itself. On a non-loopback bind the shell is read-only unless `--allow-origin <origin>` is passed — same-origin POSTs carry an `Origin` header that the CORS wall rejects (403) — so pass `--allow-origin` for any bind beyond loopback. |
There was a problem hiding this comment.
[Suggestion] R4-1: The §8 rewrite adds the absolute claim "Every API route it calls stays token-gated", but in the loopback developer default (no token, no --require-auth) bearerAuth (packages/cli/src/serve/auth.ts:272) returns a pass-through middleware, so there is no token gate at all in the most common launch mode. Even with a token configured, /health stays pre-auth on loopback unless --require-auth is set (exposeHealthPreAuth = loopback && !opts.requireAuth, routes/health.ts:160; deliberately pinned by server.test.ts "exempts /health from bearer auth so liveness probes work without credentials"). — Failure scenario: a reader auditing whether pre-auth shell mounting weakens API auth gets a blanket guarantee the code does not provide.
Suggested fix: qualify the clause — "every API route it calls still passes through bearerAuth (open only in the loopback developer default without --require-auth; /health additionally stays pre-auth on loopback unless --require-auth is set)". The same phrasing is added by this diff at 02-serve-runtime.md:52 (subsystems table) and 17-configuration.md:35 (--web flag row), and also appears at docs/users/qwen-serve.md:415 — worth fixing together.
中文说明
§8 的重写新增了绝对化表述 "Every API route it calls stays token-gated",但在 loopback 开发者默认模式(无 token、无 --require-auth)下,bearerAuth(packages/cli/src/serve/auth.ts:272)返回的是直通中间件,即最常见的启动模式下根本没有 token 门槛。即使配置了 token,只要未设置 --require-auth,/health 在 loopback 上仍是 pre-auth(exposeHealthPreAuth = loopback && !opts.requireAuth,routes/health.ts:160;server.test.ts 的 "exempts /health from bearer auth so liveness probes work without credentials" 专门钉住了该行为)。——触发场景:评审者评估 pre-auth 挂载 shell 是否削弱 API 认证时,会得到代码并未提供的绝对保证。
建议修复:收窄该句——"every API route it calls still passes through bearerAuth(open only in the loopback developer default without --require-auth;/health additionally stays pre-auth on loopback unless --require-auth is set)"。同样的措辞在本 diff 中还出现在 02-serve-runtime.md:52(subsystems 表)与 17-configuration.md:35(--web flag 行),另见 docs/users/qwen-serve.md:415——建议一并修改。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| next, | ||
| ); | ||
| it('exempts no GET path other than /health', () => { | ||
| // Pins the shape of the GET/HEAD exemption after `/demo` was removed |
There was a problem hiding this comment.
[Suggestion] R3-5: The new pinning test's comment claims it "Pins the shape of the GET/HEAD exemption" after /demo was removed from it, but the loop only sends method: 'GET' — no HEAD request is exercised anywhere in this file (grep-verified: the only "HEAD" hits are setHeader/Retry-After header). The implementation exempts GET and HEAD alike (resolveTier in rate-limit.ts), so the HEAD half of the claimed contract is unpinned. — Failure scenario: a future change dropping HEAD from the exemption predicate (or splitting its tier) leaves every test green while HEAD probes against /health start consuming read tokens and 429ing on hardened deploys — the exact regression the comment says this test exists to catch.
Suggested fix: either exercise each path with both GET and HEAD in the loop, or narrow the comment to the GET exemption it actually pins:
// Pins the shape of the GET exemption after `/demo` was removed中文说明
新增钉住测试的注释声称它 "Pins the shape of the GET/HEAD exemption"(在 /demo 被移出豁免之后),但循环只发送 method: 'GET'——整个文件没有任何 HEAD 请求被执行(grep 验证:仅有的 "HEAD" 命中是 setHeader/Retry-After header)。实现对 GET 和 HEAD 一视同仁地豁免(rate-limit.ts 的 resolveTier),因此所声称契约中 HEAD 的一半并没有被钉住。——触发场景:未来某个改动把 HEAD 从豁免谓词里去掉(或拆分它的 tier)时,所有测试仍是绿的,而针对 /health 的 HEAD 探测开始消耗 read 配额、在加固部署上返回 429——恰好就是注释声称本测试要拦截的那种回归。
建议修复:要么在循环里对每个路径同时执行 GET 和 HEAD,要么把注释收窄为它实际钉住的 GET 豁免:
// Pins the shape of the GET exemption after `/demo` was removed— qwen3.8-max via Qwen Code /review (v0.21.8)
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. ✅
|
Released in v0.21.9. |
4 participants
What this PR does
Removes the daemon's
/demodebug page and folds its remaining infrastructure back into the health route.The daemon has shipped a real browser UI for a while now:
qwen servelocates the bundled Web Shell assets at startup and serves them from the root path, so opening the daemon in a browser already lands on a full client with chat, session list, workspace inspection, and permission handling./demostayed behind as a 663-line self-contained HTML console covering the same ground with none of the reach.The demo handler shared a module with
/health, so that module is now health-only and drops the port accessor it needed solely to render the page. The rate-limit exemption list, the--allow-origin '*'boot warning, and the daemon documentation lose their/demoarms. The loopback self-origin regression test was already asserting through/health— only its title mentioned the demo — so its coverage is untouched.Why it's needed
The page is not just dead weight, it actively misroutes work. It is the only place in the tree that pairs an event log with daemon HTTP traffic, so anything that presents as "the event stream is noisy" or "the console renders this wrong" gets attracted to it even when the observation came from somewhere else. #8762 is the clearest case: the flooding was found while driving
/reviewthrough the Web Shell, and the fix landed entirely inside the demo page's rendering — its own risk note says no Web Shell changes. The real client kept whatever behaviour it had, and a page nobody drives the daemon through got three rounds of tests.Nobody depends on it either. Releases bundle the Web Shell next to the CLI bundle, and the daemon dev script starts the Web Shell dev server rather than the demo page, so neither the shipped path nor the development path passes through
/demo. Protocol-level debugging does not regress: subscribing to a session's event stream yields the same raw frames the Events tab was printing, which is what anyone actually inspecting the wire format reaches for.Reviewer Test Plan
How to verify
/demono longer exists. On a tokenless loopback daemon a browser navigating to the old URL is caught by the SPA fallback and gets the Web Shell rather than a dead link, while a non-navigation request returns 404. Once a token is configured — with or without--require-auth— that navigation is refused with 401 instead, because the SPA fallback sits behind the bearer./healthbehaves exactly as before in both gating modes: reachable without a token on a loopback bind, and gated behind the bearer on a non-loopback bind or with--require-auth. Deep probes (?deep=1) still return the aggregate daemon counters./health, and the daemon still starts and serves its API surface unchanged.Local results:
npx vitest run src/servein the CLI package: 4221 passed, 1 skipped, across 147 files. One run had two unrelated flakes (workspace-generation.test.tsfailing withParse Error: Expected HTTP/, RTSP/ or ICE/, a socket-level hiccup); a clean re-run was fully green.npx tsc --noEmitin the CLI package introduces no new errors — the ones it reports (missingqrcode-terminaltypes, a stale session-service declaration) are already present onmain./healthreturns{"status":"ok"},/demoreturns 404 for a plain request and the SPA shell for a browser-style document navigation.Evidence (Before & After)
N/A — no user-visible UI changes. The only surface removed is a debug page superseded by the Web Shell.
Tested on
Environment (optional)
Unit tests and a supertest-level smoke check against
createServeAppwith the built Web Shell assets present.Risk & Scope
usage_updateframe cadence that motivated fix(serve): stop usage_update frames from flooding the demo event log #8762 is unchanged and remains correct protocol behaviour. Windows and Linux were not exercised locally — CI covers them.GET /demois gone. It was a debug affordance, not part of the wire protocol, and no SDK or client code referenced it.Linked Issues
Follow-up to #8762.
中文说明
这个 PR 做了什么
删除 daemon 的
/demo调试页,并把它残留的基础设施收回到 health 路由里。daemon 早就带了真正的浏览器 UI:
qwen serve启动时会定位随包的 Web Shell 资源并从根路径提供服务,所以用浏览器打开 daemon 本来就落在一个完整的客户端上——聊天、会话列表、workspace 检查、权限处理都有。/demo只是留下来的一个 663 行自包含 HTML 控制台,覆盖同样的场景却完全没有这些能力。demo 处理器原本和
/health共用一个模块,现在这个模块只剩 health,并去掉了仅为渲染页面而需要的端口访问器。限流豁免名单、--allow-origin '*'的启动告警、以及 daemon 文档都去掉了各自的/demo分支。loopback 同源回归测试本来就是打/health的——只有标题里提到 demo——所以它的覆盖没有受影响。为什么需要
这个页面不只是死代码,它会实实在在地把工作引到错误的地方。它是仓库里唯一把事件日志和 daemon HTTP 流量放在一起的地方,所以任何表现为"事件流太吵"或"控制台渲染不对"的问题都会被它吸过去,哪怕现象根本来自别处。#8762 是最清楚的例子:刷屏是在通过 Web Shell 跑
/review时发现的,修复却完全落在 demo 页的渲染里——它自己的风险栏就写着不改 Web Shell。真正的客户端行为一点没变,而一个没人用来驱动 daemon 的页面得到了三轮测试。也没有人依赖它。发布产物把 Web Shell 打包在 CLI bundle 旁边,daemon 开发脚本启动的是 Web Shell 的 dev server 而不是 demo 页,所以无论发布路径还是开发路径都不经过
/demo。协议层调试能力没有退化:订阅会话事件流拿到的就是 Events 标签在打印的那些原始帧,真要看线协议的人本来也是用它。审阅者测试计划
如何验证
/demo不再存在。在无 token 的 loopback daemon 上,浏览器访问旧 URL 会被 SPA 兜底接住、拿到 Web Shell 而不是死链,非导航类请求返回 404。一旦配置了 token(无论是否带--require-auth),该导航会改为 401,因为 SPA 兜底位于 bearer 之后。/health在两种网关模式下行为完全不变:loopback 绑定下无 token 可达,非 loopback 绑定或带--require-auth时由 bearer 把关。深度探测(?deep=1)仍返回 daemon 聚合计数。/health,daemon 仍正常启动并提供不变的 API 面。本地结果:
npx vitest run src/serve:147 个文件,4221 通过、1 跳过。其中一次有两个无关抖动(workspace-generation.test.ts报Parse Error: Expected HTTP/, RTSP/ or ICE/,socket 层面的偶发);干净重跑全绿。npx tsc --noEmit没有新增错误——它报的那些(缺qrcode-terminal类型、一处陈旧的 session service 声明)在main上本来就有。/health返回{"status":"ok"},/demo对普通请求返回 404、对浏览器式文档导航返回 SPA 外壳。证据(改动前后)
N/A —— 没有用户可见的 UI 变化。唯一移除的界面是一个已被 Web Shell 取代的调试页。
测试平台
环境(可选)
单元测试,以及在 Web Shell 构建产物存在的前提下对
createServeApp做的 supertest 级冒烟检查。风险与范围
usage_update帧节奏没有改动,它本来就是正确的协议行为。Windows 和 Linux 未在本地跑——由 CI 覆盖。GET /demo不再存在。它是调试用的便利设施,不属于线协议,没有任何 SDK 或客户端代码引用它。关联 Issue
#8762 的后续。