feat(serve): add daemon-stamped client identity#4231
Merged
Conversation
Contributor
📋 Review SummaryThis PR implements daemon-stamped client identity for 🔍 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. |
Collaborator
Author
|
发现一个需要在合入前补齐的验证缺口：PR7 新增了 我本地在当前 stacked 工作区跑了： npm run build && npm run bundle && cd integration-tests && QWEN_SANDBOX=false npx vitest run cli/qwen-serve-routes.test.ts -t "capabilities envelope"结果确认失败：运行时返回包含 建议：PR7 层先把 integration expected features 同步到包含 Generated by GPT-5 model |
wenshao
reviewed
May 17, 2026
| ...(body as object), | ||
| outcome, | ||
| } as Parameters<HttpAcpBridge['respondToPermission']>[1], | ||
| clientId !== undefined ? { clientId } : undefined, |
Collaborator
There was a problem hiding this comment.
Critical: Permission route returns 500 instead of 400 for InvalidClientIdError
This PR passes clientId context into bridge.respondToPermission(), which can now throw InvalidClientIdError (via resolveTrustedClientId). However, the surrounding catch block (unchanged in this PR) only handles InvalidPermissionOptionError and re-throws everything else via throw err → Express default 500.
Every other state-changing route (/prompt, /cancel, /model) uses sendBridgeError(res, err, ...) which correctly maps InvalidClientIdError → 400 with code: "invalid_client_id". The permission route is the sole outlier.
Suggested fix: Replace the bare throw err at the end of the catch with sendBridgeError(res, err, { route: "POST /permission/:requestId" }), consistent with all other routes.
Reviewed by mimo-v2.5-pro
wenshao
reviewed
May 17, 2026
| ), | ||
| ).rejects.toBeInstanceOf(InvalidClientIdError); | ||
| await bridge.shutdown(); | ||
| }); |
Collaborator
There was a problem hiding this comment.
Suggestion: Add bridge-level InvalidClientIdError tests for sendPrompt and cancelSession
This test covers setSessionModel rejecting unregistered client IDs — great. However, sendPrompt and cancelSession also call resolveTrustedClientId and throw InvalidClientIdError, but lack equivalent bridge-level tests. The server-layer tests cover the prompt error-path wiring ("400 invalid_client_id when the bridge rejects prompt originator"), but not the cancel or permission-vote routes.
Adding parallel tests for sendPrompt and cancelSession rejecting { clientId: 'client-not-issued' } would complete the enforcement test matrix and guard against future regressions in these identity-validation paths.
Reviewed by mimo-v2.5-pro
Collaborator
PR Review: feat(serve): add daemon-stamped client identityReviewed by: mimo-v2.5-pro SummaryWell-structured implementation of daemon-stamped client identity. The trust model is sound: daemon generates opaque Findings
Verification
|
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Solid foundational PR — daemon-controlled stamping, header-only echo, unknown-id rejection on state-changing routes all look right. Inline notes below are mostly correctness/consistency follow-ups; the clientIds shrink path and the DaemonSessionClient reconnect-identity gap are the two I'd most like to see closed before adapter PRs start integrating against this primitive.
| promptQueue: Promise.resolve(), | ||
| modelChangeQueue: Promise.resolve(), | ||
| pendingPermissionIds: new Set(), | ||
| clientIds: new Set(), |
Collaborator
There was a problem hiding this comment.
clientIds has no shrink path on detach. detachClient (around line 2787) only decrements attachCount; it never removes the corresponding id from this set. Under sessionScope: 'single' with frequent reconnects (TUI cycling HTTP/2 streams, flaky web clients re-attaching), the set accumulates ~43 B per attach indefinitely until the session is killed.
Not catastrophic — ~25k attaches ≈ 1 MB — but theoretically unbounded. Suggest threading the issued clientId back into detachClient(sessionId, clientId) and calling entry.clientIds.delete(clientId) to mirror the attachCount-- symmetry. OK as a follow-up if you'd rather not expand the bridge contract in this PR.
Comment on lines
+2409
to
+2423
| const previousOriginator = entry.activePromptOriginatorClientId; | ||
| if (originatorClientId === undefined) { | ||
| delete entry.activePromptOriginatorClientId; | ||
| } else { | ||
| entry.activePromptOriginatorClientId = originatorClientId; | ||
| } | ||
| const promptPromise = entry.connection | ||
| .prompt(normalized) | ||
| .finally(() => { | ||
| if (previousOriginator === undefined) { | ||
| delete entry.activePromptOriginatorClientId; | ||
| } else { | ||
| entry.activePromptOriginatorClientId = previousOriginator; | ||
| } | ||
| }); |
Collaborator
There was a problem hiding this comment.
The save/restore stacking here only matches a world where prompts can nest. They can't — entry.promptQueue.then(...) strictly serializes them. Under FIFO, previousOriginator is always undefined, so the conditional branch is dead.
There's also a subtle race in the transport-closed path: racedPromise settles on transportClosed, but promptPromise.finally(restore) does NOT fire until the underlying connection.prompt() settles (against a dead agent it may never). Anything that queues a new prompt before the stale .finally() runs would read the prior originator as its previousOriginator; when the stale .finally() eventually fires it can clobber the new value mid-prompt.
In practice the entry is reaped on channel.exited, so this is mostly defensive. The cleanest fix is to drop the stacking and unconditionally delete entry.activePromptOriginatorClientId in the finally — matches what FIFO actually guarantees, removes the race window. Or keep it and add a comment that this is FIFO-safe defensive code so a future refactor doesn't mis-read it as concurrent-prompt-safe.
| async cancelSession(sessionId, req, context) { | ||
| const entry = byId.get(sessionId); | ||
| if (!entry) throw new SessionNotFoundError(sessionId); | ||
| resolveTrustedClientId(entry, context?.clientId); |
Collaborator
There was a problem hiding this comment.
Validates the id but doesn't propagate it — the cascading permission_resolved events fired by cancelPendingForSession end up with no originator, and the agent-side connection.cancel notification below isn't stamped either. Probably intentional ("system cancelled" vs "client voted"), but a one-line comment here would prevent a future reader from reading this as an oversight.
Comment on lines
+2538
to
+2545
| const pending = pendingPermissions.get(requestId); | ||
| let originatorClientId: string | undefined; | ||
| if (pending && context?.clientId !== undefined) { | ||
| const entry = byId.get(pending.sessionId); | ||
| if (entry) { | ||
| originatorClientId = resolveTrustedClientId(entry, context.clientId); | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
When the requestId is unknown (pending undefined), the client-id check is skipped entirely. An attacker can probe with arbitrary X-Qwen-Client-Id values without ever triggering InvalidClientIdError — the 404 (accepted: false) gives no signal about clientId validity.
Inconsistent with every other state-changing route, which validates eagerly. Minor info-disclosure surface, but worth tightening for symmetry — e.g. always validate against any registered set when a context.clientId is supplied, regardless of whether pending resolved.
Comment on lines
+138
to
+140
| get clientId(): string | undefined { | ||
| return this.session.clientId; | ||
| } |
Collaborator
There was a problem hiding this comment.
The getter is here, but DaemonSessionClient.createOrAttach / load / resume (above, lines 71–124) never accept or forward a clientId arg — they always call the raw DaemonClient without one. The effect: every reconnect via this wrapper mints a NEW server-side id, so an adapter that wants to suppress echoes of its own events across reconnects can't preserve identity through this layer.
The raw DaemonClient.createOrAttachSession does accept clientId, so the plumbing's already there. Either (a) accept an optional clientId on createOrAttach/load/resume here and thread it through, or (b) explicitly document that the wrapper is reconnect-fresh-id-only and persistent-identity adapters must drop down to DaemonClient. Whichever you pick, the follow-up adapter PRs (TUI/IDE/web) will need to know which surface to integrate against.
This is the gap I'd most like to see closed in this PR — it directly affects how the primitive gets adopted downstream.
Comment on lines
+1001
to
+1014
| function parseClientIdHeader( | ||
| req: import('express').Request, | ||
| res: import('express').Response, | ||
| ): string | undefined | null { | ||
| const raw = req.get(CLIENT_ID_HEADER); | ||
| if (raw === undefined || raw === '') return undefined; | ||
| if (raw.length > MAX_CLIENT_ID_LENGTH || !CLIENT_ID_RE.test(raw)) { | ||
| res.status(400).json({ | ||
| error: | ||
| '`X-Qwen-Client-Id` must be a non-empty token of 128 characters or fewer', | ||
| code: 'invalid_client_id', | ||
| }); | ||
| return null; | ||
| } |
Collaborator
There was a problem hiding this comment.
The three-state return (string | undefined | null) — null means "validation failed, response already sent", undefined means "no header", string means valid — is functional but easy to mis-handle. Every call site needs if (clientId === null) return; followed by clientId !== undefined ? { clientId } : undefined. Current call sites are all correct, but a small refactor (e.g. return { ok: true; clientId?: string } | { ok: false }, or have the function take a callback) would make accidental misuse harder. Optional.
wenshao
previously approved these changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — thorough implementation of daemon-stamped client identity.
Reviewed 10 files (+672/-63) across bridge, server routes, SDK client, and tests. Ran 9 parallel review agents covering correctness, security, code quality, performance, test coverage, and three undirected audit personas. Key observations:
- Client ID generation uses
randomUUID()withclient_prefix — 122 bits of entropy, unguessable - Header validation is strict (regex
[A-Za-z0-9._:-]{1,128}, prototype pollution keys blocked in body parser) resolveTrustedClientIdcorrectly rejects unknown IDs on state-changing routes while allowing no-header (backward compat)- Originator save/restore around prompt execution uses
.finally()for proper cleanup - All 271 tests pass (bridge + server + SDK)
Minor low-confidence suggestions (not blocking):
- Test coverage gaps:
sendPromptoriginator save/restore,cancelSessionwith invalid context,registerClientunknown-ID fallthrough,permission_requestoriginator stamping loadSession/resumeSessioninacpAgent.tsshare an identical setup sequence- Protocol docs (qwen-serve-protocol.md) don't yet document the
client_identitycapability
— glm-5.1 via Qwen Code /review
Collaborator
Author
|
已处理最新 review 中影响合入/后续 adapter 接入的点：
本地验证： cd packages/cli && npx vitest run src/serve/server.test.ts src/serve/httpAcpBridge.test.ts
cd packages/sdk-typescript && npx vitest run test/unit/DaemonSessionClient.test.ts
cd packages/cli && npm run typecheck
cd packages/sdk-typescript && npm run build
cd packages/sdk-typescript && npm run typecheck
cd packages/cli && npx eslint src/serve/server.ts src/serve/server.test.ts src/serve/httpAcpBridge.ts src/serve/httpAcpBridge.test.ts --max-warnings 0 --no-warn-ignored
npx eslint packages/sdk-typescript/src/daemon/DaemonSessionClient.ts packages/sdk-typescript/test/unit/DaemonSessionClient.test.ts integration-tests/cli/qwen-serve-routes.test.ts --max-warnings 0 --no-warn-ignored
npx prettier --check packages/cli/src/serve/server.ts packages/cli/src/serve/server.test.ts packages/cli/src/serve/httpAcpBridge.ts packages/cli/src/serve/httpAcpBridge.test.ts packages/sdk-typescript/src/daemon/DaemonSessionClient.ts packages/sdk-typescript/test/unit/DaemonSessionClient.test.ts integration-tests/cli/qwen-serve-routes.test.ts
npm run build && npm run bundle
cd integration-tests && QWEN_SANDBOX=false npx vitest run cli/qwen-serve-routes.test.ts -t "capabilities envelope"补充： Generated by GPT-5 model |
wenshao
approved these changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Re-approving after 52295980f4 fix(serve): harden daemon client identity handling. Strong response — addresses the actual Critical and six of the other seven concerns from my prior pass.
Verified addressed
- Critical: permission route returned 500 instead of 400 for
InvalidClientIdError(server.ts:648) — barethrow errreplaced withsendBridgeError(res, err, { route: 'POST /permission/:requestId' })matching every other route. New regression test400 invalid_client_id when the bridge rejects permission voter. DaemonSessionClientAPI gap (DaemonSessionClient.ts:71/91/120) —createOrAttach,load, andresumeall now accept an optionalclientIdand thread it through to the underlyingDaemonClient. New testforwards a persisted client id through create, load, and resume. This was the gap I was most worried about for downstream adapter adoption.clientIdsSet had no shrink path — refactored fromSet<string>toMap<string, number>with ref-counting, plus a newunregisterClienthelper anddetachClient(sessionId, clientId?). Two new tests:detachClient unregisters only the detached client idanddetachClient preserves an echoed client id owned by an earlier attach. The ref-count direction is actually nicer than my "just delete on detach" suggestion — concurrent attaches with the same echoed id no longer step on each other.- Dead save/restore stacking + transport-closed race (
httpAcpBridge.ts:2406) —previousOriginatorremoved entirely;finallyis now unconditionaldelete entry.activePromptOriginatorClientId, matching what FIFO actually guarantees. - Unknown-
requestIdclientId-validation skip (httpAcpBridge.ts:2545) — new helperresolveAnyTrustedClientIdvalidates against any registered client set even when no specificpendingresolved. New testrejects unknown permission votes with unregistered client ids. Closes the attacker-probe surface I'd flagged. cancelSessionvalidates but doesn't propagate clientId — explicit comment added at the validation site: "cancellation resolves permissions as system cancellations, so those generated events intentionally omit an originator client id." Documents the intent.
Partially addressed (non-blocking)
- The bridge-level
InvalidClientIdErrortest coverage now includes the permission path;sendPromptandcancelSessionstill lack dedicated bridge-level rejection tests, but the server-layer test400 invalid_client_id when the bridge rejects prompt originatoralready covers the wire-level enforcement for prompt. Worth a follow-up.
Acknowledged-and-skipped
- Three-state
string | undefined | nullreturn forvalidateClientIdHeader(server.ts:1014) — I explicitly marked this Optional; fine to leave.
Local verification on 52295980f4
httpAcpBridge.test.ts+eventBus.test.ts: 119 passed- SDK targeted vitest (
DaemonClient.test.ts+DaemonSessionClient.test.ts+daemonEvents.test.ts): 77 passed sdk-typescripttypecheck: cleantsc --noEmit -p packages/cli/tsconfig.jsonfiltered to PR-touched files: 0 errorsserver.test.tsdoesn't load locally (missingsupertestdep on this install — pre-existing env issue, CI ran it green on the prior commit and is re-running now)
Remote CI on the new commit is still IN_PROGRESS at time of writing (Classify PR is the only completed check); approving on substance, please wait for the matrix to come back before pulling the merge trigger.
LGTM.
This was referenced May 17, 2026
wenshao
reviewed
May 17, 2026
| } | ||
| sessionScope = rawSessionScope; | ||
| } | ||
| const clientId = parseClientIdHeader(req, res); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The parseClientIdHeader + clientId context construction pattern is repeated verbatim across 7 route handlers (create, load, resume, prompt, cancel, model, permission). Extracting a helper would reduce ~30 lines of duplication and ensure consistency:
Suggested change
| const clientId = parseClientIdHeader(req, res); | |
| function extractClientContext(req: Request, res: Response): BridgeClientRequestContext | null { | |
| const clientId = parseClientIdHeader(req, res); | |
| if (clientId === null) return null; | |
| return clientId !== undefined ? { clientId } : undefined; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
reviewed
May 17, 2026
| ...(entry.activePromptOriginatorClientId | ||
| ? { originatorClientId: entry.activePromptOriginatorClientId } | ||
| : {}), | ||
| }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] BridgeClient.requestPermission writes entry.activePromptOriginatorClientId into the originatorClientId field of permission_requested events, which are broadcast to all SSE subscribers of the session via the event bus. This makes client IDs public to every connected client sharing the session. While resolveTrustedClientId currently only checks clientIds.has() (not that the sender is the original recipient), this design means any client in a shared session can see and echo other clients' IDs for permission voting. Consider either documenting that clientId is not a secret in the client_identity capability docs, or stripping the field from subscriber-facing events while retaining it for internal audit.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
reviewed
May 17, 2026
| const CLIENT_ID_RE = /^[A-Za-z0-9._:-]+$/; | ||
|
|
||
| /** | ||
| * Coerce `req.body` into a safe `Record<string, unknown>` for route |
Collaborator
There was a problem hiding this comment.
[Suggestion] CLIENT_ID_RE = /^[A-Za-z0-9._:-]+$/ here and createClientId = () => 'client_' + randomUUID() in httpAcpBridge.ts:1337 are implicitly coupled — the UUID format (hex + dashes) matches the regex character class by coincidence, but this is not enforced by any shared constant or test. If createClientId changes to a different format (e.g., base64url), the daemon would reject its own generated IDs. Consider exporting a shared isValidClientId() helper from one location and importing it in the other.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
reviewed
May 17, 2026
| @@ -3270,6 +3339,43 @@ describe('createHttpAcpBridge', () => { | |||
| await bridge.shutdown(); | |||
Collaborator
There was a problem hiding this comment.
[Suggestion] Bridge-level rejection tests for cancelSession and sendPrompt with unregistered client IDs are missing. setSessionModel and respondToPermission have equivalent tests (e.g., rejects permission votes with unregistered client ids), but cancelSession and sendPrompt do not. The server-level sendPrompt test uses a fake bridge that throws a hardcoded error, bypassing the real resolveTrustedClientId logic. Additionally, request_permission, session_update, and model_switch_failed events now carry originatorClientId but are not assertion-tested for this field — only permission_resolved and model_switched events are tested. Consider adding: it('rejects cancel/sendPrompt with unregistered client id') and verifying originatorClientId on all four event types.
— 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.
Review Summary
No critical issues found — the PR correctly handles InvalidClientIdError in the permission route via sendBridgeError, and the cancelSession originator omission is intentional (documented as "system cancellations"). Posted 4 inline suggestions covering:
- Repeated boilerplate across 7 route handlers — extract
extractClientContexthelper - Client ID broadcast via
requestPermissionevents — document or strip from subscriber-facing events - Implicit format coupling between
CLIENT_ID_REandcreateClientId— share a validator - Test coverage gaps — missing bridge-level rejection tests for
cancelSession/sendPromptandoriginatorClientIdassertions on 3 event types
Build passes, all 271 PR-specific tests pass. LGTM for the core design — the identity primitive is sound.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
This was referenced May 17, 2026
Merged
doudouOUC
added a commit
that referenced
this pull request
May 17, 2026
Adds POST /session/:id/heartbeat plus SDK helpers so long-lived adapters (TUI/IDE/web) can refresh the daemon's last-seen bookkeeping. Bridge stores per-session and per-client timestamps behind a getHeartbeatState() snapshot accessor that PR 12 read-only diagnostics and PR 24 revocation policy will consume. - Capability tag: client_heartbeat (advertised on /capabilities.features) - Identified clients must echo X-Qwen-Client-Id; the bridge validates the id BEFORE bumping any timestamp so a forged id can't mask client absence - Per-client entries are dropped together with the registration ref-count in unregisterClient, so churn doesn't leak stale ids - getHeartbeatState returns a snapshot Map; mutating it does not leak into bridge state - Anonymous heartbeats bump only the per-session watermark Errors mirror the rest of the routes — 404 SessionNotFoundError, 400 invalid_client_id (header malformed or unknown for this session). Roadmap PR 9 from #4175. Depends on PR 7 (#4231 client identity, merged) for the trusted clientId registry. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
doudouOUC
added a commit
that referenced
this pull request
May 17, 2026
* feat(serve): add client heartbeat route Adds POST /session/:id/heartbeat plus SDK helpers so long-lived adapters (TUI/IDE/web) can refresh the daemon's last-seen bookkeeping. Bridge stores per-session and per-client timestamps behind a getHeartbeatState() snapshot accessor that PR 12 read-only diagnostics and PR 24 revocation policy will consume. - Capability tag: client_heartbeat (advertised on /capabilities.features) - Identified clients must echo X-Qwen-Client-Id; the bridge validates the id BEFORE bumping any timestamp so a forged id can't mask client absence - Per-client entries are dropped together with the registration ref-count in unregisterClient, so churn doesn't leak stale ids - getHeartbeatState returns a snapshot Map; mutating it does not leak into bridge state - Anonymous heartbeats bump only the per-session watermark Errors mirror the rest of the routes — 404 SessionNotFoundError, 400 invalid_client_id (header malformed or unknown for this session). Roadmap PR 9 from #4175. Depends on PR 7 (#4231 client identity, merged) for the trusted clientId registry. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * feat(sdk): re-export HeartbeatResult from package root The published @qwen-code/sdk only exposes the root entrypoint via `exports`; daemon subpath imports are not part of the public API. Adding HeartbeatResult to packages/sdk-typescript/src/daemon/index.ts made it reachable internally but not for downstream consumers writing `import type { HeartbeatResult } from '@qwen-code/sdk'` — every other daemon result type (PromptResult, SetModelResult, DaemonSession, etc.) is forwarded through the root barrel, so HeartbeatResult was the only hole in the heartbeat helper's public surface. Inserted alphabetically between DaemonStreamLifecycleEvent and KnownDaemonEvent to match the existing ordering convention.
wenshao
pushed a commit
that referenced
this pull request
May 17, 2026
Wave 2.5 PR 10) (#4237) * feat(serve): SSE replay sizing + slow_client_warning backpressure #4175 Wave 2.5 PR 10. Closes the SSE replay / backpressure knobs called out in #3803 §02 so chatty Stage 1 sessions get an honest reconnect window and operators get a heads-up signal before clients are summarily evicted. - **`DEFAULT_RING_SIZE` 4000 → 8000.** Per-session replay ring depth now matches the #3803 §02 target for chatty sessions. - **`--event-ring-size <n>`** CLI flag (default 8000) lets operators tune the ring per daemon. Threaded `ServeOptions` → `BridgeOptions.eventRingSize` → both `new EventBus()` construction sites (fresh sessions + restore path). Validation is fail-CLOSED (positive finite integer; 0 / NaN / negative throw at boot). - **`slow_client_warning` SSE frame.** When a subscriber's queue crosses 75% full the bus force-pushes a synthetic `slow_client_warning` to that subscriber once per overflow episode, carrying `{queueSize, maxQueued, lastEventId}`. The flag re-arms after the queue drains below 37.5% (hysteresis, no flap near threshold). If the queue actually overflows after the warning, the existing `client_evicted` terminal frame path still fires. Like `client_evicted`, the warning has no `id` (synthetic frame; must not burn a sequence slot for other subscribers). - **`?maxQueued=N`** query param on `GET /session/:id/events` (range `[16, 2048]`, default 256). Lets cold reconnect clients pre-size their per-subscriber backlog so a large `Last-Event-ID: 0` replay doesn't trip the warning on the first publish. Range rationale: lower bound 16 (smaller is useless for any replay); upper bound 2048 (so a single subscriber can't pin ~1 MB just by asking). Out-of-range / non-decimal returns `400 invalid_max_queued` BEFORE opening the SSE stream — clean 4xx beats half-opening a stream + emitting a `stream_error` (which EventSource would auto-reconnect on). - **`slow_client_warning` capability tag** — single source of truth for the warning frame + `?maxQueued` query param + ring-size knob. Old daemons silently lack all of these; pre-flight via `caps.features`. - **SDK extensions** (`@qwen-code/sdk`): typed `DaemonSlowClientWarningEvent` (added to known event union and `DaemonStreamLifecycleEvent`); schema-validated by a new `isSlowClientWarningData` predicate; reducer (`reduceDaemonSessionEvent`) increments `slowClientWarningCount` + stores `lastSlowClientWarning`. Warning is **non-terminal** — `alive` stays true (only `client_evicted` / `stream_error` / `session_died` close the stream). Re-exported from the public SDK entry. - **Docs**: `qwen-serve-protocol.md` updates the features list (adds `slow_client_warning` and the previously-missing `client_identity` to match reality post-#4231), documents the `?maxQueued` query param, adds the warning frame to the event table, and notes the new default ring size. `qwen-serve.md` adds the `--event-ring-size` flag row. Tests: 19 eventBus (4 new: warning at 75%, once per episode, no `id` on the synthetic frame, hysteresis re-arm), 106 bridge (2 new: validate eventRingSize accept/reject), 111 server (4 new: ?maxQueued accept/absent/non-decimal/out-of-range + EXPECTED_STAGE1_FEATURES update), 14 SDK daemonEvents (2 new: schema validation + non-terminal reducer behavior). 321 focused tests total, all green. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(serve): adopt PR #4237 review feedback (eventBus polish) Address the actionable items from the Qwen Code review bot's pass on PR #4237: - Pre-compute `warnThreshold` / `warnResetThreshold` per `InternalSub` at `subscribe()` time so `publish()`'s per-event hot path is one integer compare per subscriber instead of a multiply + compare. The `!warned` short-circuit still collapses the steady state to a single boolean read; this just shaves a multiply when the threshold check actually fires. - Document the back-of-queue ordering choice for the synthetic `slow_client_warning` frame in `EventBus.publish()`: front-push was considered but mid-stream front-insertion would mis-count `forcedInBuf` in `BoundedAsyncQueue.next()`, and `forcePush` already short-circuits via `resolvers.shift()` for the active-consumer case — the back-of-queue path only matters for stalled consumers, who can't drain regardless of warning position. - Reuse the existing `collect()` helper in the "default ring size 8000" test for consistency with the rest of the file; the new test also tightens the assertion by checking that the first retained event id is 2 (id=1 dropped by the ring) and the last is 8001. - Soften the "~500 B per session" magic number in `BridgeOptions.eventRingSize`'s JSDoc to a qualitative description (each retained `BridgeEvent` is a reference plus its serialized payload; ceiling scales as `ringSize × average-event-size`). Rejected: - Bot's claim that the error JSON contains `\`...\`` escape sequences — bot misread the JS template-literal source as the wire output; `JSON.stringify` does not escape backticks, and the existing `cwd` error messages use the same style. - Bot's "use `Record<string, never>` instead of `[key: string]: unknown`" suggestion on `DaemonSlowClientWarningData` — every other event-data type in `sdk-typescript/src/daemon/events.ts` carries the same index signature for additive-field compatibility. - Bot's "features list breaks alphabetical order" — the capability list is grouped by protocol lifecycle (health → capabilities → session lifecycle → events → permissions), not alphabetical. Tests: 139 focused tests across eventBus + httpAcpBridge + SDK daemon events — all passing. Behavior unchanged; this is hot-path micro-opt + comment polish only. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve): correct queue tagging + plumb maxQueued through SDK Address both P2 findings from the Codex review pass on PR #4237. **Bug 1: `BoundedAsyncQueue.forcedInBuf` position-invariant break** The previous `forcedInBuf` counter only tracked LIVE-vs-FORCED correctly when all forced entries lived at the FRONT of the buffer (subscribe-time `Last-Event-ID` replay). The new mid-stream `slow_client_warning` path force-pushes to the BACK of the queue while the queue is still open, which the existing accounting was not designed for: - publish 6 events at maxQueued=8 → 75% threshold trips → force-push warning at the back → buf=[1..6, warning], forcedInBuf=1. - consumer shifts `1` → forcedInBuf decremented to 0 (incorrect: `1` was a live frame, not the forced one). - consumer drains 2..6 + warning → buf=[], forcedInBuf=0, true live count = 0, but `size` getter and `push()` cap check then use `buf.length - forcedInBuf` which drifts over subsequent refills, causing premature warn / eviction before the cap is actually reached. Replace the position-dependent counter with a per-entry `{value, forced}` tag. `liveCount` is incremented in `push()` / decremented in `next()` only when the shifted entry was non-forced — position becomes irrelevant. `size` getter returns `liveCount` directly. The class doc comment is rewritten to call out that the new tag is the position-independent replacement for the old "forced frames must stay at the front" invariant. Regression test in `eventBus.test.ts` reproduces the codex trace (warn at 75%, drain past warning, refill to cap) and asserts no premature eviction. **Bug 2: SDK does not expose `?maxQueued`** `docs/users/qwen-serve.md` and `docs/developers/qwen-serve-protocol.md` both document `?maxQueued=N` as something SDK clients can request, but `SubscribeOptions` on `DaemonClient` only declared `lastEventId` + `signal`, and `subscribeEvents()` always fetched `/events` without a query string. Typed-SDK consumers had no way to opt in without hand-crafting URLs. - Add `SubscribeOptions.maxQueued?: number` with JSDoc noting the daemon range `[16, 2048]` and the pre-flight requirement on `caps.features.slow_client_warning`. - `DaemonClient.subscribeEvents` builds the URL with an optional `?maxQueued=<n>` segment. No client-side range validation — the daemon's `parseMaxQueuedQuery` is the source of truth and returns structured `400 invalid_max_queued`; duplicating the bounds in two layers would diverge on the next tweak. - `DaemonSessionSubscribeOptions extends SubscribeOptions` so the new field flows through `DaemonSessionClient` automatically. Three new SDK tests: - subscribeEvents appends `?maxQueued=N` when set - omits the query string when absent (existing behavior preserved) - propagates a `400 invalid_max_queued` unchanged Tests: 214 focused tests across eventBus / bridge / SDK DaemonClient / DaemonSessionClient / daemonEvents, plus 111 in the server suite. All green; the new eventBus regression case proves the position-invariant fix. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(serve): adopt PR #4237 copilot review feedback Address 6 of 8 copilot-reviewer findings on PR #4237; the other 2 (#1 forcedInBuf live-size corruption, #5 SDK lacks maxQueued) were already fixed in bae42c8 — replied on the threads with the commit hash. - **[2] server.ts:1068** — `?maxQueued=` (present-but-empty) now fails closed with `400 invalid_max_queued` instead of silently falling back to the default queue cap. The API documents fail-closed for any malformed value before opening SSE, so an empty string is unambiguously malformed. New server.test.ts case locks this in. - **[3] commands/serve.ts:93** — CLI help text for `--event-ring-size` no longer mis-shapes `Last-Event-ID` as a query parameter. It is an HTTP header, and the daemon's SSE route does not parse a `?Last-Event-ID=` query. - **[4] docs/developers/qwen-serve-protocol.md:351** — clarify that `?maxQueued=N` controls the LIVE-event backlog cap. Replay frames are force-pushed and exempt from the cap; what consumes it is live events that arrive while the subscriber is still draining a cold-reconnect replay. Bumping for cold reconnects is still the right answer, but for the live tail, not for the replay frames themselves. - **[6] eventBus.ts:214** — stale `ringSize=4000` performance comment updated to the new `ringSize=8000` default with a note about the O(n) `shift()` cost scaling. - **[7] sdk-typescript events.ts:492** — `isSlowClientWarningData` now uses the existing `isFiniteNumber` helper instead of bare `typeof === 'number'`. Mirrors the sibling predicates and rejects `NaN` / `Infinity` payloads as schema garbage. New daemonEvents.test.ts assertions cover both. - **[8] server.ts:127** — `createServeApp`'s default-bridge construction now also forwards `opts.eventRingSize` to `createHttpAcpBridge`, symmetric with the `runQwenServe.ts` path. Direct embeds / tests that called `createServeApp` without supplying their own bridge but did pass `ServeOptions.eventRingSize` were silently getting the default 8000 ring. Tests: 326 focused tests across eventBus / bridge / SDK DaemonClient / DaemonSessionClient / daemonEvents / server. All green; the new server.test.ts case + the extended daemonEvents.test.ts assertions cover the tightened guards. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(serve): adopt PR #4237 wenshao round-2 review feedback Six adopted findings from @wenshao's second review pass on PR #4237. The seventh ([10] forcedInBuf 3rd case invariant) was already fixed in bae42c8 — replied on that thread. - **[9] + [14] server.ts** — Sanitize attacker-controlled values before stderr interpolation in both `parseMaxQueuedQuery` and `parseLastEventId`. New `safeLogValue()` helper uses `JSON.stringify` to escape control characters (`\n`/`\r`/…) so a URL-encoded newline in `?maxQueued=%0a` can't inject extra log lines into journald/Loki/Splunk pipelines. Matches the `workspace_mismatch` sanitization style in `sendBridgeError`. Fixed in both helpers (the sibling pre-existing `parseLastEventId` had the same shape) so the file stays consistent. - **[11] httpAcpBridge.ts** — `!Number.isFinite(eventRingSize)` was redundant: `Number.isInteger(NaN)` and `Number.isInteger(Infinity)` both return `false`, so the sibling `!Number.isInteger` already catches both. Drop the dead guard. - **[12] httpAcpBridge.ts** — Add soft upper bound `MAX_EVENT_RING_SIZE = 1_000_000` on `eventRingSize` to catch operator typos (`--event-ring-size 80000000` vs `8000000`). At ~500 B per `BridgeEvent` an 1M-frame ring already pins ~500 MB per session — well past any realistic workload. Not a security boundary (operator-controlled flag), pure typo defense. Existing bridge construction test extended with an `80_000_000` case. - **[13] commands/serve.ts** — CLI `--event-ring-size` flag now sources its default from `DEFAULT_RING_SIZE` (imported from `serve/eventBus.js`) instead of the hardcoded literal `8000`. Without this, a future bump of the bus default would silently not take effect for daemons launched through the CLI because the flag always overrides — single source of truth fixes that. - **[15] eventBus.ts** — Drop unreachable `event.id ?? this.lastEventId` fallback in the `slow_client_warning` frame. `event` is locally constructed at the top of `publish()` with `id: this.nextId++` and is guaranteed defined. Use `event.id as number` directly + an inline note about the invariant. Tests: 197 (eventBus 20 / bridge 107 / SDK DaemonClient 57 / SDK daemonEvents 14) + 112 server. All green; the new upper-bound bridge case + the existing log assertions pin the changed behaviors. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
46 tasks
doudouOUC
added a commit
that referenced
this pull request
May 20, 2026
…4335) 6 follow-up findings from wenshao Round 8 review #4326742064 (state: COMMENTED — not blocking but addresses leftover risk surfaces). **[Suggestion] Legacy `respondToPermission` info leak** (3272493777). Round 7 closed the cross-session client-registration oracle on the session-scoped vote route, but the legacy workspace-level route (`POST /permission/<requestId>`) still called `resolveAnyTrustedClientId` on unknown-requestId paths, throwing `InvalidClientIdError` (400) for unregistered clientIds and returning false (404) for registered ones — the same oracle. The PR #4231 reasoning ("preserve security boundary") was inverted: the 400-vs-404 distinction WAS the leak. Removed the call, deleted the now-unused `resolveAnyTrustedClientId` helper, and updated the previously-leak-asserting test (`rejects unknown permission votes with unregistered client ids`) to assert the new uniform `false` behavior across all 3 input shapes (unregistered / registered / no-clientId). **[Suggestion] Error-precedence regression test gap + observability inconsistency** (3272493792). Two parts: - Added regression test `returns false (not InvalidClientIdError) when session exists but requestId is unknown and clientId is unregistered` to lock the Round-7 fix against future refactors. - Promoted the error-precedence guard's stderr line from debug-gated `writeServeDebugLine` to unconditional `writeStderrLine`, matching the `writeForbiddenStderr` posture in the mediator. Operators tailing stderr at 3 AM no longer need `QWEN_SERVE_DEBUG=1` to see unexpected 404s on the permission endpoint. **[Suggestion] Settings description "UNANIMITY for even M" was factually wrong** (3272493795). `floor(M/2)+1` equals M only when M=2; for M=4 it gives 3 (supermajority), M=6 gives 4 (~67%). The mediator's own unanimity warning correctly fires only when M=2. Settings description now reads "UNANIMITY for M=2 (quorum=2, both must agree) and supermajority for larger even M (M=4 → quorum=3; M=6 → quorum=4)". VSCode JSON schema regenerated. **[Suggestion] runQwenServe.ts inline policy unions** (3272493805). Same drift-protection rationale as the types.ts fix in Round 7. Imported `PermissionPolicy` from `@qwen-code/acp-bridge`, replaced 3 inline unions: the `let` declaration, the `as` cast, and the `VALID_PERMISSION_POLICIES` Set construction. Used a typed-array + Set<string> pattern (drift caught at array construction; runtime Set keeps `.has(string)` ergonomics). **[Suggestion] InvalidPolicyConfigError discrimination needs positive tests** (3272493818). Extracted the inline `policyConfig`-validation logic into an exported `validatePolicyConfig(policyConfig, onWarning?)` helper and exported `InvalidPolicyConfigError` itself. Added 7 unit tests covering: empty config, all 4 valid literals, invalid literal throws (with class identity check + message regex), 4 non-positive-integer quorum cases throw, valid combination returns, mismatch (consensusQuorum + non-consensus strategy) emits warning without throwing, no-warning happy path, and error messages name the failed field. The boot path in `runQwenServe` now delegates to the helper (one call site, DRY). **[Suggestion] Unanimity breadcrumb spammed per-request** (3272493829). The Round-7 unanimity stderr line fires inside the synchronous Promise executor of every `request()` call, which for a 2-client consensus session is EVERY permission request (M=2 unanimity is the normal operating mode, not a rare edge). Added `unanimityBreadcrumbEmitted` boolean to the mediator class (per-mediator dedup, parallel to `consensusQuorumCapNoted` on `MediatorPending`). One emit per daemon lifetime — visible at boot, silent thereafter. Comment also corrects the "for even M" generalization to "for M=2" specifically, matching the actual condition (`floor(M/2)+1 === M` only for M=1 and M=2). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
doudouOUC
added a commit
that referenced
this pull request
May 20, 2026
…4335) * feat(acp-bridge): F3 — multi-client permission coordination (#4175) [rebased onto F1] Squashed F3 implementation rebased from origin/main onto daemon_mode_b_main (post-F1 #4319). F1 lifted the bridge core to @qwen-code/acp-bridge package; F3's edits to the pre-F1 httpAcpBridge.ts BridgeClient class + factory were ported to the new file locations: - BridgeClient.requestPermission rewrite → bridgeClient.ts - Factory mediator construction / pendingPermissions deletion / cancelPendingForSession refactor / respondTo*Permission rewrites / pendingPermissionCount + permissionPolicy getters / teardown sites (closeSession, killSession, shutdown drain) → bridge.ts - Error class re-exports → cli/src/serve/httpAcpBridge.ts shim (added CancelSentinelCollisionError, PermissionForbiddenError, PermissionPolicyNotImplementedError to the F1 re-export block) This commit folds 13 logical F3 commits + 4 review fold-ins (Copilot inline comments + 3 final-pass agent reviews) into a single post-rebase squash. The full review trail is in .claude/plans/fluttering-coalescing-kettle*.md (worktree-local). Strategies (4): first-responder (default, byte-for-byte preserved), designated, consensus (default N=floor(M/2)+1), local-only. New SSE events: permission_partial_vote, permission_forbidden. Capability tag: permission_mediation (always-on with build-supported modes list); active policy at /capabilities.policy.permission. Settings: policy.permissionStrategy enum + policy.consensusQuorum number, both requiresRestart: true (F3 v1 reads at boot). 3 new typed errors: PermissionForbiddenError → 403, PermissionPolicyNotImplementedError → 501 (forward-compat for future policy literals), CancelSentinelCollisionError → 500 (agent / daemon contract violation). Hardness invariants: N1 synchronous-register, N2 cleanup ordering, N3 originatorClientId stamping, O5 cancel sentinel pre-publish collision check, O8 pre-F3 permission_resolved wire shape preserved. Tests: 35 mediator unit + 10 audit ring + 56 SDK reducer + 6 bridgeClient + 3 bridge integration. Pre-existing httpAcpBridge.test.ts cross-session-vote suite passes byte-for-byte. Issue: #4175 (F3) * fix(f3): build/capability fixes from Copilot review (#4335) - packages/sdk-typescript/src/daemon/index.ts: re-export the four F3 permission event types (`DaemonPermissionForbiddenData/Event`, `DaemonPermissionPartialVoteData/Event`) so the public package barrel at `src/index.ts` (which forwards them via `from './daemon/index.js'`) resolves at build time. Without this fix `npm run build --workspace=packages/sdk-typescript` failed with TS2305/TS2724; vitest passed only because it resolves TS source via tsx and bypasses tsc compilation. Reported in PR #4335 review comments 3270615836 / 3270622302 (wenshao via Qwen Code /review). - packages/cli/src/serve/server.test.ts: append `'permission_mediation'` to `EXPECTED_STAGE1_FEATURES` and adjust `EXPECTED_REGISTERED_FEATURES` reordering so the test fixture matches the registry's actual order (`...workspace_mcp_restart, require_auth, auth_device_flow, permission_mediation`). Without this fix four `serve capability registry` tests asserted via `.toEqual` against a stale list. - docs/developers/qwen-serve-protocol.md: swap `permission_mediation` and `auth_device_flow` in the documented capability list so the order mirrors `SERVE_CAPABILITY_REGISTRY` declaration order. - packages/vscode-ide-companion/schemas/settings.schema.json: regenerate the IDE-companion JSON schema with the new `policy` section (was pending from Commit 5 of the F3 series; checked in here so the IDE companion sees the same `permissionStrategy` / `consensusQuorum` shape that the CLI accepts). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): wire production audit ring + restore timeout stderr (#4335) Wenshao review #4335 surfaced two related Critical findings: 1. **Audit publisher silently no-op in production** (3270622298). The `bridgeOptions.ts:305` JSDoc claimed "the bridge allocates an internal `PermissionAuditRing`" but the actual fallback at `bridge.ts:543` is `createNoOpPermissionAuditPublisher()`, and `runQwenServe.ts` never wired one. All 5 audit record types (`requested`, `voted`, `forbidden`, `resolved`, `timeout`) were silently discarded — the forensic audit trail the F3 plan committed to ("ring 留给后续 PR 加查询接口") never existed in any deployed daemon. 2. **Timeout breadcrumb lost** (3270622304). Pre-F3 wrote `"timed out after Xms"` to daemon stderr on every permission timeout. F3 removed that direct write and delegated to `audit.recordTimeout()`, but the audit publisher is the no-op fallback in production (see #1). Operators tailing daemon stderr could no longer observe permission timeouts. Fixes: - `runQwenServe.ts` allocates a `PermissionAuditRing` (default cap 512) + `createPermissionAuditPublisher` and passes the publisher via `BridgeOptions.permissionAudit`. The ring is held in the daemon host's closure for the lifetime of the daemon — a future `GET /workspace/permission/audit` route (out of F3 v1 scope) can lift it out for query without further bridge changes. - `permissionMediator.ts` writes the stderr breadcrumb directly from the timer callback, before forwarding to the (potentially no-op) audit publisher. Wrapped in try/catch because `process.stderr.write` can synchronously throw on EPIPE — losing observability is preferable to crashing the timer queue. - `bridgeOptions.ts` JSDoc rewritten to match reality: the bridge falls back to a no-op publisher; production wiring lives in `runQwenServe.ts`; the stderr breadcrumb is in the mediator (independent of the publisher). - New unit test `writes a stderr breadcrumb when the timer fires` spies on `process.stderr.write` and asserts the breadcrumb format contains the requestId, sessionId, and the timeout duration so future refactors can't silently drop the line again. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): drop dead helper + propagate originator to F3 view state (#4335) Two small follow-ups from wenshao review #4335: - **`bridge.ts:672-682` — dead `_resolutionToAcpResponse` helper** (3270622309). Defined and immediately suppressed with `void`. The identical `resolutionToAcpResponse` lives at `bridgeClient.ts:41` and is the one actually used by `BridgeClient.requestPermission` — the bridge-factory copy was a stranded leftover from the lift out of inline closures into the mediator pattern. Removed declaration, `void` statement, and the now-unused `RequestPermissionResponse` (`@agentclientprotocol/sdk`) and `PermissionResolution` (`./permission.js`) imports. - **SDK reducer `mergeOriginator` for F3 events** (3270622311). The mediator stamps `originatorClientId` (= prompt originator per N3) on the `permission_partial_vote` / `permission_forbidden` envelope, but the reducer cases used `next.push({ ...event.data })` which only copies `data` fields. SDK consumers reading `permissionVoteProgress[reqId]` / `forbiddenVotes[i]` could not determine which client's prompt was targeted by the partial-vote progress / forbidden vote — same gap PR #4282 fixed for approval-mode / tool-toggle / workspace-init / mcp-restart. Applied the existing `mergeOriginator` helper to both reducer cases. Added `originatorClientId?: string` to both Data interfaces with JSDoc explaining the propagation contract (preserve any pre-existing `data.originatorClientId`; otherwise stamp from the envelope; for forbidden votes the field is distinct from `data.clientId` which carries the rejected voter). Three new reducer tests: 1. `permission_partial_vote` propagates envelope originator into `permissionVoteProgress`. 2. `permission_forbidden` propagates envelope originator into `forbiddenVotes`, distinct from `data.clientId`. 3. `mergeOriginator` preserves any pre-existing `data.originatorClientId` over the envelope value. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): wenshao Round 4 — defensive stderr, audit accuracy, orphan cleanup (#4335) Four findings from wenshao review #4324937255 — the Critical one masked an actual hang scenario; the other three are observability / correctness fixes that round out F3 v1. **[Critical] safeEmit / safeAudit stderr breadcrumb wraps** (3271041461). Both helpers wrote `process.stderr.write` inside their `catch` block WITHOUT a nested `try/catch`. If stderr itself synchronously throws (EPIPE during daemon shutdown), the exception escapes the "safe" wrapper. In `resolveEntry`'s cleanup ladder (`safeEmit → rememberResolved → safeAudit → pending.resolve`), an escaping safeEmit exception aborts before `pending.resolve(resolution)` runs — the request was already deleted from `this.pending` (no double-resolve guard), so the agent's awaiting Promise never settles. `requestPermission` hangs until the timeout fires. The timer callback already wraps its breadcrumb in `try/catch` for the same reason — applied the matching pattern to safeEmit + safeAudit. **[Suggestion] Idempotent re-vote audit shows attempted optionId, not the original** (3271041464). When `client_A` originally voted for `proceed_once` and later attempts `proceed_always`, the tally silently keeps `proceed_once` (idempotent) but the audit ring recorded `optionId: proceed_always`. An operator reading the ring would see a vote for proceed_always that never counted toward quorum. Look up the originally-voted option from the tally and substitute it into the audit record. Added regression test asserting the audit reflects tally state. **[Suggestion] SDK reducer leaks `permissionVoteProgress` on mid-permission reconnect** (3271041465). When an SDK client reconnects and misses `permission_request`, then receives `permission_partial_vote` (stored in `permissionVoteProgress`), then receives `permission_resolved` — the early-return path on unmatched `requestId` did NOT clear `permissionVoteProgress`. The orphan progress entry persisted until session end. Both `permission_resolved` and `permission_already_resolved` reducer cases now unconditionally clear any orphan entry on the unmatched path. Two new reducer tests cover the recovery contract; the misleading "the next `permission_resolved` will clear both" comment on `permission_partial_vote` is corrected. **[Suggestion] Document votersAtIssue snapshot timing window** (3271041469). The snapshot fires synchronously after `entry.events.publish`, with no event-loop yield between, so a NEW HTTP client cannot register between publish and snapshot. But an SSE-only subscriber (no `X-Qwen-Client-Id` registered yet) that connected BEFORE publish is invisible to the snapshot — `consensus` silently rejects its later vote as `forbidden`. Documented the window in `votersForSession` JSDoc; future PRs surfacing `eligibleVoters[]` on `permission_request.data` should source it from the same snapshot for consistency. No code change — the narrow window is acceptable for F3 v1, and the structural fix (snapshot at publish time) requires bridge-level refactor. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): wenshao Round 5 — sentinel injection guard, observability, /8 loopback (#4335) Four findings from wenshao review #4325130053. The Critical one is a real security gap; the others are observability + correctness hardening. **[Critical] Cancel sentinel injection bypass** (3271185588). The mediator's `vote()` recognizes `CANCEL_VOTE_SENTINEL` BEFORE validating the option against `allowedOptionIds`, so a wire client sending `{outcome:'selected', optionId:'__cancelled__'}` would short-circuit ALL policy dispatch (designated originator check, consensus quorum, local-only loopback gate). The mediator's JSDoc documented the precondition ("callers MUST NOT forward an incoming vote.optionId === CANCEL_VOTE_SENTINEL from a wire client") but the precondition was never enforced — the bridge's `respondToSessionPermission` mapped the wire optionId straight through. Added an explicit `InvalidPermissionOptionError` throw when the wire payload is `{selected, CANCEL_VOTE_SENTINEL}`. The collision-defense at request issue time (`CancelSentinelCollisionError`) already prevents agents from advertising the sentinel as a legitimate option; this closes the remaining vector. **[Suggestion] Silent quorum cap + M=0 hang observability** (3271185594). Two related diagnostic gaps in the consensus policy: - When `policy.consensusQuorum` exceeds `votersAtIssue.size`, the cap fires silently. Operators investigating "why did consensus resolve at N=2 when I configured 5?" had no breadcrumb. - When `policy === 'consensus'` and `votersAtIssue.size === 0`, every vote rejects as `forbidden: designated_mismatch` because the empty snapshot can never match any voter clientId. The request hangs until `permissionTimeoutMs` with no diagnostic signal. Added stderr breadcrumbs at both points: cap-applied (once per request via a `consensusQuorumCapNoted` flag on `MediatorPending`) and at issue time when consensus M=0. No semantic change — the cap and the timeout-only resolution behavior are intentional per the F3 plan; the breadcrumbs just make them debuggable. **[Suggestion] detectFromLoopback misses 127.0.0.0/8** (3271185597). Per RFC 1122 the entire `127.0.0.0/8` block is loopback. The exact-match Set of three literals (`127.0.0.1`, `::1`, `::ffff:127.0.0.1`) silently fail-CLOSED on legitimate `127.0.0.2` / `127.0.1.1` / `::ffff:127.0.0.2` peers, causing unexpected `remote_not_allowed` rejections under `local-only` policy. Switched to a prefix test so the entire `/8` and its dual-stack mirror are accepted. Direction stays fail-CLOSED for unrecognized address shapes. **[Suggestion] VSCode JSON schema integer/min validation** (3271185604). `runQwenServe.ts` validates `Number.isInteger(consensusQuorum) && >= 1`, but the generated `settings.schema.json` declared `"type": "number"` so VSCode's inline JSON Schema validation accepted `0` / `-1` / `1.5` and the user only learned the value was invalid on the next daemon restart. Added `jsonSchemaOverride: {type:'integer', minimum:1}` to the `consensusQuorum` settings entry and regenerated the schema. IDE editors now flag invalid values immediately. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 6 — wenshao APPROVED + DeepSeek follow-ups (#4335) Mixed batch: bridge-test backfill from wenshao's APPROVED review plus 4 DeepSeek/v4-pro suggestions and the 3 typecheck/test blockers DeepSeek named in CHANGES_REQUESTED #4325674833. **Pre-merge blockers (DeepSeek #4325674833 body)** - `server.test.ts:529` `FakeBridge` — added the F3-required `permissionPolicy: 'first-responder' as const`. Tests don't exercise mediation; the literal pins the pre-F3 default so existing assertions stay shape-compatible. - `server.test.ts:3994` `WorkspaceFileSystemFactory.forRequest()` mock — added the missing `writeTextOverwrite` method that PR #4334 introduced on `WorkspaceFileSystem` after this branch forked. - 4 vote-context test failures from `fromLoopback` plumbing — updated the four `expect(...).toEqual(...)` assertions in `POST /session/:id/permission/:requestId` and `POST /permission/:requestId` to include `fromLoopback: true` on the captured context. The supertest peer is `127.0.0.1`, so `detectFromLoopback(req)` correctly stamps the field; the pre-F3 expected shape was stale. **Inline suggestions adopted** - **3271420267** (wenshao APPROVED, security-critical) — added bridge-level test `rejects cancel sentinel injection via {selected,'__cancelled__'}` in `httpAcpBridge.test.ts`. Without it, a future refactor could silently remove the wire-injection guard that closes the policy-bypass attack surface introduced in Round 5 (#3271185588). Required `npm run build --workspace=packages/acp-bridge` to refresh `dist/` before vitest picked up the F3 bridge.ts changes; documented for future contributors editing F3 acp-bridge code. - **3271627444** (DeepSeek) — `request()` JSDoc rewritten to drop "Promise contract — never rejects" without qualification. The `CancelSentinelCollisionError` synchronous throw is real and intentional (a never-settling Promise alongside a thrown error is worse than fail-fast), but callers must be aware of it. Updated the contract doc to call out the sync-throw exception explicitly and documented that async callers get the throw via their own Promise machinery. - **3271627446** (DeepSeek) — fixed "Bounded LRU" comment on `MAX_RESOLVED_PERMISSION_RECORDS` to "Bounded FIFO" since `rememberResolved` uses `resolvedOrder.shift()` (drop oldest). Mirrors the parallel `PermissionAuditRing` correction in commit b0242dd. - **3271627457** (DeepSeek) — added stderr breadcrumbs to all 3 forbidden-vote sites (voteDesignated / voteConsensus / voteLocalOnly). Audit ring is in-memory only (no v1 query route), SSE events are transient — operators tailing daemon stderr previously had zero indication of permission rejections. New `writeForbiddenStderr` helper centralizes the formatting + try/catch defensive posture (mirrors the timeout breadcrumb pattern from Round 4). - **3271627459** (DeepSeek) — added a `TODO(forward-compat)` comment at `voteConsensus`'s rejection site documenting the `designated_mismatch` reason-code overload. The same wire string covers two distinct semantic cases: "voter is not the prompt originator" (designated policy) and "voter not in consensus votersAtIssue snapshot" (consensus). Splitting them into distinct codes is deferred to a future PR once an SDK consumer needs to disambiguate. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 7 — error precedence + 7 hardening fixes from wenshao (#4335) 8 findings from wenshao Round 7. The Critical one closes a session- existence information leak; 6 Suggestions improve observability, type safety, and test coverage; 1 documents the cancel-sentinel escape hatch in the local-only setting description. **[Critical] Error precedence regression in respondToSessionPermission** (3271978329). When `peekSessionFor(requestId)` returned `undefined` (timed out / LRU-evicted / never registered), the cross-session guard at line 2033 didn't fire (`!== undefined` skips it), so execution fell through to `resolveTrustedClientId` which throws `InvalidClientIdError` (HTTP 400) when the caller's clientId isn't registered. Pre-F3 returned `false` (HTTP 404) for unknown requestIds regardless of clientId validity. Without the explicit guard, a probe with a fabricated clientId could distinguish "session exists with these registered clients" (400) from "no such request" (404). Added an explicit `actualSessionId === undefined → return false` short-circuit BEFORE the clientId validation. The defensive `unknown_request` switch case below becomes unreachable in practice; left in place for defense-in-depth. **[Suggestion] Cancel sentinel cross-policy escape hatch under `local-only`** (3271978336). Documented in `voteLocalOnly` JSDoc and the settings description that a remote voter can ABORT a pending permission via `{outcome:'cancelled'}` even though they cannot RESOLVE one. The F3 plan calls this out as intentional (cross-policy cancel for consistency with first-responder / designated / consensus); operators wanting strict-cancel-too need a dedicated loopback-bound daemon. Doc-only — semantic change deferred. **[Suggestion] CapabilitiesEnvelope.policy.permission widens silently** (3271978342). Replaced the inlined string-literal union with `import type { PermissionPolicy } from '@qwen-code/acp-bridge'`. Adding a 5th policy upstream would now trigger a compile error here instead of silently accepting the narrower set. **[Suggestion] M=2 unanimity surprise** (3271978356). Default quorum `floor(M/2)+1` requires unanimity for even M (M=2 → quorum=2; both voters must agree). An operator picking `consensus` with two clients expecting "majority of 2 = 1" gets unanimity instead — a split vote silently hangs until `permissionTimeoutMs`. Added stderr breadcrumb at issue time when the default formula yields unanimity (M ≥ 2 and floor(M/2)+1 == M). Mirrors the existing M=0 / cap-applied breadcrumbs added in Round 5. Formula stays unchanged (true majority for all M is mutually exclusive with M=1 → quorum=1). Description in the settings schema also calls out the M=2 case explicitly. **[Suggestion] Cancel sentinel adversarial test gap** (3271978359). The existing "resolves cancelled regardless of policy" test used the originator under designated and a votersAtIssue voter under consensus — those would be ACCEPTED by the policies even without the sentinel bypass. Added two adversarial tests that pin the cross-policy escape hatch: non-originator voter under designated and not-in-snapshot voter under consensus. **[Suggestion] BridgeClient pre-publish collision test gap** (3271978365). `bridgeClient.requestPermission` throws `CancelSentinelCollisionError` BEFORE publishing the SSE `permission_request` to prevent orphan events (the mediator-level collision check in `mediator.request` happens too late if publish goes first). Added test asserting the throw + asserting publish was NOT called + asserting `pendingPermissionIds` was NOT incremented. **[Suggestion] Settings descriptions missing security caveats** (3271978370). Added explicit caveats to `permissionStrategy` description: (a) `designated` notes that client identity is self-declared with no proof-of-possession (impersonation by observing originatorClientId on SSE frames is possible); (b) `local-only` notes the cancel-sentinel cross-policy escape hatch. Schema regenerated to `vscode-ide-companion/schemas/settings.schema.json`. **[Suggestion] Boot validation error class** (3271978374). Replaced `err.message.includes('invalid policy.')` substring matching with a dedicated `InvalidPolicyConfigError` class checked via `instanceof`. A future reworded validation message would have silently downgraded operator misconfiguration to "fall back to defaults" under the previous fragile match. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 8 — close legacy clientId oracle + 5 hardening fixes (#4335) 6 follow-up findings from wenshao Round 8 review #4326742064 (state: COMMENTED — not blocking but addresses leftover risk surfaces). **[Suggestion] Legacy `respondToPermission` info leak** (3272493777). Round 7 closed the cross-session client-registration oracle on the session-scoped vote route, but the legacy workspace-level route (`POST /permission/<requestId>`) still called `resolveAnyTrustedClientId` on unknown-requestId paths, throwing `InvalidClientIdError` (400) for unregistered clientIds and returning false (404) for registered ones — the same oracle. The PR #4231 reasoning ("preserve security boundary") was inverted: the 400-vs-404 distinction WAS the leak. Removed the call, deleted the now-unused `resolveAnyTrustedClientId` helper, and updated the previously-leak-asserting test (`rejects unknown permission votes with unregistered client ids`) to assert the new uniform `false` behavior across all 3 input shapes (unregistered / registered / no-clientId). **[Suggestion] Error-precedence regression test gap + observability inconsistency** (3272493792). Two parts: - Added regression test `returns false (not InvalidClientIdError) when session exists but requestId is unknown and clientId is unregistered` to lock the Round-7 fix against future refactors. - Promoted the error-precedence guard's stderr line from debug-gated `writeServeDebugLine` to unconditional `writeStderrLine`, matching the `writeForbiddenStderr` posture in the mediator. Operators tailing stderr at 3 AM no longer need `QWEN_SERVE_DEBUG=1` to see unexpected 404s on the permission endpoint. **[Suggestion] Settings description "UNANIMITY for even M" was factually wrong** (3272493795). `floor(M/2)+1` equals M only when M=2; for M=4 it gives 3 (supermajority), M=6 gives 4 (~67%). The mediator's own unanimity warning correctly fires only when M=2. Settings description now reads "UNANIMITY for M=2 (quorum=2, both must agree) and supermajority for larger even M (M=4 → quorum=3; M=6 → quorum=4)". VSCode JSON schema regenerated. **[Suggestion] runQwenServe.ts inline policy unions** (3272493805). Same drift-protection rationale as the types.ts fix in Round 7. Imported `PermissionPolicy` from `@qwen-code/acp-bridge`, replaced 3 inline unions: the `let` declaration, the `as` cast, and the `VALID_PERMISSION_POLICIES` Set construction. Used a typed-array + Set<string> pattern (drift caught at array construction; runtime Set keeps `.has(string)` ergonomics). **[Suggestion] InvalidPolicyConfigError discrimination needs positive tests** (3272493818). Extracted the inline `policyConfig`-validation logic into an exported `validatePolicyConfig(policyConfig, onWarning?)` helper and exported `InvalidPolicyConfigError` itself. Added 7 unit tests covering: empty config, all 4 valid literals, invalid literal throws (with class identity check + message regex), 4 non-positive-integer quorum cases throw, valid combination returns, mismatch (consensusQuorum + non-consensus strategy) emits warning without throwing, no-warning happy path, and error messages name the failed field. The boot path in `runQwenServe` now delegates to the helper (one call site, DRY). **[Suggestion] Unanimity breadcrumb spammed per-request** (3272493829). The Round-7 unanimity stderr line fires inside the synchronous Promise executor of every `request()` call, which for a 2-client consensus session is EVERY permission request (M=2 unanimity is the normal operating mode, not a rare edge). Added `unanimityBreadcrumbEmitted` boolean to the mediator class (per-mediator dedup, parallel to `consensusQuorumCapNoted` on `MediatorPending`). One emit per daemon lifetime — visible at boot, silent thereafter. Comment also corrects the "for even M" generalization to "for M=2" specifically, matching the actual condition (`floor(M/2)+1 === M` only for M=1 and M=2). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 9 — terminal-event forbidden cleanup + 7 hardening fixes (#4335) 8 follow-up findings from wenshao Round 9 (4 separate review records: 4326832742 / 4326833568 / 4326844430 / 4326851074, the last one a non-blocking comment review). 1 Critical + 7 Suggestions. **[Critical] Terminal events leaked forbiddenVotes history** (3272576003). `session_died` / `session_closed` / `client_evicted` / `stream_error` reducer cases cleared `pendingPermissions` and `permissionVoteProgress` but not `forbiddenVotes` / `forbiddenVoteCount`. Adapters reading view state for a dead session would render stale rejection data. All 4 cases now zero out the rejection ring + counter. Parameterized regression test asserts the cleanup contract. **[Suggestion] safeAudit JSDoc was orphaned over writeForbiddenStderr** (3272567323). Two consecutive JSDoc blocks were stacked back-to-back but the method definitions followed in the opposite order, so IDE hover and API doc generation showed `safeAudit`'s docs as `writeForbiddenStderr`'s. Reordered method definitions so each JSDoc precedes its actual method. **[Suggestion] writeForbiddenStderr had no test coverage** (3272568031). Added a 3-path test (designated / consensus / local-only) that spies on `process.stderr.write` and asserts each breadcrumb contains the expected reason fragment plus the requestId + sessionId for grep-ability. Pins the format so a future refactor can't silently drop the line. **[Suggestion] resolveEntry numbered list contradicted code** (3272581553). The N2-invariant cleanup ladder docstring bundled "delete from pending + write to resolved" into step 2 ahead of the SSE emit, but the actual code defers `rememberResolved` until AFTER `safeEmit` (the I5 inline comment on line 1103 correctly explains this). Split step 2 into two halves around the emit so the spec faithfully describes the ordering invariant. **[Suggestion] Dead exports in bridgeClient.ts** (3272581548). `MAX_RESOLVED_PERMISSION_RECORDS`, `PendingPermission`, and `PermissionResolutionRecord` were defined and exported but no longer referenced — the mediator owns the same state under different names (`permissionMediator.ts:77` / `:319`). The JSDoc still pointed at deleted closures (`registerPending`, `resolvedPermissions` map). Removed all three definitions and the matching re-exports in `cli/src/serve/httpAcpBridge.ts`. **[Suggestion] detectFromLoopback prefix-match had no direct test** (3272581557). Supertest in the broader server.test.ts suite always connects from `127.0.0.1`, so the Round-5 prefix-match fix for `127.x`-beyond-`.0.0.1`, `::1`, `::ffff:127.*`, and the fail-closed branches had no coverage. Exported the helper from `server.ts` (loosened parameter type to a minimal shape so tests don't need to spin up Express) and added an `it.each` table covering the variants the fix targets, plus an explicit "does NOT consult X-Forwarded-For" assertion as a security pin. **[Suggestion] Validate-policies set is a 4th hardcoded copy** (3272581563). The policy literals already exist in 3 places — `PermissionPolicy` type, `SERVE_CAPABILITY_REGISTRY.permission_ mediation.modes`, and `settingsSchema.ts` enum options. `validatePolicyConfig` now derives its valid-set from `SERVE_CAPABILITY_REGISTRY.permission_mediation.modes` (single runtime source of truth). Adding a 5th policy upstream lands in one place; a future drift between the registry and the type union would still surface at the `as PermissionPolicy` cast. **[Suggestion] BridgeClient over-coupled to MultiClientPermissionMediator** (3272581569). `BridgeClient` only ever calls `mediator.request()` but its field was typed as the concrete class, forcing every test stub to fake all 6 mediator members. Narrowed the field type to `Pick<PermissionMediator, 'request'>` (the frozen interface from `permission.ts`); the bridge factory still passes the full `MultiClientPermissionMediator` instance via structural typing. Test stubs simplified from 6 placeholder members to 1. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 10 — wenshao APPROVED + 3 final polish (#4335) wenshao APPROVED the PR (review 4327485978: "No issues found in the latest Round 9 changes... LGTM ✅") with 3 minor follow-up suggestions in a separate COMMENTED review (4327443147). All adopted; the 4th suggestion (3273077262) was already addressed in Round 9. **[Suggestion] Symmetric stderr breadcrumb on legacy respondToPermission** (3273077256). The session-scoped sibling already writes an unconditional `writeStderrLine` on its `actualSessionId === undefined` rejection path (Round 8 / 3272493792); the legacy `POST /permission/<id>` route returned `false` silently after the Round-8 oracle removal, leaving an observability gap. Added matching `writeStderrLine`. Operators tailing stderr at 3 AM now see legacy-route 404s without needing QWEN_SERVE_DEBUG=1. **[Suggestion] consensusQuorum contract mismatch** (3273077270). The warning text told the operator "the override will be ignored" but the function still propagated `permissionConsensusQuorum` to BridgeOptions. The downstream mediator only reads it under the consensus policy, so behavior was correct — but the public contract contradicted itself. Adopt option (a): drop the value to `undefined` when the strategy is not 'consensus' so the returned struct matches what the warning promises. Updated the existing `validatePolicyConfig` test to assert the new contract. **[Suggestion] Stderr-breadcrumb assertion missing from error-precedence regression test** (3273077272). The Round-8 test pinned the return-value behavior (`false`) but not the unconditional-stderr promotion that was the primary behavioral change of that hunk. Added `vi.spyOn(process.stderr, 'write')` + assertions for both "rejected permission vote" and the literal requestId in the test. A future refactor that drops or downgrades the log line is now caught. **[Suggestion] _validPolicies underscore-prefix misleading** (3273077262 — already addressed). Round 9's commit 6793b89 replaced the literal `_validPolicies` array with a single Set derived from `SERVE_CAPABILITY_REGISTRY.permission_mediation.modes` (per separate suggestion 3272581563). The underscore-prefixed identifier is gone in current HEAD; replied via PR comment pointing wenshao at the existing fix. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
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
3 participants
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
qwen serveattach/create flows, returns the stamped id in session responses, accepts it back throughX-Qwen-Client-Id, and uses trusted ids to stamp relevant daemon events withoriginatorClientId.Validation
X-Qwen-Client-Id.npm run buildpassed with the existing vscode companion curly warning ineditorGroupUtils.ts.client_identitycapability plusoriginatorClientIdassertions in bridge tests.Scope / Risk
clientIdis optional for compatibility with older daemons.Testing Matrix
Testing matrix notes:
npm run lint --workspace packages/sdk-typescriptcurrently fails before reaching this PR's changed files due to an existing ESLint rule-loading error while lintingProcessTransport.test.ts; the changed SDK files were checked with rootnpx eslint.Linked Issues / Bugs