feat(web-shell): add git mode selector for new session creation - #7471
Merged
Conversation
Support three git workflows when creating a new session:
1. Current branch (default, unchanged behavior)
2. New branch — daemon runs git checkout -b before spawning
3. Worktree isolation (existing, now unified into the same UI)
The mode selector lives in the composer's git chip as a popover,
replacing the previous worktree-only toggle in the welcome header.
API: POST /session accepts branch: { name } (mutually exclusive
with worktree). Server validates branch name, checks dirty tree,
creates branch, and rolls back on spawn failure.
Design doc: docs/design/2026-07-22-webshell-session-git-mode.md
Collaborator
|
Re-run at Template: Core sections present and well-filled — What this PR does, Why it's needed, Reviewer Test Plan with step-by-step verification and Playwright screenshots. Missing the Tested on / Risk & Scope / Linked Issues sections from the template, but the body is thorough enough that this is a minor gap, not a blocker. Problem: Real workflow gap, not theoretical. Users who want to "start a new branch for this task" currently have to manually Direction: Aligned. This extends the existing worktree session infrastructure with a third, lighter-weight git workflow. CHANGELOG shows Claude Code has been investing in worktree and git-related features (worktree-isolated subagents, worktree symlink fixes, etc.), so the area is actively relevant. The unified popover replacing the worktree-only toggle is a clean UX consolidation. Size: Cross-package change spanning Approach: The implementation mirrors the existing Moving on to code review. 🔍 中文说明在 模板： 核心章节齐全且内容充实——PR 做了什么、为什么需要、评审者测试计划（含逐步验证步骤和 Playwright 截图）。缺少模板中的 测试环境 / 风险与范围 / 关联 Issue 章节，但 PR 描述已足够详尽，这只是小瑕疵，不构成阻塞。 问题： 真实的工作流缺口，非理论性问题。用户想"开个新分支做这个任务"时，目前只能手动 方向： 对齐。这是在现有 worktree 会话基础设施上扩展第三种更轻量的 git 工作流。CHANGELOG 显示 Claude Code 一直在投资 worktree 和 git 相关功能，该领域持续活跃。用统一 popover 替代仅 worktree 的开关是干净的 UX 整合。 规模： 跨包变更，涉及 方案： 实现几乎逐行镜像现有 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewRe-run at No critical blockers found. Specific observations:
sequenceDiagram
participant P1 as Web Shell composer
participant P2 as Daemon POST session
participant P3 as git
participant P4 as Session bridge
P1->>P2: createSession with branch name
P2->>P2: validate name, reject if worktree also set
P2->>P3: rev-parse, status porcelain (exists, dirty)
P2->>P2: check inFlight and activeBranchSessions guards
P2->>P3: checkout -b name
P2->>P4: spawnOrAttach with branch meta
P4-->>P2: session
P2->>P2: activeBranchSessions.set, clear inFlight
P2-->>P1: 200 with branch name and baseBranch
Note over P2,P3: on spawn failure, checkout baseBranch then branch -D name
Files changed (35 total, key files shown)
Real-scenario testingRan the actual daemon from this PR's code ( Unit tests (all pass on this head):
中文说明代码审查在 未发现关键阻塞项。 具体观察：
真实场景测试从本 PR 代码（ 单元测试（在此 head 上全部通过）：
— Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, well-tested implementation that does exactly what it promises; the design mirrors existing patterns, the security surface is tight, and real-scenario testing confirms every path works. Capped at 4 (not 5) because the cross-package scope is wide and the Stepping back: this PR solves a real, everyday friction — "I want to start a new branch for this task" shouldn't require a manual The one thing a maintainer should consciously accept: "new branch" mode mutates the shared workspace HEAD. That's the point of the feature, but it's a different safety model from worktree isolation. The dirty-tree check and the one-branch-session-per-workspace guard mitigate the obvious footguns, but a user with two browser tabs and no attached client in one could still be surprised. The design doc acknowledges this ("不做会话结束时自动切回原分支") and the tradeoff is reasonable for v1. After seeing it run, the results match what the PR promised. Every error path returns the right code and message; the happy path creates the branch, switches HEAD, and returns the metadata. 245 unit tests pass across all touched packages. If I had to maintain this in six months, I'd thank the author — the code reads like the worktree flow's sibling, not a bolt-on. Approving. ✅ 中文说明置信度：4/5 —— 干净、测试充分的实现，完全兑现了承诺；设计镜像现有模式，安全面收紧，真实场景测试确认每条路径均正常工作。给 4 而非 5，是因为跨包范围较广，且 退一步看：这个 PR 解决了一个真实的日常摩擦——"我想为这个任务开个新分支"不应该需要手动 维护者应有意识接受的一点："新建分支"模式会改变共享 workspace 的 HEAD。这是功能的设计目的，但与 worktree 隔离是不同的安全模型。脏树检查和每 workspace 单分支会话守卫缓解了明显的误操作，但用户若开了两个浏览器标签且其中一个无已连接客户端，仍可能感到意外。设计文档承认了这一点（"不做会话结束时自动切回原分支"），对 v1 而言权衡合理。 运行结果与 PR 承诺一致。每条错误路径返回正确的代码和消息；正常路径创建分支、切换 HEAD、返回元数据。245 个单元测试在所有触及的包中通过。如果六个月后我要维护这段代码，我会感谢作者——代码读起来像 worktree 流程的兄弟，而非外挂。 批准。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
The portal container used by Web Shell's Popover primitive is not recognized by Radix's DismissableLayer, causing the popover to close on any interaction. Add onInteractOutside prevention so the popover only closes via explicit selection. Also replace prototype screenshots with real Playwright captures and add e2e test + screenshot capture script.
…ader The composer git chip popover now fully covers worktree selection, making the welcome header toggle/badge redundant. Remove the toggle UI, its state (worktreeToggleEligible, refs, handlers, focus effect), and all associated tests (unit + e2e + visuals).
- Add missing setSessionBranch(undefined) in loadSidebarSession, createNewSession, and session switch effect (!sid path) - Add setSessionBranch(summary.branch) in session status restore - Add branch rollback in client disconnect (!res.writable) path - Fix onInteractOutside: use containment check instead of unconditional prevention so genuine outside clicks close popover - Hoist promisify(execFile) to module level - Narrow reserved branch name check to only HEAD (FETCH_HEAD etc. are valid branch names)
Collaborator
Author
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
qwen-code-ci-bot
requested changes
Jul 22, 2026
Collaborator
|
🤝 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. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
added 2 commits
July 22, 2026 06:08
…564e # Conflicts: # packages/web-shell/client/App.test.tsx # packages/web-shell/client/App.tsx
Collaborator
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: Review feedback addressedConflict resolutionMerged
Feedback points
Verification
中文说明已处理的评审反馈冲突解决将
反馈要点
验证结果
Base-conflict check: conflicted with main — resolved in this push. Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. 🧠 Handled by Qwen Code · model/模型 |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Collaborator
Author
Code Review —
|
qwen-code-ci-bot
requested changes
Jul 22, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
[Critical] DaemonSessionProvider.tsx:2282-2309: createDetachedSession Pick type ('approvalMode' | 'sourceType' | 'worktree') omits 'branch'. On the cold-start (no active session) path, the branch override is silently dropped — session created on current branch with no error despite user selecting 'New branch' in the UI.
— qwen3.7-max via Qwen Code /review
Comment on lines
+1218
to
+1222
| // ── Branch creation ──────────────────────────────────────────── | ||
| // When `branch` is present, create and checkout a new git branch | ||
| // before spawning. The session runs in the same working directory | ||
| // but on the new branch. Mutually exclusive with `worktree`. | ||
| let branchMeta: { name: string; baseBranch: string } | undefined; |
Collaborator
There was a problem hiding this comment.
[Suggestion] The branch creation path (~116 lines: validation, git ops, concurrency guard, rollback) has no unit tests, while the parallel worktree path has comprehensive coverage in server.test.ts. Six validation checks, two subprocess calls, the activeBranchSessions concurrency map, and multi-step rollback can all regress silently.
Failure scenario: A regex change accepts an invalid name or rejects a valid one; a refactor skips the dirty-tree check; rollback fails to switch back to the base branch (.catch(() => {}) swallows the failure).
Suggested fix: Add unit tests to server.test.ts mirroring the worktree test structure: (1) success, (2) 400 mutual exclusion, (3) 400 invalid name, (4) 400 non-git repo, (5) 409 branch exists, (6) 409 dirty tree, (7) 409 concurrent conflict, (8) 500 checkout failure, (9) rollback verification on spawn failure.
— qwen3.7-max via Qwen Code /review
This PR adds the new-session git-mode selector (current branch / new branch / worktree) but no visuals scenario renders it, so the before/after preview showed no image for an entirely new UI — the empty result was a coverage gap, not a clean bill of health. The PR also removed the `worktree empty state` scenario (its `worktree-welcome-toggle` no longer exists, replaced by this popover), leaving the suite with no view of the new-session empty state at all. Add a `git mode selector` scenario that seeds a trusted git-repo workspace and lands on the empty state (the only place App.tsx wires the intent props), then captures the composer chip and the opened three-mode popover in both themes. Both are byte-stable across runs (0% pixel diff), and asserting an option is visible makes a regression that fails to open the popover fail here rather than only in the screenshot. The branch-name sub-state is deliberately not captured: its input autoFocuses and the popover then dismisses on the idle frame the capture waits for, so it can't be shot stably through this pipeline — the functional web-shell.git-mode.spec.ts already drives that path. Restores the empty-state coverage this PR dropped and gives the new selector a head-only (NEW) preview.
- Forward the branch override in createDetachedSession so the cold-start (no active session) path no longer silently drops a user-selected new branch. - Reserve the workspace before 'git checkout -b' to close the TOCTOU in the activeBranchSessions guard; two concurrent branch creations could both pass the guard and race on HEAD. The reservation is released on every exit path. - Return (and close the browser) when the branch input never appears in the screenshot script instead of falling through to a guaranteed throw. - Add an e2e test asserting the default current-branch submit sends neither branch nor worktree.
Collaborator
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: Review feedback addressedCommit Implemented[Critical] 🟠 TOCTOU in the [rc:3628338013] Screenshot script catch block falls through (automated reviewer) [rc:3628338028] No E2E test for the default "current branch" submit path (automated reviewer) Deferred (recorded reasons)
Verification
中文说明已处理的评审反馈提交 已实现[Critical] 🟠 [rc:3628338013] 截图脚本的 catch 块未返回（自动评审机器人） [rc:3628338028] 缺少默认“当前分支”提交路径的 E2E 测试（自动评审机器人） 延后处理（附记录原因）
验证
Base-conflict check: no conflict with main. Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
pushed a commit
that referenced
this pull request
Jul 22, 2026
gwinthis
reviewed
Jul 22, 2026
gwinthis
left a comment
gwinthis
left a comment
Collaborator
There was a problem hiding this comment.
Architecture Review — PR #7471
Verdict: ✅ Approve — well-designed unified git mode selector.
Design pattern: Unified intent state machine
Replaces the previous pendingWorktreeRef + worktreePending boolean with a single discriminated union:
type SessionGitIntent =
| { mode: 'current' }
| { mode: 'branch'; name: string }
| { mode: 'worktree'; slug?: string };One state variable, one setter, one clear-on-success path. The old code had two coupled pieces of state (ref + boolean) that could drift; the union makes illegal states unrepresentable.
Key design decisions
-
Composer-embedded UI: The mode selector lives in the composer's bottom toolbar (git chip position), not as a separate welcome block. The entry point is where the user's attention already is — no vertical space cost in the welcome area.
-
Server-side validation only: Branch name validation, dirty tree detection, and conflict checking all happen server-side. The UI does NOT pre-detect dirty state — this avoids divergence from git's actual behavior at execution time.
-
TOCTOU prevention:
inFlightBranchWorkspacesSet closes the race between the conflict guard andactiveBranchSessions.set()(which only happens after spawn). The synchronous check-and-add (no await between) serializes the checkout. -
Symmetric rollback: On spawn failure or client disconnect:
git checkout baseBranch && git branch -D name— mirrors the existing worktree rollback pattern exactly. -
Concurrency guard: Only one branch session per workspace (they share HEAD), but worktree sessions are unlimited (independent directories). The guard correctly distinguishes the two isolation models.
-
Command preview: Popover footer shows the exact git command (
$ git checkout -b feat/x ← main) — transparency principle, the user knows exactly what will happen.
Minor observation
The validateBranchName function is duplicated between GitModePopover.tsx (client-side UX feedback) and routes/session.ts (server-side enforcement). This is intentional and correct — the client copy is for immediate UX feedback, the server copy is the authoritative check. The two could drift, but the server always wins.
Collaborator
Author
Code Review —
|
Collaborator
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: Review Feedback Summary — PR #7471Feedback from @gwinthis (Architecture Review, Approve)No action items — the review is a positive architecture assessment with no requested changes. Feedback from @wenshao (Code Review)
Changes
Verification
ConflictNo conflict — 中文说明审查反馈总结 — PR #7471来自 @gwinthis 的反馈（架构审查，批准）无需处理——该审查是对架构的正面评价，未提出任何修改要求。 来自 @wenshao 的反馈（代码审查）
变更内容
验证结果
冲突无冲突—— Base-conflict check: no conflict with main. Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
pushed a commit
that referenced
this pull request
Jul 22, 2026
Collaborator
Author
Code review —
|
qwen-code-ci-bot
pushed a commit
that referenced
this pull request
Jul 23, 2026
qwen-code-ci-bot
requested changes
Jul 24, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not reviewed: reverse audit of chunks 1, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13 — launched with a prompt that is not the one the CLI built.
Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
— qwen3.7-max via Qwen Code /review
Comment on lines
+1378
to
+1381
| try { | ||
| await createBranch(workspaceCwd, branchName); | ||
| } catch (checkoutErr) { | ||
| inFlightBranchWorkspaces.delete(workspaceCwd); |
Collaborator
There was a problem hiding this comment.
[Critical] createBranch() catch block does not roll back when git partially created the branch. — Failure scenario: A failing post-checkout hook (or timeout after ref update) causes createBranch to reject, but git has already created and checked out the branch. The catch block releases inFlightBranchWorkspaces and returns branch_checkout_failed, but because branchMeta is assigned after the await, none of the existing rollback paths run. The API reports failure and creates no session while silently leaving the shared workspace on the new branch; a retry gets branch_already_exists, and ordinary sessions operate on the unintended branch.
Fix: after any createBranch() rejection, inspect whether HEAD moved or the target branch now exists, restore baseBranch/baseCommit, and delete the partially-created branch when safe.
— qwen3.7-max via Qwen Code /review
…out branch creation (#7471)
Collaborator
|
🤖 Addressed the latest review feedback (round 9/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 9/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedRe-review feedback on Implemented1. [Critical] Roll back when
The catch is now transactional: it calls the existing
2. Shared-checkout hazard — the merge gate (@ytahdn's open Critical, endorsed by @wenshao) The reservation guard only prevented branch-vs-branch conflicts; a concurrent current-branch session in the same workspace was not blocked, so Added a same-cwd guard in the 3. [Low] Added byte-length caps to both predicates (kept in sync like the rest of the validation): reject when any 4. Nit (a11y) The three mode options now expose Deferred (recorded, not dropped)
VerificationCommands actually run (in the workflow checkout, after the commit):
No settings source changed, so 中文说明已处理的评审反馈针对 已实现1. [Critical]
现在该 catch 是事务性的：调用已有的
2. 共享 checkout 隐患——合并门槛（@ytahdn 未关闭的 Critical，由 @wenshao 背书） 保留守卫此前只防分支对分支冲突；同一工作区内并发的当前分支会话不受阻拦，因此 在 3. [Low] 在两端谓词中都加了字节长度上限（与校验其余部分一样保持同步）：任一 4. 无障碍小问题（Nit） 三个模式选项现在暴露 延后处理（已记录，非丢弃）
验证实际运行的命令（在工作流 checkout 中，提交之后）：
未改动任何 settings 源，故无需 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
pushed a commit
that referenced
this pull request
Jul 24, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Unresolved, please confirm: [Critical] selection-text.ts:7 — ink patch missing semantic selection types (out of this PR's diff scope; build passes at PR head but types not verified against the patch file) [Critical] @ytahdn shared-checkout session protection — code has a guard for live non-worktree sessions with dirty-tree check mitigation, but whether this is sufficient is a design decision requiring maintainer confirmation [Critical] @ytahdn TOCTOU improvements — the concurrent guard has activeBranchSessions + inFlightBranchWorkspaces with stale-entry self-healing, but whether further hardening is needed is a design decision [Critical] @ytahdn branch metadata persistence — branch data exists only in memory on the live bridge SessionEntry; whether this should be persisted across daemon restart is a design decision [Critical] DaemonSessionProvider.tsx:2282 — branch field on cold-start path (autofix rounds claim addressed but body truncated; could not verify against current code) Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries. Not reviewed: verification — the review posts findings, but no verifier was launched with a prompt this skill builds — they were ruled on, if at all, without the verdict bar its brief carries.
— qwen3.7-max via Qwen Code /review
Collaborator
|
🤖 Addressed the latest review feedback (round 10/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 10/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #7471Addressed the actionable inline review findings. The two review bodies Inline findings
Conflicts
Verification
中文说明Autofix 审查轮次 — PR #7471已处理可操作的行内审查意见。两条 review 正文（ 行内意见
冲突
验证
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
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Unresolved, please confirm: [Critical] session.ts — @ytahdn shared-checkout session protection: code has a guard for live non-worktree sessions with dirty-tree check mitigation, but whether this is sufficient is a design decision requiring maintainer confirmation [Critical] session.ts — @ytahdn TOCTOU improvements: concurrent guard has activeBranchSessions + inFlightBranchWorkspaces with stale-entry self-healing, but whether further hardening is needed is a design decision [Critical] session.ts — @ytahdn branch metadata persistence: branch data exists only in memory on the live bridge SessionEntry; whether this should be persisted across daemon restart is a design decision Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
— qwen3.7-max via Qwen Code /review
ytahdn
approved these changes
Jul 24, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. 增量 review 无 Critical 缺陷。
核心确认：
- Branch 创建流程：验证 → 并发检查（
activeBranchSessions）→ TOCTOU guard（inFlightBranchWorkspaces）→ dirty tree 检查 →git checkout -b→ spawn → 失败回滚（先 checkout base 再 delete branch，顺序正确） - git-branch-ops：所有操作 30s timeout，
deleteBranch用-Dforce delete（rollback 正确），isDirtyTree用--untracked-files=no（仅 tracked changes） - GitModePopover：radio-style ARIA（
role=radio+aria-checked），validateBranchName镜像服务端（TextEncoderUTF-8 byte 计数），portal containeronInteractOutside处理 - App.tsx：
gitModeIntent替代旧pendingWorktreeRef，sessionBranch从 server 恢复（同 worktree 模式） - sessionPreparation：
branch参数透传 + 返回值并列 - 测试覆盖：server（创建/dirty/并发/TOCTOU/rollback）+ git-branch-ops + GitModePopover + E2E
实现与 worktree 模式一致，防御性编程到位。
— qwen3.7-plus via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 24, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
|
🤖 Addressed the latest review feedback (round 11/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 11/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[Suggestion] rc:3642693022 — Missing test for workspace-scoped batch delete clearing branch-session entryDecision: Implemented. Change: Added
This mirrors the existing [Critical] Design-decision questions (3 items)Decision: Declined — not actionable in this headless workflow. All three "Critical" items are framed as design decisions requiring maintainer confirmation:
These are product/architecture questions that require maintainer input, not code defects that can be fixed autonomously. Verification
中文说明已处理的评审反馈[建议] rc:3642693022 — 缺少工作区级批量删除清除分支会话条目的测试决定： 已实现。 变更： 在
这与现有的 [严重] 设计决策问题（3 项）决定： 拒绝 — 在此无头工作流中不可操作。 所有三个"严重"项都被表述为需要维护者确认的设计决策：
这些是需要维护者输入的产品/架构问题，而非可以自主修复的代码缺陷。 验证
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/模型 |
doudouOUC
reviewed
Jul 24, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: chunk 6, chunk 8, chunk 2, chunk 3, chunk 1, chunk 12, chunk 14, chunk 10, chunk 7, chunk 9, chunk 11, chunk 5, chunk 4, chunk 13 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Test coverage matrix (whole-diff), Agent 1b: Removed-behavior audit, Agent 1c: Cross-file tracer, Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies. Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it.
— qwen3.7-max via Qwen Code /review
Comment on lines
+420
to
+422
| const activeBranchSessions = new Map<string, string>(); | ||
| // Workspaces with a branch creation currently in flight (reserved between | ||
| // the conflict guard and `activeBranchSessions.set`, which only happens |
Collaborator
There was a problem hiding this comment.
[Suggestion] activeBranchSessions has no cleanup hook on natural session end (idle timeout, process exit, crash) — only through explicit DELETE routes (lines 3199, 3228, 3312). The self-healing guard at the conflict check (getSessionSummary + catch/cleanup) mitigates user-visible impact, but the stale entry persists between the session's natural end and the next branch-creation attempt for that workspace.
— Concrete cost: between natural termination and the next POST /session { branch }, the workspace is incorrectly reported as having an active branch session to any concurrent caller.
— Suggested fix: compose clearBranchSessionEntry into the bridge's sessionLifecycle callback so 'removed' events clear the tracking entry regardless of how the session ended.
— qwen3.7-max via Qwen Code /review
Comment on lines
+3227
to
3230
| for (const removedId of result.removed) { | ||
| clearBranchSessionEntry(removedId); | ||
| } | ||
| res.status(200).json(result); |
Collaborator
There was a problem hiding this comment.
[Suggestion] Bulk delete endpoints (POST /sessions/delete here and POST /workspaces/:workspace/sessions/delete at ~3311) call clearBranchSessionEntry for each removed session, but no test exercises either path. The single DELETE /session/:id has a dedicated test at server.test.ts:16629, but the bulk delete paths are untested.
— Failure scenario: a future refactor that removes the clearBranchSessionEntry call from bulk delete would go undetected, introducing stale entries that cause spurious 409 branch_session_conflict.
— Suggested fix: add a test that creates a branch session, bulk-deletes it via POST /sessions/delete, then verifies a second branch session for the same workspace succeeds without 409.
— qwen3.7-max via Qwen Code /review
Comment on lines
+1650
to
+1652
| if (branchMeta) { | ||
| inFlightBranchWorkspaces.delete(workspaceCwd); | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] This safety net clears inFlightBranchWorkspaces without transferring the session to activeBranchSessions. The comment correctly notes this path is unreachable for branch sessions (sessionScope='thread' forces fresh spawn, never attach), but if it were ever triggered, the workspace would appear available for new branch sessions while the original session is still running.
— Concrete cost: a subsequent POST /session { branch } could check out a new branch, moving HEAD away from the still-running (but untracked) session.
— Suggested fix: also call activeBranchSessions.set(workspaceCwd, session.sessionId) before clearing inFlightBranchWorkspaces, mirroring the catch block at ~1628.
— qwen3.7-max via Qwen Code /review
wenshao
pushed a commit
that referenced
this pull request
Jul 25, 2026
…rules Borrow the image-evidence and quantified-verification patterns from hand-run rounds (#7265, #7471, #7686 r2 and the pr-assets convention): - publish-verify now hosts agent-produced evidence/*.png on the pr-assets branch (verify/pr<N>-<run>-<attempt>/) and appends them below the escaped report. Untrusted-payload discipline: strict filename allowlist, 8-image / 2 MB caps enforced in the find predicates, racing-push retry, and every failure degrades to a text-only comment. VERIFY_ASSETS_REMOTE is a test seam; the block was dry-run against a local bare remote covering hosting, hostile filenames, oversize files, dotfiles, missing branch, and no-image runs - skill: evidence images are named as kebab-case captions binding image to claim, before/after pairs over lone after-shots; follow-up rounds lead with a previous-finding status table (fixed/stands/superseded/declined, with adjudication) and re-measure instead of diffing the old report; size/perf claims get measured-metric Δ tables with residual deltas accounted for; unreachable branches get the configuration that reaches them constructed; defensive guards get their accept path checked against real production artifacts, not just mocked rejects
pull Bot
pushed a commit
to Stars1233/qwen-code
that referenced
this pull request
Jul 26, 2026
* feat(triage): add sandboxed /verify deep-verification lane @qwen-code /verify on a PR now runs a local-verification-style evidence round in the isolated /tmux sandbox contract (container, token-free agent env, loopback model proxy, author-write gate) and publishes the report via a separate PR-code-free job: - new verify job: merge-ref checkout at depth 2 (base tip + PR head for A/B), skills pinned from base so the tree under test can never rewrite its own verifier, PR-planted tmp/*-verify-* artifacts dropped, git exec-vector sweep for the persistent workspace, agent verdict allowlisted before it reaches workflow outputs - new publish-verify job: upserts one marker comment (running status -> final report), HTML-escapes the untrusted report, reports skip/na/ prepare-fail/infra outcomes explicitly since /verify is always an explicit request - new verify-pr skill: A/B load-bearing proof, vacuity check on new tests, mock-free wire-oracle harnesses, targeted gates, fixed report/ verdict/assertions artifact contract, counts-are-sacred rules - triage skill Stage 2c now names /verify (not just /tmux) as the trigger to recommend when a PR's central claim needs behavioral evidence The verify check-runs ride the issue_comment event, which the finalize workflow's event == "pull_request" universe structurally excludes, so they cannot pollute the CI table or the deferred-approval gate. * feat(triage): teach /verify round continuity and artifact-matched methods Fold two more hand-verification patterns into the verify lane: - round continuity: the resolve step snapshots the previous verify report (if any) into the agent context before the status upsert overwrites it, and the skill re-checks each prior finding at the new head (fixed/stands/superseded), scoping new probes to the delta - harness quality: prefer configuration seams over module interception, encode the upstream's real semantics in the fake peer, add decoy targets - artifact-matched methods: per-commit load-bearing tables for multi-commit PRs; workflow/CI PRs get embedded-script replay against real data, repo lint gates, and day-one trigger cost math from real event history; every new config knob must trace to an observable effect, and default-path dispatch combinations get probed - findings quality: blockers enumerate blast radius, demonstrate the sharpest consequence end-to-end when budget allows, and carry a collapsed minimal suggested fix preserving the original commit's intent * feat(triage): host /verify evidence images and encode quantified-A/B rules Borrow the image-evidence and quantified-verification patterns from hand-run rounds (QwenLM#7265, QwenLM#7471, QwenLM#7686 r2 and the pr-assets convention): - publish-verify now hosts agent-produced evidence/*.png on the pr-assets branch (verify/pr<N>-<run>-<attempt>/) and appends them below the escaped report. Untrusted-payload discipline: strict filename allowlist, 8-image / 2 MB caps enforced in the find predicates, racing-push retry, and every failure degrades to a text-only comment. VERIFY_ASSETS_REMOTE is a test seam; the block was dry-run against a local bare remote covering hosting, hostile filenames, oversize files, dotfiles, missing branch, and no-image runs - skill: evidence images are named as kebab-case captions binding image to claim, before/after pairs over lone after-shots; follow-up rounds lead with a previous-finding status table (fixed/stands/superseded/declined, with adjudication) and re-measure instead of diffing the old report; size/perf claims get measured-metric Δ tables with residual deltas accounted for; unreachable branches get the configuration that reaches them constructed; defensive guards get their accept path checked against real production artifacts, not just mocked rejects * fix(triage): address /review suggestions on the verify lane - skill: local invocation resolves --repo and passes it to every gh call - skill: call out the dependency confound when the base A/B side reuses the PR-installed node_modules and the PR touches package.json/lockfile - workflow: document the pin step's bootstrap logic — issue_comment jobs run the default branch's YAML, so base always carries the verify-pr skill by the time this job exists * fix(triage): harden /verify gate, comment budget, and evidence hosting per review Address review round 5078770575 items 1-3 plus the cheap follow-ups: - authorize: /verify now requires write from BOTH the PR author (whose code runs) and the commenter (who spends a scarce runner slot + model budget) — a drive-by account can no longer burn 45 minutes of ecs-qwen on someone else's PR; duplicates check once; /tmux and /triage gates unchanged. Replayed 8 principal scenarios against a stubbed gh - authorize acks /verify with the eyes reaction from the always-hosted job, so a queued/saturated sandbox pool no longer means total silence - publish: emit_block escapes FIRST and caps the escaped size (45 KB for the report) — a raw-side cap let dense <>& content inflate past GitHub's 65,536-char comment limit, 422 the post, and strand the running status with no report at all; iconv -c keeps a UTF-8 sequence split by the byte cut (likely, given the mandated 中文 summary) from shipping broken; replayed: 50 KB dense report -> 45,873-byte body - publish: image cap is byte-exact (-size -2097153c; find's -2M rounds sizes UP to MiB, silently making the documented 2 MB cap 1 MiB), bytes must carry the PNG magic (extension is attacker-choosable), duplicate sanitized names dedupe instead of overwriting + double-rendering, and dropped images are reported in the comment instead of vanishing - publish: weak terminal notices (cancelled/infra/skipped/n-a) only replace this run's own running status; a previous round's real report survives as the marker comment and the notice posts fresh - publish: report.md/assertions.json lookups pin the artifact-dir shape and sort (bare find -name order is filesystem-dependent); the verify job's verdict.txt lookup sorts likewise - verify: global npm install runs from RUNNER_TEMP (the persistent workspace still holds the PREVIOUS run's tree, whose .npmrc would apply to a root install); both cleanup passes remove leftover tmp/ worktrees (git worktree prune alone only drops metadata); the run step no longer re-chowns 50k node_modules files; pr-assets clone sets its committer identity once so the racing-push rebase retry can commit - skill: worktree guidance now tells the agent to remove its base tree itself, with the workflow sweep as backstop only * fix(triage): close runtime-plant and stale-RUNNER_TEMP channels in /verify Address review round 2 (comment 5079157987) and the CHANGES_REQUESTED round on the verify lane: - run step re-sweeps tmp/*-verify-* AFTER npm ci/build and before the agent starts: the pin step's sweep runs before PR lifecycle scripts (postinstall etc.), which could re-plant a fake artifact dir whose zeroed timestamp deterministically wins the sorted collector. From the sweep on, only the agent writes those dirs; a steered agent forging its own artifacts remains the documented advisory-report residual - RUNNER_TEMP verify-results/verify-context are rm'd before mkdir: the pool is persistent and runner temp hygiene is runner-managed — a stale report or previous-report.md from ANOTHER PR must never ride along - symlinks are stripped from verify-results before upload: actions/upload-artifact dereferences them, so a node-planted link would exfiltrate whatever it points at into the artifact - a trusted commenter invoking /verify on a PR whose author lacks write now gets an explanation comment from the hosted authorize job instead of total silence (the commenter is checked first; drive-by accounts and API errors still get nothing); job timeout 45->60 so a slow install can never let the JOB limit kill the agent past its own graceful 25m budget - stale tmp/base-tree (skill's canonical scratch worktree) is removed by name at job start — a plain dir isn't git-registered, so the worktree sweep alone misses it and the next worktree add would fail - scripts/tests/qwen-triage-workflow.test.js gains a verify-lane describe block: an 8-arm stub-gh replay of the dual principal gate (drive-by deny, author-without-write deny + explain flag, self-comment dedupe, 404 fail-closed, /tmux and /triage unchanged) plus guards for the post-prepare sweep placement, the symlink strip, and the RUNNER_TEMP resets — the replay found this commit's sweep edit had silently not applied, which is exactly the regression class it exists to catch * fix(triage): close proxy-hijack, gate-bypass, and false-verdict paths in /verify Address the Codex /review round (19 findings) and the bot's follow-up. Each fix was replayed locally; the proxy fix has a decisive A/B. Gate and routing: - the shell command match is case-insensitive: GitHub Actions expression comparisons ignore case, so `@QWEN-CODE /VERIFY` reached the step and fell through to the commenter-only branch — running the PR author's code with the author never checked - the verify ack and denial notice require github.event.issue.pull_request: /verify on a plain issue was acknowledged but could never report - publish-verify joins the verify job's per-PR concurrency group, and a failed PATCH falls back to posting fresh instead of going silent Untrusted-input paths: - the model proxy binds an EPHEMERAL port, reports it through a root-owned file, and its health check must echo a per-run nonce with the recorded PID alive. A/B with a squatter on 8787: the old code's proxy dies EADDRINUSE yet still reports enabled and points qwen at the squatter; the new code comes up unaffected on an ephemeral port - worktree-scoped git config is deleted before hooksPath is resolved: `extensions.worktreeConfig` is allowlisted and .git/config.worktree is invisible to `git config --local`, so a prior run could set core.hooksPath=/ and make the hook sweep's recursive delete walk / as root (verified locally). The sweep now also refuses any hooks path outside the repository's git dir - marker-comment lookups accept only bot-owned comments that START with the marker: any user can paste the marker and divert the bot into PATCHing a stranger's comment - the upload staging dir is re-flushed after npm lifecycle scripts Honest verdicts: - the docs-only classifier no longer uses a pipeline (grep -q made the writer take SIGPIPE, so under pipefail a long file list with an early code file classified a code PR as docs-only and skipped verification), and executable markdown/YAML (.qwen, .github/workflows, scripts) is classified as behavioral before the extension rule - tee's status is checked alongside qwen's: a full results volume made a truncated evidence stream publish as pass - 137 is split by elapsed budget into watchdog timeout vs crash/OOM - the agent's verdict is honored only for VERDICT=pass with a report and zero failed assertions; otherwise the process outcome headlines and the scope paragraph says the run did not complete - verdict.txt is read through a bounded prefix (SIGPIPE under pipefail) Skill contract corrections: per-commit tables only when the commits are reachable at depth 2 (else aggregate + Not covered); internal workspace symlinks must have their realpaths asserted before a base control is trusted; repo lint gates and event-history cost math are qualified to what the token-free container can actually run; --repo is never inferred from `origin` (a fork holds a different PR under the same number). Tests: 9 new guards, all mutation-verified (reverting each fix turns one red), including an executable escaping/size-cap/UTF-8 test for the publisher's own emit_block and a fix to the earlier command-file test, which matched the tmux job's identically named step. * fix(triage): re-establish the /verify trust boundary after PR code runs Third review round (31 findings). The unifying defect: everything the lane pinned or swept happened BEFORE npm ci/build executed PR-authored lifecycle scripts as node, so each control could be undone afterwards. Trust boundary, rebuilt in order before the agent starts: - kill every process owned by the build user and fail the step if any survives — a detached postinstall child could otherwise wait out each one-shot sweep and re-plant afterwards - re-pin .qwen from the base commit again, now root-owned and read-only: the prepare step chowns the workspace to node, so a lifecycle script could rewrite the very skill that defines /verify-pr - give the agent a fresh HOME/QWEN_HOME: qwen loads user-scope file commands from $HOME/.qwen, and /home/node belongs to the build user, so a planted commands/verify-pr.toml could shadow the pinned skill - the model proxy now requires a per-run bearer token, closing the blind-localhost-scan path to an unauthenticated signer for the real model credential (a command the agent itself launches still inherits it — documented residual, not closed) Authorization and lifecycle: - re-verify the PR author's write permission at execution time and pin the authorized head OID; refuse if the checked-out HEAD^2 differs, so a push during the runner wait cannot smuggle in unreviewed code - validate each principal separately: an empty author vanished in word splitting and left only the commenter checked - honor MAINTAINER_ECS_RUNNER_DISABLED with an explicit notice instead of queueing forever against a disabled pool - status comments carry a machine state marker; inferring 'running' from prose let a report quoting that sentence be overwritten - previous-report.md snapshots the newest substantive report, never a weak/cancelled notice, so prior findings survive into the next round - bot-identity lookup failures fail closed instead of widening the ownership filter to every user's comments - publish-verify uses a per-run concurrency group: a per-PR group holds only one pending job, so a second /verify could cancel a completed run's pending publisher Correctness: - install/build failures are classified: signals, ENOSPC, registry and network errors are infra-error, not a PR verdict - watchdog classification measures the child's own elapsed time, not shell-global $SECONDS which includes proxy setup - assertions.json must be three non-negative integers with a positive total and total == pass + fail before it counts as evidence - the proxy keeps its upstream deadline armed until the body ends and aborts upstream when the client disconnects - cleanups remove .qwen/tmp itself: PR code can make it a symlink, and globbing below it deleted the target's contents as root (verified) - emit_block materializes the escaped text and truncates on a character boundary via node — iconv -c passes an incomplete trailing sequence through on BSD (measured), which the new test caught Skill: local mode requires the same isolation CI provides and must not assume HEAD^1/HEAD^2 on a plain head checkout; shallow boundaries make rev-list counts unreliable for per-commit claims; never run scripts/lint.js with no arguments (it runs prettier --write and rewrites the tree under the harnesses); a vacuity check must fail the intended assertion, not the import. pr-workflow.md now says both sandboxed lanes need the author to have write, so triage stops recommending a guaranteed denial on external PRs. Tests: 9 more guards, all mutation-verified, including executable replays of the docs-only classifier (SIGPIPE + executable-markdown cases), the uppercase-command gate, the empty-principal deny, and the untrusted-image hosting path against a bare pr-assets remote. * test(triage): pass the classifier fixture through a file, not argv The new docs-only classifier replay passed on macOS and failed on CI with `Cannot read properties of undefined (reading 'trim')`: its 60,001-entry fixture is ~889 KB and was passed as a single argv element. Linux caps one argument at MAX_ARG_STRLEN (128 KB), so the spawn failed with E2BIG and stdout was undefined; macOS has no per-argument limit and only a ~1 MB total, so the same call succeeded locally (verified both). Write the list to a temp file and pass the path. The harness now also asserts the spawn succeeded, so a future spawn failure reports itself instead of surfacing as a TypeError on undefined output. * fix(triage): make the /verify report match what the run actually produced Three publisher findings, all introduced by my own previous round: - an artifact download failure (the step is continue-on-error) let the full-report path run with no results: the headline read 'completed' and the scope paragraph claimed the A/B, the harnesses and the gates had run when nothing had been delivered. The download outcome is now an input, and its failure gets its own body saying the results could not be retrieved - the prepare-failure branch ignored the verdict the prepare step had just computed, so an install killed by a registry outage or OOM (classified infra-error) still told the author 'this is treated as a PR failure verdict rather than an infrastructure failure' — the exact opposite. It now branches on the verdict, and an infra-classified prepare failure is a weak body that cannot overwrite a real report - weak notices were being snapshotted as the follow-up round's previous-report.md: they lack the running marker, so 'newest non-running comment' selected them. Bodies that carry findings now mark themselves (qwen-triage:verify-substantive) and the snapshot selects on that marker. A/B on the real jq: report A then cancelled B now snapshots A (101), the old filter picked B (102) Tests: 4 more guards, all mutation-verified — the publisher is rendered for each outcome with a stubbed gh and the assertions read the body it would post, and the snapshot test runs the workflow's own jq program verbatim against a paginate-shaped fixture. * fix(triage): stop PR build output from masquerading as an infra failure Two review findings plus a test-helper hazard: - classify_failure grepped the prepare log for bare words like ENOSPC and ETIMEDOUT, but that log is written by PR-controlled code: a genuine build failure that merely prints 'expected ETIMEDOUT to equal ok' would be published as an infrastructure incident, telling the author to re-run something that fails identically. The patterns are now anchored to lines only npm's reporter or the kernel emits ('npm ERR! code E…', 'npm ERR! network …', kernel OOM, bare 'Killed'); a signal exit still needs no log evidence. Replayed 10 cells: four PR-authored logs quoting infra words stay 'fail', five real diagnostics and one signal exit are 'infra-error' - the two execution-time controls added last round — re-verifying the author's permission after the runner wait, and refusing a head that moved since authorization — had no tests. Both are now executed: the re-auth snippet against a stubbed permission API (write proceeds and pins head_oid; read skips with a publishable reason), and the pin step against a real git repo with a real merge commit (matching head proceeds, moved head exits non-zero) - add a stepIn(job, step) test helper. Several step names exist in both the tmux and verify jobs, and the unscoped step() returns the first match, so a verify-lane assertion silently tests the tmux copy — that has now bitten this suite three times, including in this commit. * docs(triage): teach verify-pr test-only PRs, differential oracles, gate liveness Fold techniques from the round-2 verification on QwenLM#7620 (an ANSI parser PR) that the skill had no equivalent for: - test-only PRs get their own method: a mutation A/B across TEST FILES (same mutants of the unmodified production file, only the test file swapped), reporting killed/total on both sides, requiring that no mutant regressed from killed to survived, checking that the killing assertion is the one the commit claims to have strengthened, and adjudicating every survivor as coverage gap or defect with independent evidence rather than by inspection - when the code emulates a known implementation, that implementation is the oracle: feed identical input to both and report disagreement counts per side, lift reference tables verbatim out of the shipped dependency, and build the corpus from bytes captured off a real producer alongside synthesized sweeps - prove a gate is live before citing it: plant a violation the linter must catch, confirm it is reported, remove it — a linter that matched no files exits 0 exactly like one that passed - attribute pre-existing failures by byte-identical failing file AND test names on both sides, with deltas, not just totals - when the base is far behind, verify the merge: trial-merge into current main, confirm it is conflict-free, and re-run the affected suite on the merged tree - round continuity gains its one legitimate shortcut: a production file proven byte-identical (sha256 quoted at both heads) carries prior evidence forward by construction * style(triage): reflow verify-pr skill to prettier's markdown wrapping The previous commit's added paragraphs were hand-wrapped and prettier --check flagged the file; the repo runs prettier over all of it. * test(triage): cover the disabled-runner-pool notice The kill-switch path had no test: a refactor could drop the notice and leave a /verify request acknowledged with 👀 but permanently unanswered, since the verify job refuses to start and publish-verify skips with it. Fold the step into the existing PR-guard loop (now scoped through stepIn, so it cannot match a same-named step in another job) and assert the parts that make the answer useful — the kill-switch and permission conditions, both languages, the alternative it points at, and the verify job's own exclusion of the disabled pool. All three mutations turn it red: removing the step, dropping its PR guard, or letting the verify job queue against the disabled pool. * fix(triage): repair a step-killing PIPESTATUS read and six forgeable controls Sixth review round, 12 findings. Several are regressions from my own two previous rounds; the first would have broken every single run. - `AGENT_STATUS=${PIPESTATUS[0]}` is itself a command and resets PIPESTATUS, so the next line's ${PIPESTATUS[1]} was unset and `set -u` aborted the step immediately after the agent finished — before artifact collection, the verdict, or anything else. Verified by replaying the exact structure: 'PIPESTATUS[1]: unbound variable'. Both elements are now snapshotted in one command - concurrency predicates were broader than the job conditions they guard, and GitHub evaluates concurrency BEFORE the job `if`: a /verify comment entered the triage job's shared per-PR group (where it could displace a pending /triage and then skip), and a /verify queued while the runner kill switch was on did the same to a real verification. Both predicates now match their job's runnable set exactly - an outward-resolving .git/hooks entry was only warned about and left in place, so the next root-owned git command would run it. It is now unlinked without traversing its target, a root-owned hooks directory is restored, and core.hooksPath is unset - the second .qwen pin re-derived HEAD^1 from git metadata after the workspace, including .git, had been handed to the build user. The base OID is now recorded while .git is still root-owned and the re-pin archives that content-addressed OID - classify_failure took both of its inputs from PR-controlled sources: a lifecycle script can exit with a signal status and can print any line the log patterns matched, turning its own deterministic breakage into 'infrastructure, please re-run' — which hid the failure and preserved a stale report. No infra verdict is derivable there, so the prepare step reports `fail` and lets the embedded log speak for itself - cleanups descended through PR-writable parents: `.qwen` itself can be a symlink, and the worktree sweep trusted git metadata with only a lexical prefix check. Symlinks are unlinked without traversal and worktree paths must canonicalize inside the workspace. Replayed all three escapes - skipped and docs-only outcomes upload no artifact, so the new download-failure branch pre-empted them and made their real reason unreachable; they are answered first now - a run that crashed before writing report.md still claimed the substantive marker, letting a headline overwrite the previous round's evidence. The marker now requires a report Skill: the byte-identical shortcut needs the whole input closure, not one file hash; the credential-free local path cannot call `gh` at all (fetch the metadata outside and mount it read-only); and the A/B base is `baseRefOid` in local mode, not `HEAD^1`. Tests: 7 new guards plus 4 updated to the new shapes, all mutation-verified (50/50). * fix(triage): answer dropped /verify requests and prove the proxy rejects Maintainer review (yiliang114), 7 items: - a third /verify while two runs are in flight is dropped by the concurrency group with no job and therefore no comment. The hosted authorize job now counts this workflow's other in-flight runs and says so; an API hiccup leaves the request alone rather than denying it - the proxy's bearer check had no executable test. It now starts the real proxy against a real upstream and issues real requests: no header and a wrong token are 401, this run's token is 200, and a route other than /chat/completions is 403 — with the health endpoint echoing the nonce - the 502 path forwarded the raw upstream error, which can name resolved hosts and TLS detail to PR code. It logs server-side and returns a generic failure - publish-verify inherited the 360-minute default; it downloads one artifact and posts one comment, so it is bounded at 10 - removing the log classifier last round left the comment block it replaced, which still said failures are classified from the exit status and the log. Deleted - that removal also left every install failure reported as the PR's fault, including a registry outage. There is exactly one signal here PR code cannot write — asking the registry ourselves, as root, with the container's resolver — so an install failure is downgraded to infra-error only when that probe fails. It proves reachability now rather than at failure time, so it can only ever downgrade, never confirm; a build failure has no equivalent and stays the tree's problem - the skill's local-invocation warning ran into the preceding sentence, which GFM renders as one paragraph Tests: 5 new guards, all mutation-verified (55/55). * fix(triage): resolve hooks hermetically and mirror symlink guards at job end Maintainer review round (doudouOUC), 6 findings. Two were Critical and both reproduced: - the hooks sweep resolved its path with the ambient git config in play. With a global core.hooksPath set — which the reviewer has and I do not, which is why my earlier replay showed a false pass — `git rev-parse --git-path hooks` returns that global path, the in-git-dir guard reads 'outside', and a planted `.git/hooks` symlink survives untouched. A/B: old code leaves the symlink under a global hooksPath, new code removes it in both environments and never touches the link target. Resolution now runs with GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM pointed at /dev/null - the END-of-job cleanup still used the bare `rm -rf .qwen/tmp` that the start-of-job cleaner was hardened against two rounds ago. The agent executes PR code between the two, so the end is no safer than the start: it now unlinks symlinks without descending and canonicalizes worktree paths inside the workspace before deleting Plus four suggestions, all valid: - the saturation notice counted this workflow's in-flight runs across every PR while the concurrency group is per-PR, so a run on another PR would trigger a warning about a queue that does not exist. It now matches on the PR title (the only per-PR handle an issue_comment run record carries) and stays silent when that cannot be resolved - the skill recommended `require.resolve` for the workspace-realpath check; these packages are ESM-only with import-only exports, so it throws ERR_PACKAGE_PATH_NOT_EXPORTED and reads like a missing module. Verified, and replaced with `readlink -f node_modules/@qwen-code/...` - the symlink-escape test inherited the developer's git config, which is what hid the first finding. It now runs with global/system config neutralized AND repeats the case with a global core.hooksPath planted - the publisher's build-phase arm was never rendered by any test (every case used 'install'), so a typo in that command name would have shipped. Now covered, along with an unrecognized phase Mutation-verified 4/4. The hooks guard needed a discriminating assertion: git's own `*.sample` files must survive the sweep, because the outward-path fallback removes the whole directory and would otherwise satisfy a bare 'planted hook is gone' check. * fix(triage): count only /verify runs for saturation, and test the PATCH arm Bot review round, 2 suggestions, both valid: - the saturation notice matched runs by PR title, which narrowed to this PR but not to /verify. /triage and /tmux live in their own concurrency groups, so two of those in flight would warn about a verify queue that is actually empty. It now also requires the run to have a job named 'verify' — the run record carries no command, but its job list does. Replayed: two non-verify runs stay silent, two verify runs warn - every publish fixture returned an empty comments listing, so the PATCH arm was never executed: a broken PATCH would have stranded the running status comment and posted a duplicate below it, with the suite green. The publisher now runs against a stubbed listing and the test asserts which verb went to which comment id — bot-owned live status is PATCHed in place, an absent comment posts fresh, and a marker comment owned by someone else is left alone and posted around Mutation-verified 3/3: counting every command, never PATCHing, and accepting foreign-owned markers each turn one test red. Two stub bugs found while writing these, both mine and both silent: ${*#pattern} applies per positional parameter rather than to the joined string (yielding a wrong run id), and the paginate fixture needs one array per page, not an array of pages. * fix(triage): fix the real silent drop and drop the step built on a wrong premise Review round 4. The blocker was mine twice over: the saturation notice I added last round had GitHub's concurrency semantics backwards, and the silent drop it claimed to cover was somewhere else entirely. - GitHub cancels the OLDER pending run in a group and admits the new one (confirmed against the workflow-syntax reference). My step told the person who had just typed /verify that their request might be dropped, when theirs is the one that runs — and said nothing to the person whose queued run actually died. This PR already had it right in publish-verify's own comment, so the file contradicted itself and the user-facing copy followed the wrong half. The step is removed rather than reworded: with the fix below there is nothing left for it to warn about, and it cost 2+N API calls on every /verify. - the actual drop: a verify job cancelled while still PENDING never reaches a runner, so its outputs block — where the "|| github.event.issue.number" fallback lived — is never evaluated. publish-verify then read an empty PR_NUMBER, hit its own guard and exited 0, making the cancelled branch unreachable in exactly the scenario that produces cancellations. The fallback now lives where the value is read. Reproduced both arms by executing the real step: with a number the cancelled notice posts, with an empty one it only warns. - same one-line class in publish-tmux, fixed alongside. Two copy defects from the classifier removal, both mis-attribution pointed the other way: - the infra-error body still named a signal/OOM kill and a full disk, none of which the current prepare step can produce — infra-error now requires npm ci to fail AND the registry probe to fail. It names that condition only, and offers a re-run instead of asserting it is the fix. - the code comment above it still described the deleted classifier. Also fixes the indentation break an earlier scripted edit left in the publish body builder, and replaces the saturation test with one that executes the cancelled path. Mutation-verified 2/2; the copy needed its own guard, since reverting the wording alone left every test green. * docs(triage): teach verify-pr survivor accounting and observability regressions Fold techniques from the re-verification on QwenLM#7709 that the skill had no equivalent for: - the mutation matrix must report the mutations that changed NOTHING, not only the ones that failed. Each survivor gets classified as an ordinary coverage gap or as dead code — a guard whose deletion leaves every test green is one of those two, and the difference is what the author needs. Survivors mirroring a pre-existing gap are labelled as such, and the set is framed as completeness reporting rather than merge conditions - the sharper case that report demonstrates: a test that passes for the WRONG REASON. If deleting the new guard leaves its own new test green, that test is pinned by an earlier early-return, not by the change, and asserts nothing about it. Name what actually pins it - and do not generalize from one dead guard to its siblings: the same report shows a clause that is unreachable on one path while being the only protection on another. Check each, report the contrast - observability regressions: when a change suppresses output, follow the value before calling the suppression correct. A bare catch on the path plus a field with no readers anywhere in the repo means the cause is now unobservable even in devtools — a real loss that no behavioural assertion can see - report structure gains a Corrections section: when an earlier round or bot comment described the code inaccurately, state the correct fact with evidence and label it as a correction to the description, not a request to change code. A wrong description left standing costs the next reader more than the original finding did --------- Co-authored-by: wenshao <wenshao@example.com>
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
Jul 27, 2026
…LM#7753) * feat(triage): add sandboxed /verify deep-verification lane @qwen-code /verify on a PR now runs a local-verification-style evidence round in the isolated /tmux sandbox contract (container, token-free agent env, loopback model proxy, author-write gate) and publishes the report via a separate PR-code-free job: - new verify job: merge-ref checkout at depth 2 (base tip + PR head for A/B), skills pinned from base so the tree under test can never rewrite its own verifier, PR-planted tmp/*-verify-* artifacts dropped, git exec-vector sweep for the persistent workspace, agent verdict allowlisted before it reaches workflow outputs - new publish-verify job: upserts one marker comment (running status -> final report), HTML-escapes the untrusted report, reports skip/na/ prepare-fail/infra outcomes explicitly since /verify is always an explicit request - new verify-pr skill: A/B load-bearing proof, vacuity check on new tests, mock-free wire-oracle harnesses, targeted gates, fixed report/ verdict/assertions artifact contract, counts-are-sacred rules - triage skill Stage 2c now names /verify (not just /tmux) as the trigger to recommend when a PR's central claim needs behavioral evidence The verify check-runs ride the issue_comment event, which the finalize workflow's event == "pull_request" universe structurally excludes, so they cannot pollute the CI table or the deferred-approval gate. * feat(triage): teach /verify round continuity and artifact-matched methods Fold two more hand-verification patterns into the verify lane: - round continuity: the resolve step snapshots the previous verify report (if any) into the agent context before the status upsert overwrites it, and the skill re-checks each prior finding at the new head (fixed/stands/superseded), scoping new probes to the delta - harness quality: prefer configuration seams over module interception, encode the upstream's real semantics in the fake peer, add decoy targets - artifact-matched methods: per-commit load-bearing tables for multi-commit PRs; workflow/CI PRs get embedded-script replay against real data, repo lint gates, and day-one trigger cost math from real event history; every new config knob must trace to an observable effect, and default-path dispatch combinations get probed - findings quality: blockers enumerate blast radius, demonstrate the sharpest consequence end-to-end when budget allows, and carry a collapsed minimal suggested fix preserving the original commit's intent * feat(triage): host /verify evidence images and encode quantified-A/B rules Borrow the image-evidence and quantified-verification patterns from hand-run rounds (QwenLM#7265, QwenLM#7471, QwenLM#7686 r2 and the pr-assets convention): - publish-verify now hosts agent-produced evidence/*.png on the pr-assets branch (verify/pr<N>-<run>-<attempt>/) and appends them below the escaped report. Untrusted-payload discipline: strict filename allowlist, 8-image / 2 MB caps enforced in the find predicates, racing-push retry, and every failure degrades to a text-only comment. VERIFY_ASSETS_REMOTE is a test seam; the block was dry-run against a local bare remote covering hosting, hostile filenames, oversize files, dotfiles, missing branch, and no-image runs - skill: evidence images are named as kebab-case captions binding image to claim, before/after pairs over lone after-shots; follow-up rounds lead with a previous-finding status table (fixed/stands/superseded/declined, with adjudication) and re-measure instead of diffing the old report; size/perf claims get measured-metric Δ tables with residual deltas accounted for; unreachable branches get the configuration that reaches them constructed; defensive guards get their accept path checked against real production artifacts, not just mocked rejects * fix(triage): address /review suggestions on the verify lane - skill: local invocation resolves --repo and passes it to every gh call - skill: call out the dependency confound when the base A/B side reuses the PR-installed node_modules and the PR touches package.json/lockfile - workflow: document the pin step's bootstrap logic — issue_comment jobs run the default branch's YAML, so base always carries the verify-pr skill by the time this job exists * fix(triage): harden /verify gate, comment budget, and evidence hosting per review Address review round 5078770575 items 1-3 plus the cheap follow-ups: - authorize: /verify now requires write from BOTH the PR author (whose code runs) and the commenter (who spends a scarce runner slot + model budget) — a drive-by account can no longer burn 45 minutes of ecs-qwen on someone else's PR; duplicates check once; /tmux and /triage gates unchanged. Replayed 8 principal scenarios against a stubbed gh - authorize acks /verify with the eyes reaction from the always-hosted job, so a queued/saturated sandbox pool no longer means total silence - publish: emit_block escapes FIRST and caps the escaped size (45 KB for the report) — a raw-side cap let dense <>& content inflate past GitHub's 65,536-char comment limit, 422 the post, and strand the running status with no report at all; iconv -c keeps a UTF-8 sequence split by the byte cut (likely, given the mandated 中文 summary) from shipping broken; replayed: 50 KB dense report -> 45,873-byte body - publish: image cap is byte-exact (-size -2097153c; find's -2M rounds sizes UP to MiB, silently making the documented 2 MB cap 1 MiB), bytes must carry the PNG magic (extension is attacker-choosable), duplicate sanitized names dedupe instead of overwriting + double-rendering, and dropped images are reported in the comment instead of vanishing - publish: weak terminal notices (cancelled/infra/skipped/n-a) only replace this run's own running status; a previous round's real report survives as the marker comment and the notice posts fresh - publish: report.md/assertions.json lookups pin the artifact-dir shape and sort (bare find -name order is filesystem-dependent); the verify job's verdict.txt lookup sorts likewise - verify: global npm install runs from RUNNER_TEMP (the persistent workspace still holds the PREVIOUS run's tree, whose .npmrc would apply to a root install); both cleanup passes remove leftover tmp/ worktrees (git worktree prune alone only drops metadata); the run step no longer re-chowns 50k node_modules files; pr-assets clone sets its committer identity once so the racing-push rebase retry can commit - skill: worktree guidance now tells the agent to remove its base tree itself, with the workflow sweep as backstop only * fix(triage): close runtime-plant and stale-RUNNER_TEMP channels in /verify Address review round 2 (comment 5079157987) and the CHANGES_REQUESTED round on the verify lane: - run step re-sweeps tmp/*-verify-* AFTER npm ci/build and before the agent starts: the pin step's sweep runs before PR lifecycle scripts (postinstall etc.), which could re-plant a fake artifact dir whose zeroed timestamp deterministically wins the sorted collector. From the sweep on, only the agent writes those dirs; a steered agent forging its own artifacts remains the documented advisory-report residual - RUNNER_TEMP verify-results/verify-context are rm'd before mkdir: the pool is persistent and runner temp hygiene is runner-managed — a stale report or previous-report.md from ANOTHER PR must never ride along - symlinks are stripped from verify-results before upload: actions/upload-artifact dereferences them, so a node-planted link would exfiltrate whatever it points at into the artifact - a trusted commenter invoking /verify on a PR whose author lacks write now gets an explanation comment from the hosted authorize job instead of total silence (the commenter is checked first; drive-by accounts and API errors still get nothing); job timeout 45->60 so a slow install can never let the JOB limit kill the agent past its own graceful 25m budget - stale tmp/base-tree (skill's canonical scratch worktree) is removed by name at job start — a plain dir isn't git-registered, so the worktree sweep alone misses it and the next worktree add would fail - scripts/tests/qwen-triage-workflow.test.js gains a verify-lane describe block: an 8-arm stub-gh replay of the dual principal gate (drive-by deny, author-without-write deny + explain flag, self-comment dedupe, 404 fail-closed, /tmux and /triage unchanged) plus guards for the post-prepare sweep placement, the symlink strip, and the RUNNER_TEMP resets — the replay found this commit's sweep edit had silently not applied, which is exactly the regression class it exists to catch * fix(triage): close proxy-hijack, gate-bypass, and false-verdict paths in /verify Address the Codex /review round (19 findings) and the bot's follow-up. Each fix was replayed locally; the proxy fix has a decisive A/B. Gate and routing: - the shell command match is case-insensitive: GitHub Actions expression comparisons ignore case, so `@QWEN-CODE /VERIFY` reached the step and fell through to the commenter-only branch — running the PR author's code with the author never checked - the verify ack and denial notice require github.event.issue.pull_request: /verify on a plain issue was acknowledged but could never report - publish-verify joins the verify job's per-PR concurrency group, and a failed PATCH falls back to posting fresh instead of going silent Untrusted-input paths: - the model proxy binds an EPHEMERAL port, reports it through a root-owned file, and its health check must echo a per-run nonce with the recorded PID alive. A/B with a squatter on 8787: the old code's proxy dies EADDRINUSE yet still reports enabled and points qwen at the squatter; the new code comes up unaffected on an ephemeral port - worktree-scoped git config is deleted before hooksPath is resolved: `extensions.worktreeConfig` is allowlisted and .git/config.worktree is invisible to `git config --local`, so a prior run could set core.hooksPath=/ and make the hook sweep's recursive delete walk / as root (verified locally). The sweep now also refuses any hooks path outside the repository's git dir - marker-comment lookups accept only bot-owned comments that START with the marker: any user can paste the marker and divert the bot into PATCHing a stranger's comment - the upload staging dir is re-flushed after npm lifecycle scripts Honest verdicts: - the docs-only classifier no longer uses a pipeline (grep -q made the writer take SIGPIPE, so under pipefail a long file list with an early code file classified a code PR as docs-only and skipped verification), and executable markdown/YAML (.qwen, .github/workflows, scripts) is classified as behavioral before the extension rule - tee's status is checked alongside qwen's: a full results volume made a truncated evidence stream publish as pass - 137 is split by elapsed budget into watchdog timeout vs crash/OOM - the agent's verdict is honored only for VERDICT=pass with a report and zero failed assertions; otherwise the process outcome headlines and the scope paragraph says the run did not complete - verdict.txt is read through a bounded prefix (SIGPIPE under pipefail) Skill contract corrections: per-commit tables only when the commits are reachable at depth 2 (else aggregate + Not covered); internal workspace symlinks must have their realpaths asserted before a base control is trusted; repo lint gates and event-history cost math are qualified to what the token-free container can actually run; --repo is never inferred from `origin` (a fork holds a different PR under the same number). Tests: 9 new guards, all mutation-verified (reverting each fix turns one red), including an executable escaping/size-cap/UTF-8 test for the publisher's own emit_block and a fix to the earlier command-file test, which matched the tmux job's identically named step. * fix(triage): re-establish the /verify trust boundary after PR code runs Third review round (31 findings). The unifying defect: everything the lane pinned or swept happened BEFORE npm ci/build executed PR-authored lifecycle scripts as node, so each control could be undone afterwards. Trust boundary, rebuilt in order before the agent starts: - kill every process owned by the build user and fail the step if any survives — a detached postinstall child could otherwise wait out each one-shot sweep and re-plant afterwards - re-pin .qwen from the base commit again, now root-owned and read-only: the prepare step chowns the workspace to node, so a lifecycle script could rewrite the very skill that defines /verify-pr - give the agent a fresh HOME/QWEN_HOME: qwen loads user-scope file commands from $HOME/.qwen, and /home/node belongs to the build user, so a planted commands/verify-pr.toml could shadow the pinned skill - the model proxy now requires a per-run bearer token, closing the blind-localhost-scan path to an unauthenticated signer for the real model credential (a command the agent itself launches still inherits it — documented residual, not closed) Authorization and lifecycle: - re-verify the PR author's write permission at execution time and pin the authorized head OID; refuse if the checked-out HEAD^2 differs, so a push during the runner wait cannot smuggle in unreviewed code - validate each principal separately: an empty author vanished in word splitting and left only the commenter checked - honor MAINTAINER_ECS_RUNNER_DISABLED with an explicit notice instead of queueing forever against a disabled pool - status comments carry a machine state marker; inferring 'running' from prose let a report quoting that sentence be overwritten - previous-report.md snapshots the newest substantive report, never a weak/cancelled notice, so prior findings survive into the next round - bot-identity lookup failures fail closed instead of widening the ownership filter to every user's comments - publish-verify uses a per-run concurrency group: a per-PR group holds only one pending job, so a second /verify could cancel a completed run's pending publisher Correctness: - install/build failures are classified: signals, ENOSPC, registry and network errors are infra-error, not a PR verdict - watchdog classification measures the child's own elapsed time, not shell-global $SECONDS which includes proxy setup - assertions.json must be three non-negative integers with a positive total and total == pass + fail before it counts as evidence - the proxy keeps its upstream deadline armed until the body ends and aborts upstream when the client disconnects - cleanups remove .qwen/tmp itself: PR code can make it a symlink, and globbing below it deleted the target's contents as root (verified) - emit_block materializes the escaped text and truncates on a character boundary via node — iconv -c passes an incomplete trailing sequence through on BSD (measured), which the new test caught Skill: local mode requires the same isolation CI provides and must not assume HEAD^1/HEAD^2 on a plain head checkout; shallow boundaries make rev-list counts unreliable for per-commit claims; never run scripts/lint.js with no arguments (it runs prettier --write and rewrites the tree under the harnesses); a vacuity check must fail the intended assertion, not the import. pr-workflow.md now says both sandboxed lanes need the author to have write, so triage stops recommending a guaranteed denial on external PRs. Tests: 9 more guards, all mutation-verified, including executable replays of the docs-only classifier (SIGPIPE + executable-markdown cases), the uppercase-command gate, the empty-principal deny, and the untrusted-image hosting path against a bare pr-assets remote. * test(triage): pass the classifier fixture through a file, not argv The new docs-only classifier replay passed on macOS and failed on CI with `Cannot read properties of undefined (reading 'trim')`: its 60,001-entry fixture is ~889 KB and was passed as a single argv element. Linux caps one argument at MAX_ARG_STRLEN (128 KB), so the spawn failed with E2BIG and stdout was undefined; macOS has no per-argument limit and only a ~1 MB total, so the same call succeeded locally (verified both). Write the list to a temp file and pass the path. The harness now also asserts the spawn succeeded, so a future spawn failure reports itself instead of surfacing as a TypeError on undefined output. * fix(triage): make the /verify report match what the run actually produced Three publisher findings, all introduced by my own previous round: - an artifact download failure (the step is continue-on-error) let the full-report path run with no results: the headline read 'completed' and the scope paragraph claimed the A/B, the harnesses and the gates had run when nothing had been delivered. The download outcome is now an input, and its failure gets its own body saying the results could not be retrieved - the prepare-failure branch ignored the verdict the prepare step had just computed, so an install killed by a registry outage or OOM (classified infra-error) still told the author 'this is treated as a PR failure verdict rather than an infrastructure failure' — the exact opposite. It now branches on the verdict, and an infra-classified prepare failure is a weak body that cannot overwrite a real report - weak notices were being snapshotted as the follow-up round's previous-report.md: they lack the running marker, so 'newest non-running comment' selected them. Bodies that carry findings now mark themselves (qwen-triage:verify-substantive) and the snapshot selects on that marker. A/B on the real jq: report A then cancelled B now snapshots A (101), the old filter picked B (102) Tests: 4 more guards, all mutation-verified — the publisher is rendered for each outcome with a stubbed gh and the assertions read the body it would post, and the snapshot test runs the workflow's own jq program verbatim against a paginate-shaped fixture. * fix(triage): stop PR build output from masquerading as an infra failure Two review findings plus a test-helper hazard: - classify_failure grepped the prepare log for bare words like ENOSPC and ETIMEDOUT, but that log is written by PR-controlled code: a genuine build failure that merely prints 'expected ETIMEDOUT to equal ok' would be published as an infrastructure incident, telling the author to re-run something that fails identically. The patterns are now anchored to lines only npm's reporter or the kernel emits ('npm ERR! code E…', 'npm ERR! network …', kernel OOM, bare 'Killed'); a signal exit still needs no log evidence. Replayed 10 cells: four PR-authored logs quoting infra words stay 'fail', five real diagnostics and one signal exit are 'infra-error' - the two execution-time controls added last round — re-verifying the author's permission after the runner wait, and refusing a head that moved since authorization — had no tests. Both are now executed: the re-auth snippet against a stubbed permission API (write proceeds and pins head_oid; read skips with a publishable reason), and the pin step against a real git repo with a real merge commit (matching head proceeds, moved head exits non-zero) - add a stepIn(job, step) test helper. Several step names exist in both the tmux and verify jobs, and the unscoped step() returns the first match, so a verify-lane assertion silently tests the tmux copy — that has now bitten this suite three times, including in this commit. * docs(triage): teach verify-pr test-only PRs, differential oracles, gate liveness Fold techniques from the round-2 verification on QwenLM#7620 (an ANSI parser PR) that the skill had no equivalent for: - test-only PRs get their own method: a mutation A/B across TEST FILES (same mutants of the unmodified production file, only the test file swapped), reporting killed/total on both sides, requiring that no mutant regressed from killed to survived, checking that the killing assertion is the one the commit claims to have strengthened, and adjudicating every survivor as coverage gap or defect with independent evidence rather than by inspection - when the code emulates a known implementation, that implementation is the oracle: feed identical input to both and report disagreement counts per side, lift reference tables verbatim out of the shipped dependency, and build the corpus from bytes captured off a real producer alongside synthesized sweeps - prove a gate is live before citing it: plant a violation the linter must catch, confirm it is reported, remove it — a linter that matched no files exits 0 exactly like one that passed - attribute pre-existing failures by byte-identical failing file AND test names on both sides, with deltas, not just totals - when the base is far behind, verify the merge: trial-merge into current main, confirm it is conflict-free, and re-run the affected suite on the merged tree - round continuity gains its one legitimate shortcut: a production file proven byte-identical (sha256 quoted at both heads) carries prior evidence forward by construction * style(triage): reflow verify-pr skill to prettier's markdown wrapping The previous commit's added paragraphs were hand-wrapped and prettier --check flagged the file; the repo runs prettier over all of it. * test(triage): cover the disabled-runner-pool notice The kill-switch path had no test: a refactor could drop the notice and leave a /verify request acknowledged with 👀 but permanently unanswered, since the verify job refuses to start and publish-verify skips with it. Fold the step into the existing PR-guard loop (now scoped through stepIn, so it cannot match a same-named step in another job) and assert the parts that make the answer useful — the kill-switch and permission conditions, both languages, the alternative it points at, and the verify job's own exclusion of the disabled pool. All three mutations turn it red: removing the step, dropping its PR guard, or letting the verify job queue against the disabled pool. * fix(triage): repair a step-killing PIPESTATUS read and six forgeable controls Sixth review round, 12 findings. Several are regressions from my own two previous rounds; the first would have broken every single run. - `AGENT_STATUS=${PIPESTATUS[0]}` is itself a command and resets PIPESTATUS, so the next line's ${PIPESTATUS[1]} was unset and `set -u` aborted the step immediately after the agent finished — before artifact collection, the verdict, or anything else. Verified by replaying the exact structure: 'PIPESTATUS[1]: unbound variable'. Both elements are now snapshotted in one command - concurrency predicates were broader than the job conditions they guard, and GitHub evaluates concurrency BEFORE the job `if`: a /verify comment entered the triage job's shared per-PR group (where it could displace a pending /triage and then skip), and a /verify queued while the runner kill switch was on did the same to a real verification. Both predicates now match their job's runnable set exactly - an outward-resolving .git/hooks entry was only warned about and left in place, so the next root-owned git command would run it. It is now unlinked without traversing its target, a root-owned hooks directory is restored, and core.hooksPath is unset - the second .qwen pin re-derived HEAD^1 from git metadata after the workspace, including .git, had been handed to the build user. The base OID is now recorded while .git is still root-owned and the re-pin archives that content-addressed OID - classify_failure took both of its inputs from PR-controlled sources: a lifecycle script can exit with a signal status and can print any line the log patterns matched, turning its own deterministic breakage into 'infrastructure, please re-run' — which hid the failure and preserved a stale report. No infra verdict is derivable there, so the prepare step reports `fail` and lets the embedded log speak for itself - cleanups descended through PR-writable parents: `.qwen` itself can be a symlink, and the worktree sweep trusted git metadata with only a lexical prefix check. Symlinks are unlinked without traversal and worktree paths must canonicalize inside the workspace. Replayed all three escapes - skipped and docs-only outcomes upload no artifact, so the new download-failure branch pre-empted them and made their real reason unreachable; they are answered first now - a run that crashed before writing report.md still claimed the substantive marker, letting a headline overwrite the previous round's evidence. The marker now requires a report Skill: the byte-identical shortcut needs the whole input closure, not one file hash; the credential-free local path cannot call `gh` at all (fetch the metadata outside and mount it read-only); and the A/B base is `baseRefOid` in local mode, not `HEAD^1`. Tests: 7 new guards plus 4 updated to the new shapes, all mutation-verified (50/50). * fix(triage): answer dropped /verify requests and prove the proxy rejects Maintainer review (yiliang114), 7 items: - a third /verify while two runs are in flight is dropped by the concurrency group with no job and therefore no comment. The hosted authorize job now counts this workflow's other in-flight runs and says so; an API hiccup leaves the request alone rather than denying it - the proxy's bearer check had no executable test. It now starts the real proxy against a real upstream and issues real requests: no header and a wrong token are 401, this run's token is 200, and a route other than /chat/completions is 403 — with the health endpoint echoing the nonce - the 502 path forwarded the raw upstream error, which can name resolved hosts and TLS detail to PR code. It logs server-side and returns a generic failure - publish-verify inherited the 360-minute default; it downloads one artifact and posts one comment, so it is bounded at 10 - removing the log classifier last round left the comment block it replaced, which still said failures are classified from the exit status and the log. Deleted - that removal also left every install failure reported as the PR's fault, including a registry outage. There is exactly one signal here PR code cannot write — asking the registry ourselves, as root, with the container's resolver — so an install failure is downgraded to infra-error only when that probe fails. It proves reachability now rather than at failure time, so it can only ever downgrade, never confirm; a build failure has no equivalent and stays the tree's problem - the skill's local-invocation warning ran into the preceding sentence, which GFM renders as one paragraph Tests: 5 new guards, all mutation-verified (55/55). * fix(triage): resolve hooks hermetically and mirror symlink guards at job end Maintainer review round (doudouOUC), 6 findings. Two were Critical and both reproduced: - the hooks sweep resolved its path with the ambient git config in play. With a global core.hooksPath set — which the reviewer has and I do not, which is why my earlier replay showed a false pass — `git rev-parse --git-path hooks` returns that global path, the in-git-dir guard reads 'outside', and a planted `.git/hooks` symlink survives untouched. A/B: old code leaves the symlink under a global hooksPath, new code removes it in both environments and never touches the link target. Resolution now runs with GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM pointed at /dev/null - the END-of-job cleanup still used the bare `rm -rf .qwen/tmp` that the start-of-job cleaner was hardened against two rounds ago. The agent executes PR code between the two, so the end is no safer than the start: it now unlinks symlinks without descending and canonicalizes worktree paths inside the workspace before deleting Plus four suggestions, all valid: - the saturation notice counted this workflow's in-flight runs across every PR while the concurrency group is per-PR, so a run on another PR would trigger a warning about a queue that does not exist. It now matches on the PR title (the only per-PR handle an issue_comment run record carries) and stays silent when that cannot be resolved - the skill recommended `require.resolve` for the workspace-realpath check; these packages are ESM-only with import-only exports, so it throws ERR_PACKAGE_PATH_NOT_EXPORTED and reads like a missing module. Verified, and replaced with `readlink -f node_modules/@qwen-code/...` - the symlink-escape test inherited the developer's git config, which is what hid the first finding. It now runs with global/system config neutralized AND repeats the case with a global core.hooksPath planted - the publisher's build-phase arm was never rendered by any test (every case used 'install'), so a typo in that command name would have shipped. Now covered, along with an unrecognized phase Mutation-verified 4/4. The hooks guard needed a discriminating assertion: git's own `*.sample` files must survive the sweep, because the outward-path fallback removes the whole directory and would otherwise satisfy a bare 'planted hook is gone' check. * fix(triage): count only /verify runs for saturation, and test the PATCH arm Bot review round, 2 suggestions, both valid: - the saturation notice matched runs by PR title, which narrowed to this PR but not to /verify. /triage and /tmux live in their own concurrency groups, so two of those in flight would warn about a verify queue that is actually empty. It now also requires the run to have a job named 'verify' — the run record carries no command, but its job list does. Replayed: two non-verify runs stay silent, two verify runs warn - every publish fixture returned an empty comments listing, so the PATCH arm was never executed: a broken PATCH would have stranded the running status comment and posted a duplicate below it, with the suite green. The publisher now runs against a stubbed listing and the test asserts which verb went to which comment id — bot-owned live status is PATCHed in place, an absent comment posts fresh, and a marker comment owned by someone else is left alone and posted around Mutation-verified 3/3: counting every command, never PATCHing, and accepting foreign-owned markers each turn one test red. Two stub bugs found while writing these, both mine and both silent: ${*#pattern} applies per positional parameter rather than to the joined string (yielding a wrong run id), and the paginate fixture needs one array per page, not an array of pages. * fix(triage): fix the real silent drop and drop the step built on a wrong premise Review round 4. The blocker was mine twice over: the saturation notice I added last round had GitHub's concurrency semantics backwards, and the silent drop it claimed to cover was somewhere else entirely. - GitHub cancels the OLDER pending run in a group and admits the new one (confirmed against the workflow-syntax reference). My step told the person who had just typed /verify that their request might be dropped, when theirs is the one that runs — and said nothing to the person whose queued run actually died. This PR already had it right in publish-verify's own comment, so the file contradicted itself and the user-facing copy followed the wrong half. The step is removed rather than reworded: with the fix below there is nothing left for it to warn about, and it cost 2+N API calls on every /verify. - the actual drop: a verify job cancelled while still PENDING never reaches a runner, so its outputs block — where the "|| github.event.issue.number" fallback lived — is never evaluated. publish-verify then read an empty PR_NUMBER, hit its own guard and exited 0, making the cancelled branch unreachable in exactly the scenario that produces cancellations. The fallback now lives where the value is read. Reproduced both arms by executing the real step: with a number the cancelled notice posts, with an empty one it only warns. - same one-line class in publish-tmux, fixed alongside. Two copy defects from the classifier removal, both mis-attribution pointed the other way: - the infra-error body still named a signal/OOM kill and a full disk, none of which the current prepare step can produce — infra-error now requires npm ci to fail AND the registry probe to fail. It names that condition only, and offers a re-run instead of asserting it is the fix. - the code comment above it still described the deleted classifier. Also fixes the indentation break an earlier scripted edit left in the publish body builder, and replaces the saturation test with one that executes the cancelled path. Mutation-verified 2/2; the copy needed its own guard, since reverting the wording alone left every test green. * docs(triage): teach verify-pr survivor accounting and observability regressions Fold techniques from the re-verification on QwenLM#7709 that the skill had no equivalent for: - the mutation matrix must report the mutations that changed NOTHING, not only the ones that failed. Each survivor gets classified as an ordinary coverage gap or as dead code — a guard whose deletion leaves every test green is one of those two, and the difference is what the author needs. Survivors mirroring a pre-existing gap are labelled as such, and the set is framed as completeness reporting rather than merge conditions - the sharper case that report demonstrates: a test that passes for the WRONG REASON. If deleting the new guard leaves its own new test green, that test is pinned by an earlier early-return, not by the change, and asserts nothing about it. Name what actually pins it - and do not generalize from one dead guard to its siblings: the same report shows a clause that is unreachable on one path while being the only protection on another. Check each, report the contrast - observability regressions: when a change suppresses output, follow the value before calling the suppression correct. A bare catch on the path plus a field with no readers anywhere in the repo means the cause is now unobservable even in devtools — a real loss that no behavioural assertion can see - report structure gains a Corrections section: when an earlier round or bot comment described the code inaccurately, state the correct fact with evidence and label it as a correction to the description, not a request to change code. A wrong description left standing costs the next reader more than the original finding did * fix(triage): carry the /verify lane's hardening across to /tmux The /tmux job executes the same untrusted PR code, as the same user, on the same persistent self-hosted pool as the /verify lane that QwenLM#7710 hardened. Five of those controls had no equivalent here. Each was found on the verify side by reproducing an attack or a failure, not by reading the code, so the same evidence applies unchanged. - the model proxy bound a FIXED port (8787). PR lifecycle scripts run before it, so a detached child can squat that port: the real proxy then dies with EADDRINUSE while the health probe succeeds against the squatter, and the agent takes its chat completions. Now an ephemeral port published through a root-owned file, a per-run nonce the health endpoint must echo, and a liveness check on the PID we started. Replayed with 8787 occupied: the proxy comes up on an OS-chosen port and answers with the nonce. - nothing swept planted artifact directories. npm ci/build run the PR's lifecycle scripts, which can create tmp/<name>-tmux-<ts>/ holding a report.md and a transcript; the collector globs *-tmux-* and the publisher takes the first match, so a planted directory could supply the comment's contents. Swept after the last PR-controlled process and before the agent. - the global npm install ran with the workspace as cwd, where the PREVIOUS run's checked-out tree still sits. npm reads a cwd .npmrc, and a --registry flag does not override script-shell or hooks, so that config reached a root-privileged install. It now runs from RUNNER_TEMP. - the end-of-job cleanup globbed below .qwen/tmp. PR code ran in this workspace, so either .qwen or .qwen/tmp can be a symlink out of the tree — verified on the verify lane, where the glob deleted the link target's contents as root. Symlinks are unlinked without descending. - emit_block capped the raw log then escaped it. Escaping inflates every & < > by 4-5 bytes, so dense content can push the assembled body past GitHub's 65,536-character comment limit, 422 the post, and leave no comment at all. It now escapes first, caps the escaped bytes, and truncates on a character boundary via node — BSD iconv -c passes an incomplete trailing UTF-8 sequence through unchanged. Tests: a tmux-lane-parity suite, all six mutations verified (restoring the fixed port, dropping the sweep, moving the install back, dropping the symlink guard, reverting to a raw-side cap, and dropping the character-boundary truncation each turn one test red; a no-op control correctly changes nothing). One pre-existing assertion updated: it pinned emit_block's old inline-capture shape, and the guarantee it protects — a render failure is caught — is asserted in the new form. Also adds the regression guard for the publish-tmux PR_NUMBER fallback that landed in QwenLM#7710 without one: a job cancelled while pending never evaluates its outputs, so without the fallback the result comment silently does not post. * fix(triage): address review — symlink guard, artifact strip, bearer auth (QwenLM#7753) * fix(triage): address R2 review — proxy parity, bearer wire tests, process kill (QwenLM#7753) * fix(triage): address R3 review — publisher parity, dedup ownership, cap budget tests (QwenLM#7753) * fix(triage): address R4 review — drop redundant tmux-lane .mjs guards (QwenLM#7753) * fix(triage): address R5 review — tmp symlink sweep guard, proxy timer clear (QwenLM#7753) * fix(triage): address R6 review — hoist proxy timer out of try, dead-upstream 502 tests (QwenLM#7753) * fix(triage): address R7 review — make proxy watchdog idle, end stalled response (QwenLM#7753) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds a unified git mode selector to the Web Shell's new session flow, embedded in the composer's git chip as a popover. Users can now choose between three git workflows when creating a session:
git checkout -b <name>before spawning the session, with validation (branch name, dirty tree check, existing branch check) and automatic rollback on failureThe selector replaces the previous worktree-only toggle. The chip shows color-coded state: green for current branch, orange for new branch (with
→ feat/xxxlabel), purple for worktree. A✕button resets to current branch mode.Why it's needed
Daily development involves three distinct git workflows. Previously only "current branch" and "worktree" were supported at session creation time. Users who wanted to "start a new branch for this task" had to manually
git checkout -bbefore creating a session, or were forced into worktree isolation (which introduces unnecessary directory separation for simple branch work).Reviewer Test Plan
How to verify
npm run dev:daemon) and open Web Shell⎇ main) in the toolbar — it should have a chevron indicating it's clickablefeat/test-mode) and click "Create branch" → the chip turns orange showing→ feat/test-modegit branchin the workspace)✕on the chip → resets to current branch modeGitBranchIconbadge, worktree sessions show the existingGitForkIconbadgeEvidence (Before & After)
Before: Worktree-only toggle in the welcome header, no branch creation support.
After: Unified git mode popover in the composer git chip (real Playwright captures):
API changes
POST /sessionnow acceptsbranch: { name: string }(mutually exclusive withworktree):409 branch_dirty_treeif uncommitted changes exist409 branch_already_existsif the branch existsNew response fields:
DaemonSession.branchandDaemonSessionSummary.branch({ name, baseBranch }).Design doc
docs/design/2026-07-22-webshell-session-git-mode.md