From 5ea8a23d80fe92781920118e89c8d026bd0f18c0 Mon Sep 17 00:00:00 2001 From: wolfy Date: Sun, 31 May 2026 13:57:30 -0500 Subject: [PATCH] fix: review should leave comments on actual files --- modes.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/modes.ts b/modes.ts index 1f4f355..41bb44f 100644 --- a/modes.ts +++ b/modes.ts @@ -372,7 +372,11 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`, **Hunt for non-anchored concerns before drafting.** After collecting your anchored findings, deliberately scan for concerns that have no specific line to point at — typically: deletion / cleanup plans for code the diff replaces or shadows; rollout sequencing (what happens to in-flight state during deploy / revert?); coverage gaps the diff implies but doesn't add; scope questions that only the human can answer (e.g. is the legacy path going away or is this a long-term dual track?); architectural risks the diff opens up that aren't a single-line bug. On substantial PRs (migrations, refactors, multi-file rewrites, version bumps that change runtime semantics), at least one such concern almost always exists; if you can't think of any, your bar is probably too high. - for surviving findings, draft inline comments — every comment must be actionable, 2-3 sentences max in the visible part. attach a \`
Technical details
\` block to any inline comment whose fix is non-trivial or has cross-file implications (see Inline technical details in the format below). + for surviving findings that anchor to a specific file and line: **ALWAYS use inline comments** (pass via the \`comments\` parameter). NEVER put a line-anchored finding in the body as a \`### \` section — that is the wrong output format and wastes the reviewer's time. Every actionable concern that has a specific line to point at MUST be an inline comment. + + for surviving findings with NO specific line anchor (absence of code, sequencing/rollout risk, design decisions): use body \`### \` sections. + + inline comments — every comment must be actionable, 2-3 sentences max in the visible part. attach a \`
Technical details
\` block to any inline comment whose fix is non-trivial or has cross-file implications (see Inline technical details in the format below). **Inline comment anchoring** (critical — get this wrong and all comments are silently dropped): - \`path\`: the source file path from the \`diff --git a/ b/\` header in the diff (e.g. \`apps/foo/bar.ts\`). This is NEVER the diffPath temp file — that path is only for \`read_file\` calls. @@ -382,6 +386,8 @@ For simple, well-defined tasks, skip the plan phase and go straight to build.`, 7. **submit**: ALWAYS submit exactly one review via \`${t("create_pull_request_review")}\`. Do NOT call \`report_progress\` — the review is the final record and the progress comment will be cleaned up automatically. + **MANDATORY pre-submission self-check**: before calling \`${t("create_pull_request_review")}\`, scan your draft body for \`### \` sections. For each one that mentions a specific file and line number, move it to the \`comments\` array as an inline comment instead. A \`### \` section in the body is ONLY justified when it has NO specific line to anchor to. If you have 3+ body sections that all have file:line references, you have made an error — convert them. + 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. 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. @@ -489,6 +495,8 @@ ${PR_SUMMARY_FORMAT}`, **Hunt for non-anchored concerns before drafting.** After collecting your anchored findings, deliberately scan for concerns that have no specific line to point at — typically: deletion / cleanup plans for code the new commits replace or shadow; rollout sequencing (what happens to in-flight state during deploy / revert?); coverage gaps the new commits imply but don't add; scope questions that only the human can answer (e.g. is the legacy path going away or is this a long-term dual track?); architectural risks the new commits open up that aren't a single-line bug. On substantial incremental diffs (migrations, refactors, multi-file rewrites, version bumps that change runtime semantics), at least one such concern almost always exists; if you can't think of any, your bar is probably too high. + for surviving findings that anchor to a specific file and line: **ALWAYS use inline comments** (pass via the \`comments\` parameter). NEVER put a line-anchored finding in the body as a \`### \` section. Every actionable concern with a specific anchor MUST be an inline comment. + draft inline comments with NEW line numbers from the full PR diff — attach a \`
Technical details
\` block to any inline comment whose fix is non-trivial or has cross-file implications (see Inline technical details in the format below). every comment must be actionable, 2-3 sentences max in the visible part. 9. **build the review body**: use the same default format as Review mode (preamble + optional cross-cutting \`### \` sections + optional \`### ℹ️ Nitpicks\`) — scoped to the **incremental delta**, not the full PR. The "Reviewed changes" bullets describe what changed since the prior shockbot review (each bullet starts with a past-tense verb, e.g. \`- Extracted shared CLI runtime into a single module\`). Do NOT include a separate "Prior review feedback" checklist — that's tracked in the rolling PR summary snapshot for the next agent run, and surfacing it in the user-facing body is noise (changes that addressed prior feedback are already covered by the Reviewed-changes bullets). In some cases you may receive a complete diff for the whole PR instead of an incremental one; when this happens, determine what changed since Pullfrog's most recent review yourself before drafting bullets. @@ -497,6 +505,8 @@ ${PR_SUMMARY_FORMAT}`, Same callout ladder as Review mode — \`[!CAUTION]\` (red, "will break") → \`[!IMPORTANT]\` (purple, "must address before merging") → \`> ℹ️ ...\` (informational, "minor suggestions only") → \`> ✅ ...\` (green friendly, "no concerns"). 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. + **MANDATORY pre-submission self-check**: before calling \`${t("create_pull_request_review")}\`, scan your draft body for \`### \` sections. For each one that mentions a specific file and line number, move it to the \`comments\` array as an inline comment instead. A \`### \` section in the body is ONLY justified when it has NO specific line to anchor to. If you have 3+ body sections that all have file:line references, you have made an error — convert them. + 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. - IF NO NEW ISSUES, NON-SUBSTANTIVE CHANGES ONLY (trivial formatting, import reordering, comment tweaks): do NOT submit a review. Instead call \`${t("report_progress")}\` with a 1-2 sentence note explaining no review was warranted (e.g. "No new issues. Changes since last review are formatting-only."). this leaves a visible signal that the run completed.