diff --git a/apps/backend/cmd/kandev/main.go b/apps/backend/cmd/kandev/main.go index 14262f34b7..fa30703824 100644 --- a/apps/backend/cmd/kandev/main.go +++ b/apps/backend/cmd/kandev/main.go @@ -423,7 +423,7 @@ func startAgentInfrastructure( // Wire GitHub service into orchestrator for PR auto-detection on push if services.GitHub != nil { orchestratorSvc.SetGitHubService(services.GitHub) - services.GitHub.SetTaskDeleter(services.Task) + services.GitHub.SetTaskDeleter(&taskDeleterAdapter{svc: services.Task}) services.GitHub.SetTaskSessionChecker(&taskSessionCheckerAdapter{repo: repos.Task}) log.Info("GitHub service configured for orchestrator (PR auto-detection enabled)") diff --git a/apps/backend/cmd/kandev/turn_adapters.go b/apps/backend/cmd/kandev/turn_adapters.go index 51421563fb..2491e9625c 100644 --- a/apps/backend/cmd/kandev/turn_adapters.go +++ b/apps/backend/cmd/kandev/turn_adapters.go @@ -2,8 +2,12 @@ package main import ( "context" + "errors" + "fmt" + "github.com/kandev/kandev/internal/github" "github.com/kandev/kandev/internal/task/models" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" taskservice "github.com/kandev/kandev/internal/task/service" ) @@ -80,3 +84,22 @@ func metaFlag(meta map[string]interface{}, key string) bool { v, ok := meta[key].(bool) return ok && v } + +// taskDeleterAdapter satisfies github.TaskDeleter and translates the task +// repository's ErrTaskNotFound sentinel to github.ErrTaskNotFound so the +// github cleanup paths can classify the "already gone" case via errors.Is +// without importing the task repository's package. +type taskDeleterAdapter struct { + svc *taskservice.Service +} + +func (a *taskDeleterAdapter) DeleteTask(ctx context.Context, taskID string) error { + err := a.svc.DeleteTask(ctx, taskID) + if err == nil { + return nil + } + if errors.Is(err, taskrepo.ErrTaskNotFound) { + return fmt.Errorf("%w: %w", github.ErrTaskNotFound, err) + } + return err +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/execution_store.go b/apps/backend/internal/agent/runtime/lifecycle/execution_store.go index 90418fb12e..74210339ab 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/execution_store.go +++ b/apps/backend/internal/agent/runtime/lifecycle/execution_store.go @@ -21,6 +21,21 @@ var ErrExecutionNotFound = errors.New("execution not found") // adding a replacement. var ErrExecutionAlreadyExistsForSession = errors.New("execution already exists for session") +// ErrAgentAlreadyRunning is returned by LaunchAgent when an agent execution is +// already tracked for the requested session. The error fires both when the +// execution is live (a concurrent caller raced us) and when it is stale (a +// prior registration whose process never started or exited without cleanup); +// callers must probe IsAgentRunningForSession to decide. Wrapped at producer +// sites with %w so callers can use errors.Is rather than string-match the +// surrounding context. +var ErrAgentAlreadyRunning = errors.New("session already has an agent running") + +// ErrAgentReported is returned when the agent subprocess publishes an error +// event during a prompt turn. Distinguished from infrastructure errors +// (network, timeout) so callers can suppress duplicate error-message creation +// — the agent failure path already records the error in the session. +var ErrAgentReported = errors.New("agent error") + // ExecutionStore provides thread-safe storage and retrieval of agent executions. // It maintains three indexes for efficient lookup by execution ID, session ID, and container ID. type ExecutionStore struct { diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go b/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go index 41aaaeeb0e..fd7d398f38 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go @@ -231,7 +231,7 @@ func (m *Manager) GetExecutionIDForSession(_ context.Context, sessionID string) if execution, exists := m.executionStore.GetBySessionID(sessionID); exists { return execution.ID, nil } - return "", fmt.Errorf("no execution found for session %s", sessionID) + return "", fmt.Errorf("%w: %s", ErrNoExecutionForSession, sessionID) } // EnsurePassthroughExecution ensures an execution exists for a passthrough session diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go index cee43c8928..132cc62857 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go @@ -806,7 +806,7 @@ func (m *Manager) launchInternal(ctx context.Context, req *LaunchRequest) (*Agen if existingExecution.AgentCommand == "" { return existingExecution, nil } - return nil, fmt.Errorf("session %q already has an agent running (execution: %s)", req.SessionID, existingExecution.ID) + return nil, fmt.Errorf("%w: session %q (execution: %s)", ErrAgentAlreadyRunning, req.SessionID, existingExecution.ID) } } @@ -928,7 +928,7 @@ func (m *Manager) registerAndPublishExecution( if addErr := m.executionStore.Add(execution); addErr != nil { if errors.Is(addErr, ErrExecutionAlreadyExistsForSession) { m.rollbackRacedExecution(ctx, rt, execInstance, execution) - return fmt.Errorf("session %q already has an agent running (race resolved during register)", sessionID) + return fmt.Errorf("%w: session %q (race resolved during register)", ErrAgentAlreadyRunning, sessionID) } return fmt.Errorf("failed to register execution: %w", addErr) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go b/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go index 6b98b0cf9a..ab4a9216cd 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go @@ -613,7 +613,7 @@ func (m *Manager) restartPassthroughProcess(ctx context.Context, execution *Agen func (m *Manager) ResumePassthroughSession(ctx context.Context, sessionID string) error { execution, exists := m.executionStore.GetBySessionID(sessionID) if !exists { - return fmt.Errorf("no execution found for session: %s", sessionID) + return fmt.Errorf("%w: %s", ErrNoExecutionForSession, sessionID) } resolved, err := m.resolvePassthroughAgent(ctx, execution) diff --git a/apps/backend/internal/agent/runtime/lifecycle/process_runner.go b/apps/backend/internal/agent/runtime/lifecycle/process_runner.go index be7d98d12c..c00ba35f12 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/process_runner.go +++ b/apps/backend/internal/agent/runtime/lifecycle/process_runner.go @@ -24,7 +24,7 @@ func (m *Manager) StartProcess(ctx context.Context, req StartProcessRequest) (*a } execution, ok := m.executionStore.GetBySessionID(req.SessionID) if !ok { - return nil, fmt.Errorf("no execution found for session %s", req.SessionID) + return nil, fmt.Errorf("%w: %s", ErrNoExecutionForSession, req.SessionID) } client := execution.GetAgentCtlClient() if client == nil { @@ -48,7 +48,7 @@ func (m *Manager) WaitForAgentctlReadyForSession(ctx context.Context, sessionID } execution, ok := m.executionStore.GetBySessionID(sessionID) if !ok { - return fmt.Errorf("no execution found for session %s", sessionID) + return fmt.Errorf("%w: %s", ErrNoExecutionForSession, sessionID) } client := execution.GetAgentCtlClient() if client == nil { @@ -83,7 +83,7 @@ func (m *Manager) StopProcessForSession(ctx context.Context, sessionID, processI } execution, ok := m.executionStore.GetBySessionID(sessionID) if !ok { - return fmt.Errorf("no execution found for session %s", sessionID) + return fmt.Errorf("%w: %s", ErrNoExecutionForSession, sessionID) } client := execution.GetAgentCtlClient() if client == nil { @@ -98,7 +98,7 @@ func (m *Manager) ListProcesses(ctx context.Context, sessionID string) ([]agentc } execution, ok := m.executionStore.GetBySessionID(sessionID) if !ok { - return nil, fmt.Errorf("no execution found for session %s", sessionID) + return nil, fmt.Errorf("%w: %s", ErrNoExecutionForSession, sessionID) } client := execution.GetAgentCtlClient() if client == nil { diff --git a/apps/backend/internal/agent/runtime/lifecycle/session.go b/apps/backend/internal/agent/runtime/lifecycle/session.go index 200b136e78..7133c8eb4d 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/session.go +++ b/apps/backend/internal/agent/runtime/lifecycle/session.go @@ -481,9 +481,9 @@ func (sm *SessionManager) waitForPromptDone(ctx context.Context, execution *Agen // skip the REVIEW task-state transition — the user is cancelling, not // hitting a real agent failure. if strings.HasPrefix(signal.Error, "cancel escalated") { - return nil, fmt.Errorf("agent error: %s: %w", signal.Error, ErrCancelEscalated) + return nil, fmt.Errorf("%w: %s: %w", ErrAgentReported, signal.Error, ErrCancelEscalated) } - return nil, fmt.Errorf("agent error: %s", signal.Error) + return nil, fmt.Errorf("%w: %s", ErrAgentReported, signal.Error) } // Peek at buffer for return value diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index e9045d5288..f4b37dc04b 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -95,8 +95,7 @@ func (c *Controller) httpConfigureToken(ctx *gin.Context) { } if err := c.service.ConfigureToken(ctx.Request.Context(), req.Token); err != nil { - // Check if it's a validation error (invalid token) - if strings.Contains(err.Error(), "invalid token") { + if errors.Is(err, ErrInvalidToken) { ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } diff --git a/apps/backend/internal/github/errors.go b/apps/backend/internal/github/errors.go new file mode 100644 index 0000000000..4c8e85dc35 --- /dev/null +++ b/apps/backend/internal/github/errors.go @@ -0,0 +1,27 @@ +package github + +import "errors" + +// ErrInvalidPRURL signals that a caller-supplied PR URL could not be parsed. +// Used by AssociateExistingPRByURL so HTTP callers can translate the failure +// into a 400 instead of a generic 500. +var ErrInvalidPRURL = errors.New("invalid PR URL") + +// ErrTaskNotFound is the sentinel that cleanup paths check to distinguish +// "the task is already gone — fine, mop up the dedup row" from a real +// upstream failure. Adapter implementations of TaskDeleter wrap this when +// the task domain reports a missing row so the github layer can recognize +// the case without string-matching the underlying error message. +var ErrTaskNotFound = errors.New("github: task not found for cleanup") + +// ErrSelfApprove is returned by SubmitReview when the authenticated user +// attempts to APPROVE their own PR. GitHub rejects this with a 422; we +// catch it server-side so the UI sees a clean, typed error rather than a +// generic upstream failure when the frontend's visibility guard is bypassed. +var ErrSelfApprove = errors.New("cannot approve your own pull request") + +// ErrInvalidToken is returned by ConfigureToken when the supplied PAT fails +// validation against the GitHub API. Wrapped around the underlying client +// error so HTTP callers can distinguish a validation failure (HTTP 400) +// from a secret-store write failure (HTTP 500). +var ErrInvalidToken = errors.New("invalid token") diff --git a/apps/backend/internal/github/errors_test.go b/apps/backend/internal/github/errors_test.go new file mode 100644 index 0000000000..37fe2335f4 --- /dev/null +++ b/apps/backend/internal/github/errors_test.go @@ -0,0 +1,55 @@ +package github + +import ( + "context" + "errors" + "fmt" + "testing" +) + +// TestErrorsAreClassifiable verifies that the sentinels declared in errors.go +// remain reachable through errors.Is even when callers wrap them, which is +// the contract HTTP handlers and cleanup paths rely on. +func TestErrorsAreClassifiable(t *testing.T) { + t.Run("isTaskNotFound recognizes sentinel-wrapped errors", func(t *testing.T) { + wrapped := fmt.Errorf("%w: task task-1", ErrTaskNotFound) + if !isTaskNotFound(wrapped) { + t.Errorf("expected sentinel-wrapped error to be classified as task-not-found") + } + if isTaskNotFound(errors.New("something else not found")) { + t.Errorf("expected unrelated 'not found' string to no longer match") + } + if isTaskNotFound(nil) { + t.Errorf("nil error must not classify as not-found") + } + }) + + t.Run("AssociateExistingPRByURL wraps ErrInvalidPRURL on malformed input", func(t *testing.T) { + // Service with a stub client so the client-nil guard does not fire + // before parsePRURL runs. + svc := &Service{client: NewMockClient()} + _, err := svc.AssociateExistingPRByURL(context.Background(), "t1", "", "not a pr url") + if err == nil { + t.Fatal("expected error for malformed PR URL") + } + if !errors.Is(err, ErrInvalidPRURL) { + t.Errorf("error not classifiable as ErrInvalidPRURL: %v", err) + } + }) + + t.Run("ErrInvalidToken survives the ConfigureToken wrap pattern", func(t *testing.T) { + // ConfigureToken wraps the underlying PAT-client error with + // `fmt.Errorf("%w: %w", ErrInvalidToken, err)`. Verify the wrap + // pattern preserves errors.Is reachability for both the sentinel + // and the inner cause. Exercising the full ConfigureToken would + // require a real HTTP roundtrip — covered by integration tests. + inner := errors.New("401 Unauthorized") + wrapped := fmt.Errorf("%w: %w", ErrInvalidToken, inner) + if !errors.Is(wrapped, ErrInvalidToken) { + t.Errorf("wrapped error not classifiable as ErrInvalidToken: %v", wrapped) + } + if !errors.Is(wrapped, inner) { + t.Errorf("wrapped error did not preserve inner cause in the chain: %v", wrapped) + } + }) +} diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index bba3aeffbd..e27b5f0c70 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -28,11 +28,6 @@ const ( AuthMethodPAT = "pat" ) -// ErrInvalidPRURL signals that a caller-supplied PR URL could not be parsed. -// Used by AssociateExistingPRByURL so HTTP callers can translate the failure -// into a 400 instead of a generic 500. -var ErrInvalidPRURL = errors.New("invalid PR URL") - // defaultBranchMain and defaultBranchMaster are the conventional default branch // names sorted to the top of branch pickers. const ( @@ -40,19 +35,6 @@ const ( defaultBranchMaster = "master" ) -// ErrTaskNotFound is the sentinel cleanup paths check for to distinguish -// "the task is already gone — fine, mop up the dedup row" from a real -// upstream failure. Adapter implementations of TaskDeleter wrap this when -// the task domain reports a missing row so the github layer can recognize -// the case without string-matching the underlying error message. -var ErrTaskNotFound = errors.New("github: task not found for cleanup") - -// ErrSelfApprove is returned by SubmitReview when the authenticated user -// attempts to APPROVE their own PR. GitHub rejects this with a 422; we -// catch it server-side so the UI sees a clean, typed error rather than a -// generic upstream failure when the frontend's visibility guard is bypassed. -var ErrSelfApprove = errors.New("cannot approve your own pull request") - // reviewEventApprove is the GitHub Reviews API event value for a positive // review. Extracted because it appears in the controller validator, the // service's self-approval guard, and tests. @@ -65,18 +47,11 @@ type TaskDeleter interface { DeleteTask(ctx context.Context, taskID string) error } -// isTaskNotFound recognizes both the typed sentinel and the legacy -// "not found" substring used by older adapters that haven't migrated yet. -// String matching stays as a fallback so this PR doesn't regress installs -// where the adapter wasn't updated in lockstep. +// isTaskNotFound reports whether an error from TaskDeleter signals the task +// was already gone. Adapters wrap their underlying not-found error with +// ErrTaskNotFound — see cmd/kandev/turn_adapters.go's taskDeleterAdapter. func isTaskNotFound(err error) bool { - if err == nil { - return false - } - if errors.Is(err, ErrTaskNotFound) { - return true - } - return strings.Contains(err.Error(), "not found") + return err != nil && errors.Is(err, ErrTaskNotFound) } // TaskSessionChecker checks whether the user genuinely engaged with a task @@ -378,7 +353,7 @@ func (s *Service) ConfigureToken(ctx context.Context, token string) error { testClient := s.newPATClient(token) user, err := testClient.GetAuthenticatedUser(ctx) if err != nil { - return fmt.Errorf("invalid token: %w", err) + return fmt.Errorf("%w: %w", ErrInvalidToken, err) } s.logger.Info("validated GitHub token", zap.String("user", user)) @@ -748,7 +723,7 @@ func (s *Service) AssociateExistingPRByURL(ctx context.Context, taskID, reposito } owner, repo, prNumber, err := parsePRURL(prURL) if err != nil { - return nil, fmt.Errorf("%w: %v", ErrInvalidPRURL, err) + return nil, fmt.Errorf("%w: %w", ErrInvalidPRURL, err) } pr, err := s.client.GetPR(ctx, owner, repo, prNumber) if err != nil { diff --git a/apps/backend/internal/github/service_review_cleanup_test.go b/apps/backend/internal/github/service_review_cleanup_test.go index 07d1094cfd..65ba16aeac 100644 --- a/apps/backend/internal/github/service_review_cleanup_test.go +++ b/apps/backend/internal/github/service_review_cleanup_test.go @@ -50,9 +50,10 @@ func TestCleanupMergedReviewTasks_TaskAlreadyDeleted(t *testing.T) { RepoName: "widget", }) - // Stub: DeleteTask returns "not found" as the real service does. + // Stub: DeleteTask returns the sentinel-wrapped not-found error as the + // real adapter (see cmd/kandev/turn_adapters.go's taskDeleterAdapter) does. svc.SetTaskDeleter(&stubTaskDeleter{ - err: fmt.Errorf("task not found: %s", taskID), + err: fmt.Errorf("%w: %s", ErrTaskNotFound, taskID), }) deleted, err := svc.CleanupMergedReviewTasks(ctx, watch) diff --git a/apps/backend/internal/office/skills/errors.go b/apps/backend/internal/office/skills/errors.go new file mode 100644 index 0000000000..0f8f733bdb --- /dev/null +++ b/apps/backend/internal/office/skills/errors.go @@ -0,0 +1,13 @@ +package skills + +import "errors" + +// ErrSkillNotFound is returned by GetSkill when no skill row matches the +// supplied id or slug. Callers should classify with errors.Is so HTTP layers +// can translate to 404 without string-matching the formatted message. +var ErrSkillNotFound = errors.New("skill not found") + +// ErrSkillFileNotFound is returned by file accessors (readLocalSkillFile, +// readUserHomeSkillInventoryFile, GetSkillFile) when the requested file is +// missing from the skill's source. Callers translate to 404. +var ErrSkillFileNotFound = errors.New("skill file not found") diff --git a/apps/backend/internal/office/skills/errors_test.go b/apps/backend/internal/office/skills/errors_test.go new file mode 100644 index 0000000000..96d4fe73dc --- /dev/null +++ b/apps/backend/internal/office/skills/errors_test.go @@ -0,0 +1,22 @@ +package skills + +import ( + "errors" + "testing" +) + +// TestErrorsAreClassifiable verifies that the file-accessor helper wraps its +// not-found error with the package sentinel so HTTP callers +// (handler.getSkillFile) can classify via errors.Is. The GetSkillFromConfig +// wrap is covered separately in service_test.go's external-package test. +func TestErrorsAreClassifiable(t *testing.T) { + t.Run("readUserHomeSkillInventoryFile returns ErrSkillFileNotFound", func(t *testing.T) { + _, err := readUserHomeSkillInventoryFile(`[]`, "missing.md") + if err == nil { + t.Fatal("expected error for missing file in empty inventory") + } + if !errors.Is(err, ErrSkillFileNotFound) { + t.Errorf("error not classifiable as ErrSkillFileNotFound: %v", err) + } + }) +} diff --git a/apps/backend/internal/office/skills/handler.go b/apps/backend/internal/office/skills/handler.go index 1a82fb385c..35e9c0c743 100644 --- a/apps/backend/internal/office/skills/handler.go +++ b/apps/backend/internal/office/skills/handler.go @@ -1,8 +1,8 @@ package skills import ( + "errors" "net/http" - "strings" "github.com/gin-gonic/gin" @@ -183,7 +183,7 @@ func (h *Handler) getSkillFile(c *gin.Context) { content, err := h.svc.GetSkillFile(c.Request.Context(), c.Param("id"), path) if err != nil { status := http.StatusInternalServerError - if strings.Contains(err.Error(), "not found") { + if errors.Is(err, ErrSkillFileNotFound) || errors.Is(err, ErrSkillNotFound) { status = http.StatusNotFound } c.JSON(status, gin.H{"error": err.Error()}) diff --git a/apps/backend/internal/office/skills/import.go b/apps/backend/internal/office/skills/import.go index 1fbd81895c..be754a57cd 100644 --- a/apps/backend/internal/office/skills/import.go +++ b/apps/backend/internal/office/skills/import.go @@ -406,7 +406,7 @@ func readLocalSkillFile(basePath, relPath string) (string, error) { data, err := os.ReadFile(safePath) if err != nil { if os.IsNotExist(err) { - return "", fmt.Errorf("file not found: %s", relPath) + return "", fmt.Errorf("%w: %s", ErrSkillFileNotFound, relPath) } return "", err } @@ -424,7 +424,7 @@ func readUserHomeSkillInventoryFile(inventory, relPath string) (string, error) { return file.Content, nil } } - return "", fmt.Errorf("file not found: %s", relPath) + return "", fmt.Errorf("%w: %s", ErrSkillFileNotFound, relPath) } // basePath returns the kandev base path (for ownership checks in local import). diff --git a/apps/backend/internal/office/skills/service.go b/apps/backend/internal/office/skills/service.go index 5f07e32f45..d447106c6b 100644 --- a/apps/backend/internal/office/skills/service.go +++ b/apps/backend/internal/office/skills/service.go @@ -247,7 +247,7 @@ func (s *SkillService) GetSkillFromConfig(ctx context.Context, idOrSlug string) return sk, nil } } - return nil, fmt.Errorf("skill not found: %s", idOrSlug) + return nil, fmt.Errorf("%w: %s", ErrSkillNotFound, idOrSlug) } // ListSkillsFromConfig returns all skills for a workspace. diff --git a/apps/backend/internal/office/skills/service_test.go b/apps/backend/internal/office/skills/service_test.go index 55edad83f1..f30d99934e 100644 --- a/apps/backend/internal/office/skills/service_test.go +++ b/apps/backend/internal/office/skills/service_test.go @@ -3,6 +3,7 @@ package skills_test import ( "context" "encoding/json" + "errors" "fmt" "net/http" "os" @@ -609,6 +610,21 @@ func TestGetSkillFile_EmptyPathReturnsSKILLMD(t *testing.T) { } } +// TestGetSkillFromConfig_MissingIDWrapsErrSkillNotFound locks in the +// production contract: a missing skill id flows through the real lookup +// path and surfaces an error wrapping ErrSkillNotFound, so handler.getSkill +// can map it to a 404 via errors.Is without parsing the formatted message. +func TestGetSkillFromConfig_MissingIDWrapsErrSkillNotFound(t *testing.T) { + svc := newTestSkillService(t) + _, err := svc.GetSkillFromConfig(context.Background(), "skill-that-does-not-exist") + if err == nil { + t.Fatal("expected GetSkillFromConfig to fail for missing id") + } + if !errors.Is(err, skills.ErrSkillNotFound) { + t.Errorf("missing-id error not classifiable as ErrSkillNotFound: %v", err) + } +} + func TestImportFromSource_RepoDiscovery(t *testing.T) { svc := newTestSkillService(t) ctx := context.Background() diff --git a/apps/backend/internal/orchestrator/errors_test.go b/apps/backend/internal/orchestrator/errors_test.go new file mode 100644 index 0000000000..d9b973b618 --- /dev/null +++ b/apps/backend/internal/orchestrator/errors_test.go @@ -0,0 +1,51 @@ +package orchestrator + +import ( + "errors" + "fmt" + "testing" + + "github.com/kandev/kandev/internal/agent/runtime/lifecycle" +) + +// TestErrorsAreClassifiable verifies that the orchestrator's error +// classification helpers rely exclusively on errors.Is + wrapped sentinels, +// not substring matches on the formatted message. A library upgrade that +// changes wording must not silently break these checks. +func TestErrorsAreClassifiable(t *testing.T) { + t.Run("isAgentPromptInProgressError requires the sentinel", func(t *testing.T) { + if !isAgentPromptInProgressError(ErrAgentPromptInProgress) { + t.Errorf("exact sentinel must classify") + } + if !isAgentPromptInProgressError(fmt.Errorf("wrap: %w", ErrAgentPromptInProgress)) { + t.Errorf("wrapped sentinel must classify") + } + if isAgentPromptInProgressError(errors.New("agent is currently processing a prompt")) { + t.Errorf("untyped lookalike must no longer classify") + } + }) + + t.Run("isSessionResetInProgressError requires the sentinel", func(t *testing.T) { + if !isSessionResetInProgressError(ErrSessionResetInProgress) { + t.Errorf("exact sentinel must classify") + } + if !isSessionResetInProgressError(fmt.Errorf("wrap: %w", ErrSessionResetInProgress)) { + t.Errorf("wrapped sentinel must classify") + } + if isSessionResetInProgressError(errors.New("session reset in progress")) { + t.Errorf("untyped lookalike must no longer classify") + } + }) + + t.Run("isAgentAlreadyRunningError uses lifecycle.ErrAgentAlreadyRunning", func(t *testing.T) { + if !isAgentAlreadyRunningError(lifecycle.ErrAgentAlreadyRunning) { + t.Errorf("exact sentinel must classify") + } + if !isAgentAlreadyRunningError(fmt.Errorf("LaunchAgent: %w", lifecycle.ErrAgentAlreadyRunning)) { + t.Errorf("wrapped sentinel must classify") + } + if isAgentAlreadyRunningError(errors.New("session already has an agent running somewhere")) { + t.Errorf("untyped lookalike must no longer classify") + } + }) +} diff --git a/apps/backend/internal/orchestrator/executor/executor_office.go b/apps/backend/internal/orchestrator/executor/executor_office.go index 31504492f2..87f3f5616e 100644 --- a/apps/backend/internal/orchestrator/executor/executor_office.go +++ b/apps/backend/internal/orchestrator/executor/executor_office.go @@ -4,21 +4,16 @@ import ( "context" "errors" "fmt" - "strings" "time" "github.com/google/uuid" "github.com/kandev/kandev/internal/task/models" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" v1 "github.com/kandev/kandev/pkg/api/v1" "go.uber.org/zap" ) -// uniqueIndexErrFragment matches the SQLite uniqueness violation surfaced -// when two concurrent EnsureSessionForAgent callers race past the SELECT and -// both INSERT a row for the same (task_id, agent_profile_id) pair. -const uniqueIndexErrFragment = "uniq_office_task_session" - // EnsureSessionForAgent returns a persistent task session for the (task, // agent) pair, creating one when no row exists. This is the office run // entry point — every run for a participant agent ends up here. Distinct @@ -56,7 +51,7 @@ func (e *Executor) EnsureSessionForAgent( } created, err := e.createOfficeSession(ctx, task, agentInstanceID, agentProfileID, executorID, executorProfileID) - if err != nil && strings.Contains(err.Error(), uniqueIndexErrFragment) { + if err != nil && errors.Is(err, taskrepo.ErrOfficeSessionRaceConflict) { // Lost the race against a concurrent caller. Re-read and reuse. raced, lookupErr := e.repo.GetTaskSessionByTaskAndAgent(ctx, task.ID, agentInstanceID) if lookupErr == nil && raced != nil { diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index fffcf358c4..a97497eb3f 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -2,8 +2,8 @@ package executor import ( "context" + "errors" "fmt" - "strings" "time" "github.com/kandev/kandev/internal/agent/runtime/lifecycle" @@ -21,7 +21,7 @@ import ( // without proper cleanup). Callers must probe IsAgentRunningForSession to // distinguish live from stale before deciding to clean up. func isAgentAlreadyRunningError(err error) bool { - return err != nil && strings.Contains(err.Error(), "already has an agent running") + return err != nil && errors.Is(err, lifecycle.ErrAgentAlreadyRunning) } // isTerminalSessionState reports whether a session state implies the agent diff --git a/apps/backend/internal/orchestrator/executor/executor_resume_test.go b/apps/backend/internal/orchestrator/executor/executor_resume_test.go index 31463b8366..4c5ebe8b61 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/task/models" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -83,7 +84,7 @@ func TestResumeSession_LiveAgentReturnsAlreadyRunning(t *testing.T) { agentMgr := &mockAgentManager{ launchAgentFunc: func(_ context.Context, req *LaunchAgentRequest) (*LaunchAgentResponse, error) { - return nil, fmt.Errorf("session %q already has an agent running (execution: %s)", req.SessionID, "exec-live") + return nil, fmt.Errorf("%w: session %q (execution: %s)", lifecycle.ErrAgentAlreadyRunning, req.SessionID, "exec-live") }, isAgentRunningForSessionFunc: func(_ context.Context, _ string) bool { return true @@ -123,7 +124,7 @@ func TestResumeSession_StaleExecutionCleansUpAndRetries(t *testing.T) { launchAgentFunc: func(_ context.Context, req *LaunchAgentRequest) (*LaunchAgentResponse, error) { launchCalls++ if launchCalls == 1 { - return nil, fmt.Errorf("session %q already has an agent running (execution: %s)", req.SessionID, "exec-stale") + return nil, fmt.Errorf("%w: session %q (execution: %s)", lifecycle.ErrAgentAlreadyRunning, req.SessionID, "exec-stale") } return &LaunchAgentResponse{ AgentExecutionID: "exec-new", @@ -329,7 +330,7 @@ func TestResumeSession_TerminalStateSkipsLivenessProbeOnFallback(t *testing.T) { launchAgentFunc: func(_ context.Context, req *LaunchAgentRequest) (*LaunchAgentResponse, error) { launchCalls++ if launchCalls == 1 { - return nil, fmt.Errorf("session %q already has an agent running (execution: %s)", req.SessionID, "exec-stale") + return nil, fmt.Errorf("%w: session %q (execution: %s)", lifecycle.ErrAgentAlreadyRunning, req.SessionID, "exec-stale") } return &LaunchAgentResponse{ AgentExecutionID: "exec-new", diff --git a/apps/backend/internal/orchestrator/handlers/handlers.go b/apps/backend/internal/orchestrator/handlers/handlers.go index e6460fa95c..2e9b815d3d 100644 --- a/apps/backend/internal/orchestrator/handlers/handlers.go +++ b/apps/backend/internal/orchestrator/handlers/handlers.go @@ -3,11 +3,12 @@ package handlers import ( "context" - "strings" + "errors" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/orchestrator" "github.com/kandev/kandev/internal/orchestrator/dto" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" ws "github.com/kandev/kandev/pkg/websocket" "go.uber.org/zap" ) @@ -114,8 +115,8 @@ func (h *Handlers) wsEnsureSession(ctx context.Context, msg *ws.Message) (*ws.Me zap.Error(err)) // Mirror httpEnsureTaskSession's NotFound mapping so the frontend can // distinguish unknown task ids from real server errors. EnsureSession - // wraps the repo error as "task not found: %w". - if strings.Contains(err.Error(), "task not found") { + // wraps the repo's ErrTaskNotFound. + if errors.Is(err, taskrepo.ErrTaskNotFound) { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, "Task not found", nil) } return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to ensure session: "+err.Error(), nil) diff --git a/apps/backend/internal/orchestrator/scheduler/scheduler.go b/apps/backend/internal/orchestrator/scheduler/scheduler.go index 8f0bd7b3e7..4494ba23c4 100644 --- a/apps/backend/internal/orchestrator/scheduler/scheduler.go +++ b/apps/backend/internal/orchestrator/scheduler/scheduler.go @@ -4,7 +4,6 @@ package scheduler import ( "context" "errors" - "strings" "sync" "sync/atomic" "time" @@ -12,6 +11,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/orchestrator/executor" "github.com/kandev/kandev/internal/orchestrator/queue" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" v1 "github.com/kandev/kandev/pkg/api/v1" "go.uber.org/zap" ) @@ -20,7 +20,6 @@ import ( var ( ErrSchedulerAlreadyRunning = errors.New("scheduler is already running") ErrSchedulerNotRunning = errors.New("scheduler is not running") - ErrTaskNotFound = errors.New("task not found") ) // SchedulerConfig holds scheduler configuration @@ -286,7 +285,7 @@ func (s *Scheduler) processTasks(ctx context.Context) { zap.Error(err)) // Don't re-enqueue if task was deleted from DB - if strings.Contains(err.Error(), "not found") { + if errors.Is(err, taskrepo.ErrTaskNotFound) { s.logger.Warn("task no longer exists, dropping from queue", zap.String("task_id", task.ID)) continue diff --git a/apps/backend/internal/orchestrator/scheduler/scheduler_test.go b/apps/backend/internal/orchestrator/scheduler/scheduler_test.go index 341ef7f962..afe4be5aa4 100644 --- a/apps/backend/internal/orchestrator/scheduler/scheduler_test.go +++ b/apps/backend/internal/orchestrator/scheduler/scheduler_test.go @@ -18,6 +18,7 @@ import ( "github.com/kandev/kandev/internal/orchestrator/executor" "github.com/kandev/kandev/internal/orchestrator/queue" "github.com/kandev/kandev/internal/task/repository" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -184,7 +185,10 @@ func (r *testTaskRepository) GetTask(ctx context.Context, taskID string) (*v1.Ta defer r.mu.RUnlock() task, exists := r.tasks[taskID] if !exists { - return nil, ErrTaskNotFound + // Mirror the production sqlite repository's wrapping so processTasks' + // errors.Is(err, taskrepo.ErrTaskNotFound) check exercises the same + // path it would in prod. + return nil, fmt.Errorf("%w: %s", taskrepo.ErrTaskNotFound, taskID) } copy := *task return ©, nil @@ -195,7 +199,7 @@ func (r *testTaskRepository) UpdateTaskState(ctx context.Context, taskID string, defer r.mu.Unlock() task, exists := r.tasks[taskID] if !exists { - return ErrTaskNotFound + return fmt.Errorf("%w: %s", taskrepo.ErrTaskNotFound, taskID) } task.State = state return nil diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index c100785a45..64b0681c37 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -41,11 +41,11 @@ var ErrAgentPromptInProgress = errors.New("agent is currently processing a promp var ErrSessionResetInProgress = errors.New("session reset in progress") func isAgentPromptInProgressError(err error) bool { - return err != nil && (errors.Is(err, ErrAgentPromptInProgress) || strings.Contains(err.Error(), ErrAgentPromptInProgress.Error())) + return err != nil && errors.Is(err, ErrAgentPromptInProgress) } func isSessionResetInProgressError(err error) bool { - return err != nil && (errors.Is(err, ErrSessionResetInProgress) || strings.Contains(err.Error(), ErrSessionResetInProgress.Error())) + return err != nil && errors.Is(err, ErrSessionResetInProgress) } // isTransientPromptError reports whether a prompt error is worth retrying via @@ -65,7 +65,7 @@ func isTransientPromptError(err error) bool { } func isAgentAlreadyRunningError(err error) bool { - return err != nil && strings.Contains(err.Error(), "already has an agent running") + return err != nil && errors.Is(err, lifecycle.ErrAgentAlreadyRunning) } func validateSessionWorktrees(session *models.TaskSession) error { diff --git a/apps/backend/internal/orchestrator/task_operations_test.go b/apps/backend/internal/orchestrator/task_operations_test.go index 4c52169456..05f3c60da2 100644 --- a/apps/backend/internal/orchestrator/task_operations_test.go +++ b/apps/backend/internal/orchestrator/task_operations_test.go @@ -1021,7 +1021,7 @@ func TestErrorClassificationFunctions(t *testing.T) { {"unrelated error", errors.New("something else"), false}, {"exact match", ErrAgentPromptInProgress, true}, {"wrapped error", fmt.Errorf("outer: %w", ErrAgentPromptInProgress), true}, - {"string contains match", errors.New("prefix: agent is currently processing a prompt, try later"), true}, + {"untyped string match no longer accepted", errors.New("prefix: agent is currently processing a prompt, try later"), false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -1042,7 +1042,7 @@ func TestErrorClassificationFunctions(t *testing.T) { {"unrelated error", errors.New("something else"), false}, {"exact match", ErrSessionResetInProgress, true}, {"wrapped error", fmt.Errorf("outer: %w", ErrSessionResetInProgress), true}, - {"string contains match", errors.New("prefix: session reset in progress, please wait"), true}, + {"untyped string match no longer accepted", errors.New("prefix: session reset in progress, please wait"), false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -1061,8 +1061,8 @@ func TestErrorClassificationFunctions(t *testing.T) { }{ {"nil error", nil, false}, {"unrelated error", errors.New("something else"), false}, - {"lifecycle manager error", fmt.Errorf("session %q already has an agent running (execution: %s)", "s1", "exec-1"), true}, - {"wrapped error", fmt.Errorf("failed to resume session: %w", fmt.Errorf("session %q already has an agent running (execution: %s)", "s1", "exec-1")), true}, + {"lifecycle manager error", fmt.Errorf("%w: session %q (execution: %s)", lifecycle.ErrAgentAlreadyRunning, "s1", "exec-1"), true}, + {"wrapped error", fmt.Errorf("failed to resume session: %w", fmt.Errorf("%w: session %q (execution: %s)", lifecycle.ErrAgentAlreadyRunning, "s1", "exec-1")), true}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/apps/backend/internal/task/handlers/errors.go b/apps/backend/internal/task/handlers/errors.go index ff3f2d5a31..88106cf764 100644 --- a/apps/backend/internal/task/handlers/errors.go +++ b/apps/backend/internal/task/handlers/errors.go @@ -1,11 +1,14 @@ package handlers import ( + "errors" "net/http" "strings" "github.com/gin-gonic/gin" "github.com/kandev/kandev/internal/common/logger" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/task/service" "go.uber.org/zap" ) @@ -40,6 +43,14 @@ func isNotFound(err error) bool { if err == nil { return false } + if errors.Is(err, taskrepo.ErrTaskNotFound) || + errors.Is(err, service.ErrDocumentNotFound) || + errors.Is(err, service.ErrTaskPlanNotFound) || + errors.Is(err, service.ErrRevisionNotFound) { + return true + } + // Legacy fallback for repository methods (sessions, environments, etc.) + // that have not yet adopted a typed sentinel. return strings.Contains(strings.ToLower(err.Error()), "not found") } diff --git a/apps/backend/internal/task/handlers/errors_test.go b/apps/backend/internal/task/handlers/errors_test.go new file mode 100644 index 0000000000..3d07dfb4b1 --- /dev/null +++ b/apps/backend/internal/task/handlers/errors_test.go @@ -0,0 +1,84 @@ +package handlers + +import ( + "context" + "errors" + "fmt" + "net" + "testing" + + "github.com/kandev/kandev/internal/agent/runtime/lifecycle" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/task/service" +) + +// TestErrorsAreClassifiable verifies that the package's error-classification +// helpers detect typed sentinels through any depth of wrapping, so HTTP +// status mapping survives downstream wrap/unwrap changes. +func TestErrorsAreClassifiable(t *testing.T) { + notFoundSentinels := map[string]error{ + "taskrepo.ErrTaskNotFound": taskrepo.ErrTaskNotFound, + "service.ErrDocumentNotFound": service.ErrDocumentNotFound, + "service.ErrTaskPlanNotFound": service.ErrTaskPlanNotFound, + "service.ErrRevisionNotFound": service.ErrRevisionNotFound, + } + for name, sentinel := range notFoundSentinels { + t.Run("isNotFound recognizes "+name, func(t *testing.T) { + wrapped := fmt.Errorf("look up failed: %w", sentinel) + if !isNotFound(wrapped) { + t.Errorf("expected wrapped %s to classify as not-found", name) + } + }) + } + + t.Run("isAgentReportedError uses lifecycle.ErrAgentReported", func(t *testing.T) { + wrapped := fmt.Errorf("waitForPromptDone: %w", lifecycle.ErrAgentReported) + if !isAgentReportedError(wrapped) { + t.Errorf("expected wrapped ErrAgentReported to classify") + } + if isAgentReportedError(errors.New("agent error: not the sentinel")) { + t.Errorf("untyped lookalike must no longer classify") + } + }) +} + +// TestIsTimeoutError pins the UX-classification contract for the prompt +// error-message renderer. The pre-refactor substring check on "timeout" +// covered three classes of producer; the typed-sentinel rewrite must keep +// all three classified or the user-facing "Request timed out…" message +// silently downgrades to the generic "Failed to send message to agent". +func TestIsTimeoutError(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"unrelated", errors.New("something else"), false}, + // net.Error with Timeout()==true (typed) — e.g. http.Client deadline. + {"net error timeout", &timeoutNetErr{}, true}, + // Plain fmt.Errorf("timeout …") from waitForSessionReady, agentctl + // health waits, agent-stream connect waits. No typed Timeout() method. + {"session-ready timeout (substring fallback)", errors.New("timeout waiting for session to become ready after resume"), true}, + {"agent-stream timeout (substring fallback)", errors.New("timeout waiting for agent stream to connect after restart"), true}, + // context.DeadlineExceeded is itself a net.Error with Timeout()==true, + // so it matches via the typed path — no separate errors.Is needed in + // createPromptErrorMessage. + {"context.DeadlineExceeded", context.DeadlineExceeded, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isTimeoutError(tc.err); got != tc.want { + t.Errorf("isTimeoutError(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +type timeoutNetErr struct{} + +func (timeoutNetErr) Error() string { return "i/o timeout" } +func (timeoutNetErr) Timeout() bool { return true } +func (timeoutNetErr) Temporary() bool { return false } + +var _ net.Error = timeoutNetErr{} diff --git a/apps/backend/internal/task/handlers/message_handlers.go b/apps/backend/internal/task/handlers/message_handlers.go index ac5425c5d0..d7641ff8c5 100644 --- a/apps/backend/internal/task/handlers/message_handlers.go +++ b/apps/backend/internal/task/handlers/message_handlers.go @@ -11,6 +11,7 @@ import ( "github.com/gin-gonic/gin" "go.uber.org/zap" + "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/orchestrator" "github.com/kandev/kandev/internal/orchestrator/executor" @@ -407,9 +408,31 @@ func (h *MessageHandlers) forwardMessageAsPrompt( } // isAgentReportedError returns true when the error originated from the agent's -// own error event (surfaced via waitForPromptDone as "agent error: ..."). +// own error event (surfaced via waitForPromptDone with the ErrAgentReported +// sentinel wrapped in). func isAgentReportedError(err error) bool { - return strings.Contains(err.Error(), "agent error: ") + return errors.Is(err, lifecycle.ErrAgentReported) +} + +// isTimeoutError reports whether err looks like a timeout. Used by +// createPromptErrorMessage to render the "Request timed out…" UX hint. +// +// Several upstream producers along the prompt path (waitForSessionReady, +// agent-stream connect waits, agentctl health waits) return +// fmt.Errorf("timeout …") rather than wrapping a typed timeout, so a strict +// errors.As(net.Error) check would silently downgrade their user message to +// the generic "Failed to send message to agent". The substring fallback +// preserves the pre-refactor UX for those cases; classifying upstream errors +// properly is tracked separately. +func isTimeoutError(err error) bool { + if err == nil { + return false + } + var netErr interface{ Timeout() bool } + if errors.As(err, &netErr) && netErr.Timeout() { + return true + } + return strings.Contains(strings.ToLower(err.Error()), "timeout") } // handlePromptWithResume attempts to resume a session and retry a prompt when the @@ -453,7 +476,10 @@ func (h *MessageHandlers) createPromptErrorMessage(ctx context.Context, taskID, zap.Error(promptErr)) errorMsg := "Failed to send message to agent" - if strings.Contains(promptErr.Error(), "context deadline exceeded") || strings.Contains(promptErr.Error(), "timeout") { + if isTimeoutError(promptErr) { + // isTimeoutError already covers context.DeadlineExceeded (which + // implements Timeout()==true) and the substring fallback for plain + // "timeout …" producers — no separate errors.Is needed here. errorMsg = "Request timed out. The agent may be processing a complex task. Please try again." } else if errors.Is(promptErr, executor.ErrExecutionNotFound) { errorMsg = "Agent is not running. Please restart the session." diff --git a/apps/backend/internal/task/handlers/process_handlers.go b/apps/backend/internal/task/handlers/process_handlers.go index 65cb31940e..e6dd6d80c2 100644 --- a/apps/backend/internal/task/handlers/process_handlers.go +++ b/apps/backend/internal/task/handlers/process_handlers.go @@ -7,6 +7,7 @@ import ( "net" "net/http" "strings" + "syscall" "time" "github.com/gin-gonic/gin" @@ -390,13 +391,13 @@ func (h *ProcessHandlers) httpListProcesses(c *gin.Context) { if err != nil { var netErr net.Error // Handle expected "no processes" conditions gracefully: - // - no execution found: agent hasn't started yet (async launch) + // - ErrNoExecutionForSession: agent hasn't started yet (async launch) // - connection refused: agent not running // - deadline exceeded: agent not responding if errors.Is(err, context.DeadlineExceeded) || - errors.As(err, &netErr) || - strings.Contains(err.Error(), "connection refused") || - strings.Contains(err.Error(), "no execution found") { + errors.Is(err, syscall.ECONNREFUSED) || + errors.Is(err, lifecycle.ErrNoExecutionForSession) || + errors.As(err, &netErr) { h.logger.Debug("process list unavailable (agent not ready)", zap.String("session_id", sessionID), zap.Error(err)) c.JSON(http.StatusOK, []agentctlclient.ProcessInfo{}) return diff --git a/apps/backend/internal/task/repository/sqlite/errors.go b/apps/backend/internal/task/repository/sqlite/errors.go new file mode 100644 index 0000000000..637c7fd7d0 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/errors.go @@ -0,0 +1,16 @@ +package sqlite + +import "errors" + +// ErrTaskNotFound is returned by Repository methods (GetTask, UpdateTask, +// DeleteTask, UpdateTaskState, …) when no row matches the supplied id. +// Callers should classify via errors.Is rather than substring-matching the +// formatted message, which includes the task id and is therefore brittle. +var ErrTaskNotFound = errors.New("task not found") + +// ErrOfficeSessionRaceConflict is returned by CreateTaskSession when the +// insert violates the uniq_office_task_session partial unique index — i.e. +// two callers raced past their SELECT-then-INSERT for the same +// (task_id, agent_profile_id) pair. Callers should re-read and reuse the +// winning row rather than treating this as a hard failure. +var ErrOfficeSessionRaceConflict = errors.New("office task session race conflict") diff --git a/apps/backend/internal/task/repository/sqlite/errors_test.go b/apps/backend/internal/task/repository/sqlite/errors_test.go new file mode 100644 index 0000000000..fcd58c6dc4 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/errors_test.go @@ -0,0 +1,47 @@ +package sqlite + +import ( + "context" + "errors" + "testing" + + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +// TestErrorsAreClassifiable verifies that the repository wraps not-found +// errors with the ErrTaskNotFound sentinel so HTTP / orchestrator callers +// can classify via errors.Is. +func TestErrorsAreClassifiable(t *testing.T) { + ctx := context.Background() + repo := newRepoForHealTests(t) + + t.Run("GetTask returns sentinel-wrapped error for missing row", func(t *testing.T) { + _, err := repo.GetTask(ctx, "task-does-not-exist") + if err == nil { + t.Fatal("expected GetTask to fail for missing id") + } + if !errors.Is(err, ErrTaskNotFound) { + t.Errorf("GetTask error not classifiable as ErrTaskNotFound: %v", err) + } + }) + + t.Run("DeleteTask returns sentinel-wrapped error for missing row", func(t *testing.T) { + err := repo.DeleteTask(ctx, "task-does-not-exist") + if err == nil { + t.Fatal("expected DeleteTask to fail for missing id") + } + if !errors.Is(err, ErrTaskNotFound) { + t.Errorf("DeleteTask error not classifiable as ErrTaskNotFound: %v", err) + } + }) + + t.Run("UpdateTaskState returns sentinel-wrapped error for missing row", func(t *testing.T) { + err := repo.UpdateTaskState(ctx, "task-does-not-exist", v1.TaskStateInProgress) + if err == nil { + t.Fatal("expected UpdateTaskState to fail for missing id") + } + if !errors.Is(err, ErrTaskNotFound) { + t.Errorf("UpdateTaskState error not classifiable as ErrTaskNotFound: %v", err) + } + }) +} diff --git a/apps/backend/internal/task/repository/sqlite/session.go b/apps/backend/internal/task/repository/sqlite/session.go index 2c51c1f6a5..c45fdbde49 100644 --- a/apps/backend/internal/task/repository/sqlite/session.go +++ b/apps/backend/internal/task/repository/sqlite/session.go @@ -265,6 +265,12 @@ func (r *Repository) CreateTaskSession(ctx context.Context, session *models.Task dialect.BoolToInt(session.IsPrimary), session.ReviewStatus, dialect.BoolToInt(session.IsPassthrough), session.TaskEnvironmentID) + if err != nil && strings.Contains(err.Error(), "uniq_office_task_session") { + // Two callers raced past their SELECT-then-INSERT for the same + // (task_id, agent_profile_id) — surface a typed sentinel so callers + // can classify with errors.Is rather than driver-message matching. + return fmt.Errorf("%w: %w", ErrOfficeSessionRaceConflict, err) + } return err } diff --git a/apps/backend/internal/task/repository/sqlite/task.go b/apps/backend/internal/task/repository/sqlite/task.go index 7e6e139cb9..6843eeab9d 100644 --- a/apps/backend/internal/task/repository/sqlite/task.go +++ b/apps/backend/internal/task/repository/sqlite/task.go @@ -185,7 +185,7 @@ func (r *Repository) GetTask(ctx context.Context, id string) (*models.Task, erro `SELECT `+taskSelectColumns("t")+` FROM tasks t WHERE t.id = ?`), id) task, err := r.scanSingleTask(row) if err == sql.ErrNoRows { - return nil, fmt.Errorf("task not found: %s", id) + return nil, fmt.Errorf("%w: %s", ErrTaskNotFound, id) } return task, err } @@ -217,7 +217,7 @@ func (r *Repository) UpdateTask(ctx context.Context, task *models.Task) error { rows, _ := result.RowsAffected() if rows == 0 { - return fmt.Errorf("task not found: %s", task.ID) + return fmt.Errorf("%w: %s", ErrTaskNotFound, task.ID) } if err := syncRunnerInTx(ctx, tx, task.WorkflowStepID, task.ID, task.AssigneeAgentProfileID); err != nil { @@ -236,7 +236,7 @@ func (r *Repository) DeleteTask(ctx context.Context, id string) error { rows, _ := result.RowsAffected() if rows == 0 { - return fmt.Errorf("task not found: %s", id) + return fmt.Errorf("%w: %s", ErrTaskNotFound, id) } return nil } @@ -661,7 +661,7 @@ func (r *Repository) ArchiveTask(ctx context.Context, id string) error { } rows, _ := result.RowsAffected() if rows == 0 { - return fmt.Errorf("task not found: %s", id) + return fmt.Errorf("%w: %s", ErrTaskNotFound, id) } return nil } @@ -739,7 +739,7 @@ func (r *Repository) UpdateTaskState(ctx context.Context, id string, state v1.Ta rows, _ := result.RowsAffected() if rows == 0 { - return fmt.Errorf("task not found: %s", id) + return fmt.Errorf("%w: %s", ErrTaskNotFound, id) } return nil }