---
name: comment-craft
description: How to write the problem/fix/suggestion text of a finding so a maintainer can act on it in one read — concreteness, failure scenarios, tone, and what to cut. Load before writing the findings JSON.
---

# Comment craft

A finding is read by someone who wrote the code, is mid-task, and has other PRs
waiting. It has one job: make the defect obvious and the fix cheap. Everything
that doesn't serve that is noise, and noise is why teams mute review bots.

## `problem` — one line, concrete, falsifiable

State **what breaks and when**, not what the code is.

- Bad: "This could potentially cause issues with error handling."
- Bad: "Consider whether this handles the null case."
- Good: "`user.email` is `None` for SSO accounts, so `.lower()` on line 44
  raises `AttributeError` on every SSO login."

Include the trigger. A defect with no input that reaches it is a style opinion.
If you can't name the trigger, either find it or drop the finding.

Never phrase a finding as a question. "Is this intentional?" puts the work back
on the author and asserts nothing. If you believe it's wrong, say so; if you're
unsure, say what you checked and what you couldn't ("`scheduleJob` is the only
caller I found; if there are others outside this repo this may be fine").

## `fix` — one line, actionable

Name the change, not the goal. "Handle the error properly" is not a fix;
"return `errors.Join(err, ctx.Err())` instead of discarding `err`" is.

Empty `fix` is allowed and honest when the remedy is architectural. Don't fill it
with a paraphrase of the problem.

## `suggestion` — literal replacement code, or empty

- It must be **the lines that replace the flagged location**, at the file's real
  indentation, in the file's language and style.
- Minimal: the changed lines only. Not the whole function, not surrounding
  context, not a diff — no `+`/`-` markers.
- It must be **safe to apply blind**. If it needs an import that isn't there, a
  new helper, or a decision the author has to make, leave `suggestion` empty and
  put the shape in `fix`.
- Empty is the right answer for missing tests, architectural notes, and anything
  spanning multiple hunks.

## Severity honesty

Inflated severity is the fastest way to get a bot ignored. Anchor each level to
consequence, not to how interesting the bug is:

- `critical`/`high` need a reachable path. If you had to invent an unusual caller
  to make it break, it's `medium`.
- One `critical` in a review is credible. Four usually means the rubric slipped.
- A clean diff with `findings: []` is a correct, valuable result. Never
  manufacture a finding to look useful.

## Cut these

- Praise ("Nice refactor!", "Good use of…"). Zero information.
- Restating the diff back to the author.
- Style and formatting the repo's formatter owns.
- Speculation with no trigger ("what if this grows to a million rows").
- Duplicates: one finding per root cause. Same bug in five files → one finding at
  the clearest site, with the other paths listed in `problem`.
- Anything already covered in `prior_reviews`.
- Meta-commentary about being an AI, about your confidence, or about the review
  process.

## Tone

Direct, technical, about the code. No hedging stacks ("it might possibly be
worth perhaps considering"), no apologies, no exclamation marks. Assume the
author is competent and busy.

## Worked example

```json
{
  "severity": "high",
  "path": "api/handlers/upload.go",
  "line": 88,
  "problem": "The extracted path is joined to uploadDir without checking the result stays inside it, so a tar entry named ../../etc/cron.d/x writes outside the upload root.",
  "fix": "Resolve the joined path and reject it unless it is within uploadDir.",
  "suggestion": "\tdst := filepath.Join(uploadDir, hdr.Name)\n\tif !strings.HasPrefix(filepath.Clean(dst)+string(os.PathSeparator), filepath.Clean(uploadDir)+string(os.PathSeparator)) {\n\t\treturn fmt.Errorf(\"illegal path in archive: %s\", hdr.Name)\n\t}",
  "reference": "https://cwe.mitre.org/data/definitions/22.html"
}
```

Trigger named, fix specific, suggestion applies cleanly, reference is the
actual weakness class rather than a generic security link.