diff --git a/packages/agents/content/skills/review-criteria/SKILL.md b/packages/agents/content/skills/review-criteria/SKILL.md index ccbcb651..d19b7a05 100644 --- a/packages/agents/content/skills/review-criteria/SKILL.md +++ b/packages/agents/content/skills/review-criteria/SKILL.md @@ -1,6 +1,6 @@ --- name: review-criteria -description: Evaluation criteria for code review — what to examine and what to skip +description: Evaluation criteria and finding classification for code review user-invocable: false --- @@ -33,5 +33,69 @@ Evaluation criteria for code review. Apply proportionally — match depth to ris ## Distinguish author work from legacy -- Focus findings (F/W/T) only on code authored in the current change -- Frame observations in pre-existing code as Legacy (L), not defects +Focus findings (F/W/T) only on code authored in the current change — observations in pre-existing code belong in Legacy (L). + +## Finding scheme (F/W/T/R/S/L) + +Used by review-producing skills and agents for structured code review findings. Also documented in [artifact conventions](../_data/artifact-conventions.md#finding-scheme-fwtrsl) for human reference. Every finding (F/W/T/R/S) must include a concrete action the author can take. Non-actionable observations belong in prose sections (e.g., Technical Assessment), not in numbered findings. + +| ID | Category | Severity | Merge-blocking? | +| ------ | -------------- | -------------- | ------------------------------------------------------------------ | +| `F{n}` | FIXME | critical | Yes — must fix before merge | +| `W{n}` | Warning | warning | May block — questionable decisions requiring justification | +| `T{n}` | TODO | todo | No — should fix, can wait for next PR | +| `R{n}` | Recommendation | recommendation | No — advisable but discretionary | +| `S{n}` | Suggestion | suggestion | No — optional improvement | +| `L{n}` | Legacy | legacy | No — observation in pre-existing code, not authored in this branch | + +### Category criteria + +**FIXME (F)** — must fix before merge: + +- Bugs: incorrect logic, unhandled error paths, data loss risks +- Security: injection, auth bypass, exposed secrets +- Contract violations: breaking API changes, type unsafety +- Test failures: tests that don't pass or don't test what they claim + +**Warning (W)** — questionable, may block merge: + +- Missing edge case handling that could cause runtime errors +- Convention violations that affect maintainability +- Decisions that seem wrong but may be intentional (require justification) + +**TODO (T)** — should fix, not in this PR: + +- Missing or inadequate tests for new functionality +- Performance issues with measurable impact +- Incomplete error handling that won't cause immediate failures + +**Recommendation (R)** — advisable but discretionary: + +- Better patterns available in the codebase +- Opportunities to reduce complexity +- Architectural improvements worth considering + +**Suggestion (S)** — optional improvement: + +- Better naming or code organization +- Additional test cases for edge cases +- Documentation improvements + +**Legacy (L)** — pre-existing code observation: + +- Issues in code not authored in this branch +- Frame as future opportunities, not current defects +- Never count against the review score + +### Overall criticality mapping + +| Findings present | Criticality | Meaning | +| ---------------------------------- | ----------- | -------------------------------------------- | +| None, or only S/R/L | `none` | Ready to merge | +| W and/or T, but no F | `low` | Acceptable to merge with optional follow-ups | +| 1–2 F (straightforward), or many W | `medium` | Needs fixes but approach is sound | +| Multiple F, or structural issues | `high` | Needs significant rework | + +### Re-review severity escalation + +`S → R → T → W → F`. L findings are never escalated. diff --git a/packages/agents/content/subagents/aspect-code-reviewer.md b/packages/agents/content/subagents/aspect-code-reviewer.md index 6b55c1e9..4d335685 100644 --- a/packages/agents/content/subagents/aspect-code-reviewer.md +++ b/packages/agents/content/subagents/aspect-code-reviewer.md @@ -7,6 +7,7 @@ skills: - anti-patterns - common-mistakes - get-default-branch + - review-criteria --- # Code aspect reviewer @@ -60,45 +61,19 @@ Do NOT flag: Each finding must include: -- **ID**: Sequential within category: - -| ID | Category | Severity | Merge-blocking? | -| ------ | -------------- | -------------- | ------------------------------------------------------------------ | -| `F{n}` | FIXME | critical | Yes — must fix before merge | -| `W{n}` | Warning | warning | May block — questionable decisions requiring justification | -| `T{n}` | TODO | todo | No — should fix, can wait for next PR | -| `R{n}` | Recommendation | recommendation | No — advisable but discretionary | -| `S{n}` | Suggestion | suggestion | No — optional improvement | -| `L{n}` | Legacy | legacy | No — observation in pre-existing code, not authored in this branch | - +- **ID**: sequential within category (F/W/T/R/S/L — see `review-criteria` skill for the full finding scheme) - **Location**: `file/path.ts:42` (file and line number) - **Description**: what the issue is - **Recommendation**: what to do about it -## Overall criticality levels - -Classify the overall review into exactly one level: - -### `none` - -- No findings, or only S/R/L findings -- Code complies with project guidelines and contains no bugs - -### `low` - -- W and/or T findings, but no F findings -- Minor guideline deviations or potential issues - -### `medium` - -- 1-2 F findings that are straightforward to fix -- OR many W findings that collectively indicate a quality concern +## Criticality classification -### `high` +Classify the overall review into exactly one level (none/low/medium/high) per the `review-criteria` skill. Domain context for this reviewer: -- Multiple F findings -- OR F findings that require significant rework -- OR systematic guideline violations indicating the developer missed key conventions +- `none`: Code complies with project guidelines and contains no bugs +- `low`: Minor guideline deviations or potential issues +- `medium`: 1-2 straightforward guideline/bug findings, or many W findings collectively indicating a quality concern +- `high`: Systematic guideline violations indicating the developer missed key conventions ## Output format diff --git a/packages/agents/content/subagents/aspect-silent-failure-reviewer.md b/packages/agents/content/subagents/aspect-silent-failure-reviewer.md index f54ddec7..18dde0cc 100644 --- a/packages/agents/content/subagents/aspect-silent-failure-reviewer.md +++ b/packages/agents/content/subagents/aspect-silent-failure-reviewer.md @@ -6,6 +6,7 @@ maxTurns: 15 skills: - anti-patterns - get-default-branch + - review-criteria --- # Silent-failure aspect reviewer @@ -57,45 +58,19 @@ Do NOT flag: Each finding must include: -- **ID**: Sequential within category: - -| ID | Category | Severity | Merge-blocking? | -| ------ | -------------- | -------------- | ------------------------------------------------------------------ | -| `F{n}` | FIXME | critical | Yes — must fix before merge | -| `W{n}` | Warning | warning | May block — questionable decisions requiring justification | -| `T{n}` | TODO | todo | No — should fix, can wait for next PR | -| `R{n}` | Recommendation | recommendation | No — advisable but discretionary | -| `S{n}` | Suggestion | suggestion | No — optional improvement | -| `L{n}` | Legacy | legacy | No — observation in pre-existing code, not authored in this branch | - +- **ID**: sequential within category (F/W/T/R/S/L — see `review-criteria` skill for the full finding scheme) - **Location**: `file/path.ts:42` (file and line number) - **Description**: what the issue is - **Recommendation**: what to do about it -## Overall criticality levels - -Classify the overall review into exactly one level: - -### `none` - -- No error-handling code in the change, or no findings -- Only S/R/L findings - -### `low` - -- W and/or T findings, but no F findings -- Error handling is present but has minor issues - -### `medium` - -- 1-2 F findings that are straightforward to fix -- OR many W findings that collectively indicate a quality concern +## Criticality classification -### `high` +Classify the overall review into exactly one level (none/low/medium/high) per the `review-criteria` skill. Domain context for this reviewer: -- Multiple F findings -- OR F findings that require significant rework -- OR systematic error suppression that indicates a fundamental approach problem +- `none`: No error-handling code in the change, or no findings +- `low`: Error handling is present but has minor issues +- `medium`: 1-2 F findings that are straightforward to fix, or many W findings that collectively indicate a quality concern +- `high`: Systematic error suppression that indicates a fundamental approach problem ## Output format diff --git a/packages/agents/content/subagents/aspect-test-reviewer.md b/packages/agents/content/subagents/aspect-test-reviewer.md index 2cb6d45b..7c377f57 100644 --- a/packages/agents/content/subagents/aspect-test-reviewer.md +++ b/packages/agents/content/subagents/aspect-test-reviewer.md @@ -7,6 +7,7 @@ skills: - anti-patterns - common-mistakes - get-default-branch + - review-criteria --- # Test aspect reviewer @@ -60,45 +61,19 @@ Do NOT flag: Each finding must include: -- **ID**: Sequential within category: - -| ID | Category | Severity | Merge-blocking? | -| ------ | -------------- | -------------- | ------------------------------------------------------------------ | -| `F{n}` | FIXME | critical | Yes — must fix before merge | -| `W{n}` | Warning | warning | May block — questionable decisions requiring justification | -| `T{n}` | TODO | todo | No — should fix, can wait for next PR | -| `R{n}` | Recommendation | recommendation | No — advisable but discretionary | -| `S{n}` | Suggestion | suggestion | No — optional improvement | -| `L{n}` | Legacy | legacy | No — observation in pre-existing code, not authored in this branch | - +- **ID**: sequential within category (F/W/T/R/S/L — see `review-criteria` skill for the full finding scheme) - **Location**: `file/path.ts:42` (file and line number) - **Description**: what the issue is - **Recommendation**: what to do about it -## Overall criticality levels - -Classify the overall review into exactly one level: - -### `none` - -- No source files requiring test coverage in the change, or no findings -- Only S/R/L findings - -### `low` - -- W and/or T findings, but no F findings -- Tests exist but have minor gaps - -### `medium` - -- 1-2 F findings that are straightforward to fix -- OR many W findings that collectively indicate inadequate coverage +## Criticality classification -### `high` +Classify the overall review into exactly one level (none/low/medium/high) per the `review-criteria` skill. Domain context for this reviewer: -- Multiple F findings -- OR new critical functionality with no tests at all -- OR tests that fundamentally don't test what they claim +- `none`: No source files requiring test coverage in the change, or no findings +- `low`: Tests exist but have minor gaps +- `medium`: 1-2 F findings that are straightforward to fix, or many W findings collectively indicating inadequate coverage +- `high`: New critical functionality with no tests, or tests that fundamentally don't test what they claim ## Output format diff --git a/packages/agents/content/subagents/orchestrated-reviewer.md b/packages/agents/content/subagents/orchestrated-reviewer.md index 533a2494..bd542b4d 100644 --- a/packages/agents/content/subagents/orchestrated-reviewer.md +++ b/packages/agents/content/subagents/orchestrated-reviewer.md @@ -40,84 +40,14 @@ You will receive: Each finding must include: -- **ID**: Sequential within category: - -| ID | Category | Severity | Merge-blocking? | -| ------ | -------------- | -------------- | ------------------------------------------------------------------ | -| `F{n}` | FIXME | critical | Yes — must fix before merge | -| `W{n}` | Warning | warning | May block — questionable decisions requiring justification | -| `T{n}` | TODO | todo | No — should fix, can wait for next PR | -| `R{n}` | Recommendation | recommendation | No — advisable but discretionary | -| `S{n}` | Suggestion | suggestion | No — optional improvement | -| `L{n}` | Legacy | legacy | No — observation in pre-existing code, not authored in this branch | - +- **ID**: sequential within category (F/W/T/R/S/L — see `review-criteria` skill for the full finding scheme) - **Location**: `file/path.ts:42` (file and line number) - **Description**: what the issue is - **Recommendation**: what to do about it -### Category criteria - -**FIXME (F)** — must fix before merge: - -- Bugs: incorrect logic, unhandled error paths, data loss risks -- Security: injection, auth bypass, exposed secrets -- Contract violations: breaking API changes, type unsafety -- Test failures: tests that don't pass or don't test what they claim - -**Warning (W)** — questionable, may block merge: - -- Missing edge case handling that could cause runtime errors -- Convention violations that affect maintainability -- Decisions that seem wrong but may be intentional (require justification) - -**TODO (T)** — should fix, not in this PR: - -- Missing or inadequate tests for new functionality -- Performance issues with measurable impact -- Incomplete error handling that won't cause immediate failures - -**Recommendation (R)** — advisable but discretionary: - -- Better patterns available in the codebase -- Opportunities to reduce complexity -- Architectural improvements worth considering - -**Suggestion (S)** — optional improvement: - -- Better naming or code organization -- Additional test cases for edge cases -- Documentation improvements - -**Legacy (L)** — pre-existing code observation: - -- Issues in code not authored in this branch -- Frame as future opportunities, not current defects -- Never count against the review score - -## Overall criticality levels - -Classify the overall review into exactly one level: - -### `none` - -- No findings, or only S/R/L findings -- Code is ready to merge - -### `low` - -- W and/or T findings, but no F findings -- Code is acceptable to merge with optional follow-ups - -### `medium` - -- 1–2 F findings that are straightforward to fix -- OR many W findings that collectively indicate a quality concern - -### `high` +## Criticality classification -- Multiple F findings -- OR F findings that require significant rework -- OR structural issues that indicate the approach needs rethinking +Classify the overall review into exactly one level (none/low/medium/high) per the `review-criteria` skill. ## Output format