feat(serve): POST /session/:id/compress + POST /session/:id/_meta (T1.3 + T1.4 from #4514)#4516
feat(serve): POST /session/:id/compress + POST /session/:id/_meta (T1.3 + T1.4 from #4514)#4516doudouOUC wants to merge 3 commits into
Conversation
….3 + T1.4 from #4514) Closes the manual compaction + per-session metadata routes called out as Tier-1 S-sized gaps in the daemon backlog inventory (#4514). ## T1.3 — POST /session/:id/compress Daemon-driven manual compaction equivalent to TUI /compress. Forwards through a new `qwen/control/session/compress` ACP extMethod into the agent's `GeminiClient.tryCompressChat(force=true)`. Returns token counts + compression status + durationMs synchronously, and publishes a `session_compacted` SSE event ONLY when `compressionStatus !== 'NOOP'` (NOOP would falsely bump the reducer's `sessionCompactedCount`). Two-tier concurrency guard: - 409 `compaction_in_flight` — another compress is mid-LLM-call. - 409 `prompt_in_flight` — a prompt is active; daemon refuses to race the agent's own pre-send `tryCompress` inside `sendMessageStream`. Non-strict mutation gate (parity with /prompt). 180s timeout. AbortSignal propagation deferred to a follow-up; operators wait or `killSession`. ## T1.4 — POST /session/:id/_meta + GET /session/:id/_meta Daemon-side per-session KV bag for channel adapters (IM bot routing context: chat_id, sender_id, thread_id). Replaces the wrapper-layer improvisation today. - Routes are daemon-side only — no ACP roundtrip; in-memory map on `SessionEntry` evicted by `byId.delete(sessionId)` in close/kill. - NOT injected into LLM prompt in v1; surfaced via the GET route, echoed on `GET /session/:id/context` (`state.meta`, always present even when `{}` so old-daemon-vs-empty-bag is unambiguous), and pushed via `session_meta_changed` SSE event on every write (carries the FULL bag, not a diff, so subscribers converge regardless of Last-Event-ID gaps). - Key regex `^[a-zA-Z][a-zA-Z0-9_.-]{0,63}$`; reserved `qwen.` prefix; total serialized cap 8 KB; `null` values under `merge:true` SET the key (do NOT delete — per-key DELETE deferred). - Not persisted across daemon restart in v1 (sessions restored via load/resume come back with `meta: {}`). ## Wire surface - 2 new capability tags: `session_compress`, `session_meta`. - 1 new ACP control extMethod: `qwen/control/session/compress`. - 2 new SSE event types: `session_compacted`, `session_meta_changed`. - 6 new SDK type exports re-exported from both barrels (result types + data shapes + envelopes). ## Tests - 15 acp-bridge tests (5 T1.3 + 10 T1.4) - 15 server route tests (6 T1.3 + 9 T1.4) - 7 DaemonClient tests (3 T1.3 + 4 T1.4) - 1 DaemonSessionClient wrapper test (covers all 3 forwarders) - 5 reducer/narrow tests (2 T1.3 + 3 T1.4) - 1 public-surface test (asserts all 6 type re-exports) - Updated EXPECTED_*_FEATURES fixtures with the 2 new tags. ## Docs - `docs/developers/qwen-serve-protocol.md`: 3 new subsections (compress, POST meta, GET meta) + capability list bump + `state.meta` echo on the context section. - `docs/users/qwen-serve.md`: 2 new bullets under "What it gives you" + updated §363 wire-event caveat (compress + meta now DO publish events). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
📋 Review SummaryThis PR implements two Tier-1 S-sized routes from the daemon capability backlog (#4514): manual session compression ( 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
compressionStatus: ('COMPRESSED' | 'NOOP' | 'COMPRESSION_FAILED_INFLATED_TOKEN_COUNT' | 'COMPRESSION_FAILED_OTHER') & {};
🔵 Low
"state": {
"meta": {}
}
✅ Highlights
|
There was a problem hiding this comment.
Pull request overview
Adds two new Tier-1 qwen serve session-scoped HTTP surfaces—manual session compaction and a daemon-side per-session metadata bag—wired end-to-end across the CLI daemon, ACP bridge, TypeScript SDK, events/reducer support, tests, and docs.
Changes:
- Add
POST /session/:id/compress(+session_compresscapability), forwarding through a new ACP extMethod and emittingsession_compactedSSE events only when history actually changes. - Add
POST /session/:id/_meta+GET /session/:id/_meta(+session_metacapability), with validation, size limits,session_meta_changedSSE events, andstate.metaechoed onGET /session/:id/context. - Extend the TypeScript SDK with new client helpers, event typing/reducer support, and public barrel exports; add broad unit/integration test coverage and update docs.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/serve/server.ts | Registers the new /compress and /_meta routes and maps new bridge errors to stable HTTP responses. |
| packages/cli/src/serve/server.test.ts | Adds route-level tests for /compress and /_meta, and updates expected capability tag fixtures. |
| packages/cli/src/serve/httpAcpBridge.ts | Re-exports newly introduced bridge error classes for server route mapping. |
| packages/cli/src/serve/capabilities.ts | Advertises new always-on capability tags: session_compress, session_meta. |
| packages/cli/src/acp-integration/acpAgent.ts | Implements ACP extMethod handler for session compression via the agent’s GeminiClient.tryCompressChat(force=true). |
| packages/acp-bridge/src/status.ts | Extends session context status type to optionally include daemon-side state.meta. |
| packages/acp-bridge/src/bridgeTypes.ts | Adds compressSession, setSessionMeta, and getSessionMeta to the HttpAcpBridge interface contract. |
| packages/acp-bridge/src/bridgeErrors.ts | Introduces typed errors for compress concurrency and _meta validation/size violations. |
| packages/acp-bridge/src/bridge.ts | Implements compress concurrency fencing + SSE publish, _meta in-memory storage, validation, SSE publish, and context meta splice-in. |
| packages/acp-bridge/src/bridge.test.ts | Adds bridge-level tests for compress behavior/eventing and _meta validation/merge semantics/eventing. |
| packages/sdk-typescript/src/daemon/DaemonClient.ts | Adds compressSession, setSessionMeta, and getSessionMeta HTTP helpers. |
| packages/sdk-typescript/src/daemon/DaemonSessionClient.ts | Adds session-bound wrappers: compress(), setMeta(), getMeta(). |
| packages/sdk-typescript/src/daemon/events.ts | Adds event types (session_compacted, session_meta_changed), guards, reducer state/counters, and union wiring. |
| packages/sdk-typescript/src/daemon/types.ts | Adds DaemonCompressSessionResult and DaemonSessionMetaResult result shapes. |
| packages/sdk-typescript/src/daemon/index.ts | Re-exports the new daemon types/events from the daemon barrel. |
| packages/sdk-typescript/src/index.ts | Re-exports new types from the published top-level SDK barrel. |
| packages/sdk-typescript/test/unit/DaemonClient.test.ts | Adds unit tests for the new DaemonClient methods and error passthrough. |
| packages/sdk-typescript/test/unit/DaemonSessionClient.test.ts | Adds unit tests ensuring session-bound wrappers forward sessionId and X-Qwen-Client-Id. |
| packages/sdk-typescript/test/unit/daemonEvents.test.ts | Adds reducer coverage for session_compacted and session_meta_changed events. |
| packages/sdk-typescript/test/unit/daemon-public-surface.test.ts | Adds type-only assertions ensuring new exports are present in the published entrypoint. |
| docs/developers/qwen-serve-protocol.md | Documents new routes, capabilities, state.meta echo semantics, and event contracts. |
| docs/users/qwen-serve.md | Adds user-facing bullets describing compress + per-session _meta behavior and event publication. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `compressionStatus` is the string name of core's `CompressionStatus` numeric enum: `'COMPRESSED'`, `'NOOP'`, or one of the `COMPRESSION_FAILED_*` flavors (the bridge translates `*_FAILED_*` to HTTP 500 below before the route returns). Typed as bare string in the SDK so future enum extensions don't crash older consumers. | ||
|
|
| - `404` — session unknown (`SessionNotFoundError`). | ||
| - `409 {code: 'compaction_in_flight', sessionId}` — another compress is already mid-LLM-call on this session. The chat history is single-threaded; concurrent compresses would race `setHistory`. Retry after a short delay. | ||
| - `409 {code: 'prompt_in_flight', sessionId}` — a prompt is currently active (`entry.activePromptOriginatorClientId` is set). The agent's own pre-send `tryCompress` inside `sendMessageStream` would race a daemon-driven compress on the same chat object. Retry after the prompt completes (subscribe to terminal `session_update` frames). **v1 limitation**: this only fences compress START; a prompt that arrives AFTER `compressInFlight` is set can still trigger the agent's pre-send path. In practice that path either NOOPs (history already compressed) or harmlessly re-compresses — hard prompt-side serialization is deferred to a follow-up. | ||
| - `500 {code: 'compress_failed', compressionStatus: '...'}` — `tryCompressChat` reported a `COMPRESSION_FAILED_*` status or threw an uncaught exception (transport / auth / OOM mid-side-query). `compressionStatus` carries the specific failure flavor when available. |
| const current = config.getApprovalMode(); | ||
| return { previous, current }; | ||
| } | ||
| case SERVE_CONTROL_EXT_METHODS.sessionCompress: { |
There was a problem hiding this comment.
[Critical] The entire sessionCompress extMethod handler (~75 lines of new code) has zero test coverage in acpAgent.test.ts. Untested branches include:
sessionIdmissing or non-string →invalidParamserrorsession.getConfig().getGeminiClient()returns null →NO_CLIENTRequestErrortryCompressChatreturns a*_FAILED_*status → translated tocompress_failedRequestErrortryCompressChatthrows a non-RequestError exception → re-wrapped as RequestError- Existing RequestError passes through untouched
CompressionStatus[info.compressionStatus] ?? 'UNKNOWN'fallback path
This is the agent-side half of the compress feature. The bridge and route layers are well-tested (198 + 270 tests), but the code that actually calls tryCompressChat and translates its result into a wire response is entirely uncovered. A regression in enum reverse-lookup, error re-wrapping, or the NO_CLIENT guard would silently reach production.
Suggested fix: Add describe('extMethod qwen/control/session/compress', ...) to acpAgent.test.ts covering at minimum: success (COMPRESSED + NOOP), missing sessionId, null GeminiClient, FAILED status translation, thrown-error re-wrap, and RequestError passthrough.
— qwen3.7-max via Qwen Code /review
| throw new CompactionInFlightError(sessionId); | ||
| } | ||
| if (entry.activePromptOriginatorClientId !== undefined) { | ||
| throw new PromptInFlightError(sessionId); |
There was a problem hiding this comment.
[Suggestion] Several specific branches in the compress/meta surface lack test coverage:
-
PromptInFlightErrorbranch (this line) —bridge.test.tscoversCompactionInFlightErrorbut never setsactivePromptOriginatorClientIdto exercise this path. The server.test.ts test only proves the route maps a thrown error correctly, not that the bridge detects the condition. -
getSessionContextStatusmeta splice — no test verifies that a priorsetSessionMetawrite is visible in the context status response, or that the defensiveentry?.meta ?? {}fallback works when the entry is gone between ACP roundtrip and splice. -
500 path on
POST /session/:id/compress— the route test block covers 200, 409, and 404 but not the upstream failure path wherecompressSessionImplthrows. -
Compress timeout path — no test exercises
withTimeoutfiring on the compress call (180s backstop). This is the most dangerous failure mode sincecompressInFlightclears but the agent'stryCompressChatcontinues running.
Suggested fix: Add targeted tests for each gap. For the timeout path, use fake timers with a short timeout override.
— qwen3.7-max via Qwen Code /review
| sessionId: string, | ||
| opts?: { clientId?: string }, | ||
| ): Promise<DaemonCompressSessionResult> { | ||
| return await this.fetchWithTimeout( |
There was a problem hiding this comment.
[Critical] SDK timeout mismatch: compressSession() uses the default 30s fetch timeout (DEFAULT_FETCH_TIMEOUT_MS), but the daemon has a 180s timeout (SESSION_COMPRESS_TIMEOUT_MS). Any compression lasting >30s will timeout on the SDK side while the daemon continues working with compressInFlight=true, causing all retries to fail with 409 compaction_in_flight for up to 150 seconds. This creates a zombie window where the operator has no way to know if the original compress is still running, when it will finish, or whether it succeeded.
| return await this.fetchWithTimeout( | |
| return await this.fetchWithTimeout( | |
| `${this.baseUrl}/session/${encodeURIComponent(sessionId)}/compress`, | |
| { | |
| method: 'POST', | |
| headers: this.headers( | |
| { 'Content-Type': 'application/json' }, | |
| opts?.clientId, | |
| ), | |
| body: '{}', | |
| }, | |
| async (res) => { | |
| if (!res.ok) { | |
| throw await this.failOnError(res, 'POST /session/:id/compress'); | |
| } | |
| return (await res.json()) as DaemonCompressSessionResult; | |
| }, | |
| 190_000, // Match daemon's 180s timeout + buffer | |
| ); |
— qwen3.7-max via Qwen Code /review
| SESSION_META_MAX_BYTES, | ||
| ); | ||
| } | ||
| entry.meta = next; |
There was a problem hiding this comment.
[Critical] Mutation before authorization: entry.meta = next and entry.metaSerializedByteSize = byteSize are assigned BEFORE resolveTrustedClientId() is called. If the caller supplies an unregistered clientId, the authorization check throws InvalidClientIdError but the meta bag has already been mutated. No session_meta_changed SSE event is published (that happens after the throw), so legitimate subscribers never learn about the poisoning. Subsequent GET /session/:id/_meta or GET /session/:id/context from legitimate clients returns the poisoned data.
The JSDoc above this function states the intended contract: "Validation order (fail-fast, before any mutation)" — but the implementation violates it. Compare with compressSession in the same file, which correctly calls resolveTrustedClientId before setting entry.compressInFlight = true.
| entry.meta = next; | |
| // Move authorization BEFORE mutation | |
| const originatorClientId = resolveTrustedClientId( | |
| entry, | |
| context?.clientId, | |
| ); | |
| entry.meta = next; | |
| entry.metaSerializedByteSize = byteSize; | |
| const changeKind: 'replace' | 'merge' = merge ? 'merge' : 'replace'; |
— qwen3.7-max via Qwen Code /review
Addresses 11 findings from the first deep review pass on PR #4516 (2 critical, 6 important, 3 suggestions). All fixes are bounded by the review report; deeper refactors (comment redundancy cleanup, T1.x prefix removal, audit logging) are parked for follow-up. ## Critical fixes ### C1 — setSessionMeta mutates state BEFORE clientId validation Move `resolveTrustedClientId` BEFORE the `entry.meta = next` write. Without this, a bad `X-Qwen-Client-Id` header left the bag changed, no event fired, and the caller saw a 400 — silent state mutation. `setSessionApprovalMode` and `updateSessionMetadata` both resolve clientId first; the pattern is now consistent. ### C2 — compressInFlight clears on timeout while underlying op keeps running Detach the flag-clearing from the `Promise.race` outer settlement; attach to the underlying `extPromise.finally` instead. `withTimeout` rejects the outer race but does NOT cancel the agent's extMethod (same FIXME as `setSessionModel`). Clearing the flag on outer-race rejection let a second compress race the first's still-pending `setHistory` — the exact corruption the flag was designed to prevent. Trade-off: a wedged extPromise (agent gone forever) leaves the flag set; recover via `killSession`. ## Important fixes ### I1 — typed CompressFailedError + sendBridgeError mapping The agent throws `RequestError({errorKind: 'compress_failed'})`; without a reconstruction the catch-all 500 leaked the JSON-RPC `code: -32004` instead of the documented `code: 'compress_failed'`. Add `CompressFailedError` class + bridge-side reconstruction (mirrors `TrustGateError` at `setSessionApprovalMode`) + `sendBridgeError` mapping. SDK consumers can now branch on `body.code === 'compress_failed'` per the protocol docs. ### I5 — reject unknown CompressionStatus in acpAgent Tighten `statusName.includes('FAILED')` to `statusName.startsWith('COMPRESSION_FAILED_')` and reject `statusName === undefined` (future enum extension or malformed core response). Previously, an unknown status slipped through as success AND emitted a misleading `session_compacted` event (bridge publishes for anything `!== 'NOOP'`). ### I6 — throw on session-died race in getSessionContextStatus Replace defensive `?? {}` fallback with a hard `SessionNotFoundError` when the entry disappears between the ACP roundtrip and the meta splice. The fallback was a false-positive 200 that race-aware reducers would record as "meta was cleared then session disappeared." ### I7 — skip publish on identity meta write Gate the `session_meta_changed` publish on actual change (byteSize + serialized-equality check). Mirrors `updateSessionMetadata`'s no-op gate so identity writes don't spam SSE subscribers and bump the SDK reducer's `sessionMetaChangedCount`. ### I9 — split DaemonSessionMetaResult → Snapshot + WriteResult Bridge interface already separates these inline (GET omits `changeKind`, POST requires it); collapse the SDK shape into a single `changeKind?: ...` optional was strictly weaker. Split into `DaemonSessionMetaSnapshot` (GET) and `DaemonSessionMetaWriteResult` (POST, extends snapshot with required `changeKind`). Legacy `DaemonSessionMetaResult` retained as deprecated union alias for one SDK major. ### I10 — widen compressionStatus to literal | (string & {}) `compressionStatus: string` is forward-compat but loses IDE completion on `'COMPRESSED'` / `'NOOP'`. Switch to `'COMPRESSED' | 'NOOP' | (string & {})` matching the existing `DaemonAuthProviderId` / `scope` pattern in the same file. NOOP in particular is load-bearing — it's the bridge's event-suppression gate. ## Test coverage additions (I2 + I3 + identity-write + reconstruction) - **I2** `state.meta` echo on `/context`: real test (not toMatchObject trivial-pass) asserts `meta: {}` on fresh session and the stored bag after a write. - **I3** Bridge-level `PromptInFlightError` test: starts a hanging prompt via `pendingPrompt: true` on `compressFactory`, then asserts `compressSession` rejects with `PromptInFlightError`. - **I6** `SessionNotFoundError` on `/context` session-died race: kills the session mid-call (using a controllable held promise) and asserts the throw fires. - **I1** `CompressFailedError` reconstruction: agent throws a real ACP `RequestError`; assert the bridge re-types as `CompressFailedError` with the right `compressionStatus`. - **I7** identity-write no-op: 3 identical writes → exactly 1 SSE event published. - **C1** bad-clientId rejection: write with unregistered clientId throws `InvalidClientIdError` AND leaves the bag unchanged. ## Suggestion fixes ### S1 — substring → startsWith `statusName.includes('FAILED')` → `statusName.startsWith('COMPRESSION_FAILED_')` in acpAgent compress handler. ### S2 — setHistory comment location Two JSDoc references corrected: `setHistory` actually lives in `GeminiChat.tryCompress`, not in `tryCompressChat` (which is the GeminiClient wrapper). ### S4 — "always present + optional" contradiction `ServeSessionContextStatus.state.meta` JSDoc rephrased to make clear the optional `?` covers pre-T1.4 daemons specifically (which omit the field), while T1.4+ daemons always populate it. ## Parked for follow-up - I4 (acpAgent isolated unit tests) — bridge-level coverage already exercises the wire shape end-to-end via `compressFactory`; an isolated test scaffold for the agent extMethod handler has high cost for marginal benefit. - I8 (operator audit log on meta writes) — defensive, can be a small follow-up. - S3, S5, S6, S7 (comment redundancy cleanup, T1.x prefix removal, 6 KB warning) — cosmetic / scope creep. ## Test counts - acp-bridge: 198 → 204 tests passing (+6) - cli serve: 270 tests passing (unchanged count; assertions strengthened) - sdk-typescript: 238 tests passing (unchanged count; public-surface test now asserts 8 names instead of 6) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Addresses 5 findings from the second deep review pass on PR #4516 (0 critical, 3 important, 2 suggestions). Remaining round-2 suggestions (key-reorder dedup false-negative, undefined-value asymmetry, wedged-extPromise memory retention, BridgeTimeoutError mapping) parked as known trade-offs or pre-existing. ## Important fixes ### R2-1 — server-level test for CompressFailedError → 500 mapping Bridge-level reconstruction was covered, but `server.ts:3022-3030` `sendBridgeError` mapping had no end-to-end test. A regression that deleted the `if (err instanceof CompressFailedError)` branch would silently route through the catch-all 500 with JSON-RPC `code: -32603`, breaking SDK consumers branching on `body.code === 'compress_failed'`. Added a test using `compressSessionImpl` that throws CompressFailedError and asserts the 500 body shape. ### R2-2 — UNKNOWN fallback path is now covered `acpAgent.ts:2333` outer catch throws `RequestError(-32004, message, {errorKind: 'compress_failed'})` WITHOUT `compressionStatus` whenever `tryCompressChat` raises a non-RequestError (transport / auth / OOM mid-side-query). The bridge's `'UNKNOWN'` fallback fires in real production but had no test. Added a bridge-level test that throws a RequestError sans compressionStatus and asserts the reconstructed CompressFailedError carries `compressionStatus: 'UNKNOWN'` + preserves the original message. ### R2-3 — doc bump: sessionId in 500 compress_failed body `docs/developers/qwen-serve-protocol.md:1298` documented `500 {code, compressionStatus}` but actual code includes `sessionId` (matching the convention of the 409 bodies above). Updated the doc to list the full field set including the UNKNOWN fallback path. ## Suggestion fixes ### R2-S1 — tighten diagnostic for non-numeric CompressionStatus `acpAgent.ts` UNKNOWN branch. `String(info.compressionStatus)` produces `'null'` / `'undefined'` / `'[object Object]'` for off-contract core responses — useless diagnostics. Tightened to: ``` typeof rawStatus === 'number' ? String(rawStatus) : `(non-numeric: ${typeof rawStatus})` ``` Operators now see e.g. `compressionStatus: '(non-numeric: object)'` when core returns a malformed shape. ### R2-S3 — C1 test fences "no event emitted" too The original C1 test (bad-clientId rejects) only verified the bag stays `{}`. A regression where `entry.events.publish(...)` ran BEFORE `entry.meta = next` (and exception-rolled back) would have passed. Added an event subscription + post-attempt `.filter(e => type === 'session_meta_changed').toHaveLength(0)` assertion so the event-not-fired half of the silent-mutation regression is also fenced. ### R2-S2 — drop stale line ref `bridge.ts:3180` C2 JSDoc referenced `setSessionModel at line ~2904`; the actual line is now 2908 (drift). Dropped the specific line number and pointed to the canonical `FIXME(stage-2)` block via grep instead — survives future refactors. ## Test counts - acp-bridge: 204 → 205 tests passing (+1 UNKNOWN fallback) - cli serve: 270 → 271 tests passing (+1 R2-1 500 mapping) - sdk-typescript: 238 tests passing (unchanged) ## Parked round-2 items (intentional) - **R2-S4** identity-write merge-mode coverage gap — gate is mode- agnostic by inspection; tight but acceptable. - **R2-S5** synchronous-throw in extMethod could strand flag — theoretical (ACP SDK is async); not worth the defensive wrap. - **Key-reorder false-negative dedup** ({a:1,b:2} vs {b:2,a:1} fires extra event) — byte-equal, not value-equal; documented. - **undefined-value JSON.stringify asymmetry** — HTTP callers can't produce undefined; documented. - **Wedged-extPromise memory retention** — trade-off in C2 JSDoc; recovers via killSession. - **BridgeTimeoutError mapping in sendBridgeError** — pre-existing gap across all routes; separate follow-up. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
left a comment
There was a problem hiding this comment.
Round 2 review at ca570660e. The two R1 Critical findings (SDK timeout mismatch, mutation-before-auth) have been addressed — setSessionMeta now resolves clientId before mutation (C1 fix), confirmed by code and tests.
5 new inline Suggestions below (test gaps + robustness). The R1 Critical test-coverage gap on acpAgent.ts:2248 is still open (skipped here — existing comment at same line).
Build/Test: typecheck clean (3 packages), 443/443 tests pass (205 bridge + 271 server + 238 SDK). CI: no checks reported.
— qwen3.7-max via Qwen Code /review
| // NOOP = below-threshold, history identical — do NOT publish. | ||
| // See JSDoc on the interface method for the rationale. | ||
| if (response.compressionStatus !== 'NOOP') { | ||
| entry.events.publish({ |
There was a problem hiding this comment.
[Suggestion] Both session_compacted (this line) and session_meta_changed (line 3328) publish without the try { ... } catch { /* bus closed */ } wrapper used by every other entry.events.publish site in this file (see lines 2492, 2567, 2977, 2992, 3088). If the session's SSE bus closes between the extMethod resolution and the publish (e.g. a concurrent killSession), publish() throws and the route converts a successful compress (history already rewritten inside the agent) into a 500. For setSessionMeta, the state mutation (entry.meta = next) happens two lines before the publish, so a throwing publish aborts the route with a 500 AFTER the daemon's state has already changed.
| entry.events.publish({ | |
| if (response.compressionStatus !== 'NOOP') { | |
| try { | |
| entry.events.publish({ | |
| type: 'session_compacted', | |
| data: result, | |
| ...(originatorClientId ? { originatorClientId } : {}), | |
| }); | |
| } catch { | |
| /* bus closed */ | |
| } | |
| } |
Apply the same try/catch pattern at line 3328 for session_meta_changed.
— qwen3.7-max via Qwen Code /review
| // keeps the flag set until the agent op truly settles. The | ||
| // `.catch(() => {})` defuses unhandled-rejection noise if the | ||
| // extPromise rejects after the outer race already rejected. | ||
| const extPromise = entry.connection.extMethod( |
There was a problem hiding this comment.
[Suggestion] If entry.connection.extMethod(...) throws synchronously (before returning a promise — e.g. a connection-disposed precondition check), extPromise is never assigned, the .finally() that clears compressInFlight is never attached, and the flag stays true permanently. Every subsequent compress on this session will 409 with CompactionInFlightError until the daemon restarts or the session is killed.
| const extPromise = entry.connection.extMethod( | |
| let extPromise: ReturnType<typeof entry.connection.extMethod>; | |
| try { | |
| extPromise = entry.connection.extMethod( | |
| SERVE_CONTROL_EXT_METHODS.sessionCompress, | |
| { sessionId }, | |
| ); | |
| } catch (err) { | |
| entry.compressInFlight = false; | |
| throw err; | |
| } | |
| extPromise | |
| .finally(() => { | |
| entry.compressInFlight = false; | |
| }) | |
| .catch(() => {}); |
— qwen3.7-max via Qwen Code /review
| response = (await Promise.race([ | ||
| withTimeout( | ||
| extPromise, | ||
| SESSION_COMPRESS_TIMEOUT_MS, |
There was a problem hiding this comment.
[Suggestion] The SESSION_COMPRESS_TIMEOUT_MS (180s) race arm has no test coverage. Bridge tests cover concurrent refusal, error reconstruction, and async-rejection finally-clearing, but no test exercises the withTimeout path. The C2 comment (line 3183) documents a load-bearing invariant — flag-clear coupled to extPromise.finally, NOT the outer Promise.race settlement — with no test pinning it. A future refactor moving the finally to the outer try/finally would silently reintroduce the setHistory race the C2 fix prevents.
Consider adding a test with a delayed extMethodImpl that exceeds a test-local timeout: verify (1) the call rejects, (2) a second compress still gets CompactionInFlightError (flag still set), (3) after the delayed promise settles, a third compress succeeds (flag cleared by extPromise.finally).
— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| describe('compressSession (T1.3 / #4514)', () => { |
There was a problem hiding this comment.
[Suggestion] The compressSession test suite covers success, clientId forwarding, and 409 compaction_in_flight. Three other error shapes from the server lack SDK-level tests:
409 { code: 'prompt_in_flight' }— callers need the distinct code to choose the right retry strategy500 { code: 'compress_failed', compressionStatus: '...' }— SDK should propagatecompressionStatusfor diagnostic branching404session not found — basic error path
Without these tests, a regression in the SDK's error parsing (e.g. stripping code or normalizing both 409s to the same shape) would go undetected. Consider adding test cases mirroring the existing 409 test, each with a recordingFetch returning the corresponding error shape.
— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| describe('POST/GET /session/:id/_meta (T1.4 / #4514)', () => { |
There was a problem hiding this comment.
[Nice to have] The _meta test suite covers the happy path for both POST and GET, but no test covers the 404 response when setSessionMeta or getSessionMeta throws SessionNotFoundError. The compress route tests do cover this 404 path, but the _meta routes' sendBridgeError mapping for dead sessions is untested end-to-end. Consider adding tests where setSessionMetaImpl/getSessionMetaImpl throw new SessionNotFoundError(sessionId), then assert 404.
— qwen3.7-max via Qwen Code /review
|
Closing this for now after re-triaging #4514. Two parts of this PR are now classified differently:
So this PR is valid as optional convenience/product work, but it should not stay open as a Tier-1 required daemon capability gap. We can reopen or split a smaller PR later if a real downstream client needs daemon-owned session metadata or structured compress semantics. |
Summary
Closes the two Tier-1 S-sized routes called out in the daemon capability backlog #4514 (claimed in this comment):
POST /session/:id/compress— manual compaction over HTTP, equivalent to TUI/compress. Mutates session history.POST /session/:id/_meta+GET /session/:id/_meta— daemon-side per-session KV bag for IM/channel adapters (chat_id, sender_id, thread_id).Bundled as ONE PR because both are S-sized session-mutation routes touching the same surface (status / bridgeTypes / bridge / server / capabilities / events / SDK / barrels / tests / docs) and share the mutation-gate plumbing. Template: shipped
POST /session/:id/approval-mode(Wave 4 PR 17).T1.3 design — POST /session/:id/compress
qwen/control/session/compressACP extMethod into the agent'sGeminiClient.tryCompressChat(force=true). Alwaysforce=trueserver-side (matches TUI); accepts empty{}body — noforcefield in v1 schema.{sessionId, originalTokenCount, newTokenCount, compressionStatus, durationMs}.compressionStatusis the string name of core'sCompressionStatusenum ('COMPRESSED','NOOP','COMPRESSION_FAILED_*').session_compactedSSE event ONLY whencompressionStatus !== 'NOOP'— NOOP means below-threshold, history unchanged; emitting would falsely bump the reducer'ssessionCompactedCount.CompactionInFlightError→ HTTP 409compaction_in_flightwhen another compress is mid-LLM-call.PromptInFlightError→ HTTP 409prompt_in_flightwhenentry.activePromptOriginatorClientIdis set; the agent's own pre-sendtryCompressinsidesendMessageStreamwould race the daemon-driven call on the same chat object./promptand feat(serve): add POST /session/:id/recap #4504/recaptemplate).SESSION_COMPRESS_TIMEOUT_MS). AbortSignal propagation deferred to a follow-up — placeholder signal in v1; operators wait orkillSession.T1.4 design — POST/GET /session/:id/_meta
SessionEntryevicted automatically bybyId.delete(sessionId)in close/kill.GET /session/:id/context(state.meta, always present even when{}when thesession_metacapability tag is advertised — avoids old-daemon-vs-empty-bag ambiguity), and pushed viasession_meta_changedSSE event on every write.^[a-zA-Z][a-zA-Z0-9_.-]{0,63}$→ 400invalid_meta_key.qwen.prefix → 400reserved_meta_key.meta_too_large.nullvalues undermerge:trueSET the key to null (per JSON semantics); per-key DELETE deferred.meta: {}.Wire surface added
session_compress,session_meta.qwen/control/session/compress.session_compacted,session_meta_changed.DaemonCompressSessionResult,DaemonSessionMetaResult(result types)DaemonSessionCompactedData,DaemonSessionMetaChangedData(event data shapes)DaemonSessionCompactedEvent,DaemonSessionMetaChangedEvent(envelopes)DaemonClient.compressSession(sessionId, {clientId?})DaemonClient.setSessionMeta(sessionId, {meta, merge?}, {clientId?})DaemonClient.getSessionMeta(sessionId, {clientId?})DaemonSessionClient.compress()/.setMeta(meta, {merge?})/.getMeta()Out of scope (parked follow-ups)
_metainto LLM prompt context (needs pilot to validate prompt format)._metaacross daemon restart (revisit when T2.1 graduatesunstable_session_resumeto stable).DELETE /_meta/:key(replace-with-fewer-keys covers v1)._meta(last-write-wins in v1).Test plan
npm run typecheck --workspace packages/acp-bridge --workspace packages/cli --workspace packages/sdk-typescript— all greenpackages/acp-bridge/src/bridge.test.ts— 198 tests passing (15 new: 5 T1.3 + 10 T1.4)packages/cli/src/serve/server.test.ts— 270 tests passing (15 new: 6 T1.3 + 9 T1.4) +EXPECTED_*_FEATURESfixtures bumped for the two new capability tagspackages/sdk-typescript/test/unit/{DaemonClient,DaemonSessionClient,daemonEvents,daemon-public-surface}.test.ts— 238 tests passing (14 new: 7 DaemonClient + 1 DaemonSessionClient + 5 reducer/narrow + 1 public-surface)qwen serve --token=dev-token:curl -N, verifysession_compacted+session_meta_changedframes arrive.compaction_in_flight.Docs
docs/developers/qwen-serve-protocol.md— 3 new subsections (compress, POST meta, GET meta) + capability list bump +state.metaecho on the context section.docs/users/qwen-serve.md— 2 new bullets under "What it gives you" + updated §363 wire-event caveat (compress + meta now DO publish events).🤖 Generated with Qwen Code