feat(web-shell): restore environment panel state - #10627
Conversation
…tachments # Conflicts: # packages/cli/src/acp-integration/acpAgent.test.ts # packages/cli/src/acp-integration/session/tasksSnapshot.test.ts # packages/cli/src/serve/acp-http/dispatch.ts # packages/cli/src/serve/acp-http/transport.test.ts # packages/cli/src/serve/server.test.ts # packages/cli/src/serve/server/telemetry-catalog.test.ts # packages/sdk-typescript/scripts/build.js # packages/sdk-typescript/src/daemon/DaemonClient.ts # packages/sdk-typescript/src/daemon/DaemonSessionClient.ts # packages/sdk-typescript/src/daemon/index.ts # packages/sdk-typescript/src/daemon/types.ts # packages/sdk-typescript/src/index.ts
|
Thanks for the continued work on this! Template looks good ✓ Problem: an observed UX gap, described concretely — refreshing or switching sessions discards the right-side panel tabs, completed subagents can fall outside the loaded transcript page, and environment sections can flash skeleton→empty. Not a theoretical concern, and the reviewer test plan spells out each scenario. One note unchanged from last pass: there is no linked issue capturing user reports — the motivation is the author's own observation, which is fine for a feature PR, just worth saying out loud. Direction: aligned, and the signal is stronger than the last pass. The session-workflow cockpit (#8583) landed on Size (re-counted on the current head): 4,503 production lines / 4,557 test lines / 40 docs lines / 0 generated, across 71 files spanning five packages ( Approach: the standing scope note still holds — the diff bundles tab restoration, expanded environment sections, a dedicated subagent snapshot/trace pipeline across daemon routes, SDK, bridge, and core, and the Workflow graph, plus a side-task duplicate-session fix. Since the last triage pass the author pushed five fix commits — the last two ( Risk: unchanged — Moving on to code review. 🔍 中文说明感谢持续的改进！ 模板完整 ✓ 问题：已观察到的 UX 缺陷，描述具体——刷新或切换会话会丢失右侧面板页签，已完成的子智能体可能落在 transcript 分页之外，环境区块可能从骨架屏闪到空状态。不是理论性问题，reviewer 测试计划也逐一列出了场景。与上次相同的一点说明：没有关联的 issue 记录用户报告——动机来自作者自己的观察，对 feature PR 来说没问题，只是明确说出来更好。 方向：对齐，且信号比上次更强。会话工作流驾驶舱（#8583）在此期间已合入 规模（按当前 head 重新统计）：4,503 行生产代码 / 4,557 行测试 / 40 行文档 / 0 行生成代码，共 71 个文件，横跨五个包（ 方案：既有的范围说明仍然成立——diff 打包了页签恢复、展开式环境区块、横跨 daemon 路由/SDK/bridge/core 的独立子智能体快照/追踪管道、Workflow 图，外加一个 side-task 重复会话修复。自上次 triage 以来，作者推送了五个修复提交——最后两个（ 风险：不变—— 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "session_attachment_list" |
features[] |
— | "session_agents" |
features[] |
— | "session_agent_trace" |
— Qwen Code · serve A/B
Code reviewRe-review at head My independent baseline for these two commits, written before reading them: persistence must happen only on stable user actions (never on transient session-switch state), restore must defer rather than silently drop tabs whose capability is not yet available, and close-time tab promotion must reuse the same hydration path as user selection. The delta matches that baseline point for point. What I verified against the head code (not just the diffs):
Non-blocking, still open:
sequenceDiagram
participant P1 as Reload or session switch
participant P2 as App restore effect
participant P3 as localStorage
participant P4 as Daemon
participant P5 as Right panel
P1->>P2: mount with session key
P2->>P3: read persisted tab keys, versioned and bounded
P3-->>P2: key-only tab descriptors
P2->>P5: open shell without animation, skeleton
P2->>P2: defer tabs whose capability is missing, keep them in persisted saves
P2->>P4: wait for transcript and artifacts to load
P4-->>P2: session ready
P2->>P2: hydrate each tab by kind, pending tabs lazy
P2->>P4: active tab fetches content, others on selection or capability arrival
P4-->>P5: content renders
Files changed (30 of 71 shown)
…and 41 more files (tests for the above, CSS, adapters). TestingThis is an unattended CI run — I do not build or execute PR code; evidence below is the PR's own CI on commit All product lanes are green at this head, including
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would still settle one remaining gap: 中文说明代码审查在 head 我在阅读这两个提交前写下的独立基线：持久化只应发生在稳定的用户动作上（绝不能发生在会话切换的瞬态状态上），恢复时对能力尚不可用的页签应延迟而非静默丢弃，关闭页签时的提升应复用与用户选中相同的水合路径。增量与这一基线逐点吻合。 针对 head 代码（而不仅是 diff）核实的内容：
非阻塞、仍未解决：
测试这是无人值守 CI 运行——我们不构建或执行 PR 代码；以下证据是提交 本 head 上所有产品检查均为绿色，包括 （CI 表格见英文正文机器可读区域。） 沙箱验证仍可补上最后一个缺口： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean re-review with fully green CI at this head, but the core-scale escalation caps the score and a maintainer sign-off is required. My honest read: this PR has genuinely improved since the last pass. The fix commits since then meet the review findings head-on — Why I'm still not approving:
⏸️ Deferring to @yiliang114 — the core-scale policy cap and the scope question above need a human call. The code itself is in the best shape it has been: CI green, round-3 Criticals addressed with tests, and the author has been responsive across five fix commits. 中文说明置信度：3/5 —— 复审干净，本 head 上 CI 全绿，但核心模块规模的升级流程设定了上限，需要维护者确认。 我的真实看法：这个 PR 自上次以来确有实质改进。此后的修复提交正面回应了 review 发现—— 仍不批准的原因：
⏸️ 转交 @yiliang114 —— 核心模块规模的政策上限与上述范围问题需要人工决定。代码本身处于迄今最好的状态：CI 绿色，第 3 轮 Critical 已带测试修复，作者在五个修复提交中一直积极响应。 — Qwen Code · qwen3.8-max Reviewed at |
…tachments # Conflicts: # packages/sdk-typescript/scripts/build.js
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 19: none — no check was cut short..
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 19：none — no check was cut short.。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const measured = widths.get(id); | ||
| if (measured !== undefined) return measured; | ||
| if (measuring.has(id)) return NODE_WIDTH; |
There was a problem hiding this comment.
[Suggestion] R1-32: The cycle guards in layoutTasks — measuring in measure (here) and visited in place — are the only thing keeping cyclic agent lineages from infinitely recursing, and no test pins them: AgentWorkflow.test.tsx exercises only an acyclic two-node lineage and the skeleton. Cyclic lineage is a modeled upstream state that reaches this component intact — readAgentTrace computes lineageState: 'cycle' and keeps the cyclic parentAgentId links on the nodes, and mergeAgentTrace propagates parentAgentId as-is. If a future refactor drops either guard, rendering the Workflow tab for any session whose trace contains a parent cycle throws RangeError: Maximum call stack size exceeded inside render and crashes the panel content, while both existing tests stay green. Add test cases rendering (a) two agents whose parentAgentIds reference each other and (b) an agent whose parentAgentId names an id absent from the list, asserting the render completes and both node buttons appear.
Witness:
Cyclic render on unmodified code: passes (guards hold today)
With `if (measuring.has(id)) return NODE_WIDTH;` deleted:
RangeError: Maximum call stack size exceeded at `measure` (probe fails; the two existing tests still pass)
Deleting either guard line must red the new tests.
中文说明
layoutTasks 中的环守卫——measure 里的 measuring（此处）与 place 里的 visited——是阻止循环智能体谱系无限递归的唯一屏障，且没有测试钉住它们：AgentWorkflow.test.tsx 只覆盖无环的两节点谱系与骨架屏。循环谱系是被建模的上游状态，会原样到达本组件——readAgentTrace 计算 lineageState: 'cycle' 并保留节点上成环的 parentAgentId 链接，mergeAgentTrace 原样传递 parentAgentId。若日后重构移除任一守卫，任何 trace 含父级环的会话渲染 Workflow 页签时都会在渲染中抛出 RangeError: Maximum call stack size exceeded，面板内容崩溃，而现有两个测试仍为绿。请补充测试：渲染 (a) 两个 parentAgentId 互相引用的智能体，以及 (b) 一个 parentAgentId 指向列表中不存在 id 的智能体，断言渲染完成且两个节点按钮都出现。
删除任一守卫行时，新测试必须为红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Not addressed in this PR. Both cycle guards are present and working; this asks for defensive layout tests. Defer to a focused AgentWorkflow layout test follow-up.
| const header = Array.from( | ||
| view.querySelectorAll<HTMLButtonElement>('button[aria-expanded]'), | ||
| ).find((button) => button.textContent?.includes('Attachments')); |
There was a problem hiding this comment.
[Suggestion] R1-33: Section collapse behaviour lost all test coverage in this diff: the deleted assertions were the only ones pinning collapsed state for Subagents/Background tasks, and the two brand-new sections (Attachments, Artifacts) introduce toggle state that no test exercises past mount — five near-identical useState(true) + setXExpanded pairs now gate content, and the only toggle assertions left target the Environment section. The new attachment/artifact tests locate the headers (as here) but never click them. A cross-wire between any two of the five expanded states — e.g. the Artifacts header toggling attachmentsExpanded, or a state initialized wrong — ships green, leaving a section the user cannot collapse (or collapsing the wrong section) with no red test. Add collapse coverage: click the Attachments header and assert 'photo.png' disappears; click the Artifacts header and assert 'report.md' disappears; and re-collapse one of Subagents/Background tasks to restore what the deleted assertions pinned.
Witness:
witness: not run — settled by enumeration: the diff flips agentsExpanded/tasksExpanded to useState(true) and adds attachmentsExpanded/artifactsExpanded (five gated pairs total); exhaustive grep shows the only toggle assertions target the Environment section (lines 102, 177-178); the new headers are located but never clicked
Cross-wiring setAttachmentsExpanded with setArtifactsExpanded in EnvironmentPanel.tsx must red the added tests.
中文说明
区块折叠行为在本 diff 中失去了全部测试覆盖：被删除的断言是仅有的钉住 Subagents/Background tasks 折叠状态的断言，而两个全新区块（Attachments、Artifacts）引入的切换状态在挂载之后没有任何测试触及——现在有五个几乎相同的 useState(true) + setXExpanded 对在门控内容，而仅存的切换断言只针对 Environment 区块。新的附件/产物测试定位了区块头（如此处）但从不点击它们。五个展开状态中任意两个交叉接线——例如 Artifacts 区块头切换了 attachmentsExpanded，或某个状态初始化错误——都能绿着上线，留下一个用户无法折叠的区块（或折叠错误的区块）而没有任何红色测试。请补充折叠覆盖：点击 Attachments 区块头断言 'photo.png' 消失；点击 Artifacts 区块头断言 'report.md' 消失；并重新折叠 Subagents/Background tasks 之一，恢复被删断言钉住的行为。
在 EnvironmentPanel.tsx 中把 setAttachmentsExpanded 与 setArtifactsExpanded 交叉接线时，新增测试必须为红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| export type EnvironmentAgentTask = DaemonSessionAgentTaskStatus & { | ||
| color?: string; | ||
| lineageState?: 'complete' | 'orphaned' | 'cycle'; | ||
| }; |
There was a problem hiding this comment.
[Suggestion] R1-34: EnvironmentAgentTask.lineageState is plumbed end-to-end and stamped onto every merged panel agent task (App.tsx:2542) but has zero read sites anywhere in web-shell — a repo-wide sweep finds exactly 6 occurrences: the write site, this type declaration, and four App.test.tsx fixture inputs feeding trace nodes. Agent rows branch only on isForkAgent(task) and task.status, and AgentWorkflow never reads it either, so an orphaned or cyclic lineage renders byte-identically to a complete one, and a regression corrupting the upstream classification ships green (AGENTS.md asks that every added field's read sites be grepped). Either surface the state in the agent row (e.g. an orphan/cycle marker alongside the existing fork tag when task.lineageState !== 'complete'), or delete the field and the setter at App.tsx:2542 until a consumer exists.
Witness:
sweep — grep 'lineageState' across packages/web-shell: 6 hits — 1 write site (App.tsx:2542), 1 type declaration, 4 test-fixture inputs, 0 render-path reads
For the render option, an EnvironmentPanel test asserting the marker renders for an orphaned task must red if the render guard is removed.
中文说明
EnvironmentAgentTask.lineageState 被端到端接入，并打印到每个合并后的面板智能体任务上（App.tsx:2542），但在整个 web-shell 中没有任何读取点——全库扫描恰好 6 处：写入点、此类型声明、以及四个向 trace 节点供数的 App.test.tsx 测试夹具。智能体行只按 isForkAgent(task) 与 task.status 分支，AgentWorkflow 也不读取它，因此孤儿或成环的谱系与完整谱系渲染得逐字节相同，上游分类被破坏的回归也能绿着上线（AGENTS.md 要求对每个新增字段 grep 读取点）。请在智能体行中呈现该状态（例如 task.lineageState !== 'complete' 时在现有 fork 标签旁显示孤儿/环标记），或在出现消费者之前删除该字段与 App.tsx:2542 的写入。
若选择渲染方案，EnvironmentPanel 测试断言 orphaned 任务渲染出该标记；移除渲染守卫时应为红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Not addressed in this PR. lineageState is metadata for future diagnostics but has no current user-facing contract; adding a marker or deleting the field changes design. Defer that decision to a focused Workflow lineage follow-up.
| ws.onerror = () => ws.close(); | ||
| }; | ||
| releaseCallbacks.set(terminalId, release); |
There was a problem hiding this comment.
[Suggestion] R1-15: The disabled-state release is a one-shot WebSocket with no retry, while the enabled path retries the release handshake up to 3 times on any close other than 4004. A restored terminal tab that was never enabled still has a live daemon PTY session (that is why releaseWebTerminal exists for it); when the user closes that tab, this single release WebSocket opens, and if it fails transiently (ws.onerror fires), the callback just closes the socket and never retries, so registry.release never runs and the daemon retains the un-released PTY for restore until its idle reclaim. The enabled path covers this exact transient case with releaseAttempts < 3 retries at RECONNECT_INITIAL_MS — give this branch the same bounded retry (or factor the enabled path's connect(true) retry logic into a shared helper), keeping the release=1 query parameter and treating close code 4004 as success (the daemon completes release at upgrade only via registry.release + ws.close(4004), routes/terminal.ts:149-151).
Witness:
BASE (unmodified PR): PROBE DISABLED RELEASE ATTEMPTS: 1
PROBE ENABLED RELEASE ATTEMPTS: 3
FIXED (bounded retry ported to disabled branch):
PROBE DISABLED RELEASE ATTEMPTS: 3
A test mirroring the existing 'retries a failed release handshake a bounded number of times' with enabled={false} — close the release socket with a non-4004 code, advance timers, expect a second release=1 WebSocket — reds while this branch stays one-shot.
中文说明
禁用态的释放是一次性 WebSocket、没有重试，而启用路径会在任何非 4004 关闭时对释放握手重试至多 3 次。一个恢复后从未启用的终端页签仍持有活跃的 daemon PTY 会话（releaseWebTerminal 正是为此存在）；用户关闭该页签时会打开这唯一一次释放 WebSocket，若它瞬时失败（触发 ws.onerror），回调只是关闭套接字、从不重试，于是 registry.release 永不执行，daemon 会保留这个未释放的 PTY 以待恢复，直到空闲回收。启用路径用 releaseAttempts < 3、间隔 RECONNECT_INITIAL_MS 的重试覆盖了完全相同的瞬时失败场景——请给这个分支同样的有界重试（或把启用路径 connect(true) 的重试逻辑抽成共享辅助），保留 release=1 查询参数，并把关闭码 4004 视为成功（daemon 仅在升级时通过 registry.release + ws.close(4004) 完成释放，routes/terminal.ts:149-151）。
请仿照现有的 'retries a failed release handshake a bounded number of times' 补一个 enabled={false} 的测试——以非 4004 码关闭释放套接字，推进计时器，期望第二次 release=1 WebSocket——只要该分支仍是一次性的，测试就应为红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Not addressed in this PR. This concerns the main-derived disabled terminal release path, not the environment/right-panel restoration changes introduced here, and requires dedicated retry semantics. Defer to a focused terminal lifecycle follow-up.
…tachments # Conflicts: # packages/acp-bridge/src/sessionAttachments.test.ts
|
Hourly monitor run completed on Fixed in this run:
Also merged the latest Verification:
Per the repository rule for PRs already beyond roughly five review rounds, the remaining non-Critical suggestions are deferred rather than widening this PR: attachment copy mtime/order; extra ACP trace/rate-tier/warning/limit fixtures; additional bridge payload and attachment-hop assertions; extra restore pagination/cross-session/width/eviction/first-retry tests; additional agent polling/activity/error/cycle/display/collapse tests; removal of the unused lineage field; and terminal-release retry hardening. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Independent cross-check of this PR (head f97a898185ac), with findings frozen before reading the prior review.
CI status
- ✅ Desktop Shell (ubuntu, windows): green
- ⏳ Test (ubuntu-latest): in-progress at time of review
⚠️ Test (windows-latest, macos-latest): SKIPPED — platform-dependent path code has no coverage on these platforms for this head⚠️ Integration Tests (CLI, No Sandbox): SKIPPED
Cross-check against prior review critical findings
R1-3 refuted — cache key includes the raw session ID.
The prior review states agentSidecarCache is "keyed solely by the sanitized session directory." The actual key is `${dir}\0${sessionId}` (see inline at tasksSnapshot.ts:321), which includes the original unsanitized sessionId after a null-byte separator. Two sessions whose IDs differ only in their sanitized forms (e.g., feat.x vs feat_x) produce distinct cache keys and never share cache entries. There is a real directory-collision risk on disk (both sessions would write meta files into the same subagents/feat_x/ directory), but that is pre-existing behaviour from sanitizeFilenameComponent and is orthogonal to the cache correctness claim.
R1-4 refuted — sessionAgentTrace uses template-literal URL concatenation, not new URL().
The prior review claims this method "builds its URL with bare new URL(...)." The actual line (see inline at DaemonClient.ts:3384) is `${this.baseUrl}/session/${urlEncode(sessionId)}/agent-trace${suffix}` — the same pattern as every other method in this class. The TypeError: Invalid URL failure mode described in R1-4 does not apply.
R1-2 refuted — rejected getTasks is preserved as a pending tab, not dropped.
The prior review claims ".catch(() => undefined) makes every error code indistinguishable from absence" and permanently deletes tabs from localStorage. The actual code (line 7408) uses .catch(() => null), and the null path explicitly returns { id, kind: 'pending', title, targetKind, sourceSessionId, taskId }, keeping the tab alive for lazy re-hydration. The code does drop tabs only when getTasks succeeds and the task is genuinely not found (!task || task.kind !== tab.kind), which is the intended behaviour for legitimately deleted tasks.
Confirmed suggestion
R1-35 confirmed — literal 500 in dispatch.ts duplicates MAX_VIRTUAL_SESSION_ID_PART_LENGTH without importing it (see inline). The REST handler at session.ts:4792 imports the constant correctly. The WebSocket dispatch handler and acpAgent.ts:8552 use the literal 500. If the constant is ever changed these two transports will diverge silently.
Cannot confirm or refute
- R1-5 (trimTranscriptState eviction during backward hydration): mechanism is plausible but requires running the code to settle — not run in this environment.
- R1-1 (stale snapshot briefly shown on panel reopen): the poller re-fires when
environmentNeedsAgentschanges back to true, so staleness should be bounded to one fetch-round-trip; could not run to confirm.
Unreviewable dimensions
- Test (windows-latest, macos-latest) SKIPPED: path-handling and filesystem code not exercised on those platforms for this head.
- Integration Tests (CLI, No Sandbox) SKIPPED: no coverage over the new
GET /session/:id/agentsandGET /session/:id/agent-traceroutes at the integration level. - No local toolchain: execution rungs 1-3 not run.
Reviewed with AI assistance.
| rootAgentId !== undefined && | ||
| (typeof rootAgentId !== 'string' || | ||
| rootAgentId.length === 0 || | ||
| rootAgentId.length > 500) |
There was a problem hiding this comment.
R1-35 confirmed. This literal 500 duplicates MAX_VIRTUAL_SESSION_ID_PART_LENGTH = 500 from virtual-subagent-sessions.ts without importing it. The REST handler at session.ts:4792 uses the constant correctly. Both this block and acpAgent.ts:8552 should import and reuse the constant so a future change to the limit stays in sync across both transports.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Approving after independent cross-check (see prior comment for full findings).
Checked:
- R1-2, R1-3, R1-4 (Critical from prior review): all refuted — the mechanisms described do not match the actual code
- R1-35 (literal
500vs constant): confirmed, suggestion-level - CI: Desktop Shell green; Test (ubuntu) green; Test (windows/macos) and Integration Tests (CLI) SKIPPED — platform and integration coverage gaps noted but not blocking
Not run (no local toolchain): R1-5 (trimTranscriptState eviction during backward hydration) could not be executed. Worst-case is tab restoration hitting the 1,000-page cap and falling back to pending — bounded, non-destructive.
Remaining suggestions (R1-35, R1-12 panel width) can be addressed in follow-up.
Reviewed with AI assistance.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the daemon/SDK core of this diff — LGTM on the production paths:
- Agent snapshots (
agent-transcript.ts,tasksSnapshot.ts): metadata validation is thorough (field typing,parentSessionId === sessionId, filename round-trip throughsanitizeFilenameComponent), the 2,000-node bound warns on truncation, lineage resolution handles cycles and orphans explicitly, and the sidecar cache re-checks directory mtime plus per-meta signatures before reuse with bounded entries and read concurrency. - New endpoints (
session/agents,session/agent_trace,session/attachmentsWS + REST twins): all gated by session ownership (requireOwned/withOwnerReadSession),rootAgentIdvalidated on both surfaces; transcriptdirection=backwardparsing is strict with cursor-param mutual exclusion. - Attachments listing: safe-name round-trip, pending-upload exclusion, readdir/stat TOCTOU tolerated, deterministic ordering.
- Persistence: only lookup keys and tab metadata are stored per the design; capability checks keep older daemons compatible.
Notes for the record: the 25 open review threads are all ci-bot R1 [Suggestion]-level items (test-coverage gaps, the hardcoded 500 vs MAX_VIRTUAL_SESSION_ID_PART_LENGTH duplication confirmed by chiga0); none is a correctness blocker in the shipped paths, and a human reviewer has approved this exact head. Ubuntu unit-test check was still running at review time — merge should wait for it to land green.
Verification report — head
|
|
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. |
…tachments # Conflicts: # packages/cli/src/acp-integration/acpAgent.ts # packages/cli/src/serve/acp-http/transport.test.ts # packages/cli/src/serve/routes/session.ts # packages/cli/src/serve/server.test.ts # packages/cli/src/serve/server/telemetry-catalog.test.ts # packages/cli/src/serve/server/telemetry.test.ts # packages/sdk-typescript/test/unit/DaemonSessionClient.test.ts # packages/web-shell/client/App.tsx # packages/web-shell/client/components/panels/EnvironmentPanel.tsx # packages/web-shell/client/hooks/useSessionArtifacts.ts
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.
11 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- literal-500 rootAgentId cap in dispatch.ts/acpAgent.ts — already reported as R1-35 (comment 3896505465; maintainer-confirmed at 3901199699; author deferred to a transport-parity follow-up)
- readAgentTrace negative paths untested (agent-transcript.ts:219) — already reported as R1-19 (comment 3896505494, open)
- image attachmentId propagation steps untested (ui/transcript.ts:343) — already reported as R1-20 (comment 3896505502, open)
- three identical pending-state blocks in the restore effect (App.tsx:5608) — already recorded in the round-8 deferral list (review 5101285810, App.tsx:5593 entry)
- hydrate calls inside the setArtifactPanelTabs updater (App.tsx:6337) — already recorded in the round-8 deferral list (review 5101285810, App.tsx:6322 entry)
- lineageState populated but never read (EnvironmentPanel.tsx:66) — already reported as R1-34 (comment 3896505628; author deferred to a Workflow lineage follow-up)
- disabled-state terminal release one-shot without retry (TerminalPanel.tsx:121) — already reported as R1-15 (comment 3896505635; author deferred to a terminal lifecycle follow-up)
- sessionAgentTrace agent-layer handler untested (acpAgent.ts:8745) — already reported as R1-18 (comment 3896505457; author deferred to an ACP handler test follow-up)
- 1000-page restore cap untested (App.tsx:5397) — already reported as R1-25 (comment 3896505517, open)
- listSessionAttachments unknown-session guard unpinned (bridge.ts:12898) — already recorded in the round-6 deferral list (review 5093773203, bridge.ts:12875 entry)
- capabilities-envelope test unreachable by workspace test commands (qwen-serve-routes.test.ts:392) — already reported as R6-3 (round-6 review body, review 5093773203)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": core subagent Config sessionId inheritance was not traced to completion, so daemon-side resolution of level-2 nested subagents against the top-level session id ….
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/acpAgent.test.ts:10310 — [review] D9-1 sessionAgents payload hygiene assertions missingpackages/cli/src/serve/routes/session.ts:4593 — [review] D9-2 (fix-induced by the R8-1 fix) backward-flush legacy-bridge fallback untestedpackages/cli/src/acp-integration/session/tasksSnapshot.test.ts:319 — [probe] D9-3 paused-duration freeze test cannot discriminate the frozen formulapackages/cli/src/acp-integration/session/tasksSnapshot.ts:465 — [probe] D9-4 sidecar cache evicts just-hit sessions; hits never refresh recencypackages/sdk-typescript/test/unit/DaemonSessionClient.test.ts:956 — [probe] D9-5 reload hydration assertion too weak to catch dropped media datapackages/web-shell/client/App.test.tsx:4716 — [review] D9-6 attachment-backed file-tab restore branch untestedpackages/web-shell/client/App.test.tsx:2989 — [review] D9-7 right-panel persistence eviction loop untested
Convergence: round 9 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/cli/src/acp-integration/session/tasksSnapshot.ts (findings in round 8; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 2, previous 2), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 11 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI.
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：core subagent Config sessionId inheritance was not traced to completion, so daemon-side resolution of level-2 nested subagents against the top-level session id …。
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 9 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/cli/src/acp-integration/session/tasksSnapshot.ts（第 8 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 2 条 Critical），首次发现的速率没有下降（本轮 2，上一轮 2），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.23.0)
…tachments # Conflicts: # docs/developers/daemon/00-index.md # packages/cli/src/acp-integration/acpAgent.ts # packages/cli/src/serve/server/telemetry-catalog.test.ts # packages/cli/src/serve/server/telemetry.test.ts # packages/sdk-typescript/test/unit/daemon-public-surface.test.ts
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.
Unresolved, please confirm:
- [Critical] R1-5 (packages/web-shell/client/App.tsx, comment 3896505444's sibling claim on transcriptEventsToMessages trim budget): body truncated in the context file and not fetched in full this round; could not be ruled against the reviewed head.
- [Critical] R5-12 (packages/web-shell/client/App.tsx: sourceTurnId positional-ordinal stability of review-tab restore): body truncated in the context file and not fetched in full this round; could not be ruled against the reviewed head.
- [Critical] R7-1 (packages/web-shell/client/App.tsx: artifactPanelStateBySessionRef lifecycle for same-session re-restores): body truncated in the context file and not fetched in full this round; could not be ruled against the reviewed head.
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 — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round; 1 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/web-shell/client/App.tsx:6763 — [review] Critical [fails-closed] [new-surface] agent-trace retry ceiling reset by every effect restart; workflow.loadFailed unreachable while agents churn (probe-confirmed)packages/acp-bridge/src/sessionAttachments.ts:676 — [review] copyFrom restamps mtime; forked sessions list attachments in copy/name order, not upload order (probe-confirmed)packages/cli/src/serve/acp-http/transport.test.ts:12373 — [review] WS rate-tier test does not pin _qwen/session/agents and _qwen/session/agent_trace as reads (sweep-confirmed)packages/web-shell/client/App.tsx:6341 — [review] hydration side effects run inside a React state updater; StrictMode double-invokes thempackages/web-shell/client/components/artifacts/AgentWorkflow.test.tsx:100 — [review] fork-prefix strip branch of displayName has no testpackages/web-shell/client/components/artifacts/ArtifactPanel.tsx:907 — [review] new pending/file/image loading render branches have no component-level testspackages/cli/src/acp-integration/acpAgent.ts:8791 — [review] sessionAgentTrace ext-method handler has no QwenAgent-layer test (mutation green)packages/cli/src/serve/routes/session.ts:4435 — [review] cursor+beforeRecordId mutual exclusion untested (mutation returns 200)packages/acp-bridge/src/bridge.ts:12906 — [review] listSessionAttachments unknown-session SessionNotFoundError branch untestedpackages/core/src/agents/agent-transcript.ts:213 — [review] readAgentTrace ENOENT empty-trace branch untestedpackages/cli/src/acp-integration/session/tasksSnapshot.test.ts:186 — [review] sidecar cache-hit / directory-mtime invalidation never exercised (probe-confirmed)packages/cli/src/acp-integration/session/tasksSnapshot.ts:359 — [review] agents-status truncation at 2000 sidecars is lexicographic and silent (probe-confirmed)packages/cli/src/serve/capabilities.ts:126 — [review] direction=backward ships without a capability tag; old daemons silently mis-pagepackages/cli/src/serve/server.test.ts:10017 — [review] agent-trace rootAgentId absent-200 / over-length-400 branches untestedpackages/cli/src/serve/server.test.ts:27552 — [review] direction validation on the workspace transcript twin route untestedpackages/acp-bridge/src/status.ts:993 — [review] trace warnings wire field populated but never read; truncation diagnostics hiddenpackages/acp-bridge/src/sessionAttachments.ts:673 — [review] list()'s only error-throwing path is untested (probe-confirmed)packages/cli/src/acp-integration/session/tasksSnapshot.test.ts:153 — [review] retention tests pin the count 32, never the newest ordering (probe-confirmed)packages/cli/src/acp-integration/session/tasksSnapshot.ts:341 — [review] cache-hit verification re-reads all sidecars unbounded, bypassing the pLimit(8) cap (probe-confirmed)packages/core/src/agents/agent-transcript.ts:393 — [review] writeAgentMeta's new atomic write is pinned by no test- …and 3 more (see the run report)
Convergence: round 10 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/web-shell/client/App.tsx (findings in round 9; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (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.)
[Critical] R5-3: [regression] The backward-read flush makes the standing guarantee in docs/developers/qwen-serve-protocol.md false, and the diff does not touch that page. The workspace_persisted_transcript paragraph (and docs/users/qwen-serve.md) documents GET /workspaces/:workspace/session/:id/transcript as a daemon-local pager that does not start ACP, query live bridge state, or load settings; post-diff every ?direction=backward first page calls runtime.bridge.flushSessionTranscript(sessionId) (routes/session.ts:4585-4604 at the reviewed head), which round-trips through the workspace bridge and can spawn the ACP child and load that workspace's settings. Still standing at the reviewed head: verified the flush block is present in the workspace route and git diff ..HEAD over docs/developers/qwen-serve-protocol.md and docs/users/qwen-serve.md is empty. Amend the docs to scope the claim (backward pages round-trip through a one-record flush barrier; forward and cursor pages remain daemon-local) and add direction/beforeRecordId to the query-parameter table and the 400 bullet.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 3 条（原文未翻译，列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 10 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 23 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 10 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/App.tsx（第 9 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R5-3: [regression] The backward-read flush makes the standing guarantee in docs/developers/qwen-serve-protocol.md false, and the diff does not touch that page. The workspace_persisted_transcript paragraph (and docs/users/qwen-serve.md) documents GET /workspaces/:workspace/session/:id/transcript as a daemon-local pager that does not start ACP, query live bridge state, or load settings; post-diff every ?direction=backward first page calls runtime.bridge.flushSessionTranscript(sessionId) (routes/session.ts:4585-4604 at the reviewed head), which round-trips through the workspace bridge and can spawn the ACP child and load that workspace's settings. Still standing at the reviewed head: verified the flush block is present in the workspace route and git diff ..HEAD over docs/developers/qwen-serve-protocol.md and docs/users/qwen-serve.md is empty. Amend the docs to scope the claim (backward pages round-trip through a one-record flush barrier; forward and cursor pages remain daemon-local) and add direction/beforeRecordId to the query-parameter table and the 400 bullet.
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
Round 10 follow-up is in ab9f804:
Verification: CLI InputPrompt 215/215; Web Shell App + TerminalPanel 761/761; affected-package typechecks; root build; root typecheck; ESLint; Prettier; two clean diff audits. |
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:
- D11-12 attachments-listing catch silently empties the section — already reported as R1-29 (comment 3896505556)
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 — its prompt was built, but no agent was launched with it — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries, and cannot be certified.
1 Suggestion(s) were drafted inline past the resolved critical posting floor; the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 11, 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:
docs/developers/qwen-serve-protocol.md:2307 — [review] Critical [fails-closed] [new-surface] doc lists direction=forward as valid; daemon 400s itdocs/developers/qwen-serve-protocol.md:2308 — [review] Critical [fails-closed] [new-surface] doc says beforeRecordId requires direction=backward; code…integration-tests/cli/qwen-serve-routes.test.ts:393 — [review] R11-3: The test file this diff extends is outside every npm workspace, so npm run test --workspaces never collects it; the new capability assertions gate nothing unless the in…packages/acp-bridge/src/sessionAttachments.ts:673 — [review] list() degraded-primary fallback branch untestedpackages/acp-bridge/src/sessionAttachments.ts:677 — [review] branched-session attachments lose upload order (copyFile…packages/cli/src/acp-integration/acpAgent.ts:8791 — [review] sessionAgentTrace child handler untestedpackages/cli/src/acp-integration/session/tasksSnapshot.test.ts:54 — [review] buildSessionAgentsStatus 2000-sidecar cap untested;…packages/cli/src/serve/acp-http/transport.test.ts:12373 — [review] WS read-tier for 2 of 3 new methods unpinnedpackages/cli/src/serve/server.test.ts:26935 — [review] flush-before-read contract and fallback branch unpinnedpackages/core/src/agents/agent-transcript.ts:196 — [review] readAgentTrace guard branches untestedpackages/sdk-typescript/src/daemon/ui/transcript.ts:341 — [review] attachmentId chain middle links untestedpackages/web-shell/client/App.test.tsx:7305 — [review] composition test does not assert the monitor tab openedpackages/web-shell/client/App.tsx:472 — [review] lazy-restore page-count cap untestedpackages/web-shell/client/App.tsx:4277 — [review] fresh [] literal defeats React bailout during streamingpackages/web-shell/client/App.tsx:4410 — [review] empty persisted entries evict real saved panel statepackages/web-shell/client/App.tsx:5270 — [review] no in-flight guard on pending-tab hydration (duplicate…packages/web-shell/client/App.tsx:6360 — [review] hydration side effects inside a StrictMode-doubled state…packages/web-shell/client/components/artifacts/AgentWorkflow.test.tsx:100 — [review] fork-prefix strip branch of displayName untestedpackages/web-shell/client/components/panels/EnvironmentPanel.tsx:64 — [review] lineageState written but never rendered
Convergence: round 11 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/web-shell/client/App.tsx (findings in round 10; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (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.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (3 Critical(s)), the rate of first-time findings is not falling (this round 2, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
[Critical] R10-2: [certifies-falsely] [regression] The round-10 doc fix updated the route-section paragraph but missed the workspace_persisted_transcript capability-tag sentence (docs/developers/qwen-serve-protocol.md:265), which still says the route "does not start ACP, query live bridge state, load settings" — yet the backward flush barrier may start ACP and load workspace settings. The workspace route now crosses a flush barrier on a backward first page for a live session (session.ts:4585-4600) -> bridge.flushSessionTranscript -> requestSessionTranscriptPage -> withEnsuredWorkspaceControl -> ensureChannel(), spawning the workspace ACP child. The PR's own rewritten route section concedes the barrier "may start ACP and load workspace settings" — a same-file contradiction. Integrators gate on capability tags; a poller issuing backward first pages against idle live sessions unexpectedly triggers ACP spawns and settings loads. Witness: bridge.test.ts "flushes transcripts through the backward page barrier" — a bridge with no channel spawned one through its channelFactory when flushSessionTranscript was called. Fix: update line 265 to keep the true negatives but replace "does not start ACP, query live bridge state, load settings" with: forward and cursor pages stay daemon-local; a backward first page for a live session crosses a one-record workspace-bridge flush barrier that may start ACP and load workspace settings. The barrier fires only under cursor === undefined && direction === "backward" && promptStateBeforeRead.live (session.ts:4585-4589); untrusted secondary workspaces never hold live bridge sessions, so the rewrite must keep "Registered untrusted secondary workspaces may read" true and daemon-local for them.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查：反向审计——它的 prompt 已构建，但没有 agent 用它启动——负责搜寻评审其余部分遗漏问题的这道工序，即便运行过，也缺失了 brief 承载的方法，无法作证。
1 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 11 轮，非阻断）——已记录，本轮不要求修改；其中 2 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 19 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 11 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/App.tsx（第 10 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 3 条 Critical），首次发现的速率没有下降（本轮 2，上一轮 1），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
[Critical] R10-2: [certifies-falsely] [regression] The round-10 doc fix updated the route-section paragraph but missed the workspace_persisted_transcript capability-tag sentence (docs/developers/qwen-serve-protocol.md:265), which still says the route "does not start ACP, query live bridge state, load settings" — yet the backward flush barrier may start ACP and load workspace settings. The workspace route now crosses a flush barrier on a backward first page for a live session (session.ts:4585-4600) -> bridge.flushSessionTranscript -> requestSessionTranscriptPage -> withEnsuredWorkspaceControl -> ensureChannel(), spawning the workspace ACP child. The PR's own rewritten route section concedes the barrier "may start ACP and load workspace settings" — a same-file contradiction. Integrators gate on capability tags; a poller issuing backward first pages against idle live sessions unexpectedly triggers ACP spawns and settings loads. Witness: bridge.test.ts "flushes transcripts through the backward page barrier" — a bridge with no channel spawned one through its channelFactory when flushSessionTranscript was called. Fix: update line 265 to keep the true negatives but replace "does not start ACP, query live bridge state, load settings" with: forward and cursor pages stay daemon-local; a backward first page for a live session crosses a one-record workspace-bridge flush barrier that may start ACP and load workspace settings. The barrier fires only under cursor === undefined && direction === "backward" && promptStateBeforeRead.live (session.ts:4585-4589); untrusted secondary workspaces never hold live bridge sessions, so the rewrite must keep "Registered untrusted secondary workspaces may read" true and daemon-local for them.
— qwen3.8-max via Qwen Code /review (v0.23.0)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Light review pass on head a4abd30. No blocking findings in the new daemon/SDK/serve surface: the session/agents, session/agent_trace, and session/attachments routes all go through ownership checks and parameter validation; readAgentTrace and buildSessionAgentsStatus bound fan-out (MAX_AGENT_TRACE_NODES, p-limit concurrency, LRU sidecar cache with mtime+signature invalidation) and handle cycles/orphans; writeAgentMeta is now atomic via temp+rename; web-shell persistence is versioned, key-only, and capped (MAX_ARTIFACT_PANEL_SESSION_STATES), and side-task creation dedups via an in-flight promise map. All required checks are green at review time (only the automatic review-pr job still running).
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-review — all verifiable Criticals confirmed fixed.
Verified at head a4abd30b:
| Finding | Status |
|---|---|
R1-1 environmentAgentTasks unconditionally prefers stale snapshot |
✅ Fixed — now gated on environmentNeedsAgents || workflowTabActive |
R1-2 .catch(() => undefined) permanently deletes tabs on transient error |
✅ Fixed — null sentinel preserves tabs as pending |
R1-3 agentSidecarCache keyed only on sanitized dir |
✅ Not a bug — key is ${dir}\0${sessionId}, raw sessionId prevents collision |
R1-4 sessionAgentTrace uses new URL(...) sync-throw |
✅ Fixed — uses URLSearchParams string concat like sibling methods |
R1-5 transcriptEventsToMessages keeps live-window byte budget |
✅ Fixed — maxRetainedBytes: Number.POSITIVE_INFINITY |
| R2-11 workspace-qualified backward transcript never flushes | ✅ Fixed — flush guard present in workspace route (routes/session.ts:5006) with capability fallback |
| R5-9 status handlers key sidecar dir off lookup-normalized sessionId | ✅ False positive — code uses persistedSessionId = session.getConfig().getSessionId() |
Checked (backend): tasksSnapshot.ts cache invalidation logic; sessionAttachments.ts list() TOCTOU handling; acpAgent.ts session ID plumbing; dispatch.ts parameter validation; bridge.ts flush mechanism; transport.test.ts rate-tier classification.
Scope: Backend files (21 changed files reviewed). App.tsx Rounds 5–11 bot findings (R5-1 through R11-2) not individually verified at the code level; human reviewers (yiliang114, chiga0 Round-4 tracking) assessed production paths as sound.
Open suggestions (non-blocking, deferred per prior rounds):
- R1-35:
rootAgentIdlength cap500is a literal in dispatch.ts/acpAgent.ts; should importMAX_VIRTUAL_SESSION_ID_PART_LENGTH - R1-24: WS read-tier classification test does not cover
_qwen/session/agentsand_qwen/session/agent_trace - R2-18:
list()skipspendingNamesbut notremovingNames(unlike otherSessionAttachmentStoreconsumers)
Reviewed with AI assistance.
|
test connectivity |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-approval after main merge — no new issues introduced.
The 19 files changed between the previous reviewed head (a4abd30b) and this head (a43060b210) are entirely incoming changes from origin/main (opentui batch 8/9 — packages/cli/src/ui/opentui/ and docs/tests). None overlap with this PR's changed files.
All previously verified fixes remain intact (R1-1 through R1-5, R2-11 workspace flush guard).
Reviewed with AI assistance.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Approval — no blocking findings at current HEAD.
Verified at head a43060b (current):
| Finding | Status |
|---|---|
R5-12: review-tab restore keys on sourceTurnId (positional-ordinal instability) |
✅ Addressed — findReviewChangesByToolCallIds(changesByTurn, tab.sourceToolCallIds) is the primary key at both restore sites (App.tsx:5426–5429, 5769–5772); sourceTurnId is ??-fallback only for legacy tabs lacking sourceToolCallIds |
R7-1: artifactPanelStateBySessionRef lifecycle for same-session re-restores |
✅ Addressed — saveCurrentState() is called before every re-restore path (including the newlyAvailableDeferredTab bypass), guarded by artifactPanelRestoredSessionKeyRef.current === nextSessionId; the bucket correctly reflects current panel state before the re-restore reads from it at line ~5694 |
Prior backend criticals (confirmed fixed at a4abd30b, unmodified since):
- R1-1
environmentAgentTasksstale snapshot — fixed - R1-2
.catch(() => undefined)tab deletion on transient error — fixed - R1-4
sessionAgentTracesync throw — fixed - R1-5
transcriptEventsToMessagesbyte budget — fixed - R2-11 backward transcript flush — fixed
Open non-blocking suggestions (carry-forward):
- R1-29: ci-bot suggestion (round 11)
- R1-35:
rootAgentIdlength cap should referenceMAX_VIRTUAL_SESSION_ID_PART_LENGTH - R1-24: WS read-tier classification test coverage gap for
_qwen/session/agents - R2-18:
list()skipspendingNamesbut notremovingNames
Reviewed with AI assistance.
5 participants
What this PR does
This PR makes the Web Shell environment panel a durable entry point for session context. Attachments, artifacts, subagents, and background tasks are shown in expanded sections with first-load skeletons and stable empty states. Subagent inventory and hierarchy come from dedicated session snapshots, and the Workflow view can open nested agents without replacing the current side panel.
It also restores right-side panel tabs after refresh or session switching. Only stable lookup keys and tab metadata are stored locally; content is fetched after session loading completes, inactive tabs remain lazy, and refresh restoration opens without animation. Artifact, attachment, terminal, subagent, workflow, and side-task tabs share this behavior.
Why it's needed
Previously, refreshing or switching sessions could discard the user's right-side workspace, transcript pagination could hide completed subagents, and environment attachments or artifacts flashed or disappeared while loading. The new behavior preserves navigation without persisting payloads such as base64 data and keeps historical agent information available independently of transcript pagination.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: refresh and session switching lost right-side tabs; completed subagents could disappear outside the loaded transcript page; loading sections could flash from skeleton to empty content.
After: tabs and selection restore per session after load, content is fetched lazily, agent history comes from dedicated snapshots, and environment sections have stable loading and empty states.
Tested on
Environment (optional)
Local worktree with the daemon development server. Full repository build and typecheck passed. Targeted verification passed for 1719 CLI tests, 604 TypeScript SDK tests, 728 Web Shell tests, and 37 core agent-trace tests.
Risk & Scope
Linked Issues
N/A
中文说明
这个 PR 做了什么
这个 PR 将 Web Shell 的环境信息面板改造成可持久恢复的会话上下文入口。附件、产物、子智能体和后台任务默认展开，首次加载展示骨架屏，空状态保持稳定。子智能体清单和层级关系来自独立的会话快照，Workflow 视图可以打开嵌套智能体，而不会替换当前侧边面板。
同时，刷新或切换会话后会恢复右侧面板页签。本地只存储稳定的查询键和页签元数据；内容在会话加载完成后请求，未激活页签保持懒加载，刷新恢复时不播放打开动画。产物、附件、终端、子智能体、Workflow 和 Side Task 页签都使用同一套行为。
为什么需要
此前刷新或切换会话可能丢失用户的右侧工作区，transcript 分页可能让已完成的子智能体不可见，环境附件或产物在加载时也可能闪烁或消失。新行为在不持久化 base64 等 payload 的前提下保留导航状态，并让历史智能体信息独立于 transcript 分页持续可用。
Reviewer 测试计划
如何验证
证据（前后对比）
改动前：刷新和切换会话会丢失右侧页签；已完成的子智能体可能因 transcript 页面未加载而消失；加载区块可能从骨架屏闪到空内容。
改动后：每个会话会在 load 后恢复页签和选中状态，内容按需请求，智能体历史来自独立快照，环境信息各区块具有稳定的加载和空状态。
已测试平台
环境（可选）
本地 worktree 与 daemon 开发服务器。全仓 build 和 typecheck 通过；定向验证通过 1719 个 CLI 测试、604 个 TypeScript SDK 测试、728 个 Web Shell 测试和 37 个 core agent-trace 测试。
风险与范围
关联 Issue
无