test(cli): pin the record_artifact workspacePath round trip - #7434
Merged
Conversation
The `workspacePath` that `write_file` hands the model is resolved by the daemon's `GET /file` route, but the two live in different packages and disagreed about what the path is relative to: `write_file` measured from the session cwd, the route resolves against the bound workspace root. For an ordinary session those coincide, so nothing caught the drift until a worktree session made them differ and every artifact preview 404'd. Neither side's unit tests could have caught it — each was internally consistent, and the producer test asserts only the literal string it emits. Pin the round trip instead: take the real reminder from `buildRecordArtifactReminder`, pull the `workspacePath` out of it, and fetch that over real HTTP through the route. Three cases: an ordinary session (unchanged behaviour, so a future fix can't quietly break the common path), a worktree session, and a worktree session with a same-named file sitting at the workspace root. That last one is the failure worth naming — without the anchoring fix the request does not 404, it returns 200 with the wrong file's contents, which surfaces as a preview showing an unrelated artifact. Verified load-bearing: reverting only the `path.relative` anchoring in write-file.ts leaves the ordinary-session case passing and fails both worktree cases. `buildRecordArtifactReminder` is exported for this, since the string it builds is the contract the consuming package has to honour.
wenshao
requested review from
LaZzyMan,
tanzhenxin and
yiliang114
as code owners
July 21, 2026 14:33
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with evidence — #7429 fixed a real worktree-session artifact 404 caused by producer/consumer disagreeing on what Direction: aligned. Cross-package contract tests for a bug that already shipped once are exactly what's needed. The CHANGELOG doesn't reference this directly but the area (artifact preview in worktree sessions) is clearly relevant. Size: 17 production lines (one export + doc comment in core), 117 test lines. Not applicable for size concerns. Approach: scope feels right — export the existing producer function, drive it and the real HTTP route in three cases. The same-named-file-at-root case is a nice touch (catches "200 with wrong content" which is worse than a 404). No unrelated changes, no scope creep. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，有证据——#7429 修复了 worktree 会话中 artifact 预览 404 的问题（生产端和消费端对 方向：对齐。跨包契约测试正是这种已经出过一次的 bug 所需要的。 规模：17 行生产代码（core 中一个导出 + 文档注释），117 行测试代码。无规模顾虑。 方案：范围合理——导出现有的生产端函数，驱动它和真实 HTTP 路由跑三个用例。根目录同名文件的用例设计得好（捕获"200 但内容错误"这种比 404 更糟的情况）。无无关改动，无范围蔓延。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewIndependent proposal: given the goal of pinning the Comparison with the diff: the PR does exactly this. No simpler path missed. Findings: no blockers, no convention violations.
Test ResultsBoth suites green — the new contract tests pass and the existing write-file tests are unaffected by the export change. 中文说明代码审查独立方案：给定锁住 与 diff 对比：PR 正是这样做的，没有遗漏更简路径。 发现：无阻塞项，无规范违反。导出干净，测试复用现有脚手架，第三个用例（根目录同名文件）最有价值。 测试结果两个测试套件全部通过（35 + 57），导出变更未影响现有测试。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean, focused contract test that pins a real cross-package drift; would merge without hesitation. This is exactly the kind of follow-up that makes a fix stick. The bug in #7429 was subtle — both sides were internally consistent, only the round trip was broken — and the third test case (same-named file at root returning 200 with wrong content) shows real thought about how the next regression might hide. The export is minimal, well-documented, and the tests reuse existing infrastructure. Nothing to maintain, nothing to curse in six months. 中文说明置信度：5/5 —— 干净、聚焦的契约测试，锁住了真实的跨包漂移；毫不犹豫地合入。 这正是让修复持久的那种后续。#7429 的 bug 很隐蔽——两端各自自洽，只有往返是坏的——第三个用例（根目录同名文件返回 200 但内容错误）展示了对下一次回归可能如何隐藏的真正思考。导出最小化、文档清晰、测试复用现有基础设施。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
approved these changes
Jul 21, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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. |
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Collaborator
Author
Code Review —
|
yiliang114
approved these changes
Jul 21, 2026
yiliang114
approved these changes
Jul 21, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
just export function from core
pull Bot
pushed a commit
to mcx/qwen-code
that referenced
this pull request
Jul 21, 2026
) The `workspacePath` that `write_file` hands the model is resolved by the daemon's `GET /file` route, but the two live in different packages and disagreed about what the path is relative to: `write_file` measured from the session cwd, the route resolves against the bound workspace root. For an ordinary session those coincide, so nothing caught the drift until a worktree session made them differ and every artifact preview 404'd. Neither side's unit tests could have caught it — each was internally consistent, and the producer test asserts only the literal string it emits. Pin the round trip instead: take the real reminder from `buildRecordArtifactReminder`, pull the `workspacePath` out of it, and fetch that over real HTTP through the route. Three cases: an ordinary session (unchanged behaviour, so a future fix can't quietly break the common path), a worktree session, and a worktree session with a same-named file sitting at the workspace root. That last one is the failure worth naming — without the anchoring fix the request does not 404, it returns 200 with the wrong file's contents, which surfaces as a preview showing an unrelated artifact. Verified load-bearing: reverting only the `path.relative` anchoring in write-file.ts leaves the ordinary-session case passing and fails both worktree cases. `buildRecordArtifactReminder` is exported for this, since the string it builds is the contract the consuming package has to honour. Co-authored-by: wenshao <wenshao@example.com>
Collaborator
|
Released in v0.20.1. |
Collaborator
|
|
11 tasks
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
3 participants
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.
What this PR does
Pins the
record_artifactworkspacePathround trip with three tests that drive the real producer and the realGET /fileroute over HTTP, and exportsbuildRecordArtifactReminderso the consuming package can reach it.Follow-up to #7429, which merged before this landed on its branch.
Why it's needed
The
workspacePaththatwrite_filehands the model is resolved by the daemon'sGET /fileroute, but the two live in different packages and disagreed about what the path is relative to:write_filemeasured from the session cwd, the route resolves against the bound workspace root. For an ordinary session those coincide, so nothing caught the drift until a worktree session made them differ and every artifact preview 404'd. #7429 fixed the drift.What it could not add was a test that catches the next one. Neither side's unit tests could have caught this one — each was internally consistent — and #7429's producer test asserts only the literal string it emits (
workspacePath ".qwen/worktrees/my-feature/report.html"). Nothing proves that string resolves to the file. The consumer side is worse off:packages/cli/src/serve/contains no file, source or test, that mentions worktrees at all.So this pins the contract itself: take the real reminder from
buildRecordArtifactReminder, pullworkspacePathout of it, and fetch that over real HTTP through the route — the automated form of the manual404 → 200check on #7429.Three cases:
The third is the one worth naming. With an unrelated
report.htmlat the workspace root, the pre-fix path does not 404 — it returns 200 with the wrong file's contents, so the artifact panel renders an unrelated artifact and looks like it worked. A test that only asserted "not 404" would pass against that.Reviewer Test Plan
How to verify
To confirm the tests are load-bearing rather than tautological, revert only the anchoring in
buildRecordArtifactReminder— replacepath.relative(baseDir, filePath)withpath.relative(config.getTargetDir(), filePath)and drop thewtMatchlines — then re-run. The ordinary-session case must still pass and both worktree cases must fail.Evidence (Before & After)
With the fix on main (this PR's tests, unmodified source):
After reverting only the anchoring in
write-file.ts:The ordinary-session case surviving the revert is the part that matters: it shows the two worktree failures are caused by the reverted logic specifically, not by the harness.
Tested on
Environment (optional)
npx vitest runagainst the workspace; the route tests usesupertestagainst the real express app built bycreateServeApp, with a real temp workspace on disk. No daemon or network needed.Risk & Scope
buildRecordArtifactReminderbecomes a public export of@qwen-code/qwen-code-core. It is exported specifically because the string it builds is a cross-package contract that the consuming package has to honour, and the doc comment says so — but it is new public surface, and an alternative would be to duplicate the tool-level mock scaffolding in the cli test instead (~35 mock fields, cast throughas unknown as Config).<projectRoot>/.qwen/worktrees/<slug>, which is the layout used byenter_worktreeand the web-shell worktree sessions.GitWorktreeServicealso maintains<base>/<sessionId>/worktrees/<name>(default base~/.qwen/worktrees, replaceable viacustomBaseDir) for Arena agents, which the regex does not match. I did not verify whether an Arena agent's artifact ever reaches this route with the same bound workspace, so these tests deliberately do not assert anything about that layout. Raised on fix(core): resolve artifact workspacePath against workspace root in worktree sessions #7429 for a decision.Linked Issues
Follow-up to #7429.
中文说明
这个 PR 做了什么
用三个测试锁住
record_artifact的workspacePath往返契约 —— 驱动真实的生产端和真实的GET /file路由走真实 HTTP,并导出buildRecordArtifactReminder让消费方所在的包能拿到它。这是 #7429 的后续 —— 那个 PR 在这部分推上分支之前就已经合入了。
为什么需要
write_file交给模型的workspacePath由 daemon 的GET /file路由解析,但两者分属不同的包,且对"相对于什么"的理解不一致:write_file从会话 cwd 起算,路由按绑定的 workspace 根解析。普通会话下两者恰好重合,所以这个漂移一直没被发现,直到 worktree 会话让它们分叉,artifact 预览全部 404。#7429 修好了这次漂移。但它没能加上一个能拦住下一次漂移的测试。两端的单测都抓不到这次问题 —— 各自都是自洽的 —— 而 #7429 的生产端测试只断言了它吐出的字面量字符串,没有任何东西证明这个字符串能解析到文件。消费端更糟:
packages/cli/src/serve/下没有任何一个文件(源码或测试)提到过 worktree。所以这里锁的是契约本身:取
buildRecordArtifactReminder的真实输出,抽出workspacePath,再通过真实 HTTP 打到路由上 —— 就是 #7429 上那次手工404 → 200验证的自动化版本。三个用例分别护住:普通会话(以后修 worktree 路径时不会悄悄破坏常规路径)、worktree 会话(#7429 的修复),以及workspace 根目录下存在同名文件的 worktree 会话。
第三个最值得点名:根目录下有个无关的
report.html时,修复前的路径不会 404,而是返回 200 加上错误文件的内容 —— artifact 面板会渲染一个无关的产物,看起来像是成功了。一个只断言"不是 404"的测试会在这种情况下通过。如何验证
见上文英文部分的命令(
35 passed/57 passed)。要确认这些测试是承重的而非同义反复,只回退buildRecordArtifactReminder里的锚定逻辑再跑一遍:普通会话用例必须仍然通过,两个 worktree 用例必须挂掉(实测2 failed | 33 passed)。普通会话用例在回退后依然通过,这一点才是关键 —— 它说明那两个失败是被回退的那段逻辑导致的,而不是测试脚手架本身的问题。风险与范围
buildRecordArtifactReminder成为 core 的公开导出。导出的理由是它构造的字符串本身就是一个跨包契约,消费方必须遵守,函数的文档注释也写明了这点 —— 但这确实是新增的公开面。替代方案是在 cli 测试里复制工具级的 mock 脚手架(约 35 个字段,as unknown as Config强转)。<projectRoot>/.qwen/worktrees/<slug>,即enter_worktree和 web-shell worktree 会话使用的布局。GitWorktreeService还维护着<base>/<sessionId>/worktrees/<name>(默认 base 为~/.qwen/worktrees,可通过customBaseDir整体替换)供 Arena agent 使用,正则不匹配这一套。我没有验证 Arena agent 的 artifact 是否会走到同一个路由、绑定同一个 workspace,因此这些测试刻意没有对那套布局做任何断言。已在 fix(core): resolve artifact workspacePath against workspace root in worktree sessions #7429 上提出待决策。