mirror of
https://github.com/trailofbits/skills.git
synced 2026-09-14 14:28:48 +08:00
main
16 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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> |
||
|
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
|
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> |
||
|
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>
|
||
|
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> |
||
|
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> |
||
|
Lixin2026
|
d5fe2e6a78 |
feat(codex): add UI metadata for skills (#175)
* feat(codex): add skill UI metadata * Use official Trail of Bits logo * fix: resolve code review findings for PR #175 Codex silently drops the icons as authored: its loader (codex-rs/core-skills resolve_asset_path) requires icon paths containing '..' to resolve under <plugin_root>/assets/, and the repo-root .codex/assets location fails that containment check. Verified empirically via codex app-server plugin/read: every iconSmall/iconLarge came back null; only brand_color applied. P1 fixed: - Vendor trail-of-bits-mark.svg into plugins/<name>/assets/ for all 38 plugins with skills and point every openai.yaml at ../../assets/trail-of-bits-mark.svg (the supported plugin-level shared asset pattern). Icons now resolve for marketplace installs too, since nothing escapes the plugin root. - Drop the .codex/ additions: .codex/skills/gh-cli/agents/ openai.yaml resolved nowhere (.codex/skills is not a Codex discovery root) and PR #173 removes the whole .codex/ tree P2 fixed: - Patch-bump all 38 touched plugins in plugin.json and marketplace.json so installed clients pick up the metadata Verified: - Static check replicating Codex's resolution algorithm: all 73 yaml files resolve under their plugin assets/ and exist - Live codex app-server probe: 71/72 loadable skills report resolved iconSmall/iconLarge and brand_color #D83A34 (claude-in-chrome-troubleshooting fails to load on main due to a pre-existing 64-char qualified-name limit, fixed by #173's rename; zeroize-audit's manifest mcpServers object is likewise a pre-existing Codex incompatibility fixed by #173) - validate_codex_skills.py, validate_plugin_metadata.py, prek all pass Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(codex): use skill-local icon assets --------- Co-authored-by: Dan Guido <dan@trailofbits.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Jonathan Hefner
|
debfb29c8e |
Fix allowed-tools to use spec-compliant space-delimited strings (#139)
* Fix `allowed-tools` to use spec-compliant space-delimited strings Per the agentskills.io specification, `allowed-tools` must be a single string of space-delimited patterns, not a YAML list. Converted all 23 SKILL.md files from the `- Item` list format to the correct `"Item1 Item2"` string format. Also updated the frontmatter examples in CLAUDE.md and the workflow-skill-design skill template to match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix remaining allowed-tools format in firebase-apk-scanner and workflow-skill-design docs - Convert firebase-apk-scanner from comma-separated to space-delimited - Update anti-patterns.md and tool-assignment-guide.md examples from YAML lists to space-delimited strings - Remove unnecessary quotes from SKILL.md template placeholder Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Cover commands, new SKILL.md files, and fix template placeholder Extends the previous spec-compliance fixes: * Convert command frontmatter (commands/*.md) — per Claude Code docs, command files use the same frontmatter as skills, so the same space-delimited rule applies. * Convert three SKILL.md files added since the original PR: mutation-testing, trailmark-structural, trailmark-summary. * Fix the placeholder in the workflow-skill-design template. The previous "[minimum tools needed, space-delimited]" was YAML flow-sequence syntax, which parses as a list — the opposite of what the placeholder claims. Replaced with a concrete-looking space-delimited example plus a comment. Zeroize-audit agent files still use `allowed-tools:` in YAML list form. They are intentionally excluded: per the project's own docs (workflow-skill-design references), agents declare tools with `tools:` (not `allowed-tools:`). Fixing those requires changing the field name as well as the format and is out of scope for this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * zeroize-audit agents: switch allowed-tools to tools Subagents declare their tool allowlist via `tools:` (comma-separated), not `allowed-tools:` — see Claude Code's subagent docs and this repo's own designing-workflow-skills/SKILL.md:47: > Skills use `allowed-tools:` in frontmatter. Agents use `tools:` > in frontmatter. Before this change, the zeroize-audit agents declared their tool list under `allowed-tools:`, which Claude Code does not read for subagents. The field was effectively a no-op; the spawned agents had no tool restriction enforced. Renames the field on all 11 agents to `tools:` and reformats the YAML list as comma-separated to match the documented format and existing agents elsewhere in the repo (e.g. function-analyzer.md, spec-compliance-checker.md). Tool sets are unchanged. Behavior change: tools now actually constrain what each spawned agent can call. The lists are the ones the original author intended. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * skill-improver: convert command allowed-tools to space-delimited The two command files in plugins/skill-improver/commands/ still used the JSON flow-array format (`allowed-tools: ["..."]`), which the rest of this PR converted everywhere else. Convert them to the spec-compliant space-delimited string form for consistency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Dan Guido <dan@trailofbits.com> |
||
|
Paweł Płatek
|
17ba9fa3e9 |
Sast improve (#119)
* experimental rules in run all mode; more explicit gates/ask-user * bump version |
||
|
Paweł Płatek
|
8f92c6dee0 |
Simpler sast (#104)
* static-analysis selects all or important queries
* make semgrep/codeql better
* semgrep/codeql - descriptions
* semgrep - workflow plugin improvements
* semgrep - workflow plugin improvements 2
* codeql - workflow plugin improvements
* codeql - workflow plugin improvements 2
* output dir and artifacts more precise handling, better db discovery
* Fix review issues: grep -P on macOS, LANG collision, stale triage refs, error handling
- Replace grep -oP with sed in macOS arm64e workaround (BSD grep lacks -P)
- Rename LANG to CODEQL_LANG across all CodeQL files to avoid POSIX locale collision
- Remove stale triage description from semgrep-scanner agent
- Rename merge_triaged_sarif.py to merge_sarif.py and update all references
- Restore {baseDir} in semgrep-scanner agent and sarif-parsing skill paths
- Fix quoted heredoc in scan-workflow.md preventing $(date) expansion
- Fix suite file location ($RESULTS_DIR -> $RAW_DIR) to match workflow
- Quote $OUTPUT_DIR in extension-yaml-format.md cp commands
- Add SARIF ruleIndex portability note for non-CodeQL tools
- Split CodeQL install check into separate command -v and --version steps
- Add semgrep install check before Pro detection in scan-workflow
- Add rm -rf guards in scanner agent and task prompt
- Narrow OSError catch in merge script with specific exception types
- Track and report skipped files in pure Python SARIF merge
- Add variable guards (${CODEQL_LANG:?}) in suite generation scripts
- Separate neutralModel into own section with column definition
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Dan Guido <dan@trailofbits.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
|
||
|
Dan Guido
|
9600b3d9e6 |
Add semgrep-scanner and semgrep-triager agents to static-analysis (#80)
* Add semgrep-scanner and semgrep-triager agents to static-analysis plugin Introduces formal agent definitions for the scanning and triage workflows. Updates SKILL.md to reference agents and bumps version to 1.1.0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix {baseDir} paths and bump marketplace.json version Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: resolve code review findings for PR #80 - Update scanner-task-prompt.md to reference semgrep-scanner subagent type instead of Bash (consistent with SKILL.md Step 4 change) - Update triage-task-prompt.md to reference semgrep-triager subagent type instead of general-purpose (consistent with SKILL.md Step 5 change) - Add Agents Included section to README.md documenting new agents - Fix Agents table column header in SKILL.md from "Type" to "Tools" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use fully-qualified agent type names for semgrep subagents Plugin agents require the `plugin-name:agent-name` format at runtime, but the skill referenced bare names (`semgrep-scanner`, `semgrep-triager`) causing "Agent type not found" errors when spawning scan/triage Tasks. Also adds agent types to the Step 3 plan template and pre-scan checklist so they appear in generated plans and survive context clearing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Explicit semgrep scan allow --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: axelm-tob <axel.mierczuk@trailofbits.com> |
||
|
Paweł Płatek
|
f821e35473 |
codeql skills merge and improve (#67)
* init codeql merge * test and improve * test and improve 2 * test and improve 3 * test and improve 4, bump version, fix authors * Bump testing-handbook-skills version for codeql skill removal Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Dan Guido <dan@trailofbits.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> |
||
|
Paweł Płatek
|
51e02cc659 |
Merge and improve semgrep skills (#65)
* init merge and rewrite * semgrep done * fix semgrep version * Fix review issues in semgrep skill - Replace destructive `semgrep logout` with non-destructive Pro detection - Align marketplace.json description with plugin.json - Add subagent_type guidance (Bash for scanners, general-purpose for triage) - Use npx --no-install instead of --yes to prevent auto-installing packages - Fix ruff formatting on merge_triaged_sarif.py - Simplify brittle run number generation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix authors * mend --------- Co-authored-by: Dan Guido <dan@trailofbits.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> |
||
|
Dan Guido
|
fd367ad81b |
ci: add lint enforcement with ruff, shellcheck, and shfmt (#10)
* ci: add lint enforcement with ruff, shellcheck, and shfmt Add pre-commit hooks (prek) and GitHub Actions CI to enforce Python and shell linting across the repository. - Add root pyproject.toml with ruff configuration (line-length=100, py311) - Add .pre-commit-config.yaml with ruff, shellcheck, shfmt, and standard hooks - Add .github/workflows/lint.yml with SHA-pinned actions - Update .github/dependabot.yml with root pip ecosystem entry - Fix existing lint violations: - Auto-fix imports and formatting with ruff - Fix duplicate dict key in ct_analyzer (contentequals -> arrays.contentequals) - Add noqa comment for required sys.path manipulation in extract_pdf.py - Format shell scripts with shfmt (case statement indentation) - Add per-file-ignores for ct_analyzer's long opcode tables and style patterns Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: use 2-space indentation for shell scripts Change shfmt configuration from 4-space to 2-space indentation to match CLAUDE.md standards. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> |
||
|
Dan Guido
|
4f5139d01d |
Add static-analysis plugin from internal repo (#3)
* Add static-analysis plugin from internal repo Migrate the static-analysis plugin which provides: - CodeQL skill for deep security analysis with taint tracking - Semgrep skill for fast pattern-based security scanning - SARIF parsing skill with jq queries and Python helpers Author: Axel Mierczuk Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix hardcoded path in sarif-parsing SKILL.md Replace /home/user/proj/ with /path/to/project/ to pass the hardcoded path CI check. The CI excludes /path/to patterns. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> |