Repository navigation
fix(core): validate git pull option combinations and detached HEAD - #10752
Merged
Merged
Conversation
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).
wenshao
requested review from
LaZzyMan,
doudouOUC,
qqqys,
tanzhenxin and
yiliang114
as code owners
September 2, 2026 00:49
Collaborator
Author
|
@qwen-code /review |
Contributor
|
Qwen Code review request accepted. Review is running in workflow run. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
Collaborator
|
E2E verification report (independent reviewer run, head Static review (at head)
Unit tests at head (scratch
Live daemon A/B (tmux, bundled head vs bundled base
After each refusal (both builds): the dirty edit survived and the stash list stayed empty — nothing touched. No Critical findings. CI at head is green on all product lanes (only the |
qqqys
approved these changes
Sep 2, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Approving per the review round: independent verification report posted above (static review at head, unit suites green, live daemon A/B base-vs-head on a real git fixture). No Critical findings. ci-bot approval stands at this exact head and all product CI lanes are green (only the review-pr meta lane pending).
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
3 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
Post-merge review follow-ups for the dirty-worktree git update (#10390), from the post-merge code review:
{ fetchOnly: true, stash: true }(orforce) used to perform a bare fetch and drop the resolution the caller asked for, because thefetchOnlybranch ran first. The route now refuses the combination with400 invalid_fetch_only_combination, andgitPullcarries a mirrored guard, matching the existingstash+forceexclusivity.HEAD does not point to a branchthrough an unclassified 500 (SDK-only surface — the UI already disables the row). It is now409 pull_failed("HEAD is detached; check out a branch from a terminal first"), thrown before anything is touched.validatedUpstreamrestructured. The branch is resolved once and the configured upstream read inline; the no-upstream tail now documents the one probe race that can reach it instead of looking unreachable.force_unsupported. That refusal is permanent for a subdirectory workspace, so re-offering the action could only loop the same refusal; the panel keeps Stash (still viable) and Cancel, with the daemon's explanation.outputis path-redacted like every other response.Why it's needed
The silent
fetchOnlywin is the one Important finding from the post-merge review: the feature's own design refuses silent option drops in favor of typed codes, and the SDK now documents these options publicly, so a future caller can hit the combination. The rest closes the review's minor findings while they are fresh.Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/utils/git-branches.test.ts, 97 passed, 2 new):fetchOnly+stash/forceboth reject with "cannot be combined"; a detached-HEAD stash and force pull both refuse as typedpull_failedmentioning the detached HEAD, with HEAD, the edit, and the (empty) stash untouched.cd packages/cli && npx vitest run src/serve/routes/workspace-git-branches.test.ts, 36 passed, 2 new): both combinations →400 invalid_fetch_only_combination.cd packages/web-shell && npx vitest run client/components/BranchPickerPopover.test.tsx, 42 passed): theforce_unsupportedpanel test now also asserts the Discard action is absent while Stash remains.tsc --noEmitclean in core, cli, and web-shell; eslint clean on all changed files.Evidence (Before & After)
Behavioral surface is API-level plus one panel state reachable only for subdirectory workspaces driven via the SDK; pinned by the component test above rather than screenshots. Before:
POST /workspace/git/pull {"fetchOnly":true,"stash":true}→ 200 with a bare fetch. After:400 {"error":"invalid_fetch_only_combination"}.Tested on
Environment (optional)
Unit tests against real local git repositories, as in #10390.
Risk & Scope
stash/forceonly from the resolution panel).parseDroppedStashSha'sDropped refs/stash@{N} (<sha>)format against Git for Windows — needs a Windows environment; the shim-based concurrency tests stayskipIf(win32).Linked Issues
Follow-up to #10390.
中文说明
这个 PR 做了什么
#10390（脏工作区 git 更新）合并后评审的后续修复，来自合并后代码评审：
{ fetchOnly: true, stash: true }（或force）过去会因fetchOnly分支先行而只做裸 fetch、丢掉调用方要求的处理方式。路由现在以400 invalid_fetch_only_combination拒绝该组合，gitPull内有对称守卫，与既有的stash+force互斥一致。HEAD does not point to a branch透传成未分类 500（仅 SDK 面可达——UI 已禁用该行）。现在在动任何东西之前就以409 pull_failed（"HEAD 已游离，请先在终端检出分支"）拒绝。validatedUpstream重构。 分支只解析一次、内联读取配置的上游；无上游尾部现在明确注明唯一可达它的探测竞态，而不是看起来不可达。force_unsupported后面板不再提供放弃。 该拒绝对子目录工作区是永久的，再次提供只会循环同一拒绝；面板保留仍可用的 Stash 与取消，并显示 daemon 的解释。output与其它响应一样做路径脱敏。为什么需要
fetchOnly静默胜出是合并后评审唯一的 Important 发现：该特性自身的设计就拒绝静默丢弃选项而偏向 typed 错误码，且 SDK 已公开记录这些选项，未来调用方可能踩中该组合。其余改动趁热收掉评审的 Minor 项。评审测试计划
如何验证
cd packages/core && npx vitest run src/utils/git-branches.test.ts，97 通过，新增 2）：fetchOnly+stash/force都以 "cannot be combined" 拒绝；游离 HEAD 上的 stash 与 force pull 都以 typedpull_failed拒绝且消息提及游离 HEAD，HEAD、修改与（空）stash 均未动。cd packages/cli && npx vitest run src/serve/routes/workspace-git-branches.test.ts，36 通过，新增 2）：两种组合 →400 invalid_fetch_only_combination。cd packages/web-shell && npx vitest run client/components/BranchPickerPopover.test.tsx，42 通过）：force_unsupported面板用例现在同时断言放弃按钮消失、Stash 保留。tsc --noEmit干净；改动文件 eslint 干净。前后对比证据
行为面在 API 层，外加一个只有子目录工作区经 SDK 才可达的面板状态；由上述组件测试钉住，未截图。改动前：
POST /workspace/git/pull {"fetchOnly":true,"stash":true}→ 200 只做裸 fetch。改动后：400 {"error":"invalid_fetch_only_combination"}。测试环境
环境（可选）
与 #10390 相同，针对真实本地 git 仓库的单元测试。
风险与范围
stash/force）。parseDroppedStashSha的Dropped refs/stash@{N} (<sha>)消息格式——需要 Windows 环境；垫片并发测试维持skipIf(win32)。关联 Issue
#10390 的后续。