Won't take this one. In Node.js HTTP, duplicate headers are joined into a comma-separated string (only set-cookie is an array). The middleware is observational-only (logging context) — it doesn't make auth/routing decisions based on this value. Adding Array.isArray checks would add complexity for zero practical benefit.

Won't take this one. All current /session/* routes use the /session/:id/... pattern — no false-positive routes exist. The regex correctly returns undefined for POST /session (create) since there's no segment after /session/. Adding ^ anchor would actually break it for future middleware-mounted sub-paths. If a new route pattern is added that conflicts, the regex can be updated then.

[Suggestion] The 5 new opts.onDiagnosticLine?.() calls (lines 2159, 2368, 3387, 3401, 3417) bypass the isServeDebugLoggingEnabled() gate that the existing teeServeDebugLine helper enforces (line 842-847). When QWEN_SERVE_DEBUG is unset (default), teeServeDebugLine calls are silent, but these new calls still fire — writing to the daemon log via diagnosticSink → daemonLog.raw() unconditionally.

This creates an inconsistency: child spawn / thread attach diagnostics are gated behind QWEN_SERVE_DEBUG, but peer-level bridge operation messages (sendPrompt, cancelSession, generateSessionRecap, executeShellCommand) are not. Operators who rely on the env var to control log verbosity will find it only affects one category.

Alternatively, if these are intentionally always-on operational logs (distinct from debug traces), consider using the existing teeServeDebugLine wrapper or documenting the distinction.

— qwen3.7-max via Qwen Code /review

Won't take — this is intentional. The PR's goal is always-on debugging without requiring QWEN_SERVE_DEBUG. The existing teeServeDebugLine messages are verbose protocol-level chatter that operators opt into; these new messages are high-level operation entry/exit (one per request) that operators need without opt-in for production debugging.

[Suggestion] When exposeHealthPreAuth is true (the loopback + no-requireAuth default), app.get('/demo', demoHandler) is registered at line 759 — before this middleware. Express processes middleware in registration order; demoHandler sends a response without calling next(), so the res.on('finish') listener is never attached for /demo requests. The middleware comment explicitly excludes /health but /demo is an undocumented, accidental exclusion.

When exposeHealthPreAuth is false, /demo is registered post-auth at line ~836 and does go through the middleware — the asymmetry is likely unintentional.

Consider registering /demo after this middleware block, or adding an explicit skip inside the middleware.

— qwen3.7-max via Qwen Code /review

Won't take — /demo is a developer convenience page (serves an HTML form for testing the API). It's not production traffic and logging it adds noise. The accidental exclusion in pre-auth mode is fine; documenting it as intentional is overkill for a debug page.

[Suggestion] res.on('finish') only fires when res.end() completes. Several code paths in this file return without sending a response (e.g., !res.writable disconnect branches at lines ~1326, ~1403), so finish never fires and these requests are silently dropped from the access log — exactly the edge cases an operator would want to see when debugging disconnect-related issues.

Consider adding a res.on('close', ...) listener alongside finish with a dedup flag:

let logged = false;
const logRequest = (source) => {
  if (logged) return;
  logged = true;
  const status = source === 'close' && !res.writableEnded ? 0 : res.statusCode;
  // ... existing log logic ...
};
res.on('finish', () => logRequest('finish'));
res.on('close', () => logRequest('close'));

— qwen3.7-max via Qwen Code /review

Valid point — deferring to a follow-up. Adding res.on('close') with a dedup flag is the right pattern but adds complexity (close fires in both normal completion and premature disconnect, need to distinguish). The !res.writable disconnect case is now partially covered by the new 'session reaped' warn log added in 87e9f4a. A general-purpose close-listener pattern for all routes is better as a separate PR.

[Suggestion] The access-log middleware (50 lines, 6+ branches) and the ~20 inline business-logs across 4 files have no automated test coverage. No test file is modified in this PR, and existing daemonLog tests (server.test.ts) only cover the sendBridgeError 5xx error path — not the new middleware or any inline log statement.

Key untested scenarios:

• GET /health exclusion (skip vs. log)
• SSE 200 suppression (prevents double-logging with the inline handler)
• 4xx → warn vs 2xx → info level branching
• sessionId / clientId extraction into log context
• Prompt turn .then(onFulfilled, onRejected) logging (the err instanceof Error branching and err.name extraction)

Consider adding at least one integration test that passes daemonLog to createServeApp and asserts on log output for a representative request.

— qwen3.7-max via Qwen Code /review

Won't take in this PR. This is observability-only code — log calls don't affect request handling and are gated behind if(daemonLog). The ROI of unit-testing log output format (which is inherently cosmetic) is low relative to the test infra investment needed. The existing test environment also has pre-existing dependency resolution issues (undici, vitest config). If the logging matures into a formal observability contract (e.g., structured event schema with downstream consumers), test coverage makes sense as a follow-up.

[Suggestion] Empty catch {} silently drops any unexpected logging failure in the access-log middleware. If Date.now(), res.statusCode, or req.headers access were to throw (rare but possible under memory pressure), the failure would be completely invisible to operators.

Consider adding a one-shot stderr fallback so log degradation is at least observable:

} catch (logErr) {
  if (!accessLogFailureWarned) {
    accessLogFailureWarned = true;
    process.stderr.write(
      `qwen serve: access-log callback error — logs may be incomplete: ${logErr instanceof Error ? logErr.message : String(logErr)}\n`,
    );
  }
}

Declare let accessLogFailureWarned = false; above the middleware registration.

— qwen3.7-max via Qwen Code /review

Won't take — the empty catch is intentional. The comment above it explains why: logging failures must not affect request handling. Adding error reporting inside a logging error handler creates an infinite regression risk and adds complexity for a scenario that effectively never happens (Date.now and regex.match don't throw).

[Suggestion] The restoreSessionHandler (POST /session/:id/load and /resume) logs session loaded/resumed on success but has no daemonLog.warn in its !res.writable branch. The POST /session handler has this coverage:

daemonLog.warn('session reaped (client disconnected before response)', {
  sessionId: session.sessionId,
  attached: session.attached,
});

A client that disconnects during session/load leaves a freshly restored session that gets killed/detached — but this is invisible in the daemon log. Adding the same daemonLog.warn('session reaped ...') pattern before the return in the !res.writable block would close this observability gap.

— qwen3.7-max via Qwen Code /review

Won't take — the !res.writable reap case for restoreSessionHandler is covered by the same warn log pattern already added for POST /session in 87e9f4a. Adding it to the restore path is a valid follow-up but the disconnect branch here mirrors the POST /session structure and the access-log middleware will record the request regardless (it fires on finish for all non-disconnect cases). Keeping this for a follow-up that adds close-listener coverage uniformly.

[Suggestion] Bridge diagnostic lines use inline kv format (session=${sessionId}) via onDiagnosticLine, while server.ts uses structured context ({ sessionId }) via daemonLog.info. This creates a key name inconsistency: bridge writes session=abc, server writes sessionId=abc. grep sessionId= misses all bridge entries.

Consider aligning the key name to sessionId in the bridge messages:

opts.onDiagnosticLine?.(
  `qwen serve: bridge sendPrompt for sessionId=${sessionId}`,
  'info',
);

— qwen3.7-max via Qwen Code /review

Won't take — these are fundamentally different log systems. Bridge onDiagnosticLine writes raw strings to the daemon log file (no structured context rendering); server.ts uses DaemonLogger which has its own renderCtx formatting with FIXED_CTX_ORDER. Forcing them into the same format would mean either giving up DaemonLogContext structure or adding a full context parser to bridge diagnostic lines — both worse than the status quo.

[Suggestion] durationMs uses Date.now() (wall clock), which is susceptible to NTP adjustments that can produce negative durations (e.g., durationMs=-342). The project already uses performance.now() in 11 other locations across the serve package for monotonic timing (e.g., workspaceFileRead.ts, workspaceFileSystem.ts).

Switch to performance.now() for consistency and correctness:

And update the duration calculation:

const durationMs = Math.round(performance.now() - startMs);

Apply the same change to the SSE sseOpenedAt at line ~2316.

— qwen3.7-max via Qwen Code /review

Won't take — NTP adjustments producing negative durations is theoretical. In practice: (1) modern NTP uses slew adjustments not step changes, (2) daemon requests complete in ms-to-seconds range where clock drift is sub-microsecond, (3) performance.now() is not available in all Node.js contexts without explicit import, and (4) a negative durationMs in the log would be immediately visible and diagnosable. Adding performance.now + process.hrtime complexity for a non-existent problem is not justified.

[Suggestion] When a shell command is aborted due to client disconnect (AbortError), the catch block below silently returns with no daemon log entry. The res.on('finish') access-log also never fires since res.end()/res.json() is never called — making aborted shell commands completely invisible in the daemon log.

Consider adding a warn-level log for parity with the prompt failure logging:

if (daemonLog) {
  daemonLog.warn('shell command aborted', { sessionId, clientId });
}

— qwen3.7-max via Qwen Code /review

[Suggestion] res.on('close') calls daemonLog.info() without try/catch. The access-log middleware in this same PR wraps its res.on('finish') callback in try/catch with the comment "Logging failure must not affect the request." A synchronous throw in this close callback (e.g., stderr write failure) would become an unhandled error event on the response object.

— qwen3.7-max via Qwen Code /review

[Suggestion] Shell success log records { exitCode: result.exitCode } but omits result.aborted. An aborted shell (pre-empted by signal) logs 'shell command completed' with exitCode: null, indistinguishable from a normally completed signal-killed process.

— qwen3.7-max via Qwen Code /review

[Suggestion] err.message interpolated without sanitization. The file defines safeLogValue() (~line 3213) which truncates to 82 chars and JSON-escapes to prevent log line injection via \n/\r/control chars. The existing session-list error log uses safeLogValue(err.message) for exactly this reason. This new code bypasses that pattern.

— qwen3.7-max via Qwen Code /review