diff --git a/src/engine/jsonExtract.ts b/src/engine/jsonExtract.ts index 9c27b86..b717f89 100644 --- a/src/engine/jsonExtract.ts +++ b/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(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 = | { ok: true; value: T } | { ok: false; error: string }; @@ -75,7 +191,9 @@ export function parseLastValidJson( 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 } diff --git a/src/engine/loop.ts b/src/engine/loop.ts index 74ebd3e..3baf8bb 100644 --- a/src/engine/loop.ts +++ b/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; /** 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, diff --git a/src/session.ts b/src/session.ts index a61656d..a045fc1 100644 --- a/src/session.ts +++ b/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 = {}; 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"; diff --git a/src/workspace/git.ts b/src/workspace/git.ts index c04e2a7..7701585 100644 --- a/src/workspace/git.ts +++ b/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 { + 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 ""; + } +} diff --git a/test/git.test.ts b/test/git.test.ts index 280789f..efbb325 100644 --- a/test/git.test.ts +++ b/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(""); + }); }); diff --git a/test/jsonExtract.test.ts b/test/jsonExtract.test.ts new file mode 100644 index 0000000..7dc6545 --- /dev/null +++ b/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}"]); + }); +}); diff --git a/test/loop.test.ts b/test/loop.test.ts index f31f316..2dbba48 100644 --- a/test/loop.test.ts +++ b/test/loop.test.ts @@ -178,3 +178,64 @@ describe("fallback self-review", () => { expect(out.nits).toHaveLength(1); // the blocker was filtered out }); }); + + +describe("ground-truth diff capture (captureDiff)", () => { + /** A critic that records the diff it was asked to review, then passes green. */ + class RecordingCritic extends MockCritic { + seenDiff: string | undefined; + constructor() { + super({ fallback: criticGreen() }); + } + override async review(req: Parameters[0]) { + if (req.kind === "task") this.seenDiff = req.bundle.diff; + return super.review(req); + } + } + + it("reviews the captured worktree diff instead of the model's self-reported diff", async () => { + const t = task(); + const actor = new MockActor({ plans: [onePlan(t)] }); // synthetic model diff + const critic = new RecordingCritic(); + + const out = await runTask(t, { + ...makeDeps({ actor, critic }), + captureDiff: () => "REAL WORKTREE DIFF\n+added line", + }); + + expect(out.finalState).toBe("GREEN"); + expect(out.lastDiff).toBe("REAL WORKTREE DIFF\n+added line"); + expect(critic.seenDiff).toContain("REAL WORKTREE DIFF"); // redaction keeps body + expect(critic.seenDiff).not.toContain("attempt 0"); // not the model's diff + }); + + it("falls back to the model diff when capture returns empty (e.g. non-editing runner)", async () => { + const t = task(); + const actor = new MockActor({ plans: [onePlan(t)] }); + const critic = new MockCritic({ fallback: criticGreen() }); + + const out = await runTask(t, { + ...makeDeps({ actor, critic }), + captureDiff: () => " ", // whitespace-only => treated as no diff + }); + + expect(out.finalState).toBe("GREEN"); + expect(out.lastDiff).toContain(`src/${t.id}.ts`); // the model's synthetic diff + }); + + it("falls back to the model diff when capture throws", async () => { + const t = task(); + const actor = new MockActor({ plans: [onePlan(t)] }); + const critic = new MockCritic({ fallback: criticGreen() }); + + const out = await runTask(t, { + ...makeDeps({ actor, critic }), + captureDiff: () => { + throw new Error("git exploded"); + }, + }); + + expect(out.finalState).toBe("GREEN"); + expect(out.lastDiff).toContain(`src/${t.id}.ts`); + }); +});