Repository navigation
feat(serve): page large text files by byte cursor - #8002
Conversation
…s set
Follow-up to the bounded large-text read path. Three changes:
Gate on any explicit window argument, not on `limit`. Gating on `limit`
had the cost model backwards in both directions: `{ line: 900_000_000,
limit: 20 }` was admitted despite walking the whole file, while
`{ maxBytes: 4096 }` — satisfiable from the first 4 KiB — was refused. A
read with no window argument at all still fails, since a caller that
believes it holds the whole file may write it back truncated.
Add MAX_TEXT_SCAN_BYTES (8 MiB). MAX_READ_BYTES caps what a read
returns; nothing capped what it cost. Line offsets are resolved by
scanning from byte 0, so a query param could turn into an
uninterruptible multi-second scan of an arbitrarily large file — and on
Windows hold a read handle for that span, blocking renames and deletes.
Past the budget the read is refused with `file_too_large` pointing at
readBytes, which reaches any offset in O(1).
Tolerate appends on streamed windows. Requiring whole-file size/mtime
stability after reading a prefix rejected reads whose returned bytes
were still valid, and the case it rejected — tailing a live log — is the
one this path exists for. Streamed windows now assert inode identity
plus "did not shrink"; truncation and replacement are still rejected.
Also: non-UTF-8 large text now returns `binary_file` rather than
`file_too_large`, so a client retrying on 413 with a smaller window
can't loop forever; and `readFileWithLineAndLimit` throws instead of
silently ignoring a caller-supplied `fileHandle` on the by-path
fallback.
Co-Authored-By: Claude Opus 5 <[email protected]>
…lpers PR QwenLM#7947 pinned large-text reads to one inode by threading a caller-owned FileHandle into readTextRange as an optional field, plus a second field, forceStreaming, to suppress the buffering fast path. Two optional fields produced four combinations: one meaningful, one used by a single test, one unreachable, and — in readFileWithLineAndLimit — one that silently fell through to a by-path read, defeating the reason the caller opened a handle. Unify the two encoding detectors. detectFileEncoding now takes a path or a borrowed handle, so detectFileHandleEncoding is deleted along with the message discrepancy between them: an encoding iconv-lite cannot load now raises LargeNonUtf8TextError naming that encoding rather than deferring to the decoder's generic invalid-utf8 variant. Both still refuse the file, and the Serve boundary maps both to binary_file. Split the reader into readTextRange (path) and readTextRangeFromHandle (always streams, both byte bounds required). The unreachable combination and its untested readFileHandleBuffer are gone, and with no fileHandle parameter left for readFileWithLineAndLimit to ignore, the RangeError guarding that fallthrough is deleted too — the trap can no longer be expressed. CoreReadTextFileHandleRequest drops its required stats field. Nothing downstream read it, and because the ACP request type it extends permits extra properties, TypeScript accepted the dead argument silently. readFileHandleChunks becomes chunksFromHandle(fh, from) — the one seam byte-cursor text paging needs. No observable change at the Serve boundary: its 222 tests pass unmodified. Two fileSystemService tests were deleted rather than repaired; they asserted the arguments readFileWithLineAndLimit received, which is nothing once the handle path stops calling it. Their coverage lives in read-text-range.test.ts against real files and in workspace-file-system.test.ts at the real boundary. 258 production lines in core, net -71 overall. Co-Authored-By: Claude Opus 5 <[email protected]>
Self-audit follow-up to f55c867. Two fields survived the reshape that the handle path never reads: - `stats` was documented as required ("must pass the Stats captured from that handle") and nothing downstream read it. The handle path always streams, so it never needs a size to choose a strategy, and the encoding probe does its own fstat. - `path` became dead once readTextRangeFromHandle replaced the path-plus-handle call. Errors are labelled with the path by the Serve boundary that owns it. Neither was caught by the compiler: the ACP ReadTextFileRequest the type derived from permits extra properties, so the CLI kept passing both silently. That is the argument for declaring the type standalone rather than Omit-ing four of six inherited fields and quietly re-admitting the rest. Also record the second behaviour delta of the detector merge in the design doc: detectFileEncoding catches I/O errors and falls back to 'utf-8', where detectFileHandleEncoding let them propagate. The failure is not lost — a handle that fails the 8 KiB probe fails the streaming read immediately after — but a different call now reports it. Co-Authored-By: Claude Opus 5 <[email protected]>
Line offsets address a byte stream, so `readText` resolves them by scanning
from byte 0. Paging a large log that way is O(n^2) across pages, and past
MAX_TEXT_SCAN_BYTES (8 MiB) a deep page is refused outright — agents had no
O(1) path short of dropping to GET /file/bytes and splitting lines themselves,
losing encoding handling, multibyte safety, and the binary_file refusal.
A response that leaves content behind now returns `hasMore`, and where a file
byte offset is derivable, an opaque `nextCursor`. Passing it back as `cursor`
resumes in O(1). Page 1 is an ordinary `limit` read, so clients never compute
byte offsets themselves, and a paging loop does not break when a file happens
to be small.
The cursor is unsigned base64url JSON carrying {off, size, dev, ino}, matching
encodeOrganizedCursor rather than the HMAC-signed transcript codec: the path is
re-resolved through the workspace boundary on every request, so a forged cursor
can only move the offset within a file the caller may already read — what
GET /file/bytes?offset= allows today. What the payload is for is staleness:
a replaced or truncated file yields hash_mismatch instead of bytes from the
wrong place, while an append leaves an outstanding cursor valid — the case the
feature exists for.
Every minted cursor points at the start of a line. When a single line exceeds
maxOutputBytes the reader emits a truncated prefix and skips to the next line
rather than resuming mid-line, because a mid-line cursor makes the following
page snap forward and silently drop the rest of that line at the seam. Windows
cut mid-line by a byte cap therefore report hasMore with no cursor, as do
non-UTF-8 snapshot reads whose decoded text is a UTF-8 re-encoding with no
mapping back to file offsets. That is why hasMore is a field rather than a
restatement of nextCursor.
Cursor reads branch before the size check, not by widening the window gate:
a cursor read of a file under MAX_READ_BYTES would otherwise land on the
snapshot path, which knows only line/limit, and silently return line 0.
Adds the workspace_file_read_cursor capability, per the convention that new
behavior gets a new tag, and retargets the scan-budget hint at cursor paging.
Co-Authored-By: Claude Opus 5 <[email protected]>
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
The engage ack rode a pull_request:labeled round-trip: takeover-command applies the label, the labeled event routes, and the takeover-ack job posts the confirmation. That event has now been observed to simply not fire twice in one day (#7999 — the author read the silence as failure and removed the label; #8002 — an engaged fork PR with no ack for hours), and fork label events can never ack at all since they carry no secrets: a fork /takeover stayed silent until the next scan picked the PR up (2h41m on #7993). takeover-command now posts the engage ack directly after applying the label — every admission gate has already passed at that point, so 'engaged' is truthful for in-repo and fork PRs alike; the fork variant adds the expectation that the first round comes from the next scheduled scan. The route side suppresses the label-path ack when the label sender is the bot (only the ack: the immediate scan still routes), and the review-scan's existing first-pickup ack dedups against the command's comment and heals it if the post failed. Two more silent paths become audible while here: a /takeover on a stacked (non-main-base) PR now refuses out loud instead of dropping with only a log line, and a /takeover stop on a non-main PR now proceeds to remove the label instead of leaving it stuck.
Co-authored-by: Qwen-Coder <[email protected]>
|
Resolved the current review/triage items and refreshed the branch in
Final local verification on macOS 26.4.1 / Node.js 22.22.3:
The earlier automated Qwen review run timed out; I did not enqueue another long-running review because the repository requires Qwen Review to be requested explicitly by name. Maintainer sign-off is still required for the core-infrastructure scope. |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action takenTriaged the feedback newer than the last evaluation on PR #8002. Nothing this
No code was modified, so no verification commands were run and there are no 中文说明Autofix 审查轮次 —— 未采取任何行动已对 PR #8002 中自上次评估以来的新反馈进行分类。本轮没有任何需要修改代码的内容：
由于未修改任何代码，因此未运行任何验证命令，也没有需要解决或回复的行内讨论串。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
- Corrected the comment's cost model: idle candidates hit 'continue' before the TARGETS append, so they never contend for the 10-target cap — the real win is the shared inspection budget plus the serial scan-walk latency (the walk is what delayed #8002's pickup by ~6 minutes), and the comment now says exactly that. - Slot quantum changed from the hour to the scan tick (600s, the same quantum as ROT_OFF): an hourly slot against the */10 cron meant 6 back-to-back inspections then a ~3h blind window per PR — same 25% average, terrible shape. The gap is now bounded at ~30 minutes, which is what the operator-facing strings promise ('gap ≤30m'). - The two scan-only signals updatedAt cannot see (a base conflict appearing when main moves; still-red checks awaiting the redcheck marker) are named in the comment instead of papered over. - The per-candidate jq fork became a single precomputed set + a bash substring test, matching the busy skip's idiom and the 'free' claim. - Tests: the skip predicate and set builder got a behavioral replay (idle+out-of-slot defers, idle+in-slot inspects, fresh inspects, missing-from-lookup inspects); the two byte-distance assertions became a loop-head slice (comment growth cannot red-light CI, and budget-consuming code between the skips and the increment fails); the --json field pin is order-independent; the 3600 quantum is pinned OUT.
Co-authored-by: Qwen-Coder <[email protected]>
|
Addressed the latest review round in
Verification on
The cancelled duplicate label job on the previous SHA was not retried because this push starts a fresh CI cycle. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action needed this roundAll seven inline findings from the automated reviewer were already addressed in commit 80b296c (current HEAD), and @doudouOUC posted replies on each thread:
The only new review this round is the automated reviewer's No code changes are warranted. 中文说明本轮无需操作自动审查器提出的全部七条行内意见已在提交 80b296c（当前 HEAD）中处理，@doudouOUC 已在各线程回复：
本轮唯一的新审查是自动审查器的 无需代码变更。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Maintainer local verification — real daemon, base vs headI built both arms from source in isolated worktrees and drove two real
Automated suites — 1848 passed, 0 failed
Live end-to-end — 32 checks against the running daemon
All five items of your Reviewer Test Plan reproduce, plus adversarial probes. Highlights:
The performance claim holds
Per-page latency across 36 pages of the 1.6 MB log: cursor paging is flat (0.85× from first-8 to last-8 average) while line paging climbs (base 1.55×, head 1.80×). Full-file walk, median of 3: base·line 221 ms → head·cursor 140 ms. Base vs head differential — 140 requests per arm
102 identical, 38 different. The differences are overwhelmingly loosening: 21 requests that base refused with Findings1 —
Bisected threshold: line 90 552 still works, line 90 553 is refused — its byte offset is 8 388 604, i.e. exactly the new 8 MiB budget. Any existing client reading by I think the tightening itself is defensible — an unbounded scan behind a security boundary is a real cost, the 2 — the same applies to the 3 — a first page cut by the byte cap returns Whether the entry page mints a cursor depends on which cap binds first. On the 484 B/line JSONL fixture: The client has to discover experimentally that it must lower 4 — from a cold start, the tail after a line longer than the output cap is unreachable by paging. On the 420 KB single-line fixture, the entry page returns a valid 262 143 B UTF-8 prefix with Findings 3 and 4 are the same root cause ( VerdictCorrectness is solid — I found nothing that returns wrong bytes, loses content mid-walk, or lets a forged cursor escape the file. The TOCTOU discipline, the line-start invariant, the append-tolerant staleness check, and the UTF-8 handling all hold up under adversarial probing, and the O(1) claim is measurably true. My ask before merge is documentation, not code: correct the "Breaking changes: None" line to name the deep- 中文说明Maintainer 本地验证 —— 真实 daemon，base 与 head 对比我在隔离的 worktree 中分别从源码构建了两个分支，并启动两个真实的
自动化测试 —— 1848 项通过，0 项失败
真实端到端验证 —— 针对运行中的 daemon 执行 32 项检查你的 Reviewer 测试计划中的五项全部复现，另加对抗性探测。要点：
性能结论成立1.6 MB 日志共 36 页的单页延迟：游标分页保持平坦（首 8 页与末 8 页均值之比为 0.85×），而按行分页持续上升（base 1.55×、head 1.80×）。整文件遍历（3 次取中位数）：base·line 221 ms → head·cursor 140 ms。 base 与 head 差异对比 —— 每侧 140 个请求102 项相同，38 项不同。差异绝大多数是放宽：21 个此前被 base 以 发现的问题1 ——
二分定位阈值：第 90 552 行仍可读取，第 90 553 行被拒绝 —— 其字节偏移为 8 388 604，正好是新的 8 MiB 预算。任何既有客户端按 我认为收紧本身是合理的 —— 安全边界后的无界扫描确实有实际代价， 2 —— 大型非 UTF-8 / 二进制内容的 3 —— 被字节上限截断的首页会返回 入口页是否签发游标，取决于哪个上限先生效。在每行 484 B 的 JSONL 测试文件上： 客户端只能靠试探发现自己必须不断调小 4 —— 从冷启动开始，超过输出上限的单行之后的内容无法通过分页读到。 在 420 KB 单行测试文件上，入口页返回了合法的 262 143 B UTF-8 前缀， 第 3、4 点根因相同（ 结论正确性扎实 —— 我没有发现任何返回错误字节、分页途中丢失内容、或让伪造游标越出文件的问题。 TOCTOU 处理、行首不变量、容忍追加的陈旧性检查以及 UTF-8 处理在对抗性探测下都站得住，O(1) 的结论也确实可测量地成立。 合并前我的请求是文档层面的，而非代码：修正「Breaking changes: None」这一行，写明深层 |
Co-authored-by: Qwen-Coder <[email protected]>
|
@wenshao Addressed in
Thank you for the thorough real-daemon A/B verification. |
Review feedback update —
|
| Feedback | Decision | Action |
|---|---|---|
Deep line reads past 8 MiB and large binary/non-UTF-8 status changes were missing from breaking-change notes |
Agree | Updated the PR body in English and Chinese with the 413 and 413 → 422 compatibility impact and migration guidance. |
Byte-capped and oversized-line entry pages can return hasMore: true with nextCursor: null without bootstrap guidance |
Agree | Updated the protocol docs with safe recovery paths: lower limit, advance via explicit line, or use /file/bytes for the full oversized line. |
Validation: Prettier check and repository pre-commit hook passed. Review threads resolved: 0/0 (the feedback was a top-level PR comment, not an inline review thread).
Co-authored-by: Qwen-Coder <[email protected]>
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Review feedback — no code change requiredAll four findings from @wenshao's maintainer verification were already addressed in commit
Failed check: Real daemon E2E / Java 11This check runs Investigation:
Without CI log access or a Java toolchain, the root cause of the Java 11 failure cannot be determined from this environment. The evidence above shows the PR's changed code paths are not exercised by that test. Verification
中文说明审查反馈 — 无需代码变更@wenshao 的 maintainer 验证中提出的四项发现均已在提交
失败检查：Real daemon E2E / Java 11该检查运行 调查：
由于无法访问 CI 日志且缺少 Java 工具链，无法从当前环境确定 Java 11 失败的根本原因。以上证据表明该 PR 变更的代码路径未被该测试执行。 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
CI fix update —
|
| Failure | Classification | Fix |
|---|---|---|
Java daemon E2E stopped during prepare: merged browser daemon SDK bundle was 181,252 bytes, 4 bytes above the 177 KiB budget |
Branch-related, deterministic merge-integration failure; not a flake | Raised the established bundle budget by 1 KiB to 178 KiB, leaving 1,020 bytes of headroom for the current merged API surface. |
Verification:
- Exact commit: full
npm run buildandnpm run typecheckpassed. - Independent latest-main synthesis: no merge conflict; SDK build passed with 181,252 / 182,272 bytes.
- PR body evidence updated in English and Chinese.
Flaky retries used: 0. Review threads resolved: 0/0 (this was a CI failure, not review feedback).
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action required this roundThere is no actionable review feedback on PR #8002:
No code changes were made. 中文说明本轮无需操作PR #8002 当前没有需要处理的评审反馈：
本轮未做任何代码变更。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed (PR #8002)Both findings from the automated reviewer were Suggestion-level. Both are addressed with focused test coverage that mirrors existing patterns; no production code changed. [rc:3677562635] ACP dispatch cursor validation has no dedicated test coverage — RESOLVEDDecision: Implement. The ACP Changes (
[rc:3677562651] Cursor path
|
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Round summaryAddressed both inline findings from the automated review. Feedback points
Conflict notesNo conflicts ( Verification
中文说明轮次总结处理了自动审查中的两个行内发现。 反馈要点
冲突说明无冲突（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
…#8049) Round-2 verification showed the scheduled scan lands every ~40-70 min on this repo, not every 10 min, so the (epoch/600)%4 slot is an independent ~25% draw per scan, not a deterministic 1-in-4 rotation. The gap is geometric (measured median ~2h, p90 ~6h), not bounded at ~40m. Reword the operator-facing strings (skip echo, fleet row) and the workflow comment to state the behaviour probabilistically and drop the false "bounded" / "no PR is unlucky forever" claims; correct the misattributed #8002 latency (queue/startup, not the serial walk). Relax the tests so a truthful number is not a CI failure: pin the mod-4 time-quantum shape instead of the exact 600s constant, and drop the /3600 exclusion that forbade the better-tailed quantum. Mechanism logic is unchanged.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action takenBoth inline suggestions on this PR have already been explicitly deferred by maintainer @doudouOUC with recorded reasoning on their respective threads:
Both deferrals are consistent with the repository's review policy: after roughly five review rounds, only Critical correctness, security, data-loss, or regression fixes are landed, and remaining Suggestions are deferred to follow-up work. No failed checks, no still-red checks, no conflicts, and no 中文说明未采取任何操作本 PR 上的两条行内建议均已由维护者 @doudouOUC 在各自的主题中明确记录了推迟理由：
两项推迟均符合仓库的审查策略：在大约五轮审查之后，仅接受 Critical 级别的正确性、安全性、数据丢失或回归修复，其余建议推迟到后续工作中处理。 当前没有失败的检查、没有持续失败的检查、没有冲突，也没有 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Maintainer re-verification (round 2) — head
|
| # | Round-1 ask | Status |
|---|---|---|
| 1 | MAX_TEXT_SCAN_BYTES breaking change undeclared |
Addressed. Body now names the deep-line 413 explicitly. Behaviour re-measured unchanged: base 200 → head 413, boundary at byte 8 388 605 (≈ 8 MiB, measured at limit=1; the exact line depends on limit because the scan must also cover the requested lines). Region still reachable by cursors — 65 requests, 377 ms. |
| 2 | 413 → 422 reclassification undeclared |
Addressed, same body line. Re-measured: 16 of 154 matrix requests move 413 file_too_large → 422 binary_file. |
| 3 | byte-cap first page: hasMore:true + nextCursor:null, no documented recovery |
Addressed by 524ef96a74. I executed the advice: halving limit from 2000 mints a cursor at 500 after 2 probes, then walks to EOF in 6 pages. |
| 4 | tail after an oversized line unreachable from a cold start | Addressed by 524ef96a74, and both documented routes work: line=2 + cursors recovers all 50 tail lines; GET /file/bytes serves the 140 KB line (64 KiB per call, with truncated: true so a client can page it). |
aa32ed13a6 is a real fix, and it is load-bearing
This commit moves one line: sawCrlf = true used to run at the top of emit(), before the byte-budget early return — so a CRLF line deferred to the next page still flipped the current page's lineEnding. I imported the pre-fix and post-fix modules into one process (the only variable is that line's position) and ran nine real files through real FileHandles:
- pre-fix wrong on 2 fixtures,
aa32ed13a6wrong on 0, with the other 7 fixtures agreeing across both arms — including pure-CRLF and "emitted CRLF + deferred LF", so the fix does not weaken genuine CRLF detection. contentandnextOffsetare identical on all 9 — this is a metadata-only fix, no content risk.- Confirmed on the wire too: a page capped at 9 199 B over a mixed LF/CRLF file returns
lineEnding: lf, and the CRLF block at offset 9 200 still returnscrlf.
This is worth flagging as more than cosmetic: reads report lineEnding, and writeTextFile accepts a client-supplied lineEnding — so a client that paged, edited, and wrote back could have converted an LF region to CRLF.
The added test does defend it. A 3-cell matrix over the real vitest runner: the previous head's own suite passes 40/40 over the buggy code (a genuine escape), the new test set fails exactly one case over that same buggy code — does not let a budget-excluded CRLF line flip lineEnding, the assertion the commit adds — and 41/41 pass at the real head.
Automated suites — 1854 passed, 0 failed
| Suite | Result |
|---|---|
packages/core — read-text-range (41) + fileSystemService (68) |
109 passed |
packages/cli — workspace-file-system (118), workspace-file-read (40), acp-http/transport (290), bridge-file-system-adapter (21), server (836) |
1305 passed |
packages/sdk-typescript — DaemonClient (302), acpRouteTable (103) |
405 passed |
integration-tests/cli/qwen-serve-routes (real spawned daemon) |
35 passed |
That is +6 versus round 1's 1848, which is exactly the six tests the delta adds (core +1, workspace-file-system +1, transport +4). CI on the PR is also clean — 25 success, 43 skipped, 0 failing.
Base vs head differential — 154 requests per arm
I widened the matrix for this round: added a mixed LF/CRLF fixture and started comparing lineEnding as well as status, errorKind and content, so a line-ending regression on the pre-existing line path would surface rather than be assumed absent.
116 identical, 38 different — the same three classes and the same counts as round 1 (21 loosening, 16 reclassification, 1 tightening). The raw run flags 22 lineEnding differences, but every one of them is just the base arm returning 413 with no lineEnding field at all; restricted to requests where both arms answered 200, there are 0 lineEnding differences. And the cursor walk still reproduces the line walk byte for byte on all six text fixtures.
I also spot-checked the 07-workspace-filesystem.md branch reorder in aa32ed13a6: it is accurate — the cursor branch really is evaluated before the ≤ 256 KiB snapshot path (dispatch at workspace-file-system.ts:1435), and a cursor on an 18 KB file is honoured rather than falling into the snapshot reader.
One new finding — minor, and the only thing I'd change
5b514150cb raises the SDK browser-bundle budget from 177 KB to 178 KB, but the gated artifact does not need it.
The gate measures packages/sdk-typescript/dist/daemon/index.js (build.js:183). Built at this head:
measured 180 396 B = 176.17 KB
budget 177 KB 181 248 B → fits, 852 B of headroom
this PR's delta 101 B (base cc617e6707 = 180 295 B)
I reverted only that constant to 177 * 1024 and re-ran the build: the gate passes. As a positive control I clamped it to 170 KB and the gate does fire (Browser daemon SDK bundle is 180396 bytes; expected <= 174080), so it is not silently broken. Both arms bundled with the lockfile-pinned [email protected] (resolved from packages/sdk-typescript/node_modules, not the root 0.25.6), so this is not a toolchain-drift artifact.
The comment attributes the bump to "workspace file byte-cursor paging after merging the workspace pairing approval SDK surface", which suggests it was needed at some intermediate state of the branch — but at the current head this PR only adds 101 bytes and there is still 852 bytes of room. Since main also just moved this constant in #8024, I'd rather not loosen a deliberate guard that isn't binding: please drop 5b514150cb and keep the budget at 177 KB. If CI disagrees on a different runner I'm happy to be shown otherwise — that would be worth knowing, since it would mean the artifact is platform-sensitive.
Verdict
All four of my round-1 asks are satisfied, and the one new code commit is a genuine bug fix that I confirmed is load-bearing, correctly scoped, and properly tested. Correctness at this head is where it was in round 1 — nothing returns wrong bytes, loses content mid-walk, or lets a forged cursor escape the file — and the differential against the merge-base is still overwhelmingly loosening, with the one tightening case now disclosed in the body.
This is good to merge from my side. The bundle-budget bump is the only outstanding item and it is a one-line revert, not a blocker — land it either way, but I'd prefer the guard stay at 177 KB.
中文说明
Maintainer 复验（第 2 轮）—— head aa32ed13a6
这是对我第 1 轮报告（head 80b296cbbc）的后续。此后新增了四个提交：524ef96a74 文档 · 5b514150cb SDK 体积预算 · 2fafb448e0 测试 · aa32ed13a6 sawCrlf 修复。merge-base 仍为 cc617e6707，因此 base 侧未变，第 1 轮的所有数据仍可直接对比。
我从源码重新构建了 head 侧，并针对两个真实的 qwen serve daemon（同一个工作区）重跑了全部验证 —— head aa32ed13a6 → :8802，base cc617e6707 → :8803。已在协议层重新确认 A/B 隔离：GET /capabilities 在 head 返回 workspace_file_read_cursor、base 不返回，且两侧 dist/ 均未引用对方 worktree。环境：macOS 15.7.7（Darwin 24.6.0）、Node 22.23.1。
第 1 轮每一项发现，均在本 head 重新测试
四项发现全部已处理，在本 head 上均不再成立。 四项原本都是文档层面的诉求，因此我不仅重跑了底层行为，还实际执行了新写入文档的绕过方案，而不是只读一遍：
| # | 第 1 轮诉求 | 状态 |
|---|---|---|
| 1 | MAX_TEXT_SCAN_BYTES 破坏性变更未声明 |
已处理。 PR 描述现已明确写出深层 line 的 413。行为实测未变：base 200 → head 413，边界在字节 8 388 605（≈ 8 MiB，在 limit=1 下测得；具体行号取决于 limit，因为扫描还需覆盖所请求的行）。该区域仍可通过游标到达 —— 65 次请求、377 ms。 |
| 2 | 413 → 422 重新归类未声明 |
已处理，同一行描述。实测：154 个矩阵请求中有 16 个从 413 file_too_large 变为 422 binary_file。 |
| 3 | 被字节上限截断的首页返回 hasMore:true + nextCursor:null，且无文档说明如何继续 |
已由 524ef96a74 处理。 我实际执行了该建议：从 2000 开始折半，limit=500 时（第 2 次探测）签发出游标，随后 6 页走到 EOF。 |
| 4 | 冷启动时超长行之后的内容无法读到 | 已由 524ef96a74 处理，且文档给出的两条路径都可用：line=2 加游标可恢复全部 50 行尾部内容；GET /file/bytes 能取到这个 140 KB 的行（每次调用 64 KiB，并带 truncated: true 供客户端继续分页）。 |
aa32ed13a6 是真实修复，且确实起作用
该提交只移动了一行：sawCrlf = true 原先位于 emit() 开头、在字节预算的提前返回之前 —— 因此一个被推迟到下一页的 CRLF 行仍会改变当前页的 lineEnding。我把修复前与修复后的两个模块导入同一个进程（唯一变量就是这一行的位置），用九个真实文件经真实 FileHandle 运行：
- 修复前有 2 个用例结论错误，
aa32ed13a6为 0 个，同时 7 个对照用例结论一致 —— 其中包括纯 CRLF 以及「已输出 CRLF + 被推迟的 LF」，说明修复并未削弱真正的 CRLF 识别能力。 - 九个用例的
content与nextOffset完全一致 —— 这是仅涉及元数据的修复，不存在内容风险。 - 协议层同样确认：在混合 LF/CRLF 文件上以 9 199 B 为上限的页返回
lineEnding: lf，而偏移 9 200 处的 CRLF 区块仍返回crlf。
这一点值得强调，它并不只是「显示问题」：读取会返回 lineEnding，而 writeTextFile 接受客户端传入的 lineEnding —— 因此一个「分页读取 → 编辑 → 写回」的客户端本可能把 LF 区域转换成 CRLF。
新增的测试确实守住了这个修复。 在真实 vitest runner 上做了三格矩阵：上一个 head 自带的测试套件在有 bug 的代码上通过 40/40（说明这是一次真实的逃逸），而新测试集在同样有 bug 的代码上恰好失败一项 —— does not let a budget-excluded CRLF line flip lineEnding，正是该提交新增的断言 —— 在真实 head 上则 41/41 全部通过。
自动化测试 —— 1854 项通过，0 项失败
| 测试套件 | 结果 |
|---|---|
packages/core —— read-text-range（41）+ fileSystemService（68） |
109 通过 |
packages/cli —— workspace-file-system（118）、workspace-file-read（40）、acp-http/transport（290）、bridge-file-system-adapter（21）、server（836） |
1305 通过 |
packages/sdk-typescript —— DaemonClient（302）、acpRouteTable（103） |
405 通过 |
integration-tests/cli/qwen-serve-routes（真实启动 daemon） |
35 通过 |
相比第 1 轮的 1848 项增加了 6 项，正好等于本次增量新增的六个测试（core +1、workspace-file-system +1、transport +4）。PR 上的 CI 同样是干净的 —— 25 项成功、43 项跳过、0 项失败。
base 与 head 差异对比 —— 每侧 154 个请求
本轮我扩大了矩阵：新增一个混合 LF/CRLF 的测试文件，并且除状态码、errorKind、内容之外，也开始比较 lineEnding，这样既有 line 路径上的行尾回归就会被暴露出来，而不是靠假设它不存在。
116 项相同、38 项不同 —— 与第 1 轮完全相同的三个类别、相同的数量（21 项放宽、16 项重新归类、1 项收紧）。原始输出标记了 22 处 lineEnding 差异，但其中每一处都只是 base 侧返回 413、根本没有 lineEnding 字段；如果只看两侧都返回 200 的请求，lineEnding 差异为 0。此外，游标遍历在全部六个文本测试文件上仍与 line 遍历逐字节一致。
我也抽查了 aa32ed13a6 中对 07-workspace-filesystem.md 分支顺序的调整：结论是准确的 —— 游标分支确实在 ≤ 256 KiB 快照路径之前判定（分发点在 workspace-file-system.ts:1435），并且对一个 18 KB 文件使用游标会走游标读取器，而不会落入快照读取器。
一处新发现 —— 轻微，也是我唯一希望改动的地方
5b514150cb 把 SDK 浏览器包体积预算从 177 KB 提到 178 KB，但被检查的产物并不需要这次上调。
该检查测量的是 packages/sdk-typescript/dist/daemon/index.js（build.js:183）。在本 head 构建后：
实测 180 396 B = 176.17 KB
177 KB 预算 181 248 B → 通过，余量 852 B
本 PR 自身增量 101 B （base cc617e6707 = 180 295 B）
我只把该常量改回 177 * 1024 并重新构建：检查通过。 作为正向对照，我把它压到 170 KB，检查确实会报错（Browser daemon SDK bundle is 180396 bytes; expected <= 174080），说明这个门禁并没有静默失效。两侧构建都使用了 lockfile 锁定的 [email protected]（从 packages/sdk-typescript/node_modules 解析，而非根目录的 0.25.6），因此这不是工具链版本漂移造成的假象。
注释把此次上调归因于「workspace file byte-cursor paging after merging the workspace pairing approval SDK surface」，这暗示它在分支的某个中间状态下确有必要 —— 但在当前 head 上，本 PR 只增加了 101 字节，仍有 852 字节余量。考虑到 main 也刚在 #8024 中调整过这个常量，我不倾向于放宽一个并未真正触及的门禁：建议撤掉 5b514150cb，把预算保持在 177 KB。 如果 CI 在其他 runner 上结论不同，我很乐意被纠正 —— 那反而是值得知道的信息，因为那意味着该产物对平台敏感。
结论
第 1 轮我提出的四项诉求全部得到满足，唯一的新增代码提交是一个真实的缺陷修复，我已确认它确实起作用、范围恰当、并且有测试守护。 本 head 的正确性与第 1 轮一致 —— 没有发现返回错误字节、分页途中丢失内容、或让伪造游标越出文件的问题；与 merge-base 的差异依然以放宽为主，而唯一收紧的那一项现已在 PR 描述中声明。
从我这边看可以合入。 体积预算的上调是唯一遗留项，且只是一行回退，并非阻塞项 —— 无论如何都可以合并，但我更希望这个门禁保持在 177 KB。
|
Thanks for the thorough re-verification. I am keeping the 178 KiB budget. The 180,396-byte head-only build is reproducible here, but the failure that prompted 5b51415 was on GitHub's required merge ref, not the head-only tree. SDK Java run 30473276861 checked out c4806b7, which merged 524ef96 into the then-current base 97aaa38, and its clean Ubuntu/Node 22.23.1 prepare build produced 181,252 bytes against the 181,248-byte limit. That exact four-byte overflow is recorded in job 90648562382. The 856-byte difference from 180,396 matches the discrepancy under discussion: the merge ref included newer base SDK surface that the head-vs-merge-base build did not. Because required CI validates PR integration with the current base, reverting to 177 KiB would risk restoring a demonstrated deterministic merge-ref failure. This is also explicitly non-blocking and the PR is beyond roughly five review rounds, so no further churn is warranted. |
|
@qwen-code /triage |
…wenLM#8043) * fix(autofix): post the takeover engage ack from the command itself The engage ack rode a pull_request:labeled round-trip: takeover-command applies the label, the labeled event routes, and the takeover-ack job posts the confirmation. That event has now been observed to simply not fire twice in one day (QwenLM#7999 — the author read the silence as failure and removed the label; QwenLM#8002 — an engaged fork PR with no ack for hours), and fork label events can never ack at all since they carry no secrets: a fork /takeover stayed silent until the next scan picked the PR up (2h41m on QwenLM#7993). takeover-command now posts the engage ack directly after applying the label — every admission gate has already passed at that point, so 'engaged' is truthful for in-repo and fork PRs alike; the fork variant adds the expectation that the first round comes from the next scheduled scan. The route side suppresses the label-path ack when the label sender is the bot (only the ack: the immediate scan still routes), and the review-scan's existing first-pickup ack dedups against the command's comment and heals it if the post failed. Two more silent paths become audible while here: a /takeover on a stacked (non-main-base) PR now refuses out loud instead of dropping with only a log line, and a /takeover stop on a non-main PR now proceeds to remove the label instead of leaving it stuck. * fix(autofix): ack command-driven releases directly and key the scan grace on the label actor Review follow-up: the engage-side fix left the release side on the fragile round-trip — a loud add next to a mute stop re-creates the exact 'did it work or did the event get lost?' ambiguity this PR set out to remove, now on release. And a fork or non-main release could never ack at all (fork unlabeled events carry no secrets; the route ignores non-main releases). - takeover-command now posts the release ack directly after removing the label, mirroring the engage side, with the same three variants as the ack job (plain release / bot-authored / bot-authored+skip) chosen from the same PR_INFO the gates used. The route suppresses the unlabeled-path ack when the label sender is the bot. - The scan's first-pickup grace is keyed on the label event's actor: a bot-applied label defers only 45s (the command's own write is seconds behind the label — fork or in-repo alike), so the immediate scan still heals a failed command ack instead of slipping to the next scheduled tick, and an ic.json snapshot taken between the label write and the command ack cannot double-post. A human in-repo label keeps the 3-minute ack-job grace; a human fork still posts right away. - The fork note now says 'usually within minutes', matching the */10 cron instead of contradicting it by 6x. - Tests kill the two surviving mutations from review: the EN/ZH fork-note swap (per-language-half assertions) and the warning fallback downgraded to '|| true' (both fallback strings pinned). * test(autofix): behaviorally pin the LAST_LABELED_BY jq extraction (QwenLM#8043) * fix(autofix): correct stale comments about scan heal timing and command blast radius (QwenLM#8043) * test(autofix): pin release-ack body identity across the command and ack jobs (QwenLM#8043) --------- Co-authored-by: verify <verify@local> Co-authored-by: Qwen Code Autofix <[email protected]>
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review triage (round 3) — no code changeHead Finding triaged@wenshao — revert Disposition: declined (no change), agreeing with the PR author's recorded
No other actionable items: @wenshao's four round-1 asks are confirmed 中文说明Autofix 审查分诊（第 3 轮）—— 无代码变更Head 已分诊的发现@wenshao —— 撤掉 处置：拒绝（不变更），与 PR 作者已记录的决定一致。理由：
没有其他可操作项：@wenshao 第 1 轮的四项诉求已确认全部满足， Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
* feat(autofix): back off scan inspection of idle candidates The scheduled scan inspects every candidate every tick. The takeover pool doubled in two days (28 open takeover PRs, 8 of them idle in 'nothing new' state for 10+ hours), and idle candidates crowd the two SHARED budgets: MAX_CANDIDATE_INSPECTIONS (60) and the 10-target cap. Observed on QwenLM#8002: freshly engaged, admitted by the 09:03 scan, then deferred by the target budget while long-idle PRs re-confirmed their idleness yet again. Candidates whose list-provided updatedAt (no extra API call) is older than 24h are now inspected on roughly every 4th scan, on a deterministic slot keyed by PR number and UTC hour so no PR waits forever. The skip is free — it sits with the busy skip before the inspection-budget increment. Safe by construction: every real wake-up bumps updatedAt (reviews, comments, labels, pushes) or routes in real time anyway, so the only thing deferred is the scheduled re-confirmation of idleness plus worst-case a few hours of base-conflict-detection latency for a PR nobody touched in a day. The forced-dispatch path never builds the list files, so a forced PR is always inspected. * feat(autofix): idle-backoff review follow-ups - Corrected the comment's cost model: idle candidates hit 'continue' before the TARGETS append, so they never contend for the 10-target cap — the real win is the shared inspection budget plus the serial scan-walk latency (the walk is what delayed QwenLM#8002's pickup by ~6 minutes), and the comment now says exactly that. - Slot quantum changed from the hour to the scan tick (600s, the same quantum as ROT_OFF): an hourly slot against the */10 cron meant 6 back-to-back inspections then a ~3h blind window per PR — same 25% average, terrible shape. The gap is now bounded at ~30 minutes, which is what the operator-facing strings promise ('gap ≤30m'). - The two scan-only signals updatedAt cannot see (a base conflict appearing when main moves; still-red checks awaiting the redcheck marker) are named in the comment instead of papered over. - The per-candidate jq fork became a single precomputed set + a bash substring test, matching the busy skip's idiom and the 'free' claim. - Tests: the skip predicate and set builder got a behavioral replay (idle+out-of-slot defers, idle+in-slot inspects, fresh inspects, missing-from-lookup inspects); the two byte-distance assertions became a loop-head slice (comment growth cannot red-light CI, and budget-consuming code between the skips and the increment fails); the --json field pin is order-independent; the 3600 quantum is pinned OUT. * test(autofix): pin the null-updatedAt defensive guard in idle-backoff replay (QwenLM#8049) * fix(autofix): extract idle-backoff predicate from workflow, fix gap bound 30→40m (QwenLM#8049) * test(autofix): pin takeover-prs.json in idle-backoff replay (QwenLM#8049) * fix(autofix): state idle-backoff gap probabilistically, unpin quantum (QwenLM#8049) Round-2 verification showed the scheduled scan lands every ~40-70 min on this repo, not every 10 min, so the (epoch/600)%4 slot is an independent ~25% draw per scan, not a deterministic 1-in-4 rotation. The gap is geometric (measured median ~2h, p90 ~6h), not bounded at ~40m. Reword the operator-facing strings (skip echo, fleet row) and the workflow comment to state the behaviour probabilistically and drop the false "bounded" / "no PR is unlucky forever" claims; correct the misattributed QwenLM#8002 latency (queue/startup, not the serial walk). Relax the tests so a truthful number is not a CI failure: pin the mod-4 time-quantum shape instead of the exact 600s constant, and drop the /3600 exclusion that forbade the better-tailed quantum. Mechanism logic is unchanged. --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-ci-bot <[email protected]>
|
@qwen-code /triage |
4 participants
What this PR does
This PR adds bounded byte-cursor paging to workspace text reads across the HTTP, ACP, TypeScript SDK, and daemon MCP surfaces. An ordinary
line/limit/maxBytesrequest can now returnhasMoreand an opaquenextCursor; passing that cursor back seeks directly to the next line start instead of scanning from byte 0 again.Large reads remain bounded to 256 KiB of returned UTF-8 and 8 MiB of line-offset scanning. Reads stay bound to the opened file descriptor, reject binary or unsupported streamed encodings, preserve BOM and CRLF metadata, avoid splitting UTF-8 characters, and keep append-only log cursors valid while rejecting detectable replacement or truncation.
The branch has also been reconciled with current
main, including the handle-bound snapshot contract and the SDK bundle-budget update. Conflict resolution retained the stricter file-size snapshot, workspace-runtime isolation, symlink checks, and mutation-error precedence while removing duplicate implementations.Why it's needed
Line-number paging scans from the beginning on every request, so walking a large log page by page is O(n²). A deep line request is also refused after the 8 MiB scan budget. The only prior O(1) alternative was raw byte reads, which force clients to reimplement line boundaries, encoding handling, multibyte safety, and binary-file refusal.
Reviewer Test Plan
How to verify
limit; confirm the response is bounded and returnshasMore: trueplusnextCursor.nextCursorwith the same path and limit until it is absent; join page contents with\nand confirm they reproduce the source text.hash_mismatch.cursorpluslineand malformed cursors returnparse_error, a deep line offset beyond 8 MiB returnsfile_too_large, and large non-UTF-8 or binary content returnsbinary_file.hasMore: true, and never emits an unsafe mid-line cursor; an unterminated final line must end paging without an error.Evidence (Before & After)
Before: each text page identified by line number rescanned the prefix, deep pages failed after 8 MiB, and the original CI run stopped at the TypeScript SDK bundle budget (
180318 > 180224).After: daemon-issued byte cursors resume at line starts in O(1), all conflict paths are reconciled with current
main, and the SDK bundle succeeds under the 178 KiB budget that accommodates the combined cursor and current-main API surfaces.npm run lintnpm run buildnpm run typecheckTested on
Environment (optional)
macOS 26.4.1 (25E253), Node.js 22.22.3, local non-sandboxed workspace.
Risk & Scope
413 file_too_largeinstead of content. Large non-UTF-8 or binary reads may now return422 binary_fileinstead of413 file_too_large. Clients that branch on these status codes must handle both responses; deep offsets should usenextCursorfrom a shallower read orreadBytes. The cursor request parameter and response fields are otherwise additive, and clients can preflight the newworkspace_file_read_cursorcapability.Linked Issues
Related: #7967 and #7947. Addresses the O(n²) paging gap left by #7946.
中文说明
此 PR 的改动
此 PR 为工作区文本读取增加了有界的字节游标分页，并贯通 HTTP、ACP、TypeScript SDK 与 daemon MCP 接口。普通的
line/limit/maxBytes请求现在可以返回hasMore和不透明的nextCursor；再次传入该游标时会直接定位到下一行行首，而不再从字节 0 重新扫描。大文件读取仍限制为最多返回 256 KiB UTF-8 内容，并最多扫描 8 MiB 来解析行偏移。读取始终绑定到已打开的文件描述符，拒绝二进制内容或流式路径不支持的编码，保留 BOM 与 CRLF 元数据，避免拆分 UTF-8 字符；仅追加日志的游标保持有效，同时可检测的文件替换或截断会被拒绝。
该分支也已与当前
main完成整合，包括基于句柄的快照契约和 SDK bundle 预算更新。冲突解决保留了更严格的文件大小快照、工作区运行时隔离、符号链接检查以及文件变更错误优先级，并移除了重复实现。为什么需要此改动
按行号分页时，每次请求都会从文件开头扫描，因此逐页读取大型日志的复杂度是 O(n²)。超过 8 MiB 扫描预算的深层行请求也会被拒绝。此前唯一的 O(1) 替代方案是读取原始字节，这会迫使客户端自行重新实现行边界、编码处理、多字节字符安全和二进制文件拒绝逻辑。
Reviewer 测试计划
如何验证
limit读取一个大于 256 KiB 的 UTF-8 日志；确认响应大小有界，并返回hasMore: true和nextCursor。nextCursor，直到游标消失；使用\n拼接各页内容，并确认结果还原源文本。hash_mismatch。cursor与line以及传入格式错误的游标会返回parse_error；超过 8 MiB 的深层行偏移返回file_too_large；大型非 UTF-8 或二进制内容返回binary_file。hasMore: true，且绝不产生不安全的行中游标；没有结尾换行符的最后一行必须正常结束分页而不能报错。证据（改动前与改动后）
改动前：每个按行号定位的文本页都会重新扫描文件前缀，深层页面在扫描 8 MiB 后失败，原始 CI 还会在 TypeScript SDK bundle 预算检查处停止（
180318 > 180224）。改动后：daemon 签发的字节游标能够以 O(1) 从行首继续读取，所有冲突路径均已与当前
main完成整合，SDK bundle 也在为游标 API 与当前 main API 合并体积设置的 178 KiB 预算内成功构建。npm run lintnpm run buildnpm run typecheck测试平台
环境（可选）
macOS 26.4.1（25E253）、Node.js 22.22.3、本地非沙箱 workspace。
风险与范围
413 file_too_large；大型非 UTF-8 或二进制读取也可能从413 file_too_large改为422 binary_file。依赖这些状态码分支的客户端需同时处理两种响应；深层偏移应从较浅读取返回的nextCursor继续分页，或使用readBytes。游标请求参数和响应字段本身仍是增量兼容，客户端可以预检新的workspace_file_read_cursorcapability。关联问题
相关：#7967 和 #7947。处理 #7946 遗留的 O(n²) 分页缺口。