Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions internal/web/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
46 changes: 46 additions & 0 deletions internal/web/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 <branch>` 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 -- <ref>` treats <ref> 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
Expand Down Expand Up @@ -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
}
Expand All @@ -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.
Expand Down
144 changes: 144 additions & 0 deletions internal/web/git_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
42 changes: 36 additions & 6 deletions internal/web/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
}
Expand Down
2 changes: 1 addition & 1 deletion web/src/components/BranchPicker.vue
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ function reset() {
<button class="bp-confirm-btn discard" :disabled="switching" @click="applyStrategy('force', close)">
{{ t('branches.confirmDiscard') }}
</button>
<button class="bp-confirm-btn cancel" :disabled="switching" @click="cancelPending">
<button class="bp-confirm-btn cancel" :disabled="switching" @click="reset">
{{ t('branches.confirmCancel') }}
</button>
</div>
Expand Down
2 changes: 1 addition & 1 deletion web/src/components/ProviderIcon.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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() || '?',
)
</script>

Expand Down
11 changes: 11 additions & 0 deletions web/src/components/Sidebar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions web/src/i18n/locales/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
Loading
Loading