feat(web-shell): Bind GitHub PRs to sessions with sidebar badge and search - #9543
Conversation
…earch When a PR is created from the Web Shell Git dialog, bind its number and URL to the current session. The daemon accepts the binding through the session metadata routes (validated at the route, bridge, SDK, and sidecar layers, with the URL restricted to http(s) since it is rendered as a link target), keeps it in live memory, and persists it as a per-session sidecar file so the binding survives daemon restarts and follows the session through archive/unarchive/delete. The sidebar renders a #N badge next to the session title (opening the PR via the desktop-aware external-link opener, shows the PR in the details tooltip, and the session search now also matches PR number, branch name, and worktree slug — so with many concurrent sessions, the one that produced a given PR is one search away. EOF )
E2E test report (protocol-level + browser UI)Environment: locally built bundle ( Protocol level — 8/8 PASS (against the built bundle):
Browser UI (mock-daemon Playwright harness) — PASS: Unit/integration: core 169, acp-bridge 11, cli serve 36 (metadata) + 509 (acp-http), web-shell 171 (sidebar) + dialog/tooltip suites — all green; build, typecheck, ESLint, Prettier clean. |
🖼️ 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 |
|
Re-run on the current head — the PR grew through five review rounds since the first pass, so this gate re-checks the final shape. Template looks good ✓ Problem: Real, observed UX gap — unchanged from the first pass and still uncontradicted: heavy Web Shell users run dozens of concurrent sessions, the Git dialog discards the created PR's number/URL after a status line, and nothing answers "which session produced PR #N". The design doc traces the broken chain precisely, and claude-code's CHANGELOG has shipped session↔PR linking in the same product category. Direction: Aligned. Session lifecycle management is core Web Shell territory, and this closes a genuine gap in it. The growth since the first pass (single Size: 49 files — ~1,209 production lines / ~2,868 test lines / 86 docs / 11 lockfile (production vs test vs docs breakdown per the Stage 0 counting rules). Core paths touched ( Approach: Scope still feels right. One validated binding threaded through the layers that already carry session metadata (route → bridge → SDK → SSE → ACP dispatch), persisted with the established worktree-sidecar pattern, badge + search on top; the hydration-before-mutation and persisted-vs-live merge machinery is what makes the list correct across daemon restarts, not extra furniture. The large deletion in Risk: No elevated risk signals — none of the 49 changed files match the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明在当前 head 上重跑——自首轮审查以来 PR 经过五轮评审演进,本次门禁按最终形态重新核对。 模板完整 ✓ 问题: 真实的 UX 缺口——与首轮结论一致且未被推翻:重度 Web Shell 用户会同时运行几十个会话,Git 对话框创建 PR 后只显示一条状态消息就丢弃 PR 号/URL,无法回答"哪个会话产生了 PR #N"。设计文档精确梳理了断链位置,claude-code 的 CHANGELOG 也已在同品类产品中落地会话↔PR 关联。 方向: 对齐。会话生命周期管理是 Web Shell 的核心领域,本 PR 补齐其中真实缺口。自首轮以来的增长(单 规模: 49 个文件——生产代码约 1,209 行 / 测试约 2,868 行 / 文档 86 行 / lockfile 11 行(按 Stage 0 计数规则拆分)。触及核心路径( 方案: 范围仍然合理。一个经过校验的绑定沿着已有的会话元数据链路贯穿(route → bridge → SDK → SSE → ACP dispatch),用既有的 worktree-sidecar 模式持久化,badge 与搜索叠加其上;mutation 前回填历史与 persisted/live 合并机制正是列表在 daemon 重启后仍然正确的原因,而非多余摆设。 风险: 无升级风险信号——49 个改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first: extend the existing session-metadata plumbing (route → bridge → SDK → SSE → ACP dispatch) with a validated PR binding, persist it in a per-session sidecar copying the worktree-sidecar lifecycle, enrich session lists from it, and put a badge + search on top — with a bounded list, because a session can produce stacked PRs. That is exactly what this PR does; I didn't find a simpler path it missed. This is the sixth pass over the PR (five review rounds preceded it), so beyond re-reading the final diff end to end I focused on verifying the R5 round's fixes actually landed in the
Standing properties I re-verified in the final diff: validation before any mutation (a combined Non-blocking observations:
sequenceDiagram
participant P1 as GitDialog web-shell
participant P2 as Daemon metadata route
participant P3 as Bridge live memory
participant P4 as PR sidecar file
participant P5 as Session list enrichment
participant P6 as Sidebar and pickers
P1->>P2: PR created, bind number and url
P2->>P4: hydrate persisted history (best effort)
P2->>P3: validate, then upsert on the session entry
P2->>P4: upsert persists the binding, reply echoes the list
P3-->>P6: catalog revision bump, refetch shows the badge
Note over P4,P5: after a daemon restart enrichment merges sidecar history back in
Files changed (30 of 49 shown)
…and 19 more files — Delete/Release/Resume dialog filters and the SDK plumbing (DaemonSessionClient, exports, build.js comment), i18n keys, the package-lock peer markers, and the colocated tests for the files above. TestingUnattended CI run — I did not build or execute any PR code. Evidence is the PR's own CI read through the API at the reviewed commit, fetched once (no polling). All six The author's earlier E2E report in this thread (protocol-level checks + mock-daemon Playwright visuals, macOS) remains the author's claim — attributed as-is, not independently re-run here. Sandboxed verification would settle the last gap:
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 中文说明代码审查独立方案先行:扩展既有会话元数据链路(route → bridge → SDK → SSE → ACP dispatch),加入校验过的 PR 绑定,用 worktree-sidecar 同款生命周期持久化,在会话列表回填,再加 badge 与搜索——并且因为有 stacked PR 场景,必须是有上限的列表。这个 PR 正是这么做的;我没有找到它遗漏的更简路径。 这是第六次过这个 PR(此前已有五轮评审),因此除了端到端重读最终 diff,重点核对了 R5 轮修复是否真实落在
最终 diff 中复核确认的既有性质:任何变更前先完成全部校验( 非阻塞观察:
测试无人值守 CI 运行——未构建或执行任何 PR 代码。证据来自在受审 commit 上通过 API 读取的 PR 自身 CI(一次性抓取,不轮询)。 受审 head 上六个 作者早前在线程中贴出的 E2E 报告(协议级验证 + mock-daemon Playwright 视觉,macOS)仍为作者声明——如实转述,未独立复跑。 沙箱验证可以补齐最后一个缺口: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean review across every stage; the only residuals are cosmetic (a stale build.js comment and a ZH wording choice), named in Stage 2. Stepping back: this PR answers one question — "which session produced PR #N" — and five review rounds have worn it down to a shape I'd merge. It extends the machinery already built for session metadata instead of bolting on a parallel system; the write path validates before it mutates at every layer; persistence copies the proven worktree-sidecar lifecycle wholesale, including the unglamorous cases (split-pair merges on archive moves, cleanup in both states, chats-dir migration); and the UI stays best-effort so a binding failure can never shadow a successful PR creation. The security-conscious pieces are real, not performative: http(s)-only URLs with length and control-character caps enforced consistently at four trust boundaries, session-id gates before any filesystem path construction, and a response/event surface that echoes the authoritative persisted list instead of lifetime-only memory. Going back to my independent proposal — the PR matches it, and the places it goes further (the bounded multi-PR list, hydration-before-mutation, live/persisted merge) are each pinned by a test that would fail if the machinery were removed. That's the check that matters: this suite does not pass unchanged without the diff. Every R5-round fix was verified in the actual increment, not taken on the reply's word. All 40+ review threads are resolved, and CI is fully green on the reviewed head including the behavior-bearing jobs (Serve A/B, Real daemon E2E, web-shell E2E Smoke, visuals). Reservations, none blocking: the stale 199→200KB budget comment after the rebase, the uncapped live/persisted merge edge that only a failed-disk scenario reaches, and the 合并请求 vs 拉取请求 wording. If I had to maintain this in six months, the sidecar precedent and the doc comments at each subtle spot (queue failure semantics, merge policy, ordering guarantees) would let me read it immediately. Verdict: approve — CI already landed green, so no deferred marker; approving pinned to the reviewed commit. 中文说明置信度:4/5 —— 各阶段审查均干净;仅剩外观层面的小项(一条过时的 build.js 注释与中文措辞),已在 Stage 2 点名。 整体看:这个 PR 只回答一个问题——"哪个会话产生了 PR #N"——五轮评审已把它打磨到可以直接合入的形态。它扩展现有的会话元数据机制而非另起炉灶;写入路径在每一层都先校验再变更;持久化完整复刻经过验证的 worktree-sidecar 生命周期,包括不起眼的边角(归档迁移时的分裂对合并、两种状态的清理、chats 目录迁移);UI 保持尽力而为,绑定失败绝不遮盖 PR 创建成功。安全相关的部分是实打实的,不是摆设:http(s)-only URL 加长度与控制字符上限,在四个信任边界一致执行;文件系统路径构造之前先过会话 ID 门禁;响应/事件面回显权威持久化列表而非仅本生命周期记忆。 对照我的独立方案——PR 与之吻合,且它走得更远的地方(有上限的多 PR 列表、mutation 前回填、live/persisted 合并)各自都有"删掉机制就会失败"的测试钉住。这才是关键检验:这套测试不是没有 diff 也能原样通过的。R5 轮的每一项修复都是在真实增量里核对过的,不是凭回复采信。40+ 个评审线程全部已解决,受审 head 上 CI 全绿,包括承载行为验证的任务(Serve A/B、Real daemon E2E、web-shell E2E Smoke、视觉截图)。 保留意见,均不阻塞:rebase 后过时的 199→200KB 预算注释、只有磁盘失败场景才会触及的 live/persisted 合并未设上限边角、"合并请求"与"拉取请求"的措辞。六个月后维护这段代码,sidecar 先例加上每个微妙处的文档注释(队列失败语义、合并策略、顺序保证)能让我立刻读懂。结论:批准——CI 已全绿落定,无需延迟标记;批准钉在受审 commit 上。 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
A session can produce several PRs (stacked or follow-up work), and keeping only the latest binding would defeat the sidebar's search-by-PR-number flow for every earlier one. The binding is now a bounded list (10, oldest dropped) ordered by binding time: re-binding the same number refreshes it and moves it to latest, the badge shows the newest number with a +N overflow, the tooltip lists every bound PR, and search matches any of them. The write API stays single-binding per call; reads, SSE events, and responses carry the full list, with the sidecar as the complete history merged over the live entry's daemon-lifetime bindings.
Conflict: the SDK browser-bundle budget — main bumped 198→199KB for persistent session attachments while this branch needed headroom for the session PR binding types; resolved at 200KB with both reasons recorded. Also fixes a latent type error currently on main: QwenLM#9477 added deleteSessionAttachments to the deleteDaemonSessions bridge contract, but the batch-delete test from QwenLM#9341 still passes a closeSession-only mock; CI did not catch it because the verify check is skipped on main pushes.
|
Update: two follow-up commits pushed.
Re-verified against the built bundle: two sequential bindings accumulate (
|
The case-variant delete test fix carried by the previous merge is superseded by QwenLM#9551, which landed the same mock on main; this merge takes main's version of session-archive.test.ts.
…alogs The mission-control overview panel and the shared session picker row (resume / delete / release dialogs) now show the same PR badge as the sidebar — latest number with a +N overflow, opening the PR via the desktop-aware opener — and the resume dialog's search matches bound PR numbers, branch names, and worktree slugs through the shared sessionMatchesGitQuery helper.
Conflict in sessionService.ts: main's prompt-ledger lifecycle (QwenLM#9426) landed in the same spots as the PR-sidecar lifecycle — both are kept: removeSessionFiles removes both, archive/unarchive move both.
The overview card badge used the panel's neutral --primary tint while the sidebar and picker badges use the accent violet; one element should read the same on every surface.
ytahdn
left a comment
There was a problem hiding this comment.
Review Verdict / 审查结论
🔄 Request changes — one verified 🔴 plus two 🟡. All findings below were cross-checked against the head tree (031f4fc). Fixes are small. Data is intact in the sidecar throughout, so this is a read/display correctness issue, not corruption.
🔄 请求修改 —— 1 条已验证 🔴 + 2 条 🟡。以下每条都已对照 head tree(031f4fc)复核。修复都不大。数据在 sidecar 中始终完好,因此这是读取/展示层的正确性问题,不是数据损坏。
🎉 Praise / 值得肯定
Validation is genuinely thorough: the number/url checks are identical across all four layers (route, bridge, SDK validator, sidecar reader — Number.isInteger + >0, ^https?://), so javascript:/data: are rejected everywhere. Path traversal is closed off (SESSION_FILE_PATTERN gate + live-registry resolution, so ../ can't reach the upsert). The bridge validates a combined displayName+pr request fully before any mutation (no partial apply). The core sidecar lifecycle (archive / unarchive / delete) mirrors the worktree sidecar line-for-line with no gap. The in-memory bridge upsert and the on-disk sidecar upsert agree on dedupe / ordering / cap. Search is exact-equality, so there's no regex-injection surface. This is careful work.
校验做得非常扎实:number/url 在 route、bridge、SDK 校验器、sidecar reader 四层完全一致(Number.isInteger+>0、^https?://),javascript:/data: 在哪一层都被拒。路径穿越已封堵(SESSION_FILE_PATTERN 门禁 + live 注册表解析,../ 到不了 upsert)。bridge 对组合请求 displayName+pr 在任何变更前先全量校验(不部分生效)。core 的 sidecar 生命周期(归档/取消归档/删除)与 worktree sidecar 逐行平行、无遗漏。bridge 内存 upsert 与磁盘 sidecar upsert 的去重/排序/cap 语义一致。搜索是精确等值比较,无正则注入面。这是下过功夫的实现。
🔴 1. Post-restart bindings hide sidecar history — the live merge overwrites the enriched full prs list
Where / 位置: packages/cli/src/serve/server/session-list.ts — mergeLiveSessionSummary (481-501) and enrichPrSidecars (411-443).
Problem / 问题 (two facets of one root bug / 同一根因的两面):
-
The merge branch inside
enrichPrSidecars(433-439, documented as "live url wins, live-only bindings sort latest") is dead code. It runs before the live merge (call sites 560, 1231), at which point the map only holds persisted summaries fromtoSummary(445-470), which never setprs. Sosummary.prsis alwaysundefinedthere and the merge never executes.
enrichPrSidecars里的合并分支(433-439,注释称"live url 优先、live-only 排最后")是死代码:它在 live 合并之前运行(560、1231),此刻 map 里只有toSummary(445-470)产出的 persisted summary,而toSummary从不设置prs,所以那里summary.prs恒为 undefined,合并永远不执行。 -
The real live+persisted merge happens later in
mergeLiveSessionSummary, which does{...existing, ...live}.prsis not handled explicitly, so a non-empty liveprs(this daemon lifetime only) wholesale overwrites the enriched full list.
真正的 live+persisted 合并发生在其后的mergeLiveSessionSummary,它用{...existing, ...live}朴素展开——prs没有专门处理,live 非空的prs(仅本 daemon 生命周期)会整体覆盖富集后的完整列表。
Failure scenario / 失败场景 (exactly the stacked-PR-across-restart case this PR targets / 正是本 PR 主打的 stacked PR + 重启场景): bind PR A → daemon restarts → session goes live again (live entry.prs is empty; the bridge never re-hydrates from the sidecar) → bind PR B (sidecar=[A,B], live=[B]). The enriched [A,B] is overwritten by {...existing,...live} down to [B] — the sidebar badge / search / tooltip loses A. All three list paths (default / organized / metadata-filtered) go through this merge, so all are affected. The data is still in the sidecar, but to the user the older PR is simply un-findable — which defeats the PR's own stated promises ("survive daemon restarts" + "search by older PR number").
先绑 PR A → daemon 重启 → 会话重新拉起(live entry.prs 为空,bridge 从不从 sidecar 恢复)→ 再绑 PR B(sidecar=[A,B]、live=[B])。富集出的 [A,B] 被 {...existing,...live} 覆盖成 [B]——侧栏 badge/搜索/tooltip 丢失 A。default/organized/metadata-filtered 三条列表路径都经过这个合并函数,全部受影响。数据仍在 sidecar 里,但对用户就是"较早的 PR 查不到了",直接击穿 PR 描述里"重启后仍在"+"按较早 PR 号反查"两条核心承诺。
Suggested fix / 建议修复 — merge prs explicitly in mergeLiveSessionSummary (same semantics as enrichPrSidecars: dedupe by number, live wins, keep history):
在 mergeLiveSessionSummary 里对 prs 做显式合并(与 enrichPrSidecars 相同语义:按 number 去重、live 优先、保留历史):
if (existing.prs || live.prs) {
const livePrs = live.prs ?? [];
merged.prs = [
...(existing.prs ?? []).filter(
(p) => !livePrs.some((l) => l.number === p.number),
),
...livePrs,
];
}(Alternatively, re-run the enrichment after the live merge.) / (或把富集挪到 live 合并之后重跑。)
🟡 2. The new pr-only session_metadata_updated event clears displayName in the SDK's public fold
Where / 位置: producer packages/acp-bridge/src/bridge.ts:9708-9714 (publishes {sessionId, prs}, no displayName); consumer packages/sdk-typescript/src/daemon/events.ts:2029-2034 (displayName: event.data.displayName).
Problem / 问题: Before this PR the event always carried displayName, so "absent" on the wire meant "cleared" (pinned by test/unit/daemonEvents.test.ts:1126-1152, which asserts a bare {sessionId} clears the name). This PR adds a displayName-less variant that still passes isSessionMetadataUpdatedData, but the fold wasn't updated — so any SDK consumer folding the event stream via reduceDaemonSessionEvent gets its session title reset to undefined the moment a PR is bound, until the next rename/load/snapshot. Combined {displayName, pr} requests are worse: the first event sets the name, the trailing pr event wipes it.
此前该事件总带 displayName,"缺省"在 wire 上等价"清除"(test/unit/daemonEvents.test.ts:1126-1152 明确把裸 {sessionId} 断言为清除)。本 PR 新增无 displayName 的变体且仍能通过 isSessionMetadataUpdatedData,但 fold 没同步——任何用 reduceDaemonSessionEvent 折叠事件流的 SDK 消费者,在绑定 PR 的瞬间标题被重置为 undefined,直到下次改名/loadSession/快照才恢复。组合请求更糟:第一个事件设置名字,紧随的 pr 事件又把它清掉。
Suggested fix / 建议修复 — echo the current name on the producer (name present → echoed; already cleared → stays absent), so existing folds stay correct with no consumer changes:
在生产端把当前名字回传(有名字原样回传、已清除时仍缺省),这样既有 fold 无需改动即正确:
data: {
sessionId,
...(entry.displayName !== undefined
? { displayName: entry.displayName }
: {}),
prs: entry.prs,
},🟡 3. upsertSessionPr read-modify-write races under the shared lock, silently dropping a binding
Where / 位置: packages/core/src/services/session-pr-service.ts:110-122; callers packages/cli/src/serve/routes/session.ts:5253 (withOwnerMutableSession → runSharedMany) and packages/cli/src/serve/acp-http/dispatch.ts:2920 (withMutableOwned → runSharedMany).
Problem / 问题: runSharedMany only reference-counts a session — it does not serialize concurrent shared holders (session-archive.ts:128-143). upsertSessionPr is read → filter → write with awaits in between and no mutual exclusion, so two concurrent bindings for the same session interleave: A reads [] → B reads [] → A writes [A] → B writes [B], and A is silently overwritten. While the daemon lives the in-memory list still shows both, but after a restart the sidecar is the only persisted source, so the dropped binding is gone for good. Two review batches flagged this independently; the trigger (consecutive stacked-PR binds, two attached tabs) is exactly what the docstring says is supported.
runSharedMany 对同一 session 只做引用计数、并不串行化并发的 shared 持有者(session-archive.ts:128-143)。upsertSessionPr 是带 await 的"读-过滤-写",中间无互斥,于是同一 session 的两个并发绑定会交错:A 读 [] → B 读 [] → A 写 [A] → B 写 [B],A 被静默覆盖。daemon 存活时内存列表仍显示两条,但重启后 sidecar 是唯一持久源,丢失的绑定永久消失。两个审查批次独立发现了这条;触发场景(stacked PR 连续绑定、两个已 attach 的标签页)正是 docstring 明示支持的。
Suggested fix / 建议修复 — serialize inside the service with a per-path in-process queue (a failed predecessor must not block later binds):
在服务内按文件路径做进程内队列串行化(前序失败不阻塞后续绑定):
const upsertQueue = new Map<string, Promise<unknown>>();
export function upsertSessionPr(filePath, pr): Promise<SessionPr[]> {
const run = async () => { /* existing read-filter-write body */ };
const previous = upsertQueue.get(filePath) ?? Promise.resolve();
const next = previous.catch(() => undefined).then(run);
upsertQueue.set(filePath, next);
void next.finally(() => {
if (upsertQueue.get(filePath) === next) upsertQueue.delete(filePath);
});
return next;
}🟢 Nits / 建议
4. ACP dispatch rewrites the sidecar on any metadata update (even a pure rename). dispatch.ts:2915-2924 reads result?.prs?.[last] and upserts; the bridge returns prs whenever entry.prs is non-empty (bridge.ts:9720-9723) regardless of whether this call bound a PR. So a displayName-only rename re-upserts the latest binding, refreshing createdAt and moving it to the end — with a full list this can evict a genuinely older entry early. Gate on metadata['pr'] !== undefined.
ACP dispatch 在任何 metadata 更新(含纯改名)时都重写 sidecar:dispatch.ts:2915-2924 取 result?.prs?.[last] 做 upsert,而 bridge 只要 entry.prs 非空就返回 prs(bridge.ts:9720-9723),与本次是否绑定 PR 无关。于是 displayName-only 改名也会重 upsert 最新绑定、刷新 createdAt 并移到末尾——列表满时可能提前挤出真正更早的条目。建议按 metadata['pr'] !== undefined 门控。
5. No length cap on pr.url (body limit is 10MB), asymmetric with the 256-char displayName cap in the same route. A token-holding client could stuff 10 near-10MB urls into the sidecar (~100MB), which every subsequent session-listing then reads + parses in full. Add url.length > 2048 to parseSessionPrBody's rejection.
pr.url 无长度上限(body 上限 10MB),与同路由 displayName 的 256 上限不对称。持有 token 的客户端可塞 10 个近 10MB 的 url,sidecar 膨胀到 ~100MB,之后每次列表都整文件读+解析。建议 parseSessionPrBody 加 url.length > 2048 拒绝。
6. No component-level scheme guard on the rendered href (defense-in-depth). Four spots render href={pr.url} directly; useExternalLinkOpener early-returns without preventDefault for non-http(s). Upstream validation currently guarantees http(s), so this is only reachable if a future path bypasses validation — consider filtering with isExternalOpenUrl before render.
渲染的 href 无组件级 scheme 防护(纵深防御):4 处直接 href={pr.url},useExternalLinkOpener 对非 http(s) 提前 return 且不 preventDefault。当前上游校验保证 http(s),只有未来某条绕过校验的路径才会触发——建议渲染前用 isExternalOpenUrl 过滤。
7. Test coverage gaps / 测试覆盖缺口. (a) The enrichPrSidecars merge logic — exactly where finding 1 lives — has zero assertions on the prs returned by the list endpoint, and "survive restart" only exercises the clientCount: 0 persisted path, never the live merge; that's why finding 1 slipped through. (b) A couple of server.test.ts cases explicitly depend on sidecar residue from earlier cases (the comment says "9517 from the previous test"), so they break if run alone or reordered. (c) bridge.test.ts has no combined-request atomicity case (valid displayName + invalid pr → name must not apply), even though the code comment states that property. (d) GitDialog's bind-failure degradation (.catch → console.warn, must not mask PR-creation success) has no regression test.
(a)enrichPrSidecars 合并逻辑——正是 finding 1 所在——对列表接口返回的 prs 零断言,且"重启后仍在"只测了 clientCount: 0 的 persisted 路径、没测 live 合并,这正是 finding 1 漏网的原因。(b)server.test.ts 有用例显式依赖前序用例的 sidecar 残留(注释自述"9517 from the previous test"),单跑或重排即失败。(c)bridge.test.ts 缺组合请求原子性用例(displayName 合法 + pr 非法时名字不应生效),尽管代码注释声明了该性质。(d)GitDialog 绑定失败降级(.catch→console.warn,不遮盖 PR 创建成功)无回归测试。
8. (Optional) dialog search consistency. Only ResumeDialog wires sessionMatchesGitQuery; Delete/Release show the badge but can't search by PR number. One line each if that wasn't a deliberate scope cut.
(可选)对话框搜索一致性:只有 ResumeDialog 接入了 sessionMatchesGitQuery,Delete/Release 能看到 badge 却搜不到 PR 号。若非有意收窄,各一行即可补齐。
Bottom line / 小结
Fix finding 1 before merge (it defeats the feature's core persistence + reverse-lookup promise); findings 2 and 3 are strongly worth fixing in the same pass. All are small, well-scoped changes. Everything else is polish.
合并前请修复 finding 1(它击穿本功能的核心持久化+反查承诺);finding 2、3 建议一并处理。都是很小的、范围明确的改动。其余为打磨项。
Read/display correctness (verified by ytahdn and the R1 review):
- mergeLiveSessionSummary merged {..existing, ..live} wholesale, so a
live entry's this-daemon-lifetime prs overwrote the sidecar-enriched
full history after a restart; prs is now merged by PR number (live
url wins, history kept), and the dead merge branch in
enrichPrSidecars is gone.
- The pr-only session_metadata_updated event carried no displayName,
which SDK folds treat as "cleared" — the title blanked on every PR
bind. The producer now echoes the current name.
- GitDialog synced sessionIdRef from the prop on every render, so the
fresh session id the dialog resolves for its own side queries was
clobbered before the binding read it; the sync now runs only when
the prop changes.
Robustness:
- upsertSessionPr's read-modify-write is serialized per sidecar path,
closing the concurrent-bind drop race under runSharedMany.
- The REST routes persist the sidecar before mutating the bridge, so a
failure on either side leaves the binding durable.
- The ACP dispatch only upserts when the call actually binds a PR (a
displayName-only rename no longer rewrites createdAt/order).
- pr.url is capped at 2048 chars across all four validation layers.
Structure & a11y:
- The three badge copies (sidebar / overview / picker) are now one
SessionPrBadge component: shared CSS, count-aware aria-label,
non-http(s) entries filtered defensively, and tabIndex=-1 inside
listbox options.
- The SDK's duplicated PR validator is a single session-pr module used
by both DaemonClient and events.
- Delete/Release dialogs' search matches bound PR numbers like Resume.
Tests: list-level live+sidecar prs merge, sidecar-vs-bridge echo
authority, route tests made order-independent, bridge
atomicity/cap/catalog-revision/displayName-echo cases, concurrent
upsert serialization, SDK fold keeps the name, GitDialog bind-failure
degradation, url-cap rejections at every layer.
|
@ytahdn Thanks for the thorough review — all findings are addressed in 🔴 1. Post-restart history loss — Fixed. The dead merge branch in 🟡 2. Title blanked by the pr event — Fixed. The producer echoes the current 🟡 3. upsert read-modify-write race — Fixed. 🟢 4. ACP re-upsert on rename — Fixed: the dispatch now gates on 🟢 5. No url length cap — Fixed: 2048 chars at all four layers (route / bridge / SDK / sidecar), with rejection tests at each. 🟢 6. Component-level scheme guard — Fixed: the three badge copies are now a single 🟢 7. Test gaps — (a) list-level merge test added (see 🔴1) and the workspace-route tests are now order-independent (each removes the sidecar it depends on first); (b) same; (c) atomicity test added (valid 🟢 8. Delete/Release search consistency — Fixed: both wire One deliberate non-fix from the inline batch: the GitDialog binding targets the connected-session prop without an ownership check because the daemon route already rejects cross-workspace updates (409), so a wrong-workspace binding degrades to a warning instead of landing — and creating a session just to bind stays ruled out. |
ytahdn
left a comment
There was a problem hiding this comment.
Re-review(增量提交 a60b0db "Address review findings on PR bindings")
上一轮 8 项 findings 全部验证已修复(逐条核实,见文末)。但修复提交把两条路由改为 persist-first(先写 sidecar 再调 bridge),这个重排本身引入了一条新的 Critical 和两条 Important。
🔴 Critical — persist-first 让归档会话在 active 目录遗留孤儿 sidecar,unarchive 时冲突并静默丢失较早的绑定
位置:packages/cli/src/serve/routes/session.ts workspace 路由 5378-5390(无条件写 'active' 路径);交互方 packages/core/src/services/sessionService.ts moveOptionalFile 1016-1017(目标已存在即 throw)、unarchive 2043-2052(throw 仅 warn、不移动)。
问题:该路由在 bridge 调用前无条件向 getPrSessionPathForArchiveState(sessionId, 'active') 写 sidecar。对已归档会话(resolveLiveSessionOwner 返回 not_found,能通过 5334-5361 检查;fallback 注释明确这是受支持的场景),bridge 抛 SessionNotFoundError,fallback 再按真实 location('archived')写一份——同一绑定落到两个文件,active 成为孤儿。旧的 bridge-first 顺序不会发生:bridge 先抛,根本走不到 active upsert。这是本次提交重排引入的回归。
失败场景(stacked PR,逐行追证可达):
- 会话绑定 PR A(live)→ active sidecar
[A]; - 归档会话 → sidecar 随 transcript 搬到 archived → archived=
[A],active 无文件; - 归档状态下经该路由(SDK
DaemonClient.updateSessionMetadata打的正是这条路)再绑 PR B → persist-first 写 active=[B](孤儿),fallback 写 archived=[A,B],响应正确; - 取消归档 →
moveOptionalFile(archived→active)因 active 已存在抛 "Archive sidecar conflict",仅 warn 不移动; - 结果:会话已 active,列表 enrichment 读 active=
[B]——PR A 从徽章/tooltip/搜索中静默消失;完整历史[A,B]孤立留在 archived 目录,无清理路径,只有一条误导性警告。location === 'conflict'分支同样在抛错前留下孤儿。
建议修复:persist 前先解析真实归档态,写正确路径:
if (pr) {
const service = createWorkspaceRuntimeSessionService(runtime);
const located = await service.getSessionLocation(sessionId);
assertRuntimeGenerationOpen?.();
if (located === 'conflict') throw new SessionConflictError(sessionId);
persistedPrs = (
await upsertSessionPr(
service.getPrSessionPathForArchiveState(sessionId, located ?? 'active'),
pr,
)
).map(({ number, url }) => ({ number, url }));
}(live 会话 transcript 在 active,located ?? 'active' 与原行为一致;fallback 的按 number 幂等 upsert 可保留不动。)
🟡 Important — /session/:id/metadata(route 1)persist-first 把"合法 pr + 非法 displayName"变成部分写入
位置:routes/session.ts 5224-5241(只判 typeof === 'string',无空串/控制字符校验)+ 5248-5257(先写 sidecar)+ 5258(后调 bridge);对照 workspace 路由 5305-5329 有完整本地校验。
问题:route 1 把 displayName 内容校验委托给 bridge。旧 bridge-first 顺序下这安全(bridge 先抛、不写 sidecar);改成 persist-first 后委托不再安全。失败场景:PATCH /session/:id/metadata 发 {displayName: "bad\u0001name", pr: {合法}} → route 校验通过 → 先写 sidecar(pr 落盘)→ bridge 因控制字符抛 InvalidSessionMetadataError → 客户端收到错误响应,但绑定已持久化:请求"失败"而徽章"生效",sidecar 与 live entry 背离。这正是 PR 明确承诺的原子性(bridge 测试 does not apply displayName when the combined pr is invalid 钉住了 bridge 层,route 层顺序却绕过了它)。
建议修复:persist 前补上与 workspace 路由一致的 displayName 本地校验(建议抽成共享 helper 供两条路由复用),或让 route 1 回到 bridge-first——该路由的会话必然 live,没有 workspace 路由的 fallback 诉求。
🟡 Important — upsertSessionPr 队列清理在写失败时产生未处理 rejection
位置:packages/core/src/services/session-pr-service.ts 137-139。
问题:void next.finally(...) 派生出一个新 promise;当 run 因 I/O 失败(ENOSPC/EACCES 等)reject 时,调用方 await 的是 next(各调用点都有兜底),但派生的 finally promise 同样 reject 且无人接手 → unhandledRejection。serve 守护进程入口未注册兜底处理器(gemini.tsx:239 的处理器只覆盖交互式 CLI 路径;workspace-extensions-controller.ts 里的同类注释说明这是已知雷区)。失败场景:磁盘满/权限错误时绑定 PR → 除正常 5xx 外,Node ≥15 默认直接终止进程——一次 sidecar 写失败不应拖垮整个 daemon。
建议修复:让清理链自身消化 rejection:
const cleanup = (): void => {
if (upsertQueue.get(filePath) === next) upsertQueue.delete(filePath);
};
void next.then(cleanup, cleanup);
return next;🟢 Nit
- route 层字面量 2048:
routes/session.ts:2115用字面量且错误文案硬编码 "2048",而 core 已导出SESSION_PR_URL_MAX_LENGTH(session-pr-service.ts:32)且本文件已从同一模块导入upsertSessionPr。建议用常量替换,避免日后调整上限时脱节。 - route 层缺 url>2048 拒绝测试:bridge/sidecar 两层都有 >2048 拒绝用例,route 层校验(2115)没有——与"各层有拒绝测试"的描述不符。
- 共享 badge 类把 sidebar 的
margin-left: 6px带进了 overview/picker:SessionPrBadge.module.css:4是三份旧拷贝的并集,但旧 overview.prBadge/旧 picker.picker-item-pr-badge无左边距(间距来自父容器 gap 8px/4px)——重构后这两处 badge 间距各多 6px。纯视觉。 - 统一后的
onKeyDownstopPropagation 波及 picker 键盘导航:SessionPrBadge.tsx:57对所有场景 stopPropagation。picker 的useListboxKeyboard挂在 window 冒泡阶段(hooks/useListboxKeyboard.ts:213,注释明确它是传播链最后一站),旧 picker badge(031f4fce 的 SessionRow)没有 onKeyDown 拦截;现在用户点击 badge 打开 PR 后焦点停在tabIndex={-1}的 anchor 上,方向键(及 DialogShell 的 Escape)被吞、导航卡住,直到重新点击行本体。建议 picker 场景(tabIndex === -1时)不拦截 keydown。
💡 Suggestion
- list 级合并测试目前只覆盖不相交 number(
[9500]+[9517]);mergeLiveSessionSummary的"同 number 跨两侧、live url 赢"分支(session-list.ts:502-507)在 list 级没有测试锚点,建议补一条 sidecar[{n, oldUrl}]+ live[{n, newUrl}]→[{n, newUrl}]的用例。 SessionPrBadge的 scheme 过滤行(SessionPrBadge.tsx:33-34)是本次安全统合的核心,但无直接测试:过滤分支(全坏 → null、混合列表只渲染可打开项)从未被执行于测试,未来误删会静默回归。建议挂组件直测。
上一轮 findings 复核(全部已验证修复)
- 🔴 重启后历史丢失:
mergeLiveSessionSummary按 number 显式合并(session-list.ts:500-508,live url 赢、live-only 排尾),死分支已从enrichPrSidecars移除;三种边界(单侧、双侧同 number 不同 url)走查正确;list 级测试真实断言。✔ - 🟡 pr 事件清空标题:唯一携带 prs 的发射点(bridge.ts)已回显当前 displayName;
reduceDaemonSessionEvents测试钉住。✔ - 🟡 upsert 竞态:按 sidecar 路径串行的队列实现正确(前驱失败不阻塞、身份比对清理、与 archive/unarchive/delete 的 exclusive 锁互斥),并发测试真并发。✔(但见上方未处理 rejection)
- 🟢 ACP 重命名重复 upsert:dispatch gate 在入参
metadata['pr']。✔ - 🟢 url 2048 上限:四层实现俱在(route 层测试缺,见 Nit)。✔
- 🟢 badge 组件级化:单一
SessionPrBadge渲染前经isExternalOpenUrl过滤,全部消费方迁移完毕,无残留自渲染;count-aware aria-label、listbox tabIndex override 统合。✔ - 🟢 测试缺口:原子性、绑定失败降级、workspace-route 顺序独立化均落实。✔
- 🟢 Delete/Release 搜索:三处
sessionMatchesGitQuery语义一致。✔
结论 | Verdict
🔄 请求修改 — 上一轮问题全部妥善修复,值得肯定;但 persist-first 重排引入的孤儿 sidecar 会在本 PR 明确承诺的 archive/unarchive 生命周期里静默丢失绑定,建议先处理 🔴 与两条 🟡(均为小改动)。
Re-review of incremental commit a60b0db "Address review findings on PR bindings"
All 8 findings from the previous round are verified fixed (item-by-item at the end). However, the fix commit reordered both metadata routes to persist-first (sidecar write before the bridge call), and that reordering itself introduces one new Critical and two Important issues.
🔴 Critical — persist-first strands an orphan active sidecar for archived sessions; unarchive then conflicts and silently drops earlier bindings
Location: packages/cli/src/serve/routes/session.ts, workspace route 5378-5390 (unconditional write to the 'active' path); interacting with packages/core/src/services/sessionService.ts moveOptionalFile 1016-1017 (throws when the destination exists) and unarchive 2043-2052 (the throw is warn-only, the file is not moved).
Problem: the route writes the sidecar to getPrSessionPathForArchiveState(sessionId, 'active') before calling the bridge. For an archived session (resolveLiveSessionOwner returns not_found and passes the checks at 5334-5361 — the fallback comment states this is a supported scenario), the bridge throws SessionNotFoundError and the fallback re-upserts at the real location ('archived') — the same binding lands in two files and the active one becomes an orphan. The old bridge-first order could not do this: the bridge threw before any active upsert ran. This is a regression introduced by this commit's reordering.
Failure scenario (stacked PRs, traced line by line):
- Session binds PR A while live → active sidecar
[A]; - Session archived → sidecar moves with the transcript → archived=
[A], no active file; - Bind PR B through this route while archived (the SDK's
DaemonClient.updateSessionMetadatatargets exactly this route) → persist-first writes active=[B](orphan), the fallback writes archived=[A,B], response is correct; - Unarchive →
moveOptionalFile(archived→active)throws "Archive sidecar conflict" because active exists; warn-only, nothing moves; - Result: the session is active and listing enrichment reads active=
[B]— PR A silently disappears from badge/tooltip/search; the full history[A,B]is stranded in the archived directory with no cleanup path, behind a misleading warning. Thelocation === 'conflict'branch leaves the same orphan before throwing.
Suggested fix: resolve the real archive state before persisting and write to the correct path:
if (pr) {
const service = createWorkspaceRuntimeSessionService(runtime);
const located = await service.getSessionLocation(sessionId);
assertRuntimeGenerationOpen?.();
if (located === 'conflict') throw new SessionConflictError(sessionId);
persistedPrs = (
await upsertSessionPr(
service.getPrSessionPathForArchiveState(sessionId, located ?? 'active'),
pr,
)
).map(({ number, url }) => ({ number, url }));
}(Live sessions keep their transcript in the active dir, so located ?? 'active' matches today's behavior; the number-keyed fallback upsert stays idempotent and untouched.)
🟡 Important — persist-first on /session/:id/metadata (route 1) turns "valid pr + invalid displayName" into a partial write
Location: routes/session.ts 5224-5241 (only typeof === 'string', no empty/control-character validation) + 5248-5257 (sidecar written first) + 5258 (bridge called after); contrast the workspace route's full local validation at 5305-5329.
Problem: route 1 delegates displayName content validation to the bridge. Under the old bridge-first order that was safe (bridge threw before any sidecar write); persist-first makes the delegation unsafe. Failure scenario: PATCH /session/:id/metadata with {displayName: "bad\u0001name", pr: {valid}} → route validation passes → sidecar written (pr persisted) → bridge throws InvalidSessionMetadataError on the control character → the client receives an error response, yet the binding is durable: the request "failed" while the badge "took effect", and sidecar/live diverge. This is precisely the atomicity the PR promises (the bridge test does not apply displayName when the combined pr is invalid pins it at bridge level, but the route-level ordering bypasses it).
Suggested fix: validate displayName locally before persisting (ideally a shared helper used by both routes), or revert route 1 to bridge-first — its sessions are necessarily live, so it has no fallback need.
🟡 Important — the upsertSessionPr queue cleanup produces an unhandled rejection on write failure
Location: packages/core/src/services/session-pr-service.ts 137-139.
Problem: void next.finally(...) derives a new promise; when run rejects on I/O failure (ENOSPC/EACCES/...), callers await next (handled at every call site), but the derived finally promise rejects with the same reason and nobody catches it → unhandledRejection. The serve daemon entry registers no fallback handler (gemini.tsx:239 covers only the interactive CLI path; the same-pattern comments in workspace-extensions-controller.ts show this is a known landmine). Failure scenario: binding a PR while the disk is full or permissions are wrong → on top of the expected 5xx, Node ≥15's default terminates the process — one failed sidecar write should not take down the daemon.
Suggested fix: make the cleanup chain absorb the rejection:
const cleanup = (): void => {
if (upsertQueue.get(filePath) === next) upsertQueue.delete(filePath);
};
void next.then(cleanup, cleanup);
return next;🟢 Nits
- Literal 2048 at route layer:
routes/session.ts:2115hardcodes the limit and the error text "2048", while core exportsSESSION_PR_URL_MAX_LENGTH(session-pr-service.ts:32) and this file already importsupsertSessionPrfrom the same module. Use the constant so a future limit change cannot desync. - Missing route-layer url>2048 rejection test: bridge and sidecar both have >2048 rejection cases; the route check (2115) has none — at odds with "rejection tests at each layer".
- Shared badge class carries the sidebar's
margin-left: 6pxinto overview/picker contexts:SessionPrBadge.module.css:4is the union of the three old copies, but the old overview.prBadge/ picker.picker-item-pr-badgehad no left margin (spacing came from the parent gap of 8px/4px) — both contexts now render the badge 6px further out. Purely visual. - The unified
onKeyDownstopPropagation reaches picker keyboard navigation:SessionPrBadge.tsx:57stops propagation in every context. The picker'suseListboxKeyboardlistens at window bubble phase (hooks/useListboxKeyboard.ts:213, documented as the last stop of the propagation chain), and the old picker badge (SessionRow at 031f4fc) had no onKeyDown interception; after the user clicks the badge to open a PR, focus stays on thetabIndex={-1}anchor and arrow keys (and DialogShell's Escape) are swallowed — navigation stalls until the row body is clicked again. Consider skipping the interception in picker context (whentabIndex === -1).
💡 Suggestions
- The list-level merge tests only cover disjoint numbers (
[9500]+[9517]); the "same number on both sides, live url wins" branch ofmergeLiveSessionSummary(session-list.ts:502-507) has no list-level anchor — add a sidecar[{n, oldUrl}]+ live[{n, newUrl}]→[{n, newUrl}]case. - The scheme-filter line in
SessionPrBadge(SessionPrBadge.tsx:33-34) is the core of this round's security consolidation but has no direct test: neither filter branch (all-bad → null; mixed list renders only openable entries) is ever exercised, so a future accidental removal would silently regress. A small component-level test would pin it.
Previous-round findings re-check (all verified fixed)
- 🔴 Post-restart history loss:
mergeLiveSessionSummarynow merges by PR number (session-list.ts:500-508, live url wins, live-only entries sort latest); the dead branch is gone fromenrichPrSidecars; all three boundary shapes (one-sided, same-number-different-url) trace correctly; the list-level test genuinely asserts. ✔ - 🟡 pr event blanking the title: the only prs-carrying emitter (bridge.ts) now echoes the current displayName; pinned by a
reduceDaemonSessionEventstest. ✔ - 🟡 upsert race: the per-sidecar-path queue is correct (failed predecessor doesn't block, identity-checked cleanup, mutually exclusive with archive/unarchive/delete exclusive locks); the concurrency test is genuinely concurrent. ✔ (but see the unhandled rejection above)
- 🟢 ACP re-upsert on rename: dispatch gates on the inbound
metadata['pr']. ✔ - 🟢 url 2048 cap: implemented at all four layers (route-layer test gap noted above). ✔
- 🟢 Component-level scheme guard: single
SessionPrBadgefiltering throughisExternalOpenUrlbefore render; all consumers migrated, no residual self-rendering; count-aware aria-label and listbox tabIndex override unified. ✔ - 🟢 Test gaps: atomicity, bind-failure degradation, and workspace-route order-independence all landed. ✔
- 🟢 Delete/Release search consistency: all three dialogs wire
sessionMatchesGitQueryidentically. ✔
Verdict
🔄 Request changes — the previous round was addressed thoroughly and that work is appreciated; however the persist-first reordering introduces an orphan sidecar that silently loses bindings in exactly the archive/unarchive lifecycle this PR promises to support. The 🔴 and the two 🟡s are all small fixes.
|
@qwen-code /takeover |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 |
- SessionPrBadge: narrow onKeyDown to Enter only so the badge no longer blocks roving-listbox navigation keys in picker dialogs (R2-15). - SDK updateSessionMetadata: per-entry prs shape gate so a hostile or buggy daemon response cannot surface javascript: urls or malformed numbers downstream; valid entries survive (R1-17). - Tests: bridge mirror atomicity (valid pr + invalid displayName), GitDialog stale-id retry binding, list-level merge dedupe by number, organized + archived listing paths keep PR sidecars, DaemonClient pr request/parse + gate, Delete/Release dialog PR-number search (R2-6, R2-13, R2-14, R1-15, R1-17, R2-20).
R2/R4 review round — all 29 threads addressed (29/29 resolved)The six Criticals (persist-first orphans, unhandled rejection, path traversal, wedge-on-conflict moves, missing relocation) were fixed by the autofix commits
|
|
@qwen-code /resolve |
bun.lock conflict: main's 07405e6 (QwenLM#9395) regenerated the desktop lockfile properly (npmmirror URLs, @types/bun 1.4.0, live-host entry) — take main's version; this branch's earlier regenerated copy was made with a local bun whose registry config differs from CI.
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
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/windows-latest, Node 22.x) were skipped in CI; the changed workspaces unit suites ran on Linux only.
Not explored to full depth (tool budget reached): chunk 17: could not execute sessionSearch.test.ts , SessionRow.test.tsx , SessionDetailsTooltip.test.tsx — no node_modules in the review worktree or parent checkout…; chunk 2: executing the 12 new pr-binding tests in bridge.test.ts (worktree lacks node_modules/built dist; npm ci + build not run); chunk 15: none — all planned checks completed within budget (tests were not executed; per the brief, verification is a later stage's job)..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
docs/design/2026-08-20-webshell-session-pr-binding.md:49 — [review] The design doc states the badge stops propagation on ALL…packages/cli/src/serve/acp-http/dispatch.ts:2945 — [review] A repeated identical binding re-upserts the sidecar and…packages/core/src/services/session-pr-service.test.ts:72 — [review] The sidecar URL-length bound is tested only on the reject…packages/core/src/services/session-pr-service.test.ts:170 — [probe] The queue failure-handling test cannot distinguish the…packages/core/src/services/session-pr-service.test.ts:198 — [review] mergeSessionPrLists' stale-incoming dedupe branch is…packages/core/src/services/session-pr-service.ts:63 — [review] isValidSessionPrList is all-or-nothing, so one malformed…packages/core/src/services/session-pr-service.ts:151 — [review] upsertSessionPr persists the caller-supplied pr without…packages/sdk-typescript/src/daemon/DaemonClient.ts:6129 — [review] WorkspaceDaemonClient.updateSessionMetadata returns the…packages/sdk-typescript/src/daemon/events.ts:2662 — [review] isSessionMetadataUpdatedData rejects the whole…packages/sdk-typescript/src/daemon/session-pr.ts:1 — [review] The new SDK module session-pr.ts is the only file in…packages/web-shell/client/components/dialogs/GitDialog.test.tsx:856 — [review] The three new GitDialog binding tests never mock…
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查: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/windows-latest, Node 22.x) were skipped in CI; the changed workspaces unit suites ran on Linux only。
未探索到全部深度(达到工具调用预算):chunk 17:could not execute sessionSearch.test.ts , SessionRow.test.tsx , SessionDetailsTooltip.test.tsx — no node_modules in the review worktree or parent checkout…;chunk 2:executing the 12 new pr-binding tests in bridge.test.ts (worktree lacks node_modules/built dist; npm ci + build not run);chunk 15:none — all planned checks completed within budget (tests were not executed; per the brief, verification is a later stage's job).。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 11 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下: Autofix review round — no action takenThis round made no code changes and no commits. The branch stays at Growth audit (required this round): verdict
|
Best-effort hydration (R5-1/R5-2/R5-3): the sidecar hydration read at all three metadata-mutation sites (ACP dispatch, primary and workspace REST routes) now absorbs non-ENOENT I/O errors as "no sidecar" instead of failing the whole call — a squatted sidecar path no longer turns a pr-less rename into a 500/-32603. This also makes the R4-2 fallback ordering test reach the branch it names. Validation hardening (R5-4): pr.url rejects control characters at all four layers (bridge via hasControlCharacter, route, SDK guard, sidecar reader) — the url is interpolated into the stderr audit line, so a newline-bearing url could forge audit records. Traversal parity (R5-5, R4-5): the ACP session/update_metadata handler now gates on isValidSessionId before any sidecar I/O, and the primary REST route's gate moved ahead of runtime resolution so traversal ids get 400 invalid_session_id identically on single- and multi-workspace daemons (previously 404 on multi-entry registries). Tooltip (R5-6): PR rows key on index+number — a hand-edited sidecar with duplicate numbers no longer risks cross-row reconciliation. Tests: FakeBridge callLog pins seed-before-mutate order (R5-9); cross-workspace pr sidecar lands in the owning workspace's chats dir (R4-4); multi-workspace traversal test; metadata-filtered listing keeps prs (R1-15); Resume dialog PR-number search (R2-20); dialog fixtures annotated DaemonSessionSummary[] (R5-7/R5-8); control-char rejection cases at bridge and sidecar layers.
R4/R5 review round — all 37 threads addressed (0 unresolved)
R5 fixes in this round:
|
|
🤖 AutoFix ran out of time before finishing (timeout (1080000ms)) (attempt 1/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间(timeout (1080000ms))(第 1/100 次尝试)—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/32489159092 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 94 passed · 0 failed · 94 total Flakiness gate: 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:94 通过 · 0 失败 · 94 总计 抖动门: Verification reportPR 9543 Deep Verification — feat(web-shell): Bind GitHub PRs to sessions with sidebar badge and searchVerdict: 中文摘要结论: 四层校验(route / bridge / sidecar reader / SDK)经 41 项在线矩阵全部验证: 变异矩阵 6/6 全部击杀( Findings:仅两条 nit——① PR 描述"空 body 仍以 invalid_metadata 拒绝"仅适用于 workspace 作用域路由;主路由空 body 为 200 no-op(与 base 语义一致、非回归:base 路由源码无此门槛,且 A/B base 臂实测缺 displayName 的请求即为 200 no-op;字面 未覆盖:浏览器 UI 实际渲染(badge/tooltip/搜索交互,无浏览器环境;组件单测已覆盖)、ACP Central claim + A/BCentral claim: the daemon accepts a PR binding through the session metadata routes, validates it, persists it as a per-session sidecar, echoes the full list in responses/listings/SSE, and the binding survives daemon restarts with the persisted history hydrated before mutations. Identical scenario driven against the head build and a base control build (worktree at
Head arm 16/16, base arm 13/13 — every base-control assertion encodes the expected absence (base failing to bind is the control passing). Witness: FindingsNo blocking findings. Two nits: N1 (nit, description clarity). The PR description says "an empty body is still rejected with the same N2 (nit, cosmetic). Validation matrix (secondary claim 1 — four-layer validation), head build, live daemon41/41 (
Lifecycle (secondary claim 2 — sidecar follows the session), head build17/17 ( Note: archiving a live session that never produced a transcript returns Mutation matrix — vacuity of the new tests (6/6 killed, no survivors)
Tree verified clean ( Targeted gates (exact counts, unmutated)
The cli serve gate includes the PR's multi-workspace coverage (cross-workspace sidecar landing in the owning workspace's chats dir — R4-4, and multi-workspace traversal parity — R5-5), which the single-workspace live harness could not reach. Not covered
MethodologyEnvironment: CI merge-ref checkout at Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds (rounds 3-5 each reported findings; every reported finding was merged into confirmed entries or rejected).
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/windows-latest, Node 22.x) were skipped in CI; the changed workspaces' unit suites ran on Linux only.
Not reviewed: test-efficacy — probe kit inconclusive (harnessValidated: null; probe invocation artifact, the new test files run green directly); the mutation/hunk-survival dimension was unmeasured, not clean.
Not explored to full depth (tool budget reached): chunk 5: run packages/cli vitest for src/serve/multi-workspace-sessions.test.ts — not executed; review worktree has no node_modules or built dist prerequisites, …; "agent reverse-audit (round 4)": none — wait, per instructions I must not write a Budget gap line when nothing was cut short; no checks were curtailed, so no such line is emitted..
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
package-lock.json:17581 — [review] eleven lightningcss platform packages gain "peer": true with no package.json change — unrelated lockfile churndocs/design/2026-08-20-webshell-session-pr-binding.md:66 — [review] impact table names DaemonSessionSummary.pr/BridgeSessionSummary.pr (singular) but implementation uses prs arraysdocs/design/2026-08-20-webshell-session-pr-binding.md:45 — [review] doc claims live entry.prs is this-daemon-lifetime-only, but the code hydrates the full sidecar history (seedSessionPrs)packages/sdk-typescript/scripts/build.js:104 — [review] budget comment claims a nonexistent 199KB→200KB bump; duplicates the base's recorded transitionpackages/cli/src/serve/server/session-list.ts:437 — [review] enrichPrSidecars serves persisted prs uncapped; a hand-edited sidecar inflates every list fetchpackages/web-shell/client/components/SessionPrBadge.tsx:33 — [review] badge filter admits mailto: against its http(s)-only docstring; hostile mailto binding renders as a phishing seedpackages/sdk-typescript/src/daemon/types.ts:1276 — [review] DaemonSessionSummary.prs catalog/list ingress is a raw cast without the isDaemonSessionPrInfo gatepackages/sdk-typescript/src/daemon/DaemonClient.ts:6129 — [review] WorkspaceDaemonClient.updateSessionMetadata returns prs via unvalidated cast; the sibling primary client filterspackages/sdk-typescript/src/daemon/events.ts:2663 — [review] events validator prs arm has no rejection-branch test; mutation to true ships greenpackages/core/src/services/session-pr-service.ts:41 — [review] hasControlCharacter misses the C1 band (U+0080-U+009F); NEL in pr.url forges audit-line breakspackages/acp-bridge/src/bridge.test.ts:26718 — [review] invalid-pr table never tries number 0; <=0→<0 mutant ships green and a persisted 0 wipes the sidecar historypackages/acp-bridge/src/bridge.test.ts:26729 — [review] exactly-2048 url boundary unpinned across four duplicated length gates; off-by-one desyncs the layerspackages/core/src/services/session-pr-service.test.ts:80 — [review] createdAt typeof clause unpinned; a numeric createdAt breaks mergeSessionPrLists' localeCompare during movespackages/core/src/services/session-pr-service.test.ts:163 — [review] readSessionPrs ENOENT-vs-rethrow contract unpinned; unconditional-null mutant silently drops history on transient readspackages/core/src/services/session-pr-service.test.ts:174 — [review] no-wedge retry never exercises previous.catch; a failed predecessor skips the queued successor bindingpackages/core/src/services/sessionService.test.ts:2006 (+2 locations) — [review] split-pair merge tests under-pin movePrSidecar's contract (read order, tie-break, rename absence)packages/cli/src/serve/server.test.ts:26341 — [review] REST 'echoes the sidecar list' tests don't pin seed-before-mutation ordering (ACP path is pinned)packages/web-shell/client/components/SessionPrBadge.tsx:33 — [review] SessionPrBadge URL-gate branch has no direct test; filter-dropping mutant ships green across 686 testspackages/web-shell/client/components/dialogs/GitDialog.test.tsx:856 — [review] two GitDialog binding tests leave generation unmocked; every run logs a TypeError and passes via the fallbackpackages/cli/src/serve/acp-http/dispatch.ts:2910 — [review] ACP session/update_metadata isValidSessionId gate has no test; removal degrades to opaque -32603- …and 11 more (see the run report)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:reverse audit — stopped at the 5-round cap without two consecutive dry rounds (rounds 3-5 each reported findings; every reported finding was merged into confirmed entries or rejected)。
未审查: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/windows-latest, Node 22.x) were skipped in CI; the changed workspaces' unit suites ran on Linux only。
未审查:test-efficacy — probe kit inconclusive (harnessValidated: null; probe invocation artifact, the new test files run green directly); the mutation/hunk-survival dimension was unmeasured, not clean。
未探索到全部深度(达到工具调用预算):chunk 5:run packages/cli vitest for src/serve/multi-workspace-sessions.test.ts — not executed; review worktree has no node_modules or built dist prerequisites, …;"agent reverse-audit (round 4)":none — wait, per instructions I must not write a Budget gap line when nothing was cut short; no checks were curtailed, so no such line is emitted.。
收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 31 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下: Autofix round: no code changes needed — all blocking findings re-verified fixed at
|
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Verified the load-bearing surfaces against the head:
- Validation chain is defense-in-depth: route-level UUID gate on
:id(strict regex, before runtime resolution so the contract is config-independent), positive-integer/http(s)/2048/control-char checks on the PR body at route + bridge + sidecar-reader + SDK layers, and the renderers re-filter throughisExternalOpenUrlbefore any href. No URL consumer reaches shell, raw HTML, redirects, or server-side fetch. - Lifecycle is consistent: sidecar delete mirrors the worktree sidecar cleanup at both delete sites, archive/unarchive moves merge split pairs via
mergeSessionPrLists, and listing enrichment merges persisted history with live bindings after restart. Cross-workspace gating is unchanged (RESTmutate({strict})+ ACP connection-scoped ownership), and malformed/hand-edited sidecars fail soft everywhere. - Bounded-list semantics hold: re-bind refreshes and moves to latest, search matches older bindings, badge derives latest from the openable tail.
Two non-blocking observations worth a follow-up if you agree:
readSessionPrsenforces per-entry shape but not the 10-entry cap — the cap is write-side only. A hand-written sidecar with thousands of shape-valid entries would flow uncapped throughenrichPrSidecarsinto list responses. Truncating to the tail on read would make the read path enforce the same bound.- If the hydrate read hits a transient non-ENOENT I/O error,
seedSessionPrslater no-ops once the live entry has any prs, sosession_metadata_updatedevents carry only this daemon lifetime's bindings until restart even after I/O recovers (HTTP replies and the listing API self-heal). Merging instead of skipping in the seed would close that window.
CI is green on head (unit, web-shell E2E, review-pr, secret scan all pass).
ytahdn
left a comment
There was a problem hiding this comment.
Re-review — feat(web-shell): session PR binding (head f489d2c6)
Verdict: 💬 Comment — no new blocking issues. This is a re-review on top of my earlier (since-dismissed) approval at 29e3a1f2a. Everything that landed afterwards — the R5 fix commits — was read line-by-line and verified against the head tree. The PR is in an approvable state from my side.
Scope
Deep pass over the source delta since 29e3a1f2a (~500 lines: four-layer control-character rejection, seedSessionPrs hydration, traversal gate moved before runtime resolution, fallback write reordering, per-entry SDK filter on the primary client, tooltip keying/filtering, Enter-only badge keydown) plus its tests; a regression audit of all round-1..3 fixes at the current head; and an independent adjudication of the items still open from rounds 5/6.
🎉 What stands out
- All ten prior fixes are intact at this head. Spot witnesses: explicit by-number
prsmerge inmergeLiveSessionSummary(session-list.ts:499-511);displayNameecho on the pr-only event (bridge.ts:9781-9787); rejection-safe queue cleanupnext.then(cleanup, cleanup)(session-pr-service.ts:169-178); both REST routes bridge-first with the conflict branch throwing before any persist (session.ts:5333-5346,5488-5513);isValidSessionIdgates on both routes;${sessionId}.pr.jsonin the session-dir migration list (config.ts:5067-5075); split-pair merge inmovePrSidecar(sessionService.ts:1078-1090); sharedSESSION_PR_*constants; pr-gated ACP upsert (dispatch.ts:2959-2964). - The hydration design is concurrency-safe.
seedSessionPrsonly fires when the live entry holds no bindings (bridge.ts:9803-9809), so a seeding race can never overwrite a completed bind; the per-path upsert queue serializes the read-modify-write; hydrating from theactivepath is correct for route 1 and the ACP path (their sessions are necessarily live), and archive/restore moves the sidecar under an exclusive lock, so the file cannot migrate mid-handler. - The fallback write reordering (sidecar before rename) is the right call. The old window — rename persisted while the sidecar write failed, with the error response never announcing the rename — is gone; the remaining window (sidecar persisted, rename failed) heals on idempotent retry because the upsert is number-keyed. The failure-ordering rationale is inlined as a comment (
session.ts:5518-5522), which is exactly how this kind of invariant should be pinned. - Making
archiveStaterequired onenrichPrSidecarsturns a future wrong-chats-dir call site into a compile error instead of silently dropping every binding; both current call sites pass a state matching their own listing (session-list.ts:572,1243). - The Enter-only keydown fix is correct. Picker-dialog listbox navigation (arrows/Home/End/Space) bubbles again, while Enter still cannot double-trigger dialog confirmation — the listbox hook independently yields Enter/Space when focus is on an anchor (
SessionPrBadge.tsx:57-63).
Independently confirmed — already recorded in earlier rounds, not new findings
Both items appear in the round-5/6 deferred lists. I re-derived them independently on the current head and confirm they are real; I agree with the non-blocking treatment and record them here only for completeness.
1. 🟢 All-or-nothing sidecar validation + destructive rewrite — session-pr-service.ts:72-76, 108, 160. isValidSessionPrList uses every(), so one bad entry makes readSessionPrs return null, and the next upsertSessionPr rewrites the file from existing = [] — permanently wiping the legitimate history, not merely hiding it at read time. New twist introduced by this round: R5 added control-character rejection to isValidSessionPr, so a sidecar written by a pre-R5 build of this very branch (when all four layers accepted control characters in urls) becomes wholesale invalid after the update. Trigger is confined to dev-machine sidecars and hand edits (the feature is unreleased and the current daemon cannot write an invalid entry). Direction: filter per-entry at read instead of all-or-nothing.
2. 🟢 Raw-cast ingress bypasses the per-entry validation standard this PR itself established — DaemonClient.ts:2657 (session list page), 2922 (sessionStatus), 6129-6140 (WorkspaceDaemonClient.updateSessionMetadata) all cast as T with no per-entry gate, while the primary client's metadata path filters entry-by-entry with an explicit threat-model comment (5366-5371). The render layer guards url values (isExternalOpenUrl) but not entry shape: a hostile-or-buggy prs: [null] throws a TypeError in SessionPrBadge.tsx:33 / sessionSearch.ts:18 during render, taking down that workspace's sidebar list and the three picker dialogs. The daemon is a locally trusted component, so this is defense-in-depth; the cheapest fix is gating the three consumers with isDaemonSessionPrInfo.
Bottom line
No new issues; all prior fixes held; the R5 delta is sound. 💬 Comment — nothing blocks re-approval from my side.
中文
结论:💬 评论 — 无新阻塞问题。 这是在我上一轮(已被新推送搁置的)批准(29e3a1f2a)之上的复审。此后落地的 R5 修复提交已逐行读完并对照 head tree 验证。就我这一侧,PR 已处于可批准状态。
范围
精读 29e3a1f2a 之后的源码增量(约 500 行:四层控制字符拒绝、seedSessionPrs 回灌、traversal 门禁前移到运行时解析之前、fallback 写入重排、主 SDK 客户端逐条过滤、tooltip keying/过滤、badge onKeyDown 只吞 Enter)及配套测试;对第 1-3 轮全部修复在当前 head 做回归审计;独立复核第 5/6 轮遗留项。
🎉 值得肯定
- 此前 10 项修复在当前 head 全部完好。 抽查 witness:
mergeLiveSessionSummary按 number 显式合并prs(session-list.ts:499-511);pr-only 事件回显displayName(bridge.ts:9781-9787);队列清理next.then(cleanup, cleanup)不泄漏 rejection(session-pr-service.ts:169-178);两条 REST 路由 bridge-first 且 conflict 分支在任何持久化之前抛出(session.ts:5333-5346、5488-5513);两条路由入口isValidSessionId门禁;会话目录迁移清单含${sessionId}.pr.json(config.ts:5067-5075);movePrSidecar分裂 pair 合并(sessionService.ts:1078-1090);共用SESSION_PR_*常量;ACP upsert 按入参pr门控(dispatch.ts:2959-2964)。 - 回灌(hydration)设计并发安全。
seedSessionPrs仅在 live entry 无绑定时生效(bridge.ts:9803-9809),seed 竞态不可能覆盖已完成的绑定;upsert 按路径队列串行读-改-写;route 1 与 ACP 路径的会话必然 live,读active路径正确;归档/恢复在 exclusive 锁下连同 sidecar 一起迁移,handler 期间文件不会被搬走。 - fallback 写入重排(sidecar 先于 rename)方向正确。 旧窗口——rename 已落盘而 sidecar 写失败、错误响应只字不提改名——已消除;剩余窗口(sidecar 已写、rename 失败)可被幂等重试治愈(upsert 按 number 去重)。失败序推理已内联为注释(
session.ts:5518-5522),这类不变量就该这样钉住。 enrichPrSidecars的archiveState改为必填,把未来"从错误 chats 目录富集"的调用点错误变成编译错误;现有两处调用传值均与各自列表归档态一致(session-list.ts:572、1243)。- onKeyDown 只吞 Enter 的修复正确。 picker 对话框的 listbox 键盘导航(方向键/Home/End/Space)恢复冒泡,Enter 也不会重复触发对话框确认——listbox hook 在焦点位于 anchor 时本就会让出 Enter/Space(
SessionPrBadge.tsx:57-63)。
独立确认 —— 早前轮次已记录,非新 finding
以下两条均已出现在第 5/6 轮 deferred 列表中。我在当前 head 独立重新推导出同样结论,确认属实;认同不阻塞处理,此处仅为完整性记录。
1. 🟢 sidecar 校验 all-or-nothing + 破坏性重写 —— session-pr-service.ts:72-76、108、160。isValidSessionPrList 用 every(),一条坏数据使 readSessionPrs 返回 null,下一次 upsertSessionPr 以 existing = [] 重写文件——合法历史被永久抹掉,而非仅读时隐藏。本轮新增的转折:R5 给 isValidSessionPr 加了控制字符拒绝,因此本分支 R5 之前构建(当时四层都接受含控制字符的 url)写出的 sidecar 升级后整体变为非法。触发面仅限开发机 sidecar 与手工篡改(功能未发布,当前 daemon 写不出非法条目)。方向:读时逐条过滤替代整表判定。
2. 🟢 raw-cast 入口绕过了本 PR 自己建立的逐条校验标准 —— DaemonClient.ts:2657(会话列表分页)、2922(sessionStatus)、6129-6140(WorkspaceDaemonClient.updateSessionMetadata)均为 as T 裸 cast,无逐条门禁;而主客户端 metadata 路径逐条过滤且注释写明威胁模型(5366-5371)。渲染层只兜底 url 值(isExternalOpenUrl),不兜底条目形状:恶意/故障 daemon 返回 prs: [null] 会在 SessionPrBadge.tsx:33 / sessionSearch.ts:18 的 render 阶段抛 TypeError,击垮该工作区的侧栏列表与三个选择对话框。daemon 是本地信任组件,此条属纵深防御;最省的修法是在三个消费点用 isDaemonSessionPrInfo 门禁。
小结
无新问题;既有修复全部保持;R5 增量健全。💬 评论 —— 就我这一侧没有任何阻塞项。
Verification run (fixes + #9370's workflow) cut the Windows failures from 69 to 5 and left the macOS lane's infra error. Follow-ups: - daemon-git-worktree-guard resolvePhysicalPath: splitting an absolute Windows path yields the drive as a segment (C:), which path.join glued back onto the root as C:\C:. Walk only the part past the root. Exposed by the first round's tokenizer fix, which let intact drive paths reach this code for the first time. - scratch-tree tests: the git-created gitfile refuses in-place overwrite on Windows even after clearing the read-only attribute; delete and recreate instead. - bridge.test stderr audit assertion: the line prints the session id through JSON.stringify, escaping Windows backslashes; match the escaped spelling (test added this morning by #9543, landed after the baseline census). - managed-scratch 'root replaced' test: dev/ino identity is not reliably observable on every Windows volume; state the precondition and skip where the swap is indistinguishable. - scripts vitest suite: the unhandled onTaskUpdate worker RPC timeout is deterministic on the macOS runners with every test green; stop letting unhandled errors fail this suite while test failures stay fatal, and drop the stale claim that the pool override removal fixed it.
Verification run (fixes + #9370's workflow) cut the Windows failures from 69 to 5 and left the macOS lane's infra error. Follow-ups: - daemon-git-worktree-guard resolvePhysicalPath: splitting an absolute Windows path yields the drive as a segment (C:), which path.join glued back onto the root as C:\C:. Walk only the part past the root. Exposed by the first round's tokenizer fix, which let intact drive paths reach this code for the first time. - scratch-tree tests: the git-created gitfile refuses in-place overwrite on Windows even after clearing the read-only attribute; delete and recreate instead. - bridge.test stderr audit assertion: the line prints the session id through JSON.stringify, escaping Windows backslashes; match the escaped spelling (test added this morning by #9543, landed after the baseline census). - managed-scratch 'root replaced' test: dev/ino identity is not reliably observable on every Windows volume; state the precondition and skip where the swap is indistinguishable. - scripts vitest suite: the unhandled onTaskUpdate worker RPC timeout is deterministic on the macOS runners with every test green; stop letting unhandled errors fail this suite while test failures stay fatal, and drop the stale claim that the pool override removal fixed it.
Verification run (fixes + #9370's workflow) cut the Windows failures from 69 to 5 and left the macOS lane's infra error. Follow-ups: - daemon-git-worktree-guard resolvePhysicalPath: splitting an absolute Windows path yields the drive as a segment (C:), which path.join glued back onto the root as C:\C:. Walk only the part past the root. Exposed by the first round's tokenizer fix, which let intact drive paths reach this code for the first time. - scratch-tree tests: the git-created gitfile refuses in-place overwrite on Windows even after clearing the read-only attribute; delete and recreate instead. - bridge.test stderr audit assertion: the line prints the session id through JSON.stringify, escaping Windows backslashes; match the escaped spelling (test added this morning by #9543, landed after the baseline census). - managed-scratch 'root replaced' test: dev/ino identity is not reliably observable on every Windows volume; state the precondition and skip where the swap is indistinguishable. - scripts vitest suite: the unhandled onTaskUpdate worker RPC timeout is deterministic on the macOS runners with every test green; stop letting unhandled errors fail this suite while test failures stay fatal, and drop the stale claim that the pool override removal fixed it.
|
Released in v0.22.0. |
* fix: repair the Windows and macOS test lane failures The platform lanes have been dark since 2026-07-02 (gated on a merge queue that is not enabled); reviving them in QwenLM#9370 exposed these pre-existing failures. 72 failing tests across 16 files, all traced to platform assumptions: Product fixes (2): - daemon-git-worktree-guard: on Windows a backslash is a path separator, not a POSIX escape. The shell-quote tokenizer consumed `\x` pairs, mangling `C:\repo\sub` into a relative word — false denials for legitimate commands AND undetected relocations for backslash-relative ones. Preserve unquoted backslashes before tokenisation on win32. - acpAgent isOwnerOnlyDirectory: hard-returning false on win32 disabled Live managed relocation entirely (Node exposes no ownership bits there). Rest on the structural checks — symlink rejection and dev/ino identity across the realpath round trip — the same trade-off serve/live/discovery.ts already makes. Test-fixture fixes (the product code was already Windows-correct): - server.test Live catalog roots use the host-native path shape (path.resolve equality proof fails for POSIX literals on win32) - review cleanup suite pins POSIX node:path semantics for its literal-keyed mocks; fetch-pr resume budget uses a native tmpdir; scratch-tree clears the DOS read-only attribute before overwriting a git-created gitfile; worktree-list assertions compare slash-normalized (git prints forward slashes on Windows) - mode-bit (0600) assertions skip on win32 (no POSIX permission bits; every read side already skips its mode check there) - O_NOFOLLOW symlink test and the unescapePath no-op test skip on win32; sidecar errno injection uses a portable NUL byte; Footer exact-hint text is platform-conditional (win32 indicator is 8 columns shorter, shifting the flex shrink by one) scripts vitest suite: drop the fixed 8-16 worker floor that oversubscribes the 3-core macOS runners — the main thread stalled past the 60s worker RPC timeout (onTaskUpdate), exiting 1 with every test green. * fix: second round of platform lane repairs from CI verification Verification run (fixes + QwenLM#9370's workflow) cut the Windows failures from 69 to 5 and left the macOS lane's infra error. Follow-ups: - daemon-git-worktree-guard resolvePhysicalPath: splitting an absolute Windows path yields the drive as a segment (C:), which path.join glued back onto the root as C:\C:. Walk only the part past the root. Exposed by the first round's tokenizer fix, which let intact drive paths reach this code for the first time. - scratch-tree tests: the git-created gitfile refuses in-place overwrite on Windows even after clearing the read-only attribute; delete and recreate instead. - bridge.test stderr audit assertion: the line prints the session id through JSON.stringify, escaping Windows backslashes; match the escaped spelling (test added this morning by QwenLM#9543, landed after the baseline census). - managed-scratch 'root replaced' test: dev/ino identity is not reliably observable on every Windows volume; state the precondition and skip where the swap is indistinguishable. - scripts vitest suite: the unhandled onTaskUpdate worker RPC timeout is deterministic on the macOS runners with every test green; stop letting unhandled errors fail this suite while test failures stay fatal, and drop the stale claim that the pool override removal fixed it. * fix(ci): skip coverage report generation on non-Linux CI CI consumes coverage only from the ubuntu lane: the artifact upload and the coverage comment both pin coverage-reports-*-ubuntu-latest. On the Windows runners the v8 report generation for 800+ files stalls the vitest main thread past the 60s worker RPC budget at the end of an all-green cli run, exiting the lane 1 (observed in verification run 32569004418). Skip coverage on non-Linux CI; local runs keep it. * fix(ci): stop all-green cli/core runs exiting red on RPC timeout The Windows lane's third verification round repeated the failure with coverage already disabled: 866 cli test files green, then the worker onTaskUpdate RPC budget (60s, hardcoded in vitest's bundled birpc) expired under runner resource pressure and the unhandled error exited the lane 1. Extend the scripts suite's treatment to the two big package suites: test failures stay fatal, unhandled errors do not. * fix: address review on the win32 guard pre-pass and lane configs R1-1 (Critical): the win32 pre-pass escaped the character after every unquoted backslash, so whitespace after a trailing separator glued the next word into the -C value — a second -C/--git-dir/-c parked there vanished from the analysis while cmd.exe still split the argv at the whitespace, allowing a destructive mutation outside the boundary. The tokenizer treats `\<space>` as an escaped space even after an even number of backslashes, so escaping forward can never express "literal backslash, then word boundary". Escape the backslash alone instead: a double-quoted backslash before whitespace and cmd boundary characters (; | & < > ( )) keeps them their separator role, and a plain escaped backslash elsewhere. Verified token boundaries for the attack shape, its tab variant, trailing-separator-before-flag, UNC, chained -C, and quoted paths; added win32-only guard tests for the boundary shapes. R1-2/3/4: gate dangerouslyIgnoreUnhandledErrors to non-Linux — the ubuntu lane and Linux local runs keep the unhandled-error signal. R1-5: pin the deterministic win32 footer truncation ('queu') instead of skipping the content assertion there. R1-6: build the Live conversations fixture root with the file's documented path.resolve(path.sep, ...) convention instead of a hardcoded C: literal. * fix: three Windows lane failures from recent main commits Verification of the revived lanes surfaced three failures introduced by commits that landed while the lanes were dark: - isSameFile compared dev/ino unconditionally; on volumes that report ino 0 (or a colliding value) for every file it equated distinct files. Treat an unverifiable inode like core's hasVerifiableInode convention and fall back to canonical spellings — losing hard-link identity there, but never equating distinct files. The hard-link test skips where the volume exposes no inode. - drive's bound-address recipe test rmSync'd its temp dir while the backgrounded service still held it (EBUSY on Windows); shorten the service's self-exit timer and retry the removal. - the FileReadCache seeding test collided under one dev:ino key when the volume reports the same inode for both MEMORY.md indexes; skip where inode identity is not real. * fix: address round-2 review on identity fail-closed checks and the win32 guard pre-pass * fix(cli): deny cmd.exe rewrite syntax in the daemon git-worktree guard (QwenLM#9728) * fix: address round-4 review on the cmd-rewrite denial reason and cmd-lane test gating Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix: address round-5 review by closing the divergent Windows shell surface structurally The win32 cmd/PowerShell lanes were analysed through a POSIX text model plus per-stage patches; each round closed one entrance of the divergence family and the next found new ones. Fail closed on syntax whose lane semantics diverge from the model (lone `&`, `( )`, cmd `#`/`;`/single quotes, /s outer-quote strip, PowerShell `--%`/`''` doubling), normalize the whole command text once before any stage reads it, drop the bash shadow model on lanes where the syntax defines nothing, and stop scoping PowerShell pipeline stages as subshells. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix: address round-6 review by gating bash-semantics tests off the win32 lane and failing closed on nested Windows shells R6-1: the whole-text divergent-syntax gate denied ~20 ungated bash-semantics expectations on the real win32/cmd merge lane (41 failures reproduced under a lane-spoof harness). Gate those blocks off the win32 non-bash lanes, splitting mixed blocks so lane-safe pins keep running there, and commit the lane-spoof harness so the whole guard suite runs as the win32/cmd merge lane on every lane. R5-1 (partial): cmd/powershell/pwsh invocations now fail closed on the undecidable-payload denial on every lane — their payloads are parsed by a grammar the POSIX text model cannot read (closes the -EncodedCommand and nested cmd /c entrances probed at this head). The remaining class closure is escalated to the maintainer as a product/scope decision. * fix: address round-7 review by gating the Windows-shell fail-closed rule to the win32 platform R7-1: the round-6 WINDOWS_SHELL_PROGRAMS branch failed closed on every lane, which denied benign cross-platform PowerShell on POSIX daemons (pwsh -NoProfile -Command Write-Output hello reproduced denied on the unresolved reason at this head). Gate the rule to platform win32 — cmd.exe and Windows PowerShell only exist there, and they stay reachable from win32 Git Bash sessions, so the gate is platform-wide rather than windowsNative; on POSIX the same names keep the base stance of ordinary unmodelled programs. Move the fail-closed pins onto spoofed win32 lanes, add a win32 Git Bash lane pin so the entrance stays closed there, and add the POSIX benign-pwsh regression test. * fix(cli): close the cmd.exe state-persisting entrances in the daemon guard Probe-verified bypasses (review R5-1): cmd.exe builtins persist state into every later &&-chained command, and the analysis modelled none of them — `set GIT_WORK_TREE=<outside>&& git reset --hard` and `chdir <outside> && git reset --hard` both analysed cwd-local while the executed command relocated outside the boundary. cmd's state-persisting builtins are a closed set, so this enumerates them instead of chasing individual shapes: - `set VAR=value` / `setx VAR value` carry the semantics of a POSIX `export VAR=value` for every later segment; route them through that machinery (GIT_* keys become relocations, unresolvable keys fail closed). Non-assignment forms (`set /p`, dynamic operands) fail closed. - `chdir` joins the cd family (cmd's synonym), `/D` consumed as the drive-switch option. - `path` and `doskey` rewrite which executable a bare name resolves to — unresolvable, failed closed. - copy/mklink/move/robocopy/xcopy join the relinking programs on win32, and the text relocation markers learn `chdir`. PowerShell-only entrances (function definitions, New-Item function:, @-splatting) already fail closed through the unmodelled-syntax gate; the new branches stay gated off Git-Bash sessions, where these words are ordinary POSIX commands. Pinned by new tests in the win32-lane harness, which spoofs win32/cmd on every platform. * Revert "fix(cli): close the cmd.exe state-persisting entrances in the daemon guard" This reverts commit a8f137a. * Reapply "fix(cli): close the cmd.exe state-persisting entrances in the daemon guard" This reverts commit b888a42. * fix: address round-10 review by closing the Windows-lane guard entrances and the ino-0 case fold * fix(cli): repair the round-10 build rejection by mapping chdir variants exhaustively * fix(cli): make the /MIR relink-switch denial assertion lane-independent The assertion spelled the unresolvable target as POSIX '/MIR', but the win32 lane resolves it through path.win32/realpathNearestExistingAsync into a backslash spelling ('\\MIR'/'C:\\MIR'), so the new test failed only on the merge_group-gated test_windows lane. Match either separator spelling. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>





What this PR does
When a pull request is created from the Web Shell Git dialog, the PR number and URL are now bound to the session that created it. A session can produce several PRs (stacked or follow-up work), so the binding is a bounded list (10 entries, oldest dropped) ordered by binding time: re-binding the same PR number refreshes it and moves it to latest, and every bound PR stays searchable.
The daemon accepts a binding through the session metadata routes (both REST and the ACP dispatch path), one PR per call, validates it at every layer — route, bridge, SDK, and the persisted sidecar — with the URL restricted to http(s) because it is rendered as a link target, and persists the full list as a per-session sidecar file so the bindings survive daemon restarts and follow the session through archive, unarchive, and deletion. Reads, SSE events, and route responses all carry the complete list; the sidecar (complete history) is merged with the live entry's bindings (this daemon lifetime) when listing.
In the sidebar, a session with bound PRs shows a small badge next to its title with the latest PR number —
#9517, or#9517 +2when more are bound — that opens the PR (routed through the desktop-aware external-link opener), the session details tooltip lists every bound PR (latest first), and the session search box matches any bound PR number (with or without#), branch name, and worktree slug in addition to the title and session id. The same badge appears in the mission-control session overview panel and in the shared session picker row used by the resume / delete / release dialogs, and the resume dialog's search matches the same git context.Design doc:
docs/design/2026-08-20-webshell-session-pr-binding.md.Why it's needed
Heavy Web Shell users run dozens of concurrent sessions. When a CI failure or review comment references PR #N, there was no way to answer "which session produced this PR" — the dialog discarded the created PR's coordinates after showing a status line, and the sidebar search only matched titles and session ids. After this change, typing the PR number into the sidebar search filters down to the exact session — including earlier PRs of a session that produced several.
Reviewer Test Plan
How to verify
#Nbadge next to the session title; clicking it opens the PR. Hovering the row lists the PR in the details tooltip.#N +1, the tooltip lists both PRs (latest first).9517or#9517) into the sidebar search box — only the bound session remains, for older bindings too. Searching by branch name or worktree slug also matches. The mission-control overview panel (sessions tab) shows the same badge on each session card, and/resume's picker shows it on each row and matches PR numbers in its own search field.PATCH /workspaces/:w/session/:id/metadatawith{"pr":{"number":9517,"url":"https://github.com/o/r/pull/9517"}}returns 200 with the fullprslist echoed; repeated calls accumulate bindings; malformed payloads (non-integer number, missing URL,javascript:URL, empty body) return 400invalid_metadata; the SSE stream emitssession_metadata_updatedcarrying the fullprslist.Evidence (Before & After)
Before: the sidebar showed only title, relative time, and a git icon; nothing referenced the created PR anywhere.
After — badge with multi-PR overflow, and search hitting an older binding:
The session overview (mission control) shows the same badge on each card:
Single-binding view and latest-number search:
Tested on
Environment (optional)
Local daemon from the built bundle (
dist/cli.js serve) with an isolated HOME, plus the mock-daemon Playwright visuals harness for the browser UI.Risk & Scope
pr-only body (previouslydisplayNamewas mandatory on the workspace-scoped route); an empty body is still rejected with the sameinvalid_metadatacode. The SDK browser-bundle size budget was raised to 200KB (commented in the build script): main's persistent-session-attachment feature took 198→199KB and this PR's new type and validators need the remaining headroom.gh pr createin a shell are not auto-discovered — only the Git dialog flow binds. No server-side search filtering (client-side matching is sufficient at current session counts). Windows/Linux not tested locally (covered by CI).Linked Issues
N/A
中文说明
这个 PR 做了什么
从 Web Shell 的 Git 对话框创建 PR 后,PR 号和 URL 现在会绑定到创建它的会话上。一个会话可能创建多个 PR(stacked PR、连续修复),因此绑定是一个有上限的列表(10 条,超出丢弃最旧),按绑定时间排序:同号重复绑定会刷新并移到最新位,所有绑定过的 PR 都可搜索。
daemon 通过会话元数据路由(REST 与 ACP dispatch 两条路径)接受绑定,每次调用绑定一个 PR,在每一层——路由、bridge、SDK、持久化 sidecar——都做校验;URL 强制为 http(s),因为它会作为链接目标被渲染。完整列表持久化为 per-session 的 sidecar 文件,daemon 重启后不丢失,且在会话归档、取消归档、删除时随之迁移/清理。读取、SSE 事件、路由响应都携带完整列表;列表接口会把 sidecar(完整历史)与 live entry(本 daemon 生命周期内的绑定)按 PR 号合并。
侧栏中,绑定了 PR 的会话在标题旁显示一个小号 badge,展示最新 PR 号——只绑一个时是
#9517,绑了多个时是#9517 +2——点击打开该 PR(走 desktop 感知的外链 opener);会话详情 tooltip 列出全部绑定 PR(最新在前);侧栏搜索框在标题和 sessionId 之外,现在还匹配任意一个绑定的 PR 号(带不带#均可)、分支名和 worktree slug。同样的 badge 也出现在 Mission Control 会话总览面板和 resume / delete / release 三个会话选择对话框的行上,resume 对话框的搜索同样匹配 PR 号。设计文档:
docs/design/2026-08-20-webshell-session-pr-binding.md。为什么需要
重度 Web Shell 用户会同时运行几十个会话。当 CI 失败或 review 评论提到 PR #N 时,无法回答"哪个会话产生了这个 PR"——对话框创建 PR 后只显示一条状态消息就丢弃了 PR 信息,侧栏搜索也只匹配标题和 sessionId。改动后,在侧栏搜索框输入 PR 号即可过滤出对应会话——包括一个会话较早创建的 PR。
Reviewer 测试计划
如何验证
#Nbadge;点击打开该 PR。hover 会话行时详情 tooltip 中列出该 PR。#N +1,tooltip 列出两个 PR(最新在前)。9517或#9517)——只剩绑定的会话,较早的绑定同样命中。按分支名或 worktree slug 搜索也命中。PATCH /workspaces/:w/session/:id/metadata携带{"pr":{"number":9517,"url":"https://github.com/o/r/pull/9517"}}返回 200 并回显完整prs列表;重复调用会累积绑定;非法载荷(非整数 number、缺 URL、javascript:URL、空 body)返回 400invalid_metadata;SSE 流发出携带完整prs列表的session_metadata_updated事件。证据(Before & After)
Before:侧栏只显示标题、相对时间和 git 图标;创建后的 PR 在任何地方都没有记录。
After——多 PR badge(带 +N)与按较早 PR 号搜索(截图见上方英文区)。
测试平台
macOS 已验证;Windows/Linux 未本地验证(由 CI 覆盖)。
风险与范围
pr的 body(此前 workspace 作用域路由强制要求displayName);空 body 仍以相同的invalid_metadata错误码拒绝。SDK 浏览器 bundle 体积预算上调至 200KB(构建脚本中已注释):main 的 persistent-session-attachment 功能占了 198→199KB,本 PR 的新增类型与校验器需要剩余空间。gh pr create的 PR 不做自动发现——只有 Git 对话框流程会绑定。无服务端搜索过滤(当前会话规模下客户端匹配足够)。Windows/Linux 未本地测试(由 CI 覆盖)。