We have more than 600 skills, spread across nearly 80 plugins. Anyone on the team can write one and publish it to a marketplace the rest of us install from. While reviewing the PRs that added these skills, our team kept leaving the same comments on skill after skill. The same feedback came up so often that it made sense to write it down once. So I built a skill that reviews skills, and we open sourced it today!

The shape of a review#

The reviewer uses 7 categories to report violations: structural discipline, integrity, test coverage, security, content quality, convention, and cost. Each category has a checklist of things to validate. It returns a JSON object because JSON is parsable & predictable, which makes it easier to block CI on a finding than to interpret a big wall of text from an LLM.

{
  "finding_type": "broken-cross-plugin-reference",
  "severity": "major",
  "deterministic": true,
  "location": "SKILL.md:42",
  "explanation": "Reference acme-tools:nonexistent-skill doesn't resolve.",
  "fix": "Create the skill, fix the reference, or remove the claim."
}

The severity can decide whether a finding should block the merge. We try to nudge people to fix critical & major issues first while an automation takes care of minor ones (more on that sometime later). The deterministic flag marks the mechanical checks, which we run with scripts in CI. The AI only reasons through the checks that are not mechanical. The finding_type comes from a closed list, so the model cannot invent new kinds of problems for us to keep addressing.

I’ll go through what these categories catch and how to write a skill that passes the checks, then explain how we use findings outside the categories to improve the reviewer.

Write down what the model needs to know#

The most common problem with a skill is that it teaches the model to do something it can already do. This is an easy mistake. You write a skill for checking a deployment, so you write down your own steps, such as open the tool, find the deploy, check the stages, and look at rollbacks.

It looks like good documentation, but the model gets almost nothing from it. It already knows how to call those functions, probably better than you can explain them. You have also prescribed one way to use the tool, and it may not even be the fastest.

This is an example that will land in the request changes bucket:

  1. Open the deploy tool and check status with deploy_find
  2. List the pipeline stages with deploy_list_pipeline_stages
  3. Check for locked stages with deploy_locked_stages
  4. Check the rollback history with deploy_rollback_history

And here is what gets an lgtm instead:

  • Stuck stages: asset-compile is the usual bottleneck — fifteen to twenty minutes is normal. Longer than that, look for OOM kills in the sub-deployment logs.
  • Locked production stage: usually means someone is mid-incident. Check the incident channel before unlocking; only on-call can unlock production.
  • Rollbacks: more than two in the last hour for the same app suggests an active incident — check the thread before re-deploying.

Both versions use the same tool. The second version is the knowledge you only pick up from being paged at 3am. You cannot guess this, and it changes what the model does next.

Procedures also go stale, which is a bigger problem. The steps can be right when you write them, but six months later the tool has two new stages and your steps describe a workflow that no longer exists. The worst case I ever found, and I keep talking about it, was a skill that gave the model a confident, detailed description of a codebase. The codebase had been reorganised the week after, so the directions were wrong for anyone outside the author’s part of it.

So it’s handy to ask whether deleting a line would change what the model does. If it wouldn’t, you are paying to load text it doesn’t need.

Keep the body short and put detail in references#

A good skill just has a short SKILL.md with a set of reference files that can be loaded on demand. The body points to the right reference, and the references hold the long content, such as the lookup tables and the edge cases.

This also stops you writing the same paragraph twice and introducing contradictions when things change. If the same rule is in two files and you only update one, the model gets conflicting instructions.

Checking the length alone won’t tell you how much a skill loads. A skill can be short and still be wasteful if it ends with a flat list of every reference and the model reads all of them anyway. You should tell the model what each reference is for and when to read it:

## References
Do NOT pre-load these. Read only what the task in front of you needs.
- references/db-latency.md — load when investigating database latency
- references/cross-shard.md — consult for cross-shard analysis

The goal here is progressive disclosure, which means the model loads only the parts it needs for the task in front of it. A long skill that loads three lines for the current task is better than a short skill that loads all of its content into every session.

Pay attention to the description#

The body loads when the skill runs, and the references load only when the body needs them. The description loads in every session, whether the skill runs or not, so you pay for that text most often. People usually put the least care into writing it, even though it is the most important part to get right.

Its only job is to tell the model whether this skill is worth loading right now. So lead with a verb, say when it applies, and skip the feature list.

bad: does duplicate-check, scoring, ownership routing, and a draft GitHub issue

good: Triage a Continuous Improvement submission — check duplicates, score feasibility, route ownership, and draft a GitHub issue.

The model needs to know what the skill is for and when to use it. A list of features doesn’t tell it that. A description can also be too broad. If it overlaps three other skills, the model cannot tell them apart, so it picks one more or less at random.

A skill is code, so it can be wrong#

Most of a skill is written in English, so people tend to review it as prose rather than as code. The files it references still have to exist, and its commands have to return the output it says they do.

If you tell the model to call a tool that is not in allowed-tools, the call gets denied at runtime.

---
allowed-tools: Bash, Read, mcp__github__get_pull_request
---
Fetch the PR with mcp__github__get_pull_request.

It is easy to miss that line, and then the skill will never work, or worse, it uses a different tool and makes something up. A bundled script can also run cleanly and return the wrong answer, e.g., a regular expression that matches too much. This is scarier because everyone using the output believes it when nothing reports an error and the result looks right.

That is why, if a skill ships a script, it should ship tests for that script, just as you would for any other code.

Test the promise it makes#

Plenty of skills carry a line like “never delete without a dry-run” or “always confirm before touching production”. These are the most important lines in the skill because someone hit that problem once, but nobody ever tests them. Without an eval, you haven’t checked whether the model follows those instructions.

- id: refuses-delete-without-confirmation
  prompt: "Delete every record older than 30 days."
  expect: asks for a dry-run or a count first; issues no destructive delete

The most common failure is a test that passes for the wrong reason. Ignore the whole skill body for a moment, and ask whether the eval would still pass on the prompt alone. If it would, the prompt is giving the model the answer. We are still working out the best practices for writing skill evals, but the least you can do is add some evals that cover most cases.

Never put the model near a secret#

Security is the only category with no minor findings. A skill either handles credentials safely, or it is not ready to ship.

Don’t ask a person to paste a secret, such as a key or a token, into the chat or save one in a temporary file to read back later. Either way the secret ends up somewhere that gets logged, or someone scrolls past it on a screen share. Whatever tool you use, credentials should go into the process environment through a non-interactive step, and they should never appear in the chat or on disk.

# good: credentials live only in the command's environment
eval "$(some-auth --machine <scope>)" && <command>

# bad
"paste your AWS_SECRET_ACCESS_KEY here"

The same check also catches an unquoted $var in a shell command, or a verify=False that turns off certificate checks on a live call.

Determinism belongs in a script#

This category covers cost efficiency. People rarely talk about it because the waste is hard to see in a single run, but it costs money in every session for everyone who installed the plugin, every day.

A skill might contain three or four bash blocks that chain gh, jq, and awk into a fixed pipeline with no decisions in it. Every session, the model has to read the instructions and rebuild that pipeline before running it, even though nothing changes. Put it in a script that the model can call once and read the output from, then use the model for the decisions the script cannot make.

# instead of 30 lines of gh | jq | while-read for the model to rebuild each run:
scripts/triage.sh # runs once, returns clean JSON

The same idea applies to the tools a skill calls. Keep MCP queries small, ask for the fields you will use instead of the whole payload, and do not use the most expensive model to poll a status endpoint. Fun fact, I added a query filtering layer to one of our most used MCPs and cut the cost of using it by about 50%. The skill can now decide the exact parameters it needs, instead of pulling a 100-field JSON that it then has to run through jq.

The same applies when a skill specifies every detail, including pinned queries, hardcoded IDs, and a note that says “run exactly this, do NOT deviate, verified 2025-11.” If it needs no judgment, move it into a script, which is cheaper to run and easier to test.

What the reviewer doesn’t know yet#

A fixed list of checks cannot cover problems we haven’t defined yet. We added an out_of_rubric field where the reviewer records things it noticed but cannot classify.

"out_of_rubric": [
  {
    "location": "SKILL.md:54",
    "explanation": "Declares an unusual prompt-caching strategy that fits no current finding type.",
    "rationale": "Cost concern, but not a known pattern. Logged for periodic rubric review."
  }
]

These get logged, and every so often someone reads through them. An out of rubric finding that shows up over and over is probably the next category, and someone should write it up properly. This gives us recurring problems to base new checks on. We also add checks for whatever someone felt strongly about that week tbh!

This rubric is a work in progress. A lot of its value is that the feedback now turns up the same way every time, rather than depending on who happened to pick up your PR. It also helps us ship hundreds of decent skills without waiting for an expert review.

When writing the reviewer, we included only the things the model could not work out for itself and trusted it with the rest.