fix: correctly sequence review retries and follow up guidance

Author: jahilldev

api/src/orchestrator/orchestrator.service.ts

@@ -33,7 +33,11 @@ import { buildPrBody } from '../summary/summary.utility.js';
 import { WorkspaceService } from '../workspace/workspace.service.js';
 import { ReviewService } from '../review/review.service.js';
 import { buildReviewPrompt } from '../review/review.prompts.js';
-import { type ReviewIssue, type ReviewResult } from '../review/review.model.js';
+import {
+  type ReviewIssue,
+  type ReviewResult,
+  UNPARSEABLE_REVIEW_TITLE,
+} from '../review/review.model.js';
 import {
   failedDimensions,
   formatIssues,
@@ -1408,27 +1412,30 @@ export class OrchestratorService {
     const maxPasses = this.review.maxPasses;
     const cycle = job.reviewCycle;
 
-    // Count only passes in the current cycle so the cap applies per-cycle and task
-    // retries continue from where they left off rather than restarting from pass 1.
-    const [priorPasses, priorUnparseable] = await Promise.all([
-      this.prisma.reviewPass.count({ where: { jobId, cycle } }),
-      this.prisma.reviewPass.count({ where: { jobId, cycle, confidence: 0 } }),
-    ]);
-    const pass = priorPasses + 1;
-
-    // Fetch issues from the immediately preceding pass (if any) so the reviewer
-    // can explicitly verify each was resolved in addition to doing a full fresh review.
-    const priorPassRecord =
-      pass > 1
-        ? await this.prisma.reviewPass.findFirst({
-            where: { jobId, cycle },
-            orderBy: { passNumber: 'desc' },
-            select: { issues: true },
-          })
-        : null;
+    // One ordered read of this cycle's passes backs three things: the pass number, the
+    // immediately-preceding pass's issues (so the reviewer can verify each was resolved), and the
+    // count of *consecutive* unparseable passes right before this one. Consecutiveness matters —
+    // a single stray unparseable pass among otherwise-valid ones must not exhaust the retry budget,
+    // since the model has clearly shown it can still emit a valid verdict. Detected via the
+    // sentinel issue title rather than `confidence: 0` (a validly-parsed review can also lack a
+    // confidence and default to 0).
+    const cyclePasses = await this.prisma.reviewPass.findMany({
+      where: { jobId, cycle },
+      orderBy: { passNumber: 'desc' },
+      select: { issues: true },
+    });
+    const pass = cyclePasses.length + 1;
+
+    let priorUnparseable = 0;
+    for (const p of cyclePasses) {
+      if (!p.issues.includes(UNPARSEABLE_REVIEW_TITLE)) {
+        break;
+      }
+      priorUnparseable++;
+    }
 
-    const priorIssues = priorPassRecord
-      ? (JSON.parse(priorPassRecord.issues) as ReviewIssue[])
+    const priorIssues = cyclePasses[0]
+      ? (JSON.parse(cyclePasses[0].issues) as ReviewIssue[])
       : undefined;
 
     // Only include PR feedback submitted after the last IMPLEMENT run — older
@@ -1538,7 +1545,7 @@ export class OrchestratorService {
           issues: [
             {
               severity: 'high',
-              title: 'Unparseable review output',
+              title: UNPARSEABLE_REVIEW_TITLE,
               detail: res.stdout.slice(0, 800),
             },
           ],

api/src/review/review.model.ts

@@ -2,6 +2,13 @@ export type ReviewVerdict = 'PASS' | 'FAIL';
 
 export type IssueSeverity = 'low' | 'medium' | 'high' | 'critical';
 
+/**
+ * Title of the sentinel issue recorded when a review pass can't be parsed into a verdict. Used both
+ * to build that fallback issue and to detect unparseable passes in the retry cap — keep them in sync
+ * (and decoupled from `confidence: 0`, which a validly-parsed but confidence-less review can also be).
+ */
+export const UNPARSEABLE_REVIEW_TITLE = 'Unparseable review output';
+
 /**
  * Rubric dimensions the reviewer grades independently. Each is a hard gate: the
  * verdict can only be PASS if every dimension holds. Confidence is advisory only.

api/src/review/review.prompts.spec.ts

@@ -0,0 +1,30 @@
+import { buildReviewPrompt } from './review.prompts.js';
+import type { ReviewPromptContext } from './review.model.js';
+
+const baseCtx: ReviewPromptContext = {
+  repoFullName: 'o/r',
+  issueTitle: 'Title',
+  issueBody: 'Body',
+  plan: 'the plan',
+  baseBranch: 'main',
+  changedFiles: ['src/a.ts'],
+  threshold: 85,
+};
+
+describe('buildReviewPrompt parseRetry guidance', () => {
+  it('omits retry guidance on a first pass', () => {
+    const p = buildReviewPrompt(baseCtx);
+    expect(p).not.toContain('RETRY');
+  });
+
+  it('injects schema-conformance guidance when retrying after an unparseable pass', () => {
+    const p = buildReviewPrompt({ ...baseCtx, parseRetry: true });
+    expect(p).toContain('RETRY');
+    // The divergences that actually caused the failure must be called out by name.
+    expect(p).toContain('"PASS" or "FAIL"');
+    expect(p).toContain('NOT a boolean');
+    expect(p).toContain('`confidence` MUST be present');
+    expect(p).toContain('issues');
+    expect(p).toContain('rationale'); // explicitly warns against this stray key
+  });
+});

api/src/review/review.prompts.ts

@@ -88,8 +88,12 @@ If the server fails to start, skip the browser step and note it in your summary
 
   if (ctx.parseRetry) {
     parts.push(
-      `IMPORTANT: Your previous response did not contain a valid \`\`\`json block and could not be parsed. ` +
-        `This time you MUST start your response with the JSON block above — do not write any preamble or narrative before it.`,
+      `IMPORTANT — RETRY: your previous response could not be parsed against the required schema, so this review is being re-run. The verdict was discarded; none of that prior analysis was recorded. Conform EXACTLY this time:\n` +
+        `- Start the response with a single \`\`\`json fenced block — no preamble, narrative, or prose before it.\n` +
+        `- \`verdict\` MUST be the string "PASS" or "FAIL" (uppercase) — NOT a boolean (\`true\`/\`false\`), number, or any other word.\n` +
+        `- \`confidence\` MUST be present, as an integer 0–100.\n` +
+        `- \`dimensions\` MUST contain all four boolean keys: \`correctness\`, \`tests\`, \`planCoverage\`, \`security\`.\n` +
+        `- Put every finding inside the \`issues\` array using the exact \`{severity,title,detail,file?}\` shape — do NOT invent other keys (e.g. \`rationale\`, \`findings\`) for them. Free-form reasoning, if any, goes AFTER the closing \`\`\`.`,
     );
   }
 

api/src/review/review.utility.spec.ts

@@ -62,6 +62,16 @@ describe('parseReview', () => {
   it('returns null when no JSON can be recovered', () => {
     expect(parseReview('no structured output here')).toBeNull();
   });
+
+  // The contract is strict: an off-schema review (here: boolean verdict + missing confidence) is
+  // rejected so the orchestrator re-runs it, rather than being coerced into a possibly-misread shape.
+  it('returns null for an off-schema verdict (must re-run, not salvage)', () => {
+    expect(
+      parseReview(
+        '```json\n{"verdict":false,"dimensions":{"correctness":false,"tests":true,"planCoverage":false,"security":true}}\n```',
+      ),
+    ).toBeNull();
+  });
 });
 
 describe('meetsThreshold', () => {

api/src/review/review.utility.ts

@@ -55,6 +55,9 @@ const dimensionsSchema = z.preprocess(
   }),
 );
 
+// The review contract is strict: a pass that doesn't conform (missing confidence, non-enum
+// verdict, findings under the wrong key, …) is NOT coerced into shape — `safeParse` fails, the
+// pass is recorded as unparseable, and the orchestrator RE-RUNS the review.
 const reviewSchema = z.object({
   confidence: z.coerce.number().int().min(0).max(100),
   verdict: z.enum(['PASS', 'FAIL']).optional(),