fix(provider): partition invalid review findings instead of failing the whole feature

CHANGELOG.md

@@ -6,6 +6,8 @@
 - 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.
 - Hardened review ingestion so provider findings must cite included files with valid line ranges and matching evidence quotes.
+- Fixed provider review to preserve valid findings when a sibling finding fails per-finding schema validation. Previously a single bad enum value caused the whole feature to lose every finding via a strict `reviewOutputSchema.parse` throw. Dropped findings are now recorded in `run.errors` with `code: "schema-drop"` and do not fail the run.
+- Fixed review evidence validation to drop only the offending finding when a quote, line range, or evidence file is rejected by `validateReviewOutput`, instead of failing the whole feature. Dropped findings are recorded in `run.errors` with `code: "validation-drop"` and do not fail the run.
 - 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.
 - Fixed first-time `clawpatch open-pr` branch creation to start from the recorded patch base.

src/app.ts

@@ -22,7 +22,7 @@ import { stableId, runId } from "./id.js";
 import { mapWithSource } from "./agent-mapper.js";
 import { mapFeatures } from "./mapper.js";
 import { emitProgress } from "./progress.js";
-import { providerByName } from "./provider.js";
+import { providerByName, type DroppedFinding } from "./provider.js";
 import { buildFixPrompt, buildReviewPromptBundle, buildRevalidatePrompt } from "./prompt.js";
 import type { ReviewMode, ReviewPromptManifest } from "./prompt.js";
 import {
@@ -32,7 +32,7 @@ import {
   renderFindingDetail,
   renderReport,
 } from "./reporting.js";
-import { validateReviewOutput } from "./review-validation.js";
+import { validateReviewOutputPartitioned } from "./review-validation.js";
 import {
   filterFeaturesByChangedFiles,
   filterFeaturesByProject,
@@ -343,6 +343,16 @@ export async function reviewCommand(
             allowNonPendingFeatureReview: stringFlag(flags, "feature") !== undefined,
           });
           findingIds.push(...reviewed.findingIds);
+          for (const dropped of reviewed.droppedFindings) {
+            const code = dropped.layer === "validation" ? "validation-drop" : "schema-drop";
+            errors.push({
+              message:
+                `dropped 1 finding from feature ${feature.featureId} ` +
+                `at ${dropped.path.join(".")}: ${dropped.message}`,
+              code,
+              error: null,
+            });
+          }
         } catch (error: unknown) {
           errors.push({
             message: error instanceof Error ? error.message : String(error),
@@ -353,7 +363,10 @@ export async function reviewCommand(
       }
     }),
   );
-  if (errors.length > 0) {
+  const fatalErrors = errors.filter(
+    (entry) => entry.code !== "schema-drop" && entry.code !== "validation-drop",
+  );
+  if (fatalErrors.length > 0) {
     await writeRun(loaded.paths, {
       ...run,
       status: "failed",
@@ -363,15 +376,16 @@ export async function reviewCommand(
     });
     emitProgress(context, "review", "failed", {
       run: currentRunId,
-      errors: errors.length,
+      errors: fatalErrors.length,
     });
-    throw errors[0]?.error ?? new ClawpatchError("review failed", 1, "review-failed");
+    throw fatalErrors[0]?.error ?? new ClawpatchError("review failed", 1, "review-failed");
   }
   const finished: RunRecord = {
     ...run,
     status: "completed",
     finishedAt: nowIso(),
     findingIds,
+    errors: errors.map(({ message, code }) => ({ message, code })),
   };
   await writeRun(loaded.paths, finished);
   emitProgress(context, "review", "done", {
@@ -635,7 +649,9 @@ type ReviewFeatureOptions = {
   allowNonPendingFeatureReview: boolean;
 };
 
-async function reviewFeature(options: ReviewFeatureOptions): Promise<{ findingIds: string[] }> {
+async function reviewFeature(
+  options: ReviewFeatureOptions,
+): Promise<{ findingIds: string[]; droppedFindings: DroppedFinding[] }> {
   const {
     context,
     loaded,
@@ -680,21 +696,27 @@ async function reviewFeature(options: ReviewFeatureOptions): Promise<{ findingId
       reviewPrompt.prompt,
       providerOptions(config),
     );
+    // Layer 1 drops: per-finding schema violations from parseReviewOutput.
+    const droppedFindings: DroppedFinding[] = [...providerOutput.droppedFindings];
     const reviewOutput = {
-      ...providerOutput,
       findings: reviewFindingsForMode(providerOutput.findings, mode).slice(
         0,
         config.review.maxFindingsPerFeature,
       ),
+      inspected: providerOutput.inspected,
     };
-    const output = await validateReviewOutput(
+    // Layer 2 drops: per-finding evidence validation (line ranges, quotes,
+    // included files). Partition so a single bad finding doesn't lose the
+    // whole feature.
+    const validated = await validateReviewOutputPartitioned(
       loaded.root,
       lockedFeature,
       config,
       reviewPrompt.manifest,
       reviewOutput,
     );
-    const records = output.findings.map((finding) =>
+    droppedFindings.push(...validated.droppedFindings);
+    const records = validated.findings.map((finding) =>
       findingFromOutput(finding, lockedFeature.featureId, currentRunId),
     );
     const findingIds: string[] = [];
@@ -735,7 +757,7 @@ async function reviewFeature(options: ReviewFeatureOptions): Promise<{ findingId
       findings: findingIds.length,
       elapsed: `${Math.round((Date.now() - started) / 1000)}s`,
     });
-    return { findingIds };
+    return { findingIds, droppedFindings };
   } catch (error: unknown) {
     const message = error instanceof Error ? error.message : String(error);
     if (locked !== null) {

src/provider.test.ts

@@ -14,10 +14,28 @@ const {
   extractOpencodeJson,
   parseAcpxAgent,
   parseCodexJson,
+  parseReviewOutput,
   piThinkingLevel,
   providerJsonSchema,
 } = __testing;
 
+function makeFinding(overrides: Record<string, unknown> = {}): Record<string, unknown> {
+  return {
+    title: "Sample finding",
+    category: "bug",
+    severity: "medium",
+    confidence: "high",
+    evidence: [],
+    reasoning: "Sample reasoning.",
+    reproduction: null,
+    recommendation: "Sample recommendation.",
+    whyTestsDoNotAlreadyCoverThis: "Tests do not encode this case.",
+    suggestedRegressionTest: null,
+    minimumFixScope: "Touch only the offending line.",
+    ...overrides,
+  };
+}
+
 function updateEnvelope(update: object): string {
   return JSON.stringify({
     jsonrpc: "2.0",
@@ -555,6 +573,85 @@ describe("extractOpencodeJson", () => {
   });
 });
 
+describe("parseReviewOutput", () => {
+  it("preserves all findings when every finding is valid (fast path)", () => {
+    const output = {
+      findings: [
+        makeFinding({ title: "first", category: "bug" }),
+        makeFinding({ title: "second", category: "security" }),
+        makeFinding({ title: "third", category: "performance" }),
+      ],
+      inspected: { files: ["src/a.ts"], symbols: [], notes: [] },
+    };
+
+    const result = parseReviewOutput(output);
+
+    expect(result.findings).toHaveLength(3);
+    expect(result.findings.map((f) => f.title)).toEqual(["first", "second", "third"]);
+    expect(result.droppedFindings).toEqual([]);
+    expect(result.inspected.files).toEqual(["src/a.ts"]);
+  });
+
+  it("keeps valid siblings when one finding has an invalid category", () => {
+    const output = {
+      findings: [
+        makeFinding({ title: "first", category: "bug" }),
+        makeFinding({ title: "second", category: "security" }),
+        makeFinding({ title: "third", category: "quality" }), // invalid enum value
+        makeFinding({ title: "fourth", category: "performance" }),
+      ],
+      inspected: { files: [], symbols: [], notes: [] },
+    };
+
+    const result = parseReviewOutput(output);
+
+    expect(result.findings).toHaveLength(3);
+    expect(result.findings.map((f) => f.title)).toEqual(["first", "second", "fourth"]);
+    expect(result.droppedFindings).toHaveLength(1);
+    const dropped = result.droppedFindings[0]!;
+    expect(dropped.path[0]).toBe("findings");
+    expect(dropped.path[1]).toBe(2);
+    expect(dropped.path).toContain("category");
+    expect(dropped.message).toBeTypeOf("string");
+    expect(dropped.sample).toContain("quality");
+    expect(dropped.sample.length).toBeLessThanOrEqual(200);
+  });
+
+  it("throws ClawpatchError when findings is not an array", () => {
+    const output = {
+      findings: "not-an-array",
+      inspected: { files: [], symbols: [], notes: [] },
+    };
+
+    try {
+      parseReviewOutput(output);
+    } catch (err) {
+      expect(err).toBeInstanceOf(ClawpatchError);
+      expect((err as ClawpatchError).code).toBe("malformed-output");
+      expect((err as ClawpatchError).exitCode).toBe(8);
+      expect((err as Error).message).toMatch(/findings/u);
+      return;
+    }
+    throw new Error("expected parseReviewOutput to throw on non-array findings");
+  });
+
+  it("truncates oversized samples to 200 characters", () => {
+    const longTitle = "x".repeat(500);
+    const output = {
+      findings: [
+        makeFinding({ title: longTitle, category: "quality" }), // invalid → dropped
+      ],
+      inspected: { files: [], symbols: [], notes: [] },
+    };
+
+    const result = parseReviewOutput(output);
+
+    expect(result.droppedFindings).toHaveLength(1);
+    expect(result.droppedFindings[0]!.sample.length).toBeLessThanOrEqual(200);
+    expect(result.droppedFindings[0]!.sample.endsWith("...")).toBe(true);
+  });
+});
+
 describe("providerByName", () => {
   it("returns provider instances for optional CLI-backed providers", () => {
     expect(providerByName("acpx").name).toBe("acpx");