fix(tasks): permanent failure on missing secrets; post-transition row from POST /api/tasks

apps/api/e2e/provisioning-no-secret.e2e.test.ts

@@ -0,0 +1,129 @@
+/**
+ * E2E: a repo task whose required agent secret is missing must fail
+ * TERMINALLY during provisioning — not retry forever.
+ *
+ * This server deliberately seeds NO secrets (unlike repo-task.e2e.test.ts).
+ * The task-worker's secret resolution throws
+ * `Secret not found: ANTHROPIC_API_KEY (scope: global)` while the task is in
+ * `provisioning`. Before the fix this was classified as recoverable, so the
+ * task bounced queued↔provisioning on 30s requeues indefinitely (the
+ * reconciler re-enqueues without the provisioningRetryCount, defeating the
+ * retry cap). Now the missing-secret error is classified as permanent:
+ * exactly one worker pickup, one `provisioning_permanent_failure` transition,
+ * terminal `failed` with the actionable message on the task row.
+ */
+import { afterAll, beforeAll, describe, expect, it } from "vitest";
+import { startApiServer, waitFor, type ApiServerHandle } from "../src/test-utils/e2e/api-server.js";
+
+const REPO_URL = "https://github.com/e2e-org/e2e-no-secret-repo";
+const REPO_FULL_NAME = "e2e-org/e2e-no-secret-repo";
+
+let server: ApiServerHandle;
+
+beforeAll(async () => {
+  server = await startApiServer();
+
+  // NOTE: no secrets are seeded — that is the scenario under test. The
+  // claude-code adapter (default api-key auth mode) hard-requires
+  // ANTHROPIC_API_KEY, so provisioning must fail permanently.
+  const { status } = await api("/api/repos", {
+    method: "POST",
+    body: JSON.stringify({ repoUrl: REPO_URL, fullName: REPO_FULL_NAME, defaultBranch: "main" }),
+  });
+  expect(status).toBe(201);
+}, 150_000);
+
+afterAll(async () => {
+  await server?.stop();
+});
+
+async function api<T = unknown>(
+  path: string,
+  init?: RequestInit,
+): Promise<{ status: number; body: T }> {
+  const res = await fetch(`${server.baseUrl}${path}`, {
+    headers: { "content-type": "application/json" },
+    ...init,
+  });
+  return { status: res.status, body: (await res.json()) as T };
+}
+
+interface TaskRow {
+  id: string;
+  state: string;
+  errorMessage: string | null;
+}
+
+interface TaskEvent {
+  fromState: string | null;
+  toState: string;
+  trigger: string;
+  message: string | null;
+}
+
+async function getTask(taskId: string): Promise<TaskRow> {
+  const { body } = await api<{ task: TaskRow }>(`/api/tasks/${taskId}`);
+  return body.task;
+}
+
+async function getEvents(taskId: string): Promise<TaskEvent[]> {
+  const { body } = await api<{ events: TaskEvent[] }>(`/api/tasks/${taskId}/events`);
+  return body.events;
+}
+
+describe("repo task provisioning without required secrets", () => {
+  it("fails terminally with the missing-secret message instead of retrying forever", async () => {
+    const { status, body } = await api<{ task: TaskRow }>("/api/tasks", {
+      method: "POST",
+      body: JSON.stringify({
+        title: "No secrets configured",
+        prompt: "E2E scenario: provisioning without secrets",
+        repoUrl: REPO_URL,
+        agentType: "claude-code",
+      }),
+    });
+    expect(status).toBe(201);
+    // The response is the post-transition row (queued), never the stale
+    // pending row.
+    expect(body.task.state).toBe("queued");
+    const taskId = body.task.id;
+
+    // The worker picks the task up, moves it to provisioning, hits the
+    // missing secret, and must land in terminal `failed` — no bounce loop.
+    const task = await waitFor(
+      async () => {
+        const t = await getTask(taskId);
+        return t.state === "failed" ? t : null;
+      },
+      { timeoutMs: 90_000, label: `task ${taskId} → failed` },
+    ).catch((err) => {
+      throw new Error(
+        `${err}\n--- server logs (tail) ---\n${server.logs().split("\n").slice(-40).join("\n")}`,
+      );
+    });
+
+    expect(task.errorMessage).toContain("Secret not found: ANTHROPIC_API_KEY");
+
+    const events = await getEvents(taskId);
+    const failure = events.find((e) => e.trigger === "provisioning_permanent_failure");
+    expect(failure).toBeDefined();
+    expect(failure!.fromState).toBe("provisioning");
+    expect(failure!.toState).toBe("failed");
+    expect(failure!.message).toContain("Secret not found: ANTHROPIC_API_KEY");
+
+    // Exactly one pickup, zero recoverable requeues — the old behavior
+    // produced an endless provisioning_retry / worker_pickup train.
+    expect(events.filter((e) => e.trigger === "worker_pickup")).toHaveLength(1);
+    expect(events.filter((e) => e.trigger === "provisioning_retry")).toHaveLength(0);
+
+    // Let a few reconciler/stall-check cycles pass (2s intervals in the e2e
+    // harness) and confirm nothing resurrects the task.
+    await new Promise((r) => setTimeout(r, 5_000));
+
+    const after = await getTask(taskId);
+    expect(after.state).toBe("failed");
+    const eventsAfter = await getEvents(taskId);
+    expect(eventsAfter.filter((e) => e.trigger === "worker_pickup")).toHaveLength(1);
+    expect(eventsAfter.filter((e) => e.trigger === "provisioning_retry")).toHaveLength(0);
+  });
+});

apps/api/e2e/repo-task.e2e.test.ts

@@ -118,11 +118,10 @@ async function createTask(title: string): Promise<string> {
     }),
   });
   expect(status).toBe(201);
-  // Actual behavior: the route transitions the task to `queued` before
-  // responding, but the response carries the ORIGINAL createTask() row — so
-  // the reported state is still `pending`. The queued transition shows up in
-  // the task's event log (asserted in the failure test).
-  expect(body.task.state).toBe("pending");
+  // The route transitions the task to `queued` before responding and the
+  // response carries the post-transition row — clients never see the
+  // already-left `pending` state.
+  expect(body.task.state).toBe("queued");
   return body.task.id;
 }
 

apps/api/src/routes/tasks.test.ts

@@ -274,9 +274,11 @@ describe("POST /api/tasks", () => {
     app = await buildTestApp();
   });
 
-  it("creates a task and enqueues it", async () => {
-    mockCreateTask.mockResolvedValue({ ...mockTaskData, id: "new-task" });
-    mockTransitionTask.mockResolvedValue(undefined);
+  it("creates a task, enqueues it, and responds with the post-transition (queued) row", async () => {
+    mockCreateTask.mockResolvedValue({ ...mockTaskData, id: "new-task", state: "pending" });
+    // transitionTask returns the updated row — the route must respond with
+    // this, not the stale `pending` row from createTask().
+    mockTransitionTask.mockResolvedValue({ ...mockTaskData, id: "new-task", state: "queued" });
 
     const res = await app.inject({
       method: "POST",
@@ -301,11 +303,16 @@ describe("POST /api/tasks", () => {
     );
     expect(mockTransitionTask).toHaveBeenCalled();
     expect(mockQueueAdd).toHaveBeenCalled();
+    expect(res.json().task.state).toBe("queued");
   });
 
   it("creates a task with dependencies", async () => {
-    mockCreateTask.mockResolvedValue({ ...mockTaskData, id: "new-task" });
-    mockTransitionTask.mockResolvedValue(undefined);
+    mockCreateTask.mockResolvedValue({ ...mockTaskData, id: "new-task", state: "pending" });
+    mockTransitionTask.mockResolvedValue({
+      ...mockTaskData,
+      id: "new-task",
+      state: "waiting_on_deps",
+    });
     mockAddDependencies.mockResolvedValue(undefined);
 
     const res = await app.inject({
@@ -334,6 +341,7 @@ describe("POST /api/tasks", () => {
     );
     // Should NOT enqueue when dependencies exist
     expect(mockQueueAdd).not.toHaveBeenCalled();
+    expect(res.json().task.state).toBe("waiting_on_deps");
   });
 
   it("rejects invalid agentType (400 from Zod body schema)", async () => {

apps/api/src/routes/tasks.ts

@@ -411,7 +411,8 @@ export async function taskRoutes(rawApp: FastifyInstance) {
         description:
           "Submit a new task to run against a repository. The task is " +
           "created in `pending` state and immediately transitioned to " +
-          "`queued` (or `waiting_on_deps` if `dependsOn` is non-empty). " +
+          "`queued` (or `waiting_on_deps` if `dependsOn` is non-empty); " +
+          "the response carries the post-transition row. " +
           "Requires `member` role.",
         tags: ["Tasks"],
         body: createTaskSchema,
@@ -545,16 +546,20 @@ export async function taskRoutes(rawApp: FastifyInstance) {
         }
       }
 
+      // transitionTask returns the post-transition row — respond with that,
+      // not the stale `pending` row from createTask(), so clients never see a
+      // state the task has already left.
+      let transitioned: typeof task;
       if (hasDeps) {
-        await taskService.transitionTask(
+        transitioned = await taskService.transitionTask(
           task.id,
           TaskState.WAITING_ON_DEPS,
           "task_submitted_with_deps",
           undefined,
           req.user?.id,
         );
       } else {
-        await taskService.transitionTask(
+        transitioned = await taskService.transitionTask(
           task.id,
           TaskState.QUEUED,
           "task_submitted",
@@ -573,7 +578,7 @@ export async function taskRoutes(rawApp: FastifyInstance) {
         );
       }
 
-      reply.status(201).send({ task: { type: "repo-task", ...task } });
+      reply.status(201).send({ task: { type: "repo-task", ...(transitioned ?? task) } });
     },
   );
 

packages/shared/src/error-classifier.test.ts

@@ -23,10 +23,13 @@ describe("classifyError", () => {
     expect(result.retryable).toBe(true);
   });
 
-  it("classifies missing secret", () => {
+  it("classifies missing secret as permanent (non-retryable)", () => {
     const result = classifyError("Secret not found: ANTHROPIC_API_KEY (scope: global)");
     expect(result.category).toBe("auth");
     expect(result.title).toContain("ANTHROPIC_API_KEY");
+    // Retrying without adding the secret fails identically — must be permanent
+    // so provisioning fails the task instead of re-queuing forever.
+    expect(result.retryable).toBe(false);
   });
 
   it("classifies wrapped decrypt failure with secret name", () => {
@@ -107,6 +110,7 @@ describe("classifyError", () => {
     const result = classifyError("Secret not found: OPENAI_API_KEY");
     expect(result.category).toBe("auth");
     expect(result.title).toContain("OPENAI_API_KEY");
+    expect(result.retryable).toBe(false);
   });
 
   it("classifies OpenAI API key error directly", () => {

packages/shared/src/error-classifier.ts

@@ -106,14 +106,21 @@ const ERROR_PATTERNS: Array<{
       retryable: true,
     }),
   },
+  // A missing required secret is a configuration error, not a transient one —
+  // retrying without adding the secret fails identically every time (and used
+  // to bounce tasks queued↔provisioning forever). Marked non-retryable so the
+  // provisioning path fails the task immediately with an actionable message.
+  // The pattern is scoped to the exact message secret-service.retrieveSecret()
+  // throws ("Secret not found: NAME (scope: ...)"), not arbitrary errors that
+  // merely mention secrets.
   {
     pattern: /Secret not found: (\w+)/i,
     classify: (match) => ({
       category: "auth",
       title: `Missing secret: ${match[1]}`,
-      description: `The required secret "${match[1]}" is not configured. The agent needs this credential to run.`,
-      remedy: `Go to Secrets and add "${match[1]}", or re-run the setup wizard.`,
-      retryable: true,
+      description: `The required secret "${match[1]}" is not configured. The agent needs this credential to run, and retrying without adding it will fail again.`,
+      remedy: `Go to Secrets and add "${match[1]}" (or re-run the setup wizard), then retry the task.`,
+      retryable: false,
     }),
   },
   // Must precede the credential-name patterns (ANTHROPIC_API_KEY, OPENAI_API_KEY, …)