feat(serve): per-request sessionScope override on POST /session (#4175 Wave 2 PR 5)#4209
Merged
Conversation
Resolves the FIXME at httpAcpBridge.ts:BridgeOptions.sessionScope from #3803 — clients can now override the daemon-wide sessionScope per request instead of being stuck with whatever boot-time value the operator picked. A VSCode window that wants strict isolation can ask for `'thread'` against a default-`'single'` daemon, and vice versa. Wire change: - POST /session body accepts optional `sessionScope: 'single' | 'thread'` - Per-request value wins; daemon-wide default remains the fallback when the field is omitted (bit-for-bit backward compat for every existing caller) - Invalid values yield 400 `{ code: 'invalid_session_scope' }` - New capability tag `session_scope_override` advertised on /capabilities.features for negotiation Bridge changes: - BridgeSpawnRequest gains optional `sessionScope` - spawnOrAttach validates the per-request value and resolves effectiveScope = req.sessionScope ?? defaultSessionScope - doSpawn now takes effectiveScope and only stamps `defaultEntry` (the single-scope attach slot) when the spawn is single-scope — fixes a mixed-scope leak where a thread-first call would let a later omitted-scope call attach to the supposedly-isolated session SDK: - CreateSessionRequest gains optional `sessionScope` - DaemonClient.createOrAttachSession conditionally spreads it into the JSON body so omitted callers send the same wire shape as before Tests: - 4 new bridge tests (override single→thread, override thread→single, mixed-scope leak regression, invalid-value rejection) - 3 new server tests (valid passthrough, invalid 400, omitted backward compat) - 2 new SDK tests (forwards/omits sessionScope on the wire) - EXPECTED_STAGE1_FEATURES updated for the new capability tag 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in, per-request sessionScope override to POST /session in qwen serve, allowing clients to request either shared ('single') or isolated ('thread') session semantics against a daemon-wide default. This resolves the prior “daemon-wide only” limitation while keeping backward compatibility when the field is omitted.
Changes:
- Extend the HTTP route + bridge spawn logic to accept/validate an optional
sessionScopeand resolve an effective scope per request. - Fix a mixed-scope leak by ensuring thread-scoped spawns never claim the single-scope attach slot.
- Advertise a new capability feature tag (
session_scope_override) and add unit coverage across server, bridge, and SDK client.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-typescript/src/daemon/DaemonClient.ts | Adds CreateSessionRequest.sessionScope? and conditionally includes it in POST /session body. |
| packages/sdk-typescript/test/unit/DaemonClient.test.ts | Verifies sessionScope is forwarded when provided and omitted from the wire shape when absent. |
| packages/cli/src/serve/server.ts | Validates sessionScope at the route boundary and forwards it to the bridge when present; returns typed 400 invalid_session_scope on invalid values. |
| packages/cli/src/serve/server.test.ts | Adds feature registry expectation for session_scope_override and covers route pass-through / 400 / omission behavior. |
| packages/cli/src/serve/httpAcpBridge.ts | Adds per-request scope handling, re-validates defensively, threads effectiveScope into spawn, and prevents thread spawns from populating defaultEntry. |
| packages/cli/src/serve/httpAcpBridge.test.ts | Adds regression + behavior tests for per-request overrides and the mixed-scope leak fix. |
| packages/cli/src/serve/capabilities.ts | Registers session_scope_override in the serve capability registry. |
| docs/developers/qwen-serve-protocol.md | Documents the new capability tag and POST /session request field + error behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
📋 Review SummaryThis PR (#4209) implements per-request 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
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. |
Three independent review passes found three real issues:
1. Bridge `TypeError` on invalid `sessionScope` collapsed to opaque 500
in `sendBridgeError` instead of the typed `400 invalid_session_scope`
the route layer guarantees. Direct embed / test / future entry-point
callers bypassing the route would see a generic 500 with stack noise
on stderr — disagreeing with the route contract.
Fix: add `InvalidSessionScopeError` class (alongside
`SessionNotFoundError` / `WorkspaceMismatchError` /
`SessionLimitExceededError`); the `spawnOrAttach` validator now
throws it, and `sendBridgeError` translates to the same
`{ error, code: 'invalid_session_scope' }` shape.
2. SDK `DaemonClient.createOrAttachSession` used a truthy check
(`req.sessionScope ? ...`) for the conditional spread, silently
erasing falsy-but-defined values (`''`, `null`, `0`) on the wire.
A buggy caller would never see the daemon's 400 — it'd inherit the
daemon-wide default while believing it requested a specific scope.
Fix: use `!== undefined` (matching the bridge's own validation
shape). Same fix to the server-side spread for consistency.
3. JSDoc and docs referenced `serve --sessionScope` as if it were a
shipping CLI flag. It isn't — `ServeOptions` has no field, neither
`runQwenServe` nor `serve.ts` plumbs one, and the production daemon
default is hardcoded to `'single'`. Strike the references; note
that #4175 may add the flag in a follow-up.
Test coverage expanded:
- Cap-bypass guard: per-request `'thread'` overrides cannot bypass
`maxSessions` on a daemon-default-`'single'` deployment. Without
this, a future refactor that gated the cap on `defaultSessionScope`
instead of `effectiveScope` would silently let `'thread'` overrides
amplify past the limit — the exact N-amplification cliff #3803 was
about.
- Symmetric mixed-scope leak: daemon-default-`'thread'` +
single-first-call followed by omitted-scope-second-call must produce
distinct sessions. Mirrors the existing daemon-default-`'single'` +
thread-first leak regression.
- Concurrent mixed-scope coalescing: simultaneous single + thread
`spawnOrAttach` against the same workspace under slow `initialize`
must not collide on `inFlightSpawns` (tracker keys differ by scope).
- Updated invalid-scope rejection test to assert
`InvalidSessionScopeError` instance + carried `sessionScope` field.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
requested changes
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] integration-tests/cli/qwen-serve-routes.test.ts:187 still asserts the old capabilities list without session_scope_override, but the production registry now advertises that feature. Running npx vitest run integration-tests/cli/qwen-serve-routes.test.ts on this PR fails at this assertion because the received list includes the new entry. Please update the expected list and the test name/count.
[Suggestion] docs/users/qwen-serve.md:193 still lists per-request sessionScope override as a Stage 1.5+ blocker that is not available and says the daemon-wide default is the only setting. This PR implements and advertises that override, so downstream integrators reading the main user guide will get the opposite guidance from /capabilities and the developer protocol docs. Please remove or reword this blocker entry and add a short user-facing note near the POST /session examples.
— gpt-5.5 via Qwen Code /review
| @@ -279,6 +294,7 @@ export class DaemonClient { | |||
| body: JSON.stringify({ | |||
Collaborator
There was a problem hiding this comment.
[Suggestion] This truthy guard drops defined but invalid values such as sessionScope: '' before they reach the daemon. The new SDK doc says anything other than 'single' or 'thread' yields 400 invalid_session_scope, and the server route validates malformed values, but SDK callers using plain JS or as any would silently fall back to the daemon default instead of seeing the validation error.
Suggested change
| body: JSON.stringify({ | |
| ...(req.sessionScope !== undefined | |
| ? { sessionScope: req.sessionScope } | |
| : {}), |
— 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 high-confidence issues found. LGTM! ✅
Minor low-confidence observations for human consideration (not blocking):
doSpawndoes not log theeffectiveScopedecision — operators cannot distinguish thread-scope from single-scope session exhaustion via stderr logsinFlightSpawnsbriefly double-counts a session during the microtask window betweenbyId.set()andfinallycleanup (conservative off-by-one, not a real risk)- Bridge-layer
TypeErroron invalid scope maps to 500 viasendBridgeError(unreachable via HTTP today; future defense-in-depth improvement) defaultEntry/byIdinvariant is implicit — future cleanup paths could miss one
The code is well-structured, thoroughly tested (9 new tests, 214/214 passing), backward-compatible, and addresses the mixed-scope leak correctly.
— 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! ✅ — DeepSeek/deepseek-v4-pro via Qwen Code /review
This was referenced May 16, 2026
wenshao
pushed a commit
that referenced
this pull request
May 16, 2026
…e override (#4214) 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)
This was referenced May 17, 2026
Merged
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
Wave 2 PR 5 of issue #4175 — resolves the FIXME at
httpAcpBridge.ts:BridgeOptions.sessionScope(originally raised in #3803).sessionScopeis no longer a daemon-wide-only setting; clients can override it per request onPOST /session.'thread') can ask for it against a default-'single'daemon, and vice versa.session_scope_overrideadvertised on/capabilities.featuresfor negotiation.sessionScope: 'thread'to surface the P1 MCP N×M amplification before M2 fixes it.Wire change
Invalid value →
400 { "code": "invalid_session_scope" }. Omitted → exact pre-PR behavior.Implementation notes
BridgeSpawnRequest.sessionScope?field;spawnOrAttachvalidates and resolveseffectiveScope = req.sessionScope ?? defaultSessionScopedoSpawnnow takeseffectiveScopeand only stampsdefaultEntry(the single-scope attach slot) when the spawn is single-scope. This fixes a mixed-scope leak found in review: pre-fix, a'thread'-first call would let a later omitted-scope call attach to the supposedly-isolated session.CreateSessionRequest.sessionScope?field;createOrAttachSessionconditionally spreads it into the JSON body so omitted callers send the same wire shape as before.Engineering principles checklist (#4175 PR template)
bridge.calls[0]shape tests on both server and SDK sides).'single'); override is opt-in per request.400 invalid_session_scopeis new (and only fires on a body field that didn't exist before).caps.features.session_scope_overridelets clients pre-flight before sending the field.Test plan
npx vitest run packages/cli/src/serve packages/sdk-typescript/test/unit/DaemonClient.test.ts— 237 / 237 passing locally (macOS arm64, node v24.12.0)npx tsc -p packages/cli --noEmitcleannpx tsc -p packages/sdk-typescript --noEmitcleannpx eslint packages/cli/src/serve packages/sdk-typescript/src/daemoncleanhttpAcpBridge.test.ts:432) — thread-first call followed by omitted-scope call must produce DISTINCT sessionsintegration-tests/cli/qwen-serve-routes.test.tsis needed (current coverage is unit + SDK unit)Tests added
httpAcpBridge.test.ts'thread'overrides daemon-wide'single'; per-request'single'overrides daemon-wide'thread'; thread-first does NOT pollute single attach slot (regression for review-found leak); invalid scope rejectionserver.test.ts400 invalid_session_scope; omitted scope = pre-PR wire shapeDaemonClient.test.ts(SDK unit)sessionScopewhen supplied (both values); omits the field from the body when absentRelated
'thread'and re-capture baseline-stage-1.json honestly🤖 Generated with Qwen Code