diff --git a/benchmark/README.md b/benchmark/README.md index c0f2312f0..33be26a62 100644 --- a/benchmark/README.md +++ b/benchmark/README.md @@ -35,7 +35,7 @@ Or from inside this directory: cd benchmark uv run benchmark /path/to/MalSkillBench/Dataset uv run benchmark .../Dataset/Prompts/indirect-injection -o out.duckdb -uv run benchmark .../Dataset/Skills/malware --limit 50 --workers 8 +uv run benchmark .../Dataset/Skills/malware --limit 50 --workers 16 uv run benchmark .../Dataset --no-llm # static analysis only ``` @@ -70,7 +70,7 @@ uv run remove-run results.duckdb ab12 9f3 c7 --yes # several at once, no promp | `--no-llm` | static analysis only (no LLM calls) | | `--categories` | comma-separated unit categories to scan; default `skill,code`. **Prompts are excluded by default** — they're raw prompt-injection samples, not the repo config files this tool classifies (PI embedded in a skill is still covered via the `skill` units). Pass `--categories skill,code,prompt` to include them. | | `--limit N` | cap units **per group** — a unit's parent directory (e.g. `--limit 20` on `Dataset/Skills` keeps 20 from `Skills/malware` *and* 20 from `Skills/benign`); 0 = no cap | -| `--workers N` | concurrent scan processes (default 8) | +| `--workers N` | concurrent scan processes; default is sized from the machine (half the RAM at 256 MB per worker, 6 per CPU, capped at 96 -- 96 on a 48 GB / 16-core Mac). Measured steady-state on the same units: 32 workers ~2 units/s, 64 ~2.6-3.0, 96 ~3.7, 128 ~3.1 -- past ~96 Bedrock queues calls (p99 wait ~46s) and slow scans approach `--timeout`. If the run summary reports scans that **lost LLM calls**, the provider is throttling: lower it. | | `--run-id` | extend an existing run instead of starting a new one (requires `-o`); re-scans only its unfinished units and appends into the same run | | `--from-run` | re-scan the units of a previous run instead of a `DATASET_PATH` (requires `-o`; reads that run from the `-o` DB and appends a new run there). Omit the positional path. | | `--failures-only` | with `--from-run`, re-scan only that run's **failed** units (FN, FP, timeouts, and errors) | diff --git a/benchmark/benchmark/config.py b/benchmark/benchmark/config.py index 5a38ae714..aaea17963 100644 --- a/benchmark/benchmark/config.py +++ b/benchmark/benchmark/config.py @@ -12,10 +12,25 @@ MODEL_ID = "nvidia.nemotron-super-3-120b" SCAN_TIMEOUT_SECONDS = 300 +# Auto --workers sizing, measured 2026-10-26 on a 48 GB / 16-core Mac: a scan peaks +# ~150 MB RSS (the largest units ~1 GB) and 96 workers use ~5 cores. Above ~96, +# Bedrock starts queueing calls (p99 wait 46s at 128) and throughput drops. +WORKER_RAM_BUDGET_BYTES = 256 * 2**20 +WORKERS_PER_CPU = 6 +MAX_AUTO_WORKERS = 96 + # SkillSpector's own "do not install" verdict == this recommendation string. MALICIOUS_RECOMMENDATION = "DO_NOT_INSTALL" +def default_workers() -> int: + """Scan processes this machine can run: the tighter of the RAM and CPU caps.""" + ram = os.sysconf("SC_PAGE_SIZE") * os.sysconf("SC_PHYS_PAGES") + by_ram = ram // 2 // WORKER_RAM_BUDGET_BYTES + by_cpu = (os.cpu_count() or 1) * WORKERS_PER_CPU + return max(1, min(by_ram, by_cpu, MAX_AUTO_WORKERS)) + + def bedrock_base_url(region: str) -> str: """Bedrock endpoint for ``region`` (keeps the URL in sync with an ``AWS_REGION`` override, which also drives the minted token's region). diff --git a/benchmark/benchmark/db.py b/benchmark/benchmark/db.py index 3653d34e4..0cf11c4e6 100644 --- a/benchmark/benchmark/db.py +++ b/benchmark/benchmark/db.py @@ -52,7 +52,9 @@ run_time DOUBLE, status TEXT, error TEXT, - num_issues INTEGER + num_issues INTEGER, + llm_calls_attempted INTEGER, + llm_calls_succeeded INTEGER -- < attempted: some LLM batches were dropped (e.g. 429s) ); CREATE TABLE issues ( run_id TEXT, @@ -109,6 +111,12 @@ """ +# Columns added after the first schema; ALTERed onto older DBs when they are opened. +_MIGRATIONS = ( + "ALTER TABLE classifications ADD COLUMN IF NOT EXISTS llm_calls_attempted INTEGER", + "ALTER TABLE classifications ADD COLUMN IF NOT EXISTS llm_calls_succeeded INTEGER", +) + # Every table keyed by run_id -- deleting a run means clearing its rows in all of # them (there is no FK cascade in the schema). _RUN_TABLES = ("runs", "units", "classifications", "issues", "components") @@ -174,6 +182,8 @@ def open_db(path: pathlib.Path, run_id: str | None): con.execute(_SCHEMA) con.execute(_EVAL_VIEW) return con, set(), False + for stmt in _MIGRATIONS: + con.execute(stmt) if run_id is None: return con, set(), False # append a new run into the existing DB if not con.execute("SELECT 1 FROM runs WHERE run_id = ? LIMIT 1", [run_id]).fetchone(): @@ -271,7 +281,9 @@ def record_result(con, run_id: str, res: ScanResult, no_llm: bool) -> None: ok = res.scan_status == "ok" predicted = (res.risk_recommendation == MALICIOUS_RECOMMENDATION) if ok else None con.execute( - "INSERT INTO classifications VALUES (?,?,?,?,?,?,?,?,?,?)", + "INSERT INTO classifications (run_id, unit_path, classification, is_malicious, " + "risk_score, risk_severity, run_time, status, error, num_issues, " + "llm_calls_attempted, llm_calls_succeeded) VALUES (?,?,?,?,?,?,?,?,?,?,?,?)", [ run_id, u.unit_path, @@ -283,6 +295,8 @@ def record_result(con, run_id: str, res: ScanResult, no_llm: bool) -> None: classification_status(res, no_llm), res.error_message, res.num_issues, + res.llm_calls_attempted, + res.llm_calls_succeeded, ], ) for idx, issue in enumerate(res.issues): @@ -337,3 +351,14 @@ def print_summary(con, run_id: str) -> None: total = tp + fp + tn + fn if total: print(f" accuracy = {(tp + tn) / total:.3f} (over {total} scored units)") + partial = con.execute( + "SELECT count(*) FROM classifications WHERE run_id = ? " + "AND llm_calls_succeeded < llm_calls_attempted", + [run_id], + ).fetchone()[0] + if partial: + print( + f" {partial} scan(s) lost some LLM calls (llm_calls_succeeded < " + "llm_calls_attempted); a jump here vs. the base run suggests throttling -- " + "lower --workers" + ) diff --git a/benchmark/benchmark/main.py b/benchmark/benchmark/main.py index a17032f1d..99c906937 100644 --- a/benchmark/benchmark/main.py +++ b/benchmark/benchmark/main.py @@ -9,7 +9,7 @@ Usage: benchmark /path/to/MalSkillBench/Dataset - benchmark .../Dataset/Skills/malware --limit 50 --workers 8 + benchmark .../Dataset/Skills/malware --limit 50 --workers 16 benchmark .../Dataset --no-llm # static analysis only benchmark .../Dataset -o out.duckdb --run-id abc123 # extend run abc123 report out.duckdb # single-run summary @@ -31,7 +31,7 @@ import click from tqdm import tqdm -from .config import SCAN_TIMEOUT_SECONDS, configure_run +from .config import SCAN_TIMEOUT_SECONDS, configure_run, default_workers from .dataset_handler import discover from .db import ( delete_runs, @@ -94,7 +94,13 @@ help="cap units PER GROUP (a unit's parent directory, e.g. Skills/malware " "vs Skills/benign); 0 = no cap", ) -@click.option("--workers", type=int, default=8, show_default=True, help="concurrent scan processes") +@click.option( + "--workers", + type=click.IntRange(min=1), + default=None, + help="concurrent scan processes (default: sized from this machine's RAM and CPUs, " + "capped at 96). Lower it if the summary reports scans that lost LLM calls.", +) @click.option( "--max-tasks-per-child", type=int, @@ -192,6 +198,9 @@ def _run_benchmark(args: SimpleNamespace) -> int: print("ERROR: --dataset only applies with --from-run") return 2 + if args.workers is None: + args.workers = default_workers() + gen_id = uuid.uuid4().hex[:12] out_path = pathlib.Path(args.output or f"benchmark_{gen_id}.duckdb").resolve() diff --git a/benchmark/benchmark/models.py b/benchmark/benchmark/models.py index 604e0d3ee..01418369a 100644 --- a/benchmark/benchmark/models.py +++ b/benchmark/benchmark/models.py @@ -54,6 +54,8 @@ class ScanResult: num_issues: int = 0 llm_requested: bool = False llm_available: bool = False + llm_calls_attempted: int | None = None + llm_calls_succeeded: int | None = None error_message: str | None = None issues: list[dict] = field(default_factory=list) components: list[dict] = field(default_factory=list) diff --git a/benchmark/benchmark/runner.py b/benchmark/benchmark/runner.py index 475ef36c0..c77abe33d 100644 --- a/benchmark/benchmark/runner.py +++ b/benchmark/benchmark/runner.py @@ -75,6 +75,8 @@ def _report_to_result(unit: Unit, report: dict, elapsed: float) -> ScanResult: num_issues=len(issues), llm_requested=bool(meta.get("llm_requested")), llm_available=bool(meta.get("llm_available")), + llm_calls_attempted=meta.get("llm_calls_attempted"), + llm_calls_succeeded=meta.get("llm_calls_succeeded"), issues=issues, components=report.get("components", []) or [], ) diff --git a/benchmark/tests/test_config.py b/benchmark/tests/test_config.py new file mode 100644 index 000000000..13e3a667e --- /dev/null +++ b/benchmark/tests/test_config.py @@ -0,0 +1,27 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Tests for the machine-sized --workers default.""" + +from __future__ import annotations + +from benchmark import config + + +def _machine(monkeypatch, ram_gib: int, cpus: int) -> None: + sizes = {"SC_PAGE_SIZE": 4096, "SC_PHYS_PAGES": ram_gib * 2**30 // 4096} + monkeypatch.setattr(config.os, "sysconf", sizes.__getitem__) + monkeypatch.setattr(config.os, "cpu_count", lambda: cpus) + + +def test_default_workers_is_capped_on_a_large_machine(monkeypatch): + _machine(monkeypatch, ram_gib=48, cpus=16) + assert config.default_workers() == config.MAX_AUTO_WORKERS + + +def test_default_workers_is_ram_bound_on_a_small_machine(monkeypatch): + _machine(monkeypatch, ram_gib=8, cpus=16) + assert config.default_workers() == 8 * 2**30 // 2 // config.WORKER_RAM_BUDGET_BYTES + + +def test_default_workers_is_cpu_bound_with_few_cores(monkeypatch): + _machine(monkeypatch, ram_gib=64, cpus=2) + assert config.default_workers() == 2 * config.WORKERS_PER_CPU diff --git a/benchmark/tests/test_db.py b/benchmark/tests/test_db.py index bd0df3c88..db1dcc7d1 100644 --- a/benchmark/tests/test_db.py +++ b/benchmark/tests/test_db.py @@ -61,3 +61,25 @@ def test_delete_runs_noop_on_empty_id_list(): _seed_run(con, "x") delete_runs(con, []) assert con.execute("SELECT count(*) FROM runs").fetchone()[0] == 1 + + +def test_open_db_adds_llm_call_columns_to_an_older_db(tmp_path): + from benchmark.db import open_db + + path = tmp_path / "old.duckdb" + con = duckdb.connect(str(path)) + con.execute( + _SCHEMA.replace(",\n llm_calls_attempted INTEGER", "").replace( + ",\n llm_calls_succeeded INTEGER -- < attempted: some LLM batches were dropped (e.g. 429s)", + "", + ) + ) + con.execute(_EVAL_VIEW) + assert "llm_calls_attempted" not in { + r[0] for r in con.execute("DESCRIBE classifications").fetchall() + } + con.close() + + con, _, _ = open_db(path, None) + cols = {r[0] for r in con.execute("DESCRIBE classifications").fetchall()} + assert {"llm_calls_attempted", "llm_calls_succeeded"} <= cols diff --git a/src/skillspector/exaforce/UPSTREAM_UPGRADE_HOLD.md b/src/skillspector/exaforce/UPSTREAM_UPGRADE_HOLD.md new file mode 100644 index 000000000..dcc350729 --- /dev/null +++ b/src/skillspector/exaforce/UPSTREAM_UPGRADE_HOLD.md @@ -0,0 +1,158 @@ +# Upstream upgrades: on indefinite hold + +**Decision (2026-10-26):** the fork stays on upstream NVIDIA/SkillSpector +**2.8.1**. Pulling 2.13.1 was attempted, benchmarked, patched, and +re-benchmarked; the best result still had **2x the false positives** and +**45 scan timeouts** against a base that had none, and the patched tree was +2.7x slower per scan. The pull is parked in a git stash (see the end of this +file). Nothing from it is on the branch. + +Everything below is from `benchmark/malskillbench_agentguard_seed.duckdb` +(4,844 units: 3,913 benign, 931 malicious; model +`nvidia.nemotron-super-3-120b` via bedrock-mantle; verdict = `DO_NOT_INSTALL`). + +## Runs + +| run | tree | correct | FP | FN | errors | precision | recall | F1 | avg scan | +|---|---|---|---|---|---|---|---|---|---| +| `efd838e2e656` | **base**: fork on upstream 2.8.1 | **0.876** | **187** | 415 | **0** | 0.734 | 0.554 | 0.632 | 20.5s | +| `ca1cf8d781dd` | pure upstream 2.13.1, no fork patches | 0.658 | 1241 | 257 | 157 | 0.314 | 0.688 | 0.431 | 99.0s | +| `3af1174df436` | 2.13.1 + fork patches re-applied as-is | 0.680 | 1347 | 166 | 35 | 0.361 | 0.821 | 0.501 | 49.7s | +| `aa767af2fb97` | 2.13.1 + fork patches reworked (below) | 0.851 | 376 | 303 | 45 | 0.624 | 0.673 | 0.648 | 55.2s | + +"correct" is (TP + TN) / 4,844 with errors counted as wrong. All 2.13.1 runs +used 96 workers; the base run's worker count was not recorded. `8ed7dfa5fc00` +was a 1,624-unit partial of the as-is tree and agrees with `3af1174df436`. + +The pure-upstream run shows the regression is upstream's, not the fork's: with +no fork code at all, 2.13.1 flags 1,241 benign skills where 2.8.1 flagged 187. + +## What changed upstream (2.8.1 → 2.13.1) and why it hurts + +Two independent changes, neither with an upstream opt-out. + +### 1. TP4 analyzes Markdown code fences (#421) + +TP4 is the LLM "description-behavior mismatch" check in +`nodes/analyzers/mcp_tool_poisoning.py`. Until 2.13 it ran only when a skill +shipped real executable files. Since #421 every `bash`/`python`/`js`/`ts`/… +fence in SKILL.md (and any other Markdown/text file) is extracted and sent to +the LLM, one batch per fence, as "the implementation". Benign skills are mostly +SKILL.md with example commands, so the LLM reports a mismatch per fence at +HIGH / ~0.92 confidence. + +| | base | 2.13.1 as-is | +|---|---|---| +| benign units with a TP4 HIGH finding | 37 | 2,183 | +| share of score points *added* to benign units | | 92 % | +| benign FPs after rescoring with TP4 removed (offline) | | 1,373 → 623 | + +Three TP4 occurrences alone contribute ~40 of the 51 points needed for +`DO_NOT_INSTALL`. This is also why scan time more than doubled: a live +20-unit check gave a median of 41s with fence scanning on and 10s with it off. + +### 2. The meta-analyzer no longer removes anything (#393) + +Before 2.13 the meta-analyzer LLM dropped findings it did not confirm +(MEDIUM/LOW outright; CRITICAL/HIGH kept on a floor). Since 2.13 it only +annotates: `report.py` scores `state["findings"]`, which merges by id and +never deletes ("Meta/LLM analysis can enrich canonical objects but cannot +remove deterministic findings"), and `finalize_ledger` re-selects every +finding. The fork's `_filter_patches.py` post-filter still ran, but its drops +never reached the score. Unconfirmed MEDIUM statics that 2.8.1 removed now all +count: on benign SKILL.md files E1 went 30 → 639 units, RA2 23 → 435, LP3 and +PE3 similarly. This is why the fork tree and pure upstream scored the same. + +## Fix attempts and outcomes + +### Attempt A: re-apply the fork's filter patch unchanged (`3af1174df436`) + +Rewritten `apply_filter` wrapper for 2.13's stamp-based review outcomes. All +unit tests passed; end to end it was a no-op for the reason in §2. Result +indistinguishable from pure upstream (1,347 vs 1,241 FP). + +### Attempt B: live isolation of the two causes (40 units, 4 configs) + +20 benign units the pull had flipped TN→FP and 20 malicious units it had +flipped FN→TP, each scanned under four configurations: + +| configuration | benign flagged /20 | malware caught /20 | +|---|---|---| +| 2.13.1 as-is | 15 | 17 | +| TP4 Markdown fences off | 9 | 7 | +| filter drops made effective | 9 | 14 | +| both | 4 | 7 | +| filter effective + TP4 follows meta verdict (offline) | 3 | 7 | +| filter effective + TP4 scored once per unit (offline) | 5 | 11 | + +The malware column is a worst case (these are exactly the units the new code +fixed), but it shows the trade: most of 2.13's recall gain comes from the same +fence scanning that causes the false positives. The meta-analyzer marked most +TP4 findings `missing` or `disagreed`; the fork's `semantic` floor kept them. + +### Attempt C: reworked patches (`aa767af2fb97`) + +1. `apply_filter` wrapper returns every finding in upstream's order but + **zeroes the confidence** of the ones it drops (tag `llm-rejected`). + `_compute_risk_score` skips confidence ≤ 0, so the drop reaches the verdict + while upstream's "always listed" contract holds. Verified by a graph-level + test that failed at score 34 before the change and 0 after. +2. **TP4 removed from the LLM severity floor**, so it follows the + meta-analyzer verdict like a static rule. + +Full suite green in all three floor modes (16 upstream tests that assert +"unconfirmed finding is retained" pass again under the new contract; 4 remain +by-design xfails). Benchmark vs base: + +| | base | attempt C | Δ | +|---|---|---|---| +| correct | 4242 | 4120 | −122 | +| false positives | 187 | 376 | +189 | +| false negatives | 415 | 303 | −112 | +| errors (all 300s timeouts) | 0 | 45 | +45 | +| precision | 0.734 | 0.624 | −0.110 | +| recall | 0.554 | 0.673 | +0.119 | +| F1 | 0.632 | 0.648 | +0.016 | +| avg / median / p95 scan | 20.5s / 14.2s / 64.9s | 55.2s / 28.1s / 237.3s | 2.7x / 2.0x / 3.7x | + +The residual 241 new false positives decompose as: + +- **TP4 the meta-analyzer confirms**: still on 355 benign units, top + contributor on 136 of the 241. Both LLMs agree, so no floor policy touches + it; only disabling fence scanning or capping TP4's contribution would. +- **Lost LLM batches**: 114 of the 376 benign FPs had + `llm_calls_succeeded < llm_calls_attempted`. A failed meta-analyzer batch + keeps every finding fail-closed at full weight (418 scans lost calls at 96 + workers; see the `llm-failure-amplification` note in the benchmark docs). +- **Timeouts**: 45 scans hit the 300s cap, all scored wrong. + +F1 is slightly up only because the harmonic mean rewards lifting the weaker +side (recall); on a corpus that is 81 % benign, overall correctness is down. + +## What would be needed to lift the hold + +Any of these would need its own controlled A/B against `efd838e2e656` +(same day, replicated; per-unit flips are noisy, judge aggregates only): + +- An upstream opt-out for TP4 fence scanning, or a fork patch that empties + `_TP4_MARKDOWN_TYPES` (restores 2.8.1 TP4 gating; cuts scan time ~2.5x; + gives up most of 2.13's recall gain). +- Capping TP4 to one scored occurrence per skill (attempt B's offline + estimate: FP 5/20, TP 11/20 on the hard sample). +- Fewer workers so batches stop being dropped, since dropped batches turn + directly into false positives under 2.13's fail-closed design. +- An upstream change to §2 that lets meta-analysis remove or down-weight + findings in the public result. + +Without at least the TP4 change the 2.13 line is slower and less precise than +what the fork ships today, and the recall gain does not pay for it. + +## Where the attempt lives + +The 2.13.1 pull plus attempt C (`_filter_patches.py`, `tests/exaforce/`, +root `conftest.py`, `docs/superpowers/EXPECTED_TEST_FAILURES.md`) is in +`git stash` on branch `bump-upstream-version`, entry +"upstream 2.13.1 pull + fork fix attempt C (on hold)". The benchmark runs +above are in the DuckDB and are not part of the stash. Fork-side diagnosis +notes are also in the Claude project memory under +`upstream-2-13-fp-regression-causes`.