Imported from D3Hunter/ai-engineer (skills/d3hunter-pr-reviewing/SKILL.md). Install upstream with npx skills add D3Hunter/ai-engineer --skill d3hunter-pr-reviewing. Copyright stays with the author.

PR Reviewing (D3Hunter Style)

Overview

Review for correctness and operational safety first, then maintainability.

The comment style is concise and action-oriented:

• ask direct questions when intent is unclear
• propose a concrete alternative when possible
• request tests for risk-bearing behavior
• use small inline suggestion blocks for local rewrites

Use references/phrase-bank.md as a required tone source before drafting final comments.

When To Use

Use this skill when a PR likely changes behavior and needs a structured second-opinion pass.

Typical triggers:

• touches versioned metadata, bootstrap, or migration paths
• changes retry/cancel/context/lifecycle behavior
• changes concurrency, locking, or task scheduling flow
• changes compatibility behavior across upgrade/downgrade or mixed-version nodes
• modifies failure classification, logging/observability, or fallback paths
• changes tests for critical behavior, negative paths, or determinism guarantees

When Not To Use

Do not use this skill as the primary workflow when:

• the request is purely mechanical formatting/lint cleanup with no behavior impact
• the diff is fully generated code where manual style feedback adds little value
• changes are typo-only or wording-only and do not affect business semantics

In those cases, keep feedback minimal and mechanical.

Review Order (Default Sequence)

1. Correctness and invariants
2. Compatibility and upgrade safety
3. Code structure, package boundaries, and duplication
4. Failure semantics and observability
5. Concurrency, context, and lifecycle
6. Scope and complexity
7. Performance and scaling
8. API clarity and naming
9. Comments and intent documentation
10. Test quality and coverage

Risk-Adaptive Entry (Hotspot First)

Use this fast-path before the full checklist when the PR is large or high risk.

Hotspot activation signals:

• touches pkg/ddl, pkg/meta, pkg/session/bootstrap, pkg/disttask, pkg/lightning
• touches transaction/lock/retry/cancel behavior
• spans many files/packages and needs quick risk triage

Hotspot pass:

1. Identify hotspot files/modules first.
2. Prioritize checks 1, 2, 3, 4, 5, 7, 10 in those hotspots.
3. If Blocker/Major evidence appears, report early with concrete scope and evidence.
4. Then complete the full 1 -> 10 default sequence for the entire diff (including hotspot files).
5. Before posting final output, verify each hotspot file/module has at least one pass over all 1 -> 10 categories (or mark explicit N/A for category/file pairs that truly do not apply).

Review Workflow (Execution Checklist)

Copy this checklist and track progress:

Review Progress:
- [ ] Step 1: Triage diff scope (behavioral vs mechanical-only changes) and identify hotspots
- [ ] Step 2: If hotspot-eligible, run Hotspot First pass; then run full Review Order (1 -> 10) across the entire diff (hotspots + non-hotspots) using `references/checklist-by-category.md`
- [ ] Step 3: Apply Evidence Gate and Confidence-to-Action Matrix for each non-trivial finding
- [ ] Step 4: Run D3Hunter wording pass using [references/phrase-bank.md](references/phrase-bank.md) for each drafted finding
- [ ] Step 5: Select output mode (Threaded Review Mode vs Inline Comment Mode vs Standalone Summary Mode)
- [ ] Step 6: Assign severity and include required rubric fields in posted output
- [ ] Step 7: Produce output using Review Output Contract

Stop conditions (do not proceed to prescriptive feedback until resolved):

• no objective signal for a non-trivial issue -> ask a clarifying question instead of prescribing a fix
• cannot name concrete scope (file/line/module) -> gather more evidence first
• cannot state concrete risk if unchanged -> downgrade to suggestion/question
• behavior changes are not test-backed and cannot be reasoned from diff -> require targeted validation before merge
• any required category checks in the Review Order pass are incomplete -> do not post final output

Review Output Contract

Use this contract for all final review output.

Global Rules (all output modes)

• Add this exact bold attribution line once in summary sections only: **AI-generated review based on @D3Hunter’s standards; manual follow-up after comments are addressed.**
• Do not repeat the attribution line inside per-finding items/comments (for example, comments[] entries in a single GitHub review object).
• One finding = one actionable issue.
• Sort by risk: present highest-risk findings first.
• Tone guardrail: each finding must use at least one adapted stem from references/phrase-bank.md (opener or change-request line).
• No-findings gate: do not output No findings. unless the full Review Order pass (1 -> 10) is completed.
• Non-trivial findings must include severity plus all 5 rubric fields: Why, Scope, Risk if unchanged, Evidence, Change request.
• Markdown structure is required: use section titles (###/####) and bold field labels (**Why**, **Scope**, etc.) exactly as shown in templates.
• Severity line format is required: use emoji + bold severity tag (🚨 **[Blocker]**, ⚠️ **[Major]**, 🟡 **[Minor]**, ℹ️ **[Info]**, 🧹 **[Nit]**).
• Nit/Info findings can be shorter, but must still be explicit about scope and requested change.

Canonical templates and payload examples are in:

• references/output-examples.md

Shared finding structure for non-trivial findings:

• Severity line (<emoji> **[Severity]** <short title>)
• Why
• Scope
• Risk if unchanged
• Evidence
• Change request

Inline Comment Mode

Use when posting direct line-level comments without a summary thread.

Requirements:

• one finding = one actionable issue
• each non-trivial finding uses severity + the full 5-field rubric above
• Nit/Info findings stay short but explicit about scope and ask
• do not prepend the attribution line in individual inline comment bodies
• template: references/output-examples.md

Threaded Review Mode (default when platform supports replies)

Top-level summary comment (exactly one)

• start with Findings (highest risk first)
• include a concise index only (severity, title, scope, risk if unchanged in one line per finding)
• do not expand the full severity + 5-field rubric in the top-level summary
• end with Required validation before merge
• template: references/output-examples.md

Sub-comments under the summary thread

• post one sub-comment per finding
• one finding = one actionable issue
• non-trivial findings must include severity + full 5-field rubric (Why, Scope, Risk if unchanged, Evidence, Change request)
• Nit/Info findings may be shorter but must keep clear scope and ask
• template: references/output-examples.md

GitHub PR submission contract (required when posting via gh api)

• create exactly one review object via POST /repos/{owner}/{repo}/pulls/{pull_number}/reviews
• set summary content in review body (use the top-level summary template, including the attribution line once)
• put each finding in a separate item in comments[] (one finding = one inline comment object)
• do not include the attribution line inside comments[] item bodies
• submit once with event=COMMENT; do not post one review per finding
• if any finding has no valid inline anchor, keep it in summary body and still submit one review object
• payload shape example: references/output-examples.md

Standalone Summary Mode (single comment fallback)

Use when reply threads/sub-comments are unavailable, or when a single-summary output is explicitly requested.

Requirements:

• start with Findings (highest risk first)
• list findings in risk order
• every non-trivial finding includes severity + the same 5 rubric fields used in Inline Comment Mode
• end with Required validation before merge and concrete test/verification items
• template: references/output-examples.md

No-findings Mode

Required output:

• write No findings.
• include Residual risks / verification gaps
• include Suggested validation
• only use this mode after the full Review Order pass (1 -> 10) is completed
• template: references/output-examples.md

Core Checklist

Use 1 -> 10 as the default pass order for every behavior-changing review.

Detailed checks, heuristics, and phrase stems live in:

• references/checklist-by-category.md
• references/phrase-bank.md

Quick category index:

1. Correctness and invariants
2. Compatibility and upgrade safety
3. Code structure, encapsulation, and duplication
4. Failure semantics and observability
5. Concurrency, context, and lifecycle
6. Scope and complexity
7. Performance and scaling
8. API clarity and naming
9. Comments and intent documentation
10. Tests and determinism

D3Hunter Wording Pass (Required)

Run this pass before posting any comment:

1. Select one stem family in references/phrase-bank.md: clarify intent, request change, tests and validation, comments and rationale, or compatibility and scale.
2. Adapt one stem to the exact code scope and evidence from the current diff. Do not leave generic wording.
3. Keep the opening short and direct (for example: why, can we, please add, seems no need), then provide full rubric fields for non-trivial findings.
4. Keep one comment to one actionable issue. Split comments if there are multiple asks.
5. Avoid robotic repetition: if the same stem appears in nearby comments, switch to another stem from the same family.

Finding Decision Pipeline (Single Flow)

Use this pipeline for every non-trivial finding. Do not skip steps.

Confidence-to-Action Matrix

1. Scope one issue:
   • one finding = one actionable issue
   • if the issue is comment-related, decide first:
     • non-obvious/business-decision logic -> ask for a short why comment
     • comment only restates obvious workflow -> suggest removing it
     • changed branch condition/invariant with stale rationale -> request comment update
2. Apply Evidence Gate (false-positive guard):
   • before prescribing a fix, confirm at least one objective signal:
     • invariant mismatch
     • reproducible failing scenario (test/case/path)
     • contradiction between code path and expected behavior
     • measurable performance or scale risk
     • concrete structure/duplication smell visible in the diff
     • duplicate definition in one scope (for example duplicated select $$ fixture entry) with overwrite/shadowing risk
   • if no objective signal is available, ask a clarifying question first and stop escalation
3. Assign confidence and action using the Confidence-to-Action Matrix:
   • if confidence changes after new evidence, update severity and requested action
4. Assign severity and merge policy:
   • Blocker: invariant break, correctness regression risk, data race risk, wrong failure model
     • action: must fix before merge
   • Major: strong package-boundary coupling, high-scale performance risk, cancellation/lifecycle bug, critical missing test
     • action: fix before merge, or document explicit justification with follow-up tracking
   • Minor: naming/placement/readability or low-risk cleanup
     • action: optional in current PR; may defer if acknowledged
   • Info: observation or low-risk concern worth validation
     • action: non-blocking; acknowledge and optionally track
   • Nit: tiny style/wording preference with negligible risk
     • action: fully optional; do not block merge
5. Format output with severity + the 5-field rubric (inline or summary):
   • why: why this matters (correctness, readability, testability, performance, maintainability)
   • scope: exact change boundary (line/block/file/module)
   • risk if unchanged: concrete consequence
   • evidence: observed behavior, contradiction, failing path, or measurable signal
   • change request: specific and actionable fix direction
6. Choose output mode and render using Review Output Contract:
   • Threaded Review Mode (default when reply threads exist): one top-level summary + per-finding sub-comments
   • Inline Comment Mode: direct line-level finding comments without a summary thread
   • Standalone Summary Mode: single-comment fallback when sub-comments/reply threads are unavailable or explicitly requested
7. Final summary contract:
   • list findings first, highest risk first
   • every non-trivial finding must include all required fields
   • end with required validation/tests before merge

Comment Decision Flow

Comment-related decisions are handled in Step 1 of Finding Decision Pipeline (Single Flow). Do not run a separate decision path; keep one issue per finding.

Comment Writing Rules

• Keep one comment to one actionable point.
• Phrase bank is mandatory input for wording: draft each comment with one adapted stem from references/phrase-bank.md.
• Prefer frequent D3Hunter starters when they match evidence: why, can we, please, seems, maybe.
• Use stems as scaffolding, not copy-paste text; always bind wording to concrete scope/risk/evidence.
• Prefer question + direction instead of vague criticism.
• Use imperative short form for clear requests.
• For repeated issues in nearby code, comment once and include explicit repeat scope (path:line list or block range).
• Use suggestion blocks only for small local changes.
• Extract by responsibility boundary, not line count alone.
• Avoid utility dumping; keep helpers near the dominant caller unless multi-package reuse is real.
• Prefer unexported helper methods first; export only when cross-package reuse is required.
• Add comments for non-obvious logic and business-driven branches, focusing on why.
• Avoid comments for plain workflow code that is already readable.
• Keep comments concise and durable; prefer intent/invariant over restating code.
• For Blocker/Major findings or summary output, expand shorthand into full rubric fields (why/scope/risk/evidence/change request).

Compact Drafting Helper

Use this while drafting, then expand to full severity + 5-field rubric before posting:

• Issue: one sentence
• Why: one sentence
• Scope: one sentence
• Evidence: one sentence
• Ask: one sentence (Ask maps to change request)

Examples:

• Issue: this branch is business-rule driven but has no rationale comment. Why: future changes may break compatibility silently. Change request: add a short why-comment with the decision/invariant.
• Issue: same validation logic appears in 3 places. Why: behavior drift and test duplication risk. Change request: extract one helper near the dominant caller and reuse it.

References

• category-level checklist and templates:
  • references/checklist-by-category.md
• phrase stems:
  • references/phrase-bank.md
• output templates and examples:
  • references/output-examples.md