feat(serve): add workspace file write/edit routes (#4175 PR20)#4280
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Contributor
📋 Review SummaryThis PR implements workspace file mutation support for 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds workspace file mutation support to qwen serve (Wave 4 / Issue #4175 PR20), enabling clients to read bounded raw byte windows and safely write/edit workspace text files with content-hash concurrency checks and strict mutation auth, plus matching TypeScript SDK helpers and docs.
Changes:
- Add
GET /file/bytesfor bounded raw-byte reads (base64 payloads; optional full-file hash). - Add strict-auth
POST /file/write+POST /file/editwithexpectedHashchecks and atomic temp+rename writes. - Extend SDK types +
DaemonClientwith workspace file helper methods; update tests and protocol/docs/capabilities.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-typescript/test/unit/DaemonClient.test.ts | Adds unit tests for new workspace file helper methods and error propagation. |
| packages/sdk-typescript/src/index.ts | Re-exports new workspace file and hash-related SDK types. |
| packages/sdk-typescript/src/daemon/types.ts | Introduces SDK wire types for file read/bytes/write/edit and content hashes. |
| packages/sdk-typescript/src/daemon/index.ts | Re-exports the new daemon types from the daemon barrel. |
| packages/sdk-typescript/src/daemon/DaemonClient.ts | Adds readWorkspaceFile* and write/edit helper methods to the SDK client. |
| packages/cli/src/serve/server.ts | Registers the new workspace file write/edit routes in the serve app. |
| packages/cli/src/serve/server.test.ts | Updates capability expectations and filesystem stubs to cover new APIs. |
| packages/cli/src/serve/routes/workspaceFileWrite.ts | Implements POST /file/write and POST /file/edit with strict mutation gating and client-id validation. |
| packages/cli/src/serve/routes/workspaceFileWrite.test.ts | Adds route-level tests for strict auth, hash mismatches, symlink escape, and edit semantics. |
| packages/cli/src/serve/routes/workspaceFileRead.ts | Exports shared helpers and adds GET /file/bytes; includes hash in GET /file responses. |
| packages/cli/src/serve/routes/workspaceFileRead.test.ts | Adds coverage for /file hash and new /file/bytes behavior and validation. |
| packages/cli/src/serve/fs/workspaceFileSystem.ts | Adds hashing, bounded byte-window reads, atomic write/edit primitives, and per-path mutexing. |
| packages/cli/src/serve/fs/workspaceFileSystem.test.ts | Adds tests for hashes, bounded windows, atomic write semantics, and atomic edits. |
| packages/cli/src/serve/fs/index.ts | Exposes new fs types/guards (hash, byte window outcome, atomic write options). |
| packages/cli/src/serve/fs/errors.ts | Extends FS error taxonomy and maps new kinds to HTTP statuses (notably 409/422). |
| packages/cli/src/serve/fs/errors.test.ts | Extends tests for new kind→status mappings. |
| packages/cli/src/serve/capabilities.ts | Advertises new capability tags: workspace_file_bytes and workspace_file_write. |
| docs/users/qwen-serve.md | Documents new file routes and strict mutation/auth + hash expectations at a high level. |
| docs/developers/qwen-serve-protocol.md | Adds protocol-level specification for workspace file routes, errors, and capability tags. |
| docs/developers/examples/daemon-client-quickstart.md | Adds SDK usage examples for reading and editing workspace files with expectedHash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
approved these changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No new issues found beyond Copilot's existing comments. LGTM! ✅
Review Summary
Deterministic checks: tsc + eslint clean on changed files. All 382 tests pass (294 CLI serve + 88 SDK).
Key areas verified:
- Atomic write flow: temp file → write → hash precondition → rename → fsync parent — correctly handled
- Content hash validation: strict
^sha256:[0-9a-f]{64}$regex, properly enforced for replace mode and edit - Path traversal / symlink escape:
resolveWithinWorkspace+isSymbolicLink()+assertInodeStableAfterReadTOCTOU protection - Concurrency:
PathMutexRegistryserializes writes/edits to the same path - Auth:
mutate({ strict: true })on write/edit routes +bearerAuthmiddleware - Error handling: typed
FsErrorwith proper errno mapping viawrapAsFsError - Edit single-match:
countOccurrencescorrectly enforces exactly one match; empty oldText rejected at filesystem layer
Two Copilot-flagged type improvements (already open on types.ts:415 and types.ts:451) are valid suggestions — DaemonContentHash could be tightened and DaemonWorkspaceFileWriteRequest could use a discriminated union for expectedHash. Non-blocking; server-side enforcement is correct.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
requested changes
May 18, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
46 tasks
wenshao
requested changes
May 18, 2026
- readBytesWindow: re-stat the open fd after read and require unchanged ino+size+mtime before emitting the response. Mirrors the hardened text-snapshot path so the full-window hash can no longer pair with bytes that drifted under in-place rewrite or append. Surface drift as retryable hash_mismatch. - atomicWriteTextResolvedFile: reject a symlinked parent up-front as defense-in-depth ahead of the parent-fd publish follow-up referenced by assertInodeStableAfterRead. - atomicWriteTextResolvedFile: publish create-mode writes via link()+unlink() instead of rename(). POSIX rename() overwrites an existing regular file, so a racing external process could break the public create contract; link() returns EEXIST atomically and is portable across POSIX/NTFS. The early assertCreateTargetAbsent check stays for friendlier errors on the non-racing path.
Resolves the EXPECTED_STAGE1_FEATURES conflict in packages/cli/src/serve/server.test.ts: PR20 added `workspace_file_bytes` and `workspace_file_write`, PR21 added `auth_device_flow`. Match the registry source-of-truth ordering in capabilities.ts (PR19 → PR20 → PR21) so the bytes/write entries land before the device-flow entry, with the existing `require_auth` filter in EXPECTED_REGISTERED_FEATURES still relocating `auth_device_flow` to the conditional tail correctly.
wenshao
approved these changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No high-confidence issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
Collaborator
本地验证报告环境：Linux / Node 22，独立 自动化测试（按 PR 描述中的命令）
真实 daemon 端到端 smoke（启动
|
| 场景 | 期望 | 实际 |
|---|---|---|
/capabilities 新增 workspace_file_write / workspace_file_bytes |
出现在 features | ✅（32 个 feature 含新增两项） |
POST /file/write mode=create 新建 |
201 + hash | ✅ |
同路径再次 create |
409 file_already_exists |
✅ |
mode=replace 错误 expectedHash |
409 hash_mismatch |
✅ |
mode=replace 正确 expectedHash |
200 + 新 hash | ✅ |
POST /file/edit 单次匹配 |
200 + 新 hash | ✅ |
oldText=\"\" |
400 parse_error |
✅ |
path=\"../escape.txt\" |
400 path_outside_workspace |
✅ |
| 无 token 调用 | 401（strict gate） | ✅ |
写入指向 /etc/passwd 的 symlink |
400 symlink_escape |
✅ |
oldText 多重命中 |
422 ambiguous_text_match |
✅ |
GET /file/bytes |
200 + base64 + hash | ✅ |
残留 .tmp 文件 |
无 | ✅ |
实现层面要点
atomicWriteTextResolvedFile走 temp+O_EXCL（fsp.open(.., 'wx')）→fsync→ 发布；create模式用link()+unlink()关闭了 POSIXrename()会覆盖既有文件的竞态窗口（commitacfc5b66的改动）。replace路径在 per-path 锁内重新lstat→ 校验 hash →chmod→rename；通过dev/ino比对防 TOCTOU swap。- 父目录
lstat拒绝符号链接父级；temp 写完后再次 stat 校验未被换为 symlink。 readBytesWindow也补了ino+size+mtime稳定性检查，保证 full-window hash 与返回字节一致。- 写/编辑路由全部走
mutate({ strict: true })，loopback 无 token 调用同样 401。 oldText=''在路由层与 boundary 层双重防护，避免''.indexOf('')===0静默前置newText。
Known limitation（PR 注释中已显式说明）
parent-fd / openat-style 发布还未实现（Node stdlib 不直接暴露），父级 symlink 在 final rename 与 hash check 之间仍存在理论窗口。属于已知 follow-up，不阻断本 PR。
CI 在 macOS / Ubuntu / Windows + Node 22 全部通过；本地补充验证亦全部通过。LGTM。
wenshao
reviewed
May 18, 2026
| if (!deps.bridge.knownClientIds().has(clientId)) { | ||
| applyReadHeaders(res); | ||
| res.status(400).json({ | ||
| error: `Client id "${clientId}" is not registered for this workspace`, |
Collaborator
There was a problem hiding this comment.
[Suggestion] Error response shape inconsistency
resolveOriginatorClientId returns {error, code, clientId} while all other error paths in this file (sendParseError, sendFsError) use {errorKind, error, status}. SDK clients that branch on errorKind will miss invalid-client-id rejections, and monitoring systems alerting on errorKind won't fire for this path.
Suggested change
| error: `Client id "${clientId}" is not registered for this workspace`, | |
| applyReadHeaders(res); | |
| res.status(400).json({ | |
| errorKind: 'invalid_client_id', | |
| error: `Client id "${clientId}" is not registered for this workspace`, | |
| status: 400, | |
| }); |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| opts: ReadBytesOptions = {}, | ||
| ): Promise<Buffer> { | ||
| const out = await this.readBytesWindow(p, opts); | ||
| return out.buffer; |
Collaborator
There was a problem hiding this comment.
[Suggestion] Silent truncation — behavioral change from previous readBytes()
The rewritten readBytes() delegates to readBytesWindow() and discards the truncated/sizeBytes metadata. The old implementation threw file_too_large for files above MAX_READ_BYTES; the new one silently returns the first MAX_READ_BYTES bytes with no signal to the caller.
Any existing caller that relied on the exception to detect oversized files will now silently receive incomplete data. enforceReadBytesSize is still defined and exported but has become dead code.
Suggested fix: check out.truncated and throw file_too_large to preserve the old contract, or update the return type to surface truncation info:
Suggested change
| return out.buffer; | |
| async readBytes( | |
| p: ResolvedPath, | |
| opts: ReadBytesOptions = {}, | |
| ): Promise<Buffer> { | |
| const out = await this.readBytesWindow(p, opts); | |
| if (out.truncated) { | |
| throw new FsError('file_too_large', `file exceeds ${MAX_READ_BYTES} bytes`, { | |
| hint: 'use readBytesWindow() for explicit windowed access', | |
| }); | |
| } | |
| return out.buffer; | |
| } |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
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
qwen serve: bounded raw byte reads, hash-bearing text reads, strict-auth text write/edit routes, content-hash concurrency checks, and TypeScript SDK helpers.Validation
Scope / Risk
workspace_file_bytesandworkspace_file_write.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Refs #4175