2 Commits

Author	SHA1	Message	Date
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>	2026-08-19 13:39:07 -04:00
Andrea Cappa (zi0Black)	cfe5d7b161	Rust review plugin (#178) ... * rust-review: add Rust security review plugin Add the rust-review plugin: a comprehensive Rust security review skill with clustered finders covering memory safety, concurrency/data races, panic-induced DoS, FFI/cross-language boundaries, error handling, resource handling, async runtime, and static hygiene. Includes worker, dedup-judge, fp-judge, and planner agents, SARIF generation with rule descriptions and regression tests, deterministic cluster chunking, and Codex skills mapping. Versioned at 1.0.0 and registered in the marketplace, CODEOWNERS, and root README. * c-review: backport rust-review protocol fixes and planner chunking Port the language-agnostic fixes made while building rust-review (which was ported from c-review) back into c-review: - worker/fp-judge: force findings, coverage gate, and REPORT.md to disk via Write instead of returning content in the reply (orchestrator context-bloat hardening); add a pre-complete file-existence check. - worker: move the cache-primer block below the normal self-check and pre-work budget so a non-primer worker does not start under a global "no tool calls" rule. - planner: add --max-passes-per-worker (default 4) with deterministic split_oversized_clusters chunking; skill passes the flag and documents the chunked-subset worker rule. - scripts: add test_split.py and test_generate_sarif.py regression tests. The SARIF test caught a missing RULE_DESCRIPTIONS entry for uninitialized-data, now added. Bump c-review to 1.2.0. * c-review/rust-review: validate artifacts, index-aware SARIF, protocol cleanups - Add validate_artifacts.py (+ tests) to both plugins to check worker shard, coverage, and finding files before accepting completions. - generate_sarif.py now reads the canonical findings-index.txt when present, falling back to findings/*.md only if the index is absent. - Merge the worker step-6 verification paragraphs and drop orchestrator -internal Phase 7 / plan.json jargon in favor of worker-facing stakes. - Tighten uninitialized-read-finder guidance: primitive integers still require initialization. * rust-review/c-review: per-cluster max_passes_per_worker override Lets output-heavy clusters declare a smaller manifest-level max_passes_per_worker so each expensive pass group gets its own worker, validated by a single shared cluster_max_passes_per_worker helper and honored by split_oversized_clusters via an explicit override (0 is rejected rather than silently falling back to the global cap). rust-review opts in concurrency-locking and recursion-dos; c-review ports the capability for parity. validate_artifacts now accepts grouped or repeated --claimed-count values. * rust-review: broaden bug-class coverage with capability-gated clusters Add layout-safety, input-os-safety, and info-disclosure clusters behind new has_packed_repr / has_fs_io capability gates so packed-repr, path, and pointer-exposure passes only run where they apply, and gate unsafe-only passes behind has_unsafe to cut noise on safe crates. Extend existing clusters with new bug classes: RefCell double-borrow panics, unflushed BufWriter, string-comparison bypasses, serialize_struct mismatches, nondeterminism, in-collection key mutation, and destructor-skip cleanup leaks. Fix detector regexes that missed or over-matched real Rust (packed-field borrows, RefCell try_borrow_mut, HashMap substrings, path push, packed inner attrs, fs/path probes) and add a regression test pinning them to snippets. * fix dedup * safety-net check for REPORT.md * on-disk data -> shards reconciliation * on-disk data -> shards reconciliation - v2 * ls -> glob * memory-safety gate * path validation * fix numbers/counting * rm PACKEDREF from FFI cluster prompt, it is in layout-safety * fix unsafe-boundary count * minor fixes for prompts * do not filter unknown-severity findings, just mark them as such * fix minor behavior changes in worker * Correctness: - generate_sarif: clamp startLine >=1 (`:0` produced schema-invalid SARIF) - generate_sarif: don't drop a judged survivor with blank severity - dedup-judge: Tier-2 carry-forward so a primary can't be demoted/orphaned - dedup-judge: crash-recovery unions shards with findings/*.md (empty-shard trap) Robustness: - generate_sarif: skip frontmatter-less files; add originalUriBaseIds Contracts: - SKILL: gate dedup-judge before fp-judge (prevent concurrent-spawn race) - worker: verbatim coverage cells; sub_prompt_paths omitted-not-empty; skip_subclasses reserved; Codebase comma format * improve prompts regexes, add missing deconflictions * prompt factual fixes * fix dozen of small prompt inconsistencies and add missing sections * more prompt fixes, fix retry guard in SKILL, small fixes in agents * dozen more small fixes * final regex fixes * fixes from rust to c-review * agents cannot use write tool for reports (strange cc limitation) - bypass via bash * spawnings agents is capped to 20 - explicit handling for that * fix glob -> read (glob is blocked for agents that has also bash) * fix regex patterns to work with grep * soften output requirements - they were violated anyway * consolidated clusters are no longer chunked — one worker owns the whole cluster, builds its shared Phase-A inventory once, and runs every phase * fix judge finding counting and low-severity guidance * fix metadata * small fix for skipped findings * Carry forward guard for `also_known_as` bucket * Gracefully handle parse_frontmatter error * Extend has_ffi coverage * Broader gate for has_concurrency * Update FFI-safe layout regex to support C, C+packed, and C+u32 in unsafe-boundary and dyn-trait-ffi-finder prompts * Small refine of regex patterns * Improve regex patterns for recursive type detection to include Mutex and RwLock * rm global .codex/rust-review * backport fixes to c-review * merge changes * Backport SARIF merge-survivor + malformed-frontmatter guards to c-review, mark missing locations, fix prompt-regex test extractor, and harden planner/validator scripts across both review plugins * fix pytest * fix global gitignore, adds / and ruff_cache * small fixes from pr-review * small fixes from pr-review - 2 * fix copilot finding --------- Co-authored-by: GrosQuildu <e2.8a.95@gmail.com>	2026-06-30 11:14:46 -04:00