Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
8 changes: 4 additions & 4 deletions docs/skills/ce-code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ A small config change triggers 6 reviewers (the 4 always-on + 2 CE always-on). A

- **Always-on (every review)** — `ce-correctness-reviewer`, `ce-testing-reviewer`, `ce-maintainability-reviewer`, `ce-project-standards-reviewer`, `ce-agent-native-reviewer`, `ce-learnings-researcher`
- **Cross-cutting conditional** — security, performance, API contract, data migrations, reliability, adversarial, previous-comments — each selected only when the diff touches its concern
- **Stack-specific conditional** — DHH-Rails, Kieran-Rails / Python / TypeScript, Julik frontend races, Swift/iOS — only when the matching stack is present
- **CE conditional (migrations)** — schema-drift detector, deployment-verification agent for diffs with migration files
- **Stack-specific conditional** — Julik frontend races, Swift/iOS — only when the matching runtime domain is touched. Structural quality (complexity deletion, 1k-line regressions, spaghetti) lives in the always-on maintainability persona.
- **CE conditional (migrations)** — `ce-deployment-verification-agent` for risky migration diffs; schema drift and migration safety are handled by the `data-migration` persona

Persona selection is agent judgment, not keyword matching. Instruction-prose files (Markdown skills, JSON schemas) are product code but skip runtime-focused reviewers (adversarial, races) — they wouldn't apply.

Expand Down Expand Up @@ -118,7 +118,7 @@ You invoke `/ce-code-review` on a feature branch with a Rails auth change that i

The skill detects you're on a feature branch (no PR yet), resolves the base from `origin/HEAD` (or PR metadata when an open PR exists), and computes the diff. Stage 2 reads commit messages and writes a 2-3 line intent summary. Stage 2b auto-discovers the plan in `docs/plans/` from the branch name and reads its Requirements (R1-R8, U1-U6).

Stage 3 selects reviewers: the 6 always-on, plus security (auth touched), reliability (background job for token cleanup), data migrations (migration file present), kieran-rails + dhh-rails (stack), schema-drift detector and deployment-verification agent (CE migration conditionals). Ten reviewers total, dispatched in parallel.
Stage 3 selects reviewers: the 6 always-on, plus security (auth touched), reliability (background job for token cleanup), data-migration (migration file present), and deployment-verification agent when the migration is risky. Seven or eight reviewers total, dispatched in parallel.

After all return, synthesis merges 23 raw findings into 14 distinct findings. Three are `safe_auto` (typo, rename, dead code) and applied automatically. Six are `gated_auto` for the auth surface — routed into the interactive walk-through. Two are `manual` (deployment Go/No-Go checklist items). Three are `advisory` (FYI notes). Each finding has anchored evidence and a stable number.

Expand Down Expand Up @@ -194,7 +194,7 @@ Conflicting mode flags stop execution with an error. Combining `base:` with a PR
Use it when it's the right tool — the quick-review short-circuit defers to it explicitly. `ce-code-review` is for cases where you want diff-aware persona selection, structured findings with calibrated severity, autofix routing, and residual work handling. It's the heavier tool; reach for it when the work warrants.

**How does it decide which personas to dispatch?**
Agent judgment over the actual diff — not keyword matching. The 4 always-on + 2 CE always-on personas run for every review. Cross-cutting and stack-specific personas are added when their concern is touched (e.g., security if auth files changed; data-migrations-reviewer if migration files are present). Instruction-prose files skip runtime-focused reviewers (adversarial, races).
Agent judgment over the actual diff — not keyword matching. The 4 always-on + 2 CE always-on personas run for every review. Cross-cutting and stack-specific personas are added when their concern is touched (e.g., security if auth files changed; `ce-data-migration-reviewer` when migration or schema dump files are present). Instruction-prose files skip runtime-focused reviewers (adversarial, races).

**What's the difference between Autofix and Headless?**
Autofix applies `safe_auto` fixes silently and emits a Residual Actionable Work summary for the caller to route. Headless is similar but returns *all* findings as structured text (including `safe_auto`) and never enters bounded re-review rounds. Headless is for programmatic skill-to-skill invocation; Autofix is for orchestrators that own the residual-handling UI.
Expand Down
4 changes: 2 additions & 2 deletions docs/skills/ce-compound.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ After capturing the new learning, `ce-compound` checks whether it should invoke

### 6. Specialized post-review

Based on the problem type, optional specialized agents review the documentation: `ce-performance-oracle` for performance issues, `ce-security-sentinel` for security, `ce-data-integrity-guardian` for database, and a stack-matched `ce-kieran-rails-reviewer` / `ce-kieran-python-reviewer` / `ce-kieran-typescript-reviewer` for code-heavy issues plus `ce-code-simplicity-reviewer` always.
Based on the problem type, optional specialized agents review the documentation: `ce-performance-oracle` for performance issues, `ce-security-sentinel` for security, `ce-data-integrity-guardian` for database, and `ce-code-simplicity-reviewer` for code-heavy issues.

### 7. Session history integration (opt-in)

Expand All @@ -102,7 +102,7 @@ Three subagents dispatch in parallel: Context Analyzer reads conversation histor

The orchestrator assembles the doc, validates frontmatter via the YAML safety script, and writes `docs/solutions/performance-issues/n-plus-one-brief-generation.md`. The discoverability check finds `AGENTS.md` doesn't mention `docs/solutions/`, proposes a one-line addition to the existing directory listing, and applies it after you confirm.

Phase 3 dispatches `ce-performance-oracle` and `ce-kieran-rails-reviewer` to validate the code examples and approach. Phase 2.5 surfaces a refresh recommendation: the older N+1 doc may benefit from consolidation review. The skill suggests `/ce-compound-refresh n-plus-one` as a narrow scope hint and ends.
Phase 3 dispatches `ce-performance-oracle` and `ce-code-simplicity-reviewer` to validate the code examples and approach. Phase 2.5 surfaces a refresh recommendation: the older N+1 doc may benefit from consolidation review. The skill suggests `/ce-compound-refresh n-plus-one` as a narrow scope hint and ends.

---

Expand Down
8 changes: 1 addition & 7 deletions plugins/compound-engineering/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,20 +114,14 @@ Agents are specialized subagents invoked by skills — you typically don't call
| `ce-code-simplicity-reviewer` | Final pass for simplicity and minimalism |
| `ce-correctness-reviewer` | Logic errors, edge cases, state bugs |
| `ce-data-integrity-guardian` | Database migrations and data integrity |
| `ce-data-migration-expert` | Validate ID mappings match production, check for swapped values |
| `ce-data-migrations-reviewer` | Migration safety with confidence calibration |
| `ce-data-migration-reviewer` | Schema drift, migration safety, mapping verification, deploy-window checks |
| `ce-deployment-verification-agent` | Create Go/No-Go deployment checklists for risky data changes |
| `ce-dhh-rails-reviewer` | Rails review from DHH's perspective |
| `ce-julik-frontend-races-reviewer` | Review JavaScript/Stimulus code for race conditions |
| `ce-kieran-rails-reviewer` | Rails code review with strict conventions |
| `ce-kieran-python-reviewer` | Python code review with strict conventions |
| `ce-kieran-typescript-reviewer` | TypeScript code review with strict conventions |
| `ce-maintainability-reviewer` | Coupling, complexity, naming, dead code |
| `ce-pattern-recognition-specialist` | Analyze code for patterns and anti-patterns |
| `ce-performance-oracle` | Performance analysis and optimization |
| `ce-performance-reviewer` | Runtime performance with confidence calibration |
| `ce-reliability-reviewer` | Production reliability and failure modes |
| `ce-schema-drift-detector` | Detect unrelated schema.rb changes in PRs |
| `ce-security-reviewer` | Exploitable vulnerabilities with confidence calibration |
| `ce-security-sentinel` | Security audits and vulnerability assessments |
| `ce-swift-ios-reviewer` | Swift and iOS code review -- SwiftUI state, retain cycles, concurrency, Core Data threading, accessibility |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ Use the anchored confidence rubric in the subagent template. Persona-specific gu
- **Code style, naming, structure, dead code** -- ce-maintainability-reviewer owns these
- **Test coverage gaps** or weak assertions -- ce-testing-reviewer owns these
- **API contract breakage** (changed response shapes, removed fields) -- ce-api-contract-reviewer owns these
- **Migration safety** (missing rollback, data integrity) -- ce-data-migrations-reviewer owns these
- **Migration safety** (missing rollback, data integrity, schema drift) -- ce-data-migration-reviewer owns these

Your territory is the *space between* these reviewers -- problems that emerge from combinations, assumptions, sequences, and emergent behavior that no single-pattern reviewer catches.

Expand Down
98 changes: 0 additions & 98 deletions plugins/compound-engineering/agents/ce-data-migration-expert.md

This file was deleted.

119 changes: 119 additions & 0 deletions plugins/compound-engineering/agents/ce-data-migration-reviewer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
---
name: ce-data-migration-reviewer
description: Conditional code-review persona for migration files, schema dumps, backfills, and data transformations. Covers schema drift, mapping correctness, deploy-window safety, and verification plans.
model: inherit
tools: Read, Grep, Glob, Bash, Write
color: blue
---

# Data Migration Reviewer

You are a data migration and schema-change reviewer. Evaluate every migration-related diff for three layers, in order:

1. **Schema drift (when `schema.rb` / `structure.sql` is in the diff)** — unrelated dump changes from other branches
2. **Migration correctness** — swapped mappings, missing backfills, deploy-window breaks, data loss
3. **Verification & rollback** — concrete post-deploy SQL and a credible rollback path for risky changes

Think in terms of the deploy window: old code on new schema, new code on old data, partial failures leaving inconsistent state. Never trust fixtures — production data shapes differ.

## Step 0: Schema drift (when a schema dump is in the diff)

Run this **first** when `db/schema.rb` or `db/structure.sql` appears in the diff. Use the review base ref from caller context (`<review-base>` — merge-base SHA or ref). **Never assume `main`.**

```bash
git diff <review-base> --name-only -- db/migrate/
```

Then diff each dump file that is actually in the PR diff (one or both may apply):

```bash
# When db/schema.rb is in the diff:
git diff <review-base> -- db/schema.rb

# When db/structure.sql is in the diff:
git diff <review-base> -- db/structure.sql
```

Cross-reference every change in each in-scope dump against migrations **in this PR's diff**:

- Schema version (or structure version stamp) should match the PR's newest migration timestamp
- Every new column/table/index in the dump must come from a PR migration
- **Drift:** columns, tables, indexes, or version bumps not explained by PR migrations

When drift is present, emit a **P1** finding on the affected dump path (`db/schema.rb` or `db/structure.sql`) with `autofix_class: manual`, concrete unrelated objects listed, and `suggested_fix`:

```bash
# schema.rb:
git checkout <review-base> -- db/schema.rb
bin/rails db:migrate

# structure.sql (regenerate after restoring and migrating):
git checkout <review-base> -- db/structure.sql
bin/rails db:migrate
```

If neither dump file is in the diff, skip this step.

## Migration safety (what you're hunting for)

- **Swapped or inverted ID/enum mappings** — `1 => TypeA, 2 => TypeB` in code but production has the reverse. Verify each CASE/IF branch and constant hash entry individually.
- **Irreversible migrations without rollback plan** — column drops, precision-losing type changes, data deletes. Destructive `down` missing or non-restorative needs explicit acknowledgment.
- **Missing backfill for new non-nullable columns** — `NOT NULL` without default or backfill fails on existing rows.
- **Deploy-window breaks** — rename/drop before all code paths stop reading; constraints that existing rows violate.
- **Orphaned references** — after drop/rename, search serializers, jobs, admin, rake tasks, `includes`/`joins` for stale columns or associations.
- **Broken dual-write** — transition period requires both old and new columns populated; rollback otherwise sees NULLs.
- **Missing transaction boundaries** — multi-table backfills without appropriate transaction scope.
- **Hot-table index changes** — large-table indexes without concurrent/online creation where available.
- **Silent data loss** — `text` → `varchar(n)` truncation, float → integer precision loss.

## Verification & observability

For non-trivial data transforms, check whether the PR includes (or clearly defers with a ticket):

- Read-only SQL to prove correctness post-deploy (mapping counts, NULL checks, dual-write verification)
- Rollback or feature-flag guardrails for risky paths

Example verification queries (adapt table/column names):

```sql
SELECT legacy_column, new_column, COUNT(*)
FROM <table_name>
GROUP BY legacy_column, new_column;

SELECT COUNT(*) FROM <table_name>
WHERE new_column IS NULL AND created_at > NOW() - INTERVAL '1 hour';
```

Flag missing verification for risky transforms as **P2** `manual` with sample SQL in `suggested_fix`.

## Confidence calibration

Use the anchored confidence rubric in the subagent template.

**Anchor 100** — mechanical: `DROP COLUMN`, `NOT NULL` without backfill, schema drift column with no matching migration, verifiable swapped mapping in code.

**Anchor 75** — migration DDL or drift visible in the diff; concrete orphaned reference you can name.

**Anchor 50** — inferred data impact from app code without visible migration handling. Surfaces only as P0 escape per synthesis rules.

**Anchor 25 or below — suppress.**

## What you don't flag

- Nullable column additions, new tables with defaults, indexes on new/small tables
- Test-only fixtures, seeds, or test DB setup
- Purely additive schema with no existing-row interaction
- Schema drift concerns when neither `db/schema.rb` nor `db/structure.sql` is in the diff

## Output format

Return your findings as JSON matching the findings schema. No prose outside the JSON.

```json
{
"reviewer": "data-migration",
"findings": [],
"residual_risks": [],
"testing_gaps": []
}
```
Loading