feat: add design and nit severities

.claude/skills/setup-code-review/ecr-reference.md

@@ -88,7 +88,7 @@ defaults). JSONC: `//` and `/* */` comments and trailing commas allowed.
 | Field | Type | Default | Notes |
 |---|---|---|---|
 | `model` | string | `"openai/gpt-5.5"` | `provider/model`. Default for all agents + coordinator. Override per-agent in frontmatter; global override via `REVIEWER_MODEL` env (empty/whitespace = unset). |
-| `policy.includeSuggestions` | bool | `false` | If false, `suggestion`-severity findings dropped. This is the "severity floor". |
+| `policy.includeSuggestions` | bool | `false` | If false, `suggestion`- and `nit`-severity findings dropped. This is the "severity floor". |
 | `policy.maxFindings` | int>0 | unset | Cap total findings (most-severe-first). |
 | `chunk.maxChangedLines` | int>0 | `1000` | Split threshold on added+removed lines. Under it = one full-context pass. |
 | `chunk.maxFiles` | int>0 | `20` | Secondary per-chunk file guard. |
@@ -256,8 +256,10 @@ line. Do NOT repeat severity/category/JSON-format in agent files — those live
 - Prompt-injection defense (diff/PR content is untrusted data; embedded instructions = a `security`
   finding).
 - Severity definitions (canonical here only): `critical` = outage/data-loss/exploit/secret leak;
-  `warning` = measurable regression/concrete risk; `suggestion` = improvement only. Current policy:
-  emit only critical + warning.
+  `warning` = measurable regression/concrete risk; `design` = design problem with a concrete cost,
+  does not block; `suggestion` = improvement only; `nit` = rename or comment-level fix. Current
+  policy: emit only critical + warning + design. A `design` finding names a concrete cost in
+  place of a failure path.
 - Simplified Technical English rules for `title`/`rationale`/`suggestion` (one term per concept,
   ≤20-word sentences, active voice, plain words) — NOT applied to `evidence`/quoted code.
 - The output JSON contract.
@@ -266,8 +268,9 @@ line. Do NOT repeat severity/category/JSON-format in agent files — those live
 pro-tier). Does not re-review; it (1) dedupes same file+root-cause findings, (2) re-judges
 severity against shared defs — never downgrades for "temporary/fixture/WIP", keeps command
 injection + logged secrets critical, (3) decides via rubric, (4) summarizes in 1-3 sentences
-from kept findings only. Decision rubric (biased to approve): `approve` (clean/suggestions only) →
-`approve_with_comments` (warnings, no prod/security risk) → `request_changes` (≥1 critical or any
+from kept findings only. Decision rubric (biased to approve): `approve` (clean/suggestions and nits
+only) → `approve_with_comments` (warnings or design findings, no prod/security risk) →
+`request_changes` (≥1 critical or any
 secret leak). A lone warning is `approve_with_comments`. Output = single JSON object
 `{decision, findings, summary}`, `evidence` preserved unchanged.
 
@@ -280,7 +283,7 @@ Diffs only ever appear in the task message, wrapped in `BEGIN DIFF (untrusted)`
 **Finding/evidence contract** (canonical in `shared.md`, enforced downstream):
 ```json
 {"findings":[{
-  "severity":"critical|warning|suggestion",
+  "severity":"critical|warning|design|suggestion|nit",
   "category":"correctness|quality|security|secrets",
   "file":"path/relative/to/repo/root.ts",
   "line":142,                     // start line in NEW file, or null if not line-specific
@@ -290,8 +293,8 @@ Diffs only ever appear in the task message, wrapped in `BEGIN DIFF (untrusted)`
   "suggestion":"optional concrete fix, or omit"
 }]}
 ```
-Reviewers return ONLY this one fenced ```json block, no prose; empty = `{"findings": []}`. Policy
-currently forbids `suggestion` items. `evidence` must be exactly one contiguous verbatim in-file
+Reviewers return ONLY this one fenced ```json block, no prose; empty = `{"findings": []}`. The
+template prompts currently forbid `suggestion` and `nit` items. `evidence` must be exactly one contiguous verbatim in-file
 line (no `…`, no multi-line spans) — the verifier uses it to locate the finding; for
 structural/missing issues quote the single most relevant real line.
 
@@ -320,9 +323,9 @@ structural/missing issues quote the single most relevant real line.
   is dropped ONLY if the verifier explicitly refutes it (`verified:false`); a verify error fails
   open (finding kept). Implication: evidence must be a verbatim in-file substring; criticals must be
   defensible under a skeptical re-read.
-- **Severity/decision semantics**: `SEVERITIES=[critical,warning,suggestion]`;
+- **Severity/decision semantics**: `SEVERITIES=[critical,warning,design,suggestion,nit]`;
   `DECISIONS=[approve, approve_with_comments, request_changes]`. `request_changes` → exit 1, else 0.
-  `suggestion` dropped unless `policy.includeSuggestions`. 0 findings → `approve`; `request_changes`
+  `suggestion` and `nit` dropped unless `policy.includeSuggestions`. 0 findings → `approve`; `request_changes`
   with no surviving critical → softened to `approve_with_comments`. Any failed/timed-out pass caps
   the result at `approve_with_comments` (never a silent clean approve on incomplete coverage); if
   every pass fails, decision is `approve_with_comments` with `couldNotComplete:true` (render as

README.md

@@ -537,7 +537,7 @@ per-repo `noise.additionalIgnores`.
 ```jsonc
 {
   "model": "anthropic/claude-opus-5-5",       // default model for the specialists
-  "policy": { "includeSuggestions": false },  // suppress suggestion-severity findings
+  "policy": { "includeSuggestions": false },  // suppress suggestion and nit findings
   "chunk": { "maxChangedLines": 1000, "maxFiles": 20 },  // concurrency defaults: 6 (API key) / 3 (subscription)
   "noise": { "additionalIgnores": ["packages/*/build/**"] },
   "review": { "trigger": "all",               // which PRs `ecr ci` reviews: "all"

llp/0002-review-engine-pipeline.explainer.md

@@ -75,7 +75,7 @@ The pass gets an **elastic** time budget — whatever remains of the passes wind
 The degraded paths keep a failed run honest:
 
 - **Coordinator failure never discards findings.** If `coordinate()` throws, `fallbackConsolidation()` merges and dedupes the agents' findings by fingerprint and picks a conservative decision — `request_changes` on any critical, else `approve_with_comments`, never a clean approve when findings exist [observed] (`review.ts:800-809`, `978-1009`).
-- **`applyReviewPolicy` runs identically on both paths** — the coordinator's normal output and the local fallback both pass through the same policy (drop suggestions unless opted in, sort by severity, cap by `maxFindings`, downgrade `approve_with_comments`→`approve` when nothing remains), so enforcement can't diverge between the happy and degraded paths [observed] (`review.ts:792` and `998-1008`).
+- **`applyReviewPolicy` runs identically on both paths** — the coordinator's normal output and the local fallback both pass through the same policy (drop suggestions and nits unless opted in, sort by severity, cap by `maxFindings`, downgrade `approve_with_comments`→`approve` when nothing remains), so enforcement can't diverge between the happy and degraded paths [observed] (`review.ts:792` and `998-1008`).
 - **Any failed/timed-out pass forces the decision off a clean approve** — downgraded to `approve_with_comments` at minimum, independent of what the coordinator decided [observed] (`review.ts:810-815`).
 - **An all-failed run sets `couldNotComplete`.** When every pass fails or times out, the run is never rendered as a normal decision string; the flag overrides presentation so the comment header can't read "Ready for human review (with comments)" over a review that reviewed nothing [observed] (`review.ts:764-777`; commit `188f852`, euxy#8).
 

llp/0005-verification-fingerprints-rendering.explainer.md

@@ -18,7 +18,7 @@ The reason the fuzzy path exists at all is a reverted experiment. An earlier rul
 Two more grounding rules follow from the same caution:
 
 - **Criticals always get the skeptical double-check.** The LLM verify call runs when evidence is `absent` OR severity is `critical`, even a perfectly grounded critical (`verify.ts:98-107`, `verify.ts:137`). Grounded, non-critical findings take the fast path and are kept with no LLM call.
-- **Short evidence concludes `unknown`, never `absent`.** Below `MIN_EVIDENCE_LEN` (12 normalized chars) `matchEvidence` returns `unknown` (`verify.ts:55-59`), so trivially short text can never look like a grounding failure and trigger the escalate-or-drop path. `verify.test.ts:46-49` guards this. The verifier prompt itself is told to [judge the substance, not the wording][judge], so paraphrased or misquoted evidence cannot by itself cause a rejection (`prompts.ts:305-310`).
+- **Short evidence concludes `unknown`, never `absent`.** Below `MIN_EVIDENCE_LEN` (12 normalized chars) `matchEvidence` returns `unknown` (`verify.ts:55-59`), so trivially short text can never look like a grounding failure and trigger the escalate-or-drop path. `verify.test.ts:46-49` guards this. The verifier prompt itself is told to [judge the substance, not the wording][judge], so paraphrased or misquoted evidence cannot by itself cause a rejection (`prompts.ts:305-310`). A `design`, `suggestion` or `nit` finding claims a cost rather than a failure, so the verifier rejects it when that cost is not real, not because no failure can happen (`buildVerifierSystem`).
 
 **Invariant:** `matchEvidence` never returns `absent` for evidence under 12 normalized chars. Do not "simplify" it back to a plain exact-substring check — that is the reverted `33a970a` design.
 
@@ -79,7 +79,7 @@ The hash is state, not display: it lives only inside the base64 state marker and
 
 The invariant that makes this safe: **dismissed findings always survive in full in the embedded state, regardless of truncation** (`render.ts:449-459`). Truncation trims only the shown/kept set; the state blob still carries every dismissed finding so `/undismiss` never permanently loses a dismissal (`render-aggregate.test.ts:117-137`). An agent "simplifying" truncation to also drop dismissed entries would break `/undismiss`.
 
-The state blob is deliberately dual-shaped: it keeps a synthesized v1-shaped `review` field (a merge across scopes) beside the real per-scope `scopes` field, so older consumers expecting the pre-routing v1 shape keep parsing while v2 consumers read real per-scope data (`render.ts:293-300`, `render-aggregate.test.ts:88-103`). `renderSeveritySections` is the single shared display path for both the single-comment and aggregate renderers, with `SEVERITIES`/`SEVERITY_RANK` as the sole ordering source of truth, so the two renderers cannot drift.
+The state blob is deliberately dual-shaped: it keeps a synthesized v1-shaped `review` field (a merge across scopes) beside the real per-scope `scopes` field, so older consumers expecting the pre-routing v1 shape keep parsing while v2 consumers read real per-scope data (`render.ts:293-300`, `render-aggregate.test.ts:88-103`). `renderSeveritySections` is the single shared display path for both the single-comment and aggregate renderers, with `SEVERITIES`/`SEVERITY_RANK` as the sole ordering source of truth, so the two renderers cannot drift. Every render path reads a finding's severity through `knownSeverity`, because the embedded state is not schema-validated on read and a newer engine can store a severity this one does not know (`design` and `nit` crashed the grouping in older engines on `/dismiss` and on a partial `--scopes` re-render). An unknown value renders as a `warning`, and the stored value is kept so the newer engine reads it back unchanged. This only protects engines from this version on.
 
 [agents]: 0000-expo-code-review-cli.explainer.md
 [trust]: 0001-trust-model.principles.md

llp/0009-adoption-templates-and-ci-workflows.guide.md

@@ -49,7 +49,7 @@ The three workflows share a posture built entirely around one fact: a reviewer j
 
 **Agents are files; the filename is the id.** Every markdown file dropped into `agents/` becomes one reviewer, with the filename as its id — there is no separate roster list to maintain [observed: `templates/config.jsonc:7-11`]. `shared.md` (prepended to every agent and the coordinator) and `coordinator.md` are reserved filenames [observed: `templates/config.jsonc:9-11`]. The trap: renaming `security.md` to `secrets.md` does not rename an agent — it creates a *new* agent id with empty history and orphans `security` wherever it is referenced (e.g. `routing.jsonc`'s `defaults.enforceAgents: ["security"]`) [observed: `templates/config.jsonc:7-11` (filename is the agent id); `templates/routing.jsonc:13`].
 
-**Phase-1 default: suggestions off.** The scaffolded `config.jsonc` ships `policy.includeSuggestions: false` — a deliberate choice to surface only critical/warning findings and keep signal high, not because the schema can't do more [observed: `templates/config.jsonc:14-17`; restated in `templates/shared.md:66-67`].
+**Phase-1 default: suggestions off.** The scaffolded `config.jsonc` ships `policy.includeSuggestions: false` — a deliberate choice to surface only critical, warning and design findings and keep signal high, not because the schema can't do more [observed: `templates/config.jsonc:14-17`; restated in `templates/shared.md:66-67`]. The engine has five severities, `critical`, `warning`, `design`, `suggestion` and `nit`, and `includeSuggestions` gates `suggestion` and `nit` together. `design` is a should-fix level that does not block the PR, so the policy never drops it by severity, the same as `warning`. The template prompts ask for `critical`, `warning` and `design`, and let a `design` finding name a concrete cost in place of a failure path; an adopter turns on suggestions and nits in its own prompts [observed: `src/core/schema.ts` `SEVERITIES`; `src/core/review.ts` `applyReviewPolicy`].
 
 **Coordinator and security pin the pro tier on purpose.** The default agent model is `openai/gpt-5.5`, but `coordinator.md` and `agents/security.md` both pin `openai/gpt-5.5-pro` in their frontmatter [observed: `templates/config.jsonc:6`; `templates/coordinator.md:1-8`; `templates/agents/security.md:1-9`]. The rationale is stated in each file: the coordinator makes the final de-dupe/severity/decision call where "consolidation quality matters more here than the small serial-tail latency it adds," and security is "the highest-stakes agent," scoped alone to bound the extra latency/rate-limit cost [observed: `templates/coordinator.md:2-5`; `templates/agents/security.md:4-7`]. Downgrading either to save cost silently reverses that tradeoff, and no test catches it [inferred: no test asserts the coordinator/security model pins, so a cost-driven downgrade would go uncaught].
 
@@ -84,7 +84,7 @@ credential actually selected by trusted config. The default remains Anthropic an
 
 **Findings prose follows ASD-STE100 Simplified Technical English.** Title, rationale, and suggestion must use one-word-one-meaning, ≤20-word sentences, active voice, and plain words, so non-native-English engineers can read them — but `evidence` and any quoted code are copied verbatim and never rewritten, because evidence is used downstream to verify the finding [observed: `templates/shared.md:70-92`; commit `509da46`].
 
-**Confidence and impact are separate axes, and both are rendered above the evidence.** A finding carries `Confidence` (how sure the reviewer is that the problem is real) and `Impact if shipped` (what it costs if it is), joined by a literal `<br>` at the head of `rationale`, with the full reasoning folded into a `<details>` block below them [observed: `templates/shared.md:102-146`]. Conflating the two is the failure this prevents: a `Medium`-confidence secret leak and a `High`-confidence style nit are not comparable, and a single severity field cannot say so. `Low` confidence is not a rendering choice — it is a report threshold, and the coordinator drops those findings outright [observed: `templates/shared.md:119-120`; `templates/coordinator.md:28-36`]. The coordinator also folds any `suggestion` into a `**Suggested remediation:**` line inside `rationale` and omits the separate field, because the reporter appends `suggestion` *after* the rationale — so a rationale ending in `</details>` would otherwise leave the fix stranded below the collapsed block [observed: `templates/coordinator.md:28-36`; `src/core/render.ts:255-261`].
+**Confidence and impact are separate axes, and both are rendered above the evidence.** A finding carries `Confidence` (how sure the reviewer is that the problem is real) and `Impact if shipped` (what it costs if it is), joined by a literal `<br>` at the head of `rationale`, with the full reasoning folded into a `<details>` block below them [observed: `templates/shared.md:102-146`]. Conflating the two is the failure this prevents: a `Medium`-confidence secret leak and a `High`-confidence style nit are not comparable, and a single severity field cannot say so. `Low` confidence is not a rendering choice — it is a report threshold, and the coordinator drops those findings outright [observed: `templates/shared.md:119-120`; `templates/coordinator.md:28-36`]. A `design` finding has no failure path, so the rubric rates it by its cost: confidence is how directly the code shows the cost, impact is how large it is. Without that rule, every `design` finding would rate `Low` confidence and the coordinator would drop it [observed: `templates/shared.md` "A `design` finding"]. The coordinator also folds any `suggestion` into a `**Suggested remediation:**` line inside `rationale` and omits the separate field, because the reporter appends `suggestion` *after* the rationale — so a rationale ending in `</details>` would otherwise leave the fix stranded below the collapsed block [observed: `templates/coordinator.md:28-36`; `src/core/render.ts:255-261`].
 
 **Overall PR risk travels as an internal handoff finding, not a channel of its own.** The cross-cutting reviewer (or the security reviewer when a small PR skips the cross-cutting pass) emits one extra finding titled `__overall_pr_risk__` at `suggestion`/`quality` severity, whose `rationale` is a fixed `Risk: … Change shape: … Existing behavior affected: … What might break: … Blast radius and rollback: …` sequence [observed: `templates/shared.md:148-187`]. The coordinator reads it, writes the summary opening `**Overall PR risk: Low|Medium|High.**`, and then removes it from `findings` [observed: `templates/coordinator.md:38-42,44-50,79-80`]. Reusing the ordinary finding channel is what makes this need no schema and no transport — but it is a *prompt-authored* convention, so the strip cannot be prompt-only. `applyReviewPolicy` filters the handoff by exact title **unconditionally, before** the severity filter, so it survives neither a coordinator that forgets to remove it nor an adopter who sets `policy.includeSuggestions: true` [observed: `src/core/review.ts:966-978`; `src/core/schema.ts:68-85` (`OVERALL_PR_RISK_TITLE` / `isOverallRiskHandoff`)]. Ordering is the load-bearing part: the handoff is `suggestion`-severity, so a strip placed *after* the severity filter would be dead code under the default config and would silently publish the handoff under `includeSuggestions: true` — the one configuration where it is not already covered [observed: `src/__tests__/review-internals.test.ts` "strips the `__overall_pr_risk__` handoff even with includeSuggestions"]. `fallbackConsolidation` inherits the strip because it routes through the same function [observed: `src/core/review.ts:1017`]. The rules originated in the euxy adopter repo (commits `30e6117`, `4f1e0f8`) and were folded back here.
 

src/__tests__/inline-comments.test.ts

@@ -73,7 +73,7 @@ test("renderInlineCommentBody: id token, severity, rationale, suggestion, and no
     "deadbeef11",
   );
   expect(body).toContain("`id:deadbeef11`");
-  expect(body).toContain("🟡 Warning");
+  expect(body).toContain("⚠️ Warning");
   expect(body).toContain("**Suggestion:** cache it");
   // Untrusted text can never smuggle a raw HTML comment opener into our body.
   expect(body).toContain("<!-- evil -->");