feat(serve+sdk): F4 prereq — daemon protocol completion (serverTimestamp / provenance / errorKind / state_resync_required)

[doudouOUC]

Summary

Two F4 prerequisites bundled into one PR against daemon_mode_b_main. Both are protocol-level daemon completions that the F4 client-adapter wave needs in place before it can render correctly:

1. Error after selecting authentication method: "Cannot read properties of undefined (reading 'value')" #19 stamping (14637cd79) — Three daemon-side stamping additions chiga0 called out in proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175 comment Error after selecting authentication method: "Cannot read properties of undefined (reading 'value')" #19. SDK side (PR feat(sdk/daemon-ui): unified completeness follow-up to #4328 (PR-A through PR-E) #4353) is forward-compat ready; we just need daemon to start emitting these fields. UI affordances are inert without them.

2. 运行不了 #15 SSE reducer gap detection (c1a2f0a78) — Closes the multi-client state divergence bug Ilya0527 raised in proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175 comment 运行不了 #15. When an SSE consumer reconnects with Last-Event-ID past the ring eviction point, daemon now emits state_resync_required so the SDK reducer can refuse to apply stale deltas until consumer recovery.

Why bundle: both are small (~80 + ~150 LOC), both touch eventBus / SSE envelope area, both are F4 prerequisites. Single review unit is more efficient than two parallel PRs. Each ships as its own commit so review can step through them independently.

Commit 1 — 14637cd79 (#19 stamping)

Files

• packages/cli/src/serve/server.ts — formatSseFrame() stamps _meta.serverTimestamp at the SSE write boundary. Preserves pre-existing _meta keys (e.g. tool_call's _meta.toolName) via spread merge.
• packages/cli/src/serve/server.ts — stream_error emit stamps errorKind via mapDomainErrorToErrorKind(err) when classifiable. undefined for unclassified errors → field omitted (strictly additive).
• packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.ts — new static resolveToolProvenance(toolName, subagentMeta) → { provenance: 'builtin' | 'mcp' | 'subagent'; serverId? }. Stamped on emitStart AND emitResult AND emitError (reconnecting clients can re-derive provenance from any update event).

Design decisions (confirmed via AskUserQuestion before coding)

#	Choice	Why
1	serverTimestamp stamped at formatSseFrame boundary	SSE-only; doesn't pollute in-memory BridgeEvent type. SDK reads via _meta.serverTimestamp (Anthropic convention).
2	Provenance via mcp__<server>__<tool> naming heuristic	Matches core MCP tool naming + SDK's own fallback in PR #4353. No new ctx-dep on emit hot path.

NOT stamped: session_died errorKind

The 3 session_died emit sites in acp-bridge/bridge.ts don't have a classifiable err in scope (channel_closed carries exitCode/signalCode only; killed + daemon_shutdown are user/operator-initiated, not domain errors). Threading channel-spawn errors through to enable errorKind: 'init_timeout' / missing_binary classification is left for a separate PR to avoid mixing protocol stamping with lifecycle plumbing.

Commit 2 — c1a2f0a78 (#15 gap detection)

Daemon side (packages/acp-bridge/src/eventBus.ts)

In subscribe()'s replay path, detect ring eviction by comparing this.ring[0]?.id against lastEventId + 1. When earliest > last + 1, force-push a synthetic terminal frame:

{ v: 1, type: 'state_resync_required',
  data: { reason: 'ring_evicted',
          lastDeliveredId: N,
          earliestAvailableId: M } }

The frame has NO id (mirrors client_evicted synthetic terminal pattern). Replay continues after it — adapters get the option to compute a "what you missed" diff later; the SDK reducer auto-skips stale deltas regardless.

SDK side (packages/sdk-typescript/src/daemon/events.ts)

Adds the event type to DAEMON_EVENT_TYPES, the data interface + event type + predicate, widens DaemonStreamLifecycleEvent, adds reducer state fields (awaitingResync / resyncRequiredCount / lastResyncRequired?), and gates non-terminal events at the top of the reducer when awaitingResync === true. The RESYNC_PASSTHROUGH_TYPES set documents which event types still apply (terminal lifecycle frames must, so a session that dies during resync limbo is observable).

Design decisions (confirmed via AskUserQuestion before coding)

#	Choice	Why
3	Continue replay after state_resync_required	Network-friendly (no reconnect). SDK has option to compute diff later.
4	SDK reducer auto-skip delta when awaitingResync	Safer default. UI doesn't render against known-stale state. Consumer recovers explicitly via loadSession + createDaemonSessionViewState.

Consumer recovery contract

When state.awaitingResync === true, consumer code calls loadSession (out of band — the SDK doesn't see that as an event), then reconstructs view state via createDaemonSessionViewState({...seedFromLoadSession}). The fresh state defaults awaitingResync: false so seeding implicitly clears the flag.

Test plan

• npx vitest run packages/cli/src/serve/server.test.ts — +3 new tests (serverTimestamp + stream_error errorKind)
• npx vitest run packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.test.ts — 46 tests pass (+11 new: resolveToolProvenance static + provenance stamping on all 3 emit paths)
• npx vitest run packages/cli/src/acp-integration/session/HistoryReplayer.test.ts — 17 tests pass (provenance flows through replay)
• cd packages/acp-bridge && npx vitest run — 6 files, 108/108 pass (+5 new resync detection tests)
• cd packages/sdk-typescript && npx vitest run — 13 files, 451/451 pass (+8 new reducer resync tests)
• Pre-commit hook (prettier + eslint --max-warnings 0) clean on all 11 modified files
• CI green across Linux / macOS / Windows
• Smoke: qwen serve + curl SSE → verify _meta.serverTimestamp on every frame; trigger an error → verify errorKind; force ring eviction → verify state_resync_required frame

Backward compat

• All daemon stampings are additive. Old SDKs ignore _meta.serverTimestamp / provenance / serverId / errorKind fields. New SDKs read them.
• state_resync_required is a NEW event type; daemons predating this PR never emit it. SDK reducer's switch fall-through ignores unknown types (per existing unrecognizedKnownEventCount semantics).
• DaemonStreamErrorData.errorKind? is optional — old daemons that don't stamp it remain valid.
• DaemonSessionViewState widened with awaitingResync / resyncRequiredCount / lastResyncRequired? — consumer reseeding via createDaemonSessionViewState() (no args) defaults awaitingResync: false, so adapters that never saw a resync event behave identically.

Refs

• F1: feat(acp-bridge): F1 — acp-bridge package self-sufficiency (#4175 mechanical lift + BridgeFileSystem seam) #4319 ✅ merged; F1 follow-up: feat(serve): F1 follow-up — BridgeFileSystem wiring + #4325 channelInfo fix #4334 ✅ merged
• F2: feat(serve): shared MCP transport pool [F2] #4336 🚧 (parallel, no overlap with this PR's files)
• F3: feat(acp-bridge): F3 — multi-client permission coordination (#4175) #4335 ✅ merged (this PR branches off the F3 merge tip)
• Tracker: proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175 — F4 prereq (daemon protocol completion before F4 wires up clients)
• Resolves issue comment chiga0 Error after selecting authentication method: "Cannot read properties of undefined (reading 'value')" #19 P0 (3 stamping items) + Ilya0527 运行不了 #15 (state divergence bug)

🤖 Generated with Qwen Code

[github-actions]

📋 Review Summary

This PR bundles two F4 prerequisites: (1) protocol-level stamping additions for SSE events (server timestamp, error classification, tool provenance), and (2) gap detection for SSE reconnects past the ring eviction point. Both are well-implemented with comprehensive test coverage. The changes are strictly additive and backward-compatible. Overall assessment: strong implementation with excellent documentation, though a few edge cases and naming considerations warrant attention before merge.

🔍 General Feedback

• Excellent documentation: Inline comments thoroughly explain design decisions, reference issue numbers (proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175), and document the "why" behind each change. This is exemplary.
• Clean separation of concerns: Each commit addresses one concern independently, making review easier despite the bundle.
• Comprehensive test coverage: Both commits include extensive unit tests covering normal paths, edge cases, and boundary conditions.
• Backward compatibility: All changes are additive—new fields are optional, and existing behavior is preserved.
• Type safety: SDK TypeScript changes maintain strict typing with proper type guards and validation functions.
• Network-friendly design: The gap detection replay-continues-after-resync pattern avoids unnecessary reconnections.

🎯 Specific Feedback

🟡 High Priority

1. packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.ts - The resolveToolProvenance method is referenced in tests but not implemented in the source file. The tests call ToolCallEmitter.resolveToolProvenance() as a static method, but the current implementation only has instance methods. This appears to be a missing implementation that would cause test failures.

   • Recommendation: Add the static resolveToolProvenance(toolName, subagentMeta?) method to classify tool provenance as 'builtin' | 'mcp' | 'subagent' with the serverId extraction logic for MCP tools.

2. packages/cli/src/serve/server.ts:2553-2571 - The _meta.serverTimestamp stamping happens at the SSE wire boundary via spread merge, but there's a potential type safety issue: BridgeEvent type doesn't include _meta in its definition, so the cast (event as { _meta?: Record<string, unknown> })._meta bypasses type checking.

   • Recommendation: Consider adding _meta?: Record<string, unknown> to the BridgeEvent type definition or create a typed interface for stamped events to maintain end-to-end type safety.

🟢 Medium Priority

3. packages/acp-bridge/src/eventBus.ts:357-402 - The gap detection logic compares earliestInRing > opts.lastEventId + 1, but the check doesn't account for the case where opts.lastEventId equals Number.MAX_SAFE_INTEGER (integer overflow edge case). While extremely rare in practice, this could cause incorrect gap detection.

   • Recommendation: Add a guard: opts.lastEventId < Number.MAX_SAFE_INTEGER to the condition, or document this as a known limitation.

4. packages/sdk-typescript/src/daemon/events.ts:1101-1114 - The auto-skip gate for awaitingResync advances lastEventId via advanceLastEventId(base) implicitly, but this isn't immediately clear from the code. The comment explains it, but the actual advancement happens in the base spread at the end of each case block.

   • Recommendation: Consider extracting the skip logic into a helper function with a clearer comment: // Skip delta application but preserve lastEventId advancement from base state.

5. packages/sdk-typescript/src/daemon/events.ts:864-884 - The RESYNC_PASSTHROUGH_TYPES set includes terminal events but the comment lists session_died, session_closed, client_evicted, stream_error. However, session_closed is not in the actual set—only 4 types are listed. This is a documentation/code mismatch.

   • Recommendation: Either add session_closed to the set if it should pass through, or remove it from the comment if intentional.

🔵 Low Priority

6. packages/cli/src/serve/server.ts:1944-1965 - The errorKind stamping uses ...(errorKind ? { errorKind } : {}) which is correct, but the pattern could be simplified to errorKind (short-hand) if the field is always defined when classified. The current approach is defensive but slightly verbose.

   • Recommendation: Consider defining errorKind as string | undefined in the type and letting JSON.stringify omit undefined values naturally (requires JSON.stringify replacer or type change).

7. packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.test.ts:700-800 - The test suite for resolveToolProvenance is comprehensive, but the test naming convention could be more consistent. Some tests use "classifies" while others use "stamps".

   • Suggestion: Standardize test descriptions to follow the pattern: "classifies [input] as [provenance] when [condition]".

8. packages/sdk-typescript/src/daemon/events.ts:23-35 - The comment for state_resync_required mentions "Synthetic (no id) so it doesn't burn a slot in the per-session monotonic sequence." This is a critical design decision that deserves a code comment at the emission site in eventBus.ts as well, not just the SDK side.

   • Suggestion: Add a cross-reference comment in eventBus.ts pointing to this SDK comment for bidirectional traceability.

9. packages/sdk-typescript/test/unit/daemonEvents.test.ts:2388-2396 - The test "rejects malformed state_resync_required payload via unrecognizedKnownEventCount" is excellent, but it would benefit from also testing the case where reason is empty string (which isNonEmptyString should reject).

   • Suggestion: Add a test case with reason: '' to verify the validation handles empty strings correctly.

✅ Highlights

• Gap detection algorithm (packages/acp-bridge/src/eventBus.ts:380-397): The ring eviction detection is elegantly simple—comparing earliest-in-ring against lastEventId+1—and the synthetic frame pattern mirrors existing client_evicted semantics. This is a clean solution to a subtle multi-client state divergence bug.
• Provenance classification heuristic (packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.test.ts:706-780): The mcp__<server>__<tool> naming convention with subagent override is a pragmatic approach that requires no new context dependencies on the emit hot path.
• Reducer auto-skip logic (packages/sdk-typescript/src/daemon/events.ts:1101-1114): The gate that skips non-terminal deltas while awaitingResync is true, but still advances lastEventId, is precisely the right trade-off between correctness and recovery flexibility.
• Test coverage for boundary conditions: The tests for exact boundary cases (lastEventId === earliest - 1), empty rings, and fresh subscribes demonstrate thorough thinking about edge cases.
• Commit organization: Each feature is a self-contained commit with clear boundaries, making rollback or cherry-pick straightforward if needed.

[Copilot]

Pull request overview

This PR completes two daemon protocol prerequisites needed for the F4 client-adapter wave: (1) daemon-side “stamping” of additional metadata (_meta.serverTimestamp, tool-call provenance, and errorKind on stream_error) and (2) SSE replay gap detection that emits a new state_resync_required event when a client resumes past the EventBus ring eviction boundary.

Changes:

• Daemon now stamps _meta.serverTimestamp on SSE frames, stamps errorKind on stream_error when classifiable, and stamps tool-call provenance (builtin/mcp/subagent + optional serverId) in tool call events.
• EventBus subscribe() detects ring-eviction gaps on resume and force-pushes a synthetic state_resync_required frame before replay frames.
• SDK adds the new state_resync_required event type and reducer behavior (awaitingResync) to skip non-terminal deltas until the consumer reseeds via loadSession.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/acp-bridge/src/eventBus.ts	Emit state_resync_required when Last-Event-ID is behind the ring head due to eviction.
packages/acp-bridge/src/eventBus.test.ts	Adds/updates unit tests for resync gap detection and ordering vs replay frames.
packages/cli/src/serve/server.ts	Stamps _meta.serverTimestamp at SSE write boundary; stamps errorKind on stream_error.
packages/cli/src/serve/server.test.ts	Updates SSE tests to account for _meta.serverTimestamp and validates errorKind stamping.
packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.ts	Adds provenance stamping and resolveToolProvenance() helper for tool calls.
packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.test.ts	Adds tests for provenance resolution and stamping across emit paths.
packages/cli/src/acp-integration/session/HistoryReplayer.test.ts	Updates replay expectations to include provenance in _meta.
packages/sdk-typescript/src/daemon/events.ts	Adds event typing + reducer state for state_resync_required and resync gating behavior.
packages/sdk-typescript/src/daemon/index.ts	Re-exports the new resync event/data types.
packages/sdk-typescript/src/index.ts	Re-exports the new resync event/data types from the SDK root.
packages/sdk-typescript/test/unit/daemonEvents.test.ts	Adds reducer tests covering resync state, skip gating, and terminal passthrough.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wenshao]

No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review

[yiliang114]

LGTM — clean protocol-level additions with solid backward compat.

Commit 1 (#19 stamping): _meta.serverTimestamp at the SSE write boundary is the right call — keeps BridgeEvent unpolluted. resolveToolProvenance as a static pure function with duck-typed mcp__ parsing and subagent precedence is well-designed; edge cases (malformed prefix, empty server segment) fall through to builtin cleanly.

Commit 2 (#15 gap detection): The earliestInRing > lastEventId + 1 check with a synthetic id-less state_resync_required frame before replay is straightforward and well-tested — positive cases, boundary off-by-one, empty ring, and fresh subscriber are all covered. SDK reducer gating via awaitingResync with RESYNC_PASSTHROUGH_TYPES for terminal lifecycle frames is a solid safety default.

preserveFsErrorOverAcp duck typing approach is consistent with the existing mapDomainErrorToErrorKind pattern for cross-package boundaries. Test matrix covers write + read paths, non-FsError passthrough, and wrong-name guard.