refactor(backend): classify errors via sentinels instead of substring matches (#1057)

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

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
 		}

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")

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)
+		}
+	})
+}

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 {

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)