mirror of
https://github.com/trailofbits/skills.git
synced 2026-09-14 14:28:48 +08:00
main
196 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Dan Guido
|
ce9ae2e2dc |
Add post-patch-validation plugin (#302)
* Add post-patch-validation plugin * Use public contact email for post-patch-validation * Fix post-patch validation CLI and artifact edge cases |
||
|
dependabot[bot]
|
321ccfe628 |
deps: Bump ruff (#300)
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.4 to 0.16.5 - [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.4...0.16.5) --- updated-dependencies: - dependency-name: ruff dependency-version: 0.16.5 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> |
||
|
dependabot[bot]
|
d3323cefbc |
deps: Bump the python-minor-patch group across 2 directories with 2 updates (#296)
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). Bumps the python-minor-patch group with 1 update in the /plugins/yara-authoring/skills/yara-rule-authoring/scripts directory: [yara-x](https://github.com/VirusTotal/yara-x). Updates `ruff` from 0.16.3 to 0.16.4 - [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.3...0.16.4) Updates `yara-x` from 1.19.0 to 1.20.0 - [Release notes](https://github.com/VirusTotal/yara-x/releases) - [Commits](https://github.com/VirusTotal/yara-x/compare/v1.19.0...v1.20.0) --- updated-dependencies: - dependency-name: ruff dependency-version: 0.16.4 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: python-minor-patch - dependency-name: yara-x dependency-version: 1.20.0 dependency-type: direct:production update-type: version-update:semver-minor 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
|
14e5a10700 |
burpsuite-project-parser: fail rather than report an unverified search as clean (#288)
* burpsuite-project-parser: fail rather than report an unverified search as clean
Burp ignores flags it does not recognise, so without the parser
extension it starts normally and drops the query. The script streamed
whatever came back and exited 0, so a missing extension was
indistinguishable from a search that matched nothing.
Output now streams through awk, which classifies it: non-JSON on the
first line means the flags were dropped (exit 4), and no output at all
means an empty result set and an unloaded extension cannot be told
apart (exit 3, stated plainly rather than reported as success). Burp's
own non-zero exits propagate instead of being masked. SIGPIPE from a
downstream head or jq is not a failure -- the documented workflows do
that deliberately.
set +e around the pipeline rather than '|| true', which is a command of
its own and resets PIPESTATUS before it can be read.
SKILL.md documents the exit codes, notes that '0 0' from the size check
is not a size to act on, and adds a rationalization for reading an
empty search as absence of traffic.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* burpsuite-project-parser: classify the whole stream, not the first line
Three defects in the first-line check, all in the same place.
The pattern accepted `{` or `[`, and `[main] INFO burp.StartBurp - ...`
is very common Java stdout. With the extension unloaded that first line
passed as JSON, awk exited 0, and the banner went to stdout as a
verified clean result -- the exact state this branch exists to prevent,
through a one-character widening.
The opposite direction was worse: a working install that prints a
licence or startup line before the JSON set not_json on line 1 and
exited 4, telling the agent the extension is missing and to install it
before trusting any result. Every documented workflow would have failed
on a setup that worked before this branch. A blank first line did the
same, with an empty string in the diagnostic.
Tightening to `{` trades one for the other, so the check is now over the
whole stream: count lines that are JSON objects, fail only when that
count is zero. A preamble is tolerated, a log line is not mistaken for
data, and only JSON objects reach stdout -- everything else goes to
stderr, so a downstream grep or jq cannot match a banner.
SKILL.md also now says the exit code is invisible through a pipe, since
nearly every documented example ends in | jq or | head and reports that
command's status. stderr is the reliable signal; pipefail and
PIPESTATUS are shown for reading the code itself.
Verified against the stub: JSON 0, empty 3, banner 4, [main] INFO log 4,
licence preamble then JSON 0, blank first line 0, Burp exit 7
propagated, SIGPIPE clean, banner text absent from stdout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* burpsuite-project-parser: flush per line, and pin the classification with a suite
Two gaps in the previous commit, both found by testing rather than reading.
awk block-buffers when its stdout is a pipe, and every documented workflow
pipes. So the claim that streaming through awk keeps `| jq` and `| head`
unbuffered was wrong in its second half: a long search over a large project
printed nothing until Burp exited, where the bare `exec` this replaced
streamed line by line. `fflush()` after each emitted object restores that for
one write per line. The temp-file half of the claim was right and stands.
The change also shipped with no test, while arguing from the AGENTS.md rule
that a checker inspecting zero items must not pass -- the same paragraph asks
for a fixture proving the checker still detects its target. The suite supplies
it: 16 assertions over a stub standing in for Burp, covering the working case,
exit 3 on no output, exit 4 on a startup banner and on a `[main] INFO` line
that a leading-bracket check would have taken for JSON, a banner ahead of real
JSON still succeeding, Burp's own non-zero status propagating with and without
output, SIGPIPE from a downstream `head` producing no stderr noise, and the
streaming behaviour above.
Verified the suite bites by mutating the script three ways -- dropping
fflush(), turning exit 3 into exit 0, and letting non-JSON reach stdout. All
three turn it red. The assertion count is asserted for the same reason the
script refuses to report an unverified empty result.
Still not covered, and recorded at the foot of the suite: the stub proves the
classification, not Burp's behaviour with the extension missing. If Burp
launches and waits rather than exiting, neither 3 nor 4 fires and the script
blocks. Settling that needs Burp Pro and a renamed extension JAR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* burpsuite-project-parser: cap the stderr mirror, and make exit 3 resolvable
Two problems with the previous commit, both in the case it exists to handle.
When no line is JSON -- the missing-extension case -- awk never writes to
stdout, so it never takes SIGPIPE, so a documented downstream `head -c 50000`
cannot close the pipeline early. It blocks to EOF while every line Burp
produced is mirrored to stderr, which the caller captures and which no
documented output limit covers. Measured on a 100k-line non-JSON stream behind
a `head -c 200` that could not terminate: 8,389,235 bytes across 100,005 lines.
Under the bare `exec` this replaced, those bytes went to stdout where `head`
truncated them, so this was a regression in the exact scenario the change
targets. Capped at 20 lines plus a suppressed count, the same stream now yields
2,014 bytes across 26 lines. Exit codes are unchanged.
Exit 3's remediation was not something the agent reading it could carry out.
`allowed-tools` is `Bash Read` and Burp runs headless, so "Check Burp Suite ->
Extensions" has no GUI to open, and re-running returns 3 again. That leaves
deciding the extension is loaded and reporting "nothing found" -- the false
negative this whole script exists to prevent, reached through its own
instructions, and reached often, since exit 3 is the common case for any
narrow regex. Both the script and SKILL.md now give a control query instead: a
selector broad enough that it must return rows if the parser works at all, run
against the same project. Rows mean the narrow query genuinely matched nothing;
exit 3 again means nothing comes back through the parser at all; exit 4 means
the flags were dropped. Asking the user to check the GUI is named as the
legitimate answer where the control is inconclusive -- guessing is not.
The suite gains two assertions for the cap, since an uncapped mirror looks
identical to a capped one on every stream the other 16 use. Verified by
removing the cap: the suite goes red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* burpsuite-project-parser: bound the mirror by length, skip blank lines, fix the control query
Three fixes to the previous commit, all in the parts that commit added.
The control query it introduced for exit 3 told the agent to run bare
`proxyHistory | head -n 1` -- the one selector this skill bans outright.
SKILL.md calls it "NEVER DO THIS" (:125), "NEVER use this directly" (:183,
:191), notes it "can return gigabytes" (:100), and sets a hard rule that body
content over 1000 chars must never enter context (:140). `head -n 1` does not
soften that; it delivers exactly one complete record, request and response and
bodies, which the file's own table says can be megabytes. And exit 3 is the
common case, so the advice would have fired constantly.
`proxyHistory.request.headers` answers the same question -- does anything at
all come back through the parser -- at the under-1KB-per-record the file
documents at :106.
The stderr cap bounded how many lines were mirrored, not how long one could
be, so a single very long line passed the count check and was written in full.
SKILL.md:217 already records that shape ("A single 10MB response on one line
will show high byte count but only 1 line"), and those bytes are captured HTTP
traffic on a channel the documented `head -c` on stdout cannot reach. Lines are
now truncated at 500 characters with the original length reported.
A whitespace-only line was counted as non-JSON, so one trailing newline turned
an empty-but-correct result into exit 4 -- "the extension is not loaded",
wrongly, on a healthy install -- and mirrored a diagnostic with nothing after
the colon, which is the empty-diagnostic defect
|
||
|
Randy Hanooman
|
49699bb474 |
second-opinion: Gemini CLI is EOL — move to Antigravity, and bump Codex to gpt-5.6-sol (#294)
Both documented invocations were broken. Verified against codex-cli
0.149.1, gemini-cli 0.57.0, and agy 1.1.21 by running each command
against a small diff with two planted defects.
Codex: the skill pinned `gpt-5.5`. Current family is gpt-5.6 with
`-sol`, `-luna`, `-terra`, and `-pro` variants; `gpt-5.6-sol` is the
general-purpose choice. Fallback chain is now sol -> 5.6 -> 5.4.
`model_reasoning_effort` gained `max` and `ultra`; `xhigh` is kept as
the default since the higher tiers cost wall time for little gain on a
diff-sized input.
Gemini: the documented path cannot authenticate at all. Google stopped
serving Gemini CLI to individual accounts (free, AI Pro, Ultra) on
2026-06-18; they now fail with
reasonCode: 'UNSUPPORTED_CLIENT'
reasonMessage: 'This client is no longer supported for Gemini Code
Assist for individuals. To continue using Gemini, please migrate to
the Antigravity suite of products: https://antigravity.google'
The replacement is Antigravity CLI, binary `agy`. Only Code Assist
Standard/Enterprise licenses and paid `GEMINI_API_KEY` still reach
Gemini CLI, so that path is retained as an explicitly-legacy reference
rather than deleted.
Three `agy` behaviours that break a naive port, all found by running it:
- The prompt must be a command-line argument. Print mode does not read
stdin unless `--input-format stream-json`; piping a diff in fails
with "empty prompt".
- `-p=<value>` is required. Bare `-p` consumes the next flag as its
prompt value and silently discards the real one.
- `--mode plan` is unsafe here. It intermittently returns a "created an
implementation plan artifact ... click Proceed" stub instead of the
review — same command, full review on one run, stub on the next.
`--json-schema` is also advisory rather than enforced: it nests the
findings as an escaped string inside a `response` field, contradicted
the schema on `code_location`, `confidence_score`, and the
`overall_correctness` enum, emitted the payload twice, and ran ~3x
slower than text. The Antigravity path therefore uses text output and
structured JSON stays Codex-only.
Also in this change:
- `--disable-slash-commands` on the `agy` call, since the prompt embeds
an untrusted diff.
- The dependency-scanning gate section is dropped from SKILL.md. It
existed only to gate `/security:scan-deps`, a Gemini security
extension command with no Antigravity equivalent. It survives in the
legacy Gemini reference, and the Antigravity reference says to use
osv-scanner/npm audit/pip-audit instead.
- `--skip-trust` added throughout the legacy Gemini path. `--yolo` is
silently downgraded to "default" approval mode in an untrusted
directory, so every headless extension invocation as previously
documented would stall waiting for approvals that cannot arrive.
- A note on why `codex exec review` is still not used despite gaining
native `--uncommitted`/`--base`/`--commit` flags: those are mutually
exclusive with `[PROMPT]`, so it cannot carry project context or a
focus area.
`make validate`, `make self-test`, and both loadability checks pass.
Co-authored-by: rhanooman12 <randy.hanooman@jasper.ai>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com>
|
||
|
dm
|
b89f6dbc35 |
Update brocards source URL (#295)
Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
kz-tob
|
6feac677af |
building-secure-contracts: require a per-pattern coverage table in the five flat scanners (#287)
* building-secure-contracts: require a per-pattern coverage table in the five flat scanners Each scanner advertises a fixed pattern count but nothing forced a report on each one, so a scan that examined four of eleven patterns produced output indistinguishable from a clean contract. Adds a coverage table with pre-filled rows per scanner (algorand 11, cairo 6, solana 6, substrate 7, ton 3) and three verdicts: found, clear, n/a with a reason. Not having looked is not n/a. Adds a chain-specific Rationalizations to Reject section to each. Also aligns ton's pattern summary with VULNERABILITY_PATTERNS.md, which the frontmatter description already matched: Integer as Boolean rather than Integer Overflow, which is a different bug class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * building-secure-contracts: build every table from the canonical pattern list The TON drift fix was right and I did not apply the same audit to the rest. Cairo and Algorand tables were built from their SKILL.md §6 summaries, which disagree with resources/VULNERABILITY_PATTERNS.md -- the file §6 sends the model to read. Cairo: only arithmetic overlapped. Every L1/L2 pattern and signature replay had no row, including "Unchecked from_address in L1 Handler", which is the file's own CRITICAL Finding Template example and the bug the new Rationalizations section calls the canonical StarkNet bridge bug. Algorand: no row for unchecked transaction fee, AssetCloseTo, access controls, asset ID verification or inner transaction fee, all five already items in the same file's §12 checklist. That is worse than before this branch: the omission used to be silent, and a coverage table turns it into a completeness claim that licenses stopping. Both summaries and both tables are now rebuilt from the canonical files, as is the plugin README, whose Cairo list was a third distinct pattern set and whose TON list named three patterns that no longer exist. In both cases the frontmatter description already matched canonical; only the summary had drifted. Also: - A checklist item in all five, so a missing table is detectable where a run checks itself rather than only in prose upstream. - The §4 output list and the §5 example reports showed a full report with no table -- a concrete artifact beats an abstract instruction, and the model copies the example. - Solana's pattern 5 is version-scoped like Substrate's pattern 4 but had no version rule, so "targets 1.17" and "did not look" read alike. - Substrate's table said "above the findings" and "below" in a skill with no findings section to anchor either. - TON §4 still listed replay protection as scope, a category with no row. Verified mechanically: all five tables now match their resource file in count and order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * building-secure-contracts: fill row 1 so the table is not a blank skeleton Every table shipped with empty Verdict and Evidence cells, and both gates -- "with all 6 rows present" and the new §12 checklist item -- are satisfied literally by a table whose every verdict is blank. To a skimming reader an emitted table looks like completed coverage, which is the silence-equals-absence failure this branch fixes, one level up. Algorand and TON had filled rows in §5 Example Output; cairo, solana and substrate had no rendering of the artifact but the blank skeleton. Row 1 of each table is now filled in with a realistic verdict and evidence -- one found, one clear naming what was searched, one n/a with a reason -- and the row-count rule now says a row with an empty Verdict cell is incomplete in the same way as a missing row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- 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
|
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> |
||
|
Akshay K
|
304c81a8ce |
static-analysis: make the codeql skill's guards real and add tests (#225)
* fix(codeql): make the skill's guards real, add tests, trim the prose Every verification step piped its command to a formatter and used the pipeline's exit status, which without pipefail belongs to the formatter, not the command. Nine sites. The sharpest was the arm64e detection: `EXIT_CODE=$?` after `| tee` compared against 137, a value it could never hold, so the check underpinning Essential Principle #5 and three Rationalizations could not fire. The build workflow now runs check_db_quality.py as its Step 4 exit condition rather than describing metrics it never compared to a threshold, and suite generation aborts on zero resolved queries. Two blocks that were invalid bash — an else branch containing only comments — are fixed. Log helpers move to scripts/build_log.sh, sourced by the workflow and by the three reference docs that use run_logged. They were previously defined in one markdown file and used from three others, so those blocks failed standalone. As the skill's first .sh file it is also the first thing here that `make shell` lints. Sourcing it now checks the log is writable: under pipefail an unwritable log made tee's failure the pipeline's, so run_logged returned 1 for a build that succeeded and the method ladder walked to --build-mode=none blaming CodeQL. Suite generation moves to scripts/generate_suite.sh, which takes the mode as its argument. Both modes had been copy-pasted into two reference docs sharing some 25 lines of identical scaffolding — guards, the third-party pack loop, the excludes, the verification call — none of it lintable where it sat. The tests now run the script instead of extracting bash from markdown, and one of them fails if either doc inlines a generation block again. check_db_quality.py counts project files under the source root recorded in codeql-database.yml instead of against a hardcoded prefix list that only knew where macOS keeps its toolchain. src.zip stores each file at its absolute path minus the leading separator, so the recorded root is a prefix of the project's entries and of nothing else — verified against a database built by CodeQL 2.25.6. It also resolves the summary-versus- severity preference per extractor: a single `extractor-failures: 0` used to suppress the fallback for every other language in the database. run-analysis.md had its own database discovery loop, which SKILL.md states the workflows do not restate. It also lacked SKILL.md's `codeql resolve database` filter, so a marker file left by a failed build could be selected as a database. It now uses the canonical block. Twenty-odd snippets across language-details, threat-models, and performance-tuning created and analysed a literal `codeql.db` in the working directory — the exact shortcut SKILL.md lists as a Rationalization to Reject, and one its Success Criteria forbid. They use "$DB_NAME" under $OUTPUT_DIR. Adds six hermetic test suites needing no CodeQL install, plus the first tests that execute build_log.sh rather than reading it. They cover the shell in every markdown block, both suite generators, the quality thresholds, the .qls templates, and the exit-status claim the build ladder rests on. Corrects the run-all coverage figures against cpp-queries 1.8.0: the pack holds 515 alert queries, not 510, and run-all leaves 307 of them unrun, not 302 — 13 of those are Security/CWE queries, which the prose described as harmless refactoring metrics. Trims the skill's markdown from 2867 lines to 2748, and SKILL.md from 269 to 257. Out: the When NOT to Use section that #216 dropped repo-wide, five Essential Principles that restated their own Rationalization, two literal prompt mock-ups, three Reference Index rows duplicating the workflow table above them, three identical qlpack.yml blocks, and code fences in performance-tuning and threat-models that each carried a single flag. Replaces three approval prompts with one confirmation gate, and drops allowed-tools entries for tools that no longer exist. Bumps static-analysis to 1.3.0. * fix(codeql): address review findings on shell-block scope and suite claims Each markdown block runs in a fresh shell, so scalars and sourced functions cross it no better than arrays do. build-database.md now says to re-source build_log.sh and re-set DB_NAME in every block using run_logged: without it run_logged exits 127, the method ladder reads that as a failed build method, and it walks to --build-mode=none having never invoked CodeQL. run-analysis.md Step 4 re-establishes DB_NAME, RAW_DIR and SUITE_FILE for the same reason -- under set -u it aborted with "unbound variable" before analysis started. create-data-extensions.md was the third caller of database discovery and still used a bare find for codeql-database.yml, which selects a marker left by a build killed mid-run; the queries then return nothing and Step 3 reports coverage as adequate. It now uses find_databases.sh like the other two. quality-assessment.md sources build_log.sh in the Collect Metrics block, so a failed quality gate is recorded rather than silently dropped by a command-not-found, and the raised-threshold override logs inside the if -- it previously wrote "raised to 15%" even when the re-run still failed. generate_suite.sh no longer describes the run-all suite as every security, experimental and quality query: it imports two suites totalling 219 of the pack's 515 alert queries, as run-all-suite.md documents. Changed in the doc template too, which test_generation_scripts.py pins to the script's output. The two remaining review findings were already fixed on this branch and needed no change: quality-assessment.md assigns ERROR_RATIO from the script's JSON before reading it, and extractor_error_count() resolves the summary-versus- severity preference per extractor rather than once for the tree. * style(codeql): trim comments in the shared shell scripts build_log.sh carried more comment than code. The consequence of an unwritable log -- the ladder walking to --build-mode=none after a build that succeeded -- was stated twice within ten lines; it is stated once now. find_databases.sh and generate_suite.sh get the same treatment. Comments only: the diff contains no code lines. 435 tests pass unchanged. * feat(codeql): ship /static-analysis:codeql-build as a dynamic workflow The build is the part of this skill with real judgement and no user in it: try a method, read the failure, apply a fix, retry, escalate. That loop now runs unattended as workflows/codeql-build.js, beside the four workflows already on main. Three phases. Detect resolves the output directory and profiles the language, build system and macOS arm64e state. Build walks the ladder. Assess runs check_db_quality.py and applies the improvements from quality-assessment.md before re-running it. The ladder is deterministic and lives in the script; diagnose-fix-retry for a single rung lives in that rung's agent, where the build output it has to read already is. The agent is told not to escalate itself -- the caller owns the order, so a rung that fails is a result rather than a licence to try something else. Go and Swift never reach Method 4, which they reject outright; an arm64e Mac starts at 2m rather than spending two rungs to reach the same SIGKILL; an interpreted language does one extraction and no ladder at all. Nothing is asked. Every method failing returns no-method-succeeded, and a database that built but sits below the quality threshold returns built-below-threshold with its metrics. Whether the remaining extractor errors are confined to code nobody needs analysed is the caller's call, so the assess phase is told not to raise --max-error-ratio to make its own gate pass. A build command exiting 0 is not a database: every rung is confirmed with codeql resolve database before it counts, because finalize after a failed trace-command leaves one that resolves and holds nothing. tests/codeql_build_harness.js compiles the workflow with stubbed agents and asserts the ladder and the guards; --self-test mutates it six ways and requires every mutation to turn a scenario red. run_codeql_build_tests.sh wraps both so CI's existing shell-suite discovery runs them, with no workflow file changes. SKILL.md documents it as the unattended alternative and keeps the manual path. Database selection, analysis planning and data extensions stay in the session. * refactor(codeql): stop the docs recomputing what check_db_quality.py reports Collect Metrics parsed baseline-info.json with an inline python3 -c into BASELINE_LOC, then read .baseline_loc out of the checker's JSON into DB_LOC in the same block -- the same number, computed two ways, logged twice under two labels. The block's own comment said "Everything downstream reads $QUALITY_JSON rather than recomputing" while the recount sat sixteen lines above it. The inline parse and the print-baseline call before it are gone; the checker is the only thing that counts now, and the Quality Criteria table cites it rather than a command the doc no longer runs. Log Assessment read five variables out of Collect Metrics' shell. It runs in its own, so they expanded to empty and the log recorded "Baseline LoC:" with no number -- the same shape as the dangling ERROR_RATIO the first review found. It re-sources the helpers and re-reads the metrics, and no longer prints an expected-file count it cannot see. test_shell_blocks.py's embedded-python guard required three matches and there are two now, which is the guard working: removing the last sample of a construct must fail rather than silently leave the extractor untested. The floor moves to two, with the reason recorded. * test(codeql): drop test_suite_resolution.py, which never ran in CI Its six tests needed the CodeQL CLI and codeql/cpp-queries; CI installs neither, so every one of them reported as a skip on every run. Confirmed by reproducing CI's environment locally -- with codeql off PATH the directory gives 428 passed, 135 skipped, matching the job log exactly. Removing it leaves 428 passed, 129 skipped, and the remaining skips are per-block parametrisations of test_shell_blocks.py whose test functions do run for other blocks. What goes with it: the only check that resolved the suite templates against a real CodeQL rather than a fake one. run-all-suite.md's coverage claims -- that run-all is not the whole pack, that important-only reaches queries run-all does not -- are now prose nobody verifies, so the doc carries the command to re-derive them instead of pointing at a test file that is gone. test_generation_scripts.py's docstring no longer claims a sibling covers real-CLI resolution. * fix(codeql): Rust supports --build-mode=none; say so The language table left Rust as "check your CLI — not listed either way" and the Overview's three categories did not cover it at all, so a reader had no route for a Rust project and codeql-build.js resolved the ambiguity silently by putting Method 4 on its ladder. Settled against CodeQL 2.25.6 rather than the help text: `codeql database create --language=rust --build-mode=none` exits 0 and writes a database with `finalised: true`. Go, for contrast, fails immediately with "Go does not support the none build mode". So `--help` omits Rust the same way it omits C/C++, which the Overview already warned about; Rust joins that category and the ladder in codeql-build.js was right. * refactor(codeql): one prose copy of the quality-gate exit codes, not two check_db_quality.py's exit contract was written out four times: the script's own docstring, codeql-build.js's assess prompt, build-database.md Steps 4-5, and quality-assessment.md's Enforce the Thresholds. The first two earn it -- one is the source of truth, the other is an agent prompt that cannot read a docstring. The two prose copies are one too many. build-database.md keeps the part a reader needs at that moment (exit 1 is not a judgement call, exit 3 is) and defers the table to quality-assessment.md, which it already links and which is where someone looks for gate detail. The raised-threshold rationale there loses two lines it did not need. * refactor(codeql): assess phase reads the exit-code table instead of copying it The Assess prompt spelled out all four exit codes while its sibling phases point at a file -- Select says to read rulesets.md rather than choose from memory, and each build rung is sent to build-fixes.md. It now reads "Enforce the Thresholds" in quality-assessment.md the same way, keeping inline only the two facts that decide what it does: exit 1 is not overridable, exit 3 is a heuristic. That leaves one prose description of the contract instead of two, and a change to the script's exits reaches the agent without a second edit. Also aligns the ladder comment with the Rust finding: --help omits C/C++ and Rust, not just C/C++. * fix(codeql): pass --format=json, the flag check_db_quality.py defines The workflow's Assess phase ran `check_db_quality.py --json`. The script takes --format {text,json}, so argparse exited 2 before reading the database, and 2 is the one exit code ASSESS_SCHEMA does not describe. test_script_flags.py checks the class: it reads each script's accepted flags from its own --help and verifies every invocation across the plugin's .md and .js. The bug shipped because test_shell_blocks.py scans the skill tree and codeql-build.js sits outside it. * test(codeql): scan every block in one test instead of parametrizing over all of them Seven tests parametrized over all 65 bash blocks, which is 455 cases for seven assertions, and three of them skipped the blocks that did not qualify. That was 129 skips, and it hid a check that matched no block at all: the unpreserved-pipeline assertion had never run against the skill. Each now scans in one pass and lists every offending file:line, so a run reports all offenders rather than the first. 557 cases down to 118, none skipped. The array collector's empty case is an assertion rather than a skip. * refactor(codeql): point the workflow at build-database.md instead of restating it codeql-build.js carried its own copy of the build procedure: the arm64e detection block verbatim, the build-system command table, every method's invocation, and the output-directory logic. Both copies were live, which is how the workflow came to pass check_db_quality.py a --json flag while the doc had --format=json. Each rung now names its section of build-database.md, the way Method 2m already pointed at macos-arm64e-workaround.md. quality-assessment.md called the checker three times and re-derived its numbers with jq, unzip and grep. check_db_quality.py now reports archive_files and finalised from the two files it already reads, so one call covers the whole assessment. The fresh-shell rule was stated in five places; SKILL.md holds it once and the workflows link to it with the consequence specific to their site. test_section_pointers.py checks what this trade depends on: every "Section" in file.md pointer and every #anchor link must land on a real heading. The repo validator resolves paths, not section names, so a renamed heading would leave the pointers aimed at nothing with every check still green. Prose 1128 -> 1084 lines, codeql-build.js 390 -> 340. * fix(codeql): address PR review findings on skill paths and database selection codeql-build.js hardcoded plugins/static-analysis/skills/codeql, which only resolves in a checkout of this repo. Installed, the first build block sourced a build_log.sh that was not there and exited 127, so every rung of the ladder reported a build failure for a project that would have built. The Detect phase now resolves the directory at runtime from $CLAUDE_PLUGIN_ROOT, $CODEX_PLUGIN_ROOT, or a find over ~/.claude and ~/.codex, accepting a candidate only when scripts/build_log.sh exists. skillDir is a required schema field validated as absolute, so an unresolved path stops the run before the ladder starts. SKILL.md built FOUND_DBS in one bash fence and looped over it in the next. Each fence is a separate shell, so the metadata loop iterated zero times and the selection prompt had no language or creation time to show. The two are now one block. run-analysis.md Step 1 branched only on the zero-database case and fell through to FOUND_DBS[0] for any other count, analysing whichever database find returned first without telling the user there was a choice. It now uses the elif/else shape from create-data-extensions.md and exits with an error when DB_NAME is still unset. Two new checks cover the class rather than the instance. test_shell_blocks.py fails when a block reads an array it does not build. test_section_pointers.py fails on a repo-relative plugins/ path in the workflow, and resolves ${SKILL_DIR} pointers against the skill root so the workflow's file references stay checked: nine of them now, up from two. The rest of the review: run-all-suite.md no longer claims total coverage, which its own measurement section refutes; the analyze block expands optional flags as ${ARR[@]+"${ARR[@]}"}, since bash 3.2 treats an empty array under set -u as unbound; find_databases.sh exits 2 when codeql is absent instead of printing nothing, which auto-detection reads as "no databases, rebuild"; make -j$(nproc) falls back to sysctl -n hw.ncpu on macOS; every . build_log.sh site is || exit 1, as none of those blocks set -e; the Reference Index lists find_databases.sh and generate_suite.sh. * fix(codeql): source build_log.sh in every block that uses its helpers Each fenced block is its own Bash call, so a helper defined in an earlier one is undefined and exits 127. The build ladder reads that as a failed method and walks to the next one, reporting failure for a build that was never attempted. test_shell_blocks.py now fails any block that uses run_logged, log_step, log_cmd, log_result or LOG_FILE without sourcing build_log.sh, with fixtures pinning the detector in both directions. * fix(codeql): gate each step of the Method 3 multi-step build build_log.sh does not set -e, so the four run_logged calls ran regardless of each other: a failed trace-command still reached finalize, and the resulting database resolves while holding nothing, which the ladder reads as success. The steps now chain through if/elif, as 2m-a already does. test_shell_blocks.py fails an ungated `codeql database finalize`. * fix(codeql): stop database discovery reporting none when it found some find_databases.sh resolves each root to an absolute path, so the old -not -path '*/.*' exclusion also matched dotted ancestors: a checkout under ~/.cache or ~/.local had every database filtered out, the script printed nothing and exited 0, and the caller rebuilt from scratch. Prune dotted directories by name instead, with -mindepth 1 so a root that is itself dotted still searches. All three callers read the script through a process substitution, whose exit status is unobservable. Exit 2 (no codeql on this shell's PATH, a fresh shell per block) arrived as an empty list and was reported as "No CodeQL database found" for a project with several. Read it with command substitution and check the status. Tests cover the dotted-ancestor case, the dot-directory-below-root case the fix must not widen into, and a block scanner rejecting a process substitution around the script. * chore(static-analysis): bump version to 1.3.1 #231 bumped the plugin to 1.3.0 on main after this branch had already done the same, so merging main left HEAD and the merge base equal and the version-increment check failed. --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Eric Quintero
|
e6066e7db1 |
Rebuild supply-chain-risk-auditor around a deterministic collector (#227)
* Rebuild supply-chain-risk-auditor around a deterministic collector Replace the gh-only audit method with two stdlib-only Python scripts bundled with the skill: collect.py queries OSV, the npm and PyPI registries, the Go module proxy, deps.dev, OpenSSF Scorecard, and GitHub, and emits a JSON artifact; render.py turns it into a Markdown report of facts only. The model's job is the judgment layer on top — remediation, replacement candidates, narrative — written in report register and labeled as judgment. The old method could not deliver its own criteria: repository contributors are not registry publish rights, and gh sees no download counts or ecosystem-keyed advisories. What it measures, for npm, PyPI, and Go: - Version-matched advisories for direct dependencies, resolved from the lockfile, manifest pins, or labeled fallbacks — and for the full lockfile-resolved transitive tree (package-lock.json, uv.lock, go 1.17+ go.mod), advisories only. - Abandoned or archived upstreams, deprecated and yanked releases, npm publisher concentration, install-time script execution, and the two OpenSSF Scorecard checks that name a concrete mechanism (Dangerous-Workflow, Binary-Artifacts). Download volume, publish provenance, and security policy are reported as context, never flagged. The structure enforces its honesty rules rather than documenting them: - Every criterion resolves to assessed-clean, assessed-flagged, or unassessable-with-a-reason. Unavailable data is never evidence of risk, and every claim is bounded by a coverage table. - An empty advisory answer counts as clean only for a package proven to exist: a registry document for npm and PyPI, a module-proxy answer for Go, and for transitive lockfile entries a registry integrity hash or registry source. Everything else — private registries, git dependencies, vendored directories — is named as unverifiable with its reason, never counted clean. - Coverage must reconcile, a run that measures nothing exits non-zero instead of reporting that nothing is wrong, the renderer refuses an artifact whose flags and coverage disagree, and third-party text is escaped before it reaches a Markdown table. 85 offline tests exercise the invariants through the collector's own cache format, and the suite is mutation-checked. evals/ ships three fixtures with graded expectations; against a no-skill baseline the skill passed ~92% of skill-agnostic assertions vs ~60%, at half the wall clock, with its edge in reproducibility — the report regenerates byte-for-byte from the artifact — and self-consistency. Version 1.0.1 -> 2.0.0: method replacement. CODEOWNERS moves to @e-q. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Establish registry identity at the parse boundary; reconcile the sweep against the raw lockfile Review findings on the PR clustered at one boundary: a non-registry or malformed identity reaching the registry-keyed pipeline. Close the class, not the instances: - Dependency gains non_registry_reason, set by the parsers (npm file:/ workspace:/git/shorthand specs; PyPI direct deps whose uv.lock source is git/directory/path). One choke point in collect() marks every criterion unassessable with that reason and excludes such deps from all lookups — a same-named public package's advisories, publishers, and deprecation belong to code the project never installs. The deps stay in the report and its coverage. - Versions extracted from requirements text pass a PEP 440-shaped gate; pip-compile continuation/hash debris ("2.19.0 \") becomes unresolved instead of a version-matched claim. Measured: OSV compares garbage versions lexically, so the debris did not fail — it matched the wrong advisory ranges. - The transitive sweep excludes direct dependencies by (ecosystem, name, version), never by name: a nested copy of a direct dependency pinned at another version is this sweep's responsibility, and the name-keyed exclusion silently dropped it (measured on axios: two recovered entries, 620 -> 622). The artifact now carries a ledger counted from the raw lockfile before any exclusion — checked + unverifiable + excluded_direct must equal it, so a dropped triple fails validation instead of vanishing while the counts balance. The checked==0 guard hole is closed: zero reconciles like any number unless a reason is stated. - pip-audit runs with --no-deps --disable-pip. Measured: --no-deps alone still audited a pip-resolved transitive set, so pip was still fetching and potentially building untrusted distributions, against the tool's no-execution promise; with both flags it audits exactly the listed pins from registry metadata. Names on both sides of the cross-check are PEP 503-normalised. - Scorecard check thresholds move to model.SCORECARD_CHECKS as the single source of truth; the renderer derives its never-flags set from threshold-is-None, ending the clean-run contradiction that described the two flagging checks as "not flagged — poor precision". - Duplicate requirements prefer the runtime declaration (the dev file sorts first, so first-seen-wins reclassified production pins as build-time at the dev version); _git_commit confines refs to .git and degrades to None on undecodable content instead of crashing the run; recognised-but-unread lockfiles (yarn.lock, pnpm-lock.yaml, poetry.lock) produce a note and the docs name exactly which lockfiles are read; third-party text cannot inject links; the runtime estimate is honest. 88 tests; the four new guards (triple-keyed exclusion, non-registry choke, lockfile ledger, zero-checked reconciliation) are each mutation-checked against the exact reviewed bug. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Mark PEP 508 direct references as non-registry at the parse boundary `flask @ git+https://...` in pyproject.toml or requirements.txt was stripped to a bare name — `@` is a name terminator in _REQ_SPLIT, so the URL was silently discarded — and the dependency was looked up on PyPI, attributing the public package's advisories and metadata to a fork that may exist precisely to fix them. The npm path guards this at spec parsing and the PyPI path guarded it only via uv.lock's source table, so any pip-managed project walked past the choke point. Direct references are now detected in the requirement text itself and carry non_registry_reason with the URL; the existing choke point does the rest. Assert folded into the requirements parsing test and mutation-checked against the reproduction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Escape third-party text everywhere in the report, not only in tables The Method-and-caveats notes and the report header interpolated untrusted strings unescaped. Reproduced end to end: a package.json dependency key containing newlines (JSON permits them) with a file: spec produced a note that wrote a `## Summary` heading and a forged "No known advisory affects any of the 12 direct dependencies" bullet into report.md — a deliverable meant to survive being pasted into a client report. The 12-byte commit field read from the target's .git/HEAD had the same reach. Whitespace collapsing is the half that matters: it confines hostile text to the line it was interpolated into, where the worst available is inline emphasis rather than forged block structure. The mechanism was already right, so this is the missing calls plus a rename — _cell is now _safe_text, since a table-shaped name is what invited skipping it off-table. The existing table test asserted on the row it expected, which is why it never noticed these paths; the new test asserts structurally that every heading and bullet in the report came from the renderer. Both escaping calls are mutation-checked. Normal reports are byte-identical: the note templates carry no pipes or brackets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Accept v-prefixed and epoch versions; stop escaping inside code spans A regression review comparing the branch tip against its first commit found that two of the earlier review fixes had costs worth paying back. The PEP 440 version gate required a leading digit, so the legal pin `django==v3.2.0` became an unresolved version: advisories were matched against the latest release and 62 real ones for that version read as assessed_clean. PEP 440 permits the prefix, pip accepts it, and OSV matches it. The gate now accepts and strips it, so the reported version is canonical. Probing that also surfaced a longer-standing defect in the same path: extraction split on `!`, which is there for `!=` and truncated a PEP 440 epoch, so `1!2.0` was reported as the pin `1`. Extraction now ends the version at whitespace, a comma, or a semicolon, which additionally recovers the real pin from pip-compile hash lines that previously fell back to unresolved. Markdown does not process backslash escapes inside a code span, so escaping there wrote the backslashes out literally: the report title and the `Scanned:` path came out as `/tmp/pkg \[v2] \| beta`, which is not a path a reader can copy. Values inside backticks now go through _safe_code, which collapses whitespace and neutralises the one character that matters there — a backtick, which would close the span early — and leaves the rest alone. Prose and table cells keep _safe_text, so link forgery and cell escapes are unchanged. One first-party note lost its literal brackets rather than being escaped around them. Non-registry dependencies now share one unassessable reason, with the specific source in the signal value and the Method note. Embedding the source in the reason gave each dependency a unique string, which defeated the report's grouping: a 7-workspace-package fixture produced 91 near-identical bullets across 13 criteria, and the Not-assessable section went from 56 lines to 132. It is back to 56. Also: a collector-level fixture now proves the transitive ledger is sourced independently of the buckets it checks — deriving it from them made the equation true by construction and left a dropped package undetected while all tests passed. The docstring no longer claims the ledger is counted from the raw lockfile, which overstated its reach, and _locked_beyond_direct's return annotation matches its five values again. 91 tests; all four fixes mutation-checked. Real reports are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Keep the pipe escape for code spans inside table cells Splitting the escaping into prose and code-span variants dropped the pipe escape from both, but a GFM table row is split on pipes before inline spans are parsed, so a pipe inside a code span still ends the cell. A dependency named `evil|forged` put six boundaries in a five-column row: the name truncated to a bare backtick and `evil`, and every later value shifted one column right, so "none known" rendered under Other findings. Verified against a CommonMark+GFM parser that this is genuinely a third context rather than a reason to revert: inside a table cell `\|` renders as a literal pipe, while outside one the backslash survives into the output — which is the corrupted `Scanned:` path the split fixed. The three table paths now use _safe_code_cell; the bullet and header paths stay on _safe_code. The new test asserts column parity across every table in the document rather than one row in one table, counting the pipes GFM actually splits on so an escaped pipe reads as content. Per-path assertions are what let this reach three call sites at once, and what missed the notes path two commits earlier. Each of the three sites is mutation-checked independently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Check the assembled document instead of trusting every escape site Escaping was applied per interpolation site, and three successive commits each fixed one set of sites correctly while leaving siblings unguarded: the notes and header fields, then three table paths, and now the download-volume line and the informational sample. Coverage depended on whoever wrote or reviewed the diff noticing every interpolation, which is the wrong thing to rest a property on when the failure mode is a client deliverable carrying a forged all-clear. render() assembles a list of lines and joins them, and every legitimate line is appended as its own element, so two invariants are precise and cannot be violated by legitimate content: no assembled line contains a newline, and every table row carries its header's unescaped-pipe count. check_no_forged_lines enforces both immediately before the join. Block forgery needs a newline to open a new block, so the first invariant catches any site that leaks, including sites not yet written; verified by reverting each of the two newly-escaped sites, which now fails seven existing tests rather than none. The two open sites are escaped as well rather than left to the invariant: with escaping a hostile name renders harmlessly and the audit completes, while the invariant alone would let any audited repository deny its own audit by naming a dependency with a newline in it. The shared test fixture now carries a newline, a backtick, and a pipe in its default name, so every render test drives adversarial input through every path it touches. Its informational criteria carry real booleans too: they were ints, and informational_section sorts on `is True` / `is False`, so the path that interpolates names into a Without: sample had never executed in any test. That combination is why the misses kept recurring. 93 tests; both invariant loops mutation-checked. Real reports are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Akshay K
|
94abac91ca |
static-analysis: convert the semgrep scan fan-out to a dynamic workflow (#231)
* feat(static-analysis): ship /static-analysis:semgrep-scan and a scan runner Two entry points over one implementation. SKILL.md keeps its gated five-step path, where the user reviews and edits the ruleset list before anything runs. workflows/semgrep-scan.js runs the same scan end to end without stopping. Both read the same references/, so a ruleset added to rulesets.md reaches both at once. This is the shape variant-analysis uses. The workflow sits at the plugin root beside the four already on main and ships as /static-analysis:semgrep-scan. Four phases: Detect resolves the output directory and profiles languages and Pro, Select reads references/rulesets.md and writes rulesets.json, Scan runs the script, Report post-filters, merges and summarizes. Step 4 is skills/semgrep/scripts/run-scans.sh, not a fan-out of subagents. Nothing in the scan phase needs judgement: the agents would run fixed commands and report $? and a jq count. Exit codes now stay with the processes that produced them and finding counts come from the JSON they wrote, so no phase reports on work a later phase has to go behind and re-verify. --metrics=off, the --include scoping, the output-directory --exclude and the severity flags are properties of the script. Cross-language rulesets run once rather than once per language and never take --include; a ruleset already in baseline is dropped from its language; language keys fold onto a canonical name, so js and javascript are one unit; two spellings of one repository collapse to one clone. Parallelism is the script's --jobs. The workflow does not stop for ruleset approval. None of main's four ask the user anything, and invoking one with a target is the opt-in. The scan is read-only over the target -- no --autofix or --fix is ever passed, every write lands inside the output directory, and the script refuses to run when the output directory is the target. Semgrep rules are declarative YAML, so pointing --config at a cloned rule repository executes nothing from it. The gate was scope confirmation rather than protection from a dangerous action, and what ran is recorded in rulesets.json and scans.json either way. A deliberate change to a security skill's stated policy, not an oversight; the gated path remains for when the ruleset selection is the thing that matters. Fixes four defects the old prose path carried: allowed-tools omitted the tool Step 4 needed; --severity MEDIUM/HIGH/CRITICAL is rejected by semgrep, so important-only mode never ran; each scanner deleted the shared repos/ clone while others were still reading it; and the scanner agent declared its tools as a comma string where the loader expects a list. Two bugs the new suites found while being written. `eval "$cmd" &` reports exit status 1 whatever the command exited with, which marked every failed scan as a success; commands are built as an argv array and executed directly, which also leaves no quoting surface. And the target was resolved with `cd && pwd` while the output directory was not, so on any path crossing a symlink -- every path under /var on macOS -- both the equality check and the inside-the-target test missed, and the run scanned its own cloned rule repositories. tests/run_scan_tests.sh covers the script: command generation via --dry-run, and execution, exit codes and clone failures against stub semgrep and git binaries. tests/workflow-harness.js compiles the workflow with stubbed globals and asserts that a relative target, an output directory equal to the target, a dead phase and a failed scan each stop the run rather than reach the report as an empty result; --self-test mutates the workflow six ways and requires every mutation to turn a scenario red. Both are hermetic, reach no network, and CI's existing shell-suite discovery runs them, so no workflow file changes. Removes references/scanner-task-prompt.md and the no-Workflow fallback it served. There is no hand-rolled path and no second implementation in prose. * fix(static-analysis): resolve the skill dir at runtime, filter the merged SARIF, clear stale raw output Three defects flagged by kz-tob on #231. The workflow hardcoded SKILL_DIR as a repo-relative path, so every scripted command only resolved inside a checkout of this repo. A marketplace install runs with the user's own project as cwd and the scan phase would have found no run-scans.sh at all. Resolved at runtime instead, folded into the Detect phase, following the cascade variants.js already uses. Each candidate ends at scripts/run-scans.sh, which makes a stale install self-excluding: verified against the 1.2.2 install on disk, which ships merge_sarif.py and nothing else, and the glob correctly declines to bind to it. An unresolved directory throws rather than leaving an agent to compose semgrep commands by hand. In important-only mode both the workflow and scan-workflow.md told the agent to apply the scan-modes.md jq filter to the merged SARIF. That filter reads .results[].extra.metadata, which SARIF does not have, so it exits with "Cannot iterate over null" and results.sarif stayed unfiltered while the JSON side was filtered. The metadata is not recoverable from SARIF, but finding identity is: (check_id, path, start.line) and (ruleId, uri, region.startLine) match field-for-field, confirmed against real semgrep output, and it is the same triple the merge already dedups on. merge_sarif.py --important keeps the findings the JSON filter kept and fails rather than filtering if any scan has no *-important.json beside it, since a partial key set would drop real findings from the deliverable. The merge command blocks in SKILL.md and scan-workflow.md show both modes, so copying the block without reading the paragraph under it cannot produce an unfiltered deliverable. run-scans.sh never cleared raw/. merge_sarif.py globs every *.sarif there, so a rerun into a reused output directory that dropped a ruleset still merged the previous run's output for it. Tests: 58 shell assertions (+3), 46 workflow assertions (+13) with three new mutations, and 16 pytest cases for merge_sarif.py. Each fix is mutation-tested; reverting any one of them turns the suite red. * fix(static-analysis): honor a bare-path arg and guard the important-only merge * fix(static-analysis): fail the important-only post-filter loudly on a jq error * fix(static-analysis): report merge failures and exclude failed scans * fix(static-analysis): log the approved plan once and flag zero-coverage rulesets * fix(static-analysis): drop the SARIF Multitool merge path * fix(static-analysis): report SARIF files the merge could not read * fix(static-analysis): record the exclude pattern applied to every scan * fix(static-analysis): move the exclude-pattern assertions after their fixtures * fix(static-analysis): stop SIGPIPE marking a healthy rule repo as empty * test(static-analysis): pin exclusion for a target holding glob metacharacters * docs(static-analysis): describe the semgrep skill as it now runs --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> |
||
|
Fredrik Dahlgren
|
7b9bd5f950 |
Add writing-lean-proofs plugin (#226)
* Add writing-lean-proofs plugin Structured Lean 4 proof writing and library design following Mathlib conventions: sorry-skeleton workflows, API-first definitions, lemma extraction, and an anti-pattern catalog mapped to the linters that enforce each rule. Includes a review-flow eval suite (evals/): five Lean fixtures derived from a real formal-verification project with known planted flaws and known non-flaws, natural review prompts, per-case rubrics, and a runner comparing a baseline arm against a skill arm with an LLM judge plus a deterministic no-rewrite check. The grader ships a self-test that asserts a known-bad review fails every criterion. Smoke-tested: the baseline arm reproduces the folk-advice mistake the skill corrects (calling redundant `show` lines noise); the skill arm passes 5/5. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address Claude review findings - Validate the grader's "id" field before writing grades.json, so a malformed grader response fails with a clear error instead of a KeyError in the summary. - Restore title-case "When to Use"/"When NOT to Use" headings: the validator's required-section check matches them case-sensitively. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address github-actions review findings Eval harness: - checksum_tree fails when it finds zero .lean files, so the no-rewrite check can no longer pass vacuously - both arms run with --setting-sources project, so a user-level install of this skill cannot contaminate the baseline arm - a failed case is recorded and the run continues; the runner exits non-zero after printing the summary - critical steps in run_case return explicitly (errexit is suppressed when the function runs in an if-context) - self-test prints its artifact dir; the on-failure leak is intentional - case 04: dropped the trailing omega so the squeezed simp only in carry_le_one is genuinely terminal (the direction test was inverted) - case 05: overall-verdict retyped from must-flag to overall, with the grader prompt told to judge such criteria solely by pass-when text - fixture overlap removed: Felt.lean's unscored unfold replaced by a bound-free lemma; Bounds.lean's unscored Fact instance replaced by the NeZero instance ZMod.val_lt actually needs Skill content (claims verified against Mathlib docs and the ImProver paper): - lake build alone does not catch sorries (they are warnings); step 4 now includes an explicit grep gate - style linters are enabled in Mathlib's own build but off by default downstream; the opt-in is now spelled out. The show, nameCheck, and setOption linters do exist, so those attributions stand. - ImProver's 100% is on the paper's accuracy metric and holds by construction (fallback to unchanged input); now stated as such - isCompact_union does not exist in Mathlib; example is isCompact_iUnion - library-design.md now distinguishes rfl-proved API lemmas (correct) from downstream proofs needing rfl (the smell) - plugin README Contents paths fixed to be relative to the skill dir Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address second-round github-actions review findings Eval harness: - case 05 gains an engagement criterion: the review must name specific declarations from the file, so an empty "looks fine" can no longer score 5/5 by omission - the grader self-test is now two-sided: the canned bad review must fail every criterion (catches a permissive judge) and a new canned good review must pass every criterion (catches an over-strict judge) - case 01 scores the native_decide in the Fact instance; it was the most severe flaw in the fixture and previously unscored - checksum_tree excludes .claude/ so both arms fingerprint the same file set - runtime check on the isolation guard: a baseline transcript that mentions writing-lean-proofs fails the case as contaminated - results stamp includes the PID so same-second runs cannot collide - case 04: mod_add_carry_mul uses non-terminal simp [carry] (a bare simp risked "no progress"/goal-closing errors that would invalidate the criterion); rubric wording updated Skill content: - the sorry check had inverted exit status for CI use (grep exits 1 on no match); now ! grep, with the comment/docstring false-positive caveat and #print axioms alternative spelled out - anti-patterns.md documents native_decide (trust-base widening, caught by #print axioms, kernel/certificate alternatives) - llm-techniques.md sibling links demoted to plain-text mentions: AGENTS.md prohibits reference chains (file1 -> file2), which the earlier link conversion had introduced Verified: two-sided self-test passes (bad 0/7, good 7/7); live case 05 run scores baseline 2/6 vs skill 6/6 with no-rewrite passing in both arms and no contamination false-positive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat: add linting, performance, and tactics references to Lean skill * Make the eval harness's own checks able to fail Five review findings, all cases of a check that reports rather than gates. - no-rewrite gates the exit status. It was written, printed, and otherwise ignored: CASE_FAILURES only counted reviewer/grader *errors*, so a reviewer that started applying its own fixes under --permission-mode acceptEdits would rewrite every fixture, print no-rewrite:fail on every line, and still exit 0 — CI would read the run as green. - Add the skill arm's mirror of the baseline contamination backstop. cp -R succeeding proves the skill is on disk, not that the CLI discovered it. If project-skill discovery under --setting-sources project ever changes, the skill arm would run bare and both arms would score alike — which is indistinguishable from the true negative "the skill provides no uplift", the one conclusion this suite exists to measure. One canary call per run asks the CLI what it can see. - Guarantee the scratch tree is removed. run_case's body moved into run_case_body so the single rm -rf covers every early return; six paths (fixture copy, skill copy, both checksums, the reviewer call, the contamination check) previously leaked a fixture copy — plus a full skill copy on the skill arm — per case. - Run the offline half of --self-test before the CLI preflight, so the schema, isolation-fixture and rubric checks work with no `claude` on PATH and no authentication — which is where you would want them, e.g. a CI shell-test suite. - Count rubric criteria with the same pattern the Python capture uses. The shell count accepted `- id: foo bar`, which the capture drops, so a malformed line surfaced later as a confusing id-mismatch diff. Also anchor linting.md's warnings-as-errors CI snippet against vacuity: Lake caches per-module artifacts and linter warnings are emitted only on recompile, so on a restored cache build.log is empty, the grep matches nothing, and the gate reports clean over live warnings and sorries — the exact failure the same file preaches against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com> Co-authored-by: Marc Ilunga <marc.ilunga@trailofbits.com> |
||
|
Marcelo Morales
|
02b8a5eb8d | Add a workflow for porting Semgrep rules to other languages (#234) |