From bb16a8cbfe3b3dc99f97b85d06a9a3b7e31a5170 Mon Sep 17 00:00:00 2001 From: Christopher Date: Mon, 6 Jul 2026 22:37:45 +1000 Subject: [PATCH] fix(eval): surface pending-merge branch and raw grader response in failures Discovered during a live WiseTechAcademy.Evals v5 dogfood run: - When a results auto-push hits a genuine content conflict, directPushResultsWithDetails already pushes the local run to a temp agentv/results-sync/... branch instead of dropping it (docs/adr/0007), but the CLI warning only printed the block reason ("...resolve it with a GitHub pull request") with no mention of the branch or its compare URL. That reads as "results were lost" when they were not. Surface the pending_merge branch name and compare URL in the warning. - An LLM-rubric grader parse failure only carried the JSON.parse error (e.g. `Unexpected identifier "This"`), discarding the raw model text. When a grader is misconfigured to point at a non-JSON-returning target, the error gives no clue what the model actually said. Include a truncated raw-response preview in the skip reason. Co-Authored-By: Claude Sonnet 5 --- apps/cli/src/commands/results/remote.ts | 25 ++++++++++++ .../results/remote-auto-export.test.ts | 38 ++++++++++++++++++- .../core/src/evaluation/graders/llm-grader.ts | 25 +++++++++++- packages/core/test/evaluation/graders.test.ts | 34 +++++++++++++++++ 4 files changed, 119 insertions(+), 3 deletions(-) diff --git a/apps/cli/src/commands/results/remote.ts b/apps/cli/src/commands/results/remote.ts index caa6c9014..405cd2c58 100644 --- a/apps/cli/src/commands/results/remote.ts +++ b/apps/cli/src/commands/results/remote.ts @@ -7,6 +7,7 @@ import { type GitListedRun, type NormalizedResultsConfig, type ResultsConfig, + type ResultsPendingMerge, type ResultsRepoStatus, type RuntimeResultsConfig, confirmResultsMergeAndPull, @@ -510,6 +511,27 @@ function relativeRunPathFromManifestPath(relativeManifestPath: string): string { : manifestDir; } +/** + * When an auto-export push is blocked by a genuine results-content conflict, + * `directPushResultsWithDetails` still pushes the local run to a temp + * `agentv/results-sync/...` branch (see docs/adr/0007) so the results are not + * lost. Surface that branch (and its compare URL, when available) so the + * warning doesn't read as "results were dropped" when they were, in fact, + * pushed and only need a human-merged pull request. + */ +export function formatPendingMergeWarnings( + pendingMerge: ResultsPendingMerge | undefined, +): string[] { + if (!pendingMerge) { + return []; + } + const { temp_branch, target_branch, compare_url } = pendingMerge; + const mergeSuffix = compare_url ? `: ${compare_url}` : '.'; + return [ + `Warning: results were pushed to '${temp_branch}' instead — they are not lost. Merge that branch into ${target_branch} with a pull request${mergeSuffix}`, + ]; +} + export async function maybeAutoExportRunArtifacts( payload: RemoteExportPayload, ): Promise { @@ -540,6 +562,9 @@ export async function maybeAutoExportRunArtifacts( throw new Error(pushResult.block_reason ?? 'Results branch push conflict'); } console.warn(`Warning: skipping results export: ${pushResult.block_reason}`); + for (const line of formatPendingMergeWarnings(pushResult.pending_merge)) { + console.warn(line); + } return 'failed'; } diff --git a/apps/cli/test/commands/results/remote-auto-export.test.ts b/apps/cli/test/commands/results/remote-auto-export.test.ts index 7bbf5f938..8d3b2b657 100644 --- a/apps/cli/test/commands/results/remote-auto-export.test.ts +++ b/apps/cli/test/commands/results/remote-auto-export.test.ts @@ -7,7 +7,10 @@ import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, spyOn } from 'bun:test'; import { AGENTV_RESULTS_ARTIFACTS_REF, type EvaluationResult } from '@agentv/core'; -import { maybeAutoExportRunArtifacts } from '../../../src/commands/results/remote.js'; +import { + formatPendingMergeWarnings, + maybeAutoExportRunArtifacts, +} from '../../../src/commands/results/remote.js'; function cleanGitEnv(): Record { const env: Record = {}; @@ -145,6 +148,39 @@ function payload(projectDir: string, runDir: string) { }; } +describe('formatPendingMergeWarnings', () => { + it('returns no warnings when there is no pending merge', () => { + expect(formatPendingMergeWarnings(undefined)).toEqual([]); + }); + + it('surfaces the sync branch and compare URL so results do not read as lost', () => { + const warnings = formatPendingMergeWarnings({ + temp_branch: 'agentv/results-sync/20260706T121445Z-agentv-results-v1-0aceae', + target_branch: 'agentv/results/v1', + compare_url: + 'https://github.com/example/repo/compare/agentv/results/v1...agentv/results-sync/20260706T121445Z-agentv-results-v1-0aceae', + created_at: '2026-07-06T12:14:45Z', + }); + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('agentv/results-sync/20260706T121445Z-agentv-results-v1-0aceae'); + expect(warnings[0]).toContain('agentv/results/v1'); + expect(warnings[0]).toContain('https://github.com/example/repo/compare/'); + expect(warnings[0]).toContain('not lost'); + }); + + it('still explains the merge without a compare URL when none is available', () => { + const warnings = formatPendingMergeWarnings({ + temp_branch: 'agentv/results-sync/20260706T121445Z-agentv-results-v1-0aceae', + target_branch: 'agentv/results/v1', + created_at: '2026-07-06T12:14:45Z', + }); + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('pull request.'); + }); +}); + describe('maybeAutoExportRunArtifacts', () => { let rootDir: string; let projectDir: string; diff --git a/packages/core/src/evaluation/graders/llm-grader.ts b/packages/core/src/evaluation/graders/llm-grader.ts index 198c40a5e..c6e82a7c4 100644 --- a/packages/core/src/evaluation/graders/llm-grader.ts +++ b/packages/core/src/evaluation/graders/llm-grader.ts @@ -1355,6 +1355,7 @@ export class LlmGrader implements Grader { let lastError: Error | undefined; let lastInvalidResponse: StructuredGenerationResult | undefined; + let lastRawText: string | undefined; let shouldAttemptStructureFix = false; for (let attempt = 1; attempt <= 3; attempt++) { @@ -1374,6 +1375,7 @@ export class LlmGrader implements Grader { data = parseResponse ? parseResponse(raw) : (schema.parse(raw) as unknown as TData); } catch (e: unknown) { lastError = e instanceof Error ? e : new Error(String(e)); + lastRawText = result.text; shouldAttemptStructureFix = canRepairResponse; continue; } @@ -1388,8 +1390,9 @@ export class LlmGrader implements Grader { } if (shouldAttemptStructureFix && lastInvalidResponse) { + let repaired: StructuredGenerationResult | undefined; try { - const repaired = await this.generateStructuredResponse({ + repaired = await this.generateStructuredResponse({ context, graderProvider, systemPrompt, @@ -1407,11 +1410,14 @@ export class LlmGrader implements Grader { }; } catch (e: unknown) { lastError = e instanceof Error ? e : new Error(String(e)); + lastRawText = repaired?.text ?? lastRawText; } } throw new Error( - `Failed to parse evaluator response after 3 attempts and 1 structure-fix attempt: ${lastError?.message}`, + `Failed to parse evaluator response after 3 attempts and 1 structure-fix attempt: ${lastError?.message}${ + lastRawText ? ` | raw response: ${truncateForErrorMessage(lastRawText)}` : '' + }`, ); } @@ -1492,6 +1498,21 @@ export function buildPromptfooRubricOutputSchema(): string { ].join('\n'); } +/** + * Cap a raw evaluator response so it stays readable inline in an error/reason + * message, while still giving enough context (e.g. "the model returned prose + * instead of JSON") to debug a grader parse failure without re-running it. + */ +const RAW_RESPONSE_PREVIEW_LENGTH = 300; + +function truncateForErrorMessage(text: string): string { + const normalized = text.trim().replace(/\s+/g, ' '); + if (normalized.length <= RAW_RESPONSE_PREVIEW_LENGTH) { + return JSON.stringify(normalized); + } + return JSON.stringify(`${normalized.slice(0, RAW_RESPONSE_PREVIEW_LENGTH)}…`); +} + function buildStructureRepairPrompt(options: { readonly validationError: string; readonly invalidResponse: string; diff --git a/packages/core/test/evaluation/graders.test.ts b/packages/core/test/evaluation/graders.test.ts index 4d0c2c5f6..bcf659b9a 100644 --- a/packages/core/test/evaluation/graders.test.ts +++ b/packages/core/test/evaluation/graders.test.ts @@ -1196,6 +1196,40 @@ describe('LlmGrader (llm-grader)', () => { expect(graderProvider.requests).toHaveLength(4); }); + it('includes a raw-response preview in the skip reason when the model never returns JSON', async () => { + // Regression for a real dogfood failure: a grader target that ignores the + // JSON-only instruction and returns prose. The JSON.parse error alone + // ("Unexpected identifier ...") gives no clue what the model actually + // said, so the skip reason must carry a preview of the raw text too. + const proseResponse = textResponse( + 'This response is relevant and accurate based on the provided sources.', + ); + const graderProvider = new SequenceCapturingProvider([ + proseResponse, + proseResponse, + proseResponse, + proseResponse, + ]); + + const evaluator = new LlmGrader({ + resolveGraderProvider: async () => graderProvider, + }); + + const result = await evaluator.evaluate({ + evalCase: { ...baseTestCase, evaluator: 'llm-grader' }, + candidate: 'Answer', + target: baseTarget, + provider: graderProvider, + attempt: 0, + promptInputs: { question: '' }, + now: new Date(), + }); + + expect(result.verdict).toBe('skip'); + expect(result.assertions[0]?.text).toContain('raw response:'); + expect(result.assertions[0]?.text).toContain('This response is relevant and accurate'); + }); + it('keeps skipping on unrecoverable malformed JSON', async () => { const graderProvider = new StubProvider(textResponse('{"score":'));