feat(daemon): add session artifact APIs#5895
Conversation
|
Re-run of Stage 1 gate on the latest PR head ( Template looks good ✓ — all required headings present (What, Why, Reviewer Test Plan, How to verify, Evidence, Tested on, Risk & Scope, Linked Issues, 中文说明). On direction: This PR adds a structured session artifact API to the daemon — agents, hooks, and clients can now register file/link/image/video artifacts that daemon clients display in artifact panels. This fills a genuine capability gap: without it, clients have to scrape tool output text to find intermediate products. The area is clearly within the project's daemon/session-management roadmap. No direct Claude Code CHANGELOG analog, but the existing On approach: The scope is large (61 files, ~9.3K additions) but the additions are proportional to the feature surface: a new tool ( The three-layer entry model (tool → hook → client POST) converging on a single Moving on to code review and testing. 🔍 中文说明Stage 1 在最新 PR head ( 模板完整 ✓ — 所有必需标题齐全。 方向： 本 PR 为 daemon 添加结构化 session artifact API——agent、hook 和 client 可以注册文件/链接/图片/视频等 artifact，daemon 客户端在 artifact 面板中展示。这填补了真实的能力空白：没有它，客户端只能从工具输出文本中抓取中间产物。该领域明确属于项目的 daemon/session 管理路线图。Claude Code CHANGELOG 中没有直接对应项，但现有的 方案： 范围较大（61 个文件，约 9.3K 行新增），但新增量与功能面成正比：新工具（ 三层入口模型（tool → hook → client POST）汇聚到单一 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
|
Re-run of Stage 2 on the latest PR head ( Code ReviewIndependent baseline: I would have designed this with (1) a new Comparison with the diff: The The validation chain is thorough:
The The hook integration in The ACP dispatch properly maps the three artifact methods ( No critical blockers found. The earlier review rounds have addressed the security concerns (cross-session injection, path-like managedId, unsafe display payloads in workspacePath/managedId, failed tool-call artifact ingestion before strip). Testing — Local VerificationCI: The maintainer @wenshao also ran comprehensive end-to-end verification against the real daemon binary (29/29 assertions covering CRUD, dedup, security guards, owner-scoped removal, strict mutation gate, and live SSE events) — all green. 中文说明代码审查独立基线： 我会用以下方式设计：(1) 新的 与 diff 的对比：
验证链全面：URL 安全、workspace 包含、XSS markup 检测、metadata 大小限制、managedId 路径检查。 无关键阻塞项。 早期审查轮次已解决安全关切（跨 session 注入、path-like managedId、workspacePath/managedId 中的不安全显示 payload、失败 tool-call 的 artifact 在 strip 前摄入）。 测试——本地验证总计 727 个测试通过，0 失败。Build 和 typecheck 均通过。CI ubuntu 通过。维护者 @wenshao 也对真实 daemon 二进制做了端到端验证（29/29 断言），全部通过。 — Qwen Code · qwen3.7-max |
|
Re-run of Stage 3 on the latest PR head ( Stepping back and looking at the whole picture: This PR adds a well-designed session artifact API that fills a genuine capability gap in the daemon. The core My independent baseline was simpler, but the PR's extras (published-artifact upgrade paths, per-source eviction reservations, PostToolBatch/PostToolUseFailure hook artifacts, cross-session injection protection, workspace status TTL batching) are justified additions for a production-ready v1, not over-engineering. 727 focused tests pass. Build and typecheck are clean. CI passes on ubuntu. The maintainer's own end-to-end verification (29/29 assertions against the real daemon binary) confirms the API behaves as documented. The author has been highly responsive across multiple review rounds — fixing security gaps promptly, cleaning up unrelated changes, and leaving genuine design decisions (read-path refresh semantics, V1 attribution model, configurable quotas) explicitly open for maintainer discussion rather than making unilateral calls. The one remaining open thread (same-batch overflow Verdict: Code quality is high, tests are comprehensive, build is clean, maintainer has independently verified end-to-end behavior. Approving. 中文说明整体审视：本 PR 添加了一套设计良好的 session artifact API，填补了 daemon 中真实的能力空白。核心 我的独立基线更简单，但 PR 的额外内容（published artifact 升级路径、per-source 淘汰预留、PostToolBatch/PostToolUseFailure hook artifacts、跨 session 注入防护、workspace 状态 TTL 批量）对于生产就绪的 v1 是合理的补充，不是过度工程。 727 个聚焦测试通过。Build 和 typecheck 干净。CI ubuntu 通过。维护者自己的端到端验证（29/29 断言，针对真实 daemon 二进制）确认 API 行为与文档一致。 作者在多轮审查中响应积极——及时修复安全缺口、清理无关变更、将真正的设计决策（读路径刷新语义、V1 归属模型、可配置配额）明确留给维护者讨论。 唯一剩余的开放线程（同批次溢出 结论： 代码质量高，测试全面，构建干净，维护者已独立验证端到端行为。批准。 — Qwen Code · qwen3.7-max |
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. ✅
callmeYe
left a comment
callmeYe
left a comment
There was a problem hiding this comment.
Thanks for the design doc. I think this needs a revision before we use it as an implementation reference.
Main points:
-
Please de-identify the internal/business-specific examples before landing this in the open-source repo. The current doc mentions DataWorks / MaxCompute / dataworks.example.com / mcp__dataworks__get_table in several places. The underlying requirement is valid, but the public design should describe it generically, e.g. "internal data platform table detail URL", "scheduler task detail URL", or "resource dashboard URL".
-
Please tighten the definition of link artifacts. I agree with the direction that ordinary links must not be auto-collected, but the rule should be stated as: a link artifact must come through an explicit artifact registration path with structured metadata. record_artifact is one such path, alongside ArtifactTool structured results, hookSpecificOutput.artifacts, and the client POST API. Assistant markdown links, web_fetch inputs, and shell/stdout URLs should stay out of the artifact store.
-
Please narrow the file-artifact derivation rule. The current design says WRITE_FILE / EDIT / NOTEBOOK_EDIT all produce file artifacts, which would make normal source-code edits show up in the artifacts panel. That feels broader than the stated product semantics of "reusable/clickable/previewable/downloadable session outputs". A safer rule would be to only include files explicitly declared as artifacts, or generated output-like files such as reports, HTML, images, PDFs, notebooks, etc.
-
Please make the v1 vs v1.1 acceptance boundary explicit. The top section marks hook artifacts and client POST as v1.1 or independent follow-ups, but the acceptance criteria later require both. That makes the implementation scope ambiguous.
|
@qwen-code-ci-bot one correction and one gap in the automated review:
So I do not think this is ready to merge as-is despite being docs-only. |
|
Additional note after checking Codex's current implementation for artifact-like outputs:
|
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Code Review
Documentation-only PR. The design document is well-structured and covers the API shape, data model, artifact sources, safety boundaries, implementation phases, and acceptance criteria clearly.
The concern about internal vendor-specific examples (DataWorks/MaxCompute references) is already raised in existing comments. No new high-confidence issues found beyond what's already being discussed.
Generated by Claude Code
设计审计意见：建议先收敛后再作为实现基准我从 qwen-code 当前 daemon/tool/hook 架构、Claude Code / Codex 的 artifact-like 输出设计、以及企业级 CloudAgent 的治理视角看了一轮。结论是：方向正确，但当前文档还不适合作为后续实现 PR 的稳定基准。建议继续调整后再合并。 总体判断这条主线是合理的：
这些方向与现有 daemon 的 EventBus、capabilities、SDK known event、ToolCallEmitter 但文档里有几处关键语义和边界需要收紧，否则实现后 artifacts 面板会很快变成“文件变更列表 + URL 收藏夹”，而不是用户可复用、可预览、可分享的 session outputs。 主要问题1. 不应把普通
|
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
Updated the design doc to address the requested changes. Summary of the revision:
Local validation: git diff --check passed, and the document was scanned for internal/vendor-specific examples and stale idempotency/path wording. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
对设计文档的一轮审查。
说明:首轮审查跑在本 PR 较早的修订上,其中关于 artifact 生命周期的几处主要问题——裁剪不发 removed 事件、kind 必填但无默认、idempotencyKey 去重无对应输入字段、missing 状态无迁移机制、POST 响应为空对象、normalizedUrl 无归一化规则——在当前 head 中均已修复,这部分补得很到位,生命周期与去重语义现在基本闭环。
以下是针对当前版本仍存在的 3 点工程质量建议,均非阻塞:
|
Follow-up adjustment based on product-scope feedback: the document no longer presents The updated framing is:
This keeps the reviewer-requested semantic boundaries while avoiding a fragmented design. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
针对最新 head (ff82501) 的二轮审计。上一轮关于 removed 事件、kind 默认、idempotencyKey、missing 迁移、POST 响应、URL 归一化、枚举跨包重复、双摄入通道、stat 热路径的问题都已修复——这版改得很扎实,Phase A-F 的拆分也比之前的 v1/v1.1 划分更清楚。
这轮聚焦当前版本仍存在的问题,共 10 条,聚成三类根因:(1) URL 归一化把「identity」和「存储/可点击 URL」混为一谈(丢 fragment + 排序 query 既造成去重误判/数据丢失,也会让点击跳到错误页);(2) artifact 采集被「工具成功」门控,且 batch 通道在 daemon 主会话上是死路(失败路径与 batch hook 产物静默丢失);(3) 事件契约不闭环(eviction 的 removed、status 惰性翻转都到不了 SSE 客户端)。前 8 条是正确性/可实施性问题,后 2 条是约定/简洁性。逐条见 inline。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
设计文档整体结构清晰、覆盖面广。以下是几个需要关注的问题：
- Critical:
SessionArtifactInput类型在 §5.3 被多次引用但全文未定义；dedup 的 last-write-wins 不将source纳入 identity，存在跨来源元数据覆写风险 - Suggestion:
title/description缺少长度和字符约束；ArtifactChangedResult返回类型未定义；Phase C 重复编号
— qwen3.7-max via Qwen Code /review
|
Follow-up update for the latest design audit. Addressed in commit
All currently visible review threads have been individually replied to and resolved; unresolved review thread count is now 0. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
针对最新 head (31af481) 的三轮审计。上一轮的 10 条(URL identity 拆分、batch 死通道、失败路径 hook、provenance、POST changes[]、status 翻转发事件、裁剪策略、managed stat、workspaceCwd、预留字段)都已修复,改得很彻底。
这轮聚焦本次改动引入的一致性/边界问题,共 10 条、去重后 8 类,集中在新的「source 纳入 identity」模型、updatedAt LRU、GET 副作用、changes[]/事件契约上。需要特别说明:其中【裁剪策略】和【GET 发事件】两条,是上一轮我的建议引入的反向 failure mode——属于真实的设计张力,我在对应 inline 里给了更平衡的解法,而非简单翻案。 逐条见下。
|
Follow-up update for the newest audit round. Addressed in commit
All currently visible review threads have been replied to and resolved; unresolved review thread count is now 0. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
针对最新 head (1027016) 的四轮审计。上一轮 8 条全部修复,且解法很漂亮——identity 移除 source、改用 provenance[] + 信任级别 + retention class,把「跨源重复 / 覆盖 / 审计」三件事一次理清。
本轮聚焦这个新引入的 provenance / 信任级别 / retention 子系统自身的边界,共发现 9 处 spec 未闭合(8 CONFIRMED + 1 cleanup),都在 §7。
一个元层面的观察:这个信任子系统是为修上一轮而加的,功能上更强,但它本身正在产生一条边界 case 的长尾(下面 9 条大多源于它)。其中最根因的是【信任级别无法从 source 推导】——DaemonSessionArtifactSource 只有 tool|hook|client 三值,而信任级别有四级(ArtifactTool 与 record_artifact 都是 source='tool'),没有 toolName→level 映射,整个覆盖/retention 规则在运行时无法落地。建议要么补齐这套映射 + 边界规则,要么考虑 V1 用更简的策略(identity 合并 + keep-first-writer + 简单 quota),把完整信任分级推到后续。逐条见 inline。
设计文档审计总账（4 轮）对 ✅ 已修复（27 项）首版已自查（6）： 第 1 轮（3）： 第 2 轮（10）：URL identity/可点击 URL 拆分（保留 fragment+query 顺序）；batch hook daemon 主会话死通道 → 明确 V1 不声明；失败路径 hook 被吞 → 独立收集；provenance last-write-wins → 冻结；POST 单值返回 → 第 3 轮（8）：source 纳入 identity 过度修正 → 移除、改 ⏳ 当前待决（9 项，head
|
| # | 类别 | 问题 | 建议 |
|---|---|---|---|
| 1 | 根因/可实施 | 信任 4 级无法从 3 值 source 推导（ArtifactTool 与 record_artifact 都是 source='tool'） |
补 toolName/storage→level 映射 |
| 2 | 数据丢失 | 200 上限被高信任占满时，新低信任 artifact 无驱逐目标、静默丢弃 | 定义拒绝事件 / 低信任配额 |
| 3 | 正确性 | lastSeenAt=max(provenance) 被低信任 upsert 抬高 → 同 class 内挤掉真高信任项 |
排序时间戳限本 class 来源 |
| 4 | 效率 | provenance[] 无界增长（key 含 per-call toolCallId） |
设上限 / toolCallId 移出 key |
| 5 | 安全 | metadata 低信任可注入新 key；合并后大小不再受 4KB 限制 | 禁注入 + 合并后重校验大小 |
| 6 | 一致性 | 展示字段「同信任级别、不同 key」未定义，且与 line 861 canonical 冲突 | 保留首次写入者 |
| 7 | 可实施 | managedId identity 无归一化规则（不同于 workspacePath/url） |
补 trim/大小写规则 |
| 8 | cleanup | 顶层 canonical 字段与 provenance[] 重复存储，易漂移 |
序列化时派生 |
（第 9 项为上表 #5 拆出的「合并后超 4KB」子项，已并入 #5。）
🔀 一个 V1 复杂度的设计取舍
待决 #1 引出一个根因层面的问题：为闭合上一轮而引入的信任级别 + retention class 子系统，本身正在产生一条边界 case 的长尾（本轮 9 条多源于它）。两条路径供权衡：
- A. 继续硬化：补全 level 映射、
provenance[]上限、metadata 重校验、starvation 处理等 —— 可闭合，但 V1 复杂度持续上升。 - B. V1 简化：identity 合并 + keep-first-writer + 简单 per-source quota，把完整四级信任分级推到后续 PR —— 边界面显著缩小，更贴合 AGENTS.md 的 Simplicity First。
建议倾向 B：当前待决项多为该子系统的长尾，对 V1 而言收益与复杂度不太成正比；但若产品确有跨来源信任分级的近期需求，则按 A 把规则补齐即可。
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
Deterministic analysis: TypeScript + ESLint clean (0 findings). All 354 tests pass.
Findings: 3 critical, 7 suggestions
Critical
-
Metadata injection into trusted published artifacts —
mergeArtifactalways callsmergeMetadataregardless of source. A client POST with a URL matching an existing published artifact's identity can inject new metadata keys while the artifact'ssourceremains'tool'. -
getInitialWorkspaceStatusswallows filesystem errors — Thecatchblock discards the original error (EACCES, EIO, ETIMEDOUT) and throws a generic message. The underlying cause is neither logged nor attached to the error. -
Published upgrade leaves stale Map key and incorrect public id — After a workspace → published upgrade, the stored artifact's
idis never recomputed. The Map entry stays keyed by the old workspace-derived id, causing every subsequent upsert to miss theMap.get()fast path and forcing the O(n) linear scan.
Suggestion
refreshWorkspaceStatusignoresonErrorparameter — catch block always preserves old statushandleArtifactEventdoesn't checkinFlightRestoreIds— hook artifacts dropped during session restore- Eviction
sourceCountsincludes newly-created artifacts — inflates per-source counts above reservation thresholds - Stored URLs preserve single quotes — XSS risk if rendered in single-quoted HTML attributes
- Duplicated validation logic between
record-artifact.tsandsessionArtifacts.ts— future divergence risk mergeArtifactallocates two throwaway objects per upsert for change detection- Eviction loop O(n²) via redundant linear scans
|
@qwen-code /triage |
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. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Overall
Well-designed, well-bounded v1 implementation. Security checks (URL validation, workspace path containment, published storage permission, client field allowlist) are solid. The enqueue serialization, eviction strategy, and identity-based upsert logic are all correct. Test coverage is thorough.
Two items to fix, one design suggestion for future.
Issues
1. Copyright header mismatch in record-artifact.ts
packages/core/src/tools/record-artifact.ts has:
Copyright 2025 Google LLC
While other new files in this PR (e.g. packages/acp-bridge/src/sessionArtifacts.ts) correctly use:
Copyright 2026 Qwen Team
If this file is newly authored for the Qwen project (not carried over from upstream), the header should be unified.
2. Spurious whitespace diff in config.ts
Line 1481 changes */ → */ (one less space). This is a no-op that pollutes git blame. Consider reverting.
Non-blocking suggestion
Type literal drift risk
ToolArtifactKind (core), DaemonSessionArtifactKind (acp-bridge), and KnownDaemonSessionArtifactKind (SDK) are three independent string literal unions that must stay in sync manually. Consider defining a single const tuple in core as the source of truth in a future iteration, so adding a new kind (e.g. dataset) cannot accidentally miss one package.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
LGTM — well-designed, well-bounded v1 implementation.
Strengths:
- Security checks are thorough: URL validation (no javascript:/data: schemes), workspace path containment with symlink escape detection, published storage restricted to trusted publishers, client field allowlist via explicit copy.
enqueueserialization handles errors correctly — rejected promises don't block the queue, errors propagate to callers.- Eviction strategy with source-based soft reservations (tool:100, client:50, hook:50) is fair and won't starve any source.
- Identity-based stable IDs (SHA-256 of session + identity key) give correct upsert semantics.
- Test coverage is comprehensive (~2800 lines across store, bridge client, record-artifact, SDK).
Minor nits (non-blocking):
packages/core/src/tools/record-artifact.tscopyright header says "2025 Google LLC" while other new files use "2026 Qwen Team" — worth unifying if this is new Qwen-authored code.config.tsL1481 has a whitespace-only diff (*/→*/) that will show up in git blame.
Future suggestion:
The three independent kind/storage string literal unions (core ToolArtifactKind, bridge DaemonSessionArtifactKind, SDK KnownDaemonSessionArtifactKind) could be consolidated into a single source-of-truth const tuple in core to prevent drift when new kinds are added.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review in progress — inline comments incoming.
|
|
||
| app.patch( | ||
| '/session/:id/metadata', | ||
| mutate({ strict: true }), |
| app.patch( | ||
| '/session/:id/metadata', | ||
| mutate({ strict: true }), | ||
| withMutableSession('PATCH /session/:id/metadata', (req, res, sessionId) => { |
| '/session/:id/metadata', | ||
| mutate({ strict: true }), | ||
| withMutableSession('PATCH /session/:id/metadata', (req, res, sessionId) => { | ||
| const body = safeBody(req); |
| mutate({ strict: true }), | ||
| withMutableSession('PATCH /session/:id/metadata', (req, res, sessionId) => { | ||
| const body = safeBody(req); | ||
| const clientId = parseClientIdHeader(req, res); |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
/review — PR #5895 feat(daemon): add session artifact APIs
61 files · +9,493/−89 · 9 parallel review agents · 15 findings (13 verified high-confidence)
Deterministic checks: tsc ✓ · eslint ✓ · 2,270 tests pass
Critical (5)
- C1 XSS:
hasUnsafeDisplayPayloadregex missesdata:application/xhtml+xml - C2
SOURCE_RESERVATIONShardcoded{tool:100,client:50,hook:50}don't scale withmaxArtifacts - C3
inputBatchLimit()=maxArtifacts * 2allows single batch to evict ALL pre-existing artifacts - C4 Dropped artifacts silently lost in non-strict mode (only
writeStderrLine, no structured event) - C5 Security-critical validation duplicated byte-for-byte across
record-artifact.tsandsessionArtifacts.ts
Suggestions (8)
- S1
mergeArtifactdiscards title/description for non-published merges - S2
remove()silently returns empty changes on ownership mismatch - S3
PATCH /session/:id/metadatagainedmutate({strict:true})— potential breaking change - S4
list()blocks onrefreshWorkspaceStatuses()fs I/O - S5 Sequential fs I/O during batch normalization
- S6 SSE
session_updatepublished before artifact upsert completes - S7
file:URLs accepted for trusted publishers with no workspace containment - S8 Symlink escape detection creates ghost artifacts (deletes
workspacePathbut keeps artifact in store)
— qwen3.7-max via Qwen Code /review
|
|
||
| function hasUnsafeDisplayPayload(value: string): boolean { | ||
| return ( | ||
| /<\s*\/?[a-z!]|&(?:#[0-9]+|#x[0-9a-f]+|[a-z][a-z0-9]+);|javascript\s*:|data\s*:\s*(?:text\/(?:html|javascript)|application\/javascript|image\/svg\+xml)/i.test( |
There was a problem hiding this comment.
[Critical · Security] hasUnsafeDisplayPayload regex misses data:application/xhtml+xml.
The current regex blocks data:text/html, data:text/javascript, data:application/javascript, and data:image/svg+xml — but data:application/xhtml+xml is not matched. XHTML data URIs can execute JavaScript and are rendered by browsers, making this an XSS vector for artifact display fields (title, description) that pass through isDisplayField validation.
Add application\/xhtml\+xml to the alternation:
| /<\s*\/?[a-z!]|&(?:#[0-9]+|#x[0-9a-f]+|[a-z][a-z0-9]+);|javascript\s*:|data\s*:\s*(?:text\/(?:html|javascript)|application\/javascript|image\/svg\+xml)/i.test( | |
| /<\s*\/\?[a-z!]|&(?:#[0-9]+|#x[0-9a-f]+|[a-z][a-z0-9]+);|javascript\s*:|data\s*:\s*(?:text\/(?:html|javascript)|application\/(?:javascript|xhtml\+xml)|image\/svg\+xml)/i.test( |
— qwen3.7-max via Qwen Code /review
|
|
||
| export type DaemonSessionArtifactStatus = 'available' | 'missing'; | ||
|
|
||
| const SOURCE_RESERVATIONS: Record<DaemonSessionArtifactSource, number> = { |
There was a problem hiding this comment.
[Critical · Correctness] SOURCE_RESERVATIONS is hardcoded as {tool:100, client:50, hook:50} (sum=200) regardless of maxArtifacts.
If maxArtifacts is set to 50 (below default 200), the reservations alone consume 4× the total capacity, effectively starving other sources. Conversely, if maxArtifacts is 1000, reservations cap each source at 5-10% of capacity — likely too conservative.
Consider scaling reservations proportionally:
const base = this.maxArtifacts;
const reservations = {
tool: Math.floor(base * 0.5),
client: Math.floor(base * 0.25),
hook: Math.floor(base * 0.25),
};— qwen3.7-max via Qwen Code /review
| this.maxArtifacts = options.maxArtifacts ?? 200; | ||
| } | ||
|
|
||
| inputBatchLimit(): number { |
There was a problem hiding this comment.
[Critical · Correctness] inputBatchLimit() returns maxArtifacts * 2 (default 400), but evictOverflow excludes createdInThisBatch from eviction candidates.
This means a batch of 400 incoming artifacts can cause eviction of ALL 200 pre-existing artifacts, since only pre-existing artifacts are candidates for eviction and the store overflows by 200. The batch effectively nuked the entire prior state.
Consider capping the input batch at maxArtifacts (not 2×), or including newly-created artifacts in eviction candidates when the overflow exceeds the batch size.
— qwen3.7-max via Qwen Code /review
| } | ||
| const message = | ||
| error instanceof Error ? error.message : String(error); | ||
| writeStderrLine( |
There was a problem hiding this comment.
[Critical · Correctness] In non-strict mode, dropped artifacts are only logged via writeStderrLine. The caller receives no structured signal that their artifact was rejected.
Clients submitting artifacts via POST /session/:id/artifacts will see a successful response for the batch, but individual artifacts that failed validation are silently lost. The changes array in the response won't include them.
Consider including a dropped array in the response, or emitting an artifact_changed event with change: 'dropped' so callers can track rejected artifacts.
— qwen3.7-max via Qwen Code /review
| ); | ||
| } | ||
|
|
||
| function hasControlCharacter( |
There was a problem hiding this comment.
[Critical · Code Quality] hasControlCharacter, hasUnsafeDisplayPayload, and isDisplayField are duplicated byte-for-byte between this file and packages/acp-bridge/src/sessionArtifacts.ts.
This is security-critical validation (XSS filtering, control character detection). If a vulnerability is found in one copy and fixed, the other copy may be missed — creating a drift risk where one entry point is patched but the other remains exploitable.
Extract these functions into a shared module (e.g., packages/core/src/tools/artifact/validation.ts) that both files import. This ensures security fixes propagate automatically.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| async list(): Promise<SessionArtifactsEnvelope> { | ||
| return this.enqueue(async () => { |
There was a problem hiding this comment.
[Suggestion · Performance] list() calls await this.refreshWorkspaceStatuses() on every invocation, which performs sequential fs stat calls for artifacts with stale lastStatAt timestamps.
For sessions with many file artifacts, this adds latency to every GET /session/:id/artifacts call. Consider:
- Lazy refresh — only stat artifacts that the client actually displays
- Background refresh — decouple the stat refresh from the list response
- Cache TTL extension — increase
WORKSPACE_STATUS_REFRESH_TTL_MSfor stable workspaces
— qwen3.7-max via Qwen Code /review
| ): Promise<SessionArtifactMutationResult> { | ||
| return this.enqueue(async () => { | ||
| const normalizedResults: NormalizedArtifact[] = []; | ||
| for (const input of inputs) { |
There was a problem hiding this comment.
[Suggestion · Performance] normalizeForStore is awaited sequentially in the batch loop. Each artifact goes through normalizeForStore which may involve fs.access / fs.stat calls for workspace path validation.
Consider parallelizing with Promise.all or Promise.allSettled:
const normalizedResults = await Promise.allSettled(
input.map((raw, idx) =>
this.normalizeForStore(raw, options.sources[idx] ?? 'tool', options),
),
);This can significantly reduce latency for large batches with many file artifacts.
— qwen3.7-max via Qwen Code /review
| trustedPublisher: isTrustedArtifactToolUpdate(params, updateMeta), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion · Correctness] The SSE session_update event is published (line ~644) before upsertAndPublishArtifacts completes (this line). Clients consuming artifact_changed events may see the session_update referencing artifacts that haven't been indexed yet.
Consider awaiting upsertAndPublishArtifacts before publishing the session_update, or emitting artifact events inline within the upsert so ordering is preserved.
— qwen3.7-max via Qwen Code /review
| if ( | ||
| parsed.protocol !== 'http:' && | ||
| parsed.protocol !== 'https:' && | ||
| !(allowFile && parsed.protocol === 'file:') |
There was a problem hiding this comment.
[Suggestion · Security] normalizeArtifactUrl accepts file: URLs when allowFile is true (trusted publishers). However, there's no workspace containment check on file: URLs — a trusted publisher could reference /etc/passwd or any file on the system.
Consider validating that file: URLs resolve to paths within the workspace:
if (allowFile && parsed.protocol === 'file:') {
const filePath = fileURLToPath(parsed);
if (!filePath.startsWith(workspaceCwd)) {
throw new SessionArtifactValidationError('file: URL must stay inside workspace', 'url');
}
}— qwen3.7-max via Qwen Code /review
| if (status.escaped) { | ||
| artifact.status = 'missing'; | ||
| artifact.sizeBytes = undefined; | ||
| delete artifact.workspacePath; |
There was a problem hiding this comment.
[Suggestion · Correctness] When symlink escape is detected, workspacePath is deleted and status is set to 'missing', but the artifact remains in the store. This creates a "ghost" artifact — it appears in list() responses but has no path, no size, and no way to resolve its content.
Consider either:
- Removing the artifact from the store entirely and emitting a
removedchange event - Adding a distinct status like
'escaped'so clients can differentiate from genuinely missing files - Documenting this as intentional behavior
— qwen3.7-max via Qwen Code /review
|
|
||
| export type DaemonSessionArtifactStatus = 'available' | 'missing'; | ||
|
|
||
| const SOURCE_RESERVATIONS: Record<DaemonSessionArtifactSource, number> = { |
|
Qwen Code review did not complete successfully. Qwen review exited with status 1. See workflow logs. |
9 participants
What this PR does
This PR implements daemon session artifacts as a complete first usable capability. Agents and tools can now attach structured artifact metadata to tool results, hooks can surface additional artifacts after tool execution, daemon clients can list/add/remove session artifacts through HTTP and ACP bridge methods, and clients can subscribe to artifact change events for right-side artifact panels.
The implementation treats links as first-class artifacts. Generated business-resource URLs, published HTML pages, workspace files, images, videos, and other session outputs share the same envelope, with explicit kind, storage, source, status, locator, title, optional description, metadata, retention hint, and timestamps.
Custom artifacts are supported through two paths: model/tool-side registration via a built-in record-artifact tool, and client-side insertion through the session artifact add API. Published artifacts remain restricted to trusted internal publishers, so arbitrary clients and hooks can register external links or workspace resources but cannot claim published storage.
The daemon bridge now keeps a per-session artifact store with validation, stable identity-based upsert, idempotent remove, workspace path containment checks, URL safety checks, missing-file refresh, and a bounded in-memory retention policy. The TypeScript SDK exposes typed list/add/remove helpers plus the artifact_changed event so daemon consumers do not need to hand-roll vendor calls.
Why it's needed
Daemon clients need a stable way to display intermediate session products while an agent is working: files it wrote, links it generated, published previews, images, videos, and business system resource URLs that users can click immediately. Without a structured artifact channel, clients have to scrape text or infer outputs from tool calls, which is fragile and makes custom non-file artifacts awkward.
This keeps the first release intentionally bounded: it is session-scoped, in-memory, append/update/remove capable, evented, SDK-accessible, and safe enough for trial usage. Future persistence, pagination, richer preview metadata, and additional artifact kinds can be added without changing the existing v1 envelope or current endpoints.
Reviewer Test Plan
How to verify
Confirm that a successful artifact-producing tool result publishes artifact metadata to daemon clients, that GET /session/:id/artifacts returns the current envelope, that POST /session/:id/artifacts can insert a client-owned external URL artifact, that DELETE /session/:id/artifacts/:artifactId is idempotent, and that artifact_changed events are emitted for created/updated/removed artifacts.
Confirm that untrusted clients cannot register published artifacts, unsafe URLs and workspace path traversal are rejected, duplicate logical artifacts are updated rather than duplicated, and generated link artifacts appear in the same list as file artifacts.
Evidence (Before & After)
N/A for visual evidence. This is a daemon/API/SDK capability; behavior is covered by focused unit tests plus build and lint.
Tested on
Environment (optional)
Local Node/npm workspace on macOS.
Verified locally:
Risk & Scope
Linked Issues
N/A
中文说明
What this PR does
本 PR 将 daemon session artifacts 从设计推进为一套可试用的完整能力。agent 和 tool 可以在 tool result 上携带结构化 artifact 元数据，hook 可以在工具执行后补充 artifact，daemon client 可以通过 HTTP 和 ACP bridge 方法 list/add/remove session artifacts，并通过 artifact_changed 事件驱动右侧产物面板更新。
本实现把 link 作为一等 artifact。模型按规则拼出的业务资源 URL、发布后的 HTML 页面、工作区文件、图片、视频以及其他 session 中间产物都使用同一份 envelope，显式包含 kind、storage、source、status、locator、title、可选 description、metadata、保留提示和时间戳。
自定义 artifact 支持两条路径：模型或工具侧通过内置 record-artifact 工具登记，client 侧通过 session artifact add API 插入。published artifact 只允许可信内部发布方写入，因此普通 client 和 hook 可以登记 external link 或 workspace resource，但不能伪造 published storage。
Daemon bridge 现在维护按 session 隔离的 artifact store，支持校验、基于稳定 identity 的 upsert、幂等删除、workspace path containment 检查、URL 安全检查、缺失文件状态刷新以及有上限的内存保留策略。TypeScript SDK 提供类型化 list/add/remove helper 和 artifact_changed 事件，daemon 消费方不需要手写 vendor call。
Why it's needed
Daemon client 需要稳定机制展示 agent 工作过程中的中间产物：写出的文件、生成的 link、发布预览、图片、视频以及用户可以立即点击的业务系统资源 URL。没有结构化 artifact 通道时，client 只能解析文本或从 tool call 里推断输出，这既脆弱，也让非文件类自定义 artifact 很难接入。
这次首版边界保持克制：session-scoped、内存态、支持新增/更新/删除、有事件、有 SDK、也具备试用所需的安全边界。后续 persistence、pagination、更丰富的 preview metadata 和更多 artifact kind 都可以在当前 v1 envelope 与 endpoint 上继续增量扩展。
Reviewer Test Plan
How to verify
请确认 artifact-producing tool result 成功后 daemon client 能收到 artifact metadata，GET /session/:id/artifacts 能返回当前 envelope，POST /session/:id/artifacts 能插入 client-owned external URL artifact，DELETE /session/:id/artifacts/:artifactId 是幂等的，并且 created/updated/removed 都会产生 artifact_changed 事件。
请确认未受信 client 不能登记 published artifact，不安全 URL 和 workspace path traversal 会被拒绝，重复逻辑 artifact 会更新而不是重复插入，生成的 link artifact 与 file artifact 会出现在同一列表中。
Evidence (Before & After)
N/A。本 PR 是 daemon/API/SDK 能力，没有可视化面板变更；行为通过聚焦单测、build 和 lint 覆盖。
Tested on
Environment (optional)
macOS 本地 Node/npm workspace。
本地验证命令：
Risk & Scope
Linked Issues
N/A
Security and compatibility notes