Thanks for the PR, @doudouOUC!

Template looks good ✓ — all required sections present, bilingual description, test plan included.

On direction: This is T3.2 from the tracked daemon capability gaps (#4514) — /rewind is currently TUI-only and web-shell/SDK clients genuinely have no way to rewind. Aligned with the session-management roadmap. The daemon_mode_b_main base branch is correct for this work. No concerns here.

On approach: The scope feels right and well-contained. Two HTTP endpoints (list snapshots + execute rewind), structured error classes (409 busy, 400 invalid target), SSE cross-client notification, and SDK plumbing — this mirrors the pattern used by other daemon endpoints (/recap, /btw, /branch). The backward-compatible rewindSession extMethod extension is a good call for existing VSCode consumers.

One observation: the Promise.all over up to 100 snapshots for diff stats (noted in the risk section) is fine for MVP but worth keeping on the radar. The supportedModes: ['interactive'] addition to rewindCommand.ts is a small but important detail — it correctly marks /rewind as interactive-only in the command registry.

No tests included in the 14 changed files — I'll check whether existing test coverage is sufficient during code review.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

📋 Review Summary

This PR adds HTTP rewind endpoints to the daemon, enabling web-shell and SDK clients to rewind a session's conversation and files to a previous turn—previously only available via TUI interactive dialog. The implementation is well-structured, extending the existing rewindSession ACP extMethod with a promptId parameter and leveraging FileHistoryService for file restoration. The error handling is robust with new SessionBusyError (409) and InvalidRewindTargetError (400) classes, and cross-client notification via session_rewound SSE event is a nice touch.

🔍 General Feedback

• Architecture: Clean separation of concerns—HTTP endpoints in server.ts delegate to bridge methods in acp-bridge, which forward to ACP agent handlers. The layered approach maintains consistency with existing patterns.
• Type Safety: Comprehensive type definitions across bridgeTypes.ts, SDK types.ts, and events.ts ensure type-safe communication between layers.
• Error Handling: Structured error taxonomy with SessionBusyError and InvalidRewindTargetError follows the established pattern in bridgeErrors.ts, enabling precise HTTP status code mapping.
• Backward Compatibility: The rewindSession extMethod supports both legacy targetTurnIndex and new promptId parameters, preserving compatibility with existing VSCode extension callers.
• Event Broadcasting: The session_rewound SSE event with originatorClientId tracking enables multi-client awareness—clients can update their UI when another client triggers a rewind.

🎯 Specific Feedback

🟡 High

• packages/cli/src/acp-integration/acpAgent.ts:2153-2177 — The getRewindSnapshots handler calls getDiffStats for all snapshots in Promise.all without pagination or limiting. The PR body acknowledges this ("may need pagination for long sessions"), but for MVP, consider at least limiting to the last 100 snapshots to prevent memory/CPU spikes on long-running sessions:

  const results = await Promise.all(
    snapshots
      .slice(0, 100) // Add limit to prevent resource exhaustion
      .filter(...)
      .map(...)
  );

• packages/acp-bridge/src/bridge.ts:3530-3536 — The session_rewound event is published without awaiting or error handling beyond the try-catch that swallows "bus closed". While the swallow is intentional, consider logging when the event bus is unexpectedly closed during a successful rewind for observability:

  } catch (err) {
    // Log for debugging: event bus closed during rewind notification
    debugLogger?.('session_rewound event publish failed: %s', err);
  }

🟢 Medium

• packages/cli/src/serve/server.ts:2038-2043 — The POST /session/:id/rewind endpoint validates promptId but doesn't validate the format (e.g., contains the ######## separator). While the agent-side handler validates this, adding early validation at the HTTP boundary provides faster feedback:

  if (!promptId.includes('########')) {
    res.status(400).json({
      error: '`promptId` must be in format: <sessionId>########<turnIndex>',
      code: 'invalid_prompt_id_format',
    });
    return;
  }

• packages/cli/src/acp-integration/acpAgent.ts:2957-2962 — The file rewind failure is silently swallowed with a comment "File history snapshot may not exist — non-fatal". While non-fatal, consider populating filesFailed with a sentinel value or logging when the snapshot is missing, so clients know file restoration was skipped:

  } catch (err) {
    // Log missing snapshot for observability
    debugLogger?.('FileHistoryService.rewind failed: %s', err);
    filesFailed.push('[snapshot unavailable]');
  }

• packages/sdk-typescript/src/daemon/DaemonClient.ts:1158-1178 — The rewindSession method accepts an optional clientId, but the DaemonSessionClient.rewind method (line 297-300 in DaemonSessionClient.ts) hardcodes this.clientId. Consider documenting whether callers should ever pass a custom clientId or if the optional parameter should be removed from the public API.

🔵 Low

• packages/acp-bridge/src/bridgeErrors.ts:435 — The InvalidRewindTargetError message says "compressed or does not exist". Consider clarifying what "compressed" means in this context (e.g., "pruned due to age" or "skipped turn") for better client-facing error messages.

• packages/cli/src/ui/commands/rewindCommand.ts:17 — The supportedModes: ['interactive'] addition is correct but worth noting that this command now has dual paths: TUI dialog for interactive mode, HTTP endpoints for daemon mode. Consider adding a comment linking to the HTTP endpoints for discoverability.

• packages/sdk-typescript/src/daemon/events.ts:1173-1183 — The isSessionRewoundData validator checks filesChanged is an array but doesn't validate element types. While TypeScript provides compile-time safety, adding runtime validation (every(item => typeof item === 'string')) would catch malformed SSE events from buggy daemon versions.

✅ Highlights

• Error Kind Propagation: The mapping from agent-side RequestError with errorKind ('session_busy', 'invalid_rewind_target') to bridge-level typed errors (SessionBusyError, InvalidRewindTargetError) to HTTP status codes (409, 400) is exemplary—each layer preserves structured error semantics without text-matching.

• PromptId Format Design: Using <sessionId>########<turnIndex> as a composite key is clever—it ties snapshots to their session namespace while preserving turn ordering. The 1-based to 0-based conversion logic (line 2925-2927) is well-commented.

• SSE Event Design: The session_rewound event includes originatorClientId, enabling multi-client setups to distinguish "I triggered this rewind" from "another client rewound the session". This is critical for collaborative debugging scenarios.

• Backward Compatibility: Supporting both targetTurnIndex (legacy) and promptId (new) in the same handler without breaking existing VSCode extension callers demonstrates thoughtful API evolution.

• Retry-After Header: The SessionBusyError response includes Retry-After: 5 header, preventing clients from hammering the endpoint while a prompt is running. This is consistent with RestoreInProgressError handling.

Copilot review response summary

wenshao review response summary (8a7a113)

Local Test Report — PR #4820

Branch: feat/serve-rewind-endpoint-v2 → daemon_mode_b_main
Environment: macOS Darwin 25.4.0 / Node v22.17.0 / npm 10.9.2

Summary

PR-Caused Issues (2)

1. SDK browser bundle size exceeded

Error: Browser daemon SDK bundle is 109280 bytes; expected <= 108544

The new rewind types and client methods (getRewindSnapshots, rewindSession, session_rewound SSE event types) push the browser bundle over the 106 KiB limit (108544 bytes). Actual size is 109280 bytes — 736 bytes over limit. The limit was bumped from 106 → 108 KiB in a separate recent PR (#4812), but this PR is based before that change. Options: rebase onto latest daemon_mode_b_main (which now includes the 108 KiB bump) and verify the new ceiling accommodates the rewind additions, or bump the limit further if needed.

2. Capability registry tests (3 failures)
Three createServeApp > serve capability registry tests fail because session_rewind is not included in the expected feature list arrays:

• returns a fresh ordered registered feature list
• advertises current-protocol features separately from the registry
• marks every current feature with its historical v1 origin

The PR adds session_rewind: { since: 'v1' } to capabilities.ts but does not update the corresponding test assertions in server.test.ts.

Pre-Existing Issues

acpAgent.test.ts — 2 failures (known on daemon_mode_b_main):

• status ext methods > status returns session data for known session
• status ext methods > status returns null for unknown session
  Both fail due to Cannot read properties of undefined (reading 'catch') in acpAgent.ts:316 (shutdown handler teardown race).

server.test.ts — 9 runQwenServe afterEach timeouts (known flaky on daemon_mode_b_main):
All timeout at 10s in afterEach cleanup hook — these are infrastructure timeouts unrelated to this PR's changes:

• accepts QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS above JS timer cap
• does not mutate process.env when caller provides mcp budget options
• preserves pre-existing process.env values
• accepts QWEN_SERVER_TOKEN from the env when binding non-loopback
• --max-connections Infinity treated as unlimited
• --max-connections 100 sets the cap as supplied
• case-insensitive loopback: --hostname Localhost / LOCALHOST does NOT require a token
• strips brackets from [::1] before passing to app.listen()
• rejects unbracketed host:port typo with a useful error

Verdict

Not merge-ready. Two actionable items must be addressed:

1. Rebase or bump SDK browser bundle size limit to accommodate rewind additions
2. Add session_rewind to capability registry test expected arrays

Tested by wenshao

Thanks for the thorough local test report.

Issue 1 (SDK bundle size): Will rebase onto latest daemon_mode_b_main which includes the 108 KiB bump from #4812.

Issue 2 (capability registry tests): Already fixed in commit 5f73d06 — session_rewind added to both EXPECTED_STAGE1_FEATURES and EXPECTED_REGISTERED_FEATURES in server.test.ts.

Pre-existing failures: Confirmed — the 2 acpAgent teardown races and 9 runQwenServe afterEach timeouts are known on daemon_mode_b_main and unrelated to this PR.

✅ Local Verification Report — PR #4820

feat(serve): add HTTP rewind endpoints for daemon/web-shell (issue #4514 T3.2)

Environment

• macOS Darwin 25.4.0, Node.js v22.17.0, npm workspaces monorepo
• Branch: feat/serve-rewind-endpoint-v2 → daemon_mode_b_main
• Verified commits up to: bb69139 (debugLogger + deduplicate response extraction)

Build Verification

Test Results

Capability Registry Count Drift (Minor Issue)

3 server.test.ts capability-registry tests fail: actual 53 features vs expected 50. The test arrays include session_rewind (added by this PR), but the expected count is 3 behind because other PRs merged to daemon_mode_b_main since this branch's rebase added capabilities not reflected in the test's expected list. Recommend rebasing onto latest daemon_mode_b_main and updating the expected feature arrays.

Pre-existing Failures (NOT introduced by this PR)

Review Issues Status

Code Review Summary

• HTTP endpoints: Clean GET /session/:id/rewind/snapshots + POST /session/:id/rewind with promptId validation, proper sendBridgeError mapping
• Bridge: rewindSession() with SessionBusyError (409) + InvalidRewindTargetError (400) error classes, session_rewound SSE event with filesFailed
• ACP handler: promptId→turnIndex resolution via snapshot array position (not suffix parsing), SESSION_ID_RE validation, debugLogger.error() on file-rewind failure
• SDK: DaemonSessionRewoundData type with filesFailed, isSessionRewoundData type guard, getRewindSnapshots()/rewindSession() client methods

Verdict

✅ PASS — Core rewind functionality works (4/4 acpAgent tests pass), all Critical review issues addressed, bridge + SDK types consistent. Minor issue: capability-registry test count is 3 behind (needs rebase). Pre-existing failures only in unrelated areas.

Verified by wenshao