feat(serve): mutation gating helper and --require-auth#4236
Merged
Conversation
Contributor
📋 Review SummaryThis PR (#4236) implements a centralized mutation gate helper and 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a centralized mutation-gating helper for serve mutation routes and introduces a --require-auth boot flag that hardens the loopback developer default by forcing bearer-token enforcement on every route (including /health). Today's mutation routes adopt the helper as a no-op marker; Wave 4 routes will opt into strict: true later. A conditional require_auth capability tag is advertised only when the operator opts in.
Changes:
- New
createMutationGate({ tokenConfigured, requireAuth })factory inserve/auth.tswith a 4-cell behavior matrix; existing mutation routes threadmutate()as a centralization marker. - New
--require-authCLI flag andServeOptions.requireAuth; boot refuses without a token, drops the loopback/healthexemption, and emits a stderr breadcrumb. - New
CONDITIONAL_SERVE_FEATURESregistry primitive plusAdvertiseFeatureToggles;require_authtag advertised only when toggled on; docs updated.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/serve/auth.ts | Adds createMutationGate factory + types implementing the gate matrix. |
| packages/cli/src/serve/auth.test.ts | New unit tests covering the four matrix cells and passthrough identity caching. |
| packages/cli/src/serve/server.ts | Wires mutate() onto existing mutation routes; conditionally registers /health pre/post bearer based on requireAuth. |
| packages/cli/src/serve/server.test.ts | Adds capability advertisement tests and /health + boot-check coverage for --require-auth. |
| packages/cli/src/serve/capabilities.ts | Adds require_auth to registry, CONDITIONAL_SERVE_FEATURES, and toggle-aware advertisement. |
| packages/cli/src/serve/types.ts | Documents new requireAuth?: boolean on ServeOptions. |
| packages/cli/src/serve/runQwenServe.ts | Boot-check refusing --require-auth without token; stderr breadcrumb when enabled. |
| packages/cli/src/serve/index.ts | Re-exports new auth helpers, types, and CONDITIONAL_SERVE_FEATURES. |
| packages/cli/src/commands/serve.ts | Adds --require-auth yargs option, wired to runQwenServe. |
| docs/users/qwen-serve.md | User-facing docs for --require-auth with example. |
| docs/developers/qwen-serve-protocol.md | Protocol doc updates for --require-auth, conditional tags table, and corrected feature list snippet. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
doudouOUC
added a commit
that referenced
this pull request
May 17, 2026
Three small follow-ups from the automated reviewers on PR #4236: 1. **Drop misleading `--require-auth` from `token_required` error message** (Copilot inline auth.ts:262). The strict-mode 401 listed three remediations but `--require-auth` is paired-required with a token at boot — naming it standalone would loop the operator into a different boot error. Keep the two valid standalone fixes (env var, --token); add inline note explaining the omission. `auth.test.ts` regex updated to `not.toMatch(/--require-auth/)` to anchor the new wording. 2. **Mention `/health` gating in `--require-auth` CLI description** (auto-reviewer Medium #2). Operators flipping the flag without reading the protocol doc would get paged when k8s/Compose probes start 401-ing. One sentence in the yargs description prevents that. 3. **Drift insurance comment between registry and `CONDITIONAL_SERVE_FEATURES`** (auto-reviewer Low #3). Document the four-step procedure for adding a new conditional tag so a future contributor doesn't update only the registry and silently advertise the tag unconditionally. Notes the Map<predicate> refactor as the right move when a second tag lands. Deferred (not in this fix-up): - Module-level PASSTHROUGH singleton (High #1) — micro-optimization, unmeasurable. - Map<feature, predicate> for conditional features (High #2) — premature abstraction with one tag. - Per-route `// non-strict marker` comments (Medium #1) — noise. - `@see` cross-ref in types.ts (Low #2) — sugar. - JSDoc bullet-list vs table (Low #1) — current format is fine. Refs: #4175 #4236 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
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
|
Replying point-by-point to the Review Summary (github-actions). Adopt-now items pushed in 21f46af; defer/reject reasoning below. 🟡 High
🟢 Medium
🔵 Low
Validation after fix-up
Net of this comment + 21f46af: 3 of 7 suggestions adopted (the two correctness/operator-visibility items + drift insurance), 4 deferred or rejected with reasoning. Ready for human review. |
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Nit pass — four non-blocking suggestions. Overall the PR is well-scoped and disciplined about backward compatibility; the createMutationGate shape gives Wave 4 routes a clean handoff. Tests + boot-loud rejection of --require-auth + no token are particularly nice.
Collaborator
Author
|
Acknowledging the Code Coverage Summary — informational only, no action needed. Coverage on the files this PR touches (post-21f46af0):
No coverage threshold was breached and no specific lines are flagged in the bot's report; closing as no-action. |
Implements issue #4175 Wave 4 PR 15. Adds the centralized state-changing-route gate that Wave 4 follow-ups (memory CRUD, file edit, MCP restart, device-flow auth) will reuse, plus the `--require-auth` deployment knob that hardens the loopback developer default for shared dev hosts / CI runners. - `createMutationGate({ tokenConfigured, requireAuth })` factory in serve/auth.ts — per-route middleware with a 4-cell behavior matrix: pass-through under `requireAuth` or any token configured; `401 token_required` for `strict: true` routes on no-token loopback defaults; baseline pass-through otherwise. - Existing Wave 1-2 mutation routes (POST /session, /session/:id/{load, resume,prompt,cancel,model}, /permission/:requestId) opt into the default non-strict factory call as the centralization marker. Wave 4 routes will pass `{ strict: true }` to require a token even on loopback. - `--require-auth` CLI flag + `ServeOptions.requireAuth`. Boot refuses without a token; closes the `/health` exemption when on so loopback `/health` also requires bearer auth; stderr breadcrumb so the hardened mode is visible in journald/docker logs. - Conditional `require_auth` capability tag advertised only when the flag is on. New `CONDITIONAL_SERVE_FEATURES` registry primitive so future per-deployment toggles follow the same shape. - 5 new unit tests in auth.test.ts covering the gate matrix; 5 added in server.test.ts for capability advertisement, conditional tag, /health 401 under --require-auth, and runQwenServe boot refusal + happy path. 245/245 serve tests pass; typecheck + eslint clean. Refs: #4175 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Three small follow-ups from the automated reviewers on PR #4236: 1. **Drop misleading `--require-auth` from `token_required` error message** (Copilot inline auth.ts:262). The strict-mode 401 listed three remediations but `--require-auth` is paired-required with a token at boot — naming it standalone would loop the operator into a different boot error. Keep the two valid standalone fixes (env var, --token); add inline note explaining the omission. `auth.test.ts` regex updated to `not.toMatch(/--require-auth/)` to anchor the new wording. 2. **Mention `/health` gating in `--require-auth` CLI description** (auto-reviewer Medium #2). Operators flipping the flag without reading the protocol doc would get paged when k8s/Compose probes start 401-ing. One sentence in the yargs description prevents that. 3. **Drift insurance comment between registry and `CONDITIONAL_SERVE_FEATURES`** (auto-reviewer Low #3). Document the four-step procedure for adding a new conditional tag so a future contributor doesn't update only the registry and silently advertise the tag unconditionally. Notes the Map<predicate> refactor as the right move when a second tag lands. Deferred (not in this fix-up): - Module-level PASSTHROUGH singleton (High #1) — micro-optimization, unmeasurable. - Map<feature, predicate> for conditional features (High #2) — premature abstraction with one tag. - Per-route `// non-strict marker` comments (Medium #1) — noise. - `@see` cross-ref in types.ts (Low #2) — sugar. - JSDoc bullet-list vs table (Low #1) — current format is fine. Refs: #4175 #4236 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
21f46af to
da4bf18
Compare
Collaborator
Author
|
Rebased onto
Conflicts and how they were resolved
Validation post-rebase
Branch state
Force-pushed with |
wenshao
reviewed
May 17, 2026
Five small follow-ups from @wenshao + DeepSeek (via Qwen Code /review) on PR #4236: 1. **Map<predicate> refactor for `CONDITIONAL_SERVE_FEATURES`** (review threads #3254467192 + #3254485912). Two reviewers asked for the same shape on the grounds that the `Set` + per-feature `if`-branch needed FOUR coordinated changes per new conditional tag and silently fail-CLOSED when the branch was missed. The Map collapses the predicate-decision and the set-membership into one entry per feature — adding a new conditional tag is now two coordinated changes (registry + Map entry) and a missing predicate is a TypeScript error rather than a silent omission. JSDoc updated. 2. **Drift-insurance test that iterates `CONDITIONAL_SERVE_FEATURES`** (review thread #3254467192 option 1, layered on top of #1). `server.test.ts` now walks every Map entry and asserts the predicate accepts/rejects as expected; future entries that don't add an assertion branch fail the test loudly so a missing predicate cannot ship silently. Adoption-of-record for the Map shape rather than relying on a hand-maintained invariant. 3. **Cache `strictDenier` for allocation symmetry** (review thread #3254467193). Wave 4 PRs will mount strict mode on multiple routes; without the cache each `mutate({strict:true})` call would allocate a fresh 401 closure. Now both the passthrough and the strict denier are pre-built singletons. Identity assertion in `auth.test.ts` anchors the cache so a future change that loses it surfaces in CI. 4. **Doc cosmetic — extra blank line in qwen-serve.md** (review thread #3254467198). Single blank line between the `>` quoted example and the following non-quoted bash block now. 5. **Doc correctness — `require_auth` is post-auth confirmation** (review thread #3254485910 from DeepSeek). When `--require-auth` is on, the global `bearerAuth` middleware gates every route including `/capabilities`, so an unauthenticated client cannot pre-flight `caps.features` to discover that auth is required — the discovery surface is the 401 response body itself. Both `qwen-serve.md` and `qwen-serve-protocol.md` rewritten to describe the tag as a post-authentication confirmation, matching the auth.ts JSDoc which already stated this correctly. Trade-offs documented (no code change): - **Body-parser ordering** (review thread #3254485915 from DeepSeek) noted as a comment block in `auth.ts`. Strict-mode 401 fires AFTER `express.json()` because the gate is per-route middleware. On loopback no-token defaults a strict route therefore parses the request body before refusing it — bounded by `express.json({limit: '10mb'})` × `--max-connections` (256 default). Strict routes Wave 4 actually adds carry small bodies in legitimate use, so this isn't a production hot path. Future routes accepting large bodies should lift the gate to app-level (maintain a strict-path Set in `createServeApp`); flagged as a Wave 4 follow-up rather than re-architecting the helper. - **`bearerAuth` body-shape inconsistency** (review thread #3254467197 from @wenshao) flagged as a Wave 4 cross-PR follow-up. `bearerAuth` returns `{error: 'Unauthorized'}` while the strict gate returns `{code: 'token_required', error: '...'}`; SDK clients have to branch on both shapes. Standardizing `bearerAuth` to also carry a `code` field is orthogonal to this PR's scope. Validation: 260/260 cli serve tests pass (was 258 — added the drift insurance test + strict denier identity test); typecheck + eslint clean. Refs: #4175 #4236 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
approved these changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Verified locally on 6149866 (force-push after the round-2 review). All eight concerns from the prior two rounds are addressed, and the four-cell behavior matrix verified end-to-end against a real qwen serve listener.
Local validation
| Command | Result |
|---|---|
npm run typecheck --workspace packages/cli |
exit 0 |
npx vitest run packages/cli/src/serve |
271/271 passed across 5 files |
npx eslint packages/cli/src/serve packages/cli/src/commands/serve.ts |
0 problems |
Manual /capabilities + /health matrix probe
Spawned the daemon in all three operationally distinct modes and confirmed behavior:
Default (no --require-auth):
/healthno auth → 200 (loopback exemption intact)/capabilitiesno auth → 200,require_authabsent fromfeatures- stderr:
bearer auth disabled (loopback default)
--require-auth without a token:
- Boot refuses with
Refusing to start with --require-auth set but no bearer token configured. Set QWEN_SERVER_TOKEN or pass --token, or omit --require-auth ... - Exit code 1 (boot-loud, matches the existing non-loopback check)
--require-auth + token:
- stderr breadcrumb fires:
--require-auth enabled (bearer token mandatory on every route, including loopback /health). /healthno auth → 401 (loopback exemption correctly closed)/healthwithAuthorization: Bearer …→ 200/capabilitiesno auth → 401{"error":"Unauthorized"}/capabilitieswith auth → 200,require_authpresent infeatures- Wrong token → 401
{"error":"Unauthorized"}
The capability tag only appears post-authentication, matching the docs/developers/qwen-serve-protocol.md update describing it as a post-auth confirmation rather than a pre-flight discovery surface.
Prior-round concerns vs HEAD 6149866
- Error-message remediation list named
--require-authstandalone (Copilot) →auth.ts:276-291lists onlyQWEN_SERVER_TOKENand--token;auth.test.ts:85anchorsnot.toMatch(/--require-auth/). Inline comment in the error block explains why. - Strict denier closure allocated per route call (allocation symmetry) →
auth.ts:276pre-buildsstrictDeniersingleton besidepassthrough; factory body reduces to a ternary.auth.test.ts:105asserts identity across multiplegate({strict: true})calls. bearerAuthvscreateMutationGatebody-shape drift (cross-PR) → Acknowledged as Wave 4 follow-up. Probe 5 above confirms the two shapes still differ ({error: 'Unauthorized'}vs{code: 'token_required'}); SDK normalization at the client boundary is the agreed near-term workaround.docs/users/qwen-serve.mdextra blank line → Fixed.require_authadvertised as pre-flight discovery in protocol doc — but/capabilitiesis itself behindbearerAuthwhen on →docs/developers/qwen-serve-protocol.md:19rewritten; the tag is now correctly described as a post-authentication confirmation, with the 401 response body called out as the actual discovery surface for unauthenticated callers.CONDITIONAL_SERVE_FEATURESwas a Set + per-featureif-branch (4-step coordination, silent fail-CLOSED on miss) →capabilities.ts:105-110converted toReadonlyMap<ServeFeature, (toggles) => boolean>; predicate lives next to the key, missing predicate is a TS error rather than a silent miss. 4-step process down to 2.- No test pins the conditional-feature contract →
server.test.ts:411honors every entry in CONDITIONAL_SERVE_FEATURESiterates the Map and throwsnew Error(...)for any unrecognized entry, forcing the contributor adding the next conditional tag to also add the assertion branch. Drift insurance as adoption-of-record. express.json()runs before strict gate → body parsed before refusal → Documented as a known trade-off inauth.ts:256-269(loopback-only surface, ~2.5GB transient worst case at 256 connections × 10MB, Wave 4 routes with small bodies); the comment also points at the future fix (lift strict-path gates to app-level via a Set increateServeApp).
Out-of-scope items the author explicitly deferred
Per the PR description, no Wave 1-2 mutation routes get flipped to { strict: true } yet — they thread mutate() as a centralization marker only. SDK-side DaemonClient adapter for the require_auth tag is deferred to a Wave 4 PR that has a consumer for it. Both are reasonable and consistent with "default off, bit-for-bit backward compatible."
CI
gh pr checks 4236 currently shows Classify PR: queued — CI started after force-push but the test matrix hasn't completed yet. Local 271-test run + 6-probe manual matrix gives me sufficient signal to approve now; the CI run should be a sanity check rather than a discovery vector.
LGTM.
Collaborator
Local validation reportVerified on 6149866 (post-force-push HEAD). Each row is the exact command and result. Unit / lint / typecheck
Real
|
This was referenced May 17, 2026
doudouOUC
added a commit
that referenced
this pull request
May 18, 2026
…4250) * refactor(serve): add FileSystemService boundary (#4175 PR 18) Introduce a per-request workspace filesystem boundary inside the `qwen serve` daemon. The boundary centralizes path canonicalization, symlink-aware boundary checks, ignore/trust policy, size/binary limits, and audit hooks behind a single typed surface — preparing PR 19 (read-only file routes) and PR 20 (write/edit routes) to share a guarded chokepoint instead of re-implementing path safety per route. Wave 4 PR 18 of #4175 — pure refactor, no new HTTP routes; depends on PR 12 (#4241) and PR 15 (#4236), both merged. New module under `packages/cli/src/serve/fs/`: - `paths.ts` extracts `canonicalizeWorkspace` from `httpAcpBridge.ts` (re-exported there for backward compatibility) and adds: - `ResolvedPath` brand and `Intent` union (read/write/edit/list/ glob/stat) with exhaustiveness checks at the trust gate - `hasSuspiciousPathPattern` — detects NTFS ADS, 8.3 short names, long-path prefixes, UNC paths, trailing dots, DOS device names, and three-or-more-dot path components (claude-code-style) - `findExistingAncestor` with explicit ENOTDIR rejection so a regular file in a path component throws `parse_error` rather than passing boundary inspection and 500-ing later - `resolveWithinWorkspace` running a chain-aware realpath check with ENOENT-tolerant ancestor walk for write/stat intents - `errors.ts` defines `FsError` / `FsErrorKind` plus `wrapAsFsError`, which categorizes raw `fs.promises` errnos (EACCES → permission_ denied, ELOOP → symlink_escape, ENOTDIR → parse_error, etc.) so body-level failures emit audit events instead of escaping uncategorized - `policy.ts` carries `MAX_READ_BYTES` (256 KiB), `MAX_WRITE_BYTES` (5 MiB), `BINARY_PROBE_BYTES` (4 KiB), `shouldIgnore` (file/ directory aware), and `assertTrustedForIntent` with an exhaustive switch over `Intent` - `audit.ts` emits typed `fs.access` / `fs.denied` `BridgeEvent` frames with SHA-256-hashed paths, optional raw-path passthrough via `QWEN_AUDIT_RAW_PATHS=1`, and discriminator `kind` fields so SDK consumers can exhaustively narrow `event.data` - `workspaceFileSystem.ts` — `WorkspaceFileSystem` interface + `createWorkspaceFileSystemFactory` with eight methods (resolve, stat, readText, readBytes, list, glob, writeText, edit). Every body method funnels failures through `recordAndWrap`, which wraps raw fs errors and always emits an `fs.denied` audit event before rethrowing. `readText` enforces `MAX_READ_BYTES` *before* delegating to the slurping core service so unbounded requests against multi-gigabyte files can no longer OOM the daemon. `glob` realpath-checks each hit against the canonical workspace and reports filtered escapes via a single aggregated `fs.denied` event with the dropped count - `index.ts` is the barrel re-export PR 19/20 will import from Modified files: - `packages/cli/src/serve/httpAcpBridge.ts` — extracted `canonicalizeWorkspace` to `fs/paths.ts`; the bridge re-exports it so existing callers in `server.ts` and `runQwenServe.ts` keep working - `packages/cli/src/serve/server.ts` — added `fsFactory?: WorkspaceFileSystemFactory` to `ServeAppDeps`; `createServeApp` builds a strict default (`trusted: false`, warn-once no-op `emit`) when none is injected so a future refactor that forgets `fsFactory` injection cannot silently allow writes against an untrusted workspace; factory parked on `app.locals` for PR 19/20 route handlers - `packages/core/src/index.ts` — re-exports `Ignore`, `loadIgnoreRules`, and `LoadIgnoreRulesOptions` from `utils/filesearch/ignore.js` for cli consumption 411 serve tests pass; typecheck clean. Engineering principles checklist: - [x] Independently mergeable (no new routes, no new capability tag) - [x] Backward compatible (no removed routes / event fields / CLI behavior) - [x] Default off (no public surface change; PR 19/20 will activate routes) - [x] qwen serve Stage 1 routes preserved - [x] Gradual migration (PR 19/20 will adopt the boundary) - [x] Reversible (single PR rollback) - [x] Tests-first (101 unit tests across the new module + contract test) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): address PR review feedback (#4250) Codex + Copilot review found 8 substantive issues against 7b0db4c; this commit fixes all of them. The first two are P1 build-breakers introduced by the pre-commit `eslint --fix` auto-promoting value imports to `import type` — 31 fs tests were failing post-commit until this fix. Issue list (links to PR comments at #4250): 1. Import type erased runtime values — workspaceFileSystem.ts:10-15. eslint's consistent-type-imports rule rewrote `import { Ignore, StandardFileSystemService, loadIgnoreRules, type WriteTextFileOptions }` -> `import type { ... }` because it saw type-only usage of `Ignore`. That erased `loadIgnoreRules` (called at runtime) and `StandardFileSystemService` (constructed at runtime), causing TS1361/TS2206 and runtime ReferenceErrors. Restored as a value import with inline `type` modifiers per-symbol and an eslint-disable line + comment so future autofixes don't repeat the regression. 2. Same import-type erasure in contract.test.ts:14 — `isFsError` was lumped under `import type` even though it's called at runtime. Same fix shape. 3. edit() OOM hole — workspaceFileSystem.ts. The earlier review pass added a pre-stat MAX_READ_BYTES gate to readText but missed edit, which fsp.readFile's the whole file before any size check. Multi-GB targets inside the workspace could OOM the daemon. Now stat-first; refuses above the cap with file_too_large; also rejects binary files (string indexOf over arbitrary bytes is meaningless). 4. glob accepted absolute / device patterns — workspaceFileSystem.ts. The ..-segment check stopped lexical traversal but /etc/** / C:\\Users\\foo\\** / \\\\server\\share\\** / //server/share/** still reached globAsync, walking outside the workspace before per-hit filtering dropped the results. Now rejects these patterns up-front with parse_error so no I/O happens outside. 5. glob ignore filter probed every hit as `file` — workspaceFileSystem.ts. The underlying `ignore` library needs a trailing-slash probe for dist/ / .git/-style directory patterns; probing as `file` silently leaked directory matches. Now lstats each hit and routes 'directory' vs 'file' to shouldIgnore so dir-only ignore rules actually match. 6. ReadTextOptions.line off-by-one — workspaceFileSystem.ts. The public option was documented as 1-based but forwarded as-is to readFileWithLineAndLimit, which is 0-based. A request with line: 1 returned content starting at the second line. Now converts 1-based -> 0-based at the boundary; doc clarified; truncation check uses the converted index. 7. ServeAppDeps.fsFactory JSDoc said trusted=true — server.ts:96. Stale from before the strict-default refactor in the same review pass. Rewrote to match the actual trusted: false + warn-once emit behavior. 8. MAX_READ_BYTES JSDoc said reads above cap return truncated — policy.ts:18. Stale from before the hard-cap refactor; now correctly states the cap throws file_too_large and that soft truncation only applies under the cap via enforceReadSize. 7 new tests cover the new behaviors: - POSIX-absolute pattern rejection - Win32 / UNC pattern rejection (4 variants) - directory-pattern ignore (dist/) - edit file-too-large - edit binary refusal - readText line: 1 returns from first line - readText line: 2 starts from second line 418/418 serve tests pass; typecheck + eslint clean. Deferred follow-ups (per PR review reply): - glob maxResults is applied after globAsync materializes every match. A streaming iterator (glob.iterate) would bound the walk too. Non-trivial; tracked as a separate hardening follow-up since current behavior is correctness-safe (just not optimal under huge trees). - Per-path glob escape hash in audit hint (currently aggregated count) — can revisit once PR 19 wires the routes and we see real audit volume. - EVENT_SCHEMA_VERSION migration mechanism — orthogonal; the whole BridgeEvent schema lacks one and that's a Wave 5+ concern. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): close 3 review-round-2 findings (#4250) The wenshao + DeepSeek reviewer pass on a81ada4 surfaced 3 more issues; this commit fixes them. 1. Dangling-symlink write escape (paths.ts) — Critical security bug. A request like `write /ws/escape` where `<ws>/escape` is a symlink whose target doesn't exist YET would pass the ENOENT-tolerant ancestor walk: realpath fails ENOENT, the walk-up returns `<ws>` as ancestor, the canonical becomes `<ws>/escape`, containment passes — but the eventual write follows the symlink and creates the file at the symlink target outside the workspace. lstat-then-readlink before the ancestor walk catches this; the symlink target is itself resolved via the deepest existing ancestor so macOS /private/var canonicalization stays consistent with boundCanonical (an absolute target inside the workspace tmpdir would otherwise have been false-flagged on macOS). 2. glob realpath catch over-reported symlink_escape (workspaceFileSystem.ts) — every realpath failure inside the per-hit boundary loop was counted as `symlink_escape`. EIO, EACCES, ENAMETOOLONG, EBUSY are environmental failures, not security events; mislabeling them poisoned the audit signal for operators trying to investigate genuine escape attempts. Now distinguished: ENOENT/ELOOP count as escapes; other errnos count as transient errors and emit a separate aggregated `fs.denied` with errorKind: 'permission_denied'. 3. policy.ts:enforceReadSize JSDoc said the boundary "intentionally does NOT throw" — stale after a81ada4's hard-cap refactor. Rewrote to clarify the helper is the soft truncation gate that only fires under the hard cap; readText itself enforces the hard cap with file_too_large via its pre-stat check. The readme/contract is now consistent with workspaceFileSystem.ts. 2 new tests: - dangling symlink targeting outside-workspace path → symlink_escape - dangling symlink targeting future-inside-workspace path → succeeds (ahead-of-mkdir flow for atomic-write-via-rename) 420/420 serve tests pass; typecheck clean. Remaining tracked follow-ups (per PR review reply): - list/glob brand cast (P2 deferred per PR description) - glob audit pathHash hashes pattern not paths - edit() TOCTOU read-modify-write race (atomic-via-temp + rename) - wrapAsFsError ENOSPC/EIO mapping to a distinct kind - runQwenServe → fsFactory injection integration test - glob maxResults streaming (glob.iterate) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): cross-platform ENOTDIR detection (#4250 CI fix) Windows CI test failure on a81ada4 surfaced a real cross-platform bug in `findExistingAncestor`. POSIX returns `ENOTDIR` when `fs.stat` traverses through a non-directory in a path component (e.g. `${ws}/file.txt/leaf` where `file.txt` is a regular file). **Windows returns `ENOENT` for the same case.** The errno-based guard added in a81ada4 only branched on `ENOTDIR`, so the Windows path silently fell through to the ancestor walk and the boundary returned a "canonical" the eventual write could not honor — `WorkspaceFileSystem - audit always emits on body errors > rejects ENOTDIR ancestor walk with parse_error rather than passing boundary` failed with `expected false to be true` on the windows-latest runner. Fix: switch from errno-based detection (platform-divergent) to dirent-kind detection. After `fs.stat` succeeds during the walk-up, if the existing ancestor is NOT a directory AND there are unresolved tail components, throw `parse_error`. Both `ENOENT` and `ENOTDIR` from `fs.stat` are now treated as "the *current* path doesn't resolve, keep walking" — the post-walk kind check fires regardless of which errno surfaced. Cross-platform-safe. The local 110/110 fs tests still pass on macOS/Linux; the Windows case will exercise the kind-check branch on next CI run. macOS CI failures on the same workflow run (`InputPrompt.test.tsx` placeholder reuse, `SettingsDialog.test.tsx` 5s timeout) are pre- existing flaky UI tests, NOT touched by this PR. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): close 4 review-round-3 findings (#4250) DeepSeek's third review pass (after b38e821) flagged four more issues; this commit fixes all of them. 1. Multi-hop dangling symlink bypass (paths.ts) — Critical security fix. The earlier single-readlink fix at efd7a46 was bypassed by chained dangling symlinks: <ws>/leak -> <ws>/middle -> /scratch/evil where every layer is a symlink and the final target doesn't exist. The fix only readlink'd the first hop (<ws>/middle), saw it was inside the workspace via findExistingAncestor, and let the chain through. The OS write at <ws>/leak would then follow both hops and create /scratch/evil. Now loops lstat + readlink up to MAX_ANCESTOR_HOPS, tracks visited inodes for cycle detection, and only validates containment on the fully-dereferenced leaf. Cycle detection rejects with symlink_escape; chains that exceed the hop bound also surface as symlink_escape with a "too long or contains a cycle" hint. 2. opts.line validation (workspaceFileSystem.ts) — the docstring committed to "1-based positive integer" but Infinity / floats / negative values flowed through to readFileWithLineAndLimit and degraded silently. Now enforces Number.isSafeInteger + line >= 1 at the boundary; everything else throws parse_error. Test covers Infinity, -Infinity, 0, -1, 1.5, NaN. 3. io_error FsErrorKind (errors.ts) — wrapAsFsError previously conflated EIO/EBUSY/ENAMETOOLONG/EMFILE/ENFILE/ENOSPC/ETXTBSY under permission_denied. Monitoring pipelines that key on errorKind for security alerting would page oncall on a full disk. New io_error kind (HTTP 503) maps the environmental- failure errnos with distinct hints. EACCES/EPERM stay on permission_denied (literal access denial); EIO (failing disk), EBUSY (busy file), ENAMETOOLONG (PATH_MAX), EMFILE/ENFILE (fd exhaustion), ENOSPC (df -h reporting 100%), ETXTBSY (text-busy) all route to io_error. 4. glob audit kind taxonomy (workspaceFileSystem.ts) — three-way classification mirrors wrapAsFsError so the per-hit realpath catch surfaces ENOENT/ELOOP -> symlink_escape, EACCES/EPERM -> permission_denied, everything else -> io_error. Each class emits its own aggregated fs.denied event. 5. edit() matchedIgnore (workspaceFileSystem.ts) — readText and writeText both stamp matchedIgnore in their access audit; edit didn't, so operators monitoring fs.access events couldn't distinguish edits to .gitignore'd files (build artifacts, logs) from edits to tracked source. Added the same shouldIgnore + matchedIgnore plumbing that readText uses. 8 new tests: - multi-hop dangling symlink (security) - symlink cycle (security) - ENOSPC/EIO/EBUSY/ETXTBSY/ENAMETOOLONG -> io_error mapping - io_error -> HTTP 503 - EMFILE/ENFILE updated to io_error (was permission_denied) - opts.line rejects Infinity/-Infinity/0/-1/1.5/NaN - edit() audit records matchedIgnore on .log file 426/426 serve tests pass; typecheck clean. Remaining tracked follow-ups (per PR review reply): - list/glob brand cast (P2 deferred per PR description) - glob audit pathHash hashes pattern not paths - edit() TOCTOU read-modify-write race (atomic-via-temp + rename) — pinned to PR 20 - runQwenServe → fsFactory injection integration test — pinned to PR 19 - glob maxResults streaming (glob.iterate) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): close 3 review-round-4 critical findings (#4250) DeepSeek's fourth review pass surfaced three more critical bugs; all are fixed in-PR. 1. TOCTOU symlink substitution in readText/readBytes/edit (workspaceFileSystem.ts) — Critical. fsp.stat(p) and the subsequent lowFs.readTextFile(p) (or fsp.readFile / fsp.readFile for edit) are two separate syscalls. An attacker who can write into the workspace can swap p from a regular file to a symlink pointing outside between the two calls; pre-stat sees the original, the read follows the swap. Fix: assertInodeStableAfterRead(p, preIno) — after each read, re-lstat p; reject with symlink_escape if the path is now a symlink (post.isSymbolicLink()) or its inode changed (preIno !== post.ino, with a 0-ino fallback for procfs / virtual mounts that don't report meaningful inodes). Catches the swap-and-leave attack and the swap-and-keep-swapped attack. Residual race (attacker swaps back AFTER our read but BEFORE our lstat) is much smaller than the original window and outside PR 18's threat model; fd-based reading via fsp.open + fileHandle.read would close it entirely but requires a new variant of lowFs that takes a FileHandle — tracked as a follow-up. 2. UTF-8 truncation corruption in readText (workspaceFileSystem.ts) — Critical. buf.subarray(0, sizeOutcome.bytesToRead).toString ('utf-8') silently emits U+FFFD when bytesToRead falls mid-codepoint (CJK, emoji). A downstream consumer parsing JSON or source code over the truncated content would see broken trailing bytes; meta.truncated would be true but the prefix is corrupt. A subsequent edit() with the corrupted string as oldText would also fail to match the on-disk content. Fix: safeUtf8Truncate(buf, maxBytes) walks back from maxBytes through any continuation bytes (0b10xxxxxx), then verifies the leading byte's full sequence fits within the cap; drops the leading byte if it doesn't. The result is always a clean prefix at a valid codepoint boundary. Test pins '中文测试' (12 bytes / 3 bytes per char) truncated at 7 bytes -> '中文' (no U+FFFD). 3. glob opts.cwd bypasses workspace boundary (workspaceFileSystem.ts) — Critical. opts.cwd was used directly as the glob root with no validation against boundWorkspace. ResolvedPath is a brand cast and a stale or forged value lets a glob('**/*', { cwd: '/etc' }) enumerate files outside the workspace. The pattern-side absolute / UNC checks added in a81ada4 only constrain the *pattern*; cwd is the actual hazard. Fix: at the entry point of glob(), path.resolve cwd and isWithinRoot-check against boundWorkspace. Throws path_outside_workspace if cwd is outside, even when the pattern itself is harmlessly relative. Test pins the case with cwd: scratch (outside workspace). 3 new tests: - readText with mid-operation symlink swap -> symlink_escape - safeUtf8Truncate keeps CJK codepoints intact at 7-byte cap - glob with opts.cwd outside workspace -> path_outside_workspace 429/429 serve tests pass; typecheck + eslint clean. Remaining tracked follow-ups (Post-PR-18 hardening, in #4175): - list/glob brand-cast contract (PR 19) - runQwenServe → fsFactory injection contract test (PR 19) - edit() write-side TOCTOU + atomic-via-temp + expectedHash (PR 20) - glob audit pathHash (independent audit.ts commit) - glob maxResults streaming (independent hardening) - glob pattern preflight refactor to reuse hasSuspiciousPathPattern (cosmetic) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): close 7 review-round-5 findings (#4250) DeepSeek round-5 surfaced 9 comments; 7 are real findings (the other 2 — UTF-8 truncation + glob opts.cwd — already fixed in 5468342 from round-4 and replied-to inline). Stack on round-4. 1. edit() empty-oldText silent-prepend (workspaceFileSystem.ts) — Critical silent data corruption. JS `''.indexOf('')` returns 0, so without an empty-string guard `current.slice(0,0) + newText + current.slice(0)` = `newText + current` — silently prepends `newText` to the whole file with a success audit event. PR 20 routes that thread user-supplied `oldText` verbatim must not be able to trigger this. Now throws `parse_error` BEFORE the read with a hint explaining why empty matches are rejected. 2. DOS device name regex misses bare names + first-extension forms (paths.ts) — Windows attack surface. The earlier /\.(CON|PRN|AUX|NUL|COM[1-9]|LPT[1-9])$/i only caught the last-extension form (file.CON). NTFS reserves these names regardless of extension: CON, NUL, CON.txt, NUL.dat, CON.foo.bar are ALL reserved device handles. New regex /(^|\.)(CON|...|LPT[1-9])(\.|$)/i covers all four positions (bare / first-ext / last-ext / middle-ext) while still admitting legitimate substrings (BACON, concat.txt, precon.go). 3. Buffer.byteLength replaces Buffer.from for size-only checks (workspaceFileSystem.ts) — 5 MB heap allocation per write eliminated. `writeText` and `edit()` previously materialized the entire UTF-8 payload (up to MAX_WRITE_BYTES) just to read `.length`; `Buffer.byteLength(content, 'utf-8')` returns the count without allocating. 4. edit() error message includes oldText snippet (workspaceFileSystem.ts) — Production debuggability. The earlier hint was just "edit() expects oldText to appear verbatim in the file" — at 3 AM an operator can't tell whether the mismatch is whitespace, a stale file, or a wrong target. Now includes a JSON-quoted truncated snippet (max 80 chars + ellipsis) of the searched-for text. 5. recordAndWrap forwards FsError message into fs.denied audit (workspaceFileSystem.ts + audit.ts) — Audit observability gap. Audit consumers debugging an incident saw `errorKind` + `hint` but lost the underlying OS error detail (path, errno text, byte count). FsDeniedAuditPayload now carries an optional `message` field; recordAndWrap forwards `fs.message` automatically. 6. EISDIR / ENOTDIR distinct hints (errors.ts) — UX. Both shared the same hint "a path component is not a directory where one was expected" — for EISDIR (path IS a directory but a file was expected) the wording was reversed. Now distinct hints with the errno name explicitly cited. 7. kindFromStats / kindFromDirent merged into kindFromStatLike (workspaceFileSystem.ts) — duplicate function bodies removed. Both fs.Stats and fs.Dirent expose the same isFile / isDirectory / isSymbolicLink interface, and both targets (FsStat['kind'], FsEntry['kind']) are the same 4-value union. Single helper avoids drift if the union grows. 4 new tests: - bare/multi-ext DOS device names (CON, NUL, CON.txt, CON.foo.bar) + legitimate substrings (BACON, concat, precon, contemplating) - edit() empty oldText -> parse_error + file unchanged - edit() not-found error includes searched snippet in hint - fs.denied audit payload carries FsError message Plus the 2 already-fixed items (UTF-8 boundary, glob opts.cwd) have new test coverage from round-4. 433/433 serve tests pass; typecheck + eslint clean. Stack: 7b0db4c → a81ada4 → efd7a46 → b38e821 → 911cb8e → 5468342 → THIS 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): close 7 review-round-6 findings (#4250) Round-6 review (wenshao + gpt-5.5) flagged 7 items including two Critical and one privacy regression I introduced in round-5. 1. writeText / edit pre-write symlink guard (workspaceFileSystem.ts) — Critical, two reviewers (wenshao #CsBcq + gpt-5.5 #CsB3M) independently flagged. `atomicWriteFile` (`packages/core/src/utils/atomicFileWrite.ts`) resolves symlinks at write time, so a swap between the boundary's `resolve()` and `lowFs.writeTextFile()` would let the write follow the symlink to outside the workspace. `writeText` had no read phase, so its window was wider than `edit()`'s. New helper `assertNotSymlinkBeforeWrite(p)` lstats the path immediately before each `lowFs.writeTextFile` call; ENOENT is fine (ahead-of-create flow), but an actual symlink throws `symlink_escape`. Used in `writeText` AND `edit()`. Residual race after this guard but before the write completes is the deferred PR 20 atomic-via-temp follow-up. 2. recordAndWrap message field bypassed privacy mode (audit.ts) — Critical privacy regression I introduced at ebd9e78 (round-5). The new `FsDeniedAuditPayload.message` field forwarded `FsError.message` unconditionally — and many throw-sites embed `${p}` (absolute paths) or user-supplied `oldText` snippets into the message. Privacy-mode operators (without `QWEN_AUDIT_RAW_PATHS=1`) saw paths leak through the message field even though they explicitly disabled raw-path logging. Fixed: `message` is now gated behind `includeRawPaths` alongside `relPath`. Privacy mode = no path-bearing fields, period. Operators wanting forensic context opt in via `QWEN_AUDIT_RAW_PATHS=1` and accept both fields together. 3. glob opts.cwd via symlink (workspaceFileSystem.ts) — gpt-5.5 #CsB3P. Textual `path.resolve(cwd) + isWithinRoot` admits `<ws>/link` even when `<ws>/link → /etc` is a symlink to outside; `globAsync` walks `/etc` before the per-hit filter drops results. Switched to `fsp.realpath(path.resolve(cwd))` so the containment check sees the actual walk root. ENOENT on cwd surfaces as `path_not_found`. 4. readText OOM via concurrent file growth (workspaceFileSystem.ts) — wenshao #CsBeE. The pre-stat `MAX_READ_BYTES` gate only sees the size at stat time; a concurrent writer can grow the file before the actual `readFileWithLineAndLimit` slurp. Added post-read `Buffer.byteLength(result.content) > MAX_READ_BYTES` check. The proper fix (fd-based read tying size + read to the same inode) is a hardening follow-up; this byte-length check is the defense-in-depth layer. 5. readBytes maxBytes can widen past MAX_READ_BYTES (policy.ts) — wenshao #CsBj5. `enforceReadBytesSize(st.size, opts.maxBytes)` used the caller-supplied `maxBytes` as the ceiling, replacing rather than clamping `MAX_READ_BYTES`. A future PR 19/20 route forwarding `req.query.maxBytes` could blindly bypass the daemon's 256 KiB safety cap. Now clamps via `Math.min(maxBytes, MAX_READ_BYTES)`. 6. ENOENT_TOLERATING_INTENTS docstring + test (paths.ts) — wenshao #CsBk3. The Intent docstring only mentioned `'write'` tolerating ENOENT; `'stat'` was in the set undocumented. A future maintainer removing `'stat'` thinking it was a copy-paste error would silently change behavior (stat on a concurrently-deleted path would throw `path_not_found` from the resolver instead of letting `fsp.lstat` throw `ENOENT` naturally). Amended docstring to call out `'stat'`'s rationale explicitly + added contract corpus case. 6 new tests: - glob cwd via symlink to outside → path_outside_workspace - writeText with mid-operation symlink swap → symlink_escape + outside file unchanged - edit with mid-operation symlink swap → symlink_escape + outside file unchanged - readBytes opts.maxBytes attempting widening → file_too_large - fs.denied message field absent in privacy mode (default) - fs.denied message field present in raw-paths mode (forensic) - contract corpus: resolve('newdir/leaf', 'stat') succeeds for ENOENT path 439/439 serve tests pass; typecheck + eslint clean. Stack: 7b0db4c → a81ada4 → efd7a46 → b38e821 → 911cb8e → 5468342 → ebd9e78 → THIS 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): close 5 review-round-7 findings (#4250) glm-5.1 round-7 review surfaced 6 items; 5 are real fixes, 1 was already addressed in 7f4f30d (writeText pre-write symlink guard — reviewer looked at ebd9e78 snapshot ~8h before the round-6 fix). Stack on round-6. 1. edit() bypassed lowFs.readTextFile (workspaceFileSystem.ts) — Critical encoding round-trip corruption. The earlier `fsp.readFile(p, 'utf-8')` included the UTF-8 BOM verbatim in `current`, breaking `oldText` matching even when the user passed the exact source text from a previous read; lost iconv-supported codepage handling (GBK / Big5 / Shift_JIS), so non-UTF-8 files would mojibake into `current` and round-trip-corrupt on write-back; and the subsequent `lowFs.writeTextFile` passed no `_meta`, stripping the BOM and forcing UTF-8 on write-back even when the original was BOM'd or non-UTF-8. Fix: read via `lowFs.readTextFile` AND forward `readResult._meta` into the write-back. Test pins UTF-8 BOM round-trip end-to-end. 2. hasSuspiciousPathPattern multi-digit and POSIX false positive (paths.ts) — `/~\\d/` only matched single-digit; NTFS allocates `~10+` on >9 collisions. POSIX false-positives: editor swap files, backup tools used `~N` legitimately. Fixed to `/~\\d+/` and gated behind `process.platform === 'win32'`, matching the ADS-colon check. 3. canonicalizeWorkspace redundant per-request syscall (paths.ts) — Performance. Factory canonicalizes once at build; every `resolveWithinWorkspace` also ran realpathSync on the same path, blocking the event loop. Added CANONICAL_BOUND_CACHE Map keyed on the input string; steady-state size = 1 per `1 daemon = 1 workspace`. 4. readBytes opts.maxBytes API contract (workspaceFileSystem.ts) — Semantic mismatch. Parameter name promised window-read; impl only used it as a hard reject gate. Now truncates the buffer post-read so `readBytes(p, { maxBytes: 1024 })` on a 200 KB file returns 1 KB. Hard `MAX_READ_BYTES` cap still throws for files above it. 5. glob walks node_modules and .git unnecessarily (workspaceFileSystem.ts) — Performance. Without an `ignore` option, `globAsync` traversed every file under those dirs before our per-hit `shouldIgnore` filter. Now passes `ignore: ['**/node_modules/**', '**/.git/**']` to short-circuit traversal. Post-filter via `shouldIgnore` remains authoritative. 5 new tests: - 8.3 short-name regex Windows / POSIX split - readBytes truncates 2048-byte file to 1024 with maxBytes - readBytes throws file_too_large only above hard cap - edit() preserves UTF-8 BOM round-trip - glob prunes node_modules and .git 442/442 serve tests pass; typecheck + eslint clean. Stack: 7b0db4c → a81ada4 → efd7a46 → b38e821 → 911cb8e → 5468342 → ebd9e78 → 7f4f30d → THIS 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): close 10 review-round-8/9 findings (#4250) Two more review passes (gpt-5.5 + DeepSeek + wenshao) flagged 13 items; 10 are real fixes, 3 are reviewer-stale-snapshot or already-tracked. Stack on round-7. Critical (5): 1. paths.ts symlink-escape hint embedded the symlink target (gpt-5.5) — Privacy regression sibling to round-6 audit `message` gate. `recordDenied` always forwards `hint` into `fs.denied` even with `QWEN_AUDIT_RAW_PATHS` off; the hint `'symlink points to /Users/alice/secret'` leaks the attacker's intended exfiltration path through audit. Hint is now path-free; operators wanting the resolved target enable `QWEN_AUDIT_RAW_PATHS=1` and read it from `relPath` / `message`. 2. paths.ts dangling-symlink chain discarded its verified canonical (DeepSeek) — After the multi-hop walk validated `cursor → canonicalTarget` was inside the workspace, the code fell through to `findExistingAncestor(absolute)`, re-walking from the original input and discarding the verified result. An attacker swapping an intermediate symlink between the verification and the re-walk could produce a different canonical than the one validated. The verified `canonicalTarget` is now captured in `symlinkResolvedCanonical` and used directly; the `findExistingAncestor(absolute)` fallthrough only runs when no symlink was traversed. 3. workspaceFileSystem.ts readBytes missing post-read size check (DeepSeek) — Same TOCTOU shape as `readText`'s round-6 fix. The pre-stat `enforceReadBytesSize` sees the size at stat time; a concurrent appender keeps the same inode but grows the file past the cap before `fsp.readFile` returns. `assertInodeStableAfterRead` catches inode changes but not same-inode growth. Added a post-read `buf.length > MAX_READ_BYTES` check matching `readText`'s defense-in-depth pattern. 4. errors.ts wrapAsFsError default = permission_denied (DeepSeek) — Misclassified non-errno errors (`TypeError`, programmer-error throws, native module exceptions) as security denials, paging security oncall for what should be a developer ticket. New `internal_error` kind (HTTP 500) is the new default; `permission_denied` reserved for actual `EACCES`/`EPERM`. 5. audit.ts AuditContext.sessionId not forwarded to BridgeEvent (DeepSeek) — Multi-session daemons couldn't trace audit events back to the session that triggered them. `originatorClientId` identifies the client, not the session. Added optional `sessionId` field to both `FsAccessAuditPayload` and `FsDeniedAuditPayload`, forwarded from `ctx.sessionId` when present. Improvements (4): 6. workspaceFileSystem.ts glob cwd realpath redundant when cwd === boundWorkspace (wenshao) — `boundWorkspace` is already canonicalized by the factory (`realpathSync.native` at build time), so calling `fsp.realpath` per-request when no `opts.cwd` was supplied is a redundant async syscall. Added a short-circuit. 7. workspaceFileSystem.ts kindFromStatLike JSDoc orphaned (wenshao) — Inserting `assertNotSymlinkBeforeWrite` between the JSDoc and `kindFromStatLike` left the doc floating above the wrong function. IDE hovers showed the wrong description. Moved the doc back to its function. 8. workspaceFileSystem.ts shared mutable Ignore object (DeepSeek) — `createWorkspaceFileSystemFactory` builds one `Ignore` instance and shares it across every `WorkspaceFileSystemImpl` returned by `forRequest()`. `Ignore.add(): this` is a public mutator. A future "per-session ignore rules" feature calling `.add()` from a request handler would silently corrupt all concurrent sessions. `Object.freeze` turns the cross-request mutation into a `TypeError` rather than a silent leak. 9. server.ts createDefaultFsAuditEmit one-shot warned (DeepSeek) — Permanent silent no-op after the first event; only logged the event `type` with no pathHash / errorKind / intent. If PR 19 forgets the real factory injection, every write 403s and audit is silent past the first warning — exactly the regression the warning exists to surface. Periodic warning (every 100th drop) + first-event context (errorKind, intent, pathHash) makes the regression actionable in production logs. Cleanup (1): 10. workspaceFileSystem.ts safeUtf8Truncate dead code (DeepSeek noted as "off-by-one") — The lead-byte seqLen-check block was dead code: `subarray(0, end)` already excludes the leading byte at `end`, so no further adjustment is ever needed. Removed the block; function is now 4 lines and still produces a valid codepoint prefix. Reviewer's suggested fix (`buf[end-1] → buf[end]`) was technically correct but redundant with the subarray cut. Already-fixed (3 reviewer-stale-snapshot, reply + resolve): - writeText pre-write symlink guard — fixed in 7f4f30d - edit() read-modify-write race — already deferred to PR 20 atomic-via-temp follow-up - glob maxResults walk-bound — already follow-up #5 3 new tests + 2 updated: - wrapAsFsError unknown errno → internal_error (default change) - internal_error has HTTP 500 - non-Error throwables → internal_error (not permission_denied) - readBytes post-stat growth → file_too_large - existing wrapAsFsError test updated for new default 445/445 serve tests pass; typecheck + eslint clean. Stack: 7b0db4c → a81ada4 → efd7a46 → b38e821 → 911cb8e → 5468342 → ebd9e78 → 7f4f30d → 1dc9d22 → THIS 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve/fs): close 3 review-round-10 findings (#4250) Round-10 review (wenshao) flagged 4 items, all marked non-blocking by the reviewer. 3 are landed in-PR; 1 (glob withFileTypes optimization) is moved to issue #4175 follow-ups since the reviewer themselves recommended "test it on a 100K-file workspace after PR 19 lands." 1. canonicalizeWorkspace docstring (paths.ts) — Documentation. The earlier FIXME warned about sync syscall blocking the event loop without mentioning the `CANONICAL_BOUND_CACHE` added in 1dc9d22 brings steady-state cost to zero. Future reviewers reading the FIXME again would re-flag the already-mitigated concern. Added a paragraph noting cache hit rate = 100% under the `1 daemon = 1 workspace` model so per-request blocking only happens at boot or on fresh workspace values (e.g. tests). 2. enforceReadBytesSize dead `maxBytes` parameter (policy.ts) — Round-7 changed `readBytes` to use post-read truncation for the soft window cap, leaving the function's `maxBytes` parameter unused at the only callsite. The `Math.min(maxBytes, MAX_READ_BYTES)` clamp branch became dead code. Reviewer's option 1: tighten the signature to `enforceReadBytesSize(fileBytes: number): void`. Done — the function is now purely the hard-cap enforcer its name implies; soft-window truncation lives in the orchestrator's `buf.subarray(0, opts.maxBytes)` post-read step where it's visible alongside the cap check it complements. 3. relForAudit cross-drive sentinel (audit.ts) — Windows-only privacy edge case. `path.relative('C:\\ws', 'D:\\evil')` returns `'D:\\evil'` (an absolute path) because Win32 can't express cross-drive relatives. Even when raw-paths mode is ENABLED, the audit `relPath` field would carry the off-drive absolute path, exposing the attacker's drive letter + directory. Added a `path.isAbsolute(rel)` post-check that substitutes a `<cross-drive>` sentinel — audit consumers see the cross-drive case distinctly without leaking the offending path. This was previously P2 deferred in PR 18's description ("Windows cross-drive path.relative"); reviewer's "few lines beats deferring" assessment was right. Tracked as follow-up (not in this commit): 4. glob withFileTypes optimization — Replace per-hit `lstat` (line ~664) with `glob` v10's `withFileTypes: true` so each hit comes back as a `Path` object with `isDirectory()` / `isFile()` / `isSymbolicLink()` already available. Saves N syscalls in large workspaces. Non-trivial restructure (return type changes from `string[]` to `Path[]`). Reviewer themselves marked "[performance / non-blocking]" and said "test it on a 100K-file workspace after PR 19 lands so we know whether it's worth it." Added to issue #4175 body's Post-PR-18 hardening follow-ups. 446/446 serve tests pass; typecheck + eslint clean. Stack: 7b0db4c → a81ada4 → efd7a46 → b38e821 → 911cb8e → 5468342 → ebd9e78 → 7f4f30d → 1dc9d22 → a33d459 → THIS 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
46 tasks
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: matt korwel <matt.korwel@gmail.com>
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
bearerAuthchecks per route, plus a--require-authdeployment knob that hardens the loopback developer default for shared dev hosts / CI runners / multi-tenant workstations.require_authcapability tag is absent unless the operator passed the flag.What's in this PR
createMutationGate({ tokenConfigured, requireAuth })helper inpackages/cli/src/serve/auth.ts. Single per-route choke point with the following 4-cell behavior matrix:requireAuth: truestrict: falsestrict: true401 token_required(1) global
bearerAuthalready 401'd unauthenticated requests; (2) globalbearerAuthenforcing.Wave 1-2 mutation routes (
POST /session,/session/:id/{load,resume,prompt,cancel,model},/permission/:requestId) opt in with the default non-strict factory call so the helper acts as a centralization marker —grep gateMutationfinds all mutation routes — without changing today's behavior. Wave 4 routes (PR 16-21) will pass{ strict: true }to require a token even on no-token loopback defaults.--require-authCLI flag +ServeOptions.requireAuth. Boot refuses without a token (boot-loud, like the existing non-loopback check); closes the/healthexemption when on so/healthalso requiresAuthorization: Bearer …; stderr breadcrumb (qwen serve: --require-auth enabled (bearer token mandatory on every route, including loopback /health).) so the hardened mode is visible in journald/docker logs without parsing/capabilities.Conditional
require_authcapability tag on/capabilities.features, advertised only when the operator opted in. Tag presence = behavior is on; absence = either an older daemon predating the tag OR a current daemon without--require-auth. Adds aCONDITIONAL_SERVE_FEATURESregistry primitive so future per-deployment toggles (e.g. eventualredact_errors) follow the same shape rather than each one growing its own ad-hoc emit branch.Out of scope (intentionally deferred)
strict: trueyet. The helper exists, existing routes stay non-strict for backward compatibility, Wave 4 PRs will flip the strict bit on routes that genuinely need token-required-on-loopback (memory writes, file edits, MCP restart, device-flow auth).Engineering principles checklist (Stage 1.5 wave PRs)
mutate()non-strict; capability tag conditional; no removed routes / event fields / CLI behavior)requireAuth: falsedefault;require_authtag absent unless flag set; existing mutation routes stay non-strict)qwen serveStage 1 routes / SDK behavior preserved (245/245 serve tests pass, including pre-PR capability + bearer auth assertions; only the historicalEXPECTED_STAGE1_FEATURES/Object.keys(SERVE_CAPABILITY_REGISTRY)test data was extended to cover the new registry entry)strict: trueindependently as they land)bearerAuth; no schema changes; no external state)server.test.tsfor capability +/health+ boot-check coverage)Validation
auth.test.ts, +5 inserver.test.ts).npx vitest run packages/cli/src/serve/auth.test.ts packages/cli/src/serve/server.test.tsthen inspect thecreateMutationGatematrix tests inauth.test.tsand the#4175 PR 15tagged tests inserver.test.ts.Scope / Risk
mutate()(default non-strict) as a centralization marker. This adds one no-op middleware per route on the hot path — measurable only as a function-call-and-return; the factory caches a single sharedpassthroughhandler when global auth is on so we don't allocate one closure per route call.DaemonClient/DaemonSessionClientadapter for therequire_authcapability tag (clients can detect the tag today viacaps.featuresarray indexOf, but a typed accessor would be cleaner — out of scope here, can land alongside Wave 4 routes that consume it).--require-authis opt-in. The conditionalrequire_authtag is additive; older clients indexing intocaps.featuresas aSet<string>see it harmlessly absent.Testing Matrix
Local macOS validation covered targeted typecheck, vitest (full
packages/cli/src/servesuite — 245/245), eslint. Windows / Linux left to CI.Linked Issues / Bugs
What's next after this lands
Wave 4 mutation routes (PR 16 memory + agents CRUD; PR 17 approval-mode + tools enable + MCP restart + workspace init; PR 18
FileSystemServiceboundary; PR 19 read-only file routes; PR 20 file write/edit (also depends on PR 8); PR 21 device-flow auth) all become unblocked. The "broad mutation routes share one gating helper rather than open-code auth per route" property of the Wave 4 batch is the deliverable PR 15 makes possible.🤖 Generated with Qwen Code