feat(review): rebuild the review as a skill plus two second-eye subagents

[anilcancakir]

What

commands/flutter-review.md was a 29-line checklist that ran in the same context as the code it reviewed. It is replaced by:

File	Role
skills/flutter-review/SKILL.md	Orchestrator: resolve scope, pick the tier, spawn, verify, report, fix, re-review
agents/flutter-reviewer.md	sonnet / effort: medium, read-only. Four gated stages
agents/flutter-reviewer-deep.md	opus / effort: high, read-only. The same four plus a cross-layer stage
references/flutter-review-core.md	The shared stages, severity and confidence rule, not-a-finding list, report shape
references/dart-style-core.md	The style floor when no personal style skill is loaded

Stages. 1 magic compliance (bootstrap, IoC, controller/view/state, models, HTTP, validation, auth, routing) gates the rest; 2 Wind compliance (dark pairing, states: over interpolation, layout contracts, dead tokens, widget choice); 3 correctness, OOP, style; 4 reuse and simplification. The deep reviewer adds stage 5: the feature read as one path (controller to view to state, model to HTTP to validation, route to middleware to authorization) plus a comparison against an existing feature in the same app.

Fix loop. The reviewer never edits. The skill shows each fix before making it, batches the mechanical ones into a single approval, asks separately for anything behavioral, applies only inside the reviewed scope, runs the analyzer and tests, then re-spawns the reviewer once. It stops after two rounds.

Why this shape

• The second eye has to be a second context. Reviewing your own edits in the window that produced them re-reads the assumptions that caused the defect. The subagent starts with a fresh context and never sees the conversation.
• Findings are claims. The skill re-opens every cited file:line, re-checks the rule against the skill that owns it, and traces the failure scenario before showing anything, then says how many it dropped. A reviewer is as fluent when it is wrong as when it is right.
• No rule is copied. The core file names which rulebook decides each check; the agents read the live wind-ui and magic-framework skills. A rulebook copied into a review prompt is the one thing here guaranteed to rot, and most of this session was spent repairing exactly that failure in the skills themselves.
• Anti-noise. Severity plus confidence with a reporting floor (CRITICAL and IMPORTANT at confidence 50+, under 80 tagged), and a disproof pass that tries to kill each candidate before it ships. A clean scope reports NONE; there is no minimum finding count.
• my-coding without a hard dependency. It is listed in each agent's skills:. Claude Code skips a missing skill with a debug warning, so the personal rules apply on machines that have them and cost nothing elsewhere, with references/dart-style-core.md as the floor either way.

Research behind the design: Anthropic's plugin and subagent references for the agents/ contract and the field list, its own /security-review (find, then a parallel per-finding false-positive filter, then a confidence gate) and /code-review (background subagent, ReportFindings), and the community implementations that hold up, whose recurring anti-noise levers are a severity floor and a numeric confidence threshold applied before dedup.

Testing

• Frontmatter of all four components parses, and both agents use only fields plugin-shipped agents support (hooks, mcpServers, permissionMode are ignored for plugin agents, so they are absent).
• Trigger surface of the skill: 1,405 characters against the 1,536 listing cap.
• Every ${CLAUDE_PLUGIN_ROOT} path referenced by the skill, both agents, and the core file resolves to a file that exists.
• actionlint clean on the modified workflow; both new gates dry-run green locally.
• No em-dash or en-dash in any added line.

Two new CI gates, both from defects hit while writing this:

1. Frontmatter parses, and agent frontmatter uses only supported fields. An unquoted description containing ": " silently disabled both agents: YAML reads it as a nested mapping, the component never loads, and nothing errors at runtime.
2. Every ${CLAUDE_PLUGIN_ROOT} path exists. A bad one fails inside a subagent as a file-not-found the user never sees.

Note on skills/index.json

skills/flutter-review/ is deliberately absent from the index: it orchestrates Claude Code subagents, which the other tools the registry serves cannot spawn. Documented in CLAUDE.md. Every other directory under skills/ remains a mirror and stays in the index.

Not yet exercised end to end

The wiring cannot be tested from this repo: plugin agents load only where the plugin is installed. The stages were dry-run against a real magic + Wind app to check they produce actionable findings rather than noise; the first live /fluttersdk:flutter-review after install is the real test.

[anilcancakir]

Dry-run against magic_example (49 Dart files) before calling this done, and it changed the design.

Stage 1 behaved as intended. Two candidates, one real and one correctly killed:

• lib/app/models/team.dart:59-83 uses getAttribute('id') with a cast in six typed getters, where magic Core Law 7 and the anti-pattern table both say get<T>('key'). A genuine IMPORTANT finding with file:line, in your own example app.
• "No controller uses the findOrPut accessor" looked like a finding until the disproof pass: the app has no MagicController subclass at all, so there is nothing to flag. Exactly the candidate the pass exists to kill.

Stage 2 found a defect in my own check. The dark-pairing rule flagged 38 of 76 className strings. Every one was a semantic alias: bg-surface, text-fg, border-color-border. lib/config/wind_theme.g.dart defines 'bg-surface': 'bg-[#FFFFFF] dark:bg-[#030712]', so the alias carries its own dark peer and the finding is false.

A check that is wrong half the time on the first real app is worse than no check. Pushed a fix: Stage 2 now resolves WindThemeData.aliases before flagging anything, and the core file carries the measurement so the next person does not re-derive it.

The wiring itself still needs an install to exercise: plugin agents load only where the plugin is installed, so the first live /fluttersdk:flutter-review is the real test.

[Copilot]

Pull request overview

This PR replaces the old /fluttersdk:flutter-review command checklist with a first-class flutter-review skill that orchestrates two read-only “second-eye” reviewer subagents, supported by shared review-core and style-core references, and adds CI gates to prevent silent plugin-load failures.

Changes:

• Add skills/flutter-review/SKILL.md orchestrator plus two reviewer subagents (flutter-reviewer, flutter-reviewer-deep) and shared rule/reference docs.
• Remove the legacy commands/flutter-review.md checklist and update docs to reflect the new skill/subagent model.
• Add CI checks to (1) parse frontmatter and validate supported agent fields, and (2) ensure ${CLAUDE_PLUGIN_ROOT} references resolve.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
skills/flutter-review/SKILL.md	New orchestrator skill for scoping, spawning reviewer agents, verifying findings, and coordinating fixes + re-review
agents/flutter-reviewer.md	New read-only standard reviewer agent (4-stage gated review contract)
agents/flutter-reviewer-deep.md	New read-only deep reviewer agent (adds cross-layer adaptation stage)
references/flutter-review-core.md	Shared staged review rubric, severity/confidence rules, and output contract
references/dart-style-core.md	Baseline Dart/Flutter style floor when no personal style skill is loaded
commands/flutter-review.md	Removes legacy checklist-based command
README.md	Documents the new review skill and reviewer subagents in plugin auto-registration list
CLAUDE.md	Updates repo architecture docs for agents/references and clarifies skills/flutter-review ownership/indexing
.github/workflows/ci.yml	Adds CI gates for frontmatter parsing/field validation and ${CLAUDE_PLUGIN_ROOT} reference existence

[anilcancakir]

Corrected the skill-preload assumption after a direct question about whether unprefixed names resolve for a plugin's own skills.

They are not documented to. The skills doc says plugin skills use a plugin-name:skill-name namespace, and that the bare form works for invocation only "unless another command already uses that name". The skills: preload field is a separate resolution path that the docs never describe for a plugin's own skill, in either form.

What settles it is the failure mode, not the odds: a name that does not resolve is skipped with a warning in the debug log and nowhere else. The reviewer would have started with no rulebook, reviewed magic and Wind from memory, and reported confident nonsense with nothing anywhere saying why.

Both agents now read ${CLAUDE_PLUGIN_ROOT}/skills/magic-framework/SKILL.md and .../wind-ui/SKILL.md by path as step 2 of execution. A path either resolves or errors, and they already open those skills' references/ files mid-review, so it is their normal mode rather than a workaround. skills: keeps my-coding, where the bare name is the documented identity for a user skill and the ac plugin's agents already prove that path works from inside a plugin agent.

[anilcancakir]

Both addressed, one of them with a correction to the premise.

PyYAML. The prediction was wrong but the concern was right. The gate ran and passed in CI (four frontmatter OK lines in the Plugin manifest + version sync job), so the runner image does ship PyYAML today. That is precisely the problem: the dependency was inherited rather than declared, and an image update would have turned this gate red with nothing wrong in the repo. Added an explicit install step.

color. Correct, and I introduced the inconsistency myself. The plugins reference enumerates plugin-agent support as name, description, model, effort, maxTurns, tools, disallowedTools, skills, memory, background, isolation; color appears in the general subagent field table but not in that list, and this repo's CLAUDE.md quotes the plugin list. Shipping a field the loader may ignore, while running a gate whose whole purpose is to prevent that, is the wrong side of the argument to be on. Removed from both agents and from the allow-list. If it turns out to be supported, the cost was two agents rendering in the default color; the reverse mistake is invisible.