From e22e4c6b4663e1a323007bbf2baa154cc4e80a9f Mon Sep 17 00:00:00 2001 From: jack Date: Tue, 23 Jun 2026 09:16:26 +0800 Subject: [PATCH] fix(web): harden git checkout, run lifecycle, and branch-picker UX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up fixes for issues found reviewing the merged PR #96. Backend changes are additive or narrowing; frontend changes are guard/copy fixes. HIGH - git checkout: reject branch names beginning with "-" before building the argv, so {branch:"-f"} can no longer become `git checkout -f` and silently discard uncommitted work while returning 200. (A "--" separator is NOT a fix — it makes git treat the ref as a pathspec and breaks switching.) - run lifecycle: guard the run-goroutine teardown with a per-run generation (Engine.runGen). A superseded old run's defer no longer nils the new run's runCancel, releases its running flag, or broadcasts a spurious idle — so a fast next turn on the same engine stays stoppable via /stop. MEDIUM - git checkout: when a stash-strategy switch fails after auto-stashing, pop the stash to restore the tree instead of orphaning it; name the stash if pop fails. - git checkout: classify the blocked response as tracked/untracked (new `kind` field) so the UI can pick safe recovery options. - setActiveEngine: fold the reclaim `running` check into the emu-guarded block so a concurrently-starting run is not torn down. - sidebar: reset the chat view (clearChat + blank currentSessionId) only when deleting the active conversation, so the next message can't target a deleted session; background-task deletes are unaffected. LOW - handleSessionDiff: capture the active engine once so snapshot and pwd come from the same repo across a swap. - handleValidatePaths: report a path missing only on confirmed not-exist, not on transient/permission stat errors that hid valid workspaces. - branch-switch copy: no longer claims only "changes"; warns discard deletes untracked files (en/zh-Hans/zh-Hant/ja/ko). - BranchPicker: route the confirm-dialog Cancel through reset() so a stale error no longer leaks into the branch list. - ProviderIcon: add the `g` flag to the initial-fallback regex. Adds regression tests for the checkout dash-reject, blockKind, and validate-paths missing detection. Co-Authored-By: Claude Opus 4.8 --- internal/web/engine.go | 15 ++- internal/web/git.go | 46 +++++++++ internal/web/git_test.go | 144 ++++++++++++++++++++++++++++ internal/web/server.go | 42 ++++++-- web/src/components/BranchPicker.vue | 2 +- web/src/components/ProviderIcon.vue | 2 +- web/src/components/Sidebar.vue | 11 +++ web/src/i18n/locales/en.ts | 6 +- web/src/i18n/locales/ja.ts | 6 +- web/src/i18n/locales/ko.ts | 6 +- web/src/i18n/locales/zh-Hans.ts | 6 +- web/src/i18n/locales/zh-Hant.ts | 6 +- 12 files changed, 267 insertions(+), 25 deletions(-) create mode 100644 internal/web/git_test.go diff --git a/internal/web/engine.go b/internal/web/engine.go index b16fd52..6194663 100644 --- a/internal/web/engine.go +++ b/internal/web/engine.go @@ -55,6 +55,12 @@ type Engine struct { history []adk.Message running atomic.Bool // per-task busy flag (was the global Server.running gate) runCancel context.CancelFunc + // runGen is bumped (under emu) each time a run installs its runCancel. A run + // goroutine captures its generation at start and only tears down (clears + // runCancel, releases running, broadcasts idle) if it is still current — so a + // finishing run that has already been superseded by the next turn on the same + // engine does not clobber the new run's cancel and leave the task unstoppable. + runGen uint64 // --- per-task model / mode axis --- providerName string @@ -370,10 +376,15 @@ func (s *Server) setActiveEngine(eng *Engine) { prev := s.Engine s.Engine = eng s.mu.Unlock() - if prev != nil && prev != eng && !prev.running.Load() { + if prev != nil && prev != eng { + // Re-check running INSIDE emu, together with the recorder check, rather + // than via an unlocked pre-check: a run starting on prev concurrently + // (running flips true, runCancel set under emu) must not be torn down. The + // folded check only ever makes reclaim more conservative — at worst it + // leaks an idle throwaway engine, never cancels a live run. reclaim := false prev.emu.Lock() - if prev.recorder == nil || !prev.recorder.HasRecording() { + if !prev.running.Load() && (prev.recorder == nil || !prev.recorder.HasRecording()) { reclaim = true } prev.emu.Unlock() diff --git a/internal/web/git.go b/internal/web/git.go index 3eb2a75..98942e6 100644 --- a/internal/web/git.go +++ b/internal/web/git.go @@ -101,6 +101,17 @@ func (s *Server) handleGitCheckout(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "branch is required"}) return } + // A leading dash would be parsed by git as a flag rather than a ref — e.g. + // branch "-f" turns `git checkout ` into `git checkout -f`, silently + // force-switching and discarding all uncommitted work (and returning 200). + // Reject it outright; valid git refs never begin with "-" (git + // check-ref-format forbids it), so this rejects nothing legitimate. Note a + // "--" separator is NOT a fix here: `git checkout -- ` treats as a + // pathspec and breaks branch switching. + if strings.HasPrefix(branch, "-") { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid branch name"}) + return + } // Force stable English git output so the block detection below doesn't depend // on the host locale. We present our own UI copy, so this C-locale text is @@ -147,15 +158,36 @@ func (s *Server) handleGitCheckout(w http.ResponseWriter, r *http.Request) { } // A plain switch aborted by uncommitted work is recoverable: report it as // such (the working tree is untouched) so the UI can offer stash/discard. + // `kind` tells the UI whether the at-risk files are tracked modifications + // (force = discard edits) or untracked files (force = irrecoverable + // deletion), so it can pick safe recovery options and accurate copy. if req.Strategy == "" && checkoutBlockedByLocalChanges(msg) { writeJSON(w, http.StatusOK, map[string]any{ "branch": "", "blocked": true, + "kind": blockKind(msg), "message": msg, "files": parseOverwriteFiles(msg), }) return } + // The checkout failed after we stashed the user's work (stash strategy). + // Restore the pre-switch tree so nothing is silently orphaned in the stash. + // After `stash push -u` the tree is clean, so pop normally applies cleanly; + // if it doesn't, name the stash so the user can recover it by hand. + if stashed { + popCmd := exec.CommandContext(r.Context(), "git", "stash", "pop") + popCmd.Dir = dir + popCmd.Env = env + if popOut, popErr := popCmd.CombinedOutput(); popErr != nil { + writeJSON(w, http.StatusConflict, map[string]string{ + "error": msg + "\n\nYour changes were stashed but could not be restored " + + "automatically; recover them with `git stash pop` (see `git stash list`): " + + strings.TrimSpace(string(popOut)), + }) + return + } + } writeJSON(w, http.StatusConflict, map[string]string{"error": msg}) return } @@ -180,6 +212,20 @@ func checkoutBlockedByLocalChanges(msg string) bool { strings.Contains(m, "please commit your changes or stash them") } +// blockKind classifies a blocked checkout so the UI can offer safe recovery +// options and accurate copy. "untracked" means new files would be clobbered +// (force = irrecoverable deletion); "tracked" means committed-file +// modifications (force = discard edits). Both remain recoverable via +// `git stash push -u`. Matched against C-locale git output. The untracked +// check is first and specific so a mixed message is classified as the more +// dangerous case. +func blockKind(msg string) string { + if strings.Contains(strings.ToLower(msg), "untracked working tree files would be overwritten") { + return "untracked" + } + return "tracked" +} + // parseOverwriteFiles pulls the tab-indented paths git lists between the // "would be overwritten" header and the trailing "Please commit…/Aborting" // lines, so the UI can show exactly which files are at risk. diff --git a/internal/web/git_test.go b/internal/web/git_test.go new file mode 100644 index 0000000..dd15146 --- /dev/null +++ b/internal/web/git_test.go @@ -0,0 +1,144 @@ +package web + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// runGit runs git in dir with an isolated config so host/global settings (default +// branch, signing, hooks) can't perturb the test. Fails the test on git error. +func runGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_CONFIG_GLOBAL=/dev/null", + "GIT_CONFIG_SYSTEM=/dev/null", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } +} + +// TestGitCheckoutRejectsDashBranch is the regression guard for the argv-injection +// fix: a branch beginning with "-" (e.g. "-f") must be rejected with 400 BEFORE +// any git runs. Previously it flowed straight into the argv as `git checkout -f`, +// which force-switched and silently discarded all uncommitted work — returning a +// 200 OK that masked the data loss. +func TestGitCheckoutRejectsDashBranch(t *testing.T) { + repo := t.TempDir() + runGit(t, repo, "init", "-q") + runGit(t, repo, "config", "user.email", "t@example.com") + runGit(t, repo, "config", "user.name", "t") + file := filepath.Join(repo, "a.txt") + if err := os.WriteFile(file, []byte("committed\n"), 0o644); err != nil { + t.Fatal(err) + } + runGit(t, repo, "add", "a.txt") + runGit(t, repo, "commit", "-q", "-m", "init") + // Uncommitted change that a stray `git checkout -f` would revert. + const dirty = "DIRTY uncommitted\n" + if err := os.WriteFile(file, []byte(dirty), 0o644); err != nil { + t.Fatal(err) + } + + s := &Server{Engine: &Engine{pwd: repo}, ctx: context.Background()} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/git/checkout", strings.NewReader(`{"branch":"-f"}`)) + s.handleGitCheckout(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("dash branch: want 400, got %d body=%q", rec.Code, rec.Body.String()) + } + // The uncommitted change must survive untouched. + got, err := os.ReadFile(file) + if err != nil { + t.Fatal(err) + } + if string(got) != dirty { + t.Fatalf("uncommitted work was destroyed: got %q want %q", got, dirty) + } +} + +// TestBlockKind covers the tracked/untracked classifier used to pick safe +// recovery options in the branch-switch UI. +func TestBlockKind(t *testing.T) { + untracked := "error: The following untracked working tree files would be overwritten by checkout:\n\tfoo.txt\n" + + "Please move or remove them before you switch branches.\nAborting" + tracked := "error: Your local changes to the following files would be overwritten by checkout:\n\tfoo.txt\n" + + "Please commit your changes or stash them before you switch branches.\nAborting" + if got := blockKind(untracked); got != "untracked" { + t.Errorf("untracked message: got %q want %q", got, "untracked") + } + if got := blockKind(tracked); got != "tracked" { + t.Errorf("tracked message: got %q want %q", got, "tracked") + } + if got := blockKind("some unrelated git error"); got != "tracked" { + t.Errorf("default: got %q want %q", got, "tracked") + } +} + +// TestValidatePathsMissingDetection guards the workspace missing-detection fix: a +// path is reported missing only when it confirmably does not exist (or is not a +// directory) — never on a transient/permission stat error, which previously hid +// still-valid workspaces from the picker. +func TestValidatePathsMissingDetection(t *testing.T) { + s := &Server{} + base := t.TempDir() + notExist := filepath.Join(base, "nope") + regularFile := filepath.Join(base, "file.txt") + if err := os.WriteFile(regularFile, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + + post := func(paths []string) []string { + t.Helper() + body, _ := json.Marshal(map[string][]string{"paths": paths}) + rec := httptest.NewRecorder() + s.handleValidatePaths(rec, httptest.NewRequest(http.MethodPost, "/api/validate-paths", strings.NewReader(string(body)))) + if rec.Code != http.StatusOK { + t.Fatalf("code=%d body=%q", rec.Code, rec.Body.String()) + } + var resp struct { + Missing []string `json:"missing"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatal(err) + } + return resp.Missing + } + + got := post([]string{base, notExist, regularFile}) + want := map[string]bool{notExist: true, regularFile: true} + if len(got) != len(want) { + t.Fatalf("missing detection: got %v, want exactly {notExist, regularFile}", got) + } + for _, p := range got { + if !want[p] { + t.Fatalf("unexpected missing path %q (existing dir must not be missing); got %v", p, got) + } + } + + // The fix's core: a path under an unsearchable parent yields EACCES (not + // not-exist) and must NOT be reported missing. + if os.Geteuid() == 0 { + t.Skip("permission check is a no-op as root") + } + denied := filepath.Join(base, "denied") + if err := os.Mkdir(denied, 0o000); err != nil { + t.Fatal(err) + } + // Restore perms so t.TempDir cleanup can remove it (runs before TempDir's own + // cleanup, which was registered earlier — LIFO). + t.Cleanup(func() { _ = os.Chmod(denied, 0o755) }) + if got := post([]string{filepath.Join(denied, "ws")}); len(got) != 0 { + t.Fatalf("EACCES path must not be reported missing, got %v", got) + } +} diff --git a/internal/web/server.go b/internal/web/server.go index 8543c77..96a7b49 100644 --- a/internal/web/server.go +++ b/internal/web/server.go @@ -696,17 +696,30 @@ func (s *Server) submitMessage(eng *Engine, message, mode, source, sessionID str // its own cancellable context so /stop cancels only that task. runCtx, runCancel := context.WithCancel(s.ctx) eng.emu.Lock() + eng.runGen++ + gen := eng.runGen eng.runCancel = runCancel eng.emu.Unlock() go func() { s.setTaskStatus(eng, true) defer func() { - eng.running.Store(false) + // Tear down only if this run is still the current one. If a newer turn + // on the same engine has already taken over (runGen advanced) it now + // owns running/runCancel — leave them so /stop still reaches the live + // run and we don't broadcast a spurious idle for it. Releasing running + // inside the same emu section that clears runCancel also closes the + // gate↔cancel interleave window the run-start CAS relies on. eng.emu.Lock() - eng.runCancel = nil + superseded := eng.runGen != gen + if !superseded { + eng.runCancel = nil + eng.running.Store(false) + } eng.emu.Unlock() - s.setTaskStatus(eng, false) + if !superseded { + s.setTaskStatus(eng, false) + } }() // Take a git snapshot before the agent run for session diff tracking. @@ -1714,11 +1727,18 @@ func (s *Server) takeSessionSnapshot(eng *Engine) { // handleSessionDiff computes the diff between the session start snapshot and current state. func (s *Server) handleSessionDiff(w http.ResponseWriter, _ *http.Request) { + // Capture the active engine ONCE so the snapshot and the working dir come + // from the same task's repo even if the active engine is swapped between the + // two reads (otherwise we could diff engine A's snapshot against engine B's + // tree). eng.pwd is immutable after creation, so reading it bare is safe. + eng := s.activeEngine() snapshot := "" - if eng := s.activeEngine(); eng != nil { + pwd := "" + if eng != nil { eng.emu.Lock() snapshot = eng.sessionSnapshot eng.emu.Unlock() + pwd = eng.pwd } type diffEntry struct { @@ -1739,7 +1759,7 @@ func (s *Server) handleSessionDiff(w http.ResponseWriter, _ *http.Request) { // Diff from snapshot to current working tree cmd := exec.CommandContext(s.ctx, "git", "diff", snapshot, "--no-color") - cmd.Dir = s.activePwd() + cmd.Dir = pwd output, _ := cmd.CombinedOutput() var entries []diffEntry @@ -2295,7 +2315,17 @@ func (s *Server) handleValidatePaths(w http.ResponseWriter, r *http.Request) { if p == "" { continue } - if info, err := os.Stat(p); err != nil || !info.IsDir() { + info, err := os.Stat(p) + if err != nil { + // Only a confirmed not-exist means the workspace is gone. Transient + // errors (permission, NFS hiccup) are inconclusive — keep the path + // rather than silently dropping a still-valid workspace from the picker. + if os.IsNotExist(err) { + missing = append(missing, p) + } + continue + } + if !info.IsDir() { missing = append(missing, p) } } diff --git a/web/src/components/BranchPicker.vue b/web/src/components/BranchPicker.vue index 3e5253a..ca40e8b 100644 --- a/web/src/components/BranchPicker.vue +++ b/web/src/components/BranchPicker.vue @@ -129,7 +129,7 @@ function reset() { - diff --git a/web/src/components/ProviderIcon.vue b/web/src/components/ProviderIcon.vue index 67fdaea..ba9847b 100644 --- a/web/src/components/ProviderIcon.vue +++ b/web/src/components/ProviderIcon.vue @@ -11,7 +11,7 @@ const svg = computed(() => iconForProvider(props.provider)) // First alphanumeric character, shown when the provider has no brand icon. const initial = computed( - () => (props.provider || '').replace(/[^a-z0-9]/i, '').charAt(0).toUpperCase() || '?', + () => (props.provider || '').replace(/[^a-z0-9]/gi, '').charAt(0).toUpperCase() || '?', ) diff --git a/web/src/components/Sidebar.vue b/web/src/components/Sidebar.vue index e9b9335..e807f2f 100644 --- a/web/src/components/Sidebar.vue +++ b/web/src/components/Sidebar.vue @@ -322,8 +322,19 @@ function isActiveTask(task: TaskItem): boolean { async function handleDelete(task: TaskItem) { const path = task.project + // Capture before mutating: deleting the conversation you're currently viewing + // must also reset the chat view, otherwise the timeline stays rendered and + // currentSessionId keeps pointing at the now-dead session (the next message + // would be sent to it). Guarded so deleting a background task never disturbs + // the open chat. + const wasActive = isActiveTask(task) await store.deleteSession(task.uuid) await refresh() + if (wasActive) { + store.clearChat() + store.currentSessionId = '' + store.isRunning = false + } // If that was the workspace's last conversation, drop the now-empty folder from // the tree too. Archived chats still count (tasksByProject keeps them), so a // folder with only archived conversations is preserved. diff --git a/web/src/i18n/locales/en.ts b/web/src/i18n/locales/en.ts index de92a1b..659d394 100644 --- a/web/src/i18n/locales/en.ts +++ b/web/src/i18n/locales/en.ts @@ -571,13 +571,13 @@ export default { search: 'Search branches', newName: 'new-branch-name', current: 'Branch: {name}', - confirmTitle: 'Uncommitted changes', - confirmIntro: 'Switching to "{branch}" would overwrite your changes to:', + confirmTitle: 'Uncommitted work', + confirmIntro: 'Switching to "{branch}" would overwrite local files at:', confirmMore: '+{count} more', confirmStash: 'Stash & switch', confirmDiscard: 'Discard & switch', confirmCancel: 'Cancel', - confirmHint: 'Stash saves your changes (recover with git stash pop). Discard deletes them permanently.', + confirmHint: 'Stash saves your work (recover with git stash pop). Discard permanently deletes these files, including untracked ones.', }, projectSwitcher: { diff --git a/web/src/i18n/locales/ja.ts b/web/src/i18n/locales/ja.ts index 9f1a74d..fd7bebd 100644 --- a/web/src/i18n/locales/ja.ts +++ b/web/src/i18n/locales/ja.ts @@ -551,13 +551,13 @@ export default { search: 'ブランチを検索', newName: '新しいブランチ名', current: 'ブランチ：{name}', - confirmTitle: '未コミットの変更があります', - confirmIntro: '「{branch}」に切り替えると、次のファイルの変更が上書きされます：', + confirmTitle: '未コミットの作業があります', + confirmIntro: '「{branch}」に切り替えると、次のローカルファイルが上書きされます：', confirmMore: '他 {count} 件', confirmStash: 'スタッシュして切り替え', confirmDiscard: '破棄して切り替え', confirmCancel: 'キャンセル', - confirmHint: 'スタッシュは変更を保存します（git stash pop で復元可能）。破棄すると変更は完全に削除されます。', + confirmHint: 'スタッシュは作業を保存します（git stash pop で復元可能）。破棄するとこれらのファイルは完全に削除されます（追跡対象外のファイルを含む）。', }, projectSwitcher: { diff --git a/web/src/i18n/locales/ko.ts b/web/src/i18n/locales/ko.ts index 07cad88..9410821 100644 --- a/web/src/i18n/locales/ko.ts +++ b/web/src/i18n/locales/ko.ts @@ -551,13 +551,13 @@ export default { search: '브랜치 검색', newName: '새-브랜치-이름', current: '브랜치: {name}', - confirmTitle: '커밋되지 않은 변경 사항', - confirmIntro: '"{branch}"(으)로 전환하면 다음 파일의 변경 사항을 덮어씁니다:', + confirmTitle: '커밋되지 않은 작업', + confirmIntro: '"{branch}"(으)로 전환하면 다음 로컬 파일을 덮어씁니다:', confirmMore: '외 {count}개', confirmStash: '스태시하고 전환', confirmDiscard: '버리고 전환', confirmCancel: '취소', - confirmHint: '스태시는 변경 사항을 저장합니다(git stash pop 으로 복구). 버리면 변경 사항이 영구 삭제됩니다.', + confirmHint: '스태시는 작업을 저장합니다(git stash pop 으로 복구). 버리면 이 파일들이 영구 삭제됩니다(추적되지 않는 파일 포함).', }, projectSwitcher: { diff --git a/web/src/i18n/locales/zh-Hans.ts b/web/src/i18n/locales/zh-Hans.ts index 3bb4998..cc49de3 100644 --- a/web/src/i18n/locales/zh-Hans.ts +++ b/web/src/i18n/locales/zh-Hans.ts @@ -551,13 +551,13 @@ export default { search: '搜索分支', newName: '新分支名', current: '分支：{name}', - confirmTitle: '有未提交的修改', - confirmIntro: '切换到“{branch}”会覆盖你对以下文件的修改：', + confirmTitle: '有未提交的改动', + confirmIntro: '切换到“{branch}”会覆盖以下本地文件：', confirmMore: '其余 {count} 项', confirmStash: '暂存并切换', confirmDiscard: '丢弃并切换', confirmCancel: '取消', - confirmHint: '暂存会保存你的修改（可用 git stash pop 恢复）；丢弃将永久删除这些修改。', + confirmHint: '暂存会保存你的改动（可用 git stash pop 恢复）；丢弃将永久删除这些文件，包括未跟踪的文件。', }, projectSwitcher: { diff --git a/web/src/i18n/locales/zh-Hant.ts b/web/src/i18n/locales/zh-Hant.ts index 628bf1d..7f137e0 100644 --- a/web/src/i18n/locales/zh-Hant.ts +++ b/web/src/i18n/locales/zh-Hant.ts @@ -552,13 +552,13 @@ export default { search: '搜尋分支', newName: '新分支名稱', current: '分支：{name}', - confirmTitle: '有未提交的修改', - confirmIntro: '切換到「{branch}」會覆蓋你對以下檔案的修改：', + confirmTitle: '有未提交的變更', + confirmIntro: '切換到「{branch}」會覆蓋以下本機檔案：', confirmMore: '其餘 {count} 項', confirmStash: '暫存並切換', confirmDiscard: '丟棄並切換', confirmCancel: '取消', - confirmHint: '暫存會保存你的修改（可用 git stash pop 還原）；丟棄將永久刪除這些修改。', + confirmHint: '暫存會保存你的變更（可用 git stash pop 還原）；丟棄將永久刪除這些檔案，包括未追蹤的檔案。', }, projectSwitcher: {