diff --git a/mcp/checkout.ts b/mcp/checkout.ts index f2d81ed..65b9d24 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -486,13 +486,19 @@ export function CheckoutPrTool(ctx: ToolContext) { commitLogUnavailable, hookWarning: checkoutResult.hookWarning, instructions: - `the diff file at diffPath contains a table of contents (TOC) listing every changed file with its line range. ` + - `use the read_file MCP tool to read sections of it. ` + - `for example, if the TOC says "src/foo.ts → lines 5-42", call: read_file({ path: diffPath, start_line: 5, end_line: 42 }). ` + + `the diff file at diffPath contains a table of contents (TOC) followed by the formatted diff for each file. ` + + `use read_file to read sections: if the TOC says "src/foo.ts → lines 5-42", call read_file({ path: diffPath, start_line: 5, end_line: 42 }). ` + + `IMPORTANT — two different sets of line numbers appear in the diff, do not confuse them: ` + + `(1) TOC line numbers like "lines 5-42" — these are DIFF-FILE positions for read_file calls only. ` + + `(2) Source file line numbers — inside each file's diff content, every line is prefixed "| oldLine | newLine | type | code". ` + + `These oldLine/newLine values are the ACTUAL file line numbers to use in create_pull_request_review comments. ` + + `For inline comments: path = the source file path from the "diff --git a/ b/" header (e.g. "apps/foo/bar.ts"), NOT the diffPath. ` + + `line = the newLine column value for RIGHT-side (added/context lines), or oldLine for LEFT-side (removed lines). ` + `IMPORTANT: to inspect the PR's changed files, read diffPath directly — ` + `do NOT run git diff or git show. The PR base branch is '${pr.baseRef}', NOT necessarily 'main' — ` + `if you must use git, use 'origin/${pr.baseRef}' as the base (e.g. git log origin/${pr.baseRef}..HEAD), ` + `but prefer diffPath for all diff analysis. ` + + `PHANTOM ISSUES: the diff only shows what changed, not the entire file. Before reporting an issue, verify it is caused by lines marked "+" in the diff (new code). Do not flag issues in pre-existing code unless the PR directly introduced or amplified the problem. ` + (incrementalDiffPath ? ` IMPORTANT: read incrementalDiffPath FIRST to understand what changed since last review, then use diffPath for full context.` : "") + diff --git a/mcp/review.ts b/mcp/review.ts index 37d073f..e52efeb 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -212,6 +212,8 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { "code block showing the corrected code. For example: " + "'SQL injection: id is interpolated directly into the query string.\\n\\n```ts\\nconst result = await db.query(\\'SELECT * FROM users WHERE id = $1\\', [id]);\\n```'. " + "The first submission may error once with a diff-coverage nudge — retry with the same arguments. " + + "Inline comments: 'path' must be the SOURCE FILE path (e.g. 'apps/foo/bar.ts') from the diff --git header, NOT the diffPath returned by checkout_pr. " + + "'line' must be the actual file line number from the '| newLine |' column in the formatted diff (not the TOC line range). " + "Inline comments can ONLY target files and lines that appear in the PR diff.", parameters: CreatePullRequestReview, execute: execute(async ({ pull_number, body, approved, commit_id, comments = [] }) => { diff --git a/modes.ts b/modes.ts index 4429d35..1f4f355 100644 --- a/modes.ts +++ b/modes.ts @@ -55,12 +55,12 @@ state before acting on findings. - Reviewed commits: - {sha_short} — {commit_subject} - ... -- Prior pullfrog review: none or {prior_sha_short} ({prior_review_html_url}) +- Prior shockbot review: none or {prior_sha_short} ({prior_review_html_url}) - Submitted at: {iso_timestamp} --> \`\`\` -Pull every metadata field from the \`checkout_pr\` tool's response — file count, commit count, base/head ref + SHA, the commit list. For \`IncrementalReview\` runs, populate \`Prior pullfrog review\` with the prior review's commit_id (short SHA) and \`html_url\` from \`list_pull_request_reviews\`. +Pull every metadata field from the \`checkout_pr\` tool's response — file count, commit count, base/head ref + SHA, the commit list. For \`IncrementalReview\` runs, populate \`Prior shockbot review\` with the prior review's commit_id (short SHA) and \`html_url\` from \`list_pull_request_reviews\`. ## 2. Cross-cutting issue sections (zero or more) @@ -73,23 +73,15 @@ For each cross-cutting concern, one \`### \` section. Use this exact shape:
Technical details -\\\`\\\`\\\`\\\`markdown -# {title repeated} - -## Affected sites +**Affected sites:** - {file path:line} — {what's wrong there} -- ... -## Required outcome +**Required outcome:** - {what the fix needs to achieve, not how to achieve it} -- ... -## Suggested approach (optional) -{When the fix shape is non-obvious, sketch one or more reasonable directions. Skip when the outcome alone makes the fix obvious.} +**Suggested approach** (optional): {sketch one or more reasonable directions when the fix shape is non-obvious} -## Open questions for the human (optional) -- {Any decision an implementing agent shouldn't make unilaterally — pricing thresholds, breaking-change policy, naming, scope of follow-up.} -\\\`\\\`\\\`\\\` +**Open questions for the human** (optional): {decisions an implementing agent shouldn't make unilaterally}
\`\`\` @@ -119,16 +111,16 @@ The example's value is its *shape*: a finding about absence (no deletion plan), **Technical-details block rules:** -- Wrapped in a 4-backtick markdown fence (\`\\\`\\\`\\\`\\\`markdown ... \\\`\\\`\\\`\\\`\`) so it's visually distinct, one-click copyable, and can contain its own 3-backtick code fences without escape gymnastics. The contents are agent-readable — a fix-agent will pull the body down and use this block as the brief. -- File paths and \`file:line\` refs are encouraged (and necessary) — the next agent uses these to navigate. Identifier density is fine here. -- Slightly more verbose than the absolute minimum is OK when it materially helps the next agent: a small code snippet showing the symptom, a short table of mismatched key/column pairs, a one-paragraph "why CI doesn't catch it" note. Skip massive regression-test scaffolding or full route rewrites — the implementing agent writes those. -- Use the four standard sections (\`Affected sites\`, \`Required outcome\`, optional \`Suggested approach\`, optional \`Open questions for the human\`). Skip the optional sections when they wouldn't add anything. +- Written as plain markdown bold-header sections directly inside \`
\` — no code fence wrapper. Use \`**Affected sites:**\`, \`**Required outcome:**\`, and optionally \`**Suggested approach:**\` and \`**Open questions for the human:**\`. Skip optional sections when they add nothing. +- File paths and \`file:line\` refs are encouraged — the next agent uses these to navigate. Identifier density is fine here. +- Slightly more verbose than the absolute minimum is OK when it materially helps the next agent: a small code snippet (3-backtick fence), a short table of mismatched values, a one-paragraph "why CI doesn't catch it" note. Skip massive scaffolding — the implementing agent writes that. +- The contents are agent-readable — a fix-agent will pull the body down and use this block as the brief. ## Inline technical details -Inline comments are short (~2-3 sentences) by default. When an inline finding has broader implications worth recording for a fix-agent — e.g. a localized bug whose proper fix requires touching several files, or where the right fix depends on a design decision the human needs to make — append a collapsed \`
Technical details\` block to the inline comment's body. Same shape as the body-section technical-details block (4-backtick fenced markdown, \`## Affected sites\` / \`## Required outcome\` / optional \`## Suggested approach\` / optional \`## Open questions for the human\`). +Inline comments are short (~2-3 sentences) by default. When an inline finding has broader implications worth recording for a fix-agent — e.g. a localized bug whose proper fix requires touching several files, or where the right fix depends on a design decision the human needs to make — append a collapsed \`
Technical details\` block to the inline comment's body. Same plain-markdown bold-header shape as the body-section technical-details block (\`**Affected sites:**\` / \`**Required outcome:**\` / optional \`**Suggested approach:**\` / optional \`**Open questions for the human:**\`). -GitHub renders the same markdown parser in inline comments as in the review body, so the collapsed-details affordance works the same way. The visible part of the inline comment stays scannable; the depth is one click away for any agent that needs it. +The visible part of the inline comment stays scannable; the depth is one click away for any agent that needs it. ## 3. \`### ℹ️ Nitpicks\` (optional, last section) @@ -146,7 +138,7 @@ Only when there are nits that for some reason can't be inlined. Filepaths in nit Inline comments use the same severity framing as body \`### \` sections, scaled down for line-anchored use: - **Lead with a 1-2 sentence problem statement.** The reader is looking at the line in question, so don't restate what the line says — describe what's wrong with it. Optionally prefix the visible line with a severity emoji (🚨 / ⚠️ / ℹ️) when severity isn't obvious from context. -- **Optional \`
Technical details...
\` collapsible** for findings whose technical context (longer file:line references, related-code snippets, suggested approach, regression-risk notes) would overwhelm the human-readable lead-in. Same agent-readable purpose, same 4-backtick fence shape, and same 4-section structure as the body's technical-details block — see *Inline technical details* above. Encouraged whenever the depth helps a downstream fix-agent; don't force one when the inline lead-in already says everything. +- **Optional \`
Technical details...
\` collapsible** for findings whose technical context (longer file:line references, related-code snippets, suggested approach, regression-risk notes) would overwhelm the human-readable lead-in. Same plain-markdown bold-header shape as the body technical-details block — see *Inline technical details* above. Encouraged whenever the depth helps a downstream fix-agent; don't force one when the inline lead-in already says everything. - **Visible portion ≤ 2-3 sentences.** If you find yourself writing more, that's the cue to split the depth into the \`Technical details\` collapsible. ## Body-wide rules @@ -175,7 +167,7 @@ export function computeModes(agentId: AgentId): Mode[] { 3. **setup**: checkout or create the branch: - **PR event, modifying the existing PR**: call \`${t("checkout_pr")}\` - - **new branch**: use \`${t("git")}\` to create a branch (\`git checkout -b pullfrog/branch-name\`) + - **new branch**: use \`${t("git")}\` to create a branch (\`git checkout -b shockbot/branch-name\`) 4. **build**: implement changes using your native file and shell tools: - follow the plan (if you ran a plan phase) @@ -380,7 +372,13 @@ 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 with NEW line numbers from the 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. use GitHub permalink format for code references. for impact-analysis findings (stale references after rename/remove), report them in the review body ordered by severity (runtime breakage > incorrect docs > stale comments) rather than as inline comments unless they're anchored to a specific line. + 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). + + **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. + - \`line\`: the value in the \`| newLine |\` column of the formatted diff for the target line (RIGHT side, for added/context lines), or \`| oldLine |\` for LEFT side (removed lines). These are actual file line numbers, NOT the TOC position numbers. + + for impact-analysis findings (stale references after rename/remove), report them in the review body ordered by severity (runtime breakage > incorrect docs > stale comments) rather than as inline comments unless they're anchored to a specific line. 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. @@ -412,7 +410,7 @@ ${PR_SUMMARY_FORMAT}`, }, // IncrementalReview shares Review's 0-or-2+ lens pattern AND its body // format (PR_SUMMARY_FORMAT), scoped to the incremental delta against the - // prior shockbot review. The "issues must be NEW since the last Pullfrog + // prior shockbot review. The "issues must be NEW since the last shockbot // review" filter lives at aggregation time (step 8), NOT in the subagent // prompt — pushing the filter into subagents matches the canonical anneal // anti-pattern of "list known pre-existing failures — don't flag these" @@ -435,11 +433,11 @@ ${PR_SUMMARY_FORMAT}`, 4. **prior feedback — read AND retire it**: fetch previous reviews via \`${t("list_pull_request_reviews")}\`, then call \`${t("get_review_comments")}\` on each prior shockbot review. Each thread renders as a section whose first line is a fenced tag \`comment author= id= review= thread=\`; section headers carry \`[RESOLVED]\` / \`[OUTDATED]\` when relevant. For every **open, Pullfrog-originated** thread, decide and act: - - **Pullfrog-originated** means the FIRST \`comment author=...\` tag in the section is \`author=pullfrog[bot]\`. The \`*\` marker on individual comments is unrelated — it flags whether a comment belongs to the queried review, not whether it is the thread root. + - **Shockbot-originated** means the FIRST \`comment author=...\` tag in the section is \`author=shockbot[bot]\`. The \`*\` marker on individual comments is unrelated — it flags whether a comment belongs to the queried review, not whether it is the thread root. - **addressed?** read the file at the thread's anchor and judge whether the substantive concern is now resolved by the new commits. Lines being modified isn't enough: reformatting, renaming, or moving the same code elsewhere doesn't address a concern. If the comment raised multiple distinct concerns, ALL must be addressed. The \`[OUTDATED]\` tag means GitHub moved the anchor (line shift, force-push, rename) — it does NOT mean the concern was addressed; re-read the code at its new location before deciding. - **if addressed**: call \`${t("reply_to_review_comment")}\` with the root tag's numeric \`id=\` as \`comment_id\` (NOT the \`thread=\` value — that's a separate GraphQL ID used only by resolve) and a one-line body (e.g. \`Addressed in .\`), then call \`${t("resolve_review_thread")}\` with the root tag's \`thread=\` value as \`thread_id\`. Do this BEFORE drafting the new review so the GitHub thread state aligns with the new review by the time it lands. - **if uncertain or partially addressed**: leave open. False-positive resolutions erode trust faster than false negatives. - - **scope**: only retire Pullfrog-originated threads. Threads from human reviewers belong to those humans to resolve, even if the commit happened to address them. + - **scope**: only retire shockbot-originated threads. Threads from human reviewers belong to those humans to resolve, even if the commit happened to address them. The remaining open threads feed step 8's dedup filter — anything already flagged and unchanged by the new commits should not be re-raised. The rolling PR summary snapshot is the durable record of retire activity; you don't need to surface it in the review body. @@ -553,7 +551,8 @@ ${PR_SUMMARY_FORMAT}`, }, { name: "ResolveConflicts", - description: "Resolve merge conflicts in a PR branch against the base branch", + description: + "Resolve merge conflicts in a PR branch against the base branch", prompt: `### Checklist 1. **task list**: create your task list for this run as your first action. diff --git a/utils/instructions.ts b/utils/instructions.ts index 17b7eac..4901a2f 100644 --- a/utils/instructions.ts +++ b/utils/instructions.ts @@ -229,7 +229,7 @@ Rules: - All code changes must be pushed to a pull request (new or existing) before the run ends. This environment is ephemeral — unpushed work is lost permanently. \`git status\` must be clean when you finish. - Protected branches (default branch) are blocked from direct pushes in restricted mode. Do not use \`git push\` directly — it will fail without credentials. - Do not attempt to configure git credentials manually — the ${shockbotMcpName} server handles all authentication internally. -- Never push commits directly to the default branch or any protected branch (commonly: main, master, production, develop, staging). Always create a feature branch following the pattern: \`pullfrog/-\` (e.g., \`pullfrog/123-fix-login-bug\`). +- Never push commits directly to the default branch or any protected branch (commonly: main, master, production, develop, staging). Always create a feature branch following the pattern: \`shockbot/-\` (e.g., \`pullfrog/123-fix-login-bug\`). - Never add co-author trailers (e.g., "Co-authored-by" or "Co-Authored-By") to commit messages. - Untracked files from tests or tooling (e.g. \`coverage/\`) often remain *after* your last commit and still block \`${t("push_branch")}\` — delete them, extend \`.gitignore\`, or only add files that truly belong in the repo. - \`${t("push_branch")}\` runs the repository's optional **prepush** hook (commonly tests or lint) — best-effort. On failure the output is returned, the hook is latched off, and every subsequent \`${t("push_branch")}\` call this run skips it. If the failure is unrelated to your changes (pre-existing breakage, env-dependent test, flaky check), just call \`${t("push_branch")}\` again. If it could be a real bug in your code, ${ctx.payload.shell === "disabled" ? `fix it from the failure output (shell is disabled, so you can't re-run the hook)` : `re-run the hook via the shell tool to iterate — \`${t("push_branch")}\` itself won't re-run it`}. Don't describe the failure as an infrastructure "timeout" unless the tool output clearly shows one.