mirror of
https://github.com/trailofbits/skills.git
synced 2026-09-14 14:28:48 +08:00
Move the contribution checklist into machinery (#206)
* Add validator self-test, structural checks, and make check The repo documented ~53 rules in AGENTS.md and machine-enforced 6 of them. This closes the gap for the ones a machine can decide, and adds the guard that keeps the checkers honest. New error-level checks (all currently pass, so none of this blocks anyone today): agent files must use `tools:` while skills use `allowed-tools:` (the loader silently ignores the wrong key, so the restriction just does not apply); subagent_type must be namespaced or the dispatch fails at runtime; plugin dir names kebab-case and <=64 chars; plugin README present, listed rather than stat'd so `Readme.md` fails on Linux CI the way it should; semver format; the forbidden runtime sidecars AGENTS.md already banned but nothing checked; and version-increment against the base branch, which is the gap that letdebfb29ship an allowed-tools fix across 25 plugins that no installed user received. New warning-level checks, reported but not blocking: the two required SKILL.md sections, the 500-line limit, and unresolved relative references. 55 warnings across 40 plugins today, concentrated in testing-handbook-skills and building-secure-contracts. The point of the exercise is the three anti-vacuity guards. A checker that has silently stopped matching reports a clean repo forever, and that failure mode has shipped repeatedly: `--self-test` builds fixtures and asserts every checker rejects a known-bad one; a full scan that resolves zero references exits 1 rather than declaring everything clean; and SELF_TEST_MINIMUM fails the self-test if it runs fewer than 20 assertions, because the self-test is itself a checker. It currently runs 26. The reference extractor skips fenced and inline code so that skill-authoring docs citing example paths do not generate warnings nobody reads. Makefile mirrors CI as one `make check`. Its RUFF_VERSION must match the ruff-pre-commit rev, and the self-test asserts that — verified by breaking it. * Fix CI checks that could pass or fail without inspecting anything Four of these are the same bug in different places: a check whose empty case is indistinguishable from success. - python-tests ran `python3 <file>` per file. A test file with no `if __name__ == "__main__"` block exits 0 having run nothing, which reads as a pass. All 10 current files happen to comply; nothing enforced it. Now pytest per directory, run via `python3 -m` from inside each one to preserve the sibling imports these suites rely on, with --import-mode=importlib because c-review and rust-review both ship scripts/test_split.py and the default import mode collides on the basename. 278 tests now run where the count was previously unknown. - bats used --no-run-if-empty, so a broken glob was a silent pass. This repo ships bats suites; finding none is a failure. - The SKILL.md frontmatter walk printed "All 0 SKILL.md files have valid frontmatter" if discovery broke. Now fails on zero. - zeroize-audit's shell regression suites matched no CI glob and had never run. Also: the validator self-test runs first, before validation. The hardcoded-path grep now covers .sh, .bats, .yml and .toml — test fixtures and install scripts are where absolute paths hide. The personal-email exclusion is anchored; an unanchored '.git' also dropped any line containing '/github'. The two npm CLI installs are pinned rather than @latest, which zizmor flagged and which made CI able to break with no commit. check_codex_loadability.py: tempdir cleanup raced the codex app-server and threw `OSError: [Errno 39] Directory not empty: '.git'`, failing the job after every loadability check had passed (seen on PR #148 today). A teardown race must not be reported as a validation failure. * Wire validators into pre-commit; fix two dead Dependabot entries pre-commit: the three .github/scripts validators were CI-only, so the first signal a contributor got was a red check after pushing — AGENTS.md asked people to remember to run them by hand. Now they run locally, with the validator self-test scoped to fire only when the validator itself changes. Adds actionlint, zizmor, check-toml, check-merge-conflict, and detect-private-key. detect-private-key earns its place: there is no secret scanning here at all, which is how a live API key sat in an untracked config file in a working tree this morning without anything noticing. Markdown linting is deliberately absent, with the measurement recorded in the config so the next person does not have to redo it: markdownlint reports ~12,400 violations across this repo (7,282 MD013 line-length alone). It would land either permanently red or with so many rules disabled that it checks nothing. Dependabot had two defects that made it quieter than it looked. The `pip` entry at / had no manifest to resolve — there is no root pyproject.toml and uv.lock is gitignored — so it reported nothing, indefinitely. And two script directories with real dependencies were uncovered: trailmark's slicing-code-context (trailmark>=0.5,<0.6) and yara-authoring's rule scripts (yara-x>=0.10.0). Switched to the uv ecosystem per the house standard, grouped minor/patch so majors still get their own CI run. * Add automated PR review, inert until a key is configured Two tiers: fast (effort low, every push, sticky comment) and deep (effort xhigh, on a deep-review label). Per-job permissions with an empty workflow-level grant; concurrency keyed per tier so a routine push cannot cancel an in-flight deep review — GitHub will not re-fire `labeled` for a label already present, so that would leave a PR sitting labeled with no review, looking reviewed. The prompt is the substance. It forbids pre-filtering: current models follow "only report high-severity issues" literally, investigating fully and then declining to report what they judge below the bar, which reads as a capability regression but is a prompt bug. It asks for everything ranked P1-P4 with a concrete failure scenario each, and filters downstream. It also names the five defect classes that actually reach main in a repo of markdown that instructs a model, rather than asking for generic code review. There is no ANTHROPIC_API_KEY secret on this repo, so every review step is gated on the secret being present. Until someone adds it these workflows check out the code and do nothing — they do not fail. A review workflow that goes red on every PR for want of a credential teaches people to ignore red checks. Fork review is a separate file with a separate decision attached. It needs pull_request_target, because under `pull_request` a fork PR gets a read-only token and the job could not post its comment at all. The usual exploit path is closed by a maintainer-only label gate, a checkout pinned to the SHA as of the labeling event (so a later force-push does not change what is reviewed), and a tool allowlist with no general Bash, so fork code is read and never executed. The residual prompt-injection risk is documented in the file header. Deleting that one file costs nothing but fork review. * Replace the PR checklist with make check Deletes the 20-item PR checklist. Current model guidance is explicit that verification scaffolding of that shape degrades output rather than improving it, and that removing it costs no capability — so the response is to move each check into machinery, not to restate it louder. Every item that could be mechanized landed in the validator first, with a fixture, before this prose was allowed to go. What replaces the checklist is two lists: what the validator enforces so you do not have to, and what no tool can check. The second list is the one that matters — whether the description actually triggers, whether the version bump is the right size, whether the README row is in the right section (the validator only checks the plugin appears somewhere in that file). Also corrects the frontmatter example, which showed `allowed-tools` as a YAML list. Every SKILL.md in the repo uses the space-delimited form;debfb29converted them deliberately. The doc was the outlier, and it caused a false finding during review of #192. Agent files genuinely do use a YAML list, under the `tools` key — both forms are now shown side by side with the reason the distinction matters. Two new sections. "Scripts a plugin ships" leads with the rule this whole change is built around: a checker that inspects zero items must fail, not pass, with the three worked examples. "Working effectively in this repo" covers effort sweeps, a subagent cap (current models over-delegate, which is a reversal from the previous generation), not adding verification scaffolding to prompts, and not telling a reviewer to pre-filter — that last one reads as a capability regression and is a prompt bug. Scopes the claude-code-guide line to questions that cannot be answered by reading this repo, and adds .opencode/ to the banned sidecar list now that the validator enforces it. * Fix three bugs CI caught in the guardrails themselves All three were mine, and the first is the one that mattered. 1. The version-increment check ran against every plugin, not just the ones the branch touched — so it demanded a version bump from all 42 plugins on a PR that changed no plugin at all. Now scoped to plugins with file changes between the base ref and HEAD. Added three self-test assertions covering it (touched plugin errors, untouched plugin does not), and verified by reverting the fix and confirming the self-test goes red. The check had no coverage before, which is exactly why it shipped broken. 2. The new pre-commit hooks invoked `uv run`, which is not installed in the Pre-commit CI job. The validator declares no dependencies, so plain python3 is correct and one less thing to install. 3. The review workflows called `claude` without installing it. Added a pinned install step, matching how validate.yml installs the same CLI. Also corrects a factual claim in both review workflow headers. I wrote that they were inert for want of an ANTHROPIC_API_KEY, based on `gh secret list` returning empty — but that only lists repository secrets. An organization-level key is visible here, so the fast tier is live on merge. The headers now say so. The deep and fork tiers still cannot fire until their labels are created. * Fix the findings from this PR's own automated review The review posted on #206 found eight issues. Seven were real. Working through them in severity order: P1 — deleted claude-review-fork.yml. It checked out the fork tree and then ran this repo's review script *from that tree*, so the script itself, and any CLAUDE.md or .claude/hooks sitting beside it, was fork-authored and executed with the org API key in the environment. My header claimed "fork code is read, never executed" and that was simply wrong; the SHA pin and the tool allowlist do not touch that path. Doing it safely means checking out base into the workspace and the fork commit into a subdirectory, which is deliberate work rather than a footnote to this change. Fork PRs get no automated review for now, and claude-review.yml explains why. P2 — the prompts instructed `gh pr comment --body-file <file>` while the tool allowlist has no Write and no general Bash, so there was nowhere to put a file. The reviewer hit this and fell back to `--body-file -`; since a give-up does not fail the step, the failure mode was a green check with no review. Now uses a heredoc on stdin. The deep tier also told the model to run the scripts a PR adds, which its allowlist cannot do; it now says so explicitly. While fixing that: both tier heredocs were unquoted so `${PR_NUMBER}` would interpolate, which means backticks in the prompt body were command substitution. Quoted them and moved substitution out to parameter expansion afterwards. shellcheck caught this. P2 — `no-version-bump` was a phantom feature: named in the validator's error message and in AGENTS.md, read by nothing. Implemented via --allow-no-bump, wired to the label through validate.yml. P2 — AGENTS.md claimed both loadability checks run in `make check`. They do not and cannot; they need two CLIs installed. It now says what runs locally, what is CI-only, and how to run the loadability checks by hand. P3 — the new shell-suites discovery reintroduced the exact bug this PR exists to remove: it printed "No shell regression suites found" and exited 0, and used `**` without globstar so it only ever matched one directory level. Now find-based and fails on empty. It is a Makefile target but not part of `make check`, because zeroize-audit's suite pipes to `python3 -` which the modern-python shim rejects — filed as #207. P3 — a failed `git diff` returned an empty changed-plugin set, silently disarming the version check for every plugin. Now raises. P3 — the `/home/user/` exclusion was a content filter applied across all of plugins/, so a skill legitimately documenting that path would be dropped from the results. Scoped to the shim bats fixtures that need it. Not fixed, deliberately: the reviewer noted `_check_ruff_parity` covers one pinned version. True, and it is the only version both files pin. * Fix the second review pass: the review job could not detect its own silence The marquee one: claude_review.sh ended with `claude --print` and nothing else, so if the model finished without calling `gh pr comment` — a denied tool, a hit timeout, or it simply summarising instead of posting — the script exited 0 and the job went green with no review attached. That is defect class 1 from the prompt this very script ships, in the script that ships it. It now timestamps before the run and fails if no comment was created or updated since. _check_ruff_parity returned None (= pass) when the Makefile or pre-commit config was missing, while still counting toward SELF_TEST_MINIMUM. A vacuous pass inside the anti-vacuity harness. Now returns an error string. The scoping claim in AGENTS.md and the Makefile was backwards. I wrote that CI scopes the validator to changed plugins while local scans everything, making local a strict superset. Both scan everything; only the version-increment check is scoped, and it is the one check CI runs that local cannot — so local is not a superset at all. Corrected in both places. README said `make check` "runs everything CI runs" and the Makefile echoed "this is what CI will run". Neither is true: it omits both loadability checks, the version-increment check, and every pre-commit hook except ruff/shellcheck/shfmt. Both now say what they actually cover. AGENTS.md already had this right. Stale count in a pre-commit comment ("these three" preceding two hooks). Accepted without change, with reasoning: the reference resolver matches any file in the plugin, so a link to a file that exists under a different skill resolves when it should not — pinning a base directory produces a flood of false positives, and the looser check is still worth having. The subagent-dispatch check only knows its own plugin's agents, so a bare cross-plugin reference is missed; catching that needs a repo-wide agent map, which is a larger change than belongs here. * Fix the third review pass, including two more silent-pass checks The hardcoded-path step had the defect this PR exists to remove. It was written as `if grep A | grep -v B | grep -v C`, so the exit status came from the last `grep -v`. A first-stage failure — plugins/ renamed, a grep built without -P — produced no output, exited 2, and the step printed "No hardcoded user paths found" while inspecting nothing. Now counts the files it scanned, fails if that is zero, and checks grep's own status via PIPESTATUS rather than the pipe's. The self-test fixture named "empty scan returns non-zero" was passing for the wrong reason: it deleted the plugin's files but left the directory, so scan_plugins_directory still returned {"demo"} and the non-zero came from the missing-README error. The guard it claimed to cover was never exercised. It now rmtree's the directory and asserts the fixture actually emptied plugins/ before relying on it. Same class of bug as everything else here, one level up. Concurrency: every `labeled` event that was not `deep-review` resolved to the `fast` key with cancel-in-progress. Adding `dependencies` mid-review cancelled the running review and then skipped both jobs, leaving a cancelled check, no review, and no event that would re-trigger one until the next push. Each label now gets its own key. The version-increment check read the old version at `base.sha` while changed_plugins diffed `base...HEAD`. Those disagree once anything lands on main after a branch forks, so a PR could fail for not out-bumping a sibling it never saw. Both halves now use the merge base, and the error message says to rebase. Smaller: the sidecar ban only matched `.agents/plugins/marketplace.json` while AGENTS.md bans `.agents/` — widened, with fixtures for it and for `plugins/*/.codex-plugin`, neither of which the harness covered. `make shell` globbed only plugins/, so the one new shell script in this PR went unchecked locally. The deep job's fork exclusion is now explicit rather than relying on secrets being unavailable to fork events. changed_plugins' docstring said working tree; it compares commits.
This commit is contained in:
+43
-36
@@ -1,44 +1,45 @@
|
||||
# Two defects fixed here, both of which made this file quieter than it looked:
|
||||
#
|
||||
# 1. There was a `pip` entry at `/` with nothing to update — there is no root
|
||||
# pyproject.toml and uv.lock is gitignored (.gitignore:18). Dependabot was
|
||||
# resolving an empty manifest and reporting nothing, indefinitely.
|
||||
# 2. Two script directories carrying real dependencies were uncovered:
|
||||
# trailmark's slicing-code-context (trailmark>=0.5,<0.6) and yara-authoring's
|
||||
# rule scripts (yara-x>=0.10.0).
|
||||
#
|
||||
# Ecosystem is `uv`, not `pip`, per the house standard. Only directories with a
|
||||
# pyproject.toml that declares dependencies are listed — c-review/scripts,
|
||||
# rust-review/scripts and let-fate-decide's scripts have manifests but no runtime
|
||||
# deps, so there is nothing for Dependabot to resolve. Add them if that changes.
|
||||
#
|
||||
# Python deps declared with PEP 723 inline metadata (the common pattern in this
|
||||
# repo, including .github/scripts/) have no Dependabot ecosystem at all and stay
|
||||
# manual. So do the pinned npm CLI versions in .github/workflows/validate.yml.
|
||||
version: 2
|
||||
|
||||
updates:
|
||||
- package-ecosystem: pip
|
||||
directory: /
|
||||
- package-ecosystem: uv
|
||||
directories:
|
||||
- /plugins/constant-time-analysis
|
||||
- /plugins/culture-index/skills/interpreting-culture-index/scripts
|
||||
- /plugins/testing-handbook-skills/scripts
|
||||
- /plugins/trailmark/skills/slicing-code-context/scripts
|
||||
- /plugins/yara-authoring/skills/yara-rule-authoring/scripts
|
||||
schedule:
|
||||
interval: weekly
|
||||
cooldown:
|
||||
default-days: 7
|
||||
# One PR per week rather than five. Majors stay separate so they get their own
|
||||
# CI run and their own read of the release notes.
|
||||
groups:
|
||||
all:
|
||||
patterns: ["*"]
|
||||
|
||||
- package-ecosystem: pip
|
||||
directory: /plugins/constant-time-analysis
|
||||
schedule:
|
||||
interval: weekly
|
||||
cooldown:
|
||||
default-days: 7
|
||||
groups:
|
||||
all:
|
||||
patterns: ["*"]
|
||||
|
||||
- package-ecosystem: pip
|
||||
directory: /plugins/culture-index/skills/interpreting-culture-index/scripts
|
||||
schedule:
|
||||
interval: weekly
|
||||
cooldown:
|
||||
default-days: 7
|
||||
groups:
|
||||
all:
|
||||
patterns: ["*"]
|
||||
|
||||
- package-ecosystem: pip
|
||||
directory: /plugins/testing-handbook-skills/scripts
|
||||
schedule:
|
||||
interval: weekly
|
||||
cooldown:
|
||||
default-days: 7
|
||||
groups:
|
||||
all:
|
||||
patterns: ["*"]
|
||||
python-minor-patch:
|
||||
update-types:
|
||||
- minor
|
||||
- patch
|
||||
commit-message:
|
||||
prefix: deps
|
||||
labels:
|
||||
- dependencies
|
||||
|
||||
- package-ecosystem: github-actions
|
||||
directory: /
|
||||
@@ -47,5 +48,11 @@ updates:
|
||||
cooldown:
|
||||
default-days: 7
|
||||
groups:
|
||||
all:
|
||||
patterns: ["*"]
|
||||
actions-minor-patch:
|
||||
update-types:
|
||||
- minor
|
||||
- patch
|
||||
commit-message:
|
||||
prefix: ci
|
||||
labels:
|
||||
- dependencies
|
||||
|
||||
@@ -115,7 +115,12 @@ def main() -> int:
|
||||
loaded_skill_count = 0
|
||||
loaded_mcp_count = 0
|
||||
request_id = 0
|
||||
tmp = tempfile.TemporaryDirectory(prefix="codex-load-check-")
|
||||
# ignore_cleanup_errors: the codex app-server can still be releasing handles under
|
||||
# the temp dir when cleanup runs, which raised
|
||||
# `OSError: [Errno 39] Directory not empty: '.git'` and failed the whole job after
|
||||
# every loadability check had already passed. A teardown race must not be reported
|
||||
# as a validation failure.
|
||||
tmp = tempfile.TemporaryDirectory(prefix="codex-load-check-", ignore_cleanup_errors=True)
|
||||
temp_root = Path(tmp.name)
|
||||
home = temp_root / "home"
|
||||
codex_home = temp_root / "codex-home"
|
||||
|
||||
Executable
+183
@@ -0,0 +1,183 @@
|
||||
#!/usr/bin/env bash
|
||||
# Post an automated review on a pull request.
|
||||
#
|
||||
# Usage: claude_review.sh <fast|deep>
|
||||
#
|
||||
# Requires ANTHROPIC_API_KEY, GH_TOKEN, PR_NUMBER, REPO in the environment. The
|
||||
# caller (claude-review.yml) gates on the API key being present, so reaching this
|
||||
# script without one is a bug, not a normal path — hence the hard check below.
|
||||
#
|
||||
# The prompts are the point of this file. Two things carry them:
|
||||
#
|
||||
# 1. They forbid pre-filtering. Current models follow "only report high-severity
|
||||
# issues" literally: they investigate just as thoroughly, find the bugs, then
|
||||
# decline to report what they judge below the bar. Precision rises and measured
|
||||
# recall falls, which reads as a capability regression but is a prompt bug.
|
||||
# Ask for everything and filter downstream.
|
||||
# 2. They name the defect classes that actually reach main in a repo of markdown
|
||||
# that instructs a model, rather than asking for generic "code review".
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
TIER="${1:?usage: claude_review.sh <fast|deep>}"
|
||||
|
||||
: "${ANTHROPIC_API_KEY:?ANTHROPIC_API_KEY is required}"
|
||||
: "${GH_TOKEN:?GH_TOKEN is required}"
|
||||
: "${PR_NUMBER:?PR_NUMBER is required}"
|
||||
: "${REPO:?REPO is required}"
|
||||
|
||||
case "$TIER" in
|
||||
fast) EFFORT="low" ;;
|
||||
deep) EFFORT="xhigh" ;;
|
||||
*)
|
||||
echo "unknown tier: $TIER (expected fast or deep)" >&2
|
||||
exit 2
|
||||
;;
|
||||
esac
|
||||
|
||||
# Shared across both tiers. Written to a file rather than interpolated into a
|
||||
# command line: PR metadata is untrusted text and must never reach a shell.
|
||||
COMMON_PROMPT=$(
|
||||
cat <<'PROMPT'
|
||||
Report every issue you find, at every severity. Do not pre-filter, do not suppress
|
||||
low-confidence findings, and do not decide something is too minor to mention. Rank
|
||||
each finding P1 to P4 and let a human filter. A finding you withheld is worth
|
||||
nothing; a P4 that turns out not to matter costs one line.
|
||||
|
||||
For each finding give: file:line, one sentence on the defect, and a concrete failure
|
||||
scenario — the input or state that produces the wrong behaviour. If you cannot state
|
||||
a failure scenario, say so and rank it lower.
|
||||
|
||||
This repository is a marketplace of Claude Code plugins. Most content is markdown
|
||||
that instructs a model, so "does this text cause correct behaviour" matters as much
|
||||
as code correctness. Weight these classes especially, because each has reached main
|
||||
in repositories like this one and none is visible on a casual read:
|
||||
|
||||
1. Verifiers that pass without verifying. A script, grader, or checklist that
|
||||
reports success while inspecting nothing. Real examples: a validator using
|
||||
`grep -oP` (unsupported by BSD grep) with stderr sent to /dev/null, so it always
|
||||
printed "valid"; a grader that judged the response text, so a run that skipped
|
||||
the real work still passed; a citation gate that only validated citations that
|
||||
were present, so zero citations passed. For any check in the diff, ask what it
|
||||
does when it finds nothing, and whether that is distinguishable from success.
|
||||
2. Agent wiring. `subagent_type` must be `<plugin>:<agent>`; a bare name is
|
||||
unregistered and the dispatch fails at runtime. Agent frontmatter declares tools
|
||||
with `tools:`, skills with `allowed-tools:` — the keys are inverted between the
|
||||
two file types and the wrong one is silently ignored, so the restriction simply
|
||||
does not apply.
|
||||
3. Generated artifacts. Skills that emit HTML must escape anything derived from the
|
||||
target codebase before it reaches innerHTML; these artifacts ship to clients and
|
||||
the codebase under audit is untrusted input. Flag external CDN or script loads in
|
||||
anything described as self-contained.
|
||||
4. Instructions that cannot work as written. Documented commands that exceed a rate
|
||||
limit, reference a skill or binary that is not installed, or depend on a path with
|
||||
no stated provenance. Check the numbers when a doc claims a cost or a limit.
|
||||
5. Silent truncation. Degraded output that is indistinguishable from a clean empty
|
||||
result in the artifact a human actually reads, especially when the warning goes
|
||||
only to stderr.
|
||||
|
||||
Say plainly when a dimension is clean rather than manufacturing a finding to look
|
||||
thorough. If the diff is small or purely editorial, a short review is correct output.
|
||||
PROMPT
|
||||
)
|
||||
|
||||
if [ "$TIER" = "fast" ]; then
|
||||
PROMPT=$(
|
||||
cat <<'PROMPT'
|
||||
Review pull request #__PR__ in __REPO__ against the diff with the base branch.
|
||||
|
||||
When you are done you MUST post your review with:
|
||||
|
||||
gh pr comment __PR__ --body-file - --edit-last --create-if-none <<'EOF'
|
||||
...your review...
|
||||
EOF
|
||||
|
||||
Nothing you write outside that comment is visible to anyone. A run that finishes
|
||||
without posting shows up as a green check with no review attached, which reads as
|
||||
"reviewed and clean" — the worst possible outcome. Post even when you found nothing,
|
||||
and say so.
|
||||
|
||||
Read from stdin, not a temp file: you have no Write tool and no general Bash, so
|
||||
there is nowhere to put one.
|
||||
|
||||
`--edit-last --create-if-none` replaces your previous review rather than appending.
|
||||
This job runs on every push, so appending would leave a reader scrolling past stale
|
||||
findings from commits that were fixed several pushes ago.
|
||||
|
||||
__COMMON__
|
||||
PROMPT
|
||||
)
|
||||
ALLOWED_TOOLS='Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Read,Grep,Glob'
|
||||
else
|
||||
PROMPT=$(
|
||||
cat <<'PROMPT'
|
||||
Perform a deep adversarial review of pull request #__PR__ in __REPO__.
|
||||
|
||||
When you are done you MUST post your review with:
|
||||
|
||||
gh pr comment __PR__ --body-file - <<'EOF'
|
||||
...your review...
|
||||
EOF
|
||||
|
||||
Nothing you write outside that comment reaches anyone.
|
||||
|
||||
Go beyond the diff where the diff depends on it: read the files it touches, read the
|
||||
scripts it adds, and check its claims against the repository rather than taking them
|
||||
at face value. When the PR states a number, a limit, or a cost, verify it against the
|
||||
files — count the things it claims to count. When it adds a check, work out by reading
|
||||
it what input would slip past, and say so.
|
||||
|
||||
You cannot execute anything: your tools are `gh pr` reads, `git log`, `git diff`,
|
||||
`Read`, `Grep`, and `Glob`. Do not plan around running code.
|
||||
|
||||
Prioritise, in order: anything that makes the plugin fail to run at all; anything
|
||||
that produces a wrong result while reporting success; anything that puts untrusted
|
||||
content into an artifact shared outside the company; and anything whose documented
|
||||
usage does not work as written.
|
||||
|
||||
__COMMON__
|
||||
|
||||
Finish with an explicit list of what you checked and found clean, so a reader can
|
||||
tell the difference between a dimension you cleared and one you never looked at.
|
||||
PROMPT
|
||||
)
|
||||
ALLOWED_TOOLS='Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(git log:*),Bash(git diff:*),Read,Grep,Glob'
|
||||
fi
|
||||
|
||||
# Substituted here rather than interpolated inside the heredocs. Both tier
|
||||
# heredocs are quoted so that backticks and $(...) in the prompt text are literal
|
||||
# rather than commands the shell runs while building the prompt.
|
||||
PROMPT="${PROMPT//__COMMON__/$COMMON_PROMPT}"
|
||||
PROMPT="${PROMPT//__PR__/$PR_NUMBER}"
|
||||
PROMPT="${PROMPT//__REPO__/$REPO}"
|
||||
|
||||
PROMPT_FILE="$(mktemp)"
|
||||
trap 'rm -f "$PROMPT_FILE"' EXIT
|
||||
printf '%s\n' "$PROMPT" >"$PROMPT_FILE"
|
||||
|
||||
# Timestamp before the run so we can tell a review that was posted from one that
|
||||
# was not. Without this the script has the exact defect its own prompt hunts: if the
|
||||
# model finishes without calling `gh pr comment` — a denied tool, a hit timeout, or
|
||||
# it simply summarises instead of posting — `claude` exits 0, the job goes green, and
|
||||
# a reader sees a passing "Claude review" check and infers the diff was reviewed.
|
||||
STARTED_AT="$(date -u +%Y-%m-%dT%H:%M:%SZ)"
|
||||
|
||||
echo "Running $TIER review (effort=$EFFORT) on $REPO#$PR_NUMBER"
|
||||
claude --print \
|
||||
--effort "$EFFORT" \
|
||||
--allowedTools "$ALLOWED_TOOLS" \
|
||||
<"$PROMPT_FILE"
|
||||
|
||||
posted="$(
|
||||
gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" \
|
||||
--jq "[.[] | select(.created_at >= \"${STARTED_AT}\" or .updated_at >= \"${STARTED_AT}\")] | length"
|
||||
)"
|
||||
|
||||
if [ "${posted:-0}" -eq 0 ]; then
|
||||
echo "ERROR: the review run finished without posting a comment." >&2
|
||||
echo "A green check with no review attached reads as 'reviewed and clean'," >&2
|
||||
echo "which is worse than no check at all. Failing instead." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "Review posted ($posted comment(s) created or updated since $STARTED_AT)."
|
||||
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,140 @@
|
||||
# Automated review on pull requests, in two tiers.
|
||||
#
|
||||
# An organization-level ANTHROPIC_API_KEY is visible to this repository, so this
|
||||
# workflow is LIVE the moment it merges — it will start reviewing PRs immediately.
|
||||
# (It is not listed by `gh secret list` at the repo level, which is what made this
|
||||
# easy to get wrong.)
|
||||
#
|
||||
# Every review step is still gated on the key being present, so if the org secret is
|
||||
# ever removed these jobs go quiet rather than red. A review workflow that fails on
|
||||
# every PR for want of a credential teaches people to ignore red checks.
|
||||
#
|
||||
# The deep tier needs a label that does not exist yet:
|
||||
# gh label create deep-review -d "Run the xhigh adversarial review pass" -c B60205
|
||||
#
|
||||
# Fork PRs get NO automated review. A `pull_request_target` variant was written and
|
||||
# then deleted: it checked out the fork tree and ran this repo's review script from
|
||||
# it, so the script itself — and any CLAUDE.md or .claude/hooks beside it — was
|
||||
# fork-authored and executed with the org API key in the environment. The SHA pin
|
||||
# and the tool allowlist did not touch that path. Doing it safely means checking
|
||||
# out base into the workspace and the fork commit into a subdirectory, running
|
||||
# everything from base; that is a deliberate piece of work, not a footnote to this
|
||||
# change.
|
||||
name: Claude Review
|
||||
|
||||
on:
|
||||
pull_request:
|
||||
# ready_for_review is load-bearing: the fast job skips drafts, so without it a
|
||||
# draft-then-ready PR gets no event and no review at all.
|
||||
types: [opened, synchronize, reopened, ready_for_review, labeled]
|
||||
|
||||
# Granted per job rather than here. A workflow-level write grant applies to every job
|
||||
# including ones that never need it, which zizmor flags as excessive-permissions.
|
||||
permissions: {}
|
||||
|
||||
concurrency:
|
||||
# Keyed per tier, not per PR. Sharing one slot lets a routine push cancel an
|
||||
# in-flight deep review — and GitHub will not re-fire `labeled` for a label that is
|
||||
# already present, so the PR would sit labeled with no review, looking reviewed.
|
||||
#
|
||||
# Every `labeled` event gets its own key, including labels neither job cares about.
|
||||
# Otherwise adding `dependencies` mid-review resolves to the `fast` key, cancels the
|
||||
# running review, and then skips both jobs — leaving a cancelled check, no review,
|
||||
# and no event that would re-trigger one until the next push.
|
||||
group: >-
|
||||
${{ github.workflow }}-${{ github.event.pull_request.number }}-${{
|
||||
github.event.action == 'labeled'
|
||||
&& format('label-{0}', github.event.label.name)
|
||||
|| 'push' }}
|
||||
cancel-in-progress: true
|
||||
|
||||
jobs:
|
||||
fast:
|
||||
name: Claude review (fast)
|
||||
# Skips: label events (that is the deep job's trigger), drafts, bots, and forks.
|
||||
# Forks are excluded: under `pull_request` they get a read-only token and could
|
||||
# not post a comment anyway, and the privileged alternative is unsafe (above).
|
||||
if: >-
|
||||
github.event.action != 'labeled' &&
|
||||
github.event.pull_request.draft == false &&
|
||||
github.event.pull_request.user.type != 'Bot' &&
|
||||
github.event.pull_request.head.repo.fork == false
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 20
|
||||
permissions:
|
||||
# read: check out the diff under review.
|
||||
contents: read
|
||||
# write: post the review as a PR comment. This is the whole point of the job —
|
||||
# a run that cannot comment produces a green check and no review, which reads
|
||||
# as "reviewed and clean".
|
||||
pull-requests: write
|
||||
steps:
|
||||
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
|
||||
with:
|
||||
fetch-depth: 1
|
||||
persist-credentials: false
|
||||
- name: Install Claude Code CLI # zizmor: ignore[adhoc-packages]
|
||||
if: env.ANTHROPIC_API_KEY != ''
|
||||
env:
|
||||
ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}
|
||||
CLAUDE_CLI_VERSION: "2.1.220"
|
||||
run: |
|
||||
npm install --global --prefix "$RUNNER_TEMP/claude-cli" \
|
||||
"@anthropic-ai/claude-code@${CLAUDE_CLI_VERSION}"
|
||||
|
||||
- name: Review
|
||||
if: env.ANTHROPIC_API_KEY != ''
|
||||
env:
|
||||
ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}
|
||||
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
|
||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||
REPO: ${{ github.repository }}
|
||||
run: |
|
||||
export PATH="$RUNNER_TEMP/claude-cli/bin:$PATH"
|
||||
.github/scripts/claude_review.sh fast
|
||||
|
||||
deep:
|
||||
name: Claude review (deep)
|
||||
# The fork exclusion is explicit rather than relying on secrets being
|
||||
# unavailable to fork `pull_request` events, so changing the gating later cannot
|
||||
# silently start running this against a fork with a read-only token.
|
||||
if: >-
|
||||
github.event.action == 'labeled' &&
|
||||
github.event.label.name == 'deep-review' &&
|
||||
github.event.pull_request.user.type != 'Bot' &&
|
||||
github.event.pull_request.head.repo.fork == false
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 30
|
||||
permissions:
|
||||
# read: check out the diff under review.
|
||||
contents: read
|
||||
# write: post the review as a PR comment. This is the whole point of the job —
|
||||
# a run that cannot comment produces a green check and no review, which reads
|
||||
# as "reviewed and clean".
|
||||
pull-requests: write
|
||||
steps:
|
||||
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
|
||||
with:
|
||||
# Needs history to diff against origin/main. At depth 1 the diff comes back
|
||||
# empty, which is indistinguishable from "nothing to find".
|
||||
fetch-depth: 0
|
||||
persist-credentials: false
|
||||
- name: Install Claude Code CLI # zizmor: ignore[adhoc-packages]
|
||||
if: env.ANTHROPIC_API_KEY != ''
|
||||
env:
|
||||
ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}
|
||||
CLAUDE_CLI_VERSION: "2.1.220"
|
||||
run: |
|
||||
npm install --global --prefix "$RUNNER_TEMP/claude-cli" \
|
||||
"@anthropic-ai/claude-code@${CLAUDE_CLI_VERSION}"
|
||||
|
||||
- name: Review
|
||||
if: env.ANTHROPIC_API_KEY != ''
|
||||
env:
|
||||
ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}
|
||||
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
|
||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||
REPO: ${{ github.repository }}
|
||||
run: |
|
||||
export PATH="$RUNNER_TEMP/claude-cli/bin:$PATH"
|
||||
.github/scripts/claude_review.sh deep
|
||||
+53
-10
@@ -40,7 +40,38 @@ jobs:
|
||||
curl -LsSf https://astral.sh/uv/install.sh | sh
|
||||
echo "$HOME/.local/bin" >> "$GITHUB_PATH"
|
||||
- name: Run bats tests
|
||||
run: find plugins -name "*.bats" -type f -print0 | xargs -0 --no-run-if-empty bats
|
||||
# --no-run-if-empty would turn "discovery broke" into a silent pass. This repo
|
||||
# ships bats suites, so finding none is a failure, not a clean run.
|
||||
run: |
|
||||
set -euo pipefail
|
||||
mapfile -d '' files < <(find plugins -name '*.bats' -type f -print0)
|
||||
if [ "${#files[@]}" -eq 0 ]; then
|
||||
echo "ERROR: no .bats files found — discovery is broken"
|
||||
exit 1
|
||||
fi
|
||||
echo "Running ${#files[@]} bats file(s)"
|
||||
bats "${files[@]}"
|
||||
|
||||
- name: Run shell regression suites
|
||||
# Matched by no glob in the previous discovery, so these never ran in CI.
|
||||
run: |
|
||||
# find, not a glob: `**` needs globstar and silently degrades to `*`
|
||||
# without it, so a suite one directory deeper stops running with no signal.
|
||||
# And an empty result is a failure, not a pass — this repo ships suites.
|
||||
set -euo pipefail
|
||||
mapfile -d '' suites < <(
|
||||
find plugins -type f -path '*/tests/*' -name 'run_*.sh' -print0
|
||||
)
|
||||
if [ "${#suites[@]}" -eq 0 ]; then
|
||||
echo "ERROR: no shell regression suites found — discovery is broken"
|
||||
exit 1
|
||||
fi
|
||||
echo "Running ${#suites[@]} shell regression suite(s)"
|
||||
for s in "${suites[@]}"; do
|
||||
echo "::group::$s"
|
||||
bash "$s"
|
||||
echo "::endgroup::"
|
||||
done
|
||||
|
||||
python-tests:
|
||||
name: Python tests
|
||||
@@ -73,20 +104,32 @@ jobs:
|
||||
- name: Discover and run plugin Python tests
|
||||
shell: bash
|
||||
run: |
|
||||
# Intentionally omit -e: we want every test file to run even if
|
||||
# one fails, then exit with a combined failure code via `failed`.
|
||||
# Intentionally omit -e: we want every directory to run even if one
|
||||
# fails, then exit with a combined failure code via `failed`.
|
||||
#
|
||||
# pytest per directory, not `python3 <file>` per file. A test file with no
|
||||
# `if __name__ == "__main__"` block exits 0 under the old loop having run
|
||||
# nothing at all, which is indistinguishable from a pass. Running pytest
|
||||
# from inside each directory (via `python3 -m`, which puts the CWD on
|
||||
# sys.path) preserves the sibling imports these suites rely on, and
|
||||
# --import-mode=importlib keeps c-review and rust-review from colliding on
|
||||
# their identically-named test_split.py.
|
||||
set -uo pipefail
|
||||
mapfile -d '' files < <(find plugins -type f \( -name 'test_*.py' -o -name '*_test.py' \) -print0)
|
||||
if [ "${#files[@]}" -eq 0 ]; then
|
||||
echo "No Python test files found under plugins/."
|
||||
exit 0
|
||||
mapfile -t dirs < <(
|
||||
find plugins -type f \( -name 'test_*.py' -o -name '*_test.py' \) \
|
||||
-exec dirname {} \; | sort -u
|
||||
)
|
||||
if [ "${#dirs[@]}" -eq 0 ]; then
|
||||
echo "ERROR: no Python test files found — discovery is broken"
|
||||
exit 1
|
||||
fi
|
||||
failed=0
|
||||
for f in "${files[@]}"; do
|
||||
echo "::group::$f"
|
||||
if ! python3 "$f"; then
|
||||
for d in "${dirs[@]}"; do
|
||||
echo "::group::$d"
|
||||
if ! ( cd "$d" && python3 -m pytest -q --import-mode=importlib . ); then
|
||||
failed=1
|
||||
fi
|
||||
echo "::endgroup::"
|
||||
done
|
||||
echo "Ran ${#dirs[@]} test directory/ies"
|
||||
exit "$failed"
|
||||
|
||||
@@ -21,6 +21,13 @@ jobs:
|
||||
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
|
||||
with:
|
||||
persist-credentials: false
|
||||
# The version-increment check diffs plugin.json against the base branch.
|
||||
fetch-depth: 0
|
||||
|
||||
# Runs first, deliberately: a checker that has stopped detecting its target
|
||||
# would otherwise report a clean build forever. That has shipped here before.
|
||||
- name: Self-test the validators
|
||||
run: python3 .github/scripts/validate_plugin_metadata.py --self-test
|
||||
|
||||
- name: Validate SKILL.md frontmatter
|
||||
run: |
|
||||
@@ -65,14 +72,48 @@ jobs:
|
||||
print(f'ERROR: {e}', file=sys.stderr)
|
||||
sys.exit(1)
|
||||
|
||||
# Finding no skills means the walk broke, not that every skill is valid.
|
||||
if skill_count == 0:
|
||||
print('ERROR: no SKILL.md files found — discovery is broken', file=sys.stderr)
|
||||
sys.exit(1)
|
||||
|
||||
print(f'All {skill_count} SKILL.md files have valid frontmatter')
|
||||
"
|
||||
|
||||
- name: Check for hardcoded paths
|
||||
shell: bash
|
||||
run: |
|
||||
set -uo pipefail
|
||||
echo "Checking for hardcoded user paths..."
|
||||
# Exclude /path/to (example paths) and /home/vscode (standard devcontainer user)
|
||||
if grep -rPn '(?<![a-zA-Z])(/home/[a-z]|/Users/[A-Z])' plugins/ --include='*.md' --include='*.py' --include='*.json' | grep -v '/path/to' | grep -v '/home/vscode'; then
|
||||
# Shell, bats, yaml and toml were previously unscanned; test fixtures and
|
||||
# install scripts are exactly where absolute paths tend to hide. The
|
||||
# *-shim.bats exclusion covers gh-shim's /home/user/... fixtures.
|
||||
INCLUDES=(--include='*.md' --include='*.py' --include='*.json'
|
||||
--include='*.sh' --include='*.bats' --include='*.yml'
|
||||
--include='*.toml')
|
||||
|
||||
# Prove the scan saw files before trusting a clean result. `if grep A |
|
||||
# grep -v B` takes its status from the LAST command in the pipe, so a
|
||||
# first-stage failure (renamed directory, a grep without -P) produced no
|
||||
# output, exited 2, and printed "No hardcoded user paths found" while
|
||||
# inspecting nothing — the same defect this PR exists to remove.
|
||||
scanned=$(grep -rl '' plugins/ "${INCLUDES[@]}" | wc -l)
|
||||
if [ "$scanned" -eq 0 ]; then
|
||||
echo "ERROR: path scan matched no files at all — discovery is broken"
|
||||
exit 1
|
||||
fi
|
||||
echo "Scanning $scanned file(s)"
|
||||
|
||||
hits=$(grep -rPn '(?<![a-zA-Z])(/home/[a-z]|/Users/[A-Z])' plugins/ \
|
||||
"${INCLUDES[@]}" --exclude='*-shim.bats' || true)
|
||||
rc=${PIPESTATUS[0]}
|
||||
if [ "$rc" -gt 1 ]; then
|
||||
echo "ERROR: grep failed with status $rc — the scan did not run"
|
||||
exit 1
|
||||
fi
|
||||
hits=$(printf '%s\n' "$hits" | grep -v '/path/to' | grep -v '/home/vscode' || true)
|
||||
if [ -n "$hits" ]; then
|
||||
printf '%s\n' "$hits"
|
||||
echo "ERROR: Found hardcoded user paths (see above)"
|
||||
exit 1
|
||||
fi
|
||||
@@ -81,27 +122,54 @@ jobs:
|
||||
- name: Check for personal emails
|
||||
run: |
|
||||
echo "Checking for personal emails..."
|
||||
if grep -r '@trailofbits.com' . --include='*.json' --include='*.toml' | grep -v 'opensource@trailofbits.com' | grep -v '.git'; then
|
||||
# The exclusion is anchored: an unanchored '.git' also drops any line
|
||||
# containing "/github", which would mask a personal email beside a URL.
|
||||
if grep -rn '@trailofbits\.com' . --include='*.json' --include='*.toml' \
|
||||
--exclude-dir=.git \
|
||||
| grep -v 'opensource@trailofbits\.com'; then
|
||||
echo "ERROR: Found personal emails (should use opensource@trailofbits.com)"
|
||||
exit 1
|
||||
fi
|
||||
echo "No personal emails found"
|
||||
|
||||
- name: Validate plugin metadata
|
||||
run: python3 .github/scripts/validate_plugin_metadata.py
|
||||
|
||||
- name: Install Claude Code CLI
|
||||
# On a PR, --base-ref turns on the version-increment check: a substantive
|
||||
# change to a plugin must raise its version, or installed clients never
|
||||
# receive it. Skipped on push-to-main, where there is nothing to diff against.
|
||||
env:
|
||||
BASE_REF: ${{ github.event.pull_request.base.sha }}
|
||||
NO_BUMP: ${{ contains(github.event.pull_request.labels.*.name, 'no-version-bump') }}
|
||||
run: |
|
||||
npm install --global --prefix "$RUNNER_TEMP/claude-cli" @anthropic-ai/claude-code@latest
|
||||
if [ -z "$BASE_REF" ]; then
|
||||
python3 .github/scripts/validate_plugin_metadata.py
|
||||
elif [ "$NO_BUMP" = "true" ]; then
|
||||
python3 .github/scripts/validate_plugin_metadata.py \
|
||||
--base-ref "$BASE_REF" --allow-no-bump
|
||||
else
|
||||
python3 .github/scripts/validate_plugin_metadata.py --base-ref "$BASE_REF"
|
||||
fi
|
||||
|
||||
# Pinned rather than @latest: an unpinned install makes CI able to break with
|
||||
# no commit, and zizmor flags it as adhoc-packages. Dependabot does not track
|
||||
# these, so bump them by hand when a new CLI feature is needed.
|
||||
- name: Install Claude Code CLI # zizmor: ignore[adhoc-packages]
|
||||
env:
|
||||
CLAUDE_CLI_VERSION: "2.1.220"
|
||||
run: |
|
||||
npm install --global --prefix "$RUNNER_TEMP/claude-cli" \
|
||||
"@anthropic-ai/claude-code@${CLAUDE_CLI_VERSION}"
|
||||
echo "$RUNNER_TEMP/claude-cli/bin" >> "$GITHUB_PATH"
|
||||
"$RUNNER_TEMP/claude-cli/bin/claude" --version
|
||||
|
||||
- name: Check Claude loadability
|
||||
run: python3 .github/scripts/check_claude_loadability.py
|
||||
|
||||
- name: Install Codex CLI
|
||||
- name: Install Codex CLI # zizmor: ignore[adhoc-packages]
|
||||
env:
|
||||
CODEX_CLI_VERSION: "0.146.0"
|
||||
run: |
|
||||
npm install --global --prefix "$RUNNER_TEMP/codex-cli" @openai/codex@latest
|
||||
npm install --global --prefix "$RUNNER_TEMP/codex-cli" \
|
||||
"@openai/codex@${CODEX_CLI_VERSION}"
|
||||
echo "$RUNNER_TEMP/codex-cli/bin" >> "$GITHUB_PATH"
|
||||
"$RUNNER_TEMP/codex-cli/bin/codex" --version
|
||||
|
||||
|
||||
@@ -27,5 +27,45 @@ repos:
|
||||
- id: check-yaml
|
||||
args: [--unsafe]
|
||||
- id: check-json
|
||||
- id: check-toml
|
||||
- id: check-merge-conflict
|
||||
- id: detect-private-key
|
||||
- id: end-of-file-fixer
|
||||
- id: trailing-whitespace
|
||||
|
||||
- repo: https://github.com/rhysd/actionlint
|
||||
rev: v1.7.12
|
||||
hooks:
|
||||
- id: actionlint
|
||||
|
||||
- repo: https://github.com/woodruffw/zizmor-pre-commit
|
||||
rev: v1.28.0
|
||||
hooks:
|
||||
- id: zizmor
|
||||
|
||||
# Markdown linting is deliberately absent. markdownlint over this repo reports
|
||||
# ~12,400 violations (7,282 of them MD013 line-length, 1,740 MD032, 1,260 MD031),
|
||||
# so it would land either permanently red or with so many rules disabled that it
|
||||
# checks nothing. Both outcomes teach people to ignore the output. Revisit if the
|
||||
# backlog is ever paid down; the measurement is in the PR that added this comment.
|
||||
|
||||
- repo: local
|
||||
hooks:
|
||||
# These were CI-only, so the first signal a contributor got was a red check
|
||||
# after pushing. AGENTS.md asked people to remember to run them. The two
|
||||
# loadability checks stay CI-only — they need the Claude Code and Codex CLIs.
|
||||
- id: validate-plugin-metadata
|
||||
name: validate plugin metadata
|
||||
entry: python3 .github/scripts/validate_plugin_metadata.py
|
||||
language: system
|
||||
pass_filenames: false
|
||||
files: ^(plugins/|\.claude-plugin/|CODEOWNERS|README\.md)
|
||||
|
||||
# Scoped to the validator itself: a checker that has silently stopped matching
|
||||
# reports a clean repo forever, so the fixtures must run whenever it is edited.
|
||||
- id: validator-self-test
|
||||
name: validator self-test
|
||||
entry: python3 .github/scripts/validate_plugin_metadata.py --self-test
|
||||
language: system
|
||||
pass_filenames: false
|
||||
files: ^(\.github/scripts/validate_plugin_metadata\.py|Makefile|\.pre-commit-config\.yaml)$
|
||||
|
||||
@@ -28,7 +28,10 @@
|
||||
- [compound-engineering-plugin](https://github.com/EveryInc/compound-engineering-plugin) - Production plugin structure
|
||||
- [getsentry/skills](https://github.com/getsentry/skills) — Production Sentry skills; `security-review` is a standout routing + progressive disclosure example
|
||||
|
||||
**For Claude:** Use the `claude-code-guide` subagent for plugin/skill questions - it has access to official documentation.
|
||||
**For Claude:** Use the `claude-code-guide` subagent for questions about official Claude
|
||||
Code behaviour that you cannot answer from this repository — it has access to the
|
||||
official documentation. For anything answerable by reading the files here, read them;
|
||||
delegating a lookup you could do directly costs a round trip and buys nothing.
|
||||
|
||||
## Technical Reference
|
||||
|
||||
@@ -38,17 +41,23 @@ This repository uses Claude plugin marketplace metadata as the canonical source
|
||||
|
||||
Rules:
|
||||
|
||||
- Do not add `.agents/plugins/marketplace.json`, `.codex/`, or `plugins/<name>/.codex-plugin/`.
|
||||
- Do not add `.agents/plugins/marketplace.json`, `.codex/`, `.opencode/`, or
|
||||
`plugins/<name>/.codex-plugin/`. The validator enforces this — sidecars drift out of
|
||||
sync with the canonical metadata, which is why the last set was removed in #173.
|
||||
- Keep plugin components at the plugin root using Codex-compatible default paths: `skills/`, `hooks/hooks.json`, `.mcp.json`, and `.app.json`.
|
||||
- If a plugin needs MCP configuration, put it in `.mcp.json` at the plugin root rather than embedding an object in `.claude-plugin/plugin.json`.
|
||||
- Before submitting, run:
|
||||
- Both loadability checks run in CI, not in `make check` — they need the Claude Code
|
||||
and Codex CLIs installed, which is not a reasonable local prerequisite. Run them by
|
||||
hand if you are changing plugin metadata:
|
||||
|
||||
```sh
|
||||
python3 .github/scripts/check_claude_loadability.py
|
||||
python3 .github/scripts/check_codex_loadability.py
|
||||
```
|
||||
```sh
|
||||
python3 .github/scripts/check_claude_loadability.py
|
||||
python3 .github/scripts/check_codex_loadability.py
|
||||
```
|
||||
|
||||
- If this check fails in CI, update the canonical Claude marketplace or plugin root components so Codex can load them through Claude marketplace compatibility.
|
||||
If one fails, update the canonical Claude marketplace or the plugin root components
|
||||
so Codex can load them through Claude marketplace compatibility — do not add a
|
||||
sidecar to work around it.
|
||||
|
||||
### Plugin Structure
|
||||
|
||||
@@ -73,16 +82,32 @@ plugins/
|
||||
|
||||
### Frontmatter
|
||||
|
||||
Skills and commands declare tools with `allowed-tools`, space-delimited:
|
||||
|
||||
```yaml
|
||||
---
|
||||
name: skill-name # kebab-case, max 64 chars
|
||||
description: "Third-person description of what it does and when to use it"
|
||||
allowed-tools: # Optional: restrict to needed tools only
|
||||
allowed-tools: Read Grep # Optional: restrict to needed tools only
|
||||
---
|
||||
```
|
||||
|
||||
Agent files under `agents/` use a different key — `tools`, as a YAML list:
|
||||
|
||||
```yaml
|
||||
---
|
||||
name: agent-name
|
||||
description: "What this agent does and when the coordinator should dispatch it"
|
||||
tools:
|
||||
- Read
|
||||
- Grep
|
||||
---
|
||||
```
|
||||
|
||||
The keys are inverted between the two file types, and the loader silently ignores
|
||||
the wrong one — the frontmatter still parses, the restriction simply does not apply
|
||||
and the agent inherits everything. The validator checks this.
|
||||
|
||||
### Naming Conventions
|
||||
|
||||
- **kebab-case**: `constant-time-analysis`, not `constantTimeAnalysis`
|
||||
@@ -196,31 +221,122 @@ See [ADVANCED.md](references/ADVANCED.md) for detailed patterns.
|
||||
See [API.md](references/API.md) for complete method documentation.
|
||||
```
|
||||
|
||||
## PR Checklist
|
||||
## Before committing
|
||||
|
||||
Before submitting:
|
||||
```sh
|
||||
make check
|
||||
```
|
||||
|
||||
**Technical (CI validates these):**
|
||||
- [ ] Valid YAML frontmatter with `name` and `description`
|
||||
- [ ] Name is kebab-case, ≤64 characters
|
||||
- [ ] All referenced files exist
|
||||
- [ ] No hardcoded paths (`/Users/...`, `/home/...`)
|
||||
- [ ] `python3 .github/scripts/check_claude_loadability.py` passes
|
||||
- [ ] `python3 .github/scripts/check_codex_loadability.py` passes
|
||||
That runs the validator self-test, ruff, shellcheck, shfmt, bats, the plugin
|
||||
Python suites, and the plugin validator.
|
||||
|
||||
**Quality (reviewers check these):**
|
||||
- [ ] Description triggers correctly (third-person, specific)
|
||||
- [ ] "When to use" and "When NOT to use" sections present
|
||||
- [ ] Examples are concrete (input → output)
|
||||
- [ ] Explains WHY, not just WHAT
|
||||
It is most of CI, not all of it. Three things run only in CI, so a green `make check`
|
||||
is strong evidence and not a guarantee:
|
||||
|
||||
**Documentation:**
|
||||
- [ ] Plugin has README.md
|
||||
- [ ] Added to root README.md table
|
||||
- [ ] Registered in root `.claude-plugin/marketplace.json` (repo-level, not the plugin's own `.claude-plugin/`)
|
||||
- [ ] Added to CODEOWNERS with plugin-specific ownership (`/plugins/<name>/ @gh-username @dguido`)
|
||||
- To find the GitHub username: run `gh api user --jq .login` (most reliable — uses authenticated GitHub identity)
|
||||
- **the two loadability checks**, which need the Claude Code and Codex CLIs installed
|
||||
- **the rest of pre-commit** — actionlint, zizmor, check-yaml/json/toml,
|
||||
detect-private-key, end-of-file-fixer, trailing-whitespace. Run `prek run -a` (or
|
||||
`pre-commit run -a`) to cover those locally.
|
||||
- **the version-increment check**, which needs a base ref to diff against and so has
|
||||
no meaning outside a PR.
|
||||
- **`make shell-suites`**, which is a target but not part of `check`: it fails on any
|
||||
machine with the `modern-python` plugin installed, because its shim intercepts the
|
||||
`python3 -` that zeroize-audit's suite uses (#207).
|
||||
|
||||
**Version updates (for existing plugins):**
|
||||
- [ ] Increment version in both `plugins/<name>/.claude-plugin/plugin.json` and the root `.claude-plugin/marketplace.json` when making substantive changes (clients only update plugins when the version number increases)
|
||||
- [ ] Ensure version numbers match between the plugin's `plugin.json` and its entry in the root `.claude-plugin/marketplace.json`
|
||||
Both scan every plugin; the validator is not scoped down in CI. Only the
|
||||
version-increment check is limited to the plugins a branch touched, and it is the one
|
||||
check CI runs that local cannot. Do not add a scoping flag to the local run — the
|
||||
zero-reference guard only arms on a full scan.
|
||||
|
||||
`make fix` applies the formatting CI would otherwise reject. `make help` lists the rest.
|
||||
|
||||
### What the validator enforces, so you do not have to
|
||||
|
||||
Each of these fails the build. There is no value in checking any of it by hand:
|
||||
|
||||
- `plugin.json` exists, parses, and has `name`, `description`, and a semver `version`
|
||||
- Plugin directory name is kebab-case and ≤64 characters
|
||||
- Plugin has a `README.md` (exact case — `Readme.md` passes on macOS and fails on CI)
|
||||
- Registered in `.claude-plugin/marketplace.json`, the root `README.md`, and `CODEOWNERS`
|
||||
- `version` matches between `plugin.json` and `marketplace.json`, **and** increases when
|
||||
you change a plugin — clients only pull an update when the number goes up, so a fix
|
||||
shipped without a bump reaches nobody. Apply the `no-version-bump` label for
|
||||
typo-only changes and CI skips the check.
|
||||
- `SKILL.md` frontmatter parses and has `name` and `description`
|
||||
- Agent files use `tools:`; skills and commands use `allowed-tools:`
|
||||
- `subagent_type` values are namespaced `<plugin>:<agent>` — a bare name is
|
||||
unregistered and the dispatch fails at runtime
|
||||
- No hardcoded `/Users/…` or `/home/…` paths
|
||||
- No `.codex/`, `.opencode/`, `.agents/`, or `plugins/*/.codex-plugin/` sidecars
|
||||
- Both loadability checks pass under the real Claude Code and Codex CLIs
|
||||
|
||||
Three more are reported as **warnings**, so they will not stop a merge and do still
|
||||
need your eye: missing `## When to Use` / `## When NOT to Use`, `SKILL.md` over 500
|
||||
lines, and references that do not resolve. A dangling `references/setup.md` link 404s
|
||||
for every user of the skill, and CI will not stop you shipping it.
|
||||
|
||||
### What no tool can check — this is the part that needs you
|
||||
|
||||
- **The description actually triggers.** Third person, names the situation, uses the
|
||||
words a user would actually type. This is the single highest-leverage line in a skill:
|
||||
a skill that never triggers may as well not exist.
|
||||
- **Examples are concrete** — real input, real output, not a shape.
|
||||
- **It explains why**, including the trade-off and when not to do the thing.
|
||||
- **The version bump is the right size.** The validator confirms the number went up;
|
||||
only you know whether the change was substantive. `MAJOR.MINOR.PATCH`, MINOR for
|
||||
features, PATCH for fixes.
|
||||
- **CODEOWNERS lists the right people**: `/plugins/<name>/ @you @dguido`. Find your
|
||||
username with `gh api user --jq .login`.
|
||||
- **The entry is in the right section** of the root `README.md`. The validator only
|
||||
checks that the plugin appears *somewhere* in that file, so a row appended to
|
||||
whichever table you scrolled to first passes CI and stays filed under the wrong
|
||||
category indefinitely.
|
||||
|
||||
## Scripts a plugin ships
|
||||
|
||||
**A checker that inspects zero items must fail, not pass.** This is the single most
|
||||
expensive class of bug in a repo like this one, because it is invisible on every read
|
||||
and in every review. Real examples, all of which were green for months:
|
||||
|
||||
- a validator using `grep -oP` (rejected by BSD grep) with stderr sent to `/dev/null`,
|
||||
so it printed "all valid" on every run without ever matching anything
|
||||
- an eval grader that judged the response text rather than the artifact, so a run that
|
||||
skipped the actual work still scored a pass
|
||||
- a citation gate that validated only the citations that were present, so a document
|
||||
with zero citations passed
|
||||
|
||||
If your script counts, filters, or matches, make it exit non-zero when the count is
|
||||
zero, and give it a fixture proving it still detects its target. The validator's
|
||||
`--self-test` is the worked example: it builds a known-bad plugin in a tempdir and
|
||||
asserts each checker rejects it, and it fails if it runs fewer assertions than it
|
||||
should — because the self-test is itself a checker.
|
||||
|
||||
Otherwise: `set -euo pipefail`, POSIX ERE rather than PCRE (`grep -oP` is not portable
|
||||
to macOS), and never send a tool's stderr to `/dev/null` unless you have handled the
|
||||
failure it would have reported.
|
||||
|
||||
## Working effectively in this repo
|
||||
|
||||
- **Effort.** Start at `xhigh` for coding and agentic work and `high` elsewhere, then
|
||||
sweep downward on your own evals — `low` and `medium` are unusually strong on current
|
||||
models and often match what an older model needed `xhigh` to do. Defaults carried over
|
||||
from a previous model are rarely the right setting.
|
||||
- **Subagents.** Delegate work that is large and genuinely independent — a wide
|
||||
multi-file investigation, several unrelated tracks. Do not spawn subagents to verify
|
||||
or double-check your own work, and do not split one modest job across several: each
|
||||
one re-establishes context, re-explores, and reports back, and then you re-read the
|
||||
report. One well-briefed agent beats three vague ones.
|
||||
- **Do not add verification scaffolding to prompts.** "Double-check your answer", "add a
|
||||
final verification step", and similar make output worse on current models rather than
|
||||
better — they cause over-verification, and removing them costs no capability. This
|
||||
inverts older advice, so it is worth stating explicitly. Put the check in `make check`
|
||||
or the validator, where it runs deterministically and cannot be talked out of firing.
|
||||
This applies to skills you write or edit; existing skills carrying the pattern are not
|
||||
a cleanup backlog, so strip it when you are already in the file rather than as its own
|
||||
sweep.
|
||||
- **Do not tell a reviewer to pre-filter.** "Only report high-severity issues" is
|
||||
followed literally: the model investigates just as thoroughly, finds the bugs, and
|
||||
then declines to report what it judges below the bar. Precision rises, recall appears
|
||||
to collapse, and the regression looks like a capability problem when it is a prompt
|
||||
problem. Ask for everything with a severity attached and filter in a separate pass —
|
||||
`c-review` and `rust-review` do this correctly if you want a model to copy.
|
||||
|
||||
@@ -0,0 +1,115 @@
|
||||
# Every target here mirrors a CI job. If `make check` passes and CI does not, that is a
|
||||
# bug in this file — fix it here rather than working around it, or the local signal stops
|
||||
# being trustworthy and everyone goes back to pushing and waiting.
|
||||
#
|
||||
# CI jobs covered: Lint (pre-commit: ruff, shellcheck, shfmt), Shell (bats),
|
||||
# Python tests, and Validate plugins and skills.
|
||||
#
|
||||
# RUFF_VERSION must match the ruff-pre-commit rev in .pre-commit-config.yaml. The
|
||||
# validator self-test asserts that; bump both together.
|
||||
RUFF_VERSION := 0.14.13
|
||||
|
||||
.DEFAULT_GOAL := check
|
||||
.NOTPARALLEL:
|
||||
.PHONY: check self-test lint shell bats shell-suites python-tests validate fix help
|
||||
|
||||
## check: most of what CI runs (this is the one you want)
|
||||
check: self-test lint shell bats python-tests validate
|
||||
@echo ""
|
||||
@echo "✓ check passed — most of CI, but not the loadability checks, the"
|
||||
@echo " version-increment check, or the non-ruff pre-commit hooks."
|
||||
|
||||
## self-test: prove the validators still detect what they exist to detect
|
||||
# Runs before validate, deliberately. A checker that has silently stopped matching
|
||||
# reports a clean repo forever; that failure mode has shipped here more than once.
|
||||
self-test:
|
||||
@echo "→ validator self-test"
|
||||
@uv run --no-project python3 .github/scripts/validate_plugin_metadata.py --self-test
|
||||
|
||||
## lint: ruff check + format, pinned to the version CI uses
|
||||
lint:
|
||||
@echo "→ ruff check"
|
||||
@uvx ruff@$(RUFF_VERSION) check --output-format=concise
|
||||
@echo "→ ruff format --check"
|
||||
@uvx ruff@$(RUFF_VERSION) format --check
|
||||
|
||||
## shell: shellcheck + shfmt over every shell script
|
||||
# plugins/ AND .github/scripts/ — globbing only plugins/ left the repo's own scripts
|
||||
# unchecked locally, which is where they are most likely to be edited.
|
||||
shell:
|
||||
@echo "→ shellcheck"
|
||||
@find plugins .github/scripts -name '*.sh' -type f \
|
||||
-exec shellcheck --severity=warning -x {} +
|
||||
@echo "→ shfmt"
|
||||
@find plugins .github/scripts -name '*.sh' -type f -exec shfmt -i 2 -ci -d {} +
|
||||
|
||||
## bats: run plugin bats suites
|
||||
# Fails when the glob matches nothing: this repo has bats suites, so finding none means
|
||||
# the discovery broke, not that the shell code is clean.
|
||||
bats:
|
||||
@echo "→ bats"
|
||||
@files=$$(find plugins -name '*.bats' -type f); \
|
||||
if [ -z "$$files" ]; then \
|
||||
echo " ✗ no .bats files found — discovery is broken (this repo ships bats suites)"; \
|
||||
exit 1; \
|
||||
fi; \
|
||||
echo "$$files" | xargs bats
|
||||
|
||||
## shell-suites: run plugin shell regression suites (CI only, see note)
|
||||
# Deliberately NOT in `check`. zeroize-audit's suite pipes a script to `python3 -`,
|
||||
# which the modern-python plugin's shim intercepts and rejects, so this target fails
|
||||
# on any machine with that plugin installed — for reasons that have nothing to do
|
||||
# with the code under test. CI has no shims and runs it there. See the tracking
|
||||
# issue: #207.
|
||||
#
|
||||
# find, not a glob: `**` needs globstar and degrades to `*` without it, so a suite
|
||||
# one directory deeper would stop running with no signal.
|
||||
shell-suites:
|
||||
@echo "→ shell regression suites"
|
||||
@suites=$$(find plugins -type f -path '*/tests/*' -name 'run_*.sh'); \
|
||||
if [ -z "$$suites" ]; then \
|
||||
echo " ✗ no shell regression suites found — discovery is broken"; \
|
||||
exit 1; \
|
||||
fi; \
|
||||
for s in $$suites; do echo " → $$s"; bash "$$s" || exit 1; done
|
||||
|
||||
## python-tests: run plugin Python test files
|
||||
# pytest, not `python3 <file>` in a loop: a file with no `if __name__ == "__main__"`
|
||||
# block exits 0 under the loop having run nothing, which reads as a pass.
|
||||
# --import-mode=importlib is required — c-review and rust-review both ship
|
||||
# scripts/test_split.py, and the default import mode collides on the basename.
|
||||
python-tests:
|
||||
@echo "→ python tests"
|
||||
@dirs=$$(find plugins -type f \( -name 'test_*.py' -o -name '*_test.py' \) \
|
||||
-exec dirname {} \; | sort -u); \
|
||||
if [ -z "$$dirs" ]; then \
|
||||
echo " ✗ no Python test files found — discovery is broken"; \
|
||||
exit 1; \
|
||||
fi; \
|
||||
failed=0; ran=0; \
|
||||
for d in $$dirs; do \
|
||||
echo " → $$d"; \
|
||||
( cd "$$d" && uv run --no-project --with pytest python3 -m pytest -q \
|
||||
--import-mode=importlib . ) || failed=1; \
|
||||
ran=$$((ran + 1)); \
|
||||
done; \
|
||||
echo " ran $$ran test director(ies)"; \
|
||||
exit $$failed
|
||||
|
||||
## validate: plugin metadata, structure, and cross-references
|
||||
# Scans every plugin. CI scopes to the plugins a PR touches, so local is a strict
|
||||
# superset and cannot pass where CI fails. Do not narrow it to match: the
|
||||
# zero-reference guard only arms on a full scan.
|
||||
validate:
|
||||
@echo "→ validate plugin metadata"
|
||||
@uv run --no-project python3 .github/scripts/validate_plugin_metadata.py
|
||||
|
||||
## fix: apply the formatting CI would otherwise reject
|
||||
fix:
|
||||
@uvx ruff@$(RUFF_VERSION) check --fix || true
|
||||
@uvx ruff@$(RUFF_VERSION) format
|
||||
@find plugins -name '*.sh' -type f -exec shfmt -i 2 -ci -w {} +
|
||||
|
||||
## help: list targets
|
||||
help:
|
||||
@grep -E '^## ' $(MAKEFILE_LIST) | sed 's/^## / /'
|
||||
@@ -147,7 +147,9 @@ When reporting bugs you've found, feel free to mention:
|
||||
|
||||
## Contributing
|
||||
|
||||
We welcome contributions! Please see [CLAUDE.md](CLAUDE.md) for skill authoring guidelines.
|
||||
We welcome contributions! See [AGENTS.md](AGENTS.md) for skill authoring guidelines, and
|
||||
run `make check` before you push — it runs most of CI locally (see AGENTS.md for
|
||||
what it does not cover).
|
||||
|
||||
## License
|
||||
|
||||
|
||||
Reference in New Issue
Block a user