fix(serve): align integration test + user doc with merged sessionScope override (#4175 follow-up)#4214
Merged
Conversation
…e override PR #4209 (Wave 2 PR 5) shipped per-request `sessionScope` override and added a `session_scope_override` capability tag to the registry. Two follow-ups from wenshao's review landed unaddressed: 1. `integration-tests/cli/qwen-serve-routes.test.ts` still asserted the pre-PR 9-element `caps.features` list and was named "all 9 Stage 1 features". Running the suite against a real daemon would fail — the daemon now advertises 10 features, with `session_scope_override` between `session_create` and `session_list` per the registry order. PR CI didn't catch this because integration tests need a real `qwen serve` spawn and run only in the release pipeline; the unit-level `EXPECTED_STAGE1_FEATURES` constant in `server.test.ts` was updated, but its integration sibling was missed. 2. `docs/users/qwen-serve.md` "Stage 1.5+ runtime guarantees" still listed per-request `sessionScope` override as item 1 of "Blockers for serious downstream use", saying "today the daemon-wide default is the only setting." Directly contradicts the merged behavior and the protocol doc, so downstream integrators reading the user guide get inverse guidance. Fixes: - Update the integration test name to "all 10 Stage 1 features" and insert `session_scope_override` in the asserted array (matching registry order); add a comment noting the unit/integration/registry triple must stay in lockstep. - Remove the obsolete blocker bullet from the user doc and renumber the remaining items (2/3 → 1/2 in Blockers, 4-7 → 3-6 in Reliability, 8-10 → 7-9 in Integration ergonomics). No production code changes. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Contributor
📋 Review SummaryThis PR addresses two follow-up items from the merged session scope override feature (#4209): fixing a stale integration test assertion and correcting outdated user documentation. Both changes are mechanical corrections that align test/doc artifacts with the shipped implementation. No production code changes. 🔍 General Feedback
🎯 Specific Feedback🔵 LowFile: File: ✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up to the merged per-request sessionScope override work, updating the serve integration test and user documentation so they match the daemon’s currently advertised capabilities and behavior.
Changes:
- Update the
qwen serveintegration test to assert the current 10 Stage 1 capability features (includingsession_scope_override) in the correct registry order. - Remove the now-obsolete documentation “blocker” claiming per-request
sessionScopeoverride is unavailable, and renumber the remaining lists for consistency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| integration-tests/cli/qwen-serve-routes.test.ts | Aligns the asserted caps.features list and test name with the current 10-feature capability registry order. |
| docs/users/qwen-serve.md | Removes outdated guidance about sessionScope override and renumbers the Stage 1.5+ guarantee lists for consistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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. |
12 tasks
wenshao
reviewed
May 16, 2026
| 1. **Per-request `sessionScope` override** on `POST /session` — today the daemon-wide default is the only setting; a VSCode extension can't say "I want a private session for this window" against a daemon configured for shared sessions. | ||
| 2. **`loadSession` / `unstable_resumeSession` over HTTP** — without this, no integration can survive a child crash or daemon restart, and any orchestrator coordinating the daemon can't recover state either. | ||
| 3. **Persistent client identity (pair tokens + per-client revocation)** — Stage 1 uses one shared bearer; a leaked token revokes everyone, and `originatorClientId` is client-self-declared rather than daemon-stamped from authenticated identity. | ||
| 1. **`loadSession` / `unstable_resumeSession` over HTTP** — without this, no integration can survive a child crash or daemon restart, and any orchestrator coordinating the daemon can't recover state either. |
Collaborator
There was a problem hiding this comment.
[Suggestion] Now that per-request sessionScope is no longer a blocker, the user guide should also tell client authors how to use the shipped override. This page currently has no mention of sessionScope or the session_scope_override capability, while the developer protocol docs describe POST /session accepting sessionScope: "thread" and recommend gating it on /capabilities. Readers of the user guide can still come away thinking same-workspace clients must share the daemon-wide default session, which is especially risky for IDE/window-style integrations that need isolation.
Consider adding a short POST /session note or example here that mirrors the developer docs: sessionScope: "thread" forces a fresh session, the default single may attach to an existing one, and clients should check /capabilities.features for session_scope_override before sending the field.
— gpt-5.5 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. Clean follow-up that correctly aligns the integration test (9→10 features) and removes the shipped blocker bullet from user docs. All three capability registries are in lockstep, renumbering is consistent, and toEqual prevents silent drift. LGTM! ✅ — glm-5.1 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! ✅
All three lockstep sources (integration test array, SERVE_CAPABILITY_REGISTRY, EXPECTED_STAGE1_FEATURES) verified in sync. Doc renumbering is consistent across all three sub-lists. Build and unit tests pass.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
本地真实场景测试报告（tmux）Head: 三处 lockstep 验证
真实 daemon
|
| 命令 | 结果 |
|---|---|
Integration test "all 10 Stage 1 features" (real daemon) |
✅ 1 pass |
Unit test "marks every current feature with its historical v1 origin" |
✅ 1 pass |
| GitHub CI | ✅ 全绿（Lint, Test ubuntu/macos/windows, CodeQL, Coverage） |
文档 renumbering
- Blockers: 删除原 pre-release: fix ci #1 (sessionScope) → 2/3 重编号成 1/2 ✓
- Reliability: 4-7 → 3-6 ✓
- Integration ergonomics: 8-10 → 7-9 ✓
每个 subsection 内连续；跨 subsection 各自编号（符合 markdown ordered-list 行为）。
LGTM ✅ — 已 APPROVED (17:26Z)，仅补本地实测留档。
Merged
11 tasks
doudouOUC
added a commit
that referenced
this pull request
May 17, 2026
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)
wenshao
pushed a commit
that referenced
this pull request
May 17, 2026
…4175 PR 4 follow-up) (#4226) * feat(serve): advertise typed_event_schema capability 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) * test(sdk): pin typed event surface at the public SDK entry, point DaemonSessionClient 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)
12 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
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
Follow-up to PR #4209 (Wave 2 PR 5, merged at
878f35fc4). Addresses two findings from wenshao'sCHANGES_REQUESTEDreview that landed unaddressed:Integration test feature list was stale.
integration-tests/cli/qwen-serve-routes.test.ts:183still asserted the pre-PR 9-elementcaps.featuresarray and was named "all 9 Stage 1 features". The merged daemon advertises 10 features (session_scope_overridebetweensession_createandsession_list), so the suite would fail when run against a real daemon. PR CI didn't catch it because integration tests require a realqwen servespawn and run only in the release pipeline; only the unit-levelEXPECTED_STAGE1_FEATURESconstant inserver.test.tswas updated.User doc contradicted shipped behavior.
docs/users/qwen-serve.md:193listed per-requestsessionScopeoverride as item 1 of "Blockers for serious downstream use" with the line "today the daemon-wide default is the only setting." That's now exactly what the daemon doesn't say — downstream integrators reading the user guide were getting inverse guidance.Changes
integration-tests/cli/qwen-serve-routes.test.ts— rename to "all 10 Stage 1 features"; insertsession_scope_overrideinto the asserted array; add a comment noting the unit / integration / registry triple must stay in lockstep.docs/users/qwen-serve.md— remove the obsolete blocker bullet; renumber remaining items in the Blockers (2/3 → 1/2), Reliability baseline (4–7 → 3–6), and Integration ergonomics (8–10 → 7–9) sub-lists for internal consistency.No production code changes.
Engineering principles checklist (#4175 PR template)
Test plan
npx vitest run packages/cli/src/serve/server.test.ts -t "marks every current feature with its historical v1 origin"— unit-level capability registry order assertion still green.npm run build && TEST_CLI_PATH=$(pwd)/packages/cli/dist/index.js npx vitest run integration-tests/cli/qwen-serve-routes.test.ts -t "advertises all 10 Stage 1 features"— expected pass; this is the test that was failing before this PR.docs/users/qwen-serve.md"Stage 1.5+ runtime guarantees" section — renumbering is consistent across all three sub-lists.Related
packages/cli/src/serve/capabilities.ts—SERVE_CAPABILITY_REGISTRYorder this PR's array must mirror.packages/cli/src/serve/server.test.ts:58-69—EXPECTED_STAGE1_FEATURESconstant this PR's array must mirror.🤖 Generated with Qwen Code