From 0a2107716f362dd70f9427f013a9534913651da5 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 15 Jun 2026 21:54:55 -0700 Subject: [PATCH 1/3] Improve tracker fidelity: statusCategory blockers, statusMap round-trip, Jira pagination, config-aware retry, 4xx fast-fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes a group of tracker-fidelity audit findings: - B11: Jira blocker "done" detection now uses the statusCategory key ("done") instead of comparing the human-readable status name against a hardcoded Done/Closed/Resolved list, so dependents aren't starved when the blocker's done status has a custom name. Mirrors LinearTracker's canonical state-type check. - B17: findIssueByIdentifier reverse-maps the raw Jira status name back to the internal critter status via statusMap, so the webhook trigger compare (issueMatchesTrigger -> statusName === trigger.status) agrees with the poll path, which queries by the forward-mapped name. - F2: Jira findIssues now paginates through nextPageToken until isLast (capped at MAX_PAGINATED_ISSUES, mirroring Linear), adds a stable ORDER BY created ASC, and logs when the cap is hit. - B12: single-issue runRetry is now config-aware — it identifies the matching critter type by trigger label and uses that type's provider, trigger.status, and outcomes.failure?.status instead of hardcoding "Todo" and the default-provider tracker, mirroring runRetryAllFailed. - 4xx fast-fail: findIssues (Jira + Linear) passes a shouldRetry predicate (isTransientTaskError) so non-retryable 4xx errors fail fast instead of burning the full retry budget. Adds src/__tests__/audit-trackers.test.ts covering statusCategory-based blocker detection, the statusMap webhook round-trip, Jira pagination/ORDER BY, 4xx fast-fail, and config-aware retry target selection. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/__tests__/audit-trackers.test.ts | 307 +++++++++++++++++++++++++++ src/cli-retry.ts | 83 +++++--- src/tracker/jira.ts | 92 ++++++-- src/tracker/linear.ts | 4 + 4 files changed, 443 insertions(+), 43 deletions(-) create mode 100644 src/__tests__/audit-trackers.test.ts diff --git a/src/__tests__/audit-trackers.test.ts b/src/__tests__/audit-trackers.test.ts new file mode 100644 index 0000000..951e0ef --- /dev/null +++ b/src/__tests__/audit-trackers.test.ts @@ -0,0 +1,307 @@ +import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { JiraTracker } from "../tracker/jira.js"; +import type { IssueTracker, IssueTrackerIssue, TrackerTask } from "../tracker/types.js"; + +// ── Jira fetch mock (used by B11 / B17 / F2 / 4xx tests) ───────────────────── +const originalFetch = globalThis.fetch; +let mockFetchFn: ReturnType; + +beforeEach(() => { + mockFetchFn = mock(() => + Promise.resolve(new Response(JSON.stringify({}), { status: 200 })), + ); + globalThis.fetch = mockFetchFn as unknown as typeof fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +function mockResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +// ── B11: blocker "done" detection via statusCategory ───────────────────────── +describe("B11 — Jira blocker detection uses statusCategory", () => { + const tracker = new JiraTracker("co.atlassian.net", "u@e.com", "tok"); + + function blockedIssueResponse(blockerStatus: { name: string; statusCategory?: { key: string } }) { + return mockResponse({ + issues: [ + { + id: "10002", + key: "PROJ-43", + fields: { + summary: "Blocked task", + description: null, + labels: ["Critter"], + project: { id: "10000", key: "PROJ", name: "My Project" }, + issuelinks: [ + { + type: { name: "Blocks", inward: "is blocked by", outward: "blocks" }, + inwardIssue: { key: "PROJ-40", fields: { status: blockerStatus } }, + }, + ], + }, + renderedFields: { description: "" }, + }, + ], + isLast: true, + }); + } + + test("excludes a blocker whose statusCategory is 'done' even with a custom status name", async () => { + // A custom done status named "Shipped to Prod" — old name-based code would + // have (wrongly) treated this as still blocking. + mockFetchFn.mockResolvedValueOnce( + blockedIssueResponse({ name: "Shipped to Prod", statusCategory: { key: "done" } }), + ); + + const tasks = await tracker.findIssues({ label: "Critter", status: "To Do" }); + + expect(tasks).toHaveLength(1); + expect(tasks[0].blockedBy).toBeUndefined(); + }); + + test("includes a blocker whose statusCategory is not 'done'", async () => { + mockFetchFn.mockResolvedValueOnce( + blockedIssueResponse({ name: "In Progress", statusCategory: { key: "indeterminate" } }), + ); + + const tasks = await tracker.findIssues({ label: "Critter", status: "To Do" }); + + expect(tasks[0].blockedBy).toEqual([{ identifier: "PROJ-40", status: "In Progress" }]); + }); + + test("treats a blocker with missing statusCategory as still blocking (safe default)", async () => { + mockFetchFn.mockResolvedValueOnce(blockedIssueResponse({ name: "Mystery" })); + + const tasks = await tracker.findIssues({ label: "Critter", status: "To Do" }); + + expect(tasks[0].blockedBy).toEqual([{ identifier: "PROJ-40", status: "Mystery" }]); + }); +}); + +// ── B17: statusMap webhook round-trip ──────────────────────────────────────── +describe("B17 — findIssueByIdentifier reverse-maps statusMap", () => { + const tracker = new JiraTracker("co.atlassian.net", "u@e.com", "tok", { + Todo: "To Do", + "In Progress": "Working", + }); + + test("reverse-maps the raw Jira status name back to the internal critter status", async () => { + mockFetchFn.mockResolvedValueOnce( + mockResponse({ + id: "1", + key: "PROJ-1", + fields: { + status: { name: "To Do" }, + labels: ["Critter"], + project: { id: "10000", key: "PROJ", name: "My Project" }, + }, + }), + ); + + const issue = await tracker.findIssueByIdentifier("PROJ-1"); + + // Internal status "Todo" — matches a trigger configured with status: "Todo" + // (the webhook path compares issue.statusName === trigger.status). + expect(issue?.statusName).toBe("Todo"); + }); + + test("round-trips: poll forward-maps to JQL, findIssueByIdentifier reverse-maps back", async () => { + // Poll path: trigger status "Todo" must be forward-mapped to "To Do" in JQL. + mockFetchFn.mockResolvedValueOnce(mockResponse({ issues: [], isLast: true })); + await tracker.findIssues({ label: "Critter", status: "Todo" }); + const searchBody = JSON.parse((mockFetchFn.mock.calls[0] as [string, RequestInit])[1].body as string); + expect(searchBody.jql).toContain('status = "To Do"'); + + // Webhook path: the same issue resolves back to the internal "Todo". + mockFetchFn.mockResolvedValueOnce( + mockResponse({ + id: "1", + key: "PROJ-1", + fields: { + status: { name: "To Do" }, + labels: ["Critter"], + project: { id: "10000", key: "PROJ", name: "My Project" }, + }, + }), + ); + const issue = await tracker.findIssueByIdentifier("PROJ-1"); + expect(issue?.statusName).toBe("Todo"); + }); + + test("leaves unmapped status names untouched", async () => { + mockFetchFn.mockResolvedValueOnce( + mockResponse({ + id: "1", + key: "PROJ-1", + fields: { + status: { name: "Backlog" }, + labels: ["Critter"], + project: { id: "10000", key: "PROJ", name: "My Project" }, + }, + }), + ); + + const issue = await tracker.findIssueByIdentifier("PROJ-1"); + expect(issue?.statusName).toBe("Backlog"); + }); +}); + +// ── F2: pagination + ORDER BY ──────────────────────────────────────────────── +describe("F2 — Jira findIssues paginates and orders", () => { + const tracker = new JiraTracker("co.atlassian.net", "u@e.com", "tok"); + + function pageIssue(key: string) { + return { + id: key, + key, + fields: { + summary: key, + description: null, + labels: ["Critter"], + project: { id: "10000", key: "PROJ", name: "My Project" }, + issuelinks: [], + }, + renderedFields: { description: "" }, + }; + } + + test("follows nextPageToken until isLast", async () => { + mockFetchFn.mockResolvedValueOnce( + mockResponse({ issues: [pageIssue("PROJ-1")], isLast: false, nextPageToken: "tok-2" }), + ); + mockFetchFn.mockResolvedValueOnce( + mockResponse({ issues: [pageIssue("PROJ-2")], isLast: true }), + ); + + const tasks = await tracker.findIssues({ label: "Critter", status: "To Do" }); + + expect(tasks.map((t) => t.identifier)).toEqual(["PROJ-1", "PROJ-2"]); + expect(mockFetchFn).toHaveBeenCalledTimes(2); + // Second request carries the page cursor. + const secondBody = JSON.parse((mockFetchFn.mock.calls[1] as [string, RequestInit])[1].body as string); + expect(secondBody.nextPageToken).toBe("tok-2"); + }); + + test("adds a stable ORDER BY clause to the JQL", async () => { + mockFetchFn.mockResolvedValueOnce(mockResponse({ issues: [], isLast: true })); + + await tracker.findIssues({ label: "Critter", status: "To Do" }); + + const body = JSON.parse((mockFetchFn.mock.calls[0] as [string, RequestInit])[1].body as string); + expect(body.jql).toContain("ORDER BY created ASC"); + }); +}); + +// ── 4xx fast-fail ──────────────────────────────────────────────────────────── +describe("4xx fast-fail — findIssues does not retry non-transient errors", () => { + const tracker = new JiraTracker("co.atlassian.net", "u@e.com", "tok"); + + test("a 400 response fails immediately without retrying", async () => { + mockFetchFn.mockResolvedValue(new Response("bad jql", { status: 400 })); + + await expect(tracker.findIssues({ label: "Critter", status: "To Do" })).rejects.toThrow(); + // No retries: exactly one HTTP call. + expect(mockFetchFn).toHaveBeenCalledTimes(1); + }); +}); + +// ── B12: config-aware single-issue retry ───────────────────────────────────── +// Mock ONLY createTracker (not loadConfig — mocking the config module leaks into +// other test files). runRetry therefore runs against the real repo config, whose +// `review` type has a non-default trigger.status ("In Review") — exactly the case +// the hardcoded-"Todo" implementation got wrong. +const mockFindIssueByIdentifier = mock<() => Promise>(() => Promise.resolve(null)); +const mockUpdateStatus = mock(() => Promise.resolve()); +const mockComment = mock(() => Promise.resolve()); + +const retryTracker: IssueTracker = { + provider: "linear", + init: mock(() => Promise.resolve()), + findIssues: mock<() => Promise>(() => Promise.resolve([])), + findIssueByIdentifier: mockFindIssueByIdentifier, + updateStatus: mockUpdateStatus, + comment: mockComment, + getComments: mock(() => Promise.resolve([])), + uploadAttachment: mock(() => Promise.resolve(null)), + getAttachments: mock(() => Promise.resolve([])), + fetchAttachmentContent: mock(() => Promise.resolve(null)), + ensureStatus: mock(() => Promise.resolve()), + ensureLabel: mock(() => Promise.resolve()), + removeLabel: mock(() => Promise.resolve()), + createIssue: mock(() => Promise.reject(new Error("not implemented"))), + listTeams: mock(() => Promise.resolve([])), +}; + +mock.module("../tracker/index.js", () => ({ + createTracker: () => retryTracker, +})); + +process.env.LINEAR_API_KEY = process.env.LINEAR_API_KEY || "test-key"; + +const { runRetry } = await import("../cli-retry.js"); + +describe("B12 — runRetry is config-aware", () => { + beforeEach(() => { + mockFindIssueByIdentifier.mockReset(); + mockUpdateStatus.mockReset(); + mockComment.mockReset(); + spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit called"); + }); + spyOn(console, "error").mockImplementation(() => {}); + spyOn(console, "log").mockImplementation(() => {}); + }); + + // The repo config's `review` type: trigger { label "Critter Review", status + // "In Review" }, failure status "Critter Failed". + function makeReviewIssue(statusName: string): IssueTrackerIssue { + return { + id: "issue-9", + identifier: "REV-9", + statusName, + labels: ["Critter Review"], + groupId: "team-9", + }; + } + + test("retries to the matched type's trigger.status (In Review), not hardcoded 'Todo'", async () => { + mockFindIssueByIdentifier.mockResolvedValueOnce(makeReviewIssue("Critter Failed")); + + await runRetry("REV-9", false); + + // The review type's trigger status is "In Review" — the old hardcoded + // implementation would have set "Todo" here. + expect(mockUpdateStatus).toHaveBeenCalledWith("issue-9", "In Review", "team-9"); + expect(mockComment).toHaveBeenCalledWith("issue-9", "Retry triggered via CLI"); + }); + + test("is a no-op when already in the matched type's trigger status", async () => { + // "In Review" is the review type's trigger status, so this is a no-op — the + // old code treated "In Review" as "currently being worked on" and errored. + mockFindIssueByIdentifier.mockResolvedValueOnce(makeReviewIssue("In Review")); + + await runRetry("REV-9", false); + + expect(mockUpdateStatus).not.toHaveBeenCalled(); + }); + + test("rejects an issue lacking any configured trigger label", async () => { + mockFindIssueByIdentifier.mockResolvedValueOnce({ + id: "issue-9", + identifier: "REV-9", + statusName: "Critter Failed", + labels: ["Unrelated"], + groupId: "team-9", + }); + + await expect(runRetry("REV-9", false)).rejects.toThrow("process.exit called"); + expect(mockUpdateStatus).not.toHaveBeenCalled(); + }); +}); diff --git a/src/cli-retry.ts b/src/cli-retry.ts index c3d8897..172ee43 100644 --- a/src/cli-retry.ts +++ b/src/cli-retry.ts @@ -4,7 +4,7 @@ import { loadConfig } from "./config.js"; import type { CritterTypeConfig } from "./critter-type.js"; import { formatError } from "./logger.js"; import { createTracker } from "./tracker/index.js"; -import type { IssueTracker, TrackerTask } from "./tracker/types.js"; +import type { IssueTracker, IssueTrackerIssue, TrackerTask } from "./tracker/types.js"; function loadEnv(): void { const cwdEnv = "./.env"; @@ -30,40 +30,73 @@ export async function runRetry(identifier: string, force: boolean): Promise(); + const getTracker = (provider: string): IssueTracker => { + let tracker = trackerMap.get(provider); + if (!tracker) { + tracker = createTracker({ + type: provider as "linear" | "jira", + apiKey: config.linear.apiKey, + host: config.jira.host, + email: config.jira.email, + apiToken: config.jira.apiToken, + statusMap: config.jira.statusMap, + }); + trackerMap.set(provider, tracker); + } + return tracker; + }; + + // Cache issue lookups per provider so we never re-fetch the same provider. + const issueByProvider = new Map(); + const lookupIssue = async (provider: string, tracker: IssueTracker): Promise => { + if (!issueByProvider.has(provider)) { + issueByProvider.set(provider, await tracker.findIssueByIdentifier(identifier)); + } + return issueByProvider.get(provider) ?? null; + }; + + // Find the critter type whose trigger label this issue carries, using that + // type's provider. This keeps retry config-aware: the matched type drives the + // retry target status and the recognized failure status. + let matched: { type: CritterTypeConfig; tracker: IssueTracker; issue: IssueTrackerIssue } | null = null; + let foundIssue: IssueTrackerIssue | null = null; + for (const ct of config.critterTypes) { + const provider = ct.provider ?? config.provider; + const tracker = getTracker(provider); + const issue = await lookupIssue(provider, tracker); + if (!issue) continue; + foundIssue = issue; + if (issue.labels.includes(ct.trigger.label)) { + matched = { type: ct, tracker, issue }; + break; + } + } + + if (!foundIssue) { console.error(`Error: Issue ${identifier} not found.`); process.exit(1); } - // Check that the issue has at least one trigger label from the configured critter types - const triggerLabels = new Set(config.critterTypes.map((ct) => ct.trigger.label)); - const hasTriggerLabel = issue.labels.some((l) => triggerLabels.has(l)); - if (!hasTriggerLabel) { - const labelList = [...triggerLabels].map((l) => `"${l}"`).join(" or "); + if (!matched) { + const labelList = config.critterTypes.map((ct) => `"${ct.trigger.label}"`).join(" or "); console.error( `Error: ${identifier} isn't a critter task (missing ${labelList} label).`, ); process.exit(1); } + const { type, tracker, issue } = matched; + const retryStatus = type.trigger.status; + const failureStatus = type.outcomes.failure?.status ?? "Critter Failed"; const statusName = issue.statusName; - if (statusName === "Todo") { + if (statusName === retryStatus) { console.log( - `${identifier} is already in Todo — it will be picked up on the next poll.`, + `${identifier} is already in ${retryStatus} — it will be picked up on the next poll.`, ); return; } @@ -73,7 +106,7 @@ export async function runRetry(identifier: string, force: boolean): Promise= MAX_PAGINATED_ISSUES) { + cappedOut = true; + break; + } + } while (nextPageToken); + + if (cappedOut) { + log(`Warning: hit pagination cap of ${MAX_PAGINATED_ISSUES} Jira issues — some issues may be skipped`); + } const tasks: TrackerTask[] = []; - for (const issue of data.issues ?? []) { + for (const issue of issues) { const description = extractPlainText(issue.renderedFields?.description ?? "") || adfToPlainText(issue.fields.description); - // Find blockers from issue links + // Find blockers from issue links. Gate on the Jira statusCategory + // ("new" | "indeterminate" | "done") rather than the human-readable + // status name, so custom "done" status names don't starve dependents. + // Mirrors LinearTracker's canonical state-type check. const blockedBy: { identifier: string; status: string }[] = []; for (const link of issue.fields.issuelinks ?? []) { if (link.type.inward === "is blocked by" && link.inwardIssue) { - const blockerStatus = link.inwardIssue.fields?.status?.name; - if (blockerStatus && blockerStatus !== "Done" && blockerStatus !== "Closed" && blockerStatus !== "Resolved") { + const blockerStatus = link.inwardIssue.fields?.status; + if (blockerStatus && blockerStatus.statusCategory?.key !== "done") { blockedBy.push({ identifier: link.inwardIssue.key, - status: blockerStatus, + status: blockerStatus.name, }); } } @@ -90,6 +118,9 @@ export class JiraTracker implements IssueTracker { { maxRetries: 3, baseDelayMs: 2000, + // Only retry transient failures — non-retryable 4xx (auth, bad JQL) + // should fail fast instead of burning the full retry budget. + shouldRetry: (error) => isTransientTaskError(error instanceof Error ? error.message : String(error)), onRetry: (_error, attempt, delayMs) => { log(`findIssues (Jira) failed, retrying in ${Math.round(delayMs)}ms... (attempt ${attempt + 1}/3)`); }, @@ -108,7 +139,11 @@ export class JiraTracker implements IssueTracker { identifier: issue.key, title: issue.fields.summary, description, - statusName: issue.fields.status?.name ?? "Unknown", + // Reverse-map the raw Jira status name back to the internal critter + // status so the webhook compare (issueMatchesTrigger → statusName === + // trigger.status) agrees with the poll path, which queries by the + // forward-mapped name. Both directions use the same statusMap. + statusName: this.reverseMapStatusName(issue.fields.status?.name ?? "Unknown"), labels: issue.fields.labels ?? [], group: issue.fields.project.name, groupId: issue.fields.project.key, @@ -286,6 +321,18 @@ export class JiraTracker implements IssueTracker { return this.statusMap[name] ?? name; } + /** + * Reverse of {@link mapStatusName}: map a raw Jira status name back to the + * internal critter status name. Used so issues fetched by identifier report + * statuses in the same vocabulary the poll path queries with. + */ + private reverseMapStatusName(jiraName: string): string { + for (const [internal, jira] of Object.entries(this.statusMap)) { + if (jira === jiraName) return internal; + } + return jiraName; + } + private async request(path: string, init?: RequestInit): Promise { const resp = await fetch(`${this.baseUrl}${path}`, { ...init, @@ -310,7 +357,11 @@ export class JiraTracker implements IssueTracker { interface JiraSearchResponse { issues: JiraIssue[]; - total: number; + total?: number; + /** Cursor for the next page in Jira's enhanced search (/search/jql). */ + nextPageToken?: string; + /** True when this is the final page of results. */ + isLast?: boolean; } interface JiraIssue { @@ -330,10 +381,15 @@ interface JiraIssue { }; } +interface JiraLinkedStatus { + name: string; + statusCategory?: { key: string }; +} + interface JiraIssueLink { type: { name: string; inward: string; outward: string }; - inwardIssue?: { key: string; fields?: { status?: { name: string } } }; - outwardIssue?: { key: string; fields?: { status?: { name: string } } }; + inwardIssue?: { key: string; fields?: { status?: JiraLinkedStatus } }; + outwardIssue?: { key: string; fields?: { status?: JiraLinkedStatus } }; } interface JiraTransition { diff --git a/src/tracker/linear.ts b/src/tracker/linear.ts index 371ed86..d743a34 100644 --- a/src/tracker/linear.ts +++ b/src/tracker/linear.ts @@ -2,6 +2,7 @@ import { LinearClient } from "@linear/sdk"; import type { TriggerConfig } from "../critter-type.js"; import { log, logError, logTaskError } from "../logger.js"; import { withRetry } from "../retry.js"; +import { isTransientTaskError } from "../task-retry.js"; import type { CreatedIssue, CreateIssueInput, IssueTracker, IssueTrackerIssue, TrackerTask, TrackerTeam } from "./types.js"; const MAX_PAGINATED_ISSUES = 200; @@ -126,6 +127,9 @@ export class LinearTracker implements IssueTracker { { maxRetries: 3, baseDelayMs: 2000, + // Only retry transient failures — non-retryable 4xx (auth, bad request) + // should fail fast instead of burning the full retry budget. + shouldRetry: (error) => isTransientTaskError(error instanceof Error ? error.message : String(error)), onRetry: (_error, attempt, delayMs) => { log(`findIssues failed, retrying in ${Math.round(delayMs)}ms... (attempt ${attempt + 1}/3)`); }, From fb1351ac808ec5768707b007394c05bb918cc957 Mon Sep 17 00:00:00 2001 From: andrew Date: Tue, 16 Jun 2026 13:55:16 -0700 Subject: [PATCH 2/3] test: assert mcp config against homedir() to fix CI flakiness resolvePhaseMcpConfig expands ~ via node:os homedir(); the test compared it to process.env.HOME. Bun caches homedir() on first call and ignores later process.env.HOME mutations, so when a sibling test changes HOME they diverge on Linux CI and this test fails (works on macOS by execution-order luck). Assert against the same homedir() the implementation uses. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/__tests__/mcp.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/__tests__/mcp.test.ts b/src/__tests__/mcp.test.ts index 87ee767..02c3f7e 100644 --- a/src/__tests__/mcp.test.ts +++ b/src/__tests__/mcp.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { homedir } from "node:os"; import { ClaudeCodeAdapter } from "../cli/claude.js"; import { CodexAdapter } from "../cli/codex.js"; import { resolvePhaseMcpConfig } from "../cli/mcp.js"; @@ -25,7 +26,7 @@ describe("resolvePhaseMcpConfig", () => { } as Config, ); - expect(result.mcpConfig).toEqual([`${process.env.HOME}/.critters/review-mcp.json`]); + expect(result.mcpConfig).toEqual([`${homedir()}/.critters/review-mcp.json`]); expect(result.strictMcpConfig).toBe(true); }); From ec199a06ca8e0a1e702503af3d6c5eedb5b8bf3d Mon Sep 17 00:00:00 2001 From: andrew Date: Sat, 18 Jul 2026 10:45:20 -0700 Subject: [PATCH 3/3] fix: retry tracker fetch failures except definitive 4xx The 4xx fast-fail change used isTransientTaskError (a whitelist written for git/subprocess errors) as the shouldRetry predicate, which silently dropped retries for real transient failures: Bun/undici network errors ("Unable to connect...", "fetch failed") and Linear SDK 5xx messages ("Graphql error (code: 502)") match nothing in that regex, so three transient blips could open the circuit breaker and pause a provider. Invert to a blacklist: fail fast only on definitive 4xx, retry everything else. Adds regression tests asserting network-style and 5xx messages are retried and 400/401/403/404/422 fail fast. Co-Authored-By: Claude Fable 5 --- src/__tests__/audit-trackers.test.ts | 41 ++++++++++++++++++++++++++++ src/task-retry.ts | 10 +++++++ src/tracker/jira.ts | 8 +++--- src/tracker/linear.ts | 8 +++--- 4 files changed, 59 insertions(+), 8 deletions(-) diff --git a/src/__tests__/audit-trackers.test.ts b/src/__tests__/audit-trackers.test.ts index 951e0ef..7069e09 100644 --- a/src/__tests__/audit-trackers.test.ts +++ b/src/__tests__/audit-trackers.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { isPermanentTrackerError } from "../task-retry.js"; import { JiraTracker } from "../tracker/jira.js"; import type { IssueTracker, IssueTrackerIssue, TrackerTask } from "../tracker/types.js"; @@ -212,6 +213,46 @@ describe("4xx fast-fail — findIssues does not retry non-transient errors", () }); }); +// ── retry predicate: blacklist 4xx, retry everything else ──────────────────── +// Regression guard: the predicate must not whitelist "transient-looking" +// messages — real network errors and Linear 5xx messages look nothing like the +// git/subprocess error shapes and must still be retried. +describe("isPermanentTrackerError — only definitive client errors fail fast", () => { + test.each([ + // Bun fetch failure (connection refused / DNS) + "Unable to connect. Is the computer able to access the url?", + // Node/undici fetch failure + "fetch failed", + // Linear SDK 5xx / rate-limit message shapes + "Graphql error (code: 502)", + "Graphql error (code: 500)", + "Graphql error (Code: 429)", + // Jira 5xx / rate-limit message shapes + "Jira API error: 502 Bad Gateway", + "Jira API error: 503 Service Unavailable", + "Jira API error: 429 Too Many Requests", + "Jira API error: 500 Internal Server Error", + // Numeric red herrings that must NOT trip the 4xx matcher + "Jira API error: 503 Service Unavailable (request id 40401)", + "connect ECONNREFUSED 10.0.0.1:443", + ])("retries: %s", (message) => { + expect(isPermanentTrackerError(message)).toBe(false); + }); + + test.each([ + "Jira API error: 400 Bad Request", + "Graphql error (code: 400)", + "Graphql error (code: 401)", + "Jira API error: 401 Unauthorized", + "Jira API error: 403 Forbidden", + "Graphql error (code: 403)", + "Jira API error: 404 Not Found", + "Jira API error: 422 Unprocessable Entity", + ])("fails fast: %s", (message) => { + expect(isPermanentTrackerError(message)).toBe(true); + }); +}); + // ── B12: config-aware single-issue retry ───────────────────────────────────── // Mock ONLY createTracker (not loadConfig — mocking the config module leaks into // other test files). runRetry therefore runs against the real repo config, whose diff --git a/src/task-retry.ts b/src/task-retry.ts index b6e8f37..b99015b 100644 --- a/src/task-retry.ts +++ b/src/task-retry.ts @@ -4,6 +4,16 @@ export function isTransientTaskError(error: string): boolean { return TRANSIENT_ERROR_RE.test(error); } +// Tracker fetch errors are the inverse problem from task errors: there is no +// reliable whitelist of transient message shapes (Bun/undici network failures +// and Linear SDK 5xx messages look nothing like git errors), so fail fast only +// on definitive client errors and retry everything else. +const PERMANENT_TRACKER_ERROR_RE = /\b(400|401|403|404|422)\b/; + +export function isPermanentTrackerError(error: string): boolean { + return PERMANENT_TRACKER_ERROR_RE.test(error); +} + export function withCappedJitter(baseDelayMs: number, maxDelayMs: number, jitter = true): number { const base = Math.min(baseDelayMs, maxDelayMs); if (!jitter) return base; diff --git a/src/tracker/jira.ts b/src/tracker/jira.ts index d3b09a3..7c644d6 100644 --- a/src/tracker/jira.ts +++ b/src/tracker/jira.ts @@ -1,7 +1,7 @@ import type { TriggerConfig } from "../critter-type.js"; import { log, logError, logTaskError } from "../logger.js"; import { withRetry } from "../retry.js"; -import { isTransientTaskError } from "../task-retry.js"; +import { isPermanentTrackerError } from "../task-retry.js"; import type { CreatedIssue, CreateIssueInput, IssueTracker, IssueTrackerIssue, TrackerTask, TrackerTeam } from "./types.js"; /** Cap on the number of issues paginated through in a single findIssues call. */ @@ -118,9 +118,9 @@ export class JiraTracker implements IssueTracker { { maxRetries: 3, baseDelayMs: 2000, - // Only retry transient failures — non-retryable 4xx (auth, bad JQL) - // should fail fast instead of burning the full retry budget. - shouldRetry: (error) => isTransientTaskError(error instanceof Error ? error.message : String(error)), + // Fail fast only on definitive client errors (auth, bad JQL); + // retry everything else (network failures, 429, 5xx). + shouldRetry: (error) => !isPermanentTrackerError(error instanceof Error ? error.message : String(error)), onRetry: (_error, attempt, delayMs) => { log(`findIssues (Jira) failed, retrying in ${Math.round(delayMs)}ms... (attempt ${attempt + 1}/3)`); }, diff --git a/src/tracker/linear.ts b/src/tracker/linear.ts index d743a34..3d47303 100644 --- a/src/tracker/linear.ts +++ b/src/tracker/linear.ts @@ -2,7 +2,7 @@ import { LinearClient } from "@linear/sdk"; import type { TriggerConfig } from "../critter-type.js"; import { log, logError, logTaskError } from "../logger.js"; import { withRetry } from "../retry.js"; -import { isTransientTaskError } from "../task-retry.js"; +import { isPermanentTrackerError } from "../task-retry.js"; import type { CreatedIssue, CreateIssueInput, IssueTracker, IssueTrackerIssue, TrackerTask, TrackerTeam } from "./types.js"; const MAX_PAGINATED_ISSUES = 200; @@ -127,9 +127,9 @@ export class LinearTracker implements IssueTracker { { maxRetries: 3, baseDelayMs: 2000, - // Only retry transient failures — non-retryable 4xx (auth, bad request) - // should fail fast instead of burning the full retry budget. - shouldRetry: (error) => isTransientTaskError(error instanceof Error ? error.message : String(error)), + // Fail fast only on definitive client errors (auth, bad request); + // retry everything else (network failures, 429, 5xx). + shouldRetry: (error) => !isPermanentTrackerError(error instanceof Error ? error.message : String(error)), onRetry: (_error, attempt, delayMs) => { log(`findIssues failed, retrying in ${Math.round(delayMs)}ms... (attempt ${attempt + 1}/3)`); },