[tsapeta]

Why

#86 makes the severity scale five ordered levels: critical, warning, design, suggestion, nit. The boolean includeSuggestions can only gate a fixed set of them, which fits an ordered scale badly. The README already called the setting the "severity floor", so policy.minSeverity replaces it.

How

policy.minSeverity takes one of the five severities, and applyReviewPolicy drops every finding below it by comparing SEVERITY_RANK. It defaults to "design", which keeps exactly what includeSuggestions: false keeps in #86, so default reviews don't change.

The config schema is a plain z.object, which strips unknown keys, so removing includeSuggestions outright would silently drop the suggestions of a repo that sets it (expo/expo sets includeSuggestions: true). So it still loads: true maps to "nit" and false to "design", and the loader adds a setup warning that names the replacement. If both keys are set, minSeverity wins and the warning says to remove includeSuggestions. Setup warnings show as setup notes in the PR comment and in ecr doctor.

This repo's own reviewer setup moves to minSeverity: "design", and its shared.md and coordinator.md get the same design rules as the templates in #86, so its self-reviews start reporting design findings. It reviews itself from source at the PR's base commit, so these prompts never run on an engine without the design level.

This is stacked on #86, which adds the design and nit levels.

Test Plan

bun test (800 pass), bun run typecheck, bun run lint, bun run fmt:check and bun run llp:check all pass. The new config loader tests fail before the change and pass after: the minSeverity floor and its default, and the includeSuggestions alias with its warning (three cases). The policy test now checks three floors. I also ran ecr doctor from source on a config with includeSuggestions: true, and it prints the deprecation warning with the fix. On this repo's own setup, ecr ref-check, ecr verify-config and ecr doctor pass with no setup warnings. I haven't run it against a live PR yet.

[github-actions]

🤖 AI code review

Decision: Ready for human review (with comments)

Overall PR risk: Medium. This PR replaces the boolean includeSuggestions setting with an ordered minSeverity floor (default design). The change modifies config loading and the policy step for every review. When the floor drops all findings, the decision changes from request_changes to approve. A config that lowers minSeverity to warning or critical drops findings that previously blocked the PR. The deprecation mapping preserves existing configs. One issue: this repo's shared.md omits the design confidence definition from the template, which will cause design findings to be dropped unreliably.

🔗 Review setup:

• This PR changes code the reviewer prompts cite (src, src/tests, src/config/load.ts, src/config/schema.ts, templates, and 1 more). Check that the guidance quoting it is still correct.

⚠️ Warning (1)

• Repo shared.md asks for design findings but omits the template's design confidence rule — .expo-agents/code-review/shared.md:124 (quality) · id:ae47861897b3
  Confidence: High — templates/shared.md:131-134 has the rule, and .expo-agents/code-review/shared.md does not.
  Impact if shipped: Medium — this repo's own reviewers can rate every design finding Low confidence and drop it.
  Suggested remediation: Add the templates/shared.md:131-134 bullet after line 124 of .expo-agents/code-review/shared.md: "- A design finding has a concrete cost instead of a failure path. Its Confidence is how directly the code shows that cost, and its Impact if shipped is how large the cost is. A design cost that is only Low is a suggestion."

  Evidence and reasoning

  This PR changes .expo-agents/code-review/shared.md and coordinator.md so that they request and keep design findings. The prompts now match the template text for the severity list, the scope rule, and the output shape.

  The template sibling templates/shared.md:131-134 also has a bullet in "Finding confidence and shipping impact". That bullet says: "A design finding has a concrete cost instead of a failure path. Its Confidence is how directly the code shows that cost...". The repo copy ends that section at line 124 without the bullet.

  Without that rule, every design finding would rate Low confidence and the coordinator would drop it. In the repo copy, High and Medium confidence are still defined only by a traced failure or exploit. Low findings must not be reported. So the new minSeverity: "design" floor in .expo-agents/code-review/config.jsonc can keep design findings, but this repo's reviewers will not produce them reliably.

---

This review is advisory — it never blocks a merge and never auto-approves.

[brentvatne]

Review notes from a multi-model review (Codex, Grok and Claude, each finding then re-checked against the code). Fixes are in 295f579, pushed to this branch.

1. "Request changes" with zero findings (holds, reproduced). applyReviewPolicy only softened approve_with_comments when nothing remained. With minSeverity: "critical" and a coordinator request_changes resting on a warning (the rubric blocks on any credential leak, whatever its severity), the floor drops every finding and the comment reads "Request changes" over "No findings.", exits 1 locally, and in a monorepo spreads to the whole PR through worstDecision. Every later step that re-derives the decision sits behind findings.length > 0.

• Fix: approve whenever no findings remain, the same rule as decisionAfterVerification. A request_changes with visible non-critical findings is deliberately left alone.

2. ecr doctor fails on the deprecated key (holds, reproduced). The deprecation rode in setupWarnings, which doctor prints with line(false, …), so every repo still setting includeSuggestions (expo/expo, and everything scaffolded from the old template) exits 1 although its reviews work.

• Fix: a separate LoadedConfig.deprecations. Reviews still show it as a setup note, but doctor prints a ⚠ and exits 0. setupWarnings goes back to frontmatter-only, which makes the comments and LLP 0006 accurate again.

3. Low. Added a test that a scope config's includeSuggestions sets that scope's floor. The "scope warning names the root file" claim was overstated: the label comes from setupDirLabel, which predates this PR, and doctor prefixes the scope name.

Not changed: when the floor drops findings, the coordinator summary can still mention them. That already happened with dropped suggestions.

Checks: typecheck, bun test, lint, format and llp:check pass, on their own and with #86's fixes merged in.