fix(sdk): support relative artifact download URLs - #9734
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: this one is real, not theoretical. The top-level Direction: aligned. Web Shell is under active development (e.g. #9730 landed this week), and same-origin daemon mode is its default configuration — artifact downloads breaking there is a genuine user-facing defect. Size: not applicable — the change is not in core paths (9 production lines in the SDK daemon client, 24 lines of tests). Approach: the scope feels right and the shape matches convention — the fix mirrors the workspace-scoped Risk: no elevated risk signals — no high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题: 这是真实存在的 bug,不是理论性问题。顶层 方向: 对齐。Web Shell 正在活跃开发(例如本周合入的 #9730),同源 daemon 模式是它的默认配置——在那里 artifact 下载坏掉是真实可见的用户缺陷。 规模: 不适用——改动不在核心路径(SDK daemon 客户端 9 行生产代码,24 行测试)。 方案: 范围合理,形态也符合惯例——修复方式与工作区作用域的 风险: 无升级风险信号——未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRead independently first: given "artifact byte downloads must work with a relative daemon base URL", my proposal would be exactly what this PR does — stop parsing the URL and instead build the query with Verified against the reviewed commit:
Standing non-blocking follow-up (restated from Stage 1): the sibling top-level helpers — Test evidence (PR's own CI, read via API — PR code never executed in this environment)The main unit/integration suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Sandboxed verification would settle the remaining gap: 中文说明代码审查先独立形成方案再对照:给定"让 artifact 字节下载支持相对 daemon 基础地址",我的方案与本 PR 完全一致——不再解析 URL,改用 已在被审查提交上核实:
既有的非阻塞跟进项(与 Stage 1 相同):顶层兄弟辅助方法—— 测试证据(来自 PR 自身 CI,通过 API 读取——本环境未执行任何 PR 代码)主单元/集成套件( 沙箱验证可以补上剩余缺口: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal, convention-matching fix for a verified defect; the only non-blocking nit is the sibling helpers that still carry the same defect (author scoped them out deliberately). Stepping back: this is exactly the kind of PR the gate should wave through quickly. The problem is real and verified in code — the Web Shell's default same-origin mode hands the client an empty base URL, and this one method insisted on parsing it as absolute, killing artifact byte downloads before the request ever left the page. My independent proposal before reading the diff was identical to what landed: adopt the Two honest reservations, neither blocking: the sibling top-level helpers ( Verdict: approve. CI is still running ( 中文说明置信度:4/5 —— 对一个已核实的缺陷给出了干净、最小、符合惯例的修复;唯一非阻塞的小遗憾是兄弟辅助方法仍带有同样的缺陷(作者已有意将其划出范围)。 退一步整体看:这正是审查关卡应该快速放行的 PR。问题真实且已在代码中核实——Web Shell 默认的同源模式会把空的基础地址交给客户端,而这一个方法坚持把它当绝对地址解析,导致 artifact 字节下载在请求发出前就失败。我在读 diff 之前独立形成的方案与最终实现完全一致:采用文件其余部分已在使用的 两点坦率的保留意见,均不阻塞:顶层兄弟辅助方法(包括 结论:批准。CI 仍在运行(被审查提交上的 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
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)
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)
Review: PR #9734 — fix(sdk): support relative artifact download URLsVerdict: Comment — no Criticals, 1 Suggestion. FindingsSuggestion — Sibling methods still fail with relative base URLs (packages/sdk-typescript/src/daemon/DaemonClient.ts) A Suggested fix: Apply the same Note: The author explicitly scoped other methods out of this PR ( Coverage
Reviewed by14 agents across all dimensions: issue fidelity, correctness, security, reuse, abstraction fit, consistency, performance, test coverage, adversarial audit (attacker, oncall, maintainer), removed-behavior audit, cross-file tracing, and build/test verification. — qwen3.8-max via Qwen Code /review |
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Checked:
- Encoding parity:
new URLSearchParams({path})+.toString()produces identical %-encoding to the oldurl.searchParams.set()path —/→%2F, spaces →+. Confirmed against the existing test expectation (path=bin.dat) and the new one (path=reports%2Freport.pdf). - Absolute-URL backward compat: the existing test
reads raw bytes as base64 payloads(line 563,baseUrl: 'http://daemon') exercises the same code path with an absolute URL and CI passes (Test (ubuntu-latest, Node 22.x): green). - New test non-vacuity: with the old code,
new URL('/daemon/file/bytes')throws beforefetchis called, so the new test would fail with an exception — not a no-op assertion. offset: 0edge case:0 !== undefined→query.set('offset', '0')included; test confirms.- Sibling:
WorkspaceDaemonClient.readWorkspaceFileBytes(line 6260) already usesURLSearchParams+ template literal — pattern is consistent with the broader codebase. - Scope of
new URL()residue:readWorkspaceFile,fileStat,dirListinDaemonClientstill usenew URL(). Author explicitly scopes these out in the PR description; no regression introduced.
Ran: CI Test (ubuntu-latest, Node 22.x) — pass; web-shell E2E Smoke (ubuntu-latest, Node 22.x) — pass.
Not covered: macOS/Windows test jobs — SKIPPING (fork-PR pattern, consistent with other PRs in this repo). The changed logic is platform-independent URL string construction; no platform-specific branches introduced.
Reviewed with AI assistance.
|
Released in v0.22.2. |
What this PR does
Allows workspace artifact byte downloads to use a relative daemon base URL while preserving the existing behavior for absolute daemon URLs. It also adds regression coverage for both URL forms.
Why it's needed
Web Shell integrations can configure a same-origin relative daemon path such as
/daemon. Artifact downloads previously constructed that path with the absolute-only URL constructor, causing Chromium to reportFailed to construct 'URL': Invalid URLbefore the byte request reached the daemon.Reviewer Test Plan
How to verify
Configure the client with a relative daemon base path and request raw bytes for a workspace artifact. Confirm that the request reaches the default REST transport with the relative path and encoded query parameters. Also confirm that an absolute daemon URL still produces the same absolute request as before.
Evidence (Before & After)
Before: workspace artifact downloads using a relative daemon base path failed before issuing the byte request with
Failed to construct 'URL': Invalid URL.After: relative and absolute daemon base paths both produce valid byte requests, and the SDK unit test file passes all 348 tests.
Tested on
Environment (optional)
Node.js 22.14.0; focused SDK unit tests and SDK type checking.
Risk & Scope
Linked Issues
N/A
中文说明
此 PR 的改动
让 workspace artifact 字节下载支持相对 daemon 基础地址,同时保持绝对 daemon 地址的现有行为,并为两种地址形式增加回归测试。
为什么需要
Web Shell 集成可以配置
/daemon这样的同源相对 daemon 路径。此前 artifact 下载使用只接受绝对地址的 URL 构造器处理该路径,导致 Chromium 在字节请求到达 daemon 前报错:Failed to construct 'URL': Invalid URL。Reviewer 测试计划
验证方式
使用相对 daemon 基础路径配置客户端并读取 workspace artifact 原始字节,确认请求通过默认 REST transport 发出,且相对路径与查询参数编码正确。同时确认绝对 daemon 地址仍生成与此前一致的绝对请求。
修改前后证据
修改前:使用相对 daemon 基础路径下载 workspace artifact 时,在字节请求发出前报错
Failed to construct 'URL': Invalid URL。修改后:相对和绝对 daemon 基础路径均能生成有效的字节请求,SDK 单元测试文件的 348 个测试全部通过。
测试平台
环境(可选)
Node.js 22.14.0;运行了 SDK 聚焦单元测试与 SDK 类型检查。
风险与范围
关联 Issue
无