Repository navigation
feat(web-shell): unblock git update on dirty working tree #9769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
4bbd6d7
ee9b1f2
71a3245
5e03ec2
c6546fd
a7aff45
82cd1ea
8eb70af
280fa7d
4591a65
4998eb1
302803f
5ab98db
de03ee0
a6f32be
39737cb
3a47a27
b067bcd
29342ad
9b25ae4
db0d9a2
0863e06
7dc521e
198e3c4
7688cc3
690817a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| # Git pull with a dirty working tree | ||
|
|
||
| ## Goal | ||
|
|
||
| Let the Web Shell "Update Project" action succeed when the workspace has | ||
| uncommitted changes, instead of dead-ending at an opaque | ||
| `dirty_working_tree` error that forces users back to a terminal. | ||
|
|
||
| ## Problem | ||
|
|
||
| The workspace git pull path (branch picker popover → SDK | ||
| `workspaceGitPull` → `POST /workspace(s)/git/pull` → core `gitPull`) ran a | ||
| plain `git pull`. With local modifications to files the incoming merge | ||
| touches, git refuses to merge; the route already classified that failure as | ||
| `409 dirty_working_tree` with a path-redacted git message, but the popover | ||
| only rendered the SDK's raw error string, so nothing actionable surfaced. | ||
|
|
||
| ## Behavior | ||
|
|
||
| `POST /workspace/git/pull` and `POST /workspaces/:workspace/git/pull` | ||
| accept two new boolean options (both default false, mutually exclusive — | ||
| sending both is a 400): | ||
|
|
||
| - `stash`: stash local changes (`git stash push --include-untracked`, so | ||
| untracked files that could block the merge are covered), run the pull, | ||
| then `git stash pop`. If the pull fails, any partial merge/rebase is | ||
| aborted and the stash is popped back, restoring the pre-pull state. If | ||
| the restore conflicts after a successful pull, the response is still a | ||
| success with `stashRestoreConflict: true` and its `output` carries git's | ||
| conflict notice; git keeps the stash entry, so nothing is lost. | ||
| - `force`: discard all local changes first (`git reset --hard` + | ||
| `git clean -fd`; ignored files are kept), then pull. Destructive. | ||
| Both commands act on the whole repository, so `force` is refused when | ||
| the workspace cwd is below the repository root (it could destroy | ||
| tracked changes outside the workspace while leaving the files that | ||
| block the merge in place). The pull is validated before anything is | ||
| discarded: the fetch runs first and a diverged branch (local commits | ||
| on both sides) is refused, because the post-discard merge could wedge | ||
| the repository mid-merge; a failed post-discard pull still aborts any | ||
| partial merge/rebase. | ||
|
|
||
| Stash detection compares `refs/stash` before/after the push instead of | ||
| parsing git output, which varies by version and locale. The pull itself | ||
| passes `--no-rebase --no-edit` when rebase is not requested, so divergent | ||
| branches merge instead of fataling on git builds without a `pull.rebase` | ||
| / `pull.ff` policy. | ||
|
|
||
| ## UI | ||
|
|
||
| When the plain pull fails with `409 dirty_working_tree`, the branch picker | ||
| footer switches from the status line to a resolution panel offering: | ||
|
|
||
| - **Stash Changes and Update** — pulls with `stash: true`. | ||
| - **Discard Changes and Update…** — destructive, so it takes a second | ||
| click: the panel swaps to a warning plus a confirm button that pulls with | ||
| `force: true`. | ||
| - **Cancel** — dismisses the panel. | ||
|
|
||
| The panel stays mounted (with a spinner on the clicked button) while its | ||
| own stash/discard pull is in flight, and resets whenever the popover is | ||
| reopened. When the blocking state is an unmerged working tree (conflicting | ||
| restore or abandoned merge), the stash option is replaced by an | ||
| explanation — `git stash push` refuses unmerged entries — leaving discard | ||
| as the recovery path. | ||
|
|
||
| ## Ownership | ||
|
|
||
| Both routes keep their existing scoping: the legacy route is | ||
| legacy-primary-workspace scoped; the qualified route resolves the trusted | ||
| runtime and contained `cwd` exactly as before. The new options only change | ||
| which git commands run inside the resolved workspace and add no new trust | ||
| surface. | ||
|
|
||
| ## Alternatives considered | ||
|
|
||
| Automatic stash on every dirty pull was rejected: silently moving the | ||
| user's changes through a stash is surprising, and the discard option is | ||
| destructive, so both need an explicit choice. `git pull --autostash` was | ||
| not usable because autostash only exists for rebase, while the default | ||
| pull here is a merge. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,6 +80,33 @@ describe('workspace Git branch routes', () => { | |
| expect(response.body.error).toBe('invalid_rebase'); | ||
| }); | ||
|
|
||
| it('rejects a wrong-typed stash with 400', async () => { | ||
| const response = await request(app()) | ||
| .post('/workspace/git/pull') | ||
| .send({ stash: 'yes' }); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| expect(response.body.error).toBe('invalid_stash'); | ||
| }); | ||
|
|
||
| it('rejects a wrong-typed force with 400', async () => { | ||
| const response = await request(app()) | ||
| .post('/workspace/git/pull') | ||
| .send({ force: 'yes' }); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| expect(response.body.error).toBe('invalid_force'); | ||
| }); | ||
|
|
||
| it('rejects combining stash and force with 400', async () => { | ||
| const response = await request(app()) | ||
| .post('/workspace/git/pull') | ||
| .send({ stash: true, force: true }); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| expect(response.body.error).toBe('invalid_stash_force'); | ||
| }); | ||
|
|
||
| it('rejects a checkout with a missing ref with 400', async () => { | ||
| const response = await request(app()) | ||
| .post('/workspace/git/checkout') | ||
|
|
@@ -187,6 +214,47 @@ function appWithWorkspace(cwd: string) { | |
| return app; | ||
| } | ||
|
|
||
| // Repo with a remote commit that modified a.txt, plus an uncommitted local | ||
| // edit to the same file — a plain pull fails with a dirty working tree. The | ||
| // edits land in separate hunks so a stash/pull/pop round trip merges cleanly. | ||
| function makeDirtyPullRepo(): string { | ||
| const dir = makeRepo(); | ||
| const lines = Array.from({ length: 10 }, (_, i) => `line ${i + 1}`); | ||
| fs.writeFileSync(path.join(dir, 'a.txt'), `${lines.join('\n')}\n`); | ||
| git(dir, 'add', '.'); | ||
| git(dir, 'commit', '-q', '-m', 'extend a.txt'); | ||
|
|
||
| const remote = fs.realpathSync( | ||
| fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-gitbranch-remote-')), | ||
| ); | ||
| tmpRoots.push(remote); | ||
| git(remote, 'init', '-q', '--bare'); | ||
| git(dir, 'remote', 'add', 'origin', remote); | ||
| git(dir, 'push', '-q', '-u', 'origin', 'HEAD'); | ||
|
|
||
| const clone = fs.realpathSync( | ||
| fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-gitbranch-clone-')), | ||
| ); | ||
| tmpRoots.push(clone); | ||
| git(clone, 'clone', '-q', remote, '.'); | ||
| git(clone, 'config', 'user.email', 'other@example.com'); | ||
| git(clone, 'config', 'user.name', 'Other'); | ||
| git(clone, 'config', 'commit.gpgsign', 'false'); | ||
| const remoteContent = fs | ||
| .readFileSync(path.join(clone, 'a.txt'), 'utf8') | ||
| .replace('line 10', 'line 10 remote'); | ||
| fs.writeFileSync(path.join(clone, 'a.txt'), remoteContent); | ||
| git(clone, 'add', '.'); | ||
| git(clone, 'commit', '-q', '-m', 'remote edit'); | ||
| git(clone, 'push', '-q', 'origin', 'HEAD'); | ||
|
|
||
| const localContent = fs | ||
| .readFileSync(path.join(dir, 'a.txt'), 'utf8') | ||
| .replace('line 1\n', 'line 1 local\n'); | ||
| fs.writeFileSync(path.join(dir, 'a.txt'), localContent); | ||
| return dir; | ||
| } | ||
|
|
||
| afterEach(() => { | ||
| for (const dir of tmpRoots.splice(0)) { | ||
| fs.rmSync(dir, { recursive: true, force: true }); | ||
|
|
@@ -245,6 +313,146 @@ describe('workspace Git branch routes against a real repo (R10 #2)', () => { | |
| expect(response.body.error).toBe('no_upstream'); | ||
| }); | ||
|
|
||
| it('classifies a plain pull on a dirty tree as dirty_working_tree', async () => { | ||
| const dir = makeDirtyPullRepo(); | ||
|
|
||
| const response = await request(appWithWorkspace(dir)) | ||
| .post('/workspace/git/pull') | ||
| .send({}); | ||
|
|
||
| expect(response.status).toBe(409); | ||
| expect(response.body.error).toBe('dirty_working_tree'); | ||
| const body = JSON.stringify(response.body); | ||
| expect(body).not.toContain(dir); | ||
| }); | ||
|
|
||
| it('updates a dirty tree and restores the local changes with stash', async () => { | ||
| const dir = makeDirtyPullRepo(); | ||
|
|
||
| const response = await request(appWithWorkspace(dir)) | ||
| .post('/workspace/git/pull') | ||
| .send({ stash: true }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(response.body.success).toBe(true); | ||
| const expected = [ | ||
| 'line 1 local', | ||
| ...Array.from({ length: 8 }, (_, i) => `line ${i + 2}`), | ||
| 'line 10 remote', | ||
| ]; | ||
| expect(fs.readFileSync(path.join(dir, 'a.txt'), 'utf8')).toBe( | ||
| `${expected.join('\n')}\n`, | ||
| ); | ||
| expect(git(dir, 'stash', 'list').trim()).toBe(''); | ||
| }); | ||
|
|
||
| it('discards the local changes and updates with force', async () => { | ||
| const dir = makeDirtyPullRepo(); | ||
|
|
||
| const response = await request(appWithWorkspace(dir)) | ||
| .post('/workspace/git/pull') | ||
| .send({ force: true }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(response.body.success).toBe(true); | ||
| const expected = [ | ||
| ...Array.from({ length: 9 }, (_, i) => `line ${i + 1}`), | ||
| 'line 10 remote', | ||
| ]; | ||
| expect(fs.readFileSync(path.join(dir, 'a.txt'), 'utf8')).toBe( | ||
| `${expected.join('\n')}\n`, | ||
| ); | ||
| }); | ||
|
|
||
| it('reports stashRestoreConflict when the stash restore conflicts', async () => { | ||
| const dir = makeDirtyPullRepo(); | ||
| // Also edit the same line the remote changed, so the stash pop | ||
| // conflicts after the pull succeeds. | ||
| const localContent = fs | ||
| .readFileSync(path.join(dir, 'a.txt'), 'utf8') | ||
| .replace('line 10', 'line 10 local'); | ||
| fs.writeFileSync(path.join(dir, 'a.txt'), localContent); | ||
|
|
||
| const response = await request(appWithWorkspace(dir)) | ||
| .post('/workspace/git/pull') | ||
| .send({ stash: true }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(response.body.success).toBe(true); | ||
| expect(response.body.stashRestoreConflict).toBe(true); | ||
| }); | ||
|
|
||
| it('classifies a pull blocked by unmerged files as dirty_working_tree', async () => { | ||
| const dir = makeRepo(); | ||
| const remote = fs.realpathSync( | ||
| fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-gitbranch-remote-')), | ||
| ); | ||
| tmpRoots.push(remote); | ||
| git(remote, 'init', '-q', '--bare'); | ||
| git(dir, 'remote', 'add', 'origin', remote); | ||
| git(dir, 'push', '-q', '-u', 'origin', 'HEAD'); | ||
|
|
||
| const clone = fs.realpathSync( | ||
| fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-gitbranch-clone-')), | ||
| ); | ||
| tmpRoots.push(clone); | ||
| git(clone, 'clone', '-q', remote, '.'); | ||
| git(clone, 'config', 'user.email', 'other@example.com'); | ||
| git(clone, 'config', 'user.name', 'Other'); | ||
| git(clone, 'config', 'commit.gpgsign', 'false'); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deferred to the next round. The six pasted 'bare remote + clone + identity + push' sequences in the route test file are real at HEAD; file-local 中文说明延后到下一轮。路由测试文件中六处粘贴的『裸远端 + 克隆 + 身份配置 + 推送』序列在 HEAD 上确实存在;镜像 core 文件新增文件级
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deferred to the next round: this round implemented the review rounds 7-8 Critical batch (collision-probe restructure to enumeration, cherry-pick/revert foreign-state guard, fail-closed stash probe, merge/rebase of the captured tip, stash-restore note kept outside the message cap, and the serialization-test gate) under the per-round batch cap and the time budget this workflow flagged. This finding stays queued for the next round rather than being dropped. R5-12 fixture-helper extraction stays queued. 顺延至下一轮:本轮在单轮批次上限与工作流时间预算内实现了第 7/8 轮评审的 Critical 批次(将冲突探测重构为枚举比对、cherry-pick/revert 外部状态守卫、暂存探测失败即拒绝、合并/变基到已捕获的上游 tip、让暂存恢复提示在消息截断时保留、并发串行化测试的环境门槛)。该发现继续排队等待下一轮处理,不会被丢弃。
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Declined. The repeated bare-remote + clone blocks that remain do so because each shapes the fixture differently (divergent commits, unmerged index, cap-sized file counts); the shared shapes already live in 中文说明拒绝。仍然保留的"裸远端 + 克隆"重复块各自需要不同的夹具形态(分叉提交、未合并索引、超大文件数量);共同的形态已经收敛进 |
||
| fs.writeFileSync(path.join(clone, 'a.txt'), 'remote change\n'); | ||
| git(clone, 'add', '.'); | ||
| git(clone, 'commit', '-q', '-m', 'remote edit'); | ||
| git(clone, 'push', '-q', 'origin', 'HEAD'); | ||
|
|
||
| // Divergent local commit touching the same file, then a failed merge: | ||
| // the index now carries unmerged entries. | ||
| fs.writeFileSync(path.join(dir, 'a.txt'), 'local change\n'); | ||
| git(dir, 'add', '.'); | ||
| git(dir, 'commit', '-q', '-m', 'local edit'); | ||
| git(dir, 'fetch', '-q', 'origin'); | ||
| let mergeFailed = false; | ||
| try { | ||
| git( | ||
| dir, | ||
| 'merge', | ||
| 'origin/' + git(dir, 'symbolic-ref', '--short', 'HEAD').trim(), | ||
| ); | ||
| } catch { | ||
| mergeFailed = true; | ||
| } | ||
| expect(mergeFailed).toBe(true); | ||
|
|
||
| const response = await request(appWithWorkspace(dir)) | ||
| .post('/workspace/git/pull') | ||
| .send({}); | ||
|
|
||
| expect(response.status).toBe(409); | ||
| expect(response.body.error).toBe('dirty_working_tree'); | ||
| const body = JSON.stringify(response.body); | ||
| expect(body).not.toContain(dir); | ||
| }); | ||
|
|
||
| it('rejects force pull from a subdirectory workspace without discarding', async () => { | ||
| const dir = makeDirtyPullRepo(); | ||
| const sub = path.join(dir, 'packages', 'app'); | ||
| fs.mkdirSync(sub, { recursive: true }); | ||
| const before = fs.readFileSync(path.join(dir, 'a.txt'), 'utf8'); | ||
|
|
||
| const response = await request(appWithWorkspace(sub)) | ||
| .post('/workspace/git/pull') | ||
| .send({ force: true }); | ||
|
|
||
| expect(response.status).toBe(500); | ||
| expect(response.body.error ?? response.body.message).toContain( | ||
| 'subdirectory', | ||
| ); | ||
| const body = JSON.stringify(response.body); | ||
| expect(body).not.toContain(dir); | ||
| // Nothing was discarded. | ||
| expect(fs.readFileSync(path.join(dir, 'a.txt'), 'utf8')).toBe(before); | ||
| }); | ||
|
|
||
| it('does not misclassify a non-dirty error when the workspace path contains "dirty"', async () => { | ||
| const parent = fs.realpathSync( | ||
| fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-dirty-utils-')), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,7 +82,15 @@ function sendGitError( | |
| res.status(404).json({ error: 'not_a_git_repository', message }); | ||
| return; | ||
| } | ||
| if (/dirty|uncommitted|would be overwritten/i.test(message)) { | ||
| // `unmerged files` / `have not concluded your merge` cover the states a | ||
| // conflicting stash restore or an abandoned merge leaves behind: pull and | ||
| // stash both refuse until the conflicts are resolved, so the resolution | ||
| // panel (whose discard path clears the state) must reappear for them. | ||
| if ( | ||
| /dirty|uncommitted|would be overwritten|unmerged files|have not concluded your merge/i.test( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stopgap implemented this round; structural closure deferred to the follow-up queue. The route classifier now covers the four demonstrated entrances — 中文说明本轮已实现权宜方案;结构性闭合记入延后跟进队列。路由分类器现已覆盖四个实证入口 —— |
||
| message, | ||
| ) | ||
| ) { | ||
| res.status(409).json({ error: 'dirty_working_tree', message }); | ||
| return; | ||
| } | ||
|
|
@@ -270,10 +278,31 @@ async function handlePull( | |
| }); | ||
| return; | ||
| } | ||
| if (body['stash'] !== undefined && typeof body['stash'] !== 'boolean') { | ||
| res | ||
| .status(400) | ||
| .json({ error: 'invalid_stash', message: 'stash must be a boolean' }); | ||
| return; | ||
| } | ||
| if (body['force'] !== undefined && typeof body['force'] !== 'boolean') { | ||
| res | ||
| .status(400) | ||
| .json({ error: 'invalid_force', message: 'force must be a boolean' }); | ||
| return; | ||
| } | ||
| const rebase = body['rebase'] === true; | ||
| const fetchOnly = body['fetchOnly'] === true; | ||
| const stash = body['stash'] === true; | ||
| const force = body['force'] === true; | ||
| if (stash && force) { | ||
| res.status(400).json({ | ||
| error: 'invalid_stash_force', | ||
| message: 'stash and force are mutually exclusive', | ||
| }); | ||
| return; | ||
| } | ||
| try { | ||
| const result = await gitPull(cwd, { rebase, fetchOnly }, env); | ||
| const result = await gitPull(cwd, { rebase, fetchOnly, stash, force }, env); | ||
| res.status(200).json(result); | ||
| } catch (err) { | ||
| sendGitError(res, err, route, sendBridgeError, cwd); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deferred as a test change, but the hazard this round's Critical fix already neutralizes: R4-19 landed
--no-autostashon both update commands (git merge --no-edit --no-autostash @{u}/git rebase --no-autostash @{u}). Probe-verified on this host: with repo-localmerge.autostash=true/rebase.autostash=true(the same channel as system config, higher precedence), both commands still refuse the dirty tree, so this test can no longer receive 200 instead of 409 from ambient autostash. The suggestedmakeRepo()pin remains available as belt-and-braces next round.作为测试改动延后,但其危害已被本轮的 Critical 修复中和:R4-19 已给两个更新命令加上
--no-autostash(git merge --no-edit --no-autostash @{u}/git rebase --no-autostash @{u})。本机探针验证:仓库本地merge.autostash=true/rebase.autostash=true(与系统配置同通道、更高优先级)时,两个命令仍拒绝脏树,本测试不会再因环境 autostash 得到 200 而非 409。建议的makeRepo()钉住仍可作为下一轮的额外保险。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deferred to the next round: this round implemented the review rounds 7-8 Critical batch (collision-probe restructure to enumeration, cherry-pick/revert foreign-state guard, fail-closed stash probe, merge/rebase of the captured tip, stash-restore note kept outside the message cap, and the serialization-test gate) under the per-round batch cap and the time budget this workflow flagged. This finding stays queued for the next round rather than being dropped. R4-13 fixture-duplication pattern stays queued (location 1 of 3).
顺延至下一轮:本轮在单轮批次上限与工作流时间预算内实现了第 7/8 轮评审的 Critical 批次(将冲突探测重构为枚举比对、cherry-pick/revert 外部状态守卫、暂存探测失败即拒绝、合并/变基到已捕获的上游 tip、让暂存恢复提示在消息截断时保留、并发串行化测试的环境门槛)。该发现继续排队等待下一轮处理,不会被丢弃。