mirror of
https://github.com/trailofbits/skills.git
synced 2026-09-14 14:28:48 +08:00
65720f8db2ca0c1d1a1805db0dacbabc190a1aa1
178 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
||
|
kz-tob
|
8c1b2670bf |
constant-time-analysis: make the VLD version lookup portable (#277)
Both copies of the PECL install snippet ran VLD_VERSION=$(curl -s ... | grep -oP 'vld-\K[0-9.]+(?=\.tgz)' | head -1) PCRE mode is a GNU extension. Under stock macOS grep this exits 2, the command substitution yields an empty string, and the next line installs a package named `vld-` with no version at all: grep: invalid option -- P VLD_VERSION=[] would run: pecl install channel://pecl.php.net/vld- Three things let that happen, so fix all three. POSIX ERE instead of PCRE, keeping the -o semantics through a sed strip. curl -fsS so an HTTP or DNS failure is reported rather than feeding empty input downstream. And a guard, so the install runs only when a version was actually found and otherwise hands the reader the URL to check by hand. Verified against the live PECL page: old and new both return 0.19.1, so behaviour is unchanged where the old form worked at all. Both snippets were then executed with PATH=/usr/bin:/bin to force BSD tools, on the success path and with PECL unreachable. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
32ad9e6b47 |
building-secure-contracts: give five skills trigger clauses (#275)
* building-secure-contracts: give five skills trigger clauses Five of the plugin's eleven skills had descriptions that were pure capability lists with no trigger clause. The six sibling vulnerability scanners all end in "Use when auditing X", so these five lost the routing coin-flip to their own siblings and rarely fired. Each keeps its capability text and gains a "Use when" clause naming the situation, drawn from that skill's own Purpose and workflow sections rather than invented. Drop three trailing phrases that said nothing: "Provides actionable recommendations", "professional scorecard" (replaced with the roadmap the skill actually produces), and "Context-aware for both token implementations and token integrations", now carried by the trigger. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * building-secure-contracts: fix invocation docs and two triggers The README advertised every skill as a bare slash command, but the plugin ships only skills/ and no commands/, so /audit-prep-assistant is unregistered. A user who read the docs and typed the documented form got nothing — the other half of the reachability problem this branch fixes. Namespace all 22 occurrences; every other plugin README in the repo already uses that form. Anchor code-maturity-assessor to the domain. Its new clause said only "judging how mature a codebase is against a rubric", which matches a Django or Go repo just as well, and the skill would then grade it on MEV risk, transaction ordering, and decentralization. Say which side of an engagement audit-prep-assistant serves. It collided with audit-context-building on "getting ready to audit this codebase"; the two serve auditee prep and auditor onboarding respectively, and neither clause said so up front. Root README now mentions the 5 assistants, matching plugin.json, and the plugin README drops "professional scorecard" to match the description. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
43d7af8064 |
gh-cli: intercept GitHub fetches from MCP fetch tools (#274)
* gh-cli: intercept GitHub fetches from MCP fetch tools The fetch hook only matched WebFetch, so a GitHub URL fetched through an MCP fetch tool bypassed it entirely. Match `mcp__.*[Ff]etch` as well, which covers Exa's `web_fetch_exa` and equivalents from other MCP servers. A matcher alone is not enough: WebFetch passes a single `url`, while MCP fetch tools pass a `urls` array and batch several pages into one call. Read both shapes. A tool call is atomic, so one GitHub URL anywhere in a batch denies the whole call, and each offending URL is labeled with its own suggestion. Single-URL calls keep the message they had. Split the URL classification into suggest_api, suggest_raw, and suggest_github_com behind a suggest_for_url dispatcher so it can run per URL in a loop, and collapse eight verbatim copies of the clone hint into one helper. Behavior is unchanged; blob and tree merge into one branch because they emitted identical text. The plugin README's interception table now also lists the pull, issues, releases, and gist patterns the hook already handled but never documented. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * gh-cli: address review on the MCP fetch interceptor Test the matcher. Nothing exercised the regex that decides whether the hook runs at all, so a matcher firing on nothing would still pass every other suite here. matcher.bats reads it out of hooks.json and checks it against real tool names, and refuses to run against an empty matcher, which would make every match assertion pass vacuously. Widen the matcher to fetch, scrape, crawl, and extract. Firecrawl's scrape and Tavily's extract retrieve a URL like any fetch tool but carry no "fetch" in the name, so they bypassed the hook while the README promised "any MCP fetch tool". The README now names what matches. Check a string-valued `urls`. A server declaring `urls: string | string[]` sent a bare string, `arrays` dropped it, and the fetch went out unauthenticated. Listing the field twice keeps it under both shapes. `prompt` is still not scanned, so a prompt mentioning a GitHub URL does not false-deny. Give the closing note its own line when several URLs are denied; it previously trailed only the last entry. Guard assert_suggestion_starts_with against an empty reason and an empty prefix, either of which let it pass while inspecting nothing. Document the api.github.com contents, releases, and actions rows. The generic `gh api` row was actively wrong for /contents/, pointing readers at the anti-pattern the shim exists to block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Paweł Płatek
|
1004934abf |
Code improver - better skill-improver (#272)
* skill-improver 2.0.0: rewrite the loop as a dynamic workflow Replaces the stop-hook/state-file loop with /skill-improver:improve, built against the failure evidence from four manual pr-review-loop sessions (SKILL-IMPROVER-V2-HANDOFF.md): - findings ledger: stable ids, one verdict per finding, persisted every round; rejected findings are not re-litigated without new evidence, and a continued run reloads the ledger instead of re-deriving findings - completion requires the last action to be a clean review; at the cap the loop runs one review-only round and exits loudly as capped-not-converged - oscillation detectors (non-decreasing counts, 3-round recurrence, re-fixed relocation) stop the loop with a structural escalation instead of burning rounds; guarantees are never silently weakened - mechanical scope guard after every fix round: git diff vs the baseline snapshot, halt on violation, no unregistered new files at completion, fixer contract bans destructive git - finalize pass strips loop narration, collapses version churn to exactly one bump, and runs scripts/collect_metrics.py (fails on zero artifacts) Ships bundled reviewer/fixer agents (no plugin-dev dependency), an offline harness with 17-mutation self-test, pytest coverage for the collector, five paid eval cases graded on artifacts, and an A/B ablation runner against v1.1.0. Deletes the hooks, setup/cancel scripts, and the cancel command. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * skill-improver evals: seal fixtures against contamination Mounting fixtures with context.add_dirs hands every agent an absolute path into this repository — one directory walk from SKILL.md and the graders — which is the baseline-contamination failure measured in goal-prompt (#248). For this suite it is worse: the improvement loop diffs and edits the tree it is pointed at, so an in-repo fixture would have eval runs mutating the checkout and git-baselining the whole repository. Fixtures are now generated by each case's scaffold.sh inside the eval workspace (--scaffold is required for every case), the in-repo fixture/ directories are gone, and check_contamination.py gates every measured run: grader filenames in an agent trace, skill-improver/evals/ paths, or expected_outcome anywhere fail the run, and having nothing to inspect is an error. Its pytest suite proves each marker class detects a planted specimen. The ablation runner gains --expect-version, --no-publish, --keep-temp, and the contamination gate on both arms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * skill-improver: survive non-interactive callers, pin the E2 contract Three defects found by the eval pilots: - SKILL.md named the workflow 'improve'; the registry needs 'skill-improver:improve' - a caller that ends its turn while the workflow runs abandons the loop mid-round (the handoff's I9 class, reproduced in the eval harness) — SKILL.md now requires waiting/polling for the result, and forbids the inline-loop fallback that a denied Workflow tool provoked (observed rewriting the fixture's guarantee) - the E2 fixture never marked its guarantee as non-negotiable, so a fixer could legitimately resolve the overclaim by correcting the docs; the scaffold now freezes the sentence contractually in AGENTS.md, the fixer contract treats edits to contractual text as weakening, and the noisy llm grader over the 17k-char ledger is replaced by regex graders over ledger.json and metrics.json Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * skill-improver: escalate structural rejections instead of converging past them Measured in the gate-case pilot: the fixer rejected the unsatisfiable- guarantee finding with the documented rationale, the next review honored the verdict, and the loop converged in two rounds — guarantee intact, no treadmill, but 'converged: true' reads as a clean bill while the README still promises the impossible. A blocking finding that is real yet rejected as structurally unsatisfiable is the user's decision, not a parked verdict: the fixer now flags such rejections structural=true and the loop exits with a structural-rejection escalation. Covered by a new harness scenario and mutation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * skill-improver evals: mechanical decoy check, longer episode timeouts The decoy-byte-identical llm judge failed 3/3 runs whose file md5 matched the planted bytes exactly — judges do not see raw bytes, so the check is now an anchored regex proven against the specimen (and against an appended line and an edited seed). The no-relitigation rubric is corrected for the severity-gated design: a trap parked as an open minor never receives a verdict and that is the correct outcome; a mechanical refile check on metrics.json rides along. Episode timeouts rise to 3600s: loops that run 3-4 fix rounds or continue past an escalation were being killed mid-round under concurrent invocations and then graded on abandoned state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * skill-improver evals: neutralize answer-key filenames in version-comparison arms The contamination gate caught its first real incident: the v1 plugin's own SKILL.md walks its plugin root for its setup script, the listing enumerated the grafted eval tree, and 10/15 baseline runs carried every grader filename in their traces — names like trap-name-kept and guarantee-byte-identical are instructions. The ablation runner now grafts arm B with neutral case-dir and grader filenames (real case names stay in case.yaml, so reports are unaffected), the checker gains content markers (grader rubric phrasing) and an explicit --allow-listing mode for arms whose own tooling lists the plugin root, and the incident is documented in the suite README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * skill-improver: scope the finalize greps; fail the scope judge only on false claims The contamination gate flagged a finalize subagent grepping the whole plugin install directory for narration patterns — its results could touch the eval tree. The finalize prompt now scopes its greps to the scope directories under the target repository. The scope-guard last-message grader was failing exemplary reports (in-scope reroute of a broken out-of-scope test, honest artifact notes); it now fails only on affirmative false claims: out-of-scope work presented as done, or a halt presented as success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * skill-improver evals: record the measured three-arm results v2 vs main (1.0.5) vs 1.1.0, 15 runs per arm, sonnet judge, contamination gates clean on every counted result. v2 medians 1.00 on all five cases; the old versions match v2 on raw defect-fixing (both fixed pins-bite's planted bug and added covering tests 6/6) and lose on what the loop exists to guarantee: the frozen guarantee survived byte-identical only under v2, only v2 escalated instead of self-declaring completion, only v2 held version discipline, and only v2 leaves a machine-checkable record (verify-pins.sh bit 3/3). Known noise documented: episode-timeout kills and one judge call lost to a spend limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * skill-improver -> code-improver generalization * code-improver on itself * rm openai, fix readme * plugin handles one-level deep agents * fix python * code-improver: check finalize, sentinel every wave, bump both files Three P2s from the pipeline review of #272. Finalize edited the tree after the last review and the last scope check, so its narration strip, version bump, and docs pass were the only edits nothing verified — a legitimate "round 2 of the tournament" could be rewritten and the run still returned converged:true. A finalize-check agent now runs last: it scope-checks the tree, reads the finalize delta (pre-finalize.diff vs post-finalize.diff) for regressions, and writes the run's final ledger, ledger.md, and metrics — so the on-disk ledger records finalize's own outcome, and a dead finalize no longer leaves a stale one. An out-of-scope edit, a new unregistered file, a regression, or a dead check all exit with converged:false and a named halt. The REVIEWER-UNAVAILABLE sentinel was checked only on the first reviewer return, so a trampoline continuation that lost its skill returned an empty review that merged as a clean bill of health. The check moves into a helper applied to every return, and the continuation prompt now carries the contract it was expected to honor. The one-bump rule scoped the version to plugin.json "and any marketplace entry inside scope", but the manifest sits at the repository root, outside the default scope — so the headline case bumped plugin.json only and left CI red on a version mismatch. The baseline now reports the marketplace file that repeats the plugin's version, the loop brings it into scope (loudly, in notes), finalize is told to set both to the same value, and the check verifies they agree. Harness: three new scenarios (finalize check, dead finalize, marketplace bump), one for the continuation sentinel, and seven mutations covering the new guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * code-improver: fix the second review round's P2s Lint: verify-pins.sh:59 used `A && B || C`, which CI's shellcheck flags as SC2015 (local 0.11.0 does not). Rewritten as an if. 1. metrics ran as `python3 "<script>"`, the one form the modern-python shims refuse outright — anyone with that plugin installed got no metrics.json, metrics_ok false, and three graders failing for environment reasons. Now `uv run --no-project`; the collector is pure stdlib. 2. findExisting recomputed `file:line:class` and never read the id the reviewer returns, so a rejected finding re-reported at a shifted line missed its ledger entry and was re-dispatched. The id is consulted first. The coarse `file::class` rescue also merged two distinct findings of one class in one file into a single entry, dropping one silently: it now only matches an entry the current review has not already claimed. 3. The scope guard could not see out-of-scope files git does not track — they are in no index, so rewriting or deleting one (the scope-guard eval's own decoy) left no trace in `git diff`. The baseline hashes each untracked file, every surface report re-hashes the out-of-scope ones, and a moved hash, a vanished file, or a hash the check did not report is a violation. Files with no baseline hash, and any past the 50-file cap, are named in the notes as unguarded rather than passing as clean. 4. The three skills launched the loop as {name: "code-improver:improve"}; the Workflow tool resolves `name` against built-in and project workflows, so a marketplace-installed plugin workflow may not answer to it — and each skill forbids an inline fallback, so the entry point would dead-end. They now resolve workflows/improve.js (plugin root, Codex root, then a bounded find) and pass scriptPath, with the workflow name as a last resort. pr-improver's allowed-tools also gained the TaskOutput/TaskStop its own polling paragraph requires. Harness: scenarios for the shifted-id match, the untracked-content guard, and the collector invocation; 46 mutations, all biting. README/skill claims updated to what the guard now checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Paweł Płatek
|
311a784a10 |
c-review rewrite to workflows + do measurements - CLA clean (#257)
* c-review rewrite to workflows + evals * clean up the workflow script * rm benchmarking tools * fixes from automatic review --------- Co-authored-by: Dan Guido <dan@trailofbits.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
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
|
6ca26096b0 |
Fix the aflpp wrapper TTY requirement and its unpasteable examples (#270)
* Make the aflpp wrapper and examples runnable without a TTY The wrapper ran `docker run -ti`, which aborts with "the input device is not a TTY" whenever stdin is not a terminal, and hardcoded `--name afl_fuzzing`, so the multi-core examples collided on the second container. Drop `-t`, keep `-i`, and name the container after the wrapper's PID. Resolve the 26 `<host/docker>` placeholders to a concrete mode. Bash parses `<host/docker>` as a redirection, so none of those lines could be pasted. The two `afl-system-config` invocations become `host` alongside the `afl-persistent-config` sequence they belong to; the rest become `docker`, matching the Quick Start. `<test_case>` gets a real crash filename for the same reason. Move `watch` to the host side of the wrapper, since it needs the terminal that `docker run -i` no longer provides, and note that gnuplot is only an install step in host mode because the aflplusplus image already ships gnuplot-nox. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * aflpp: keep backgrounded fuzzers off the terminal's stdin The wrapper keeps `docker run -i`, so the docker client reads its own stdin. A backgrounded process that reads the controlling terminal is sent SIGTTIN, whose default action stops it, so the four `&` examples produced Stopped jobs rather than running fuzzers — verified under a pty: the backgrounded reader lands in state T, and `</dev/null` runs to completion. Redirect stdin in all four and say why the redirect is load-bearing. The cmplog example also wrote to secondary02.log, clobbering the secondary it tells you to start alongside it; give it its own log. Three prose corrections: the wrapper joins its arguments into one shell string rather than passing them "unchanged", so quoted paths with spaces are word-split; `docker ps` truncates COMMAND so the per-PID container names are only visible under `--no-trunc`; and the statistics table now says where those fields appear without a TTY, since no `docker`-mode example can draw the full-screen UI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014UFqJu1ada7gXjD9peo1rX --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Dan Guido
|
43d16a56d8 |
zeroize-audit: give SKILL.md a run entry point and link the Rust patterns reference (#267)
* zeroize-audit: give SKILL.md a run entry point, link Rust patterns
SKILL.md described what the skill finds but never told the model how to
start a run: prompts/task.md, which holds the phase loop the whole
pipeline hangs off, appeared only as a parenthetical in a table caption.
Add a "How to Run" section that maps a user request onto collecting the
inputs, reading task.md and system.md, executing the phase loop over
workflows/phase-{N}-*.md, and returning final-report.md, plus the
orchestrator-state.json resume path.
references/rust-zeroization-patterns.md had no inbound reference from
anywhere in the plugin. Link it from SKILL.md's detection strategy and
from the two Rust analyzer agents, each pointing at the section covering
the patterns that agent's scripts match: Section A/B from
2b-rust-source-analyzer, Section C from 3b-rust-compiler-analyzer, with
Section D flagged there as a coverage gap a clean run does not rule out.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Correct the completeness claims in the new reference links
Review found the link text overstated what the pattern reference covers,
in a way that risks suppressing real findings. Verified against the
scripts and corrected:
- Section C documents 12 patterns, not every pattern the three scripts
match: check_mir_patterns.py defines 8 detectors against 3 entries, and
check_llvm_patterns.py emits classes for secret return values and
by-value aggregate arguments with no entry at all. The 3b agent now
says Section C is a subset and that a pattern with no entry is still a
valid finding carrying the script's own evidence, so a missing entry
cannot read as "not a real pattern".
- Section A is not a 1:1 map of the 2b bullet list: the
missing-zeroize-dependency check has no entry, and A6 (ManuallyDrop<T>
field) has no bullet. Counts coincide at 12; contents do not.
- 34 of the 40 patterns are detected; Section D's 6 are known gaps, so
SKILL.md no longer calls all 40 "what the tooling looks for".
- C-ASM3 and C-ASM4 carry no snippet, so "each entry gives a reproducing
snippet" is now "most also give".
Also fill in the max_tus and mcp_timeout_ms defaults from
schemas/input.json, which the new "leave other fields at their defaults"
step depends on, and add the three reference files missing from the
README index, including the one this branch de-orphaned.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* zeroize-audit: register the agent dispatches and route coverage gaps to the report
The run path this PR documents dispatches every agent by bare name — twelve
sites across the phase workflows, none carrying a subagent_type — so the first
thing a user hits after following the new "How to Run" section is an
unregistered dispatch. Namespace all twelve as `zeroize-audit:<agent>`, matching
rust-review and the names the agent files actually declare.
The Section D paragraph told 3b to record unaudited patterns in notes.md, but
the report assembler reads only the *-findings.json files, so a crate using
`Arc<SecretKey>` or a `static LazyLock<MasterKey>` produced a final report
indistinguishable from a crate using neither. Write them to coverage-gaps.json
instead, have the assembler read it, and require Analysis Coverage to state
either the gaps or that none were reported — a missing line reads as full
coverage.
2b told the agent to use a Section A snippet to judge whether a flagged type "is
a false positive", with nothing in the file permitting or recording a drop; 3b
already carries a "never drop or downgrade" guard. Give 2b the same guard: a
weak match is a needs_review finding whose evidence says how the code differs,
never an omission.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014UFqJu1ada7gXjD9peo1rX
* zeroize-audit: drop the duplicated merge paragraph in the assembler
The Step 1 collection section carried the same instruction twice, the second
copy predating the rust-compiler-analysis directory it omits.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014UFqJu1ada7gXjD9peo1rX
* zeroize-audit: arm the dispatch check and make the coverage survey real
The namespaced dispatches were written as `` `subagent_type: zeroize-audit:X` ``,
which matches neither validator extractor — both need a quote or backtick
immediately after the colon — so validate_subagent_dispatch inspected zero sites
in this plugin and a regression back to a bare name would have passed CI. Quote
the values; the validator now sees all twelve, and reverting one to a bare name
fails the build.
coverage-gaps.json was read as item 4 of Step 1, but Mode Branching runs Step 1
in interim mode only while Analysis Coverage is Step 6, final-only — so the file
was read, discarded, and the report still said "no coverage gaps were reported".
Read it where it is used instead.
Nothing told 3b to perform the Section D survey: the paragraph sat above Step 1,
the numbered steps never looked, and the agent reads only MIR/IR/assembly while
five of the six D patterns are source constructs. Add Step 6 with the grep
markers and the sensitive-type filter, require the file unconditionally so an
empty survey is distinguishable from no survey, and declare it in the phase-2
workflow, the always-write list, system.md's layout, and the report template.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014UFqJu1ada7gXjD9peo1rX
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com>
|
||
|
Dan Guido
|
922ec62527 |
Fix firebase-apk-scanner scanner.sh path and command handoff (#269)
* Fix firebase-apk-scanner scanner.sh path and command handoff
scanner.sh sat at the plugin root, but SKILL.md invoked it as
{baseDir}/scanner.sh, where {baseDir} is the skill directory, so the skill's
central action pointed at a nonexistent path. Move the script into
skills/firebase-apk-scanner/ with git mv so the path resolves; the SKILL.md
references are unchanged.
The firebase-apk-scanner skill sets disable-model-invocation: true, so the
model cannot invoke it and the command body's "Invoke the skill" instruction
was a dead end. Rework commands/scan-apk.md to read and follow the skill's
SKILL.md workflow directly, resolving the skill base directory via the plugin
root environment variable.
Fix README usage: the command is /trailofbits:scan-apk (not /firebase-scan),
and the standalone script path is now skills/firebase-apk-scanner/scanner.sh.
Bump version 2.1.1 -> 2.2.0.
Claude-Session: https://claude.ai/code/session_014UFqJu1ada7gXjD9peo1rX
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* firebase-apk-scanner: make the command reachable and stop reporting empty scans as clean
Claude Code ignores `name` in a command file, so `name: trailofbits:scan-apk`
never took effect and `/trailofbits:scan-apk` — the only entry point the README
documented — was unreachable. Commands namespace by plugin directory name, as
insecure-defaults already does with no `name` key, so drop the key and document
`/firebase-apk-scanner:scan-apk`.
A directory holding no .apk files ran the loop zero times and still printed
"Vulnerable: 0" in green at exit 0, and an APK that failed to decompile counted
toward "Total APKs" while counting toward nothing else, so "1 scanned, 0
vulnerable" covered an APK that was never tested. Both now fail: an empty
directory exits 1, an all-failed run exits 1, and a partial failure reports
"Failed to scan: N" and carries failed_apks in scan_report.json. The skill's
summary table gained the matching row. Also match *.APK, which was silently
skipped and fed the zero-item pass.
The summary block passed colors as printf arguments while the color variables
held a literal \033, so those lines printed the escape as four characters. Use
ANSI-C quoting so the variables carry real ESC bytes in both positions.
Give the command a plugin-root fallback and an abort, matching c-review, so it
does not dead-end under Codex where CLAUDE_PLUGIN_ROOT is unset, and say that
$ARGUMENTS in the workflow file is literal text to substitute rather than a
shell variable to expand.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014UFqJu1ada7gXjD9peo1rX
* firebase-apk-scanner: stop calling an untested APK secure
extract_firebase_config always writes a config file, emitting [] for every field
it found nothing for, so the CONFIG_FAILED branch only fires if the write itself
fails. The case that actually happens — the APK decompiles but carries no
Firebase config, because it is obfuscated or does not use Firebase — skipped
every endpoint test on its own guard, left apk_vulnerable false, and wrote
SECURE. Output was "Total APKs: 1 / Vulnerable: 0" in green at exit 0 with zero
endpoints probed, byte-identical to a real clean scan.
Detect the all-empty config, mark the APK NO_CONFIG, count it in UNTESTED_APKS,
and report it on its own line and as untested_apks in scan_report.json. A run
where every APK either failed or had no config now exits 1, since nothing was
tested. The skill's summary table gained the row and says what NO_CONFIG means.
Verified with a stub apktool that decompiles to an empty tree: before, exit 0
and status SECURE; after, exit 1, status NO_CONFIG, untested_apks 1.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014UFqJu1ada7gXjD9peo1rX
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com>
|
||
|
Julius Alexandre
|
293fb74c31 |
Add modern-cpp plugin for C++20/23/26 best practices (#142)
* Add modern-cpp plugin for C++20/23/26 best practices
Modern C++ skill guiding Claude toward modern idioms with a security
emphasis. Mirrors modern-python in spirit but focuses on language
standards rather than toolchain.
Features tiered by practical usability:
- Tier 1 (Use Today): C++20/23 features with solid compiler support
- Tier 2 (Deploy Now): Compiler hardening, sanitizers, hardened libc++
- Tier 3 (Plan For): C++26 reflection
- Tier 4 (Watch): Contracts, std::execution
Includes SKILL.md entry point + 6 reference docs:
- anti-patterns.md (30+ legacy-to-modern swaps)
- cpp20-features.md (concepts, ranges, span, format, coroutines)
- cpp23-features.md (expected, print, deducing this, flat_map)
- cpp26-features.md (reflection, contracts, memory safety)
- compiler-hardening.md (flags, sanitizers, hardened libc++)
- safe-idioms.md (security patterns by vulnerability class)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Remove .codex/ sidecar
AGENTS.md bans runtime sidecars: Claude marketplace metadata is the single
canonical source and Codex reads it through that compatibility. The rule landed
in
|
||
|
Dan Guido
|
7be90d6e55 |
Close unterminated fences in cairo, algorand, and ton scanner skills (#268)
* Close unterminated fences in three chain scanner skills
The cairo, algorand, and ton scanner SKILL.md files each opened a code
fence for the "Example Output" report and never closed it. Everything
after that point - sections 6 through 12, roughly two thirds of each
skill - rendered as one code block instead of as markdown. Each file
also carried three sections numbered "## 5.".
Close the fence at the end of the surviving example report in each file,
then renumber the H2 sections 1..12 in document order so they are unique
and increasing. This matches solana-vulnerability-scanner, the fourth
sibling, which was repaired the same way in #160.
The example reports themselves are still truncated: cairo retains only
its banner line, and algorand and ton both announce two findings but
show one. That content was already gone in the initial import (
|
||
|
Dan Guido
|
1e0cc133f4 |
trailmark: pass --language to graph-evolution's diff calls (#266)
* trailmark: pass --language to graph-evolution's diff calls `trailmark diff` defaults `--language` to `python`. On a non-Python target it exits 0 and writes well-formed JSON with empty `nodes`, `edges`, and `entrypoints` arrays, which the skill's own "stop if empty" guard read as "no changes" rather than "wrong language". Both call sites now pass `--language auto`, mirroring the `language="auto"` default that Phase 2's `build_and_export` already uses, and the guard text says to confirm the language before treating an empty diff as an unchanged codebase. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * trailmark: fix the two remaining bare diff calls and the stale empty-diff guard The core skill's Quick Start (SKILL.md:147) and query-patterns.md:223 still ran `trailmark diff` without `--language`, hitting the same silent python default this PR fixes in graph-evolution — worse, the entrypoints line directly above each passes `--language auto`, so the omission read as deliberate. Pass `--language auto` at both. With `auto` in the documented command, "empty most often means the language was wrong" no longer names the likely cause, and "compare the two outputs" gives no decision rule. Key the empty-diff guard on Phase 2's graph summaries instead: empty diff plus healthy node counts on both snapshots is genuine stability, empty diff with a (near-)zero count means the parse missed the code. The checklist item now states that checkable condition rather than a self-attested "language was confirmed". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014UFqJu1ada7gXjD9peo1rX --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Scott Arciszewski <147527775+tob-scott-a@users.noreply.github.com> |
||
|
Dan Guido
|
9b2813356e |
static-analysis: resolve SARIF severity from the rule, not just result.level (#271)
* static-analysis: resolve SARIF severity from the rule result.level is optional in SARIF 2.1.0 and CodeQL never emits it: severity lives on the rule as defaultConfiguration.level and the result inherits it. sarif-parsing read result.level directly in the helper, the jq reference and the SKILL, so the documented CI gate counted zero errors on a CodeQL run however many it found. resolve_level() now joins the rule by ruleIndex, falls back to ruleId, and returns result.level when present, "warning" when neither states one, and "none" for a kind other than "fail" so passing compliance records do not inherit an error. The jq queries and every SKILL example resolve the same way. compute_fingerprint() hashed the basename alone, so the same rule at the same line in two directories collided and deduplicate() dropped the second finding. It now hashes the whole normalized path. Two fixtures and a pytest suite pin both: fixtures/codeql-no-level.sarif holds one error reachable only through its rule, fixtures/levels-on-results.sarif holds one error on the result, and the suite runs the documented jq gate over both so the docs cannot drift from the helper. Fixes #262 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * static-analysis: reject a negative ruleIndex in the jq resolver SARIF writes ruleIndex: -1 for "no rule", and $rules[-1] in jq is the last element, so the documented function labelled those results with whatever severity the final rule in the array happened to carry. The Python resolver already rejected it through its 0 <= index < len(rules) bound; the jq copies now require >= 0 too, and a test pins both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * static-analysis: run the documented GitHub Actions gate in the suite The workflow step inlines its own copy of the resolver, since a workflow has no shell variable to paste LEVEL_FN into, so comparing the LEVEL_FN blocks left the one artifact issue #262 named untested. The suite now extracts that step's jq program and runs it over both fixtures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * static-analysis: coalesce null kind and drop the misleading pysarif level example resolve_level read `result.get("kind", "fail")`, which only defaults when the key is absent; an explicit `"kind": null` returned "none" and hid a real error, where the jq gate's `// "fail"` coalesces it to a fail. Coalesce null to "fail" so the two resolvers agree, and add a null/fail regression test the buggy form fails. The pysarif Strategy 2 example computed `result.level or rule_levels.get(...)`, but pysarif fills a missing result.level with "warning", so the rule-inheritance fallback was dead code and a CodeQL error printed as "warning". Drop it and point severity gating at Strategy 1's level() or resolve_level(), which resolve from the rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014UFqJu1ada7gXjD9peo1rX --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.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>
|
||
|
Paweł Płatek
|
07bce8a2c8 |
Add goal-prompt plugin for copy-ready /goal commands (#248)
* Add goal-prompt plugin for copy-ready Codex /goal commands Migrated from trailofbits/codex-skills#6, converted from the Codex sidecar layout (.codex-plugin/, .agents/marketplace.json) to this repo's canonical Claude plugin structure, which Codex loads through marketplace compatibility. Both loadability checks pass. The skill drafts a goal-mode objective and pipes it through a deterministic stdlib-only formatter that collapses whitespace to one line and rejects output over the 4,000-character /goal cap, with a pytest suite covering normalization and both failure modes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add goal-prompt plugin for copy-ready /goal commands Migrated from trailofbits/codex-skills#6, converted from the Codex sidecar layout (.codex-plugin/, .agents/marketplace.json) to this repo's canonical Claude plugin structure, which Codex loads through marketplace compatibility. Works with goal mode in both Claude Code and Codex; both loadability checks pass. The skill drafts a goal-mode objective and pipes it through a deterministic stdlib-only formatter that collapses whitespace to one line and rejects output over the 4,000-character /goal cap, with a pytest suite covering normalization and both failure modes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: encode goal-mode limits and termination-contract guidance Researched both harnesses' official docs. Claude Code's /goal condition and Codex's stored objective are each capped at 4,000 characters, and Claude Code's evaluator is a transcript-only small model that cannot run tools — so a condition is judgeable only when the agent runs the check and shows the output. SKILL.md now separates the plugin's two jobs: draft a termination contract (end state not activity, stated check with transcript-visible proof, invariants including never weakening the gate, stop bound or blocked clause, AND not "or"), then format it. Platform mechanics with sources live in references/goal-mode.md. The formatter gains one deterministic non-fatal check: it warns when the objective has no numeric stop bound and no blocked clause, the documented top failure mode for goal loops. Tests cover the new detection both ways. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: fold in Trail of Bits field guidance from codex-config The codex-config README's /goal section adds three things the plugin was missing. The drafting checklist gains the work-order fields it lacked: scope to read first, and for multi-checkpoint goals a final evidence deliverable plus a progress log file for durable state. A when-to-use heuristic (an instruction repeated three turns in a row belongs in the goal; chain small goals rather than one giant one). And a security-research section hardening audit goals against reward hacking: neutral wording, threat-model scoping, demonstrated attacker preconditions, known-findings checks, per-finding human review, and second-pass validation. references/goal-mode.md gains the full work-order template, the codex exec caveat, and the missing official links. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: tighten SKILL.md and README.md Same content, less prose: SKILL.md drops the meta-commentary and keeps the checklist, security hardening, formatter contract, and example; README.md explains the two jobs in two paragraphs for a human reader. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: never invent missing goal elements Every checklist element must be grounded in the user's request, the conversation, or the repository (look up the real check command, don't guess one). When nothing grounds an element, the skill still optimizes and formats what the user provided, but reports the gap in a Missing: list after the fenced block instead of fabricating a success condition that would terminate the goal on the wrong contract. The example now shows both the grounded case and the flag-the-gaps case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: fold goal-mode reference into SKILL.md The reference file mostly restated the checklist. What survives into SKILL.md: the official doc links, the Codex feature flag and interactive-only caveat, and the Claude Code resume caveat (turn bounds silently extend across resumes), placed next to the stop-bound rule it affects. Everything else was lifecycle and mechanics detail the drafting job does not need. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: close easy-outs, keep goals small Two additions that pull in opposite directions, stated together so neither wins by default. Before formatting, reread the condition as a lazy model would and close the cheapest ways to satisfy the letter without the intent: delete-or-stub, pass-on-a-subset, game-the-gate, claim-without-running. But every constraint narrows the state space the model can explore, so prefer pairing existing checks over adding constraints, collapse to one terminating criterion when possible, and drop non-goals. Outs that cannot be closed from grounded information go in the Missing: list as warnings, never as invented constraints. The security section now leads with the collapsed pattern: one criterion referencing a THREATMODEL.md that carries scope, attacker powers, severity baseline, and known findings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: strip links and config detail from SKILL.md Skill bodies are for the drafting agent: the doc links, the Codex feature-flag and codex-exec caveats, and the fenced security-goal example added context without changing behavior. The never-invent rule loses its check-command specificity. The security section keeps only what changes the drafted text: one criterion, scoping file, neutral wording, demonstrated preconditions, per-finding review, second-pass validation. The Example section stays — Anthropic's authoring checklist calls for concrete input/output examples, and this skill's output shape is the point. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: replace README wall-of-prose with a capability list Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: set Paweł Płatek as author, trim CODEOWNERS Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: add with/without ablation evals Three cases, each the same bare "Improve this goal: ..." prompt run with and without the plugin so the score delta isolates what the skill adds: grounded-migration (single-line output, check command grounded in the fixture's package.json, stop clause), ungrounded-vague (no invented metrics or benchmark commands when nothing grounds "faster"; gaps flagged back), easy-out-closed (the user's grep-only success check is deletable-code-satisfiable; the goal must pair it with the fixture's real test suite). Graders judge the returned artifact against fixture contents: regex for the mechanical stop clause, LLM graders for grounding, invention, and easy-out closure. Also adds "improve" to the skill's trigger list since the eval prompts (and users) phrase it that way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: fix eval graders to judge only the /goal line Live ablation runs exposed the grader bug AGENTS.md warns about: the haiku judges failed correct answers because the Missing: list's illustrative examples ("e.g. p95 under 300ms") were read as inventions and as extra command candidates. Graders now scope judgment to the single line inside the fenced block and explicitly exempt the gap list; the README pins --judge-model sonnet since haiku cannot follow that scoping. Measured results (4 runs/arm/case): plugin arm 100% across all graders; bare arm bimodal — the deterministic stop-clause regex alone failed half its grounded-migration runs. On strong models the plugin's demonstrated value is consistency, recorded as such in the README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: record baseline contamination in eval results Checked our eval logs against trailofbits/skills-internal#546 and we were hit: baseline-arm responses cite the fixture's absolute repo path and reproduce SKILL.md sentences verbatim ("scope to read first", "terminates on the wrong contract"), so the no-plugin arm read the plugin under test off disk and imitated it. Our runs were more exposed than the issue's report — the baseline had full Bash, not just ungated Read/Glob. The README now marks measured deltas as lower bounds, notes the uncontaminated baselines scored 0, and prescribes --keep-temp plus a leakage audit of baseline traces until the harness can deny the baseline Read access to the plugin directory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * goal-prompt: isolate eval fixtures from the repo, gate on contamination The ablation baseline could read the skill under test: add_dirs handed every agent an absolute path into this repo, one directory walk from SKILL.md and the graders, and baseline responses reproduced SKILL.md sentences verbatim. Contaminated baselines imitate the skill, deflating the delta to near zero. Fixtures are now generated inside the eval's temp scaffold by each case's scaffold.sh (run with --scaffold), so no agent sees a repo path; a kept-temp probe confirmed the scaffold gets its own home/, config/, and cwd/ with zero repo paths in the baseline trace. check_contamination.py fails a run whose baseline responses contain the plugin path, script name, or verbatim SKILL.md phrases, and fails when it has nothing to inspect; its pytest suite proves both directions. Clean rerun: plugin arm 1.00 everywhere; baselines 0.40/0.29/0.25; mean delta +0.69 (was +0.04 contaminated). The checker flags the old contaminated result and passes the new one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix owner * goal-prompt: run scripts through uv, fix the dead contamination marker Two fixes on top of #248. The skill's only operative command was `python3 {baseDir}/scripts/ format_goal_prompt.py`. The modern-python plugin in this same marketplace ships PATH shims that reject a bare `python3 <script>`, so the Format step failed for anyone who has it installed — and #255's narrowing does not help, because a bare script run is exactly what `uv run` replaces and stays intercepted by design. Now `uv run --no-project`, matching the form the Makefile already uses in all four of its invocations. Verified with the shim on PATH: byte-for-byte identical output to the old command run shim-free. Same fix in evals/README.md. The `scope to read first` contamination marker could never fire: SKILL.md writes `**Scope to read first**` and the match was case-sensitive. Every existing test quoted the marker's own lowercase spelling rather than the file's, so 22 tests passed over a dead marker. Matching is now case-folded, the markers are split into path and phrase groups, and two tests guard the recurrence — one asserts every phrase marker is still present in SKILL.md, the other quotes SKILL.md verbatim. Both were mutation-tested: reverting either fix fails exactly one of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Dan Guido <dan@trailofbits.com> |
||
|
Daniel McCarney
|
4db56eff30 |
trailmark: update vector-forge skill w/ wycheproof tooling (#251)
* trailmark: update vector-forge skill w/ wycheproof tooling We landed some helpful tooling for adding/editing/replacing vector data upstream in Wycheproof that is helpful for LLMs to know about. This commit updates stale references to the Python based tooling and points to the new `vectorgen` tool/docs. * version 0.10.0 -> 0.10.1 --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> Co-authored-by: Dan Guido <dan@trailofbits.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
|
c199e0cc7d |
Narrow the modern-python shims to the commands uv run replaces (#255)
* Narrow the modern-python shims to the commands uv run replaces Closes #207. The shims sit on PATH, so they intercept every subprocess any tool spawns, not just what Claude types. Two of the intercepted invocations were not package management at all, and blocking them broke real tooling. `uv pip` now passes through when it carries --project, --directory or --target. Those say a tool is building an environment it owns, where `uv add` is not the available advice: prek installs every hook with `uv pip install --project / --directory <cache>`, so the refusal made `git commit` fail in any repo whose hooks need a Python environment. A bare `uv pip install requests` is still refused. `python -c`, `python -m <module>` and `python -` now reach the real interpreter. None of them resolves a script against a project's dependencies, which is what `uv run` exists to do, and `uv run python3 -` is not a drop-in replacement inside a pipeline. `python -m pip` stays intercepted, as do bare `python` and `python script.py`. Passing anything through is new for the python shim, which previously ended every branch in exit 1, so it gains the same skip-my-own-dir PATH walk the uv shim already had. That walk now uses parameter expansion rather than basename, because the one case where it must report failure is a PATH holding nothing but the shim, where shelling out to coreutils fails first with a confusing error. Verified by A/B on the two symptoms #207 reports, running each suite against the old shim and the new one: - zeroize-audit's rust-regression smoke test: FAILED at line 72 before, "Rust regression smoke checks passed." after. - prek hook installation from a cold cache: refused before, "check json Passed" after. bats goes from 19 cases to 38. Five python cases inverted rather than being deleted: the ones asserting that -c and -m are refused now assert they run. AGENTS.md's note on `make shell-suites` is corrected rather than removed — the #207 interceptions are gone, but the target still fails because variant-analysis invokes `python3 <script>.py`, which the shim intercepts by design. That one belongs to variant-analysis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Decide on the mode selector, not on argument position Two gaps in the narrowing, both from review. `uv pip install --help` documents `-t, --target <TARGET>`, so the short form has to be exempt alongside the long one. Without it the same tool-managed install was allowed or refused depending on spelling. The python shim read only $1 to find the mode selector, so `python -u -c 'code'` was refused while `python -c 'code'` ran, even though they are the same invocation. It now steps over interpreter flags to find the selector, giving `-W`, `-X` and `--check-hash-based-pycs` the two slots they take. `-u -m pip` is still refused, and so is `-u script.py`: a script path is what `uv run` replaces regardless of what precedes it. bats 38 -> 43. Both #207 regressions re-verified after the restructure: zeroize-audit's smoke test passes and prek installs hooks from a cold cache. Not fixed here, deliberately: `uv --no-progress pip install requests` still slips past the refusal, because the subcommand check reads $1 as well. Parsing that correctly means knowing which uv global flags take a value, and getting it wrong would refuse a command that works today. The failure mode is a missed nudge rather than a breakage — the real uv runs and behaves correctly — so it does not belong in a change whose purpose is to refuse less. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Dan Guido
|
d537432501 |
Pin the node test reporter in semgrep-rule-variant-creator (#254)
`node_test()` ran `node --test` with no `--test-reporter` and then read `# fail 0` and `# pass N` out of the output. Those are TAP markers, and node's default reporter is not a stable contract: node 22 emits tap when stdout is not a TTY, node 23+ emits spec, which prints `ℹ pass N`. CI pins node 22 so it stayed green, while `make check` failed 28 tests for anyone on a current node. The suites themselves were always fine. Pinning `--test-reporter=tap` fixes it. Confirmed load-bearing by reverting the flag: 28 failed, 54 passed, exactly the reported symptom, and 82 passed with it back. While here, `test_node_suites_pass` asserted `"# fail 0" in output` immediately after `assert code == 0`, which only restates the exit code. Replaced with a floor on the reported pass count, because the failure it could not see is a suite that stopped running its tests and passed anyway. Node makes that case subtle: a file containing no tests still reports `# tests 1 # pass 1`, counting the file itself, so a plain non-zero check would not have caught it either. Verified against a gutted suite, which reports 1 against a floor of 10. 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> |
||
|
dependabot[bot]
|
2bb398210d |
deps: Bump ruff (#252)
Bumps the python-minor-patch group with 1 update in the /plugins/trailmark/skills/slicing-code-context/scripts directory: [ruff](https://github.com/astral-sh/ruff). Updates `ruff` from 0.16.1 to 0.16.2 - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](https://github.com/astral-sh/ruff/compare/0.16.1...0.16.2) --- updated-dependencies: - dependency-name: ruff dependency-version: 0.16.2 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: python-minor-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
kz-tob
|
3e433ffee0 |
Exclude supply-chain eval fixtures from Dependabot scans (#245)
* Exclude supply-chain eval fixtures from Dependabot scans The supply-chain-risk-auditor evals assert on deliberately stale manifests — requests==2.19.0, flask==1.0.2, axios@0.21.0 are the findings the cases expect. A Dependabot bump would leave the cases passing with nothing left to detect. No block scans them today: the uv directories are listed explicitly, and the Cargo.toml and package.json fixtures have no matching ecosystem entry. This guards against a later edit widening that list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Update dependabot.yml added the slash * Update dependabot.yml removing slash because dependabot breaks with it --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Dan Guido
|
a551f0b5f7 |
Revise links in README 'Also see' section
Updated the 'Also see' section with new links. |
||
|
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> |
||
|
dependabot[bot]
|
4db88ee79d |
deps: Bump ruff (#246)
Bumps the python-minor-patch group with 1 update in the /plugins/trailmark/skills/slicing-code-context/scripts directory: [ruff](https://github.com/astral-sh/ruff). Updates `ruff` from 0.16.0 to 0.16.1 - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](https://github.com/astral-sh/ruff/compare/0.16.0...0.16.1) --- updated-dependencies: - dependency-name: ruff dependency-version: 0.16.1 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: python-minor-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Scott Arciszewski <147527775+tob-scott-a@users.noreply.github.com> |
||
|
Paweł Płatek
|
cfea213cce |
skill-improver: remove the trash dependency and the duplicate command surface (#249)
* skill-improver: remove the trash dependency and the duplicate command surface Fixes two issues: - #244: hooks/stop-hook.sh and scripts/cancel-skill-improver.sh called `trash`, which is absent on stock Linux; under `set -e` the stop hook died before removing the state file even when the completion marker was detected, so the loop could never terminate, and the cancel escape hatch failed too. lib.sh now provides remove_state_file(), preferring trash and falling back to rm -f. The helper is not named `trash` because `command -v` resolves shell functions, so a same-named wrapper would always take the trash branch and still die. A hermetic regression suite runs both scripts on a stub PATH without trash; it fails 5/8 assertions on the old code. - #199: commands/skill-improver.md and skills/skill-improver/SKILL.md both registered as skill-improver:skill-improver, listing twice in the model-facing skill index. Commands are the legacy surface since the commands/skills unification, so the command's path-resolution and setup steps moved into SKILL.md (argument-hint, scoped Bash rule for the setup script) and the command file is gone. Invocation stays /skill-improver. SKILL.md now also tells the model to skip setup on stop-hook continuation prompts so a mid-loop re-trigger cannot start a second parallel session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * pr-review done * rm regression test --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Akshay K
|
304c81a8ce |
static-analysis: make the codeql skill's guards real and add tests (#225)
* fix(codeql): make the skill's guards real, add tests, trim the prose Every verification step piped its command to a formatter and used the pipeline's exit status, which without pipefail belongs to the formatter, not the command. Nine sites. The sharpest was the arm64e detection: `EXIT_CODE=$?` after `| tee` compared against 137, a value it could never hold, so the check underpinning Essential Principle #5 and three Rationalizations could not fire. The build workflow now runs check_db_quality.py as its Step 4 exit condition rather than describing metrics it never compared to a threshold, and suite generation aborts on zero resolved queries. Two blocks that were invalid bash — an else branch containing only comments — are fixed. Log helpers move to scripts/build_log.sh, sourced by the workflow and by the three reference docs that use run_logged. They were previously defined in one markdown file and used from three others, so those blocks failed standalone. As the skill's first .sh file it is also the first thing here that `make shell` lints. Sourcing it now checks the log is writable: under pipefail an unwritable log made tee's failure the pipeline's, so run_logged returned 1 for a build that succeeded and the method ladder walked to --build-mode=none blaming CodeQL. Suite generation moves to scripts/generate_suite.sh, which takes the mode as its argument. Both modes had been copy-pasted into two reference docs sharing some 25 lines of identical scaffolding — guards, the third-party pack loop, the excludes, the verification call — none of it lintable where it sat. The tests now run the script instead of extracting bash from markdown, and one of them fails if either doc inlines a generation block again. check_db_quality.py counts project files under the source root recorded in codeql-database.yml instead of against a hardcoded prefix list that only knew where macOS keeps its toolchain. src.zip stores each file at its absolute path minus the leading separator, so the recorded root is a prefix of the project's entries and of nothing else — verified against a database built by CodeQL 2.25.6. It also resolves the summary-versus- severity preference per extractor: a single `extractor-failures: 0` used to suppress the fallback for every other language in the database. run-analysis.md had its own database discovery loop, which SKILL.md states the workflows do not restate. It also lacked SKILL.md's `codeql resolve database` filter, so a marker file left by a failed build could be selected as a database. It now uses the canonical block. Twenty-odd snippets across language-details, threat-models, and performance-tuning created and analysed a literal `codeql.db` in the working directory — the exact shortcut SKILL.md lists as a Rationalization to Reject, and one its Success Criteria forbid. They use "$DB_NAME" under $OUTPUT_DIR. Adds six hermetic test suites needing no CodeQL install, plus the first tests that execute build_log.sh rather than reading it. They cover the shell in every markdown block, both suite generators, the quality thresholds, the .qls templates, and the exit-status claim the build ladder rests on. Corrects the run-all coverage figures against cpp-queries 1.8.0: the pack holds 515 alert queries, not 510, and run-all leaves 307 of them unrun, not 302 — 13 of those are Security/CWE queries, which the prose described as harmless refactoring metrics. Trims the skill's markdown from 2867 lines to 2748, and SKILL.md from 269 to 257. Out: the When NOT to Use section that #216 dropped repo-wide, five Essential Principles that restated their own Rationalization, two literal prompt mock-ups, three Reference Index rows duplicating the workflow table above them, three identical qlpack.yml blocks, and code fences in performance-tuning and threat-models that each carried a single flag. Replaces three approval prompts with one confirmation gate, and drops allowed-tools entries for tools that no longer exist. Bumps static-analysis to 1.3.0. * fix(codeql): address review findings on shell-block scope and suite claims Each markdown block runs in a fresh shell, so scalars and sourced functions cross it no better than arrays do. build-database.md now says to re-source build_log.sh and re-set DB_NAME in every block using run_logged: without it run_logged exits 127, the method ladder reads that as a failed build method, and it walks to --build-mode=none having never invoked CodeQL. run-analysis.md Step 4 re-establishes DB_NAME, RAW_DIR and SUITE_FILE for the same reason -- under set -u it aborted with "unbound variable" before analysis started. create-data-extensions.md was the third caller of database discovery and still used a bare find for codeql-database.yml, which selects a marker left by a build killed mid-run; the queries then return nothing and Step 3 reports coverage as adequate. It now uses find_databases.sh like the other two. quality-assessment.md sources build_log.sh in the Collect Metrics block, so a failed quality gate is recorded rather than silently dropped by a command-not-found, and the raised-threshold override logs inside the if -- it previously wrote "raised to 15%" even when the re-run still failed. generate_suite.sh no longer describes the run-all suite as every security, experimental and quality query: it imports two suites totalling 219 of the pack's 515 alert queries, as run-all-suite.md documents. Changed in the doc template too, which test_generation_scripts.py pins to the script's output. The two remaining review findings were already fixed on this branch and needed no change: quality-assessment.md assigns ERROR_RATIO from the script's JSON before reading it, and extractor_error_count() resolves the summary-versus- severity preference per extractor rather than once for the tree. * style(codeql): trim comments in the shared shell scripts build_log.sh carried more comment than code. The consequence of an unwritable log -- the ladder walking to --build-mode=none after a build that succeeded -- was stated twice within ten lines; it is stated once now. find_databases.sh and generate_suite.sh get the same treatment. Comments only: the diff contains no code lines. 435 tests pass unchanged. * feat(codeql): ship /static-analysis:codeql-build as a dynamic workflow The build is the part of this skill with real judgement and no user in it: try a method, read the failure, apply a fix, retry, escalate. That loop now runs unattended as workflows/codeql-build.js, beside the four workflows already on main. Three phases. Detect resolves the output directory and profiles the language, build system and macOS arm64e state. Build walks the ladder. Assess runs check_db_quality.py and applies the improvements from quality-assessment.md before re-running it. The ladder is deterministic and lives in the script; diagnose-fix-retry for a single rung lives in that rung's agent, where the build output it has to read already is. The agent is told not to escalate itself -- the caller owns the order, so a rung that fails is a result rather than a licence to try something else. Go and Swift never reach Method 4, which they reject outright; an arm64e Mac starts at 2m rather than spending two rungs to reach the same SIGKILL; an interpreted language does one extraction and no ladder at all. Nothing is asked. Every method failing returns no-method-succeeded, and a database that built but sits below the quality threshold returns built-below-threshold with its metrics. Whether the remaining extractor errors are confined to code nobody needs analysed is the caller's call, so the assess phase is told not to raise --max-error-ratio to make its own gate pass. A build command exiting 0 is not a database: every rung is confirmed with codeql resolve database before it counts, because finalize after a failed trace-command leaves one that resolves and holds nothing. tests/codeql_build_harness.js compiles the workflow with stubbed agents and asserts the ladder and the guards; --self-test mutates it six ways and requires every mutation to turn a scenario red. run_codeql_build_tests.sh wraps both so CI's existing shell-suite discovery runs them, with no workflow file changes. SKILL.md documents it as the unattended alternative and keeps the manual path. Database selection, analysis planning and data extensions stay in the session. * refactor(codeql): stop the docs recomputing what check_db_quality.py reports Collect Metrics parsed baseline-info.json with an inline python3 -c into BASELINE_LOC, then read .baseline_loc out of the checker's JSON into DB_LOC in the same block -- the same number, computed two ways, logged twice under two labels. The block's own comment said "Everything downstream reads $QUALITY_JSON rather than recomputing" while the recount sat sixteen lines above it. The inline parse and the print-baseline call before it are gone; the checker is the only thing that counts now, and the Quality Criteria table cites it rather than a command the doc no longer runs. Log Assessment read five variables out of Collect Metrics' shell. It runs in its own, so they expanded to empty and the log recorded "Baseline LoC:" with no number -- the same shape as the dangling ERROR_RATIO the first review found. It re-sources the helpers and re-reads the metrics, and no longer prints an expected-file count it cannot see. test_shell_blocks.py's embedded-python guard required three matches and there are two now, which is the guard working: removing the last sample of a construct must fail rather than silently leave the extractor untested. The floor moves to two, with the reason recorded. * test(codeql): drop test_suite_resolution.py, which never ran in CI Its six tests needed the CodeQL CLI and codeql/cpp-queries; CI installs neither, so every one of them reported as a skip on every run. Confirmed by reproducing CI's environment locally -- with codeql off PATH the directory gives 428 passed, 135 skipped, matching the job log exactly. Removing it leaves 428 passed, 129 skipped, and the remaining skips are per-block parametrisations of test_shell_blocks.py whose test functions do run for other blocks. What goes with it: the only check that resolved the suite templates against a real CodeQL rather than a fake one. run-all-suite.md's coverage claims -- that run-all is not the whole pack, that important-only reaches queries run-all does not -- are now prose nobody verifies, so the doc carries the command to re-derive them instead of pointing at a test file that is gone. test_generation_scripts.py's docstring no longer claims a sibling covers real-CLI resolution. * fix(codeql): Rust supports --build-mode=none; say so The language table left Rust as "check your CLI — not listed either way" and the Overview's three categories did not cover it at all, so a reader had no route for a Rust project and codeql-build.js resolved the ambiguity silently by putting Method 4 on its ladder. Settled against CodeQL 2.25.6 rather than the help text: `codeql database create --language=rust --build-mode=none` exits 0 and writes a database with `finalised: true`. Go, for contrast, fails immediately with "Go does not support the none build mode". So `--help` omits Rust the same way it omits C/C++, which the Overview already warned about; Rust joins that category and the ladder in codeql-build.js was right. * refactor(codeql): one prose copy of the quality-gate exit codes, not two check_db_quality.py's exit contract was written out four times: the script's own docstring, codeql-build.js's assess prompt, build-database.md Steps 4-5, and quality-assessment.md's Enforce the Thresholds. The first two earn it -- one is the source of truth, the other is an agent prompt that cannot read a docstring. The two prose copies are one too many. build-database.md keeps the part a reader needs at that moment (exit 1 is not a judgement call, exit 3 is) and defers the table to quality-assessment.md, which it already links and which is where someone looks for gate detail. The raised-threshold rationale there loses two lines it did not need. * refactor(codeql): assess phase reads the exit-code table instead of copying it The Assess prompt spelled out all four exit codes while its sibling phases point at a file -- Select says to read rulesets.md rather than choose from memory, and each build rung is sent to build-fixes.md. It now reads "Enforce the Thresholds" in quality-assessment.md the same way, keeping inline only the two facts that decide what it does: exit 1 is not overridable, exit 3 is a heuristic. That leaves one prose description of the contract instead of two, and a change to the script's exits reaches the agent without a second edit. Also aligns the ladder comment with the Rust finding: --help omits C/C++ and Rust, not just C/C++. * fix(codeql): pass --format=json, the flag check_db_quality.py defines The workflow's Assess phase ran `check_db_quality.py --json`. The script takes --format {text,json}, so argparse exited 2 before reading the database, and 2 is the one exit code ASSESS_SCHEMA does not describe. test_script_flags.py checks the class: it reads each script's accepted flags from its own --help and verifies every invocation across the plugin's .md and .js. The bug shipped because test_shell_blocks.py scans the skill tree and codeql-build.js sits outside it. * test(codeql): scan every block in one test instead of parametrizing over all of them Seven tests parametrized over all 65 bash blocks, which is 455 cases for seven assertions, and three of them skipped the blocks that did not qualify. That was 129 skips, and it hid a check that matched no block at all: the unpreserved-pipeline assertion had never run against the skill. Each now scans in one pass and lists every offending file:line, so a run reports all offenders rather than the first. 557 cases down to 118, none skipped. The array collector's empty case is an assertion rather than a skip. * refactor(codeql): point the workflow at build-database.md instead of restating it codeql-build.js carried its own copy of the build procedure: the arm64e detection block verbatim, the build-system command table, every method's invocation, and the output-directory logic. Both copies were live, which is how the workflow came to pass check_db_quality.py a --json flag while the doc had --format=json. Each rung now names its section of build-database.md, the way Method 2m already pointed at macos-arm64e-workaround.md. quality-assessment.md called the checker three times and re-derived its numbers with jq, unzip and grep. check_db_quality.py now reports archive_files and finalised from the two files it already reads, so one call covers the whole assessment. The fresh-shell rule was stated in five places; SKILL.md holds it once and the workflows link to it with the consequence specific to their site. test_section_pointers.py checks what this trade depends on: every "Section" in file.md pointer and every #anchor link must land on a real heading. The repo validator resolves paths, not section names, so a renamed heading would leave the pointers aimed at nothing with every check still green. Prose 1128 -> 1084 lines, codeql-build.js 390 -> 340. * fix(codeql): address PR review findings on skill paths and database selection codeql-build.js hardcoded plugins/static-analysis/skills/codeql, which only resolves in a checkout of this repo. Installed, the first build block sourced a build_log.sh that was not there and exited 127, so every rung of the ladder reported a build failure for a project that would have built. The Detect phase now resolves the directory at runtime from $CLAUDE_PLUGIN_ROOT, $CODEX_PLUGIN_ROOT, or a find over ~/.claude and ~/.codex, accepting a candidate only when scripts/build_log.sh exists. skillDir is a required schema field validated as absolute, so an unresolved path stops the run before the ladder starts. SKILL.md built FOUND_DBS in one bash fence and looped over it in the next. Each fence is a separate shell, so the metadata loop iterated zero times and the selection prompt had no language or creation time to show. The two are now one block. run-analysis.md Step 1 branched only on the zero-database case and fell through to FOUND_DBS[0] for any other count, analysing whichever database find returned first without telling the user there was a choice. It now uses the elif/else shape from create-data-extensions.md and exits with an error when DB_NAME is still unset. Two new checks cover the class rather than the instance. test_shell_blocks.py fails when a block reads an array it does not build. test_section_pointers.py fails on a repo-relative plugins/ path in the workflow, and resolves ${SKILL_DIR} pointers against the skill root so the workflow's file references stay checked: nine of them now, up from two. The rest of the review: run-all-suite.md no longer claims total coverage, which its own measurement section refutes; the analyze block expands optional flags as ${ARR[@]+"${ARR[@]}"}, since bash 3.2 treats an empty array under set -u as unbound; find_databases.sh exits 2 when codeql is absent instead of printing nothing, which auto-detection reads as "no databases, rebuild"; make -j$(nproc) falls back to sysctl -n hw.ncpu on macOS; every . build_log.sh site is || exit 1, as none of those blocks set -e; the Reference Index lists find_databases.sh and generate_suite.sh. * fix(codeql): source build_log.sh in every block that uses its helpers Each fenced block is its own Bash call, so a helper defined in an earlier one is undefined and exits 127. The build ladder reads that as a failed method and walks to the next one, reporting failure for a build that was never attempted. test_shell_blocks.py now fails any block that uses run_logged, log_step, log_cmd, log_result or LOG_FILE without sourcing build_log.sh, with fixtures pinning the detector in both directions. * fix(codeql): gate each step of the Method 3 multi-step build build_log.sh does not set -e, so the four run_logged calls ran regardless of each other: a failed trace-command still reached finalize, and the resulting database resolves while holding nothing, which the ladder reads as success. The steps now chain through if/elif, as 2m-a already does. test_shell_blocks.py fails an ungated `codeql database finalize`. * fix(codeql): stop database discovery reporting none when it found some find_databases.sh resolves each root to an absolute path, so the old -not -path '*/.*' exclusion also matched dotted ancestors: a checkout under ~/.cache or ~/.local had every database filtered out, the script printed nothing and exited 0, and the caller rebuilt from scratch. Prune dotted directories by name instead, with -mindepth 1 so a root that is itself dotted still searches. All three callers read the script through a process substitution, whose exit status is unobservable. Exit 2 (no codeql on this shell's PATH, a fresh shell per block) arrived as an empty list and was reported as "No CodeQL database found" for a project with several. Read it with command substitution and check the status. Tests cover the dotted-ancestor case, the dot-directory-below-root case the fix must not widen into, and a block scanner rejecting a process substitution around the script. * chore(static-analysis): bump version to 1.3.1 #231 bumped the plugin to 1.3.0 on main after this branch had already done the same, so merging main left HEAD and the merge base equal and the version-increment check failed. --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Eric Quintero
|
e6066e7db1 |
Rebuild supply-chain-risk-auditor around a deterministic collector (#227)
* Rebuild supply-chain-risk-auditor around a deterministic collector Replace the gh-only audit method with two stdlib-only Python scripts bundled with the skill: collect.py queries OSV, the npm and PyPI registries, the Go module proxy, deps.dev, OpenSSF Scorecard, and GitHub, and emits a JSON artifact; render.py turns it into a Markdown report of facts only. The model's job is the judgment layer on top — remediation, replacement candidates, narrative — written in report register and labeled as judgment. The old method could not deliver its own criteria: repository contributors are not registry publish rights, and gh sees no download counts or ecosystem-keyed advisories. What it measures, for npm, PyPI, and Go: - Version-matched advisories for direct dependencies, resolved from the lockfile, manifest pins, or labeled fallbacks — and for the full lockfile-resolved transitive tree (package-lock.json, uv.lock, go 1.17+ go.mod), advisories only. - Abandoned or archived upstreams, deprecated and yanked releases, npm publisher concentration, install-time script execution, and the two OpenSSF Scorecard checks that name a concrete mechanism (Dangerous-Workflow, Binary-Artifacts). Download volume, publish provenance, and security policy are reported as context, never flagged. The structure enforces its honesty rules rather than documenting them: - Every criterion resolves to assessed-clean, assessed-flagged, or unassessable-with-a-reason. Unavailable data is never evidence of risk, and every claim is bounded by a coverage table. - An empty advisory answer counts as clean only for a package proven to exist: a registry document for npm and PyPI, a module-proxy answer for Go, and for transitive lockfile entries a registry integrity hash or registry source. Everything else — private registries, git dependencies, vendored directories — is named as unverifiable with its reason, never counted clean. - Coverage must reconcile, a run that measures nothing exits non-zero instead of reporting that nothing is wrong, the renderer refuses an artifact whose flags and coverage disagree, and third-party text is escaped before it reaches a Markdown table. 85 offline tests exercise the invariants through the collector's own cache format, and the suite is mutation-checked. evals/ ships three fixtures with graded expectations; against a no-skill baseline the skill passed ~92% of skill-agnostic assertions vs ~60%, at half the wall clock, with its edge in reproducibility — the report regenerates byte-for-byte from the artifact — and self-consistency. Version 1.0.1 -> 2.0.0: method replacement. CODEOWNERS moves to @e-q. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Establish registry identity at the parse boundary; reconcile the sweep against the raw lockfile Review findings on the PR clustered at one boundary: a non-registry or malformed identity reaching the registry-keyed pipeline. Close the class, not the instances: - Dependency gains non_registry_reason, set by the parsers (npm file:/ workspace:/git/shorthand specs; PyPI direct deps whose uv.lock source is git/directory/path). One choke point in collect() marks every criterion unassessable with that reason and excludes such deps from all lookups — a same-named public package's advisories, publishers, and deprecation belong to code the project never installs. The deps stay in the report and its coverage. - Versions extracted from requirements text pass a PEP 440-shaped gate; pip-compile continuation/hash debris ("2.19.0 \") becomes unresolved instead of a version-matched claim. Measured: OSV compares garbage versions lexically, so the debris did not fail — it matched the wrong advisory ranges. - The transitive sweep excludes direct dependencies by (ecosystem, name, version), never by name: a nested copy of a direct dependency pinned at another version is this sweep's responsibility, and the name-keyed exclusion silently dropped it (measured on axios: two recovered entries, 620 -> 622). The artifact now carries a ledger counted from the raw lockfile before any exclusion — checked + unverifiable + excluded_direct must equal it, so a dropped triple fails validation instead of vanishing while the counts balance. The checked==0 guard hole is closed: zero reconciles like any number unless a reason is stated. - pip-audit runs with --no-deps --disable-pip. Measured: --no-deps alone still audited a pip-resolved transitive set, so pip was still fetching and potentially building untrusted distributions, against the tool's no-execution promise; with both flags it audits exactly the listed pins from registry metadata. Names on both sides of the cross-check are PEP 503-normalised. - Scorecard check thresholds move to model.SCORECARD_CHECKS as the single source of truth; the renderer derives its never-flags set from threshold-is-None, ending the clean-run contradiction that described the two flagging checks as "not flagged — poor precision". - Duplicate requirements prefer the runtime declaration (the dev file sorts first, so first-seen-wins reclassified production pins as build-time at the dev version); _git_commit confines refs to .git and degrades to None on undecodable content instead of crashing the run; recognised-but-unread lockfiles (yarn.lock, pnpm-lock.yaml, poetry.lock) produce a note and the docs name exactly which lockfiles are read; third-party text cannot inject links; the runtime estimate is honest. 88 tests; the four new guards (triple-keyed exclusion, non-registry choke, lockfile ledger, zero-checked reconciliation) are each mutation-checked against the exact reviewed bug. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Mark PEP 508 direct references as non-registry at the parse boundary `flask @ git+https://...` in pyproject.toml or requirements.txt was stripped to a bare name — `@` is a name terminator in _REQ_SPLIT, so the URL was silently discarded — and the dependency was looked up on PyPI, attributing the public package's advisories and metadata to a fork that may exist precisely to fix them. The npm path guards this at spec parsing and the PyPI path guarded it only via uv.lock's source table, so any pip-managed project walked past the choke point. Direct references are now detected in the requirement text itself and carry non_registry_reason with the URL; the existing choke point does the rest. Assert folded into the requirements parsing test and mutation-checked against the reproduction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Escape third-party text everywhere in the report, not only in tables The Method-and-caveats notes and the report header interpolated untrusted strings unescaped. Reproduced end to end: a package.json dependency key containing newlines (JSON permits them) with a file: spec produced a note that wrote a `## Summary` heading and a forged "No known advisory affects any of the 12 direct dependencies" bullet into report.md — a deliverable meant to survive being pasted into a client report. The 12-byte commit field read from the target's .git/HEAD had the same reach. Whitespace collapsing is the half that matters: it confines hostile text to the line it was interpolated into, where the worst available is inline emphasis rather than forged block structure. The mechanism was already right, so this is the missing calls plus a rename — _cell is now _safe_text, since a table-shaped name is what invited skipping it off-table. The existing table test asserted on the row it expected, which is why it never noticed these paths; the new test asserts structurally that every heading and bullet in the report came from the renderer. Both escaping calls are mutation-checked. Normal reports are byte-identical: the note templates carry no pipes or brackets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Accept v-prefixed and epoch versions; stop escaping inside code spans A regression review comparing the branch tip against its first commit found that two of the earlier review fixes had costs worth paying back. The PEP 440 version gate required a leading digit, so the legal pin `django==v3.2.0` became an unresolved version: advisories were matched against the latest release and 62 real ones for that version read as assessed_clean. PEP 440 permits the prefix, pip accepts it, and OSV matches it. The gate now accepts and strips it, so the reported version is canonical. Probing that also surfaced a longer-standing defect in the same path: extraction split on `!`, which is there for `!=` and truncated a PEP 440 epoch, so `1!2.0` was reported as the pin `1`. Extraction now ends the version at whitespace, a comma, or a semicolon, which additionally recovers the real pin from pip-compile hash lines that previously fell back to unresolved. Markdown does not process backslash escapes inside a code span, so escaping there wrote the backslashes out literally: the report title and the `Scanned:` path came out as `/tmp/pkg \[v2] \| beta`, which is not a path a reader can copy. Values inside backticks now go through _safe_code, which collapses whitespace and neutralises the one character that matters there — a backtick, which would close the span early — and leaves the rest alone. Prose and table cells keep _safe_text, so link forgery and cell escapes are unchanged. One first-party note lost its literal brackets rather than being escaped around them. Non-registry dependencies now share one unassessable reason, with the specific source in the signal value and the Method note. Embedding the source in the reason gave each dependency a unique string, which defeated the report's grouping: a 7-workspace-package fixture produced 91 near-identical bullets across 13 criteria, and the Not-assessable section went from 56 lines to 132. It is back to 56. Also: a collector-level fixture now proves the transitive ledger is sourced independently of the buckets it checks — deriving it from them made the equation true by construction and left a dropped package undetected while all tests passed. The docstring no longer claims the ledger is counted from the raw lockfile, which overstated its reach, and _locked_beyond_direct's return annotation matches its five values again. 91 tests; all four fixes mutation-checked. Real reports are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Keep the pipe escape for code spans inside table cells Splitting the escaping into prose and code-span variants dropped the pipe escape from both, but a GFM table row is split on pipes before inline spans are parsed, so a pipe inside a code span still ends the cell. A dependency named `evil|forged` put six boundaries in a five-column row: the name truncated to a bare backtick and `evil`, and every later value shifted one column right, so "none known" rendered under Other findings. Verified against a CommonMark+GFM parser that this is genuinely a third context rather than a reason to revert: inside a table cell `\|` renders as a literal pipe, while outside one the backslash survives into the output — which is the corrupted `Scanned:` path the split fixed. The three table paths now use _safe_code_cell; the bullet and header paths stay on _safe_code. The new test asserts column parity across every table in the document rather than one row in one table, counting the pipes GFM actually splits on so an escaped pipe reads as content. Per-path assertions are what let this reach three call sites at once, and what missed the notes path two commits earlier. Each of the three sites is mutation-checked independently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Check the assembled document instead of trusting every escape site Escaping was applied per interpolation site, and three successive commits each fixed one set of sites correctly while leaving siblings unguarded: the notes and header fields, then three table paths, and now the download-volume line and the informational sample. Coverage depended on whoever wrote or reviewed the diff noticing every interpolation, which is the wrong thing to rest a property on when the failure mode is a client deliverable carrying a forged all-clear. render() assembles a list of lines and joins them, and every legitimate line is appended as its own element, so two invariants are precise and cannot be violated by legitimate content: no assembled line contains a newline, and every table row carries its header's unescaped-pipe count. check_no_forged_lines enforces both immediately before the join. Block forgery needs a newline to open a new block, so the first invariant catches any site that leaks, including sites not yet written; verified by reverting each of the two newly-escaped sites, which now fails seven existing tests rather than none. The two open sites are escaped as well rather than left to the invariant: with escaping a hostile name renders harmlessly and the audit completes, while the invariant alone would let any audited repository deny its own audit by naming a dependency with a newline in it. The shared test fixture now carries a newline, a backtick, and a pipe in its default name, so every render test drives adversarial input through every path it touches. Its informational criteria carry real booleans too: they were ints, and informational_section sorts on `is True` / `is False`, so the path that interpolates names into a Without: sample had never executed in any test. That combination is why the misses kept recurring. 93 tests; both invariant loops mutation-checked. Real reports are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Akshay K
|
94abac91ca |
static-analysis: convert the semgrep scan fan-out to a dynamic workflow (#231)
* feat(static-analysis): ship /static-analysis:semgrep-scan and a scan runner Two entry points over one implementation. SKILL.md keeps its gated five-step path, where the user reviews and edits the ruleset list before anything runs. workflows/semgrep-scan.js runs the same scan end to end without stopping. Both read the same references/, so a ruleset added to rulesets.md reaches both at once. This is the shape variant-analysis uses. The workflow sits at the plugin root beside the four already on main and ships as /static-analysis:semgrep-scan. Four phases: Detect resolves the output directory and profiles languages and Pro, Select reads references/rulesets.md and writes rulesets.json, Scan runs the script, Report post-filters, merges and summarizes. Step 4 is skills/semgrep/scripts/run-scans.sh, not a fan-out of subagents. Nothing in the scan phase needs judgement: the agents would run fixed commands and report $? and a jq count. Exit codes now stay with the processes that produced them and finding counts come from the JSON they wrote, so no phase reports on work a later phase has to go behind and re-verify. --metrics=off, the --include scoping, the output-directory --exclude and the severity flags are properties of the script. Cross-language rulesets run once rather than once per language and never take --include; a ruleset already in baseline is dropped from its language; language keys fold onto a canonical name, so js and javascript are one unit; two spellings of one repository collapse to one clone. Parallelism is the script's --jobs. The workflow does not stop for ruleset approval. None of main's four ask the user anything, and invoking one with a target is the opt-in. The scan is read-only over the target -- no --autofix or --fix is ever passed, every write lands inside the output directory, and the script refuses to run when the output directory is the target. Semgrep rules are declarative YAML, so pointing --config at a cloned rule repository executes nothing from it. The gate was scope confirmation rather than protection from a dangerous action, and what ran is recorded in rulesets.json and scans.json either way. A deliberate change to a security skill's stated policy, not an oversight; the gated path remains for when the ruleset selection is the thing that matters. Fixes four defects the old prose path carried: allowed-tools omitted the tool Step 4 needed; --severity MEDIUM/HIGH/CRITICAL is rejected by semgrep, so important-only mode never ran; each scanner deleted the shared repos/ clone while others were still reading it; and the scanner agent declared its tools as a comma string where the loader expects a list. Two bugs the new suites found while being written. `eval "$cmd" &` reports exit status 1 whatever the command exited with, which marked every failed scan as a success; commands are built as an argv array and executed directly, which also leaves no quoting surface. And the target was resolved with `cd && pwd` while the output directory was not, so on any path crossing a symlink -- every path under /var on macOS -- both the equality check and the inside-the-target test missed, and the run scanned its own cloned rule repositories. tests/run_scan_tests.sh covers the script: command generation via --dry-run, and execution, exit codes and clone failures against stub semgrep and git binaries. tests/workflow-harness.js compiles the workflow with stubbed globals and asserts that a relative target, an output directory equal to the target, a dead phase and a failed scan each stop the run rather than reach the report as an empty result; --self-test mutates the workflow six ways and requires every mutation to turn a scenario red. Both are hermetic, reach no network, and CI's existing shell-suite discovery runs them, so no workflow file changes. Removes references/scanner-task-prompt.md and the no-Workflow fallback it served. There is no hand-rolled path and no second implementation in prose. * fix(static-analysis): resolve the skill dir at runtime, filter the merged SARIF, clear stale raw output Three defects flagged by kz-tob on #231. The workflow hardcoded SKILL_DIR as a repo-relative path, so every scripted command only resolved inside a checkout of this repo. A marketplace install runs with the user's own project as cwd and the scan phase would have found no run-scans.sh at all. Resolved at runtime instead, folded into the Detect phase, following the cascade variants.js already uses. Each candidate ends at scripts/run-scans.sh, which makes a stale install self-excluding: verified against the 1.2.2 install on disk, which ships merge_sarif.py and nothing else, and the glob correctly declines to bind to it. An unresolved directory throws rather than leaving an agent to compose semgrep commands by hand. In important-only mode both the workflow and scan-workflow.md told the agent to apply the scan-modes.md jq filter to the merged SARIF. That filter reads .results[].extra.metadata, which SARIF does not have, so it exits with "Cannot iterate over null" and results.sarif stayed unfiltered while the JSON side was filtered. The metadata is not recoverable from SARIF, but finding identity is: (check_id, path, start.line) and (ruleId, uri, region.startLine) match field-for-field, confirmed against real semgrep output, and it is the same triple the merge already dedups on. merge_sarif.py --important keeps the findings the JSON filter kept and fails rather than filtering if any scan has no *-important.json beside it, since a partial key set would drop real findings from the deliverable. The merge command blocks in SKILL.md and scan-workflow.md show both modes, so copying the block without reading the paragraph under it cannot produce an unfiltered deliverable. run-scans.sh never cleared raw/. merge_sarif.py globs every *.sarif there, so a rerun into a reused output directory that dropped a ruleset still merged the previous run's output for it. Tests: 58 shell assertions (+3), 46 workflow assertions (+13) with three new mutations, and 16 pytest cases for merge_sarif.py. Each fix is mutation-tested; reverting any one of them turns the suite red. * fix(static-analysis): honor a bare-path arg and guard the important-only merge * fix(static-analysis): fail the important-only post-filter loudly on a jq error * fix(static-analysis): report merge failures and exclude failed scans * fix(static-analysis): log the approved plan once and flag zero-coverage rulesets * fix(static-analysis): drop the SARIF Multitool merge path * fix(static-analysis): report SARIF files the merge could not read * fix(static-analysis): record the exclude pattern applied to every scan * fix(static-analysis): move the exclude-pattern assertions after their fixtures * fix(static-analysis): stop SIGPIPE marking a healthy rule repo as empty * test(static-analysis): pin exclusion for a target holding glob metacharacters * docs(static-analysis): describe the semgrep skill as it now runs --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Fredrik Dahlgren
|
7b9bd5f950 |
Add writing-lean-proofs plugin (#226)
* Add writing-lean-proofs plugin Structured Lean 4 proof writing and library design following Mathlib conventions: sorry-skeleton workflows, API-first definitions, lemma extraction, and an anti-pattern catalog mapped to the linters that enforce each rule. Includes a review-flow eval suite (evals/): five Lean fixtures derived from a real formal-verification project with known planted flaws and known non-flaws, natural review prompts, per-case rubrics, and a runner comparing a baseline arm against a skill arm with an LLM judge plus a deterministic no-rewrite check. The grader ships a self-test that asserts a known-bad review fails every criterion. Smoke-tested: the baseline arm reproduces the folk-advice mistake the skill corrects (calling redundant `show` lines noise); the skill arm passes 5/5. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address Claude review findings - Validate the grader's "id" field before writing grades.json, so a malformed grader response fails with a clear error instead of a KeyError in the summary. - Restore title-case "When to Use"/"When NOT to Use" headings: the validator's required-section check matches them case-sensitively. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address github-actions review findings Eval harness: - checksum_tree fails when it finds zero .lean files, so the no-rewrite check can no longer pass vacuously - both arms run with --setting-sources project, so a user-level install of this skill cannot contaminate the baseline arm - a failed case is recorded and the run continues; the runner exits non-zero after printing the summary - critical steps in run_case return explicitly (errexit is suppressed when the function runs in an if-context) - self-test prints its artifact dir; the on-failure leak is intentional - case 04: dropped the trailing omega so the squeezed simp only in carry_le_one is genuinely terminal (the direction test was inverted) - case 05: overall-verdict retyped from must-flag to overall, with the grader prompt told to judge such criteria solely by pass-when text - fixture overlap removed: Felt.lean's unscored unfold replaced by a bound-free lemma; Bounds.lean's unscored Fact instance replaced by the NeZero instance ZMod.val_lt actually needs Skill content (claims verified against Mathlib docs and the ImProver paper): - lake build alone does not catch sorries (they are warnings); step 4 now includes an explicit grep gate - style linters are enabled in Mathlib's own build but off by default downstream; the opt-in is now spelled out. The show, nameCheck, and setOption linters do exist, so those attributions stand. - ImProver's 100% is on the paper's accuracy metric and holds by construction (fallback to unchanged input); now stated as such - isCompact_union does not exist in Mathlib; example is isCompact_iUnion - library-design.md now distinguishes rfl-proved API lemmas (correct) from downstream proofs needing rfl (the smell) - plugin README Contents paths fixed to be relative to the skill dir Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address second-round github-actions review findings Eval harness: - case 05 gains an engagement criterion: the review must name specific declarations from the file, so an empty "looks fine" can no longer score 5/5 by omission - the grader self-test is now two-sided: the canned bad review must fail every criterion (catches a permissive judge) and a new canned good review must pass every criterion (catches an over-strict judge) - case 01 scores the native_decide in the Fact instance; it was the most severe flaw in the fixture and previously unscored - checksum_tree excludes .claude/ so both arms fingerprint the same file set - runtime check on the isolation guard: a baseline transcript that mentions writing-lean-proofs fails the case as contaminated - results stamp includes the PID so same-second runs cannot collide - case 04: mod_add_carry_mul uses non-terminal simp [carry] (a bare simp risked "no progress"/goal-closing errors that would invalidate the criterion); rubric wording updated Skill content: - the sorry check had inverted exit status for CI use (grep exits 1 on no match); now ! grep, with the comment/docstring false-positive caveat and #print axioms alternative spelled out - anti-patterns.md documents native_decide (trust-base widening, caught by #print axioms, kernel/certificate alternatives) - llm-techniques.md sibling links demoted to plain-text mentions: AGENTS.md prohibits reference chains (file1 -> file2), which the earlier link conversion had introduced Verified: two-sided self-test passes (bad 0/7, good 7/7); live case 05 run scores baseline 2/6 vs skill 6/6 with no-rewrite passing in both arms and no contamination false-positive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat: add linting, performance, and tactics references to Lean skill * Make the eval harness's own checks able to fail Five review findings, all cases of a check that reports rather than gates. - no-rewrite gates the exit status. It was written, printed, and otherwise ignored: CASE_FAILURES only counted reviewer/grader *errors*, so a reviewer that started applying its own fixes under --permission-mode acceptEdits would rewrite every fixture, print no-rewrite:fail on every line, and still exit 0 — CI would read the run as green. - Add the skill arm's mirror of the baseline contamination backstop. cp -R succeeding proves the skill is on disk, not that the CLI discovered it. If project-skill discovery under --setting-sources project ever changes, the skill arm would run bare and both arms would score alike — which is indistinguishable from the true negative "the skill provides no uplift", the one conclusion this suite exists to measure. One canary call per run asks the CLI what it can see. - Guarantee the scratch tree is removed. run_case's body moved into run_case_body so the single rm -rf covers every early return; six paths (fixture copy, skill copy, both checksums, the reviewer call, the contamination check) previously leaked a fixture copy — plus a full skill copy on the skill arm — per case. - Run the offline half of --self-test before the CLI preflight, so the schema, isolation-fixture and rubric checks work with no `claude` on PATH and no authentication — which is where you would want them, e.g. a CI shell-test suite. - Count rubric criteria with the same pattern the Python capture uses. The shell count accepted `- id: foo bar`, which the capture drops, so a malformed line surfaced later as a confusing id-mismatch diff. Also anchor linting.md's warnings-as-errors CI snippet against vacuity: Lake caches per-module artifacts and linter warnings are emitted only on recompile, so on a restored cache build.log is empty, the grep matches nothing, and the gate reports clean over live warnings and sorries — the exact failure the same file preaches against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> Co-authored-by: Marc Ilunga <marc.ilunga@trailofbits.com> |
||
|
Marcelo Morales
|
02b8a5eb8d | Add a workflow for porting Semgrep rules to other languages (#234) | ||
|
kz-tob
|
1a4ba9f1d6 | Add first-pass eval suite for yara-authoring (#219) | ||
|
Clinton Thomas
|
ea7c60e628 |
Convert audit-context-building to a dynamic workflow (#228)
* Convert audit-context-building to a dynamic workflow The skill held a fixed plan that ran the same steps over many functions and spawned subagents from prose. That is a workflow, so it is one now. workflows/audit-context.js orients, analyzes each function in its own subagent, and synthesizes a dossier. Each subagent writes its prose to audit-context/functions/ and returns a schema-validated record, so only the records reach the calling session. The skill routes to the workflow rather than analyzing inline. Also in this change: - Resolve a contradiction between SKILL.md and OUTPUT_REQUIREMENTS.md, which gave different minimum assumption counts for the same section while the agent was pointed at both. - Collapse OUTPUT_REQUIREMENTS.md and COMPLETENESS_CHECKLIST.md into one ANALYSIS_FORMAT.md. The per-function checklist existed in four places. - Drop the numeric quotas. Minimum counts of invariants, assumptions, and applications of a technique produce padding rather than analysis. - Delete commands/audit-context.md. Its --focus flag reached a skill that never accepted one, and its command name collided with the workflow's. - Add DOMAIN_NOTES.md mapping the format across smart contracts, C and C++, decompiled firmware, and web services. - Rework the worked example to cover C and Solidity. - Fix two README links to plugins that do not exist. - Rewrite user-facing text in plainer language. Add four eval cases under evals/, covering C source, Solidity, and Ghidra output. dispatches-not-inlines checks that the skill routes instead of analyzing in the caller's context; the other three check that analysis follows a call into the function being called and walks every path through it, not only the one that succeeds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Raise dispatches-not-inlines turn limit; note measurement gaps The plugin arm was hitting max_turns 20 and being truncated before it answered, which the graders scored as a failure to route. SKILL.md points at three reference files that are read before any work starts, so it needs roughly twice the turns a bare agent uses on the same prompt. Raised to 40, and the timeout to 900s after a run hit the old 600s ceiling. Restore the imperative wording in the routing section. The plainer phrasing was not the cause of the truncation, but this is the revision that was measured, so keep it. Record in the README what is and is not known: dispatches-not-inlines has not been re-measured since the turn limit changed, the Solidity and Ghidra cases score 1.00 with no plugin loaded, and the plugin arm costs about twice the turns of a bare agent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Clinton Thomas
|
798b591aa4 |
Convert spec-to-code-compliance to a dynamic workflow (#230)
* Convert spec-to-code-compliance to a dynamic workflow
The skill held a fixed seven-phase plan and drove a subagent from prose, which
is the shape Maker Week asks us to move into a script. Phase 3 required
line-by-line YAML IR for every function in the codebase before Phase 4 could
start, so on any real target the window was exhausted partway through and the
remaining requirements got a plausible check rather than a real one.
workflows/spec-compliance.js inverts that. Requirements are extracted once, then
each one gets its own agent to hunt the code with, so no context ever holds the
whole behavioral model. Divergences go to two agents that did not produce them,
one re-reading the code and one re-reading the document, and what either refutes
is dropped. A separate agent sweeps the reverse direction, which the
requirement-driven pass cannot cover.
The removed resources are replaced by the workflow's schemas: OUTPUT_REQUIREMENTS
set minimum item counts that a spec with fewer requirements can only meet by
inventing them, alongside a "zero speculation" rule in the same file.
COMPLETENESS_CHECKLIST was a self-verification pass. IR_EXAMPLES demonstrated
YAML formats the schemas now enforce. SKILL.md keeps the judgment that stays
judgment: which verdicts matter, and when a gap is a code fix or a docs fix.
Fixes a classification bug in passing. SKILL.md called undocumented behavior
UNDOCUMENTED CODE PATH while the other two resources called it
code_stronger_than_spec, and the former was not one of the six legal match_type
values, so following the skill emitted a verdict outside its own enum.
commands/spec-compliance.md is removed: it forwarded two arguments to the skill,
required naming the spec that Phase 0 exists to discover, and would have collided
with the workflow on the same slash command.
Adds three eval cases. routes-not-inline measures Δ +1.00 for dispatch.
name-is-not-evidence and documents-contradict both measure Δ 0.00 — Opus 5
handles those unaided — and say so in their own descriptions rather than
implying coverage they do not have.
* Restore domain guidance dropped in the workflow conversion
The conversion deleted IR_EXAMPLES.md on the grounds that the workflow's schemas
replace it. The schemas replace the YAML formats it demonstrated; they do not
replace knowing that `unchecked` suspends a guarantee the calling code is written
as though it still has, or that a 0.3% fee is implemented as `amountIn * 997`
and matches nothing you can grep for. A Solidity user brought the same spec and
got a checker with no calibration for it.
audit-context-building kept its worked examples and added DOMAIN_NOTES.md for the
same cross-domain problem. Following that:
- DOMAIN_NOTES.md maps what counts as a specification, what enforcement looks
like, and where it hides across contracts, C and C++, services, and decompiled
firmware, plus scoping a check against an RFC or standard rather than a project
document. Generalizing past contracts was previously done by deleting the
"non-blockchain" exclusion and adding nothing.
- WORKED_EXAMPLE.md carries three requirements to a verdict, one per verdict
that is easy to get wrong: arithmetic that satisfies a requirement it does not
resemble, an absence whose credibility is the search record, and enforcement
present on every path but the one nobody tested.
- ANALYSIS_FORMAT.md holds the on-disk format in one place. The agent and the
workflow prompt were both describing it, which is the duplication this
conversion was meant to remove.
README gains a migration note: the 1.x command is gone, arguments inverted, PDFs
work and DOCX never did, and the report is no longer a fixed 16 sections.
* Survive an unresolvable checker agent instead of failing the phase
Running the workflow end to end for the first time failed all six requirement
checks with "agent type 'spec-to-code-compliance:spec-compliance-checker' not
found" and returned nothing salvageable. The proximate cause was a stale session
— the plugin had been installed after the session started, so its agents were not
in the registry — but a hard dependency on a namespaced agentType with no
fallback turns any resolution failure into total loss of the run.
checkRequirement now retries on the default agent with the checker's load-bearing
rules inlined, and logs once that it did. The fan-out is concurrent, so every
item in the first batch attempts the specialized agent before the flag is set;
those attempts fail at spawn without consuming tokens, and later batches skip
straight to the fallback.
The prompt no longer says "in the format your instructions define", which was
only true on the specialized path and left the fallback with no format at all.
Verified end to end against evals/documents-contradict/fixture: 26 requirements
extracted from two documents, 6 checked, 3 divergences found and none refuted —
the operator zeroing balances via reassign (critical), and the Senior-tier
collateral bypass from both directions (high). The SPEC/README fee contradiction
was reported as a documentation fix with the note that README frames it as
deliberate. The report named all 20 unchecked requirements as unknown rather than
compliant, and declined to give the unchecked fee requirements a verdict while
still reporting the contradiction as a direct observation.
Confirms the id-collision fix in
|
||
|
Nuno Sabino
|
4822dc3876 |
variant-analysis: convert the skill to a dynamic workflow (#232)
* Converted skill into a dynamic workflow. Still working on the tests * Added gradio test with injected vulns * Fix grader * Fix trailing whitespaces * Bump version number * Remove trailing whitespaces from a git patch... * Run pre-commit * Add claude evals * Address PR claude review * variant-analysis: fix problems found by testing #232 before release (#237) * variant-analysis: fix three workflow defects found in a cold run Prose args killed the run on the first line. The model wrote `bug: ...; root: /path; lang: python` instead of an object, and the invocation died with `args.bug is required` before a single agent started. Parse that shape, and say in whenToUse that args is a JSON object. The baseline command was not shell-safe. The pattern went through JSON.stringify, which looks like quoting but yields a double-quoted string where $(...) and backticks still expand -- and the pattern is model-generated from codebase content. The root was not quoted at all, so any path with a space broke the command. Single-quote both. The sweep had no size floor. It spawned 25 agents against a 5-file fixture, re-reading in parallel what one agent holds at once. The eval's own negative result already said so: five small synthetic codebases showed no difference between the workflow and the skill alone because the fan-out had nothing to buy. Below 40 source files, sweep two axes in one round -- 7 agents on the same fixture. The baseline gate reports the file count, and a single-round sweep is now reported as the deliberate bound it is rather than as a truncated one. The report stage now has to emit `**Location:**` fields. Without them the grader falls through to a permissive path its own docstring calls over-counting, which is what happened on the cold run: a real report scored through the fallback and nothing said so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * variant-analysis: score construct spans, not line proximity A cold run scored a correct report as wrong. The report flagged a helper at lines 4 and 7 of a file whose safe site began at line 10; LINE_WINDOW=30 credited it as the safe site being reported as real, and the run failed. Two different functions three lines apart, conflated. Ground truth now records a `span` per site -- the function's real line range -- and a reported location has to fall inside it. verify_fixtures.py fails if a span stops containing its own anchor line, so a stale hand-edit cannot reintroduce the failure silently. LINE_WINDOW drops 30 -> 12 as the fallback for entries carrying no span. Line-less mentions now lean opposite ways for recall and precision, and both directions favour not failing a run that did the work. A report naming the right file without a line is still credited for recall. It is no longer treated as claiming the decoy: the decoy's file in the real fixture also holds a genuine upstream finding, so any run reporting the real one without a line number was marked as having flagged the decoy. Three self-tests added, all reduced from the cold run. Both fixes were mutation-checked: reverting the span logic and reverting require_line each fail the suite. Also removeprefix("./") for lstrip("./"), which took a character set and ate the leading dot of paths like .github/scripts/x.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * variant-analysis: surface loose scoring, plumb --strict-decoy, parse the workflow summarize.py prints a `loose` column counting runs scored through score.py's permissive fallback. A score built on it is worth less than one built on location fields, and that was invisible. --strict-decoy was documented in the README and implemented in score.py but unreachable from eval.sh, which exited 2 on the unknown option. Plumbed through. The usage header also advertised `--codebase go`, left over from the five synthetic codebases; gradio is the only one, and passing both modes needs quoting. run_fixtures.sh now runs `node --check` on the workflow. It is the only JavaScript in the repo and nothing in CI parses it, so a syntax error would surface only inside a paid eval.sh run. Skipped, not failed, where node is absent. setup-gradio.sh reported "the checkout is not at $SHA" for any failed apply --check, including a checkout at the right SHA whose patch is already partly applied -- reachable, since the unpatched probe only looks at one of the three files. Name both causes and the recovery. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * variant-analysis: drop eval graders no arm can fail, correct the firing claim The skill-not-fired graders on cases 06-07 set `arm: both`, which makes them scored, and neither arm can fail them: the baseline arm has no plugin so Skill never fires, and the with-plugin arm does not fire on these shapes either. The suite's own guidance says a grader no arm can fail is worth deleting rather than reweighting. The type: llm grader on each case carries the real check. The "skill does not fire" limitation was overstated as a property of the skill. A 9-run cold run across three prompt shapes locates the actual cause: it fires 2/3 on a conversational prompt and 3/3 on the description's trigger language when there is a codebase on disk, and 0/3 on an inline candidate panel -- which is the shape of every case in this directory. With nothing to sweep, declining the skill is arguably correct. Giving these cases files on disk would fix the saturated delta and the trigger rate at once; that is the highest-value change left here and it is not a small one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * variant-analysis: describe the trigger that actually fires The skill description was generic where the measured trigger is specific: a bug just found in a named file, and the question of where else it occurs. It now leads with that situation and names the bare conversational form, which is what fired 2/3 in a cold run. The old description was diagnosed as the reason the skill never fired; it was not, but it was still vague. The README's entry-point table claimed the skill is "best for a narrow search where you want a say in each generalization" and triggers on its own. Measured on a real codebase, Claude reaches for the workflow in 4 of 5 firing runs and the skill in 1 of 9 -- so ask for the skill by name if you want to weigh in. Also records the size floor, and that args is a JSON object. tests/README.md documents spans, the recall/precision asymmetry on line-less mentions, and the loose column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Document dynamic workflow layout; variant-analysis 2.0.1 AGENTS.md described only skills/<skill>/workflows/, the prose step-by-step kind, so the plugin-root workflows/*.js layout that ships as /<plugin>:<workflow> was undocumented -- and variant-analysis is the first plugin in the repo to use it. Names both, says which one a "Phase 1 / for each / repeat until" SKILL.md belongs in, and records that ${CLAUDE_PLUGIN_ROOT} is unavailable inside a workflow script. Version bumped 2.0.0 -> 2.0.1 since these are behavioural changes on top of an unmerged 2.0.0. Squash it back to 2.0.0 if you would rather ship one version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * variant-analysis: close gaps found by review of the fix PR A line-less claim on the safe site's file fell into the gap between the strict accusation check and the permissive examined check: it stayed out of decoy_reported_as_real (correct -- it names no line), matched `known` permissively so it dropped out of unreviewed_findings, and then satisfied decoy_examined_and_ruled_out. A run was credited with correctly ruling out the site it had just listed under Findings, and passed even under --strict-decoy. Now surfaced as decoy_claimed_without_line, kept visible in unreviewed_findings, and it blocks the ruled-out credit without counting as a false positive. The small-tree bound could drop expansion axes with no record in the artifact. With axesPerRound=2 and one round, a 6-axis root cause left four generalizations unattempted and only the live progress log said so; REPORT.md was indistinguishable from an exhausted sweep. The report prompt and the return value now carry swept/total axes and name the unswept ones. Spans are exact def..return, which left no room for a decorator directly above a def. RECALL_PAD=3 covers that on the recall side only; the safe site gets no slack, since padding it walks back into the conflation the spans fixed. verify_fixtures.py now requires a span on every entry and validates the range. Without that, a dropped span silently reverted the grader to a proximity window with a green suite, while ground-truth's own comment documented a guarantee that no longer held. source_file_count was `rg --files | wc -l`, which counts assets and fixtures. A 25-source-file project behind 300 fixtures reported 325 and missed the floor it was built for. The prompt and the schema now ask for source files only. `node --check` runs against an .mjs copy. On a .js file whose first statement is `export`, it only passes on Node ~22.7+, and lint.yml pins no Node version. Also: pinned the extraction-mode labels as constants with a self-test, so renaming one cannot leave summarize.py's loose column reading zero forever; fixed a self-test fixture whose span did not contain its own anchor line, a shape verify_fixtures.py now rejects; dropped a dead condition in parseArgs; third-person skill description per AGENTS.md. score.py self-test 16 -> 18 checks, summarize.py 6 -> 7. The label-rename and span-removal mutations were both confirmed to fail the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix node check. Claude workflows have syntax like top-level returns that will trip the linter * Add trailing newline --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> Co-authored-by: Clinton Thomas <1033162+KernelClint@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Thomas Chauchefoin
|
82e8e0ad77 |
insecure-defaults: convert the skill to a dynamic workflow (#224)
* insecure-defaults: convert the skill to a dynamic workflow Rewrites the plugin as a four-phase workflow: recon profiles the target, parallel sweeps collect candidates, a refuting pass adjudicates them, and a report assigns severity with coverage accounting. The skill is removed and `/insecure-defaults:audit [path]` is the only entry point, since the workflow needs the command to locate its detection corpora. It also adds offline tests: scenarios over the workflow's control flow, a mutation self-test that proves they bite, and a check that every documented example is matched by a seed pattern. This does not replace running the command end-to-end against a real codebase. Bumps to 2.0.0. * insecure-defaults: run the node suites in CI The harness and seed-coverage checks ran only when someone remembered to. CI's shell-suite discovery matches plugins/*/tests/run_*.sh, so wrap the three node invocations in run_seeds.sh and both the lint workflow and `make shell-suites` pick them up with no changes to either. Without this, adding a row to CATEGORIES without its references/<id>.json passes CI and then aborts every real run with corpus-unreadable. No setup-node step: ubuntu-latest ships Node, and the scripts are plain CommonJS with no dependencies. The command -v guard makes a missing interpreter a loud failure rather than a suite that quietly does not run. * insecure-defaults: abort when the verify phase adjudicates nothing If every verify batch died, confirmed was empty and the run returned no-findings-confirmed, which commands/audit.md considers a completed audit. Guard on unadjudicated.length === candidates.length and return verify-failed, carrying coverage so the caller sees what went unjudged. Adds a scenario for both failure shapes (all agents dead, all verdict lists empty) and two mutations covering the guard firing and over-firing. * insecure-defaults: report per-category scan counts The zero-scanned guard is on the sum, so five failed searches beside one that worked cleared it and categories_run listed all six. Add files_scanned_by_category and unsearched_categories to coverage, keyed off CATEGORIES so a dead sweep counts as 0, and have the report name them under a Not searched heading. Adds a scenario covering a searching sweep, a zero-file one and a dead one, plus three mutations. * insecure-defaults: count sweep failures against the category list The corpus-unreadable note and the seed-only log divided by the sweeps that returned, so with sweeps dead the ratio read 1/1 rather than 1/6. * insecure-defaults: drop the assertion-count floor from the harness * insecure-defaults: guard a dead report agent agent() returns null on terminal failure, so a report agent that died returned status "findings" with no report and the caller printed nothing while the findings sat in the structured return. Return report-failed with the findings and coverage, and have the command render them. * insecure-defaults: stop labelling a genuine clean run a failure Step 3 accepted findings/no-findings-confirmed and called every other status an incomplete audit, so no-candidates, the deliberate honest-negative status, told the user the run failed. It is now a per-status table. * insecure-defaults: anchor the noisiest seeds (DES|RC4|...) matched NODES and MODES, 0o?(666|777|...) matched any digits, and getMessage() matched all Java exception handling. On the Python stdlib the first two drop from 147 and 228 matching lines to 0 and 84. random. and getMessage() can't be fixed by anchoring, so they now require context: a security-material identifier near the RNG call, and concatenation into a string literal for getMessage(). seed-coverage.js confirms all 18 documented VULNERABLE examples still match. * insecure-defaults: indent the seed wrapper the way shfmt wants --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Joop van de Pol
|
9ea55c5987 |
Fix constant-time analyzer backends and trim the skill (#220)
* Add per-language triage fixtures for the constant-time analyzer One fixture per supported language, each pairing operations the analyzer reports identically but that triage must separate: a division on a private-key coefficient beside one on a public buffer length, and, where the backend detects them, a weak RNG seeding a nonce beside the same call jittering a retry delay. Expected verdicts are deliberately absent from the fixture source. These files double as evaluation input, and a verdict written next to the code is a verdict the reviewer reads instead of deriving. A later commit adds the manifest that records them. Fixture shapes are dictated by what each toolchain emits: the Go helpers are //go:noinline so each stays a distinct symbol, its main() takes input from argv so the arithmetic is not constant-folded away, and the Swift entry points use @_cdecl to keep symbol names readable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Configure ty and fix the typing weaknesses it reports `ty check` reported 50 diagnostics, all pre-existing. Most were noise from module resolution: the tests reach the modules via `sys.path.insert` and import `analyzer` rather than `ct_analyzer.analyzer`, which ty cannot know without being told. `tool.ty.environment extra-paths` tells it, dropping 32 unresolved-import reports and leaving the substantive ones visible. The rest were real, if not yet bugs: - Six compilers and several entry points annotated `list[str]`/`str` while defaulting to None. Now spelled `| None`. - `get_compiler` was annotated to require a compiler name, but its body auto-detects from the language when the name is falsy, which is how every caller uses it. - Each parser tracked functions as a bare dict, typing the values `str | int`, so the `functions[-1]["instructions"] += 1` counter present in all eight parsers read as string addition. A `ParsedFunction` TypedDict names the shape instead. Three unresolved-import reports remain by construction: analyzer.py is documented to run as a script as well as import as a package, so both the relative and top-level spellings are needed and only one can resolve. Those carry an inline ignore with the reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix analyzer backends that reported PASSED on vulnerable code Auditing each backend against real toolchain output found six of them skipping unparseable lines and reporting success. Every one was covered by tests that replay hand-written listings, which is why the drift went unnoticed; the tests added here drive the actual toolchains. Python, on 3.11 and later: - The BINARY_OP oparg map read 12 as `//`, but 12 is `^`. Floor division on a secret was missed and every constant-time XOR was reported as a division. Keyed on the operator symbol dis prints, which cannot silently renumber. - The instruction regex required a byte offset that 3.13 no longer prints, so all but the first instruction of each source line was skipped. On 3.13 `key_coef // (2 * gamma2)` came back PASSED. Every column ahead of the opcode is now optional, and the source line carries forward. PHP: `_parse_opcache_output` delegated to the VLD parser on the claim the formats were "similar enough"; they share nothing structural. `analyze()` also called the VLD parser unconditionally, so the function was dead either way. VLD is an unbundled PECL build, so this is the path most users hit: a file with intdiv, mt_rand and base64_encode reported zero findings. Adds a real parser, plus intdiv/fdiv, which perform division through a call and so emit no DIV opcode. JavaScript and TypeScript: V8 prints `67 E> 0x… @ 14 : 3e 03 04 Div a0, [4]`, not `offset : Mnemonic`, so every file parsed as zero instructions. `--no-lazy` covers functions a module exports without calling. Bytecode blocks are filtered to names the file declares, since V8 dumps node's internals identically. Source positions are byte offsets, now converted to lines except for transpiled TypeScript, where they index generated output. Go: `go build` links the runtime in, so 23 of 24 findings came from the allocator and garbage collector while the caller's code was silent. Objdump headers name the source file, so the filter is exact. Go also writes arm64 in Plan 9 syntax, where a 32-bit divide is SDIVW — absent from the table, so `int32` division, the shape of every polynomial coefficient divide, read as clean. Rust: compiled as a bin crate, so any library file failed E0601. Crypto lives in libraries. Swift: target triples were hardcoded to Apple platforms, so swiftc rejected them on Linux and no Swift file could be analyzed there. Java: the weak-RNG pattern matched only `new Random(`, missing the fully-qualified form that needs no import. All six source-level scanners treated comments as code, so `/** a / b */` counted as a division; comment bodies are now blanked while preserving offsets. Finally, the text report labelled every scripting-language error `[WARN]` while the summary counted it as an error: analyzer.py runs as `__main__` while script_analyzers imports it as `analyzer`, giving two Severity enums, so identity comparison failed. Severity is compared by value now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record triage verdicts for every fixture and assert the pairs hold expectations.json gives each fixture's cases a verdict and the reasoning behind it: 53 cases over 13 languages, covering arithmetic everywhere, conditional branches on the native backends, weak RNG wherever a backend detects it, and PHP's encoding functions. TestTriageMatrix asserts the mechanical half — that the analyzer still reports both members of every true/false-positive pair, since the premise of the skill's triage step is that the tool cannot tell them apart. Four of its checks need no toolchain, so the matrix stays guarded on machines that cannot compile most of these languages: every supported extension has a fixture, every fixture pairs a true with a false positive, every line-based locator resolves exactly once, and no fixture leaks its verdict into a comment. The run itself fails rather than skipping when no fixture could be exercised, and names the languages it skipped. Two cases record measured behaviour rather than the intent: Rust claims no branch true positive, because rustc emits no flagged conditional branch in the tag comparison at O0 on arm64 and asserting one would encode a fixture artefact; and Go's branch false positive is its stack-growth check, which appears in every Go function and depends on nothing the caller passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Trim the constant-time-analysis skill and state its real coverage SKILL.md goes from 219 to 165 lines by removing what was duplicated or inert: a decision tree that restated the trigger bullets below it, five near-identical invocation blocks, and JVM/.NET setup copied from references/vm-compiled.md. It gains the frontmatter it was missing — allowed-tools, which it never declared and so inherited everything, and an explicit effort level. The additions come from evaluating the skill rather than from review. Runs at three effort levels all recommended fixing a secret-dependent divide by handing the compiler a constant divisor; measuring that fix showed it still emits a real divide on gcc riscv64 at every optimization level, on gcc arm64 and x86_64 at Os and Oz, and on clang arm64 at O0 and Oz. The sweep section now says so, with the table. Coverage is not uniform across backends, and a clean report means different things depending on the language, so there is a table for that too. Triage guidance now also covers the weak-RNG and encoding findings, where no operand is secret and the question is what the result is used for — seeding a nonce or jittering a retry delay. "When NOT to Use" names the constant-time-testing skill and draws the line between them: that one measures a running binary, this one reads compiler output and never executes the code. Both READMEs listed nine languages when thirteen are supported, and the skill's file tree was missing four reference files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix warning-level detectors that no source could ever match Extending the triage matrix to the comparison, lookup and encoding families found that several of those detectors were unreachable. The tables listed them, so the coverage looked complete, but no input could trigger them. - Ruby built every warning pattern as `\.<name>`, which is right for method calls like `.start_with?(` but turns the module-qualified keys into `\.base64\.encode64`. Five entries — both base64 functions, json.parse and both marshal functions — could not match any Ruby source. - JavaScript did the same to its non-method entries, so `btoa(`, `atob(`, `decodeURIComponent(` and `JSON.parse(` never matched. - The JS keyed-property mnemonics were V8 9.x spellings. Node 22 emits `GetKeyedProperty`, so secret-indexed array access — the whole point of that family — was undetectable. The named-property entry is removed rather than updated: a named access has a constant property name and cannot be indexed by a secret, so flagging it reports every `Math.trunc` call. - Java, Kotlin and C# selected patterns with if/elif chains covering three or four keys and skipping the rest, leaving the base64 and convert entries unreachable. They now share `qualified_call_pattern`, which distinguishes the two shapes the keys use: `string.equals` is a method on any receiver, while `arrays.equals` and `base64.getencoder` are members of a named type and must match only the qualified form. Case-insensitivity is embedded in the pattern because the keys are lowercase while the source spells them `Base64.getEncoder` and `.Equals`. Ruby's warning mnemonics also kept the dots from their keys, unlike every other severity and backend, so they read `BASE64.ENCODE64`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Cover the comparison, lookup and encoding families in the triage matrix The matrix covered two of the four families the analyzer implements. It now covers all four, taking it from 53 cases to 99. Each of the eight bytecode and scripting fixtures gains three pairs: an early-exit comparison of a secret tag against the same comparison on a public header, a table lookup indexed by a key byte against one indexed by a fixed offset into a public header, and a base64 encode of a decrypted session key against one of the public algorithm identifier. PHP already had the encoding pair. Native fixtures are unchanged — their instruction tables carry no comparison or load entries, and C's tag comparison is already covered as a branch. All three families are warning severity, so their cases run with `include_warnings`. That is worth noting on its own: the invocation the skill documents does not pass `--warnings`, so a reviewer following it sees none of this — including early-exit comparison, which is the most common timing bug in practice. Every case was measured against the real toolchain before being written down. Python's fixture avoids a `list[bytes]` annotation in a signature because the subscript in the annotation is itself reported at module scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Refuse an architecture the compiler cannot target Every compiler looked its architecture up with `.get()` and simply omitted the target flag when the lookup missed, while `analyze_source` still labelled the report with the architecture that was requested and applied that architecture's instruction table. So `--arch riscv64` on Swift, whose Linux map holds two entries, compiled for the host and reported `architecture: riscv64` with no findings; `--arch mips` on gcc reported `passed: true` the same way. "PASSED for riscv64" is what a reviewer writes down, so producing host output under the requested label is worse than failing. All five compilers now refuse and name what they do support. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Stop the Go symbol filter readmitting the runtime by basename The filter compares the objdump block's source path against the file under analysis, but when the realpath comparison failed it fell back to comparing basenames — and Go's runtime ships map.go, slice.go, string.go, time.go and select.go. Analyzing a user file named map.go therefore matched every `TEXT runtime.…(SB) /usr/lib/go/src/runtime/map.go` block, reported runtime.makeBucketArray among the findings, and parsed 14 functions instead of 5. That is the behaviour the filter exists to prevent, and the existing test missed it only because its fixture is named triage_go.go. objdump prints an absolute path, so the exact comparison is sufficient there; the basename fallback now applies only when the printed path is relative. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Make comment blanking string-aware Blanking comments before the source-level scans stopped doc comments counting as code, but the regexes knew nothing about string literals, so a comment marker inside one blanked live code: - `const u = "http://example.com"; const k = a / b;` lost the division, and `String u = "http://x"; Random r = new Random();` reported clean — for Java, Kotlin and C# the source scan is the only detector for `new Random()`. - Ruby's `puts "tag #{a / b}"` lost everything from the interpolation on. - Worst, a lone `const marker = "/*";` plus any later `*/` anywhere in the file blanked every line in between, with no diagnostic. String and template literals are now matched first and preserved. This is still a regex rather than a lexer — unterminated literals, JavaScript regex literals, Ruby heredocs and `%w[]` are not modelled — and the docstring says so; the failure mode is a blanked or unblanked span, never a crash. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Test the empty-parse guards and close smaller robustness gaps The test named for PHP's "nothing parsed" refusal asserted only that the parser returned empty lists, which the pre-fix code did too, so deleting the raise in `analyze()` left the suite green. It now asserts the RuntimeError, and the Go equivalent — the symbol filter keeping nothing — is covered as well. Both were confirmed to fail with their guard removed. Four gaps that each degrade quietly: - The OPcache offset group was `\d{4}` exactly, so an op_array past 9999 opcodes stopped parsing mid-function. `functions` is non-empty by then, so the empty-parse guard cannot fire and the report reads as a partial pass. - The V8 name filter treated "could not read the compiled file" as "filtering disabled", listing hundreds of node-internal functions with nothing to distinguish that from a genuinely noisy file. Unreadable input is now an explicit None with a note on stderr. - `last_line_num` was not reset per code object, so the first instructions of a function inherited the previous function's line when dis omits one. - The triage matrix printed its skip list to stdout, where CI summaries do not show it, so an image that lost a toolchain would keep passing while the claim that all backends are exercised quietly stopped being true. Skips are now emitted per language and appear in the count. Two test-quality fixes: the matrix completeness check compared set sizes, so renaming one language while dropping another stayed green — it compares sets now, through an explicit display-name to `detect_language()` mapping. And `_have` uses `shutil.which` rather than running `--version`, because javap exits non-zero for it, which would have skipped Java and Kotlin everywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Pass --warnings in the documented invocation The Quick Start ran the analyzer with no flags, which reports only error-severity findings: division, modulo and weak RNG. Four families are warning severity and stayed silent — secret-dependent branches, early-exit comparison, secret-indexed table lookups, and variable-time encoding. A reviewer following the skill exactly never saw the early-exit comparison of an authentication tag, which is the most common timing bug in practice and what Lucky Thirteen was. `--warnings` is now in the documented command, the directory sweep and the flag table, with the two consequences of turning it on stated alongside. Warnings do not affect the PASSED line, so `Result: PASSED` beside `Warnings: 6` is normal and is not a clean result. And comparison and lookup findings need their own triage question: for a lookup it is the index that must be secret, not the contents. Confirmed comparison findings have a per-language constant-time primitive, so those are tabulated rather than left as "write it branch-free"; a secret-indexed lookup gets the honest answer that no drop-in exists. Also names the plugin that owns constant-time-testing, since a reader cannot assume it is installed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix the fixture path and the sdist include list Two defects that both make something unreachable: The pointer to the triage fixtures was written as a bare relative path, while every other path in SKILL.md is `{baseDir}`-prefixed. The skill runs against the user's repository, so a model following it looked for `ct_analyzer/tests/triage_samples/` inside the project being audited. The validator resolves markdown links rather than inline code, so this passed 359 reference checks while being unusable at runtime. The sdist include list named three globs covering C, Go and Rust, so a packaged install shipped none of the Python, Ruby, PHP, JS, TS, Java, C#, Kotlin or Swift samples, nor the triage fixtures and their manifest, and the tests that read them could not run from an sdist. A built sdist now carries all 27. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Test that cross-compilation really targets the requested architecture Nothing covered cross-compilation. The pre-existing cross-architecture test asks for arm64, which is the host on an aarch64 machine, so it passes there without ever crossing — and a silent fall back to the host is exactly the failure `reject_arch` was added to prevent. These assert instruction names unique to the target: x86 `DIVL`/`DIVQ`/`IDIVL` and RISC-V `DIVU`/`DIVW`/`REMU` must appear, and arm64 `SDIV` must not. Dropping clang's `--target` makes both fail, which a report-shape assertion would not have caught. Go gets the same treatment for amd64, where GOARCH cross-builds because CGO is disabled. Only the paths that work are covered: on this host clang crosses to both targets and Go to amd64, while gcc, rustc and Go/riscv64 cannot cross here at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drive an explicit compiler with its own flags, and name it in the report Cross-compiling with gcc was impossible through the analyzer. `--arch x86_64` passed gcc's native ISA switch `-m64`, which the gcc on an arm64 host rejects, and the escape hatch that should have worked — handing over the cross binary — was broken: `get_compiler` treated every unrecognized `--compiler` value as clang, so `--compiler x86_64-linux-gnu-gcc` was driven with `--target=` and failed on "unrecognized command-line option". An explicit compiler is now dispatched on which compiler it actually is: by filename first, falling back to its `--version` banner, so `cc` is recognized as whatever it points at. `--compiler x86_64-linux-gnu-gcc --arch x86_64` and `--compiler riscv64-linux-gnu-gcc --arch riscv64` both work and emit that target's division instructions. Nothing is substituted on the caller's behalf. The analyzer could look for `<triple>-gcc` and use it automatically, but a Debian cross gcc is frequently a different major version than the host's, so its codegen is not the host gcc's; silently swapping binaries would put one compiler's output under another's name — the same mislabelling this commit removes. Instead, when gcc is asked for an architecture it cannot build, the error names the binary to pass, and the report records the binary that ran rather than the family, so `compiler:` is no longer "gcc" when a cross build produced the assembly. SKILL.md now states how each toolchain crosses, since "sweep more than one --arch" was not actionable for gcc users, and advises comparing against the toolchain that builds the product rather than a packaged cross build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Detect software division, and fix the armhf flags that hid it Installing the remaining cross toolchains turned up two problems that are not about missing dependencies. The arm flags were self-contradictory: `-march=armv7-a -mfloat-abi=hard` is rejected with "selected architecture lacks an FPU", because armv7-a alone specifies no FPU. Adding `-mfpu=vfpv3-d16`, Debian's armhf baseline, makes the build work. With it building, arm reported no findings at all on a fixture with three divisions. armv7-a has no hardware divider, so gcc emits `bl __aeabi_idiv` rather than a division instruction, and the analyzer matches mnemonics: a branch-with-link is not in any table, so the file read as PASSED. Software division loops over its operands, making it more operand-dependent than the instruction it replaces, so this was the worst kind of false negative. Calls to the libgcc and Arm EABI division routines are now reported by call target, which also covers __divti3 — used on x86_64 for __int128 division, which crypto code does perform. Verified against every cross target now installed: x86_64 DIVQ/IDIVL, i386 DIVL/IDIVL, arm AEABI_IDIV, riscv64 DIVU/DIVW/REMU, ppc64le DIVDU/DIVW, s390x DLGR/DSGFR, arm64 SDIV/UDIV. The ppc64le and s390x instruction tables had never been exercised before. Also stops the new "pass a cross build" hint from telling the caller to do what they just did: it is suppressed when the compiler already is a cross build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Cross-compile a freestanding probe, not a fixture that needs a cross libc CI failed on `test_clang_targets_riscv64` with "bits/libc-header-start.h file not found". The test compiled the C triage fixture, which includes <stdint.h>, so cross-compiling it needs the target's C library headers as well as the compiler. The x86_64 case passed only because the runner is x86_64, making that target native; my dev box passed riscv64 only because installing gcc-riscv64-linux-gnu had pulled in libc6-dev-riscv64-cross. So the test was asserting an environment property, not the analyzer's behaviour. Nothing about checking that a division instruction reaches the assembly needs a libc. The cross tests now use a freestanding sample with no includes, verified to compile for x86_64, riscv64 and s390x with `-nostdinc` — that is, with no headers reachable at all — while still yielding that target's division mnemonics. This also corrects the claim in SKILL.md that clang needs nothing installed to cross-compile: it needs no second compiler, but a source including libc headers still needs the target's headers. When that is what failed, the error now names the package that supplies them, which is the analyzer's own advice to sweep `--arch` being made actionable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Scott Arciszewski <147527775+tob-scott-a@users.noreply.github.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Eric Kilmer
|
ec2450d05e |
Trim dwarf-expert: single-file skill, effort level, contract test (#223)
* Trim dwarf-expert: single-file skill, effort level, contract test - Consolidate SKILL.md from four files into one (115 lines); delete reference/. Cut generic behavior rules, duplicate When-to-Use sections, a 9-line readelf reference hop, and an ASCII decision tree; fix five typos. - Fix --lookup=<offset> misuse carried over from the original: DIE at a section offset is --debug-info=<offset>; --lookup takes a program address. Verified live against llvm-dwarfdump. - Drop the invented v3-v5 version scoping from the skill, plugin.json, marketplace.json, and both READMEs. Version-specific judgment that earned its place (the -gdwarf-N pin inference, v2 surface forms) lives in the Verifying section. - Frontmatter: add effort: medium, rewrite the trigger description, extend allowed-tools with Write/Edit/WebFetch. - Add tests/test_skill_contract.py (stdlib + pytest): every dwarfdump flag documented in SKILL.md must exist in a live llvm-dwarfdump (>= 19, the verified floor for --error-display/--verify-json), plus a frontmatter contract check. Zero extracted flags or a missing tool fails rather than skips. - CI: python-tests installs llvm-19, falling back to the default llvm on future runner images. - Bump version 1.0.1 -> 1.1.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address review: fence-aware extraction, capability-gated resolver - body_sections() no longer strips fenced blocks from section text: fences are ignored only for heading detection, so flags documented inside examples are extracted and verified. New unit test covers both properties; a fenced --bogus-flag mutation now goes red. - Replace the parsed-version LLVM floor with a capability gate: the resolver requires --error-display/--verify-json in --help, because Apple's LLVM numbering does not track upstream and a numeric major is not comparable across toolchains. Failure lists rejected tools with their version banners. README wording matches. - Raise the extraction floor from 10 to 15 (21 flags documented). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Clinton Thomas
|
696dee5717 |
Remove debug-buttercup, re-homed in trailofbits/buttercup (#221)
The skill only makes sense inside the codebase it debugs. It hardcodes Buttercup's crs namespace, the 13 Redis stream names and 5 consumer groups from common/queues.py, the app= label selectors from its Helm charts, and its /tmp/health_check_alive liveness convention. None of that helps anyone outside that repo, and all of it goes stale when that code changes with no signal here. It now lives at .claude/skills/debug-buttercup/ in trailofbits/buttercup, next to the queue definitions and charts it documents, alongside the existing buttercup-langfuse skill. Nothing is lost. Also drops the Infrastructure section from the README, which this was the only entry under. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Scott Arciszewski <147527775+tob-scott-a@users.noreply.github.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Eric Quintero
|
8071528176 |
Remove seatbelt-sandboxer plugin (#218)
* Remove seatbelt-sandboxer plugin The skill's central justification was wrong: it taught that `file-read*` reads from any path, when a filtered `file-read*` is bounded exactly like `file-read-data`, and it never mentioned that SBPL applies the last matching rule, so a deny placed above a broad allow silently enforces nothing. Guidance that is wrong is worse than absent, because it replaces a correct instinct with a false justification, and `sandbox-exec` settles any SBPL question locally in seconds for anyone who needs one answered. With no recorded production use to weigh against four defects found in a single afternoon, the maintenance cost was not earning its keep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Update marketplace.json fix conflict resolution --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Kevin Valerio
|
3742e65294 |
chore(ask-questions-if-underspecified): remove the plugin (#214)
The skill content is untouched since 2026-01-29. The only commits to the directory since were repo-wide sweeps (formatting in #98, codex UI metadata in #175), not maintenance. It also spends a lot of context on something you can get from one English sentence to Claude. Asking clarifying questions when a request is ambiguous is native Claude Code behaviour now, through AskUserQuestion, so an 86-line skill that mostly explains how to format multiple-choice options is paying context for a built-in. Knock-on edits: AGENTS.md used this plugin as its Basic reference example, now git-cleanup, which is a better fit anyway at five files and one self-contained SKILL.md. let-fate-decide pointed at the skill by name in its description and its When NOT to Use list, now reworded to plain "ask clarifying questions"; its description feeds skill triggering, so it gets a patch bump to 1.2.2. Co-authored-by: kevin-valerio <kevin-valerio@users.noreply.github.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
kz-tob
|
daa4ad03eb | Update CODEOWNERS (#236) | ||
|
Tarun Bansal
|
1256982d4d |
Remove workflow-skill-design plugin (#215)
Co-authored-by: Benjamin Samuels <1222451+bsamuels453@users.noreply.github.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 |
||
|
Evan Sultanik
|
0cb5f95840 |
Add open-sourcing plugin (#202)
* Add open-sourcing plugin Skill for preparing a repository for public release, generalized from the internal open-sourcing guide. Generic workflow (secrets audit, licensing, docs, CI, release automation) with a Trail of Bits policy overlay loaded via git-remote/committer-email detection. Includes per-language packaging references and readiness-check scripts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Modernize open-sourcing skill to current toolchain practice Align guidance with the current cookiecutter-python toolchain and 2026 ecosystem state, verified against primary sources: respect-existing- tooling principle (warn on stale toolchains, adopt modern defaults only when absent), SLSA provenance job in the PyPI release workflow, ty pre-1.0 pinning caveat, uv audit/interrogate, rulesets wording, Dependabot grouping and cooldown, and a new JavaScript/TypeScript reference covering npm trusted publishing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Dan Guido <dan@trailofbits.com> |
||
|
Evan Sultanik
|
65ecafa7ec |
Add github-triage plugin (#192)
* Add github-triage plugin Triages open GitHub issues for the current repository via the gh CLI: closes already-resolved issues with comments citing the resolving PR or commit, cross-links issues with pending fix PRs, and assigns local-only priority and change-size (size/XS–XXL) estimates for everything else. All GitHub writes are gated behind a single review-and-iterate approval; priority/effort are never posted. Registers the plugin in the marketplace, README, and CODEOWNERS. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Add optional PR triage to github-triage skill Before issue triage, optionally clear open PRs (so merges feed the "already resolved" issue check): - incrementally merge allowlisted bot PRs and maintainer-approved PRs, one at a time, re-verifying mergeability/CI before each and confirming each landed; - spawn one read-only review subagent per never-reviewed PR, saving each review to github-pr-<number>-review.md locally (never posted). PR readiness uses verified gh --json semantics: mergeStateStatus==CLEAN (MERGEABLE alone is insufficient; UNKNOWN is never-merge), per-node statusCheckRollup (CheckRun status+conclusion vs StatusContext state), author.is_bot + trusted allowlist, and latestReviews state+authorAssociation rather than the branch-protection-driven reviewDecision. All merges gated; never --auto/--admin/force. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix github-triage bugs found in end-to-end dry run Validated the skill against trailofbits/graphtage (real issues + PRs), which surfaced four bugs: - gh repo view takes the repo positionally, not -R; fixed the default-branch and merge-method lookups. - CI readiness treated NEUTRAL/SKIPPED checks as failures, wrongly blocking mergeable Dependabot PRs (CLEAN with a NEUTRAL CodeQL run). Reworked to "CI not blocking" (hard failures + pending only), with mergeStateStatus==CLEAN as the authority. - Not-ready bot PRs were routed to the review-subagent bucket; bots are now excluded so they fall to Needs work. - Bot allowlist now normalizes gh's author.login renderings (app/dependabot and dependabot[bot]) so bot detection actually matches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Dan Guido <dan@trailofbits.com> |
||
|
Scott Arciszewski
|
841bffed0d |
trailmark: add v0.4/v0.5 support and graph-assisted security workflows (#183)
* trailmark: update skills to use v0.4.0 features * fix: resolve code review findings for PR #183 Redraws the version boundary to match the actual Trailmark release history, verified against the v0.2.0/v0.2.1/v0.2.2/v0.3.1/v0.4.0 tags of trailofbits/trailmark: P1 (misclassified APIs, fixed): - ancestors_of(), reachable_from(), entrypoint_paths_to(), nodes_with_annotation(), clear_annotations() and the diff/entrypoints CLI exist since v0.2.0 — moved from the v0.4+ list to the v0.2-safe baseline; removed needless hasattr() gates and degraded fallbacks - supported_languages()/detect_languages() (trailmark.parse) are 0.3+ modules, not v0.2-safe — annotated as such - CLI --version/version were added in 0.2.2, not 0.4 — documented as 0.2.2+ and version-probe failure semantics clarified - graph-evolution/SKILL.md reverted to main: native diff has existed since v0.2.0 with identical args, so the 0.2.x-fallback rewrite was built on a false premise (also resolves the quality-checklist contradiction flagged in review) P3 (fixed): lexical version comparison hazard noted; stale diagramming-code checklist label; README baseline list now defers to the SKILL.md Version Gate instead of keeping a second divergent copy Dismissed: 'diff CLI signatures contradict' (both forms valid — before/ after positionals accept paths or git refs, --repo/--json exist since 0.2.0); 'subgraph_edges has no edge_kinds param' (v0.4.0 signature has edge_kinds keyword); 'diagram.py fallback broken on 0.2.x' (trailmark.diagram module exists in 0.2.x; only the CLI subcommand is new); 'phantom diff_against()' (real, v0.2-safe, now in baseline) Verified: check_claude_loadability.py, check_codex_loadability.py, pre-commit hooks pass; all SKILL.md files under 500 lines Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * trailmark: add assurance workflow skills (#187) * trailmark: add assurance workflow skills * fix: drop hasattr gates on v0.2-baseline query APIs entrypoint_paths_to() and reachable_from() are in the v0.2-safe baseline, so the hasattr fallbacks were dead code — and the entrypoint_paths_to fallback indexed attack_surface() entries with entry["name"] instead of node_id, which would raise KeyError. Addresses PR #187 review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(trailmark): make detect_languages import v0.2-safe in operational skills The trailmark-summary and trailmark-structural skills claimed v0.2-safe workflows but unconditionally imported trailmark.parse, a 0.3+ module. detect_languages() has existed in trailmark.query.api since v0.2.0 (kept as a deprecated alias in 0.3+), so gate the import with a fallback instead of relabeling the skills as 0.3+. Version Gate docs updated to document the v0.2-safe import path; supported_languages() remains 0.3+ with no 0.2.x equivalent. Verified against trailmark v0.2.0 source (fallback branch) and the current 0.4 line (canonical branch). Addresses review feedback on PR #183. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * trailmark: document and enable v0.5.0 features Trailmark 0.5.0 adds a PostgreSQL-oriented sql parser (node kinds schema/table/view/procedure), the stable .trailmark/links.toml configuration for cross-language/FFI/RPC/external links, repository links/proxies/type_uses edges on single-language parses, Solidity entrypoints from parser metadata (visibility/mutability/overridden-by attributes), node attributes in attack_surface() entries, TypeScript constructed-receiver resolution, and C# file-scoped namespaces. Updates the Version Gate with a v0.5+ section and a structural probe ('SCHEMA' in NodeKind.__members__ — 0.5.0 adds no new QueryEngine methods, so hasattr() cannot detect it), adds a Repository Links section and cross-boundary query recipe, extends the parser list and graph model docs, notes the reachability-vs-taint limitation, and threads the 0.5 output additions through trailmark-structural and audit-augmentation. Plugin version 0.9.0 -> 0.10.0. All version claims verified against trailofbits/trailmark v0.2.0 and v0.5.0 builds, including a live links.toml materialization test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * trailmark: add slicing-code-context skill (#203) Adds a skill for delegating focused code tasks to constrained or locally hosted models using bounded, graph-informed Trailmark source packets: - scripts/build_slice_packet.py: deterministic PEP 723 packet builder (Trailmark 0.5.x) with five selection modes, whole-unit budget admission, explicit omission accounting, path-traversal rejection, an embedded untrusted-source notice, and structured JSON errors (26 tests, including a real Trailmark integration test) - agents/code-slice-worker.md: repository-tool-free Haiku worker returning a source-cited JSON contract - SKILL.md + references/slice-packet.md: coordinator workflow, packet and worker response contracts, and validation rules Reviewed with a multi-agent Claude pass and two Codex passes; fixes from those reviews are included (doc/selection-order reconciliation, relationship deduplication, line-range anchors no longer expand to full nodes in path/entrypoint modes, background-safe worker toolset, structured io_error handling, replacement-packet expansion semantics). Co-authored-by: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Dan Guido <dan@trailofbits.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |