feat(web-shell): add unified session sources - #11262
Conversation
callmeYe
changed the title
Local implementation and E2E acceptanceImplementation commit: The product daemon, ACP child, tool scheduler, recording files, session APIs, and installed Chrome ran locally. Model replies came from a deterministic localhost fixture with a test-only credential; this does not claim external-provider acceptance. Only isolated test configuration and runtime directories were used.
The real API/lifecycle runs also covered 44 registration/admission checks, 22 restart/fork/rewind/compaction/archive checks, and 19 malformed-snapshot/write-recovery/privacy checks. Later UI-only changes were followed by targeted component/browser checks and final full build/typecheck. These counts describe separate runs with overlapping coverage; they are not a single aggregate suite count. Confirmed behavior includes historical attachment access without registration, metadata-title precedence and deduplication, registration removal without automatic metadata resurrection, inert HTML after tab restoration, visibly rendered PDF body text, and binary downloads whose bytes match the original upload. Failed source/attachment-list reads remain independently visible. The actual metadata Retry issued no additional prompt or attachment upload; a definite rejected prompt cleaned the uploaded bytes. Declared fault injection: targeted metadata/list HTTP 503 responses, definite prompt HTTP 400 rejection, and a capability response without source support; all unaffected requests used the real daemon. Recording tests used only dedicated backed-up fixtures for an unsupported snapshot version and a truncated final JSON line, plus a temporary read-only recording to verify the existing degraded-writer recovery boundary. Local evidence remains under |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Thanks for the PR — this is a substantial one, so here is where the gate lands before the code review. Template ✓ Every required section is present and filled in, including a real reviewer test plan, the Tested-on table, and a full Chinese translation. Problem. This is a feature rather than a bug fix, so there is no reproduction to ask for — but there is also no linked issue and no user report behind it. The motivation is a product judgement: the attachments list only covers uploaded files, while workspace references and links have no session-level list. That may well be right; it just is not evidenced by anyone asking for it. Worth saying plainly, because it changes who should make the call. Direction. Web Shell is a first-class surface here, and folding uploaded files, workspace references and links into one list is coherent with how the environment panel already works. I checked the reference agent's CHANGELOG: no direct match for a unified session sources list, though session attachments and their restore robustness are an actively patched area — so the territory is relevant rather than off-mission. My concern is scale, not direction. Size. Production logic 2,590 lines across 42 files; tests 2,166 across 17; docs/design 696 across 5; generated/schema 0. Of the production total, 779 lines across 12 files sit in core paths ( Approach. Most of my simplification instincts were already answered by the diff, and I want to say so: the old attachments section and its row component are deleted rather than left sitting beside the new one, hosts that still ask for Risk. One high-risk path matched: Escalating to a maintainer for awareness on size and product direction — a notification, not a rejection. Moving on to code review. 🔍 中文说明感谢贡献！这个 PR 体量不小，先说明在进入代码审查之前 gate 的判断。 模板 ✓ 所有必填章节都完整填写，包含真实的 reviewer test plan、Tested-on 表格和完整中文翻译。 问题。 这是 feature 而非 bug fix，所以不需要复现——但也没有关联 issue 或用户反馈作为支撑。动机是一个产品判断：现有附件列表只覆盖上传文件，工作区引用和链接没有会话级清单。这个判断可能是对的，只是没有"有人提出过这个需求"作为证据。直说是因为这决定了该由谁来拍板。 方向。 Web Shell 在本仓库是一等公民，把上传文件、工作区引用和链接合并为一个清单，与 environment panel 现有形态是一致的。查了参考 agent 的 CHANGELOG：没有统一的会话来源清单的直接对应项，但会话附件及其恢复健壮性是持续在修的区域——所以这个领域是相关的，不算偏离主线。我的顾虑在于规模，不在于方向。 规模。 生产代码 2,590 行 / 42 个文件；测试 2,166 行 / 17 个；文档与设计 696 行 / 5 个；生成/schema 0 行。其中 779 行 / 12 个文件落在核心路径（ 方案。 我原本想提的简化点，diff 里大多已经回答了，这点要明确说：旧的 attachments 分区和它的行组件是被删除的，而不是与新组件并存；仍然声明 风险。 命中一个高风险路径： 因规模和产品方向转交维护者知悉——这是通知，不是拒绝。进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "session_sources" |
health-deep-with-session
| field | PR base (before) | this PR (after) |
|---|---|---|
activeWorkStaleMs |
5 |
6 |
— Qwen Code · serve A/B
Code reviewI read the core service, the new tool, the daemon routes, the bridge plumbing, the ACP child handlers, the recording/restore path and the Web Shell surface. No critical blocker — nothing I can demonstrate as a correctness bug, a security hole or a regression. Three things are worth the author's attention, and a longer list of things I checked and found clean, because at this size knowing what was verified matters as much as what was not. 1. ACP error flattening turns authorization failures into 503. In 2. 3. Every mutation costs one redundant refetch. What I checked and found clean:
Test coverage is genuinely good — 58 added cases, and they aim at the right things: owner replacement, workspace change, malformed and future-version snapshots, revision monotonicity, blob URL revocation, HTML-as-text, fork id regeneration, and registration only after prompt admission. sequenceDiagram
participant P1 as Web Shell client
participant P2 as Daemon session route
participant P3 as ACP bridge
participant P4 as ACP child agent
participant P5 as SessionSourceService
participant P6 as ChatRecordingService
P1->>P2: POST sources with bound client id
P2->>P3: upsertSessionSource owner scoped
P3->>P3: validate input and attachment existence
P3->>P4: ext method sources upsert
P4->>P5: upsert validated input
P5->>P6: append sources snapshot record
P6-->>P5: append acknowledged
P5-->>P4: new revision and source
P4-->>P3: result
P3-->>P2: result
P2-->>P1: 200 with new revision
P5-->>P4: notify revision after commit
P4-->>P3: sources changed notification
P3-->>P1: source_changed event
P1->>P2: GET sources for authoritative list
Files changed (22 rows covering 31 of 64 files — production code only)
Testing evidenceThis is an unattended CI run, so I did not build, run or execute anything from this PR — the review above is static. The evidence below is the PR's own CI, read through the API for the reviewed commit. No tmux capture applies on this path. Nothing has failed. The load-bearing legs are still running, so there is no green signal to report yet: the ubuntu unit suite, lint and static analysis, the serve A/B comparison, the no-AK integration tests, the real daemon E2E and the web-shell visual capture were all in progress at review time. What has completed is green — desktop shell on ubuntu and windows, live host on macOS, TUI parity snapshots, the no-flicker gate, and the Java matrix. One gap worth naming: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle this:
Not verified: runtime behaviour on any platform, by me, for the reason stated above. The macOS acceptance results in the PR description are the author's claim, not independently re-run evidence. 中文说明代码审查。 我读了 core service、新工具、daemon 路由、bridge 管线、ACP child 处理、录制与恢复路径以及 Web Shell 界面。没有发现关键阻断项——没有可以证明的正确性 bug、安全漏洞或回归。有三点值得作者关注，另外附上一份我确认过没问题的清单，因为在这个体量下，"哪些被验证过"和"哪些没有"同样重要。 1. ACP 错误扁平化会把鉴权失败变成 503。 2. 3. 每次变更多一次冗余重新拉取。 确认过没问题的部分：
测试覆盖确实不错——新增 58 个用例，而且对准了正确的位置：owner 被替换、工作区变更、损坏与未来版本快照、revision 单调性、blob URL 释放、HTML 以文本渲染、fork id 重生成、以及仅在 prompt 准入之后才登记。 测试证据。 这是无人值守的 CI 运行，因此我没有构建、运行或执行本 PR 的任何代码——以上审查是静态的。下面的证据来自 PR 自身的 CI，通过 API 针对被审查的 commit 读取。本路径不适用 tmux 抓取。 没有失败项。但关键几项仍在运行，所以目前还没有可报告的绿色信号：ubuntu 单元测试、lint 与静态分析、serve A/B 对比、no-AK 集成测试、real daemon E2E 和 web-shell 视觉抓取，在审查时都还在进行中。已完成的都是绿的——ubuntu 与 windows 的 desktop shell、macOS 的 live host、TUI parity 快照、no-flicker gate 以及 Java 矩阵。 有一个缺口需要点名： 沙箱验证可以定论：
未验证：任何平台上的运行时行为，由我，原因如上。PR 描述中的 macOS 验收结果是作者的主张，不是独立复跑的证据。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the review itself came out clean and I would not block this, but Stage 0 escalated it for maintainer awareness on size and core breadth, and that caps the score as a matter of policy rather than doubt. Stepping back. Before reading the diff I wrote down what I would have done: extend the existing attachment record with optional metadata, render one list, and skip both the model-facing tool and the fork-time copy. The PR went further than that, and some of the extra scope is genuinely justified — keeping metadata durability separate from byte durability is a real requirement if a failed metadata write has to be retryable without resending a message, and a single snapshot record inside the existing recording service is simpler than the event-log or tombstone designs I had assumed it would need. Where my instinct held up is the rest: agent-side registration and cross-session fork copying are not needed to deliver "one place to reopen your materials", and they are where nearly all of the risk in this PR sits. The honest answer to "does this solve something users care about" is probably, but nobody has asked for it. There is no linked issue and no user report. That is the single biggest reason this belongs with a human rather than with the gate — a polished 2,590-line product bet is exactly the shape that gets approved because it is polished. On the code itself I have no reservations worth blocking on. It does not feel like it is trying too hard at the implementation level; the ambition is in the scope, not the style. The conventions are respected, the ownership scoping on the new routes is right, the validation is tighter than I expected, and the test suite aims at the failure modes that actually matter rather than at coverage numbers. The three findings in the review comment are all minor. If I had to maintain this in six months, the thing I would most expect to bite someone is So: I am deferring, not approving, and not requesting changes. Why this needs a human call — what I could not resolve from the diff, the tests and the description:
One process note: I tried to resolve an accountable maintainer deterministically and got nothing back — no No approval is posted in this run, and no approve-on-green marker is attached, so the finalize job will not approve this automatically when CI lands either — the escalation has to be cleared by a human first. 中文说明Confidence: 3/5 —— 审查本身是干净的，我不会阻断这个 PR，但 Stage 0 因规模和核心路径覆盖面把它升级为"需维护者知悉"，按规则这会把评分上限压到 3/5；这是政策导致的上限，不是我真的存疑。 退一步看整体。在读 diff 之前我写下了自己会怎么做：在现有附件记录上扩展可选元数据、渲染成一个清单，并且不做模型可见工具、也不做 fork 时的跨会话复制。这个 PR 走得比我的方案更远，其中一部分额外范围确实是必要的——如果元数据写入失败后必须能在不重发消息的前提下重试，那么元数据的持久化契约与文件字节的持久化契约就必须分开；而在现有录制服务里写一条快照记录，也比我原先以为的事件日志或墓碑方案更简单。但我的直觉在其余部分依然成立：agent 侧登记和跨会话 fork 复制，对"给用户一个重新打开资料的入口"这个目标来说不是必需的，而这个 PR 几乎所有的风险都集中在它们身上。 对"这是否解决了用户在意的问题"，诚实的回答是大概是的，但没有人提出过这个需求。没有关联 issue，也没有用户反馈。这正是它应该由人来判断、而不是由 gate 判断的最主要理由——一个打磨得很精致的 2,590 行产品决策，恰恰是最容易"因为精致而被批准"的形态。 对代码本身我没有值得阻断的保留意见。它在实现层面并没有用力过猛；野心体现在范围上，而不是写法上。约定被遵守，新路由的归属作用域是对的，校验比我预期的更严格，测试对准的是真正重要的失败模式而不是覆盖率数字。审查评论里的三条发现都很轻微。如果六个月后由我来维护，我最担心会出问题的是 所以：我选择 defer，既不批准，也不要求修改。 为什么需要人来拍板——无法从 diff、测试和 PR 描述中解决的：
一个流程说明：我尝试用确定性方式解析出应负责的维护者，但没有得到结果——没有 本次运行不会提交批准，也不会附带 approve-on-green 标记，因此 CI 变绿后 finalize job 也不会自动批准——这个升级必须先由人解除。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
本 PR 主要做了什么
在 web-shell 里把「上传文件 / 工作区文件引用 / HTTP(S) 链接」统一成一个 Sources 面板：新增一个只登记元数据（不读文件、不抓 URL、不进 prompt）的 record_source 工具与 GET/POST/DELETE /session/:id/sources 三个 owner 作用域 REST 接口，用带版本+revision 的 session_sources_snapshot 系统记录做持久化，走既有 chat-recording 严格落盘通道；acp-bridge 负责路由与 source_changed 失效事件；SDK 与 web-shell 提供 client 方法、事件归一（非对话气泡）与前端 hook/预览 tab。UI 侧用一个 Sources 分区替换旧的 Attachments 分区，按 attachmentId 去重、显式标题优先、默认三行可「查看全部/收起」，预览复用既有 owner/工作区信任校验，来源 HTML 强制按文本渲染、URL 不自动抓取。改动横跨 core / cli-acp / bridge / sdk / web-shell 五个包，并附了较完整的设计文档。
What this PR does
Unifies uploaded files, workspace-file references and HTTP(S) links into one web-shell "Sources" panel. Adds a metadata-only record_source tool (no file read / URL fetch / prompt injection) and three owner-scoped GET/POST/DELETE /session/:id/sources routes, persisted as a versioned session_sources_snapshot through the existing strict chat-recording append path. acp-bridge handles routing + a source_changed invalidation event; SDK/web-shell add client methods, non-transcript event normalization, a data hook and preview tab. UI replaces the old Attachments section, dedups by attachmentId with explicit-title precedence, 3-row view-all/collapse, reuses owner/workspace trust checks, forces source HTML to text, and never auto-fetches URLs. Spans core / cli-acp / bridge / sdk / web-shell, with a thorough design doc.
结论 / Verdict：工程完成度和契约对齐都很高，但本 PR 当前 CI 是红的，且有两处我核对过的真实缺陷，故以 COMMENT 发布（无 Critical）。
🔴/🟡 需要处理 / Must address
I-1 (Important) 三处「配套登记表」没跟着更新，直接导致 CI 漂移守卫失败
head a9be7a0a 上 Test (ubuntu-latest)、Integration Tests (no-AK)、web-shell E2E Smoke 全部 fail。Test job 里失败的三个测试与本 PR 的新增一一对应，而我确认这三处配套文件都没出现在 diff 里：
src/i18n/index.test.ts > has a zh translation for every core tool display name：新增了RECORD_SOURCE/'RecordSource'（tool-names.ts:65,123）但没有对应中文译名。本 PR 只改了 web-shell 的i18n.tsx，没动 CLI/core 的中文工具名目录。src/serve/capabilities-docs-contract.test.ts > documents exactly the conditional feature registry keys及> keeps the daemon index capability counts in sync：session_sources加进了capabilities.ts，但没同步到条件能力文档登记表 / daemon index 计数。src/serve/server/telemetry-catalog.test.ts > ... matches the explicit Express route registrations in both directions：新加了三个/session/:id/sources路由，却没在 telemetry catalog 注册。
（我本地 main 落后于合并基，没法直接跟真 main 逐条 diff，但三条失败测试的名字精确对应本 PR 的新增，且相应配套文件均未改动。）请把这三处配套登记补上，CI 应能转绿。
I-2 (Important) 附件元数据登记发生在「准入后取消/清理」之前，会留下坏状态
packages/web-shell/client/daemon/session/actions.ts:1327 先调 registerAcceptedAttachmentSources(...)（void、不 await；其体内只守 sessionRef.current!==session，不守 signal.aborted），紧接着 :1332 才判断 if (options?.signal?.aborted)，走 removePendingPrompt → removeUploadedAttachments（清掉上传字节）→ 返回 removedAfterAbort。
竞态后果二选一：(a) upsert 先落库、附件字节随后被清，留下一条指向已删除附件的持久化来源行（元数据与字节生命周期独立，取消登记不会删元数据）；(b) upsert 后到 → daemon 返回 404 source_attachment_not_found → 用户明明取消了，却弹出「Message sent; some source details could not be saved」并带一个永远重试不成功的 Retry。设计里明确「取消（acceptance 之前）应什么都不登记」。修法：把 registerAcceptedAttachmentSources(...) 移到 abort 分支之后，即只有在会真正保留该 prompt（走 return { promptId }、未 removedAfterAbort）时才登记。
I-3 (Important，纵深防御) 来源 URL 的 <a href> 没走 isSafeHref
packages/web-shell/client/components/artifacts/ArtifactPanel.tsx:3044 直接 href={locator.url}、:3047 openExternal(event, locator.url)。同文件里 artifact.url 那条路径是守的：:2706 const safeUrl = isSafeHref(artifact.url) ? artifact.url : undefined; → :2813/:2822。isSafeHref 已在 :55 导入却在来源路径上没用。当前服务端 validateSessionSourceInput（session-sources.ts:157-163）与快照恢复校验（parseSessionSourcesSnapshot→:226）都强制 http(s)，所以正常流程不可达；但兄弟路径都防了、这条没防，面对可被编辑/损坏后回放的 transcript 快照，建议对齐：href={isSafeHref(locator.url) ? locator.url : undefined}，非法时不渲染链接。
🟢 建议 / Nits
session-sources.ts:152-171把locator.url存成parsed.href（会补尾斜杠/重编码，属规范化；但设计只禁止「靠重定向推断规范 URL」，query/fragment 保留，非破约）；且把>2048的 href 长度校验并进了「must be HTTP(S) without credentials」这句措辞里，建议把超长单独报错。session-sources.ts:455-488copyFrom用copied（仅父列表）整体 commit 而非合并目标已有列表；目前唯一调用方是 fork 到新会话（列表为空，无碍），属潜在问题。可加「目标非空则拒绝/合并」以策安全。SourcesSection.tsx:371的AttachmentRow按钮只有title、没有aria-label（登记的来源行:143有aria-label），a11y 不一致。- i18n
sources.remove键定义了但没用到（设计本就禁止逐行删除），属死键。
🎉 做得好的地方 / Positives
- 工具只接受
workspace_file|url（record-source.ts的 oneOf 明确不含 attachment）且直接调本会话 service，无法绕过 daemon 侧「附件必须存在于本会话」的校验；只有await upsert成功才回执 ID。 - 路由 owner 归属严谨：读用
withOwnerReadSession、写用withOwnerMutableSession+strict gate，resolveOwnerSessionRuntime从不回退到 primary（多工作区测试逐条覆盖 unknown/untrusted/ambiguous/replacing），归档会话拒绝变更，unknown 与越权附件返回同一个 404，不构成跨会话存在性探针。 - 持久化走
appendRecordStrict(updateActiveTail:false)，commit 仅在落盘 ack 后置为 live、失败置loaded=false强制重载且绝不 ack 易失成功；恢复只读最后一条，损坏/未来版本 →sourcesUnavailable且不复活旧列表。 - SDK 与 server 的 wire 类型逐字段对齐；
source_changed归为非对话 bookkeeping 事件；revision/owner 陈旧响应有守卫；准入后补登记不 await、不改 prompt body、Retry 不重传消息。 - 预览严格按 owner +
workspaceCwd匹配、绝不把路径 rebase 到新 cwd；来源 HTML 强制文本、URL 不自动抓取；frame-src只加blob:，未引入外部 origin。
Reviewer note: static review only — no tests/build run; the three failing checks above are read from the Actions logs on head a9be7a0a, and each cited line is verified against that commit's tree.
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 of chunk 1 — the round-2 auditor never returned.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": whether the normal resume/ loadSession read path tolerates a torn JSONL tail that readSessionSources treats as complete:false → sourcesUnavailable , i.e. wh…; "agent reverse-audit (round 2)": whether the Web Shell client's connection.workspaceCwd follows a mid-session relocation, and whether the daemon canonicalizes a workspace path at session-crea…; "agent reverse-audit (round 1)": whether sessionArtifactsPersistenceAvailable — the toggle now gating session_sources — is actually derived from artifact-persistence availability or from th…; "agent reverse-audit (round 1)": the full record_artifact → record_source registry parity sweep — my repo-wide grep returned 122 matches and was truncated, so beyond rule-parser.ts:252-253,…; "agent reverse-audit (round 2)": the Compaction lifecycle row — I did not verify that a compaction preserves the latest session_sources_snapshot and keeps it out of model context and summarie…, and 7 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not reviewed: "agent verify (round 2)" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：reverse audit of chunk 1 — the round-2 auditor never returned.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：whether the normal resume/ loadSession read path tolerates a torn JSONL tail that readSessionSources treats as complete:false → sourcesUnavailable , i.e. wh…；"agent reverse-audit (round 2)"：whether the Web Shell client's connection.workspaceCwd follows a mid-session relocation, and whether the daemon canonicalizes a workspace path at session-crea…；"agent reverse-audit (round 1)"：whether sessionArtifactsPersistenceAvailable — the toggle now gating session_sources — is actually derived from artifact-persistence availability or from th…；"agent reverse-audit (round 1)"：the full record_artifact → record_source registry parity sweep — my repo-wide grep returned 122 matches and was truncated, so beyond rule-parser.ts:252-253,…；"agent reverse-audit (round 2)"：the Compaction lifecycle row — I did not verify that a compaction preserves the latest session_sources_snapshot and keeps it out of model context and summarie…，另有 7 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未审查："agent verify (round 2)"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
— qwen3.8-max via Qwen Code /review (v0.23.0)
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.
23 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-9 copySources is the only best-effort child call in branchSession with no timeout — still stands; already reported (comment 3953036869, packages/acp-bridge/src/bridge.ts:11210)
- R1-10 the fork source-copy path is untested end to end — still stands; already reported (comment 3953036874, packages/acp-bridge/src/bridge.ts:11243)
- R1-6 the fork source-copy path is untested end to end (child side) — still stands; already reported (comment 3953036877, packages/cli/src/acp-integration/acpAgent.ts:10330)
- R1-12 the only test of the sources trusted-parent gate cannot fail if the gate is deleted — still stands; already reported (comment 3953036878, packages/cli/src/acp-integration/acpAgent.test.ts:6864)
- R1-14 the sources ext-method catch-all discards the original error twice over — still stands; already reported (comment 3953036886, packages/cli/src/acp-integration/acpAgent.ts:8505)
- R1-11 a teardown failure in the finally block replaces the already-successful copyFrom result — still stands; already reported (comment 3953036890, packages/cli/src/acp-integration/acpAgent.ts:10373)
- R1-60 the new source branch returns before any recordExpectedBridgeError call — still stands; already reported (comment 3953036893, packages/cli/src/serve/server/error-response.ts:290)
- R1-64 the workspace_file guard applies Windows path syntax unconditionally — still stands; already reported (comment 3953036894, packages/core/src/services/session-sources.ts:121)
- R1-22 the new fail-closed branch voids source restoration for the whole index and is untested — still stands; already reported (comment 3953036903, packages/core/src/services/session-transcript-reader.ts:2002)
- R1-21 the needsDeferredDispatch disjunct for sourcesUuid is untested for the first-record shape — still stands; already reported (comment 3953036907, packages/core/src/services/session-transcript-reader.ts:2959)
- R1-23 the widened restore/resume projections carry sourcesSnapshot with no production reader — still stands; already reported (comment 3953036908, packages/core/src/services/sessionService.ts:3094)
- R1-24 conversation-branches.test.ts pins neutral-tail collapsing with hardcoded subtype lists and was not extended — still stands; already reported (comment 3953036912, packages/core/src/utils/conversation-branches.ts:21)
- R1-25 the only negative case for source_changed normalization drops sessionId — still stands; already reported (comment 3953036916, packages/sdk-typescript/test/unit/SessionSources.test.ts:82)
- R1-27 the prune removes source tabs without the panel bookkeeping both other prune paths perform — still stands; already reported (comment 3953036918, packages/web-shell/client/App.tsx:5034)
- R1-85 the source-tab prune effect matches by id only and keeps the captured snapshot — still stands; already reported (comment 3953036919, packages/web-shell/client/App.tsx:5043)
- R1-26 the prune effect depends on the whole artifactWorkspaceTarget object, rebuilt every render — still stands; already reported (comment 3953036922, packages/web-shell/client/App.tsx:5048)
- R1-86 retry closures accumulate one per failed prompt-send and one click replays all — still stands; already reported (comment 3953036931, packages/web-shell/client/App.tsx:10190)
- R1-87 sourcePreview is decided by the entry point rather than the attachment and is not part of the tab id — still stands; already reported (comment 3953036937, packages/web-shell/client/App.tsx:18627)
- R1-100 the placeholder skeleton is now rendered in addition to the list, ungated on first hydration — still stands; already reported (comment 3953036945, packages/web-shell/client/components/panels/SourcesSection.tsx:181)
- R1-101 the fallback title derived from the location is not clamped to the 200-character cap — still stands; already reported (comment 3953036951, packages/web-shell/client/components/panels/SourcesSection.tsx:255)
- …and 3 more (see the run report)
3 candidate finding(s) this round's reviewers re-derived matched entries already carried on this PR and were set aside before verification (R1-60, R1-25, R1-101) — a matched posted finding is ruled in the previous-round status as always, and a matched deferral stays on the standing deferral record.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit findings R2-60, R2-61, R2-62 — the verifier never ruled on them.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": whether the workspace_file read site ( readWorkspaceFileAsBlob / readFileBytes in the web-shell SourcePreview path) applies a realpath or containment check…; "agent reverse-audit (round 2)": whether the fork path rewrites each record's sessionId field, which decides if readSessionSources 's restoreSessionSources(records, sessionId) (requested i…; "agent reverse-audit (round 2)": the "Reuse existing size limits and supported image/PDF/text previews" claim (doc line ~372) — the source preview lives inside ArtifactPanel.tsx (there is no …; "agent reverse-audit (round 2)": provenance of AttachmentRow 's use of attachment.attachmentId as the visible row label and as FileTypeIcon 's name (doc line ~337 promises "a single-line, t…; "agent reverse-audit (round 2)": the matrix rows "Conversation isolation" and "Compatibility" ("unrelated clients ignore metadata safely", "source notifications produce no transcript bubble") —…, and 15 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
[Critical] R1-4 The three new /session/:id/sources* route registrations were not added to legacySessionTelemetryRoutes (packages/cli/src/serve/server/telemetry.ts:45), so npm test --workspace="packages/cli" fails in src/serve/server/telemetry-catalog.test.ts — measured AssertionError: expected [ 'DELETE /session/:id', …(71) ] to have a length of 69 but got 72 at :101, exactly the three routes this diff registers, and the following expect(registered).toEqual(catalog) is also violated. This is net-new by measurement (the file fails on the PR side only against base 7dc5cc745). It is not only a stale count: daemonTelemetryMiddleware (telemetry.ts:858) calls resolveDaemonTelemetryRoute(req) and does if (!route) { next(); return; }, and that resolver matches only against legacySessionTelemetryRoutes (telemetry.ts:495), so a real GET/POST/DELETE on the new sources routes from the Sources panel produces no OTel span, no recordDaemonHttpRequest counter sample and no entry in the Daemon Status requests time-series — sources traffic is invisible to daemon telemetry while every neighbouring session route is measured. Fix: add three entries mirroring the artifacts block at telemetry.ts:191-207 — { method: 'GET', path: '/session/:id/sources', attribution: 'handler_resolved', route: 'GET /session/:id/sources' } and the same for POST /session/:id/sources and DELETE /session/:id/sources/:sourceId. The fix must not violate packages/cli/src/serve/server/telemetry.test.ts:1094-1120 (currently green), which asserts expect(keys).toHaveLength(69), expect(new Set(keys).size).toBe(69), handler_resolved → 67 and pre_resolved → 2 with the pre_resolved set pinned to exactly ['POST /permission/:requestId', 'POST /session/:id/a2ui-action'] — so adding 3 entries requires 69→72 AND 67→70, the new routes must be handler_resolved, and route must be the exact "METHOD path" concatenation. Acceptance criterion: telemetry-catalog.test.ts > "matches the explicit Express route registrations in both directions" (already red) and telemetry.test.ts > "matches every catalog entry with its declared canonical attribution", which calls resolveDaemonTelemetryRoute for each catalog path and so pins that /session/session-1/sources actually resolves rather than merely being listed. Please prove it by mutation: remove the added catalog entries, run those tests, and confirm they go red. This Critical could not be anchored to a diff line — the route string '/session/:id/sources' appears in more than one added hunk position and the resolver refused to choose between them — so it is reported here in the body instead. 三条新的 /session/:id/sources* 路由注册没有被加入 legacySessionTelemetryRoutes（packages/cli/src/serve/server/telemetry.ts:45），因此 npm test --workspace="packages/cli" 在 src/serve/server/telemetry-catalog.test.ts 失败：:101 实测 AssertionError: expected [ 'DELETE /session/:id', …(71) ] to have a length of 69 but got 72，正好是本 diff 注册的三条路由，随后的 expect(registered).toEqual(catalog) 也被违反。按实测这是本 PR 新引入的失败（相对合并基 7dc5cc745 只在 PR 侧失败）。这不只是计数过期：daemonTelemetryMiddleware（telemetry.ts:858）调用 resolveDaemonTelemetryRoute(req) 并执行 if (!route) { next(); return; }，而该解析器只匹配 legacySessionTelemetryRoutes（telemetry.ts:495），所以来源面板发出的真实 GET/POST/DELETE 不会产生 OTel span、不会有 recordDaemonHttpRequest 计数样本、也不会出现在 Daemon Status 的请求时序里——来源流量对 daemon 遥测不可见，而相邻的每个会话路由都被度量。修法：仿照 telemetry.ts:191-207 的 artifacts 块补三条目录项。约束：telemetry.test.ts:1094-1120（当前为绿）断言 toHaveLength(69)、new Set(keys).size 为 69、handler_resolved → 67、pre_resolved → 2 且该集合精确为 ['POST /permission/:requestId', 'POST /session/:id/a2ui-action']，因此新增 3 条要求 69→72 且 67→70，新路由必须是 handler_resolved，route 必须是精确的 "METHOD path" 拼接。验收标准：上述两个测试用例。请用变异法证明：移除新增目录项，跑这些测试，确认变红。该 Critical 无法锚定到 diff 行——路由字符串 '/session/:id/sources' 出现在多个新增位置且解析器拒绝在其间做选择——因此改在正文中报告。
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 23 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
本轮评审重新推导出的 3 条候选发现与本 PR 已携带的条目匹配，已在验证前搁置（R1-60, R1-25, R1-101）——被匹配的已发布条目照常在上一轮状态区裁定，被匹配的延后条目仍保留在延后清单记录中。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：reverse audit findings R2-60, R2-61, R2-62 — the verifier never ruled on them.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：whether the workspace_file read site ( readWorkspaceFileAsBlob / readFileBytes in the web-shell SourcePreview path) applies a realpath or containment check…；"agent reverse-audit (round 2)"：whether the fork path rewrites each record's sessionId field, which decides if readSessionSources 's restoreSessionSources(records, sessionId) (requested i…；"agent reverse-audit (round 2)"：the "Reuse existing size limits and supported image/PDF/text previews" claim (doc line ~372) — the source preview lives inside ArtifactPanel.tsx (there is no …；"agent reverse-audit (round 2)"：provenance of AttachmentRow 's use of attachment.attachmentId as the visible row label and as FileTypeIcon 's name (doc line ~337 promises "a single-line, t…；"agent reverse-audit (round 2)"：the matrix rows "Conversation isolation" and "Compatibility" ("unrelated clients ignore metadata safely", "source notifications produce no transcript bubble") —…，另有 15 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R1-4 The three new /session/:id/sources* route registrations were not added to legacySessionTelemetryRoutes (packages/cli/src/serve/server/telemetry.ts:45), so npm test --workspace="packages/cli" fails in src/serve/server/telemetry-catalog.test.ts — measured AssertionError: expected [ 'DELETE /session/:id', …(71) ] to have a length of 69 but got 72 at :101, exactly the three routes this diff registers, and the following expect(registered).toEqual(catalog) is also violated. This is net-new by measurement (the file fails on the PR side only against base 7dc5cc745). It is not only a stale count: daemonTelemetryMiddleware (telemetry.ts:858) calls resolveDaemonTelemetryRoute(req) and does if (!route) { next(); return; }, and that resolver matches only against legacySessionTelemetryRoutes (telemetry.ts:495), so a real GET/POST/DELETE on the new sources routes from the Sources panel produces no OTel span, no recordDaemonHttpRequest counter sample and no entry in the Daemon Status requests time-series — sources traffic is invisible to daemon telemetry while every neighbouring session route is measured. Fix: add three entries mirroring the artifacts block at telemetry.ts:191-207 — { method: 'GET', path: '/session/:id/sources', attribution: 'handler_resolved', route: 'GET /session/:id/sources' } and the same for POST /session/:id/sources and DELETE /session/:id/sources/:sourceId. The fix must not violate packages/cli/src/serve/server/telemetry.test.ts:1094-1120 (currently green), which asserts expect(keys).toHaveLength(69), expect(new Set(keys).size).toBe(69), handler_resolved → 67 and pre_resolved → 2 with the pre_resolved set pinned to exactly ['POST /permission/:requestId', 'POST /session/:id/a2ui-action'] — so adding 3 entries requires 69→72 AND 67→70, the new routes must be handler_resolved, and route must be the exact "METHOD path" concatenation. Acceptance criterion: telemetry-catalog.test.ts > "matches the explicit Express route registrations in both directions" (already red) and telemetry.test.ts > "matches every catalog entry with its declared canonical attribution", which calls resolveDaemonTelemetryRoute for each catalog path and so pins that /session/session-1/sources actually resolves rather than merely being listed. Please prove it by mutation: remove the added catalog entries, run those tests, and confirm they go red. This Critical could not be anchored to a diff line — the route string '/session/:id/sources' appears in more than one added hunk position and the resolver refused to choose between them — so it is reported here in the body instead. 三条新的 /session/:id/sources* 路由注册没有被加入 legacySessionTelemetryRoutes（packages/cli/src/serve/server/telemetry.ts:45），因此 npm test --workspace="packages/cli" 在 src/serve/server/telemetry-catalog.test.ts 失败：:101 实测 AssertionError: expected [ 'DELETE /session/:id', …(71) ] to have a length of 69 but got 72，正好是本 diff 注册的三条路由，随后的 expect(registered).toEqual(catalog) 也被违反。按实测这是本 PR 新引入的失败（相对合并基 7dc5cc745 只在 PR 侧失败）。这不只是计数过期：daemonTelemetryMiddleware（telemetry.ts:858）调用 resolveDaemonTelemetryRoute(req) 并执行 if (!route) { next(); return; }，而该解析器只匹配 legacySessionTelemetryRoutes（telemetry.ts:495），所以来源面板发出的真实 GET/POST/DELETE 不会产生 OTel span、不会有 recordDaemonHttpRequest 计数样本、也不会出现在 Daemon Status 的请求时序里——来源流量对 daemon 遥测不可见，而相邻的每个会话路由都被度量。修法：仿照 telemetry.ts:191-207 的 artifacts 块补三条目录项。约束：telemetry.test.ts:1094-1120（当前为绿）断言 toHaveLength(69)、new Set(keys).size 为 69、handler_resolved → 67、pre_resolved → 2 且该集合精确为 ['POST /permission/:requestId', 'POST /session/:id/a2ui-action']，因此新增 3 条要求 69→72 且 67→70，新路由必须是 handler_resolved，route 必须是精确的 "METHOD path" 拼接。验收标准：上述两个测试用例。请用变异法证明：移除新增目录项，跑这些测试，确认变红。该 Critical 无法锚定到 diff 行——路由字符串 '/session/:id/sources' 出现在多个新增位置且解析器拒绝在其间做选择——因此改在正文中报告。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
Follow-up in a2b12d0, with current main incorporated by af2c846. The source lifecycle, cancellation, preview state, capability catalogs, translations and telemetry findings are addressed. In particular, standalone activation now binds sources, refreshes the model tool snapshot, and rebuilds the existing startup context after replay; a real provider request confirmed the default deferred Validation:
The review threads record explicit follow-ups for acknowledged copy cancellation/draining, unused restore-projection cleanup, host retry-notice aggregation, the portable path namespace, and a separate writer-error transport taxonomy. A plain timeout was deliberately not added around a mutation that may still hold the target writer. Required reviews and CI remain in force; this update is validation evidence, not a replacement for those gates. |
This automated review inspected 0b51688. All current 39 review threads are resolved through fixes, supported rebuttals or explicitly recorded follow-ups. The original Critical findings are fixed by a2b12d0, and the additional subagent/teammate source boundary is fixed by current head 4bdfaba. Full build/typecheck, focused regressions and real daemon/Chrome/provider-request verification pass. The human correctness concerns about catalogs, queued cancellation, and URL rendering are addressed. Dismissing this stale bot decision leaves required review and CI gates in force; it is not an approval.
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.
18 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- copySources awaits the child source-copy RPC with no timeout — already reported (comment 3953036869, R1-9); the author explicitly deferred a bounded cancellation/draining contract to a separate change
- the sourcesSnapshot / sourcesUnavailable restore fields have no production reader — already reported (comment 3953036908, R1-23); the author explicitly deferred it as projection cleanup
- the source ext-method catch-all sits above getSessionWriterError, flattening writer classifications — already reported (comment 3953036886, R1-14)
- a malformed source_changed frame is dropped without a fallbackDebug diagnostic — already recorded (review 5145009229, round-3 deferral list, normalizer.ts:375)
- an empty required field is reported to the model as a length overflow — already recorded (review 5145009229, round-3 deferral list, session-sources.ts:90)
- SourceDetail hand-inlines the base64-to-Blob conversion the exported helper exists for — already recorded (review 5145009229, round-3 deferral list, ArtifactPanel.tsx:3013)
- the download anchor omits its imageDownloadButton class — already recorded (review 5145009229, round-3 deferral list, ArtifactPanel.tsx:3172)
- the new SDK source types were inserted between a JSDoc comment and the declaration it documented — already recorded (review 5145009229, round-3 deferral list, types.ts:1663)
- the design doc internal child-method enumeration omits qwen/session/sources/copy — already recorded (review 5145009229, round-3 deferral list, web-shell-session-sources.md:261)
- source_changed was added to REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES but not to the it.each that pins it — already recorded (review 5145009229, round-3 deferral list, bridge.ts:1989)
- the only writer of the snapshot record is executed by no test — already recorded (review 5145009229, round-3 deferral list, chatRecordingService.ts:3001)
- the list-ordering contract is never asserted — already recorded (review 5145009229, round-3 deferral list, session-sources.test.ts:83)
- the provider link of the invalidation chain is untested — already recorded (review 5145009229, round-3 deferral list, DaemonSessionProvider.tsx:5394)
- the registration-failed Retry affordance is rendered by no test — already recorded (review 5145009229, round-3 deferral list, EnvironmentPanel.tsx:40)
- the sourceDialogOpen dismiss suppression is unpinned — already recorded (review 5145009229, round-3 deferral list, EnvironmentPanel.tsx:204)
- R3-2 the retry classifier decides permanence from a hand-listed set of three codes — still standing, already reported (comment 3960586024)
- R3-3 the branch-session source-warning toast sits below the switchStarted early return — still standing, already reported (comment 3960586029)
- R3-4 the beforeAdmit forwarding on the live-boundary branch is untested — still standing, already reported (comment 3960586040)
Not reviewed: reverse audit round 3 — stopped before round 3 by the review time budget.
Not reviewed: reverse audit rounds 1-2 candidate gaps — the convergence pair reported new candidates that no Step 4 verifier ruled on before the budget ran out, so none of them is part of this round's findings.
Not reviewed: test-efficacy — the probe harness produced no verdict (harnessValidated: null): its positive control ran and died before answering, so 0 mutants and 0 hunk-necessity probes were measured and every revert probe came back inconclusive.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) is outside the scoped npm test set, so integration-tests/cli/qwen-serve-routes.test.ts did not run in this review.
Not reviewed: build-and-test — packages/live-host is negated out of the npm workspace graph, so its own tsx --test suite (including the new package-manager.test.ts) was not executed here.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI and their suites did not run locally, so the Windows path-normalization arms of the new locator validator were exercised on Linux only.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": the doc's "must not acquire an idle-only prompt gate or hold a prompt scheduling lock" — I read the qwen/session/sources/* ext-method case (acpAgent.ts:10276-…; "agent reverse-audit (round 1)": the body of the qwen/notify/session/sources-changed handler at bridgeClient.ts:2210 — I confirmed the method name is handled and that the normalizer requires …; "agent reverse-audit (round 1)": the "Archive | disable mutations under existing archive rules" row — I inferred it from mutate({ strict: true }) + withOwnerMutableSession on both mutation …; "agent reverse-audit (round 2)": the capability-advertising claim in implementation step 5 ("then advertise session_sources and expose the tool for supported sessions") — I never opened pack…; "agent reverse-audit (round 2)": the cross-client compatibility claim ("Python/Java SDKs, standalone CLI, other ACP clients, and Desktop receive no new public source API in this phase; their ex…, and 21 more.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/App.tsx:5028 — [review] openSourcePanel does not check the same…packages/core/src/services/session-transcript-reader.ts:1996 — [review] Source availability is gated on the integrity of *every*…packages/core/src/services/sessionService.ts:2941 — [review] readSessionSources parses the whole live transcript into…packages/core/src/services/sessionService.ts:2951 — [review] Both new trust-boundary throws in readSessionSources —…packages/core/src/services/sessionService.ts:3885 — [review] forkSession 's new exclusion of session_sources_snapshot …packages/core/src/tools/record-source.ts:86 — [review] The model-facing schema and tool description never say…packages/core/src/services/session-sources.test.ts:124 — [review] Both scheme-rejection rows in the invalid-input list are…packages/web-shell/client/hooks/useSessionSources.ts:113 — [review] The remove mutation path is unreachable from any…packages/web-shell/client/hooks/useSessionSources.ts:84 — [review] upsert — the only user-reachable mutation in this hook —…packages/web-shell/client/hooks/useSessionSources.test.tsx:96 — [review] The test named ignores an older revision and refreshes…packages/web-shell/client/components/panels/EnvironmentPanel.tsx:189 — [review] Unlike the branch picker, the source dialog is not…packages/web-shell/client/components/panels/EnvironmentPanel.test.tsx:667 — [review] Retitling the section to "Sources" leaves the panel's only…packages/web-shell/client/components/panels/EnvironmentPanel.test.tsx:854 — [review] The Add-source dialog's url branch is never submitted by…packages/web-shell/client/components/panels/SourcesSection.tsx:178 — [review] The loading placeholder is a verbatim second copy of…packages/web-shell/client/components/artifacts/SourcePreview.test.tsx:135 — [review] The case named rejects a changed workspace and revoked…packages/web-shell/client/components/artifacts/ArtifactPanel.tsx:1071 — [review] The new kind: 'source' tab variant has no branch in the…packages/web-shell/client/daemon/session/actions.ts:2957 — [review] The sourceWarnings chain is tested only at its origin —…docs/design/web-shell-session-sources.md:239 — [review] The doc promises the user-visible notice unconditionally…packages/cli/src/serve/server.test.ts:24212 — [review] The new GET route test only exercises the…packages/cli/src/serve/routes/session.ts:6328 — [review] The same nine-line client_id_required 403 block is…- …and 1 more (see the run report)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 18 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：reverse audit round 3 — stopped before round 3 by the review time budget.
未审查（原文为英文）：reverse audit rounds 1-2 candidate gaps — the convergence pair reported new candidates that no Step 4 verifier ruled on before the budget ran out, so none of them is part of this round's findings.
未审查（原文为英文）：test-efficacy — the probe harness produced no verdict (harnessValidated: null): its positive control ran and died before answering, so 0 mutants and 0 hunk-necessity probes were measured and every revert probe came back inconclusive.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) is outside the scoped npm test set, so integration-tests/cli/qwen-serve-routes.test.ts did not run in this review.
未审查（原文为英文）：build-and-test — packages/live-host is negated out of the npm workspace graph, so its own tsx --test suite (including the new package-manager.test.ts) was not executed here.
未审查（原文为英文）：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI and their suites did not run locally, so the Windows path-normalization arms of the new locator validator were exercised on Linux only.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：the doc's "must not acquire an idle-only prompt gate or hold a prompt scheduling lock" — I read the qwen/session/sources/* ext-method case (acpAgent.ts:10276-…；"agent reverse-audit (round 1)"：the body of the qwen/notify/session/sources-changed handler at bridgeClient.ts:2210 — I confirmed the method name is handled and that the normalizer requires …；"agent reverse-audit (round 1)"：the "Archive | disable mutations under existing archive rules" row — I inferred it from mutate({ strict: true }) + withOwnerMutableSession on both mutation …；"agent reverse-audit (round 2)"：the capability-advertising claim in implementation step 5 ("then advertise session_sources and expose the tool for supported sessions") — I never opened pack…；"agent reverse-audit (round 2)"：the cross-client compatibility claim ("Python/Java SDKs, standalone CLI, other ACP clients, and Desktop receive no new public source API in this phase; their ex…，另有 21 条。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 21 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.1)
|
Closed the current source security/bootstrap review findings and the CI regressions in Source metadata now rejects Unicode control and format characters while retaining significant filename whitespace. All three source operations use the established standalone activation guard; ordinary persistence failures keep their existing semantics. The history regression respects main's deferred-row loading and now proves response-time anchor recapture by mutation. The split-session tests use the renamed activity hook, and the HTML source/attachment preview test waits for its scheduled list load and asserts the button exists before clicking. Validation: full locked install, build, typecheck and bundle passed; 871 focused unit/contract tests passed, including all 819 tests in the two previously failing WebShell files. The current original browser Smoke suite passed 58/58, and all three built-asset history cases passed at CPU6. All final browser/test runs used zero retries. The original failures, invalid-input red tests and the anchor callback mutation failure were retained as evidence; no assertion tolerance or timeout was relaxed. The three Suggestion threads now explicitly record their remaining follow-up scope (general retry classification, session-scoped branch warnings, and an additional live-boundary mutation test). They are not claimed implemented. Valid Critical findings are fixed; fresh CI and required review remain authoritative for merging. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": the two lifecycle/ownership sentences "Archived-session mutations follow the shared archive coordinator's rejection" and "Persisted-only sessions must be resume…; "agent reverse-audit (round 2)": reading packages/core/src/services/session-sources.test.ts — both Fix witness fields above are stated as the test that must exist, not as a confirmed present-…; chunk 27: npm run typecheck for packages/web-shell was not run (the SDK dist/ output the typecheck resolves through is absent in this worktree); type-level conclusion…; "agent reverse-audit (round 1)": whether a failed retrySourceRegistrations (App.tsx:4056, which clears sourceRegistrationRetries *before* awaiting the closures) re-emits its notice.sourceR…; "agent reverse-audit (round 2)": whether attachmentsRoots.root passed at packages/cli/src/serve/server.ts:1153 and run-qwen-serve.ts:5498/6071/6749 can itself be undefined in the shipped da…, and 12 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not reviewed: verification — its prompt was built, but no agent was launched with it, so the posted findings cannot be counted as verified.
— [unverified] tag when the loop ended — the verifier never ruled on them, and they are not confirmed.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/bridge.ts:1988 — [review] The set exists so an idle non-turn event does not defeat the refresh-append of a queued turn-error terminal (isRefreshAppendTurnContent, bridge.ts:2056, consumed at :2377). Every other membe…packages/acp-bridge/src/bridge.ts:11253 — [review] 'catch {' binds nothing, so for a channel death mid-copy (getTransportClosedReject(entry) rejecting), or a non-ENOENT readdir/stat failure thrown out of attachments.list() (sessionAttachmen…packages/core/src/services/session-sources.ts:197 — [review] session-sources.test.ts:96 is titled 'keeps identity, creation time and order on metadata edits...' but asserts only id, createdAt, description, change and revision on a single-so…packages/core/src/services/sessionService.ts:3092 (+3 locations) — [review] Grepping every read site of both identifiers across packages/*/src and integrations: the only non-test reads are session-sources.ts:349 and :354 inside SessionSourc…packages/core/src/services/sessionService.ts:3885 — [review] sessionService.test.ts:6779 'drops creation metadata so the fork inherits no lineage or source' seeds parent_session and session_source records and asserts they are not copied, bu…packages/sdk-typescript/src/daemon/types.ts:1663 — [review] Confirmed at the reviewed commit (git show HEAD:packages/sdk-typescript/src/daemon/types.ts, lines 1662-1667): the comment sits immediately above 'export type SessionSourceLocator'…packages/web-shell/client/App.tsx:1868 — [review] Open a registered session source from the Sources section (openSourcePanel, App.tsx:5027) so it is the only right-panel tab, then reload. serializeArtifactPanelTabs yields [], so the layout …packages/web-shell/client/App.tsx:5078 (+2 locations) — [review] useSessionSources.refresh() stores the freshly parsed {...result}, so every refresh yields new SessionSource object identities even when nothing changed; 'fresh !== tab.source…packages/web-shell/client/App.tsx:10415 — [review] actions.test.ts:4768 asserts the notice carries a callable sourceRetry that re-issues the upsert without resubmitting the prompt. From there the chain is untested: this effect collects the …packages/web-shell/client/components/artifacts/ArtifactPanel.tsx:3013 — [review] The added code is an identical atob + byte-wise Uint8Array + new Blob([bytes],{type}) conversion; App.tsx:5493 and ChatPane.tsx:1047 both call the canonical ba…packages/web-shell/client/components/artifacts/ArtifactPanel.tsx:3072 — [review] .pdf is a member of DOWNLOAD_ONLY_EXTENSIONS (artifacts/artifactUtils.ts:94-101), so deleting the !isPdf conjunct makes every PDF source take the unsupported b…packages/web-shell/client/components/panels/EnvironmentPanel.test.tsx:988 — [review] The panel used to render t('environment.attachments') as the section heading; after this change the unified section is headed t('sources.title')==='Sources…packages/web-shell/client/components/panels/EnvironmentPanel.tsx:203 — [review] Delete sourceDialogOpen from this early return and from the dependency array at EnvironmentPanel.tsx:230, and EnvironmentPanel.test.tsx still passes. Every poin…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:5394 — [review] useSessionSources re-fetches only when refresh identity or signals?.sourcesVersion changes (useSessionSources.ts:76-82). useSessionSources.test.tsx:28 drives…packages/web-shell/client/daemon/session/actions.ts:1175 — [review] packages/web-shell/client/components/ChatPane.tsx:871 calls actions.sendPrompt(trimmed, {...files}) whenever the pane is idle, so this call site, not the queued one, is the…packages/web-shell/client/daemon/session/actions.ts:2957 — [review] The design requires a target snapshot-write failure to be 'reported as sources not copied' without invalidating the fork, and the plan states warnings are visible 'for both…packages/web-shell/client/daemon/session/turn-navigation-store.ts:1196 — [review] The new test (npx vitest run client/daemon/session/turn-navigation-store.test.ts -t 'pre-admission capture' -> 3 passed) must take the 'direction !== "newer" …packages/web-shell/client/hooks/useSessionSources.ts:113 (+3 locations) — [review] Grepping the whole tree, the only caller of state.remove is the hook's own test (useSessionSources.test.tsx:133); no component references remove on the sourc…
Convergence: round 5 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 3 (2 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：the two lifecycle/ownership sentences "Archived-session mutations follow the shared archive coordinator's rejection" and "Persisted-only sessions must be resume…；"agent reverse-audit (round 2)"：reading packages/core/src/services/session-sources.test.ts — both Fix witness fields above are stated as the test that must exist, not as a confirmed present-…；chunk 27：npm run typecheck for packages/web-shell was not run (the SDK dist/ output the typecheck resolves through is absent in this worktree); type-level conclusion…；"agent reverse-audit (round 1)"：whether a failed retrySourceRegistrations (App.tsx:4056, which clears sourceRegistrationRetries *before* awaiting the closures) re-emits its notice.sourceR…；"agent reverse-audit (round 2)"：whether attachmentsRoots.root passed at packages/cli/src/serve/server.ts:1153 and run-qwen-serve.ts:5498/6071/6749 can itself be undefined in the shipped da…，另有 12 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未审查：验证——它的 prompt 已构建，但没有 agent 用它启动，发布的发现不能算作已验证。
— [unverified] 标记——验证者从未对它们作出裁决，它们不算已确认。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 18 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 5 条行内评论，其中 5 条是首次提出；上一轮发布了 3 条（其中 2 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.1)
|
Resolved the conflict with main Locked installation, full build, typecheck and bundle passed. Source/history tests passed 103/103 and entry/authentication/notification tests passed 48/48. An independent browser check of the actual standalone entry confirmed Sources and its add action after startup. The built-asset 200-record history scenario also passed at CPU6, retaining the reading row at 12px. Final browser assertions and timeouts are unchanged. The current original Smoke suite passed all 58 cases with zero retries (2.0m), including main's updated transcript/composer edge alignment. No browser page errors were recorded. |
|
Fixed the two reproduced round-5 UI defects: hiding the environment panel now closes its Add source portal through the existing dialog state callback, and standalone URL source details stay open without requiring workspace file authority. File previews still require their trusted workspace target and matching stored cwd; unavailable standalone file references show an explanation and perform no workspace reads. Unsafe URLs remain non-clickable. Both defects were reproduced in an isolated Chromium browser before editing. The same two browser cases now pass with zero retries, including modal/body-pointer cleanup and the standalone Open original link. New React regressions cover floating/docked hidden panels, restored outside dismissal, standalone URL and unavailable workspace-file behavior, and unsafe URLs. The collection Suggestions were checked against actual runners: Live Host job 102382259435 logs show the packaging dependency-tree regression ran and passed. The serve route E2E is explicitly collected by the dedicated E2E runner. Packaging dependency ownership is recorded as a separate maintenance follow-up; this UI fix does not restructure that package. Full build/typecheck/bundle and lint passed. Final complete App/environment tests passed 868/868, and the original full Smoke suite passed 58/58 (9 workers, zero retries, 1.9m). The first Smoke run had two transient failures (catalog count and details popup bounds); both passed unchanged in a focused run and then in the final full run. The first failure logs and traces are retained; no product, assertion or timeout change was made for them. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewing the overall trajectory rather than re-deriving the per-line findings: this PR has now run five review rounds (R1–R5), and each round has surfaced fresh Criticals — R5 at 07:37Z still opened two new ones (the EnvironmentPanel Add-source path and the standalone record_source path), fixed only in c817df5c8b at 08:07Z. That's the signature of a diff too large to converge: 6,831 additions across 88 files spanning core, ACP bridge, CLI serve, SDK, and Web Shell.
The individual fixes look responsive, but the fact that every pass discovers new certifies-falsely/fails-closed surface in a different layer suggests the review is sampling rather than covering. My recommendation is to split this into the source-model + persistence core and the Web Shell UI/panel layer (or at least pause until a full round lands with no new Critical), rather than continue pushing through single-fix rounds on a diff this size.
|
Acknowledged the convergence concern in review 5151984218. The current head is c817df5, with both R5 findings reproduced in Chromium and fixed, all 51 threads addressed, and its CI passing. Those results cover the fixes; they do not establish that the whole change has converged. I will wait for the full review already running on this head (34327474704) and a round with no new Critical findings before pursuing merge. The old blocking reviews remain in place. If that full round still finds new Criticals across layers, I will bring back a concrete split proposal rather than treating another isolated fix as proof of convergence. No review gate is being dismissed or bypassed. |
|
Following up on the convergence concern with concrete work on I reviewed the complete added/changed production logic across core persistence/tool activation, ACP ownership and copying, HTTP routes/error mapping, SDK events, and the WebShell state/preview paths. An independent test engineer reran the real daemon and browser workflows against isolated local fixtures. This was broader than the last changed lines, and it found one further real defect: an uploaded standalone attachment previewed normally until it was registered as a source, when the source preview incorrectly required workspace authority. That finding is fixed in this commit. Session-owned bytes now use the existing Blob/previewOnly path; workspace file references still require their trusted target and stored cwd, and HTML stays text-only. The new text/HTML regressions failed before the change and pass afterward, with revoked-owner access denied and no workspace reads. Evidence and limits:
The PDF observation establishes frame visibility and sizing, not exhaustive browser-PDF rendering. Held activation and every possible race are not claimed newly proven by real E2E; existing focused tests cover those gates. The post-fix delta was checked against the complete boundary map with no additional confirmed Critical. This is author-side review plus independent execution evidence, not a self-approval or a substitute for your formal judgment. There is also a concrete split option, with per-file ownership mapped locally: runtime/API/SDK (56 files, 3,486 additions), WebShell UI (20 files, 2,527 additions), independent history-anchor follow-up (5 files, 98 additions), and the packaging regression separately (1 file, 22 additions); docs account for another 698 lines. Runtime comes first, UI depends on its contracts, and public history/test coverage would be preserved. Splitting is ready to plan from those boundaries rather than deleting tests to reduce the number. No replacement PR has been published without agreement. Please re-review the repaired head against the full convergence concern. All 51 inline threads are addressed; the two old formal bot requests refer to commits 1673176 and b265680 and their findings have been fixed or explicitly dispositioned. The maintainer's broader convergence discussion remains visible and the required review/CI checks remain in force. |
Superseded technical findings at 1673176/b2656809 are fixed or explicitly dispositioned on fe5ebb1; all 51 inline threads are addressed. Cc/Cf validation, standalone activation guards, viewport regression and source preview fixes have passing regression/E2E evidence. The author explicitly requested dismissal of stale bot requests. This is not an approval: a fresh maintainer review from yiliang114 was requested for the overall convergence concern, and required CI/review remain in force.
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
增量复审 / Incremental re-review
基线 → head：a9be7a0a6 → fe5ebb17a（本轮只走查该区间内属于本 PR 的提交，合并 main 的 merge commit 不计；引用的每一行都在 head fe5ebb17a 的树上重新核对过）。
Baseline → head: a9be7a0a6 → fe5ebb17a. This pass only walks the PR's own commits in that range (main-merge commits excluded); every cited line is re-verified against the head fe5ebb17a tree.
本 PR 主要做了什么 / What this PR does
把 web-shell 里「上传文件 / 工作区文件引用 / HTTP(S) 链接」统一成一个 Sources 面板：新增一个只登记元数据（不读文件、不抓 URL、不进 prompt）的 record_source 工具与 owner 作用域的 /session/:id/sources REST 接口，用带 version+revision 的 session_sources_snapshot 系统记录、经既有 chat-recording 严格落盘通道持久化；acp-bridge 负责路由与 source_changed 失效事件，SDK/web-shell 提供 client 方法、非对话事件归一、数据 hook 与预览 tab，UI 用 Sources 分区替换旧 Attachments 分区。横跨 core / cli-acp / bridge / sdk / web-shell 五个包。
Unifies uploaded files, workspace-file references and HTTP(S) links into one web-shell "Sources" panel: a metadata-only record_source tool (no file read / URL fetch / prompt injection) plus owner-scoped /session/:id/sources routes, persisted as a versioned session_sources_snapshot through the existing strict chat-recording append path. acp-bridge handles routing and a source_changed invalidation event; SDK/web-shell add client methods, non-transcript event normalization, a data hook and a preview tab; the UI replaces the old Attachments section. Spans core / cli-acp / bridge / sdk / web-shell.
结论 / Verdict：上一轮我提的三条 Important（I-1/I-2/I-3）在 head 上全部已修，CI 也转绿了。但我在 core 侧新核出两处 Important（错误分类/可观测性、来源读取的全量转录解析），故仍以 COMMENT 发布（无 Critical）。
All three Importants from my last round (I-1/I-2/I-3) are fixed at head, and CI is green. But I found two new Important issues on the core side (error classification/observability, and a full-transcript parse to read sources), so this stays a COMMENT (no Critical).
✅ 上一轮 findings 已解决 / Prior findings resolved
- I-1（CI 红：三处配套登记表缺更新）已解决。 head 上
Test (ubuntu)、Integration (no-AK)、Lint、Desktop Shell、Capture web-shell visuals、web-shell E2E Smoke全绿（仅 bot 的review-pr还 pending）。提交a2b12d047补齐了packages/cli/src/i18n/locales/{zh,zh-TW,en}.js、serve/server/telemetry-catalog.test.ts+telemetry.ts、以及docs/developers/daemon/00-index.md+qwen-serve-protocol.md的能力登记。 - I-2（附件来源登记早于 abort 清理，留坏状态）已解决。
daemon/session/actions.ts里登记调用已挪到 abort-removal 分支之后：:1338命中signal.aborted且removePendingPrompt成功时，先removeUploadedAttachments再return {removedAfterAbort:true}（:1343-1344），从而跳过:1363的registerAcceptedAttachmentSources；另一条准入路径也在:1107先removeUploadedAttachments+throwIfAborted再登记（:1174）。「acceptance 前取消 → 什么都不登记」的契约现在成立。 - I-3（来源 URL 的
<a href>未走isSafeHref）已解决。components/artifacts/ArtifactPanel.tsx:3054现在用{isSafeHref(locator.url) && (...)}包住链接，:3057的href与:3060的openExternal仅在通过校验时渲染，与兄弟路径artifact.url（:2723）对齐。
🟡 本轮新发现 / New this round (Important)
R2-1 (Important) 四个 qwen/session/sources/* ext-method 把所有非 SessionSourceError 一律压成 source_persistence_unavailable，丢失错误分类且几乎无可观测性
packages/cli/src/acp-integration/acpAgent.ts:8492-8520 的 catch：
} catch (error) {
if (['qwen/session/sources/list','.../upsert','.../remove','.../copy'].includes(method)) {
if (!(error instanceof SessionSourceError)) debugLogger.error('[ACP] Session source ext-method error:', error);
return { sourceError: error instanceof SessionSourceError
? { code: error.code, message: error.message }
: { code: 'source_persistence_unavailable', message: 'Session source operation failed' } };
}
const writerError = getSessionWriterError(error); // :8514 — 对 sources 方法永远到不了这里
if (writerError) throw new RequestError(writerError.rpcCode, writerError.message, { errorKind: writerError.errorKind });
throw error;
}
问题一：永久性的调用方错误被当成瞬时存储故障上报。 copy 处理器在目标不是本会话 fork 时 throw RequestError.invalidParams(...)（acpAgent.ts:10348，另见 :10333 的 invalid copy target），这是 4xx 级的「你请求错了、重试也没用」；但它不是 SessionSourceError，于是被这个 catch 压成 code:'source_persistence_unavailable' —— 下游 serve/server/error-response.ts:283-284 把该 code 映射成 503。客户端由此无法区分「请求非法（永久）」与「存储挂了（可重试）」，会对一个永远不会成功的操作做重试。
问题二：真正的持久化故障没有任何诊断信息。 ENOSPC / EACCES / writer 拒绝等 5xx 级失败，同样落到 source_persistence_unavailable 这个泛化 message，底层 cause 只在 debugLogger.error 里（debug 构建才可见）。再叠加 services/session-sources.ts 自身的三处裸 catch {}（:313 快照解析失败、:343 load 失败、:374 persist 失败）也都把 cause 丢掉换成泛化 SessionSourceError，整个 sources 子系统在生产环境几乎无法排障。
建议：在这个 catch 里把 RequestError（尤其 invalidParams）与 writer 错误放行到 :8514 的既有路径，只对真正的 SessionSourceError/意外异常返回 source_persistence_unavailable；并在 session-sources.ts 的 catch {} 里保留 cause（new SessionSourceError(code, msg) 附带 { cause: error } 或至少一条非 debug 级日志）。
The catch at acpAgent.ts:8492-8520 flattens every non-SessionSourceError thrown by the four qwen/session/sources/* ext-methods into { code: 'source_persistence_unavailable' } before getSessionWriterError (:8514) is ever reached. (1) Permanent caller errors get reported as transient storage failures: the copy handler deliberately throw RequestError.invalidParams(...) when the target is not a fork of this session (acpAgent.ts:10348, also :10333), a 4xx-class "your request is wrong, retrying won't help" — but it is not a SessionSourceError, so it collapses to source_persistence_unavailable, which serve/server/error-response.ts:283-284 maps to 503. The client can no longer tell "invalid request (permanent)" from "storage down (retryable)" and will retry an operation that can never succeed. (2) Genuine persistence failures (ENOSPC/EACCES/writer-rejected) surface with the same generic message and their cause only in debugLogger.error (debug builds), compounded by three bare catch {} in services/session-sources.ts (:313, :343, :374) that also discard the cause — leaving the whole sources subsystem near-undebuggable in production. Suggest letting RequestError/writer errors fall through to the existing :8514 path and preserving cause in the session-sources.ts catches.
R2-2 (Important) 读取会话来源要把整份转录 jsonl 全量解析一遍，只为 findLast 一条快照记录
packages/core/src/services/sessionService.ts:2941-2942：
const { records, complete } =
await jsonl.readLinesWithIntegrity<ChatRecord>(filePath, Infinity);
Infinity 上限意味着把整份会话转录逐行读出并 JSON.parse 成 ChatRecord[]，随后 restoreSessionSources(records, sessionId)（services/session-sources.ts:301）只做一次 records.findLast(type==='system' && subtype==='session_sources_snapshot') —— 即只需要最后一条快照记录，却物化了整份文件。
调用时机：acpAgent.ts:14203 bindSessionSourceService 把它接成 service 的 load()，在该会话首次触碰来源（打开 Sources 面板 / list / upsert / remove / copy）时触发一次，且前面还 await recording.flush()。放大因素：ACP 子进程是单线程事件循环、多会话复用同一个 child，一次大文件的全量 JSON.parse 会在这个循环上形成一个同步尖峰，stall 掉同一 child 上的其它会话；会话转录越大（长会话可达数 MB），首开 Sources 面板的这一下越明显。
这是「一次性、惰性」的成本，不是每条来源操作都付，所以严重度有限；但既然快照就是「最后一条 system 记录」，用从尾部反向的有界扫描（或复用 readRestoreProjection 那条已有的选择性读取路径）就能避免整份物化。
Reading a session's sources parses the entire transcript: sessionService.ts:2941-2942 calls readLinesWithIntegrity<ChatRecord>(filePath, Infinity), materializing every record, only for restoreSessionSources (session-sources.ts:301) to findLast the single latest session_sources_snapshot record. It runs once per session, on first sources access, via bindSessionSourceService's load() (acpAgent.ts:14203, after recording.flush()). The amplifier: the ACP child is a single-threaded event loop shared by multiple sessions, so a full-file JSON.parse is a synchronous spike that stalls every other session on that child, and it grows with transcript size (long sessions reach multiple MB). It is a one-time lazy cost, so severity is bounded — but since the snapshot is just "the last system record", a bounded reverse scan from the tail (or reusing the existing selective readRestoreProjection path) would avoid materializing the whole file.
🟢 建议 / Nits
services/session-sources.ts:489copyFrom仍以commit(copied)（只含父列表）整体替换目标快照而非合并；当前唯一调用方有「目标必须是本会话 fork」的守卫（acpAgent.ts:10343）且 fork 初始为空，故实际安全，但若将来对已自行登记过来源的 fork 再次 copy，会静默丢弃子会话自己的来源。（上一轮已提，未变，留作潜在项。）tools/record-source.ts:36RecordSourceInvocation.execute()以零参覆盖了基类签名，忽略了AbortSignal（tools.ts:154），in-flight 的元数据写入无法被取消；鉴于只是一次快速的快照 append，影响很小。getDefaultPermission继承默认'allow'（tools.ts:117）不弹权限确认 —— 因为该工具只登记元数据、不读文件内容、且validateSessionSourceInput强制workspacePath为相对且不逃逸根目录，这个默认是站得住的，只是提醒这是有意为之。
🎉 做得好的地方 / Positives
- 上一轮三条 Important 全部按建议修掉，且修法干净（登记挪到 abort 分支之后、
isSafeHref与兄弟路径对齐、配套登记表补齐使 CI 转绿）。 - 持久化契约依旧稳：
appendRecordStrict、commit 仅在落盘 ack 后置 live、失败置loaded=false强制重载、恢复只读最后一条、损坏/未来版本 →sourcesUnavailable且不复活旧列表。 - 路由 owner 归属、归档会话拒写、unknown 与越权附件同一 404（不构成跨会话存在性探针）、来源 HTML 强制文本、URL 不自动抓取 —— 这些安全边界在本轮增量里保持不变。
Reviewer note: 纯静态审查 —— 未跑测试/构建。CI 状态读自 head fe5ebb17a 的 Actions（除 bot 的 review-pr 仍 pending 外全绿）。每条引用的行号都在 head fe5ebb17a 的文件树上核对；core 侧文件（acpAgent.ts / sessionService.ts / session-sources.ts / record-source.ts / error-response.ts）自 c817df5c 起未变，head fe5ebb17a 仅改了 ArtifactPanel.tsx 与 SourcePreview.test.tsx。
Static review only — no tests/build run. CI read from Actions on head fe5ebb17a (all green except the bot's review-pr, still pending). Every cited line is verified against the head fe5ebb17a tree; the core files (acpAgent.ts / sessionService.ts / session-sources.ts / record-source.ts / error-response.ts) are unchanged since c817df5c, and head fe5ebb17a touched only ArtifactPanel.tsx and SourcePreview.test.tsx.
|
Thanks for rechecking the previous findings. I checked R2-1/R2-2 against fe5ebb1 and am recording both as follow-up work, consistent with the repository's late-review rule to land only Critical fixes after roughly five rounds. This is not a claim that either improvement has been implemented.
The copy-to-an-already-populated-fork and cancellation points are also retained as follow-up considerations, not silently marked fixed. Current user-facing Critical defects have been addressed with regression and real-browser evidence; fresh CI and the requested full/maintainer review remain the merge gates. |
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D6-7 branch-time sourceWarnings toast unreachable below the switchStarted guard (packages/web-shell/client/App.tsx:11994) — already reported (round 5 deferral list, packages/web-shell/client/daemon/session/actions.ts:2957)
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and the suites ran only on Linux (locally and in CI), while this diff adds platform-dependent path handling.
Not reviewed: verification — 39 of 41 finder candidates and 49 of the convergence pair's 67 candidates were not put to a verifier within this round's time budget, so they are not reported as confirmed.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": the zh design doc's lifecycle-table rows in this chunk (compaction retaining the latest snapshot as session metadata, rewind carrying the latest snapshot into r…; "agent reverse-audit (round 1)": the zh doc's notification claims ( qwen/notify/session/sources-changed carrying session ID + revision, bridge republishing source_changed with the same field…; chunk 8: verifying the literal value of ToolDisplayNames.RECORD_SOURCE in core against the added toolDisplayName.RecordSource i18n keys (left to packages/cli/src/i1…; "agent reverse-audit (round 2)": whether the Web Shell's connection.workspaceCwd actually advances on a live cwd change (the last link making the stale record render "unavailable" rather than…; "agent reverse-audit (round 2)": my assigned diff read was offset=0, limit=8 (the two binary PNG entries only), while my brief carries a file-specific rule for docs/developers/qwen-serve-pro…, and 19 more.
— [unverified] tag when the loop ended — the verifier never ruled on them, and they are not confirmed.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round; 2 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/core/src/services/session-sources.ts:85 — [review] Critical [fails-closed] [new-surface] Invisible-character gate rejects all of Unicode Cf, blocking ZWNJ/ZWJ inputpackages/core/src/services/session-sources.ts:343 — [review] Critical [fails-closed] [new-surface] One latched recorder write failure permanently breaks read-only source listingpackages/web-shell/client/daemon/session/actions.ts:608 — [review] Mid-turn admission path never registers attachment sourcespackages/web-shell/client/components/panels/SourcesSection.tsx:262 — [review] A source description can never be cleared from the Web Shell dialogpackages/web-shell/client/components/panels/SourcesSection.tsx:265 — [review] Source error surfaces render the raw REST route label to end userspackages/web-shell/client/components/artifacts/ArtifactPanel.tsx:3073 — [review] Source preview classifies download-only from the file extension alone
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：reverse audit — stopped before round 3 by the review time budget.
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and the suites ran only on Linux (locally and in CI), while this diff adds platform-dependent path handling.
未审查（原文为英文）：verification — 39 of 41 finder candidates and 49 of the convergence pair's 67 candidates were not put to a verifier within this round's time budget, so they are not reported as confirmed.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：the zh design doc's lifecycle-table rows in this chunk (compaction retaining the latest snapshot as session metadata, rewind carrying the latest snapshot into r…；"agent reverse-audit (round 1)"：the zh doc's notification claims ( qwen/notify/session/sources-changed carrying session ID + revision, bridge republishing source_changed with the same field…；chunk 8：verifying the literal value of ToolDisplayNames.RECORD_SOURCE in core against the added toolDisplayName.RecordSource i18n keys (left to packages/cli/src/i1…；"agent reverse-audit (round 2)"：whether the Web Shell's connection.workspaceCwd actually advances on a live cwd change (the last link making the stale record render "unavailable" rather than…；"agent reverse-audit (round 2)"：my assigned diff read was offset=0, limit=8 (the two binary PNG entries only), while my brief carries a file-specific rule for docs/developers/qwen-serve-pro…，另有 19 条。
— [unverified] 标记——验证者从未对它们作出裁决，它们不算已确认。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改；其中 2 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 6 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.1)
Local real-environment verification (maintainer pass) — head
|
| Piece | What was real |
|---|---|
| Daemon | node packages/cli/dist/index.js serve --port 8931 --workspace <git repo>, isolated HOME, own trustedFolders.json |
| Web Shell | bundle index-POJ2tBKO.js, served from packages/web-shell/dist (startup banner confirms), driven in Chrome |
| Transcripts | real ~/.qwen/projects/.../chats/<id>.jsonl files, inspected and mutated on disk between runs |
| Model | local deterministic SSE fixture (only the model is fake) |
| Build | npm run build clean, npm run typecheck clean |
Fixtures in the workspace: a Markdown file, an HTML file containing <script>window.__RIG_HTML_EXECUTED = true;</script>, a 64×64 PNG, a one-page PDF with visible text, an application/octet-stream blob, plus two files whose names contain U+200C / U+200D.
Reviewer test plan — reproduced
| Plan item | Result |
|---|---|
| One Sources section, dedupe by attachment ID, explicit title wins, 3 rows + View all/Collapse, count correct | ✅ 10 registered + 4 uploaded → header Sources 13; uploaded-diagram.png appears once, as its explicit title "Architecture diagram (explicit title)"; collapsed 3 → expanded 13 → collapsed 3 |
| Workspace-file + HTTP(S) registration: identity/createdAt preserved, identical retry does not bump revision, edits keep ordering, invalid locators rejected, 200-cap applies to metadata only | ✅ create → revision 1 / created; identical retry → revision 1 / unchanged; title edit → revision 2 / updated with createdAt unchanged. Rejected: absolute path (400), ../.. escape (400), file:// (400), https://user:pw@… (400), unknown field (400), unknown attachment (404). Cap: 200 accepted, 201st → 409 source_limit_reached, and the UI still showed Sources 201 because the uploaded file stays listed |
| Previews: PDF body renders, binary bytes intact, HTML stays text after refresh, links not fetched, owner cannot redirect preview | ✅ PDF renders its page text; all 4 attachment downloads byte-identical to source (sha256); HTML renders as literal <h1>/<script> text, no iframe, window.__RIG_HTML_EXECUTED still undefined after a full page reload; a URL source pointed at my own HTTP server got 0 requests. |
| Attachment send + injected metadata failure: message/file usable, Retry adds no upload or prompt, removing only the registration keeps the bytes | ✅ send with a degraded recorder → message answered, file uploaded and listed, banner "Message sent; some source details could not be saved · Try again"; Retry left the transcript byte-identical and the attachment count unchanged. Deleting only the registration → removed:true, attachment bytes still served, nothing auto-re-registered |
| Restart/resume, rewind, compact, fork; malformed/future snapshots | ✅ daemon restart + resume → revision and all rows intact; rewind (rewound:true, truncatedCount:6) and /compress ("Context compressed (10 -> 10)") both left registered metadata untouched; fork copied all 10 sources with fresh ids, preserved createdAt, revision reset to 1, and carried all 4 attachments into the fork (spot-checked byte-identical), and later writes to the fork did not touch the parent; archived session → 404 on GET/POST/DELETE. A hand-planted version: 2 snapshot → 503 sources unavailable, old list not resurrected, while attachments listed 200 and the conversation loaded and rendered normally |
| Older daemon + independent list failures | ✅ with session_sources removed from the capability registry (140 features), the Add-source button disappears, no registered rows, no error, and all 5 uploaded files stay listed and openable. With the source list failing and attachments healthy, the panel shows the source error inline with its own Try again and keeps the uploaded files |
Screenshots for each of these are in the gallery below.
The two deferred Criticals both reproduce
1. session-sources.ts:85 — the \p{Cf} gate rejects real files, not just decorative characters.
The finding as recorded says "blocking ZWNJ/ZWJ input", which reads like a cosmetic title restriction. Live it is stronger: text() also validates workspacePath, so a file that exists in the workspace cannot be registered at all if its name contains U+200C or U+200D — which is ordinary for Persian and Indic filenames and for any emoji ZWJ sequence. In the UI the failure is also mis-attributed: the Add-source dialog derives the title from the filename, so pasting the path first fails with "Invalid title", and only after you type an ASCII title do you get the real reason, "Invalid workspacePath".
| Add source, path only | Add source, with an ASCII title |
|---|---|
 |
 |
It is genuinely fail-closed (nothing is stored, nothing wrong is certified), so it is not a merge blocker — but the cheap fix is to keep \p{Cc} plus the specific format characters worth banning (the bidi overrides U+202A–U+202E and U+2066–U+2069, and U+FEFF) instead of all of \p{Cf}.
2. session-sources.ts:343 — one write failure latches the read-only listing, and repairing the disk does not clear it.
The PR's stated failure boundary is "a degraded recording writer rejects metadata writes until session recovery/restart". Live, the read side goes down with it, and it stays down after the cause is gone: commit() sets loaded = false on a persist failure, so the next list() re-enters load(), which does await recording.flush() — and ChatRecordingService.writeFailure is a permanent per-process latch that pre-dates this PR. Neither repairing the file nor POST /session/:id/resume clears it; only a daemon restart does.
The user-visible shape of this is the one flow the test plan already asks about — and it is where the two findings meet:
| After the injected failure | After the disk is repaired, pressing Try again |
|---|---|
 |
 |
Retry can never succeed, and pressing it adds a second, permanent error to the panel. Nothing is lost — the uploaded file stays listed and the restart-time list is correct (no false success) — but "Try again" is offering an action that cannot work. A read-only list() could serve the in-memory snapshot, or load() could skip flush() when it is only reading.
On the standing round-6 Importants
- R2-1 (error classification). Through every REST route I could reach, the mapping is correct:
invalid_source→400,source_limit_reached→409,source_attachment_not_found→404,source_persistence_unavailable→503. The 4xx-flattened-to-503 case is confined toqwen/session/sources/copy, which has no REST route and is only reachable from the bridge's fork path — so the user-facing blast radius today is smaller than the finding implies. The second half of R2-1 is real and I hit it repeatedly: the daemon log shows only the generic message, never the underlyingEACCES. - R2-2 (full transcript parse). Measured rather than argued:
One cold read per session at ≈1.7 ms/MB (219 ms at 129 MB), warm reads flat at ~2 ms, and transient ACP-child RSS that tracks file size and does not settle back (+123 MB at 129 MB). The "synchronous spike stalls other sessions on the same child" part does not reproduce: the read is a streaming readline loop with an await per line, and a second session on the same child polled 60× during the 61 MB cold read saw p90 1.1 ms / max 2.5 ms. So: worth a tail-bounded read as follow-up, not a blocker.
Mutation matrix
8 of 10 killed. Both survivors are defence-in-depth rather than the primary path: M4 is the parse-side 200 cap, only reachable via a hand-edited or corrupted snapshot because the write path caps first; M8 drops workspaceCwd from sessionSourceId, which changes the hash but which I could not turn into a live divergence, since a session's project root is fixed for its lifetime. Worth one test each, not worth blocking on.
Local suite results at this head
| Suite | Result |
|---|---|
npm run build / npm run typecheck |
✅ clean |
| core (session-sources, record-source, session-transcript-reader, session-source-tool-activation, conversation-branches) | ✅ 202/202 |
acp-bridge (session-sources + bridge) |
✅ 933/933 |
sdk-typescript SessionSources |
✅ 2/2 |
cli serve (server, error-response, telemetry, telemetry-catalog, web-shell-static, multi-workspace-sessions) |
✅ 1539/1539 |
| web-shell, full client suite | ✅ 294 files / 6912 tests |
One earlier acp-bridge run reported 2 failures; that run overlapped the 6912-test web-shell run on the same machine and it is green on a clean re-run — load flake, not a real failure.
Smaller things I noticed
- Error strings render route templates to end users: "GET /session/:id/sources: Stored sources are unavailable" and "POST /session/:id/sources: Invalid title" in the panel and the Add-source dialog.
- Opening a workspace-file binary shows a stale "Loading file…" line above
GET /file: binary file: /abs/host/path/..., with no download affordance — the uploaded-attachment path for the same bytes offers a clean "Preview is not available for this file type" card with a download button. The absolute host path also comes through verbatim. - At the 200-source cap, sending an attachment silently does not register it (
source_limit_reachedis deliberately treated as permanent and dropped from the retry queue). Nothing is lost — the file still shows as an uploaded file — but there is no signal at all. Fine as designed; flagging in case the silence was not intended.
Not verified
Owner-unavailable / owner-replaced preview redirection; "a definite prompt rejection cleans the uploaded bytes"; cross-client source copying through the standalone CLI; Windows and Linux (macOS only); external model providers. Compaction was exercised via /compress on a small transcript, so it proved "registered metadata survives compaction" but not the large-context compaction path.
On the convergence concern in review 5151984218
For what it is worth as a data point: this pass was independent of the review rounds — real daemon, real browser, mutation matrix, failure injection at the filesystem — and it surfaced no new Critical. It reproduced the two already-recorded deferred ones and sharpened their reachability, and everything else it found is a nit or a follow-up. That does not settle the diff-size argument, but it is one full pass over the feature's behaviour that did not open a new fails-closed surface.
Screenshot gallery (Web Shell, real daemon)
| Collapsed — default 3 rows, total 13 | Expanded — all 13, dedup + explicit title |
|---|---|
 |
 |
| Workspace PDF — body actually renders | Workspace HTML — stays literal text, script never runs |
|---|---|
 |
 |
| Binary attachment — no preview, download offered | Source list down, attachments healthy — independent errors |
|---|---|
 |
 |
Older daemon (capability removed) — no Add-source affordance, uploads still listed:
Evidence images: wenshao/qwen-code@assets-pr11262 · rig details reproducible from the tables above.
中文说明
本地真实环境验证（维护者复核）— head fe5ebb17a1
我把这个 PR 构建出来当产品来跑：真实的 qwen serve daemon、真实 ACP 子进程、磁盘上真实的转录文件、真实 Chrome 打真实的 Web Shell 产物。没有 mock daemon，没有 mock bridge。只有模型回复来自本机确定性的 OpenAI 兼容 fixture，其余（调度、存储、REST/SSE、浏览器）全部是真的。
结论：功能与描述一致，声明的失败边界成立——我跑过的所有探针里，没有任何一次给出错误结果、也没有任何一次静默丢文件。两条被记为「推迟」的 Critical 都能在活体上复现，其中一条会挡住一个完全正常的日常输入（文件名里带波斯语／印地语／emoji 连接符的工作区文件）。两条都不属于数据丢失或错误结论，所以我认为可以合入，但那个不可见字符门建议在合入前后尽快收窄。
验证台
| 部件 | 真实性 |
|---|---|
| Daemon | node packages/cli/dist/index.js serve --port 8931 --workspace <git 仓库>，隔离 HOME，独立 trustedFolders.json |
| Web Shell | 产物 index-POJ2tBKO.js，由 packages/web-shell/dist 提供（启动横幅可证），在 Chrome 中操作 |
| 转录 | 真实的 ~/.qwen/projects/.../chats/<id>.jsonl，在各轮之间直接读取和篡改 |
| 模型 | 本机确定性 SSE fixture（只有模型是假的） |
| 构建 | npm run build、npm run typecheck 均通过 |
工作区夹具：Markdown 文件、含 <script>window.__RIG_HTML_EXECUTED = true;</script> 的 HTML、64×64 PNG、带可见文字的单页 PDF、application/octet-stream 二进制，另加两个文件名里含 U+200C / U+200D 的真实文件。
审阅验证计划 — 复现结果
| 计划条目 | 结果 |
|---|---|
| 只有一个「来源」分区、按 attachment ID 去重、显式标题优先、默认三条 + 查看全部/收起、总数正确 | ✅ 10 条登记 + 4 个上传 → 标题 Sources 13；uploaded-diagram.png 只出现一次，显示为显式标题「Architecture diagram (explicit title)」；收起 3 → 展开 13 → 再收起 3 |
| 工作区文件与 HTTP(S) 登记：身份/创建时间保留、相同重试不涨 revision、编辑不改排序、非法定位被拒、200 上限只约束元数据 | ✅ 创建 → revision 1 / created；相同重试 → revision 1 / unchanged；改标题 → revision 2 / updated 且 createdAt 不变。被拒：绝对路径（400）、../.. 逃逸（400）、file://（400）、https://user:pw@…（400）、未知字段（400）、未知附件（404）。上限：接受 200 条，第 201 条 → 409 source_limit_reached，而界面仍显示 Sources 201，因为上传文件照常列出 |
| 预览：PDF 正文渲染、二进制字节一致、刷新后 HTML 仍为文本、链接不自动抓取、归属不可改向 | ✅ PDF 渲染出页面文字；4 个附件下载 sha256 与源文件逐字节一致；HTML 渲染为字面 <h1>/<script> 文本，无 iframe，整页刷新后 window.__RIG_HTML_EXECUTED 仍是 undefined；指向我自建 HTTP 服务的 URL 来源收到 0 次请求。 |
| 发送附件 + 注入元数据失败：消息与文件仍可用、重试不增加上传或 prompt、仅删除登记后字节仍在 | ✅ 在录制器降级状态下发送 → 消息拿到回复、文件上传并列出、出现横幅「Message sent; some source details could not be saved · Try again」；点重试后转录逐字节不变、附件数量不变。仅删除登记 → removed:true，附件字节仍可下载，且不会自动重新登记 |
| 重启/恢复、回退、压缩、分叉；损坏/未来版本快照 | ✅ daemon 重启 + resume → revision 与全部条目完好；回退（rewound:true、truncatedCount:6）与 /compress（"Context compressed (10 -> 10)"）都没有动登记元数据；分叉复制了全部 10 条来源、ID 全部不同、createdAt 保留、revision 归 1，4 个附件全部带入分叉（抽查逐字节一致），之后写分叉不影响父会话；归档会话对 GET/POST/DELETE 一律 404。手工植入 version: 2 快照 → 503 sources unavailable，不会复活旧列表，同时附件列表 200、对话正常加载渲染 |
| 旧 daemon 与两侧失败独立 | ✅ 把 session_sources 从能力表移除后（140 项），新增来源按钮消失、无登记条目、无报错，5 个上传文件照常列出并可打开。来源列表失败而附件正常时，面板单独显示来源错误并带自己的重试，同时保留上传文件 |
收起/展开/PDF/HTML 文本化/二进制/降级/旧 daemon 的截图见文末链接。
两条推迟的 Critical 都能复现
1. session-sources.ts:85 — \p{Cf} 门挡的不只是装饰字符，而是真实文件。
记录里写的是「阻止 ZWNJ/ZWJ 输入」，读起来像是标题的装饰性限制。活体上更强：text() 同样校验 workspacePath，所以只要文件名含 U+200C 或 U+200D，工作区里真实存在的文件根本无法登记——而这在波斯语、印地语文件名和任何 emoji ZWJ 序列里都是常态。界面上报错还归错了字段：新增来源对话框会从文件名推导标题，所以只粘路径先报 「Invalid title」，再手填一个 ASCII 标题才会看到真正原因「Invalid workspacePath」。
这确实是 fail-closed（什么都没存、没有认定错误结果），所以不是合并阻断项——但便宜的修法是保留 \p{Cc} 再加上确实该禁的少数格式字符（bidi 覆盖 U+202A–U+202E、U+2066–U+2069 以及 U+FEFF），而不是整个 \p{Cf}。
2. session-sources.ts:343 — 一次写失败会闩死只读列表，修好磁盘也解不开。
PR 声明的失败边界是「录制写入器降级后，在会话恢复/重启前拒绝元数据写入」。活体上读也一起挂，而且原因消失后依然挂着：commit() 在持久化失败时把 loaded = false，于是下一次 list() 重新进入 load()，而 load() 会 await recording.flush()——ChatRecordingService.writeFailure 是本 PR 之前就存在的进程级永久闩。修好文件不行，POST /session/:id/resume 也不行，只有重启 daemon 能恢复。
用户看到的形态正是测试计划已经问到的那条流程，也是两条发现交汇的地方：重试永远不会成功，而且点一次就在面板上多出第二条永久错误。数据没丢——上传文件仍在列表里，重启后的列表也正确（没有虚假成功）——但「Try again」提供的是一个不可能成功的动作。只读的 list() 可以直接返回内存快照，或者 load() 在纯读取时跳过 flush()。
关于第 6 轮仍未决的两条 Important
- R2-1（错误分类）。 我能触达的每一条 REST 路由，映射都是对的：
invalid_source→400、source_limit_reached→409、source_attachment_not_found→404、source_persistence_unavailable→503。4xx 被压成 503 的情况仅限于qwen/session/sources/copy，它没有 REST 路由，只能从 bridge 的分叉路径到达——所以目前对用户的影响面比该发现描述的要小。R2-1 的后半部分是真的，我反复撞到：daemon 日志只有泛化 message，从来看不到底层的EACCES。 - R2-2（全量转录解析）。 我用测量代替争论：每个会话一次冷读约 1.7 ms/MB（129 MB 时 219 ms），热读稳定在 ~2 ms，ACP 子进程的瞬时 RSS 随文件大小增长且不回落（129 MB 时 +123 MB）。而「同步尖峰会 stall 同一 child 上的其它会话」这部分复现不出来：读取是每行带
await的流式readline循环，61 MB 冷读期间同 child 上另一个会话轮询 60 次，p90 1.1 ms、max 2.5 ms。所以：值得作为后续改成从尾部有界读取，但不是阻断项。
变异矩阵
10 个变异体杀死 8 个。两个存活的都是纵深防御而非主路径：M4 是解析侧的 200 上限，只有手工编辑或损坏的快照才能到达（写入路径先行截断）；M8 把 workspaceCwd 从 sessionSourceId 里去掉，哈希确实变了，但我构造不出活体差异，因为一个会话的项目根在其生命周期内是固定的。各补一个用例即可，不值得阻断。
本 head 上的本地套件结果
| 套件 | 结果 |
|---|---|
npm run build / npm run typecheck |
✅ 通过 |
| core（session-sources、record-source、session-transcript-reader、session-source-tool-activation、conversation-branches） | ✅ 202/202 |
acp-bridge（session-sources + bridge） |
✅ 933/933 |
sdk-typescript SessionSources |
✅ 2/2 |
cli serve（server、error-response、telemetry、telemetry-catalog、web-shell-static、multi-workspace-sessions） |
✅ 1539/1539 |
| web-shell 全量 client 套件 | ✅ 294 个文件 / 6912 个用例 |
早先有一次 acp-bridge 运行报了 2 个失败；那次与 6912 用例的 web-shell 运行在同一台机器上重叠，单独重跑全绿——是负载抖动，不是真失败。
其它小问题
- 报错字符串把路由模板暴露给最终用户：面板里的 「GET /session/:id/sources: Stored sources are unavailable」，以及新增来源对话框里的 「POST /session/:id/sources: Invalid title」。
- 打开工作区文件类型的二进制时，会在
GET /file: binary file: /绝对/宿主/路径/...上方留着一行过时的「Loading file…」，且没有下载入口——而同样的字节走上传附件路径时，是一张干净的「Preview is not available for this file type」卡片加下载按钮。绝对宿主路径也原样透出。 - 在 200 条上限时发送附件会静默不登记（
source_limit_reached被有意当作永久失败从重试队列里剔除）。没有任何损失——文件仍以普通上传文件显示——但也完全没有提示。作为设计我认为是合理的，只是提出来确认这份沉默是有意的。
未验证
归属不可用/被替换时的预览改向；「明确拒绝消息时应清理已上传字节」；独立 CLI 的跨客户端来源复制；Windows 与 Linux（仅 macOS）；外部模型提供方。压缩是在小转录上通过 /compress 跑的，所以只证明了「登记元数据在压缩后存活」，没有覆盖大上下文压缩路径。
关于 review 5151984218 的收敛性顾虑
作为一个数据点：这一轮与既有评审轮次相互独立——真 daemon、真浏览器、变异矩阵、文件系统层的故障注入——而且没有产生新的 Critical。它复现了已记录的两条推迟项并把可达性讲清楚了，其余发现都是小问题或后续项。这不能了结关于 diff 体量的争论，但它是对该功能行为的一次完整走查，没有打开新的 fails-closed 面。
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approving. The round-6 incremental review at this head is under the convergence posture: the two remaining Criticals (the Unicode-Cf invisible-character gate at session-sources.ts:85, and the latched recorder write-failure path at :343) are both fails-closed on new surface — no wrong result is certified and the merge base had neither the surface nor the defect — so they are deferred as follow-up work, not blockers.
CI is fully green (Lint, Test, web-shell E2E, Capture visuals, no-AK integration, Desktop Shell, Real daemon E2E, Serve A/B, Live Host). This has run six rounds with the blocking surface shrinking each time; the remaining items are recorded follow-ups.
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
结论 / Verdict: APPROVE（附条件说明 / with explicit caveats）
中文 — head 未变（仍为 fe5ebb17a），与我上一条 COMMENT（review id 5152619692）所钉的 commit 完全一致，其间没有新提交。本次 APPROVE 不改变我此前的技术判断，而是对当前状态做出放行决定，并如实记录仍未关闭的两项。
我此前提出、且在 fe5ebb17a 复核确认依然存在的两条 Important，均未在本 PR 内修复，由维护者显式登记为后续跟进项（非 Critical），依据仓库「审到约第五轮后只落地 Critical 修复」的 late-review 约定放行：
- R2-1 —
qwen/session/sources/*错误压平：acpAgent.ts对任何非SessionSourceError统一返回source_persistence_unavailable，经error-response.ts映射为 503，内部 cause 仅走debugLogger，不经 HTTP 暴露。维护者的范围澄清成立——copy「目标非本会话 fork」是受信任的私有父进程操作，bridge 只消费一次sourceError产出 copy 警告、不重试非法目标；公开侧非法 source 输入本就已映射invalid_source/400。因此这条更准确的定性是诊断/可观测性层面的改进（保留 cause、细化私有 ACP 错误分类，且不泄漏私有子进程路径），并非用户可见的错误状态码缺陷。作为 follow-up 接受。 - R2-2 —
readSessionSources全量物化 transcript：首次访问 source 时以readLinesWithIntegrity(..., Infinity)一次性解析整份 transcript，运行在多个会话共享的单线程 ACP child 上，大历史下可能阻塞其他会话。维护者确认「首次访问确实一次性物化整份 transcript」，且完整审计已声明大历史性能未做新基准。后续若做有界/选择性读取，须单独 benchmark 并保住 fail-closed 完整性规则（最新快照损坏/不支持必须失败关闭，不得回退到删除前的旧快照），以现有 corruption/restore 测试与真实恢复探针为验收基线。作为 follow-up 接受。
放行的其余依据：所有 Critical 级用户可见缺陷均已带回归与真机（真 daemon / 真 ACP child / 真 transcript / 真 Chrome）证据修复；CI 在本 head 全绿（Test、Lint & Static、web-shell E2E Smoke、Integration、Capture web-shell visuals、Real daemon E2E、Desktop Shell 均 pass）；另有维护者 yiliang114 已 APPROVE。
一句话：代码可合并；R2-1/R2-2 是已知、被明确登记、非 Critical 的延后项，不是被忽略或被判为不存在。
English — The head is unchanged (still fe5ebb17a), identical to the commit pinned by my previous COMMENT (review id 5152619692); no new commits since. This APPROVE does not revise my earlier technical assessment — it records a ship decision on the current state, with the two still-open items documented honestly.
The two Important findings I raised, re-verified as still present at fe5ebb17a, are not fixed within this PR and have been explicitly logged by the maintainer as follow-up work (non-Critical), consistent with the repository's late-review convention of landing only Critical fixes after roughly five rounds:
- R2-1 —
qwen/session/sources/*error flattening: inacpAgent.ts, any non-SessionSourceErroris collapsed tosource_persistence_unavailable, mapped byerror-response.tsto 503, with the internal cause only going todebugLoggerand never surfaced over HTTP. The maintainer's scope clarification holds — the copy "target is not a fork of this session" case is a trusted private-parent operation; the bridge consumes thesourceErroronce to produce a copy warning and does not retry the invalid target, while public invalid-source input already maps toinvalid_source/400. So this is more precisely a diagnostics/observability improvement (preserve causes, refine the private-ACP error taxonomy, without leaking private child paths through the HTTP response), not a user-facing wrong-status defect. Accepted as follow-up. - R2-2 —
readSessionSourcesmaterializes the full transcript: first source access parses the entire transcript viareadLinesWithIntegrity(..., Infinity), on the single-threaded ACP child shared by multiple sessions, which can stall other sessions on large histories. The maintainer confirmed first access still materializes the transcript once, and the complete audit explicitly listed large-history performance as not newly benchmarked. Any bounded/selective implementation should be benchmarked separately and preserve the fail-closed integrity rule (a malformed/unsupported newest snapshot must fail closed, never falling back to an older pre-removal snapshot), using the existing corruption/restore tests and real recovery probes as the acceptance baseline. Accepted as follow-up.
Remaining basis for shipping: all user-facing Critical defects are fixed with regression and real-environment (real daemon / real ACP child / real transcript / real Chrome) evidence; CI is fully green at this head (Test, Lint & Static, web-shell E2E Smoke, Integration, Capture web-shell visuals, Real daemon E2E, Desktop Shell all pass); and maintainer yiliang114 has separately APPROVED.
Bottom line: the code is mergeable; R2-1/R2-2 are known, explicitly logged, non-Critical deferred items — not ignored, and not judged absent.
|
Released in v0.23.3. |
6 participants
What this PR does
Adds a unified Sources list for uploaded files, workspace-file references, and HTTP(S) links. Existing attachments remain discoverable without metadata migration, and registered attachment details take precedence without displaying the same file twice. Rows are single-line, default to three items, and offer View all / Collapse.
Adds explicit agent/client source registration, owner-routed session APIs, durable metadata snapshots, and list invalidation events. Registration stores metadata without reading files, fetching URLs, adding resource contents to prompts, or creating output artifacts. File previews reuse the existing session/workspace access checks; source HTML stays text and binary attachments remain downloadable.
Why it's needed
The existing attachment list only covers uploaded files, while workspace documents and links have no unified session reference list. Users need one place to reopen their materials without distinguishing upload storage from optional reference metadata. Keeping registration separate from message delivery also lets a failed metadata write be retried without resending a message.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: uploaded files appeared in Attachments, with no unified registry for workspace references and links. After: one Sources list covers those materials, preserves historical uploads, and uses compact single-line rows with reversible three-item disclosure.
Full repository build and typecheck passed. Focused core, bridge, daemon, SDK and Web Shell regressions passed, including the latest 33 environment-panel checks. Actual local acceptance covered the daemon, ACP child, tool scheduler, recording files, REST/SSE and Chrome: 20 unified-flow checks, three capacity/long-filename checks and two final visual checks passed. The final single-line and 3 → all → 3 interactions were also exercised in Chrome. A separate PR comment records the E2E results and validation boundaries.
Tested on
Environment
Node 22.17.0 and Chrome 152.0.7977.82. Model responses used a deterministic localhost fixture with a test-only credential; the product runtime, scheduling, storage and browser interactions were real. No external model endpoint is claimed as validated.
Risk & Scope
Linked Issues
None.
中文说明
本 PR 的内容
增加统一的“来源”清单，展示上传文件、工作区文件引用和 HTTP(S) 链接。历史附件不需要迁移元数据就能找到；有登记信息的附件优先使用该信息，同一文件不会重复展示。列表统一单行，默认三条，支持“查看全部 / 收起”。
增加 Agent/客户端显式来源登记、按会话归属路由的接口、持久化元数据快照和列表失效通知。登记只保存元数据，不读取文件、不抓取 URL、不把资源内容加入 prompt，也不创建产物。文件预览复用现有会话/工作区访问检查；来源 HTML 保持文本预览，二进制附件支持下载。
为什么需要
现有附件列表只覆盖上传文件，工作区文档和链接没有统一的会话参考资料清单。用户需要一个重新打开资料的入口，无须区分上传存储和可选的引用元数据。登记与消息投递分开后，元数据写入失败可以独立重试，不会重发消息。
审阅验证计划
如何验证
证据（前后对比）
之前：上传文件显示在“附件”，工作区引用与链接缺少统一登记清单。之后：一个“来源”清单覆盖这些资料，保留历史上传文件，以单行展示，并支持三条与全部之间的往返切换。
全仓 build 和 typecheck 通过。Core、bridge、daemon、SDK 与 Web Shell 的定向回归通过，包括最新 33 项环境面板检查。实际本地验收覆盖 daemon、ACP child、工具调度、录制文件、REST/SSE 和 Chrome：统一流程 20 项、容量/长文件名 3 项、最终视觉检查 2 项通过。最后的单行与 3 → 全部 → 3 交互也在 Chrome 中实际验证。独立 PR 评论记录 E2E 结果和验证边界。
验证系统
环境
Node 22.17.0、Chrome 152.0.7977.82。模型回复使用带测试占位凭据的本机确定性 fixture；产品运行时、调度、存储和浏览器交互均真实执行。不声称已验证外部模型端点。
风险与范围
关联 Issue
无。