fix(exaforce): let LLM verdict override static severity floor - #15
Conversation
Upstream's meta-analyzer keeps CRITICAL/HIGH findings the LLM rejected, tagged llm-unconfirmed. In production that surfaces regex noise the LLM already dismissed. Emptying the floor entirely cost recall: the floor was mostly shielding the semantic analyzers' LLM findings from a second LLM pass, and the meta-analyzer vetoed CRITICAL findings on real droppers. Keep the floor only for LLM-backed findings (SQP/SDI/SSD, TP4); every static finding now follows the meta-analyzer verdict. Mode is selectable per call via SKILLSPECTOR_META_SEVERITY_FLOOR=none|semantic|upstream (default semantic). Same-day A/B/C on 188 borderline unit-scans: upstream 68 TP/10 FP, semantic 65 TP/7 FP, none 54 TP/3 FP. Four upstream TestApplyFilterSeverityFloor tests now fail by design; recorded in docs/superpowers/EXPECTED_TEST_FAILURES.md, left untouched to avoid upstream-sync conflicts.
smoy
left a comment
smoy
left a comment
There was a problem hiding this comment.
/code-review pass (high effort) on _filter_patches.py and its wiring. 7 findings inline: 4 I'd fix before merge, 3 nits.
What held up under verification, so this doesn't read as uniformly negative — I checked the core mechanics against upstream (meta_analyzer.py:376-505) and they're correct:
_HIGH_SEVERITY_FLOORis read viaself.,batch_resultsis a real list at the only call site (meta_analyzer.py:702), andFinding.finding_idis a UUID default (models.py:65-76) forwarded unchanged by both upstream branches — so the two-pass partition and the re-sort by input index are sound.- I hand-traced all four new tests' expected
kept/unconfirmedlists; all four match what the code produces. LLM_RULE_PREFIXES+TP4do cover every LLM-backed finding source in the tree today (threeLLMAnalyzerBasesemantic analyzers plusmcp_tool_poisoning.py:826); no other node callsget_chat_model/chat_completion.- Failed and never-returned batches genuinely never reach
apply_filter(llm_analyzer_base.py:719-740routes them tofailures), and the ledger (meta_analyzer.py:523) tolerates arbitrary drops — so the docstring's claims there are accurate. - Instance-attribute shadowing is safe from races: the analyzer is constructed per node call, and the benchmark harness uses
ProcessPoolExecutor.
The one that matters most is the empty-verdict fail-open: it reintroduces a false-negative path in a security gate, and it interacts directly with the decode-runaway failure mode #14 was written to suppress. Both it and the is_llm_finding discriminator are small changes that don't touch the measured behaviour.
|
|
||
| def _warn_on_empty_verdicts(batch_results: Any) -> None: | ||
| for batch, llm_items in batch_results: | ||
| if batch.findings and not llm_items: |
There was a problem hiding this comment.
Fail-open when the meta-analyzer returns a valid-but-empty verdict list — worth fixing before merge.
A batch that returns MetaAnalyzerResult(findings=[]) is counted in outcome.successful, so it never reaches the no-verdict fallback at meta_analyzer.py:712. Under the new default semantic mode every static finding for that file is then dropped.
Concrete scenario: the meta-analyzer hits its output cap or emits a degenerate decode that still validates against the pruned schema — exactly the failure mode frequency_penalty=0.1 (#14) was added to suppress. A file whose static analyzers flagged a CRITICAL E2 / YARA dropper match is reported clean, where upstream would have kept it tagged llm-unconfirmed. _warn_on_empty_verdicts only logs it.
This case is cleanly distinguishable from a normal per-finding rejection, so treating batch.findings and not llm_items as a batch failure — routing those findings to _fallback_filtered — would preserve fail-closed behaviour without giving up any of the measured FP reduction.
|
|
||
| def is_llm_finding(finding: Finding) -> bool: | ||
| rule_id = (finding.rule_id or "").strip().upper() | ||
| return rule_id in LLM_RULE_IDS or rule_id.startswith(LLM_RULE_PREFIXES) |
There was a problem hiding this comment.
is_llm_finding keys off free-form LLM output when a reliable discriminator exists — worth fixing before merge.
The module docstring asserts "Finding carries no source-analyzer field, so the rule id is the only stable discriminator". That isn't the case: LLMFinding.to_finding (llm_analyzer_base.py:146-158) sets neither category nor tags, while every static finding gets category = (af.tags[0] if af.tags else None) or get_category(af.rule_id) (static_runner.py:292-307), and TP4 sets category=_CATEGORY explicitly.
rule_id is never validated or normalized anywhere — it's only prompt-instructed (e.g. semantic_security_discovery.py:47). Scenario: the semantic analyzer emits SSD1, SSD_1, or Semantic-Prompt-Injection for a CRITICAL prompt-injection hit; the meta-analyzer omits it; is_llm_finding returns False, the finding lands in the unfloored pass and is dropped. That's the exact one-LLM-overrules-another recall loss this PR exists to prevent, and it's invisible in the logs.
finding.category is None or rule_id == "TP4" fails in the safe direction: an unknown static rule with no tags and no category mapping gets the floor rather than losing it.
| return # already applied | ||
| # The per-call shadowing above only works if upstream reads the floor | ||
| # through the instance. Fail at import time if that access path changes. | ||
| if "self._HIGH_SEVERITY_FLOOR" not in inspect.getsource(current): |
There was a problem hiding this comment.
inspect.getsource can raise OSError and take down import skillspector — worth fixing before merge.
apply() runs at import time from skillspector/__init__.py:41. inspect.getsource raises OSError: could not get source code whenever the .py is unavailable — zipimport, a PyInstaller/frozen build, a .pyc-only deployment. That propagates out of import skillspector as a bare OSError, killing the CLI at startup with an error that names neither the fork nor the patch.
Every other guard in this codebase raises PatchDriftError (_patchlib.py:19), and _patchlib deliberately uses inspect.signature, never getsource. Wrap the call in try/except OSError and either skip the source check or re-raise as PatchDriftError.
|
|
||
| Each fails on `assert len(result) == 1` / a missing `llm-unconfirmed` tag — | ||
| not an import/collection error. Note `.github/workflows/ci.yml` runs the full | ||
| suite via `make test-ci`, so fork CI is red by design (8 failures). Do not deselect, xfail, or |
There was a problem hiding this comment.
This doubles a red-by-design CI gate rather than making the expectation machine-checkable — worth fixing before merge.
.github/workflows/ci.yml:91 gates on uv run make test-ci, which is pytest -m "not integration and not provider" ... tests/ (Makefile:105-106) with no deselection. So the test job goes from 4 permanent failures to 8 and can never pass — meaning this PR's own change cannot be validated by CI, and a genuine regression in test_llm_analyzer_base.py would be indistinguishable from the expected noise.
The stated fork policy (don't deselect/xfail/edit upstream tests, to avoid sync conflicts) is satisfiable without touching upstream test source: a fork-owned root conftest.py with a pytest_collection_modifyitems hook that attaches xfail(strict=True) to exactly these 8 node ids keeps CI green, keeps upstream files byte-identical, and — via strict — additionally alerts you if an upstream sync ever makes one of them pass again. That last case is one the doc-only approach cannot detect.
| """ | ||
|
|
||
| @functools.wraps(original) | ||
| def apply_filter(self: Any, findings: list[Finding], batch_results: Any) -> list[Finding]: |
There was a problem hiding this comment.
Nit: batch_results is widened to Any and now consumed three times.
Upstream's signature is list[tuple[Batch, list[dict[str, Any]]]]; the wrapper annotates it Any and then iterates it in _warn_on_empty_verdicts plus passes it to original twice in semantic mode.
Today's sole caller passes a list, so this is latent. But if any future caller or test passes a generator or a zip/map object, the warn pass exhausts it and both original calls see zero verdicts — silently dropping every finding rather than raising. Keep upstream's list[...] annotation, or batch_results = list(batch_results) once at the top.
| self._HIGH_SEVERITY_FLOOR = frozenset() | ||
| kept.extend(original(self, unfloored, batch_results)) | ||
| finally: | ||
| self.__dict__.pop("_HIGH_SEVERITY_FLOOR", None) |
There was a problem hiding this comment.
Nit: the finally clears instance state it did not necessarily own.
self.__dict__.pop("_HIGH_SEVERITY_FLOOR", None) unconditionally deletes the instance attribute, so an LLMMetaAnalyzer that had a legitimately configured per-instance floor before the call silently reverts to the class value afterwards. Saving and restoring the prior instance value (sentinel + restore) is the same amount of code and has no downside.
| All four fail with an `AssertionError` (or `KeyError`) about a pruned key | ||
| (`explanation`, `intent`) being absent — not an import/collection error. | ||
|
|
||
| ## Severity floor (added 2026-09-02, `exaforce/_filter_patches.py`) |
There was a problem hiding this comment.
Nit: the new section is spliced into the middle of the schema-pruning section, leaving the surrounding prose wrong.
The intro ("These upstream tests ... assert the un-pruned schema") and "All four fail with an AssertionError ... about a pruned key" now sit above a section describing four failures that are about neither the schema nor a pruned key. The trailing "Confirmed bounded to these two files" paragraph and its code fence now follow the new section, reading as if they describe the severity-floor group.
Separately, the updated totals line changes 34 deselected, 6 xfailed → 38 deselected, 4 xfailed, which this PR does not touch. Those two numbers moving suggests the count was captured under a different invocation or after an unrelated sync, so the block no longer corroborates the documented failure set.
Move the new section below the existing one, or give each group its own captured summary.
Addresses the four pre-merge findings from the code review of PR #15. 1. Empty verdict lists no longer fail open. A batch that returns MetaAnalyzerResult(findings=[]) counts as successful, so it never reaches upstream's no-verdict fallback; with the floor lifted every one of its findings was dropped. A truncated or degenerate decode that still validates against the pruned schema — the failure mode frequency_penalty=0.1 was added to suppress — would therefore report a file with a CRITICAL dropper match as clean. Those findings now keep the upstream floor: CRITICAL/HIGH retained and tagged llm-unconfirmed, MEDIUM/LOW dropped, identical to `upstream` mode, and the event is logged. Deciding it on the floor rather than diverting to _fallback_filtered keeps the fork's change confined to the floor — _fallback_filtered also *keeps* MEDIUM/LOW findings that upstream drops, which would add false positives in the case this patch exists to reduce. 2. is_llm_finding keys off Finding.category, not the rule id. Rule ids are free-form LLM output, never validated or normalized, so a semantic analyzer emitting SSD_1 or Semantic-Prompt-Injection lost the floor — the one-LLM- overrules-another recall loss this patch exists to prevent, and invisible in the logs. LLMFinding.to_finding (and the fork's pruned replacement) set no category, while analyzer_finding_to_finding always does, falling back to "Security" for an unmapped id. The prefixes are kept as a secondary signal; TP4 still matches by id since it carries a category. Both extra checks can only add the floor, so the failure direction is a retained false positive rather than a silent drop. 3. inspect.getsource no longer breaks `import skillspector`. It raises OSError under zipimport, a frozen build, or a .pyc-only deploy, and apply() runs at import time — so the CLI died at startup with an error naming neither the fork nor the patch. It now warns and applies the patch unverified. 4. CI is green instead of red by design. A fork-owned root conftest.py attaches xfail(strict=True) to the upstream tests the patches invert, at collection time. Upstream test files stay byte-identical, so an upstream sync still never conflicts in them, and strict makes the expectation machine-checkable in both directions: a real regression in those files still fails the run, and an XPASS flags an entry gone stale after a sync. Fixes 1 and 2 together mean the floor patch inverts no upstream assertion in the default `semantic` mode. Every fixture in upstream's TestApplyFilterSeverityFloor either builds a bare Finding with no category (LLM-backed by the discriminator above) or passes an empty verdict list, so all seven now pass — non-vacuously, still exercising the floored path. Only the opt-in `none` mode inverts one, marked conditionally. Tests: 1924 passed, 13 skipped, 38 deselected, 8 xfailed, 0 failed. Green in all three modes (`none`: 1923 passed / 9 xfailed). Fork tests 17/17, three new: the empty-verdict case, the category invariant the discriminator rests on, and import survival when getsource raises. ruff check, ruff format --check, and mypy clean on the changed files. The benchmark numbers in the PR body predate fix 2, so `semantic`-mode routing has shifted slightly — static findings with a None category now keep the floor. Worth a confirmation run before merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Steven Moy <smoy@exaforce.com>
Addresses the four pre-merge findings from the code review of PR #15. 1. Empty verdict lists no longer fail open. A batch that returns MetaAnalyzerResult(findings=[]) counts as successful, so it never reaches upstream's no-verdict fallback; with the floor lifted every one of its findings was dropped. A truncated or degenerate decode that still validates against the pruned schema — the failure mode frequency_penalty=0.1 was added to suppress — would therefore report a file with a CRITICAL dropper match as clean. Those findings now keep the upstream floor: CRITICAL/HIGH retained and tagged llm-unconfirmed, MEDIUM/LOW dropped, identical to `upstream` mode, and the event is logged. Deciding it on the floor rather than diverting to _fallback_filtered keeps the fork's change confined to the floor — _fallback_filtered also *keeps* MEDIUM/LOW findings that upstream drops, which would add false positives in the case this patch exists to reduce. 2. is_llm_finding keys off Finding.category, not the rule id. Rule ids are free-form LLM output, never validated or normalized, so a semantic analyzer emitting SSD_1 or Semantic-Prompt-Injection lost the floor — the one-LLM- overrules-another recall loss this patch exists to prevent, and invisible in the logs. LLMFinding.to_finding (and the fork's pruned replacement) set no category, while analyzer_finding_to_finding always does, falling back to "Security" for an unmapped id. The prefixes are kept as a secondary signal; TP4 still matches by id since it carries a category. Both extra checks can only add the floor, so the failure direction is a retained false positive rather than a silent drop. 3. inspect.getsource no longer breaks `import skillspector`. It raises OSError under zipimport, a frozen build, or a .pyc-only deploy, and apply() runs at import time — so the CLI died at startup with an error naming neither the fork nor the patch. It now warns and applies the patch unverified. 4. CI is green instead of red by design. A fork-owned root conftest.py attaches xfail(strict=True) to the upstream tests the patches invert, at collection time. Upstream test files stay byte-identical, so an upstream sync still never conflicts in them, and strict makes the expectation machine-checkable in both directions: a real regression in those files still fails the run, and an XPASS flags an entry gone stale after a sync. Fixes 1 and 2 together mean the floor patch inverts no upstream assertion in the default `semantic` mode. Every fixture in upstream's TestApplyFilterSeverityFloor either builds a bare Finding with no category (LLM-backed by the discriminator above) or passes an empty verdict list, so all seven now pass — non-vacuously, still exercising the floored path. Only the opt-in `none` mode inverts one, marked conditionally. Tests: 1924 passed, 13 skipped, 38 deselected, 8 xfailed, 0 failed. Green in all three modes (`none`: 1923 passed / 9 xfailed). Fork tests 17/17, three new: the empty-verdict case, the category invariant the discriminator rests on, and import survival when getsource raises. ruff check, ruff format --check, and mypy clean on the changed files. The benchmark numbers in the PR body predate fix 2, so `semantic`-mode routing has shifted slightly — static findings with a None category now keep the floor. Worth a confirmation run before merge. Signed-off-by: Steven Moy <smoy@exaforce.com>
Both violations are pre-existing on main, in fork-owned files, and predate the severity-floor work — they are what keeps the `lint` CI job red: - I001 in src/skillspector/__init__.py:40 (from e6ac14c) — ruff wants a blank line after the deliberately-late `exaforce` import. No reordering; the import stays below the graph import and the warning-filter setup, and `apply_patches()` still runs last. - UP037 in src/skillspector/exaforce/_schema_patches.py:18 — the quoted annotation is unnecessary under `from __future__ import annotations`, which the module already has. Both are ruff --fix output, applied verbatim. `make lint` and `make format-check` now pass. Verified the patch layer still activates on import (apply_filter wrapped, schema keys pruned, to_finding leaves category unset) and fork tests are 17/17. Signed-off-by: Steven Moy <smoy@exaforce.com>
dfaa6c6 to
249f2d6
Compare
2 participants
Summary
PE3,P1,YR4,TP4,E2,RA1, …) on benign skills.SQP-*,SDI-*,SSD-*) andTP4, whichmcp_tool_poisoningderives from achat_completionreply. Upstream's floor was mostly shielding those first-pass LLM verdicts from a second LLM pass; removing it wholesale let the meta-analyzer veto CRITICAL findings on real droppers andexec()payloads.SKILLSPECTOR_META_SEVERITY_FLOOR=none|semantic|upstream(defaultsemantic), read on everyapply_filtercall so it can be flipped at runtime for in-process A/Bs as well as per benchmark run.src/skillspector/exaforce/; no upstream-tracked file is modified.Details
Why the floor is not simply removed. Every analyzer's findings flow into the meta-analyzer, whose prompt labels them all "static analysis findings". Corpus-wide on MalSkillBench the semantic rules produce ~2× as many HIGH/CRITICAL findings as all static rules combined, so upstream's floor (
src/skillspector/nodes/meta_analyzer.py:376, applied at:455) was primarily an LLM-vs-LLM tiebreak. Same-day controlled re-scan of the 94 borderline units a full 900-unitnonerun got wrong, two replicates each (188 unit-scans per arm),nvidia.nemotron-super-3-120b:upstreamsemantic(this PR's default)noneSpot-checked
nonelosses included a base64 PowerShell download-and-execute and Fernet-decryptedexec()insetup.py, flagged CRITICAL at confidence ≥ 0.9 by the semantic analyzers and then dropped by the meta-analyzer. Spot-checkedsemanticdrops of staticRA1/AS1hits were 11/12 regex misfires on benign text ("never modify your own rules", tutorial heredocs), including on malware units whose payload lived elsewhere.Patch.
apply()insrc/skillspector/exaforce/_filter_patches.pywrapsLLMMetaAnalyzer.apply_filteronce at import. Each call resolves the mode and dispatches:upstreamcalls through;noneshadowsself._HIGH_SEVERITY_FLOORwith an empty set for one call;semanticruns upstream's filter twice — floor on for LLM-backed findings, floor off for static — then restores input order byfinding_id. Drift guards raisePatchDriftErrorat import if the floor's value changes,apply_filterdisappears, or upstream stops reading the floor viaself.(the shadowing depends on that access path).Findingcarries no source-analyzer field, sois_llm_findingmatches normalized rule ids (LLM_RULE_PREFIXES+LLM_RULE_IDS); the benchmark corpus shows rare LLM-emitted variants likessd-2, hence the case-insensitive strip. A warning is logged when a batch returns zero verdicts for a file that had findings, since undernone/semanticthat drops the file's static findings where upstream kept CRITICAL/HIGH.Tests.
tests/exaforce/test_patches.pyruns one subprocess per mode against a mixed static/LLM batch (explicit denial, omission, confirmation, plus an omittedTP4), and one that flips the env var at runtime and checks instance state is restored. Four upstream tests inTestApplyFilterSeverityFloornow fail by design (they assert the static floor), on top of the four schema-pruning tests that already did;docs/superpowers/EXPECTED_TEST_FAILURES.mdis updated. Per fork policy those upstream tests are left untouched — no deselect/xfail — so CI stays red by design.Tests: 1917 passed, 8 failed by design (
uv run pytest);ruff check,ruff format --check, andmypyclean on the new module.Implementation Plan
Context. Fork policy is "trust the LLM over static". Upstream's CRITICAL/HIGH floor in the meta-analyzer keeps static findings the LLM rejected, producing noise in production. A first attempt emptied the floor entirely; a 900-unit benchmark showed −24 TP / +8 TN, and investigation traced the recall loss to the meta-analyzer overruling the semantic analyzers rather than to static rules.
Approach. Keep the floor only where it arbitrates between two LLMs; drop it wherever a static rule disagrees with the LLM. Expose the three policies as an env-selected mode so the benchmark can run controlled arms.
src/skillspector/exaforce/_filter_patches.pyapply_filterwrapper, drift guardssrc/skillspector/exaforce/__init__.py_filter_patches.apply()tests/exaforce/test_patches.pydocs/superpowers/EXPECTED_TEST_FAILURES.mdVerification. Fork tests 14/14; mypy clean on the new module; a fresh-context
/code-reviewpass produced 8 findings, all addressed (LLM-backedTP4coverage, per-call mode resolution, source-access drift guard, empty-verdict warning, mypy) or explicitly declined (xfail of upstream tests — conflicts with fork policy). Full suite 1917 passed / 8 by-design upstream failures. Same-day benchmark A/B/C on--from-run <head> --failures-only, two replicates per arm (runs864ccf78be3b,a5c5a221a8c6,f5aaf9771d20,2310deadaad9,729b41e62cfc,838e2a2288ccinbenchmark/benchmark.db). Still outstanding: a same-day full 900-unitsemanticvsupstreampair before treating the magnitude as settled.