pr-review-comments

Files (2)

• SKILL.md
• post_review_comments.py · scripts/

SKILL.md (first lines)

# PR Review Comments

Transform a single security review finding into targeted, actionable GitHub PR review comments.

## Input

The user provides:
1. A **finding** from a differential review report (raw text or reference to a finding ID like "F1", "L1")
2. The **PR number** and **repo** (or infer from current branch/context)
3. The **commit SHA** to anchor comments on (use HEAD of the PR branch)

## Writing Style

Write all comment text, including `<details>` blocks and code comments inside suggestions, in clear technical English inspired by ASD-STE100. Do not claim formal compliance.

- Lead with the conclusion or required action.
- Use common, direct words and one term for each concept.
- Avoid needless jargon, idioms, filler, repetition, and ornamental prose.
- Prefer active voice. Use imperative verbs for instructions.
- Aim for 20 words per instructional sentence and 25 words per descriptive sentence.
- Keep one topic in each paragraph.
- Preserve exact code, commands, identifiers, quotations, and technical terms.
- Accuracy, evidence, and necessary caveats take priority over brevity.
- Use hyphens instead of em dashes.
- Every word earns its place.

Apply these rules to review comments as follows:

- **Order:** the title names the defect. The first sentence says what breaks and when. The cause follows.
- **Verbs first:** start each fix bullet, task item, and change request with a verb: "Store", "Reject", "Add".
- **Plain words:** describe behavior with common verbs: "store", not "anchor"; "goes into", not "binds"; "stop", not "halt". Keep identifiers exact, and keep a domain term when the code uses it.
- **One term per concept** across the primary, its `<details>`, and every companion.
- **No skill vocabulary:** the PR author does not know "primary", "companion", "details block", "B-current", "landing zone", or "above the fold". Write "the comment on `File.sol`".
- **Length:** excluding code and diagrams, aim for about 100 words above the fold and 200 inside `<details>` in Format B, and about 200 words in Format C. Go over only for evidence or caveats the author needs.
- **Small suggestions:** select the smallest line range that contains the change. Keep code comments in a suggestion to one line where possible.
- **Positive, parallel phrasing:** "so X can find them" beats "otherwise X cannot find them". "No A, no B, no C" beats "it skips A, B, and C".
- **Numeric ranges:** use power notation. `[2^31, 2^32-1]` beats `[2,147,483,648, 4,294,967,295]`.
- **Read the whole comment once before showing it.** Rewrite any clause that makes you stumble. Split any sentence you must re-read.

These revisions turned a draft the user rejected as too verbose into the approved comment:

| Rejected | Approved |
|---|---|
| "On the live proxy `blockHashes` is a new, empty slot, so the first finalization after the upgrade takes this branch. It accepts only `parentBlockHash = 0`, and `_computePublicInput` binds that zero into the public input." | "After the upgrade, `blockHashes` is empty on the live proxy. The first finalization must then pass `parentBlockHash = 0`, and that zero goes into the public input." |
| "anchor the parent block hash in `reinitializeLineaRollupV10` (suggestion on the `LinethRollup.sol` companion), drop the zero-hash branch here" | "store the last finalized block hash in `reinitializeLineaRollupV10` (see the `LinethRollup.sol` comment), reject an empty parent here" |
| "In-PR landing zone: this reinitializer is the only point that can anchor the last finalized block hash before the first finalization binds it as `parentBlockHash`." | "The fix lands here: store the last finalized block hash before the first finalization needs it." |
| `<details>` of 380 words in five headed parts: evidence bullets, diagram, exploitability bound, fix notes, and a task list with full paths | `<details>` of 190 words: one diagram that carries the citations, three bullets, and a four-item task list |
| A companion suggestion that replaced 22 lines, most of them unchanged | A companion suggestion that replaced only the 3 lines that change |

## Output Formats

Three templates depending on complexity:

### Format A: Single-target fix (LOW findings, or MEDIUM where the fix is trivially self-contained)

For findings where the fix is one code change in one location AND the recommendation does not need to be split between this PR and a follow-up ticket. If either condition fails, use Format B.

**LOW findings prefer Format A** when one suggestion carries the core fix, even if a regression test is also needed. Make the suggestion compile on its own, for example by reusing an existing error. Name the test in one sentence after the severity bound. Use Format B when the core fix itself needs edits in more than one place.

Structure:
1. GitHub suggestion block with the fix
2. Bold severity+title: `**[LOW-N] Title that captures the issue**`
3. 2-3 sentences: what breaks, the cause, and why severity is not higher

Post as a **single PR review comment** targeting the exact file + line.

Example shape:
````
```suggestion
<fixed code>
```

**[LOW-1] TTL evictions share a 2-thread pool with 2PC timeouts**

A burst of terminal bundles can delay 2PC session timeouts. `BundleStatusStore` runs TTL evictions on the same 2-thread `xcall-circ` pool as those timeouts. Severity is LOW: each eviction is near-zero work, and the impact is delay only.
````

Rules:
- Title: `[SEVERITY-N]` + noun phrase capturing the defect (not the fix)
- Reader should absorb the issue in <10 seconds
- No "Recommended fix" section and no prose that restates the fix - the suggestion block IS the fix
- Bound the risk in the description (why it's LOW not HIGH)

### Format B: Multi-file or structural finding

Use Format B when the finding spans multiple files, needs non-contiguous edits, requires structural explanation, or has any evidence-backed split recommendation. The multi-file and structural paths describe comment shape; an evidence-backed split also requires Format B's two-scope structure even when its current-PR mitigation has one target. For a LOW finding, apply the Format A preference first.

**Choose one delivery mode before drafting:**

- **B-current (default):** the complete production fix and regression coverage land in this PR. Multi-file scope, structural work, lack of a suggestion block, additional tests, and LOW severity do not justify a follow-up ticket by themselves.
- **B-split (exception):** part of the fix cannot reasonably land in this PR because the user, maintainer, or governing ticket excludes it; an unmet prerequisite blocks it; it belongs to another repository, owner, release, or deployment boundary; or required test infrastructure does not exist and cannot be added within the current ticket. Cite the qualifying evidence in the draft. "Too large," "architectural," and "better as follow-up" do not qualify without a concrete blocker.

If no qualifying evidence exists, use B-current.

**B-current primary - three parts, fixed order:**

````
**[SEVERITY-N] Title that captures the defect**

<What's wrong: 2-4 sentences. What breaks and when, then the cause. No
citations, no diagrams.>

**Recommended fix**

- **In this PR (#TICKET, omit if none):** <the complete production fix and regression coverage, starting with a verb; include a ```suggestion``` block here when the fix is a small self-contained change on this file; otherwise name the file whose comment carries the change>

---

<details>
<summary><b>Problem detail and implementation scope</b></summary>

<Evidence: the failure path as a diagram or 2-4 bullets with file:line
citations. Severity bound: one sentence, unless the intro gives it. The full
fix description only when it does not fit above the fold. A numbered In-PR
task list of short items that start with verbs. Nothing here restates text
above the fold.>

</details>
````

From kyzooghost/skills, MIT.

More from kyzooghost/skills