diff --git a/.gitignore b/.gitignore index 890a64ccd8..8faa4f30ec 100644 --- a/.gitignore +++ b/.gitignore @@ -304,6 +304,9 @@ node_modules/ # skill-validator build output eng/skill-validator/dist/ +# skill-validator eval results +eng/skill-validator/.skill-validator-results/ + # Visual Studio 6 build log *.plg diff --git a/eng/skill-validator/src/json-utils.ts b/eng/skill-validator/src/json-utils.ts new file mode 100644 index 0000000000..9fcdc99d27 --- /dev/null +++ b/eng/skill-validator/src/json-utils.ts @@ -0,0 +1,80 @@ +/** + * Utilities for handling JSON produced by LLMs, which may contain + * invalid escape sequences or other structural quirks that trip up + * JSON.parse. + */ + +const SIMPLE_ESCAPE_CHARS = new Set(['"', '\\', '/', 'b', 'f', 'n', 'r', 't']); +const HEX_CHARS = new Set('0123456789abcdefABCDEF'); + +/** + * Fix invalid JSON escape sequences that LLMs sometimes produce. + * + * Valid JSON escapes are: \" \\ \/ \b \f \n \r \t \uXXXX. + * Anything else (e.g. \M, \S, \p, or malformed \u sequences) is turned + * into a double-backslash so that JSON.parse reads it as a literal backslash + * + the following characters. + * + * The function walks character-by-character, tracking whether we are + * inside a JSON string, so it never modifies structural characters + * outside of strings. + * + * Uses an array-based builder (joined at the end) to keep runtime linear + * and avoid quadratic string concatenation for large inputs. + */ +export function sanitizeJsonEscapes(jsonStr: string): string { + const parts: string[] = []; + let inString = false; + let i = 0; + + while (i < jsonStr.length) { + const ch = jsonStr[i]; + + if (!inString) { + parts.push(ch); + if (ch === '"') inString = true; + i++; + continue; + } + + // Inside a JSON string value + if (ch === '\\') { + const next = jsonStr[i + 1]; + if (next === 'u') { + // \u must be followed by exactly 4 hex digits to be valid + if ( + i + 5 < jsonStr.length && + HEX_CHARS.has(jsonStr[i + 2]) && + HEX_CHARS.has(jsonStr[i + 3]) && + HEX_CHARS.has(jsonStr[i + 4]) && + HEX_CHARS.has(jsonStr[i + 5]) + ) { + parts.push(jsonStr.slice(i, i + 6)); + i += 6; + } else { + // Malformed \u — escape the backslash so it becomes literal + parts.push('\\\\'); + i++; + } + } else if (next !== undefined && SIMPLE_ESCAPE_CHARS.has(next)) { + // Valid simple escape sequence — keep as-is + parts.push(ch, next); + i += 2; + } else { + // Invalid escape (or trailing backslash) — emit an escaped + // backslash so JSON.parse sees a literal "\" + parts.push('\\\\'); + i++; + } + } else if (ch === '"') { + parts.push(ch); + inString = false; + i++; + } else { + parts.push(ch); + i++; + } + } + + return parts.join(''); +} diff --git a/eng/skill-validator/src/judge.ts b/eng/skill-validator/src/judge.ts index b1dca61558..61135c6a82 100644 --- a/eng/skill-validator/src/judge.ts +++ b/eng/skill-validator/src/judge.ts @@ -1,6 +1,7 @@ import type { JudgeResult, RubricScore, RunMetrics, EvalScenario } from "./types.js"; import type { PermissionRequest } from "@github/copilot-sdk"; import { getSharedClient, checkPermission } from "./runner.js"; +import { sanitizeJsonEscapes } from "./json-utils.js"; export interface JudgeOptions { model: string; @@ -218,8 +219,10 @@ function parseJudgeResponse( throw new Error(`Judge response contained no JSON. Raw response:\n${content.slice(0, 500)}`); } + const sanitized = sanitizeJsonEscapes(jsonStr); + try { - const parsed = JSON.parse(jsonStr); + const parsed = JSON.parse(sanitized); const rubricScores: RubricScore[] = (parsed.rubric_scores || []).map( (s: { criterion: string; score: number; reasoning: string }) => ({ criterion: s.criterion, @@ -236,7 +239,7 @@ function parseJudgeResponse( overallReasoning: parsed.overall_reasoning || "", }; } catch (error) { - throw new Error(`Judge response parsing failed: ${error}\nExtracted JSON:\n${jsonStr.slice(0, 500)}`); + throw new Error(`Judge response parsing failed: ${error}\nExtracted JSON:\n${sanitized.slice(0, 500)}`); } } diff --git a/eng/skill-validator/src/pairwise-judge.ts b/eng/skill-validator/src/pairwise-judge.ts index a8dfbbfb13..5fbf06d208 100644 --- a/eng/skill-validator/src/pairwise-judge.ts +++ b/eng/skill-validator/src/pairwise-judge.ts @@ -8,6 +8,7 @@ import type { import { PAIRWISE_MAGNITUDE_SCORES } from "./types.js"; import type { PermissionRequest } from "@github/copilot-sdk"; import { getSharedClient, checkPermission } from "./runner.js"; +import { sanitizeJsonEscapes } from "./json-utils.js"; export interface PairwiseJudgeOptions { model: string; @@ -270,7 +271,16 @@ function parsePairwiseResponse( throw new Error(`Pairwise judge response contained no JSON (${direction})`); } - const parsed = JSON.parse(jsonStr); + const sanitized = sanitizeJsonEscapes(jsonStr); + let parsed: any; + try { + parsed = JSON.parse(sanitized); + } catch (err: any) { + throw new Error( + `Failed to parse pairwise judge JSON (${direction}): ${err?.message ?? err}\n` + + `Sanitized JSON (truncated): ${trunc(sanitized, 1000)}` + ); + } const rubricResults: PairwiseRubricResult[] = (parsed.rubric_results || []).map( (r: any) => { diff --git a/eng/skill-validator/tests/json-utils.test.ts b/eng/skill-validator/tests/json-utils.test.ts new file mode 100644 index 0000000000..5968363ae0 --- /dev/null +++ b/eng/skill-validator/tests/json-utils.test.ts @@ -0,0 +1,182 @@ +import { describe, it, expect } from "vitest"; +import { sanitizeJsonEscapes } from "../src/json-utils.js"; + +describe("sanitizeJsonEscapes", () => { + it("returns valid JSON unchanged", () => { + const input = '{"key": "hello world", "num": 42}'; + expect(sanitizeJsonEscapes(input)).toBe(input); + }); + + it("preserves all valid escape sequences", () => { + // Every valid JSON escape: \" \\ \/ \b \f \n \r \t + const input = '{"a": "quote\\" slash\\\\ solidus\\/ bs\\b ff\\f nl\\n cr\\r tab\\t"}'; + expect(sanitizeJsonEscapes(input)).toBe(input); + }); + + it("preserves valid \\u unicode escapes", () => { + const input = '{"emoji": "\\u0041\\u0042"}'; + expect(sanitizeJsonEscapes(input)).toBe(input); + }); + + it("fixes truncated \\u escape with fewer than 4 hex digits", () => { + const input = '{"a": "bad\\u1"}'; + const result = sanitizeJsonEscapes(input); + const parsed = JSON.parse(result); + expect(parsed.a).toBe("bad\\u1"); + }); + + it("fixes \\u escape with non-hex characters", () => { + const input = '{"a": "bad\\u12G4"}'; + const result = sanitizeJsonEscapes(input); + const parsed = JSON.parse(result); + expect(parsed.a).toBe("bad\\u12G4"); + }); + + it("fixes a single invalid escape inside a string", () => { + // \M is not a valid JSON escape + const input = '{"criterion": "Identified MultiTargetLib\\MultiTargetLib correctly"}'; + const expected = '{"criterion": "Identified MultiTargetLib\\\\MultiTargetLib correctly"}'; + expect(sanitizeJsonEscapes(input)).toBe(expected); + // Verify the result parses + const parsed = JSON.parse(sanitizeJsonEscapes(input)); + expect(parsed.criterion).toBe("Identified MultiTargetLib\\MultiTargetLib correctly"); + }); + + it("fixes multiple invalid escapes in the same string", () => { + const input = '{"path": "C:\\Users\\Admin\\Source"}'; + const result = sanitizeJsonEscapes(input); + const parsed = JSON.parse(result); + // \U, \A, \S are all invalid → doubled backslash. But \n is NOT here. + expect(parsed.path).toBe("C:\\Users\\Admin\\Source"); + }); + + it("does not touch backslashes outside of JSON strings", () => { + // Backslash in key-structural area shouldn't happen in real JSON, + // but ensure we only modify string interiors + const input = '{"a": "ok"}'; + expect(sanitizeJsonEscapes(input)).toBe(input); + }); + + it("handles escaped quotes inside strings correctly", () => { + const input = '{"a": "she said \\"hello\\""}'; + expect(sanitizeJsonEscapes(input)).toBe(input); + const parsed = JSON.parse(sanitizeJsonEscapes(input)); + expect(parsed.a).toBe('she said "hello"'); + }); + + it("handles already-escaped backslash followed by normal char", () => { + // \\N in JSON means literal backslash + N — already valid + const input = '{"a": "path\\\\Name"}'; + expect(sanitizeJsonEscapes(input)).toBe(input); + const parsed = JSON.parse(sanitizeJsonEscapes(input)); + expect(parsed.a).toBe("path\\Name"); + }); + + it("fixes trailing invalid escape at end of string", () => { + // \S at end of value + const input = '{"a": "test\\S"}'; + const result = sanitizeJsonEscapes(input); + expect(JSON.parse(result).a).toBe("test\\S"); + }); + + it("handles empty strings", () => { + const input = '{"a": ""}'; + expect(sanitizeJsonEscapes(input)).toBe(input); + }); + + it("handles multiline JSON with invalid escapes", () => { + // Simulates what the LLM judge actually produces + const input = [ + '{', + ' "rubric_scores": [', + ' {', + ' "criterion": "Identified MultiTargetLib\\MultiTargetLib clash",', + ' "score": 4,', + ' "reasoning": "The agent found the bin\\obj clash in the\\Solution"', + ' }', + ' ],', + ' "overall_score": 4,', + ' "overall_reasoning": "Good analysis"', + '}', + ].join('\n'); + + const result = sanitizeJsonEscapes(input); + const parsed = JSON.parse(result); + expect(parsed.rubric_scores[0].criterion).toBe( + "Identified MultiTargetLib\\MultiTargetLib clash" + ); + expect(parsed.rubric_scores[0].reasoning).toBe( + "The agent found the bin\\obj clash in the\\Solution" + ); + expect(parsed.overall_score).toBe(4); + }); + + it("does not mangle valid \\n inside strings", () => { + const input = '{"a": "line1\\nline2"}'; + expect(sanitizeJsonEscapes(input)).toBe(input); + expect(JSON.parse(sanitizeJsonEscapes(input)).a).toBe("line1\nline2"); + }); + + it("handles mix of valid and invalid escapes in one string", () => { + // \n is valid, \P is not, \t is valid, \G is not + const input = '{"a": "new\\npath\\Pand\\tthen\\Go"}'; + const result = sanitizeJsonEscapes(input); + const parsed = JSON.parse(result); + expect(parsed.a).toBe("new\npath\\Pand\tthen\\Go"); + }); + + it("returns empty string for empty input", () => { + expect(sanitizeJsonEscapes("")).toBe(""); + }); + + it("passes through valid LLM pairwise judge JSON unchanged", () => { + const input = JSON.stringify({ + rubric_results: [ + { + criterion: "Quality", + winner: "A", + magnitude: "slightly-better", + reasoning: "Response A was better", + }, + ], + overall_winner: "A", + overall_magnitude: "slightly-better", + overall_reasoning: "Overall A was better", + }); + // Valid JSON should pass through unchanged + expect(sanitizeJsonEscapes(input)).toBe(input); + }); + + it("fixes invalid escapes in a pairwise judge response", () => { + const input = [ + '{', + ' "rubric_results": [{', + ' "criterion": "Quality",', + ' "winner": "A",', + ' "magnitude": "slightly-better",', + ' "reasoning": "Response A handled C:\\Projects\\MyApp better"', + ' }],', + ' "overall_winner": "A",', + ' "overall_magnitude": "slightly-better",', + ' "overall_reasoning": "A analyzed the bin\\obj clash correctly"', + '}', + ].join('\n'); + const parsed = JSON.parse(sanitizeJsonEscapes(input)); + expect(parsed.rubric_results[0].reasoning).toBe( + "Response A handled C:\\Projects\\MyApp better" + ); + expect(parsed.overall_reasoning).toBe( + "A analyzed the bin\\obj clash correctly" + ); + }); + + it("handles Windows-style paths that LLMs commonly include", () => { + const input = + '{"reasoning": "The project at C:\\Projects\\MyApp\\src had issues"}'; + const result = sanitizeJsonEscapes(input); + const parsed = JSON.parse(result); + expect(parsed.reasoning).toBe( + "The project at C:\\Projects\\MyApp\\src had issues" + ); + }); +});