fix: review process and cleanup
This commit is contained in:
@@ -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:
|
||||
|
||||
<details><summary>Technical details</summary>
|
||||
|
||||
\\\`\\\`\\\`\\\`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}
|
||||
|
||||
</details>
|
||||
\`\`\`
|
||||
@@ -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 \`<details>\` — 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 \`<details><summary>Technical details</summary>\` 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 \`<details><summary>Technical details</summary>\` 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 \`<details><summary>Technical details</summary>...</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 \`<details><summary>Technical details</summary>...</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 \`<details>Technical details</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 \`<details>Technical details</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/<path> b/<path>\` 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=<login> id=<fullDatabaseId> review=<reviewId> thread=<graphqlId>\`; 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 <short-sha>.\`), 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.
|
||||
|
||||
Reference in New Issue
Block a user