📋 Review Summary

This PR adds HTTP endpoint support for /btw (side question) in daemon mode, following the established recap pattern. The implementation is well-structured with proper timeout handling, abort signal propagation, and cache-safe parameter extraction. All tests pass (359 total) and type-check succeeds.

🔍 General Feedback

• Strong architectural consistency: The implementation follows the established "recap pattern" for ext-method control-plane operations, maintaining consistency with existing codebase patterns
• Good timeout layering: Bridge-side 60s timeout races against ACP child 55s self-guard — appropriate staggered timeout design
• Clean separation of concerns: buildBtwPrompt and buildBtwCacheSafeParams extracted to packages/core for shared use between CLI and ACP bridge
• Comprehensive test coverage: New ACP mode tests cover success, error, and fire-and-forget prevention scenarios
• Graceful degradation: Returns { answer: null } when no conversation context exists rather than throwing 500 errors

🎯 Specific Feedback

🟡 High

• packages/cli/src/serve/server.ts:1840 - The onResClose handler removes itself in the clientId === null early return path, but the abort.abort() cleanup only happens in the finally block. If parseClientIdHeader returns null, the abort controller is never cleaned up. While low-impact (no leak), consider moving res.off('close', onResClose) into the finally block for consistency with the error handling paths.

• packages/core/src/utils/btwUtils.ts:27-48 - The buildBtwCacheSafeParams function silently catches all errors and returns null. This is appropriate for expected failures (e.g., no Gemini client), but may hide unexpected bugs. Consider logging debug information in the catch block or narrowing the caught error types:

  } catch (err) {
    // Expected: getChat/getHistory/getGenerationConfig may not exist
    // Unexpected errors are suppressed here - consider debug logging
    return null;
  }

🟢 Medium

• packages/cli/src/ui/commands/btwCommand.ts:29-34 - The getBtwCacheSafeParams function now delegates to buildBtwCacheSafeParams(config) with a fallback to getCacheSafeParams(). This is cleaner than the original inline implementation, but the function could be renamed to reflect its new role (e.g., resolveBtwCacheParams) since it no longer "gets" params directly but rather "resolves" them from multiple sources.

• packages/acp-bridge/src/bridge.ts:3401 - The generateSessionBtw implementation passes _context (unused parameter). Consider removing the underscore prefix since the parameter is genuinely unused here, or document why it's kept for interface compatibility.

• packages/cli/src/acp-integration/acpAgent.ts:2571 - The timeout is hardcoded as AbortSignal.timeout(55_000) which is slightly less than the bridge's 60s timeout. This is correct design, but consider extracting this as a named constant (e.g., ACP_BTW_TIMEOUT_MS = 55_000) near the import statements for clarity and maintainability.

🔵 Low

• packages/cli/src/ui/commands/btwCommand.test.ts:179-199 - The test "should fall back to getCacheSafeParams when buildBtwCacheSafeParams returns null" verifies the fallback behavior, but doesn't assert that mockBuildBtwCacheSafeParams was called. Consider adding expect(mockBuildBtwCacheSafeParams).toHaveBeenCalled() before the mockRunForkedAgent assertion to make the test's intent more explicit.

• packages/cli/src/serve/capabilities.ts:177 - The capability comment mentions "Single-turn, tool-free LLM call via runForkedAgent (cache path)" which is implementation detail. Consider simplifying to just describe the feature: "Side question (/btw) against the session's conversation context."

• packages/core/src/utils/btwUtils.ts:10 - The buildBtwPrompt function uses a system-reminder pattern. Consider extracting the constraint bullets into a constant array for easier maintenance if these constraints evolve over time.

✅ Highlights

• Excellent timeout design: The 60s bridge timeout racing against 55s ACP timeout with client-disconnect AbortSignal shows thoughtful defensive programming
• Clean refactoring: Extracting buildBtwPrompt and buildBtwCacheSafeParams to core utilities demonstrates good code organization and reusability
• Proper abort handling: The HTTP endpoint correctly propagates abort signals and handles client disconnects without logging errors
• Comprehensive test additions: The new ACP mode tests (success, error, no fire-and-forget) show thorough coverage of the new execution path
• Backward compatibility: Expanding supportedModes to include 'acp' while maintaining interactive mode behavior is a clean, non-breaking extension

Review round 1 — wenshao

All 5 suggestions addressed in 90d9f3109:

All 361 tests pass. Regarding the typecheck note in the review summary — this is a pre-existing lint issue in server.test.ts (lines 1040-1099, not in our diff). Build and all tests pass cleanly.

Review round 2 — wenshao

4/5 suggestions fixed in 2799acdbe, 1 pushed back:

All 361 tests pass.

Review round — addressed wenshao's /review findings (347ed6c9)

Both Critical issues are fixed and pushed. The four Suggestion threads are left open for your input rather than unilaterally resolved.

Verification Report — PR #4610

Commit: 347ed6c91 (feat(daemon): add POST /session/:id/btw endpoint for side questions)
Base: daemon_mode_b_main
Tester: wenshao
Date: 2026-05-30

Test Results

Test Details

TypeScript Error Analysis

Pre-existing Issues (not caused by this PR)

• server.test.ts cannot run due to missing vitest alias for @qwen-code/acp-bridge/bridge subpath on daemon_mode_b_main. This is the same infrastructure gap as other PRs targeting this base branch.

Items Not Verified (require live infrastructure)

• qwen serve → create session → POST /session/:id/btw → verify 200 response
• GET /session/:id/supported-commands includes btw
• Client disconnect aborts cleanly (no error logs)

Verdict

✅ All automatable checks pass. 352 tests total (22 btw + 330 acp-bridge), all 3 package typechecks clean (PR actually reduces CLI TS errors by 7). Safe to merge pending live endpoint verification.