Skip to content

Persona/skill duplication: code-reviewer.md inlines the 5-axis framework instead of delegating to code-review-and-quality skill #172

@qinhaihong-red

Description

@qinhaihong-red

Summary

agents/code-reviewer.md (98 lines) inlines its own copy of the five-axis
review framework that already exists in
skills/code-review-and-quality/SKILL.md (348 lines). The persona file
never references the skill — there is no link, import, or "follow the skill"
directive. This contradicts the repo's stated design and is already showing
wording drift.

The intended design (from the repo's own docs)

agents/README.md:21:

"Skills are mandatory hops inside a persona's workflow."

AGENTS.md:76:

"A persona may invoke skills."

The expected pattern: persona = voice + output format + "go follow the skill".
The skill is the single source of truth for the what and how.

What the repo actually has

agents/code-reviewer.md:10-46 contains a self-contained 5-axis framework
(Correctness, Readability, Architecture, Security, Performance), each with
3-5 bullets describing what to check. The persona file never mentions
code-review-and-quality anywhere — including in its ## Composition
block (lines 93-97), which only references /review and /ship slash
commands.

skills/code-review-and-quality/SKILL.md:22-80 contains its own, more
detailed version of those same five axes, plus a severity rubric, multi-
model review pattern, dead-code hygiene, dependency discipline, change
sizing table, and a long exit checklist that exists nowhere else.

Note the asymmetry: the skill does cross-link to other skills
(skills/code-review-and-quality/SKILL.md:61security-and-hardening,
:74performance-optimization), but the persona does not link to
the skill that is supposed to be its mandatory hop.

Drift is already showing

The two copies are not in sync. Examples:

Axis Persona (code-reviewer.md) Skill (code-review-and-quality/SKILL.md)
Correctness — tests "Do the tests actually verify the behavior? Are they testing the right things?" (line 17) "Does it pass all tests? Are the tests actually testing the right things?" (line 33)
Readability 4 bullets, no mention of dead code, no abstraction-cost rule (lines 20-24) 8 bullets including dead-code artifacts, "abstractions earning their complexity", "could this be done in fewer lines?" (lines 38-47)
Architecture 5 bullets, no mention of code duplication (lines 26-31) 5 bullets, includes "Is there code duplication that should be shared?" (lines 53-57)
Security 5 bullets, no security-and-hardening cross-link (lines 33-38) 8 bullets + cross-link to security-and-hardening for detailed guidance (lines 59-70)

When the skill is updated (e.g. a new severity rule, a new dead-code item),
the persona will silently drift further. Code-paths that route to the
persona vs the skill will deliver inconsistent reviews of the same change.

Why this is the structural root cause of the routing problem

(See companion issue.) Because the persona body duplicates the skill rather
than calling it, the persona's description has to claim "I do code review"
in a way that overlaps with the skill's description. The two descriptions
compete for the same intent precisely because the two artifacts redundantly
cover the same job.

Proposed fixes (sketch)

Option A — true delegation (preferred): Strip the inline framework
from code-reviewer.md and replace it with an explicit invocation:

## Process
Invoke the `code-review-and-quality` skill and follow its workflow exactly.

## Voice and output format
[the persona-specific bits: Staff Engineer tone,
 Verdict / Critical / Important / Suggestion / What's Done Well structure]

Pros: single source of truth, no drift, persona description can shrink to
"voice + output format only".
Cons: persona loses portability to harnesses that can't invoke skills
(plain Markdown system-prompt setups). Mitigation: keep an "Appendix for
harnesses without skill support" block that mirrors the skill's checklist.

Option B — explicit standalone copy with a sync marker. Keep both
copies but add a Synced from code-review-and-quality SKILL.md @ <commit>
line at the top of the persona file plus a CI check that fails if the
skill's framework changes without the persona being touched.
Pros: persona stays portable.
Cons: ongoing maintenance burden; drift just becomes harder, not impossible.

Option C — collapse. Remove the persona entirely; have /review go
straight to the skill. The persona's distinct voice and output format would
move into either the skill or the slash command.

The same duplication pattern exists for security-auditor
security-and-hardening and test-engineertest-driven-development,
so whichever direction is chosen should be applied across all three pairs.

Happy to send a PR implementing Option A if the direction sounds right.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions