diff --git a/internal/application/tickets/orchestrator.go b/internal/application/tickets/orchestrator.go index 00c3ef7..0f42eae 100644 --- a/internal/application/tickets/orchestrator.go +++ b/internal/application/tickets/orchestrator.go @@ -68,12 +68,8 @@ func (o *Orchestrator) StartFlow(ctx context.Context, ticketNumber string) error } state, loadErr := o.Store.LoadState(ticketNumber) - if os.IsNotExist(loadErr) { + if errors.Is(loadErr, os.ErrNotExist) { state = workflowstate.New(ticketNumber) - saveErr := o.Store.SaveState(ticketNumber, state) - if saveErr != nil { - return fmt.Errorf("save initial ticket state: %w", saveErr) - } } else if loadErr != nil { return fmt.Errorf("load ticket state: %w", loadErr) } @@ -154,12 +150,8 @@ func (o *Orchestrator) MoveToState(ctx context.Context, ticketNumber, target str } state, loadErr := o.Store.LoadState(ticketNumber) - if os.IsNotExist(loadErr) { + if errors.Is(loadErr, os.ErrNotExist) { state = workflowstate.New(ticketNumber) - saveErr := o.Store.SaveState(ticketNumber, state) - if saveErr != nil { - return fmt.Errorf("save initial ticket state: %w", saveErr) - } } else if loadErr != nil { return fmt.Errorf("load ticket state: %w", loadErr) } diff --git a/internal/application/tickets/orchestrator_test.go b/internal/application/tickets/orchestrator_test.go index 646832f..61564e7 100644 --- a/internal/application/tickets/orchestrator_test.go +++ b/internal/application/tickets/orchestrator_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -12,6 +13,7 @@ import ( "github.com/Neokil/AutoPR/internal/config" workflowstate "github.com/Neokil/AutoPR/internal/domain/workflowstate" "github.com/Neokil/AutoPR/internal/providers" + statepkg "github.com/Neokil/AutoPR/internal/state" ) // ── in-memory mocks ──────────────────────────────────────────────────────── @@ -133,6 +135,31 @@ func newOrchestrator(repoRoot string, store *memStore, prov *mockProvider) *tick ) } +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_AUTHOR_NAME=Test User", + "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=Test User", + "GIT_COMMITTER_EMAIL=test@example.com", + ) + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s failed: %v\n%s", strings.Join(args, " "), err, string(output)) + } +} + +func setupGitRepo(t *testing.T, root string) { + t.Helper() + runGit(t, root, "init", "-b", "main") + runGit(t, root, "config", "user.name", "Test User") + runGit(t, root, "config", "user.email", "test@example.com") + runGit(t, root, "add", ".") + runGit(t, root, "commit", "-m", "initial commit") +} + // ── StartFlow ───────────────────────────────────────────────────────────── func TestStartFlow_newTicket_endsWaiting(t *testing.T) { @@ -213,6 +240,41 @@ func TestStartFlow_providerError_setsFailedStatus(t *testing.T) { } } +func TestStartFlow_newTicketPersistsStateInWorktreeOnly(t *testing.T) { + t.Parallel() + + root := setupRepo(t, minimalWorkflow, "Investigate this ticket.") + setupGitRepo(t, root) + store := statepkg.NewStore(root, ".auto-pr-state") + prov := &mockProvider{result: providers.ExecuteResult{RawOutput: "analysis done"}} + orch := tickets.NewWithStore( + config.Config{StateDirName: ".auto-pr-state", BaseBranch: "main"}, + root, + store, + prov, + ) + + err := orch.StartFlow(context.Background(), "42") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + worktreeStatePath := filepath.Join(root, ".auto-pr-state", "worktrees", "42", ".auto-pr", statepkg.StateFileName) + if _, statErr := os.Stat(worktreeStatePath); statErr != nil { + t.Fatalf("expected worktree-backed state file at %s: %v", worktreeStatePath, statErr) + } + + legacyStatePath := filepath.Join(root, ".auto-pr-state", "42", statepkg.StateFileName) + if _, statErr := os.Stat(legacyStatePath); !os.IsNotExist(statErr) { + t.Fatalf("expected no legacy state file at %s, got err=%v", legacyStatePath, statErr) + } + + legacyDirPath := filepath.Join(root, ".auto-pr-state", "42") + if _, statErr := os.Stat(legacyDirPath); !os.IsNotExist(statErr) { + t.Fatalf("expected no legacy ticket dir at %s, got err=%v", legacyDirPath, statErr) + } +} + func TestDiscoverTickets_persistsLogsUnderUserHome(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) diff --git a/internal/server/job_failures.go b/internal/server/job_failures.go index 9c661b9..f42c55f 100644 --- a/internal/server/job_failures.go +++ b/internal/server/job_failures.go @@ -1,6 +1,7 @@ package server import ( + "errors" "fmt" "os" "strings" @@ -16,10 +17,11 @@ func (s *server) persistTicketFailure(repoID, repoRoot, ticket string, repoRt *r ticketState, err := repoRt.store.LoadState(ticket) if err != nil { - if !os.IsNotExist(err) { + if !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("load ticket state: %w", err) } - ticketState = workflowstate.New(ticket) + + return s.meta.DeleteTicket(repoID, ticket) } msg := strings.TrimSpace(cause.Error()) diff --git a/internal/server/ticket_lifecycle_test.go b/internal/server/ticket_lifecycle_test.go new file mode 100644 index 0000000..b3c00a4 --- /dev/null +++ b/internal/server/ticket_lifecycle_test.go @@ -0,0 +1,94 @@ +package server + +import ( + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/Neokil/AutoPR/internal/serverstate" + "github.com/Neokil/AutoPR/internal/state" +) + +func newTestServer(t *testing.T, repoRoot string) (*server, *state.Store, string) { + t.Helper() + + meta, err := serverstate.NewStore(filepath.Join(t.TempDir(), "server-state.json")) + if err != nil { + t.Fatalf("NewStore() error = %v", err) + } + + store := state.NewStore(repoRoot, ".auto-pr-state") + repoID := "repo-1" + + return &server{ + meta: meta, + runtimes: map[string]*repoRuntime{ + repoRoot: { + repoRoot: repoRoot, + store: store, + }, + }, + }, store, repoID +} + +func TestEnsureQueuedTicketDoesNotPersistFreshState(t *testing.T) { + t.Parallel() + + repoRoot := t.TempDir() + srv, store, repoID := newTestServer(t, repoRoot) + + err := srv.ensureQueuedTicket(repoID, repoRoot, "GH-12") + if err != nil { + t.Fatalf("ensureQueuedTicket() error = %v", err) + } + + _, loadErr := store.LoadState("GH-12") + if !errors.Is(loadErr, os.ErrNotExist) { + t.Fatalf("expected no persisted state, got err=%v", loadErr) + } + + _, statErr := os.Stat(store.TicketDir("GH-12")) + if !os.IsNotExist(statErr) { + t.Fatalf("expected no legacy ticket dir, got err=%v", statErr) + } +} + +func TestPersistTicketFailureWithoutStateDeletesMetadataOnly(t *testing.T) { + t.Parallel() + + repoRoot := t.TempDir() + srv, store, repoID := newTestServer(t, repoRoot) + err := srv.meta.UpsertTicket(serverstate.TicketRecord{ + RepoID: repoID, + RepoPath: repoRoot, + TicketNumber: "GH-12", + Status: "queued", + UpdatedAt: time.Now().UTC(), + }) + if err != nil { + t.Fatalf("UpsertTicket() error = %v", err) + } + + err = srv.persistTicketFailure(repoID, repoRoot, "GH-12", &repoRuntime{repoRoot: repoRoot, store: store}, queuedJob{ + record: serverstate.JobRecord{Action: jobRun}, + }, errors.New("worktree creation failed")) + if err != nil { + t.Fatalf("persistTicketFailure() error = %v", err) + } + + _, loadErr := store.LoadState("GH-12") + if !errors.Is(loadErr, os.ErrNotExist) { + t.Fatalf("expected no persisted state, got err=%v", loadErr) + } + + _, statErr := os.Stat(store.TicketDir("GH-12")) + if !os.IsNotExist(statErr) { + t.Fatalf("expected no legacy ticket dir, got err=%v", statErr) + } + + if records := srv.meta.ListTickets(repoID); len(records) != 0 { + t.Fatalf("expected metadata to be removed, got %#v", records) + } +} diff --git a/internal/server/ticket_sync.go b/internal/server/ticket_sync.go index 9f9e840..2bfd1d6 100644 --- a/internal/server/ticket_sync.go +++ b/internal/server/ticket_sync.go @@ -24,13 +24,7 @@ func (s *server) ensureQueuedTicket(repoID, repoRoot, ticket string) error { return fmt.Errorf("load ticket state: %w", loadErr) } - st := workflowstate.New(ticket) - err = repoRt.store.SaveState(ticket, st) - if err != nil { - return fmt.Errorf("save initial ticket state: %w", err) - } - - return s.syncTicketFromRepo(repoID, repoRoot, ticket, repoRt, true) + return nil } func (s *server) syncTicketFromRepo(repoID, repoRoot, ticket string, rt *repoRuntime, emitEvent bool) error { diff --git a/internal/state/store.go b/internal/state/store.go index f9912a3..ae88459 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -30,13 +30,13 @@ func NewStore(repoRoot, stateDirName string) *Store { } } -// TicketDir returns the directory used to store state for the given ticket before a worktree exists. +// TicketDir returns the legacy pre-worktree directory for the given ticket. func (s *Store) TicketDir(ticketNumber string) string { return filepath.Join(s.StateRoot, ticketNumber) } // LoadState reads the persisted state for the ticket, preferring the worktree location -// when it exists and falling back to the pre-worktree state directory. +// and falling back to the legacy pre-worktree state directory for compatibility. func (s *Store) LoadState(ticketNumber string) (workflowstate.State, error) { // Prefer the worktree location when it exists. wtStatePath := filepath.Join(s.worktreePath(ticketNumber), ".auto-pr", StateFileName) @@ -54,7 +54,8 @@ func (s *Store) LoadState(ticketNumber string) (workflowstate.State, error) { } // SaveState persists st, writing to the worktree location once a worktree exists -// and removing the pre-worktree copy to keep a single source of truth. +// and removing the legacy copy to keep a single source of truth. +// Callers that save before a worktree exists still write to the legacy location. func (s *Store) SaveState(ticketNumber string, state workflowstate.State) error { state.Touch() data, err := json.MarshalIndent(state, "", " ") @@ -101,7 +102,7 @@ func (s *Store) ListTicketDirs() ([]string, error) { seen := map[string]struct{}{} var out []string - // Tickets with state still in the pre-worktree location. + // Tickets with state still in the legacy pre-worktree location. entries, err := os.ReadDir(s.StateRoot) if err != nil && !os.IsNotExist(err) { return nil, fmt.Errorf("read state root: %w", err) @@ -141,7 +142,7 @@ func (s *Store) ListTicketDirs() ([]string, error) { return out, nil } -// RemoveTicketDir deletes the pre-worktree state directory for the given ticket. +// RemoveTicketDir deletes the legacy pre-worktree state directory for the given ticket. func (s *Store) RemoveTicketDir(ticketNumber string) error { err := os.RemoveAll(s.TicketDir(ticketNumber)) if err != nil { diff --git a/internal/state/store_test.go b/internal/state/store_test.go index 522da8d..2fdb986 100644 --- a/internal/state/store_test.go +++ b/internal/state/store_test.go @@ -1,6 +1,7 @@ package state_test import ( + "encoding/json" "os" "path/filepath" "slices" @@ -10,22 +11,61 @@ import ( "github.com/Neokil/AutoPR/internal/state" ) -func TestSaveStateCreatesPreWorktreeStateFile(t *testing.T) { +func writeLegacyState(t *testing.T, store *state.Store, ticketState workflowstate.State) { + t.Helper() + err := os.MkdirAll(store.TicketDir(ticketState.TicketNumber), 0o755) + if err != nil { + t.Fatalf("MkdirAll() error = %v", err) + } + data, err := json.MarshalIndent(ticketState, "", " ") + if err != nil { + t.Fatalf("MarshalIndent() error = %v", err) + } + err = os.WriteFile(filepath.Join(store.TicketDir(ticketState.TicketNumber), state.StateFileName), data, 0o644) + if err != nil { + t.Fatalf("WriteFile() error = %v", err) + } +} + +func TestLoadStateFallsBackToLegacyStateFile(t *testing.T) { t.Parallel() repoRoot := t.TempDir() store := state.NewStore(repoRoot, ".auto-pr-state") ticketState := workflowstate.New("GH-12") + writeLegacyState(t, store, ticketState) + + got, err := store.LoadState(ticketState.TicketNumber) + if err != nil { + t.Fatalf("LoadState() error = %v", err) + } + if got.TicketNumber != ticketState.TicketNumber { + t.Fatalf("expected ticket %q, got %q", ticketState.TicketNumber, got.TicketNumber) + } +} + +func TestSaveStateWritesWorktreeStateWithoutLegacyDir(t *testing.T) { + t.Parallel() + + repoRoot := t.TempDir() + store := state.NewStore(repoRoot, ".auto-pr-state") + ticketState := workflowstate.New("GH-12") + ticketState.WorktreePath = filepath.Join(repoRoot, ".auto-pr-state", "worktrees", ticketState.TicketNumber) err := store.SaveState(ticketState.TicketNumber, ticketState) if err != nil { t.Fatalf("SaveState() error = %v", err) } - statePath := filepath.Join(store.TicketDir(ticketState.TicketNumber), state.StateFileName) - _, statErr := os.Stat(statePath) - if statErr != nil { - t.Fatalf("expected pre-worktree state file at %s: %v", statePath, statErr) + worktreeStatePath := filepath.Join(ticketState.WorktreePath, ".auto-pr", state.StateFileName) + _, worktreeStateErr := os.Stat(worktreeStatePath) + if worktreeStateErr != nil { + t.Fatalf("expected worktree state file at %s: %v", worktreeStatePath, worktreeStateErr) + } + + _, legacyDirErr := os.Stat(store.TicketDir(ticketState.TicketNumber)) + if !os.IsNotExist(legacyDirErr) { + t.Fatalf("expected no legacy ticket dir, got err=%v", legacyDirErr) } } @@ -36,13 +76,10 @@ func TestSaveStateMigratesStateAndRemovesEmptyLegacyDir(t *testing.T) { store := state.NewStore(repoRoot, ".auto-pr-state") ticketState := workflowstate.New("GH-12") - err := store.SaveState(ticketState.TicketNumber, ticketState) - if err != nil { - t.Fatalf("initial SaveState() error = %v", err) - } + writeLegacyState(t, store, ticketState) ticketState.WorktreePath = filepath.Join(repoRoot, ".auto-pr-state", "worktrees", ticketState.TicketNumber) - err = store.SaveState(ticketState.TicketNumber, ticketState) + err := store.SaveState(ticketState.TicketNumber, ticketState) if err != nil { t.Fatalf("migrating SaveState() error = %v", err) } @@ -72,13 +109,10 @@ func TestSaveStateKeepsLegacyDirWhenItContainsOtherFiles(t *testing.T) { store := state.NewStore(repoRoot, ".auto-pr-state") ticketState := workflowstate.New("GH-12") - err := store.SaveState(ticketState.TicketNumber, ticketState) - if err != nil { - t.Fatalf("initial SaveState() error = %v", err) - } + writeLegacyState(t, store, ticketState) notePath := filepath.Join(store.TicketDir(ticketState.TicketNumber), "note.txt") - err = os.WriteFile(notePath, []byte("keep me\n"), 0o644) + err := os.WriteFile(notePath, []byte("keep me\n"), 0o644) if err != nil { t.Fatalf("WriteFile() error = %v", err) } @@ -102,13 +136,10 @@ func TestListTicketDirsIncludesMigratedWorktreeState(t *testing.T) { store := state.NewStore(repoRoot, ".auto-pr-state") ticketState := workflowstate.New("GH-12") - err := store.SaveState(ticketState.TicketNumber, ticketState) - if err != nil { - t.Fatalf("initial SaveState() error = %v", err) - } + writeLegacyState(t, store, ticketState) ticketState.WorktreePath = filepath.Join(repoRoot, ".auto-pr-state", "worktrees", ticketState.TicketNumber) - err = store.SaveState(ticketState.TicketNumber, ticketState) + err := store.SaveState(ticketState.TicketNumber, ticketState) if err != nil { t.Fatalf("migrating SaveState() error = %v", err) }