Add JSON repair + review ground-truth worktree diff

src/engine/jsonExtract.ts

@@ -9,6 +9,13 @@ import type { z } from "zod";
  * `{...}` substrings, validate each against a schema, and prefer the LAST one
  * that satisfies it (a model's concluding answer comes last).
  *
+ * Models also routinely emit JSON that is *almost* valid: raw control
+ * characters (a literal newline/tab inside a string) or invalid backslash
+ * escapes. Stock `JSON.parse` rejects these, which used to cost a whole
+ * repair-retry round-trip (or a NEEDS_HUMAN give-up). Before failing a
+ * candidate we therefore run a conservative {@link repairJson} pass and try
+ * again, so a recoverable formatting slip no longer wastes a model call.
+ *
  * Both role outputs (the critic review, and the actor's plan/build payloads)
  * go through this single code path so parsing behaves identically everywhere.
  */
@@ -47,6 +54,115 @@ export function extractJsonCandidates(text: string): string[] {
   return candidates;
 }
 
+// ---------------------------------------------------------------------------
+// JSON repair (ported from pi: packages/ai/src/utils/json-parse.ts, MIT).
+// Trimmed to the non-streaming path loopwright needs (no partial-json dep).
+// ---------------------------------------------------------------------------
+
+const VALID_JSON_ESCAPES = new Set(['"', "\\", "/", "b", "f", "n", "r", "t", "u"]);
+
+function isControlCharacter(char: string): boolean {
+  const codePoint = char.codePointAt(0);
+  return codePoint !== undefined && codePoint >= 0x00 && codePoint <= 0x1f;
+}
+
+function escapeControlCharacter(char: string): string {
+  switch (char) {
+    case "\b":
+      return "\\b";
+    case "\f":
+      return "\\f";
+    case "\n":
+      return "\\n";
+    case "\r":
+      return "\\r";
+    case "\t":
+      return "\\t";
+    default:
+      return `\\u${char.codePointAt(0)?.toString(16).padStart(4, "0") ?? "0000"}`;
+  }
+}
+
+/**
+ * Repairs malformed JSON string literals by:
+ * - escaping raw control characters inside strings (e.g. a literal newline)
+ * - doubling backslashes before invalid escape characters
+ *
+ * Conservative by construction: it only rewrites characters that stock JSON
+ * would reject, so a string that already parses is returned byte-for-byte.
+ */
+export function repairJson(json: string): string {
+  let repaired = "";
+  let inString = false;
+
+  for (let index = 0; index < json.length; index++) {
+    const char = json[index] as string;
+
+    if (!inString) {
+      repaired += char;
+      if (char === '"') inString = true;
+      continue;
+    }
+
+    if (char === '"') {
+      repaired += char;
+      inString = false;
+      continue;
+    }
+
+    if (char === "\\") {
+      const nextChar = json[index + 1];
+      if (nextChar === undefined) {
+        repaired += "\\\\";
+        continue;
+      }
+      if (nextChar === "u") {
+        const unicodeDigits = json.slice(index + 2, index + 6);
+        if (/^[0-9a-fA-F]{4}$/.test(unicodeDigits)) {
+          repaired += `\\u${unicodeDigits}`;
+          index += 5;
+          continue;
+        }
+        // Invalid \u escape (not followed by exactly four hex digits): keep it
+        // literal by doubling the backslash so JSON.parse doesn't choke on it.
+        repaired += "\\\\u";
+        index += 1;
+        continue;
+      }
+      if (VALID_JSON_ESCAPES.has(nextChar)) {
+        repaired += `\\${nextChar}`;
+        index += 1;
+        continue;
+      }
+      // An invalid escape (e.g. a lone "\" before a normal char): keep the
+      // backslash literally by doubling it.
+      repaired += "\\\\";
+      continue;
+    }
+
+    repaired += isControlCharacter(char) ? escapeControlCharacter(char) : char;
+  }
+
+  return repaired;
+}
+
+/**
+ * Parses JSON, falling back to a single {@link repairJson} pass when the raw
+ * text won't parse. Throws the ORIGINAL parse error when repair doesn't change
+ * anything (so the error message reflects the real input), matching pi.
+ */
+export function parseJsonWithRepair<T = unknown>(json: string): T {
+  try {
+    return JSON.parse(json) as T;
+  } catch (error) {
+    const repaired = repairJson(json);
+    if (repaired !== json) {
+      return JSON.parse(repaired) as T;
+    }
+    throw error;
+  }
+}
+
 export type JsonParseResult<T> =
   | { ok: true; value: T }
   | { ok: false; error: string };
@@ -75,7 +191,9 @@ export function parseLastValidJson<S extends z.ZodTypeAny>(
   for (const candidate of candidates) {
     let parsed: unknown;
     try {
-      parsed = JSON.parse(candidate);
+      // Try strict parse first, then a conservative repair pass (raw control
+      // chars / invalid escapes) before giving up on this candidate.
+      parsed = parseJsonWithRepair(candidate);
     } catch {
       continue; // e.g. "{foo}" in prose -- not JSON, try the next candidate
     }

src/engine/loop.ts

@@ -45,6 +45,15 @@ export interface LoopDeps {
    * config.stuckThresholdMs when set. <= 0 disables it. (Milestone 3, Task 18)
    */
   stuckThresholdMs?: number;
+  /**
+   * Optional ground-truth diff capture for the build working directory (the
+   * task's git worktree). When provided and it returns a non-empty diff, the
+   * engine reviews THAT instead of the actor's self-reported diff — so the
+   * artifact the critic sees is the artifact that actually gets integrated.
+   * Falls back to the model-reported diff when absent or empty (e.g. a runner
+   * that returns a diff but does not edit files on disk).
+   */
+  captureDiff?: (cwd: string) => Promise<string> | string;
   /** cancels the run cooperatively: checked between steps; kills gate commands */
   signal?: AbortSignal;
 }
@@ -274,6 +283,18 @@ export async function runTask(
         feedback = undefined;
         lastDiff = built.diff;
         lastTouched = built.touchedFiles;
+        // Prefer the real worktree diff (ground truth) over the model's
+        // self-reported diff when a capture seam is wired (worktree runs). This
+        // is read BEFORE the mechanical gate so build/test artifacts don't
+        // pollute the reviewed diff. Any failure falls back to the model diff.
+        if (deps.captureDiff) {
+          try {
+            const realDiff = await deps.captureDiff(deps.cwd);
+            if (realDiff.trim() !== "") lastDiff = realDiff;
+          } catch {
+            /* keep the model-reported diff */
+          }
+        }
         await deps.observer?.attempt?.({
           taskId: task.id,
           attempt: buildAttempts,

src/session.ts

@@ -13,6 +13,7 @@ import { integrate, type IntegrationResult } from "./engine/integrator.js";
 import { publish, type PublishResult, type PrCreator } from "./engine/publisher.js";
 import { GitWorktreeManager } from "./workspace/worktrees.js";
 import type { GitExec } from "./workspace/git.js";
+import { worktreeDiff } from "./workspace/git.js";
 import type { IntegrationBranch } from "./engine/integrator.js";
 import type { CommandExecutor } from "./engine/mechanicalGate.js";
 import type { Store, SessionStatus } from "./storage/store.js";
@@ -210,6 +211,10 @@ export async function runGoal(
     const schedulerExtra: Partial<SchedulerDeps> = {};
     if (wt) {
       schedulerExtra.workspaceFor = async (t) => (await wt.acquire(t.id)).path;
+      // Review the real worktree diff (ground truth) rather than the actor's
+      // self-reported diff — the changes on disk are what gets committed and
+      // integrated, so they are what the critic should judge.
+      schedulerExtra.captureDiff = (taskCwd) => worktreeDiff(taskCwd, gitExec);
       schedulerExtra.onTaskSettled = async (t, r) => {
         const unblocking =
           r.outcome?.finalState === "GREEN" || r.outcome?.finalState === "UNVERIFIED_BY_CRITIC";

src/workspace/git.ts

@@ -116,3 +116,32 @@ export async function commitCount(
     return 0;
   }
 }
+
+/**
+ * Returns the unified diff of everything currently changed in the working tree
+ * at `dir`, relative to HEAD, INCLUDING newly created (untracked) files.
+ *
+ * This is the engine's ground-truth view of what an actor actually did: a
+ * file-editing runner edits the worktree on disk, and this is the artifact that
+ * later gets committed and integrated. The critic should review THIS, not the
+ * model's self-reported diff (which can diverge from disk).
+ *
+ * Untracked files are surfaced via `add -A --intent-to-add`, which writes only
+ * empty index entries so the new paths appear in `git diff`; the commit path's
+ * later `git add -A` overwrites them with real content, so this is safe to call
+ * mid-loop. Never throws: any git failure (unborn branch, git missing) degrades
+ * to an empty string so the caller can fall back to the model-reported diff.
+ */
+export async function worktreeDiff(dir: string, exec: GitExec = spawnGit): Promise<string> {
+  try {
+    // Intent-to-add so brand-new files show up as additions in the diff.
+    await exec(["add", "-A", "--intent-to-add"], dir);
+    const res = await exec(["diff", "HEAD"], dir);
+    if (res.exitCode === 0) return res.stdout;
+    // Unborn branch (no HEAD yet) or similar: diff against the index instead.
+    const fallback = await exec(["diff"], dir);
+    return fallback.exitCode === 0 ? fallback.stdout : "";
+  } catch {
+    return "";
+  }
+}

test/git.test.ts

@@ -8,6 +8,7 @@ import {
   isGitRepo,
   remoteUrl,
   spawnGit,
+  worktreeDiff,
 } from "../src/workspace/git.js";
 
 /** Initializes a throwaway git repo and returns its path. */
@@ -58,4 +59,25 @@ describe("git helpers", () => {
     // feature is one commit ahead of main.
     expect(await commitCount(repo, "feature", "main")).toBe(1);
   });
+
+  it("worktreeDiff captures modified AND newly created files", async () => {
+    // No changes yet.
+    expect((await worktreeDiff(repo)).trim()).toBe("");
+
+    // Modify a tracked file and add a brand-new (untracked) file.
+    await writeFile(join(repo, "base.txt"), "base\nmore\n");
+    await writeFile(join(repo, "new.txt"), "hello\n");
+
+    const diff = await worktreeDiff(repo);
+    expect(diff).toContain("base.txt");
+    expect(diff).toContain("+more");
+    // The untracked file must appear as an addition (intent-to-add).
+    expect(diff).toContain("new.txt");
+    expect(diff).toContain("+hello");
+  });
+
+  it("worktreeDiff never throws on a non-repo and returns empty", async () => {
+    const plain = await mkdtemp(join(tmpdir(), "loopwright-plain-"));
+    expect(await worktreeDiff(plain)).toBe("");
+  });
 });

test/jsonExtract.test.ts

@@ -0,0 +1,82 @@
+import { describe, it, expect } from "vitest";
+import { z } from "zod";
+import {
+  extractJsonCandidates,
+  parseJsonWithRepair,
+  parseLastValidJson,
+  repairJson,
+} from "../src/engine/jsonExtract.js";
+
+describe("repairJson", () => {
+  it("leaves already-valid JSON byte-for-byte unchanged", () => {
+    const valid = '{"a":"b\\n c","n":1,"u":"\\u00e9"}';
+    expect(repairJson(valid)).toBe(valid);
+    expect(JSON.parse(repairJson(valid))).toEqual({ a: "b\n c", n: 1, u: "é" });
+  });
+
+  it("escapes a raw newline inside a string", () => {
+    const broken = '{"summary":"line one\nline two"}';
+    expect(() => JSON.parse(broken)).toThrow();
+    const fixed = parseJsonWithRepair<{ summary: string }>(broken);
+    expect(fixed.summary).toBe("line one\nline two");
+  });
+
+  it("escapes raw tab/return control characters inside strings", () => {
+    const broken = '{"s":"a\tb\rc"}';
+    expect(() => JSON.parse(broken)).toThrow();
+    expect(parseJsonWithRepair<{ s: string }>(broken).s).toBe("a\tb\rc");
+  });
+
+  it("doubles a backslash before an invalid escape (e.g. a Windows path)", () => {
+    const broken = '{"path":"C:\\Users\\me"}';
+    expect(() => JSON.parse(broken)).toThrow();
+    expect(parseJsonWithRepair<{ path: string }>(broken).path).toBe("C:\\Users\\me");
+  });
+
+  it("preserves valid \\uXXXX escapes", () => {
+    const broken = '{"s":"snow \\u2603 man\nnext"}'; // valid unicode + raw newline
+    const fixed = parseJsonWithRepair<{ s: string }>(broken);
+    expect(fixed.s).toBe("snow \u2603 man\nnext");
+  });
+
+  it("rethrows the original error when repair changes nothing", () => {
+    // Structurally broken (missing value) — not a string-literal problem.
+    expect(() => parseJsonWithRepair('{"a":}')).toThrow();
+  });
+});
+
+describe("parseLastValidJson with repair", () => {
+  const schema = z.object({ diff: z.string(), summary: z.string() });
+
+  it("recovers a build payload whose diff contains raw newlines", () => {
+    // The kind of output a model emits when it forgets to escape a diff body.
+    const raw =
+      'Here is the change:\n' +
+      '{"diff":"--- a/x.ts\n+++ b/x.ts\n@@\n-old\n+new","summary":"swap"}';
+    const res = parseLastValidJson(raw, schema, "build");
+    expect(res.ok).toBe(true);
+    if (res.ok) {
+      expect(res.value.summary).toBe("swap");
+      expect(res.value.diff).toContain("+new");
+    }
+  });
+
+  it("still prefers the LAST valid object when several are present", () => {
+    const raw =
+      '{"diff":"d1\nx","summary":"first"} then ' +
+      '{"diff":"d2\ny","summary":"second"}';
+    const res = parseLastValidJson(raw, schema, "build");
+    expect(res.ok).toBe(true);
+    if (res.ok) expect(res.value.summary).toBe("second");
+  });
+
+  it("reports a schema failure when no candidate validates", () => {
+    const res = parseLastValidJson('{"diff":"only"}', schema, "build");
+    expect(res.ok).toBe(false);
+    if (!res.ok) expect(res.error).toContain("schema validation");
+  });
+
+  it("extractJsonCandidates still ignores prose braces", () => {
+    expect(extractJsonCandidates("use {foo} carefully")).toEqual(["{foo}"]);
+  });
+});