From d442c8799c9a145786a09483be143c6fbcc3050c Mon Sep 17 00:00:00 2001 From: Agnik47 <140933190+Agnik47@users.noreply.github.com> Date: Fri, 28 Aug 2026 22:24:31 +0530 Subject: [PATCH] fix(docs-review): constrain the advisory review with the schema it already defines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The advisory docs-sync review posts "🟠 Maintainer review suggested — low confidence / The automated review returned an invalid structured result" on pull requests whose documentation impact it never actually judged. That message comes from `validateGeminiReview` when the model's JSON parses but `verdict` is missing or not one of the three allowed values. It has no way to know the key: #260 removed `response_format: json_schema` from the request to work around an HTTP 400, and the prompt names the verdict *values* in prose but never the object shape. `REVIEW_JSON_SCHEMA` has been exported and unused since. Every model response since has been a guess at the contract, and a wrong guess degrades silently to a low-confidence comment instead of a review. The request now carries the schema again, and the 400 fallback drops one capability per rung — `reasoning_effort` first, `response_format` only if the model also rejects that — so an unusable parameter no longer costs the schema. `maxItems` leaves the schema because strict structured output rejects array length keywords, which is the likely original 400; `validateGeminiReview` already caps findings at 5. The prompt states the exact keys so the schema-less rung produces a valid object too, and fenced-JSON parsing stays for it. --- scripts/docs-sync-review.ts | 36 ++++++++++++++--- src/docs-sync-review-cli.test.ts | 66 ++++++++++++++++++++++++++++++++ src/docs-sync-review.test.ts | 45 ++++++++++++++++++++++ src/docs-sync-review.ts | 5 ++- 4 files changed, 146 insertions(+), 6 deletions(-) diff --git a/scripts/docs-sync-review.ts b/scripts/docs-sync-review.ts index ec7778d3..6e6a2494 100644 --- a/scripts/docs-sync-review.ts +++ b/scripts/docs-sync-review.ts @@ -3,6 +3,7 @@ import { isAbsolute, resolve, sep } from 'node:path'; import { pathToFileURL } from 'node:url'; import { REVIEW_COMMENT_MARKER, + REVIEW_JSON_SCHEMA, buildReviewPrompts, classifyPullRequest, createDeferredResult, @@ -181,13 +182,25 @@ export function loadDocumentation( return excerpts; } +/** + * Request rungs, most capable first. A model that rejects a parameter answers + * HTTP 400, so each rung drops exactly one capability rather than abandoning + * structured output altogether: an unusable `reasoning_effort` must not cost us + * the schema that keeps `verdict` well-formed. + */ +const REQUEST_LADDER: ReadonlyArray<{ reasoningEffort: boolean; structuredOutput: boolean }> = [ + { reasoningEffort: true, structuredOutput: true }, + { reasoningEffort: false, structuredOutput: true }, + { reasoningEffort: false, structuredOutput: false }, +]; + export async function generateOpenAIReview( prompt: string, model: string, apiKey: string, fetchImpl: FetchLike = fetch, ): Promise { - const response = await requestOpenAIReview(prompt, model, apiKey, fetchImpl, true); + const response = await requestOpenAIReview(prompt, model, apiKey, fetchImpl, 0); const data = await response.json() as { choices?: Array<{ message?: { content?: string | null } }>; }; @@ -201,8 +214,9 @@ async function requestOpenAIReview( model: string, apiKey: string, fetchImpl: FetchLike, - useLowReasoning: boolean, + rung: number, ): Promise { + const options = REQUEST_LADDER[rung] ?? REQUEST_LADDER[REQUEST_LADDER.length - 1]!; const response = await fetchImpl('https://api.openai.com/v1/chat/completions', { method: 'POST', headers: { @@ -218,12 +232,24 @@ async function requestOpenAIReview( }, { role: 'user', content: prompt }, ], - ...(useLowReasoning ? { reasoning_effort: 'low' } : {}), + ...(options.reasoningEffort ? { reasoning_effort: 'low' } : {}), + ...(options.structuredOutput + ? { + response_format: { + type: 'json_schema', + json_schema: { + name: 'docs_sync_review', + strict: true, + schema: REVIEW_JSON_SCHEMA, + }, + }, + } + : {}), }), signal: AbortSignal.timeout(180_000), }); - if (response.status === 400 && useLowReasoning) { - return requestOpenAIReview(prompt, model, apiKey, fetchImpl, false); + if (response.status === 400 && rung + 1 < REQUEST_LADDER.length) { + return requestOpenAIReview(prompt, model, apiKey, fetchImpl, rung + 1); } if (!response.ok) { const detail = (await response.text()).slice(0, 600); diff --git a/src/docs-sync-review-cli.test.ts b/src/docs-sync-review-cli.test.ts index 060b8de4..52f92e90 100644 --- a/src/docs-sync-review-cli.test.ts +++ b/src/docs-sync-review-cli.test.ts @@ -361,6 +361,72 @@ describe('OpenAI and documentation boundaries', () => { expect(result).toEqual({ verdict: 'no_update_needed', summary: 'Covered.', findings: [] }); }); + it('constrains the response with the strict review schema', async () => { + const fetchImpl = vi.fn(async () => jsonResponse({ + choices: [{ message: { content: '{"verdict":"no_update_needed","summary":"Covered.","findings":[]}' } }], + })); + + await generateOpenAIReview('review prompt', 'gpt-test', 'api-key', fetchImpl); + + const body = JSON.parse((fetchImpl.mock.calls as unknown as Array<[unknown, { body: string }]>)[0][1].body); + expect(body.response_format).toMatchObject({ + type: 'json_schema', + json_schema: { name: 'docs_sync_review', strict: true }, + }); + expect(body.response_format.json_schema.schema.properties.verdict.enum) + .toEqual(['no_update_needed', 'review_suggested', 'likely_missing']); + }); + + it('sends no schema keyword that strict structured output rejects', async () => { + const fetchImpl = vi.fn(async () => jsonResponse({ + choices: [{ message: { content: '{"verdict":"no_update_needed","summary":"Covered.","findings":[]}' } }], + })); + + await generateOpenAIReview('review prompt', 'gpt-test', 'api-key', fetchImpl); + + const body = (fetchImpl.mock.calls as unknown as Array<[unknown, { body: string }]>)[0][1].body; + expect(body).not.toContain('maxItems'); + expect(body).not.toContain('minItems'); + }); + + it('keeps the schema when only low reasoning is rejected', async () => { + const fetchImpl = vi.fn() + .mockResolvedValueOnce(jsonResponse({ error: { message: 'unsupported parameter' } }, 400)) + .mockResolvedValueOnce(jsonResponse({ + choices: [{ message: { content: '{"verdict":"no_update_needed","summary":"Covered.","findings":[]}' } }], + })); + + await expect(generateOpenAIReview('review prompt', 'gpt-test', 'api-key', fetchImpl)) + .resolves.toEqual({ verdict: 'no_update_needed', summary: 'Covered.', findings: [] }); + const second = (fetchImpl.mock.calls as unknown as Array<[unknown, { body: string }]>)[1][1].body; + expect(second).not.toContain('reasoning_effort'); + expect(second).toContain('"json_schema"'); + }); + + it('drops the schema only after the model also rejects it', async () => { + const fetchImpl = vi.fn() + .mockResolvedValueOnce(jsonResponse({ error: { message: 'unsupported parameter' } }, 400)) + .mockResolvedValueOnce(jsonResponse({ error: { message: 'response_format unsupported' } }, 400)) + .mockResolvedValueOnce(jsonResponse({ + choices: [{ message: { content: '{"verdict":"no_update_needed","summary":"Covered.","findings":[]}' } }], + })); + + await expect(generateOpenAIReview('review prompt', 'gpt-test', 'api-key', fetchImpl)) + .resolves.toEqual({ verdict: 'no_update_needed', summary: 'Covered.', findings: [] }); + expect(fetchImpl).toHaveBeenCalledTimes(3); + const third = (fetchImpl.mock.calls as unknown as Array<[unknown, { body: string }]>)[2][1].body; + expect(third).not.toContain('reasoning_effort'); + expect(third).not.toContain('response_format'); + }); + + it('stops retrying when the last rung still fails', async () => { + const fetchImpl = vi.fn(async () => jsonResponse({ error: { message: 'bad request' } }, 400)); + + await expect(generateOpenAIReview('review prompt', 'gpt-test', 'api-key', fetchImpl)) + .rejects.toThrow('OpenAI request failed with HTTP 400'); + expect(fetchImpl).toHaveBeenCalledTimes(3); + }); + it('accepts fenced OpenAI JSON output', async () => { const fetchImpl = vi.fn(async () => jsonResponse({ choices: [{ message: { content: '```json\n{"verdict":"no_update_needed","summary":"Covered.","findings":[]}\n```' } }], diff --git a/src/docs-sync-review.test.ts b/src/docs-sync-review.test.ts index 34b0d265..f40ca177 100644 --- a/src/docs-sync-review.test.ts +++ b/src/docs-sync-review.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { MAX_DIFF_CHARACTERS, + REVIEW_JSON_SCHEMA, buildReviewPrompts, classifyPullRequest, createDeferredResult, @@ -264,6 +265,50 @@ describe('review context', () => { expect(result.truncated).toBe(true); expect(result.diffText).toContain('[patch unavailable]'); }); + + it('names the response keys the validator reads', () => { + const context: PullRequestReviewContext = { + number: 72, + title: 'Add profile option', + body: null, + draft: false, + headSha: 'abc123', + labels: [], + files: [{ path: 'src/cli.ts', status: 'modified', patch: '+ .option("--profile ")' }], + }; + + const [result] = buildReviewPrompts(context, []); + + // The schema-less fallback rung has only the prompt to go on, so the keys + // validateGeminiReview reads must be stated there too. + expect(result.prompt).toContain('"verdict"'); + expect(result.prompt).toContain('"summary"'); + expect(result.prompt).toContain('"findings"'); + expect(result.prompt).toContain('"suggestedPath"'); + expect(result.prompt).toContain('"behaviorChange"'); + }); +}); + +describe('REVIEW_JSON_SCHEMA', () => { + it('describes every key the validator requires', () => { + expect(REVIEW_JSON_SCHEMA.required).toEqual(['verdict', 'summary', 'findings']); + expect(REVIEW_JSON_SCHEMA.properties.findings.items.required).toEqual([ + 'surface', + 'behaviorChange', + 'changedPath', + 'evidence', + 'suggestedPath', + 'reason', + ]); + }); + + it('stays inside the strict structured-output subset', () => { + // strict: true rejects array length keywords; validateGeminiReview caps the + // findings list instead. + expect(JSON.stringify(REVIEW_JSON_SCHEMA)).not.toContain('maxItems'); + expect(REVIEW_JSON_SCHEMA.additionalProperties).toBe(false); + expect(REVIEW_JSON_SCHEMA.properties.findings.items.additionalProperties).toBe(false); + }); }); describe('validateGeminiReview', () => { diff --git a/src/docs-sync-review.ts b/src/docs-sync-review.ts index 43487e41..c28d1250 100644 --- a/src/docs-sync-review.ts +++ b/src/docs-sync-review.ts @@ -80,7 +80,7 @@ export const REVIEW_JSON_SCHEMA = { }, findings: { type: 'array', - maxItems: 5, + description: 'At most 5 findings; an empty array when no documentation update is needed.', items: { type: 'object', additionalProperties: false, @@ -316,6 +316,9 @@ export function buildReviewPrompts( 'Use no_update_needed only when the supplied changes require no README, docs, or skill update.', 'Use review_suggested when context or evidence is ambiguous or incomplete.', 'Use likely_missing only when an exact changed-file excerpt supports a specific missing documentation update.', + 'Respond with a JSON object using exactly these keys: {"verdict": "no_update_needed" | "review_suggested" | "likely_missing", "summary": string, "findings": array}.', + 'Each finding uses exactly these keys: {"surface": "readme" | "docs" | "skill", "behaviorChange": string, "changedPath": string, "evidence": string, "suggestedPath": string, "reason": string}.', + 'Return at most 5 findings, and an empty findings array when the verdict is no_update_needed.', '', 'BEGIN UNTRUSTED PULL REQUEST DATA', `PR: #${context.number}`,