addyosmani / addyosmani/agent-skills

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

未關閉
#172 2 則留言 1 個 reaction 已指派 0 人 在 GitHub 檢視
主要語言
JavaScript
星號
93.8k
分支
10k
平均合併
3 天 15 小時
30 天內合併 PR
23

描述

## 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:61` → `security-and-hardening`,
`:74` → `performance-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:

~~~markdown
## 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 @ `
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-engineer` ↔ `test-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.

貢獻指南

開啟貢獻指南

評估

這個 Issue 還沒有評估資料。

把新 issue 寄到你的電子郵件信箱

精選適合新手參與的 GitHub issue 摘要。