From c07e3e34b6377d66f72d0f96f75946a50812dca2 Mon Sep 17 00:00:00 2001 From: Jon Wiggins Date: Sun, 2 Aug 2026 17:12:40 -0600 Subject: [PATCH 1/2] fix(tasks): fail provisioning permanently when a required secret is missing A repo task whose agent secret was never stored (e.g. claude-code with no ANTHROPIC_API_KEY) threw "Secret not found: NAME (scope: global)" during provisioning, which the error classifier marked retryable. The task-worker re-queued it on a 30s delay, and because the reconciler re-enqueues queued tasks without the provisioningRetryCount job field, the 3-retry cap reset every cycle: the task bounced between queued and provisioning forever with no terminal state and no actionable surface. Classify the exact "Secret not found: NAME" shape thrown by secret-service.retrieveSecret() as non-retryable, so the provisioning catch takes its permanent-failure branch: one worker pickup, a provisioning_permanent_failure transition, and terminal failed with the actionable message on the task row. The classification stays narrow; other auth errors keep their existing retry semantics. Covered by a dedicated e2e (provisioning-no-secret.e2e.test.ts) that boots the real API server with no secrets seeded and proves the task lands in failed with no provisioning_retry events, staying failed across reconciler cycles. --- .../e2e/provisioning-no-secret.e2e.test.ts | 129 ++++++++++++++++++ packages/shared/src/error-classifier.test.ts | 6 +- packages/shared/src/error-classifier.ts | 13 +- 3 files changed, 144 insertions(+), 4 deletions(-) create mode 100644 apps/api/e2e/provisioning-no-secret.e2e.test.ts diff --git a/apps/api/e2e/provisioning-no-secret.e2e.test.ts b/apps/api/e2e/provisioning-no-secret.e2e.test.ts new file mode 100644 index 00000000..fa98c4a3 --- /dev/null +++ b/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( + 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 { + const { body } = await api<{ task: TaskRow }>(`/api/tasks/${taskId}`); + return body.task; +} + +async function getEvents(taskId: string): Promise { + 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); + }); +}); diff --git a/packages/shared/src/error-classifier.test.ts b/packages/shared/src/error-classifier.test.ts index 782e5a7b..55ecb7d3 100644 --- a/packages/shared/src/error-classifier.test.ts +++ b/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", () => { diff --git a/packages/shared/src/error-classifier.ts b/packages/shared/src/error-classifier.ts index 2997bb54..95c9f351 100644 --- a/packages/shared/src/error-classifier.ts +++ b/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, …) From f032643b96513d22421c2acd221ce06a531472a4 Mon Sep 17 00:00:00 2001 From: Jon Wiggins Date: Sun, 2 Aug 2026 17:13:21 -0600 Subject: [PATCH 2/2] fix(tasks): return the post-transition row from POST /api/tasks The repo-task branch of POST /api/tasks inserted the task, transitioned it to queued (or waiting_on_deps), enqueued the BullMQ job, and then responded with the stale row from createTask() - state "pending", a state the task had already left. Clients acting on the response immediately saw the wrong state. transitionTask() already returns the post-transition row, so respond with that instead of re-fetching. The e2e assertion that pinned the old behavior (repo-task.e2e.test.ts expected "pending" with a comment noting it was the PRE-transition row) now asserts "queued", and the route unit tests assert the response state for both the queued and waiting_on_deps paths. --- apps/api/e2e/repo-task.e2e.test.ts | 9 ++++----- apps/api/src/routes/tasks.test.ts | 18 +++++++++++++----- apps/api/src/routes/tasks.ts | 13 +++++++++---- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/apps/api/e2e/repo-task.e2e.test.ts b/apps/api/e2e/repo-task.e2e.test.ts index ee31ffc4..8e61c327 100644 --- a/apps/api/e2e/repo-task.e2e.test.ts +++ b/apps/api/e2e/repo-task.e2e.test.ts @@ -118,11 +118,10 @@ async function createTask(title: string): Promise { }), }); 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; } diff --git a/apps/api/src/routes/tasks.test.ts b/apps/api/src/routes/tasks.test.ts index 1bf8e9bb..7fe15e13 100644 --- a/apps/api/src/routes/tasks.test.ts +++ b/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 () => { diff --git a/apps/api/src/routes/tasks.ts b/apps/api/src/routes/tasks.ts index fa0e43cc..15097388 100644 --- a/apps/api/src/routes/tasks.ts +++ b/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,8 +546,12 @@ 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", @@ -554,7 +559,7 @@ export async function taskRoutes(rawApp: FastifyInstance) { 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) } }); }, );