diff --git a/review/SKILL.md b/review/SKILL.md index 740795aa..32c597a3 100644 --- a/review/SKILL.md +++ b/review/SKILL.md @@ -121,9 +121,11 @@ Run `git diff origin/main` to get the full diff. This includes both committed an Apply the checklist against the diff in two passes: -1. **Pass 1 (CRITICAL):** SQL & Data Safety, LLM Output Trust Boundary +1. **Pass 1 (CRITICAL):** SQL & Data Safety, Race Conditions & Concurrency, LLM Output Trust Boundary, Enum & Value Completeness 2. **Pass 2 (INFORMATIONAL):** Conditional Side Effects, Magic Numbers & String Coupling, Dead Code & Consistency, LLM Prompt Issues, Test Gaps, View/Frontend +**Enum & Value Completeness requires reading code OUTSIDE the diff.** When the diff introduces a new enum value, status, tier, or type constant, use Grep to find all files that reference sibling values, then Read those files to check if the new value is handled. This is the one category where within-diff review is insufficient. + Follow the output format specified in the checklist. Respect the suppressions — do NOT flag items listed in the "DO NOT flag" section. --- diff --git a/review/SKILL.md.tmpl b/review/SKILL.md.tmpl index 68d8a1d1..124a5393 100644 --- a/review/SKILL.md.tmpl +++ b/review/SKILL.md.tmpl @@ -64,9 +64,11 @@ Run `git diff origin/main` to get the full diff. This includes both committed an Apply the checklist against the diff in two passes: -1. **Pass 1 (CRITICAL):** SQL & Data Safety, LLM Output Trust Boundary +1. **Pass 1 (CRITICAL):** SQL & Data Safety, Race Conditions & Concurrency, LLM Output Trust Boundary, Enum & Value Completeness 2. **Pass 2 (INFORMATIONAL):** Conditional Side Effects, Magic Numbers & String Coupling, Dead Code & Consistency, LLM Prompt Issues, Test Gaps, View/Frontend +**Enum & Value Completeness requires reading code OUTSIDE the diff.** When the diff introduces a new enum value, status, tier, or type constant, use Grep to find all files that reference sibling values, then Read those files to check if the new value is handled. This is the one category where within-diff review is insufficient. + Follow the output format specified in the checklist. Respect the suppressions — do NOT flag items listed in the "DO NOT flag" section. --- diff --git a/review/checklist.md b/review/checklist.md index e3218900..6052c33b 100644 --- a/review/checklist.md +++ b/review/checklist.md @@ -48,6 +48,13 @@ Be terse. For each issue: one line describing the problem, one line with the fix - LLM-generated values (emails, URLs, names) written to DB or passed to mailers without format validation. Add lightweight guards (`EMAIL_REGEXP`, `URI.parse`, `.strip`) before persisting. - Structured tool output (arrays, hashes) accepted without type/shape checks before database writes. +#### Enum & Value Completeness +When the diff introduces a new enum value, status string, tier name, or type constant: +- **Trace it through every consumer.** Read (don't just grep — READ) each file that switches on, filters by, or displays that value. If any consumer doesn't handle the new value, flag it. Common miss: adding a value to the frontend dropdown but the backend model/compute method doesn't persist it. +- **Check allowlists/filter arrays.** Search for arrays or `%w[]` lists containing sibling values (e.g., if adding "revise" to tiers, find every `%w[quick lfg mega]` and verify "revise" is included where needed). +- **Check `case`/`if-elsif` chains.** If existing code branches on the enum, does the new value fall through to a wrong default? +To do this: use Grep to find all references to the sibling values (e.g., grep for "lfg" or "mega" to find all tier consumers). Read each match. This step requires reading code OUTSIDE the diff. + ### Pass 2 — INFORMATIONAL #### Conditional Side Effects @@ -101,8 +108,8 @@ Be terse. For each issue: one line describing the problem, one line with the fix CRITICAL (blocks /ship): INFORMATIONAL (in PR body): ├─ SQL & Data Safety ├─ Conditional Side Effects ├─ Race Conditions & Concurrency ├─ Magic Numbers & String Coupling -└─ LLM Output Trust Boundary ├─ Dead Code & Consistency - ├─ LLM Prompt Issues +├─ LLM Output Trust Boundary ├─ Dead Code & Consistency +└─ Enum & Value Completeness ├─ LLM Prompt Issues ├─ Test Gaps ├─ Crypto & Entropy ├─ Time Window Safety