feat(serve): session metadata and close/delete lifecycle (#4175 Wave 2.5 PR 11)#4240
Merged
Conversation
Contributor
📋 Review SummaryThis PR implements session lifecycle management ( 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
Summary: This is a well-engineered PR that addresses a known gap in the session lifecycle API. The main concern is ensuring |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the qwen serve daemon protocol and the TypeScript SDK with explicit session lifecycle closure (DELETE /session/:id) and mutable session metadata updates (PATCH /session/:id/metadata), plus richer session listings and corresponding typed events.
Changes:
- Adds
DELETE /session/:idandPATCH /session/:id/metadataroutes, bridge support, and capability tags (session_close,session_metadata). - Adds SDK methods (
DaemonClient.closeSession/updateSessionMetadata,DaemonSessionClient.close/updateMetadata) and new typed events (session_closed,session_metadata_updated) with reducer support. - Enriches workspace session listings with
createdAt,displayName,clientCount,hasActivePrompt, and updates docs + tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/serve/server.ts | Adds DELETE + PATCH routes and maps InvalidSessionMetadataError to 400 responses. |
| packages/cli/src/serve/server.test.ts | Adds route-level unit coverage for session close + metadata update endpoints and capability tags. |
| packages/cli/src/serve/httpAcpBridge.ts | Implements closeSession, updateSessionMetadata, session createdAt, and enriched session summaries. |
| packages/cli/src/serve/httpAcpBridge.test.ts | Adds bridge-level tests for close semantics, metadata events, and enriched listings. |
| packages/cli/src/serve/capabilities.ts | Registers new serve capabilities (session_close, session_metadata). |
| packages/sdk-typescript/src/daemon/DaemonClient.ts | Adds closeSession and updateSessionMetadata client methods. |
| packages/sdk-typescript/src/daemon/DaemonSessionClient.ts | Adds session-scoped convenience methods close() and updateMetadata(). |
| packages/sdk-typescript/src/daemon/events.ts | Adds new event types + reducer handling for session_closed and session_metadata_updated. |
| packages/sdk-typescript/src/daemon/types.ts | Extends daemon session/session-summary types with metadata + listing fields. |
| packages/sdk-typescript/src/daemon/index.ts | Re-exports newly introduced daemon event types. |
| packages/sdk-typescript/test/unit/DaemonClient.test.ts | Adds unit tests for new DaemonClient methods. |
| packages/sdk-typescript/test/unit/DaemonSessionClient.test.ts | Verifies new DaemonSessionClient methods are wired and send expected requests/headers. |
| packages/sdk-typescript/test/unit/daemonEvents.test.ts | Adds schema + reducer tests for session_closed and session_metadata_updated. |
| integration-tests/cli/qwen-serve-routes.test.ts | Adds integration coverage for new endpoints and enriched session listing fields. |
| docs/developers/qwen-serve-protocol.md | Documents new routes, events, and capability tags; updates session listing example. |
Comments suppressed due to low confidence (1)
packages/cli/src/serve/server.ts:595
PATCH /session/:id/metadataresponds with the raw requestdisplayName, but the bridge normalizes''toundefined(clearing the name). That means a request withdisplayName: ''will report an empty string in the HTTP response even though the stored/listed value and emittedsession_metadata_updatedevent will havedisplayNameomitted/undefined. Consider normalizing the response to the effective value (e.g., omitdisplayNamewhen clearing) or have the bridge return the stored metadata so the route can echo the canonical result.
const displayName = body['displayName'];
if (displayName !== undefined && typeof displayName !== 'string') {
res.status(400).json({
error: '`displayName` must be a string',
code: 'invalid_metadata',
});
return;
}
try {
bridge.updateSessionMetadata(
sessionId,
{ displayName },
clientId !== undefined ? { clientId } : undefined,
);
res.status(200).json({ sessionId, displayName });
} catch (err) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…2.5 PR 11) Add explicit session close and metadata management to the daemon serve infrastructure, closing the Stage 1 limitation that sessions could only end via child crash or daemon shutdown. - DELETE /session/:id — force-closes a live session (cancels active prompt, resolves pending permissions, publishes session_closed event) - PATCH /session/:id/metadata — update mutable displayName - Enriched GET /workspace/:id/sessions with createdAt, displayName, clientCount, hasActivePrompt - session_closed + session_metadata_updated SDK event types with validation, reducer, and terminal event priority - DaemonClient.closeSession / updateSessionMetadata + session client wrappers - Capabilities: session_close, session_metadata
c51e525 to
e7436f5
Compare
- Fix JSDoc on closeSession: clarify that bridge throws SessionNotFoundError (SDK absorbs 404 for client-side idempotency) - Tighten event validators: isSessionClosedData checks closedBy type, isSessionMetadataUpdatedData checks displayName type - PATCH /session/:id/metadata now returns effective stored metadata instead of echoing request fields, avoiding ambiguous no-op responses - Only publish session_metadata_updated event when displayName changes - Update chooseTerminalEvent comment to reflect session_closed
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. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
PR #4240 — Code Review
2 errors, 4 warnings found across typecheck, correctness, security, and operational dimensions.
Errors
-
TS type error in test (TS2322) —
server.test.ts:1196
The mock forlistWorkspaceSessionsreturns{ sessionId, workspaceCwd }butBridgeSessionSummarynow requirescreatedAt,clientCount, andhasActivePrompt. This will breaktsc. -
createDaemonSessionViewStatedropsdisplayNamefrom seed —events.ts:210
ThedisplayNamefield was added toDaemonSessionViewState(line 188) butcreateDaemonSessionViewState()never copiesseed.displayNameinto the returned object. On SSE reconnect the client will seedisplayName: undefineduntil the nextsession_metadata_updatedevent.
Warnings
-
closeSessionhas no logging —httpAcpBridge.ts:2852
Irreversible teardown (maps removed, permissions cancelled, channel killed) with zerowriteStderrLinecalls. Operators have no audit trail. -
displayNameaccepts control characters —httpAcpBridge.ts:2908
Only length is validated. ANSI escape sequences or control characters can be injected viadisplayName. -
updateSessionMetadatahas no logging —httpAcpBridge.ts:2892
Persistent state mutation with no audit trail. -
Channel kill error silently swallowed —
httpAcpBridge.ts:2888
.catch(() => {})discards all errors unconditionally.
All 6 findings are auto-fixable. Let me know if you'd like me to apply fixes.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Nice PR — clean shape, good backward-compat story (all new fields optional, new caps gated via pre-flight), and the test coverage on terminal-event upgrade + pending-permission cancellation is exactly the contract I'd worry about most.
Inline comments are mostly nits. The two I'd most like to see addressed before merge:
events.close()beforeconnection.cancel()ordering incloseSessionPromise<void>onDaemonClient.updateSessionMetadatadiscards the effective metadata the server already returned
Smaller observations I didn't leave as inline comments:
- Test coverage gaps: invalid client ID path for
PATCH /metadata(covered forcloseSessionbut not metadata),displayName: ""clearing the stored name (documented behavior but no regression test), andcloseSessionkeeping the channel alive when other sessions on the same channel still exist. - PATCH with empty body returns 200 + current metadata: works as a free read, but undocumented. Either document it or reject as
400 nothing_to_update. session_metadata_updatedreducer doesn't guard onalive: an event arriving aftersession_closedwould still mutatedisplayName. Consistent with howsession_diedis handled today so not a regression, just an observation.
Doc nit: the protocol doc now lists typed_event_schema in the capabilities array alongside the new session_close / session_metadata — that's a good catch-up against #4226. Worth a line in the PR description noting that this PR also brings the previously-runtime-only cap into the documented list.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] [typecheck] packages/cli/src/serve/server.test.ts:1231 — fakeBridge 的 listImpl mock 返回类型不匹配。BridgeSessionSummary 新增了必填字段 createdAt、clientCount、hasActivePrompt，但该 mock 仍返回旧格式 { sessionId, workspaceCwd }，导致 tsc --noEmit 编译失败 (TS2322)。
修复： 在 mock 返回值中补充新字段：
listImpl: () => [
{ sessionId: 's-1', workspaceCwd: WS_BOUND, createdAt: new Date().toISOString(), clientCount: 1, hasActivePrompt: false },
{ sessionId: 's-2', workspaceCwd: WS_BOUND, createdAt: new Date().toISOString(), clientCount: 0, hasActivePrompt: false },
],— DeepSeek/deepseek-v4-pro via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
已处理这条 Critical typecheck review： |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
approved these changes
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Suggestion] Test coverage gaps for new session lifecycle methods:
packages/cli/src/serve/httpAcpBridge.test.ts—closeSessiontests don't verify theclosedByfield insession_closedevents orInvalidClientIdErrorfor unregistered client IDs.packages/cli/src/serve/httpAcpBridge.test.ts—loadSession/resumeSessionbridge tests don't coverWorkspaceMismatchErroron the restore path (onlyspawnOrAttachis tested).packages/sdk-typescript/test/unit/DaemonSessionClient.test.ts—unstable_resumeSessiondoesn't test the session-not-found guard (onlyloadSessioncovers it).
[Suggestion — pre-existing] DaemonSessionClient.openEventSubscription's release() unconditionally sets this.subscriptionActive = false even when the generator never called acquire(). If a consumer creates two generators via events(), starts iterating the first, and calls return() on the second (never iterated), the second generator's cleanup breaks the first's subscription invariant. Fix: only clear subscriptionActive when started is true in the release() closure.
— glm-5.1 via Qwen Code /review
| 'model_switched', | ||
| 'model_switch_failed', | ||
| 'session_died', | ||
| 'session_closed', |
Collaborator
There was a problem hiding this comment.
[Suggestion] No compile-time exhaustiveness check ensures asKnownDaemonEvent covers every element of DAEMON_KNOWN_EVENT_TYPE_VALUES. Adding a new type to this array without a corresponding case in the switch would silently classify events as "unrecognized" rather than causing a build failure.
Consider adding a type-level assertion after the switch:
const _exhaustive: Record<DaemonKnownEventType, true> = {
session_update: true,
permission_request: true,
// ... all types ...
stream_error: true,
};— glm-5.1 via Qwen Code /review
| headers: this.headers({}, clientId), | ||
| }, | ||
| async (res) => { | ||
| if (res.status === 204 || res.status === 404) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] closeSession absorbs HTTP 404 identically to 204, so SDK consumers cannot distinguish between "I closed it", "another client already closed it", and "the session ID was never valid". The doc comment mentions idempotency, but some consumers may need to differentiate these outcomes (e.g., to show "closed by you" vs "session already gone" in a UI).
Consider returning a discriminator (e.g., { closed: true } for 204 vs { alreadyGone: true } for 404), or at minimum document the 404 absorption semantics in the method's docblock.
— glm-5.1 via Qwen Code /review
This was referenced May 17, 2026
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
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
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
DELETE /session/:idfor explicit client-initiated session close — force-closes even with other clients attached, cancels active prompt, resolves pending permissions as cancelled, publishessession_closedeventPATCH /session/:id/metadatato update mutable sessiondisplayName(max 256 chars)GET /workspace/:id/sessionswithcreatedAt,displayName,clientCount,hasActivePromptsession_closed(terminal) andsession_metadata_updatedevent types to the SDK with validation, reducer, and terminal event priorityDaemonClient.closeSession()/updateSessionMetadata()andDaemonSessionClient.close()/updateMetadata()SDK methodssession_closeandsession_metadatacapabilitiesEngineering principles checklist (Stage 1.5 wave PRs)
qwen serveStage 1 routes / SDK behavior preservedValidation
Commands run:
Results: 226 CLI tests + 95 SDK tests = 321 tests passing. Lint clean. Build clean.
Linked Issues
DELETE /session/:id" Stage 1 limitation from Daemon mode (qwen serve): proposal & open decisions #3803Test plan
DELETE /session/:idreturns 204 and removes the session from listingPATCH /session/:id/metadataupdates displayName and it appears in session listingsession_closedevent is received by SSE subscribers before stream endssession_metadata_updatedevent fires on metadata changescreatedAt,clientCount,hasActivePromptsession_closeandsession_metadata🤖 Generated with Qwen Code