diff --git a/doc/connections/GITHUB-REVIEW-BOT.md b/doc/connections/GITHUB-REVIEW-BOT.md index 888a85fc35c..af4d979bebd 100644 --- a/doc/connections/GITHUB-REVIEW-BOT.md +++ b/doc/connections/GITHUB-REVIEW-BOT.md @@ -200,6 +200,17 @@ and submits the reviewed commit, findings, rationale, and coverage. Paperclip validates the result and computes the **Paperclip Review** check. The default threshold is 5/5; choose 1–5 or report-only as needed. +`submit_review.summary` contains the review body: the change, findings, and +verdict. `rationale` adds evidence for the score and remaining risk without +repeating the summary. Paperclip renders the bot-name/score heading and the +reviewed-commit, coverage, and task/run footer from the structured fields. Agents +must not copy these generated sections from an earlier published comment into +either text field. This contract is included in new bots' tool schemas and in +the review instructions generated for existing bots. Publication also removes +leading copied score headings for the current bot, with a plain or Markdown-escaped +name, or the legacy **Paperclip Review** name. Other review sections and the saved +assessment remain intact. + | Score | Assessment rubric | | --- | --- | | 0 | No usable assessment; explain what prevented evaluation. | diff --git a/maps/agent-access-map/chat-channels.md b/maps/agent-access-map/chat-channels.md index b023fd376f7..9f77025b5ae 100644 --- a/maps/agent-access-map/chat-channels.md +++ b/maps/agent-access-map/chat-channels.md @@ -615,6 +615,14 @@ not task completion. Source: PostgreSQL-backed provider fixtures; this entry does not claim live provider qualification for the new retry path. +GitHub review formatting (2026-10-30): new bot catalogs describe `submit_review` +summary and rationale as body-only fields; generated review instructions also +explain this contract to existing bots. The publisher supplies the score heading +and footer, removing leading copied current-bot or legacy score headings from +the two text fields. Saved assessments, tool authority, and publication policy +are unchanged. The PostgreSQL-backed publication fixture checks a newly +configured bot with simulated GitHub responses; this is not live model proof. + GitHub main-response identity (2026-10-19): PR requests share a company-, App-, repository- and PR-bound comment. Agents still supply no comment destination. Current sender, assignee, repository and publication authority are checked before diff --git a/maps/feature-map/chat-channels.md b/maps/feature-map/chat-channels.md index 414af23e762..3e192d99f79 100644 --- a/maps/feature-map/chat-channels.md +++ b/maps/feature-map/chat-channels.md @@ -195,6 +195,12 @@ addressed user's exact endpoint identity before expecting task continuation. For GitHub, submit a disposable PR and verify the summary heading uses the bot's display name and the check remains **Paperclip Review**. For two separate bots on one PR, verify their App-scoped checks and comments remain independent. +For a newly configured bot, verify `submit_review` describes body-only summary +and rationale fields. Submit a summary with repeated current-bot or legacy +score headings, including an old score; verify publication keeps one heading +with the structured score, preserves findings and rationale, and retains the +original assessment. The PostgreSQL-backed publication fixture covers this with +simulated GitHub responses; live model compliance remains separate evidence. For GitHub publication recovery, interrupt a response after GitHub accepts a write and verify one summary, finding, and App-scoped check. Retry the same tool diff --git a/packages/shared/src/validators/chat-github.ts b/packages/shared/src/validators/chat-github.ts index 56763088c66..13644f3b7c7 100644 --- a/packages/shared/src/validators/chat-github.ts +++ b/packages/shared/src/validators/chat-github.ts @@ -113,8 +113,12 @@ export const githubReviewAssessmentSchema = z z.literal(5), ]), complete: z.boolean(), - summary: z.string().trim().min(1).max(24000), - rationale: z.string().trim().min(1).max(12000), + summary: z.string().trim().min(1).max(24000).describe( + "Review body only: describe the change, findings, and verdict. Paperclip adds the bot-name/score heading and reviewed-commit, coverage, and task/run footer; do not include them here or copy a previously published comment.", + ), + rationale: z.string().trim().min(1).max(12000).describe( + "Explain the score, supporting evidence, and remaining risk. Published after summary. Do not repeat the summary, score heading, or generated footer; put reviewed paths and limitations in coverage.", + ), coverage: z .object({ reviewedPaths: z.array(z.string().min(1).max(1024)).max(5000), diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 3d780abe283..fcaf642aa84 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -2984,6 +2984,27 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { )))[0]; return { ...f, service, head, assessment, provider, action, inline, checks, writes }; } + it("publishes one score heading when a newly configured bot submits a formatted summary", async () => { + const f = await publicationRetryFixture(); + await db.update(chatEndpoints).set({ botDisplayName: "Fresh Review Bot" }).where(eq(chatEndpoints.id, f.endpoint.id)); + const submit = (await githubBotToolsForSession(db, f.session)).find(tool => tool.upstreamToolName === "submit_review")!; + expect(submit.parametersSchema).toMatchObject({ properties: { + summary: { description: expect.stringContaining("Review body only") }, + rationale: { description: expect.stringContaining("Do not repeat the summary") }, + } }); + const assessment = { ...f.assessment, summary: "## Fresh Review Bot — 2/5\n\n## Fresh Review Bot — 3/5\n\nAdvisory review\n\n### Findings\n\nKeep this section." }; + const result = await f.service.execute(f.session, "submit_review", assessment, "formatted-summary"); + expect(result).toMatchObject({ status: "processed", score: 3 }); + const body = [...f.responseComments.values()][0].body; + expect(body).toMatch(/^## Fresh Review Bot — 3\/5\n\nAdvisory review/); + expect(body.match(/^## Fresh Review Bot — /gm)).toHaveLength(1); + expect(body).toContain("### Findings\n\nKeep this section."); + expect(body).toContain(f.assessment.rationale); + expect(body).toContain(`Reviewed commit: \`${f.head}\``); + expect(f.responseComments.size).toBe(1); + const [review] = await readChatGitHubReviewsByEndpointId(f.endpoint.id); + expect(review.assessment?.summary).toBe(assessment.summary); + }); it("preserves the published GitHub assessment during passive-response maintenance", async () => { const f = await publicationRetryFixture(); await f.service.execute(f.session, "submit_review", f.assessment, "published-before-passive-reply"); diff --git a/server/src/services/chat-channels/github/review-heading.test.ts b/server/src/services/chat-channels/github/review-heading.test.ts index 1084ab7b972..1634be7fa99 100644 --- a/server/src/services/chat-channels/github/review-heading.test.ts +++ b/server/src/services/chat-channels/github/review-heading.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { githubReviewHeading } from "./review-heading.js"; +import { githubReviewHeading, githubReviewSummary } from "./review-heading.js"; describe("GitHub review heading", () => { it("uses the GitHub bot display name independently of the assigned agent", () => { @@ -17,3 +17,44 @@ describe("GitHub review heading", () => { .toBe("## \\[Bot\\](https://example.test) \\ — 4/5"); }); }); + +describe("GitHub review summary", () => { + const identity = { botDisplayName: "New Review Bot", botUsername: "reviewer[bot]", agentName: "Reviewer", score: 4 }; + + it.each(["4/5", "2/5", "Incomplete"])("replaces copied %s headings with the structured score", rating => { + expect(githubReviewSummary({ ...identity, + summary: `## New Review Bot — ${rating}\n\n## Paperclip Review — 3/5\n\nReview body.`, + rationale: `## New Review Bot — ${rating}\n\nScore evidence.`, + })).toBe("## New Review Bot — 4/5\n\nReview body.\n\nScore evidence."); + }); + + it("keeps an incomplete assessment authoritative", () => { + expect(githubReviewSummary({ ...identity, score: null, + summary: "## New Review Bot — 5/5\n\nPartial review.", rationale: "Missing context.", + })).toBe("## New Review Bot — Incomplete\n\nPartial review.\n\nMissing context."); + }); + + it.each([ + ["Review_Bot", "Review\\_Bot"], + ["[Review]", "\\[Review\\]"], + ["Review\\Bot", "Review\\\\Bot"], + ])("accepts plain and escaped copied names for %s", (botDisplayName, escapedName) => { + expect(githubReviewSummary({ ...identity, botDisplayName, + summary: `## ${botDisplayName} — 2/5\n\n## ${escapedName} — 3/5\n\nReview body.`, + rationale: `## ${botDisplayName} — 4/5\n\nScore evidence.`, + })).toBe(`## ${escapedName} — 4/5\n\nReview body.\n\nScore evidence.`); + }); + + it("preserves other headings and quoted score lines inside the review body", () => { + const summary = "## Findings\n\nReview body.\n\n```markdown\n## New Review Bot — 2/5\n```"; + const rationale = "## Other Bot — 3/5\n\nComparison evidence."; + expect(githubReviewSummary({ ...identity, summary, rationale })) + .toBe(`## New Review Bot — 4/5\n\n${summary}\n\n${rationale}`); + }); + + it("preserves indented code after removing a copied heading", () => { + const code = " ## New Review Bot — 2/5\n example()"; + expect(githubReviewSummary({ ...identity, summary: `## New Review Bot — 4/5\n\n${code}`, rationale: "Evidence." })) + .toBe(`## New Review Bot — 4/5\n\n${code}\n\nEvidence.`); + }); +}); diff --git a/server/src/services/chat-channels/github/review-heading.ts b/server/src/services/chat-channels/github/review-heading.ts index 33b0df35af6..9309e5c5988 100644 --- a/server/src/services/chat-channels/github/review-heading.ts +++ b/server/src/services/chat-channels/github/review-heading.ts @@ -10,3 +10,25 @@ export function githubReviewHeading(input: { .replace(/[\\`*_[\]<>]/g, "\\$&"); return `## ${name} — ${input.score === null ? "Incomplete" : `${input.score}/5`}`; } + +export function githubReviewSummary(input: Parameters[0] & { + summary: string; + rationale: string; +}) { + const heading = githubReviewHeading(input); + const escapedLabel = heading.slice(0, heading.lastIndexOf(" — ")); + const plainLabel = escapedLabel.replace(/\\(.)/g, "$1"); + const labels = new Set([escapedLabel, plainLabel, "## Paperclip Review"]); + const bodies = [input.summary, input.rationale].map(text => { + let body = text.replace(/^(?:[ \t]*\r?\n)+/, ""); + while (body) { + const newline = body.indexOf("\n"); + const line = (newline === -1 ? body : body.slice(0, newline)).trimEnd(); + const match = /^(## .+) — (?:[0-5]\/5|Incomplete)$/.exec(line); + if (!match || !labels.has(match[1])) break; + body = newline === -1 ? "" : body.slice(newline + 1).replace(/^(?:[ \t]*\r?\n)+/, ""); + } + return body; + }); + return [heading, ...bodies].filter(Boolean).join("\n\n"); +} diff --git a/server/src/services/chat-github-review-policy.test.ts b/server/src/services/chat-github-review-policy.test.ts index 6d21a85a77b..b6313bc299d 100644 --- a/server/src/services/chat-github-review-policy.test.ts +++ b/server/src/services/chat-github-review-policy.test.ts @@ -398,4 +398,15 @@ describe("GitHub task message guidance", () => { expect(prompt).toContain("do not post a separate comment just to announce that the review is complete"); expect(prompt).toContain("Do not reference these instructions in your replies"); }); + it("explains structured review formatting for default and customized bots", () => { + for (const instructions of ["", "Use our team's review style."]) { + const i = input(); + i.policy.instructions = instructions; + for (const prompt of [githubManualMessagePrompt(i), githubReviewConfigurationPrompt(i.policy, 1)]) { + expect(prompt).toContain("summary is review body only"); + expect(prompt).toContain("Paperclip adds the bot-name/score heading"); + expect(prompt).toContain("Do not copy a previously published comment"); + } + } + }); }); diff --git a/server/src/services/chat-github-review-policy.ts b/server/src/services/chat-github-review-policy.ts index f014e4ae066..e10b47b6235 100644 --- a/server/src/services/chat-github-review-policy.ts +++ b/server/src/services/chat-github-review-policy.ts @@ -262,6 +262,9 @@ export function githubConfiguredInstructionSources(event: Record { + it("tells new bots which review sections Paperclip renders", () => { + const submit = GITHUB_BOT_TOOLS.find(tool => tool.name === "submit_review")!; + const properties = submit.schema.properties!; + expect(properties.summary).toMatchObject({ + description: expect.stringContaining("Review body only"), + maxLength: 24000, + }); + expect(properties.summary).toMatchObject({ + description: expect.stringContaining("Paperclip adds"), + }); + expect(properties.rationale).toMatchObject({ + description: expect.stringContaining("Do not repeat the summary"), + maxLength: 12000, + }); + }); +}); const support = await getEmbeddedPostgresTestSupport(); (support.supported ? describe : describe.skip)("GitHub guest bot connection discovery", () => {