Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
556c978
refactor(review): consolidate migration personas and trim stack revie…
tmchow May 22, 2026
4df7533
fix(review): address Codex feedback on structure.sql and plan deepening
tmchow May 22, 2026
0b6b5c5
fix(review): restore P3 output and legacy cleanup for removed personas
tmchow May 22, 2026
7d8c2f1
fix(cli): sweep removed ce-* review agents in stale cleanup
tmchow May 22, 2026
0346bea
refactor(review): make ce-code-review review-only and split apply to …
tmchow May 30, 2026
5391c57
merge: integrate origin/main into review-only refactor branch
tmchow May 30, 2026
9bee6ec
fix(review): self-contain followup refs and align agent output templa…
tmchow May 30, 2026
a2ec295
fix(review): JSON skip responses and pr-remote file inspection (#881)
tmchow May 30, 2026
3f22822
fix(review): branch-remote scope and local-aligned PR diffs
tmchow May 30, 2026
bc9d302
fix(review): verify PR head identity and thread scope into validators…
tmchow Jun 2, 2026
633576a
fix(review): use resolved branch ref for intent log; stop double revi…
tmchow Jun 2, 2026
0b4baf7
fix(review): guarantee critical-finding validation and enforce findin…
tmchow Jun 2, 2026
729b7fe
refactor(review): drop Route from per-severity findings tables
tmchow Jun 2, 2026
ee7891e
feat(review): apply safe verified fixes in interactive ce-code-review
tmchow Jun 2, 2026
cbc3439
fix(review): scrub leftover review-only contradictions from the autof…
tmchow Jun 2, 2026
4af3754
fix(review): terse-cell + keyed detail-line findings format with rend…
tmchow Jun 3, 2026
818112e
fix(review): concrete named test for terse Issue-cell discipline
tmchow Jun 3, 2026
f435b99
fix(review): commit safe fixes on a clean tree; gate the push, not th…
tmchow Jun 3, 2026
89cff21
fix(review): resolve PR #881 Codex feedback on the apply / agent-mode…
tmchow Jun 3, 2026
96dbd99
fix(review): keep P0/P1 findings when a Stage 5b validator infra-fails
tmchow Jun 3, 2026
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
582 changes: 157 additions & 425 deletions plugins/compound-engineering/skills/ce-code-review/SKILL.md

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# `autofix_class` rubric (personas)

`autofix_class` describes the **intrinsic shape** of follow-up work — not whether a caller should auto-apply a fix. This skill does not apply fixes; callers interpret findings and own apply policy.

| `autofix_class` | Meaning |
|-----------------|---------|
| `gated_auto` | A concrete change is proposed in `suggested_fix`. Callers may apply after their own judgment. |
| `manual` | Actionable work that needs design input or a decision before code changes. Include `suggested_fix` when you can propose a defensible default. |
| `advisory` | Report-only — learnings, residual risk, rollout notes. |

## Persona guidance

- Prefer `gated_auto` when you can write a defensible `suggested_fix` for a localized change.
- Use `manual` when the right fix depends on product intent, architecture, or cross-cutting refactors.
- Use `advisory` when nothing breaks if left unfixed but the observation has value.
- Do **not** emit `safe_auto` — callers decide what to apply; reviewers classify and propose.

## Owner field

| `owner` | Meaning |
|---------|---------|
| `downstream-resolver` | Caller or human should act after review. |
| `human` | Judgment required before implementation. |
| `release` | Operational / rollout follow-up. |

Do not use `review-fixer`.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@
},
"autofix_class": {
"type": "string",
"enum": ["safe_auto", "gated_auto", "manual", "advisory"],
"description": "Routing class for downstream fixer dispatch. safe_auto = local mechanical fix the fixer applies without approval (test: a one-sentence fix with no 'depends on' clauses, AND no change to function signature, public-API/error contract, security posture, or permission model; for helper extraction, naming/placement must follow mechanically from the shared shape). gated_auto = concrete fix that changes contracts/permissions or whose placement requires a design conversation; needs user approval before apply. manual = actionable work needing design decisions; usually paired with a suggested_fix the user can confirm. advisory = report-only, no code change. The wrong-side cost is symmetric -- bias toward safe_auto when the rubric permits, since misclassifying mechanical fixes as gated_auto makes users triage findings the fixer could have applied."
"enum": ["gated_auto", "manual", "advisory"],
"description": "Routing hint for the caller after review (this skill does not apply fixes). gated_auto = concrete suggested_fix proposed; caller applies after judgment. manual = needs design or cross-cutting decisions. advisory = report-only."
},
"owner": {
"type": "string",
"enum": ["review-fixer", "downstream-resolver", "human", "release"],
"enum": ["downstream-resolver", "human", "release"],
"description": "Who should own the next action for this finding after synthesis"
},
"requires_verification": {
Expand Down Expand Up @@ -119,16 +119,14 @@
"P3": "Low-impact, narrow scope, minor improvement. User's discretion."
},
"autofix_classes": {
"safe_auto": "Local, deterministic code or test fix suitable for the in-skill fixer. Examples: extract duplicated helper, add missing nil check, fix off-by-one, add missing test, remove dead code. Do not default to advisory when a concrete safe fix exists.",
"gated_auto": "Concrete fix exists, but it changes behavior, permissions, contracts, or other sensitive areas that deserve explicit approval. Examples: add auth to unprotected endpoint, change API response shape.",
"manual": "Actionable issue that requires design decisions or cross-cutting changes. Examples: redesign data model, add pagination strategy, choose between architectural approaches.",
"advisory": "Informational or operational item that should be surfaced in the report only. Examples: design asymmetry the PR improves but does not fully resolve, residual risk notes, deployment considerations."
"gated_auto": "Concrete suggested_fix proposed. Caller may apply after judgment — not by this skill.",
"manual": "Actionable issue requiring design decisions or cross-cutting changes.",
"advisory": "Informational or operational item for the report only."
},
"owners": {
"review-fixer": "The in-skill fixer can own this when policy allows.",
"downstream-resolver": "Turn this into residual work for later resolution.",
"human": "A person must make a judgment call before code changes should continue.",
"release": "Operational or rollout follow-up; do not convert into code-fix work automatically."
"downstream-resolver": "Caller or human should act after review.",
"human": "Judgment required before implementation.",
"release": "Operational or rollout follow-up."
},
"return_tiers": {
"description": "Finding fields are split into two tiers. The full schema (with all required fields) applies to the artifact file on disk. The compact return to the orchestrator omits detail-tier fields. Both are valid uses of this schema in different contexts.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Use this **exact format** when presenting synthesized review findings. Findings

**Scope:** merge-base with the review base branch -> working tree (14 files, 342 lines)
**Intent:** Add order export endpoint with CSV and JSON format support
**Mode:** autofix
**Mode:** interactive

**Reviewers:** correctness, testing, maintainability, security, api-contract
- security -- new public endpoint accepts user-provided format parameter
Expand All @@ -29,31 +29,27 @@ Use this **exact format** when presenting synthesized review findings. Findings

| # | File | Issue | Reviewer | Confidence | Route |
|---|------|-------|----------|------------|-------|
| 2 | `export_service.rb:87` | Loads all orders into memory -- unbounded for large accounts | performance | 100 | `safe_auto -> review-fixer` |
| 2 | `export_service.rb:87` | Loads all orders into memory -- unbounded for large accounts | performance | 100 | `gated_auto -> downstream-resolver` |
| 3 | `export_service.rb:91` | No pagination -- response size grows linearly with order count | api-contract, performance | 75 | `manual -> downstream-resolver` |

### P2 -- Moderate

| # | File | Issue | Reviewer | Confidence | Route |
|---|------|-------|----------|------------|-------|
| 4 | `export_service.rb:45` | Missing error handling for CSV serialization failure | correctness | 75 | `safe_auto -> review-fixer` |
| 4 | `export_service.rb:45` | Missing error handling for CSV serialization failure | correctness | 75 | `gated_auto -> downstream-resolver` |

### P3 -- Low

| # | File | Issue | Reviewer | Confidence | Route |
|---|------|-------|----------|------------|-------|
| 5 | `export_helper.rb:12` | Format detection could use early return instead of nested conditional | maintainability | 75 | `advisory -> human` |

### Applied Fixes
### Actionable Findings

- `safe_auto`: Added bounded export pagination guard and CSV serialization failure test coverage in this run

### Residual Actionable Work

| # | File | Issue | Route | Next Step |
|---|------|-------|-------|-----------|
| 1 | `orders_controller.rb:42` | Ownership check missing on export lookup | `gated_auto -> downstream-resolver` | Defer via tracker (requires explicit approval before behavior change) |
| 3 | `export_service.rb:91` | Pagination contract needs a broader API decision | `manual -> downstream-resolver` | Defer via tracker with contract and client impact details |
| # | File | Issue | Route | Notes |
|---|------|-------|-------|-------|
| 1 | `orders_controller.rb:42` | Ownership check missing on export lookup | `gated_auto -> downstream-resolver` | `suggested_fix` present — caller decides whether to apply |
| 3 | `export_service.rb:91` | Pagination contract needs a broader API decision | `manual -> downstream-resolver` | Needs design input before implementation |

### Pre-existing Issues

Expand Down Expand Up @@ -116,15 +112,14 @@ This fails because: no pipe-delimited tables, no severity-grouped `###` headers,
- **Pipe-delimited markdown tables** for findings -- never ASCII box-drawing characters or per-finding horizontal-rule separators between entries (the report-level `---` before the verdict is still required)
- **Escape literal `|` in table cells** -- any `|` inside a finding title, issue description, code snippet, regex pattern, or delimited-string example must be written as `\|`. Unescaped pipes are parsed as column separators and corrupt the row's `Reviewer`, `Confidence`, and `Route` columns. Applies especially to cache-key delimiter examples, regex alternations, and logical-OR operators quoted inside findings.
- **Severity-grouped sections** -- `### P0 -- Critical`, `### P1 -- High`, `### P2 -- Moderate`, `### P3 -- Low`. Omit empty severity levels.
- **Stable sequential finding numbers** -- assign finding numbers once after sorting, continue them across severity sections, and reuse those same numbers when findings are repeated in Residual Actionable Work. Do not restart at `1` for each severity or route bucket.
- **Stable sequential finding numbers** -- assign finding numbers once after sorting, continue them across severity sections, and reuse those same numbers when findings are repeated in Actionable Findings. Do not restart at `1` for each severity or route bucket.
- **Always include file:line location** for code review issues
- **Reviewer column** shows which persona(s) flagged the issue. Multiple reviewers = cross-reviewer agreement.
- **Confidence column** shows the finding's anchor as an integer (`50`, `75`, or `100`). Never render as a float.
- **Route column** shows the synthesized handling decision as ``<autofix_class> -> <owner>``.
- **Header includes** scope, intent, and reviewer team with per-conditional justifications
- **Mode line** -- include `interactive`, `autofix`, `report-only`, or `headless`
- **Applied Fixes section** -- include only when a fix phase ran in this review invocation
- **Residual Actionable Work section** -- include only when unresolved actionable findings were handed off for later work
- **Mode line** -- include `interactive`, `report-only`, or `agent`
- **Actionable Findings section** -- include when the actionable queue is non-empty (findings for the caller to handle)
- **Pre-existing section** -- separate table, no confidence column (these are informational)
- **Learnings & Past Solutions section** -- results from ce-learnings-researcher, with links to docs/solutions/ files
- **Agent-Native Gaps section** -- results from ce-agent-native-reviewer. Omit if no gaps found.
Expand All @@ -136,7 +131,7 @@ This fails because: no pipe-delimited tables, no severity-grouped `###` headers,

## Headless Mode Format

In `mode:headless`, replace the interactive pipe-delimited table report with a structured text envelope. The headless format is defined in the `### Headless output format` section of SKILL.md. Key differences from the interactive format:
In agent mode (`mode:agent`), replace the interactive pipe-delimited table report with a structured text envelope. The agent format is defined in the `### Agent output format` section of SKILL.md. Key differences from the interactive format:

- **No pipe-delimited tables.** Findings use `[severity][autofix_class -> owner] File: <file:line> -- <title>` line format with indented Why/Evidence/Suggested fix lines.
Comment thread
tmchow marked this conversation as resolved.
Outdated
- **Findings grouped by autofix_class** (gated-auto, manual, advisory) instead of severity. Within each group, findings are sorted by severity.
Expand Down
Loading