feat(sdk): add DaemonSessionClient skeleton#4201
Merged
Merged
Conversation
This was referenced May 16, 2026
Contributor
📋 Review SummaryThis PR introduces an experimental 🔍 General Feedback
🎯 Specific Feedback🟢 Medium
🔵 Low
✅ Highlights
|
wenshao
requested changes
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Test coverage gaps (these don't map to a single diff line):
prompt()accepts an optionalAbortSignalparameter (line 107) but no test passes a signal. The signal passthrough path is completely unverified — if it's broken, callers' prompt cancellation silently fails.- The guard
if (event.id !== undefined)at line 128 is never triggered in tests — all test SSE frames carryid:. Synthetic frames (e.g.stream_error) without anidwould silently corruptlastSeenEventIdif the guard is wrong. - No test verifies error propagation through
prompt(),cancel(),setModel(),respondToPermission(), orsubscribeEvents(). All test responses use 200/204; a daemon 500 or network failure path has zero coverage.
| for await (const event of this.client.subscribeEvents(this.sessionId, { | ||
| ...subscribeOpts, | ||
| lastEventId, | ||
| })) { |
Collaborator
There was a problem hiding this comment.
[Critical] lastSeenEventId is committed before yield event, not after. If the consumer crashes while processing the yielded event, the id has already advanced. On reconnect with resume: true (the default), the daemon replays after that id, so the not-yet-successfully-processed event is silently dropped — no error, no log, no crash.
This is the classic "ack before processing" anti-pattern applied to SSE resume state.
Suggested change
| })) { | |
| yield event; | |
| if (event.id !== undefined) this.lastSeenEventId = event.id; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Valid, and fixed in the current branch. DaemonSessionClient.subscribeEvents() now yields the event first and only advances lastSeenEventId after the consumer resumes, so reconnect replay does not skip a not-yet-processed event. The focused unit test also asserts the cursor stays unset before the first yielded event is acknowledged.
| return this.subscribeEvents(opts); | ||
| } | ||
|
|
||
| async *subscribeEvents( |
Collaborator
There was a problem hiding this comment.
[Critical] subscribeEvents() reads and writes this.lastSeenEventId without any synchronization, mutual exclusion, or active-subscription check. Two concurrent calls to events()/subscribeEvents() would produce non-deterministic lastSeenEventId as both async iterators interleave writes to the same field.
This breaks the documented SSE replay contract for any adapter that forks the event stream (e.g. one connection for logging, another for UI).
Suggested change
| async *subscribeEvents( | |
| async *subscribeEvents( | |
| opts: DaemonSessionSubscribeOptions = {}, | |
| ): AsyncGenerator<DaemonEvent> { | |
| if (this.subscriptionActive) { | |
| throw new Error( | |
| 'Another event subscription is already active on this session. ' + | |
| 'Reuse the existing AsyncGenerator or create a separate DaemonSessionClient.', | |
| ); | |
| } | |
| this.subscriptionActive = true; | |
| try { | |
| const { resume = true, ...subscribeOpts } = opts; | |
| const lastEventId = | |
| subscribeOpts.lastEventId ?? (resume ? this.lastSeenEventId : undefined); | |
| for await (const event of this.client.subscribeEvents(this.sessionId, { | |
| ...subscribeOpts, | |
| lastEventId, | |
| })) { | |
| if (event.id !== undefined) this.lastSeenEventId = event.id; | |
| yield event; | |
| } | |
| } finally { | |
| this.subscriptionActive = false; | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Valid, and fixed in the current branch. A single DaemonSessionClient now rejects concurrent subscriptions with a clear error, while callers that need fan-out can create separate session clients over the same underlying daemon session. The test suite covers the concurrent subscription guard. I intentionally kept the cursor advancement after yield, matching the ack-after-consume fix from the other thread.
wenshao
reviewed
May 16, 2026
| headers: { 'content-type': 'application/json' }, | ||
| }); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
[Suggestion] Test helpers (jsonResponse, sseResponse, CapturedRequest, recordingFetch) are copy-pasted (~60 lines) from DaemonClient.test.ts. As the daemon adapter surface grows, these duplicates will diverge.
Suggested change
| // Consider extracting to a shared test helper, e.g.: | |
| // import { jsonResponse, sseResponse, recordingFetch } from '../helpers/daemonFetch.ts'; |
— glm-5.1 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Reasonable suggestion, but I am leaving this out of this PR to keep the SDK skeleton small and reviewable. The duplicated helpers are test-local and not part of the shipped surface; once the typed daemon event schema PR adds more protocol-level tests, extracting a shared daemon fetch/SSE helper will pay for itself without creating churn here.
Collaborator
Author
|
处理了这轮 review 里我认为真实有效的问题：
我认为 test helper 抽取是合理 cleanup 建议，但不是当前 PR 必须项：现在只是在两个 SDK 测试里有少量 recording fetch helper，先保留局部 helper，等 daemon SDK 测试继续扩张后再统一抽公共测试工具，避免这个 skeleton PR 扩范围。 验证：
|
4f49825 to
1fe2b04
Compare
wenshao
requested changes
May 16, 2026
| const session = await DaemonSessionClient.createOrAttach(daemon, { | ||
| workspaceCwd: caps.workspaceCwd, | ||
| }); | ||
|
|
Collaborator
There was a problem hiding this comment.
[Critical] This recommended session-client flow can miss attach-time daemon events. The existing daemon docs require clients that use modelServiceId on attach to either subscribe before POST /session or pass Last-Event-ID: 0, because model_switch_failed is emitted only on SSE and the HTTP create/attach call still succeeds. DaemonSessionClient.createOrAttach() performs the POST /session before callers can subscribe, and this first session.events() call sends no cursor, so adapters following this example can silently miss a failed model switch and continue on the wrong model. Please make the first subscription replay from the start (or explicitly seed/pass lastEventId: 0) and document that pattern.
Suggested change
| for await (const event of session.events({ | |
| signal: eventController.signal, | |
| lastEventId: 0, | |
| })) { |
— gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Valid catch, fixed in da6a35c. DaemonSessionClient.createOrAttach() now seeds lastEventId: 0 whenever the request carries modelServiceId, so the first session.events() subscription replays the daemon ring and can observe attach-time model_switch_failed / model_switched events. I also documented the raw DaemonClient pattern and added a unit test that verifies the first SSE request sends Last-Event-ID: 0 in this path.
1fe2b04 to
da6a35c
Compare
wenshao
approved these changes
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Verified locally on top of main:
npx vitest run test/unit/DaemonClient.test.ts test/unit/DaemonSessionClient.test.ts: 53 passed (8 + 45)- Full sdk-typescript suite: 272 passed / 9 files
npm run typecheckandnpm run build(ESM + CJS + .d.ts): clean;DaemonSessionClientcorrectly exported in dist- Targeted eslint on PR files and root-level eslint over
packages/sdk-typescript/{src,test}with--max-warnings 0: clean prettier --checkon PR files: clean- Smoke test against the built
dist/index.mjswith a mock fetch:createOrAttach, prompt forwarding, SSE iteration withLast-Event-IDresume, and the concurrent-subscription rejection all behave as expected
Design notes:
- Forwarding shape (prompt/cancel/setModel/respondToPermission) matches
DaemonClient;respondToPermissioncorrectly stays un-scoped since permission ids are global. - Replay cursor advances after
yield, so a mid-iteration crash resumes at the last successfully consumed event. Frames without an SSE id don't reset the cursor. subscriptionActiveguard prevents iterator interleaving and is released infinally.createOrAttachseedslastEventId = 0whenmodelServiceIdis supplied so the firstevents()picks up attach-timemodel_switched/model_switch_failedfrom the daemon replay ring.
Scope is purely additive (5 new files in packages/sdk-typescript). Branch is 3 commits behind main; rebase will be trivial.
Nit (not a blocker): the "Evidence" lines in the PR body ("4 tests passed" / "47 tests passed") are stale — actual counts after the later commits are 8 + 45 = 53. Worth a quick body update.
LGTM.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
Update after the latest review round:
Validation:
Re-requested review from @wenshao and @doudouOUC. |
Collaborator
|
Supplement to my approval review — the actual tmux session transcript from local verification. 1. Targeted vitest (PR body's listed validation)2. typecheck3. build (ESM + CJS + .d.ts)4. eslint — targeted (PR-touched files only)5. prettier --check6. Full sdk-typescript test suite7. Root-level eslint over sdk-typescript (CI-equivalent)8. Package-level
|
wenshao
requested changes
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Cross-cutting findings (files outside this PR's diff scope, tracked for follow-up):
[Suggestion]packages/core/src/core/geminiChat.ts: Heap-pressure compression bypass triggersstructuredClone(history)under memory pressure, risking OOM when the large history itself is the cause of heap pressure. Consider adding a history-size guard before the clone, or using a lightweight tail-only compression strategy under heap pressure.[Suggestion]packages/core/src/core/geminiChat.ts: Heap-pressure compression result is not logged. Theelse if (bypassTokenThreshold)branch sets the cooldown but doesn't log whether compression succeeded or failed.[Suggestion]packages/core/src/agents/background-agent-resume.ts: Resume path doesn't explicitly setisBackgrounded: true(relies on spread), while other registration sites do set it explicitly.[Suggestion]packages/core/src/agents/background-agent-resume.ts: Cancel-during-resume race between the tworegister()calls — a concurrentcancel()in the async gap gets overwritten by the second register.[Suggestion]packages/core/src/agents/background-tasks.ts:register()usesas AgentTaskcast-and-mutate pattern that is fragile against futureTaskBasefield additions.[Suggestion]packages/cli/src/ui/statusLinePresets.ts:model-with-reasoningpreset is functionally identical tomodel— no reasoning level is ever displayed.[Suggestion]packages/core/src/core/geminiChat.ts: Multiple chat instances independently react to process-wide heap pressure, potentially amplifying memory pressure by triggering simultaneous compression.[Suggestion]packages/core/src/utils/nextSpeakerChecker.ts: Filler messageparts.push({ text: '' })mutates a clone, not real history — the push is dead code and potentially confusing.
— glm-5.1 via Qwen Code /review
| */ | ||
| static async createOrAttach( | ||
| client: DaemonClient, | ||
| req: CreateSessionRequest = {}, |
Collaborator
There was a problem hiding this comment.
[Suggestion] lastEventId: 0 assumption about daemon-side event ID numbering is undocumented.
createOrAttach seeds lastEventId: 0 when modelServiceId is present, assuming daemon SSE IDs start at 0. If a different daemon implementation starts event IDs at a non-zero value, this will silently miss attach-time events. The daemon-side contract should be documented.
Suggested change
| req: CreateSessionRequest = {}, | |
| // Seeds lastEventId=0 because the daemon's SSE replay ring uses 0-based | |
| // monotonic IDs; Last-Event-ID: 0 triggers replay from ring start so | |
| // create-then-subscribe clients observe attach-time model switch events. | |
| const lastEventId = req.modelServiceId ? 0 : undefined; |
— glm-5.1 via Qwen Code /review
|
|
||
| events( | ||
| opts: DaemonSessionSubscribeOptions = {}, | ||
| ): AsyncGenerator<DaemonEvent> { |
Collaborator
There was a problem hiding this comment.
[Suggestion] events() subscription guard activates lazily — two synchronous events() calls both succeed.
events() returns this.subscribeEvents(opts) which is an async generator. The subscriptionActive guard inside subscribeEvents() only runs when the generator body executes (first .next()). Two synchronous calls to events() both return generator objects without error. The error only appears when iteration begins, making the root cause hard to trace.
Consider setting subscriptionActive = true eagerly in events() and refactoring subscribeEvents into a private internal method that doesn't re-check:
Suggested change
| ): AsyncGenerator<DaemonEvent> { | |
| events( | |
| opts: DaemonSessionSubscribeOptions = {}, | |
| ): AsyncGenerator<DaemonEvent> { | |
| if (this.subscriptionActive) { | |
| throw new Error( | |
| 'Another event subscription is already active on this session. ' + | |
| 'Reuse the existing AsyncGenerator or create a separate DaemonSessionClient.', | |
| ); | |
| } | |
| this.subscriptionActive = true; | |
| return this.subscribeEventsInternal(opts); | |
| } |
— glm-5.1 via Qwen Code /review
wenshao
requested changes
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] createOrAttach without modelServiceId has zero test coverage. The no-replay path (const lastEventId = req.modelServiceId ? 0 : undefined → undefined branch) — the simpler, more common production path — is never exercised. All existing tests pass modelServiceId: 'qwen-prod'. Add a test: call createOrAttach(client, { workspaceCwd: '/work/a' }) without modelServiceId, assert session.lastEventId is undefined, iterate events(), and assert no last-event-id header is sent.
[Suggestion] No abort signal test for events() subscription. prompt() has an abort-signal test but events() does not. Add a test: start session.events({ signal: controller.signal }), call controller.abort(), assert the stream rejects with AbortError and a subsequent events() call succeeds (guard cleared).
[Suggestion] setLastEventId() public setter is never called in any test. Constructor lastEventId option is tested, getter is tested, but the setter method is entirely uncovered. Add a case to the replay-state test: create session without lastEventId, call session.setLastEventId(7), iterate events(), assert header is last-event-id: 7.
[Suggestion] respondToPermission error paths (404 → false vs throwing) and cancel()/setModel() error paths untested. The 404 semantic is important for callers to distinguish "already handled" from network errors. Add a 404 test for respondToPermission asserting resolves.toBe(false) and error-path tests for cancel/setModel.
[Suggestion] Concurrent guard recovery after cleanup not verified. In the "rejects concurrent subscriptions" test, after await first.return(undefined), no assertion verifies a new events() call can succeed. Add: const third = session.events(); await expect(third.next()).resolves.toBeDefined();
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return this.lastSeenEventId; | ||
| } | ||
|
|
||
| setLastEventId(lastEventId: number | undefined): void { |
Collaborator
There was a problem hiding this comment.
[Suggestion] setLastEventId() silently accepts NaN, negative numbers, Infinity, and non-integers. Downstream DaemonClient.subscribeEvents calls String(opts.lastEventId), producing garbage headers like Last-Event-ID: NaN. A caller restoring from corrupt storage would get confusing protocol errors with no SDK-level signal.
Suggested change
| setLastEventId(lastEventId: number | undefined): void { | |
| setLastEventId(lastEventId: number | undefined): void { | |
| if (lastEventId !== undefined && (!Number.isFinite(lastEventId) || lastEventId < 0 || !Number.isInteger(lastEventId))) { | |
| return; | |
| } | |
| this.lastSeenEventId = lastEventId; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return await this.client.respondToPermission(requestId, response); | ||
| } | ||
|
|
||
| events( |
Collaborator
There was a problem hiding this comment.
[Suggestion] events() and subscribeEvents() are redundant public APIs with identical behavior and signatures. The README and all tests use only events(). Having both public forces callers to wonder which is canonical.
Consider making subscribeEvents() private and keeping events() as the sole public entry point.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| throw new Error( | ||
| 'Another event subscription is already active on this session. ' + | ||
| 'Reuse the existing AsyncGenerator or create a separate DaemonSessionClient.', | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] lastSeenEventId updates after yield event, so it lags one event behind during active iteration. This is intentional (at-least-once semantics, fixed from a prior ack-before-processing bug), but undocumented. Callers checkpointing session.lastEventId mid-stream in a for await loop will persist a stale cursor and get duplicate events on reconnect.
Add a JSDoc note to events(): lastEventId trails the most recently yielded event by one during iteration; only checkpoint it after the for await loop completes or when the stream signals completion/interruption.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
approved these changes
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — glm-5.1 via Qwen Code /review
Merged
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 #4200.