feat(serve): advertise typed_event_schema + pin SDK public surface (#4175 PR 4 follow-up)#4226
Merged
Merged
Conversation
Contributor
📋 Review SummaryThis PR introduces an SDK-layer discriminated union ( 🔍 General Feedback
🎯 Specific Feedback🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an SDK-layer “typed protocol schema” for daemon SSE events (v1) so SDK consumers can safely narrow DaemonEvent into a discriminated union and fold it into a pure SessionState, while keeping all existing daemon event plumbing and raw event APIs backward compatible. The daemon also advertises an informational typed_event_schema capability tag.
Changes:
- Introduce
TypedDaemonEvent(8-frame union),KNOWN_DAEMON_EVENT_TYPES,narrowDaemonEvent, andisTypedDaemonEventin the SDK. - Add a pure session-level reducer skeleton (
SessionState,applyDaemonEvent, reducer factories) for typed events. - Re-export the typed surface from the SDK public entry and add unit + integration tests; advertise
typed_event_schemain the serve capability registry.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-typescript/src/daemon/events.ts | Defines typed daemon event envelope, frame data shapes, union, narrow helper, and capability tag constant. |
| packages/sdk-typescript/src/daemon/sessionState.ts | Adds pure session state + reducer (applyDaemonEvent) for folding typed events. |
| packages/sdk-typescript/src/daemon/index.ts | Re-exports typed event schema and session state reducer from the daemon sub-entry. |
| packages/sdk-typescript/src/index.ts | Re-exports typed event schema + reducer from the package public entrypoint. |
| packages/sdk-typescript/src/daemon/DaemonSessionClient.ts | Updates docstring to point to the new typed schema/reducer surface. |
| packages/sdk-typescript/test/unit/daemon-events.test.ts | Unit tests for narrowing/type guard behavior, known types, and capability tag constant. |
| packages/sdk-typescript/test/unit/daemon-session-state.test.ts | Unit tests for SessionState + reducer behavior, terminal absorption, and purity. |
| packages/sdk-typescript/test/unit/public-sdk-surface.test.ts | Regression test ensuring typed event API is available from the published SDK entry (runtime + compile-time). |
| packages/sdk-typescript/package.json | Adds @agentclientprotocol/sdk dependency to support exported ACP-referenced types. |
| packages/cli/src/serve/capabilities.ts | Advertises new typed_event_schema feature in SERVE_CAPABILITY_REGISTRY. |
| packages/cli/src/serve/server.test.ts | Updates Stage 1 feature expectation list to include typed_event_schema. |
| integration-tests/cli/qwen-serve-routes.test.ts | Updates capabilities integration test to expect 11 Stage 1 features including typed_event_schema. |
| package-lock.json | Lockfile updates reflecting the new dependency and npm metadata changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+95
to
+103
| /** | ||
| * Pure reducer signature. Implementations must return a new state | ||
| * object; mutating `state` is forbidden so consumers can compare | ||
| * snapshots by reference. | ||
| */ | ||
| export type SessionStateReducer = ( | ||
| state: SessionState, | ||
| event: TypedDaemonEvent, | ||
| ) => SessionState; |
Comment on lines
+164
to
+169
| const next: SessionState = { | ||
| ...state, | ||
| currentModelId: event.data.modelId, | ||
| }; | ||
| delete next.modelSwitchError; | ||
| return next; |
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. |
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Review: Typed Daemon Event Schema v1
Reviewer: Qwen Code
Model: mimo-v2.5-pro
Date: 2026-03-31
Scope: packages/sdk-typescript, packages/cli/src/serve, integration-tests
Analysis Summary
Deterministic: 0 code-quality findings (tsc, eslint)
Build: npm run build --workspace=packages/sdk-typescript passed
Tests: npm run test --workspace=packages/sdk-typescript passed (12 files, 320 tests)
LLM Audit: 4 suggestions, 2 nice-to-haves, 1 informational note. No critical issues.
Reviewed files: events.ts, sessionState.ts, index.ts (×2), capabilities.ts, DaemonSessionClient.ts, package.json, daemon-events.test.ts, daemon-session-state.test.ts, public-sdk-surface.test.ts, server.test.ts, qwen-serve-routes.test.ts
Not reviewed: package-lock.json (auto-generated, ~140 "peer": true annotations)
Inline Comments
4 suggestions posted inline.
Additional Notes
Info — Type-discriminator narrowing design: The narrowDaemonEvent → applyDaemonEvent pipeline intentionally validates only the type discriminator, not the data payload. This is a documented and tested design choice aligned with the trusted-daemon model. The applyDaemonEvent docstring shows direct usage with TypedDaemonEvent — if this function is ever consumed independently of narrowDaemonEvent, a minimal event.data != null guard at the reducer entry would prevent confusing TypeError stack traces from malformed frames.
Nice to have — resolvedPermissions cardinality: The permission_resolved handler accumulates entries without bound. In practice, sessions are lifecycle-bounded (the terminated guard caps growth) and each entry is ~50 bytes, so this is not a memory concern. A brief JSDoc note on expected cardinality would help consumers sizing their state containers.
Nice to have — SessionUpdateData alias missing: Every frame type except session_update has a corresponding *Data type alias. Adding export type SessionUpdateData = ACP.SessionNotification would complete the naming symmetry.
| return { kind: 'unknown', event }; | ||
| } | ||
|
|
||
| /** Type guard mirroring {@link narrowDaemonEvent}. */ |
Collaborator
There was a problem hiding this comment.
[suggestion] isTypedDaemonEvent duplicates the guard logic from narrowDaemonEvent (event.v === 1 && isKnownEventType(event.type)). If the version check or type gate evolves, both functions must be updated in lockstep.
Delegating to narrowDaemonEvent eliminates the duplication:
export function isTypedDaemonEvent(
event: DaemonEvent,
): event is TypedDaemonEvent {
return narrowDaemonEvent(event).kind === 'typed';
}| * for the matching request, or when the session terminates. | ||
| */ | ||
| readonly pendingPermissions: ReadonlyMap< | ||
| string, |
Collaborator
There was a problem hiding this comment.
[suggestion] The pendingPermissions map value is an anonymous inline object type. SDK consumers who need to reference this shape in their own code (e.g., function renderPending(p: PendingPermissionEntry)) must duplicate it or use ReturnType gymnastics.
Extracting and exporting a named type improves discoverability:
export interface PendingPermissionEntry {
sessionId: string;
toolCall: ACP.ToolCallUpdate;
options: ACP.PermissionOption[];
}
readonly pendingPermissions: ReadonlyMap<string, PendingPermissionEntry>;| * but it gives consumers a stable hook if the reducer ever needs | ||
| * configuration (e.g. opt-in payload accumulation in PR 25). | ||
| */ | ||
| export function createSessionStateReducer(): SessionStateReducer { |
Collaborator
There was a problem hiding this comment.
[suggestion] createSessionStateReducer() is a zero-argument factory that returns applyDaemonEvent unchanged. It adds no configuration or behavior — the JSDoc notes it exists as a hook for future PR 25.
Consider deferring this function to PR 25 (where it gains a real configuration bag) or tagging it @beta so consumers know the contract may change. Right now it presents two equivalent ways to do the same thing, which can confuse consumers about which to use.
| // --- Frame aliases -------------------------------------------------- | ||
|
|
||
| export type SessionUpdateFrame = DaemonEventEnvelope< | ||
| 'session_update', |
Collaborator
There was a problem hiding this comment.
[suggestion] The eight frame type aliases (SessionUpdateFrame, PermissionRequestFrame, etc.) have no JSDoc. TypeScript propagates source-level JSDoc through re-exports, so SDK consumers hovering over these types in from '@qwen-code/sdk' will see nothing.
Adding one-line JSDoc to each alias improves IDE discoverability:
/** Typed envelope for `permission_request` SSE frames. */
export type PermissionRequestFrame = DaemonEventEnvelope<...>;
wenshao
previously approved these changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No high-confidence issues found across 9 review dimensions (correctness, security, code quality, performance, test coverage, attacker/oncall/maintainer audits, build & test).
All confirmed findings are low-confidence and relate to future improvements:
- Unbounded
resolvedPermissionsmap growth (replay semantics, intentional design) - No runtime
datavalidation innarrowDaemonEvent(forward-compat by design) - No compile-time link between
TypedDaemonEventunion andKNOWN_DAEMON_EVENT_TYPESarray terminatedtype flattensSessionDiedDatasub-discrimination- Minor test matrix gap:
currentModelIdsurvival throughsession_died
Deterministic analysis: tsc + eslint clean (0 findings). Build and all 515 tests pass.
— glm-5.1 via Qwen Code /review
wenshao
reviewed
May 17, 2026
| "node": ">=22.0.0" | ||
| }, | ||
| "dependencies": { | ||
| "@agentclientprotocol/sdk": "^0.14.1", |
Collaborator
There was a problem hiding this comment.
[Suggestion] @agentclientprotocol/sdk (^0.14.1) is added as a regular dependency, but all imports from it in this PR are import type * as ACP — type-only, erased at compile time. The PR description states "ACP types are type-only imports so the SDK does not pull ACP runtime", but listing it as a dependency does pull the runtime into every consumer's node_modules.
Consider moving to peerDependencies, or keep as dependency but acknowledge the runtime cost in the description.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No high-confidence issues found. One Suggestion-level finding posted as inline comment about ACP dependency placement. Build and all 515 tests pass.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Follow-up to #4217 (`feat(protocol): add typed daemon event schema v1`, Wave 1 PR 4 of #4175), which landed the SDK-side typed schema + `KnownDaemonEvent` union + reducer but did not register a daemon-side capability tag for it. Without the tag, non-SDK clients (web debug UI, third-party adapters, channel/IDE backends not yet on `@qwen-code/sdk`) have no way to detect at the protocol envelope level that the daemon promises to emit only `KnownDaemonEvent`-shaped frames — they would either pin against SDK version, or pre-flight every frame defensively. Add `typed_event_schema: { since: 'v1' }` to `SERVE_CAPABILITY_REGISTRY`, inserted right after `session_events` (the route that delivers the frames whose schema this tag describes). The capability is purely informational — `narrowDaemonEvent`/`asKnownDaemonEvent` already fall back to "unknown" for older daemons that don't advertise the tag, so the SDK does not gate any behavior off the tag. Sync `EXPECTED_STAGE1_FEATURES` (server.test.ts) and the integration test array (qwen-serve-routes.test.ts) with the registry order, the same lockstep discipline #4214 codified. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…monSessionClient docstring at it Two small follow-ups to #4217 (Wave 1 PR 4 of #4175). 1. Public-entry regression fence `@qwen-code/sdk` is a single-entry package: `package.json.exports` only exposes `.` (`dist/index.{cjs,mjs,d.ts}`), and the bundle is built from `src/index.ts`. Symbols re-exported only from `src/daemon/index.ts` are unreachable to consumers unless they are also forwarded by `src/index.ts`. #4217 forwards the typed event schema correctly today, but the two-layer chain has no compile-time test pinning it — a future daemon export that lands in `src/daemon/index.ts` but is missed by `src/index.ts` would ship invisibly. Add `test/unit/daemon-public-surface.test.ts` that imports `* as Public from '../../src/index.js'`, asserts at runtime that every PR 4 value is `typeof === 'function'` (or a primitive of the expected shape), round-trips a raw `DaemonEvent` through the public `asKnownDaemonEvent` to prove the wire-up actually works, and compile-imports every PR 4 type so any drift fails to build. 2. DaemonSessionClient docstring pointer The class docstring already deferred typed event consumption to "the protocol schema layer" without a concrete pointer. Now that #4217 has put `asKnownDaemonEvent` and `reduceDaemonSessionEvent` in `./events.js`, name them so future readers can find the typed surface without grepping. No code change. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
e030149 to
4e24cf7
Compare
wenshao
approved these changes
May 17, 2026
| session_cancel: { since: 'v1' }, | ||
| session_events: { since: 'v1' }, | ||
| // SDK consumers can detect `KnownDaemonEvent` schema support without | ||
| // pinning against this SDK release — `narrowDaemonEvent` falls back |
Collaborator
There was a problem hiding this comment.
[Suggestion] Comment references narrowDaemonEvent falling back to kind: 'unknown', but the actual API is asKnownDaemonEvent which returns undefined for unknown/unrecognized events.
Suggested change
| // pinning against this SDK release — `narrowDaemonEvent` falls back | |
| // SDK consumers can detect `KnownDaemonEvent` schema support without | |
| // pinning against this SDK release — `asKnownDaemonEvent` returns | |
| // `undefined` for daemons that don't advertise the tag or send | |
| // malformed payloads, so the tag is purely informational. |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Local validation reportVerified on 4e24cf7 with a clean checkout of pr-4226. Each item below is the exact command and result. Unit / typecheck
Manual
|
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
Two small follow-ups to #4217 (
feat(protocol): add typed daemon event schema v1, Wave 1 PR 4 of #4175), now that #4217 has merged:feat(serve): advertise typed_event_schema capability— daemon-side capability tag was missing from feat(protocol): typed daemon event schema v1 #4217. Without it, non-SDK clients (web debug UI, third-party adapters not on@qwen-code/sdk) have no protocol-envelope-level way to detect the daemon promisesKnownDaemonEvent-shaped frames.test(sdk): pin typed event surface at the public SDK entry, point DaemonSessionClient docstring at it— regression fence soimport { asKnownDaemonEvent } from '@qwen-code/sdk'keeps working through the two-hop re-export chain (src/daemon/index.ts→src/index.ts→ published bundle), plus theDaemonSessionClient.ts:47docstring now names where the typed reducer actually lives.Total: 5 files / +112 / -1.
Why these two and not more
This branch originally proposed a full Wave 1 PR 4 — typed union, narrow helper, reducer skeleton — but #4217 by @chiga0 landed the same feature on main while this branch was in flight. The original 7 commits became a duplicate. After diffing the two designs, the genuinely independent value reduces to:
The two design pushbacks ACP-typed shapes vs
Record<string, unknown>and discriminatedsession_died.reason— are explicitly out of scope for this PR. They modify already-merged code and should be discussed via issue / PR comment with @chiga0 + @wenshao before being proposed.Capability tag detail
SERVE_CAPABILITY_REGISTRYgainstyped_event_schema: { since: 'v1' }betweensession_eventsandsession_set_model. The SDK does not gate any behavior off this tag —narrowDaemonEvent/asKnownDaemonEventalready fall back to the unknown branch for older daemons that don't advertise it, so the tag is purely informational.EXPECTED_STAGE1_FEATURES(unit) and the integration test array updated in lockstep with the registry, the same discipline #4214 codified.Public-surface fence detail
@qwen-code/sdkis a single-entry package —package.json.exportsonly exposes.(dist/index.{cjs,mjs,d.ts}), and the bundle is built fromsrc/index.ts. Symbols re-exported only fromsrc/daemon/index.tsare unreachable to consumers unless they are also forwarded bysrc/index.ts.#4217 forwards the typed event schema correctly today, but the two-layer chain has no compile-time test pinning it. A future daemon export landing in
src/daemon/index.tsbut missed bysrc/index.tswould ship invisibly. The newdaemon-public-surface.test.ts:* as Public from '../../src/index.js'(the same path the published bundle is built from)typeof === 'function'DaemonEventthrough the publicasKnownDaemonEventto prove the wire-up works end-to-endEngineering principles checklist
qwen serveStage 1 routes / SDK behavior preserved — no route logic touched, no emit code touchedRisks / explicitly out of scope
These are pushbacks against #4217's design that should be discussed separately, not merged here:
Record<string, unknown>+ index signatures for frame data shapes — much looser than possible if ACP types were imported. Tightening would couple SDK to@agentclientprotocol/sdk(currently no ACP dep).DaemonSessionDiedData.reason: string— the daemon actually emits exactly three reasons ('channel_closed' | 'killed' | 'daemon_shutdown'); a sub-discriminated union would let consumers narrowexitCode/signalCodecorrectly.KNOWN_DAEMON_EVENT_TYPESis a hand-maintainedSetconstant — same brittleness as a tuple if a future daemon adds a frame type.I'll open a separate issue/comment thread for these.
Test plan
npm --workspace=@qwen-code/sdk run typecheckcleannpm --workspace=@qwen-code/sdk run test— 305 / 305 passing (includes the new 3-test surface fence)npm --workspace=@qwen-code/qwen-code run test -- src/serve— 234 / 234 passing (covers the registry-array lockstep)daemon-public-surface.test.ts:import { asKnownDaemonEvent } from '@qwen-code/sdk'round-trips a rawDaemonEventthrough the typed narrow pathqwen serve, hitGET /capabilities, verifytyped_event_schemais incaps.featuresHistory note
This branch previously held the duplicate Wave 1 PR 4 implementation. The 7 commits were force-replaced with the 2 follow-up commits after #4217 landed. The original force-pushed history still exists in the GitHub PR timeline if anyone wants to compare.
Refs
🤖 Generated with Qwen Code