diff --git a/packages/github-bot/README.md b/packages/github-bot/README.md index 22b1ee54a..7a1b170c2 100644 --- a/packages/github-bot/README.md +++ b/packages/github-bot/README.md @@ -213,6 +213,7 @@ Two prompt templates in `src/prompts.ts`: - Run `gh pr diff` for the full diff - Submit a review via `gh api .../reviews` - Post inline comments via `gh api .../comments` +- Emit applyable `suggestion` fences in inline comment bodies (see below) **`buildCommentActionPrompt`** — Includes the user's request (with @mention stripped) and instructions to: @@ -220,7 +221,37 @@ instructions to: - Check prior conversation via `gh pr view --comments` - Make code changes and push, or respond with analysis - Post a summary comment via `gh api .../issues/{n}/comments` -- Reply to a specific review thread (when `commentId` is present) +- Reply to a specific review thread (when `commentId` is present), optionally with an applyable + `suggestion` fence — the summary comment cannot carry one, since an issue comment has no line + anchor + +### Applyable Suggestions + +`buildSuggestionGuidelines` serves both prompt paths. A fenced `suggestion` block inside a +**line-anchored** comment is what GitHub renders with a "Commit suggestion" button; the fence +content replaces the anchored lines verbatim. Nothing in this package calls a special suggestion +API—the existing inline-comment and thread-reply routes carry the markdown body. + +All review, inline-comment, thread-reply, and summary bodies are passed as `-F body=@` rather +than an inline `-f body="…"` argument. The prompts require the agent to use its file-write tool or a +quoted heredoc delimiter; an unquoted heredoc performs command and variable substitution on +untrusted review text before `gh` reads the file. + +For a full review, the agent selects the anchor and must prove every selected line falls inside a +diff hunk. For a thread reply, GitHub inherits the parent comment's anchor, so the agent instead +checks that the supplied code-location hunk still matches the current head and emits no fence after +a commit has moved those lines. Both paths require verbatim replacement, exact indentation, a +scratch-copy check, and a prose fallback when the replacement cannot be proved safe. + +### Repository Identity in API Routes + +Every `gh api repos/...` route embedded in these prompts uses +`encodeRepositoryPathSegments({ repoOwner: owner, repoName: repo })` +(`@open-inspect/shared/types/repositories`) rather than maintaining another route-construction rule. +GitHub login owners are currently single segments, so this is behavior-preserving for webhook +traffic; using the shared helper keeps the prompt aligned with the repository-wide identity +contract, which also permits nested namespaces on other source-control providers. Human-readable +`${owner}/${repo}` prose stays unencoded. The prompts embed only metadata from the webhook payload. The agent gathers everything else. diff --git a/packages/github-bot/src/prompts.ts b/packages/github-bot/src/prompts.ts index 92fc22e13..9cac7e661 100644 --- a/packages/github-bot/src/prompts.ts +++ b/packages/github-bot/src/prompts.ts @@ -1,3 +1,5 @@ +import { encodeRepositoryPathSegments } from "@open-inspect/shared/types/repositories"; + function buildCustomInstructionsSection(instructions: string | null | undefined): string { if (!instructions?.trim()) return ""; return `\n## Custom Instructions\n${instructions}`; @@ -14,6 +16,67 @@ function buildCommentGuidelines(isPublicRepo: boolean): string { - Compose your full response before posting any comments.`; } +function buildSuggestionGuidelines(anchorMode: "selected" | "inherited"): string { + const anchorRules = + anchorMode === "selected" + ? `- Prefer a single-line anchor (\`line\` only) even when the replacement is several lines: + one anchored line may be replaced by any number of lines. Use \`start_line\` only to replace a + contiguous range. +- EVERY anchor line must fall inside a hunk of \`gh pr diff\` — the single \`line\` of a + single-line anchor, and both \`start_line\` and \`line\` of a range. An anchor outside the diff + rejects that comment with HTTP 422 and the feedback is lost.` + : `- A thread reply inherits the parent comment's anchor; do not send \`line\`, \`start_line\`, + or \`side\` fields on the reply request. +- Verify the inherited anchor against the ## Code Location hunk and the current head. If any pushed + commit moved or replaced those lines, post prose and NO fence.`; + const verification = + anchorMode === "selected" + ? `1. Run \`gh pr diff\` (it accepts a PR number, URL, or branch — never a file path) and + confirm every anchor line — including a lone \`line\` anchor, not only range endpoints — falls + inside a hunk. +2. Print the exact anchored lines (\`sed -n 'START,ENDp' \`) and confirm your fence is a + correct verbatim replacement for precisely those lines. +3. Apply the replacement to a scratch copy and run the cheapest correctness check the repo offers + for that file (syntax parse, type check, or its linter).` + : `1. Confirm the ## Code Location hunk still describes the current head and identify the exact + inherited lines it anchors. +2. Apply the replacement to a scratch copy and run the cheapest correctness check the repo offers + for that file (syntax parse, type check, or its linter).`; + + return ` +## Applyable Suggestions +A fenced \`suggestion\` block inside a line-anchored review comment renders in GitHub with a +"Commit suggestion" button, so the author applies your fix in one click. Use one whenever the fix +is a concrete, local edit you can state exactly: + +\`\`\`suggestion + +\`\`\` + +Hard rules — the fence content REPLACES the comment's anchored lines verbatim: +- It is not a diff and not an excerpt. Never put \`+\`/\`-\` markers, \`...\`, placeholders, TODOs, + or prose inside the fence. +- Reproduce the original leading whitespace exactly. A suggestion with wrong indentation still + applies cleanly and breaks the file. +- Include only the anchored lines — no surrounding unchanged lines for context. +${anchorRules} +- One suggestion per comment. The explanation goes above the fence, never inside it. + +Verify before you suggest. The repo is checked out on the PR head branch, so check instead of +guessing: +${verification} + +If any check fails, or the real fix spans multiple files, needs an import or declaration +elsewhere, or turns on a judgment call, describe the fix in prose and post NO fence. A wrong +suggestion is worse than none: it is one click away from being merged.`; +} + +function writeBodyFileInstruction(path: string): string { + return `write \`${path}\` with your file-write tool. If you use the shell, use a quoted heredoc + delimiter such as \`cat > ${path} <<'EOF'\`; an unquoted delimiter performs command and variable + substitution on untrusted review text`; +} + function buildUntrustedUserContentBlock(params: { source: string; author: string; @@ -60,6 +123,7 @@ export function buildCodeReviewPrompt(params: { codeReviewInstructions, isSelfReview = false, } = params; + const repoPath = encodeRepositoryPathSegments({ repoOwner: owner, repoName: repo }); const reviewEvent = isSelfReview ? "COMMENT" : "COMMENT|APPROVE|REQUEST_CHANGES"; const reviewEventGuidance = isSelfReview ? "Use COMMENT because GitHub does not allow pull request authors to approve their own PRs." @@ -107,25 +171,33 @@ ${prDescriptionBlock} - Performance implications - Code clarity and maintainability 3. You may read individual files in the repo for additional context beyond the diff -4. When your review is complete, submit it via: +4. When your review is complete, ${writeBodyFileInstruction("/tmp/review.md")}. Then submit it via: - gh api repos/${owner}/${repo}/pulls/${number}/reviews \\ + gh api repos/${repoPath}/pulls/${number}/reviews \\ --method POST \\ - -f body="" \\ + -F body=@/tmp/review.md \\ -f event="${reviewEvent}" ${reviewEventGuidance} -5. For inline comments on specific files: +5. For inline comments on specific files, first ${writeBodyFileInstruction("/tmp/comment.md")}: - gh api repos/${owner}/${repo}/pulls/${number}/comments \\ + gh api repos/${repoPath}/pulls/${number}/comments \\ --method POST \\ - -f body="" \\ + -F body=@/tmp/comment.md \\ -f path="" \\ - -f commit_id="$(gh api repos/${owner}/${repo}/pulls/${number} --jq '.head.sha')" \\ - -f line= \\ + -f commit_id="$(gh api repos/${repoPath}/pulls/${number} --jq '.head.sha')" \\ + -F line= \\ -f side="RIGHT" + \`line\` and \`start_line\` must go through \`-F\` (typed), not \`-f\` (raw string): the API rejects + a string line with \`"3" is not an integer\`. Add \`-F start_line=\` and + \`-f start_side="RIGHT"\` to anchor a contiguous range instead of a single line. + + Prefer a suggestion over prose whenever the fix is a concrete, local edit — see + ## Applyable Suggestions below. It is the difference between a review the author has to + re-implement and one they can apply. +${buildSuggestionGuidelines("selected")} ${buildCustomInstructionsSection(codeReviewInstructions)} ${buildCommentGuidelines(isPublic)}`; } @@ -160,6 +232,7 @@ export function buildCommentActionPrompt(params: { commentId, commentActionInstructions, } = params; + const repoPath = encodeRepositoryPathSegments({ repoOwner: owner, repoName: repo }); const intro = head ? `You are working on Pull Request #${number} in ${owner}/${repo}.\nThe repository has been cloned and you are on the ${head} branch.` @@ -178,8 +251,19 @@ export function buildCommentActionPrompt(params: { } let replyInstruction = ""; + let suggestionSection = ""; if (commentId) { - replyInstruction = `\n5. If you need to reply to the specific review thread:\n\n gh api repos/${owner}/${repo}/pulls/${number}/comments/${commentId}/replies \\\n --method POST \\\n -f body=""`; + replyInstruction = ` +5. If you need to reply to the specific review thread, first ${writeBodyFileInstruction("/tmp/reply.md")}: + + gh api repos/${repoPath}/pulls/${number}/comments/${commentId}/replies \\ + --method POST \\ + -F body=@/tmp/reply.md + + A thread reply inherits the parent comment's line anchor, so it can carry an applyable + suggestion. The summary comment cannot: an issue comment has no line anchor, and a suggestion + fence there renders as an inert code block.`; + suggestionSection = `\n${buildSuggestionGuidelines("inherited")}`; } return `${intro}${prDetails}${codeLocation} @@ -197,11 +281,11 @@ ${buildUntrustedUserContentBlock({ 3. Address the request: - If code changes are needed, make them and push to the current branch - If it's a question, respond with your analysis -4. When done, post a summary comment on the PR: +4. When done, ${writeBodyFileInstruction("/tmp/summary.md")}. Then post it on the PR: - gh api repos/${owner}/${repo}/issues/${number}/comments \\ + gh api repos/${repoPath}/issues/${number}/comments \\ --method POST \\ - -f body=""${replyInstruction} + -F body=@/tmp/summary.md${replyInstruction}${suggestionSection} ${buildCustomInstructionsSection(commentActionInstructions)} ${buildCommentGuidelines(isPublic)}`; } diff --git a/packages/github-bot/test/prompts.test.ts b/packages/github-bot/test/prompts.test.ts index 87c8b6fab..675ff4a3d 100644 --- a/packages/github-bot/test/prompts.test.ts +++ b/packages/github-bot/test/prompts.test.ts @@ -62,6 +62,74 @@ describe("buildCodeReviewPrompt", () => { expect(prompt).toContain("repos/acme/widgets/pulls/42/comments"); }); + it("encodes a nested-namespace owner as a single route segment in every API call", () => { + const prompt = buildCodeReviewPrompt({ ...baseParams, owner: "group/platform" }); + // Use the shared repository-route contract even though GitHub webhook owners are currently + // single-segment logins. + expect(prompt).toContain("repos/group%2Fplatform/widgets/pulls/42/reviews"); + expect(prompt).toContain("repos/group%2Fplatform/widgets/pulls/42/comments"); + expect(prompt).toContain('-f commit_id="$(gh api repos/group%2Fplatform/widgets/pulls/42 --jq'); + expect(prompt).not.toContain("repos/group/platform/widgets"); + // The human-readable intro line is prose, not a route, and stays unencoded. + expect(prompt).toContain("Pull Request #42 in group/platform/widgets."); + }); + + it("teaches the applyable suggestion fence and its range anchors", () => { + const prompt = buildCodeReviewPrompt(baseParams); + // The whole point: a `suggestion` fence in a line-anchored comment body is what GitHub + // renders with a "Commit suggestion" button, and start_line/start_side anchors a range. + expect(prompt).toContain("## Applyable Suggestions"); + expect(prompt).toContain("```suggestion"); + expect(prompt).toContain("-F start_line="); + expect(prompt).toContain('-f start_side="RIGHT"'); + }); + + it("passes every markdown body through safely-written files", () => { + const prompt = buildCodeReviewPrompt(baseParams); + expect(prompt).toContain("-F body=@/tmp/review.md"); + expect(prompt).toContain("-F body=@/tmp/comment.md"); + expect(prompt).not.toContain("-f body="); + expect(prompt).toContain("quoted heredoc"); + expect(prompt).toContain("<<'EOF'"); + expect(prompt).toContain("untrusted review text"); + }); + + it("sends line numbers as typed integers", () => { + const prompt = buildCodeReviewPrompt(baseParams); + // `-f` is a raw string field, and the API rejects a string line with + // `For 'properties/line', "3" is not an integer` — verified live against the REST API. + expect(prompt).toContain("-F line="); + expect(prompt).not.toContain("-f line="); + expect(prompt).toContain('"3" is not an integer'); + }); + + it("fences the suggestion contract against the ways an applied suggestion breaks code", () => { + const prompt = buildCodeReviewPrompt(baseParams); + // A suggestion is one click from merge, so each of these is load-bearing: the fence replaces + // the anchored lines verbatim, so a diff marker, an ellipsis, or lost indentation applies + // cleanly and corrupts the file. + expect(prompt).toContain("REPLACES the comment's anchored lines verbatim"); + expect(prompt).toContain("Reproduce the original leading whitespace exactly"); + expect(prompt).toContain("HTTP 422"); + expect(prompt).toContain("post NO fence"); + // And the agent must check rather than guess — it has the head branch checked out. + expect(prompt).toContain("sed -n 'START,ENDp' "); + }); + + it("requires every anchor — a lone line as much as a range endpoint — to sit inside a diff hunk", () => { + const prompt = buildCodeReviewPrompt(baseParams); + // Regression: this used to gate diff-hunk membership on start_line only, so a single-line + // `line` anchor outside the diff could still be suggested and would be rejected with 422. + expect(prompt).toContain("EVERY anchor line must fall inside a hunk of `gh pr diff`"); + expect(prompt).toContain("single `line` of a"); + expect(prompt).toContain("both `start_line` and `line`"); + expect(prompt).toContain("including a lone `line`"); + expect(prompt).toContain("not only range endpoints"); + // Regression guard: the old wording scoped the requirement to range endpoints only. + expect(prompt).not.toContain("only when BOTH endpoints appear in a hunk"); + expect(prompt).not.toContain("gh pr diff "); + }); + it("limits self-reviews to comments", () => { const prompt = buildCodeReviewPrompt({ ...baseParams, isSelfReview: true }); expect(prompt).toContain('-f event="COMMENT"'); @@ -189,9 +257,50 @@ describe("buildCommentActionPrompt", () => { expect(prompt).not.toContain("reply to the specific review thread"); }); + it("offers applyable suggestions only on the inherited thread anchor", () => { + const prompt = buildCommentActionPrompt({ ...baseParams, commentId: 999 }); + expect(prompt).toContain("## Applyable Suggestions"); + expect(prompt).toContain("```suggestion"); + expect(prompt).toContain("-F body=@/tmp/reply.md"); + expect(prompt).toContain("-F body=@/tmp/summary.md"); + expect(prompt).toContain("inherits the parent comment's anchor"); + expect(prompt).toContain("do not send `line`, `start_line`,"); + expect(prompt).toContain("If any pushed"); + expect(prompt).not.toContain("EVERY anchor line"); + expect(prompt).not.toContain("-F start_line="); + // The summary lands on issues/{n}/comments, which has no line anchor, so a fence there is + // inert — the agent has to know which of its two posting paths can carry one. + expect(prompt).toContain("renders as an inert code block"); + }); + + it("omits the suggestion contract when there is no thread to reply to", () => { + const prompt = buildCommentActionPrompt(baseParams); + expect(prompt).not.toContain("## Applyable Suggestions"); + expect(prompt).not.toContain("```suggestion"); + }); + it("includes summary comment instruction with correct repo path", () => { const prompt = buildCommentActionPrompt(baseParams); expect(prompt).toContain("repos/acme/widgets/issues/42/comments"); + expect(prompt).toContain("-F body=@/tmp/summary.md"); + expect(prompt).not.toContain("-f body="); + expect(prompt).toContain("<<'EOF'"); + }); + + it("encodes a nested-namespace owner as a single route segment in every API call", () => { + const prompt = buildCommentActionPrompt({ + ...baseParams, + owner: "group/platform", + filePath: "src/cache.ts", + diffHunk: "@@ -10,3 +10,5 @@\n+const cache = new Map();", + commentId: 999, + }); + // Route construction stays aligned with the shared repository-identity contract. + expect(prompt).toContain("repos/group%2Fplatform/widgets/issues/42/comments"); + expect(prompt).toContain("repos/group%2Fplatform/widgets/pulls/42/comments/999/replies"); + expect(prompt).not.toContain("repos/group/platform/widgets"); + // The human-readable intro line is prose, not a route, and stays unencoded. + expect(prompt).toContain("Pull Request #42 in group/platform/widgets."); }); it("escapes embedded closing user_content tags in comment body", () => {