Add skill-review skill - #14
peterlaczkorailsware wants to merge 3 commits into
Conversation
A repo-level utility for reviewing skills before merge. It carries the eleven defects this repo has actually shipped — asserted units, dead ends that are not dead ends, fixes the assistant cannot perform, non-additive metrics aggregated, internal contradictions, claims recalled rather than verified, redundant frontmatter in the body, convention drift, unresolved cross-references, registration and merge mechanics, and inert additions — each with the check that catches it. The rule underneath all of them is verify, never recall, so the skill leads with a sources-of-truth table in priority order: the live MCP over the connector config, the config over the knowledge base, the knowledge base over the skill under review. It updates itself. A new defect qualifies only if it shipped, changes the model's output, and comes from a pattern that will recur. Additions go through a PR against this file rather than a silent edit mid-review, and entries are removed when their cause is gone. Registered under coupler-utilities and listed in the README. The root allowlist in .gitignore gains an entry, since the repo ignores everything at root by default. The manifest version is deliberately not bumped: PRs #12 and #13 are both bumping it, and a third identical bump would merge without a conflict and be silently lost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The README had no contributing guidance at all — it ended at Installation — so a table row was the only place skill-review appeared, and nobody adding a skill would have seen it. The new section leads with running skill-review before opening a PR, then covers the conventions its checks enforce: placement, the frontmatter rules, manifest and README wiring, the .gitignore root allowlist, and the version-bump collision between concurrent PRs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # README.md
| | What can a dataflow do — sources, transformations, joins? | `coupler-io-knowledge-base/coupler-product.md` | | ||
| | Known traps for this connector | `coupler-io-knowledge-base/sources/category/<area>/<connector>/` | | ||
| | Default column labels, formats and visibility | `coupler-io-web/config/integrations/sources/<slug>/transformations/` | | ||
| | Platform behaviour (budgets, attribution, metric definitions) | The platform's own docs, fetched and read. Never memory | |
There was a problem hiding this comment.
when do you plan to invoke the skill? our agent has no access to web, so it will not be able to check it
There was a problem hiding this comment.
It is not for our agent but for people reviewing skills submitted to this repo. This is how the PR description starts:
A repo-level skill for reviewing other skills, in skill-review/. It encodes the defects found while reviewing #9, #12 and #13 — the ones that shipped, not hypotheticals — so the next reviewer starts from the list instead of rediscovering it.
|
|
||
| The opposite failure, and the one a thorough reviewer commits. A true fact that changes nothing the model does is not worth a line. These files run to hundreds of lines and every line competes for attention, so padding them with correct-but-unused detail makes the load-bearing instructions harder to find. | ||
|
|
||
| Before proposing an addition, ask what output changes if it is absent. If the answer is nothing, drop it. |
There was a problem hiding this comment.
just like this line 102 is not needed as line 104 describes everything in short
|
|
||
| ### 2. Dead ends that are not dead ends | ||
|
|
||
| "That section is dead." "Adding a report type will not fix it, that needs a change upstream." "Skip the section and say why." |
There was a problem hiding this comment.
this is very specific, I guess the description below should suffice
|
|
||
| ### 3. Fixes the assistant cannot perform | ||
|
|
||
| The mirror image of defect 2, and easier to miss because the text reads helpfully. A skill that says "unhide the column in the dataset step" is naming an action no MCP tool exposes. A skill that says "add the audience dimension" on a connector whose dimensions sit in `mcp_excluded_params` is naming an action the assistant cannot take. |
There was a problem hiding this comment.
it's a bad practice to have crossreferences the same file. in the future, this might not be defect 2. also, first sentence doesn't add to the sense. and again, examples are too specific
|
|
||
| ### 4. Non-additive metrics aggregated | ||
|
|
||
| Ratio metrics and de-duplicated counts cannot be summed or averaged across rows: |
There was a problem hiding this comment.
this might not be a full list, and there's no indication about this
| | `name` | Exactly the folder slug, which is also how siblings cross-reference it | | ||
| | `metadata.category` | One of the categories already in `skills-index.json`, not a new invention | | ||
| | `metadata.sources` | The integration's `name` from `get-integration`, verbatim, or `[]` for connector-agnostic skills | | ||
| | `metadata.version` | `1.0.0` for a new skill. Do not adopt a sibling's version to look consistent | |
There was a problem hiding this comment.
what happens to the existing skill? there's no sign of version guidance
|
|
||
| ## Keeping this skill current | ||
|
|
||
| **This skill updates itself.** It is the exception to the "cannot modify itself" rule the domain skills carry, because its whole value is being the current list of defects rather than a snapshot. |
There was a problem hiding this comment.
can this skill actually modify itself?
There was a problem hiding this comment.
This is for people reviewing the skill submissions, so of course they can modify this by submitting PRs to update this skill.
| 2. **It changes output.** Following the skill produces a wrong or unactionable answer. Cosmetic inconsistencies do not qualify. | ||
| 3. **It will recur.** The defect comes from a pattern — copied structure, an assumed platform fact, a Coupler behaviour that is easy to guess wrong — rather than a one-off slip. | ||
|
|
||
| To add one: write it in the same shape as the existing entries — the wrong pattern in the skill's own words, why it matters, and the check that catches it. Cite where it was found. Then open a PR against this file; **never rewrite it silently in the middle of another review**, because the reviewer needs to see the list change. |
There was a problem hiding this comment.
how we will run this? you say it's supposed to open a PR
|
|
||
| To add one: write it in the same shape as the existing entries — the wrong pattern in the skill's own words, why it matters, and the check that catches it. Cite where it was found. Then open a PR against this file; **never rewrite it silently in the middle of another review**, because the reviewer needs to see the list change. | ||
|
|
||
| Remove an entry when the cause is gone — a connector gains a parameter, an `mcp_excluded_params` entry is lifted, a platform behaviour changes. A stale defect wastes a check on every future review, which is defect 11 applied to this file. |
There was a problem hiding this comment.
how AI can check this?
| - Verify or say you did not. Never present an unsourced claim as checked. | ||
| - Quote the evidence with its file and line, or the tool call that produced it. A reviewer's assertion is worth no more than the skill author's. | ||
| - When the author pushes back, re-read before defending. Two findings in the review that produced this skill were withdrawn under questioning, and both withdrawals were correct. | ||
| - Review the skill that exists, not the one you would have written. |
There was a problem hiding this comment.
overall it feels overloaded with detail and not clear how to run and when. I'll be checking more skills as I have some time now, so I might suggest another skill or additions to this one
| @@ -0,0 +1,140 @@ | |||
| --- | |||
There was a problem hiding this comment.
we can follow the common repo setup when system skills, agents and commands are stored in the .claude or .agents folders.
there is an example from posthog
https://github.com/PostHog/posthog/tree/master/.agents/skills
There was a problem hiding this comment.
that would allow us to exclude this whole directory from the skills-index.json and use for internal skills
otherwise skill-review would end up in the product, which might not be that useful
or do you mean to restructure the whole repo instead for the skills to be included in the product?
There was a problem hiding this comment.
Discussed on standup. Agreed to have skill-review as an internal skill. To be moved to .agents/skills.
4 participants
What
A repo-level skill for reviewing other skills, in
skill-review/. It encodes the defects found while reviewing #9, #12 and #13 — the ones that shipped, not hypotheticals — so the next reviewer starts from the list instead of rediscovering it.The eleven defects
Each entry gives the wrong pattern in the skill's own words, why it matters, and the check that catches it.
[Source tag: X]duplicatingmetadata.sourcesnamevs folder slug, category, sources, versionThe rule underneath them
Verify, never recall. The skill leads with a sources-of-truth table in priority order — the live MCP over the connector config, the config over the knowledge base, the knowledge base over the skill under review — and says to record it when two disagree, because a KB page contradicting the product is its own bug.
Defect 3 in particular turns on
mcp_excluded_paramsin the connector config, which is the precise record of what the assistant can and cannot set.llm_instructionsis broader and advisory; reading it as the constraint produced one wrong finding in the review this came from.Self-updating
The skill is the exception to the "cannot modify itself" rule the domain skills carry, since its value is being current rather than a snapshot. A new defect qualifies only if it shipped, changes the model's output, and comes from a pattern that will recur. Additions go through a PR against the file, never a silent edit mid-review, and entries are removed when their cause is gone.
Notes
.gitignoregains!/skill-review/. The repo ignores everything at root and allowlists specific directories, so a root-level skill needs the entry. If you would rather keep the root to the ICP-first structure the allowlist comment describes,utilities/skill-review/needs no.gitignorechange and is equally runnable — say the word and I will move it.coupler-utilities, listed in the README utilities table and the install table.skill-reviewbefore opening a PR, then covers placement, frontmatter rules, manifest and README wiring, the.gitignoreroot allowlist, and the version-bump collision.skills-index.jsonleft to the generator. Ran it to check:path: skill-review,category: utilities, 21 skills.mainin a scratch worktree: clean alongside Add Meta / Facebook Ads skill pack (11 skills) #12 and Add TikTok Ads skill pack (11 skills) #13. Against Correct Google Ads connector claims, add google-ads-custom-gaql #11 there is a one-line README conflict, the same adjacency Correct Google Ads connector claims, add google-ads-custom-gaql #11 already has with Add Meta / Facebook Ads skill pack (11 skills) #12 and Add TikTok Ads skill pack (11 skills) #13 — both edit rows in the install table.🤖 Generated with Claude Code