fix(sdk): support relative daemon base URLs in remaining workspace file helpers - #9817
Conversation
…le helpers readWorkspaceFile, fileStat, dirList, workspacePathSuggestions, glob, and uploadFileToPath still built request URLs with the absolute-only URL constructor, so a relative daemon base URL such as /daemon (used by Web Shell) threw "Invalid URL" before the request was sent. Apply the same URLSearchParams + string interpolation pattern that fixed readWorkspaceFileBytes, and add relative-base-URL regression tests for each helper.
github-actions
Bot
added
the
review/self-reported
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @shoemoney — thanks for the follow-up to #9734, and for the concrete reproduction in #9816. The scope reads clearly, but the PR body is missing several required sections from the PR template, so it has to bounce before code review:
## Risk & Scope— the three bullets (main risk or tradeoff / not validated / breaking changes). For a URL-construction change this is exactly the section reviewers will look for — e.g. base URLs with a trailing slash, or upload paths that already carry a query string.## Linked Issues—Fixes #9816currently sits at the bottom of the Reviewer Test Plan; move it into this section so the issue auto-closes on merge.- Reviewer Test Plan sub-sections —
### How to verify,### Evidence (Before & After)(N/Ais fine here, nothing user-visible), and the### Tested onOS table. - Bilingual summary — the
<details><summary>中文说明</summary>translation of the body, per the template.
No code changes needed for this — fill in the sections above, then comment @qwen-code /triage to re-run the gate.
中文说明
@shoemoney 感谢这个对 #9734 的后续修复，也感谢 #9816 中给出的具体复现。改动范围很清晰，但 PR 正文缺少 PR 模板 中的几个必填部分，需要先补齐才能进入代码审查：
## Risk & Scope—— 三个要点（主要风险或权衡 / 未验证项 / 破坏性变更）。对 URL 构造类改动，这正是审查者最关心的部分——例如带尾斜杠的 base URL，或已携带 query string 的上传路径。## Linked Issues——Fixes #9816目前写在 Reviewer Test Plan 末尾，请移到该节，以便合并时自动关闭 issue。- Reviewer Test Plan 子节 ——
### How to verify、### Evidence (Before & After)（本 PR 无用户可见界面，写N/A即可）以及### Tested on操作系统表格。 - 双语摘要 —— 按模板要求提供
<details><summary>中文说明</summary>的正文翻译。
无需改动代码 —— 补齐以上部分后，评论 @qwen-code /triage 即可重新进入 gate。
— Qwen Code · qwen3.8-max
|
Thanks for the precise checklist — all four are addressed, no code changes.
@qwen-code /triage |
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.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (opts.maxBytes !== undefined) { | ||
| url.searchParams.set('maxBytes', String(opts.maxBytes)); | ||
| query.set('maxBytes', String(opts.maxBytes)); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The rewritten maxBytes branch is exercised by no test on either base URL. A mutation probe confirmed it: deleting this branch keeps the entire packages/sdk-typescript suite green (35 files, 1659/1659 passed), while deleting the adjacent tested line branch fails 2 tests — the harness catches sibling deletions, only this one is invisible. A future edit could drop the branch with no test failing, and the client would silently omit maxBytes from GET /file, so the daemon serves its default read instead of the byte window the caller requested. The gap predates this diff and the identical maxBytes serialization in readWorkspaceFileBytes is tested, so risk is limited — one added test case closes it.
// test/unit/DaemonClient.test.ts — extend the new relative-base test:
client.readWorkspaceFile('src/a.ts', { maxBytes: 100, line: 2, limit: 3 })
// expect url: /daemon/file?path=src%2Fa.ts&maxBytes=100&line=2&limit=3— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Fixed in 4c7da78 — and thank you for the mutation probe, it reproduced exactly.
Baseline on this branch: 35 files, 1659/1659 passed. Deleting the maxBytes branch from readWorkspaceFile leaves that identical — 35 files, 1659/1659 — so the branch really was invisible to the suite.
Worth noting why it hid: maxBytes does appear in two existing assertions (lines 578 and 602), but both belong to readWorkspaceFileBytes hitting GET /file/bytes. Different method, different endpoint. Of the four optional params readWorkspaceFile serialises, cursor is covered at line 533 and line/limit by the new relative-base test — maxBytes was the only one nothing pinned on either base URL.
The relative-base test now asserts a second call carrying it:
await expect(
client.readWorkspaceFile('src/a.ts', { maxBytes: 100, line: 2, limit: 3 }),
).resolves.toEqual(payload);
expect(calls[1]?.url).toBe(
'/daemon/file?path=src%2Fa.ts&maxBytes=100&line=2&limit=3',
);I kept the original { line: 2, limit: 3 } assertion rather than replacing it, so the omitted case still proves the branch is conditional.
Re-running the same mutation with this in place now fails:
AssertionError: expected '/daemon/file?path=src%2Fa.ts&line=2&l…'
to be '/daemon/file?path=src%2Fa.ts&maxBytes…'
Test Files 1 failed (1)
Tests 1 failed | 353 passed (354)
Full suite green with the branch restored: 35 files, 1659/1659.
中文说明
已在 4c7da78 中修复——感谢提供变异测试探针，本地完全复现。
本分支基线：35 files, 1659/1659 passed。从 readWorkspaceFile 中删除 maxBytes 分支后结果完全相同——35 files、1659/1659，说明该分支确实未被测试覆盖。
补充一点它为何被漏掉：maxBytes 确实出现在现有的两处断言中（第 578、602 行），但它们属于 readWorkspaceFileBytes 请求的 GET /file/bytes——方法不同，端点也不同。readWorkspaceFile 序列化的四个可选参数中，cursor 由第 533 行覆盖，line/limit 由新增的相对 base URL 测试覆盖，只有 maxBytes 在两种 base URL 下都无测试锚定。
相对 base URL 的测试现在增加了一次携带该参数的调用断言，并保留了原有的 { line: 2, limit: 3 } 断言，以便同时验证该分支是条件生效的。
在此改动下重跑同一变异测试会失败；恢复分支后全量测试通过：35 files、1659/1659。
| async fileStat(filePath: string): Promise<unknown> { | ||
| const url = new URL(`${this.baseUrl}/stat`); | ||
| url.searchParams.set('path', filePath); | ||
| const query = new URLSearchParams({ path: filePath }); |
There was a problem hiding this comment.
[Suggestion] This URL-composition pattern (new URLSearchParams(...) + `${this.baseUrl}<path>?${query.toString()}`) is now pasted into seven methods in this file — the six converted here plus the pre-existing copy in readWorkspaceFileBytes — with no shared helper. Any future change to URL composition (an encoding edge case, trailing-slash handling, a proxied path prefix) requires seven synchronized edits, and any site missed ships broken — this PR itself demonstrates the cost: it re-fixes the relative-base defect in the six copies #9734 left behind. Two consolidation targets exist: the private jsonRequest (line ~924) is behaviorally identical for the bodyless GET-JSON path that fileStat/dirList/workspacePathSuggestions/glob/readWorkspaceFile hand-roll here, and a small private helper would localize composition for the remainder. Non-blocking: the inline idiom is the SDK's established convention since #9734, and this PR follows it correctly.
private requestUrl(path: string, query: URLSearchParams): string {
return `${this.baseUrl}${path}?${query.toString()}`;
}— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Agreed on the diagnosis, and I'd rather not take it in this PR — reasoning below, happy to be overruled.
You're right that the duplication is what caused this: #9734 fixed one copy, six were left behind, and #9816 is the bill for that. A helper is the correct end state.
Two reasons to do it separately:
-
It would widen the blast radius past the fix. Consolidating means touching
readWorkspaceFileBytestoo, which this PR doesn't otherwise modify and which is the one site that was already correct. A regression there would be caused by the cleanup, not by the bug — and it'd land in a PR whose stated scope is "make the six broken siblings work with a relative base." -
jsonRequestisn't a drop-in for all five. You scoped that carefully — "the bodyless GET-JSON path" — and that's right, but the five methods don't share a response shape or error string (GET /file,GET /stat,GET /list,GET /globeach callfailOnErrorwith their own label, andworkspacePathSuggestionshas a distinct!res.okbranch). Routing them throughjsonRequestis a real refactor with its own test surface, not a mechanical extraction.
The smaller requestUrl(path, query) helper you sketched is the part I'd take first — it's mechanical, covers all seven sites, and closes the "any site missed ships broken" hole without touching response handling. Say the word and I'll open it as a follow-up against main once this lands, so the two are reviewable independently and the fix isn't gated on the cleanup.
中文说明
同意你的判断，但我倾向于不在本 PR 中处理——理由如下，如有异议我可以照办。
重复代码确实是根因：#9734 只修复了一处，遗留了六处，#9816 就是代价。抽取 helper 是正确的最终形态。
分开处理的两个理由：
-
会把影响面扩大到修复范围之外。 合并意味着还要改动
readWorkspaceFileBytes，而本 PR 并未涉及该方法，且它本就是唯一正确的一处。若在那里引入回归，原因将是这次清理而非原缺陷。 -
jsonRequest并非五处都能直接套用。 你的限定很准确——"无 body 的 GET-JSON 路径"——但这五个方法的响应结构和错误信息并不一致（GET /file、GET /stat、GET /list、GET /glob各自以不同标签调用failOnError，而workspacePathSuggestions有独立的!res.ok分支）。改用jsonRequest是一次带有独立测试面的真实重构，而非机械抽取。
你给出的较小的 requestUrl(path, query) helper 是我会优先采纳的部分：它是机械性的，可覆盖全部七处，且无需触碰响应处理。若你同意，本 PR 合并后我会针对 main 单独提交后续 PR，使二者可独立审查，修复也不必被清理工作阻塞。
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
@qwen-code /triage |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.8-max via Qwen Code /review (v0.21.10)
Addresses review finding R1-1. readWorkspaceFile serialises four optional params; maxBytes was the only one no test pinned on either base URL. Reproduced the reviewer's mutation probe locally: deleting the maxBytes branch left the whole sdk-typescript suite green (35 files, 1659/1659), so a future edit could drop it and the client would silently omit the byte window from GET /file. The relative-base test now asserts a second call carrying maxBytes alongside line and limit. Re-running the same mutation with this in place fails the suite, so the branch is pinned.
|
Pushed R1-1 ( R1-2 (URL-composition helper) — replied, not taken here. Agreed the duplication is the root cause and a helper is the right end state; I'd rather land it against On the stage-1a bounce: all four items were filled in at the time of my earlier comment, and the body currently carries @qwen-code /triage 中文说明已推送 R1-1（ R1-2（URL 组装 helper）—— 已回复，本 PR 暂不处理。 认同重复代码是根因，抽取 helper 是正确的最终形态；但我倾向于在本 PR 合并后针对 关于 stage-1a 的退回：四项在我此前的评论时即已补齐，当前正文包含 |
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! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Released in v0.23.0. |
4 participants
What this PR does
Applies the URL construction pattern from #9734 to the six workspace file helpers that still used the absolute-only
URLconstructor:readWorkspaceFile,fileStat,dirList,workspacePathSuggestions,glob, anduploadFileToPath. Adds a relative-base-URL regression test for each.Why it's needed
Web Shell integrations configure a same-origin relative daemon base path such as
/daemon. #9734 fixedreadWorkspaceFileBytesand listed the remaining helpers as out of scope; each of them still throwsFailed to construct 'URL': Invalid URLbefore the request reaches the daemon. This finishes that migration with the identicalURLSearchParams+ string interpolation approach, so absolute base URLs produce byte-for-byte the same requests as before.Reviewer Test Plan
How to verify
Run the SDK unit suite:
npm test -- test/unit/DaemonClient.test.tsinpackages/sdk-typescript. With the new tests but pristine source, all six new cases fail withTypeError: Invalid URL; with the fix the file passes 354/354. Pre-existing absolute-URL coverage for these helpers passes unchanged, which is the no-regression check for the standard configuration.npm run lintandnpm run typecheckfor the SDK package are clean.Evidence (Before & After)
N/A (no user-visible change; evidence is the RED/GREEN test output described above).
Tested on
Environment (optional)
N/A (unit tests only).
Risk & Scope
readWorkspaceFileBytesfrom fix(sdk): support relative artifact download URLs #9734, since the construction is the same pattern.uploadFileToPathagainst a live daemon (unit-mocked only); base paths that already carry a query string (none of the six methods accept one today); the seventh helperreadWorkspaceFileBytesis untouched, it was fixed in fix(sdk): support relative artifact download URLs #9734.Linked Issues
Fixes #9816
中文说明
本 PR 的内容
将 #9734 的 URL 构造模式应用到仍在使用仅支持绝对地址的
URL构造函数的六个工作区文件辅助方法：readWorkspaceFile、fileStat、dirList、workspacePathSuggestions、glob和uploadFileToPath。并为每个方法添加了相对 base URL 的回归测试。为什么需要
Web Shell 集成会配置同源的相对 daemon 基础路径（例如
/daemon）。#9734 修复了readWorkspaceFileBytes，并把其余辅助方法列为超出范围；这些方法在请求到达 daemon 之前仍会抛出Failed to construct 'URL': Invalid URL。本 PR 用完全相同的URLSearchParams+ 字符串插值方式完成该迁移，绝对 base URL 生成的请求与之前逐字节一致。审查者测试计划
在
packages/sdk-typescript中运行npm test -- test/unit/DaemonClient.test.ts。仅有新测试而未应用修复时，六个新用例全部因TypeError: Invalid URL失败；应用修复后该文件 354/354 通过。已有的绝对 URL 用例保持不变通过。npm run lint与npm run typecheck均无问题。本地在 macOS 上验证；Windows 与 Linux 依赖 CI。风险与范围
readWorkspaceFileBytes完全一致，因为构造模式相同。uploadFileToPath未对真实 daemon 端到端验证（仅单元 mock）；已携带 query string 的基础路径（这六个方法目前都不接受）；readWorkspaceFileBytes未改动，已在 fix(sdk): support relative artifact download URLs #9734 修复。关联 Issue
Fixes #9816