mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-20 01:01:53 +00:00
36760ca63c
* feat(serve): auth device-flow route
Implements issue #4175 Wave 4 PR 21. Brokers OAuth 2.0 Device
Authorization Grant (RFC 8628) through the `qwen serve` daemon 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. Static-source grep
test fails the build on `node:child_process` / `open` / `xdg-open` /
`shell.openExternal` / `execa` / `shelljs` / `process.spawn` and
their dynamic-import / require variants.
- `POST /workspace/auth/device-flow` — strict mutation gate; returns
201 fresh / 200 idempotent take-over with `attached: true`. Per
per-`providerId` singleton: a second POST while pending takes over
rather than allocating a new `device_code`.
- `GET /workspace/auth/device-flow/:id` — public state read. Pending
entries echo `userCode/verificationUri/expiresAt/intervalMs`;
terminal entries (5-min grace) drop them and surface
`status/errorKind/hint`.
- `DELETE /workspace/auth/device-flow/:id` — strict; idempotent
(terminal → 204 no-op; unknown → 404).
- `GET /workspace/auth/status` — pending flows + supported providers
snapshot. v1 stub for `providers: []` (populated in fold-in 1).
`DeviceFlowRegistry` (`packages/cli/src/serve/auth/deviceFlow.ts`)
is the in-memory state holder:
- per-`providerId` singleton with idempotent take-over
- workspace-wide cap of 4 active flows (abuse defense)
- 5-min terminal grace so SDK reconnects can still observe results
- TTL sweeper evicts grace-expired entries every 30s
- in-flight `Promise` map coalesces concurrent `start()` calls so two
parallel POSTs don't double-allocate IdP `device_code`
- `transitionTerminal` returns `boolean` so caller-side emit/audit
guard prevents sweeper × poll-tick double-fire
- `dispose()` wired into `runQwenServe.close()`'s shutdown drain;
cancels `provider.poll()` mid-flight via `cancelController`,
records `lost_success` audit when an IdP-minted token is dropped
by transition
`DeviceFlowProvider` interface accepts `start({signal})` +
`poll(state, {signal})`. `QwenOAuthDeviceFlowProvider` wraps the
existing `QwenOAuth2Client.requestDeviceAuthorization` /
`pollDeviceToken` primitives directly (NOT
`authWithQwenDeviceFlow`, which calls `open(url)`). PKCE is
provider-required by Qwen but optional in the interface for future
non-PKCE providers. `success.persist()` writes to disk FIRST, then
updates the in-process client — a failed disk write no longer
leaves the daemon with a zombie in-memory token. Maps RFC 8628
errors via an anchored regex (`^Device token poll failed:
(expired_token|access_denied|invalid_grant)`) so an
`error_description` containing one of those literals can't
mis-classify an unrelated upstream error.
`BrandedSecret<T extends string>` holds the `device_code` and PKCE
verifier. Earlier draft used `new String()` wrapper which leaked
through `+` / template literals (`Symbol.toPrimitive` →
`valueOf` returned the primitive). Final shape: frozen plain object
+ `WeakMap` indirection + 4-way redaction
(`toString` / `toJSON` / `Symbol.toPrimitive` / numeric coercion →
`'[redacted]'` or `NaN`) + `unique symbol` brand. 6 leak-path
tests: `JSON.stringify` / `String()` / concat / template / `+x` /
reveal-roundtrip.
5 new daemon events (workspace-scoped, fanned out to every active
session bus via `bridge.broadcastWorkspaceEvent`):
- `auth_device_flow_started` — `{deviceFlowId, providerId, expiresAt}`
(no userCode/verificationUri — see PR 21 design §3)
- `auth_device_flow_throttled` — `{deviceFlowId, intervalMs}`,
emitted only on upstream `slow_down` interval bumps
- `auth_device_flow_authorized` — `{deviceFlowId, providerId,
expiresAt?, accountAlias?}`; `accountAlias` is best-effort
non-PII (never email/phone)
- `auth_device_flow_failed` — `{deviceFlowId, errorKind, hint?}`
with `errorKind ∈ {expired_token, access_denied, invalid_grant,
upstream_error, persist_failed}`
- `auth_device_flow_cancelled` — `{deviceFlowId}` (DELETE on pending)
Workspace-scoped reducer `reduceDaemonAuthEvent` produces
`DaemonAuthState { flows: Partial<Record<ProviderId, ...>> }` —
parallel to `reduceDaemonSessionEvent`. Session reducer no-ops on
auth events (workspace-scoped state belongs in its own reducer).
`bridge.broadcastWorkspaceEvent` is intentionally distinct from PR
16's `publishWorkspaceEvent` to avoid merge conflict; collapses to
the shared helper as a fold-in once #4249 lands (~25 LoC).
`@qwen-code/sdk` (`packages/sdk-typescript/`):
- 4 new `DaemonClient` methods: `startDeviceFlow`, `getDeviceFlow`,
`cancelDeviceFlow`, `getAuthStatus` — typed against the wire
shapes, errors mapped through the existing `DaemonHttpError`.
- High-level `client.auth` getter (lazy `DaemonAuthFlow` singleton)
exposes a `start(...).awaitCompletion()` shape mirroring `gh auth
login`'s UX: print code first, let the SDK consumer decide where
to open the browser. `awaitCompletion` polls GET on the
daemon-supplied `intervalMs`, honors `slow_down` bumps, and
fall-back-recovers from 404 (entry evicted post-grace).
POST + DELETE flow through PR 15's `mutate({strict: true})` —
401 `token_required` on token-less loopback defaults. GET routes
use only the global `bearerAuth`. Every state transition
(`started/authorized/failed/cancelled/expired/lost_success`)
records a structured stderr breadcrumb (`[serve] auth.device-flow:
provider=... deviceFlowId=abc12... clientId=... status=...`)
since `mutate()` doesn't carry an audit hook — events alone aren't
enough since SDK can silently drop them; stderr → journald/docker
logs is the unfalsifiable record.
`auth_device_flow` advertised unconditionally on
`/capabilities.features`. Supported providers list lives on
`/workspace/auth/status` to keep the registry descriptor uniform.
- `packages/core/src/qwen/qwenOAuth2.ts`:
- exports `cacheQwenCredentials` (was a private function; needed
by the daemon's device-flow registry)
- `cacheQwenCredentials` now calls `SharedTokenManager.clearCache()`
after writing, folding what was previously a paired call site at
L820+L829. Idempotent change.
- file mode `0o600` on `oauth_creds.json` (was default 0o666 +
umask). Mirrors opencode's `auth/index.ts`.
- `packages/cli/src/serve/runQwenServe.ts`: device-flow registry
`dispose()` wired into the shutdown drain (BEFORE
`bridge.shutdown()`).
- `auth/deviceFlow.test.ts` — 21 tests: BrandedSecret leak paths,
state machine (slow_down / success / error), terminal grace,
concurrent-start coalescing, dispose, cancel idempotency, static-
source grep against browser-spawn primitives.
- `server.test.ts` — 10 device-flow integration tests:
POST 201/200 take-over, strict 401, 400 `unsupported_provider`,
GET / DELETE / `/workspace/auth/status`, 502 `upstream_error`
mapping, sweeper-driven auto-expiry with controlled clock,
capability advertisement.
- `daemonEvents.test.ts` — 5 SDK reducer tests: type guards, per-
provider state projection, `failed` always → `status: 'error'`
(errorKind carries the kind, including new `persist_failed`),
session reducer no-ops on auth events.
369/369 serve + SDK tests pass; typecheck + `eslint
--max-warnings 0` clean across 14 PR 21 files.
- [x] Independently mergeable (depends only on merged PR 4 / PR 7 /
PR 12 / PR 15)
- [x] Backward compatible (4 new routes + 1 capability tag + 5 typed
events + 4 SDK helpers; existing routes/events untouched)
- [x] Default off (capability advertised but no client is forced to
use it; CLI `qwen` OAuth flow unchanged)
- [x] `qwen serve` Stage 1 routes / SDK behavior preserved
- [x] Gradual migration (v1 only `qwen-oauth`; future providers
register through the `DeviceFlowProvider` interface)
- [x] Reversible (revert removes 4 routes + 1 tag + 5 events with no
schema migration)
- [x] Tests-first (28 new tests across 3 layers)
- Inline `bridge.broadcastWorkspaceEvent` → fold-in to PR 16 (#4249)
`publishWorkspaceEvent` once that lands
- `/workspace/auth/status` vs PR 12 `/workspace/providers` boundary
— separate route in v1; merge alternative discussed
- Wave 4 PRs 17/19/20 should adopt the same mutate-strict +
workspace event-fan-out pattern
5 items from pre-PR specialist passes parked for a focused
follow-up: `DeviceFlowEntry` discriminated union, single-source SDK
status / ProviderId unions, `awaitCompletion` memoization,
broadcast-100%-fail stderr elevation, SDK 404 →
`not_found_or_evicted` errorKind.
Refs: #4175
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fixup(serve): address PR #4255 round-1 review feedback
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)
* fixup(serve): address PR #4255 wenshao round-1 review feedback
15 items from @wenshao's review batches on #4255. Catches a handful
of real bugs that the earlier round (commit 3d9f082f5) 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)
* fixup(serve): address PR #4255 wenshao round-2 review feedback
10 new threads from @wenshao's second deep-review pass on #4255.
Verified status: 5 real issues, 1 improvement, 3 stale (already
fixed; comments lagged), 1 false alarm (typecheck demonstrably
clean).
## Critical fixes
- **fold-in 2 C4 REVERSED**: when `provider.poll()` returns success
AND `cancel()` / `dispose()` transitioned the entry mid-`persist()`,
the registry now FORCES the entry to `authorized` and keeps the
on-disk credentials. The earlier rollback (`unpersist()`) wasted
the user's IdP approval because the RFC 8628 `device_code` is
single-use — re-running the flow would force them through the
whole browser-prompt + paste-code dance again for a click whose
intent was likely "stop the wait" rather than "undo my already-
completed approval". Aligns with gh CLI / Auth0 SDK / git-
credential-manager. Audit captures the race via `hint:
'lost_success_kept ...'`. `DeviceFlowPollResult.success.unpersist`
field + Qwen provider's `clearQwenCredentials` rollback removed.
- **#1 GET /workspace/auth/device-flow/:id strict gate**: this GET
surfaces `userCode` / `verificationUri` for pending entries, which
on the loopback no-token default were readable by any local
process. POST + DELETE were already strict; aligning GET closes
the information-disclosure asymmetry. `/workspace/auth/status`
stays bearer-only (its `pendingDeviceFlows` entries intentionally
omit `userCode`).
- **#2 `inFlightStarts` hard timeout**: a hung `provider.start()`
(network partition, unresponsive IdP) used to leave the per-
`providerId` slot in `inFlightStarts` occupied forever, blocking
every subsequent POST until daemon restart. New
`DEVICE_FLOW_START_TIMEOUT_MS = 30_000` arms a timer that
`cancelController.abort()`s the start; the rejected promise
unwinds through the `try/finally` clearing the slot.
- **#10 chain-completing the C3 persist-timeout**: the earlier C3
fix armed a 30s timer that fired `cancelController.abort()` then
`await result.persist({signal})`, but the chain ended at the
registry boundary — `cacheQwenCredentials` didn't take a signal,
so `fs.writeFile` couldn't be aborted. Now `cacheQwenCredentials`
accepts an optional `{signal}` and threads it into
`fs.writeFile(..., {signal})` (Node native). The Qwen provider's
`persist({signal})` forwards the entry's
`cancelController.signal` end-to-end.
## Improvement (#4): 404 fallback errorKind
`pollUntilTerminal`'s 404 catch used to synthesize
`{status: 'expired'}` for ALL evicted entries — conflating "your
flow expired during your disconnect", "the daemon was restarted",
and "your deviceFlowId was wrong". Now returns
`status: 'error'` + `errorKind: 'not_found_or_evicted'` + a `hint`
so SDK consumers branching on errorKind can distinguish.
## Information leak (#9): start() path raw IdP message
S2 (fold-in 2) sanitized `poll()`'s upstream-error hint, but
`start()` still embedded the raw `err.message` (full IdP response,
potentially HTML from a reverse proxy / WAF) into the
`UpstreamDeviceFlowError` that flowed to SDK clients via the 502.
Now uses static messages for the SDK-visible errors; raw detail
goes through `writeStderrLine` for operator audit only. Mirrors
S2's approach.
## Stale comments cleaned (#5, #7)
`qwenDeviceFlowProvider.ts:177` claimed
`cacheQwenCredentials` "doesn't currently take a signal — that's
a follow-up". After #10 above, that's no longer true; the comment
is replaced with the actual end-to-end signal-threading note.
## Not adopted (1 false alarm)
- Thread on `types.ts:330` claimed type-only-import-after-
declarations breaks `tsc` and fails `daemonEvents.test.ts:670`
with TS2345. Demonstrably false: `npx tsc -p
packages/sdk-typescript/tsconfig.json --noEmit` exits 0;
`daemonEvents.test.ts` is the post-fold-in-2 file with the
open-allowlist assertion (test 28/28 passes). The reviewer may
have been looking at a transient state during their analysis.
## 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` — 398/398
pass
- `npx eslint --max-warnings 0` over the PR 21 surface — clean
Refs: #4175 #4255
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fixup(serve): address PR #4255 wenshao round-3 review feedback
5 new threads from the third deep-review pass on #4255. 3 real
issues fixed; 1 stale (already done in fold-in 3); 1 deferred as
non-blocking design suggestion.
- **A — `expiresIn` / `interval` non-finite guard**
(`deviceFlow.ts`). The provider contract types both as `number`,
but a misbehaving / future provider could hand `undefined` /
`NaN` / `Infinity`. `Math.max(0, NaN) * 1000` is `NaN`, then
`now() + NaN` is `NaN`, then `now >= NaN` is always `false` —
the sweeper would NEVER evict the entry, pinning an upstream
`device_code` slot until daemon restart. Same hazard on
`interval * 1000` (NaN → `setTimeout(NaN)` fires immediately,
Infinity → scheduler clamps to TIMEOUT_MAX). Now both fields go
through `Number.isFinite(x) && x > 0`; missing/bad values fall
back to RFC 8628's recommended ceilings (10 min for expiry, 5s
for interval).
- **D — typed `app.locals` accessor**
(`deviceFlow.ts` + writer/reader call sites). The
`app.locals['deviceFlowRegistry']` string key was shared between
`createServeApp` (writer) and `runQwenServe` (reader); a typo on
either side would compile cleanly and the shutdown dispose call
would silently no-op, leaving polling timers running until the
`unref()` rescue. New `setDeviceFlowRegistry(app, registry)` /
`getDeviceFlowRegistry(app)` pair gives both call sites
type-checked access; the string literal is encapsulated in one
module.
- **E — `UnsupportedDeviceFlowProviderError` docstring**
(`deviceFlow.ts`). After fold-in 2's W2 fix split
`invalid_request` from `unsupported_provider`, the route layer
screens unknown ids against `DEVICE_FLOW_SUPPORTED_PROVIDERS`
before reaching the registry — so this error is now reachable
ONLY on a daemon-internal invariant violation (id is declared
supported but not registered in the runtime provider map).
Docstring + thrown message updated to reflect that this branch
signals a programmer error, not user input.
- **B** claimed `cacheQwenCredentials(credentials)` doesn't forward
signal to `fs.writeFile`. Verified: fold-in 3 (#10) at
`qwenDeviceFlowProvider.ts:204` calls
`cacheQwenCredentials(credentials, { signal: persistOpts.signal })`
and the core helper threads it into `fs.writeFile(..., {mode,
signal})`. The reviewer was looking at the comment block above
(lines 174-181) without scrolling to the actual call site.
- **C — SDK `cancelDeviceFlow` lossy 204/404 collapse**.
Suggested returning `{existed: boolean; alreadyTerminal: boolean}`
instead of resolving void on both 204 and 404. Real signal-loss
but tagged "[非阻塞]" by the reviewer; changing requires a
daemon route shape change (200 + body instead of 204) which is
better as a focused follow-up PR. Acknowledged in-thread;
deferred to a fold-in PR after #4255 lands.
- `npm run typecheck` — clean across `packages/{cli,sdk-typescript,core}`
- `npx vitest run packages/cli/src/serve/
packages/sdk-typescript/test/unit/daemonEvents.test.ts` — 398/398
- `npx eslint --max-warnings 0` over the PR 21 surface — clean
Refs: #4175 #4255
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fixup(serve): address PR #4255 wenshao round-4 review feedback
4 threads from the fourth review pass on #4255. 3 adopted + 1
deferred (out-of-scope rename of PR 15's `mutate` helper).
## Adopted
### #1 — `persistInFlight` flag suppresses cancel × persist event-stream UX trap
When `provider.poll()` returns success and we await `persist()`, a
concurrent `cancel()` would synchronously transition the entry to
`cancelled` and emit `auth_device_flow_cancelled` — then `persist()`
resolves and (per fold-in 3 C4) force-overrides to `authorized` +
emits `auth_device_flow_authorized`. The reducer state correctly
last-write-wins on `authorized`, but DIRECT event-stream consumers
(close-dialog handlers, telemetry, UI cleanup) race onto an unmounted
UI when the second event lands.
Now: while persist is in-flight, `cancel()` and the sweeper SKIP the
state transition + event emit. They register intent (set
`cancelRequestedDuringPersist=true` for cancel; sweeper just no-ops)
and let the persist resolution decide:
- persist succeeds → `authorized` (IdP wins per fold-in 3 C4)
- persist fails AND cancel was requested → `cancelled`
- persist fails AND `now >= expiresAt` → `expired` / `expired_token`
- persist fails otherwise → `error` / `persist_failed`
Result: at most one terminal event per flow. Imperative SSE
consumers no longer see oscillating terminal states. Audit captures
the race (`hint: 'lost_success_kept ...'`) for incident-response
correlation.
### #2 — `revealSecret` → `unsafeRevealSecret` rename
The earlier JSDoc claimed "the `unsafeReveal_` naming is intentional:
greppable in code review, easy to allowlist in lint rules, hard to
invoke by accident" — but the actual function was named
`revealSecret`. The promised safety properties didn't exist; a code
reviewer wouldn't single out `revealSecret` as suspicious, and a
`no-restricted-syntax` ESLint rule wouldn't flag it.
Renamed to `unsafeRevealSecret` so the JSDoc-promised "greppable" /
"lintable" property is now actually true. Two call sites in the
Qwen provider + 4 test references updated. Internal symbol; not
exposed through the SDK package.
### #4 — `QwenOAuthPollError` typed class replaces substring regex
The earlier RFC 8628 error mapper used an anchored regex against the
thrown error message text — an implicit cross-file string contract
between `qwenOAuth2.ts` (throws) and `qwenDeviceFlowProvider.ts`
(matches). If `qwenOAuth2.ts` ever changed its message format, ALL
RFC 8628 errors (`expired_token` / `access_denied` / `invalid_grant`)
would silently fall through to `upstream_error` — wrong errorKind
flowing through telemetry with no test or type-system check to catch
the drift.
Now `QwenOAuth2Client.pollDeviceToken` throws a structured
`QwenOAuthPollError extends Error` with `oauthError` / `description`
/ `status` fields. The provider branches on `instanceof
QwenOAuthPollError` and reads `.oauthError` directly via a
dedicated `mapRfc8628OAuthCode(code)` switch. The drift hazard is
gone: a future code change that touches the typed class will
fail tsc until both sides are updated. Message format preserved
for any pre-existing log-parsing / substring matchers.
## Not adopted
### #3 — `mutate({strict:true})` semantic awkwardness on GET
Reviewer correctly noted that `mutate` is named for state-changing
routes, but `GET /workspace/auth/device-flow/:id` uses it for an
information-disclosure defense (only reachable code path is reading
state). Suggested rename: `mutate` → `strictHttpGate`.
Deferred: the rename touches PR 15's helper which has many call
sites in `server.ts` and is shared infrastructure for Wave 4 PRs
17/19/20. PR 21 is the first / only consumer of the strict-on-GET
form so far; widening the rename to a Wave 4 follow-up keeps the
fold-in scope tight. Replied in-thread.
## Validation
- `npm run typecheck` — clean across `packages/{cli,sdk-typescript,core}`
- `npx vitest run packages/cli/src/serve/
packages/sdk-typescript/test/unit/daemonEvents.test.ts` — 544/544
- `npx eslint --max-warnings 0` over the PR 21 surface — clean
Refs: #4175 #4255
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fixup(serve): address PR #4255 wenshao round-5 review feedback
Five small adopt items from the round-5 review pass; one stale thread
already addressed in
|
||
|---|---|---|
| .. | ||
| daemon-client-adapters | ||
| development | ||
| examples | ||
| tools | ||
| _meta.ts | ||
| architecture.md | ||
| channel-plugins.md | ||
| contributing.md | ||
| qwen-serve-protocol.md | ||
| roadmap.md | ||
| sdk-java.md | ||
| sdk-python.md | ||
| sdk-typescript.md | ||