Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions skills/code-review/SKILL.md

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[logic-error] MEDIUM — The frontmatter description (lines 5-6) still says this skill "evaluates across six review dimensions" with no qualifier — a separate, still-unconditional claim from the already-fixed "all six dimensions" wording in the Constraints section below (that fix was correctly scoped to the Standard branch; this frontmatter line predates it and wasn't touched).

Remediation: Update to something like "evaluates across the applicable review dimensions (two for simple changes, six for standard)."

Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,21 @@ git log --oneline -10 -- <test-file-path>

### 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.
Comment on lines +68 to +76

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[design-smell] HIGH — This section and pr-review/SKILL.md:327-351 are meant to define the same "simple" classification but have drifted in substance, not just wording. pr-review has a "no added lines" qualifier, concrete CI/doc file-type lists, and 4 edge cases (security-guard deletion, CI permission/secret changes, "1 line of new logic → standard," "3+ files even if mechanical renames → standard"). This section reproduces only 1 of those 4 edge cases in a single dense paragraph. A standalone code-review invocation (its documented use case) can classify a change as simple that pr-review's fuller criteria would correctly reject as standard. (Flagged as a low "design-smell"/sync-comment suggestion in a prior review round — deeper look shows it's a real behavioral gap between the two entry points, not cosmetic duplication.)

Remediation: Port pr-review's full edge-case list and qualifiers into this section verbatim, or have one file explicitly reference the other as the source of truth for the criteria.


**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.

Expand Down Expand Up @@ -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.)
118 changes: 104 additions & 14 deletions skills/pr-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[premature-decision] MEDIUM — Issue #439 reserves full review for changes that (among other things) "span multiple subsystems." This criterion never tests that directly — two files belonging to unrelated subsystems (e.g., one workflow file for one service plus one doc file for a different one) would still pass every "simple" criterion. The file-count proxy was never discussed or confirmed against the issue's actual "multiple subsystems" language.

Remediation: Either add an explicit "does not span multiple subsystems/services" criterion, or note in the PR that file count is treated as a sufficient proxy and get that confirmed.

- The change does not modify production logic (application code,
library code, controllers, handlers, models)
Comment on lines +334 to +335

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[edge-case] MEDIUM — This criterion excludes changes that "modify" production logic, but criterion 1 above independently allows any "purely subtractive (deletion-only, no added lines)" diff. If deleting a file wholesale isn't read as "modifying" it, a full removal of a non-auth controller/handler/model could pass both criteria — yet plausibly still needs intent-coherence (was the deletion authorized/intentional?) and cross-repo-contracts (does anything still call it?) review. Only the security-guard variant of this pattern is called out as an edge case below.

Remediation: Reword to "does not add, modify, or remove production logic," or add an edge case: "deletion of a production application/library file (controller, handler, model, or public function) is standard, not simple."

- 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
Comment on lines +343 to +351

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[fail-open] HIGH — Neither this edge-case list nor code-review/SKILL.md's calibration section excludes test-file deletions from "simple." A deletion-only PR removing test files/cases qualifies as simple today, which routes it to the trivial scope constraint at line 624 — "Do NOT read... git history" — forbidding exactly the check code-review/SKILL.md:44-50 prescribes for this scenario (git log --oneline -10 -- <test-file-path> to catch weakened coverage). This codebase's own correctness dimension explicitly tracks test-weakened/test-removed/coverage-reduced as findings, so this is a first-class risk falling through the simple path's trivial scope.

Remediation: Add an edge case here mirroring the existing security-guard carve-out, e.g. "A change that deletes or modifies test files/test cases is NOT simple — test coverage changes require the full correctness check, including git history on the affected test file."


#### 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`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[premature-decision] MEDIUM — Issue #439's "Proposed change" asks only for shorter output ("produce a brief summary... rather than a multi-dimension analysis table"). This dispatch rule additionally skips 4 sub-agents entirely and (via the line-624 scope constraint) caps the remaining 2 at 5 tool calls with git-history/context reads forbidden — a materially larger change to investigation depth than what the issue asked for, not explicitly confirmed anywhere in the PR or issue thread as an intended scope expansion.

Remediation: Confirm with the issue author/reviewer whether reduced investigation (not just reduced write-up) was intended; if so, say so explicitly in the PR description rather than leaving it implicit here.

Do not dispatch conditional sub-agents (`security`,
`intent-coherence`, `docs-currency`, `cross-repo-contracts`)
regardless of step 3b's domain classification. The challenger pass

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[logic-error] MEDIUM — This "regardless of step 3b's domain classification" override doesn't address the separately-stated "Re-review dispatch (prior-finding-aware)" rule later in this same step (unchanged by this PR): when PRIOR_REVIEW_PROVENANCE is app-verified and prior findings exist, dimensions with prior findings dispatch at normal scope "to verify the fixes." Neither rule states precedence for a re-review whose current diff also now satisfies the simple criteria (e.g., a fix that shrank the diff to a 1-file deletion). This is the same shape of ambiguity round 1 flagged for challenger (resolved by making challenger unconditional) but was never generalized to the other four conditional dimensions.

Remediation: Add an explicit precedence rule, e.g. "a dimension with prior findings always dispatches at normal scope to verify the fix, even when the current diff classifies as simple."

(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.
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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) |
Expand Down Expand Up @@ -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
<!-- **Head SHA:** abc1234 -->
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.
```
Comment on lines +1188 to +1194

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[logic-error] HIGH — This example asserts two conclusions the simple path's own scope constraint can't back up. "No remaining references to the deleted task" requires searching other files, but the trivial scope constraint at line 624 is a stated hard limit: "Do NOT read... surrounding files... directory listings." And "No logic or security implications" is asserted despite security never being dispatched for simple changes. Worth noting: issue #439's own "what happened" section cites "'No security implications — removes a documentation-enforcement CI check only,' which are self-evident for a deletion-only change" as the exact noise this PR exists to eliminate — this example now reproduces almost that same phrase.

Remediation: Rewrite to claims verifiable from the diff alone under trivial scope, e.g. "the deletion is confined to the task file and its README entry; the diff touches no security-sensitive paths" instead of asserting an investigation-backed conclusion.


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.
Comment on lines +1196 to +1199

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[logic-error] HIGH — This escape hatch only changes which markdown template step 7 uses; it can't actually re-investigate. By this point, dispatch already happened at step 3c/4 (this document is explicitly sequential — "Follow these steps in order. Do not skip steps.", line 84 — with no reclassify/re-dispatch/loop-back instruction anywhere in the file). So "fall through to the standard format" still only has correctness/style-conventions findings to show, in a template that implies full 6-dimension coverage. code-review/SKILL.md:74-76's parallel language ("reclassify as standard and evaluate all dimensions") works there because it's single-pass — the equivalent doesn't exist here.

Remediation: State that a misclassification discovered here triggers a return to step 3c/4 to dispatch the previously-skipped conditional sub-agents before synthesis — not just a template change at step 7.


#### 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,
Expand Down Expand Up @@ -1158,9 +1229,21 @@ where `[open]` = `<` + `!--` and `[close]` = `--` + `>`.

...

#### Medium / Low / Info
#### Medium

...

#### Low

...

<details>
<summary>Info findings</summary>

- **[<category>]** `<file>:<line>` — <description>
- ...

</details>
```

**Formatting rules:**
Expand All @@ -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 `<details>`
block with `<summary>Info findings</summary>`. 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 `<details>` 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.
Expand Down
Loading