Skip to content

fix(vscode): restore pre-cutover conversation history in the panel - #11495

Merged
yiliang114 merged 3 commits into
mainfrom
fix/vscode-history-include-legacy-sessions
Sep 10, 2026
Merged

fix(vscode): restore pre-cutover conversation history in the panel#11495
yiliang114 merged 3 commits into
mainfrom
fix/vscode-history-include-legacy-sessions

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

Upgrading the VS Code companion from v0.21.x to v0.23.x made the panel's conversation history come up empty even though every transcript was still on disk. Root cause: the WebShell UI cutover (#9811) moved the panel history to the daemon's session catalog filtered by sourceType: 'vscode', and sessions recorded before creator attribution existed carry no source metadata at all, so the filter excluded every pre-upgrade conversation.

This PR restores those conversations without weakening the source separation the cutover introduced:

  • At bootstrap, the extension host reads the legacy globalState conversation store read-only and ships only the restorable conversation ids (entries that were renamed to a real daemon session id; conv_*/temp* drafts excluded) to the webview as an allowlist.
  • The panel's history loader intersects that allowlist with the daemon's default catalog (where unattributed sessions file server-side) and merges the matches into the history list. The scan is bounded (10 pages × 100) and runs once per bootstrap, so a never-matching id cannot re-page the catalog on every dropdown open.
  • When the user opens one of those sessions, DaemonSessionProvider now forwards the host's existing sessionSourceType on workspace load/resume, and the daemon's existing apply-if-missing restore attribution (applyRestoreSourceIfMissing) stamps it vscode — after one open the session is permanently back in the vscode catalog.

globalState is never written (downgrade keeps working), CLI terminal and browser Web Shell sessions stay out of the panel, and there are no daemon/serve/SDK API changes — the fix rides the existing restore-time attribution field.

Why it's needed

Users upgrading from v0.21.x to v0.23.x lose their entire visible conversation history in the panel (P1, reporter lost 69 conversations with messages). No data was deleted — the new catalog query filtered it out — so the fix only needs to make the existing transcripts visible and resumable again, not migrate anything.

Reviewer Test Plan

How to verify

Component tests cover both new boundaries end to end:

  • packages/vscode-ide-companion: npx vitest run src/webview/EmbeddedApp.test.tsx src/webview/providers/WebViewProvider.test.ts — bootstrap ships only restorable legacy ids (conv_*/temp* excluded, store untouched); the history list merges allowlisted unattributed sessions from the default catalog while excluding non-allowlisted CLI sessions and browser-stamped ones; the legacy scan runs once per bootstrap (mutation-checked: removing the guard turns the new assertion red).
  • packages/web-shell: npx vitest run client/daemon/session/DaemonSessionProvider.test.tsx — workspace restore requests now carry the host sessionSourceType, and standalone restores provably do not (the standalone request type omits source attribution).

Manual spot-check for reviewers who want the real thing: install a v0.21.x companion, have a few conversations, upgrade to a build from this branch, open the panel history — the old conversations are listed again, and opening one keeps it in the history after a reload (now via the vscode catalog). CLI sessions from the same workspace must not appear.

Evidence (Before & After)

Before: EmbeddedApp.loadSessionHistory queried only sourceType: 'vscode'; a pre-upgrade session has sourceType === undefined, so the panel rendered an empty history while ~/.qwen/tmp/<project-hash>/chats/*.jsonl still held every transcript (reporter's downgrade probe in the issue confirms the data survives). After: the new EmbeddedApp test seeds a vscode session, an unattributed allowlisted legacy session, an unattributed CLI session, and a browser-stamped session, and asserts the panel shows exactly the vscode + legacy pair.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Unit/component tests only: env -i npx vitest run per package (clean-env to isolate from a running Qwen Code session).

Risk & Scope

  • Main risk or tradeoff: the legacy scan pages the daemon's default catalog on the first history load after a bootstrap; it is bounded (≤1000 catalog entries), fail-open (a scan error never blocks the ordinary vscode list, and the next open retries), and skipped entirely when no legacy conversations exist.
  • Not validated / out of scope: no real VS Code extension-host E2E (the package has no e2e harness); conversations that only ever existed as empty conv_* drafts have no daemon transcript and stay unlisted; agent-host.db is not involved (it is not the transcript store).
  • Breaking changes / migration notes: none. The legacy globalState data is never modified, and restore-time attribution only fills in missing source metadata through the existing apply-if-missing path — it never overwrites an existing source.

Linked Issues

Fixes #11489

中文说明

这个 PR 做了什么

将 VS Code 插件从 v0.21.x 升级到 v0.23.x 后,面板的会话历史会变成空白,尽管所有 transcript 仍然完好地保存在磁盘上。根因:WebShell UI 切换(#9811)把面板历史列表改成了按 sourceType: 'vscode' 过滤的 daemon 会话目录查询,而在来源标记机制存在之前录制的会话没有任何 source 元数据,因此过滤器把所有升级前的会话全部排除在外。

本 PR 在不削弱切换引入的来源隔离的前提下恢复这些会话:

  • 插件宿主在 bootstrap 时以只读方式读取遗留的 globalState 会话存储,只把可恢复的会话 id(已被重命名为真实 daemon 会话 id 的条目;conv_*/temp* 草稿被排除)作为白名单发给 webview。
  • 面板的历史加载器把该白名单与 daemon 的 default 目录(服务端把无标记会话归入此处)求交集,把命中的会话合并进历史列表。扫描有上限(10 页 × 100 条)且每次 bootstrap 只执行一次,因此永远匹配不到的 id 不会在每次打开下拉框时反复翻页。
  • 当用户打开其中一个会话时,DaemonSessionProvider 现在会在工作区会话的 load/resume 请求中转发宿主已有的 sessionSourceType,daemon 现有的"缺失才补"的恢复归因(applyRestoreSourceIfMissing)会把它标记为 vscode——打开一次之后,该会话就永久回到 vscode 目录中。

globalState 全程不被写入(降级依然可用),CLI 终端会话和浏览器 Web Shell 会话不会进入面板,且没有 daemon/serve/SDK 的 API 变更——修复完全复用现有的恢复时归因字段。

为什么需要

从 v0.21.x 升级到 v0.23.x 的用户会在面板中丢失全部可见的会话历史(P1,报告者丢失了 69 条带消息的会话)。数据从未被删除——只是被新的目录查询过滤掉了——所以修复只需要让现有 transcript 重新可见、可继续,不需要任何数据迁移。

评审验证计划

如何验证

组件级测试端到端覆盖了两个新边界:

  • packages/vscode-ide-companionnpx vitest run src/webview/EmbeddedApp.test.tsx src/webview/providers/WebViewProvider.test.ts——bootstrap 只携带可恢复的遗留 id(排除 conv_*/temp*,且不写存储);历史列表会从 default 目录合并白名单命中的无标记会话,同时排除未在白名单中的 CLI 会话和已打浏览器标记的会话;遗留扫描每次 bootstrap 只跑一次(已做变异验证:去掉守卫后新断言会变红)。
  • packages/web-shellnpx vitest run client/daemon/session/DaemonSessionProvider.test.tsx——工作区会话的恢复请求现在携带宿主的 sessionSourceType,且 standalone 恢复被证明不会携带(standalone 请求类型本身不含来源归因)。

想做真实环境抽查的评审:安装 v0.21.x 插件并进行几轮会话,升级到本分支构建,打开面板历史——旧会话重新出现,打开其中一个后重新加载依然存在(这次走 vscode 目录)。同一工作区的 CLI 会话不应出现。

前后对比证据

修复前:EmbeddedApp.loadSessionHistory 只查询 sourceType: 'vscode';升级前的会话 sourceType === undefined,因此面板渲染出空历史,而 ~/.qwen/tmp/<project-hash>/chats/*.jsonl 里仍保存着全部 transcript(issue 中报告者的降级探测证实数据完好)。修复后:新的 EmbeddedApp 测试构造了一个 vscode 会话、一个白名单命中的无标记遗留会话、一个无标记 CLI 会话和一个浏览器标记会话,并断言面板恰好只显示前两者。

测试平台

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

仅单元/组件测试:每个包内用 env -i npx vitest run(干净环境以隔离正在运行的 Qwen Code 会话)。

风险与范围

  • 主要风险或取舍:遗留扫描会在 bootstrap 后首次加载历史时翻页 daemon 的 default 目录;它有上限(≤1000 条目录项)、失败开放(扫描错误不会阻塞常规 vscode 列表,下次打开会重试),且不存在遗留会话时完全跳过。
  • 未验证/超出范围:未做真实 VS Code 插件宿主 E2E(该包没有 e2e harness);只以空 conv_* 草稿存在过的会话没有 daemon transcript,保持不可见;agent-host.db 与本修复无关(它不是 transcript 存储)。
  • 破坏性变更/迁移说明:无。遗留 globalState 数据从不被修改;恢复时归因只通过现有的"缺失才补"路径填充缺失的来源元数据,绝不覆盖已有来源。

关联 Issue

Fixes #11489

The WebShell cutover (#9811) switched the companion's history list to the
daemon catalog filtered by sourceType 'vscode'. Sessions recorded before
creator attribution existed carry no source metadata, so upgrading to
v0.23.x emptied the panel history even though every transcript is still on
disk (#11489).

Claim the panel's own legacy sessions back instead of widening the filter:
the host ships its globalState conversation ids as an allowlist, the panel
intersects it with the daemon's default (unattributed) catalog, and opening
one of them lazily stamps the 'vscode' source through the existing
restore-time attribution path. globalState stays untouched for downgrade
safety, and unattributed CLI/browser sessions stay out of the panel.
Review feedback: the allowlist never converges for ids that can no longer
match (other workspaces' conversations, deleted transcripts, sessions
already stamped by a restore), so keying the scan to every history-dropdown
open re-paged the default catalog indefinitely. Gate it on a per-bootstrap
done flag instead; failures still retry on the next open. Also unpin the
scan page size from the test.
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on 4e00d72 and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— 4e00d72 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every required section is filled in, including the Tested-on table and the Risk & Scope bullets.

Problem: observed, not theoretical. #11489 is a real P1 report with a downgrade/upgrade cross-check (135 conversations still in state.vscdb, 69 with messages, invisible after the upgrade), and you confirmed the root cause in that thread. That is about as well-evidenced as a bug report gets.

Direction: aligned, and the part I like is that it does not add a migration. I traced the mechanism instead of taking the description's word for it, and every hop already exists on main:

  • the daemon's session list deliberately folds unattributed sessions into the default catalog (session-list.ts:310-314, comment reads "Legacy sessions without source metadata belong to the default catalog");
  • the restore route applies the caller's source only when the persisted transcript has none (routes/session.ts:3866-3874), with applyRestoreSourceIfMissing behind it (bridge.ts:7250);
  • RestoreSessionRequest.sourceType is already in the SDK and already documented as "Restore-time attribution for legacy/unattributed sessions", including the capability fail-soft (DaemonClient.ts:3789-3801).

So this PR wires the host into infrastructure that was built for exactly this case, and changes no daemon/serve/SDK API. Reading globalState read-only is also the right call — it keeps the reporter's downgrade path working.

Size: this is a cross-package change (companion + web-shell), so Stage 0 applies. 149 production lines against 314 test lines and nothing generated. Under the 500-line bar, fix type — no escalation on size.

Approach: the scope feels right and I could not find a materially simpler path. Dropping the sourceType: 'vscode' filter would leak CLI and browser sessions into the panel, and the legacy id list is genuinely the only signal that separates a pre-cutover VS Code conversation from a pre-cutover CLI one in the same workspace — the legacy Conversation record carries no cwd. Two things I'd think about before merge, neither a blocker:

  • The allowlist is global rather than workspace-scoped (it cannot be, given the record shape), so the remaining.size > 0 early exit almost never fires for anyone who used more than one project. The bounded worst case is the common case, not the edge case.
  • Nothing ever retires the allowlist, so the scan re-runs on the first history open after every bootstrap, indefinitely, for anyone who used v0.21.x — including long after everything was recovered. Defensible given the read-only intent; worth a conscious decision rather than a default.

Risk: no elevated risk signals — no Stage 1e high-risk path matched.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必填小节都写了,包括测试平台表格和风险与范围。

问题:是已观测到的真实问题,不是理论性加固。#11489 是 P1 报告,报告者做了降级/升级交叉验证(state.vscdb 里仍有 135 条会话、69 条带消息,升级后全部不可见),你也在那个 thread 里确认了根因。证据相当充分。

方向:对齐。我最认可的一点是它没有引入迁移逻辑。我逐跳核对了机制而不是只看描述,main 上每一跳都已存在:

  • daemon 的会话列表刻意把无归因会话归入 default 目录(session-list.ts:310-314,注释就写着"没有 source 元数据的遗留会话属于 default 目录");
  • 恢复路由只在持久化 transcript 没有 source 时才应用调用方传入的 source(routes/session.ts:3866-3874),背后是 applyRestoreSourceIfMissingbridge.ts:7250);
  • SDK 里 RestoreSessionRequest.sourceType 早已存在,注释即为"遗留/无归因会话的恢复时归因",并且已有 capability 降级处理(DaemonClient.ts:3789-3801)。

所以本 PR 是把宿主接到本就为此场景准备的基础设施上,且不改 daemon/serve/SDK 的任何 API。globalState 只读也是正确选择——保住了报告者的降级退路。

规模:跨包改动(companion + web-shell),触发 Stage 0。生产代码 149 行,测试 314 行,无生成/schema 文件。低于 500 行阈值,类型为 fix——不因规模升级。

方案:范围合理,我没有找到明显更简的路径。去掉 sourceType: 'vscode' 过滤会把 CLI 和浏览器会话泄进面板;而遗留 id 列表确实是同一 workspace 下区分"切换前的 VS Code 会话"和"切换前的 CLI 会话"的唯一信号——遗留 Conversation 记录里没有 cwd。合并前有两点建议思考,都不是阻塞项:

  • 白名单是全局的而非按 workspace(受记录结构所限,也只能如此),因此对用过多个项目的用户来说,remaining.size > 0 的提前退出几乎永远不会触发。有上限的最坏情况其实是常态,不是边缘情况。
  • 白名单永远不会被清理,所以只要用过 v0.21.x,每次 bootstrap 后的首次历史加载都会重跑扫描,并且在一切都已恢复之后依然如此。考虑到只读的设计意图这可以接受,但希望是一个明确的决定,而不是默认结果。

风险:无升级风险信号——未命中 Stage 1e 的高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 4e00d72055dd907e5243bd7ae6048e32af0464c1 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head 35d96aa. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

ℹ️ No screenshot changed against the PR base — but this PR edits 2 render-shaping files:

  • packages/web-shell/client/components/WorkspaceSessionProvider.tsx
  • packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx

Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to packages/web-shell/client/e2e/visuals/screenshots.spec.ts that seeds whatever state the UI is gated on; it then appears here as a head-only (NEW) capture.

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Code review

Before reading the diff I wrote down what I would have done from the title and the motivation alone: the panel filters the daemon catalog by sourceType: 'vscode', pre-cutover sessions carry no source, so either relax the filter for unattributed rows or stamp attribution onto them. Relaxing is wrong on its own — the daemon is shared with the CLI and the browser for the same workspace, so unattributed rows include terminal sessions the user never opened in the panel — and the only client-side signal that separates them is the extension's own legacy record. So I landed on the same shape you did: an allowlist out of globalState, intersected with the default catalog, plus restore-time attribution so each session is reclaimed once. No simpler path that I can see.

No critical blockers. One suggestion I'd act on, and two notes.

The first history load now waits for the scan. In loadSessionHistory the await loadLegacyAllowlistedSessions(...) sits before the single setSessions call, so the ordinary vscode page — already in hand — is not rendered until up to ten sequential catalog round-trips finish. Setting sessions from pageSessions first and merging the legacy rows in a second update costs one extra render and keeps the dropdown instant. As written, the latency lands exactly on the users this PR exists for, on the first thing they do after upgrading.

The early exit almost never fires, and nothing retires the allowlist. Both covered in the gate comment; together they mean the bounded worst case is the typical case, repeated on every bootstrap indefinitely. Not a defect — the record shape leaves no workspace to scope by, and keeping globalState untouched is a deliberate trade for the downgrade path — but it is a cost that never goes away, so it should be a decision rather than a default.

Three other <DaemonSessionProvider> call sites do not forward the new prop (SideTaskPanel.tsx:85, SubagentDetail.tsx:376, SplitView.tsx:601). Safe — the prop is optional and the daemon only fills missing attribution, so those paths behave exactly as they do today — but it does mean the panel's main provider is the only surface that reclaims a legacy session. Worth one line somewhere so the next reader does not think it was overlooked.

Things I verified rather than assumed, since most of them are outside the diff:

  • The standalone guard is type-motivated, not decorative. BridgeStandaloneRestoreSessionRequest is Omit<…, 'sourceType' | 'sourceId'>, and one restoreRequest object feeds both branches; effectSessionContext?.kind !== 'standalone' is exactly the complement of the branch that calls loadStandalone/resumeStandalone, including the reconnectSessionId path.
  • 'vscode' cannot 400 a restore: parseSessionSource accepts [a-z][a-z0-9_-]{0,63}, and it is neither the reserved standalone nor the reserved live source. This was the one way the change could have broken session loading outright, so I checked it specifically.
  • sessionSourceTypeRef mirroring is right — the prop in the effect deps would re-trigger a session load whenever the host re-renders.
  • pageSize: 100 equals the server's MAX_SESSION_PAGE_SIZE, so the advertised bound really is 1000 catalog rows.
  • The allowlist premise holds: SessionMessageHandler.ts:1003-1012 renames the conversation-store id to the ACP session id after the first message, so legacy conversations that actually exchanged prompts do carry daemon session ids, and getRestorableDaemonSessionId filters precisely the conv_*/temp* drafts that never did.
  • The bootstrap needs no handler change: the webview casts the whole payload and the host posts an untyped literal, so the field flows through — and is correctly absent rather than [] when there is nothing to restore, which matters because the panel gates on !== undefined && length > 0.
  • Appended legacy rows land in the dropdown's "Older" bucket, which groups by timestamp, so the merge order does not scramble the list.
  • Only ids cross the bridge, never message bodies — the right call given the reporter's store is 5.7 MB of prompt content.
sequenceDiagram
    participant P1 as VS Code extension host
    participant P2 as Panel webview
    participant P3 as Daemon session catalog
    P1->>P1: read legacy globalState (read-only)
    P1->>P2: bootstrap with legacyConversationIds
    P2->>P3: list page, sourceType vscode
    P3-->>P2: attributed sessions
    P2->>P3: list pages, sourceType default (max 10)
    P3-->>P2: unattributed, CLI and browser sessions
    P2->>P2: keep allowlisted rows with no sourceType
    P2->>P3: open session, restore with sourceType vscode
    P3->>P3: stamp vscode only if source is missing
Loading

On the tests: the three new behaviors are pinned and the assertions test the right things. The second half of the EmbeddedApp test — two more dropdown opens, then default calls still exactly 1 while vscode calls are ≥ 2 — is what makes the once-per-bootstrap guard load-bearing instead of decorative. The WebViewProvider pair pins both the id filter and the absent-not-empty contract; the provider pair pins attribution on workspace restore and its absence on standalone.

Test evidence

This was an unattended CI run, so nothing was built, executed, or installed here — no PR-derived code ran. The evidence below is the PR's own CI, read through the API for the reviewed commit. Not verified: any real extension-host or daemon behavior, for the reason given under the next heading.

At review time Qwen Code CI (Test ubuntu-latest Node 22.x, Lint & Static) and Web-shell Visuals were still running and nothing had failed. review-pr and triage are bot orchestration jobs on the same head SHA, not PR CI. verify, tmux-testing and the publish-* jobs are skipped because they wait on a maintainer comment trigger, and the macOS/Windows Test jobs are skipped in this matrix.

Final CI results for 4e00d72 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

What the suite cannot show, and the lane that would. The EmbeddedApp test mocks listWorkspaceSessionsPage, so it proves the client-side merge and exclusion rules but not the load-bearing half of the claim: that a real daemon actually files this reporter's pre-upgrade transcripts under sourceType: 'default' for that workspace, and that opening one stamps it vscode. The server branch exists and is commented as intentional (session-list.ts:313), but the suite passes identically whether or not it matches those sessions. Your own Tested-on row says it too — component tests only, no extension-host E2E, because the package has no harness.

Sandboxed verification would settle this: @qwen-code /verify — an A/B against the base build is what would show the panel's history actually changing for a workspace holding unattributed pre-cutover transcripts, which no mocked test here can. You have write access, so /tmux is available too, but it drives the TUI and this surface is a VS Code panel, so /verify is the lane that fits.

中文说明

代码审查

在读 diff 之前,我只根据标题和动机写下了自己的方案:面板按 sourceType: 'vscode' 过滤 daemon 目录,切换前的会话没有 source,那要么放宽过滤让无归因行进来,要么给它们补上归因。单纯放宽是错的——daemon 与同一 workspace 的 CLI 和浏览器共享,无归因行里包含用户从未在面板中打开过的终端会话——而客户端唯一能区分它们的信号就是插件自己的遗留记录。所以我得到的形态和你一致:从 globalState 取白名单,与 default 目录求交集,再加恢复时归因使每个会话只被认领一次。我没有看到更简的路径。

没有阻塞性问题。 一条建议我会真的改,另有两条说明。

首次加载历史现在要等扫描完成。 loadSessionHistoryawait loadLegacyAllowlistedSessions(...) 位于唯一的 setSessions 之前,因此已经拿到手的 vscode 分页结果要等最多十次串行目录请求跑完才渲染。先用 pageSessions 设置一次、再把遗留行合并进去,只多一次渲染,就能让下拉框保持即时。按现在的写法,这段延迟恰好落在本 PR 要服务的用户身上,且发生在他们升级后做的第一件事上。

提前退出几乎不会触发,且白名单永不退役。 两点都在 gate 评论里说过;合起来意味着有上限的最坏情况就是常态,并且每次 bootstrap 都会重演、永不结束。这不是缺陷——记录结构里没有可用于限定的 workspace,保持 globalState 不动是为降级路径做的明确取舍——但这是一笔永不消失的成本,所以它应该是一个决定,而不是默认结果。

另外三处 <DaemonSessionProvider> 没有转发新 propSideTaskPanel.tsx:85SubagentDetail.tsx:376SplitView.tsx:601)。这是安全的——prop 可选,daemon 只补缺失归因,所以这些路径与今天行为完全一致——但确实意味着只有面板的主 provider 会认领遗留会话。建议在某处补一句说明,免得后来的读者以为是漏掉了。

以下是我实际核对过(而非假设)的点,其中大部分在 diff 之外:

  • standalone 守卫是类型驱动的,不是装饰性的。BridgeStandaloneRestoreSessionRequestOmit<…, 'sourceType' | 'sourceId'>,而两个分支共用同一个 restoreRequest 对象;effectSessionContext?.kind !== 'standalone' 恰好是调用 loadStandalone/resumeStandalone 分支的补集,reconnectSessionId 路径也一样。
  • 'vscode' 不会让恢复请求 400:parseSessionSource 接受 [a-z][a-z0-9_-]{0,63},它既不是保留的 standalone source,也不是保留的 live source。这是本改动唯一可能彻底弄坏会话加载的地方,所以我专门查了。
  • sessionSourceTypeRef 镜像是对的——把该 prop 放进 effect 依赖会导致宿主每次重渲染都重新触发一次会话加载。
  • pageSize: 100 等于服务端 MAX_SESSION_PAGE_SIZE,所以宣称的 1000 行上限是真实的。
  • 白名单的前提成立:SessionMessageHandler.ts:1003-1012 在首条消息后把会话存储 id 重命名为 ACP 会话 id,因此真正交互过的遗留会话确实携带 daemon 会话 id,而 getRestorableDaemonSessionId 过滤掉的正好是从未交互过的 conv_*/temp* 草稿。
  • bootstrap 不需要改 handler:webview 直接整体断言 payload,宿主发的是无类型字面量,所以新字段能流通——并且在无可恢复内容时是字段缺失而不是 [],这一点很关键,因为面板的判断是 !== undefined && length > 0
  • 追加的遗留行会落入下拉框的"更早"分组(按时间戳分组),所以合并顺序不会打乱列表。
  • 过桥的只有 id,没有消息正文——考虑到报告者那份 5.7 MB 存储里全是 prompt 内容,这个选择是对的。

关于测试:三个新行为都被钉住了,断言也断在了正确的地方。EmbeddedApp 测试的后半段——再开两次下拉框,然后 default 调用仍恰好为 1 而 vscode 调用 ≥ 2——正是让"每次 bootstrap 只扫一次"这个守卫成为承重结构而非装饰的部分。WebViewProvider 那对测试同时钉住了 id 过滤和"缺失而非空数组"的契约;provider 那对钉住了工作区恢复携带归因、standalone 恢复不携带。

测试证据

本次为无人值守 CI 运行,因此这里没有构建、执行或安装任何东西——没有运行任何来自 PR 的代码。下面的证据是 PR 自己的 CI,通过 API 读取被审查 commit 的结果。未验证:任何真实的插件宿主或 daemon 行为,原因见下一节。

审查时 Qwen Code CI(Test ubuntu-latest Node 22.x、Lint & Static)与 Web-shell Visuals 仍在运行,没有任何失败。review-prtriage 是同一 head SHA 上的机器人编排任务,不属于 PR CI。verifytmux-testingpublish-* 显示 skipped,因为它们要等维护者评论触发;macOS/Windows 的 Test 任务在此矩阵中也是 skipped

(上方表格中的检查名与结论即为 CI 现状,由 finalize 任务在 CI 结束后原地更新。)

测试套件无法证明的部分,以及能证明它的路径。 EmbeddedApp 测试 mock 了 listWorkspaceSessionsPage,所以它证明的是客户端的合并与排除规则,而不是这个主张中承重的那一半:真实 daemon 是否确实把该报告者升级前的 transcript 归入那个 workspace 的 sourceType: 'default',以及打开其中一个是否会被标记为 vscode。服务端那个分支存在且注释表明是有意为之(session-list.ts:313),但无论它是否命中这些会话,本套件的结果都一样。你的"测试平台"一栏也说明了这点——只有组件测试,没有插件宿主 E2E,因为该包没有 harness。

沙箱验证可以定这件事:@qwen-code /verify —— 与 base 构建做 A/B,才能看出当一个 workspace 里存在无归因的切换前 transcript 时,面板历史是否真的发生了变化,这是这里任何 mock 测试都做不到的。你有写权限,所以 /tmux 也可用,但它驱动的是 TUI,而本 PR 的界面是 VS Code 面板,因此合适的路径是 /verify

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 4e00d72055dd907e5243bd7ae6048e32af0464c1 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — the wiring is correct at every hop I could check statically and it adds no new mechanism; the missing fifth is that nobody has yet watched a real daemon hand back a real pre-cutover transcript.

Stepping back: the best thing about this PR is what it does not contain. There is no migration, no new daemon route, no SDK surface, nothing written to a store the user might still downgrade into. I went in expecting a recovery subsystem and found 149 production lines of wiring onto branches that already existed for exactly this case — including a server-side comment that says out loud that unattributed sessions belong to the default catalog. If I pick this up in six months it reads as "the host tells the daemon who it is on restore", which is a sentence I can hold in my head. That is the shape that ages well.

I also went looking for the way this breaks something, since a change to the restore request has a nasty failure mode available to it. The one that would have been serious is a 400 on restore — parseRequestedSessionSource rejects reserved sources before the bridge ever sees the request, and a rejected restore means the panel cannot open sessions at all. 'vscode' is neither reserved standalone nor reserved live and matches the source pattern, and the panel already creates sessions under that same value today, so the gate is proven by production traffic rather than by my reading alone. The second failure mode I looked for is CLI and browser sessions leaking into the panel, which is the whole reason the sourceType: 'vscode' filter exists. The server's default catalog returns explicitly-stamped default rows alongside unattributed ones, and the client keeps only sourceType === undefined — that check is the load-bearing line in the scan, and the new test pins it with a browser-stamped row that must not appear.

What I would still want before calling this done, in descending order of how much it matters:

  1. Somebody watches it work once against a real daemon. The suite mocks the catalog client, so it cannot distinguish "the merge logic is right" from "the reporter's 69 conversations come back". @qwen-code /verify is the lane; the claim it would settle is named in the review comment above.
  2. Render the ordinary page before awaiting the scan. Small change, and it removes the only regression this PR introduces for the users it is trying to help.
  3. Decide consciously that the scan runs on every bootstrap forever. It is defensible; it just should not be an accident of keeping globalState read-only.

None of those are reasons to hold the PR back, and I am not approving because I ran out of objections — I have three, and all three are things you can fix in a follow-up or wave off with a sentence.

CI on this commit was still running when I finished (Qwen Code CI and Web-shell Visuals; nothing red, no failure to explain), so approval is deferred until CI lands green on 4e00d72055dd907e5243bd7ae6048e32af0464c1 — the finalize job posts the commit-pinned approval then, and withholds it if anything fails or the head moves.

中文说明

置信度:4/5 —— 凡是能静态核对的每一跳,接线都是正确的,而且它没有引入任何新机制;差的这一分在于:还没有人看着真实 daemon 把一份真实的切换前 transcript 交回来。

退一步看:这个 PR 最好的地方是它没有包含什么。没有迁移,没有新的 daemon 路由,没有新的 SDK 接口,也没有向用户可能还要降级回去的存储写入任何东西。我本来以为会看到一个恢复子系统,结果是 149 行生产代码,接到了本就为这个场景准备的分支上——服务端甚至有一句注释明确写着无归因会话属于 default 目录。半年后我再拿起它,读到的是"宿主在恢复时告诉 daemon 自己是谁",这是一句我能装进脑子里的话。这种形态才经得起时间。

我也专门去找它会弄坏什么,因为改动恢复请求有一个很难看的失败模式可用。真正严重的那个是恢复请求 400——parseRequestedSessionSource 会在 bridge 看到请求之前就拒绝保留 source,而恢复被拒意味着面板根本打不开会话。'vscode' 既不是保留的 standalone 也不是保留的 live,并且符合 source 格式;而且面板今天已经在用同一个值创建会话,所以这道门是被生产流量证明的,不只是被我读代码证明的。我找的第二个失败模式是 CLI 和浏览器会话泄进面板——那正是 sourceType: 'vscode' 过滤存在的全部理由。服务端的 default 目录会把显式标记为 default 的行和无归因行一起返回,而客户端只保留 sourceType === undefined 的行——这个判断是扫描里承重的那一行,新测试用一条"必须不出现"的浏览器标记行把它钉住了。

在称之为完成之前,我仍然希望做到以下几点,按重要性递减:

  1. 有人对着真实 daemon 看它跑通一次。套件 mock 了目录客户端,所以它无法区分"合并逻辑是对的"和"报告者那 69 条会话回来了"。路径是 @qwen-code /verify;它要定的那个主张在上面的审查评论里写明了。
  2. 在等待扫描之前先渲染常规分页。改动很小,却能消除本 PR 给它想帮助的用户带来的唯一一处退化。
  3. 明确决定"扫描会在每次 bootstrap 永久运行"这件事。这是可以接受的;它只是不应该成为"保持 globalState 只读"的一个意外副作用。

这些都不是拦下这个 PR 的理由,我也不是因为找不到反对意见才批准——我有三条,而这三条都可以在后续 PR 里修掉,或者用一句话带过。

我完成审查时该 commit 的 CI 仍在运行(Qwen Code CIWeb-shell Visuals;没有红灯,也没有需要解释的失败),因此批准推迟到 CI 在 4e00d72055dd907e5243bd7ae6048e32af0464c1 上全绿之后——届时由 finalize 任务发出绑定该 commit 的批准;若有检查失败或 head 变动,则不会批准。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 4e00d72055dd907e5243bd7ae6048e32af0464c1 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs.

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the fix end-to-end against #11489's root cause — the chain holds at every link:

  1. Bootstrap: WebViewProvider reads the legacy globalState store read-only and ships only getRestorableDaemonSessionId-filtered ids (conv_*/temp* drafts excluded) on webShellBootstrap. Matches the reporter's downgrade probe: the data was never lost, just filtered out.
  2. List: the server already files unattributed sessions under the default catalog (packages/cli/src/serve/server/session-list.tsfilter.sourceType === 'default' && session.sourceType === undefined), so the allowlist intersect recovers exactly the pre-cutover companion sessions, while the client-side session.sourceType === undefined check keeps default-stamped sessions (scheduled task runs etc.) out. CLI sessions without attribution are excluded by the allowlist itself.
  3. Restore: the session route's restoreRequestMetadata prefers the transcript-persisted source and only falls back to the client-supplied one when none exists, and applyRestoreSourceIfMissing stamps only when entry.sourceType === undefined — existing attribution is never overwritten, and one open permanently re-homes the session into the vscode catalog.

Local verification at 35d96aa (worktree build + targeted runs):

  • packages/vscode-ide-companion: EmbeddedApp.test.tsx + WebViewProvider.test.ts — 92 passed.
  • packages/web-shell: DaemonSessionProvider.test.tsx — 320 passed.
  • Mutation check: removing the legacyScanDoneRef guard turns the new scans-once-per-bootstrap assertion red, so the convergence test isn't vacuous; restoring the guard goes green again.

Nothing blocking from my side. One non-blocking observation: the legacy scan is bounded to the newest ~1000 default-catalog entries (10 pages × 100), so a workspace with more unattributed sessions than that would stop recovering the oldest ones — the bound is documented in the PR and comfortable for realistic histories.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R1-13 history page withheld behind the legacy scan — already reported (issue comment 5605192247, triage stage 2; repeated as item 2 in 5605207884)
  • R1-14 recovery never retired, re-paid on every bootstrap — already reported (issue comment 5605192247, triage stage 2; repeated as item 3 in 5605207884)

Test Plan (not a blocker): src/webview/EmbeddedApp.test.tsxno such file or directory; src/webview/providers/WebViewProvider.test.tsno such file or directory; client/daemon/session/DaemonSessionProvider.test.tsxno such file or directory.

中文说明

已审查——无阻断问题。 建议见行内评论。

本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

Test Plan(非阻断):src/webview/EmbeddedApp.test.tsxno such file or directory; src/webview/providers/WebViewProvider.test.tsno such file or directory; client/daemon/session/DaemonSessionProvider.test.tsxno such file or directory

— qwen3.8-max via Qwen Code /review (v0.23.2)

// legacy ids it still has in globalState; the panel then claims exactly
// those sessions back from the daemon's default catalog — without
// surfacing unattributed CLI sessions or browser-stamped ones.
sdkMocks.listWorkspaceSessionsPage.mockImplementation(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This mockImplementation outlives the test that installs it. The file's beforeEach calls vi.clearAllMocks() (:156-161), which clears call history but does not remove implementations — only mockReset() / vi.resetAllMocks() does. Since this is the last test in describe('EmbeddedApp host wiring') (the block ends at :958), the fake catalog (vscode-1 for the vscode filter; legacy-1/cli-1/web-1 for default) plus the branching implementation stay installed for all of describe('web shell permission decision messages') from :961 onward and for anything appended later. A future test in that block which opens the history dropdown inherits three sessions it never arranged for, and passes or fails for reasons nowhere in its own body. Nothing is red today — listWorkspaceSessionsPage is not referenced after :950 — so this is a latent hazard with a concrete cost rather than a live failure; the file's own leakage-safe convention is mockResolvedValueOnce at :762. Add sdkMocks.listWorkspaceSessionsPage.mockReset() to the beforeEach, or wrap this test body in try { … } finally { sdkMocks.listWorkspaceSessionsPage.mockReset(); }.

Witness:

probe in an isolated vitest project (this repo's vitest 3.2.7) mirroring this file's shape —
hoisted sdk mock, beforeEach cleanup, implementation installed by the last test of block A
and observed from block B:
ARM 1  vi.clearAllMocks()   (what :157 does)
       implementationStillInstalled: true, callHistoryClearedByBeforeEach: 0
       vscodePage.sessions: [vscode-1]   defaultPage.sessions: [legacy-1, cli-1]
       Tests 2 passed (2)
ARM 2  vi.resetAllMocks()   (the suggested fix family)
       probe FLIPS: expect(implInstalled).toBe(true) fails
       Tests 1 failed | 1 passed (2)

The cleanup in this file is entirely explicit, so the reset has to be added rather than configured: beforeEach at :156-161 calls vi.clearAllMocks() only, and packages/vscode-ide-companion/vitest.config.ts sets none of restoreMocks, mockReset or clearMocks.

merges allowlisted pre-cutover sessions into the history list must stay green once the reset is added — it depends on the implementation being live for its own duration — so the mutation to run is removing the reset again and confirming the leakage returns.

中文说明

这个 mockImplementation 的生命周期超出了安装它的测试。本文件的 beforeEach 调用的是 vi.clearAllMocks()(:156-161),它只清空调用历史,并不会移除 implementation——只有 mockReset() / vi.resetAllMocks() 才会。由于这是 describe('EmbeddedApp host wiring') 里的最后一个测试(该 block 到 :958 结束),这份假目录(vscode 过滤返回 vscode-1;default 返回 legacy-1/cli-1/web-1)连同按 sourceType 分支的 implementation,会继续留在 :961 之后的整个 describe('web shell permission decision messages') 以及之后新增的任何测试里。该 block 中若有测试打开历史下拉框,就会继承三个它从未安排的会话,并因自身代码里找不到原因的理由通过或失败。今天没有测试变红——:950 之后再没有引用 listWorkspaceSessionsPage——所以这是一个有明确代价的潜在隐患,而不是已经发生的失败;本文件自己的防泄漏写法是 :762 的 mockResolvedValueOnce。建议在 beforeEach 中加上 sdkMocks.listWorkspaceSessionsPage.mockReset(),或把本测试主体包进 try { … } finally { sdkMocks.listWorkspaceSessionsPage.mockReset(); }

证据:在隔离的 vitest 工程(本仓库的 vitest 3.2.7)中按本文件形态做探针——hoisted sdk mock、beforeEach 清理、由 block A 最后一个测试安装 implementation 并从 block B 观察。第一组用 vi.clearAllMocks()(即 :157 的做法):implementation 仍然安装着(implementationStillInstalled: true),调用历史被清空,后续 block 仍拿到 vscode-1legacy-1/cli-1,2 个测试全绿。第二组改用 vi.resetAllMocks()(即建议的修复族):探针翻转,expect(implInstalled).toBe(true) 失败,1 失败 1 通过。

约束:本文件的 mock 清理完全是显式的,所以这个 reset 必须手写加上而不能靠配置——:156-161 的 beforeEach 只调用了 vi.clearAllMocks(),而 packages/vscode-ide-companion/vitest.config.ts 没有设置 restoreMocksmockResetclearMocks 中的任何一个。

验收:加上 reset 之后 merges allowlisted pre-cutover sessions into the history list 必须仍然是绿的(它依赖 implementation 在自身执行期间有效),因此要跑的变异是再次移除 reset,确认泄漏重新出现。

— qwen3.8-max via Qwen Code /review (v0.23.2)

workspaceCwd: '/workspace',
sessionId: 'session-1',
hostKind: 'panel',
legacyConversationIds: ['legacy-1', 'never-recorded'],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The scan's session.sourceType === undefined discriminator — the line that keeps CLI and browser sessions out of the panel history — is not pinned by any test, so it can be deleted with the whole package suite unchanged. In this fixture the only allowlisted id present in the default catalog (legacy-1) carries no sourceType, so cli-1 and web-1 are excluded by the allowlist alone and their toBeNull() assertions at :917-918 pass either way. Removing the clause from EmbeddedApp.tsx:88 then ships the regression it exists for: a pre-cutover conversation that another surface stamped default gets pulled into the panel's vscode history — a session owned elsewhere appearing as restorable in VS Code — and 519 passing tests would not notice. The stamp is one-way (bridge.ts:7276-7282 only fills a missing source), and session-list.ts:311-315 files unattributed rows into the same default bucket the scan queries, so a stamped legacy row is producible by any host that sets sessionSourceType. Adding web-1 to the allowlist makes the exclusion assertion that already exists do the pinning; the short-circuit means an allowlisted-but-stamped id never leaves remaining.

Suggested change
legacyConversationIds: ['legacy-1', 'never-recorded'],
legacyConversationIds: ['legacy-1', 'web-1', 'never-recorded'],

Witness:

four arms measured in an isolated copy of the reviewed commit:
pristine                          -> Tests 20 passed (20)
delete the sourceType clause      -> Tests 20 passed (20)   mutation SURVIVES
whole package, pristine vs mutant -> byte-identical failure sets
                                     Tests 5 failed | 519 passed | 1 skipped (525)
                                     (all 5 in src/ide-server.test.ts, environment-induced)
suggested fixture alone           -> Tests 20 passed (20), and the
                                     ).toHaveLength(1); convergence assertion still green
suggested fixture + mutation      -> Tests 1 failed | 19 passed (20)
                                     expected <div role="option" …> to be null
                                     at :918 for data-session-id="web-1"

if (!result.nextCursor) break; at EmbeddedApp.tsx:94 is what keeps the ).toHaveLength(1); convergence assertion at one request — measured to still hold with this fixture change, because nextCursor: undefined trips the break after one page even though web-1 now stays in remaining forever; pairing the same change with a nextCursor would turn that assertion into ten calls.

The mutation to run is deleting session.sourceType === undefined && from EmbeddedApp.tsx:88: with this fixture the web-1 toBeNull() assertion at :918 must go red, and it must go green again once the clause is restored.

中文说明

扫描里的 session.sourceType === undefined 判断——也就是把 CLI 与浏览器会话挡在面板历史之外的那一行——没有任何测试钉住它,因此把它删掉整个包的测试套件也不会有变化。在这份 fixture 里,default 目录中唯一出现在白名单里的 id(legacy-1)本身不带 sourceType,所以 cli-1web-1 只靠白名单就被排除了,:917-918 的 toBeNull() 断言在两种情况下都会通过。一旦从 EmbeddedApp.tsx:88 去掉这个条件,就会放走它本来要防的回归:一个被其他界面标记为 default 的切换前会话会被拉进面板的 vscode 历史里——一个属于别处的会话在 VS Code 中显示为可恢复——而 519 个通过的测试不会察觉。这个标记是单向的(bridge.ts:7276-7282 只补缺失的 source),且 session-list.ts:311-315 把无归因行归入扫描所查询的同一个 default 桶,所以任何设置了 sessionSourceType 的宿主都可能造出一条已标记的遗留行。把 web-1 加进白名单,就能让已经存在的排除断言真正起到钉住作用;由于短路求值,一个在白名单里但已被标记的 id 永远不会离开 remaining

证据:在被审 commit 的隔离副本中测了四组。原始代码:20 个测试全绿。删掉 sourceType 条件:仍然 20 个全绿,变异存活。整包对比原始与变异:失败集合逐字节相同(Tests 5 failed | 519 passed | 1 skipped (525),5 个都在 src/ide-server.test.ts,由副本环境导致)。只加建议的 fixture:20 个全绿,且 ).toHaveLength(1); 收敛断言仍然是绿的。建议 fixture + 变异:Tests 1 failed | 19 passed (20),:918 处 data-session-id="web-1"expected <div role="option" …> to be null

约束:EmbeddedApp.tsx:94if (!result.nextCursor) break; 是让 ).toHaveLength(1); 收敛断言保持一次请求的原因——实测在这样改 fixture 之后依然成立,因为 nextCursor: undefined 会在一页之后触发 break,即便 web-1 从此永远留在 remaining 里;如果把同样的改动和 nextCursor 一起加,该断言就会变成十次调用。

验收:要跑的变异是从 EmbeddedApp.tsx:88 删掉 session.sourceType === undefined &&——加上这份 fixture 后,:918 的 web-1 toBeNull() 断言必须变红,恢复该条件后必须重新变绿。

— qwen3.8-max via Qwen Code /review (v0.23.2)

Comment on lines +50 to +51
const LEGACY_SESSION_SCAN_PAGE_SIZE = 100;
const LEGACY_SESSION_SCAN_MAX_PAGES = 10;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This window is taken from the wrong end of the catalog, and giving up is recorded as success. The metadata-filtered listing sorts strictly newest-first (session-list.ts:799-805, b.activityTime - a.activityTime) and the default bucket holds every unattributed row plus every default-stamped CLI, browser and scheduled-task session in the workspace — while the conversations this scan exists to recover are the oldest rows in it. A workspace that has accumulated more than ~1000 newer default-catalog sessions (a long-time CLI user in the same folder, which is exactly the population that has pre-cutover panel history) never reaches them: the loop exits on LEGACY_SESSION_SCAN_MAX_PAGES, legacyScanDoneRef.current = true at :527 still runs because the helper returned normally, and those conversations stay absent from the panel history with no error, no log and no retry — indistinguishable from "there was nothing to recover", for the users with the most to recover. The allowlist is never pruned, so every bootstrap repeats the same doomed walk. Note the bound cannot be widened by page size: MAX_SESSION_PAGE_SIZE = 100 (session-list.ts:37, applied at :1352 before the sourceType branch) clamps every page, so 100 is already the ceiling and the SDK's Math.min(1000, …) is a non-binding pre-clamp. Latch convergence only when the scan actually converged (remaining.size === 0, or the catalog exhausted via !result.nextCursor), and carry the cursor across dropdown opens so a deep catalog is walked incrementally instead of never; better still, push the intersection server-side (an id-set or unattributed filter on the workspace listing) so one gathered scan returns exactly the allowlisted rows. Failing both, stop paging once a page's oldest updatedAt predates the oldest allowlisted Conversation.updatedAt, and surface the unresolved remainder so hitting the cap is observable.

Witness:

witness: not run — settling this needs a live daemon with a seeded default catalog
over 1000 rows (review drive), beyond this pass's budget. The deciding facts are
quoted from unchanged server code at the reviewed commit:
  session-list.ts:37    const MAX_SESSION_PAGE_SIZE = 100;
  session-list.ts:1352  const pageSize = Math.min(Math.max(requestedSize, 1), MAX_SESSION_PAGE_SIZE);
  session-list.ts:803   const byTime = b.activityTime - a.activityTime;
  EmbeddedApp.tsx:527   legacyScanDoneRef.current = true;   (success path only)
corroborating run on the unmodified tree: v8 coverage `stmt lines 84: hits=1` —
the suite observes exactly one page, so neither the cap nor the remaining.size exit runs.

Any new filter parameter must stay behind the same capability preflight the existing source filter uses (requireCapability('session_source_metadata') in DaemonClient.ts) and must preserve the legacy fallback (filter.sourceType === 'default' && session.sourceType === undefined) at session-list.ts:314.

The test that must go red is a new case in EmbeddedApp.test.tsx whose default mock returns non-matching pages with a live nextCursor chain and the allowlisted session beyond the cap: it must assert either that the row is eventually rendered or that the exhaustion is reported — today the cap is latched as converged and neither happens.

中文说明

这个窗口取的是目录的错误一端,而且"放弃"被记录成了"成功"。带元数据过滤的列表严格按时间倒序排列(session-list.ts:799-805b.activityTime - a.activityTime),而 default 桶里既有全部无归因行,也有该 workspace 中所有被标记为 default 的 CLI、浏览器与定时任务会话——但本次扫描要恢复的会话恰恰是其中最的那些。如果一个 workspace 累积了超过约 1000 条更新的 default 目录会话(长期在同一目录使用 CLI 的用户,正是最可能有切换前面板历史的人群),扫描永远走不到它们:循环在 LEGACY_SESSION_SCAN_MAX_PAGES 处退出,:527 的 legacyScanDoneRef.current = true 依然会执行(因为 helper 正常返回),于是这些会话在面板历史里持续缺席,没有错误、没有日志、也不会重试——与"本来就没有可恢复的会话"完全无法区分,而且发生在最有东西可恢复的用户身上。白名单从不被清理,所以每次 bootstrap 都会重复这趟注定无果的遍历。另外,这个上限无法靠加大页大小来放宽:MAX_SESSION_PAGE_SIZE = 100session-list.ts:37,在 :1352sourceType 分支之前生效)会钳制每一页,所以 100 已经是天花板,SDK 的 Math.min(1000, …) 只是不具约束力的前置钳制。建议只在扫描真正收敛时(remaining.size === 0,或通过 !result.nextCursor 判定目录已走完)才置位收敛标记,并把 cursor 跨下拉框打开保留下来,让很深的目录能被增量走完而不是永远走不到;更好的做法是把求交集下推到服务端(在 workspace 列表接口上加 id 集合或 unattributed 过滤),让一次聚合扫描就返回恰好命中白名单的行。若两者都不做,也可以在一页中最旧的 updatedAt 早于白名单中最旧的 Conversation.updatedAt 时停止翻页,并把未解析的剩余 id 暴露出来,使"触到上限"这件事可观测。

证据:未运行——要定这件事需要一个已注入超过 1000 条 default 目录行的真实 daemon(review drive),超出本轮预算。决定性事实引自被审 commit 上未改动的服务端代码:session-list.ts:37const MAX_SESSION_PAGE_SIZE = 100;:1352const pageSize = Math.min(Math.max(requestedSize, 1), MAX_SESSION_PAGE_SIZE);:803const byTime = b.activityTime - a.activityTime;,以及 EmbeddedApp.tsx:527 只在成功路径执行的 legacyScanDoneRef.current = true;。在未修改的代码树上的旁证运行:v8 覆盖率 stmt lines 84: hits=1——测试套件只观察到一页,所以上限与 remaining.size 出口都没有被执行过。

约束:任何新增的过滤参数都必须留在既有 source 过滤所用的同一道能力预检之后(DaemonClient.ts 中的 requireCapability('session_source_metadata')),并且必须保留 session-list.ts:314 的遗留回退 (filter.sourceType === 'default' && session.sourceType === undefined)

验收:必须变红的是 EmbeddedApp.test.tsx 中一个新用例——其 default mock 返回带 nextCursor 链条的不命中分页,而命中白名单的会话位于上限之外;它必须断言该行最终被渲染出来,或断言"已走完/未解析"被上报。今天上限被当作已收敛置位,两件事都没有发生。

— qwen3.8-max via Qwen Code /review (v0.23.2)

Comment on lines +94 to +95
if (!result.nextCursor) break;
cursor = result.nextCursor;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No test executes this cursor advance, so the paging path — and the bound the whole feature is reasoned about — is unpinned. Every nextCursor in the added fixture is undefined, the loop body runs once, and cursor = result.nextCursor; never executes: deleting it cannot change any observation, and CI stays green while real users with more than 100 default-catalog rows get page 1 re-read up to ten times — ten times the catalog cost, the history list blocked behind it, and a window that never advances to the older sessions the scan exists to recover. Dropping LEGACY_SESSION_SCAN_PAGE_SIZE from 100 to 10 would shrink the reachable window from 1000 catalog entries to 100, also green, because the only request-shape assertion in the test pins archiveState and not pageSize. Add a case whose default mock returns two pages (nextCursor: 'p2' then undefined) with the allowlisted id on page 2, asserting the id renders, that the second call carried cursor: 'p2', and that no third page is requested once the allowlist is satisfied.

Witness:

v8 statement coverage of EmbeddedApp.tsx under EmbeddedApp.test.tsx,
unmodified tree, suite green (92 tests):
  stmt lines 94-94: hits=1    if (!result.nextCursor) break;
  stmt lines 95-95: hits=0    cursor = result.nextCursor;
  stmt lines 96-96: hits=0    loop back-edge — never a second iteration
population swept: `nextCursor` occurs 4x in EmbeddedApp.test.tsx
(:770, :839, :865, :868) and every one is undefined; loadLegacyAllowlistedSessions
is module-local with a single call site, so no other suite can reach the loop.

LEGACY_SESSION_SCAN_PAGE_SIZE = 100 already equals the server's MAX_SESSION_PAGE_SIZE (session-list.ts:37), so a fixture that raises the page size would exercise a shape the daemon clamps away; and the existing ).toHaveLength(1); convergence assertion must stay green.

The new case must go red when cursor = result.nextCursor is deleted (page 2 is never fetched, so the id is never found) and red when the remaining.size > 0 loop condition is dropped (an extra page is requested after convergence).

中文说明

没有任何测试执行到这里的 cursor 推进,因此翻页路径——以及整个功能据以推理的那个上限——都没有被钉住。新增 fixture 里每个 nextCursor 都是 undefined,循环体只跑一次,cursor = result.nextCursor; 从未执行:删掉它不会改变任何可观察结果,CI 依然全绿;而现实中 default 目录超过 100 行的用户,会让第 1 页被重复读取最多十次——十倍的目录开销、被卡在后面的历史列表,以及一个永远不会推进到扫描本该恢复的更旧会话的窗口。把 LEGACY_SESSION_SCAN_PAGE_SIZE 从 100 改成 10,可触达窗口会从 1000 条目录项缩到 100 条,同样全绿,因为测试里唯一关于请求形状的断言钉的是 archiveState 而不是 pageSize。建议新增一个用例:default mock 返回两页(先 nextCursor: 'p2',再 undefined),命中白名单的 id 位于第 2 页,断言该 id 被渲染、第二次调用携带了 cursor: 'p2',并且白名单满足后不会请求第三页。

证据:在未修改的代码树上,用 EmbeddedApp.test.tsxEmbeddedApp.tsx 采集 v8 语句覆盖率(套件全绿,92 个测试):stmt lines 94-94: hits=1if (!result.nextCursor) break;)、stmt lines 95-95: hits=0cursor = result.nextCursor;)、stmt lines 96-96: hits=0(循环回边——从未进入第二次迭代)。总体排查:nextCursorEmbeddedApp.test.tsx 中出现 4 次(:770、:839、:865、:868),全部是 undefinedloadLegacyAllowlistedSessions 是模块内函数且只有一个调用点,所以没有其他套件能触达这个循环。

约束:LEGACY_SESSION_SCAN_PAGE_SIZE = 100 已经等于服务端的 MAX_SESSION_PAGE_SIZEsession-list.ts:37),所以抬高页大小的 fixture 会演练一个被 daemon 钳制掉的形状;同时既有的 ).toHaveLength(1); 收敛断言必须保持绿色。

验收:删掉 cursor = result.nextCursor 时新用例必须变红(第 2 页永远不会被取到,id 也就永远找不到);去掉 remaining.size > 0 循环条件时也必须变红(收敛之后还会多请求一页)。

— qwen3.8-max via Qwen Code /review (v0.23.2)

Comment on lines +446 to +447
legacyScanDoneRef.current = false;
}, [runtime?.legacyConversationIds]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Nothing pins this effect, and as shipped nothing can fire it. Reverting the hunk on its own left the whole packages/vscode-ide-companion suite green, so the behaviour the comment promises ("A re-bootstrap delivers a fresh allowlist — reopen the scan for it") is unprotected. Tracing that promise at the commit shows it is also unreachable: webShellBootstrap has exactly one poster (WebViewProvider.ts:2096) and webShellReady exactly one (EmbeddedApp.tsx:1007); every dependency of that effect (:1008-1015) is mount-stable — clearInsight is a useCallback with [], closeOpenPermissionDiffs and updateTranscript depend only on the module-level vscode singleton, handleShellError on [clearInsight, t] — and t cannot be rebuilt without a remount (:369-370, with /language in VSCODE_HIDDEN_SLASH_COMMANDS); readRuntimeConfig() (:284-300) carries no legacyConversationIds, so no scan can be in flight when the identity changes once; and a remount recreates both the ref and sessions. So the effect and its comment document a recovery path nothing exercises: a future host that re-bootstraps in place would depend on a guard no test protects, and a reader today cannot tell whether the latch is ever re-opened. Either add a test that dispatches a second webShellBootstrap with a fresh legacyConversationIds and asserts a second sourceType: 'default' page call plus rendering of the new legacy row, or drop the effect and its comment until an in-place re-bootstrap path exists.

Witness:

test-efficacy hunk probe (harnessValidated: true):
  hunk index 3, header @@ -372,6 +441,11 @@  ->  verdict: survived
  (reverting this hunk alone left the whole packages/vscode-ide-companion suite green)
reachability quoted at the commit:
  EmbeddedApp.tsx:1007  the only webShellReady poster, last statement of its effect
  EmbeddedApp.tsx:369-370  const language = useMemo(readLanguage, []);
                           const t = useMemo(() => createChromeStrings(language), [language]);
  WebViewProvider.ts:2096  the only webShellBootstrap poster

EmbeddedApp.test.tsx:949-954 asserts ).toHaveLength(1); for the sourceType === 'default' calls across three dropdown opens, so a new test must keep the scan closed on ordinary re-opens and re-open only on a changed allowlist, or that convergence assertion breaks.

The test to add must go red when this useEffect is deleted — today deleting it changes nothing, which is exactly what the hunk probe measured.

中文说明

没有任何测试钉住这个 effect,而且按当前代码它也不可能被触发。单独回退这个 hunk 之后,整个 packages/vscode-ide-companion 套件依然全绿,所以注释所承诺的行为("再一次 bootstrap 会带来新的白名单——为它重新打开扫描")没有任何保护。在 commit 上追踪这个承诺还会发现它不可达:webShellBootstrap 只有一个发送点(WebViewProvider.ts:2096),webShellReady 也只有一个(EmbeddedApp.tsx:1007);该 effect 的每个依赖(:1008-1015)都是按挂载稳定的——clearInsight 是依赖为 []useCallbackcloseOpenPermissionDiffsupdateTranscript 只依赖模块级 vscode 单例,handleShellError 依赖 [clearInsight, t]——而 t 在不重新挂载的情况下无法被重建(:369-370,且 /languageVSCODE_HIDDEN_SLASH_COMMANDS 中);readRuntimeConfig()(:284-300)不带 legacyConversationIds,所以在这个数组标识唯一一次变化时不可能有扫描在飞行中;而重新挂载会同时重建该 ref 与 sessions。因此这个 effect 和它的注释描述的是一条没有任何代码会走的恢复路径:将来若有宿主真的原地重新 bootstrap,就会依赖一个没有测试保护的守卫,而今天的读者也无法判断这个闩是否会被重新打开。建议要么补一个测试——派发第二次 webShellBootstrap 并带上新的 legacyConversationIds,断言发起了第二次 sourceType: 'default' 分页调用、且新的遗留行被渲染——要么在原地重 bootstrap 的路径真实存在之前,先删掉这个 effect 和它的注释。

证据:test-efficacy 的 hunk 探针(harnessValidated: true)——hunk 序号 3,头部 @@ -372,6 +441,11 @@,结论 survived(单独回退这个 hunk 后整个 packages/vscode-ide-companion 套件仍然全绿)。可达性引自 commit:EmbeddedApp.tsx:1007 是唯一的 webShellReady 发送点,且是其 effect 的最后一句;EmbeddedApp.tsx:369-370const language = useMemo(readLanguage, []);const t = useMemo(() => createChromeStrings(language), [language]);WebViewProvider.ts:2096 是唯一的 webShellBootstrap 发送点。

约束:EmbeddedApp.test.tsx:949-954 断言三次打开下拉框时 sourceType === 'default' 的调用数 ).toHaveLength(1);,所以新测试必须让扫描在普通重开时保持关闭、只在白名单变化时才重新打开,否则该收敛断言会被破坏。

验收:新增的测试必须在删除这个 useEffect 时变红——今天删除它不会有任何变化,而这正是 hunk 探针实测到的结果。

— qwen3.8-max via Qwen Code /review (v0.23.2)

);
// The legacy store doubles as the downgrade/recovery path; the bootstrap
// must stay read-only against it.
expect(conversationStoreMocks.getAllConversations).toHaveBeenCalled();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This assertion cannot fail, so the property the comment above it claims is certified by nothing. The payload assertion at :2431-2437 already requires legacyConversationIds: ['550e8400-…'], whose only producer is that read, so toHaveBeenCalled() adds no discriminating power — it verifies a read happened, not that no write happened, while the comment names this store as the downgrade/recovery path that "must stay read-only". A future change that prunes or re-saves legacy conversations during bootstrap would destroy that path and keep this test green. The constructible case today is a write through one of the two mocked mutators (createConversation/addMessage succeed, so both assertions stay green while the real store would have been written); the mocked class exposes only four members (:342-353) while the real store has ten, so deleteConversation, upsertConversation, renameConversationId, replaceMessages, truncateFromUserTurn and setCurrentConversationId are not even observable here. data: expect.objectContaining(...) likewise passes if a future change ships conversation bodies into the webview, so the "only ids cross the bridge" comment is uncertified too. Hoist the mutating methods into conversationStoreMocks the way getAllConversations is hoisted (:334-340) and assert not.toHaveBeenCalled(), and pin the ids-only property with an exact-keys assertion or expect(bootstrap?.data).not.toHaveProperty('legacyConversations') — or drop the comment sentence nothing certifies.

Witness:

witness: not run — the settling mutation (insert a call to a mocked mutator into the
bootstrap before the postMessage and re-run this file, expecting green) needs an edit;
review scratch-tree refused on this repository (unreadable includeIf content-filter
config) and the review worktree is read-only for this pass.
Both assertions and the mock's member list are quoted from the committed file:
  :2431-2437  payload assertion requiring legacyConversationIds: ['550e8400-…']
  :2440       expect(conversationStoreMocks.getAllConversations).toHaveBeenCalled();
  :342-353    mocked members: createConversation, addMessage,
              getCurrentConversationId, getAllConversations
  conversationStore.ts:67, :88, :137, :157, :200, :212  the six real mutators absent
Suite green unmodified (72 tests).

createConversation/addMessage are per-instance class-field vi.fn()s (:344-349), so a not.toHaveBeenCalled() assertion requires hoisting them into conversationStoreMocks first — asserting on a fresh per-instance vi.fn() would always pass and prove nothing. The file already uses that hoisted shape at :935, :999, :1288-1289 and :1469.

ships restorable legacy conversation ids in the bootstrap payload must go red if the bootstrap ever calls a mutating ConversationStore method or adds a snapshot-bearing field to data.

中文说明

这个断言不可能失败,因此它上面那句注释所声称的性质其实没有任何东西在证明。:2431-2437 的 payload 断言已经要求 legacyConversationIds: ['550e8400-…'],而它唯一的生产者就是这次读取,所以 toHaveBeenCalled() 没有任何区分能力——它验证的是"读发生过",而不是"没有发生写",可注释却把这个存储称为必须"保持只读"的降级/恢复路径。将来若有改动在 bootstrap 期间清理或重写遗留会话,会摧毁这条路径而这个测试依然是绿的。今天可以构造出来的情形是通过两个被 mock 的变更方法写入(createConversation/addMessage 会成功返回,于是两个断言都保持绿色,而真实存储其实已被写入);被 mock 的类只暴露四个成员(:342-353),而真实存储有十个,所以 deleteConversationupsertConversationrenameConversationIdreplaceMessagestruncateFromUserTurnsetCurrentConversationId 在这里甚至不可观测。同样地,data: expect.objectContaining(...) 在未来把会话正文送进 webview 时也会通过,所以"只有 id 过桥"这句注释也是没有证明的。建议像 getAllConversations 那样(:334-340)把这些变更方法 hoist 进 conversationStoreMocks 并断言 not.toHaveBeenCalled(),同时用精确键断言或 expect(bootstrap?.data).not.toHaveProperty('legacyConversations') 把"只有 id"这一性质钉住——或者删掉那句无人证明的注释。

证据:未运行——能定这件事的变异(在 postMessage 之前向 bootstrap 里插入一次对被 mock 的变更方法的调用,然后重跑本文件,预期仍绿)需要修改代码;review scratch-tree 在本仓库拒绝执行(无法读完的 includeIf 内容过滤配置),而本轮中审查用的 worktree 是只读的。两个断言与 mock 的成员列表都引自已提交的文件::2431-2437 是要求 legacyConversationIds: ['550e8400-…'] 的 payload 断言;:2440 是 expect(conversationStoreMocks.getAllConversations).toHaveBeenCalled();;:342-353 是被 mock 的成员 createConversationaddMessagegetCurrentConversationIdgetAllConversationsconversationStore.ts:67, :88, :137, :157, :200, :212 是缺失的六个真实变更方法。未修改状态下套件全绿(72 个测试)。

约束:createConversation/addMessage 是每实例的类字段 vi.fn()(:344-349),所以 not.toHaveBeenCalled() 断言必须先把它们 hoist 进 conversationStoreMocks——对一个新建的每实例 vi.fn() 做断言永远通过,什么也证明不了。本文件在 :935、:999、:1288-1289 和 :1469 已经使用了这种 hoist 写法。

验收:如果 bootstrap 将来调用了 ConversationStore 的任何变更方法,或往 data 里加入了携带快照的字段,ships restorable legacy conversation ids in the bootstrap payload 必须变红。

— qwen3.8-max via Qwen Code /review (v0.23.2)

Comment on lines +2018 to +2019
logger.warn(
'[WebViewProvider] Failed to read legacy conversations:',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Nothing exercises this catch, so removing it would ship a failed panel instead of a working one. Both added tests mock getAllConversations to resolve (:2405, :2444, plus the beforeEach default at :2352-2353) and there is no mockRejectedValue anywhere in the file, so the branch that degrades a broken legacy store to "no allowlist, panel works" never runs. Make the store throw on read — a globalState entry written by an older companion whose shape no longer deserializes, or a quota/read error — and with this catch present the bootstrap still posts webShellBootstrap with no legacyConversationIds; delete it and the same throw reaches the enclosing bootstrap catch at :2159-2170, which logs "Failed to start WebShell daemon" and posts webShellBootstrapError, so the user's chat panel renders a failure instead of a working panel with no legacy history. Add a case with conversationStoreMocks.getAllConversations.mockRejectedValue(new Error('legacy store corrupt')) asserting that webShellBootstrap is still posted, that legacyConversationIds is absent, and that no webShellBootstrapError was sent.

Witness:

v8 statement coverage of WebViewProvider.ts under WebViewProvider.test.ts,
unmodified tree (72 tests green):
  stmt lines 2009: hits=15    await this.conversationStore.getAllConversations()
  stmt lines 2015: hits=1     legacyConversationIds = legacyIds;
  stmt lines 2018: hits=0     logger.warn(
  stmt lines 2019-2021: hits=0
  stmt lines 2022: hits=0     } — end of the inner catch
The inner catch body never executes, so removing the try/catch cannot fail any test.

The outer bootstrap catch posts type: 'webShellBootstrapError' (:2164), so the new assertion must check that message type is absent — otherwise it passes for the degraded-to-broken behaviour it exists to exclude.

That new case must go red when this inner try/catch at :2007-2022 is removed, with the bootstrap replaced by webShellBootstrapError.

中文说明

没有任何测试演练到这个 catch,因此把它删掉将会交付一个失败的面板,而不是一个可用的面板。两个新增测试都把 getAllConversations mock 成 resolve(:2405、:2444,以及 :2352-2353 的 beforeEach 默认值),文件里也完全没有 mockRejectedValue,所以"遗留存储损坏时降级为没有白名单但面板可用"这条分支从未执行。让存储在读时抛错——比如旧版插件写入、如今已无法反序列化的 globalState 条目,或配额/读取错误——在保留这个 catch 的情况下,bootstrap 仍会发出 webShellBootstrap,只是不带 legacyConversationIds;而删掉它之后,同一个异常会传到 :2159-2170 外层 bootstrap 的 catch,记录 "Failed to start WebShell daemon" 并发出 webShellBootstrapError,于是用户的聊天面板会渲染成失败,而不是一个可用但没有遗留历史的面板。建议补一个用例:conversationStoreMocks.getAllConversations.mockRejectedValue(new Error('legacy store corrupt')),断言 webShellBootstrap 仍然被发出、legacyConversationIds 缺席、且没有发出 webShellBootstrapError

证据:在未修改的代码树上,用 WebViewProvider.test.tsWebViewProvider.ts 采集 v8 语句覆盖率(72 个测试全绿):stmt lines 2009: hits=15await this.conversationStore.getAllConversations())、stmt lines 2015: hits=1legacyConversationIds = legacyIds;)、stmt lines 2018: hits=0logger.warn()、stmt lines 2019-2021: hits=0stmt lines 2022: hits=0(内层 catch 的结尾)。内层 catch 的主体从未执行,所以移除这个 try/catch 不可能让任何测试失败。

约束:外层 bootstrap 的 catch 会发出 type: 'webShellBootstrapError'(:2164),所以新断言必须检查该消息类型不存在——否则它会对"由降级变成彻底损坏"的行为照样通过,而那正是它要排除的。

验收:移除 :2007-2022 这个内层 try/catch 时,新用例必须变红,此时 bootstrap 会被 webShellBootstrapError 取代。

— qwen3.8-max via Qwen Code /review (v0.23.2)

<DaemonSessionProvider
key="main-session"
sessionId={effectiveSessionId}
sessionSourceType={webShellProps.sessionSourceType}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This one line is the whole connection between the host's sessionSourceType and DaemonSessionProvider, and no test at any level covers it. EmbeddedApp.test.tsx mocks @qwen-code/web-shell to a component returning null and only asserts the prop reaches that mock (:176), while the new DaemonSessionProvider tests pass sessionSourceType straight into renderWithProvider, bypassing this line; WorkspaceSessionProvider.test.tsx already records every provider prop in mocks.providerProps (:32) but never mentions sessionSourceType. Delete this line and restore requests carry no sourceType, applyRestoreSourceIfMissing (bridge.ts:7276) returns without stamping, and the half of the feature that makes the recovery durable never happens — the reopened conversation stays unattributed, the vscode-scoped history query never returns it, and the panel must re-run its catalog scan on every bootstrap forever. The whole suite stays green. Add a case rendering the provider with webShellProps={{ sessionSourceType: 'vscode' }} and asserting expect(mocks.providerProps.at(-1)).toMatchObject({ sessionSourceType: 'vscode' }), plus the negative (webShellProps={{}} forwards undefined).

Witness:

mutation probe in an isolated copy of the reviewed commit:
INTACT  (line 334 present)  -> WorkspaceSessionProvider.test.tsx (28 tests) Tests 28 passed (28)
MUTANT  (sed -i '334d')     -> Tests 28 passed (28)          no flip
CONTROL (sed -i '333d', the adjacent sessionId prop)
                            -> Tests 9 failed | 19 passed (28)   comparator can report a difference
FULL SUITE under the mutant -> Test Files 1 failed | 296 passed (297)
                               (the only failures were build-artifact.test.ts ENOENT from
                                the copy's excluded dist/, which pass 17/17 once linked)
CANDIDATE FIX added         -> Tests 29 passed (29) intact; on the mutant:
                               expected { sessionId: 'session-a', …(8) } to match object
                               { sessionSourceType: 'vscode' }   Tests 1 failed | 28 passed (29)

Assert against mocks.providerProps.at(-1) in the existing style (:134) — mocks.providerProps is pushed by the mocked provider at :37-41 and reset in beforeEach at :97, so capturing props a second way would duplicate an existing mechanism.

That new case must go red when this line is deleted, which no current test does; the candidate fix above was measured to flip exactly that way.

中文说明

这一行是宿主的 sessionSourceTypeDaemonSessionProvider 之间的全部连接,而任何层级都没有测试覆盖它。EmbeddedApp.test.tsx@qwen-code/web-shell mock 成一个返回 null 的组件,只断言该 prop 到达了这个 mock(:176);新增的 DaemonSessionProvider 测试则把 sessionSourceType 直接传给 renderWithProvider,绕过了这一行;WorkspaceSessionProvider.test.tsx 已经把每个 provider prop 记录进 mocks.providerProps(:32),却从未提到 sessionSourceType。删掉这一行之后,恢复请求就不带 sourceTypeapplyRestoreSourceIfMissingbridge.ts:7276)直接返回而不打标记,让恢复得以持久的那一半功能就永远不会发生——被重新打开的会话仍是无归因的,按 vscode 过滤的历史查询永远不会返回它,面板也就必须在每次 bootstrap 时重跑目录扫描。而整个测试套件依然全绿。建议补一个用例:以 webShellProps={{ sessionSourceType: 'vscode' }} 渲染该 provider,并断言 expect(mocks.providerProps.at(-1)).toMatchObject({ sessionSourceType: 'vscode' }),再加一个反向用例(webShellProps={{}} 时转发 undefined)。

证据:在被审 commit 的隔离副本中做变异探针。原始(保留 334 行):WorkspaceSessionProvider.test.tsx (28 tests) Tests 28 passed (28)。变异(sed -i '334d'):Tests 28 passed (28),没有翻转。对照(sed -i '333d',删掉相邻的 sessionId prop):Tests 9 failed | 19 passed (28),说明对照手段能够报出差异。变异下跑全套:Test Files 1 failed | 296 passed (297)(唯一的失败是副本排除了 dist/ 导致的 build-artifact.test.ts ENOENT,链接 dist 后 17/17 通过)。加上候选修复后:原始状态 Tests 29 passed (29);在变异下报 expected { sessionId: 'session-a', …(8) } to match object { sessionSourceType: 'vscode' }Tests 1 failed | 28 passed (29)

约束:请按既有写法断言 mocks.providerProps.at(-1)(:134)——mocks.providerProps 由 :37-41 的被 mock provider 推入、并在 :97 的 beforeEach 中重置,所以用第二种方式捕获 props 会重复已有机制。

验收:删除这一行时新用例必须变红,而目前没有任何测试会;上面的候选修复已实测正好以这种方式翻转。

— qwen3.8-max via Qwen Code /review (v0.23.2)

Comment on lines +2042 to +2045
...(effectSessionContext?.kind !== 'standalone' &&
sessionSourceTypeRef.current !== undefined
? { sourceType: sessionSourceTypeRef.current }
: {}),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Restore-time attribution is unconditional for any workspace restore, so a host that sets this public prop permanently claims unattributed sessions it has no provenance for. The guard is on the session-context kind, not on whether the host has any evidence it created the session being restored — and the contrast with this PR's own scan is sharp: the scan gates its claim on a provenance record (ids the extension itself wrote to globalState, EmbeddedApp.tsx:85-91), while the restore path gates on nothing. sessionSourceType is a public prop of the published @qwen-code/web-shell, and the UI it ships lists sessions with sourceType: 'default' (useScopedSessions.ts:45,64, useOtherWorkspaceSessions.ts:48), a filter the daemon satisfies with unattributed rows too (session-list.ts:309-314); those rows open through App.tsx:12897 sessionActions.loadSession(...), the same action that now carries the host's source type. So any consumer that sets the prop and lets a user pick a pre-attribution CLI conversation from ResumeDialog, SessionOverviewPanel or SplitView stamps it — irreversibly, since apply-if-missing never overwrites (bridge.ts:7276-7282) and nothing on the restore path clears a source. The session leaves the default catalog for good and disappears from the browser Web Shell's history, resume dialog and session overviews, while the CLI user who created it sees it vanish. The VS Code panel avoids this only by configuration — sidebar={false} (EmbeddedApp.tsx:1535) and 'resume' in VSCODE_HIDDEN_SLASH_COMMANDS (:162), with hidden commands submitted as prompts rather than executed — so one entry removed from that list makes it reachable there too: the invariant holds by host configuration, not by construction. Consider making reclaim an explicit per-restore decision: keep sessionSourceType for creation and attach sourceType to a restore only when the caller marks it a legacy reclaim (for example loadSession(id, { claimUnattributedSource: true }) threaded into restoreRequest), with the panel setting it for ids it received in legacyConversationIds and for its persisted runtime.sessionId. Alternatively widen the panel's scan to accept a default stamp for ids its allowlist vouches for, since the allowlist — not the missing stamp — is the provenance evidence.

Witness:

probe in an isolated copy — a plain loadSession('legacy-unattributed') from a provider
with sessionSourceType: 'vscode' and no reclaim flag:
  Tests 1 passed | 319 skipped (320)
  MockDaemonSessionClient.load last called with
    { workspaceCwd: '/mock-workspace', timeoutMs: 70_000, sourceType: 'vscode' }
permanence read at the commit:
  routes/session.ts:3866-3875  folds in the caller's restoreSource whenever
                               hasPersistedSource is false
  bridge.ts:7276-7282          if (entry.sourceType !== undefined || req.sourceType === undefined)
                                 return undefined;  → apply-if-missing, no client can un-stamp
the browser arm of this claim was checked and REJECTED: main.tsx:210-247 builds
webShellProps without sessionSourceType, so browser restores forward undefined.

bridge.ts:7276 means no client-side fix can un-stamp an already-claimed session, and EmbeddedApp.tsx:504 queries sourceType: VSCODE_SESSION_SOURCE_TYPE, so the fix must still stamp the panel's own allowlisted restores — dropping attribution wholesale would leave recovered conversations dependent on the legacy scan at every bootstrap.

The test to add mirrors this diff's own standalone assertion (expect.not.objectContaining({ sourceType: expect.anything() })) for a plain loadSession('legacy-unattributed') from a provider with sessionSourceType: 'vscode' and no reclaim flag; it must go red if the gating is removed, while the flagged variant still asserts sourceType: 'vscode' is sent.

中文说明

恢复时的归因对任何工作区恢复都是无条件生效的,因此设置了这个公开 prop 的宿主会永久认领它并无出处依据的无归因会话。守卫判断的是 session context 的类型,而不是宿主是否有任何证据表明被恢复的会话是它创建的——这与本 PR 自己的扫描形成鲜明对比:扫描把认领建立在一份出处记录上(插件自己写进 globalState 的 id,EmbeddedApp.tsx:85-91),而恢复路径什么也不校验。sessionSourceType 是已发布包 @qwen-code/web-shell 的公开 prop,而它自带的界面会以 sourceType: 'default' 列出会话(useScopedSessions.ts:45,64useOtherWorkspaceSessions.ts:48),daemon 对这个过滤条件同样会用无归因行来满足(session-list.ts:309-314);这些行通过 App.tsx:12897 sessionActions.loadSession(...) 打开,正是现在会携带宿主 source type 的那个 action。所以任何设置了该 prop 的宿主,只要让用户从 ResumeDialog、SessionOverviewPanel 或 SplitView 里选中一个归因机制出现之前的 CLI 会话,就会给它打上标记——而且不可逆,因为"缺失才补"从不覆盖(bridge.ts:7276-7282),恢复路径上也没有任何东西会清除 source。该会话从此永久离开 default 目录,并从浏览器 Web Shell 的历史、恢复对话框与会话总览中消失,而创建它的 CLI 用户会看到自己的会话凭空不见。VS Code 面板之所以避开这一点,靠的只是配置——sidebar={false}EmbeddedApp.tsx:1535)以及 'resume' 位于 VSCODE_HIDDEN_SLASH_COMMANDS(:162),且隐藏命令是作为 prompt 提交而非执行——所以只要从那个列表里去掉一项,面板里也会变得可达:这条不变量是靠宿主配置成立的,不是靠结构成立的。建议把"认领"改成每次恢复的显式决定:sessionSourceType 仍用于创建,而只有当调用方标记这是一次遗留认领时(例如 loadSession(id, { claimUnattributedSource: true }) 并透传进 restoreRequest)才给恢复请求附上 sourceType,面板则只对它在 legacyConversationIds 中收到的 id 以及自己持久化的 runtime.sessionId 设置该标记。另一种做法是放宽面板的扫描,对白名单担保的 id 接受 default 标记,因为真正的出处证据是白名单,而不是"缺少标记"这件事。

证据:在隔离副本中探针——从一个 sessionSourceType: 'vscode' 且没有认领标记的 provider 发起普通的 loadSession('legacy-unattributed')Tests 1 passed | 319 skipped (320)MockDaemonSessionClient.load 最后一次被调用时的参数是 { workspaceCwd: '/mock-workspace', timeoutMs: 70_000, sourceType: 'vscode' }。不可逆性引自 commit:routes/session.ts:3866-3875hasPersistedSource 为 false 时把调用方的 restoreSource 合入;bridge.ts:7276-7282if (entry.sourceType !== undefined || req.sourceType === undefined) return undefined;,即缺失才补,任何客户端都无法撤销标记。本条主张中关于浏览器的那一半已被核查并否定main.tsx:210-247 构造的 webShellProps 不含 sessionSourceType,所以浏览器恢复转发的是 undefined

约束:bridge.ts:7276 意味着任何客户端侧修复都无法撤销已被认领会话的标记,而 EmbeddedApp.tsx:504 查询的是 sourceType: VSCODE_SESSION_SOURCE_TYPE,所以修复仍必须为面板自己白名单内的恢复打标记——完全去掉归因会让恢复出来的会话在每次 bootstrap 时都依赖遗留扫描。

验收:要补的测试参照本 diff 自己的 standalone 断言(expect.not.objectContaining({ sourceType: expect.anything() })),针对从 sessionSourceType: 'vscode' 且无认领标记的 provider 发起的普通 loadSession('legacy-unattributed');去掉该门控时它必须变红,而带标记的变体仍要断言 sourceType: 'vscode' 被发出。

— qwen3.8-max via Qwen Code /review (v0.23.2)

/** Stable client identity to reuse for session-scoped daemon requests. */
clientId?: string;
/**
* Creator attribution forwarded on workspace session load/resume. The

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This prop is a free-form string that now rides on every workspace load/resume into a daemon field which is pattern-validated, reserved-source-checked and read as restore routing — and neither this JSDoc nor the public prop doc states any of it. A maintainer adding the next host wires sessionSourceType="JetBrains IDE" (or "VS Code"): parseRequestedSessionSource runs on the restore route before any restore work (routes/session.ts:3681-3682) and answers 400 invalid_session_source, and the SDK degrades gracefully only for a missing capability, not a rejected value (DaemonClient.ts:3793-3803 rethrows), so every session load and resume in that host fails and the user sees a session-load error rather than missing attribution. A host that picks a valid-but-foreign value gets worse: 'channel' passes validation, isChannelRestore (routes/session.ts:3876-3877) then flips the restore onto the channel path — worktree-context suppression at :3884 plus the ownership-lock redirect at :3921/:3932 — and the value is persisted onto pre-existing sessions the host never created. Nothing is broken today (the only in-tree caller passes 'vscode'); the hazard is that the constraint lives three packages away from the prop it governs, and the doc integrators actually read — WebShellProps.sessionSourceType at App.tsx:1141-1148 — still says "Creator attribution recorded on sessions this shell creates", which this line makes false for restore. State the contract there and here (forwarded on workspace load/resume; recorded on any restored session with no persisted source; must match the daemon's pattern and must not be reserved, because an invalid value fails the whole restore; and a host thereby claims any unattributed session it restores), and add it to packages/web-shell/README.md, where the prop is currently absent — or narrow the type / validate before it reaches restoreRequest.

Witness:

sweep of 21 plausible host labels through the real validators, in the route's own order
(routes/session.ts:747-780, SESSION_SOURCE_TYPE_PATTERN = /^[a-z][a-z0-9_-]{0,63}$/):
  vscode                          -> accepted   (the only in-tree caller)
  JetBrains IDE / VS Code / Zed / Cursor IDE / 1editor / _private
                                  -> 400 invalid_session_source
  standalone                      -> 400 reserved_session_source
  channel / default / web-shell / zed / jetbrains / trae
                                  -> accepted
  rejected count: 7 / 21
routing half verified at the commit:
  routes/session.ts:3876-3877  const isChannelRestore = restoreRequestMetadata.sourceType === 'channel';
  gates :3884 (worktree-context suppression), :3921 / :3932 (ownership-lock redirect,
  WorktreeSessionSupersededError); restoreRequestMetadata is spread into the bridge
  restore calls at :4000 / :4015, where bridge.ts:7276-7282 persists it.

The error text lives at packages/acp-bridge/src/session-source.ts:49 and reserved values are rejected at routes/session.ts:750-780; any exclusion must not weaken the standalone omission this diff relies on (BridgeStandaloneRestoreSessionRequest omits the field, bridgeTypes.ts:278-281, pinned by the omits restore-time attribution for standalone sessions test).

If a guard or normalization is added rather than documentation, a DaemonSessionProvider.test.tsx case asserting an invalid sessionSourceType is dropped from the load request pins it — removing the guard must make the request carry the value again.

中文说明

这个 prop 是一个自由格式的 string,如今会随每一次工作区 load/resume 进入一个 daemon 字段,而该字段既要过格式校验、又要过保留 source 校验,还会被当作恢复路由来读——可无论是这段 JSDoc 还是公开的 prop 文档,都没有说明其中任何一点。维护者接入下一个宿主时写下 sessionSourceType="JetBrains IDE"(或 "VS Code"):parseRequestedSessionSource 会在恢复路由做任何恢复工作之前执行(routes/session.ts:3681-3682)并返回 400 invalid_session_source,而 SDK 只对"能力缺失"优雅降级、不会对"值被拒绝"降级(DaemonClient.ts:3793-3803 会重新抛出),于是该宿主里每一次会话加载与恢复都失败,用户看到的是会话加载错误,而不是"归因缺失"。选了一个合法但属于别处的值会更糟:'channel' 能通过校验,随后 isChannelRestoreroutes/session.ts:3876-3877)会把恢复切到 channel 路径——:3884 抑制 worktree 上下文,:3921/:3932 触发归属锁重定向——并且这个值会被持久化到宿主从未创建过的既有会话上。今天没有任何东西是坏的(仓库内唯一的调用方传的是 'vscode');风险在于这条约束与它所约束的 prop 相隔三个包,而集成方真正会读的文档——App.tsx:1141-1148WebShellProps.sessionSourceType——仍然写着"记录在本 shell 创建的会话上的创建者归因",这一行让它在恢复语义上变成了假的。建议在这里和那里都写清契约(会在工作区 load/resume 时转发;会被记录到任何没有持久化 source 的被恢复会话上;必须符合 daemon 的格式且不能是保留值,否则整个恢复会失败;并且宿主因此会认领它恢复的任何无归因会话),并补进目前没有提到该 prop 的 packages/web-shell/README.md——或者收窄类型、在到达 restoreRequest 之前做校验。

证据:把 21 个可能的宿主标签按恢复路由自身的顺序(routes/session.ts:747-780SESSION_SOURCE_TYPE_PATTERN = /^[a-z][a-z0-9_-]{0,63}$/)过一遍真实校验函数:vscode 通过(仓库内唯一调用方);JetBrains IDEVS CodeZedCursor IDE1editor_private 返回 400 invalid_session_sourcestandalone 返回 400 reserved_session_sourcechanneldefaultweb-shellzedjetbrainstrae 通过;拒绝数 7/21。路由那一半在 commit 上已核实:routes/session.ts:3876-3877const isChannelRestore = restoreRequestMetadata.sourceType === 'channel'; 控制 :3884(抑制 worktree 上下文)与 :3921/:3932(归属锁重定向、WorktreeSessionSupersededError);restoreRequestMetadata 会在 :4000/:4015 被展开进 bridge 的恢复调用,并由 bridge.ts:7276-7282 持久化。

约束:错误文案在 packages/acp-bridge/src/session-source.ts:49,保留值在 routes/session.ts:750-780 被拒绝;任何排除逻辑都不得削弱本 diff 依赖的 standalone 省略(BridgeStandaloneRestoreSessionRequest 省略该字段,bridgeTypes.ts:278-281,并由 omits restore-time attribution for standalone sessions 测试钉住)。

验收:如果加的是守卫或归一化而不是文档,那么 DaemonSessionProvider.test.tsx 中一个断言非法 sessionSourceType 不会出现在 load 请求里的用例即可钉住它——移除该守卫后,请求必须重新携带该值。

— qwen3.8-max via Qwen Code /review (v0.23.2)

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Follow-up with a real-daemon verification, no mocks: ran qwen serve from this head against an isolated QWEN_HOME with four hand-written transcripts in the workspace chats dir — two unattributed (pre-cutover style), one session_source: vscode, one session_source: default.

  • GET /sessions?sourceType=vscode → only the vscode-stamped session.
  • GET /sessions?sourceType=default → the default-stamped CLI session plus both unattributed ones, and their JSON has the sourceType key absent (not null), so the panel's session.sourceType === undefined check passes for exactly the right records.
  • POST /session/:id/load on an unattributed session with {sourceType: 'vscode'}sourcePersisted: true, a session_source record appended to the transcript tail, and the session moves from the default catalog into the vscode one on subsequent queries.
  • Same call on the already-vscode-stamped session with {sourceType: 'cli'} → stays vscode, transcript untouched — existing attribution is never overwritten.

The server-side contract this PR relies on behaves exactly as assumed against real files on disk.

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking findings.
Approval blockers: none.

Triage: Standard — contained feature fix touching extension host → webview → daemon chain, no persisted format changes, no wire protocol changes.

What I checked:

  1. Allowlist contract (WebViewProvider → EmbeddedApp): WebViewProvider.getAllConversations() reads globalState read-only; getRestorableDaemonSessionId filters out conv_*/temp* drafts; only real daemon IDs cross the bridge. No writes to globalState — downgrade keeps working.

  2. Legacy scan correctness (loadLegacyAllowlistedSessions): requests sourceType: 'default' which the server maps to unattributed sessions (confirmed by author's real-daemon verification: the JSON sourceType key is genuinely absent for these, so session.sourceType === undefined is the correct filter). Sessions explicitly stamped default (CLI/Web Shell) have sourceType: 'default' (defined) and are correctly excluded by the === undefined check. Workspace scoping via workspaceByCwd prevents cross-workspace CLI session leakage.

  3. Source attribution restoration: DaemonSessionProvider forwards sessionSourceType via ref (mirrors prop without triggering effect), only for non-standalone contexts. The effectSessionContext?.kind !== 'standalone' guard correctly prevents standalone requests from carrying source attribution — confirmed by the negative test. The daemon's applyRestoreSourceIfMissing never overwrites existing attribution — confirmed by author's real-daemon test (vscode-stamped session + cli sourceType request → stays vscode).

  4. Scan convergence: legacyScanDoneRef prevents re-scanning after first successful scan; reset on runtime?.legacyConversationIds change (fresh bootstrap). The worst case is 10 pages × 100 = 1000 catalog reads, bounded and one-shot.

  5. Error handling: Legacy scan failure → fail-open (empty legacy list, ordinary vscode history unaffected); conversationStore read failure → warn and ship no legacy IDs. Neither path blocks the panel from loading.

  6. Test validity: EmbeddedApp.test.tsx asserts the panel shows exactly the vscode + legacy pair while excluding CLI and browser sessions — mutation of the new guard (removing !legacyScanDoneRef.current) is claimed to turn the new assertion red, confirming the assertion is load-bearing. DaemonSessionProvider.test.tsx has both a positive test (forwards sourceType) and negative test (standalone omits sourceType).

  7. Cross-check vs existing reviews: qwen-code-ci-bot approved (LGTM). Author's real-daemon verification at the current head confirms the server-side contract (sourceType absent in JSON for unattributed sessions; load with sourceType moves session between catalogs; existing attribution never overwritten). No outstanding blockers from other reviewers.

Unreviewed dimensions: Windows runtime behavior (no host available), no real E2E harness in the companion package. Not approval blockers — the author's real-daemon verification against an isolated QWEN_HOME covers the server-side contract with real files.

Reviewed with AI assistance.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.3.

yiliang114 added a commit that referenced this pull request Sep 11, 2026
Dropping the sourceType filter (#11574) also re-admitted machine-owned
catalog rows — channel conversations, scheduled-task keepalives, side-task
branches, and sub-agent children — which rendered as ordinary chats and
carried the panel's rename/permanent-delete actions with no source-ownership
guard on the daemon side. Filter those rows before they reach the dropdown
and fetch until a full page of presentable sessions is collected, since
client-side filtering shortens each page while the raw cursor still advances.

Also pin the removal of the #11495 legacy allowlist on both sides of the
bootstrap payload contract, and correct the session-source docs/tests that
still asserted the reversed source-scoped decision.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtwii8zex8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extension update drops all conversation history (v0.21.x → v0.23.x)

4 participants