diff --git a/skills/code-review/SKILL.md b/skills/code-review/SKILL.md index 9a037ff6..216ffdee 100644 --- a/skills/code-review/SKILL.md +++ b/skills/code-review/SKILL.md @@ -61,6 +61,21 @@ git log --oneline -10 -- ### 3. Evaluate each dimension +#### Complexity calibration + +Before evaluating dimensions, classify the change complexity: + +**Simple** — ALL of the following hold: the diff is purely subtractive +(deletion-only) OR touches only CI/CD configuration or documentation +files; affects 2 or fewer files; does not modify production logic; does +not touch security-sensitive paths. For simple changes, evaluate only +**Correctness** and **Style/conventions** — skip the other four +dimensions. Produce a brief 2-3 sentence assessment rather than a +per-dimension breakdown. If a simple change reveals unexpected +concerns (e.g., a deletion removes a security guard), reclassify as +standard and evaluate all dimensions. + +**Standard** — any change that does not meet all simple criteria. Evaluate all six dimensions independently. Do not let confidence in one dimension carry over to another — each requires its own scrutiny. @@ -306,6 +321,8 @@ wins. - **Never review from the diff alone.** Always read full source files to understand surrounding context. - **Report failure rather than producing a partial review.** If you - cannot complete all six dimensions (tool failure, missing context, - ambiguous findings), state that clearly rather than producing an - incomplete result. + cannot complete all applicable dimensions (tool failure, missing + context, ambiguous findings), state that clearly rather than + producing an incomplete result. ("Applicable" means the dimensions + required by the complexity classification — two for simple changes, + six for standard.) diff --git a/skills/pr-review/SKILL.md b/skills/pr-review/SKILL.md index c033791e..84a5afa0 100644 --- a/skills/pr-review/SKILL.md +++ b/skills/pr-review/SKILL.md @@ -319,13 +319,55 @@ dimensions are relevant: - Repository has documentation files → `docs-currency` - Always included → `style-conventions` +#### 3b-1. Classify change complexity + +After classifying change domains, classify the overall change +complexity as **simple** or **standard**: + +**Simple** — ALL of the following must hold: + +- The diff is purely subtractive (deletion-only with no added lines), + OR the change touches only CI/CD configuration files (workflows, + pipeline definitions, `.pre-commit-config.yaml`) or documentation + files (`.md`, `.adoc`, `.rst`, `README`, `CHANGELOG`) +- The change affects 2 or fewer files +- The change does not modify production logic (application code, + library code, controllers, handlers, models) +- The change does not touch security-sensitive paths (auth, + permissions, secrets, RBAC, token handling) + +**Standard** — any change that does not meet ALL simple criteria. + +Record the classification for use in steps 3c, 3e, and 7. + +**Edge cases:** + +- A deletion that removes a security check or auth guard is NOT + simple — removing security controls requires full analysis +- A CI config change that modifies permissions blocks, secret + references, or token scoping is NOT simple — these are + security-sensitive +- Mostly deletions with even 1 line of new logic → standard +- 3+ files even if all are mechanical renames → standard + #### 3c. Select sub-agents Based on the domain classification, select sub-agents for dispatch. All selected sub-agents run in parallel (with the exception of the challenger, which runs by itself after all other sub-agents have finished). -**Dispatch sub-agents based on the classification — typically 3-6.** +**Simple change dispatch:** When the change is classified as simple +(step 3b-1), dispatch ONLY `correctness` and `style-conventions`. +Do not dispatch conditional sub-agents (`security`, +`intent-coherence`, `docs-currency`, `cross-repo-contracts`) +regardless of step 3b's domain classification. The challenger pass +(step 6d) still runs after dimension sub-agents complete — it is +never skipped. This prevents multi-dimension analysis noise on +deletion-only diffs, README edits, and CI config tweaks while +preserving adversarial challenge of all findings. + +**Standard change dispatch:** For standard changes, dispatch +sub-agents based on the classification — typically 3-6. The orchestrator should auto-select which sub-agents are relevant for the specific change rather than dispatching all agents by default. A complex PR that triggers all conditions legitimately needs all 6. @@ -398,17 +440,21 @@ normal scope (current behavior preserved). **Dispatch examples:** -| PR type | Agents dispatched | -|----------------------------------------------------------|----------------------------------------------------------------------------------| -| Implementation plan | correctness, style-conventions, intent-coherence, docs-currency | -| Typo fix in README | correctness, style-conventions | -| Bug fix in auth middleware | correctness, security, style-conventions, intent-coherence | -| New API endpoint with tests | correctness, security, style-conventions, cross-repo-contracts | -| Large refactor across packages | correctness, style-conventions, intent-coherence, docs-currency | -| CI/CD pipeline change | correctness, security, style-conventions, intent-coherence | -| DB migration + API change | correctness, security, style-conventions, cross-repo-contracts, docs-currency | -| Re-review after fix (prior findings in correctness only) | correctness (full scope), style-conventions (trivial scope), challenger | -| Re-review after fix (prior findings in security only) | correctness (full scope), security (normal scope), style-conventions (trivial scope), challenger | +| PR type | Complexity | Agents dispatched | +|----------------------------------------------------------|------------|----------------------------------------------------------------------------------| +| Deletion of CI config + README edit (2 files) | simple | correctness (trivial scope), style-conventions (trivial scope) | +| Deletion-only removal of a build task | simple | correctness (trivial scope), style-conventions (trivial scope) | +| Typo fix in README | simple | correctness (trivial scope), style-conventions (trivial scope) | +| Implementation plan | standard | correctness, style-conventions, intent-coherence, docs-currency | +| Bug fix in auth middleware | standard | correctness, security, style-conventions, intent-coherence | +| New API endpoint with tests | standard | correctness, security, style-conventions, cross-repo-contracts | +| Large refactor across packages | standard | correctness, style-conventions, intent-coherence, docs-currency | +| CI/CD pipeline change | standard | correctness, security, style-conventions, intent-coherence | +| DB migration + API change | standard | correctness, security, style-conventions, cross-repo-contracts, docs-currency | +| Re-review after fix (prior findings in correctness only) | standard | correctness (full scope), style-conventions (trivial scope) | +| Re-review after fix (prior findings in security only) | standard | correctness (full scope), security (normal scope), style-conventions (trivial scope) | + +The challenger always runs after all dimension sub-agents complete (step 6d) — it is not listed per row because it is never conditional. #### 3c-1. Security-critical file triage (large PRs) @@ -575,6 +621,7 @@ sub-agents must honor — it overrides their default exploration budget. | Change classification | `scope_constraint` | |------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------| +| Simple (step 3b-1) | `"trivial: ≤5 tool calls. Read ONLY the diff and linked issue. Do NOT read project docs, surrounding files, git history, or directory listings. Return findings immediately after scope verification."` | | Mechanical / value-only (digest bump, version bump, hash swap, URL update, feature flag toggle) | `"trivial: ≤5 tool calls. Read ONLY the diff and linked issue. Do NOT read project docs, surrounding files, git history, or directory listings. Return findings immediately after scope verification."` | | Small non-mechanical (under 20 changed lines, structural) | `"small: ≤15 tool calls. Read the diff, linked issue, and up to 3 context files directly relevant to the change."` | | Standard / large | `"none"` (sub-agent uses its own exploration budget) | @@ -1130,7 +1177,31 @@ challenger-adjudicated finding set and evaluate: ### 7. Produce the review result -Compose the review comment using this structure: +#### 7a. Brief format (simple changes) + +When the change was classified as **simple** (step 3b-1) AND the +outcome is `approve` (no medium+ findings), produce a brief review +body: the hidden SHA comment followed by a 2-3 sentence summary of +what was checked and the result. Do not use the `## Review` header, +`### Findings` section, or severity sub-headings. Example: + +```markdown + +Deletion of the Tekton task definition and corresponding README +reference. No logic or security implications — the removal is +clean with no remaining references to the deleted task. Looks +good. +``` + +If a simple change produces medium+ findings (unexpected for a +genuinely simple change — consider whether the complexity +classification was wrong), fall through to the standard format +below. + +#### 7b. Standard format + +For standard changes (or simple changes with medium+ findings), +compose the review comment using this structure: The first line must be an HTML comment embedding the head SHA. Construct it by concatenating: the HTML comment open delimiter, @@ -1158,9 +1229,21 @@ where `[open]` = `<` + `!--` and `[close]` = `--` + `>`. ... -#### Medium / Low / Info +#### Medium ... + +#### Low + +... + +
+Info findings + +- **[]** `:` — +- ... + +
``` **Formatting rules:** @@ -1180,6 +1263,13 @@ where `[open]` = `<` + `!--` and `[close]` = `--` + `>`. section. If there are no findings at all, set the body to the hidden SHA comment followed by a newline and "Looks good to me" — omit the `## Review` header and `### Findings` section entirely. +- **Info-level findings in collapsed section.** When info-level + findings are present, render them inside a collapsed `
` + block with `Info findings`. This keeps the + review focused on actionable findings while preserving info-level + observations for readers who expand the section. If info is the + only severity present, the `
` block appears directly + under `### Findings` (no other severity sub-headings). - **No footer.** Do not append any footer, action-hints block, or boilerplate after findings. The post-review pipeline appends action hints deterministically when appropriate.