mirror of
https://github.com/trailofbits/skills.git
synced 2026-09-14 14:28:48 +08:00
main
13 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 kz-tob
|
4b1b74b181 |
Give differential-review a trigger, and name every component in its README (#278)
* Give differential-review a trigger, and name every component in its README differential-review's description listed what it does and never named a situation, so it competed on capability wording alone. It now closes with the triggers its own README already documents — reviewing a PR, commit, or diff; checking whether a change re-introduces a fixed bug; asking what else a change could break; finding modified code with no test. The same plugin's README never mentioned adversarial-modeler, which is what Phase 5 dispatches for HIGH RISK changes. Checking whether that was isolated turned up more of it, and the sweep found three kinds of gap: zeroize-audit's agent table was missing three of its eleven agents — 0-preflight, which gates the entire run, plus 5b-poc-validator and 5c-poc-verifier. All three appear in the phase diagram directly above the table, which is why they read as present. constant-time-analysis documents the ct-analyzer CLI end to end and never says the plugin also ships a skill and a command. entry-point-analyzer lists phrases that trigger its skill but never names the skill or its command. Three more READMEs describe their skill without naming it. That matters most where the skill name is not the plugin name and a user cannot guess it: chrome-mcp-troubleshooting and interpreting-culture-index. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix review findings and make the README sweep a gate The two PoC rows I added to zeroize-audit said Phase 4. The diagram three lines above them, SKILL.md, and workflows/phase-5-poc-validation.md all say Phase 5, steps 5a and 5b. "Wave 5a" is a label that exists nowhere. A debugger consulting the table — the artifact this branch designates as what runs when — would have opened phase-4-poc-generation.md and found no validation in it. Also corrected the sentence introducing that table, which still said 10 agents across 8 phases against 11 across 9, and the Phase 0 diagram line, which still credited the orchestrator for a gate the new row credits to 0-preflight. differential-review's README claimed the agent is "dispatched", and named it bare in a column whose other rows are namespaced. Nothing dispatches it: the only instruction is prose in SKILL.md, and a bare subagent_type fails at runtime. Namespaced both, and corrected the five stale line counts in the same file — reporting.md is 369 lines, not the ~120 the token-efficiency section budgets for. Drop the dead `name: trailofbits:<cmd>` key from five command files. The three newest command files carry no name: at all, #275 namespaced 22 bare invocations, and this branch documents the `/<plugin>:<cmd>` form — so the key contradicts the docs it sits next to. Then make the sweep repeatable. Doing this by hand three times found eight gaps and missed two more, both of the same shape: a workflow ships under meta.name, not its filename, so a README citing the filename never writes the name a reader types. The validator now checks that a README names every skill, agent, command, and workflow its plugin ships, reading meta.name for workflows. It refuses a run that inspected zero components, and six self-test assertions hold it to known-bad fixtures. It found git-cleanup on its first run: ships as /git-cleanup:git-cleanup-analysis, README cites workflows/analyze-branches.js four times and that name never. static-analysis had the same gap for codeql-build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix both P2s: the gate was a substring test, and the dispatch was still bare The README gate ran `name not in text`. That reads as thorough and could not fail for a large share of what it counted: `draw` was satisfied by "(draw cards instead)", `semgrep-rule` by the plugin's own name in the install line, `burp-search` by a `scripts/burp-search.sh` path that is a different thing, and `audit` by the prose "shared-state struct audit". Match by kind instead. Commands and workflows are reachable only as `/<plugin>:<name>`, so require that literal — it is the only string a user can type. Agents are dispatched by identifier and never typed as prose, so require an identifier-shaped mention. Skills are genuinely referred to by bare name, so require only a delimited occurrence, which is what stops "draws" counting as `draw`. That surfaced seven real gaps, the four above plus insecure-defaults' audit-pipeline workflow, mutation-testing's skill, and trailmark's code-slice-worker. All seven fixed. adversarial-modeler was still bare at SKILL.md:96. Line 77 was the decision-tree mention; line 96 is the "Delegate to this agent" instruction a model actually acts on, so the runtime failure the last commit claimed to fix survived it. Namespaced, and it now says why. Also from the review: a per-kind floor, since a single total stays healthy while skill_files() — 63% of coverage — silently stops matching; workflow_names anchored to the meta block, because a bare search takes any earlier `name:` in a comment, and .mjs was invisible; and AGENTS.md documents the new hard failure. Self-test 88 -> 96, each new rule with a negative control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
65720f8db2 |
Update claude_review.sh to collapse findings and cover whole diff (#279)
* Update claude_review.sh to collapse findings and cover whole diff
* Fix whitespace-only line and typo in review prompt
Line 94 was a whitespace-only separator, which the trailing-whitespace
pre-commit hook rejects. Also fixes "cenario" -> "scenario".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Require < escaping in review finding summary lines
The <summary> line is raw HTML, so GitHub's sanitizer deletes anything
that parses as an unknown tag. A finding about <plugin>:<agent> rendered
as ":" with no sign that text was dropped, and a quoted <!-- hid the rest
of the line. Backticks do not help: inline markdown is not processed in
<summary>.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Correct the summary escaping rule and extend it to the body
Three defects in the rule added by
|
||
|
Francesco Bertolaccini
|
9e7054ee8a |
git-cleanup: convert the skill to a command driving a dynamic workflow (#217)
* git-cleanup: convert the skill to a command driving a dynamic workflow Replace the prose SKILL.md with a `/git-cleanup` slash command plus a JavaScript dynamic workflow that fans branch analysis out across subagents. The split is the safety property, not an implementation detail. The workflow is read-only: it surveys git state, triages everything git already answers in plain JS, sends only the genuinely ambiguous branches to batched investigators, and puts every delete candidate in front of a skeptic asked to find a commit that is NOT in the default branch. Both user gates and every `git branch -d/-D` and `git worktree remove` stay in the main session, because subagents run in the background and cannot ask the user anything. Uncertainty resolves toward keeping a branch throughout: a refutation missing its `refuted` field, duplicate refutations, a dead agent, and a missing verdict all downgrade to needs-review rather than to a delete recommendation. A wrong keep costs another look at a branch list; a wrong delete costs work that exists nowhere else. Also adds a `js-tests` make target and CI job. Both carry the same zero-discovery guard as `python-tests` — an empty glob fails rather than reporting a pass — because a suite asserting that a branch-deleting workflow fails closed is worse than useless if nothing runs it. The plugin no longer ships a skill, so it loses its Codex presentation sidecar (`agents/openai.yaml` and the brand mark); that metadata only attaches to skills in this repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: set persist-credentials: false on the js-tests checkout zizmor's artipacked audit flagged it. Every other checkout in this workflow already opts out; the new job was copied without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * git-cleanup: address automated review findings Correctness: - Split oversized clusters (MAX_BRANCHES_PER_UNIT). Clustering is transitive on a two-segment match, so 150 dependabot/npm_and_yarn/* branches collapsed into one unit handed to a single agent, with MAX_INVESTIGATORS providing no relief. - Scope the refuter to what each claim actually asserts. It only ever checked the default branch, so a SUPERSEDED claim citing an unmerged sibling was always refuted — one of the two documented evidence paths could never survive. - Permit `fetch --prune` explicitly in READ_ONLY. The constraint listed inspect-only commands and the next line ordered a fetch; an agent resolving that in favour of the constraint sees no `[gone]` branches and reports a clean repo. - Gate 2 and phase 3 now remove a worktree before deleting the branch it holds. Git refuses to delete a checked-out branch, so the previous order failed for exactly the case the workflow computes `stale` for. - Keep the inline evidence standard unconditional. `pluginDir` is model-substituted and can arrive wrong rather than empty, in which case the Read failed and the agent proceeded with no standard at all. Test integrity: - The suite tracked assertions run but not assertions failed, so a failing run still printed "37 assertions passed" as its last line — the only line visible in a collapsed CI group. - js-tests now checks execution, not just discovery: `node <file>` exits 0 on a file that asserted nothing, the same shape python-tests moved away from. Each suite must print a `<n> assertions passed` line with n > 0. Also: 2.0.0, not 1.1.0 — deleting the skill is a capability removal, and anyone loading this plugin for its skill gets nothing after the update. Document node as a `make check` prerequisite. Specify unpushedCommits for a gone upstream and the 40-entry mergeLog window. Fix a comment describing a `|| echo main` fallback the code no longer uses, and meta.whenToUse still naming the deleted skill. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * git-cleanup: add an eval suite for the gate-1 analysis The existing js suite stubs every agent and tests the triage logic in analyze-branches.js. Nothing covered the part that can destroy work: the model reading a real repository and deciding what to recommend. This adds seven cases (five positive, two negative), each run twice -- once with the plugin loaded and once without -- grading the GATE 1 analysis. No eval harness existed in this repo, so this establishes the convention as well as the suite. Two make targets, split by cost: eval-selftest free, no API calls, part of `make check` evals the real suite, opt-in only That split is the point. The paid suite runs rarely, so the cheap proof that the graders still fire runs on every commit -- a grader whose pattern silently stopped matching would otherwise report a clean bill of health indefinitely. Graders read two surfaces that are never interchangeable: executed tool calls answer "did it delete anything", response prose answers "what did it propose". Conflating them scores intentions instead of outcomes. Findings from the first full run, recorded in evals/README.md so they are not rediscovered: - Never regex a command string in prose. A regex cannot tell a recommendation from a mention. Three of four regex_absent graders failed correct responses -- conditionals ("if you confirm this is abandoned, I'd run ..."), explicit refusals, and worked examples answering the question asked. One briefly produced a headline "+0.20 uplift" that was pure artifact. One regex grader remains, on headings. - Never grade a gate-2 artifact. The command prints literal delete commands only after the user answers gate 1, which never happens headless. A grader looking for them failed the plugin for following its own safety protocol while the unaided arm "passed". - Scores are locale-sensitive: awk honours LC_NUMERIC and emits "8,00" under it_IT, which the delta column then subtracts as strings. Results: 6 of 7 cases show delta 0.00 -- Opus handles the analysis correctly unaided. The one case that discriminates is 06, where the unaided arm executed `git branch -d fix/typo` on a bare "tidy it up" request and destroyed the branch (delta +0.75, verified against repo state and the tool-call log, not prose). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fix SC1091 in the eval scripts, and the Makefile gap that hid it The Lint job failed on plugins/git-cleanup/evals: shellcheck could not follow either `source` directive. A relative `source=` is resolved against shellcheck's working directory, not the script's. Running `shellcheck -x plugins/.../run-evals.sh` from the repo root therefore looks for ./lib/graders.sh and does not find it. `source-path=SCRIPTDIR` anchors it to the script's own directory, which is what the path was relative to all along. The reason this passed locally is the more useful half. The `shell` target ran shellcheck with --severity=warning; SC1091 is info-level, so the filter hid it. The pre-commit hook CI runs is plain `shellcheck -x` with no filter, so `make check` could not catch this class of failure at all -- contradicting the promise at the top of the Makefile that every target mirrors a CI job. Dropped the filter so the two match. The repo is already clean under the stricter args, so this costs nothing today and closes the gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * git-cleanup: address review findings from hbrodin Blockers: - `git branch -d` is not the backstop the SAFE_TO_DELETE comment claimed. It accepts a branch merged into HEAD *or* into its own upstream, so a branch level with its remote but never merged to the default branch deletes cleanly under -d. That was the only delete category with nothing behind it. Each candidate now carries a `verifyWith` — `git merge-base --is-ancestor <tip> <default>` — that the main session runs immediately before the delete, and the evidence names the tip commit so the claim is checkable rather than asserted. - PROTECTED covered four names. `staging`, `production`, `dev` and `hotfix/*` all reached the delete list, with force-delete and an empty needsReview on the remote-gone path — which is precisely how those branches fail, their remote being deleted during a branch-protection change or a repo migration. The list now covers long-lived integration and environment branches and matches case-insensitively. Also: - Quoting guidance on the agent-facing path said `"$branch"`, under which `$(...)` still substitutes. Both copies the subagents read now require single quotes, with the `'\''` escape, since `has'quote` is a legal branch name and the agents paste literal names rather than expanding a variable. - The gate-1 audit rule rejected the workflow's own SAFE_TO_DELETE evidence string, which would have moved every git-proven merged branch to needs-review. - `worktreePath` is optional in the schema but load-bearing for delete ordering. The join is now derived from the required `worktrees[]` array. - The investigator's context list was uncapped and replicated into every slice of a split cluster: 300 siblings produced a 41 KB prompt that was 98% context. Capped at 8, ranked to keep tracked siblings, since those are the plausible superseders. - Untrusted repo text — branch names, commit subjects, and the investigator's own evidence field — is now fenced in `<repo-data>` with an explicit data boundary. `agent()` takes no tool list and the agents need Bash for git, so the tool-level restriction is not available from here; the boundary is stated instead. - Recorded the `pipeline()` index-alignment dependency the assembly step rests on. Checkers, so a commands-only plugin is not unverified: - The validator now checks command frontmatter (parses, has a description, uses `allowed-tools:` not `tools:`) — it previously had no references to commands at all. - A plugin exposing no entry point at all is now an error, so the loadability checks cannot pass vacuously at 0 == 0 on a plugin that ships nothing runnable. - Six new self-test assertions cover both, including that a valid command is accepted and that commands alone satisfy the entry-point rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * git-cleanup: lead with the typical agent count, not the ceiling hbrodin measured a dozen branches spawning three agents, because the deterministic triage decides most of them without spawning anything. Eleven was the worst case presented as the headline number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: let the js-tests guard recognise node:test suites The execution guard demanded a `<n> assertions passed` line, which is git-cleanup's own convention. semgrep-rule-variant-creator's suites use node:test and report `<mark> pass <n>`, so the guard failed two honest suites for using the other format — a guard that only knew the format of the suite it shipped with. Both formats now count. The node:test branch does not anchor on `^.`: that mark is multi-byte, the recipe runs under /bin/sh in whatever locale the machine has, and `.` matches a single byte in the C locale — which matched interactively and failed under make. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * git-cleanup: address the automated review's findings The nine hbrodin threads were already handled in c5358a1; these are the github-actions review's, which were not. Correctness: - `verifyWith` now names `refs/heads/<branch>` rather than the tip sha the survey agent reported. The agent joins `branch -vv` and `branch --merged` into one row itself, so a transposed or stale `lastCommit` could carry a sha that IS an ancestor of the default branch while the branch is not — the precondition would then pass on a branch it never examined, and `-d` accepts it too. A refname cannot desynchronise from the branch it names. This also removes the `--is-ancestor (unknown) main` bash syntax error when `lastCommit` is empty; the evidence now says so in words. - Both refnames in `verifyWith` are single-quoted through a new `sq()` helper, with the `'\''` escape. Refnames may legally contain `$(...)`, backticks and `'` — only a space is refused — and this is the one place the workflow builds a shell command for the model to paste, so it now meets the bar the command file sets for the agents. - `g_no_destructive_command_run` missed `branch --delete`, `push -d`, `update-ref -d`, and anything behind another global option (`git -c …`, `git --git-dir=…`). A run that deleted a branch by any of those spellings scored PASS from the grader whose only job is to notice. Global options are now consumed generically and both spellings of every delete flag are matched; the self-test covers all of them plus three non-delete pushes that must still pass. - `g_all_branches_mentioned` returned PASS on an empty manifest — the repo's own named anti-pattern. It now ERRORs, with an assertion proving it. - `make-repo.sh` claimed reproducible shas while inheriting the caller's git config. `eval-self-tests` is in `make check`, so a developer with `commit.gpgsign = true` would have had the whole build block on a passphrase. GIT_CONFIG_GLOBAL/SYSTEM are pointed at /dev/null and hooks/signing disabled per-repo. The pinned `3fcf672`, `64b5c2a` and `a2f470c` are unchanged. - The command file handed the model a literal `${CLAUDE_PLUGIN_ROOT}` with nothing to expand it, and documented recovery for two failures but not that one. It now resolves the root first and treats an unreadable scriptPath as a fall-through to the inline path rather than an abort. Docs that contradicted the code: - git-cleanup README stated the `git branch -d` safety rationale this PR exists to disprove, and never mentioned `verifyWith` — a maintainer reading it would have dropped the precondition as redundant. Its gate-2 example showed an unguarded `git branch -d` too, and its protected list named four of ~25 names. - merge-evidence.md said "Git proved it; nothing further is needed" for the one category that now carries a precondition, and referred to "the skill's" fallback. - evals/README.md credited `analyze-branches.test.mjs` with covering gate-2 prose it does not read. - Makefile said CI scopes the validator to touched plugins. It does not — only the version-increment check is scoped; AGENTS.md had it right. - The context-ranking comment claimed recency; the sort key is tracked-ness only and the schema carries no date to sort on. - A dead `grep -v` in the self-test, overwritten by the next line. Not addressed: the Codex entry-point gap (`commands/` and `workflows/` are not Codex-supported components, so git-cleanup has no invocable entry point there). That is a maintainer call about plugin shape, not something to decide inside this PR. Suites: 47 JS assertions, 49 eval self-test assertions, 53 validator assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * git-cleanup: report protected branches, and normalize the refs the survey reports Both findings from hbrodin's second pass. Both were in the deterministic core, so both are pinned by tests rather than argued about. **Protected branches vanished from the report.** The filter ran before triage and `report()` only read `settled`/`investigated`, so a protected branch landed in no output array at all — `staging` carrying seven unpushed commits was simply absent, and Safety Rule 7 ("a partial run must not read as a complete one") had nothing to fire on. Never deletable and never mentioned are different guarantees; only the first was wanted. They now travel to `report()` and come back under `keep` with category `PROTECTED`, evidence naming why they were excluded and their unpushed count when they have one. An unpushed count on a protected branch is logged as well. Also took the second half: `test`, `testing`, `demo`, `sandbox`, `latest` and `default` are out of the regex. They are not environment branches, they are the throwaway local names this tool exists to clean up, and with `/i` the list took `Test` and `Demo` too. Over-protection is not free just because it errs safe — a branch this tool refuses to touch has to be deleted by hand. **`defaultBranch` arrived as a remote ref.** `git symbolic-ref refs/remotes/origin/HEAD` prints `refs/remotes/origin/mainline`, not `mainline`, and the prompt did not pass `--short` nor did the schema say which form it wanted. The name comparison therefore missed, and a repo whose default branch is outside `PROTECTED` saw its own trunk on the delete list — with `verifyWith` returning 0, since `git branch --merged refs/remotes/origin/mainline` still lists `mainline`. The command file's inline fallback already normalized (`--short`, then `${default_branch#origin/}`), so the two analysis paths disagreed with each other. Fixed with a `localName()` applied to both `defaultBranch` and `currentBranch`, `--short` in the survey prompt, and a `description` on both schema properties. `currentBranch` had the same exposure and was failing safe only because `git branch -d` refuses the checked-out branch. Tests: 61 JS assertions, up from 47. Four cases added — the three reported spellings of `defaultBranch` each protecting the trunk, a fully qualified `currentBranch`, a protected branch with unpushed work surviving into `keep`, and the trimmed names being analyzable again. Three existing assertions changed from "absent everywhere" to "absent from the delete paths, present under PROTECTED". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Dan Guido
|
9e06dc67a3 |
Make every documented command runnable under our own python shims (#258)
* Make every documented command runnable under our own python shims
The modern-python plugin ships PATH shims that refuse `python <script>`,
`pip install`, `python -m pip` and `uv pip install`. Twelve other plugins
in this marketplace issued exactly those forms, so installing our own
plugin broke our own skills — and CI was green throughout.
The worst case was not theoretical. c-review and rust-review both call
their Phase 4 planner as `python3 "${PLUGIN_ROOT}/scripts/build_run_plan.py"`,
so with the shim installed every run died before spawning a worker.
Verified both directions: the new form exits 0 with the shim on PATH, the
old form exits 1.
Phase 1's reading pass named 16 skills. A mechanical sweep found 96
candidate lines across 44 files, and scanning shell scripts as well as
markdown found 10 more the docs sweep had missed. That gap is the reason
the check below exists.
The fix is not one substitution. Four classes needed different treatment:
- Our own scripts become `uv run --no-project <script>`. Not bare `uv run`,
because these execute inside the *target* repo, which may be a Python
project that cannot resolve; verified against a broken pyproject.toml and
against validate_artifacts.py's sibling import of generate_sarif.
- Package installs become `uv add` for a dependency, `uv tool install` for a
CLI, `uv sync` for a project's own editable install.
- Third-party CLIs we merely document — OSS-Fuzz's infra/helper.py, yarGen —
become `uv run --no-project python <script>`, which keeps upstream's exact
semantics rather than handing their script an environment we manage.
- atheris's instrumented build keeps its source build, as
`uv add --no-binary-package cbor2`. Dropping that flag would silently
produce an uninstrumented fuzzer, which is worse than a visible failure.
Its prose was updated to name the flag it now uses.
Two factual corrections fell out. `pip install caracal` was wrong twice
over: caracal is a Rust tool (Cargo.toml at its root), so it is now
upstream's own `cargo install --git`, not a uv equivalent that would fetch
an unrelated PyPI package. And `pip install uv` cannot bootstrap uv under
a shim that intercepts pip, so culture-index now points at the official
installer.
Thirteen lines stay as they are, each deliberately: Dockerfile `RUN` lines
and oss-fuzz's build.sh run in containers where our shims are absent;
codeql's pip calls install the *analysed* project's dependencies, and that
project is arbitrary; trailmark's dispatch skills must keep saying "Do NOT
run `pip install`"; and modern-python documents what it intercepts.
`make shell-suites` passes again as a result — exit 0 with the 1.6.0 shim,
where AGENTS.md previously recorded it as broken by variant-analysis.
The guardrail: check_python_invocations scans 698 markdown and shell files
and fails on the four refused forms, with structural exemptions for
dockerfile fences and an `allow-legacy-python: <reason>` marker that scopes
to its code block. Eleven self-test fixtures cover it, four asserting it
fires and seven asserting it stays quiet on the compliant forms. It was
mutation-tested in both languages, and it caught its own worst bug during
development: unanchored patterns first flagged `uv run --no-project python
fuzz.py`, the very form the advice recommends. Self-test goes 45 -> 56.
* Review pass: fix the atheris flow, drop a stray exemption, trim comments
Three corrections from reviewing the branch diff:
- atheris's install now opens with `uv init --bare`, without which the
documented `uv add atheris` errors in a bare harness directory. The old
pip form assumed an activated venv, so setup was always implicit; now
it is one explicit line.
- ossfuzz carried an allow-legacy-python marker on a C++ build block that
contains no python at all — yesterday's insertion matched the first of
three "Build in build.sh" headings instead of the python one. The
exemption now sits only on the block that needs it.
- The anti-vacuity message said "read no markdown" for a scan that also
covers shell scripts.
The rest is weight: the new check's comment blocks, the hardcoded-path
constants' commentary, the AGENTS.md bullets and the three exemption
markers all said the same things at two to three times the length. Each
keeps its one-line why; the narratives are gone. No behavioural change —
self-test still passes 56 assertions and the full scan is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Address the review: fix where packages land, widen the check to match the shim
The review's core insight was right twice over. Several substitutions had
changed WHERE a package lands, breaking the documented next step, and the
checker enforced a narrower invariant than the shim it exists to mirror.
Where packages land:
- trailmark is imported as a library from five skills, and a `uv tool
install` environment is not importable — the retry loop at
trailmark/SKILL.md:47-51 would have spun forever on the exact error it
names. The CLI install stays `uv tool install`; the import snippets now
run under `uv run --with trailmark python -`.
- `uv add` writes to the manifest of whatever project you are standing
in, which for sarif-parsing is the audited repo. Its scripting rows,
ijson comment and jsonschema example now use `uv run --with <pkg>`,
which leaves no trace. atheris keeps `uv add` deliberately: the fuzzing
harness is the user's own project, made explicit by `uv init --bare`.
- `uv sync` leaves ct-analyzer in .venv/bin, so the README's very next
line failed with command not found. Now `uv tool install .`, verified
end to end: the console script lands on PATH and --help runs.
- yarGen needs pefile/lxml/yara-python, which `--no-project` had detached;
now `uv run --with-requirements requirements.txt`.
- The cbor2 source-build preference now persists via
`no-binary-package = ["cbor2"]` under [tool.uv] (field verified against
uv's accepted-settings list), so a later `uv sync` cannot silently swap
in an uninstrumented wheel.
The checker, widened to the shim's actual behaviour:
- `python3 --version` and `python3 -u foo.py` are refused by the shim but
passed the old patterns; one live instance (constant-time-analysis
README) proved it. Both forms are now caught.
- Every `uv pip` subcommand is refused, not just install; `-t` joins the
allowed tool-managed flags.
- .py files are scanned too: usage strings and error messages told users
to run refused commands from ten scripts, including the --help of the
very planner this PR fixed. All rewritten.
- The evals/tests exemption now tests path parts relative to plugins/, so
a checkout under a directory named tests no longer exempts every file.
- An allow-marker's scope ends at a blank line as well as a fence, so one
marker cannot blanket a whole file; quality-assessment.md gains the
second marker that scoping made necessary.
Also from the review: zeroize's preflight gets `which python3` back (a
helper script still needs the binary; the shim never required removing
it), the Makefile's shell-suites note no longer describes an interception
that is gone, and the cairo CI example warns that it rebuilds caracal
from source each run.
Self-test 56 -> 63; every new pattern and exemption is fixture-covered
and was mutation-probed against the real tree. Full scan: 0 findings over
773 files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Address the second review: prerequisite probe, checker parity, package placement
The review's P2 was a regression this PR introduced for a population the
first fix ignored: c-review and rust-review now require uv, and a box
with python3 but no uv would die at Phase 4 exactly the way shimmed boxes
died before. Phase 1 (Prerequisites) in both skills now probes
`command -v uv` and aborts with install guidance. zeroize-audit's
preflight already checked uv. The four converted shell suites gain the
same guard with a clear message instead of a bare 127 mid-run.
Checker parity with the shims, second pass:
- pipx and the non-install pip subcommands are refused by catch-all shim
arms and passed the checker; both get named-subcommand patterns.
- A script named by variable or path (`python3 "$MERGE"`) has no `.py`
token; a new pattern covers it and immediately caught one live
instance — a codeql test stub that fakes uv itself, now carrying an
allow-marker with its reason.
- finditer everywhere: a compliant `uv run` earlier on a line no longer
masks a refused command later on it, which was exactly the table-cell
case the unanchored design exists for.
- Prohibition phrases now test the text BEFORE the match, so
"Use `pip install semgrep` instead of the tarball" is flagged while
"Do NOT run `pip install`" stays exempt.
- The uv-pip allowance matches whole flags after the command, so
`--target-dir` no longer counts as `--target` and a trailing `-t /tmp`
does; `uv pip` precedes `pip` in the pattern order so its lines get
the right advice; a pip match directly after `uv ` defers to the
uv-pip verdict instead of double-reporting.
Package placement, continued from the same insight as round one:
- yarGen regains --no-project alongside --with-requirements, plus a cd
into the checkout so requirements.txt resolves where it lives.
- sarif-parsing's jsonschema example no longer names a script that does
not exist, and the table's run-forms show a concrete script.py.
- culture-index's two messages now agree and name the actual remedy
(`uv run --project` on the scripts directory) instead of re-adding a
dependency its pyproject already declares.
- merge_sarif's usage line gains --no-project; the generator plugin's
install section stops prescribing a venv its own runner never uses.
- generate_poc declared requires-python >=3.9 while using `str | None`
in a signature, a TypeError on 3.9 that uv's interpreter selection
made reachable; now >=3.10.
- The GitLab CI example exports ~/.local/bin onto PATH, without which
`uv tool install` warns and the next line dies command-not-found.
Self-test 63 -> 71; the masking, prohibition-direction, flag-position
and pipx cases are all fixtures, and each new pattern was probed live
against the tree (plant, error, remove, clean — 0 findings over 773
files).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Address the third review: importable trailmark, honest probes, sturdier scan
The P2 was the residue of round two's own fix, applied to the siblings
but not the flagship: trailmark/SKILL.md told the model to cure an import
error with `uv tool install`, which cannot cure it — a tool env is not
importable — while forbidding every fallback. The install block now says
what each remedy is for: `uv tool install` for the CLI, `uv run --with
trailmark python -` for the snippets, and the other five library-first
docs carry the same one-line annotation next to their install command.
Empirically settled rather than taken from the review: `uv run python3
<script>` works fine under the shims — uv prepends its environment's bin
directory, so python3 resolves to a real interpreter, not the shim. The
review's claim to the contrary would have meant rewriting the Makefile
and a bats suite; a two-minute transcript said no. Also declined: a
zeroize uv-prerequisite (its preflight already lists uv and uvx; the
C/C++ `which` line now names uv too).
Real and fixed:
- ct-analyzer's availability probe ran `python3 --version` by subprocess
— the one refused form — so under the shims it reported "Python is not
available" on machines where it plainly is. It now probes
sys.executable, the interpreter the analyzer itself runs under.
Verified under the shim: probe returns True.
- The flag step-over in both script patterns handles long and
value-taking flags (`python3 -W ignore harness.py`, `--verbose
tool.py`), matching the shim's two-slot consumption.
- A bare `allow-legacy-python:` with no reason no longer exempts
anything; the reason the docs demand is now enforced.
- `uv run {baseDir}/...` gets --no-project at the ten semgrep and
culture-index call sites that round two missed, and the culture-index
remediation strings now name that same runnable command instead of a
--project mechanism nothing uses.
- pip gains cache/config; the pattern comment now says the subcommand
list is deliberately a subset.
- Both filesystem scans skip .venv/node_modules-style directories, after
a stray local .venv (left by this session's own uv probe, and invisible
to CI) turned the path scan red.
Smaller review items: the uv-probe prose says "Phase 4 onward" rather
than a wrong phase range, run_fixtures' comment stops claiming PEP 723
headers its stdlib-only helpers do not have, the yarGen one-liners say
to run from the checkout, `uv tool install` sites note or export the
tool bin dir the way a fresh container needs, and sarif-parsing's table
column says Install / run and stops naming a file that does not exist.
Self-test 71 -> 74. Full scan: 0 findings over 773 files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Dan Guido
|
488d37c71c |
Collapse the review into one xhigh opus tier, and stop invented evidence (#256)
* Pin the review model, raise effort to xhigh, stop invented evidence Three problems with the automated review, found while auditing what was actually reviewing PRs #253-#255. **The model was never specified.** `claude_review.sh` passed `--effort` but no `--model`, so the reviewer was whatever the CLI happened to default to at run time, and nothing in the logs said which. The CLI version was pinned to stop CI changing without a commit while the larger lever on review quality was left floating. Both tiers now pin `opus`, and the run line records model, effort and resolved CLI version. **The CLI is now deliberately unpinned**, in both this workflow and the loadability check, so they track releases as they ship. For loadability that is the point: the version worth proving plugins load against is the one users run. The logged version is what makes a surprising result attributable after the fact. **The review claimed to run things it cannot run.** Its allowlist is `gh pr` reads plus Read, Grep and Glob — no interpreter, no test runner. Across three PRs it reported a Python snippet it had "verified directly" whose regex cannot compile, a pytest suite it had "run locally" with a pass count that does not match reality, and a shell script three "independent verifiers" had supposedly executed. Every conclusion was correct and every proof was fabricated. The deep prompt already said "you cannot execute anything"; the prompt that actually runs never did. That statement moves into the shared prompt, extended to forbid reporting output no tool produced. Effort goes low -> xhigh on the tier that runs on every push, so the strongest review is the default rather than something to remember to ask for. The shared prompt also now says to rank on consequence rather than diff size, after a one-character fault that made a checker miss its own target was filed as a nit. The `fast` name is kept: it is the check name branch protection matches on, and it describes the trigger rather than the effort. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Collapse the two review tiers into one The split was a cheap pass on every push plus an xhigh adversarial pass behind a `deep-review` label. It did not survive contact: the label was never created, so the deep tier skipped 9 times and ran zero, and every review this repository has ever received came from the cheap tier. With both tiers now on opus at xhigh, the only thing left separating them was scope, and there was no reason to gate the better scope behind a label somebody has to remember to apply. So there is one job. It takes the deep tier's prompt and tools — reads beyond the diff, gets git history, checks the PR's claims against the files — and the cheap tier's `--edit-last` posting, which matters now that it runs on every push. `fast` is gone from the check name, the script's tier argument, and the concurrency key. The name was already inaccurate once effort went to xhigh, and the ruleset on main requires license/cla, Validate, Pre-commit and bats — not this check — so nothing depended on it. The elaborate label-keyed concurrency group goes too; it existed only to stop a push cancelling an in-flight deep review, and there is no second tier to collide with. Timeout follows the deep tier at 30 minutes, since xhigh on a large diff needs the room. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Address the review of this PR The new xhigh reviewer found real problems in its own configuration. **The API key was in the unpinned install step's environment.** It was there only to satisfy `if: env.ANTHROPIC_API_KEY != ''`, since `secrets` is not available in a step-level `if:`. Combined with `@latest`, that runs npm lifecycle scripts from a version nobody chose beside an org-wide credential. Presence is now reduced to a boolean in its own step, and the install step holds no secret at all — which is what makes @latest acceptable there. **The posting guard counted comments it did not write.** It matched any issue comment in the window, so a maintainer replying to the previous review could stand in for a review this run never posted: green check, no review. Measured on this PR's siblings — the old query counts 2 on #253 and #255, the new bot-only query counts 1. Also paginated, since the unfiltered call would miss a review past the first 30 comments. **`AGENTS.md` reaches the reviewer and tells it to run things.** CLAUDE.md is just `@AGENTS.md`, so it loads as project instructions telling the model to run `make check`, run `prek run -a`, and consult a `claude-code-guide` subagent — none of which it can do. That is the fabrication channel this PR exists to close, arriving by a route the prompt did not address. The prompt now names those files and says they are addressed to someone else. **validate.yml goes back to a pinned CLI.** Unpinning it was my extension, not what was asked, and the risk is misplaced: that job is a required check, so an upstream release renaming a field in `plugin list --json` reddens every open PR at once and blocks merges with no commit to explain it. The review job can go red harmlessly. This also re-aligns Codex and Claude, both pinned there again, and keeps dependabot.yml's note about "the pinned npm CLI versions" accurate. Smaller: the prompt described its allowlist as "only `gh pr` reads" while mandating `gh pr comment`, a write, and granting `git log`/`git diff`. And the comment over `MODEL` claimed an attributability the `opus` alias does not provide, since it tracks new Opus releases by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Unpin the Claude Code CLI on the loadability check too Reverses the revert two commits back, deliberately and with the argument against it on the record. The review made the case for pinning here: `Validate plugins and skills` is a required check, so a Claude Code release that breaks plugin loading reddens every open PR at once with no commit to explain it. That reading is correct about the mechanics and wrong about which failure costs more. A pin nobody remembers to bump drifts until CI is proving loadability against a version no user runs, which is the one thing this check exists to establish — and it fails silently, by passing. The loud break is the preferable failure, and it is now the chosen one rather than an oversight. Dependabot does not track npm CLIs installed this way, so the real choice was a live version or a stale one, never a maintained one. The Codex CLI beside it stays pinned at 0.146.0. Same class of manual pin and arguably the same argument applies, but unpinning another vendor's CLI was not asked for and would widen this change past its subject. dependabot.yml's note is corrected to match: singular, and naming which one is pinned and why the other is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Dan Guido
|
f3c8d2a73f |
Back three AGENTS.md enforcement claims with validator checks (#253)
* Back three AGENTS.md enforcement claims with validator checks AGENTS.md lists these under "What the validator enforces, so you do not have to", the heading that tells contributors and Claude to skip checking by hand. The validator did not perform any of them. - Hardcoded `/Users/…` and `/home/…` paths: moved out of the CI workflow and into find_hardcoded_paths(), so `make check` and pre-commit cover it too. Python's re has lookbehind natively, which drops the `grep -P` dependency BSD grep cannot satisfy. Same file types, same `*-shim.bats` exemption and `/path/to` and `/home/vscode` placeholders as before, and the scan carries the anti-vacuity guard across: zero files scanned is a hard failure, not a clean result. - Command files: validate_agent_frontmatter() only ever opened agent and skill files, so the "and commands" half of the allowed-tools claim was unbacked. Renamed to validate_tools_frontmatter() now that it covers all three. - subagent_type: the check returned early for a plugin with no agents/ dir and otherwise only flagged a bare name matching that plugin's own agent. It now resolves against a repo-wide agent registry, so a bare name borrowed from another plugin reports that plugin's namespace, and one that names no agent at all is reported as a dispatch that fails at runtime. Also documents three checks the validator already enforced but AGENTS.md never listed: plugin.json name matching the directory, marketplace source and description parity, and dependabot lockfiles. All three flag zero violations against the repo as it stands, verified by planting each defect and confirming the validator rejects it. Self-test goes from 34 assertions to 43, and SELF_TEST_MINIMUM is now the exact count rather than a loose floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Catch lowercase home directories in the hardcoded-path check The pattern inherited from the CI step matched `/home/[a-z]` but `/Users/[A-Z]`, so it only caught macOS paths whose account name starts with a capital. Account short names are lowercase by convention, which means the common form went undetected: `/Users/alice/...`, and this repo's own `/Users/user/cc/skills`, all passed clean. The check was missing the thing it exists to find, and my mutation test did not catch that because I happened to plant `/Users/Someone` with a capital S — the one spelling the pattern could see. Both branches now accept either case. Widening turned up exactly one new match across the repo, and it is a false positive: c-review's SKILL.md uses "/Users/me/My Repo" to show that a path containing a space has to stay quoted. That and `/Users/Shared`, a real macOS system directory, join the placeholder list. Self-test goes 43 -> 45: the lowercase path must be caught, and `/Users/Shared` must not be. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Graham Sutherland
|
04b241176f |
Rework property-based-testing skill and add eval suites. (#235)
* Rework property-based-testing skill and add eval suites. * Fix shellcheck SC2015 and exclude eval fixtures from CI pytest * Run eval self-tests under uv so `make check` survives the python3 shim `eval-self-tests` discovers harnesses repo-wide, and all three it finds today pipe a script to `python3`: property-based-testing's run.sh and effectiveness.sh, and writing-lean-proofs' run.sh. On any machine with the modern-python plugin installed, its `python3` shim rejects those calls and `make check` fails for reasons that have nothing to do with the code under test. That is the trap #207 documents, and the reason `shell-suites` is deliberately kept out of `check`. Excluding a second target is the wrong answer here — these self-tests are free and they are what makes an eval result trustworthy — so run them under `uv run --no-project`, which puts a real interpreter ahead of the shim on PATH. Harnesses should still call uv themselves. The wrapper only applies inside `make check`, and the real sweeps are invoked by hand. * Move evals-extra out of the skill directory to the plugin root `evals/` already sits at the plugin root; `evals-extra/` sat inside `skills/property-based-testing/`, so every user of the plugin shipped 900 lines of bash, a `requirements.txt` naming hypothesis, and a fixture whose tests are broken on purpose — inside the one directory the model reads guidance from. run.sh's own header notes the hazard of the model finding skill files by filesystem exploration; this removes the material it would find. Nothing in the machinery cares which of the two locations it is in: the Makefile, the CI pytest exclusion, ruff's per-file ignores and the plugin validator all match on an `evals*` prefix anywhere under `plugins/`. Verified by running eval-self-tests and validate from the new layout. `plugin_root` in both harnesses walks up one directory now instead of three. * Re-execute run.sh by path in its own self-test instead of $0 The four end-to-end assertions run the whole sweep in a subprocess by invoking `"$0"`. That is only a runnable command when the caller passed a path with a slash in it: `bash run.sh --self-test` from this directory sets `$0` to `run.sh`, which is not on PATH, so all four exited 127 while the eleven unit assertions passed. It worked by accident because the Makefile and the README both happen to pass a path. Use `$here/run.sh`, which is invocation-independent. * Report a detector that could not run as a failure, not as a non-trigger `skill_invoked` returned an exit status, and both "the model did not call the skill" and "python3 blew up" came back as 1. `check_triggered` then fell through its ladder to `no` — the one verdict the aggregator treats as a measurement. So a broken interpreter did not fail the sweep; it scored every positive session as a clean negative, and 45 sessions and $36 came back looking like a recall regression. This is not hypothetical. The modern-python plugin's `python3` shim rejects the call (#207), which is how it was found: the self-test's "skill invoked -> yes" case returned `no`. The verdict is now a printed token — `yes`, `no`, or `error:<detail>` — because an exit status cannot carry the distinction: 1 is both python3's own failure status and the detector's "not found". A healthy session whose detector failed lands in a new `crash:detector` branch, which invalidates the sweep like any other failed session and puts the interpreter's message in the NOTE column. Every python3 call in the script goes through `uv run --no-project` for the same reason, so the harness also runs correctly by hand under the shim rather than only under `make check`. That adds a uv dependency, guarded at startup alongside the existing claude CLI check. Pinned by a new assertion that points the detector at a nonexistent interpreter and asserts `crash:detector`, not `no`. 16 assertions, was 15. * Refuse to grade an effectiveness run whose patch never applied `grade()` diffs the failing tests before and after replacing canonicalize_url with an identity stub. It called `patch_codec` and never checked the result, on the assumption that `set -e` would abort — but errexit does not propagate out of a function into the command substitution `got="$(grade "$d")"` runs in. So a failed patch left the "after" suite running against the UNPATCHED fixture: before and after come out identical, nothing moves, and a suite that genuinely caught the defect is written down as `part` — "suite fails, but not on this defect". That also silently disarmed the drift guard inside patch_codec, whose entire job is to refuse to grade in exactly this situation. Its message went to stderr and the grade continued. A failed patch is now ERR, and the fixture is restored from the backup on that path. patch_codec goes through `uv run --no-project` like the rest of the repo, so the shim (#207) is not what triggers it either. Pinned by a new assertion that hands the grader a fixture with no canonicalize_url to replace and asserts ERR rather than a grade. 4 assertions, was 3. * Refuse a multi-level effort sweep while SKILL.md pins `effort:` A skill's `effort:` frontmatter overrides the session level, so the `--effort` that effectiveness.sh passes each session is ignored the moment the skill loads. SKILL.md pins `effort: low`, so the default `EFFORTS="low medium high"` ran three sessions at `low` and printed the level each one *asked* for in the EFFORT column. Nothing in the output gives that away. Three rows agreeing is also what a healthy sweep looks like when effort genuinely does not matter, which is the conclusion the table invites — and the conclusion that keeps the pin at `low` forever. It is the same shape as the failures this suite already guards against: a checker that has quietly stopped varying its independent variable reports a clean result. It also means the recorded sweep cannot be reproduced against the plugin as shipped. Either that sweep predates the pin, or it was already this artefact; there is no third reading. That matters because re-running an effort sweep is what AGENTS.md asks for whenever the model changes, and this is the check that would have been re-run. Requesting the pinned level alone is still allowed — that scores the shipped configuration and the label is true. NOPLUGIN loads no skill, so nothing overrides and a sweep there is honest. Anything else with a pin present exits 2 and names both ways forward. The `q` in the sed matters: a second `effort:` line anywhere in the file, a fenced YAML example say, would otherwise make `$pinned` multi-line and refuse even a correct `EFFORTS=low`. Both READMEs documented a bare `./evals-extra/effectiveness.sh`, which now exits 2, so they move to `EFFORTS=low` here rather than in a follow-up that would leave the docs describing a failing command in between. Known cost, not fixed here: `EFFORTS=low` is one session, where the broken sweep at least sampled the same configuration three times. run.sh:20-22 rejects n=1 for the sibling metric on the grounds that invocation is stochastic. Fixing it means a repetition knob or a different default — a change to how the eval samples rather than to what it reports, so it is left to the author. Pinned by two new assertions: a pinned skill refuses `low medium high`, and allows `low`. 6 assertions, was 4. * Check for the claude CLI below run.sh's --self-test dispatch, not above it The preflight sat at the top of the script, so it ran before the `--self-test` branch and the self-test exited 2 on any machine without Claude Code installed, having run zero of its sixteen assertions: $ env PATH=/usr/bin:/bin bash run.sh --self-test claude CLI not found: claude `claude_bin` is only swapped for the stub inside `self_test()` itself, which is far too late to matter. So the guarantee in the comment above that function — "uses a stub binary, so it costs nothing and can run in CI" — was false, and `make eval-self-tests`, and therefore `make check`, broke for any contributor without the CLI. AGENTS.md draws exactly this line for the two loadability checks: they run in CI rather than in `make check` precisely because needing the Claude Code CLI is not a reasonable local prerequisite. A self-test that claims to be free must not smuggle that requirement back in. The uv check stays above the dispatch, because the self-test genuinely needs it: the detectors run through `uv run --no-project python3`, and uv is already a prerequisite everywhere else in the repo. effectiveness.sh had this split right and was the template. Verified with `claude` absent from PATH and uv plus GNU coreutils present: all 17 assertions pass. With neither present it now stops on uv, which is the honest dependency rather than a borrowed one. Worth knowing and not fixed here: `timeout(1)` is still an undeclared dependency of both a real sweep and the self-test, and it does not exist on a stock macOS PATH. Absent, the child-sweep assertions fail with 127. CI is Linux so it is covered there, and the bash-3.2 accommodation at the end of the self-test suggests stock macOS is meant to work, so it wants either a preflight alongside uv or a documented prerequisite. Pinned by a new assertion that runs a real sweep with a nonexistent CLAUDE_BIN and asserts exit 2. The risk on the next edit is the check being deleted rather than moved, which would turn a typo'd CLAUDE_BIN into 45 crash:rc127 sessions instead of an immediate refusal. 17 assertions, was 16. * Fix sed issue on BSD * Restore refactoring.md --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> Co-authored-by: Emilio López <emilio.lopez@trailofbits.com> |
||
|
Samuel Moelius
|
09dfbd9153 |
Drop the "When to Use" / "When NOT to Use" requirement (#216)
* Drop the "When to Use" / "When NOT to Use" requirement * Fix inconsistency in AGENTS.md * Eliminate `REQUIRED_SKILL_SECTIONS` check in validate_plugin_metadata.py |
||
|
Dan Guido
|
ca08fc8a91 |
Commit plugin lockfiles; unblock Dependabot (#213)
* Commit plugin lockfiles so Dependabot can do something useful The uv ecosystem config added in #206 pointed at four directories that declare PEP 621 ranges and carry no lockfile. With nothing to pin, Dependabot's only available action is raising the lower bound of an already-open range — which changes nothing about what installs and only drops support for older versions. It opened five such PRs within a minute of #206 merging (#208-#212), all no-ops: the existing ranges already resolved to exactly the versions being proposed as new floors. The one directory that did have a lockfile, constant-time-analysis, produced no PR at all, because there was genuinely nothing to update. That is the whole diagnosis. Lockfiles committed for the other four. .gitignore ignored uv.lock globally, which is why they were missing; constant-time-analysis's was tracked only because it predates the rule. Now scoped to the root file (ephemeral — there is no root pyproject.toml) with plugin lockfiles explicitly allowed, matching the pattern already used for .mcp.json. Also fixes two bugs #206 introduced: - The version-increment check failed all five Dependabot PRs, and Dependabot can neither bump a plugin version nor label its own PR, so every future dependency PR would have been permanently red. Exempted by actor. - The 'no-version-bump' label was documented in AGENTS.md and wired into validate.yml but never created, so the escape hatch did not exist. Created. * Re-run CI with the no-version-bump label applied The version-increment check fired on this PR: adding uv.lock under plugins/<name>/ counts as touching those plugins. Correct behaviour — the lockfiles pin exactly what the existing ranges already resolve to, so nothing changes for anyone installing these plugins, which is what the label is for. First real use of the escape hatch created in this same PR. * Fix the three findings from this PR's review A local uv setting leaked into all four new lockfiles. /etc/uv/uv.toml on ToB machine images sets exclude-newer = "1 week", so every lock carried an [options] block with exclude-newer-span = "P1W" and pinned versions resolved a week stale — diverging from constant-time-analysis/uv.lock, which predates this PR and has no such block. Regenerated with UV_NO_CONFIG=1. That cooldown is the org's supply-chain posture and it belongs in dependabot.yml's 'cooldown: default-days: 7', where it already is; baking it into committed lockfiles was my environment leaking, not a decision. "EVERY directory here must carry a committed uv.lock" was enforced by a comment, which is precisely the anti-pattern AGENTS.md tells people to avoid. Now a validator check: it parses the uv ecosystem block out of dependabot.yml and asserts a uv.lock beside each listed directory. Scoped to that block rather than grepping for '- /plugins/...' so a future ecosystem's paths are not swept in, and it errors if the block exists but no directories parse out — otherwise the checker could inspect zero items and report clean, which is the exact failure it exists to prevent. Three self-test fixtures, and verified by deleting a real lockfile and confirming CI would go red. The Dependabot exemption keyed on github.actor, which on a synchronize event is whoever pushed. A human adding one commit to a Dependabot branch would re-arm the version check and turn the PR red — making the follow-up bump mandatory exactly where the comment says it is discretionary. Keyed on PR authorship now. |
||
|
Dan Guido
|
8ea3b6a700 |
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 let |
||
|
William Tan
|
f09e5c729a |
Remove legacy codex compatiblity scripts/shims. (#173)
* Remove legacy codex compatiblity scripts/shims. Codex supports claude plugins so this shouldn't be necessary. Add a script to test the plugin loadablility in both claude and codex * fix: resolve code review findings for PR #173 Review findings addressed (4 reviewers: pr-review-toolkit agents, Codex gpt-5.3-codex, direct diff review): P2 fixed: - Bump versions for the 5 substantively changed plugins in both plugin.json and marketplace.json (gh-cli 1.5.0 new skill, claude-in-chrome-troubleshooting 1.1.0 skill rename, modern-python 1.5.1 / skill-improver 1.0.3 hooks change, zeroize-audit 0.1.1 MCP config relocation) so clients pick up the changes - README Codex install: replace unpasteable /plugins slash-command block with verified CLI syntax (codex plugin marketplace add) - check_claude_loadability: parse_json_output now fails fast with command context on empty CLI output instead of returning None - check_codex_loadability: surface skipped RPC error messages in timeout failures instead of a bare TimeoutError P3 fixed: - Both checkers: error out when marketplace.json lists no plugins instead of passing vacuously Dismissed: - @latest CLI installs in validate.yml: deliberate; the check validates against the clients users actually run - select.select portability: CI-only script on ubuntu-latest - Divergent mcpServers validation between checkers: intentional; the Codex checker enforces the repo's .mcp.json convention Verified: ruff, prek, validate_plugin_metadata.py, and both loadability checks pass end-to-end (39 plugins, 74 skills, 2 MCP servers load in Claude Code and Codex) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Dan Guido <dan@trailofbits.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Artem Dinaburg
|
aab484c5b7 |
Add Codex-native skill installation support (#123)
* Add Codex-native skill installation support * Document Codex install commands in README * Enforce Codex skill mappings in CI * Fix review issues in Codex skill support - Fix ruff line-length violations in validate_codex_skills.py - Rewrite gh-cli SKILL.md description to third-person voice - Fix misleading error messages: describe actual symlink fix instead of referencing the user-local installer script - Add early check for missing .codex/skills/ directory - Distinguish dangling symlinks from mismatched symlinks - Add defensive ValueError handling in rel() - Add PLUGINS_DIR existence guard - Add install count and zero-install warning to installer - Add SOURCE_DIR existence check to installer Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Dan Guido <dan@trailofbits.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
Paweł Płatek
|
f4b2a7218f |
better ci validations, fix marketplace error (#85)
* better ci validations, fix marketplace error * fix lints * fix lint script * fix lint script2 * fix ruff errors * fix abs path regex * fix shellcheck lint * fix: resolve code review findings for PR #85 - Remove dead CHANGED_FILES code path (unreachable in CI) - Add version and description consistency checks between plugin.json and marketplace.json - Eliminate duplicate plugin.json reads via parse_plugin_json() - Fix bats test discovery for filenames with spaces (print0/xargs -0) - Sync marketplace.json with plugin.json for 5 pre-existing mismatches (second-opinion version, ask-questions/burpsuite/fix-review/insecure-defaults descriptions) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: gh-cli bats tests fail when gh is in /usr/bin The _no_gh test helpers used PATH=/usr/bin:/bin to exclude gh, but on Ubuntu CI runners gh is installed at /usr/bin/gh. Fix by creating a temp directory with symlinks to only jq and bash. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: improve plugin descriptions for better skill triggering - ask-questions-if-underspecified: restore trigger context ("asking questions") while keeping invocation constraint - burpsuite-project-parser: drop filler "directly from the command line", use outcome-oriented "for security analysis" - insecure-defaults: restore specific scenarios (hardcoded credentials, fallback secrets, weak auth defaults) for better matching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Dan Guido <dan@trailofbits.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> |