fix(docs-review): constrain the advisory review with the schema it already defines

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<unknown> {
-  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<Response> {
+  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);

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```' } }],

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 <name>")' }],
+    };
+
+    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', () => {

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}`,