From 0db2bde70719522d99083819de468e38add3f9dc Mon Sep 17 00:00:00 2001 From: Sebastien Taggart Date: Sun, 7 Jun 2026 09:06:22 -0400 Subject: [PATCH] Tier /submit-for-review outcomes (clean/informational/needs-attention/must-address) and tighten WARNING/NOTE rubric --- .agents/skills/submit-for-review/SKILL.md | 53 +++++++++++++---------- .claude/commands/submit-for-review.md | 53 +++++++++++++---------- .claude/review-agent-prompt.md | 53 +++++++++++++++++++++-- .cursor/rules/submit-for-review.mdc | 53 +++++++++++++---------- .gemini/skills/submit-for-review/SKILL.md | 53 +++++++++++++---------- skills/github-agile/review-agent.md | 51 ++++++++++++++++++++-- skills/github-agile/submit-for-review.md | 51 +++++++++++++--------- 7 files changed, 251 insertions(+), 116 deletions(-) diff --git a/.agents/skills/submit-for-review/SKILL.md b/.agents/skills/submit-for-review/SKILL.md index 3d2f786..b9e365a 100644 --- a/.agents/skills/submit-for-review/SKILL.md +++ b/.agents/skills/submit-for-review/SKILL.md @@ -224,26 +224,32 @@ Apply QA label and report success (see below). **If `ai` is `"ai"` (default):** -**If APPROVE (no CRITICAL findings):** +Classify the review output into a tier based on which finding tags are present. Emit the tier line to the user **before any action** so the routing decision is visible: -Collect any non-blocking findings (`[WARNING]` or `[NOTE]` lines) from the review output. +- No findings → **`Tier: clean`** — no findings. +- `[NOTE]` lines only (no WARNING, no CRITICAL) → **`Tier: informational`** — N note(s), no action implied. +- One or more `[WARNING]` (no CRITICAL) → **`Tier: needs-attention`** — N warning(s) flagged for your decision. +- One or more `[CRITICAL]` → **`Tier: must-address`** — N blocking finding(s). -- **If there are no non-blocking findings** (clean review): Run the merge command immediately. Apply QA label and report success (see below). +Route on the tier: -- **If there are non-blocking findings**: Present the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` prefix) and say: +**`clean` or `informational`** → auto-merge. Run the merge command immediately. For `informational`, list the NOTEs in the merge confirmation so they remain visible, but do not prompt. Apply QA label and report success (see below). Step 9 does not run for these tiers — NOTEs never become follow-up tickets. - > "The review approved with N non-blocking finding(s): - > - > - > - > Would you like to **address these first** before merging, or **merge now**?" +**`needs-attention`** → stop and ask. Present the WARNINGs as a numbered list (preserve the `[WARNING]` prefix; include any NOTEs separately below for context but do not number them) and say: - Wait for the user to respond. +> "The review approved with N actionable warning(s): +> +> +> +> Would you like to **address now** (return to coding), **follow up later** (merge and create follow-up tickets), or **accept as-is** (merge without follow-ups)?" - - User says **address / fix** → return to the coding loop. Say: "Fix the findings and run `/submit-for-review` again when ready." Do NOT merge. - - User says **merge now** → run the merge command. Apply QA label and report success (see below). Proceed to Step 9 as normal (which will offer to create follow-up issues for any remaining findings). +Wait for the user to respond. -**If REQUEST CHANGES (at least one CRITICAL finding):** Report the findings to the user. Do NOT merge. Say: +- User says **address / fix / now** → return to the coding loop. Say: "Fix the warnings and run `/submit-for-review` again when ready." Do NOT merge. +- User says **follow up / later** → run the merge command. Apply QA label and report success. Proceed to Step 9 to create follow-up issues for the WARNINGs. +- User says **accept / as-is / merge** → run the merge command. Apply QA label and report success. Skip Step 9. + +**`must-address`** → Report the CRITICAL findings to the user. Do NOT merge. Say: > "The review found blocking issues (see above). Fix them and run `/submit-for-review` again." @@ -286,23 +292,26 @@ Report success based on mode: --- -## Step 9 — Offer follow-up issues for non-blocking findings +## Step 9 — Offer follow-up issues for actionable findings **Gate this step entirely** if any of the following are true: - `ai` is `"off"` (no review was performed). - The merge in Step 8 did not actually happen (e.g. `ai` mode with REQUEST CHANGES). -- The review output contains no non-blocking findings. +- The review output contains no actionable findings (no WARNINGs, and no CRITICALs that were merged-over in advisory mode). +- The `ai`-mode `needs-attention` path was routed to "accept as-is" by the user (they explicitly declined follow-ups). +- The `ai`-mode `clean` or `informational` tier was taken (NOTEs never become tickets). -**Collect non-blocking findings** from the review output retained from Step 7: -- Always include lines starting with `[WARNING]` or `[NOTE]`. +**Collect actionable findings** from the review output retained from Step 7: +- Always include lines starting with `[WARNING]`. - If `ai` is `"advisory"`, also include any `[CRITICAL]` lines — the user chose to merge over them, so they are now follow-up candidates too. - If `ai` is `"ai"`, do not include `[CRITICAL]` lines (there should not be any on the merge path, but guard anyway). +- Never include `[NOTE]` lines. NOTEs are purely informational by definition; if the reviewer wanted action, it would have been a WARNING. If the collected list is empty, skip the rest of this step silently. -**Present and ask once.** Show the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: +**Present and ask once** (skip the prompt if the `needs-attention` path already routed to "follow up later" — in that case go straight to creating issues for all WARNINGs). Show the findings as a numbered list (preserve the `[WARNING]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: -> "The review flagged N non-blocking finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." +> "The review flagged N actionable finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." Accept: comma-separated numbers, `all`, or `none`/`skip`/empty. If the input is unparseable, re-prompt once; if still invalid, treat as `none` and move on. @@ -322,7 +331,7 @@ Then create the issue (do NOT use `--body` or heredocs): ``` gh issue create \ - --title "" \ + --title "" \ [--label ""] \ --body-file /followup_body.md ``` @@ -349,5 +358,5 @@ If a single `gh issue create` call fails, report the failure for that finding an - When `ai` is `"off"`, skip the review agent entirely — merge immediately after checks pass. - `/submit-for-review` merges only to `dev` — never directly to `main`. - If `make merge` fails for any reason, report it and stop — do not attempt workarounds. -- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced non-blocking findings. Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. - +- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced actionable findings (WARNINGs in `ai` mode, plus CRITICALs in `advisory` mode). Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. NOTEs never become follow-up tickets. + diff --git a/.claude/commands/submit-for-review.md b/.claude/commands/submit-for-review.md index 78e4f4d..682cfe2 100644 --- a/.claude/commands/submit-for-review.md +++ b/.claude/commands/submit-for-review.md @@ -219,26 +219,32 @@ Apply QA label and report success (see below). **If `ai` is `"ai"` (default):** -**If APPROVE (no CRITICAL findings):** +Classify the review output into a tier based on which finding tags are present. Emit the tier line to the user **before any action** so the routing decision is visible: -Collect any non-blocking findings (`[WARNING]` or `[NOTE]` lines) from the review output. +- No findings → **`Tier: clean`** — no findings. +- `[NOTE]` lines only (no WARNING, no CRITICAL) → **`Tier: informational`** — N note(s), no action implied. +- One or more `[WARNING]` (no CRITICAL) → **`Tier: needs-attention`** — N warning(s) flagged for your decision. +- One or more `[CRITICAL]` → **`Tier: must-address`** — N blocking finding(s). -- **If there are no non-blocking findings** (clean review): Run the merge command immediately. Apply QA label and report success (see below). +Route on the tier: -- **If there are non-blocking findings**: Present the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` prefix) and say: +**`clean` or `informational`** → auto-merge. Run the merge command immediately. For `informational`, list the NOTEs in the merge confirmation so they remain visible, but do not prompt. Apply QA label and report success (see below). Step 9 does not run for these tiers — NOTEs never become follow-up tickets. - > "The review approved with N non-blocking finding(s): - > - > - > - > Would you like to **address these first** before merging, or **merge now**?" +**`needs-attention`** → stop and ask. Present the WARNINGs as a numbered list (preserve the `[WARNING]` prefix; include any NOTEs separately below for context but do not number them) and say: - Wait for the user to respond. +> "The review approved with N actionable warning(s): +> +> +> +> Would you like to **address now** (return to coding), **follow up later** (merge and create follow-up tickets), or **accept as-is** (merge without follow-ups)?" - - User says **address / fix** → return to the coding loop. Say: "Fix the findings and run `/submit-for-review` again when ready." Do NOT merge. - - User says **merge now** → run the merge command. Apply QA label and report success (see below). Proceed to Step 9 as normal (which will offer to create follow-up issues for any remaining findings). +Wait for the user to respond. -**If REQUEST CHANGES (at least one CRITICAL finding):** Report the findings to the user. Do NOT merge. Say: +- User says **address / fix / now** → return to the coding loop. Say: "Fix the warnings and run `/submit-for-review` again when ready." Do NOT merge. +- User says **follow up / later** → run the merge command. Apply QA label and report success. Proceed to Step 9 to create follow-up issues for the WARNINGs. +- User says **accept / as-is / merge** → run the merge command. Apply QA label and report success. Skip Step 9. + +**`must-address`** → Report the CRITICAL findings to the user. Do NOT merge. Say: > "The review found blocking issues (see above). Fix them and run `/submit-for-review` again." @@ -281,23 +287,26 @@ Report success based on mode: --- -## Step 9 — Offer follow-up issues for non-blocking findings +## Step 9 — Offer follow-up issues for actionable findings **Gate this step entirely** if any of the following are true: - `ai` is `"off"` (no review was performed). - The merge in Step 8 did not actually happen (e.g. `ai` mode with REQUEST CHANGES). -- The review output contains no non-blocking findings. +- The review output contains no actionable findings (no WARNINGs, and no CRITICALs that were merged-over in advisory mode). +- The `ai`-mode `needs-attention` path was routed to "accept as-is" by the user (they explicitly declined follow-ups). +- The `ai`-mode `clean` or `informational` tier was taken (NOTEs never become tickets). -**Collect non-blocking findings** from the review output retained from Step 7: -- Always include lines starting with `[WARNING]` or `[NOTE]`. +**Collect actionable findings** from the review output retained from Step 7: +- Always include lines starting with `[WARNING]`. - If `ai` is `"advisory"`, also include any `[CRITICAL]` lines — the user chose to merge over them, so they are now follow-up candidates too. - If `ai` is `"ai"`, do not include `[CRITICAL]` lines (there should not be any on the merge path, but guard anyway). +- Never include `[NOTE]` lines. NOTEs are purely informational by definition; if the reviewer wanted action, it would have been a WARNING. If the collected list is empty, skip the rest of this step silently. -**Present and ask once.** Show the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: +**Present and ask once** (skip the prompt if the `needs-attention` path already routed to "follow up later" — in that case go straight to creating issues for all WARNINGs). Show the findings as a numbered list (preserve the `[WARNING]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: -> "The review flagged N non-blocking finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." +> "The review flagged N actionable finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." Accept: comma-separated numbers, `all`, or `none`/`skip`/empty. If the input is unparseable, re-prompt once; if still invalid, treat as `none` and move on. @@ -317,7 +326,7 @@ Then create the issue (do NOT use `--body` or heredocs): ``` gh issue create \ - --title "" \ + --title "" \ [--label ""] \ --body-file /followup_body.md ``` @@ -344,5 +353,5 @@ If a single `gh issue create` call fails, report the failure for that finding an - When `ai` is `"off"`, skip the review agent entirely — merge immediately after checks pass. - `/submit-for-review` merges only to `dev` — never directly to `main`. - If `make merge` fails for any reason, report it and stop — do not attempt workarounds. -- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced non-blocking findings. Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. - +- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced actionable findings (WARNINGs in `ai` mode, plus CRITICALs in `advisory` mode). Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. NOTEs never become follow-up tickets. + diff --git a/.claude/review-agent-prompt.md b/.claude/review-agent-prompt.md index 1f2b743..bea3999 100644 --- a/.claude/review-agent-prompt.md +++ b/.claude/review-agent-prompt.md @@ -73,6 +73,51 @@ Check these categories in order of priority: - Test coverage demands (flag only if a critical path has zero coverage) - Documentation completeness +## Sensitive-area gate (force CRITICAL) + +If the PR touches any of the following surfaces, emit at least one `[CRITICAL]` finding identifying the area, **regardless of code quality** — the operator must explicitly approve before merge: + +- Authentication or authorization logic +- Payments, billing, or financial transactions +- Secrets handling (API keys, tokens, credentials, encryption keys) +- Production configuration (deploy targets, prod env vars, prod-only feature flags) +- Destructive operations (e.g. `DROP TABLE`, `rm -rf`, `git push --force`, mass deletes, schema drops) + +Phrasing example: `[CRITICAL] PR modifies authentication logic — operator review required regardless of code quality.` + +This gate exists because mistakes in these areas are high-blast-radius and cheap to silently miss. A clean-looking diff in auth code still warrants a human decision. + +## Finding tags — semantic rubric + +Use exactly three tags. The distinction between WARNING and NOTE matters: it determines whether the operator is asked to weigh in. + +### `[CRITICAL]` — blocker + +Something must change before merge. Examples: +- A bug that breaks the stated functionality. +- A security flaw (SQL injection, leaked secret, XSS). +- A sensitive-area touch (see gate above). +- A regression to existing behavior. + +### `[WARNING]` — non-blocking but **actionable** + +Something the operator should decide on: address now, address later (follow-up ticket), or consciously accept. The finding implies *an action*, even if not required to merge. Examples: +- "Function `foo` is duplicated with `bar` in another file — should be consolidated." +- "This handler swallows errors silently — consider logging." +- "New env var `X` has no default — risks breaking local setups." +- "Loop is O(n²) where O(n) is straightforward — fine at current scale but worth simplifying." + +### `[NOTE]` — purely **informational** + +Something the operator might find useful to know but where **no action is implied**. The finding is observational, not advisory. Examples: +- "PR touches both backend and frontend — consistent with the description." +- "The new helper follows the same pattern as existing helpers in this directory." +- "Diff includes a regenerated lock file alongside the dependency bump, as expected." + +If the observation suggests *anything* the operator might want to do — fix, simplify, follow up, document — it is a WARNING, not a NOTE. NOTEs are restatements and confirmations, not nudges. + +When in doubt between WARNING and NOTE, choose WARNING. NOTE is for when you genuinely have nothing to suggest. + ## Review Output Format Post your review as a PR comment via `gh pr comment` with a body structured like this: @@ -85,13 +130,13 @@ Post your review as a PR comment via `gh pr comment` with a body structured like ### Findings - [CRITICAL] Description of blocking issue (if any) -- [WARNING] Description of non-blocking concern (if any) -- [NOTE] Minor observation (if any) +- [WARNING] Description of non-blocking actionable concern (if any) +- [NOTE] Purely informational observation (if any) If no findings: "No issues found. Code looks correct and follows project conventions." ``` -**Phrasing rule for all findings (CRITICAL, WARNING, and NOTE):** Write each one as a self-contained sentence that would make sense as a standalone ticket title — no "see above", "same as previous", or references to other findings. Non-blocking findings may be converted into follow-up GitHub issues after merge, and CRITICAL findings are promoted to follow-up issue titles when the review runs in advisory mode; vague phrasing produces unusable tickets in either case. +**Phrasing rule for CRITICAL and WARNING:** Write each one as a self-contained sentence that would make sense as a standalone ticket title — no "see above", "same as previous", or references to other findings. WARNINGs may be converted into follow-up GitHub issues; CRITICAL findings are promoted to follow-up issue titles when the review runs in advisory mode. Vague phrasing produces unusable tickets in either case. NOTEs are exempt — they are informational and never become tickets, so they can be phrased conversationally. ## Decision Rules @@ -107,4 +152,4 @@ If no findings: "No issues found. Code looks correct and follows project convent - Do not suggest improvements outside the scope of the PR. - Do not re-review files that haven't changed. - Your review is posted to GitHub for the audit trail — keep it professional. - + diff --git a/.cursor/rules/submit-for-review.mdc b/.cursor/rules/submit-for-review.mdc index 7926da6..c98f3d7 100644 --- a/.cursor/rules/submit-for-review.mdc +++ b/.cursor/rules/submit-for-review.mdc @@ -225,26 +225,32 @@ Apply QA label and report success (see below). **If `ai` is `"ai"` (default):** -**If APPROVE (no CRITICAL findings):** +Classify the review output into a tier based on which finding tags are present. Emit the tier line to the user **before any action** so the routing decision is visible: -Collect any non-blocking findings (`[WARNING]` or `[NOTE]` lines) from the review output. +- No findings → **`Tier: clean`** — no findings. +- `[NOTE]` lines only (no WARNING, no CRITICAL) → **`Tier: informational`** — N note(s), no action implied. +- One or more `[WARNING]` (no CRITICAL) → **`Tier: needs-attention`** — N warning(s) flagged for your decision. +- One or more `[CRITICAL]` → **`Tier: must-address`** — N blocking finding(s). -- **If there are no non-blocking findings** (clean review): Run the merge command immediately. Apply QA label and report success (see below). +Route on the tier: -- **If there are non-blocking findings**: Present the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` prefix) and say: +**`clean` or `informational`** → auto-merge. Run the merge command immediately. For `informational`, list the NOTEs in the merge confirmation so they remain visible, but do not prompt. Apply QA label and report success (see below). Step 9 does not run for these tiers — NOTEs never become follow-up tickets. - > "The review approved with N non-blocking finding(s): - > - > - > - > Would you like to **address these first** before merging, or **merge now**?" +**`needs-attention`** → stop and ask. Present the WARNINGs as a numbered list (preserve the `[WARNING]` prefix; include any NOTEs separately below for context but do not number them) and say: - Wait for the user to respond. +> "The review approved with N actionable warning(s): +> +> +> +> Would you like to **address now** (return to coding), **follow up later** (merge and create follow-up tickets), or **accept as-is** (merge without follow-ups)?" - - User says **address / fix** → return to the coding loop. Say: "Fix the findings and run `/submit-for-review` again when ready." Do NOT merge. - - User says **merge now** → run the merge command. Apply QA label and report success (see below). Proceed to Step 9 as normal (which will offer to create follow-up issues for any remaining findings). +Wait for the user to respond. -**If REQUEST CHANGES (at least one CRITICAL finding):** Report the findings to the user. Do NOT merge. Say: +- User says **address / fix / now** → return to the coding loop. Say: "Fix the warnings and run `/submit-for-review` again when ready." Do NOT merge. +- User says **follow up / later** → run the merge command. Apply QA label and report success. Proceed to Step 9 to create follow-up issues for the WARNINGs. +- User says **accept / as-is / merge** → run the merge command. Apply QA label and report success. Skip Step 9. + +**`must-address`** → Report the CRITICAL findings to the user. Do NOT merge. Say: > "The review found blocking issues (see above). Fix them and run `/submit-for-review` again." @@ -287,23 +293,26 @@ Report success based on mode: --- -## Step 9 — Offer follow-up issues for non-blocking findings +## Step 9 — Offer follow-up issues for actionable findings **Gate this step entirely** if any of the following are true: - `ai` is `"off"` (no review was performed). - The merge in Step 8 did not actually happen (e.g. `ai` mode with REQUEST CHANGES). -- The review output contains no non-blocking findings. +- The review output contains no actionable findings (no WARNINGs, and no CRITICALs that were merged-over in advisory mode). +- The `ai`-mode `needs-attention` path was routed to "accept as-is" by the user (they explicitly declined follow-ups). +- The `ai`-mode `clean` or `informational` tier was taken (NOTEs never become tickets). -**Collect non-blocking findings** from the review output retained from Step 7: -- Always include lines starting with `[WARNING]` or `[NOTE]`. +**Collect actionable findings** from the review output retained from Step 7: +- Always include lines starting with `[WARNING]`. - If `ai` is `"advisory"`, also include any `[CRITICAL]` lines — the user chose to merge over them, so they are now follow-up candidates too. - If `ai` is `"ai"`, do not include `[CRITICAL]` lines (there should not be any on the merge path, but guard anyway). +- Never include `[NOTE]` lines. NOTEs are purely informational by definition; if the reviewer wanted action, it would have been a WARNING. If the collected list is empty, skip the rest of this step silently. -**Present and ask once.** Show the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: +**Present and ask once** (skip the prompt if the `needs-attention` path already routed to "follow up later" — in that case go straight to creating issues for all WARNINGs). Show the findings as a numbered list (preserve the `[WARNING]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: -> "The review flagged N non-blocking finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." +> "The review flagged N actionable finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." Accept: comma-separated numbers, `all`, or `none`/`skip`/empty. If the input is unparseable, re-prompt once; if still invalid, treat as `none` and move on. @@ -323,7 +332,7 @@ Then create the issue (do NOT use `--body` or heredocs): ``` gh issue create \ - --title "" \ + --title "" \ [--label ""] \ --body-file /followup_body.md ``` @@ -350,5 +359,5 @@ If a single `gh issue create` call fails, report the failure for that finding an - When `ai` is `"off"`, skip the review agent entirely — merge immediately after checks pass. - `/submit-for-review` merges only to `dev` — never directly to `main`. - If `make merge` fails for any reason, report it and stop — do not attempt workarounds. -- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced non-blocking findings. Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. - +- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced actionable findings (WARNINGs in `ai` mode, plus CRITICALs in `advisory` mode). Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. NOTEs never become follow-up tickets. + diff --git a/.gemini/skills/submit-for-review/SKILL.md b/.gemini/skills/submit-for-review/SKILL.md index 2de8e1e..1f58800 100644 --- a/.gemini/skills/submit-for-review/SKILL.md +++ b/.gemini/skills/submit-for-review/SKILL.md @@ -224,26 +224,32 @@ Apply QA label and report success (see below). **If `ai` is `"ai"` (default):** -**If APPROVE (no CRITICAL findings):** +Classify the review output into a tier based on which finding tags are present. Emit the tier line to the user **before any action** so the routing decision is visible: -Collect any non-blocking findings (`[WARNING]` or `[NOTE]` lines) from the review output. +- No findings → **`Tier: clean`** — no findings. +- `[NOTE]` lines only (no WARNING, no CRITICAL) → **`Tier: informational`** — N note(s), no action implied. +- One or more `[WARNING]` (no CRITICAL) → **`Tier: needs-attention`** — N warning(s) flagged for your decision. +- One or more `[CRITICAL]` → **`Tier: must-address`** — N blocking finding(s). -- **If there are no non-blocking findings** (clean review): Run the merge command immediately. Apply QA label and report success (see below). +Route on the tier: -- **If there are non-blocking findings**: Present the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` prefix) and say: +**`clean` or `informational`** → auto-merge. Run the merge command immediately. For `informational`, list the NOTEs in the merge confirmation so they remain visible, but do not prompt. Apply QA label and report success (see below). Step 9 does not run for these tiers — NOTEs never become follow-up tickets. - > "The review approved with N non-blocking finding(s): - > - > - > - > Would you like to **address these first** before merging, or **merge now**?" +**`needs-attention`** → stop and ask. Present the WARNINGs as a numbered list (preserve the `[WARNING]` prefix; include any NOTEs separately below for context but do not number them) and say: - Wait for the user to respond. +> "The review approved with N actionable warning(s): +> +> +> +> Would you like to **address now** (return to coding), **follow up later** (merge and create follow-up tickets), or **accept as-is** (merge without follow-ups)?" - - User says **address / fix** → return to the coding loop. Say: "Fix the findings and run `/submit-for-review` again when ready." Do NOT merge. - - User says **merge now** → run the merge command. Apply QA label and report success (see below). Proceed to Step 9 as normal (which will offer to create follow-up issues for any remaining findings). +Wait for the user to respond. -**If REQUEST CHANGES (at least one CRITICAL finding):** Report the findings to the user. Do NOT merge. Say: +- User says **address / fix / now** → return to the coding loop. Say: "Fix the warnings and run `/submit-for-review` again when ready." Do NOT merge. +- User says **follow up / later** → run the merge command. Apply QA label and report success. Proceed to Step 9 to create follow-up issues for the WARNINGs. +- User says **accept / as-is / merge** → run the merge command. Apply QA label and report success. Skip Step 9. + +**`must-address`** → Report the CRITICAL findings to the user. Do NOT merge. Say: > "The review found blocking issues (see above). Fix them and run `/submit-for-review` again." @@ -286,23 +292,26 @@ Report success based on mode: --- -## Step 9 — Offer follow-up issues for non-blocking findings +## Step 9 — Offer follow-up issues for actionable findings **Gate this step entirely** if any of the following are true: - `ai` is `"off"` (no review was performed). - The merge in Step 8 did not actually happen (e.g. `ai` mode with REQUEST CHANGES). -- The review output contains no non-blocking findings. +- The review output contains no actionable findings (no WARNINGs, and no CRITICALs that were merged-over in advisory mode). +- The `ai`-mode `needs-attention` path was routed to "accept as-is" by the user (they explicitly declined follow-ups). +- The `ai`-mode `clean` or `informational` tier was taken (NOTEs never become tickets). -**Collect non-blocking findings** from the review output retained from Step 7: -- Always include lines starting with `[WARNING]` or `[NOTE]`. +**Collect actionable findings** from the review output retained from Step 7: +- Always include lines starting with `[WARNING]`. - If `ai` is `"advisory"`, also include any `[CRITICAL]` lines — the user chose to merge over them, so they are now follow-up candidates too. - If `ai` is `"ai"`, do not include `[CRITICAL]` lines (there should not be any on the merge path, but guard anyway). +- Never include `[NOTE]` lines. NOTEs are purely informational by definition; if the reviewer wanted action, it would have been a WARNING. If the collected list is empty, skip the rest of this step silently. -**Present and ask once.** Show the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: +**Present and ask once** (skip the prompt if the `needs-attention` path already routed to "follow up later" — in that case go straight to creating issues for all WARNINGs). Show the findings as a numbered list (preserve the `[WARNING]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: -> "The review flagged N non-blocking finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." +> "The review flagged N actionable finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." Accept: comma-separated numbers, `all`, or `none`/`skip`/empty. If the input is unparseable, re-prompt once; if still invalid, treat as `none` and move on. @@ -322,7 +331,7 @@ Then create the issue (do NOT use `--body` or heredocs): ``` gh issue create \ - --title "" \ + --title "" \ [--label ""] \ --body-file /followup_body.md ``` @@ -349,5 +358,5 @@ If a single `gh issue create` call fails, report the failure for that finding an - When `ai` is `"off"`, skip the review agent entirely — merge immediately after checks pass. - `/submit-for-review` merges only to `dev` — never directly to `main`. - If `make merge` fails for any reason, report it and stop — do not attempt workarounds. -- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced non-blocking findings. Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. - +- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced actionable findings (WARNINGs in `ai` mode, plus CRITICALs in `advisory` mode). Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. NOTEs never become follow-up tickets. + diff --git a/skills/github-agile/review-agent.md b/skills/github-agile/review-agent.md index 33ff33e..9dbeb1b 100644 --- a/skills/github-agile/review-agent.md +++ b/skills/github-agile/review-agent.md @@ -85,6 +85,51 @@ Check these categories in order of priority: - Test coverage demands (flag only if a critical path has zero coverage) - Documentation completeness +## Sensitive-area gate (force CRITICAL) + +If the PR touches any of the following surfaces, emit at least one `[CRITICAL]` finding identifying the area, **regardless of code quality** — the operator must explicitly approve before merge: + +- Authentication or authorization logic +- Payments, billing, or financial transactions +- Secrets handling (API keys, tokens, credentials, encryption keys) +- Production configuration (deploy targets, prod env vars, prod-only feature flags) +- Destructive operations (e.g. `DROP TABLE`, `rm -rf`, `git push --force`, mass deletes, schema drops) + +Phrasing example: `[CRITICAL] PR modifies authentication logic — operator review required regardless of code quality.` + +This gate exists because mistakes in these areas are high-blast-radius and cheap to silently miss. A clean-looking diff in auth code still warrants a human decision. + +## Finding tags — semantic rubric + +Use exactly three tags. The distinction between WARNING and NOTE matters: it determines whether the operator is asked to weigh in. + +### `[CRITICAL]` — blocker + +Something must change before merge. Examples: +- A bug that breaks the stated functionality. +- A security flaw (SQL injection, leaked secret, XSS). +- A sensitive-area touch (see gate above). +- A regression to existing behavior. + +### `[WARNING]` — non-blocking but **actionable** + +Something the operator should decide on: address now, address later (follow-up ticket), or consciously accept. The finding implies *an action*, even if not required to merge. Examples: +- "Function `foo` is duplicated with `bar` in another file — should be consolidated." +- "This handler swallows errors silently — consider logging." +- "New env var `X` has no default — risks breaking local setups." +- "Loop is O(n²) where O(n) is straightforward — fine at current scale but worth simplifying." + +### `[NOTE]` — purely **informational** + +Something the operator might find useful to know but where **no action is implied**. The finding is observational, not advisory. Examples: +- "PR touches both backend and frontend — consistent with the description." +- "The new helper follows the same pattern as existing helpers in this directory." +- "Diff includes a regenerated lock file alongside the dependency bump, as expected." + +If the observation suggests *anything* the operator might want to do — fix, simplify, follow up, document — it is a WARNING, not a NOTE. NOTEs are restatements and confirmations, not nudges. + +When in doubt between WARNING and NOTE, choose WARNING. NOTE is for when you genuinely have nothing to suggest. + ## Review Output Format Post your review as a PR comment via `gh pr comment` with a body structured like this: @@ -97,13 +142,13 @@ Post your review as a PR comment via `gh pr comment` with a body structured like ### Findings - [CRITICAL] Description of blocking issue (if any) -- [WARNING] Description of non-blocking concern (if any) -- [NOTE] Minor observation (if any) +- [WARNING] Description of non-blocking actionable concern (if any) +- [NOTE] Purely informational observation (if any) If no findings: "No issues found. Code looks correct and follows project conventions." ``` -**Phrasing rule for all findings (CRITICAL, WARNING, and NOTE):** Write each one as a self-contained sentence that would make sense as a standalone ticket title — no "see above", "same as previous", or references to other findings. Non-blocking findings may be converted into follow-up GitHub issues after merge, and CRITICAL findings are promoted to follow-up issue titles when the review runs in advisory mode; vague phrasing produces unusable tickets in either case. +**Phrasing rule for CRITICAL and WARNING:** Write each one as a self-contained sentence that would make sense as a standalone ticket title — no "see above", "same as previous", or references to other findings. WARNINGs may be converted into follow-up GitHub issues; CRITICAL findings are promoted to follow-up issue titles when the review runs in advisory mode. Vague phrasing produces unusable tickets in either case. NOTEs are exempt — they are informational and never become tickets, so they can be phrased conversationally. ## Decision Rules diff --git a/skills/github-agile/submit-for-review.md b/skills/github-agile/submit-for-review.md index 3cda6e6..eb59186 100644 --- a/skills/github-agile/submit-for-review.md +++ b/skills/github-agile/submit-for-review.md @@ -248,26 +248,32 @@ Apply QA label and report success (see below). **If `{{REVIEW_GATE}}` is `"ai"` (default):** -**If APPROVE (no CRITICAL findings):** +Classify the review output into a tier based on which finding tags are present. Emit the tier line to the user **before any action** so the routing decision is visible: -Collect any non-blocking findings (`[WARNING]` or `[NOTE]` lines) from the review output. +- No findings → **`Tier: clean`** — no findings. +- `[NOTE]` lines only (no WARNING, no CRITICAL) → **`Tier: informational`** — N note(s), no action implied. +- One or more `[WARNING]` (no CRITICAL) → **`Tier: needs-attention`** — N warning(s) flagged for your decision. +- One or more `[CRITICAL]` → **`Tier: must-address`** — N blocking finding(s). -- **If there are no non-blocking findings** (clean review): Run the merge command immediately. Apply QA label and report success (see below). +Route on the tier: -- **If there are non-blocking findings**: Present the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` prefix) and say: +**`clean` or `informational`** → auto-merge. Run the merge command immediately. For `informational`, list the NOTEs in the merge confirmation so they remain visible, but do not prompt. Apply QA label and report success (see below). Step 9 does not run for these tiers — NOTEs never become follow-up tickets. - > "The review approved with N non-blocking finding(s): - > - > - > - > Would you like to **address these first** before merging, or **merge now**?" +**`needs-attention`** → stop and ask. Present the WARNINGs as a numbered list (preserve the `[WARNING]` prefix; include any NOTEs separately below for context but do not number them) and say: - Wait for the user to respond. +> "The review approved with N actionable warning(s): +> +> +> +> Would you like to **address now** (return to coding), **follow up later** (merge and create follow-up tickets), or **accept as-is** (merge without follow-ups)?" - - User says **address / fix** → return to the coding loop. Say: "Fix the findings and run `/submit-for-review` again when ready." Do NOT merge. - - User says **merge now** → run the merge command. Apply QA label and report success (see below). Proceed to Step 9 as normal (which will offer to create follow-up issues for any remaining findings). +Wait for the user to respond. -**If REQUEST CHANGES (at least one CRITICAL finding):** Report the findings to the user. Do NOT merge. Say: +- User says **address / fix / now** → return to the coding loop. Say: "Fix the warnings and run `/submit-for-review` again when ready." Do NOT merge. +- User says **follow up / later** → run the merge command. Apply QA label and report success. Proceed to Step 9 to create follow-up issues for the WARNINGs. +- User says **accept / as-is / merge** → run the merge command. Apply QA label and report success. Skip Step 9. + +**`must-address`** → Report the CRITICAL findings to the user. Do NOT merge. Say: > "The review found blocking issues (see above). Fix them and run `/submit-for-review` again." @@ -326,23 +332,26 @@ Report success based on mode: --- -## Step 9 — Offer follow-up issues for non-blocking findings +## Step 9 — Offer follow-up issues for actionable findings **Gate this step entirely** if any of the following are true: - `{{REVIEW_GATE}}` is `"off"` (no review was performed). - The merge in Step 8 did not actually happen (e.g. `ai` mode with REQUEST CHANGES). -- The review output contains no non-blocking findings. +- The review output contains no actionable findings (no WARNINGs, and no CRITICALs that were merged-over in advisory mode). +- The `ai`-mode `needs-attention` path was routed to "accept as-is" by the user (they explicitly declined follow-ups). +- The `ai`-mode `clean` or `informational` tier was taken (NOTEs never become tickets). -**Collect non-blocking findings** from the review output retained from Step 7: -- Always include lines starting with `[WARNING]` or `[NOTE]`. +**Collect actionable findings** from the review output retained from Step 7: +- Always include lines starting with `[WARNING]`. - If `{{REVIEW_GATE}}` is `"advisory"`, also include any `[CRITICAL]` lines — the user chose to merge over them, so they are now follow-up candidates too. - If `{{REVIEW_GATE}}` is `"ai"`, do not include `[CRITICAL]` lines (there should not be any on the merge path, but guard anyway). +- Never include `[NOTE]` lines. NOTEs are purely informational by definition; if the reviewer wanted action, it would have been a WARNING. If the collected list is empty, skip the rest of this step silently. -**Present and ask once.** Show the findings as a numbered list (preserve the `[WARNING]` / `[NOTE]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: +**Present and ask once** (skip the prompt if the `needs-attention` path already routed to "follow up later" — in that case go straight to creating issues for all WARNINGs). Show the findings as a numbered list (preserve the `[WARNING]` / `[CRITICAL]` prefix in the display for clarity) and ask exactly: -> "The review flagged N non-blocking finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." +> "The review flagged N actionable finding(s). Create follow-up issues for any of them? Enter numbers (e.g. `1,3`), `all`, or `none`." Accept: comma-separated numbers, `all`, or `none`/`skip`/empty. If the input is unparseable, re-prompt once; if still invalid, treat as `none` and move on. @@ -362,7 +371,7 @@ Then create the issue (do NOT use `--body` or heredocs): ``` gh issue create \ - --title "" \ + --title "" \ [--label ""] \ --body-file /followup_body.md ``` @@ -394,4 +403,4 @@ If a single `gh issue create` call fails, report the failure for that finding an - Merges target `{{BRANCH_PROD}}` (trunk mode). {{/if}} - If `{{MERGE_CMD}}` fails for any reason, report it and stop — do not attempt workarounds. -- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced non-blocking findings. Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. +- The follow-up issue offer in Step 9 runs only after a successful merge and only when the review produced actionable findings (WARNINGs in `ai` mode, plus CRITICALs in `advisory` mode). Never prompt the user for follow-ups when the review blocked the merge — those findings should be fixed, not ticketed. NOTEs never become follow-up tickets.