diff --git a/backend/internal/adapters/agent/agy/hooks.go b/backend/internal/adapters/agent/agy/hooks.go index 016892938..ec494c119 100644 --- a/backend/internal/adapters/agent/agy/hooks.go +++ b/backend/internal/adapters/agent/agy/hooks.go @@ -84,6 +84,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi return fmt.Errorf("agy.GetAgentHooks: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(hooksPath), agyHooksFileName); err != nil { + return fmt.Errorf("agy.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/claudecode/hooks.go b/backend/internal/adapters/agent/claudecode/hooks.go index 0f9bd92c9..da8fbb6a6 100644 --- a/backend/internal/adapters/agent/claudecode/hooks.go +++ b/backend/internal/adapters/agent/claudecode/hooks.go @@ -107,6 +107,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := writeClaudeSettings(settingsPath, topLevel, rawHooks); err != nil { return fmt.Errorf("claude-code.GetAgentHooks: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(settingsPath), claudeSettingsFileName); err != nil { + return fmt.Errorf("claude-code.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/cline/hooks.go b/backend/internal/adapters/agent/cline/hooks.go index fc2d0fb98..53df0d0b7 100644 --- a/backend/internal/adapters/agent/cline/hooks.go +++ b/backend/internal/adapters/agent/cline/hooks.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/hookutil" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) @@ -75,6 +76,10 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi return fmt.Errorf("cline.GetAgentHooks: create hook dir: %w", err) } + // Only scripts AO actually wrote go into the workspace .gitignore: a + // user-authored script at one of these paths must keep counting as dirt so + // workspace teardown preserves it. + written := make([]string, 0, len(clineManagedHooks)) for _, spec := range clineManagedHooks { scriptPath := filepath.Join(hooksDir, spec.Event) // Never clobber a user-authored hook with the same event name. @@ -85,6 +90,10 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := atomicWriteFile(scriptPath, []byte(script), 0o700); err != nil { return fmt.Errorf("cline.GetAgentHooks: write %s: %w", spec.Event, err) } + written = append(written, spec.Event) + } + if err := hookutil.EnsureWorkspaceGitignore(hooksDir, written...); err != nil { + return fmt.Errorf("cline.GetAgentHooks: gitignore: %w", err) } return nil } diff --git a/backend/internal/adapters/agent/copilot/hooks.go b/backend/internal/adapters/agent/copilot/hooks.go index f6fafe355..65e2e280d 100644 --- a/backend/internal/adapters/agent/copilot/hooks.go +++ b/backend/internal/adapters/agent/copilot/hooks.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/hookutil" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) @@ -121,6 +122,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := writeCopilotHooks(hooksPath, file); err != nil { return fmt.Errorf("copilot.GetAgentHooks: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(hooksPath), copilotHooksFileName); err != nil { + return fmt.Errorf("copilot.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/cursor/hooks.go b/backend/internal/adapters/agent/cursor/hooks.go index 7655f6075..6112e6bf4 100644 --- a/backend/internal/adapters/agent/cursor/hooks.go +++ b/backend/internal/adapters/agent/cursor/hooks.go @@ -93,6 +93,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := writeCursorHooks(hooksPath, topLevel, rawHooks); err != nil { return fmt.Errorf("cursor.GetAgentHooks: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(hooksPath), cursorHooksFileName); err != nil { + return fmt.Errorf("cursor.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/droid/hooks.go b/backend/internal/adapters/agent/droid/hooks.go index e9ed3287a..55c4f561c 100644 --- a/backend/internal/adapters/agent/droid/hooks.go +++ b/backend/internal/adapters/agent/droid/hooks.go @@ -113,6 +113,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := writeDroidHooks(hooksPath, topLevel, rawHooks); err != nil { return fmt.Errorf("droid.GetAgentHooks: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(hooksPath), droidHooksFileName); err != nil { + return fmt.Errorf("droid.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/goose/hooks.go b/backend/internal/adapters/agent/goose/hooks.go index 0e128a75b..f631659e5 100644 --- a/backend/internal/adapters/agent/goose/hooks.go +++ b/backend/internal/adapters/agent/goose/hooks.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/hookutil" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) @@ -99,6 +100,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := writeGooseHooks(hooksPath, topLevel, rawHooks); err != nil { return fmt.Errorf("goose.GetAgentHooks: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(hooksPath), gooseHooksFileName); err != nil { + return fmt.Errorf("goose.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/hookutil/hookutil.go b/backend/internal/adapters/agent/hookutil/hookutil.go index 4333577fa..cf1f6fe6d 100644 --- a/backend/internal/adapters/agent/hookutil/hookutil.go +++ b/backend/internal/adapters/agent/hookutil/hookutil.go @@ -4,10 +4,55 @@ package hookutil import ( + "errors" + "fmt" "os" "path/filepath" + "strings" ) +// GitignoreSentinel marks a workspace .gitignore as AO-managed so +// EnsureWorkspaceGitignore can rewrite its own file idempotently while never +// touching a user- or repo-provided .gitignore at the same path. +const GitignoreSentinel = "# managed by agent-orchestrator: AO hook files stay out of git status" + +// EnsureWorkspaceGitignore writes a self-ignoring .gitignore into dir covering +// the named AO-installed files. Hook files land in fresh session worktrees as +// untracked files, and `git worktree remove` (without --force) refuses on ANY +// untracked file — without this ignore, AO's own hook files would make every +// session workspace permanently undeletable. The patterns are anchored to dir +// and name only AO's files, so anything else an agent drops in the same +// directory still counts as dirt and keeps blocking teardown. +// +// A .gitignore at the same path that lacks the sentinel is left untouched and +// the install proceeds: the worktree then simply stays dirty and teardown +// preserves it, which is the safe degradation. +func EnsureWorkspaceGitignore(dir string, names ...string) error { + path := filepath.Join(dir, ".gitignore") + existing, err := os.ReadFile(path) //nolint:gosec // path built from caller-owned workspace dir + if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("read %s: %w", path, err) + } + if err == nil && !strings.Contains(string(existing), GitignoreSentinel) { + return nil + } + var b strings.Builder + b.WriteString(GitignoreSentinel) + b.WriteString("\n/.gitignore\n") + for _, name := range names { + b.WriteString("/") + b.WriteString(filepath.ToSlash(name)) + b.WriteString("\n") + } + if err := os.MkdirAll(dir, 0o750); err != nil { + return fmt.Errorf("create %s: %w", dir, err) + } + if err := AtomicWriteFile(path, []byte(b.String()), 0o600); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + return nil +} + // AtomicWriteFile writes data to path via a temp file in the same directory // followed by a rename, so a crash or signal mid-write can't leave a truncated // or empty file that the agent then fails to parse (silently disabling hooks). diff --git a/backend/internal/adapters/agent/hookutil/hookutil_test.go b/backend/internal/adapters/agent/hookutil/hookutil_test.go new file mode 100644 index 000000000..0f97739e1 --- /dev/null +++ b/backend/internal/adapters/agent/hookutil/hookutil_test.go @@ -0,0 +1,73 @@ +package hookutil + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestEnsureWorkspaceGitignoreWritesSelfIgnoringFile(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".codex") + if err := EnsureWorkspaceGitignore(dir, "hooks.json", "config.toml"); err != nil { + t.Fatalf("ensure: %v", err) + } + data, err := os.ReadFile(filepath.Join(dir, ".gitignore")) + if err != nil { + t.Fatalf("read: %v", err) + } + content := string(data) + if !strings.Contains(content, GitignoreSentinel) { + t.Fatalf("content missing sentinel: %q", content) + } + // Entries are anchored so only AO's files in THIS directory are ignored — + // an agent's own files (even in the same dir) must keep counting as dirt. + for _, want := range []string{"/.gitignore\n", "/hooks.json\n", "/config.toml\n"} { + if !strings.Contains(content, want) { + t.Errorf("content missing entry %q: %q", want, content) + } + } +} + +func TestEnsureWorkspaceGitignoreIsIdempotent(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".codex") + if err := EnsureWorkspaceGitignore(dir, "hooks.json"); err != nil { + t.Fatalf("first ensure: %v", err) + } + first, err := os.ReadFile(filepath.Join(dir, ".gitignore")) + if err != nil { + t.Fatalf("read: %v", err) + } + if err := EnsureWorkspaceGitignore(dir, "hooks.json"); err != nil { + t.Fatalf("second ensure: %v", err) + } + second, err := os.ReadFile(filepath.Join(dir, ".gitignore")) + if err != nil { + t.Fatalf("read: %v", err) + } + if string(first) != string(second) { + t.Fatalf("rewrite changed content:\nfirst: %q\nsecond: %q", first, second) + } +} + +func TestEnsureWorkspaceGitignoreLeavesForeignFileUntouched(t *testing.T) { + dir := filepath.Join(t.TempDir(), ".codex") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + foreign := "# user rules\n*.log\n" + path := filepath.Join(dir, ".gitignore") + if err := os.WriteFile(path, []byte(foreign), 0o600); err != nil { + t.Fatalf("seed: %v", err) + } + if err := EnsureWorkspaceGitignore(dir, "hooks.json"); err != nil { + t.Fatalf("ensure: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read: %v", err) + } + if string(data) != foreign { + t.Fatalf("foreign .gitignore was modified: %q", data) + } +} diff --git a/backend/internal/adapters/agent/kilocode/hooks.go b/backend/internal/adapters/agent/kilocode/hooks.go index 329740773..e0e2056ed 100644 --- a/backend/internal/adapters/agent/kilocode/hooks.go +++ b/backend/internal/adapters/agent/kilocode/hooks.go @@ -10,6 +10,7 @@ import ( _ "embed" + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/hookutil" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) @@ -94,6 +95,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := atomicWriteFile(pluginPath, []byte(kilocodePluginSource), 0o600); err != nil { return fmt.Errorf("kilocode.GetAgentHooks: write plugin: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(pluginPath), kilocodePluginFileName); err != nil { + return fmt.Errorf("kilocode.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/kiro/hooks.go b/backend/internal/adapters/agent/kiro/hooks.go index e5f814992..9ad42106f 100644 --- a/backend/internal/adapters/agent/kiro/hooks.go +++ b/backend/internal/adapters/agent/kiro/hooks.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/hookutil" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) @@ -99,6 +100,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := writeKiroHooks(hooksPath, topLevel, rawHooks); err != nil { return fmt.Errorf("kiro.GetAgentHooks: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(hooksPath), kiroAgentFileName); err != nil { + return fmt.Errorf("kiro.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/opencode/hooks.go b/backend/internal/adapters/agent/opencode/hooks.go index 6f87dc48e..269e7f6a7 100644 --- a/backend/internal/adapters/agent/opencode/hooks.go +++ b/backend/internal/adapters/agent/opencode/hooks.go @@ -94,6 +94,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := hookutil.AtomicWriteFile(pluginPath, []byte(opencodePluginSource), 0o600); err != nil { return fmt.Errorf("opencode.GetAgentHooks: write plugin: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(pluginPath), opencodePluginFileName); err != nil { + return fmt.Errorf("opencode.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/qwen/hooks.go b/backend/internal/adapters/agent/qwen/hooks.go index a488e2298..eb6e91ee6 100644 --- a/backend/internal/adapters/agent/qwen/hooks.go +++ b/backend/internal/adapters/agent/qwen/hooks.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/hookutil" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) @@ -112,6 +113,9 @@ func (p *Plugin) GetAgentHooks(ctx context.Context, cfg ports.WorkspaceHookConfi if err := writeQwenSettings(settingsPath, topLevel, rawHooks); err != nil { return fmt.Errorf("qwen.GetAgentHooks: %w", err) } + if err := hookutil.EnsureWorkspaceGitignore(filepath.Dir(settingsPath), qwenSettingsFileName); err != nil { + return fmt.Errorf("qwen.GetAgentHooks: gitignore: %w", err) + } return nil } diff --git a/backend/internal/adapters/agent/registry/registry_test.go b/backend/internal/adapters/agent/registry/registry_test.go new file mode 100644 index 000000000..269abced9 --- /dev/null +++ b/backend/internal/adapters/agent/registry/registry_test.go @@ -0,0 +1,85 @@ +package registry + +import ( + "context" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/hookutil" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// TestGetAgentHooksFootprintIsGitignored enforces a contract every shipped +// (and future) adapter must hold: any file GetAgentHooks writes into a session +// worktree must be covered by a sibling AO-managed self-ignoring .gitignore +// (hookutil.EnsureWorkspaceGitignore). Hook files are untracked, and +// `git worktree remove` (without --force) refuses on any untracked file — an +// uncovered hook file makes every one of that adapter's session workspaces +// permanently undeletable (kill/cleanup can never free them). +func TestGetAgentHooksFootprintIsGitignored(t *testing.T) { + for _, ha := range Harnessed() { + t.Run(string(ha.Harness), func(t *testing.T) { + ws := t.TempDir() + cfg := ports.WorkspaceHookConfig{ + SessionID: "proj-1", + WorkspacePath: ws, + DataDir: t.TempDir(), + } + if err := ha.Agent.GetAgentHooks(context.Background(), cfg); err != nil { + t.Fatalf("GetAgentHooks: %v", err) + } + files := workspaceFiles(t, ws) + for _, rel := range files { + gitignorePath := filepath.Join(ws, filepath.Dir(rel), ".gitignore") + data, err := os.ReadFile(gitignorePath) //nolint:gosec // test-owned temp dir + if err != nil { + t.Errorf("hook file %q has no sibling .gitignore (%v); it will keep the session worktree permanently dirty", rel, err) + continue + } + content := string(data) + if !strings.Contains(content, hookutil.GitignoreSentinel) { + t.Errorf(".gitignore next to %q is not AO-managed (missing sentinel)", rel) + continue + } + if entry := "/" + filepath.Base(rel); !hasLine(content, entry) { + t.Errorf(".gitignore next to %q does not list %q", rel, entry) + } + } + }) + } +} + +// workspaceFiles returns every regular file under root, relative to root. +func workspaceFiles(t *testing.T, root string) []string { + t.Helper() + var files []string + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.Type().IsRegular() { + rel, err := filepath.Rel(root, path) + if err != nil { + return err + } + files = append(files, rel) + } + return nil + }) + if err != nil { + t.Fatalf("walk workspace: %v", err) + } + return files +} + +func hasLine(content, line string) bool { + for _, l := range strings.Split(content, "\n") { + if strings.TrimSpace(l) == line { + return true + } + } + return false +} diff --git a/backend/internal/adapters/workspace/gitworktree/commands.go b/backend/internal/adapters/workspace/gitworktree/commands.go index 356a50e11..fe696443c 100644 --- a/backend/internal/adapters/workspace/gitworktree/commands.go +++ b/backend/internal/adapters/workspace/gitworktree/commands.go @@ -31,6 +31,13 @@ func worktreePruneArgs(repo string) []string { return []string{"-C", repo, "worktree", "prune"} } +// statusPorcelainArgs probes the worktree at path for uncommitted changes or +// untracked files — the condition `git worktree remove` (without --force) +// refuses on — so Destroy can classify a refusal as ports.ErrWorkspaceDirty. +func statusPorcelainArgs(path string) []string { + return []string{"-C", path, "status", "--porcelain"} +} + func worktreeListPorcelainArgs(repo string) []string { return []string{"-C", repo, "worktree", "list", "--porcelain"} } diff --git a/backend/internal/adapters/workspace/gitworktree/workspace.go b/backend/internal/adapters/workspace/gitworktree/workspace.go index 2ec013081..a54fb93e6 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace.go @@ -157,6 +157,18 @@ func (w *Workspace) Destroy(ctx context.Context, info ports.WorkspaceInfo) error } if _, ok := findWorktree(records, path); ok { if removeErr != nil { + // Distinguish the dirty-worktree refusal (uncommitted agent work) + // from other registration leftovers (e.g. a locked worktree) so the + // Session Manager can preserve the workspace without erroring. + dirty, statusErr := w.isDirty(ctx, path) + if statusErr == nil && dirty { + return fmt.Errorf("gitworktree: refusing to remove %q: %w (worktree remove: %w)", path, ports.ErrWorkspaceDirty, removeErr) + } + if statusErr != nil { + // A failed probe must stay visible: without it the caller can't + // tell "not dirty" from "couldn't check". + return fmt.Errorf("gitworktree: refusing to remove %q: path is still registered after git worktree prune (worktree remove: %w; dirty probe: %w)", path, removeErr, statusErr) + } return fmt.Errorf("gitworktree: refusing to remove %q: path is still registered after git worktree prune (worktree remove: %w)", path, removeErr) } return fmt.Errorf("gitworktree: refusing to remove %q: path is still registered after git worktree prune", path) @@ -302,6 +314,17 @@ func (w *Workspace) refExists(ctx context.Context, repo, ref string) (bool, erro return false, fmt.Errorf("gitworktree: verify ref %q: %w", ref, err) } +// isDirty reports whether the worktree at path has uncommitted changes or +// untracked files — the same check `git worktree remove` performs before +// refusing without --force. +func (w *Workspace) isDirty(ctx context.Context, path string) (bool, error) { + out, err := w.run(ctx, w.binary, statusPorcelainArgs(path)...) + if err != nil { + return false, fmt.Errorf("gitworktree: status %q: %w", path, err) + } + return strings.TrimSpace(string(out)) != "", nil +} + func (w *Workspace) listRecords(ctx context.Context, repo string) ([]worktreeRecord, error) { out, err := w.run(ctx, w.binary, worktreeListPorcelainArgs(repo)...) if err != nil { diff --git a/backend/internal/adapters/workspace/gitworktree/workspace_integration_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_integration_test.go index 6dc50f767..78c87590b 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_integration_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_integration_test.go @@ -92,6 +92,68 @@ func TestWorkspaceIntegrationDestroyRefusesLockedWorktree(t *testing.T) { } } +// TestWorkspaceIntegrationDestroyDirtyWorktree proves the two halves of the +// dirty-teardown contract against real git: +// +// 1. A worktree whose only untracked files are covered by a self-ignoring +// .gitignore (the shape agent adapters install for their hook files) is +// clean in git's eyes, so Destroy succeeds without --force. +// 2. Real uncommitted work makes Destroy refuse with ports.ErrWorkspaceDirty +// and preserves the worktree. +func TestWorkspaceIntegrationDestroyDirtyWorktree(t *testing.T) { + git := requireGit(t) + tmp := t.TempDir() + repo := setupOriginClone(t, git, tmp) + root := filepath.Join(tmp, "managed") + ws, err := New(Options{Binary: git, ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}}) + if err != nil { + t.Fatalf("new: %v", err) + } + ctx := context.Background() + info, err := ws.Create(ctx, ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/dirty"}) + if err != nil { + t.Fatalf("create: %v", err) + } + + // AO-managed hook files behind a self-ignoring .gitignore: invisible to git + // status, so they must not block teardown. + hookDir := filepath.Join(info.Path, ".codex") + if err := os.MkdirAll(hookDir, 0o750); err != nil { + t.Fatalf("mkdir hook dir: %v", err) + } + if err := os.WriteFile(filepath.Join(hookDir, "hooks.json"), []byte("{}\n"), 0o600); err != nil { + t.Fatalf("write hooks.json: %v", err) + } + if err := os.WriteFile(filepath.Join(hookDir, ".gitignore"), []byte(".gitignore\nhooks.json\n"), 0o600); err != nil { + t.Fatalf("write .gitignore: %v", err) + } + + // Real agent work must keep blocking teardown, typed as ErrWorkspaceDirty. + wip := filepath.Join(info.Path, "wip.txt") + if err := os.WriteFile(wip, []byte("uncommitted\n"), 0o600); err != nil { + t.Fatalf("write wip: %v", err) + } + err = ws.Destroy(ctx, info) + if !errors.Is(err, ports.ErrWorkspaceDirty) { + t.Fatalf("destroy dirty error = %v, want ports.ErrWorkspaceDirty", err) + } + if _, statErr := os.Stat(wip); statErr != nil { + t.Fatalf("dirty worktree was not preserved: %v", statErr) + } + + // With the real work gone, only the ignored AO files remain — git considers + // the worktree clean and Destroy succeeds without --force. + if err := os.Remove(wip); err != nil { + t.Fatalf("remove wip: %v", err) + } + if err := ws.Destroy(ctx, info); err != nil { + t.Fatalf("destroy with ignored-only files: %v", err) + } + if _, err := os.Stat(info.Path); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("path after destroy stat err = %v, want not exist", err) + } +} + func requireGit(t *testing.T) string { t.Helper() git, err := exec.LookPath("git") diff --git a/backend/internal/adapters/workspace/gitworktree/workspace_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_test.go index 4563d086b..ed9068955 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_test.go @@ -33,6 +33,7 @@ func TestCommandArgs(t *testing.T) { {"remove", worktreeRemoveArgs(repo, path), []string{"-C", repo, "worktree", "remove", path}}, {"prune", worktreePruneArgs(repo), []string{"-C", repo, "worktree", "prune"}}, {"list", worktreeListPorcelainArgs(repo), []string{"-C", repo, "worktree", "list", "--porcelain"}}, + {"status", statusPorcelainArgs(path), []string{"-C", path, "status", "--porcelain"}}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -216,6 +217,12 @@ func TestDestroyRefusesStillRegisteredPathAndPreservesDirectory(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "still registered") { t.Fatalf("destroy error = %v", err) } + // The stub reports a clean `git status`, so the refusal must NOT be typed as + // a dirty workspace — Kill/Cleanup would otherwise silently skip a refusal + // that has a different cause (e.g. a locked worktree). + if errors.Is(err, ports.ErrWorkspaceDirty) { + t.Fatalf("destroy error = %v, want non-dirty refusal for clean status", err) + } if _, statErr := os.Stat(filepath.Join(path, "keep.txt")); statErr != nil { t.Fatalf("expected directory to be preserved: %v", statErr) } @@ -229,6 +236,46 @@ func TestDestroyRefusesStillRegisteredPathAndPreservesDirectory(t *testing.T) { } } +// TestDestroyClassifiesDirtyWorktree covers the typed dirty refusal: when +// `git worktree remove` fails, the path stays registered, and `git status` +// reports uncommitted work, Destroy must wrap ports.ErrWorkspaceDirty so the +// Session Manager can preserve the workspace (Kill freed=false, Cleanup +// skipped-with-reason) instead of surfacing an opaque 500. +func TestDestroyClassifiesDirtyWorktree(t *testing.T) { + root := t.TempDir() + repo := t.TempDir() + ws, err := New(Options{ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}}) + if err != nil { + t.Fatalf("new: %v", err) + } + path := filepath.Join(ws.managedRoot, "proj", "sess") + if err := mkdirFile(path, "keep.txt"); err != nil { + t.Fatalf("seed path: %v", err) + } + ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "worktree remove"): + return []byte("contains modified or untracked files"), errors.New("remove failed") + case strings.Contains(joined, "worktree prune"): + return nil, nil + case strings.Contains(joined, "worktree list --porcelain"): + return []byte("worktree " + path + "\nbranch refs/heads/feature/one\n"), nil + case strings.Contains(joined, "status --porcelain"): + return []byte("?? keep.txt\n"), nil + default: + return nil, nil + } + } + err = ws.Destroy(context.Background(), ports.WorkspaceInfo{Path: path, ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) + if !errors.Is(err, ports.ErrWorkspaceDirty) { + t.Fatalf("destroy error = %v, want ports.ErrWorkspaceDirty", err) + } + if _, statErr := os.Stat(filepath.Join(path, "keep.txt")); statErr != nil { + t.Fatalf("expected dirty worktree to be preserved: %v", statErr) + } +} + // TestAddWorktreeRefusesBranchCheckedOutElsewhere covers Bug 3 (a): if the // requested branch is already checked out in another worktree of the same repo, // Create must surface ports.ErrWorkspaceBranchCheckedOutElsewhere so the HTTP diff --git a/backend/internal/cli/dto_drift_e2e_test.go b/backend/internal/cli/dto_drift_e2e_test.go index 3957ea30f..2488d8f4e 100644 --- a/backend/internal/cli/dto_drift_e2e_test.go +++ b/backend/internal/cli/dto_drift_e2e_test.go @@ -82,8 +82,8 @@ func (f *fakeSessionService) RollbackSpawn(context.Context, domain.SessionID) (s return sessionsvc.RollbackOutcome{}, nil } -func (f *fakeSessionService) Cleanup(context.Context, domain.ProjectID) ([]domain.SessionID, error) { - return nil, nil +func (f *fakeSessionService) Cleanup(context.Context, domain.ProjectID) (sessionsvc.CleanupOutcome, error) { + return sessionsvc.CleanupOutcome{}, nil } func (f *fakeSessionService) Rename(context.Context, domain.SessionID, string) error { diff --git a/backend/internal/cli/session.go b/backend/internal/cli/session.go index 2e0d84cc3..f57e8740d 100644 --- a/backend/internal/cli/session.go +++ b/backend/internal/cli/session.go @@ -68,6 +68,7 @@ type sessionResponse struct { type killSessionResponse struct { SessionID string `json:"sessionId"` + Freed bool `json:"freed"` } type restoreSessionResponse struct { @@ -80,8 +81,14 @@ type renameSessionResponse struct { DisplayName string `json:"displayName"` } +type cleanupSkippedSession struct { + SessionID string `json:"sessionId"` + Reason string `json:"reason"` +} + type cleanupSessionsResponse struct { - Cleaned []string `json:"cleaned"` + Cleaned []string `json:"cleaned"` + Skipped []cleanupSkippedSession `json:"skipped"` } type claimPRRequest struct { @@ -431,7 +438,13 @@ func (c *commandContext) killSession(ctx context.Context, cmd *cobra.Command, id if err := c.postJSON(ctx, "sessions/"+url.PathEscape(id)+"/kill", struct{}{}, &res); err != nil { return err } - _, err := fmt.Fprintf(cmd.OutOrStdout(), "session %s killed\n", res.SessionID) + if res.Freed { + _, err := fmt.Fprintf(cmd.OutOrStdout(), "session %s killed\n", res.SessionID) + return err + } + // freed=false: the workspace was preserved (e.g. uncommitted changes) — the + // session is terminated either way, but the worktree is left for inspection. + _, err := fmt.Fprintf(cmd.OutOrStdout(), "session %s killed (workspace preserved)\n", res.SessionID) return err } @@ -530,7 +543,20 @@ func (c *commandContext) cleanupSessions(ctx context.Context, cmd *cobra.Command return err } } - _, err = fmt.Fprintf(out, "\nCleanup complete. %d session%s cleaned.\n", len(cleaned), pluralS(len(cleaned))) + for _, skip := range res.Skipped { + label := skip.SessionID + if mapped := labelByID[skip.SessionID]; mapped != "" { + label = mapped + } + if _, err := fmt.Fprintf(out, " Skipped: %s (%s)\n", label, skip.Reason); err != nil { + return err + } + } + summary := fmt.Sprintf("\nCleanup complete. %d session%s cleaned", len(cleaned), pluralS(len(cleaned))) + if len(res.Skipped) > 0 { + summary += fmt.Sprintf(", %d skipped", len(res.Skipped)) + } + _, err = fmt.Fprintf(out, "%s.\n", summary) return err } diff --git a/backend/internal/cli/session_test.go b/backend/internal/cli/session_test.go index f33723901..d7aa2f513 100644 --- a/backend/internal/cli/session_test.go +++ b/backend/internal/cli/session_test.go @@ -69,7 +69,7 @@ func sessionCommandServer(t *testing.T) (*httptest.Server, *sessionRequestLog) { } _, _ = io.WriteString(w, `{"ok":true,"sessionId":"demo-1","prs":[{"url":`+jsonQuote(req.PR)+`,"number":142,"state":"open","ci":"passing","review":"review_required","mergeability":"mergeable","reviewComments":false,"updatedAt":"2026-06-04T12:00:00Z"}],"branchChanged":true,"takenOverFrom":["demo-0"]}`) case r.Method == http.MethodPost && r.URL.Path == "/api/v1/sessions/cleanup": - _, _ = io.WriteString(w, `{"ok":true,"cleaned":["demo-old","demo-orch"]}`) + _, _ = io.WriteString(w, `{"ok":true,"cleaned":["demo-old","demo-orch"],"skipped":[]}`) case r.Method == http.MethodPost && r.URL.Path == "/api/v1/sessions/demo-1/kill": _, _ = io.WriteString(w, `{"ok":true,"sessionId":"demo-1","freed":true}`) case r.Method == http.MethodPost && r.URL.Path == "/api/v1/sessions/demo-1/restore": @@ -225,6 +225,33 @@ func TestSessionKill_SuccessWithProjectScope(t *testing.T) { } } +// TestSessionKill_PreservedWorkspaceNote: freed=false means the daemon +// terminated the session but kept the worktree (uncommitted changes are never +// force-deleted) — the CLI must say so instead of implying a full teardown. +func TestSessionKill_PreservedWorkspaceNote(t *testing.T) { + cfg := setConfigEnv(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPost && r.URL.Path == "/api/v1/sessions/demo-1/kill" { + _, _ = io.WriteString(w, `{"ok":true,"sessionId":"demo-1","freed":false}`) + return + } + http.NotFound(w, r) + })) + t.Cleanup(srv.Close) + writeRunFileFor(t, cfg, srv) + + out, errOut, err := executeCLI(t, Deps{ + ProcessAlive: func(int) bool { return true }, + }, "session", "kill", "demo-1") + if err != nil { + t.Fatalf("session kill failed: %v\nstderr=%s", err, errOut) + } + if !strings.Contains(out, "session demo-1 killed (workspace preserved)") { + t.Fatalf("unexpected kill output:\n%s", out) + } +} + func TestSessionRestore_SuccessWithProjectScope(t *testing.T) { cfg := setConfigEnv(t) srv, log := sessionCommandServer(t) @@ -274,6 +301,45 @@ func TestSessionCleanup_YesSkipsPrompt(t *testing.T) { } } +// TestSessionCleanup_ReportsSkippedWorkspaces: a session whose workspace was +// preserved must be listed with its reason and counted in the summary — +// previously the CLI printed "Would clean N" then "0 sessions cleaned" with no +// explanation, leaking workspaces invisibly. +func TestSessionCleanup_ReportsSkippedWorkspaces(t *testing.T) { + cfg := setConfigEnv(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/sessions": + _, _ = io.WriteString(w, `{"sessions":[`+ + sessionJSON("demo-old", "demo", "worker", "terminated", true)+`,`+ + sessionJSON("demo-orch", "demo", "orchestrator", "terminated", true)+`]}`) + case r.Method == http.MethodPost && r.URL.Path == "/api/v1/sessions/cleanup": + _, _ = io.WriteString(w, `{"ok":true,"cleaned":["demo-old"],"skipped":[{"sessionId":"demo-orch","reason":"workspace has uncommitted changes"}]}`) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + writeRunFileFor(t, cfg, srv) + + out, errOut, err := executeCLI(t, Deps{ + ProcessAlive: func(int) bool { return true }, + }, "session", "cleanup", "--project", "demo", "--yes") + if err != nil { + t.Fatalf("session cleanup failed: %v\nstderr=%s", err, errOut) + } + for _, want := range []string{ + "Cleaned: demo-old", + "Skipped: demo-orch (workspace has uncommitted changes)", + "Cleanup complete. 1 session cleaned, 1 skipped.", + } { + if !strings.Contains(out, want) { + t.Fatalf("cleanup output missing %q:\n%s", want, out) + } + } +} + func TestSessionCleanup_PromptFailsWithoutInput(t *testing.T) { cfg := setConfigEnv(t) srv, log := sessionCommandServer(t) diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml index c1c3a087b..a5f472eff 100644 --- a/backend/internal/httpd/apispec/openapi.yaml +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -1028,9 +1028,24 @@ components: type: array ok: type: boolean + skipped: + items: + $ref: '#/components/schemas/CleanupSkippedSession' + type: array required: - ok - cleaned + - skipped + type: object + CleanupSkippedSession: + properties: + reason: + type: string + sessionId: + type: string + required: + - sessionId + - reason type: object DegradedProject: properties: diff --git a/backend/internal/httpd/apispec/specgen/build.go b/backend/internal/httpd/apispec/specgen/build.go index 5a3ab42d9..c198eddea 100644 --- a/backend/internal/httpd/apispec/specgen/build.go +++ b/backend/internal/httpd/apispec/specgen/build.go @@ -139,6 +139,7 @@ var schemaNames = map[string]string{ "ControllersRenameSessionResponse": "RenameSessionResponse", "ControllersRestoreSessionResponse": "RestoreSessionResponse", "ControllersCleanupSessionsResponse": "CleanupSessionsResponse", + "ControllersCleanupSkippedSession": "CleanupSkippedSession", "ControllersKillSessionResponse": "KillSessionResponse", "ControllersRollbackSessionResponse": "RollbackSessionResponse", "ControllersSendSessionMessageRequest": "SendSessionMessageRequest", diff --git a/backend/internal/httpd/controllers/dto.go b/backend/internal/httpd/controllers/dto.go index f7907c407..557c42ee9 100644 --- a/backend/internal/httpd/controllers/dto.go +++ b/backend/internal/httpd/controllers/dto.go @@ -167,10 +167,19 @@ type RollbackSessionResponse struct { Killed bool `json:"killed,omitempty"` } +// CleanupSkippedSession is one terminal session whose workspace cleanup +// preserved rather than reclaimed (a dirty worktree is never force-deleted), +// with the user-facing reason. +type CleanupSkippedSession struct { + SessionID domain.SessionID `json:"sessionId"` + Reason string `json:"reason"` +} + // CleanupSessionsResponse is the body of POST /api/v1/sessions/cleanup. type CleanupSessionsResponse struct { - OK bool `json:"ok"` - Cleaned []domain.SessionID `json:"cleaned"` + OK bool `json:"ok"` + Cleaned []domain.SessionID `json:"cleaned"` + Skipped []CleanupSkippedSession `json:"skipped"` } // SendSessionMessageRequest is the body of POST /api/v1/sessions/{sessionId}/send. diff --git a/backend/internal/httpd/controllers/sessions.go b/backend/internal/httpd/controllers/sessions.go index dc7d6cfc7..bf0335841 100644 --- a/backend/internal/httpd/controllers/sessions.go +++ b/backend/internal/httpd/controllers/sessions.go @@ -31,7 +31,7 @@ type SessionService interface { Restore(ctx context.Context, id domain.SessionID) (domain.Session, error) Kill(ctx context.Context, id domain.SessionID) (bool, error) RollbackSpawn(ctx context.Context, id domain.SessionID) (sessionsvc.RollbackOutcome, error) - Cleanup(ctx context.Context, project domain.ProjectID) ([]domain.SessionID, error) + Cleanup(ctx context.Context, project domain.ProjectID) (sessionsvc.CleanupOutcome, error) Rename(ctx context.Context, id domain.SessionID, displayName string) error Send(ctx context.Context, id domain.SessionID, message string) error ListPRs(ctx context.Context, id domain.SessionID) ([]domain.PRFacts, error) @@ -244,12 +244,16 @@ func (c *SessionsController) cleanup(w http.ResponseWriter, r *http.Request) { apispec.NotImplemented(w, r, "POST", "/api/v1/sessions/cleanup") return } - cleaned, err := c.Svc.Cleanup(r.Context(), domain.ProjectID(r.URL.Query().Get("project"))) + out, err := c.Svc.Cleanup(r.Context(), domain.ProjectID(r.URL.Query().Get("project"))) if err != nil { envelope.WriteError(w, r, err) return } - envelope.WriteJSON(w, http.StatusOK, CleanupSessionsResponse{OK: true, Cleaned: cleaned}) + skipped := make([]CleanupSkippedSession, 0, len(out.Skipped)) + for _, skip := range out.Skipped { + skipped = append(skipped, CleanupSkippedSession{SessionID: skip.SessionID, Reason: skip.Reason}) + } + envelope.WriteJSON(w, http.StatusOK, CleanupSessionsResponse{OK: true, Cleaned: out.Cleaned, Skipped: skipped}) } func (c *SessionsController) send(w http.ResponseWriter, r *http.Request) { diff --git a/backend/internal/httpd/controllers/sessions_test.go b/backend/internal/httpd/controllers/sessions_test.go index 6ed967cc5..3bc53b607 100644 --- a/backend/internal/httpd/controllers/sessions_test.go +++ b/backend/internal/httpd/controllers/sessions_test.go @@ -22,6 +22,7 @@ type fakeSessionService struct { sent string cleanupProjects []domain.ProjectID cleanupResult []domain.SessionID + cleanupSkipped []sessionsvc.CleanupSkipped claimErr error listPRErr error } @@ -100,12 +101,13 @@ func (f *fakeSessionService) RollbackSpawn(_ context.Context, id domain.SessionI return sessionsvc.RollbackOutcome{}, nil } -func (f *fakeSessionService) Cleanup(_ context.Context, project domain.ProjectID) ([]domain.SessionID, error) { +func (f *fakeSessionService) Cleanup(_ context.Context, project domain.ProjectID) (sessionsvc.CleanupOutcome, error) { f.cleanupProjects = append(f.cleanupProjects, project) - if f.cleanupResult != nil { - return f.cleanupResult, nil + cleaned := f.cleanupResult + if cleaned == nil { + cleaned = []domain.SessionID{"ao-1"} } - return []domain.SessionID{"ao-1"}, nil + return sessionsvc.CleanupOutcome{Cleaned: cleaned, Skipped: f.cleanupSkipped}, nil } func (f *fakeSessionService) Rename(_ context.Context, id domain.SessionID, displayName string) error { @@ -318,6 +320,7 @@ func TestSessionsAPI_SendValidation(t *testing.T) { func TestSessionsAPI_CleanupWithProjectFilter(t *testing.T) { svc := newFakeSessionService() svc.cleanupResult = []domain.SessionID{"ao-1"} + svc.cleanupSkipped = []sessionsvc.CleanupSkipped{{SessionID: "ao-2", Reason: "workspace has uncommitted changes"}} srv := newSessionTestServer(t, svc) body, status, _ := doRequest(t, srv, "POST", "/api/v1/sessions/cleanup?project=ao", "") @@ -327,11 +330,18 @@ func TestSessionsAPI_CleanupWithProjectFilter(t *testing.T) { var got struct { OK bool `json:"ok"` Cleaned []string `json:"cleaned"` + Skipped []struct { + SessionID string `json:"sessionId"` + Reason string `json:"reason"` + } `json:"skipped"` } mustJSON(t, body, &got) if !got.OK || len(got.Cleaned) != 1 || got.Cleaned[0] != "ao-1" { t.Fatalf("cleanup response = %#v", got) } + if len(got.Skipped) != 1 || got.Skipped[0].SessionID != "ao-2" || got.Skipped[0].Reason != "workspace has uncommitted changes" { + t.Fatalf("cleanup skipped = %#v, want preserved workspace with reason", got.Skipped) + } if len(svc.cleanupProjects) != 1 || svc.cleanupProjects[0] != "ao" { t.Fatalf("cleanupProjects = %#v, want [ao]", svc.cleanupProjects) } diff --git a/backend/internal/httpd/envelope/envelope.go b/backend/internal/httpd/envelope/envelope.go index 3768d19fe..b3ed37a02 100644 --- a/backend/internal/httpd/envelope/envelope.go +++ b/backend/internal/httpd/envelope/envelope.go @@ -1,6 +1,7 @@ package envelope import ( + "context" "encoding/json" "errors" "net/http" @@ -10,6 +11,30 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apierr" ) +// errCapture is a request-scoped slot WriteError records the raw service error +// into. The wire envelope deliberately hides internals behind "Internal server +// error", which previously meant a 500's cause was lost entirely — the access +// log reads the captured error back so the daemon log keeps the diagnosis. +type errCapture struct{ err error } + +type errCaptureKey struct{} + +// WithErrorCapture returns a copy of the request whose context carries an +// error-capture slot, plus a getter for the error recorded by WriteError while +// handling it. The request logger installs it and reads it after the handler. +func WithErrorCapture(r *http.Request) (*http.Request, func() error) { + capture := &errCapture{} + req := r.WithContext(context.WithValue(r.Context(), errCaptureKey{}, capture)) + return req, func() error { return capture.err } +} + +// captureError records err for the request if a capture slot is present. +func captureError(r *http.Request, err error) { + if c, ok := r.Context().Value(errCaptureKey{}).(*errCapture); ok { + c.err = err + } +} + // APIError is the locked wire shape for every non-2xx response. type APIError struct { Error string `json:"error"` @@ -42,6 +67,7 @@ func WriteAPIError(w http.ResponseWriter, r *http.Request, status int, kind, cod // back to a 500 for any other error so internal details never leak. This is the // only place an apierr.Kind is translated into an HTTP status and wire word. func WriteError(w http.ResponseWriter, r *http.Request, err error) { + captureError(r, err) var e *apierr.Error if errors.As(err, &e) { status, kind := httpStatus(e.Kind) diff --git a/backend/internal/httpd/log.go b/backend/internal/httpd/log.go index abb9462ec..43eb0d784 100644 --- a/backend/internal/httpd/log.go +++ b/backend/internal/httpd/log.go @@ -6,6 +6,8 @@ import ( "time" "github.com/go-chi/chi/v5/middleware" + + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope" ) // requestLogger emits one structured access-log line per request via the @@ -18,13 +20,18 @@ import ( // is accurate even when the handler returns without calling WriteHeader. The // request id is read off the context populated by middleware.RequestID, so // this middleware must be mounted after it. +// +// A 5xx line additionally carries the raw service error recorded by +// envelope.WriteError: the wire envelope hides internals ("Internal server +// error"), so without this the cause of a 500 was lost entirely. func requestLogger(log *slog.Logger) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor) + r, capturedErr := envelope.WithErrorCapture(r) start := time.Now() defer func() { - log.Info("http request", + attrs := []any{ "id", middleware.GetReqID(r.Context()), "method", r.Method, "path", r.URL.Path, @@ -32,7 +39,11 @@ func requestLogger(log *slog.Logger) func(http.Handler) http.Handler { "bytes", ww.BytesWritten(), "duration", time.Since(start), "remote", r.RemoteAddr, - ) + } + if err := capturedErr(); err != nil && ww.Status() >= http.StatusInternalServerError { + attrs = append(attrs, "error", err) + } + log.Info("http request", attrs...) }() next.ServeHTTP(ww, r) }) diff --git a/backend/internal/httpd/log_test.go b/backend/internal/httpd/log_test.go new file mode 100644 index 000000000..c1d1f23a8 --- /dev/null +++ b/backend/internal/httpd/log_test.go @@ -0,0 +1,53 @@ +package httpd + +import ( + "bytes" + "errors" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apierr" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope" +) + +// TestRequestLoggerRecords5xxCause: the wire envelope collapses unrecognized +// service errors into "Internal server error", so the access log line is the +// only place the cause can survive. A 500 must carry it; a typed 4xx (whose +// envelope already explains itself) must not. +func TestRequestLoggerRecords5xxCause(t *testing.T) { + cases := []struct { + name string + err error + wantInLog string + absent bool + }{ + {name: "raw error on 500 is logged", err: errors.New("gitworktree: worktree remove exploded"), wantInLog: "gitworktree: worktree remove exploded"}, + {name: "typed 404 carries no error attr", err: apierr.NotFound("SESSION_NOT_FOUND", "Unknown session"), wantInLog: "error=", absent: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + log := slog.New(slog.NewTextHandler(&buf, nil)) + handler := requestLogger(log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + envelope.WriteError(w, r, tc.err) + })) + + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/api/v1/sessions/x/kill", nil)) + + got := buf.String() + if tc.absent { + if strings.Contains(got, tc.wantInLog) { + t.Fatalf("log line unexpectedly contains %q:\n%s", tc.wantInLog, got) + } + return + } + if !strings.Contains(got, tc.wantInLog) { + t.Fatalf("log line missing %q:\n%s", tc.wantInLog, got) + } + }) + } +} diff --git a/backend/internal/ports/outbound.go b/backend/internal/ports/outbound.go index d10fdff47..5e1cdbaff 100644 --- a/backend/internal/ports/outbound.go +++ b/backend/internal/ports/outbound.go @@ -108,7 +108,7 @@ type Workspace interface { Restore(ctx context.Context, cfg WorkspaceConfig) (WorkspaceInfo, error) } -// Workspace-level sentinels surfaced through Create/Restore so the HTTP layer +// Workspace-level sentinels surfaced through Create/Restore/Destroy so callers // can map them to typed errors rather than collapsing every adapter failure // into an opaque 500. Adapters wrap these via fmt.Errorf("...: %w", sentinel). var ( @@ -118,6 +118,10 @@ var ( // ErrWorkspaceBranchNotFetched reports the requested branch exists nowhere // reachable (no local head, no remote-tracking branch, no tag). ErrWorkspaceBranchNotFetched = errors.New("workspace: branch is not fetched") + // ErrWorkspaceDirty reports Destroy refused to remove a workspace because + // it holds uncommitted changes or untracked files. Teardown is never + // forced; callers treat the workspace as intentionally preserved. + ErrWorkspaceDirty = errors.New("workspace: uncommitted changes present") ) // WorkspaceConfig is the spec for creating or restoring a session's workspace. diff --git a/backend/internal/service/session/service.go b/backend/internal/service/session/service.go index 4d33ae416..51e43396e 100644 --- a/backend/internal/service/session/service.go +++ b/backend/internal/service/session/service.go @@ -40,7 +40,7 @@ type commander interface { Restore(ctx context.Context, id domain.SessionID) (domain.SessionRecord, error) Kill(ctx context.Context, id domain.SessionID) (bool, error) Send(ctx context.Context, id domain.SessionID, message string) error - Cleanup(ctx context.Context, project domain.ProjectID) ([]domain.SessionID, error) + Cleanup(ctx context.Context, project domain.ProjectID) (sessionmanager.CleanupResult, error) RollbackSpawn(ctx context.Context, id domain.SessionID) (deleted, killed bool, err error) } @@ -52,6 +52,19 @@ type RollbackOutcome struct { Killed bool `json:"killed"` } +// CleanupOutcome reports what session cleanup reclaimed and what it preserved. +type CleanupOutcome struct { + Cleaned []domain.SessionID `json:"cleaned"` + Skipped []CleanupSkipped `json:"skipped"` +} + +// CleanupSkipped is one terminal session whose workspace was preserved by +// cleanup (never force-deleted), with the user-facing reason. +type CleanupSkipped struct { + SessionID domain.SessionID `json:"sessionId"` + Reason string `json:"reason"` +} + type scmProvider interface { ParseRepository(remote string) (ports.SCMRepo, bool) FetchPullRequests(ctx context.Context, refs []ports.SCMPRRef) ([]ports.SCMObservation, error) @@ -211,9 +224,21 @@ func (s *Service) Rename(ctx context.Context, id domain.SessionID, displayName s return nil } -// Cleanup delegates terminal workspace cleanup to the internal manager. -func (s *Service) Cleanup(ctx context.Context, project domain.ProjectID) ([]domain.SessionID, error) { - return s.manager.Cleanup(ctx, project) +// Cleanup delegates terminal workspace cleanup to the internal manager and +// reports both reclaimed and preserved (skipped) workspaces. +func (s *Service) Cleanup(ctx context.Context, project domain.ProjectID) (CleanupOutcome, error) { + res, err := s.manager.Cleanup(ctx, project) + if err != nil { + return CleanupOutcome{}, err + } + out := CleanupOutcome{Cleaned: res.Cleaned, Skipped: make([]CleanupSkipped, 0, len(res.Skipped))} + if out.Cleaned == nil { + out.Cleaned = []domain.SessionID{} + } + for _, skip := range res.Skipped { + out.Skipped = append(out.Skipped, CleanupSkipped{SessionID: skip.SessionID, Reason: skip.Reason}) + } + return out, nil } // List returns sessions as enriched display models after applying API filters. diff --git a/backend/internal/service/session/service_test.go b/backend/internal/service/session/service_test.go index d3a7bba22..9af3ceea8 100644 --- a/backend/internal/service/session/service_test.go +++ b/backend/internal/service/session/service_test.go @@ -10,6 +10,7 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apierr" "github.com/aoagents/agent-orchestrator/backend/internal/ports" + sessionmanager "github.com/aoagents/agent-orchestrator/backend/internal/session_manager" ) type fakeStore struct { @@ -144,13 +145,32 @@ func (f *fakeCommander) Kill(_ context.Context, id domain.SessionID) (bool, erro return true, nil } func (f *fakeCommander) Send(context.Context, domain.SessionID, string) error { return nil } -func (f *fakeCommander) Cleanup(context.Context, domain.ProjectID) ([]domain.SessionID, error) { - return nil, nil +func (f *fakeCommander) Cleanup(context.Context, domain.ProjectID) (sessionmanager.CleanupResult, error) { + return sessionmanager.CleanupResult{ + Cleaned: []domain.SessionID{"mer-1"}, + Skipped: []sessionmanager.CleanupSkip{{SessionID: "mer-2", Reason: "workspace has uncommitted changes"}}, + }, nil } func (f *fakeCommander) RollbackSpawn(context.Context, domain.SessionID) (bool, bool, error) { return false, false, nil } +// TestCleanupMapsManagerResult: the service forwards both reclaimed and +// skipped sessions, with non-nil slices so the wire shape stays stable. +func TestCleanupMapsManagerResult(t *testing.T) { + svc := &Service{manager: &fakeCommander{}} + out, err := svc.Cleanup(context.Background(), "mer") + if err != nil { + t.Fatalf("Cleanup: %v", err) + } + if len(out.Cleaned) != 1 || out.Cleaned[0] != "mer-1" { + t.Fatalf("cleaned = %#v", out.Cleaned) + } + if len(out.Skipped) != 1 || out.Skipped[0].SessionID != "mer-2" || out.Skipped[0].Reason != "workspace has uncommitted changes" { + t.Fatalf("skipped = %#v", out.Skipped) + } +} + func TestSpawnOrchestratorCleanKillsActiveOrchestratorsBeforeSpawn(t *testing.T) { st := newFakeStore() st.projects["mer"] = domain.ProjectRecord{ID: "mer"} diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index 4e2fd2e78..6b34f11fd 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -362,7 +362,8 @@ func (m *Manager) RollbackSpawn(ctx context.Context, id domain.SessionID) (delet // Kill records terminal intent with the LCM, then tears down the runtime and // workspace. A workspace teardown refused by the worktree-remove safety -// (uncommitted work) surfaces as an error with freed=false and is never forced. +// (uncommitted work) is never forced: the session still terminates and Kill +// succeeds with freed=false, signalling the workspace was preserved. func (m *Manager) Kill(ctx context.Context, id domain.SessionID) (bool, error) { rec, ok, err := m.store.GetSession(ctx, id) if err != nil { @@ -383,6 +384,9 @@ func (m *Manager) Kill(ctx context.Context, id domain.SessionID) (bool, error) { return false, fmt.Errorf("kill %s: runtime: %w", id, err) } if err := m.workspace.Destroy(ctx, ws); err != nil { + if errors.Is(err, ports.ErrWorkspaceDirty) { + return false, nil + } return false, fmt.Errorf("kill %s: workspace: %w", id, err) } return true, nil @@ -471,14 +475,28 @@ func (m *Manager) Send(ctx context.Context, id domain.SessionID, message string) return nil } +// CleanupSkip reports one terminal session whose workspace was preserved +// rather than reclaimed, and why. +type CleanupSkip struct { + SessionID domain.SessionID + Reason string +} + +// CleanupResult reports what Cleanup reclaimed and what it preserved. +type CleanupResult struct { + Cleaned []domain.SessionID + Skipped []CleanupSkip +} + // Cleanup reclaims the workspaces of terminal sessions in a project. A workspace -// whose teardown is refused (uncommitted work) is skipped, never forced. -func (m *Manager) Cleanup(ctx context.Context, project domain.ProjectID) ([]domain.SessionID, error) { +// whose teardown is refused (uncommitted work) is never forced; it is reported +// in Skipped with the reason so the refusal is visible instead of silent. +func (m *Manager) Cleanup(ctx context.Context, project domain.ProjectID) (CleanupResult, error) { recs, err := m.cleanupRecords(ctx, project) if err != nil { - return nil, fmt.Errorf("cleanup %s: %w", project, err) + return CleanupResult{}, fmt.Errorf("cleanup %s: %w", project, err) } - cleaned := make([]domain.SessionID, 0, len(recs)) + result := CleanupResult{Cleaned: make([]domain.SessionID, 0, len(recs)), Skipped: []CleanupSkip{}} for _, rec := range recs { if !rec.IsTerminated { continue @@ -491,11 +509,28 @@ func (m *Manager) Cleanup(ctx context.Context, project domain.ProjectID) ([]doma _ = m.runtime.Destroy(ctx, h) // best effort; usually already gone } if err := m.workspace.Destroy(ctx, ws); err != nil { - continue // skipped: uncommitted work + if !errors.Is(err, ports.ErrWorkspaceDirty) { + // The public reason stays a fixed string (the raw error carries + // internal filesystem paths); the full cause lands here. + m.logger.Warn("cleanup: workspace teardown failed", "sessionID", rec.ID, "path", ws.Path, "error", err) + } + result.Skipped = append(result.Skipped, CleanupSkip{SessionID: rec.ID, Reason: cleanupSkipReason(err)}) + continue } - cleaned = append(cleaned, rec.ID) + result.Cleaned = append(result.Cleaned, rec.ID) } - return cleaned, nil + return result, nil +} + +// cleanupSkipReason renders a workspace teardown refusal as a short +// user-facing reason for the cleanup report. Deliberately not the raw error: +// it flows to the API response and CLI output, and teardown errors embed +// internal filesystem paths. +func cleanupSkipReason(err error) string { + if errors.Is(err, ports.ErrWorkspaceDirty) { + return "workspace has uncommitted changes" + } + return "workspace teardown failed" } func (m *Manager) cleanupRecords(ctx context.Context, project domain.ProjectID) ([]domain.SessionRecord, error) { diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index b15cd5968..1f742e670 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -355,6 +355,41 @@ func TestKill_RefusesIncompleteHandle(t *testing.T) { t.Fatalf("want ErrIncompleteHandle, got %v", err) } } + +// TestKill_DirtyWorkspaceTerminatesAndPreserves: a workspace teardown refused +// because of uncommitted work must NOT fail the kill — the session terminates, +// the runtime is gone, and freed=false reports the preserved worktree. This is +// the normal path for any session with in-progress changes, so an error here +// would turn every such kill into a 500. +func TestKill_DirtyWorkspaceTerminatesAndPreserves(t *testing.T) { + m, st, rt, ws := newManager() + st.sessions["mer-1"] = mkLive("mer-1") + ws.destroyErr = fmt.Errorf("gitworktree: refusing to remove: %w", ports.ErrWorkspaceDirty) + freed, err := m.Kill(ctx, "mer-1") + if err != nil { + t.Fatalf("kill dirty workspace err = %v, want nil", err) + } + if freed { + t.Fatal("freed = true, want false for preserved workspace") + } + if rt.destroyed != 1 { + t.Fatal("runtime should be destroyed") + } + if !st.sessions["mer-1"].IsTerminated { + t.Fatal("session should be terminated") + } +} + +// TestKill_OtherWorkspaceErrorStillFails: only the typed dirty refusal is a +// success-with-preserved-workspace; any other teardown failure keeps erroring. +func TestKill_OtherWorkspaceErrorStillFails(t *testing.T) { + m, st, _, ws := newManager() + st.sessions["mer-1"] = mkLive("mer-1") + ws.destroyErr = errors.New("disk on fire") + if _, err := m.Kill(ctx, "mer-1"); err == nil || !strings.Contains(err.Error(), "disk on fire") { + t.Fatalf("kill err = %v, want workspace error surfaced", err) + } +} func TestRestore_ReopensTerminal(t *testing.T) { m, st, rt, _ := newManager() seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", Branch: "b", AgentSessionID: "agent-x"}) @@ -396,18 +431,58 @@ func TestCleanup_ReclaimsTerminalWorkspaces(t *testing.T) { m, st, _, ws := newManager() seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1"}) st.sessions["mer-2"] = mkLive("mer-2") - cleaned, err := m.Cleanup(ctx, "mer") + res, err := m.Cleanup(ctx, "mer") if err != nil { t.Fatal(err) } - if len(cleaned) != 1 || cleaned[0] != "mer-1" { - t.Fatalf("got %v", cleaned) + if len(res.Cleaned) != 1 || res.Cleaned[0] != "mer-1" { + t.Fatalf("got %v", res.Cleaned) + } + if len(res.Skipped) != 0 { + t.Fatalf("skipped = %v, want none", res.Skipped) } if ws.destroyed != 1 { t.Fatal("live workspace must not be destroyed") } } +// TestCleanup_ReportsSkippedWorkspaces: a refused teardown must be visible in +// the result with a reason — a silent skip leaves users staring at +// "Would clean N … 0 sessions cleaned" with no explanation. +func TestCleanup_ReportsSkippedWorkspaces(t *testing.T) { + m, st, _, ws := newManager() + seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1"}) + ws.destroyErr = fmt.Errorf("gitworktree: refusing to remove: %w", ports.ErrWorkspaceDirty) + res, err := m.Cleanup(ctx, "mer") + if err != nil { + t.Fatal(err) + } + if len(res.Cleaned) != 0 { + t.Fatalf("cleaned = %v, want none", res.Cleaned) + } + if len(res.Skipped) != 1 || res.Skipped[0].SessionID != "mer-1" { + t.Fatalf("skipped = %v, want mer-1", res.Skipped) + } + if res.Skipped[0].Reason != "workspace has uncommitted changes" { + t.Fatalf("reason = %q", res.Skipped[0].Reason) + } + + // A non-dirty teardown failure is reported too — but with a fixed public + // reason: the raw cause carries internal filesystem paths and belongs in + // the server log, not the API response. + ws.destroyErr = errors.New("disk on fire") + res, err = m.Cleanup(ctx, "mer") + if err != nil { + t.Fatal(err) + } + if len(res.Skipped) != 1 || res.Skipped[0].Reason != "workspace teardown failed" { + t.Fatalf("skipped = %v, want fixed teardown-failed reason", res.Skipped) + } + if strings.Contains(res.Skipped[0].Reason, "disk on fire") { + t.Fatalf("raw internal error leaked into public reason: %q", res.Skipped[0].Reason) + } +} + func TestSpawn_DefaultsBranchFromSessionID(t *testing.T) { m, st, _, _ := newManager() s, err := m.Spawn(ctx, ports.SpawnConfig{ProjectID: "mer", Kind: domain.KindWorker}) diff --git a/docs/agent/README.md b/docs/agent/README.md index efc39c401..89385c054 100644 --- a/docs/agent/README.md +++ b/docs/agent/README.md @@ -40,6 +40,7 @@ Implementation layout: - Agent-specific hook installation should live beside the agent adapter in `backend/internal/adapters/agent//hooks.go`; the hook commands are defined in code, not embedded template files. - Launch, restore, and session-info behavior can stay in the main agent implementation unless the file grows enough to justify another split. +- Every file an adapter writes into the session worktree must be covered by a sibling self-ignoring `.gitignore` written via `hookutil.EnsureWorkspaceGitignore`. Hook files are untracked, and `git worktree remove` (never run with `--force`) refuses on any untracked file — an uncovered hook file makes every session workspace permanently undeletable. The registry conformance test (`registry.TestGetAgentHooksFootprintIsGitignored`) enforces this for all adapters. ## Metadata Keys diff --git a/frontend/src/api/schema.ts b/frontend/src/api/schema.ts index 8acb85eef..34a782c74 100644 --- a/frontend/src/api/schema.ts +++ b/frontend/src/api/schema.ts @@ -353,6 +353,11 @@ export interface components { CleanupSessionsResponse: { cleaned: string[]; ok: boolean; + skipped: components["schemas"]["CleanupSkippedSession"][]; + }; + CleanupSkippedSession: { + reason: string; + sessionId: string; }; DegradedProject: { id: string;