Skip to content

feat(web-shell): unblock git update on dirty working tree - #10390

Merged
wenshao merged 11 commits into
QwenLM:mainfrom
wenshao:feat/git-pull-dirty-worktree-v2
Sep 1, 2026
Merged

wenshao merged 11 commits into
QwenLM:mainfrom
wenshao:feat/git-pull-dirty-worktree-v2

Conversation

@wenshao

@wenshao wenshao commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

The workspace "Update Project" action in the Web Shell now handles a dirty working tree instead of dead-ending on it. When a plain pull is blocked by uncommitted changes, the branch picker's footer switches from an opaque one-line error to a resolution panel with two ways forward: Stash Changes and Update (stash the local changes including untracked files, pull, restore them) or Discard Changes and Update… (behind a second confirming click). Cancelling dismisses the panel; reopening the picker or starting any other action resets it.

The pull endpoint accepts two opt-in booleans, stash and force (mutually exclusive, both off by default). They do exactly what a user would do in a terminal, and the repository is always left in a known state:

  • stash: git stash push --include-untracked, the same git pull as before, then restore. The entry is identified by SHA (refs/stash compared before/after the push, git stash apply <sha>, drop by slot looked up at drop time), never by stack position, so a terminal pushing to the shared stash meanwhile is neither consumed nor in the way. If the pull fails, the merge or rebase it started is aborted and the entry is restored → 409 pull_failed with git's message. If the restore itself conflicts, the response is a success with stashRestoreConflict: true; git keeps the entry and the output names it.
  • force: git reset --hard + git clean -fd (ignored files kept), then git pull --ff-only. Validated before anything is destroyed: fetch first, refuse a diverged branch (409 diverged) while the local changes are still intact, so the post-discard pull can only ever fast-forward. Refused from a workspace below the repository root (409 force_unsupported), because reset --hard would also erase changes outside the workspace.
  • Both are refused (409 operation_in_progress) while a merge, cherry-pick, revert, rebase or am is parked in the worktree, since stash push and reset --hard both clear that state; the failure recovery therefore only ever aborts what the pull itself started.

A plain pull with no option is byte-for-byte the previous behavior. The SDK's workspaceGitPull gains the two options, the stashRestoreConflict result field, and a per-call timeout so the popover can outsize the client's default fetch budget for the multi-command flows.

Replaces #9769. That PR started as this same 743-line feature and grew to +8217/−228 over 18 autofix rounds, each adding a preflight or guard (ignored-file collision probe, per-repo pull lock, identity re-verification, pinned merge flags overriding the user's git policy, hermetic-config shields in tests) whose edge cases the next review round then found — 235 review threads, all unresolved, and the bot itself asking for the PR to be reduced. This PR keeps the three properties that are closed by construction (restore by identity, validate before discard, abort only what we started) and records the rest as explicit non-goals in docs/design/git-pull-dirty-worktree.md: ambient git configuration is honored as in a terminal, ignored files are expendable as in git, and concurrent pulls fail loudly on git's own index lock rather than being serialized. Net: +1627/−77 across the same 12 files, core git-branches.ts +301 lines instead of +1362.

Why it's needed

Users who keep uncommitted work in a workspace could not use the Web Shell git update at all: the pull was refused and the UI only rendered the raw daemon error code, forcing everyone back to a terminal to stash or clean by hand. The workspace list already knows the working tree state for its git chip, so surfacing the two standard resolutions inline closes the loop without leaving the shell.

Reviewer Test Plan

How to verify

Automated coverage runs every layer against real git repositories (bare remote + workspace clone + a second clone standing in for another developer); all targeted runs pass locally:

  • Core (cd packages/core && npx vitest run src/utils/git-branches.test.ts, 72 passed, 15 new): plain dirty pull still refused by git unchanged; stash round trip restores tracked edits and untracked files with an empty stash list; nothing-to-stash is a plain pull; stash + rebase replays the local commit linearly; conflicting restore keeps the entry and reports its SHA; failed merge and failed rebase both restore the exact pre-pull state (HEAD, no MERGE_HEAD/rebase dir, edits, untracked file, empty stash); a foreign stash pushed mid-pull (via a real post-merge hook) is left untouched while ours is applied and dropped by identity; missing upstream fails before stashing; force discards tracked/untracked and keeps ignored; force refuses a diverged branch and a subdirectory cwd before discarding; stash+force throws; merge-in-progress and stopped-rebase refuse both flows and keep the state.
  • Serve routes (cd packages/cli && npx vitest run src/serve/routes/workspace-git-branches.test.ts, 32 passed, 10 new): wrong-typed / combined options → 400; dirty plain pull → 409 dirty_working_tree path-redacted; stash → 200 with changes restored; conflicting restore → 200 + stashRestoreConflict; recovered failure → 409 pull_failed path-redacted with MERGE_HEAD gone; force → 200; diverged force → 409 diverged with edits intact; in-progress merge → 409 operation_in_progress.
  • Web Shell (cd packages/web-shell && npx vitest run client/components/BranchPickerPopover.test.tsx, 10 passed, 7 new; full web-shell suite 4401 passed): panel on dirty 409, stash click → { stash: true } with the 300 s timeout, discard needs the confirm click before { force: true }, panel stays mounted (button disabled) while in flight, stashRestoreConflict renders as a warning, other refusals render the daemon message without the panel, Cancel sends no request, reopen resets.
  • SDK (cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts, 353 passed, 2 new): per-call timeout overrides the client budget on both routes and stays out of the JSON body.

Real stack, no mocks: npm run bundle → node dist/cli.js serve --workspace <fixture> with an isolated QWEN_HOME, driven by Playwright in Chromium with a /git/pull request ledger, then the repository inspected with git after each scenario. Fixture: 20-line README, upstream edits line 1; workspace edits line 20 (clean-restore) or line 1 (conflict-restore), plus an untracked notes.txt and an ignored dist/out.txt.

Scenario Ledger Repository after
Update Project on the dirty tree {} → 409 dirty_working_tree untouched
Discard… then Cancel, then Cancel the panel no further request untouched
Stash Changes and Update {"stash":true} → 200 HEAD at upstream; README has both the upstream line 1 and the local line 20; notes.txt back; dist/out.txt kept; git stash list empty
Stash with a colliding local edit {"stash":true} → 200 + stashRestoreConflict HEAD at upstream; README UU with conflict markers; stash@{0}: qwen-code: auto-stash before pull still carries +line 1 (local WIP); notes.txt back
Discard and Update {"force":true} → 200 HEAD at upstream; tree clean; notes.txt gone; dist/out.txt kept
?lang=zh-CN same 409 panel rendered in Chinese

Evidence (Before & After)

Before (main, from the #9769 verification run) After: resolution panel
The footer renders the SDK label POST /workspaces/:workspace/git/p…, truncated, no actions. Stash / Discard / Cancel.
Discard needs a second click Stash succeeded (pull output only)
Stash restore conflicted (entry kept) Discard succeeded

zh-CN:

Tested on

OS Status
🍏 macOS ✅
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

macOS, Node 24.18.1, git 2.55.0. Unit and integration tests against real local git repositories; real qwen serve daemon from npm run bundle + Playwright/Chromium for the browser evidence. npm run typecheck is clean for every package touched (the only failures in this worktree are the pre-existing ajv-version errors in integrations/external-context-mem0, unrelated to this change); eslint clean on all changed files.

Risk & Scope

  • Main risk or tradeoff: the discard path is destructive — mitigated by the second explicit confirmation, by refusing before discarding whenever the update could not fast-forward, and by keeping ignored files. The stash path can surface a conflict on restore; git keeps the entry and the UI says where the changes are.
  • Not validated / out of scope: the non-goals above are deliberate and documented — no ignored-file collision preflight (git's own semantics, identical to a terminal git pull and to the plain pull on main today), no override of the user's pull.rebase/pull.ff/autostash policy, no cross-request pull serialization. The route's pre-existing text-based classification of plain-pull errors is unchanged.
  • Breaking changes / migration notes: none — both options default to false; stashRestoreConflict and the SDK timeout parameter are additive.

Linked Issues

Replaces #9769 (same feature, reduced to a closed design; see the cross-reference comment there).

中文说明

这个 PR 做了什么

Web Shell 中工作区的「更新项目」操作现在能处理脏工作区,而不是遇到它就卡死。当裸 pull 被未提交的修改阻塞时,分支选择器底部会从一行看不懂的错误切换为一个选项面板,提供两种处理方式:Stash 修改并更新(把本地修改含未跟踪文件 stash 起来、pull、再恢复)或 放弃修改并更新…(需要第二次点击确认)。取消会收起面板;重新打开选择器或执行任何其它操作都会重置它。

pull 接口新增两个可选布尔值 stash 与 force(互斥,默认都关)。它们做的正是用户在终端里会做的事,并且仓库始终处于已知状态:

  • stash:git stash push --include-untracked,跑与之前完全相同的 git pull,再恢复。stash 条目按 SHA 识别(push 前后比对 refs/stash、git stash apply <sha>、drop 时再查槽位),从不按栈顶位置——终端在此期间往共享 stash 里 push 既不会被误消费也不会挡路。pull 失败时中止它自己发起的 merge/rebase 并恢复条目 → 409 pull_failed 附 git 的说明。恢复本身冲突时,响应仍为成功但带 stashRestoreConflict: true;git 保留条目,output 里点名。
  • force:git reset --hard + git clean -fd(保留 ignored 文件),再 git pull --ff-only。破坏性操作前先校验:先 fetch,分叉分支在本地修改尚未丢弃时就拒绝(409 diverged),因此丢弃后的 pull 只可能快进。工作区位于仓库根目录之下时拒绝(409 force_unsupported),因为 reset --hard 会连工作区之外的修改一起抹掉。
  • 工作树中有进行中的 merge / cherry-pick / revert / rebase / am 时两者都拒绝(409 operation_in_progress),因为 stash push 与 reset --hard 都会清掉这些状态;失败恢复因此只会中止 pull 自己发起的操作。

不带选项的裸 pull 与之前逐字节一致。SDK 的 workspaceGitPull 新增两个选项、stashRestoreConflict 结果字段,以及按调用指定的超时,让弹窗为多命令流程放大客户端默认预算。

替代 #9769。 那个 PR 起点就是这个 743 行的特性,经 18 轮 autofix 膨胀到 +8217/−228,每轮加一层预检或护栏(ignored 文件碰撞探针、按仓库的 pull 锁、身份复核、覆盖用户 git 策略的固定 merge 参数、测试里的 hermetic 配置盾),而下一轮评审又找出它们的边角——235 条评审线程全部未 resolve,bot 自己也要求缩减 PR。本 PR 只保留三条构造上闭合的性质(按身份恢复、先校验再丢弃、只中止自己发起的操作),其余在 docs/design/git-pull-dirty-worktree.md 里明确记为非目标:像终端一样尊重环境 git 配置;ignored 文件按 git 语义视为可牺牲;并发 pull 靠 git 自己的 index lock 大声失败而不做串行化。净变化:同样 12 个文件 +1627/−77,core 的 git-branches.ts +301 行而非 +1362。

为什么需要

工作区里有未提交修改的用户完全无法使用 Web Shell 的 git 更新:pull 被拒绝,而 UI 只显示原始错误码,用户只能回到终端手动 stash 或清理。工作区列表的 git 徽标本就掌握工作区状态,把两种标准处理方式直接呈现在界面上,可以不离开 Web Shell 完成闭环。

评审测试计划

如何验证

自动化覆盖逐层针对真实 git 仓库(裸远端 + 工作区 clone + 扮演另一位开发者的第二个 clone)运行,本地定向测试全部通过:

  • Core(cd packages/core && npx vitest run src/utils/git-branches.test.ts,72 通过,新增 15):脏树裸 pull 仍由 git 拒绝且行为不变;stash 往返恢复已跟踪修改与未跟踪文件且 stash 列表为空;无可 stash 内容时等同裸 pull;stash + rebase 线性回放本地提交;恢复冲突时保留条目并报告其 SHA;merge 失败与 rebase 失败都恢复到精确的 pull 前状态(HEAD、无 MERGE_HEAD/rebase 目录、修改、未跟踪文件、空 stash);pull 途中被推入的外来 stash(用真实 post-merge 钩子制造)原样保留,而我们的条目按身份 apply+drop;缺 upstream 在 stash 前就失败;force 丢弃已跟踪/未跟踪并保留 ignored;force 在分叉分支与子目录 cwd 上丢弃前即拒绝;stash+force 抛错;进行中的 merge 与停住的 rebase 让两种流程都拒绝且状态保留。
  • 服务路由(cd packages/cli && npx vitest run src/serve/routes/workspace-git-branches.test.ts,32 通过,新增 10):类型错误/组合选项 → 400;脏树裸 pull → 409 dirty_working_tree 且路径脱敏;stash → 200 且修改恢复;恢复冲突 → 200 + stashRestoreConflict;已恢复的失败 → 409 pull_failed 路径脱敏且 MERGE_HEAD 已清;force → 200;分叉 force → 409 diverged 且修改完好;进行中的 merge → 409 operation_in_progress。
  • Web Shell(cd packages/web-shell && npx vitest run client/components/BranchPickerPopover.test.tsx,10 通过,新增 7;web-shell 全套 4401 通过):脏树 409 出面板,点 stash 以 { stash: true } 与 300 s 超时调用,放弃需先确认才发 { force: true },请求在途时面板保持挂载(按钮禁用),stashRestoreConflict 渲染为警告,其它拒绝显示 daemon 消息且不出面板,取消不发请求,重开重置。
  • SDK(cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts,353 通过,新增 2):按调用的超时在两条路由上都覆盖客户端预算,且不进入 JSON body。

真实栈、无 mock:npm run bundle → node dist/cli.js serve --workspace <夹具> 配隔离 QWEN_HOME,Playwright/Chromium 驱动并记录 /git/pull 请求台账,每个场景后用 git 检查仓库。夹具:20 行 README,上游改第 1 行;工作区改第 20 行(干净恢复)或第 1 行(冲突恢复),另有未跟踪 notes.txt 与 ignored 的 dist/out.txt。

场景 台账 之后的仓库状态
在脏树上点更新项目 {} → 409 dirty_working_tree 未动
放弃…后取消,再取消面板 无后续请求 未动
Stash 修改并更新 {"stash":true} → 200 HEAD 到上游;README 同时含上游第 1 行与本地第 20 行;notes.txt 回来;dist/out.txt 保留;git stash list 为空
本地修改撞行的 Stash {"stash":true} → 200 + stashRestoreConflict HEAD 到上游;README 为 UU 带冲突标记;stash@{0}: qwen-code: auto-stash before pull 仍含 +line 1 (local WIP);notes.txt 回来
放弃并更新 {"force":true} → 200 HEAD 到上游;树干净;notes.txt 消失;dist/out.txt 保留
?lang=zh-CN 同样的 409 面板中文渲染

前后对比证据

改动前(main,取自 #9769 的验证运行) 改动后:选项面板
底部只渲染 SDK 标签 POST /workspaces/:workspace/git/p…,被截断,没有任何操作。 Stash / 放弃 / 取消。
放弃需要第二次点击 Stash 成功(仅显示 pull 输出)
Stash 恢复冲突(条目保留) 放弃成功

zh-CN:

测试环境

操作系统 状态
🍏 macOS ✅
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

macOS,Node 24.18.1,git 2.55.0。单元与集成测试针对真实本地 git 仓库;浏览器证据来自 npm run bundle 起的真实 qwen serve daemon + Playwright/Chromium。所有触及的包 npm run typecheck 干净(本 worktree 仅 integrations/external-context-mem0 有既存的 ajv 版本错误,与本改动无关);改动文件 eslint 干净。

风险与范围

  • 主要风险或权衡:放弃路径是破坏性的——通过二次确认、更新不能快进时在丢弃前就拒绝、以及保留 ignored 文件来缓解。stash 路径恢复时可能出现冲突;git 保留条目,UI 说明修改在哪。
  • 未验证 / 超出范围:上述非目标是刻意且有文档的——不做 ignored 文件碰撞预检(git 自身语义,与终端 git pull 及当前 main 的裸 pull 一致)、不覆盖用户的 pull.rebase/pull.ff/autostash 策略、不做跨请求 pull 串行化。路由里既有的按文本分类裸 pull 错误的逻辑未变。
  • 破坏性变更 / 迁移说明:无——两个选项默认为 false;stashRestoreConflict 与 SDK 超时参数都是增量。

关联 Issue

替代 #9769(同一特性,收敛为闭合设计;见该 PR 上的交叉引用评论)。

The workspace "Update Project" action ran a plain git pull, so any
uncommitted changes left users with a raw dirty_working_tree error
and no way forward outside a terminal.

The pull endpoint accepts two opt-in resolutions, the two things a
user would do in a terminal: stash the local changes (including
untracked files) around the pull and restore them by identity, or
discard them and fast-forward. Both are refused while a merge,
rebase, cherry-pick, revert or am is parked in the worktree, the
discard is validated (fetch + ancestor check) before anything is
destroyed, and a failed stash pull aborts only the merge or rebase
it started before restoring the entry. The branch picker offers the
two resolutions inline when the plain pull is blocked, with the
destructive one behind a confirming click, and renders the daemon's
message for every other refusal.

Ambient git configuration, ignored-file semantics and concurrent
pulls are deliberately left to git; docs/design records each as a
non-goal.
@wenshao

wenshao commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /takeover

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Aug 28, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
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. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

@qwen-code-dev-bot

qwen-code-dev-bot commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator

⚠️ AutoFix round 5 ended without publishing a report — view run.

中文说明

⚠️ AutoFix 第 5 轮结束但未发布报告 —— 查看运行。

…pull

Address the round-1 review of the resolution flows, by identity rather
than by adding guards:

- The auto-stash is captured by provenance (the new entry carrying the
  auto-stash message, from a before/after listing), never as "the top of
  refs/stash", so a terminal push landing in the gap is left alone.
- The drop checks the SHA git reports as dropped; if the slots shifted
  under it, the other entry is stored back and ours is reported as kept.
- Failure recovery aborts a merge or rebase only when its MERGE_HEAD or
  rebase-*/onto points at the upstream tip the pull was integrating; the
  sequencer probe runs immediately before stash push and reset --hard.
- The stash flow always throws a typed pull_failed after aborting, also
  when nothing was stashed or git refused the stash itself, so the client
  shows git's reason instead of re-offering the same resolution.
- The force flow fetches with --prune, re-verifies the upstream, and
  integrates the validated tip with merge --ff-only @{upstream} instead
  of fetching again after the discard.
- Successful responses are path-redacted like the failures; the conflict
  result carries the stash SHA, which the popover now names.
- The popover keeps the panel on force_unsupported, keeps the restore
  warning across a reopen, only dismisses the panel for a branch creation
  that actually runs, and allows 420s for the multi-command flows.

Tests drive the concurrent interleavings deterministically through a
PATH git shim that injects a terminal's actions around one invocation.
@wenshao

wenshao commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Round-1 review addressed in 2abcc2c963 (all 17 threads replied and resolved), and main merged in a0c7b7c310 — the conflicts were with #10397's action hints (new i18n keys and the popover test's mock shape; both sides kept). Every finding was taken, but closed by identity rather than by adding locks or preflights:

Finding What changed
R1-1 capture by position The entry is picked from a before/after stash list diff by its auto-stash subject (pushAutoStash), never as "the new top".
R1-2 second fetch after discard git merge --ff-only @{upstream} integrates the validated tip; no re-fetch.
R1-3 one-shot sequencer guard Probe re-runs immediately before stash push and before reset --hard; recovery aborts only when MERGE_HEAD / rebase-*/onto equals the upstream tip the pull integrated (abortOwnPullState).
R1-4 positional drop Drop reports its SHA (Dropped refs/stash@{n} (<sha>)); a mismatch stores the other entry back and keeps ours.
R1-5 stale tracking ref fetch --prune + upstream re-verified before anything is discarded (409 pull_failed, nothing touched).
R1-6 raw rethrow before abort The stash flow always aborts (by identity) and throws a typed pull_failed, also when nothing was stashed.
R1-7 unredacted 200 200 bodies go through the same path redaction (without the error cap).
R1-11 / 12 / 13 / 14 / 17 (popover) 420 s budget; panel dismissed only by a branch creation that runs; restore warning survives a reopen; warning names the stash entry (stashSha threaded core → SDK → UI); force_unsupported keeps the panel with the daemon's explanation.
R1-16 stash refused git stash push failure (intent-to-add) is a typed pull_failed with git's reason.
R1-8 / 9 / 10 / 15 Tests added (unrestorable-entry arm, cherry-pick and revert guards, competing checkout, route force_unsupported).

The concurrent interleavings are now pinned deterministically: a PATH git shim runs the real binary and injects a terminal's action immediately before or after one invocation (a foreign stash push after ours, a foreign push before the drop, a conflicting merge topic before the pull, an orphan force-push after the ancestor check, an untracked file after merge --abort).

Local runs on the pushed head: core git-branches.test.ts 85 passed (13 new), route 34 passed (2 new), popover 38 passed (4 new, plus the fixture update for #10397), web-shell suite 4440 passed, SDK 353 passed. Real stack (bundled qwen serve + Chromium, same fixture as the description) re-run after the change: stash → 200 with the entry restored and dropped; conflicting restore → 200 + stashRestoreConflict + stashSha, the footer now naming the entry; discard → 200 via merge --ff-only.

Restore conflict now names the entry Clean restore (pull output only)

Two things outside this PR, for the record: the earlier web-shell visuals failure on 3d83d4a was the sidebar scenario's Run auth migration strict-mode violation (two matches), not a branch-picker scenario; and main's new @xterm/addon-fit dependency (#9984) must be installed for the web-shell build — a stale node_modules silently leaves dist/web-shell at the previous build.

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Could not produce a passing fix for this feedback (round 1/100) — the verification gate rejected the attempt. This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own.

⚠️ This change was NOT pushed — any commit referenced below was made only in the runner workspace and has been discarded. What the agent reported:

Autofix review round 2 — PR #10390

Same-run verification repair round. The previous commit 13a8cc719a (which
resolved all 7 Critical findings plus R1-11 and R1-16) is preserved; this
round adds one verified follow-up commit 9ebf90d2b6 that addresses the
remaining 8 deferred Suggestions and re-verifies the tree. Net growth this
round: source +56 / test +213 (budgets 400/400).

Same-run verification rejection: environmental, evidenced

The deterministic gate rejected 13a8cc719a because
vitest run --changed origin/main --passWithNoTests in packages/cli reported
9 failed test files / 15 failed tests. Evidence gathered this round:

  • None of the 9 failing files is touched by this PR: src/commands/serve.test.ts, src/commands/update.test.ts, src/i18n/index.test.ts, src/serve/server-default-bridge-wiring.test.ts, src/serve/workspace-registration-store.test.ts, src/ui/voice/voice-keyterms-race.test.ts, src/ui/utils/clipboardUtils.test.ts, src/serve/voice/resolve-voice-config.test.ts, src/commands/review/lib/git.integration.test.ts (extracted from the gate's junit.xml).
  • Failure signatures are resource exhaustion, not behavior: 12 of 15 are bare timeouts (15s/20s/30s); one is an explicit error: could not lock config file .git/config: No space left on device; the run's collect phase alone consumed ~19,000s of CPU across the parallel suite.
  • The runner's /tmp is a 124G tmpfs at 99% usage (2.0G free) — measured with df at round

Why it was not pushed:

tests failed in packages/cli

90m213| �[39m        ([baseDir]) �[33m=>�[39m path�[33m.�[39m�[34mresolve�[39m(�[33mString�[39m(baseDir))�[33m,�[39m
    �[90m214| �[39m      )�[33m;�[39m
    �[90m215| �[39m      �[34mexpect�[39m(writtenBaseDirs)�[33m.�[39m�[34mtoEqual�[39m([
    �[90m   | �[39m                              �[31m^�[39m
    �[90m216| �[39m        path�[33m.�[39m�[34mresolve�[39m(runtime)�[33m,�[39m
    �[90m217| �[39m        path�[33m.�[39m�[34mresolve�[39m(stable)�[33m,�[39m

�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[22/29]⎯�[22m�[39m

�[41m�[1m FAIL �[22m�[49m src/serve/voice/resolve-voice-config.test.ts�[2m > �[22mloadDaemonVoiceContext�[2m > �[22mresolves providerProtocol-mapped custom provider groups for voice
�[31m�[1mError�[22m: Test timed out in 15000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".�[39m
�[36m �[2m❯�[22m src/serve/voice/resolve-voice-config.test.ts:�[2m97:3�[22m�[39m
    �[90m 95| �[39m  })�[33m;�[39m
    �[90m 96| �[39m
    �[90m 97| �[39m  it('resolves providerProtocol-mapped custom provider groups for voic…
    �[90m   | �[39m  �[31m^�[39m
    �[90m 98| �[39m    // Managed deployments can place a gateway under a custom provider…
    �[90m 99| �[39m    // id; the daemon's ModelsConfig must thread providerProtocol thro…

�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[23/29]⎯�[22m�[39m

�[41m�[1m FAIL �[22m�[49m src/serve/voice/resolve-voice-config.test.ts�[2m > �[22mloadDaemonVoiceContext�[2m > �[22mskips workspace settings when the runtime is untrusted
�[31m�[1mError�[22m: Test timed out in 15000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".�[39m
�[36m �[2m❯�[22m src/serve/voice/resolve-voice-config.test.ts:�[2m135:3�[22m�[39m
    �[90m133| �[39m  })�[33m;�[39m
    �[90m134| �[39m
    �[90m135| �[39m  it('skips workspace settings when the runtime is untrusted', async (…
    �[90m   | �[39m  �[31m^�[39m
    �[90m136| �[39m    mocks�[33m.�[39mloadSettings�[33m.�[39m�[34mmockReturnValue�[39m({
    �[90m137| �[39m      merged�[33m:�[39m {

�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[24/29]⎯�[22m�[39m


�[2m Test Files �[22m �[1m�[31m9 failed�[39m�[22m�[2m | �[22m�[1m�[32m690 passed�[39m�[22m�[90m (699)�[39m
�[2m      Tests �[22m �[1m�[31m28 failed�[39m�[22m�[2m | �[22m�[1m�[32m21976 passed�[39m�[22m�[2m | �[22m�[33m90 skipped�[39m�[90m (22094)�[39m
�[2m   Start at �[22m 07:48:58
�[2m   Duration �[22m 505.25s�[2m (transform 820.57s, setup 267.44s, collect 20452.29s, tests 1628.69s, environment 470.83s, prepare 276.95s)�[22m

JUNIT report written to /home/github-runner/actions-runner-test-22/_work/qwen-code/qwen-code/packages/cli/junit.xml
npm error Lifecycle script `test` failed with error:
npm error code 1
npm error path /home/github-runner/actions-runner-test-22/_work/qwen-code/qwen-code/packages/cli
npm error workspace @qwen-code/[email protected]
npm error location /home/github-runner/actions-runner-test-22/_work/qwen-code/qwen-code/packages/cli
npm error command failed
npm error command sh -c vitest run --changed origin/main --passWithNoTests
中文说明

🤖 未能为该反馈产生可通过验证的修复(第 1/100 轮) —— 验证门拒绝了该尝试。此项现在需要人工处理;循环保持在线,仍会拾取新反馈与 base 冲突,但不会自行重试此项。

验证门的拒绝原因与日志证据见上方英文部分(gate-rejection 不翻译)。

Run log: https://github.com/QwenLM/qwen-code/actions/runs/33208001835


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run.

中文说明

🔀 已更新 base:红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。

… best-effort

Address the round-2 review of the resolution flows:

- Failure recovery aborts a merge or rebase only when git itself created
  it: a pre-existing state makes `git pull` exit 128 before touching the
  tree, so only an exit of 1 with the state pointing at the integrated
  upstream tip identifies the pull's own. A same-tip merge a terminal
  parked meanwhile is left in place with its staged resolution.
- Every recovery step is best-effort: a failed probe or listing never
  turns a recovered repository into an unclassified error, the post-push
  re-listing failure points at the entry by its message, and a failed
  store-back after a drop shift names the displaced entry and the command
  that recovers it.
- Both flows fetch (`--prune`) before checking the upstream, so a pruned
  tracking ref heals when the remote branch exists again; a configured
  upstream whose branch is gone is a typed refusal, while a branch with
  no upstream keeps git's own message.
- The force flow fast-forwards to the validated SHA rather than the
  symbolic `@{upstream}`, which a concurrent fetch can move.
- Kept-entry notices always carry the SHA; the restored arm of a failed
  update carries the drop diagnostic; silent git failures name the usual
  lock-file cause; the popover budget covers the 16-command worst case.
@wenshao

wenshao commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

Round-2 review addressed in 0780078ec7 (all 15 threads replied and resolved; the bot's base merge d29b524a97 is kept as-is). The theme of this round was "recovery must have provenance and must never fail":

Finding What changed
R2-18 same-tip terminal merge aborted Provenance from git's exit code: a pre-existing merge/rebase makes git pull exit 128 before touching the tree, so only an exit of 1 (git attempted and stopped on conflicts) plus the tip check identifies the pull's own state. Your probe's interleaving now leaves MERGE_HEAD and the staged resolution untouched.
R2-1 / R2-8 unguarded recovery listings abortOwnPullState and the post-apply listing are best-effort (typed failure always reached, entry named by SHA); a failed post-push re-listing stops before pulling and points at the entry by its label.
R2-2 swallowed store-back Failure is observable: the displaced entry's SHA and git stash store <sha> are in the output.
R2-9 merge by symbolic @{upstream} merge --ff-only <validated sha>; the witness now also fetches in the window and asserts the interleaving happened (R2-10).
R2-14 upstream checked before the fetch Both flows fetch --prune first; a pruned-but-configured upstream heals when the branch is back, a still-missing one is a typed refusal, no upstream keeps git's own message.
R2-4 / R2-5 / R2-6 / R2-20 Tests: stopped git am, stashSha producer pinned in core and route, competing push and valid new-branch dismiss the panel, non-sticky blocked line reset on reopen.
R2-17 / R2-19 / R2-21 / R1-11 Silent git failures name the lock-file cause; the restored arm carries the drop diagnostic ahead of git's detail; drop-failure notice carries the SHA; popover budget 600 s for the 16-command worst case.

Local runs on the pushed head: core git-branches.test.ts 93 passed (8 new), route 34 passed, popover 41 passed (3 new), typecheck clean for every touched package. The Test (ubuntu-latest) red on 2abcc2c963 was run-qwen-serve.test.ts (daemon logger / env-scrub cases, untouched here) and went green on the base-updated head.

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run.

中文说明

🔀 已更新 base:红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 AutoFix ran out of time before finishing (timeout (3600000ms)) (attempt 2/100) — it will retry on the next scan.

⚠️ This change was NOT pushed — any commit referenced below was made only in the runner workspace and has been discarded. What the agent reported:
Qwen failed during address-review: timeout (3600000ms).

See the Qwen Autofix agent step logs for model/tool output.

中文说明

🤖 AutoFix 在完成前耗尽了时间(timeout (3600000ms))(第 2/100 次尝试)—— 将在下次扫描时重试。

Run log: https://github.com/QwenLM/qwen-code/actions/runs/33264363801


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

main moved the daemon React bindings out of @qwen-code/webui and into
web-shell's own client/daemon-react-sdk, rewriting every import. Take
main's path for useWorkspace and keep this branch's DaemonHttpError
import; the pull API this branch extends still lives in
@qwen-code/sdk/daemon, which the forked bindings re-use unchanged.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 AutoFix hit a verification-gate error before reaching a verdict (attempt 3/100) — it will retry on the next scan.

⚠️ This change was NOT pushed — any commit referenced below was made only in the runner workspace and has been discarded. What the agent reported:

Round-3 summary

This round was scoped down per the budget warning (the previous round exhausted
its time budget): only the two new round-3 findings were implemented, the
round-2 findings the maintainer already fixed in 0780078ec7 were re-verified
against the code and their test suites, and the red Java 17 check was traced
to its inputs. No conflict merge was needed (--conflict false).

Implemented this round (commit 42feea9756)

  • R3-1 — types the failure and names the entry when the auto-stash cannot be re-listed
    compared git rev-parse HEAD with itself, so "the update never moved HEAD"
    was unpinned. The test now captures headBefore before the pull and asserts
    against it. Mutation witness: deferring the re-list-failure throw in
    pushAutoStash until after the pull makes the test fail (expected '139799b…' to be '55ecd83…' — HEAD advanced); with the code restored it is green.
  • R3-12 — the success-path kept/displaced-entry notices from restoreStash
    rendered as a non-sticky success line that a reopen wiped, leaving a dropped
    entry's recovery command as a dangling commit the user can no longer name.
    Implemented with the structured-field option (the reviewer's recommended one,
    matching the existing stashSha precedent) instead of string-sniffing: core
    sets stashKept whenever a successful stash pull carries a kept-entry
    notice, the SDK type carries it, and the popover renders it as a sticky
    warning like `stashRest
中文说明

🤖 AutoFix 在得出结论之前遇到验证门错误(第 3/100 次尝试)—— 将在下次扫描时重试。

Run log: https://github.com/QwenLM/qwen-code/actions/runs/33271344698


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 AutoFix hit a verification-gate error before reaching a verdict (attempt 4/100) — it will retry on the next scan.

⚠️ This change was NOT pushed — any commit referenced below was made only in the runner workspace and has been discarded. What the agent reported:

Round summary

Addressed the round-4 review of the dirty-worktree pull flows (PR #10390). Commit 383c178efa on feat/git-pull-dirty-worktree-v2.

Feedback points and dispositions

Finding Disposition What changed
[Critical] R4-1 (rc:3887937825) — a pull killed by the flow's own 30s timeout carries killed: true with no numeric exit code, so abortOwnPullState skipped the abort; git writes MERGE_HEAD before merging contents, so the pull's own conflicted merge stayed parked, was reported as fully restored, and a later commit could finalize the stray merge Fixed New pullExitCode() treats a pull that died without an exit code (killed or signal set) as 1 for abort purposes; the tip-identity check in abortOwnPullState is unchanged, so a merge a terminal parked meanwhile is still left alone. Pinned by aborts the conflicted merge a killed pull left behind: a shim runs a real conflicting merge and dies by SIGTERM like a timeout kill; the test asserts MERGE_HEAD is gone, the restore happened, HEAD did not move, and the stash stack is empty.
[Critical] R4-2 (rc:3887937828) — forcePull's final merge --ff-only can fail after reset --hard + clean -fd when a skip-worktree file blocks it; the raw error was classified as dirty_working_tree and the branch picker re-offered a discard that can never succeed, looping forever Fixed The merge tail is wrapped in core and throws `GitPullFailure('pull_failed', 'd
中文说明

🤖 AutoFix 在得出结论之前遇到验证门错误(第 4/100 次尝试)—— 将在下次扫描时重试。

Run log: https://github.com/QwenLM/qwen-code/actions/runs/33280763433


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 AutoFix stopped after 5 consecutive rounds that pushed nothing (failed rounds, timeouts, gate rejections, or stops under instruction). Retrying at the same per-round budget is not converging — this usually means the PR is too large or conflicts with a fast-moving main. A human should rebase, split, or reduce it, then comment @qwen-code /retry to re-arm. Until then future scans will skip this PR.

⚠️ This change was NOT pushed — any commit referenced below was made only in the runner workspace and has been discarded. What the agent reported:

Round summary — PR #10390 (commit 4e55698bfc)

Addressed both round-4 Critical findings and the still-standing R3-1 test pin. Deferred R3-12 (non-blocking Suggestion) to the next round because this run carried a budget warning — the previous round ran out of time before finishing anything, so the batch was bounded to the smallest blocking subset.

Feedback dispositions

Finding Disposition What changed
R4-1 [Critical] killed pull leaves its own merge parked (rc:3887937825) Fixed A pull that dies without an exit code is now treated like a conflicted exit 1 when deciding whether to abort parked merge/rebase state, via a new pullExitCode helper in the stash flow's recovery; the tip-identity guard in abortOwnPullState is unchanged. One deliberate widening beyond the suggested .killed check: a signal-shaped death also counts. Node reports killed: true only for kills it initiated itself; an external or OOM kill arrives as signal with killed: false (verified by probing execFile's error shapes), and it leaves the identical stranded MERGE_HEAD with the same downstream history-corruption path. Witness: stash pull aborts the merge it started when the pull is killed mid-integration — the shim runs the real fetch, makes the conflicting merge that writes MERGE_HEAD, then dies by SIGTERM (the exact timeout-kill shape). Removing the kill handling leaves MERGE_HEAD parked and turns the test red.
R4-2 **[Critica
中文说明

🤖 AutoFix 已停止:连续 5 轮未能推送任何内容(失败轮次、超时、验证门拒绝或按指示停止)。以相同的单轮预算重试并不收敛 —— 这通常意味着 PR 过大,或与快速变动的 main 冲突。应由人工 rebase、拆分或缩减它,然后评论 @qwen-code /retry 重新武装。在此之前,后续扫描将跳过本 PR。

Run log: https://github.com/QwenLM/qwen-code/actions/runs/33280928936


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/needs-human The autofix loop stopped on this PR — a human must re-arm, split, merge, or close it label Aug 30, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

⏸️ Takeover paused: this PR reached its round cap (100/100). Comment @qwen-code /takeover to re-arm a fresh window and continue management, or @qwen-code /takeover stop to release.

中文说明

⏸️ 托管已暂停:本 PR 达到轮次上限(100/100)。评论 @qwen-code /takeover 可重新武装、开启新窗口继续托管;或评论 @qwen-code /takeover stop 释放。

Address the round-3/4 review of the dirty-worktree pull flows:

- A pull that dies without a numeric exit code — the flow's own 30s
  timeout kill, or an external/OOM signal — may already have written its
  MERGE_HEAD, so it now counts like a conflicted exit for the abort
  decision; the tip-identity guard is unchanged, so a merge a terminal
  parked meanwhile is still left alone. Previously the pull's own merge
  stayed parked while the response claimed a full restore, and a later
  commit could finalize the stray merge and silently exclude the
  upstream's content from history.
- The force flow's final ff-only merge is wrapped: a refusal after the
  discard (e.g. a skip-worktree file the reset cannot clear) is a typed
  pull_failed instead of raw text the route would re-classify as
  dirty_working_tree, looping the panel on a discard that can never
  succeed.
- A successful stash pull that had to keep a stash entry (failed drop,
  slot shift, failed store-back) reports it structurally (stashKept +
  stashSha); the popover renders that notice as a sticky warning, since
  it is the only record of where the entries went.
- The re-list-failure test pins "HEAD never moved" against a captured
  headBefore instead of comparing rev-parse with itself.
@wenshao

wenshao commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Rounds 3–4 addressed in 3f1511df09, and the branch is current again: ed679357ab merges today's main (58 commits, clean auto-merge — the sdk types / i18n / popover-test overlaps merged without conflict).

Finding What changed
R4-1 killed pull leaves its own merge parked pullExitCode treats a pull that died without a numeric code (killed: true from the flow's own 30 s timeout, or a signal-shaped external/OOM kill) as a conflicted exit for the abort decision; the tip-identity guard is unchanged. Pinned by a shim that performs the real fetch + conflicting merge and dies by SIGTERM: MERGE_HEAD gone, HEAD unmoved, changes restored, stash empty.
R4-2 post-discard force failure re-classified as dirty_working_tree The final merge --ff-only <validated> is wrapped in core: a refusal becomes pull_failed ("discard applied, but the update failed: …"), so the panel cannot loop on a discard that can never succeed. Pinned with a skip-worktree file the reset cannot clear.
R3-12 kept/displaced-entry notice wiped on reopen Structured, as recommended: core sets stashKept + stashSha whenever a successful stash pull kept an entry (failed drop, slot shift, failed store-back), the SDK type carries it, and the popover renders the notice as a sticky warning like stashRestoreConflict. Ordinary success lines stay non-sticky.
R3-1 self-comparing HEAD assertion headBefore captured before the call; the mutant that defers the re-list-failure throw now goes red.

Local runs on the pushed head: core git-branches.test.ts 95 passed (2 new), route 34, popover 42 (1 new), SDK 397, full web-shell suite 5391 passed, and tsc --noEmit clean in core / cli / sdk-typescript / web-shell.

On the two red checks on the previous head, for the record: Test (ubuntu-latest) failed in src/json-string-bytes.test.ts (a 5 s timeout in a file this PR does not touch), and web-shell E2E Smoke died on the job's 20-minute execution cap with the retries stuck in the collapsed-groups and github-prs specs — the git-mode spec passed and does not exercise the branch-picker pull. Both should re-run on the updated head.

@wenshao

wenshao commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

On the Test (ubuntu-latest) red at 3f1511df09: the 7 failures were spread across five files this PR does not touch (voice-keyterms-race, hook-runner.process, recall-scan-latency, shellReadOnlyChecker, imageSupport.bundle), every signature a wall-clock budget or timeout (126 ms > 50 ms, 1390 ms > 1000 ms, 5 s / 10 s test timeouts) on a job that ran 1 h 23 m — shared-runner contention, the same class #10648 ("stabilize shared-runner budgets", merged to main two minutes before that run started) is addressing. All five files pass locally on the same head (251 + 3 + 2 tests).

2c8b619dcc merges current main (7 commits, clean; brings #10648) and re-runs CI. The PR's own suites on the merged head: core git-branches.test.ts 95, SDK DaemonClient 400, popover 42 — all green.

@qqqys

qqqys commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Review + verification report (head 2c8b619)

Critical review — no merge-blocking issue found.

Full diff read at head; the load-bearing claims check out against the code:

  • core git-branches.ts: the auto-stash entry is identified by provenance (pre-push listing diff + message match), applied by SHA, and dropped via a slot resolved immediately before the drop, with the SHA git reports as dropped verified afterwards — a displaced entry is stored back and the result says so. git stash pop is never used.
  • force path validates before destroying anything: fetch --prune → upstream resolution → merge-base --is-ancestor HEAD <upstream-sha> (exit 1 → 409 diverged while local changes are still intact), then integrates exactly the validated commit via merge --ff-only <sha>; workspaces below the repository root are refused via realpath comparison before any reset --hard. Ignored files are kept (clean -fd without -x).
  • sequencer-state guard resolves MERGE_HEAD / CHERRY_PICK_HEAD / REVERT_HEAD / rebase-merge / rebase-apply through rev-parse --git-path (linked-worktree safe) and runs immediately before each absorbing command; both flows refuse while one is parked.
  • failed-pull recovery only aborts what the pull itself started: exit code 1 plus a provenance check (MERGE_HEAD content / rebase onto equals the upstream tip); a kill without exit code is treated as 1 and left to the tip-identity check. The dropped-stash parse is stable because gitEnv pins LC_ALL=C (verified in head).
  • route layer: strict boolean validation → 400, mutual exclusivity → 400, GitPullFailure → 409 with the typed code, and path redaction now covers the success output as well (previously unredacted). Plain-pull behavior is unchanged.
  • SDK: options are additive; the per-call timeoutMs rides existing jsonRequest support and stays out of the JSON body (only the options object is serialized).
  • web-shell: discard requires the second confirming click; a force_unsupported refusal keeps the panel up with the daemon's explanation; the restore warning is sticky across reopen; competing actions reset the panel; en + zh strings are both present.
  • no other production callers of gitPull exist at head.

Local verification at head (scratch build from the head tarball, npm ci + npm run bundle):

  • Units: core git-branches.test.ts 94/95, cli workspace-git-branches.test.ts 34/34, sdk DaemonClient.test.ts 400/400, web-shell BranchPickerPopover.test.tsx 42/42.
    • The single core failure is environmental, not a defect: types the refusal with a lock hint when the index is wedged asserts git's wording contains "lock", but this host's git 2.43.7 emits error: could not write index for a wedged .git/index.lock — reproduced with a raw git stash push here — while the newer git in CI (and the author's 2.55) includes the lock hint. The typed refusal and the untouched tree hold under both gits.
  • Real-stack e2e: bundled node dist/cli.js serve --workspace <fixture> in tmux (loopback bind, hermetic QWEN_HOME); fixture = bare upstream + dirty workspace clone (line-20 WIP edit, untracked notes.txt, ignored dist/out.txt), with upstream edits pushed from a second clone:
Scenario Request Response Repository after
plain pull, dirty tree {} 409 dirty_working_tree untouched
stash & update {"stash":true} 200 HEAD at upstream; README has upstream line-1 edit and local line-20 edit; notes.txt restored; dist/out.txt kept; git stash list empty
stash w/ colliding line-1 edit {"stash":true} 200 + stashRestoreConflict + stashSha HEAD at upstream; README UU with conflict markers; auto-stash entry kept and named; non-conflicting line-20 edit applied
force on diverged branch {"force":true} 409 diverged local commit and all files intact
force on dirty tree {"force":true} 200 HEAD at upstream, tree clean, notes.txt gone, dist/out.txt kept
stash / force w/ merge parked each 409 operation_in_progress MERGE_HEAD preserved
wrong-typed / combined options {"stash":"yes"} / both true 400 invalid_stash / invalid_stash_force —

CI at head: product lanes green (Test ubuntu, Integration no-AK, web-shell smoke, Desktop Shell, Java); mac/win/CLI-integration lanes skipped as normal for fork PRs. Not approving — no maintainer/ci-bot review on this head yet.

Automated verification round by qqqys (review-only mandate; local builds at the PR head, no mocks in the e2e stack).

@wenshao

wenshao commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the full head at 2c8b619dccf and re-verified every previously reported Critical against the current tree. No blockers found.

The destructive/recovery invariants now hold: auto-stashes are identified by provenance and SHA; restore/drop verifies the SHA actually removed and surfaces a recoverable displaced-entry command; recovery distinguishes pull-created merge/rebase state from a terminal's parked operation; killed pulls take the same safe recovery path; and discard fetches/prunes, validates fast-forwardability, then integrates the exact validated commit rather than a movable upstream ref. Post-discard checkout failures are typed instead of looping back to the dirty-tree panel. The legacy and workspace-qualified routes retain their ownership/trust boundaries, validate mutually exclusive options, and redact git paths on both success notices and typed failures. SDK timeout plumbing and the Web Shell's two-step discard/sticky stash warning match the wire contract.

All current reported CI lanes are green (26 passing; remaining checks are conditional skips). The remaining round-5/6 items are non-blocking follow-ups under the repository's convergence policy. Approving.

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff with focus on the destructive paths — LGTM, the three by-construction properties hold in code:

Restore by identity, never by position: pushAutoStash identifies its entry via pre/post SHA-set diff + subject match; restoreStash applies by SHA (never stash pop), resolves the slot immediately before drop, verifies the SHA git reports as dropped, and re-stores a displaced entry if the stack shifted — a concurrent terminal push is neither consumed nor blocked.

Validate before discard: forcePull refuses sub-repo-root workspaces via realpath comparison, fetches with --prune, and requires merge-base --is-ancestor HEAD <upstream-tip> before reset --hard + clean -fd; the integration then merges the validated SHA with --ff-only rather than a symbolic ref that could move. Diverged branches and gone upstreams are typed refusals with the tree untouched.

Abort only what the pull started: abortOwnPullState acts only on exit code 1 and proves provenance via tip identity (MERGE_HEAD vs upstream, rebase onto vs upstream from head-name); a parked terminal merge/rebase is left alone, and refuseOperationInProgress (resolved through --git-path, so linked worktrees can't mislead) is re-checked immediately before each state-destroying command.

Route layer: boolean typing + mutual-exclusion validation, and path redaction applied on every response including the success output and the unclassified fall-through. Plain pull is byte-for-byte the previous path.

CI green (Ubuntu tests, no-AK integration, Desktop Shell both OSes, web-shell E2E smoke, visuals, CVE/TruffleHog). Note: the remaining CHANGES_REQUESTED verdicts are stale ci-bot reviews on superseded commits (latest head has 0 unresolved threads).

@ytahdn

ytahdn commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Code review: dirty-worktree git pull resolution (bd59085..2c8b619)

Read-only review of the full diff against the PR description and docs/design/git-pull-dirty-worktree.md. Tests were not re-run here; assessment is from code reading.

Verdict: Ready to merge ✅

No Critical issues. One Important finding worth addressing; the rest are Minor.

Strengths

  • The three "closed by construction" properties are genuinely closed in code:
    • Restore by identity — git stash apply <sha>; the drop resolves the slot at drop time and re-verifies the SHA git reports, compensating with git stash store if slots shifted (git-branches.ts:794-880).
    • Validate before discard — fetch → ancestor check → 409 diverged while edits are intact; the validated SHA is integrated via merge --ff-only, so a concurrent remote rewrite cannot turn a validated fast-forward into a post-discard refusal (forcePull, git-branches.ts:1005-1085).
    • Abort only what we started — exit 128 ⇒ pre-existing state left alone; exit 1 + MERGE_HEAD/rebase-onto matching the integrated upstream tip ⇒ the pull's own state (abortOwnPullState, git-branches.ts:711-771). Killed/timeout pulls count as exit 1, with the tip check as final arbiter (pullExitCode, git-branches.ts:609).
  • Exceptional test quality: real bare remotes + two clones, a git PATH shim injecting terminal actions deterministically, a real post-merge hook for the concurrent stash. The interleaving tests pin the exact failure modes (foreign stash left alone, slot-shift store-back, terminal-parked merge left alone, remote-moves-after-check, wedged-index refusals) by asserting real repo state.
  • Backward compatibility is clean: options default false; stashRestoreConflict/stashSha/stashKept additive; SDK timeoutMs is a separate jsonRequest option threaded to fetchWithTimeout and never enters the JSON body (DaemonClient.ts:999-1050); plain pull's git invocation and the route's pre-existing error classification are unchanged.
  • Non-goals respected: no extra preflights, no per-repo locks, no policy overrides. The success output is now also path-redacted (workspace-git-branches.ts:301-307).

Important (Should Fix)

  1. fetchOnly silently wins over stash/force — missing combination validation. packages/core/src/utils/git-branches.ts:1084 checks opts.fetchOnly before stash/force, so { stash: true, fetchOnly: true } performs a bare fetch and silently ignores the stash request. The route validates the types of all four fields and rejects stash && force (route :268-287) but never rejects fetchOnly combined with either option. This is the kind of silent option-drop the design otherwise refuses (typed GitPullFailure codes over silent behavior), and the SDK now publicly documents these options, so a future caller can hit it. Fix: one 400 in the route (fetchOnly + stash/force → typed invalid_fetch_only_combination) plus a guard in gitPull mirroring the stash && force check, with a route test.

Minor (Nice to Have)

  1. Dead throw in validatedUpstream. git-branches.ts:897-901 runs rev-parse --abbrev-ref @{upstream} solely to produce git's native message, then throws a GitPullFailure — but that rev-parse either succeeds (contradicting the earlier --verify --quiet failure at :889) or throws, so the final GitPullFailure is unreachable. Either restructure to produce the typed failure explicitly or delete the dead branch.
  2. Detached-HEAD stash/force via SDK → unclassified 500. On a detached HEAD, validatedUpstream surfaces git's fatal: HEAD does not point to a branch, which matches none of the route's regexes (route :84-129), falling through to a 500. Unreachable from the UI (pull disabled on detached HEAD, BranchPickerPopover.tsx:714), so it is SDK-surface-only; a typed refusal would fit the design better.
  3. force_unsupported keeps a dead Discard button in the panel. For a subdirectory workspace, the panel (BranchPickerPopover.tsx:964-1023) still offers Discard, looping confirm → force_unsupported forever. Stash is the only viable option there; consider dimming/hiding Discard when the refusal was force_unsupported.
  4. Store-back changes the displaced entry's position. The drop-shift compensation (restoreStash) puts the displaced entry back on top of the stack, so a terminal's next git stash pop picks it up instead of the user's own entry. Identity-based flows are unaffected and the output explains it; worth a sentence in the design doc so it isn't mistaken for a bug.
  5. "Byte-for-byte previous behavior" is not quite literal. The route now path-redacts the success output of the plain pull (route :301-307). Benign in practice — the claim is about the git invocation, not the response bytes.

Recommendations

  • Add the fetchOnly-combination 400 and a route test pinning it.
  • The gitShim concurrency tests are skipIf(!unix): the SHA-verified drop and slot-shift compensation are untested on Windows git. Verify the Dropped refs/stash@{N} (<sha>) message format and LC_ALL=C pinning once against Git-for-Windows, since parseDroppedStashSha (git-branches.ts:773) is the single point the drop-shift safety rests on.

@wenshao
wenshao dismissed a stale review September 1, 2026 02:47

fixed

@wenshao
wenshao added this pull request to the merge queue Sep 1, 2026
Merged via the queue into QwenLM:main with commit 0547f18 Sep 1, 2026
91 checks passed
@wenshao

wenshao commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review — sharp catches. Dispositions, now that this is merged:

netbrah pushed a commit to netbrah/qwen-code-upstream-pr that referenced this pull request Sep 2, 2026
…wenLM#10752)

Post-merge review follow-ups for the dirty-worktree pull (QwenLM#10390):

- fetchOnly combined with stash or force was silently winning: the flow
  performed a bare fetch and dropped the resolution the caller asked
  for. Both combinations are refused now — a 400
  invalid_fetch_only_combination at the route and a mirrored guard in
  gitPull — matching the existing stash+force exclusivity.
- A stash or force pull on a detached HEAD surfaced git's raw
  "HEAD does not point to a branch" through an unclassified 500; it is a
  typed pull_failed refusal now, before anything is touched.
- validatedUpstream resolves the branch once and reads the configured
  upstream inline, so the no-upstream tail is explicit about the one
  probe race that can reach it instead of looking unreachable.
- The resolution panel no longer offers Discard after the daemon refused
  it with force_unsupported: the refusal is permanent for a subdirectory
  workspace, so re-offering it could only loop.
- Design doc: note that a stored-back stash entry lands on top of the
  stack, and state the plain pull's invariant precisely (same git
  invocation; output now path-redacted like every other response).
pull Bot pushed a commit to Stars1233/qwen-code that referenced this pull request Sep 3, 2026
…wenLM#10754)

* fix(web-shell): disable Push while the branch is behind its upstream

Follow-up to QwenLM#10397's sandboxed verification (run 33195566824), which
measured two gaps left at merge time:

- F1: with an upstream and `behind > 0` — behind-only or diverged — the push
  row stayed enabled, but the exact `git push` the daemon runs is refused
  unconditionally as a non-fast-forward, contradicting the derivation's own
  "disabled means git refuses it" contract. The row is now disabled in those
  states (the verification's measured M6' rule): `detached || (!operation &&
  hasUpstream && behind > 0)`. Mid-operation the row still only warns — the
  behind count is in flux until the operation concludes — and conflicts alone
  still don't block a push.
- F2: nothing pinned `newerStatus`'s equal-`computedAt` tie-break (mutant
  M5 survived), so the popover's own fetch winning ties was unasserted. A
  test now renders the fetched counters when both stamps are equal.

The QwenLM#10390 competing-push panel test gets an ahead-only listing fixture,
since its scenario needs a state where push is actually possible.

Local mutation A/B: reverting the rule to `detached` fails 3 tests;
`>=` → `>` in the tie-break fails exactly the new test.

* fix(web-shell): reason about the push destination, not the upstream

Review round 1 follow-ups:

- The branch listing now carries the push side: `pushTarget` /
  `pushAhead` / `pushBehind` / `pushGone` from git's own `%(push:short)` and
  `%(push:track,nobracket)` atoms (no push-destination precedence is
  re-derived), plus `pushConfigured` (`branch.<name>.pushRemote` or
  `remote.pushDefault` present). Notably, under the default
  `push.default=simple` git refuses to resolve `@{push}` in exactly the
  triangular shape where a plain `git push` succeeds, so the resolved target
  cannot be the only signal.
- The push row's disable and counts now use the push destination: a resolved
  target brings its own ahead/behind; configured-but-unresolvable fails open
  (never disables on upstream counts); a missing push ref (`pushGone`) never
  disables; the plain-clone shape falls back to the upstream, which is where
  a plain `git push` goes.
- The rule-site comment states the counts are as of the last fetch, and all
  `handlePull` error paths now re-fetch the listing — a pull's embedded
  fetch has usually updated the refs, so a failed Update no longer strands
  the rows on a pre-pull snapshot.
- The E2E plan's unreachable "click Push while the panel shows" state is
  rewritten: the reachable check asserts Push is disabled while the
  409 panel is up, and the competing-push race keeps its unit coverage on a
  triangular fixture — a state real git can occupy, unlike
  ahead-only-with-a-409.

Tests: real-git triangular and push-gone fixtures in core (simple →
configured-but-unknown, current → resolved counts); derivation cases for the
triangular, diverged-from-push-target, push-gone, and
configured-but-unresolvable shapes (the last mutation-checked: dropping the
fail-open guard fails exactly that test); a pull-failure listing-refetch
test.

* fix(web-shell): warn instead of disabling when the push counts look doomed

Review round 2 measured the behind-based disable misfiring across
independent config axes — a `remote.<name>.push` refspec (Gerrit), forcing
refspecs, the triangular `push.default=simple` shape, the everyday
`checkout -b hotfix origin/main` name-mismatch clone, and plain last-fetch
staleness. The common thread: whether a remote will accept a push is not
decidable from local state, so every disable built on the counts acquires
another carve-out per config axis. This round makes the structural cut
instead of the next carve-out:

- Push is disabled only on a detached HEAD — the one push failure provable
  locally. Behind or diverged counts render as warning-tone hints on an
  enabled row (`↓3`, `↑1 ↓1 · diverged`), and the click surfaces git's own
  authoritative message. The derivation's contract comment now says exactly
  that.
- The information layer stays push-side and gets honest in the shapes review
  flagged: a resolved push target brings its own counts; a missing push ref
  says "Creates <target>" instead of a dimmed "Nothing to push"; a
  configured-but-unresolvable destination says nothing rather than
  presenting pull-side numbers as push-side ones.
- The `pushConfigured` probe also matches `remote.<name>.push` refspecs, so
  the Gerrit shape reads as configured rather than as a plain clone.
- A rejected push re-reads the listing and the working-tree status (the
  strongest evidence the counts were stale); a failed pull now refreshes the
  status alongside the listing so the hints don't mix snapshots.

Test hygiene from the same review: the core push-side fixtures run under a
hermetic env with `push.default` pinned; new real-git coverage for
`remote.pushDefault`, the refspec probe, per-branch case-preserving
pushRemote scoping, and a nonzero `pushBehind`; the shared popover fixture
is annotated with the wire type (it silently failed to typecheck before);
the triangular race fixture uses distinct upstream/push refs (the same-ref
contradictory counts were impossible for real git); the pull-failure test
asserts the refreshed rows, not just the fetch call; the pushGone test pins
its copy. The E2E plan is rewritten for the warn-only semantics with
`push.default` made explicit where resolution requires it.

* fix(web-shell): key the push row's silence on git naming no destination

Review round 3 measured the push row deciding whether it could speak from
"a push override is configured" rather than from the boundary its own
comment states — git declining to name a destination. Real git shows the
key was wrong in both directions:

- A tracking upstream whose name the branch does not match under the
  default `push.default=simple`, and `push.default=nothing`, both leave
  `%(push)` empty and make a bare `git push` exit 128, yet the row asserted
  the upstream counts for that refused push (`↑1`, `↑2`).
- A branch with no upstream in a repo that sets `remote.pushDefault` lost
  the accurate "Sets upstream on push" hint, for a push the daemon performs
  with an explicit refspec that git accepts.

Keying the silence on a live upstream with no push destination covers both,
and leaves `pushConfigured` with no reader anywhere in the product — so the
atom and the `git config --get-regexp` probe that produced it are removed
rather than extended. That also retires the probe's serial round-trip on
the listing's critical path and the two doc blocks that disagreed about
which overrides it detected.

The post-action refresh gains a single owner. It is best-effort, so a
re-read that fails next to the action that triggered it keeps the stale but
usable rows instead of replacing them with its own error; and the pull path
no longer awaits it, which had left the resolution panel's buttons disabled
for a listing round-trip the panel never needed.

Coverage: real-git core fixtures for all three silence shapes, decision
table cases for both boundary directions and for the gone-upstream in-sync
corner, a push counterpart to the pull-failure refresh test that also pins
the status leg, and cases for the best-effort refresh and the responsive
panel. Fixtures that meant to exercise the count path now carry the
push-side atoms core actually emits. Every guard added here was mutation
probed; the E2E plan gains the two states the re-keyed boundary changes.

* fix(web-shell): keep the stale rows on screen through a post-action refresh

Review round 5 measured the silent refresh added in round 3 defeating the
property it was added for. `fetchBranches(true)` raised the same `loading`
flag the on-open fetch does, and the render gate swaps the whole row set —
action rows and branch sections alike — for the "Loading branches…"
placeholder while that flag is set. So after a rejected push or a failed
Update Project the listing was replaced for the full daemon round-trip,
exactly the window its own comment calls "stale but usable": against the
correlated failure it exists for (a closing daemon generation, with the
SDK's 30s fetch timeout) the user faced a blank list instead of clickable
rows. The push-side `await`'s stated purpose — holding the row spinner up
until the refresh lands — was likewise unobservable, because the row
carrying that spinner was not mounted. Suppressing the toggle for silent
refreshes only keeps the placeholder on the on-open path the gate is fed by.

The same round asked whether the push-failure re-read heals the counts that
got the push rejected. Real git says it cannot: a non-fast-forward rejection
moves no local ref, so the listing and the status read both come back
byte-identical (`[ahead 1]` before and after the rejected push; `[ahead 1,
behind 1]` only once something fetches). Adding a fetch to make it heal was
declined rather than worked around — the design already makes git's
click-time message the authority on remote acceptance, that message reaches
the status line from the same catch, the only fetch timeout in the component
is the pull path's 600s, and a reconciliation effect already re-reads the
listing when a newer status contradicts it. The rule-site comment and the
E2E plan's push bullet now claim a re-read and name git's message as the
authority, so the copy stops promising a healing this path cannot deliver.

Coverage: one post-rejection test holding the second listing promise pending
witnesses both guards — the rows stay mounted with no placeholder while it is
in flight, and the push row stays disabled until it settles. Mutation probed:
dropping `!silent` fails it on the placeholder assertion, flipping the
push-side `await` to `void` fails it on the disabled one, restored control
green.

---------

Co-authored-by: qwen-code-dev-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofix/needs-human The autofix loop stopped on this PR — a human must re-arm, split, merge, or close it autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants