addyosmani / addyosmani/agent-skills

Review surfaces rank findings by consequence only — no likelihood axis, so improbable findings block merges

未关闭
#436 2 条评论 1 个 reaction 已指派 0 人 在 GitHub 查看
主要语言
JavaScript
星标
93.8k
派生
10k
平均合并
3 天 15 小时
30 天内合并 PR
23

描述

## The gap

Every review surface in this repo classifies findings by **consequence** — what happens *if* the issue fires. None of the general ones ask **how likely it is to fire at all**. So a finding that is true, and would be bad, but needs a conjunction of conditions that will never occur in the project, is indistinguishable from one that bites on the first realistic input.

This is the dominant failure mode of LLM reviewers in practice. Both GPT- and Claude-family models will produce a near-endless supply of technically-correct findings whose triggering conditions are absurd — "a second worker could observe a torn snapshot" against a design that pins one consumer, "this breaks at 10M rows" against an internal tool with 400. The reviewer is not wrong. The finding is still waste: the author has to read it, evaluate it, and dismiss it, and under `REQUEST CHANGES` semantics it can block a merge.

Neither existing axis can filter these:

- **Severity can't.** Severity is defined by blast radius, and a rare bug is still severe *if* it fires. `agents/code-reviewer.md:51` defines Critical as "security vulnerability, data loss risk, broken functionality" — all consequence, no probability.
- **Confidence can't.** Confidence answers "is my claim true?". These findings score high on it. A finding can be 0.95 certainly-true and 0.05 will-ever-happen at the same time; one number cannot carry both.

## Evidence (all citations at `7829ffd`)

| Surface | How findings are ranked | Likelihood handling |
|---|---|---|
| `agents/code-reviewer.md:51-55` | Critical / Important / Suggestion | **none** |
| `skills/code-review-and-quality/SKILL.md:181-187` | Critical / Nit / Optional / FYI prefix table | **none** |
| `skills/doubt-driven-development/SKILL.md:172-177` | contract-misread / valid+actionable / valid-trade-off / noise | **none** (see below) |
| `agents/security-auditor.md:57-65, 99` | severity by exploitability | informal, prose only |
| `agents/web-performance-auditor.md:41-51` | severity by CWV threshold | Metric-Honesty Rule only |

**This is an internal inconsistency, which is one of the reasons CONTRIBUTING lists for filing.** `security-auditor` already encodes exactly the right instinct — rule 1 is "Focus on exploitable vulnerabilities, not theoretical risks" (`:99`) and its severity table bottoms out at `Low = "Theoretical risk"` (`:64`). `security-and-hardening/SKILL.md:295` asks "Is the vulnerability exploitable given your deployment context". `web-performance-auditor` has the Metric-Honesty Rule, which is the same discipline applied to measurement. The security and perf specialists got a likelihood discipline; the general reviewer that runs on *every* change did not.

Two specifics worth calling out:

**`doubt-driven-development` is the most affected**, because its whole design multiplies finding volume: the reviewer prompt at `:90-104` is explicitly "Find what is wrong… Assume the author is overconfident… Do NOT validate", and Step 5 keeps looping while findings are non-trivial. RECONCILE's four classes look like they cover this but don't: `valid trade-off` (`:176`) is about *fix cost* exceeding *acceptance cost*, and `noise` (`:177`) is about the reviewer *lacking context*. A finding that is valid, cheap to fix, and correctly understood by a fully-contexted reviewer — but will never occur — has no class. It lands in `valid + actionable`, triggers a change, and re-loops.

**`code-review-and-quality` already gestures at this and stops short.** "Lead with what matters" (`:191`) orders by leverage, and "Quantify problems when possible" (`:275`) asks for "~50ms per item" instead of "could be slow" — quantified *impact*. Neither asks for quantified *probability*, and neither gates anything.

## Proposed fix

Add an explicit likelihood axis, scored separately from severity and confidence. Naming is the maintainers' call — I use **`materialization`**: *if this ships as written, how likely is it that this actually bites?* Score the probability that the triggering conditions occur, not the blast radius.

Shared bands, so the number means the same thing across surfaces:

| band | meaning |
|---|---|
| `>0.7` | bites on the normal path or the first realistic input |
| `0.3-0.7` | needs a specific but plausible condition — an error path, a concurrent write, a large input |
| `<0.3` | needs an unlikely conjunction, a scenario the design already rules out, or a scale this system will not reach |

Per surface, roughly:

- **`agents/code-reviewer.md`** — every Critical/Important finding carries a score and one sentence naming the triggering condition. Below `0.3` it moves to Suggestion regardless of instinct; Critical requires `≥ 0.5`. `REQUEST CHANGES` then requires at least one finding above the floor, so a review can't block a merge on a list of things that will never happen. *"If you cannot name the condition, you have not established the finding"* is the load-bearing sentence — it is also what stops the score from being reverse-engineered to justify a severity already chosen.
- **`skills/code-review-and-quality/SKILL.md`** — one row in the Step 4 prefix table, and a line under "Honesty in Review": quantifying probability is the same discipline as quantifying impact. This is a small edit; the section already has the right shape.
- **`skills/doubt-driven-development/SKILL.md`** — a fifth RECONCILE class between `valid + actionable` and `valid trade-off`: **valid but immaterial** — real, correctly understood, and its conditions will not occur. Document and do not re-loop. This is the class that lets Step 5 terminate honestly instead of grinding.
- **`agents/security-auditor.md` / `agents/web-performance-auditor.md`** — mostly making the existing instinct explicit and consistent with the shared bands, so `Low = "Theoretical risk"` becomes a scored judgment rather than a category the model can route around by picking `High`.

**Two thresholds, not one, if you want the strong version.** They do different jobs and conflating them is the easy mistake: a *floor* (~0.3) decides what reaches the author at all; a *gate* (~0.5) decides what is allowed to block. A finding between them is worth reporting and fixing, but not worth blocking on.

Whatever the mechanism, one property matters: **nothing filtered may be dropped silently.** A demoted finding keeps its score and its condition sentence so the author can overrule. A filter the author cannot see reads as "the reviewer found nothing".

## Prior art / why I'm confident this works

I shipped this in my own skill pack and it measurably cut waste in a Codex review loop: [pySilver/cc-tools@3ccbedd](https://github.com/pySilver/cc-tools/commit/3ccbedd). Two details from that implementation are worth stealing:

1. **An unknown score must read as maximum, not zero.** Absent or unparseable → treat as `1.0`. Otherwise a reviewer that forgets to score silently suppresses its own findings, and in a loop a broken scorer terminates it early. Fail toward "keep working".
2. **The scorer should not be the party with the inflation bias.** In my loop the reviewer self-scores, and then an independent pass re-scores *blind to the first number* and overrides it. Shown the original score, the second pass anchors to it. If a single agent scores its own findings, expect inflation and say so in the skill.

Happy to open a PR — for all five surfaces or a narrower slice, whichever you prefer. Wanted the shape agreed before writing the diff, per CONTRIBUTING's guidance on justifying the gap first.

贡献指南

打开贡献指南

评估

这个 Issue 还没有评估数据。

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。