fix(cli): address review on Web Shell serving

Review fixes for #5392 (qwen-code-ci-bot):

- [Critical] SPA fallback no longer shadows /health or /demo on non-loopback
  binds — those paths fall through to their own routes / bearerAuth instead
  of receiving index.html.
- [Critical] --open trims the bearer token before putting it in the browser
  URL, matching runQwenServe's own trimming, so a trailing newline from
  `$(cat token.txt)` no longer makes every API call 401.
- --open is wrapped in its own try/catch so a failed browser launch can't
  take down the already-listening daemon; it normalizes wildcard binds
  (0.0.0.0 / ::) to loopback, and only fires when the UI is actually mounted
  (new RunHandle.webShellMounted).
- resolveWebShellDir() now requires BOTH index.html and assets/, so a partial
  build degrades to API-only instead of serving a shell whose chunks 404.
- runQwenServe logs a positive "Web Shell UI served from <dir>" breadcrumb,
  and warns that on a non-loopback bind without --allow-origin the shell is
  read-only (same-origin POSTs are blocked by the CORS wall).
- Document the --open token-in-process-list exposure in help text + a stderr
  note when a token is forwarded.
- Tests: POST method guard, sec-fetch navigation signal, /health not shadowed,
  sendFile 500 path, plus isDocumentNavigation and resolveWebShellDir units.

packages/cli/src/commands/serve.ts

@@ -147,7 +147,7 @@ export const serveCommand: CommandModule<unknown, ServeArgs> = {
         type: 'boolean',
         default: false,
         description:
-          'Open the Web Shell in a browser once the daemon is listening. No-op with --no-web or in headless/CI/SSH environments.',
+          'Open the Web Shell in a browser once the daemon is listening. With a token configured, the launch URL (token included) is handed to the browser launcher and is visible in the process list, so prefer opening the URL manually on multi-user hosts. No-op with --no-web, when the UI assets are absent, or in headless/CI/SSH environments.',
       })
       .option('event-ring-size', {
         type: 'number',
@@ -474,15 +474,48 @@ export const serveCommand: CommandModule<unknown, ServeArgs> = {
         ...(rateLimitRead !== undefined ? { rateLimitRead } : {}),
         ...(rateLimitWindowMs !== undefined ? { rateLimitWindowMs } : {}),
       });
-      // Open the Web Shell in a browser once the listener is up. `handle.url`
+      // Open the Web Shell in a browser once the listener is up. Gated on
-      // already reflects the resolved port (so `--port 0` works), and
+      // `handle.webShellMounted` so we never point a browser at an API-only
-      // `shouldLaunchBrowser()` suppresses this in CI / SSH / headless. Best
+      // daemon (assets absent or --no-web). `shouldLaunchBrowser()` suppresses
-      // effort: a failed launch must not take down the daemon.
+      // this in CI / SSH / headless. The whole block is wrapped in its own
-      if (argv.open && argv.web && shouldLaunchBrowser()) {
+      // try/catch: openBrowserSecurely can throw (URL validation, unsupported
-        const target = new URL(handle.url);
+      // platform), and the outer catch calls process.exit(1) — a failed
-        const browserToken = argv.token || process.env['QWEN_SERVER_TOKEN'];
+      // browser launch must NOT take down the already-listening daemon.
-        if (browserToken) target.searchParams.set('token', browserToken);
+      if (argv.open && handle.webShellMounted && shouldLaunchBrowser()) {
-        await openBrowserSecurely(target.toString());
+        try {
+          const target = new URL(handle.url);
+          // `handle.url` carries the raw bind host; a wildcard bind is not
+          // client-addressable, so send the browser to loopback instead.
+          if (
+            target.hostname === '0.0.0.0' ||
+            target.hostname === '::' ||
+            target.hostname === '[::]'
+          ) {
+            target.hostname = '127.0.0.1';
+          }
+          // Trim to match runQwenServe's own token normalization — otherwise a
+          // trailing newline from `$(cat token.txt)` makes the browser's token
+          // mismatch the server's trimmed value and 401 every API call.
+          const rawBrowserToken =
+            argv.token || process.env['QWEN_SERVER_TOKEN'];
+          const browserToken =
+            typeof rawBrowserToken === 'string' &&
+            rawBrowserToken.trim().length > 0
+              ? rawBrowserToken.trim()
+              : undefined;
+          if (browserToken) {
+            target.searchParams.set('token', browserToken);
+            writeStderrLine(
+              'qwen serve: --open passes the token in the browser launch command ' +
+                '(visible via `ps` / /proc); on a multi-user host open the URL manually instead.',
+            );
+          }
+          await openBrowserSecurely(target.toString());
+        } catch (browserErr) {
+          writeStderrLine(
+            `qwen serve: failed to open browser: ${browserErr instanceof Error ? browserErr.message : String(browserErr)}`,
+          );
+        }
       }
     } catch (err) {
       writeStderrLine(

packages/cli/src/serve/runQwenServe.ts

@@ -347,6 +347,12 @@ export interface RunHandle {
   server: Server;
   url: string;
   bridge: AcpSessionBridge;
+  /**
+   * Whether the Web Shell UI was actually mounted (assets resolved and
+   * `serveWebShell !== false`). The `--open` launcher checks this so it never
+   * points a browser at an API-only daemon.
+   */
+  webShellMounted: boolean;
   /** Resolves when the listener has fully closed and the bridge is drained. */
   close(): Promise<void>;
 }
@@ -993,14 +999,33 @@ export async function runQwenServe(
         'qwen serve: Web Shell assets not found; serving API only. ' +
           'Build the web-shell workspace (npm run build) or pass --no-web to silence this.',
       );
-    } else if (!isLoopbackBind(opts.hostname)) {
+    } else {
-      writeStderrLine(
+      // Positive happy-path breadcrumb so operators can confirm the UI is live
-        'qwen serve: Web Shell UI is served WITHOUT auth on a non-loopback ' +
+      // (the only other lines are negative-path warnings).
-          'bind (the static shell has no secrets; the API stays token-gated). ' +
+      writeStderrLine(`qwen serve: Web Shell UI served from ${webShellDir}`);
-          'Pass --no-web to disable the UI.',
+      if (!isLoopbackBind(opts.hostname)) {
-      );
+        writeStderrLine(
+          'qwen serve: Web Shell UI is served WITHOUT auth on a non-loopback ' +
+            'bind (the static shell has no secrets; the API stays token-gated). ' +
+            'Pass --no-web to disable the UI.',
+        );
+        // The shell HTML/JS loads (GET carries no Origin), but its same-origin
+        // POSTs (create session, prompt, permission vote) send an Origin the
+        // daemon's CORS wall rejects with 403 unless allow-listed — so without
+        // --allow-origin the UI is effectively read-only on a non-loopback
+        // bind. Front the daemon with a same-origin reverse proxy, or pass
+        // --allow-origin <origin>, to make mutations work.
+        if (!opts.allowOrigins || opts.allowOrigins.length === 0) {
+          writeStderrLine(
+            'qwen serve: without --allow-origin the Web Shell is read-only on a ' +
+              'non-loopback bind — same-origin POSTs are blocked by CORS (403). ' +
+              'Pass --allow-origin <origin> or front it with a same-origin proxy.',
+          );
+        }
+      }
     }
   }
+  const webShellMounted = !!webShellDir && opts.serveWebShell !== false;
 
   // Pass the already-canonical `boundWorkspace` into `createServeApp`
   // via `deps.boundWorkspace`. That field is the pre-canonicalized
@@ -1227,6 +1252,7 @@ export async function runQwenServe(
         server,
         url,
         bridge,
+        webShellMounted,
         close: () => {
           // Idempotent: cache the in-flight (or settled) close promise so
           // overlapping calls (e.g. test harness + signal handler firing

packages/cli/src/serve/server.test.ts

@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { realpathSync, promises as fsp } from 'node:fs';
+import { existsSync, realpathSync, promises as fsp } from 'node:fs';
 import type { ServerResponse } from 'node:http';
 import * as os from 'node:os';
 import * as path from 'node:path';
@@ -19,6 +19,7 @@ import {
   resolvePromptDeadlineMs,
 } from './server.js';
 import { runQwenServe, type RunHandle } from './runQwenServe.js';
+import { resolveWebShellDir, isDocumentNavigation } from './webShellStatic.js';
 import {
   CONDITIONAL_SERVE_FEATURES,
   getAdvertisedServeFeatures,
@@ -1703,6 +1704,68 @@ describe('createServeApp', () => {
         .set('Accept', 'text/html');
       expect(res.text).not.toContain('<div id="root">');
     });
+
+    it('does not serve the shell for POST navigations (method guard)', async () => {
+      const app = createServeApp(baseOpts, undefined, { webShellDir });
+      const res = await request(app)
+        .post('/session/abc')
+        .set('Host', host)
+        .set('Accept', 'text/html');
+      expect(res.text).not.toContain('<div id="root">');
+    });
+
+    it('falls back to the shell on a sec-fetch navigation signal', async () => {
+      const app = createServeApp(baseOpts, undefined, { webShellDir });
+      const res = await request(app)
+        .get('/session/deep')
+        .set('Host', host)
+        .set('Accept', '*/*')
+        .set('Sec-Fetch-Mode', 'navigate');
+      expect(res.status).toBe(200);
+      expect(res.text).toContain('<div id="root">');
+    });
+
+    it('does not shadow /health on a browser navigation (Critical #1)', async () => {
+      // Non-loopback + requireAuth registers /health POST-auth. A browser
+      // navigation (Accept text/html) must fall THROUGH the SPA fallback to
+      // bearerAuth (401), not receive the shell. Without the /health guard the
+      // pre-auth fallback would return index.html instead.
+      const app = createServeApp(
+        { ...baseOpts, hostname: '0.0.0.0', token: 'secret', requireAuth: true },
+        undefined,
+        { webShellDir },
+      );
+      const res = await request(app)
+        .get('/health')
+        .set('Host', '0.0.0.0:4170')
+        .set('Accept', 'text/html');
+      expect(res.text).not.toContain('<div id="root">');
+    });
+
+    it('returns 500 when index.html is unreadable after mount', async () => {
+      const app = createServeApp(baseOpts, undefined, { webShellDir });
+      await fsp.rm(path.join(webShellDir, 'index.html'));
+      const res = await request(app).get('/').set('Host', host);
+      expect(res.status).toBe(500);
+    });
+
+    it('isDocumentNavigation recognizes each navigation signal', () => {
+      const nav = (headers: Record<string, string>) =>
+        isDocumentNavigation({ headers } as never);
+      expect(nav({ 'sec-fetch-mode': 'navigate' })).toBe(true);
+      expect(nav({ 'sec-fetch-dest': 'document' })).toBe(true);
+      expect(nav({ accept: 'text/html,application/xhtml+xml' })).toBe(true);
+      expect(nav({ accept: 'application/json' })).toBe(false);
+      expect(nav({})).toBe(false);
+    });
+
+    it('resolveWebShellDir returns undefined or a dir with index.html + assets', () => {
+      const dir = resolveWebShellDir();
+      if (dir !== undefined) {
+        expect(existsSync(path.join(dir, 'index.html'))).toBe(true);
+        expect(existsSync(path.join(dir, 'assets'))).toBe(true);
+      }
+    });
   });
 
   describe('GET /health', () => {

packages/cli/src/serve/webShellStatic.ts

@@ -53,7 +53,16 @@ export function resolveWebShellDir(): string | undefined {
     path.resolve(selfDir, '..', '..', '..', 'web-shell', 'dist'),
   ];
   for (const dir of candidates) {
-    if (existsSync(path.join(dir, 'index.html'))) return dir;
+    // Require BOTH index.html and assets/: a partial build (index.html
+    // without its hashed chunks) would otherwise pass and serve a shell
+    // whose every script/style 404s. copy_bundle_assets.js applies the same
+    // two-part check before copying.
+    if (
+      existsSync(path.join(dir, 'index.html')) &&
+      existsSync(path.join(dir, 'assets'))
+    ) {
+      return dir;
+    }
   }
   return undefined;
 }
@@ -136,6 +145,13 @@ export function registerWebShell(app: Application, webShellDir: string): void {
   app.use((req: Request, res: Response, next: NextFunction) => {
     if (req.method !== 'GET' && req.method !== 'HEAD') return next();
     if (!isDocumentNavigation(req)) return next();
+    // Don't shadow the daemon's own browser-reachable endpoints. On
+    // non-loopback binds /health and /demo are registered AFTER bearerAuth
+    // (i.e. after this pre-auth fallback), so without this guard a browser
+    // navigation to them would receive index.html instead of their real
+    // response. API routes send Accept: application/json and already fail
+    // isDocumentNavigation, so they need no listing here.
+    if (req.path === '/health' || req.path === '/demo') return next();
     sendIndex(res);
   });
 }