fix(artifacts): expand recorded directories into per-file artifacts - #9395
Conversation
github-actions
Bot
added
the
review/self-reported
|
Re-run gate notes — the picture changed since the last pass, mostly for the better.
Moving on to code review. 🔍 中文说明Re-run 门禁结论——与上一轮相比情况已有变化，且大多在变好。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code reviewIndependent baseline first: the minimal way to satisfy #9385 is expansion at the store — the single choke point every ingest path shares — per-file kind inference with office documents download-only, a stat-first client guard for legacy dirty records, and the 3-card collapse. The PR matches that baseline. Its one addition is a second expansion site inside the No critical blockers found in a full static pass over the delta. Verified integration points against the base:
Non-blocking notes — several correspond to still-open earlier-round threads that the current code has largely outgrown (worth resolving on the PR):
sequenceDiagram
participant P1 as record_artifact tool
participant P2 as hooks and client POST
participant P3 as SessionArtifactStore
participant P4 as directory walker
participant P5 as Web Shell panel
P1->>P4: walk directory, caps and skips
P4-->>P1: recordable files
P1->>P3: one artifact per file, expansion marker
P2->>P3: raw input, directory path possible
P3->>P4: expand at the choke point
P4-->>P3: one input per file
P3-->>P5: per-file artifact changes
P5->>P5: stat before open or download
Files changed (28 of 28 shown)
Test evidence — the PR's own CI (unattended run; no PR code executed here)CI has settled on this head: every
The one red check on the commit ( Sandboxed verification would settle what this suite cannot: a sponsored 中文说明代码审查：我独立给出的最小方案与 PR 的实现形状一致——在所有入口共享的 store 咽喉处展开、按文件推断 kind、office 文档只走下载、用现有 stat 接口守住客户端、3 卡折叠。它多出的一处（工具内第二个展开点，让模型拿到"已展开目录…"的结果）站得住脚：hooks 与客户端 POST 入口不经过工具，store 本来也必须展开。静态通读整个增量未发现阻塞性缺陷。已对照 base 验证关键集成点：根因修在正确位置（ 非阻塞备注（多条对应仍未关闭的早前评审线程，当前代码已基本解决，建议在 PR 上 resolve）：office 扩展名列表在 core 与 Web Shell 各存一份（客户端 bundle 无法 import core，已加同步注释）； 测试证据（无人值守运行，未执行任何 PR 代码）：该 head 的 CI 已收敛，所有 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review, nothing blocking found in the delta; capped by policy, because a core-touching fix at this size gets a maintainer's decision, not a bot approval. Stepping back: what changed since the last pass is all in this PR's favor. The #9142 stacking is gone — it merged on 2026-08-18 and I verified its merge commit is an ancestor of this head, so the previous "review the delta, merge order undecided" limbo no longer exists. CI settled green on this head across every lane that runs for PR events. And the earlier-round review threads have been largely answered by the code itself: the store-side validation gap, the orphaned overflow warning, the false depth disclosures, the worktree-root edge, and the sibling-prefix grouping bug are all verifiably handled in the current diff. My independent proposal for this problem and the PR's implementation are the same shape, and the one place it goes beyond the minimum — expanding inside the tool as well so the model sees what happened — is justified because hooks and client POST ingest never touch the tool. The root cause is fixed where it lives, the new
⏸️ Deferring to @yiliang114 (who triggered this re-run) and @wenshao (engaged on the related #9142) — this needs a human sign-off on direction and the contract flip, not a code-quality veto: nothing in the current delta looks blocking to me. Once the verify report lands and a maintainer signs off, re-running 中文说明置信度：3/5 —— 审查干净，增量中未发现阻塞项；按政策封顶：触及核心且达到该规模的 fix 应由 maintainer 决策，而不是由 bot 批准。 整体回顾：上一轮之后的变化全部有利于本 PR。#9142 堆叠已消除——它已于 2026-08-18 合入，我已验证其合并提交是本 head 的祖先，此前"审增量、合并顺序未定"的悬置状态不复存在。CI 在该 head 上所有 PR 事件会运行的通道均为绿色。早前评审轮次的线程也大多已被代码本身回答：store 侧校验缺口、无人消费的溢出告警、误报的深度披露、worktree 根目录边界、兄弟路径前缀误配，在当前 diff 中均可验证已处理。 我独立提出的方案与 PR 的实现形状一致，而它超出最小集的一处（工具内也展开、让模型看到结果）是合理的：hooks 与客户端 POST 入口不经过工具。根因修在正确位置，新
⏸️ 转交 @yiliang114（本次 re-run 的触发者）与 @wenshao（参与过相关的 #9142）——需要人类对方向与契约翻转拍板，而非代码质量上的否决：当前增量中没有我认为阻塞的内容。待 verify 报告落地且 maintainer 背书后，重新运行 — Qwen Code · qwen3.8-max Reviewed at |
🩺 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 |
e125cd3 to
d014a39
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #9395 fix(artifacts): expand recorded directories into per-file artifacts
Type: Bug Fix + New Feature (office document kind)
Change size: +994/-87 across 23 files
Findings Summary
- Critical/Major: 0
- Minor: 1 — silent depth-limit truncation
- Nit: 2 — duplicate extension set, redundant type label
Key Observations
This is a well-scoped, well-tested fix. The core logic (collectRecordableWorkspaceFiles, expandWorkspaceDirectoryInput, expandDirectoryLocator) is clean and correctly decomposes at record time rather than read time. The isSameWorkspacePathOrChild fix in turnOutputSelectors.ts and the new TURN_OUTPUT_VISIBLE_LIMIT collapse control for artifact cards are both solid additions. Test coverage is thorough for happy paths, empty-directory rejection, and the file-count cap.
One behaviour gap: the 100-file count cap correctly surfaces a truncated warning, but the depth-4 cap silently drops files from over-deep subtrees — returning false ("not truncated") even though content was omitted. The two nits are minor maintainability items.
Cross-Validation
No prior reviews on this PR — independent analysis only.
| Finding | Other Reviewer | My Assessment |
|---|---|---|
| Unique-1: silent depth truncation (workspace-artifact-directory.ts:76) | — | New finding |
| Unique-2: duplicate OFFICE_DOCUMENT_EXTENSIONS (artifactUtils.ts:49) | — | Nit |
| Unique-3: type label rendered twice (ArtifactPanel.tsx:2548+2553) | — | Nit |
Additional Audit Coverage
- Handler parallelism:
expandWorkspaceDirectoryInputandexpandDirectoryLocatorboth delegate to the samecollectRecordableWorkspaceFiles— no drift between the two call sites. - Data structure blast radius:
'document'kind added consistently tonormalizeKind,inferKind,isArtifactKind, and JSON schema;write-file.tsmap andARTIFACT_FORMAT_ICONSare updated too. - i18n completeness:
common.download/common.downloadingalready exist; the two newturnOutputs.*keys are correctly added to both EN and ZH. - Path safety:
isOutsidePath+lstat+isWithinRootchain in both call paths provides adequate traversal defence;path.relativenormalises before the escape check. - Backward compatibility: the optional
workspacePathparameter onartifactKindLabelis backward-compatible; existing callers without it default to extension-agnostic behaviour. inferKindextension casing:path.extname(...).toLowerCase()is applied beforeisOfficeDocumentExtension, so the backend correctly handles uppercase extensions (.XLSXetc.).getWorkspaceStatusrefactor: the oldif (stat.isFile())guard implicitly returned{ status: 'available' }(no sizeBytes/mtimeMs) for non-file paths; the new early-throw is a correct clean-up.
Final Verdict
Ready to merge once the depth-truncation behaviour is addressed (or explicitly decided to be acceptable) — the rest of the change is high quality.
This review was generated by QoderWork AI
| } | ||
| } | ||
|
|
||
| const OFFICE_DOCUMENT_EXTENSIONS = new Set([ |
There was a problem hiding this comment.
[Nit] OFFICE_DOCUMENT_EXTENSIONS is defined identically in both artifactUtils.ts and workspace-artifact-directory.ts (core)
The two sets are currently in sync, but they live in different packages with no compile-time link between them. If a new extension is added to one (e.g. .docb, .numbers), a developer may forget to update the other, causing the backend to record the file as kind: 'file' while the frontend labels it 'Word' (or vice versa).
A short comment in each file pointing to the other is the minimum fix:
// NOTE: keep in sync with OFFICE_DOCUMENT_EXTENSIONS in
// packages/core/src/utils/workspace-artifact-directory.ts
const OFFICE_DOCUMENT_EXTENSIONS = new Set([...]);Longer-term, exporting a shared constant from a package both sides depend on (e.g. @qwen-code/sdk) would remove the duplication entirely.
This review was generated by QoderWork AI
| <div className={styles.fieldGrid}> | ||
| <Field | ||
| label="Type" | ||
| value={artifactKindLabel(artifact.kind, artifact.workspacePath)} |
There was a problem hiding this comment.
[Nit] artifactKindLabel is called twice for the same value in DownloadableWorkspaceArtifact
Line 2548 renders the kind label as the sectionTitle, then line 2553 renders it again as <Field label="Type" value={...} />. The two renders are identical, so the panel shows the type twice.
You could drop the <Field label="Type"> row (the title already serves as the type header) or replace the sectionTitle with a fixed string like 'Download' / t('common.download') to keep both rows meaningful.
This review was generated by QoderWork AI
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (depth > MAX_DIRECTORY_ARTIFACT_DEPTH) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The depth cap silently omits recordable files — Concrete cost: the walk returns false (not truncated) for entries beyond MAX_DIRECTORY_ARTIFACT_DEPTH, so neither the tool message nor the store warnings disclose depth-skipped files: recording a tree whose report files sit deeper than 4 levels below the recorded root silently excludes them, the tool result lists what was recorded with no hint of omission, and a tree with only deep files reports a directory with no recordable files. The PR's Risk section and design doc disclose only the 100-file cap.
Witness:
Probe: tree with only exports/2026/08/18/region/store/report.xlsx (deep) + exports/shallow.xlsx
→ {"files":["exports/shallow.xlsx"],"truncated":false}
Boundary: depth-4 file collected, depth-5 dropped
Propagate a truncation signal when the depth limit cuts the walk off (e.g. a depthLimited flag alongside truncated) and disclose it in the tool message and store warnings.
中文说明
[Suggestion] 深度上限会静默漏掉可记录的文件 — 具体代价：对超过 MAX_DIRECTORY_ARTIFACT_DEPTH 的条目，遍历直接返回 false（而不是 truncated），因此工具消息和 store 告警都不会披露因深度被跳过的文件：当目录中的报告文件位于记录根目录下超过 4 层时会被静默排除；工具结果只列出已记录的文件、毫无遗漏提示；只有深层文件的目录会被报成 a directory with no recordable files。PR 的风险说明和设计文档只披露了 100 文件上限。建议：当深度上限截断遍历时传播一个截断信号（例如在 truncated 之外增加 depthLimited 标志），并在工具消息和 store 告警中披露。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (!stat.isFile()) { | ||
| throw new Error('path is not a regular file'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Test coverage pattern — changed behavior with no test (location 2 of 6): this store-side refusal of legacy dirty directory records is untested — Concrete cost: the new empty-dir test short-circuits earlier in expandWorkspaceDirectoryInput and never reaches this throw, which is reachable via a symlinked-directory input or a persisted directory record being restored. If it regresses to the old { status: 'available' } fallback, a legacy directory record restores as available again — the exact dirty state this PR says it refuses — and clients retry open/download against a directory; no test goes red.
Witness:
Probe flip check: with the pre-PR fallback restored, a persisted directory record and a symlink->dir input
→ {"artifactCount":1,"statuses":["available"],"warnings":[]} (dirty state restored silently)
Add a store test seeding/restoring a record whose workspacePath is a non-empty directory and assert rejection rather than available.
中文说明
[Suggestion] 测试覆盖模式 — 行为已改但无测试（6 处之 2）：这个「store 端拒绝历史脏目录记录」的改动没有测试 — 具体代价：新增的空目录测试在 expandWorkspaceDirectoryInput 更早处就短路，永远到不了这个 throw；而该 throw 可通过符号链接目录输入或恢复已持久化的目录记录到达。若它回归为旧的 { status: 'available' } 回退，历史目录记录会再次以 available 恢复 —— 正是本 PR 声称要拒绝的脏状态 —— 客户端会再次对目录尝试打开/下载；没有测试会变红。建议：增加一个 store 测试，构造/恢复 workspacePath 为非空目录的记录，断言被拒绝而非 available。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| export function shouldSkipDirectoryArtifactName(name: string): boolean { | ||
| return ( | ||
| name.startsWith('.') || | ||
| name.startsWith('~$') || | ||
| SKIP_DIRECTORY_ARTIFACT_NAMES.has(name) |
There was a problem hiding this comment.
[Suggestion] Test coverage pattern — changed behavior with no test (location 3 of 6): the junk-directory skip set, the depth cap, and the symlink-entry skip have no fixture coverage anywhere — Concrete cost: this file has no test file at all; the existing expansion fixtures cover hidden/~$ skips and depth-1/2 nesting only. If the skip set (node_modules, .git, __pycache__, dist, .qwen), the depth check, or the symlink-entry skip regresses, expansion records up to 100 junk files (wrong files occupying the cap) or walks unbounded depth, and no test goes red in either test suite.
Extend one expansion fixture with node_modules/junk.txt, a file at depth > 5, and a symlink entry, asserting none are recorded.
中文说明
[Suggestion] 测试覆盖模式 — 行为已改但无测试（6 处之 3）：垃圾目录跳过集合、深度上限、符号链接条目跳过在任何地方都没有 fixture 覆盖 — 具体代价：本文件完全没有对应的测试文件；现有展开 fixture 只覆盖隐藏文件/~$ 跳过和 1-2 层嵌套。若跳过集合（node_modules、.git、__pycache__、dist、.qwen）、深度检查或符号链接条目跳过发生回归，展开会记录最多 100 个垃圾文件（错误的文件占满上限）或无深度限制地遍历，两个测试套件都不会变红。建议：在某个展开 fixture 中加入 node_modules/junk.txt、一个深度 > 5 的文件和一个符号链接条目，断言它们都不被记录。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| 'audio', | ||
| 'pdf', | ||
| 'notebook', | ||
| 'document', | ||
| 'other', | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] Test coverage pattern — changed behavior with no test (location 4 of 6): persistence restore accepting the new 'document' kind is untested (no test references kind: 'document'; the persistence fixtures use only kind: 'link') — Concrete cost: normalizeLiteral returns undefined for disallowed values and the guard below drops the WHOLE artifact with skipped malformed artifact; if 'document' regresses out of this allowlist, every persisted document artifact fails to restore after a daemon restart — worse than just losing its kind — and no roundtrip test goes red.
Add a restore/normalize case asserting a persisted artifact with kind: 'document' roundtrips with its kind intact.
中文说明
[Suggestion] 测试覆盖模式 — 行为已改但无测试（6 处之 4）：持久化恢复接受新 'document' kind 的路径没有测试（没有任何测试引用 kind: 'document'；持久化 fixture 只用 kind: 'link'）— 具体代价：normalizeLiteral 对不在白名单的值返回 undefined，下方守卫会以 skipped malformed artifact 丢弃整个产物；若 'document' 从这个白名单回归掉，所有已持久化的 document 产物在 daemon 重启后都会恢复失败 —— 比单纯丢失 kind 更严重 —— 且没有往返测试会变红。建议：增加一个恢复/规范化用例，断言 kind: 'document' 的持久化产物往返后 kind 保持不变。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (isDownloadOnlyWorkspaceArtifact(artifact)) { | ||
| return ( | ||
| <DownloadableWorkspaceArtifact |
There was a problem hiding this comment.
[Suggestion] Test coverage pattern — changed behavior with no test (location 5 of 6): the routing of document/pdf/video/audio artifacts to the new download-only view is only tested at the predicate level — Concrete cost: no component test asserts the routing, the Download button, or that clicking it calls downloadWorkspaceFile; if the isDownloadOnlyWorkspaceArtifact check regresses or moves after the generic WorkspaceFilePreview branch, an .xlsx artifact falls back to reading a binary as text — the original defect this PR fixes — and no test goes red.
Add an ArtifactPanel test rendering a kind: 'document' workspace artifact and asserting the download button renders (and readWorkspaceFile is not called for preview).
中文说明
[Suggestion] 测试覆盖模式 — 行为已改但无测试（6 处之 5）：document/pdf/video/audio 产物路由到新的「仅下载」视图只在谓词层面有测试 — 具体代价：没有组件级测试断言该路由、Download 按钮、或点击后会调用 downloadWorkspaceFile；若 isDownloadOnlyWorkspaceArtifact 检查回归或被移到通用 WorkspaceFilePreview 分支之后，.xlsx 产物会回退到把二进制当文本读取 —— 正是本 PR 要修的原始缺陷 —— 且没有测试会变红。建议：增加一个 ArtifactPanel 测试，渲染 kind: 'document' 的工作区产物，断言出现下载按钮（且预览未调用 readWorkspaceFile）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (stat.type === 'directory') { | ||
| throw new Error('Directories cannot be opened as artifacts.'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Test coverage pattern — changed behavior with no test (location 6 of 6): the stat-first directory gate in useWorkspaceFileContent is untested — Concrete cost: the only added ArtifactPanel.test.tsx mock resolves type: 'file' to keep an existing test green; the directory rejection is tested only for the blob-download utility, not this hook. If the gate regresses (e.g. the stat call is removed), the preview issues readWorkspaceFile against a directory and surfaces a raw backend error instead of the friendly message for legacy directory records; no test goes red.
Add a panel test where the stat mock resolves { type: 'directory' } and assert the error is shown and readWorkspaceFile is never called.
中文说明
[Suggestion] 测试覆盖模式 — 行为已改但无测试（6 处之 6）：useWorkspaceFileContent 中「先 stat 再读取」的目录门禁没有测试 — 具体代价：ArtifactPanel.test.tsx 新增的唯一 stat mock 解析为 type: 'file'（仅为让既有测试通过）；目录拒绝只在 blob 下载工具函数上测过，未覆盖此 hook。若该门禁回归（例如 stat 调用被移除），预览会对目录调用 readWorkspaceFile，历史脏目录记录将看到原始后端错误而非友好提示；没有测试会变红。建议：增加一个面板测试，让 stat mock 解析 { type: 'directory' }，断言显示错误且 readWorkspaceFile 从未被调用。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
A generated folder was stored as one file artifact, so Open and Download hit file APIs on a directory and returned 400. Record each file instead, and treat office documents as downloadable artifacts. Co-authored-by: Cursor <cursoragent@cursor.com>
Report depth truncation, keep later files on their own turn, and stop one bad filename or a full-content hash from breaking directory ingest. Co-authored-by: Cursor <cursoragent@cursor.com>
0d53cb4 to
e54995c
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| options.trustedPublisher === true, | ||
| ), | ||
| ); | ||
| const expanded = await this.expandWorkspaceDirectoryInput(input); |
There was a problem hiding this comment.
[Suggestion] R2-17: The per-request input cap is enforced BEFORE directory expansion, so each capped input can fan out to up to 100 children inside upsertMany — one hook event can drive ~40,000 sequential normalizeInput calls on the serialized store queue — Failure scenario: a hook/tool update carrying up to 400 directory artifacts → ~200k awaited fs round-trips execute serially in enqueue, stalling every list()/get() for the session (the web-shell artifact panel) for the duration; the cap exists to bound exactly this work, but expansion bypasses it by up to 100×.
Witness:
Probe: upsertMany(3 directory inputs) → changes.length=200
warnings: ["dropped 100 newly created artifacts because the store is full"]
store.inputBatchLimit() === 400 (applied pre-expansion)
Suggested fix: enforce the bound on the post-expansion total — pass a remaining-input budget into expandWorkspaceDirectoryInput (or stop collecting once accumulated inputs reach the limit) and surface a batch-limit warning like the pre-expansion cap does.
中文说明
[Suggestion] 每请求输入上限在目录展开「之前」执行，因此每个受限输入最多可在 upsertMany 内展开成 100 个子项 —— 单个 hook 事件可在串行化的 store 队列上触发约 40,000 次顺序 normalizeInput 调用 —— 失败场景：携带最多 400 个目录产物的 hook/工具更新 → 约 20 万次 await 的 fs 往返在 enqueue 中串行执行，期间该会话的所有 list()/get()（web-shell 产物面板）被阻塞；该上限本就是为了限制这类工作而存在，但展开可将其绕过最多 100 倍。
证据见上（probe 输出）。
建议修复： 对展开后的总量设限 —— 把剩余输入预算传入 expandWorkspaceDirectoryInput（或累计输入达到上限即停止收集），并像展开前上限一样给出批量超限告警。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| persistenceStrict, | ||
| ); | ||
| changes.push(...overflowRemoved.removed); | ||
| if (overflowRemoved.droppedCreated > 0) { |
There was a problem hiding this comment.
[Suggestion] R2-19: Test coverage pattern — changed behavior with no test (location 1 of 3): the model-visible overflow warning ("dropped N newly created artifacts because the store is full") has no test — Failure scenario: mutation droppedCreated > 0 → false (or deleting the push) keeps all 115 tests green; callers/models silently lose the only disclosure that newly created artifacts were dropped. Directory expansion makes same-batch overflow realistically reachable in a single call.
Witness: mutation droppedCreated > 0 && false → Tests 115 passed (115); a control probe asserting the warning flips red under the mutation and is green on pristine code.
Suggested fix: extend the same-batch overflow test (drops newest artifacts created in the same batch…) to also assert warnings contains the drop message.
中文说明
[Suggestion] 测试覆盖模式 —— 行为有变更但无测试（第 1/3 处）：模型可见的溢出告警（"dropped N newly created artifacts because the store is full"）没有任何测试 —— 失败场景：变异 droppedCreated > 0 → false（或删除该 push）时全部 115 个测试仍为绿；调用方/模型会静默失去「新建产物被丢弃」的唯一披露。目录展开使同批溢出在单次调用中即可真实触发。
证据：变异 droppedCreated > 0 && false → Tests 115 passed (115)；断言该告警的对照 probe 在变异下变红、在原始代码下为绿。
建议修复： 扩展同批溢出测试，额外断言 warnings 包含该丢弃信息。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| artifact.workspacePath, | ||
| workspaceCwd, | ||
| ) && | ||
| (!artifact.toolCallId || artifact.toolCallId === reference.callId) |
There was a problem hiding this comment.
[Suggestion] R2-6: The directory-child branch treats a MISSING artifact.toolCallId as matching every recorded-directory reference whose path is an ancestor — store-side expansion from client POST and hook ingestion stamp no toolCallId, yet the metadata.expandedFromDirectory marker (stamped by both expansion paths and exposed on DaemonSessionArtifact.metadata) is not consulted — Failure scenario: turn 1 records reports; later a hook/client-POST expansion creates reports/extra/… children without toolCallId → the cards render under turn 1 instead of their creation turn; with reports recorded in two turns the same card appears under BOTH (dedup is per-turn only). The diff's own negative test ("does not attach later artifacts…") stays green only because its fixture is stamped.
Witness:
Probe (real selectors, PR code):
scenarioA (later unstamped child under recorded dir): attachedTurns = ["u1"] (old turn)
scenarioB (dir recorded in two turns): attachedTurns = ["u1","u2"] (duplicate)
Flip (require a matching toolCallId): both → []
Suggested fix: gate the fallback on the expansion marker instead of absence of toolCallId, e.g. (!artifact.toolCallId ? artifact.metadata?.['expandedFromDirectory'] === true : artifact.toolCallId === reference.callId), and update the first test's fixtures to carry the marker both expansion paths actually produce.
中文说明
[Suggestion] 目录子项分支把「缺失 artifact.toolCallId」当作匹配所有路径为祖先的已记录目录引用 —— 客户端 POST 与 hook 摄取的 store 侧展开不打 toolCallId，而两条展开路径都会打、且已暴露在 DaemonSessionArtifact.metadata 上的 metadata.expandedFromDirectory 标记却未被使用 —— 失败场景：第 1 轮记录 reports；之后 hook/客户端 POST 展开在 reports/extra/… 下创建无 toolCallId 的子项 → 卡片渲染在第 1 轮而不是创建它的轮次；若 reports 在两个轮次都被记录，同一张卡片会同时出现在两轮（去重只在单轮内进行）。diff 自带的反向测试（"does not attach later artifacts…"）之所以保持绿，仅因为其 fixture 打了 toolCallId。
证据见上（probe 输出）。
建议修复： 让回退分支以展开标记为门槛，而不是以 toolCallId 缺失为门槛，例如 (!artifact.toolCallId ? artifact.metadata?.['expandedFromDirectory'] === true : artifact.toolCallId === reference.callId)；并更新第一个测试的 fixture，携带两条展开路径实际产生的标记。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const normalizedParent = stripWorkspacePath(parent, workspaceCwd); | ||
| const normalizedChild = stripWorkspacePath(child, workspaceCwd); |
There was a problem hiding this comment.
[Suggestion] R2-4: isSameWorkspacePathOrChild inherits stripWorkspacePath's workspace-root collapse (path === cwd returns the cwd's BASENAME), so a workspace-root recording reference is treated as a recording of a top-level subdirectory named after the workspace's basename — the false-positive twin of the root false-negative in Needs Human Review (filed Critical, downgraded by verification: UI mis-grouping, narrow trigger) — Failure scenario: in a worktree session (where root recording succeeds), a record_artifact of the workspace root normalizes to project (the basename); an unstamped artifact under <root>/project/... — a top-level directory sharing the workspace's basename, e.g. a nested checkout of the same repo — satisfies 'project/report.pdf'.startsWith('project/') and attaches to the root-recording turn; if a genuine recording of that subdirectory also exists, the same artifact duplicates across both turns.
Witness:
Probe (cwd=/home/user/project, root recording in u1, unstamped 'project/report.pdf'):
PR: single recording → {"u1":["artifact-1"]} (attached to root turn)
PR: + genuine 'project' record → {"u1":["artifact-1"],"u2":["artifact-1"]} (duplicate)
FLIP (root → '' instead of basename): {} and {"u2":["artifact-1"]}
Suggested fix: special-case a root parent in isSameWorkspacePathOrChild — when the parent IS the workspace root, match any cwd-relative child instead of comparing against the collapsed basename.
中文说明
[Suggestion] isSameWorkspacePathOrChild 继承了 stripWorkspacePath 的工作区根折叠（path === cwd 时返回 cwd 的「basename」），于是对工作区根的登记引用会被当作对「与工作区同名的顶层子目录」的登记 —— 这是「根目录假阴性」（见 Needs Human Review）的假阳性孪生问题（按 Critical 提交，验证后降级：UI 归组错误，触发条件窄）—— 失败场景：在 worktree 会话中（根登记可成功），对工作区根的 record_artifact 规范化为 project（basename）；位于 <root>/project/... 下的未打 toolCallId 产物 —— 与工作区同名的顶层目录（例如同仓库的嵌套 checkout）—— 满足 'project/report.pdf'.startsWith('project/')，被挂到根登记轮次；若该子目录还存在真实登记，同一产物会在两轮重复出现。
证据见上（probe 输出）。
建议修复： 在 isSameWorkspacePathOrChild 中对根目录父项特判 —— 当父项就是工作区根时，匹配任何 cwd 相对子项，而不是与折叠后的 basename 比较。
— qwen3.8-max via Qwen Code /review (v0.21.13)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent test-matrix": None. All checks completed within the tool budget..
Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent test-matrix"：None. All checks completed within the tool budget.。
未审查：反向审计——没有审计 agent 是用本 skill 构建的 prompt 启动的——负责搜寻评审其余部分遗漏问题的这道工序，即便运行过，也缺失了 brief 承载的方法。
— qwen3.8-max via Qwen Code /review (v0.21.13)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Two-stage review summary for PR #9395
Model: deepseek-v4-flash (round 1)
Head reviewed: e54995ce864ea8b759567281732dd2b669271999
Verdict: ✅ No new Critical issues, 2 Suggestion-level findings remain.
Scope
- 26 files, +1,519 / −103
- Directory expansion for session artifacts,
kind: documentfor Office files, 3-card collapse in the artifacts panel, turn grouping by folder prefix.
Findings
| Dimension | Result |
|---|---|
| Issue Fidelity | PR faithfully implements #9385; root cause (client-side contract mismatch) is fixed. |
| Code-path review (7 chunks) | No new Critical findings. |
| Cross-file tracer | Signatures consistent. |
| Removed-behavior audit | Removed behaviors are intentionally superseded. |
| Build & Test | Affected packages build; the only failure is pre-existing (missing Python for node-gyp in an untouched package). |
| Test Coverage | Adequate, with 2 Suggestion-level coverage gaps noted in the detailed review. |
Previously raised Critical blockers — verified fixed
- Title /
workspacePathvalidation — fixed via per-child try/catch innormalizeInput. - 3-card cap breaking tests — fixed, test updated to drive the show-more toggle.
- Path-prefix hijacking — fixed via status filtering,
toolCallIdgate, and trailing-slash guard.
The detailed review comments (including the Suggestion-level coverage gaps) have been posted inline on this PR.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: the executable-script lint — the report is stale or its diff could not be verified; re-run qwen review script-lint.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：the executable-script lint — the report is stale or its diff could not be verified; re-run qwen review script-lint。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| persistenceStrict, | ||
| ); | ||
| changes.push(...overflowRemoved.removed); | ||
| if (overflowRemoved.droppedCreated > 0) { |
There was a problem hiding this comment.
[Suggestion] R2-3: The new droppedCreated overflow warning has no consumer on the only ingest path where it can fire — bridgeClient.upsertAndPublishArtifacts reads result.changes and discards result.warnings (grep-confirmed). — Concrete cost: directory expansion makes store overflow reachable with 2-3 inputs (a hook event reporting three directory artifacts of ≤100 children each creates up to 300 entries in one upsertMany against the 200-artifact cap). Probe: 3×100 expanded files into an empty store → warnings=['dropped 100 newly created artifacts because the store is full'], created changes stripped, store size 200 — no artifact_changed event reaches clients; the drop is observable only in daemon stderr while the session believes the artifacts exist. Fix: surface ingest warnings — e.g. have upsertAndPublishArtifacts log them via writeStderrLine or publish a warning event.
中文说明
[Suggestion] R2-3：新增的 droppedCreated 溢出告警在唯一可能触发的入口路径上没有消费者 —— bridgeClient.upsertAndPublishArtifacts 只读 result.changes，丢弃 result.warnings（grep 已证实）。— 具体代价：目录展开让 store 溢出只需 2-3 个输入即可触发（一个 hook 事件上报 3 个各 ≤100 子项的目录产物，一次 upsertMany 就产生最多 300 条，对上 200 的容量上限）。探针：空 store 一次写入 3×100 个展开文件 → warnings=['dropped 100 newly created artifacts because the store is full']，created 变更被剥离，store 停在 200 —— 客户端收不到任何 artifact_changed 事件；丢弃只出现在 daemon stderr，而会话以为产物都存在。修复：把入口告警透出 —— 例如让 upsertAndPublishArtifacts 通过 writeStderrLine 记录或发布告警事件。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| >, | ||
| existing: Pick<NormalizedArtifact, 'toolName' | 'source' | 'hookEventName'>, | ||
| ): boolean { | ||
| if (incoming.metadata?.['expandedFromDirectory'] === true) { |
There was a problem hiding this comment.
[Suggestion] R2-4: The expandedFromDirectory display-refresh suppression chain has no test at either end. Location 1 (here, consumer): deleting this guard keeps the full sessionArtifacts suite green (115/115 measured), while re-recording a directory with a new toolCallId then overwrites each child's toolCallId/description — silently moving files out of their original turn. Location 2 (producer, record-artifact.ts:212): no test asserts the flag on the tool's output (repo-wide grep: zero test references to expandedFromDirectory); the new expansion tests use toMatchObject without the metadata key, so dropping or misspelling the flag at the producer ships green and re-opens the same consequence for tool-originated records. Fix: add a store test (record reports/a.xlsx with one toolCallId, upsert the reports directory with a different toolCallId, assert the child keeps its title/description/toolCallId) and assert metadata: { expandedFromDirectory: true } in the existing record-artifact expansion test.
中文说明
[Suggestion] R2-4：expandedFromDirectory 显示刷新抑制链两端都没有测试。位置 1（此处，消费端）：删除该守卫后 sessionArtifacts 全套测试仍为绿（实测 115/115），而用新的 toolCallId 重新记录目录会覆盖每个子项的 toolCallId/description —— 把文件悄悄移出它原来的轮次。位置 2（生产端，record-artifact.ts:212）：没有任何测试断言工具输出上的该标志（全库 grep：测试对 expandedFromDirectory 零引用）；新增的展开测试用的 toMatchObject 不含 metadata 键，因此生产端删掉或写错该标志也能绿灯通过，并对工具来源的记录重新打开同样的后果。修复：补一个 store 测试（先用某个 toolCallId 记录 reports/a.xlsx，再用不同 toolCallId 登记 reports 目录，断言子项保留原 title/description/toolCallId），并在现有 record-artifact 展开测试中断言 metadata: { expandedFromDirectory: true }。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Use the locator's realpath workspace root, map unlistable directories to validation errors, skip junk roots, and keep walk caps/warnings aligned with what was actually recorded. Co-authored-by: Cursor <cursoragent@cursor.com>
Addressed on later commits (through cb316e5). Round 8 re-review on current head posted COMMENTED with 0 findings.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review (AI Generated) — Round 3
PR: #9395 fix(artifacts): expand recorded directories into per-file artifacts
❌ CI Blocker — cannot approve
Live Host (macos-latest) is red (run 32439854331):
error: lockfile had changes, but lockfile is frozen
note: try re-running without --frozen-lockfile and commit the updated lockfile
bun.lockb has uncommitted changes. Run bun install at the repo root, commit the updated lockfile, and push.
⏳ Pending / skipped CI
| Check | Status |
|---|---|
| Test (ubuntu-latest, Node 22.x) | pending |
| Real daemon E2E / Java 11 | pending |
| Integration Tests (CLI, No Sandbox) | skipped — no evidence |
| Test (windows-latest, Node 22.x) | skipped — no evidence |
| Test (macos-latest, Node 22.x) | skipped — no evidence |
The PR author's own test matrix flags Linux and Windows as "not tested". The skipped platform jobs leave those dimensions unreviewed.
What I checked at this head
workspace-artifact-directory.ts:pathHasSkippedDirectoryComponentcorrectly usesstripWorktreeArtifactPrefixbefore the junk-segment check, so worktree-relative paths are handled.inspectWorkspaceCandidate: directory now flows through withisDirectory: true; workspace-root guard fires beforetoCanonicalWorkspaceArtifactPath.expandWorkspaceDirectoryInput(store side): validation order — title/description/metadata — looks correct;normalizeInputruns on each child after expansion.- Existing bot findings R2-5 (metadata-budget overflow after appending
expandedFromDirectorymarker) and R2-8 (files excluded byisRecordableDerivedChildsilently skipped without disclosure) remain open per the bot's round-5+ ledger; I'll defer to the bot's ongoing tracking.
Fix the lockfile and let the pending jobs resolve before merging.
Reviewed with AI assistance.
Qwen Live Host CI runs `bun install --frozen-lockfile` with Bun 1.3.9; the checked-in lockfile drifted and fails on main and any PR that touches Live Host path filters. Regenerate with the CI Bun version so install stays reproducible. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 549 passed · 0 failed · 549 total Flakiness gate: ✅ 10 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：549 通过 · 0 失败 · 549 总计 抖动门：✅ 10 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9395 deep verification — fix(artifacts): expand recorded directories into per-file artifactsVerdict: 中文摘要
Central claim and A/B proofCentral claim: Control arm = head's compiled dist with exactly two reverted hunks reproducing HEAD^1's
Store-side flip (every ingest path shares it). Witness:
Client-side guards (web-shell source, driven mock-free with real fs where a seam exists): 13-case download-only matrix, extension-first labels (Word/Excel/PowerPoint/CSV), Targeted gates (all green, witness Mutation matrix (witnesses Reviewer Test Plan walkthrough: step 1 ✅ (harness1 dir-basic + harness2 S1); step 2 ✅ at the unit level (G1/G2 + ArtifactPanel tests FindingsF1 (low) — the Suggested fix (measured shape, not applied)Regenerate `bun.lock` on a machine whose bun resolves the default registry (or the repo's `.npmrc` registry) so the resolved-URL fields return to `""` as on base; keep the workspace-version and `@qwen-code/live-host` entries that fix the frozen install. I could not execute this in-container (no `bun` binary), so it is unmeasured — flagging for the author instead.F2 (nit) — the same commit silently bumps F3 (nit) — Not covered
MethodologyEnvironment: CI Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
上次阻断项（lockfile 未提交）已修复，所有关键 CI 现在全绿：Test / Live Host / Real daemon E2E / Desktop Shell 均 SUCCESS，mergeable。Approve。
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
bun.lock conflict: main's 07405e6 (QwenLM#9395) regenerated the desktop lockfile properly (npmmirror URLs, @types/bun 1.4.0, live-host entry) — take main's version; this branch's earlier regenerated copy was made with a local bun whose registry config differs from CI.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Final verification pass at 249452 (post-rebase delta + bun.lock refresh). The feature code checks out:
- Walker caps: the 100-file cap warns at record time, and the earlier silent depth-4 truncation is fixed — over-deep directories now set
depthLimitedand surface 'files deeper than 4 levels' via a warning or dedicated error in both the tool and bridge paths. - Turn grouping:
isSameWorkspacePathOrChildrequires a separator boundary, so a recorded/a/bcannot claim/a/b-x; expanded files stay attached to the recording turn viatoolCallId. - Directory records never reach the file-content APIs: expansion happens at record time on both tool and bridge paths, Open/Download stat-refuse directories, and office-kind detection shares one extension set between write-file and bridge inference (csv stays text-previewable).
Security pass is clean: expanded paths are containment-proven via realpath + path.relative/isOutsidePath rather than string prefix, symlinks are skipped via dirent lstat semantics before any descent, non-regular files cannot become artifacts, and the previously flagged unguarded lstat is try/catch-guarded at this head.
I also independently reproduced the F1 lockfile comparison: all 1831 new registry.npmmirror.com URLs are routing-only — the sha512 set differs solely by the @types/bun/bun-types 1.3.14→1.4.0 bump, no content changes.
|
Released in v0.22.0. |
5 participants
What this PR does
When a session records a generated folder as one workspace artifact, clients later try to open or download it as a regular file and get HTTP 400. This change keeps the path verification now on
mainfrom #9142, but a directory is no longer rejected: it is expanded into one artifact per recordable file. Hidden names, Excel lock files, symlinks, and well-known junk directories are skipped. Empty directories still fail. The original folder path is never stored as an openable artifact.Office documents (Word / Excel / PowerPoint and OpenDocument) are a first-class downloadable artifact type. They no longer fall through to a text preview. The same download-only treatment applies to PDF, video, and audio. If a leftover dirty record still points at a directory, Open/Download refuse it after
GET /statinstead of calling the file APIs.Completed-turn artifact cards show at most three items, with the same expand/collapse control already used for edited files. Chat grouping treats the recorded folder path as a prefix, so the expanded files stay on that turn.
Rebased onto current
mainafter #9142 landed. Review this PR as the follow-up delta only.Why it's needed
Models often produce a folder of spreadsheets or reports and register the folder itself. Users then click Open or Download and hit a 400 because those APIs require a regular file. Rejecting the directory (#9142) stops the 400 but still does not give the user one card per generated file. See #9385.
Reviewer Test Plan
How to verify
.xlsxfiles (plus a hidden file and a~$Excel lock file). Record that folder as a workspace artifact. The tool result should say the directory was expanded, list the files, and must not say the folder itself was recorded. The artifacts panel should show one card per real file (not the folder, not the lock/hidden files)..docx/.pptxshould behave the same..xlsxwith the write-file tool (artifact recording enabled). It should be recorded as an office document, not as a generic text file. A.csvshould stay a text-previewable file.Unit coverage: directory expansion and empty/cap cases, write-file office kinds, turn grouping by folder prefix, office labels, and directory-stat refusal before reading bytes.
Evidence (Before & After)
Before: one card titled like "timeline data (17 Excel files)" with
workspacePathpointing at the folder; Open/Download returned 400.After: one card per file (Excel/Word/PowerPoint labeled as such), Open/Download work on files, folder path is not stored, and more than three cards collapse behind "show more".
Tested on
Environment (optional)
Local vitest:
packages/corerecord-artifact + write-file (124),packages/acp-bridgesession artifacts (114),packages/web-shellartifact panel / turn outputs / selectors (102).Risk & Scope
record_artifacton a non-empty directory now succeeds and emits per-file artifacts instead ofTARGET_IS_DIRECTORY. Callers that expected a hard failure for any directory will see success plus expansion. Path canonicalization from fix(artifacts): verify and canonicalize record_artifact workspace paths #9142 is unchanged for regular files.Linked Issues
Fixes #9385
Follow-up to #9142 (already merged)
中文说明
这个 PR 做什么
当会话把生成的文件夹登记成一条 workspace 产物时，客户端稍后会按普通文件去打开或下载，得到 HTTP 400。这次保留已合入
main的 #9142 路径校验，但目录不再被拒绝：会展开成「每个可登记文件一条产物」。隐藏名、Excel 锁文件、symlink 和常见垃圾目录会跳过。空目录仍然失败。原来的文件夹路径不会被存成可打开的产物。Office 文档（Word / Excel / PowerPoint 以及 OpenDocument）成为一等可下载产物，不再掉进文本预览。PDF、视频、音频同样只走下载。如果历史脏数据仍指向目录，Open/Download 会在
GET /stat后拒绝，而不会再打文件接口。一轮完成的聊天产物卡片最多显示 3 张，展开/收起控制和已编辑文件相同。聊天归组把当时登记的文件夹路径当前缀，所以展开后的文件仍挂在同一轮。
已在 #9142 合入后 rebase 到当前
main。请只审这一层增量。为什么需要
模型经常产出一文件夹表格或报告，却把文件夹本身登记进去。用户一点打开/下载就 400，因为那些接口要求普通文件。#9142 拒绝目录能挡住 400，但用户仍然拿不到「每个生成文件一张卡」。见 #9385。
评审测试计划
如何验证
.xlsx的文件夹（再加一个隐藏文件和一个~$Excel 锁文件），把该文件夹登记为 workspace 产物。工具结果应说明目录已展开并列文件，且不能声称登记了文件夹本身。产物面板应是每个真实文件一张卡（没有文件夹卡，也没有锁文件/隐藏文件）。.docx/.pptx同样。.xlsx，应记成 office 文档而不是普通文本。.csv仍可文本预览。证据（Before & After）
之前：一张「时间线数据（17 个 Excel 文件）」卡，
workspacePath指向文件夹；打开/下载 400。之后：每个文件一张卡（Word/Excel/PowerPoint 有对应类型），打开/下载对文件生效，文件夹路径不落库，超过 3 张会收起。
风险与范围
大目录上限 100 个文件（并告警）。只含隐藏文件的目录按空目录失败。不做目录浏览器 kind、登记时打包 zip，也不自动登记每一次 write/shell 输出。对非空目录，
record_artifact从硬失败改为展开成功；普通文件的 #9142 规范化不变。关联 issue
Fixes #9385
Follow-up to #9142（已合入）