feat(sdk): add DaemonSessionClient skeleton#4195
Conversation
📋 Review SummaryThis PR introduces an experimental 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
wenshao
left a comment
There was a problem hiding this comment.
No issues found. Clean, well-tested, additive-only SDK wrapper that correctly forwards Stage 1 HTTP/SSE operations with proper Last-Event-ID replay tracking. LGTM! ✅ — glm-5.1 via Qwen Code /review
| return this.subscribeEvents(opts); | ||
| } | ||
|
|
||
| async *subscribeEvents( |
There was a problem hiding this comment.
[Suggestion] subscribeEvents() is public but only called internally by events() (line 115). Having two public methods with identical behavior creates API ambiguity — callers won't know which to use, and future maintainers won't know which one to extend.
| async *subscribeEvents( | |
| private async *subscribeEvents( |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return this.lastSeenEventId; | ||
| } | ||
|
|
||
| setLastEventId(lastEventId: number | undefined): void { |
There was a problem hiding this comment.
[Suggestion] setLastEventId accepts any number without validation. Values like NaN, Infinity, negative numbers, or non-integers pass through to DaemonClient.subscribeEvents() and become invalid Last-Event-ID headers (e.g., Last-Event-ID: NaN). The daemon will silently ignore these, so the client thinks it's resuming but actually starts live — events are silently dropped with no error.
| setLastEventId(lastEventId: number | undefined): void { | |
| setLastEventId(lastEventId: number | undefined): void { | |
| if (lastEventId !== undefined) { | |
| if (!Number.isSafeInteger(lastEventId) || lastEventId < 1) { | |
| throw new TypeError( | |
| `lastEventId must be a safe positive integer, got ${lastEventId}`, | |
| ); | |
| } | |
| } | |
| this.lastSeenEventId = lastEventId; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| async *subscribeEvents( | ||
| opts: DaemonSessionSubscribeOptions = {}, | ||
| ): AsyncGenerator<DaemonEvent> { |
There was a problem hiding this comment.
[Suggestion] When the inner SSE stream throws (network error, daemon restart), this.lastSeenEventId retains its stale value. The next events() call with default resume: true sends a Last-Event-ID that belongs to a potentially dead session. After a daemon restart the new session resets IDs from 1, so the stale ID causes either a 400 error or silent replay from the wrong position.
Wrap the for await in try/finally to clear the cursor on stream failure:
| ): AsyncGenerator<DaemonEvent> { | |
| try { | |
| for await (const event of this.client.subscribeEvents(this.sessionId, { | |
| ...subscribeOpts, | |
| lastEventId, | |
| })) { | |
| if (event.id !== undefined) this.lastSeenEventId = event.id; | |
| yield event; | |
| } | |
| } catch { | |
| this.lastSeenEventId = undefined; | |
| throw; | |
| } |
This is the 3 AM oncall nightmare: duplicate or missing events after a network blip, with no error log to explain why.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| async *subscribeEvents( | ||
| opts: DaemonSessionSubscribeOptions = {}, | ||
| ): AsyncGenerator<DaemonEvent> { | ||
| const { resume = true, ...subscribeOpts } = opts; |
There was a problem hiding this comment.
[Suggestion] The ?? operator on this line treats explicit undefined the same as "key not present": subscribeOpts.lastEventId ?? (resume ? this.lastSeenEventId : undefined). A caller passing { lastEventId: undefined, resume: false } (intending a fresh subscription) will unexpectedly trigger replay because ?? falls through to the resume branch.
Use 'lastEventId' in subscribeOpts to distinguish "key present with value undefined" from "key absent":
| const { resume = true, ...subscribeOpts } = opts; | |
| const lastEventId = | |
| 'lastEventId' in subscribeOpts | |
| ? subscribeOpts.lastEventId | |
| : resume | |
| ? this.lastSeenEventId | |
| : undefined; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Review Findings (unmapped to diff lines)
[Critical] subscribeEvents 中的 if (event.id !== undefined) 守卫分支未测试。所有 4 个测试事件都带显式 id: 行，但 daemon 规范中 stream_error 等事件故意不包含 id。如果未来重构修改该逻辑，lastEventId 可能被合成事件污染。请在 SSE 响应帧中添加一个无 id: 的事件，断言 session.lastEventId 不变。
[Suggestion] respondToPermission 仅测试了 200/true 路径；DaemonClient.respondToPermission 在 HTTP 404 时返回 false（表示权限请求已被其他客户端回复），该路径完全未覆盖。请添加一个返回 HTTP 404 的测试，断言 false 返回值。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ...subscribeOpts, | ||
| lastEventId, | ||
| })) { | ||
| if (event.id !== undefined) this.lastSeenEventId = event.id; |
There was a problem hiding this comment.
[Critical] lastSeenEventId 在 yield event 之前更新，导致消费者出错/崩溃时静默丢失事件。
当前代码（第 128-130 行）：
if (event.id !== undefined) this.lastSeenEventId = event.id;
yield event;如果消费者在处理 yield 的事件时抛出异常、break 出循环或进程崩溃，lastSeenEventId 已指向该事件之后。下次 events() 重连时 daemon 从下一个事件恢复，消费者永久丢失该事件。
| if (event.id !== undefined) this.lastSeenEventId = event.id; | |
| yield event; | |
| if (event.id !== undefined) this.lastSeenEventId = event.id; |
这样确保 lastSeenEventId 仅在消费者成功接收事件后才提交。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Summary
DaemonSessionClientSDK wrapper that binds aDaemonClientto one daemon session, forwards existing Stage 1 HTTP/SSE operations, tracksLast-Event-IDreplay state, and exports the new API from the daemon and package entrypoints.qwen servethrough HTTP + SSE without threadingsessionIdthrough every call site.Validation
DaemonClientbehavior.DaemonClient+DaemonSessionClientunit tests passed, typecheck passed, SDK build passed, targeted eslint passed, prettier check passed.DaemonSessionClient.test.ts: 4 tests passed.DaemonClient.test.ts+DaemonSessionClient.test.ts: 47 tests passed.npm run typecheck: passed.npm run build: passed.Note:
cd packages/sdk-typescript && npm run lintcurrently fails before linting on the local dependency/config mix with@typescript-eslint/no-unused-expressionsreading missingallowShortCircuit. The staged-file pre-commit eslint and the targeted root eslint command both passed.Scope / Risk
DaemonSessionClienttracks replay state by default; callers that want a fresh SSE attach should pass{ resume: false }.DaemonClientmethods.Engineering principles checklist (Stage 1.5 wave PRs)
qwen serveStage 1 routes / SDK behavior preservedTesting Matrix
Testing matrix notes:
npm runcoverage:typecheck,build; package-levellinthas the local rule-load issue noted above.npxcoverage: focused vitest, targeted eslint, and prettier check.Linked Issues / Bugs
Related to #4175 Wave 1 PR 3 and #3803.
Depends on #4191.