feat(serve): backfill session PR bindings and refresh their merge state - #9729
feat(serve): backfill session PR bindings and refresh their merge state#9729wenshao wants to merge 24 commits into
Conversation
Legacy sessions predate the PR-binding feature, so the sidebar had no way to answer 'which session produced PR N'. An on-demand route scans every trusted workspace's persisted sessions, resolves PR numbers from the worktree slug/branch convention and from transcript gitBranch x gh headRefName intersections (the dominant source in practice), and writes the existing .pr.json sidecars. Bound PRs now carry a state snapshot (open/merged/closed) that a 5-minute daemon sweep advances via a slim gh pr list --state all query, and the sidebar badge dims merged PRs while the tooltip names merged/closed ones.
|
Thanks for the follow-up to the session↔PR binding work! Template looks good ✓ Problem: observed, not theoretical. The description carries real operator data — 6,454 persisted sessions across 25 workspaces with zero bindings before backfill, and bound badges keeping their open accent long after the PR merged. It extends #9543, which landed on main this morning. Direction: aligned. This completes the "which session produced PR N, and is it still open?" story for serve-mode operators. No direct reference in the comparison CHANGELOG, but the area is a continuation of a feature that just merged. Size: the numbers GitHub shows for this PR (58 files, +5516/−229) are misleading — the branch diverged from main before #9543 was merged, so the displayed diff re-includes #9543's already-merged content. The true delta on top of merged #9543 is ~25 files, +1707/−48 (≈790 production lines, ≈860 test lines, the rest docs). It does touch core paths ( Approach: sound and reuse-first. The backfill extends the existing Risk: no matches against the revert-history high-risk paths.
Moving on to code review. 🔍 中文说明感谢这个会话↔PR 绑定功能的后续 PR！ 模板完整 ✓ 问题：已观测到的真实问题，不是理论性的。描述里有真实的运营数据——回填前 25 个 workspace 的 6454 个持久化会话零绑定，且已绑定的 badge 在 PR 合入后长期保持 open 高亮。这是今早刚合入 main 的 #9543 的延续。 方向：对齐。补齐了 serve 模式操作者"哪个会话产出了 PR N，它是否还开着"的闭环。对比 CHANGELOG 无直接条目，但该方向是刚合入功能的自然延续。 规模：GitHub 上显示的规模（58 文件，+5516/−229）有误导性——分支在 #9543 合入之前从 main 分出，因此展示的 diff 重复包含了 #9543 已合入的内容。相对已合入 #9543 的真实增量约 25 个文件、+1707/−48（约 790 行生产代码、约 860 行测试，其余为文档）。确实触及核心路径（ 方案：合理且优先复用。回填通过 slim 字段集扩展了现有 风险：未命中 revert 历史高风险路径。
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed the true delta of this PR — One real finding — the merge-state refresh does not propagate to an open sidebar:
Everything else I checked reads clean:
The state-refresh flow, including where propagation currently stalls: sequenceDiagram
participant P1 as Backfill route (on demand)
participant P2 as Refresh timer (default 5 min)
participant P3 as gh pr list (slim, state all)
participant P4 as Session PR sidecars
participant P5 as Web Shell sidebar
P1->>P3: one batched query per workspace
P3-->>P1: number, url, headRefName, state
P1->>P4: upsert bindings with state
P2->>P4: read non-merged bindings
P2->>P3: only when targets exist
P3-->>P2: current states
P2->>P4: rewrite state in place
P5->>P4: full refetch only on catalog version change
Note over P4,P5: sweep writes do not bump the catalog version
Files changed — true delta (14 production files of 25 shown; tests omitted)
TestingThis is an unattended CI run: no PR code was built or executed here. Test evidence below is the PR's own CI, read through the API — and at review time the build/test lanes have not started for this head SHA. Only the bot orchestration checks exist (precheck and labeler completed green; triage and the review job still in flight). No
The finalize workflow rewrites the table above in place once CI settles. Not verified: the author's live-daemon numbers (575 bindings written, 272-of-342 transcript-branch hits, 99/100 list rows carrying 中文说明代码审查按本 PR 的真实增量审查（ 一个真实发现——合入状态刷新不会传播到已打开的侧栏：
其余检查均干净：
测试这是无人值守 CI 运行：此处未构建或执行任何 PR 代码。下方测试证据来自 PR 自身 CI 的 API 读取——审查时刻构建/测试通道尚未启动。只有机器人编排类检查（precheck 与打标签已绿；triage 与审查任务进行中）。 未验证：作者的真实 daemon 数字（写入 575 条绑定、342 个会话中 272 个经 transcript 分支命中、列表首页 100 行中 99 行带 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the implementation itself is solid and reviews clean apart from one fixable propagation gap; the score reflects merge-readiness, because the branch as it stands cannot merge. Stepping back: the motivation is real (operator data, not a hypothetical), the design doc was updated alongside the code, and the true delta (~790 production lines over merged #9543) is tight, reuse-first, and carries roughly a 1:1 test ratio. If this had been rebased onto current main, it would have been an easy approve with one Suggestion — the sweep not bumping the catalog revision, so merged badges don't dim in an open sidebar until something else triggers a refetch. What blocks it is mechanical but hard: the branch diverged before #9543 landed, GitHub reports it CONFLICTING, and the displayed diff re-includes #9543's already-merged content — a reviewer reading the GitHub diff would be reviewing 58 files and 5,700 lines when the actual contribution is 25 files and ~1,750. On top of that, no 中文说明信心：2/5 —— 实现本身扎实，除一个可修复的传播缺口外审查干净；这个分数反映的是合入就绪度，因为当前分支无法合入。 整体看：动机真实（运营数据，不是假设），设计文档随代码同步更新，相对已合入 #9543 的真实增量（约 790 行生产代码）紧凑、优先复用，测试比例接近 1:1。如果它已 rebase 到最新 main，本可以轻松通过，只带一条建议——定时刷新没有 bump catalog revision，导致已打开侧栏中的 merged badge 要等到别的变更触发重新拉取才会弱化。 阻塞项是机械性但硬性的：分支在 #9543 合入前分出，GitHub 报告冲突，且展示的 diff 重复包含了 #9543 已合入的内容——照 GitHub diff 审查的人会在 58 个文件、5700 行里打转，而真实贡献只有 25 个文件、约 1750 行。此外此 head SHA 上没有任何 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs a rebase onto current main before it can merge: the branch diverged before #9543 landed, so it conflicts with main and the displayed diff re-includes #9543's already-merged content (true contribution is ~25 files, not 58). The true delta reviews cleanly — one Suggestion on badge refresh propagation (the sweep should bump the bridge catalog revision), details in my notes above. @wenshao once rebased, re-run @qwen-code /triage and I'll review the new head. 🙏
8f09d60 to
0c0dbd2
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
🖼️ 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 ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
run-qwen-serve is a pre-listen bundle root whose static closure must stay free of the SessionService chain (glob et al.). Loading session-pr-refresh statically pulled that chain in; a dynamic import() of the core barrel from inside the refresh module was worse — it made the barrel's full namespace live and poisoned the shared chunk for every static barrel importer (ACP agent included). Load the whole refresh module through a dynamic import at timer start instead, guarded by a generation counter against dispose races.
process-env-guard scans serve sources for process.env reads; register QWEN_SESSION_PR_REFRESH_MINUTES as a documented process-scoped switch. GitDialog now sends state 'open' with the binding, so the dialog tests assert it.
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/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，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
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.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (minutes === 0) return undefined; | ||
| return minutes * 60_000; |
There was a problem hiding this comment.
[Critical] R1-17: An interval whose converted milliseconds exceed Node's 32-bit timer max (2,147,483,647 ms — anything above ~24.8 days) passes the finite/non-negative gates on minutes, but setInterval clamps out-of-range delays to 1 ms — converting "longer interval" into a continuous sweep hot loop. QWEN_SESSION_PR_REFRESH_MINUTES=1e308 reaches the same clamp another way: minutes is finite, but minutes * 60_000 overflows, and the gate checks minutes, not the converted value.
Trigger: an operator sets QWEN_SESSION_PR_REFRESH_MINUTES=43200 (a plausible "monthly" refresh). The daemon then runs full sweeps back-to-back — paginated listSessions over active+archived, a sidecar read per bound session, one gh pr list spawn per workspace with non-merged bindings — burning CPU/IO and GitHub rate limits until restart, while the operator believes the sweep is monthly. The running flag serializes ticks but does not space them.
Witness (probe on unmodified code): resolveSessionPrRefreshIntervalMs({'43200'}) → 2592000000; a real setInterval at that value → TimeoutOverflowWarning: 2592000000 does not fit into a 32-bit signed integer. Timeout duration was set to 1. and 93 ticks in 100 ms; '1e308' behaves the same. With the validation below patched in: both inputs → 300000 ms, 0 ticks, no warnings.
| if (minutes === 0) return undefined; | |
| return minutes * 60_000; | |
| if (minutes === 0) return undefined; | |
| const ms = minutes * 60_000; | |
| return Number.isFinite(ms) && ms <= 2 ** 31 - 1 ? ms : DEFAULT_SESSION_PR_REFRESH_INTERVAL_MS; |
中文说明
[Critical] R1-17：当换算后的毫秒数超过 Node 32 位定时器上限（2,147,483,647 ms，约 24.8 天以上）时，minutes 本身仍能通过有限/非负校验，但 setInterval 会把超限延时钳制为 1 ms——"更长的间隔"反而变成持续的扫描热循环。QWEN_SESSION_PR_REFRESH_MINUTES=1e308 以另一种方式触发同样的钳制：minutes 有限但 minutes * 60_000 溢出，而校验只检查 minutes 本身。
触发场景：运维设置 QWEN_SESSION_PR_REFRESH_MINUTES=43200（"每月一次"）。daemon 会背靠背地连续执行完整扫描——对 active+archived 分页 listSessions、逐个读取绑定 sidecar、对每个仍含未合入绑定的 workspace 拉起一次 gh pr list——持续消耗 CPU/IO 与 GitHub 速率配额直到重启，而运维以为扫描是每月一次。running 标志只能串行化 tick，无法拉开间隔。
证据（对未修改代码的探针）：resolveSessionPrRefreshIntervalMs({'43200'}) → 2592000000；以该值实际调用 setInterval → TimeoutOverflowWarning: 2592000000 does not fit into a 32-bit signed integer. Timeout duration was set to 1.，100 ms 内触发 93 次；'1e308' 行为相同。打入下方校验后：两种输入均 → 300000 ms，0 次触发，无警告。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const slugMatch = SLUG_PR_PATTERN.exec(slug ?? ''); | ||
| if (slugMatch) return Number(slugMatch[1]); |
There was a problem hiding this comment.
[Critical] R1-25: The slug-convention parser has no positivity gate — parsePrNumberFromWorktree('pr-0') returns Number('0') = 0 (SLUG_PR_PATTERN matches, and there is no n > 0 check, unlike the canonical GitWorktreeService.parsePRReference). A worktree named pr-0 is a legal user slug (validateUserWorktreeSlug accepts it). Backfill then persists { number: 0, url } — but isValidSessionPr requires number > 0, so one zero entry invalidates the whole sidecar: readSessionPrs returns null, every binding for that session vanishes from listings/tooltip, and the next upsertSessionPr (treating null as []) rewrites the file, permanently dropping the prior bindings. pr-00/pr-000 are the same hole.
Trigger: an operator or agent creates a worktree named pr-0, a session runs in it, and backfill binds number 0 → the session's existing bindings disappear and are eventually dropped for good.
Witness (probe against real code): validateUserWorktreeSlug('pr-0') → accepted; backfill with a seeded existing binding #42 → {bound: 1}, readAfterBackfill: NULL (whole file rejected), next upsert → [43] — binding 42 permanently dropped. With an n > 0 gate patched in: bound: 0, existing binding intact.
| const slugMatch = SLUG_PR_PATTERN.exec(slug ?? ''); | |
| if (slugMatch) return Number(slugMatch[1]); | |
| const slugMatch = SLUG_PR_PATTERN.exec(slug ?? ''); | |
| if (slugMatch) { | |
| const n = Number(slugMatch[1]); | |
| return n > 0 ? n : undefined; | |
| } |
中文说明
[Critical] R1-25：slug 约定解析缺少正数校验——parsePrNumberFromWorktree('pr-0') 返回 Number('0') = 0（SLUG_PR_PATTERN 可以匹配，且没有 n > 0 检查，而规范的 GitWorktreeService.parsePRReference 有）。名为 pr-0 的 worktree 是合法的用户 slug（validateUserWorktreeSlug 接受它）。回填随后会持久化 { number: 0, url }——但 isValidSessionPr 要求 number > 0，于是一条 0 值条目使整个 sidecar 失效：readSessionPrs 返回 null，该会话的所有绑定从列表/tooltip 中消失，下一次 upsertSessionPr（把 null 当作 []）重写文件，永久丢弃之前的绑定。pr-00/pr-000 是同样的漏洞。
触发场景：运维或 agent 创建名为 pr-0 的 worktree，会话在其中运行，回填绑定了数字 0 → 该会话已有的绑定消失并最终被永久丢弃。
证据（对真实代码的探针）：validateUserWorktreeSlug('pr-0') → 接受；在已有绑定 #42 的会话上回填 → {bound: 1}、readAfterBackfill: NULL（整个文件被拒绝）、下一次 upsert → [43]——绑定 42 被永久丢弃。打入 n > 0 校验后：bound: 0，已有绑定完好。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ...livePrs.map((l) => { | ||
| const persisted = persistedByNumber.get(l.number); | ||
| return persisted?.state !== undefined && persisted.state !== l.state |
There was a problem hiding this comment.
[Critical] R1-27: Live-only sessions (no storage record yet) bypass both this sidecar-wins state merge and the refresh sweep, so their PR state is frozen at bind time. The metadata-bind handler writes the sidecar via upsertSessionPr without flushing a session record; the sweep enumerates via storage-backed sessionService.listSessions (SESSION_FILE_PATTERN matches only .jsonl transcripts, never a bare .pr.json); and the live-only insertion branches on all three listing paths insert {...live} without reading the sidecar.
Trigger: a session binds PR #42 before its first flush (e.g. bound before its first turn); PR #42 merges. The listing keeps reporting state: 'open' — badge and tooltip stuck on "open" — until the session's first flush plus one further sweep; for a session that never turns, indefinitely. After a daemon restart the bridge entry is recreated without prs, so an unflushed live session's binding is not shown at all.
Witness (probe splitting on the single variable "transcript present"): live-only arm (sidecar only) → sweep {scanned: 0, updated: 0, state: 'open'}; control arm (transcript present) → {scanned: 1, updated: 1, state: 'merged'}. With the sweep discovering orphaned sidecars directly: both arms {scanned: 1, updated: 1, state: 'merged'}.
Suggested fix: best-effort read the session's PR sidecar in the live-only insertion branch (as enrichPrSidecars does) and let its state win over the live bind-time state; and/or make the sweep discover sidecars of sessions not yet in storage.
中文说明
[Critical] R1-27：live-only 会话（尚无存储记录）同时绕过了这里的 sidecar 优先状态合并与刷新扫描，其 PR 状态被冻结在绑定时刻。元数据绑定处理器通过 upsertSessionPr 写入 sidecar 但不 flush 会话记录；扫描通过存储层的 sessionService.listSessions 枚举（SESSION_FILE_PATTERN 只匹配 .jsonl transcript，永远不会匹配孤立的 .pr.json）；且三条列表路径的 live-only 插入分支都直接插入 {...live} 而不读取 sidecar。
触发场景：会话在首次 flush 前绑定了 PR #42（例如首轮对话前绑定），随后 PR #42 合入。列表会继续返回 state: 'open'——badge 与 tooltip 停留在 "open"——直到该会话首次 flush 加上再一次扫描；对于始终未产生对话的会话，永久如此。daemon 重启后 bridge 条目重建时不带 prs，未 flush 的 live 会话的绑定完全不可见。
证据（仅以"有无 transcript"单一变量分裂的探针）：live-only 分支（仅 sidecar）→ 扫描 {scanned: 0, updated: 0, state: 'open'}；对照分支（有 transcript）→ {scanned: 1, updated: 1, state: 'merged'}。让扫描直接发现孤立 sidecar 后：两个分支均为 {scanned: 1, updated: 1, state: 'merged'}。
建议修复：在 live-only 插入分支中尽力读取该会话的 PR sidecar（如同 enrichPrSidecars），让其 state 覆盖 live 绑定时刻的状态；和/或让扫描能够发现尚未进入存储的会话的 sidecar。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const have = new Set(existing?.map((pr) => pr.number)); | ||
| for (const number of numbers) { | ||
| if (have.has(number)) { |
There was a problem hiding this comment.
[Critical] R1-32: Backfill is permanently non-idempotent for sessions resolving more than SESSION_PR_LIST_LIMIT (10) PRs. upsertSessionPr slices to the latest 10, evicting the oldest numbers; have is rebuilt from the capped sidecar each run, so evicted numbers look unbound and are re-upserted with a fresh createdAt — evicting the next pair in turn and moving re-bound entries to the end, which flips the "latest" entry the badge renders (last = latest) and the tooltip's latest-first order. This contradicts the design doc's own 重复调用幂等 promise.
Trigger: a long-lived session whose worktree branch + transcript gitBranch records map to 12 distinct PRs. Every manual POST /sessions/backfill-prs forever reports new binds, rewrites createdAt, and reshuffles which PR the UI shows as newest.
Witness (probe, three consecutive runs on unmodified code, one session with 12 mapped PRs): run1 {bound: 12} sidecar [3..12] (1, 2 evicted); run2 {bound: 2, alreadyBound: 10} sidecar [5..12, 1, 2] — latest flips 12→2; run3 {bound: 2} sidecar [7..12, 1, 2, 3, 4] — rotates forever. Candidate fix (bind only numbers.slice(-SESSION_PR_LIST_LIMIT)): run1 {bound: 10}, run2/3 {bound: 0, alreadyBound: 10}, sidecar stable, createdAt identical.
Suggested fix: bound per-session binding to the cap instead of writing then evicting — resolve numbers first and bind only the last SESSION_PR_LIST_LIMIT of them (counting the rest separately), so a second run finds every persisted number already in have and reports bound: 0.
中文说明
[Critical] R1-32：对于解析出超过 SESSION_PR_LIST_LIMIT（10）个 PR 的会话，回填永久不幂等。upsertSessionPr 截取最新 10 条、逐出最旧的编号；have 每次运行都从被截断的 sidecar 重建，于是被逐出的编号看起来未绑定，会以新的 createdAt 重新 upsert——继而逐出下一对，并把重绑条目移到末尾，从而翻转 badge 渲染的"最新"条目（last = latest）与 tooltip 的最新优先顺序。这与设计文档自己承诺的"重复调用幂等"相悖。
触发场景：一个长生命周期会话，其 worktree 分支 + transcript gitBranch 记录映射到 12 个不同的 PR。此后每次手动 POST /sessions/backfill-prs 都会永远报告新绑定、重写 createdAt，并翻转 UI 显示的"最新" PR。
证据（探针，未修改代码上连续三次运行，单个会话映射 12 个 PR）：run1 {bound: 12} sidecar [3..12]（1、2 被逐出）；run2 {bound: 2, alreadyBound: 10} sidecar [5..12, 1, 2]——最新从 12 翻转为 2；run3 {bound: 2} sidecar [7..12, 1, 2, 3, 4]——永久轮转。候选修复（只绑定 numbers.slice(-SESSION_PR_LIST_LIMIT)）：run1 {bound: 10}，run2/3 {bound: 0, alreadyBound: 10}，sidecar 稳定，createdAt 不变。
建议修复：把每会话的绑定限制在上限内，而不是先写入再逐出——先解析 numbers，只绑定其中最后 SESSION_PR_LIST_LIMIT 个（其余单独计数），这样第二次运行会发现所有已持久化编号都在 have 中，报告 bound: 0。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| hasControlCharacter((pr as SessionPrInfo).url) || | ||
| ((pr as SessionPrInfo).state !== undefined && | ||
| (pr as SessionPrInfo).state !== 'open' && |
There was a problem hiding this comment.
[Suggestion] R1-1: The new state validation reuses the error message that only describes the number/url constraints, so an invalid state is misreported as a malformed number/url (the REST route validator in session.ts does the same). A client sending { pr: { number: 5, url: 'https://github.com/o/r/pull/5', state: 'draft' } } — number and url valid, but 'draft' outside {open, merged, closed} — gets "must be an object with a positive integer number and an http(s) url …", pointing at two fields that are fine. 'draft' is a likely mistake: gh reports draft PRs and core's GitHubPullRequestState includes it. Consider extending the message, e.g. "… and an optional state of 'open' | 'merged' | 'closed'".
中文说明
[Suggestion] R1-1：新增的 state 校验复用了只描述 number/url 约束的错误消息，导致非法 state 被误报为 number/url 格式错误（session.ts 的 REST 路由校验器同样如此）。客户端发送 { pr: { number: 5, url: 'https://github.com/o/r/pull/5', state: 'draft' } }——number 与 url 合法，但 'draft' 不在 {open, merged, closed} 中——会得到"必须是带正整数 number 与 http(s) url 的对象……"，把问题指向两个实际合法的字段。'draft' 是很可能的误用：gh 会报告 draft PR，core 的 GitHubPullRequestState 也包含它。建议扩展消息，例如"……以及可选的 state，取值 'open' | 'merged' | 'closed'"。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round. This round was bounded to the four Critical findings plus their coupled hardening; the error-message wording fix (mentioning the optional state union) is a small follow-up and stays on the queue.
中文说明
延迟到下一轮。本轮范围限定在 4 个 Critical 发现及其配套加固；错误消息措辞修复（提及可选的 state 取值范围）是一个小的后续项，保留在队列中。
There was a problem hiding this comment.
Deferred to the next round: this round's batch was capped (~8 findings) and prioritized the new Critical R4-1 (convention-number eviction through cap writes) plus its sibling hardening; this finding stays queued and valid. The one-line message extension (mentioning the optional state union) will land with the session.ts twin.
中文说明
延后至下一轮处理：本轮批次受限（约 8 条），优先处理了新的 Critical R4-1（上限写入导致的约定编号逐出）及其相关加固；该发现仍然有效，已排队待处理。一行错误消息扩展（提及可选 state 取值）将与 session.ts 的孪生问题一起落地。
| runtime.env.effectiveEnv, | ||
| { state: 'all', limit: 500, slim: true }, |
There was a problem hiding this comment.
[Suggestion] R1-18: Bindings whose PR number falls outside the single 500-entry gh page are skipped every sweep and never get their state refreshed — and the window cannot be widened meaningfully because runGhPrList clamps limit to ≤ 1000 and fetches a single page. gh pr list returns newest-updated first, so long-settled PRs — precisely the ones an open→merged sweep must transition — fall out of the window first. In any repo with more than 500 PRs (this repository exceeds it ~19×), a session bound to an out-of-window PR keeps its stale snapshot forever: the UI shows "open" for a PR merged years ago — the exact staleness this sweep was added to fix. Consider resolving numbers still missing from numberToState after the page fetch (e.g. gh pr view <number> --json state, keeping the conservative no-reset semantics).
中文说明
[Suggestion] R1-18：PR 编号落在单页 500 条 gh 结果之外的绑定，每次扫描都会被跳过、状态永不刷新——而且窗口无法实质性放宽，因为 runGhPrList 把 limit 钳制在 ≤ 1000 且只取单页。gh pr list 按最近更新优先返回，因此早已尘埃落定的 PR——恰恰是 open→merged 扫描必须转换的对象——最先跌出窗口。在任何 PR 数超过 500 的仓库（本仓库约为其 19 倍），绑定到窗口外 PR 的会话会永久保留陈旧快照：UI 对多年前已合入的 PR 显示 "open"——正是这个扫描要消除的陈旧。建议在分页拉取后，对 numberToState 中仍缺失的编号逐个解析（如 gh pr view <number> --json state，保持保守的"不重置"语义）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round. Resolving numbers absent from the 500-entry page (e.g. targeted gh pr view <number> --json state, keeping the no-reset semantics) is real for large repos, but adds per-number gh spawns that need a rate-limit budget; queued as its own change.
中文说明
延迟到下一轮。对落在 500 条分页之外的编号逐个解析（如定向 gh pr view <number> --json state，保持不重置语义）在大型仓库中是真实缺口，但会引入按编号的 gh 调用、需要速率配额预算；作为独立改动排队。
There was a problem hiding this comment.
Deferred to the next round: this round's batch was capped (~8 findings) and prioritized the new Critical R4-1 (convention-number eviction through cap writes) plus its sibling hardening; this finding stays queued and valid. Resolving out-of-window numbers (per-number gh pr view, keeping no-reset semantics) is a self-contained extension and gets its own round.
中文说明
延后至下一轮处理：本轮批次受限（约 8 条），优先处理了新的 Critical R4-1（上限写入导致的约定编号逐出）及其相关加固；该发现仍然有效，已排队待处理。窗口外编号解析（逐个 gh pr view、保持不重置语义）是独立扩展，将单独占一轮。
There was a problem hiding this comment.
Deferred. Resolving numbers outside the single gh page needs per-number gh pr view calls (bounded by how many stale bindings a workspace carries) — a rate-limit/design tradeoff that should be a deliberate choice, not a drive-by addition in a Critical-fix round. Stays on the backlog with this trail.
中文说明
延后。解析单页 gh 结果之外的编号需要按编号调用 gh pr view（次数受限于一个 workspace 承载多少陈旧绑定）——这是速率配额/设计上的取舍，应当是刻意的决策，而不是 Critical 修复轮次里的顺手添加。在此留痕，保留在待办清单。
There was a problem hiding this comment.
Deferred to the follow-up queue (still unresolved at the current head). Verified real: bindings whose PR number falls outside the single 500-entry gh page are skipped every sweep. Resolving them needs per-number gh pr lookups — a rate-limit vs staleness tradeoff that deserves its own change rather than this converging round.
中文说明
延后到跟进队列（当前 head 上仍未解决）。已核实属实：PR 编号落在单页 500 条 gh 窗口之外的绑定每次扫描都被跳过。解决它们需要逐编号的 gh pr 查询——速率限制与陈旧度之间的权衡，值得单独一次改动，而不是放在本收敛轮中。
| for (const target of pendingNumbers) { | ||
| const states = new Map<number, SessionPrState>(); | ||
| for (const number of target.numbers) { |
There was a problem hiding this comment.
[Suggestion] R1-6: A failing sidecar write aborts the rest of the workspace sweep — per-session isolation exists for reads (try/catch around readSessionPrs) but not for these updateSessionPrStates writes. Probe: session A (active, chats dir chmod 555) + session B (archived), gh reporting both merged → the write on A throws EACCES, result = undefined, and stateB stays open — B was never advanced. Wrapping the per-target update in try/catch flipped it to result {scanned: 2, updated: 1}, stateB = merged. Since pendingNumbers order is stable (mtime-descending), a persistently failing sidecar first in that order starves every older session's state updates forever — permanent staleness for a one-file fault. The code documents the sibling intent one level up ("A single workspace's failure must not starve the rest").
Suggested fix: wrap the per-target update in try/catch and continue, e.g. try { if (await updateSessionPrStates(target.prPath, states)) updated += states.size; } catch { /* one unwritable sidecar must not starve the rest */ }.
中文说明
[Suggestion] R1-6：一次失败的 sidecar 写入会中止该 workspace 扫描的剩余部分——读取有按会话隔离（readSessionPrs 外的 try/catch），但这些 updateSessionPrStates 写入没有。探针：会话 A（active，chats 目录 chmod 555）+ 会话 B（archived），gh 报告两者均已合入 → 对 A 的写入抛出 EACCES，result = undefined，stateB 保持 open——B 从未被推进。把按目标的更新包进 try/catch 后翻转为 result {scanned: 2, updated: 1}、stateB = merged。由于 pendingNumbers 顺序稳定（mtime 降序），一个在该顺序靠前且持续写入失败的 sidecar 会永久饿死所有更旧会话的状态更新——一个文件故障换来永久陈旧。代码在上一层已经写明了同类意图（"单个 workspace 的失败不得饿死其余"）。
建议修复：把按目标的更新包进 try/catch 并继续，例如 try { if (await updateSessionPrStates(target.prPath, states)) updated += states.size; } catch { /* 一个不可写 sidecar 不得饿死其余 */ }。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (states.size === 0) continue; | ||
| if (await updateSessionPrStates(target.prPath, states)) { | ||
| updated += states.size; |
There was a problem hiding this comment.
[Suggestion] R1-24: updated is documented as "Bindings whose state was rewritten" but adds states.size — every binding present in the gh page — whenever updateSessionPrStates changed anything in that sidecar, so unchanged bindings are counted as rewritten. Probe, exact scenario: a session bound to #42 open and #43 open, gh reporting #42 merged / #43 open → observed updated: 2 though exactly one binding changed (the unchanged #43 hits state === entry.state inside updateSessionPrStates). The timer discards the result today (zero runtime cost), but the new tests assert updated as a measure of rewrites and none exercises the mixed multi-binding case, so the inflated semantics get codified; any future logging/metrics consumer inherits the wrong number. Consider counting actual rewrites (have updateSessionPrStates return the number of entries it changed — it already tracks changed).
中文说明
[Suggestion] R1-24：updated 的文档是"状态被重写的绑定数"，但实际累加的是 states.size——gh 分页中出现的全部绑定——只要 updateSessionPrStates 改变了该 sidecar 中的任何内容，未变化的绑定也被计为重写。探针，精确场景：会话绑定 #42 open 与 #43 open，gh 报告 #42 merged / #43 open → 观察到 updated: 2，而实际只有一个绑定变化（未变化的 #43 在 updateSessionPrStates 内部命中 state === entry.state）。定时器目前丢弃结果（运行时零开销），但新测试把 updated 当作重写次数来断言，且没有覆盖多绑定混合场景，膨胀语义因此被固化；未来任何日志/指标消费者都会继承错误的数字。建议统计真实重写数（让 updateSessionPrStates 返回实际改变的条目数——它已经跟踪了 changed）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round. Making updated count actual rewrites (returning the changed-entry count from updateSessionPrStates) touches the core service signature; queued with its mixed-multi-binding test.
中文说明
延迟到下一轮。让 updated 统计真实重写数（由 updateSessionPrStates 返回实际改变的条目数）涉及 core 服务签名改动；与其多绑定混合场景测试一同排队。
There was a problem hiding this comment.
Deferred to the next round: this round's batch was capped (~8 findings) and prioritized the new Critical R4-1 (convention-number eviction through cap writes) plus its sibling hardening; this finding stays queued and valid. Counting actual rewrites (returning the changed-entry count from updateSessionPrStates) is queued.
中文说明
延后至下一轮处理：本轮批次受限（约 8 条），优先处理了新的 Critical R4-1（上限写入导致的约定编号逐出）及其相关加固；该发现仍然有效，已排队待处理。统计真实重写数（让 updateSessionPrStates 返回实际改变的条目数）已排队。
| for (const runtime of deps.workspaceRegistry.listAll()) { | ||
| if (!runtime.trusted) continue; |
There was a problem hiding this comment.
[Suggestion] R1-11: startSessionPrRefreshTimer has no test at all — the test file imports only refreshWorkspaceSessionPrStates and resolveSessionPrRefreshIntervalMs, so the untrusted-workspace skip here, the running re-entrancy guard, the disabled (undefined) path, and dispose() are uncovered. Deleting if (!runtime.trusted) continue; makes the sweep read and write .pr.json sidecars in untrusted workspaces' session storage and spawn gh from their cwd — the route-level equivalent is tested ("untrusted workspace skipped"), but the timer's is not, so that mutation ships green. Consider a timer test with fake timers: a registry with one trusted and one untrusted runtime, asserting only the trusted workspace's sidecar is touched, an overlapping tick is skipped, and QWEN_SESSION_PR_REFRESH_MINUTES=0 returns undefined.
中文说明
[Suggestion] R1-11：startSessionPrRefreshTimer 完全没有测试——测试文件只导入了 refreshWorkspaceSessionPrStates 与 resolveSessionPrRefreshIntervalMs，因此这里的受信任跳过、running 重入守卫、关闭（undefined）路径与 dispose() 都无覆盖。删除 if (!runtime.trusted) continue; 会让扫描读写不受信任 workspace 会话存储中的 .pr.json sidecar，并从其 cwd 拉起 gh——路由层的等价场景有测试（"untrusted workspace skipped"），定时器这里没有，因此该变异能绿着上线。建议用假定时器补一个定时器测试：注册表含一个受信任与一个不受信任 runtime，断言只有受信任 workspace 的 sidecar 被触及、重叠的 tick 被跳过、QWEN_SESSION_PR_REFRESH_MINUTES=0 返回 undefined。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round. The timer-level tests (untrusted-workspace skip, re-entrancy guard, disabled path, dispose) with fake timers are queued as their own batch.
中文说明
延迟到下一轮。定时器级测试（不受信任 workspace 跳过、重入守卫、关闭路径、dispose）用假定时器作为独立批次排队。
There was a problem hiding this comment.
Deferred to the next round: this round's batch was capped (~8 findings) and prioritized the new Critical R4-1 (convention-number eviction through cap writes) plus its sibling hardening; this finding stays queued and valid. Fake-timer tests for the trust guard, 0 short-circuit, re-entrancy, and dispose are queued.
中文说明
延后至下一轮处理：本轮批次受限（约 8 条），优先处理了新的 Critical R4-1（上限写入导致的约定编号逐出）及其相关加固；该发现仍然有效，已排队待处理。信任守卫、0 短路、重入与 dispose 的假定时器测试已排队。
| {pr.state === 'merged' || pr.state === 'closed' | ||
| ? ` · ${ | ||
| pr.state === 'merged' |
There was a problem hiding this comment.
[Suggestion] R1-12: The new "· Merged"/"· Closed" state suffix (and the two new i18n keys sidebar.sessionPrStateMerged/Closed) is a changed user-visible behavior with no test coverage, although SessionDetailsTooltip.test.tsx already exercises PR-row rendering and its fixture entries carry no state. Swapping the two label branches (merged renders "Closed" and vice versa) or deleting the block ships green — this label and the badge dimming are the only UI assertions of merge state. Consider extending the existing "shows the bound pull request as a link" case (or adding a sibling): give one binding state: 'merged' and one state: 'closed', assert the row text contains · Merged / · Closed, and assert an open/state-less binding renders without the suffix.
中文说明
[Suggestion] R1-12：新增的"· Merged"/"· Closed"状态后缀（以及两个新 i18n 键 sidebar.sessionPrStateMerged/Closed）是用户可见的行为变更，却没有测试覆盖，而 SessionDetailsTooltip.test.tsx 已经在测试 PR 行渲染，其 fixture 条目都不带 state。交换两个标签分支（merged 渲染 "Closed"，反之亦然）或删除整块都能绿着上线——该标签与 badge 弱化是合入状态仅有的两处 UI 断言。建议扩展既有的"将绑定的 pull request 显示为链接"用例（或新增一个）：给一个绑定 state: 'merged'、另一个 state: 'closed'，断言行文本包含 · Merged / · Closed，并断言 open/无 state 的绑定不带后缀。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round. Extending SessionDetailsTooltip.test.tsx with state: 'merged'/'closed' suffix assertions is a small web-shell test batch on its own.
中文说明
延迟到下一轮。在 SessionDetailsTooltip.test.tsx 中补充 state: 'merged'/'closed' 后缀断言，作为独立的 web-shell 小测试批次。
There was a problem hiding this comment.
Deferred to the next round: this round's batch was capped (~8 findings) and prioritized the new Critical R4-1 (convention-number eviction through cap writes) plus its sibling hardening; this finding stays queued and valid. Tooltip · Merged/· Closed suffix cases in SessionDetailsTooltip.test.tsx are queued.
中文说明
延后至下一轮处理：本轮批次受限（约 8 条），优先处理了新的 Critical R4-1（上限写入导致的约定编号逐出）及其相关加固；该发现仍然有效，已排队待处理。SessionDetailsTooltip.test.tsx 中 · Merged/· Closed 后缀用例已排队。
There was a problem hiding this comment.
Deferred. The tooltip state suffix (· Merged / · Closed) still has no witness in SessionDetailsTooltip.test.tsx; adding the two fixture cases is straightforward but did not fit this round's bounded Critical-first batch. Tracked here so it is not dropped.
中文说明
延后。tooltip 的状态后缀（· Merged / · Closed）在 SessionDetailsTooltip.test.tsx 中仍无见证；补两个 fixture 用例本身简单，但未纳入本轮有上限的 Critical 优先批次。在此记录以免遗漏。
Review round 1 findings on the PR-state feature. The refresh interval resolver validated minutes but not the converted milliseconds: values above setInterval's 32-bit max clamp to 1 ms (a "monthly" interval becomes a continuous sweep hot loop), sub-minute values degenerate the same way, and a blank env value parsed as 0 and silently disabled the sweep. Fall back to the default for all three; keep 0 as the disable. parsePrNumberFromWorktree accepted `pr-0` (a legal user slug) and bound number 0, which isValidSessionPr rejects — invalidating the whole sidecar and permanently dropping prior bindings on the next upsert. Gate both slug and branch conventions on n > 0. Live-only sessions (bound before their first transcript flush) bypassed both the sidecar-wins state merge and the sweep, freezing their PR state at bind time. All three listing paths now build the live-only insertion through a shared helper that best-effort reads the sidecar with the same sidecar-wins merge rule, and the sweep enumerates `.pr.json` sidecars directly (new SessionService.listSessionIdsWithPrSidecar) instead of walking transcripts — discovering unflushed sessions and removing the pagination loop entirely. Backfill bound every resolved number and let upsertSessionPr evict past the cap, so sessions with more than SESSION_PR_LIST_LIMIT PRs re-bound the evicted numbers forever, rotating the badge's latest entry each run. Bind only the cap's tail (excess counted as overLimit), making repeat runs converge to bound: 0. Both the sweep and backfill write phases also isolate per-sidecar failures instead of aborting the rest.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review round summary — PR #9729This round addresses all four Critical findings from the automated review plus the closely-coupled cheap Suggestions, and adds regression witnesses for two more. 25 remaining Suggestions are deferred to the next round with per-thread replies ( Critical findings — all four fixed (each witnessed by a test that fails on the pre-round code)
Suggestions addressed this round
Review bodies
Deferred to the next round25 Suggestions (error-message wording R1-1×2, bridge state-equality/coverage R1-31/R1-9/R1-10, backfill test hardening R1-7/R1-36/R1-37/R1-14, URL/remote corners R1-3, transcript parsing R1-2/R1-16, gh degradation visibility R1-33, branch→number staleness R1-4, archive/delete races R1-26/R1-39, negative memo R1-19, binding order R1-21, import Verification
中文说明审查轮次总结 — PR #9729本轮处理自动审查的全部 4 个 Critical 发现，以及与之紧密耦合的低成本 Suggestion，并为另外两个发现补充了回归见证测试。其余 25 个 Suggestion 延迟到下一轮，并逐线程附回复（见 Critical 发现 — 4 个全部修复（每个都有"在改动前代码上必然失败"的测试作见证）
本轮处理的 Suggestion
审查主体评论
延迟到下一轮25 个 Suggestion（错误消息措辞 R1-1×2、bridge state 相等/覆盖 R1-31/R1-9/R1-10、回填测试加固 R1-7/R1-36/R1-37/R1-14、URL/remote 边角 R1-3、transcript 解析 R1-2/R1-16、gh 降级可见性 R1-33、branch→number 陈旧 R1-4、归档/删除竞态 R1-26/R1-39、失败结果记忆 R1-19、绑定顺序 R1-21、import 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
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:
- R2-8 backfill never reserves sidecar capacity for existing bindings — dropped as overlapping the existing R1-32 comment (3836900701) at packages/cli/src/serve/routes/session-pr-backfill.ts:263
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| numberToState.set(pr.number, pr.state === 'draft' ? 'open' : pr.state); | ||
| if (pr.headRefName) branchToNumber.set(pr.headRefName, pr.number); |
There was a problem hiding this comment.
[Critical] R1-5: branchToNumber is built with last-write-wins over a newest-first PR list, so when several PRs share a head branch the OLDEST PR wins the mapping and the current PR is never bound. The slim field set omits updatedAt, so parseGhPrList's sort is a stable no-op and the list stays in gh's newest-created-first order.
Branch chore/deps produced PR #10 (merged in January) and was reused for PR #250 (open in July); a July session whose transcript records gitBranch: chore/deps is backfill-bound to PR #10 — the stale merged PR — while PR #250 can never be bound through the branch mapping. The session listing permanently shows the wrong PR link. No test covers a duplicated head branch.
Witness: Real-population sweep over this repo's newest-500-PR window: 5 head branches are reused; simulating line 226's mapping, 5 of 5 bind the OLDEST PR. End-to-end probe: session on chore/deeps with gh order [pr(250, open), pr(10, merged)] → bound [{"number":10,"state":"merged"}] (PR #250 never bound); with first-write-wins fix → bound [{"number":250,"state":"open"}].
Suggested fix: First-write-wins on the newest-first list: if (pr.headRefName && !branchToNumber.has(pr.headRefName)) branchToNumber.set(pr.headRefName, pr.number);
中文说明
branchToNumber 在“最新在前”的 PR 列表上用 last-write-wins 构建：多个 PR 共用同一 head 分支时，映射会落到最旧的 PR，当前 PR 永远无法通过分支映射绑定。机制更深一层：slim 字段集（number,url,headRefName,state）不含 updatedAt，mapEntry 对每条记录赋 updatedAt: 0，parseGhPrList 的排序成为稳定的空操作——列表保持 gh pr list 的创建时间倒序。触发场景：分支 chore/deps 一月产出 PR #10（已合入）、七月复用产出 PR #250（open）；七月会话的 transcript 记录了该分支 → 回填绑定到陈旧的 #10，列表永久显示错误 PR。证据：对本仓库最近 500 个 PR 的真实扫描发现 5 个复用分支，按此映射 5/5 全部绑定最旧 PR；端到端探针在修复（first-write-wins）前后翻转。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| async function liveOnlySummary( | ||
| live: BridgeSessionSummary, | ||
| sessionService: SessionService, |
There was a problem hiding this comment.
[Critical] R2-2: The list route's live-only fast path (listLiveWorkspaceSessionsForResponse) bypasses liveOnlySummary, so live-only rows served through it render the bind-time state — violating the exact invariant this diff adds liveOnlySummary (and three tests) to establish.
A secondary workspace runtime with zero persisted active transcripts (hasActivePersistedSessions counts transcripts, never sidecars). A live session binds PR #5 while open; the bind route persists the sidecar before any flush; PR #5 merges and the sweep rewrites the sidecar to merged. A client polling the first page takes the fast path (usePersisted === false) and gets state: 'open' — raw bridge prs, no sidecar read — while the same request on the persisted path returns 'merged'. For an archived-only secondary workspace the stale state is served indefinitely.
Witness: Route probe via supertest against createServeApp with a secondary workspace: GET /workspaces//sessions → prs[0] = { number: 9517, state: 'open' } while the sidecar says 'merged'; forcing the gate onto the persisted path returns state 'merged' — probe flips.
Suggested fix: Route the fast path through the same sidecar merge — make listLiveWorkspaceSessionsForResponse async and apply liveOnlySummary per row, or drop the fast path when any live row carries prs; alternatively document the fast path as bind-time-only if that staleness is intended.
中文说明
列表路由的 live-only 快速路径（listLiveWorkspaceSessionsForResponse）绕过了 liveOnlySummary，因此经由它的 live-only 行渲染的是绑定时刻的 state——恰好违反本 diff 新增 liveOnlySummary（及三个测试）所要建立的不变量。触发场景：次级（非 primary）workspace 且没有已持久化的活跃 transcript（hasActivePersistedSessions 只统计 transcript，从不统计 sidecar）；live 会话在首次 flush 前绑定 PR #5（open），sidecar 已写入；PR 合入后定时扫描把 sidecar 更新为 merged。客户端轮询第一页走快速路径（usePersisted === false）拿到 state: 'open'（原始 bridge prs，不读 sidecar），而同一请求走持久化路径却返回 merged。对“仅归档”的次级 workspace，过期状态会无限期持续。证据：对 createServeApp 的 supertest 探针：prs[0].state 为 open 而 sidecar 为 merged；把门禁强制走持久化路径后返回 merged，探针翻转。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| await fsp.chmod(chatsDir, 0o555); | ||
| try { | ||
| const result = await refreshWorkspaceSessionPrStates(runtime); |
There was a problem hiding this comment.
[Critical] R2-1: The new permission-based test 'keeps sweeping archived sessions when a sidecar write fails' relies on POSIX directory-permission semantics without the win32/root guard that every comparable test in this repo applies, so it fails on the Windows merge-queue lane and any root run.
The test_windows merge-group job runs packages/cli vitest; on Windows fs.chmod(dir, 0o555) only toggles the read-only attribute and file creation is governed by the directory ACL, so the intended EACCES never happens — the active sidecar write succeeds, the sweep returns { scanned: 2, updated: 2 } instead of { scanned: 2, updated: 1 }, and the follow-up state assertion also fails. The identical failure occurs whenever the suite runs as root (root bypasses the 0o555 bit). Repo convention for this shape: if (process.platform === 'win32' || process.getuid?.() === 0) return; (workspace-artifact-directory.test.ts:149, record-artifact.test.ts:385).
Witness: uid=0 container run of the real test (node:22-bookworm): × keeps sweeping archived sessions when a sidecar write fails → AssertionError: expected { scanned: 2, updated: 2 } to deeply equal { scanned: 2, updated: 1 } at session-pr-refresh.test.ts:374; same file passes 14/14 as non-root.
Suggested fix: Guard the test like its siblings: if (process.platform === 'win32' || process.getuid?.() === 0) return; (or it.skipIf(...)).
中文说明
新增的基于权限的测试 keeps sweeping archived sessions when a sidecar write fails 依赖 POSIX 目录权限语义，却没有本仓库同类测试都加的 win32/root 守卫。Windows 上 fs.chmod(dir, 0o555) 只切换只读属性、文件创建由目录 ACL 决定，预期的 EACCES 不会发生——活跃 sidecar 写入成功，扫描返回 { scanned: 2, updated: 2 } 而非 { scanned: 2, updated: 1 }，后续状态断言也会失败；root 运行（root 无视 0o555 位）同样失败。而 test_windows 是 merge_group 必跑门禁。按仓库惯例加守卫即可。证据：以 uid=0 容器实际运行该测试 → AssertionError: expected { scanned: 2, updated: 2 } to deeply equal { scanned: 2, updated: 1 }；非 root 下该文件 14/14 全绿。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| for (const sessionId of sessionService.listSessionIdsWithPrSidecar( | ||
| archiveState, | ||
| )) { |
There was a problem hiding this comment.
[Critical] R2-3: This round's switch from transcript-driven listSessions to the new sidecar-driven listSessionIdsWithPrSidecar drops the project-membership filter that listSessions applies via sessionBelongsToCurrentProject — so the sweep rewrites PR sidecars belonging to other projects that share the same chats dir.
Storage.getProjectDir keys the project dir by sanitizeCwd(cwd), which maps every non-alphanumeric to '-', so distinct repos like ~/dev/my-app and ~/dev/my.app collide onto one chats dir — the hazard listSessions documents and filters out, and which the old sweep inherited. resolveSessionRuntimeBaseDir defaults every workspace to the shared ~/.qwen base. Workspace B's sweep then reads workspace A's .pr.json, fetches gh pr list in repo B, and rewrites A's bindings with repo B's states whenever small PR numbers collide. A wrongly-set 'merged' is terminal (merged entries are filtered out of every future sweep) — permanently wrong state from another repo's PR; an 'open' result makes the two workspaces' sweeps rewrite the same sidecar back and forth forever.
Witness: Probe: runtimes A (/work/my-app) and B (/work/my.app), shared base dir — chatsDir(A) === chatsDir(B) (-work-my-app); B sees A's session via listSessionIdsWithPrSidecar but [] via listSessions (old filter dropped it); B's sweep with gh reporting its own PR #42 merged → result { scanned: 1, updated: 1 }, A sidecar → state:'merged'; with the membership filter restored → { scanned: 0, updated: 0 }, A sidecar stays 'open'.
Suggested fix: Restore membership filtering on the new path: when the session's transcript exists, apply the same sessionBelongsToCurrentProject check before queueing the sidecar — or, collision-safe for pre-flush sidecars too, stamp the project hash into the sidecar at bind time and skip mismatches.
中文说明
本轮把扫描枚举从“transcript 驱动的 listSessions”切换为新的“sidecar 驱动的 listSessionIdsWithPrSidecar”，丢掉了 listSessions 经由 sessionBelongsToCurrentProject 施加的项目归属过滤——扫描可能改写其他项目共享同一 chats 目录的 PR sidecar。Storage.getProjectDir 以 sanitizeCwd(cwd) 为键（所有非字母数字字符映射为 -），~/dev/my-app 与 ~/dev/my.app 会碰撞到同一目录——这正是 listSessions 文档明言并用哈希过滤防御的场景；resolveSessionRuntimeBaseDir 默认所有 workspace 共享 ~/.qwen 基目录。触发场景：workspace B 的扫描读到 workspace A 的 <uuid>.pr.json，用仓库 B 的 gh pr list 结果改写 A 的绑定；一旦错误置为 merged 即为终态（merged 条目被后续扫描过滤），永久错误；置为 open 则两个 workspace 的扫描每 5 分钟互相翻写同一文件。证据：探针构造 /work/my-app 与 /work/my.app 两个 runtime → chatsDir 相同；B 经新枚举看到 A 的会话（旧 listSessions 过滤为空）；B 的扫描使 A 的 sidecar 变为 merged；恢复归属过滤后 { scanned: 0, updated: 0 }，A 保持 open。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| }); | ||
| }); | ||
|
|
||
| describe('backfillWorkspaceSessionPrs', () => { |
There was a problem hiding this comment.
[Suggestion] R2-10: Every backfill test mocks fetchGitHubPullRequests with mockResolvedValue and never inspects the call arguments (no toHaveBeenCalledWith anywhere in the file), so the load-bearing fetch options { state: 'all', limit: 500, slim: true } can regress without any test turning red.
A one-token edit from state: 'all' to state: 'open' leaves all 21 tests green (runGhPrList builds gh pr list --state options.state ?? 'open'); gh then excludes merged/closed PRs, so sessions whose branches map only to a merged PR are never bound — silently breaking backfill's primary use case (binding worktrees whose PR already merged). The same blind spot covers limit: 500 and slim: true.
Witness: Mutant state: 'open' → Tests 21 passed (suite blind); adding the seam assertion → 1 failed; reverting the mutant with the assertion in place → 21 passed.
Suggested fix: In one existing case assert the seam: expect(fetchGitHubPullRequestsMock).toHaveBeenCalledWith(expect.anything(), expect.anything(), expect.objectContaining({ state: 'all', limit: 500, slim: true }));
中文说明
所有回填测试都用 mockResolvedValue mock fetchGitHubPullRequests 且从不检查调用参数（文件里没有一处 toHaveBeenCalledWith），因此实现传入的关键抓取参数 { state: 'all', limit: 500, slim: true } 回归时没有任何测试变红。场景：把 state: 'all' 改成 state: 'open'（runGhPrList 组装 gh pr list --state options.state ?? 'open'），21 个测试全绿；此后 gh 排除已合入/关闭的 PR，只映射到已合入 PR 的会话永远不会被绑定——静默破坏回填的首要用途（绑定 PR 已合入的 worktree）。证据：变异 → 21 全过；加上参数断言 → 1 失败；还原变异后全绿。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| let sidecar: Awaited<ReturnType<typeof readSessionPrs>>; | ||
| try { | ||
| const sidecarPath = sessionService.getPrSessionPathForArchiveState( |
There was a problem hiding this comment.
[Suggestion] R2-12: The best-effort sidecar-read guard inside the new liveOnlySummary — the try/catch that degrades an unreadable/invalid live-only .pr.json to the bridge's bind-time state — has no test on any of the three listing paths this diff rewires onto it.
A future simplification drops the try/catch (or replaces readSessionPrs with a variant that rethrows). readSessionPrs deliberately rethrows non-ENOENT I/O errors (EACCES/EISDIR). With a live-only session whose sidecar path is a directory or an unreadable file, every session-list request on the plain path then fails outright, and on the organized/metadata paths the outer catch flips to liveMergeFailed, dropping live sessions merged after the bad row instead of one row degrading. The four new server.test.ts cases seed only valid sidecars.
Witness: Probe placing a directory at the live-only session's .pr.json path → unmodified code passes (row renders with bind-time prs); mutant removing the try/catch → probe fails with EISDIR while the four existing tests stay green.
Suggested fix: Add a case placing a directory at the live-only session's .pr.json path (EISDIR avoids the chmod win32/root hazard), asserting the row still renders with the bridge's bind-time prs and the plain-path request succeeds.
中文说明
新 liveOnlySummary 内的尽力读取守卫（把不可读/非法的 live-only .pr.json 降级为 bridge 绑定时刻状态的 try/catch）在本 diff 切换到的三条列表路径上均无测试。场景：未来简化删掉 try/catch：readSessionPrs 对非 ENOENT I/O 错误（EACCES/EISDIR）故意重抛——当某 live-only 会话的 sidecar 路径是目录或不可读文件时，普通路径的整个列表请求直接失败；组织/元数据路径的外层 catch 置 liveMergeFailed，丢弃坏行之后合并的 live 会话。新增的四个测试只种子有效 sidecar。证据：在 sidecar 路径放目录的探针对原码绿、对删除 try/catch 的变异报 EISDIR，且四个现有测试在该变异下仍绿。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round (not declined): this round's batch was bounded by the four Critical fixes and their witnesses. The EISDIR case for the liveOnlySummary read guard (directory at the live-only session's .pr.json path, row degrades to bind-time prs) remains planned as suggested.
中文说明
顺延至下一轮（非拒绝）：本轮批次被四条 Critical 修复及其见证测试占满。liveOnlySummary 读取守卫的 EISDIR 用例（在 live-only 会话的 .pr.json 路径放目录，行降级为绑定时刻 prs）仍按原建议排期。
There was a problem hiding this comment.
Deferred to the next round: this round's batch was capped (~8 findings) and prioritized the new Critical R4-1 (convention-number eviction through cap writes) plus its sibling hardening; this finding stays queued and valid. The liveOnlySummary unreadable-sidecar case (directory at the path, plain path still renders) is queued.
中文说明
延后至下一轮处理：本轮批次受限（约 8 条），优先处理了新的 Critical R4-1（上限写入导致的约定编号逐出）及其相关加固；该发现仍然有效，已排队待处理。liveOnlySummary 不可读 sidecar 用例（路径放目录、普通路径仍正常渲染）已排队。
There was a problem hiding this comment.
Deferred. The liveOnlySummary degrade-guard case (a directory at the live-only session's .pr.json path, asserting the row still renders with bind-time prs) did not fit this round's bounded Critical-first batch. The guard itself is in place; the witness is on the backlog and tracked here.
中文说明
延后。liveOnlySummary 降级守卫的用例（在 live-only 会话的 .pr.json 路径放一个目录，断言该行仍以绑定时刻的 prs 渲染）未纳入本轮有上限的 Critical 优先批次。守卫本身已就位；见证测试列入待办清单并在此记录。
| }); | ||
| }); | ||
|
|
||
| describe('refreshWorkspaceSessionPrStates', () => { |
There was a problem hiding this comment.
[Suggestion] R1-35: The sweep's corrupt/unreadable-sidecar skip guard (try { prs = await readSessionPrs(prPath); } catch { continue; } + if (!prs) continue;) has zero test coverage — no test seeds an invalid or unreadable .pr.json. The enumeration rewrite preserved both skip arms, but the coverage test deferred in round 1 never landed.
A future simplification removes if (!prs) continue; (or the try/catch). A hand-edited or partially written sidecar makes readSessionPrs return null and prs.filter(...) throws a TypeError; a permission-denied sidecar throws EACCES. The exception escapes refreshWorkspaceSessionPrStates, the timer's per-workspace catch swallows it, and every binding in that workspace silently stops refreshing until the file is repaired by hand.
Witness: Mutation A (remove null-skip): proposed corrupt-sidecar test red — TypeError: Cannot read properties of null (reading 'filter'); existing 14/14 green. Mutation B (remove read try/catch): test red — EACCES; existing suite green.
Suggested fix: Seed two sessions with valid open sidecars, corrupt one (await fsp.writeFile(prPathB, '{invalid'); avoid chmod-based read failures unless guarded per the repo convention), mock gh returning both numbers merged, and assert { scanned: 1, updated: 1 }, the healthy sidecar became merged, and the corrupt file was left untouched.
中文说明
扫描对损坏/不可读 sidecar 的跳过守卫（try { … readSessionPrs … } catch { continue; } 与 if (!prs) continue;）零测试覆盖——枚举重写保留了这两个分支，但第 1 轮承诺的覆盖测试未落地。场景：未来简化删掉 if (!prs) continue;（或 try/catch）：手工编辑/部分写入的 sidecar 使 readSessionPrs 返回 null，prs.filter(...) 抛 TypeError；权限拒绝的 sidecar 抛 EACCES。异常逃出 refreshWorkspaceSessionPrStates，被定时器每-workspace catch 吞掉——该 workspace 的所有绑定静默停止刷新，直到手工修复文件。证据：两个方向的变异都让建议的损坏-sidecar 测试红、而现有 14 个测试绿。建议：种子两个有效 sidecar、把其中一个写成 '{invalid'，断言 { scanned: 1, updated: 1 }、健康者变 merged、损坏文件原样保留。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round (not declined): this round's batch was bounded by the four Critical fixes and their witnesses. The corrupt-sidecar sweep case (two valid sidecars, one corrupted with '{invalid', gh reporting both merged, assert { scanned: 1, updated: 1 } and the corrupt file untouched) remains planned as suggested.
中文说明
顺延至下一轮（非拒绝）：本轮批次被四条 Critical 修复及其见证测试占满。损坏 sidecar 的扫描用例（两个有效 sidecar、其一写成 '{invalid'、gh 报告两者 merged、断言 { scanned: 1, updated: 1 } 且损坏文件原样保留）仍按原建议排期。
| await upsertSessionPr(prPath, { | ||
| number: 999, | ||
| url: 'https://github.com/o/r/pull/999', | ||
| state: 'open', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R2-9: The test 'does not write back open for bindings missing from the gh page' cannot detect the regression it is named for: because the seeded binding is already 'open', a mutation that maps gh-absent numbers to 'open' produces no rewrite, so { scanned: 1, updated: 0 } and the 'open' assertion still pass — the mutation survives.
A future 'simplification' defaults absent numbers to 'open'. A session bound to a closed PR that falls outside gh pr list's 500-entry page is then rewritten to state: 'open' on the next sweep, and every session listing shows a closed PR as open. The implementation's own comment marks the skip-absent invariant load-bearing ('Only a number ABSENT from gh's page is skipped'), yet no test turns red under the mutation.
Witness: Mutant states.set(number, state ?? 'open') → all 14 existing refresh tests pass; the suggested sibling case (seed 'closed') is red against the mutant and green on clean code.
Suggested fix: Add a sibling case seeding the binding with state: 'closed' and the same gh page missing 999, asserting { scanned: 1, updated: 0 } and that the persisted state stays 'closed'.
中文说明
测试 does not write back open for bindings missing from the gh page 无法捕获其命名所指回归：种子绑定已是 'open'，若变异把 gh 缺页编号映射为 'open'（states.set(number, state ?? 'open')），updateSessionPrStates 见到 state === entry.state 返回 null、不产生改写，{ scanned: 1, updated: 0 } 与 'open' 断言照样通过——变异存活。场景：未来“简化”把缺页编号默认为 'open'：绑定到 closed PR 且落在 500 条页外的会话会被改写为 'open'，列表把已关闭 PR 显示为 open；实现自己的注释标明该不变量是关键（“Only a number ABSENT from gh's page is skipped”）。证据：变异下 14 个现有测试全过；种子 'closed' 的兄弟用例对变异红、对原码绿。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| try { | ||
| if (await updateSessionPrStates(target.prPath, states)) { | ||
| updated += states.size; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-24: updated counts every pending binding of a sidecar whenever any one of them changed (updated += states.size), contradicting its documented meaning 'Bindings whose state was rewritten'.
A sidecar holds PR 42 (open) and PR 43 (open); gh reports 42 merged and 43 still open. updateSessionPrStates rewrites only entry 42 and returns non-null, so updated += 2 although exactly 1 binding's state changed. The timer discards the result today, but any future logging/monitoring/surfacing consumer trusting the documented semantics overcounts.
Witness: Probe: sidecar seeded 42 open + 43 open, gh returning 42 merged / 43 open → result { scanned: 1, updated: 2 }, persisted [[42,'merged'],[43,'open']] — one change counted as two.
Suggested fix: Count actual rewrites (have updateSessionPrStates return the changed entries/count and add that), or amend the doc to count sidecars covered by a rewrite.
中文说明
updated 在某 sidecar 只要有任一绑定变化时就累加 states.size（该 sidecar 的全部待处理绑定数），与文档“Bindings whose state was rewritten”不符。场景：sidecar 有 PR 42（open）、43（open）；gh 报告 42 merged、43 仍 open → 只改写了 1 条，updated 却加 2。当前定时器丢弃结果，但任何未来信任该语义的日志/监控消费方都会高估。证据：探针 → result { scanned: 1, updated: 2 }，持久化 [[42,'merged'],[43,'open']]。建议：让 updateSessionPrStates 返回实际改写的条目/数量并累加该值。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round (not declined): this round's batch was bounded by the four Critical fixes and their witnesses. Making updated count actual rewrites (returning the changed count from updateSessionPrStates) or amending the documented semantics remains planned as suggested.
中文说明
顺延至下一轮（非拒绝）：本轮批次被四条 Critical 修复及其见证测试占满。让 updated 统计实际改写数（updateSessionPrStates 返回改写计数）或修订文档语义，仍按原建议排期。
There was a problem hiding this comment.
Deferred to the next round: Same finding as the round-1 updated-semantics thread; deferred under it — this round's batch was capped (~8 findings) and prioritized the new Critical R4-1 (convention-number eviction through cap writes) plus its sibling hardening; this finding stays queued and valid.
中文说明
延后至下一轮处理：与第 1 轮 updated 语义线程是同一发现，归入该线程一并延后——本轮批次受限（约 8 条），优先处理了新的 Critical R4-1（上限写入导致的约定编号逐出）及其相关加固；该发现仍然有效，已排队待处理。
| export function startSessionPrRefreshTimer(deps: { | ||
| workspaceRegistry: WorkspaceRegistry; | ||
| env?: Readonly<Record<string, string | undefined>>; |
There was a problem hiding this comment.
[Suggestion] R1-11: startSessionPrRefreshTimer has no test anywhere: the untrusted-workspace skip, the QWEN_SESSION_PR_REFRESH_MINUTES=0 short-circuit inside the timer, the re-entrancy guard, and dispose() are all unexercised (resolveSessionPrRefreshIntervalMs itself is unit-tested).
If the timer's trust guard regressed, the daemon would spawn gh and rewrite .pr.json sidecars inside a workspace that was never granted trust — the exact boundary the backfill route's tested guard protects. A regression turning '0' back into 'default' would re-enable a sweep the operator explicitly switched off, with no failing test.
Witness: Repo-wide grep: startSessionPrRefreshTimer appears only in its implementation and the production call site (run-qwen-serve.ts:5168); no test imports it.
Suggested fix: Fake-timer tests: (a) untrusted runtime → no refresh after advancing past FIRST_RUN_DELAY_MS; (b) '0' → returns undefined, schedules nothing; (c) dispose() prevents subsequent ticks.
中文说明
startSessionPrRefreshTimer 完全没有测试：不受信任 workspace 跳过、QWEN_SESSION_PR_REFRESH_MINUTES=0 在定时器内的短路、重入守卫、dispose() 均未覆盖（resolveSessionPrRefreshIntervalMs 本身有单测）。若信任守卫回归，daemon 会在从未授予信任的 workspace 里拉起 gh 并改写 .pr.json——正是回填路由已测守卫所保护的边界；若 0 回归为默认值，会重新启用运维明确关闭的扫描。建议：fake-timer 测试三个分支。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round (not declined): this round's batch was bounded by the four Critical fixes and their witnesses. The fake-timer cases for startSessionPrRefreshTimer (untrusted skip, 0 disable, dispose) remain planned as suggested.
中文说明
顺延至下一轮（非拒绝）：本轮批次被四条 Critical 修复及其见证测试占满。startSessionPrRefreshTimer 的 fake-timer 用例（不受信任跳过、0 关闭、dispose）仍按原建议排期。
There was a problem hiding this comment.
Deferred to the next round: Same finding as the round-1 timer-tests thread; deferred under it — this round's batch was capped (~8 findings) and prioritized the new Critical R4-1 (convention-number eviction through cap writes) plus its sibling hardening; this finding stays queued and valid.
中文说明
延后至下一轮处理：与第 1 轮定时器测试线程是同一发现，归入该线程一并延后——本轮批次受限（约 8 条），优先处理了新的 Critical R4-1（上限写入导致的约定编号逐出）及其相关加固；该发现仍然有效，已排队待处理。
- Backfill branch-to-PR mapping is first-write-wins on the newest-first gh list, so a reused head branch binds the newest PR, not the oldest. - The over-cap slice reserves a slot for the convention (pr-<N>) number instead of evicting it first. - The refresh sweep re-checks project membership for sidecar-discovered sessions, so sanitized-cwd collisions cannot cross-rewrite sidecars. - The live-only list fast path merges the PR sidecar like the persisted paths, rendering the sweep-refreshed state instead of bind-time state. - The invalid-pr 400 message now names the state constraint; the permission-based sweep test gets the repo's win32/root guard.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round: no action (growth-audit round, verdict
|
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9729 (same-run verification repair)This round repairs the deterministic rejection of commit The rejectionThe verification gate failed Root causeMain commit Reproduction evidence (this round):
Fix (one commit, test-only)
Carried dispositions of the 82 inline findings (commit f04f177, preserved)
VerificationCommands actually run this round:
Environment note: test runs inside this interactive agent session initially showed unrelated failures — the session exports 中文说明本轮总结 — PR #9729（同轮验证修复）本轮修复提交 拒绝原因验证门禁在 根因main 上的提交 复现证据（本轮）：
修复（一次提交，纯测试）
82 条行内发现的承继处置（提交 f04f177，保留）
验证本轮实际运行的命令：
环境说明：在本交互式 agent 会话内的测试运行最初出现过无关失败——会话导出了 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
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.
8 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- transcript-branch regex escape truncation (session-pr-backfill.ts:149) — already reported as R1-2 (comment 3836900725)
- daemon refresh-timer happy-path wiring untested (run-qwen-serve.ts:5194) — already recorded in the round-9 deferral list (review 5003656717)
- createServeApp mount of POST /sessions/backfill-prs untested (server.ts:2106) — already recorded in the round-11 deferral list (review 5006236253)
- normalizeRemoteToWebUrl rejects scp-style remotes with non-git users (session-pr-backfill.ts:73) — already recorded in the round-4 deferral list (review 5001878012)
- sweep 500-PR window staleness (session-pr-refresh.ts:123) — already reported as R1-18 (comment 3836900766)
- sweep-write sidecar resurrection race (session-pr-service.ts:225) — already recorded in the round-9 deferral list (review 5003656717)
- getRemoteWebUrl unbounded synchronous execSync (session-pr-backfill.ts:95) — already recorded in the round-7 deferral list (review 5002868793)
- file-scoped throwing vi.mock of session-pr-refresh (run-qwen-serve.test.ts:8359) — already recorded in the round-11 deferral list (review 5006236253)
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) unit lanes were skipped in CI; only the linux lane and the local linux run exercised the suites.
Not explored to full depth (tool budget reached): "agent 1c": none — though I did not run typecheck/tests; the compile-level edges (barrel exports, signatures) were verified by reading declarations instead.; chunk 11: could not execute session-pr-refresh.test.ts — the review worktree lacks built workspace-package dist outputs and npm run build failed silently in this envi…; chunk 4: could not execute packages/cli/src/serve/routes/session-pr-backfill.test.ts — the review worktree has no node_modules , and a full install + build exceeds th….
Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/routes/session-pr-backfill.ts:249 — [probe] degraded run (unknown default branch) reports ghAvailable: true, defeating the field's contractpackages/cli/src/serve/server/session-pr-refresh.ts:111 — [probe] stateless sidecar entries (the sweep's reason to exist) have no test witness; an excluding mutation ships greenpackages/acp-bridge/src/bridge.ts:9906 — [probe] no-republish clause unpinned for stateless re-binds over a stateful entry; spurious event + revision bump mutant ships greenpackages/cli/src/serve/server/session-list.ts:539 — [probe] mergeSummaryPrs stateless-persisted branch unpinned; dropping it strips state from every live badge, 1087 tests greenpackages/cli/src/serve/server/session-pr-refresh.test.ts:55 — [probe] refresh-interval negative-value branch unpinned; a <= 0 consolidation silently disables the sweeppackages/core/src/services/session-pr-service.ts:195 — [probe] upsertSessionPr explicit-state precedence (pr.state ?? known?.state) unpinned; operand-swap mutant ships greenpackages/cli/src/serve/routes/session-pr-backfill.ts:244 — [review] backfill head-branch mapping hard-limited to the newest-500 gh page; window misses leave no binding and no signalpackages/cli/src/serve/routes/session-pr-backfill.ts:278 — [probe] backfill closed-state passthrough unpinned; a closed-to-open mutant passes 52/52packages/cli/src/serve/routes/session.ts:5482 — [review] rename-only PATCH returns prs state from the bridge entry the sweep never updates; response lags the sidecarpackages/cli/src/serve/server/session-pr-refresh.ts:186 — [review] timer tick per-workspace failure-isolation guard unwitnessed; guard-removal starves later workspaces, suite greenpackages/web-shell/client/i18n.tsx:4388 — [review] new ZH state labels unwitnessed; a swap ships green while zh users see merged/closed labels flippedpackages/cli/src/serve/server/session-pr-refresh.test.ts:222 — [probe] order-preservation contract pinned only with ascending seeds; a number-sort mutant ships green
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 8 条建议级发现已在 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/windows-latest, Node 22.x) unit lanes were skipped in CI; only the linux lane and the local linux run exercised the suites。
未探索到全部深度（达到工具调用预算）："agent 1c"：none — though I did not run typecheck/tests; the compile-level edges (barrel exports, signatures) were verified by reading declarations instead.；chunk 11：could not execute session-pr-refresh.test.ts — the review worktree lacks built workspace-package dist outputs and npm run build failed silently in this envi…；chunk 4：could not execute packages/cli/src/serve/routes/session-pr-backfill.test.ts — the review worktree has no node_modules , and a full install + build exceeds th…。
未审查：反向审计——有审计 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，agent 实际被要求做的并不是本 skill 所认证的内容。
收敛姿态下延后（第 13 轮，非阻断）——已记录，本轮不要求修改：共 12 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (!existing) return 0; | ||
| let changed = 0; | ||
| const next = existing.map((entry) => { | ||
| const state = states.get(entry.number); |
There was a problem hiding this comment.
[Critical] updateSessionPrStates applies a number-keyed state map — built solely from the workspace repo's gh pr list — to every persisted entry, ignoring each entry's url. A binding whose URL points at a different repository is rewritten with the workspace repo's same-numbered PR's state, and because merged entries are filtered out of every future sweep, the wrong state is permanent.
The metadata routes accept any http(s) pr.url (parseSessionPrBody validates scheme/length/control-chars only), so a client can bind {number: 42, url: 'https://github.com/other-org/other-repo/pull/42'} while the workspace repo also has a PR #42. The next daemon sweep runs gh pr list --state all in the workspace, sees #42 merged, and writes state: 'merged' onto the external-repo binding — the badge then lies about the other repository's PR. Worse, the sweep's eligibility filter (p.state !== 'merged') exempts the wrongly-merged entry from every future refresh, so the corruption never self-heals.
Witness (probe against unmodified code, sweep driven with the binding above and a mocked workspace fetch reporting the workspace's own #42 merged):
persisted: [{"number":42,"url":"https://github.com/other-org/other-repo/pull/42","state":"merged"}]
result: {"scanned":1,"updated":1}
With a url-matching guard (apply state only when the fetched PR's url equals the binding's url) the probe flips to {"scanned":1,"updated":0} with the binding's state still 'open'.
Key the refresh by repository, not bare number — either group pending numbers by the host/owner/repo parsed from each entry's url and fetch per repo, or skip entries whose URL does not match the workspace remote:
// in refreshWorkspaceSessionPrStates, when building numberToState:
// keep the fetched PR's url alongside the state...
const numberToState = new Map<number, { state: SessionPrState; url: string }>();
// ...and in updateSessionPrStates, apply only on match:
const mapped = states.get(entry.number);
if (mapped && mapped.url === entry.url) { /* rewrite state */ }中文说明
[Critical] updateSessionPrStates 把“仅从当前 workspace 仓库的 gh pr list 构建、以 PR 编号为键”的状态映射套用到每一条持久化绑定上，完全忽略各条目的 url。若某条绑定的 URL 指向另一个仓库，它会被改写成 workspace 仓库中同号 PR 的状态；又因为 merged 条目会被排除在后续所有刷新之外，错误状态将永久存在。
元数据路由接受任意 http(s) 的 pr.url（parseSessionPrBody 只校验协议/长度/控制字符），因此客户端可以绑定 {number: 42, url: 'https://github.com/other-org/other-repo/pull/42'}，而 workspace 仓库恰好也有一个 #42。下一次 daemon 扫描在 workspace 里执行 gh pr list --state all，看到 #42 已合并，就把 state: 'merged' 写到这条指向外部仓库的绑定上——badge 从此对另一个仓库的 PR 显示错误状态。更糟的是，扫描的过滤条件（p.state !== 'merged'）会把这条被误标为 merged 的条目排除在此后所有刷新之外，错误无法自愈。
证据（对未修改代码的探针：用上述绑定驱动扫描，并让 workspace 的 mock 返回“本仓库 #42 已合并”）：
persisted: [{"number":42,"url":"https://github.com/other-org/other-repo/pull/42","state":"merged"}]
result: {"scanned":1,"updated":1}
加上“仅当抓取到的 PR url 与绑定 url 一致才应用状态”的守卫后，探针翻转为 {"scanned":1,"updated":0}，绑定状态保持 'open'。
建议按仓库（而非裸编号）建立刷新映射：把待定编号按各条目 url 解析出的 host/owner/repo 分组、按仓库分别抓取；或跳过 URL 与 workspace 远端不匹配的条目（示例代码见英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind Autofix round stopped: gate rejection is load-induced test timeouts, not a code defectThe deterministic verification rejected commit Evidence
中文说明🤖 AutoFix 更新了一个过期的 base —— 修复未通过验证，但本 PR 落后于 Autofix 本轮停止：门禁拒绝是负载导致的测试超时，并非代码缺陷确定性验证拒绝了提交 50289bde9c（“fix(core): scope session PR state refresh to the binding's url (#9729)”），原因是 packages/cli 中的 证据
另外已排除的因素：
Run log: https://github.com/QwenLM/qwen-code/actions/runs/32801566273 🧠 Handled by Qwen Code · model/模型 |
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 reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 14, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/routes/session.ts:5498 — [review] REST PATCH metadata accept/persist/echo of a valid pr.state is untestedpackages/cli/src/serve/run-qwen-serve.ts:5191 — [review] Refresh-timer daemon wiring (start/generation guard/teardown) is untestedpackages/cli/src/serve/routes/session-pr-backfill.ts:269 — [review] Default-branch guard trusts clone-time-only origin/HEAD; staleness re-enables fork-PR misattributionpackages/web-shell/client/components/sidebar/SessionDetailsTooltip.test.tsx:162 — [review] Tooltip test never renders state:open; a truthy-refactor would mislabel open bindings as Closedpackages/cli/src/serve/routes/session-pr-backfill.test.ts:492 — [review] gh-available-but-number-out-of-window fallback path has no test witnesspackages/cli/src/serve/routes/session-pr-backfill.test.ts:175 — [probe] No test pins that credentials are stripped from the derived web URL persisted into the sidecarpackages/cli/src/serve/routes/session-pr-backfill.ts:73 — [probe] scp-style remotes with a non-git user are silently left unresolvedpackages/cli/src/serve/routes/session-pr-backfill.ts:157 — [review] Whole-transcript in-memory reads in backfill can spike daemon heappackages/cli/src/serve/server.test.ts:15507 — [probe] mergeSummaryPrs stateless-sidecar branch has no witness; guard-removal ships greenpackages/cli/src/serve/server/session-pr-refresh.ts:174 — [review] Production process.env fallback for the refresh interval is exercised by no testpackages/cli/src/serve/server/session-pr-refresh.ts:190 — [probe] Reentrancy-flag reset path is unwitnessed; a one-shot timer mutation ships greenpackages/cli/src/serve/routes/session-pr-backfill.test.ts:482 — [probe] Remote-URL memoization hit path is unwitnessed; miss-only-cache mutation ships greenpackages/cli/src/serve/routes/session-pr-backfill.ts:376 — [probe] Cap trim evicts in plan order; a re-resolvable dialog binding can be evictedpackages/cli/src/serve/server/session-pr-refresh.ts:184 — [probe] Per-workspace sweep failure isolation is unwitnessed; removing it crashes the daemonpackages/core/src/services/session-pr-service.ts:69 — [probe] New persisted-state validation clause has no rejection-path test row
Convergence: round 14 posted 4 inline comment(s), 3 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/core/src/services/session-pr-service.ts (findings in round 13; 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.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 14 轮，非阻断）——已记录，本轮不要求修改：共 15 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 14 轮发布了 4 条行内评论，其中 3 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/core/src/services/session-pr-service.ts（第 13 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (!existing) return 0; | ||
| let changed = 0; | ||
| const next = existing.map((entry) => { | ||
| const state = states.get(entry.number); |
There was a problem hiding this comment.
[Critical] R13-1: updateSessionPrStates applies a number-keyed state map — built solely from this workspace's gh pr list — to every persisted entry, ignoring each entry's url. A binding whose URL points at a different repository is rewritten with this workspace's same-numbered PR's state, and because merged entries are filtered out of every future sweep, the wrong state is permanent. A client can bind {number: 42, url: 'https://github.com/other-org/other-repo/pull/42'} (the metadata route accepts any http(s) url) while this workspace also has a PR #42; the next sweep sees #42 merged here and stamps merged onto the external binding, and the state !== 'merged' filter then exempts it from every future refresh — it never self-heals. Re-checked at this commit: the mechanism still fires.
Witness (probe at the reviewed commit):
seeded: [{number:42, url: other-org/other-repo/pull/42, state:"open"}]
sweep: workspace gh page reports its own #42 merged
result: {scanned:1, updated:1}
after: [{number:42, url: other-org/other-repo/pull/42, state:"merged"}]
Key the refresh by repository, not bare number — carry each fetched PR's url alongside its state and apply it only when the fetched url equals the binding's url, or group pending numbers by the host/owner/repo parsed from each entry's url and fetch per repo.
中文说明
[Critical] R13-1：updateSessionPrStates 把"仅从当前 workspace 仓库 gh pr list 构建、以编号为键"的状态映射套用到每一条持久化绑定上，忽略各条目的 url。URL 指向其他仓库的绑定会被改写成 workspace 仓库同号 PR 的状态；又因 merged 条目被排除在后续所有刷新之外，错误状态永久存在。客户端可绑定 {number:42, url:'https://github.com/other-org/other-repo/pull/42'}（metadata 路由接受任意 http(s) url），而本仓库恰好也有 #42；下一次扫描见本仓库 #42 已合入，就把 merged 写到这条外部绑定上，且 state !== 'merged' 过滤使其豁免于此后所有刷新——无法自愈。已在当前 commit 复查：机制仍然触发。
修复建议：按仓库而非裸编号建立刷新映射——抓取时把每个 PR 的 url 与状态一并保存，仅当抓取到的 url 与绑定 url 一致时才应用；或按各条目 url 解析出的 host/owner/repo 分组、按仓库分别抓取。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
|
||
| // Transcript records carry the branch the session was on; the set is small | ||
| // per session and only ever compared against PR head branches. | ||
| const GIT_BRANCH_PATTERN = /"gitBranch":"([^"]+)"/g; |
There was a problem hiding this comment.
[Critical] R14-1: collectTranscriptBranches extracts branches with a hand-rolled regex over the rendered JSONL transcript instead of a structured parse, so a gitBranch key nested anywhere inside a record's structured JSON (tool-call arguments/results, MCP payloads in message.parts/toolCallResult) is indistinguishable from the record's own top-level field. The injected branch is mapped to a PR and persisted as a binding the session never ran on, and bogus entries also consume the 64-branch cap, which can push out the genuine branch. The transcript writers JSON-stringify each record, so quotes inside plain text are escaped and don't match — but nested structured values serialize with unescaped keys, which is exactly what the pattern captures. The PR's tests only feed top-level gitBranch records, so this path ships untested.
Witness (probe at the reviewed commit):
record: top-level gitBranch "real-branch" + nested functionResponse.response.gitBranch "feature/login-flow"
regex: matches ["feature/login-flow","real-branch"]
backfill: session bound to the PR whose head branch is feature/login-flow (a PR it never ran on)
fix: per-line JSON.parse reading only top-level gitBranch -> bound:0
Parse each transcript line and read only the top-level gitBranch field (or delegate to the existing transcript reader) instead of matching the rendered text:
for (const line of raw.split('\n')) {
if (!line) continue;
try {
const record = JSON.parse(line) as { gitBranch?: unknown };
if (typeof record.gitBranch === 'string') branches.add(record.gitBranch);
} catch {
// tolerate corrupt/truncated lines
}
if (branches.size >= MAX_DISTINCT_BRANCHES) break;
}中文说明
[Critical] R14-1：collectTranscriptBranches 用手工正则扫描渲染后的 JSONL 文本，而非结构化解析，因此记录嵌套 JSON 里任意位置的 gitBranch 键（工具调用参数/结果、message.parts/toolCallResult 中的 MCP 负载）与记录自身的顶层字段无法区分。被注入的分支会被映射到某个 PR 并持久化为该会话从未运行过的绑定；伪造条目还会占用 64 分支上限，可能挤出真实分支。transcript 写入端对每条记录做 JSON.stringify，纯文本中的引号会被转义而不匹配——但嵌套结构化值序列化时键不带转义，恰是该正则能捕获的形态。本 PR 的测试只喂顶层 gitBranch 记录，因此该路径未经测试即上线。
修复建议：逐行 JSON.parse 且只读取顶层 gitBranch 字段（或复用现有 transcript 读取器），不要用正则匹配渲染文本（示例代码见英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| changed += 1; | ||
| return { ...entry, state }; | ||
| }); | ||
| if (changed === 0) return 0; | ||
| await writeSessionPrs(filePath, next); |
There was a problem hiding this comment.
[Critical] R14-2: updateSessionPrStates (the refresh-sweep write path) can resurrect a PR sidecar that a concurrent session deletion or archive move just removed. movePrSidecar/removePrSidecars run outside mutationQueue, and this queued read→write cycle has no liveness guard — unlike the backfill planner this same PR guards for the identical hazard with existsSync(candidate.transcriptPath). A sweep tick collects session X's numbers, runs gh, and queues the write; the user deletes or archives X between the queued function's readSessionPrs (still sees the file) and writeSessionPrs; the write (mkdir + atomic write) recreates <id>.pr.json at the stale path. For a deleted session the orphan is permanent — sessionPrSidecarBelongsToCurrentProject fails open on a missing transcript, so every future sweep keeps scanning it; for an archive move the next unarchive merge ties on createdAt and the incoming half wins, regressing a freshly written merged snapshot to open.
Witness (probe at the reviewed commit, deletion forced between the queued read and write):
PR code: sidecarExistsAfterDeleteAndWrite:true, readBack state:"merged" (resurrected, transcript gone)
guard: no resurrection (flip)
mergeSessionPrLists(base=[merged], incoming=[open]) at equal createdAt -> state:"open"
Add an in-queue liveness re-check equivalent to backfill's — e.g. an optional predicate evaluated after the in-queue read (if (validate && !validate()) return 0;) that the sweep populates with a transcript/sidecar existence check — or route movePrSidecar/removePrSidecars through enqueuePrMutation so moves/deletes serialize with state writes.
中文说明
[Critical] R14-2：updateSessionPrStates（刷新扫描的写入路径）可能复活一个刚被并发删除/归档移动移除的 PR sidecar。movePrSidecar/removePrSidecars 在 mutationQueue 之外运行，而这个入队的"读→写"循环没有存活校验——本 PR 的 backfill 规划器对同一隐患已用 existsSync(candidate.transcriptPath) 防护。扫描周期收集会话 X 的编号、执行 gh、入队写入；用户在入队函数的 readSessionPrs（仍能看到文件）与 writeSessionPrs 之间删除/归档 X；写入（mkdir + 原子写）会在陈旧路径上重建 <id>.pr.json。对已删除会话，该孤儿永久存在——sessionPrSidecarBelongsToCurrentProject 对缺失 transcript 失败放行，后续每次扫描都会继续扫它；对归档移动，下次取消归档合并时 createdAt 相同、传入的一半胜出，会把刚写入的 merged 快照退回 open。
修复建议：为 updateSessionPrStates 增加入队后的存活复检（例如在入队读取后评估一个可选谓词 if (validate && !validate()) return 0;，由扫描传入 transcript/sidecar 存在性检查）；或把 movePrSidecar/removePrSidecars 也走 enqueuePrMutation，使移动/删除与状态写入串行化。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ), | ||
| ...live.map((l) => { | ||
| const persisted = persistedByNumber.get(l.number); | ||
| return persisted?.state !== undefined && persisted.state !== l.state |
There was a problem hiding this comment.
[Critical] R14-3: Backfill cap eviction rewrites the sidecar without syncing the bridge, and mergeSummaryPrs's union membership resurrects the evicted bindings from stale hydrated live entries — the rendered badge list exceeds the 10-entry cap until the daemon restarts. Backfill A binds 10 branch-mapped PRs to a live session S (sidecar full at SESSION_PR_LIST_LIMIT); any metadata PATCH hydrates the bridge entry with the full sidecar via seedSessionPrs; backfill B plans 12 numbers, all 10 existing droppable, trims the plan and evicts 2 from the sidecar, then invalidates the cache and bumps the catalog. The sidebar refetch runs mergeSummaryPrs(sidecar=[n3..n12], live=[n1..n10]) and keeps n1,n2 as live-only entries → 12 badges rendered including the 2 deliberately evicted. Nothing removes them from the bridge — seedSessionPrs is a no-op once populated, the refresh timer only rewrites state, and backfill never touches bridge entries — until a restart/close-reload/archive-restore recreates the entry.
Witness (probe driving the real listWorkspaceSessionsForResponse at the reviewed commit):
sidecar=[3..12], stale hydrated live=[1..10]
PR code: merged=[11,12,1,2,3,4,5,6,7,8,9,10] length 12 (evicted #1,#2 resurrected, cap 10 exceeded)
pruned: merged=[11,12,3,4,5,6,7,8,9,10] length 10 (live pruned to persisted membership)
Give the bridge a replacement API (e.g. setSessionPrs(sessionId, prs) that overwrites entry.prs even when non-empty, unlike seedSessionPrs) and call it from the backfill route for live sessions whose sidecar write returned a changed list — mirroring the cache-invalidation + catalog pairing it already performs; alternatively prune the live side of mergeSummaryPrs to persisted membership when a persisted sidecar exists.
中文说明
[Critical] R14-3：backfill 上限驱逐重写 sidecar 时未同步 bridge，而 mergeSummaryPrs 的并集成员关系会从陈旧水合的 live 条目中复活被驱逐的绑定——渲染出的徽章列表会超过 10 条上限，直到 daemon 重启。backfill A 为活跃会话 S 绑定 10 个分支映射 PR（sidecar 达到 SESSION_PR_LIST_LIMIT 上限）；任意 metadata PATCH 都会经 seedSessionPrs 把完整 sidecar 水合进 bridge 条目；backfill B 规划 12 个编号，其中 10 个已存在且可丢弃，裁剪计划并从 sidecar 驱逐 2 条，随后失效缓存并提升 catalog 版本。侧栏重新获取时执行 mergeSummaryPrs(sidecar=[n3..n12], live=[n1..n10])，把 n1、n2 作为仅 live 条目保留→渲染出 12 个徽章，包含被刻意驱逐的 2 条。没有任何机制把它们从 bridge 移除——seedSessionPrs 在已有条目时是空操作、刷新定时器只重写 state、backfill 从不触碰 bridge 条目——直到重启/关闭重载/归档恢复重建该条目。
修复建议：为 bridge 提供一个替换式 API（例如 setSessionPrs(sessionId, prs)，即使 entry.prs 非空也整体覆写，不同于 seedSessionPrs），并在 backfill 路由中对 sidecar 写入返回了变化列表的活跃会话调用它——与其已执行的"缓存失效 + catalog 提升"配对；或者，当存在持久化 sidecar 时，把 mergeSummaryPrs 的 live 一侧裁剪到持久化成员集合。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…tes (QwenLM#9729) Address the round-13/14 Critical review findings on the session PR state feature: - updateSessionPrStates applies a fetched state only when the fetched PR's url matches the binding's url, so a workspace PR can no longer stamp its state onto a binding pointing at another repository's same-numbered PR (a wrong terminal state was permanent — merged entries leave the sweep). - The refresh sweep re-checks sidecar liveness at the write commit step via assertCanCommit, so a session deleted or archived mid-sweep no longer gets its sidecar resurrected at the stale path. - collectTranscriptBranches parses each transcript line as JSON and reads only the top-level gitBranch, so nested structured values (tool-call arguments/results, MCP payloads) can no longer inject a branch the session never ran on. - Backfill syncs the hydrated bridge entry through a new overwrite-capable bridge.setSessionPrs after a capped plan evicts bindings, so the summary merge can no longer resurrect evicted numbers from a stale live entry until daemon restart.
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9729 (address-review)All five inline Critical findings are resolved in code (rc:3849083091 and rc:3850994550 are the same R13-1 point). No finding was declined, deferred, or escalated. No conflicts ( Findings and decisionsrc:3849083091 + rc:3850994550 — R13-1 (Critical): sweep stamps workspace-repo state onto foreign-repo bindings — FIXEDReproduced first: a binding Fix: the refresh is now keyed by repository, not bare number. Witnesses: rc:3850994557 — R14-1 (Critical): transcript branch extraction via text regex binds injected nested branches — FIXEDReproduced first: a record whose Fix: Witness: rc:3850994568 — R14-2 (Critical): sweep write resurrects a sidecar a concurrent delete/archive just removed — FIXEDReproduced first: with the sidecar deletion landing between the queued read and the write, unmodified code recreated Fix: Witnesses: rc:3850994574 — R14-3 (Critical): backfill cap eviction desyncs the bridge; mergeSummaryPrs resurrects evicted bindings — FIXEDReproduced first: with a full 10-entry sidecar, a hydrated live bridge entry, and a 12-number plan evicting 2, the rendered merged list held 12 badges including the evicted numbers on unmodified code (new end-to-end test fails on the pre-round code path). Fix (the reviewer's first suggested shape): a new overwrite-capable bridge API Witnesses: Verification
Round-13/14 review bodies listed further items as "Deferred under the convergence posture … recorded, not requested in this round"; per the feedback they are out of scope for this round and untouched. 中文说明轮次总结 — PR #9729（address-review）5 条行内 Critical 发现全部在代码中解决（rc:3849083091 与 rc:3850994550 是同一条 R13-1）。没有拒绝、延后或升级任何发现。无冲突（ 发现与处理rc:3849083091 + rc:3850994550 — R13-1（Critical）：扫描把 workspace 仓库的状态写到指向外部仓库的绑定上 — 已修复先复现：绑定 修复：刷新改为按仓库而非裸编号建立映射。 证据： rc:3850994557 — R14-1（Critical）：文本正则提取 transcript 分支，导致嵌套注入的分支被绑定 — 已修复先复现：记录的 修复： 证据： rc:3850994568 — R14-2（Critical）：扫描写入复活刚被并发删除/归档移除的 sidecar — 已修复先复现：让删除恰好落在入队读取与写入之间，未修改代码会在陈旧路径上重建 修复： 证据： rc:3850994574 — R14-3（Critical）：backfill 上限驱逐与 bridge 失联，mergeSummaryPrs 复活被驱逐的绑定 — 已修复先复现：sidecar 满 10 条、live bridge 条目已水合、规划 12 个编号并驱逐 2 条时，未修改代码渲染出的合并列表有 12 个徽章，含被驱逐的编号（新端到端测试在修改前代码路径上失败）。 修复（采用评审建议的第一种形态）：bridge 新增可覆写 API 证据： 验证
第 13/14 轮评审正文中其余条目被列为"收敛姿态下延后……已记录，本轮不要求修改"；按反馈说明，它们不在本轮范围内，未做改动。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
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 3: could not execute session-pr-backfill.test.ts — the review worktree (and parent checkout) has no node_modules , and npm ci plus the prerequisite builds exc….
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/routes/session-pr-backfill.ts:95 — [review] getRemoteWebUrl blocking execSync with no timeout in the request pathpackages/cli/src/serve/routes/session-pr-backfill.ts:156 — [review] collectTranscriptBranches whole-file readFile + synchronous per-line JSON.parse on the event looppackages/cli/src/serve/server/session-pr-refresh.test.ts:1020 — [review] Timer suite never verifies a second successful sweeppackages/cli/src/serve/server/session-pr-refresh.test.ts:897 — [review] Timer fixtures couple primary to trusted, so the trust filter is indistinguishable from a primary filterpackages/cli/src/serve/server/session-pr-refresh.test.ts:1023 — [review] Overlap test exercises the running guard only in the held direction; the finally release on throw is untestedpackages/cli/src/serve/server/session-pr-refresh.ts:126 — [review] Sweep swallows every gh failure with zero loggingpackages/core/src/services/session-pr-service.ts:227 — [review] Exact-string url equality in the cross-repo guard freezes benign url variants permanentlypackages/cli/src/serve/routes/session-pr-backfill.test.ts:1650 — [review] Backfill route suite never seeds an untrusted primary workspacepackages/cli/src/serve/routes/session-pr-backfill.test.ts:1824 — [review] Per-workspace cache-invalidation/catalog attribution never pinned across two writing workspacespackages/cli/src/serve/server/session-pr-refresh.test.ts:215 — [review] Sweep suite never seeds a stateless binding — backfill's gh-down shape is unpinnedpackages/cli/src/serve/server/session-pr-refresh.test.ts:942 — [review] The deps.env ?? process.env fallback is never exercised by any timer testpackages/cli/src/serve/server/session-pr-refresh.test.ts:936 — [review] Disabled-via-env test asserts API shape only, not that nothing was scheduledpackages/core/src/services/session-pr-service.ts:251 — [review] replaceSessionPrs cannot forward assertCanCommit — backfill commit step unguarded against out-of-queue deletionpackages/cli/src/serve/routes/session-pr-backfill.test.ts:524 — [review] droppable filter unpinned — unresolved convention number never combined with cap pressurepackages/cli/src/serve/server/session-pr-refresh.test.ts:953 — [review] Tick-loop per-workspace exception isolation unpinned — no timer test registers two trusted runtimespackages/cli/src/serve/routes/session-pr-backfill.ts:298 — [review] Fork-PR headRefName collision can shadow same-branch base-repo PRs in the branch mapping
Convergence: round 15 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 4 (3 new). Findings keep coming back to the same files: packages/core/src/services/session-pr-service.ts (findings in rounds 13, 14; 1 more now); packages/cli/src/serve/routes/session-pr-backfill.ts (findings in round 14; 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.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 3：could not execute session-pr-backfill.test.ts — the review worktree (and parent checkout) has no node_modules , and npm ci plus the prerequisite builds exc…。
收敛姿态下延后（第 15 轮，非阻断）——已记录，本轮不要求修改：共 16 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 15 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 4 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/core/src/services/session-pr-service.ts（第 13、14 轮已出过发现，本轮又有 1 条）；packages/cli/src/serve/routes/session-pr-backfill.ts（第 14 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const run = async (): Promise<SessionPr[]> => { | ||
| return enqueuePrMutation(filePath, async () => { | ||
| const existing = (await readSessionPrs(filePath)) ?? []; | ||
| const known = existing.find((entry) => entry.number === pr.number); |
There was a problem hiding this comment.
[Critical] R15-1: upsertSessionPr carries the existing entry's state onto a re-bind matched by PR number alone, even when the url changes to a different repository's same-numbered PR — known is looked up by number only, and the new entry spreads pr.state ?? known?.state unconditionally. This is the exact cross-repo contamination the new sweep guard in updateSessionPrStates (url-match below) exists to prevent; the re-bind path has no such guard. Because merged entries leave the sweep's fetch list (state !== 'merged' filter) and updateSessionPrStates corrects state only when the fetched url matches the entry url, a carried 'merged' is never healed — the wrong terminal state is permanent. The bridge live-entry twin of this pattern is posted separately on bridge.ts.
Failure scenario: a sidecar holds {number: 100, url: repoA/.../pull/100, state: 'merged'}; a client re-binds {number: 100, url: repoB/.../pull/100} with no state (REST parseSessionPrBody and ACP session/update_metadata both accept any http(s) url and state is optional). The entry gets repoB's url and a fresh createdAt but inherits repoA's 'merged' — repoB's open PR displays as merged permanently, in the badge and in every list response.
Witness (probe, both arms at the reviewed commit): PR code persisted {"number":100,"url":"https://github.com/repoB/o/pull/100",...,"state":"merged"} after a stateless re-bind from repoA; with the url-gated carry below the persisted entry has no state key (34/34 existing session-pr-service tests stay green under the fix).
Fix shape — gate the carry on the binding target, comparing normalized urls (a bare strict-equality flip breaks this diff's own same-PR ?v=2 variant behavior; the URL parser already lowercases the host, so normalize repo-path case and trailing slash):
const canon = (u: string) => u.toLowerCase().replace(/\/+$/, '');
const carried =
known && canon(known.url) === canon(pr.url) ? known.state : undefined;
const state = pr.state ?? carried;
// ...spread `state ? { state } : {}` as today中文说明
upsertSessionPr 在重新绑定（re-bind）时仅按 PR 编号匹配已有条目，即使 url 已经换成了另一个仓库的同编号 PR，也会把旧条目的 state 带过去——known 只按 number 查找，新条目无条件展开 pr.state ?? known?.state。这正是本轮在 updateSessionPrStates（下方刷新扫描路径）新增 url 匹配守卫所要防止的跨仓库状态污染，而 re-bind 路径没有这个守卫。由于 merged 条目会离开扫描的拉取列表（state !== 'merged' 过滤），且 updateSessionPrStates 只在拉取到的 url 与条目 url 一致时才纠正状态，被带过去的 'merged' 永远不会被修复——错误的终态将是永久的。同样的模式在 bridge 的实时条目（live entry）中另有一条行内评论。
触发场景：sidecar 中存有 {number: 100, url: repoA/.../pull/100, state: 'merged'}；客户端以不带 state 的 {number: 100, url: repoB/.../pull/100} 重新绑定（REST parseSessionPrBody 与 ACP session/update_metadata 都接受任意 http(s) url，且 state 可选）。条目换成 repoB 的 url、获得新的 createdAt，却继承 repoA 的 'merged'——repoB 处于 open 状态的 PR 将在徽章和所有列表响应中永久显示为已合并。
证据（探针，在受审提交上双臂验证）：PR 代码在从 repoA 无 state 重绑后持久化了 {"number":100,"url":"https://github.com/repoB/o/pull/100",...,"state":"merged"}；采用下面的 url 匹配携带后，持久化条目不再含 state 键（修复下现有 34/34 个 session-pr-service 测试全部通过）。
修复方向——仅当绑定目标不变时才携带 state，并比较规范化后的 url（直接严格相等会破坏本 diff 自身的同 PR ?v=2 变体行为；URL 解析器已会将 host 小写，因此规范化仓库路径大小写与末尾斜杠即可）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Re-binding a number refreshes it and moves it to latest. | ||
| // Re-binding a number refreshes it and moves it to latest; an | ||
| // omitted state preserves the known one (mirrors the sidecar). | ||
| const known = existing.find((p) => p.number === bound.number); |
There was a problem hiding this comment.
[Critical] R15-1: the bridge live-entry re-bind in updateSessionMetadata carries the previous binding's state over to a different PR matched by number alone — the live-side twin of the sidecar finding posted on session-pr-service.ts. The same-binding early-return requires latest.url === bound.url, but the carry lookup known does not, so a cross-repo re-bind without state inherits the old repository's state; markSessionCatalogChanged() and the session_metadata_updated event then publish it. Even with the sidecar carry fixed, mergeSummaryPrs only overrides live state when the persisted entry has one, so list responses keep serving the wrong live state, and the refresh sweep rewrites only sidecars (never live entries) — with QWEN_SESSION_PR_REFRESH_MINUTES=0 the wrong state is permanent.
Failure scenario: the live entry holds {number: 100, url: repoA/.../pull/100, state: 'merged'} (set by an earlier bind or hydrated via seedSessionPrs); a client re-binds {number: 100, url: repoB/.../pull/100} without state — the early-return fails on the url, known matches repoA's entry by number, and the new live entry is stamped merged and broadcast in the metadata event: repoB's open PR is shown as merged.
Witness (probe, both arms at the reviewed commit): PR code served [ { number: 100, state: "merged", url: "https://github.com/repoB/o/pull/100" } ] after the cross-repo stateless re-bind; gating the carry on the binding target removes it. Note the existing preserves the known state on a stateless re-bind test re-binds a same-repo ?v=2 url, so the fix must compare normalized urls (see the session-pr-service.ts comment) rather than flip to strict equality, which breaks that test.
const canon = (u: string) => u.toLowerCase().replace(/\/+$/, '');
const known = existing.find(
(p) => p.number === bound.number && canon(p.url) === canon(bound.url),
);中文说明
updateSessionMetadata 中 bridge 实时条目的重新绑定仅按 number 匹配，就会把上一个绑定的 state 带到另一个仓库的同编号 PR 上——这是 session-pr-service.ts 上 sidecar 发现的实时侧孪生体。同绑定早退（early-return）要求 latest.url === bound.url，但携带查找 known 没有这个要求，因此不带 state 的跨仓库重绑会继承旧仓库的状态；随后 markSessionCatalogChanged() 与 session_metadata_updated 事件会把该状态发布出去。即使修复了 sidecar 侧的携带，mergeSummaryPrs 只在持久化条目有 state 时才覆盖实时 state，所以列表响应仍会提供错误的实时状态；而刷新扫描只改写 sidecar、从不改写实时条目——在 QWEN_SESSION_PR_REFRESH_MINUTES=0 时错误状态将永久存在。
触发场景：实时条目持有 {number: 100, url: repoA/.../pull/100, state: 'merged'}（来自早先绑定或经 seedSessionPrs 注水）；客户端以不带 state 的 {number: 100, url: repoB/.../pull/100} 重绑——早退因 url 不同而失败，known 按编号命中 repoA 的条目，新的实时条目被打上 merged 并通过元数据事件广播：repoB 处于 open 状态的 PR 被显示为已合并。
证据（探针，在受审提交上双臂验证）：跨仓库无 state 重绑后，PR 代码返回 [ { number: 100, state: "merged", url: "https://github.com/repoB/o/pull/100" } ]；将携带限定在绑定目标一致后该状态消失。注意现有 preserves the known state on a stateless re-bind 测试重绑的是同仓库 ?v=2 url，因此修复必须比较规范化后的 url（见 session-pr-service.ts 的评论），而不是直接改为严格相等——那会使该测试失败。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // step with the sidecar; a capped plan can evict numbers, and the | ||
| // stale entry would resurrect them in the summary merge until a | ||
| // daemon restart. No-op when the session is not live. | ||
| runtime.bridge.setSessionPrs?.( |
There was a problem hiding this comment.
[Critical] R15-2: the new setSessionPrs live-entry sync propagates a cap-trim eviction of a concurrently committed client re-bind. The planner's "never dropped" protection (plannedFor = droppable.has(n) && existingNumbers.has(n)) keys on the pre-run snapshot, so it covers only numbers absent from the snapshot: a client upsertSessionPr of a snapshot-held planned number that commits inside the same per-path mutation queue before this rewrite is trimmed out of the plan and evicted from kept, contradicting the comment above the planner. Before this round the stale live entry masked the storage drop in the summary merge; this new setSessionPrs call now overwrites the live entry with the trimmed list, so the binding the daemon just confirmed in the upsert response disappears from storage and every summary at once.
Failure scenario: a session's sidecar holds ~9 of the 10-slot cap and backfill plans numbers for it; while the backfill request runs, a client re-binds PR 5 (a snapshot-held planned number) via ACP/REST. Both writers serialize on the same mutationQueue keyed by sidecar path — queue order alone decides: upsert first → #5 is trimmed, evicted, and setSessionPrs overwrites the live entry with the #5-less list; upsert after the write → #5 survives.
Witness (probe at the reviewed commit, deterministic seam): arm A (client upsert of #5 commits before the rewrite) — persisted [101,102,103,104,105,106,107,108,6,7] and setSessionPrs called with the same list: #5 gone from storage and the live entry, result {bound: 2, alreadyBound: 0, overLimit: 1} (the dropped confirm counts nowhere); arm B (same upsert after the write) — persisted [102,...,108,6,7,5], binding survives. The fix below flips arm A to [101..108,5,7] with all 54 shipped backfill tests green.
Fix shape — exclude entries newer than the run snapshot from the plan (capture a snapshot timestamp before planning and extend the kept filter with || entry.createdAt > snapshotAt), or re-upsert after the rewrite any live binding whose number the rewrite dropped.
中文说明
新增的 setSessionPrs 实时条目同步会把"容量裁剪导致的驱逐"传播出去：当一个并发提交的客户端重绑被裁剪驱逐时，它也会被从实时条目中抹掉。规划器的"永不丢弃"保护（plannedFor = droppable.has(n) && existingNumbers.has(n)）以运行前的快照为键，因此只保护快照中不存在的编号：快照中已有、且已被规划的编号，如果在同一个按路径排序的变更队列里先于本次重写提交（客户端 upsertSessionPr），就会被裁剪移出计划、从 kept 中驱逐——这与规划器上方注释的承诺相矛盾。本轮之前，过期的实时条目会在摘要合并中掩盖存储层的丢失；本轮新增的 setSessionPrs 调用会用裁剪后的列表覆盖实时条目，于是守护进程刚刚在 upsert 响应中确认的绑定会同时从存储和所有摘要中消失。
触发场景：某会话的 sidecar 已占用 10 个容量中的约 9 个，backfill 为其规划了若干编号；在 backfill 请求运行期间，客户端通过 ACP/REST 重绑 PR 5（快照中已有的规划编号）。两个写入者串行于同一按 sidecar 路径为键的 mutationQueue——仅由队列顺序决定结果：upsert 在前 → #5 被裁剪、驱逐，setSessionPrs 用不含 #5 的列表覆盖实时条目；upsert 在写入之后 → #5 保留。
证据（在受审提交上的探针，确定性接缝）：臂 A（客户端对 #5 的 upsert 先于重写提交）——持久化为 [101,102,103,104,105,106,107,108,6,7]，setSessionPrs 收到同样的列表：#5 同时从存储与实时条目中消失，结果 {bound: 2, alreadyBound: 0, overLimit: 1}（被丢弃的确认完全不计数）；臂 B（同一 upsert 在写入之后）——持久化为 [102,...,108,6,7,5]，绑定保留。下面的修复方向可让臂 A 变为 [101..108,5,7]，且现有 54 个 backfill 测试全部通过。
修复方向——把晚于运行快照的条目排除在计划之外（规划前记录快照时间戳，并在 kept 过滤中追加 || entry.createdAt > snapshotAt），或在重写结束后，把被重写丢弃但仍存活的绑定重新 upsert 一次。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…ion (QwenLM#9729) Address the round-15 Critical review findings: - upsertSessionPr and the bridge's updateSessionMetadata carry an existing entry's state onto a re-bind only when the binding target is the same PR: urls are compared in canonical form (host/path case, trailing slash, query, and fragment never change which PR a url names), so a different repository's same-numbered PR no longer inherits a state the refresh sweep can never heal. - The backfill planner treats an entry committed after the run's snapshot read as foreign, so a client re-bind of a snapshot-held planned number can no longer be trimmed out of the plan and evicted by the capped rewrite the daemon just confirmed; slots recompute around it and the live-entry sync publishes the surviving binding.
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9729 (round 16)Critical-only mode was active (growth engagement). Three Critical inline findings were in scope; all three were reproduced on the pre-round code, fixed at the root cause, and pinned with regression tests. One commit: Feedback points and dispositions[rc:3853542416] R15-1 — [rc:3853542426] R15-1 twin — bridge live-entry carry across a cross-repository re-bind (Critical) → RESOLVED. [rc:3853542431] R15-2 — backfill [rv:5019571274] Review body — "Partially reviewed — gaps disclosed" (CHANGES_REQUESTED) → NO ACTION REQUIRED THIS ROUND. Growth audit: Conflict notes: none — VerificationAll commands actually run, in order:
中文说明轮次总结 — PR #9729（第 16 轮）本轮处于仅处理 Critical 的模式（由增长触发）。范围内共 3 条 Critical 行内发现；三条均在改动前代码上复现，从根因修复，并补充回归测试固定。提交一个： 反馈要点与处理结果[rc:3853542416] R15-1 — [rc:3853542426] R15-1 孪生体 — bridge 实时条目跨仓库重绑携带状态（Critical）→ 已解决。 [rc:3853542431] R15-2 — backfill 的 [rv:5019571274] 评审正文 — "部分评审完成——缺口已披露"（CHANGES_REQUESTED）→ 本轮无需处理。 增长审计： 冲突说明：无—— 验证实际运行的全部命令，按顺序：
Deferred non-Critical feedbackCritical-only mode is active: the PR's diff grew src 88 / test 579 net lines beyond this counting window's baseline (budgets: 400/400). The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback continues to flow unaffected during a growth-only engagement (the per-author batch budget applies only after 5 change-producing rounds). ( 中文说明已进入仅处理 Critical 的模式：本计数窗口内 diff 净增长已达 源码 88 / 测试 579 行（预算 400/400）。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。纯增长触发期间维护者反馈照常流动（按作者的批次预算仅在完成 5 个产生改动的轮次后生效）。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
🤖 Could not produce a passing fix for this feedback (round 1/100) — the verification gate rejected the attempt. This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. Address-review summary — PR #9729One commit this round: This round's feedback re-listed the findings of all 15 prior review rounds (the review mechanism lost its incremental anchor, so the whole diff was re-read). Verified against the current head, every Critical finding and nearly every Suggestion was already fixed by earlier commits on this branch ( Implemented this round
Why it was not pushed: bite check: changed tests pass on the pre-round tree (claimed defect does not reproduce) 中文说明🤖 未能为该反馈产生可通过验证的修复（第 1/100 轮） —— 验证门拒绝了该尝试。此项现在需要人工处理；循环保持在线，仍会拾取新反馈与 base 冲突，但不会自行重试此项。 验证门的拒绝原因与日志证据见上方英文部分（gate-rejection 不翻译）。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/32879569566 🧠 Handled by Qwen Code · model/模型 |
4 participants
What this PR does
Extends the session↔PR binding feature (#9543) in two directions. First, an on-demand daemon route backfills PR bindings onto sessions that predate the feature: for every trusted workspace it scans the persisted session catalog (active and archived), resolves each session's PR numbers from the worktree slug/branch convention and — the dominant source in practice — from the intersection of the git branches recorded in each session's transcript with the repository's PR head branches (one batched, slim
gh pr list --state allper workspace), then writes the existing per-session PR sidecars. Second, every binding now carries a merge-state snapshot (open / merged / closed): the creation dialog recordsopen, backfill records the state observed at query time, and a low-frequency daemon sweep (default 5 minutes, configurable, off-able via env) advances the snapshots by re-querying only workspaces that still hold non-merged bindings, rewriting state in place without touching binding order or timestamps. The sidebar badge dims merged PRs and the session details tooltip labels merged/closed rows.Why it's needed
Operators running dozens of concurrent sessions rely on "find the session by PR number", but sessions created before the binding feature had no binding, and even bound PRs went stale: a badge kept its open accent long after the PR merged. Real data showed the originally-planned backfill sources (worktree slug/branch convention) hit almost nothing — PRs are practically never submitted from worktree branches — while the transcript's recorded git branches intersect PR head branches for the large majority of sessions (272 of 342 in the primary workspace), so backfill now uses that intersection. The state snapshot plus the background sweep make the sidebar answer "which of my sessions produced PR N, and is it still open?" without any manual re-run and without putting network calls on the session-list polling path.
Reviewer Test Plan
How to verify
--state all/limit passthrough, MERGED/CLOSED mapping), the backfill route (convention binding, remote-URL fallback, transcript-branch binding, multi-PR binding, idempotency), the refresh sweep (open→merged rewrite, no gh call when everything is merged, gh failure swallowed, reopened-closed PR tracked back to open, interval env parsing), the list merge (sidecar state wins over the live bind-time state), and the badge (merged dimmed, open/stateless accent).qwen serveagainst a workspace with persisted sessions,POST /sessions/backfill-prs, and confirm the response reports scanned/bound counts and that the session list now returnsprswithstate; the Web Shell sidebar shows the badge and dims it once the sweep (or a manual wait past the interval) flips a binding to merged.Evidence (Before & After)
Before: 6454 persisted sessions across 25 registered workspaces carried zero bindings; the list returned no
prs. After running the backfill once on a real daemon: 575 bindings written (288 in the primary workspace, 157 in fastjson2, …), and the first page of the session list showedprson 99 of 100 rows. Before: a merged PR's badge kept the accent style and the tooltip showed no state. After: merged badges render dimmed and tooltip rows read e.g. "Pull Request #9517 · Merged" / "合并请求 #9517 · 已合入".Tested on
Environment
npm run dev -- serveagainst the operator's real daemon (25 workspaces, ~6.4k sessions) plus the repo's vitest suites; gh 2.91 authenticated.Risk & Scope
gh pr list --state all --limit 500per workspace per interval, but only for workspaces holding non-merged bindings; full-field queries at that size hit GitHub GraphQL 504s, hence the slim field set.stateis optional at every validation layer (route, bridge, SDK, sidecar reader), old sidecars withoutstatekeep working, and the sweep is disabled withQWEN_SESSION_PR_REFRESH_MINUTES=0.Linked Issues
Follow-up to #9543 (session↔PR binding).
中文说明
这个 PR 做了什么
在会话↔PR 绑定功能（#9543）基础上扩展两个方向。其一，新增按需 daemon 路由为存量会话回填 PR 绑定：遍历所有 trusted workspace 的持久化会话（active + archived），从 worktree slug/branch 约定、以及（实践中最主要的来源）会话 transcript 记录的 git 分支与仓库 PR head 分支的交集解析 PR 号（每 workspace 一次批量 slim
gh pr list --state all），写入既有的每会话 PR sidecar。其二，每个绑定现在携带合入状态快照（open / merged / closed）：创建对话框记open，回填记查询时刻的状态，一个低频 daemon 定时任务（默认 5 分钟，可用环境变量配置间隔或关闭）只对仍含未合入绑定的 workspace 重新查询并原地回写状态，不改变绑定顺序与时间戳。侧栏 badge 对已合入 PR 弱化显示，会话详情 tooltip 为 merged/closed 行标注状态。为什么需要
同时运行数十个会话的操作者依赖"按 PR 号找会话"，但绑定功能之前的存量会话没有绑定；即使绑定了，PR 合入后 badge 也长期保持 open 样式。真实数据显示原计划的回填来源（worktree slug/branch 约定）几乎零命中——PR 基本不从 worktree 分支提交——而 transcript 记录的 git 分支与 PR head 分支的交集能覆盖大多数会话（主 workspace 342 个会话命中 272 个），因此回填改用该交集。状态快照 + 后台刷新让侧栏无需手动重跑即可回答"哪个会话产出了 PR N，它合入了吗"，且不在会话列表轮询热路径上放网络调用。
审查者测试计划
如何验证
--state all/limit 透传、MERGED/CLOSED 映射）、回填路由（约定绑定、remote URL 兜底、transcript 分支绑定、多 PR 绑定、幂等）、刷新任务（open→merged 回写、全 merged 时零 gh 调用、gh 失败静默、closed 重开回 open、间隔环境变量解析）、列表合并（sidecar state 优先于 live 绑定时 state）、badge（merged 弱化、open/无 state 保持高亮）。qwen serve，POST /sessions/backfill-prs，确认响应 scanned/bound 计数与会话列表返回带state的prs；Web Shell 侧栏显示 badge，合入后弱化。证据（前后对比）
之前：25 个注册 workspace 的 6454 个存量会话零绑定，列表无
prs。真实 daemon 跑一次回填后：写入 575 条绑定（主 workspace 288、fastjson2 157 等），列表首页 100 行中 99 行带prs。之前：已合入 PR 的 badge 保持高亮、tooltip 无状态。之后：merged badge 弱化显示，tooltip 显示"Pull Request #9517 · Merged"/"合并请求 #9517 · 已合入"。测试平台
macOS ✅；Windows/Linux⚠️ 未本地验证（无 OS 特有路径）。
环境
npm run dev -- serve操作者真实 daemon（25 workspace、约 6.4k 会话）+ 仓库 vitest 套件；gh 2.91 已认证。风险与范围
gh pr list --state all --limit 500，且仅对含未合入绑定的 workspace 发起；全字段查询在该规模下触发 GitHub GraphQL 504，故用 slim 字段。gh pr create的自动发现（需在定时器里重扫 transcript，刻意不做；对话框创建路径自绑定，覆盖主流）。Windows/Linux 未本地跑。state在所有校验层（route/bridge/SDK/sidecar 读取）均为可选，旧 sidecar 无state继续可用；QWEN_SESSION_PR_REFRESH_MINUTES=0可关闭定时刷新。关联
#9543（会话↔PR 绑定）的后续。