fix(acp): reject fractional readTextFile limits#6704
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
This PR addresses a real code-level inconsistency — sliceLineRange with a fractional endLine (e.g. want = 1.5) iterates the newline-walk loop one extra time, silently returning an additional line. The fix is minimal and correct.
However, no reproduction demonstrates this has actually happened in practice. The production serve adapter (BridgeFileSystem → WorkspaceFileSystem.readText()) already rejects fractional limits, so the gap only exists in the inline fallback path (tests + Mode A + channels + IDE companion). The PR body provides a theoretical scenario ("a request like { limit: 1.5 }") but no evidence a real client has ever sent such a value.
Could you provide a before/after reproduction or describe a concrete scenario where a fractional limit reaches this path? Without one, this theoretical concern is better tracked as an issue first.
中文说明
这个 PR 解决的是一个真实存在的代码不一致——sliceLineRange 在小数 endLine（如 want = 1.5）下会多迭代一次循环，静默返回多余的一行。修复本身很小且正确。
然而，没有复现证明这实际发生过。生产 serve adapter（BridgeFileSystem → WorkspaceFileSystem.readText()）已经会拒绝小数 limit，因此这个缺口只存在于 inline fallback 路径（tests + Mode A + channels + IDE companion）。PR 正文只提供了一个理论场景（"{ limit: 1.5 }"），没有证据表明真实客户端曾发送过这样的值。
能否提供一个 before/after 复现，或描述一个具体的场景来说明小数 limit 会到达这条路径？没有复现的话，这个理论性顾虑更适合作为 issue 先跟踪。
— Qwen Code · qwen3.7-max
|
Thanks for the review. I agree with the framing here: this should not be presented as a main After re-checking the reachable paths, the value of this PR is narrower than the original wording suggested. The main The remaining value is better described as embedded ACP fallback hardening. The reachable scenario is: That is a real code path, but it is a narrow one. It requires a direct or embedded For context, this split is consistent with the project history:
So I think the honest conclusion is: this PR has some implementation value as low-risk fallback parity / robustness hardening, but its application value is weak. Accepting it as a small defensive cleanup seems reasonable; rejecting it or asking to track it as an issue first is also reasonable. If you prefer, I can either narrow the PR body to this embedded-fallback framing, or close this PR and open an issue that records the fallback inconsistency without implying a demonstrated stock-user bug. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| if (typeof params.limit === 'number' && params.limit <= 0) { | ||
| return { content: '' }; | ||
| } | ||
| if ( |
There was a problem hiding this comment.
[Suggestion] The new guard validates limit but params.line has no equivalent check. A fractional line (e.g. 1.5) flows into const start = startLine > 0 ? startLine - 1 : 0, producing start = 0.5, which feeds sliceLineRange with a fractional index. Meanwhile workspace-file-system.ts validates both line and limit with isSafeInteger. Consider adding a parallel guard for params.line here for symmetry.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks, addressed in the latest push. I added the symmetric positive-integer guard for line in the inline fallback, so fractional line values now fail before reaching sliceLineRange(), matching the limit validation shape.
| ) { | ||
| throw RequestError.invalidParams( | ||
| undefined, | ||
| '`limit` must be a positive integer.', |
There was a problem hiding this comment.
[Suggestion] The error message omits the received value. The sister validation in workspace-file-system.ts includes it: `limit must be a positive integer, got ${opts.limit}`. Including the actual value helps debugging at a glance.
| '`limit` must be a positive integer.', | |
| '`limit` must be a positive integer, got ${params.limit}', |
The existing test assertion (toContain(' + "" + limit + "" + must be a positive integer')) still passes.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks, done. The inline fallback now includes the received value in both invalid-params messages: line and limit report the malformed value that was rejected.
| expect(response.content).toBe('x\ny\n'); | ||
| }); | ||
|
|
||
| it('propagates parse_error for fractional positive limits', async () => { |
There was a problem hiding this comment.
[Suggestion] This test goes through adapter.readText() → WorkspaceFileSystem.readText(), which has its own pre-existing isSafeInteger guard. The new BridgeClient.readTextFile() guard (the inline fallback path) is never reached here. The test name is technically accurate, but a reader skimming the PR may assume it validates the new code. Consider renaming to clarify, e.g. 'readText surfaces workspace-file-system parse_error for fractional positive limits'.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks, renamed. The adapter test now explicitly says it surfaces the existing WorkspaceFileSystem.readText() parse_error behavior, so it should no longer look like coverage for the new inline fallback guard.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| typeof params.limit === 'number' && | ||
| params.limit > 0 && | ||
| !Number.isSafeInteger(params.limit) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] The new inline guard correctly throws RequestError.invalidParams() (JSON-RPC -32602, Invalid Params). However, the adapter path (production qwen serve) wraps the same WorkspaceFileSystem parse_error as RequestError(-32603) (Internal Error) via preserveFsErrorOverAcp(). The same client error produces different JSON-RPC codes depending on the path: -32602 inline vs -32603 in production.
Consider having preserveFsErrorOverAcp map parse_error kind to -32602 (Invalid Params), or adding a fractional-limit guard in the adapter before wfs.readText so the error is thrown before the -32603 wrapping.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks, addressed. I updated preserveFsErrorOverAcp() so FsError('parse_error') maps to JSON-RPC Invalid Params (-32602) while preserving the existing data.errorKind, data.status, and data.hint fields. Other filesystem error kinds still use the existing internal-error mapping.
Reject positive fractional line and limit values in the BridgeClient readTextFile inline fallback before they reach manual line slicing. Include the received value in the invalid params message, and keep the serve adapter regression test labeled as WorkspaceFileSystem path coverage. Also map FsError(parse_error) through the ACP wrapper as JSON-RPC Invalid Params so injected and inline read-window validation classify malformed params consistently while preserving errorKind/status/hint. Co-authored-by: qwen-code-ci-bot <[email protected]> Co-authored-by: Shaojin Wen <[email protected]>
|
Thanks for the review. I pushed 46c170b with the requested follow-ups:
Local validation: pm --workspace packages/acp-bridge run test -- bridgeClient.test.tspm --workspace packages/cli run test -- src/serve/bridge-file-system-adapter.test.tspm --workspace packages/acp-bridge run typecheckpm --workspace packages/cli run typecheck
Remote CI has started on the latest commit; ubuntu Node 22 is still pending at the moment. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
🧪 Maintainer local verification — ✅ LGTM (macOS)Built and tested this PR locally as a merge reference. All author-stated checks reproduce green, plus additional lint/whitespace/independent-behavior checks pass. This fills the macOS coverage that the PR description marked " Environment: macOS (Darwin 24.6.0) · Node
1) Author-stated checks — reproduced on macOS
2) Independent before/after behavior checkBeyond re-running the author's tests, I wrote a fresh 13-assertion spec (not copied from the PR) and ran the same spec against both the pre-PR
3) Scope confirmationI confirmed the PR's own framing: this is embedded/no- Verdict✅ Verified on macOS — recommend merge. 93 focused tests + typecheck + lint pass, an independent before/after check demonstrates the intended behavior change, and no regressions to existing behavior were observed. 🇨🇳 中文版本（点击展开）🧪 Maintainer 本地验证 — ✅ 建议合并（macOS）作为合并参考，我在本地构建并测试了本 PR。作者列出的全部检查都复现通过，另外补充的 lint / 空白字符 / 独立行为验证也全部通过。 本次验证补齐了 PR 描述中标记为 " 环境： macOS (Darwin 24.6.0) · Node 1）作者列出的检查 — 在 macOS 复现
2）独立的 before/after 行为验证除了复跑作者的测试，我另外写了一份全新的 13 条断言 spec（不是从 PR 拷贝的），并把同一份 spec 分别跑在 pre-PR 的
3）范围确认我确认了 PR 自身的定位：这是针对 embedded / 无 结论✅ 已在 macOS 验证 — 建议合并。 93 个聚焦测试 + typecheck + lint 全部通过，独立的 before/after 验证展示了预期的行为变化，且未观察到对既有行为的回归。 Verification performed locally on macOS by the maintainer. Screenshots hosted on the |
|
@qwen-code /triage |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| if (typeof params.limit === 'number' && params.limit <= 0) { | ||
| return { content: '' }; | ||
| } | ||
| if ( |
There was a problem hiding this comment.
[Suggestion] The guard condition params.limit > 0 && !Number.isSafeInteger(params.limit) has a NaN-shaped hole. NaN > 0 evaluates to false, so NaN falls through both the limit <= 0 early-return (since NaN <= 0 is also false) and this new guard, reaching sliceLineRange with NaN parameters. The sister validation in workspace-file-system.ts:456 uses !Number.isSafeInteger(opts.limit) || opts.limit < 1, which catches NaN because !Number.isSafeInteger(NaN) is true. Adopting the same pattern here would close the gap and align the two paths:
| if ( | |
| if (typeof params.limit === 'number' && !Number.isSafeInteger(params.limit)) { | |
| if (params.limit < 1) { | |
| return { content: '' }; | |
| } | |
| throw RequestError.invalidParams( | |
| undefined, | |
| `\`limit\` must be a positive integer, got ${params.limit}`, | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| if ( | ||
| typeof params.line === 'number' && | ||
| params.line > 0 && | ||
| !Number.isSafeInteger(params.line) |
There was a problem hiding this comment.
[Suggestion] The limit <= 0 early-return at line 1880 runs before this line validation, so a request like { line: 1.5, limit: 0 } silently returns { content: '' } without flagging the fractional line. Moving the line guard before the limit <= 0 check — or at least running both validations before any early-return — would ensure malformed parameters are always reported regardless of ordering.
— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
Rejects positive fractional
line/limitvalues in the ACPreadTextFileinline fallback before they reach manual line slicing.This is intentionally scoped to the embedded/no-
BridgeFileSystemfallback path. The normal stockqwen servepath already routes ACP file reads throughcreateBridgeFileSystemAdapter(...)andWorkspaceFileSystem.readText(), which rejects malformed positive fractional windows asparse_error. This PR keeps an adapter regression test to document that existing production-path validation, and also maps ACP-wrappedFsError('parse_error')to JSON-RPC Invalid Params (-32602) so the injected and inline paths classify malformed read-window params consistently.Why it's needed
What Problem This Solves
readTextFile.lineandreadTextFile.limitare discrete line-window controls.lineselects a 1-based starting line andlimitcaps the maximum number of lines returned. Positive fractional values such as1.5do not have a meaningful line-window interpretation.Before this change, the inline
BridgeClient.readTextFile()fallback only handled non-positivelimitvalues. A malformed positive fractional request could continue into manual slicing when noBridgeFileSystemwas injected:A sibling malformed request with
line: 1.5could also compute a fractionalstartbefore callingsliceLineRange():The realistic application scope is narrow: custom or embedded ACP consumers that construct
@qwen-code/acp-bridge/createAcpSessionBridge(...)withoutBridgeOptions.fileSystem, then accept raw ACPreadTextFileparams from a custom child. This is not a demonstrated stock-userqwen servebug.Changes
if (typeof params.limit === 'number' && params.limit <= 0) { return { content: '' }; } +if ( + typeof params.limit === 'number' && + params.limit > 0 && + !Number.isSafeInteger(params.limit) +) { + throw RequestError.invalidParams( + undefined, + `\`limit\` must be a positive integer, got ${params.limit}`, + ); +} +if ( + typeof params.line === 'number' && + params.line > 0 && + !Number.isSafeInteger(params.line) +) { + throw RequestError.invalidParams( + undefined, + `\`line\` must be a positive integer, got ${params.line}`, + ); +}The inline fallback now rejects positive non-safe-integer
line/limitvalues before opening the file or entering manual line slicing. Existing non-positivelimitbehavior is preserved.The ACP error wrapper now maps
FsError('parse_error')to JSON-RPC Invalid Params (-32602) while still preservingdata.errorKind,data.status, anddata.hint. OtherFsErrorkinds continue to use the existing internal-error wrapper.The adapter test is intentionally named as existing-path coverage: it confirms that
createBridgeFileSystemAdapter()->WorkspaceFileSystem.readText()already rejects positive fractionallimitvalues in the production injected filesystem path.Evidence
The new inline fallback tests cover both malformed values:
limit: 1.5now fails with ACP invalid params before slicing.line: 1.5now fails with ACP invalid params before calculating a fractional start line.The new FsError wrapping test confirms that adapter-path
parse_errorfailures now use JSON-RPC-32602while preserving structured error data.The serve adapter test covers the sibling production path and confirms its pre-existing behavior:
WorkspaceFileSystem.readText()rejectslimit: 1.5asparse_error.Local validation:
Possible call chain / impact
Embedded fallback path changed by this PR:
Stock
qwen serveread-window validation remains owned byWorkspaceFileSystem.readText():This PR does not change non-positive
limithandling,WorkspaceFileSystem.readText()validation, or lower-levelreadTextRange()slicing helpers.Reviewer Test Plan
How to verify
A reviewer can verify this with the focused tests:
The important behavior is that malformed positive fractional
line/limitvalues no longer reach inline manual slicing when noBridgeFileSystemis injected. The serve adapter test documents the existing production-pathWorkspaceFileSystemparse_errorbehavior, and the bridge client test confirms that ACP-wrappedparse_errornow maps to Invalid Params.Evidence (Before & After)
Before:
BridgeClient.readTextFile()only checkedlimit <= 0, so positive fractionallimitorlinevalues could flow intosliceLineRange()through the inline fallback.After:
BridgeClient.readTextFile()rejects positive non-safe-integerline/limitvalues before reading/slicing. The adapter-path test name now makes clear that it covers the existingWorkspaceFileSystem.readText()validation path, not the new inline guard. The ACP error wrapper also mapsparse_errorto-32602, aligning malformed read-window params withRequestError.invalidParams().Local validation on Windows:
Tested on
Environment (optional)
Windows local validation used the repository npm workspace scripts.
Risk & Scope
line/limitvalues now fail earlier in the ACP inline fallback instead of being interpreted by manual slicing.BridgeFileSystemconsumers. It is not a demonstrated stock-userqwen servebug.FsError('parse_error')now maps to JSON-RPC-32602; otherFsErrorkinds keep the existing-32603behavior.qwen servevalidation,WorkspaceFileSystem.readText()behavior, non-positivelimitbehavior, or lower-levelreadTextRange()helpers.Linked Issues
N/A
中文说明
What this PR does
这个 PR 会在 ACP
readTextFile的 inline fallback 路径里提前拒绝正的小数line/limit，避免它进入手写的行切片逻辑。这个改动刻意限定在 embedded / no-
BridgeFileSystemfallback 路径。正常 stockqwen serve路径已经会通过createBridgeFileSystemAdapter(...)和WorkspaceFileSystem.readText()处理 ACP 文件读取，并将格式错误的正小数窗口拒绝为parse_error。本 PR 保留 adapter 回归测试只是为了记录这个既有生产路径 validation；同时会把 ACP 包装后的FsError('parse_error')映射到 JSON-RPC Invalid Params (-32602)，让 injected path 和 inline path 对 malformed read-window params 的分类保持一致。Why it's needed
What Problem This Solves
readTextFile.line和readTextFile.limit都是离散的行窗口控制。line表示 1-based 起始行，limit表示最多返回多少行。像1.5这样的正小数没有合理的行窗口语义。在这个改动之前，inline
BridgeClient.readTextFile()fallback 只处理了非正数limit。当没有注入BridgeFileSystem时，格式错误的正小数请求仍可能进入手写切片：同类的
line: 1.5也会在调用sliceLineRange()前算出小数start：真实应用范围很窄：自定义或 embedded ACP consumer 使用
@qwen-code/acp-bridge/createAcpSessionBridge(...)，但没有传入BridgeOptions.fileSystem，并且接受自定义 child 发来的 raw ACPreadTextFile参数。这不是已经证明存在的 stock-userqwen servebug。Changes
if (typeof params.limit === 'number' && params.limit <= 0) { return { content: '' }; } +if ( + typeof params.limit === 'number' && + params.limit > 0 && + !Number.isSafeInteger(params.limit) +) { + throw RequestError.invalidParams( + undefined, + `\`limit\` must be a positive integer, got ${params.limit}`, + ); +} +if ( + typeof params.line === 'number' && + params.line > 0 && + !Number.isSafeInteger(params.line) +) { + throw RequestError.invalidParams( + undefined, + `\`line\` must be a positive integer, got ${params.line}`, + ); +}inline fallback 现在会在打开文件或进入手写行切片逻辑之前拒绝正数但非 safe integer 的
line/limit。现有非正数limit行为保持不变。ACP error wrapper 现在会把
FsError('parse_error')映射到 JSON-RPC Invalid Params (-32602)，同时继续保留data.errorKind、data.status和data.hint。其他FsErrorkind 继续沿用既有 internal-error wrapper。adapter 测试也刻意改成既有路径覆盖：它确认
createBridgeFileSystemAdapter()->WorkspaceFileSystem.readText()在生产注入文件系统路径中本来就会拒绝正小数limit。Evidence
新增 inline fallback 测试覆盖两类 malformed 值：
limit: 1.5现在会在切片前以 ACP invalid params 失败。line: 1.5现在会在计算小数起始行前以 ACP invalid params 失败。新增 FsError wrapping 测试确认 adapter-path
parse_errorfailure 现在会使用 JSON-RPC-32602，并保留结构化 error data。serve adapter 测试覆盖 sibling 生产路径，并确认其既有行为：
WorkspaceFileSystem.readText()会把limit: 1.5拒绝为parse_error。本地验证：
Possible call chain / impact
本 PR 修改的 embedded fallback 路径：
stock
qwen serve的 read-window validation 仍由WorkspaceFileSystem.readText()负责：本 PR 不改变非正数
limit行为、不改变WorkspaceFileSystem.readText()校验，也不改动更底层的readTextRange()切片 helper。Reviewer Test Plan
How to verify
Reviewer 可以通过聚焦测试验证：
核心行为是：当没有注入
BridgeFileSystem时，格式错误的正小数line/limit不再进入 inline 手写切片逻辑。serve adapter 测试则用于记录既有生产路径WorkspaceFileSystemparse_error行为；bridge client 测试确认 ACP 包装后的parse_error现在会映射到 Invalid Params。Evidence (Before & After)
Before：
BridgeClient.readTextFile()只检查limit <= 0，所以正小数limit或line可以通过 inline fallback 进入sliceLineRange()。After：
BridgeClient.readTextFile()会在读文件/切片之前拒绝正的非 safe integerline/limit。adapter 路径测试名也已明确它覆盖的是既有WorkspaceFileSystem.readText()validation 路径，而不是新的 inline guard。ACP error wrapper 也会把parse_error映射到-32602，与RequestError.invalidParams()对 malformed read-window params 的分类保持一致。Windows 本地验证：
Tested on
Environment (optional)
Windows 本地验证使用仓库 npm workspace scripts。
Risk & Scope
line/limit现在会在 ACP inline fallback 中更早失败，而不是被手写切片逻辑解释。BridgeFileSystemconsumer 的 embedded ACP fallback hardening，不是已经证明存在的 stock-userqwen servebug。FsError('parse_error')现在映射到 JSON-RPC-32602；其他FsErrorkind 继续沿用既有-32603行为。qwen servevalidation、不改变WorkspaceFileSystem.readText()行为、不改变非正数limit行为，也不改变底层readTextRange()helper。Linked Issues
N/A