feat(serve): auth device-flow route (#4175 Wave 4 PR 21)#4255
Merged
Conversation
Contributor
📋 Review SummaryThis PR implements OAuth 2.0 Device Authorization Grant (RFC 8628) for the 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an OAuth 2.0 Device Authorization Grant (“device flow”) surface to the qwen serve daemon and TypeScript SDK so remote clients can trigger daemon-local login (tokens persisted on the daemon host), with workspace-scoped SSE events and supporting tests/docs.
Changes:
- CLI serve: add
/workspace/auth/*device-flow routes, capability tag, workspace event fan-out, and shutdown disposal wiring. - Core auth: export credential-caching helper and set credential file permissions intent.
- SDK: add daemon auth types/events/reducer +
DaemonAuthFlowconvenience wrapper and unit tests; update protocol/user docs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-typescript/test/unit/daemonEvents.test.ts | Adds tests for auth device-flow events + reducer behavior. |
| packages/sdk-typescript/src/daemon/types.ts | Introduces wire types for device-flow routes and status snapshot. |
| packages/sdk-typescript/src/daemon/index.ts | Re-exports new auth flow helper and auth-related types/reducers. |
| packages/sdk-typescript/src/daemon/events.ts | Extends known event schema with auth events and adds auth reducer/state. |
| packages/sdk-typescript/src/daemon/DaemonClient.ts | Adds client methods for device-flow routes + lazy client.auth helper. |
| packages/sdk-typescript/src/daemon/DaemonAuthFlow.ts | New high-level start(...).awaitCompletion() wrapper with polling fallback. |
| packages/core/src/qwen/qwenOAuth2.ts | Exports cacheQwenCredentials, adds file mode constant, clears token cache after write. |
| packages/cli/src/serve/server.ts | Wires device-flow registry/provider; adds /workspace/auth/* routes and response mappers. |
| packages/cli/src/serve/server.test.ts | Adds integration tests for device-flow routes and capability advertising. |
| packages/cli/src/serve/runQwenServe.ts | Disposes device-flow registry during shutdown before bridge teardown. |
| packages/cli/src/serve/httpAcpBridge.ts | Adds broadcastWorkspaceEvent to fan workspace events to all session buses. |
| packages/cli/src/serve/capabilities.ts | Advertises new auth_device_flow capability tag. |
| packages/cli/src/serve/auth/qwenDeviceFlowProvider.ts | Implements Qwen OAuth device-flow provider without browser spawning; persists creds. |
| packages/cli/src/serve/auth/deviceFlow.ts | Adds device-flow registry, secret redaction wrapper, polling/sweeper logic. |
| packages/cli/src/serve/auth/deviceFlow.test.ts | Adds registry/state-machine tests + static-source “no browser spawn” grep guard. |
| docs/users/qwen-serve.md | Documents remote-daemon login flow and SDK usage. |
| docs/developers/qwen-serve-protocol.md | Documents new routes/events and capability tag in the protocol reference. |
Comments suppressed due to low confidence (2)
packages/sdk-typescript/src/daemon/events.ts:708
auth_device_flow_failedalways projects tostatus: 'error', but the daemon can set the flow’s GET state tostatus: 'expired'on time-based expiry while still emitting anauth_device_flow_failedevent witherrorKind: 'expired_token'. If consumers rely on SSE, they’ll never seeexpired. Consider storingexpiresAtfromauth_device_flow_startedand mappingexpired_tokentostatus:'expired'whenDate.now() >= expiresAt(otherwise keepstatus:'error').
case 'auth_device_flow_failed': {
// The daemon's status machine reserves 'expired' for the time-based
// path (now >= expiresAt). Upstream RFC 8628 errors — including
// `expired_token` — go to 'error' with `errorKind` carrying the
// distinction. Earlier drafts collapsed `errorKind: 'expired_token'`
packages/sdk-typescript/src/daemon/DaemonAuthFlow.ts:168
- The 404-evicted fallback returns
providerId: 'qwen-oauth'unconditionally. This will be wrong for any future provider and can be inconsistent with the flow being awaited. Usestart.providerId(from the original start response) instead of hardcoding.
// The entry was evicted post-grace; treat as terminal and stop.
return {
deviceFlowId: start.deviceFlowId,
providerId: 'qwen-oauth',
status: 'expired',
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Solid PR overall — design is mature, tests are focused, and the BrandedSecret rewrite addresses the leak path well. A few items worth a second look before merge; see inline comments. The high-priority one is the now-duplicated clearCache() call site in qwenOAuth2.ts.
wenshao
requested changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Qwen Code Review — feat(serve): auth device-flow route
Files examined (17)
Changed files
- docs/developers/qwen-serve-protocol.md
- docs/users/qwen-serve.md
- packages/cli/src/serve/auth/deviceFlow.test.ts
- packages/cli/src/serve/auth/deviceFlow.ts
- packages/cli/src/serve/auth/qwenDeviceFlowProvider.ts
- packages/cli/src/serve/capabilities.ts
- packages/cli/src/serve/httpAcpBridge.ts
- packages/cli/src/serve/runQwenServe.ts
- packages/cli/src/serve/server.test.ts
- packages/cli/src/serve/server.ts
- packages/core/src/qwen/qwenOAuth2.ts
- packages/sdk-typescript/src/daemon/DaemonAuthFlow.ts
- packages/sdk-typescript/src/daemon/DaemonClient.ts
- packages/sdk-typescript/src/daemon/events.ts
- packages/sdk-typescript/src/daemon/index.ts
- packages/sdk-typescript/src/daemon/types.ts
- packages/sdk-typescript/test/unit/daemonEvents.test.ts
Deterministic analysis
- tsc: 1 pre-existing error in daemonEvents.test.ts:670 (not introduced by this PR)
- eslint: clean (0 errors)
- Tests: 204/204 pass (deviceFlow: 21, server: 155, daemonEvents: 28)
Aggregated findings
Critical (3)
| # | File:Line | Finding |
|---|---|---|
| C1 | DaemonAuthFlow.ts:167 | providerId hardcoded to 'qwen-oauth' in 404-eviction fallback; start.providerId is available but not used |
| C2 | events.ts:965 | isAuthDeviceFlowErrorKind silently drops failed events with unknown error kinds — no compile-time link between daemon and SDK error-kind unions |
| C3 | deviceFlow.ts:637 | persist() called with no AbortSignal/timeout — can hang indefinitely, blocking clean daemon shutdown |
Suggestion (6)
| # | File:Line | Finding |
|---|---|---|
| S1 | deviceFlow.ts:651 | persist_failed hint leaks daemon filesystem paths via SSE to all connected clients |
| S2 | qwenDeviceFlowProvider.ts:134 | Raw IdP response body (HTML error pages, internal IPs) forwarded verbatim to SDK clients |
| S3 | deviceFlow.ts:324 | DeviceFlowNotFoundError exported but never imported/used — dead code |
| S4 | types.ts:321 | Duplicate status/error-kind type definitions within the same package |
| S5 | httpAcpBridge.ts:2628 | broadcastWorkspaceEvent catch uses debug-gated logging — silent failure in production |
| S6 | deviceFlow.ts:454 | doStart() doesn't check this.disposed after await provider.start() yield point |
Verdict: Request Changes — 3 Critical findings require attention before merge.
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
reviewed
May 17, 2026
wenshao
requested changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] requestDeviceAuthorization() logs the full successful device-authorization response before this PR wraps the device_code in BrandedSecret. The new daemon provider calls this helper, so enabling debug logging for qwen serve can write the raw upstream device_code (and user code / verification URLs) into daemon logs, violating the secret-hygiene contract for remote device flow.
Please remove this log or redact the sensitive fields before logging, e.g. log only non-secret metadata such as expires_in / interval.
— gpt-5.5 via Qwen Code /review
doudouOUC
added a commit
that referenced
this pull request
May 17, 2026
Eleven items from copilot-pull-request-reviewer's round-1 pass on #4255 — 4 inline threads + 7 from the PR-level review summary. ## Adopted (11 items, code/doc changes) - **`lastSeenAt` → `lastSeenEventId`** (`events.ts`, `DaemonDeviceFlowReducerState`). The field was set from `rawEvent.id` (SSE event id) but documented as "epoch ms" — a real semantic mismatch that would mislead consumers into time-based logic against a monotonic counter. Rename + tighten the JSDoc to describe it as an event-id counter; reducer cases updated. - **`DEVICE_FLOW_EXPIRY_GRACE_MS = 30_000` extracted** in `DaemonAuthFlow.ts` (was a magic number on `start.expiresAt + 30_000`). `AwaitCompletionOptions.timeoutMs` doc now describes the actual grace-past-expiry behavior + the rationale (clock skew + daemon sweeper interval + network latency) instead of the wrong "defaults to expiresAt - Date.now()" claim. - **Explicit `chmod 0o600`** in `cacheQwenCredentials` after every write. `fs.writeFile`'s `mode` only applies on file creation; a pre-existing `oauth_creds.json` written under a broader umask kept its old permissions across upgrades. The chmod now tightens it on every write; chmod failure (Windows / hardened FS) surfaces via `debugLogger.warn` instead of silently dropping the invariant. - **`SharedTokenManager.clearCache()` failure now logs** `debugLogger.warn` (was a silent `try { } catch { }`). In production a swallowed clearCache means in-process callers serve stale credentials until the SharedTokenManager mtime watcher catches up — a recoverable degradation worth a log line. - **Protocol doc** lists `persist_failed` in the `auth_device_flow_failed.errorKind` union (was added to the type but missed in the doc). - **`pollDeviceToken({signal})`** plumbed through `IQwenOAuth2Client` interface + `QwenOAuth2Client` impl + the Qwen device-flow provider. Cancel / dispose during a slow IdP response now aborts the in-flight HTTP socket immediately instead of waiting for the upstream timeout. Two new registry tests assert `cancel()` / `dispose()` propagate abort to the signal observed by `provider.poll`. - **`revealSecret` error message** clarified: was "secret has been GC-evicted" (impossible — WeakMap doesn't evict reachable keys). Now points at the actual reachable failure modes (forged shape / serialize+reparse losing the WeakMap binding). - **`transitionTerminal` JSDoc** clarifies that the PRIMARY guard against late timer secret leaks is the `entry.status !== 'pending'` check at the top of `runPollTick`; secret-clearing here is defense-in-depth. - **`DeviceFlowErrorKind` JSDoc'd per variant** so consumers can tell when each fires (RFC 8628 distinctions + `persist_failed` vs `upstream_error` boundary). - **Stale "PR 16 / PR 21 §3" temporal references** in `DaemonAuthFlow.ts:124` rephrased to be timeless ("workspace-scoped events fan out through whatever session buses happen to be live" — no PR number references that rot when those PRs merge). ## Not adopted (4 items, replied to in-thread) - **`authWithQwenDeviceFlow` browser-launch separation** — correct architectural advice but out of #4255 scope (would refactor a CLI auth UX module that PR 21 only touched additively). Tracked as a Wave 5 follow-up. - **Copyright header year range** — repo-wide convention "2025"; not introduced by this PR. - **Spread `...(x ? {x} : {})` → `x: x ?? undefined`** — the two are not semantically equivalent. The current form omits the key entirely on falsy `x`; the suggested form always includes the key. Tests assert object shape and would break under the change. - **Eager `client.auth` getter** — public API boundary. Lazy construction matches `DaemonSessionClient` precedent + saves the module load for SDK consumers that never touch auth. Refs: #4175 #4255 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
requested changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] tsc reports a type error in packages/sdk-typescript/test/unit/daemonEvents.test.ts:670 — Argument of type '{ v: number; type: string; data: { queueSize: number; ... } }' is not assignable to parameter of type 'DaemonEvent' (TS2345). This causes npm run typecheck to fail. The test object at that line (pre-existing code for slow_client_warning schema validation) no longer matches the updated DaemonEvent type introduced by this PR.
[Critical] New SDK files have zero test coverage:
DaemonAuthFlow.ts(213 lines of async poll loop, 404 fallback, abort handling, timeout ceiling) — no test file exists.DaemonClient.tsnew methods (startDeviceFlow,getDeviceFlow,cancelDeviceFlow,getAuthStatus, lazyauthgetter) — existingDaemonClient.test.tswas not updated.- SDK public surface test (
daemon-public-surface.test.ts) does not verify any of the new auth exports (DaemonAuthFlow,DaemonAuthFlowHandle,reduceDaemonAuthEvent, etc.).
These are the core SDK touch-points for device-flow consumers. Recommend adding at least happy-path + error-path tests before merge.
— glm-5.1 via Qwen Code /review
doudouOUC
added a commit
that referenced
this pull request
May 18, 2026
15 items from @wenshao's review batches on #4255. Catches a handful of real bugs that the earlier round (commit 3d9f082) didn't surface. ## Critical fixes - **C1 — `pollUntilTerminal` providerId pass-through** (`DaemonAuthFlow.ts:185`). The synthetic 404 fallback hardcoded `providerId: 'qwen-oauth'`; the parent `awaitCompletion` already receives the real providerId via `start.providerId` but `pollUntilTerminal`'s parameter type stripped it. Add the field to the param type, propagate. - **C2 — open `errorKind` allowlist** (`events.ts`). The closed 5-value union in the type guard silently dropped any `failed` event whose errorKind the daemon added without mirroring SDK-side (e.g. a future `rate_limited`). The flow's reducer state would never transition to terminal, leaving SDK consumers stuck on `pending` forever. Open the union with `(string & {})` and accept any non-empty string in the runtime guard. Updated test asserts forward-compat behavior + still rejects the truly-malformed empty-string case. - **C3 — `persist()` timeout + signal** (`deviceFlow.ts`). A wedged disk I/O (NFS stall, encrypted-volume contention) without bounds would pin the entry in `pending` until the upstream `expires_in` elapsed (potentially minutes). The registry now passes its `cancelController.signal` AND arms a hard `DEVICE_FLOW_PERSIST_TIMEOUT_MS = 30_000` timer; persist failure surfaces as `persist_failed` immediately. The `DeviceFlowPollResult` `success` variant signature changed to `persist({signal})`. - **C4 — cancel × success race rollback** (`deviceFlow.ts` + Qwen provider). Today, if `cancel()` transitions while `persist()` is in flight, the credentials get written but the flow's status is `cancelled`. User sees cancelled, daemon disk has a valid token. `DeviceFlowPollResult.success` gains an optional `unpersist()` callback the registry calls when `transitionTerminal(authorized)` fails — the Qwen provider wires it to `clearQwenCredentials()`. Rollback failure is audited but not propagated (re-running auth would overwrite anyway). - **C5 — don't `unref()` the `awaitCompletion` sleep timer** (`DaemonAuthFlow.ts`). On a standalone Node CLI/script doing just `client.auth.start().awaitCompletion()`, the unref'd between-poll timer was the only event-loop handle, so Node could exit before the user finished authorization. The poll wait is foreground work the caller explicitly awaits — keep it ref'd. ## Information-leak fixes - **S1 — sanitize `persist_failed` hint**. `err.message` from `cacheQwenCredentials` embeds the full `~/.qwen/oauth_creds.json` path. Broadcast via SSE, that path leaks the daemon's home layout to every connected session subscriber. Replace user-facing hint with `"credentials could not be written to the daemon filesystem — check disk space and permissions"`; full err goes to stderr audit only. - **S2 — sanitize upstream `pollDeviceToken` hint**. The class embedded the entire raw IdP response body (which can be an HTML error page from a reverse proxy) into the thrown message. Same broadcast leak path. Replace upstream-error hint with `"unexpected response from identity provider"`; RFC 8628 errors use `"Qwen IdP returned ${kind}"`. ## Cleanup / forward-compat - **D1 — drop duplicate `clearCache()`** at `qwenOAuth2.ts:840`. The paired call became redundant once `cacheQwenCredentials` folded the clearCache in (PR #4255 fold-in 1). The fold-in 1 message said this would be done; the duplicate slipped through. - **S3 — drop unused `DeviceFlowNotFoundError`** (`deviceFlow.ts`). Exported but never imported; route handlers do inline 404 JSON. - **S4 — single-source SDK status / errorKind unions** (`types.ts`). `DaemonAuthDeviceFlowSdkStatus` / `DaemonAuthDeviceFlowSdkErrorKind` were parallel literal copies of the canonical events.ts definitions — drift waiting to happen. Now imported + aliased as type-only re-exports. - **S5 — broadcast 100% fail elevates to stderr** (`httpAcpBridge.ts`). Per-session bus failures stay debug-only, but a broadcast where EVERY session bus refused is operationally interesting (clients won't see the event). Track success / fail counts; `writeStderrLine` when `successCount === 0`. - **S6 — `this.disposed` check after `await provider.start()`** (`deviceFlow.ts`). `dispose()` mid-start would orphan the freshly- inserted entry (`schedulePoll` guards on `disposed` so no poll fires; the entry never transitions). Throw post-await if disposed. - **W1 — thread `signal` into `requestDeviceAuthorization`** (`qwenOAuth2.ts` + Qwen provider). `start()` had the same cancellation gap that `pollDeviceToken` had — a slow device-authorization request couldn't be aborted during shutdown. Now plumbed end-to-end. - **W2 — split `invalid_request` from `unsupported_provider`** (`server.ts`). Conflating them surfaced misleading remediation hints to SDK consumers branching on `code` ("this provider isn't supported here" when the real cause was a serializer dropping the field). Bad-shape now returns `code: 'invalid_request'`; unknown-but-well-formed stays `unsupported_provider`. - **W3 — drop never-populated `accountAlias`** (Qwen provider). The field was wired through types / events / reducer / audit but the Qwen IdP's token response doesn't carry one (no `name` / `email` / `sub`). Returning only `{expiresAt}` makes the field type-honestly absent rather than always-undefined. Future provider with an alias-bearing response can populate it. - **W4 — `DaemonAuthFlow` JSDoc accuracy**. Doc claimed "first attempts to consume an SSE event stream … falls back to GET-based polling"; actual is GET-only with SSE as a real-time hint for clients already subscribed to a session stream. - **W5 — clearer unit arithmetic** in interval normalization. The `(_INTERVAL_MS / 1000) * 1000` cancelation hid the s↔ms boundary; expanded form makes both branches unit-explicit. ## Test changes - `daemonEvents.test.ts` updated to match the now-OPEN errorKind union (forward-compat assertion + empty-string still rejected). - `deviceFlow.test.ts` `FakeProvider.poll` aligned with the new `persist({signal})` signature + optional `unpersist`. ## Validation - `npm run typecheck --workspace packages/cli --workspace packages/sdk-typescript --workspace packages/core` — clean - `npx vitest run packages/cli/src/serve/ packages/sdk-typescript/test/unit/daemonEvents.test.ts` — 368/368 - `npx eslint --max-warnings 0` over the 11 PR 21 surface files — clean Refs: #4175 #4255 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
Round-1 review fold-in summaryAll 25 inline review threads have individual replies pointing at the addressing commit. Top-level summary so reviewers can spot-check at a glance:
|
doudouOUC
added a commit
that referenced
this pull request
May 18, 2026
Eleven items from copilot-pull-request-reviewer's round-1 pass on #4255 — 4 inline threads + 7 from the PR-level review summary. ## Adopted (11 items, code/doc changes) - **`lastSeenAt` → `lastSeenEventId`** (`events.ts`, `DaemonDeviceFlowReducerState`). The field was set from `rawEvent.id` (SSE event id) but documented as "epoch ms" — a real semantic mismatch that would mislead consumers into time-based logic against a monotonic counter. Rename + tighten the JSDoc to describe it as an event-id counter; reducer cases updated. - **`DEVICE_FLOW_EXPIRY_GRACE_MS = 30_000` extracted** in `DaemonAuthFlow.ts` (was a magic number on `start.expiresAt + 30_000`). `AwaitCompletionOptions.timeoutMs` doc now describes the actual grace-past-expiry behavior + the rationale (clock skew + daemon sweeper interval + network latency) instead of the wrong "defaults to expiresAt - Date.now()" claim. - **Explicit `chmod 0o600`** in `cacheQwenCredentials` after every write. `fs.writeFile`'s `mode` only applies on file creation; a pre-existing `oauth_creds.json` written under a broader umask kept its old permissions across upgrades. The chmod now tightens it on every write; chmod failure (Windows / hardened FS) surfaces via `debugLogger.warn` instead of silently dropping the invariant. - **`SharedTokenManager.clearCache()` failure now logs** `debugLogger.warn` (was a silent `try { } catch { }`). In production a swallowed clearCache means in-process callers serve stale credentials until the SharedTokenManager mtime watcher catches up — a recoverable degradation worth a log line. - **Protocol doc** lists `persist_failed` in the `auth_device_flow_failed.errorKind` union (was added to the type but missed in the doc). - **`pollDeviceToken({signal})`** plumbed through `IQwenOAuth2Client` interface + `QwenOAuth2Client` impl + the Qwen device-flow provider. Cancel / dispose during a slow IdP response now aborts the in-flight HTTP socket immediately instead of waiting for the upstream timeout. Two new registry tests assert `cancel()` / `dispose()` propagate abort to the signal observed by `provider.poll`. - **`revealSecret` error message** clarified: was "secret has been GC-evicted" (impossible — WeakMap doesn't evict reachable keys). Now points at the actual reachable failure modes (forged shape / serialize+reparse losing the WeakMap binding). - **`transitionTerminal` JSDoc** clarifies that the PRIMARY guard against late timer secret leaks is the `entry.status !== 'pending'` check at the top of `runPollTick`; secret-clearing here is defense-in-depth. - **`DeviceFlowErrorKind` JSDoc'd per variant** so consumers can tell when each fires (RFC 8628 distinctions + `persist_failed` vs `upstream_error` boundary). - **Stale "PR 16 / PR 21 §3" temporal references** in `DaemonAuthFlow.ts:124` rephrased to be timeless ("workspace-scoped events fan out through whatever session buses happen to be live" — no PR number references that rot when those PRs merge). ## Not adopted (4 items, replied to in-thread) - **`authWithQwenDeviceFlow` browser-launch separation** — correct architectural advice but out of #4255 scope (would refactor a CLI auth UX module that PR 21 only touched additively). Tracked as a Wave 5 follow-up. - **Copyright header year range** — repo-wide convention "2025"; not introduced by this PR. - **Spread `...(x ? {x} : {})` → `x: x ?? undefined`** — the two are not semantically equivalent. The current form omits the key entirely on falsy `x`; the suggested form always includes the key. Tests assert object shape and would break under the change. - **Eager `client.auth` getter** — public API boundary. Lazy construction matches `DaemonSessionClient` precedent + saves the module load for SDK consumers that never touch auth. Refs: #4175 #4255 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
doudouOUC
added a commit
that referenced
this pull request
May 18, 2026
15 items from @wenshao's review batches on #4255. Catches a handful of real bugs that the earlier round (commit 3d9f082) didn't surface. ## Critical fixes - **C1 — `pollUntilTerminal` providerId pass-through** (`DaemonAuthFlow.ts:185`). The synthetic 404 fallback hardcoded `providerId: 'qwen-oauth'`; the parent `awaitCompletion` already receives the real providerId via `start.providerId` but `pollUntilTerminal`'s parameter type stripped it. Add the field to the param type, propagate. - **C2 — open `errorKind` allowlist** (`events.ts`). The closed 5-value union in the type guard silently dropped any `failed` event whose errorKind the daemon added without mirroring SDK-side (e.g. a future `rate_limited`). The flow's reducer state would never transition to terminal, leaving SDK consumers stuck on `pending` forever. Open the union with `(string & {})` and accept any non-empty string in the runtime guard. Updated test asserts forward-compat behavior + still rejects the truly-malformed empty-string case. - **C3 — `persist()` timeout + signal** (`deviceFlow.ts`). A wedged disk I/O (NFS stall, encrypted-volume contention) without bounds would pin the entry in `pending` until the upstream `expires_in` elapsed (potentially minutes). The registry now passes its `cancelController.signal` AND arms a hard `DEVICE_FLOW_PERSIST_TIMEOUT_MS = 30_000` timer; persist failure surfaces as `persist_failed` immediately. The `DeviceFlowPollResult` `success` variant signature changed to `persist({signal})`. - **C4 — cancel × success race rollback** (`deviceFlow.ts` + Qwen provider). Today, if `cancel()` transitions while `persist()` is in flight, the credentials get written but the flow's status is `cancelled`. User sees cancelled, daemon disk has a valid token. `DeviceFlowPollResult.success` gains an optional `unpersist()` callback the registry calls when `transitionTerminal(authorized)` fails — the Qwen provider wires it to `clearQwenCredentials()`. Rollback failure is audited but not propagated (re-running auth would overwrite anyway). - **C5 — don't `unref()` the `awaitCompletion` sleep timer** (`DaemonAuthFlow.ts`). On a standalone Node CLI/script doing just `client.auth.start().awaitCompletion()`, the unref'd between-poll timer was the only event-loop handle, so Node could exit before the user finished authorization. The poll wait is foreground work the caller explicitly awaits — keep it ref'd. ## Information-leak fixes - **S1 — sanitize `persist_failed` hint**. `err.message` from `cacheQwenCredentials` embeds the full `~/.qwen/oauth_creds.json` path. Broadcast via SSE, that path leaks the daemon's home layout to every connected session subscriber. Replace user-facing hint with `"credentials could not be written to the daemon filesystem — check disk space and permissions"`; full err goes to stderr audit only. - **S2 — sanitize upstream `pollDeviceToken` hint**. The class embedded the entire raw IdP response body (which can be an HTML error page from a reverse proxy) into the thrown message. Same broadcast leak path. Replace upstream-error hint with `"unexpected response from identity provider"`; RFC 8628 errors use `"Qwen IdP returned ${kind}"`. ## Cleanup / forward-compat - **D1 — drop duplicate `clearCache()`** at `qwenOAuth2.ts:840`. The paired call became redundant once `cacheQwenCredentials` folded the clearCache in (PR #4255 fold-in 1). The fold-in 1 message said this would be done; the duplicate slipped through. - **S3 — drop unused `DeviceFlowNotFoundError`** (`deviceFlow.ts`). Exported but never imported; route handlers do inline 404 JSON. - **S4 — single-source SDK status / errorKind unions** (`types.ts`). `DaemonAuthDeviceFlowSdkStatus` / `DaemonAuthDeviceFlowSdkErrorKind` were parallel literal copies of the canonical events.ts definitions — drift waiting to happen. Now imported + aliased as type-only re-exports. - **S5 — broadcast 100% fail elevates to stderr** (`httpAcpBridge.ts`). Per-session bus failures stay debug-only, but a broadcast where EVERY session bus refused is operationally interesting (clients won't see the event). Track success / fail counts; `writeStderrLine` when `successCount === 0`. - **S6 — `this.disposed` check after `await provider.start()`** (`deviceFlow.ts`). `dispose()` mid-start would orphan the freshly- inserted entry (`schedulePoll` guards on `disposed` so no poll fires; the entry never transitions). Throw post-await if disposed. - **W1 — thread `signal` into `requestDeviceAuthorization`** (`qwenOAuth2.ts` + Qwen provider). `start()` had the same cancellation gap that `pollDeviceToken` had — a slow device-authorization request couldn't be aborted during shutdown. Now plumbed end-to-end. - **W2 — split `invalid_request` from `unsupported_provider`** (`server.ts`). Conflating them surfaced misleading remediation hints to SDK consumers branching on `code` ("this provider isn't supported here" when the real cause was a serializer dropping the field). Bad-shape now returns `code: 'invalid_request'`; unknown-but-well-formed stays `unsupported_provider`. - **W3 — drop never-populated `accountAlias`** (Qwen provider). The field was wired through types / events / reducer / audit but the Qwen IdP's token response doesn't carry one (no `name` / `email` / `sub`). Returning only `{expiresAt}` makes the field type-honestly absent rather than always-undefined. Future provider with an alias-bearing response can populate it. - **W4 — `DaemonAuthFlow` JSDoc accuracy**. Doc claimed "first attempts to consume an SSE event stream … falls back to GET-based polling"; actual is GET-only with SSE as a real-time hint for clients already subscribed to a session stream. - **W5 — clearer unit arithmetic** in interval normalization. The `(_INTERVAL_MS / 1000) * 1000` cancelation hid the s↔ms boundary; expanded form makes both branches unit-explicit. ## Test changes - `daemonEvents.test.ts` updated to match the now-OPEN errorKind union (forward-compat assertion + empty-string still rejected). - `deviceFlow.test.ts` `FakeProvider.poll` aligned with the new `persist({signal})` signature + optional `unpersist`. ## Validation - `npm run typecheck --workspace packages/cli --workspace packages/sdk-typescript --workspace packages/core` — clean - `npx vitest run packages/cli/src/serve/ packages/sdk-typescript/test/unit/daemonEvents.test.ts` — 368/368 - `npx eslint --max-warnings 0` over the 11 PR 21 surface files — clean Refs: #4175 #4255 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
13af380 to
018981e
Compare
wenshao
requested changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Missing Test Coverage (Critical)
Three key modules with 568+ lines of new code have zero unit tests:
DaemonAuthFlow(213 lines):awaitCompletion,pollUntilTerminal,cancel, timeout, and 404 handling are untested. This is the primary SDK developer-facing API for device-flow auth.DaemonClientauth methods:startDeviceFlow,getDeviceFlow,cancelDeviceFlow,getAuthStatusplus the lazyauthgetter have no tests inDaemonClient.test.ts. Route path construction, error code mapping (201/200/404/409/502), and DELETE idempotency are unverified.QwenDeviceFlowProvider(213 lines): The only production IdP adapter.start(),poll(), PKCE handling,mapRfc8628ErrorMessageregex, and persist ordering are all untested.
SDK auth methods lack AbortSignal (Suggestion)
DaemonClient.startDeviceFlow(), getDeviceFlow(), cancelDeviceFlow(), and getAuthStatus() do not accept an AbortSignal parameter despite fetchWithTimeout supporting signal composition via init.signal. This means pollUntilTerminal — which checks opts.signal between polls — cannot cancel an in-flight HTTP request, blocking up to fetchTimeoutMs (default 30s).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Two small non-blocking items from the round-9 pass; defensive shape + docs only. The 4 deferred test-coverage threads (#1-4 of round-8) are still tracked for fold-in 10. #6 — `lastSeenEventId` typed `number` with `?? 0` defaults in the `auth_device_flow_started` reducer case. The daemon-side `EventBus` assigns ids ≥ 1 so the `0` sentinel has no real-traffic meaning, but the monotonic gate (`rawEventId <= flow.lastSeenEventId`) would reject any future SDK-internal synthetic frame using `id: 0`. Changed the field type to `number | undefined` and dropped the `?? 0` from the started case. The `updateMatchingFlow` / `auth_device_flow_authorized` guards already short-circuit on `existing.lastSeenEventId !== undefined`, so undefined is safe end-to-end. Existing 34 reducer tests still pass unchanged. #7 — Added `@remarks` block to `DeviceFlowErrorKind.persist_failed`'s JSDoc explaining the lost-success retry UX. When fold-in 9 #7's `lost_success_after_timeout` audit fires (non-conforming provider violates signal contract; disk write succeeds after registry published `persist_failed`), a naive SDK retry hits the IdP a second time with a fresh `device_code` and prompts the user twice — but the first credential set is already valid. JSDoc now documents the mitigation: SDK consumers writing retry logic on `persist_failed` should call `client.auth.getStatus()` BEFORE re-prompting; operators can grep stderr/audit for `lost_success_after_timeout` to detect occurrences. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Lands the four deferred test-coverage items the round-8 review flagged (and round-9 re-surfaced) as a hard merge prerequisite. Net +41 tests across registry / SDK helper / client HTTP / HTTP route layers. #1 — `deviceFlow.test.ts` `persist failure paths` describe (3 tests, +3). The success arm's three terminal mappings — pure `persist_failed`, `cancelled` (cancel during persist), and `persist_failed` past `expiresAt` (the fold-in 9 #13 reclassification with `persist_also_failed_past_expiry` audit hint) — were 0-covered. Now pinned. Test #2 also asserts the fold-in 9 #5 cancellerClientId routing on the deferred `cancelled` event. #2 — new `DaemonAuthFlow.test.ts` (+14 tests). Mock DaemonClient with sequenced `getDeviceFlow` replies. Covers happy-path polling → `authorized`; `slow_down`-driven `intervalMs` bump firing `onThrottled`; `signal.abort()` rejection; `signal` propagation through `client.getDeviceFlow` (fold-in 7 #6); `timeoutMs` ceiling final-read; `timeoutMs:0` immediate-return (round-9 #6); NaN/Infinity → `sanitizePositiveMs` fallback to default ceiling (fold-in 7 #5); 404 → synthetic `error`/`not_found_or_evicted` (fold-in 3 #4) at BOTH the loop body AND the timeoutMs ceiling read (fold-in 7 #4); non-404 DaemonHttpError rethrown; `cancel()` and top-level `status()`/`cancel()` wrappers forward correctly. #3 — `DaemonClient.test.ts` `device-flow methods` describe (+11 tests). POSTs `/workspace/auth/device-flow` happy path + clientId header + body shape; 200/201 acceptance; non-2xx → `DaemonHttpError`. GETs URL-encode the deviceFlowId; forward `opts.signal` to `fetchWithTimeout`'s composed signal (fold-in 7 #6 — verified by aborting caller signal and observing the fetch's signal flip to `aborted`); 404 throws. DELETEs swallow 204 + 404 (idempotent, mirrors `closeSession`); non- 204/404 throws. `getAuthStatus` plain GET. `client.auth` lazy-instantiated singleton. #4 — `server.test.ts` 5 supplementary contract tests (+5). The existing 8 `it()`s cover happy paths + take-over + 401 POST + DELETE pending/terminal/unknown + 502 upstream + sweeper. This commit plugs gaps: 400 `invalid_request` for missing / non-string providerId (fold-in W2 split this from `unsupported_provider`); 409 `too_many_active_flows` (via injected fake registry); 401 `token_required` on DELETE without bearer; the asymmetric GET posture (`/workspace/auth/device-flow/:id` IS strict-gated to prevent peer-process userCode shoulder-surf; `/workspace/auth/status` stays read-only because its `pendingDeviceFlows` entries intentionally redact `userCode`). Validation: cli serve 631/631 (+8 from #1, #4); sdk 384/384 (+25 from #2, #3, +/- some pre-existing churn). Typecheck + lint clean. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
8045153 to
6bd252e
Compare
Merged
11 tasks
…round-11 #2) gpt-5.5 /review flagged a real correctness/security gap: the post-write `chmod` ordering left a window where freshly-written credentials could land in a broadly-readable existing `oauth_creds.json` before the chmod tightened it. On POSIX, a chmod failure additionally degraded to a debug warning while the broadly-readable tokens stayed on disk. New shape mirrors the standard atomic-write idiom: 1. Write `${filePath}.tmp.${pid}.${randomUUID()}` with `mode: 0o600`. The temp path doesn't exist beforehand, so the `mode` flag actually applies on creation (it doesn't on existing files, which was the root of the original race). 2. Defensive `chmod` on the temp file. POSIX failure is now a HARD ERROR (refuses to publish broad-perm credentials to the canonical filename). Windows logs a debug breadcrumb and proceeds, since chmod is a no-op on most NTFS volumes (perms go through ACLs). 3. Atomic `fs.rename` over `filePath`. The canonical path is ALWAYS at `0o600` from the moment it contains the new tokens; readers see either the old creds or the new creds, never a partially-written or broadly-readable state. 4. Best-effort `fs.unlink` of the temp file on any failure path so failed writes don't leave `.tmp.<pid>.<uuid>` litter on disk. Test mock in `qwenOAuth2.test.ts` extended with `chmod` + `rename` no-op stubs so the existing 158 core/qwen tests still pass; no test behavior change beyond the mock surface. Validation: typecheck clean (cli + core + sdk-typescript); core qwen 158/158; cli serve 643/643; sdk 384/384. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
reviewed
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the device-flow registry, Qwen provider, HTTP routes, qwenOAuth2.ts side fixes, SDK DaemonAuthFlow / DaemonClient, and the new test bundle. Overall the PR is in excellent shape — the BrandedSecret implementation correctly defeats the four coercion paths the previous new String() shape leaked through, persist signal is threaded end-to-end (registry → provider → fs.writeFile), authoritative timeouts via Promise.race make the registry robust against non-cooperative providers, and the latest atomic temp+chmod+rename for cacheQwenCredentials closes the post-hoc-chmod race nicely.
Five observations below. Of these, #1 (intervalMs unbounded growth) and #2 (tiny expiresIn floor) are real correctness issues; #3 is defense-in-depth; #4 is a test-coverage gap; #5 is a minor info-leak. None are blockers — the practical impact is small and the design context for each is already well-considered elsewhere in the file.
Strong points worth calling out
- 6 leak-path tests on
BrandedSecret(String()/+/ template /JSON.stringify/ numeric coercion /unsafeRevealSecret) — empirically pins the property the previous shape violated. - Concurrent-start coalescing via
inFlightStartsprevents the orphan-poll-timer leak. transitionTerminalreturningboolean+ idempotence guard correctly serializes sweeper × poll-tick × cancel races.cancel-during-persistdeferred terminal mapping eliminates the imperative-handler UX trap (cancelled→authorized cascade).lost_success_after_timeoutaudit breadcrumb captures forward-defense scenarios for non-conforming future providers.- Static-source grep test for browser-spawn primitives makes the daemon-runtime-locality contract regression-proof.
- 502
upstream_errormapping forprovider.startfailures distinguishes upstream IdP issues from daemon bugs in SDK retry routing. - Atomic write pattern in
cacheQwenCredentials(round-11): temp-with-mode + chmod-verify + rename matches what mature credential-storage libs do.
wenshao
reviewed
May 18, 2026
wenshao
requested changes
May 18, 2026
…back Eight findings from a wenshao + gpt-5.5 /review pass: 1 critical correctness, 2 real defensive defects, 4 edge cases / minor hardening, 1 test gap. All adopted. CRITICAL CORRECTNESS #1 CzSpN — `dispose()` race: after `await provider.poll(...)` the post-await guard checked only `entry.status !== 'pending'`, NOT `this.disposed`. `dispose()` clears the registry maps and aborts the entry's signal but doesn't mutate `entry.status`, so a provider whose poll already resolved (or doesn't honor abort) could enter the success branch and call `result.persist({...})` — committing credentials on a shutting-down daemon. Added the `if (this.disposed) return;` guard symmetric with the top-of-method check. REAL DEFENSIVE DEFECTS #2 Cy_ZG — sync-throw escape: the `result.persist({signal})` call happens BEFORE the `new Promise` constructor that captures it (`persistTracker` is closed-over inside the constructor). A non-conforming provider whose persist throws synchronously (e.g. top-of-function validation) would escape past the outer `try/catch (await new Promise(...))` and become an `unhandledRejection` since `runPollTick` is fire-and-forget via `void`. Wrapped the persist invocation in a try/catch that routes the sync-throw into the same `persistError` branch. #3 CzSpe — runtime provider map: provider validation hardcoded `DEVICE_FLOW_SUPPORTED_PROVIDERS` even though `deps.deviceFlowProviders` is the documented extension hook for tests/future providers. Switched both POST validation and `/workspace/auth/status` `supportedDeviceFlowProviders` to derive from `deviceFlowProviderMap.keys()` — single source of truth matches the registry's `resolveProvider`. EDGE CASES / MINOR HARDENING #4 Cy_Y9 — `slow_down` re-clamp: `intervalMs += SLOW_DOWN_BUMP_MS` can push past `DEVICE_FLOW_MAX_INTERVAL_MS` (the bound that keeps `setTimeout` from clamping to TIMEOUT_MAX). Wrapped in `Math.min(MAX_INTERVAL_MS, ...)` symmetric with the doStart clamp. #5 Cy_ZF — `expiresInSec` lower bound: `0.5` was finite-positive and produced `expiresAt = now() + 500 ms` — first poll (clamped at ≥1 s) fires AFTER expiresAt → flow expires before any user could authorize. Added `DEVICE_FLOW_MIN_EXPIRES_IN_SEC = 30` (RFC 8628 §3.2 calls 5–30 minutes "reasonable"; sub-30s is non-compliant). #6 CzHOK — take-over response privacy: `initiatorClientId` was echoed to ANY take-over POST caller, including those with no `X-Qwen-Client-Id` header. Bearer-gated already, but the asymmetry "anonymous caller learns who started it" violated the no-header-as-privacy-signal contract. Now only echoed when the caller's id matches the entry's initiator. #7 CzSpd — production audit visibility: production audit sink dropped `line.hint`, but the registry uses hints for operator-only breadcrumbs (`provider.poll() threw (raw)...`, `lost_success_after_timeout`, `persist_also_failed_past_expiry`, take-over correlation, `deferred (persist in flight; ...)`). The documented troubleshooting trail was invisible in production stderr. Now included with a 1 KiB bound + JSON-quoted so multi- word hints stay parseable. TEST GAP #8 Cy_ZH — `lost_success_after_timeout` audit: the fold-in 9 #7 split-brain detector for non-cooperative providers had no test pinning it. Added a controllable `latePersist` Promise + test that drives poll → success → enters persist race → fires PERSIST_TIMEOUT (registry publishes persist_failed) → resolves persist late → asserts the lost_success audit fires. Validation: typecheck + lint clean; cli serve 644/644 (+1 from the new test); sdk-typescript 384/384. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
46 tasks
doudouOUC
added a commit
that referenced
this pull request
May 18, 2026
Adds POST /workspace/tools/:name/enable — strict-gated mutation route that toggles a tool name in the workspace's `tools.disabled` settings list. Pure file IO + workspace-scoped event fan-out; no ACP roundtrip. - Bridge `setWorkspaceToolEnabled(toolName, enabled, originatorClientId)` invokes the new `BridgeOptions.persistDisabledTools` callback. The default `runQwenServe` wires it to `loadSettings(workspace).setValue( 'tools.disabled', merged)` with a fresh load on each call so concurrent edits from other writers stay safe across the read/modify/write window - New private `broadcastWorkspaceEvent` helper fan-outs to every live session SSE bus, swallowing per-bus errors so a single torn-down session can't block its peers. Naming mirrors PR 21 #4255 (the post- PR-16 fold-in will collapse the two helpers) - Unknown tool names are accepted: the daemon has no authoritative tool registry to validate against (built-ins live inside the ACP child, MCP tools are discovered post-spawn). Pre-disabling a not-yet-installed MCP tool is a legitimate use case - Live ACP children retain already-registered tools — the toggle takes effect on the next ACP child spawn (`tools.disabled` is consulted at Config construction time, gated in ToolRegistry.registerTool by PR 17 commit 2) SDK additions: - `DaemonClient.setWorkspaceToolEnabled(toolName, enabled, clientId?)` with URL-encoded tool name - `DaemonToolToggleResult` + `DaemonToolToggledEvent` typed event, reducer integration on `DaemonSessionViewState` (`toolToggleCount` / `lastToolToggle`) - `asKnownDaemonEvent` runtime guard for `tool_toggled` AND `approval_mode_changed` (the latter was missed in commit 3 — without this entry the events were silently filed as `unrecognizedKnownEvent` by `reduceDaemonSessionEvent`, never reaching the typed reducer cases) New capability tag `workspace_tool_toggle` (always-on, since v1). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
previously requested changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] npm run typecheck fails in packages/sdk-typescript/test/unit/daemonEvents.test.ts because the synthetic slow_client_warning object around line 664 is inferred with type: string, so it is not assignable to DaemonEvent when passed to asKnownDaemonEvent at line 670. The targeted Vitest file still passes, but the PR is not typecheck-clean. Please give the object a DaemonEvent annotation or otherwise preserve the literal discriminator type, e.g. const warning: DaemonEvent = { ... }.
— gpt-5.5 via Qwen Code /review
…und-13 #1) gpt-5.5 /review caught a real workspace-wide cap bypass: `countActive()` only counted entries already installed in `byProvider`, but the cap check at the top of `start()` runs before any provider's `inFlightStarts` slot completes `provider.start()`. A burst of fresh starts for `DEVICE_FLOW_MAX_CONCURRENT + 1` distinct providers all run synchronously to the cap check (each `start()` is async but runs to its first await — the await happens AFTER the cap check), all observe `count === 0` (no `byProvider` entries installed yet), and all pass — eventually installing more than the documented four pending flows. Fix: include `inFlightStarts.size` in `countActive()`. The two maps are disjoint by construction (the existing-pending fast-path catches any provider with both), so simple addition cannot double-count. The second concurrent caller sees count=1, the third count=2, …, and the (MAX+1)th caller is rejected with `TooManyActiveDeviceFlowsError`. Test: `caps at DEVICE_FLOW_MAX_CONCURRENT under CONCURRENT distinct-provider starts`. Fires `MAX+1` concurrent starts via `Promise.allSettled`, asserts exactly `MAX` fulfilled + exactly 1 rejected with the typed error. Pre-fix this test fails (all `MAX+1` succeed); post-fix it passes. Validation: typecheck clean across all 4 workspaces; deviceFlow.test.ts 35/35 (was 34); cli serve 645/645. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
|
Empirically refuted — the project's The mismatch with your /review tool's claim comes from "include": ["src/**/*.ts"],
"exclude": ["node_modules", "dist", "test", ".integration-tests"]This is the project's intentional contract: tests run via vitest (transpiled by SWC/esbuild without strict type checking), and Same finding as |
See PR comment #4255 (comment) — empirical refutation: npm run typecheck is clean across all 4 workspaces; the project's tsconfig deliberately excludes test/ (intentional contract). The /review tool's claim points to a pre-existing widening pattern from Stage 1 commit 870bdf2 that is NOT a PR 21 regression and NOT visible to CI. Dismissing as not-actionable on this PR.
wenshao
approved these changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
Collaborator
Post-merge local validation reportVerified the merged commit ( Build & static checks
Unit tests (PR surface, 452 cases)
Real end-to-end HTTP smoke (tmux + curl against live Qwen IdP)Daemon launched:
Caveats / known noiseTwo failures ( VerdictThe implementation matches the PR description point-by-point under live traffic: idempotent take-over, in-flight coalesce, terminal grace + secret redaction, error mapping, runtime-locality guarantee, and session-scoped event fan-out. The merged code is healthy as shipped — no follow-up correctness fix required from this validation pass. Outstanding design decision still on me: |
wenshao
requested changes
May 18, 2026
doudouOUC
added a commit
that referenced
this pull request
May 18, 2026
Adds POST /workspace/tools/:name/enable — strict-gated mutation route that toggles a tool name in the workspace's `tools.disabled` settings list. Pure file IO + workspace-scoped event fan-out; no ACP roundtrip. - Bridge `setWorkspaceToolEnabled(toolName, enabled, originatorClientId)` invokes the new `BridgeOptions.persistDisabledTools` callback. The default `runQwenServe` wires it to `loadSettings(workspace).setValue( 'tools.disabled', merged)` with a fresh load on each call so concurrent edits from other writers stay safe across the read/modify/write window - New private `broadcastWorkspaceEvent` helper fan-outs to every live session SSE bus, swallowing per-bus errors so a single torn-down session can't block its peers. Naming mirrors PR 21 #4255 (the post- PR-16 fold-in will collapse the two helpers) - Unknown tool names are accepted: the daemon has no authoritative tool registry to validate against (built-ins live inside the ACP child, MCP tools are discovered post-spawn). Pre-disabling a not-yet-installed MCP tool is a legitimate use case - Live ACP children retain already-registered tools — the toggle takes effect on the next ACP child spawn (`tools.disabled` is consulted at Config construction time, gated in ToolRegistry.registerTool by PR 17 commit 2) SDK additions: - `DaemonClient.setWorkspaceToolEnabled(toolName, enabled, clientId?)` with URL-encoded tool name - `DaemonToolToggleResult` + `DaemonToolToggledEvent` typed event, reducer integration on `DaemonSessionViewState` (`toolToggleCount` / `lastToolToggle`) - `asKnownDaemonEvent` runtime guard for `tool_toggled` AND `approval_mode_changed` (the latter was missed in commit 3 — without this entry the events were silently filed as `unrecognizedKnownEvent` by `reduceDaemonSessionEvent`, never reaching the typed reducer cases) New capability tag `workspace_tool_toggle` (always-on, since v1). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
5 tasks
doudouOUC
added a commit
that referenced
this pull request
May 18, 2026
Adds POST /workspace/tools/:name/enable — strict-gated mutation route that toggles a tool name in the workspace's `tools.disabled` settings list. Pure file IO + workspace-scoped event fan-out; no ACP roundtrip. - Bridge `setWorkspaceToolEnabled(toolName, enabled, originatorClientId)` invokes the new `BridgeOptions.persistDisabledTools` callback. The default `runQwenServe` wires it to `loadSettings(workspace).setValue( 'tools.disabled', merged)` with a fresh load on each call so concurrent edits from other writers stay safe across the read/modify/write window - New private `broadcastWorkspaceEvent` helper fan-outs to every live session SSE bus, swallowing per-bus errors so a single torn-down session can't block its peers. Naming mirrors PR 21 #4255 (the post- PR-16 fold-in will collapse the two helpers) - Unknown tool names are accepted: the daemon has no authoritative tool registry to validate against (built-ins live inside the ACP child, MCP tools are discovered post-spawn). Pre-disabling a not-yet-installed MCP tool is a legitimate use case - Live ACP children retain already-registered tools — the toggle takes effect on the next ACP child spawn (`tools.disabled` is consulted at Config construction time, gated in ToolRegistry.registerTool by PR 17 commit 2) SDK additions: - `DaemonClient.setWorkspaceToolEnabled(toolName, enabled, clientId?)` with URL-encoded tool name - `DaemonToolToggleResult` + `DaemonToolToggledEvent` typed event, reducer integration on `DaemonSessionViewState` (`toolToggleCount` / `lastToolToggle`) - `asKnownDaemonEvent` runtime guard for `tool_toggled` AND `approval_mode_changed` (the latter was missed in commit 3 — without this entry the events were silently filed as `unrecognizedKnownEvent` by `reduceDaemonSessionEvent`, never reaching the typed reducer cases) New capability tag `workspace_tool_toggle` (always-on, since v1). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
This was referenced May 18, 2026
This was referenced May 18, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements issue #4175 Wave 4 PR 21. Brokers OAuth 2.0 Device Authorization Grant (RFC 8628) through the
qwen servedaemon so a remote SDK client can trigger a Qwen-account login whose tokens land on the daemon filesystem, not on the client. The daemon polls the IdP itself; the client's only job is to display the verification URL + user code.Runtime locality (#4175 §11): the daemon NEVER spawns a browser or calls
open(url)— even when running locally. A static-source grep test fails the build onnode:child_process/open/xdg-open/shell.openExternal/execa/shelljs/process.spawnand their dynamic-import / require variants.What ships
4 new HTTP routes under
/workspace/auth/:POST/workspace/auth/device-flowmutate({ strict: true })— strict; idempotent take-over for repeat POSTs on the sameproviderIdGET/workspace/auth/device-flow/:idbearerAuthonly — pending entries echouserCode/verificationUri/expiresAt; terminal entries (5-min grace) drop themDELETE/workspace/auth/device-flow/:idGET/workspace/auth/status1 capability tag:
auth_device_flow(advertised unconditionally).5 new typed daemon events workspace-scoped, fanned out to every active session bus:
auth_device_flow_started/_throttled/_authorized/_failed/_cancelledSDK surface (
@qwen-code/sdk/packages/sdk-typescript):DaemonClientmethods + lazyclient.authgetter exposingDaemonAuthFlow.start(...).awaitCompletion()mirroringgh auth login's "show code first, let consumer decide where to open browser" UXreduceDaemonAuthEventparallel toreduceDaemonSessionEventSide fixes in
packages/core/src/qwen/qwenOAuth2.ts:cacheQwenCredentials(was private; needed by the daemon's device-flow registry)SharedTokenManager.clearCache()call intocacheQwenCredentials(was previously a paired call site at L820+L829)0o600onoauth_creds.json(was default umask)Notable design decisions
providerIdsingleton + idempotent take-over. Repeat POST returns the existing pending entry withattached: truerather than allocating a newdevice_code. Concurrent calls coalesce through an in-flight Promise map so two parallel POSTs don't double-allocate.BrandedSecret<T>holdsdevice_code+ PKCE verifier. After a pre-PR specialist pass found the earliernew String()shape leaked through+/ template literals (Symbol.toPrimitivereturned the primitive), reimplemented as frozen plain object +WeakMap+ 4-way redaction (toString/toJSON/Symbol.toPrimitive/numeric coercion →'[redacted]'/NaN) +unique symbolbrand. 6 leak-path tests.provider.persist()order: disk write FIRST, in-processsetCredentialssecond — a failed disk write no longer leaves a zombie in-memory token.provider.poll()acceptssignal; cancel/dispose mid-poll honors RFC 8628 single-use semantics. Lost-success branch (poll succeeded after entry transitioned) recordsstatus: 'lost_success'audit for incident-response correlation.transitionTerminalreturns boolean; every emit/audit guarded to prevent sweeper × poll-tick double-fire.bridge.broadcastWorkspaceEventis intentionally distinct from PR 16'spublishWorkspaceEventto avoid merge conflict; collapses to the shared helper as a fold-in (~25 LoC) once feat(serve): workspace memory and agents CRUD (#4175 Wave 4 PR 16) #4249 lands.BridgeClient.dispose()wired intorunQwenServe.close()'s shutdown drain.Engineering principles checklist
qwenOAuth flow unchanged)qwen serveStage 1 routes / SDK behavior preservedqwen-oauth; future providers register throughDeviceFlowProviderinterface)Pre-PR specialist passes (fold-in 0)
3 specialist agents reviewed the implementation before opening:
code-reviewer,silent-failure-hunter,type-design-analyzer. 12 P0/P1 items folded in — see #4175 (comment) for the full list. Highlights: BrandedSecret rewrite (above),dispose()shutdown wiring, concurrent-start coalescing, persist order,poll()signal, transitionTerminal returns boolean, anchored RFC 8628 error regex, dead-code removal, SDK reducer alignment with daemon, broadened static-source grep, sweeper integration test, 502upstream_errormapping.Coordination items (recorded in #4175 comment)
bridge.broadcastWorkspaceEvent→ fold-in to PR 16'spublishWorkspaceEventonce feat(serve): workspace memory and agents CRUD (#4175 Wave 4 PR 16) #4249 lands/workspace/auth/statusvs PR 12/workspace/providersboundary — separate route in v1; merge alternative discussed (need @wenshao input)mutate({ strict: true })+ workspace event-fan-out patternFold-in 1 (deferred, post-merge)
5 items from specialist passes parked for a focused follow-up PR after this lands:
DeviceFlowEntry→ discriminated union overstatusDaemonAuthFlowHandle.awaitCompletionmemoizebroadcastWorkspaceEventelevate to stderr when ALL session buses failawaitCompletion404 →'not_found_or_evicted'errorKindTest plan
npx vitest run packages/cli/src/serve/— 204 passednpx vitest run packages/sdk-typescript/test/unit/daemonEvents.test.ts— 28 passednpm run typecheck— clean across cli + sdk-typescript packagesnpx eslint --max-warnings 0 <PR 21 surface>— cleanJSON.stringify/String()/+/ template / numeric / reveal)start()race —Promise.all([start, start, start])assertsprovider.startCount === 1upstream_errormapping forprovider.start()IdP failuresqwen serve --token=dev+ SDKclient.auth.start({providerId: 'qwen-oauth'})against staging Qwen IdP/workspace/auth/statusvs PR 12 boundaryRefs: #4175
🤖 Generated with Qwen Code