From dcfec0d23c23b3aaa2af6b72e7d5337265033744 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Tue, 26 May 2026 13:07:46 +0100 Subject: [PATCH] refactor(backend): classify errors via sentinels instead of substring matches (#1057) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(backend): classify errors via sentinels instead of substring matches Introduce package-local sentinel errors in the github, task repo, agent lifecycle, and office/skills packages. Producers wrap with fmt.Errorf("%w: ...") so callers can classify via errors.Is/errors.As instead of brittle strings.Contains(err.Error(), ...) checks that break on library upgrades. Sentinels added (or relocated to errors.go): - github: ErrInvalidToken (new); ErrInvalidPRURL, ErrTaskNotFound, ErrSelfApprove moved from service.go. - task/repository/sqlite: ErrTaskNotFound, ErrOfficeSessionRaceConflict. - lifecycle: ErrAgentAlreadyRunning, ErrAgentReported. - office/skills: ErrSkillNotFound, ErrSkillFileNotFound. Consumers updated to errors.Is/errors.As across orchestrator, task/handlers, github, and office/skills. AssociateExistingPRByURL switches %v -> %w so the inner parsePRURL error stays in the chain. process_handlers now uses errors.Is(err, syscall.ECONNREFUSED) and lifecycle.ErrNoExecutionForSession plus a small isTimeoutError helper backed by errors.As. A new taskDeleterAdapter in cmd/kandev/turn_adapters.go translates taskrepo.ErrTaskNotFound to github.ErrTaskNotFound at the wiring boundary, avoiding a layering violation between the github and task packages. The one remaining substring check, in CreateTaskSession, is encapsulated inside the repo and converts the SQLite UNIQUE-violation message into the typed ErrOfficeSessionRaceConflict sentinel so callers never see the driver string. Co-Authored-By: Claude Opus 4.7 * test(backend): add TestErrorsAreClassifiable for refactored packages Pin the new sentinel-based contract for each refactored package. Each test exercises the producer paths and asserts errors.Is reaches the sentinel through the wrap chain, and (where applicable) confirms untyped lookalike strings no longer classify. - github: isTaskNotFound, AssociateExistingPRByURL/ErrInvalidPRURL, ConfigureToken/ErrInvalidToken (using a noop secret manager). - task/repository/sqlite: GetTask, DeleteTask, UpdateTaskState wrap ErrTaskNotFound. - task/handlers: isNotFound + isAgentReportedError follow the typed chain. - orchestrator: each isXError classifies exact + wrapped sentinels, rejects untyped lookalikes. - office/skills: readUserHomeSkillInventoryFile wraps ErrSkillFileNotFound; ErrSkillNotFound survives fmt.Errorf("%w", ...). Co-Authored-By: Claude Opus 4.7 * test(github): drop network-hitting subtest from errors_test The "ConfigureToken wraps ErrInvalidToken when validation fails" subtest constructed a real PATClient and called GetAuthenticatedUser, which makes a live HTTP request to api.github.com. That's flaky in CI / offline dev and violates the unit-test boundary (caught in review). Replace with a sentinel-survives-wrap-pattern check that locks in the exact %w/%w shape used by ConfigureToken, plus an assertion that the inner cause stays reachable through errors.Is. The full ConfigureToken flow stays covered by service_token_test.go and integration tests. Remove the now-unused noopSecretManager stub type. Co-Authored-By: Claude Opus 4.7 * fix(task): restore "timed out" UX for plain-fmt.Errorf timeout producers The original createPromptErrorMessage rendered the friendly "Request timed out…" hint via strings.Contains(err.Error(), "timeout"). The refactor replaced it with errors.Is(context.DeadlineExceeded) + a net.Error Timeout() helper — strictly typed. But several upstream producers along the prompt path (service.WaitForSessionReady, agent-stream connect waits in lifecycle/session.go and manager_interaction.go, agentctl health waits in agentctl/client.go and launcher.go, the orchestrator's own waitForSessionReady) return plain fmt.Errorf("timeout …") with no typed Timeout() method, so the typed check missed them and the user saw the generic "Failed to send message to agent" instead. Add a substring fallback inside isTimeoutError as a UX safety net while keeping the primary typed paths intact. Lock the contract with table tests covering all three classes (net.Error timeout, plain string, context.DeadlineExceeded) so any future trim of the fallback breaks loudly. Migrating each upstream producer to a typed sentinel is tracked as out-of-scope follow-up. Co-Authored-By: Claude Opus 4.7 * test(backend): tighten classification tests to exercise typed-sentinel contracts Addresses bot-review feedback on PR #1057: - task/handlers/errors_test.go: cover all four not-found sentinels (taskrepo.ErrTaskNotFound, service.ErrDocumentNotFound, service.ErrTaskPlanNotFound, service.ErrRevisionNotFound) in TestErrorsAreClassifiable via a table-driven loop, instead of only one. Drop the spurious Deadline() *time.Time method on timeoutNetErr — it isn't part of net.Error and the existing `var _ net.Error = timeoutNetErr{}` line already pins the interface. - orchestrator/scheduler/scheduler_test.go: testTaskRepository was still returning the local scheduler.ErrTaskNotFound, which doesn't satisfy the production processTasks check (now errors.Is on taskrepo.ErrTaskNotFound). Mirror the real sqlite repo's wrap so the mock and prod paths agree. The local scheduler.ErrTaskNotFound is truly dead after that, so drop it from scheduler.go. - office/skills: the wrap-pattern subtest in errors_test.go was only testing fmt.Errorf, not production code. Move the contract assertion to service_test.go (external skills_test package) where it drives the real GetSkillFromConfig lookup path against an empty in-memory repository. Co-Authored-By: Claude Opus 4.7 * refactor(backend): drop redundant context.DeadlineExceeded check and fix doc grammar Addresses bot-review feedback on PR #1057: - task/handlers/message_handlers.go::createPromptErrorMessage: the explicit errors.Is(promptErr, context.DeadlineExceeded) is redundant with isTimeoutError, which already matches via errors.As against Timeout() — context.DeadlineExceeded implements Timeout()==true. Inline comment documents the consolidation. - github/errors.go: fix grammar in the ErrTaskNotFound doc comment ("is the sentinel cleanup paths check for to distinguish" → "is the sentinel that cleanup paths check to distinguish"). Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 4.7 --- apps/backend/cmd/kandev/main.go | 2 +- apps/backend/cmd/kandev/turn_adapters.go | 23 +++++ .../runtime/lifecycle/execution_store.go | 15 ++++ .../runtime/lifecycle/manager_execution.go | 2 +- .../agent/runtime/lifecycle/manager_launch.go | 4 +- .../runtime/lifecycle/manager_passthrough.go | 2 +- .../agent/runtime/lifecycle/process_runner.go | 8 +- .../agent/runtime/lifecycle/session.go | 4 +- apps/backend/internal/github/controller.go | 3 +- apps/backend/internal/github/errors.go | 27 ++++++ apps/backend/internal/github/errors_test.go | 55 ++++++++++++ apps/backend/internal/github/service.go | 37 ++------ .../github/service_review_cleanup_test.go | 5 +- apps/backend/internal/office/skills/errors.go | 13 +++ .../internal/office/skills/errors_test.go | 22 +++++ .../backend/internal/office/skills/handler.go | 4 +- apps/backend/internal/office/skills/import.go | 4 +- .../backend/internal/office/skills/service.go | 2 +- .../internal/office/skills/service_test.go | 16 ++++ .../internal/orchestrator/errors_test.go | 51 +++++++++++ .../orchestrator/executor/executor_office.go | 9 +- .../orchestrator/executor/executor_resume.go | 4 +- .../executor/executor_resume_test.go | 7 +- .../orchestrator/handlers/handlers.go | 7 +- .../orchestrator/scheduler/scheduler.go | 5 +- .../orchestrator/scheduler/scheduler_test.go | 8 +- .../internal/orchestrator/task_operations.go | 6 +- .../orchestrator/task_operations_test.go | 8 +- apps/backend/internal/task/handlers/errors.go | 11 +++ .../internal/task/handlers/errors_test.go | 84 +++++++++++++++++++ .../task/handlers/message_handlers.go | 32 ++++++- .../task/handlers/process_handlers.go | 9 +- .../internal/task/repository/sqlite/errors.go | 16 ++++ .../task/repository/sqlite/errors_test.go | 47 +++++++++++ .../task/repository/sqlite/session.go | 6 ++ .../internal/task/repository/sqlite/task.go | 10 +-- 36 files changed, 478 insertions(+), 90 deletions(-) create mode 100644 apps/backend/internal/github/errors.go create mode 100644 apps/backend/internal/github/errors_test.go create mode 100644 apps/backend/internal/office/skills/errors.go create mode 100644 apps/backend/internal/office/skills/errors_test.go create mode 100644 apps/backend/internal/orchestrator/errors_test.go create mode 100644 apps/backend/internal/task/handlers/errors_test.go create mode 100644 apps/backend/internal/task/repository/sqlite/errors.go create mode 100644 apps/backend/internal/task/repository/sqlite/errors_test.go 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 }