Review Severity
Every reviewer tiers findings on one model, so a finding lands the same tier whoever
raises it. A tier is earned by a named consequence plus a reachable path -- not by
unease, vividness, or how easy it is to fix.
The spine
Severity = consequence-if-shipped = impact x reachability. Confidence is not part
of it (separate gate, below).
- Impact = KIND (corruption / breach / crash / wrong-result / disclosure) x MAGNITUDE
(damage per occurrence) x TIME-HORIZON (irreversibility is just long-horizon impact).
- Reachability = P(path reached) x P(trigger), classed live / edge / dead. Judge it
at the trust boundary -- a public/exported API or persisted state is a live entry point
even with no in-repo caller; a missing or weakened control on a boundary (authz,
rate-limit, crypto strength, secure default) defaults live, its impact being the breach
it fails to prevent. And at intended-completion for a diff -- a named wiring/flag/call
path is pending-live; only provably unwired code is dead. "I couldn't find the caller"
is not dead.
Tiers (directives)
- Critical -- never ship / regenerate. On a reachable path the code misbehaves or is
unsafe above the magnitude floor: data loss/corruption (including silent), security
breach, crash/hang/resource-exhaustion, won't build-or-run, broken reference, or a wrong
result users/callers/downstream act on ("behaves right" means correct output, not merely
"runs"). Also Critical when severe damage is confined to an edge but irreversible --
bounded to three kinds only: (i) silent corruption/loss, (ii) external-contract lock-in
(published API/schema/event consumers depend on), (iii) irreversible disclosure (secret/
PII/key that can't be un-leaked). Cost-to-fix never demotes.
- Important -- avoid; fix by default. Deferral is a conscious, logged, recoverable choice
(owner + reason), never silent. Two doors, tagged with a
{landmine | debt} sub-label:
maintainability-debt -- correct on reachable paths today but a compounding cost a future
change must pay (missing tests on load-bearing logic, fragile coupling, drift-prone
duplication, a misleading contract); recoverable-latent-landmine -- severe impact whose
only reachable path is a genuine recoverable edge (rare input, flag-gated with a
named-but-unwired path, an error branch that degrades). Irreversible impact rounds up to
Critical, never sits here.
- Minor -- optional polish, safe to leave. Expected damage ~0 even when reached:
cosmetic/preference, comments restating code, micro-optimizations with no measured effect,
debug noise, a non-exploitable info leak. A severe defect on demonstrably-dead code
(proven no caller, not exported, no named flag path) is Minor -- latent-flagged so it
re-tiers the instant a path is wired.
Magnitude floor (on Critical)
Below the floor, no live path makes a finding Critical. It clears only if a user, caller, or
downstream would make a materially different decision, or money/records/persisted-state/
credentials change. A mis-rounded non-critical stat, a wrong tooltip, a non-exploitable info
leak stay low even on a live path. (Security: attacker-reachability alone isn't Critical -- a
generic stack trace or version banner stays low; credential/PII exposure, auth bypass, and
injection clear the floor.)
Verdict
A review's verdict is its highest tier: BLOCK if any Critical, WARNING if any
Important but no Critical, APPROVE if only Minor or none.
Boundary tests
- Critical <-> Important -- does material-magnitude damage land on a reachable path (live,
attacker-edge, rare-but-guaranteed, or irreversible-on-edge)? -> Critical. Correct on
reachable paths today, or severe damage confined to a recoverable edge? -> Important.
- Important <-> Minor -- is there a named payer of a named accruing cost (drift, untested
load-bearing logic, a contract that misleads callers), or material impairment of an
identifiable user class? -> Important. Pure taste, no reachable failure, or demonstrably
dead? -> Minor.
Tie-break
On any boundary, default to the lower tier unless you can name the concrete consequence
and the reachable path (Critical), or the named payer and the accruing cost (Important).
Three counterweights keep default-down from becoming downward drift:
- Anti-demotion -- public/exported API, persisted state, attacker-controlled sinks, and a
missing control on a boundary default live; they can't be waved down by "found no
caller." Irreversible impact resists demotion.
- Anti-aggregation (symmetric) -- a pile of nits sums to at most one Important, never a
Critical; a pile of one Important-class defect sums to one systemic Important. Exception
both ways: repetition that is one systemic defect at many sites -> tier the underlying bug.
- Magnitude floor -- as above.
Confidence and cost-to-fix never enter the tie-break.
Confidence -- a separate report gate
Confidence is never a tier input; a suspected-but-severe finding keeps its high tier. The
gate only decides whether to surface it: report if >80% sure; for security, lower the floor to
~50%. Log-not-drop suspected criticals (surface as a flagged question) -- don't silently
discard a genuine-but-uncertain severe issue.
The machine-readable record is two orthogonal fields: severity (an ordered enum) and
status (confirmed | suspected). A CI consumer thresholds severity on confirmed
findings (block if >= Critical) and routes suspected to a non-blocking flagged-question
channel. Folding confidence into severity -- or emitting severity with no status field --
collapses the gate and is forbidden.
Knowledge-cutoff blind spot
A finding that hinges on a post-cutoff fact -- a version you think doesn't exist or is newer
than you know (Sphinx 9.1.0 when you last knew 9.0.0), a missing API, a deprecation, any
"latest is X" claim -- is a blind spot, never confirmed from memory. Presume a
newer-than-known version is real until a live source says otherwise: verify against a registry
(pip index versions <pkg>), changelog, or docs before asserting; if you can't, mark it
suspected.
No "Optimization" tier
"Optimize" maps to no directive a generator can act on (block / fix-or-log / polish). Every
optimization item is Important (a real accruing cost -- drift-prone duplication, N+1 on a
hot path, a monolith the team churns) or Minor (marginal -- cosmetic simplification, a
micro-opt, an O(n^2) loop over a provably-and-enforced tiny bound), chosen by the
Important<->Minor test.
Extensibility (seams, not machinery)
- Severity is a named ordinal; CI thresholds on named levels, not a dense index. A new
tier appended at an end is cheap. A mid-order insert -- e.g. splitting Important's
landmine vs debt into its own tier -- forces a one-time re-tier; it's deferred, and the
{landmine|debt} sub-label pre-shapes that seam. Don't build it before a real dispute.
- A new reviewer category (security / performance / infra) binds to the impact/reachability
inputs, never to new tiers. A "security is always Critical" policy is a post-computation
max() over the enum, not a branch inside the product.
- Per-path / per-project strictness is a post-pass over the pure tier. Precedence: path
input-overrides (edge->live on a hot path) apply first, then output caps/floors compose via
max/min. Keep the base computation pure. Hardcode the first override inline; generalize on
the second.
Calibration -- illustrative anchors, not a lookup table
Tier a finding by running the spine, not by matching this list. These pin the boundaries:
| Finding |
Tier |
Reason |
| Injection sink on a live path (SQL/XSS) |
Critical |
breach, live, clears the floor |
| Hardcoded secret in source |
Critical |
live disclosure; irreversible |
| Inverted validation guard on a live path |
Critical |
unsafe input admitted |
| Off-by-one that returns wrong/missing records users act on |
Critical |
wrong output above the floor |
| Broken import (won't run) |
Critical |
cost-to-fix never demotes |
| Data race / silent-loss on a live path (unsync shared state, un-awaited side effect) |
Critical |
silent corruption, irreversible |
| Stub / canned return trusted as real on a live path |
Critical |
wrong output callers act on |
except: pass around a write the caller relies on |
Critical |
silent loss (Important if the write is recoverable) |
Blanket # type: ignore / suppression |
Important |
masking debt; any bug it hides tiers on its own |
| Wrong-layer / misleading name / drift-prone duplication |
Important |
debt a future change pays (floor -- a skipped invariant tiers separately) |
| Missing test on load-bearing logic |
Important |
named accruing cost |
| Missing alt text on a content image |
Important |
impairs an identifiable user class |
Debug print left behind |
Minor |
noise below the floor (Critical if it emits secrets) |
| Dead / commented-out code |
Minor |
zero behavioral consequence; git owns it |
| Non-conventional casing, comment restating code, unanchored TODO on an incomplete branch |
Minor |
preference / expected context |
| O(n^2) over a provably-bounded tiny list |
Minor |
reachability high, impact ~0 |
1---2name: review-severity3description: One severity model for code review -- Critical/Important/Minor by consequence-if-shipped (impact x reachability), with confidence as a separate report gate.4---56# Review Severity78Every reviewer tiers findings on one model, so a finding lands the same tier whoever9raises it. A tier is **earned** by a named consequence plus a reachable path -- not by10unease, vividness, or how easy it is to fix.1112## The spine1314**Severity = consequence-if-shipped = impact x reachability.** Confidence is *not* part15of it (separate gate, below).1617- **Impact** = KIND (corruption / breach / crash / wrong-result / disclosure) x MAGNITUDE18 (damage per occurrence) x TIME-HORIZON (irreversibility is just long-horizon impact).19- **Reachability** = P(path reached) x P(trigger), classed **live / edge / dead**. Judge it20 at the **trust boundary** -- a public/exported API or persisted state is a live entry point21 even with no in-repo caller; a *missing or weakened* control on a boundary (authz,22 rate-limit, crypto strength, secure default) defaults **live**, its impact being the breach23 it fails to prevent. And at **intended-completion** for a diff -- a named wiring/flag/call24 path is *pending-live*; only *provably* unwired code is dead. "I couldn't find the caller"25 is not dead.2627## Tiers (directives)2829- **Critical -- never ship / regenerate.** On a reachable path the code misbehaves or is30 unsafe above the magnitude floor: data loss/corruption (including *silent*), security31 breach, crash/hang/resource-exhaustion, won't build-or-run, broken reference, or a wrong32 result users/callers/downstream act on ("behaves right" means correct *output*, not merely33 "runs"). Also Critical when severe damage is confined to an edge but **irreversible** --34 bounded to three kinds only: (i) silent corruption/loss, (ii) external-contract lock-in35 (published API/schema/event consumers depend on), (iii) irreversible disclosure (secret/36 PII/key that can't be un-leaked). Cost-to-fix never demotes.37- **Important -- avoid; fix by default.** Deferral is a conscious, logged, recoverable choice38 (owner + reason), never silent. Two doors, tagged with a `{landmine | debt}` sub-label:39 **maintainability-debt** -- correct on reachable paths today but a compounding cost a future40 change must pay (missing tests on load-bearing logic, fragile coupling, drift-prone41 duplication, a misleading contract); **recoverable-latent-landmine** -- severe impact whose42 only reachable path is a genuine *recoverable* edge (rare input, flag-gated with a43 named-but-unwired path, an error branch that degrades). Irreversible impact rounds up to44 Critical, never sits here.45- **Minor -- optional polish, safe to leave.** Expected damage ~0 even when reached:46 cosmetic/preference, comments restating code, micro-optimizations with no measured effect,47 debug noise, a non-exploitable info leak. A *severe* defect on **demonstrably-dead** code48 (proven no caller, not exported, no named flag path) is Minor -- latent-flagged so it49 re-tiers the instant a path is wired.5051### Magnitude floor (on Critical)5253Below the floor, no live path makes a finding Critical. It clears only if a user, caller, or54downstream would make a **materially different decision**, or money/records/persisted-state/55credentials change. A mis-rounded non-critical stat, a wrong tooltip, a non-exploitable info56leak stay low even on a live path. (Security: attacker-reachability alone isn't Critical -- a57generic stack trace or version banner stays low; credential/PII exposure, auth bypass, and58injection clear the floor.)5960## Verdict6162A review's verdict is its highest tier: **BLOCK** if any Critical, **WARNING** if any63Important but no Critical, **APPROVE** if only Minor or none.6465## Boundary tests6667- **Critical <-> Important** -- does material-magnitude damage land on a *reachable* path (live,68 attacker-edge, rare-but-guaranteed, or irreversible-on-edge)? -> **Critical**. Correct on69 reachable paths today, or severe damage confined to a *recoverable* edge? -> **Important**.70- **Important <-> Minor** -- is there a named payer of a named accruing cost (drift, untested71 load-bearing logic, a contract that misleads callers), or material impairment of an72 identifiable user class? -> **Important**. Pure taste, no reachable failure, or demonstrably73 dead? -> **Minor**.7475## Tie-break7677On any boundary, default to the **lower** tier unless you can name the concrete consequence78*and* the reachable path (Critical), or the named payer *and* the accruing cost (Important).79Three counterweights keep default-down from becoming downward drift:80811. **Anti-demotion** -- public/exported API, persisted state, attacker-controlled sinks, and a82 missing control on a boundary default **live**; they can't be waved down by "found no83 caller." Irreversible impact resists demotion.842. **Anti-aggregation (symmetric)** -- a pile of nits sums to at most one Important, never a85 Critical; a pile of one Important-class defect sums to one systemic Important. Exception86 both ways: repetition that is *one* systemic defect at many sites -> tier the underlying bug.873. **Magnitude floor** -- as above.8889Confidence and cost-to-fix never enter the tie-break.9091## Confidence -- a separate report gate9293Confidence is never a tier input; a suspected-but-severe finding **keeps** its high tier. The94gate only decides whether to surface it: report if >80% sure; for security, lower the floor to95~50%. **Log-not-drop** suspected criticals (surface as a flagged question) -- don't silently96discard a genuine-but-uncertain severe issue.9798The machine-readable record is **two orthogonal fields**: `severity` (an ordered enum) and99`status` (`confirmed` | `suspected`). A CI consumer thresholds *severity* on confirmed100findings (block if >= Critical) and routes suspected to a non-blocking flagged-question101channel. Folding confidence into severity -- or emitting severity with no status field --102collapses the gate and is forbidden.103104### Knowledge-cutoff blind spot105106A finding that hinges on a post-cutoff fact -- a version you think doesn't exist or is newer107than you know (Sphinx `9.1.0` when you last knew `9.0.0`), a missing API, a deprecation, any108"latest is X" claim -- is a blind spot, never *confirmed* from memory. Presume a109newer-than-known version is real until a live source says otherwise: verify against a registry110(`pip index versions <pkg>`), changelog, or docs before asserting; if you can't, mark it111`suspected`.112113## No "Optimization" tier114115"Optimize" maps to no directive a generator can act on (block / fix-or-log / polish). Every116optimization item is **Important** (a real accruing cost -- drift-prone duplication, N+1 on a117hot path, a monolith the team churns) or **Minor** (marginal -- cosmetic simplification, a118micro-opt, an O(n^2) loop over a provably-and-enforced tiny bound), chosen by the119Important<->Minor test.120121## Extensibility (seams, not machinery)122123- Severity is a **named ordinal**; CI thresholds on named levels, not a dense index. A new124 tier appended at an *end* is cheap. A *mid-order* insert -- e.g. splitting Important's125 landmine vs debt into its own tier -- forces a one-time re-tier; it's deferred, and the126 `{landmine|debt}` sub-label pre-shapes that seam. Don't build it before a real dispute.127- A new reviewer category (security / performance / infra) binds to the impact/reachability128 *inputs*, never to new tiers. A "security is always Critical" policy is a post-computation129 `max()` over the enum, not a branch inside the product.130- Per-path / per-project strictness is a post-pass over the pure tier. Precedence: path131 input-overrides (edge->live on a hot path) apply first, then output caps/floors compose via132 max/min. Keep the base computation pure. Hardcode the first override inline; generalize on133 the second.134135## Calibration -- illustrative anchors, not a lookup table136137Tier a finding by running the spine, not by matching this list. These pin the boundaries:138139| Finding | Tier | Reason |140|---|---|---|141| Injection sink on a live path (SQL/XSS) | Critical | breach, live, clears the floor |142| Hardcoded secret in source | Critical | live disclosure; irreversible |143| Inverted validation guard on a live path | Critical | unsafe input admitted |144| Off-by-one that returns wrong/missing records users act on | Critical | wrong output above the floor |145| Broken import (won't run) | Critical | cost-to-fix never demotes |146| Data race / silent-loss on a live path (unsync shared state, un-awaited side effect) | Critical | silent corruption, irreversible |147| Stub / canned return trusted as real on a live path | Critical | wrong output callers act on |148| `except: pass` around a write the caller relies on | Critical | silent loss (Important if the write is recoverable) |149| Blanket `# type: ignore` / suppression | Important | masking debt; any bug it hides tiers on its own |150| Wrong-layer / misleading name / drift-prone duplication | Important | debt a future change pays (floor -- a skipped invariant tiers separately) |151| Missing test on load-bearing logic | Important | named accruing cost |152| Missing alt text on a content image | Important | impairs an identifiable user class |153| Debug `print` left behind | Minor | noise below the floor (Critical if it emits secrets) |154| Dead / commented-out code | Minor | zero behavioral consequence; git owns it |155| Non-conventional casing, comment restating code, unanchored TODO on an incomplete branch | Minor | preference / expected context |156| O(n^2) over a provably-bounded tiny list | Minor | reachability high, impact ~0 |