From ef394277c10c52b8d407db2dff1c1aa41fcc3765 Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Mon, 11 May 2026 17:14:03 +0000 Subject: [PATCH] =?UTF-8?q?review:=20synthesize=20`[!NOTE]`=20informationa?= =?UTF-8?q?l=20tier=20with=20#644=20alert=20judiciousness=20=E2=80=94=204-?= =?UTF-8?q?callout=20visual=20ladder=20+=20`approved`=20Fix-gate=20(#653)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * review: NOTE-tier callout + `actionable` flag to suppress Fix buttons Adds an `actionable` parameter to the `create_pull_request_review` tool (defaults true) so the agent can opt out of the Fix-it/Fix-all/Fix-👍s footer affordance on informational reviews. Threaded through `createAndSubmitWithFooter` so the buttons are omitted when `actionable: false`. Updates `Review` and `IncrementalReview` mode prompts with a 4th tier: `> [!NOTE]` + `actionable: false` for mergeable, FYI-style observations (prior feedback addressed cleanly, minor stale doc reference, etc.). Calibration note: `[!IMPORTANT]`/`[!CAUTION]` are reserved for findings that warrant code changes, because that's what trains users to click Fix. `[!NOTE]` reviews must not carry inline comments — if a point is concrete enough to anchor to a line, upgrade the whole review tier. * review: drop redundant `actionable` flag, key Fix buttons off `approved` `approved` already encodes "this PR is mergeable, nothing for the Fix button to act on" — `actionable` was a second flag carrying the same signal. Drop it from the tool schema and `FooterOpts`; the footer gate stays `if (!opts.approved)` (unchanged from pre-PR behavior, with a new comment documenting the UX rationale). NOTE-tier reviews now use `approved: true` + `> [!NOTE]` body instead of `approved: false` + `actionable: false`. For repos with `prApproveEnabled: false`, the runtime already downgrades APPROVE to COMMENT, so the GitHub-side shape is identical to the prior design. * review: address Pullfrog feedback — drop ambiguous parenthetical + update postRun nudge - Review-mode calibration: drop the "(or no callout at all)" parenthetical that didn't map cleanly to a bullet; replace with explicit "both the `[!NOTE]` tier and the 'no actionable issues' tier below use approved: true" so the bullet-list anchor is obvious. - `buildUnsubmittedReviewPrompt` (Review mode): the fallback nudge for unsubmitted reviews now defers to the mode prompt's tier matrix and acknowledges that `> [!NOTE]` informational reviews submit with `approved: true` alongside the canonical "No new issues found." path. Previously the nudge only described the pre-NOTE binary world. --- agents/postRun.ts | 2 +- mcp/review.ts | 5 ++++- modes.ts | 14 ++++++++++++-- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/agents/postRun.ts b/agents/postRun.ts index f499057..099715e 100644 --- a/agents/postRun.ts +++ b/agents/postRun.ts @@ -125,7 +125,7 @@ export function buildUnsubmittedReviewPrompt(mode: "Review" | "IncrementalReview return [ `MISSING REVIEW OUTPUT — you selected Review mode but stopped without calling \`create_pull_request_review\`. the user has no visible signal that this run produced anything; the progress comment will be deleted on exit and no review will appear on the PR.`, "", - "call `create_pull_request_review` now with your aggregated review (body + inline comments). if you found no actionable issues, submit with `approved: true` and a body opening with `No new issues found.` per the mode prompt — Review mode does not have a no-submit exit. the first call may error once with a diff-coverage nudge — retry the same call to proceed.", + "call `create_pull_request_review` now with your aggregated review (body + inline comments). pick the tier per the mode prompt — Review mode has no no-submit exit, so even informational `> [!NOTE]` reviews and `No new issues found.` reviews must be submitted (both use `approved: true`). the first call may error once with a diff-coverage nudge — retry the same call to proceed.", "", "do NOT stop again until `create_pull_request_review` has been called successfully.", ].join("\n"); diff --git a/mcp/review.ts b/mcp/review.ts index cb91762..7ff56b1 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -314,7 +314,7 @@ export const CreatePullRequestReview = type({ .optional(), approved: type.boolean .describe( - "Set to true to submit as an approval. ONLY when the review contains no actionable feedback — neither inline comments nor actionable content in the body. Defaults to false (comment-only review). Rejections are not supported." + "Set to true to submit as an approval. Use for both 'no issues found' and informational `> [!NOTE]` reviews where the PR is mergeable as-is and nothing in the body warrants code changes — approving also suppresses the Fix-button footer affordance so users don't dispatch a fix run on non-actionable feedback. Reserve approved: false for `> [!IMPORTANT]` (recommended changes) and `> [!CAUTION]` (critical) reviews. Defaults to false (comment-only review). Rejections are not supported." ) .optional(), commit_id: type.string @@ -842,6 +842,9 @@ async function createAndSubmitWithFooter( // API_URL is misconfigured, and future footer-building changes could // introduce new throw paths. keep the whole body wrapped. try { + // Fix buttons are suppressed on approving reviews — those are mergeable + // by definition (either "no issues found" or `> [!NOTE]` informational + // observations), so dispatching a fix run would be a UX trap. const customParts: string[] = []; if (!opts.approved) { const apiUrl = getApiUrl(); diff --git a/modes.ts b/modes.ts index 95ac843..b1cddc9 100644 --- a/modes.ts +++ b/modes.ts @@ -243,7 +243,14 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`, The review body is structured as: \`[optional alert blockquote]\` → \`[PR summary using the default format below]\`. Inline comments are passed via the \`comments\` parameter, not in the body. - GitHub alert blockquotes (\`> [!CAUTION]\`, \`> [!IMPORTANT]\`) are loud — they render as full-width colored callouts that dominate the review and create alert fatigue when overused. Use them judiciously: reserve them for findings the author genuinely must act on. A single JSDoc nit, a "rough edge" observation, a "consider also" suggestion, or a summary of what changed do NOT warrant a callout — open with the PR summary instead. If you find yourself reaching for \`[!IMPORTANT]\` to wrap something the author can defer or ignore without consequence, drop the alert. + GitHub alert blockquotes render at four visual intensities — the callout is what the author sees first, so pick the one that matches what you want them to do: + + - \`[!CAUTION]\` — large red banner. Reads as "this will break something." + - \`[!IMPORTANT]\` — large purple banner. Reads as "you need to look at this before merging." + - \`[!NOTE]\` — small blue inline callout. Reads as "FYI, here's something worth noting." + - no callout — plain text. Reads as routine review output. + + Two reinforcing levers: callout intensity (above) and \`approved\` (which gates the footer Fix-button affordance — Fix renders on every non-approving review, so \`approved: true\` suppresses it). Wrapping mergeable feedback in \`[!IMPORTANT]\` trains users to click Fix on reviews that don't need fixing. Pick the tier the author's actual next action justifies. - **critical issues** (blocks merge — bugs, security, data loss, broken core flows): \`approved: false\`. Body opens with \`> [!CAUTION]\\n> This PR introduces ...\`, followed by the PR summary. Include all inline comments via \`comments\`. @@ -251,6 +258,8 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`, \`approved: false\`. Body opens with \`> [!IMPORTANT]\\n> ...\`, followed by the PR summary. Reserve this tier for findings with concrete fallout — do NOT use \`[!IMPORTANT]\` for nits, style preferences, or "consider also" suggestions. Include all inline comments via \`comments\`. - **minor suggestions only** (single-line nits, doc/comment polish, defer-able observations, "rough edges"): \`approved: false\`. NO alert blockquote. Body opens directly with the PR summary. Include all inline comments via \`comments\`. + - **informational observations** (mergeable as-is, nothing actionable — e.g. prior feedback addressed cleanly, surfacing a minor stale doc reference, calling out something noteworthy without recommending a change): + \`approved: true\`. Body opens with \`> [!NOTE]\\n> ...\`, followed by the PR summary. Do NOT include inline \`comments\` — \`[!NOTE]\` signals "no action needed", which contradicts an actionable anchor; if a point is concrete enough to anchor to a line, downgrade the whole review to "minor suggestions only" (\`approved: false\`) instead. - **no actionable issues**: \`approved: true\`. Body opens with \`No new issues found.\` followed by the PR summary. @@ -310,7 +319,7 @@ ${PR_SUMMARY_FORMAT}`, 7. Submit — every run must end with EXACTLY ONE of \`${t("create_pull_request_review")}\` (substantive review) or \`${t("report_progress")}\` (no-review acknowledgement). do NOT call \`create_issue_comment\` for review output. - GitHub alert blockquotes (\`> [!CAUTION]\`, \`> [!IMPORTANT]\`) are loud — they render as full-width colored callouts that dominate the review and create alert fatigue when overused. Use them judiciously: reserve them for findings the author genuinely must act on. A single JSDoc nit, a "rough edge" observation, a "consider also" suggestion, or a summary of what changed do NOT warrant a callout — open with the Reviewed-changes summary instead. If you find yourself reaching for \`[!IMPORTANT]\` to wrap something the author can defer or ignore without consequence, drop the alert. + Same callout-intensity ladder as Review mode — \`[!CAUTION]\` (large red, "will break") → \`[!IMPORTANT]\` (large purple, "must address before merging") → \`[!NOTE]\` (small blue, "FYI") → no callout (plain text). And the same Fix-button lever: the footer renders a Fix button on every non-approving review, so \`approved: true\` suppresses it. Wrapping mergeable feedback in \`[!IMPORTANT]\` trains users to click Fix on reviews that don't need fixing — pick the tier the author's actual next action justifies. Follow these rules: - note: the first create_pull_request_review submission may error with a one-time diff-coverage nudge listing unread TOC regions. retry the same call to proceed — optionally after reading the listed ranges. the pre-flight will not block again this session. @@ -318,6 +327,7 @@ ${PR_SUMMARY_FORMAT}`, - ELSE IF NEW CRITICAL ISSUES (blocks merge — bugs, security, data loss, broken core flows): call \`${t("create_pull_request_review")}\` with \`approved: false\`, all comments, and the review body. body opens with \`> [!CAUTION]\\n> This PR introduces ...\`, then the Reviewed-changes summary. - ELSE IF NEW MUST-ADDRESS NON-CRITICAL FINDINGS (real consequences if shipped — incorrect behavior, missing validation, regressions the author should fix before merge): call \`${t("create_pull_request_review")}\` with \`approved: false\`, all comments, and the review body. body opens with \`> [!IMPORTANT]\\n> ...\`, then the Reviewed-changes summary. Do NOT use this tier for nits, style preferences, or "consider also" suggestions. - ELSE IF NEW MINOR SUGGESTIONS ONLY (single-line nits, doc/comment polish, defer-able observations, "rough edges"): call \`${t("create_pull_request_review")}\` with \`approved: false\`, all comments, and the review body. body opens directly with \`Reviewed the following changes:\\n\` (NO alert blockquote), then the Reviewed-changes summary. + - ELSE IF INFORMATIONAL OBSERVATIONS (mergeable as-is, but worth surfacing — e.g. prior feedback addressed cleanly with one minor stale doc reference, or a noteworthy positive observation): call \`${t("create_pull_request_review")}\` with \`approved: true\`, NO inline comments, and the review body. body opens with \`> [!NOTE]\\n> ...\` alert, then the Reviewed-changes summary. If a point is concrete enough to anchor to a line, downgrade the whole review to "minor suggestions only" (\`approved: false\`) instead — \`[!NOTE]\` and inline comments don't mix. - ELSE IF NO NEW ISSUES, SUBSTANTIVE CHANGES (new functionality, behavior changes, or fixes to prior review feedback): call \`${t("create_pull_request_review")}\` to create a PR review. If all previous reviews have been properly addressed and no new issues were discovered, you can set \`approved: true\`. body opens with \`No new issues. Reviewed the following changes:\\n\`, then the Reviewed-changes summary.`, }, {