Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
59 changes: 59 additions & 0 deletions eng/skill-validator/src/json-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/**
* Utilities for handling JSON produced by LLMs, which may contain
* invalid escape sequences or other structural quirks that trip up
* JSON.parse.
*/

const VALID_ESCAPE_CHARS = new Set(['"', '\\', '/', 'b', 'f', 'n', 'r', 't', 'u']);

/**
* 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) is turned into a double-backslash
* so that JSON.parse reads it as a literal backslash + letter.
*
* The function walks character-by-character, tracking whether we are
* inside a JSON string, so it never modifies structural characters
* outside of strings.
*/
export function sanitizeJsonEscapes(jsonStr: string): string {
let result = '';
let inString = false;
let i = 0;

while (i < jsonStr.length) {
const ch = jsonStr[i];

if (!inString) {
result += ch;
if (ch === '"') inString = true;
i++;
continue;
}

// Inside a JSON string value
if (ch === '\\') {
const next = jsonStr[i + 1];
if (next !== undefined && VALID_ESCAPE_CHARS.has(next)) {
// Valid escape sequence — keep as-is
result += ch + next;
i += 2;
Comment thread
JanKrivanek marked this conversation as resolved.
Outdated
} else {
// Invalid escape (or trailing backslash) — emit an escaped
// backslash so JSON.parse sees a literal "\"
result += '\\\\';
i++;
}
} else if (ch === '"') {
result += ch;
inString = false;
i++;
} else {
result += ch;
i++;
}
Comment thread
JanKrivanek marked this conversation as resolved.
}

return result;
}
7 changes: 5 additions & 2 deletions eng/skill-validator/src/judge.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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)}`);
}
}

Expand Down
4 changes: 3 additions & 1 deletion eng/skill-validator/src/pairwise-judge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -270,7 +271,8 @@ function parsePairwiseResponse(
throw new Error(`Pairwise judge response contained no JSON (${direction})`);
}

const parsed = JSON.parse(jsonStr);
const sanitized = sanitizeJsonEscapes(jsonStr);
const parsed = JSON.parse(sanitized);
Comment thread
JanKrivanek marked this conversation as resolved.
Outdated

const rubricResults: PairwiseRubricResult[] = (parsed.rubric_results || []).map(
(r: any) => {
Expand Down
145 changes: 145 additions & 0 deletions eng/skill-validator/tests/json-utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
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 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("handles a real-world LLM pairwise judge response with invalid escapes", () => {
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);
});
Comment thread
JanKrivanek marked this conversation as resolved.
Outdated

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"
);
});
});
Loading