feat(review): retry transient acpx failures + pass --prompt-retries

CHANGELOG.md

@@ -5,6 +5,7 @@
 - Added `clawpatch ci` to initialize, map, review, write a report, and append a GitHub Actions step summary in one CI-friendly command.
 - Added `clawpatch open-pr --patch <id>` to turn an applied patch attempt into an explicit GitHub pull request.
 - Added review prompt provenance and budget accounting for included files, omitted files, prompt bytes, and approximate tokens.
+- Added retries for transient acpx JSON review failures via `--prompt-retries` and `CLAWPATCH_REVIEW_RETRIES`, thanks @coletebou.
 - Hardened review ingestion so provider findings must cite included files with valid line ranges and matching evidence quotes.
 - Fixed `clawpatch open-pr` so repositories without default-branch metadata use a dedicated patch branch and let GitHub choose the PR base.
 - Fixed `clawpatch open-pr` retries to push the recorded patch commit instead of any later local branch tip.

src/app.test.ts

@@ -0,0 +1,267 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { __testing as appTesting, AppContext } from "./app.js";
+import { ClawpatchError } from "./errors.js";
+import type { ReviewOutput } from "./types.js";
+
+// eslint-disable-next-line no-underscore-dangle
+const { isRetryableReviewError, reviewRetries, runProviderReviewWithRetry } = appTesting;
+
+const QUIET_CONTEXT: AppContext = {
+  root: "/tmp/test-root",
+  options: {
+    root: "/tmp/test-root",
+    json: false,
+    plain: false,
+    quiet: true,
+    verbose: false,
+    debug: false,
+    noColor: true,
+    noInput: true,
+  },
+};
+
+function emptyReview(): ReviewOutput {
+  return { findings: [], inspected: { files: [], symbols: [], notes: ["ok"] } };
+}
+
+function withEnv(name: string, value: string | undefined, fn: () => void): void {
+  const previous = process.env[name];
+  if (value === undefined) {
+    delete process.env[name];
+  } else {
+    process.env[name] = value;
+  }
+  try {
+    fn();
+  } finally {
+    if (previous === undefined) {
+      delete process.env[name];
+    } else {
+      process.env[name] = previous;
+    }
+  }
+}
+
+describe("isRetryableReviewError", () => {
+  it("returns true for malformed-output ClawpatchError", () => {
+    expect(isRetryableReviewError(new ClawpatchError("bad", 8, "malformed-output"))).toBe(true);
+  });
+
+  it("returns false for provider-auth", () => {
+    expect(isRetryableReviewError(new ClawpatchError("nope", 4, "provider-auth"))).toBe(false);
+  });
+
+  it("returns false for unsupported-provider", () => {
+    expect(isRetryableReviewError(new ClawpatchError("nope", 2, "unsupported-provider"))).toBe(
+      false,
+    );
+  });
+
+  it("returns false for agent-refused", () => {
+    expect(isRetryableReviewError(new ClawpatchError("nope", 1, "agent-refused"))).toBe(false);
+  });
+
+  it("returns false for agent-cancelled", () => {
+    expect(isRetryableReviewError(new ClawpatchError("nope", 1, "agent-cancelled"))).toBe(false);
+  });
+
+  it("returns false for provider-failure (acpx-layer handles those)", () => {
+    expect(isRetryableReviewError(new ClawpatchError("nope", 1, "provider-failure"))).toBe(false);
+  });
+
+  it("returns false for plain Error", () => {
+    expect(isRetryableReviewError(new Error("oops"))).toBe(false);
+  });
+});
+
+describe("reviewRetries", () => {
+  afterEach(() => {
+    delete process.env["CLAWPATCH_REVIEW_RETRIES"];
+  });
+
+  it("defaults to 1", () => {
+    delete process.env["CLAWPATCH_REVIEW_RETRIES"];
+    expect(reviewRetries()).toBe(1);
+  });
+
+  it("respects 0 (disables retry)", () => {
+    withEnv("CLAWPATCH_REVIEW_RETRIES", "0", () => {
+      expect(reviewRetries()).toBe(0);
+    });
+  });
+
+  it("respects positive integer override", () => {
+    withEnv("CLAWPATCH_REVIEW_RETRIES", "2", () => {
+      expect(reviewRetries()).toBe(2);
+    });
+  });
+
+  it("falls back to 1 on garbage input", () => {
+    withEnv("CLAWPATCH_REVIEW_RETRIES", "abc", () => {
+      expect(reviewRetries()).toBe(1);
+    });
+  });
+});
+
+function fakeProvider(reviewImpl: (...args: unknown[]) => Promise<ReviewOutput>): {
+  name: string;
+  check: () => Promise<string>;
+  map: () => Promise<never>;
+  review: (...args: unknown[]) => Promise<ReviewOutput>;
+  fix: () => Promise<never>;
+  revalidate: () => Promise<never>;
+} {
+  return {
+    name: "fake",
+    async check(): Promise<string> {
+      return "fake";
+    },
+    async map(): Promise<never> {
+      throw new Error("not used");
+    },
+    review: reviewImpl,
+    async fix(): Promise<never> {
+      throw new Error("not used");
+    },
+    async revalidate(): Promise<never> {
+      throw new Error("not used");
+    },
+  };
+}
+
+describe("runProviderReviewWithRetry", () => {
+  afterEach(() => {
+    delete process.env["CLAWPATCH_REVIEW_RETRIES"];
+  });
+
+  it("returns output on first try without retry", async () => {
+    delete process.env["CLAWPATCH_REVIEW_RETRIES"];
+    const review = vi.fn().mockResolvedValue(emptyReview());
+    const provider = fakeProvider(review);
+    const result = await runProviderReviewWithRetry({
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      provider: provider as any,
+      root: "/tmp",
+      prompt: "hi",
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      options: {} as any,
+      context: QUIET_CONTEXT,
+      featureId: "feat_x",
+      index: 0,
+      total: 1,
+    });
+    expect(result.findings).toEqual([]);
+    expect(review).toHaveBeenCalledTimes(1);
+  });
+
+  it("retries once on malformed-output then succeeds", async () => {
+    delete process.env["CLAWPATCH_REVIEW_RETRIES"];
+    const review = vi
+      .fn()
+      .mockRejectedValueOnce(new ClawpatchError("garbled", 8, "malformed-output"))
+      .mockResolvedValueOnce(emptyReview());
+    const provider = fakeProvider(review);
+    const result = await runProviderReviewWithRetry({
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      provider: provider as any,
+      root: "/tmp",
+      prompt: "hi",
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      options: {} as any,
+      context: QUIET_CONTEXT,
+      featureId: "feat_x",
+      index: 0,
+      total: 1,
+    });
+    expect(result.findings).toEqual([]);
+    expect(review).toHaveBeenCalledTimes(2);
+  });
+
+  it("does NOT retry on provider-auth", async () => {
+    delete process.env["CLAWPATCH_REVIEW_RETRIES"];
+    const err = new ClawpatchError("auth", 4, "provider-auth");
+    const review = vi.fn().mockRejectedValue(err);
+    const provider = fakeProvider(review);
+    await expect(
+      runProviderReviewWithRetry({
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        provider: provider as any,
+        root: "/tmp",
+        prompt: "hi",
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        options: {} as any,
+        context: QUIET_CONTEXT,
+        featureId: "feat_x",
+        index: 0,
+        total: 1,
+      }),
+    ).rejects.toBe(err);
+    expect(review).toHaveBeenCalledTimes(1);
+  });
+
+  it("does NOT retry on agent-cancelled", async () => {
+    delete process.env["CLAWPATCH_REVIEW_RETRIES"];
+    const err = new ClawpatchError("cancel", 1, "agent-cancelled");
+    const review = vi.fn().mockRejectedValue(err);
+    const provider = fakeProvider(review);
+    await expect(
+      runProviderReviewWithRetry({
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        provider: provider as any,
+        root: "/tmp",
+        prompt: "hi",
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        options: {} as any,
+        context: QUIET_CONTEXT,
+        featureId: "feat_x",
+        index: 0,
+        total: 1,
+      }),
+    ).rejects.toBe(err);
+    expect(review).toHaveBeenCalledTimes(1);
+  });
+
+  it("respects CLAWPATCH_REVIEW_RETRIES=0 (no retry, malformed-output fails on first attempt)", async () => {
+    process.env["CLAWPATCH_REVIEW_RETRIES"] = "0";
+    const err = new ClawpatchError("garbled", 8, "malformed-output");
+    const review = vi.fn().mockRejectedValue(err);
+    const provider = fakeProvider(review);
+    await expect(
+      runProviderReviewWithRetry({
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        provider: provider as any,
+        root: "/tmp",
+        prompt: "hi",
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        options: {} as any,
+        context: QUIET_CONTEXT,
+        featureId: "feat_x",
+        index: 0,
+        total: 1,
+      }),
+    ).rejects.toBe(err);
+    expect(review).toHaveBeenCalledTimes(1);
+  });
+
+  it("re-throws after maxAttempts when malformed-output persists", async () => {
+    process.env["CLAWPATCH_REVIEW_RETRIES"] = "1";
+    const err = new ClawpatchError("garbled", 8, "malformed-output");
+    const review = vi.fn().mockRejectedValue(err);
+    const provider = fakeProvider(review);
+    await expect(
+      runProviderReviewWithRetry({
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        provider: provider as any,
+        root: "/tmp",
+        prompt: "hi",
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        options: {} as any,
+        context: QUIET_CONTEXT,
+        featureId: "feat_x",
+        index: 0,
+        total: 1,
+      }),
+    ).rejects.toBe(err);
+    expect(review).toHaveBeenCalledTimes(2);
+  });
+});

src/app.ts

@@ -675,11 +675,16 @@ async function reviewFeature(options: ReviewFeatureOptions): Promise<{ findingId
       mode,
       customPrompt,
     );
-    const providerOutput = await provider.review(
-      loaded.root,
-      reviewPrompt.prompt,
-      providerOptions(config),
-    );
+    const providerOutput = await runProviderReviewWithRetry({
+      provider,
+      root: loaded.root,
+      prompt: reviewPrompt.prompt,
+      options: providerOptions(config),
+      context,
+      featureId: feature.featureId,
+      index,
+      total,
+    });
     const reviewOutput = {
       ...providerOutput,
       findings: reviewFindingsForMode(providerOutput.findings, mode).slice(
@@ -773,6 +778,55 @@ async function reviewFeature(options: ReviewFeatureOptions): Promise<{ findingId
   }
 }
 
+type ReviewProvider = ReturnType<typeof providerByName>;
+type ProviderReviewOutput = Awaited<ReturnType<ReviewProvider["review"]>>;
+
+async function runProviderReviewWithRetry(args: {
+  provider: ReviewProvider;
+  root: string;
+  prompt: string;
+  options: Parameters<ReviewProvider["review"]>[2];
+  context: AppContext;
+  featureId: string;
+  index: number;
+  total: number;
+}): Promise<ProviderReviewOutput> {
+  const { provider, root, prompt, options, context, featureId, index, total } = args;
+  const maxAttempts = 1 + reviewRetries();
+  let lastError: unknown;
+  for (let attempt = 1; attempt <= maxAttempts; attempt += 1) {
+    try {
+      return await provider.review(root, prompt, options);
+    } catch (error: unknown) {
+      lastError = error;
+      if (!isRetryableReviewError(error) || attempt === maxAttempts) {
+        throw error;
+      }
+      emitProgress(context, "review", "feature-retry", {
+        index: index + 1,
+        total,
+        feature: featureId,
+        attempt,
+        reason: error instanceof ClawpatchError ? error.code : "unknown",
+      });
+    }
+  }
+  throw lastError ?? new ClawpatchError("review retry exhausted", 1, "review-retry-exhausted");
+}
+
+function reviewRetries(): number {
+  const raw = process.env["CLAWPATCH_REVIEW_RETRIES"];
+  if (raw === undefined) {
+    return 1;
+  }
+  const parsed = Number(raw);
+  return Number.isFinite(parsed) && parsed >= 0 ? Math.floor(parsed) : 1;
+}
+
+function isRetryableReviewError(error: unknown): boolean {
+  return error instanceof ClawpatchError && error.code === "malformed-output";
+}
+
 export async function revalidateCommand(
   context: AppContext,
   flags: Record<string, string | boolean>,
@@ -2031,3 +2085,10 @@ function stringFlag(flags: Record<string, string | boolean>, name: string): stri
   const value = flags[name];
   return typeof value === "string" ? value : undefined;
 }
+
+// eslint-disable-next-line no-underscore-dangle
+export const __testing = {
+  isRetryableReviewError,
+  reviewRetries,
+  runProviderReviewWithRetry,
+};