diff --git a/plan-ceo-review/SKILL.md b/plan-ceo-review/SKILL.md index b15ff6d2..50af9075 100644 --- a/plan-ceo-review/SKILL.md +++ b/plan-ceo-review/SKILL.md @@ -212,8 +212,8 @@ Run the following commands: git log --oneline -30 # Recent history git diff --stat # What's already changed git stash list # Any stashed work -grep -r "TODO\|FIXME\|HACK\|XXX" --include="*.rb" --include="*.js" -l -find . -name "*.rb" -newer Gemfile.lock | head -20 # Recently touched files +grep -r "TODO\|FIXME\|HACK\|XXX" -l --exclude-dir=node_modules --exclude-dir=vendor --exclude-dir=.git | head -30 +git log --since=30.days --name-only --format="" | sort | uniq -c | sort -rn | head -20 # Recently touched files ``` Then read CLAUDE.md, TODOS.md, and any existing architecture docs. When reading TODOS.md, specifically: * Note any TODOs this plan touches, blocks, or unlocks @@ -397,24 +397,24 @@ For every new method, service, or codepath that can fail, fill in this table: ``` METHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS -------------------------|-----------------------------|----------------- - ExampleService#call | API timeout | Faraday::TimeoutError + ExampleService.call | API timeout | TimeoutError | API returns 429 | RateLimitError - | API returns malformed JSON | JSON::ParserError - | DB connection pool exhausted| ActiveRecord::ConnectionTimeoutError - | Record not found | ActiveRecord::RecordNotFound + | API returns malformed JSON | JSONParseError + | DB connection pool exhausted| ConnectionPoolExhausted + | Record not found | RecordNotFound -------------------------|-----------------------------|----------------- EXCEPTION CLASS | RESCUED? | RESCUE ACTION | USER SEES -----------------------------|-----------|------------------------|------------------ - Faraday::TimeoutError | Y | Retry 2x, then raise | "Service temporarily unavailable" + TimeoutError | Y | Retry 2x, then raise | "Service temporarily unavailable" RateLimitError | Y | Backoff + retry | Nothing (transparent) - JSON::ParserError | N ← GAP | — | 500 error ← BAD - ConnectionTimeoutError | N ← GAP | — | 500 error ← BAD - ActiveRecord::RecordNotFound | Y | Return nil, log warning | "Not found" message + JSONParseError | N ← GAP | — | 500 error ← BAD + ConnectionPoolExhausted | N ← GAP | — | 500 error ← BAD + RecordNotFound | Y | Return nil, log warning | "Not found" message ``` Rules for this section: -* `rescue StandardError` is ALWAYS a smell. Name the specific exceptions. -* `rescue => e` with only `Rails.logger.error(e.message)` is insufficient. Log the full context: what was being attempted, with what arguments, for what user/request. +* Catching all errors generically (`rescue StandardError`, `catch (Exception e)`, `except Exception`) is ALWAYS a smell. Name the specific exceptions. +* Logging only the error message is insufficient. Log the full context: what was being attempted, with what arguments, for what user/request. * Every rescued error must either: retry with backoff, degrade gracefully with a user-visible message, or re-raise with added context. "Swallow and continue" is almost never acceptable. * For each GAP (unrescued error that should be rescued): specify the rescue action and what the user should see. * For LLM/AI service calls specifically: what happens when the response is malformed? When it's empty? When it hallucinates invalid JSON? When the model returns a refusal? Each of these is a distinct failure mode. diff --git a/plan-ceo-review/SKILL.md.tmpl b/plan-ceo-review/SKILL.md.tmpl index f4b8b903..adf7b926 100644 --- a/plan-ceo-review/SKILL.md.tmpl +++ b/plan-ceo-review/SKILL.md.tmpl @@ -91,8 +91,8 @@ Run the following commands: git log --oneline -30 # Recent history git diff --stat # What's already changed git stash list # Any stashed work -grep -r "TODO\|FIXME\|HACK\|XXX" --include="*.rb" --include="*.js" -l -find . -name "*.rb" -newer Gemfile.lock | head -20 # Recently touched files +grep -r "TODO\|FIXME\|HACK\|XXX" -l --exclude-dir=node_modules --exclude-dir=vendor --exclude-dir=.git | head -30 +git log --since=30.days --name-only --format="" | sort | uniq -c | sort -rn | head -20 # Recently touched files ``` Then read CLAUDE.md, TODOS.md, and any existing architecture docs. When reading TODOS.md, specifically: * Note any TODOs this plan touches, blocks, or unlocks @@ -276,24 +276,24 @@ For every new method, service, or codepath that can fail, fill in this table: ``` METHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS -------------------------|-----------------------------|----------------- - ExampleService#call | API timeout | Faraday::TimeoutError + ExampleService.call | API timeout | TimeoutError | API returns 429 | RateLimitError - | API returns malformed JSON | JSON::ParserError - | DB connection pool exhausted| ActiveRecord::ConnectionTimeoutError - | Record not found | ActiveRecord::RecordNotFound + | API returns malformed JSON | JSONParseError + | DB connection pool exhausted| ConnectionPoolExhausted + | Record not found | RecordNotFound -------------------------|-----------------------------|----------------- EXCEPTION CLASS | RESCUED? | RESCUE ACTION | USER SEES -----------------------------|-----------|------------------------|------------------ - Faraday::TimeoutError | Y | Retry 2x, then raise | "Service temporarily unavailable" + TimeoutError | Y | Retry 2x, then raise | "Service temporarily unavailable" RateLimitError | Y | Backoff + retry | Nothing (transparent) - JSON::ParserError | N ← GAP | — | 500 error ← BAD - ConnectionTimeoutError | N ← GAP | — | 500 error ← BAD - ActiveRecord::RecordNotFound | Y | Return nil, log warning | "Not found" message + JSONParseError | N ← GAP | — | 500 error ← BAD + ConnectionPoolExhausted | N ← GAP | — | 500 error ← BAD + RecordNotFound | Y | Return nil, log warning | "Not found" message ``` Rules for this section: -* `rescue StandardError` is ALWAYS a smell. Name the specific exceptions. -* `rescue => e` with only `Rails.logger.error(e.message)` is insufficient. Log the full context: what was being attempted, with what arguments, for what user/request. +* Catching all errors generically (`rescue StandardError`, `catch (Exception e)`, `except Exception`) is ALWAYS a smell. Name the specific exceptions. +* Logging only the error message is insufficient. Log the full context: what was being attempted, with what arguments, for what user/request. * Every rescued error must either: retry with backoff, degrade gracefully with a user-visible message, or re-raise with added context. "Swallow and continue" is almost never acceptable. * For each GAP (unrescued error that should be rescued): specify the rescue action and what the user should see. * For LLM/AI service calls specifically: what happens when the response is malformed? When it's empty? When it hallucinates invalid JSON? When the model returns a refusal? Each of these is a distinct failure mode. diff --git a/plan-eng-review/SKILL.md b/plan-eng-review/SKILL.md index 96902f6e..896e43ff 100644 --- a/plan-eng-review/SKILL.md +++ b/plan-eng-review/SKILL.md @@ -205,7 +205,7 @@ Evaluate: **STOP.** For each issue found in this section, call AskUserQuestion individually. One issue per call. Present options, state your recommendation, explain WHY. Do NOT batch multiple issues into one AskUserQuestion. Only proceed to the next section after ALL issues in this section are resolved. ### 3. Test review -Make a diagram of all new UX, new data flow, new codepaths, and new branching if statements or outcomes. For each, note what is new about the features discussed in this branch and plan. Then, for each new item in the diagram, make sure there is a JS or Rails test. +Make a diagram of all new UX, new data flow, new codepaths, and new branching if statements or outcomes. For each, note what is new about the features discussed in this branch and plan. Then, for each new item in the diagram, make sure there is a corresponding test. For LLM/prompt changes: check the "Prompt/LLM changes" file patterns listed in CLAUDE.md. If this plan touches ANY of those patterns, state which eval suites must be run, which cases should be added, and what baselines to compare against. Then use AskUserQuestion to confirm the eval scope with the user. diff --git a/plan-eng-review/SKILL.md.tmpl b/plan-eng-review/SKILL.md.tmpl index 2a2dcced..e5eeec5c 100644 --- a/plan-eng-review/SKILL.md.tmpl +++ b/plan-eng-review/SKILL.md.tmpl @@ -101,7 +101,7 @@ Evaluate: **STOP.** For each issue found in this section, call AskUserQuestion individually. One issue per call. Present options, state your recommendation, explain WHY. Do NOT batch multiple issues into one AskUserQuestion. Only proceed to the next section after ALL issues in this section are resolved. ### 3. Test review -Make a diagram of all new UX, new data flow, new codepaths, and new branching if statements or outcomes. For each, note what is new about the features discussed in this branch and plan. Then, for each new item in the diagram, make sure there is a JS or Rails test. +Make a diagram of all new UX, new data flow, new codepaths, and new branching if statements or outcomes. For each, note what is new about the features discussed in this branch and plan. Then, for each new item in the diagram, make sure there is a corresponding test. For LLM/prompt changes: check the "Prompt/LLM changes" file patterns listed in CLAUDE.md. If this plan touches ANY of those patterns, state which eval suites must be run, which cases should be added, and what baselines to compare against. Then use AskUserQuestion to confirm the eval scope with the user.