feat(core): bind PRs created via gh pr create in the session shell - #9739
feat(core): bind PRs created via gh pr create in the session shell#9739wenshao wants to merge 43 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.
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.
The shell tool now recognizes a completed gh pr create (excluding --dry-run) by the PR URL gh prints on success and writes the session's PR sidecar directly with state open, mirroring the worktree sidecar pattern (CLI and daemon modes alike). The backfill route gains the same detector as a third, retroactive source, pairing run_shell_command calls with their responses in persisted transcripts so sessions predating the live hook bind too.
|
Re-run on Template looks good ✓ Problem: real, but narrower than the description says. The live half holds: the merged session↔PR feature (#9543) has no writer for PRs the agent creates via Direction: aligned — the natural completion of the session↔PR binding stream (#9543 → #9729 → this), and the design doc is kept in sync with the removal rather than drifting from it. No CHANGELOG reference, but the area is the merged #9543's declared follow-up. Stack note — changed since the last pass: the branch has diverged from #9729 (21 commits ahead, 29 behind its head). The body's "the PR diff shrinks automatically afterwards" no longer holds — this branch now carries its own copy of the stacked scope, and after #9729 lands it needs a rebase with expected conflicts in the shared files. Merge order stays #9729 first. Size: the divergence changes the size picture too. The displayed diff against main is now 43 files, +5256/−101 ≈ 1.7k production / 3.5k test / ~40 docs lines. For a Approach: the core design remains right — one execution gate plus gh-side attribution (pre/post Risk: Moving on to code review. 🔍 中文说明由 模板完整 ✓ 问题：真实存在，但比描述所说的更窄。实时一半成立：已合入的会话↔PR 功能（#9543）对 agent 在 shell 里 方向：对齐——会话↔PR 绑定系列（#9543 → #9729 → 本 PR）的自然收尾，设计文档与删除保持同步而非漂移。CHANGELOG 无直接引用，但该领域是已合入 #9543 声明的后续。 堆叠提示——与上次审查相比有变化：本分支已与 #9729 分叉（领先 21 个提交、落后其 head 29 个）。正文中"#9729 合入后 diff 自动收缩"的说法不再成立——本分支现在携带堆叠范围的独立副本，#9729 合入后需要 rebase，共享文件预期会冲突。合入顺序仍是先 #9729。 规模：分叉也改变了规模图景。对 main 的展示 diff 现为 43 个文件，+5256/−101 ≈ 生产 1.7k / 测试 3.5k / 文档约 40 行。对触及核心的 方案：核心设计仍然正确——一个执行闸门 + gh 侧归因（运行前后 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code reviewRe-reviewed at the current head. Because the branch has diverged from stacked #9729 (21 ahead / 29 behind its head), this pass covers the whole displayed diff — 43 files, ≈1.7k production lines — not an isolated delta. The wiring quality is high: payload validation on every boundary, URL-identity rules on state inheritance, catalog-clock propagation, cross-process locking. The test suite is substantial (≈3.5k lines). But this pass cannot clear the PR: the Critical findings the review loop posted on this exact head are genuine — I verified the load-bearing ones against the code below, and they stand unresolved here. 1. The PR body describes a feature the code removed. The description says backfill gains a third source pairing 2. Live-hook attribution bypasses — one root cause, three standing findings. The pre-run snapshot carries no repo or branch identity;
3. Backfill re-runs rotate once the sidecar is occupied (standing). Offered candidates are trimmed to the full cap without accounting for persisted occupants backfill never re-offers (live 4. Metadata surfaces downgrade provenance (standing). The three REST PATCH metadata routes and the ACP 5. Also standing, mechanisms verified: Minor, non-blocking: the prettier reflows in sequenceDiagram
participant P1 as Shell tool foreground path
participant P2 as Execution gate
participant P3 as gh pr view attribution
participant P4 as PR sidecar file
participant P5 as Daemon catalog clock
participant P6 as Sidebar badge
P1->>P2: command completed with exit 0
P2->>P3: pre-run snapshot, post-run fetch
P3-->>P1: open PR whose URL is in the output, or nothing
P1->>P4: locked upsert with source create, best-effort
P1->>P5: pr-binding notification marks catalog
P5-->>P6: clients refetch within about 2s
Files changed — grouped overview (43 files)
TestingUnattended CI run — no local build or execution of PR code (triage rules). Evidence below is the PR's own CI on the reviewed commit, fetched via the API; the finalize workflow updates the table region in place if checks move. CI is fully settled and green on this head: every
Skipped checks omitted; Sandboxed verification would settle this: 中文说明代码审查在当前 head 重新审查。由于分支已与堆叠的 #9729 分叉（领先 21 / 落后其 head 29），本次覆盖整个展示 diff——43 个文件、约 1.7k 生产行——而非隔离增量。接线质量高：每个边界都有载荷校验、状态继承有 URL 同一性规则、目录时钟传播、跨进程锁。测试套件可观（约 3.5k 行）。但本次审查不能放行该 PR：review loop 在此 head 上发布的 Critical 发现是真实的——我对照代码验证了其中承重部分，它们在此 head 上仍未解决。 1. PR 正文描述了一个代码已删除的功能。 正文说 backfill 新增第三源（按 part id 配对 2. 实时 hook 归因绕过——一个根因、三条在案发现。 运行前快照不携带仓库或分支身份； 3. sidecar 已有占用时，backfill 重跑会轮转（在案）。 候选按满上限裁剪，未考虑 backfill 从不重新提供的既有占用（实时 4. 元数据写入面降级来源标记（在案）。 三个 REST PATCH 元数据路由与 ACP 5. 其余在案、机制已验证： 次要、不阻断： 时序图展示实时路径：shell 前台命令完成 → 执行闸门 → gh 归因（运行前快照 + 运行后解析）→ 加锁写 sidecar（source create）→ 通知 daemon 目录时钟 → 客户端约 2s 内 refetch → 侧栏 badge。 测试无人值守 CI 运行——不本地构建或执行 PR 代码（triage 规则）。以下为审查提交上 PR 自身 CI 的 API 证据；若检查变动，finalize 工作流会就地更新表格区域。 此 head 上 CI 已全部落定且为绿：所有 沙箱验证可以落定此事： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — the design is right, the wiring is careful, and CI is green on this head; but verified Critical findings stand unresolved on this exact head, and the description no longer matches the code. Stepping back: my independent proposal from the first pass — one execution gate, gh-side attribution via a pre/post snapshot, a best-effort sidecar write, backfill limited to sources with real attribution — is still what is on the branch, and the loop's removal of the forgeable transcript source was the right tightening, not a scope loss I'd contest. I did not find a simpler path the implementation missed. What I cannot sign:
Verdict: defer — comment, no approval. This is not a rejection: the direction and structure are sound, CI is green, and most of what stands is mechanical once the root cause (snapshot identity) is addressed instead of its instances. @wenshao — three things unblock this: the standing findings closed at the root cause (or the autofix loop converging on them), a body rewrite matching the shipped design, and the post-#9729 rebase. Re-run 中文说明置信度：3/5 —— 设计正确、接线谨慎、此 head 上 CI 为绿；但有已验证的 Critical 发现在此 head 上仍未解决，且描述已与代码不符。 退一步看：我在首轮写下的独立方案——一个执行闸门、经运行前后快照的 gh 侧归因、尽力而为的 sidecar 写入、backfill 限定在有真实归因的源——仍是分支上的实现；loop 删除可伪造的 transcript 源是正确的收紧，不是我要反对的范围损失。我没有找到实现漏掉的更简路径。我不能签字的部分：
结论：暂缓——评论、不批准。 这不是拒绝：方向与结构健全，CI 为绿，且在案发现大多在根因（快照身份）被处理而非逐个修实例后即可机械性闭合。@wenshao——三件事可以解锁：以根因闭合在案发现（或 autofix loop 收敛）、正文重写以匹配落地的设计、#9729 合入后的 rebase。完成后重新 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
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.
A search command whose arguments mention 'gh pr create' (e.g. grep) paired with a PR-looking URL in its output would bind a bogus entry; require the phrase to start a command segment (env-prefixed and piped forms still count). Summarized output also elides owner/repo (github.com/.../pull/N) — such URLs are not usable link targets and are rejected.
|
@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.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Review round 2 for the session PR binding feature: - Bind the LAST printed /pull/N URL instead of the first, and require it to belong to the workspace repository (host/owner/repo vs the origin remote); the live shell hook additionally requires exit code 0 and skips when the remote cannot be resolved. Closes forged/foreign URL persistence via compound commands. - Backfill inserts bindings in ascending authority (branch-mapped first, gh-pr-create evidence next, worktree convention last) so the session's own PR survives the tail-10 cap, and maps a shared head branch to the newest PR (slim query now requests updatedAt; first-wins mapping). - Reject pr-0 and leading-zero worktree slugs — number 0 poisoned the whole sidecar read. - Refresh sweep only stamps states onto bindings whose URL belongs to the queried repository, and counts entries actually rewritten. - Remote resolution is async with a bounded timeout and attempted once per run; normalizeRemoteToWebUrl moved to core, drops ssh:// ports, and accepts any scp-style user.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review round summary — PR #9739 (commit
|
| Finding | Disposition | Change |
|---|---|---|
[Critical] R1-2 (rc:3836860789) — detector binds the FIRST /pull/N URL anywhere, no exit-code gate, no attribution |
Fixed | detectGhPrCreateBinding now binds the LAST matching URL and, when given a repo key, only URLs whose host/owner/repo matches it. Live shell hook additionally requires result.exitCode === 0 and resolves the workspace origin remote (fetchRemoteWebUrl) — an unresolvable remote binds nothing. Backfill applies the same repo gate to transcript-recovered URLs. |
| [Critical] R1-3 (rc:3836860792) — insertion order evicts the most authoritative binding first | Fixed | Backfill now inserts in ascending authority: branch-mapped → gh pr create evidence → worktree slug/branch convention last, so the session's own PR survives the tail-10 cap. |
| [Critical] R1-4 (rc:3836860793) — slim field set breaks newest-first ordering; last-write branch map resolves to the oldest PR | Fixed | Both halves: updatedAt added to GH_PR_LIST_FIELDS_SLIM (restores the newest-first sort) and branchToNumber is now first-wins, so a shared head branch maps to the newest PR. |
[Critical] R1-5 (rc:3836860795) — pr-0 slug binds number 0, poisoning the whole sidecar read |
Fixed | Both patterns are now [1-9]\d{0,8} (mirrors parsePRReference's n > 0 invariant; leading zeros rejected too). A pr-0 worktree session binds nothing. |
| [Critical] R1-6 (rc:3836860796) — sweep stamps states by bare number across repositories | Fixed | The sweep resolves the workspace repo key and only stamps bindings whose URL belongs to it; foreign-repo bindings are skipped, and an unresolvable remote updates nothing (fail closed, no gh call). |
[Suggestion] R1-11 (rc:3836860805) — blocking execSync remote lookup, retried per candidate |
Fixed | Remote resolution moved to core fetchRemoteWebUrl (async execFile, 5s timeout) and cached by attempt — one lookup per backfill run even when it fails. |
[Suggestion] R1-19 (rc:3836860817) — ssh:// port kept in badge links; non-git@ scp remotes rejected |
Fixed | normalizeRemoteToWebUrl (moved to core) reassembles with url.hostname (port dropped) and accepts any [user@]host:path scp-style remote. Regression tests added for ssh://git@host:2222/o/r.git and jdoe@host:o/r.git. |
[Suggestion] R1-20 (rc:3836860818) — updated counts gh-confirmed numbers, not rewrites |
Fixed | updateSessionPrStates returns the count of entries actually rewritten; the sweep accumulates it. Two-binding test asserts updated: 1 when gh confirms one change. |
The design doc section describing the shell post-hook was updated to match the hardened semantics.
Deferred to the next round (explicit thread replies posted)
- [Critical] R1-1 (rc:3836860788) — live binding never notifies the bridge, and [Critical] R1-7 (rc:3836860798) — cross-process sidecar writer race. These two share one root-cause fix and are intentionally tackled together: route the child-detected binding over the existing
qwen/notify/*extNotification side-channel (title-update precedent) into a daemon-side handler that performs the GitDialog sequence (seedSessionPrs→bridge.updateSessionMetadata→upsertSessionPr), making the daemon the sole serialized writer and bumping the catalog revision. This spanscore(sink on Config),cli(ACP Session wiring), andacp-bridge(demux + handler) and exceeds this round's budget; the fallback for R1-7 alone is a two-tierproper-lockfileguard (mailbox.ts convention; dependency already present). - Suggestions R1-8, R1-9, R1-10, R1-12, R1-13, R1-14, R1-15, R1-16, R1-17, R1-18, R1-21, R1-22, R1-23 — deferred per the per-round batch bound; each has a reply recording the deferral. R1-23's surviving-mutant list (alreadyBound read-back, out-of-alignment transcript pairing, effectiveEnv fixture,
toHaveBeenCalledWitharg pins, untrusted-primary fixture) is the test plan for those fixes. - rv:5000918652 (review body) — its actionable content is the inline findings above; the disclosed gaps (integration suite not run locally, reverse-audit cap) are process notes, addressed here by the focused verification below.
- ic:5381304346 (web-shell visual preview) — "one or more scenarios failed to render" against the CI mock daemon; no code-level evidence of a defect was available (linked artifacts are CI-side). The badge/merged-state wiring was independently audited and is covered by
SessionPrBadge.test.tsx. If the preview keeps failing on the next head, its workflow artifacts are the place to look. - ic:5381345419 (serve A/B) — passed, no action.
Verification
Commands actually run this round (post-fix, at commit bd968f2b23):
npm run build— passed (also re-run after formatting; 0 errors)npm run typecheck— passed (0 TS errors)npm run lint— passednpx prettier --checkon all 10 changed code files — passed (after--writeon 5)cd packages/core && npx vitest run src/services/session-pr-service.test.ts src/utils/github-prs.test.ts src/tools/shell.test.ts— 384 passed (incl. 5 shell binding tests, 10 detector tests, 20 new helper tests)cd packages/core && npx vitest run src/services/sessionService.test.ts src/utils/atomicFileWrite.test.ts(adjacent suites) — passedcd packages/cli && npx vitest run src/serve/routes/session-pr-backfill.test.ts src/serve/server/session-pr-refresh.test.ts src/serve/server.test.ts src/serve/process-env-guard.test.ts— 1112 passed (20 backfill + 11 refresh tests incl. 8 new)cd packages/web-shell && npx vitest run client/components/SessionPrBadge.test.tsx client/components/dialogs/GitDialog.test.tsx— 18 passed
Mutation probes (each fix's witness verified red on pre-round code, then restored green):
- R1-2 detector reverted to first-match/no-key → 3 detector tests fail
- R1-2 exit-code gate removed → "does not bind a non-zero exit even when the output carries a URL" fails
- R1-2 backfill repo gate removed → "does not recover a transcript URL from another repository" fails
- R1-3 order reverted → "keeps the convention binding when weaker numbers overflow the cap" fails
- R1-4 last-wins mapping restored → "maps a shared head branch to the newest PR" fails
- R1-5 patterns reverted to
\d{1,9}→ both zero/leading-zero tests fail - R1-6 scoping removed → "never stamps a same-number PR of another repository" fails
- R1-11 attempt cache removed → "resolves the git remote at most once per backfill run" fails
- R1-19 normalization reverted (port kept,
git@-only) → both new remote-shape tests fail - R1-20
states.sizecounting restored → "counts only bindings whose state was actually rewritten" fails
Integration tests were not run: every changed behavior is exercised by the focused Vitest suites above (route tests use supertest; the sweep/backfill are tested through their public functions), not only through the bundled CLI.
中文说明
审查轮次总结 — PR #9739（提交 bd968f2b23）
本轮实现了全部五个有界的 Critical 发现（R1-2 … R1-6），以及与它们纠缠在一起的三个 Suggestion（R1-11、R1-19、R1-20）。每一项都附带回归测试，并已验证该测试在轮次前代码上会失败（见下方变异探针）。按照每轮约 8 个发现的上限，其余发现推迟到下一轮，并在各自线程中留下了明确的回复（comment-replies.json）。
已在代码中解决
| 发现 | 处置 | 变更 |
|---|---|---|
[Critical] R1-2（rc:3836860789）——检测器绑定输出中任意位置的第一个 /pull/N URL，无退出码闸门、无归属判定 |
已修复 | detectGhPrCreateBinding 现在绑定最后一个匹配 URL，且在给定仓库 key 时只接受 host/owner/repo 与之匹配的 URL。实时 shell hook 额外要求 result.exitCode === 0，并解析工作区 origin remote（fetchRemoteWebUrl）——remote 无法解析时不做任何绑定。回填对 transcript 恢复出的 URL 应用同样的仓库闸门。 |
| [Critical] R1-3（rc:3836860792）——插入顺序导致最权威的绑定最先被挤出 | 已修复 | 回填现在按权威性升序插入：分支映射 → gh pr create 证据 → worktree slug/分支约定最后，确保会话自身的 PR 在尾部 10 个上限下幸存。 |
| [Critical] R1-4（rc:3836860793）——slim 字段集破坏"最新优先"排序；后写覆盖的分支映射解析到最老 PR | 已修复 | 两步都做：GH_PR_LIST_FIELDS_SLIM 增加 updatedAt（恢复最新优先排序），且 branchToNumber 改为先到优先，使共享 head 分支映射到最新 PR。 |
[Critical] R1-5（rc:3836860795）——pr-0 slug 绑定数字 0，毒害整个 sidecar 读取 |
已修复 | 两个模式均改为 [1-9]\d{0,8}（对齐 parsePRReference 的 n > 0 不变量；同时拒绝前导零）。pr-0 worktree 会话不再产生任何绑定。 |
| [Critical] R1-6（rc:3836860796）——扫描按裸 PR 号跨仓库盖状态 | 已修复 | 扫描解析工作区仓库 key，只对 URL 属于该仓库的绑定盖状态；外部仓库绑定直接跳过；remote 无法解析时本轮不更新任何内容（失败即关闭，不发 gh 调用）。 |
[Suggestion] R1-11（rc:3836860805）——阻塞式 execSync remote 查询，且按候选重试 |
已修复 | remote 解析移入 core fetchRemoteWebUrl（异步 execFile，5 秒超时），并按尝试缓存——即使失败，每次回填运行也只查询一次。 |
[Suggestion] R1-19（rc:3836860817）——ssh:// 端口保留进 badge 链接；非 git@ 的 scp remote 被拒绝 |
已修复 | normalizeRemoteToWebUrl（移入 core）用 url.hostname 重组（丢弃端口），并接受任意 [user@]host:path scp 风格 remote。为 ssh://git@host:2222/o/r.git 与 jdoe@host:o/r.git 补充回归测试。 |
[Suggestion] R1-20（rc:3836860818）——updated 计的是 gh 页面确认过的数字，而非实际重写数 |
已修复 | updateSessionPrStates 返回实际被重写的条目数；扫描累加该值。双绑定测试断言 gh 恰好确认一条变化时 updated: 1。 |
设计文档中描述 shell post-hook 的章节已同步更新为加固后的语义。
推迟到下一轮（线程中已有明确回复）
- [Critical] R1-1（rc:3836860788）——实时绑定从不通知 bridge；[Critical] R1-7（rc:3836860798）——sidecar 跨进程写竞态。这两者共享同一个根因修复，有意放在一起处理：把子进程检测到的绑定经现有
qwen/notify/*extNotification 侧信道（title-update 先例）路由到 daemon 侧处理器，执行 GitDialog 的完整序列（seedSessionPrs→bridge.updateSessionMetadata→upsertSessionPr），让 daemon 成为唯一的串行写入方并 bump catalog revision。该方案横跨core（Config 上的 sink）、cli（ACP Session 接线）、acp-bridge（demux + 处理器），超出本轮预算；R1-7 单独的兜底方案是两层proper-lockfile守卫（mailbox.ts 约定；依赖已存在）。 - Suggestion 类 R1-8、R1-9、R1-10、R1-12、R1-13、R1-14、R1-15、R1-16、R1-17、R1-18、R1-21、R1-22、R1-23——按每轮批量上限推迟；每项都有记录推迟原因的回复。R1-23 的存活变异体清单（alreadyBound 回读、错位配对的 transcript、effectiveEnv fixture、
toHaveBeenCalledWith参数钉死、不可信 primary fixture）就是那些修复的测试计划。 - rv:5000918652（审查正文）——其可执行内容即上述 inline 发现；披露的缺口（集成套件未本地运行、反向审计达到轮数上限）属于流程说明，已由下方的聚焦验证覆盖。
- ic:5381304346（web-shell 可视化预览）——"一个或多个场景渲染失败"（CI mock daemon 环境）；没有可用的代码级缺陷证据（链接产物在 CI 侧）。badge/merged 状态接线已独立审计，且由
SessionPrBadge.test.tsx覆盖。若下个 head 预览仍失败，应查看其工作流产物。 - ic:5381345419（serve A/B）——通过，无需处理。
验证
本轮实际执行的命令（修复后，提交 bd968f2b23）：
npm run build— 通过（格式化后再次执行；0 错误）npm run typecheck— 通过（0 个 TS 错误）npm run lint— 通过- 对全部 10 个改动的代码文件执行
npx prettier --check— 通过（其中 5 个先--write） cd packages/core && npx vitest run src/services/session-pr-service.test.ts src/utils/github-prs.test.ts src/tools/shell.test.ts— 384 通过（含 5 个 shell 绑定测试、10 个检测器测试、20 个新 helper 测试）cd packages/core && npx vitest run src/services/sessionService.test.ts src/utils/atomicFileWrite.test.ts（相邻套件）— 通过cd packages/cli && npx vitest run src/serve/routes/session-pr-backfill.test.ts src/serve/server/session-pr-refresh.test.ts src/serve/server.test.ts src/serve/process-env-guard.test.ts— 1112 通过（20 个回填 + 11 个刷新测试，含 8 个新增）cd packages/web-shell && npx vitest run client/components/SessionPrBadge.test.tsx client/components/dialogs/GitDialog.test.tsx— 18 通过
变异探针（每个修复的见证测试均已验证：轮次前代码上为红，恢复后为绿）：
- R1-2 检测器回退为首个匹配/无 key → 3 个检测器测试失败
- 移除 R1-2 退出码闸门 → "退出码非 0 即使输出带 URL 也不绑定"失败
- 移除 R1-2 回填仓库闸门 → "不恢复来自其他仓库的 transcript URL"失败
- R1-3 顺序回退 → "弱编号溢出上限时保留约定绑定"失败
- R1-4 恢复后写覆盖映射 → "共享 head 分支映射到最新 PR"失败
- R1-5 模式回退为
\d{1,9}→ 两个 zero/前导零测试失败 - 移除 R1-6 仓库限定 → "不给其他仓库的同号码 PR 盖状态"失败
- 移除 R1-11 尝试缓存 → "每次回填运行至多解析一次 git remote"失败
- R1-19 归一化回退（保留端口、仅
git@）→ 两个新 remote 形态测试失败 - R1-20 恢复
states.size计数 → "只统计状态实际被重写的绑定"失败
未运行集成测试：所有变更行为均由上述聚焦 Vitest 套件覆盖（路由测试使用 supertest；扫描/回填经公共函数测试），并非只能通过打包后的 CLI 验证。
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/模型 qwen3.8-max
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.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- timer trust-skip/reentrancy/dispose untested (f7) — named in R1-23 (re-posted on the PR)
- core state validation clause negative test missing (f8) — named in R1-23 (re-posted on the PR)
- draft→open guards untested in both consumers (f9) — named in R1-23 (re-posted on the PR)
- !result.aborted conjunct unexercised (f10) — named in R1-23 (re-posted on the PR)
- backfill gh call-arguments unasserted (g5) — named in R1-23 item (4) (re-posted on the PR)
- sweep archived-state branch untested (g7) — named in R1-23 (re-posted on the PR)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped at the reverse-audit round cap of 5 without converging.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/shell.ts:3076 — [review] Remote resolved before the cheap detection gate — every exit-0 command pays a git walk + spawnpackages/cli/src/serve/routes/session-pr-backfill.test.ts:653 — [review] Route aggregation test asserts neither the trusted entry nor non-zero reduce totalspackages/cli/src/serve/run-qwen-serve.ts:5164 — [probe] Sweep-starting dynamic import has no .catch — uncaught exception (daemon crash) on the serve fast pathpackages/cli/src/serve/routes/session-pr-backfill.test.ts:66 — [probe] pr() helper hardcodes state:'open' — merged/closed snapshot write path unpinned (flattening mutant 20/20 green)packages/cli/src/serve/server/session-pr-refresh.test.ts:179 — [probe] All-merged fast-path test cannot pin early-return-before-remote-resolution (hoisting mutant 11/11 green)packages/cli/src/serve/server/session-pr-refresh.test.ts:211 — [probe] No malformed-sidecar resilience test although R2-2's poisoning chain makes invalid sidecars reachablepackages/cli/src/serve/routes/session-pr-backfill.ts:142 — [probe] Backfill runs the detector unkeyed — a trailing foreign URL after gh's own loses a binding the live keyed path would makepackages/cli/src/serve/server/session-pr-refresh.test.ts:59 — [probe] Negative-minutes guard untested — deleting it keeps the suite green and yields the 1 ms loop shapepackages/cli/src/serve/server/session-pr-refresh.test.ts:160 — [probe] gh call count unpinned — a per-session-call mutant keeps 11/11 green (probe: committed 1 call vs mutant 2)docs/design/2026-08-20-webshell-session-pr-binding.md:65 — [review] Design doc contradicts itself and the code on the slim field set (says three fields; constant has five)packages/cli/src/serve/server/session-pr-refresh.test.ts:181 — [probe] Multi-target update loop never exercised past the first entry — break-after-first mutant 11/11 greenpackages/core/src/services/session-pr-service.ts:163 — [probe] --dry-run substring gate suppresses a real create when the flag appears in a quoted title/body or a later segment
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the reverse-audit round cap of 5 without converging。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 12 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #9739 (review round 3)Growth auditVerdict: drift ( Feedback dispositions
R2-1 — detector attribution class (fixed)Reproduced all five entrances against the committed code (probe output: failed exit-0 compound → binds 1234; supersede echo → binds 42 instead of created 100; quoted phrase / multi-line Fix — gh is the attribution authority:
Witnesses: new shell tests (attribution positive, corner-A flip, three forged shapes, R2-2 — write-time validation (fixed)Reproduced on committed code: a 2130-char same-repo URL and an ESC-bearing URL persisted through R2-3 — cross-run convention eviction (fixed)Fix: in the backfill loop, an already-bound number with convention authority is re-upserted (moved to the end with a fresh R2-5 — fork-layout gh-list gate (fixed)Fix: while building R1-1 — badge never appears in live-state workspaces (fixed)The child persists the sidecar, then notifies the daemon via a new R1-7 — cross-process sidecar races (fixed)Fix: every sidecar mutation ( Deferred suggestions (13)Batch cap (~8 findings/round, Critical first) deferred R2-6, R2-7, R2-8, R2-9, R1-8, R1-10, R1-12, R1-13, R1-14, R1-15, R1-16, R1-21, R1-23 to the next round; each has a reply on its own thread ( Notes
VerificationCommands actually run this round (results):
中文说明Autofix 轮次总结 — PR #9739（评审第 3 轮）增长审计结论：drift（见 反馈处置
R2-1 — 检测器归因类问题（已修复）已对提交代码实测复现全部五个入口（探针输出：退出码为 0 的失败复合命令 → 绑定 1234；覆盖式 echo → 实际创建 100 却绑定 42；引号内短语 / 多行 修复——以 gh 为归因权威：
见证：新增 shell 测试（归因正向、corner A 翻转、三种伪造形态、 R2-2 — 写入时校验（已修复）已在提交代码上复现：2130 字符同仓库 URL 与含 ESC 的 URL 均可经 R2-3 — 跨运行约定号被挤出（已修复）修复：回填循环中，已绑定且属约定权威的号码仍会重新 upsert（移到末位、刷新 R2-5 — fork 布局 gh 列表闸门（已修复）修复：构建 R1-1 — live-state workspace 中 badge 永不出现（已修复）子进程持久化 sidecar 后，经新增的 R1-7 — 跨进程 sidecar 竞态（已修复）修复：所有 sidecar 变更（ 延后的建议（13 条）批次上限（每轮约 8 条、Critical 优先）将 R2-6、R2-7、R2-8、R2-9、R1-8、R1-10、R1-12、R1-13、R1-14、R1-15、R1-16、R1-21、R1-23 延后到下一轮；每条均已在各自线程回复（ 备注
验证本轮实际执行的命令（结果）：
Deferred non-Critical feedbackCritical-only mode is active: the PR's diff grew src 137 / test 450 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 净增长已达 源码 137 / 测试 450 行（预算 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/模型 |
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:
- state-rejection 400 message does not name the
stateconstraint (bridge + route) — already reported on the PR in round 1 (comment 3836860820, thread at routes/session.ts:2177); independently re-detected this round by three auditors
Not reviewed: reverse audit — stopped at the reverse-audit round cap of 5 without converging.
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 6: executed run of packages/cli/src/serve/server/session-pr-refresh.test.ts (no node_modules/dist in review worktree; install+build not feasible in budget); chunk 3: executing the test file (no node_modules in the review worktree; npm ci plus the prerequisite npm run build for vitest's dist guard exceeded the remaining t….
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/bridge.ts:9769 — [review] same-binding no-change check ignores statepackages/cli/src/serve/server/session-pr-refresh.ts:161 — [review] env? option on startSessionPrRefreshTimer is a dead switchpackages/acp-bridge/src/bridge.ts:9682 — [review] state validation clauses untested at all three write gatespackages/cli/src/serve/acp-http/dispatch.ts:2946 — [review] ACP update_metadata state passthrough untestedpackages/cli/src/serve/server/session-pr-refresh.ts:157 — [review] startSessionPrRefreshTimer timer lifecycle untestedpackages/cli/src/serve/routes/session-pr-backfill.ts:255 — [probe] alreadyBound+unresolved double count for unresolvable convention numberspackages/cli/src/serve/routes/session-pr-backfill.ts:117 — [review] backfill pagination loop untestedpackages/cli/src/serve/server/session-pr-refresh.test.ts:179 — [probe] sweep merged-skip test vacuouspackages/cli/src/serve/server/session-pr-refresh.test.ts:113 — [probe] sweep archived half untestedpackages/web-shell/client/components/sidebar/SessionDetailsTooltip.tsx:171 — [probe] tooltip state suffix untestedpackages/cli/src/serve/routes/session-pr-backfill.test.ts:203 — [probe] backfill gh query contract unpinnedpackages/cli/src/serve/routes/session-pr-backfill.test.ts:280 — [probe] non-convention alreadyBound skip untestedpackages/cli/src/serve/server/session-pr-refresh.test.ts:113 — [probe] sweep pagination loop untestedpackages/cli/src/serve/routes/session-pr-backfill.test.ts:280 — [review] 'without rewriting the sidecar' test name contradicts behaviorpackages/cli/src/serve/routes/session-pr-backfill.ts:286 — [review] backfill/sweep iterate listAll() including internal live-conversation runtimespackages/cli/src/serve/server/session-pr-refresh.test.ts:46 — [review] draft→open normalization unpinnedpackages/cli/src/serve/server/session-pr-refresh.test.ts:160 — [review] one-gh-call-per-workspace property unpinnedpackages/web-shell/client/components/SessionPrBadge.test.tsx:50 — [review] badge tests never pin the base layout classpackages/cli/src/serve/routes/session-pr-backfill.test.ts:733 — [probe] route aggregation test never asserts the trusted entrypackages/cli/src/serve/server/session-pr-refresh.ts:88 — [probe] sweep read-failure isolation guard unpinned- …and 2 more (see the run report)
Convergence: round 3 posted 25 inline comment(s), 14 of them reported for the first time; the previous round posted 20 (9 new). Findings keep coming back to the same files: packages/cli/src/serve/routes/session-pr-backfill.ts (findings in rounds 1, 2; 5 more now); packages/cli/src/serve/routes/session-pr-backfill.test.ts (findings in rounds 1, 2; 2 more now); packages/core/src/tools/shell.ts (findings in rounds 1, 2; 2 more now), and 2 more file(s). 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, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had 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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — stopped at the reverse-audit round cap of 5 without converging。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 6：executed run of packages/cli/src/serve/server/session-pr-refresh.test.ts (no node_modules/dist in review worktree; install+build not feasible in budget)；chunk 3：executing the test file (no node_modules in the review worktree; npm ci plus the prerequisite npm run build for vitest's dist guard exceeded the remaining t…。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 22 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 25 条行内评论，其中 14 条是首次提出；上一轮发布了 20 条（其中 9 条首次提出）。发现反复回到同一批文件：packages/cli/src/serve/routes/session-pr-backfill.ts（第 1、2 轮已出过发现，本轮又有 5 条）；packages/cli/src/serve/routes/session-pr-backfill.test.ts（第 1、2 轮已出过发现，本轮又有 2 条）；packages/core/src/tools/shell.ts（第 1、2 轮已出过发现，本轮又有 2 条），另有 2 个文件。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2436 passed · 0 failed · 2436 total Flakiness gate: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2436 通过 · 0 失败 · 2436 总计 抖动门： Verification reportPR #9739 Deep Verification —
|
| # | Cell | Arm | Oracle (sidecar file / notification) | Result |
|---|---|---|---|---|
| 1 | gh pr create --title x --body y |
head | [{number:42, url:…/pull/42, state:'open', source:'create'}], 1 notification |
bound ✅ |
| 1 | same | base | no sidecar, no notification | absent ✅ |
| 2 | retry: gh pr create --fill || gh pr view … with PR 41 pre-existing |
head | no sidecar (output carried …/pull/41, exit 0) |
declined ✅ |
| 2 | same | base | no sidecar | absent ✅ |
| 3 | pre-run snapshot errors (rate-limit shape), create succeeds | head | no sidecar — fail closed despite a real PR | declined ✅ |
| 4 | gh pr create --dry-run (URL-shaped output, nothing created) |
head | no sidecar | declined ✅ |
| 5 | create exits 1 | head | no sidecar (non-zero exit gate) | declined ✅ |
| 6 | grep -rn 'gh pr create' notes.txt with a PR URL in output |
head | no sidecar (phrase is an argument, not a segment start) | declined ✅ |
| 7 | gh pr view 41 (non-create) |
head | no sidecar | declined ✅ |
| 8 | create succeeds but > /dev/null hides the URL |
head | no sidecar (output must carry gh's URL) | declined ✅ |
| 9 | static: base dist shell.js |
base | no bindGhPrCreate/commandRunsGhPrCreate symbols |
absent ✅ |
16/16 scripted checks passed (ab-matrix.mjs).
Secondary claim — backfill route (new at head; absent at base by git cat-file): 21/21 checks passed (backfill-harness.mjs): /review 42 → source:'review', state:'open'; /review <same-repo url> binds; worktree slug pr-7 → source:'worktree', state:'merged'; cap/eviction (11 reviews + convention → 10 persisted, convention survives, oldest reviews evicted first; re-run idempotent with alreadyBound); fork layout binds the PARENT repo URL, not a synthesized fork URL, and accepts /review <parent-url> while rejecting a third repo's URL; no origin → fail closed (bound:0, unresolved:2); transcript gh pr create call/response pairs bind nothing (source removed); forged traversal sessionId in a transcript's first record is rejected before path construction; pr-0/pr-007 slugs rejected; base tree has neither the route file nor the backfill-prs registration.
Corrections (to the PR description)
- The body's "Retroactive" paragraph and Reviewer Test Plan step "Re-run
POST /sessions/backfill-prson a workspace with old transcripts containinggh pr createruns and confirmboundcounts them" describe a source that does not exist at head. The backfill route header states transcriptgh pr createtraces are "deliberately NOT a source" (no gh-side attribution per historical command; text alone could forge a binding), the design doc records the removal, andsession-pr-backfill.test.tspins it ("does not bind PRs from transcript gh pr create traces (source removed)"). The removal is a deliberate security hardening across review rounds — but the description still advertises the old behavior and the test plan's step is structurally unsatisfiable. This is a description correction, not a code-change request. - Likewise, the body's "extracts the PR URL that gh prints on success" understates the final mechanism: attribution is gh-resolved (
fetchCurrentBranchPullRequestbefore and after the run), with an errored pre-run snapshot failing closed and pre-existing PRs declined — strictly stronger than the text says.
Findings
F1 — gh pr new (documented alias) bypasses the live binding hook — Low
commandRunsGhPrCreate gates on gh(?:\.exe)?\s+pr\s+create\b; gh pr new — which this same file's attribution rewriter explicitly treats as an alias of gh pr create (shell.ts:894-895, 4939) — never enters the hook. Measured on head with a working create (URL in output, exit 0): no sidecar, no notification. Consequence is a false negative only (missing badge; no bad data), and the retroactive path no longer exists to recover it.
Reproduce:
node tmp/pr9739-verify-20260825-091359/run-cell.mjs # CELL_COMMAND='gh pr new --title x --body y', see logs/ (cell printed sidecar: null)Measured candidate fix (one line)
packages/core/src/services/session-pr-service.ts:
const GH_PR_CREATE_SEGMENT_PATTERN =
- /^\s*(?:[A-Za-z_][A-Za-z0-9_]*=\S+\s+)*gh(?:\.exe)?\s+pr\s+create\b/;
+ /^\s*(?:[A-Za-z_][A-Za-z0-9_]*=\S+\s+)*gh(?:\.exe)?\s+pr\s+(?:create|new)\b/;Applied in a scratch worktree, rebuilt core, re-driven through the same wire harness:
gh pr new --title x --body y→ binds[{number:42, url:…/pull/42, state:'open', source:'create'}]gh pr create --title x --body y→ byte-identical binding shape (zero collateral)grep -rn 'gh pr create' …with a PR URL in output → still no binding (anchor intact)commandRunsGhPrCreatesuite: 4/4 green both with and without the patch — the suite pins nothing along the alias axis, so the fix should ship with a fixture such asexpect(commandRunsGhPrCreate('gh pr new --fill')).toBe(true)beside the existing bare-segment case.
F2 — PR body / Reviewer Test Plan describe the removed transcript backfill source — Medium (docs)
See Corrections. A maintainer following the test plan would spend time on step 3 and conclude a regression where there is an intentional removal. Recommend updating the PR body before merge (the design doc is already correct).
Not covered
- Per-commit attribution: the merge-ref checkout is depth 2 —
git rev-list HEAD^1..HEAD^2returns 1 locally while the metadata snapshot lists 22 commits, so individual review-round commits were not exercised separately; the aggregateHEAD^1..HEADdiff is what was verified. - Full daemon E2E (badge appearing in a live Web Shell within ~2 s): the two halves were verified separately — agent-side sidecar write +
qwen/notify/session/pr-bindingemission (wire harness) and bridge-side payload validation + catalog mark (784-testbridge.test.tsgate, incl. +226 new pr-binding tests) — but no real daemon + client session was driven. - Refresh sweep timing (5-minute timer, first-run delay): unit-gated (
session-pr-refresh.test.ts, incl. env-knob resolution and overflow clamp), not observed against a live clock. Session.test.tsfull suite (huge; its +3 lines are asetSessionPrBoundCallbackmock stub) — the siblingSession.review-lease/Session.worktreesuites with the identical stub change were run green.- Repo-wide lint/typecheck/test gates were not claimed; the environment's pre-verification build (tsc emit) at head is the type evidence, and only the affected workspaces' suites were run. One base-side rebuild emitted under a pre-existing
@lydell/node-ptyTS7016 type-declaration notice (environmental: same import on both arms; emit unaffected, modules load). - Windows/Linux-specific paths (author declares none; no OS conditionals found in the changed surface).
Methodology
All harnesses ran against compiled dist output (head: CI-built at the merge commit; base: packages/core rebuilt in a tmp/base-tree worktree at HEAD^1, reusing root node_modules — clean control because the PR changes no package.json/lockfile, and the base dist was asserted free of the PR's symbols and cross-workspace imports). The live-hook harness (run-cell.mjs) drives the real ShellToolInvocation/ShellExecutionService/SessionService against a scratch git repo per cell; gh is a shim (fake-gh) implementing real semantics (create persists state; view resolves it; error/absent modes), so pre/post attribution calls go through the actual execFile path. Config stubs exist only at the telemetry/permission boundary surface the foreground cells never exercise. The backfill harness drives the real backfillWorkspaceSessionPrs with real transcript/worktree fixtures and a real gh pr list shim; membership, path layout, and isValidSessionId gates all run unmocked. Mutations were applied as single-hunk source edits in a separate worktree and run through the repo's vitest (mutation-matrix.mjs), each reverted afterwards (worktree clean asserted). Gates: npx vitest run per affected file from each package. Raw logs in logs/; rerunnable harnesses: ab-matrix.mjs, backfill-harness.mjs, mutation-matrix.mjs, gates-summary.sh.
Flakiness gate log
rounds=5 files=16 skipped=0
file packages/acp-bridge/src/bridge.test.ts: (cd packages/acp-bridge) npx --no-install vitest run ./src/bridge.test.ts
file packages/cli/src/acp-integration/session/Session.review-lease.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/Session.review-lease.test.ts
file packages/cli/src/acp-integration/session/Session.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/Session.test.ts
file packages/cli/src/acp-integration/session/Session.worktree.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/Session.worktree.test.ts
file packages/cli/src/serve/fast-path-open.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/fast-path-open.test.ts
file packages/cli/src/serve/process-env-guard.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/process-env-guard.test.ts
file packages/cli/src/serve/routes/session-pr-backfill.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/routes/session-pr-backfill.test.ts
file packages/cli/src/serve/server.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/server.test.ts
file packages/cli/src/serve/server/session-pr-refresh.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/server/session-pr-refresh.test.ts
file packages/core/src/config/config-session-env.test.ts: (cd packages/core) npx --no-install vitest run ./src/config/config-session-env.test.ts
file packages/core/src/config/config.test.ts: (cd packages/core) npx --no-install vitest run ./src/config/config.test.ts
file packages/core/src/services/session-pr-service.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/session-pr-service.test.ts
file packages/core/src/tools/shell.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/shell.test.ts
file packages/core/src/utils/github-prs.test.ts: (cd packages/core) npx --no-install vitest run ./src/utils/github-prs.test.ts
file packages/web-shell/client/components/SessionPrBadge.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/SessionPrBadge.test.tsx
file packages/web-shell/client/components/dialogs/GitDialog.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/dialogs/GitDialog.test.tsx
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/acp-bridge/src/bridge.test.ts: PPP
packages/cli/src/acp-integration/session/Session.review-lease.test.ts: PPP
packages/cli/src/acp-integration/session/Session.test.ts: PPP
packages/cli/src/acp-integration/session/Session.worktree.test.ts: PPP
packages/cli/src/serve/fast-path-open.test.ts: PPP
packages/cli/src/serve/process-env-guard.test.ts: PPP
packages/cli/src/serve/routes/session-pr-backfill.test.ts: PPP
packages/cli/src/serve/server.test.ts: PPP
packages/cli/src/serve/server/session-pr-refresh.test.ts: PPP
packages/core/src/config/config-session-env.test.ts: PPP
packages/core/src/config/config.test.ts: PPP
packages/core/src/services/session-pr-service.test.ts: PPP
packages/core/src/tools/shell.test.ts: PPP
packages/core/src/utils/github-prs.test.ts: PPP
packages/web-shell/client/components/SessionPrBadge.test.tsx: PP
packages/web-shell/client/components/dialogs/GitDialog.test.tsx: PP
verdict: timeout
summary: only 2 of 5 rounds fit the 15-minute budget; the completed rounds agreed
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/Session.review-lease.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/Session.worktree.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/fast-path-open.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/routes/session-pr-backfill.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/server/session-pr-refresh.test.ts: P (exit 0)
round 1 · packages/core/src/config/config-session-env.test.ts: P (exit 0)
round 1 · packages/core/src/config/config.test.ts: P (exit 0)
round 1 · packages/core/src/services/session-pr-service.test.ts: P (exit 0)
round 1 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 1 · packages/core/src/utils/github-prs.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/SessionPrBadge.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/components/dialogs/GitDialog.test.tsx: P (exit 0)
round 2 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/session/Session.review-lease.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/session/Session.worktree.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/fast-path-open.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/routes/session-pr-backfill.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/server/session-pr-refresh.test.ts: P (exit 0)
round 2 · packages/core/src/config/config-session-env.test.ts: P (exit 0)
round 2 · packages/core/src/config/config.test.ts: P (exit 0)
round 2 · packages/core/src/services/session-pr-service.test.ts: P (exit 0)
round 2 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 2 · packages/core/src/utils/github-prs.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/SessionPrBadge.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/components/dialogs/GitDialog.test.tsx: P (exit 0)
round 3 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/session/Session.review-lease.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/session/Session.worktree.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/fast-path-open.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/routes/session-pr-backfill.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/server/session-pr-refresh.test.ts: P (exit 0)
round 3 · packages/core/src/config/config-session-env.test.ts: P (exit 0)
round 3 · packages/core/src/config/config.test.ts: P (exit 0)
round 3 · packages/core/src/services/session-pr-service.test.ts: P (exit 0)
round 3 · packages/core/src/tools/shell.test.ts: P (exit 0)
round 3 · packages/core/src/utils/github-prs.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summaryBudget warning in effect: the previous round expired without completing, so this round addressed the smallest blocking subset — the outstanding Critical findings verified at the current head — committed as soon as complete, and deferred every other unresolved item explicitly (see Resolved this round (commit 2c2c22c)All seven are round-11 Criticals re-verified at this head; each fix has a focused witness test, and every guard was mutation-probed (removed → witness fails → restored):
Also landed: the R10-12 JSDoc correction (the binder doc no longer claims "the number did not already exist before the run"; it states the per-session scope of the proof and discloses the shared-checkout window). Re-verified as already fixed by prior commits (resolved, no new code)
Partially addressed
Deferred to the next round122 unresolved comments (Critical and Suggestion findings from rounds 1–10 that round 11 did not re-post) each received a thread reply deferring them to the next round for re-verification against the new head. Two pre-existing cap-contract tests were updated because R10-6's fix supersedes their old "evict seeded occupants to land every new binding" expectation — the new contract never evicts persisted occupants from backfill. Conflict notesNone — VerificationCommands actually run this round (all after the final code state unless noted):
Integration tests were not run: the touched behavior is covered by the unit suites above, matching the skipped integration lanes of prior pushes. Settings sources were not changed, so no schema regeneration was needed. 中文说明Autofix 审查轮次总结预算警告生效中：上一轮超时未完成，因此本轮处理了最小的阻塞子集——在当前 HEAD 上已核实的未解决 Critical 发现——完成后立即提交，并将其余所有未解决项逐一显式推迟（见 本轮已解决（提交 2c2c22c）以下七项均为第 11 轮在当前 HEAD 上重新核实的 Critical；每项修复都有针对性的见证测试，且每个守卫都做了变异探测（移除守卫 → 见证测试失败 → 恢复守卫）：
同时落地：R10-12 的 JSDoc 更正（绑定端文档不再声称"该编号在运行前不存在"，而是说明证明的按会话范围并披露共享 checkout 的窗口局限）。 经重新核实已由先前提交修复（已解决，无新代码）
部分处理
推迟到下一轮122 条未解决评论（第 1–10 轮的 Critical 与 Suggestion 发现，第 11 轮未重新张贴）各收到一条线程回复，推迟到下一轮针对新的 HEAD 重新验证。两个既有的上限契约测试已更新，因为 R10-6 的修复取代了它们旧的"驱逐已播种占用项以容纳全部新绑定"的预期——新契约永不在回填中驱逐已持久化的占用项。 冲突说明无—— 验证本轮实际运行的命令（除注明外均为最终代码状态）：
未运行集成测试：本次触及的行为已由上述单元测试覆盖，与先前推送跳过的集成通道一致。未改动设置源，因此无需重新生成 schema。 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/模型 |
…eate # Conflicts: # .github/workflows/.size-baseline
… feat/session-pr-gh-create # Conflicts: # packages/core/src/services/sessionService.ts
Merge of main's persisted-session lifecycle (QwenLM#9626/QwenLM#9764) with the locked sidecar-move refactor: moveSessionPrSidecar gains the optional generation fence (threaded to writeSessionPrs assertCanCommit), conflict-repair paths merge split PR sidecars under the lock, and the fence tests assert the fence rides on the locked move.
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: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the Windows-specific parsing this review probed (drive-letter remotes) is not exercised on that OS here.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": verifying whether teammate-bound envelope text delivered via TeamManager.flushNextMessage reaches the recorded user-role request unwrapped (decides reachability…; "agent reverse-audit (round 2)": verifying ACP-session transcripts persist to the same workspace chats dir backfill scans (storage root of the Session.ts recording service).
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
docs/design/2026-08-20-webshell-session-pr-binding.md:39 — [probe] design doc attributes retry-misbinding protection to the open gate; the snapshot comparison is what blocks itpackages/acp-bridge/src/bridge.ts:9869 (+2 locations) — [review] state-validation rejection messages omit the state constraintpackages/acp-bridge/src/bridge.ts:9961 — [probe] no-op suppression is latest-only; byte-identical re-bind of a non-latest entry still bumps the catalogpackages/cli/src/acp-integration/session/Session.ts:8246 (+13 locations) — [probe] 13 new behavior branches have no mutation-pinning test (notification wiring, source stamping, failure isolation, state rejection/round-trip, read-side enum g…packages/cli/src/serve/routes/session-pr-backfill.ts:174 — [review] PR description still claims the removed transcript-pairing backfill source; test-plan step will failpackages/cli/src/serve/routes/session-pr-backfill.ts:390 — [probe] free-slot offer sizing ignores already-bound candidates; fresh numbers permanently unbound, re-runs never convergepackages/cli/src/serve/routes/session.ts:2262 (+2 locations) — [review] explicit state:null now rejected (previously ignored); generated clients' combined metadata update droppedpackages/cli/src/serve/run-qwen-serve.ts:5191 — [probe] fire-and-forget dynamic import without .catch; rejection kills the daemon under Node >=22packages/cli/src/serve/server/session-list.ts:535 — [probe] at-cap live-only drop heuristic misses the bind-route pre-sidecar-write window; fresh binding invisible at the cappackages/cli/src/serve/server/session-pr-refresh.ts:47 — [probe] interval resolver has no lower bound; sub-minute values spawn the gh busy loop the upper guard exists to preventpackages/cli/src/serve/server/session-pr-refresh.ts:85 — [review] sweep enumerates every transcript to find the handful with sidecars; enumerate *.pr.json insteadpackages/cli/src/serve/server/session-pr-refresh.ts:161 — [probe] sweep has no per-session failure isolation; one bad sidecar aborts the workspace and loses the catalog bump for rewrites already appliedpackages/core/src/services/session-pr-service.test.ts:154 (+2 locations) — [probe] foreign-lock test doesn't pin read-inside-lock; batch concurrency test's scenario is structurally unreachablepackages/core/src/services/session-pr-service.ts:155 — [probe] class-level: quote-blind hand-rolled shell segmentation — wrappers hide real creates (binding permanently lost), quoted separators forge matchespackages/core/src/services/session-pr-service.ts:156 — [probe] execution gate misses gh's documented gh pr new alias while the sibling attribution rewriter handles itpackages/core/src/services/session-pr-service.ts:339 — [probe] fresh binding with explicit source:'review' loses provenance to the phantom undefined rank (1 > 0)packages/core/src/services/session-pr-service.ts:340 — [probe] source inheritance crosses a URL change — a different repo's PR inherits the replaced binding's provenance rankpackages/core/src/services/sessionService.ts:2023 — [probe] enumerateSessionIdsForArchiveState silently truncates at 10000 despite the FULL-set docstring; sibling surfaces a truncated flagpackages/core/src/tools/shell.ts:2758 — [review] binding hook fires only on the foreground settle path; background and promoted creates never bind (background git commit IS refused for this exact reason)packages/core/src/tools/shell.ts:3136 — [review] new comment promises 'backfill recovers the rest' but this diff removed the only source that could- …and 3 more (see the run report)
Convergence: round 12 posted 7 inline comment(s), 7 of them reported for the first time; the previous round posted 7 (3 new). Findings keep coming back to the same files: packages/cli/src/serve/routes/session-pr-backfill.ts (findings in round 10; 2 more now); packages/core/src/services/session-pr-service.ts (findings in round 10; 2 more now); packages/core/src/utils/github-prs.ts (findings in round 11; 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。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the Windows-specific parsing this review probed (drive-letter remotes) is not exercised on that OS here。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：verifying whether teammate-bound envelope text delivered via TeamManager.flushNextMessage reaches the recorded user-role request unwrapped (decides reachability…；"agent reverse-audit (round 2)"：verifying ACP-session transcripts persist to the same workspace chats dir backfill scans (storage root of the Session.ts recording service)。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
收敛姿态下延后（第 12 轮，非阻断）——已记录，本轮不要求修改：共 23 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 12 轮发布了 7 条行内评论，其中 7 条是首次提出；上一轮发布了 7 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/cli/src/serve/routes/session-pr-backfill.ts（第 10 轮已出过发现，本轮又有 2 条）；packages/core/src/services/session-pr-service.ts（第 10 轮已出过发现，本轮又有 2 条）；packages/core/src/utils/github-prs.ts（第 11 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| entry.prs = ( | ||
| known && known.url === bound.url | ||
| ? existing.map((p) => (p.number === bound.number ? merged : p)) | ||
| : [...existing.filter((p) => p.number !== bound.number), merged] | ||
| ).slice(-SESSION_PR_LIST_LIMIT); |
There was a problem hiding this comment.
[Critical] The in-memory catalog cap here stays positional (.slice(-SESSION_PR_LIST_LIMIT)), while this PR rewrites the persisted sidecar cap from the same positional slice to capSessionPrListByAuthority (provenance-ranked). Both stores are maintained by the same metadata request, so once 11 distinct PR numbers accumulate they evict different entries and diverge for the daemon entry's lifetime — SessionPrInfo carries no source, so this live list cannot reproduce the authority cap. Concrete trigger: a session accumulates its own created PR #1 (source:'create', rank 2) at position 0 plus 9 reviewed PRs #2–#10 (rank 0); an 11th number bound via PATCH /session/:id/metadata or ACP update_metadata makes this merge evict #1 positionally (live=[#2..#11]) while upsertSessionPr evicts #2 by authority (persisted keeps #1). Every later session_metadata_updated event publishes prs: entry.prs containing #2 (gone from disk) and missing #1 (present); rename-only PATCH responses echo the same diverged list; seedSessionPrs only hydrates EMPTY entries, so nothing reconciles until daemon restart — event-stream SDK consumers and rename responses serve the wrong bindings indefinitely, directly defeating the new cap's documented invariant that created and convention bindings survive an accumulation of reviewed numbers.
witness (probe driving the real bridge + real upsertSessionPr over a seeded list [create #1 at pos 0, reviews #2–#10] plus an 11th bind):
LIVE evicted=[1]; numbers=[2..11]
PERSIST evicted=[2]; numbers=[1,3..11]
DIVERGED: true
Flip (carry source, cap by authority): LIVE evicted=[2]; numbers=[1,3..11], DIVERGED: false.
Make the two stores share one eviction policy: after upsertSessionPr succeeds in the REST/dispatch metadata routes, replace the bridge entry's prs with the authoritative persisted list (a seed variant that overwrites), or carry source into SessionPrInfo and apply capSessionPrListByAuthority here.
中文说明
此处内存 catalog 的上限仍是按位置截断（.slice(-SESSION_PR_LIST_LIMIT)），而本 PR 已把持久化 sidecar 的上限从同样的按位置截断改为按来源权威排序的 capSessionPrListByAuthority。两个存储由同一个元数据请求维护，一旦累积超过 11 个不同的 PR 编号，两者会逐出不同的条目并在 daemon 生命周期内永久分叉——SessionPrInfo 不携带 source，此处的活列表无法复现权威上限。具体触发：会话先积累自己创建的 PR #1（source:'create'，rank 2，位置 0）和 9 个 review 绑定的 PR #2–#10（rank 0）；第 11 个编号经 PATCH /session/:id/metadata 或 ACP update_metadata 绑定时，此合并按位置逐出 #1（live=[#2..#11]），而 upsertSessionPr 按权威逐出 #2（持久化保留 #1）。此后每个 session_metadata_updated 事件都会发布包含 #2（磁盘上已不存在）、缺失 #1（磁盘上存在）的 prs: entry.prs；仅改名的 PATCH 响应也回显同一分叉列表；seedSessionPrs 只填充空条目，直到 daemon 重启前没有任何机制能对账——事件流 SDK 消费者与改名响应会无限期提供错误绑定，直接破坏新上限“创建与约定绑定应在 review 编号积累中幸存”的既定不变式。探针驱动真实 bridge 与真实 upsertSessionPr（播种 [创建 #1 位于位置 0，review #2–#10] 后绑定第 11 个）：LIVE evicted=[1]; numbers=[2..11]，PERSIST evicted=[2]; numbers=[1,3..11]，DIVERGED: true；修复（携带 source 并按权威截断）后 DIVERGED: false。建议让两个存储共享同一逐出策略：REST/dispatch 元数据路由在 upsertSessionPr 成功后，用权威的持久化列表覆盖 bridge 条目的 prs；或让 SessionPrInfo 携带 source 并在此处应用 capSessionPrListByAuthority。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const { parent } = await fetchRepoKeys( | ||
| runtime.workspaceCwd, | ||
| runtime.env.effectiveEnv, | ||
| ); | ||
| pageMapTrusted = parent === pageRepoKey; |
There was a problem hiding this comment.
[Critical] This fork-parent confirmation for a divergent gh page ignores resolved, so a gh resolution diverged to ANY fork of the page's repo marks an unrelated page trusted. fetchAttributionRepoKeys returns resolved (the repo gh itself resolved), but production never reads it — the comment above this gate says divergent resolution must never feed bindings. Concrete trigger: workspace origin resolves to repo W (key known); a prior gh repo set-default someone/their-fork in this checkout makes gh's resolution diverge to an unrelated repo R that is a fork of P. gh pr list then lists parent P's PRs (pageRepoKey=P≠W); fetchRepoKeys returns {resolved:R, parent:P}; parent === pageRepoKey is true → pageMapTrusted=true. A worktree-convention session (slug pr-42) then binds P's PR 42 — a stranger's PR via pageUrlByNumber, the exact outcome the numberToUrl fail-closed gate exists to prevent; a /review https://W/pull/5 form also gets P's PR-5 state stamped, and the refresh sweep keys stamps by the binding-URL repo key, so the wrong state is never corrected. The fork-layout test passes only because its mock supplies resolved matching the workspace key — a field production never consumes.
witness (probe, workspace origin https://github.com/me/workspace, mocked fetchAttributionRepoKeys → {resolved:'github.com/someone/their-fork', parent:'github.com/parent/repo'}, page listing https://github.com/parent/repo/pull/42):
persisted binding url = "https://github.com/parent/repo/pull/42" (expected fail-closed fallback https://github.com/me/workspace/pull/42)
Flip: requiring resolved === workspaceRepoKey makes the probe pass and the whole existing session-pr-backfill.test.ts suite (incl. the fork-layout test) still passes.
| const { parent } = await fetchRepoKeys( | |
| runtime.workspaceCwd, | |
| runtime.env.effectiveEnv, | |
| ); | |
| pageMapTrusted = parent === pageRepoKey; | |
| const { resolved, parent } = await fetchRepoKeys( | |
| runtime.workspaceCwd, | |
| runtime.env.effectiveEnv, | |
| ); | |
| pageMapTrusted = | |
| workspaceRepoKey !== undefined && | |
| resolved === workspaceRepoKey && | |
| parent === pageRepoKey; |
中文说明
此处分叉页面（divergent page）的“fork 父仓库确认”忽略了 resolved，导致 gh 解析偏移到页面仓库的任意 fork 时，一个无关页面也会被标记为可信。fetchAttributionRepoKeys 返回 resolved（gh 自身解析出的仓库），但生产代码从未读取它——而此门上方的注释明确说偏移的解析绝不能用于绑定。具体触发：工作区 origin 解析为仓库 W（key 已知）；本检出中先前的 gh repo set-default someone/their-fork 使 gh 解析偏移到无关仓库 R，而 R 恰好是 P 的 fork。gh pr list 列出父仓库 P 的 PR（pageRepoKey=P≠W）；fetchRepoKeys 返回 {resolved:R, parent:P}；parent === pageRepoKey 为真 → pageMapTrusted=true。随后一个 worktree 约定会话（slug pr-42）会经 pageUrlByNumber 绑定 P 的 PR 42——陌生人的 PR，这正是 numberToUrl fail-closed 门要防止的结果；/review https://W/pull/5 形式也会被打上 P 的 PR 5 的状态，且刷新扫描按绑定 URL 的仓库 key 打标，错误状态永不会纠正。fork 布局测试之所以通过，仅因其 mock 提供了与工作区 key 一致的 resolved——而生产代码根本不消费该字段。探针（工作区 origin https://github.com/me/workspace，mock fetchAttributionRepoKeys → {resolved:'github.com/someone/their-fork', parent:'github.com/parent/repo'}，页面列出 https://github.com/parent/repo/pull/42）：持久化绑定 url = https://github.com/parent/repo/pull/42（期望 fail-closed 回退 https://github.com/me/workspace/pull/42）；修复（要求 resolved === workspaceRepoKey）后探针通过且现有 backfill 全套测试仍绿。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const formUrlByNumber = new Map<number, string>( | ||
| candidate.reviewedUrlForms.map((form) => [form.number, form.url]), | ||
| ); |
There was a problem hiding this comment.
[Critical] formUrlByNumber is built from ALL transcript URL forms, not the repo-gated subset, so a gate-rejected foreign-repo form can supply the URL for a legitimately bound number (it is consulted in the resolution chain before the remote fallback, and the Map constructor keeps the LAST entry per number). Concrete trigger: transcript has /review 42 (bare, legitimate) and /review https://github.com/other-org/repoB/pull/42 (foreign). Number 42 enters numbers via the bare form; the foreign form fails the allowedRepoKeys gate so adds no number — but formUrlByNumber.get(42) still returns the foreign URL. With gh unavailable, the number outside the 500-window, or a fork layout (numberToUrl empty), the binding persists {number:42, url:repoB's URL} instead of the correct ${remote}/pull/42 fallback — a stranger's PR linked under the workspace's number, contradicting the comment above this map ("already repo-gated"). Variant: two same-number forms where only the first passes the gate — the Map keeps LAST, so the foreign URL wins. The existing test covers only the foreign-form-only case.
witness (probe, gh unavailable kind:'cli_unavailable', transcript /review 42 + /review https://github.com/other-org/repoB/pull/42 --comment):
persisted url = "https://github.com/other-org/repoB/pull/42" (expected workspace fallback https://github.com/o/r/pull/42)
variant (own-repo form first, foreign second, same number): foreign URL wins via Map last-entry
Flip: passing only the repo-gated form subset makes both probes pass with the full existing backfill suite still green.
Gate the URL map: build formUrlByNumber only from forms whose repoKeyFromWebUrl(form.url) is in the allowed set — pass allowedRepoKeys into bindCandidateNumbers via sources, or store the gated form URL alongside the number when the candidate loop accepts it.
中文说明
formUrlByNumber 由全部 transcript URL 形式构建，而非仅经仓库门筛选的子集，因此被门拒绝的外仓库形式仍可为合法绑定的编号提供 URL（它在解析链中先于 remote 回退被查询，且 Map 构造器对同一 key 保留最后一个条目）。具体触发：transcript 中有 /review 42（裸编号，合法）和 /review https://github.com/other-org/repoB/pull/42（外仓库）。编号 42 经裸形式进入 numbers；外形式未过 allowedRepoKeys 门故不贡献编号——但 formUrlByNumber.get(42) 仍返回外仓库 URL。当 gh 不可用、编号超出 500 窗口或 fork 布局（numberToUrl 为空）时，绑定会持久化为 {number:42, url:repoB 的 URL} 而非正确的 ${remote}/pull/42 回退——工作区编号下挂了陌生人的 PR，与此 map 上方注释（“已按仓库过滤”）矛盾。变体：同号两个形式且仅第一个过门——Map 保留最后一个，外仓库 URL 胜出。现有测试仅覆盖“只有外形式”的情形。探针（gh 不可用，上述 transcript）：持久化 url = https://github.com/other-org/repoB/pull/42（期望工作区回退 https://github.com/o/r/pull/42）；修复（仅传过门的子集）后两个探针均通过且现有 backfill 全套测试仍绿。建议：只用 repoKeyFromWebUrl(form.url) 在允许集合内的形式构建 formUrlByNumber——经 sources 把 allowedRepoKeys 传入 bindCandidateNumbers，或在候选循环接受编号时一并保存过门的形式 URL。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| return liveEntry | ||
| ? { | ||
| ...liveEntry, | ||
| ...(p.state !== undefined ? { state: p.state } : {}), | ||
| } | ||
| : p; |
There was a problem hiding this comment.
[Critical] This merge overlays the persisted sidecar state onto the live entry keyed on number alone, with no p.url === liveEntry.url check — re-introducing exactly the cross-repo state poisoning this diff's three writers each guard against (upsertSessionPr conditions state carry-over on URL identity; updateSessionPrStates verifies the URL inside the lock; the bridge preserves state only when the URL is unchanged — each documenting "the same number in another repository is another PR"). Concrete trigger: a session binds repoA #5; the refresh sweep stamps state:'merged' in the sidecar. The user re-binds #5 to repoB's PR (fork layouts share numbers — this diff's own comment says so). The bind route bumps the catalog BEFORE awaiting the sidecar write; a list render in that window — or a sidecar write that throws after the bridge mutation was published — merges live {5, repoB-url} with persisted {state:'merged'} resolved for repoA #5: repoB's OPEN PR renders dimmed/"Merged". The sweep treats 'merged' as terminal and never re-queries the entry, so when the write failed the poisoning persists for the daemon lifetime.
witness (probe through the real listWorkspaceSessionsForResponse, persisted {5, repo-a, state:'merged'}, live {5, repo-b}):
PR: MERGED prs=[{"number":5,"url":"https://github.com/repo-b/r/pull/5","state":"merged"}]
— repoA's terminal state stamped onto repoB's URL
flip (state overlay conditioned on p.url === liveEntry.url):
prs=[{"number":5,"url":"https://github.com/repo-b/r/pull/5"}] — no state
| return liveEntry | |
| ? { | |
| ...liveEntry, | |
| ...(p.state !== undefined ? { state: p.state } : {}), | |
| } | |
| : p; | |
| return liveEntry | |
| ? { | |
| ...liveEntry, | |
| ...(p.state !== undefined && p.url === liveEntry.url | |
| ? { state: p.state } | |
| : {}), | |
| } | |
| : p; |
中文说明
此合并仅按编号把持久化 sidecar 的 state 覆盖到活条目上，没有 p.url === liveEntry.url 检查——重新引入了本 diff 三个写入方各自防范的跨仓库状态污染（upsertSessionPr 的状态继承以 URL 一致为前提；updateSessionPrStates 在锁内校验 URL；bridge 仅在 URL 不变时保留状态——三处都写明“另一仓库的同编号是另一个 PR”）。具体触发：会话绑定 repoA #5；刷新扫描在 sidecar 中打上 state:'merged'。用户把 #5 重绑到 repoB 的 PR（fork 布局下同编号常见——本 diff 注释自己也这么说）。绑定路由先 bump catalog 再 await sidecar 写入；该窗口内的一次列表渲染——或 bridge 变更已发布后 sidecar 写入抛错——会把活 {5, repoB-url} 与为 repoA #5 解析的持久 {state:'merged'} 合并：repoB 的 OPEN PR 被渲染成灰暗/“Merged”。扫描把 'merged' 视为终态不再查询该条目，写入失败时污染将持续整个 daemon 生命周期。探针经真实 listWorkspaceSessionsForResponse（持久 {5, repo-a, state:'merged'}，活 {5, repo-b}）：合并结果 prs=[{number:5, url:repo-b..., state:'merged'}]——repoA 的终态被盖到 repoB 的 URL 上；修复（状态覆盖以 p.url === liveEntry.url 为前提）后状态消失。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| for (const candidate of candidates) { | ||
| if (existingNumbers.has(candidate.number)) { | ||
| alreadyBound.push(candidate.number); | ||
| continue; |
There was a problem hiding this comment.
[Critical] upsertSessionPrs dedups candidates by bare PR number (existingNumbers.has(candidate.number)), ignoring URL and source — a gh-verified 'create' binding whose number collides with another repo's already-bound PR is silently dropped, contradicting the singular upsertSessionPr's own documented rule ("The same number in another repository is another PR") and the repo-identity gating used everywhere else in this diff. The shell hook feeds exactly this function and gates its notification on applied.added, and the new test blesses the number-only semantics. Concrete trigger: the sidecar holds repoA/pull/5 (source:'review', bound via a /review <url> form — bindCandidateNumbers explicitly binds the named URL itself). The user runs gh pr create in repoB within the same session; gh resolves repoB's new PR, which is also #5 (PR numbers are per-repo; low numbers collide routinely in the fork layouts this diff itself calls routine). All shell-hook attribution gates pass, then the number-only skip hits: added is empty, no binding, no emitSessionPrBound. The session's own created PR is permanently unbound — no badge, no 'create' eviction protection, no recovery (backfill deliberately declines transcript gh pr create traces; the sweep only rewrites states). Same-repo variant: a number first bound 'review' is never upgraded to 'worktree' by a later backfill run.
witness (probe against the real functions, sidecar seeded {5, repo-a, source:'review'}, candidate {5, repo-b/pull/5, state:'open', source:'create'}):
PLURAL (unmodified): added=[] alreadyBound=[5]; persisted=[{5, repo-a, review}] — dropped
SINGULAR (same file): persisted=[{5, repo-b, create}] — binds
flip (URL-aware skip mirroring the singular semantics): PLURAL added=[5]; persisted=[{5, repo-b, create}]
When the existing entry with the same number has a DIFFERENT url, treat it as a re-bind the way upsertSessionPr does (replace the entry, fresh createdAt, apply the source-upgrade rule); restrict the "untouched" path to same-URL candidates. Add a test offering a candidate whose number is bound to another repo's URL.
中文说明
upsertSessionPrs 仅按 PR 编号去重（existingNumbers.has(candidate.number)），忽略 URL 与来源——一个经 gh 验证的 'create' 绑定，若其编号与另一仓库已绑定的 PR 相同，会被静默丢弃；这与单数版 upsertSessionPr 自身写明的规则（“另一仓库的同编号是另一个 PR”）及本 diff 其他各处的仓库身份门相矛盾。shell 钩子恰恰调用此函数，并以 applied.added 作为发通知的门；新增测试还认可了“仅按编号”的语义。具体触发：sidecar 已有 repoA/pull/5（source:'review'，经 /review <url> 形式绑定——bindCandidateNumbers 明确绑定用户指名的 URL 本身）。用户在同一会话中于 repoB 执行 gh pr create；gh 解析出 repoB 的新 PR 也是 #5（PR 编号按仓库计，本 diff 自己也说 fork 布局下低编号冲突是常态）。shell 钩子所有归属门都通过后，命中“仅编号”跳过：added 为空、不绑定、不发 emitSessionPrBound。会话自己创建的 PR 永久失去绑定——无徽章、无 'create' 逐出保护、无恢复途径（backfill 刻意不回收 transcript 中的 gh pr create 痕迹；扫描只刷状态）。同仓库变体：先以 'review' 绑定的编号，之后的 backfill 永远不会把它升级为 'worktree'。探针（真实函数，播种 {5, repo-a, source:'review'}，候选 {5, repo-b/pull/5, state:'open', source:'create'}）：PLURAL（未改）added=[] alreadyBound=[5]，持久仍为 repoA/review——被丢弃；单数版同文件则持久 {5, repo-b, create}；修复（按 URL 感知、对齐单数语义）后 added=[5] 且持久为 repoB/create。建议：同号但 URL 不同时按 upsertSessionPr 的方式视为重绑（替换条目、新 createdAt、应用来源升级规则）；“保持不变”路径仅限同 URL 候选。并新增“候选编号已被另一仓库 URL 占用”的测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (!existsSync(sourcePath)) return; | ||
| await fs.mkdir(path.dirname(destinationPath), { recursive: true }); | ||
| if (!existsSync(destinationPath)) { |
There was a problem hiding this comment.
[Critical] withSidecarLock materializes its lock target before locking (await fs.appendFile(filePath, ''), ~line 265), and moveSessionPrSidecar routes BOTH endpoints through it — so both files always exist before this guarded body runs: the source-absent guard is dead code, the rename fast path never fires, and every no-op move creates a stray empty sidecar. The removed predecessor movePrSidecar checked existence BEFORE creating anything — this is a regression introduced by this diff. No-op mutations through other paths (backfill with all-unresolved candidates, validation-declined binds) materialize stray sidecars at the same root. Concrete trigger: sessionService.archiveSessions/unarchiveSessions call this unconditionally for every session, so every archive/restore of a session that never bound a PR — the common case — leaves a stray empty <sessionId>.pr.json ping-ponging between chats/ and chats/archive/ forever.
witness (probe on unmodified code — moveSessionPrSidecar(src, dst) with neither file existing):
observed: { dstExists: true, dstSize: 0, srcExists: false }
documented contract: "only a still-present file moves" — destination must not appear
flip (guard !existsSync(sourcePath) before the lock chain): probe passes; existing lock-coverage tests unaffected
The committed test "does nothing when the source is absent" passes only because readSessionPrs reads an empty file as null — it never checks existence.
Stop materializing paths a mutation does not write: give withSidecarLock a createIfMissing flag (default true) and do a non-materializing existence check in moveSessionPrSidecar before enqueuing, so an absent source returns without touching either endpoint; only the destination — the path that can be written — should be pre-created.
中文说明
withSidecarLock 在加锁前会把锁目标实体化（await fs.appendFile(filePath, '')，约 265 行），而 moveSessionPrSidecar 把两个端点都交给它——因此受保护主体运行时两个文件必然已存在：源缺失守卫成为死代码、rename 快路径永不触发、每次空操作移动都会创建一个多余的空 sidecar。被删除的前身 movePrSidecar 是在创建任何东西之前先检查存在性——这是本 diff 引入的回归。其他路径的空操作变更（全部候选未解析的 backfill、被校验拒绝的绑定）也会在同一根因处实体化出多余 sidecar。具体触发：sessionService.archiveSessions/unarchiveSessions 对每个会话无条件调用它，因此任何从未绑定 PR 的会话（常见情形）每次归档/恢复都会留下一个在 chats/ 与 chats/archive/ 之间来回乒乓的空 <sessionId>.pr.json。探针（未改代码，两个文件都不存在时调用 moveSessionPrSidecar(src, dst)）：观察到 { dstExists: true, dstSize: 0, srcExists: false }；文档契约为“仅仍存在的文件才被移动”——目标不应出现；修复（锁链之前先守卫 !existsSync(sourcePath)）后探针通过、现有锁覆盖测试不受影响。已提交的测试“源缺失时不做事”之所以通过，仅因 readSessionPrs 把空文件读成 null——它从未检查存在性。建议：不要为不会被写入的路径做实体化——给 withSidecarLock 增加 createIfMissing 标志（默认 true），并在 moveSessionPrSidecar 入队前做不实体化的存在性检查，使源缺失时不触碰任何端点直接返回；只有可能被写入的目标路径才应预创建。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| } else if (!/^[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(input)) { | ||
| // scp-style [user@]host:path — any user, not only `git`. | ||
| const scp = /^(?:[^@\s/]+@)?([^:\s/]+):(.+)$/.exec(input); |
There was a problem hiding this comment.
[Critical] normalizeRemoteToWebUrl mis-parses a Windows drive-letter local-path origin (C:\src\origin.git) as an scp-style remote: the drive letter matches the scp host, WHATWG URL folds the backslashes, and the function fabricates https://c//src/origin plus repo key c/src/origin instead of returning undefined like every other non-web branch — backfill's workspaceRepoKey === undefined fail-closed guard is then bypassed by the fabricated key. Concrete trigger: a Windows workspace cloned from a local path (git clone C:\src\origin.git → git remote get-url origin returns the drive path). In backfill the workspace key is defined-but-garbage: every real gh-page entry fails the key gate (numberToUrl stays empty; the undefined-key fail-closed path never fires), and every convention//review number falls to the ${remote}/pull/${number} fallback persisting URLs like https://c//src/origin/pull/5 into sidecars — dead badge links, systematically for every binding in that layout.
witness (probe on unmodified code):
normalizeRemoteToWebUrl('C:\\src\\origin.git') -> "https://c//src/origin"
normalizeRemoteToWebUrl('D:/repos/origin.git') -> "https://d//repos/origin" (expected undefined)
repoKeyFromWebUrl(...) -> "c/src/origin"
flip (drive-letter guard): returns undefined; all 53 existing github-prs tests still pass
Reject drive-letter "hosts" in the scp branch before building the URL — real hostnames are never a single character; Windows drive letters always are:
const scp = /^(?:[^@\s/]+@)?([^:\s/]+):(.+)$/.exec(input);
if (scp && /^[A-Za-z]$/.test(scp[1])) return undefined;中文说明
normalizeRemoteToWebUrl 会把 Windows 盘符本地路径 origin（C:\src\origin.git）误判为 scp 风格远端：盘符匹配到 scp 的 host，WHATWG URL 把反斜杠折叠，函数凭空造出 https://c//src/origin 及仓库 key c/src/origin，而不是像其他非 web 分支那样返回 undefined——backfill 的 workspaceRepoKey === undefined fail-closed 守卫因此被这个伪造 key 绕过。具体触发：Windows 上从本地路径克隆的工作区（git clone C:\src\origin.git → git remote get-url origin 返回盘符路径）。backfill 中工作区 key 有值但是垃圾值：所有真实 gh 页面条目都过不了 key 门（numberToUrl 为空；未定义 key 的 fail-closed 路径永不触发），每个约定//review 编号都落到 ${remote}/pull/${number} 回退，把 https://c//src/origin/pull/5 这类 URL 持久化进 sidecar——该布局下所有绑定的徽章链接系统性失效。探针（未改代码）：normalizeRemoteToWebUrl('C:\\src\\origin.git') → https://c//src/origin；'D:/repos/origin.git' → https://d//repos/origin（期望 undefined）；repoKeyFromWebUrl(...) → c/src/origin；修复（盘符守卫）后返回 undefined，现有 53 个 github-prs 测试全部仍通过。建议：在 scp 分支构建 URL 前拒绝盘符“host”——真实主机名从不只有一个字符，Windows 盘符永远是。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…code into feat/session-pr-gh-create
…ger mock (QwenLM#9739) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9739 (same-run verification repair of the rejected commit)Context
Feedback dispositionsReview body
Inline Critical findings — all 7 resolved in code by the preserved commit, re-verified this roundThe fixes were implemented by the preserved rejected commit
Deferred non-Critical feedbackCritical-only mode is active; the Failed checks
Changes this roundOne file,
Mutation probes: each guard's absent state was actually run and was red — the pre-round tree (no hunk 1) produced 40 errors with exit 1; the tree with hunk 1 only (no hunk 2) produced the 1 Commit: Environment-specific local failures (disclosed, out of scope)On this self-hosted runner the full VerificationCommands actually run this round:
中文说明轮次总结 — PR #9739（对被拒提交的同轮验证修复）背景
反馈处置评审主体
行内 Critical 发现 — 7 条全部已由保留提交在代码中解决，本轮重新验证这些修复由被拒提交
延后的非 Critical 反馈仅 Critical 模式已启用； 失败检查
本轮改动单个文件
变异探针：每个守卫的"缺失态"都被实际运行过且为红——轮前树（无 hunk 1）产生 40 个错误、退出码 1；仅有 hunk 1 的树（无 hunk 2）产生 1 个 提交： 环境特定的本地失败（披露，超出范围）在本自托管运行器上，完整 验证本轮实际执行的命令：
Deferred non-Critical feedbackCritical-only mode is active: the PR's diff grew src 261 / test 453 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 净增长已达 源码 261 / 测试 453 行（预算 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/模型 |
Reserve pr-<number> worktree slugs (R1-21), mention state in the pr validation errors (R1-22), and tighten tests (alreadyBound read-back, gh query args, invalid-state PATCH, slug reservation). The bot rounds already landed the lock/fence/attribution fixes; residual text-detector limits are documented.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: test-efficacy — probe harness could not self-validate (no probe file was green in the unmutated baseline; vitest dist prerequisite in the per-file probe runner).
Not explored to full depth (tool budget reached): "agent invariant-b (packages/core/src/utils/github-prs.ts)": none — no check was cut short.; chunk 3: none — wait, per the brief, since nothing was cut short, no budget line is warranted. Nothing was cut short..
Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:
docs/design/2026-08-20-webshell-session-pr-binding.md:26 — [review] This PR amends this exact doc bullet list (adds the…docs/design/2026-08-20-webshell-session-pr-binding.md:69 — [review] The new backfill section of the design doc documents an…packages/acp-bridge/src/bridge.test.ts:25406 — [review] The new pr-binding suite never exercises the handler's…packages/acp-bridge/src/bridge.ts:9870 — [review] The bridge's new pr.state enum check throws…packages/acp-bridge/src/bridge.ts:9960 — [review] The "no change, no event" de-dup in updateSessionMetadata …packages/cli/src/acp-integration/session/Session.ts:8246 — [review] The wire connecting the shell-hook binding to the daemon…packages/cli/src/serve/fast-path-open.test.ts:311 — [review] The flake fix (10s vi.waitFor timeout) is applied to 3…packages/cli/src/serve/routes/session-pr-backfill.test.ts:199 — [review] The backfill suite never asserts the gh page fetch…packages/cli/src/serve/routes/session-pr-backfill.test.ts:427 — [review] The PR description still advertises the removed transcript…packages/cli/src/serve/routes/session-pr-backfill.test.ts:1206 — [review] The fork-layout test's bare-number case (SESSION_B)…packages/cli/src/serve/routes/session-pr-backfill.test.ts:1274 — [review] The route's per-workspace error isolation (try/catch at…packages/cli/src/serve/routes/session-pr-backfill.ts:97 — [review] REVIEW_COMMAND_PATTERN 's URL alternative is…packages/cli/src/serve/routes/session-pr-backfill.ts:98 — [review] The #? token — the /review #N invocation form…packages/cli/src/serve/routes/session-pr-backfill.ts:98 — [review] The URL alternative terminates the PR number with only…packages/cli/src/serve/routes/session-pr-backfill.ts:143 — [review] The user-record fallback regex-scans the first text part…packages/cli/src/serve/routes/session-pr-backfill.ts:386 — [review] The full-sidecar skip equates "number present" with…packages/cli/src/serve/routes/session-pr-backfill.ts:396 — [review] Free-slot offer sizing keeps the strongest freeSlots …packages/cli/src/serve/routes/session-pr-backfill.ts:424 — [review] Backfill's per-candidate failure isolation (the failed …packages/cli/src/serve/routes/session.ts:2262 — [review] The new state write-boundary validation has no test at…packages/cli/src/serve/routes/session.ts:2263 — [review] The new pr.state validation rejects requests through the…- …and 36 more (see the run report)
Convergence: round 13 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 7 (7 new). Findings keep coming back to the same files: packages/cli/src/serve/routes/session-pr-backfill.ts (findings in round 12; 1 more now); packages/cli/src/serve/server/session-list.ts (findings in round 12; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：test-efficacy — probe harness could not self-validate (no probe file was green in the unmutated baseline; vitest dist prerequisite in the per-file probe runner)。
未探索到全部深度（达到工具调用预算）："agent invariant-b (packages/core/src/utils/github-prs.ts)"：none — no check was cut short.；chunk 3：none — wait, per the brief, since nothing was cut short, no budget line is warranted. Nothing was cut short.。
收敛姿态下延后（第 13 轮，非阻断）——已记录，本轮不要求修改：共 56 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 13 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 7 条（其中 7 条首次提出）。发现反复回到同一批文件：packages/cli/src/serve/routes/session-pr-backfill.ts（第 12 轮已出过发现，本轮又有 1 条）；packages/cli/src/serve/server/session-list.ts（第 12 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| for (const liveEntry of livePrs) { | ||
| if ( | ||
| !persistedNumbers.has(liveEntry.number) && | ||
| ordered.length < SESSION_PR_LIST_LIMIT | ||
| ) { |
There was a problem hiding this comment.
[Critical] R13-1: The live-only append loop gates on the running merged length (ordered.length < SESSION_PR_LIST_LIMIT) instead of the persisted list's length, so below the cap it drops the session's NEWEST live-only bindings once the running total reaches the cap — contradicting the comment directly above it ("below it a live-only entry is genuinely the newest"). The bind routes are bridge-first: they append to the live entry, then await upsertSessionPr, then reconcile; with the 2s-TTL persisted snapshot still short, two overlapping binds (or one bind in flight) make live [P2..P9, B1, B2] against persisted [P1..P9] — the merge appends B1, ordered.length hits 10, and B2, the newest binding, fails the gate and is dropped, so the badge shows B1 as latest until the next fresh listing. The single-bind variant at persisted=10 hides a brand-new binding for the whole in-flight window.
Witness (probe driving the real listWorkspaceSessionsForResponse in an isolated tree):
persisted=[1..9], live=[2..9,11,12]:
PR code -> merged numbers [1,2,3,4,5,6,7,8,9,11] <- newest binding 12 dropped, stale 1 kept
with fix -> merged numbers [2,3,4,5,6,7,8,9,11,12] <- flips, both arms pass
Gate on the persisted size and let the existing final slice remain the only cap:
| for (const liveEntry of livePrs) { | |
| if ( | |
| !persistedNumbers.has(liveEntry.number) && | |
| ordered.length < SESSION_PR_LIST_LIMIT | |
| ) { | |
| for (const liveEntry of livePrs) { | |
| if ( | |
| !persistedNumbers.has(liveEntry.number) && | |
| persistedPrs.length < SESSION_PR_LIST_LIMIT | |
| ) { |
中文说明
活列表追加循环用的是合并后列表的当前长度（ordered.length < SESSION_PR_LIST_LIMIT）而非持久化列表的长度，因此在未达上限时，一旦合并长度达到 10 就会丢弃会话最新的活绑定——与上方注释（“未达上限时活条目确实是最新的”）直接矛盾。绑定路由是先写 bridge 再 await upsertSessionPr 再对账，配合 2 秒 TTL 的持久化快照，两个并发绑定（或一个在途绑定）会形成活列表 [P2..P9, B1, B2] 对持久 [P1..P9]：合并追加 B1 后 ordered.length 到 10，最新绑定 B2 被丢弃，徽章会把 B1 显示为最新，直到下次列表刷新。探针（驱动真实 listWorkspaceSessionsForResponse）：PR 代码下合并结果 [1,2,...,9,11]（最新绑定 12 被丢、过期的 1 保留）；按建议修复后为 [2,...,9,11,12]，两侧均验证通过。建议改为以持久列表长度作为门槛，最终的 slice 继续作为唯一上限。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(sessionService.emitSessionPrBound).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('binds when a promote is refused after the command settled', async () => { |
There was a problem hiding this comment.
[Critical] D13-44: A SUCCESSFUL promote and any is_background: true gh pr create never reach bindGhPrCreate — its sole call site (shell.ts:2759) sits AFTER the if (result.promoted) return handlePromotedForeground(...) early return, and executeBackground never calls it — yet this same PR removed the transcript gh pr create backfill source whose committed justification (session-pr-backfill.ts:176) asserts "live creates bind through the shell tool post-hook", and the refresh sweep only re-states existing bindings. The new suite pins only the promote-REFUSED arm that still binds. So a Ctrl+B mid-gh pr create --fill, or an agent-run background create, completes exit 0 printing the PR URL and the session badge never shows the PR — permanently, with no recovery path.
Witness (probe in the PR's own binding harness, isolated tree):
background arm: is_background:true, exit 0, PR URL in output
-> registry.complete called; upsertSessionPrsMock NEVER called
promote arm: promoted:true resolution
-> upsertSessionPrsMock never called
BASE 20/20 green; candidate fix (run the gh-verified gate on the settle path) flips the probe
Re-run the same gh-verified gate (pre/post snapshot, open-state, repo-key, branch-identity, URL-in-output) when a background or promoted shell settles — the settle hook already exists. If background/promoted creates are deliberately out of scope instead, say so in bindGhPrCreate's doc comment, correct the backfill comment that justifies the transcript-source removal, and pin the exclusion with a negative test next to this one.
中文说明
成功 promote 以及任何 is_background: true 的 gh pr create 都到不了 bindGhPrCreate——其唯一调用点（shell.ts:2759）位于 if (result.promoted) return handlePromotedForeground(...) 提前返回之后，executeBackground 也从不触发它；而本 PR 又删除了 transcript gh pr create 回填源，其提交时的理由（session-pr-backfill.ts:176）声称“实时创建由 shell 工具后置钩子绑定”，刷新定时器也只刷新已有绑定的状态。新增测试只钉住了仍会绑定的 promote-被拒绝分支。于是：gh pr create --fill 进行中被 Ctrl+B 转后台、或 agent 以后台方式执行创建，命令以 exit 0 结束并打印 PR URL，但会话徽章永远不会显示这个 PR——且没有任何恢复途径。探针（使用 PR 自带的绑定测试框架）：后台分支下 registry.complete 被调用而 upsertSessionPrsMock 从未被调用；promote 分支同样从未调用；基线 20/20 全绿，候选修复（在 settle 路径上执行同一套 gh 验证门）可使探针翻转。建议：后台/promoted 命令落定时重跑同一套 gh 验证门（settle 钩子已存在）；若刻意不覆盖这些形态，请在 bindGhPrCreate 文档注释中说明、修正背书删除回填源的注释，并在此测试旁补一个负例钉住该排除。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| url = `${sources.remote}/pull/${number}`; | ||
| } | ||
| } | ||
| const state = sources.pageStateByNumber.get(number); |
There was a problem hiding this comment.
[Critical] D13-50: bindCandidateNumbers stamps state from the gh page by PR number alone, regardless of which repo the resolved url belongs to. In the fork layout — fork-trust branch holding, gh page listing the PARENT's PRs — a /review <fork-url> form resolves the fork URL correctly but pairs it with the parent repo's same-numbered PR's state (PR numbers are per-repo, so that is a DIFFERENT PR): fork PR #7 open + parent PR #7 merged persists {number: 7, url: .../me/fork/pull/7, state: 'merged'} and the badge/tooltip renders the fork's OPEN PR as merged. The refresh sweep cannot correct it — it keys stamps by the page's repo key, looks each binding up under its own URL's key, and its own test pins fork-URL bindings as skipped — and 'merged' is terminal, so the wrong state persists. Same-repo layouts are immune (numberToUrl pairs URL and state from one entry).
Witness (probe, isolated tree):
fork layout (origin me/fork, fetchAttributionRepoKeys -> {resolved: fork, parent: parent}),
/review https://github.com/me/fork/pull/7, parent page listing PR 7 merged:
PR code -> persisted {number:7, url:"https://github.com/me/fork/pull/7", state:"merged"}
with fix (stamp only when the bound URL's repo matches the page's repo) -> state dropped;
all 38 shipped backfill tests still pass
Thread pageRepoKey into sources and stamp state only when the bound URL's repo matches the page's repo, e.g. set sources.pageRepoKey = pageRepoKey in the gate construction and use it here:
const state =
url !== undefined && repoKeyFromWebUrl(url) === sources.pageRepoKey
? sources.pageStateByNumber.get(number)
: undefined;中文说明
bindCandidateNumbers 仅按 PR 编号从 gh 页面取 state，不检查解析出的 url 属于哪个仓库。在 fork 布局下（fork 信任分支成立、gh 页面列出的是父仓库的 PR），/review <fork-url> 形式能正确解析出 fork 的 URL，却配上了父仓库同号 PR 的状态（PR 编号按仓库计，这是另一个 PR）：fork 的 PR #7 是 open、父仓库的 PR #7 是 merged 时，会持久化 {number: 7, url: .../me/fork/pull/7, state: 'merged'}，徽章/工具提示会把 fork 的 OPEN PR 显示为已合并。刷新扫描也纠正不了：它按页面仓库 key 建状态索引、按绑定自身 URL 的 key 查找（其自身测试已钉住 fork-URL 绑定会被跳过），且 'merged' 是终态，错误状态会一直存在。同仓库布局不受影响（numberToUrl 的 URL 与状态来自同一条目）。探针：按建议修复（仅当绑定 URL 的仓库与页面仓库一致时打标）后状态不再写入，现有 38 个 backfill 测试全部仍绿。建议把 pageRepoKey 传入 sources，并在此处加上仓库一致性条件。
— qwen3.8-max via Qwen Code /review (v0.22.0)
R1-10/R3-10: accept verified wrapper shapes (sudo/env/nohup/command with flags or KEY=val, path-qualified gh binaries, the pr new alias) in the execution gate; quote-awareness stays approximate by design (the shell-aware tokenizer cannot enter the serve closure). R1-22: pr validation errors now name the state constraint at the route and bridge.
… feat/session-pr-gh-create
…oncile test The reconcile test built its sidecar path from the bridge session id, which is `sess:<cwd>` — on Windows that expands to `sess:D:\work\a`, so `path.join` turned the filename into extra path segments and `writeSessionPrs`' `mkdir(dirname)` failed with ENOENT on the illegal drive colon: ENOENT: no such file or directory, mkdir 'C:\...\Temp\bridge-pr-reconcile-XXXX\sess:D:\work' The id was never meaningful here — the test only ever refers to the file through `sidecarPath`. Use a fixed name inside the temp directory. Production paths are unaffected: they come from `sessionService.getPrSessionPathForArchiveState()`, which keys off the persisted session UUID like the existing `.jsonl` and `.worktree.json` sidecars.
The gh pr create gate advertises inline-token shapes (GH_TOKEN=x gh pr create --fill), but fetchCurrentBranchPullRequest ran its verification legs bare: with no ambient gh auth the legs errored and the binding silently missed. Extract the leading GH_*/GITHUB_* assignments from the create segment and thread them into both the pre-run snapshot and the post-run attribution leg, so they authenticate the way the create itself did.
The inline-credential threading added for the gh pr create verification legs needs the resolver to authenticate with the same env; restore the optional env parameter (gitEnv(env)) that the round-6 rewrite dropped.
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: test-efficacy — probe harness could not self-validate (no probe file was green in the unmutated baseline; vitest dist prerequisite in the probe tree).
Not explored to full depth (tool budget reached): chunk 17: none — no checks were cut short..
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 14, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/acp-http/dispatch.ts:2979 — [review] reconcile runs after session_metadata_updated publishes — the event payload carries the diverged positional-cap listpackages/acp-bridge/src/bridgeClient.ts:2243 — [review] shell-detected bindings only mark the catalog clock; pr-less metadata echoes miss the shell-bound PRpackages/cli/src/serve/routes/session-pr-backfill.test.ts:1216 — [review] fork-layout SESSION_B asserts only the number — a trust-branch regression ships green with the 404 fork URLpackages/cli/src/serve/routes/session-pr-backfill.ts:336 — [review] allowedRepoKeys admits pageRepoKey unconditionally, including divergent untrusted pagespackages/cli/src/serve/routes/session-pr-backfill.ts:396 — [review] free-slot sizing counts already-bound candidates — fresh weaker candidates are permanently spliced awaypackages/cli/src/serve/run-qwen-serve.ts:6100 — [review] dynamic import of session-pr-refresh.js has no .catch — an unhandled rejection crashes the daemon on fast-path bootpackages/cli/src/serve/server/session-pr-refresh.ts:51 — [review] no lower interval floor — 0.001 minutes installs a 60ms gh-spawning busy looppackages/core/src/services/session-pr-service.ts:160 — [review] gate regex enumerates corner shapes — six verified-real create forms miss (stacked wrappers, timeout/time, ./ and $HOME paths, quoted assignments)packages/core/src/services/session-pr-service.ts:402 — [review] upsertSessionPr drops an explicit source:'review' on NEW entries (undefined ranks above review)packages/core/src/utils/github-prs.test.ts:554 — [review] no test passes env to the three fetchers — dropping the env override ships greenpackages/cli/src/acp-integration/session/Session.ts:8246 — [review] the pr-bound callback registration/emission (middle link of the propagation chain) is untestedpackages/sdk-typescript/src/daemon/session-pr.ts:40 — [review] SDK shape gate's new state acceptance has no test pairingpackages/acp-bridge/src/bridge.ts:9870 — [review] bridge-level invalid-state rejection absent from bridge.test.ts's table — sole gate on the ACP pathpackages/web-shell/client/components/sidebar/SessionDetailsTooltip.tsx:171 — [review] tooltip merged/closed suffix and two new i18n keys untested — label-swap and suffix-deletion mutants ship green (probe)packages/cli/src/serve/acp-http/transport.test.ts:461 — [review] FakeBridge setSessionPrs drops state — the reconcile's state propagation is pinned by nothingpackages/cli/src/serve/server/session-pr-refresh.test.ts:111 — [review] the sweep's archived loop half has zero coverage in the file that adds itpackages/core/src/services/session-pr-service.test.ts:451 — [review] no-write tests use byte-equality that cannot detect an identical rewrite — guard removal ships green (mutant+inode probe)packages/core/src/services/session-pr-service.ts:616 — [review] moveSessionPrSidecar's rename fast path is unreachable — both locks materialize the destinationpackages/core/src/services/sessionService.ts:2023 — [review] enumerateSessionIdsForArchiveState silently truncates at 10000 despite the FULL-set doc (probe: 10001 seeded → 1 dropped deterministically)packages/core/src/tools/shell.test.ts:431 — [review] output.includes(url) substring gate — a superstring URL (pull/771 ⊃ pull/77) passes and falsifies creator attribution (probe e2e)- …and 12 more (see the run report)
Convergence: round 14 posted 10 inline comment(s), 7 of them reported for the first time; the previous round posted 3 (3 new). Findings keep coming back to the same files: packages/cli/src/serve/routes/session-pr-backfill.ts (findings in round 13; 1 more now); packages/cli/src/serve/server/session-list.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.)
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.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：test-efficacy — probe harness could not self-validate (no probe file was green in the unmutated baseline; vitest dist prerequisite in the probe tree)。
未探索到全部深度（达到工具调用预算）：chunk 17：none — no checks were cut short.。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 14 轮，非阻断）——已记录，本轮不要求修改：共 32 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 14 轮发布了 10 条行内评论，其中 7 条是首次提出；上一轮发布了 3 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/cli/src/serve/routes/session-pr-backfill.ts（第 13 轮已出过发现，本轮又有 1 条）；packages/cli/src/serve/server/session-list.ts（第 13 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| !persistedNumbers.has(liveEntry.number) && | ||
| ordered.length < SESSION_PR_LIST_LIMIT |
There was a problem hiding this comment.
[Critical] R13-1 (round 13 — re-checked at this commit, still stands; no commit touched this file since the comment): the live-only append loop gates on the running merged length (ordered.length < SESSION_PR_LIST_LIMIT) instead of the persisted list's length, so below the cap it drops the session's NEWEST live-only bindings once the running total reaches the cap — contradicting the comment directly above it ("below it a live-only entry is genuinely the newest"). The bind routes are bridge-first (append live, await upsertSessionPr, then reconcile); with the 2s-TTL persisted snapshot still short, two overlapping binds make live [P2..P9, B1, B2] against persisted [P1..P9] — the merge appends B1, ordered.length hits 10, and B2, the newest binding, fails the gate and is dropped, so the badge shows B1 as latest until the next fresh listing.
Witness (round-13 probe driving the real listWorkspaceSessionsForResponse):
persisted=[1..9], live=[2..9,11,12]:
PR code -> merged [1,2,3,4,5,6,7,8,9,11] <- newest binding 12 dropped, stale 1 kept
with fix -> merged [2,3,4,5,6,7,8,9,11,12]
Gate on the persisted size and let the existing final slice remain the only cap:
| !persistedNumbers.has(liveEntry.number) && | |
| ordered.length < SESSION_PR_LIST_LIMIT | |
| !persistedNumbers.has(liveEntry.number) && | |
| persistedPrs.length < SESSION_PR_LIST_LIMIT |
中文说明
[Critical] R13-1（第 13 轮发现，本提交复核后仍然成立——该文件自评论后无任何提交）：活列表追加循环用合并后列表的当前长度（ordered.length < SESSION_PR_LIST_LIMIT）而非持久化列表的长度作门槛，因此在未达上限时，一旦合并长度达到 10 就会丢弃会话最新的活绑定——与上方注释（"未达上限时活条目确实是最新的"）直接矛盾。绑定路由是先写 bridge 再 await upsertSessionPr 再对账；配合 2 秒 TTL 的持久化快照，两个并发绑定会形成活列表 [P2..P9, B1, B2] 对持久 [P1..P9]：合并追加 B1 后 ordered.length 到 10，最新绑定 B2 被门槛丢弃，徽章会把 B1 显示为最新，直到下次全新列表刷新。探针（驱动真实 listWorkspaceSessionsForResponse）：PR 代码下合并结果 [1..9,11]（最新绑定 12 被丢、过期的 1 保留）；按建议修复后为 [2..9,11,12]。建议以持久列表长度作门槛，最终 slice 继续作为唯一上限。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(sessionService.emitSessionPrBound).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('binds when a promote is refused after the command settled', async () => { |
There was a problem hiding this comment.
[Critical] R13-2 (round 13 — re-checked at this commit, still stands): a SUCCESSFUL promote and any is_background: true gh pr create never reach bindGhPrCreate — its sole call site (shell.ts:2765) sits AFTER the if (result.promoted) early return, the promote settle handler only transitions the registry entry, and executeBackground never calls it — yet this PR removed the transcript gh pr create backfill source whose committed justification (session-pr-backfill.ts) still asserts "live creates bind through the shell tool post-hook". So a Ctrl+B mid-gh pr create --fill (promote succeeds), or an agent-run background create, completes exit 0 printing the PR URL and the session badge never shows the PR — permanently, with no recovery path (backfill sources are only /review commands and the worktree convention; the refresh sweep only re-states existing bindings). The suite pins only the promote-REFUSED arm (this test); there is no exclusion note in bindGhPrCreate's doc comment and no negative test.
Re-run the same gh-verified gate (pre/post snapshot, open-state, repo-key, branch-identity, URL-in-output) when a background or promoted shell settles — the settle hook already exists. If background/promoted creates are deliberately out of scope instead, say so in bindGhPrCreate's doc comment, correct the backfill comment that justifies the transcript-source removal, and pin the exclusion with a negative test next to this one.
中文说明
[Critical] R13-2（第 13 轮发现，本提交复核后仍然成立）：成功 promote 以及任何 is_background: true 的 gh pr create 都到不了 bindGhPrCreate——其唯一调用点（shell.ts:2765）位于 if (result.promoted) 提前返回之后，promote 落定处理器只转移 registry 状态，executeBackground 也从不触发它；而本 PR 又删除了 transcript gh pr create 回填源，其提交时的理由（session-pr-backfill.ts）仍声称"实时创建由 shell 工具后置钩子绑定"。于是：gh pr create --fill 进行中被 Ctrl+B 转后台（promote 成功）、或 agent 以后台方式执行创建，命令以 exit 0 结束并打印 PR URL，但会话徽章永远不会显示这个 PR——且没有任何恢复途径（回填源只有 /review 命令与 worktree 约定；刷新定时器只刷新已有绑定的状态）。测试只钉住了仍会绑定的 promote-被拒绝分支（本测试）；bindGhPrCreate 文档注释无排除说明，也没有负例测试。建议：后台/promoted 命令落定时重跑同一套 gh 验证门（settle 钩子已存在）；若刻意不覆盖这些形态，请在文档注释中说明、修正背书删除回填源的注释，并在此测试旁补一个负例钉住该排除。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| url = `${sources.remote}/pull/${number}`; | ||
| } | ||
| } | ||
| const state = sources.pageStateByNumber.get(number); |
There was a problem hiding this comment.
[Critical] R13-3 (round 13 — re-checked at this commit, still stands; no commit touched this file since the comment): bindCandidateNumbers stamps state from the gh page by PR number alone, regardless of which repo the resolved url belongs to. In the fork layout — fork-trust branch holding, gh page listing the PARENT's PRs — a /review <fork-url> form resolves the fork URL correctly but pairs it with the parent repo's same-numbered PR's state (PR numbers are per-repo, so that is a DIFFERENT PR): fork PR #7 open + parent PR #7 merged persists {number: 7, url: .../me/fork/pull/7, state: 'merged'} and the badge/tooltip renders the fork's OPEN PR as merged. The refresh sweep cannot correct it — it keys stamps by the page's repo key, looks each binding up under its own URL's key, and its own test pins fork-URL bindings as skipped — and 'merged' is terminal, so the wrong state persists.
Witness (round-13 probe, isolated tree):
fork layout, /review https://github.com/me/fork/pull/7, parent page listing PR 7 merged:
PR code -> persisted {number:7, url:"https://github.com/me/fork/pull/7", state:"merged"}
with fix (stamp only when the bound URL's repo matches the page's repo) -> state dropped;
all 38 shipped backfill tests still pass
Thread pageRepoKey into sources and stamp state only when the bound URL's repo matches the page's repo:
const state =
url !== undefined && repoKeyFromWebUrl(url) === sources.pageRepoKey
? sources.pageStateByNumber.get(number)
: undefined;中文说明
[Critical] R13-3（第 13 轮发现，本提交复核后仍然成立——该文件自评论后无任何提交）：bindCandidateNumbers 仅按 PR 编号从 gh 页面取 state，不检查解析出的 url 属于哪个仓库。在 fork 布局下（fork 信任分支成立、gh 页面列出的是父仓库的 PR），/review <fork-url> 形式能正确解析出 fork 的 URL，却配上了父仓库同号 PR 的状态（PR 编号按仓库计，这是另一个 PR）：fork 的 PR #7 是 open、父仓库的 PR #7 是 merged 时，会持久化 {number: 7, url: .../me/fork/pull/7, state: 'merged'}，徽章/工具提示会把 fork 的 OPEN PR 显示为已合并。刷新定时器也纠正不了：它按页面仓库 key 建状态索引、按绑定自身 URL 的 key 查找（其自身测试已钉住 fork-URL 绑定会被跳过），且 'merged' 是终态，错误状态会一直存在。建议把 pageRepoKey 传入 sources，仅当绑定 URL 的仓库与页面仓库一致时打标（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (slug.includes('..') || slug.startsWith('.') || slug.startsWith('-')) { | ||
| return 'Worktree name must not start with "." or "-" or contain "..".'; | ||
| } | ||
| if (/^pr-[1-9]\d{0,8}$/.test(slug)) { |
There was a problem hiding this comment.
[Critical] R14-1: the new pr-<number> reservation sits in the shared validateUserWorktreeSlug, which createUserWorktree (gitWorktreeService.ts:1648) also runs — so it rejects the exact system-generated pr-<N> slug that the --worktree=#<N> launch produces, breaking PR-worktree creation; the same validator gates exit_worktree and --worktree <slug> re-attach, so existing pr-<N> worktrees can no longer be exited or re-entered by name.
qwen --worktree=#42 when .qwen/worktrees/pr-42 does not exist: worktreeStartup resolves slug pr-42 (bypassing user-slug validation by design), fetches the PR ref successfully, then calls service.createUserWorktree('pr-42', sha) — this reservation returns Worktree name must not look like "pr-<number>"… and startup aborts. exit_worktree({name:'pr-42'}) fails validateToolParams for the same reason, so users inside a legitimate PR worktree cannot keep/remove it through the tool — while this PR's own backfill treats the pr-<N> sidecar convention as its highest-authority binding source, and the new unit test cements the rejection.
Witness: the pre-existing packages/cli/src/startup/worktreeStartup.test.ts "creates a pr- worktree from FETCH_HEAD when fetch succeeds (local fake remote)" FAILS deterministically at this commit (expect(res!.ok).toBe(true) → false, 3 consecutive runs) and PASSES 18/18 at the merge base; validateUserWorktreeSlug('pr-42') returning that exact rejection string was reproduced against the built dist.
Keep the reservation for user-typed slugs only: bypass it for internal PR worktree creation (e.g. an internal option on createUserWorktree for the --worktree=#<N> launch path), and let exit_worktree / the startup re-attach path accept existing pr-<N> slugs.
中文说明
[Critical] R14-1：新增的 pr-<number> 保留名校验放在共享的 validateUserWorktreeSlug 中，而 createUserWorktree（gitWorktreeService.ts:1648）也会执行该校验——因此它会拒绝 --worktree=#<N> 启动路径自身生成的系统 slug pr-<N>，导致 PR worktree 创建失败；同一校验也把守着 exit_worktree 与 --worktree <slug> 重挂载，已有的 pr-<N> worktree 从此无法按名退出或重新进入。失败场景：.qwen/worktrees/pr-42 不存在时运行 qwen --worktree=#42：worktreeStartup 按设计绕过用户 slug 校验解析出 pr-42、成功 fetch PR ref，随后调用 createUserWorktree('pr-42', sha)——本保留名校验返回错误、启动中止；exit_worktree({name:'pr-42'}) 也因同样原因过不了参数校验——而本 PR 自己的回填却把 pr-<N> sidecar 约定当作最高权威绑定源，新加的单测还把该拒绝固化了下来。证据：既有测试 worktreeStartup.test.ts "creates a pr- worktree from FETCH_HEAD when fetch succeeds" 在本提交确定性失败（3 次连续运行），在合并基上 18/18 全绿；validateUserWorktreeSlug('pr-42') 的拒绝串已在构建产物上复现。建议：保留名只约束用户手输的 slug——为内部 PR worktree 创建路径（如 createUserWorktree 增加内部选项）绕过该校验，并让 exit_worktree / 启动重挂载路径接受已有的 pr-<N> slug。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| return liveEntry | ||
| ? { | ||
| ...liveEntry, |
There was a problem hiding this comment.
[Critical] R14-2: the merge spreads ...liveEntry unconditionally for numbers present in both stores, so when a sidecar-only writer (the shell hook / backfill) re-binds a PR number to a DIFFERENT repository, every list response keeps the stale live entry's url while the persisted order correctly puts the re-bind last — the badge links to the wrong repository's PR until daemon restart or another metadata bind on that session. Only the state overlay is guarded by p.url === liveEntry.url; the URL direction is unargued. The shell hook's own comment names this re-bind shape ("A same-number entry pointing at ANOTHER repo … is re-bound here"), and emitSessionPrBound is notification-only — nothing reconciles the live entry afterwards (the three merge call sites are read-only response builders).
Session binds repo-a's PR #5 via the metadata route (both stores hold {number:5, url:repo-a}); a gh pr create in the same session creates repo-b's PR #5 and the shell post-hook rewrites the sidecar entry to repo-b — every subsequent list request hits liveByNumber.get(5), {...liveEntry} wins, and the newest binding renders repo-a's URL.
Witness (verifier probe against the unmodified built dist): sidecar upserted repo-a #5 then re-bound to repo-b #5 (source:'create'), live left at repo-a → merged prs: [{"number":5,"url":"https://github.com/repo-a/r/pull/5"}] (stale live URL wins); the same probe with the one-line fix below → url repo-b (correct), both arms verified.
Resolve URL conflicts in favor of the persisted entry — the merge's own comment names the sidecar "the append-only binding-time record" (the opposite race is bounded to one request because the bind route invalidates the session-list cache in its finally):
return liveEntry
? {
...liveEntry,
url: p.url,
...(p.state !== undefined && p.url === liveEntry.url
? { state: p.state }
: {}),中文说明
[Critical] R14-2：合并逻辑对两个存储中都存在的编号无条件展开 ...liveEntry，因此当仅写 sidecar 的写入方（shell 钩子/回填）把某个 PR 编号重新绑定到另一个仓库时，每次列表响应都会保留过期活条目的 url，而持久化顺序已正确把重绑定排在最后——徽章会链接到错误仓库的 PR，直到 daemon 重启或该会话再次发生元数据绑定。只有 state 覆盖有 p.url === liveEntry.url 守卫，URL 方向毫无防护。shell 钩子自己的注释就描述了这种重绑定形态（"指向其他仓库的同编号条目……在此被重新绑定"），而 emitSessionPrBound 只是通知——之后没有任何东西对账活条目（三处合并调用点都是只读的响应构建器）。失败场景：会话先经元数据路由绑定 repo-a 的 PR #5（两存储均为 repo-a）；同一会话里 gh pr create 创建了 repo-b 的 PR #5，shell 后置钩子把 sidecar 改写为 repo-b——此后每次列表请求命中 liveByNumber.get(5)，{...liveEntry} 胜出，最新绑定渲染的是 repo-a 的 URL。证据（验证探针驱动未改动的构建产物）：先写 repo-a #5 再重绑定为 repo-b #5 后，合并结果为 repo-a 的 URL（过期活 URL 胜出）；应用下方单行修复后为 repo-b（正确），两侧均验证。建议：URL 冲突时以持久化条目为准——合并逻辑自己的注释就说 sidecar 是"按绑定时间追加的权威记录"（反向竞态只影响一次请求，因为绑定路由在 finally 里会使会话列表缓存失效）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| [preRunPrSnapshot, preRunBranch, preRunRepoKeys] = await Promise.all([ | ||
| fetchCurrentBranchPullRequest(cwd, preRunGhEnv), | ||
| fetchCurrentBranchName(cwd), | ||
| fetchAttributionRepoKeys(cwd), |
There was a problem hiding this comment.
[Critical] R14-3: the pre-run repo-identity verification leg is the only gh leg called WITHOUT the command's inline GH_*/GITHUB_* credentials (preRunGhEnv), although fetchAttributionRepoKeys declares an env? parameter and the comment two lines above requires every verification leg to authenticate the way the create itself does.
Agent runs GH_TOKEN=x gh pr create --fill on a host with no ambient gh auth — the exact token shape this PR added ghPrCreateInlineEnv to support. The create succeeds (the spawned command carries the token), but gh repo view --json url,parent runs bare, fails authentication, resolves {}, and bindGhPrCreate's repo gate (createdRepoKey !== preRunRepoKeys.resolved && !== .parent, both undefined) declines — the session's PR is created but never bound, silently, every time. Backfill cannot recover it (transcript traces were deliberately removed as a source).
Witness (verifier probe): RAW gh repo view (no token): exit 4 | gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable; fetchAttributionRepoKeys(repo) [no env arg] => {}; bindGhPrCreate repo gate declines? true; the fix shape resolves {resolved: 'github.com/cli/cli'}. Note: threading the PARTIAL record alone is not sufficient — see R14-4; pass a full-environment overlay.
fetchAttributionRepoKeys(cwd, ghEnv), // ghEnv = { ...process.env, ...preRunGhEnv }中文说明
[Critical] R14-3：预运行仓库身份验证腿是唯一没有传入命令内联 GH_*/GITHUB_* 凭据（preRunGhEnv）的 gh 腿，尽管 fetchAttributionRepoKeys 声明了 env? 参数、且上方两行的注释要求每条验证腿都以与创建命令相同的方式认证。失败场景：在没有环境 gh 认证的主机上运行 GH_TOKEN=x gh pr create --fill——正是本 PR 新增 ghPrCreateInlineEnv 要支持的形态。创建本身成功（生成的命令自带 token），但 gh repo view --json url,parent 裸跑、认证失败、解析为 {}，bindGhPrCreate 的仓库门（两个值均为 undefined）拒绝——会话的 PR 创建成功却永远不绑定，且静默发生。回填也恢复不了（transcript 痕迹已被有意移除）。证据（验证探针）：裸跑 gh repo view exit 4；fetchAttributionRepoKeys 无 env 参数返回 {}；仓库门拒绝；修复形态可解析出仓库键。注意：只传部分记录（{GH_TOKEN}）并不够——见 R14-4，请传完整环境叠加。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // advertised token shape binds nothing. | ||
| preRunGhEnv = ghPrCreateInlineEnv(commandToExecute); | ||
| [preRunPrSnapshot, preRunBranch, preRunRepoKeys] = await Promise.all([ | ||
| fetchCurrentBranchPullRequest(cwd, preRunGhEnv), |
There was a problem hiding this comment.
[Critical] R14-4: the inline gh env is passed as a PARTIAL record ({GH_TOKEN: …} extracted by ghPrCreateInlineEnv), but gitEnv(base) in git-branches.ts spreads base ?? process.env as the child's ENTIRE environment — so the gh verification legs spawn with no PATH, HOME, or any other process variable. execFile('gh') then fails to resolve the binary outside libc's /bin:/usr/bin fallback (ENOENT), and even a gh in /usr/bin dies with "unable to find git executable in PATH". The same defect hits the post-run leg (~3145).
Every inline-token gh pr create — including the clean GH_TOKEN=t0ken gh pr create --fill shape the new unit test advertises — spawns its verification legs with env keys [GH_TOKEN, LANG, LC_ALL] only; the pre-run leg resolves {status:'error'} and bindGhPrCreate declines at the errored-snapshot arm (~3140), so the advertised token shape binds nothing. The test pins the broken shape (toHaveBeenCalledWith('/test/dir', { GH_TOKEN: 't0ken' })) with execFile mocked throughout, hiding the PATH loss.
Witness (verifier probe, fake gh on a PATH-only location): ARM A (env=undefined, PATH inherited): {status:'pr',number:77} / ARM B (env={GH_TOKEN:x}, the reviewed shape): {status:'error'} / ARM C (env=process.env+GH_TOKEN): {status:'pr',number:77}; also env -i GH_TOKEN=… /usr/bin/gh repo view → "unable to find git executable in PATH".
Merge the inline assignments onto the full process env before passing (gitEnv still strips the repo-shifting GIT_*/GH_REPO vars):
const ghEnv = preRunGhEnv ? { ...process.env, ...preRunGhEnv } : undefined;
// pass ghEnv to both fetchCurrentBranchPullRequest legs and fetchAttributionRepoKeys中文说明
[Critical] R14-4：内联 gh 环境是以部分记录（ghPrCreateInlineEnv 提取出的 {GH_TOKEN: …}）传入的，但 git-branches.ts 的 gitEnv(base) 把 base ?? process.env 整体作为子进程的全部环境——于是各 gh 验证腿生成时没有 PATH、HOME 等任何进程变量。execFile('gh') 在 libc 的 /bin:/usr/bin 回退之外无法解析可执行文件（ENOENT），即便 gh 在 /usr/bin 也会报 "unable to find git executable in PATH"。同一缺陷也命中后置验证腿（约 3145 行）。失败场景：任何内联 token 的 gh pr create——包括新单测展示的标准形态 GH_TOKEN=t0ken gh pr create --fill——其验证腿的子进程环境只有 [GH_TOKEN, LANG, LC_ALL]；预运行腿解析为 {status:'error'}，bindGhPrCreate 在错误快照分支（约 3140 行）拒绝——宣传的 token 形态什么都绑定不了。单测因全程 mock execFile 而钉住了这个坏形态，掩盖了 PATH 丢失。证据（验证探针，gh 放在仅 PATH 可达的位置）：ARM A（env=undefined，继承 PATH）→ {status:'pr'}；ARM B（env={GH_TOKEN:x}，即被审查形态）→ {status:'error'}；ARM C（env=process.env+GH_TOKEN）→ {status:'pr'}。建议：传入前把内联赋值合并到完整进程环境上（见上方代码），并同时修复后置腿与 fetchAttributionRepoKeys。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const GH_INLINE_ENV_ASSIGNMENT_PATTERN = | ||
| /^(GH_[A-Za-z0-9_]*|GITHUB_[A-Za-z0-9_]*)=(\S+)$/; |
There was a problem hiding this comment.
[Critical] R14-5: ghPrCreateInlineEnv captures inline assignment values as raw shell text — this pattern keeps surrounding quotes and never expands $VAR/${VAR} — so the gh verification legs authenticate with a different credential than the real gh pr create used, and the binding silently misses.
A session with no ambient gh auth runs GH_TOKEN="$SECRET" gh pr create --fill or GH_TOKEN=$CI_TOKEN gh pr create --fill (both pass the segment gate). The capture returns GH_TOKEN with literal quotes / the unexpanded $CI_TOKEN; gitEnv spreads the record verbatim into execFile's env (Node performs no shell unquoting/expansion), so gh pr view/gh repo view receive the literal text, fail auth, the pre-run snapshot resolves {status:'error'}, and bindGhPrCreate declines — the successfully created PR is never bound. Quoted literals and variable references are the common shapes for inline tokens in CI.
Witness (verifier probe on the committed function): GH_TOKEN="$SECRET" gh pr create --fill → capture {"GH_TOKEN":"\"$SECRET\""}; GH_TOKEN=$CI_TOKEN … → {"GH_TOKEN":"$CI_TOKEN"}; driving the committed fetchCurrentBranchPullRequest with those captures (ambient auth removed) → {status:'error'} for both arms; the candidate fix (strip matching quotes + expand $VAR + full-env overlay) flipped both arms to {status:'none'}.
Strip one layer of matching surrounding quotes from the captured value; for values containing $, backticks, or quotes that cannot be resolved, decline the extraction (return undefined) or expand $VAR/${VAR} at the call site against the child's effective environment.
中文说明
[Critical] R14-5：ghPrCreateInlineEnv 以原始 shell 文本捕获内联赋值——该模式保留引号、且从不展开 $VAR/${VAR}——导致 gh 验证腿使用的凭据与真实 gh pr create 所用的不同，绑定静默丢失。失败场景：无环境 gh 认证的会话运行 GH_TOKEN="$SECRET" gh pr create --fill 或 GH_TOKEN=$CI_TOKEN gh pr create --fill（两者都通过段闸门）。捕获结果分别是带字面引号的 GH_TOKEN / 未展开的 $CI_TOKEN；gitEnv 原样把它铺进 execFile 的环境（Node 不做 shell 去引号/展开），于是 gh pr view/gh repo view 收到字面文本、认证失败，预运行快照解析为 {status:'error'}，bindGhPrCreate 拒绝——成功创建的 PR 永不绑定。引号字面量与变量引用正是 CI 中内联 token 的常见形态。证据（验证探针，作用于已提交函数）：两种形态分别捕获为 {"GH_TOKEN":"\"$SECRET\""} 与 {"GH_TOKEN":"$CI_TOKEN"}；用这些捕获值驱动真实 fetchCurrentBranchPullRequest（移除环境认证）→ 两臂均 {status:'error'}；候选修复（去匹配引号 + 展开 $VAR + 完整环境叠加）使两臂翻转为 {status:'none'}。建议：对捕获值剥去一层匹配的成对引号；对含 $、反引号或无法解析引号的值，拒绝提取（返回 undefined），或在调用处按子进程的有效环境展开 $VAR/${VAR}。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (previousWasFlag) { | ||
| // A flag's value (`sudo -u runner`), not the binary. | ||
| previousWasFlag = false; | ||
| continue; | ||
| } | ||
| break; |
There was a problem hiding this comment.
[Critical] R14-6: the ghPrCreateInlineEnv token scan breaks on any non-GH_*/GITHUB_* assignment token, but the gate grammar it claims to mirror accepts arbitrary leading assignments ((?:[A-Za-z_][A-Za-z0-9_]*=\S+\s+)*) — so one non-GH assignment prefixing the GH ones makes the extraction return undefined while commandRunsGhPrCreate still fires.
A session with no ambient gh auth runs CI=true GH_TOKEN=ghp_x gh pr create --fill. The gate returns true (verified against the exact segment regex), but the scan breaks at CI=true (not a GH assignment, not the binary, not a wrapper/flag → the terminating break below) and never captures GH_TOKEN; the verification legs run on ambient auth only, gh exits with an auth error, the pre-run snapshot becomes {status:'error'}, and bindGhPrCreate declines — the created PR is silently unbound. The only negative test (FOO=bar gh pr create --fill, no GH assignment at all) does not cover the mixed-prefix shape, so this stays green.
Witness (verifier probe on committed code): {command:'CI=true GH_TOKEN=ghp_x gh pr create --fill', gate:true, env:null}; the bare-auth leg → {status:'error'}; candidate fix (continue past NAME=value tokens instead of breaking) → captured env keys ['GH_TOKEN'], leg → {status:'none'}.
In the scan, consume any token matching the gate's assignment class (/^[A-Za-z_][A-Za-z0-9_]*=\S+$/) with continue (capturing only GH_/GITHUB_ ones) instead of breaking, so the scan and the gate admit the same prefix grammar.
中文说明
[Critical] R14-6：ghPrCreateInlineEnv 的 token 扫描遇到任何非 GH_*/GITHUB_* 赋值 token 就 break，但它声称对齐的闸门文法接受任意前导赋值——于是 GH 赋值之前多一个非 GH 赋值，提取就返回 undefined，而 commandRunsGhPrCreate 仍然触发。失败场景：无环境 gh 认证的会话运行 CI=true GH_TOKEN=ghp_x gh pr create --fill：闸门返回 true（已按确切段正则验证），但扫描在 CI=true 处 break（下方这个终结 break），永远捕获不到 GH_TOKEN；验证腿只能用环境认证裸跑、gh 认证失败、预运行快照变为 {status:'error'}，bindGhPrCreate 拒绝——创建出的 PR 静默不绑定。唯一的负例测试（FOO=bar gh pr create --fill，完全无 GH 赋值）不覆盖混合前缀形态，因此保持绿色。证据（验证探针，作用于已提交代码）：{gate:true, env:null}；裸认证腿 {status:'error'}；候选修复（对 NAME=value token 用 continue 而非 break）→ 捕获到 ['GH_TOKEN']，腿 → {status:'none'}。建议：扫描对匹配闸门赋值类的任意 token 用 continue 消费（只捕获 GH_/GITHUB_），使扫描与闸门接受同一前缀文法。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const prPath = sessionService.getPrSessionPathForArchiveState( | ||
| candidate.sessionId, | ||
| candidate.archiveState, | ||
| ); |
There was a problem hiding this comment.
[Critical] R14-7: backfill fixes each candidate's archive state at enumeration time (candidate.archiveState) and never re-checks it before writing, so an archive/restore transition landing during the scan+gh window strands the new bindings in the WRONG archive state's chats dir — an orphan sidecar no reader of the session's current state ever reads. The sibling shell binder explicitly re-resolves its location for this exact race ("an archive transition landing during the gh round-trip above must not strand the binding", shell.ts); this writer does not, and the grep finds no getSessionLocation call anywhere in this file.
POST /sessions/backfill-prs on a workspace with many transcripts: it collects all candidates first (reading every transcript fully), then runs the git/gh legs (~4s+), then writes per candidate. During that window the user archives session S — moveSessionPrSidecar moves the sidecar to the archived dir — but the write phase still holds archiveState: 'active' for S: readSessionPrs on the now-absent active path returns null (freeSlots = 10), and upsertSessionPrs → withSidecarLock materializes and writes a brand-new ACTIVE sidecar for an archived session. The badge reads the archived path and never shows the bindings; moveSessionPrSidecar's merge-heal only fires on S's NEXT transition, which an archived session may never have.
Witness (verifier probe — the mocked fetch performs the daemon's real archiveSessions inside the window): the session genuinely archived, backfill reported {scanned:1, bound:1, failed:0}, transcript in archive dir: true — but ACTIVE sidecar holds [{number:7,…,source:'worktree'}] and ARCHIVED sidecar: null (orphaned).
Re-resolve the session's current archive state just before the upsert (mirroring the shell binder's getSessionLocation re-resolve) and skip or re-target the write when it no longer matches the enumerated state.
中文说明
[Critical] R14-7：回填在枚举时就固定了每个候选的归档状态（candidate.archiveState），写入前不再复核；因此在"扫描+gh"窗口内发生的归档/恢复迁移，会把新绑定写进错误归档状态的 chats 目录——成为一个任何当前状态读取方都读不到的孤儿 sidecar。兄弟的 shell 绑定器正是为这个竞态显式重解析位置（shell.ts："gh 往返期间发生的归档迁移不得使绑定搁浅在复活的 active sidecar 上"）；本写入方没有，整个文件也搜不到任何 getSessionLocation 调用。失败场景：对含大量 transcript 的 workspace 触发 POST /sessions/backfill-prs：先收集全部候选（整读每个 transcript），再跑 git/gh 腿（约 4 秒以上），然后逐候选写入。窗口内用户归档会话 S——moveSessionPrSidecar 把 sidecar 移入归档目录——但写阶段仍持有 S 的 archiveState: 'active'：对已不存在的 active 路径 readSessionPrs 返回 null（freeSlots = 10），upsertSessionPrs → withSidecarLock 物化并为已归档会话写入一个全新的 ACTIVE sidecar。徽章读归档路径，永远看不到绑定；moveSessionPrSidecar 的合并修复只在 S 的下一次迁移时触发，而归档会话可能永远没有下一次。证据（验证探针——mock 的 fetch 在窗口内执行 daemon 真实的 archiveSessions）：会话确实被归档，回填报告 {scanned:1, bound:1, failed:0}，transcript 已在归档目录——但 ACTIVE sidecar 持有 [{number:7,…}]，ARCHIVED sidecar 为 null（被孤儿化）。建议：在 upsert 前重解析会话当前归档状态（对齐 shell 绑定器的 getSessionLocation 重解析），与枚举状态不符时跳过或改写目标。
— qwen3.8-max via Qwen Code /review (v0.22.0)
The pr-<N> slug reservation (review R1-21) also rejected the legitimate
PR-backed path: setupStartupWorktree("QwenLM#42") routes through
createUserWorktree, whose internal slug validation reserved the shape
for PR-backed worktrees while refusing to create one. Add a prBacked
option that lifts only the reservation check; user-chosen slugs keep it.
… feat/session-pr-gh-create
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: test-efficacy — probe harness could not self-validate (probe baseline collected nothing; vitest dist prerequisite in the probe tree).
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round:
docs/design/2026-08-20-webshell-session-pr-binding.md:69 — [review] design doc describes a re-offer-based eviction policy the…packages/acp-bridge/src/bridge.ts:9960 — [review] no-op guard keyed on the LATEST entry only —…packages/acp-bridge/src/bridge.ts:10027 — [review] setSessionPrs reconciles AFTER the event publish/catalog…packages/cli/src/acp-integration/session/Session.ts:8254 — [review] Session-side pr-bound callback registration + dispose…packages/cli/src/serve/acp-http/transport.test.ts:5561 — [review] setSessionPrs reconciliation assertion cannot distinguish…packages/cli/src/serve/fast-path-open.test.ts:113 — [review] vi.waitFor 1s→10s flake fix applied to 3 of 7 polls; the…packages/cli/src/serve/routes/session-pr-backfill.test.ts:208 — [review] env argument pinned only on the fetchGitHubPullRequests…packages/cli/src/serve/routes/session-pr-backfill.test.ts:749 — [review] run-4 test pins starvation: with one free slot the offer…packages/cli/src/serve/routes/session-pr-backfill.test.ts:782 — [review] no positive /review #N test although the pattern carries a…packages/cli/src/serve/routes/session-pr-backfill.test.ts:1255 — [review] backfill failed counter / unwritable-sidecar…packages/cli/src/serve/routes/session-pr-backfill.ts:184 — [review] dead DI surface: fetchPullRequests/fetchRepoKeys optional…packages/cli/src/serve/routes/session-pr-backfill.ts:513 — [review] backfill route registers non-strict mutate() while the…packages/cli/src/serve/routes/session.ts:2275 — [review] no positive state test through either metadata route…packages/cli/src/serve/run-qwen-serve.ts:6093 — [review] daemon wiring of the state-refresh sweep untested…packages/cli/src/serve/run-qwen-serve.ts:6100 — [review] dynamic import of session-pr-refresh.js has no .catch — an…packages/cli/src/serve/server/session-pr-refresh.test.ts:111 — [review] sweep suite never exercises the 'archived' archive-state…packages/cli/src/serve/server/session-pr-refresh.test.ts:186 — [review] sweep suite pins the credential-bearing env argument as…packages/cli/src/serve/server/session-pr-refresh.test.ts:437 — [review] "one slim gh pr list per workspace per sweep" has no…packages/cli/src/serve/server/session-pr-refresh.ts:48 — [review] resolveSessionPrRefreshIntervalMs clamps only the upper…packages/cli/src/serve/server/session-pr-refresh.ts:161 — [review] one poisoned sidecar aborts the entire workspace sweep (no…- …and 22 more (see the run report)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had 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.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：test-efficacy — probe harness could not self-validate (probe baseline collected nothing; vitest dist prerequisite in the probe tree)。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
收敛姿态下延后（第 15 轮，非阻断）——已记录，本轮不要求修改：共 42 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| !persistedNumbers.has(liveEntry.number) && | ||
| ordered.length < SESSION_PR_LIST_LIMIT | ||
| ) { | ||
| ordered.push(liveEntry); | ||
| } |
There was a problem hiding this comment.
[Critical] R13-1 (round 13 — re-checked at this commit, still stands): the live-only append loop gates on the running merged length (ordered.length < SESSION_PR_LIST_LIMIT) instead of the persisted list's length, so below the cap it drops the session's NEWEST live-only bindings once the running total reaches the cap — contradicting the comment directly above it ("below it a live-only entry is genuinely the newest").
Failure scenario: with the persisted list below the cap (e.g. 8 entries) and several live-only bindings present, the loop stops appending once the running total hits 10 — the newest bindings are dropped, while the final slice(-SESSION_PR_LIST_LIMIT) would have kept them and evicted the oldest persisted entries instead, per the sidecar's binding-time order the same comment appeals to.
Witness: not run — ledger re-check: the merged-length gate is present unchanged in the rewritten merge at the reviewed commit.
| if ( | |
| !persistedNumbers.has(liveEntry.number) && | |
| ordered.length < SESSION_PR_LIST_LIMIT | |
| ) { | |
| ordered.push(liveEntry); | |
| } | |
| if (!persistedNumbers.has(liveEntry.number)) { | |
| ordered.push(liveEntry); | |
| } |
中文说明
[Critical] R13-1（第 13 轮提出——本轮在该提交上复查，仍然成立）：仅存在于 live 条目的追加循环以“合并后的当前长度”（ordered.length < SESSION_PR_LIST_LIMIT）为闸门，而不是以持久化列表的长度为闸门——因此在未达上限时，一旦累计长度达到上限就会丢弃会话最新的 live-only 绑定，与正上方注释（“未达上限时，live-only 条目确实是最新的”）直接矛盾。
触发场景：持久化列表未达上限（例如 8 条）且存在多个 live-only 绑定时，循环在累计达到 10 时停止追加——最新的绑定被丢弃；而末尾的 slice(-SESSION_PR_LIST_LIMIT) 本可以保留它们、改为逐出最旧的持久化条目，这才符合该注释所援引的 sidecar 绑定时间顺序。
证据：未执行——账本复查：被重写后的合并逻辑中该“累计长度”闸门原样存在。
建议修复：改为不以累计长度为闸门追加（让末尾的 slice 负责逐出最旧条目）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ((!result.aborted || wasPromoteRefused) && result.exitCode === 0) { | ||
| this.bindGhPrCreate( |
There was a problem hiding this comment.
[Critical] R13-2 (round 13 — re-checked at this commit, still stands): a SUCCESSFUL promote and any is_background: true gh pr create never reach bindGhPrCreate — this sole call site sits AFTER the if (result.promoted) early return above, and executeBackground never calls it — yet this PR removed the transcript gh pr create backfill source, so these creates are permanently unbound.
Failure scenario: the user presses Ctrl+B mid-run and the promote succeeds (or the agent passes is_background: true); the child exits 0 having created the PR; no sidecar write, no emitSessionPrBound, and backfill deliberately never attributes gh pr create traces — the session badge never shows the PR this session created. The design update accepts the pure-background shape as a limitation, but not the successful-promote shape.
Witness: not run — ledger re-check: sole call site verified after the promoted early return at the reviewed commit; corroborated this round by both convergence-pair auditors tracing the same call graph.
Suggested fix: capture the same pre-run snapshot in executeBackground / the promote settle hook and run the binding gate when the entry settles with exit code 0, or refuse gh pr create-shaped commands in background mode the way git commit is refused.
中文说明
[Critical] R13-2（第 13 轮提出——本轮在该提交上复查，仍然成立）：成功的 promote 以及任何 is_background: true 的 gh pr create 都永远到不了 bindGhPrCreate——这个唯一调用点位于上方 if (result.promoted) 提前返回之后，而 executeBackground 从不调用它——但本 PR 又移除了转录中的 gh pr create 回填来源，因此这类创建永久无法绑定。
触发场景：用户在运行中按 Ctrl+B 且 promote 成功（或 agent 传入 is_background: true）；子进程以退出码 0 结束并已创建 PR；没有 sidecar 写入、没有 emitSessionPrBound，而 backfill 又刻意不归因 gh pr create 痕迹——该会话的徽章永远不会显示这个 PR。设计更新仅把“纯后台”形态列为已知限制，并未包含“promote 成功”形态。
证据：未执行——账本复查：在审查提交上确认唯一调用点位于 promoted 提前返回之后；本轮两个收敛对审计者追踪同一调用图再次印证。
建议修复：在 executeBackground / promote settle 钩子中捕获同样的运行前快照，并在条目以退出码 0 结束时执行绑定闸门；或像拒绝 git commit 一样，在后台模式拒绝 gh pr create 形态的命令。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| url = `${sources.remote}/pull/${number}`; | ||
| } | ||
| } | ||
| const state = sources.pageStateByNumber.get(number); |
There was a problem hiding this comment.
[Critical] R13-3 (round 13 — re-checked at this commit, still stands): bindCandidateNumbers stamps state from the gh page by PR number alone, regardless of which repo the resolved url belongs to.
Failure scenario: in the fork layout (fork-trust branch holding, gh page listing the PARENT's PRs) a /review <fork-url> form resolves the fork URL correctly but pairs it with the parent repo's same-numbered PR's state — a DIFFERENT PR, since numbers are per-repo; the sidecar then carries a state that belongs to another repository's PR.
Witness: not run — ledger re-check: const state = sources.pageStateByNumber.get(number) at the reviewed commit carries no repo check against the resolved url; the URL form path can resolve a fork URL while the page map is the parent's.
Suggested fix: key the state lookup on (repoKey, number) — only consume pageStateByNumber when the resolved url's repo key matches the page's repo key.
中文说明
[Critical] R13-3（第 13 轮提出——本轮在该提交上复查，仍然成立）：bindCandidateNumbers 仅凭 PR 编号从 gh 页面打上 state，不管解析出的 url 属于哪个仓库。
触发场景：fork 布局下（fork 信任分支成立、gh 页面列出的是父仓库的 PR），/review <fork-url> 形态能正确解析出 fork 的 URL，却配上父仓库同号 PR 的状态——编号按仓库独立计数，那是另一个 PR；sidecar 因此带上了属于另一个仓库 PR 的状态。
证据：未执行——账本复查：审查提交上的 const state = sources.pageStateByNumber.get(number) 对解析出的 url 没有任何仓库校验；URL 形态路径可能解析出 fork URL，而页面映射却是父仓库的。
建议修复：以 (仓库键, 编号) 作为状态查找的键——仅当解析出的 url 的仓库键与页面仓库键一致时才消费 pageStateByNumber。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| /^pr-[1-9]\d{0,8}$/.test(slug) && | ||
| options?.allowPrBackedShape !== true | ||
| ) { |
There was a problem hiding this comment.
[Critical] R14-1 (re-checked at this commit — the creation half is fixed by prBacked: isPullRequest; the exit_worktree half still stands): the new pr-<number> reservation sits in the shared validateUserWorktreeSlug, and the unmodified ExitWorktreeTool.validateToolParams (exit-worktree.ts:596) calls it without allowPrBackedShape — so exit_worktree now rejects the exact PR-backed worktrees this PR's own --worktree=#<N> flow creates.
Failure scenario: a session launched with qwen --worktree=#42 runs inside the pr-42 worktree; when the user asks to leave or clean up, the agent calls exit_worktree({name: 'pr-42'}) and validation returns "Worktree name must not look like "pr-"" — for a worktree that IS PR-backed. Headless sessions have no WorktreeExitDialog fallback; only manual git worktree remove remains. exit_worktree never creates slugs, so the squatting concern cannot apply.
Witness: probe — ExitWorktreeTool.validateToolParams({name:'pr-42', action:'keep'}) → "Worktree name must not look like \"pr-<number>\": that shape is reserved for PR-backed worktrees."; control my-feature → null; fix arm (allowPrBackedShape: true) → null. Source: [probe]
| if ( | |
| /^pr-[1-9]\d{0,8}$/.test(slug) && | |
| options?.allowPrBackedShape !== true | |
| ) { | |
| if ( | |
| /^pr-[1-9]\d{0,8}$/.test(slug) && | |
| options?.allowPrBackedShape !== true && | |
| !options?.allowExistingPrBackedShape | |
| ) { |
(or simply pass { allowPrBackedShape: true } from ExitWorktreeTool.validateToolParams)
中文说明
[Critical] R14-1（本轮复查——创建路径的一半已由 prBacked: isPullRequest 修复；exit_worktree 的一半仍然成立）：新的 pr-<number> 保留位位于共享的 validateUserWorktreeSlug 中，而未被本 PR 修改的 ExitWorktreeTool.validateToolParams（exit-worktree.ts:596）调用它时不带 allowPrBackedShape——于是 exit_worktree 现在会拒绝本 PR 自己的 --worktree=#<N> 流程所创建的 PR 工作树。
触发场景：以 qwen --worktree=#42 启动的会话运行在 pr-42 工作树中；用户要求退出或清理时，agent 调用 exit_worktree({name: 'pr-42'})，校验返回“工作树名不能形如 pr-”——而它正是 PR 工作树。无头会话没有 WorktreeExitDialog 兜底，只剩手工 git worktree remove。exit_worktree 从不创建 slug，因此抢占（squatting）顾虑在这里不可能成立。
证据：探针——ExitWorktreeTool.validateToolParams({name:'pr-42', action:'keep'}) → 返回保留位错误；对照 my-feature → null；修复分支（allowPrBackedShape: true）→ null。Source: [probe]
建议修复：在 ExitWorktreeTool.validateToolParams 中传入 { allowPrBackedShape: true }（任何已存在且形如该模式的工作树必然就是 PR 工作树）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| return liveEntry | ||
| ? { | ||
| ...liveEntry, | ||
| ...(p.state !== undefined && p.url === liveEntry.url | ||
| ? { state: p.state } | ||
| : {}), | ||
| } | ||
| : p; |
There was a problem hiding this comment.
[Critical] R14-2 (round 14 — re-checked at this commit, still stands): the merge spreads ...liveEntry unconditionally for numbers present in both stores, so when a sidecar-only writer (the shell hook / backfill) re-binds a PR number to a DIFFERENT repository, every list response keeps the stale live entry's url while the persisted order correctly puts the re-bind last — the badge links to the wrong repository's PR until daemon restart or another live re-bind.
Only state carries the p.url === liveEntry.url guard; the url itself always comes from the live entry, although the sidecar is the authoritative binding-time record.
Witness: not run — ledger re-check: the unconditional ...liveEntry spread for matching numbers is present in mergeLiveSessionSummary at the reviewed commit; the same-URL guard exists only on the state stamp.
Suggested fix: when the persisted url differs from the live url, prefer the persisted entry (or at least its url) — the sidecar is authoritative for re-binds.
中文说明
[Critical] R14-2（第 14 轮提出——本轮在该提交上复查，仍然成立）：合并时对两个存储中都存在的编号无条件展开 ...liveEntry——当仅写 sidecar 的写入方（shell 钩子 / backfill）把某个 PR 编号重新绑定到另一个仓库时，所有列表响应仍保留过期 live 条目的 url，而持久化顺序却正确地把重绑放在最后——徽章会链接到错误仓库的 PR，直到守护进程重启或再次 live 重绑。
只有 state 带有 p.url === liveEntry.url 守卫；url 本身永远取自 live 条目——尽管 sidecar 才是权威的绑定时间记录。
证据：未执行——账本复查：审查提交上的 mergeLiveSessionSummary 中对匹配编号的无条件 ...liveEntry 展开原样存在；同 URL 守卫只作用于 state 的打印。
建议修复：当持久化 url 与 live url 不同时，优先使用持久化条目（至少其 url）——重绑应以 sidecar 为准。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| fetchCurrentBranchPullRequest(cwd, preRunGhEnv), | ||
| fetchCurrentBranchName(cwd), | ||
| fetchAttributionRepoKeys(cwd), |
There was a problem hiding this comment.
[Critical] R14-3 (round 14 — re-checked at this commit, still stands): the pre-run repo-identity verification leg is the only gh leg called WITHOUT the command's inline GH_*/GITHUB_* credentials (preRunGhEnv), although fetchAttributionRepoKeys declares an env? parameter and the comment two lines above requires every verification leg to authenticate the way the create itself does.
Failure scenario: on a daemon/CI host with no ambient gh auth, GH_TOKEN=x gh pr create --fill runs; gh repo view --json url,parent errors unauthenticated, fetchAttributionRepoKeys resolves {}, and the repo-key gate in bindGhPrCreate declines — every legitimate inline-token create silently never binds. The backfill route's identical call correctly passes the workspace env.
Witness: probe (sanitized env) — fetchAttributionRepoKeys(no env) → {}; raw gh spawn exits 4 with "To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable." Source: [probe]
| fetchCurrentBranchPullRequest(cwd, preRunGhEnv), | |
| fetchCurrentBranchName(cwd), | |
| fetchAttributionRepoKeys(cwd), | |
| fetchCurrentBranchPullRequest(cwd, preRunGhEnv), | |
| fetchCurrentBranchName(cwd), | |
| fetchAttributionRepoKeys(cwd, preRunGhEnv), |
中文说明
[Critical] R14-3（第 14 轮提出——本轮在该提交上复查，仍然成立）：运行前的仓库身份校验分支是唯一没有携带命令内联 GH_*/GITHUB_* 凭证（preRunGhEnv）的 gh 分支——尽管 fetchAttributionRepoKeys 声明了 env? 参数，且上方两行的注释要求每个校验分支都以与 create 本身相同的方式鉴权。
触发场景：在没有环境级 gh 鉴权的守护进程/CI 主机上执行 GH_TOKEN=x gh pr create --fill；gh repo view --json url,parent 因未鉴权而报错，fetchAttributionRepoKeys 解析为 {}，bindGhPrCreate 中的仓库键闸门拒绝——每一个合法的内联令牌创建都静默地永远无法绑定。backfill 路由中同样的调用正确地传入了工作区 env。
证据：探针（净化环境）——fetchAttributionRepoKeys(无 env) → {}；裸 gh 派生以退出码 4 报 “To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable.”。Source: [probe]
建议修复：把 preRunGhEnv 也传给 fetchAttributionRepoKeys。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // The verification legs must authenticate the way the create itself | ||
| // does (inline GH_TOKEN with no ambient gh auth), or the gate's | ||
| // advertised token shape binds nothing. | ||
| preRunGhEnv = ghPrCreateInlineEnv(commandToExecute); |
There was a problem hiding this comment.
[Critical] R14-4 (round 14 — re-checked at this commit, still stands): the inline gh env is passed as a PARTIAL record ({GH_TOKEN: …} extracted by ghPrCreateInlineEnv), but gitEnv(base) spreads base ?? process.env as the child's ENTIRE environment — so the gh verification legs spawn with no PATH, HOME, or any other process variable.
Failure scenario: ghPrCreateInlineEnv('GH_TOKEN=x gh pr create --fill') yields {GH_TOKEN:'x'}; gitEnv({GH_TOKEN:'x'}) yields only {GH_TOKEN, LC_ALL, LANG}; real gh under that env exits non-zero with "unable to find git executable in PATH", the snapshot resolves {status:'error'}, and bindGhPrCreate declines — every inline-token create (the exact shape the adjacent comment advertises) silently never binds, on every host, even after R14-3's fix passes env to the repo-keys leg.
Witness: probe — ARM A (PR code shape, partial inline env): {"status":"error"} / ARM B (merged over process.env): {"status":"none"} / raw partial-env gh spawn stderr: unable to find git executable in PATH; please install git before retrying (exit 1). Source: [probe]
| preRunGhEnv = ghPrCreateInlineEnv(commandToExecute); | |
| const inlineGhEnv = ghPrCreateInlineEnv(commandToExecute); | |
| preRunGhEnv = inlineGhEnv ? { ...process.env, ...inlineGhEnv } : undefined; |
中文说明
[Critical] R14-4（第 14 轮提出——本轮在该提交上复查，仍然成立）：内联 gh env 以部分记录传入（ghPrCreateInlineEnv 提取出的 {GH_TOKEN: …}），但 gitEnv(base) 会把 base ?? process.env 作为子进程的全部环境展开——于是 gh 校验分支派生时没有 PATH、HOME 或任何其他进程变量。
触发场景：ghPrCreateInlineEnv('GH_TOKEN=x gh pr create --fill') 得到 {GH_TOKEN:'x'}；gitEnv({GH_TOKEN:'x'}) 只得到 {GH_TOKEN, LC_ALL, LANG}；真实 gh 在该环境下以非零退出并报 “unable to find git executable in PATH”，快照解析为 {status:'error'}，bindGhPrCreate 拒绝——每一个内联令牌创建（恰是相邻注释宣称支持的形态）在任何主机上都静默地永远无法绑定；即便 R14-3 修复后把 env 传给了仓库键分支也无济于事。
证据：探针——ARM A（PR 代码形态，部分内联 env）：{"status":"error"} / ARM B（并入 process.env）：{"status":"none"} / 裸部分 env 的 gh 派生 stderr：unable to find git executable in PATH; please install git before retrying（退出码 1）。Source: [probe]
建议修复：在调用点把内联赋值并入进程环境（{ ...process.env, ...inlineGhEnv }）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const GH_INLINE_ENV_ASSIGNMENT_PATTERN = | ||
| /^(GH_[A-Za-z0-9_]*|GITHUB_[A-Za-z0-9_]*)=(\S+)$/; |
There was a problem hiding this comment.
[Critical] R14-5 (round 14 — re-checked at this commit, still stands): ghPrCreateInlineEnv captures inline assignment values as raw shell text — this pattern keeps surrounding quotes and never expands $VAR/${VAR} — so the gh verification legs authenticate with a different credential than the real gh pr create used, and the binding silently misses.
Failure scenario: a session with no ambient gh auth runs GH_TOKEN="$SECRET" gh pr create --fill (or GH_TOKEN=${TOK}): the extraction yields the literal quoted/unexpanded string, the verification legs authenticate with it (or fail to), and the binding silently misses while the create itself succeeded.
Witness: probe — GH_TOKEN="$SECRET" gh pr create --fill → extracted {"GH_TOKEN":"\"$SECRET\""} (quotes kept, variable unexpanded). Source: [probe]
Suggested fix: expand the common shell forms (strip matching quotes, expand $VAR/${VAR} from the process environment) before handing the value to the verification legs.
中文说明
[Critical] R14-5（第 14 轮提出——本轮在该提交上复查，仍然成立）：ghPrCreateInlineEnv 把内联赋值的值当作原始 shell 文本捕获——该模式保留外层引号、且不展开 $VAR/${VAR}——于是 gh 校验分支用来鉴权的凭证与真实 gh pr create 使用的不同，绑定被静默漏掉。
触发场景：没有环境级 gh 鉴权的会话执行 GH_TOKEN="$SECRET" gh pr create --fill（或 GH_TOKEN=${TOK}）：提取得到的是带引号/未展开的字面量，校验分支用它鉴权（或鉴权失败），创建本身成功了，绑定却被静默漏掉。
证据：探针——GH_TOKEN="$SECRET" gh pr create --fill → 提取结果 {"GH_TOKEN":"\"$SECRET\""}（引号保留、变量未展开）。Source: [probe]
建议修复：在把值交给校验分支之前展开常见 shell 形态（剥离配对引号、从进程环境展开 $VAR/${VAR}）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (previousWasFlag) { | ||
| // A flag's value (`sudo -u runner`), not the binary. | ||
| previousWasFlag = false; | ||
| continue; | ||
| } | ||
| break; |
There was a problem hiding this comment.
[Critical] R14-6 (round 14 — re-checked at this commit, still stands): the ghPrCreateInlineEnv token scan breaks on any non-GH_*/GITHUB_* assignment token, but the gate grammar it claims to mirror accepts arbitrary leading assignments ((?:[A-Za-z_][A-Za-z0-9_]*=\S+\s+)*) — so one non-GH assignment prefixing the GH ones makes the extraction return undefined while commandRunsGhPrCreate still fires.
Failure scenario: a session with no ambient gh auth runs FOO=bar GH_TOKEN=x gh pr create --fill: the gate returns true, the scan breaks at FOO=bar, preRunGhEnv is undefined, the verification legs run unauthenticated and error, and the binding silently misses. Likewise gh pr create --web; GH_TOKEN=x gh pr create --fill — the first gate-matching segment shadows the later segment's token.
Witness: probe — FOO=bar GH_TOKEN=x gh pr create --fill → gate true, extraction null; gh pr create --web; GH_TOKEN=x gh pr create --fill → gate true, extraction null. Source: [probe]
Suggested fix: skip non-GH assignments instead of breaking, and scan every gate-matching segment (not only the first).
中文说明
[Critical] R14-6（第 14 轮提出——本轮在该提交上复查，仍然成立）：ghPrCreateInlineEnv 的令牌扫描遇到任何非 GH_*/GITHUB_* 的赋值令牌就会中断，但它声称对齐的闸门语法却接受任意前置赋值（(?:[A-Za-z_][A-Za-z0-9_]*=\S+\s+)*）——于是 GH 赋值前面只要有一个非 GH 赋值，提取就返回 undefined，而 commandRunsGhPrCreate 仍然触发。
触发场景：没有环境级 gh 鉴权的会话执行 FOO=bar GH_TOKEN=x gh pr create --fill：闸门返回 true，扫描在 FOO=bar 处中断，preRunGhEnv 为 undefined，校验分支无鉴权运行并报错，绑定被静默漏掉。同理 gh pr create --web; GH_TOKEN=x gh pr create --fill——第一个过闸门的段落遮蔽了后面段落的令牌。
证据：探针——FOO=bar GH_TOKEN=x gh pr create --fill → 闸门 true、提取 null；gh pr create --web; GH_TOKEN=x gh pr create --fill → 闸门 true、提取 null。Source: [probe]
建议修复：遇到非 GH 赋值时跳过而不是中断，并扫描每一个过闸门的段落（而不只是第一个）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ): Promise<void> { | ||
| const prPath = sessionService.getPrSessionPathForArchiveState( | ||
| candidate.sessionId, | ||
| candidate.archiveState, | ||
| ); |
There was a problem hiding this comment.
[Critical] R14-7 (round 14 — re-checked at this commit, still stands): backfill fixes each candidate's archive state at enumeration time (candidate.archiveState) and never re-checks it before writing, so an archive/restore transition landing during the scan+gh window strands the new bindings in the WRONG archive state's chats dir — an orphan sidecar no reader of the session's current state ever reads.
Failure scenario: the daemon archives a session while backfill is scanning it (archive is a documented routine bulk operation); the candidate's archiveState still says active, the gh round-trip lands, and upsertSessionPrs writes the active path of a now-archived session; listing/badge/sweep all resolve the sidecar by the session's CURRENT archive state, so the bindings never surface. The sibling shell binder explicitly re-resolves the archive location immediately before its locked mutation.
Witness: not run — ledger re-check: both prPath computations (pre-read at :375, write at :445) use candidate.archiveState captured at enumeration; no re-resolution exists before the write at the reviewed commit.
Suggested fix: re-resolve the archive state inside (or immediately before) the locked write, the way the shell binder does.
中文说明
[Critical] R14-7（第 14 轮提出——本轮在该提交上复查，仍然成立）：backfill 在枚举时固化每个候选的归档状态（candidate.archiveState），写入前不再复查——扫描+gh 往返窗口内若发生归档/恢复迁移，新绑定会被写到错误归档状态的 chats 目录，成为一个无人读取的孤儿 sidecar（所有读取方都按会话当前状态解析路径）。
触发场景：backfill 扫描期间守护进程归档了某会话（归档是被文档化的例行批量操作）；候选的 archiveState 仍是 active，gh 往返落定后 upsertSessionPrs 写入一个已归档会话的 active 路径；列表/徽章/刷新扫描都按会话当前归档状态解析 sidecar，这些绑定永远不会显现。同族的 shell 绑定器会在其加锁写入前显式重新解析归档位置。
证据：未执行——账本复查：两处 prPath 计算（:375 预读、:445 写入）都使用枚举时捕获的 candidate.archiveState；审查提交上写入前不存在任何重新解析。
建议修复：像 shell 绑定器一样，在加锁写入内部（或紧接其前）重新解析归档状态。
— 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 review round — PR #9739 (same-run verification repair)The previous commit of this round was REJECTED by deterministic verification: Rejection repaired
Why it was not pushed: Note: the base has since been auto-updated; the verdict below predates that update, and the next round's re-measurement may charge the round. tests failed in packages/cli 中文说明🤖 AutoFix 更新了一个过期的 base —— 修复未通过验证，但本 PR 落后于 验证门的拒绝原因与日志证据见上方英文部分（gate-rejection 不翻译）。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/32969394174 🧠 Handled by Qwen Code · model/模型 |
4 participants
What this PR does
Closes the last binding-source gap of the session↔PR feature: sessions whose PR was created by the agent running
gh pr createin the shell (instead of the Web Shell Git dialog) now get bound too. Two complementary paths share one detector. Live: after a foreground shell command completes, the shell tool recognizesgh pr create(excluding--dry-run), extracts the PR URL that gh prints on success, and writes the session's PR sidecar directly with stateopen— the same tool-process-writes-sidecar pattern as worktree sessions, so it works in both CLI and daemon modes and shows up in the sidebar within the existing ~2s list refresh. Retroactive: the on-demand backfill route gains a third source that pairs eachrun_shell_commandcall with its response in persisted transcripts (by part id) and applies the same detector, recovering PRs created before the live hook existed. Failed or dry-run creates print no URL, which is the false-positive gate on both paths.Why it's needed
Operators whose agents create PRs from the shell saw no PR badge on those sessions and could not search them by PR number — the original feature only bound GitDialog creations, and the branch-based backfill misses PRs whose head branch never appeared as a session git branch. The printed-URL source is authoritative for exactly this flow, live and historically.
Reviewer Test Plan
How to verify
gh.exe, non-create commands, dry-run/failure), the shell post-hook (mocked execution resolving with a PR URL writes the sidecar withstate: 'open'; failure output writes nothing), and the backfill retroactive source (a transcript with a pairedgh pr createcall/response binds the printed URL even when gh is unavailable).gh pr createagainst a scratch repo); the sidebar badge for that session appears within ~2s. Re-runPOST /sessions/backfill-prson a workspace with old transcripts containinggh pr createruns and confirmboundcounts them.Evidence (Before & After)
Before: a session that ran
gh pr createcarried noprsbinding (badge absent, search by PR number missed it) unless its git branch happened to be a PR head. After: the binding is written at create time (live) or by backfill (historical), with the same badge/search/tooltip behavior as GitDialog-bound sessions.Tested on
Environment
Repo vitest suites (core 33 + shell 305 + cli 27 targeted) plus build/typecheck/lint; macOS local.
Risk & Scope
gh pr createplus a PR-looking URL in output would bind; the URL requirement keeps this near-impossible in practice, and bindings are append-only/bounded (cap 10) with idempotent re-runs. The shell hook is best-effort and can never alter the tool result.Linked Issues
Follow-up to #9543 (session↔PR binding); stacked on #9729.
中文说明
这个 PR 做了什么
补齐会话↔PR 绑定的最后一个来源缺口：agent 在 shell 里
gh pr create创建的 PR（而非 Web Shell GitDialog）现在也能绑定。两条互补路径共享一个检测器。实时：前台 shell 命令完成后，shell 工具识别gh pr create（排除--dry-run），提取 gh 成功时打印的 PR URL，直接以 stateopen写会话 PR sidecar——与 worktree 会话相同的"工具进程直写 sidecar"模式，CLI/daemon 双模生效，约 2s 内在侧栏展示。回填：按需 backfill 路由新增第三来源，按 part id 配对 transcript 里run_shell_command的 call/response 并复用同一检测器，恢复 live hook 存在之前创建的 PR。失败/dry-run 不打印 URL，是两条路径天然的防误报闸门。为什么需要
agent 从 shell 创建 PR 的操作者看不到这些会话的 PR badge，也无法按 PR 号搜索——原功能只绑 GitDialog 创建，基于分支的回填又漏掉 head 分支从未作为会话 gitBranch 出现的 PR。"创建时打印的 URL"对这一流程是权威来源，实时与存量皆然。
审查者测试计划
如何验证
state:'open'写 sidecar；失败输出不写）、backfill 回填源（gh 不可用时也能按 transcript 里配对 call/response 的打印 URL 绑定）。gh pr create（或模拟其输出），约 2s 内出现 badge；对含历史gh pr create痕迹的 workspace 重跑POST /sessions/backfill-prs，确认bound计数。证据（前后对比）
之前：跑过
gh pr create的会话无prs绑定（badge 缺、按号搜索漏），除非其 git 分支恰为 PR head。之后：创建时（实时）或 backfill（存量）写入绑定，badge/搜索/tooltip 行为与 GitDialog 绑定一致。测试平台
macOS ✅；Windows/Linux⚠️ 未本地验证（无 OS 特有路径）。
环境
仓库 vitest 套件（core 33 + shell 305 + cli 27 定向）+ build/typecheck/lint；macOS 本地。
风险与范围
gh pr create字样且输出恰有 PR 样式 URL 会误绑；URL 必要条件使其实践中几乎不可能，且绑定 append-only/有界（cap 10）、重跑幂等。shell hook best-effort，绝不改变工具结果。关联
#9543（会话↔PR 绑定）后续；叠在 #9729 之上。
设计更新（最终形态，覆盖上文旧描述）
经多轮 review 迭代，绑定来源与闸门已重构，上文"检测器启发式/转录 call-response 配对"等描述作废：
/review <N|#N|url>命令（优先读 slash_command 原始命令记录）与 worktreepr-<N>约定（slug 已保留该命名空间）。转录中的gh pr create痕迹与 session git 分支两个来源已移除（前者无法归因、存在伪造向量；后者实测纯噪声）。