From 9e8c48cb47aa9dc7044779566632251fa69bff6c Mon Sep 17 00:00:00 2001 From: will-exaforce Date: Mon, 26 Oct 2026 15:21:17 -0500 Subject: [PATCH 1/2] feat(benchmark): size --workers from the machine and flag dropped LLM calls The fixed default of 8 workers left most of the machine idle. Measured on a 48 GB / 16-core Mac, throughput climbs to ~3.7 units/s at 96 workers and then falls as Bedrock starts queueing calls. The default is now the smallest of half the RAM at 256 MB per worker, 6 workers per CPU, and 96. The scan report already counts LLM calls attempted vs. succeeded. The harness now stores both counts, and the run summary warns when some scans lost calls, which is the throttling signal that means --workers should come down. --- benchmark/README.md | 4 ++-- benchmark/benchmark/config.py | 15 +++++++++++++++ benchmark/benchmark/db.py | 29 +++++++++++++++++++++++++++-- benchmark/benchmark/main.py | 15 ++++++++++++--- benchmark/benchmark/models.py | 2 ++ benchmark/benchmark/runner.py | 2 ++ benchmark/tests/test_config.py | 27 +++++++++++++++++++++++++++ benchmark/tests/test_db.py | 22 ++++++++++++++++++++++ 8 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 benchmark/tests/test_config.py 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 From c3cc13f2ad2f7a74bfdd30e1a2eac0c6f9ee8639 Mon Sep 17 00:00:00 2001 From: will-exaforce Date: Mon, 26 Oct 2026 15:23:06 -0500 Subject: [PATCH 2/2] docs(exaforce): record why upstream 2.13.1 upgrades are on hold --- .../exaforce/UPSTREAM_UPGRADE_HOLD.md | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 src/skillspector/exaforce/UPSTREAM_UPGRADE_HOLD.md 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`.