Files

T

Marcos 30d2a3d7da feat(factory): five review skills + a per-review cost model

Skills — the primary now loads conditionally (each one is input tokens), per a
load table in pragent.md:

- attention-tiering: classify every PR trivial/lite/full/oversized BEFORE
  reading anything, and cap file reads, linter runs and subagent fan-out per
  tier. This is the cost governor; the other skills defer to its budget.
- linter-playbook: per-ecosystem detect-and-run commands scoped to changed
  files, the never-install rule, and how to turn a diagnostic into a finding
  instead of pasting tool output.
- security-lens: the inline security checklist for when @security isn't worth
  delegating, built around a source -> sink test each finding must pass.
- malicious-change: hostile-PR detection — injection aimed at the reviewer,
  install/CI-time hooks, obfuscated payloads, dependency confusion, logic
  backdoors. Complements the runtime containment added in the previous commit:
  that stops the agent being hijacked, this makes it report the attempt.
- comment-craft: how to write problem/fix/suggestion so a maintainer can act in
  one read, and what to cut.

pilot/cost_model.py — prices a review against published Claude and OpenAI rates
(fetched 2026-08-18). Prompt sizes are measured from the factory files rather
than guessed; per-tier workloads come from the tiering budgets. The model is
explicit about the thing that actually dominates an agent loop: the whole
conversation is resent every step, so caching moves ~2.3x of the bill.

Blended over a 5/35/55/5 mix with caching on: ~$0.61/PR on Opus 5 or GPT-5.6
Sol, ~$0.24 on Sonnet 5 or Terra, ~$0.12 on Haiku 4.5, ~$0.02 on Luna. At 350
PRs/month that's ~$212 / ~$85 / ~$43 / ~$8.50.

Tests: 101 -> 122.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B11e8TZZxJyzHW7jj7KWUN

2026-08-18 04:53:49 +00:00

4.1 KiB

Raw Permalink Blame History

name, description

name	description
comment-craft	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

{
  "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.

4.1 KiB Raw Permalink Blame History