fix(core): auto-record artifact file writes - #7914
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: the issue is real and structural — artifact recording depends on the model making a second Direction: aligned. Making artifact recording deterministic for the common file-write path is a natural improvement to an existing feature. It doesn't expand scope — it tightens a probabilistic mechanism into a reliable one. Size: 78 production lines (write-file.ts), 84 test lines. Core path ( Approach: the scope feels right. Extracting the workspace-path logic into a shared helper and returning structured Risk: no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：问题是真实且结构性的——Artifact 记录依赖模型在成功写入后再调用一次 方向：对齐。让文件写入路径的 Artifact 记录变成确定性行为，是对现有功能的自然改进，不扩大范围。 规模：78 行生产代码（write-file.ts），84 行测试代码。触及核心路径（ 方案：范围合理。将 workspace-path 逻辑提取为共享辅助函数，并在写入结果中返回结构化 风险：无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewThe approach is what I'd have done independently: extract the workspace-path guard into a shared helper, build a A few things done well:
No critical blockers found. No convention violations. TestingCI is still running on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 No failures so far. The macOS/Windows test jobs and integration tests are skipped (likely gated on the ubuntu unit suite passing first). The author reports Not verified: real-scenario tmux testing (unattended CI run — a maintainer can trigger the isolated 中文说明代码审查实现方案与我的独立评估一致：将 workspace-path 守卫提取为共享辅助函数，在成功写入路径上构建 几个做得好的地方：
未发现关键阻塞问题。无规范违反。 测试CI 仍在运行中。目前无失败。macOS/Windows 测试和集成测试被跳过（可能依赖 ubuntu 单元测试先通过）。作者报告本地测试通过——这是作者的声明，此处未独立验证。 未验证：真实场景 tmux 测试（无人值守 CI 运行——如需行为验证，维护者可触发隔离的 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, minimal change that follows existing patterns; CI still pending. This PR does one thing and does it well: it turns a probabilistic artifact-recording mechanism (model must remember to call Every change in the diff is necessary for the stated goal. No scope creep, no drive-by refactors. The test coverage is thorough — positive cases, negative cases (disabled, non-artifact, outside workspace), case-insensitivity, and the worktree path contract. If I had to maintain this in six months, I'd thank the author. Approval deferred until CI lands green on 中文说明置信度：4/5 —— 干净、最小的改动，遵循现有模式；CI 仍在运行中。 这个 PR 只做一件事并且做得很好：将概率性的 Artifact 记录机制（模型需要记住调用 diff 中的每个改动都是目标所必需的。无范围蔓延，无顺手重构。测试覆盖全面——正向用例、反向用例（禁用、非 Artifact、workspace 外）、大小写不敏感、以及 worktree 路径契约。 批准推迟至 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
🩺 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 |
|
Recovered after restart; restarting task |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
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. ✅
|
🤝 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 冲突，直到移除标签或达到轮次上限。移除 |
…7914) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3663994649]
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.7-max via Qwen Code /review
Maintainer local verification — real daemon, not just unit testsVerified head Verdict: the PR does what it claims, deterministically. Recommend merge. One behavioural side effect is worth a line in the PR body or a follow-up — details in §4. 1. Live daemon A/B — the claim under real conditionsA real The two arms are the same daemon build. The only thing that changes is
The empty list on BASE is a real absence, not a broken probe. Step 5 of the driver has the model call
2. Live negative controls and idempotencyRun against the same session, so these are not separate-fixture claims:
3. Do the new tests actually pin the behaviour?Ten mutations against the PR source, each asserted to have textually landed, then the PR's own
Base-overlay differential (PR tests + BASE source, PR test files kept): 10 failed / 55 passed, so ten of the new assertions genuinely discriminate. With PR source: 65 / 65. The one survivor is 4. New finding — an auto-recorded artifact shadows a later curated
|
| BASE | HEAD | |
|---|---|---|
after record_artifact({title:'manual-positive-control', description:'Q3 revenue deep-dive, curated title'}) |
title = manual-positive-control, description present |
title stays quarterly.html, description undefined |
| what the tool told the model | Recorded artifact "manual-positive-control". |
Recorded artifact "manual-positive-control". |
The cause is pre-existing and not a bug introduced here: mergeArtifact in packages/acp-bridge/src/sessionArtifacts.ts only replaces title/description on a published upgrade, so for two storage: 'workspace' upserts on the same workspacePath the first writer wins. What this PR changes is who writes first — before, write_file never created an entry, so the model's curated title landed; now the filename-titled entry is always there already.
Practical effect: for auto-recorded workspace artifacts the model has no working way to supply a better title or a description, and the attempt silently no-ops while reporting success. The PR body notes titles "default to the filename"; it does not note that they can no longer be improved afterwards.
Not a merge blocker — the tool result now tells the model no follow-up call is needed, so most turns will not attempt it. Worth either a sentence in the Risk & Scope section or a small follow-up (let a same-workspacePath upsert from record_artifact refresh title/description, or have the tool report that the field was ignored).
5. Smaller notes
inferWorkspaceArtifactKind'sdefault: return 'file'is unreachable — every member ofARTIFACT_LIKE_EXTENSIONSis handled explicitly. Already spotted in triage; harmless.buildRecordArtifactReminderis still exported frompackages/core/src/index.tsunder that name, but it no longer builds arecord_artifactreminder. The doc comment was updated; the identifier reads stale for a public export.- CI on the head: 20 success, 19 skipped, 0 failures.
Harness details (reproducible)
- Isolated worktree at PR head; a separate APFS clone of a
maincheckout as the E2E tree, so the daemon runs a real build rather than a symlinked one. - Driver:
integration-tests/pr7914-driver.ts—spawnDaemon-style boot on port 0 with--token,approvalMode: 'yolo', isolatedHOME/QWEN_HOME,security.folderTrust.enabled: false,OPENAI_BASE_URLpointed at the fake server. The fake model branches statelessly on a prompt marker plus a content sentinel, so retries cannot double-fire and a later turn in the same session is not confused by an earlier turn'srole:"tool"message. - Two harness-side traps that would have produced a false negative, both fixed before the numbers above were taken: macOS
/var→/private/var(an unresolved workspace path falls outside the bound workspace and suppresses artifact metadata for reasons that have nothing to do with the PR), and a stalemime@4at the repo root where the lockfile wants2.6.0undersuperagent(breakssupertestcollection, unrelated to this change). Thetsc --buildtype errors seen locally are the pre-existing MCP-SDK/zod mismatch;diststill emits.
中文说明
维护者本地验证 —— 真实 daemon，而不只是单元测试
在 macOS 上验证 head 866fe07f88（base b3873571aa）。triage bot 明确写了"real-scenario testing: not verified"，所以这一轮做的是 真实 qwen serve 的 A/B，外加对新增测试的变异审计。
结论：PR 确实做到了它声称的事，而且是确定性的。建议合并。 有一个行为副作用值得在 PR 描述里补一句或者留个 follow-up，见第 4 节。
1. 真实 daemon A/B —— 在真实条件下检验主张
真实的 qwen serve（构建产物 dist、真实 ACP 子进程、真实 write_file）绑定到临时 workspace，模型侧用仓库自带的 integration-tests/fake-openai-server.ts——不需要 API key，不走网络。脚本化的模型写出 reports/quarterly.html 之后直接结束回合，从不调用 record_artifact，这正是本 PR 针对的失败模式。
两个 arm 用的是同一份 daemon 构建，唯一变化的是 packages/core/src/tools/write-file.ts，并且经过完全相同的 build_package.js 流程重新编译。单变量已验证：3409 个 dist 产物文件中，只有 write-file.{js,d.ts,js.map} 不同。
BASE b3873571 |
HEAD 866fe07f |
|
|---|---|---|
写完 HTML 后的 GET /session/:id/artifacts |
[] —— 文件在磁盘上，面板里什么都没有 |
1 个 artifact —— title quarterly.html、kind html、storage workspace、mimeType text/html、sizeBytes 151、toolName write_file |
| 模型看到的工具结果 | "…call record_artifact with workspacePath …" |
"…automatically recorded … No extra artifact registration step is needed." |
GET /file?path=reports/quarterly.html |
不适用 | 200，内容可往返 |
存储下来的 artifact 上的 toolName: "write_file" 直接证明这条记录来自写入本身。
BASE 上的空列表是真实的"没有"，不是探针坏了。 driver 的第 5 步让模型显式调用 record_artifact；在 BASE 上这会通过同一个端点产出 artifact。也就是说这个 oracle 在两个 arm 里都是工作的。
2. 真实的负向对照与幂等性
以下都在同一个 session 内跑，不是各自独立的 fixture：
- 模型写
src/index.ts→ 没有新增 artifact，工具结果里也没有那句提示。普通源码不会进面板。 - 模型重写同一个 HTML → 仍然只有 1 个 artifact，按
workspacePath去重（updatedAt更新，sha256变化）。反复重新生成报告不会把面板刷爆。
3. 新增的测试真的钉住行为了吗？
对 PR 源码做了十次变异，每次都断言变异确实落地，然后原封不动跑 PR 自己的 write-file.test.ts。9 / 10 被抓到。
Base-overlay 差分（PR 测试 + BASE 源码，保留 PR 的测试文件）：10 失败 / 55 通过，说明其中十条新断言确实有区分力。用 PR 源码：65 / 65。packages/cli/.../workspace-file-read.test.ts：对构建后的 PR core 跑出 35 / 35（它通过 @qwen-code/qwen-code-core 导入 buildRecordArtifactReminder，所以如果用 symlink 的 node_modules，测的其实是 main——这里是对 PR 自己的构建产物跑的）。prettier --check 与 git diff --check 均干净。
唯一存活的变异是 .htm：它在 ARTIFACT_LIKE_EXTENSIONS 里，却是唯一一个 kind 映射没有任何测试覆盖的后缀——把它改成 'file'，65 个测试依然全绿。在现有 it.each 里加一条元组即可。不阻塞合并。
4. 新发现 —— 自动记录的 artifact 会屏蔽后续精心命名的 record_artifact
同一脚本、两个 arm 的实测结果（截图中的第 5 步）：
| BASE | HEAD | |
|---|---|---|
调用 record_artifact({title:'manual-positive-control', description:'Q3 revenue deep-dive, curated title'}) 之后 |
title = manual-positive-control，description 存在 |
title 仍是 quarterly.html，description 为 undefined |
| 工具告诉模型的结果 | Recorded artifact "manual-positive-control". |
Recorded artifact "manual-positive-control". |
根因是既有逻辑，并非本 PR 引入：packages/acp-bridge/src/sessionArtifacts.ts 里的 mergeArtifact 只在 published 升级时替换 title/description，所以对同一 workspacePath 的两次 storage: 'workspace' upsert，先写入者胜出。本 PR 改变的是谁先写入——以前 write_file 从不建条目，模型精心取的标题就能落地；现在以文件名为标题的条目总是已经在那里了。
实际影响：对自动记录的 workspace artifact，模型没有任何可用途径去提供更好的标题或描述，而且这次尝试会静默失效、却仍然报告成功。PR 描述提到标题"默认使用文件名"，但没有提到之后也无法再改。
不阻塞合并——工具结果现在已经告诉模型不需要后续调用，所以大多数回合不会去尝试。建议要么在 Risk & Scope 里补一句，要么做个小的 follow-up（让来自 record_artifact、workspacePath 相同的 upsert 能刷新 title/description，或者让工具明确告知该字段被忽略了）。
5. 其他小点
inferWorkspaceArtifactKind的default: return 'file'不可达——ARTIFACT_LIKE_EXTENSIONS的每个成员都被显式处理了。triage 已经指出，无害。buildRecordArtifactReminder仍以该名字从packages/core/src/index.ts导出，但它已经不再构建record_artifact提示了。文档注释更新了，但作为公开导出，这个标识符名字读起来已经过时。- head 上的 CI：20 成功、19 跳过、0 失败。
验证环境细节（可复现）
- 在 PR head 上开独立 worktree；另外用 APFS clone 出一个
main检出作为 E2E 树，让 daemon 跑真实构建而不是 symlink 拼出来的树。 - Driver：
integration-tests/pr7914-driver.ts—— 仿spawnDaemon的 port 0 +--token启动，approvalMode: 'yolo'，隔离的HOME/QWEN_HOME，security.folderTrust.enabled: false，OPENAI_BASE_URL指向 fake server。fake 模型按 prompt marker 加内容 sentinel 做无状态分支，所以重试不会重复触发，同一 session 里后续回合也不会被前一回合的role:"tool"消息干扰。 - 有两个会造成假阴性的 harness 陷阱，在取上述数据之前已经修掉：macOS 的
/var→/private/var（未解析的 workspace 路径会落在绑定 workspace 之外，从而以与本 PR 无关的原因抑制 artifact metadata），以及仓库根部残留的mime@4（lockfile 要求superagent下用2.6.0，否则supertest收集阶段就失败，与本改动无关）。本地看到的tsc --build类型错误是既有的 MCP-SDK/zod 不匹配问题；dist仍能正常产出。
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review — LGTM (no blockers)
Reviewed at HEAD 866fe07. This is a self-PR, so posting as COMMENT rather than APPROVE. No Critical or Major issues found. The change correctly makes artifact recording deterministic for artifact-like workspace writes by reusing the existing ToolResult.artifacts pipeline that record_artifact already depends on.
What I verified
Artifact consumption chain is real, not aspirational: The artifacts: [artifact] returned from the write success path flows through the same infrastructure as an explicit record_artifact call — coreToolScheduler collects toolResult.artifacts into the response (lines 4595-4596 / 4757-4758), Session.ts forwards them via toolCallEmitter.emitResult({ artifacts }) (line 8193), and the daemon/session surfaces consume them as metadata (per the ToolResult.artifacts doc contract). So the new "automatically recorded … no extra artifact registration step is needed" message is accurate, and telling the model to skip the second call prevents a redundant record_artifact round-trip.
Type and gate consistency: The constructed object matches ToolArtifact exactly (title/kind/storage:'workspace'/workspacePath/mimeType/sizeBytes, all valid union members). ARTIFACT_LIKE_EXTENSIONS and inferWorkspaceArtifactKind agree on the extension set — every gated extension maps to a specific kind. The artifact and the reminder are co-gated through getRecordArtifactWorkspacePath, so they can never diverge (reminder present ⇔ artifact present).
Contract preservation: The worktree-anchoring logic moved into getRecordArtifactWorkspacePath unchanged, and the cross-package workspacePath contract with the daemon's GET /file route remains pinned by workspace-file-read.test.ts. I confirmed the test's emittedWorkspacePath regex (/workspacePath "([^"]+)"/) still matches the new message format, so the round-trip pin is intact despite the wording change. buildRecordArtifactReminder's public export status is unchanged; its contract is the workspacePath, not the message text.
Edge cases: postWriteSizeBytes is best-effort (undefined on stat failure, and sizeBytes is optional); files outside the workspace, ordinary source files, and artifact-disabled sessions all correctly yield no artifact and no reminder; case-insensitive extensions (.HTML) are handled via toLowerCase().
Cross-Validation
| Finding | Other Reviewer | My Assessment |
|---|---|---|
Kind-group test coverage for inferWorkspaceArtifactKind (write-file.ts:734) |
ci-bot (Suggestion) | OBSOLETE — commit 866fe07 adds an it.each covering all kind groups (.ipynb→notebook, .pdf→pdf, .png/.jpeg/.jpg/.svg/.webp→image); verified in the test patch |
The ci-bot's earlier APPROVE (4795357028, since dismissed on the new commit) and the follow-up "CI still running" comment are consistent with a clean review; the single inline suggestion was addressed by the test commit.
Additional Audit Coverage
- Double-recording adversarial check: if a model ignored the "no extra step needed" note and still called
record_artifactwith the sameworkspacePath, the panel would see the path twice — but the message is explicit, the pre-PR behavior already relied on model cooperation, and this change is strictly more deterministic. Not a blocker. getRecordArtifactWorkspacePathis invoked twice (once for metadata, once for the reminder). Deterministic and cheap (string ops + a config flag); the current separation keeps each builder self-contained. Nit at most.inferWorkspaceArtifactKind'sdefault: 'file'is unreachable through the write path (the gate only admits extensions with specific kinds) but makes the function total — harmless and sensible if reused.- Failure path: artifact emission lives inside the success
tryblock after the write completes; a failed write takes thecatchbranch and emits nothing. Correct.
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.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Round summaryOne test-only change addressing the maintainer's mutation-audit finding. No production code modified. Feedback triage
ConflictNo conflict ( 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/模型 |
|
Fixed the Serve A/B CI build failure in 16af7b8. The failure was a TypeScript-only issue from the review cleanup: ToolArtifact.workspacePath is optional in the public type, while write_file only builds workspace artifacts here. I narrowed the local helper return type to a workspace artifact with a required workspacePath, without changing runtime behavior. Validation: npm run build --workspace=packages/core; cd packages/core && npx vitest run src/tools/write-file.test.ts src/tools/record-artifact.test.ts; npx prettier --check packages/core/src/tools/write-file.ts; git diff --check. |
Local verification of #7914 — real daemon, real Web Shell, before/afterI built this branch ( Harness
Result — the core claim holds
The artifact the daemon actually stored on the "after" run: {
"id": "fd5ba0c40a7853b9",
"kind": "html",
"storage": "workspace",
"source": "tool",
"status": "available",
"title": "weather.html",
"workspacePath": "reports/weather.html",
"mimeType": "text/html",
"sizeBytes": 112,
"toolCallId": "call_ez5fk6bc",
"toolName": "write_file"
}Clicking Open on that card renders the generated HTML in the sandboxed artifact preview — the full path from a
One thing that makes the motivation stronger than the PR description states. Negative controls — all holdSame harness, one multi-write turn:
Backwards compatibility: a model that still calls Tests, lint, typecheck
RED/GREEN: reverting only Follow-ups🟠 1. The note is phrased as a fact, but the daemon store can still reject the artifact
The store correctly refuses it, so this is not a security problem — it's a truthfulness problem. The model is told the file was recorded, and the new A markup-bearing filename is rare, so this is not a blocker — but the fix is cheap: mirror the store's guard in 🟡 2. The filename title is now permanent, not just the default
The PR already lists "titles default to the filename" as a tradeoff. Worth stating the stronger form: the model can no longer supply a better title or description for a file it wrote, at all. If richer titles matter, one option is to let a subsequent 🟡 3. Scope: this records every artifact-extension write, not only user-facing reportsIn my negative-control run, 🟡 4. Subagent writes now get a message that isn't true
🔵 5. Nits
Verdict: LGTM. The mechanism works exactly as described, the before/after is unambiguous, the negative controls and the disable path hold, 中文说明#7914 本地验证 —— 真实 daemon、真实 Web Shell、before/after 对比我在本地分别构建了这个分支（ 验证环境
结果 —— 核心结论成立
"after" 那一侧 daemon 实际存下来的 artifact： {
"id": "fd5ba0c40a7853b9",
"kind": "html",
"storage": "workspace",
"source": "tool",
"status": "available",
"title": "weather.html",
"workspacePath": "reports/weather.html",
"mimeType": "text/html",
"sizeBytes": 112,
"toolCallId": "call_ez5fk6bc",
"toolName": "write_file"
}点击卡片上的 Open 会在沙箱化的 artifact 预览里渲染出生成的 HTML —— 从 
有一点让这个改动的必要性比 PR 描述里说的更强。 反向对照 —— 全部符合预期同一套环境，一轮里连续写多个文件：
向后兼容：如果模型在写入之后仍然对同一个 测试、lint、typecheck
**RED/GREEN：**只把 后续建议🟠 1. 提示语是以"事实"陈述的，但 daemon store 仍然可能把这个 artifact 丢掉
store 正确地拒绝了它，所以这不是安全问题，而是"说了不实的话"的问题。模型被告知文件已经被记录，而新的 文件名里带标记字符的情况很少见，所以这不是阻塞项 —— 但修起来很便宜：在 🟡 2. 文件名标题现在是"永久"的，而不只是默认值
PR 里已经把"标题默认用文件名"列为取舍了。这里想补充一个更强的表述：模型现在完全无法为自己写出的文件提供更好的标题或描述。如果更丰富的标题有价值，一个可选做法是允许后续对同一路径的 🟡 3. 影响范围：现在是"所有 artifact 扩展名的写入"都会被记录，而不只是面向用户的报告在我的反向对照运行里， 🟡 4. 子 agent 的写入现在会收到一条不成立的消息
🔵 5. 小问题
**结论：LGTM。**机制与描述完全一致，before/after 对比毫无歧义，反向对照和关闭开关都成立， 🤖 Generated with Claude Code — Claude Opus 5 (1M context) |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedInline findings
Issue-level findings (@wenshao's local verification, round 2 + round 3)
Changes in this commit
Verification
中文说明审查反馈处理行内发现
Issue 级发现（@wenshao 的本地验证第 2 轮 + 第 3 轮）
本次提交的改动
验证
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.7-max via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No changes were made in this round. The only feedback was an automated review-bot comment ( No code changes are warranted. 中文说明本轮未做任何更改。 唯一的反馈来自自动审查机器人（ 无需进行代码更改。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Maintainer local verification, round 4 — head
|
| write | GET /session/:id/artifacts |
note in the tool result |
|---|---|---|
reports/q3-weather.html |
kind:html, storage:workspace, mimeType:text/html, sizeBytes:181 |
"automatically recorded" |
notes/analysis.ipynb |
kind:notebook, mimeType:application/x-ipynb+json |
"automatically recorded" |
reports/chart.svg |
kind:image, mimeType:image/svg+xml |
"automatically recorded" |
src/index.ts |
(absent) | none |
/tmp/…/outside.html |
(absent) | write itself refused: path escapes workspace |
reports/<img src=x onerror=alert(1)>.html |
(absent) | none — 🟠1 fixed |
same reports/dupe.html written twice |
1 artifact, same id, sizeBytes refreshed to the second write's 288 B, updatedAt bumped |
dedupe holds |
QWEN_CODE_DISABLE_ARTIFACT=1 |
[] |
none |
The two behaviours added by 2674aa4a1 — the .ipynb mimeType and the markup-filename suppression — are both confirmed live.
3. Open: the 🟠1 fix closes 1 of the 5 rules the store enforces
buildWorkspaceArtifactMetadata (write-file.ts:702) mirrors hasUnsafeDisplayPayload(title). SessionArtifactStore.normalizeInput validates the same payload against five rules:
| rule | store | mirrored in write_file? |
|---|---|---|
markup in title |
sessionArtifacts.ts:1267 → isDisplayField |
✅ this commit |
markup in workspacePath |
sessionArtifacts.ts:2534 → isDisplayField |
❌ |
title ≤ 200 chars |
sessionArtifacts.ts:1267 |
❌ |
workspacePath ≤ 500 chars |
sessionArtifacts.ts:2534 |
❌ |
| control characters | normalizeString |
❌ |
When any of the four unmirrored rules fires, the exact 🟠1 symptom returns: the model is told the file was recorded and that no follow-up is needed, while the store drops it. Reproduced twice against a live daemon on this head:
# 203-character filename
tool result → …This file was automatically recorded as a workspace artifact… No extra artifact registration step is needed.
daemon.log → [artifacts] session=b990a7a6… action=dropped reason="title exceeds 200 characters"
# directory component carrying an HTML entity (ws/Q&A/summary.html)
tool result → …automatically recorded as a workspace artifact with workspacePath "Q&A/summary.html"…
daemon.log → [artifacts] session=0f537d78… action=dropped reason="workspacePath contains unsafe markup"
Both triggers are edge cases — a >200-char basename, or a directory name containing &entity; / <tag — so this is not a blocker, and it is strictly narrower than what 🟠1 covered. Cheapest fix keeps the shape of the guard already there:
const title = path.basename(filePath);
if (
hasUnsafeDisplayPayload(title) ||
hasUnsafeDisplayPayload(workspacePath) ||
title.length > 200 ||
workspacePath.length > 500
) {
return null;
}Or soften the sentence — "submitted for artifact recording" rather than "was automatically recorded … no extra artifact registration step is needed" — so it stays true whenever the store declines. Either works as a follow-up; I would not hold the merge for it.
4. Status of the other round-3 follow-ups
Re-checked on this head, no new findings:
- 🟡2 filename title is permanent — unchanged.
mergeArtifactstill keeps...existingfor twostorage:'workspace'upserts, so a laterrecord_artifactcannot refinetitle/description. Still not mentioned in Risk & Scope. - 🟡3 every artifact-extension write is recorded — unchanged, and re-confirmed:
src/assets/icon-a.svgandicon-b.svg, ordinary repo assets, both became session artifacts in this run. - 🟡4 subagent writes get a message that isn't true — unchanged.
AgentToolResultEvent(agents/runtime/agent-events.ts:128) still has noartifactsfield, so a subagent'swrite_fileis told "automatically recorded … no extra artifact registration step is needed" while nothing reaches the parent session's store. - 🔵
.ipynbhad no mimeType — fixed in this commit, verified live above. - 🔵
buildRecordArtifactReminderis production-dead / misnamed — unchanged. Still exported frompackages/core/src/index.tsunder a name that no longer describes what it builds.
5. Gates
| check | result |
|---|---|
packages/core write-file.test.ts |
68 / 68 |
packages/core record-artifact.test.ts |
15 / 15 |
packages/cli workspace-file-read.test.ts (against the PR's own build) |
35 / 35 |
packages/acp-bridge sessionArtifacts + bridgeClient |
185 / 185 |
prettier --check on the 5 changed files |
clean |
eslint on the 5 changed files |
clean |
npm run typecheck --workspace=packages/core |
clean |
git diff --check |
clean |
CI on 2674aa4a1 |
19 pass, 19 skipped, 0 fail |
Mutation matrix — six mutations applied to the PR source, each asserted to have textually landed, then the PR's own unchanged write-file.test.ts run. Baseline 68/68.
| mutation | result |
|---|---|
M1 remove the hasUnsafeDisplayPayload(title) guard |
1 failed ✅ |
M2 remove the .ipynb mimeType fallback |
1 failed ✅ |
M3 map .htm → 'file' |
1 failed ✅ |
M4 map .svg → 'file' |
1 failed ✅ |
M5 stop emitting artifacts on the tool result |
12 failed ✅ |
M6 remove the worktree anchoring in getRecordArtifactWorkspacePath |
1 failed ✅ |
6 / 6 caught. M1 and M2 pin the two behaviours this commit adds. M3 is the .htm gap I reported in round 1 as the one mutation that survived — the it.each tuple added since closes it. Reverting only write-file.ts to the merge base while keeping the PR's tests flips 12 of 68 red, so the suite is not vacuous.
Harness (reproducible)
- Worktrees: PR head
2674aa4a1and merge baseb3873571a, each fully built and bundled, sopackages/clireally loads its own arm's core rather than a symlinked one. - Mock model is an SSE OpenAI-compatible server; step selection is stateless (emit step N once the request already carries N
role:"tool"messages), so a session opened fresh in the browser replays the script from the start and a retry cannot double-fire. - Daemon:
node dist/cli.js serve --port N --hostname 127.0.0.1 --workspace <ws>, isolatedHOMEwiped between runs,approvalMode: 'yolo', loopback (auth-free). - Browser: Playwright Chromium 1440×900 / DPR 2 against the daemon-served Web Shell; the prompt is typed into the real composer, not injected.
- The §3 gap check also runs producer → consumer directly:
buildWorkspaceArtifactMetadatafrom the built core fed into a realSessionArtifactStorefrom the built acp-bridge, so the disagreement is between the two shipped modules rather than a hand-written mock.
中文说明
维护者本地验证 · 第 4 轮 —— head 2674aa4a1，Linux
我的第 3 轮验证针对的是 head 16af7b82f0。此后新增了一个提交 2674aa4a1（guard artifact title safety and fill ipynb mimeType），处理的正是那一轮的 🟠1 以及 🔵5 中的一条 nit。本轮在 Linux 上重新对当前 head 做了完整端到端验证（PR 描述里 Linux 仍标记为未测试）。
两侧都从零构建（npm run build && npm run bundle，没有软链 dist），通过真实 qwen serve daemon 和 daemon 自带的 Web Shell、真实 headless Chromium 驱动，模型侧接脚本化的 mock OpenAI 端点。被测路径上没有任何桩。
结论：🟠1 确实修好了，我认为可以合并。 🟠1 还残留一小块，非阻塞，见第 3 节。
1. 前后对比（真实 Web Shell）
相同 prompt、相同 mock 模型、相同脚本，唯一变量是 daemon 由哪个 checkout 构建。两侧模型都零次调用 record_artifact —— 这正是重点。点击 Open 会通过 daemon 的 GET /file 路由解析 workspacePath 并渲染文件，说明注释里强调的「生产者↔路由」契约在这个 head 上运行时同样成立。
2. 行为矩阵（在运行中的 daemon 上实测）
| 写入 | GET /session/:id/artifacts |
工具结果中的提示 |
|---|---|---|
reports/q3-weather.html |
kind:html、storage:workspace、mimeType:text/html、sizeBytes:181 |
“automatically recorded” |
notes/analysis.ipynb |
kind:notebook、mimeType:application/x-ipynb+json |
“automatically recorded” |
reports/chart.svg |
kind:image、mimeType:image/svg+xml |
“automatically recorded” |
src/index.ts |
无 | 无 |
/tmp/…/outside.html |
无 | 写入本身被拒：path escapes workspace |
reports/<img src=x onerror=alert(1)>.html |
无 | 无 —— 🟠1 已修复 |
同一个 reports/dupe.html 写两次 |
1 个 artifact，id 相同，sizeBytes 更新为第二次写入的 288 B，updatedAt 更新 |
去重成立 |
QWEN_CODE_DISABLE_ARTIFACT=1 |
[] |
无 |
2674aa4a1 新增的两个行为（.ipynb 的 mimeType、带危险标记文件名的抑制）都已实测确认。
3. 遗留：🟠1 的修复只覆盖了 store 五条规则中的一条
buildWorkspaceArtifactMetadata（write-file.ts:702）镜像了 hasUnsafeDisplayPayload(title)。而 SessionArtifactStore.normalizeInput 对同一份 payload 有五条校验：
| 规则 | store 位置 | write_file 是否镜像 |
|---|---|---|
title 含危险标记 |
sessionArtifacts.ts:1267 → isDisplayField |
✅ 本次提交 |
workspacePath 含危险标记 |
sessionArtifacts.ts:2534 → isDisplayField |
❌ |
title ≤ 200 字符 |
sessionArtifacts.ts:1267 |
❌ |
workspacePath ≤ 500 字符 |
sessionArtifacts.ts:2534 |
❌ |
| 控制字符 | normalizeString |
❌ |
未镜像的四条中任意一条触发时，🟠1 的症状就会原样重现：模型被告知文件「已经记录、无需后续步骤」，而 store 实际把它丢弃了。在当前 head 的真实 daemon 上复现了两次：
# 203 字符文件名
工具结果 → …This file was automatically recorded as a workspace artifact… No extra artifact registration step is needed.
daemon.log → [artifacts] session=b990a7a6… action=dropped reason="title exceeds 200 characters"
# 目录名含 HTML 实体（ws/Q&A/summary.html）
工具结果 → …automatically recorded as a workspace artifact with workspacePath "Q&A/summary.html"…
daemon.log → [artifacts] session=0f537d78… action=dropped reason="workspacePath contains unsafe markup"
两个触发条件都属于边缘场景（basename 超过 200 字符，或目录名里含 &entity; / <tag），所以不是阻塞项，范围也严格小于 🟠1。最小改法保持现有 guard 的形状：
const title = path.basename(filePath);
if (
hasUnsafeDisplayPayload(title) ||
hasUnsafeDisplayPayload(workspacePath) ||
title.length > 200 ||
workspacePath.length > 500
) {
return null;
}或者把措辞放软 —— 用「submitted for artifact recording」替代「was automatically recorded … no extra artifact registration step is needed」—— 这样 store 拒绝时这句话依然成立。两种都可以作为 follow-up，我不会因此卡住合并。
4. 第 3 轮其他 follow-up 的现状
在当前 head 上逐条复查，没有新增问题：
- 🟡2 文件名标题不可改 —— 未变。对同一
workspacePath的两次storage:'workspace'upsert，mergeArtifact仍保留...existing，后续record_artifact无法改进title/description。仍未写入 Risk & Scope。 - 🟡3 所有 artifact 扩展名的写入都会被记录 —— 未变，并再次确认：
src/assets/icon-a.svg和icon-b.svg这类普通仓库资源在本轮中都变成了会话 artifact。 - 🟡4 子代理写入会收到不真实的提示 —— 未变。
AgentToolResultEvent（agents/runtime/agent-events.ts:128）仍然没有artifacts字段，因此子代理的write_file会被告知「automatically recorded … 无需额外注册」，而实际上什么都没有进入父会话的 store。 - 🔵
.ipynb缺 mimeType —— 已修复，上面已实测确认。 - 🔵
buildRecordArtifactReminder已是生产环境死代码且命名不符 —— 未变，仍以这个名字从packages/core/src/index.ts导出。
5. 各项检查
| 检查 | 结果 |
|---|---|
packages/core write-file.test.ts |
68 / 68 |
packages/core record-artifact.test.ts |
15 / 15 |
packages/cli workspace-file-read.test.ts（针对 PR 自身构建） |
35 / 35 |
packages/acp-bridge sessionArtifacts + bridgeClient |
185 / 185 |
5 个改动文件的 prettier --check |
通过 |
5 个改动文件的 eslint |
通过 |
npm run typecheck --workspace=packages/core |
通过 |
git diff --check |
通过 |
2674aa4a1 上的 CI |
19 通过，19 跳过，0 失败 |
变异测试 —— 对 PR 源码施加 6 个变异，每个都确认文本已落地，再运行 PR 自带、未修改的 write-file.test.ts。基线 68/68。
| 变异 | 结果 |
|---|---|
M1 移除 hasUnsafeDisplayPayload(title) 保护 |
1 failed ✅ |
M2 移除 .ipynb mimeType 兜底 |
1 failed ✅ |
M3 把 .htm 映射成 'file' |
1 failed ✅ |
M4 把 .svg 映射成 'file' |
1 failed ✅ |
M5 工具结果不再输出 artifacts |
12 failed ✅ |
M6 移除 getRecordArtifactWorkspacePath 里的 worktree 锚定 |
1 failed ✅ |
6 / 6 全部被捕获。M1、M2 正好覆盖本次提交新增的两个行为。M3 是我第 1 轮报告中唯一漏网的 .htm 变异，此后补上的 it.each 元组已经把它堵住。只把 write-file.ts 回退到 merge base、保留 PR 自带测试时，68 个中有 12 个变红，说明测试并非空转。
复现环境
- worktree：PR head
2674aa4a1与 merge baseb3873571a，各自完整构建并打包，因此packages/cli确实加载各自分支的 core，而不是软链过来的。 - mock 模型是 SSE 的 OpenAI 兼容服务；步骤选择无状态（请求中已有 N 条
role:"tool"消息时返回第 N 步），因此浏览器里新开的会话会从头重放脚本，重试也不会重复触发。 - daemon：
node dist/cli.js serve --port N --hostname 127.0.0.1 --workspace <ws>，每轮之间清空隔离的HOME，approvalMode: 'yolo'，走 loopback（免鉴权）。 - 浏览器：Playwright Chromium，1440×900 / DPR 2，访问 daemon 提供的 Web Shell；prompt 是敲进真实输入框的，不是注入的。
- 第 3 节的 gap 检查还直接串联生产者与消费者：从构建产物取
buildWorkspaceArtifactMetadata，喂给构建产物中真实的SessionArtifactStore，因此这个分歧发生在两个实际发布的模块之间，而不是手写的 mock。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Requesting changes on one blocker: the new unsafe-markup test uses a Windows-reserved filename, so it fails in the merge queue — where this PR's CI does not run it. Everything else is inline and non-blocking.
中文说明
请求修改，阻断问题一处：新增的 unsafe-markup 测试使用了 Windows 保留字符文件名，会在合并队列中失败——而本 PR 的 CI 并不运行该任务。其余均为行内的非阻断建议。
— claude-opus-5[1m] via Claude Code /review
| return null; | ||
| } | ||
| return { | ||
| title, |
There was a problem hiding this comment.
[Suggestion] Making write_file the artifact's creator means this basename title becomes permanent, and record_artifact turns into a silent no-op for the same path.
Artifact identity is workspace:${workspacePath} (sessionArtifacts.ts:2393), and mergeArtifact builds next from {...existing} and only reassigns next.title / next.description inside the if (publishedUpdate) branch (:1752). A file must exist before it can be recorded, so after this change write_file is structurally always the first writer for every workspace artifact.
Verified against the real store — upsert exactly what this function emits, then upsert a curated record for the same path:
upsert {title:'q3.html', workspacePath:'reports/q3.html'}
upsert {title:'Q3 Revenue Report', description:'Quarterly revenue breakdown…',
workspacePath:'reports/q3.html'}
changes = []
store = [{"title":"q3.html"}] // no description, no artifact_changed event
…while record_artifact still returns Recorded artifact "Q3 Revenue Report". to the model and Recorded artifact **Q3 Revenue Report**. to the user. It fails silently in both directions.
The Risk section says titles "default to the filename". In practice they lock to it, and description becomes unreachable for every workspace artifact — the panel shows report.html / index.html for everything. That is a user-visible regression in the feature this PR is improving, and it is the half not stated in the description.
The same {...existing} merge freezes toolCallId/toolName — mergeArtifact never reassigns either. Verified with two upserts on identity workspace:report.html carrying call-1 then call-5: sizeBytes updates, toolCallId stays call-1. Since getArtifactsByTurn only collects per-turn references from record_artifact calls and otherwise falls back to artifact.toolCallId, a later turn that regenerates the same report shows no artifact chip — the chip stays pinned to the first turn. "Regenerate the report" is exactly the flow this PR makes the default.
Options, roughly in order of preference:
- Let a same-identity upsert refresh
title/descriptionwhen the existing record's title still equalspath.basename(workspacePath)— i.e. treat the auto title as provisional. - Skip the auto-record when the store already holds that
workspacePath. - At minimum, stop
record_artifactreporting success when the store discarded its title, and say so in its description.
— claude-opus-5[1m] via Claude Code /review
There was a problem hiding this comment.
Declined for this PR. Changing mergeArtifact to let a same-identity upsert refresh title/description requires modifying packages/acp-bridge/src/sessionArtifacts.ts — a different package and a deeper design decision about store merge semantics. Worth a follow-up issue.
中文说明
本 PR 中拒绝处理。要让同 identity 的 upsert 能刷新 title/description，需要修改 packages/acp-bridge/src/sessionArtifacts.ts 中的 mergeArtifact —— 属于不同包、更深层的 store 合并语义设计决策。建议单独开 issue 跟进。
| function formatRecordArtifactReminder(workspacePath: string): string { | ||
| return ( | ||
| `This file was automatically recorded as a workspace artifact with ` + | ||
| `workspacePath "${workspacePath}". No extra artifact registration step ` + | ||
| `is needed.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] This is an unconditional statement of completed fact, but only the daemon/ACP surface actually consumes ToolResult.artifacts — on the others the sentence is false and the model will relay it to the user verbatim.
- Subagents.
write_fileis not inEXCLUDED_TOOLS_FOR_SUBAGENTS, so a subagent writingreport.htmlgets both the artifacts array and this sentence. ButAgentToolResultEventhas noartifactsfield, andSubAgentTrackercallstoolCallEmitter.emitResult({...})without one, so_meta.artifactsis never stamped andextractSessionUpdateArtifactssees nothing. There is noaction=droppedline either — nothing ever reaches the store, so the drop is completely silent. Recovery is closed as well:record_artifactis deliberately excluded from subagents ("V1 session artifacts are owned by the parent daemon session"), and the newrecord_artifactdescription tells the parent not to re-record the path. - TUI and headless
--prompt.isRecordArtifactEnabled()is true on both (it only excludes SDK mode), but the sole reader ofToolCallResponseInfo.artifactsispackages/cli/src/acp-integration/session/Session.ts. There is no artifacts consumer underpackages/cli/src/ui.
The previous wording was a conditional imperative — "If this file is a reusable user-facing artifact, call record_artifact…" — which was harmless where nothing consumed it. Asserting a completed action is not.
Worth either gating the sentence on a session that has an artifact consumer, or phrasing it as a capability rather than a finished action, e.g. "recorded for clients that display an artifacts panel".
— claude-opus-5[1m] via Claude Code /review
There was a problem hiding this comment.
Declined. Gating the sentence on a session that has an artifact consumer would require plumbing surface awareness into the write path; softening the wording would churn ~6 test assertions for a non-blocking preference. The maintainer's round-4 verification confirmed comfort with merging as-is.
中文说明
拒绝。按会话类型门控需要在写入路径中引入场景感知；软化措辞会导致约 6 个测试断言的变动，且属于非阻断偏好。维护者第 4 轮验证已确认可按现状合并。
| function getRecordArtifactWorkspacePath( | ||
| config: Config, | ||
| filePath: string, | ||
| ): string | null { | ||
| if (!config.isRecordArtifactEnabled()) { | ||
| return null; | ||
| } | ||
| if (!ARTIFACT_LIKE_EXTENSIONS.has(path.extname(filePath).toLowerCase())) { | ||
| if (!ARTIFACT_KIND_BY_EXTENSION.has(path.extname(filePath).toLowerCase())) { |
There was a problem hiding this comment.
[Suggestion] This helper anchors the emitted path at config.getTargetDir() and special-cases exactly one divergence — the .qwen/worktrees/<slug> shape. But getTargetDir() is mutable, and the store it feeds is not.
POST /session/:id/cd → bridge.changeSessionCwd → config.relocateWorkingDirectory(..., { skipArtifactMigration: true }) moves targetDir to any trusted absolute directory. changeSessionCwd only publishes a session_cwd_changed event — it never rebuilds entry.artifacts, and SessionArtifactStore is constructed once with workspaceCwd (bridge.ts:3843-3847), which is also GET /file's resolution root. So after a cd into a subdirectory, the emitted workspacePath is relative to the new cwd while both consumers resolve it against the original root.
Reproduced with this PR's own workspace-file-read.test.ts harness, session cwd = <workspace>/packages/core:
emitted workspacePath = "report.html"
GET /file -> 200, "<!doctype html><h1>UNRELATED ROOT FILE</h1>" // wrong file
GET /file (weather) -> 404 path_not_found
That 200-with-the-wrong-file is verbatim what the contract test's comment calls "worse than a 404".
To be clear about attribution: this arithmetic is not introduced here — the pre-existing helper computed the same path. What changes is the consequence. Before, it was a hint the model could ignore; now emission is unconditional and write_file asserts to the model that the file "was automatically recorded … No extra artifact registration step is needed". A defect that used to be probabilistic becomes deterministic for every artifact-like write in a relocated session.
Worth anchoring at the session's bound workspace root rather than getTargetDir() plus a path-shape regex — config.storage.getProjectRoot() is that value on the ACP path, and using it would let the .qwen/worktrees regex be deleted rather than extended. (Check the CLI --worktree startup path first, where Config is constructed with targetDir already inside the worktree.) A smaller alternative: return null — no artifact, no claim — when getTargetDir() is neither the bound root nor <root>/.qwen/worktrees/<slug>.
— claude-opus-5[1m] via Claude Code /review
There was a problem hiding this comment.
Declined. The reviewer confirmed this path arithmetic is pre-existing, not introduced by this PR. Anchoring at the bound workspace root and auditing the CLI --worktree startup path is a separate concern best handled in its own change.
中文说明
拒绝。评审者确认此路径计算是既有逻辑，非本 PR 引入。锚定到绑定的工作区根目录并审计 CLI --worktree 启动路径属于独立关注点，最好单独处理。
| '.png', | ||
| '.svg', | ||
| '.webp', | ||
| const ARTIFACT_KIND_BY_EXTENSION = new Map<string, ToolArtifactKind>([ |
There was a problem hiding this comment.
[Suggestion] Auto-recording changes the rate at which the store's 200-artifact cap is reached, and eviction gives no protection to the artifacts the user actually cares about.
maxArtifacts defaults to 200 and is never overridden (bridge.ts constructs the store with only sessionId/workspaceCwd/persistence), SOURCE_RESERVATIONS.tool = 100, and selectEvictionCandidate (sessionArtifacts.ts:1934) ranks on status === 'missing' → source over-reservation → !clientRetained → oldest createdAt. It never consults storage or retention, and clientRetained is only ever true for source === 'client' — so a published artifact with a live URL is fully evictable. Auto-recorded writes land in source: 'tool', the same bucket as record_artifact and Artifact-publish entries.
Reaching 100 tool artifacts used to require 100 deliberate registration calls. It now requires 100 .html/.svg/.ipynb file writes.
Measured on the real store: seed one published artifact and one record_artifact artifact, then ingest 220 auto-recorded writes → stored=200 publishedSurvived=false recordArtifactSurvived=false. Evictions carry durableTombstoneRequired and are persisted, and rebuild applies removed, so the loss is permanent rather than live-list only. The only trace is a stderr line naming an opaque artifactId.
Two things keep this from being worse, and are why I've filed it as a Suggestion: write_file writes model-generated text, so the image/PDF extensions here are practically unreachable, and rewrites of the same path merge by identity — you need >200 distinct artifact-like paths in one session. An SVG icon set, a generated docs tree, or a long session that accumulates across resumes will get there.
Worth either narrowing this map to the motivating kinds (.html, .htm, .ipynb, .pdf) and leaving bulk .svg to explicit registration, or giving auto-emitted artifacts a lower-priority eviction class so they cannot displace explicitly published ones.
— claude-opus-5[1m] via Claude Code /review
There was a problem hiding this comment.
Declined. Eviction policy lives in packages/acp-bridge/src/sessionArtifacts.ts. Narrowing the extension map or adding a lower-priority eviction class are both out of scope for this PR — worth a follow-up issue.
中文说明
拒绝。淘汰策略位于 packages/acp-bridge/src/sessionArtifacts.ts。缩窄扩展名映射或添加低优先级淘汰类别均超出本 PR 范围，建议单独开 issue 跟进。
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round 5 — Review feedback addressedCritical
Suggestions implemented
Suggestions declined
Verification
中文说明第 5 轮 —— 评审反馈处理阻断项（Critical）
已实施的建议
已拒绝的建议
验证
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/模型 |
Maintainer local verification — round 3 (blocker re-check + live daemon A/B)Verified head Verdict: merge-ready. The blocker is fixed, the guard now genuinely matches what the store enforces, and the feature works live. What's left is three untested guard clauses (§4) and one narrow behaviour change worth a follow-up (§5) — none of them reasons to hold the merge. 1. Blocker resolved —
|
| case | store's reason (before) | reviewed 2674aa4a12 |
head a96c93bd |
|---|---|---|---|
| 206-char basename | title exceeds 200 characters |
note shown, dropped | no artifact, no note |
U+200B in filename |
title contains control characters |
note shown, dropped | no artifact, no note |
out&dir/weather.html |
workspacePath contains unsafe markup |
note shown, dropped | no artifact, no note |
| 524-char path | workspacePath exceeds 500 characters |
note shown, dropped | no artifact, no note |
I also confirmed the guard is not over-strict in the direction that matters: all nine extensions in ARTIFACT_KIND_BY_EXTENSION round-trip into the store with the right kind, and the new .ipynb fallback is load-bearing and accepted — getSpecificMimeType('a.ipynb') really does return undefined, and the store takes application/x-ipynb+json (it is a display field capped at 120 chars, so this was worth checking rather than assuming).
I checked the store's remaining rules for anything the guard still misses. normalizeString trims before measuring, so the untrimmed check here is stricter, never looser; normalizeWorkspacePath additionally rejects absolute and escaping paths, which getRecordArtifactWorkspacePath already handles. I found no remaining case where the note is emitted and the store discards it.
3. Live qwen serve A/B at the current head
Real daemon, real write_file, fake model. The model writes the file and ends the turn — it never calls record_artifact, in either arm. Whatever appears in GET /session/:id/artifacts got there from the write alone.
Single-variable by construction: one tree, one daemon, only packages/core/src/tools/write-file.ts swapped between arms and rebuilt through the same scripts/build_package.js. Hashing the output — 3607 files per arm, only src/tools/write-file.{js,d.ts,js.map} and tsconfig.tsbuildinfo differ. acp-bridge, the CLI, the store and the routes are byte-identical.
BASE (origin/main) reports/quarterly.html -> file on disk, 0 artifacts
HEAD (a96c93bd) reports/quarterly.html -> file on disk, 1 artifact
{"title":"quarterly.html","kind":"html","storage":"workspace",
"workspacePath":"reports/quarterly.html","mimeType":"text/html",
"sizeBytes":59,"toolName":"write_file"}
Negative controls in the same run, both arms: the guard-rejected filename and src/index.ts each produce a file on disk and zero artifacts, so the HEAD result is not "everything becomes an artifact".
4. Mutation matrix — three clauses no test would notice losing
Nine mutations, each proved to land before its run counted. Four caught, five survived — but only three of those are gaps:
M2 / M3 are not gaps. title is path.basename(filePath) and workspacePath always ends with that same basename, so any control character or markup in the title is also in the path. The title-side clauses can never be the sole reason to reject — they are redundant with M5/M6 by construction, and no test can catch removing them. Worth knowing before someone "adds coverage" for them.
These three are real — each is independently reachable and would regress in silence:
- M4
workspacePath.length > 500— reachable with a short filename in deep directories (my case: 524-char path, 6-char title). - M5
hasControlCharacter(workspacePath)— reachable with a control character in a directory segment only. - M9 the delegation.
buildRecordArtifactRemindernow delegating tobuildWorkspaceArtifactMetadatais exactly the fix I asked for, but nothing pins it: I reverted it to compute the path independently and all 70 core tests plus all 40workspace-file-read.test.tstests still passed. The new equality assertion inemittedWorkspacePathonly ever runs on paths that pass the guard, which is where the two agree anyway.
M9 is the one I'd most like to see pinned, because the whole point of the delegation is that a future caller can't reinstate the false claim. One assertion on a rejected filename does it — it can't go in emittedWorkspacePath, which throws when the reminder is null. I wrote it and checked both directions: 71/71 on the unmutated source, and it fails on M9 (1 failed / 70 passed).
// packages/core/src/tools/write-file.test.ts
import {
WriteFileTool,
buildRecordArtifactReminder,
buildWorkspaceArtifactMetadata,
} from './write-file.js';
it('keeps the reminder and the artifact in lockstep when the guard rejects', () => {
const config = {
isRecordArtifactEnabled: () => true,
getTargetDir: () => path.resolve('/ws'),
} as unknown as Config;
const rejected = path.resolve('/ws', 'reports', 'chart onerror=alert(1).html');
expect(buildWorkspaceArtifactMetadata(config, rejected)).toBeNull();
expect(buildRecordArtifactReminder(config, rejected)).toBeNull();
});(path.resolve rather than a literal so the assertion holds on Windows too.)
5. One consequence of the guard worth a follow-up
Choosing drop over fall back to the old reminder is right in three of the four cases — but not the fourth. On main all five rejected filenames still got If this file is a reusable user-facing artifact, call record_artifact with workspacePath "…". At head they get no note at all, and record_artifact's new description tells the model write_file already handles HTML files.
I asked the real store whether a model-chosen title could have survived for each:
| case | record_artifact with a clean model title |
|---|---|
| 206-char basename | RECORDED — recoverable on main, lost at head |
U+200B in filename |
dropped too (workspacePath contains control characters) |
| markup in directory | dropped too (workspacePath contains unsafe markup) |
| 524-char path | dropped too (workspacePath exceeds 500 characters) |
So the guard costs nothing for three of them — it converts a false claim into honest silence, which is a clear improvement. Only the long-filename case loses something real: main had a (probabilistic) path where the model registered it under its own short title, and head has none. Emitting the old reminder text instead of returning null would close that, and is a small change. Narrow enough that I would not hold the merge for it.
6. Carried-over items you declined — re-verified, and I agree they're follow-ups
sessionArtifacts.ts is byte-identical to when I reviewed, so these still stand. I re-ran the most consequential one against the current producer output rather than a hand-written artifact:
write_file emits : {"title":"q3.html"}
record_artifact : {"title":"Q3 Revenue Report"}
changes emitted by the curated upsert : []
stored after both : [{"title":"q3.html"}]
control — different path : change control — external_url : change
write_file is now structurally the first writer for every workspace artifact, and mergeArtifact only reassigns title/description inside if (publishedUpdate). changes: [] means publishArtifactChanges emits nothing, so no client is notified — while record_artifact still answers Recorded artifact "Q3 Revenue Report". The controls confirm the empty change set is real and not a broken probe.
I accept all four as out of scope for this PR — they live in packages/acp-bridge and are design decisions, not defects introduced here. They are worth an issue: title/description lock-in (above), eviction pressure (auto-records share the 100-slot tool reservation with published artifacts), the "automatically recorded" claim on non-daemon surfaces (subagents, TUI, headless --prompt have no ToolResult.artifacts consumer), and getTargetDir() drift after POST /session/:id/cd. Happy to file them.
7. What I ran
packages/core src/tools/write-file.test.ts 70 passed
packages/core src/tools/record-artifact.test.ts 15 passed
packages/cli src/serve/routes/workspace-file-read.test.ts 40 passed
packages/core src/tools/ (full) 2709 passed | 1 failed
prettier --check (all five changed files) clean
The single failure is client-mcp-registrar.test.ts, from an ajv / ajv-formats version split in my local node_modules (addFormats throws inside the MCP SDK's validator). Both that test and its subject are byte-identical to main on this branch, so it is not attributable to this PR.
Harness sources, raw output and the dist hash manifests for both arms are on pr-assets/pr-7914-verify under r3-*.
Recommendation: merge. The four items the bot implemented are all genuinely fixed, and the live A/B shows the feature doing what the PR claims at the current head. I'd like M9 pinned by a test — either in this PR or immediately after — since it's the assertion that keeps the two exports from drifting apart again. Everything else is follow-up work in another package.
中文说明
维护者本地验证 —— 第 3 轮（阻断项复检 + 真实守护进程 A/B）
在隔离的构建树中，于 macOS 上基于 origin/main 验证了 head a96c93bdc2。本轮复检了我今早提交的阻断项以及 bot 同批实现的三条建议，并在新 head 上重跑了端到端证据。
结论：可以合并。 阻断项已修复，guard 现在确实与 store 的校验规则一致，功能在真实环境下工作正常。剩下的是三条未被测试覆盖的 guard 子句（§4）和一处值得后续跟进的窄范围行为变化（§5），都不构成阻止合并的理由。
1. 阻断项已解决 —— chart onerror=alert(1).html
新文件名在 Win32 下合法，且仍能触发 guard，合并队列的 Windows 任务不会再因此失败：
"<img src=x onerror=alert(1)>.html" win32Reserved=true triggersGuard=true (旧)
"chart onerror=alert(1).html" win32Reserved=false triggersGuard=true (新)
对照 Win32 路径组件的保留字符集 [<>:"/\|?*\x00-\x1f] 以及实际的 hasUnsafeDisplayPayload 正则做了校验，后者仍通过 on…= 分支匹配。局限：我验证的是字符集与 guard 行为，并未在 Windows 上实际执行测试套件。
2. guard 现在与 store 一致 —— 这是本次真正的修复
我的评审指出 guard 只镜像了 SessionArtifactStore.normalizeInput 五条规则中的一条，因此"已自动记录"的提示仍可能是假的。我用同一套装置重跑：真实 producer 输入 真实 SessionArtifactStore.upsertMany，非 strict 模式，与 bridgeClient.upsertAndPublishArtifacts 的调用方式完全一致。
此前四种"告诉模型已记录、随后被静默丢弃"的情况现在都被正确跳过：
| 用例 | store 丢弃原因（修复前） | 评审时 2674aa4a12 |
head a96c93bd |
|---|---|---|---|
| 206 字符文件名 | title exceeds 200 characters |
显示提示，被丢弃 | 无 artifact，无提示 |
文件名含 U+200B |
title contains control characters |
显示提示，被丢弃 | 无 artifact，无提示 |
out&dir/weather.html |
workspacePath contains unsafe markup |
显示提示，被丢弃 | 无 artifact，无提示 |
| 524 字符路径 | workspacePath exceeds 500 characters |
显示提示，被丢弃 | 无 artifact，无提示 |
同时确认 guard 没有在关键方向上过严：ARTIFACT_KIND_BY_EXTENSION 中全部九种扩展名都能以正确的 kind 进入 store；新增的 .ipynb 回退确实起作用且被接受 —— getSpecificMimeType('a.ipynb') 的确返回 undefined，store 也接受 application/x-ipynb+json（它是受 120 字符限制的 display field，所以值得实测而非想当然）。
我还核对了 store 其余规则是否仍有 guard 未覆盖之处：normalizeString 先 trim 再判断长度，因此这里的未 trim 检查只会更严、不会更松；normalizeWorkspacePath 额外拒绝绝对路径和越界路径，而 getRecordArtifactWorkspacePath 已处理。未发现仍会"发出提示但被 store 丢弃"的情况。
3. 当前 head 上的真实 qwen serve A/B
真实守护进程、真实 write_file、伪造模型。模型写完文件后直接结束回合 —— 两个分支中都从未调用 record_artifact。 因此 GET /session/:id/artifacts 中出现的任何内容都只可能来自写入本身。
单变量由构造保证：同一棵树、同一个守护进程，两个分支之间只替换 packages/core/src/tools/write-file.ts，并通过同一个 scripts/build_package.js 重新构建。对产物做哈希 —— 每个分支 3607 个文件，仅 src/tools/write-file.{js,d.ts,js.map} 与 tsconfig.tsbuildinfo 不同。 acp-bridge、CLI、store 和路由逐字节相同。
BASE (origin/main) reports/quarterly.html -> 文件已落盘，0 个 artifact
HEAD (a96c93bd) reports/quarterly.html -> 文件已落盘，1 个 artifact
同一次运行中的反向对照（两个分支都成立）：被 guard 拒绝的文件名与 src/index.ts 都会落盘且产生零个 artifact，因此 HEAD 的结果并非"什么都变成 artifact"。
4. 变异矩阵 —— 三条子句被删掉后没有任何测试会发现
九个变异，每个都先证明确实生效才计入结果。四个被捕获，五个存活 —— 但其中只有三个是真正的缺口：
M2 / M3 不是缺口。 title 就是 path.basename(filePath)，而 workspacePath 必然以同一个 basename 结尾，所以 title 中的任何控制字符或标记也必然在 path 中。title 侧的两条子句永远不可能成为唯一的拒绝原因 —— 它们与 M5/M6 在构造上冗余，任何测试都无法捕获其删除。在有人试图"补测试覆盖"之前值得先知道这一点。
这三个是真的，每个都可独立触发，且失效时不会有任何声响：
- M4
workspacePath.length > 500—— 深层目录 + 短文件名即可触发（我的用例：524 字符路径，6 字符标题）。 - M5
hasControlCharacter(workspacePath)—— 仅在目录段含控制字符时触发。 - M9 委托关系。
buildRecordArtifactReminder委托给buildWorkspaceArtifactMetadata正是我要求的修复，但没有任何测试固定它：我把它改回独立计算路径后，core 的 70 个测试与workspace-file-read.test.ts的 40 个测试全部依然通过。emittedWorkspacePath中新增的相等断言只会在通过 guard 的路径上运行，而那正是两者本就一致的地方。
M9 是我最希望被固定的一个，因为委托的全部意义就在于让未来的调用方无法重新引入那个虚假声明。一个针对被拒绝文件名的断言即可 —— 它不能放进 emittedWorkspacePath，因为提示为 null 时该函数会抛错。我已写好并双向验证：未变异源码下 71/71 通过，且在 M9 下失败（1 失败 / 70 通过）。示例代码见英文部分。
5. guard 带来的一个后果，值得后续跟进
选择丢弃而非回退到旧提示，在四种情况中的三种是正确的，但第四种不是。在 main 上，这五个被拒绝的文件名仍会得到 If this file is a reusable user-facing artifact, call record_artifact with workspacePath "…"。而在 head 上它们完全没有提示，同时 record_artifact 的新描述又告诉模型 write_file 已经处理了 HTML 文件。
我向真实 store 询问了每种情况下"模型自选标题"能否存活：
| 用例 | 以干净的模型标题调用 record_artifact |
|---|---|
| 206 字符文件名 | 成功记录 —— 在 main 上可挽救，在 head 上丢失 |
文件名含 U+200B |
同样被丢弃（workspacePath contains control characters） |
| 目录含标记 | 同样被丢弃（workspacePath contains unsafe markup） |
| 524 字符路径 | 同样被丢弃（workspacePath exceeds 500 characters） |
所以 guard 在其中三种情况下没有任何代价 —— 它把虚假声明变成了诚实的沉默，这是明确的改进。只有超长文件名这一种确实有损失：main 上存在一条（概率性的）路径，模型可以用自己的短标题注册它，而 head 上没有了。把返回 null 改成发出旧的提示文本即可闭合，改动很小。范围足够窄，我不会为此阻止合并。
6. 你们拒绝的遗留项 —— 已复验，我同意作为后续处理
sessionArtifacts.ts 与我评审时逐字节相同，因此这些结论依然成立。我用当前的 producer 输出（而非手写 artifact）重跑了其中最关键的一项：
write_file 产出 : {"title":"q3.html"}
record_artifact : {"title":"Q3 Revenue Report"}
curated upsert 产生的 changes : []
两次写入后 store 内容 : [{"title":"q3.html"}]
对照 —— 不同路径 : change 对照 —— external_url : change
write_file 现在在结构上成为每个 workspace artifact 的首个写入方，而 mergeArtifact 只在 if (publishedUpdate) 分支内重新赋值 title/description。changes: [] 意味着 publishArtifactChanges 不会发出任何事件，因此没有任何客户端会收到通知 —— 与此同时 record_artifact 仍然回复 Recorded artifact "Q3 Revenue Report".。两个对照用例证明这个空 changes 集是真实的，而不是探针失效。
我认可这四项都超出本 PR 范围 —— 它们位于 packages/acp-bridge，属于设计决策而非本次引入的缺陷。但值得开 issue 跟进：title/description 锁定（见上）、淘汰压力（自动记录与已发布 artifact 共享 tool 的 100 个配额）、非 daemon 场景下的"已自动记录"声明（subagent、TUI、headless --prompt 都没有 ToolResult.artifacts 的消费方）、以及 POST /session/:id/cd 之后 getTargetDir() 漂移。我可以负责提这些 issue。
7. 执行的验证
packages/core src/tools/write-file.test.ts 70 通过
packages/core src/tools/record-artifact.test.ts 15 通过
packages/cli src/serve/routes/workspace-file-read.test.ts 40 通过
packages/core src/tools/ （全量） 2709 通过 | 1 失败
prettier --check（全部五个改动文件） 无问题
唯一的失败是 client-mcp-registrar.test.ts，源于我本地 node_modules 中 ajv / ajv-formats 的版本分裂（addFormats 在 MCP SDK 的校验器内抛错）。该测试及其被测文件在本分支上与 main 逐字节相同，因此与本 PR 无关。
装置源码、原始输出以及两个分支的 dist 哈希清单都在 pr-assets/pr-7914-verify 的 r3-* 下。
建议：合并。 bot 实现的四项都确实修复了，真实 A/B 也证明功能在当前 head 上达成了 PR 所声称的效果。我希望 M9 能被一个测试固定住 —— 本 PR 内或紧随其后都可以 —— 因为正是这个断言能防止两个导出再次分叉。其余都是另一个 package 中的后续工作。
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-preview via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Round summaryAddressed all three inline suggestions from the automated reviewer plus the maintainer's request to pin the reminder/artifact delegation (M9). No conflict ( Feedback points
Mutation proofTemporarily removed the two path-side guard clauses and reverted the delegation; all three new tests failed (3 failed | 70 skipped), then the source was restored and re-verified green. This confirms the tests pin M4, M5, and M9 rather than passing for an unrelated reason. Verification
中文说明本轮小结处理了自动评审器提出的全部三条行内建议，以及维护者要求固定 reminder/artifact 委托关系（M9）的请求。无冲突（ 反馈点
变异验证临时删除两条 path 侧 guard 子句并回退委托关系后，三个新测试全部失败（3 失败 | 70 跳过）；随后恢复源码并重新验证为全绿。这证明这些测试固定的是 M4、M5、M9，而非因无关原因恰好通过。 验证
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/模型 |
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-preview via Qwen Code /review
…unds (QwenLM#8010) * feat(verify-pr): add four techniques from maintainer verification rounds Two hand-written maintainer rounds contained methods the skill could not have produced. Checked each against the current text before adding it; these four had no coverage at all. From QwenLM#7914 (live daemon A/B on the artifact-recording change): - Run every control on BOTH arms. That round's sharpest finding came from a control whose only job was to validate the BASE probe — "the empty list is a real absence, so have the model call record_artifact and watch an entry appear". Run on head as well, it showed the curated title being silently discarded. The control was not hunting for a bug; running it symmetrically is what found one. - A new writer into a shared store is an ordering change. The PR added write_file as a second writer into the artifact list; the bug was not in the new writer but in the collision, where a pre-existing first-writer-wins merge began discarding record_artifact's curated title and description while still reporting success. Enumerate the other writers, exercise the collision in both orders, and check what the loser is told — and separate the pre-existing cause from the PR's contribution so the author is not blamed for the policy. From QwenLM#7998 (ink cursor fix, real-terminal A/B): - When the oracle is an instrument, corroborate it with a mechanism that does not use that instrument. The hardware cursor row came from `tmux display-message -p '#{cursor_y}'`, then from a marker printed after the TUI exits — which lands wherever the cursor actually was. Two agreeing instruments turn a measurement into evidence; one tool's report about the system is not the system. - Re-run the generator on committed generated artifacts and diff. That round re-ran `npx patch-package ink` and found byte-different hunk headers, proving the .d.ts hunks were hand-written rather than regenerated as the description claimed. Also strengthens Not covered: proving a limitation is environmental requires an A/A control (boot base and head identically, show both fail the same way), because "seems environmental" and a real regression look identical in a report. Mutation-verified 4/4, each with landing proof. Two initially reported `landed: False` — the assertions match the whitespace-normalised text while the rules wrap across lines in the source, so the replace never fired and the green result proved nothing. Re-run against line-accurate anchors, both kill. 89/89 tests; prettier and eslint clean. * feat(verify-pr): teach the timing-race and scenario-arrival checks Third maintainer round mined for method (QwenLM#7934 R4). The blocker it found had zero coverage in the skill — `timer`, `wall-clock`, `flake`, `retry`, `duration`, `deterministic` all returned 0, and the one `race` hit was a substring of "trace". - **Timing-triggered assertions have a threshold — measure it, do not sample it.** A new guard (`expect(false).toBe(true)` after an abort loop) turned a vacuous pass into a deterministic failure, because the case triggers its abort from `setTimeout(..., 1000)` while the query's duration is set by CLI startup rather than the server. Natural completion measured 730-2151 ms, so every box on the fast side of 1000 ms fails. The rule says to measure the operation's natural duration with the trigger disabled and compare it to the timer, because a green run only proves this box was slow enough. - **A speed-correlated failure is not flake, and a retry budget does not absorb it.** Random flake becomes a pass under `retry: 2`; this failed 5/5 runs on all three attempts. The two get opposite verdicts, so the kind has to be established before the verdict is written. Stated plainly in the skill: the verify job runs on a shared, loaded runner — the regime where such a test PASSES. Repetition cannot reproduce a fast-machine failure there; only computing the margin can. A rule that said "run it more times" would be useless in this lane. - **The failure one level before vacuity: the scenario never reached the code under test.** The vacuity check asks whether an assertion can fail; this asks whether the code ever ran. Four abort cases fired during CLI process startup, so the fake server saw zero requests and a suite named for mid-stream aborts never streamed — with every assertion green. Instrument the seam and assert the count is non-zero. Mutation-verified 5/5, each with landing proof against line-accurate anchors. 89/89 tests; prettier and eslint clean. Skill is 472 lines, up from 392. * feat(verify-pr): six more techniques, from three maintainer rounds Mined QwenLM#7836 R2, QwenLM#7885 and QwenLM#7899 for method. Checked each candidate against the current text first; six had zero coverage, the rest were already there (harness teeth-checks, pin dereferencing, boundary probing, and the follow-up round's "re-measure, never diff"). The one that corrects the skill's own core method, from QwenLM#7836: - **Before calling a survivor vacuous, escalate to a finer mutation.** A whole-file revert is blunt enough to remove the PRECONDITION a test depends on, so a good test goes green because its scenario no longer occurs — from the outside, identical to a test that asserts nothing. A `finally`-cleanup test survived reverting four production files and died to deleting one line. Coarse survived + fine killed ⇒ the test is fine and the mutation was wrong. A false "your test is vacuous" costs the author more than a missed survivor does. From QwenLM#7836, the root cause shared by both of its blockers: - **When the same predicate is checked in two places, verify they see the same state.** A guard duplicated across a process boundary is two implementations of one question that diverge when their INPUTS differ. One settings key made a route ask sessionExistsInAnyState() with an unpinned runtime dir while the child asked with a pinned one, turning a clean 409 into a 500 plus a process.exit(1) that killed every session on the channel. Includes the temporal half: lazily-created backing files leave a window where a just-created entity is invisible to any on-disk existence check. - **Measure the blast radius on bystanders.** The caller's own error code understates a shared-state failure; the number that matters is an unrelated session going 200 -> 404 and a workspace list going 2 -> 0. From QwenLM#7885, which took a performance claim apart: - **Isolate the slice the mechanism can actually affect.** A speedup claim is two claims: the mechanism works, and the thing it speeds up matters. `--ignore-scripts` isolated what an npm download cache can touch — 36s of a 226s install — so the ceiling was 20s and the real saving 15%, not the claimed 75%. Then check it against the whole job: 33s off 14m37s. - **A mechanism that persists something has a cost — price it.** 219 MB per lockfile hash into a pool at 9.98 GB of a 10 GB cap, with 39 distinct lockfile states in 30 days: at the cap every entry evicts by LRU, including entries other jobs need and its own. - **Test the scarier consequences and report which do NOT hold.** The write-path finding was real; code injection was disproved (npm integrity-checks a tampered cache and refetches) and privilege escalation was disproved (chown -R does not follow symlinks). A finding that names what it is not is harder to wave away. - **Verify third-party actions from their own manifest.** The PR said the cache dir was discarded after the job; `action.yml` declares `post: dist/save/index.js` with `post-if: success()`, which uploads it as root with credentials intact — the opposite of the claim, and the whole finding. From QwenLM#7899: - **To exercise real production data safely, interpose a refusing proxy on the write path.** Wrap the client so every mutating call hard- fails, then run the shipped script verbatim: real counts, mechanical guarantee of no side effects. Mutation-verified 9/9, each with landing proof against line-accurate anchors. 89/89 tests; prettier and eslint clean. The skill is now 546 lines, up from 392 — the growth is deliberate and called out in the PR body's risk section. * feat(verify-pr): decomposed fixes, contextual limits, destination counts From QwenLM#7862 R4. Three additions, and a deliberate stop. - **When one fix bundles two changes, build the intermediate variants.** An A/B against base proves the pair works and says nothing about what each half does. That round compiled a third build with only the ordering change reverted, and the three-row table showed the halves do different jobs: moving `initialized = true` after the fallible work converts a 2,999-and-climbing backlog flood into a fail-safe retry, while `reduce()` restores liveness. Either alone leaves a channel that floods or wedges — a conclusion the two-cell A/B cannot reach. - **A limit measured in isolation does not transfer to the real call site.** The same `Math.max` spread threw between 110k and 130k elements inside a deep async stack, well below a standalone micro-benchmark. Bisect thresholds through the real code path and quote the harness; a limit taken from documentation or a toy loop is a guess about the system under test. - **Count at the destination, not at the component boundary.** The mirror of the scenario-arrival rule added earlier: envelopes the adapter emitted and prompts that reached the agent are different numbers, and every gate lives between them. A count taken at the seam can be right while the feature is silently dropped downstream. Declined from the same report, to protect prompt budget rather than because they are wrong: siblings-as-convention-oracle (the lockfile version table across five channels), degenerate fixtures that cannot distinguish two sort keys, and naming the condition under which a cosmetic finding becomes real. Each is a good technique; none is worth another rule competing for attention with the ones already here. The skill is now 578 lines, up from 392 on main (+47%) across this branch. That growth is the main risk on this PR and further additions should wait until a live round shows the current set changes behaviour. Mutation-verified 3/3 with landing proof. One mutation initially SURVIVED — it deleted text sitting AFTER the asserted phrase, so the assertion still matched and the green proved nothing; re-run against the phrase itself, it kills. 89/89 tests; prettier and eslint clean. * test(scripts): drop stale technique count from verify test name (QwenLM#8010) * fix(triage): correct verify-skill worked examples and verdict path (QwenLM#8010) Address review feedback on the verification-techniques skill: - Make the npm-cache worked example's numbers close: separate the 20 s download-slice ceiling (36 s to 16 s) from the 15% end-to-end saving (226 s to 193 s) rather than conflating them. - Stop overstating the tarball experiment: one tarball was poisoned, and the 2262-entry integrity coverage is a separate static fact. - Give the speed-correlated-failure rule a contract-legal verdict path by encoding the margin as a scripted assertion, and mark the load/idle sweep as the local-mode variant. - Fix the one bullet that broke its 2-space list continuation. - Pin the new contract-encoding clause in the workflow test. --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
|
Released in v0.21.2. |
6 participants
What this PR does
This PR makes successful file writes for artifact-like workspace outputs emit structured artifact metadata directly in the tool result. The user-facing note now says the file was already recorded instead of asking the model to perform a second registration step.
The metadata stays workspace-scoped and metadata-only. It does not publish HTML, upload files, open a browser, or change the existing publishing flow for interactive artifacts.
Why it's needed
Artifact recording was still probabilistic for generated HTML reports because the file write only reminded the model to call the registration tool afterwards. If the model ended the turn immediately, the artifacts panel never received metadata even though the file had been created successfully.
Returning artifact metadata from the successful write path makes the common generated-HTML case deterministic while preserving the explicit disable behavior, workspace-boundary checks, and worktree path contract.
Reviewer Test Plan
How to verify
Ask the model to create an HTML report using the normal file-write path. The resulting tool call should include a workspace artifact with kind
html, storageworkspace, the workspace-relative path, MIME type, and size metadata without requiring a follow-up registration tool call. Verify that ordinary source files, files outside the workspace, and artifact-disabled sessions do not emit artifact metadata.Evidence (Before & After)
Before: a generated HTML file could be written successfully, but no artifact appeared unless the model chose to make a second registration call. After: the successful file write itself carries the artifact metadata, so the artifacts panel can update from the same tool result.
Local validation:
npm run typecheck --workspace=packages/cliwas also attempted after rebuilding core, but it still fails on existing ACP bridge/replay type mismatches that are unrelated to this change, including missingsessionMcpRuntimeAdd,sessionMcpRuntimeRemove,TODO_STOP_GUARD_CONTINUATION_CLAIM_METHOD, andgoalStatetype fields.Tested on
Environment (optional)
Local macOS development checkout with package-targeted Vitest, Prettier, TypeScript typecheck, and core build commands.
Risk & Scope
Linked Issues
N/A
中文说明
What this PR does
这个 PR 让成功写入的、看起来像 Artifact 的 workspace 文件直接在工具结果里返回结构化 artifact metadata。面向模型的提示也从“请再执行一次注册”改为“文件已经自动记录”。
这些 metadata 仍然只表示 workspace 内的元数据。它不会发布 HTML、上传文件、打开浏览器，也不会改变现有交互式 Artifact 的发布流程。
Why it's needed
之前生成 HTML 报告时，Artifact 记录仍然是概率行为：文件写入成功后，系统只是提醒模型后续调用注册工具。如果模型直接结束当前轮次，Artifacts 面板就收不到 metadata，即使文件已经成功创建。
让成功写入路径直接返回 artifact metadata，可以让常见的 HTML 生成场景变成确定性行为，同时保留显式关闭、workspace 边界检查和 worktree 路径约定。
Reviewer Test Plan
How to verify
让模型通过普通文件写入路径创建一个 HTML 报告。对应工具调用应该直接包含一个 workspace artifact，字段包括 kind
html、storageworkspace、workspace-relative path、MIME type 和 size metadata，并且不需要后续注册工具调用。还应验证普通源码文件、workspace 外文件以及 artifact 关闭场景不会返回 artifact metadata。Evidence (Before & After)
Before：HTML 文件可以成功写出，但除非模型额外执行一次注册调用，否则不会出现在 Artifacts 面板。After：成功写入本身携带 artifact metadata，Artifacts 面板可以从同一个工具结果更新。
本地验证：
重建 core 后也尝试过
npm run typecheck --workspace=packages/cli，但它仍然因为现有 ACP bridge/replay 类型不一致失败，错误包括缺少sessionMcpRuntimeAdd、sessionMcpRuntimeRemove、TODO_STOP_GUARD_CONTINUATION_CLAIM_METHOD和goalState等字段；这些与本次改动无关。Tested on
Environment (optional)
本地 macOS 开发 checkout，执行了 package-targeted Vitest、Prettier、TypeScript typecheck 和 core build 命令。
Risk & Scope
Linked Issues
N/A