Skip to content

Commit 20fa596

Browse files
authored
Merge pull request #98 from cnjack/fix/git-checkout-safety-and-run-lifecycle
fix(web): harden git checkout, run lifecycle, and branch-picker UX
2 parents e697fb2 + e22e4c6 commit 20fa596

12 files changed

Lines changed: 267 additions & 25 deletions

File tree

internal/web/engine.go

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,12 @@ type Engine struct {
5555
history []adk.Message
5656
running atomic.Bool // per-task busy flag (was the global Server.running gate)
5757
runCancel context.CancelFunc
58+
// runGen is bumped (under emu) each time a run installs its runCancel. A run
59+
// goroutine captures its generation at start and only tears down (clears
60+
// runCancel, releases running, broadcasts idle) if it is still current — so a
61+
// finishing run that has already been superseded by the next turn on the same
62+
// engine does not clobber the new run's cancel and leave the task unstoppable.
63+
runGen uint64
5864

5965
// --- per-task model / mode axis ---
6066
providerName string
@@ -370,10 +376,15 @@ func (s *Server) setActiveEngine(eng *Engine) {
370376
prev := s.Engine
371377
s.Engine = eng
372378
s.mu.Unlock()
373-
if prev != nil && prev != eng && !prev.running.Load() {
379+
if prev != nil && prev != eng {
380+
// Re-check running INSIDE emu, together with the recorder check, rather
381+
// than via an unlocked pre-check: a run starting on prev concurrently
382+
// (running flips true, runCancel set under emu) must not be torn down. The
383+
// folded check only ever makes reclaim more conservative — at worst it
384+
// leaks an idle throwaway engine, never cancels a live run.
374385
reclaim := false
375386
prev.emu.Lock()
376-
if prev.recorder == nil || !prev.recorder.HasRecording() {
387+
if !prev.running.Load() && (prev.recorder == nil || !prev.recorder.HasRecording()) {
377388
reclaim = true
378389
}
379390
prev.emu.Unlock()

internal/web/git.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,17 @@ func (s *Server) handleGitCheckout(w http.ResponseWriter, r *http.Request) {
101101
writeJSON(w, http.StatusBadRequest, map[string]string{"error": "branch is required"})
102102
return
103103
}
104+
// A leading dash would be parsed by git as a flag rather than a ref — e.g.
105+
// branch "-f" turns `git checkout <branch>` into `git checkout -f`, silently
106+
// force-switching and discarding all uncommitted work (and returning 200).
107+
// Reject it outright; valid git refs never begin with "-" (git
108+
// check-ref-format forbids it), so this rejects nothing legitimate. Note a
109+
// "--" separator is NOT a fix here: `git checkout -- <ref>` treats <ref> as a
110+
// pathspec and breaks branch switching.
111+
if strings.HasPrefix(branch, "-") {
112+
writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid branch name"})
113+
return
114+
}
104115

105116
// Force stable English git output so the block detection below doesn't depend
106117
// 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) {
147158
}
148159
// A plain switch aborted by uncommitted work is recoverable: report it as
149160
// such (the working tree is untouched) so the UI can offer stash/discard.
161+
// `kind` tells the UI whether the at-risk files are tracked modifications
162+
// (force = discard edits) or untracked files (force = irrecoverable
163+
// deletion), so it can pick safe recovery options and accurate copy.
150164
if req.Strategy == "" && checkoutBlockedByLocalChanges(msg) {
151165
writeJSON(w, http.StatusOK, map[string]any{
152166
"branch": "",
153167
"blocked": true,
168+
"kind": blockKind(msg),
154169
"message": msg,
155170
"files": parseOverwriteFiles(msg),
156171
})
157172
return
158173
}
174+
// The checkout failed after we stashed the user's work (stash strategy).
175+
// Restore the pre-switch tree so nothing is silently orphaned in the stash.
176+
// After `stash push -u` the tree is clean, so pop normally applies cleanly;
177+
// if it doesn't, name the stash so the user can recover it by hand.
178+
if stashed {
179+
popCmd := exec.CommandContext(r.Context(), "git", "stash", "pop")
180+
popCmd.Dir = dir
181+
popCmd.Env = env
182+
if popOut, popErr := popCmd.CombinedOutput(); popErr != nil {
183+
writeJSON(w, http.StatusConflict, map[string]string{
184+
"error": msg + "\n\nYour changes were stashed but could not be restored " +
185+
"automatically; recover them with `git stash pop` (see `git stash list`): " +
186+
strings.TrimSpace(string(popOut)),
187+
})
188+
return
189+
}
190+
}
159191
writeJSON(w, http.StatusConflict, map[string]string{"error": msg})
160192
return
161193
}
@@ -180,6 +212,20 @@ func checkoutBlockedByLocalChanges(msg string) bool {
180212
strings.Contains(m, "please commit your changes or stash them")
181213
}
182214

215+
// blockKind classifies a blocked checkout so the UI can offer safe recovery
216+
// options and accurate copy. "untracked" means new files would be clobbered
217+
// (force = irrecoverable deletion); "tracked" means committed-file
218+
// modifications (force = discard edits). Both remain recoverable via
219+
// `git stash push -u`. Matched against C-locale git output. The untracked
220+
// check is first and specific so a mixed message is classified as the more
221+
// dangerous case.
222+
func blockKind(msg string) string {
223+
if strings.Contains(strings.ToLower(msg), "untracked working tree files would be overwritten") {
224+
return "untracked"
225+
}
226+
return "tracked"
227+
}
228+
183229
// parseOverwriteFiles pulls the tab-indented paths git lists between the
184230
// "would be overwritten" header and the trailing "Please commit…/Aborting"
185231
// lines, so the UI can show exactly which files are at risk.

internal/web/git_test.go

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
package web
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"net/http"
7+
"net/http/httptest"
8+
"os"
9+
"os/exec"
10+
"path/filepath"
11+
"strings"
12+
"testing"
13+
)
14+
15+
// runGit runs git in dir with an isolated config so host/global settings (default
16+
// branch, signing, hooks) can't perturb the test. Fails the test on git error.
17+
func runGit(t *testing.T, dir string, args ...string) {
18+
t.Helper()
19+
cmd := exec.Command("git", args...)
20+
cmd.Dir = dir
21+
cmd.Env = append(os.Environ(),
22+
"GIT_CONFIG_GLOBAL=/dev/null",
23+
"GIT_CONFIG_SYSTEM=/dev/null",
24+
)
25+
if out, err := cmd.CombinedOutput(); err != nil {
26+
t.Fatalf("git %v: %v\n%s", args, err, out)
27+
}
28+
}
29+
30+
// TestGitCheckoutRejectsDashBranch is the regression guard for the argv-injection
31+
// fix: a branch beginning with "-" (e.g. "-f") must be rejected with 400 BEFORE
32+
// any git runs. Previously it flowed straight into the argv as `git checkout -f`,
33+
// which force-switched and silently discarded all uncommitted work — returning a
34+
// 200 OK that masked the data loss.
35+
func TestGitCheckoutRejectsDashBranch(t *testing.T) {
36+
repo := t.TempDir()
37+
runGit(t, repo, "init", "-q")
38+
runGit(t, repo, "config", "user.email", "t@example.com")
39+
runGit(t, repo, "config", "user.name", "t")
40+
file := filepath.Join(repo, "a.txt")
41+
if err := os.WriteFile(file, []byte("committed\n"), 0o644); err != nil {
42+
t.Fatal(err)
43+
}
44+
runGit(t, repo, "add", "a.txt")
45+
runGit(t, repo, "commit", "-q", "-m", "init")
46+
// Uncommitted change that a stray `git checkout -f` would revert.
47+
const dirty = "DIRTY uncommitted\n"
48+
if err := os.WriteFile(file, []byte(dirty), 0o644); err != nil {
49+
t.Fatal(err)
50+
}
51+
52+
s := &Server{Engine: &Engine{pwd: repo}, ctx: context.Background()}
53+
rec := httptest.NewRecorder()
54+
req := httptest.NewRequest(http.MethodPost, "/api/git/checkout", strings.NewReader(`{"branch":"-f"}`))
55+
s.handleGitCheckout(rec, req)
56+
57+
if rec.Code != http.StatusBadRequest {
58+
t.Fatalf("dash branch: want 400, got %d body=%q", rec.Code, rec.Body.String())
59+
}
60+
// The uncommitted change must survive untouched.
61+
got, err := os.ReadFile(file)
62+
if err != nil {
63+
t.Fatal(err)
64+
}
65+
if string(got) != dirty {
66+
t.Fatalf("uncommitted work was destroyed: got %q want %q", got, dirty)
67+
}
68+
}
69+
70+
// TestBlockKind covers the tracked/untracked classifier used to pick safe
71+
// recovery options in the branch-switch UI.
72+
func TestBlockKind(t *testing.T) {
73+
untracked := "error: The following untracked working tree files would be overwritten by checkout:\n\tfoo.txt\n" +
74+
"Please move or remove them before you switch branches.\nAborting"
75+
tracked := "error: Your local changes to the following files would be overwritten by checkout:\n\tfoo.txt\n" +
76+
"Please commit your changes or stash them before you switch branches.\nAborting"
77+
if got := blockKind(untracked); got != "untracked" {
78+
t.Errorf("untracked message: got %q want %q", got, "untracked")
79+
}
80+
if got := blockKind(tracked); got != "tracked" {
81+
t.Errorf("tracked message: got %q want %q", got, "tracked")
82+
}
83+
if got := blockKind("some unrelated git error"); got != "tracked" {
84+
t.Errorf("default: got %q want %q", got, "tracked")
85+
}
86+
}
87+
88+
// TestValidatePathsMissingDetection guards the workspace missing-detection fix: a
89+
// path is reported missing only when it confirmably does not exist (or is not a
90+
// directory) — never on a transient/permission stat error, which previously hid
91+
// still-valid workspaces from the picker.
92+
func TestValidatePathsMissingDetection(t *testing.T) {
93+
s := &Server{}
94+
base := t.TempDir()
95+
notExist := filepath.Join(base, "nope")
96+
regularFile := filepath.Join(base, "file.txt")
97+
if err := os.WriteFile(regularFile, []byte("x"), 0o644); err != nil {
98+
t.Fatal(err)
99+
}
100+
101+
post := func(paths []string) []string {
102+
t.Helper()
103+
body, _ := json.Marshal(map[string][]string{"paths": paths})
104+
rec := httptest.NewRecorder()
105+
s.handleValidatePaths(rec, httptest.NewRequest(http.MethodPost, "/api/validate-paths", strings.NewReader(string(body))))
106+
if rec.Code != http.StatusOK {
107+
t.Fatalf("code=%d body=%q", rec.Code, rec.Body.String())
108+
}
109+
var resp struct {
110+
Missing []string `json:"missing"`
111+
}
112+
if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil {
113+
t.Fatal(err)
114+
}
115+
return resp.Missing
116+
}
117+
118+
got := post([]string{base, notExist, regularFile})
119+
want := map[string]bool{notExist: true, regularFile: true}
120+
if len(got) != len(want) {
121+
t.Fatalf("missing detection: got %v, want exactly {notExist, regularFile}", got)
122+
}
123+
for _, p := range got {
124+
if !want[p] {
125+
t.Fatalf("unexpected missing path %q (existing dir must not be missing); got %v", p, got)
126+
}
127+
}
128+
129+
// The fix's core: a path under an unsearchable parent yields EACCES (not
130+
// not-exist) and must NOT be reported missing.
131+
if os.Geteuid() == 0 {
132+
t.Skip("permission check is a no-op as root")
133+
}
134+
denied := filepath.Join(base, "denied")
135+
if err := os.Mkdir(denied, 0o000); err != nil {
136+
t.Fatal(err)
137+
}
138+
// Restore perms so t.TempDir cleanup can remove it (runs before TempDir's own
139+
// cleanup, which was registered earlier — LIFO).
140+
t.Cleanup(func() { _ = os.Chmod(denied, 0o755) })
141+
if got := post([]string{filepath.Join(denied, "ws")}); len(got) != 0 {
142+
t.Fatalf("EACCES path must not be reported missing, got %v", got)
143+
}
144+
}

internal/web/server.go

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -696,17 +696,30 @@ func (s *Server) submitMessage(eng *Engine, message, mode, source, sessionID str
696696
// its own cancellable context so /stop cancels only that task.
697697
runCtx, runCancel := context.WithCancel(s.ctx)
698698
eng.emu.Lock()
699+
eng.runGen++
700+
gen := eng.runGen
699701
eng.runCancel = runCancel
700702
eng.emu.Unlock()
701703

702704
go func() {
703705
s.setTaskStatus(eng, true)
704706
defer func() {
705-
eng.running.Store(false)
707+
// Tear down only if this run is still the current one. If a newer turn
708+
// on the same engine has already taken over (runGen advanced) it now
709+
// owns running/runCancel — leave them so /stop still reaches the live
710+
// run and we don't broadcast a spurious idle for it. Releasing running
711+
// inside the same emu section that clears runCancel also closes the
712+
// gate↔cancel interleave window the run-start CAS relies on.
706713
eng.emu.Lock()
707-
eng.runCancel = nil
714+
superseded := eng.runGen != gen
715+
if !superseded {
716+
eng.runCancel = nil
717+
eng.running.Store(false)
718+
}
708719
eng.emu.Unlock()
709-
s.setTaskStatus(eng, false)
720+
if !superseded {
721+
s.setTaskStatus(eng, false)
722+
}
710723
}()
711724

712725
// Take a git snapshot before the agent run for session diff tracking.
@@ -1714,11 +1727,18 @@ func (s *Server) takeSessionSnapshot(eng *Engine) {
17141727

17151728
// handleSessionDiff computes the diff between the session start snapshot and current state.
17161729
func (s *Server) handleSessionDiff(w http.ResponseWriter, _ *http.Request) {
1730+
// Capture the active engine ONCE so the snapshot and the working dir come
1731+
// from the same task's repo even if the active engine is swapped between the
1732+
// two reads (otherwise we could diff engine A's snapshot against engine B's
1733+
// tree). eng.pwd is immutable after creation, so reading it bare is safe.
1734+
eng := s.activeEngine()
17171735
snapshot := ""
1718-
if eng := s.activeEngine(); eng != nil {
1736+
pwd := ""
1737+
if eng != nil {
17191738
eng.emu.Lock()
17201739
snapshot = eng.sessionSnapshot
17211740
eng.emu.Unlock()
1741+
pwd = eng.pwd
17221742
}
17231743

17241744
type diffEntry struct {
@@ -1739,7 +1759,7 @@ func (s *Server) handleSessionDiff(w http.ResponseWriter, _ *http.Request) {
17391759

17401760
// Diff from snapshot to current working tree
17411761
cmd := exec.CommandContext(s.ctx, "git", "diff", snapshot, "--no-color")
1742-
cmd.Dir = s.activePwd()
1762+
cmd.Dir = pwd
17431763
output, _ := cmd.CombinedOutput()
17441764

17451765
var entries []diffEntry
@@ -2295,7 +2315,17 @@ func (s *Server) handleValidatePaths(w http.ResponseWriter, r *http.Request) {
22952315
if p == "" {
22962316
continue
22972317
}
2298-
if info, err := os.Stat(p); err != nil || !info.IsDir() {
2318+
info, err := os.Stat(p)
2319+
if err != nil {
2320+
// Only a confirmed not-exist means the workspace is gone. Transient
2321+
// errors (permission, NFS hiccup) are inconclusive — keep the path
2322+
// rather than silently dropping a still-valid workspace from the picker.
2323+
if os.IsNotExist(err) {
2324+
missing = append(missing, p)
2325+
}
2326+
continue
2327+
}
2328+
if !info.IsDir() {
22992329
missing = append(missing, p)
23002330
}
23012331
}

web/src/components/BranchPicker.vue

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ function reset() {
129129
<button class="bp-confirm-btn discard" :disabled="switching" @click="applyStrategy('force', close)">
130130
{{ t('branches.confirmDiscard') }}
131131
</button>
132-
<button class="bp-confirm-btn cancel" :disabled="switching" @click="cancelPending">
132+
<button class="bp-confirm-btn cancel" :disabled="switching" @click="reset">
133133
{{ t('branches.confirmCancel') }}
134134
</button>
135135
</div>

web/src/components/ProviderIcon.vue

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const svg = computed(() => iconForProvider(props.provider))
1111
1212
// First alphanumeric character, shown when the provider has no brand icon.
1313
const initial = computed(
14-
() => (props.provider || '').replace(/[^a-z0-9]/i, '').charAt(0).toUpperCase() || '?',
14+
() => (props.provider || '').replace(/[^a-z0-9]/gi, '').charAt(0).toUpperCase() || '?',
1515
)
1616
</script>
1717

web/src/components/Sidebar.vue

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,8 +322,19 @@ function isActiveTask(task: TaskItem): boolean {
322322
323323
async function handleDelete(task: TaskItem) {
324324
const path = task.project
325+
// Capture before mutating: deleting the conversation you're currently viewing
326+
// must also reset the chat view, otherwise the timeline stays rendered and
327+
// currentSessionId keeps pointing at the now-dead session (the next message
328+
// would be sent to it). Guarded so deleting a background task never disturbs
329+
// the open chat.
330+
const wasActive = isActiveTask(task)
325331
await store.deleteSession(task.uuid)
326332
await refresh()
333+
if (wasActive) {
334+
store.clearChat()
335+
store.currentSessionId = ''
336+
store.isRunning = false
337+
}
327338
// If that was the workspace's last conversation, drop the now-empty folder from
328339
// the tree too. Archived chats still count (tasksByProject keeps them), so a
329340
// folder with only archived conversations is preserved.

web/src/i18n/locales/en.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -571,13 +571,13 @@ export default {
571571
search: 'Search branches',
572572
newName: 'new-branch-name',
573573
current: 'Branch: {name}',
574-
confirmTitle: 'Uncommitted changes',
575-
confirmIntro: 'Switching to "{branch}" would overwrite your changes to:',
574+
confirmTitle: 'Uncommitted work',
575+
confirmIntro: 'Switching to "{branch}" would overwrite local files at:',
576576
confirmMore: '+{count} more',
577577
confirmStash: 'Stash & switch',
578578
confirmDiscard: 'Discard & switch',
579579
confirmCancel: 'Cancel',
580-
confirmHint: 'Stash saves your changes (recover with git stash pop). Discard deletes them permanently.',
580+
confirmHint: 'Stash saves your work (recover with git stash pop). Discard permanently deletes these files, including untracked ones.',
581581
},
582582

583583
projectSwitcher: {

0 commit comments

Comments
 (0)