From e175304a51401e5f57bf78ac44727a0a69be7216 Mon Sep 17 00:00:00 2001 From: Luca Giordano Date: Fri, 3 Jul 2026 10:55:42 +0200 Subject: [PATCH] fix: postPrReview falls back to a plain comment when self-review is blocked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live-confirmed root cause: GitHub's REST API rejects REQUEST_CHANGES reviews from a PR's own author ("Review Can not request changes on your own pull request", HTTP 422) — the normal case for this repo, since the orchestrator's GH_TOKEN both opens every PR and posts its AI review. Both the inline-comment attempt and the existing body-only fallback hit the identical 422, so every changes-requested verdict has silently failed to post, historically. Doesn't break safety (auto-merge is gated by the reducer's own in-memory state, not GitHub's review status) but means the AI's feedback never actually reached a human. postPrReview now tries, in order: (1) inline review, (2) body-only review, (3) a plain PR comment carrying the same verdict + summary + per-file comments (formatReviewAsComment) — a plain comment has no self-review restriction. Live-verified against PR #96: both review attempts 422'd exactly as predicted, and the comment posted successfully. --- .sandcastle/issue-source.ts | 36 +++++++++++++++++++++++++++++++++--- .sandcastle/reduce.test.ts | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/.sandcastle/issue-source.ts b/.sandcastle/issue-source.ts index 5175a67..6b33674 100644 --- a/.sandcastle/issue-source.ts +++ b/.sandcastle/issue-source.ts @@ -12,6 +12,20 @@ import { READY_LABEL, type Issue, type ReviewOutput } from "./reduce.ts"; const exec = promisify(execFile); +/** + * Render a structured review verdict as plain markdown, for postPrReview's + * final fallback (a plain PR comment) when GitHub's formal review API is + * unavailable (self-review restriction — see postPrReview). + */ +export function formatReviewAsComment(output: ReviewOutput): string { + const header = `**AI review: ${output.verdict}**\n\n${output.summary}`; + if (output.comments.length === 0) return header; + const commentsBlock = output.comments + .map((c) => `**${c.path}:${c.line}** — ${c.body}`) + .join("\n\n"); + return `${header}\n\n---\n${commentsBlock}`; +} + /** Parse "Blocked by #N" references from an issue body (one or many). */ export function parseBlockedBy(body: string): number[] { const idx = body.search(/blocked\s+by\b/i); @@ -228,7 +242,16 @@ export class IssueSource { /** * Post a REQUEST_CHANGES review on the PR for the given issue. * Uses the GitHub API to attach the structured verdict + filtered inline comments. - * Best-effort: if inline-comment posting fails, falls back to a plain PR comment. + * Best-effort, two-stage fallback: + * 1. inline comments may fail when lines are no longer in the diff — retry + * as a plain top-level review (no inline comments). + * 2. GitHub rejects REQUEST_CHANGES reviews from a PR's OWN AUTHOR + * ("Review Can not request changes on your own pull request", HTTP 422) + * — the normal case here, since this orchestrator's GH_TOKEN both opens + * the PR and posts its AI review. A plain PR comment carries the same + * feedback without that restriction, so it is the final fallback rather + * than letting the verdict go unposted (confirmed live: both the inline + * and body-only review attempts hit the identical 422 in that case). */ async postPrReview(issueId: number, output: ReviewOutput): Promise { const { stdout: prJson } = await this.gh([ @@ -254,16 +277,23 @@ export class IssueSource { ["api", `repos/{owner}/{repo}/pulls/${prInfo.number}/reviews`, "--method", "POST", "--input", "-"], JSON.stringify(reviewBody), ); + return; } catch (err) { - // Inline comments may fail when lines are no longer in the diff; fall back to a - // plain top-level review without inline comments so the verdict is still visible. console.warn(`[issue-source] inline review post failed — falling back to body-only review:`, err); + } + + try { const bodyOnly = { ...reviewBody, comments: [] }; await this.ghWithInput( ["api", `repos/{owner}/{repo}/pulls/${prInfo.number}/reviews`, "--method", "POST", "--input", "-"], JSON.stringify(bodyOnly), ); + return; + } catch (err) { + console.warn(`[issue-source] body-only review post also failed — falling back to a plain PR comment:`, err); } + + await this.comment(issueId, formatReviewAsComment(output)); } /** Open a PR for an already-pushed head branch. Returns the PR URL. */ diff --git a/.sandcastle/reduce.test.ts b/.sandcastle/reduce.test.ts index 64a6709..711e49e 100644 --- a/.sandcastle/reduce.test.ts +++ b/.sandcastle/reduce.test.ts @@ -9,7 +9,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { reduce, READY_LABEL, IN_PROGRESS_LABEL, IN_REVIEW_LABEL, reviewVerdict, reconcileFromLabels, type State, type CiStatus, type Pr, type ReviewOutput } from "./reduce.ts"; -import { parseBlockedBy } from "./issue-source.ts"; +import { parseBlockedBy, formatReviewAsComment } from "./issue-source.ts"; import { sweepOrphanedSandboxes, ensureSandboxNetwork, parseConcurrency, withRetry, resetAgentBranch, refreshBase, validateSignature, classifyDelivery, parseSmeeEvent, parseOrchEnv, resolveCredentials, resolveRunMode, resolveDockerHost } from "./main.ts"; import { createHmac } from "node:crypto"; import { SANDBOX_LABEL, PROJECT_LABEL_KEY, deriveProject } from "./sandbox-runner.ts"; @@ -105,6 +105,40 @@ test("parseBlockedBy: stops at next markdown section", () => { ); }); +// ─── formatReviewAsComment ─────────────────────────────────────────────────── + +test("formatReviewAsComment: no comments — just verdict and summary", () => { + const rendered = formatReviewAsComment({ verdict: "pass", summary: "Looks good.", comments: [] }); + assert.equal(rendered, "**AI review: pass**\n\nLooks good."); +}); + +test("formatReviewAsComment: includes each comment with path:line", () => { + const rendered = formatReviewAsComment({ + verdict: "changes-requested", + summary: "One issue found.", + comments: [{ path: "src/foo.ts", line: 42, body: "This is wrong." }], + }); + assert.equal( + rendered, + "**AI review: changes-requested**\n\nOne issue found.\n\n---\n**src/foo.ts:42** — This is wrong.", + ); +}); + +test("formatReviewAsComment: multiple comments joined with blank lines", () => { + const rendered = formatReviewAsComment({ + verdict: "changes-requested", + summary: "Two issues.", + comments: [ + { path: "a.ts", line: 1, body: "first" }, + { path: "b.ts", line: 2, body: "second" }, + ], + }); + assert.equal( + rendered, + "**AI review: changes-requested**\n\nTwo issues.\n\n---\n**a.ts:1** — first\n\n**b.ts:2** — second", + ); +}); + // ─── Dependency blocking (isReady + Tick) ──────────────────────────────────── test("issue with no blockers (blockedBy: []) behaves as before", () => {